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

[Jacek Anaszewski]

On 10/16/19 10:13 AM, Jean-Jacques Hiblot wrote:
> 
> On 14/10/2019 20:48, Jacek Anaszewski wrote:
>> 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.
> 
> That is a problem only if the LED driver is powered by the same
> regulator, which is not the case in the diagram.

Indeed.

> This series make sense only for boards where LEDs have a dedicated
> voltage rail or can be modeled this way.
> 
> My use case is a LED panel driven by a LED current-sink controller that
> uses both a PWM-style control for brightness AND a active-low enable
> pin. If the enable pin is not HIGH, the panel is never completely dark
> even if the LED brightness is set to 0. I modeled this switch with a
> fixed-voltage regulator which is a common way of doing it (it is after
> all how this things is done inside the panel).

So this use case would justify that feature. Nonetheless the solution
you proposed for mitigating regulator handling overhead may not work
for high blink rates. Maybe it would be worth to add an optional sysfs
file for determining if regulator should be handled by LED core.

The file could be created by LED class drivers that would demand it.

-- 
Best regards,
Jacek Anaszewski