Date:	Mon, 16 May 2016 11:21:45 +0200
From:	Jacek Anaszewski <j.anaszewski@...sung.com>
To:	Tony Makkiel <tony.makkiel@...ri.com>
Cc:	Linux LED Subsystem <linux-leds@...r.kernel.org>,
	Stas Sergeev <stsp@...rs.sourceforge.net>,
	Pavel Machek <pavel@....cz>,
	lkml <linux-kernel@...r.kernel.org>
Subject: Re: Brightness control irrespective of blink state.

Hi Tony,

On 05/13/2016 04:20 PM, Tony Makkiel wrote:
>
>
> On 12/05/16 11:26, Jacek Anaszewski wrote:
>> On 05/11/2016 03:42 PM, Tony Makkiel wrote:
>>>
>>>
>>> On 11/05/16 10:41, Jacek Anaszewski wrote:
>>>> On 05/10/2016 06:55 PM, Tony Makkiel wrote:
>>>>>
>>>>>
>>>>> On 10/05/16 14:26, Jacek Anaszewski wrote:
>>>>>> On 05/10/2016 11:36 AM, Tony Makkiel wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 09/05/16 15:45, Jacek Anaszewski wrote:
>>>>>>>> Hi Tony,
>>>>>>>>
>>>>>>>> On 05/09/2016 03:27 PM, Tony Makkiel wrote:
>>>>>>>>> Hi Jacek,
>>>>>>>>>      Thank you for getting back. I updated my kernel to 4.5 and
>>>>>>>>> have
>>>>>>>>> the
>>>>>>>>> updated "led_set_brightness" now.
>>>>>>>>>
>>>>>>>>> It sets
>>>>>>>>>          led_cdev->flags |= LED_BLINK_BRIGHTNESS_CHANGE;
>>>>>>>>>          led_cdev->blink_brightness = brightness;
>>>>>>>>>
>>>>>>>>>      The new implementation requires hardware specific drivers to
>>>>>>>>> poll
>>>>>>>>> for flag change. Shouldn't the led-core driver be calling the
>>>>>>>>> hardware
>>>>>>>>> specific brightness_set (led_set_brightness_nosleep)
>>>>>>>>> irrespective of
>>>>>>>>> the
>>>>>>>>> blink settings?
>>>>>>>>>
>>>>>>>>> Unfortunately, it place additional requirement on drivers, to
>>>>>>>>> implement
>>>>>>>>> a polling mechanism which won't be needed otherwise. Why are the
>>>>>>>>> brightness calls dependent on blink settings?
>>>>>>>>
>>>>>>>> If your question is still:
>>>>>>>>
>>>>>>>> "Is there a reason for rejecting brightness change requests when
>>>>>>>> either of the blink_delays are set?"
>>>>>>>>
>>>>>>>> then the answer is: yes, brightness setting is deferred until
>>>>>>>> the next timer tick to avoid avoid problems in case we are called
>>>>>>>> from hard irq context. It should work fine for software blinking.
>>>>>>>
>>>>>>>
>>>>>>> Sorry, was focused debugging 'hardware accelerated blink' on the
>>>>>>> driver
>>>>>>> I am working on, I missed the software blinking implementation.
>>>>>>>
>>>>>>>>
>>>>>>>> Nonetheless, your question, made it obvious that we have a problem
>>>>>>>> here in case of hardware accelerated blinking, i.e. drivers that
>>>>>>>> implement blink_set op. Is this your use case?
>>>>>>>>
>>>>>>>
>>>>>>> Yes, the brightness requests from user space
>>>>>>> (/sys/class/leds/*/brightness) does not get passed to hardware
>>>>>>> specific
>>>>>>> driver via the blink_set implemented, unless led_cdev->flags is
>>>>>>> polled.
>>>>>>>
>>>>>>>> Anyway, I've noticed a discrepancy between the LED core code and
>>>>>>>> both Documentation/leds/leds-class.txt and comment over
>>>>>>>> blink_set() op
>>>>>>>> in include/linux/leds.h, which say that blinking is deactivated
>>>>>>>> upon setting the brightness again. Many drivers apply this rule.
>>>>>>>>
>>>>>>>> In effect, LED_BLINK_BRIGHTNESS_CHANGE will have to be removed,
>>>>>>>> and your question will be groundless, as changing the blink
>>>>>>>> brightness should be impossible by design.
>>>>>>>>
>>>>>>> In my opinion, disabling blink, when brightness change requested
>>>>>>> doesn't
>>>>>>> sound like the right thing to do. There could be cases in which the
>>>>>>> brightness of the blinking LED needs to be changed.
>>>>>>
>>>>>> It could be accomplished with following sequence:
>>>>>>
>>>>>> $ echo LED_FULL > brightness //set brightness
>>>>>> $ echo "timer" > trigger     //enable blinking with
>>>>>> brightness=LED_FULL
>>>>>> $ echo 1 > brightness        //stop blinking and set brightness to 1
>>>>>> $ echo "timer" > trigger     //enable blinking with brightness=1
>>>>>>
>>>>>> The only drawback here would be interfered blinking rhythm while
>>>>>> resetting blink brightness. Most drivers that implement blink_set
>>>>>> op observe what documentation says and disable blinking when
>>>>>> new brightness is set. Unfortunately, led_set_brightness() after
>>>>>> modifications doesn't take into account drivers that implement
>>>>>> blink_set op. It needs to be fixed, i.e. made compatible with
>>>>>> the docs.
>>>>>>
>>>>>>> Maybe we can let the
>>>>>>> hardware driver deal with the blink request if it has implemented
>>>>>>> brightness_set? The change below seem to work.
>>>>>>>
>>>>>>>
>>>>>>> Subject: [PATCH] led-core: Use hardware blink when available
>>>>>>>
>>>>>>> At present hardware implemented brightness is not called
>>>>>>> if blink delays are set.
>>>>>>>
>>>>>>> Signed-off-by: Tony Makkiel <tony.makkiel@...ri.com>
>>>>>>> ---
>>>>>>>   drivers/leds/led-core.c | 4 +++-
>>>>>>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>>>>>>
>>>>>>> diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
>>>>>>> index 19e1e60d..02dd0f6 100644
>>>>>>> --- a/drivers/leds/led-core.c
>>>>>>> +++ b/drivers/leds/led-core.c
>>>>>>> @@ -208,8 +208,10 @@ void led_set_brightness(struct led_classdev
>>>>>>> *led_cdev,
>>>>>>>       /*
>>>>>>>        * In case blinking is on delay brightness setting
>>>>>>>        * until the next timer tick.
>>>>>>> +     * Or if brightness_set is defined, use the associated
>>>>>>> implementation.
>>>>>>>        */
>>>>>>> -    if (led_cdev->blink_delay_on || led_cdev->blink_delay_off) {
>>>>>>> +    if ((!led_cdev->brightness_set) &&
>>>>>>
>>>>>> s/brightness_set/blink_set/ AFAICT
>>>>>>
>>>>>> It wouldn't cover all cases as the fact that a driver implements
>>>>>> blink_set doesn't necessarily mean that hardware blinking is used
>>>>>> for current blinking parameters. There are drivers that resort to
>>>>>> using software fallback in case the LED controller device doesn't
>>>>>> support requested blink intervals.
>>>>>>
>>>>>> I'm planning on addition of a LED_BLINKING_HW flag, that would
>>>>>> be set after successful execution of blink_set op. It would allow to
>>>>>> distinguish between hardware and software blinking modes reliably.
>>>>>>
>>>>>
>>>>> Did you mean something like
>>>>>
>>>>>
>>>>> diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
>>>>> index 19e1e60d..4a8b46d 100644
>>>>> --- a/drivers/leds/led-core.c
>>>>> +++ b/drivers/leds/led-core.c
>>>>> @@ -210,6 +210,13 @@ void led_set_brightness(struct led_classdev
>>>>> *led_cdev,
>>>>>           * until the next timer tick.
>>>>>           */
>>>>>          if (led_cdev->blink_delay_on || led_cdev->blink_delay_off) {
>>>>> +               if (led_cdev->brightness_set)
>>>>> +                       led_set_brightness_nosleep(led_cdev,
>>>>> brightness);
>>>>
>>>> brightness_set is always initialized, probably you meant blink_set.
>>>
>>> Your solution from your last comment below sounds better (so probably
>>> can skip reading this section).
>>>
>>> But no, It was not a mistake. Actually, copied from
>>> 'led_set_brightness_nopm' in case to protect from any drivers which
>>> doesn't define it. The change should follow existing architecture, and
>>> was hoping to work for both existing and new drivers.
>>
>> Right, brightness_set can remain uninitialized while
>> brightness_set_blocking is provided.
>>
>>> Existing Chip drivers:  Note, the added brightness_set is called only
>>> when the blink is active. The flag LED_BLINKING_HW won't be set, either
>>> because driver is not updated to include the flag, or driver wants
>>> software fallback to deal with it. The problems I can think of is
>>>
>>> -  the software flow will also call brightness_set later when the task
>>> is serviced. But that is with the same values. So shouldn't make
>>> difference to the user.
>>>
>>> New Drivers with brightness support while blinking:- They set brightness
>>> and the flag. They wont need the software fallback. If for any reason
>>> brightness couldn't be set, flag is not set and normal procedure
>>> applies. Again problem I see here is
>>>
>>> -    Additional responsibility on chip drivers to set the flag, if
>>
>> Ah, now I understand your approach.
>>
>>> successfully managed to set brightness while blink is active. This
>>> shouldn't be a problem on new drivers, as they might just set the flag
>>> every time brightness change is requested, irrespective of blink
>>> settings. If they don't set the flag, it falls back to the software flow
>>> as in existing architecture.
>>>
>>>>
>>>>> +
>>>>> +               if (led_cdev->flags & LED_BLINKING_HW) {
>>>>> +                       led_cdev->flags &= ~LED_BLINKING_HW;
>>>>> +                       return;
>>>>> +               }
>>>>
>>>> The dependencies are quite versatile if we wanted to properly implement
>>>> what documentation claims. Setting brightness to any value while
>>>> blinking is on should stop blinking. It was so before the commit:
>>>>
>>>> 76931edd5 ('leds: fix brightness changing when software blinking is
>>>> active')
>>>>
>>>> The problem was that timer trigger remained active after stopping
>>>> blinking, which led us to changing the semantics on new brightness
>>>> set, rather than solving the problem with unregistered trigger.
>>>> This was also against documentation claims, which was overlooked.
>>>>
>>>> I tried yesterday to deactivate trigger on brightness set
>>>> executed during blinking, but there are circular dependencies,
>>>> since led_set_brightness(led_cdev, LED_OFF) is called on deactivation.
>>>> It is also called from led_trigger_set in the trigger removal path.
>>>> Generally it seems non-trivial to enforce current documentation claims
>>>> in case of software blinking.
>>>>
>>>> The easiest thing to do now would be changing the semantics, so that
>>>> only setting brightness to LED_OFF would disable the trigger, which
>>>> in fact is true since few releases. The problem is that many drivers
>>>> that implement hardware blinking resets it on any brightness change.
>>>> We would have to left them intact, but apply a new semantics in the
>>>> LED core, that would allow for new drivers to just update hardware
>>>> blinking brightness upon updating the brightness.
>>>>
>>>> If we followed this path then the LED_BLINKING_HW flag would have to
>>>> be set in led_blink_setup() after blink_set op returns 0. Thanks to
>>>> that, we could distinguish in led_set_brightness whether hardware
>>>> or software blinking is enabled. For !LED_BLINKING_HW case we would
>>>> proceed as currently, i.e. set the LED_BLINK_BRIGHTNESS_CHANGE flag
>>>> and defer brightness setting until next timer tick. For the opposite
>>>> case we wouldn't do anything and let the led_set_brightness_nosleep()
>>>> to call the appropriate brightness_set/brightness_set_blocking op.
>>>> Old drivers would proceed as currently, by disabling blinking
>>>> on brightness change, and new ones could apply new semantics by
>>>> changing brightness but leaving hardware blinking active.
>>>>
>>>
>>> This sounds better as we do not have to rely on drivers to set the flag
>>> and does not have the problems mentioned above. I tried the following
>>> and works for my set up :) .
>>>
>>>
>>> diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
>>> index 19e1e60d..3698b67 100644
>>> --- a/drivers/leds/led-core.c
>>> +++ b/drivers/leds/led-core.c
>>> @@ -141,8 +141,10 @@ static void led_blink_setup(struct led_classdev
>>> *led_cdev,
>>>   {
>>>          if (!(led_cdev->flags & LED_BLINK_ONESHOT) &&
>>>              led_cdev->blink_set &&
>>> -           !led_cdev->blink_set(led_cdev, delay_on, delay_off))
>>> +           !led_cdev->blink_set(led_cdev, delay_on, delay_off)){
>>> +               led_cdev->flags |= LED_BLINKING_HW;
>>>                  return;
>>> +       }
>>>
>>>          /* blink with 1 Hz as default if nothing specified */
>>>          if (!*delay_on && !*delay_off)
>>> @@ -209,7 +211,8 @@ void led_set_brightness(struct led_classdev
>>> *led_cdev,
>>>           * In case blinking is on delay brightness setting
>>>           * until the next timer tick.
>>>           */
>>> -       if (led_cdev->blink_delay_on || led_cdev->blink_delay_off) {
>>> +       if (!(led_cdev->flags & LED_BLINKING_HW) &&
>>> +           (led_cdev->blink_delay_on || led_cdev->blink_delay_off)) {
>>>                  /*
>>>                   * If we need to disable soft blinking delegate this to
>>> the
>>>                   * work queue task to avoid problems in case we are
>>> called
>>
>> We would have to also clear the flag upon blink deactivation, i.e.
>> when brightness to be set equals LED_OFF. Existing drivers that
>> implement blink_set op and deactivate blinking on any brightness set
>> would have to be modified to clear the LED_BLINKING_HW flag in their
>> brightness_{set|set_blocking} ops.
>>
>
>
> diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
> index 19e1e60d..7a15035 100644
> --- a/drivers/leds/led-core.c
> +++ b/drivers/leds/led-core.c
> @@ -98,6 +98,9 @@ static void set_brightness_delayed(struct work_struct
> *ws)
>
> led_cdev->delayed_set_value);
>          else
>                  ret = -ENOTSUPP;
> +
> +       if (led_cdev->delayed_set_value == LED_OFF)
> +               led_cdev->flags &= ~LED_BLINKING_HW;
>          if (ret < 0)
>                  dev_err(led_cdev->dev,
>                          "Setting an LED's brightness failed (%d)\n", ret);
> @@ -141,8 +144,10 @@ static void led_blink_setup(struct led_classdev
> *led_cdev,
>   {
>          if (!(led_cdev->flags & LED_BLINK_ONESHOT) &&
>              led_cdev->blink_set &&
> -           !led_cdev->blink_set(led_cdev, delay_on, delay_off))
> +           !led_cdev->blink_set(led_cdev, delay_on, delay_off)){
> +               led_cdev->flags |= LED_BLINKING_HW;
>                  return;
> +       }
>
>          /* blink with 1 Hz as default if nothing specified */
>          if (!*delay_on && !*delay_off)
> @@ -209,7 +214,8 @@ void led_set_brightness(struct led_classdev *led_cdev,
>           * In case blinking is on delay brightness setting
>           * until the next timer tick.
>           */
> -       if (led_cdev->blink_delay_on || led_cdev->blink_delay_off) {
> +       if (!(led_cdev->flags & LED_BLINKING_HW) &&
> +           (led_cdev->blink_delay_on || led_cdev->blink_delay_off)) {
>                  /*
>                   * If we need to disable soft blinking delegate this to
> the
>                   * work queue task to avoid problems in case we are called
> @@ -235,6 +241,8 @@ void led_set_brightness_nopm(struct led_classdev
> *led_cdev,
>          /* Use brightness_set op if available, it is guaranteed not to
> sleep */
>          if (led_cdev->brightness_set) {
>                  led_cdev->brightness_set(led_cdev, value);
> +               if (value == LED_OFF)
> +                       led_cdev->flags &= ~LED_BLINKING_HW;
>                  return;
>          }
>
> @@ -267,6 +275,9 @@ int led_set_brightness_sync(struct led_classdev
> *led_cdev,
>          if (led_cdev->flags & LED_SUSPENDED)
>                  return 0;
>
> +       if (value == LED_OFF)
> +               led_cdev->flags &= ~LED_BLINKING_HW;
> +
>          if (led_cdev->brightness_set_blocking)
>                  return led_cdev->brightness_set_blocking(led_cdev,
>
> led_cdev->brightness);
> diff --git a/include/linux/leds.h b/include/linux/leds.h
> index bc1476f..f5fa566 100644
> --- a/include/linux/leds.h
> +++ b/include/linux/leds.h
> @@ -48,6 +48,7 @@ struct led_classdev {
>   #define LED_BLINK_DISABLE      (1 << 21)
>   #define LED_SYSFS_DISABLE      (1 << 22)
>   #define LED_DEV_CAP_FLASH      (1 << 23)
> +#define LED_BLINKING_HW        (1 << 24)
>
>          /* Set LED brightness level */
>          /* Must not sleep, use a workqueue if needed */
> ~
>
>
>>> diff --git a/include/linux/leds.h b/include/linux/leds.h
>>> index bc1476f..f5fa566 100644
>>> --- a/include/linux/leds.h
>>> +++ b/include/linux/leds.h
>>> @@ -48,6 +48,7 @@ struct led_classdev {
>>>   #define LED_BLINK_DISABLE      (1 << 21)
>>>   #define LED_SYSFS_DISABLE      (1 << 22)
>>>   #define LED_DEV_CAP_FLASH      (1 << 23)
>>> +#define LED_BLINKING_HW        (1 << 24)
>>>
>>>          /* Set LED brightness level */
>>>          /* Must not sleep, use a workqueue if needed */
>>>
>>>> I wonder if it would be safe to rely on timer_pending() instead of
>>>> introducing LED_BLINKING_HW flag...
>>>>
>>> How would that work? I am assuming this has something to do with
>>> software blink? Does it take hardware blink to account?
>>
>> This way we could distinguish between software and hardware blinking.
>> It wouldn't require modifications in drivers:
>>
>> -       if (led_cdev->blink_delay_on || led_cdev->blink_delay_off) {
>> +       if (timer_pending(&led_cdev->blink_timer) &&
>> +           (led_cdev->blink_delay_on || led_cdev->blink_delay_off)) {
>>                   /*
>>                    * If we need to disable soft blinking delegate this
>>
>> It'd must be verified however if it isn't possible that
>> led_set_brightness is called when timer is already expired,
>> between two ticks.
>
> if blink_set is successful, would work without problem,
>
> When blink_set successful : The timer wont be triggered resulting the
> function to return null all the time. --> No problem here
>
> Software blink : I do not have hardware to actually test this case. I
> tried simulating the case.But going through the code. Following are my
> understanding.

You can test it by leaving blink_set op uninitialized in your driver.

>
> Timer Active (Most of the time) : Work as normal. --> No problem here
>
> Timer Expired :led_set_brightness_nopm called.
> - Case in which brightness == LED_OFF, LED will be turned off.
> led_cdev->blink_delay_on and delay_off will retain its value. The timer
> will keep on running. So it will re-enable back blink. --> LED_OFF will
> be ignored.
>
> - Case in which brightness != LED_OFF, new brightness set and resume
> normal operation.  --> No problem here
>

Thanks for this analysis. I have a new idea - wouldn't it be more robust
if we added LED_BLINKING_SW flag and set it in led_set_software_blink()?

The line

if (led_cdev->blink_delay_on || led_cdev->blink_delay_off)

in led_set_brightness() could be then changed to

if (led_cdev->flags & LED_BLINK_SW) .

LED_BLINK_SW flag would have to be cleared in led_stop_software_blink()
and in the first two conditions in the led_timer_function().

>
>>
>>>>>                  /*
>>>>>                   * If we need to disable soft blinking delegate
>>>>> this to
>>>>> the
>>>>>                   * work queue task to avoid problems in case we are
>>>>> called
>>>>> diff --git a/include/linux/leds.h b/include/linux/leds.h
>>>>> index bc1476f..f5fa566 100644
>>>>> --- a/include/linux/leds.h
>>>>> +++ b/include/linux/leds.h
>>>>> @@ -48,6 +48,7 @@ struct led_classdev {
>>>>>   #define LED_BLINK_DISABLE      (1 << 21)
>>>>>   #define LED_SYSFS_DISABLE      (1 << 22)
>>>>>   #define LED_DEV_CAP_FLASH      (1 << 23)
>>>>> +#define LED_BLINKING_HW        (1 << 24)
>>>>>
>>>>>          /* Set LED brightness level */
>>>>>          /* Must not sleep, use a workqueue if needed */
>>>>>
>>>>>>
>>>>>>> +        (led_cdev->blink_delay_on || led_cdev->blink_delay_off)) {
>>>>>>>           /*
>>>>>>>            * If we need to disable soft blinking delegate this to
>>>>>>> the
>>>>>>>            * work queue task to avoid problems in case we are called
>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>
>>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-leds" in
>>> the body of a message to majordomo@...r.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>
>>>
>>
>>
>
>


-- 
Best regards,
Jacek Anaszewski

Powered by blists - more mailing lists