From: "Jan Kundrát" <jan.kundrat@cesnet.cz>
To: <linux-leds@vger.kernel.org>
Cc: "Dan Murphy" <dmurphy@ti.com>, "Pavel Machek" <pavel@ucw.cz>,
"Andy Shevchenko" <andriy.shevchenko@linux.intel.com>,
"Christophe JAILLET" <christophe.jaillet@wanadoo.fr>,
"Marek Behún" <kabel@kernel.org>
Subject: lp50xx: LED banking appears to be broken
Date: Sat, 31 Jul 2021 10:14:10 +0200 [thread overview]
Message-ID: <d049e22d-5ff8-4a68-a46c-3a1d533afcd0@cesnet.cz> (raw)
Hi there,
I'm trying to use the LP5009 chip with the following HW setup:
- channels 1-6 drive a big, 20mm LED module which internally consists of
six independent LEDs
- channels 7, 8 and 9 drive a RGB LED as usual.
I thought that a DT bindings like this will work:
led-controller@0c {
compatible = "ti,lp5009";
reg = <0x0c>;
#address-cells = <1>;
#size-cells = <0>;
status = "okay";
multi-led@1 {
#address-cells = <1>;
#size-cells = <0>;
reg = <1>;
label = "tally:1";
led {
color = <LED_COLOR_ID_RED>;
};
};
multi-led@2 {
#address-cells = <1>;
#size-cells = <0>;
reg = <2>;
label = "tally:2";
led {
color = <LED_COLOR_ID_RED>;
};
};
multi-led@3 {
#address-cells = <1>;
#size-cells = <0>;
reg = <3>;
label = "tally:3";
led {
color = <LED_COLOR_ID_RED>;
};
};
multi-led@4 {
#address-cells = <1>;
#size-cells = <0>;
reg = <4>;
label = "tally:4";
led {
color = <LED_COLOR_ID_RED>;
};
};
multi-led@5 {
#address-cells = <1>;
#size-cells = <0>;
reg = <5>;
label = "tally:5";
led {
color = <LED_COLOR_ID_RED>;
};
};
multi-led@6 {
#address-cells = <1>;
#size-cells = <0>;
reg = <6>;
label = "tally:6";
led {
color = <LED_COLOR_ID_RED>;
};
};
multi-led@7 {
#address-cells = <1>;
#size-cells = <2>;
reg = <7 8 9>;
color = <LED_COLOR_ID_RGB>;
label = "preview";
led@7 {
color = <LED_COLOR_ID_RED>;
};
led@8 {
color = <LED_COLOR_ID_GREEN>;
};
led@9 {
color = <LED_COLOR_ID_BLUE>;
};
};
};
This has drawbacks:
- I get a multicolor sysfs entry for each of the six red sub-LEDs, which
probably doesn't make much sense. I cannot do a one "multicolor" LED with
six channels because there appears to be a limit of 3 channels, and because
the order of channels is documented to be non-deterministic, so that would
require me to come up with fake names or something. Also, driving this from
userspace means two writes for each sub-LED.
- The sysfs entries do not appear to drive correct LEDs. For example, a
write to tally:5 or tally:6 results in an error:
lp50xx 1-000c: Cannot write intensity value -5
leds tally:6: Setting an LED's brightness failed (-5)
I tried to simplify this, and kept just the one RGB LED (that is, the
multi-led@7 and led@7, led@8 and led@9 stanzas). This resulted in the
following regmap entries after init:
# cat /sys/kernel/debug/regmap/1-000c/registers
00: 40
01: 3c
02: 80
03: ff
04: 0f
05: 0f
06: 0f
07: 0f
08: ff
09: ff
0a: ff
0b: 0f
0c: 00
0d: 00
0e: 00
0f: 00
10: 00
11: 00
12: 00
13: 00
14: 00
15: 00
16: 00
17: ff
Clearly, that's wrong because it sets register's 0x02 reserved bits to
non-zero. It looks as if the LED's channel number gets translated to the
bank number, which is wrong. There are nine LEDs on LP5009, but only three
individual banks.
Also, I don't think that the concept of "banks" as defined in LP50xx chips
should be used in the Linux driver. The datasheet is not terribly specific
on details, but it looks to me that the "banks" are for a use case where
multiple physical LEDs are to, e.g., "breathe together". The chip indeed
imposes some limitations when banking is enabled:
- LED0 will always be on channels 1-3. That's incompatible with the current
code which uses the `reg` DT property and allows arbitrary assigning of
channels to a LED's color inputs. I can have a Linux RGB LED which uses
channels 1, 5 and 9 just fine, but I cannot use banking for that.
- Bank A always drives the first color of all LEDs that have banking
enabled. Bank B is always for the second color, and bank C always applies
to the third color.
As far as I can tell, there's no support for cross-LED control in Linux, so
I think that we can just rip support for banking from this driver. The main
motivation appears to be saving some I2C bandwidth and MCU cycles. If the
driver was serious about this, it would use register auto-increment as a
first step I suppose, but the regmap subsystem as-is updates all registers
independently.
Before I send a patch which implements all that, I wanted to ask if I
understood everything right, and to check whether these suggestions make
sense to the maintainer and to the original author of the driver (and,
hopefully, to the users as well).
With kind regards,
Jan
next reply other threads:[~2021-07-31 8:14 UTC|newest]
Thread overview: 2+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-07-31 8:14 Jan Kundrát [this message]
2021-08-02 22:17 ` lp50xx: LED banking appears to be broken Jacek Anaszewski
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=d049e22d-5ff8-4a68-a46c-3a1d533afcd0@cesnet.cz \
--to=jan.kundrat@cesnet.cz \
--cc=andriy.shevchenko@linux.intel.com \
--cc=christophe.jaillet@wanadoo.fr \
--cc=dmurphy@ti.com \
--cc=kabel@kernel.org \
--cc=linux-leds@vger.kernel.org \
--cc=pavel@ucw.cz \
/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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.