From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: backlight: Add SY7758 6-channel High Efficiency LED Driver support Date: Tue, 05 May 2026 10:44:01 +1000 Message-ID: In-Reply-To: <20260430-topic-sm8650-ayaneo-pocket-s2-sy7758-v2-2-308140640de9@linaro.org> References: <20260430-topic-sm8650-ayaneo-pocket-s2-sy7758-v2-0-308140640de9@linaro.org> <20260430-topic-sm8650-ayaneo-pocket-s2-sy7758-v2-2-308140640de9@linaro.org> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Patch Review **Signed-off-by ordering issue:** The `From:` header indicates KancyJoe is the original author, but the SOB c= hain is: ``` Signed-off-by: Neil Armstrong Signed-off-by: KancyJoe ``` The original author's SOB should come first, then the submitter/forwarder's: ``` Signed-off-by: KancyJoe Signed-off-by: Neil Armstrong ``` **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 h= ardcodes specific current settings, PWM frequency, boost voltage limits, an= d UVLO thresholds: ```c ret =3D regmap_write(sydev->regmap, REG_OTP_CFG5, FIELD_PREP(MSK_CFG5_PS_MODE, 6) | FIELD_PREP(MSK_CFG5_PWM_FREQ, 4)); ... ret =3D regmap_write(sydev->regmap, REG_OTP_CFG0, FIELD_PREP(MSK_CFG0_CURRENT_LOW, 85)); ... ret =3D 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 le= ast be defaults that can be overridden. If this chip is ever used on anothe= r board, the driver as-is would apply incorrect current/voltage settings, w= hich could damage the hardware. At minimum, these magic numbers deserve def= ines 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-imp= act. **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 =E2=80=94 it just returns the value unchanged. Not a bug= , but misleading =E2=80=94 it implies bit-field positioning when there is n= one. 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 =3D 0xF= F0 =3D 255 * 16. This may be a hardware limitation of this specific chip, b= ut it's not documented. A brief comment explaining why 4080 rather than 409= 5 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 af= ter probe. Since `devm_gpiod_get()` is managed, storing the descriptor is o= nly needed if `remove()`/suspend needs to toggle the GPIO =E2=80=94 which t= his driver doesn't do. **Unnecessary initialization (line 161):** ```c int ret =3D 0; ``` `ret` is always assigned before use; the `=3D 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). Dependi= ng on hardware requirements, the chip may continue consuming power after th= e driver is unloaded. **No parsing of `default-brightness` / `max-brightness` DT properties:** The binding inherits these from `common.yaml`, but the driver uses hardcode= d `DEFAULT_BRIGHTNESS` (1024) and `MAX_BRIGHTNESS` (4080). A DT user settin= g `default-brightness =3D <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 regist= er map has value, some reviewers prefer adding defines only when they're ne= eded to reduce noise. This is a style preference. --- Generated by Claude Code Patch Reviewer