public inbox for drm-ai-reviews@public-inbox.freedesktop.org
 help / color / mirror / Atom feed
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

  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