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@xxxxxxxxxxxxxxxxx> > > > > > >> --- > > > > > >> .../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
