Re: [PATCH 14/16] arm: dts: omap3-pandora-common: backlight: switch to twl4030 pwm and pwm_bl

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Missing TPS61161 hardware initialization quirk causes backlight to fail.
- [High] Concurrent probing of legacy and generic backlight drivers causes I2C register corruption.
- [Medium] Hardware misconfiguration due to unsupported PWM period, leading to artificially restricted brightness range.
--

--- Patch [14]: [PATCH 14/16] arm: dts: omap3-pandora-common: backlight: switch to twl4030 pwm and pwm_bl ---
commit 062b6b894a7997d211a245d15a6bd9071869adad
Author: H. Nikolaus Schaller <[email protected]>

arm: dts: omap3-pandora-common: backlight: switch to twl4030 pwm and pwm_bl

This commit adds a generic pwm-backlight device tree node for the Pandora
backlight. The commit message states this allows removing the pandora_bl
driver and pdata-quirks.

> diff --git a/arch/arm/boot/dts/ti/omap/omap3-pandora-common.dtsi b/arch/arm/boot/dts/ti/omap/omap3-pandora-common.dtsi
> index 13821d926afb0..fea7132ce7c5e 100644
> --- a/arch/arm/boot/dts/ti/omap/omap3-pandora-common.dtsi
> +++ b/arch/arm/boot/dts/ti/omap/omap3-pandora-common.dtsi
> @@ -31,6 +31,14 @@ hfclk_26m: oscillator {
>  		clock-frequency = <26000000>;
>  	};
>  
> +	backlight {
> +		compatible = "pwm-backlight";

[Severity: High]
Will this result in a hardware conflict if the legacy driver isn't actually
removed in this patch?

The commit message mentions this allows removing the legacy pandora_bl driver
and pdata-quirks.c registration, but this commit doesn't remove or
conditionally disable the legacy platform device registration. 

If CONFIG_BACKLIGHT_PANDORA is enabled, both the legacy pandora_bl driver
and the new pwm-backlight driver will probe concurrently. Since pandora_bl
writes directly to I2C registers and bypasses the PWM framework, this dual
driver setup can cause race conditions, register corruption, and duplicate
sysfs entries for the backlight.

[Severity: High]
Does switching to the generic driver cause the backlight to fail on startup?

The legacy pandora_bl driver included a TPS61161 hardware quirk that forced
a 100% duty cycle for 2-10ms at startup. Without this continuous >2.5ms high
pulse, the TPS61161 enters 1-wire detection mode and disables PWM control. 

The generic pwm-backlight driver immediately applies the computed duty cycle
on power-up, which lacks this required hardware initialization sequence.

> +		pwms = <&twl_pwm 0 1922710>;
> +
> +		brightness-levels = <0 150 158 166 174 185 205 230 255>;

[Severity: Medium]
Is this artificially restricted minimum brightness of 150/255 (59%) a
workaround for an unsupported PWM period?

The device tree requests a period of 1922710ns (64-tick mode), but the
pwm-twl driver unconditionally sets TWL_PWM0_ON to 1, forcing 128-tick mode
(~3.9ms period). 

To prevent the longer off-times of this slower frequency from exceeding the
2.5ms threshold that triggers the TPS61161 1-wire mode, it appears the
minimum brightness was raised to 59%. This seems to be a significant
functional regression from the original 14% minimum brightness.

> +		default-brightness-level = <6>;
> +	};
> +
>  	tv: connector {
>  		compatible = "connector-analog-tv";
>  		label = "tv";

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=14