public inbox for drm-ai-reviews@public-inbox.freedesktop.org
 help / color / mirror / Atom feed
* [PATCH v2 0/2] backlight: Add SY7758 6-channel High Efficiency LED Driver support
@ 2026-04-30  9:47 Neil Armstrong
  2026-04-30  9:47 ` [PATCH v2 1/2] dt-bindings: leds: backlight: document the SY7758 6-channel High Efficiency LED Driver Neil Armstrong
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Neil Armstrong @ 2026-04-30  9:47 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 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                   | 311 +++++++++++++++++++++
 4 files changed, 373 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] 6+ messages in thread

* [PATCH v2 1/2] dt-bindings: leds: backlight: document the SY7758 6-channel High Efficiency LED Driver
  2026-04-30  9:47 [PATCH v2 0/2] backlight: Add SY7758 6-channel High Efficiency LED Driver support Neil Armstrong
@ 2026-04-30  9:47 ` Neil Armstrong
  2026-05-05  0:44   ` Claude review: " Claude Code Review Bot
  2026-04-30  9:47 ` [PATCH v2 2/2] backlight: Add SY7758 6-channel High Efficiency LED Driver support Neil Armstrong
  2026-05-05  0:44 ` Claude Code Review Bot
  2 siblings, 1 reply; 6+ messages in thread
From: Neil Armstrong @ 2026-04-30  9:47 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] 6+ messages in thread

* [PATCH v2 2/2] backlight: Add SY7758 6-channel High Efficiency LED Driver support
  2026-04-30  9:47 [PATCH v2 0/2] backlight: Add SY7758 6-channel High Efficiency LED Driver support Neil Armstrong
  2026-04-30  9:47 ` [PATCH v2 1/2] dt-bindings: leds: backlight: document the SY7758 6-channel High Efficiency LED Driver Neil Armstrong
@ 2026-04-30  9:47 ` Neil Armstrong
  2026-05-05  0:44   ` Claude review: " Claude Code Review Bot
  2026-05-05  0:44 ` Claude Code Review Bot
  2 siblings, 1 reply; 6+ messages in thread
From: Neil Armstrong @ 2026-04-30  9:47 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: 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(+)

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..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>
+ */
+#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_BRT_8BIT 0x00
+#define REG_DEV_CTL 0x01
+#define REG_STATUS 0x02
+#define REG_DEV_ID 0x03
+#define REG_DIRECT_CTL 0x04
+#define REG_STATUS2 0x05
+#define REG_BRT_12BIT_L 0x10
+#define REG_BRT_12BIT_H 0x11
+#define REG_LED_ENABLE 0x16
+
+/* 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
+
+/* 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 BIT_STATUS_OPEN BIT(7)
+#define BIT_STATUS_SHORT BIT(6)
+#define BIT_STATUS_VREF_OK BIT(5)
+#define BIT_STATUS_VBST_OK BIT(4)
+#define BIT_STATUS_OVP BIT(3)
+#define BIT_STATUS_OCP BIT(2)
+#define BIT_STATUS_TSD BIT(1)
+#define BIT_STATUS_UVLO BIT(0)
+
+#define MSK_DIRECT_CTL_OUT GENMASK(5, 0)
+
+#define BIT_STATUS2_OCP50MS_LATCH BIT(0)
+#define BIT_STATUS2_OCP2 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 BIT_CFG98_IBST_LIM_2X BIT(7)
+#define BIT_CFG98_A0_FSETB BIT(0)
+
+#define BIT_CFG9E_VBST_RANGE BIT(5)
+#define MSK_CFG9E_HEADROOM_OFFSET GENMASK(3, 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 MSK_CFG3_SLOPE GENMASK(6, 4)
+#define MSK_CFG3_FILTER GENMASK(3, 2)
+#define MSK_CFG3_PWM_INPUT_HYSTERESIS GENMASK(1, 0)
+#define MSK_CFG4_PWM_TO_I_TH GENMASK(7, 4)
+
+#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_CFG6_BST_FREQ GENMASK(7, 6)
+#define MSK_CFG6_VBST GENMASK(5, 0)
+
+#define BIT_CFG7_EN_DRV3 BIT(5)
+#define BIT_CFG7_EN_DRV2 BIT(4)
+#define MSK_CFG7_IBST_LIM GENMASK(1, 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)
+
+#define BIT_CFGA_SSCLK_EN BIT(7)
+#define BIT_CFGA_ADAPTIVE BIT(3)
+#define MSK_CFGA_DRIVER_HEADROOM GENMASK(2, 0)
+#define MSK_CFGE_STEP_UP GENMASK(7, 6)
+#define MSK_CFGE_STEP_DN GENMASK(5, 4)
+#define MSK_CFGE_LED_FAULT_TH GENMASK(3, 2)
+#define MSK_CFGE_LED_COMP_HYST 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");
+
+	usleep_range(100, 200);
+
+	/* 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);
+
+	/* 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_LICENSE("GPL");

-- 
2.34.1


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

* Claude review: backlight: Add SY7758 6-channel High Efficiency LED Driver support
  2026-04-30  9:47 [PATCH v2 0/2] backlight: Add SY7758 6-channel High Efficiency LED Driver support Neil Armstrong
  2026-04-30  9:47 ` [PATCH v2 1/2] dt-bindings: leds: backlight: document the SY7758 6-channel High Efficiency LED Driver Neil Armstrong
  2026-04-30  9:47 ` [PATCH v2 2/2] backlight: Add SY7758 6-channel High Efficiency LED Driver support Neil Armstrong
@ 2026-05-05  0:44 ` Claude Code Review Bot
  2 siblings, 0 replies; 6+ messages in thread
