Messages in this thread | | | From | Jean-Jacques Hiblot <> | Subject | Re: [PATCH v5 3/3] leds: Add control of the voltage/current regulator to the LED core | Date | Wed, 25 Sep 2019 10:41:48 +0200 |
| |
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; >> }; >> >> /** >>
| |