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
> <[email protected]> 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 <[email protected]>
>
> 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 --
> [email protected]
>
> 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
