This patch rearranges the core LED subsystem code, so that it
now shifts the responsibility for using work queues from drivers,
in case their brightness_set ops can sleep, onto the LED core
Addition of two flags: LED_BRIGHTNESS_FAST and LED_BLINK_DISABLE
as well as new_brightness_value property to the struct led_classdev
allows for employing existing set_brightness_work to do the job.
The modifications allows also to get rid of brightness_set_sync op,
as flash LED devices can now be handled properly only basing on the
SET_BRIGHTNESS_SYNC flag.
Signed-off-by: Jacek Anaszewski <[email protected]>
---
drivers/leds/led-class.c | 18 +++++++----
drivers/leds/led-core.c | 50 +++++++++++++++++------------
drivers/leds/leds.h | 41 +++++++++++++----------
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 | 9 ++----
10 files changed, 84 insertions(+), 66 deletions(-)
diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
index beabfbc..07ccaeb 100644
--- a/drivers/leds/led-class.c
+++ b/drivers/leds/led-class.c
@@ -109,7 +109,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;
}
@@ -121,10 +121,10 @@ 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) {
+ if (led_cdev->new_brightness_value) {
led_cdev->blink_brightness =
- led_cdev->delayed_set_value;
- led_cdev->delayed_set_value = 0;
+ led_cdev->new_brightness_value;
+ led_cdev->new_brightness_value = 0;
}
brightness = led_cdev->blink_brightness;
delay = led_cdev->blink_delay_on;
@@ -137,7 +137,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 +
@@ -161,9 +161,13 @@ 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_stop_software_blink(led_cdev);
+ led_cdev->flags &= ~LED_BLINK_DISABLE;
+ return;
+ }
- led_set_brightness_async(led_cdev, led_cdev->delayed_set_value);
+ __led_set_brightness(led_cdev, led_cdev->delayed_set_value);
}
/**
diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
index 549de7e..0e13fb0 100644
--- a/drivers/leds/led-core.c
+++ b/drivers/leds/led-core.c
@@ -42,13 +42,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;
}
@@ -117,27 +118,36 @@ EXPORT_SYMBOL_GPL(led_stop_software_blink);
void led_set_brightness(struct led_classdev *led_cdev,
enum led_brightness brightness)
{
- int ret = 0;
-
- /* delay brightness if soft-blink is active */
+ /*
+ * In case blinking is on delay brightness setting
+ * to 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)
- schedule_work(&led_cdev->set_brightness_work);
- return;
+ led_cdev->new_brightness_value = brightness;
+
+ /* New brightness will be set on next timer tick. */
+ if (brightness != LED_OFF)
+ return;
+ /*
+ * 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.
+ */
+ led_cdev->flags |= LED_BLINK_DISABLE;
+ } else {
+ /*
+ * Don't use work queue if brightness has to be set as quickly
+ * as possible or if this is fast LED.
+ */
+ if ((led_cdev->flags & SET_BRIGHTNESS_SYNC) ||
+ (led_cdev->flags & LED_BRIGHTNESS_FAST)) {
+ __led_set_brightness(led_cdev, brightness);
+ return;
+ }
}
- if (led_cdev->flags & SET_BRIGHTNESS_ASYNC) {
- led_set_brightness_async(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);
+ led_cdev->delayed_set_value = brightness;
+ schedule_work(&led_cdev->set_brightness_work);
}
EXPORT_SYMBOL(led_set_brightness);
diff --git a/drivers/leds/leds.h b/drivers/leds/leds.h
index bc89d7a..dd9dd9b 100644
--- a/drivers/leds/leds.h
+++ b/drivers/leds/leds.h
@@ -2,8 +2,10 @@
* LED Core
*
* Copyright 2005 Openedhand Ltd.
+ * Copyright 2014, 2015 Samsung Electronics Co., Ltd.
*
* Author: Richard Purdie <[email protected]>
+ * Author: Jacek Anaszewski <[email protected]>
*
* This program is free software; you can redistribute it and/or modify
* it under the terms of the GNU General Public License version 2 as
@@ -16,27 +18,15 @@
#include <linux/rwsem.h>
#include <linux/leds.h>
-static inline void led_set_brightness_async(struct led_classdev *led_cdev,
+static inline void __led_set_brightness(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)
-{
- int ret = 0;
-
led_cdev->brightness = min(value, led_cdev->max_brightness);
- if (!(led_cdev->flags & LED_SUSPENDED))
- ret = led_cdev->brightness_set_sync(led_cdev,
- led_cdev->brightness);
- return ret;
+ if (led_cdev->flags & LED_SUSPENDED)
+ return;
+
+ led_cdev->brightness_set(led_cdev, led_cdev->brightness);
}
static inline int led_get_brightness(struct led_classdev *led_cdev)
@@ -44,6 +34,23 @@ static inline int led_get_brightness(struct led_classdev *led_cdev)
return led_cdev->brightness;
}
+static inline void led_set_brightness_nosleep(struct led_classdev *led_cdev,
+ enum led_brightness value)
+{
+ /*
+ * Delegate setting brightness to a work queue task only for slow LEDs
+ * as the FAST ones are guaranteed not to sleep while setting
+ * brightness.
+ */
+ if (!(led_cdev->flags & LED_BRIGHTNESS_FAST)) {
+ led_cdev->delayed_set_value = value;
+ schedule_work(&led_cdev->set_brightness_work);
+ return;
+ }
+
+ __led_set_brightness(led_cdev, value);
+}
+
void led_stop_software_blink(struct led_classdev *led_cdev);
extern struct rw_semaphore leds_list_lock;
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);
diff --git a/include/linux/leds.h b/include/linux/leds.h
index b122eea..ea99de9 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -48,17 +48,13 @@ struct led_classdev {
#define SET_BRIGHTNESS_ASYNC (1 << 21)
#define SET_BRIGHTNESS_SYNC (1 << 22)
#define LED_DEV_CAP_FLASH (1 << 23)
+#define LED_BRIGHTNESS_FAST (1 << 24)
+#define LED_BLINK_DISABLE (1 << 25)
/* Set LED brightness level */
/* 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);
/* Get LED brightness level */
enum led_brightness (*brightness_get)(struct led_classdev *led_cdev);
@@ -87,6 +83,7 @@ struct led_classdev {
struct work_struct set_brightness_work;
int delayed_set_value;
+ int new_brightness_value;
#ifdef CONFIG_LEDS_TRIGGERS
/* Protects the trigger data below */
--
1.7.9.5
Commit 483a3122 ("leds: Use set_brightness_work for brightness_set
ops that can sleep") removed from LED subsystem drivers the
responsibility of using work queues internally.
Modify the driver to benefit from this modification.
Signed-off-by: Jacek Anaszewski <[email protected]>
---
drivers/leds/leds-pwm.c | 24 ++++--------------------
1 file changed, 4 insertions(+), 20 deletions(-)
diff --git a/drivers/leds/leds-pwm.c b/drivers/leds/leds-pwm.c
index 1d07e3e..0068297 100644
--- a/drivers/leds/leds-pwm.c
+++ b/drivers/leds/leds-pwm.c
@@ -22,12 +22,10 @@
#include <linux/pwm.h>
#include <linux/leds_pwm.h>
#include <linux/slab.h>
-#include <linux/workqueue.h>
struct led_pwm_data {
struct led_classdev cdev;
struct pwm_device *pwm;
- struct work_struct work;
unsigned int active_low;
unsigned int period;
int duty;
@@ -51,14 +49,6 @@ static void __led_pwm_set(struct led_pwm_data *led_dat)
pwm_enable(led_dat->pwm);
}
-static void led_pwm_work(struct work_struct *work)
-{
- struct led_pwm_data *led_dat =
- container_of(work, struct led_pwm_data, work);
-
- __led_pwm_set(led_dat);
-}
-
static void led_pwm_set(struct led_classdev *led_cdev,
enum led_brightness brightness)
{
@@ -75,10 +65,7 @@ static void led_pwm_set(struct led_classdev *led_cdev,
led_dat->duty = duty;
- if (led_dat->can_sleep)
- schedule_work(&led_dat->work);
- else
- __led_pwm_set(led_dat);
+ __led_pwm_set(led_dat);
}
static inline size_t sizeof_pwm_leds_priv(int num_leds)
@@ -89,11 +76,8 @@ static inline size_t sizeof_pwm_leds_priv(int num_leds)
static void led_pwm_cleanup(struct led_pwm_priv *priv)
{
- while (priv->num_leds--) {
+ while (priv->num_leds--)
led_classdev_unregister(&priv->leds[priv->num_leds].cdev);
- if (priv->leds[priv->num_leds].can_sleep)
- cancel_work_sync(&priv->leds[priv->num_leds].work);
- }
}
static int led_pwm_add(struct device *dev, struct led_pwm_priv *priv,
@@ -122,8 +106,8 @@ static int led_pwm_add(struct device *dev, struct led_pwm_priv *priv,
}
led_data->can_sleep = pwm_can_sleep(led_data->pwm);
- if (led_data->can_sleep)
- INIT_WORK(&led_data->work, led_pwm_work);
+ if (!led_data->can_sleep)
+ led_data->cdev.flags |= LED_BRIGHTNESS_FAST;
led_data->period = pwm_get_period(led_data->pwm);
if (!led_data->period && (led->pwm_period_ns > 0))
--
1.7.9.5
After removing from LED class drivers the responsibility for
using work queues on their own in case they can sleep while
setting brightness, the brightness_set_sync op is no longer
required.
Signed-off-by: Jacek Anaszewski <[email protected]>
---
drivers/leds/led-class-flash.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/drivers/leds/led-class-flash.c b/drivers/leds/led-class-flash.c
index 3b25734..fb9bc8d 100644
--- a/drivers/leds/led-class-flash.c
+++ b/drivers/leds/led-class-flash.c
@@ -298,9 +298,6 @@ 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)
- return -EINVAL;
-
ops = fled_cdev->ops;
if (!ops || !ops->strobe_set)
return -EINVAL;
--
1.7.9.5
Commit 483a3122 ("leds: Use set_brightness_work for brightness_set
ops that can sleep") removed from LED subsystem drivers the
responsibility of using work queues internally.
Modify the driver to benefit from this modification.
Signed-off-by: Jacek Anaszewski <[email protected]>
---
drivers/leds/leds-max77693.c | 55 ++++++------------------------------------
1 file changed, 7 insertions(+), 48 deletions(-)
diff --git a/drivers/leds/leds-max77693.c b/drivers/leds/leds-max77693.c
index b8b0eec..797ab5b 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 void 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,6 @@ 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);
}
static int max77693_led_flash_brightness_set(
@@ -931,15 +895,12 @@ 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->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 +1022,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
Commit 483a3122 ("leds: Use set_brightness_work for brightness_set
ops that can sleep") removed from LED subsystem drivers the
responsibility of using work queues internally.
Modify the driver to benefit from this modification.
Signed-off-by: Jacek Anaszewski <[email protected]>
---
drivers/leds/leds-aat1290.c | 50 +++++++++++--------------------------------
1 file changed, 12 insertions(+), 38 deletions(-)
diff --git a/drivers/leds/leds-aat1290.c b/drivers/leds/leds-aat1290.c
index 1ac525b..318bcf0 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 void 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) {
@@ -160,37 +168,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;
-}
-
static int aat1290_led_flash_strobe_set(struct led_classdev_flash *fled_cdev,
bool state)
@@ -510,10 +487,8 @@ 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->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 +523,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
>
> /* Set LED brightness level */
> /* 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);
> /* Get LED brightness level */
> enum led_brightness (*brightness_get)(struct led_classdev *led_cdev);
>
NAK. Feel free to rearrange the code so that driver's work is easier,
but keep separate callbacks for "can sleep" and "can not sleep"
situations. 4 bytes are not worth the confusion.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Hi Jacek,
On Tue, Jun 30, 2015 at 03:59:26PM +0200, Jacek Anaszewski wrote:
> This patch rearranges the core LED subsystem code, so that it
> now shifts the responsibility for using work queues from drivers,
> in case their brightness_set ops can sleep, onto the LED core
> Addition of two flags: LED_BRIGHTNESS_FAST and LED_BLINK_DISABLE
> as well as new_brightness_value property to the struct led_classdev
> allows for employing existing set_brightness_work to do the job.
> The modifications allows also to get rid of brightness_set_sync op,
> as flash LED devices can now be handled properly only basing on the
> SET_BRIGHTNESS_SYNC flag.
Nice patch! Thanks!
Looks like this is the favourite topic nowadays. ;-)
The documentation should be improved to tell how the API is expected to be
have, e.g. which functions may block. I think this is out of scope for this
patch though.
I think all the existing drivers that implement the set_brightness()
callback have a fast (and non-blocking) implementation, and some of these
drivers use a work queue. In order to avoid modifying those drivers right
now, how about adding a flag for slow devices instead? "Slow" handlers
should be those that do at least one of the following: 1) sleep and 2) take
excessive amount of time to run.
How about splitting the patch as follows:
- set_brightness()/set_brightness_sync() -> set_brightness() +
LED_BRIGHTNESS_FAST + slow handlers in a work queue,
- add LED_BLINK_DISABLE flag,
- fix the heartbeat trigger (it's sleeping in a timer if LED_BRIGHTNESS_SYNC
is set).
I'd propose to drop led_set_brightness_async() and just make
led_set_brightness() asynchronous (or non-blocking if you wish) as it was
before the LED flash class patches. Considering the nature and tradition of
the framework, that's probably how most users want it to be. One can always
use led_set_brightness_sync() if needed.
The caller should indeed decide whether the operation is synchronous or not,
that's not really a property of the LED. I requested that for the V4L2
framework due to the very different use cases that are typical for the LED
class devices.
I have some patches along these lines, but I probably won't have much time
to work on them, and I can rebase mine on yours later on. If you're
interested in taking a peek they're here:
<URL:http://salottisipuli.retiisi.org.uk/cgi-bin/gitweb.cgi?p=~sailus/linux.git;a=shortlog;h=refs/heads/leds-as3645a>
As the result the as3645a driver is quite a bit smaller than the V4L2
sub-device one, which is a good sign. :-)
--
Kind regards,
Sakari Ailus
e-mail: [email protected] XMPP: [email protected]
On Wed, Jul 01, 2015 at 01:24:50AM +0300, Sakari Ailus wrote:
> The documentation should be improved to tell how the API is expected to be
> have, e.g. which functions may block. I think this is out of scope for this
> patch though.
> I think all the existing drivers that implement the set_brightness()
> callback have a fast (and non-blocking) implementation, and some of these
Yes, they need to be since triggers can happen in any context.
Hi Sakari,
On 07/01/2015 12:24 AM, Sakari Ailus wrote:
> Hi Jacek,
>
> On Tue, Jun 30, 2015 at 03:59:26PM +0200, Jacek Anaszewski wrote:
>> This patch rearranges the core LED subsystem code, so that it
>> now shifts the responsibility for using work queues from drivers,
>> in case their brightness_set ops can sleep, onto the LED core
>> Addition of two flags: LED_BRIGHTNESS_FAST and LED_BLINK_DISABLE
>> as well as new_brightness_value property to the struct led_classdev
>> allows for employing existing set_brightness_work to do the job.
>> The modifications allows also to get rid of brightness_set_sync op,
>> as flash LED devices can now be handled properly only basing on the
>> SET_BRIGHTNESS_SYNC flag.
>
> Nice patch! Thanks!
>
> Looks like this is the favourite topic nowadays. ;-)
Yeah, this allows to believe that we will manage to tackle the issue
finally :)
> The documentation should be improved to tell how the API is expected to be
> have, e.g. which functions may block. I think this is out of scope for this
> patch though.
Yes, I planned to cover this after the patch is accepted.
> I think all the existing drivers that implement the set_brightness()
> callback have a fast (and non-blocking) implementation, and some of these
> drivers use a work queue. In order to avoid modifying those drivers right
> now, how about adding a flag for slow devices instead? "Slow" handlers
> should be those that do at least one of the following: 1) sleep and 2) take
> excessive amount of time to run.
As Andrew Lunn mentioned, he was also working on this issue and he did
the vast majority of work [1] needed to remove work queues from existing
drivers. Only new flags would have to be added.
> How about splitting the patch as follows:
>
> - set_brightness()/set_brightness_sync() -> set_brightness() +
> LED_BRIGHTNESS_FAST + slow handlers in a work queue,
> - add LED_BLINK_DISABLE flag,
> - fix the heartbeat trigger (it's sleeping in a timer if LED_BRIGHTNESS_SYNC
> is set).
With my solution heartbeat trigger is not sleeping even if
LED_BRIGHTNESS_SYNC is set as all triggers use the new function -
led_set_brightness_nosleep.
> I'd propose to drop led_set_brightness_async() and just make
> led_set_brightness() asynchronous (or non-blocking if you wish) as it was
> before the LED flash class patches. Considering the nature and tradition of
> the framework, that's probably how most users want it to be. One can always
> use led_set_brightness_sync() if needed.
>
> The caller should indeed decide whether the operation is synchronous or not,
> that's not really a property of the LED. I requested that for the V4L2
> framework due to the very different use cases that are typical for the LED
> class devices.
>
> I have some patches along these lines, but I probably won't have much time
> to work on them, and I can rebase mine on yours later on. If you're
> interested in taking a peek they're here:
>
> <URL:http://salottisipuli.retiisi.org.uk/cgi-bin/gitweb.cgi?p=~sailus/linux.git;a=shortlog;h=refs/heads/leds-as3645a>
I've skimmed through the patches and I find them valuable.
Could you elaborate on the patch
"leds: Serialise access to LED framework data and drivers",
what problem does it solve?
>
> As the result the as3645a driver is quite a bit smaller than the V4L2
> sub-device one, which is a good sign. :-)
>
[1] https://github.com/lunn/linux.git 3.19-rc4-led-devel-workqueue
--
Best Regards,
Jacek Anaszewski
On 07/01/2015 11:52 AM, Jacek Anaszewski wrote:
> Hi Sakari,
>
> On 07/01/2015 12:24 AM, Sakari Ailus wrote:
>> Hi Jacek,
>>
>> On Tue, Jun 30, 2015 at 03:59:26PM +0200, Jacek Anaszewski wrote:
>>> This patch rearranges the core LED subsystem code, so that it
>>> now shifts the responsibility for using work queues from drivers,
>>> in case their brightness_set ops can sleep, onto the LED core
>>> Addition of two flags: LED_BRIGHTNESS_FAST and LED_BLINK_DISABLE
>>> as well as new_brightness_value property to the struct led_classdev
>>> allows for employing existing set_brightness_work to do the job.
>>> The modifications allows also to get rid of brightness_set_sync op,
>>> as flash LED devices can now be handled properly only basing on the
>>> SET_BRIGHTNESS_SYNC flag.
>>
>> Nice patch! Thanks!
>>
>> Looks like this is the favourite topic nowadays. ;-)
>
> Yeah, this allows to believe that we will manage to tackle the issue
> finally :)
>
>> The documentation should be improved to tell how the API is expected
>> to be
>> have, e.g. which functions may block. I think this is out of scope for
>> this
>> patch though.
>
> Yes, I planned to cover this after the patch is accepted.
>
>> I think all the existing drivers that implement the set_brightness()
>> callback have a fast (and non-blocking) implementation, and some of these
>> drivers use a work queue. In order to avoid modifying those drivers right
>> now, how about adding a flag for slow devices instead? "Slow" handlers
>> should be those that do at least one of the following: 1) sleep and 2)
>> take
>> excessive amount of time to run.
>
> As Andrew Lunn mentioned, he was also working on this issue and he did
> the vast majority of work [1] needed to remove work queues from existing
> drivers. Only new flags would have to be added.
>
>> How about splitting the patch as follows:
>>
>> - set_brightness()/set_brightness_sync() -> set_brightness() +
>> LED_BRIGHTNESS_FAST + slow handlers in a work queue,
>> - add LED_BLINK_DISABLE flag,
>> - fix the heartbeat trigger (it's sleeping in a timer if
>> LED_BRIGHTNESS_SYNC
>> is set).
>
> With my solution heartbeat trigger is not sleeping even if
> LED_BRIGHTNESS_SYNC is set as all triggers use the new function -
> led_set_brightness_nosleep.
>
>> I'd propose to drop led_set_brightness_async() and just make
>> led_set_brightness() asynchronous (or non-blocking if you wish) as it was
>> before the LED flash class patches. Considering the nature and
>> tradition of
>> the framework, that's probably how most users want it to be. One can
>> always
>> use led_set_brightness_sync() if needed.
led_set_brightness called brightness_set op in a synchronous way
before LED flash class patches. It was up to driver how it implemented
the op - blocking or non-blocking. API was not async by default then.
Adding public API led_set_brightness_sync would introduce ambiguity, as
led_set_brightness also can be synchronous.
>> The caller should indeed decide whether the operation is synchronous
>> or not,
>> that's not really a property of the LED. I requested that for the V4L2
>> framework due to the very different use cases that are typical for the
>> LED
>> class devices.
I agree that caller should decide, but we would have to have unambiguous
API for this. I am wondering if renaming led_set_brightness to
led_set_brightness_async and making it always scheduling the work queue
would be harmless solution. This would modify only kernel internal API.
We could introduce led_set_brightness_sync API then, which would call
brightness_set op in a synchronous way.
>> I have some patches along these lines, but I probably won't have much
>> time
>> to work on them, and I can rebase mine on yours later on. If you're
>> interested in taking a peek they're here:
>>
>> <URL:http://salottisipuli.retiisi.org.uk/cgi-bin/gitweb.cgi?p=~sailus/linux.git;a=shortlog;h=refs/heads/leds-as3645a>
>>
>
> I've skimmed through the patches and I find them valuable.
> Could you elaborate on the patch
> "leds: Serialise access to LED framework data and drivers",
> what problem does it solve?
>
>>
>> As the result the as3645a driver is quite a bit smaller than the V4L2
>> sub-device one, which is a good sign. :-)
>>
>
> [1] https://github.com/lunn/linux.git 3.19-rc4-led-devel-workqueue
>
--
Best Regards,
Jacek Anaszewski
Hi Jacek,
On Wed, Jul 01, 2015 at 11:52:11AM +0200, Jacek Anaszewski wrote:
> Hi Sakari,
>
> On 07/01/2015 12:24 AM, Sakari Ailus wrote:
> >Hi Jacek,
> >
> >On Tue, Jun 30, 2015 at 03:59:26PM +0200, Jacek Anaszewski wrote:
> >>This patch rearranges the core LED subsystem code, so that it
> >>now shifts the responsibility for using work queues from drivers,
> >>in case their brightness_set ops can sleep, onto the LED core
> >>Addition of two flags: LED_BRIGHTNESS_FAST and LED_BLINK_DISABLE
> >>as well as new_brightness_value property to the struct led_classdev
> >>allows for employing existing set_brightness_work to do the job.
> >>The modifications allows also to get rid of brightness_set_sync op,
> >>as flash LED devices can now be handled properly only basing on the
> >>SET_BRIGHTNESS_SYNC flag.
> >
> >Nice patch! Thanks!
> >
> >Looks like this is the favourite topic nowadays. ;-)
>
> Yeah, this allows to believe that we will manage to tackle the issue
> finally :)
>
> >The documentation should be improved to tell how the API is expected to be
> >have, e.g. which functions may block. I think this is out of scope for this
> >patch though.
>
> Yes, I planned to cover this after the patch is accepted.
Ok.
> >I think all the existing drivers that implement the set_brightness()
> >callback have a fast (and non-blocking) implementation, and some of these
> >drivers use a work queue. In order to avoid modifying those drivers right
> >now, how about adding a flag for slow devices instead? "Slow" handlers
> >should be those that do at least one of the following: 1) sleep and 2) take
> >excessive amount of time to run.
>
> As Andrew Lunn mentioned, he was also working on this issue and he did
> the vast majority of work [1] needed to remove work queues from existing
> drivers. Only new flags would have to be added.
Excellent! This is what I was thinking as well.
It think it'd be good if we could get the framework improvements in without
having to change the existing drivers. They could be fixed later on; I
wonder how testing could be arranged.
> >How about splitting the patch as follows:
> >
> >- set_brightness()/set_brightness_sync() -> set_brightness() +
> > LED_BRIGHTNESS_FAST + slow handlers in a work queue,
> >- add LED_BLINK_DISABLE flag,
> >- fix the heartbeat trigger (it's sleeping in a timer if LED_BRIGHTNESS_SYNC
> > is set).
>
> With my solution heartbeat trigger is not sleeping even if
> LED_BRIGHTNESS_SYNC is set as all triggers use the new function -
> led_set_brightness_nosleep.
Correct. I'd suggest still to put that into a separate patch as it's a
bugfix.
> >I'd propose to drop led_set_brightness_async() and just make
> >led_set_brightness() asynchronous (or non-blocking if you wish) as it was
> >before the LED flash class patches. Considering the nature and tradition of
> >the framework, that's probably how most users want it to be. One can always
> >use led_set_brightness_sync() if needed.
> >
> >The caller should indeed decide whether the operation is synchronous or not,
> >that's not really a property of the LED. I requested that for the V4L2
> >framework due to the very different use cases that are typical for the LED
> >class devices.
> >
> >I have some patches along these lines, but I probably won't have much time
> >to work on them, and I can rebase mine on yours later on. If you're
> >interested in taking a peek they're here:
> >
> ><URL:http://salottisipuli.retiisi.org.uk/cgi-bin/gitweb.cgi?p=~sailus/linux.git;a=shortlog;h=refs/heads/leds-as3645a>
>
> I've skimmed through the patches and I find them valuable.
> Could you elaborate on the patch
> "leds: Serialise access to LED framework data and drivers",
> what problem does it solve?
The LED class framework is quite simple, yet it doesn't do much for drivers.
For that reason, there are quite a few drivers repeating the same patterns
such as implementing serialisation in drivers. That patch does it in the
framework itself, while also making it possible to remove locks in the
drivers.
--
Kind regards,
Sakari Ailus
e-mail: [email protected] XMPP: [email protected]
Hi Jacek,
On Wed, Jul 01, 2015 at 02:01:54PM +0200, Jacek Anaszewski wrote:
> On 07/01/2015 11:52 AM, Jacek Anaszewski wrote:
> >Hi Sakari,
> >
> >On 07/01/2015 12:24 AM, Sakari Ailus wrote:
> >>Hi Jacek,
> >>
> >>On Tue, Jun 30, 2015 at 03:59:26PM +0200, Jacek Anaszewski wrote:
> >>>This patch rearranges the core LED subsystem code, so that it
> >>>now shifts the responsibility for using work queues from drivers,
> >>>in case their brightness_set ops can sleep, onto the LED core
> >>>Addition of two flags: LED_BRIGHTNESS_FAST and LED_BLINK_DISABLE
> >>>as well as new_brightness_value property to the struct led_classdev
> >>>allows for employing existing set_brightness_work to do the job.
> >>>The modifications allows also to get rid of brightness_set_sync op,
> >>>as flash LED devices can now be handled properly only basing on the
> >>>SET_BRIGHTNESS_SYNC flag.
> >>
> >>Nice patch! Thanks!
> >>
> >>Looks like this is the favourite topic nowadays. ;-)
> >
> >Yeah, this allows to believe that we will manage to tackle the issue
> >finally :)
> >
> >>The documentation should be improved to tell how the API is expected
> >>to be
> >>have, e.g. which functions may block. I think this is out of scope for
> >>this
> >>patch though.
> >
> >Yes, I planned to cover this after the patch is accepted.
> >
> >>I think all the existing drivers that implement the set_brightness()
> >>callback have a fast (and non-blocking) implementation, and some of these
> >>drivers use a work queue. In order to avoid modifying those drivers right
> >>now, how about adding a flag for slow devices instead? "Slow" handlers
> >>should be those that do at least one of the following: 1) sleep and 2)
> >>take
> >>excessive amount of time to run.
> >
> >As Andrew Lunn mentioned, he was also working on this issue and he did
> >the vast majority of work [1] needed to remove work queues from existing
> >drivers. Only new flags would have to be added.
> >
> >>How about splitting the patch as follows:
> >>
> >>- set_brightness()/set_brightness_sync() -> set_brightness() +
> >> LED_BRIGHTNESS_FAST + slow handlers in a work queue,
> >>- add LED_BLINK_DISABLE flag,
> >>- fix the heartbeat trigger (it's sleeping in a timer if
> >>LED_BRIGHTNESS_SYNC
> >> is set).
> >
> >With my solution heartbeat trigger is not sleeping even if
> >LED_BRIGHTNESS_SYNC is set as all triggers use the new function -
> >led_set_brightness_nosleep.
> >
> >>I'd propose to drop led_set_brightness_async() and just make
> >>led_set_brightness() asynchronous (or non-blocking if you wish) as it was
> >>before the LED flash class patches. Considering the nature and
> >>tradition of
> >>the framework, that's probably how most users want it to be. One can
> >>always
> >>use led_set_brightness_sync() if needed.
>
> led_set_brightness called brightness_set op in a synchronous way
> before LED flash class patches. It was up to driver how it implemented
> the op - blocking or non-blocking. API was not async by default then.
The framework did not implement it but the drivers did. Quite a few drivers
actually change the LED state asynchronously, while the set_brightness()
callback does not block.
>
> Adding public API led_set_brightness_sync would introduce ambiguity, as
> led_set_brightness also can be synchronous.
Well, it could be synchronous, indeed. But synchronous operation is not
guaranteed. The essence of this is that led_set_brightness() does not sleep.
Whether the LED state is changed synchronously or not is not important.
>
> >>The caller should indeed decide whether the operation is synchronous
> >>or not,
> >>that's not really a property of the LED. I requested that for the V4L2
> >>framework due to the very different use cases that are typical for the
> >>LED
> >>class devices.
>
> I agree that caller should decide, but we would have to have unambiguous
> API for this. I am wondering if renaming led_set_brightness to
> led_set_brightness_async and making it always scheduling the work queue
> would be harmless solution. This would modify only kernel internal API.
We don't want to queue work if we just want to write to a register. The work
queue should only be used for slow handlers that are better not called e.g.
from interrupt context.
> We could introduce led_set_brightness_sync API then, which would call
> brightness_set op in a synchronous way.
Considering the pre-flash LED use cases and what the V4L2 flash API
requirements are, my understanding is that we should do with two LED class
API functions for setting LED brightness:
- led_set_brightness (which will not block and is very fast, but the LED
brightness change may happen asynchronously) and
- led_set_brightness_sync (which is always synchronous).
--
Kind regards,
Sakari Ailus
e-mail: [email protected] XMPP: [email protected]
> > As Andrew Lunn mentioned, he was also working on this issue and he did
> > the vast majority of work [1] needed to remove work queues from existing
> > drivers. Only new flags would have to be added.
>
> Excellent! This is what I was thinking as well.
>
> It think it'd be good if we could get the framework improvements in without
> having to change the existing drivers. They could be fixed later on; I
> wonder how testing could be arranged.
The work for the drivers is mostly brainless donkey work. So long as a
few people review each patch to catch dumb errors, the risk should be
quite low.
So ask people to test, but don't hold back drivers which nobody
tested, would be my suggestion.
Andrew
Hi Sakari,
On 07/01/2015 11:44 PM, Sakari Ailus wrote:
> Hi Jacek,
>
> On Wed, Jul 01, 2015 at 02:01:54PM +0200, Jacek Anaszewski wrote:
>> On 07/01/2015 11:52 AM, Jacek Anaszewski wrote:
>>> Hi Sakari,
>>>
>>> On 07/01/2015 12:24 AM, Sakari Ailus wrote:
>>>> Hi Jacek,
>>>>
>>>> On Tue, Jun 30, 2015 at 03:59:26PM +0200, Jacek Anaszewski wrote:
>>>>> This patch rearranges the core LED subsystem code, so that it
>>>>> now shifts the responsibility for using work queues from drivers,
>>>>> in case their brightness_set ops can sleep, onto the LED core
>>>>> Addition of two flags: LED_BRIGHTNESS_FAST and LED_BLINK_DISABLE
>>>>> as well as new_brightness_value property to the struct led_classdev
>>>>> allows for employing existing set_brightness_work to do the job.
>>>>> The modifications allows also to get rid of brightness_set_sync op,
>>>>> as flash LED devices can now be handled properly only basing on the
>>>>> SET_BRIGHTNESS_SYNC flag.
>>>>
>>>> Nice patch! Thanks!
>>>>
>>>> Looks like this is the favourite topic nowadays. ;-)
>>>
>>> Yeah, this allows to believe that we will manage to tackle the issue
>>> finally :)
>>>
>>>> The documentation should be improved to tell how the API is expected
>>>> to be
>>>> have, e.g. which functions may block. I think this is out of scope for
>>>> this
>>>> patch though.
>>>
>>> Yes, I planned to cover this after the patch is accepted.
>>>
>>>> I think all the existing drivers that implement the set_brightness()
>>>> callback have a fast (and non-blocking) implementation, and some of these
>>>> drivers use a work queue. In order to avoid modifying those drivers right
>>>> now, how about adding a flag for slow devices instead? "Slow" handlers
>>>> should be those that do at least one of the following: 1) sleep and 2)
>>>> take
>>>> excessive amount of time to run.
>>>
>>> As Andrew Lunn mentioned, he was also working on this issue and he did
>>> the vast majority of work [1] needed to remove work queues from existing
>>> drivers. Only new flags would have to be added.
>>>
>>>> How about splitting the patch as follows:
>>>>
>>>> - set_brightness()/set_brightness_sync() -> set_brightness() +
>>>> LED_BRIGHTNESS_FAST + slow handlers in a work queue,
>>>> - add LED_BLINK_DISABLE flag,
>>>> - fix the heartbeat trigger (it's sleeping in a timer if
>>>> LED_BRIGHTNESS_SYNC
>>>> is set).
>>>
>>> With my solution heartbeat trigger is not sleeping even if
>>> LED_BRIGHTNESS_SYNC is set as all triggers use the new function -
>>> led_set_brightness_nosleep.
>>>
>>>> I'd propose to drop led_set_brightness_async() and just make
>>>> led_set_brightness() asynchronous (or non-blocking if you wish) as it was
>>>> before the LED flash class patches. Considering the nature and
>>>> tradition of
>>>> the framework, that's probably how most users want it to be. One can
>>>> always
>>>> use led_set_brightness_sync() if needed.
>>
>> led_set_brightness called brightness_set op in a synchronous way
>> before LED flash class patches. It was up to driver how it implemented
>> the op - blocking or non-blocking. API was not async by default then.
>
> The framework did not implement it but the drivers did.
Roughly half of the total number of drivers use work queues.
The rest of them set the brightness synchronously.
When I run following command:
find drivers/leds -name "*.c" | xargs grep INIT_WORK | awk '{print $1}'
| uniq | wc -l
it shows 31.
> Quite a few drivers
> actually change the LED state asynchronously, while the set_brightness()
> callback does not block.
>
>>
>> Adding public API led_set_brightness_sync would introduce ambiguity, as
>> led_set_brightness also can be synchronous.
>
> Well, it could be synchronous, indeed. But synchronous operation is not
> guaranteed. The essence of this is that led_set_brightness() does not sleep.
> Whether the LED state is changed synchronously or not is not important.
I agree. I changed the approach in the new version of the patch set.
>>
>>>> The caller should indeed decide whether the operation is synchronous
>>>> or not,
>>>> that's not really a property of the LED. I requested that for the V4L2
>>>> framework due to the very different use cases that are typical for the
>>>> LED
>>>> class devices.
>>
>> I agree that caller should decide, but we would have to have unambiguous
>> API for this. I am wondering if renaming led_set_brightness to
>> led_set_brightness_async and making it always scheduling the work queue
>> would be harmless solution. This would modify only kernel internal API.
>
> We don't want to queue work if we just want to write to a register. The work
> queue should only be used for slow handlers that are better not called e.g.
> from interrupt context.
Right. Ignore this.
>> We could introduce led_set_brightness_sync API then, which would call
>> brightness_set op in a synchronous way.
>
> Considering the pre-flash LED use cases and what the V4L2 flash API
> requirements are, my understanding is that we should do with two LED class
> API functions for setting LED brightness:
>
> - led_set_brightness (which will not block and is very fast, but the LED
> brightness change may happen asynchronously) and
>
> - led_set_brightness_sync (which is always synchronous).
>
Exactly.
--
Best Regards,
Jacek Anaszewski
Hi Sakari,
On 07/01/2015 11:36 PM, Sakari Ailus wrote:
> Hi Jacek,
>
> On Wed, Jul 01, 2015 at 11:52:11AM +0200, Jacek Anaszewski wrote:
>> Hi Sakari,
>>
>> On 07/01/2015 12:24 AM, Sakari Ailus wrote:
>>> Hi Jacek,
>>>
>>> On Tue, Jun 30, 2015 at 03:59:26PM +0200, Jacek Anaszewski wrote:
>>>> This patch rearranges the core LED subsystem code, so that it
>>>> now shifts the responsibility for using work queues from drivers,
>>>> in case their brightness_set ops can sleep, onto the LED core
>>>> Addition of two flags: LED_BRIGHTNESS_FAST and LED_BLINK_DISABLE
>>>> as well as new_brightness_value property to the struct led_classdev
>>>> allows for employing existing set_brightness_work to do the job.
>>>> The modifications allows also to get rid of brightness_set_sync op,
>>>> as flash LED devices can now be handled properly only basing on the
>>>> SET_BRIGHTNESS_SYNC flag.
>>>
>>> Nice patch! Thanks!
>>>
>>> Looks like this is the favourite topic nowadays. ;-)
>>
>> Yeah, this allows to believe that we will manage to tackle the issue
>> finally :)
>>
>>> The documentation should be improved to tell how the API is expected to be
>>> have, e.g. which functions may block. I think this is out of scope for this
>>> patch though.
>>
>> Yes, I planned to cover this after the patch is accepted.
>
> Ok.
>
>>> I think all the existing drivers that implement the set_brightness()
>>> callback have a fast (and non-blocking) implementation, and some of these
>>> drivers use a work queue. In order to avoid modifying those drivers right
>>> now, how about adding a flag for slow devices instead? "Slow" handlers
>>> should be those that do at least one of the following: 1) sleep and 2) take
>>> excessive amount of time to run.
>>
>> As Andrew Lunn mentioned, he was also working on this issue and he did
>> the vast majority of work [1] needed to remove work queues from existing
>> drivers. Only new flags would have to be added.
>
> Excellent! This is what I was thinking as well.
>
> It think it'd be good if we could get the framework improvements in without
> having to change the existing drivers.
It would be hard to achieve without loosing performance. With the third
version of the patch set drivers will still work, but LED core will
schedule work queue tasks while setting brightness, despite that
some drivers don't need that all implement them on their own.
> They could be fixed later on; I wonder how testing could be arranged.
Well, majority of people who are familiar with LED drivers is on CC
for this thread. I'd add patches removing work queues from drivers
and adding LED_BRIGHTNESS_FAST flag where applicable and ask people to
test the patches.
>>> How about splitting the patch as follows:
>>>
>>> - set_brightness()/set_brightness_sync() -> set_brightness() +
>>> LED_BRIGHTNESS_FAST + slow handlers in a work queue,
>>> - add LED_BLINK_DISABLE flag,
>>> - fix the heartbeat trigger (it's sleeping in a timer if LED_BRIGHTNESS_SYNC
>>> is set).
>>
>> With my solution heartbeat trigger is not sleeping even if
>> LED_BRIGHTNESS_SYNC is set as all triggers use the new function -
>> led_set_brightness_nosleep.
>
> Correct. I'd suggest still to put that into a separate patch as it's a
> bugfix.
Actually this is rather 'semantic' bug, which doesn't break anything
in comparison to the state from time before the led_set_brightness_async
was introduced. brightness_set ops are implemented for all drivers, so
that they are guaranteed no to sleep. Buggy is the function name which
misleadingly implies that it calls brightness_set op asynchronously,
which is not true for drivers that don't use work queues.
But of course, I will gather replacing led_set_brightness_async
in the triggers with led_set_brightness in the separate patch.
>>> I'd propose to drop led_set_brightness_async() and just make
>>> led_set_brightness() asynchronous (or non-blocking if you wish) as it was
>>> before the LED flash class patches. Considering the nature and tradition of
>>> the framework, that's probably how most users want it to be. One can always
>>> use led_set_brightness_sync() if needed.
>>>
>>> The caller should indeed decide whether the operation is synchronous or not,
>>> that's not really a property of the LED. I requested that for the V4L2
>>> framework due to the very different use cases that are typical for the LED
>>> class devices.
>>>
>>> I have some patches along these lines, but I probably won't have much time
>>> to work on them, and I can rebase mine on yours later on. If you're
>>> interested in taking a peek they're here:
>>>
>>> <URL:http://salottisipuli.retiisi.org.uk/cgi-bin/gitweb.cgi?p=~sailus/linux.git;a=shortlog;h=refs/heads/leds-as3645a>
>>
>> I've skimmed through the patches and I find them valuable.
>> Could you elaborate on the patch
>> "leds: Serialise access to LED framework data and drivers",
>> what problem does it solve?
>
> The LED class framework is quite simple, yet it doesn't do much for drivers.
> For that reason, there are quite a few drivers repeating the same patterns
> such as implementing serialisation in drivers. That patch does it in the
> framework itself, while also making it possible to remove locks in the
> drivers.
>
Thanks for the explanation. Wouldn't the gain be too small in comparison
to the increased code complexity?
--
Best Regards,
Jacek Anaszewski