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

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

Patch Review

Overall a solid driver. A few items to flag:

**1. Potential refcount issue with `bridge.next_bridge` assignment (medium concern):**

```c
lvds->bridge.next_bridge = devm_drm_of_get_bridge(dev, dev->of_node, 1, 0);
```

The `next_bridge` field is documented as "automatically put when this bridge is freed" — i.e., `drm_bridge_put()` is called on it at destroy time. However, `devm_drm_of_get_bridge()` returns either a devm-managed panel bridge (refcount 1, also devm-cleaned) or an unrefcounted bridge pointer from `of_drm_find_bridge()`. Storing it directly into `next_bridge` without calling `drm_bridge_get()` risks a refcount mismatch — the auto-put at bridge destruction would underflow.

All existing in-tree drivers that store into `bridge.next_bridge` use `of_drm_find_and_get_bridge()`, `of_drm_get_bridge_by_endpoint()`, or `drm_bridge_get()` — functions that increment the refcount. Drivers that use `devm_drm_of_get_bridge()` store the result in a private struct field instead (like `rzg2l_mipi_dsi.c` does with `dsi->next_bridge`).

Consider either storing in a private `lvds->next_bridge` field (matching the pattern in `rzg2l_mipi_dsi.c`), or using `of_drm_get_bridge_by_endpoint()` with a `drm_bridge_get()` for the `bridge.next_bridge` assignment.

**2. Unnecessary include:**

```c
#include <linux/of_device.h>
```

This header is not needed — the driver doesn't use any `of_device_*` functions. The sibling `rzg2l_mipi_dsi.c` driver only includes `<linux/of.h>` and `<linux/of_graph.h>`. This include can be dropped.

**3. Register header: inconsistent bit-field style (`rzg3l_lvds_regs.h`):**

The header mixes `(1 << N)` with `BIT()`:

```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)
...
#define LVDS_0_PHY_CH_EN_BGR		BIT(8)
#define LVDS_0_PHY_CH_EN_LDO		BIT(9)
```

Prefer `BIT()` throughout for consistency with kernel style.

**4. Register header: redundant definitions:**

`LVDS_CMN_RST_PHY0_SEL` and `LVDS_CMN_RST_PHY0_SEL_CH0` are both defined as `(1 << 24)` — they are identical. Only `LVDS_CMN_RST_PHY0_SEL` is used in the driver; `LVDS_CMN_RST_PHY0_SEL_CH0` is dead code.

Similarly, `LVDS_0_PHY_CH_IO_EN` is defined as `(LVDS_0_PHY_CH_IO_EN_MSK << 0)` which equals `LVDS_0_PHY_CH_IO_EN_MSK` — the `<< 0` is a no-op and the two defines have the same value. This is confusing since one is used as a mask and the other as a value, but they're identical. Consider clarifying the intent or consolidating.

**5. Kconfig: `select DRM_PANEL` and `select DRM_KMS_HELPER`:**

```
config DRM_RZG3L_LVDS
	def_tristate DRM_RZG2L_DU
	depends on DRM_RZG3L_USE_LVDS
	select DRM_KMS_HELPER
	select DRM_PANEL
```

`DRM_KMS_HELPER` is already selected by `DRM_RZG2L_DU` (through `DRM_GEM_DMA_HELPER` dependencies). And `DRM_PANEL` — verify this is actually needed. The driver doesn't directly use `drm_panel_*` APIs; it uses `devm_drm_of_get_bridge()` which internally handles panels via panel-bridge. The `DRM_PANEL` select may be needed transitively for `devm_drm_of_get_bridge()` to find panels, but it would be worth confirming.

**6. Minor: `return ret` at end of probe:**

```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;
```

Since the error is already handled, `return ret` here is always `return 0`. Could be simplified to `return 0` for clarity, though functionally identical.

**7. The `attach` callback accesses `next_bridge` through the bridge struct:**

```c
static int rzg3l_lvds_attach(struct drm_bridge *bridge,
                 struct drm_encoder *encoder,
                 enum drm_bridge_attach_flags flags)
{
    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);
}
```

This is correct for the `bridge.next_bridge` approach, but note that returning 0 when there's no next bridge means the encoder pipeline would be incomplete (no connector). This only works if the bridge connector infrastructure handles it. If you switch to a private `next_bridge` field per item 1 above, this would need adjustment too.

**No issues with:**
- The `drm_atomic_commit` type usage (matches current in-tree bridge callback signatures)
- The `pm_runtime_get_sync` + `WARN_ON` pattern (appropriate for atomic enable which shouldn't fail)
- The reset handling (optional `rst`/`arst` asserted-and-left, shared `prst`/`lvdrst` toggled via PM runtime)
- The `devm_drm_bridge_alloc` usage (correct modern API)
- The mode_valid clock range checking (25-87 MHz matching the binding description)

---
Generated by Claude Code Patch Reviewer