Re: [PATCH v6 11/11] arm64: dts: qcom: generalmobile-shamrock: new device

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] arm64: dts: qcom: shamrock: missing color property on status LEDs breaks RGB
--

commit 8fec21f5539dc733f3dc08f3078a4a994d335e11
Author: Muzaffer Kadir <muzafferkadir@mainlining.org>

arm64: dts: qcom: generalmobile-shamrock: new device

This commit adds initial device tree support for the Qualcomm MSM8952-based
General Mobile GM 5 Plus (shamrock) smartphone.

> diff --git a/arch/arm64/boot/dts/qcom/msm8952-generalmobile-shamrock.dts b/arch/arm64/boot/dts/qcom/msm8952-generalmobile-shamrock.dts
> new file mode 100644
> index 0000000000000..26a269f0f7694
> --- /dev/null
> +++ b/arch/arm64/boot/dts/qcom/msm8952-generalmobile-shamrock.dts
> @@ -0,0 +1,439 @@
[ ... ]
> +	multi-led {
> +		compatible = "leds-group-multicolor";
> +		color = <LED_COLOR_ID_RGB>;
> +		function = LED_FUNCTION_INDICATOR;
> +		leds = <&red_status>, <&green_status>, <&blue_status>;
> +	};
[ ... ]
> +	led-controller@68 {
> +		compatible = "si-en,sn3193";
> +		reg = <0x68>;
> +
> +		shutdown-gpios = <&tlmm 68 GPIO_ACTIVE_HIGH>;
> +
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		red_status: led@1 {
> +			reg = <1>;
> +			label = "red:status";
> +			led-max-microamp = <17500>;
> +		};
> +
> +		green_status: led@2 {
> +			reg = <2>;
> +			label = "green:status";
> +			led-max-microamp = <17500>;
> +		};
> +
> +		blue_status: led@3 {
> +			reg = <3>;
> +			label = "blue:status";
> +			led-max-microamp = <17500>;
> +		};
> +	};

[Severity: Medium]
Should the child LED nodes under led-controller@68 use the color and function
properties instead of the deprecated label property?

When is31fl319x_probe() registers each child LED via
devm_led_classdev_register_ext(), led_classdev_register_ext() in
drivers/leds/led-class.c only populates led_cdev->color when the color
property is present on the node:

	if (fwnode_property_present(init_data->fwnode, "color"))
		fwnode_property_read_u32(init_data->fwnode, "color",
					 &led_cdev->color);

Because red_status, green_status, and blue_status only specify label and omit
color, led_cdev->color remains 0 (LED_COLOR_ID_WHITE) for all three LEDs.

When leds_gmc_probe() in drivers/leds/rgb/leds-group-multicolor.c subsequently
probes the multi-led node, it sets each channel's color_index from
led_cdev->color:

	for (i = 0; i < count; i++) {
		struct led_classdev *led_cdev = priv->monochromatics[i];

		subled[i].color_index = led_cdev->color;

		/* Configure the LED intensity to its maximum */
		subled[i].intensity = max_brightness;
	}

Without color = <LED_COLOR_ID_RED>, <LED_COLOR_ID_GREEN>, and
<LED_COLOR_ID_BLUE> on led@1, led@2, and led@3, multi_index in sysfs reports
"white white white" instead of "red green blue".

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261028-msm8952-initial-support-v6-0-e07ddd8bcf4c@mainlining.org?part=11