Re: [GIT PULL v2] LM3532 backlight support improvements and relocation

[Dan Murphy]

Pavel

Thanks for the review

On 4/7/19 5:17 PM, Pavel Machek wrote:
> Hi!
> 
>> Changes since v1:
>>
>> - synchronized DT label properties in DT bindings with what has been agreed
>>   for the patch "ARM: dts: omap4-droid4: Update backlight dt properties"
>>
>> The following changes since commit 9e98c678c2d6ae3a17cb2de55d17f69dddaa231b:
>>
>>   Linux 5.1-rc1 (2019-03-17 14:22:26 -0700)
>>
>> are available in the git repository at:
>>
>>   git://git.kernel.org/pub/scm/linux/kernel/git/j.anaszewski/linux-leds.git tags/lm3532-driver-improvements
>>
>> for you to fetch changes up to bc1b8492c764fea940fc66206047e37a7f8d77e1:
>>
>>   leds: lm3532: Introduce the lm3532 LED driver (2019-04-07 20:45:49 +0200)
>>
>> Thanks,
>> Jacek Anaszewski
>>
>> ----------------------------------------------------------------
>> LM3532 backlight driver improvements and relocation
>> ----------------------------------------------------------------
>> Dan Murphy (4):
>>       dt: lm3532: Add lm3532 dt doc and update ti_lmu doc
> 
> Sorry, this one will need more work.
> 
> When did Rob acknowledge this? I do not remember that mail & quick
> google does not find it.
> 
> I still don't like the fact that it is using different binding from
> other similar chips. It for example replaces "ramp-up-msec" with
> "ramp-up-us".
> 

It does not replace anything.  The LM3532 has a completely different ramping table then
the other LMU devices.  The ramp times for this part is in uS not mS and there is no alignment on
common values or deltas.

> This is certainly wrong:
> 
> +Optional properties if ALS mode is used:
> +	  - ti,als-vmin - Minimum ALS voltage defined in Volts
> +	  - ti,als-vmax - Maximum ALS voltage defined in Volts
> +	  Per the data sheet the max ALS voltage is 2V and the min is
> 0V
> 
> ...but milivolts (or microvolts?) make sense there, and clearly
> milivolts are used because 2000V is way out of range.:
> 

I will send a patch that changes "Volts" to "millivolts" in the description.

> +	  ti,als-vmin = <0>;
> +	  ti,als-vmax = <2000>;
> 
> Plus:
> 
> +Required child properties:
> ...
> +	- ti,led-mode : Defines if the LED strings are manually controlled or
> +			if the LED strings are controlled by the ALS.
> +			       0x00 - LED strings are I2C controlled via full scale brightness control register
> +        		  	  0x01 - LED strings are ALS controlled
> 
> Dunno. Normally we'd have "ti,automatic-led-mode", and if it was not
> present, we'd default to manual, no? (Also "led-mode" is a bit too
> generic. ti,als-enabled would be better description).
> 
> Plus, I'd kind of expect ALS enabled/disabled to be runtime controled,
> not from the device tree.

We can always add runtime override control to the driver.

Just need to see if there is a common interface from input or IIO we can adopt.

Dan


> 
> Best regards,
> 									Pavel
> 


-- 
------------------
Dan Murphy