From: Aren <aren@peacevolution.org>
To: "Ondřej Jirman" <megi@xff.cz>,
linux-kernel@vger.kernel.org,
"Krzysztof Kozlowski" <krzysztof.kozlowski+dt@linaro.org>,
"Rob Herring" <robh+dt@kernel.org>,
devicetree@vger.kernel.org,
"Jean-Jacques Hiblot" <jjhiblot@traphandler.com>,
"Chen-Yu Tsai" <wens@csie.org>,
linux-sunxi@lists.linux.dev, "Lee Jones" <lee@kernel.org>,
"Pavel Machek" <pavel@ucw.cz>,
linux-arm-kernel@lists.infradead.org,
"Jernej Skrabec" <jernej.skrabec@gmail.com>,
linux-leds@vger.kernel.org, "Conor Dooley" <conor+dt@kernel.org>,
"Miles Alan" <m@milesalan.com>,
"Samuel Holland" <samuel@sholland.org>
Subject: Re: [PATCH v2 3/4] arm64: dts: sun50i-a64-pinephone: add multicolor led node
Date: Fri, 23 Feb 2024 21:52:04 -0500 [thread overview]
Message-ID: <lkhwdfw67nfgsz2pectd7xgr724owf4z2qlyinmxncv5ncf4mw@si7tvaao7ums> (raw)
In-Reply-To: <xhowm7pne2og7iyhoa7wi26qa5q23fvl2ptaxtwnp4upgunubo@q5unl6mzvluy>
On Sat, Feb 24, 2024 at 12:28:36AM +0100, Ondřej Jirman wrote:
> Hello Aren,
>
> On Fri, Feb 23, 2024 at 10:22:36AM -0500, Aren wrote:
> > On Fri, Feb 23, 2024 at 09:46:25AM +0100, Ondřej Jirman wrote:
> > > Hello Aren,
> > >
> > > On Tue, Feb 06, 2024 at 01:13:19PM -0500, Aren Moynihan wrote:
> > > > The red, green, and blue leds currently in the device tree represent a
> > > > single rgb led on the front of the PinePhone.
> > > >
> > > > Signed-off-by: Aren Moynihan <aren@peacevolution.org>
> > > > ---
> > > >
> > > > Changes in v2:
> > > > - remove function property from individual led nodes
> > > >
> > > > .../boot/dts/allwinner/sun50i-a64-pinephone.dtsi | 16 ++++++++++------
> > > > 1 file changed, 10 insertions(+), 6 deletions(-)
> > > >
> > > > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi
> > > > index ad2476ee01e4..e53e0d4579a7 100644
> > > > --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi
> > > > +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi
> > > > @@ -39,28 +39,32 @@ chosen {
> > > > leds {
> > > > compatible = "gpio-leds";
> > > >
> > > > - led-0 {
> > > > - function = LED_FUNCTION_INDICATOR;
> > >
> > > This looks like a needless change that will just break people's current scripts
> > > and setup. It does mine, and there sure are others that will be surprised, too.
> > >
> > > This leads to a change in sysfs path from:
> > >
> > > /sys/class/leds/blue:indicator
> > >
> > > to
> > >
> > > /sys/class/leds/blue:
> > >
> > > which is 1) a weird name and 2) a backwards compatibility break for seemingly
> > > no apparent reason. Any reaons for the change?
> >
> > Leds-group-multicolor will make those read-only, so that will break when
> > it's enabled either way. Removing the function property makes it less
> > likely that programs attempting to discover leds will use the wrong
> > path.
>
> Allright. Then this breaks even more usecases then I thought. I quite depend on
> being able to apply various kernel based LED triggers to individual LEDs in the
> RGB LED. (The LED is not really multi-color in practice - you don't get a mix of
> colors. It's only technically a RGB LED, but if you turn on red and green, you
> see red and green, individually. I suspect it's a clear capped LED, not
> a diffused one and expects an external difuser - which Pinephone doesn't have.
> So this works out quite fine from the user perspective, because you see
> individual colors and can assign meanings to them individually)
>
> So this change makes it that I'll not be able to map different triggers
> to individual colors now.
>
> Eg. I have a power manager driver for Pinephone keyboard that exports LED triggers
> to indicate various charging states of the keyboard battery, so that I get
> warnings when the KB is discharged, or disconnected from control algorithm
> due to its tendency to shut off, and disappear from I2C bus, etc. AXP20x driver
> also exports a bunch of useful triggers.
>
> ---
>
> As a side note, this new multicolor abstraction seems a bit lacking and
> cumbersome, too.
>
> I still have to control individual LEDs via multi_intensity file and have to
> parse multi_index to even know which number corresponds to what color. In addition
> I have to ensure correct value in brightness file.
>
> If I want to change one of the LEDs, I now have to first read the whole status
> of multi-color LED and update it somehow in a non-racy way, which is
> impossible with this API without going through some userspace daemon that
> centralizes the LED control.
>
> If this is expected to be usable in device independent way, then there's another
> problem: multi_intensity values don't seem to have any range defined in sysfs,
> so I can't know what is the max usable value. How do I know the range is 0-1 vs
> 0-255 or 0-200000? There's max_brightness but that seems to be related to
> possible values in brightness file. Seems like I can write pretty much anything
> to multi_intensity, even things like this:
>
> https://megous.com/dl/tmp/4c6db9376f148951.png
>
> (looks like all LEDs are turned on even for negative intensity)
>
> If multi_intensity is supposed to be some relative measure of brightenss of
> individual LEDs in RGB LED, then echo '1 1 1000' > multi_intensity
> should pretty much result in only red LED being on. But all are on at full
> brightness. So the kernel doesn't even represent the desired color properly,
> based on HW limitations it knows about. There's no aproximation/rounding to the
> limits of control of individual LEDs. Even 1 millionth of relative intensity
> will result in the LED being turned fully on.
>
> Discoverability of how to control the LEDs is also quite lacking. Individual
> directories under /sys/class/leds are still there for individual parts of RGB
> LED, and it's not clearly identifiable that you can't control them. brightness
> file still has -rw-r--r-- permissions. But writing to it produces EBUSY.
> As far as I can see, the only indication that something is amiss is that
> there's a device/consumer:platform:multi-led symlink under the individual LEDs.
> But that can have any name in practice, and how can one know that just because
> some device has a in-kernel consumer, that it's a kind of a consumer that blocks
> changes to the device?
>
> If I have to discover that some LED is controlled by some other driver only
> by trying to light it up, then that's not a great API, I'd say.
>
> End of side note. :)
>
> ---
>
> ^^^^ All this from the PoV of a (API) user. I can understand the device-tree
> appeal of representing the technically RGB LED as RGB node, but as a user who
> messes with sysfs, and likes the simplicity and flexibility of how things work
> now, with the upstream Pinephone DT, I really dislike the change, because it
> forces unpleasant userspace changes, incl. necessity for some userspace LED
> control daemon to get race-free LED control from multiple sources. And I'm not
> even sure what I'd do with the keyboard driver, and how to keep the triggers
> working.
>
> Anyway, I and others can get around all this by:
>
> echo multi-led > /sys/class/leds/rgb:status/device/driver/unbind
>
> at system startup and get back control via idndividual LED directories.
> But please don't change the names of directories that were there up to now. That
> will just break many people's scripts for no reason. So my only request is to
> make this fallback to previous way it worked easy.
Makes sense, I'll send v1 again with a revised commit message when I get
a chance (this was the only change to the device tree part of this
patchset between v1 and v2).
Thanks
- Aren
> Kind regards,
> o.
>
> > I left these in v1 of this patch, but was recommended to remove them.
> > https://lore.kernel.org/lkml/k26bellccok4tj3kz2nrtp2vth2rnsiea677e2kzm56m767wjx@pnkqiz5hmiyb/
> >
> > Thanks for taking a look at this
> > - Aren
> >
> > > People normally hardcode these paths in eg. /etc/tmpfiles.d to apply LED triggers
> > > to particular LEDs.
> > >
> > > Kind regards,
> > > o.
> > >
> > > > + led0: led-0 {
> > > > color = <LED_COLOR_ID_BLUE>;
> > > > gpios = <&pio 3 20 GPIO_ACTIVE_HIGH>; /* PD20 */
> > > > retain-state-suspended;
> > > > };
> > > >
> > > > - led-1 {
> > > > - function = LED_FUNCTION_INDICATOR;
> > > > + led1: led-1 {
> > > > color = <LED_COLOR_ID_GREEN>;
> > > > gpios = <&pio 3 18 GPIO_ACTIVE_HIGH>; /* PD18 */
> > > > retain-state-suspended;
> > > > };
> > > >
> > > > - led-2 {
> > > > - function = LED_FUNCTION_INDICATOR;
> > > > + led2: led-2 {
> > > > color = <LED_COLOR_ID_RED>;
> > > > gpios = <&pio 3 19 GPIO_ACTIVE_HIGH>; /* PD19 */
> > > > retain-state-suspended;
> > > > };
> > > > };
> > > >
> > > > + multi-led {
> > > > + compatible = "leds-group-multicolor";
> > > > + color = <LED_COLOR_ID_RGB>;
> > > > + function = LED_FUNCTION_INDICATOR;
> > > > + leds = <&led0>, <&led1>, <&led2>;
> > > > + };
> > > > +
> > > > reg_ps: ps-regulator {
> > > > compatible = "regulator-fixed";
> > > > regulator-name = "ps";
> > > > --
> > > > 2.43.0
> > > >
next prev parent reply other threads:[~2024-02-24 2:52 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-02-06 18:13 [PATCH v2 1/4] leds: rgb: leds-group-multicolor: allow leds to stay on in suspend Aren Moynihan
2024-02-06 18:13 ` [PATCH v2 2/4] arm64: dts: sun50i-a64-pinephone: Retain leds state " Aren Moynihan
2024-02-06 18:13 ` [PATCH v2 3/4] arm64: dts: sun50i-a64-pinephone: add multicolor led node Aren Moynihan
2024-02-23 8:46 ` Ondřej Jirman
2024-02-23 15:22 ` Aren
2024-02-23 23:28 ` Ondřej Jirman
2024-02-24 2:52 ` Aren [this message]
2024-02-06 18:13 ` [PATCH v2 4/4] arm64: dts: sun50i-a64-pinephone: change led type to status Aren Moynihan
2024-02-22 20:57 ` Jernej Škrabec
2024-02-23 10:29 ` Lee Jones
2024-02-23 16:30 ` Aren
2024-02-23 16:36 ` Jernej Škrabec
2024-02-22 21:36 ` [PATCH v2 1/4] leds: rgb: leds-group-multicolor: allow leds to stay on in suspend Pavel Machek
2024-02-23 10:28 ` Lee Jones
2024-02-23 10:31 ` (subset) " Lee Jones
2024-02-23 10:35 ` Lee Jones
2024-02-23 15:32 ` Aren
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=lkhwdfw67nfgsz2pectd7xgr724owf4z2qlyinmxncv5ncf4mw@si7tvaao7ums \
--to=aren@peacevolution.org \
--cc=conor+dt@kernel.org \
--cc=devicetree@vger.kernel.org \
--cc=jernej.skrabec@gmail.com \
--cc=jjhiblot@traphandler.com \
--cc=krzysztof.kozlowski+dt@linaro.org \
--cc=lee@kernel.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-leds@vger.kernel.org \
--cc=linux-sunxi@lists.linux.dev \
--cc=m@milesalan.com \
--cc=megi@xff.cz \
--cc=pavel@ucw.cz \
--cc=robh+dt@kernel.org \
--cc=samuel@sholland.org \
--cc=wens@csie.org \
/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 read-only IMAP folder(s) and NNTP newsgroup(s).