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

[PATCH v5 0/3] 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.
Because updating a regulator state can block, it is always defered to
set_brightness_delayed().

changes in v5:
- fixed build error in led_set_brightness_sync(). Explain the role of
  flush__work()

changes in v4:
- Add a new patch to make led_set_brightness_sync() use
  led_set_brightness_nosleep() and then wait the work to be done
- Rework how the core knows how the regulator needs to be updated.

changes in v3:
- reword device-tree description
- reword commit log
- remove regulator updates from functions used in atomic context. If the
  regulator must be updated, it is defered to a workqueue.
- Fix led_set_brightness_sync() to work with the non-blocking function
  __led_set_brightness()

changes in v2:
- use devm_regulator_get_optional() to avoid using the dummy regulator and
  do some unnecessary work

Jean-Jacques Hiblot (3):
  led: make led_set_brightness_sync() use led_set_brightness_nosleep()
  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                      | 17 ++++
 drivers/leds/led-core.c                       | 78 +++++++++++++++++--
 drivers/leds/leds.h                           |  3 +
 include/linux/leds.h                          |  5 ++
 5 files changed, 101 insertions(+), 8 deletions(-)

-- 
2.17.1

[PATCH v5 1/3] led: make led_set_brightness_sync() use led_set_brightness_nosleep()

[Jean-Jacques Hiblot]

Making led_set_brightness_sync() use led_set_brightness_nosleep() has 2
advantages:
- works for LED controllers that do not provide brightness_set_blocking()
- When the blocking callback is used, it uses the workqueue to update the
  LED state, removing the need for mutual exclusion between
  led_set_brightness_sync() and set_brightness_delayed().

Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
---
 drivers/leds/led-core.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
index f1f718dbe0f8..d318f9b0382d 100644
--- a/drivers/leds/led-core.c
+++ b/drivers/leds/led-core.c
@@ -297,12 +297,13 @@ int led_set_brightness_sync(struct led_classdev *led_cdev,
 	if (led_cdev->blink_delay_on || led_cdev->blink_delay_off)
 		return -EBUSY;
 
-	led_cdev->brightness = min(value, led_cdev->max_brightness);
-
-	if (led_cdev->flags & LED_SUSPENDED)
-		return 0;
-
-	return __led_set_brightness_blocking(led_cdev, led_cdev->brightness);
+	led_set_brightness_nosleep(led_cdev, value);
+	/* 
+	 * led_set_brightness_nosleep() may have deferred some work.
+	 * Wait until it is complete.
+	 */
+	flush_work(&led_cdev->set_brightness_work);
+	return 0;
 }
 EXPORT_SYMBOL_GPL(led_set_brightness_sync);
 
-- 
2.17.1

