Messages in this thread Patch in this message | | | Date | Thu, 12 May 2016 12:26:33 +0200 | From | Jacek Anaszewski <> | 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@daqri.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@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > >
-- Best regards, Jacek Anaszewski
| |