Claude review: backlight: sy7758: add Silergy SY7758 backlight driver

[Claude Code Review Bot <claude-review@example.com>]

Patch Review

**Bug — 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 SY7758 chips exist on different I2C buses, their brightness updates serialize against each other needlessly. The mutex should be a member of `struct sy7758` and initialized with `mutex_init()` in probe (or use `DEFINE_MUTEX` equivalent per-allocation). The only other backlight driver using a static mutex (`kb3886_bl.c`) is an ancient single-instance driver accessing global port I/O — not a pattern to follow.

**Bug — 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 brightness values 1 through 15, both register writes produce 0x00, making the backlight fully off — yet `sydev->led_on` remains `true` (it was set in `sy7758_init()` or by the `if` branch above). This means a subsequent write of brightness=0 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 shutdown/re-init semantics.

Given MAX_BRIGHTNESS=4080 (0xFF0) and the 4-bit discard, there are only ~256 distinct hardware levels out of the 4081 userspace values. Consider either (a) making `max_brightness = 255` and shifting internally, or (b) documenting that the effective step size is 16.

**Major concern — 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_init()`, the chip could be left in a partially-configured state with no indication to userspace. `sy7758_init()` should return `int` and propagate errors, and `update_status()` should propagate write errors instead of unconditionally 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 register 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 — e.g., "enable 4-channel mode" or "set LED current to 20mA").

**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 (0x10=0x40, 0x11=0x01 → brightness=0x140=320). Then `backlight_update_status()` is called, which sees `led_on == true` (set by `sy7758_init`) and writes the `DEFAULT_BRIGHTNESS` (1500) to the brightness registers. 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 the `!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_update_status()`.

**Unused includes:**

```c
#include <linux/gpio/consumer.h>
#include <linux/of.h>
```

The driver does not use any GPIO consumer API. `linux/of.h` is not strictly needed — `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 backlight_properties props = { };`.

**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, tracing) or call `regmap_write()` directly.

**Copyright/authorship discrepancy:**

The driver header says `Copyright (C) 2025 Kancy Joe <kancy2333@outlook.com>`, but Kancy Joe doesn't appear in the commit's Signed-off-by chain or MAINTAINERS. The commit is signed by Alexandre Hamamdjian and co-developed by Philippe Simons. If the code originated from Kancy Joe's work, the attribution 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 the register encoding for maintainability, they should be at the register `#define` sites. As-is they sit after two writes of hardcoded values and are misleading (they describe values different from what was just written).

---
Generated by Claude Code Patch Reviewer