Re: [PATCH v3 1/3] backlight: aw99706: Fix DT property names to match binding

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [High] The driver attempts to read the I2C CHIPID register before configuring and asserting the hardware enable GPIO.
- [High] The `update_status` callback directly uses `bl->props.brightness` and fails to account for backlight blanking or suspend power states.
- [High] The initial `default-brightness` parsed from the device tree is never applied to the hardware.
--

--- Patch [1]: [PATCH v3 1/3] backlight: aw99706: Fix DT property names to match binding ---
commit 0780fe78c9307963271e82c84a46a86db0bcde59
Author: Junjie Cao <[email protected]>

backlight: aw99706: Fix DT property names to match binding

The driver reads four tuning properties without the unit suffixes that
the binding mandates. Fix by aligning the property name strings in
aw99706_dt_props[] with the binding.

> diff --git a/drivers/video/backlight/aw99706.c b/drivers/video/backlight/aw99706.c
> index 18299faf06ad4..e130f164303a2 100644
> --- a/drivers/video/backlight/aw99706.c
> +++ b/drivers/video/backlight/aw99706.c
> @@ -130,23 +130,23 @@ static const struct aw99706_dt_prop aw99706_dt_props[] = {
>  		AW99706_CFG0_REG, AW99706_DIM_MODE_MASK, 1,
>  	},
>  	{
> -		"awinic,sw-freq", aw99706_dt_property_lookup,
> +		"awinic,sw-freq-hz", aw99706_dt_property_lookup,

[ ... ]

[Severity: High]
This is a pre-existing issue, but does aw99706_bl_update_status() handle
backlight blanking or suspend power states?

drivers/video/backlight/aw99706.c:aw99706_bl_update_status() {
	struct aw99706_device *aw = bl_get_data(bl);

	return aw99706_update_brightness(aw, bl->props.brightness);
}

By directly accessing bl->props.brightness instead of using
backlight_get_brightness(bl), the driver ignores BL_CORE_FBBLANK and
BL_CORE_SUSPENDED flags, keeping the screen illuminated when it should be
blanked.

[Severity: High]
This is a pre-existing issue, but can the device probe successfully if the
I2C read occurs before the hardware enable GPIO is asserted?

drivers/video/backlight/aw99706.c:aw99706_probe() {
	...
	ret = aw99706_chip_id_read(aw);
	if (ret != AW99706_ID)
		return dev_err_probe(dev, -ENODEV,
				     "Unknown chip id 0x%02x\n", ret);

	aw99706_dt_parse(aw, &props);

	aw->hwen_gpio = devm_gpiod_get(aw->dev, "enable", GPIOD_OUT_LOW);
	...
}

If the bootloader does not leave the enable GPIO asserted, the device will be
powered off or in reset, rendering I2C communication impossible.

[Severity: High]
This is a pre-existing issue, but is the initial default-brightness applied
to the hardware?

drivers/video/backlight/aw99706.c:aw99706_probe() {
	...
	bl_dev = devm_backlight_device_register(dev, "aw99706-backlight", dev,
						aw, &aw99706_bl_ops, &props);
	if (IS_ERR(bl_dev))
		return dev_err_probe(dev, PTR_ERR(bl_dev),
				     "Failed to register backlight!\n");

	aw->bl_dev = bl_dev;

	return 0;
}

The probe routine successfully parses default-brightness and registers the
backlight device, but doesn't call backlight_update_status(bl_dev) to sync
the parsed initial brightness to the hardware registers.

Without an explicit initial brightness update to overwrite the hardware reset
state, the display remains off upon boot until userspace manually sets the
brightness.

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