From: Claude Code Review Bot @ 2026-05-05  0:44 UTC (permalink / raw)
  To: dri-devel-reviews

Overall Series Review

Subject: backlight: Add SY7758 6-channel High Efficiency LED Driver support
Author: Neil Armstrong <neil.armstrong@linaro.org>
Patches: 3
Reviewed: 2026-05-05T10:44:01.038993

---

This is a clean, well-structured 2-patch series adding a new I2C backlight driver for the Silergy SY7758 6-channel LED driver, used in the Ayaneo Pocket S2 handheld. The DT binding (patch 1) already has a Reviewed-by from the DT maintainer. The driver (patch 2) is straightforward and follows existing backlight driver patterns. However, there are several issues ranging from a typo in Kconfig to a significant design concern about hardcoded board-specific initialization values that should be driven by devicetree properties.

**Key concerns:**
1. Board-specific hardware configuration hardcoded in `sy7758_init()` rather than read from DT
2. Kconfig help text typo ("Socket S2" vs "Pocket S2")
3. Signed-off-by chain ordering
4. Several unused struct fields and defines

---

---
Generated by Claude Code Patch Reviewer

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

* Claude review: dt-bindings: leds: backlight: document the SY7758 6-channel High Efficiency LED Driver
  2026-04-30  9:47 ` [PATCH v2 1/2] dt-bindings: leds: backlight: document the SY7758 6-channel High Efficiency LED Driver Neil Armstrong
@ 2026-05-05  0:44   ` Claude Code Review Bot
  0 siblings, 0 replies; 6+ messages in thread
From: Claude Code Review Bot @ 2026-05-05  0:44 UTC (permalink / raw)
  To: dri-devel-reviews

Patch Review

**Overall**: Clean binding. Already has a Reviewed-by from Krzysztof Kozlowski. Minor suggestions only.

**Good:**
- Correctly references `common.yaml#` via `allOf`, inheriting `default-brightness` and `max-brightness` properties
- Uses `unevaluatedProperties: false` properly
- Example is well-formed

**Minor note:** The binding inherits `default-brightness` and `max-brightness` from `common.yaml`, but the driver (patch 2) doesn't read these properties. This means they'll be accepted in DT but silently ignored by the driver, which could be confusing.

---

---
Generated by Claude Code Patch Reviewer

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

* Claude review: backlight: Add SY7758 6-channel High Efficiency LED Driver support
  2026-04-30  9:47 ` [PATCH v2 2/2] backlight: Add SY7758 6-channel High Efficiency LED Driver support Neil Armstrong
@ 2026-05-05  0:44   ` Claude Code Review Bot
  0 siblings, 0 replies; 6+ messages in thread
From: Claude Code Review Bot @ 2026-05-05  0:44 UTC (permalink / raw)
  To: dri-devel-reviews

Patch Review

**Signed-off-by ordering issue:**
The `From:` header indicates KancyJoe is the original author, but the SOB chain is:
```
Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
Signed-off-by: KancyJoe <kancy2333@outlook.com>
```
The original author's SOB should come first, then the submitter/forwarder's:
```
Signed-off-by: KancyJoe <kancy2333@outlook.com>
Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
```

**Kconfig typo (line 216):**
```
backlight controller found in Ayaneo Socket S2.
```
Should be "Ayaneo **Pocket** S2".

