From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: Re: [PATCH 2/2] drm/bridge: lt9211: Add drive-strength-microamp DT property Date: Sat, 16 May 2026 13:11:28 +1000 Message-ID: In-Reply-To: References: <20260512164609.3390700-1-bstruempfel@data-modul.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 **Looks good.** The implementation is correct and backward compatible. **Register value correctness**: The original hardcoded value was `0x48`: ```c { 0x823f, 0x48 }, ``` The patch changes this to: ```c { 0x823f, 0x40 | ctx->lvds_hsdrv_isel }, ``` Since `0x48 =3D 0x40 | 0x08` and the default `lvds_hsdrv_isel =3D 8`, this = correctly preserves the original value when the DT property is absent. The = upper nibble (`0x40`) is preserved while bits `[3:0]` carry the drive-stren= gth select. This is correct. **DT parsing**: The linear search through `lt9211_hsdrv_microamp[]` (16 ent= ries) is fine, and the error path for invalid values is handled correctly w= ith `dev_err` + `-EINVAL`. Minor points: 1. **`of_property_read_u32` vs `device_property_read_u32`**: The patch uses= `of_property_read_u32()`, which is consistent with the rest of this driver= (it's heavily OF-dependent with `of_graph_*` calls). Modern kernel convent= ion prefers the firmware-agnostic `device_property_read_u32(dev, ...)`, but= consistency with the existing driver is a reasonable choice here. 2. **Lookup table could use `match_string`-style helper**: The kernel has `= match_u32_property_value()` or similar patterns, but the explicit loop is p= erfectly clear for 16 elements. No change needed. 3. **The `const` array becomes runtime-initialized**: `system_lt9211_tx_phy= _seq[]` was previously fully compile-time constant (eligible for `.rodata`)= . With `ctx->lvds_hsdrv_isel`, it's now stack-initialized at each call. Thi= s is fine =E2=80=94 the same pattern already exists in the array with `ctx-= >lvds_dual_link` on the line just above: ```c { 0x823b, 0x38 | (ctx->lvds_dual_link ? BIT(7) : 0) }, ``` So this is consistent with the existing code. 4. **Validation redundancy**: The DT schema enum already constrains values,= but the driver validates too. This is good practice since not all DTs are = schema-validated at runtime. --- Generated by Claude Code Patch Reviewer