Devicetree
 help / color / mirror / Atom feed
* [PATCH v3 0/2] backlight: Add SY7758 6-channel High Efficiency LED Driver support
@ 2026-05-19  8:43 Neil Armstrong
  2026-05-19  8:43 ` [PATCH v3 1/2] dt-bindings: leds: backlight: document the SY7758 6-channel High Efficiency LED Driver Neil Armstrong
  2026-05-19  8:43 ` [PATCH v3 2/2] backlight: Add SY7758 6-channel High Efficiency LED Driver support Neil Armstrong
  0 siblings, 2 replies; 11+ messages in thread
From: Neil Armstrong @ 2026-05-19  8:43 UTC (permalink / raw)
  To: Lee Jones, Daniel Thompson, Jingoo Han, Pavel Machek, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, Helge Deller
  Cc: dri-devel, linux-leds, devicetree, linux-kernel, linux-fbdev,
	KancyJoe, Neil Armstrong, Krzysztof Kozlowski

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>
---
Changes in v3:
- Dropped unused macros
- Added second autho entry to match header and commit message
- Move my signof at the end
- Switched to flseep()
- Link to v2: https://patch.msgid.link/20260430-topic-sm8650-ayaneo-pocket-s2-sy7758-v2-0-308140640de9@linaro.org

Changes in v2:
- Fixed bindings subject and removed "|"
- Added review tag
- Added higher delay before reading ID from HW (100us was too short)
- Removed probe defer if i2c read fails
- Link to v1: https://patch.msgid.link/20260428-topic-sm8650-ayaneo-pocket-s2-sy7758-v1-0-0caade5fdb32@linaro.org

---
KancyJoe (1):
      backlight: Add SY7758 6-channel High Efficiency LED Driver support

Neil Armstrong (1):
      dt-bindings: leds: backlight: document the SY7758 6-channel High Efficiency LED Driver

 .../bindings/leds/backlight/silergy,sy7758.yaml    |  53 +++++
 drivers/video/backlight/Kconfig                    |   8 +
 drivers/video/backlight/Makefile                   |   1 +
 drivers/video/backlight/sy7758.c                   | 259 +++++++++++++++++++++
 4 files changed, 321 insertions(+)
---
base-commit: 39704f00f747aba3144289870b5fd8ac230a9aaf
change-id: 20260428-topic-sm8650-ayaneo-pocket-s2-sy7758-3081ee7f1e25

Best regards,
--  
Neil Armstrong <neil.armstrong@linaro.org>


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v3 1/2] dt-bindings: leds: backlight: document the SY7758 6-channel High Efficiency LED Driver
  2026-05-19  8:43 [PATCH v3 0/2] backlight: Add SY7758 6-channel High Efficiency LED Driver support Neil Armstrong
@ 2026-05-19  8:43 ` Neil Armstrong
  2026-05-29 10:07   ` Daniel Thompson
  2026-05-19  8:43 ` [PATCH v3 2/2] backlight: Add SY7758 6-channel High Efficiency LED Driver support Neil Armstrong
  1 sibling, 1 reply; 11+ messages in thread
From: Neil Armstrong @ 2026-05-19  8:43 UTC (permalink / raw)
  To: Lee Jones, Daniel Thompson, Jingoo Han, Pavel Machek, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, Helge Deller
  Cc: dri-devel, linux-leds, devicetree, linux-kernel, linux-fbdev,
	KancyJoe, Neil Armstrong, Krzysztof Kozlowski

Document the Silergy SY7758 6-channel High Efficiency LED Driver
used for backlight brightness control.

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>
Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
---
 .../bindings/leds/backlight/silergy,sy7758.yaml    | 53 ++++++++++++++++++++++
 1 file changed, 53 insertions(+)

diff --git a/Documentation/devicetree/bindings/leds/backlight/silergy,sy7758.yaml b/Documentation/devicetree/bindings/leds/backlight/silergy,sy7758.yaml
new file mode 100644
index 000000000000..80e978d691c2
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/backlight/silergy,sy7758.yaml
@@ -0,0 +1,53 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/leds/backlight/silergy,sy7758.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Silergy SY7758 6-channel High Efficiency LED Driver
+
+maintainers:
+  - Neil Armstrong <neil.armstrong@linaro.org>
+
+description:
+  Silergy SY7758 is a high efficiency 6-channels LED backlight
+  driver with I2C brightness control.
+
+allOf:
+  - $ref: common.yaml#
+
+properties:
+  compatible:
+    const: silergy,sy7758
+
+  reg:
+    maxItems: 1
+
+  vddio-supply: true
+
+  enable-gpios:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+  - vddio-supply
+  - enable-gpios
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        backlight@11 {
+            compatible = "silergy,sy7758";
+            reg = <0x11>;
+            vddio-supply = <&bl_vddio>;
+            enable-gpios = <&gpio 16 GPIO_ACTIVE_HIGH>;
+        };
+    };

-- 
2.34.1


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH v3 2/2] backlight: Add SY7758 6-channel High Efficiency LED Driver support
  2026-05-19  8:43 [PATCH v3 0/2] backlight: Add SY7758 6-channel High Efficiency LED Driver support Neil Armstrong
  2026-05-19  8:43 ` [PATCH v3 1/2] dt-bindings: leds: backlight: document the SY7758 6-channel High Efficiency LED Driver Neil Armstrong
@ 2026-05-19  8:43 ` Neil Armstrong
  2026-05-19  9:08   ` sashiko-bot
  1 sibling, 1 reply; 11+ messages in thread
From: Neil Armstrong @ 2026-05-19  8:43 UTC (permalink / raw)
  To: Lee Jones, Daniel Thompson, Jingoo Han, Pavel Machek, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, Helge Deller
  Cc: dri-devel, linux-leds, devicetree, linux-kernel, linux-fbdev,
	KancyJoe, Neil Armstrong

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: KancyJoe <kancy2333@outlook.com>
Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
---
 drivers/video/backlight/Kconfig  |   8 ++
 drivers/video/backlight/Makefile |   1 +
 drivers/video/backlight/sy7758.c | 259 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 268 insertions(+)

