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
