[PATCH v2 0/3] leds: blink resolution improvements

[PATCH v2 0/3] leds: blink resolution improvements

[Stas Sergeev]

Hello.

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

[PATCH 1/3] 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 1/3] 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/3] ledtrig-timer: add blink delay_unit control

[Stas Sergeev]

Add delay_unit device attr to specify the timer delay unit.
Implement the following delay units to led trigger timer:
'nsec' for nanosecond delay_unit
'usec' for microsecond delay_unit
'msec' for millisecond delay_unit
The default is 'msec' for backward compatibility.

echo usec > /sys/class/leds/<led>/delay_unit
will specify microsecond delay unit.
Similarly you can do
echo u > /sys/class/leds/<led>/delay_unit
for a shorter notation.

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: Jacek Anaszewski <j.anaszewski81@gmail.com>
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 |   77 ++++++++++++++++++++++++++++++++++
 include/linux/leds.h                 |    7 ++++
 3 files changed, 100 insertions(+), 2 deletions(-)

diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
index f95ce912..602d823 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->delay_unit) {
+	case LED_BLINK_DU_MS:
+		k_delay = ms_to_ktime(delay);
+		break;
+	case LED_BLINK_DU_US:
+		k_delay = ns_to_ktime(delay * 1000);
+		break;
+	case LED_BLINK_DU_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..2838178 100644
--- a/drivers/leds/trigger/ledtrig-timer.c
+++ b/drivers/leds/trigger/ledtrig-timer.c
@@ -68,8 +68,73 @@ static ssize_t led_delay_off_store(struct device *dev,
 	return size;
 }

+#ifdef CONFIG_HIGH_RES_TIMERS
+static ssize_t led_dunit_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	static const char * const delay_units[] = {
+		[LED_BLINK_DU_MS] = "msec",
+		[LED_BLINK_DU_US] = "usec",
+		[LED_BLINK_DU_NS] = "nsec",
+	};
+	struct led_classdev *led_cdev = dev_get_drvdata(dev);
+	int ret = 0;
+	int i, max;
+
+	max = hrtimer_is_hres_active(&led_cdev->blink_timer) ?
+		LED_BLINK_DU_NS : LED_BLINK_DU_MS;
+	for (i = 0; i <= max; i++) {
+		char fmt[16];
+
+		if (led_cdev->delay_unit == i)
+			strcpy(fmt, "[%s]");
+		else
+			strcpy(fmt, "%s");
+		if (i < max)
+			strcat(fmt, " ");
+		else
+			strcat(fmt, "\n");
+		ret += sprintf(buf + ret, fmt, delay_units[i]);
+	}
+	return ret;
+}
+
+static ssize_t led_dunit_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->delay_unit = LED_BLINK_DU_MS;
+		break;
+	case 'u':
+		led_cdev->delay_unit = LED_BLINK_DU_US;
+		break;
+	case 'n':
+		led_cdev->delay_unit = LED_BLINK_DU_NS;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	led_blink_set(led_cdev, &led_cdev->blink_delay_on,
+		      &led_cdev->blink_delay_off);
+
+	return ret;
+}
+#endif
+
 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);
+#ifdef CONFIG_HIGH_RES_TIMERS
+static DEVICE_ATTR(delay_unit, 0644, led_dunit_show, led_dunit_store);
+#endif

 static void timer_trig_activate(struct led_classdev *led_cdev)
 {
@@ -83,6 +148,11 @@ 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;
+#ifdef CONFIG_HIGH_RES_TIMERS
+	rc = device_create_file(led_cdev->dev, &dev_attr_delay_unit);
+	if (rc)
+		goto err_out_delayoff;
+#endif

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

 	return;

+#ifdef CONFIG_HIGH_RES_TIMERS
+err_out_delayoff:
+	device_remove_file(led_cdev->dev, &dev_attr_delay_off);
+#endif
 err_out_delayon:
 	device_remove_file(led_cdev->dev, &dev_attr_delay_on);
 }
@@ -99,6 +173,9 @@ 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);
+#ifdef CONFIG_HIGH_RES_TIMERS
+		device_remove_file(led_cdev->dev, &dev_attr_delay_unit);
+#endif
 		led_cdev->activated = false;
 	}

diff --git a/include/linux/leds.h b/include/linux/leds.h
index 68f5a23..d6bc30f 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -30,6 +30,12 @@ enum led_brightness {
 	LED_FULL	= 255,
 };

+enum led_blink_delay_unit {
+	LED_BLINK_DU_MS,
+	LED_BLINK_DU_US,
+	LED_BLINK_DU_NS,
+};
+
 struct led_classdev {
 	const char		*name;
 	enum led_brightness	 brightness;
@@ -82,6 +88,7 @@ struct led_classdev {

 	unsigned long		 blink_delay_on, blink_delay_off;
 	struct hrtimer		 blink_timer;
+	enum led_blink_delay_unit delay_unit;
 	int			 blink_brightness;
 	void			(*flash_resume)(struct led_classdev *led_cdev);

-- 
1.7.9.5

[PATCH 3/3] leds: update documentation about new delay units

[Stas Sergeev]

This adds the description of /sys/class/leds/<device>/delay_unit.
It allows you to specify the delay unit for the led trigger timer.

CC: Jonathan Corbet <corbet@lwn.net>
CC: linux-doc@vger.kernel.org
CC: linux-kernel@vger.kernel.org

Signed-off-by: Stas Sergeev <stsp@users.sourceforge.net>
---
 Documentation/leds/leds-class.txt |    4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/leds/leds-class.txt b/Documentation/leds/leds-class.txt
index 79699c2..0e5ecf5 100644
--- a/Documentation/leds/leds-class.txt
+++ b/Documentation/leds/leds-class.txt
@@ -23,6 +23,10 @@ parameters and work on a per LED basis. The timer trigger is an example.
 The timer trigger will periodically change the LED brightness between
 LED_OFF and the current brightness setting. The "on" and "off" time can
 be specified via /sys/class/leds/<device>/delay_{on,off} in milliseconds.
+If you want to specify the delay in microseconds or nanoseconds, you can
+write 'usec' or 'nsec' to /sys/class/leds/<device>/delay_unit. That
+functionality is available only if you have high-resolution timers
+configured (check CONFIG_HIGH_RES_TIMERS in your kernel config).
 You can change the brightness value of a LED independently of the timer
 trigger. However, if you set the brightness value to LED_OFF it will
 also disable the timer trigger.
-- 
1.7.9.5

Re: [PATCH v2 0/3] leds: blink resolution improvements

[Pavel Machek]

Hi!

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

Are you sure that is good idea? Doing LED pwm with main cpu is quite harsh...

We already have "brightness" for... well... brightness level.

Would it make sense to have an option (Kconfig?) to do PWM using timer on hardware
that does not do hardware PWM?
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH v2 0/3] leds: blink resolution improvements

[Stas Sergeev]

Hi.

27.04.2015 23:54, Pavel Machek пишет:
> Hi!
>
>> The following patches improve the precision of led
>> timer trigger and add the delay unit control.
>> That allows to make PWM brightness control with timer
>> trigger.
> Are you sure that is good idea? Doing LED pwm with main cpu is quite harsh...
Do you remember the pc-speaker driver? :)
In fact I tried the maximal possible freq on armada-xp
board (it doesn't go above approx 100KHz, perhaps this
all the HW timers can do on that board) and I haven't
measured any noticeable CPU load with htop.

