Re: [PATCH v2 3/3] backlight: aw99706: Honor the core blank state in update_status()

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] I2C chip ID read is performed before the hardware enable GPIO is requested and asserted.
- [High] Hardware is manually disabled in `remove()` while the backlight device remains registered via `devm_`, leading to a use-after-disable race.
- [Medium] Missing delay after asserting the enable GPIO before issuing I2C commands.
- [Medium] Incomplete suspend/resume logic leaves hardware enable GPIO asserted.
--

--- Patch [3]: [PATCH v2 3/3] backlight: aw99706: Honor the core blank state in update_status() ---
commit 0ee82864b515a444eccb8c16ea1fd1e52d9895f0
Author: Junjie Cao <junjie.cao@linux.dev>
Date:   Mon Aug 3 14:13:10 2026 +0000

    backlight: aw99706: Honor the core blank state in update_status()
    
    update_status() passes props.brightness straight to the hardware and
    ignores the power/blank state tracked by the core. Writing 4 to the
    bl_power sysfs attribute or blanking the framebuffer therefore leaves
    the backlight lit.
    
    Use backlight_get_brightness(), which returns 0 while the device is
    blanked.
    
    Fixes: 147b38a5ad06 ("backlight: aw99706: Add support for Awinic AW99706 backlight")
    Signed-off-by: Junjie Cao <junjie.cao@linux.dev>

> diff --git a/drivers/video/backlight/aw99706.c b/drivers/video/backlight/aw99706.c
> index dfb4b38b5b4c7..d035ff5af52c6 100644
> --- a/drivers/video/backlight/aw99706.c
> +++ b/drivers/video/backlight/aw99706.c
> @@ -340,7 +340,7 @@ static int aw99706_bl_update_status(struct backlight_device *bl)
>  {
>  	struct aw99706_device *aw = bl_get_data(bl);
>  
> -	return aw99706_update_brightness(aw, bl->props.brightness);
> +	return aw99706_update_brightness(aw, backlight_get_brightness(bl));
>  }

While reviewing this patch, I noticed a few pre-existing issues in the driver:

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 (IS_ERR(aw->hwen_gpio))
		return dev_err_probe(dev, PTR_ERR(aw->hwen_gpio),
				     "Failed to get enable gpio\n");
    ...
}

[Severity: High]
This is a pre-existing issue, but doesn't aw99706_chip_id_read() perform an
I2C read before the hardware enable GPIO is requested and asserted?

If the hardware is disabled by default or left in reset by the bootloader,
won't this I2C read fail and make the driver completely non-functional?


drivers/video/backlight/aw99706.c:aw99706_remove() {
	struct aw99706_device *aw = i2c_get_clientdata(client);

	aw99706_update_brightness(aw, 0);

	msleep(50);

	gpiod_set_value_cansleep(aw->hwen_gpio, 0);
}

[Severity: High]
This is a pre-existing issue, but does disabling the hardware here create a
use-after-disable race?

The backlight device is registered with devm_backlight_device_register(), so
devres will unregister it after aw99706_remove() returns.

If a userspace process accesses the sysfs backlight attributes in this window,
could aw99706_bl_update_status() issue I2C writes to a powered-down chip?


drivers/video/backlight/aw99706.c:aw99706_hw_init() {
	int ret, i;

	gpiod_set_value_cansleep(aw->hwen_gpio, 1);

	for (i = 0; i < ARRAY_SIZE(aw->init_tbl); i++) {
		ret = aw99706_i2c_update_bits(aw, aw->init_tbl[i].reg,
    ...
}

[Severity: Medium]
This is a pre-existing issue, but does this sequence need a stabilization
delay between asserting hwen_gpio and issuing I2C commands?

Could the initialization commands be dropped or NACKed if the controller
requires a few milliseconds to wake up before accepting I2C traffic?


drivers/video/backlight/aw99706.c:aw99706_suspend() {
	struct aw99706_device *aw = dev_get_drvdata(dev);

	return aw99706_update_brightness(aw, 0);
}

static int aw99706_resume(struct device *dev)
{
	struct aw99706_device *aw = dev_get_drvdata(dev);

	return aw99706_hw_init(aw);
}

[Severity: Medium]
This is a pre-existing issue, but do these suspend and resume operations
handle the hardware enable GPIO correctly?

aw99706_suspend() zeros out the brightness but leaves hwen_gpio asserted,
and aw99706_resume() blindly re-initializes all hardware registers without
the power ever being removed.

Should aw99706_suspend() pull hwen_gpio low to actually save power, or are
these operations redundant given BL_CORE_SUSPENDRESUME already handles setting
brightness to 0?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260803141310.1379194-1-junjie.cao@linux.dev?part=3