Messages in this thread Patch in this message | | | Subject | Re: [PATCH v7 2/2] leds: Add control of the voltage/current regulator to the LED core | From | Tomi Valkeinen <> | Date | Fri, 24 Apr 2020 15:47:18 +0300 |
| |
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
| |