> We already have "brightness" for... well... brightness level.
>
> Would it make sense to have an option (Kconfig?) to do PWM using timer on hardware
> that does not do hardware PWM?
I think it would make sense, but not for the "timer" trigger.
Maybe for "none" trigger, or inventing another trigger called
"pwm".
For the timer trigger I would pretty much like my approach to stay.
The reason is that the PWM I need to do, is not strictly a PWM -
it needs the ON period in range of tens or hundreds of milliseconds,
while the OFF period is in a couple of usecs (or vice-versa). No
generic PWM driver can afford such distribution, so I'd like to have
the "timer" trigger for the unusual things like that.
As I said, your idea can still be implemented for "none" trigger
or some new trigger, so its still all for good.

Re: [PATCH v2 0/3] leds: blink resolution improvements

[Pavel Machek]

Hi!

> >>The following patches improve the precision of led
> >>timer trigger and add the delay unit control.
> >>That allows to make PWM brightness control with timer
> >>trigger.
> >Are you sure that is good idea? Doing LED pwm with main cpu is quite harsh...
> Do you remember the pc-speaker driver? :)

Actually yes, I do, and that one has the same interface as "normal"
sound cards.

> For the timer trigger I would pretty much like my approach to stay.
> The reason is that the PWM I need to do, is not strictly a PWM -
> it needs the ON period in range of tens or hundreds of milliseconds,
> while the OFF period is in a couple of usecs (or vice-versa). No

That is a PWM, right? I see why you'd want to have short "on", but I
don't see why you'd want short "off"...

There's one thing you have to do: having two files, one specifiying
units and second one specifying timeout is not going to work.

What about simply "echo 0.001 > delay_on"?

Thanks,
									Pavel

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

Re: [PATCH v2 0/3] leds: blink resolution improvements

[Stas Sergeev]

30.04.2015 20:30, Pavel Machek пишет:
>> For the timer trigger I would pretty much like my approach to stay.
>> The reason is that the PWM I need to do, is not strictly a PWM -
>> it needs the ON period in range of tens or hundreds of milliseconds,
>> while the OFF period is in a couple of usecs (or vice-versa). No
> That is a PWM, right? I see why you'd want to have short "on", but I
> don't see why you'd want short "off"...
OK so you demand the full details... sigh. :)
Well the board I have, was created by some crazy people.
They wanted to add an external watchdog, and what they
did was to program one of the leds to "heartbeat" and wire
it to the external MCU that controls the power. So when it
heartbeats, watchdog will not re-power the board.
Now I want to use that led as a led, not as a watchdog.
For that I unfortunately need those "almost ON/almost OFF"
timings.
This board is all but linux-friendly. For example I have already
submitted the patches that allow mvneta MAC to work without
MDIO. And there is a lot of fun ahead in getting the rest to work.

> There's one thing you have to do: having two files, one specifiying
> units and second one specifying timeout is not going to work.
Oh but there is already 2: delay_on/delay_off.

> What about simply "echo 0.001 > delay_on"?
This is possible.
But please consider the following reservations:
- There is already 2 files, so you are not going to write settings
atomically anyway. When resolution changes, it might be better
to just reset to the sane defaults (not in my current patch).
- As was already discussed in the same thread, not all drivers
can support sub-ms delays. For these drivers such resolutions
should not be available. With separate file this is naturally
achieved: you either don't create it at all, or list only the possible
resolutions. With your approach you never know whether you
can write 0.0001 or not.
- You will set the delay in ms units. For example for 100us you'll
write 0.1. IMHO it is counter-intuitive: people will make a mistake
and try 0.0001 instead, wrongly assuming that this is in seconds.
And nanoseconds should then better be removed, as writing
nanosecond delay will just require too much zeros.

Now I am not saying I am against that approach.
More like I have already considered it and have not prioritized
it over the one with the new file. But feel free to convince me
that it is better and I'll implement it in the next update.

Re: [PATCH v2 0/3] leds: blink resolution improvements

[Pavel Machek]

> >What about simply "echo 0.001 > delay_on"?
> This is possible.
> But please consider the following reservations:
> - There is already 2 files, so you are not going to write settings
> atomically anyway. When resolution changes, it might be better
> to just reset to the sane defaults (not in my current patch).

Sane defaults would be mandatory, but lets get reasonable interface.

Someone left 1 in delay_on. You want 100 nsec. You echo nsec >
delay_on_units, bang, dead machine, looping in kernel.

Someone left 100 / nsec in delay on. You want one usec. Echo 1 >
delay_on, bang, dead machine.

> - As was already discussed in the same thread, not all drivers
> can support sub-ms delays. For these drivers such resolutions
> should not be available. With separate file this is naturally
> achieved: you either don't create it at all, or list only the possible
> resolutions. With your approach you never know whether you
> can write 0.0001 or not.

Well, so you get back einval. Knowing "unit" is not enough to know how
short delays hw can support.

> - You will set the delay in ms units. For example for 100us you'll
> write 0.1. IMHO it is counter-intuitive: people will make a mistake
> and try 0.0001 instead, wrongly assuming that this is in seconds.
> And nanoseconds should then better be removed, as writing
> nanosecond delay will just require too much zeros.

This is machine-to-machine interface. And users can handle this.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH v2 0/3] leds: blink resolution improvements

[Stas Sergeev]

03.05.2015 13:34, Pavel Machek пишет:
>>> What about simply "echo 0.001 > delay_on"?
>> This is possible.
>> But please consider the following reservations:
>> - There is already 2 files, so you are not going to write settings
>> atomically anyway. When resolution changes, it might be better
>> to just reset to the sane defaults (not in my current patch).
> Sane defaults would be mandatory, but lets get reasonable interface.
>
> Someone left 1 in delay_on. You want 100 nsec. You echo nsec >
> delay_on_units, bang, dead machine, looping in kernel.
That's exactly what the aforementioned "reset to the sane defaults"
should solve: if you change the delay_units, all delays should
reset to the sane defaults. FYI, there was no "delay_on_units",
just "delay_unit" for both ON and OFF delays.

> Someone left 100 / nsec in delay on. You want one usec. Echo 1 >
> delay_on, bang, dead machine.
There are 2 things needed to address this:
1. No interface should allow a michine lock-up. Locking up the
machine by writing 0.000000001 is just as bad as by writing
"nsec" and then "1". So I'll simply remove nsecs, regardless of
what interface I'll end up implementing.
2. Since changing "delay_unit" should reset the delays to the
sane default, the user should always write the delay_unit first,
then the value.

>> - As was already discussed in the same thread, not all drivers
>> can support sub-ms delays. For these drivers such resolutions
>> should not be available. With separate file this is naturally
>> achieved: you either don't create it at all, or list only the possible
>> resolutions. With your approach you never know whether you
>> can write 0.0001 or not.
> Well, so you get back einval. Knowing "unit" is not enough to know how
> short delays hw can support.
The idea was that you can at least find out the order of the
supported delay. But indeed checking for EINVAL looks like a
more robust and precise solution.

