| Message ID | 20210528090629.1800173-3-lee.jones@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | Rid W=1 warnings from LED | expand |
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/ |
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
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/ |
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
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/ |
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
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/ |
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
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