Re: [PATCH v7 2/6] clk: renesas: rzv2h-cpg: Add support for DSI clocks

[Geert Uytterhoeven]

Hi Prabhakar,

On Mon, 28 Jul 2025 at 22:14, Prabhakar <prabhakar.csengg@xxxxxxxxx> wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
>
> Add support for PLLDSI and PLLDSI divider clocks.
>
> Introduce the `renesas-rzv2h-cpg-pll.h` header to centralize and share
> PLLDSI related data structures, limits, and algorithms between the
> RZ/V2H(P) CPG and DSI drivers.
>
> The DSI PLL is functionally similar to the CPG's PLLDSI, but has slightly
> different parameter limits and omits the programmable divider present in
> CPG. To ensure precise frequency calculations, especially for milliHz-level
> accuracy needed by the DSI driver, the shared algorithm allows both drivers
> to compute PLL parameters consistently using the same logic and input
> clock.
>
> Co-developed-by: Fabrizio Castro <fabrizio.castro.jz@xxxxxxxxxxx>
> Signed-off-by: Fabrizio Castro <fabrizio.castro.jz@xxxxxxxxxxx>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
> ---
> v6->v7:
> - Made struct rzv2h_pll_limits more modular also added Ffout limits
> - Made the alogirithm modular and also added apis based on the
>   needs for lvds and dpi

Thanks for the update!

> --- a/drivers/clk/renesas/rzv2h-cpg.c
> +++ b/drivers/clk/renesas/rzv2h-cpg.c

> +static struct rzv2h_pll_dsi_info *rzv2h_get_pll_dsi_info(struct clk_hw *pll_dsi,
> +                                                        struct rzv2h_cpg_priv *priv)
> +{
> +       struct pll_clk *pll_clk = to_pll(pll_dsi);
> +
> +       return &priv->pll_dsi_info[pll_clk->pll.instance];
> +}

This (very simple helper) is used twice, while there are two (almost
three) other locations where it is open-coded.  Perhaps just open-code
it everywhere?

> @@ -246,7 +526,8 @@ static const struct clk_ops rzv2h_cpg_pll_ops = {
>  static struct clk * __init
>  rzv2h_cpg_pll_clk_register(const struct cpg_core_clk *core,
>                            struct rzv2h_cpg_priv *priv,
> -                          const struct clk_ops *ops)
> +                          const struct clk_ops *ops,
> +                          bool is_plldsi)

No need for this parameter...

>  {
>         struct device *dev = priv->dev;
>         struct clk_init_data init;
> @@ -263,6 +544,10 @@ rzv2h_cpg_pll_clk_register(const struct cpg_core_clk *core,
>         if (!pll_clk)
>                 return ERR_PTR(-ENOMEM);
>
> +       if (is_plldsi)

... as you can just test "core->type == CLK_TYPE_PLLDSI" here.

> +               priv->pll_dsi_info[core->cfg.pll.instance].pll_dsi_limits =
> +                       core->cfg.pll.limits;
> +
>         parent_name = __clk_get_name(parent);
>         init.name = core->name;
>         init.ops = ops;

The rest LGTM, so
Reviewed-by: Geert Uytterhoeven <geert+renesas@xxxxxxxxx>

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds