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
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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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;
> };
>
> /**
>
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<[email protected]>
>> ---
>> 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;
>> };
>>
>> /**
>>
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
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
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
>
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.
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
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.
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