Re: [PATCH v4 1/9] leds: multicolor: Add sysfs interface definition

[Jacek Anaszewski <jacek.anaszewski@gmail.com>]

Hi Dan,

Thank you for the v4.

I have a bunch of comments below. Please take a look.

On 7/25/19 8:28 PM, Dan Murphy wrote:
> Add a documentation of LED Multicolor LED class specific
> sysfs attributes.
> 
> Signed-off-by: Dan Murphy <dmurphy@ti.com>
> ---
>  .../ABI/testing/sysfs-class-led-multicolor    | 67 +++++++++++++++++++
>  1 file changed, 67 insertions(+)
>  create mode 100644 Documentation/ABI/testing/sysfs-class-led-multicolor
> 
> diff --git a/Documentation/ABI/testing/sysfs-class-led-multicolor b/Documentation/ABI/testing/sysfs-class-led-multicolor
> new file mode 100644
> index 000000000000..59839f0eae76
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-class-led-multicolor
> @@ -0,0 +1,67 @@
> +What:		/sys/class/leds/<led>/brightness
> +Date:		Sept 2019
> +KernelVersion:	TBD
> +Contact:	Dan Murphy <dmurphy@ti.com>
> +Description:	read/write
> +		The multicolor class will redirect the device drivers call back
> +		function for brightness control to the multicolor class
> +		brightness control function.
> +
> +		Writing to this file will update all LEDs within the group to a
> +		calculated percentage of what each color LED in the group is set
> +		to.  Please refer to the leds-class-multicolor.txt in the
> +		Documentation directory for a complete description.

Instead of redirecting the reader to led-class-multicolor.txt I'd prefer
to have at least the formula to calculate the colors laid out here.
Aside of that - it is more helpful to have a full path to the referenced
file.

> +
> +		The value of the color is from 0 to
> +		/sys/class/leds/<led>/max_brightness.
> +
> +What:		/sys/class/leds/<led>/colors/color_mix
> +Date:		Sept 2019
> +KernelVersion:	TBD
> +Contact:	Dan Murphy <dmurphy@ti.com>
> +Description:	read/write
> +		The color_mix file allows writing all registered multicolor LEDs
> +		virtually at the same time.  The value(s) written to this file

I'd drop parentheses form "value(s)". Multi color LED class device is
supposed to always have more then one LED. And if I understand it
correctly we have to pass intensities of all colors supported by LED
multicolor class device here, even we're changing single one.

> +		contain the intensity values for each multicolor LED within
> +		the colors directory.  The color indexes are reported in the
> +		color_id file as defined in this document.

This is a bit misleading. It sounds as if single color_id file would be
reporting more than one index.

> +		Please refer to the leds-class-multicolor.txt in the
> +		Documentation directory for a complete description.

Here, similarly as for brightness, I would prefer to have complete
documentation of this file.

How about:

The values written to this file should contain the intensity values of
each multicolor LED within the colors directory. The index of given
color is reported by the color_id file present in colors/<color>
directory. The index determines the position in the sequence of
intensities on which the related intensity should be passed to this
file.

And here we could have the examples from leds-class-multicolor.txt.

> +
> +What:		/sys/class/leds/<led>/colors/<led_color>/color_id
> +Date:		Sept 2019
> +KernelVersion:	TBD
> +Contact:	Dan Murphy <dmurphy@ti.com>
> +Description:	read only
> +		This file when read will return the index of the color in the
> +		color_mix.
> +		Please refer to the leds-class-multicolor.txt in the
> +		Documentation directory for a complete description.
> +
> +What:		/sys/class/leds/<led>/colors/<led_color>/intensity
> +Date:		Sept 2019
> +KernelVersion:	TBD
> +Contact:	Dan Murphy <dmurphy@ti.com>
> +Description:	read/write
> +		The led_color directory is dynamically created based on the
> +		colors defined by the registrar of the class.
> +		The led_color can be but not limited to red, green, blue,
> +		white, amber, yellow and violet.  Drivers can also declare a

Instead of this vague sentence about the available colors I propose to
maintain the list of supported colors in leds-class.rst or in a separate
file and keep it in sync with the led_colors array. Then we could refer
to that file here.

> +		LED color for presentation.  There is one directory per color

I'd not let drivers define their custom colors. It would entail issues
related to lack of generic LED_COLOR_ID and DT parsing failure.

> +		presented.  The brightness file is created under each
> +		led_color directory and controls the individual LED color
> +		setting.
> +
> +		The value of the color is from 0 to
> +		/sys/class/leds/<led>/colors/<led_color>/max_intensity.
> +
> +What:		/sys/class/leds/<led>/colors/<led_color>/max_intensity
> +Date:		Sept 2019
> +KernelVersion:	TBD
> +Contact:	Dan Murphy <dmurphy@ti.com>
> +Description:	read only
> +		Maximum intensity level for the LED color, default is
> +		255 (LED_FULL).
> +
> +		If the LED does not support different intensity levels, this
> +		should be 1.
> 

-- 
Best regards,
Jacek Anaszewski