[PATCH 0/2] leds: Add control of the voltage/current regulator to the LED core

[PATCH 0/2] leds: Add control of the voltage/current regulator to the LED core

[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

[PATCH 1/2] leds: Add control of the voltage/current regulator to the LED core

[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

[PATCH 2/2] dt-bindings: leds: document new "power-supply" property

[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

Re: [PATCH 2/2] dt-bindings: leds: document new "power-supply" property

[Dan Murphy]

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

Re: [PATCH 1/2] leds: Add control of the voltage/current regulator to the LED core

[Dan Murphy]

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,

Re: [PATCH 1/2] leds: Add control of the voltage/current regulator to the LED core

[Jean-Jacques Hiblot]

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,

Re: [PATCH 1/2] leds: Add control of the voltage/current regulator to the LED core

[Daniel Thompson]

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,

Re: [PATCH 1/2] leds: Add control of the voltage/current regulator to the LED core

[Jean-Jacques Hiblot]

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,

Re: [PATCH 1/2] leds: Add control of the voltage/current regulator to the LED core

[Daniel Thompson]

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.

Re: [PATCH 2/2] dt-bindings: leds: document new "power-supply" property

[Rob Herring]

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
> 

Re: [PATCH 2/2] dt-bindings: leds: document new "power-supply" property

[Jean-Jacques Hiblot]

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
>>

Re: [PATCH 2/2] dt-bindings: leds: document new "power-supply" property

[Daniel Thompson]

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
> > > 

Re: [PATCH 2/2] dt-bindings: leds: document new "power-supply" property

[Rob Herring]

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