| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Wed, 25 Sep 2019 10:41:48 +0200
From: Jean-Jacques Hiblot <jjhiblot@...com>
To: Jacek Anaszewski <jacek.anaszewski@...il.com>, <pavel@....cz>,
<daniel.thompson@...aro.org>
CC: <linux-leds@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
<dmurphy@...com>, <tomi.valkeinen@...com>
Subject: Re: [PATCH v5 3/3] leds: Add control of the voltage/current regulator
to the LED core
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@...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;
>> };
>>
>> /**
>>
Powered by blists - more mailing lists