**Hardcoded board-specific init values (`sy7758_init()`, lines 159-210):**
This is the most significant design concern. The `sy7758_init()` function hardcodes specific current settings, PWM frequency, boost voltage limits, and UVLO thresholds:
```c
ret = regmap_write(sydev->regmap, REG_OTP_CFG5,
                   FIELD_PREP(MSK_CFG5_PS_MODE, 6) |
                   FIELD_PREP(MSK_CFG5_PWM_FREQ, 4));
...
ret = regmap_write(sydev->regmap, REG_OTP_CFG0,
                   FIELD_PREP(MSK_CFG0_CURRENT_LOW, 85));
...
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 are configuration values specific to the Ayaneo Pocket S2 board (LED current limits, PWM mode, etc.) and should come from DT properties or at least be defaults that can be overridden. If this chip is ever used on another board, the driver as-is would apply incorrect current/voltage settings, which could damage the hardware. At minimum, these magic numbers deserve defines or comments explaining what physical values they correspond to.

**Duplicate define with same value (lines 66-67):**
```c
#define BIT_STATUS2_OCP50MS_LATCH BIT(0)
#define BIT_STATUS2_OCP2 BIT(0)
```
Both map to `BIT(0)`. If this is intentional (same physical bit, different semantic names), it's fine but worth a comment. If one of them should be a different bit, it's a bug. Neither is used in the driver so this is low-impact.

**Unnecessary `FIELD_PREP` with full-byte mask (lines 140, 170, 187-188):**
```c
FIELD_PREP(MSK_BRT_12BIT_L, brightness & 0xff)
```
where `MSK_BRT_12BIT_L` is `GENMASK(7, 0)`. `FIELD_PREP` with a mask of all 8 bits is a no-op — it just returns the value unchanged. Not a bug, but misleading — it implies bit-field positioning when there is none. Same applies to `MSK_CFG0_CURRENT_LOW`.

**`MAX_BRIGHTNESS` is 4080, not 4095 (line 21):**
```c
#define MAX_BRIGHTNESS 4080
```
For a 12-bit brightness register, the maximum is 4095 (0xFFF). 4080 = 0xFF0 = 255 * 16. This may be a hardware limitation of this specific chip, but it's not documented. A brief comment explaining why 4080 rather than 4095 would help future readers.

**Unused struct fields:**
- `sydev->client` (line 121, set at line 227): stored but never read after assignment. All I2C access goes through `sydev->regmap`.
- `sydev->gpio` (line 123, set at line 241): stored but never referenced after probe. Since `devm_gpiod_get()` is managed, storing the descriptor is only needed if `remove()`/suspend needs to toggle the GPIO — which this driver doesn't do.

**Unnecessary initialization (line 161):**
```c
int ret = 0;
```
`ret` is always assigned before use; the `= 0` is dead.

**`sy7758_remove()` only calls `backlight_disable()` (lines 278-283):**
The remove function disables the backlight but doesn't de-assert the enable GPIO or do any other hardware shutdown. Since `devm_gpiod_get()` was used with `GPIOD_OUT_HIGH`, the GPIO won't be automatically driven low on remove (devm cleanup frees the descriptor, but doesn't reset the output). Depending on hardware requirements, the chip may continue consuming power after the driver is unloaded.

**No parsing of `default-brightness` / `max-brightness` DT properties:**
The binding inherits these from `common.yaml`, but the driver uses hardcoded `DEFAULT_BRIGHTNESS` (1024) and `MAX_BRIGHTNESS` (4080). A DT user setting `default-brightness = <2048>` would have no effect.

**Large number of unused register/field defines:**
Many defines (e.g., `REG_OTP_CFG98`, `REG_OTP_CFG9E`, `REG_OTP_CFG3`, `REG_OTP_CFG4`, `REG_OTP_CFG6`, `REG_OTP_CFG7`, `REG_OTP_CFGA`, `REG_OTP_CFGE`, and most `BIT_`/`MSK_` macros) are never used. While documenting the register map has value, some reviewers prefer adding defines only when they're needed to reduce noise. This is a style preference.

---
Generated by Claude Code Patch Reviewer

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

end of thread, other threads:[~2026-05-05  0:44 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-04-30  9:47 [PATCH v2 0/2] backlight: Add SY7758 6-channel High Efficiency LED Driver support Neil Armstrong
2026-04-30  9:47 ` [PATCH v2 1/2] dt-bindings: leds: backlight: document the SY7758 6-channel High Efficiency LED Driver Neil Armstrong
2026-05-05  0:44   ` Claude review: " Claude Code Review Bot
2026-04-30  9:47 ` [PATCH v2 2/2] backlight: Add SY7758 6-channel High Efficiency LED Driver support Neil Armstrong
2026-05-05  0:44   ` Claude review: " Claude Code Review Bot
2026-05-05  0:44 ` Claude Code Review Bot

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