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

[Pavel Machek]

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".

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.:

+	  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.

Best regards,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Attachment: signature.asc
Description: Digital signature