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