[PATCH v5 2/3] 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 9fa6f9795d50..54857c16573d 100644
--- a/Documentation/devicetree/bindings/leds/common.txt
+++ b/Documentation/devicetree/bindings/leds/common.txt
@@ -77,6 +77,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
@@ -124,6 +129,7 @@ led-controller@0 {
 		function = LED_FUNCTION_STATUS;
 		linux,default-trigger = "heartbeat";
 		gpios = <&gpio0 0 GPIO_ACTIVE_HIGH>;
+		power-supply = <&led_regulator>;
 	};
 
 	led1 {
-- 
2.17.1

[PATCH v5 3/3] 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 | 17 +++++++++++
 drivers/leds/led-core.c  | 65 ++++++++++++++++++++++++++++++++++++++--
 drivers/leds/leds.h      |  3 ++
 include/linux/leds.h     |  5 ++++
 4 files changed, 88 insertions(+), 2 deletions(-)

diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
index e11177d77b4c..d122b6982efd 100644
--- a/drivers/leds/led-class.c
+++ b/drivers/leds/led-class.c
@@ -352,6 +352,7 @@ int led_classdev_register_ext(struct device *parent,
 	char final_name[LED_MAX_NAME_SIZE];
 	const char *proposed_name = composed_name;
 	int ret;
+	struct regulator *regulator;
 
 	if (init_data) {
 		if (init_data->devname_mandatory && !init_data->devicename) {
@@ -387,6 +388,22 @@ int led_classdev_register_ext(struct device *parent,
 		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 (regulator != ERR_PTR(-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;
+		led_cdev->regulator_state = REG_OFF;
+		atomic_set(&led_cdev->target_regulator_state, REG_UNKNOWN);
+	}
+
 	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 d318f9b0382d..155a158c7b8d 100644
--- a/drivers/leds/led-core.c
+++ b/drivers/leds/led-core.c
@@ -37,6 +37,43 @@ const char * const led_colors[LED_COLOR_ID_MAX] = {
 };
 EXPORT_SYMBOL_GPL(led_colors);
 
+static int __led_handle_regulator(struct led_classdev *led_cdev)
+{
+	int rc;
+	int target_state = led_cdev->delayed_set_value ?  REG_ON : REG_OFF;
+
+	if (!led_cdev->regulator)
+		return 0;
+
+	/*
+	 * if the current regulator state is not the target state, we
+	 * need to update it.
+	 * note: No need for spinlock or atomic here because
+	 * led_cdev->regulator_state is modified only in the context of
+	 * the worqueue
+	 */
+	if (led_cdev->regulator_state != target_state) {
+
+		if (target_state == REG_ON)
+			rc = regulator_enable(led_cdev->regulator);
+		else
+			rc = regulator_disable(led_cdev->regulator);
+		if (rc) {
+			/*
+			 * If something went wrong with the regulator update,
+			 * Make sure that led_set_brightness_nosleep() assume
+			 * that the regultor is in the right state.
+			 */
+			atomic_set(&led_cdev->target_regulator_state,
+				   REG_UNKNOWN);
+			return rc;
+		}
+
+		led_cdev->regulator_state = target_state;
+	}
+	return 0;
+}
+
 static int __led_set_brightness(struct led_classdev *led_cdev,
 				enum led_brightness value)
 {
@@ -135,6 +172,11 @@ static void set_brightness_delayed(struct work_struct *ws)
 	    (led_cdev->flags & LED_HW_PLUGGABLE)))
 		dev_err(led_cdev->dev,
 			"Setting an LED's brightness failed (%d)\n", ret);
+
+	ret = __led_handle_regulator(led_cdev);
+	if (ret)
+		dev_err(led_cdev->dev,
+			"Updating regulator state failed (%d)\n", ret);
 }
 
 static void led_set_software_blink(struct led_classdev *led_cdev,
@@ -269,8 +311,27 @@ EXPORT_SYMBOL_GPL(led_set_brightness);
 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))
+	bool update_regulator = false;
+	int old, new;
+
+	if (led_cdev->regulator) {
+		/*
+		 * Check if the regulator need to be updated.
+		 * We use an atomic here because multiple threads could
+		 * be calling this function at the same time. Using
+		 * atomic_xchg() ensures the consistency between
+		 * target_regulator_state, value and update_regulator
+		 */
+		new = !!value;
+		old = atomic_xchg(&led_cdev->target_regulator_state, new);
+		update_regulator = (old != new);
+	}
+
+	/*
+	 * If regulator state doesn't need to be changed, use brightness_set
+	 * op if available, it is guaranteed not to sleep
+	 */
+	if (!update_regulator && !__led_set_brightness(led_cdev, value))
 		return;
 
 	/* If brightness setting can sleep, delegate it to a work queue task */
diff --git a/drivers/leds/leds.h b/drivers/leds/leds.h
index 0b577cece8f7..02f261ce77f2 100644
--- a/drivers/leds/leds.h
+++ b/drivers/leds/leds.h
@@ -11,6 +11,9 @@
 
 #include <linux/rwsem.h>
 #include <linux/leds.h>
+#include <linux/regulator/consumer.h>
+
+enum { REG_OFF = 0, REG_ON, REG_UNKNOWN };
 
 static inline int led_get_brightness(struct led_classdev *led_cdev)
 {
diff --git a/include/linux/leds.h b/include/linux/leds.h
index 88bf2ceaabe6..8ce7cf937192 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -149,6 +149,11 @@ struct led_classdev {
 
 	/* Ensures consistent access to the LED Flash Class device */
 	struct mutex		led_access;
+
+	/* regulator */
+	struct regulator	*regulator;
+	int			regulator_state;
+	atomic_t		target_regulator_state;
 };
 
 /**
-- 
2.17.1

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

[Jacek Anaszewski]

Hi Jean,

Thank you for the patch.

I must say I'm not a big fan of this change.
It adds a bunch of code to the LED core and gives small
functionality in a reward. It may also influence maximum
software blinking rate, so I'd rather avoid calling
regulator_enable/disable when timer trigger is set.

It will of course require more code.

Since AFAIR Pavel was original proponent of this change then
I'd like to see his opinion before we move on to discussing
possible improvements to this patch.

Best regards,
Jacek Anaszewski

On 9/23/19 12:20 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 | 17 +++++++++++
>  drivers/leds/led-core.c  | 65 ++++++++++++++++++++++++++++++++++++++--
>  drivers/leds/leds.h      |  3 ++
>  include/linux/leds.h     |  5 ++++
>  4 files changed, 88 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
> index e11177d77b4c..d122b6982efd 100644
> --- a/drivers/leds/led-class.c
> +++ b/drivers/leds/led-class.c
> @@ -352,6 +352,7 @@ int led_classdev_register_ext(struct device *parent,
>  	char final_name[LED_MAX_NAME_SIZE];
>  	const char *proposed_name = composed_name;
>  	int ret;
> +	struct regulator *regulator;
>  
>  	if (init_data) {
>  		if (init_data->devname_mandatory && !init_data->devicename) {
> @@ -387,6 +388,22 @@ int led_classdev_register_ext(struct device *parent,
>  		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 (regulator != ERR_PTR(-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;
> +		led_cdev->regulator_state = REG_OFF;
> +		atomic_set(&led_cdev->target_regulator_state, REG_UNKNOWN);
> +	}
> +
>  	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 d318f9b0382d..155a158c7b8d 100644
> --- a/drivers/leds/led-core.c
> +++ b/drivers/leds/led-core.c
> @@ -37,6 +37,43 @@ const char * const led_colors[LED_COLOR_ID_MAX] = {
>  };
>  EXPORT_SYMBOL_GPL(led_colors);
>  
> +static int __led_handle_regulator(struct led_classdev *led_cdev)
> +{
> +	int rc;
> +	int target_state = led_cdev->delayed_set_value ?  REG_ON : REG_OFF;
> +
> +	if (!led_cdev->regulator)
> +		return 0;
> +
> +	/*
> +	 * if the current regulator state is not the target state, we
> +	 * need to update it.
> +	 * note: No need for spinlock or atomic here because
> +	 * led_cdev->regulator_state is modified only in the context of
> +	 * the worqueue
> +	 */
> +	if (led_cdev->regulator_state != target_state) {
> +
> +		if (target_state == REG_ON)
> +			rc = regulator_enable(led_cdev->regulator);
> +		else
> +			rc = regulator_disable(led_cdev->regulator);
> +		if (rc) {
> +			/*
> +			 * If something went wrong with the regulator update,
> +			 * Make sure that led_set_brightness_nosleep() assume
> +			 * that the regultor is in the right state.
> +			 */
> +			atomic_set(&led_cdev->target_regulator_state,
> +				   REG_UNKNOWN);
> +			return rc;
> +		}
> +
> +		led_cdev->regulator_state = target_state;
> +	}
> +	return 0;
> +}
> +
>  static int __led_set_brightness(struct led_classdev *led_cdev,
>  				enum led_brightness value)
>  {
> @@ -135,6 +172,11 @@ static void set_brightness_delayed(struct work_struct *ws)
>  	    (led_cdev->flags & LED_HW_PLUGGABLE)))
>  		dev_err(led_cdev->dev,
>  			"Setting an LED's brightness failed (%d)\n", ret);
> +
> +	ret = __led_handle_regulator(led_cdev);
> +	if (ret)
> +		dev_err(led_cdev->dev,
> +			"Updating regulator state failed (%d)\n", ret);
>  }
>  
>  static void led_set_software_blink(struct led_classdev *led_cdev,
> @@ -269,8 +311,27 @@ EXPORT_SYMBOL_GPL(led_set_brightness);
>  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))
> +	bool update_regulator = false;
> +	int old, new;
> +
> +	if (led_cdev->regulator) {
> +		/*
> +		 * Check if the regulator need to be updated.
> +		 * We use an atomic here because multiple threads could
> +		 * be calling this function at the same time. Using
> +		 * atomic_xchg() ensures the consistency between
> +		 * target_regulator_state, value and update_regulator
> +		 */
> +		new = !!value;
> +		old = atomic_xchg(&led_cdev->target_regulator_state, new);
> +		update_regulator = (old != new);
> +	}
> +
> +	/*
> +	 * If regulator state doesn't need to be changed, use brightness_set
> +	 * op if available, it is guaranteed not to sleep
> +	 */
> +	if (!update_regulator && !__led_set_brightness(led_cdev, value))
>  		return;
>  
>  	/* If brightness setting can sleep, delegate it to a work queue task */
> diff --git a/drivers/leds/leds.h b/drivers/leds/leds.h
> index 0b577cece8f7..02f261ce77f2 100644
> --- a/drivers/leds/leds.h
> +++ b/drivers/leds/leds.h
> @@ -11,6 +11,9 @@
>  
>  #include <linux/rwsem.h>
>  #include <linux/leds.h>
> +#include <linux/regulator/consumer.h>
> +
> +enum { REG_OFF = 0, REG_ON, REG_UNKNOWN };
>  
>  static inline int led_get_brightness(struct led_classdev *led_cdev)
>  {
> diff --git a/include/linux/leds.h b/include/linux/leds.h
> index 88bf2ceaabe6..8ce7cf937192 100644
> --- a/include/linux/leds.h
> +++ b/include/linux/leds.h
> @@ -149,6 +149,11 @@ struct led_classdev {
>  
>  	/* Ensures consistent access to the LED Flash Class device */
>  	struct mutex		led_access;
> +
> +	/* regulator */
> +	struct regulator	*regulator;
> +	int			regulator_state;
> +	atomic_t		target_regulator_state;
>  };
>  
>  /**
> 

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

[Jean-Jacques Hiblot]

On 24/09/2019 20:58, Jacek Anaszewski wrote:
> Hi Jean,
>
> Thank you for the patch.
>
> I must say I'm not a big fan of this change.
> It adds a bunch of code to the LED core and gives small
> functionality in a reward.

I disagree. I remember having to tweak DTS in the past to force some 
regulators outputs, and then they would be always turned on. If all 
sub-systems were power-supply/regulator-aware, that kind of hack would 
not be needed.


>   It may also influence maximum
> software blinking rate, so I'd rather avoid calling
> regulator_enable/disable when timer trigger is set.
>
> It will of course require more code.

True. We might be able to mitigate this by delaying turning off the 
regulator; Turning it on would be an immediate action, turning it off 
would be delayed by a configurable amount of ms. That should take care 
of the max blinking rate and reduce the overhead for a LED that changes 
states quite often (like a mass storage indicator)

JJ

>
> Since AFAIR Pavel was original proponent of this change then
> I'd like to see his opinion before we move on to discussing
> possible improvements to this patch.
>
> Best regards,
> Jacek Anaszewski
>
> On 9/23/19 12:20 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 | 17 +++++++++++
>>   drivers/leds/led-core.c  | 65 ++++++++++++++++++++++++++++++++++++++--
>>   drivers/leds/leds.h      |  3 ++
>>   include/linux/leds.h     |  5 ++++
>>   4 files changed, 88 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
>> index e11177d77b4c..d122b6982efd 100644
>> --- a/drivers/leds/led-class.c
>> +++ b/drivers/leds/led-class.c
>> @@ -352,6 +352,7 @@ int led_classdev_register_ext(struct device *parent,
>>   	char final_name[LED_MAX_NAME_SIZE];
>>   	const char *proposed_name = composed_name;
>>   	int ret;
>> +	struct regulator *regulator;
>>   
>>   	if (init_data) {
>>   		if (init_data->devname_mandatory && !init_data->devicename) {
>> @@ -387,6 +388,22 @@ int led_classdev_register_ext(struct device *parent,
>>   		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 (regulator != ERR_PTR(-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;
>> +		led_cdev->regulator_state = REG_OFF;
>> +		atomic_set(&led_cdev->target_regulator_state, REG_UNKNOWN);
>> +	}
>> +
>>   	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 d318f9b0382d..155a158c7b8d 100644
>> --- a/drivers/leds/led-core.c
>> +++ b/drivers/leds/led-core.c
>> @@ -37,6 +37,43 @@ const char * const led_colors[LED_COLOR_ID_MAX] = {
>>   };
>>   EXPORT_SYMBOL_GPL(led_colors);
>>   
>> +static int __led_handle_regulator(struct led_classdev *led_cdev)
>> +{
>> +	int rc;
>> +	int target_state = led_cdev->delayed_set_value ?  REG_ON : REG_OFF;
>> +
>> +	if (!led_cdev->regulator)
>> +		return 0;
>> +
>> +	/*
>> +	 * if the current regulator state is not the target state, we
>> +	 * need to update it.
>> +	 * note: No need for spinlock or atomic here because
>> +	 * led_cdev->regulator_state is modified only in the context of
>> +	 * the worqueue
>> +	 */
>> +	if (led_cdev->regulator_state != target_state) {
>> +
>> +		if (target_state == REG_ON)
>> +			rc = regulator_enable(led_cdev->regulator);
>> +		else
>> +			rc = regulator_disable(led_cdev->regulator);
>> +		if (rc) {
>> +			/*
>> +			 * If something went wrong with the regulator update,
>> +			 * Make sure that led_set_brightness_nosleep() assume
>> +			 * that the regultor is in the right state.
>> +			 */
>> +			atomic_set(&led_cdev->target_regulator_state,
>> +				   REG_UNKNOWN);
>> +			return rc;
>> +		}
>> +
>> +		led_cdev->regulator_state = target_state;
>> +	}
>> +	return 0;
>> +}
>> +
>>   static int __led_set_brightness(struct led_classdev *led_cdev,
>>   				enum led_brightness value)
>>   {
>> @@ -135,6 +172,11 @@ static void set_brightness_delayed(struct work_struct *ws)
>>   	    (led_cdev->flags & LED_HW_PLUGGABLE)))
>>   		dev_err(led_cdev->dev,
>>   			"Setting an LED's brightness failed (%d)\n", ret);
>> +
>> +	ret = __led_handle_regulator(led_cdev);
>> +	if (ret)
>> +		dev_err(led_cdev->dev,
>> +			"Updating regulator state failed (%d)\n", ret);
>>   }
>>   
>>   static void led_set_software_blink(struct led_classdev *led_cdev,
>> @@ -269,8 +311,27 @@ EXPORT_SYMBOL_GPL(led_set_brightness);
>>   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))
>> +	bool update_regulator = false;
>> +	int old, new;
>> +
>> +	if (led_cdev->regulator) {
>> +		/*
>> +		 * Check if the regulator need to be updated.
>> +		 * We use an atomic here because multiple threads could
>> +		 * be calling this function at the same time. Using
>> +		 * atomic_xchg() ensures the consistency between
>> +		 * target_regulator_state, value and update_regulator
>> +		 */
>> +		new = !!value;
>> +		old = atomic_xchg(&led_cdev->target_regulator_state, new);
>> +		update_regulator = (old != new);
>> +	}
>> +
>> +	/*
>> +	 * If regulator state doesn't need to be changed, use brightness_set
>> +	 * op if available, it is guaranteed not to sleep
>> +	 */
>> +	if (!update_regulator && !__led_set_brightness(led_cdev, value))
>>   		return;
>>   
>>   	/* If brightness setting can sleep, delegate it to a work queue task */
>> diff --git a/drivers/leds/leds.h b/drivers/leds/leds.h
>> index 0b577cece8f7..02f261ce77f2 100644
>> --- a/drivers/leds/leds.h
>> +++ b/drivers/leds/leds.h
>> @@ -11,6 +11,9 @@
>>   
>>   #include <linux/rwsem.h>
>>   #include <linux/leds.h>
>> +#include <linux/regulator/consumer.h>
>> +
>> +enum { REG_OFF = 0, REG_ON, REG_UNKNOWN };
>>   
>>   static inline int led_get_brightness(struct led_classdev *led_cdev)
>>   {
>> diff --git a/include/linux/leds.h b/include/linux/leds.h
>> index 88bf2ceaabe6..8ce7cf937192 100644
>> --- a/include/linux/leds.h
>> +++ b/include/linux/leds.h
>> @@ -149,6 +149,11 @@ struct led_classdev {
>>   
>>   	/* Ensures consistent access to the LED Flash Class device */
>>   	struct mutex		led_access;
>> +
>> +	/* regulator */
>> +	struct regulator	*regulator;
>> +	int			regulator_state;
>> +	atomic_t		target_regulator_state;
>>   };
>>   
>>   /**
>>

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

[Pavel Machek]

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

Hi!

> I must say I'm not a big fan of this change.
> It adds a bunch of code to the LED core and gives small
> functionality in a reward. It may also influence maximum
> software blinking rate, so I'd rather avoid calling
> regulator_enable/disable when timer trigger is set.
> 
> It will of course require more code.
> 
> Since AFAIR Pavel was original proponent of this change then
> I'd like to see his opinion before we move on to discussing
> possible improvements to this patch.

Was I?

Okay, this series looks quite confusing to me. First, 1/3 looks
"interesting" (would have to analyze it way more).

Second, 3/3... So we have a LED driver _and_ a regulator? So yes, the
chip driving a LED is usually ... voltage/current regulator. What is
second regulator doing there? Is that a common setup?

Best regards,
								Pavel
								
-- 
(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 --]

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

[Jacek Anaszewski]

On 10/13/19 2:09 PM, Pavel Machek wrote:
> Hi!
> 
>> I must say I'm not a big fan of this change.
>> It adds a bunch of code to the LED core and gives small
>> functionality in a reward. It may also influence maximum
>> software blinking rate, so I'd rather avoid calling
>> regulator_enable/disable when timer trigger is set.
>>
>> It will of course require more code.
>>
>> Since AFAIR Pavel was original proponent of this change then
>> I'd like to see his opinion before we move on to discussing
>> possible improvements to this patch.
> 
> Was I?

See [0]:

"For the record, I also believe regulator and enable gpio should be
handled in the core."

> Okay, this series looks quite confusing to me. First, 1/3 looks
> "interesting" (would have to analyze it way more).
> 
> Second, 3/3... So we have a LED driver _and_ a regulator? So yes, the
> chip driving a LED is usually ... voltage/current regulator. What is
> second regulator doing there? Is that a common setup?
> 
> Best regards,
> 								Pavel
> 								
> 

[0]
https://lore.kernel.org/linux-leds/20190705100851.zn2jkipj4fxq5we6@devuan/

-- 
Best regards,
Jacek Anaszewski

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

[Jean-Jacques Hiblot]

On 13/10/2019 14:09, Pavel Machek wrote:
> Hi!
>
>> I must say I'm not a big fan of this change.
>> It adds a bunch of code to the LED core and gives small
>> functionality in a reward. It may also influence maximum
>> software blinking rate, so I'd rather avoid calling
>> regulator_enable/disable when timer trigger is set.
>>
>> It will of course require more code.
>>
>> Since AFAIR Pavel was original proponent of this change then
>> I'd like to see his opinion before we move on to discussing
>> possible improvements to this patch.
> Was I?
>
> Okay, this series looks quite confusing to me. First, 1/3 looks
> "interesting" (would have to analyze it way more).
>
> Second, 3/3... So we have a LED driver _and_ a regulator? So yes, the
> chip driving a LED is usually ... voltage/current regulator. What is
> second regulator doing there? Is that a common setup?

This is quite common with current-sink LED drivers.

The setup looks like this:

+-----------+
|           |
| Regulator |
|           +------------------------+
|           |                        |
+-----------+                      __|__
                                    \   /
          +---------------------+    \ / led
          |                     |     V
          |    Led Driver       |   --+--
          |                     |     |
          |                     |     |
          |                +----------+
          |              \      |
          |               \     |
          |                +    |
          |                |    |
          +---------------------+
                           |
                        +--+--+
                        ///////


Only the regulator usually does not supply only one LED.

JJ


>
> Best regards,
> 								Pavel
> 								

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

[Daniel Thompson]

On Mon, Oct 14, 2019 at 12:49:07PM +0200, Jean-Jacques Hiblot wrote:
> 
> On 13/10/2019 14:09, Pavel Machek wrote:
> > Hi!
> > 
> > > I must say I'm not a big fan of this change.
> > > It adds a bunch of code to the LED core and gives small
> > > functionality in a reward. It may also influence maximum
> > > software blinking rate, so I'd rather avoid calling
> > > regulator_enable/disable when timer trigger is set.
> > > 
> > > It will of course require more code.
> > > 
> > > Since AFAIR Pavel was original proponent of this change then
> > > I'd like to see his opinion before we move on to discussing
> > > possible improvements to this patch.
> > Was I?
> > 
> > Okay, this series looks quite confusing to me. First, 1/3 looks
> > "interesting" (would have to analyze it way more).
> > 
> > Second, 3/3... So we have a LED driver _and_ a regulator? So yes, the
> > chip driving a LED is usually ... voltage/current regulator. What is
> > second regulator doing there? Is that a common setup?
> 
> This is quite common with current-sink LED drivers.
> 
> The setup looks like this:
> 
> +-----------+
> |           |
> | Regulator |
> |           +------------------------+
> |           |                        |
> +-----------+                      __|__
>                                    \   /
>          +---------------------+    \ / led
>          |                     |     V
>          |    Led Driver       |   --+--
>          |                     |     |
>          |                     |     |
>          |                +----------+
>          |              \      |
>          |               \     |
>          |                +    |
>          |                |    |
>          +---------------------+
>                           |
>                        +--+--+
>                        ///////
> 
> 
> Only the regulator usually does not supply only one LED.

Given questions have been raised about the complexity of the change I
wondered whether, for a system with this topology, the regulator
could/should be enabled when the LED driver driver probes?


Daniel.

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

[Jacek Anaszewski]

On 10/14/19 2:38 PM, Daniel Thompson wrote:
> On Mon, Oct 14, 2019 at 12:49:07PM +0200, Jean-Jacques Hiblot wrote:
>>
>> On 13/10/2019 14:09, Pavel Machek wrote:
>>> Hi!
>>>
>>>> I must say I'm not a big fan of this change.
>>>> It adds a bunch of code to the LED core and gives small
>>>> functionality in a reward. It may also influence maximum
>>>> software blinking rate, so I'd rather avoid calling
>>>> regulator_enable/disable when timer trigger is set.
>>>>
>>>> It will of course require more code.
>>>>
>>>> Since AFAIR Pavel was original proponent of this change then
>>>> I'd like to see his opinion before we move on to discussing
>>>> possible improvements to this patch.
>>> Was I?
>>>
>>> Okay, this series looks quite confusing to me. First, 1/3 looks
>>> "interesting" (would have to analyze it way more).
>>>
>>> Second, 3/3... So we have a LED driver _and_ a regulator? So yes, the
>>> chip driving a LED is usually ... voltage/current regulator. What is
>>> second regulator doing there? Is that a common setup?
>>
>> This is quite common with current-sink LED drivers.
>>
>> The setup looks like this:
>>
>> +-----------+
>> |           |
>> | Regulator |
>> |           +------------------------+
>> |           |                        |
>> +-----------+                      __|__
>>                                    \   /
>>          +---------------------+    \ / led
>>          |                     |     V
>>          |    Led Driver       |   --+--
>>          |                     |     |
>>          |                     |     |
>>          |                +----------+
>>          |              \      |
>>          |               \     |
>>          |                +    |
>>          |                |    |
>>          +---------------------+
>>                           |
>>                        +--+--+
>>                        ///////
>>
>>
>> Only the regulator usually does not supply only one LED.
> 
> Given questions have been raised about the complexity of the change I
> wondered whether, for a system with this topology, the regulator
> could/should be enabled when the LED driver driver probes?

And this is how are doing that now.

Moreover, just after seeing your ASCII art it has become obvious to me
that we can't simply do regulator_disable() while setting brightness to
LED_OFF since it can result in powering off the LED controller, which
would need to be reconfigured on next transition to
brightness > LED_OFF.

It looks that the idea is flawed I'm afraid.

-- 
Best regards,
Jacek Anaszewski

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

[Jean-Jacques Hiblot]

On 14/10/2019 20:48, Jacek Anaszewski wrote:
> On 10/14/19 2:38 PM, Daniel Thompson wrote:
>> On Mon, Oct 14, 2019 at 12:49:07PM +0200, Jean-Jacques Hiblot wrote:
>>> On 13/10/2019 14:09, Pavel Machek wrote:
>>>> Hi!
>>>>
>>>>> I must say I'm not a big fan of this change.
>>>>> It adds a bunch of code to the LED core and gives small
>>>>> functionality in a reward. It may also influence maximum
>>>>> software blinking rate, so I'd rather avoid calling
>>>>> regulator_enable/disable when timer trigger is set.
>>>>>
>>>>> It will of course require more code.
>>>>>
>>>>> Since AFAIR Pavel was original proponent of this change then
>>>>> I'd like to see his opinion before we move on to discussing
>>>>> possible improvements to this patch.
>>>> Was I?
>>>>
>>>> Okay, this series looks quite confusing to me. First, 1/3 looks
>>>> "interesting" (would have to analyze it way more).
>>>>
>>>> Second, 3/3... So we have a LED driver _and_ a regulator? So yes, the
>>>> chip driving a LED is usually ... voltage/current regulator. What is
>>>> second regulator doing there? Is that a common setup?
>>> This is quite common with current-sink LED drivers.
>>>
>>> The setup looks like this:
>>>
>>> +-----------+
>>> |           |
>>> | Regulator |
>>> |           +------------------------+
>>> |           |                        |
>>> +-----------+                      __|__
>>>                                     \   /
>>>           +---------------------+    \ / led
>>>           |                     |     V
>>>           |    Led Driver       |   --+--
>>>           |                     |     |
>>>           |                     |     |
>>>           |                +----------+
>>>           |              \      |
>>>           |               \     |
>>>           |                +    |
>>>           |                |    |
>>>           +---------------------+
>>>                            |
>>>                         +--+--+
>>>                         ///////
>>>
>>>
>>> Only the regulator usually does not supply only one LED.
>> Given questions have been raised about the complexity of the change I
>> wondered whether, for a system with this topology, the regulator
>> could/should be enabled when the LED driver driver probes?
> And this is how are doing that now.
>
> Moreover, just after seeing your ASCII art it has become obvious to me
> that we can't simply do regulator_disable() while setting brightness to
> LED_OFF since it can result in powering off the LED controller, which
> would need to be reconfigured on next transition to
> brightness > LED_OFF.

That is a problem only if the LED driver is powered by the same 
regulator, which is not the case in the diagram.

This series make sense only for boards where LEDs have a dedicated 
voltage rail or can be modeled this way.

My use case is a LED panel driven by a LED current-sink controller that 
uses both a PWM-style control for brightness AND a active-low enable 
pin. If the enable pin is not HIGH, the panel is never completely dark 
even if the LED brightness is set to 0. I modeled this switch with a 
fixed-voltage regulator  which is a common way of doing it (it is after 
all how this things is done inside the panel).

JJ
> It looks that the idea is flawed I'm afraid.

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

[Jacek Anaszewski]

On 10/16/19 10:13 AM, Jean-Jacques Hiblot wrote:
> 
> On 14/10/2019 20:48, Jacek Anaszewski wrote:
>> On 10/14/19 2:38 PM, Daniel Thompson wrote:
>>> On Mon, Oct 14, 2019 at 12:49:07PM +0200, Jean-Jacques Hiblot wrote:
>>>> On 13/10/2019 14:09, Pavel Machek wrote:
>>>>> Hi!
>>>>>
>>>>>> I must say I'm not a big fan of this change.
>>>>>> It adds a bunch of code to the LED core and gives small
>>>>>> functionality in a reward. It may also influence maximum
>>>>>> software blinking rate, so I'd rather avoid calling
>>>>>> regulator_enable/disable when timer trigger is set.
>>>>>>
>>>>>> It will of course require more code.
>>>>>>
>>>>>> Since AFAIR Pavel was original proponent of this change then
>>>>>> I'd like to see his opinion before we move on to discussing
>>>>>> possible improvements to this patch.
>>>>> Was I?
>>>>>
>>>>> Okay, this series looks quite confusing to me. First, 1/3 looks
>>>>> "interesting" (would have to analyze it way more).
>>>>>
>>>>> Second, 3/3... So we have a LED driver _and_ a regulator? So yes, the
>>>>> chip driving a LED is usually ... voltage/current regulator. What is
>>>>> second regulator doing there? Is that a common setup?
>>>> This is quite common with current-sink LED drivers.
>>>>
>>>> The setup looks like this:
>>>>
>>>> +-----------+
>>>> |           |
>>>> | Regulator |
>>>> |           +------------------------+
>>>> |           |                        |
>>>> +-----------+                      __|__
>>>>                                     \   /
>>>>           +---------------------+    \ / led
>>>>           |                     |     V
>>>>           |    Led Driver       |   --+--
>>>>           |                     |     |
>>>>           |                     |     |
>>>>           |                +----------+
>>>>           |              \      |
>>>>           |               \     |
>>>>           |                +    |
>>>>           |                |    |
>>>>           +---------------------+
>>>>                            |
>>>>                         +--+--+
>>>>                         ///////
>>>>
>>>>
>>>> Only the regulator usually does not supply only one LED.
>>> Given questions have been raised about the complexity of the change I
>>> wondered whether, for a system with this topology, the regulator
>>> could/should be enabled when the LED driver driver probes?
>> And this is how are doing that now.
>>
>> Moreover, just after seeing your ASCII art it has become obvious to me
>> that we can't simply do regulator_disable() while setting brightness to
>> LED_OFF since it can result in powering off the LED controller, which
>> would need to be reconfigured on next transition to
>> brightness > LED_OFF.
> 
> That is a problem only if the LED driver is powered by the same
> regulator, which is not the case in the diagram.

Indeed.

> This series make sense only for boards where LEDs have a dedicated
> voltage rail or can be modeled this way.
> 
> My use case is a LED panel driven by a LED current-sink controller that
> uses both a PWM-style control for brightness AND a active-low enable
> pin. If the enable pin is not HIGH, the panel is never completely dark
> even if the LED brightness is set to 0. I modeled this switch with a
> fixed-voltage regulator  which is a common way of doing it (it is after
> all how this things is done inside the panel).

So this use case would justify that feature. Nonetheless the solution
you proposed for mitigating regulator handling overhead may not work
for high blink rates. Maybe it would be worth to add an optional sysfs
file for determining if regulator should be handled by LED core.

The file could be created by LED class drivers that would demand it.

-- 
Best regards,
Jacek Anaszewski