Re: [PATCH v3 3/4] backlight: pwm_bl: compute brightness of LED linearly to human eye.

[Enric Balletbo i Serra]

Hi Matthias,

On 10/6/19 22:39, Matthias Kaehlcke wrote:
> Hi Enric
> 
> On Mon, Jun 10, 2019 at 12:00:02PM +0200, Enric Balletbo i Serra wrote:
>> Hi Matthias,
>>
>> On 8/6/19 23:02, Pavel Machek wrote:
>>> Hi!
>>>
>>>>> +	 * Note that this method is based on empirical testing on different
>>>>> +	 * devices with PWM of 8 and 16 bits of resolution.
>>>>> +	 */
>>>>> +	n = period;
>>>>> +	while (n) {
>>>>> +		counter += n % 2;
>>>>> +		n >>= 1;
>>>>> +	}
>>>>
>>>> I don't quite follow the heuristics above. Are you sure the number of
>>>> PWM bits can be infered from the period? What if the period value (in
>>>> ns) doesn't directly correspond to a register value? And even if it
>>>> did, counting the number of set bits (the above loops is a
>>>> re-implementation of ffs()) doesn't really result in the dividers
>>>> mentioned in the comment. E.g. a period of 32768 ns (0x8000) results
>>>> in a divider of 1, i.e. 32768 brighness levels.
>>>>
>>
>> Right, I think that only works on the cases that we only have one pwm cell, and
>> looks like during my tests I did only tests on devices with one pwm cell :-(
>>
>> And as you point the code is broken for other cases (pwm-cells > 1)
>>
>>>> On veyron minnie the period is 1000000 ns, which results in 142858
>>>> levels (1000000 / 7)!
>>>>
>>>> Not sure if there is a clean solution using heuristics, a DT property
>>>> specifying the number of levels could be an alternative. This could
>>>> also be useful to limit the number of (mostly) redundant levels, even
>>>> the intended max of 4096 seems pretty high.
>>>>
>>
>> Looking again looks like we _can not_ deduce the number of bits of a pwm, it is
>> not exposed at all, so I think we will need to end adding a property to specify
>> this. Something similar to what leds-pwm binding does, it has:
>>
>> max-brightness : Maximum brightness possible for the LED
> 
> Thanks for the confirmation that I didn't just miss some clever trick.
> 
> I also think that some kind of DT property is needed, I'll try to come
> up with a reasonable name, keeping in mind that some devices might not
> want to use the entire range of levels.
> 

Note that, If I remember correctly, the original idea behind all these patches
was provide a default curve with enough points following the  CIE 1931 formula
(which describes how we perceive light). When default doesn't work for your
hardware, you could play and define your own curve using the
num-interpolated-steps property I.e:

 brightness-levels = <0 2048 4096 8192 16384 65535>;
 num-interpolated-steps = <2048>;
 default-brightness-level = <4096>;

Or even expose all the possible levels, like you do with your chromeos kernel.

 brightness-levels = <0 65535>;
 num-interpolated-steps = <65535>;
 default-brightness-level = <4096>;

The above should work independently of the bug found (that of course needs to be
fixed)

Enric