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

[Jacek Anaszewski]

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