[02/15] leds: leds-gpio-register: Supply description for param 'id'

Message ID 20210528090629.1800173-3-lee.jones@linaro.org
State New
Headers show
Series
  • Rid W=1 warnings from LED
Related show

Commit Message

Lee Jones May 28, 2021, 9:06 a.m.
Fixes the following W=1 kernel build warning(s):

 drivers/leds/leds-gpio-register.c:24: warning: Function parameter or member 'id' not described in 'gpio_led_register_device'

Cc: Pavel Machek <pavel@ucw.cz>
Cc: Uwe Kleine-Koenig <u.kleine-koenig@pengutronix.de>
Cc: linux-leds@vger.kernel.org
Signed-off-by: Lee Jones <lee.jones@linaro.org>

---
 drivers/leds/leds-gpio-register.c | 1 +
 1 file changed, 1 insertion(+)

-- 
2.31.1

Comments

Uwe Kleine-König May 28, 2021, 9:41 a.m. | #1
Hello Lee,

On Fri, May 28, 2021 at 10:06:16AM +0100, Lee Jones wrote:
> diff --git a/drivers/leds/leds-gpio-register.c b/drivers/leds/leds-gpio-register.c

> index b9187e71e0cf2..de3f12c2b80d7 100644

> --- a/drivers/leds/leds-gpio-register.c

> +++ b/drivers/leds/leds-gpio-register.c

> @@ -11,6 +11,7 @@

