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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux