[PATCH 0/3] led: ledtrig-transient: add support for hrtimer

[PATCH 0/3] led: ledtrig-transient: add support for hrtimer

[David Lin]

Hi,

These patch series add the LED_BRIGHTNESS_FAST flag support for
ledtrig-transient to use hrtimer so that platforms with high-resolution timer
support can have better accuracy in the trigger duration timing. The need for
this support is driven by the fact that Android has removed the timed_ouput [1]
and is now using led-trigger for handling vibrator control which requires the
timer to be accurate up to a millisecond. However, this flag support would also
allow hrtimer to co-exist with the ktimer without causing warning to the
existing drivers [2].

David

[1] https://patchwork.kernel.org/patch/8664831/
[2] https://lkml.org/lkml/2015/4/28/260

David Lin (3):
  leds: Replace flags bit shift with BIT() macros
  leds: Add the LED_BRIGHTNESS_FAST flag
  led: ledtrig-transient: add support for hrtimer

 Documentation/leds/leds-class.txt        |  5 +++
 drivers/leds/trigger/ledtrig-transient.c | 59 +++++++++++++++++++++++++++++---
 include/linux/leds.h                     | 13 +++----
 3 files changed, 66 insertions(+), 11 deletions(-)

-- 
2.13.0.rc0.306.g87b477812d-goog

[PATCH 1/3] leds: Replace flags bit shift with BIT() macros

[David Lin]

This is for readability as well as to avoid checkpatch warnings when
adding new bit flag information in the future.

Signed-off-by: David Lin <dtwlin@google.com>
---
 include/linux/leds.h | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/include/linux/leds.h b/include/linux/leds.h
