ec: Consider adopting kernel-doc format for struct description |
||||
Issue descriptionFor function prototypes in header files, we're already following a format very similar to kernel-doc. Shall we also follow kernel-doc for describing structs? https://www.kernel.org/doc/Documentation/kernel-doc-nano-HOWTO.txt /** * struct blah - the basic blah structure * @mem1: describe the first member of struct blah * @mem2: describe the second member of struct blah, * perhaps with more lines and words. * * Longer description of this structure. */ Background here is that kernel upstream reviewers are being strict about documentation in ec_commands.h, which is copied almost verbatim into the kernel project (and depthcharge, coreboot, u-boot, flashrom, mosys as well). If we all agree that changing struct descriptions to use kernel-doc is an overall improvement to cros-ec then I think we should go ahead and change it. Currently we're a bit inconsistent with our struct commenting so I think standardizing things would be beneficial. Comments?
,
Jun 18 2016
Yeah, do it.
,
Jun 18 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/platform/ec/+/81e3b15305882d5cfe6145d822bb5656bfe3d6b6 commit 81e3b15305882d5cfe6145d822bb5656bfe3d6b6 Author: Brian Norris <briannorris@chromium.org> Date: Fri Jun 17 19:23:26 2016 ec_commands: use hex to make EC_PWM_MAX_DUTY clearer Some comments in upstream Linux review have suggested this be hex. Makes sense to me. BUG= chromium:621123 TEST=build BRANCH=none Change-Id: Ib7143acc96a2fe593d5e02ad0fba3a501bd8cea2 Signed-off-by: Brian Norris <briannorris@chromium.org> Reviewed-on: https://chromium-review.googlesource.com/353681 Reviewed-by: Douglas Anderson <dianders@chromium.org> Reviewed-by: Shawn N <shawnn@chromium.org> [modify] https://crrev.com/81e3b15305882d5cfe6145d822bb5656bfe3d6b6/include/ec_commands.h
,
Jun 20 2016
SGTM. How are we going to transition to and enforce the new style?
,
Jun 20 2016
Transition: Reformatting comments is really easy to review. Anyone need to boost their "number of lines of changes" for the next perf cycle? Enforce: We could review the style guidelines quickly at the next firmware team meeting so we have consistent expectations for code reviews. And then peer pressure.
,
Jun 20 2016
> Enforce: We could review the style guidelines quickly at the next firmware team > meeting so we have consistent expectations for code reviews. And then peer pressure. Unfortunately, checkpatch.pl doesn't have a check but we can probably utilize scripts/kernel-doc of Linux kernel. With a minor modification, it should be able to tell whether the style is violated or not.
,
Jul 15 2016
Just curious, is somebody planning to claim this bug? I'm planning to resend the upstream Linux patch series that started this discussion.
,
Apr 7 2017
,
Nov 9
The time has come. Somebody else reformatted our upstream copy to (mostly) kerneldoc, and it's now extremely noisy when trying to sync with upstream: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=e2bbf91cad09118d7500f1fdaaa83d7741d30395 Subject: mfd: cros_ec: Fix and improve kerneldoc comments. It doesn't sound like anybody objected to using this style in the EC, so I'm going to at least sync our trees to this state. There's additional stuff we've added in the EC repository that hasn't been synced to Linux, and maybe I'll stab at reformatting that too. Not sure whether I'll tackle style enforcement.
,
Nov 9
The presubmit wasn't too hard, so I did it too: https://chromium-review.googlesource.com/c/chromiumos/platform/ec/+/1330106 third_party/kernel-doc: import from Linux https://chromium-review.googlesource.com/c/chromiumos/platform/ec/+/1330107 PRESUBMIT: sanity check include/ec_commands.h for kernel-doc https://chromium-review.googlesource.com/c/chromiumos/platform/ec/+/1330108 ec_commands: sync some header comments, kerneldoc from kernel tree https://chromium-review.googlesource.com/c/chromiumos/platform/ec/+/1330109 ec_commands: more kerneldoc conversions
,
Nov 15
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/platform/ec/+/2d32226a189a1bcfec7544f75d8218b32f737f7f commit 2d32226a189a1bcfec7544f75d8218b32f737f7f Author: Brian Norris <briannorris@chromium.org> Date: Thu Nov 15 16:11:22 2018 ec_commands: sync some header comments, kerneldoc from kernel tree External folks helped rewrite much of the kernel tree's header style: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=e2bbf91cad09118d7500f1fdaaa83d7741d30395 Subject: mfd: cros_ec: Fix and improve kerneldoc comments. Let's sync with that, to reduce the noise when syncing the EC tree to the kernel tree. BRANCH=none BUG= chromium:621123 TEST=presubmits Change-Id: I5b6b7810d8943a164a4b7738d6911a9393261aeb Signed-off-by: Brian Norris <briannorris@chromium.org> Reviewed-on: https://chromium-review.googlesource.com/1330108 [modify] https://crrev.com/2d32226a189a1bcfec7544f75d8218b32f737f7f/include/ec_commands.h
,
Nov 15
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/platform/ec/+/c9e12e5ec28e0e6c7297862cc7b42f799ad6aec2 commit c9e12e5ec28e0e6c7297862cc7b42f799ad6aec2 Author: Brian Norris <briannorris@chromium.org> Date: Thu Nov 15 16:11:22 2018 ec_commands: more kerneldoc conversions It's not necessarily required that all structs be using kerneldoc, but several of these are adequately documented and fit the same pattern of conversions that were done upstream -- except that they currently only exist in the EC sources. Let's convert preemptively. BRANCH=none BUG= chromium:621123 TEST=presubmits Change-Id: I0922db83983ed03f9a02142606c425f0af6313d1 Signed-off-by: Brian Norris <briannorris@chromium.org> Reviewed-on: https://chromium-review.googlesource.com/1330109 [modify] https://crrev.com/c9e12e5ec28e0e6c7297862cc7b42f799ad6aec2/include/ec_commands.h
,
Nov 15
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/platform/ec/+/d67134434d7255adeb2effdc1246ee6ef37e4524 commit d67134434d7255adeb2effdc1246ee6ef37e4524 Author: Brian Norris <briannorris@chromium.org> Date: Thu Nov 15 16:11:22 2018 ec_commands: move __ec_* macros to end of struct definition This matches the typical style for kernel structs, and it avoids tripping up the kernel's kernel-doc parser/validator. This matters, because we sync this header with the Linux kernel tree regularly. This transformation was done entirely with vim macros, to match '^struct __ec_' and move the second word down to the next '^};'. BRANCH=none BUG= chromium:621123 TEST=presubmits Change-Id: I54c1b5b2daa47a54727049551ef1401a4881e1a5 Signed-off-by: Brian Norris <briannorris@chromium.org> Reviewed-on: https://chromium-review.googlesource.com/1335685 Reviewed-by: Randall Spangler <rspangler@chromium.org> [modify] https://crrev.com/d67134434d7255adeb2effdc1246ee6ef37e4524/include/ec_commands.h
,
Dec 11
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/repohooks/+/23c62e9f8087539c08cbee2c477853849aa69f3b commit 23c62e9f8087539c08cbee2c477853849aa69f3b Author: Brian Norris <briannorris@chromium.org> Date: Tue Dec 11 21:09:33 2018 pre-upload: add 'kerneldoc_check' presubmit kernel-doc script is taken from Linux v4.20-rc1. Don't enable this by default, and don't enable any files by default, since we don't know what files a project intends to process. Files should be added with --include_regex. BUG= chromium:621123 TEST=unit tests; run against EC CLs Change-Id: I7ec829b4c3abbc381c381d7f9a8f348844ca9518 Signed-off-by: Brian Norris <briannorris@chromium.org> Reviewed-on: https://chromium-review.googlesource.com/1336039 Reviewed-by: Ben Chan <benchan@chromium.org> [add] https://crrev.com/23c62e9f8087539c08cbee2c477853849aa69f3b/kernel-doc [modify] https://crrev.com/23c62e9f8087539c08cbee2c477853849aa69f3b/PRESUBMIT.cfg [modify] https://crrev.com/23c62e9f8087539c08cbee2c477853849aa69f3b/pre-upload.py
,
Dec 12
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/platform/ec/+/7d03d8557bb3d99ff741478e4417c35a4ca8b8f7 commit 7d03d8557bb3d99ff741478e4417c35a4ca8b8f7 Author: Brian Norris <briannorris@chromium.org> Date: Wed Dec 12 21:55:56 2018 PRESUBMIT: enable kernel-doc check for include/ec_commands.h ec_commands.h is often synced with the Linux sources, so it's nice to sanity-check it. CQ-DEPEND=CL:1336039 BRANCH=none BUG= chromium:621123 TEST=.../pre-upload.py Change-Id: I56645e613f8689c26ddf6ded1325e270be35da8a Signed-off-by: Brian Norris <briannorris@chromium.org> Reviewed-on: https://chromium-review.googlesource.com/1330107 Reviewed-by: Randall Spangler <rspangler@chromium.org> [modify] https://crrev.com/7d03d8557bb3d99ff741478e4417c35a4ca8b8f7/PRESUBMIT.cfg
,
Dec 12
|
||||
►
Sign in to add a comment |
||||
Comment 1 by rspangler@chromium.org
, Jun 17 2016