Message-Id: <20250508142648.7978-1-trannamatk@gmail.com>
Date: Thu,  8 May 2025 21:26:48 +0700
From: Nam Tran <trannamatk@...il.com>
To: 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

On Thu, 8 May 2025 Lee Jones wrote:

> On Thu, 08 May 2025, Andy Shevchenko wrote:
> 
> > 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.
> 
> This is fine.
> 
> Just be aware, before you submit to LEDs again, that you need to use
> what is available in the LEDs subsystem to it's fullest, before
> hand-rolling all of your own APIs.  The first submission didn't use a
> single LED API.  This, as before, would be a big NACK also.

Thanks for the clarification.

Just to confirm — the current version of the driver is customized to allow
user space to directly manipulate LP5812 registers and to support the
device’s full feature set. Because of this, it doesn’t follow the standard
LED interfaces.

Given that, would it be acceptable to submit this driver under the misc subsystem instead?

Best regards,
Nam Tran

Powered by blists - more mailing lists