* [PATCH v3 0/3] leds: Add control of the voltage/current regulator to the LED core @ 2019-07-17 13:59 Jean-Jacques Hiblot 2019-07-17 13:59 ` [PATCH v3 1/3] dt-bindings: leds: document the "power-supply" property Jean-Jacques Hiblot ` (2 more replies) 0 siblings, 3 replies; 8+ messages in thread From: Jean-Jacques Hiblot @ 2019-07-17 13:59 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. While at it, throw in a fix for led_set_brightness_sync() so that it can work with drivers that don't provide brightness_set_blocking() changes in v3: - reword device-tree description - reword commit log - remove regulator updates from functions used in atomic context. If the regulator must be updated, it is defered to a workqueue. - Fix led_set_brightness_sync() to work with the non-blocking function __led_set_brightness() changes in v2: - use devm_regulator_get_optional() to avoid using the dummy regulator and do some unnecessary work Jean-Jacques Hiblot (3): dt-bindings: leds: document the "power-supply" property leds: Add control of the voltage/current regulator to the LED core leds: Make led_set_brightness_sync() also use __led_set_brightness() .../devicetree/bindings/leds/common.txt | 4 ++ drivers/leds/led-class.c | 15 ++++++ drivers/leds/led-core.c | 53 +++++++++++++++++-- drivers/leds/leds.h | 1 + include/linux/leds.h | 4 ++ 5 files changed, 73 insertions(+), 4 deletions(-) -- 2.17.1 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v3 1/3] dt-bindings: leds: document the "power-supply" property 2019-07-17 13:59 [PATCH v3 0/3] leds: Add control of the voltage/current regulator to the LED core Jean-Jacques Hiblot @ 2019-07-17 13:59 ` Jean-Jacques Hiblot 2019-07-17 13:59 ` [PATCH v3 2/3] leds: Add control of the voltage/current regulator to the LED core Jean-Jacques Hiblot 2019-07-17 13:59 ` [PATCH v3 3/3] leds: Make led_set_brightness_sync() also use __led_set_brightness() Jean-Jacques Hiblot 2 siblings, 0 replies; 8+ messages in thread From: Jean-Jacques Hiblot @ 2019-07-17 13:59 UTC (permalink / raw) To: jacek.anaszewski, pavel, robh+dt, mark.rutland, daniel.thompson Cc: dmurphy, linux-leds, linux-kernel, devicetree, Jean-Jacques Hiblot Sometimes 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> Reviewed-by: Dan Murphy <dmurphy@ti.com> --- Documentation/devicetree/bindings/leds/common.txt | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/Documentation/devicetree/bindings/leds/common.txt b/Documentation/devicetree/bindings/leds/common.txt index 70876ac11367..f496ec1c1542 100644 --- a/Documentation/devicetree/bindings/leds/common.txt +++ b/Documentation/devicetree/bindings/leds/common.txt @@ -61,6 +61,9 @@ 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 : Is a phandle to a voltage/current regulator used to to power + the LED. + - 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 +109,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] 8+ messages in thread
* [PATCH v3 2/3] leds: Add control of the voltage/current regulator to the LED core 2019-07-17 13:59 [PATCH v3 0/3] leds: Add control of the voltage/current regulator to the LED core Jean-Jacques Hiblot 2019-07-17 13:59 ` [PATCH v3 1/3] dt-bindings: leds: document the "power-supply" property Jean-Jacques Hiblot @ 2019-07-17 13:59 ` Jean-Jacques Hiblot 2019-07-18 12:24 ` Jacek Anaszewski 2019-07-17 13:59 ` [PATCH v3 3/3] leds: Make led_set_brightness_sync() also use __led_set_brightness() Jean-Jacques Hiblot 2 siblings, 1 reply; 8+ messages in thread From: Jean-Jacques Hiblot @ 2019-07-17 13:59 UTC (permalink / raw) To: jacek.anaszewski, pavel, robh+dt, mark.rutland, daniel.thompson Cc: dmurphy, linux-leds, linux-kernel, devicetree, Jean-Jacques Hiblot Sometimes LEDs are powered by an external voltage/current regulator. Let the LED core know about it. This allows the LED core to turn on or off managed power supplies. Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com> Reviewed-by: Dan Murphy <dmurphy@ti.com> --- drivers/leds/led-class.c | 15 +++++++++++++ drivers/leds/led-core.c | 47 +++++++++++++++++++++++++++++++++++++--- drivers/leds/leds.h | 1 + include/linux/leds.h | 4 ++++ 4 files changed, 64 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..dab32cf778f2 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) { @@ -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) && @@ -256,8 +285,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 +315,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 +325,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] 8+ messages in thread
* Re: [PATCH v3 2/3] leds: Add control of the voltage/current regulator to the LED core 2019-07-17 13:59 ` [PATCH v3 2/3] leds: Add control of the voltage/current regulator to the LED core Jean-Jacques Hiblot @ 2019-07-18 12:24 ` Jacek Anaszewski 2019-07-18 13:31 ` Jean-Jacques Hiblot 0 siblings, 1 reply; 8+ messages in thread From: Jacek Anaszewski @ 2019-07-18 12:24 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 updated patch set. I have some more comments below. On 7/17/19 3:59 PM, Jean-Jacques Hiblot wrote: > Sometimes LEDs are powered by an external voltage/current regulator. Let > the LED core know about it. This allows the LED core to turn on or off > managed power supplies. > > Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com> > Reviewed-by: Dan Murphy <dmurphy@ti.com> > --- > drivers/leds/led-class.c | 15 +++++++++++++ > drivers/leds/led-core.c | 47 +++++++++++++++++++++++++++++++++++++--- > drivers/leds/leds.h | 1 + > include/linux/leds.h | 4 ++++ > 4 files changed, 64 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..dab32cf778f2 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); How about: bool new_state = !!brightness; > + > + 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; > +} Let's have these function names without leading underscores. > static int __led_set_brightness(struct led_classdev *led_cdev, > enum led_brightness value) > { > @@ -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 you called it from __led_set_brightness() and from __led_set_brightness_blocking() you wouldn't need this change here. > + > if (ret < 0 && > /* LED HW might have been unplugged, therefore don't warn */ > !(ret == -ENODEV && (led_cdev->flags & LED_UNREGISTERING) && > @@ -256,8 +285,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; > + } This change should be also not needed then. > > /* If brightness setting can sleep, delegate it to a work queue task */ > led_cdev->delayed_set_value = value; > @@ -280,6 +315,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 +325,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); As well as this one. > } > 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] 8+ messages in thread
* Re: [PATCH v3 2/3] leds: Add control of the voltage/current regulator to the LED core 2019-07-18 12:24 ` Jacek Anaszewski @ 2019-07-18 13:31 ` Jean-Jacques Hiblot 2019-07-18 17:49 ` Jacek Anaszewski 0 siblings, 1 reply; 8+ messages in thread From: Jean-Jacques Hiblot @ 2019-07-18 13:31 UTC (permalink / raw) To: Jacek Anaszewski, pavel, robh+dt, mark.rutland, daniel.thompson Cc: dmurphy, linux-leds, linux-kernel, devicetree On 18/07/2019 14:24, Jacek Anaszewski wrote: > Hi Jean, > > Thank you for the updated patch set. > > I have some more comments below. > > On 7/17/19 3:59 PM, Jean-Jacques Hiblot wrote: >> >> +static bool __led_need_regulator_update(struct led_classdev *led_cdev, >> + int brightness) >> +{ >> + bool new_state = (brightness != LED_OFF); > How about: > > bool new_state = !!brightness; Throughout the code LED_OFF is used when the LED is turned off. I think it would be more consistent to use it there too. > >> + >> + 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; >> +} > Let's have these function names without leading underscores. OK. > >> static int __led_set_brightness(struct led_classdev *led_cdev, >> enum led_brightness value) >> { >> @@ -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 you called it from __led_set_brightness() and We cannot call it from __led_set_brightness() because it is supposed not to block. JJ ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v3 2/3] leds: Add control of the voltage/current regulator to the LED core 2019-07-18 13:31 ` Jean-Jacques Hiblot @ 2019-07-18 17:49 ` Jacek Anaszewski 2019-09-20 12:29 ` Jean-Jacques Hiblot 0 siblings, 1 reply; 8+ messages in thread From: Jacek Anaszewski @ 2019-07-18 17:49 UTC (permalink / raw) To: Jean-Jacques Hiblot, pavel, robh+dt, mark.rutland, daniel.thompson Cc: dmurphy, linux-leds, linux-kernel, devicetree On 7/18/19 3:31 PM, Jean-Jacques Hiblot wrote: > > On 18/07/2019 14:24, Jacek Anaszewski wrote: >> Hi Jean, >> >> Thank you for the updated patch set. >> >> I have some more comments below. >> >> On 7/17/19 3:59 PM, Jean-Jacques Hiblot wrote: >>> +static bool __led_need_regulator_update(struct led_classdev >>> *led_cdev, >>> + int brightness) >>> +{ >>> + bool new_state = (brightness != LED_OFF); >> How about: >> >> bool new_state = !!brightness; > > Throughout the code LED_OFF is used when the LED is turned off. I think > it would be more consistent to use it there too. Basically brightness is a scalar and 0 always means off. We treat enum led_brightness as a legacy type - it is no longer valid on the whole its span since LED_FULL = 255 was depreciated with addition of max_brightness property. IMHO use of reverse logic here only hinders code analysis. >>> + >>> + 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; >>> +} >> Let's have these function names without leading underscores. > OK. >> >>> static int __led_set_brightness(struct led_classdev *led_cdev, >>> enum led_brightness value) >>> { >>> @@ -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 you called it from __led_set_brightness() and > > We cannot call it from __led_set_brightness() because it is supposed not > to block. You're right. The problematic part is that with regulator handling we cannot treat the whole brightness setting operation uniformly for brightness_set op case, i.e. without mediation of a workqueue. Now you have to fire workqueue in led_set_brightness_nopm() even for brightness_set() op path, if regulator state needs update. This is ugly and can be misleading. Can be also error prone and have non-obvious implications for software blink state transitions. I think we would first need to improve locking between the workqueue and led_timer_function(). I proposed a patch [0] over a year ago. Only then we could think of adding another asynchronous dependency to the brightness setting chain. [0] https://lkml.org/lkml/2018/1/17/1144 -- Best regards, Jacek Anaszewski ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v3 2/3] leds: Add control of the voltage/current regulator to the LED core 2019-07-18 17:49 ` Jacek Anaszewski @ 2019-09-20 12:29 ` Jean-Jacques Hiblot 0 siblings, 0 replies; 8+ messages in thread From: Jean-Jacques Hiblot @ 2019-09-20 12:29 UTC (permalink / raw) To: Jacek Anaszewski, pavel, robh+dt, mark.rutland, daniel.thompson Cc: dmurphy, linux-leds, linux-kernel, devicetree Hi Jacek, On 18/07/2019 19:49, Jacek Anaszewski wrote: > On 7/18/19 3:31 PM, Jean-Jacques Hiblot wrote: >> On 18/07/2019 14:24, Jacek Anaszewski wrote: >>> Hi Jean, >>> >>> Thank you for the updated patch set. >>> >>> I have some more comments below. >>> >>> On 7/17/19 3:59 PM, Jean-Jacques Hiblot wrote: >>>> +static bool __led_need_regulator_update(struct led_classdev >>>> *led_cdev, >>>> + int brightness) >>>> +{ >>>> + bool new_state = (brightness != LED_OFF); >>> How about: >>> >>> bool new_state = !!brightness; >> Throughout the code LED_OFF is used when the LED is turned off. I think >> it would be more consistent to use it there too. > Basically brightness is a scalar and 0 always means off. > We treat enum led_brightness as a legacy type - it is no > longer valid on the whole its span since LED_FULL = 255 > was depreciated with addition of max_brightness property. > > IMHO use of reverse logic here only hinders code analysis. > >>>> + >>>> + 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; >>>> +} >>> Let's have these function names without leading underscores. >> OK. >>>> static int __led_set_brightness(struct led_classdev *led_cdev, >>>> enum led_brightness value) >>>> { >>>> @@ -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 you called it from __led_set_brightness() and >> We cannot call it from __led_set_brightness() because it is supposed not >> to block. > You're right. The problematic part is that with regulator handling > we cannot treat the whole brightness setting operation uniformly > for brightness_set op case, i.e. without mediation of a workqueue. > > Now you have to fire workqueue in led_set_brightness_nopm() > even for brightness_set() op path, if regulator state needs update. > This is ugly and can be misleading. Can be also error prone and > have non-obvious implications for software blink state transitions. Taking your queue I reworked the series to take better care of the concurrency issues. I believe it's in better shape right now. > > I think we would first need to improve locking between the workqueue > and led_timer_function(). I proposed a patch [0] over a year > ago. I tried the patch and get a lot of warning because of triggers on storage devices. Making led_set_brightness() not callable from a IRQ context, is probably not the right approach anymore. JJ > > Only then we could think of adding another asynchronous dependency > to the brightness setting chain. > > [0] https://lkml.org/lkml/2018/1/17/1144 > ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v3 3/3] leds: Make led_set_brightness_sync() also use __led_set_brightness() 2019-07-17 13:59 [PATCH v3 0/3] leds: Add control of the voltage/current regulator to the LED core Jean-Jacques Hiblot 2019-07-17 13:59 ` [PATCH v3 1/3] dt-bindings: leds: document the "power-supply" property Jean-Jacques Hiblot 2019-07-17 13:59 ` [PATCH v3 2/3] leds: Add control of the voltage/current regulator to the LED core Jean-Jacques Hiblot @ 2019-07-17 13:59 ` Jean-Jacques Hiblot 2 siblings, 0 replies; 8+ messages in thread From: Jean-Jacques Hiblot @ 2019-07-17 13:59 UTC (permalink / raw) To: jacek.anaszewski, pavel, robh+dt, mark.rutland, daniel.thompson Cc: dmurphy, linux-leds, linux-kernel, devicetree, Jean-Jacques Hiblot There are some LED drivers that do not implement brightness_set_blocking(), for those drivers led_set_brightness_sync() cannot work. Fixing it by calling first __led_set_brightness() and falling back to __led_set_brightness_blocking() if it failed. Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com> --- drivers/leds/led-core.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c index dab32cf778f2..4a0506081c0e 100644 --- a/drivers/leds/led-core.c +++ b/drivers/leds/led-core.c @@ -320,15 +320,19 @@ int led_set_brightness_sync(struct led_classdev *led_cdev, if (led_cdev->blink_delay_on || led_cdev->blink_delay_off) return -EBUSY; - led_cdev->brightness = min(value, led_cdev->max_brightness); + value = min(value, led_cdev->max_brightness); if (led_cdev->flags & LED_SUSPENDED) return 0; - ret = __led_set_brightness_blocking(led_cdev, led_cdev->brightness); + ret = __led_set_brightness(led_cdev, value); + if (ret == -ENOTSUPP) + ret = __led_set_brightness_blocking(led_cdev, value); if (ret) return ret; + led_cdev->brightness = value; + return __led_handle_regulator(led_cdev, led_cdev->brightness); } EXPORT_SYMBOL_GPL(led_set_brightness_sync); -- 2.17.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
end of thread, other threads:[~2019-09-20 12:30 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-07-17 13:59 [PATCH v3 0/3] leds: Add control of the voltage/current regulator to the LED core Jean-Jacques Hiblot 2019-07-17 13:59 ` [PATCH v3 1/3] dt-bindings: leds: document the "power-supply" property Jean-Jacques Hiblot 2019-07-17 13:59 ` [PATCH v3 2/3] leds: Add control of the voltage/current regulator to the LED core Jean-Jacques Hiblot 2019-07-18 12:24 ` Jacek Anaszewski 2019-07-18 13:31 ` Jean-Jacques Hiblot 2019-07-18 17:49 ` Jacek Anaszewski 2019-09-20 12:29 ` Jean-Jacques Hiblot 2019-07-17 13:59 ` [PATCH v3 3/3] leds: Make led_set_brightness_sync() also use __led_set_brightness() 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).