| Message ID | 20180503141517.28207-1-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 >
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 > >
> 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 */
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 */ >
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
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 */ > > > > > >
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
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.
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 */
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(-)