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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 2 Jun 2021 10:24:36 +0200
From:   Uwe Kleine-König <u.kleine-koenig@...gutronix.de>
To:     Lee Jones <lee.jones@...aro.org>, Pavel Machek <pavel@....cz>
Cc:     linux-kernel@...r.kernel.org, linux-leds@...r.kernel.org
Subject: Re: [PATCH 02/15] leds: leds-gpio-register: Supply description for
 param 'id'

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/ |

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)