>> - You will set the delay in ms units. For example for 100us you'll
>> write 0.1. IMHO it is counter-intuitive: people will make a mistake
>> and try 0.0001 instead, wrongly assuming that this is in seconds.
>> And nanoseconds should then better be removed, as writing
>> nanosecond delay will just require too much zeros.
> This is machine-to-machine interface. And users can handle this.
OK. I think you mostly convinced me that this solution is not
that bad. :) But I am going to add an explicit "Acked-by" then,
to emphasize that it is your idea and not mine.

I'll post a v3 a couple of weeks later, thanks!

Re: [PATCH v2 0/3] leds: blink resolution improvements

[Pavel Machek]

> >>- You will set the delay in ms units. For example for 100us you'll
> >>write 0.1. IMHO it is counter-intuitive: people will make a mistake
> >>and try 0.0001 instead, wrongly assuming that this is in seconds.
> >>And nanoseconds should then better be removed, as writing
> >>nanosecond delay will just require too much zeros.
> >This is machine-to-machine interface. And users can handle this.
> OK. I think you mostly convinced me that this solution is not
> that bad. :) But I am going to add an explicit "Acked-by" then,
> to emphasize that it is your idea and not mine.

Feel free to note that it is my idea, but I'd have to see&like the
patch before you can add Acked-by.

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

Re: [PATCH v2 0/3] leds: blink resolution improvements

[Stas Sergeev]

27.04.2015 23:54, Pavel Machek пишет:
> Hi!
>
>> The following patches improve the precision of led
>> timer trigger and add the delay unit control.
>> That allows to make PWM brightness control with timer
>> trigger.
> Are you sure that is good idea? Doing LED pwm with main cpu is quite harsh...
>
> We already have "brightness" for... well... brightness level.
Btw, your concern can also be addressed in a simple way:
just remove the "brightness" attribute in "timer" mode if the
HW does not support brightness. In some modes you still
need brightness to write either 0 or 255, but in timer mode
it seems absolutely redundant without a HW implementation.
I'd like to create the patch to conditionally disable the
brightness attr in "timer" mode, but I wonder if the change
will have the unexpected compatibility problems...

Re: [PATCH v2 0/3] leds: blink resolution improvements

[Jacek Anaszewski]

Hi Stas,

Have you tested it? I tried it with Samsung M0 board and
my leds-aat1290 driver. It didn't work well. And for small delay
intervals it will not have a chance to work reliably with all drivers,
especially the ones which use mutex in their brightness_set op,
since mutex can sleep.

I am afraid that we have to stay with what we have currently.

On 04/27/2015 07:08 PM, Stas Sergeev wrote:
> Hello.
>
> The following patches improve the precision of led
> timer trigger and add the delay unit control.
> That allows to make PWM brightness control with timer
> trigger.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>


-- 
Best Regards,
Jacek Anaszewski

Re: [PATCH v2 0/3] leds: blink resolution improvements

[Stas Sergeev]

28.04.2015 11:57, Jacek Anaszewski пишет:
> Hi Stas,
> 
> Have you tested it?
Of course I did.
Works with gpio driver and provides up to 10usec precision on
armada-xp board.
This is 1000 times better than without my patch - the precision
was 10ms (jiffy).

> I tried it with Samsung M0 board and
> my leds-aat1290 driver. It didn't work well. And for small delay
> intervals it will not have a chance to work reliably with all drivers,
> especially the ones which use mutex in their brightness_set op,
> since mutex can sleep.
OK, I can remove the nsec resolution.
I added it for the future, it doesn't work for me either, but
hrtimer has an API for it, so I thought it may work on another hw.
I don't see why it can't stay, but if it bothers you that much,
I'll remove it.

> I am afraid that we have to stay with what we have currently.
This is a counter-productive conclusion.
My patch does 1000 times precision improvement with gpio driver.
If you only want nsec to be removed as it doesn't work - that's
fine with me.

Re: [PATCH v2 0/3] leds: blink resolution improvements

[Jacek Anaszewski]

On 04/28/2015 12:12 PM, Stas Sergeev wrote:
> 28.04.2015 11:57, Jacek Anaszewski пишет:
>> Hi Stas,
>>
>> Have you tested it?
> Of course I did.
> Works with gpio driver and provides up to 10usec precision on
> armada-xp board.
> This is 1000 times better than without my patch - the precision
> was 10ms (jiffy).

Please take into account that this could work reliably only for gpio
LEDs. For the LEDs driven though a bus (e.g. I2C) delays below 1ms
might be hard to achieve. The minimum available delay would vary from
driver to driver.

We could think of adding the hr_timer mode to the led-class.
The mode could be turned on with use of a new led_set_high_res_timer
API. The API would be called by drivers/leds/leds-gpio.c driver when
a dedicated sysfs attribute was set adequately.
The other drivers could also set this mode if they controlled device
with a suitable LED switching rate. The minimum delay value could
be made configurable by the driver and readable through sysfs
when in hr_timer mode.

>> I tried it with Samsung M0 board and
>> my leds-aat1290 driver. It didn't work well. And for small delay
>> intervals it will not have a chance to work reliably with all drivers,
>> especially the ones which use mutex in their brightness_set op,
>> since mutex can sleep.
> OK, I can remove the nsec resolution.

usec also didn't work, please look at my use case and warning:

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

Only some time later I realized that for AAT1290 brightness is set
through ASCwire pulse protocol, which takes few ms.

Please note that with this approach users would have to wonder why
they are getting the warnings and why they can't get their LEDs to work
with given settings.

> I added it for the future, it doesn't work for me either, but
> hrtimer has an API for it, so I thought it may work on another hw.
> I don't see why it can't stay, but if it bothers you that much,
> I'll remove it.
>
>> I am afraid that we have to stay with what we have currently.
> This is a counter-productive conclusion.
 >
> My patch does 1000 times precision improvement with gpio driver.
> If you only want nsec to be removed as it doesn't work - that's
> fine with me.
>


-- 
Best Regards,
Jacek Anaszewski

Re: [PATCH v2 0/3] leds: blink resolution improvements

[Stas Sergeev]

28.04.2015 15:58, Jacek Anaszewski пишет:
> On 04/28/2015 12:12 PM, Stas Sergeev wrote:
>> 28.04.2015 11:57, Jacek Anaszewski пишет:
>>> Hi Stas,
>>>
>>> Have you tested it?
>> Of course I did.
>> Works with gpio driver and provides up to 10usec precision on
>> armada-xp board.
>> This is 1000 times better than without my patch - the precision
>> was 10ms (jiffy).
> Please take into account that this could work reliably only for gpio
> LEDs. For the LEDs driven though a bus (e.g. I2C) delays below 1ms
> might be hard to achieve. The minimum available delay would vary from
> driver to driver.
> 
> We could think of adding the hr_timer mode to the led-class.
> The mode could be turned on with use of a new led_set_high_res_timer
> API. The API would be called by drivers/leds/leds-gpio.c driver when
> a dedicated sysfs attribute was set adequately.
> The other drivers could also set this mode if they controlled device
> with a suitable LED switching rate. The minimum delay value could
> be made configurable by the driver and readable through sysfs
> when in hr_timer mode.
Why such a complexity?
Wouldn't it be enough if the driver can set the minimum delay
value, yet to always use the hrtimer?
Please note that 10ms (jiffy) is an inadequate minimum delay
pretty much for any driver, I guess. At least 1ms should be possible,
because, well, you can write 1ms to sysfs attribute even without
my patch. So even the existing options need hrtimer to work right.

