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