>  /**

>   * gpio_led_register_device - register a gpio-led device

>   * @pdata: the platform data used for the new device

> + * @id: platform ID

>   *


Given that id is the first parameter and pdata the second I suggest to
swap the order here and describe id first.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |
Lee Jones May 28, 2021, 9:55 a.m. | #2
On Fri, 28 May 2021, Uwe Kleine-König wrote:

> Hello Lee,

> 

> On Fri, May 28, 2021 at 10:06:16AM +0100, Lee Jones wrote:

> > diff --git a/drivers/leds/leds-gpio-register.c b/drivers/leds/leds-gpio-register.c

> > index b9187e71e0cf2..de3f12c2b80d7 100644

> > --- a/drivers/leds/leds-gpio-register.c

> > +++ b/drivers/leds/leds-gpio-register.c

> > @@ -11,6 +11,7 @@

> >  /**

> >   * gpio_led_register_device - register a gpio-led device

> >   * @pdata: the platform data used for the new device

> > + * @id: platform ID

> >   *

> 

> Given that id is the first parameter and pdata the second I suggest to

> swap the order here and describe id first.


That's super picky.

I can do it as a follow-up patch if you *really* care about it.

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog
Uwe Kleine-König May 28, 2021, 10:35 a.m. | #3
Hey Lee,

On Fri, May 28, 2021 at 10:55:31AM +0100, Lee Jones wrote:
> On Fri, 28 May 2021, Uwe Kleine-König wrote:

> > On Fri, May 28, 2021 at 10:06:16AM +0100, Lee Jones wrote:

> > > diff --git a/drivers/leds/leds-gpio-register.c b/drivers/leds/leds-gpio-register.c

> > > index b9187e71e0cf2..de3f12c2b80d7 100644

> > > --- a/drivers/leds/leds-gpio-register.c

> > > +++ b/drivers/leds/leds-gpio-register.c

> > > @@ -11,6 +11,7 @@

> > >  /**

> > >   * gpio_led_register_device - register a gpio-led device

> > >   * @pdata: the platform data used for the new device

> > > + * @id: platform ID

> > >   *

> > 

> > Given that id is the first parameter and pdata the second I suggest to

> > swap the order here and describe id first.

> 

> That's super picky.

> 

> I can do it as a follow-up patch if you *really* care about it.


I'd say introducing the one-line description for id now in the "wrong"
location and then reordering as a followup is ridiculus. But having said
that: I don't care at all.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |
Lee Jones June 1, 2021, 9:05 a.m. | #4
On Fri, 28 May 2021, Uwe Kleine-König wrote:

> Hey Lee,

> 

> On Fri, May 28, 2021 at 10:55:31AM +0100, Lee Jones wrote:

> > On Fri, 28 May 2021, Uwe Kleine-König wrote:

> > > On Fri, May 28, 2021 at 10:06:16AM +0100, Lee Jones wrote:

> > > > diff --git a/drivers/leds/leds-gpio-register.c b/drivers/leds/leds-gpio-register.c

> > > > index b9187e71e0cf2..de3f12c2b80d7 100644

> > > > --- a/drivers/leds/leds-gpio-register.c

> > > > +++ b/drivers/leds/leds-gpio-register.c

> > > > @@ -11,6 +11,7 @@

> > > >  /**

> > > >   * gpio_led_register_device - register a gpio-led device

> > > >   * @pdata: the platform data used for the new device

> > > > + * @id: platform ID

> > > >   *

> > > 

> > > Given that id is the first parameter and pdata the second I suggest to

> > > swap the order here and describe id first.

> > 

> > That's super picky.

> > 

> > I can do it as a follow-up patch if you *really* care about it.

> 

> I'd say introducing the one-line description for id now in the "wrong"

> location and then reordering as a followup is ridiculus. But having said

> that: I don't care at all.


It's only "wrong" according to you.

I see these presented in a different order to their counterparts all
the time.  I do however appreciate that it does make more sense and
is easier on the eye, which is why I am more than happy to rectify.

With regards to the follow-up scenario, it makes far less sense for an
already merged patch in a history tree to be reverted, or for history
to be unnecessarily re-written for something as trivial as this.

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog
Uwe Kleine-König June 2, 2021, 8:24 a.m. | #5
Hello Lee,

On Tue, Jun 01, 2021 at 10:05:03AM +0100, Lee Jones wrote:
> On Fri, 28 May 2021, Uwe Kleine-König wrote:

> > On Fri, May 28, 2021 at 10:55:31AM +0100, Lee Jones wrote:

> > > On Fri, 28 May 2021, Uwe Kleine-König wrote:

> > > > On Fri, May 28, 2021 at 10:06:16AM +0100, Lee Jones wrote:

> > > > > diff --git a/drivers/leds/leds-gpio-register.c b/drivers/leds/leds-gpio-register.c

> > > > > index b9187e71e0cf2..de3f12c2b80d7 100644

> > > > > --- a/drivers/leds/leds-gpio-register.c

> > > > > +++ b/drivers/leds/leds-gpio-register.c

> > > > > @@ -11,6 +11,7 @@

> > > > >  /**

> > > > >   * gpio_led_register_device - register a gpio-led device

> > > > >   * @pdata: the platform data used for the new device

> > > > > + * @id: platform ID

> > > > >   *

> > > > 

> > > > Given that id is the first parameter and pdata the second I suggest to

> > > > swap the order here and describe id first.

> > > 

> > > That's super picky.

> > > 

> > > I can do it as a follow-up patch if you *really* care about it.

> > 

> > I'd say introducing the one-line description for id now in the "wrong"

> > location and then reordering as a followup is ridiculus. But having said

> > that: I don't care at all.

> 

> It's only "wrong" according to you.

> 

> I see these presented in a different order to their counterparts all

> the time.


This is a poor justification. In software bugs happen all the time, this
doesn't mean you shouldn't make it better when you touch some code.

> I do however appreciate that it does make more sense and

> is easier on the eye, which is why I am more than happy to rectify.


... still more if you agree that the feedback makes sense.

> With regards to the follow-up scenario, it makes far less sense for an

> already merged patch in a history tree to be reverted, or for history

> to be unnecessarily re-written for something as trivial as this.


Ah, that the patch is already merged is news to me. Indeed, then fixing
this is not sensible. My initial feedback was less than an hour after
you sent the patch and it appeared just yesterday in next, so this
wasn't easily noticeable for me.

Usually I'm annoyed about maintainers who don't react to patch series
and don't apply it. Here I'm more annoyed that I was Cc:d---which I
interpret as a request for feedback---and an hour later was already too
late for my review reply and there was (up to today) no maintainer mail
that the patch set was applied.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |
Alexander Dahl June 2, 2021, 8:33 a.m. | #6
Hello Uwe,

Am Wed, Jun 02, 2021 at 10:24:36AM +0200 schrieb Uwe Kleine-König:
> Ah, that the patch is already merged is news to me. Indeed, then fixing

> this is not sensible. My initial feedback was less than an hour after

> you sent the patch and it appeared just yesterday in next, so this

> wasn't easily noticeable for me.

> 

> Usually I'm annoyed about maintainers who don't react to patch series

> and don't apply it. Here I'm more annoyed that I was Cc:d---which I

> interpret as a request for feedback---and an hour later was already too

> late for my review reply and there was (up to today) no maintainer mail

> that the patch set was applied.


Pavel applied (part of) the series quite quickly and stated in reply
to patch 14/15:

https://lore.kernel.org/linux-leds/20210528093921.GA2209@amd/

So I was also surprised my review of two patches, one hour after
posting the series, was already too late.

This is not really motivating to review at all. ¯\_(ツ)_/¯

Greets
Alex
Uwe Kleine-König June 4, 2021, 5:05 a.m. | #7
Hello Alex,

On Wed, Jun 02, 2021 at 10:33:11AM +0200, Alexander Dahl wrote:
> Am Wed, Jun 02, 2021 at 10:24:36AM +0200 schrieb Uwe Kleine-König:

> > Ah, that the patch is already merged is news to me. Indeed, then fixing

> > this is not sensible. My initial feedback was less than an hour after

> > you sent the patch and it appeared just yesterday in next, so this

> > wasn't easily noticeable for me.

> > 

> > Usually I'm annoyed about maintainers who don't react to patch series

> > and don't apply it. Here I'm more annoyed that I was Cc:d---which I

> > interpret as a request for feedback---and an hour later was already too

> > late for my review reply and there was (up to today) no maintainer mail

> > that the patch set was applied.

> 

> Pavel applied (part of) the series quite quickly and stated in reply

> to patch 14/15:

> 

> https://lore.kernel.org/linux-leds/20210528093921.GA2209@amd/


Just for the record: I only got the cover letter, patch 02 and the
thread below the latter. So seeing the info there isn't straight
forward either.

> This is not really motivating to review at all. ¯\_(ツ)_/¯


+1

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Patch

diff --git a/drivers/leds/leds-gpio-register.c b/drivers/leds/leds-gpio-register.c
index b9187e71e0cf2..de3f12c2b80d7 100644
--- a/drivers/leds/leds-gpio-register.c
+++ b/drivers/leds/leds-gpio-register.c
@@ -11,6 +11,7 @@ 
 /**
  * gpio_led_register_device - register a gpio-led device
  * @pdata: the platform data used for the new device
+ * @id: platform ID
  *
  * Makes a copy of pdata and pdata->leds and registers a new leds-gpio device
  * with the result. This allows to have pdata and pdata-leds in .init.rodata