>>> I tried it with Samsung M0 board and
>>> my leds-aat1290 driver. It didn't work well. And for small delay
>>> intervals it will not have a chance to work reliably with all drivers,
>>> especially the ones which use mutex in their brightness_set op,
>>> since mutex can sleep.
>> OK, I can remove the nsec resolution.
> usec also didn't work, please look at my use case and warning:
> 
> echo "timer" > trigger
> echo 1 > delay_on
> echo 1 > delay_off
> echo usec > delay_unit
> [  178.584433] hrtimer: interrupt took 300747 ns
I think I should try a tasklet then.

> Only some time later I realized that for AAT1290 brightness is set
> through ASCwire pulse protocol, which takes few ms.
> 
> Please note that with this approach users would have to wonder why
> they are getting the warnings and why they can't get their LEDs to work
> with given settings.
OK, so we need a tasklet and the ability for the driver to set the
minimal delay. I think I can implement the former and hope someone
else will later implement the later. :)
Lets just solve the problem step-by-step. I can't solve all the
problems at once, but you can't deny the fact that the problem
exists and needs to be solved.

Re: [PATCH v2 0/3] leds: blink resolution improvements

[Jacek Anaszewski]

On 04/28/2015 03:26 PM, Stas Sergeev wrote:
> 28.04.2015 15:58, Jacek Anaszewski пишет:
>> On 04/28/2015 12:12 PM, Stas Sergeev wrote:
>>> 28.04.2015 11:57, Jacek Anaszewski пишет:
>>>> Hi Stas,
>>>>
>>>> Have you tested it?
>>> Of course I did.
>>> Works with gpio driver and provides up to 10usec precision on
>>> armada-xp board.
>>> This is 1000 times better than without my patch - the precision
>>> was 10ms (jiffy).
>> Please take into account that this could work reliably only for gpio
>> LEDs. For the LEDs driven though a bus (e.g. I2C) delays below 1ms
>> might be hard to achieve. The minimum available delay would vary from
>> driver to driver.
>>
>> We could think of adding the hr_timer mode to the led-class.
>> The mode could be turned on with use of a new led_set_high_res_timer
>> API. The API would be called by drivers/leds/leds-gpio.c driver when
>> a dedicated sysfs attribute was set adequately.
>> The other drivers could also set this mode if they controlled device
>> with a suitable LED switching rate. The minimum delay value could
>> be made configurable by the driver and readable through sysfs
>> when in hr_timer mode.
> Why such a complexity?
> Wouldn't it be enough if the driver can set the minimum delay
> value, yet to always use the hrtimer?

Hr timers would be advantageous only for leds-gpio AFAICS. There is no
reason for making them default for all LED class drivers. I assume that
they provide higher resolution at a cost of consuming more CPU/system
resources.

> Please note that 10ms (jiffy) is an inadequate minimum delay
> pretty much for any driver, I guess. At least 1ms should be possible,
> because, well, you can write 1ms to sysfs attribute even without
> my patch. So even the existing options need hrtimer to work right.

As I mentioned before even with hr timer it wouldn't be possible
to assure that 1ms is achievable due to locking and bus latency
reasons.

>>>> I tried it with Samsung M0 board and
>>>> my leds-aat1290 driver. It didn't work well. And for small delay
>>>> intervals it will not have a chance to work reliably with all drivers,
>>>> especially the ones which use mutex in their brightness_set op,
>>>> since mutex can sleep.
>>> OK, I can remove the nsec resolution.
>> usec also didn't work, please look at my use case and warning:
>>
>> echo "timer" > trigger
>> echo 1 > delay_on
>> echo 1 > delay_off
>> echo usec > delay_unit
>> [  178.584433] hrtimer: interrupt took 300747 ns
> I think I should try a tasklet then.
>
>> Only some time later I realized that for AAT1290 brightness is set
>> through ASCwire pulse protocol, which takes few ms.
>>
>> Please note that with this approach users would have to wonder why
>> they are getting the warnings and why they can't get their LEDs to work
>> with given settings.
> OK, so we need a tasklet and the ability for the driver to set the
> minimal delay. I think I can implement the former and hope someone
> else will later implement the later. :)
> Lets just solve the problem step-by-step. I can't solve all the
> problems at once, but you can't deny the fact that the problem
> exists and needs to be solved.
>

This seems not to be an urgent issue. Therefore it is better to
provide complex solution in one patch set.


-- 
Best Regards,
Jacek Anaszewski

Re: [PATCH v2 0/3] leds: blink resolution improvements

[Stas Sergeev]

28.04.2015 15:58, Jacek Anaszewski пишет:
> On 04/28/2015 12:12 PM, Stas Sergeev wrote:
>> 28.04.2015 11:57, Jacek Anaszewski пишет:
>>> Hi Stas,
>>>
>>> Have you tested it?
>> Of course I did.
>> Works with gpio driver and provides up to 10usec precision on
>> armada-xp board.
>> This is 1000 times better than without my patch - the precision
>> was 10ms (jiffy).
> 
> Please take into account that this could work reliably only for gpio
> LEDs. For the LEDs driven though a bus (e.g. I2C) delays below 1ms
> might be hard to achieve. The minimum available delay would vary from
> driver to driver.
> 
> We could think of adding the hr_timer mode to the led-class.
> The mode could be turned on with use of a new led_set_high_res_timer
> API. The API would be called by drivers/leds/leds-gpio.c driver when
> a dedicated sysfs attribute was set adequately.
> The other drivers could also set this mode if they controlled device
> with a suitable LED switching rate. The minimum delay value could
> be made configurable by the driver and readable through sysfs
> when in hr_timer mode.
I've found the SET_BRIGHTNESS_ASYNC and SET_BRIGHTNESS_SYNC flags.
Sounds interesting for my patch, but the only "documentation" I was
able to find, is this:
---
        /* Setting a torch brightness needs to have immediate effect */
        led_cdev->flags &= ~SET_BRIGHTNESS_ASYNC;
        led_cdev->flags |= SET_BRIGHTNESS_SYNC;
---
Aren't these flags mutually exclusive, and so just one could have
been used?
Anyway, from that comment I can try to guess that if the driver
supports ASYNC mode, it should be fast enough and without sleeps.
The drivers that do i2c transfers with sleeps, should be using SYNC
mode. Or was the intention for these flags entirely different?

My intention is to use either a work-queue or a direct hrtimer
callback, depending on whether the driver supports sync or async
mode. This is instead of the driver being able to set the minimum
delay - much simpler to implement. Makes sense?
Can I use the above flags for that purpose, or will I need
a new one?

Re: [PATCH v2 0/3] leds: blink resolution improvements

[Jacek Anaszewski]