diff --git a/drivers/video/backlight/Kconfig b/drivers/video/backlight/Kconfig
index a7a3fbaf7c29..052ac80c8213 100644
--- a/drivers/video/backlight/Kconfig
+++ b/drivers/video/backlight/Kconfig
@@ -207,6 +207,14 @@ config BACKLIGHT_KTZ8866
 		Say Y to enable the backlight driver for the Kinetic KTZ8866
 		found in Xiaomi Mi Pad 5 series.
 
+config BACKLIGHT_SY7758
+	tristate "Backlight Driver for Silergy SY7758"
+	depends on I2C
+	select REGMAP_I2C
+	help
+	  Say Y to enable the backlight driver for the Silergy SY7758
+	  backlight controller found in Ayaneo Socket S2.
+
 config BACKLIGHT_LM3533
 	tristate "Backlight Driver for LM3533"
 	depends on MFD_LM3533
diff --git a/drivers/video/backlight/Makefile b/drivers/video/backlight/Makefile
index 794820a98ed4..39ef588b1cf2 100644
--- a/drivers/video/backlight/Makefile
+++ b/drivers/video/backlight/Makefile
@@ -56,6 +56,7 @@ obj-$(CONFIG_BACKLIGHT_PWM)		+= pwm_bl.o
 obj-$(CONFIG_BACKLIGHT_QCOM_WLED)	+= qcom-wled.o
 obj-$(CONFIG_BACKLIGHT_RT4831)		+= rt4831-backlight.o
 obj-$(CONFIG_BACKLIGHT_SAHARA)		+= kb3886_bl.o
+obj-$(CONFIG_BACKLIGHT_SY7758)		+= sy7758.o
 obj-$(CONFIG_BACKLIGHT_SKY81452)	+= sky81452-backlight.o
 obj-$(CONFIG_BACKLIGHT_TPS65217)	+= tps65217_bl.o
 obj-$(CONFIG_BACKLIGHT_WM831X)		+= wm831x_bl.o
