[PATCH] led: ledtrig-transient: replace timer_list with hrtimer

[PATCH] led: ledtrig-transient: replace timer_list with hrtimer

[David Lin]

This patch replaces the kernel timer used by led transient trigger as an
one-shot timer with an hrtimer. As Android is moving away from the
obsoleted timed_output to ledtrig-transient for the vibrator HAL,
ledtrig-transient needs to be able to handle the "duration" property to
millisecond precision as modern haptic actuators can be driven in
precisely one cycle (~1 ms) in order to provide a crisp and subtle
feedback.

Cc: Richard Purdie <rpurdie@rpsys.net>
Cc: Jacek Anaszewski <jacek.anaszewski@gmail.com>
Cc: Pavel Machek <pavel@ucw.cz>
Cc: Rob Herring <robh@kernel.org>
Cc: Rom Lemarchand <romlem@google.com>
Cc: Joel Fernandes <joelaf@google.com>
Cc: stable@vger.kernel.org
Signed-off-by: David Lin <dtwlin@google.com>
---
 drivers/leds/trigger/ledtrig-transient.c | 29 +++++++++++++++++------------
 1 file changed, 17 insertions(+), 12 deletions(-)

diff --git a/drivers/leds/trigger/ledtrig-transient.c b/drivers/leds/trigger/ledtrig-transient.c
index 7e6011bd3646..94bb3bfc46e9 100644
--- a/drivers/leds/trigger/ledtrig-transient.c
+++ b/drivers/leds/trigger/ledtrig-transient.c
@@ -23,25 +23,28 @@
 #include <linux/init.h>
 #include <linux/device.h>
 #include <linux/slab.h>
-#include <linux/timer.h>
+#include <linux/hrtimer.h>
 #include <linux/leds.h>
 #include "../leds.h"
 
 struct transient_trig_data {
+	struct led_classdev *led_cdev;
 	int activate;
 	int state;
 	int restore_state;
 	unsigned long duration;
-	struct timer_list timer;
+	struct hrtimer timer;
 };
 
