linux-kernel - Re: [PATCH v7 2/2] leds: Add control of the voltage/current regulator to the LED core

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Fri, 24 Apr 2020 15:47:18 +0300
From:   Tomi Valkeinen <tomi.valkeinen@...com>
To:     Pavel Machek <pavel@....cz>, <jacek.anaszewski@...il.com>,
        Dan Murphy <dmurphy@...com>
CC:     <linux-leds@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v7 2/2] leds: Add control of the voltage/current regulator
 to the LED core

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