[PATCH 0/5] LED core improvements

[PATCH 0/5] LED core improvements

[Jacek Anaszewski]

This patch set prepares the ground for removing work queues
from LED class drivers, and is a follow up of the patch set [1].
LED core modifications have been reorganized to make them
more clear and easier to review. The patch set is reduced
in comparison to it its predecessor, to expose the modifications
indispensable for the LED core to gain the capability of handling
brightness_set_blocking ops, that is without work queues.

Thanks,
Jacek Anaszewski

[1] https://lkml.org/lkml/2015/8/20/426

Jacek Anaszewski (5):
  leds: core: Move LED core callbacks out of led-class.c
  leds: core: Add LED_BLINK_CHANGE and LED_BLINK_DISABLE flags
  leds: Rename brightness_set_sync op to brightness_set_blocking
  leds: core: Add an internal led_set_brightness_nosleep function
  leds: core: Use set_brightness_work for the blocking op

 drivers/leds/led-class-flash.c            |    2 +-
 drivers/leds/led-class.c                  |   69 +-------------
 drivers/leds/led-core.c                   |  140 +++++++++++++++++++++++++++--
 drivers/leds/leds-aat1290.c               |    2 +-
 drivers/leds/leds-ktd2692.c               |    2 +-
 drivers/leds/leds-max77693.c              |    2 +-
 drivers/leds/leds.h                       |   15 +---
 drivers/leds/trigger/ledtrig-backlight.c  |    8 +-
 drivers/leds/trigger/ledtrig-default-on.c |    2 +-
 drivers/leds/trigger/ledtrig-gpio.c       |    6 +-
 drivers/leds/trigger/ledtrig-heartbeat.c  |    4 +-
 drivers/leds/trigger/ledtrig-oneshot.c    |    4 +-
 drivers/leds/trigger/ledtrig-transient.c  |    8 +-
 include/linux/leds.h                      |   21 +++--
 14 files changed, 169 insertions(+), 116 deletions(-)

-- 
1.7.9.5

[PATCH 1/5] leds: core: Move LED core callbacks out of led-class.c

[Jacek Anaszewski]

Since the API for controlling LED brightness and blinking is defined in
the LED core, move the related timer and work callbacks to the led-core.c,
and initialize them through a new led_core_init API.

Signed-off-by: Jacek Anaszewski <j.anaszewski@samsung.com>
---
 drivers/leds/led-class.c |   69 +------------------------------------------
 drivers/leds/led-core.c  |   73 ++++++++++++++++++++++++++++++++++++++++++++++
 drivers/leds/leds.h      |    1 +
 3 files changed, 75 insertions(+), 68 deletions(-)

diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
index ca51d58..7385f98 100644
--- a/drivers/leds/led-class.c
+++ b/drivers/leds/led-class.c
@@ -102,70 +102,6 @@ static const struct attribute_group *led_groups[] = {
 	NULL,
 };
 
-static void led_timer_function(unsigned long data)
-{
-	struct led_classdev *led_cdev = (void *)data;
-	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;
-	}
-
-	if (led_cdev->flags & LED_BLINK_ONESHOT_STOP) {
-		led_cdev->flags &= ~LED_BLINK_ONESHOT_STOP;
-		return;
-	}
-
-	brightness = led_get_brightness(led_cdev);
-	if (!brightness) {
-		/* Time to switch the LED on. */
-		if (led_cdev->delayed_set_value) {
-			led_cdev->blink_brightness =
-					led_cdev->delayed_set_value;
-			led_cdev->delayed_set_value = 0;
-		}
-		brightness = led_cdev->blink_brightness;
-		delay = led_cdev->blink_delay_on;
-	} else {
-		/* Store the current brightness value to be able
-		 * to restore it when the delay_off period is over.
-		 */
-		led_cdev->blink_brightness = brightness;
-		brightness = LED_OFF;
-		delay = led_cdev->blink_delay_off;
-	}
-
-	led_set_brightness_async(led_cdev, brightness);
-
-	/* Return in next iteration if led is in one-shot mode and we are in
-	 * the final blink state so that the led is toggled each delay_on +
-	 * delay_off milliseconds in worst case.
-	 */
-	if (led_cdev->flags & LED_BLINK_ONESHOT) {
-		if (led_cdev->flags & LED_BLINK_INVERT) {
-			if (brightness)
-				led_cdev->flags |= LED_BLINK_ONESHOT_STOP;
-		} else {
-			if (!brightness)
-				led_cdev->flags |= LED_BLINK_ONESHOT_STOP;
-		}
-	}
-
-	mod_timer(&led_cdev->blink_timer, jiffies + msecs_to_jiffies(delay));
-}
-
-static void set_brightness_delayed(struct work_struct *ws)
-{
-	struct led_classdev *led_cdev =
-		container_of(ws, struct led_classdev, set_brightness_work);
-
-	led_stop_software_blink(led_cdev);
-
-	led_set_brightness_async(led_cdev, led_cdev->delayed_set_value);
-}
-
 /**
  * led_classdev_suspend - suspend an led_classdev.
  * @led_cdev: the led_classdev to suspend.
@@ -283,10 +219,7 @@ int led_classdev_register(struct device *parent, struct led_classdev *led_cdev)
 
 	led_update_brightness(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);
+	led_init_core(led_cdev);
 
 #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 549de7e..2cb4e37 100644
--- a/drivers/leds/led-core.c
+++ b/drivers/leds/led-core.c
@@ -25,6 +25,70 @@ EXPORT_SYMBOL_GPL(leds_list_lock);
 LIST_HEAD(leds_list);
 EXPORT_SYMBOL_GPL(leds_list);
 
+static void led_timer_function(unsigned long data)
+{
+	struct led_classdev *led_cdev = (void *)data;
+	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;
+	}
+
+	if (led_cdev->flags & LED_BLINK_ONESHOT_STOP) {
+		led_cdev->flags &= ~LED_BLINK_ONESHOT_STOP;
+		return;
+	}
+
+	brightness = led_get_brightness(led_cdev);
+	if (!brightness) {
+		/* Time to switch the LED on. */
+		if (led_cdev->delayed_set_value) {
+			led_cdev->blink_brightness =
+					led_cdev->delayed_set_value;
+			led_cdev->delayed_set_value = 0;
+		}
+		brightness = led_cdev->blink_brightness;
+		delay = led_cdev->blink_delay_on;
+	} else {
+		/* Store the current brightness value to be able
+		 * to restore it when the delay_off period is over.
+		 */
+		led_cdev->blink_brightness = brightness;
+		brightness = LED_OFF;
+		delay = led_cdev->blink_delay_off;
+	}
+
+	led_set_brightness_async(led_cdev, brightness);
+
+	/* Return in next iteration if led is in one-shot mode and we are in
+	 * the final blink state so that the led is toggled each delay_on +
+	 * delay_off milliseconds in worst case.
+	 */
+	if (led_cdev->flags & LED_BLINK_ONESHOT) {
+		if (led_cdev->flags & LED_BLINK_INVERT) {
+			if (brightness)
+				led_cdev->flags |= LED_BLINK_ONESHOT_STOP;
+		} else {
+			if (!brightness)
+				led_cdev->flags |= LED_BLINK_ONESHOT_STOP;
+		}
+	}
+
+	mod_timer(&led_cdev->blink_timer, jiffies + msecs_to_jiffies(delay));
+}
+
+static void set_brightness_delayed(struct work_struct *ws)
+{
+	struct led_classdev *led_cdev =
+		container_of(ws, struct led_classdev, set_brightness_work);
+
+	led_stop_software_blink(led_cdev);
+
+	led_set_brightness_async(led_cdev, led_cdev->delayed_set_value);
+}
+
 static void led_set_software_blink(struct led_classdev *led_cdev,
 				   unsigned long delay_on,
 				   unsigned long delay_off)
@@ -72,6 +136,15 @@ static void led_blink_setup(struct led_classdev *led_cdev,
 	led_set_software_blink(led_cdev, *delay_on, *delay_off);
 }
 
+void led_init_core(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);
+}
+EXPORT_SYMBOL(led_init_core);
+
 void led_blink_set(struct led_classdev *led_cdev,
 		   unsigned long *delay_on,
 		   unsigned long *delay_off)
diff --git a/drivers/leds/leds.h b/drivers/leds/leds.h
index bc89d7a..4238fbc 100644
--- a/drivers/leds/leds.h
+++ b/drivers/leds/leds.h
@@ -44,6 +44,7 @@ static inline int led_get_brightness(struct led_classdev *led_cdev)
 	return led_cdev->brightness;
 }
 
+void led_init_core(struct led_classdev *led_cdev);
 void led_stop_software_blink(struct led_classdev *led_cdev);
 
 extern struct rw_semaphore leds_list_lock;
