Re: [PATCH v1 1/2] leds: aw200xx: support for hw pattern controllers

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

On Fri, Jan 26, 2024 at 03:23:10PM +0300, Dmitry Rokosov wrote:
> Hello Lee,
> 
> On Thu, Jan 25, 2024 at 01:00:49PM +0000, Lee Jones wrote:
> > Looping in Jacek (LEDS) and Greg (SYFS) for some knowledgable input.
> > 
> > On Fri, 12 Jan 2024, Martin Kurbanov wrote:
> > > On 21.12.2023 19:10, Lee Jones wrote:
> > > > On Thu, 07 Dec 2023, Martin Kurbanov wrote:
> > > > 
> > > >> This led-controller supports 3 pattern controllers for auto breathing or
> > > >> group dimming control. Each pattern controller can work in auto
> > > >> breathing or manual control mode. All breathing parameters including
> > > >> rising/falling slope, on/off time, repeat times, min/max brightness
> > > >> and so on are configurable.
> > > >>
> > > >> Signed-off-by: Martin Kurbanov <mmkurbanov@salutedevices.com>
> > > >> ---
> > > >>  .../testing/sysfs-class-led-driver-aw200xx    | 108 +++
> > > >>  Documentation/leds/leds-aw200xx.rst           | 274 ++++++++
> > > >>  drivers/leds/leds-aw200xx.c                   | 649 ++++++++++++++++++
> > > >>  3 files changed, 1031 insertions(+)
> > > >>  create mode 100644 Documentation/leds/leds-aw200xx.rst
> > > > 
> > > > This interface is bananas.  Exposing an entire register interface to
> > > > sysfs does not sit will with me at all.  When we add support to a sysfs
> > > > class, we usually require it to be generic and work across all devices.
> > > > Adding device specific interfaces is generally decried and to be
> > > > avoided.  Don't forget, once we commit something to sysfs, it becomes
> > > > ABI and we have to support it forever.
> > > > 
> > > > A far better approach would be to add support for this in userspace
> > > > instead  You can use the standard I2C character device API to achieve
> > > > the same result.  That way we don't have the same level of commitment
> > > > and is generally a much more flexible/future-proof.
> > > > 
> > > 
> > > I used sysfs similarly to other LED drivers (for example, leds-lm3533).
> > > Additionally, the controller has interrupts about the completion of the pattern,
> > > which is best to handle in the kernel. In the case of implementation in user
> > > mode, there may be synchronization problems, as the controller has several
> > > memory pages that can be switched by writing the page number to register 0xF0.
> > 
> > leds-lm3533 is a 12 year old legacy exception AND has less than half of
> > the sysfs exports proposed here.  What makes aw200xx so different it
> > needs to an incomparable interface to any other that we currently
> > support?
> 
> >From my point of view, direct I2C raw requests from userspace are not a
> good solution as well due to synchronization problems, as Martin
> mentioned in the previous message.

Sorry, I missed this, what is the synchronization problem?  This is an
led, shouldn't have any real specific performance issues.

> We have honestly been attempting to integrate this functionality into
> the official LED pattern interface, but it cannot be achieved due to the
> absence of this interface's functionality:
> 1) Page-based access
> 2) Interrupts

I don't understand this, sorry.

> HW patterns are very useful mechanism to draw animation faster without
> any interactions with CPU, so I think we need to find the best architect
> approach for its integration.

The CPU is totally involved here, that should be identical.

> What is an alternative way to access such a hardware pattern interface?
> Debugfs? Or perhaps we should consider extending the LED pattern
> interface?

Or again, userspace.

debugfs is for debugging.

thanks,

greg k-h