Re: [PATCH RFC] leds: leds-lp50xx: Handle reg to get correct multi_index

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

Hi Johan,

On 5/13/25 15:04, Johan Adolfsson wrote:
> 
> Hi Jacek,
> 
>> From: Jacek Anaszewski <jacek.anaszewski@gmail.com>
>> Sent: Monday, May 12, 2025 20:10
>> To: Johan Adolfsson; Lee Jones; Pavel Machek
>> Cc: linux-leds@vger.kernel.org; linux-kernel@vger.kernel.org; Kernel
>> Subject: Re: [PATCH RFC] leds: leds-lp50xx: Handle reg to get correct multi_index
>>
>> On 5/12/25 12:59, Johan Adolfsson wrote:
>>>
>>> From: Jacek Anaszewski <jacek.anaszewski@gmail.com>
>>> Sent: Saturday, May 10, 2025 17:32
>>> To: Johan Adolfsson; Lee Jones; Pavel Machek
>>> Cc: linux-leds@vger.kernel.org; linux-kernel@vger.kernel.org; Kernel
>>> Subject: Re: [PATCH RFC] leds: leds-lp50xx: Handle reg to get correct multi_index
>>>
>>>> Hi Johan,
>>>>
>>>> On 5/6/25 12:39, Johan Adolfsson wrote:
>>>>> mc_subled used for multi_index needs well defined array indexes,
>>>>> to guarantee the desired result, optionally use reg for that.
>>>>>
>>>>> If devicetree child nodes is processed in random or reverse order
>>>>> you may end up with multi_index "blue green red" instead of the expected
>>>>> "red green blue".
>>>>> If user space apps uses multi_index to deduce how to control the leds
>>>>> they would most likely be broken without this patch if devicetree
>>>>> processing is reversed (which it appears to be).
>>>>
>>>> Are you trying to solve some real problem that occurred to you?
>>>
>>> Yes! Since the subled is indexed by the processing order, it gets reversed if devicetree processing is reversed
>>> (which I understand is a "feature"), so instead of "red green blue" I would get "blue green red" in the multi_index file without this patch.
>>> The mapping to the hardware does not match that, so writing "255 0 0" to multi_intensity will give me red and not blue.
> 
>> You are expected to write intensities to the multi_intensity file
>> according to the order of colors listed in multi_index file.
> 
> That is what I did above, but LED turned red and not blue.
> 
>>>> The order of DT nodes parsing is not a problem here - we save
>>>> color index in subled_info to be able to figure out which color
>>>> is on which position. This information can be retrieved in sysfs
>>>> by reading multi_index file.
>>>
>>> Maybe the bug is somewhere else in the leds-lp50xx if that is supposed to work,
>>> but I still think it's a good thing to be able to get the expected order in the multi_index file.
>>
>> Please make sure you read Documentation/leds/leds-class-multicolor.rst.
>> There is no such term as "expected order in the multi_index file".
>> The framework is called multicolor, not rgb. The order of colors does
>> not need to be RGB.
> 
> Maybe I'm missing something here - but how do i specify that a certain pin on the driver IC is connected to a certain color of the LED.
> The devicetree looks like this:
> It seems the first number in multi_intensity seem to go to pin regardless of what multi_index says.

OK, indeed there is a problem. Let's continue in your patch thread.

> 
> 			multi-led@0 {
> 				reg = <0x0>;
> 				color = <LED_COLOR_ID_RGB>;
> 				label = "led:rgb";
> 				function = "led";
> 				linux,default-trigger = "default-on";
> 				max-brightness = <255>;
> 				#address-cells = <1>;
> 				#size-cells = <0>;
> 
> 				/* Need BLUE GREEN RED here or reg to give red green blue in multi_index! */
> 				led-0@0 {
> 					color = <LED_COLOR_ID_RED>;
> 					reg = <0>;
> 				};
> 
> 				led-1@1 {
> 					color = <LED_COLOR_ID_GREEN>;
> 					reg = <1>;
> 				};
> 
> 				led-2@2 {
> 					color = <LED_COLOR_ID_BLUE>;
> 					reg = <4>;

This needs to match node-name[@unit-address], i.e. here 2.

> 				};
> 			};
> 
-- 
Best regards,
Jacek Anaszewski