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

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

Patch Review

This is the main driver patch. Several items to address:

**1. Unnecessary includes** (`rzg3l_lvds.c` lines 9, 12, 16, 17, 27, 28):

```c
#include <linux/clk.h>
#include <linux/io.h>
#include <linux/of_device.h>
#include <linux/of_graph.h>
...
#include <drm/drm_panel.h>
#include <drm/drm_probe_helper.h>
```

- `clk.h` — no `clk_*` calls anywhere in the driver (clocks managed by pm_runtime/syscon parent).
- `io.h` — no `ioread`/`iowrite`/`readl`/`writel` calls; registers accessed via regmap.
- `of_device.h` — no `of_match_device()` or other `of_device` APIs used. The sibling `rzg2l_mipi_dsi.c` doesn't include this.
- `of_graph.h` — no `of_graph_*` APIs called directly.
- `drm_panel.h` — no `drm_panel_*` functions called anywhere.
- `drm_probe_helper.h` — no probe helper functions called.

Please drop these unused includes.

**2. Register header: redundant and inconsistent definitions** (`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)
```

`LVDS_CMN_RST_PHY0_SEL` and `LVDS_CMN_RST_PHY0_SEL_CH0` have the same value and `_CH0` is never used. Drop `LVDS_CMN_RST_PHY0_SEL_CH0`.

Also, the file mixes `(1 << N)` with `BIT(N)` (used for bits 8 and 9). Use `BIT()` consistently throughout:
```c
#define LVDS_CMN_RST_PHY0_SEL   BIT(24)
#define LVDS_CMN_PHY_RESET       BIT(0)
```

Similarly:
```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 `0x1f << 0` which equals `LVDS_0_PHY_CH_IO_EN_MSK`. These are the same value, and the `<< 0` is a no-op. Should be `GENMASK(4, 0)` for the mask, and `LVDS_0_PHY_CH_IO_EN` can be dropped (just use the mask directly, since the value written is the mask itself).

**3. `pm_runtime_get_sync` error handling** (`rzg3l_lvds.c` lines 87-91):

```c
ret = pm_runtime_get_sync(lvds->dev);
if (ret < 0) {
    dev_err(lvds->dev, "pm_runtime_get_sync error\n");
    return;
}
```

The v2 changelog explains the switch from `pm_runtime_resume_and_get()` to `pm_runtime_get_sync()` because "atomic_enable doesn't fail and for each enable there always will be an atomic_disable() call." This rationale is valid — the DRM framework always pairs enable/disable. However, `pm_runtime_get_sync()` increments the usage count even on failure. If it fails and `atomic_disable` later calls `pm_runtime_put`, the count rebalances but the hardware was never properly initialized, and `atomic_disable` will write teardown register sequences to a device that never fully resumed.

Consider either:
- Using `pm_runtime_resume_and_get()` for cleaner semantics (the extra `pm_runtime_put` in disable on failure path is harmless since it just decrements to 0), or
- Dropping the error check entirely if this truly can't fail (since `atomic_enable` returns void and can't propagate errors anyway).

The current code is a hybrid that checks for errors but can't properly handle them.

**4. Probe ordering** (`rzg3l_lvds.c` lines 242-254):

```c
next_bridge = devm_drm_of_get_bridge(dev, dev->of_node, 1, 0);
if (IS_ERR(next_bridge))
    return dev_err_probe(dev, PTR_ERR(next_bridge),
                 "failed to get next bridge\n");

lvds->bridge.next_bridge = next_bridge;
ret = reset_control_assert(rstc);
...
ret = reset_control_assert(arstc);
```

The `rst` and `arst` resets (MIPI_DSI_CMN_RSTB and MIPI_DSI_ARESET_N) must be asserted as a precondition for LVDS operation. Currently they're asserted *after* `devm_drm_of_get_bridge`, which can return `-EPROBE_DEFER`. Consider moving the reset assertions earlier (right after obtaining them), so they're asserted regardless of deferred probe. This avoids repeated assert/defer/assert cycles and ensures the hardware precondition is established as early as possible.

**5. Dead code in attach** (`rzg3l_lvds.c` lines 134-135):

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

Since `probe()` returns an error if `devm_drm_of_get_bridge()` fails, `next_bridge` is always non-NULL by the time the bridge is registered and `attach` can be called. This check is dead code. It's a common defensive pattern and not harmful, but worth noting.

**6. Kconfig: `select DRM_PANEL` likely unnecessary**:

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

The driver doesn't call any `drm_panel_*` functions. Panel support is handled transparently through the bridge chain (via `devm_drm_of_get_bridge` which wraps panels as bridges). `select DRM_PANEL` and the `drm_panel.h` include appear to be unnecessary. `select DRM_KMS_HELPER` is also already selected by `DRM_RZG2L_DU` (which this depends on transitively via `def_tristate DRM_RZG2L_DU`), though being explicit isn't wrong.

**7. Minor: `return ret` at end of probe** (`rzg3l_lvds.c` line 261):

```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. Clearer to write `return 0;`.

**Summary for Patch 3:** The driver is functionally correct and follows established patterns. The main items to address are: cleaning up the unused includes, making the register definitions consistent (use `BIT()`/`GENMASK()`, drop redundant defs), and reconsidering the `pm_runtime_get_sync` error handling approach. The probe ordering of reset assertions could also be improved.

---
Generated by Claude Code Patch Reviewer