Re: [PATCH v8 0/5] auxdisplay: add support for TI LP5812 4x3 Matrix LED driver

[Lee Jones <lee@kernel.org>]

On Mon, 12 May 2025, Andy Shevchenko wrote:

> On Mon, May 12, 2025 at 8:38 PM Nam Tran <trannamatk@gmail.com> wrote:
> > On Mon, 12 May 2025 Andy Shevchenko wrote:
> > > On Sat, May 10, 2025 at 02:48:02PM +0700, Nam Tran wrote:
> > > > On Thu, 8 May 2025 Lee Jones wrote:
> > > > > On Thu, 08 May 2025, Andy Shevchenko wrote:
> > > > > > On Thu, May 8, 2025 at 5:27 PM Nam Tran <trannamatk@gmail.com> wrote:
> > > > > > > On Thu, 8 May 2025 Lee Jones wrote:
> > > > > > > > On Thu, 08 May 2025, Andy Shevchenko wrote:
> 
> ...
> 
> > > > I think setting PWM also same as brightness_set API. However, there are
> > > > many PWM config for a LED and it is one of other config to make autonomous mode work.
> > > > Therefore, standard led API can use in some use cases only.
> > > >
> > > > Please see the link below for a better visualization of how to configure the LP5812.
> > > > https://dev.ti.com/gallery/view/LED/LP581x/ver/0.10.0/
> > >
> > > To me it sounds like you should start from the small steps, i.e. do not
> > > implement everything at once. And starting point of the 4 RGB LEDs sounds
> > > the best approach to me. Then, if needed, you can always move on with
> > > fancy features of this hardware on top of the existing code.
> >
> > Thanks for the suggestion.
> > I understand your point and agree that starting with standard LED APIs is the preferred approach.
> >
> > However, the LP5812 hardware offers more advanced features, and I’d like to support end users all
> > features as shown in the link: https://dev.ti.com/gallery/view/LED/LP581x/ver/0.10.0/.
> > It is easy for end user to investigate and use driver.
> 
> I see. Good luck then!
> 
> > If I want to keep the current driver interface to meet this expectation, would it be acceptable
> > to move it to the misc subsystem to better support the hardware?
> 
> Don't ask me, I don't know what you want at the end and I have not so
> much time to invest in this anymore. Only what I'm sure about I
> already expressed in the previous replies and emails.

Briefly spoke to Greg (now Cc:ed).  We can all discuss this together.

My 2c, whilst falling short of deep-diving, is as follows:

1. No one is going to enjoy reviewing a 3k line submission!

   - Instead, submit the smallest driver you can whilst still retaining
     functionality.  It is not good practice to fully enable a
     complicated driver such as this in a single submission - let alone
     a single patch.

2. Hand-rolling your own API from scratch is to be highly discouraged.

   - There seems to be quite a bit of overlap in functionality between
     your bespoke API and LED's.  The LED subsystem already supports a
     lot of what's being implemented here.  Instead of letting the user
     configure the device directly, why not offer some high level
     options and attempt to infer the rest.  If possible, the complexity
     of the device should be handed by driver, rather than placing the
     onus on the user.

3. Shoving this into Misc because you don't want to use the APIs that
   are already offered to you is not a good approach.

-- 
Lee Jones [李琼斯]