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
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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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 <[email protected]>
<snip>
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 <[email protected]>
> ---
> 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
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.
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 <[email protected]>
>> ---
>> 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 <[email protected]>
>
> <snip>
>
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
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.