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

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

[Daniel Vetter]

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@ffwll.ch>
Date:   Wed Apr 25 19:42:52 2018 +0200

    backlight: Nuke BL_CORE_DRIVER1

Cc: Jani Nikula <jani.nikula@intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Cc: Lee Jones <lee.jones@linaro.org>
Cc: Daniel Thompson <daniel.thompson@linaro.org>
Cc: Jingoo Han <jingoohan1@gmail.com>
---
 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 */
-- 
2.17.0

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

[Daniel Thompson]

On Thu, May 03, 2018 at 04:15:17PM +0200, Daniel Vetter 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@ffwll.ch>
> Date:   Wed Apr 25 19:42:52 2018 +0200
> 
>     backlight: Nuke BL_CORE_DRIVER1
> 
> Cc: Jani Nikula <jani.nikula@intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Lee Jones <lee.jones@linaro.org>
> Cc: Daniel Thompson <daniel.thompson@linaro.org>
> Cc: Jingoo Han <jingoohan1@gmail.com>

Acked-by: Daniel Thompson <daniel.thompson@linaro.org>

> ---
>  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 */
> -- 
> 2.17.0
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Daniel Vetter]

On Thu, May 03, 2018 at 03:32:38PM +0100, Daniel Thompson wrote:
> On Thu, May 03, 2018 at 04:15:17PM +0200, Daniel Vetter 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@ffwll.ch>
> > Date:   Wed Apr 25 19:42:52 2018 +0200
> > 
> >     backlight: Nuke BL_CORE_DRIVER1
> > 
> > Cc: Jani Nikula <jani.nikula@intel.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > Cc: Lee Jones <lee.jones@linaro.org>
> > Cc: Daniel Thompson <daniel.thompson@linaro.org>
> > Cc: Jingoo Han <jingoohan1@gmail.com>
> 
> Acked-by: Daniel Thompson <daniel.thompson@linaro.org>

I don't see this yet in linux-next ... should I stuff it into drm-misc or
will it show up?

Thanks, Daniel
> 
> > ---
> >  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 */
> > -- 
> > 2.17.0
> > 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Lee Jones]

> 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@ffwll.ch>
> 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.

> Cc: Jani Nikula <jani.nikula@intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Lee Jones <lee.jones@linaro.org>
> Cc: Daniel Thompson <daniel.thompson@linaro.org>
> Cc: Jingoo Han <jingoohan1@gmail.com>
> ---
>  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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Lee Jones]

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@ffwll.ch>
> > 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.

> > Cc: Jani Nikula <jani.nikula@intel.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > Cc: Lee Jones <lee.jones@linaro.org>
> > Cc: Daniel Thompson <daniel.thompson@linaro.org>
> > Cc: Jingoo Han <jingoohan1@gmail.com>
> > ---
> >  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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Daniel Vetter]

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@ffwll.ch>
> > > 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 and
the 2nd mail still says "This hurts my eyes!".

Over whitespace in the commit message.

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.

Thanks, Daniel

> 
> > > Cc: Jani Nikula <jani.nikula@intel.com>
> > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > > Cc: Lee Jones <lee.jones@linaro.org>
> > > Cc: Daniel Thompson <daniel.thompson@linaro.org>
> > > Cc: Jingoo Han <jingoohan1@gmail.com>
> > > ---
> > >  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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Lee Jones]

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@ffwll.ch>
> > > > 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@intel.com>
> > > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > > > Cc: Lee Jones <lee.jones@linaro.org>
> > > > Cc: Daniel Thompson <daniel.thompson@linaro.org>
> > > > Cc: Jingoo Han <jingoohan1@gmail.com>
> > > > ---
> > > >  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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Daniel Vetter]

Hi Lee,

On Wed, Jul 4, 2018 at 12:34 PM, Lee Jones <lee.jones@linaro.org> wrote:
> 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@ffwll.ch>
>> > > > 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.

Unfortunately minute details like this aren't consistent across the
kernel at all, and white space and layout issues are the number 1
reason I get shot at for random patches I'm sending out. So maybe
there are people who learned all these local expectations (Arnd
perhaps, or Kees?), it's definitely not me. Not after 10 years for
sure.

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

This ain't about my feelings, but working together efficiently and in
a constructive environment.

Also, failing to have adequate maintainer coverage over absence, or
generally being overloaded, is never a valid excuse for how you deal
with contributors. It takes some effort and a bit of time, but group
maintainership in one form or another can take care of this very well.
Brevity justified as efficient communication tends to torpedo that,
since at least in my experience it just drive prospective volunteers
away to more welcoming places.

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Lee Jones]

On Wed, 04 Jul 2018, Daniel Vetter wrote:

> Hi Lee,
> 
> On Wed, Jul 4, 2018 at 12:34 PM, Lee Jones <lee.jones@linaro.org> wrote:
> > 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@ffwll.ch>
> >> > > > 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.
> 
> Unfortunately minute details like this aren't consistent across the
> kernel at all, and white space and layout issues are the number 1
> reason I get shot at for random patches I'm sending out. So maybe
> there are people who learned all these local expectations (Arnd
> perhaps, or Kees?), it's definitely not me. Not after 10 years for
> sure.
> 
> >> 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.
> 
> This ain't about my feelings, but working together efficiently and in
> a constructive environment.
> 
> Also, failing to have adequate maintainer coverage over absence, or
> generally being overloaded, is never a valid excuse for how you deal
> with contributors. It takes some effort and a bit of time, but group
> maintainership in one form or another can take care of this very well.
> Brevity justified as efficient communication tends to torpedo that,
> since at least in my experience it just drive prospective volunteers
> away to more welcoming places.

I'm unsure of the foundations which this scenario builds upon.  Maybe
you've had some bad experiences with other Maintainers in the past
which have made you uber-sensitive, but FWIW I think you're
over-reacting to what were perfectly adequate review comments provided
from one Maintainer to another.

There wasn't any malice or harshness in my recommendations to you nor
did I make any unreasonable requests of you.  Merely an innocent
misunderstanding in the first instance and some gentle advice in the
second.  If constructive feedback isn't something that you deal with
well, perhaps life as a contributor (outside of your own domain at
least) isn't for you.

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel