[PATCH 0/2] leds: blink resolution improvements

[PATCH 0/2] leds: blink resolution improvements

[Stas Sergeev]

Hello.

The following patches improve the precision of led
timer trigger and add the resolution control.
That allows to make PWM brightness control with timer
trigger.

[PATCH 1/2] leds: use hrtimer for blinking

[Stas Sergeev]

Normal timer has a jiffy resolution, usually 10ms.
But leds trigger timer control allows to set the delays with 1ms granularity.
In order to make this to really work we need to use hrtimer.

CC: Bryan Wu <cooloney@gmail.com>
CC: Richard Purdie <rpurdie@rpsys.net>
CC: linux-leds@vger.kernel.org
CC: linux-kernel@vger.kernel.org

Signed-off-by: Stas Sergeev <stsp@users.sourceforge.net>
---
 drivers/leds/led-class.c |   19 ++++++++++++-------
 drivers/leds/led-core.c  |    9 +++++----
 include/linux/leds.h     |    4 ++--
 3 files changed, 19 insertions(+), 13 deletions(-)

diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
index 795ec99..f95ce912 100644
--- a/drivers/leds/led-class.c
+++ b/drivers/leds/led-class.c
@@ -102,20 +102,21 @@ static const struct attribute_group *led_groups[] = {
 	NULL,
 };

-static void led_timer_function(unsigned long data)
+static enum hrtimer_restart led_timer_function(struct hrtimer *timer)
 {
-	struct led_classdev *led_cdev = (void *)data;
+	struct led_classdev *led_cdev = container_of(timer,
+			     struct led_classdev, blink_timer);
 	unsigned long brightness;
 	unsigned long delay;

 	if (!led_cdev->blink_delay_on || !led_cdev->blink_delay_off) {
 		led_set_brightness_async(led_cdev, LED_OFF);
-		return;
+		return HRTIMER_NORESTART;
 	}

 	if (led_cdev->flags & LED_BLINK_ONESHOT_STOP) {
 		led_cdev->flags &= ~LED_BLINK_ONESHOT_STOP;
-		return;
+		return HRTIMER_NORESTART;
 	}

 	brightness = led_get_brightness(led_cdev);
@@ -148,7 +149,10 @@ static void led_timer_function(unsigned long data)
 		}
 	}

-	mod_timer(&led_cdev->blink_timer, jiffies + msecs_to_jiffies(delay));
+	hrtimer_forward(&led_cdev->blink_timer,
+			hrtimer_get_expires(&led_cdev->blink_timer),
+			ms_to_ktime(delay));
+	return HRTIMER_RESTART;
 }

 static void set_brightness_delayed(struct work_struct *ws)
@@ -243,8 +247,9 @@ int led_classdev_register(struct device *parent, struct led_classdev *led_cdev)

 	INIT_WORK(&led_cdev->set_brightness_work, set_brightness_delayed);

-	setup_timer(&led_cdev->blink_timer, led_timer_function,
-		    (unsigned long)led_cdev);
+	hrtimer_init(&led_cdev->blink_timer, CLOCK_MONOTONIC,
+		     HRTIMER_MODE_REL);
+	led_cdev->blink_timer.function = led_timer_function;

 #ifdef CONFIG_LEDS_TRIGGERS
 	led_trigger_set_default(led_cdev);
diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
index 9886dac..2937259 100644
--- a/drivers/leds/led-core.c
+++ b/drivers/leds/led-core.c
@@ -52,7 +52,8 @@ static void led_set_software_blink(struct led_classdev *led_cdev,
 		return;
 	}

-	mod_timer(&led_cdev->blink_timer, jiffies + 1);
+	hrtimer_start(&led_cdev->blink_timer, ktime_set(0, 0),
+		      HRTIMER_MODE_REL);
 }


@@ -76,7 +77,7 @@ void led_blink_set(struct led_classdev *led_cdev,
 		   unsigned long *delay_on,
 		   unsigned long *delay_off)
 {
-	del_timer_sync(&led_cdev->blink_timer);
+	hrtimer_cancel(&led_cdev->blink_timer);

 	led_cdev->flags &= ~LED_BLINK_ONESHOT;
 	led_cdev->flags &= ~LED_BLINK_ONESHOT_STOP;
@@ -91,7 +92,7 @@ void led_blink_set_oneshot(struct led_classdev *led_cdev,
 			   int invert)
 {
 	if ((led_cdev->flags & LED_BLINK_ONESHOT) &&
-	     timer_pending(&led_cdev->blink_timer))
+	     hrtimer_active(&led_cdev->blink_timer))
 		return;

 	led_cdev->flags |= LED_BLINK_ONESHOT;
@@ -108,7 +109,7 @@ EXPORT_SYMBOL(led_blink_set_oneshot);

 void led_stop_software_blink(struct led_classdev *led_cdev)
 {
-	del_timer_sync(&led_cdev->blink_timer);
+	hrtimer_cancel(&led_cdev->blink_timer);
 	led_cdev->blink_delay_on = 0;
 	led_cdev->blink_delay_off = 0;
 }
diff --git a/include/linux/leds.h b/include/linux/leds.h
index f70f84f..68f5a23 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -16,7 +16,7 @@
 #include <linux/mutex.h>
 #include <linux/rwsem.h>
 #include <linux/spinlock.h>
-#include <linux/timer.h>
+#include <linux/hrtimer.h>
 #include <linux/workqueue.h>

 struct device;
@@ -81,7 +81,7 @@ struct led_classdev {
 	const char		*default_trigger;	/* Trigger to use */

 	unsigned long		 blink_delay_on, blink_delay_off;
-	struct timer_list	 blink_timer;
+	struct hrtimer		 blink_timer;
 	int			 blink_brightness;
 	void			(*flash_resume)(struct led_classdev *led_cdev);

-- 
1.7.9.5

[PATCH 2/2] ledtrig-timer: add blink resolution control

[Stas Sergeev]

Add the following resolutions to led trigger timer:
'n' for nanosecond resolution
'u' for microsecond resolution
'm' for millisecond resolution
The default is 'm' for backward compatibility.

This functionality is needed for things like PWM for software
brightness control, because the default mS resolution is not enough
for that tasks.

CC: Bryan Wu <cooloney@gmail.com>
CC: Richard Purdie <rpurdie@rpsys.net>
CC: linux-leds@vger.kernel.org
CC: linux-kernel@vger.kernel.org

Signed-off-by: Stas Sergeev <stsp@users.sourceforge.net>
---
 drivers/leds/led-class.c             |   18 +++++++++++--
 drivers/leds/trigger/ledtrig-timer.c |   49 ++++++++++++++++++++++++++++++++++
 include/linux/leds.h                 |    7 +++++
 3 files changed, 72 insertions(+), 2 deletions(-)

diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
index f95ce912..2cfbb9d 100644
--- a/drivers/leds/led-class.c
+++ b/drivers/leds/led-class.c
@@ -108,6 +108,7 @@ static enum hrtimer_restart led_timer_function(struct hrtimer *timer)
 			     struct led_classdev, blink_timer);
 	unsigned long brightness;
 	unsigned long delay;
+	ktime_t k_delay;

 	if (!led_cdev->blink_delay_on || !led_cdev->blink_delay_off) {
 		led_set_brightness_async(led_cdev, LED_OFF);
@@ -149,9 +150,22 @@ static enum hrtimer_restart led_timer_function(struct hrtimer *timer)
 		}
 	}

+	switch (led_cdev->resolution) {
+	case LED_BLINK_MS:
+		k_delay = ms_to_ktime(delay);
+		break;
+	case LED_BLINK_US:
+		k_delay = ns_to_ktime(delay * 1000);
+		break;
+	case LED_BLINK_NS:
+		k_delay = ns_to_ktime(delay);
+		break;
+	default:
+		/* should not happen */
+		return HRTIMER_NORESTART;
+	}
 	hrtimer_forward(&led_cdev->blink_timer,
-			hrtimer_get_expires(&led_cdev->blink_timer),
-			ms_to_ktime(delay));
+			hrtimer_get_expires(&led_cdev->blink_timer), k_delay);
 	return HRTIMER_RESTART;
 }

