search

davidker / unisys

Master repository for new changes to drivers/staging/unisys and drivers/visorbus
Other
2 stars 1 forks source link

visorbus: gripes about kerneldoc function comments #60

Closed selltc closed 8 years ago

selltc commented 8 years ago

Source: Thomas Gleixner tglx@linutronix.de 5/18/2016:

Try to mimic kerneldoc comments, i.e. start with: /** but do not implement any of the kerneldoc requirements.

See KanBoard-1252.

ghost commented 8 years ago

branch: adjust_visorbus_func_comments; commits: 4b5a11d642608b1f7051aba6cabe850d399212ec, 3028fe3bf6e09736fbe715a6514283657ec58ee0, 62cfa1195fbcf61dacc3308de1efd1352dc35ebb, 72b5f18f2e919cf8ff25a115e058426c264ff0be, 3443add77a388868c3f903840068a88b9ccbe67a checkpatch-cleared: yes T710-1 verified: yes

selltc commented 8 years ago

I could be wrong, but it is my belief that what tglx was complaining about was the lack of standard structured kerneldoc comments. Have a peek at Documentation/kernel-doc-nano-HOWTO.txt in the kernel source tree, which has an example like this:

**
 * foobar() - short function description of foobar
 * @arg1:   Describe the first argument to foobar.
 * @arg2:   Describe the second argument to foobar.
 *      One can provide multiple line descriptions
 *      for arguments.
 *
 * A longer description, with more discussion of the function foobar()
 * that might be useful to those using or modifying it.  Begins with
 * empty comment line, and may include additional embedded empty
 * comment lines.
 *
 * The longer description can have multiple paragraphs.
 *
 * Return: Describe the return value of foobar.
 */

In other words, tglx wants to see:

But I could be wrong; I would be interested to hear the interpretation of others. Unfortunately, my interpretation would require a LOT more work. :-(

selltc commented 8 years ago

I should clarify what I said a bit. I do NOT think it is necessary that we provide kerneldoc comments for all functions, as indicated by this comment in Documentation/kernel-doc-nano-HOWTO.txt:

We definitely need kernel-doc formatted documentation for functions
that are exported to loadable modules using EXPORT_SYMBOL.

We also look to provide kernel-doc formatted documentation for
functions externally visible to other kernel files (not marked
"static").

We also recommend providing kernel-doc formatted documentation
for private (file "static") routines, for consistency of kernel
source code layout.  But this is lower priority and at the
discretion of the MAINTAINER of that kernel source file.

However, for functions that we deem important enough to document (as-per the above guidelines), we should always use kerneldoc comments using the recommended conventions.

ghost commented 8 years ago

I ultimately interpreted the feedback to ask for kerneldoc-style wrapping (i.e. begin with /** on its own line, and end with `*/ on its own line), but to leave out the kerneldoc style formatting within. But I also could be wrong, and would be interested in hearing other opinions.

For what its worth, I did begin implementing a full kerneldoc workup last week, should what I committed not be enough (see attachment):

kerneldoc.zip

ghost commented 8 years ago

More commits: branch: visorbus_kernel_doc; commits: 1013c601e6cd9024e7ecd2200dcc5348fb35a906, 6778a0739cd8735bb214772034a016a41cc60a82 checkpatch-cleared: yes T710-1 verified: (pending)

selltc commented 8 years ago

Comments on this from Thomas Gleixner tglx@linutronix.de 6/1:

That was not my feedback. If you start a comment with '/**' then it should be a kernel-doc function/struct documentation. If not then it starts with '/*'.

ghost commented 8 years ago

Per my comments above, 7 commits were made to the visorbus_kernel_doc branch. I verified that all 7 were committed to the upstream, and therefore need to be reconsidered: 4b5a11d642608b1f7051aba6cabe850d399212ec 3028fe3bf6e09736fbe715a6514283657ec58ee0 62cfa1195fbcf61dacc3308de1efd1352dc35ebb 72b5f18f2e919cf8ff25a115e058426c264ff0be 3443add77a388868c3f903840068a88b9ccbe67a 6778a0739cd8735bb214772034a016a41cc60a82 1013c601e6cd9024e7ecd2200dcc5348fb35a906

ghost commented 8 years ago

My corrections, per Thomas Gleixner's comments, can be found here. I also removed all other non-kerneldoc instances where /\ existed.

branch: correct_kerneldoc_comments; commits: b3856706ec43383ea83f4fcabbb44595de3c698b, 5676b78e7d6d4a7385f1568bdd047e68db487b08, ef019c0bb13ac4eb66fc3178e73c1f0e64e0ad27, c72cfe898abd27c4424666cc6721b1ce1b2b344c, 8d7746b7c8b7581240c87a80e004ef83ebdb7e35 checkpatch-cleared: yes T710-1 verified: yes

ghost commented 8 years ago

This commit adds the appropriate @ prefix before the members of struct visor_device:

branch: correct_kerneldoc_comments; commits: a51545f1176e45ceff3f52a988ef27f7a101bed8

checkpatch-cleared: yes T710-1 verified: yes

selltc commented 8 years ago

For our latest-round of enhancements to go into our upcoming v3 patches:

selltc commented 8 years ago

I updated the visorbus_main.c changes, and pushed a new branch. I updated the previous comment so that it references the new corrected commits.

ghost commented 8 years ago

Comment changes for visorchipset.c (v3 submission): df0cfeb1cba5d14ed03e4a41fb4b87e2e01735ea

Mine also compiled cleanly, and did not introduce checkpatch issues.

selltc commented 8 years ago

I think these are the last commits we need to complete this then:

selltc commented 8 years ago

As-per David's request, I split commit 506b215f606ae2b6fd0cbcc0644dd113fa19180e into commit 6d82f9ab13a52633e929f9bd16a14e2348b5d821 and commit 0e12688156efe4c437fb787fedd2e78599d6b32b, in preparation for submitting v3 of our visorbus patchset.

selltc commented 8 years ago

8/15 - Greg committed these patches into staging-next with: