From: Daniel Thompson <danielt@kernel.org> To: Neil Armstrong <neil.armstrong@linaro.org> Cc: Lee Jones <lee@kernel.org>, Jingoo Han <jingoohan1@gmail.com>, Pavel Machek <pavel@kernel.org>, Rob Herring <robh@kernel.org>, Krzysztof Kozlowski <krzk+dt@kernel.org>, Conor Dooley <conor+dt@kernel.org>, Helge Deller <deller@gmx.de>, dri-devel@lists.freedesktop.org, linux-leds@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, linux-fbdev@vger.kernel.org, KancyJoe <kancy2333@outlook.com> Subject: Re: [PATCH v2 2/2] backlight: Add SY7758 6-channel High Efficiency LED Driver support Date: Fri, 15 May 2026 17:28:14 +0100 [thread overview] Message-ID: <agdJnpz9O00lywRm@aspen.lan> (raw) In-Reply-To: <20260430-topic-sm8650-ayaneo-pocket-s2-sy7758-v2-2-308140640de9@linaro.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.