Re: [PATCH v31 06/12] leds: lp55xx: Add multicolor framework support to lp55xx

From: Dan Murphy <dmurphy@ti.com>
To: Pavel Machek <pavel@ucw.cz>
Cc: <jacek.anaszewski@gmail.com>, <robh@kernel.org>,
	<marek.behun@nic.cz>, <devicetree@vger.kernel.org>,
	<linux-leds@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
	<linux-arm-kernel@lists.infradead.org>
Subject: Re: [PATCH v31 06/12] leds: lp55xx: Add multicolor framework support to lp55xx
Date: Wed, 22 Jul 2020 11:40:08 -0500	[thread overview]
Message-ID: <a92226e0-4388-722b-7447-c9d3d3222e0f@ti.com> (raw)
In-Reply-To: <20200721211135.GE5966@amd>

Pavel

On 7/21/20 4:11 PM, Pavel Machek wrote:
> Hi!
>
>> Add multicolor framework support for the lp55xx family.
>>
>> Acked-by: Pavel Machek <pavel@ucw.cz>
>> Acked-by: Jacek Anaszewski <jacek.anaszewski@gmail.com>
>> Signed-off-by: Dan Murphy <dmurphy@ti.com>
> Applied 4,5,6 and 8,9.
>
>>   config LEDS_LP55XX_COMMON
>>   	tristate "Common Driver for TI/National LP5521/5523/55231/5562/8501"
>> -	depends on LEDS_LP5521 || LEDS_LP5523 || LEDS_LP5562 || LEDS_LP8501
>> +	depends on LEDS_CLASS_MULTICOLOR || !LEDS_CLASS_MULTICOLOR
>> +	depends on OF
>>   	select FW_LOADER
>>   	select FW_LOADER_USER_HELPER
> But I have to ask: what does this do to userland interface once
> LEDS_CLASS_MULTICOLOR is enabled?
If the DT is instrumented with MC FW properties there will be a change 
to the user land interface.  If the properties follow the LED properties 
then there should be no change to the userland interface.  See the DT 
and user interface examples below.  So the n900 should see no delta in 
the lighting since it has the LED properties and not the MC FW 
properties.  I hope this answers your question.
> Will users see some changes? Will they see some changes after dts
> parts are applied?

In my testing I did not see any delta in the user interface.  I tested 
with both MC FW and non-MC FW properties on the LP5523 evm.

So the legacy DT's should not be affected and should work as is. The DT 
patches were to make the DT's compliant with the updated bindings.

Only the u8500 defconfig patch needs to be added or the u8500_defconfig 
will break in compilation so we need that patch as well.

DTS changes not applied

With multicolor framework properties in the DT

ls
beaglebone:green:heartbeat  beaglebone:green:usr2 lp5523:channel1
beaglebone:green:mmc0       beaglebone:green:usr3 lp5523:channel5

         chan5 {
             color = <LED_COLOR_ID_WHITE>;
             chan-name = "lp5523:channel5";
             reg = <0x5>;
             led-cur = /bits/ 8 <50>;
             max-cur = /bits/ 8 <100>;
         };

         multi-led@0 {
             #address-cells = <1>;
             #size-cells = <0>;
             reg = <0>;
             color = <LED_COLOR_ID_MULTI>;
             function = LED_FUNCTION_STANDBY;
             linux,default-trigger = "heartbeat";

             led@0 {
                 led-cur = /bits/ 8 <50>;
                 max-cur = /bits/ 8 <100>;
                 reg = <0x0>;
                 color = <LED_COLOR_ID_GREEN>;
             };

             led@1 {
                 led-cur = /bits/ 8 <50>;
                 max-cur = /bits/ 8 <100>;
                 reg = <0x1>;
                 color = <LED_COLOR_ID_BLUE>;
             };

             led@6 {
                 led-cur = /bits/ 8 <50>;
                 max-cur = /bits/ 8 <100>;
                 reg = <0x6>;
                 color = <LED_COLOR_ID_RED>;
             };
         };

lp5523:channel5# ls

brightness      device          led_current     max_brightness 
max_current     power           subsystem       trigger uevent

ls lp5523\:channel1
brightness       device           max_brightness multi_index      
multi_intensity  power subsystem        trigger          uevent

Without MC FW DT properties

And as individual LEDs as the DTs are populated today.

ls
beaglebone:green:heartbeat  beaglebone:green:usr2 lp5523:channel0 
lp5523:channel5
beaglebone:green:mmc0       beaglebone:green:usr3 
lp5523:channel2             lp5523:channel6

         chan0 {
             color = <LED_COLOR_ID_GREEN>;
             chan-name = "lp5523:channel0";
             reg = <0x0>;
             led-cur = /bits/ 8 <50>;
             max-cur = /bits/ 8 <100>;
         };
         chan7 {
             color = <LED_COLOR_ID_RED>;
             chan-name = "lp5523:channel6";
             reg = <0x6>;
             led-cur = /bits/ 8 <50>;
             max-cur = /bits/ 8 <100>;
         };
         chan2 {
             color = <LED_COLOR_ID_BLUE>;
             chan-name = "lp5523:channel2";
             reg = <0x1>;
             led-cur = /bits/ 8 <50>;
             max-cur = /bits/ 8 <100>;
         };

         chan5 {
             color = <LED_COLOR_ID_WHITE>;
             chan-name = "lp5523:channel5";
             reg = <0x5>;
             led-cur = /bits/ 8 <50>;
             max-cur = /bits/ 8 <100>;
         };

Dan