Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1204979 > unrolled thread

[PATCH/RFC v5 01/57] leds: Add brightness_set_nonblocking op

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH/RFC v5 01/57] leds: Add brightness_set_nonblocking op Jacek Anaszewski <j.anaszewski@samsung.com> - 2015-08-11 12:00 +0200
    Re: [PATCH/RFC v5 01/57] leds: Add brightness_set_nonblocking op Pavel Machek <pavel@ucw.cz> - 2015-08-13 16:30 +0200
    Re: [PATCH/RFC v5 01/57] leds: Add brightness_set_nonblocking op Andrew Lunn <andrew@lunn.ch> - 2015-08-13 16:30 +0200
      Re: [PATCH/RFC v5 01/57] leds: Add brightness_set_nonblocking op Jacek Anaszewski <j.anaszewski@samsung.com> - 2015-08-14 10:00 +0200
        Re: [PATCH/RFC v5 01/57] leds: Add brightness_set_nonblocking op Jacek Anaszewski <j.anaszewski@samsung.com> - 2015-08-19 16:10 +0200
          Re: [PATCH/RFC v5 01/57] leds: Add brightness_set_nonblocking op Andrew Lunn <andrew@lunn.ch> - 2015-08-19 16:20 +0200

#1204979 — [PATCH/RFC v5 01/57] leds: Add brightness_set_nonblocking op

This patch adds a new brightness_set_nonblocking op to the LED subsystem.
The op is intended for drivers that set brightness in a non-blocking way,
i.e. they neither sleep nor use delays while setting brightness.

Signed-off-by: Jacek Anaszewski <j.anaszewski@samsung.com>
Cc: Bryan Wu <cooloney@gmail.com>
Cc: Andrew Lunn <andrew@lunn.ch>
Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
Cc: Pavel Machek <pavel@ucw.cz>
Cc: Stas Sergeev <stsp@users.sourceforge.net>
---
 include/linux/leds.h |    3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/linux/leds.h b/include/linux/leds.h