On 04/29/2015 01:26 PM, Stas Sergeev wrote:
> 28.04.2015 15:58, Jacek Anaszewski пишет:
>> On 04/28/2015 12:12 PM, Stas Sergeev wrote:
>>> 28.04.2015 11:57, Jacek Anaszewski пишет:
>>>> Hi Stas,
>>>>
>>>> Have you tested it?
>>> Of course I did.
>>> Works with gpio driver and provides up to 10usec precision on
>>> armada-xp board.
>>> This is 1000 times better than without my patch - the precision
>>> was 10ms (jiffy).
>>
>> Please take into account that this could work reliably only for gpio
>> LEDs. For the LEDs driven though a bus (e.g. I2C) delays below 1ms
>> might be hard to achieve. The minimum available delay would vary from
>> driver to driver.
>>
>> We could think of adding the hr_timer mode to the led-class.
>> The mode could be turned on with use of a new led_set_high_res_timer
>> API. The API would be called by drivers/leds/leds-gpio.c driver when
>> a dedicated sysfs attribute was set adequately.
>> The other drivers could also set this mode if they controlled device
>> with a suitable LED switching rate. The minimum delay value could
>> be made configurable by the driver and readable through sysfs
>> when in hr_timer mode.
> I've found the SET_BRIGHTNESS_ASYNC and SET_BRIGHTNESS_SYNC flags.
> Sounds interesting for my patch, but the only "documentation" I was
> able to find, is this:
> ---
>          /* Setting a torch brightness needs to have immediate effect */
>          led_cdev->flags &= ~SET_BRIGHTNESS_ASYNC;
>          led_cdev->flags |= SET_BRIGHTNESS_SYNC;
> ---
> Aren't these flags mutually exclusive, and so just one could have
> been used?

Yes they are.

> Anyway, from that comment I can try to guess that if the driver
> supports ASYNC mode, it should be fast enough and without sleeps.
> The drivers that do i2c transfers with sleeps, should be using SYNC
> mode. Or was the intention for these flags entirely different?

Sync mode was designed for flash LED devices, for which turning
torch immediately has a big importance because they are often
synchronized with camera sensor.

Triggers use led_set_brightness_async API anyway.

> My intention is to use either a work-queue or a direct hrtimer
> callback, depending on whether the driver supports sync or async
> mode. This is instead of the driver being able to set the minimum
> delay - much simpler to implement. Makes sense?
> Can I use the above flags for that purpose, or will I need
> a new one?
>

These flags are intended for internal LED core use.


-- 
Best Regards,
Jacek Anaszewski

Re: [PATCH v2 0/3] leds: blink resolution improvements

[Stas Sergeev]

28.04.2015 15:58, Jacek Anaszewski пишет:
>>> I tried it with Samsung M0 board and
>>> my leds-aat1290 driver. It didn't work well. And for small delay
>>> intervals it will not have a chance to work reliably with all drivers,
>>> especially the ones which use mutex in their brightness_set op,
>>> since mutex can sleep.
>> OK, I can remove the nsec resolution.
> 
> usec also didn't work, please look at my use case and warning:
> 
> echo "timer" > trigger
> echo 1 > delay_on
> echo 1 > delay_off
> echo usec > delay_unit
> [  178.584433] hrtimer: interrupt took 300747 ns
> 
> Only some time later I realized that for AAT1290 brightness is set
> through ASCwire pulse protocol, which takes few ms.
> 
> Please note that with this approach users would have to wonder why
> they are getting the warnings and why they can't get their LEDs to work
> with given settings.
I've now found that the drivers itself use a work queue
when needed. And some drivers, like leds_gpio, even do this:
---
        if (led_dat->can_sleep) {
                led_dat->new_level = level;
                schedule_work(&led_dat->work);
        } else {
                set_brightness_now();
        }
---
So it seems the problem is already solved on the per-driver
basis. I don't have leds-aat1290 driver, it is probably not
in the kernel. It is likely forgetting to use the work-queue
the way all other drivers do. So I think my patch is good for
the in-kernel drivers.

There is also a led_cdev->set_brightness_work, and it looks
unused. I could use it for my patch, but for what, if the
drivers already use the work queue when needed?

Re: [PATCH v2 0/3] leds: blink resolution improvements

[Jacek Anaszewski]

On 04/30/2015 07:11 PM, Stas Sergeev wrote:
> 28.04.2015 15:58, Jacek Anaszewski пишет:
>>>> I tried it with Samsung M0 board and
>>>> my leds-aat1290 driver. It didn't work well. And for small delay
>>>> intervals it will not have a chance to work reliably with all drivers,
>>>> especially the ones which use mutex in their brightness_set op,
>>>> since mutex can sleep.
>>> OK, I can remove the nsec resolution.
>>
>> usec also didn't work, please look at my use case and warning:
>>
>> echo "timer" > trigger
>> echo 1 > delay_on
>> echo 1 > delay_off
>> echo usec > delay_unit
>> [  178.584433] hrtimer: interrupt took 300747 ns
>>
>> Only some time later I realized that for AAT1290 brightness is set
>> through ASCwire pulse protocol, which takes few ms.
>>
>> Please note that with this approach users would have to wonder why
>> they are getting the warnings and why they can't get their LEDs to work
>> with given settings.
> I've now found that the drivers itself use a work queue
> when needed. And some drivers, like leds_gpio, even do this:
> ---
>          if (led_dat->can_sleep) {
>                  led_dat->new_level = level;
>                  schedule_work(&led_dat->work);
>          } else {
>                  set_brightness_now();
>          }
> ---

This is to handle GPIO expander chips, for which gpio set/get functions
must sleep while waiting for I2C or SPI transfer completion.

> So it seems the problem is already solved on the per-driver
> basis. I don't have leds-aat1290 driver, it is probably not
> in the kernel.

It is currently on linux-next/master branch.

> It is likely forgetting to use the work-queue
> the way all other drivers do. So I think my patch is good for
> the in-kernel drivers.
>
> There is also a led_cdev->set_brightness_work, and it looks
> unused. I could use it for my patch, but for what, if the
> drivers already use the work queue when needed?

It is used in led_set_brightness function.

I think that using work queues would compromise the whole idea of
introducing intervals less than 1ms. After the task is delegated to
work queue we are losing the control over the moment when it will get
executed.

I am becoming reluctant towards the whole idea, as we will be
unable to guarantee the stability of a delay interval.

-- 
Best Regards,
Jacek Anaszewski

Re: [PATCH v2 0/3] leds: blink resolution improvements

[Stas Sergeev]

04.05.2015 10:55, Jacek Anaszewski пишет:
>> So it seems the problem is already solved on the per-driver
>> basis. I don't have leds-aat1290 driver, it is probably not
>> in the kernel.
> It is currently on linux-next/master branch.
So that driver is not in line with others.

