Re: [PATCH v5 2/2] arm64: dts: qcom: Add support for Acer Swift Go Pro AI (SFA14-11)

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] arm64: dts: qcom: x1e78100-acer-sfa14-11: inverted backlight turns full on at 0
--

commit 66fa11a1de0967d5bc8b8fce1f271477c9fa4b31
Author: Kaipeng Zeng <kaipeng94@gmail.com>

arm64: dts: qcom: Add support for Acer Swift Go Pro AI (SFA14-11)

This commit adds the device tree for the Acer Swift Go Pro AI (SFA14-11)
laptop based on the Qualcomm Snapdragon X Elite (X1E78100) SoC.

> diff --git a/arch/arm64/boot/dts/qcom/x1e78100-acer-sfa14-11.dts b/arch/arm64/boot/dts/qcom/x1e78100-acer-sfa14-11.dts
> new file mode 100644
> index 0000000000000..5814cf3d000ce
> --- /dev/null
> +++ b/arch/arm64/boot/dts/qcom/x1e78100-acer-sfa14-11.dts
> @@ -0,0 +1,1541 @@
[ ... ]
> +	backlight: backlight {
> +		compatible = "pwm-backlight";
> +
> +		pwms = <&pm8550_pwm 3 5000000>;
> +
> +		brightness-levels = <254 225 200 175 150 125 100 75 65 60 55>;
> +		default-brightness-level = <8>;
> +
> +		power-supply = <&vreg_edp_bl>;
> +	};
[ ... ]
> +	vreg_edp_bl: regulator-edp-bl {
> +		compatible = "regulator-fixed";
> +
> +		regulator-name = "VBL9";
> +		regulator-min-microvolt = <3600000>;
> +		regulator-max-microvolt = <3600000>;
> +
> +		regulator-always-on;
> +		regulator-boot-on;
> +	};

[Severity: Medium]
Does setting the backlight brightness to 0, blanking the display, or
suspending the system cause this active-low backlight to jump to full
brightness instead of turning off?

The backlight node uses descending brightness-levels from 254 (off) down to
55 (maximum brightness) without PWM_POLARITY_INVERTED, has no enable-gpios,
and points power-supply at vreg_edp_bl, which has no GPIO and is marked
regulator-always-on.

In drivers/video/backlight/pwm_bl.c, pwm_backlight_update_status() only
consults pb->levels[brightness] via compute_duty_cycle() when brightness > 0.
When brightness is 0, it ignores pb->levels[0] and instead disables the
regulator and sets duty_cycle = 0:

drivers/video/backlight/pwm_bl.c:pwm_backlight_update_status() {
	if (brightness > 0) {
		pwm_get_state(pb->pwm, &state);
		state.duty_cycle = compute_duty_cycle(pb, brightness, &state);
		state.enabled = true;
		pwm_apply_might_sleep(pb->pwm, &state);

		pwm_backlight_power_on(pb);
	} else {
		pwm_backlight_power_off(pb);

		pwm_get_state(pb->pwm, &state);
		state.duty_cycle = 0;
		state.enabled = !pb->power_supply && !pb->enable_gpio;
		pwm_apply_might_sleep(pb->pwm, &state);
	}
}

Because vreg_edp_bl is a GPIO-less fixed regulator with regulator-always-on,
regulator_disable() in pwm_backlight_power_off() does not cut power to the
backlight, while lpg_pwm_apply() in drivers/leds/rgb/leds-qcom-lpg.c disables
the PWM output channel and drives it to a constant logical 0 (0% duty cycle),
which corresponds to full brightness on an active-low backlight.

In addition, having descending values in brightness-levels causes unsigned
integer wraparound in pwm_backlight_is_linear():

drivers/video/backlight/pwm_bl.c:pwm_backlight_is_linear() {
	unsigned int nlevels = data->max_brightness + 1;
	unsigned int min_val = data->levels[0];
	unsigned int max_val = data->levels[nlevels - 1];
	unsigned int slope = (128 * (max_val - min_val)) / nlevels;
	unsigned int margin = (max_val - min_val) / 20; /* 5% */
	...
}

With min_val = 254 and max_val = 55, max_val - min_val wraps around in
unsigned arithmetic, inflating margin and misreporting the non-linear table
as BACKLIGHT_SCALE_LINEAR.

Note that pm8550_pwm in hamoa-pmics.dtsi uses #pwm-cells = <2>, which does
not accept a third PWM_POLARITY_INVERTED flag cell unless #pwm-cells is
overridden to <3> or the driver/DT handling is adjusted.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261023180513.36417-1-kaipeng94@gmail.com?part=2