Re: [PATCH] backlight: remove obsolete comment for ->state

From: Lee Jones
Date: Wed Jul 04 2018 - 06:34:10 EST


On Wed, 04 Jul 2018, Daniel Vetter wrote:
> On Wed, Jul 04, 2018 at 10:38:16AM +0100, Lee Jones wrote:
> > On Wed, 04 Jul 2018, Lee Jones wrote:
> >
> > > > Jani spotted this when reviewing my earlier patch to remove the driver
> > > > internal usage of this field in
> > > >
> > > > commit 3cf91adaa594e8933af1727942ac560e5c7bc70e
> > > > Author: Daniel Vetter <daniel.vetter@xxxxxxxx>
> > > > Date: Wed Apr 25 19:42:52 2018 +0200
> > > >
> > > > backlight: Nuke BL_CORE_DRIVER1
> > >
> > > FYI, sending patches like this is not a good idea.
> > >
> > > I'll clean it up for you this time, but in future please send patches
> > > properly and place any additional comments you may have below the
> > > '---' line.
> >
> > Ah, I see what you've tried to do. This hurt my eyes! :)
> >
> > It's more conventional to reference commits like:
> >
> > Commit 3cf91adaa594 ("backlight: Nuke BL_CORE_DRIVER1")
> >
> > Again, I'll make the amendment to avoid further confusion.
>
> So the first mail doesn't even bother to explain what's
> objectionable

In the first instance it looked as though you'd copied and pasted `git
log`, which if you'd done so would have been obvious to you and would
have required no further explanation.

Also bear in mind that I took your standing within the kernel
community into consideration, so speaking to you like a n00b or going
into unnecessary detail could have been considered superfluous at best
and condescending at worst.

> the 2nd mail still says "This hurts my eyes!".

It certainly did, yes.

Usually taken to meaning that it was hard to read in these scenarios.

> Over whitespace in the commit message.

Nothing to do with white space. It was the format by which you chose
to reference a previous commit. Instead it looked like a patch
formatting error. I have received patches pasted from `git log`
before, and this looked just the same.

Once I'd realised what was going on, I followed up to explain and
provided some feedback on what to do differently in future.

> This kind of stuff is why graphics people really don't enjoy contributing
> to the kernel at large. A friendly request to resend with the color choice
> adjusted would go a lot further.

I apologise if my brevity hurt your feelings. I have 290 emails to
get though post-paternity leave before I can start to think about
getting some real/paid work done.

> > > > Cc: Jani Nikula <jani.nikula@xxxxxxxxx>
> > > > Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxxx>
> > > > Cc: Lee Jones <lee.jones@xxxxxxxxxx>
> > > > Cc: Daniel Thompson <daniel.thompson@xxxxxxxxxx>
> > > > Cc: Jingoo Han <jingoohan1@xxxxxxxxx>
> > > > ---
> > > > include/linux/backlight.h | 1 -
> > > > 1 file changed, 1 deletion(-)
> > > >
> > > > diff --git a/include/linux/backlight.h b/include/linux/backlight.h
> > > > index 7fbf0539e14a..0b5897446dca 100644
> > > > --- a/include/linux/backlight.h
> > > > +++ b/include/linux/backlight.h
> > > > @@ -79,7 +79,6 @@ struct backlight_properties {
> > > > /* Backlight type */
> > > > enum backlight_type type;
> > > > /* Flags used to signal drivers of state changes */
> > > > - /* Upper 4 bits are reserved for driver internal use */
> > > > unsigned int state;
> > > >
> > > > #define BL_CORE_SUSPENDED (1 << 0) /* backlight is suspended */
> > >
> >
>

--
Lee Jones [æçæ]
Linaro Services Technical Lead
Linaro.org â Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog