linux-kernel - Re: [PATCH v5 3/3] leds: Add control of the voltage/current regulator to the LED core

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <939cf695-b955-da01-2a2b-3b5ce0dc3660@gmail.com>
Date:   Mon, 14 Oct 2019 20:48:48 +0200
From:   Jacek Anaszewski <jacek.anaszewski@...il.com>
To:     Daniel Thompson <daniel.thompson@...aro.org>,
        Jean-Jacques Hiblot <jjhiblot@...com>
Cc:     Pavel Machek <pavel@....cz>, linux-leds@...r.kernel.org,
        linux-kernel@...r.kernel.org, dmurphy@...com, tomi.valkeinen@...com
Subject: Re: [PATCH v5 3/3] leds: Add control of the voltage/current regulator
 to the LED core

On 10/14/19 2:38 PM, Daniel Thompson wrote:
> On Mon, Oct 14, 2019 at 12:49:07PM +0200, Jean-Jacques Hiblot wrote:
>>
>> On 13/10/2019 14:09, Pavel Machek wrote:
>>> Hi!
>>>
>>>> I must say I'm not a big fan of this change.
>>>> It adds a bunch of code to the LED core and gives small
>>>> functionality in a reward. It may also influence maximum
>>>> software blinking rate, so I'd rather avoid calling
>>>> regulator_enable/disable when timer trigger is set.
>>>>
>>>> It will of course require more code.
>>>>
>>>> Since AFAIR Pavel was original proponent of this change then
>>>> I'd like to see his opinion before we move on to discussing
>>>> possible improvements to this patch.
>>> Was I?
>>>
>>> Okay, this series looks quite confusing to me. First, 1/3 looks
>>> "interesting" (would have to analyze it way more).
>>>
>>> Second, 3/3... So we have a LED driver _and_ a regulator? So yes, the
>>> chip driving a LED is usually ... voltage/current regulator. What is
>>> second regulator doing there? Is that a common setup?
>>
>> This is quite common with current-sink LED drivers.
>>
>> The setup looks like this:
>>
>> +-----------+
>> |           |
>> | Regulator |
>> |           +------------------------+
>> |           |                        |
>> +-----------+                      __|__
>>                                    \   /
>>          +---------------------+    \ / led
>>          |                     |     V
>>          |    Led Driver       |   --+--
>>          |                     |     |
>>          |                     |     |
>>          |                +----------+
>>          |              \      |
>>          |               \     |
>>          |                +    |
>>          |                |    |
>>          +---------------------+
>>                           |
>>                        +--+--+
>>                        ///////
>>
>>
>> Only the regulator usually does not supply only one LED.
> 
> Given questions have been raised about the complexity of the change I
> wondered whether, for a system with this topology, the regulator
> could/should be enabled when the LED driver driver probes?

And this is how are doing that now.

Moreover, just after seeing your ASCII art it has become obvious to me
that we can't simply do regulator_disable() while setting brightness to
LED_OFF since it can result in powering off the LED controller, which
would need to be reconfigured on next transition to
brightness > LED_OFF.

It looks that the idea is flawed I'm afraid.

-- 
Best regards,
Jacek Anaszewski