From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753215AbcEMOUy (ORCPT ); Fri, 13 May 2016 10:20:54 -0400 Received: from mail-wm0-f54.google.com ([74.125.82.54]:35653 "EHLO mail-wm0-f54.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752948AbcEMOUv (ORCPT ); Fri, 13 May 2016 10:20:51 -0400 Subject: Re: Brightness control irrespective of blink state. To: Jacek Anaszewski References: <1461881020-13964-1-git-send-email-ezequiel@vanguardiasur.com.ar> <1461881020-13964-2-git-send-email-ezequiel@vanguardiasur.com.ar> <57230B26.9010300@samsung.com> <572C5DEE.3070307@samsung.com> <572CE1B0.8040001@daqri.com> <572CE715.6060504@gmail.com> <57309039.3060305@daqri.com> <5730A293.9050209@samsung.com> <5731ABB6.10607@daqri.com> <5731E194.1010004@samsung.com> <57321299.8090603@daqri.com> <5732FE64.6000907@samsung.com> <573336D9.7040105@daqri.com> <57345A59.5010607@samsung.com> Cc: Linux LED Subsystem , Stas Sergeev , Pavel Machek , lkml From: Tony Makkiel Message-ID: <5735E2BD.4070405@daqri.com> Date: Fri, 13 May 2016 15:20:45 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.7.2 MIME-Version: 1.0 In-Reply-To: <57345A59.5010607@samsung.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 >>>>>> --- >>>>>> 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. 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 > >>>> /* >>>> * 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@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html >> >> > >