linux-kernel - Re: [PATCH v2 2/4] leds: add support for TI LP5860 LED driver chip

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20250718131855.GC11056@google.com>
Date: Fri, 18 Jul 2025 14:18:55 +0100
From: Lee Jones <lee@...nel.org>
To: Steffen Trumtrar <s.trumtrar@...gutronix.de>
Cc: Pavel Machek <pavel@....cz>, Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Steffen Trumtrar <kernel@...gutronix.de>,
	Pavel Machek <pavel@...nel.org>, linux-leds@...r.kernel.org,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/4] leds: add support for TI LP5860 LED driver chip

On Mon, 14 Jul 2025, Steffen Trumtrar wrote:

> 
> Hi,
> 
> On 2025-05-23 at 11:28 +01, Lee Jones <lee@...nel.org> wrote:
> 
> > On Wed, 14 May 2025, Steffen Trumtrar wrote:
> > 
> > > Add support for the Texas Instruments LP5860 LED driver chip
> > > via SPI interfaces.
> > > > The LP5860 is an LED matrix driver for up to 196 LEDs, which
> > supports
> > > short and open detection of the individual channel select lines.
> > > > The original driver is from an unknown author at Texas Instruments.
> > Only
> > > the cryptic handle 'zlzx' is known.
> > > > Co-developed-by: Steffen Trumtrar <s.trumtrar@...gutronix.de>
> > > Signed-off-by: Steffen Trumtrar <s.trumtrar@...gutronix.de>
> > > ---
> > >  Documentation/ABI/testing/sysfs-class-spi-lp5860 |  23 ++
> > >  drivers/leds/Kconfig                             |  23 ++
> > >  drivers/leds/Makefile                            |   2 +
> > >  drivers/leds/leds-lp5860-core.c                  | 276 ++++++++++++++++++++
> > >  drivers/leds/leds-lp5860-spi.c                   |  99 +++++++
> > 
> > Are you going to follow-up with another option?  Say I2C?
> 
> the chip also supports connection via I2C, but it's unlikely that I will add that myself.
> 
> > >  drivers/leds/leds-lp5860.h                       | 315 +++++++++++++++++++++++
> > >  6 files changed, 738 insertions(+)
> > > > diff --git a/Documentation/ABI/testing/sysfs-class-spi-lp5860
> > b/Documentation/ABI/testing/sysfs-class-spi-lp5860
> > 
> > This doesn't belong here.
> > 
> > > new file mode 100644
> > > index 0000000000000000000000000000000000000000..d24b49d38ecae55f1a1a4e465fbe71d30eff497e
> > > --- /dev/null
> > > +++ b/Documentation/ABI/testing/sysfs-class-spi-lp5860
> > > @@ -0,0 +1,23 @@
> > > +What:           /sys/class/spi_master/spi<bus>/spi<bus>.<dev>/b_current_set
> > 
> > Why would you want to change the current of the SPI bus?
> > 
> 
> Where does it belong? I grepped and followed the eeprom
> (Documentation/ABI/testing/sysfs-class-spi-eeprom) which uses sernum
> in the same way. Directions welcome.

I mean, in this patch.  Submit this part to the SPI subsystem maintainer.

> > > +Date:           May 2025
> > > +KernelVersion:  6.15
> > > +Contact:        Steffen Trumtrar <kernel@...gutronix.de>
> > > +Description:
> > > +	Contains and sets the current for the B color group.
> > 
> > What does the current set?  Brightness?
> > 
> > If so, the user shouldn't be expected to know what current set in order
> > to obtain a specific brightness.  Instead, shouldn't you use
> > /sys/class/leds/<led>/multi_intensity and let the driver deal with the
> > particulars of setting that brightness?
> > 
> 
> The chip has a global setting for the current of the three color
> groups. And an indiviual setting for every LED itself. The
> multi_intensity is for one LED as far as I understand. And the
> brightness of the whole matrix can be controlled via this global
> current setting.

What I mean is - don't call it current.  Use what it does when you
change the current.

> (...)
> 
> Thanks for the rest of the feedback. I already addressed all of that
> in my patches, but I'm not sure what is the correct way to proceed
> with the sysfs ABI entries.
> 
> 
> Thanks,
> Steffen
> 
> -- 
> Pengutronix e.K.                | Dipl.-Inform. Steffen Trumtrar |
> Steuerwalder Str. 21            | https://www.pengutronix.de/    |
> 31137 Hildesheim, Germany       | Phone: +49-5121-206917-0       |
> Amtsgericht Hildesheim, HRA 2686| Fax:   +49-5121-206917-5555    |
> 

-- 
Lee Jones [李琼斯]