-- 
1.7.9.5

[PATCH 2/5] leds: core: Add LED_BLINK_CHANGE and LED_BLINK_DISABLE flags

[Jacek Anaszewski]

This patch adds LED_BLINK_CHANGE flag to indicate that blink brightness
has changed, and LED_BLINK_DISABLE flag to indicate that blinking
deactivation has been requested. In order to use the flags
led_timer_function and set_brightness_delayed callbacks as well as
led_set_brightness function are being modified. The main goal of these
modifications is to prepare set_brightness_work for extension of the
scope of its responsibilities.

Signed-off-by: Jacek Anaszewski <j.anaszewski@samsung.com>
---
 drivers/leds/led-core.c |   36 ++++++++++++++++++++++++++----------
 include/linux/leds.h    |   10 ++++++----
 2 files changed, 32 insertions(+), 14 deletions(-)

diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
index 2cb4e37..1968e24 100644
--- a/drivers/leds/led-core.c
+++ b/drivers/leds/led-core.c
@@ -44,18 +44,18 @@ static void led_timer_function(unsigned long data)
 	brightness = led_get_brightness(led_cdev);
 	if (!brightness) {
 		/* Time to switch the LED on. */
-		if (led_cdev->delayed_set_value) {
-			led_cdev->blink_brightness =
-					led_cdev->delayed_set_value;
-			led_cdev->delayed_set_value = 0;
-		}
 		brightness = led_cdev->blink_brightness;
 		delay = led_cdev->blink_delay_on;
 	} else {
 		/* Store the current brightness value to be able
 		 * to restore it when the delay_off period is over.
+		 * Do it only if there is no pending blink brightness
+		 * change, to avoid overwriting the new value.
 		 */
-		led_cdev->blink_brightness = brightness;
+		if (!(led_cdev->flags & LED_BLINK_CHANGE))
+			led_cdev->blink_brightness = brightness;
+		else
+			led_cdev->flags &= ~LED_BLINK_CHANGE;
 		brightness = LED_OFF;
 		delay = led_cdev->blink_delay_off;
 	}
@@ -84,7 +84,11 @@ static void set_brightness_delayed(struct work_struct *ws)
 	struct led_classdev *led_cdev =
 		container_of(ws, struct led_classdev, set_brightness_work);
 
-	led_stop_software_blink(led_cdev);
+	if (led_cdev->flags & LED_BLINK_DISABLE) {
+		led_cdev->delayed_set_value = LED_OFF;
+		led_stop_software_blink(led_cdev);
+		led_cdev->flags &= ~LED_BLINK_DISABLE;
+	}
 
 	led_set_brightness_async(led_cdev, led_cdev->delayed_set_value);
 }
@@ -192,11 +196,23 @@ void led_set_brightness(struct led_classdev *led_cdev,
 {
 	int ret = 0;
 
-	/* delay brightness if soft-blink is active */
+	/*
+	 * In case blinking is on delay brightness setting
+	 * until the next timer tick.
+	 */
 	if (led_cdev->blink_delay_on || led_cdev->blink_delay_off) {
-		led_cdev->delayed_set_value = brightness;
-		if (brightness == LED_OFF)
+		/*
+		 * If need to disable soft blinking delegate this to the
+		 * work queue task to avoid problems in case we are
+		 * called from hard irq context.
+		 */
+		if (brightness == LED_OFF) {
+			led_cdev->flags |= LED_BLINK_DISABLE;
 			schedule_work(&led_cdev->set_brightness_work);
+		} else {
+			led_cdev->flags |= LED_BLINK_CHANGE;
+			led_cdev->blink_brightness = brightness;
+		}
 		return;
 	}
 
diff --git a/include/linux/leds.h b/include/linux/leds.h
index b122eea..188352c 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -44,10 +44,12 @@ struct led_classdev {
 #define LED_BLINK_ONESHOT	(1 << 17)
 #define LED_BLINK_ONESHOT_STOP	(1 << 18)
 #define LED_BLINK_INVERT	(1 << 19)
-#define LED_SYSFS_DISABLE	(1 << 20)
-#define SET_BRIGHTNESS_ASYNC	(1 << 21)
-#define SET_BRIGHTNESS_SYNC	(1 << 22)
-#define LED_DEV_CAP_FLASH	(1 << 23)
+#define LED_BLINK_CHANGE	(1 << 20)
+#define LED_BLINK_DISABLE	(1 << 21)
+#define LED_SYSFS_DISABLE	(1 << 22)
+#define SET_BRIGHTNESS_ASYNC	(1 << 23)
+#define SET_BRIGHTNESS_SYNC	(1 << 24)
+#define LED_DEV_CAP_FLASH	(1 << 25)
 
 	/* Set LED brightness level */
 	/* Must not sleep, use a workqueue if needed */
-- 
1.7.9.5

[PATCH 3/5] leds: Rename brightness_set_sync op to brightness_set_blocking

[Jacek Anaszewski]

The initial purpose of brightness_set_sync op, introduced along with
the LED flash class extension, was to add a means for setting torch LED
brightness as soon as possible, which couldn't have been guaranteed by
brightness_set op. This patch renames the op to brightness_set_blocking,
which describes its purpose in a more generic way, and is beneficial
in view of the prospective changes in the core related to using
LED core's set_brightness_work for setting brightness for LED class
drivers that can sleep or use delays while setting brightness.

Signed-off-by: Jacek Anaszewski <j.anaszewski@samsung.com>
---
 drivers/leds/led-class-flash.c |    2 +-
 drivers/leds/leds-aat1290.c    |    2 +-
 drivers/leds/leds-ktd2692.c    |    2 +-
 drivers/leds/leds-max77693.c   |    2 +-
 drivers/leds/leds.h            |    2 +-
 include/linux/leds.h           |    9 +++------
 6 files changed, 8 insertions(+), 11 deletions(-)

diff --git a/drivers/leds/led-class-flash.c b/drivers/leds/led-class-flash.c
index 3b25734..300a2c9 100644
--- a/drivers/leds/led-class-flash.c
+++ b/drivers/leds/led-class-flash.c
@@ -298,7 +298,7 @@ int led_classdev_flash_register(struct device *parent,
 	led_cdev = &fled_cdev->led_cdev;
 
 	if (led_cdev->flags & LED_DEV_CAP_FLASH) {
-		if (!led_cdev->brightness_set_sync)
+		if (!led_cdev->brightness_set_blocking)
 			return -EINVAL;
 
 		ops = fled_cdev->ops;
diff --git a/drivers/leds/leds-aat1290.c b/drivers/leds/leds-aat1290.c
index fd7c25f..4bff3b5 100644
--- a/drivers/leds/leds-aat1290.c
+++ b/drivers/leds/leds-aat1290.c
@@ -510,7 +510,7 @@ static int aat1290_led_probe(struct platform_device *pdev)
 
 	/* Initialize LED Flash class device */
 	led_cdev->brightness_set = aat1290_led_brightness_set;
-	led_cdev->brightness_set_sync = aat1290_led_brightness_set_sync;
+	led_cdev->brightness_set_blocking = aat1290_led_brightness_set_sync;
 	led_cdev->max_brightness = led_cfg.max_brightness;
 	led_cdev->flags |= LED_DEV_CAP_FLASH;
 	INIT_WORK(&led->work_brightness_set, aat1290_brightness_set_work);
diff --git a/drivers/leds/leds-ktd2692.c b/drivers/leds/leds-ktd2692.c
index 2ae8c4d..4c14bed 100644
--- a/drivers/leds/leds-ktd2692.c
+++ b/drivers/leds/leds-ktd2692.c
@@ -382,7 +382,7 @@ static int ktd2692_probe(struct platform_device *pdev)
 
 	led_cdev->max_brightness = led_cfg.max_brightness;
 	led_cdev->brightness_set = ktd2692_led_brightness_set;
-	led_cdev->brightness_set_sync = ktd2692_led_brightness_set_sync;
+	led_cdev->brightness_set_blocking = ktd2692_led_brightness_set_sync;
 	led_cdev->flags |= LED_CORE_SUSPENDRESUME | LED_DEV_CAP_FLASH;
 
 	mutex_init(&led->lock);
diff --git a/drivers/leds/leds-max77693.c b/drivers/leds/leds-max77693.c
index b8b0eec..f69bcb4 100644
--- a/drivers/leds/leds-max77693.c
+++ b/drivers/leds/leds-max77693.c
@@ -931,7 +931,7 @@ static void max77693_init_fled_cdev(struct max77693_sub_led *sub_led,
 	led_cdev->name = led_cfg->label[fled_id];
 
 	led_cdev->brightness_set = max77693_led_brightness_set;
-	led_cdev->brightness_set_sync = max77693_led_brightness_set_sync;
+	led_cdev->brightness_set_blocking = max77693_led_brightness_set_sync;
 	led_cdev->max_brightness = (led->iout_joint ?
 					led_cfg->iout_torch_max[FLED1] +
 					led_cfg->iout_torch_max[FLED2] :
diff --git a/drivers/leds/leds.h b/drivers/leds/leds.h
index 4238fbc..cf6d448 100644
--- a/drivers/leds/leds.h
+++ b/drivers/leds/leds.h
@@ -34,7 +34,7 @@ static inline int led_set_brightness_sync(struct led_classdev *led_cdev,
 	led_cdev->brightness = min(value, led_cdev->max_brightness);
 
 	if (!(led_cdev->flags & LED_SUSPENDED))
-		ret = led_cdev->brightness_set_sync(led_cdev,
+		ret = led_cdev->brightness_set_blocking(led_cdev,
 						led_cdev->brightness);
 	return ret;
 }
diff --git a/include/linux/leds.h b/include/linux/leds.h
index 188352c..9f850e6 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -55,12 +55,9 @@ struct led_classdev {
 	/* Must not sleep, use a workqueue if needed */
 	void		(*brightness_set)(struct led_classdev *led_cdev,
 					  enum led_brightness brightness);
-	/*
-	 * Set LED brightness level immediately - it can block the caller for
-	 * the time required for accessing a LED device register.
-	 */
-	int		(*brightness_set_sync)(struct led_classdev *led_cdev,
-					enum led_brightness brightness);
+	/* Can sleep or use delays */
+	int (*brightness_set_blocking)(struct led_classdev *led_cdev,
+				       enum led_brightness brightness);
 	/* Get LED brightness level */
 	enum led_brightness (*brightness_get)(struct led_classdev *led_cdev);
 
-- 
1.7.9.5

[PATCH 4/5] leds: core: Add an internal led_set_brightness_nosleep function

[Jacek Anaszewski]

This patch adds led_set_brightness_nosleep function, which guarantees
setting LED brightness in a non-blocking way. It also replaces
led_set_brightness_async with led_set_brightness_nosleep in all places
where the most vital was setting brightness in a non sleeping way but
not necessarily asynchronously, which is not needed for non-blocking
drivers.

Signed-off-by: Jacek Anaszewski <j.anaszewski@samsung.com>
---
 drivers/leds/led-core.c                   |   29 ++++++++++++++++++++++++-----
 drivers/leds/leds.h                       |    2 ++
 drivers/leds/trigger/ledtrig-backlight.c  |    8 ++++----
 drivers/leds/trigger/ledtrig-default-on.c |    2 +-
 drivers/leds/trigger/ledtrig-gpio.c       |    6 +++---
 drivers/leds/trigger/ledtrig-heartbeat.c  |    4 ++--
 drivers/leds/trigger/ledtrig-oneshot.c    |    4 ++--
 drivers/leds/trigger/ledtrig-transient.c  |    8 ++++----
 8 files changed, 42 insertions(+), 21 deletions(-)

diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
index 1968e24..fe6c2df 100644
--- a/drivers/leds/led-core.c
+++ b/drivers/leds/led-core.c
@@ -32,7 +32,7 @@ static void led_timer_function(unsigned long data)
 	unsigned long delay;
 
 	if (!led_cdev->blink_delay_on || !led_cdev->blink_delay_off) {
-		led_set_brightness_async(led_cdev, LED_OFF);
+		led_set_brightness_nosleep(led_cdev, LED_OFF);
 		return;
 	}
 
@@ -60,7 +60,7 @@ static void led_timer_function(unsigned long data)
 		delay = led_cdev->blink_delay_off;
 	}
 
-	led_set_brightness_async(led_cdev, brightness);
+	led_set_brightness_nosleep(led_cdev, brightness);
 
 	/* Return in next iteration if led is in one-shot mode and we are in
 	 * the final blink state so that the led is toggled each delay_on +
@@ -110,13 +110,14 @@ static void led_set_software_blink(struct led_classdev *led_cdev,
 
 	/* never on - just set to off */
 	if (!delay_on) {
-		led_set_brightness_async(led_cdev, LED_OFF);
+		led_set_brightness_nosleep(led_cdev, LED_OFF);
 		return;
 	}
 
 	/* never off - just set to brightness */
 	if (!delay_off) {
-		led_set_brightness_async(led_cdev, led_cdev->blink_brightness);
+		led_set_brightness_nosleep(led_cdev,
+					   led_cdev->blink_brightness);
 		return;
 	}
 
@@ -217,7 +218,7 @@ void led_set_brightness(struct led_classdev *led_cdev,
 	}
 
 	if (led_cdev->flags & SET_BRIGHTNESS_ASYNC) {
-		led_set_brightness_async(led_cdev, brightness);
+		led_set_brightness_nosleep(led_cdev, brightness);
 		return;
 	} else if (led_cdev->flags & SET_BRIGHTNESS_SYNC)
 		ret = led_set_brightness_sync(led_cdev, brightness);
@@ -230,6 +231,24 @@ void led_set_brightness(struct led_classdev *led_cdev,
 }
 EXPORT_SYMBOL(led_set_brightness);
 
+void led_set_brightness_nosleep(struct led_classdev *led_cdev,
+					enum led_brightness value)
+{
+	led_cdev->brightness = min(value, led_cdev->max_brightness);
+
+	if (led_cdev->flags & LED_SUSPENDED)
+		return;
+
+	/* Use brightness_set op if available, it is guaranteed not to sleep */
+	if (led_cdev->brightness_set)
+		led_cdev->brightness_set(led_cdev, led_cdev->brightness);
+
+	/* If brightness setting can sleep, delegate it to a work queue task */
+	led_cdev->delayed_set_value = led_cdev->brightness;
+	schedule_work(&led_cdev->set_brightness_work);
+}
+EXPORT_SYMBOL(led_set_brightness_nosleep);
+
 int led_update_brightness(struct led_classdev *led_cdev)
 {
 	int ret = 0;
diff --git a/drivers/leds/leds.h b/drivers/leds/leds.h
index cf6d448..11520eb 100644
--- a/drivers/leds/leds.h
+++ b/drivers/leds/leds.h
@@ -46,6 +46,8 @@ static inline int led_get_brightness(struct led_classdev *led_cdev)
 
 void led_init_core(struct led_classdev *led_cdev);
 void led_stop_software_blink(struct led_classdev *led_cdev);
+void led_set_brightness_nosleep(struct led_classdev *led_cdev,
+				enum led_brightness value);
 
 extern struct rw_semaphore leds_list_lock;
 extern struct list_head leds_list;
diff --git a/drivers/leds/trigger/ledtrig-backlight.c b/drivers/leds/trigger/ledtrig-backlight.c
index 59eca17..1ca1f16 100644
--- a/drivers/leds/trigger/ledtrig-backlight.c
+++ b/drivers/leds/trigger/ledtrig-backlight.c
@@ -51,9 +51,9 @@ static int fb_notifier_callback(struct notifier_block *p,
 
 	if ((n->old_status == UNBLANK) ^ n->invert) {
 		n->brightness = led->brightness;
-		led_set_brightness_async(led, LED_OFF);
+		led_set_brightness_nosleep(led, LED_OFF);
 	} else {
-		led_set_brightness_async(led, n->brightness);
+		led_set_brightness_nosleep(led, n->brightness);
 	}
 
 	n->old_status = new_status;
@@ -89,9 +89,9 @@ static ssize_t bl_trig_invert_store(struct device *dev,
 
 	/* After inverting, we need to update the LED. */
 	if ((n->old_status == BLANK) ^ n->invert)
-		led_set_brightness_async(led, LED_OFF);
+		led_set_brightness_nosleep(led, LED_OFF);
 	else
-		led_set_brightness_async(led, n->brightness);
+		led_set_brightness_nosleep(led, n->brightness);
 
 	return num;
 }
diff --git a/drivers/leds/trigger/ledtrig-default-on.c b/drivers/leds/trigger/ledtrig-default-on.c
index 6f38f88..ff455cb 100644
--- a/drivers/leds/trigger/ledtrig-default-on.c
+++ b/drivers/leds/trigger/ledtrig-default-on.c
@@ -19,7 +19,7 @@
 
 static void defon_trig_activate(struct led_classdev *led_cdev)
 {
-	led_set_brightness_async(led_cdev, led_cdev->max_brightness);
+	led_set_brightness_nosleep(led_cdev, led_cdev->max_brightness);
 }
 
 static struct led_trigger defon_led_trigger = {
diff --git a/drivers/leds/trigger/ledtrig-gpio.c b/drivers/leds/trigger/ledtrig-gpio.c
index 4cc7040..51288a4 100644
--- a/drivers/leds/trigger/ledtrig-gpio.c
+++ b/drivers/leds/trigger/ledtrig-gpio.c
@@ -54,12 +54,12 @@ static void gpio_trig_work(struct work_struct *work)
 
 	if (tmp) {
 		if (gpio_data->desired_brightness)
-			led_set_brightness_async(gpio_data->led,
+			led_set_brightness_nosleep(gpio_data->led,
 					   gpio_data->desired_brightness);
 		else
-			led_set_brightness_async(gpio_data->led, LED_FULL);
+			led_set_brightness_nosleep(gpio_data->led, LED_FULL);
 	} else {
-		led_set_brightness_async(gpio_data->led, LED_OFF);
+		led_set_brightness_nosleep(gpio_data->led, LED_OFF);
 	}
 }
 
diff --git a/drivers/leds/trigger/ledtrig-heartbeat.c b/drivers/leds/trigger/ledtrig-heartbeat.c
index fea6871..3dc6f0c 100644
--- a/drivers/leds/trigger/ledtrig-heartbeat.c
+++ b/drivers/leds/trigger/ledtrig-heartbeat.c
@@ -37,7 +37,7 @@ static void led_heartbeat_function(unsigned long data)
 	unsigned long delay = 0;
 
 	if (unlikely(panic_heartbeats)) {
-		led_set_brightness(led_cdev, LED_OFF);
+		led_set_brightness_nosleep(led_cdev, LED_OFF);
 		return;
 	}
 
@@ -74,7 +74,7 @@ static void led_heartbeat_function(unsigned long data)
 		break;
 	}
 
-	led_set_brightness_async(led_cdev, brightness);
+	led_set_brightness_nosleep(led_cdev, brightness);
 	mod_timer(&heartbeat_data->timer, jiffies + delay);
 }
 
diff --git a/drivers/leds/trigger/ledtrig-oneshot.c b/drivers/leds/trigger/ledtrig-oneshot.c
index fbd02cd..6729317 100644
--- a/drivers/leds/trigger/ledtrig-oneshot.c
+++ b/drivers/leds/trigger/ledtrig-oneshot.c
@@ -63,9 +63,9 @@ static ssize_t led_invert_store(struct device *dev,
 	oneshot_data->invert = !!state;
 
 	if (oneshot_data->invert)
-		led_set_brightness_async(led_cdev, LED_FULL);
+		led_set_brightness_nosleep(led_cdev, LED_FULL);
 	else
-		led_set_brightness_async(led_cdev, LED_OFF);
+		led_set_brightness_nosleep(led_cdev, LED_OFF);
 
 	return size;
 }
diff --git a/drivers/leds/trigger/ledtrig-transient.c b/drivers/leds/trigger/ledtrig-transient.c
index 3c34de4..1dddd8f 100644
--- a/drivers/leds/trigger/ledtrig-transient.c
+++ b/drivers/leds/trigger/ledtrig-transient.c
@@ -41,7 +41,7 @@ static void transient_timer_function(unsigned long data)
 	struct transient_trig_data *transient_data = led_cdev->trigger_data;
 
 	transient_data->activate = 0;
-	led_set_brightness_async(led_cdev, transient_data->restore_state);
+	led_set_brightness_nosleep(led_cdev, transient_data->restore_state);
 }
 
 static ssize_t transient_activate_show(struct device *dev,
@@ -72,7 +72,7 @@ static ssize_t transient_activate_store(struct device *dev,
 	if (state == 0 && transient_data->activate == 1) {
 		del_timer(&transient_data->timer);
 		transient_data->activate = state;
-		led_set_brightness_async(led_cdev,
+		led_set_brightness_nosleep(led_cdev,
 					transient_data->restore_state);
 		return size;
 	}
@@ -81,7 +81,7 @@ static ssize_t transient_activate_store(struct device *dev,
 	if (state == 1 && transient_data->activate == 0 &&
 	    transient_data->duration != 0) {
 		transient_data->activate = state;
-		led_set_brightness_async(led_cdev, transient_data->state);
+		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,
@@ -204,7 +204,7 @@ static void transient_trig_deactivate(struct led_classdev *led_cdev)
 
 	if (led_cdev->activated) {
 		del_timer_sync(&transient_data->timer);
-		led_set_brightness_async(led_cdev,
+		led_set_brightness_nosleep(led_cdev,
 					transient_data->restore_state);
 		device_remove_file(led_cdev->dev, &dev_attr_activate);
 		device_remove_file(led_cdev->dev, &dev_attr_duration);
-- 
1.7.9.5

[PATCH 5/5] leds: core: Use set_brightness_work for the blocking op

[Jacek Anaszewski]

This patch makes LED core capable of setting brightness for drivers
that implement brightness_set_blocking op. It removes from LED class
drivers responsibility for using work queues on their own.

In order to achieve this set_brightness_delayed callback is being
modified to call newly added __led_set_brightness helper instead of
led_set_brightness_async.

led_set_brightness_async function didn't set brightness in an
asynchronous way in all cases. It was mistakenly assuming that all
LED subsystem drivers used work queue in their brightness_set op,
whereas only half of them did that. Since it has no users now,
it is being removed.

Signed-off-by: Jacek Anaszewski <j.anaszewski@samsung.com>
---
 drivers/leds/led-core.c |   22 +++++++++++++++++++++-
 drivers/leds/leds.h     |   10 ----------
 include/linux/leds.h    |    2 +-
 3 files changed, 22 insertions(+), 12 deletions(-)

diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
index fe6c2df..d8649f1 100644
--- a/drivers/leds/led-core.c
+++ b/drivers/leds/led-core.c
@@ -25,6 +25,22 @@ EXPORT_SYMBOL_GPL(leds_list_lock);
 LIST_HEAD(leds_list);
 EXPORT_SYMBOL_GPL(leds_list);
 
+static int __led_set_brightness(struct led_classdev *led_cdev,
+				enum led_brightness brightness)
+{
+	int ret = 0;
+
+	if (led_cdev->brightness_set)
+		led_cdev->brightness_set(led_cdev, led_cdev->brightness);
+	else if (led_cdev->brightness_set_blocking)
+		ret = led_cdev->brightness_set_blocking(led_cdev,
+							led_cdev->brightness);
+	else
+		return -EINVAL;
+
+	return ret;
+}
+
 static void led_timer_function(unsigned long data)
 {
 	struct led_classdev *led_cdev = (void *)data;
@@ -83,6 +99,7 @@ static void set_brightness_delayed(struct work_struct *ws)
 {
 	struct led_classdev *led_cdev =
 		container_of(ws, struct led_classdev, set_brightness_work);
+	int ret;
 
 	if (led_cdev->flags & LED_BLINK_DISABLE) {
 		led_cdev->delayed_set_value = LED_OFF;
@@ -90,7 +107,10 @@ static void set_brightness_delayed(struct work_struct *ws)
 		led_cdev->flags &= ~LED_BLINK_DISABLE;
 	}
 
-	led_set_brightness_async(led_cdev, led_cdev->delayed_set_value);
+	ret = __led_set_brightness(led_cdev, led_cdev->delayed_set_value);
+	if (ret < 0)
+		dev_err(led_cdev->dev,
+			"Setting an LED's brightness failed (%d)\n", ret);
 }
 
 static void led_set_software_blink(struct led_classdev *led_cdev,
diff --git a/drivers/leds/leds.h b/drivers/leds/leds.h
index 11520eb..04b8e41 100644
--- a/drivers/leds/leds.h
+++ b/drivers/leds/leds.h
@@ -16,16 +16,6 @@
 #include <linux/rwsem.h>
 #include <linux/leds.h>
 
-static inline void led_set_brightness_async(struct led_classdev *led_cdev,
-					enum led_brightness value)
-{
-	value = min(value, led_cdev->max_brightness);
-	led_cdev->brightness = value;
-
-	if (!(led_cdev->flags & LED_SUSPENDED))
-		led_cdev->brightness_set(led_cdev, value);
-}
-
 static inline int led_set_brightness_sync(struct led_classdev *led_cdev,
 					enum led_brightness value)
 {
diff --git a/include/linux/leds.h b/include/linux/leds.h
index 9f850e6..ae3c178 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -155,7 +155,7 @@ extern void led_blink_set_oneshot(struct led_classdev *led_cdev,
  *
  * Set an LED's brightness, and, if necessary, cancel the
  * software blink timer that implements blinking when the
- * hardware doesn't.
+ * hardware doesn't. This function is guaranteed not to sleep.
  */
 extern void led_set_brightness(struct led_classdev *led_cdev,
 			       enum led_brightness brightness);
-- 
1.7.9.5

Re: [PATCH 5/5] leds: core: Use set_brightness_work for the blocking op

[Sakari Ailus]

Hi Jacek,

Jacek Anaszewski wrote:
> This patch makes LED core capable of setting brightness for drivers
> that implement brightness_set_blocking op. It removes from LED class
> drivers responsibility for using work queues on their own.
> 
> In order to achieve this set_brightness_delayed callback is being
> modified to call newly added __led_set_brightness helper instead of
> led_set_brightness_async.
> 
> led_set_brightness_async function didn't set brightness in an
> asynchronous way in all cases. It was mistakenly assuming that all
> LED subsystem drivers used work queue in their brightness_set op,
> whereas only half of them did that. Since it has no users now,
> it is being removed.
> 
> Signed-off-by: Jacek Anaszewski <j.anaszewski@samsung.com>
> ---
>  drivers/leds/led-core.c |   22 +++++++++++++++++++++-
>  drivers/leds/leds.h     |   10 ----------
>  include/linux/leds.h    |    2 +-
>  3 files changed, 22 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
> index fe6c2df..d8649f1 100644
> --- a/drivers/leds/led-core.c
> +++ b/drivers/leds/led-core.c
> @@ -25,6 +25,22 @@ EXPORT_SYMBOL_GPL(leds_list_lock);
>  LIST_HEAD(leds_list);
>  EXPORT_SYMBOL_GPL(leds_list);
>  
> +static int __led_set_brightness(struct led_classdev *led_cdev,
> +				enum led_brightness brightness)
> +{
> +	int ret = 0;
> +
> +	if (led_cdev->brightness_set)
> +		led_cdev->brightness_set(led_cdev, led_cdev->brightness);
> +	else if (led_cdev->brightness_set_blocking)
> +		ret = led_cdev->brightness_set_blocking(led_cdev,

You can return here, and drop ret.

> +							led_cdev->brightness);
> +	else
> +		return -EINVAL;
> +
> +	return ret;
> +}
> +
>  static void led_timer_function(unsigned long data)
>  {
>  	struct led_classdev *led_cdev = (void *)data;
> @@ -83,6 +99,7 @@ static void set_brightness_delayed(struct work_struct *ws)
>  {
>  	struct led_classdev *led_cdev =
>  		container_of(ws, struct led_classdev, set_brightness_work);
> +	int ret;
>  
>  	if (led_cdev->flags & LED_BLINK_DISABLE) {
>  		led_cdev->delayed_set_value = LED_OFF;
> @@ -90,7 +107,10 @@ static void set_brightness_delayed(struct work_struct *ws)
>  		led_cdev->flags &= ~LED_BLINK_DISABLE;
>  	}
>  
> -	led_set_brightness_async(led_cdev, led_cdev->delayed_set_value);
> +	ret = __led_set_brightness(led_cdev, led_cdev->delayed_set_value);

As __led_set_brightness() isn't used elsewhere and both are rather
simple functions, I'd just move the contents of that function here.

> +	if (ret < 0)
> +		dev_err(led_cdev->dev,
> +			"Setting an LED's brightness failed (%d)\n", ret);
>  }
>  
>  static void led_set_software_blink(struct led_classdev *led_cdev,
> diff --git a/drivers/leds/leds.h b/drivers/leds/leds.h
> index 11520eb..04b8e41 100644
> --- a/drivers/leds/leds.h
> +++ b/drivers/leds/leds.h
> @@ -16,16 +16,6 @@
>  #include <linux/rwsem.h>
>  #include <linux/leds.h>
>  
> -static inline void led_set_brightness_async(struct led_classdev *led_cdev,
> -					enum led_brightness value)
> -{
> -	value = min(value, led_cdev->max_brightness);
> -	led_cdev->brightness = value;
> -
> -	if (!(led_cdev->flags & LED_SUSPENDED))
> -		led_cdev->brightness_set(led_cdev, value);
> -}
> -
>  static inline int led_set_brightness_sync(struct led_classdev *led_cdev,
>  					enum led_brightness value)
>  {
> diff --git a/include/linux/leds.h b/include/linux/leds.h
> index 9f850e6..ae3c178 100644
> --- a/include/linux/leds.h
> +++ b/include/linux/leds.h
> @@ -155,7 +155,7 @@ extern void led_blink_set_oneshot(struct led_classdev *led_cdev,
>   *
>   * Set an LED's brightness, and, if necessary, cancel the
>   * software blink timer that implements blinking when the
> - * hardware doesn't.
> + * hardware doesn't. This function is guaranteed not to sleep.
>   */
>  extern void led_set_brightness(struct led_classdev *led_cdev,
>  			       enum led_brightness brightness);
> 

-- 
Kind regards,

Sakari Ailus
sakari.ailus@linux.intel.com

Re: [PATCH 2/5] leds: core: Add LED_BLINK_CHANGE and LED_BLINK_DISABLE flags

[Andrew Lunn]

On Wed, Sep 16, 2015 at 12:47:41PM +0200, Jacek Anaszewski wrote:
> This patch adds LED_BLINK_CHANGE flag to indicate that blink brightness
> has changed

Hi Jacek

The name LED_BLINK_CHANGE does not make me think of changing
brightness. Maybe a better name would be LED_BLINK_BRIGHTNESS_CHANGE,
although that it getting a bit long.

, and LED_BLINK_DISABLE flag to indicate that blinking
> deactivation has been requested. In order to use the flags
> led_timer_function and set_brightness_delayed callbacks as well as
> led_set_brightness function are being modified. The main goal of these
> modifications is to prepare set_brightness_work for extension of the
> scope of its responsibilities.
> 
> Signed-off-by: Jacek Anaszewski <j.anaszewski@samsung.com>
> ---
>  drivers/leds/led-core.c |   36 ++++++++++++++++++++++++++----------
>  include/linux/leds.h    |   10 ++++++----
>  2 files changed, 32 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
> index 2cb4e37..1968e24 100644
> --- a/drivers/leds/led-core.c
> +++ b/drivers/leds/led-core.c
> @@ -44,18 +44,18 @@ static void led_timer_function(unsigned long data)
>  	brightness = led_get_brightness(led_cdev);
>  	if (!brightness) {
>  		/* Time to switch the LED on. */
> -		if (led_cdev->delayed_set_value) {
> -			led_cdev->blink_brightness =
> -					led_cdev->delayed_set_value;
> -			led_cdev->delayed_set_value = 0;
> -		}
>  		brightness = led_cdev->blink_brightness;
>  		delay = led_cdev->blink_delay_on;
>  	} else {
>  		/* Store the current brightness value to be able
>  		 * to restore it when the delay_off period is over.
> +		 * Do it only if there is no pending blink brightness
> +		 * change, to avoid overwriting the new value.
>  		 */
> -		led_cdev->blink_brightness = brightness;
> +		if (!(led_cdev->flags & LED_BLINK_CHANGE))
> +			led_cdev->blink_brightness = brightness;
> +		else
> +			led_cdev->flags &= ~LED_BLINK_CHANGE;
>  		brightness = LED_OFF;
>  		delay = led_cdev->blink_delay_off;
>  	}
> @@ -84,7 +84,11 @@ static void set_brightness_delayed(struct work_struct *ws)
>  	struct led_classdev *led_cdev =
>  		container_of(ws, struct led_classdev, set_brightness_work);
>  
> -	led_stop_software_blink(led_cdev);
> +	if (led_cdev->flags & LED_BLINK_DISABLE) {
> +		led_cdev->delayed_set_value = LED_OFF;
> +		led_stop_software_blink(led_cdev);
> +		led_cdev->flags &= ~LED_BLINK_DISABLE;
> +	}
>  
>  	led_set_brightness_async(led_cdev, led_cdev->delayed_set_value);
>  }
> @@ -192,11 +196,23 @@ void led_set_brightness(struct led_classdev *led_cdev,
>  {
>  	int ret = 0;
>  
> -	/* delay brightness if soft-blink is active */
> +	/*
> +	 * In case blinking is on delay brightness setting
> +	 * until the next timer tick.
> +	 */
>  	if (led_cdev->blink_delay_on || led_cdev->blink_delay_off) {
> -		led_cdev->delayed_set_value = brightness;
> -		if (brightness == LED_OFF)
> +		/*
> +		 * If need to disable soft blinking delegate this to the

If _we_ need to...

> +		 * work queue task to avoid problems in case we are
> +		 * called from hard irq context.
> +		 */
> +		if (brightness == LED_OFF) {
> +			led_cdev->flags |= LED_BLINK_DISABLE;
>  			schedule_work(&led_cdev->set_brightness_work);
> +		} else {
> +			led_cdev->flags |= LED_BLINK_CHANGE;
> +			led_cdev->blink_brightness = brightness;
> +		}
>  		return;
>  	}
>  
> diff --git a/include/linux/leds.h b/include/linux/leds.h
> index b122eea..188352c 100644
> --- a/include/linux/leds.h
> +++ b/include/linux/leds.h
> @@ -44,10 +44,12 @@ struct led_classdev {
>  #define LED_BLINK_ONESHOT	(1 << 17)
>  #define LED_BLINK_ONESHOT_STOP	(1 << 18)
>  #define LED_BLINK_INVERT	(1 << 19)
> -#define LED_SYSFS_DISABLE	(1 << 20)
> -#define SET_BRIGHTNESS_ASYNC	(1 << 21)
> -#define SET_BRIGHTNESS_SYNC	(1 << 22)
> -#define LED_DEV_CAP_FLASH	(1 << 23)
> +#define LED_BLINK_CHANGE	(1 << 20)
> +#define LED_BLINK_DISABLE	(1 << 21)
> +#define LED_SYSFS_DISABLE	(1 << 22)
> +#define SET_BRIGHTNESS_ASYNC	(1 << 23)
> +#define SET_BRIGHTNESS_SYNC	(1 << 24)
> +#define LED_DEV_CAP_FLASH	(1 << 25)
>  
>  	/* Set LED brightness level */
>  	/* Must not sleep, use a workqueue if needed */
> -- 
> 1.7.9.5
> 

Re: [PATCH 3/5] leds: Rename brightness_set_sync op to brightness_set_blocking

[Andrew Lunn]

On Wed, Sep 16, 2015 at 12:47:42PM +0200, Jacek Anaszewski wrote:
> The initial purpose of brightness_set_sync op, introduced along with
> the LED flash class extension, was to add a means for setting torch LED
> brightness as soon as possible, which couldn't have been guaranteed by
> brightness_set op. This patch renames the op to brightness_set_blocking,
> which describes its purpose in a more generic way, and is beneficial
> in view of the prospective changes in the core related to using
> LED core's set_brightness_work for setting brightness for LED class
> drivers that can sleep or use delays while setting brightness.

...

> -	/*
> -	 * Set LED brightness level immediately - it can block the caller for
> -	 * the time required for accessing a LED device register.
> -	 */
> -	int		(*brightness_set_sync)(struct led_classdev *led_cdev,
> -					enum led_brightness brightness);
> +	/* Can sleep or use delays */
> +	int (*brightness_set_blocking)(struct led_classdev *led_cdev,

I'm no expert when it comes to flash photography with digital
cameras. But to me the old comment seems better. Doesn't the caller
want to know the flash is now giving out light? The new comment gives
no indication of this, where as the old one does?

   Andrew


>  	/* Get LED brightness level */
>  	enum led_brightness (*brightness_get)(struct led_classdev *led_cdev);
>  
> -- 
> 1.7.9.5
> 

Re: [PATCH 4/5] leds: core: Add an internal led_set_brightness_nosleep function

[Andrew Lunn]

> +void led_set_brightness_nosleep(struct led_classdev *led_cdev,
> +					enum led_brightness value)
> +{
> +	led_cdev->brightness = min(value, led_cdev->max_brightness);
> +
> +	if (led_cdev->flags & LED_SUSPENDED)
> +		return;
> +
> +	/* Use brightness_set op if available, it is guaranteed not to sleep */
> +	if (led_cdev->brightness_set)
> +		led_cdev->brightness_set(led_cdev, led_cdev->brightness);
> +
> +	/* If brightness setting can sleep, delegate it to a work queue task */
> +	led_cdev->delayed_set_value = led_cdev->brightness;
> +	schedule_work(&led_cdev->set_brightness_work);
> +}

To me, it looks like there is a missing else, or return here. We don't
want both led_cdev->brightness_set() and the work queue.


> +EXPORT_SYMBOL(led_set_brightness_nosleep);

There seems to be a mixture of EXPORT_SYMBOL and
EXPORT_SYMBOL_GPL. Have you figured out the pattern? led-class.c seems
to be all _GPL, but led-core.c has a mixture.

   Andrew

Re: [PATCH 1/5] leds: core: Move LED core callbacks out of led-class.c

[Andrew Lunn]

On Wed, Sep 16, 2015 at 12:47:40PM +0200, Jacek Anaszewski wrote:
> Since the API for controlling LED brightness and blinking is defined in
> the LED core, move the related timer and work callbacks to the led-core.c,
> and initialize them through a new led_core_init API.
> 
> Signed-off-by: Jacek Anaszewski <j.anaszewski@samsung.com>

Acked-by: Andrew Lunn <andrew@lunn.ch>

Andrew

> ---
>  drivers/leds/led-class.c |   69 +------------------------------------------
>  drivers/leds/led-core.c  |   73 ++++++++++++++++++++++++++++++++++++++++++++++
>  drivers/leds/leds.h      |    1 +
>  3 files changed, 75 insertions(+), 68 deletions(-)
> 
> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
> index ca51d58..7385f98 100644
> --- a/drivers/leds/led-class.c
> +++ b/drivers/leds/led-class.c
> @@ -102,70 +102,6 @@ static const struct attribute_group *led_groups[] = {
>  	NULL,
>  };
>  
> -static void led_timer_function(unsigned long data)
> -{
> -	struct led_classdev *led_cdev = (void *)data;
> -	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;
> -	}
> -
> -	if (led_cdev->flags & LED_BLINK_ONESHOT_STOP) {
> -		led_cdev->flags &= ~LED_BLINK_ONESHOT_STOP;
> -		return;
> -	}
> -
> -	brightness = led_get_brightness(led_cdev);
> -	if (!brightness) {
> -		/* Time to switch the LED on. */
> -		if (led_cdev->delayed_set_value) {
> -			led_cdev->blink_brightness =
> -					led_cdev->delayed_set_value;
> -			led_cdev->delayed_set_value = 0;
> -		}
> -		brightness = led_cdev->blink_brightness;
> -		delay = led_cdev->blink_delay_on;
> -	} else {
> -		/* Store the current brightness value to be able
> -		 * to restore it when the delay_off period is over.
> -		 */
> -		led_cdev->blink_brightness = brightness;
> -		brightness = LED_OFF;
> -		delay = led_cdev->blink_delay_off;
> -	}
> -
> -	led_set_brightness_async(led_cdev, brightness);
> -
> -	/* Return in next iteration if led is in one-shot mode and we are in
> -	 * the final blink state so that the led is toggled each delay_on +
> -	 * delay_off milliseconds in worst case.
> -	 */
> -	if (led_cdev->flags & LED_BLINK_ONESHOT) {
> -		if (led_cdev->flags & LED_BLINK_INVERT) {
> -			if (brightness)
> -				led_cdev->flags |= LED_BLINK_ONESHOT_STOP;
> -		} else {
> -			if (!brightness)
> -				led_cdev->flags |= LED_BLINK_ONESHOT_STOP;
> -		}
> -	}
> -
> -	mod_timer(&led_cdev->blink_timer, jiffies + msecs_to_jiffies(delay));
> -}
> -
> -static void set_brightness_delayed(struct work_struct *ws)
> -{
> -	struct led_classdev *led_cdev =
> -		container_of(ws, struct led_classdev, set_brightness_work);
> -
> -	led_stop_software_blink(led_cdev);
> -
> -	led_set_brightness_async(led_cdev, led_cdev->delayed_set_value);
> -}
> -
>  /**
>   * led_classdev_suspend - suspend an led_classdev.
>   * @led_cdev: the led_classdev to suspend.
> @@ -283,10 +219,7 @@ int led_classdev_register(struct device *parent, struct led_classdev *led_cdev)
>  
>  	led_update_brightness(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);
> +	led_init_core(led_cdev);
>  
>  #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 549de7e..2cb4e37 100644
> --- a/drivers/leds/led-core.c
> +++ b/drivers/leds/led-core.c
> @@ -25,6 +25,70 @@ EXPORT_SYMBOL_GPL(leds_list_lock);
>  LIST_HEAD(leds_list);
>  EXPORT_SYMBOL_GPL(leds_list);
>  
> +static void led_timer_function(unsigned long data)
> +{
> +	struct led_classdev *led_cdev = (void *)data;
> +	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;
> +	}
> +
> +	if (led_cdev->flags & LED_BLINK_ONESHOT_STOP) {
> +		led_cdev->flags &= ~LED_BLINK_ONESHOT_STOP;
> +		return;
> +	}
> +
> +	brightness = led_get_brightness(led_cdev);
> +	if (!brightness) {
> +		/* Time to switch the LED on. */
> +		if (led_cdev->delayed_set_value) {
> +			led_cdev->blink_brightness =
> +					led_cdev->delayed_set_value;
> +			led_cdev->delayed_set_value = 0;
> +		}
> +		brightness = led_cdev->blink_brightness;
> +		delay = led_cdev->blink_delay_on;
> +	} else {
> +		/* Store the current brightness value to be able
> +		 * to restore it when the delay_off period is over.
> +		 */
> +		led_cdev->blink_brightness = brightness;
> +		brightness = LED_OFF;
> +		delay = led_cdev->blink_delay_off;
> +	}
> +
> +	led_set_brightness_async(led_cdev, brightness);
> +
> +	/* Return in next iteration if led is in one-shot mode and we are in
> +	 * the final blink state so that the led is toggled each delay_on +
> +	 * delay_off milliseconds in worst case.
> +	 */
> +	if (led_cdev->flags & LED_BLINK_ONESHOT) {
> +		if (led_cdev->flags & LED_BLINK_INVERT) {
> +			if (brightness)
> +				led_cdev->flags |= LED_BLINK_ONESHOT_STOP;
> +		} else {
> +			if (!brightness)
> +				led_cdev->flags |= LED_BLINK_ONESHOT_STOP;
> +		}
> +	}
> +
> +	mod_timer(&led_cdev->blink_timer, jiffies + msecs_to_jiffies(delay));
> +}
> +
> +static void set_brightness_delayed(struct work_struct *ws)
> +{
> +	struct led_classdev *led_cdev =
> +		container_of(ws, struct led_classdev, set_brightness_work);
> +
> +	led_stop_software_blink(led_cdev);
> +
> +	led_set_brightness_async(led_cdev, led_cdev->delayed_set_value);
> +}
> +
>  static void led_set_software_blink(struct led_classdev *led_cdev,
>  				   unsigned long delay_on,
>  				   unsigned long delay_off)
> @@ -72,6 +136,15 @@ static void led_blink_setup(struct led_classdev *led_cdev,
>  	led_set_software_blink(led_cdev, *delay_on, *delay_off);
>  }
>  
> +void led_init_core(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);
> +}
> +EXPORT_SYMBOL(led_init_core);
> +
>  void led_blink_set(struct led_classdev *led_cdev,
>  		   unsigned long *delay_on,
>  		   unsigned long *delay_off)
> diff --git a/drivers/leds/leds.h b/drivers/leds/leds.h
> index bc89d7a..4238fbc 100644
> --- a/drivers/leds/leds.h
> +++ b/drivers/leds/leds.h
> @@ -44,6 +44,7 @@ static inline int led_get_brightness(struct led_classdev *led_cdev)
>  	return led_cdev->brightness;
>  }
>  
> +void led_init_core(struct led_classdev *led_cdev);
>  void led_stop_software_blink(struct led_classdev *led_cdev);
>  
>  extern struct rw_semaphore leds_list_lock;
> -- 
> 1.7.9.5
> 

Re: [PATCH 2/5] leds: core: Add LED_BLINK_CHANGE and LED_BLINK_DISABLE flags

[Jacek Anaszewski]

Hi Andrew,

Thanks for the review.

On 09/22/2015 08:44 PM, Andrew Lunn wrote:
> On Wed, Sep 16, 2015 at 12:47:41PM +0200, Jacek Anaszewski wrote:
>> This patch adds LED_BLINK_CHANGE flag to indicate that blink brightness
>> has changed
>
> Hi Jacek
>
> The name LED_BLINK_CHANGE does not make me think of changing
> brightness. Maybe a better name would be LED_BLINK_BRIGHTNESS_CHANGE,
> although that it getting a bit long.

I had the same dilemma. We could go for shortcut like
LED_BLINK_BR_CHANGE, but is is not too meaningful either.
I will rename LED_BLINK_CHANGE to LED_BLINK_BRIGHTNESS_CHANGE, unless
better option appears.

> , and LED_BLINK_DISABLE flag to indicate that blinking
>> deactivation has been requested. In order to use the flags
>> led_timer_function and set_brightness_delayed callbacks as well as
>> led_set_brightness function are being modified. The main goal of these
>> modifications is to prepare set_brightness_work for extension of the
>> scope of its responsibilities.
>>
>> Signed-off-by: Jacek Anaszewski <j.anaszewski@samsung.com>
>> ---
>>   drivers/leds/led-core.c |   36 ++++++++++++++++++++++++++----------
>>   include/linux/leds.h    |   10 ++++++----
>>   2 files changed, 32 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
>> index 2cb4e37..1968e24 100644
>> --- a/drivers/leds/led-core.c
>> +++ b/drivers/leds/led-core.c
>> @@ -44,18 +44,18 @@ static void led_timer_function(unsigned long data)
>>   	brightness = led_get_brightness(led_cdev);
>>   	if (!brightness) {
>>   		/* Time to switch the LED on. */
>> -		if (led_cdev->delayed_set_value) {
>> -			led_cdev->blink_brightness =
>> -					led_cdev->delayed_set_value;
>> -			led_cdev->delayed_set_value = 0;
>> -		}
>>   		brightness = led_cdev->blink_brightness;
>>   		delay = led_cdev->blink_delay_on;
>>   	} else {
>>   		/* Store the current brightness value to be able
>>   		 * to restore it when the delay_off period is over.
>> +		 * Do it only if there is no pending blink brightness
>> +		 * change, to avoid overwriting the new value.
>>   		 */
>> -		led_cdev->blink_brightness = brightness;
>> +		if (!(led_cdev->flags & LED_BLINK_CHANGE))
>> +			led_cdev->blink_brightness = brightness;
>> +		else
>> +			led_cdev->flags &= ~LED_BLINK_CHANGE;
>>   		brightness = LED_OFF;
>>   		delay = led_cdev->blink_delay_off;
>>   	}
>> @@ -84,7 +84,11 @@ static void set_brightness_delayed(struct work_struct *ws)
>>   	struct led_classdev *led_cdev =
>>   		container_of(ws, struct led_classdev, set_brightness_work);
>>
>> -	led_stop_software_blink(led_cdev);
>> +	if (led_cdev->flags & LED_BLINK_DISABLE) {
>> +		led_cdev->delayed_set_value = LED_OFF;
>> +		led_stop_software_blink(led_cdev);
>> +		led_cdev->flags &= ~LED_BLINK_DISABLE;
>> +	}
>>
>>   	led_set_brightness_async(led_cdev, led_cdev->delayed_set_value);
>>   }
>> @@ -192,11 +196,23 @@ void led_set_brightness(struct led_classdev *led_cdev,
>>   {
>>   	int ret = 0;
>>
>> -	/* delay brightness if soft-blink is active */
>> +	/*
>> +	 * In case blinking is on delay brightness setting
>> +	 * until the next timer tick.
>> +	 */
>>   	if (led_cdev->blink_delay_on || led_cdev->blink_delay_off) {
>> -		led_cdev->delayed_set_value = brightness;
>> -		if (brightness == LED_OFF)
>> +		/*
>> +		 * If need to disable soft blinking delegate this to the
>
> If _we_ need to...

OK.

>> +		 * work queue task to avoid problems in case we are
>> +		 * called from hard irq context.
>> +		 */
>> +		if (brightness == LED_OFF) {
>> +			led_cdev->flags |= LED_BLINK_DISABLE;
>>   			schedule_work(&led_cdev->set_brightness_work);
>> +		} else {
>> +			led_cdev->flags |= LED_BLINK_CHANGE;
>> +			led_cdev->blink_brightness = brightness;
>> +		}
>>   		return;
>>   	}
>>
>> diff --git a/include/linux/leds.h b/include/linux/leds.h
>> index b122eea..188352c 100644
>> --- a/include/linux/leds.h
>> +++ b/include/linux/leds.h
>> @@ -44,10 +44,12 @@ struct led_classdev {
>>   #define LED_BLINK_ONESHOT	(1 << 17)
>>   #define LED_BLINK_ONESHOT_STOP	(1 << 18)
>>   #define LED_BLINK_INVERT	(1 << 19)
>> -#define LED_SYSFS_DISABLE	(1 << 20)
>> -#define SET_BRIGHTNESS_ASYNC	(1 << 21)
>> -#define SET_BRIGHTNESS_SYNC	(1 << 22)
>> -#define LED_DEV_CAP_FLASH	(1 << 23)
>> +#define LED_BLINK_CHANGE	(1 << 20)
>> +#define LED_BLINK_DISABLE	(1 << 21)
>> +#define LED_SYSFS_DISABLE	(1 << 22)
>> +#define SET_BRIGHTNESS_ASYNC	(1 << 23)
>> +#define SET_BRIGHTNESS_SYNC	(1 << 24)
>> +#define LED_DEV_CAP_FLASH	(1 << 25)
>>
>>   	/* Set LED brightness level */
>>   	/* Must not sleep, use a workqueue if needed */
>> --
>> 1.7.9.5
>>
>


-- 
Best Regards,
Jacek Anaszewski

Re: [PATCH 3/5] leds: Rename brightness_set_sync op to brightness_set_blocking

[Jacek Anaszewski]

On 09/22/2015 08:54 PM, Andrew Lunn wrote:
> On Wed, Sep 16, 2015 at 12:47:42PM +0200, Jacek Anaszewski wrote:
>> The initial purpose of brightness_set_sync op, introduced along with
>> the LED flash class extension, was to add a means for setting torch LED
>> brightness as soon as possible, which couldn't have been guaranteed by
>> brightness_set op. This patch renames the op to brightness_set_blocking,
>> which describes its purpose in a more generic way, and is beneficial
>> in view of the prospective changes in the core related to using
>> LED core's set_brightness_work for setting brightness for LED class
>> drivers that can sleep or use delays while setting brightness.
>
> ...
>
>> -	/*
>> -	 * Set LED brightness level immediately - it can block the caller for
>> -	 * the time required for accessing a LED device register.
>> -	 */
>> -	int		(*brightness_set_sync)(struct led_classdev *led_cdev,
>> -					enum led_brightness brightness);
>> +	/* Can sleep or use delays */
>> +	int (*brightness_set_blocking)(struct led_classdev *led_cdev,
>
> I'm no expert when it comes to flash photography with digital
> cameras.

This op is now not specific to flash LEDs. The last sentence in the
commit message explains this, but now I see that it is too long.
Let's change it to:

"This patch renames the op to brightness_set_blocking,
which describes its purpose in a more generic way. It is beneficial
in view of the prospective changes in the LED core, aiming at removing
the need for using work queues in LED class drivers that can sleep
or use delays while setting brightness."

> But to me the old comment seems better.

I changed it to highlight the essence of how it differs from
brightness_set.

> Doesn't the caller
> want to know the flash is now giving out light?

We have strobe_get op for this, but it is in led-class-flash extension.
This is irrelevant here.

> The new comment gives
> no indication of this, where as the old one does?



-- 
Best Regards,
Jacek Anaszewski

Re: [PATCH 4/5] leds: core: Add an internal led_set_brightness_nosleep function

[Jacek Anaszewski]

On 09/22/2015 09:02 PM, Andrew Lunn wrote:
>> +void led_set_brightness_nosleep(struct led_classdev *led_cdev,
>> +					enum led_brightness value)
>> +{
>> +	led_cdev->brightness = min(value, led_cdev->max_brightness);
>> +
>> +	if (led_cdev->flags & LED_SUSPENDED)
>> +		return;
>> +
>> +	/* Use brightness_set op if available, it is guaranteed not to sleep */
>> +	if (led_cdev->brightness_set)
>> +		led_cdev->brightness_set(led_cdev, led_cdev->brightness);
>> +
>> +	/* If brightness setting can sleep, delegate it to a work queue task */
>> +	led_cdev->delayed_set_value = led_cdev->brightness;
>> +	schedule_work(&led_cdev->set_brightness_work);
>> +}
>
> To me, it looks like there is a missing else, or return here. We don't
> want both led_cdev->brightness_set() and the work queue.

Thanks for spotting this.

>> +EXPORT_SYMBOL(led_set_brightness_nosleep);
>
> There seems to be a mixture of EXPORT_SYMBOL and
> EXPORT_SYMBOL_GPL. Have you figured out the pattern? led-class.c seems
> to be all _GPL, but led-core.c has a mixture.

In fact, this is weird. I didn't pay sufficient attention to this,
I must admit. We should switch to using EXPORT_SYMBOL_GPL consequently [1].

[1] https://lwn.net/Articles/603187/

-- 
Best Regards,
Jacek Anaszewski

Re: [PATCH 3/5] leds: Rename brightness_set_sync op to brightness_set_blocking

[Jacek Anaszewski]

On 09/23/2015 10:36 AM, Jacek Anaszewski wrote:
> On 09/22/2015 08:54 PM, Andrew Lunn wrote:
>> On Wed, Sep 16, 2015 at 12:47:42PM +0200, Jacek Anaszewski wrote:
>>> The initial purpose of brightness_set_sync op, introduced along with
>>> the LED flash class extension, was to add a means for setting torch LED
>>> brightness as soon as possible, which couldn't have been guaranteed by
>>> brightness_set op. This patch renames the op to brightness_set_blocking,
>>> which describes its purpose in a more generic way, and is beneficial
>>> in view of the prospective changes in the core related to using
>>> LED core's set_brightness_work for setting brightness for LED class
>>> drivers that can sleep or use delays while setting brightness.
>>
>> ...
>>
>>> -    /*
>>> -     * Set LED brightness level immediately - it can block the
>>> caller for
>>> -     * the time required for accessing a LED device register.
>>> -     */
>>> -    int        (*brightness_set_sync)(struct led_classdev *led_cdev,
>>> -                    enum led_brightness brightness);
>>> +    /* Can sleep or use delays */
>>> +    int (*brightness_set_blocking)(struct led_classdev *led_cdev,
>>
>> I'm no expert when it comes to flash photography with digital
>> cameras.
>
> This op is now not specific to flash LEDs. The last sentence in the
> commit message explains this, but now I see that it is too long.
> Let's change it to:
>
> "This patch renames the op to brightness_set_blocking,
> which describes its purpose in a more generic way. It is beneficial
> in view of the prospective changes in the LED core, aiming at removing
> the need for using work queues in LED class drivers that can sleep
> or use delays while setting brightness."
>
>> But to me the old comment seems better.
>
> I changed it to highlight the essence of how it differs from
> brightness_set.
>
>> Doesn't the caller
>> want to know the flash is now giving out light?
>
> We have strobe_get op for this, but it is in led-class-flash extension.
> This is irrelevant here.

I focused myself on flash mode of flash LED, but you asked
probably about brightness in torch mode, which can be obtained
with brightness_get op, if implemented by the driver.


-- 
Best Regards,
Jacek Anaszewski

Re: [PATCH 1/5] leds: core: Move LED core callbacks out of led-class.c

[Pavel Machek]

On Wed 2015-09-16 12:47:40, Jacek Anaszewski wrote:
> Since the API for controlling LED brightness and blinking is defined in
> the LED core, move the related timer and work callbacks to the led-core.c,
> and initialize them through a new led_core_init API.
> 
> Signed-off-by: Jacek Anaszewski <j.anaszewski@samsung.com>

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

Re: [PATCH 2/5] leds: core: Add LED_BLINK_CHANGE and LED_BLINK_DISABLE flags

[Pavel Machek]

On Wed 2015-09-16 12:47:41, Jacek Anaszewski wrote:
> This patch adds LED_BLINK_CHANGE flag to indicate that blink brightness
> has changed, and LED_BLINK_DISABLE flag to indicate that blinking
> deactivation has been requested. In order to use the flags
> led_timer_function and set_brightness_delayed callbacks as well as
> led_set_brightness function are being modified. The main goal of these
> modifications is to prepare set_brightness_work for extension of the
> scope of its responsibilities.
> 
> Signed-off-by: Jacek Anaszewski <j.anaszewski@samsung.com>

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


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

Re: [PATCH 3/5] leds: Rename brightness_set_sync op to brightness_set_blocking

[Pavel Machek]

On Wed 2015-09-16 12:47:42, Jacek Anaszewski wrote:
> The initial purpose of brightness_set_sync op, introduced along with
> the LED flash class extension, was to add a means for setting torch LED
> brightness as soon as possible, which couldn't have been guaranteed by
> brightness_set op. This patch renames the op to brightness_set_blocking,
> which describes its purpose in a more generic way, and is beneficial
> in view of the prospective changes in the core related to using
> LED core's set_brightness_work for setting brightness for LED class
> drivers that can sleep or use delays while setting brightness.
> 
> Signed-off-by: Jacek Anaszewski <j.anaszewski@samsung.com>

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

Re: [PATCH 1/5] leds: core: Move LED core callbacks out of led-class.c

[Jacek Anaszewski]

On 10/06/2015 05:31 PM, Pavel Machek wrote:
> On Wed 2015-09-16 12:47:40, Jacek Anaszewski wrote:
>> Since the API for controlling LED brightness and blinking is defined in
>> the LED core, move the related timer and work callbacks to the led-core.c,
>> and initialize them through a new led_core_init API.
>>
>> Signed-off-by: Jacek Anaszewski <j.anaszewski@samsung.com>
>
> Acked-by: Pavel Machek <pavel@ucw.cz>
>
>

Applied, thanks.

-- 
Best Regards,
Jacek Anaszewski