Re: [PATCH v4 3/5] docs: ABI: Document LP5812 LED sysfs interfaces

[Nam Tran]

On Sun, 6 Apr 2025, Krzysztof Kozlowski wrote:

> On 05/04/2025 20:32, Nam Tran wrote:
> > The LP5812 is a 4 × 3 matrix RGB LED driver
> > with autonomous animation engine control.
> > 
> > The driver provides interfaces to configure
> > LED modes manual/autonomous, set PWM/DC values,
> > and manage autonomous animation engines.
> > 
> > Signed-off-by: Nam Tran <trannamatk@xxxxxxxxx>
> > ---
> >  .../ABI/testing/sysfs-bus-i2c-devices-lp5812  | 150 ++++++++++++++++++
> >  MAINTAINERS                                   |   1 +
> >  2 files changed, 151 insertions(+)
> >  create mode 100644 Documentation/ABI/testing/sysfs-bus-i2c-devices-lp5812
> > 
> > diff --git a/Documentation/ABI/testing/sysfs-bus-i2c-devices-lp5812 b/Documentation/ABI/testing/sysfs-bus-i2c-devices-lp5812
> > new file mode 100644
> > index 000000000000..e745f0f936c5
> > --- /dev/null
> > +++ b/Documentation/ABI/testing/sysfs-bus-i2c-devices-lp5812
> > @@ -0,0 +1,150 @@
> > +What:		/sys/bus/i2c/devices/.../lp5812_chip_setup/device_enable
> 
> I do not see reason for such ABI. If you want to disable, just unbind it.

I think the name is confusing, it is Chip_EN register. It is used to "enable the internal circuits".
The hardware supports low power consumption.
When we write 0 to the bit, it will save all configurations and go to save power mode.
When we write 1, it will restore. Therefore, I support the end user in controlling this register.

> > +Date:		April 2025
> 
> Not possible...

Sorry, I understand now. Should I use the current date or the date when the patch is accepted?

> > +KernelVersion:	6.14
> 
> You cannot go to the past. 6.14 was released. This will be v6.16 or later.

It is my mistake. I will update the KernelVersion to v6.16 or later accordingly.

> > +Contact:	Nam Tran <trannamatk@xxxxxxxxx>
> > +Description:
> > +        Enables or disables the LP5812 device. (RW)
> > +        0 - Disable
> > +        1 - Enable
> > +
> > +What:		/sys/bus/i2c/devices/.../lp5812_chip_setup/dev_config
> 
> Looks like wrong path here and everywhere else. I think other name it as
> led driver, e.g.
> Documentation/ABI/testing/sysfs-class-led-driver-lm3533

The LP5812 driver is basically an I2C driver.
It doesn't work as current common or multi-LEDs supported by the Kernel framework.
However, the LP5812 driver still supports LED functions.

> > +Date:		April 2025
> > +KernelVersion:	6.14
> > +Contact:	Nam Tran <trannamatk@xxxxxxxxx>
> > +Description:
> > +        Configures drive mode and scan order. (RW)
> > +        Some valid values: tcmscan:4:0:1:2:3 (default), tcmscan:3:0:1:2, mixscan:2:2:0:3, mixscan:3:0:1:2:3
> > +
> > +What:		/sys/bus/i2c/devices/.../lp5812_chip_setup/device_command
> > +Date:		April 2025
> > +KernelVersion:	6.14
> > +Contact:	Nam Tran <trannamatk@xxxxxxxxx>
> > +Description:
> > +        Issues device-level commands. (WO)
> > +        Valid values: "update", "start", "stop", "pause", "continue"
> > +
> > +What:		/sys/bus/i2c/devices/.../lp5812_chip_setup/device_reset
> > +Date:		April 2025
> > +KernelVersion:	6.14
> > +Contact:	Nam Tran <trannamatk@xxxxxxxxx>
> > +Description:
> > +        Triggers a software reset of the device. (WO)
> > +        1 - resets device
> > +        0 - does not reset device
> 
> I do not see kernel exposing it for other devices, drop.

This is sw_reset register of hardware. It is used to request the hardware reset.
I think I will change the name to "sw_reset" to make it clearly.

> > +
> > +What:		/sys/bus/i2c/devices/.../lp5812_chip_setup/fault_clear
> > +Date:		April 2025
> > +KernelVersion:	6.14
> > +Contact:	Nam Tran <trannamatk@xxxxxxxxx>
> > +Description:
> > +        Clears fault status. (WO)
> > +        1 - clears fault status
> > +        0 - does not clear fault status
> > +
> > +What:		/sys/bus/i2c/devices/.../lp5812_chip_setup/tsd_config_status
> > +Date:		April 2025
> > +KernelVersion:	6.14
> > +Contact:	Nam Tran <trannamatk@xxxxxxxxx>
> > +Description:
> > +        Report the current thermal shutdown config status. (RO)
> > +
> > +What:		/sys/bus/i2c/devices/.../led_<id>/enable
> > +Date:		April 2025
> > +KernelVersion:	6.14
> > +Contact:	Nam Tran <trannamatk@xxxxxxxxx>
> > +Description:
> > +        Enables or disables the specified LED channel. (RW)
> > +        1 - Enable
> > +        0 - Disable
> 
> No, you already have standard ABI for this. I also already told you that
> you cannot duplicate existing kernel interface.

