On Fri, Jun 17, 2016 at 05:55:52PM +0200, Thierry Reding wrote:

On Fri, Jun 17, 2016 at 09:06:35AM +0100, Lee Jones wrote:

quoted

On Thu, 16 Jun 2016, Doug Anderson wrote:

quoted

On Thu, Jun 16, 2016 at 8:38 AM, Lee Jones [off-list ref] wrote:

[...]

quoted

quoted

quoted

Please use kerneldoc format.

[...]

quoted

quoted

Probably the reason for all of these non-kernel-isms is that this
isn't a kernel file.  From the top of the file:

 * NOTE: This file is copied verbatim from the ChromeOS EC Open Source
 * project in an attempt to make future updates easy to make.

So the source of truth for this file is
<https://chromium.googlesource.com/chromiumos/platform/ec/+/master/include/ec_commands.h>.

Someone could probably submit a CL to that project to make it a little
more kernel-ish and then we'd have to see if the EC team would accept
such changes...

Hmmm... that kinda puts me in a difficult position.  Do I except
non-kernel code, which does not conform to our stands?

I could have sworn that there was an exception for existing code;
changing styles just for the sake of changing styles can hurt more than
help (e.g., in this instance, in comparing this file with the source of
truth -- the Chrome OS EC firmware sources). But I can't find an
explicit reference to this right now...

I think usually code that doesn't adhere to kernel coding style ends up
in the staging tree until it's been cleaned up enough.

The rule doesn't quite apply here because cleaning up isn't the issue.
But I don't know if we have any best practices for this kind of thing.

One thing that I've seen done in the past is to have this kind of
cross-OS header generated from some sort of definition file (XML, ...)
with an output filter for all supported OSes. Is that something that
could perhaps be done here?

I think I can safely say that rewriting this in XML is out of the
question for us.

That said, this might not even be worth it in this case. While I see how
it makes sense to avoid work updating this file for various coding
styles, the content in this file defines an ABI, so really the only
changes will be additions, and once they're merged they become set in
stone anyway. The amount of work updating the header, even taking into
account different coding styles should be very low.

IMO, there is value in being able to compare the two sources, and there
is little (but non-zero) value in making these rather minor suggested
changes. Additionally, reformatting this particular change to use
kerneldoc, when the rest of the file doesn't, seems inconsistent. How
does the following sound instead?

(1) Keep the existing patch as-is (perhaps modulo one or two of Lee's
    non-kerneldoc-related comments)
(2) I send a follow-up patch, to help synchronize the out-of-tree EC
    header with the kernel header, after we've considered converting it
    to better match kernel styles. BTW, we've filed a bug for this
    discussion [1], to ensure it doesn't get dropped on the floor.

Brian

[1] https://bugs.chromium.org/p/chromium/issues/detail?id=621123