index 64c56d454f7d..f9d10a9efcbe 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -43,12 +43,12 @@ struct led_classdev {
 #define LED_SUSPENDED		(1 << 0)
 #define LED_UNREGISTERING	(1 << 1)
 	/* Upper 16 bits reflect control information */
-#define LED_CORE_SUSPENDRESUME	(1 << 16)
-#define LED_SYSFS_DISABLE	(1 << 17)
-#define LED_DEV_CAP_FLASH	(1 << 18)
-#define LED_HW_PLUGGABLE	(1 << 19)
-#define LED_PANIC_INDICATOR	(1 << 20)
-#define LED_BRIGHT_HW_CHANGED	(1 << 21)
+#define LED_CORE_SUSPENDRESUME	BIT(16)
+#define LED_SYSFS_DISABLE	BIT(17)
+#define LED_DEV_CAP_FLASH	BIT(18)
+#define LED_HW_PLUGGABLE	BIT(19)
+#define LED_PANIC_INDICATOR	BIT(20)
+#define LED_BRIGHT_HW_CHANGED	BIT(21)
 
 	/* set_brightness_work / blink_timer flags, atomic, private. */
 	unsigned long		work_flags;
-- 
2.13.0.rc0.306.g87b477812d-goog

Re: [PATCH 1/3] leds: Replace flags bit shift with BIT() macros

[Jacek Anaszewski]

Hi David,

On 04/30/2017 11:36 PM, David Lin wrote:
> This is for readability as well as to avoid checkpatch warnings when
> adding new bit flag information in the future.
> 
> Signed-off-by: David Lin <dtwlin@google.com>
> ---
>  include/linux/leds.h | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/include/linux/leds.h b/include/linux/leds.h
> index 64c56d454f7d..f9d10a9efcbe 100644
> --- a/include/linux/leds.h
> +++ b/include/linux/leds.h
> @@ -43,12 +43,12 @@ struct led_classdev {
>  #define LED_SUSPENDED		(1 << 0)
>  #define LED_UNREGISTERING	(1 << 1)

Could we update also these bits?

>  	/* Upper 16 bits reflect control information */
> -#define LED_CORE_SUSPENDRESUME	(1 << 16)
> -#define LED_SYSFS_DISABLE	(1 << 17)
> -#define LED_DEV_CAP_FLASH	(1 << 18)
> -#define LED_HW_PLUGGABLE	(1 << 19)
> -#define LED_PANIC_INDICATOR	(1 << 20)
> -#define LED_BRIGHT_HW_CHANGED	(1 << 21)
> +#define LED_CORE_SUSPENDRESUME	BIT(16)
> +#define LED_SYSFS_DISABLE	BIT(17)
> +#define LED_DEV_CAP_FLASH	BIT(18)
> +#define LED_HW_PLUGGABLE	BIT(19)
> +#define LED_PANIC_INDICATOR	BIT(20)
> +#define LED_BRIGHT_HW_CHANGED	BIT(21)
>  
>  	/* set_brightness_work / blink_timer flags, atomic, private. */
>  	unsigned long		work_flags;
> 

-- 
Best regards,
Jacek Anaszewski

[PATCH 2/3] leds: Add the LED_BRIGHTNESS_FAST flag

[David Lin]

This patch adds the LED_BRIGHTNESS_FAST flag to allow the driver to
indicate that the brightness_set() callback is implemented on a fastpath
so that the LED core may choose to for example use a hrtimer to
implement the duration of a trigger for better timing accuracy.

Suggested-by: Jacek Anaszewski <jacek.anaszewski@gmail.com>
Signed-off-by: David Lin <dtwlin@google.com>
---
 Documentation/leds/leds-class.txt | 5 +++++
 include/linux/leds.h              | 1 +
 2 files changed, 6 insertions(+)

diff --git a/Documentation/leds/leds-class.txt b/Documentation/leds/leds-class.txt
index 836cb16d6f09..70d7a3dca621 100644
--- a/Documentation/leds/leds-class.txt
+++ b/Documentation/leds/leds-class.txt
@@ -80,6 +80,11 @@ flag must be set in flags before registering. Calling
 led_classdev_notify_brightness_hw_changed on a classdev not registered with
 the LED_BRIGHT_HW_CHANGED flag is a bug and will trigger a WARN_ON.
 
+Optionally, the driver may choose to register with the LED_BRIGHTNESS_FAST flag.
+This flag indicates that the driver implements the brightness_set() callback
+function using a fastpath so the LED core can use hrtimer if the driver requires
+high precision for the trigger timing.
+
 Hardware accelerated blink of LEDs
 ==================================
 
diff --git a/include/linux/leds.h b/include/linux/leds.h
index f9d10a9efcbe..78d2880ccd39 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -49,6 +49,7 @@ struct led_classdev {
 #define LED_HW_PLUGGABLE	BIT(19)
 #define LED_PANIC_INDICATOR	BIT(20)
 #define LED_BRIGHT_HW_CHANGED	BIT(21)
+#define LED_BRIGHTNESS_FAST	BIT(22)
 
 	/* set_brightness_work / blink_timer flags, atomic, private. */
 	unsigned long		work_flags;
-- 
2.13.0.rc0.306.g87b477812d-goog

[PATCH 3/3] led: ledtrig-transient: add support for hrtimer

[David Lin]

This patch adds a hrtimer to ledtrig-transient so that when driver is
registered with LED_BRIGHTNESS_FAST, the hrtimer is used for the better
time accuracy in handling the duration.

Signed-off-by: David Lin <dtwlin@google.com>
---
 drivers/leds/trigger/ledtrig-transient.c | 59 +++++++++++++++++++++++++++++---
 1 file changed, 54 insertions(+), 5 deletions(-)

diff --git a/drivers/leds/trigger/ledtrig-transient.c b/drivers/leds/trigger/ledtrig-transient.c
index 7e6011bd3646..63be54772596 100644
--- a/drivers/leds/trigger/ledtrig-transient.c
+++ b/drivers/leds/trigger/ledtrig-transient.c
@@ -24,15 +24,18 @@
 #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 hrtimer;
 };
 
 static void transient_timer_function(unsigned long data)
@@ -44,6 +47,54 @@ static void transient_timer_function(unsigned long data)
 	led_set_brightness_nosleep(led_cdev, transient_data->restore_state);
 }
 
+static enum hrtimer_restart transient_hrtimer_function(struct hrtimer *timer)
+{
+	struct transient_trig_data *transient_data =
+		container_of(timer, struct transient_trig_data, hrtimer);
+
+	transient_timer_function((unsigned long)transient_data->led_cdev);
+
+	return HRTIMER_NORESTART;
+}
+
+static inline void transient_timer_setup(struct led_classdev *led_cdev)
+{
+	struct transient_trig_data *tdata = led_cdev->trigger_data;
+
+	if (led_cdev->flags & LED_BRIGHTNESS_FAST) {
+		tdata->led_cdev = led_cdev;
+		hrtimer_init(&tdata->hrtimer, CLOCK_MONOTONIC,
+			     HRTIMER_MODE_REL);
+		tdata->hrtimer.function = transient_hrtimer_function;
+	} else {
+		setup_timer(&tdata->timer, transient_timer_function,
+			    (unsigned long)led_cdev);
+	}
+}
+
+static inline void transient_timer_start(struct led_classdev *led_cdev)
+{
+	struct transient_trig_data *tdata = led_cdev->trigger_data;
+
+	if (led_cdev->flags & LED_BRIGHTNESS_FAST) {
+		hrtimer_start(&tdata->hrtimer, ms_to_ktime(tdata->duration),
+			      HRTIMER_MODE_REL);
+	} else {
+		mod_timer(&tdata->timer,
+			  jiffies + msecs_to_jiffies(tdata->duration));
+	}
+}
+
+static inline void transient_timer_cancel(struct led_classdev *led_cdev)
+{
+	struct transient_trig_data *tdata = led_cdev->trigger_data;
+
+	if (led_cdev->flags & LED_BRIGHTNESS_FAST)
+		hrtimer_cancel(&tdata->hrtimer);
+	else
+		del_timer_sync(&tdata->timer);
+}
+
 static ssize_t transient_activate_show(struct device *dev,
 		struct device_attribute *attr, char *buf)
 {
@@ -70,7 +121,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);
+		transient_timer_cancel(led_cdev);
 		transient_data->activate = state;
 		led_set_brightness_nosleep(led_cdev,
 					transient_data->restore_state);
@@ -84,8 +135,7 @@ 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));
+		transient_timer_start(led_cdev);
 	}
 
 	/* state == 0 && transient_data->activate == 0
@@ -182,8 +232,7 @@ 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);
+	transient_timer_setup(led_cdev);
 	led_cdev->activated = true;
 
 	return;
-- 
2.13.0.rc0.306.g87b477812d-goog

Re: [PATCH 3/3] led: ledtrig-transient: add support for hrtimer

[Jacek Anaszewski]

Hi David,

Thanks for the patch. I have one reservation below.

On 04/30/2017 11:37 PM, David Lin wrote:
> This patch adds a hrtimer to ledtrig-transient so that when driver is
> registered with LED_BRIGHTNESS_FAST, the hrtimer is used for the better
> time accuracy in handling the duration.
> 
> Signed-off-by: David Lin <dtwlin@google.com>
> ---
>  drivers/leds/trigger/ledtrig-transient.c | 59 +++++++++++++++++++++++++++++---
>  1 file changed, 54 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/leds/trigger/ledtrig-transient.c b/drivers/leds/trigger/ledtrig-transient.c
> index 7e6011bd3646..63be54772596 100644
> --- a/drivers/leds/trigger/ledtrig-transient.c
> +++ b/drivers/leds/trigger/ledtrig-transient.c
> @@ -24,15 +24,18 @@
>  #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 hrtimer;
>  };
>  
>  static void transient_timer_function(unsigned long data)
> @@ -44,6 +47,54 @@ static void transient_timer_function(unsigned long data)
>  	led_set_brightness_nosleep(led_cdev, transient_data->restore_state);
>  }
>  
> +static enum hrtimer_restart transient_hrtimer_function(struct hrtimer *timer)
> +{
> +	struct transient_trig_data *transient_data =
> +		container_of(timer, struct transient_trig_data, hrtimer);
> +
> +	transient_timer_function((unsigned long)transient_data->led_cdev);
> +
> +	return HRTIMER_NORESTART;
> +}
> +
> +static inline void transient_timer_setup(struct led_classdev *led_cdev)
> +{
> +	struct transient_trig_data *tdata = led_cdev->trigger_data;
> +
> +	if (led_cdev->flags & LED_BRIGHTNESS_FAST) {
> +		tdata->led_cdev = led_cdev;
> +		hrtimer_init(&tdata->hrtimer, CLOCK_MONOTONIC,
> +			     HRTIMER_MODE_REL);
> +		tdata->hrtimer.function = transient_hrtimer_function;
> +	} else {
> +		setup_timer(&tdata->timer, transient_timer_function,
> +			    (unsigned long)led_cdev);
> +	}
> +}
> +
> +static inline void transient_timer_start(struct led_classdev *led_cdev)
> +{
> +	struct transient_trig_data *tdata = led_cdev->trigger_data;
> +
> +	if (led_cdev->flags & LED_BRIGHTNESS_FAST) {
> +		hrtimer_start(&tdata->hrtimer, ms_to_ktime(tdata->duration),
> +			      HRTIMER_MODE_REL);
> +	} else {
> +		mod_timer(&tdata->timer,
> +			  jiffies + msecs_to_jiffies(tdata->duration));
> +	}
> +}
> +
> +static inline void transient_timer_cancel(struct led_classdev *led_cdev)
> +{
> +	struct transient_trig_data *tdata = led_cdev->trigger_data;
> +
> +	if (led_cdev->flags & LED_BRIGHTNESS_FAST)
> +		hrtimer_cancel(&tdata->hrtimer);
> +	else
> +		del_timer_sync(&tdata->timer);
> +}

These functions don't seem to be good candidates for marking them with
inline modifier due to below reasons:

- they have more than three lines of code
- their parameters are not compiletime constants
- the functions are static and used only once - in this case
  gcc is capable of inlining them automatically without help

This is concise excerpt from the Documentation/process/coding-style.rst,
chapter 15.

>  static ssize_t transient_activate_show(struct device *dev,
>  		struct device_attribute *attr, char *buf)
>  {
> @@ -70,7 +121,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);
> +		transient_timer_cancel(led_cdev);
>  		transient_data->activate = state;
>  		led_set_brightness_nosleep(led_cdev,
>  					transient_data->restore_state);
> @@ -84,8 +135,7 @@ 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));
> +		transient_timer_start(led_cdev);
>  	}
>  
>  	/* state == 0 && transient_data->activate == 0
> @@ -182,8 +232,7 @@ 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);
> +	transient_timer_setup(led_cdev);
>  	led_cdev->activated = true;
>  
>  	return;
> 

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH 0/3] led: ledtrig-transient: add support for hrtimer

[David Lin]

+Cc: Greg KH

On Sun, Apr 30, 2017 at 2:36 PM, David Lin <dtwlin@google.com> wrote:
> Hi,
>
> These patch series add the LED_BRIGHTNESS_FAST flag support for
> ledtrig-transient to use hrtimer so that platforms with high-resolution timer
> support can have better accuracy in the trigger duration timing. The need for
> this support is driven by the fact that Android has removed the timed_ouput [1]
> and is now using led-trigger for handling vibrator control which requires the
> timer to be accurate up to a millisecond. However, this flag support would also
> allow hrtimer to co-exist with the ktimer without causing warning to the
> existing drivers [2].
>
> David
>
> [1] https://patchwork.kernel.org/patch/8664831/
> [2] https://lkml.org/lkml/2015/4/28/260
>
> David Lin (3):
>   leds: Replace flags bit shift with BIT() macros
>   leds: Add the LED_BRIGHTNESS_FAST flag
>   led: ledtrig-transient: add support for hrtimer
>
>  Documentation/leds/leds-class.txt        |  5 +++
>  drivers/leds/trigger/ledtrig-transient.c | 59 +++++++++++++++++++++++++++++---
>  include/linux/leds.h                     | 13 +++----
>  3 files changed, 66 insertions(+), 11 deletions(-)
>
> --
> 2.13.0.rc0.306.g87b477812d-goog
>

Re: [PATCH 0/3] led: ledtrig-transient: add support for hrtimer

[Pavel Machek]

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

On Sun 2017-04-30 14:36:58, David Lin wrote:
> Hi,
> 
> These patch series add the LED_BRIGHTNESS_FAST flag support for
> ledtrig-transient to use hrtimer so that platforms with high-resolution timer
> support can have better accuracy in the trigger duration timing. The need for
> this support is driven by the fact that Android has removed the timed_ouput [1]
> and is now using led-trigger for handling vibrator control which requires the
> timer to be accurate up to a millisecond. However, this flag support would also
> allow hrtimer to co-exist with the ktimer without causing warning to the
> existing drivers [2].

Yes, and objection still stands: You are misusing LED subsystem for
vibration motors. We already have support for haptic feedback in input
subsystem.

									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 0/3] led: ledtrig-transient: add support for hrtimer

[Jacek Anaszewski]

On 05/08/2017 11:06 PM, Pavel Machek wrote:
> On Sun 2017-04-30 14:36:58, David Lin wrote:
>> Hi,
>>
>> These patch series add the LED_BRIGHTNESS_FAST flag support for
>> ledtrig-transient to use hrtimer so that platforms with high-resolution timer
>> support can have better accuracy in the trigger duration timing. The need for
>> this support is driven by the fact that Android has removed the timed_ouput [1]
>> and is now using led-trigger for handling vibrator control which requires the
>> timer to be accurate up to a millisecond. However, this flag support would also
>> allow hrtimer to co-exist with the ktimer without causing warning to the
>> existing drivers [2].
> 
> Yes, and objection still stands: You are misusing LED subsystem for
> vibration motors. We already have support for haptic feedback in input
> subsystem.

Regardless of whether it is a misuse or not (ledtrig-transient
documentation suggests that it is one of use cases) we have to keep
it as it has been around for a long time and it has userspace users [0].

Moreover there seems to be broad consensus about it among kernel
people [1].


[0]
https://android.googlesource.com/platform%2Fhardware%2Flibhardware/+/61701df363310a5cbd95e3e1638baa9526e42c9b
[1] https://patchwork.kernel.org/patch/8664831/

-- 
Best regards,
Jacek Anaszewski