From: Claude Code Review Bot <claude-review@example.com>
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 [thread overview]
Message-ID: <review-patch1-24d3853ca2522df21e6a071a23e23ba4ca4b7276.1775636898.git.tommaso.merciai.xr@bp.renesas.com> (raw)
In-Reply-To: <24d3853ca2522df21e6a071a23e23ba4ca4b7276.1775636898.git.tommaso.merciai.xr@bp.renesas.com>
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
next prev parent reply other threads:[~2026-04-12 2:45 UTC|newest]
Thread overview: 70+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-04-08 10:36 [PATCH v6 00/21] Add support for DU and DSI on the Renesas RZ/G3E SoC Tommaso Merciai
2026-04-08 10:36 ` [PATCH v6 01/21] clk: renesas: rzv2h: Add PLLDSI clk mux support Tommaso Merciai
2026-04-08 13:19 ` Geert Uytterhoeven
2026-04-12 2:45 ` Claude Code Review Bot [this message]
2026-04-08 10:36 ` [PATCH v6 02/21] clk: renesas: r9a09g047: Add CLK_PLLETH_LPCLK support Tommaso Merciai
2026-04-12 2:45 ` Claude review: " Claude Code Review Bot
2026-04-08 10:36 ` [PATCH v6 03/21] clk: renesas: r9a09g047: Add CLK_PLLDSI{0,1} clocks Tommaso Merciai
2026-04-12 2:45 ` Claude review: " Claude Code Review Bot
2026-04-08 10:36 ` [PATCH v6 04/21] clk: renesas: r9a09g047: Add CLK_PLLDSI{0, 1}_DIV7 clocks Tommaso Merciai
2026-04-12 2:45 ` Claude review: " Claude Code Review Bot
2026-04-08 10:36 ` [PATCH v6 05/21] clk: renesas: r9a09g047: Add CLK_PLLDSI{0, 1}_CSDIV clocks Tommaso Merciai
2026-04-12 2:45 ` Claude review: " Claude Code Review Bot
2026-04-08 10:36 ` [PATCH v6 06/21] clk: renesas: r9a09g047: Add support for SMUX2_DSI{0, 1}_CLK Tommaso Merciai
2026-04-08 13:23 ` Geert Uytterhoeven
2026-04-12 2:45 ` Claude review: " Claude Code Review Bot
2026-04-08 10:36 ` [PATCH v6 07/21] clk: renesas: r9a09g047: Add support for DSI clocks and resets Tommaso Merciai
2026-04-12 2:45 ` Claude review: " Claude Code Review Bot
2026-04-08 10:36 ` [PATCH v6 08/21] clk: renesas: r9a09g047: Add support for LCDC{0, 1} " Tommaso Merciai
2026-04-12 2:45 ` Claude review: " Claude Code Review Bot
2026-04-08 10:36 ` [PATCH v6 09/21] dt-bindings: display: renesas, rzg2l-du: Refuse port@1 for RZ/G2UL Tommaso Merciai
2026-04-08 12:21 ` [PATCH v6 09/21] dt-bindings: display: renesas,rzg2l-du: " Laurent Pinchart
2026-04-09 6:21 ` Krzysztof Kozlowski
2026-04-12 2:45 ` Claude review: dt-bindings: display: renesas, rzg2l-du: " Claude Code Review Bot
2026-04-08 10:36 ` [PATCH v6 10/21] dt-bindings: display: renesas, rzg2l-du: Add support for RZ/G3E SoC Tommaso Merciai
2026-04-08 12:24 ` [PATCH v6 10/21] dt-bindings: display: renesas,rzg2l-du: " Laurent Pinchart
2026-04-08 14:02 ` Tommaso Merciai
2026-04-08 14:16 ` Laurent Pinchart
2026-04-08 14:44 ` Tommaso Merciai
2026-04-08 15:00 ` Laurent Pinchart
2026-04-09 11:15 ` Tommaso Merciai
2026-04-09 13:24 ` Laurent Pinchart
2026-04-10 13:21 ` Tommaso Merciai
2026-04-12 2:45 ` Claude review: dt-bindings: display: renesas, rzg2l-du: " Claude Code Review Bot
2026-04-08 10:36 ` [PATCH v6 11/21] dt-bindings: display: bridge: renesas, dsi: " Tommaso Merciai
2026-04-12 2:45 ` Claude review: " Claude Code Review Bot
2026-04-08 10:36 ` [PATCH v6 12/21] drm: renesas: rz-du: mipi_dsi: Add out_port to OF data Tommaso Merciai
2026-04-08 12:30 ` Laurent Pinchart
2026-04-12 2:45 ` Claude review: " Claude Code Review Bot
2026-04-08 10:36 ` [PATCH v6 13/21] drm: renesas: rz-du: mipi_dsi: Add RZ_MIPI_DSI_FEATURE_GPO0R feature Tommaso Merciai
2026-04-08 12:31 ` Laurent Pinchart
2026-04-08 14:12 ` Tommaso Merciai
2026-04-08 14:17 ` Laurent Pinchart
2026-04-08 14:58 ` Tommaso Merciai
2026-04-08 15:08 ` Laurent Pinchart
2026-04-09 11:14 ` Tommaso Merciai
2026-04-09 13:22 ` Laurent Pinchart
2026-04-12 2:45 ` Claude review: " Claude Code Review Bot
2026-04-08 10:36 ` [PATCH v6 14/21] drm: renesas: rz-du: mipi_dsi: Add support for RZ/G3E Tommaso Merciai
2026-04-12 2:45 ` Claude review: " Claude Code Review Bot
2026-04-08 10:37 ` [PATCH v6 15/21] drm: renesas: rz-du: Add RZ/G3E support Tommaso Merciai
2026-04-12 2:45 ` Claude review: " Claude Code Review Bot
2026-04-08 10:37 ` [PATCH v6 16/21] media: dt-bindings: media: renesas, vsp1: Document RZ/G3E Tommaso Merciai
2026-04-08 10:52 ` [PATCH v6 16/21] media: dt-bindings: media: renesas,vsp1: " Laurent Pinchart
2026-04-12 2:45 ` Claude review: media: dt-bindings: media: renesas, vsp1: " Claude Code Review Bot
2026-04-08 10:37 ` [PATCH v6 17/21] media: dt-bindings: media: renesas, fcp: Document RZ/G3E SoC Tommaso Merciai
2026-04-08 10:53 ` [PATCH v6 17/21] media: dt-bindings: media: renesas,fcp: " Laurent Pinchart
2026-04-12 2:45 ` Claude review: media: dt-bindings: media: renesas, fcp: " Claude Code Review Bot
2026-04-08 10:37 ` [PATCH v6 18/21] arm64: dts: renesas: r9a09g047: Add fcpvd{0, 1} nodes Tommaso Merciai
2026-04-08 11:32 ` [PATCH v6 18/21] arm64: dts: renesas: r9a09g047: Add fcpvd{0,1} nodes Laurent Pinchart
2026-04-12 2:45 ` Claude review: arm64: dts: renesas: r9a09g047: Add fcpvd{0, 1} nodes Claude Code Review Bot
2026-04-08 10:37 ` [PATCH v6 19/21] arm64: dts: renesas: r9a09g047: Add vspd{0,1} nodes Tommaso Merciai
2026-04-08 11:33 ` Laurent Pinchart
2026-04-12 2:46 ` Claude review: " Claude Code Review Bot
2026-04-08 10:37 ` [PATCH v6 20/21] arm64: dts: renesas: r9a09g047: Add DU{0, 1} and DSI nodes Tommaso Merciai
2026-04-08 12:11 ` Laurent Pinchart
2026-04-12 2:46 ` Claude review: " Claude Code Review Bot
2026-04-08 10:37 ` [PATCH v6 21/21] arm64: dts: renesas: r9a09g047e57-smarc: Enable DU0 and DSI support Tommaso Merciai
2026-04-08 13:01 ` Geert Uytterhoeven
2026-04-12 2:46 ` Claude review: " Claude Code Review Bot
2026-04-12 2:45 ` Claude review: Add support for DU and DSI on the Renesas RZ/G3E SoC Claude Code Review Bot
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=review-patch1-24d3853ca2522df21e6a071a23e23ba4ca4b7276.1775636898.git.tommaso.merciai.xr@bp.renesas.com \
--to=claude-review@example.com \
--cc=dri-devel-reviews@example.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox