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: video: leds: backlight: lm3533: Support getting LED sources from DT
Date: Mon, 18 May 2026 16:19:47 +1000
Message-ID: <review-patch6-20260517074306.30937-7-clamor95@gmail.com>
In-Reply-To: <20260517074306.30937-7-clamor95@gmail.com>
References: <20260517074306.30937-1-clamor95@gmail.com> <20260517074306.30937-7-clamor95@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

This patch has multiple bugs:

**Bug =E2=80=94 wrong register in LED driver**: The second `lm3533_update` =
for LVLED4/5 writes to `LM3533_REG_OUTPUT_CONF1` instead of `LM3533_REG_OUT=
PUT_CONF2`:
```c
/* LVLED4 and LVLED5 */
ret =3D lm3533_update(led->lm3533, LM3533_REG_OUTPUT_CONF1,  // should be O=
UTPUT_CONF2
                    output_cfg_val >> OUTPUT_CONF2_SHIFT,
                    OUTPUT_CONF2_MASK);
```

**Bug =E2=80=94 off-by-one in led-sources reading**: Both drivers have the =
same issue:
```c
if (led->num_leds > 0 && led->num_leds < LM3533_MAX_LEDS) {
```
This excludes the case where `num_leds =3D=3D LM3533_MAX_LEDS` (5), meaning=
 if all 5 LVLEDs are specified, the array is never read. The condition shou=
ld be `<=3D`:
```c
if (led->num_leds > 0 && led->num_leds <=3D LM3533_MAX_LEDS) {
```
Same issue in the backlight driver with `LM3533_MAX_LED_STRINGS` (2) =E2=80=
=94 specifying both HVLED strings skips the read.

**Design flaw =E2=80=94 wide masks clobber other banks**: The output config=
 registers are shared between all banks. The LED driver uses `OUTPUT_CONF1_=
MASK =3D GENMASK(7, 2)` which covers ALL three LVLEDs, and `OUTPUT_CONF2_MA=
SK =3D GENMASK(3, 0)` covers both LVLED4/5. Each LED bank probe overwrites =
the entire set of LVLED mappings, clobbering configurations from previously=
-probed banks.

Compare with the existing `lm3533_set_lvled_config()` which correctly compu=
tes a **per-LVLED** 2-bit mask:
```c
mask =3D LM3533_LED_ID_MASK << shift;
val =3D led << shift;
ret =3D lm3533_update(lm3533, reg, val, mask);
```

The patch should use narrow per-LVLED masks or, better yet, call the existi=
ng `lm3533_set_lvled_config`/`lm3533_set_hvled_config` functions instead of=
 reimplementing the register manipulation.

**Same clobbering issue in backlight**: The backlight driver uses `OUTPUT_C=
ONF1_MASK =3D GENMASK(1, 0)`, writing both HVLED config bits at once. If ba=
nk A probes first and sets HVLED1, bank B probing second will overwrite bot=
h bits.

---
Generated by Claude Code Patch Reviewer