linux-kernel - Re: [PATCH v14 0/4] leds: add new LED driver for TI LP5812

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]

Message-Id: <20250922181341.10761-1-trannamatk@gmail.com>
Date: Tue, 23 Sep 2025 01:13:41 +0700
From: Nam Tran <trannamatk@...il.com>
To: gregkh@...uxfoundation.org
Cc: lee@...nel.org,
	pavel@...nel.org,
	rdunlap@...radead.org,
	christophe.jaillet@...adoo.fr,
	krzk+dt@...nel.org,
	robh@...nel.org,
	conor+dt@...nel.org,
	corbet@....net,
	linux-leds@...r.kernel.org,
	linux-kernel@...r.kernel.org,
	devicetree@...r.kernel.org,
	linux-doc@...r.kernel.org
Subject: Re: [PATCH v14 0/4] leds: add new LED driver for TI LP5812

On Thu, 11 Sep 2025, Greg KH wrote:

> On Sun, Sep 07, 2025 at 11:09:40PM +0700, Nam Tran wrote:
> > This patch series adds initial support for the TI LP5812,
> > a 4x3 matrix RGB LED driver with autonomous engine control.
> > This version provides a minimal, clean implementation focused
> > on core functionality only. The goal is to upstream a solid
> > foundation, with the expectation that additional features can
> > be added incrementally in future patches.
> > 
> > The driver integrates with the LED multicolor framework and
> > supports a set of basic sysfs interfaces for LED control and
> > chip management.
> > 
> > Signed-off-by: Nam Tran <trannamatk@...il.com>
> 
> The sysfs api is really odd here.  WHy not do the same thing as this
> other controller recently submitted does:
> 	https://lore.kernel.org/r/20250911-v6-14-topic-ti-lp5860-v3-0-390738ef9d71@pengutronix.de

Thank you for the feedback!
I agree that consistency is important, and I've reviewed the patch you referenced.

I also checked the LP5860 datasheet and noticed that its driver exposes sysfs entries
for configuring registers like `R_current_set`, `G_current_set`, and `B_current_set`.
Similarly, the LP5812 requires register-level configuration for operation.

In my driver, I've implemented the following sysfs attributes:
- '/sys/bus/i2c/devices/.../lp5812_chip_setup/dev_config' - Configures drive mode and
scan order (Dev_Config_1 and Dev_Config_2 registers).
- '/sys/bus/i2c/devices/.../lp5812_chip_setup/sw_reset' - Triggers a software reset of
the device (Reset register).
- '/sys/bus/i2c/devices/.../lp5812_chip_setup/fault_clear' - Clears fault status
(Fault_Clear register).
- '/sys/class/leds/led_<id>/activate' - Activate or deactivate the specified LED channel
in runtime (led_en_1, led_en_2 registers).
- '/sys/class/leds/led_<id>/led_current' - To change DC/PWM current level of each led
(Manual_DC_xx and Manual_PWM_xx registers).
- '/sys/class/leds/led_<id>/max_current' - To show max current setting (Dev_Config_0 register).
- '/sys/class/leds/led_<id>/lod_lsd' - To read lod, lsd status of each LED
(LOD_Status_0, LOD_Status_1, LSD_Status_0, LSD_Status_1 registers).

These attributes map directly to LP5812 registers. I’ve kept the interface minimal and
focused only on essential functionality needed to operate the device.

If any of these attributes seem unconventional or redundant, I’d appreciate clarification
so I can revise accordingly.

> but better yet, why does this need to be a kernel driver at all?  Why
> can't you just control this directly from userspace with a program
> there?

LP5812 is controlled via I2C, and its register map is non-trivial. Moving control to userspace
would require users to manually handle I2C transactions and understand the register layout,
which is error-prone and not user-friendly.

Moreover, the driver integrates with the LED multicolor framework, allowing standardized control
via existing userspace tools. This abstraction is difficult to achieve reliably from userspace alone.

> For USB, we generally do not allow these types of crazy apis to be added
> to the kernel when controlling the device can be done from userspace.  I
> think the same thing can happen here too, right?

USB devices benefit from standardized descriptors and interfaces, which reduce the need for custom
sysfs APIs. In contrast, LP5812 has no such standard interface, and some customization is necessary.

I’m open to improving the sysfs interface or moving parts to another method if that’s more appropriate.
Please let me know which specific changes you’d recommend.

For completeness, I considered these methods:
- sysfs: Recommended and standard for LED drivers.
- i2c-tools: Not recommended, intended for development/debug only.
- ioctl: Not recommended for new LED drivers.
- debugfs: For debugging only.
- Direct I2C register access: Requires users to know the register map and protocol.

Thanks again for the review!

Best regards,
Nam Tran