diff --git a/drivers/video/backlight/sy7758.c b/drivers/video/backlight/sy7758.c
new file mode 100644
index 000000000000..a6087e687b64
--- /dev/null
+++ b/drivers/video/backlight/sy7758.c
@@ -0,0 +1,259 @@
+// 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>
+ */
+#include <linux/backlight.h>
+#include <linux/module.h>
+#include <linux/i2c.h>
+#include <linux/of.h>
+#include <linux/err.h>
+#include <linux/bits.h>
+#include <linux/regmap.h>
+#include <linux/bitfield.h>
+#include <linux/gpio/consumer.h>
+#include <linux/regulator/consumer.h>
+
+#define DEFAULT_BRIGHTNESS	1024
+#define MAX_BRIGHTNESS		4080
+#define REG_MAX			0xAE
+
+/* Registers */
+#define REG_DEV_CTL		0x01
+#define REG_DEV_ID		0x03
+#define REG_BRT_12BIT_L		0x10
+#define REG_BRT_12BIT_H		0x11
+
+/* OTP memory */
+#define REG_OTP_CFG0		0xA0
+#define REG_OTP_CFG1		0xA1
+#define REG_OTP_CFG2		0xA2
+#define REG_OTP_CFG5		0xA5
+#define REG_OTP_CFG9		0xA9
+
+/* Fields */
+#define BIT_DEV_CTL_FAST	BIT(7)
+#define MSK_DEV_CTL_BRT_MODE	GENMASK(2, 1)
+#define BIT_DEV_CTL_BL_CTLB	BIT(0)
+
+#define MSK_BRT_12BIT_L		GENMASK(7, 0)
+#define MSK_BRT_12BIT_H		GENMASK(3, 0)
+#define MSK_LED_ENABLE		GENMASK(5, 0)
+
+#define MSK_CFG0_CURRENT_LOW	GENMASK(7, 0)
+
+#define BIT_CFG1_PDET_STDBY	BIT(7)
+#define MSK_CFG1_CURRENT_MAX	GENMASK(6, 4)
+#define MSK_CFG1_CURRENT_HIGH	GENMASK(3, 0)
+
+#define BIT_CFG2_UVLO_EN	BIT(5)
+#define BIT_CFG2_UVLO_TH	BIT(4)
+#define BIT_CFG2_BL_ON		BIT(3)
+#define BIT_CFG2_ISET_EN	BIT(2)
+#define BIT_CFG2_BST_ESET_EN	BIT(1)
+
+#define BIT_CFG5_PWM_DIRECT	BIT(7)
+#define MSK_CFG5_PS_MODE	GENMASK(6, 4)
+#define MSK_CFG5_PWM_FREQ	GENMASK(3, 0)
+
+#define MSK_CFG9_VBST_MAX	GENMASK(7, 5)
+#define BIT_CFG9_JUMP_EN	BIT(4)
+#define MSK_CFG9_JUMP_TH	GENMASK(3, 2)
+#define MSK_CFG9_JUMP_VOLTAGE	GENMASK(1, 0)
+
+struct sy7758 {
+	struct i2c_client *client;
+	struct regmap *regmap;
+	struct gpio_desc *gpio;
+	struct backlight_device *bl;
+};
+
+static const struct regmap_config sy7758_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+	.max_register = REG_MAX,
+};
+
+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));
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static const struct backlight_ops sy7758_backlight_ops = {
+	.options = BL_CORE_SUSPENDRESUME,
+	.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));
+	if (ret)
+		return ret;
+
+	ret = regmap_write(sydev->regmap, REG_BRT_12BIT_L,
+			   FIELD_PREP(MSK_BRT_12BIT_L,
+				      DEFAULT_BRIGHTNESS & 0xff));
+	if (ret)
+		return ret;
+
+	ret = regmap_write(sydev->regmap, REG_BRT_12BIT_H,
+			   FIELD_PREP(MSK_BRT_12BIT_H,
+				      (DEFAULT_BRIGHTNESS >> 8)));
+	if (ret)
+		return ret;
+
+	ret = regmap_write(sydev->regmap, REG_OTP_CFG5,
+			   FIELD_PREP(MSK_CFG5_PS_MODE, 6) |
+			   FIELD_PREP(MSK_CFG5_PWM_FREQ, 4));
+	if (ret)
+		return ret;
+
+	ret = regmap_write(sydev->regmap, REG_OTP_CFG0,
+			   FIELD_PREP(MSK_CFG0_CURRENT_LOW, 85));
+	if (ret)
+		return ret;
+
+	ret = regmap_write(sydev->regmap, REG_OTP_CFG1,
+			   BIT_CFG1_PDET_STDBY |
+			   FIELD_PREP(MSK_CFG1_CURRENT_MAX, 1) |
+			   FIELD_PREP(MSK_CFG1_CURRENT_HIGH, 10));
+	if (ret)
+		return ret;
+
+	ret = regmap_write(sydev->regmap, REG_OTP_CFG9,
+			   FIELD_PREP(MSK_CFG9_VBST_MAX, 4));
+	if (ret)
+		return ret;
+
+	ret = regmap_write(sydev->regmap, REG_OTP_CFG2,
+			   BIT_CFG2_BL_ON | BIT_CFG2_UVLO_EN);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+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");
+
+	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");
+
+	/* Let some time for HW to settle */
+	fsleep(10000);
+
+	/* 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);
+}
+
+static void sy7758_remove(struct i2c_client *client)
+{
+	struct sy7758 *sydev = i2c_get_clientdata(client);
+
+	backlight_disable(sydev->bl);
+}
+
+static const struct i2c_device_id sy7758_ids[] = {
+	{ "sy7758" },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, sy7758_ids);
+
+static const struct of_device_id sy7758_match_table[] = {
+	{ .compatible = "silergy,sy7758", },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, sy7758_match_table);
+
+static struct i2c_driver sy7758_driver = {
+	.driver = {
+		.name = "sy7758",
+		.of_match_table = sy7758_match_table,
+	},
+	.probe = sy7758_probe,
+	.remove = sy7758_remove,
+	.id_table = sy7758_ids,
+};
+
+module_i2c_driver(sy7758_driver);
+
+MODULE_DESCRIPTION("Silergy SY7758 Backlight Driver");
+MODULE_AUTHOR("Kancy Joe <kancy2333@outlook.com>");
+MODULE_AUTHOR("Neil Armstrong <neil.armstrong@linaro.org>");
+MODULE_LICENSE("GPL");

-- 
2.34.1


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH v3 2/2] backlight: Add SY7758 6-channel High Efficiency LED Driver support
  2026-05-19  8:43 ` [PATCH v3 2/2] backlight: Add SY7758 6-channel High Efficiency LED Driver support Neil Armstrong
@ 2026-05-19  9:08   ` sashiko-bot
  2026-05-19  9:40     ` Neil Armstrong
  0 siblings, 1 reply; 11+ messages in thread
From: sashiko-bot @ 2026-05-19  9:08 UTC (permalink / raw)
  To: Neil Armstrong; +Cc: devicetree, conor+dt, robh

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] The 'enable' GPIO is not properly deasserted before the 'vddio' regulator is disabled, causing potential hardware backpowering.
- [High] Loss of critical register configuration on system resume if power is lost.
- [Medium] The generic driver hardcodes board-specific electrical parameters instead of parsing them from the Device Tree.
- [Medium] The driver treats the 'enable' GPIO as strictly mandatory, preventing it from probing on boards where the pin is hardwired.
- [Medium] Missing `<linux/delay.h>` header inclusion for `fsleep()`.
- [Low] Typo in Kconfig help text ('Socket' instead of 'Pocket').
- [Low] The `MSK_LED_ENABLE` macro is defined but never used.
--

commit ad75708c40d354dbaee72f90622daea36e29195d
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/Kconfig b/drivers/video/backlight/Kconfig
> index a7a3fbaf7c29e..052ac80c82137 100644
> --- a/drivers/video/backlight/Kconfig
> +++ b/drivers/video/backlight/Kconfig
[ ... ]
> +config BACKLIGHT_SY7758
> +	tristate "Backlight Driver for Silergy SY7758"
> +	depends on I2C
> +	select REGMAP_I2C
> +	help
> +	  Say Y to enable the backlight driver for the Silergy SY7758
> +	  backlight controller found in Ayaneo Socket S2.

This isn't a bug, but there is a typo (Socket instead of Pocket) in the help
text.

> diff --git a/drivers/video/backlight/sy7758.c b/drivers/video/backlight/sy7758.c
> new file mode 100644
> index 0000000000000..a6087e687b640
> --- /dev/null
> +++ b/drivers/video/backlight/sy7758.c
> @@ -0,0 +1,259 @@
> +// 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>
> + */
> +#include <linux/backlight.h>
> +#include <linux/module.h>
> +#include <linux/i2c.h>
> +#include <linux/of.h>
> +#include <linux/err.h>
> +#include <linux/bits.h>
> +#include <linux/regmap.h>
> +#include <linux/bitfield.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/regulator/consumer.h>

Should <linux/delay.h> be included here? The driver explicitly calls fsleep()
in sy7758_probe(), but relies on implicit header inclusion.

[ ... ]
> +#define MSK_BRT_12BIT_H		GENMASK(3, 0)
> +#define MSK_LED_ENABLE		GENMASK(5, 0)

Is MSK_LED_ENABLE intended to be used? It appears to be defined here but is
never used anywhere in the driver code.

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

Since BL_CORE_SUSPENDRESUME is set, the backlight core will call
update_status() on resume. If the system turns off the vddio regulator or I2C
power domain during suspend, won't the configuration registers programmed in
sy7758_init() (such as REG_DEV_CTL and the OTP configurations) be lost?

Does this driver need explicit suspend and resume callbacks to reapply the
hardware initialization sequence?

