From: Dan Murphy <dmurphy@ti.com>
To: Pavel Machek <pavel@ucw.cz>,
Jacek Anaszewski <jacek.anaszewski@gmail.com>, <robh@kernel.org>
Cc: <lee.jones@linaro.org>, <tony@atomide.com>,
<linux-leds@vger.kernel.org>, <linux-kernel@vger.kernel.org>
Subject: Re: [GIT PULL v2] LM3532 backlight support improvements and relocation
Date: Mon, 8 Apr 2019 10:53:59 -0500 [thread overview]
Message-ID: <f96d09dc-3ddc-4ac7-3668-508df430f686@ti.com> (raw)
In-Reply-To: <20190407221740.GA6327@amd>
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
next prev parent reply other threads:[~2019-04-08 15:54 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-04-07 19:09 [GIT PULL v2] LM3532 backlight support improvements and relocation Jacek Anaszewski
2019-04-07 22:17 ` Pavel Machek
2019-04-07 22:50 ` Pavel Machek
2019-04-08 15:42 ` Dan Murphy
2019-04-08 15:53 ` Dan Murphy [this message]
2019-04-08 19:30 ` Tony Lindgren
2019-04-08 20:03 ` Dan Murphy
2019-04-09 14:56 ` Tony Lindgren
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=f96d09dc-3ddc-4ac7-3668-508df430f686@ti.com \
--to=dmurphy@ti.com \
--cc=jacek.anaszewski@gmail.com \
--cc=lee.jones@linaro.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-leds@vger.kernel.org \
--cc=pavel@ucw.cz \
--cc=robh@kernel.org \
--cc=tony@atomide.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).