Claude review: drm: renesas: rz-du: Add support for RZ/G3L LVDS encoder

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

Patch Review

**Issues:**

**(1) `rst` and `arst` reset controls are fire-and-forget (medium severity)**

In `rzg3l_lvds_probe()`, the `rstc` and `arstc` resets are acquired, asserted, and then the local variables go out of scope. They are never stored in `struct rzg3l_lvds`, so they can never be deasserted:

```c
struct reset_control *rstc, *arstc;
...
rstc = devm_reset_control_get_optional_exclusive(dev, "rst");
...
arstc = devm_reset_control_get_optional_exclusive(dev, "arst");
...
ret = reset_control_assert(rstc);
...
ret = reset_control_assert(arstc);
```

Per the cover letter, these resets "must be asserted before using the LVDS module" and should stay asserted. If this is intentional (assert once, never deassert), it works because `devm_*` will release the reset control on driver remove, but a comment would clarify the intent. Additionally, using `_exclusive` reset controls that are only asserted and never deasserted is unusual — if they truly must stay asserted for the lifetime of the device, consider whether `_shared` would be more appropriate if the DSI driver also needs to manipulate them.

**(2) `pm_runtime_get_sync` error path leaks PM refcount (medium severity)**

In `rzg3l_lvds_atomic_enable()`:

```c
if (WARN_ON(pm_runtime_get_sync(lvds->dev) < 0))
    return;
```

`pm_runtime_get_sync()` increments the reference count even on failure. On error, you must call `pm_runtime_put_noidle()` or `pm_runtime_put()` to avoid a PM refcount leak. The matching `pm_runtime_put()` in `rzg3l_lvds_atomic_disable()` will never be called if enable bailed out. Consider:
```c
if (WARN_ON(pm_runtime_get_sync(lvds->dev) < 0)) {
    pm_runtime_put_noidle(lvds->dev);
    return;
}
```
Or alternatively, use `pm_runtime_resume_and_get()` which doesn't increment the refcount on failure.

**(3) Unused includes (minor)**

Several headers appear unnecessary:
- `linux/of_device.h` — nothing from this header is used (no `of_device_get_match_data()` etc.)
- `linux/of_graph.h` — no `of_graph_*` functions are called
- `linux/bitfield.h` — wait, `FIELD_PREP` is used, so this one is needed
- `linux/mfd/syscon.h` — `syscon_node_to_regmap()` is used, so needed
- `linux/regmap.h` — `regmap_*` functions used, needed
- `linux/media-bus-format.h` — `MEDIA_BUS_FMT_*` used, needed
- `drm/drm_panel.h` — no panel functions called directly, but `DRM_PANEL` is selected in Kconfig. This include is not needed in this source file.
- `drm/drm_probe_helper.h` — no probe helper functions called.

**(4) Register header mixes `BIT()` and raw shifts (minor style)**

In `rzg3l_lvds_regs.h`:

```c
#define LVDS_CMN_RST_PHY0_SEL		(1 << 24)
#define LVDS_CMN_RST_PHY0_SEL_CH0	(1 << 24)
#define LVDS_CMN_PHY_RESET		(1 << 0)
```

These should use `BIT()` for consistency with the rest of the file which uses `BIT(8)`, `BIT(9)`, `GENMASK()`:

```c
#define LVDS_0_PHY_CH_EN_BGR		BIT(8)
#define LVDS_0_PHY_CH_EN_LDO		BIT(9)
```

Also, `LVDS_CMN_RST_PHY0_SEL` and `LVDS_CMN_RST_PHY0_SEL_CH0` are defined to the same value `(1 << 24)` — one of them appears redundant.

**(5) `LVDS_0_PHY_CH_IO_EN` is redundant (minor)**

```c
#define LVDS_0_PHY_CH_IO_EN_MSK		(0x1f)
#define LVDS_0_PHY_CH_IO_EN		(LVDS_0_PHY_CH_IO_EN_MSK << 0)
```

`LVDS_0_PHY_CH_IO_EN` is just `LVDS_0_PHY_CH_IO_EN_MSK << 0`, which is the same value. Having both is confusing. This could be simplified, or at least the `<< 0` should be removed.

**(6) `select DRM_PANEL` in Kconfig may be unnecessary (minor)**

The driver doesn't use any panel API directly — it uses `devm_drm_of_get_bridge()` which handles panel wrapping via `drm_panel_bridge`. The `DRM_PANEL` symbol is typically pulled in transitively by the bridge infrastructure. Check whether `DRM_PANEL` is really needed here.

**(7) `select DRM_KMS_HELPER` — check necessity (minor)**

Similar to above, `DRM_KMS_HELPER` is often pulled in by `DRM_BRIDGE`. Verify it's needed as a direct dependency.

**(8) The `return ret;` at the end of probe could be `return 0;` (nitpick)**

```c
ret = devm_drm_bridge_add(dev, &lvds->bridge);
if (ret)
    return dev_err_probe(dev, ret,
                 "Failed to register drm bridge\n");

return ret;
```

At this point `ret` is always 0 (the error case already returned). Using `return 0;` is clearer.

**(9) Version numbering confusion (meta)**

The cover letter changelog says "v2->v3" but the subject lines say `[PATCH v2 0/2]`. This appears to actually be v3 based on the changelog content.

**(10) Bridge attach returns 0 when there's no next_bridge (needs verification)**

```c
static int rzg3l_lvds_attach(...)
{
    struct rzg3l_lvds *lvds = bridge_to_rzg3l_lvds(bridge);

    if (!lvds->bridge.next_bridge)
        return 0;

    return drm_bridge_attach(encoder, lvds->bridge.next_bridge, bridge, flags);
}
```

The `next_bridge` is set from `devm_drm_of_get_bridge()` in probe, which returns an error if not found. So `next_bridge` should always be non-NULL if probe succeeded. This NULL check is harmless defensive code but shouldn't be reachable. Not a bug, but the check is dead code given the current probe logic.

---
Generated by Claude Code Patch Reviewer