Re: [PATCH 1/9 v3] staging: spi: mt7621: Switch to SPDX identifier

From: NeilBrown <neil@brown.name>
To: Mark Brown <broonie@kernel.org>
Cc: devel@driverdev.osuosl.org,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	John Crispin <john@phrozen.org>,
	linux-spi@vger.kernel.org,
	Sankalp Negi <sankalpnegi2310@gmail.com>,
	Stefan Roese <sr@denx.de>
Subject: Re: [PATCH 1/9 v3] staging: spi: mt7621: Switch to SPDX identifier
Date: Tue, 05 Feb 2019 10:09:26 +1100	[thread overview]
Message-ID: <871s4njgm1.fsf@notabene.neil.brown.name> (raw)
In-Reply-To: <20190204085302.GB23441@sirena.org.uk>

[-- Attachment #1.1: Type: text/plain, Size: 2291 bytes --]

On Mon, Feb 04 2019, Mark Brown wrote:

> On Mon, Feb 04, 2019 at 09:34:56AM +1100, NeilBrown wrote:
>
>>  It is extremely common in the kernel for a file to start
>>    // SPDX-License-Identifier.....
>
>>  and to have that immediately followed by a comment lile:
>
>>  /*
>>   * .....
>>   * ....
>
> Yes, there was a lot of automated conversion AFAICT (and a lot of
> confusion with all this stuff only being documented in random mailing
> list posts for a long time).
>
>>  This patch makes this file match much of the rest of the kernel.  Why
>>  do you want something different?
>
> Like I said because it makes the comments look more like someone
> actually meant to add a C++ comment - it's what the rest of the
> subsystem is doing too.

Ahh.. the argument "what the rest of the subsystem is doing" makes a lot
more sense to me than "look more like someone actually mean to add a C++
comment", because I really don't understand how

    // SPDX-License-Identifier.....
    /*
     * old comment
     */

doesn't look like it was meant to be added.

Looking around the kernel, the pattern of "// SPDX-..." followed by a
"//" happens a lot in

     99 arch/csky/
    122 arch/nds32/
    152 sound/soc/
    201 drivers/media/
    273 arch/arm/

(numbers are how many instances I found), and less so in

     10 drivers/soc/
     11 drivers/soundwire/
     12 drivers/misc/
     12 drivers/pci/
     13 drivers/mfd/
     13 drivers/power/
     14 drivers/dma/
     14 drivers/input/
     14 scripts/coccinelle/
     16 drivers/leds/
     18 drivers/spi/
     25 arch/arm64/
     26 drivers/regulator/
     28 drivers/clk/
     29 drivers/net/
     36 tools/testing/
     52 drivers/pinctrl/

and much less else where.
Not at all in fs/ or mm/ or net/

It would be nice to have some consistency.

I would be in favour of avoiding // as much as possible.
The "// SPDX-" lines were (presumably) added nearly automatically.
I would prefer that if it were don't manually, it would always be

  /*
   * SPDX- ....
   * etc
   */

but I guess that horse has left the gate, though we do have that pattern
in about 129 files.

What a mess.

Any way, if you make a case based on "that is what most other files in
drivers/spi/ do", then I won't have any disagreement with that.

Thanks,
NeilBrown

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

[-- Attachment #2: Type: text/plain, Size: 169 bytes --]

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel