Re: [PATCH v3 2/2] leds: lp8864: New driver

["Sverdlin, Alexander" <alexander.sverdlin@siemens.com>]

Hi Lee!

On Thu, 2024-12-12 at 18:59 +0000, Lee Jones wrote:
> On Mon, 09 Dec 2024, A. Sverdlin wrote:
> 
> > From: Alexander Sverdlin <alexander.sverdlin@siemens.com>
> > 
> > Add driver for TI LP8864, LP8864S, LP8866 4/6 channel LED-backlight drivers
> > with I2C interface.
> > 
> > Link: https://www.ti.com/lit/gpn/lp8864-q1
> > Link: https://www.ti.com/lit/gpn/lp8864s-q1
> > Link: https://www.ti.com/lit/gpn/lp8866-q1
> > Link: https://www.ti.com/lit/gpn/lp8866s-q1
> > Signed-off-by: Alexander Sverdlin <alexander.sverdlin@siemens.com>
> > ---
> > Changelog:
> > v3:
> > - dropped lp8864_init(), REGCACHE_NONE, %pe in dev_err_probe(),
> >    i2c_set_clientdata()
> > - added devm_add_action_or_reset() return value check, dev_err_probe() after
> >    devm_regmap_init_i2c()
> > v2: no changes
> > 
> >   MAINTAINERS                |   7 +
> >   drivers/leds/Kconfig       |  12 ++
> >   drivers/leds/Makefile      |   1 +
> >   drivers/leds/leds-lp8864.c | 308 +++++++++++++++++++++++++++++++++++++
> >   4 files changed, 328 insertions(+)
> >   create mode 100644 drivers/leds/leds-lp8864.c
> > 
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 21f855fe468bc..a89f0b9d991fb 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -23262,6 +23262,13 @@ S:	Supported
> >   F:	Documentation/devicetree/bindings/iio/dac/ti,dac7612.yaml
> >   F:	drivers/iio/dac/ti-dac7612.c
> >   
> > +TEXAS INSTRUMENTS' LB8864 LED BACKLIGHT DRIVER
> > +M:	Alexander Sverdlin <alexander.sverdlin@siemens.com>
> > +L:	linux-leds@vger.kernel.org
> > +S:	Maintained
> > +F:	Documentation/devicetree/bindings/leds/backlight/ti,lp8864.yaml
> > +F:	drivers/leds/leds-lp8864.c
> > +
> >   TEXAS INSTRUMENTS' SYSTEM CONTROL INTERFACE (TISCI) PROTOCOL DRIVER
> >   M:	Nishanth Menon <nm@ti.com>
> >   M:	Tero Kristo <kristo@kernel.org>
> > diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> > index b784bb74a8378..6d0e88e501614 100644
> > --- a/drivers/leds/Kconfig
> > +++ b/drivers/leds/Kconfig
> > @@ -511,6 +511,18 @@ config LEDS_LP8860
> >   	  on the LP8860 4 channel LED driver using the I2C communication
> >   	  bus.
> >   
> > +config LEDS_LP8864
> > +	tristate "LED support for the TI LP8864/LP8866 4/6 channel LED drivers"
> > +	depends on LEDS_CLASS && I2C && OF
> > +	select REGMAP_I2C
> > +	help
> > +	  If you say yes here you get support for the TI LP8864-Q1,
> > +	  LP8864S-Q1, LP8866-Q1, LP8866S-Q1 4/6 channel LED backlight
> > +	  drivers with I2C interface.
> > +
> > +	  To compile this driver as a module, choose M here: the
> > +	  module will be called leds-lp8864.
> > +
> >   config LEDS_CLEVO_MAIL
> >   	tristate "Mail LED on Clevo notebook"
> >   	depends on LEDS_CLASS && BROKEN
> > diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> > index 18afbb5a23ee5..f66bf2e13665f 100644
> > --- a/drivers/leds/Makefile
> > +++ b/drivers/leds/Makefile
> > @@ -57,6 +57,7 @@ obj-$(CONFIG_LEDS_LP55XX_COMMON)	+= leds-lp55xx-common.o
> >   obj-$(CONFIG_LEDS_LP8501)		+= leds-lp8501.o
> >   obj-$(CONFIG_LEDS_LP8788)		+= leds-lp8788.o
> 
> >   obj-$(CONFIG_LEDS_LP8860)		+= leds-lp8860.o
> > +obj-$(CONFIG_LEDS_LP8864)		+= leds-lp8864.o
> 
> How different are these two devices?  Do you need a new driver?

From the marketing perspective these are similar devices, but it turns out the
implementation is very different, new devices are much simpler, they
don't have "cluster mode", only one global brightness. No user-accessible EEPROM
(handling of which feels like 60% of the 8860 driver).
The only similar thing is the register name to control brightness, but register
map is completely different, with 16-bit wide registers instead of 8 in the
legacy devices. I actually made a diff between them, only couple of boilerplate
lines remain from the original one. There would be no code reuse from the original
driver if I'd add the support into 8860.

Thank you for your review, I'll incorporate your comments into v4!

-- 
Alexander Sverdlin
Siemens AG
www.siemens.com