Re: [PATCH v26 03/15] leds: multicolor: Introduce a multicolor class definition

[Dan Murphy <dmurphy@ti.com>]

Pavek

Thanks for the review

On 6/6/20 10:53 AM, Pavel Machek wrote:
> Hi!
>
>> Introduce a multicolor class that groups colored LEDs
>> within a LED node.
>>
>> The multi color class groups monochrome LEDs and allows controlling two
>> aspects of the final combined color: hue and lightness. The former is
>> controlled via the intensity file and the latter is controlled
>> via brightness file.
>>
>> Acked-by: Jacek Anaszewski <jacek.anaszewski@gmail.com>
>> Signed-off-by: Dan Murphy <dmurphy@ti.com>
>> diff --git a/Documentation/ABI/testing/sysfs-class-led-multicolor b/Documentation/ABI/testing/sysfs-class-led-multicolor
>> new file mode 100644
>> index 000000000000..7d33a82a4b07
>> --- /dev/null
>> +++ b/Documentation/ABI/testing/sysfs-class-led-multicolor
>> @@ -0,0 +1,34 @@
>> +What:		/sys/class/leds/<led>/brightness
>> +Date:		March 2020
>> +KernelVersion:	5.8
>> +Contact:	Dan Murphy <dmurphy@ti.com>
>> +Description:	read/write
>> +		Writing to this file will update all LEDs within the group to a
>> +		calculated percentage of what each color LED intensity is set
>> +		to. The percentage is calculated for each grouped LED via the
>> +		equation below:
>> +		led_brightness = brightness * multi_intensity/max_brightness
>> +
>> +		For additional details please refer to
>> +		Documentation/leds/leds-class-multicolor.rst.
>> +
>> +		The value of the color is from 0 to
>> +		/sys/class/leds/<led>/max_brightness.
> It is not too clear to me what "color" means here.
>
> It would be worth mentioning that this is single integer.

OK I will update this


>> +What:		/sys/class/leds/<led>/multi_index
>> +Date:		March 2020
>> +KernelVersion:	5.8
>> +Contact:	Dan Murphy <dmurphy@ti.com>
>> +Description:	read
>> +		The multi_index array, when read, will output the LED colors
>> +		by name as they are indexed in the multi_intensity file.
> This should specify that it is array of strings.

Yeah this sounds better


>> +What:		/sys/class/leds/<led>/multi_intensity
>> +Date:		March 2020
>> +KernelVersion:	5.8
>> +Contact:	Dan Murphy <dmurphy@ti.com>
>> +Description:	read/write
>> +		Intensity level for the LED color within the array.
>> +		The intensities for each color must be entered based on the
>> +		multi_index array.
> I'd mention here that it is array of integers, and what the maximum
> values are.

Same here.  I will indicate max value cannot exceed max_brightness

But that was what the max_intensity file was for in prior patchsets.

>
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -9533,6 +9533,14 @@ F:	Documentation/devicetree/bindings/leds/
>>   F:	drivers/leds/
>>   F:	include/linux/leds.h
>>   
>> +LED MULTICOLOR FRAMEWORK
>> +M:	Dan Murphy <dmurphy@ti.com>
>> +L:	linux-leds@vger.kernel.org
> I'd like to be mentioned here, too. "M: Pavel Machek
> <pavel@ucw.cz>". And I'm not sure if I should be taking MAINTAINER
> file update through a LED tree. Should definitely go to separate
> patch.

Oh definitely.  I thought it was implied that you and Jacek are both 
Maintainers as well.

I will add you but will wait to see if Jacek wants to be added.

I will separate this out and make it a separate patch


>
>> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
>> index 9cdc4cfc5d11..fe7d90d4fa23 100644
>> --- a/drivers/leds/Kconfig
>> +++ b/drivers/leds/Kconfig
>> @@ -30,6 +30,16 @@ config LEDS_CLASS_FLASH
>>   	  for the flash related features of a LED device. It can be built
>>   	  as a module.
>>   
>> +config LEDS_CLASS_MULTI_COLOR
>> +	tristate "LED MultiColor LED Class Support"
> "LED MultiColor Class Support"

OK.

Dan

>
> Best regards,
> 									Pavel