Re: [PATCH] clk: actions: Use the correct style for SPDX License Identifier

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

On Thu, May 02, 2019 at 12:45:05PM +0200, Andreas Färber wrote:
> Am 02.05.19 um 12:38 schrieb Greg Kroah-Hartman:
> > On Thu, May 02, 2019 at 12:25:36PM +0200, Andreas Färber wrote:
> >> Am 02.05.19 um 09:07 schrieb Greg Kroah-Hartman:
> >>> On Wed, May 01, 2019 at 10:20:44PM +0200, Andreas Färber wrote:
> >>>> + linux-actions
> >>>>
> >>>> Am 01.05.19 um 09:07 schrieb Nishad Kamdar:
> >>>>> This patch corrects the SPDX License Identifier style
> >>>>> in header files related to Clock Drivers for Actions Semi Socs.
> >>>>> For C header files Documentation/process/license-rules.rst
> >>>>> mandates C-like comments (opposed to C source files where
> >>>>> C++ style should be used)
> >>>> [...]
> >>>>>  drivers/clk/actions/owl-common.h       | 2 +-
> >>>>>  drivers/clk/actions/owl-composite.h    | 2 +-
> >>>>>  drivers/clk/actions/owl-divider.h      | 2 +-
> >>>>>  drivers/clk/actions/owl-factor.h       | 2 +-
> >>>>>  drivers/clk/actions/owl-fixed-factor.h | 2 +-
> >>>>>  drivers/clk/actions/owl-gate.h         | 2 +-
> >>>>>  drivers/clk/actions/owl-mux.h          | 2 +-
> >>>>>  drivers/clk/actions/owl-pll.h          | 2 +-
> >>>>>  drivers/clk/actions/owl-reset.h        | 2 +-
> >>>>>  9 files changed, 9 insertions(+), 9 deletions(-)
> >>>>
> >>>> Where's the practical benefit of this patch? These are all private
> >>>> headers used from C files, so they can handle C++ comments just fine,
> >>>> otherwise we would've seen build failures.
> >>>
> >>> Please read Documentation/process/license-rules.rst, the section
> >>> entitled "Style", for what the documented formats are for SPDX lines,
> >>> depending on the file type.
> >>
> >> That does in no way answer my question! You conveniently dropped my
> >> paragraph indicating that I understand why we would do that for public
> >> headers in include/, but none of these private headers here are included
> >> in .lds files. So there really seems to be no benefit of switching from
> >> one style to another for in-tree code.
> > 
> > It should answer the question, it was "decreed" that all header files
> > use /* */, and all C files use // for their SPDX lines, so we documented
> > it that way.
> > 
> > Yes, maybe it doesn't make "sense" in that this really is only needed
> > for headers that get included into asm files, which is why we had to do
> > it this way, but it's better to be consistant than to have random
> > breakages at times.
> > 
> > It's not an issue of public headers at all, sorry.
> > 
> > Consistency is good, as we can have automatic tools check these types of
> > things, which is the only way to reliably handle the format of something
> > that needs to be in every file in a project with 63,100+ different
> > files.
> 
> Okay, if it's about consistency then there will be more cases to fix.

Agreed, hopefully checkpatch is up to date enough to catch these.

> What about this one:
> 
> My interpretation of the documentation has been that I should end the
> comment after the identifiers:
> 
> /* SPDX-... */
> /* ...
>  */

Correct.

> Some people deviate by doing
> 
> /* SPDX-...
>  * foo
>  */

Not correct.

> So the documentation may need to be extended to clarify that for full
> consistency, as well as clarify the previous scenario:
>   "If a specific tool cannot handle the standard comment style, then the
>    appropriate comment mechanism which the tool accepts shall be used."
> To me that reads very different from what you just said above.

Documentation can always be updated, a patch to make it clearer is
always appreciated.  But look at what we have today in the document, I
think it should be pretty obvious that:
	/* SPDX... */
is the thing to use for C header files.

If you disagree, that's fine, please send a patch to make it clearer and
we can all review it.

thanks,

greg k-h

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel