linux-kernel - Re: [PATCH v3 2/3] 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] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 18 Jul 2019 15:31:56 +0200
From:   Jean-Jacques Hiblot <jjhiblot@...com>
To:     Jacek Anaszewski <jacek.anaszewski@...il.com>, <pavel@....cz>,
        <robh+dt@...nel.org>, <mark.rutland@....com>,
        <daniel.thompson@...aro.org>
CC:     <dmurphy@...com>, <linux-leds@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, <devicetree@...r.kernel.org>
Subject: Re: [PATCH v3 2/3] leds: Add control of the voltage/current regulator
 to the LED core


On 18/07/2019 14:24, Jacek Anaszewski wrote:
> Hi Jean,
>
> Thank you for the updated patch set.
>
> I have some more comments below.
>
> On 7/17/19 3:59 PM, Jean-Jacques Hiblot wrote:
>>   
>> +static bool __led_need_regulator_update(struct led_classdev *led_cdev,
>> +					int brightness)
>> +{
>> +	bool new_state = (brightness != LED_OFF);
> How about:
>
> bool new_state = !!brightness;

Throughout the code LED_OFF is used when the LED is turned off. I think 
it would be more consistent to use it there too.

>
>> +
>> +	return led_cdev->regulator && led_cdev->regulator_state != new_state;
>> +}
>> +static int __led_handle_regulator(struct led_classdev *led_cdev,
>> +				int brightness)
>> +{
>> +	int rc;
>> +
>> +	if (__led_need_regulator_update(led_cdev, brightness)) {
>> +
>> +		if (brightness != LED_OFF)
>> +			rc = regulator_enable(led_cdev->regulator);
>> +		else
>> +			rc = regulator_disable(led_cdev->regulator);
>> +		if (rc)
>> +			return rc;
>> +
>> +		led_cdev->regulator_state = (brightness != LED_OFF);
>> +	}
>> +	return 0;
>> +}
> Let's have these function names without leading underscores.
OK.
>
>>   static int __led_set_brightness(struct led_classdev *led_cdev,
>>   				enum led_brightness value)
>>   {
>> @@ -115,6 +142,8 @@ static void set_brightness_delayed(struct work_struct *ws)
>>   	if (ret == -ENOTSUPP)
>>   		ret = __led_set_brightness_blocking(led_cdev,
>>   					led_cdev->delayed_set_value);
>> +	__led_handle_regulator(led_cdev, led_cdev->delayed_set_value)
> If you called it from __led_set_brightness() and

We cannot call it from __led_set_brightness() because it is supposed not 
to block.

JJ