Re: [PATCH v5 01/20] clk: renesas: rzv2h: Add PLLDSI clk mux support

[Geert Uytterhoeven]

Hi Tommaso,

On Fri, 13 Feb 2026 at 17:28, Tommaso Merciai
<tommaso.merciai.xr@xxxxxxxxxxxxxx> wrote:
> Add PLLDSI clk mux support to select PLLDSI clock from different clock
> sources.
>
> Introduce the DEF_PLLDSI_SMUX() macro to define these muxes and register
> them in the clock driver.
>
> Extend the determine_rate callback to calculate and propagate PLL
> parameters via rzv2h_get_pll_dtable_pars() when LVDS output is selected,
> using a new helper function rzv2h_cpg_plldsi_smux_lvds_determine_rate().
>
> The CLK_SMUX2_DSI{0,1}_CLK clock multiplexers select between two paths
> with different duty cycles:
>
> - CDIV7_DSIx_CLK (LVDS path, parent index 0): asymmetric H/L=4/3 duty (4/7)
> - CSDIV_DSIx (DSI/RGB path, parent index 1): symmetric 50% duty (1/2)
>
> Implement rzv2h_cpg_plldsi_smux_{get,set}_duty_cycle clock operations to
> allow the DRM driver to query and configure the appropriate clock path
> based on the required output duty cycle.
>
> Signed-off-by: Tommaso Merciai <tommaso.merciai.xr@xxxxxxxxxxxxxx>

Thanks for your patch!

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

> +static struct clk * __init
> +rzv2h_cpg_plldsi_smux_clk_register(const struct cpg_core_clk *core,
> +                                  struct rzv2h_cpg_priv *priv)
> +{
> +       struct rzv2h_plldsi_mux_clk *clk_hw_data;
> +       struct clk_init_data init;
> +       struct clk_hw *clk_hw;
> +       struct smuxed smux;
> +       u8 width, mask;
> +       int ret;
> +
> +       smux = core->cfg.smux;
> +       mask = smux.width;
> +       width = fls(mask) - ffs(mask) + 1;
> +
> +       if (width + smux.width > 16) {
> +               dev_err(priv->dev, "mux value exceeds LOWORD field\n");
> +               return ERR_PTR(-EINVAL);
> +       }

I am totally confused by this: smux.width is not a mask, but the size
of a register bitifield.
Perhaps:

    if (smux.shift + smux.width > 16) { ... }

?

> +
> +       clk_hw_data = devm_kzalloc(priv->dev, sizeof(*clk_hw_data), GFP_KERNEL);
> +       if (!clk_hw_data)
> +               return ERR_PTR(-ENOMEM);
> +
> +       clk_hw_data->priv = priv;
> +
> +       init.name = core->name;
> +       init.ops = &rzv2h_cpg_plldsi_smux_ops;
> +       init.flags = core->flag;
> +       init.parent_names = core->parent_names;
> +       init.num_parents = core->num_parents;
> +
> +       clk_hw_data->mux.reg = priv->base + smux.offset;
> +
> +       clk_hw_data->mux.shift = smux.shift;
> +       clk_hw_data->mux.mask = smux.width;

Again, smux.width is not a mask.
Perhaps GENMASK_U16(smux.shift - 1, 0)?

> +       clk_hw_data->mux.flags = core->mux_flags;
> +       clk_hw_data->mux.lock = &priv->rmw_lock;
> +
> +       clk_hw = &clk_hw_data->mux.hw;
> +       clk_hw->init = &init;
> +
> +       ret = devm_clk_hw_register(priv->dev, clk_hw);
> +       if (ret)
> +               return ERR_PTR(ret);
> +
> +       return clk_hw->clk;
> +}

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