From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: clk: renesas: rzv2h: Add PLLDSI clk mux support Date: Sun, 12 Apr 2026 12:45:55 +1000 Message-ID: In-Reply-To: <24d3853ca2522df21e6a071a23e23ba4ca4b7276.1775636898.git.tommaso.merciai.xr@bp.renesas.com> References: <24d3853ca2522df21e6a071a23e23ba4ca4b7276.1775636898.git.tommaso.merciai.xr@bp.renesas.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 This is the largest and most complex patch in the series. It adds the PLLDS= I 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 und= erstand 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 =3D 0; else parent_idx =3D 1; ``` Using `set_duty_cycle`/`get_duty_cycle` as a mechanism to select clock pare= nts is unconventional. The duty cycle API was designed to reflect actual el= ectrical duty cycle properties of clocks, not as a side-channel for mux sel= ection. This works, but it may confuse future readers. The v2 changelog say= s this was preferred over using clk-provider API to select the right parent= directly =E2=80=94 it would be good to understand why the simpler approach= of just selecting the parent directly was rejected. Was there a review req= uest 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 =3D to_pll(clk_hw_get_parent(hw)); ``` This gets the parent of the mux clock and casts it to `pll_clk`. But the mu= x has two possible parents (div7 and csdiv), and the parent at call time ma= y not correspond to the path being calculated. If `get_parent` currently re= turns the LVDS path (index 0), but the caller intends DSI, we'd still get t= he 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 + smu= x.width > 16` in the register function is good defensive programming for th= e LOWORD mask. --- Generated by Claude Code Patch Reviewer