[PATCH 0/5] LED flash: Set brightness in a sync way on demand

[PATCH 0/5] LED flash: Set brightness in a sync way on demand

[Jacek Anaszewski]

Since it has become apparent that user should decide whether brightness
is to be set in a synchronous or an asynchronousi way, export internal
led_set_brightness_sync API and remove SET_BRIGHTNESS_SYNC and
SET_BRIGHTNESS_ASYNC flags from the LED core. After patch set [1] flash
LED drivers don't longer have to implement brightness_set op and use work
queues internally. Remove the redundant code from them.

Thanks,
Jacek Anaszewski

[1] https://lkml.org/lkml/2015/9/16/299 

Jacek Anaszewski (5):
  leds: core: Drivers shouldn't enforce SYNC/ASYNC brightness setting
  Documentation: leds: Add description of brightness setting API
  leds: max77693: Remove work queue
  leds: aat1290: Remove work queue
  leds: ktd2692: Remove work queue

 Documentation/leds/leds-class.txt |   21 ++++++++++++++
 drivers/leds/led-class-flash.c    |    4 ---
 drivers/leds/led-class.c          |    2 --
 drivers/leds/led-core.c           |   33 ++++++++++++---------
 drivers/leds/leds-aat1290.c       |   50 +++++++++-----------------------
 drivers/leds/leds-ktd2692.c       |   41 ++++----------------------
 drivers/leds/leds-max77693.c      |   57 ++++++-------------------------------
 drivers/leds/leds.h               |   13 ---------
 include/linux/leds.h              |   19 +++++++++++--
 9 files changed, 84 insertions(+), 156 deletions(-)

-- 
1.7.9.5

[PATCH 1/5] leds: core: Drivers shouldn't enforce SYNC/ASYNC brightness setting

[Jacek Anaszewski]

This patch removes SET_BRIGHTNESS_ASYNC and SET_BRIGHTNESS flags.
led_set_brightness now calls led_set_brightness_nosleep instead of
choosing between sync and async op basing on the flags defined by the
driver.

>From now on, if a user wants to make sure that brightness will be set
synchronously, they have to use led_set_brightness_sync API. It is now
being made publicly available since it has become apparent that it is
a caller who should decide whether brightness is to be set in
a synchronous or an asynchronous way.

Signed-off-by: Jacek Anaszewski <j.anaszewski@samsung.com>
---
 drivers/leds/led-class-flash.c |    4 ----
 drivers/leds/led-class.c       |    2 --
 drivers/leds/led-core.c        |   33 +++++++++++++++++++--------------
 drivers/leds/leds.h            |   13 -------------
 include/linux/leds.h           |   19 ++++++++++++++++---
 5 files changed, 35 insertions(+), 36 deletions(-)

diff --git a/drivers/leds/led-class-flash.c b/drivers/leds/led-class-flash.c
index 300a2c9..f53783b 100644
--- a/drivers/leds/led-class-flash.c
+++ b/drivers/leds/led-class-flash.c
@@ -316,10 +316,6 @@ int led_classdev_flash_register(struct device *parent,
 	if (ret < 0)
 		return ret;
 
-	/* Setting a torch brightness needs to have immediate effect */
-	led_cdev->flags &= ~SET_BRIGHTNESS_ASYNC;
-	led_cdev->flags |= SET_BRIGHTNESS_SYNC;
-
 	return 0;
 }
 EXPORT_SYMBOL_GPL(led_classdev_flash_register);
diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
index 7385f98..37eec1a 100644
--- a/drivers/leds/led-class.c
+++ b/drivers/leds/led-class.c
@@ -215,8 +215,6 @@ int led_classdev_register(struct device *parent, struct led_classdev *led_cdev)
 	if (!led_cdev->max_brightness)
 		led_cdev->max_brightness = LED_FULL;
 
-	led_cdev->flags |= SET_BRIGHTNESS_ASYNC;
-
 	led_update_brightness(led_cdev);
 
 	led_init_core(led_cdev);
diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
index d8649f1..d0eb838 100644
--- a/drivers/leds/led-core.c
+++ b/drivers/leds/led-core.c
@@ -215,8 +215,6 @@ EXPORT_SYMBOL_GPL(led_stop_software_blink);
 void led_set_brightness(struct led_classdev *led_cdev,
 			enum led_brightness brightness)
 {
-	int ret = 0;
-
 	/*
 	 * In case blinking is on delay brightness setting
 	 * until the next timer tick.
@@ -235,19 +233,9 @@ void led_set_brightness(struct led_classdev *led_cdev,
 			led_cdev->blink_brightness = brightness;
 		}
 		return;
-	}
-
-	if (led_cdev->flags & SET_BRIGHTNESS_ASYNC) {
+	} else {
 		led_set_brightness_nosleep(led_cdev, brightness);
-		return;
-	} else if (led_cdev->flags & SET_BRIGHTNESS_SYNC)
-		ret = led_set_brightness_sync(led_cdev, brightness);
-	else
-		ret = -EINVAL;
-
-	if (ret < 0)
-		dev_dbg(led_cdev->dev, "Setting LED brightness failed (%d)\n",
-			ret);
+	}
 }
 EXPORT_SYMBOL(led_set_brightness);
 
@@ -269,6 +257,23 @@ void led_set_brightness_nosleep(struct led_classdev *led_cdev,
 }
 EXPORT_SYMBOL(led_set_brightness_nosleep);
 
+int led_set_brightness_sync(struct led_classdev *led_cdev,
+			    enum led_brightness value)
+{
+	WARN_ON(led_cdev->blink_delay_on || led_cdev->blink_delay_off);
+
+	led_cdev->brightness = min(value, led_cdev->max_brightness);
+
+	if (led_cdev->flags & LED_SUSPENDED)
+		return 0;
+
+	if (led_cdev->brightness_set_blocking)
+		return led_cdev->brightness_set_blocking(led_cdev,
+							 led_cdev->brightness);
+	return -EINVAL;
+}
+EXPORT_SYMBOL(led_set_brightness_sync);
+
 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 04b8e41..62d9eba 100644
--- a/drivers/leds/leds.h
+++ b/drivers/leds/leds.h
@@ -16,19 +16,6 @@
 #include <linux/rwsem.h>
 #include <linux/leds.h>
 
-static inline int led_set_brightness_sync(struct led_classdev *led_cdev,
-					enum led_brightness value)
-{
-	int ret = 0;
-
-	led_cdev->brightness = min(value, led_cdev->max_brightness);
-
-	if (!(led_cdev->flags & LED_SUSPENDED))
-		ret = led_cdev->brightness_set_blocking(led_cdev,
-						led_cdev->brightness);
-	return ret;
-}
-
 static inline int led_get_brightness(struct led_classdev *led_cdev)
 {
 	return led_cdev->brightness;
diff --git a/include/linux/leds.h b/include/linux/leds.h
index ae3c178..2019929 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -47,9 +47,7 @@ struct led_classdev {
 #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)
+#define LED_DEV_CAP_FLASH	(1 << 23)
 
 	/* Set LED brightness level */
 	/* Must not sleep, use a workqueue if needed */
@@ -159,6 +157,21 @@ extern void led_blink_set_oneshot(struct led_classdev *led_cdev,
  */
 extern void led_set_brightness(struct led_classdev *led_cdev,
 			       enum led_brightness brightness);
+
+/**
+ * led_set_brightness_sync - set LED brightness synchronously
+ * @led_cdev: the LED to set
+ * @brightness: the brightness to set it to
+ *
+ * Set an LED's brightness immediately. This function will block
+ * the caller for the time required for accessing device register,
+ * and it can sleep.
+ *
+ * Returns: 0 on success or negative error value on failure
+ */
+extern int led_set_brightness_sync(struct led_classdev *led_cdev,
+				   enum led_brightness value);
+
 /**
  * led_update_brightness - update LED brightness
  * @led_cdev: the LED to query
-- 
1.7.9.5

[PATCH 2/5] Documentation: leds: Add description of brightness setting API

[Jacek Anaszewski]

This patch adds description of the LED subsystem API for
setting an LED brightness.

Signed-off-by: Jacek Anaszewski <j.anaszewski@samsung.com>
---
 Documentation/leds/leds-class.txt |   21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/Documentation/leds/leds-class.txt b/Documentation/leds/leds-class.txt
index 62261c0..2cc38fa 100644
--- a/Documentation/leds/leds-class.txt
+++ b/Documentation/leds/leds-class.txt
@@ -52,6 +52,27 @@ above leaves scope for further attributes should they be needed. If sections
 of the name don't apply, just leave that section blank.
 
 
+Brightness setting API
+======================
+
+LED subsystem core exposes following API for setting brightness:
+
+    - led_set_brightness : if necessary, cancels the software blink timer that
+		implements blinking when the hardware doesn't; it is guaranteed
+		not to sleep, which implies the possibility of delegating the
+		job to a work queue task (uses led_set_brightness_nosleep
+		underneath - see below),
+    - led_set_brightness_sync : for use cases when immediate effect is desired;
+		it can block the caller for the time required for accessing
+		device registers and can sleep,
+    - led_set_brightness_nosleep : sets an LEDs brightness using either
+		brightness_set op, which is guaranteed not to sleep, or, if only
+		brightness_set_blocking op is available, delegates it to a work
+		queue task; this API is inteded for use by LED core and
+		led-triggers, as they can be called from atomic context, and thus
+		cannot sleep.
+
+
 Hardware accelerated blink of LEDs
 ==================================
 
-- 
1.7.9.5

[PATCH 3/5] leds: max77693: Remove work queue

[Jacek Anaszewski]

Now the core implements the work queue, remove it from the drivers,
and switch to using brightness_set_blocking op.

Signed-off-by: Jacek Anaszewski <j.anaszewski@samsung.com>
---
 drivers/leds/leds-max77693.c |   57 +++++++-----------------------------------
 1 file changed, 9 insertions(+), 48 deletions(-)

diff --git a/drivers/leds/leds-max77693.c b/drivers/leds/leds-max77693.c
index f69bcb4..28f3d05 100644
--- a/drivers/leds/leds-max77693.c
+++ b/drivers/leds/leds-max77693.c
@@ -19,7 +19,6 @@
 #include <linux/platform_device.h>
 #include <linux/regmap.h>
 #include <linux/slab.h>
-#include <linux/workqueue.h>
 #include <media/v4l2-flash-led-class.h>
 
 #define MODE_OFF		0
@@ -61,8 +60,6 @@ struct max77693_sub_led {
 	int fled_id;
 	/* corresponding LED Flash class device */
 	struct led_classdev_flash fled_cdev;
-	/* assures led-triggers compatibility */
-	struct work_struct work_brightness_set;
 	/* V4L2 Flash device */
 	struct v4l2_flash *v4l2_flash;
 
@@ -462,10 +459,14 @@ static int max77693_setup(struct max77693_led_device *led,
 	return max77693_set_mode_reg(led, MODE_OFF);
 }
 
-static int __max77693_led_brightness_set(struct max77693_led_device *led,
-					int fled_id, enum led_brightness value)
+/* LED subsystem callbacks */
+static int max77693_led_brightness_set(struct led_classdev *led_cdev,
+					enum led_brightness value)
 {
-	int ret;
+	struct led_classdev_flash *fled_cdev = lcdev_to_flcdev(led_cdev);
+	struct max77693_sub_led *sub_led = flcdev_to_sub_led(fled_cdev);
+	struct max77693_led_device *led = sub_led_to_led(sub_led);
+	int fled_id = sub_led->fled_id, ret;
 
 	mutex_lock(&led->lock);
 
@@ -493,43 +494,8 @@ static int __max77693_led_brightness_set(struct max77693_led_device *led,
 			ret);
 unlock:
 	mutex_unlock(&led->lock);
-	return ret;
-}
 
-static void max77693_led_brightness_set_work(
-					struct work_struct *work)
-{
-	struct max77693_sub_led *sub_led =
-			container_of(work, struct max77693_sub_led,
-					work_brightness_set);
-	struct max77693_led_device *led = sub_led_to_led(sub_led);
-
-	__max77693_led_brightness_set(led, sub_led->fled_id,
-				sub_led->torch_brightness);
-}
-
-/* LED subsystem callbacks */
-
-static int max77693_led_brightness_set_sync(
-				struct led_classdev *led_cdev,
-				enum led_brightness value)
-{
-	struct led_classdev_flash *fled_cdev = lcdev_to_flcdev(led_cdev);
-	struct max77693_sub_led *sub_led = flcdev_to_sub_led(fled_cdev);
-	struct max77693_led_device *led = sub_led_to_led(sub_led);
-
-	return __max77693_led_brightness_set(led, sub_led->fled_id, value);
-}
-
-static void max77693_led_brightness_set(
-				struct led_classdev *led_cdev,
-				enum led_brightness value)
-{
-	struct led_classdev_flash *fled_cdev = lcdev_to_flcdev(led_cdev);
-	struct max77693_sub_led *sub_led = flcdev_to_sub_led(fled_cdev);
-
-	sub_led->torch_brightness = value;
-	schedule_work(&sub_led->work_brightness_set);
+	return ret;
 }
 
 static int max77693_led_flash_brightness_set(
@@ -930,16 +896,13 @@ 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_blocking = max77693_led_brightness_set_sync;
+	led_cdev->brightness_set_blocking = max77693_led_brightness_set;
 	led_cdev->max_brightness = (led->iout_joint ?
 					led_cfg->iout_torch_max[FLED1] +
 					led_cfg->iout_torch_max[FLED2] :
 					led_cfg->iout_torch_max[fled_id]) /
 				   TORCH_IOUT_STEP;
 	led_cdev->flags |= LED_DEV_CAP_FLASH;
-	INIT_WORK(&sub_led->work_brightness_set,
-			max77693_led_brightness_set_work);
 
 	max77693_init_flash_settings(sub_led, led_cfg);
 
@@ -1061,13 +1024,11 @@ static int max77693_led_remove(struct platform_device *pdev)
 	if (led->iout_joint || max77693_fled_used(led, FLED1)) {
 		v4l2_flash_release(sub_leds[FLED1].v4l2_flash);
 		led_classdev_flash_unregister(&sub_leds[FLED1].fled_cdev);
-		cancel_work_sync(&sub_leds[FLED1].work_brightness_set);
 	}
 
 	if (!led->iout_joint && max77693_fled_used(led, FLED2)) {
 		v4l2_flash_release(sub_leds[FLED2].v4l2_flash);
 		led_classdev_flash_unregister(&sub_leds[FLED2].fled_cdev);
-		cancel_work_sync(&sub_leds[FLED2].work_brightness_set);
 	}
 
 	mutex_destroy(&led->lock);
-- 
1.7.9.5

[PATCH 4/5] leds: aat1290: Remove work queue

[Jacek Anaszewski]

Now the core implements the work queue, remove it from the drivers,
and switch to using brightness_set_blocking op.

Signed-off-by: Jacek Anaszewski <j.anaszewski@samsung.com>
---
 drivers/leds/leds-aat1290.c |   50 +++++++++++--------------------------------
 1 file changed, 13 insertions(+), 37 deletions(-)

diff --git a/drivers/leds/leds-aat1290.c b/drivers/leds/leds-aat1290.c
index 4bff3b5..d802fbb 100644
--- a/drivers/leds/leds-aat1290.c
+++ b/drivers/leds/leds-aat1290.c
@@ -20,7 +20,6 @@
 #include <linux/pinctrl/consumer.h>
 #include <linux/platform_device.h>
 #include <linux/slab.h>
-#include <linux/workqueue.h>
 #include <media/v4l2-flash-led-class.h>
 
 #define AAT1290_MOVIE_MODE_CURRENT_ADDR	17
@@ -82,8 +81,6 @@ struct aat1290_led {
 
 	/* brightness cache */
 	unsigned int torch_brightness;
-	/* assures led-triggers compatibility */
-	struct work_struct work_brightness_set;
 };
 
 static struct aat1290_led *fled_cdev_to_led(
@@ -92,6 +89,12 @@ static struct aat1290_led *fled_cdev_to_led(
 	return container_of(fled_cdev, struct aat1290_led, fled_cdev);
 }
 
+static struct led_classdev_flash *led_cdev_to_fled_cdev(
+				struct led_classdev *led_cdev)
+{
+	return container_of(led_cdev, struct led_classdev_flash, led_cdev);
+}
+
 static void aat1290_as2cwire_write(struct aat1290_led *led, int addr, int value)
 {
 	int i;
@@ -134,9 +137,14 @@ static void aat1290_set_flash_safety_timer(struct aat1290_led *led,
 							flash_tm_reg);
 }
 
-static void aat1290_brightness_set(struct aat1290_led *led,
+/* LED subsystem callbacks */
+
+static int aat1290_led_brightness_set(struct led_classdev *led_cdev,
 					enum led_brightness brightness)
 {
+	struct led_classdev_flash *fled_cdev = led_cdev_to_fled_cdev(led_cdev);
+	struct aat1290_led *led = fled_cdev_to_led(fled_cdev);
+
 	mutex_lock(&led->lock);
 
 	if (brightness == 0) {
@@ -158,35 +166,6 @@ static void aat1290_brightness_set(struct aat1290_led *led,
 	}
 
 	mutex_unlock(&led->lock);
-}
-
-/* LED subsystem callbacks */
-
-static void aat1290_brightness_set_work(struct work_struct *work)
-{
-	struct aat1290_led *led =
-		container_of(work, struct aat1290_led, work_brightness_set);
-
-	aat1290_brightness_set(led, led->torch_brightness);
-}
-
-static void aat1290_led_brightness_set(struct led_classdev *led_cdev,
-					enum led_brightness brightness)
-{
-	struct led_classdev_flash *fled_cdev = lcdev_to_flcdev(led_cdev);
-	struct aat1290_led *led = fled_cdev_to_led(fled_cdev);
-
-	led->torch_brightness = brightness;
-	schedule_work(&led->work_brightness_set);
-}
-
-static int aat1290_led_brightness_set_sync(struct led_classdev *led_cdev,
-					enum led_brightness brightness)
-{
-	struct led_classdev_flash *fled_cdev = lcdev_to_flcdev(led_cdev);
-	struct aat1290_led *led = fled_cdev_to_led(fled_cdev);
-
-	aat1290_brightness_set(led, brightness);
 
 	return 0;
 }
@@ -509,11 +488,9 @@ static int aat1290_led_probe(struct platform_device *pdev)
 	mutex_init(&led->lock);
 
 	/* Initialize LED Flash class device */
-	led_cdev->brightness_set = aat1290_led_brightness_set;
-	led_cdev->brightness_set_blocking = aat1290_led_brightness_set_sync;
+	led_cdev->brightness_set_blocking = aat1290_led_brightness_set;
 	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);
 
 	aat1290_init_flash_timeout(led, &led_cfg);
 
@@ -548,7 +525,6 @@ static int aat1290_led_remove(struct platform_device *pdev)
 
 	v4l2_flash_release(led->v4l2_flash);
 	led_classdev_flash_unregister(&led->fled_cdev);
-	cancel_work_sync(&led->work_brightness_set);
 
 	mutex_destroy(&led->lock);
 
-- 
1.7.9.5

[PATCH 5/5] leds: ktd2692: Remove work queue

[Jacek Anaszewski]

Now the core implements the work queue, remove it from the drivers,
and switch to using brightness_set_blocking op.

Signed-off-by: Jacek Anaszewski <j.anaszewski@samsung.com>
Cc: Ingi Kim <ingi2.kim@samsung.com>
---
 drivers/leds/leds-ktd2692.c |   41 ++++++-----------------------------------
 1 file changed, 6 insertions(+), 35 deletions(-)

diff --git a/drivers/leds/leds-ktd2692.c b/drivers/leds/leds-ktd2692.c
index 4c14bed..d70af1a 100644
--- a/drivers/leds/leds-ktd2692.c
+++ b/drivers/leds/leds-ktd2692.c
@@ -18,7 +18,6 @@
 #include <linux/of.h>
 #include <linux/platform_device.h>
 #include <linux/regulator/consumer.h>
-#include <linux/workqueue.h>
 
 /* Value related the movie mode */
 #define KTD2692_MOVIE_MODE_CURRENT_LEVELS	16
@@ -82,7 +81,6 @@ struct ktd2692_context {
 	/* secures access to the device */
 	struct mutex lock;
 	struct regulator *regulator;
-	struct work_struct work_brightness_set;
 
 	struct gpio_desc *aux_gpio;
 	struct gpio_desc *ctrl_gpio;
@@ -158,9 +156,12 @@ static void ktd2692_expresswire_write(struct ktd2692_context *led, u8 value)
 	ktd2692_expresswire_end(led);
 }
 
-static void ktd2692_brightness_set(struct ktd2692_context *led,
-				   enum led_brightness brightness)
+static int ktd2692_led_brightness_set(struct led_classdev *led_cdev,
+				       enum led_brightness brightness)
 {
+	struct led_classdev_flash *fled_cdev = lcdev_to_flcdev(led_cdev);
+	struct ktd2692_context *led = fled_cdev_to_led(fled_cdev);
+
 	mutex_lock(&led->lock);
 
 	if (brightness == LED_OFF) {
@@ -174,33 +175,6 @@ static void ktd2692_brightness_set(struct ktd2692_context *led,
 
 	ktd2692_expresswire_write(led, led->mode | KTD2692_REG_MODE_BASE);
 	mutex_unlock(&led->lock);
-}
-
-static void ktd2692_brightness_set_work(struct work_struct *work)
-{
-	struct ktd2692_context *led =
-		container_of(work, struct ktd2692_context, work_brightness_set);
-
-	ktd2692_brightness_set(led, led->torch_brightness);
-}
-
-static void ktd2692_led_brightness_set(struct led_classdev *led_cdev,
-				       enum led_brightness brightness)
-{
-	struct led_classdev_flash *fled_cdev = lcdev_to_flcdev(led_cdev);
-	struct ktd2692_context *led = fled_cdev_to_led(fled_cdev);
-
-	led->torch_brightness = brightness;
-	schedule_work(&led->work_brightness_set);
-}
-
-static int ktd2692_led_brightness_set_sync(struct led_classdev *led_cdev,
-					   enum led_brightness brightness)
-{
-	struct led_classdev_flash *fled_cdev = lcdev_to_flcdev(led_cdev);
-	struct ktd2692_context *led = fled_cdev_to_led(fled_cdev);
-
-	ktd2692_brightness_set(led, brightness);
 
 	return 0;
 }
@@ -381,12 +355,10 @@ static int ktd2692_probe(struct platform_device *pdev)
 	fled_cdev->ops = &flash_ops;
 
 	led_cdev->max_brightness = led_cfg.max_brightness;
-	led_cdev->brightness_set = ktd2692_led_brightness_set;
-	led_cdev->brightness_set_blocking = ktd2692_led_brightness_set_sync;
+	led_cdev->brightness_set_blocking = ktd2692_led_brightness_set;
 	led_cdev->flags |= LED_CORE_SUSPENDRESUME | LED_DEV_CAP_FLASH;
 
 	mutex_init(&led->lock);
-	INIT_WORK(&led->work_brightness_set, ktd2692_brightness_set_work);
 
 	platform_set_drvdata(pdev, led);
 
@@ -408,7 +380,6 @@ static int ktd2692_remove(struct platform_device *pdev)
 	int ret;
 
 	led_classdev_flash_unregister(&led->fled_cdev);
-	cancel_work_sync(&led->work_brightness_set);
 
 	if (led->regulator) {
 		ret = regulator_disable(led->regulator);
-- 
1.7.9.5

Re: [PATCH 1/5] leds: core: Drivers shouldn't enforce SYNC/ASYNC brightness setting

[Sakari Ailus]

Hi Jacek,

Thanks for the patchset. A few comments below.

Jacek Anaszewski wrote:
> This patch removes SET_BRIGHTNESS_ASYNC and SET_BRIGHTNESS flags.
> led_set_brightness now calls led_set_brightness_nosleep instead of
> choosing between sync and async op basing on the flags defined by the
> driver.
> 
> From now on, if a user wants to make sure that brightness will be set
> synchronously, they have to use led_set_brightness_sync API. It is now
> being made publicly available since it has become apparent that it is
> a caller who should decide whether brightness is to be set in
> a synchronous or an asynchronous way.
> 
> Signed-off-by: Jacek Anaszewski <j.anaszewski@samsung.com>
> ---
>  drivers/leds/led-class-flash.c |    4 ----
>  drivers/leds/led-class.c       |    2 --
>  drivers/leds/led-core.c        |   33 +++++++++++++++++++--------------
>  drivers/leds/leds.h            |   13 -------------
>  include/linux/leds.h           |   19 ++++++++++++++++---
>  5 files changed, 35 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/leds/led-class-flash.c b/drivers/leds/led-class-flash.c
> index 300a2c9..f53783b 100644
> --- a/drivers/leds/led-class-flash.c
> +++ b/drivers/leds/led-class-flash.c
> @@ -316,10 +316,6 @@ int led_classdev_flash_register(struct device *parent,
>  	if (ret < 0)
>  		return ret;
>  
> -	/* Setting a torch brightness needs to have immediate effect */
> -	led_cdev->flags &= ~SET_BRIGHTNESS_ASYNC;
> -	led_cdev->flags |= SET_BRIGHTNESS_SYNC;
> -
>  	return 0;
>  }
>  EXPORT_SYMBOL_GPL(led_classdev_flash_register);
> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
> index 7385f98..37eec1a 100644
> --- a/drivers/leds/led-class.c
> +++ b/drivers/leds/led-class.c
> @@ -215,8 +215,6 @@ int led_classdev_register(struct device *parent, struct led_classdev *led_cdev)
>  	if (!led_cdev->max_brightness)
>  		led_cdev->max_brightness = LED_FULL;
>  
> -	led_cdev->flags |= SET_BRIGHTNESS_ASYNC;
> -
>  	led_update_brightness(led_cdev);
>  
>  	led_init_core(led_cdev);
> diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
> index d8649f1..d0eb838 100644
> --- a/drivers/leds/led-core.c
> +++ b/drivers/leds/led-core.c
> @@ -215,8 +215,6 @@ EXPORT_SYMBOL_GPL(led_stop_software_blink);
>  void led_set_brightness(struct led_classdev *led_cdev,
>  			enum led_brightness brightness)
>  {
> -	int ret = 0;
> -
>  	/*
>  	 * In case blinking is on delay brightness setting
>  	 * until the next timer tick.
> @@ -235,19 +233,9 @@ void led_set_brightness(struct led_classdev *led_cdev,
>  			led_cdev->blink_brightness = brightness;
>  		}
>  		return;
> -	}
> -
> -	if (led_cdev->flags & SET_BRIGHTNESS_ASYNC) {
> +	} else {

Useless use of else.

>  		led_set_brightness_nosleep(led_cdev, brightness);
> -		return;
> -	} else if (led_cdev->flags & SET_BRIGHTNESS_SYNC)
> -		ret = led_set_brightness_sync(led_cdev, brightness);
> -	else
> -		ret = -EINVAL;
> -
> -	if (ret < 0)
> -		dev_dbg(led_cdev->dev, "Setting LED brightness failed (%d)\n",
> -			ret);
> +	}
>  }
>  EXPORT_SYMBOL(led_set_brightness);
>  
> @@ -269,6 +257,23 @@ void led_set_brightness_nosleep(struct led_classdev *led_cdev,
>  }
>  EXPORT_SYMBOL(led_set_brightness_nosleep);
>  
> +int led_set_brightness_sync(struct led_classdev *led_cdev,
> +			    enum led_brightness value)
> +{
> +	WARN_ON(led_cdev->blink_delay_on || led_cdev->blink_delay_off);

How about simply returning an error instead?

> +
> +	led_cdev->brightness = min(value, led_cdev->max_brightness);
> +
> +	if (led_cdev->flags & LED_SUSPENDED)
> +		return 0;
> +
> +	if (led_cdev->brightness_set_blocking)
> +		return led_cdev->brightness_set_blocking(led_cdev,
> +							 led_cdev->brightness);
> +	return -EINVAL;
> +}
> +EXPORT_SYMBOL(led_set_brightness_sync);
> +
>  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 04b8e41..62d9eba 100644
> --- a/drivers/leds/leds.h
> +++ b/drivers/leds/leds.h
> @@ -16,19 +16,6 @@
>  #include <linux/rwsem.h>
>  #include <linux/leds.h>
>  
> -static inline int led_set_brightness_sync(struct led_classdev *led_cdev,
> -					enum led_brightness value)
> -{
> -	int ret = 0;
> -
> -	led_cdev->brightness = min(value, led_cdev->max_brightness);
> -
> -	if (!(led_cdev->flags & LED_SUSPENDED))
> -		ret = led_cdev->brightness_set_blocking(led_cdev,
> -						led_cdev->brightness);
> -	return ret;
> -}
> -
>  static inline int led_get_brightness(struct led_classdev *led_cdev)
>  {
>  	return led_cdev->brightness;
> diff --git a/include/linux/leds.h b/include/linux/leds.h
> index ae3c178..2019929 100644
> --- a/include/linux/leds.h
> +++ b/include/linux/leds.h
> @@ -47,9 +47,7 @@ struct led_classdev {
>  #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)
> +#define LED_DEV_CAP_FLASH	(1 << 23)
>  
>  	/* Set LED brightness level */
>  	/* Must not sleep, use a workqueue if needed */
> @@ -159,6 +157,21 @@ extern void led_blink_set_oneshot(struct led_classdev *led_cdev,
>   */
>  extern void led_set_brightness(struct led_classdev *led_cdev,
>  			       enum led_brightness brightness);
> +
> +/**
> + * led_set_brightness_sync - set LED brightness synchronously
> + * @led_cdev: the LED to set
> + * @brightness: the brightness to set it to
> + *
> + * Set an LED's brightness immediately. This function will block
> + * the caller for the time required for accessing device register,
> + * and it can sleep.
> + *
> + * Returns: 0 on success or negative error value on failure
> + */
> +extern int led_set_brightness_sync(struct led_classdev *led_cdev,
> +				   enum led_brightness value);
> +
>  /**
>   * led_update_brightness - update LED brightness
>   * @led_cdev: the LED to query
> 

-- 
Kind regards,

Sakari Ailus
sakari.ailus@linux.intel.com

Re: [PATCH 1/5] leds: core: Drivers shouldn't enforce SYNC/ASYNC brightness setting

[Jacek Anaszewski]

Hi Sakari,

Thanks for the review.
Just to avoid a confusion - this patch set depends on [1], which is
not merged yet and also needs a review. I reimplemented and split [2]
into two patch sets, so that [1] contained only modifications required
for removing work queues from LED class drivers, but yesterday decided
to send also the remaining part.

[1] https://lkml.org/lkml/2015/9/16/299
[2] https://lkml.org/lkml/2015/8/20/426

On 09/22/2015 08:42 AM, Sakari Ailus wrote:
> Hi Jacek,
>
> Thanks for the patchset. A few comments below.
>
> Jacek Anaszewski wrote:
>> This patch removes SET_BRIGHTNESS_ASYNC and SET_BRIGHTNESS flags.
>> led_set_brightness now calls led_set_brightness_nosleep instead of
>> choosing between sync and async op basing on the flags defined by the
>> driver.
>>
>>  From now on, if a user wants to make sure that brightness will be set
>> synchronously, they have to use led_set_brightness_sync API. It is now
>> being made publicly available since it has become apparent that it is
>> a caller who should decide whether brightness is to be set in
>> a synchronous or an asynchronous way.
>>
>> Signed-off-by: Jacek Anaszewski <j.anaszewski@samsung.com>
>> ---
>>   drivers/leds/led-class-flash.c |    4 ----
>>   drivers/leds/led-class.c       |    2 --
>>   drivers/leds/led-core.c        |   33 +++++++++++++++++++--------------
>>   drivers/leds/leds.h            |   13 -------------
>>   include/linux/leds.h           |   19 ++++++++++++++++---
>>   5 files changed, 35 insertions(+), 36 deletions(-)
>>
>> diff --git a/drivers/leds/led-class-flash.c b/drivers/leds/led-class-flash.c
>> index 300a2c9..f53783b 100644
>> --- a/drivers/leds/led-class-flash.c
>> +++ b/drivers/leds/led-class-flash.c
>> @@ -316,10 +316,6 @@ int led_classdev_flash_register(struct device *parent,
>>   	if (ret < 0)
>>   		return ret;
>>
>> -	/* Setting a torch brightness needs to have immediate effect */
>> -	led_cdev->flags &= ~SET_BRIGHTNESS_ASYNC;
>> -	led_cdev->flags |= SET_BRIGHTNESS_SYNC;
>> -
>>   	return 0;
>>   }
>>   EXPORT_SYMBOL_GPL(led_classdev_flash_register);
>> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
>> index 7385f98..37eec1a 100644
>> --- a/drivers/leds/led-class.c
>> +++ b/drivers/leds/led-class.c
>> @@ -215,8 +215,6 @@ int led_classdev_register(struct device *parent, struct led_classdev *led_cdev)
>>   	if (!led_cdev->max_brightness)
>>   		led_cdev->max_brightness = LED_FULL;
>>
>> -	led_cdev->flags |= SET_BRIGHTNESS_ASYNC;
>> -
>>   	led_update_brightness(led_cdev);
>>
>>   	led_init_core(led_cdev);
>> diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
>> index d8649f1..d0eb838 100644
>> --- a/drivers/leds/led-core.c
>> +++ b/drivers/leds/led-core.c
>> @@ -215,8 +215,6 @@ EXPORT_SYMBOL_GPL(led_stop_software_blink);
>>   void led_set_brightness(struct led_classdev *led_cdev,
>>   			enum led_brightness brightness)
>>   {
>> -	int ret = 0;
>> -
>>   	/*
>>   	 * In case blinking is on delay brightness setting
>>   	 * until the next timer tick.
>> @@ -235,19 +233,9 @@ void led_set_brightness(struct led_classdev *led_cdev,
>>   			led_cdev->blink_brightness = brightness;
>>   		}
>>   		return;
>> -	}
>> -
>> -	if (led_cdev->flags & SET_BRIGHTNESS_ASYNC) {
>> +	} else {
>
> Useless use of else.

Right.

>>   		led_set_brightness_nosleep(led_cdev, brightness);
>> -		return;
>> -	} else if (led_cdev->flags & SET_BRIGHTNESS_SYNC)
>> -		ret = led_set_brightness_sync(led_cdev, brightness);
>> -	else
>> -		ret = -EINVAL;
>> -
>> -	if (ret < 0)
>> -		dev_dbg(led_cdev->dev, "Setting LED brightness failed (%d)\n",
>> -			ret);
>> +	}
>>   }
>>   EXPORT_SYMBOL(led_set_brightness);
>>
>> @@ -269,6 +257,23 @@ void led_set_brightness_nosleep(struct led_classdev *led_cdev,
>>   }
>>   EXPORT_SYMBOL(led_set_brightness_nosleep);
>>
>> +int led_set_brightness_sync(struct led_classdev *led_cdev,
>> +			    enum led_brightness value)
>> +{
>> +	WARN_ON(led_cdev->blink_delay_on || led_cdev->blink_delay_off);
>
> How about simply returning an error instead?

Indeed, it will be more relevant here.

>> +
>> +	led_cdev->brightness = min(value, led_cdev->max_brightness);
>> +
>> +	if (led_cdev->flags & LED_SUSPENDED)
>> +		return 0;
>> +
>> +	if (led_cdev->brightness_set_blocking)
>> +		return led_cdev->brightness_set_blocking(led_cdev,
>> +							 led_cdev->brightness);
>> +	return -EINVAL;
>> +}
>> +EXPORT_SYMBOL(led_set_brightness_sync);
>> +
>>   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 04b8e41..62d9eba 100644
>> --- a/drivers/leds/leds.h
>> +++ b/drivers/leds/leds.h
>> @@ -16,19 +16,6 @@
>>   #include <linux/rwsem.h>
>>   #include <linux/leds.h>
>>
>> -static inline int led_set_brightness_sync(struct led_classdev *led_cdev,
>> -					enum led_brightness value)
>> -{
>> -	int ret = 0;
>> -
>> -	led_cdev->brightness = min(value, led_cdev->max_brightness);
>> -
>> -	if (!(led_cdev->flags & LED_SUSPENDED))
>> -		ret = led_cdev->brightness_set_blocking(led_cdev,
>> -						led_cdev->brightness);
>> -	return ret;
>> -}
>> -
>>   static inline int led_get_brightness(struct led_classdev *led_cdev)
>>   {
>>   	return led_cdev->brightness;
>> diff --git a/include/linux/leds.h b/include/linux/leds.h
>> index ae3c178..2019929 100644
>> --- a/include/linux/leds.h
>> +++ b/include/linux/leds.h
>> @@ -47,9 +47,7 @@ struct led_classdev {
>>   #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)
>> +#define LED_DEV_CAP_FLASH	(1 << 23)
>>
>>   	/* Set LED brightness level */
>>   	/* Must not sleep, use a workqueue if needed */
>> @@ -159,6 +157,21 @@ extern void led_blink_set_oneshot(struct led_classdev *led_cdev,
>>    */
>>   extern void led_set_brightness(struct led_classdev *led_cdev,
>>   			       enum led_brightness brightness);
>> +
>> +/**
>> + * led_set_brightness_sync - set LED brightness synchronously
>> + * @led_cdev: the LED to set
>> + * @brightness: the brightness to set it to
>> + *
>> + * Set an LED's brightness immediately. This function will block
>> + * the caller for the time required for accessing device register,
>> + * and it can sleep.
>> + *
>> + * Returns: 0 on success or negative error value on failure
>> + */
>> +extern int led_set_brightness_sync(struct led_classdev *led_cdev,
>> +				   enum led_brightness value);
>> +
>>   /**
>>    * led_update_brightness - update LED brightness
>>    * @led_cdev: the LED to query
>>
>


-- 
Best Regards,
Jacek Anaszewski

Re: [PATCH 2/5] Documentation: leds: Add description of brightness setting API

[Sakari Ailus]

Hi Jacek,

Jacek Anaszewski wrote:
> This patch adds description of the LED subsystem API for
> setting an LED brightness.
> 
> Signed-off-by: Jacek Anaszewski <j.anaszewski@samsung.com>
> ---
>  Documentation/leds/leds-class.txt |   21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/Documentation/leds/leds-class.txt b/Documentation/leds/leds-class.txt
> index 62261c0..2cc38fa 100644
> --- a/Documentation/leds/leds-class.txt
> +++ b/Documentation/leds/leds-class.txt
> @@ -52,6 +52,27 @@ above leaves scope for further attributes should they be needed. If sections
>  of the name don't apply, just leave that section blank.
>  
>  
> +Brightness setting API
> +======================
> +
> +LED subsystem core exposes following API for setting brightness:
> +
> +    - led_set_brightness : if necessary, cancels the software blink timer that
> +		implements blinking when the hardware doesn't; it is guaranteed
> +		not to sleep, which implies the possibility of delegating the
> +		job to a work queue task (uses led_set_brightness_nosleep
> +		underneath - see below),
> +    - led_set_brightness_sync : for use cases when immediate effect is desired;
> +		it can block the caller for the time required for accessing
> +		device registers and can sleep,
> +    - led_set_brightness_nosleep : sets an LEDs brightness using either
> +		brightness_set op, which is guaranteed not to sleep, or, if only
> +		brightness_set_blocking op is available, delegates it to a work
> +		queue task; this API is inteded for use by LED core and
> +		led-triggers, as they can be called from atomic context, and thus
> +		cannot sleep.

led_set_brightness_nosleep() isn't part of the API intended to be used
outside the LED framework --- I wouldn't document it here, only the part
relevant for users outside the LED class and triggers frameworks.

> +
> +
>  Hardware accelerated blink of LEDs
>  ==================================
>  
> 


-- 
Kind regards,

Sakari Ailus
sakari.ailus@linux.intel.com

Re: [PATCH 3/5] leds: max77693: Remove work queue

[Sakari Ailus]

Jacek Anaszewski wrote:
> Now the core implements the work queue, remove it from the drivers,
> and switch to using brightness_set_blocking op.
> 
> Signed-off-by: Jacek Anaszewski <j.anaszewski@samsung.com>

Nice patches! For patches 3--5:

Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>

-- 
Sakari Ailus
sakari.ailus@linux.intel.com

Re: [PATCH 2/5] Documentation: leds: Add description of brightness setting API

[Jacek Anaszewski]

Hi Sakari,

On 09/22/2015 12:26 PM, Sakari Ailus wrote:
> Hi Jacek,
>
> Jacek Anaszewski wrote:
>> This patch adds description of the LED subsystem API for
>> setting an LED brightness.
>>
>> Signed-off-by: Jacek Anaszewski <j.anaszewski@samsung.com>
>> ---
>>   Documentation/leds/leds-class.txt |   21 +++++++++++++++++++++
>>   1 file changed, 21 insertions(+)
>>
>> diff --git a/Documentation/leds/leds-class.txt b/Documentation/leds/leds-class.txt
>> index 62261c0..2cc38fa 100644
>> --- a/Documentation/leds/leds-class.txt
>> +++ b/Documentation/leds/leds-class.txt
>> @@ -52,6 +52,27 @@ above leaves scope for further attributes should they be needed. If sections
>>   of the name don't apply, just leave that section blank.
>>
>>
>> +Brightness setting API
>> +======================
>> +
>> +LED subsystem core exposes following API for setting brightness:
>> +
>> +    - led_set_brightness : if necessary, cancels the software blink timer that
>> +		implements blinking when the hardware doesn't; it is guaranteed
>> +		not to sleep, which implies the possibility of delegating the
>> +		job to a work queue task (uses led_set_brightness_nosleep
>> +		underneath - see below),
>> +    - led_set_brightness_sync : for use cases when immediate effect is desired;
>> +		it can block the caller for the time required for accessing
>> +		device registers and can sleep,
>> +    - led_set_brightness_nosleep : sets an LEDs brightness using either
>> +		brightness_set op, which is guaranteed not to sleep, or, if only
>> +		brightness_set_blocking op is available, delegates it to a work
>> +		queue task; this API is inteded for use by LED core and
>> +		led-triggers, as they can be called from atomic context, and thus
>> +		cannot sleep.
>
> led_set_brightness_nosleep() isn't part of the API intended to be used
> outside the LED framework --- I wouldn't document it here, only the part
> relevant for users outside the LED class and triggers frameworks.

OK, I'll move this description to drivers/leds/leds.h, next to the
function declaration.

>> +
>> +
>>   Hardware accelerated blink of LEDs
>>   ==================================
>>
>>
>
>


-- 
Best Regards,
Jacek Anaszewski

Re: [PATCH 1/5] leds: core: Drivers shouldn't enforce SYNC/ASYNC brightness setting

[Andrew Lunn]

> +int led_set_brightness_sync(struct led_classdev *led_cdev,
> +			    enum led_brightness value)
> +{
> +	WARN_ON(led_cdev->blink_delay_on || led_cdev->blink_delay_off);
> +
> +	led_cdev->brightness = min(value, led_cdev->max_brightness);
> +
> +	if (led_cdev->flags & LED_SUSPENDED)
> +		return 0;
> +
> +	if (led_cdev->brightness_set_blocking)
> +		return led_cdev->brightness_set_blocking(led_cdev,
> +							 led_cdev->brightness);
> +	return -EINVAL;

Maybe -ENOTSUP would be better. It is not an invalid argument, the
driver does not support it.


> diff --git a/include/linux/leds.h b/include/linux/leds.h
> index ae3c178..2019929 100644
> --- a/include/linux/leds.h
> +++ b/include/linux/leds.h
> @@ -47,9 +47,7 @@ struct led_classdev {
>  #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)
> +#define LED_DEV_CAP_FLASH	(1 << 23)
>  
>  	/* Set LED brightness level */
>  	/* Must not sleep, use a workqueue if needed */
> @@ -159,6 +157,21 @@ extern void led_blink_set_oneshot(struct led_classdev *led_cdev,
>   */
>  extern void led_set_brightness(struct led_classdev *led_cdev,
>  			       enum led_brightness brightness);
> +
> +/**
> + * led_set_brightness_sync - set LED brightness synchronously
> + * @led_cdev: the LED to set
> + * @brightness: the brightness to set it to
> + *
> + * Set an LED's brightness immediately. This function will block
> + * the caller for the time required for accessing device register,

registers - plural.

	  Andrew

Re: [PATCH 2/5] Documentation: leds: Add description of brightness setting API

[Andrew Lunn]

On Mon, Sep 21, 2015 at 04:29:27PM +0200, Jacek Anaszewski wrote:
> This patch adds description of the LED subsystem API for
> setting an LED brightness.
> 
> Signed-off-by: Jacek Anaszewski <j.anaszewski@samsung.com>
> ---
>  Documentation/leds/leds-class.txt |   21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/Documentation/leds/leds-class.txt b/Documentation/leds/leds-class.txt
> index 62261c0..2cc38fa 100644
> --- a/Documentation/leds/leds-class.txt
> +++ b/Documentation/leds/leds-class.txt
> @@ -52,6 +52,27 @@ above leaves scope for further attributes should they be needed. If sections
>  of the name don't apply, just leave that section blank.
>  
>  
> +Brightness setting API
> +======================
> +
> +LED subsystem core exposes following API for setting brightness:
> +
> +    - led_set_brightness : if necessary, cancels the software blink timer that
> +		implements blinking when the hardware doesn't; it is guaranteed
> +		not to sleep

I would put this in the reverse order. Not sleeping is the most
important bit. Also, stopping blinking should also happen if the
hardware is performing the blinking. There is no need to mention
software blinking, that is an implementation detail.

                which implies the possibility of delegating the
> +		job to a work queue task (uses led_set_brightness_nosleep
> +		underneath - see below),

This bit is also an implementation detail and not relevant to the API.


> +    - led_set_brightness_sync : for use cases when immediate effect is desired;
> +		it can block the caller for the time required for accessing
> +		device registers and can sleep,

In fact, i would probably have a separate paragraph that says passing
LED_OFF to either of these two functions stops blinking.

	Andrew

Re: [PATCH 2/5] Documentation: leds: Add description of brightness setting API

[Jacek Anaszewski]

On 09/22/2015 09:27 PM, Andrew Lunn wrote:
> On Mon, Sep 21, 2015 at 04:29:27PM +0200, Jacek Anaszewski wrote:
>> This patch adds description of the LED subsystem API for
>> setting an LED brightness.
>>
>> Signed-off-by: Jacek Anaszewski <j.anaszewski@samsung.com>
>> ---
>>   Documentation/leds/leds-class.txt |   21 +++++++++++++++++++++
>>   1 file changed, 21 insertions(+)
>>
>> diff --git a/Documentation/leds/leds-class.txt b/Documentation/leds/leds-class.txt
>> index 62261c0..2cc38fa 100644
>> --- a/Documentation/leds/leds-class.txt
>> +++ b/Documentation/leds/leds-class.txt
>> @@ -52,6 +52,27 @@ above leaves scope for further attributes should they be needed. If sections
>>   of the name don't apply, just leave that section blank.
>>
>>
>> +Brightness setting API
>> +======================
>> +
>> +LED subsystem core exposes following API for setting brightness:
>> +
>> +    - led_set_brightness : if necessary, cancels the software blink timer that
>> +		implements blinking when the hardware doesn't; it is guaranteed
>> +		not to sleep
>
> I would put this in the reverse order. Not sleeping is the most
> important bit.

OK.

> Also, stopping blinking should also happen if the
> hardware is performing the blinking. There is no need to mention
> software blinking, that is an implementation detail.

OK.

>
>                  which implies the possibility of delegating the
>> +		job to a work queue task (uses led_set_brightness_nosleep
>> +		underneath - see below),
>
> This bit is also an implementation detail and not relevant to the API.
>
>
>> +    - led_set_brightness_sync : for use cases when immediate effect is desired;
>> +		it can block the caller for the time required for accessing
>> +		device registers and can sleep,
>
> In fact, i would probably have a separate paragraph that says passing
> LED_OFF to either of these two functions stops blinking.

In case of led_set_brightness_sync passing LED_OFF will disable only
hardware blinking. I noticed this discrepancy before a while. We can't
disable soft blinking, because we would have to do it in a work queue
task, which would in turn compromise the contract offering by the API,
i.e. brightness is guaranteed to be set after function returns.
Since Sakari proposed to return error from the function if soft blinking
is on, then this has to be explicitly stated here too.


-- 
Best Regards,
Jacek Anaszewski

Re: [PATCH 1/5] leds: core: Drivers shouldn't enforce SYNC/ASYNC brightness setting

[Pavel Machek]

On Mon 2015-09-21 16:29:26, Jacek Anaszewski wrote:
> This patch removes SET_BRIGHTNESS_ASYNC and SET_BRIGHTNESS flags.
> led_set_brightness now calls led_set_brightness_nosleep instead of
> choosing between sync and async op basing on the flags defined by the
> driver.

Are those flags unused in the version of kernel you are patching?

If so, tell us so in the changelog. Thanks,
								Pavel

Re: [PATCH 5/5] leds: ktd2692: Remove work queue

[Pavel Machek]

On Mon 2015-09-21 16:29:30, Jacek Anaszewski wrote:
> Now the core implements the work queue, remove it from the drivers,
> and switch to using brightness_set_blocking op.
> 
> Signed-off-by: Jacek Anaszewski <j.anaszewski@samsung.com>
> Cc: Ingi Kim <ingi2.kim@samsung.com>

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

Re: [PATCH 1/5] leds: core: Drivers shouldn't enforce SYNC/ASYNC brightness setting

[Jacek Anaszewski]

On 10/08/2015 05:50 PM, Pavel Machek wrote:
> On Mon 2015-09-21 16:29:26, Jacek Anaszewski wrote:
>> This patch removes SET_BRIGHTNESS_ASYNC and SET_BRIGHTNESS flags.

s/SET_BRIGHTNESS/SET_BRIGHTNESS_SYNC/

>> led_set_brightness now calls led_set_brightness_nosleep instead of
>> choosing between sync and async op basing on the flags defined by the
>> driver.
>
> Are those flags unused in the version of kernel you are patching?

The flags are used until this patch.

> If so, tell us so in the changelog. Thanks,


-- 
Best Regards,
Jacek Anaszewski

Re: [PATCH 1/5] leds: core: Drivers shouldn't enforce SYNC/ASYNC brightness setting

[Pavel Machek]

On Fri 2015-10-09 08:28:44, Jacek Anaszewski wrote:
> On 10/08/2015 05:50 PM, Pavel Machek wrote:
> >On Mon 2015-09-21 16:29:26, Jacek Anaszewski wrote:
> >>This patch removes SET_BRIGHTNESS_ASYNC and SET_BRIGHTNESS flags.
> 
> s/SET_BRIGHTNESS/SET_BRIGHTNESS_SYNC/
> 
> >>led_set_brightness now calls led_set_brightness_nosleep instead of
> >>choosing between sync and async op basing on the flags defined by the
> >>driver.
> >
> >Are those flags unused in the version of kernel you are patching?
> 
> The flags are used until this patch.

Hmm. I seen you removing the definitions, but not removing any
users. I guess I missed something.

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

Re: [PATCH 1/5] leds: core: Drivers shouldn't enforce SYNC/ASYNC brightness setting

[Jacek Anaszewski]

On 10/09/2015 09:02 AM, Pavel Machek wrote:
> On Fri 2015-10-09 08:28:44, Jacek Anaszewski wrote:
>> On 10/08/2015 05:50 PM, Pavel Machek wrote:
>>> On Mon 2015-09-21 16:29:26, Jacek Anaszewski wrote:
>>>> This patch removes SET_BRIGHTNESS_ASYNC and SET_BRIGHTNESS flags.
>>
>> s/SET_BRIGHTNESS/SET_BRIGHTNESS_SYNC/
>>
>>>> led_set_brightness now calls led_set_brightness_nosleep instead of
>>>> choosing between sync and async op basing on the flags defined by the
>>>> driver.
>>>
>>> Are those flags unused in the version of kernel you are patching?
>>
>> The flags are used until this patch.
>
> Hmm. I seen you removing the definitions, but not removing any
> users. I guess I missed something.

Only LED core is the user. It sets SET_BRIGHTNESS_SYNC flag
for LED flash class drivers.

-- 
Best Regards,
Jacek Anaszewski

Re: [PATCH 1/5] leds: core: Drivers shouldn't enforce SYNC/ASYNC brightness setting

[Pavel Machek]

On Fri 2015-10-09 10:08:12, Jacek Anaszewski wrote:
> On 10/09/2015 09:02 AM, Pavel Machek wrote:
> >On Fri 2015-10-09 08:28:44, Jacek Anaszewski wrote:
> >>On 10/08/2015 05:50 PM, Pavel Machek wrote:
> >>>On Mon 2015-09-21 16:29:26, Jacek Anaszewski wrote:
> >>>>This patch removes SET_BRIGHTNESS_ASYNC and SET_BRIGHTNESS flags.
> >>
> >>s/SET_BRIGHTNESS/SET_BRIGHTNESS_SYNC/
> >>
> >>>>led_set_brightness now calls led_set_brightness_nosleep instead of
> >>>>choosing between sync and async op basing on the flags defined by the
> >>>>driver.
> >>>
> >>>Are those flags unused in the version of kernel you are patching?
> >>
> >>The flags are used until this patch.
> >
> >Hmm. I seen you removing the definitions, but not removing any
> >users. I guess I missed something.
> 
> Only LED core is the user. It sets SET_BRIGHTNESS_SYNC flag
> for LED flash class drivers.

Aha. All is right, then.

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