* [PATCH v2 0/2] leds: Add control of the voltage/current regulator to the LED core @ 2019-07-15 15:56 Jean-Jacques Hiblot 2019-07-15 15:56 ` [PATCH v2 1/2] dt-bindings: leds: document the "power-supply" property Jean-Jacques Hiblot 2019-07-15 15:56 ` [PATCH v2 2/2] leds: Add control of the voltage/current regulator to the LED core Jean-Jacques Hiblot 0 siblings, 2 replies; 11+ messages in thread From: Jean-Jacques Hiblot @ 2019-07-15 15:56 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. changes in v2: - use devm_regulator_get_optional() to avoid using the dummy regulator and do some unnecessary work Jean-Jacques Hiblot (2): dt-bindings: leds: document the "power-supply" property leds: Add control of the voltage/current regulator to the LED core .../devicetree/bindings/leds/common.txt | 6 +++ drivers/leds/led-class.c | 15 ++++++ drivers/leds/led-core.c | 50 +++++++++++++++++-- drivers/leds/leds.h | 1 + include/linux/leds.h | 4 ++ 5 files changed, 73 insertions(+), 3 deletions(-) -- 2.17.1 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2 1/2] dt-bindings: leds: document the "power-supply" property 2019-07-15 15:56 [PATCH v2 0/2] leds: Add control of the voltage/current regulator to the LED core Jean-Jacques Hiblot @ 2019-07-15 15:56 ` Jean-Jacques Hiblot 2019-07-15 18:52 ` Dan Murphy 2019-08-19 11:31 ` Pavel Machek 2019-07-15 15:56 ` [PATCH v2 2/2] leds: Add control of the voltage/current regulator to the LED core Jean-Jacques Hiblot 1 sibling, 2 replies; 11+ messages in thread From: Jean-Jacques Hiblot @ 2019-07-15 15:56 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 it 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 | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/Documentation/devicetree/bindings/leds/common.txt b/Documentation/devicetree/bindings/leds/common.txt index 70876ac11367..539e124b1457 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 @@ -106,6 +111,7 @@ gpio-leds { label = "Status"; linux,default-trigger = "heartbeat"; gpios = <&gpio0 0 GPIO_ACTIVE_HIGH>; + power-supply = <&led_regulator>; }; usb { -- 2.17.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/2] dt-bindings: leds: document the "power-supply" property 2019-07-15 15:56 ` [PATCH v2 1/2] dt-bindings: leds: document the "power-supply" property Jean-Jacques Hiblot @ 2019-07-15 18:52 ` Dan Murphy 2019-08-19 11:31 ` Pavel Machek 1 sibling, 0 replies; 11+ messages in thread From: Dan Murphy @ 2019-07-15 18:52 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/15/19 10:56 AM, Jean-Jacques Hiblot wrote: > Most of the LEDs are powered by a voltage/current regulator. Describing it > 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 | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/Documentation/devicetree/bindings/leds/common.txt b/Documentation/devicetree/bindings/leds/common.txt > index 70876ac11367..539e124b1457 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 Is the phandle to a voltage/current regulator used to to power the LED > + LED is turned off, the LED core disable its regulator. The The regulator is only disabled if it is the only consumer and/or the number of users = 0. > + 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 > @@ -106,6 +111,7 @@ gpio-leds { > label = "Status"; > linux,default-trigger = "heartbeat"; > gpios = <&gpio0 0 GPIO_ACTIVE_HIGH>; > + power-supply = <&led_regulator>; > }; > > usb { Reviewed-by: Dan Murphy <dmurphy@ti.com> ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/2] dt-bindings: leds: document the "power-supply" property 2019-07-15 15:56 ` [PATCH v2 1/2] dt-bindings: leds: document the "power-supply" property Jean-Jacques Hiblot 2019-07-15 18:52 ` Dan Murphy @ 2019-08-19 11:31 ` Pavel Machek 1 sibling, 0 replies; 11+ messages in thread From: Pavel Machek @ 2019-08-19 11:31 UTC (permalink / raw) To: Jean-Jacques Hiblot Cc: jacek.anaszewski, robh+dt, mark.rutland, daniel.thompson, dmurphy, linux-leds, linux-kernel, devicetree [-- Attachment #1: Type: text/plain, Size: 470 bytes --] On Mon 2019-07-15 17:56:56, Jean-Jacques Hiblot wrote: > Most of the LEDs are powered by a voltage/current regulator. Describing it > 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> Acked-by: Pavel Machek <pavel@ucw.cz> -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 181 bytes --] ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2 2/2] leds: Add control of the voltage/current regulator to the LED core 2019-07-15 15:56 [PATCH v2 0/2] leds: Add control of the voltage/current regulator to the LED core Jean-Jacques Hiblot 2019-07-15 15:56 ` [PATCH v2 1/2] dt-bindings: leds: document the "power-supply" property Jean-Jacques Hiblot @ 2019-07-15 15:56 ` Jean-Jacques Hiblot 2019-07-15 18:59 ` Dan Murphy ` (2 more replies) 1 sibling, 3 replies; 11+ messages in thread From: Jean-Jacques Hiblot @ 2019-07-15 15:56 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 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 | 15 ++++++++++++ drivers/leds/led-core.c | 50 +++++++++++++++++++++++++++++++++++++--- drivers/leds/leds.h | 1 + include/linux/leds.h | 4 ++++ 4 files changed, 67 insertions(+), 3 deletions(-) diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c index 4793e77808e2..cadd43c30d50 100644 --- a/drivers/leds/led-class.c +++ b/drivers/leds/led-class.c @@ -253,6 +253,7 @@ int of_led_classdev_register(struct device *parent, struct device_node *np, { char name[LED_MAX_NAME_SIZE]; int ret; + struct regulator *regulator; ret = led_classdev_next_name(led_cdev->name, name, sizeof(name)); if (ret < 0) @@ -272,6 +273,20 @@ 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)); + regulator = devm_regulator_get_optional(led_cdev->dev, "power"); + if (IS_ERR(regulator)) { + if (PTR_ERR(regulator) != -ENODEV) { + 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(regulator); + } + led_cdev->regulator = NULL; + } else { + led_cdev->regulator = 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..a12b880b0a2f 100644 --- a/drivers/leds/led-core.c +++ b/drivers/leds/led-core.c @@ -23,6 +23,33 @@ 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_state = (brightness != LED_OFF); + + return led_cdev->regulator && led_cdev->regulator_state != new_state; +} + +static int __led_handle_regulator(struct led_classdev *led_cdev, + int brightness) +{ + int rc; + + if (__led_need_regulator_update(led_cdev, brightness)) { + + if (brightness != LED_OFF) + rc = regulator_enable(led_cdev->regulator); + else + rc = regulator_disable(led_cdev->regulator); + if (rc) + return rc; + + 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 +107,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 +143,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 +171,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 +179,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 +288,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 +318,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 +328,11 @@ 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; + + return __led_handle_regulator(led_cdev, led_cdev->brightness); } EXPORT_SYMBOL_GPL(led_set_brightness_sync); diff --git a/drivers/leds/leds.h b/drivers/leds/leds.h index 47b229469069..5aa5c038bd38 100644 --- a/drivers/leds/leds.h +++ b/drivers/leds/leds.h @@ -11,6 +11,7 @@ #include <linux/rwsem.h> #include <linux/leds.h> +#include <linux/regulator/consumer.h> static inline int led_get_brightness(struct led_classdev *led_cdev) { 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] 11+ messages in thread
* Re: [PATCH v2 2/2] leds: Add control of the voltage/current regulator to the LED core 2019-07-15 15:56 ` [PATCH v2 2/2] leds: Add control of the voltage/current regulator to the LED core Jean-Jacques Hiblot @ 2019-07-15 18:59 ` Dan Murphy 2019-07-17 13:14 ` Jean-Jacques Hiblot 2019-07-15 20:42 ` Jacek Anaszewski 2019-07-16 10:50 ` Daniel Thompson 2 siblings, 1 reply; 11+ messages in thread From: Dan Murphy @ 2019-07-15 18:59 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/15/19 10:56 AM, Jean-Jacques Hiblot wrote: > A LED is usually powered by a voltage/current regulator. Let the LED core > know about it. This allows the LED core to turn on or off the power supply > as needed. This allows the LED core to turn on or off managed power supplies. > > Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com> > --- > drivers/leds/led-class.c | 15 ++++++++++++ > drivers/leds/led-core.c | 50 +++++++++++++++++++++++++++++++++++++--- > drivers/leds/leds.h | 1 + > include/linux/leds.h | 4 ++++ > 4 files changed, 67 insertions(+), 3 deletions(-) > > diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c > index 4793e77808e2..cadd43c30d50 100644 > --- a/drivers/leds/led-class.c > +++ b/drivers/leds/led-class.c > @@ -253,6 +253,7 @@ int of_led_classdev_register(struct device *parent, struct device_node *np, > { > char name[LED_MAX_NAME_SIZE]; > int ret; > + struct regulator *regulator; > > ret = led_classdev_next_name(led_cdev->name, name, sizeof(name)); > if (ret < 0) > @@ -272,6 +273,20 @@ 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)); > > + regulator = devm_regulator_get_optional(led_cdev->dev, "power"); Store the regulator in led_cdev->regulator and drop the else case below. > + if (IS_ERR(regulator)) { > + if (PTR_ERR(regulator) != -ENODEV) { > + 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(regulator); > + } > + led_cdev->regulator = NULL; > + } else { > + led_cdev->regulator = 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..a12b880b0a2f 100644 > --- a/drivers/leds/led-core.c > +++ b/drivers/leds/led-core.c > @@ -23,6 +23,33 @@ 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_state = (brightness != LED_OFF); > + > + return led_cdev->regulator && led_cdev->regulator_state != new_state; > +} > + > +static int __led_handle_regulator(struct led_classdev *led_cdev, > + int brightness) > +{ > + int rc; Should there be a check for the regulator pointer. If (!led_cdev->regulator) return 0; Otherwise Reviewed-by: Dan Murphy <dmurphy@ti.com> <snip> ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/2] leds: Add control of the voltage/current regulator to the LED core 2019-07-15 18:59 ` Dan Murphy @ 2019-07-17 13:14 ` Jean-Jacques Hiblot 0 siblings, 0 replies; 11+ messages in thread From: Jean-Jacques Hiblot @ 2019-07-17 13:14 UTC (permalink / raw) To: Dan Murphy, jacek.anaszewski, pavel, robh+dt, mark.rutland, daniel.thompson Cc: linux-leds, linux-kernel, devicetree Hi Dan, On 15/07/2019 20:59, Dan Murphy wrote: > JJ > > On 7/15/19 10:56 AM, Jean-Jacques Hiblot wrote: >> A LED is usually powered by a voltage/current regulator. Let the LED >> core >> know about it. This allows the LED core to turn on or off the power >> supply >> as needed. > > This allows the LED core to turn on or off managed power supplies. > > >> >> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com> >> --- >> 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..a12b880b0a2f 100644 >> --- a/drivers/leds/led-core.c >> +++ b/drivers/leds/led-core.c >> @@ -23,6 +23,33 @@ 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_state = (brightness != LED_OFF); >> + >> + return led_cdev->regulator && led_cdev->regulator_state != >> new_state; >> +} >> + >> +static int __led_handle_regulator(struct led_classdev *led_cdev, >> + int brightness) >> +{ >> + int rc; > > Should there be a check for the regulator pointer. > > If (!led_cdev->regulator) > > return 0; Not required because __led_need_regulator_update() returns false if led_cdev->regulator is NULL. Thanks for the review JJ > > > Otherwise > > Reviewed-by: Dan Murphy <dmurphy@ti.com> > > <snip> > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/2] leds: Add control of the voltage/current regulator to the LED core 2019-07-15 15:56 ` [PATCH v2 2/2] leds: Add control of the voltage/current regulator to the LED core Jean-Jacques Hiblot 2019-07-15 18:59 ` Dan Murphy @ 2019-07-15 20:42 ` Jacek Anaszewski 2019-07-17 13:23 ` Jean-Jacques Hiblot 2019-07-16 10:50 ` Daniel Thompson 2 siblings, 1 reply; 11+ messages in thread From: Jacek Anaszewski @ 2019-07-15 20:42 UTC (permalink / raw) To: Jean-Jacques Hiblot, pavel, robh+dt, mark.rutland, daniel.thompson Cc: dmurphy, linux-leds, linux-kernel, devicetree Hi Jean, Thank you for the patch. I have one issue. Please refer below. On 7/15/19 5:56 PM, Jean-Jacques Hiblot wrote: > A LED is usually powered by a voltage/current regulator. 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 | 15 ++++++++++++ > drivers/leds/led-core.c | 50 +++++++++++++++++++++++++++++++++++++--- > drivers/leds/leds.h | 1 + > include/linux/leds.h | 4 ++++ > 4 files changed, 67 insertions(+), 3 deletions(-) > > diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c > index 4793e77808e2..cadd43c30d50 100644 > --- a/drivers/leds/led-class.c > +++ b/drivers/leds/led-class.c > @@ -253,6 +253,7 @@ int of_led_classdev_register(struct device *parent, struct device_node *np, > { > char name[LED_MAX_NAME_SIZE]; > int ret; > + struct regulator *regulator; > > ret = led_classdev_next_name(led_cdev->name, name, sizeof(name)); > if (ret < 0) > @@ -272,6 +273,20 @@ 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)); > > + regulator = devm_regulator_get_optional(led_cdev->dev, "power"); > + if (IS_ERR(regulator)) { > + if (PTR_ERR(regulator) != -ENODEV) { > + 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(regulator); > + } > + led_cdev->regulator = NULL; > + } else { > + led_cdev->regulator = 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..a12b880b0a2f 100644 > --- a/drivers/leds/led-core.c > +++ b/drivers/leds/led-core.c > @@ -23,6 +23,33 @@ 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_state = (brightness != LED_OFF); > + > + return led_cdev->regulator && led_cdev->regulator_state != new_state; > +} > + > +static int __led_handle_regulator(struct led_classdev *led_cdev, > + int brightness) > +{ > + int rc; > + > + if (__led_need_regulator_update(led_cdev, brightness)) { > + > + if (brightness != LED_OFF) > + rc = regulator_enable(led_cdev->regulator); > + else > + rc = regulator_disable(led_cdev->regulator); > + if (rc) > + return rc; > + > + 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 +107,7 @@ static void led_timer_function(struct timer_list *t) > } > > led_set_brightness_nosleep(led_cdev, brightness); > + __led_handle_regulator(led_cdev, brightness); This cannot be called from atomic context since regulator_enable/disable use mutex beneath, that can sleep on contention. Therefore this call has to be made in two places instead: __led_set_brightness() __led_set_brightness_blocking() > > /* 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 +143,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 +171,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 +179,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 +288,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 +318,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 +328,11 @@ 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; > + > + return __led_handle_regulator(led_cdev, led_cdev->brightness); > } > EXPORT_SYMBOL_GPL(led_set_brightness_sync); > > diff --git a/drivers/leds/leds.h b/drivers/leds/leds.h > index 47b229469069..5aa5c038bd38 100644 > --- a/drivers/leds/leds.h > +++ b/drivers/leds/leds.h > @@ -11,6 +11,7 @@ > > #include <linux/rwsem.h> > #include <linux/leds.h> > +#include <linux/regulator/consumer.h> > > static inline int led_get_brightness(struct led_classdev *led_cdev) > { > 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, > -- Best regards, Jacek Anaszewski ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/2] leds: Add control of the voltage/current regulator to the LED core 2019-07-15 20:42 ` Jacek Anaszewski @ 2019-07-17 13:23 ` Jean-Jacques Hiblot 0 siblings, 0 replies; 11+ messages in thread From: Jean-Jacques Hiblot @ 2019-07-17 13:23 UTC (permalink / raw) To: Jacek Anaszewski, pavel, mark.rutland, daniel.thompson Cc: dmurphy, linux-leds, linux-kernel Hi Jacek, On 15/07/2019 22:42, Jacek Anaszewski wrote: > @@ -80,6 +107,7 @@ static void led_timer_function(struct timer_list *t) > } > > led_set_brightness_nosleep(led_cdev, brightness); > + __led_handle_regulator(led_cdev, brightness); > This cannot be called from atomic context since regulator_enable/disable > use mutex beneath, that can sleep on contention. Therefore this call > has to be made in two places instead: > > __led_set_brightness() > __led_set_brightness_blocking() Thanks. I'll fix this in v3. JJ ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/2] leds: Add control of the voltage/current regulator to the LED core 2019-07-15 15:56 ` [PATCH v2 2/2] leds: Add control of the voltage/current regulator to the LED core Jean-Jacques Hiblot 2019-07-15 18:59 ` Dan Murphy 2019-07-15 20:42 ` Jacek Anaszewski @ 2019-07-16 10:50 ` Daniel Thompson 2019-07-17 13:47 ` Jean-Jacques Hiblot 2 siblings, 1 reply; 11+ messages in thread From: Daniel Thompson @ 2019-07-16 10:50 UTC (permalink / raw) To: Jean-Jacques Hiblot Cc: jacek.anaszewski, pavel, robh+dt, mark.rutland, dmurphy, linux-leds, linux-kernel, devicetree On Mon, Jul 15, 2019 at 05:56:57PM +0200, Jean-Jacques Hiblot wrote: > A LED is usually powered by a voltage/current regulator. Let the LED core This is almost certainly nitpicking but since there's enough other review comments that you will have to respin anyway ;-) Is an LED really "usually powered by a voltage/current regulator"? Some LEDs have a software controlled power supply but I'm not sure it is the usual case. Likewise its a little confusing to be talking about LEDs with an external current regulator since, although that is possible, it is also one the main features provided by LED driver chips. Daniel. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/2] leds: Add control of the voltage/current regulator to the LED core 2019-07-16 10:50 ` Daniel Thompson @ 2019-07-17 13:47 ` Jean-Jacques Hiblot 0 siblings, 0 replies; 11+ messages in thread From: Jean-Jacques Hiblot @ 2019-07-17 13:47 UTC (permalink / raw) To: Daniel Thompson Cc: jacek.anaszewski, pavel, robh+dt, mark.rutland, dmurphy, linux-leds, linux-kernel, devicetree On 16/07/2019 12:50, Daniel Thompson wrote: > On Mon, Jul 15, 2019 at 05:56:57PM +0200, Jean-Jacques Hiblot wrote: >> A LED is usually powered by a voltage/current regulator. Let the LED core > This is almost certainly nitpicking but since there's enough other > review comments that you will have to respin anyway ;-) No problems. comments are welcome. > Is an LED really "usually powered by a voltage/current regulator"? Some > LEDs have a software controlled power supply but I'm not sure it is > the usual case. True. I'll reword this. > Likewise its a little confusing to be talking about LEDs with an > external current regulator since, although that is possible, it is also > one the main features provided by LED driver chips. In my experience LED drivers are quite often current sinks. In that case the power is provided externally, and sometimes by a managed regulator. Thanks, JJ > > Daniel. ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2019-08-19 11:31 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-07-15 15:56 [PATCH v2 0/2] leds: Add control of the voltage/current regulator to the LED core Jean-Jacques Hiblot 2019-07-15 15:56 ` [PATCH v2 1/2] dt-bindings: leds: document the "power-supply" property Jean-Jacques Hiblot 2019-07-15 18:52 ` Dan Murphy 2019-08-19 11:31 ` Pavel Machek 2019-07-15 15:56 ` [PATCH v2 2/2] leds: Add control of the voltage/current regulator to the LED core Jean-Jacques Hiblot 2019-07-15 18:59 ` Dan Murphy 2019-07-17 13:14 ` Jean-Jacques Hiblot 2019-07-15 20:42 ` Jacek Anaszewski 2019-07-17 13:23 ` Jean-Jacques Hiblot 2019-07-16 10:50 ` Daniel Thompson 2019-07-17 13:47 ` Jean-Jacques Hiblot
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).