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

[Steffen Trumtrar]

Hi,

On 2025-05-23 at 11:28 +01, Lee Jones <lee@xxxxxxxxxx> 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@xxxxxxxxxxxxxx>
> > Signed-off-by: Steffen Trumtrar <s.trumtrar@xxxxxxxxxxxxxx>
> > ---
> >  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.

> > +Date:           May 2025
> > +KernelVersion:  6.15
> > +Contact:        Steffen Trumtrar <kernel@xxxxxxxxxxxxxx>
> > +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.


(...)

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    |