According to function of led and register, I will change it to "activate" interface.

> > +
> > +What:		/sys/bus/i2c/devices/.../led_<id>/mode
> > +Date:		April 2025
> > +KernelVersion:	6.14
> > +Contact:	Nam Tran <trannamatk@xxxxxxxxx>
> > +Description:
> > +        Selects LED operation mode. (RW)
> > +        Valid values: "manual", "autonomous"
> > +
> > +What:		/sys/bus/i2c/devices/.../led_<id>/manual_dc
> > +Date:		April 2025
> > +KernelVersion:	6.14
> > +Contact:	Nam Tran <trannamatk@xxxxxxxxx>
> > +Description:
> > +        DC current level in manual mode. (RW)
> > +        Valid values: 0 - 255
> 
> NAK, duplicating existing brightness.

There are some brighness mode in this hardware. manual_dc is a interface to to control dc current in manual mode.

> > +
> > +What:		/sys/bus/i2c/devices/.../led_<id>/manual_pwm
> > +Date:		April 2025
> > +KernelVersion:	6.14
> > +Contact:	Nam Tran <trannamatk@xxxxxxxxx>
> > +Description:
> > +        PWM duty cycle in manual mode. (RW)
> > +        Valid values: 0 - 255
> > +> +What:		/sys/bus/i2c/devices/.../led_<id>/autonomous_dc
> > +Date:		April 2025
> > +KernelVersion:	6.14
> > +Contact:	Nam Tran <trannamatk@xxxxxxxxx>
> > +Description:
> > +        DC current level used in autonomous mode. (RW)
> > +        Valid values: 0 - 255
> > +
> > +What:		/sys/bus/i2c/devices/.../led_<id>/autonomous_dc
> > +Date:		April 2025
> > +KernelVersion:	6.14
> > +Contact:	Nam Tran <trannamatk@xxxxxxxxx>
> > +Description:
> > +        DC current level used in autonomous mode. (RW)
> > +        Valid values: 0 - 255
> 
> Also duplicating brigthness.

It is not brightness only. It is another registers.
It is used for autonomous_dc that mean the led will blink with several brighness levels automotically.

> > +
> > +What:		/sys/bus/i2c/devices/.../led_<id>/pwm_dimming_scale
> > +Date:		April 2025
> > +KernelVersion:	6.14
> > +Contact:	Nam Tran <trannamatk@xxxxxxxxx>
> > +Description:
> > +        PWM dimming scale type. (RW)
> > +        Valid values: "linear", "exponential"
> > +
> > +What:		/sys/bus/i2c/devices/.../led_<id>/pwm_phase_align
> > +Date:		April 2025
> > +KernelVersion:	6.14
> > +Contact:	Nam Tran <trannamatk@xxxxxxxxx>
> > +Description:
> > +        Configures PWM phase alignment. (RW)
> > +        Valid values: "forward", "middle", "backward"
> > +
> > +What:		/sys/bus/i2c/devices/.../led_<id>/autonomous_animation
> > +Date:		April 2025
> > +KernelVersion:	6.14
> > +Contact:	Nam Tran <trannamatk@xxxxxxxxx>
> > +Description:
> > +        Controls AEU configuration and playback. (RW)
> > +        Format: (aeu number):(start pause time):(stop pause time):(playback time)
> > +        with aeu number 1, 2, 3; playback time 0 - 15
> > +
> > +What:		/sys/bus/i2c/devices/.../led_<id>/aep_status
> > +Date:		April 2025
> > +KernelVersion:	6.14
> > +Contact:	Nam Tran <trannamatk@xxxxxxxxx>
> > +Description:
> > +        Shows current animation pattern status, value from 0 to 7. (RO)
> > +
> > +What:		/sys/bus/i2c/devices/.../led_<id>/auto_pwm_val
> > +Date:		April 2025
> > +KernelVersion:	6.14
> > +Contact:	Nam Tran <trannamatk@xxxxxxxxx>
> > +Description:
> > +        Shows the pwm value in autonomous mode when pause the animation, value from 0 to 255. (RO)
> > +
> > +What:		/sys/bus/i2c/devices/.../led_<id>/lod_lsd
> > +Date:		April 2025
> > +KernelVersion:	6.14
> > +Contact:	Nam Tran <trannamatk@xxxxxxxxx>
> > +Description:
> > +        0 0 mean no lod and lsd fault detected, 1 1 mean lod and lsd fault detected (RO)
> > +
> > +
> > +
> > +
> > +
> > +
> 
> Why so many blank lines? Drop

It's my mistake. I will remove the blank lines.

Thanks for your detailed review.
Appreciate your time and feedback!

Best regards,
Nam Tran