Claude review: clk: renesas: rzv2h: Add PLLDSI clk mux support

Claude Code Review Bot
2026-04-12T02:45:55Z

Patch Review

This is the largest and most complex patch in the series. It adds the PLLDSI SMUX clock mux infrastructure to the RZ/V2H CPG driver.

**Issue 1 (minor): Wrapping `clk_mux_ops` calls**

```c
static u8 rzv2h_cpg_plldsi_smux_get_parent(struct clk_hw *hw)
{
return clk_mux_ops.get_parent(hw);
}

static int rzv2h_cpg_plldsi_smux_set_parent(struct clk_hw *hw, u8 index)
{
return clk_mux_ops.set_parent(hw, index);
}
```

These are trivial wrappers that just delegate to `clk_mux_ops`. While I understand they may be needed because the ops table must use a consistent set of function pointers, it would be worth a comment explaining why they can't just reference the `clk_mux_ops` callbacks directly in the ops struct.

**Issue 2 (design): Duty cycle as a mux selection mechanism**

```c
static int rzv2h_cpg_plldsi_smux_set_duty_cycle(struct clk_hw *hw,
struct clk_duty *duty)
{
if (duty->num * CPG_PLLDSI_SMUX_DSI_RGB_DUTY_DEN >
duty->den * CPG_PLLDSI_SMUX_DSI_RGB_DUTY_NUM)
parent_idx = 0;
else
parent_idx = 1;
```

Using `set_duty_cycle`/`get_duty_cycle` as a mechanism to select clock parents is unconventional. The duty cycle API was designed to reflect actual electrical duty cycle properties of clocks, not as a side-channel for mux selection. This works, but it may confuse future readers. The v2 changelog says this was preferred over using clk-provider API to select the right parent directly — it would be good to understand why the simpler approach of just selecting the parent directly was rejected. Was there a review request for this approach?

**Issue 3 (minor): `determine_rate` gets parent via `clk_hw_get_parent`**

```c
static int rzv2h_cpg_plldsi_smux_determine_rate(struct clk_hw *hw,
struct clk_rate_request *req)
{
struct pll_clk *pll_clk = to_pll(clk_hw_get_parent(hw));
```

This gets the parent of the mux clock and casts it to `pll_clk`. But the mux has two possible parents (div7 and csdiv), and the parent at call time may not correspond to the path being calculated. If `get_parent` currently returns the LVDS path (index 0), but the caller intends DSI, we'd still get the correct PLL via `clk_hw_get_parent(hw)` since both paths share the same PLL grandparent. However, this indirect assumption is fragile.

Otherwise the patch is well-structured. The validation of `smux.shift + smux.width > 16` in the register function is good defensive programming for the LOWORD mask.

---
Generated by Claude Code Patch Reviewer