drivers: led: lp50xx fix write_channels

[allemanm]

Fix implementation of write_channels of the lp50xx driver.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[pdgendt]

Ping @Mani-Sadhasivam @simonguinot

[simonguinot]

Ping @Mani-Sadhasivam @simonguinot

pong @pdgendt :)

Did you test it with the lp50xx sample ?

[pdgendt]

Did you test it with the lp50xx sample ?

The sample isn't really useful. We simply use the channels to control the different outputs of the LP5018 controller.

	lp5018@3c {
		compatible = "ti,lp5018";
		reg = <0x3c>;
		enable-gpios = <&gpio1 14 (GPIO_ACTIVE_HIGH | GPIO_PULL_DOWN)>;
		log-scale-en;
	};

And confirmed that we can control the individual out# pins

uart:~$ led set_channel lp5018@3c 0 255
lp5018@3c: setting channel 0 to 255
uart:~$ led set_channel lp5018@3c 5 123
lp5018@3c: setting channel 5 to 123
uart:~$ led write_channels lp5018@3c 8 128 128 128 128
lp5018@3c: writing from channel 8: 128 128 128 128

Which isn't possible without this PR.

[simonguinot]

Hi @allemanm,

First, let me apologize for the delay. When I saw this patch, I thought it was suspect and that I should do some testing in order to check it. But unfortunately, I never got to the stage where I look through my cabinet full of boards to find an LP50xx evaluation board :)

And I still haven't. But as I said, I find this patch suspect. Please see my comment below.

In addition, at the very least, you should explain the bug or behavior you are trying to fix. "fix write_channel" is way to vague.

[simonguinot]

Please can you explain what you are fixing ?

I find this change highly suspicious. For each module, there are three color registers and one brightness register, so LP50XX_COLORS_PER_LED + 1 makes sense. And the ->write_channels() API also exposes the bank registers. And that's (nmodules) + 1.

So I don't understand :)

[simonguinot]

Did you test it with the lp50xx sample ?

The sample isn't really useful. We simply use the channels to control the different outputs of the LP5018 controller.

	lp5018@3c {
		compatible = "ti,lp5018";
		reg = <0x3c>;
		enable-gpios = <&gpio1 14 (GPIO_ACTIVE_HIGH | GPIO_PULL_DOWN)>;
		log-scale-en;
	};

And confirmed that we can control the individual out# pins

uart:~$ led set_channel lp5018@3c 0 255
lp5018@3c: setting channel 0 to 255
uart:~$ led set_channel lp5018@3c 5 123
lp5018@3c: setting channel 5 to 123
uart:~$ led write_channels lp5018@3c 8 128 128 128 128
lp5018@3c: writing from channel 8: 128 128 128 128

Which isn't possible without this PR.

Well, there is something wrong with this patch I think. The LP50XX_MAX_CHANNELS macro in the current code looks correct. Note that the bank registers are also exposed by the ->write_channel() API. So probably that an offset (of 4 ?) is needed with led shell commands to access the LED color registers. Did you take this into account with your testing ?

Note that the lp50xx sample provides a run_channel_test function. It relies on the LP50XX_LED_BRIGHT_CHAN_BASE and LP50{12,24,36}_LED_COL_CHAN_BASE macros defined in lp50xx.h. So it is probably the right way to test...

[pdgendt]

Well, there is something wrong with this patch I think. The LP50XX_MAX_CHANNELS macro in the current code looks correct. Note that the bank registers are also exposed by the ->write_channel() API. So probably that an offset (of 4 ?) is needed with led shell commands to access the LED color registers. Did you take this into account with your testing ?

Maybe we're just failing to understand how the API is supposed to work with this controller. We are under the impression that channels map directly to the OUT# pins of the IC, is this a false assumption?

Note that the lp50xx sample provides a run_channel_test function. It relies on the LP50XX_LED_BRIGHT_CHAN_BASE and LP50{12,24,36}_LED_COL_CHAN_BASE macros defined in lp50xx.h. So it is probably the right way to test...

This might be part of what we fail to understand.

We could go with adding individual LED nodes to the device tree, but we're not controlling color LEDs, but simple white LEDs. Is that possible with the current implementation to define this?

[simonguinot]

Well, there is something wrong with this patch I think. The LP50XX_MAX_CHANNELS macro in the current code looks correct. Note that the bank registers are also exposed by the ->write_channel() API. So probably that an offset (of 4 ?) is needed with led shell commands to access the LED color registers. Did you take this into account with your testing ?

Maybe we're just failing to understand how the API is supposed to work with this controller. We are under the impression that channels map directly to the OUT# pins of the IC, is this a false assumption?

I think that the ->write_channels API is not well documented for the lp50xx driver. That's the issue. The only information is in lp50xx.h.

So yes, your assumption is wrong :) This driver exposes all the registers, starting with the bank registers (LP50XX_BANK_CHAN_BASE). The idea was to use the ->write_channels API to give control to users over bank registers, which are quite powerful but not accessible through other LED API methods.

Note that the lp50xx sample provides a run_channel_test function. It relies on the LP50XX_LED_BRIGHT_CHAN_BASE and LP50{12,24,36}_LED_COL_CHAN_BASE macros defined in lp50xx.h. So it is probably the right way to test...

This might be part of what we fail to understand.

We could go with adding individual LED nodes to the device tree, but we're not controlling color LEDs, but simple white LEDs. Is that possible with the current implementation to define this?

Is that related with #85208 ? And yes you can use the ->write_channels API to handle that case.

[pdgendt]

Closing as it is clear this limits the (ab)use of write_channels in the LP50XX driver.

Adding an offset of 12 (LP5024_LED_COL_CHAN_BASE) allows for setting the individual OUT# pins.

[simonguinot]

Sorry again for the delay...