led: ledtrig-transient: replace timer_list with hrtimer

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.

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 <[email protected]>

Cc Jacek Anaszewski <[email protected]>

Cc Pavel Machek <[email protected]>

Cc Rob Herring <[email protected]>

Cc Rom Lemarchand <[email protected]>

Cc Joel Fernandes <[email protected]>

Cc [email protected]

Signed-off-by David Lin <[email protected]>

---
 drivers/leds/trigger/ledtrig-transient.c | 29 +++++++++++++++++------------
 1 file changed, 17 insertions(+), 12 deletions(-)

drivers/leds/trigger/ledtrig-transient.c +17 -12

index 7e6011bd3646..94bb3bfc46e9 100644
@@ -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);

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.

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.

Hi Jacek,

Quoted 13 lines · 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.

Quoted 4 lines

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?

Quoted 7 lines

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].

Quoted 13 lines · Jacek Anaszewski

Quoted 7 lines

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.

Quoted 2 lines

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?

Hi Jacek,

Quoted 9 lines · 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].

Quoted 8 lines

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

Quoted 22 lines · David Lin

Hi Jacek,

Quoted 9 lines · 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].

Quoted 8 lines

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.

Quoted 6 lines

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 <[email protected]>

Documentation/leds/ledtrig-transient.txt +0 -2

index 3bd38b4..c5cf475 100644
@@ -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