Messages in this thread | | | Subject | Re: [PATCH v3 2/3] leds: Add control of the voltage/current regulator to the LED core | From | Jean-Jacques Hiblot <> | Date | Fri, 20 Sep 2019 14:29:58 +0200 |
| |
Hi Jacek,
On 18/07/2019 19:49, Jacek Anaszewski wrote: > 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.
Taking your queue I reworked the series to take better care of the concurrency issues.
I believe it's in better shape right now.
> > 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.
I tried the patch and get a lot of warning because of triggers on storage devices.
Making led_set_brightness() not callable from a IRQ context, is probably not the right approach anymore.
JJ
> > Only then we could think of adding another asynchronous dependency > to the brightness setting chain. > > [0] https://lkml.org/lkml/2018/1/17/1144 >
| |