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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Fri, 12 Jan 2024 14:43:16 +0300
From: Martin Kurbanov <mmkurbanov@...utedevices.com>
To: Lee Jones <lee@...nel.org>
CC: Pavel Machek <pavel@....cz>, 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>, Greg Kroah-Hartman
	<gregkh@...uxfoundation.org>
Subject: Re: [PATCH v1 1/2] leds: aw200xx: support for hw pattern controllers

Hello Lee,

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.

-- 
Best Regards,
Martin Kurbanov