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

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

Hi Johan,

On 5/15/25 09:36, Johan Adolfsson wrote:
> Hi Jacek,
> 
>> From: Jacek Anaszewski <jacek.anaszewski@gmail.com>
>> Sent: Wednesday, May 14, 2025 21:55
>> 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/14/25 16:34, Johan Adolfsson wrote:
>> From: Jacek Anaszewski <jacek.anaszewski@gmail.com>
>> Sent: Tuesday, May 13, 2025 21:52
>> 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
>> ..
>>>>                         }
>>>> +                     ret = fwnode_property_read_u32(led_node, "reg", &multi_index);
>>>> +                     if (ret) {
>>>> +                             multi_index = num_colors;
>>>
>>> Why not to fail if 'reg' parsing fails?
>>> It is marked required in DT bindings [0].
>>
>>> I didn't want to start failing if reg is missing since it has never been handled until now, despite what the doc says since 2022...
>>
>> There is one in-tree user [0], and we will need to patch it as well,
>> because it uses reg 0,1,2 for each RGB LED module, instead of iout
>> numbers as it will be after your change.
> 
> Not sure i follow you here. What works for me is starting with reg = <0> for the childs of each bank (multi-led).
> The register (maps to iout I guess) is calculated based on led_number * 3 + i where i is the offset as well as the color_index in subled_info (reg).
> So I don't think additional changes to that dts is needed.

OK, if we document that reg properly as an index of the LED within
given RGB LED module. Initially I thought that it would be better to map
it directly to the IOUT, but the way you propose will allow to keep
existing DTS unchanged.

>> We will need to also state clearly in the bindings that 'reg' property
>> maps to iouts for the non-banked RGB LED modules.
> 
> Not sure it actually does, haven't really tested that, or at least not gotten it to work as I expected.
> For the non-rgb led in the product I requested those to be on the first pin on each bank (wasting 2 pins per bank),
> since I couldn't figure out how to configure it in another way when prototyping.
> 
> Since the 3 outputs per bank share a single brightness setting, it could complicate things as well.
> (And we didn't need all the pins anyway)

Here I was talking about non-banked LEDs. But regarding banked LEDs,
setting LEDn_Bank_EN bits assigns all three LEDs from given RGB LED
module to the bank. Then all IOUTs from the module are controlled via
BANK_A/B/C_Color registers, and by common Bank_Brightness register.

>> For banked RGB LED modules it is more tricky, because there is one
>> LED multicolor class device created for them. Probably to be correct
>> we would need make the 'reg' properties in the subnodes also arrays
>> reflecting iouts that will be governed by BANK_A_Color, BANK_B_Color,
>> and BANK_C_Color registers respectively. And DT parser in the driver
>> would need to enforce proper iout definition for the banked modules
> 
>> E.g. the multi-led@3 node from the example should look like below:
>>
>>              multi-led@3 {
>>                  #address-cells = <1>;
>>                  #size-cells = <0>;
>>                  reg = <0x3>, <0x4>, <0x5>;
>>                  color = <LED_COLOR_ID_RGB>;
>>                  function = LED_FUNCTION_STANDBY;
>>
>>                  led@9 {
>>                      reg = <0x9>, <0xc>, <0xf>;
>>                      color = <LED_COLOR_ID_RED>;
>>                  };
>>
>>                  led@a {
>>                      reg = <0xa>, <0xd>, <0x10>;
>>                      color = <LED_COLOR_ID_GREEN>;
>>                  };
>>
>>                  led@b {
>>                      reg = <0xb>, <0xe>, <0x11>;
>>                      color = <LED_COLOR_ID_BLUE>;
>>                  };
>>
>>
>> [0] arch/arm/boot/dts/aspeed/aspeed-bmc-facebook-fuji.dts
> 
> Not sure about that, I dont' think we handle an array of reg values within the led node.

Above I proposed to implement handling of it. But, if we are not going
to treat subLED's reg properties as IOUTs, but as an index of the LED
within RGB LED module, then we should change the bindings example for
the banked case to the following, i.e. have subLED's reg also 0,1,2:

             multi-led@3 {
                 #address-cells = <1>;
                 #size-cells = <0>;
                 reg = <0x3>, <0x4>, <0x5>;
                 color = <LED_COLOR_ID_RGB>;
                 function = LED_FUNCTION_STANDBY;

                 led@0 {
                     reg = <0x0>;
                     color = <LED_COLOR_ID_RED>;
                 };

                 led@1 {
                     reg = <0x1>;
                     color = <LED_COLOR_ID_GREEN>;
                 };

                 led@2 {
                     reg = <0x2>;
                     color = <LED_COLOR_ID_BLUE>;
                 };
            };


-- 
Best regards,
Jacek Anaszewski