* [PATCH 0/2] leds: Add control of the voltage/current regulator to the LED core @ 2019-07-08 10:35 Jean-Jacques Hiblot 2019-07-08 10:35 ` Jean-Jacques Hiblot ` (2 more replies) 0 siblings, 3 replies; 18+ messages in thread From: Jean-Jacques Hiblot @ 2019-07-08 10:35 UTC (permalink / raw) To: jacek.anaszewski, pavel, robh+dt, mark.rutland, daniel.thompson Cc: dmurphy, linux-leds, linux-kernel, devicetree, Jean-Jacques Hiblot This series makes it possible for the LED core to manage the power supply of a LED. It uses the regulator API to disable/enable the power if when the LED is turned on/off. This is especially useful in situations where the LED driver/controller is not supplying the power. Jean-Jacques Hiblot (2): leds: Add control of the voltage/current regulator to the LED core dt-bindings: leds: document new "power-supply" property .../devicetree/bindings/leds/common.txt | 5 ++ drivers/leds/led-class.c | 10 ++++ drivers/leds/led-core.c | 53 +++++++++++++++++-- include/linux/leds.h | 4 ++ 4 files changed, 69 insertions(+), 3 deletions(-) -- 2.17.1 ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 0/2] leds: Add control of the voltage/current regulator to the LED core 2019-07-08 10:35 [PATCH 0/2] leds: Add control of the voltage/current regulator to the LED core Jean-Jacques Hiblot @ 2019-07-08 10:35 ` Jean-Jacques Hiblot 2019-07-08 10:35 ` [PATCH 1/2] " Jean-Jacques Hiblot 2019-07-08 10:35 ` [PATCH 2/2] dt-bindings: leds: document new "power-supply" property Jean-Jacques Hiblot 2 siblings, 0 replies; 18+ messages in thread From: Jean-Jacques Hiblot @ 2019-07-08 10:35 UTC (permalink / raw) To: jacek.anaszewski, pavel, robh+dt, mark.rutland, daniel.thompson Cc: dmurphy, linux-leds, linux-kernel, devicetree, Jean-Jacques Hiblot This series makes it possible for the LED core to manage the power supply of a LED. It uses the regulator API to disable/enable the power if when the LED is turned on/off. This is especially useful in situations where the LED driver/controller is not supplying the power. Jean-Jacques Hiblot (2): leds: Add control of the voltage/current regulator to the LED core dt-bindings: leds: document new "power-supply" property .../devicetree/bindings/leds/common.txt | 5 ++ drivers/leds/led-class.c | 10 ++++ drivers/leds/led-core.c | 53 +++++++++++++++++-- include/linux/leds.h | 4 ++ 4 files changed, 69 insertions(+), 3 deletions(-) -- 2.17.1 ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 1/2] leds: Add control of the voltage/current regulator to the LED core 2019-07-08 10:35 [PATCH 0/2] leds: Add control of the voltage/current regulator to the LED core Jean-Jacques Hiblot 2019-07-08 10:35 ` Jean-Jacques Hiblot @ 2019-07-08 10:35 ` Jean-Jacques Hiblot 2019-07-08 10:35 ` Jean-Jacques Hiblot 2019-07-12 18:49 ` Dan Murphy 2019-07-08 10:35 ` [PATCH 2/2] dt-bindings: leds: document new "power-supply" property Jean-Jacques Hiblot 2 siblings, 2 replies; 18+ messages in thread From: Jean-Jacques Hiblot @ 2019-07-08 10:35 UTC (permalink / raw) To: jacek.anaszewski, pavel, robh+dt, mark.rutland, daniel.thompson Cc: dmurphy, linux-leds, linux-kernel, devicetree, Jean-Jacques Hiblot A LED is usually powered by a voltage/current regulator. Let the LED core about it. This allows the LED core to turn on or off the power supply as needed. Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com> --- drivers/leds/led-class.c | 10 ++++++++ drivers/leds/led-core.c | 53 +++++++++++++++++++++++++++++++++++++--- include/linux/leds.h | 4 +++ 3 files changed, 64 insertions(+), 3 deletions(-) diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c index 4793e77808e2..e01b2d982564 100644 --- a/drivers/leds/led-class.c +++ b/drivers/leds/led-class.c @@ -17,6 +17,7 @@ #include <linux/slab.h> #include <linux/spinlock.h> #include <linux/timer.h> +#include <linux/regulator/consumer.h> #include <uapi/linux/uleds.h> #include "leds.h" @@ -272,6 +273,15 @@ int of_led_classdev_register(struct device *parent, struct device_node *np, dev_warn(parent, "Led %s renamed to %s due to name collision", led_cdev->name, dev_name(led_cdev->dev)); + led_cdev->regulator = devm_regulator_get(led_cdev->dev, "power"); + if (IS_ERR(led_cdev->regulator)) { + dev_err(led_cdev->dev, "Cannot get the power supply for %s\n", + led_cdev->name); + device_unregister(led_cdev->dev); + mutex_unlock(&led_cdev->led_access); + return PTR_ERR(led_cdev->regulator); + } + if (led_cdev->flags & LED_BRIGHT_HW_CHANGED) { ret = led_add_brightness_hw_changed(led_cdev); if (ret) { diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c index 7107cd7e87cf..139de6b08cad 100644 --- a/drivers/leds/led-core.c +++ b/drivers/leds/led-core.c @@ -16,6 +16,7 @@ #include <linux/rwsem.h> #include <linux/slab.h> #include "leds.h" +#include <linux/regulator/consumer.h> DECLARE_RWSEM(leds_list_lock); EXPORT_SYMBOL_GPL(leds_list_lock); @@ -23,6 +24,31 @@ EXPORT_SYMBOL_GPL(leds_list_lock); LIST_HEAD(leds_list); EXPORT_SYMBOL_GPL(leds_list); +static bool __led_need_regulator_update(struct led_classdev *led_cdev, + int brightness) +{ + bool new_regulator_state = (brightness != LED_OFF); + + return led_cdev->regulator_state != new_regulator_state; +} + +static int __led_handle_regulator(struct led_classdev *led_cdev, + int brightness) +{ + if (__led_need_regulator_update(led_cdev, brightness)) { + int ret; + + if (brightness != LED_OFF) + ret = regulator_enable(led_cdev->regulator); + else + ret = regulator_disable(led_cdev->regulator); + if (ret) + return ret; + led_cdev->regulator_state = (brightness != LED_OFF); + } + return 0; +} + static int __led_set_brightness(struct led_classdev *led_cdev, enum led_brightness value) { @@ -80,6 +106,7 @@ static void led_timer_function(struct timer_list *t) } led_set_brightness_nosleep(led_cdev, brightness); + __led_handle_regulator(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 + @@ -115,6 +142,8 @@ static void set_brightness_delayed(struct work_struct *ws) if (ret == -ENOTSUPP) ret = __led_set_brightness_blocking(led_cdev, led_cdev->delayed_set_value); + __led_handle_regulator(led_cdev, led_cdev->delayed_set_value); + if (ret < 0 && /* LED HW might have been unplugged, therefore don't warn */ !(ret == -ENODEV && (led_cdev->flags & LED_UNREGISTERING) && @@ -141,6 +170,7 @@ static void led_set_software_blink(struct led_classdev *led_cdev, /* never on - just set to off */ if (!delay_on) { led_set_brightness_nosleep(led_cdev, LED_OFF); + __led_handle_regulator(led_cdev, LED_OFF); return; } @@ -148,6 +178,7 @@ static void led_set_software_blink(struct led_classdev *led_cdev, if (!delay_off) { led_set_brightness_nosleep(led_cdev, led_cdev->blink_brightness); + __led_handle_regulator(led_cdev, led_cdev->blink_brightness); return; } @@ -256,8 +287,14 @@ void led_set_brightness_nopm(struct led_classdev *led_cdev, enum led_brightness value) { /* Use brightness_set op if available, it is guaranteed not to sleep */ - if (!__led_set_brightness(led_cdev, value)) - return; + if (!__led_set_brightness(led_cdev, value)) { + /* + * if regulator state doesn't need to be changed, that is all/ + * Otherwise delegate the change to a work queue + */ + if (!__led_need_regulator_update(led_cdev, value)) + return; + } /* If brightness setting can sleep, delegate it to a work queue task */ led_cdev->delayed_set_value = value; @@ -280,6 +317,8 @@ EXPORT_SYMBOL_GPL(led_set_brightness_nosleep); int led_set_brightness_sync(struct led_classdev *led_cdev, enum led_brightness value) { + int ret; + if (led_cdev->blink_delay_on || led_cdev->blink_delay_off) return -EBUSY; @@ -288,7 +327,15 @@ int led_set_brightness_sync(struct led_classdev *led_cdev, if (led_cdev->flags & LED_SUSPENDED) return 0; - return __led_set_brightness_blocking(led_cdev, led_cdev->brightness); + ret = __led_set_brightness_blocking(led_cdev, led_cdev->brightness); + if (ret) + return ret; + + ret = __led_handle_regulator(led_cdev, led_cdev->brightness); + if (ret) + return ret; + + return 0; } EXPORT_SYMBOL_GPL(led_set_brightness_sync); diff --git a/include/linux/leds.h b/include/linux/leds.h index 9b2bf574a17a..bee8e3f8dddd 100644 --- a/include/linux/leds.h +++ b/include/linux/leds.h @@ -123,6 +123,10 @@ struct led_classdev { /* Ensures consistent access to the LED Flash Class device */ struct mutex led_access; + + /* regulator */ + struct regulator *regulator; + bool regulator_state; }; extern int of_led_classdev_register(struct device *parent, -- 2.17.1 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH 1/2] leds: Add control of the voltage/current regulator to the LED core 2019-07-08 10:35 ` [PATCH 1/2] " Jean-Jacques Hiblot @ 2019-07-08 10:35 ` Jean-Jacques Hiblot 2019-07-12 18:49 ` Dan Murphy 1 sibling, 0 replies; 18+ messages in thread From: Jean-Jacques Hiblot @ 2019-07-08 10:35 UTC (permalink / raw) To: jacek.anaszewski, pavel, robh+dt, mark.rutland, daniel.thompson Cc: dmurphy, linux-leds, linux-kernel, devicetree, Jean-Jacques Hiblot A LED is usually powered by a voltage/current regulator. Let the LED core about it. This allows the LED core to turn on or off the power supply as needed. Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com> --- drivers/leds/led-class.c | 10 ++++++++ drivers/leds/led-core.c | 53 +++++++++++++++++++++++++++++++++++++--- include/linux/leds.h | 4 +++ 3 files changed, 64 insertions(+), 3 deletions(-) diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c index 4793e77808e2..e01b2d982564 100644 --- a/drivers/leds/led-class.c +++ b/drivers/leds/led-class.c @@ -17,6 +17,7 @@ #include <linux/slab.h> #include <linux/spinlock.h> #include <linux/timer.h> +#include <linux/regulator/consumer.h> #include <uapi/linux/uleds.h> #include "leds.h" @@ -272,6 +273,15 @@ int of_led_classdev_register(struct device *parent, struct device_node *np, dev_warn(parent, "Led %s renamed to %s due to name collision", led_cdev->name, dev_name(led_cdev->dev)); + led_cdev->regulator = devm_regulator_get(led_cdev->dev, "power"); + if (IS_ERR(led_cdev->regulator)) { + dev_err(led_cdev->dev, "Cannot get the power supply for %s\n", + led_cdev->name); + device_unregister(led_cdev->dev); + mutex_unlock(&led_cdev->led_access); + return PTR_ERR(led_cdev->regulator); + } + if (led_cdev->flags & LED_BRIGHT_HW_CHANGED) { ret = led_add_brightness_hw_changed(led_cdev); if (ret) { diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c index 7107cd7e87cf..139de6b08cad 100644 --- a/drivers/leds/led-core.c +++ b/drivers/leds/led-core.c @@ -16,6 +16,7 @@ #include <linux/rwsem.h> #include <linux/slab.h> #include "leds.h" +#include <linux/regulator/consumer.h> DECLARE_RWSEM(leds_list_lock); EXPORT_SYMBOL_GPL(leds_list_lock); @@ -23,6 +24,31 @@ EXPORT_SYMBOL_GPL(leds_list_lock); LIST_HEAD(leds_list); EXPORT_SYMBOL_GPL(leds_list); +static bool __led_need_regulator_update(struct led_classdev *led_cdev, + int brightness) +{ + bool new_regulator_state = (brightness != LED_OFF); + + return led_cdev->regulator_state != new_regulator_state; +} + +static int __led_handle_regulator(struct led_classdev *led_cdev, + int brightness) +{ + if (__led_need_regulator_update(led_cdev, brightness)) { + int ret; + + if (brightness != LED_OFF) + ret = regulator_enable(led_cdev->regulator); + else + ret = regulator_disable(led_cdev->regulator); + if (ret) + return ret; + led_cdev->regulator_state = (brightness != LED_OFF); + } + return 0; +} + static int __led_set_brightness(struct led_classdev *led_cdev, enum led_brightness value) { @@ -80,6 +106,7 @@ static void led_timer_function(struct timer_list *t) } led_set_brightness_nosleep(led_cdev, brightness); + __led_handle_regulator(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 + @@ -115,6 +142,8 @@ static void set_brightness_delayed(struct work_struct *ws) if (ret == -ENOTSUPP) ret = __led_set_brightness_blocking(led_cdev, led_cdev->delayed_set_value); + __led_handle_regulator(led_cdev, led_cdev->delayed_set_value); + if (ret < 0 && /* LED HW might have been unplugged, therefore don't warn */ !(ret == -ENODEV && (led_cdev->flags & LED_UNREGISTERING) && @@ -141,6 +170,7 @@ static void led_set_software_blink(struct led_classdev *led_cdev, /* never on - just set to off */ if (!delay_on) { led_set_brightness_nosleep(led_cdev, LED_OFF); + __led_handle_regulator(led_cdev, LED_OFF); return; } @@ -148,6 +178,7 @@ static void led_set_software_blink(struct led_classdev *led_cdev, if (!delay_off) { led_set_brightness_nosleep(led_cdev, led_cdev->blink_brightness); + __led_handle_regulator(led_cdev, led_cdev->blink_brightness); return; } @@ -256,8 +287,14 @@ void led_set_brightness_nopm(struct led_classdev *led_cdev, enum led_brightness value) { /* Use brightness_set op if available, it is guaranteed not to sleep */ - if (!__led_set_brightness(led_cdev, value)) - return; + if (!__led_set_brightness(led_cdev, value)) { + /* + * if regulator state doesn't need to be changed, that is all/ + * Otherwise delegate the change to a work queue + */ + if (!__led_need_regulator_update(led_cdev, value)) + return; + } /* If brightness setting can sleep, delegate it to a work queue task */ led_cdev->delayed_set_value = value; @@ -280,6 +317,8 @@ EXPORT_SYMBOL_GPL(led_set_brightness_nosleep); int led_set_brightness_sync(struct led_classdev *led_cdev, enum led_brightness value) { + int ret; + if (led_cdev->blink_delay_on || led_cdev->blink_delay_off) return -EBUSY; @@ -288,7 +327,15 @@ int led_set_brightness_sync(struct led_classdev *led_cdev, if (led_cdev->flags & LED_SUSPENDED) return 0; - return __led_set_brightness_blocking(led_cdev, led_cdev->brightness); + ret = __led_set_brightness_blocking(led_cdev, led_cdev->brightness); + if (ret) + return ret; + + ret = __led_handle_regulator(led_cdev, led_cdev->brightness); + if (ret) + return ret; + + return 0; } EXPORT_SYMBOL_GPL(led_set_brightness_sync); diff --git a/include/linux/leds.h b/include/linux/leds.h index 9b2bf574a17a..bee8e3f8dddd 100644 --- a/include/linux/leds.h +++ b/include/linux/leds.h @@ -123,6 +123,10 @@ struct led_classdev { /* Ensures consistent access to the LED Flash Class device */ struct mutex led_access; + + /* regulator */ + struct regulator *regulator; + bool regulator_state; }; extern int of_led_classdev_register(struct device *parent, -- 2.17.1 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH 1/2] leds: Add control of the voltage/current regulator to the LED core 2019-07-08 10:35 ` [PATCH 1/2] " Jean-Jacques Hiblot 2019-07-08 10:35 ` Jean-Jacques Hiblot @ 2019-07-12 18:49 ` Dan Murphy 2019-07-12 18:49 ` Dan Murphy 2019-07-15 9:01 ` Jean-Jacques Hiblot 1 sibling, 2 replies; 18+ messages in thread From: Dan Murphy @ 2019-07-12 18:49 UTC (permalink / raw) To: Jean-Jacques Hiblot, jacek.anaszewski, pavel, robh+dt, mark.rutland, daniel.thompson Cc: linux-leds, linux-kernel, devicetree JJ On 7/8/19 5:35 AM, Jean-Jacques Hiblot wrote: > A LED is usually powered by a voltage/current regulator. Let the LED core Let the LED core know > about it. This allows the LED core to turn on or off the power supply > as needed. > > Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com> > --- > drivers/leds/led-class.c | 10 ++++++++ > drivers/leds/led-core.c | 53 +++++++++++++++++++++++++++++++++++++--- > include/linux/leds.h | 4 +++ > 3 files changed, 64 insertions(+), 3 deletions(-) > > diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c > index 4793e77808e2..e01b2d982564 100644 > --- a/drivers/leds/led-class.c > +++ b/drivers/leds/led-class.c > @@ -17,6 +17,7 @@ > #include <linux/slab.h> > #include <linux/spinlock.h> > #include <linux/timer.h> > +#include <linux/regulator/consumer.h> What if you move this to leds.h so core and class can both include it. > #include <uapi/linux/uleds.h> > #include "leds.h" > > @@ -272,6 +273,15 @@ int of_led_classdev_register(struct device *parent, struct device_node *np, > dev_warn(parent, "Led %s renamed to %s due to name collision", > led_cdev->name, dev_name(led_cdev->dev)); > > + led_cdev->regulator = devm_regulator_get(led_cdev->dev, "power"); Is the regulator always going to be called power? > + if (IS_ERR(led_cdev->regulator)) { > + dev_err(led_cdev->dev, "Cannot get the power supply for %s\n", > + led_cdev->name); > + device_unregister(led_cdev->dev); > + mutex_unlock(&led_cdev->led_access); > + return PTR_ERR(led_cdev->regulator); This is listed as optional in the DT doc. This appears to be required. I prefer to keep it optional. Many LED drivers are connected to fixed non-managed supplies. > + } > + > if (led_cdev->flags & LED_BRIGHT_HW_CHANGED) { > ret = led_add_brightness_hw_changed(led_cdev); > if (ret) { > diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c > index 7107cd7e87cf..139de6b08cad 100644 > --- a/drivers/leds/led-core.c > +++ b/drivers/leds/led-core.c > @@ -16,6 +16,7 @@ > #include <linux/rwsem.h> > #include <linux/slab.h> > #include "leds.h" > +#include <linux/regulator/consumer.h> > > DECLARE_RWSEM(leds_list_lock); > EXPORT_SYMBOL_GPL(leds_list_lock); > @@ -23,6 +24,31 @@ EXPORT_SYMBOL_GPL(leds_list_lock); > LIST_HEAD(leds_list); > EXPORT_SYMBOL_GPL(leds_list); > > +static bool __led_need_regulator_update(struct led_classdev *led_cdev, > + int brightness) > +{ > + bool new_regulator_state = (brightness != LED_OFF); > + > + return led_cdev->regulator_state != new_regulator_state; > +} > + > +static int __led_handle_regulator(struct led_classdev *led_cdev, > + int brightness) > +{ > + if (__led_need_regulator_update(led_cdev, brightness)) { > + int ret; Prefer to this to be moved up. > + > + if (brightness != LED_OFF) > + ret = regulator_enable(led_cdev->regulator); > + else > + ret = regulator_disable(led_cdev->regulator); > + if (ret) > + return ret; new line > + led_cdev->regulator_state = (brightness != LED_OFF); > + } > + return 0; > +} > + > static int __led_set_brightness(struct led_classdev *led_cdev, > enum led_brightness value) > { > @@ -80,6 +106,7 @@ static void led_timer_function(struct timer_list *t) > } > > led_set_brightness_nosleep(led_cdev, brightness); > + __led_handle_regulator(led_cdev, brightness); Again this seems to indicate that the regulator is a required property for the LEDs This needs to be made optional. And the same comment through out for every call. > > /* 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 + > @@ -115,6 +142,8 @@ static void set_brightness_delayed(struct work_struct *ws) > if (ret == -ENOTSUPP) > ret = __led_set_brightness_blocking(led_cdev, > led_cdev->delayed_set_value); > + __led_handle_regulator(led_cdev, led_cdev->delayed_set_value); > + > if (ret < 0 && > /* LED HW might have been unplugged, therefore don't warn */ > !(ret == -ENODEV && (led_cdev->flags & LED_UNREGISTERING) && > @@ -141,6 +170,7 @@ static void led_set_software_blink(struct led_classdev *led_cdev, > /* never on - just set to off */ > if (!delay_on) { > led_set_brightness_nosleep(led_cdev, LED_OFF); > + __led_handle_regulator(led_cdev, LED_OFF); > return; > } > > @@ -148,6 +178,7 @@ static void led_set_software_blink(struct led_classdev *led_cdev, > if (!delay_off) { > led_set_brightness_nosleep(led_cdev, > led_cdev->blink_brightness); > + __led_handle_regulator(led_cdev, led_cdev->blink_brightness); > return; > } > > @@ -256,8 +287,14 @@ void led_set_brightness_nopm(struct led_classdev *led_cdev, > enum led_brightness value) > { > /* Use brightness_set op if available, it is guaranteed not to sleep */ > - if (!__led_set_brightness(led_cdev, value)) > - return; > + if (!__led_set_brightness(led_cdev, value)) { > + /* > + * if regulator state doesn't need to be changed, that is all/ > + * Otherwise delegate the change to a work queue > + */ > + if (!__led_need_regulator_update(led_cdev, value)) > + return; > + } > > /* If brightness setting can sleep, delegate it to a work queue task */ > led_cdev->delayed_set_value = value; > @@ -280,6 +317,8 @@ EXPORT_SYMBOL_GPL(led_set_brightness_nosleep); > int led_set_brightness_sync(struct led_classdev *led_cdev, > enum led_brightness value) > { > + int ret; > + > if (led_cdev->blink_delay_on || led_cdev->blink_delay_off) > return -EBUSY; > > @@ -288,7 +327,15 @@ int led_set_brightness_sync(struct led_classdev *led_cdev, > if (led_cdev->flags & LED_SUSPENDED) > return 0; > > - return __led_set_brightness_blocking(led_cdev, led_cdev->brightness); > + ret = __led_set_brightness_blocking(led_cdev, led_cdev->brightness); > + if (ret) > + return ret; > + > + ret = __led_handle_regulator(led_cdev, led_cdev->brightness); Can't you just return here? Dan > + if (ret) > + return ret; > + > + return 0; > } > EXPORT_SYMBOL_GPL(led_set_brightness_sync); > > diff --git a/include/linux/leds.h b/include/linux/leds.h > index 9b2bf574a17a..bee8e3f8dddd 100644 > --- a/include/linux/leds.h > +++ b/include/linux/leds.h > @@ -123,6 +123,10 @@ struct led_classdev { > > /* Ensures consistent access to the LED Flash Class device */ > struct mutex led_access; > + > + /* regulator */ > + struct regulator *regulator; > + bool regulator_state; > }; > > extern int of_led_classdev_register(struct device *parent, ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/2] leds: Add control of the voltage/current regulator to the LED core 2019-07-12 18:49 ` Dan Murphy @ 2019-07-12 18:49 ` Dan Murphy 2019-07-15 9:01 ` Jean-Jacques Hiblot 1 sibling, 0 replies; 18+ messages in thread From: Dan Murphy @ 2019-07-12 18:49 UTC (permalink / raw) To: Jean-Jacques Hiblot, jacek.anaszewski, pavel, robh+dt, mark.rutland, daniel.thompson Cc: linux-leds, linux-kernel, devicetree JJ On 7/8/19 5:35 AM, Jean-Jacques Hiblot wrote: > A LED is usually powered by a voltage/current regulator. Let the LED core Let the LED core know > about it. This allows the LED core to turn on or off the power supply > as needed. > > Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com> > --- > drivers/leds/led-class.c | 10 ++++++++ > drivers/leds/led-core.c | 53 +++++++++++++++++++++++++++++++++++++--- > include/linux/leds.h | 4 +++ > 3 files changed, 64 insertions(+), 3 deletions(-) > > diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c > index 4793e77808e2..e01b2d982564 100644 > --- a/drivers/leds/led-class.c > +++ b/drivers/leds/led-class.c > @@ -17,6 +17,7 @@ > #include <linux/slab.h> > #include <linux/spinlock.h> > #include <linux/timer.h> > +#include <linux/regulator/consumer.h> What if you move this to leds.h so core and class can both include it. > #include <uapi/linux/uleds.h> > #include "leds.h" > > @@ -272,6 +273,15 @@ int of_led_classdev_register(struct device *parent, struct device_node *np, > dev_warn(parent, "Led %s renamed to %s due to name collision", > led_cdev->name, dev_name(led_cdev->dev)); > > + led_cdev->regulator = devm_regulator_get(led_cdev->dev, "power"); Is the regulator always going to be called power? > + if (IS_ERR(led_cdev->regulator)) { > + dev_err(led_cdev->dev, "Cannot get the power supply for %s\n", > + led_cdev->name); > + device_unregister(led_cdev->dev); > + mutex_unlock(&led_cdev->led_access); > + return PTR_ERR(led_cdev->regulator); This is listed as optional in the DT doc. This appears to be required. I prefer to keep it optional. Many LED drivers are connected to fixed non-managed supplies. > + } > + > if (led_cdev->flags & LED_BRIGHT_HW_CHANGED) { > ret = led_add_brightness_hw_changed(led_cdev); > if (ret) { > diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c > index 7107cd7e87cf..139de6b08cad 100644 > --- a/drivers/leds/led-core.c > +++ b/drivers/leds/led-core.c > @@ -16,6 +16,7 @@ > #include <linux/rwsem.h> > #include <linux/slab.h> > #include "leds.h" > +#include <linux/regulator/consumer.h> > > DECLARE_RWSEM(leds_list_lock); > EXPORT_SYMBOL_GPL(leds_list_lock); > @@ -23,6 +24,31 @@ EXPORT_SYMBOL_GPL(leds_list_lock); > LIST_HEAD(leds_list); > EXPORT_SYMBOL_GPL(leds_list); > > +static bool __led_need_regulator_update(struct led_classdev *led_cdev, > + int brightness) > +{ > + bool new_regulator_state = (brightness != LED_OFF); > + > + return led_cdev->regulator_state != new_regulator_state; > +} > + > +static int __led_handle_regulator(struct led_classdev *led_cdev, > + int brightness) > +{ > + if (__led_need_regulator_update(led_cdev, brightness)) { > + int ret; Prefer to this to be moved up. > + > + if (brightness != LED_OFF) > + ret = regulator_enable(led_cdev->regulator); > + else > + ret = regulator_disable(led_cdev->regulator); > + if (ret) > + return ret; new line > + led_cdev->regulator_state = (brightness != LED_OFF); > + } > + return 0; > +} > + > static int __led_set_brightness(struct led_classdev *led_cdev, > enum led_brightness value) > { > @@ -80,6 +106,7 @@ static void led_timer_function(struct timer_list *t) > } > > led_set_brightness_nosleep(led_cdev, brightness); > + __led_handle_regulator(led_cdev, brightness); Again this seems to indicate that the regulator is a required property for the LEDs This needs to be made optional. And the same comment through out for every call. > > /* 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 + > @@ -115,6 +142,8 @@ static void set_brightness_delayed(struct work_struct *ws) > if (ret == -ENOTSUPP) > ret = __led_set_brightness_blocking(led_cdev, > led_cdev->delayed_set_value); > + __led_handle_regulator(led_cdev, led_cdev->delayed_set_value); > + > if (ret < 0 && > /* LED HW might have been unplugged, therefore don't warn */ > !(ret == -ENODEV && (led_cdev->flags & LED_UNREGISTERING) && > @@ -141,6 +170,7 @@ static void led_set_software_blink(struct led_classdev *led_cdev, > /* never on - just set to off */ > if (!delay_on) { > led_set_brightness_nosleep(led_cdev, LED_OFF); > + __led_handle_regulator(led_cdev, LED_OFF); > return; > } > > @@ -148,6 +178,7 @@ static void led_set_software_blink(struct led_classdev *led_cdev, > if (!delay_off) { > led_set_brightness_nosleep(led_cdev, > led_cdev->blink_brightness); > + __led_handle_regulator(led_cdev, led_cdev->blink_brightness); > return; > } > > @@ -256,8 +287,14 @@ void led_set_brightness_nopm(struct led_classdev *led_cdev, > enum led_brightness value) > { > /* Use brightness_set op if available, it is guaranteed not to sleep */ > - if (!__led_set_brightness(led_cdev, value)) > - return; > + if (!__led_set_brightness(led_cdev, value)) { > + /* > + * if regulator state doesn't need to be changed, that is all/ > + * Otherwise delegate the change to a work queue > + */ > + if (!__led_need_regulator_update(led_cdev, value)) > + return; > + } > > /* If brightness setting can sleep, delegate it to a work queue task */ > led_cdev->delayed_set_value = value; > @@ -280,6 +317,8 @@ EXPORT_SYMBOL_GPL(led_set_brightness_nosleep); > int led_set_brightness_sync(struct led_classdev *led_cdev, > enum led_brightness value) > { > + int ret; > + > if (led_cdev->blink_delay_on || led_cdev->blink_delay_off) > return -EBUSY; > > @@ -288,7 +327,15 @@ int led_set_brightness_sync(struct led_classdev *led_cdev, > if (led_cdev->flags & LED_SUSPENDED) > return 0; > > - return __led_set_brightness_blocking(led_cdev, led_cdev->brightness); > + ret = __led_set_brightness_blocking(led_cdev, led_cdev->brightness); > + if (ret) > + return ret; > + > + ret = __led_handle_regulator(led_cdev, led_cdev->brightness); Can't you just return here? Dan > + if (ret) > + return ret; > + > + return 0; > } > EXPORT_SYMBOL_GPL(led_set_brightness_sync); > > diff --git a/include/linux/leds.h b/include/linux/leds.h > index 9b2bf574a17a..bee8e3f8dddd 100644 > --- a/include/linux/leds.h > +++ b/include/linux/leds.h > @@ -123,6 +123,10 @@ struct led_classdev { > > /* Ensures consistent access to the LED Flash Class device */ > struct mutex led_access; > + > + /* regulator */ > + struct regulator *regulator; > + bool regulator_state; > }; > > extern int of_led_classdev_register(struct device *parent, ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/2] leds: Add control of the voltage/current regulator to the LED core 2019-07-12 18:49 ` Dan Murphy 2019-07-12 18:49 ` Dan Murphy @ 2019-07-15 9:01 ` Jean-Jacques Hiblot 2019-07-15 9:24 ` Daniel Thompson 1 sibling, 1 reply; 18+ messages in thread From: Jean-Jacques Hiblot @ 2019-07-15 9:01 UTC (permalink / raw) To: Dan Murphy, jacek.anaszewski, pavel, robh+dt, mark.rutland, daniel.thompson Cc: linux-leds, linux-kernel, devicetree Hi Dan, On 12/07/2019 20:49, Dan Murphy wrote: > JJ > > On 7/8/19 5:35 AM, Jean-Jacques Hiblot wrote: >> A LED is usually powered by a voltage/current regulator. Let the LED >> core > Let the LED core know >> about it. This allows the LED core to turn on or off the power supply >> as needed. > >> >> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com> >> --- >> drivers/leds/led-class.c | 10 ++++++++ >> drivers/leds/led-core.c | 53 +++++++++++++++++++++++++++++++++++++--- >> include/linux/leds.h | 4 +++ >> 3 files changed, 64 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c >> index 4793e77808e2..e01b2d982564 100644 >> --- a/drivers/leds/led-class.c >> +++ b/drivers/leds/led-class.c >> @@ -17,6 +17,7 @@ >> #include <linux/slab.h> >> #include <linux/spinlock.h> >> #include <linux/timer.h> >> +#include <linux/regulator/consumer.h> > > What if you move this to leds.h so core and class can both include it. > > >> #include <uapi/linux/uleds.h> >> #include "leds.h" >> @@ -272,6 +273,15 @@ int of_led_classdev_register(struct device >> *parent, struct device_node *np, >> dev_warn(parent, "Led %s renamed to %s due to name collision", >> led_cdev->name, dev_name(led_cdev->dev)); >> + led_cdev->regulator = devm_regulator_get(led_cdev->dev, "power"); > > Is the regulator always going to be called power? Actually in the dts, that will be "power-supply". I lacked the imagination to come up with a better name. > >> + if (IS_ERR(led_cdev->regulator)) { >> + dev_err(led_cdev->dev, "Cannot get the power supply for %s\n", >> + led_cdev->name); >> + device_unregister(led_cdev->dev); >> + mutex_unlock(&led_cdev->led_access); >> + return PTR_ERR(led_cdev->regulator); > > This is listed as optional in the DT doc. This appears to be required. The regulator core will provide a dummy regulator if none is given in the device tree. I would rather have an error in that case, but that is not how it works. > > I prefer to keep it optional. Many LED drivers are connected to fixed > non-managed supplies. > >> + } >> + >> if (led_cdev->flags & LED_BRIGHT_HW_CHANGED) { >> ret = led_add_brightness_hw_changed(led_cdev); >> if (ret) { >> diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c >> index 7107cd7e87cf..139de6b08cad 100644 >> --- a/drivers/leds/led-core.c >> +++ b/drivers/leds/led-core.c >> @@ -16,6 +16,7 @@ >> #include <linux/rwsem.h> >> #include <linux/slab.h> >> #include "leds.h" >> +#include <linux/regulator/consumer.h> >> DECLARE_RWSEM(leds_list_lock); >> EXPORT_SYMBOL_GPL(leds_list_lock); >> @@ -23,6 +24,31 @@ EXPORT_SYMBOL_GPL(leds_list_lock); >> LIST_HEAD(leds_list); >> EXPORT_SYMBOL_GPL(leds_list); >> +static bool __led_need_regulator_update(struct led_classdev >> *led_cdev, >> + int brightness) >> +{ >> + bool new_regulator_state = (brightness != LED_OFF); >> + >> + return led_cdev->regulator_state != new_regulator_state; >> +} >> + >> +static int __led_handle_regulator(struct led_classdev *led_cdev, >> + int brightness) >> +{ >> + if (__led_need_regulator_update(led_cdev, brightness)) { >> + int ret; > > Prefer to this to be moved up. ok > >> + >> + if (brightness != LED_OFF) >> + ret = regulator_enable(led_cdev->regulator); >> + else >> + ret = regulator_disable(led_cdev->regulator); >> + if (ret) >> + return ret; > new line >> + led_cdev->regulator_state = (brightness != LED_OFF); >> + } >> + return 0; >> +} >> + >> static int __led_set_brightness(struct led_classdev *led_cdev, >> enum led_brightness value) >> { >> @@ -80,6 +106,7 @@ static void led_timer_function(struct timer_list *t) >> } >> led_set_brightness_nosleep(led_cdev, brightness); >> + __led_handle_regulator(led_cdev, brightness); > > Again this seems to indicate that the regulator is a required property > for the LEDs > > This needs to be made optional. And the same comment through out for > every call. > > >> /* 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 + >> @@ -115,6 +142,8 @@ static void set_brightness_delayed(struct >> work_struct *ws) >> if (ret == -ENOTSUPP) >> ret = __led_set_brightness_blocking(led_cdev, >> led_cdev->delayed_set_value); >> + __led_handle_regulator(led_cdev, led_cdev->delayed_set_value); >> + >> if (ret < 0 && >> /* LED HW might have been unplugged, therefore don't warn */ >> !(ret == -ENODEV && (led_cdev->flags & LED_UNREGISTERING) && >> @@ -141,6 +170,7 @@ static void led_set_software_blink(struct >> led_classdev *led_cdev, >> /* never on - just set to off */ >> if (!delay_on) { >> led_set_brightness_nosleep(led_cdev, LED_OFF); >> + __led_handle_regulator(led_cdev, LED_OFF); >> return; >> } >> @@ -148,6 +178,7 @@ static void led_set_software_blink(struct >> led_classdev *led_cdev, >> if (!delay_off) { >> led_set_brightness_nosleep(led_cdev, >> led_cdev->blink_brightness); >> + __led_handle_regulator(led_cdev, led_cdev->blink_brightness); >> return; >> } >> @@ -256,8 +287,14 @@ void led_set_brightness_nopm(struct >> led_classdev *led_cdev, >> enum led_brightness value) >> { >> /* Use brightness_set op if available, it is guaranteed not to >> sleep */ >> - if (!__led_set_brightness(led_cdev, value)) >> - return; >> + if (!__led_set_brightness(led_cdev, value)) { >> + /* >> + * if regulator state doesn't need to be changed, that is all/ >> + * Otherwise delegate the change to a work queue >> + */ >> + if (!__led_need_regulator_update(led_cdev, value)) >> + return; >> + } >> /* If brightness setting can sleep, delegate it to a work >> queue task */ >> led_cdev->delayed_set_value = value; >> @@ -280,6 +317,8 @@ EXPORT_SYMBOL_GPL(led_set_brightness_nosleep); >> int led_set_brightness_sync(struct led_classdev *led_cdev, >> enum led_brightness value) >> { >> + int ret; >> + >> if (led_cdev->blink_delay_on || led_cdev->blink_delay_off) >> return -EBUSY; >> @@ -288,7 +327,15 @@ int led_set_brightness_sync(struct >> led_classdev *led_cdev, >> if (led_cdev->flags & LED_SUSPENDED) >> return 0; >> - return __led_set_brightness_blocking(led_cdev, >> led_cdev->brightness); >> + ret = __led_set_brightness_blocking(led_cdev, >> led_cdev->brightness); >> + if (ret) >> + return ret; >> + >> + ret = __led_handle_regulator(led_cdev, led_cdev->brightness); > > Can't you just return here? ok thanks for the review JJ > > Dan > >> + if (ret) >> + return ret; >> + >> + return 0; >> } >> EXPORT_SYMBOL_GPL(led_set_brightness_sync); >> diff --git a/include/linux/leds.h b/include/linux/leds.h >> index 9b2bf574a17a..bee8e3f8dddd 100644 >> --- a/include/linux/leds.h >> +++ b/include/linux/leds.h >> @@ -123,6 +123,10 @@ struct led_classdev { >> /* Ensures consistent access to the LED Flash Class device */ >> struct mutex led_access; >> + >> + /* regulator */ >> + struct regulator *regulator; >> + bool regulator_state; >> }; >> extern int of_led_classdev_register(struct device *parent, ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/2] leds: Add control of the voltage/current regulator to the LED core 2019-07-15 9:01 ` Jean-Jacques Hiblot @ 2019-07-15 9:24 ` Daniel Thompson 2019-07-15 9:47 ` Jean-Jacques Hiblot 0 siblings, 1 reply; 18+ messages in thread From: Daniel Thompson @ 2019-07-15 9:24 UTC (permalink / raw) To: Jean-Jacques Hiblot Cc: Dan Murphy, jacek.anaszewski, pavel, robh+dt, mark.rutland, linux-leds, linux-kernel, devicetree On Mon, Jul 15, 2019 at 11:01:29AM +0200, Jean-Jacques Hiblot wrote: > Hi Dan, > > On 12/07/2019 20:49, Dan Murphy wrote: > > JJ > > > > On 7/8/19 5:35 AM, Jean-Jacques Hiblot wrote: > > > A LED is usually powered by a voltage/current regulator. Let the LED > > > core > > Let the LED core know > > > about it. This allows the LED core to turn on or off the power supply > > > as needed. > > > > > > > > Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com> > > > --- > > > drivers/leds/led-class.c | 10 ++++++++ > > > drivers/leds/led-core.c | 53 +++++++++++++++++++++++++++++++++++++--- > > > include/linux/leds.h | 4 +++ > > > 3 files changed, 64 insertions(+), 3 deletions(-) > > > > > > diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c > > > index 4793e77808e2..e01b2d982564 100644 > > > --- a/drivers/leds/led-class.c > > > +++ b/drivers/leds/led-class.c > > > @@ -17,6 +17,7 @@ > > > #include <linux/slab.h> > > > #include <linux/spinlock.h> > > > #include <linux/timer.h> > > > +#include <linux/regulator/consumer.h> > > > > What if you move this to leds.h so core and class can both include it. > > > > > > > #include <uapi/linux/uleds.h> > > > #include "leds.h" > > > @@ -272,6 +273,15 @@ int of_led_classdev_register(struct device > > > *parent, struct device_node *np, > > > dev_warn(parent, "Led %s renamed to %s due to name collision", > > > led_cdev->name, dev_name(led_cdev->dev)); > > > + led_cdev->regulator = devm_regulator_get(led_cdev->dev, "power"); > > > > Is the regulator always going to be called power? > > Actually in the dts, that will be "power-supply". I lacked the imagination > to come up with a better name. > > > > > > > > + if (IS_ERR(led_cdev->regulator)) { > > > + dev_err(led_cdev->dev, "Cannot get the power supply for %s\n", > > > + led_cdev->name); > > > + device_unregister(led_cdev->dev); > > > + mutex_unlock(&led_cdev->led_access); > > > + return PTR_ERR(led_cdev->regulator); > > > > This is listed as optional in the DT doc. This appears to be required. > > The regulator core will provide a dummy regulator if none is given in the > device tree. I would rather have an error in that case, but that is not how > it works. If you actively wanted to get -ENODEV back when there is no regulator then you can use devm_regulator_get_optional() for that. However perhaps be careful what you wish for. If you use get_optional() then you will have to sprinkle NULL or IS_ERR() checks everywhere. I'd favour using the current approach! Daniel. > > > > > > I prefer to keep it optional. Many LED drivers are connected to fixed > > non-managed supplies. > > > > > + } > > > + > > > if (led_cdev->flags & LED_BRIGHT_HW_CHANGED) { > > > ret = led_add_brightness_hw_changed(led_cdev); > > > if (ret) { > > > diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c > > > index 7107cd7e87cf..139de6b08cad 100644 > > > --- a/drivers/leds/led-core.c > > > +++ b/drivers/leds/led-core.c > > > @@ -16,6 +16,7 @@ > > > #include <linux/rwsem.h> > > > #include <linux/slab.h> > > > #include "leds.h" > > > +#include <linux/regulator/consumer.h> > > > DECLARE_RWSEM(leds_list_lock); > > > EXPORT_SYMBOL_GPL(leds_list_lock); > > > @@ -23,6 +24,31 @@ EXPORT_SYMBOL_GPL(leds_list_lock); > > > LIST_HEAD(leds_list); > > > EXPORT_SYMBOL_GPL(leds_list); > > > +static bool __led_need_regulator_update(struct led_classdev > > > *led_cdev, > > > + int brightness) > > > +{ > > > + bool new_regulator_state = (brightness != LED_OFF); > > > + > > > + return led_cdev->regulator_state != new_regulator_state; > > > +} > > > + > > > +static int __led_handle_regulator(struct led_classdev *led_cdev, > > > + int brightness) > > > +{ > > > + if (__led_need_regulator_update(led_cdev, brightness)) { > > > + int ret; > > > > Prefer to this to be moved up. > ok > > > > > + > > > + if (brightness != LED_OFF) > > > + ret = regulator_enable(led_cdev->regulator); > > > + else > > > + ret = regulator_disable(led_cdev->regulator); > > > + if (ret) > > > + return ret; > > new line > > > + led_cdev->regulator_state = (brightness != LED_OFF); > > > + } > > > + return 0; > > > +} > > > + > > > static int __led_set_brightness(struct led_classdev *led_cdev, > > > enum led_brightness value) > > > { > > > @@ -80,6 +106,7 @@ static void led_timer_function(struct timer_list *t) > > > } > > > led_set_brightness_nosleep(led_cdev, brightness); > > > + __led_handle_regulator(led_cdev, brightness); > > > > Again this seems to indicate that the regulator is a required property > > for the LEDs > > > > This needs to be made optional. And the same comment through out for > > every call. > > > > > > > /* 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 + > > > @@ -115,6 +142,8 @@ static void set_brightness_delayed(struct > > > work_struct *ws) > > > if (ret == -ENOTSUPP) > > > ret = __led_set_brightness_blocking(led_cdev, > > > led_cdev->delayed_set_value); > > > + __led_handle_regulator(led_cdev, led_cdev->delayed_set_value); > > > + > > > if (ret < 0 && > > > /* LED HW might have been unplugged, therefore don't warn */ > > > !(ret == -ENODEV && (led_cdev->flags & LED_UNREGISTERING) && > > > @@ -141,6 +170,7 @@ static void led_set_software_blink(struct > > > led_classdev *led_cdev, > > > /* never on - just set to off */ > > > if (!delay_on) { > > > led_set_brightness_nosleep(led_cdev, LED_OFF); > > > + __led_handle_regulator(led_cdev, LED_OFF); > > > return; > > > } > > > @@ -148,6 +178,7 @@ static void led_set_software_blink(struct > > > led_classdev *led_cdev, > > > if (!delay_off) { > > > led_set_brightness_nosleep(led_cdev, > > > led_cdev->blink_brightness); > > > + __led_handle_regulator(led_cdev, led_cdev->blink_brightness); > > > return; > > > } > > > @@ -256,8 +287,14 @@ void led_set_brightness_nopm(struct > > > led_classdev *led_cdev, > > > enum led_brightness value) > > > { > > > /* Use brightness_set op if available, it is guaranteed not to > > > sleep */ > > > - if (!__led_set_brightness(led_cdev, value)) > > > - return; > > > + if (!__led_set_brightness(led_cdev, value)) { > > > + /* > > > + * if regulator state doesn't need to be changed, that is all/ > > > + * Otherwise delegate the change to a work queue > > > + */ > > > + if (!__led_need_regulator_update(led_cdev, value)) > > > + return; > > > + } > > > /* If brightness setting can sleep, delegate it to a work > > > queue task */ > > > led_cdev->delayed_set_value = value; > > > @@ -280,6 +317,8 @@ EXPORT_SYMBOL_GPL(led_set_brightness_nosleep); > > > int led_set_brightness_sync(struct led_classdev *led_cdev, > > > enum led_brightness value) > > > { > > > + int ret; > > > + > > > if (led_cdev->blink_delay_on || led_cdev->blink_delay_off) > > > return -EBUSY; > > > @@ -288,7 +327,15 @@ int led_set_brightness_sync(struct > > > led_classdev *led_cdev, > > > if (led_cdev->flags & LED_SUSPENDED) > > > return 0; > > > - return __led_set_brightness_blocking(led_cdev, > > > led_cdev->brightness); > > > + ret = __led_set_brightness_blocking(led_cdev, > > > led_cdev->brightness); > > > + if (ret) > > > + return ret; > > > + > > > + ret = __led_handle_regulator(led_cdev, led_cdev->brightness); > > > > Can't you just return here? > > ok > > > thanks for the review > > JJ > > > > > Dan > > > > > + if (ret) > > > + return ret; > > > + > > > + return 0; > > > } > > > EXPORT_SYMBOL_GPL(led_set_brightness_sync); > > > diff --git a/include/linux/leds.h b/include/linux/leds.h > > > index 9b2bf574a17a..bee8e3f8dddd 100644 > > > --- a/include/linux/leds.h > > > +++ b/include/linux/leds.h > > > @@ -123,6 +123,10 @@ struct led_classdev { > > > /* Ensures consistent access to the LED Flash Class device */ > > > struct mutex led_access; > > > + > > > + /* regulator */ > > > + struct regulator *regulator; > > > + bool regulator_state; > > > }; > > > extern int of_led_classdev_register(struct device *parent, ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/2] leds: Add control of the voltage/current regulator to the LED core 2019-07-15 9:24 ` Daniel Thompson @ 2019-07-15 9:47 ` Jean-Jacques Hiblot 2019-07-15 15:19 ` Daniel Thompson 0 siblings, 1 reply; 18+ messages in thread From: Jean-Jacques Hiblot @ 2019-07-15 9:47 UTC (permalink / raw) To: Daniel Thompson Cc: Dan Murphy, jacek.anaszewski, pavel, robh+dt, mark.rutland, linux-leds, linux-kernel, devicetree On 15/07/2019 11:24, Daniel Thompson wrote: > On Mon, Jul 15, 2019 at 11:01:29AM +0200, Jean-Jacques Hiblot wrote: >> Hi Dan, >> >> On 12/07/2019 20:49, Dan Murphy wrote: >>> JJ >>> >>> On 7/8/19 5:35 AM, Jean-Jacques Hiblot wrote: >>>> A LED is usually powered by a voltage/current regulator. Let the LED >>>> core >>> Let the LED core know >>>> about it. This allows the LED core to turn on or off the power supply >>>> as needed. >>>> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com> >>>> --- >>>> drivers/leds/led-class.c | 10 ++++++++ >>>> drivers/leds/led-core.c | 53 +++++++++++++++++++++++++++++++++++++--- >>>> include/linux/leds.h | 4 +++ >>>> 3 files changed, 64 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c >>>> index 4793e77808e2..e01b2d982564 100644 >>>> --- a/drivers/leds/led-class.c >>>> +++ b/drivers/leds/led-class.c >>>> @@ -17,6 +17,7 @@ >>>> #include <linux/slab.h> >>>> #include <linux/spinlock.h> >>>> #include <linux/timer.h> >>>> +#include <linux/regulator/consumer.h> >>> What if you move this to leds.h so core and class can both include it. >>> >>> >>>> #include <uapi/linux/uleds.h> >>>> #include "leds.h" >>>> @@ -272,6 +273,15 @@ int of_led_classdev_register(struct device >>>> *parent, struct device_node *np, >>>> dev_warn(parent, "Led %s renamed to %s due to name collision", >>>> led_cdev->name, dev_name(led_cdev->dev)); >>>> + led_cdev->regulator = devm_regulator_get(led_cdev->dev, "power"); >>> Is the regulator always going to be called power? >> Actually in the dts, that will be "power-supply". I lacked the imagination >> to come up with a better name. >> >> >> >>>> + if (IS_ERR(led_cdev->regulator)) { >>>> + dev_err(led_cdev->dev, "Cannot get the power supply for %s\n", >>>> + led_cdev->name); >>>> + device_unregister(led_cdev->dev); >>>> + mutex_unlock(&led_cdev->led_access); >>>> + return PTR_ERR(led_cdev->regulator); >>> This is listed as optional in the DT doc. This appears to be required. >> The regulator core will provide a dummy regulator if none is given in the >> device tree. I would rather have an error in that case, but that is not how >> it works. > If you actively wanted to get -ENODEV back when there is no regulator > then you can use devm_regulator_get_optional() for that. > > However perhaps be careful what you wish for. If you use get_optional() > then you will have to sprinkle NULL or IS_ERR() checks everywhere. I'd > favour using the current approach! Thanks for the info. I think I'll use the get_optionnal(). That will add a bit of complexity, but it will avoid deferring some work in led_set_brightness_nopm() when it is not needed. JJ > > > Daniel. > >> >>> I prefer to keep it optional. Many LED drivers are connected to fixed >>> non-managed supplies. >>> >>>> + } >>>> + >>>> if (led_cdev->flags & LED_BRIGHT_HW_CHANGED) { >>>> ret = led_add_brightness_hw_changed(led_cdev); >>>> if (ret) { >>>> diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c >>>> index 7107cd7e87cf..139de6b08cad 100644 >>>> --- a/drivers/leds/led-core.c >>>> +++ b/drivers/leds/led-core.c >>>> @@ -16,6 +16,7 @@ >>>> #include <linux/rwsem.h> >>>> #include <linux/slab.h> >>>> #include "leds.h" >>>> +#include <linux/regulator/consumer.h> >>>> DECLARE_RWSEM(leds_list_lock); >>>> EXPORT_SYMBOL_GPL(leds_list_lock); >>>> @@ -23,6 +24,31 @@ EXPORT_SYMBOL_GPL(leds_list_lock); >>>> LIST_HEAD(leds_list); >>>> EXPORT_SYMBOL_GPL(leds_list); >>>> +static bool __led_need_regulator_update(struct led_classdev >>>> *led_cdev, >>>> + int brightness) >>>> +{ >>>> + bool new_regulator_state = (brightness != LED_OFF); >>>> + >>>> + return led_cdev->regulator_state != new_regulator_state; >>>> +} >>>> + >>>> +static int __led_handle_regulator(struct led_classdev *led_cdev, >>>> + int brightness) >>>> +{ >>>> + if (__led_need_regulator_update(led_cdev, brightness)) { >>>> + int ret; >>> Prefer to this to be moved up. >> ok >>>> + >>>> + if (brightness != LED_OFF) >>>> + ret = regulator_enable(led_cdev->regulator); >>>> + else >>>> + ret = regulator_disable(led_cdev->regulator); >>>> + if (ret) >>>> + return ret; >>> new line >>>> + led_cdev->regulator_state = (brightness != LED_OFF); >>>> + } >>>> + return 0; >>>> +} >>>> + >>>> static int __led_set_brightness(struct led_classdev *led_cdev, >>>> enum led_brightness value) >>>> { >>>> @@ -80,6 +106,7 @@ static void led_timer_function(struct timer_list *t) >>>> } >>>> led_set_brightness_nosleep(led_cdev, brightness); >>>> + __led_handle_regulator(led_cdev, brightness); >>> Again this seems to indicate that the regulator is a required property >>> for the LEDs >>> >>> This needs to be made optional. And the same comment through out for >>> every call. >>> >>> >>>> /* 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 + >>>> @@ -115,6 +142,8 @@ static void set_brightness_delayed(struct >>>> work_struct *ws) >>>> if (ret == -ENOTSUPP) >>>> ret = __led_set_brightness_blocking(led_cdev, >>>> led_cdev->delayed_set_value); >>>> + __led_handle_regulator(led_cdev, led_cdev->delayed_set_value); >>>> + >>>> if (ret < 0 && >>>> /* LED HW might have been unplugged, therefore don't warn */ >>>> !(ret == -ENODEV && (led_cdev->flags & LED_UNREGISTERING) && >>>> @@ -141,6 +170,7 @@ static void led_set_software_blink(struct >>>> led_classdev *led_cdev, >>>> /* never on - just set to off */ >>>> if (!delay_on) { >>>> led_set_brightness_nosleep(led_cdev, LED_OFF); >>>> + __led_handle_regulator(led_cdev, LED_OFF); >>>> return; >>>> } >>>> @@ -148,6 +178,7 @@ static void led_set_software_blink(struct >>>> led_classdev *led_cdev, >>>> if (!delay_off) { >>>> led_set_brightness_nosleep(led_cdev, >>>> led_cdev->blink_brightness); >>>> + __led_handle_regulator(led_cdev, led_cdev->blink_brightness); >>>> return; >>>> } >>>> @@ -256,8 +287,14 @@ void led_set_brightness_nopm(struct >>>> led_classdev *led_cdev, >>>> enum led_brightness value) >>>> { >>>> /* Use brightness_set op if available, it is guaranteed not to >>>> sleep */ >>>> - if (!__led_set_brightness(led_cdev, value)) >>>> - return; >>>> + if (!__led_set_brightness(led_cdev, value)) { >>>> + /* >>>> + * if regulator state doesn't need to be changed, that is all/ >>>> + * Otherwise delegate the change to a work queue >>>> + */ >>>> + if (!__led_need_regulator_update(led_cdev, value)) >>>> + return; >>>> + } >>>> /* If brightness setting can sleep, delegate it to a work >>>> queue task */ >>>> led_cdev->delayed_set_value = value; >>>> @@ -280,6 +317,8 @@ EXPORT_SYMBOL_GPL(led_set_brightness_nosleep); >>>> int led_set_brightness_sync(struct led_classdev *led_cdev, >>>> enum led_brightness value) >>>> { >>>> + int ret; >>>> + >>>> if (led_cdev->blink_delay_on || led_cdev->blink_delay_off) >>>> return -EBUSY; >>>> @@ -288,7 +327,15 @@ int led_set_brightness_sync(struct >>>> led_classdev *led_cdev, >>>> if (led_cdev->flags & LED_SUSPENDED) >>>> return 0; >>>> - return __led_set_brightness_blocking(led_cdev, >>>> led_cdev->brightness); >>>> + ret = __led_set_brightness_blocking(led_cdev, >>>> led_cdev->brightness); >>>> + if (ret) >>>> + return ret; >>>> + >>>> + ret = __led_handle_regulator(led_cdev, led_cdev->brightness); >>> Can't you just return here? >> ok >> >> >> thanks for the review >> >> JJ >> >>> Dan >>> >>>> + if (ret) >>>> + return ret; >>>> + >>>> + return 0; >>>> } >>>> EXPORT_SYMBOL_GPL(led_set_brightness_sync); >>>> diff --git a/include/linux/leds.h b/include/linux/leds.h >>>> index 9b2bf574a17a..bee8e3f8dddd 100644 >>>> --- a/include/linux/leds.h >>>> +++ b/include/linux/leds.h >>>> @@ -123,6 +123,10 @@ struct led_classdev { >>>> /* Ensures consistent access to the LED Flash Class device */ >>>> struct mutex led_access; >>>> + >>>> + /* regulator */ >>>> + struct regulator *regulator; >>>> + bool regulator_state; >>>> }; >>>> extern int of_led_classdev_register(struct device *parent, ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/2] leds: Add control of the voltage/current regulator to the LED core 2019-07-15 9:47 ` Jean-Jacques Hiblot @ 2019-07-15 15:19 ` Daniel Thompson 0 siblings, 0 replies; 18+ messages in thread From: Daniel Thompson @ 2019-07-15 15:19 UTC (permalink / raw) To: Jean-Jacques Hiblot Cc: Dan Murphy, jacek.anaszewski, pavel, robh+dt, mark.rutland, linux-leds, linux-kernel, devicetree On Mon, Jul 15, 2019 at 11:47:08AM +0200, Jean-Jacques Hiblot wrote: > > > > > + if (IS_ERR(led_cdev->regulator)) { > > > > > + dev_err(led_cdev->dev, "Cannot get the power supply for %s\n", > > > > > + led_cdev->name); > > > > > + device_unregister(led_cdev->dev); > > > > > + mutex_unlock(&led_cdev->led_access); > > > > > + return PTR_ERR(led_cdev->regulator); > > > > This is listed as optional in the DT doc. This appears to be required. > > > The regulator core will provide a dummy regulator if none is given in the > > > device tree. I would rather have an error in that case, but that is not how > > > it works. > > If you actively wanted to get -ENODEV back when there is no regulator > > then you can use devm_regulator_get_optional() for that. > > > > However perhaps be careful what you wish for. If you use get_optional() > > then you will have to sprinkle NULL or IS_ERR() checks everywhere. I'd > > favour using the current approach! > > Thanks for the info. I think I'll use the get_optionnal(). That will add a > bit of complexity, but it will avoid deferring some work in > led_set_brightness_nopm() when it is not needed. Makes sense, I didn't notice that it allows you to avoid deferred work. Daniel. ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 2/2] dt-bindings: leds: document new "power-supply" property 2019-07-08 10:35 [PATCH 0/2] leds: Add control of the voltage/current regulator to the LED core Jean-Jacques Hiblot 2019-07-08 10:35 ` Jean-Jacques Hiblot 2019-07-08 10:35 ` [PATCH 1/2] " Jean-Jacques Hiblot @ 2019-07-08 10:35 ` Jean-Jacques Hiblot 2019-07-08 10:35 ` Jean-Jacques Hiblot ` (2 more replies) 2 siblings, 3 replies; 18+ messages in thread From: Jean-Jacques Hiblot @ 2019-07-08 10:35 UTC (permalink / raw) To: jacek.anaszewski, pavel, robh+dt, mark.rutland, daniel.thompson Cc: dmurphy, linux-leds, linux-kernel, devicetree, Jean-Jacques Hiblot Most of the LEDs are powered by a voltage/current regulator. describing in the device-tree makes it possible for the LED core to enable/disable it when needed. Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com> --- Documentation/devicetree/bindings/leds/common.txt | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/Documentation/devicetree/bindings/leds/common.txt b/Documentation/devicetree/bindings/leds/common.txt index 70876ac11367..e093a2b7eb90 100644 --- a/Documentation/devicetree/bindings/leds/common.txt +++ b/Documentation/devicetree/bindings/leds/common.txt @@ -61,6 +61,11 @@ Optional properties for child nodes: - panic-indicator : This property specifies that the LED should be used, if at all possible, as a panic indicator. +- power-supply : A voltage/current regulator used to to power the LED. When a + LED is turned off, the LED core disable its regulator. The + same regulator can power many LED (or other) devices. It is + turned off only when all of its users disabled it. + - trigger-sources : List of devices which should be used as a source triggering this LED activity. Some LEDs can be related to a specific device and should somehow indicate its state. E.g. USB 2.0 -- 2.17.1 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH 2/2] dt-bindings: leds: document new "power-supply" property 2019-07-08 10:35 ` [PATCH 2/2] dt-bindings: leds: document new "power-supply" property Jean-Jacques Hiblot @ 2019-07-08 10:35 ` Jean-Jacques Hiblot 2019-07-12 18:38 ` Dan Murphy 2019-07-24 16:47 ` Rob Herring 2 siblings, 0 replies; 18+ messages in thread From: Jean-Jacques Hiblot @ 2019-07-08 10:35 UTC (permalink / raw) To: jacek.anaszewski, pavel, robh+dt, mark.rutland, daniel.thompson Cc: dmurphy, linux-leds, linux-kernel, devicetree, Jean-Jacques Hiblot Most of the LEDs are powered by a voltage/current regulator. describing in the device-tree makes it possible for the LED core to enable/disable it when needed. Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com> --- Documentation/devicetree/bindings/leds/common.txt | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/Documentation/devicetree/bindings/leds/common.txt b/Documentation/devicetree/bindings/leds/common.txt index 70876ac11367..e093a2b7eb90 100644 --- a/Documentation/devicetree/bindings/leds/common.txt +++ b/Documentation/devicetree/bindings/leds/common.txt @@ -61,6 +61,11 @@ Optional properties for child nodes: - panic-indicator : This property specifies that the LED should be used, if at all possible, as a panic indicator. +- power-supply : A voltage/current regulator used to to power the LED. When a + LED is turned off, the LED core disable its regulator. The + same regulator can power many LED (or other) devices. It is + turned off only when all of its users disabled it. + - trigger-sources : List of devices which should be used as a source triggering this LED activity. Some LEDs can be related to a specific device and should somehow indicate its state. E.g. USB 2.0 -- 2.17.1 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2] dt-bindings: leds: document new "power-supply" property 2019-07-08 10:35 ` [PATCH 2/2] dt-bindings: leds: document new "power-supply" property Jean-Jacques Hiblot 2019-07-08 10:35 ` Jean-Jacques Hiblot @ 2019-07-12 18:38 ` Dan Murphy 2019-07-12 18:38 ` Dan Murphy 2019-07-24 16:47 ` Rob Herring 2 siblings, 1 reply; 18+ messages in thread From: Dan Murphy @ 2019-07-12 18:38 UTC (permalink / raw) To: Jean-Jacques Hiblot, jacek.anaszewski, pavel, robh+dt, mark.rutland, daniel.thompson Cc: linux-leds, linux-kernel, devicetree JJ On 7/8/19 5:35 AM, Jean-Jacques Hiblot wrote: > Most of the LEDs are powered by a voltage/current regulator. describing in > the device-tree makes it possible for the LED core to enable/disable it > when needed. This should be patch 1. > Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com> > --- > Documentation/devicetree/bindings/leds/common.txt | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/Documentation/devicetree/bindings/leds/common.txt b/Documentation/devicetree/bindings/leds/common.txt > index 70876ac11367..e093a2b7eb90 100644 > --- a/Documentation/devicetree/bindings/leds/common.txt > +++ b/Documentation/devicetree/bindings/leds/common.txt > @@ -61,6 +61,11 @@ Optional properties for child nodes: > - panic-indicator : This property specifies that the LED should be used, > if at all possible, as a panic indicator. > > +- power-supply : A voltage/current regulator used to to power the LED. When a > + LED is turned off, the LED core disable its regulator. The > + same regulator can power many LED (or other) devices. It is > + turned off only when all of its users disabled it. > + > - trigger-sources : List of devices which should be used as a source triggering > this LED activity. Some LEDs can be related to a specific > device and should somehow indicate its state. E.g. USB 2.0 Do you have an example update? Dan ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2] dt-bindings: leds: document new "power-supply" property 2019-07-12 18:38 ` Dan Murphy @ 2019-07-12 18:38 ` Dan Murphy 0 siblings, 0 replies; 18+ messages in thread From: Dan Murphy @ 2019-07-12 18:38 UTC (permalink / raw) To: Jean-Jacques Hiblot, jacek.anaszewski, pavel, robh+dt, mark.rutland, daniel.thompson Cc: linux-leds, linux-kernel, devicetree JJ On 7/8/19 5:35 AM, Jean-Jacques Hiblot wrote: > Most of the LEDs are powered by a voltage/current regulator. describing in > the device-tree makes it possible for the LED core to enable/disable it > when needed. This should be patch 1. > Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com> > --- > Documentation/devicetree/bindings/leds/common.txt | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/Documentation/devicetree/bindings/leds/common.txt b/Documentation/devicetree/bindings/leds/common.txt > index 70876ac11367..e093a2b7eb90 100644 > --- a/Documentation/devicetree/bindings/leds/common.txt > +++ b/Documentation/devicetree/bindings/leds/common.txt > @@ -61,6 +61,11 @@ Optional properties for child nodes: > - panic-indicator : This property specifies that the LED should be used, > if at all possible, as a panic indicator. > > +- power-supply : A voltage/current regulator used to to power the LED. When a > + LED is turned off, the LED core disable its regulator. The > + same regulator can power many LED (or other) devices. It is > + turned off only when all of its users disabled it. > + > - trigger-sources : List of devices which should be used as a source triggering > this LED activity. Some LEDs can be related to a specific > device and should somehow indicate its state. E.g. USB 2.0 Do you have an example update? Dan ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2] dt-bindings: leds: document new "power-supply" property 2019-07-08 10:35 ` [PATCH 2/2] dt-bindings: leds: document new "power-supply" property Jean-Jacques Hiblot 2019-07-08 10:35 ` Jean-Jacques Hiblot 2019-07-12 18:38 ` Dan Murphy @ 2019-07-24 16:47 ` Rob Herring 2019-07-25 11:08 ` Jean-Jacques Hiblot 2 siblings, 1 reply; 18+ messages in thread From: Rob Herring @ 2019-07-24 16:47 UTC (permalink / raw) To: Jean-Jacques Hiblot Cc: jacek.anaszewski, pavel, mark.rutland, daniel.thompson, dmurphy, linux-leds, linux-kernel, devicetree On Mon, Jul 08, 2019 at 12:35:47PM +0200, Jean-Jacques Hiblot wrote: > Most of the LEDs are powered by a voltage/current regulator. describing in > the device-tree makes it possible for the LED core to enable/disable it > when needed. > > Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com> > --- > Documentation/devicetree/bindings/leds/common.txt | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/Documentation/devicetree/bindings/leds/common.txt b/Documentation/devicetree/bindings/leds/common.txt > index 70876ac11367..e093a2b7eb90 100644 > --- a/Documentation/devicetree/bindings/leds/common.txt > +++ b/Documentation/devicetree/bindings/leds/common.txt > @@ -61,6 +61,11 @@ Optional properties for child nodes: > - panic-indicator : This property specifies that the LED should be used, > if at all possible, as a panic indicator. > > +- power-supply : A voltage/current regulator used to to power the LED. When a > + LED is turned off, the LED core disable its regulator. The > + same regulator can power many LED (or other) devices. It is > + turned off only when all of its users disabled it. Not sure this should be common. It wouldn't apply to cases where we have an LED controller parent nor gpio and pwm LEDs and those are most cases. Perhaps what makes sense here is an regulator-led binding. > + > - trigger-sources : List of devices which should be used as a source triggering > this LED activity. Some LEDs can be related to a specific > device and should somehow indicate its state. E.g. USB 2.0 > -- > 2.17.1 > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2] dt-bindings: leds: document new "power-supply" property 2019-07-24 16:47 ` Rob Herring @ 2019-07-25 11:08 ` Jean-Jacques Hiblot 2019-07-26 10:06 ` Daniel Thompson 0 siblings, 1 reply; 18+ messages in thread From: Jean-Jacques Hiblot @ 2019-07-25 11:08 UTC (permalink / raw) To: Rob Herring Cc: jacek.anaszewski, pavel, mark.rutland, daniel.thompson, dmurphy, linux-leds, linux-kernel, devicetree Hi Rob, On 24/07/2019 18:47, Rob Herring wrote: > On Mon, Jul 08, 2019 at 12:35:47PM +0200, Jean-Jacques Hiblot wrote: >> Most of the LEDs are powered by a voltage/current regulator. describing in >> the device-tree makes it possible for the LED core to enable/disable it >> when needed. >> >> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com> >> --- >> Documentation/devicetree/bindings/leds/common.txt | 5 +++++ >> 1 file changed, 5 insertions(+) >> >> diff --git a/Documentation/devicetree/bindings/leds/common.txt b/Documentation/devicetree/bindings/leds/common.txt >> index 70876ac11367..e093a2b7eb90 100644 >> --- a/Documentation/devicetree/bindings/leds/common.txt >> +++ b/Documentation/devicetree/bindings/leds/common.txt >> @@ -61,6 +61,11 @@ Optional properties for child nodes: >> - panic-indicator : This property specifies that the LED should be used, >> if at all possible, as a panic indicator. >> >> +- power-supply : A voltage/current regulator used to to power the LED. When a >> + LED is turned off, the LED core disable its regulator. The >> + same regulator can power many LED (or other) devices. It is >> + turned off only when all of its users disabled it. > Not sure this should be common. It wouldn't apply to cases where we have > an LED controller parent nor gpio and pwm LEDs and those are most cases. It does make sense for GPIO and PWM bindings if the anode of LED is tied to a regulated voltage and the cathod to the control line. The same is true for a certain class of true LED controller that do not deliver power but act like current sinks. JJ > > Perhaps what makes sense here is an regulator-led binding. > >> + >> - trigger-sources : List of devices which should be used as a source triggering >> this LED activity. Some LEDs can be related to a specific >> device and should somehow indicate its state. E.g. USB 2.0 >> -- >> 2.17.1 >> ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2] dt-bindings: leds: document new "power-supply" property 2019-07-25 11:08 ` Jean-Jacques Hiblot @ 2019-07-26 10:06 ` Daniel Thompson 2019-07-26 22:44 ` Rob Herring 0 siblings, 1 reply; 18+ messages in thread From: Daniel Thompson @ 2019-07-26 10:06 UTC (permalink / raw) To: Jean-Jacques Hiblot Cc: Rob Herring, jacek.anaszewski, pavel, mark.rutland, dmurphy, linux-leds, linux-kernel, devicetree On Thu, Jul 25, 2019 at 01:08:46PM +0200, Jean-Jacques Hiblot wrote: > Hi Rob, > > On 24/07/2019 18:47, Rob Herring wrote: > > On Mon, Jul 08, 2019 at 12:35:47PM +0200, Jean-Jacques Hiblot wrote: > > > Most of the LEDs are powered by a voltage/current regulator. describing in > > > the device-tree makes it possible for the LED core to enable/disable it > > > when needed. > > > > > > Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com> > > > --- > > > Documentation/devicetree/bindings/leds/common.txt | 5 +++++ > > > 1 file changed, 5 insertions(+) > > > > > > diff --git a/Documentation/devicetree/bindings/leds/common.txt b/Documentation/devicetree/bindings/leds/common.txt > > > index 70876ac11367..e093a2b7eb90 100644 > > > --- a/Documentation/devicetree/bindings/leds/common.txt > > > +++ b/Documentation/devicetree/bindings/leds/common.txt > > > @@ -61,6 +61,11 @@ Optional properties for child nodes: > > > - panic-indicator : This property specifies that the LED should be used, > > > if at all possible, as a panic indicator. > > > +- power-supply : A voltage/current regulator used to to power the LED. When a > > > + LED is turned off, the LED core disable its regulator. The > > > + same regulator can power many LED (or other) devices. It is > > > + turned off only when all of its users disabled it. > > Not sure this should be common. It wouldn't apply to cases where we have > > an LED controller parent nor gpio and pwm LEDs and those are most cases. > > It does make sense for GPIO and PWM bindings if the anode of LED is tied to > a regulated voltage and the cathod to the control line. > > The same is true for a certain class of true LED controller that do not > deliver power but act like current sinks. > > JJ > > > > > Perhaps what makes sense here is an regulator-led binding. You didn't comment on this alternative... and I confess I'm not quite sure what Rob means by a regulator-led binding so I can't really comment either. Rob, is there any analogous example for a regulator-<something-else> binding to compare with? Daniel. > > > > > + > > > - trigger-sources : List of devices which should be used as a source triggering > > > this LED activity. Some LEDs can be related to a specific > > > device and should somehow indicate its state. E.g. USB 2.0 > > > -- > > > 2.17.1 > > > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2] dt-bindings: leds: document new "power-supply" property 2019-07-26 10:06 ` Daniel Thompson @ 2019-07-26 22:44 ` Rob Herring 0 siblings, 0 replies; 18+ messages in thread From: Rob Herring @ 2019-07-26 22:44 UTC (permalink / raw) To: Daniel Thompson, Jean-Jacques Hiblot Cc: Jacek Anaszewski, Pavel Machek, Mark Rutland, Dan Murphy, Linux LED Subsystem, linux-kernel, devicetree On Fri, Jul 26, 2019 at 4:06 AM Daniel Thompson <daniel.thompson@linaro.org> wrote: > > On Thu, Jul 25, 2019 at 01:08:46PM +0200, Jean-Jacques Hiblot wrote: > > Hi Rob, > > > > On 24/07/2019 18:47, Rob Herring wrote: > > > On Mon, Jul 08, 2019 at 12:35:47PM +0200, Jean-Jacques Hiblot wrote: > > > > Most of the LEDs are powered by a voltage/current regulator. describing in > > > > the device-tree makes it possible for the LED core to enable/disable it > > > > when needed. > > > > > > > > Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com> > > > > --- > > > > Documentation/devicetree/bindings/leds/common.txt | 5 +++++ > > > > 1 file changed, 5 insertions(+) > > > > > > > > diff --git a/Documentation/devicetree/bindings/leds/common.txt b/Documentation/devicetree/bindings/leds/common.txt > > > > index 70876ac11367..e093a2b7eb90 100644 > > > > --- a/Documentation/devicetree/bindings/leds/common.txt > > > > +++ b/Documentation/devicetree/bindings/leds/common.txt > > > > @@ -61,6 +61,11 @@ Optional properties for child nodes: > > > > - panic-indicator : This property specifies that the LED should be used, > > > > if at all possible, as a panic indicator. > > > > +- power-supply : A voltage/current regulator used to to power the LED. When a > > > > + LED is turned off, the LED core disable its regulator. The > > > > + same regulator can power many LED (or other) devices. It is > > > > + turned off only when all of its users disabled it. > > > Not sure this should be common. It wouldn't apply to cases where we have > > > an LED controller parent nor gpio and pwm LEDs and those are most cases. > > > > It does make sense for GPIO and PWM bindings if the anode of LED is tied to > > a regulated voltage and the cathod to the control line. Okay. Is one of those your case, or you only have regulator control? The latter would need a new binding. If you want to use power-supply with either GPIO and PWM LED bindings, then it should still be listed in those as an applicable property. > > The same is true for a certain class of true LED controller that do not > > deliver power but act like current sinks. > > > > JJ > > > > > > > > Perhaps what makes sense here is an regulator-led binding. > > You didn't comment on this alternative... and I confess I'm not quite > sure what Rob means by a regulator-led binding so I can't really comment > either. > > Rob, is there any analogous example for a regulator-<something-else> binding > to compare with? regulator-haptic is the only one I found in a quick search. Rob ^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2019-07-26 22:44 UTC | newest] Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-07-08 10:35 [PATCH 0/2] leds: Add control of the voltage/current regulator to the LED core Jean-Jacques Hiblot 2019-07-08 10:35 ` Jean-Jacques Hiblot 2019-07-08 10:35 ` [PATCH 1/2] " Jean-Jacques Hiblot 2019-07-08 10:35 ` Jean-Jacques Hiblot 2019-07-12 18:49 ` Dan Murphy 2019-07-12 18:49 ` Dan Murphy 2019-07-15 9:01 ` Jean-Jacques Hiblot 2019-07-15 9:24 ` Daniel Thompson 2019-07-15 9:47 ` Jean-Jacques Hiblot 2019-07-15 15:19 ` Daniel Thompson 2019-07-08 10:35 ` [PATCH 2/2] dt-bindings: leds: document new "power-supply" property Jean-Jacques Hiblot 2019-07-08 10:35 ` Jean-Jacques Hiblot 2019-07-12 18:38 ` Dan Murphy 2019-07-12 18:38 ` Dan Murphy 2019-07-24 16:47 ` Rob Herring 2019-07-25 11:08 ` Jean-Jacques Hiblot 2019-07-26 10:06 ` Daniel Thompson 2019-07-26 22:44 ` Rob Herring
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).