Re: [PATCH v3 2/3] leds: Add control of the voltage/current regulator to the LED core

[Jacek Anaszewski]

On 7/18/19 3:31 PM, Jean-Jacques Hiblot wrote:
> 
> 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.

Basically brightness is a scalar and 0 always means off.
We treat enum led_brightness as a legacy type - it is no
longer valid on the whole its span since LED_FULL = 255
was depreciated with addition of max_brightness property.

IMHO use of reverse logic here only hinders code analysis.

>>> +
>>> +ÂÂÂ 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.

You're right. The problematic part is that with regulator handling
we cannot treat the whole brightness setting operation uniformly
for brightness_set op case, i.e. without mediation of a workqueue.

Now you have to fire workqueue in led_set_brightness_nopm()
even for brightness_set() op path, if regulator state needs update.
This is ugly and can be misleading. Can be also error prone and
have non-obvious implications for software blink state transitions.

I think we would first need to improve locking between the workqueue
and led_timer_function(). I proposed a patch [0] over a year
ago.

Only then we could think of adding another asynchronous dependency
to the brightness setting chain.

[0] https://lkml.org/lkml/2018/1/17/1144

-- 
Best regards,
Jacek Anaszewski