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

[Jacek Anaszewski <jacek.anaszewski@gmail.com>]

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;
>  };
>  
>  /**
>