> +static int sy7758_init(struct sy7758 *sydev)
> +{
[ ... ]
> +	ret = regmap_write(sydev->regmap, REG_OTP_CFG0,
> +			   FIELD_PREP(MSK_CFG0_CURRENT_LOW, 85));
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_write(sydev->regmap, REG_OTP_CFG1,
> +			   BIT_CFG1_PDET_STDBY |
> +			   FIELD_PREP(MSK_CFG1_CURRENT_MAX, 1) |
> +			   FIELD_PREP(MSK_CFG1_CURRENT_HIGH, 10));

These values (85, 1, 10) appear to be board-specific electrical limits for the
Ayaneo panel. Could hardcoding them cause issues if this generic driver is
reused for other boards?

Should these parameters be parsed from Device Tree properties instead?

[ ... ]
> +static int sy7758_probe(struct i2c_client *client)
> +{
[ ... ]
> +	/* 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);

On driver unbind or probe failure, devres will release resources in LIFO
order. This means the GPIO descriptor is freed via gpiod_put() before the
vddio regulator is disabled.

Because gpiod_put() does not change the physical pin state, won't the enable
pin remain driven high while the main chip power is turned off? Could this
cause hardware backpowering through the chip's protection diodes?

Does the driver need a custom devm_add_action_or_reset() callback to
explicitly drive the GPIO low before the regulator is disabled?

Additionally, if a board implements the SY7758 with the enable pin permanently
pulled high in hardware, this devm_gpiod_get() call will return -ENOENT and
unconditionally fail probe.

Could this use devm_gpiod_get_optional() to allow the driver to probe
successfully on such boards?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260519-topic-sm8650-ayaneo-pocket-s2-sy7758-v3-0-ec8194bbc885@linaro.org?part=2

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v3 2/2] backlight: Add SY7758 6-channel High Efficiency LED Driver support
  2026-05-19  9:08   ` sashiko-bot
@ 2026-05-19  9:40     ` Neil Armstrong
  0 siblings, 0 replies; 11+ messages in thread
From: Neil Armstrong @ 2026-05-19  9:40 UTC (permalink / raw)
  To: sashiko-reviews; +Cc: devicetree, conor+dt, robh

On 5/19/26 11:08, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
> - [High] The 'enable' GPIO is not properly deasserted before the 'vddio' regulator is disabled, causing potential hardware backpowering.
> - [High] Loss of critical register configuration on system resume if power is lost.
> - [Medium] The generic driver hardcodes board-specific electrical parameters instead of parsing them from the Device Tree.
> - [Medium] The driver treats the 'enable' GPIO as strictly mandatory, preventing it from probing on boards where the pin is hardwired.
> - [Medium] Missing `<linux/delay.h>` header inclusion for `fsleep()`.
> - [Low] Typo in Kconfig help text ('Socket' instead of 'Pocket').
> - [Low] The `MSK_LED_ENABLE` macro is defined but never used.
> --
> 
> commit ad75708c40d354dbaee72f90622daea36e29195d
> 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/Kconfig b/drivers/video/backlight/Kconfig
>> index a7a3fbaf7c29e..052ac80c82137 100644
>> --- a/drivers/video/backlight/Kconfig
>> +++ b/drivers/video/backlight/Kconfig
> [ ... ]
>> +config BACKLIGHT_SY7758
>> +	tristate "Backlight Driver for Silergy SY7758"
>> +	depends on I2C
>> +	select REGMAP_I2C
>> +	help
>> +	  Say Y to enable the backlight driver for the Silergy SY7758
>> +	  backlight controller found in Ayaneo Socket S2.
> 
> This isn't a bug, but there is a typo (Socket instead of Pocket) in the help
> text.
> 
>> diff --git a/drivers/video/backlight/sy7758.c b/drivers/video/backlight/sy7758.c
>> new file mode 100644
>> index 0000000000000..a6087e687b640
>> --- /dev/null
>> +++ b/drivers/video/backlight/sy7758.c
>> @@ -0,0 +1,259 @@
>> +// 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>
>> + */
>> +#include <linux/backlight.h>
>> +#include <linux/module.h>
>> +#include <linux/i2c.h>
>> +#include <linux/of.h>
>> +#include <linux/err.h>
>> +#include <linux/bits.h>
>> +#include <linux/regmap.h>
>> +#include <linux/bitfield.h>
>> +#include <linux/gpio/consumer.h>
>> +#include <linux/regulator/consumer.h>
> 
> Should <linux/delay.h> be included here? The driver explicitly calls fsleep()
> in sy7758_probe(), but relies on implicit header inclusion.

Fixed

> 
> [ ... ]
>> +#define MSK_BRT_12BIT_H		GENMASK(3, 0)
>> +#define MSK_LED_ENABLE		GENMASK(5, 0)
> 
> Is MSK_LED_ENABLE intended to be used? It appears to be defined here but is
> never used anywhere in the driver code.

Dropped

> 
> [ ... ]
>> +static const struct backlight_ops sy7758_backlight_ops = {
>> +	.options = BL_CORE_SUSPENDRESUME,
>> +	.update_status = sy7758_backlight_update_status,
>> +};
> 
> Since BL_CORE_SUSPENDRESUME is set, the backlight core will call
> update_status() on resume. If the system turns off the vddio regulator or I2C
> power domain during suspend, won't the configuration registers programmed in
> sy7758_init() (such as REG_DEV_CTL and the OTP configurations) be lost?
> 
> Does this driver need explicit suspend and resume callbacks to reapply the
> hardware initialization sequence?

This won't happen, the vddio & gpio are kept on

> 
>> +static int sy7758_init(struct sy7758 *sydev)
>> +{
> [ ... ]
>> +	ret = regmap_write(sydev->regmap, REG_OTP_CFG0,
>> +			   FIELD_PREP(MSK_CFG0_CURRENT_LOW, 85));
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = regmap_write(sydev->regmap, REG_OTP_CFG1,
>> +			   BIT_CFG1_PDET_STDBY |
>> +			   FIELD_PREP(MSK_CFG1_CURRENT_MAX, 1) |
>> +			   FIELD_PREP(MSK_CFG1_CURRENT_HIGH, 10));
> 
> These values (85, 1, 10) appear to be board-specific electrical limits for the
> Ayaneo panel. Could hardcoding them cause issues if this generic driver is
> reused for other boards?
> 
> Should these parameters be parsed from Device Tree properties instead?

Will be added later if needed

> 
> [ ... ]
>> +static int sy7758_probe(struct i2c_client *client)
>> +{
> [ ... ]
>> +	/* 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);
> 
> On driver unbind or probe failure, devres will release resources in LIFO
> order. This means the GPIO descriptor is freed via gpiod_put() before the
> vddio regulator is disabled.
> 
> Because gpiod_put() does not change the physical pin state, won't the enable
> pin remain driven high while the main chip power is turned off? Could this
> cause hardware backpowering through the chip's protection diodes?
> 
> Does the driver need a custom devm_add_action_or_reset() callback to
> explicitly drive the GPIO low before the regulator is disabled?
> 
> Additionally, if a board implements the SY7758 with the enable pin permanently
> pulled high in hardware, this devm_gpiod_get() call will return -ENOENT and
> unconditionally fail probe.
> 
> Could this use devm_gpiod_get_optional() to allow the driver to probe
> successfully on such boards?
> 

Not an issue, as specified in the spec the gpio level is powered by the vddio.



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v3 1/2] dt-bindings: leds: backlight: document the SY7758 6-channel High Efficiency LED Driver
  2026-05-19  8:43 ` [PATCH v3 1/2] dt-bindings: leds: backlight: document the SY7758 6-channel High Efficiency LED Driver Neil Armstrong
@ 2026-05-29 10:07   ` Daniel Thompson
  2026-05-29 10:16     ` Neil Armstrong
  0 siblings, 1 reply; 11+ messages in thread
From: Daniel Thompson @ 2026-05-29 10:07 UTC (permalink / raw)
  To: Neil Armstrong
  Cc: Lee Jones, Jingoo Han, Pavel Machek, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, Helge Deller, dri-devel,
	linux-leds, devicetree, linux-kernel, linux-fbdev, KancyJoe,
	Krzysztof Kozlowski

On Tue, May 19, 2026 at 10:43:38AM +0200, Neil Armstrong wrote:
> Document the Silergy SY7758 6-channel High Efficiency LED Driver
> used for backlight brightness control.
>
> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>
> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
> ---
>  .../bindings/leds/backlight/silergy,sy7758.yaml    | 53 ++++++++++++++++++++++
>  1 file changed, 53 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/leds/backlight/silergy,sy7758.yaml b/Documentation/devicetree/bindings/leds/backlight/silergy,sy7758.yaml
> new file mode 100644
> index 000000000000..80e978d691c2
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/leds/backlight/silergy,sy7758.yaml
> @@ -0,0 +1,53 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/leds/backlight/silergy,sy7758.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Silergy SY7758 6-channel High Efficiency LED Driver
> +
> +maintainers:
> +  - Neil Armstrong <neil.armstrong@linaro.org>
> +
> +description:
> +  Silergy SY7758 is a high efficiency 6-channels LED backlight
> +  driver with I2C brightness control.
> +
> +allOf:
> +  - $ref: common.yaml#
> +
> +properties:
> +  compatible:
> +    const: silergy,sy7758
> +
> +  reg:
> +    maxItems: 1
> +
> +  vddio-supply: true
> +
> +  enable-gpios:
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - reg
> +  - vddio-supply

Sorry for missing this in v2 but is vddio-supply really a required
property?

It's unusual for supplies to be mandatory (and the it is not mandatory
in the driver implementation).


Daniel.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v3 1/2] dt-bindings: leds: backlight: document the SY7758 6-channel High Efficiency LED Driver
  2026-05-29 10:07   ` Daniel Thompson
@ 2026-05-29 10:16     ` Neil Armstrong
  2026-05-29 10:35       ` Daniel Thompson
  0 siblings, 1 reply; 11+ messages in thread
From: Neil Armstrong @ 2026-05-29 10:16 UTC (permalink / raw)
  To: Daniel Thompson
  Cc: Lee Jones, Jingoo Han, Pavel Machek, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, Helge Deller, dri-devel,
	linux-leds, devicetree, linux-kernel, linux-fbdev, KancyJoe,
	Krzysztof Kozlowski

On 5/29/26 12:07, Daniel Thompson wrote:
> On Tue, May 19, 2026 at 10:43:38AM +0200, Neil Armstrong wrote:
>> Document the Silergy SY7758 6-channel High Efficiency LED Driver
>> used for backlight brightness control.
>>
>> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>
>> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
>> ---
>>   .../bindings/leds/backlight/silergy,sy7758.yaml    | 53 ++++++++++++++++++++++
>>   1 file changed, 53 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/leds/backlight/silergy,sy7758.yaml b/Documentation/devicetree/bindings/leds/backlight/silergy,sy7758.yaml
>> new file mode 100644
>> index 000000000000..80e978d691c2
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/leds/backlight/silergy,sy7758.yaml
>> @@ -0,0 +1,53 @@
>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/leds/backlight/silergy,sy7758.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Silergy SY7758 6-channel High Efficiency LED Driver
>> +
>> +maintainers:
>> +  - Neil Armstrong <neil.armstrong@linaro.org>
>> +
>> +description:
>> +  Silergy SY7758 is a high efficiency 6-channels LED backlight
>> +  driver with I2C brightness control.
>> +
>> +allOf:
>> +  - $ref: common.yaml#
>> +
>> +properties:
>> +  compatible:
>> +    const: silergy,sy7758
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  vddio-supply: true
>> +
>> +  enable-gpios:
>> +    maxItems: 1
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - vddio-supply
> 
> Sorry for missing this in v2 but is vddio-supply really a required
> property?
> 
> It's unusual for supplies to be mandatory (and the it is not mandatory
> in the driver implementation).

This device is a little bit special, the VDDIO regulator is used to provide
power for the I/O via the enable input, so basically the enable gpio power
level is provided by VDDIO.

This is the recommended way from the datasheet, and I assume it will be used
like that on other platforms (if it exists...)

This is why it's mandatory and enabled first before setting the enable pin.

This should probably be a comment in the code.

Neil

> 
> 
> Daniel.


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v3 1/2] dt-bindings: leds: backlight: document the SY7758 6-channel High Efficiency LED Driver
  2026-05-29 10:16     ` Neil Armstrong
@ 2026-05-29 10:35       ` Daniel Thompson
  2026-05-29 12:50         ` Neil Armstrong
  0 siblings, 1 reply; 11+ messages in thread
From: Daniel Thompson @ 2026-05-29 10:35 UTC (permalink / raw)
  To: Neil Armstrong
  Cc: Lee Jones, Jingoo Han, Pavel Machek, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, Helge Deller, dri-devel,
	linux-leds, devicetree, linux-kernel, linux-fbdev, KancyJoe,
	Krzysztof Kozlowski

On Fri, May 29, 2026 at 12:16:07PM +0200, Neil Armstrong wrote:
> On 5/29/26 12:07, Daniel Thompson wrote:
> > On Tue, May 19, 2026 at 10:43:38AM +0200, Neil Armstrong wrote:
> > > Document the Silergy SY7758 6-channel High Efficiency LED Driver
> > > used for backlight brightness control.
> > >
> > > Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>
> > > Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
> > > ---
> > >   .../bindings/leds/backlight/silergy,sy7758.yaml    | 53 ++++++++++++++++++++++
> > >   1 file changed, 53 insertions(+)
> > >
> > > diff --git a/Documentation/devicetree/bindings/leds/backlight/silergy,sy7758.yaml b/Documentation/devicetree/bindings/leds/backlight/silergy,sy7758.yaml
> > > new file mode 100644
> > > index 000000000000..80e978d691c2
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/leds/backlight/silergy,sy7758.yaml
> > > @@ -0,0 +1,53 @@
> > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/leds/backlight/silergy,sy7758.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: Silergy SY7758 6-channel High Efficiency LED Driver
> > > +
> > > +maintainers:
> > > +  - Neil Armstrong <neil.armstrong@linaro.org>
> > > +
> > > +description:
> > > +  Silergy SY7758 is a high efficiency 6-channels LED backlight
> > > +  driver with I2C brightness control.
> > > +
> > > +allOf:
> > > +  - $ref: common.yaml#
> > > +
> > > +properties:
> > > +  compatible:
> > > +    const: silergy,sy7758
> > > +
> > > +  reg:
> > > +    maxItems: 1
> > > +
> > > +  vddio-supply: true
> > > +
> > > +  enable-gpios:
> > > +    maxItems: 1
> > > +
> > > +required:
> > > +  - compatible
> > > +  - reg
> > > +  - vddio-supply
> >
> > Sorry for missing this in v2 but is vddio-supply really a required
> > property?
> >
> > It's unusual for supplies to be mandatory (and the it is not mandatory
> > in the driver implementation).
>
> This device is a little bit special, the VDDIO regulator is used to provide
> power for the I/O via the enable input, so basically the enable gpio power
> level is provided by VDDIO.

I don't follow. The EN pin acts as both VDDIO and as an enable but it's
still effectively a power rail isn't it (albeit one with very low current
draw).

It looked to me like the correct way to model to two power rails
going into the chip is vdd-supply (main power supply) and vddio-supply
(EN/VDDIO) I don't understand why a single pin needs both a regulator
*and* a GPIO in the DT bindings?


> This is the recommended way from the datasheet, and I assume it will be used
> like that on other platforms (if it exists...)
>
> This is why it's mandatory and enabled first before setting the enable pin.

It's not mandatory for the C implementation. devm_regulator_get_enable()
will provide a dummy regulator if the property is omitted.


Daniel.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v3 1/2] dt-bindings: leds: backlight: document the SY7758 6-channel High Efficiency LED Driver
  2026-05-29 10:35       ` Daniel Thompson
@ 2026-05-29 12:50         ` Neil Armstrong
  2026-05-29 14:15           ` Daniel Thompson
  0 siblings, 1 reply; 11+ messages in thread
From: Neil Armstrong @ 2026-05-29 12:50 UTC (permalink / raw)
  To: Daniel Thompson
  Cc: Lee Jones, Jingoo Han, Pavel Machek, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, Helge Deller, dri-devel,
	linux-leds, devicetree, linux-kernel, linux-fbdev, KancyJoe,
	Krzysztof Kozlowski

On 5/29/26 12:35, Daniel Thompson wrote:
> On Fri, May 29, 2026 at 12:16:07PM +0200, Neil Armstrong wrote:
>> On 5/29/26 12:07, Daniel Thompson wrote:
>>> On Tue, May 19, 2026 at 10:43:38AM +0200, Neil Armstrong wrote:
>>>> Document the Silergy SY7758 6-channel High Efficiency LED Driver
>>>> used for backlight brightness control.
>>>>
>>>> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>
>>>> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
>>>> ---
>>>>    .../bindings/leds/backlight/silergy,sy7758.yaml    | 53 ++++++++++++++++++++++
>>>>    1 file changed, 53 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/leds/backlight/silergy,sy7758.yaml b/Documentation/devicetree/bindings/leds/backlight/silergy,sy7758.yaml
>>>> new file mode 100644
>>>> index 000000000000..80e978d691c2
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/leds/backlight/silergy,sy7758.yaml
>>>> @@ -0,0 +1,53 @@
>>>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>>>> +%YAML 1.2
>>>> +---
>>>> +$id: http://devicetree.org/schemas/leds/backlight/silergy,sy7758.yaml#
>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>> +
>>>> +title: Silergy SY7758 6-channel High Efficiency LED Driver
>>>> +
>>>> +maintainers:
>>>> +  - Neil Armstrong <neil.armstrong@linaro.org>
>>>> +
>>>> +description:
>>>> +  Silergy SY7758 is a high efficiency 6-channels LED backlight
>>>> +  driver with I2C brightness control.
>>>> +
>>>> +allOf:
>>>> +  - $ref: common.yaml#
>>>> +
>>>> +properties:
>>>> +  compatible:
>>>> +    const: silergy,sy7758
>>>> +
>>>> +  reg:
>>>> +    maxItems: 1
>>>> +
>>>> +  vddio-supply: true
>>>> +
>>>> +  enable-gpios:
>>>> +    maxItems: 1
>>>> +
>>>> +required:
>>>> +  - compatible
>>>> +  - reg
>>>> +  - vddio-supply
>>>
>>> Sorry for missing this in v2 but is vddio-supply really a required
>>> property?
>>>
>>> It's unusual for supplies to be mandatory (and the it is not mandatory
>>> in the driver implementation).
>>
>> This device is a little bit special, the VDDIO regulator is used to provide
>> power for the I/O via the enable input, so basically the enable gpio power
>> level is provided by VDDIO.
> 
> I don't follow. The EN pin acts as both VDDIO and as an enable but it's
> still effectively a power rail isn't it (albeit one with very low current
> draw).

Here's the datasheet description:
```
Dual-purpose pin serving both as a chip enable and as a power supply
reference for PWM, SDA, and SCL inputs.
```

The VDD input is directly provided by the panel, so Linux has no control
of it so I haven't added it.

> 
> It looked to me like the correct way to model to two power rails
> going into the chip is vdd-supply (main power supply) and vddio-supply
> (EN/VDDIO) I don't understand why a single pin needs both a regulator
> *and* a GPIO in the DT bindings?

I don't have a the schematics of the board, but as I understood one gpio is
actually enabling an regulator which provides power to the IC (vddio) and
a second gpio will either drive the EN signal to GND or VDDIO to provide a
clean rising edge on the EN pin.

So it's not really 2 regulators, and having regulators means the enable
signal can be shared and would have regulator characteristics which it hasn't.

> 
>> This is the recommended way from the datasheet, and I assume it will be used
>> like that on other platforms (if it exists...)
>>
>> This is why it's mandatory and enabled first before setting the enable pin.
> 
> It's not mandatory for the C implementation. devm_regulator_get_enable()
> will provide a dummy regulator if the property is omitted.

So yeah if you prefer I'll re-spin with the vddio regulator as optional
because between both, the VDDIO is the only which could be shared with
other devices or always-on.

Neil

> 
> 
> Daniel.


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v3 1/2] dt-bindings: leds: backlight: document the SY7758 6-channel High Efficiency LED Driver
  2026-05-29 12:50         ` Neil Armstrong
@ 2026-05-29 14:15           ` Daniel Thompson
  2026-05-29 14:36             ` Daniel Thompson
  0 siblings, 1 reply; 11+ messages in thread
From: Daniel Thompson @ 2026-05-29 14:15 UTC (permalink / raw)
  To: Neil Armstrong
  Cc: Daniel Thompson, Lee Jones, Jingoo Han, Pavel Machek, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, Helge Deller, dri-devel,
	linux-leds, devicetree, linux-kernel, linux-fbdev, KancyJoe,
	Krzysztof Kozlowski

On Fri, May 29, 2026 at 02:50:43PM +0200, Neil Armstrong wrote:
> On 5/29/26 12:35, Daniel Thompson wrote:
> > On Fri, May 29, 2026 at 12:16:07PM +0200, Neil Armstrong wrote:
> > > On 5/29/26 12:07, Daniel Thompson wrote:
> > > > On Tue, May 19, 2026 at 10:43:38AM +0200, Neil Armstrong wrote:
> > > > > Document the Silergy SY7758 6-channel High Efficiency LED Driver
> > > > > used for backlight brightness control.
> > > > >
> > > > > Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>
> > > > > Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
> > > > > ---
> > > > >    .../bindings/leds/backlight/silergy,sy7758.yaml    | 53 ++++++++++++++++++++++
> > > > >    1 file changed, 53 insertions(+)
> > > > >
> > > > > diff --git a/Documentation/devicetree/bindings/leds/backlight/silergy,sy7758.yaml b/Documentation/devicetree/bindings/leds/backlight/silergy,sy7758.yaml
> > > > > new file mode 100644
> > > > > index 000000000000..80e978d691c2
> > > > > --- /dev/null
> > > > > +++ b/Documentation/devicetree/bindings/leds/backlight/silergy,sy7758.yaml
> > > > > @@ -0,0 +1,53 @@
> > > > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > > > > +%YAML 1.2
> > > > > +---
> > > > > +$id: http://devicetree.org/schemas/leds/backlight/silergy,sy7758.yaml#
> > > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > > +
> > > > > +title: Silergy SY7758 6-channel High Efficiency LED Driver
> > > > > +
> > > > > +maintainers:
> > > > > +  - Neil Armstrong <neil.armstrong@linaro.org>
> > > > > +
> > > > > +description:
> > > > > +  Silergy SY7758 is a high efficiency 6-channels LED backlight
> > > > > +  driver with I2C brightness control.
> > > > > +
> > > > > +allOf:
> > > > > +  - $ref: common.yaml#
> > > > > +
> > > > > +properties:
> > > > > +  compatible:
> > > > > +    const: silergy,sy7758
> > > > > +
> > > > > +  reg:
> > > > > +    maxItems: 1
> > > > > +
> > > > > +  vddio-supply: true
> > > > > +
> > > > > +  enable-gpios:
> > > > > +    maxItems: 1
> > > > > +
> > > > > +required:
> > > > > +  - compatible
> > > > > +  - reg
> > > > > +  - vddio-supply
> > > >
> > > > Sorry for missing this in v2 but is vddio-supply really a required
> > > > property?
> > > >
> > > > It's unusual for supplies to be mandatory (and the it is not mandatory
> > > > in the driver implementation).
> > >
> > > This device is a little bit special, the VDDIO regulator is used to provide
> > > power for the I/O via the enable input, so basically the enable gpio power
> > > level is provided by VDDIO.
> >
> > I don't follow. The EN pin acts as both VDDIO and as an enable but it's
> > still effectively a power rail isn't it (albeit one with very low current
> > draw).
>
> Here's the datasheet description:
> ```
> Dual-purpose pin serving both as a chip enable and as a power supply
> reference for PWM, SDA, and SCL inputs.
> ```

Thanks for the quote. It's a big help (I've been working from a site
that makes me read a page at a time so I struggled to keep track of
things).

This says that the GPIO from the host is merely serving as a voltage
reference (for the an internal LDO regulator?). That means it is *not*
a power supply!

It sounds to me like the chip is designed to work with a host where
enable GPIO and I2C interface use the same I/O voltage. By having an
active-high enables the chip can *avoid* having a separate vddio pin.

However, in a design with no separate vddio pin then it would make no
sense to model a vddio-supply in the DT.


> The VDD input is directly provided by the panel, so Linux has no control
> of it so I haven't added it.

Power supplies are often added ad-hoc when the first board that includes
a regulator enable appears. On that basis omitting vdd-supply would be
relatively harmless if you don't need it but I wonder if you do, see below!


> > It looked to me like the correct way to model to two power rails
> > going into the chip is vdd-supply (main power supply) and vddio-supply
> > (EN/VDDIO) I don't understand why a single pin needs both a regulator
> > *and* a GPIO in the DT bindings?
>
> I don't have a the schematics of the board, but as I understood one gpio is
> actually enabling an regulator which provides power to the IC (vddio) and
> a second gpio will either drive the EN signal to GND or VDDIO to provide a
> clean rising edge on the EN pin.

This doesn't make sense since it is a single pin. It cannot be both a
power supply to the chip *and* a GPIO input. It's one or the other...
and from the above is sounds so me like GPIO).


> So it's not really 2 regulators, and having regulators means the enable
> signal can be shared and would have regulator characteristics which it hasn't.

Agreed. If the EN pin is merely use as an enable and voltage reference
then it are not two regulators.

However, it is also *not* vddio-supply and enable-gpios. We don't need
the board design to check this. The pinout diagram in the datasheet
should be sufficient!

If you have to activate vddio-supply for the backlight to work on the
board are you sure you don't just have a misnamed vdd-supply that needs
to be taken care of? That would make much more sense given the datasheet.


> > > This is the recommended way from the datasheet, and I assume it will be used
> > > like that on other platforms (if it exists...)
> > >
> > > This is why it's mandatory and enabled first before setting the enable pin.
> >
> > It's not mandatory for the C implementation. devm_regulator_get_enable()
> > will provide a dummy regulator if the property is omitted.
>
> So yeah if you prefer I'll re-spin with the vddio regulator as optional
> because between both, the VDDIO is the only which could be shared with
> other devices or always-on.

Based on the above, I'd be happy with an optional vdd-supply and an
enable-gpios.


Daniel.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v3 1/2] dt-bindings: leds: backlight: document the SY7758 6-channel High Efficiency LED Driver
  2026-05-29 14:15           ` Daniel Thompson
@ 2026-05-29 14:36             ` Daniel Thompson
  0 siblings, 0 replies; 11+ messages in thread
From: Daniel Thompson @ 2026-05-29 14:36 UTC (permalink / raw)
  To: Neil Armstrong
  Cc: Daniel Thompson, Lee Jones, Jingoo Han, Pavel Machek, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, Helge Deller, dri-devel,
	linux-leds, devicetree, linux-kernel, linux-fbdev, KancyJoe,
	Krzysztof Kozlowski

On Fri, May 29, 2026 at 03:15:03PM +0100, Daniel Thompson wrote:
> On Fri, May 29, 2026 at 02:50:43PM +0200, Neil Armstrong wrote:
> > On 5/29/26 12:35, Daniel Thompson wrote:
> > > On Fri, May 29, 2026 at 12:16:07PM +0200, Neil Armstrong wrote:
> > So it's not really 2 regulators, and having regulators means the enable
> > signal can be shared and would have regulator characteristics which it hasn't.
>
> Agreed. If the EN pin is merely use as an enable and voltage reference
> then it are not two regulators.
>
> However, it is also *not* vddio-supply and enable-gpios. We don't need
> the board design to check this. The pinout diagram in the datasheet
> should be sufficient!
>
> If you have to activate vddio-supply for the backlight to work on the
> board are you sure you don't just have a misnamed vdd-supply that needs
> to be taken care of? That would make much more sense given the datasheet.

After posting this I figured there is another possibility.

If the host GPIO pin is not capable of delivering the 1mA requires by
the chip then the board designer would have to add a buffer and that
buffer would need a power supply... and that power supply could, in
pinciple, be switchable.

However if that were the case then I don't think the power supply for
the buffer would belong in the bindings for the sy7758 so I'm afraid
whichever way I turn it I can't make vddio-supply make sense.


Daniel.

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2026-05-29 14:37 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-05-19  8:43 [PATCH v3 0/2] backlight: Add SY7758 6-channel High Efficiency LED Driver support Neil Armstrong
2026-05-19  8:43 ` [PATCH v3 1/2] dt-bindings: leds: backlight: document the SY7758 6-channel High Efficiency LED Driver Neil Armstrong
2026-05-29 10:07   ` Daniel Thompson
2026-05-29 10:16     ` Neil Armstrong
2026-05-29 10:35       ` Daniel Thompson
2026-05-29 12:50         ` Neil Armstrong
2026-05-29 14:15           ` Daniel Thompson
2026-05-29 14:36             ` Daniel Thompson
2026-05-19  8:43 ` [PATCH v3 2/2] backlight: Add SY7758 6-channel High Efficiency LED Driver support Neil Armstrong
2026-05-19  9:08   ` sashiko-bot
2026-05-19  9:40     ` Neil Armstrong

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox