| Message ID | 20231207125938.175119-1-mmkurbanov@salutedevices.com |
|---|---|
| Headers | show |
| Series | leds: aw200xx: support for hw pattern controllers | expand |
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.
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?
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?
Hi! > > 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. If you do git grep hw_pattern, you should get pointers to qcom-lpg driver that solves similar problem, with interface that should be acceptable. Best regards, Pavel
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@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. > 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
Hello Pavel, On Sat, Jan 27, 2024 at 12:21:48AM +0100, Pavel Machek wrote: > Hi! > > > > 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. > > If you do git grep hw_pattern, you should get pointers to qcom-lpg > driver that solves similar problem, with interface that should be > acceptable. Thank you for pointing that out. Yes, it's a very similar situation to ours. But I haven't observed the merging of this driver. Was it encountering similar issues with the sysfs interface?