index b122eea..c32f1b8 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -53,6 +53,9 @@ struct led_classdev {
 	/* Must not sleep, use a workqueue if needed */
 	void		(*brightness_set)(struct led_classdev *led_cdev,
 					  enum led_brightness brightness);
+	/* Intended for drivers that set brightness in a non-blocking way */
+	void (*brightness_set_nonblocking)(struct led_classdev *led_cdev,
+					  enum led_brightness brightness);
 	/*
 	 * Set LED brightness level immediately - it can block the caller for
 	 * the time required for accessing a LED device register.
-- 
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1206852

On Thu 2015-08-13 16:15:47, Andrew Lunn wrote:
> On Tue, Aug 11, 2015 at 11:37:14AM +0200, Jacek Anaszewski wrote:
> > This patch adds a new brightness_set_nonblocking op to the LED subsystem.
> > The op is intended for drivers that set brightness in a non-blocking way,
> > i.e. they neither sleep nor use delays while setting brightness.
> > 
> > Signed-off-by: Jacek Anaszewski <j.anaszewski@samsung.com>
> > Cc: Bryan Wu <cooloney@gmail.com>
> > Cc: Andrew Lunn <andrew@lunn.ch>
> > Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
> > Cc: Pavel Machek <pavel@ucw.cz>
> > Cc: Stas Sergeev <stsp@users.sourceforge.net>
> > ---
> >  include/linux/leds.h |    3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/include/linux/leds.h b/include/linux/leds.h
> > index b122eea..c32f1b8 100644
> > --- a/include/linux/leds.h
> > +++ b/include/linux/leds.h
> > @@ -53,6 +53,9 @@ struct led_classdev {
> >  	/* Must not sleep, use a workqueue if needed */
> >  	void		(*brightness_set)(struct led_classdev *led_cdev,
> >  					  enum led_brightness brightness);
> > +	/* Intended for drivers that set brightness in a non-blocking way */
> > +	void (*brightness_set_nonblocking)(struct led_classdev *led_cdev,
> > +					  enum led_brightness brightness);
> 
> Hi Jacek
> 
> >From an API design point of view, i'm not sure this is the best way to
> go. You now have two calls which do the same thing, with the plan that
> you want to invert the meaning of brightness_set, the old well known
> API call, sometime later. This inverting the meaning is going to catch
> people out and introduce bugs.

Yes please.
									Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1206857

On Tue, Aug 11, 2015 at 11:37:14AM +0200, Jacek Anaszewski wrote:
> This patch adds a new brightness_set_nonblocking op to the LED subsystem.
> The op is intended for drivers that set brightness in a non-blocking way,
> i.e. they neither sleep nor use delays while setting brightness.
> 
> Signed-off-by: Jacek Anaszewski <j.anaszewski@samsung.com>
> Cc: Bryan Wu <cooloney@gmail.com>
> Cc: Andrew Lunn <andrew@lunn.ch>
> Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
> Cc: Pavel Machek <pavel@ucw.cz>
> Cc: Stas Sergeev <stsp@users.sourceforge.net>
> ---
>  include/linux/leds.h |    3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/include/linux/leds.h b/include/linux/leds.h
> index b122eea..c32f1b8 100644
> --- a/include/linux/leds.h
> +++ b/include/linux/leds.h
> @@ -53,6 +53,9 @@ struct led_classdev {
>  	/* Must not sleep, use a workqueue if needed */
>  	void		(*brightness_set)(struct led_classdev *led_cdev,
>  					  enum led_brightness brightness);
> +	/* Intended for drivers that set brightness in a non-blocking way */
> +	void (*brightness_set_nonblocking)(struct led_classdev *led_cdev,
> +					  enum led_brightness brightness);

Hi Jacek

From an API design point of view, i'm not sure this is the best way to
go. You now have two calls which do the same thing, with the plan that
you want to invert the meaning of brightness_set, the old well known
API call, sometime later. This inverting the meaning is going to catch
people out and introduce bugs.

I would rather add a brightness_set_blocking op. Then as you go
thought the drivers stripping out the work queue, move the driver to
use this brightness_set_blocking.

    Andrew
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1207390

H Andrew,

On 08/13/2015 04:15 PM, Andrew Lunn wrote:
> On Tue, Aug 11, 2015 at 11:37:14AM +0200, Jacek Anaszewski wrote:
>> This patch adds a new brightness_set_nonblocking op to the LED subsystem.
>> The op is intended for drivers that set brightness in a non-blocking way,
>> i.e. they neither sleep nor use delays while setting brightness.
>>
>> Signed-off-by: Jacek Anaszewski <j.anaszewski@samsung.com>
>> Cc: Bryan Wu <cooloney@gmail.com>
>> Cc: Andrew Lunn <andrew@lunn.ch>
>> Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
>> Cc: Pavel Machek <pavel@ucw.cz>
>> Cc: Stas Sergeev <stsp@users.sourceforge.net>
>> ---
>>   include/linux/leds.h |    3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/include/linux/leds.h b/include/linux/leds.h
>> index b122eea..c32f1b8 100644
>> --- a/include/linux/leds.h
>> +++ b/include/linux/leds.h
>> @@ -53,6 +53,9 @@ struct led_classdev {
>>   	/* Must not sleep, use a workqueue if needed */
>>   	void		(*brightness_set)(struct led_classdev *led_cdev,
>>   					  enum led_brightness brightness);
>> +	/* Intended for drivers that set brightness in a non-blocking way */
>> +	void (*brightness_set_nonblocking)(struct led_classdev *led_cdev,
>> +					  enum led_brightness brightness);
>
> Hi Jacek
>
>>From an API design point of view, i'm not sure this is the best way to
> go. You now have two calls which do the same thing, with the plan that
> you want to invert the meaning of brightness_set, the old well known
> API call, sometime later. This inverting the meaning is going to catch
> people out and introduce bugs.
>
> I would rather add a brightness_set_blocking op. Then as you go
> thought the drivers stripping out the work queue, move the driver to
> use this brightness_set_blocking.

There are around 60 drivers in the other kernel subsystems that register
LED class devices. If we chose the way you proposed then we would have
to adjust all of them to the LED core changes, which could complicate
the situation during merge window if there were other modifications in
the affected drivers.

With my approach all old-fashion drivers will be treated by the
LED core in the old-fashion way. This is why I introduced
LED_BRIGHTNESS_BLOCKING flag - only drivers aware of the LED core
changes will set it, which will allow for the LED core to find out
which drivers are aware of brightness_set semantics change and
implement the op accordingly.

This way the change of brightness_set semantics will not be painful.

After all drivers across all subsystems are adapted to the LED core
changes the flag will be made redundant and we will remove it.

-- 
Best Regards,
Jacek Anaszewski
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1209838

On 08/14/2015 09:57 AM, Jacek Anaszewski wrote:
> H Andrew,
>
> On 08/13/2015 04:15 PM, Andrew Lunn wrote:
>> On Tue, Aug 11, 2015 at 11:37:14AM +0200, Jacek Anaszewski wrote:
>>> This patch adds a new brightness_set_nonblocking op to the LED
>>> subsystem.
>>> The op is intended for drivers that set brightness in a non-blocking
>>> way,
>>> i.e. they neither sleep nor use delays while setting brightness.
>>>
>>> Signed-off-by: Jacek Anaszewski <j.anaszewski@samsung.com>
>>> Cc: Bryan Wu <cooloney@gmail.com>
>>> Cc: Andrew Lunn <andrew@lunn.ch>
>>> Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
>>> Cc: Pavel Machek <pavel@ucw.cz>
>>> Cc: Stas Sergeev <stsp@users.sourceforge.net>
>>> ---
>>>   include/linux/leds.h |    3 +++
>>>   1 file changed, 3 insertions(+)
>>>
>>> diff --git a/include/linux/leds.h b/include/linux/leds.h
>>> index b122eea..c32f1b8 100644
>>> --- a/include/linux/leds.h
>>> +++ b/include/linux/leds.h
>>> @@ -53,6 +53,9 @@ struct led_classdev {
>>>       /* Must not sleep, use a workqueue if needed */
>>>       void        (*brightness_set)(struct led_classdev *led_cdev,
>>>                         enum led_brightness brightness);
>>> +    /* Intended for drivers that set brightness in a non-blocking
>>> way */
>>> +    void (*brightness_set_nonblocking)(struct led_classdev *led_cdev,
>>> +                      enum led_brightness brightness);
>>
>> Hi Jacek
>>
>>> From an API design point of view, i'm not sure this is the best way to
>> go. You now have two calls which do the same thing, with the plan that
>> you want to invert the meaning of brightness_set, the old well known
>> API call, sometime later. This inverting the meaning is going to catch
>> people out and introduce bugs.
>>
>> I would rather add a brightness_set_blocking op. Then as you go
>> thought the drivers stripping out the work queue, move the driver to
>> use this brightness_set_blocking.
>
> There are around 60 drivers in the other kernel subsystems that register
> LED class devices. If we chose the way you proposed then we would have
> to adjust all of them to the LED core changes, which could complicate
> the situation during merge window if there were other modifications in
> the affected drivers.
>
> With my approach all old-fashion drivers will be treated by the
> LED core in the old-fashion way. This is why I introduced
> LED_BRIGHTNESS_BLOCKING flag - only drivers aware of the LED core
> changes will set it, which will allow for the LED core to find out
> which drivers are aware of brightness_set semantics change and
> implement the op accordingly.
>
> This way the change of brightness_set semantics will not be painful.
>
> After all drivers across all subsystems are adapted to the LED core
> changes the flag will be made redundant and we will remove it.
>

Hi Andrew, Pavel and others,

Do you have any comments to my explanation? It would be great if
we could merge this during upcoming merge window, but I'd like to
obtain some acks for the LED core part beforehand.

-- 
Best Regards,
Jacek Anaszewski
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1209840

On Wed, Aug 19, 2015 at 04:03:19PM +0200, Jacek Anaszewski wrote:
> On 08/14/2015 09:57 AM, Jacek Anaszewski wrote:
> >H Andrew,
> >
> >On 08/13/2015 04:15 PM, Andrew Lunn wrote:
> >>On Tue, Aug 11, 2015 at 11:37:14AM +0200, Jacek Anaszewski wrote:
> >>>This patch adds a new brightness_set_nonblocking op to the LED
> >>>subsystem.
> >>>The op is intended for drivers that set brightness in a non-blocking
> >>>way,
> >>>i.e. they neither sleep nor use delays while setting brightness.
> >>>
> >>>Signed-off-by: Jacek Anaszewski <j.anaszewski@samsung.com>
> >>>Cc: Bryan Wu <cooloney@gmail.com>
> >>>Cc: Andrew Lunn <andrew@lunn.ch>
> >>>Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
> >>>Cc: Pavel Machek <pavel@ucw.cz>
> >>>Cc: Stas Sergeev <stsp@users.sourceforge.net>
> >>>---
> >>>  include/linux/leds.h |    3 +++
> >>>  1 file changed, 3 insertions(+)
> >>>
> >>>diff --git a/include/linux/leds.h b/include/linux/leds.h
> >>>index b122eea..c32f1b8 100644
> >>>--- a/include/linux/leds.h
> >>>+++ b/include/linux/leds.h
> >>>@@ -53,6 +53,9 @@ struct led_classdev {
> >>>      /* Must not sleep, use a workqueue if needed */
> >>>      void        (*brightness_set)(struct led_classdev *led_cdev,
> >>>                        enum led_brightness brightness);
> >>>+    /* Intended for drivers that set brightness in a non-blocking
> >>>way */
> >>>+    void (*brightness_set_nonblocking)(struct led_classdev *led_cdev,
> >>>+                      enum led_brightness brightness);
> >>
> >>Hi Jacek
> >>
> >>>From an API design point of view, i'm not sure this is the best way to
> >>go. You now have two calls which do the same thing, with the plan that
> >>you want to invert the meaning of brightness_set, the old well known
> >>API call, sometime later. This inverting the meaning is going to catch
> >>people out and introduce bugs.
> >>
> >>I would rather add a brightness_set_blocking op. Then as you go
> >>thought the drivers stripping out the work queue, move the driver to
> >>use this brightness_set_blocking.
> >
> >There are around 60 drivers in the other kernel subsystems that register
> >LED class devices. If we chose the way you proposed then we would have
> >to adjust all of them to the LED core changes, which could complicate
> >the situation during merge window if there were other modifications in
> >the affected drivers.

You don't need to change anything, if the semantics of
brightness_set() does not change. All current drivers don't
sleep. They use a work queue if needed to ensure they don't
sleep. Hence they are correct.

By adding a new operation, brightness_set_blocking(), you can strip
out this work queue and move to the new op member one driver at a
time. And you can take as long as you want doing this. No flag day
when an API suddenly means something totally different.

	Andrew
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web