Re: [PATCH v5 2/2] backlight: Add SY7758 6-channel High Efficiency LED Driver support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] The enable GPIO is asserted high during probe but never explicitly driven low during driver removal or probe failure, risking back-powering and hardware damage.
- [High] The driver relies on `BL_CORE_SUSPENDRESUME` but fails to restore volatile hardware initialization state upon resume.
- [Medium] The enable GPIO is requested as strictly mandatory, breaking support for hardware designs where the pin is hardwired.
- [Medium] The backlight sysfs interface remains active after `backlight_disable()` is called in `remove()`, creating a race condition where userspace can re-enable the hardware during teardown.
--

commit 55527bb448bde80cb22811193477de9c8839514e
Author: KancyJoe <kancy2333@outlook.com>

backlight: Add SY7758 6-channel High Efficiency LED Driver support

Implement support for the Silergy SY7758 6-channel High Efficiency LED
Driver used for backlight brightness control in the Ayaneo Pocket S2
dual-DSI panel.

> diff --git a/drivers/video/backlight/sy7758.c b/drivers/video/backlight/sy7758.c
> new file mode 100644
> index 0000000000000..786589cb8df91
> --- /dev/null
> +++ b/drivers/video/backlight/sy7758.c

[ ... ]

> +static int sy7758_backlight_update_status(struct backlight_device *backlight_dev)
> +{

[ ... ]

> +}
> +
> +static const struct backlight_ops sy7758_backlight_ops = {
> +	.options = BL_CORE_SUSPENDRESUME,
> +	.update_status = sy7758_backlight_update_status,
> +};

[Severity: High]
This driver sets the BL_CORE_SUSPENDRESUME option, which relies on the
backlight core to blank and unblank the brightness on suspend and resume.
However, if the system's power management cuts power to the vdd regulator
during suspend, won't the device lose all the configuration state established
in sy7758_init()?

Because there are no standard pm_ops to re-apply this initialization sequence
upon resume, it seems the backlight might operate with incorrect parameters or
fail to turn on. Should this driver implement a resume callback to restore the
volatile hardware configuration?

[ ... ]

> +static int sy7758_probe(struct i2c_client *client)
> +{
> +	struct backlight_properties props = { };
> +	struct device *dev = &client->dev;
> +	struct sy7758 *sydev;

[ ... ]

> +	/* Get and enable regulator */
> +	ret = devm_regulator_get_enable(dev, "vdd");
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to get regulator\n");
> +
> +	fsleep(100);
> +
> +	/* Get enable GPIO and set to high */
> +	sydev->gpio = devm_gpiod_get(dev, "enable", GPIOD_OUT_HIGH);
> +	if (IS_ERR(sydev->gpio))
> +		return dev_err_probe(dev, PTR_ERR(sydev->gpio),
> +				     "failed to get enable GPIO\n");

[Severity: Medium]
Is it possible for the enable pin to be hardwired to a power rail on some
hardware designs? Using devm_gpiod_get() makes this GPIO strictly mandatory
and fails the probe if the enable-gpios property is absent in the Device Tree.
Would devm_gpiod_get_optional() be more appropriate here?

[Severity: High]
The enable GPIO is asserted high here, but is there a mechanism to ensure it
is explicitly driven low during driver removal or if the probe fails later?

If devres automatically disables the vdd regulator while the host is still
driving the GPIO high, couldn't this back-power the chip through its internal
ESD protection diodes and cause hardware damage? Using
devm_add_action_or_reset() to drive the GPIO low before the regulator is
disabled might prevent this.

[ ... ]

> +static void sy7758_remove(struct i2c_client *client)
> +{
> +	struct sy7758 *sydev = i2c_get_clientdata(client);
> +
> +	backlight_disable(sydev->bl);
> +}

[Severity: Medium]
Since the backlight was registered using devm_backlight_device_register(),
devres will unregister the sysfs interface after sy7758_remove() returns.

Does this leave a race window where userspace could concurrently write to the
sysfs brightness file and re-enable the hardware right after
backlight_disable() turns it off?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260529-topic-sm8650-ayaneo-pocket-s2-sy7758-v5-0-03aacd49747c@linaro.org?part=2