diff --git a/drivers/leds/trigger/ledtrig-timer.c b/drivers/leds/trigger/ledtrig-timer.c
index 8d09327..222c755 100644
--- a/drivers/leds/trigger/ledtrig-timer.c
+++ b/drivers/leds/trigger/ledtrig-timer.c
@@ -68,8 +68,51 @@ static ssize_t led_delay_off_store(struct device *dev,
 	return size;
 }

+static ssize_t led_res_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	const char res_suffix[] = {
+		[LED_BLINK_MS] = 'm',
+		[LED_BLINK_US] = 'u',
+		[LED_BLINK_NS] = 'n',
+	};
+	struct led_classdev *led_cdev = dev_get_drvdata(dev);
+
+	return sprintf(buf, "%c\n", res_suffix[led_cdev->resolution]);
+}
+
+static ssize_t led_res_store(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t size)
+{
+	struct led_classdev *led_cdev = dev_get_drvdata(dev);
+	int ret = strlen(buf);
+	/* char and \n */
+	if (ret != 2)
+		return -EINVAL;
+
+	switch (buf[0]) {
+	case 'm':
+		led_cdev->resolution = LED_BLINK_MS;
+		break;
+	case 'u':
+		led_cdev->resolution = LED_BLINK_US;
+		break;
+	case 'n':
+		led_cdev->resolution = LED_BLINK_NS;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	led_blink_set(led_cdev, &led_cdev->blink_delay_on,
+		      &led_cdev->blink_delay_off);
+
+	return ret;
+}
+
 static DEVICE_ATTR(delay_on, 0644, led_delay_on_show, led_delay_on_store);
 static DEVICE_ATTR(delay_off, 0644, led_delay_off_show, led_delay_off_store);
+static DEVICE_ATTR(resolution, 0644, led_res_show, led_res_store);

 static void timer_trig_activate(struct led_classdev *led_cdev)
 {
@@ -83,6 +126,9 @@ static void timer_trig_activate(struct led_classdev *led_cdev)
 	rc = device_create_file(led_cdev->dev, &dev_attr_delay_off);
 	if (rc)
 		goto err_out_delayon;
+	rc = device_create_file(led_cdev->dev, &dev_attr_resolution);
+	if (rc)
+		goto err_out_delayoff;

 	led_blink_set(led_cdev, &led_cdev->blink_delay_on,
 		      &led_cdev->blink_delay_off);
@@ -90,6 +136,8 @@ static void timer_trig_activate(struct led_classdev *led_cdev)

 	return;

+err_out_delayoff:
+	device_remove_file(led_cdev->dev, &dev_attr_delay_off);
 err_out_delayon:
 	device_remove_file(led_cdev->dev, &dev_attr_delay_on);
 }
@@ -99,6 +147,7 @@ static void timer_trig_deactivate(struct led_classdev *led_cdev)
 	if (led_cdev->activated) {
 		device_remove_file(led_cdev->dev, &dev_attr_delay_on);
 		device_remove_file(led_cdev->dev, &dev_attr_delay_off);
+		device_remove_file(led_cdev->dev, &dev_attr_resolution);
 		led_cdev->activated = false;
 	}

diff --git a/include/linux/leds.h b/include/linux/leds.h
index 68f5a23..5e6fe26 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -30,10 +30,17 @@ enum led_brightness {
 	LED_FULL	= 255,
 };

+enum led_blink_resolution {
+	LED_BLINK_MS,
+	LED_BLINK_US,
+	LED_BLINK_NS,
+};
+
 struct led_classdev {
 	const char		*name;
 	enum led_brightness	 brightness;
 	enum led_brightness	 max_brightness;
+	enum led_blink_resolution resolution;
 	int			 flags;

 	/* Lower 16 bits reflect status */
-- 
1.7.9.5

Re: [PATCH 1/2] leds: use hrtimer for blinking

[Jacek Anaszewski]

Hi Stas,

On 22.04.2015 19:06, Stas Sergeev wrote:> 
> Add the following resolutions to led trigger timer:
> 'n' for nanosecond resolution
> 'u' for microsecond resolution
> 'm' for millisecond resolution
> The default is 'm' for backward compatibility.
> 
> This functionality is needed for things like PWM for software
> brightness control, because the default mS resolution is not enough
> for that tasks.
> 
> CC: Bryan Wu <cooloney@gmail.com>
> CC: Richard Purdie <rpurdie@rpsys.net>
> CC: linux-leds@vger.kernel.org
> CC: linux-kernel@vger.kernel.org
> 
> Signed-off-by: Stas Sergeev <stsp@users.sourceforge.net>
> ---
>   drivers/leds/led-class.c             |   18 +++++++++++--
>   drivers/leds/trigger/ledtrig-timer.c |   49
> ++++++++++++++++++++++++++++++++++
> include/linux/leds.h                 |    7 +++++ 3 files changed, 72
> insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
> index f95ce912..2cfbb9d 100644
> --- a/drivers/leds/led-class.c
> +++ b/drivers/leds/led-class.c
> @@ -108,6 +108,7 @@ static enum hrtimer_restart
> led_timer_function(struct hrtimer *timer) struct led_classdev,
> blink_timer); unsigned long brightness;
>   	unsigned long delay;
> +	ktime_t k_delay;
> 
>   	if (!led_cdev->blink_delay_on
> || !led_cdev->blink_delay_off) { led_set_brightness_async(led_cdev,
> LED_OFF); @@ -149,9 +150,22 @@ static enum hrtimer_restart
> led_timer_function(struct hrtimer *timer) }
>   	}
> 
> +	switch (led_cdev->resolution) {
> +	case LED_BLINK_MS:
> +		k_delay = ms_to_ktime(delay);
> +		break;
> +	case LED_BLINK_US:
> +		k_delay = ns_to_ktime(delay * 1000);
> +		break;
> +	case LED_BLINK_NS:
> +		k_delay = ns_to_ktime(delay);
> +		break;
> +	default:
> +		/* should not happen */
> +		return HRTIMER_NORESTART;
> +	}
>   	hrtimer_forward(&led_cdev->blink_timer,
> -			hrtimer_get_expires(&led_cdev->blink_timer),
> -			ms_to_ktime(delay));
> +			hrtimer_get_expires(&led_cdev->blink_timer),
> k_delay); return HRTIMER_RESTART;
>   }
> 
> diff --git a/drivers/leds/trigger/ledtrig-timer.c
> b/drivers/leds/trigger/ledtrig-timer.c index 8d09327..222c755 100644
> --- a/drivers/leds/trigger/ledtrig-timer.c
> +++ b/drivers/leds/trigger/ledtrig-timer.c
> @@ -68,8 +68,51 @@ static ssize_t led_delay_off_store(struct device
> *dev, return size;
>   }
> 
> +static ssize_t led_res_show(struct device *dev,
> +		struct device_attribute *attr, char *buf)
> +{
> +	const char res_suffix[] = {
> +		[LED_BLINK_MS] = 'm',
> +		[LED_BLINK_US] = 'u',
> +		[LED_BLINK_NS] = 'n',
> +	};
> +	struct led_classdev *led_cdev = dev_get_drvdata(dev);

Semantics of sysfs attributes should be self-explanatory.
I propose to rename the attribute to delay_units. Also unit identifiers
could be renamed to milliseconds, microseconds and nanoseconds
respectively. 
Additionally available_delay_units attribute should be added.
The attribute when read shoud return a space separated list of
acceptable delay_units values.

> +	return sprintf(buf, "%c\n",
> res_suffix[led_cdev->resolution]); +}
> +
> +static ssize_t led_res_store(struct device *dev,
> +		struct device_attribute *attr, const char *buf,
> size_t size) +{
> +	struct led_classdev *led_cdev = dev_get_drvdata(dev);
> +	int ret = strlen(buf);
> +	/* char and \n */
> +	if (ret != 2)
> +		return -EINVAL;
> +
> +	switch (buf[0]) {
> +	case 'm':
> +		led_cdev->resolution = LED_BLINK_MS;
> +		break;
> +	case 'u':
> +		led_cdev->resolution = LED_BLINK_US;
> +		break;
> +	case 'n':
> +		led_cdev->resolution = LED_BLINK_NS;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	led_blink_set(led_cdev, &led_cdev->blink_delay_on,
> +		      &led_cdev->blink_delay_off);
> +
> +	return ret;
> +}
> +
>   static DEVICE_ATTR(delay_on, 0644, led_delay_on_show,
> led_delay_on_store); static DEVICE_ATTR(delay_off, 0644,
> led_delay_off_show, led_delay_off_store); +static
> DEVICE_ATTR(resolution, 0644, led_res_show, led_res_store);
> 
>   static void timer_trig_activate(struct led_classdev *led_cdev)
>   {
> @@ -83,6 +126,9 @@ static void timer_trig_activate(struct
> led_classdev *led_cdev) rc = device_create_file(led_cdev->dev,
> &dev_attr_delay_off); if (rc)
>   		goto err_out_delayon;
> +	rc = device_create_file(led_cdev->dev, &dev_attr_resolution);
> +	if (rc)
> +		goto err_out_delayoff;

This attribute should be created only if support for hr timers
is enabled in the kernel config.

>   	led_blink_set(led_cdev, &led_cdev->blink_delay_on,
>   		      &led_cdev->blink_delay_off);
> @@ -90,6 +136,8 @@ static void timer_trig_activate(struct
> led_classdev *led_cdev)
> 
>   	return;
> 
> +err_out_delayoff:
> +	device_remove_file(led_cdev->dev, &dev_attr_delay_off);
>   err_out_delayon:
>   	device_remove_file(led_cdev->dev, &dev_attr_delay_on);
>   }
> @@ -99,6 +147,7 @@ static void timer_trig_deactivate(struct
> led_classdev *led_cdev) if (led_cdev->activated) {
>   		device_remove_file(led_cdev->dev,
> &dev_attr_delay_on); device_remove_file(led_cdev->dev,
> &dev_attr_delay_off);
> +		device_remove_file(led_cdev->dev,
> &dev_attr_resolution); led_cdev->activated = false;
>   	}
> 
> diff --git a/include/linux/leds.h b/include/linux/leds.h
> index 68f5a23..5e6fe26 100644
> --- a/include/linux/leds.h
> +++ b/include/linux/leds.h
> @@ -30,10 +30,17 @@ enum led_brightness {
>   	LED_FULL	= 255,
>   };
> 
> +enum led_blink_resolution {
> +	LED_BLINK_MS,
> +	LED_BLINK_US,
> +	LED_BLINK_NS,
> +};
> +
>   struct led_classdev {
>   	const char		*name;
>   	enum led_brightness	 brightness;
>   	enum led_brightness	 max_brightness;
> +	enum led_blink_resolution resolution;

I'd rename resolution to delay_unit and put it after blink_timer.

Similarly let's rename enum led_blink_resolution to
enum led_blink_delay_unit


>   	int			 flags;
> 
>   	/* Lower 16 bits reflect status */
>

Documentation/leds/leds-class.txt should also be updated in this patch
set.

Please add there information on what CONFIG_* symbols have to be
defined to add support for hr timers.

It would be also nice to have:
- information that not every platform may support hr timers
- description of newly added sysfs attributes
- 

-- 
Best Regards,
Jacek Anaszewski