>> It is likely forgetting to use the work-queue
>> the way all other drivers do. So I think my patch is good for
>> the in-kernel drivers.
>>
>> There is also a led_cdev->set_brightness_work, and it looks
>> unused. I could use it for my patch, but for what, if the
>> drivers already use the work queue when needed?
> It is used in led_set_brightness function.
Only under that condition:
---
if (led_cdev->blink_delay_on || led_cdev->blink_delay_off) {
         led_cdev->delayed_set_value = brightness;
         schedule_work(&led_cdev->set_brightness_work);
---

But the main condition is:
---
if (led_cdev->flags & SET_BRIGHTNESS_ASYNC) {
         led_set_brightness_async(led_cdev, brightness);
---

So I think it is actually unused.
I don't see why schedule_work() above can't be just replaced
with led_set_brightness_async(). Is there the reason not to do so?

> I think that using work queues would compromise the whole idea of
> introducing intervals less than 1ms. After the task is delegated to
> work queue we are losing the control over the moment when it will get
> executed.
No one is going to allow sub-ms intervals when work-queue
is used. The proper solution would be to use work-queue for
drivers that can sleep, and allow sub-ms resolution for others.
Fortunately the drivers seem to already have that information
internally, and use work-queue on their own when needed.
leds-aat1290 may be an exception from that.

> I am becoming reluctant towards the whole idea, as we will be
> unable to guarantee the stability of a delay interval.
So why are you against the idea of improving the precision,
rather than against the code that prevents us from doing so?
The per-driver work queue use can be moved to led-core,
and the precise intervals can be guaranteed for the drivers
that do not need work queue.
Now your leds-aat1290 already asks for such a change,
because it can sleep but does not use a work-queue the
way other drivers do.
If I do such a massive change, I will certainly not be able
to properly test it, while you have a good test-case and
even a driver that needs such a change anyway. So I don't
see the point of being against that.

So what should we do?
I can try the aforementioned massive clean-up with removing
the work-queue from every driver and using the one in
led-core, but my attempts have few chances to succeed
because of no test-cases. Or can you do this instead, so
that leds-aat1290 driver is in line with the others? Or any
other options I can try?

Re: [PATCH v2 0/3] leds: blink resolution improvements

[Jacek Anaszewski]

On 05/04/2015 02:12 PM, Stas Sergeev wrote:
> 04.05.2015 10:55, Jacek Anaszewski пишет:
>>> So it seems the problem is already solved on the per-driver
>>> basis. I don't have leds-aat1290 driver, it is probably not
>>> in the kernel.
>> It is currently on linux-next/master branch.
> So that driver is not in line with others.
>
>>> It is likely forgetting to use the work-queue
>>> the way all other drivers do. So I think my patch is good for
>>> the in-kernel drivers.
>>>
>>> There is also a led_cdev->set_brightness_work, and it looks
>>> unused. I could use it for my patch, but for what, if the
>>> drivers already use the work queue when needed?
>> It is used in led_set_brightness function.
> Only under that condition:
> ---
> if (led_cdev->blink_delay_on || led_cdev->blink_delay_off) {
>          led_cdev->delayed_set_value = brightness;
>          schedule_work(&led_cdev->set_brightness_work);
> ---
>
> But the main condition is:
> ---
> if (led_cdev->flags & SET_BRIGHTNESS_ASYNC) {
>          led_set_brightness_async(led_cdev, brightness);
> ---
>
> So I think it is actually unused.
> I don't see why schedule_work() above can't be just replaced
> with led_set_brightness_async(). Is there the reason not to do so?

set_brightness_work not only sets the brightness but also
stops software blinking, which was the primary reason
for adding this work queue I think. Here is the commit message:

------------------------

     leds: delay led_set_brightness if stopping soft-blink

     Delay execution of led_set_brightness() if need to stop soft-blink
     timer.

     This allows led_set_brightness to be called in hard-irq context even if
     soft-blink was activated on that LED.

------------------------


Moreover, I've just realized that there is inconsistency among
LED drivers, related to the brightness_set op implementation.
Some drivers use work queue in brightness_set op it and some of them
don't, despite they do locking either directly or indirectly.
Those drivers are incompatible with timer trigger.

>> I think that using work queues would compromise the whole idea of
>> introducing intervals less than 1ms. After the task is delegated to
>> work queue we are losing the control over the moment when it will get
>> executed.
> No one is going to allow sub-ms intervals when work-queue
> is used. The proper solution would be to use work-queue for
> drivers that can sleep, and allow sub-ms resolution for others.
 >
> Fortunately the drivers seem to already have that information
> internally, and use work-queue on their own when needed.
> leds-aat1290 may be an exception from that.

leds-aat1290 also uses work queue in its brightness_set op
which is called from led_timer_function when timer trigger
is on.

>> I am becoming reluctant towards the whole idea, as we will be
>> unable to guarantee the stability of a delay interval.
> So why are you against the idea of improving the precision,
> rather than against the code that prevents us from doing so?
> The per-driver work queue use can be moved to led-core,
> and the precise intervals can be guaranteed for the drivers
> that do not need work queue.

Removing work queues from drivers and using brightness_set_work
instead would be nice. We had also discussion about similar
solution during LED Flash class implementation, however it
wasn't as clear as right now that the solution had been almost ready.

> Now your leds-aat1290 already asks for such a change,
> because it can sleep but does not use a work-queue the
> way other drivers do.

It doesn't need this change - it defines two ops: brightness_set
(the async one) and brightness_set_sync (the sync one). The
former is called from led_set_brightness_async and the latter
form led_set_brightness_sync.
led_set_brightness_async is called from led_set_brightness
for drivers that define SET_BRIGHTNESS_ASYNC flag and
led_set_brightness_sync for the drivers that define
SET_BRIGHTNESS_SYNC flags.

led_timer_function calls always led_set_brightness_async.

> So what should we do?
> I can try the aforementioned massive clean-up with removing
> the work-queue from every driver and using the one in
> led-core, but my attempts have few chances to succeed
> because of no test-cases. Or can you do this instead, so
> that leds-aat1290 driver is in line with the others? Or any
> other options I can try?
>

It would have to be done for the LED core and all drivers
in one patch set. We would have to get acks from all LED drivers'
authors (or at least from majority of them).

Once this is done we could think about adding optional hr timers
based triggers and invite people for testing.

-- 
Best Regards,
Jacek Anaszewski

Re: [PATCH v2 0/3] leds: blink resolution improvements

[Stas Sergeev]

04.05.2015 18:22, Jacek Anaszewski пишет:
> On 05/04/2015 02:12 PM, Stas Sergeev wrote:
>> Only under that condition:
>> ---
>> if (led_cdev->blink_delay_on || led_cdev->blink_delay_off) {
>>          led_cdev->delayed_set_value = brightness;
>>          schedule_work(&led_cdev->set_brightness_work);
>> ---
>>
>> But the main condition is:
>> ---
>> if (led_cdev->flags & SET_BRIGHTNESS_ASYNC) {
>>          led_set_brightness_async(led_cdev, brightness);
>> ---
>>
>> So I think it is actually unused.
>> I don't see why schedule_work() above can't be just replaced
>> with led_set_brightness_async(). Is there the reason not to do so?
> set_brightness_work not only sets the brightness but also
> stops software blinking, which was the primary reason
> for adding this work queue I think. Here is the commit message:
But led_trigger_set() does led_stop_software_blink(), which
IMHO means led_set_brightness() will in most cases be called
when sw blocking is already stopped. There seem to be just a
few cases where this is not true: oneshot_trig_deactivate() and
timer_trig_deactivate(), and I think I'll just change these two to
led_stop_software_blink(). I am pretty sure the work-queue is
not needed, but I'll have to test that with the patch it seems.

> ------------------------
>
>     leds: delay led_set_brightness if stopping soft-blink
>
>     Delay execution of led_set_brightness() if need to stop soft-blink
>     timer.
>
>     This allows led_set_brightness to be called in hard-irq context 
> even if 
> soft-blink was activated on that LED.
Instead of disabling the soft-blink beforehand, which is what 
led_trigger_set()
already does? I am probably missing something.

> > Now your leds-aat1290 already asks for such a change,
>> because it can sleep but does not use a work-queue the
>> way other drivers do.
> It doesn't need this change - it defines two ops: brightness_set
> (the async one) and brightness_set_sync (the sync one). The
> former is called from led_set_brightness_async and the latter
> form led_set_brightness_sync.
> led_set_brightness_async is called from led_set_brightness
> for drivers that define SET_BRIGHTNESS_ASYNC flag and
> led_set_brightness_sync for the drivers that define
> SET_BRIGHTNESS_SYNC flags.
>
> led_timer_function calls always led_set_brightness_async.
OK, I googled the patch:
https://lkml.org/lkml/2015/3/4/960
So the async one uses the work-queue, and the sync one
does not. Since led_timer_function calls always led_set_brightness_async,
it should always be using a work-queue.
But then I fail to explain your diagnostic that with my patch and
your driver, the hrtimer gives warning about a high interrupt
latency. I thought this is because your driver does sleeps and
does not use a work queue. Its not the case. Could you please
clarify, what then caused the high interrupt latency warning in
your testing?

>> So what should we do?
>> I can try the aforementioned massive clean-up with removing
>> the work-queue from every driver and using the one in
>> led-core, but my attempts have few chances to succeed
>> because of no test-cases. Or can you do this instead, so
>> that leds-aat1290 driver is in line with the others? Or any
>> other options I can try?
>>
> It would have to be done for the LED core and all drivers
> in one patch set. We would have to get acks from all LED drivers'
> authors (or at least from majority of them).
>
> Once this is done we could think about adding optional hr timers
> based triggers and invite people for testing.
As long as all drivers use the work-queue when needed and
there is no warning about high interrupt latency, I wonder if
there are some short-cuts to that route. :)
But I first need to understand where this latency came from.

Re: [PATCH v2 0/3] leds: blink resolution improvements

[Jacek Anaszewski]

On 05/04/2015 07:20 PM, Stas Sergeev wrote:
> 04.05.2015 18:22, Jacek Anaszewski пишет:
>> On 05/04/2015 02:12 PM, Stas Sergeev wrote:
>>> Only under that condition:
>>> ---
>>> if (led_cdev->blink_delay_on || led_cdev->blink_delay_off) {
>>>          led_cdev->delayed_set_value = brightness;
>>>          schedule_work(&led_cdev->set_brightness_work);
>>> ---
>>>
>>> But the main condition is:
>>> ---
>>> if (led_cdev->flags & SET_BRIGHTNESS_ASYNC) {
>>>          led_set_brightness_async(led_cdev, brightness);
>>> ---
>>>
>>> So I think it is actually unused.
>>> I don't see why schedule_work() above can't be just replaced
>>> with led_set_brightness_async(). Is there the reason not to do so?
>> set_brightness_work not only sets the brightness but also
>> stops software blinking, which was the primary reason
>> for adding this work queue I think. Here is the commit message:
> But led_trigger_set() does led_stop_software_blink(), which
> IMHO means led_set_brightness() will in most cases be called
> when sw blocking is already stopped. There seem to be just a
> few cases where this is not true: oneshot_trig_deactivate() and
> timer_trig_deactivate(), and I think I'll just change these two to
> led_stop_software_blink(). I am pretty sure the work-queue is
> not needed, but I'll have to test that with the patch it seems.

It is used e.g. in the following case:

#echo "timer" > trigger
#echo 1 > brightness

>> ------------------------
>>
>>     leds: delay led_set_brightness if stopping soft-blink
>>
>>     Delay execution of led_set_brightness() if need to stop soft-blink
>>     timer.
>>
>>     This allows led_set_brightness to be called in hard-irq context
>> even if soft-blink was activated on that LED.
> Instead of disabling the soft-blink beforehand, which is what
> led_trigger_set()
> already does? I am probably missing something.
>
>> > Now your leds-aat1290 already asks for such a change,
>>> because it can sleep but does not use a work-queue the
>>> way other drivers do.
>> It doesn't need this change - it defines two ops: brightness_set
>> (the async one) and brightness_set_sync (the sync one). The
>> former is called from led_set_brightness_async and the latter
>> form led_set_brightness_sync.
>> led_set_brightness_async is called from led_set_brightness
>> for drivers that define SET_BRIGHTNESS_ASYNC flag and
>> led_set_brightness_sync for the drivers that define
>> SET_BRIGHTNESS_SYNC flags.
>>
>> led_timer_function calls always led_set_brightness_async.
> OK, I googled the patch:
> https://lkml.org/lkml/2015/3/4/960
> So the async one uses the work-queue, and the sync one
> does not. Since led_timer_function calls always led_set_brightness_async,
> it should always be using a work-queue.
> But then I fail to explain your diagnostic that with my patch and
> your driver, the hrtimer gives warning about a high interrupt
> latency. I thought this is because your driver does sleeps and
> does not use a work queue. Its not the case. Could you please
> clarify, what then caused the high interrupt latency warning in
> your testing?

An accurate explanation would require thorough investigation.
It can be related to the fact that the driver uses delays.

>>> So what should we do?
>>> I can try the aforementioned massive clean-up with removing
>>> the work-queue from every driver and using the one in
>>> led-core, but my attempts have few chances to succeed
>>> because of no test-cases. Or can you do this instead, so
>>> that leds-aat1290 driver is in line with the others? Or any
>>> other options I can try?
>>>
>> It would have to be done for the LED core and all drivers
>> in one patch set. We would have to get acks from all LED drivers'
>> authors (or at least from majority of them).
>>
>> Once this is done we could think about adding optional hr timers
>> based triggers and invite people for testing.
> As long as all drivers use the work-queue when needed and
> there is no warning about high interrupt latency, I wonder if
> there are some short-cuts to that route. :)
> But I first need to understand where this latency came from.
>

It was probably caused by interfering delays in leds-aat1290.

In the first place we have to take into account that Linux is not
a real time operating system. The feature you're trying to implement
is realized by hardware with use of pwm. There might be narrow group
of drivers that could benefit from it in specific circumstances
(the system couldn't be too busy at the time when timer trigger is
running), but this is too weak argument in favour of supporting small
delay intervals.

-- 
Best Regards,
Jacek Anaszewski

Re: [PATCH v2 0/3] leds: blink resolution improvements

[Stas Sergeev]

05.05.2015 11:22, Jacek Anaszewski пишет:
> On 05/04/2015 07:20 PM, Stas Sergeev wrote:
>> 04.05.2015 18:22, Jacek Anaszewski пишет:
>>> On 05/04/2015 02:12 PM, Stas Sergeev wrote:
>>>> Only under that condition:
>>>> ---
>>>> if (led_cdev->blink_delay_on || led_cdev->blink_delay_off) {
>>>>          led_cdev->delayed_set_value = brightness;
>>>> schedule_work(&led_cdev->set_brightness_work);
>>>> ---
>>>>
>>>> But the main condition is:
>>>> ---
>>>> if (led_cdev->flags & SET_BRIGHTNESS_ASYNC) {
>>>>          led_set_brightness_async(led_cdev, brightness);
>>>> ---
>>>>
>>>> So I think it is actually unused.
>>>> I don't see why schedule_work() above can't be just replaced
>>>> with led_set_brightness_async(). Is there the reason not to do so?
>>> set_brightness_work not only sets the brightness but also
>>> stops software blinking, which was the primary reason
>>> for adding this work queue I think. Here is the commit message:
>> But led_trigger_set() does led_stop_software_blink(), which
>> IMHO means led_set_brightness() will in most cases be called
>> when sw blocking is already stopped. There seem to be just a
>> few cases where this is not true: oneshot_trig_deactivate() and
>> timer_trig_deactivate(), and I think I'll just change these two to
>> led_stop_software_blink(). I am pretty sure the work-queue is
>> not needed, but I'll have to test that with the patch it seems.
> It is used e.g. in the following case:
>
> #echo "timer" > trigger
> #echo 1 > brightness
Indeed, thanks.
I'll study that case next week when my board is back to me.
Looking at sources, it seems in that case it would disable the
software blinking (del_timer_sync()) without changing the
trigger back to "none", which does not make sense to me.

>
>>
>>> > Now your leds-aat1290 already asks for such a change,
>>>> because it can sleep but does not use a work-queue the
>>>> way other drivers do.
>>> It doesn't need this change - it defines two ops: brightness_set
>>> (the async one) and brightness_set_sync (the sync one). The
>>> former is called from led_set_brightness_async and the latter
>>> form led_set_brightness_sync.
>>> led_set_brightness_async is called from led_set_brightness
>>> for drivers that define SET_BRIGHTNESS_ASYNC flag and
>>> led_set_brightness_sync for the drivers that define
>>> SET_BRIGHTNESS_SYNC flags.
>>>
>>> led_timer_function calls always led_set_brightness_async.
>> OK, I googled the patch:
>> https://lkml.org/lkml/2015/3/4/960
>> So the async one uses the work-queue, and the sync one
>> does not. Since led_timer_function calls always 
>> led_set_brightness_async,
>> it should always be using a work-queue.
>> But then I fail to explain your diagnostic that with my patch and
>> your driver, the hrtimer gives warning about a high interrupt
>> latency. I thought this is because your driver does sleeps and
>> does not use a work queue. Its not the case. Could you please
>> clarify, what then caused the high interrupt latency warning in
>> your testing?
> An accurate explanation would require thorough investigation.
> It can be related to the fact that the driver uses delays.
Even if your driver just does schedule_work() and nothing
more in an async method? Strange.

> In the first place we have to take into account that Linux is not
> a real time operating system. The feature you're trying to implement
> is realized by hardware with use of pwm. There might be narrow group
> of drivers that could benefit from it in specific circumstances
> (the system couldn't be too busy at the time when timer trigger is
> running), but this is too weak argument in favour of supporting small
> delay intervals.
If you mean the drivers that don't have any sleeps, then the
system load is irrelevant because the hrtimer callback is AFAIK
running in an irq context. So for them it would be a clear win,
not just in a specific circumstances. Of course I wonder if it is
only leds-gpio, or anything else too. :) Though I could suspect
that leds-gpio have a very wide usage, and it may worth the
troubles even to improve just leds-gpio alone.

Re: [PATCH v2 0/3] leds: blink resolution improvements

[Jacek Anaszewski]

On 05/05/2015 03:02 PM, Stas Sergeev wrote:
> 05.05.2015 11:22, Jacek Anaszewski пишет:
>> On 05/04/2015 07:20 PM, Stas Sergeev wrote:
>>> 04.05.2015 18:22, Jacek Anaszewski пишет:
>>>> On 05/04/2015 02:12 PM, Stas Sergeev wrote:
>>>>> Only under that condition:
>>>>> ---
>>>>> if (led_cdev->blink_delay_on || led_cdev->blink_delay_off) {
>>>>>          led_cdev->delayed_set_value = brightness;
>>>>> schedule_work(&led_cdev->set_brightness_work);
>>>>> ---
>>>>>
>>>>> But the main condition is:
>>>>> ---
>>>>> if (led_cdev->flags & SET_BRIGHTNESS_ASYNC) {
>>>>>          led_set_brightness_async(led_cdev, brightness);
>>>>> ---
>>>>>
>>>>> So I think it is actually unused.
>>>>> I don't see why schedule_work() above can't be just replaced
>>>>> with led_set_brightness_async(). Is there the reason not to do so?
>>>> set_brightness_work not only sets the brightness but also
>>>> stops software blinking, which was the primary reason
>>>> for adding this work queue I think. Here is the commit message:
>>> But led_trigger_set() does led_stop_software_blink(), which
>>> IMHO means led_set_brightness() will in most cases be called
>>> when sw blocking is already stopped. There seem to be just a
>>> few cases where this is not true: oneshot_trig_deactivate() and
>>> timer_trig_deactivate(), and I think I'll just change these two to
>>> led_stop_software_blink(). I am pretty sure the work-queue is
>>> not needed, but I'll have to test that with the patch it seems.
>> It is used e.g. in the following case:
>>
>> #echo "timer" > trigger
>> #echo 1 > brightness
> Indeed, thanks.
> I'll study that case next week when my board is back to me.
> Looking at sources, it seems in that case it would disable the
> software blinking (del_timer_sync()) without changing the
> trigger back to "none", which does not make sense to me.

Yes, this needs to be fixed.

>>
>>>
>>>> > Now your leds-aat1290 already asks for such a change,
>>>>> because it can sleep but does not use a work-queue the
>>>>> way other drivers do.
>>>> It doesn't need this change - it defines two ops: brightness_set
>>>> (the async one) and brightness_set_sync (the sync one). The
>>>> former is called from led_set_brightness_async and the latter
>>>> form led_set_brightness_sync.
>>>> led_set_brightness_async is called from led_set_brightness
>>>> for drivers that define SET_BRIGHTNESS_ASYNC flag and
>>>> led_set_brightness_sync for the drivers that define
>>>> SET_BRIGHTNESS_SYNC flags.
>>>>
>>>> led_timer_function calls always led_set_brightness_async.
>>> OK, I googled the patch:
>>> https://lkml.org/lkml/2015/3/4/960
>>> So the async one uses the work-queue, and the sync one
>>> does not. Since led_timer_function calls always
>>> led_set_brightness_async,
>>> it should always be using a work-queue.
>>> But then I fail to explain your diagnostic that with my patch and
>>> your driver, the hrtimer gives warning about a high interrupt
>>> latency. I thought this is because your driver does sleeps and
>>> does not use a work queue. Its not the case. Could you please
>>> clarify, what then caused the high interrupt latency warning in
>>> your testing?
>> An accurate explanation would require thorough investigation.
>> It can be related to the fact that the driver uses delays.
> Even if your driver just does schedule_work() and nothing
> more in an async method? Strange.

There can be indirect correlation.

>> In the first place we have to take into account that Linux is not
>> a real time operating system. The feature you're trying to implement
>> is realized by hardware with use of pwm. There might be narrow group
>> of drivers that could benefit from it in specific circumstances
>> (the system couldn't be too busy at the time when timer trigger is
>> running), but this is too weak argument in favour of supporting small
>> delay intervals.
> If you mean the drivers that don't have any sleeps, then the
> system load is irrelevant because the hrtimer callback is AFAIK
> running in an irq context. So for them it would be a clear win,
> not just in a specific circumstances. Of course I wonder if it is
> only leds-gpio, or anything else too. :) Though I could suspect
> that leds-gpio have a very wide usage, and it may worth the
> troubles even to improve just leds-gpio alone.

If you have a strong belief that it is possible to implement this
feature in a manner acceptable for everyone, feel free to experiment
with the implementation. If people will find it useful and reliable
then we will merge it.

-- 
Best Regards,
Jacek Anaszewski