Re: [PATCH v4 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 driver incorrectly uses regmap_write() to update specific fields in configuration registers, wiping out unreferenced bitfields.
- [High] The 'enable' GPIO is driven HIGH during probe but never explicitly set LOW during removal or on probe failure, causing hardware resource leaks and potential electrical back-powering.
- [Medium] Critical hardware configuration is lost across system suspend/resume cycles.
- [Medium] Non-atomic updates to the 12-bit brightness registers may cause visible backlight flicker.
--

commit 98100c9f3380a75d76d0213c9564c18833a54b8e
Author: KancyJoe <kancy2333@outlook.com>

backlight: Add SY7758 6-channel High Efficiency LED Driver support
    
This commit implements 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..198d559394389
> --- /dev/null
> +++ b/drivers/video/backlight/sy7758.c

[ ... ]

> +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);
> +	int ret;
> +
> +	ret = regmap_write(sydev->regmap, REG_BRT_12BIT_L,
> +			   FIELD_PREP(MSK_BRT_12BIT_L,
> +				      brightness & 0xff));
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_write(sydev->regmap, REG_BRT_12BIT_H,
> +			   FIELD_PREP(MSK_BRT_12BIT_H,
> +				      (brightness >> 8) & 0xf));

[Severity: Medium]
Will writing the LSB and MSB in separate I2C transactions cause visible
flicker during smooth dimming?

If the brightness transitions across a byte boundary (e.g., from 0x0FF to
0x100), writing the LSB first will momentarily set the hardware to 0x000
before the MSB write corrects it to 0x100.

Unless the hardware specifically latches the value only when the MSB is
written, could this use regmap_bulk_write() to update both contiguous
registers atomically?

> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static const struct backlight_ops sy7758_backlight_ops = {
> +	.options = BL_CORE_SUSPENDRESUME,

[Severity: Medium]
Since BL_CORE_SUSPENDRESUME is set, does the driver also need PM suspend/resume
callbacks to restore the configuration registers?

If the system cuts power to the controller during suspend, the initialization
performed in sy7758_init() might be permanently lost upon resume, causing the
device to operate with uninitialized or default settings.

Could a regmap cache with regcache_sync() in a resume handler be used, or
perhaps re-invoking sy7758_init() on resume?

> +	.update_status = sy7758_backlight_update_status,
> +};
> +
> +static int sy7758_init(struct sy7758 *sydev)
> +{
> +	int ret = 0;
> +
> +	ret = regmap_write(sydev->regmap, REG_DEV_CTL,
> +			   BIT_DEV_CTL_FAST | BIT_DEV_CTL_BL_CTLB |
> +			   FIELD_PREP(MSK_DEV_CTL_BRT_MODE, 2));

[Severity: High]
Does using regmap_write() here wipe out unreferenced bitfields in these
configuration registers?

Because regmap_write() replaces the entire register contents, it implicitly
clears all unreferenced bitfields to 0. For example, in REG_OTP_CFG9 below,
writing FIELD_PREP(MSK_CFG9_VBST_MAX, 4) forces JUMP_EN, JUMP_TH, and
JUMP_VOLTAGE to 0, which might destroy their default or OTP-loaded values.

Could this use regmap_update_bits() when modifying specific fields to preserve
the state of other bits?

> +	if (ret)
> +		return ret;

[ ... ]

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

[ ... ]

> +	/* Get and enable regulators */
> +	ret = devm_regulator_get_enable(dev, "vddio");
> +	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);

[Severity: High]
Is the enable GPIO ever explicitly driven LOW during removal or on probe
failure?

During driver removal or probe failure, devres automatically calls
gpiod_put(), which simply releases the software descriptor without actively
driving the pin LOW. Immediately afterward, devres disables the vddio regulator.

This sequence leaves the host driving a logic HIGH into the enable pin of an
unpowered SY7758 controller, which violates electrical sequencing and might
cause back-powering or hardware damage.

Could the driver register a custom devm action (e.g., using
devm_add_action_or_reset()) to explicitly set the GPIO to 0 before the
regulator is disabled?

> +	if (IS_ERR(sydev->gpio))
> +		return dev_err_probe(dev, PTR_ERR(sydev->gpio),
> +				     "failed to get enable GPIO\n");

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260521-topic-sm8650-ayaneo-pocket-s2-sy7758-v4-0-73c732615e4a@linaro.org?part=2