Re: [PATCH v3 1/2] drivers: led: is31fl319x: 1/3/6/9-channel light effect led driver

[Jacek Anaszewski <j.anaszewski@samsung.com>]

Hi Nikolaus,

On 07/13/2016 08:09 AM, H. Nikolaus Schaller wrote:
[...]
>>> +	/*
>>> +	 * Kernel conventions require per-LED led-max-microamp property.
>>> +	 * But the chip does not allow to limit individual LEDs.
>>> +	 * So we take minimum from all subnodes.
>>
>> Why minimum? Choose maximum and reduce max_brightness properties
>> of the sub-LEDs with lesser led-max-microamp.
>
> Hm. Is this really the correct way to handle it?
>
> Assume you connect an LED which is specified with 10mA peak current.
> And another with 20mA peak current.
>
> So you define led-max-microamp in the DT as 10 mA and 20 mA.
>
> Firstly a user can set the brightness only to 50% of LED_FULL since it is limited
> by a reduced max_brightness. And heshe finds that not all LEDs have the same
> max_brightness. The first LED will have 127 and the second one 255 for reasons
> not directly understandable.

You have to know that led-max-microamp property was introduced
to standarize Device Tree definition of maximum brightness allowed for
a sub-LED.

Earlier people defined max-brightness property in DT, but at
some point we realized that brightness level is not a proper unit for
describing a hardware property.

It was not global max-microamp setting that appears in recent LED
controllers that drove the introduction of led-max-microamp property.

If you question the idea of having different maximum brightness per
sub-LEDs controlled by the same device, then it means that you have
objections to the entire idea of LED subsystem max_brightness property,
whereas it has been broadly accepted and successfully used for years.

> This entangles "brightness" with "max-current" - which are IMHO two independent
> things.

The fact that recent LED controllers use the same notion for one of its
settings shouldn't mislead us.

> Next, this will set the hardware limit to 20 mA. So there will be current peaks
> of 20 mA for an LED where the DT developer thinks to have specified a led-max-microamp
> of 10 mA. So you run the LED outside of its specs although the DT seems to
> tell that it is inside and user-space thinks it is ok. This will reduce lifetime of LEDs.

In what circumstances would such current peaks occur? On reset?
Shouldn't such a device be considered as broken by design?
What if all LEDs would have low amperage? Would there be some way
to protect them from being damaged by current peaks?

> So either "led-max-microamp" is the wrong name for this property (could better
> be "led-max-average-microamp") or the whole logic is broken.
>
> This is why we hesitate to hide (or even disable because you can't set the limit
> to 10mA by DT) the current limiting chip feature by such a difficult to understand
> automatic feature.
>
> Using the minimum of all led-max-microamp keeps everything on the safe
> side, running some LEDs with less current than specified. But never outside
> of the spec. And all LEDs have the same max_brightness which is IMHO
> more intuitive for the user.
>
> Our original proposal was to define led-max-microamp for the whole chip
> instead of individual LEDs, which is IMHO much easier to understand,
> because it corresponds one-to-one with the data sheet.



-- 
Best regards,
Jacek Anaszewski