Message-ID: <20240205105717.bmppb3xalmmqapqg@CAB-WSD-L081021>
Date: Mon, 5 Feb 2024 13:57:17 +0300
From: Dmitry Rokosov <ddrokosov@...utedevices.com>
To: Greg Kroah-Hartman <gregkh@...uxfoundation.org>, Lee Jones
	<lee@...nel.org>, Pavel Machek <pavel@....cz>
CC: Martin Kurbanov <mmkurbanov@...utedevices.com>, Jacek Anaszewski
	<jacek.anaszewski@...il.com>, Rob Herring <robh+dt@...nel.org>, Krzysztof
 Kozlowski <krzysztof.kozlowski+dt@...aro.org>, Conor Dooley
	<conor+dt@...nel.org>, Jonathan Corbet <corbet@....net>, Andy Shevchenko
	<andy.shevchenko@...il.com>, <linux-kernel@...r.kernel.org>,
	<linux-leds@...r.kernel.org>, <devicetree@...r.kernel.org>,
	<linux-doc@...r.kernel.org>, <kernel@...utedevices.com>
Subject: Re: [PATCH v1 1/2] leds: aw200xx: support for hw pattern controllers

Hello Greg, Lee and Pavel,

Apologies for the ping, but I would appreciate it if you could spare a
couple of minutes to decide on the next steps. From my perspective, the
problems I previously described persist, and we need to discuss the
possible solutions.

On Mon, Jan 29, 2024 at 05:13:39PM +0300, Dmitry Rokosov wrote:
> Hello Greg,
> 
> On Fri, Jan 26, 2024 at 06:09:43AM -0800, Greg Kroah-Hartman wrote:
> > 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@...utedevices.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.
> > 
> 
> By referring to the "synchronization problem," I am highlighting the
> following issue:
> 
> The LED controller incorporates PAGE registers for accessing multiple
> registers, which can be explained with the analogy of a "window." When
> the driver needs to update the brightness value or perform any other
> action, it must select the appropriate PAGE window to access the
> register offset. It is challenging to believe that the driver,
> responsible for adjusting brightness values, can be synchronized with
> raw userspace I2C transactions in any manner. However, synchronization
> is necessary because the hardware pattern employs the same PAGE
> registers used for brightness and other parameter setups.
> 
> In the kernel driver, this issue is addressed through the simple mutex
> usage.
> 
> > > 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.
> > 
> 
> The PAGE window registers, which I described a few lines earlier, are
> relevant. Regarding interrupts, the AW200XX LED controller incorporates
> a hardware pattern event based on a straightforward GPIO-connected
> interrupt line. It's advisable to asynchronously wait for the hardware
> pattern ending event using methods like poll() and to notify userspace
> using sysfs_notify().
> 
> > > 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.
> > 
> 
> Currently, we have two methods for playing LED animations:
> 1) Direct sysfs access to the appropriate brightness node.
> 2) LED-triggered pattern with hrtimer.
> 
> The first method always runs a separate kworker on each access,
> resulting in slow performance and complete CPU involvement.  The second
> method is straightforward in terms of hardware pattern mechanism
> synchronization, but it lacks specific support for the appropriate
> hardware (as mentioned earlier). In this option, the CPU is not entirely
> engaged because we can free the CPU from the waiting task until the LED
> pattern is executed.
> 
> > > 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

-- 
Thank you,
Dmitry

Powered by blists - more mailing lists