Re: [PATCH v2 5/9] ARM: tegra: lg-x3: Add flash LEDs controller node

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] dt-bindings: inaccurate modeling of single physical flash LED as two nodes
--

commit a769a0c3851e51b75e86359c4dd2c87f24520282
Author: Svyatoslav Ryhel <clamor95@gmail.com>

ARM: tegra: lg-x3: Add flash LEDs controller node

This commit adds a device tree node for the TI LM3559 LED controller used for
the flash LEDs in the LG X3 device. It also hooks up the new LED nodes to
the rear camera flash properties.

> diff --git a/arch/arm/boot/dts/nvidia/tegra30-lg-x3.dtsi b/arch/arm/boot/dts/nvidia/tegra30-lg-x3.dtsi
> index 4cd2bbc94f9d7..99afd5d634e2b 100644
> --- a/arch/arm/boot/dts/nvidia/tegra30-lg-x3.dtsi
> +++ b/arch/arm/boot/dts/nvidia/tegra30-lg-x3.dtsi
> @@ -1233,6 +1233,7 @@ rear-camera@10 {
>  			rotation = <90>;
>  
>  			nvmem = <&m24c08>;
> +			flash-leds = <&lm3559_led0>, <&lm3559_led1>;

[Severity: Medium]
Does linking two separate flash nodes here force user-space applications to
coordinate both to achieve the intended brightness?

The V4L2 framework will expose two separate flash subdevices for a single
physical LED. Any user-space app using the camera flash will encounter this
quirk.

>  			lens-focus = <&dw9714>;
>  
>  			assigned-clocks = <&tegra_car TEGRA30_CLK_VI_SENSOR>,

[ ... ]

> @@ -1267,6 +1268,35 @@ m24c08: eeprom@50 {
>  			vcc-supply = <&vio_1v8_rear>;
>  		};
>  
> +		led-controller@53 {
> +			compatible = "ti,lm3559";
> +			reg = <0x53>;
> +
> +			enable-gpios = <&gpio TEGRA_GPIO(BB, 3) GPIO_ACTIVE_HIGH>;
> +			vin-supply = <&vdd_3v3_vbat>;
> +
> +			flash-max-timeout-us = <1024000>;
> +			ti,peak-current-microamp = <1400000>;
> +
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +
> +			/* single flash led is on 2 lines */
> +			lm3559_led0: led@0 {
> +				reg = <0>;

[Severity: Medium]
Does modeling this as two distinct logical LED nodes inaccurately describe the
physical hardware?

The comment explicitly acknowledges the hardware topology is one physical LED
connected to two output lines. Device Tree principles dictate that the DT must
accurately describe the physical hardware structure.

Should a single physical LED be modeled as a single node with
led-sources = <0 1>; instead of two separate nodes? While the lm3560 driver
may currently lack support for led-sources, modeling hardware based on
software driver limitations is generally discouraged.

> +
> +				flash-max-microamp = <393750>;
> +				led-max-microamp = <140625>;
> +			};
> +
> +			lm3559_led1: led@1 {
> +				reg = <1>;
> +
> +				flash-max-microamp = <393750>;
> +				led-max-microamp = <140625>;
> +			};
> +		};

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261004154456.145362-1-clamor95@gmail.com?part=5