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

[PATCH v2 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.

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

[PATCH v2 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.

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

[PATCH v2 1/2] dt-bindings: leds: document the "power-supply" property

[Jean-Jacques Hiblot]

Most of the LEDs are powered by a voltage/current regulator. Describing it
in the device-tree makes it possible for the LED core to enable/disable it
when needed.

Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
---
 Documentation/devicetree/bindings/leds/common.txt | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/leds/common.txt b/Documentation/devicetree/bindings/leds/common.txt
index 70876ac11367..539e124b1457 100644
--- a/Documentation/devicetree/bindings/leds/common.txt
+++ b/Documentation/devicetree/bindings/leds/common.txt
@@ -61,6 +61,11 @@ Optional properties for child nodes:
 - panic-indicator : This property specifies that the LED should be used,
 		    if at all possible, as a panic indicator.
 
+- power-supply : A voltage/current regulator used to to power the LED. When a
+		 LED is turned off, the LED core disable its regulator. The
+		 same regulator can power many LED (or other) devices. It is
+		 turned off only when all of its users disabled it.
+
 - trigger-sources : List of devices which should be used as a source triggering
 		    this LED activity. Some LEDs can be related to a specific
 		    device and should somehow indicate its state. E.g. USB 2.0
@@ -106,6 +111,7 @@ gpio-leds {
 		label = "Status";
 		linux,default-trigger = "heartbeat";
 		gpios = <&gpio0 0 GPIO_ACTIVE_HIGH>;
+		power-supply = <&led_regulator>;
 	};
 
 	usb {
-- 
2.17.1

[PATCH v2 1/2] dt-bindings: leds: document the "power-supply" property

[Jean-Jacques Hiblot]

Most of the LEDs are powered by a voltage/current regulator. Describing it
in the device-tree makes it possible for the LED core to enable/disable it
when needed.

Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
---
 Documentation/devicetree/bindings/leds/common.txt | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/leds/common.txt b/Documentation/devicetree/bindings/leds/common.txt
index 70876ac11367..539e124b1457 100644
--- a/Documentation/devicetree/bindings/leds/common.txt
+++ b/Documentation/devicetree/bindings/leds/common.txt
@@ -61,6 +61,11 @@ Optional properties for child nodes:
 - panic-indicator : This property specifies that the LED should be used,
 		    if at all possible, as a panic indicator.
 
+- power-supply : A voltage/current regulator used to to power the LED. When a
+		 LED is turned off, the LED core disable its regulator. The
+		 same regulator can power many LED (or other) devices. It is
+		 turned off only when all of its users disabled it.
+
 - trigger-sources : List of devices which should be used as a source triggering
 		    this LED activity. Some LEDs can be related to a specific
 		    device and should somehow indicate its state. E.g. USB 2.0
@@ -106,6 +111,7 @@ gpio-leds {
 		label = "Status";
 		linux,default-trigger = "heartbeat";
 		gpios = <&gpio0 0 GPIO_ACTIVE_HIGH>;
+		power-supply = <&led_regulator>;
 	};
 
 	usb {
-- 
2.17.1

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

[Dan Murphy]

JJ

On 7/15/19 10:56 AM, Jean-Jacques Hiblot wrote:
> Most of the LEDs are powered by a voltage/current regulator. Describing it
> in the device-tree makes it possible for the LED core to enable/disable it
> when needed.
>
> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
> ---
>   Documentation/devicetree/bindings/leds/common.txt | 6 ++++++
>   1 file changed, 6 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/leds/common.txt b/Documentation/devicetree/bindings/leds/common.txt
> index 70876ac11367..539e124b1457 100644
> --- a/Documentation/devicetree/bindings/leds/common.txt
> +++ b/Documentation/devicetree/bindings/leds/common.txt
> @@ -61,6 +61,11 @@ Optional properties for child nodes:
>   - panic-indicator : This property specifies that the LED should be used,
>   		    if at all possible, as a panic indicator.
>   
> +- power-supply : A voltage/current regulator used to to power the LED. When a
Is the phandle to a voltage/current regulator used to to power the LED
> +		 LED is turned off, the LED core disable its regulator. The
The regulator is only disabled if it is the only consumer and/or the 
number of users = 0.
> +		 same regulator can power many LED (or other) devices. It is
> +		 turned off only when all of its users disabled it.
> +
>   - trigger-sources : List of devices which should be used as a source triggering
>   		    this LED activity. Some LEDs can be related to a specific
>   		    device and should somehow indicate its state. E.g. USB 2.0
> @@ -106,6 +111,7 @@ gpio-leds {
>   		label = "Status";
>   		linux,default-trigger = "heartbeat";
>   		gpios = <&gpio0 0 GPIO_ACTIVE_HIGH>;
> +		power-supply = <&led_regulator>;
>   	};
>   
>   	usb {


Reviewed-by: Dan Murphy <dmurphy@ti.com>

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

[Dan Murphy]

JJ

On 7/15/19 10:56 AM, Jean-Jacques Hiblot wrote:
> Most of the LEDs are powered by a voltage/current regulator. Describing it
> in the device-tree makes it possible for the LED core to enable/disable it
> when needed.
>
> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
> ---
>   Documentation/devicetree/bindings/leds/common.txt | 6 ++++++
>   1 file changed, 6 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/leds/common.txt b/Documentation/devicetree/bindings/leds/common.txt
> index 70876ac11367..539e124b1457 100644
> --- a/Documentation/devicetree/bindings/leds/common.txt
> +++ b/Documentation/devicetree/bindings/leds/common.txt
> @@ -61,6 +61,11 @@ Optional properties for child nodes:
>   - panic-indicator : This property specifies that the LED should be used,
>   		    if at all possible, as a panic indicator.
>   
> +- power-supply : A voltage/current regulator used to to power the LED. When a
Is the phandle to a voltage/current regulator used to to power the LED
> +		 LED is turned off, the LED core disable its regulator. The
The regulator is only disabled if it is the only consumer and/or the 
number of users = 0.
> +		 same regulator can power many LED (or other) devices. It is
> +		 turned off only when all of its users disabled it.
> +
>   - trigger-sources : List of devices which should be used as a source triggering
>   		    this LED activity. Some LEDs can be related to a specific
>   		    device and should somehow indicate its state. E.g. USB 2.0
> @@ -106,6 +111,7 @@ gpio-leds {
>   		label = "Status";
>   		linux,default-trigger = "heartbeat";
>   		gpios = <&gpio0 0 GPIO_ACTIVE_HIGH>;
> +		power-supply = <&led_regulator>;
>   	};
>   
>   	usb {


Reviewed-by: Dan Murphy <dmurphy@ti.com>

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

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 470 bytes --]

On Mon 2019-07-15 17:56:56, Jean-Jacques Hiblot wrote:
> Most of the LEDs are powered by a voltage/current regulator. Describing it
> in the device-tree makes it possible for the LED core to enable/disable it
> when needed.
> 
> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>

Acked-by: Pavel Machek <pavel@ucw.cz>

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[PATCH v2 2/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
know about it. This allows the LED core to turn on or off the power supply
as needed.

Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
---
 drivers/leds/led-class.c | 15 ++++++++++++
 drivers/leds/led-core.c  | 50 +++++++++++++++++++++++++++++++++++++---
 drivers/leds/leds.h      |  1 +
 include/linux/leds.h     |  4 ++++
 4 files changed, 67 insertions(+), 3 deletions(-)

diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
index 4793e77808e2..cadd43c30d50 100644
--- a/drivers/leds/led-class.c
+++ b/drivers/leds/led-class.c
@@ -253,6 +253,7 @@ int of_led_classdev_register(struct device *parent, struct device_node *np,
 {
 	char name[LED_MAX_NAME_SIZE];
 	int ret;
+	struct regulator *regulator;
 
 	ret = led_classdev_next_name(led_cdev->name, name, sizeof(name));
 	if (ret < 0)
@@ -272,6 +273,20 @@ int of_led_classdev_register(struct device *parent, struct device_node *np,
 		dev_warn(parent, "Led %s renamed to %s due to name collision",
 				led_cdev->name, dev_name(led_cdev->dev));
 
+	regulator = devm_regulator_get_optional(led_cdev->dev, "power");
+	if (IS_ERR(regulator)) {
+		if (PTR_ERR(regulator) != -ENODEV) {
+			dev_err(led_cdev->dev, "Cannot get the power supply for %s\n",
+				led_cdev->name);
+			device_unregister(led_cdev->dev);
+			mutex_unlock(&led_cdev->led_access);
+			return PTR_ERR(regulator);
+		}
+		led_cdev->regulator = NULL;
+	} else {
+		led_cdev->regulator = regulator;
+	}
+
 	if (led_cdev->flags & LED_BRIGHT_HW_CHANGED) {
 		ret = led_add_brightness_hw_changed(led_cdev);
 		if (ret) {
diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
index 7107cd7e87cf..a12b880b0a2f 100644
--- a/drivers/leds/led-core.c
+++ b/drivers/leds/led-core.c
@@ -23,6 +23,33 @@ EXPORT_SYMBOL_GPL(leds_list_lock);
 LIST_HEAD(leds_list);
 EXPORT_SYMBOL_GPL(leds_list);
 
+static bool __led_need_regulator_update(struct led_classdev *led_cdev,
+					int brightness)
+{
+	bool new_state = (brightness != LED_OFF);
+
+	return led_cdev->regulator && led_cdev->regulator_state != new_state;
+}
+
+static int __led_handle_regulator(struct led_classdev *led_cdev,
+				int brightness)
+{
+	int rc;
+
+	if (__led_need_regulator_update(led_cdev, brightness)) {
+
+		if (brightness != LED_OFF)
+			rc = regulator_enable(led_cdev->regulator);
+		else
+			rc = regulator_disable(led_cdev->regulator);
+		if (rc)
+			return rc;
+
+		led_cdev->regulator_state = (brightness != LED_OFF);
+	}
+	return 0;
+}
+
 static int __led_set_brightness(struct led_classdev *led_cdev,
 				enum led_brightness value)
 {
@@ -80,6 +107,7 @@ static void led_timer_function(struct timer_list *t)
 	}
 
 	led_set_brightness_nosleep(led_cdev, brightness);
+	__led_handle_regulator(led_cdev, brightness);
 
 	/* Return in next iteration if led is in one-shot mode and we are in
 	 * the final blink state so that the led is toggled each delay_on +
@@ -115,6 +143,8 @@ static void set_brightness_delayed(struct work_struct *ws)
 	if (ret == -ENOTSUPP)
 		ret = __led_set_brightness_blocking(led_cdev,
 					led_cdev->delayed_set_value);
+	__led_handle_regulator(led_cdev, led_cdev->delayed_set_value);
+
 	if (ret < 0 &&
 	    /* LED HW might have been unplugged, therefore don't warn */
 	    !(ret == -ENODEV && (led_cdev->flags & LED_UNREGISTERING) &&
@@ -141,6 +171,7 @@ static void led_set_software_blink(struct led_classdev *led_cdev,
 	/* never on - just set to off */
 	if (!delay_on) {
 		led_set_brightness_nosleep(led_cdev, LED_OFF);
+		__led_handle_regulator(led_cdev, LED_OFF);
 		return;
 	}
 
@@ -148,6 +179,7 @@ static void led_set_software_blink(struct led_classdev *led_cdev,
 	if (!delay_off) {
 		led_set_brightness_nosleep(led_cdev,
 					   led_cdev->blink_brightness);
+		__led_handle_regulator(led_cdev, led_cdev->blink_brightness);
 		return;
 	}
 
@@ -256,8 +288,14 @@ void led_set_brightness_nopm(struct led_classdev *led_cdev,
 			      enum led_brightness value)
 {
 	/* Use brightness_set op if available, it is guaranteed not to sleep */
-	if (!__led_set_brightness(led_cdev, value))
-		return;
+	if (!__led_set_brightness(led_cdev, value)) {
+		/*
+		 * if regulator state doesn't need to be changed, that is all/
+		 * Otherwise delegate the change to a work queue
+		 */
+		if (!__led_need_regulator_update(led_cdev, value))
+			return;
+	}
 
 	/* If brightness setting can sleep, delegate it to a work queue task */
 	led_cdev->delayed_set_value = value;
@@ -280,6 +318,8 @@ EXPORT_SYMBOL_GPL(led_set_brightness_nosleep);
 int led_set_brightness_sync(struct led_classdev *led_cdev,
 			    enum led_brightness value)
 {
+	int ret;
+
 	if (led_cdev->blink_delay_on || led_cdev->blink_delay_off)
 		return -EBUSY;
 
@@ -288,7 +328,11 @@ int led_set_brightness_sync(struct led_classdev *led_cdev,
 	if (led_cdev->flags & LED_SUSPENDED)
 		return 0;
 
-	return __led_set_brightness_blocking(led_cdev, led_cdev->brightness);
+	ret = __led_set_brightness_blocking(led_cdev, led_cdev->brightness);
+	if (ret)
+		return ret;
+
+	return __led_handle_regulator(led_cdev, led_cdev->brightness);
 }
 EXPORT_SYMBOL_GPL(led_set_brightness_sync);
 
diff --git a/drivers/leds/leds.h b/drivers/leds/leds.h
index 47b229469069..5aa5c038bd38 100644
--- a/drivers/leds/leds.h
+++ b/drivers/leds/leds.h
@@ -11,6 +11,7 @@
 
 #include <linux/rwsem.h>
 #include <linux/leds.h>
+#include <linux/regulator/consumer.h>
 
 static inline int led_get_brightness(struct led_classdev *led_cdev)
 {
diff --git a/include/linux/leds.h b/include/linux/leds.h
index 9b2bf574a17a..bee8e3f8dddd 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -123,6 +123,10 @@ struct led_classdev {
 
 	/* Ensures consistent access to the LED Flash Class device */
 	struct mutex		led_access;
+
+	/* regulator */
+	struct regulator	*regulator;
+	bool			regulator_state;
 };
 
 extern int of_led_classdev_register(struct device *parent,
-- 
2.17.1

[PATCH v2 2/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
know about it. This allows the LED core to turn on or off the power supply
as needed.

Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
---
 drivers/leds/led-class.c | 15 ++++++++++++
 drivers/leds/led-core.c  | 50 +++++++++++++++++++++++++++++++++++++---
 drivers/leds/leds.h      |  1 +
 include/linux/leds.h     |  4 ++++
 4 files changed, 67 insertions(+), 3 deletions(-)

diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
index 4793e77808e2..cadd43c30d50 100644
--- a/drivers/leds/led-class.c
+++ b/drivers/leds/led-class.c
@@ -253,6 +253,7 @@ int of_led_classdev_register(struct device *parent, struct device_node *np,
 {
 	char name[LED_MAX_NAME_SIZE];
 	int ret;
+	struct regulator *regulator;
 
 	ret = led_classdev_next_name(led_cdev->name, name, sizeof(name));
 	if (ret < 0)
@@ -272,6 +273,20 @@ int of_led_classdev_register(struct device *parent, struct device_node *np,
 		dev_warn(parent, "Led %s renamed to %s due to name collision",
 				led_cdev->name, dev_name(led_cdev->dev));
 
+	regulator = devm_regulator_get_optional(led_cdev->dev, "power");
+	if (IS_ERR(regulator)) {
+		if (PTR_ERR(regulator) != -ENODEV) {
+			dev_err(led_cdev->dev, "Cannot get the power supply for %s\n",
+				led_cdev->name);
+			device_unregister(led_cdev->dev);
+			mutex_unlock(&led_cdev->led_access);
+			return PTR_ERR(regulator);
+		}
+		led_cdev->regulator = NULL;
+	} else {
+		led_cdev->regulator = regulator;
+	}
+
 	if (led_cdev->flags & LED_BRIGHT_HW_CHANGED) {
 		ret = led_add_brightness_hw_changed(led_cdev);
 		if (ret) {
diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
index 7107cd7e87cf..a12b880b0a2f 100644
--- a/drivers/leds/led-core.c
+++ b/drivers/leds/led-core.c
@@ -23,6 +23,33 @@ EXPORT_SYMBOL_GPL(leds_list_lock);
 LIST_HEAD(leds_list);
 EXPORT_SYMBOL_GPL(leds_list);
 
+static bool __led_need_regulator_update(struct led_classdev *led_cdev,
+					int brightness)
+{
+	bool new_state = (brightness != LED_OFF);
+
+	return led_cdev->regulator && led_cdev->regulator_state != new_state;
+}
+
+static int __led_handle_regulator(struct led_classdev *led_cdev,
+				int brightness)
+{
+	int rc;
+
+	if (__led_need_regulator_update(led_cdev, brightness)) {
+
+		if (brightness != LED_OFF)
+			rc = regulator_enable(led_cdev->regulator);
+		else
+			rc = regulator_disable(led_cdev->regulator);
+		if (rc)
+			return rc;
+
+		led_cdev->regulator_state = (brightness != LED_OFF);
+	}
+	return 0;
+}
+
 static int __led_set_brightness(struct led_classdev *led_cdev,
 				enum led_brightness value)
 {
@@ -80,6 +107,7 @@ static void led_timer_function(struct timer_list *t)
 	}
 
 	led_set_brightness_nosleep(led_cdev, brightness);
+	__led_handle_regulator(led_cdev, brightness);
 
 	/* Return in next iteration if led is in one-shot mode and we are in
 	 * the final blink state so that the led is toggled each delay_on +
@@ -115,6 +143,8 @@ static void set_brightness_delayed(struct work_struct *ws)
 	if (ret == -ENOTSUPP)
 		ret = __led_set_brightness_blocking(led_cdev,
 					led_cdev->delayed_set_value);
+	__led_handle_regulator(led_cdev, led_cdev->delayed_set_value);
+
 	if (ret < 0 &&
 	    /* LED HW might have been unplugged, therefore don't warn */
 	    !(ret == -ENODEV && (led_cdev->flags & LED_UNREGISTERING) &&
@@ -141,6 +171,7 @@ static void led_set_software_blink(struct led_classdev *led_cdev,
 	/* never on - just set to off */
 	if (!delay_on) {
 		led_set_brightness_nosleep(led_cdev, LED_OFF);
+		__led_handle_regulator(led_cdev, LED_OFF);
 		return;
 	}
 
@@ -148,6 +179,7 @@ static void led_set_software_blink(struct led_classdev *led_cdev,
 	if (!delay_off) {
 		led_set_brightness_nosleep(led_cdev,
 					   led_cdev->blink_brightness);
+		__led_handle_regulator(led_cdev, led_cdev->blink_brightness);
 		return;
 	}
 
@@ -256,8 +288,14 @@ void led_set_brightness_nopm(struct led_classdev *led_cdev,
 			      enum led_brightness value)
 {
 	/* Use brightness_set op if available, it is guaranteed not to sleep */
-	if (!__led_set_brightness(led_cdev, value))
-		return;
+	if (!__led_set_brightness(led_cdev, value)) {
+		/*
+		 * if regulator state doesn't need to be changed, that is all/
+		 * Otherwise delegate the change to a work queue
+		 */
+		if (!__led_need_regulator_update(led_cdev, value))
+			return;
+	}
 
 	/* If brightness setting can sleep, delegate it to a work queue task */
 	led_cdev->delayed_set_value = value;
@@ -280,6 +318,8 @@ EXPORT_SYMBOL_GPL(led_set_brightness_nosleep);
 int led_set_brightness_sync(struct led_classdev *led_cdev,
 			    enum led_brightness value)
 {
+	int ret;
+
 	if (led_cdev->blink_delay_on || led_cdev->blink_delay_off)
 		return -EBUSY;
 
@@ -288,7 +328,11 @@ int led_set_brightness_sync(struct led_classdev *led_cdev,
 	if (led_cdev->flags & LED_SUSPENDED)
 		return 0;
 
-	return __led_set_brightness_blocking(led_cdev, led_cdev->brightness);
+	ret = __led_set_brightness_blocking(led_cdev, led_cdev->brightness);
+	if (ret)
+		return ret;
+
+	return __led_handle_regulator(led_cdev, led_cdev->brightness);
 }
 EXPORT_SYMBOL_GPL(led_set_brightness_sync);
 
diff --git a/drivers/leds/leds.h b/drivers/leds/leds.h
index 47b229469069..5aa5c038bd38 100644
--- a/drivers/leds/leds.h
+++ b/drivers/leds/leds.h
@@ -11,6 +11,7 @@
 
 #include <linux/rwsem.h>
 #include <linux/leds.h>
+#include <linux/regulator/consumer.h>
 
 static inline int led_get_brightness(struct led_classdev *led_cdev)
 {
diff --git a/include/linux/leds.h b/include/linux/leds.h
index 9b2bf574a17a..bee8e3f8dddd 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -123,6 +123,10 @@ struct led_classdev {
 
 	/* Ensures consistent access to the LED Flash Class device */
 	struct mutex		led_access;
+
+	/* regulator */
+	struct regulator	*regulator;
+	bool			regulator_state;
 };
 
 extern int of_led_classdev_register(struct device *parent,
-- 
2.17.1

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

[Dan Murphy]

JJ

On 7/15/19 10:56 AM, Jean-Jacques Hiblot wrote:
> A LED is usually powered by a voltage/current regulator. Let the LED core
> know about it. This allows the LED core to turn on or off the power supply
> as needed.

This allows the LED core to turn on or off managed power supplies.


>
> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
> ---
>   drivers/leds/led-class.c | 15 ++++++++++++
>   drivers/leds/led-core.c  | 50 +++++++++++++++++++++++++++++++++++++---
>   drivers/leds/leds.h      |  1 +
>   include/linux/leds.h     |  4 ++++
>   4 files changed, 67 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
> index 4793e77808e2..cadd43c30d50 100644
> --- a/drivers/leds/led-class.c
> +++ b/drivers/leds/led-class.c
> @@ -253,6 +253,7 @@ int of_led_classdev_register(struct device *parent, struct device_node *np,
>   {
>   	char name[LED_MAX_NAME_SIZE];
>   	int ret;
> +	struct regulator *regulator;
>   
>   	ret = led_classdev_next_name(led_cdev->name, name, sizeof(name));
>   	if (ret < 0)
> @@ -272,6 +273,20 @@ int of_led_classdev_register(struct device *parent, struct device_node *np,
>   		dev_warn(parent, "Led %s renamed to %s due to name collision",
>   				led_cdev->name, dev_name(led_cdev->dev));
>   
> +	regulator = devm_regulator_get_optional(led_cdev->dev, "power");
Store the regulator in  led_cdev->regulator and drop the else case below.
> +	if (IS_ERR(regulator)) {
> +		if (PTR_ERR(regulator) != -ENODEV) {
> +			dev_err(led_cdev->dev, "Cannot get the power supply for %s\n",
> +				led_cdev->name);
> +			device_unregister(led_cdev->dev);
> +			mutex_unlock(&led_cdev->led_access);
> +			return PTR_ERR(regulator);
> +		}
> +		led_cdev->regulator = NULL;
> +	} else {
> +		led_cdev->regulator = regulator;
> +	}
> +
>   	if (led_cdev->flags & LED_BRIGHT_HW_CHANGED) {
>   		ret = led_add_brightness_hw_changed(led_cdev);
>   		if (ret) {
> diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
> index 7107cd7e87cf..a12b880b0a2f 100644
> --- a/drivers/leds/led-core.c
> +++ b/drivers/leds/led-core.c
> @@ -23,6 +23,33 @@ EXPORT_SYMBOL_GPL(leds_list_lock);
>   LIST_HEAD(leds_list);
>   EXPORT_SYMBOL_GPL(leds_list);
>   
> +static bool __led_need_regulator_update(struct led_classdev *led_cdev,
> +					int brightness)
> +{
> +	bool new_state = (brightness != LED_OFF);
> +
> +	return led_cdev->regulator && led_cdev->regulator_state != new_state;
> +}
> +
> +static int __led_handle_regulator(struct led_classdev *led_cdev,
> +				int brightness)
> +{
> +	int rc;

Should there be a check for the regulator pointer.

If (!led_cdev->regulator)

     return 0;


Otherwise

Reviewed-by: Dan Murphy <dmurphy@ti.com>

<snip>

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

[Dan Murphy]

JJ

On 7/15/19 10:56 AM, Jean-Jacques Hiblot wrote:
> A LED is usually powered by a voltage/current regulator. Let the LED core
> know about it. This allows the LED core to turn on or off the power supply
> as needed.

This allows the LED core to turn on or off managed power supplies.


>
> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
> ---
>   drivers/leds/led-class.c | 15 ++++++++++++
>   drivers/leds/led-core.c  | 50 +++++++++++++++++++++++++++++++++++++---
>   drivers/leds/leds.h      |  1 +
>   include/linux/leds.h     |  4 ++++
>   4 files changed, 67 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
> index 4793e77808e2..cadd43c30d50 100644
> --- a/drivers/leds/led-class.c
> +++ b/drivers/leds/led-class.c
> @@ -253,6 +253,7 @@ int of_led_classdev_register(struct device *parent, struct device_node *np,
>   {
>   	char name[LED_MAX_NAME_SIZE];
>   	int ret;
> +	struct regulator *regulator;
>   
>   	ret = led_classdev_next_name(led_cdev->name, name, sizeof(name));
>   	if (ret < 0)
> @@ -272,6 +273,20 @@ int of_led_classdev_register(struct device *parent, struct device_node *np,
>   		dev_warn(parent, "Led %s renamed to %s due to name collision",
>   				led_cdev->name, dev_name(led_cdev->dev));
>   
> +	regulator = devm_regulator_get_optional(led_cdev->dev, "power");
Store the regulator in  led_cdev->regulator and drop the else case below.
> +	if (IS_ERR(regulator)) {
> +		if (PTR_ERR(regulator) != -ENODEV) {
> +			dev_err(led_cdev->dev, "Cannot get the power supply for %s\n",
> +				led_cdev->name);
> +			device_unregister(led_cdev->dev);
> +			mutex_unlock(&led_cdev->led_access);
> +			return PTR_ERR(regulator);
> +		}
> +		led_cdev->regulator = NULL;
> +	} else {
> +		led_cdev->regulator = regulator;
> +	}
> +
>   	if (led_cdev->flags & LED_BRIGHT_HW_CHANGED) {
>   		ret = led_add_brightness_hw_changed(led_cdev);
>   		if (ret) {
> diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
> index 7107cd7e87cf..a12b880b0a2f 100644
> --- a/drivers/leds/led-core.c
> +++ b/drivers/leds/led-core.c
> @@ -23,6 +23,33 @@ EXPORT_SYMBOL_GPL(leds_list_lock);
>   LIST_HEAD(leds_list);
>   EXPORT_SYMBOL_GPL(leds_list);
>   
> +static bool __led_need_regulator_update(struct led_classdev *led_cdev,
> +					int brightness)
> +{
> +	bool new_state = (brightness != LED_OFF);
> +
> +	return led_cdev->regulator && led_cdev->regulator_state != new_state;
> +}
> +
> +static int __led_handle_regulator(struct led_classdev *led_cdev,
> +				int brightness)
> +{
> +	int rc;

Should there be a check for the regulator pointer.

If (!led_cdev->regulator)

     return 0;


Otherwise

Reviewed-by: Dan Murphy <dmurphy@ti.com>

<snip>

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

[Jean-Jacques Hiblot]

Hi Dan,

On 15/07/2019 20:59, Dan Murphy wrote:
> JJ
>
> On 7/15/19 10:56 AM, Jean-Jacques Hiblot wrote:
>> A LED is usually powered by a voltage/current regulator. Let the LED 
>> core
>> know about it. This allows the LED core to turn on or off the power 
>> supply
>> as needed.
>
> This allows the LED core to turn on or off managed power supplies.
>
>
>>
>> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
>> ---

>>       if (led_cdev->flags & LED_BRIGHT_HW_CHANGED) {
>>           ret = led_add_brightness_hw_changed(led_cdev);
>>           if (ret) {
>> diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
>> index 7107cd7e87cf..a12b880b0a2f 100644
>> --- a/drivers/leds/led-core.c
>> +++ b/drivers/leds/led-core.c
>> @@ -23,6 +23,33 @@ EXPORT_SYMBOL_GPL(leds_list_lock);
>>   LIST_HEAD(leds_list);
>>   EXPORT_SYMBOL_GPL(leds_list);
>>   +static bool __led_need_regulator_update(struct led_classdev 
>> *led_cdev,
>> +                    int brightness)
>> +{
>> +    bool new_state = (brightness != LED_OFF);
>> +
>> +    return led_cdev->regulator && led_cdev->regulator_state != 
>> new_state;
>> +}
>> +
>> +static int __led_handle_regulator(struct led_classdev *led_cdev,
>> +                int brightness)
>> +{
>> +    int rc;
>
> Should there be a check for the regulator pointer.
>
> If (!led_cdev->regulator)
>
>     return 0;

Not required because __led_need_regulator_update() returns false if 
led_cdev->regulator is NULL.

Thanks for the review

JJ

>
>
> Otherwise
>
> Reviewed-by: Dan Murphy <dmurphy@ti.com>
>
> <snip>
>

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

[Jean-Jacques Hiblot]

Hi Dan,

On 15/07/2019 20:59, Dan Murphy wrote:
> JJ
>
> On 7/15/19 10:56 AM, Jean-Jacques Hiblot wrote:
>> A LED is usually powered by a voltage/current regulator. Let the LED 
>> core
>> know about it. This allows the LED core to turn on or off the power 
>> supply
>> as needed.
>
> This allows the LED core to turn on or off managed power supplies.
>
>
>>
>> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
>> ---

>>       if (led_cdev->flags & LED_BRIGHT_HW_CHANGED) {
>>           ret = led_add_brightness_hw_changed(led_cdev);
>>           if (ret) {
>> diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
>> index 7107cd7e87cf..a12b880b0a2f 100644
>> --- a/drivers/leds/led-core.c
>> +++ b/drivers/leds/led-core.c
>> @@ -23,6 +23,33 @@ EXPORT_SYMBOL_GPL(leds_list_lock);
>>   LIST_HEAD(leds_list);
>>   EXPORT_SYMBOL_GPL(leds_list);
>>   +static bool __led_need_regulator_update(struct led_classdev 
>> *led_cdev,
>> +                    int brightness)
>> +{
>> +    bool new_state = (brightness != LED_OFF);
>> +
>> +    return led_cdev->regulator && led_cdev->regulator_state != 
>> new_state;
>> +}
>> +
>> +static int __led_handle_regulator(struct led_classdev *led_cdev,
>> +                int brightness)
>> +{
>> +    int rc;
>
> Should there be a check for the regulator pointer.
>
> If (!led_cdev->regulator)
>
>     return 0;

Not required because __led_need_regulator_update() returns false if 
led_cdev->regulator is NULL.

Thanks for the review

JJ

>
>
> Otherwise
>
> Reviewed-by: Dan Murphy <dmurphy@ti.com>
>
> <snip>
>

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

[Jacek Anaszewski]

Hi Jean,

Thank you for the patch.

I have one issue. Please refer below.

On 7/15/19 5:56 PM, Jean-Jacques Hiblot wrote:
> A LED is usually powered by a voltage/current regulator. Let the LED core
> know about it. This allows the LED core to turn on or off the power supply
> as needed.
> 
> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
> ---
>  drivers/leds/led-class.c | 15 ++++++++++++
>  drivers/leds/led-core.c  | 50 +++++++++++++++++++++++++++++++++++++---
>  drivers/leds/leds.h      |  1 +
>  include/linux/leds.h     |  4 ++++
>  4 files changed, 67 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
> index 4793e77808e2..cadd43c30d50 100644
> --- a/drivers/leds/led-class.c
> +++ b/drivers/leds/led-class.c
> @@ -253,6 +253,7 @@ int of_led_classdev_register(struct device *parent, struct device_node *np,
>  {
>  	char name[LED_MAX_NAME_SIZE];
>  	int ret;
> +	struct regulator *regulator;
>  
>  	ret = led_classdev_next_name(led_cdev->name, name, sizeof(name));
>  	if (ret < 0)
> @@ -272,6 +273,20 @@ int of_led_classdev_register(struct device *parent, struct device_node *np,
>  		dev_warn(parent, "Led %s renamed to %s due to name collision",
>  				led_cdev->name, dev_name(led_cdev->dev));
>  
> +	regulator = devm_regulator_get_optional(led_cdev->dev, "power");
> +	if (IS_ERR(regulator)) {
> +		if (PTR_ERR(regulator) != -ENODEV) {
> +			dev_err(led_cdev->dev, "Cannot get the power supply for %s\n",
> +				led_cdev->name);
> +			device_unregister(led_cdev->dev);
> +			mutex_unlock(&led_cdev->led_access);
> +			return PTR_ERR(regulator);
> +		}
> +		led_cdev->regulator = NULL;
> +	} else {
> +		led_cdev->regulator = regulator;
> +	}
> +
>  	if (led_cdev->flags & LED_BRIGHT_HW_CHANGED) {
>  		ret = led_add_brightness_hw_changed(led_cdev);
>  		if (ret) {
> diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
> index 7107cd7e87cf..a12b880b0a2f 100644
> --- a/drivers/leds/led-core.c
> +++ b/drivers/leds/led-core.c
> @@ -23,6 +23,33 @@ EXPORT_SYMBOL_GPL(leds_list_lock);
>  LIST_HEAD(leds_list);
>  EXPORT_SYMBOL_GPL(leds_list);
>  
> +static bool __led_need_regulator_update(struct led_classdev *led_cdev,
> +					int brightness)
> +{
> +	bool new_state = (brightness != LED_OFF);
> +
> +	return led_cdev->regulator && led_cdev->regulator_state != new_state;
> +}
> +
> +static int __led_handle_regulator(struct led_classdev *led_cdev,
> +				int brightness)
> +{
> +	int rc;
> +
> +	if (__led_need_regulator_update(led_cdev, brightness)) {
> +
> +		if (brightness != LED_OFF)
> +			rc = regulator_enable(led_cdev->regulator);
> +		else
> +			rc = regulator_disable(led_cdev->regulator);
> +		if (rc)
> +			return rc;
> +
> +		led_cdev->regulator_state = (brightness != LED_OFF);
> +	}
> +	return 0;
> +}
> +
>  static int __led_set_brightness(struct led_classdev *led_cdev,
>  				enum led_brightness value)
>  {
> @@ -80,6 +107,7 @@ static void led_timer_function(struct timer_list *t)
>  	}
>  
>  	led_set_brightness_nosleep(led_cdev, brightness);
> +	__led_handle_regulator(led_cdev, brightness);

This cannot be called from atomic context since regulator_enable/disable
use mutex beneath, that can sleep on contention. Therefore this call
has to be made in two places instead:

__led_set_brightness()
__led_set_brightness_blocking()

>  
>  	/* Return in next iteration if led is in one-shot mode and we are in
>  	 * the final blink state so that the led is toggled each delay_on +
> @@ -115,6 +143,8 @@ static void set_brightness_delayed(struct work_struct *ws)
>  	if (ret == -ENOTSUPP)
>  		ret = __led_set_brightness_blocking(led_cdev,
>  					led_cdev->delayed_set_value);
> +	__led_handle_regulator(led_cdev, led_cdev->delayed_set_value);
> +
>  	if (ret < 0 &&
>  	    /* LED HW might have been unplugged, therefore don't warn */
>  	    !(ret == -ENODEV && (led_cdev->flags & LED_UNREGISTERING) &&
> @@ -141,6 +171,7 @@ static void led_set_software_blink(struct led_classdev *led_cdev,
>  	/* never on - just set to off */
>  	if (!delay_on) {
>  		led_set_brightness_nosleep(led_cdev, LED_OFF);
> +		__led_handle_regulator(led_cdev, LED_OFF);
>  		return;
>  	}
>  
> @@ -148,6 +179,7 @@ static void led_set_software_blink(struct led_classdev *led_cdev,
>  	if (!delay_off) {
>  		led_set_brightness_nosleep(led_cdev,
>  					   led_cdev->blink_brightness);
> +		__led_handle_regulator(led_cdev, led_cdev->blink_brightness);
>  		return;
>  	}
>  
> @@ -256,8 +288,14 @@ void led_set_brightness_nopm(struct led_classdev *led_cdev,
>  			      enum led_brightness value)
>  {
>  	/* Use brightness_set op if available, it is guaranteed not to sleep */
> -	if (!__led_set_brightness(led_cdev, value))
> -		return;
> +	if (!__led_set_brightness(led_cdev, value)) {
> +		/*
> +		 * if regulator state doesn't need to be changed, that is all/
> +		 * Otherwise delegate the change to a work queue
> +		 */
> +		if (!__led_need_regulator_update(led_cdev, value))
> +			return;
> +	}
>  
>  	/* If brightness setting can sleep, delegate it to a work queue task */
>  	led_cdev->delayed_set_value = value;
> @@ -280,6 +318,8 @@ EXPORT_SYMBOL_GPL(led_set_brightness_nosleep);
>  int led_set_brightness_sync(struct led_classdev *led_cdev,
>  			    enum led_brightness value)
>  {
> +	int ret;
> +
>  	if (led_cdev->blink_delay_on || led_cdev->blink_delay_off)
>  		return -EBUSY;
>  
> @@ -288,7 +328,11 @@ int led_set_brightness_sync(struct led_classdev *led_cdev,
>  	if (led_cdev->flags & LED_SUSPENDED)
>  		return 0;
>  
> -	return __led_set_brightness_blocking(led_cdev, led_cdev->brightness);
> +	ret = __led_set_brightness_blocking(led_cdev, led_cdev->brightness);
> +	if (ret)
> +		return ret;
> +
> +	return __led_handle_regulator(led_cdev, led_cdev->brightness);
>  }
>  EXPORT_SYMBOL_GPL(led_set_brightness_sync);
>  
> diff --git a/drivers/leds/leds.h b/drivers/leds/leds.h
> index 47b229469069..5aa5c038bd38 100644
> --- a/drivers/leds/leds.h
> +++ b/drivers/leds/leds.h
> @@ -11,6 +11,7 @@
>  
>  #include <linux/rwsem.h>
>  #include <linux/leds.h>
> +#include <linux/regulator/consumer.h>
>  
>  static inline int led_get_brightness(struct led_classdev *led_cdev)
>  {
> diff --git a/include/linux/leds.h b/include/linux/leds.h
> index 9b2bf574a17a..bee8e3f8dddd 100644
> --- a/include/linux/leds.h
> +++ b/include/linux/leds.h
> @@ -123,6 +123,10 @@ struct led_classdev {
>  
>  	/* Ensures consistent access to the LED Flash Class device */
>  	struct mutex		led_access;
> +
> +	/* regulator */
> +	struct regulator	*regulator;
> +	bool			regulator_state;
>  };
>  
>  extern int of_led_classdev_register(struct device *parent,
> 

-- 
Best regards,
Jacek Anaszewski

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

[Jean-Jacques Hiblot]

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

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

[Daniel Thompson]

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.

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

[Jean-Jacques Hiblot]

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.

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

[Jean-Jacques Hiblot]

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.