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

[Daniel Thompson <danielt@kernel.org>]

On Thu, Apr 30, 2026 at 11:47:16AM +0200, Neil Armstrong wrote:
> From: KancyJoe <kancy2333@outlook.com>
>
> 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.
>
> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
> Signed-off-by: KancyJoe <kancy2333@outlook.com>
> ---
>  drivers/video/backlight/Kconfig  |   8 +
>  drivers/video/backlight/Makefile |   1 +
>  drivers/video/backlight/sy7758.c | 311 +++++++++++++++++++++++++++++++++++++++
>  3 files changed, 320 insertions(+)
> <snip>
> diff --git a/drivers/video/backlight/sy7758.c b/drivers/video/backlight/sy7758.c
> new file mode 100644
> index 000000000000..9b2d3bbb4ded
> --- /dev/null
> +++ b/drivers/video/backlight/sy7758.c
> @@ -0,0 +1,311 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Silergy SY7758 6-channel High Efficiency LED Driver
> + *
> + * Copyright (C) 2025 Kancy Joe <kancy2333@outlook.com>
> + * Copyright (C) 2026 Linaro Limited
> + * Author: Neil Armstrong <neil.armstrong@linaro.org>

I'm a bit confused by this comment. The git author and the
MODULE_AUTHOR() is Kancy Joe. What does this comment signify?


> + */
> <snip>
> +/* OTP memory */
> +#define REG_OTP_CFG98 0x98
> +#define REG_OTP_CFG9E 0x9E
> +#define REG_OTP_CFG0 0xA0
> +#define REG_OTP_CFG1 0xA1
> +#define REG_OTP_CFG2 0xA2
> +#define REG_OTP_CFG3 0xA3
> +#define REG_OTP_CFG4 0xA4
> +#define REG_OTP_CFG5 0xA5
> +#define REG_OTP_CFG6 0xA6
> +#define REG_OTP_CFG7 0xA7
> +#define REG_OTP_CFG9 0xA9
> +#define REG_OTP_CFGA 0xAA
> +#define REG_OTP_CFGE 0xAE

There seems to be a lot of unused macros here, especially
combined with the unused bitfields that tell us how to interpret
the values.

Do we need them?


> <snip>
> +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;
> +
> +	sydev = devm_kzalloc(dev, sizeof(*sydev), GFP_KERNEL);
> +	if (!sydev)
> +		return -ENOMEM;
> +
> +	i2c_set_clientdata(client, sydev);
> +
> +	/* Initialize regmap */
> +	sydev->client = client;
> +	sydev->regmap = devm_regmap_init_i2c(client, &sy7758_regmap_config);
> +	if (IS_ERR(sydev->regmap))
> +		return dev_err_probe(dev, PTR_ERR(sydev->regmap),
> +				     "failed to init regmap\n");
> +
> +	/* Get and enable regulators */
> +	ret = devm_regulator_get_enable(dev, "vddio");
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to get regulator\n");
> +
> +	usleep_range(100, 200);

Any reason not to use fsleep() here?


> +	/* 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");
> +
> +	/* Let some time for HW to settle */
> +	usleep_range(10000, 11000);

And here?


> +
> +	/* try read and check device id */
> +	ret = regmap_read(sydev->regmap, REG_DEV_ID, &dev_id);
> +	if (ret < 0)
> +		return dev_err_probe(dev, ret, "failed to read device id\n");
> +	if (dev_id != 0x63) {
> +		dev_err(dev, "unexpected device id: 0x%02x\n", dev_id);
> +		return -ENODEV;
> +	}
> +
> +	/* Initialize and set default brightness */
> +	ret = sy7758_init(sydev);
> +	if (ret)
> +		return ret;
> +
> +	props.type = BACKLIGHT_RAW;
> +	props.max_brightness = MAX_BRIGHTNESS;
> +	props.brightness = DEFAULT_BRIGHTNESS;
> +	props.scale = BACKLIGHT_SCALE_LINEAR;
> +
> +	sydev->bl = devm_backlight_device_register(dev, "sy7758-backlight",
> +						   dev, sydev, &sy7758_backlight_ops,
> +						   &props);
> +	if (IS_ERR(sydev->bl))
> +		return dev_err_probe(dev, PTR_ERR(sydev->bl),
> +				     "failed to register backlight device\n");
> +
> +	return backlight_update_status(sydev->bl);
> +}


Daniel.