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. > 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
