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: sy7758: add Silergy SY7758 backlight driver Date: Sat, 16 May 2026 16:17:42 +1000 Message-ID: In-Reply-To: <20260511-sy7758-v1-2-999a33081304@gmail.com> References: <20260511-sy7758-v1-0-999a33081304@gmail.com> <20260511-sy7758-v1-2-999a33081304@gmail.com> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Patch Review **Bug =E2=80=94 global static mutex instead of per-device:** ```c static DEFINE_MUTEX(sy7758_update_backlight_mutex); ``` This is a single mutex shared by *all* instances of the driver. If two SY77= 58 chips exist on different I2C buses, their brightness updates serialize a= gainst each other needlessly. The mutex should be a member of `struct sy775= 8` and initialized with `mutex_init()` in probe (or use `DEFINE_MUTEX` equi= valent per-allocation). The only other backlight driver using a static mute= x (`kb3886_bl.c`) is an ancient single-instance driver accessing global por= t I/O =E2=80=94 not a pattern to follow. **Bug =E2=80=94 brightness values 1-15 silently produce zero:** ```c sy7758_write(sydev, BL_BRT_L, brightness & 0xf0); sy7758_write(sydev, BL_BRT_H, (brightness >> 8) & 0xf); ``` The mask `brightness & 0xf0` discards the low 4 bits. For userspace brightn= ess values 1 through 15, both register writes produce 0x00, making the back= light fully off =E2=80=94 yet `sydev->led_on` remains `true` (it was set in= `sy7758_init()` or by the `if` branch above). This means a subsequent writ= e of brightness=3D0 won't trigger re-initialization, it'll just clear `led_= on`. Then a later non-zero brightness will correctly re-init. The practical= effect is that values 1-15 behave identically to 0 but without proper shut= down/re-init semantics. Given MAX_BRIGHTNESS=3D4080 (0xFF0) and the 4-bit discard, there are only ~= 256 distinct hardware levels out of the 4081 userspace values. Consider eit= her (a) making `max_brightness =3D 255` and shifting internally, or (b) doc= umenting that the effective step size is 16. **Major concern =E2=80=94 no error checking on register writes:** ```c static void sy7758_init(struct sy7758 *sydev) { sy7758_write(sydev, 0x01, 0x85); sy7758_write(sydev, 0x10, 0x00); ... ``` `sy7758_write()` returns `int` (propagating `regmap_write()`'s error), but = every caller discards the return value. If any write fails during `sy7758_i= nit()`, the chip could be left in a partially-configured state with no indi= cation to userspace. `sy7758_init()` should return `int` and propagate erro= rs, and `update_status()` should propagate write errors instead of uncondit= ionally returning 0. **Magic register values with no documentation:** ```c sy7758_write(sydev, 0x01, 0x85); sy7758_write(sydev, 0xa5, 0x64); sy7758_write(sydev, 0xa0, 0x55); sy7758_write(sydev, 0xa1, 0x9a); sy7758_write(sydev, 0xa9, 0x80); sy7758_write(sydev, 0xa2, 0x28); ``` All register addresses and values are bare hex literals. There's no way for= a reviewer to verify correctness without a datasheet reference. Each regis= ter should have a `#define` with a meaningful name (e.g., `SY7758_REG_MODE`= , `SY7758_REG_LED_CURRENT`, etc.), and the values should be explained (even= briefly =E2=80=94 e.g., "enable 4-channel mode" or "set LED current to 20m= A"). **Redundant double-init in probe:** ```c sy7758_init(sydev); i2c_set_clientdata(client, backlight_dev); backlight_update_status(backlight_dev); ``` `sy7758_init()` programs the chip including writing brightness registers (0= x10=3D0x40, 0x11=3D0x01 =E2=86=92 brightness=3D0x140=3D320). Then `backligh= t_update_status()` is called, which sees `led_on =3D=3D true` (set by `sy77= 58_init`) and writes the `DEFAULT_BRIGHTNESS` (1500) to the brightness regi= sters. The init brightness write is immediately overwritten. Either remove = the brightness register writes from `sy7758_init()`, or don't call `sy7758_= init()` directly from probe (let the first `update_status` trigger it via t= he `!led_on` path). **Unused forward declaration:** ```c static void sy7758_init(struct sy7758 *sydev); ``` This can be eliminated by moving `sy7758_init()` above `sy7758_backlight_up= date_status()`. **Unused includes:** ```c #include #include ``` The driver does not use any GPIO consumer API. `linux/of.h` is not strictly= needed =E2=80=94 `struct of_device_id` comes from `linux/mod_devicetable.h= ` which is pulled in transitively. **Unused struct member:** ```c struct sy7758 { struct i2c_client *client; struct regmap *regmap; bool led_on; }; ``` `sydev->client` is assigned in probe but never referenced again. Everything= goes through `regmap`. Remove it unless future use is planned. **`memset` instead of zero-initialization:** ```c memset(&props, 0, sizeof(props)); ``` Preferred style for stack variables in modern kernel code is `struct backli= ght_properties props =3D { };`. **Trivial wrapper adds no value:** ```c static int sy7758_write(struct sy7758 *sydev, unsigned int reg, unsigned int val) { return regmap_write(sydev->regmap, reg, val); } ``` This is a 1:1 forwarding wrapper. Either add value (e.g., error logging, tr= acing) or call `regmap_write()` directly. **Copyright/authorship discrepancy:** The driver header says `Copyright (C) 2025 Kancy Joe `, but Kancy Joe doesn't appear in the commit's Signed-off-by chain or MAI= NTAINERS. The commit is signed by Alexandre Hamamdjian and co-developed by = Philippe Simons. If the code originated from Kancy Joe's work, the attribut= ion chain should be clear (e.g., Co-developed-by or Original-author tag). **Comments that belong in the commit, not the code:** ```c // Max brightness // 0x10: 0xf0 Low // 0x11: 0x0f High // Min brightness // 0x10: 0x10 Low // 0x11: 0x00 High ``` These read like developer notes, not code documentation. If they explain th= e register encoding for maintainability, they should be at the register `#d= efine` sites. As-is they sit after two writes of hardcoded values and are m= isleading (they describe values different from what was just written). --- Generated by Claude Code Patch Reviewer