From mboxrd@z Thu Jan  1 00:00:00 1970
From: Claude Code Review Bot <claude-review@example.com>
To: dri-devel-reviews@example.com
Subject: Claude review: backlight: Add SY7758 6-channel High Efficiency LED Driver support
Date: Mon, 25 May 2026 20:43:17 +1000
Message-ID: <review-patch2-20260521-topic-sm8650-ayaneo-pocket-s2-sy7758-v4-2-73c732615e4a@linaro.org>
In-Reply-To: <20260521-topic-sm8650-ayaneo-pocket-s2-sy7758-v4-2-73c732615e4a@linaro.org>
References: <20260521-topic-sm8650-ayaneo-pocket-s2-sy7758-v4-0-73c732615e4a@linaro.org> <20260521-topic-sm8650-ayaneo-pocket-s2-sy7758-v4-2-73c732615e4a@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

**Makefile ordering (minor):** The new entry is placed between `SAHARA` and=
 `SKY81452`, but alphabetically `SY7758` sorts after `SKY81452`:

```c
 obj-$(CONFIG_BACKLIGHT_SAHARA)		+=3D kb3886_bl.o
+obj-$(CONFIG_BACKLIGHT_SY7758)		+=3D sy7758.o
 obj-$(CONFIG_BACKLIGHT_SKY81452)	+=3D sky81452-backlight.o
```

Should be:

```
 obj-$(CONFIG_BACKLIGHT_SKY81452)	+=3D sky81452-backlight.o
+obj-$(CONFIG_BACKLIGHT_SY7758)		+=3D sy7758.o
 obj-$(CONFIG_BACKLIGHT_TPS65217)	+=3D tps65217_bl.o
```

**Kconfig ordering (minor):** The entry is placed between `KTZ8866` (K) and=
 `LM3533` (L), but `SY7758` (S) should sort much later. That said, the exis=
ting Kconfig is not strictly alphabetical (e.g. `HP680` appears after `OMAP=
1`, `APPLE` after `MT6370`), so this is cosmetic at best.

**Redundant `FIELD_PREP` for bit-0 fields (style nit):** `MSK_BRT_12BIT_L` =
is `GENMASK(7, 0)` and `MSK_BRT_12BIT_H` is `GENMASK(3, 0)`. Since both mas=
ks start at bit 0, `FIELD_PREP` is a no-op =E2=80=94 it doesn't shift. The =
explicit `& 0xff` / `& 0xf` masks are also redundant when using `FIELD_PREP=
`, since it already masks the value:

```c
ret =3D regmap_write(sydev->regmap, REG_BRT_12BIT_L,
                   FIELD_PREP(MSK_BRT_12BIT_L,
                              brightness & 0xff));
```

Either use `FIELD_PREP` alone (dropping the manual masks) or drop `FIELD_PR=
EP` entirely (just write `brightness & 0xff`). Having both is belt-and-susp=
enders that adds clutter. The same pattern appears in `sy7758_init` for the=
 default brightness writes.

**Unnecessary initialization (trivial):** In `sy7758_init`:

```c
int ret =3D 0;
```

`ret` is immediately assigned on the next line, so `=3D 0` is unnecessary.

**`update_status` return style (trivial):** The trailing `return 0` could b=
e simplified:

```c
ret =3D regmap_write(sydev->regmap, REG_BRT_12BIT_H,
                   FIELD_PREP(MSK_BRT_12BIT_H,
                              (brightness >> 8) & 0xf));
if (ret)
        return ret;

return 0;
```

Could simply be `return regmap_write(...)` for the last write, or `return r=
et;` instead of the `if`/`return 0` pair. This matches patterns in other ba=
cklight drivers.

**Hardcoded init configuration (observation, not a blocker):** `sy7758_init=
` writes fixed values to multiple OTP/config registers (current limits, boo=
st voltage, PWM frequency, etc.). These are specific to the Ayaneo Pocket S=
2. If this chip is used on other boards, these values would need to come fr=
om DT properties. Acceptable for now given there is only one user, but wort=
h noting for future extensibility.

**`MAX_BRIGHTNESS =3D 4080` (question):** The 12-bit register range would s=
upport up to 4095 (0xFFF). The chosen max of 4080 (0xFF0) presumably comes =
from the datasheet =E2=80=94 if so, no issue, but it's worth confirming thi=
s isn't a typo.

**Everything else looks correct:** Probe sequence handles errors well with =
consistent `dev_err_probe` usage, devm resource management is clean, the de=
vice ID check at probe is good practice, and `sy7758_remove` calling `backl=
ight_disable` before devm teardown is correct.

---
Generated by Claude Code Patch Reviewer