[PATCH] ipq40xx: google (gale) add LED aliases

[Brian Norris]

Hi,

On Mon, Mar 27, 2023 at 07:29:39AM +0200, openwrt at aiyionpri.me wrote:
> From: Jan-Niklas Burfeind <git at aiyionpri.me>
> 
> this is similar to
> commit 583ac0e11df7 ("mpc85xx: update lp5521 led-controller node for
> 5.10")
> 
> Google uses white for running and red for an issue
> 
> Signed-off-by: Jan-Niklas Burfeind <git at aiyionpri.me>
> ---
> Good morning.
> 
> Currently the tristate LED does not have an assigned function,
> while it does have several in stock firmware.
> 
> Once the multi-intensity section becomes active, the device should use
> white/blueish light as running indicator.
> For now blue is used for LED_RUNNING.
> 
> If there's a way to use white instead of blue, just let me know;
> I'd gladly fix that at once.

I'll admit, I wasn't familiar with these led-* aliases (and the custom
package/base-files/files/lib/functions/leds.sh that parses them) until
now. But it looks like they only support a single LED for a given
function, so while we might like to enable the red, blue and green (to
get white), that doesn't seem possible without switching over the to
full multi-color LED device tree binding. But then, I don't think LUCI
knows how to configure multi-color LEDs yet.

I guess the choices you made here are better than the "nothing" that is
currently here, so:

Reviewed-by: Brian Norris <computersforpeace at gmail.com>

One more note below:

> --- a/target/linux/ipq40xx/files/arch/arm/boot/dts/qcom-ipq4019-wifi.dts
> +++ b/target/linux/ipq40xx/files/arch/arm/boot/dts/qcom-ipq4019-wifi.dts

> @@ -255,12 +259,13 @@
>  		clock-mode = /bits/ 8 <1>;
>  
>  #if 1
> -		led at 0 {
> +		led0_red: led at 0 {
>  			reg = <0>;
>  			chan-name = "LED0_Red";
>  			led-cur = /bits/ 8 <0x64>;
>  			max-cur = /bits/ 8 <0x78>;
>  			color = <LED_COLOR_ID_RED>;
> +			function = LED_FUNCTION_FAULT;

Are you sure this 'function' property is actually doing anything? I know
the common DT bindings provide this, but I'm pretty sure the lp5523
driver doesn't actually use it for anything -- it derives its LED names
from either 'chan-name' or as a combination of a few other properties
('label', channel number, etc.).

I guess it's not wrong, per se, to include it, but I did purposely only
specify the properties that made sense for this driver.

Same comment applies to the other 'function' uses.

Brian

>  		};
>  
>  		led at 1 {
> @@ -271,12 +276,13 @@
>  			color = <LED_COLOR_ID_GREEN>;
>  		};
>  
> -		led at 2 {
> +		led0_blue: led at 2 {
>  			reg = <2>;
>  			chan-name = "LED0_Blue";
>  			led-cur = /bits/ 8 <0x64>;
>  			max-cur = /bits/ 8 <0x78>;
>  			color = <LED_COLOR_ID_BLUE>;
> +			function = LED_FUNCTION_POWER;
>  		};
>  #else
>  		/*
> @@ -285,6 +291,7 @@
>  		 * # echo 255 > /sys/class/leds/tricolor/brightness
>  		 */
>  		multi-led at 2 {
> +			function = LED_FUNCTION_POWER;
>  			reg = <2>;
>  			color = <LED_COLOR_ID_RGB>;
>  			#address-cells = <1>;
> -- 
> 2.40.0
>