Re: [PATCH 2/2] backlight: sy7758: add Silergy SY7758 backlight driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] Premature device registration exposes initialization race
- [Medium] Redundant global mutex causes cross-device contention
- [Medium] Hardcoded sysfs device name prevents multiple device instances
- [Medium] Unhandled I2C errors during initialization allow phantom devices
- [Medium] Visual glitch due to hardcoded brightness values upon unblanking
- [Medium] Improper brightness bitmasking limits resolution and breaks linearity
- [Medium] Lifecycle race between sysfs writes and device removal
--

commit 0d56a1ebe95bbf1d06c227b2612ebb2fdce6d26a
Author: Alexandre Hamamdjian <azkali.limited@gmail.com>

backlight: sy7758: add Silergy SY7758 backlight driver
    
Add a driver for the Silergy SY7758 I2C-controlled multi-channel LED
backlight controller. The chip drives the edge-lit LED strings of LCD
panels in handheld and embedded devices, and is for example present on
the Ayaneo Pocket DS handheld where it drives the panel backlight.

> diff --git a/drivers/video/backlight/sy7758.c b/drivers/video/backlight/sy7758.c
> new file mode 100644
> index 0000000000000..6a318bd62030a
> --- /dev/null
> +++ b/drivers/video/backlight/sy7758.c
[ ... ]
> +static DEFINE_MUTEX(sy7758_update_backlight_mutex);

Could this global mutex cause cross-device contention? Since the backlight
core already serializes calls to update_status() for each device using
bd->update_lock, is sy7758_update_backlight_mutex necessary?

[ ... ]
> +static int sy7758_backlight_update_status(struct backlight_device *backlight_dev)
> +{
> +	struct sy7758 *sydev = bl_get_data(backlight_dev);
> +	unsigned int brightness = backlight_get_brightness(backlight_dev);
> +
> +	mutex_lock(&sy7758_update_backlight_mutex);
> +
> +	if (!sydev->led_on && brightness > 0) {
> +		sy7758_init(sydev);
> +		sydev->led_on = true;
> +	} else if (brightness == 0) {
> +		sydev->led_on = false;
> +	}
> +
> +	sy7758_write(sydev, BL_BRT_L, brightness & 0xf0);

Does dropping the lower 4 bits here limit the resolution and break linearity?

The max_brightness is set to 4080, but this masking maps blocks of 16
consecutive brightness values to the exact same hardware output. If the
hardware uses an 8-bit resolution shifted by 4 bits, would it be better to
expose max_brightness as 255 and shift the value internally?

> +
> +	sy7758_write(sydev, BL_BRT_H, (brightness >> 8) & 0xf);
> +
> +	mutex_unlock(&sy7758_update_backlight_mutex);
> +	return 0;
> +}
[ ... ]
> +static void sy7758_init(struct sy7758 *sydev)
> +{
> +	sy7758_write(sydev, 0x01, 0x85);
> +	sy7758_write(sydev, 0x10, 0x00);
> +	sy7758_write(sydev, 0x11, 0x00);

Should we handle I2C errors during initialization?

Since sy7758_write() returns an error code but sy7758_init() ignores it, the
probe function will succeed even if the hardware is disconnected or
unresponsive, potentially creating a non-functional phantom device.

[ ... ]
> +	usleep_range(10000, 11000);
> +
> +	sy7758_write(sydev, 0x10, 0x40);
> +	sy7758_write(sydev, 0x11, 0x01);

Does this hardcoded value cause a brief visual glitch?

When the display is unblanked, update_status() calls sy7758_init(), which
unconditionally writes 0x40 and 0x01 (brightness 320) before update_status()
overwrites it with the target brightness.

[ ... ]
> +static int sy7758_probe(struct i2c_client *client)
> +{
[ ... ]
> +	backlight_dev = devm_backlight_device_register(&client->dev, "sy7758-backlight",

Is it possible for this hardcoded sysfs device name to prevent multiple device
instances from probing? If multiple controllers are present on the same
system, the second instance might fail to probe due to a sysfs naming
collision. Using dev_name(&client->dev) might avoid this.

> +					&client->dev, sydev, &sy7758_backlight_ops, &props);
> +	if (IS_ERR(backlight_dev))
> +		return dev_err_probe(&client->dev, PTR_ERR(backlight_dev),
> +				"failed to register backlight device\n");
> +
> +	sy7758_init(sydev);

Does registering the device before calling sy7758_init() expose an
initialization race?

If a sysfs write occurs right after registration, the backlight core could
invoke update_status(), which calls sy7758_init() concurrently with the probe
function continuing to call sy7758_init() below.

> +
> +	i2c_set_clientdata(client, backlight_dev);
> +	backlight_update_status(backlight_dev);
> +
> +	return 0;
> +}
> +
> +static void sy7758_remove(struct i2c_client *client)
> +{
> +	struct backlight_device *backlight_dev = i2c_get_clientdata(client);
> +
> +	backlight_dev->props.brightness = 0;
> +	backlight_update_status(backlight_dev);
> +}

Could there be a lifecycle race between sysfs writes and device removal here?

Because the driver uses devm_backlight_device_register(), the sysfs interface
remains active until the devres cleanup phase executes after remove() returns.
A concurrent userspace write to the brightness sysfs file could turn the
backlight back on just before unregistration, leaving the hardware stranded in
an ON state.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260511-sy7758-v1-0-999a33081304@gmail.com?part=2