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

[Tommaso Merciai <tommaso.merciai.xr@bp.renesas.com>]

Hi Geert,
Thanks for your review!

On Fri, Feb 27, 2026 at 11:47:58AM +0100, Geert Uytterhoeven wrote:
> Hi Tommaso,
> 
> On Fri, 13 Feb 2026 at 17:28, Tommaso Merciai
> <tommaso.merciai.xr@bp.renesas.com> 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@bp.renesas.com>
> 
> 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) { ... }
> 
> ?

Ouch, you are right.
Should be:

	if (smux.shift + smux.width > 16) {
		dev_err(priv->dev, "mux value exceeds LOWORD field\n");
		return ERR_PTR(-EINVAL);
	}

Will fix this in v6.

> 
> > +
> > +       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)?

Or maybe we can use:

	clk_hw_data->mux.mask = clk_div_mask(smux.width);
?

I'll fix this in v6.
Thanks.

Kind Regards,
Tommaso

> 
> > +       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@linux-m68k.org
> 
> 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