From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932276AbcELK0n (ORCPT ); Thu, 12 May 2016 06:26:43 -0400 Received: from mailout2.w1.samsung.com ([210.118.77.12]:52759 "EHLO mailout2.w1.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932217AbcELK0i (ORCPT ); Thu, 12 May 2016 06:26:38 -0400 X-AuditID: cbfec7f4-f796c6d000001486-a8-57345a5a901b Message-id: <57345A59.5010607@samsung.com> Date: Thu, 12 May 2016 12:26:33 +0200 From: Jacek Anaszewski User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130804 Thunderbird/17.0.8 MIME-version: 1.0 To: Tony Makkiel Cc: Linux LED Subsystem , Stas Sergeev , Pavel Machek , lkml Subject: Re: Brightness control irrespective of blink state. 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> In-reply-to: <573336D9.7040105@daqri.com> Content-type: text/plain; charset=UTF-8; format=flowed Content-transfer-encoding: 7bit X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFjrFLMWRmVeSWpSXmKPExsVy+t/xa7rRUSbhBmdELS7vmsNmsfXNOkaL u6eOsll09k1jsZi6YDqjA6vHnv/LWD1WrP7O7tF0qp3V4/MmuQCWKC6blNSczLLUIn27BK6M +08/MBf8K6w4ftOxgfFKZBcjJ4eEgInE3In9zBC2mMSFe+vZuhi5OIQEljJKLNi0mRXCecYo MeP4L6Aqdg5eAS2JTg6QehYBVYmWr2fBetkEDCV+vnjNBGKLCkRI/Dm9jxXE5hUQlPgx+R4L iC0ioClxqK2REWQkM8j8bZO6GEESwgI2EnsXf2QHsYUEVrNInHkSCmJzAjVsW9QL1swsYCbx qGUdM4QtL7F5zVvmCYwCs5DsmIWkbBaSsgWMzKsYRVNLkwuKk9JzDfWKE3OLS/PS9ZLzczcx QkL4yw7GxcesDjEKcDAq8fB6PDQOF2JNLCuuzD3EKMHBrCTCqx9pEi7Em5JYWZValB9fVJqT WnyIUZqDRUmcd+6u9yFCAumJJanZqakFqUUwWSYOTqkGRvaCou/ci2VWeTvMzbtQqLXza+Z7 hiVTL4kXXvFiiun+sSX78Ze0TXLapn198xqnejLIv0xSUrGfv8Jv6fbl/pvc9NK5pq5ZF/pm dt0G2ZPfjoi+/hDwb+Hh9HfHmv7lXp55+s3y54Wpcq0Om6NTZ/16t/jatXA7f1PLld3952e3 pNheeMcyca8SS3FGoqEWc1FxIgBLOcfHXQIAAA== Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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/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