Message-ID: <CAHp75VcNuXouL25ZRiym97AjR9249=ENMPFDQ7imZ_ZoeKc3Ng@mail.gmail.com>
Date: Thu, 8 May 2025 11:24:25 +0300
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: Nam Tran <trannamatk@...il.com>, Lee Jones <lee@...nel.org>
Cc: andy@...nel.org, geert@...ux-m68k.org, pavel@....cz, robh@...nel.org, 
	krzk+dt@...nel.org, conor+dt@...nel.org, christophe.jaillet@...adoo.fr, 
	corbet@....net, devicetree@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-doc@...r.kernel.org, florian.fainelli@...adcom.com, 
	bcm-kernel-feedback-list@...adcom.com, linux-rpi-kernel@...ts.infradead.org, 
	linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v8 0/5] auxdisplay: add support for TI LP5812 4x3 Matrix
 LED driver

First of all, I just noticed that you excluded Lee from the
distribution list. Don't do that as he is a stakeholder here as well
since it has not been decided yet where to go with your stuff.

On Wed, May 7, 2025 at 7:42 PM Nam Tran <trannamatk@...il.com> wrote:
> On Tue, 29 Apr 2025 Andy Shevchenko wrote:
> > On Tue, Apr 29, 2025 at 8:02 PM Nam Tran <trannamatk@...il.com> wrote:
> > > On Mon, 28 Apr 2025 Pavel Machek wrote:
> > > > > On Mon, 28 Apr 2025 Geert Uytterhoeven wrote:
> > > >
> > > > > > > > > - Move driver to drivers/auxdisplay/ instead of drivers/leds/.
> > > > > > > > > - Rename files from leds-lp5812.c/.h to lp5812.c/.h.
> > > > > > > > > - Move ti,lp5812.yaml binding to auxdisplay/ directory,
> > > > > > > > >   and update the title and $id to match new path.
> > > > > > > > > - No functional changes to the binding itself (keep Reviewed-by).
> > > > > > > > > - Update commit messages and patch titles to reflect the move.
> > > > > > > > > - Link to v7: https://lore.kernel.org/linux-leds/20250422190121.46839-1-trannamatk@gmail.com/
> > > > > > > >
> > > > > > > > Out of sudden without discussing with auxdisplay maintainers/reviewers?
> > > > > > > > Thanks, no.
> > > > > > > > Please, put into the cover letter the meaningful summary of what's
> > > > > > > > going on and why this becomes an auxdisplay issue. Brief review of the
> > > > > > > > bindings sounds more likely like LEDS or PWM subsystems.
> > > > > > >
> > > > > > > It is 4x3 matrix. That means it is not suitable for LEDs. I don't
> > > > > > > believe it is suitable for PWM, either -- yes, it is 36 PWM outputs,
> > > > > > > but...
> > > > > >
> > > > > > Is it intended to be used as a 4x3 matrix, or is this just an internal
> > > > > > wiring detail, and should it be exposed as 12 individual LEDs instead?
> > > > >
> > > > > The 4×3 matrix is a real and fundamental aspect of the LP5812’s operation.
> > > > > It is not just an internal wiring detail.
> > > > > The device adopts a Time-Cross-Multiplexing (TCM) structure, where 4 output
> > > > > pins control 12 LED dots individually through scanning. Each pin includes
> > > > > both high-side and low-side drive circuits, meaning matrix multiplexing is
> > > > > required for proper operation — it cannot be treated as 12 completely
> > > > > independent LEDs.
> > > >
> > > > Scanning is really a detail.
> > > >
> > > > If this is used as rectangular 4x3 display, then it goes to auxdisplay.
> > > >
> > > > If this is used as a power LED, SD activity LED, capslock and numlock
> > > > ... placed randomly all around the device, then it goes LED subsystem.
> > >
> > > The LP5812 is used for LED status indication in devices like smart speakers,
> > > wearables, and routers, not as a structured rectangular display.
> > >
> > > Given that, it seems to match the LED subsystem better than auxdisplay, doesn't it?
> >
> > I have mixed feelings about all this. As per hardware organisation it
> > sounds more like a matrix (for example. keyboard), where all entities
> > are accessed on a scanline, but at the same time each of the entities
> > may have orthogonal functions to each other. Have you checked with DRM
> > for the sake of completeness?
> > Personally I lean more to the something special, which doesn't fit
> > existing subsystems. Auxdisplay subsystem more or less about special
> > alphanumeric displays (with the exception of some FB kinda devices,
> > that were even discussed to have drivers be removed). Also maybe FB
> > might have something suitable, but in any case it looks quite
> > non-standard...
>
> I understand your mixed feelings about where the LP5812 fits within
> the existing subsystems.
>
> While the LP5812 uses a matrix-based structure for controlling LEDs,
> it is not intended for displaying structured text or graphics. Instead,
> it controls up to 4 RGB LEDs for status indication, where each RGB LED
> consists of 3 individual color LEDs: red, green, and blue. Based on this,

So, you probably should have started with this. As I read above that
this has to reside in drivers/leds/rgb for colour ones which seems to
me closest to your case. On top you might add an upper level
management to prevent users from using patterns whenever the LEDs are
requested individually. So, this driver should represent 4 RGB leds
and, possibly, the upper layer with those fancy stuff like breathing.

At least, based on the above it's my formal NAK from an auxdisplay perspective.

> I think it aligns more closely with the LED subsystem rather than DRM or FB.

Right.

-- 
With Best Regards,
Andy Shevchenko

Powered by blists - more mailing lists