Date:	Thu, 12 May 2016 12:26:33 +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.

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