lkml.org 
[lkml]   [2020]   [Apr]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
SubjectRe: [PATCH v7 2/2] leds: Add control of the voltage/current regulator to the LED core
From
Date
Hi,

On 04/12/2019 14:37, Pavel Machek wrote:
> Hi!
>
>> 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.
>>
>> Because turning ON/OFF a regulator might block, it is not done
>> synchronously but done in a workqueue. Turning ON the regulator is
>> always

JJ had to leave this unfinished, and now I'm trying to pick this work up.

> How will this interact with LEDs that can be used from atomic context?

I think the idea here is that if the LED uses a regulator, and the regulator needs to be enabled,
then the whole work is scheduled. If there's no regulator or if the regulator is already enabled,
the brightness is set directly.

>> +static ssize_t regulator_auto_off_store(struct device *dev,
>> + struct device_attribute *attr, const char *buf, size_t size)
>> +{
>> + struct led_classdev *led_cdev = dev_get_drvdata(dev);
>> + ssize_t ret = size;
>> + bool auto_off;
>> +
>> + if (strncmp(buf, "enable\n", size) == 0)
>> + auto_off = true;
>> + else if (strncmp(buf, "disable\n", size) == 0)
>> + auto_off = false;
>> + else
>> + return -EINVAL;
>
> Sounds like device power management to me. Is it compatible with that?

Can you elaborate what you mean?

>> @@ -135,6 +203,8 @@ 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);
>> +
>> + led_handle_regulator(led_cdev);
>> }
>>
>
> You only modify set_brigthness_delays, so this will not work at all
> for non-blocking LEDs, right?

I'm not that familiar with led framework yet, but if setting of the brightness always goes via
led_set_brightness_nopm(), then I think this would work for all LEDs, as led_set_brightness_nopm
will schedule the work if needed (which goes to set_brightness_delayed).

>> static void led_set_software_blink(struct led_classdev *led_cdev,
>> @@ -189,6 +259,7 @@ static void led_blink_setup(struct led_classdev *led_cdev,
>> void led_init_core(struct led_classdev *led_cdev)
>> {
>> INIT_WORK(&led_cdev->set_brightness_work, set_brightness_delayed);
>> + INIT_DELAYED_WORK(&led_cdev->reg_off_work, turn_off_regulator_delayed);
>>
>
> Could this re-use the workqueue? Many systems will not need
> regulators, so this is overhead...

You mean re-use the 'set_brightness_work' work? I'm not sure how that would be done, they're a bit
different things. set_brightness_work is used to change the context from atomic to non-atomic, and
reg_off_work is used to turn the regulator off after a specified delay.

>> + /*
>> + * the regulator must be turned off. This cannot be
>
> Use "The", and fix double spaces between must and be.
>
>> + } else if (regulator_on && old == REG_R_OFF_U_OFF) {
>> + /*
>> + * the regulator must be enabled. This cannot be here
>
> "The"
>
>> + /*
>> + * small optimization. Cancel the work that had been started
>
> "Small."
>
>> +#include <linux/regulator/consumer.h>
>> +
>> +/*
>> + * The regulator state tracks 2 boolean variables:
>> + * - the state of regulator (or more precisely the state required by
>> + * led core layer, as many users can interact with the same regulator).
>> + * It is tracked by bit 0.
>> + * - the state last asked-for by the LED user. It is tracked by bit 1.
>> + */
>> +#define REG_R_ON BIT(0)
>> +#define REG_U_ON BIT(1)
>> +
>> +enum { REG_R_OFF_U_OFF = 0,
>> + REG_R_ON_U_OFF = REG_R_ON,
>> + REG_R_OFF_U_ON = REG_U_ON,
>> + REG_R_ON_U_ON = REG_R_ON | REG_U_ON
>> +};
>
> That's quite weird use of enum.

Yes, these could as well be defines, if that's what you mean.

>> +++ b/include/linux/leds.h
>> @@ -149,6 +149,15 @@ struct led_classdev {
>>
>> /* Ensures consistent access to the LED Flash Class device */
>> struct mutex led_access;
>> +
>> + /* regulator */
>
> "Regulator".

Fixed this and the other cosmetic changes, thanks!

Overall, I wonder if all this is worth it as this is somewhat complex change, compared to just
having the regulator always enabled...

Tomi

--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
\
 
 \ /
  Last update: 2020-04-24 14:48    [W:0.059 / U:0.512 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site