Re: [PATCH v2 1/2] led: lp5860: expose fault state via sysfs

[Lee Jones]

On Wed, 11 Mar 2026, Steffen Trumtrar wrote:

> Return the fault state to the userspase via sysfs and allow to reset it.

"userspace" is misspelled here and in the subject line.

> 
> The LP5860 has a global fault state, that just indicates that a short or
> open fault was detected on any LED. This is exposed via 'fault_state'.
> 
> The 'fault_state_open' exposes the LED name and channel where an open
> condition was detected.
> 
> The 'fault_state_short' exposes the LED name and channel where a short
> condition was detected.
> 
> To: Mark Brown <broonie@xxxxxxxxxx>
> Cc: linux-spi@xxxxxxxxxxxxxxx

These should be below the --- marker.

> Signed-off-by: Steffen Trumtrar <s.trumtrar@xxxxxxxxxxxxxx>
> ---
>  Documentation/ABI/testing/sysfs-class-spi-lp5860 | 49 ++++++++++++++++++++++++

You're the first write driver documentation like this.

That has to tell you something.

>  1 file changed, 49 insertions(+)
> 
> diff --git a/Documentation/ABI/testing/sysfs-class-spi-lp5860 b/Documentation/ABI/testing/sysfs-class-spi-lp5860
> new file mode 100644
> index 0000000000000..31082bd78f51e
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-class-spi-lp5860
> @@ -0,0 +1,49 @@
> +What:           /sys/class/spi_master/spi<bus>/spi<bus>.<dev>/fault_state

I believe this path is incorrect. Attributes for an SPI slave device should
be under `/sys/bus/spi/devices/spi<bus>.<dev>/`, not under the master
device's class directory.

> +Date:           March 2026
> +KernelVersion:  7.0
> +Contact:        Steffen Trumtrar <kernel@xxxxxxxxxxxxxx>

This is different to your sign-off address.

> +Description:
> +	Contains and sets the global fault state:
> +
> +	* 3: Open and short detected
> +	* 2: Open detected
> +	* 1: Short detected

Exposing a raw bitmask like this is not a good ABI. The sysfs convention is
"one value per file". It would be better to have separate read-only files
like `fault_short` and `fault_open` which would contain "1" if a fault is
active and "0" otherwise.

> +
> +	Can be cleared by writing the corresponding value back to fault_state.

This "write back what you read" mechanism is non-standard and racy. A
better approach is to provide a separate write-only `fault_clear` file, or
allow writing '0' to the individual fault files to clear them.

> +
> +	Example usage::
> +
> +		## Read
> +		# cat /sys/class/spi_master/spi<bus>/spi<bus>.<dev>/fault_state
> +		2
> +
> +		## Write
> +		# echo 2 > /sys/class/spi_master/spi<bus>/spi<bus>.<dev>/fault_state
> +
> +What:           /sys/class/spi_master/spi<bus>/spi<bus>.<dev>/fault_state_open
> +Date:           March 2026
> +KernelVersion:  7.0
> +Contact:        Steffen Trumtrar <kernel@xxxxxxxxxxxxxx>
> +Description:
> +	Contains all LEDs and channels where an open condition was detected.

I'm also really confused by the cross-over here.

Are we documenting SPI behaviour or LED?

> +	The format is ledname:channel.
> +
> +	Example usage::
> +
> +		## Read
> +		# cat /sys/class/spi_master/spi<bus>/spi<bus>.<dev>/fault_state_open
> +		rgb1:0 rgb2:4
> +
> +What:           /sys/class/spi_master/spi<bus>/spi<bus>.<dev>/fault_state_short
> +Date:           March 2026
> +KernelVersion:  7.0
> +Contact:        Steffen Trumtrar <kernel@xxxxxxxxxxxxxx>
> +Description:
> +	Contains all LEDs and channels where a short condition was detected.
> +	The format is ledname:channel.
> +
> +	Example usage::
> +
> +		## Read
> +		# cat /sys/class/spi_master/spi<bus>/spi<bus>.<dev>/fault_state_short
> +		rgb1:0 rgb2:4
> 
> -- 
> 2.51.0
> 

-- 
Lee Jones [李琼斯]