-static void transient_timer_function(unsigned long data)
+static enum hrtimer_restart transient_timer_function(struct hrtimer *timer)
 {
-	struct led_classdev *led_cdev = (struct led_classdev *) data;
-	struct transient_trig_data *transient_data = led_cdev->trigger_data;
+	struct transient_trig_data *transient_data =
+		container_of(timer, struct transient_trig_data, timer);
 
 	transient_data->activate = 0;
-	led_set_brightness_nosleep(led_cdev, transient_data->restore_state);
+	led_set_brightness_nosleep(transient_data->led_cdev,
+				   transient_data->restore_state);
+	return HRTIMER_NORESTART;
 }
 
 static ssize_t transient_activate_show(struct device *dev,
@@ -70,7 +73,7 @@ static ssize_t transient_activate_store(struct device *dev,
 
 	/* cancel the running timer */
 	if (state == 0 && transient_data->activate == 1) {
-		del_timer(&transient_data->timer);
+		hrtimer_cancel(&transient_data->timer);
 		transient_data->activate = state;
 		led_set_brightness_nosleep(led_cdev,
 					transient_data->restore_state);
@@ -84,8 +87,9 @@ static ssize_t transient_activate_store(struct device *dev,
 		led_set_brightness_nosleep(led_cdev, transient_data->state);
 		transient_data->restore_state =
 		    (transient_data->state == LED_FULL) ? LED_OFF : LED_FULL;
-		mod_timer(&transient_data->timer,
-			  jiffies + msecs_to_jiffies(transient_data->duration));
+		hrtimer_start(&transient_data->timer,
+			      ms_to_ktime(transient_data->duration),
+			      HRTIMER_MODE_REL);
 	}
 
 	/* state == 0 && transient_data->activate == 0
@@ -168,6 +172,7 @@ static void transient_trig_activate(struct led_classdev *led_cdev)
 			"unable to allocate transient trigger\n");
 		return;
 	}
+	tdata->led_cdev = led_cdev;
 	led_cdev->trigger_data = tdata;
 
 	rc = device_create_file(led_cdev->dev, &dev_attr_activate);
@@ -182,8 +187,8 @@ static void transient_trig_activate(struct led_classdev *led_cdev)
 	if (rc)
 		goto err_out_state;
 
-	setup_timer(&tdata->timer, transient_timer_function,
-		    (unsigned long) led_cdev);
+	hrtimer_init(&tdata->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
+	tdata->timer.function = transient_timer_function;
 	led_cdev->activated = true;
 
 	return;
@@ -203,7 +208,7 @@ static void transient_trig_deactivate(struct led_classdev *led_cdev)
 	struct transient_trig_data *transient_data = led_cdev->trigger_data;
 
 	if (led_cdev->activated) {
-		del_timer_sync(&transient_data->timer);
+		hrtimer_cancel(&transient_data->timer);
 		led_set_brightness_nosleep(led_cdev,
 					transient_data->restore_state);
 		device_remove_file(led_cdev->dev, &dev_attr_activate);
-- 
2.12.2.816.g2cccc81164-goog

Re: [PATCH] led: ledtrig-transient: replace timer_list with hrtimer

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 890 bytes --]

On Sun 2017-04-23 21:42:54, David Lin wrote:
> This patch replaces the kernel timer used by led transient trigger as an
> one-shot timer with an hrtimer. As Android is moving away from the
> obsoleted timed_output to ledtrig-transient for the vibrator HAL,
> ledtrig-transient needs to be able to handle the "duration" property to
> millisecond precision as modern haptic actuators can be driven in
> precisely one cycle (~1 ms) in order to provide a crisp and subtle
> feedback.

(Insert rant about using LED subsystem for something that is not a
LED; if we are going to these subtleties, LEDs _do_ have  different
properties than vibration motor...).

Otherwise it looks good.

Acked-by: Pavel Machek <pavel@ucw.cz>
								Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH] led: ledtrig-transient: replace timer_list with hrtimer

[Jacek Anaszewski]

Hi David,

Thanks for the patch.

Unfortunately we cannot switch to using hr timers just like that
without introducing side effects for many devices. We had similar
attempt of increasing timer tirgger accuracy two years ago [0].

In short words, for drivers that can sleep while setting brightness
and/or are using a bus like I2C you will not be able to enforce
1ms delay period.

I recommend you to go through the thread [0] so that we had
a well defined ground for the discussion on how to address this
issue properly.

Alternatively, in order to avoid all quirks related to LED subsystem,
I'd propose to implement this feature in the GPIO subsystem, which
seems to be more suitable place for it.

[0] https://lkml.org/lkml/2015/4/28/260

Best regards,
Jacek Anaszewski

On 04/24/2017 06:42 AM, David Lin wrote:
> This patch replaces the kernel timer used by led transient trigger as an
> one-shot timer with an hrtimer. As Android is moving away from the
> obsoleted timed_output to ledtrig-transient for the vibrator HAL,
> ledtrig-transient needs to be able to handle the "duration" property to
> millisecond precision as modern haptic actuators can be driven in
> precisely one cycle (~1 ms) in order to provide a crisp and subtle
> feedback.
> 
> Cc: Richard Purdie <rpurdie@rpsys.net>
> Cc: Jacek Anaszewski <jacek.anaszewski@gmail.com>
> Cc: Pavel Machek <pavel@ucw.cz>
> Cc: Rob Herring <robh@kernel.org>
> Cc: Rom Lemarchand <romlem@google.com>
> Cc: Joel Fernandes <joelaf@google.com>
> Cc: stable@vger.kernel.org
> Signed-off-by: David Lin <dtwlin@google.com>
> ---
>  drivers/leds/trigger/ledtrig-transient.c | 29 +++++++++++++++++------------
>  1 file changed, 17 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/leds/trigger/ledtrig-transient.c b/drivers/leds/trigger/ledtrig-transient.c
> index 7e6011bd3646..94bb3bfc46e9 100644
> --- a/drivers/leds/trigger/ledtrig-transient.c
> +++ b/drivers/leds/trigger/ledtrig-transient.c
> @@ -23,25 +23,28 @@
>  #include <linux/init.h>
>  #include <linux/device.h>
>  #include <linux/slab.h>
> -#include <linux/timer.h>
> +#include <linux/hrtimer.h>
>  #include <linux/leds.h>
>  #include "../leds.h"
>  
>  struct transient_trig_data {
> +	struct led_classdev *led_cdev;
>  	int activate;
>  	int state;
>  	int restore_state;
>  	unsigned long duration;
> -	struct timer_list timer;
> +	struct hrtimer timer;
>  };
>  
> -static void transient_timer_function(unsigned long data)
> +static enum hrtimer_restart transient_timer_function(struct hrtimer *timer)
>  {
> -	struct led_classdev *led_cdev = (struct led_classdev *) data;
> -	struct transient_trig_data *transient_data = led_cdev->trigger_data;
> +	struct transient_trig_data *transient_data =
> +		container_of(timer, struct transient_trig_data, timer);
>  
>  	transient_data->activate = 0;
> -	led_set_brightness_nosleep(led_cdev, transient_data->restore_state);
> +	led_set_brightness_nosleep(transient_data->led_cdev,
> +				   transient_data->restore_state);
> +	return HRTIMER_NORESTART;
>  }
>  
>  static ssize_t transient_activate_show(struct device *dev,
> @@ -70,7 +73,7 @@ static ssize_t transient_activate_store(struct device *dev,
>  
>  	/* cancel the running timer */
>  	if (state == 0 && transient_data->activate == 1) {
> -		del_timer(&transient_data->timer);
> +		hrtimer_cancel(&transient_data->timer);
>  		transient_data->activate = state;
>  		led_set_brightness_nosleep(led_cdev,
>  					transient_data->restore_state);
> @@ -84,8 +87,9 @@ static ssize_t transient_activate_store(struct device *dev,
>  		led_set_brightness_nosleep(led_cdev, transient_data->state);
>  		transient_data->restore_state =
>  		    (transient_data->state == LED_FULL) ? LED_OFF : LED_FULL;
> -		mod_timer(&transient_data->timer,
> -			  jiffies + msecs_to_jiffies(transient_data->duration));
> +		hrtimer_start(&transient_data->timer,
> +			      ms_to_ktime(transient_data->duration),
> +			      HRTIMER_MODE_REL);
>  	}
>  
>  	/* state == 0 && transient_data->activate == 0
> @@ -168,6 +172,7 @@ static void transient_trig_activate(struct led_classdev *led_cdev)
>  			"unable to allocate transient trigger\n");
>  		return;
>  	}
> +	tdata->led_cdev = led_cdev;
>  	led_cdev->trigger_data = tdata;
>  
>  	rc = device_create_file(led_cdev->dev, &dev_attr_activate);
> @@ -182,8 +187,8 @@ static void transient_trig_activate(struct led_classdev *led_cdev)
>  	if (rc)
>  		goto err_out_state;
>  
> -	setup_timer(&tdata->timer, transient_timer_function,
> -		    (unsigned long) led_cdev);
> +	hrtimer_init(&tdata->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
> +	tdata->timer.function = transient_timer_function;
>  	led_cdev->activated = true;
>  
>  	return;
> @@ -203,7 +208,7 @@ static void transient_trig_deactivate(struct led_classdev *led_cdev)
>  	struct transient_trig_data *transient_data = led_cdev->trigger_data;
>  
>  	if (led_cdev->activated) {
> -		del_timer_sync(&transient_data->timer);
> +		hrtimer_cancel(&transient_data->timer);
>  		led_set_brightness_nosleep(led_cdev,
>  					transient_data->restore_state);
>  		device_remove_file(led_cdev->dev, &dev_attr_activate);
> 

Re: [PATCH] led: ledtrig-transient: replace timer_list with hrtimer

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1199 bytes --]

Hi!
> Hi David,
> 
> Thanks for the patch.
> 
> Unfortunately we cannot switch to using hr timers just like that
> without introducing side effects for many devices. We had similar
> attempt of increasing timer tirgger accuracy two years ago [0].
> 
> In short words, for drivers that can sleep while setting brightness
> and/or are using a bus like I2C you will not be able to enforce
> 1ms delay period.
> 
> I recommend you to go through the thread [0] so that we had
> a well defined ground for the discussion on how to address this
> issue properly.
> 
> Alternatively, in order to avoid all quirks related to LED subsystem,
> I'd propose to implement this feature in the GPIO subsystem, which
> seems to be more suitable place for it.

Actually.. make that "implement it in force feedback subsystem where
it belongs". And we actually have force feedback subsystem, already,
see drivers/input/ff-core.c .

(Nokia N900 actually uses that subsystem for the vibration motor, so
there's existing code...)

Thanks,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH] led: ledtrig-transient: replace timer_list with hrtimer

[David Lin]

Hi Jacek,

On Mon, Apr 24, 2017 at 12:59 PM, Jacek Anaszewski
<jacek.anaszewski@gmail.com> wrote:
>
> Hi David,
>
> Thanks for the patch.
>
> Unfortunately we cannot switch to using hr timers just like that
> without introducing side effects for many devices. We had similar
> attempt of increasing timer tirgger accuracy two years ago [0].
>
> In short words, for drivers that can sleep while setting brightness
> and/or are using a bus like I2C you will not be able to enforce
> 1ms delay period.
>
> I recommend you to go through the thread [0] so that we had
> a well defined ground for the discussion on how to address this
> issue properly.
>

I think I understand the background now, and would agree that not all
the LED driver require hrtimer as human eye can't probably tell
there's a 10ms variation in a blink. However, there's a need to
support hrtimer if the LED subsystem claims support the use case of
vibrator (please see Documentation/leds/ledtrig-transient.txt) as even
a 5ms of variation is perceivable to the user. I'm thinking if a
better interim solution is to introduce a
LEDS_TRIGGER_TRANSIENT_HRTIMER config to work with both timers in
compile time. Would you agree?

David

Re: [PATCH] led: ledtrig-transient: replace timer_list with hrtimer

[Jacek Anaszewski]

Hi David,

On 04/25/2017 05:05 AM, David Lin wrote:
> Hi Jacek,
> 
> On Mon, Apr 24, 2017 at 12:59 PM, Jacek Anaszewski
> <jacek.anaszewski@gmail.com> wrote:
>>
>> Hi David,
>>
>> Thanks for the patch.
>>
>> Unfortunately we cannot switch to using hr timers just like that
>> without introducing side effects for many devices. We had similar
>> attempt of increasing timer tirgger accuracy two years ago [0].
>>
>> In short words, for drivers that can sleep while setting brightness
>> and/or are using a bus like I2C you will not be able to enforce
>> 1ms delay period.
>>
>> I recommend you to go through the thread [0] so that we had
>> a well defined ground for the discussion on how to address this
>> issue properly.
>>
> 
> I think I understand the background now, and would agree that not all
> the LED driver require hrtimer as human eye can't probably tell
> there's a 10ms variation in a blink.

The main problem are side effects occurring when an event
scheduled by hrtimer can't finish before the next one begins.
We get warnings like in the example below (copied from [0]) then,
and they have probably negative impact on the whole system performance.

echo "timer" > trigger
echo 1 > delay_on
echo 1 > delay_off
echo usec > delay_unit
[  178.584433] hrtimer: interrupt took 300747 ns

> However, there's a need to
> support hrtimer if the LED subsystem claims support the use case of
> vibrator (please see Documentation/leds/ledtrig-transient.txt) as even
> a 5ms of variation is perceivable to the user. I'm thinking if a
> better interim solution is to introduce a
> LEDS_TRIGGER_TRANSIENT_HRTIMER config to work with both timers in
> compile time. Would you agree?

I think that it would be better if LED class driver set a flag
marking itself as capable of setting brightness with high rate.
I'd limit that only to leds-gpio and devices driven through
memory mapped registers.

Having the flag e.g. LED_BRIGHTNESS_FAST, we could add support for
hr timers also to ledtrig-timer.

You can try also the other option mentioned by Pavel in [1].

[1] https://lkml.org/lkml/2017/4/24/881

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH] led: ledtrig-transient: replace timer_list with hrtimer

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 3362 bytes --]

On Mon 2017-04-24 20:05:59, David Lin wrote:
> Hi Jacek,
> 
> On Mon, Apr 24, 2017 at 12:59 PM, Jacek Anaszewski
> <jacek.anaszewski@gmail.com> wrote:
> >
> > Hi David,
> >
> > Thanks for the patch.
> >
> > Unfortunately we cannot switch to using hr timers just like that
> > without introducing side effects for many devices. We had similar
> > attempt of increasing timer tirgger accuracy two years ago [0].
> >
> > In short words, for drivers that can sleep while setting brightness
> > and/or are using a bus like I2C you will not be able to enforce
> > 1ms delay period.
> >
> > I recommend you to go through the thread [0] so that we had
> > a well defined ground for the discussion on how to address this
> > issue properly.
> >
> 
> I think I understand the background now, and would agree that not all
> the LED driver require hrtimer as human eye can't probably tell
> there's a 10ms variation in a blink. However, there's a need to
> support hrtimer if the LED subsystem claims support the use case of
> vibrator (please see Documentation/leds/ledtrig-transient.txt) as even
> a 5ms of variation is perceivable to the user. I'm thinking if a

I believe we should fix the documentation. It is LED subsystem,
requirements are different, and we _already_ have haptic feedback
subsystem.
								Pavel

IOW, I suggest this: (hmm, and more led->LED is needed, and more
english fixes. Oh well.)

Signed-off-by: Pavel Machek <pavel@ucw.cz>

diff --git a/Documentation/leds/ledtrig-transient.txt b/Documentation/leds/ledtrig-transient.txt
index 3bd38b4..c5cf475 100644
--- a/Documentation/leds/ledtrig-transient.txt
+++ b/Documentation/leds/ledtrig-transient.txt
@@ -16,17 +16,11 @@ set a timer to hold a state, however when user space application crashes or
 goes away without deactivating the timer, the hardware will be left in that
 state permanently.
 
-As a specific example of this use-case, let's look at vibrate feature on
-phones. Vibrate function on phones is implemented using PWM pins on SoC or
-PMIC. There is a need to activate one shot timer to control the vibrate
-feature, to prevent user space crashes leaving the phone in vibrate mode
-permanently causing the battery to drain.
-
 Transient trigger addresses the need for one shot timer activation. The
 transient trigger can be enabled and disabled just like the other leds
 triggers.
 
-When an led class device driver registers itself, it can specify all leds
+When an LED class device driver registers itself, it can specify all leds
 triggers it supports and a default trigger. During registration, activation
 routine for the default trigger gets called. During registration of an led
 class device, the LED state does not change.
@@ -144,7 +138,6 @@ repeat the following step as needed:
 	echo none > trigger
 
 This trigger is intended to be used for for the following example use cases:
- - Control of vibrate (phones, tablets etc.) hardware by user space app.
  - Use of LED by user space app as activity indicator.
  - Use of LED by user space app as a kind of watchdog indicator -- as
        long as the app is alive, it can keep the LED illuminated, if it dies


-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH] led: ledtrig-transient: replace timer_list with hrtimer

[Jacek Anaszewski]

On 04/26/2017 12:34 AM, Pavel Machek wrote:
> On Mon 2017-04-24 20:05:59, David Lin wrote:
>> Hi Jacek,
>>
>> On Mon, Apr 24, 2017 at 12:59 PM, Jacek Anaszewski
>> <jacek.anaszewski@gmail.com> wrote:
>>>
>>> Hi David,
>>>
>>> Thanks for the patch.
>>>
>>> Unfortunately we cannot switch to using hr timers just like that
>>> without introducing side effects for many devices. We had similar
>>> attempt of increasing timer tirgger accuracy two years ago [0].
>>>
>>> In short words, for drivers that can sleep while setting brightness
>>> and/or are using a bus like I2C you will not be able to enforce
>>> 1ms delay period.
>>>
>>> I recommend you to go through the thread [0] so that we had
>>> a well defined ground for the discussion on how to address this
>>> issue properly.
>>>
>>
>> I think I understand the background now, and would agree that not all
>> the LED driver require hrtimer as human eye can't probably tell
>> there's a 10ms variation in a blink. However, there's a need to
>> support hrtimer if the LED subsystem claims support the use case of
>> vibrator (please see Documentation/leds/ledtrig-transient.txt) as even
>> a 5ms of variation is perceivable to the user. I'm thinking if a
> 
> I believe we should fix the documentation. It is LED subsystem,
> requirements are different, and we _already_ have haptic feedback
> subsystem.
> 								Pavel
> 
> IOW, I suggest this: (hmm, and more led->LED is needed, and more
> english fixes. Oh well.)
> 
> Signed-off-by: Pavel Machek <pavel@ucw.cz>
> 
> diff --git a/Documentation/leds/ledtrig-transient.txt b/Documentation/leds/ledtrig-transient.txt
> index 3bd38b4..c5cf475 100644
> --- a/Documentation/leds/ledtrig-transient.txt
> +++ b/Documentation/leds/ledtrig-transient.txt
> @@ -16,17 +16,11 @@ set a timer to hold a state, however when user space application crashes or
>  goes away without deactivating the timer, the hardware will be left in that
>  state permanently.
>  
> -As a specific example of this use-case, let's look at vibrate feature on
> -phones. Vibrate function on phones is implemented using PWM pins on SoC or
> -PMIC. There is a need to activate one shot timer to control the vibrate
> -feature, to prevent user space crashes leaving the phone in vibrate mode
> -permanently causing the battery to drain.
> -
>  Transient trigger addresses the need for one shot timer activation. The
>  transient trigger can be enabled and disabled just like the other leds
>  triggers.
>  
> -When an led class device driver registers itself, it can specify all leds
> +When an LED class device driver registers itself, it can specify all leds

Also: s/leds/LEDs/ :-)


>  triggers it supports and a default trigger. During registration, activation
>  routine for the default trigger gets called. During registration of an led
>  class device, the LED state does not change.
> @@ -144,7 +138,6 @@ repeat the following step as needed:
>  	echo none > trigger
>  
>  This trigger is intended to be used for for the following example use cases:
> - - Control of vibrate (phones, tablets etc.) hardware by user space app.
>   - Use of LED by user space app as activity indicator.
>   - Use of LED by user space app as a kind of watchdog indicator -- as
>         long as the app is alive, it can keep the LED illuminated, if it dies
> 
> 

Ack. Will you submit an official patch?

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH] led: ledtrig-transient: replace timer_list with hrtimer

[David Lin]

On Tue, Apr 25, 2017 at 1:15 PM, Jacek Anaszewski
<jacek.anaszewski@gmail.com> wrote:
>> However, there's a need to
>> support hrtimer if the LED subsystem claims support the use case of
>> vibrator (please see Documentation/leds/ledtrig-transient.txt) as even
>> a 5ms of variation is perceivable to the user. I'm thinking if a
>> better interim solution is to introduce a
>> LEDS_TRIGGER_TRANSIENT_HRTIMER config to work with both timers in
>> compile time. Would you agree?
>
> I think that it would be better if LED class driver set a flag
> marking itself as capable of setting brightness with high rate.
> I'd limit that only to leds-gpio and devices driven through
> memory mapped registers.
>
> Having the flag e.g. LED_BRIGHTNESS_FAST, we could add support for
> hr timers also to ledtrig-timer.

Can I resubmit the patch implementing LED_BRIGHTNESS_FAST using hrtimer?

>
> You can try also the other option mentioned by Pavel in [1].

Thanks, Pavel. It does look like that input-ff is a more appropriate
subsystem for implementing a vibrator/haptics driver. It also seems
that it's relying on the userspace to control the timing of the
play/stop events which is likely to be less accurate than a hrtimer in
the kernel. But it provides more effect control than the LED
subsystem.

Re: [PATCH] led: ledtrig-transient: replace timer_list with hrtimer

[Jacek Anaszewski]

On 04/27/2017 05:48 AM, David Lin wrote:
> On Tue, Apr 25, 2017 at 1:15 PM, Jacek Anaszewski
> <jacek.anaszewski@gmail.com> wrote:
>>> However, there's a need to
>>> support hrtimer if the LED subsystem claims support the use case of
>>> vibrator (please see Documentation/leds/ledtrig-transient.txt) as even
>>> a 5ms of variation is perceivable to the user. I'm thinking if a
>>> better interim solution is to introduce a
>>> LEDS_TRIGGER_TRANSIENT_HRTIMER config to work with both timers in
>>> compile time. Would you agree?
>>
>> I think that it would be better if LED class driver set a flag
>> marking itself as capable of setting brightness with high rate.
>> I'd limit that only to leds-gpio and devices driven through
>> memory mapped registers.
>>
>> Having the flag e.g. LED_BRIGHTNESS_FAST, we could add support for
>> hr timers also to ledtrig-timer.
> 
> Can I resubmit the patch implementing LED_BRIGHTNESS_FAST using hrtimer?

Yeah, but please split the changes into two patches:

1/2 - addition of a flag to linux/leds.h and corresponding update of
      Documentation/leds/leds-class.txt
2/2 - addition of hr timer support to ledtrig-transient.c

-- 
Best regards,
Jacek Anaszewski