Re: [PATCH 4/6] clk: renesas: rzg2l: Add support for RZ/G3L DSI mux

[Geert Uytterhoeven]

Hi Biju,

On Tue, 28 Jul 2026 at 14:35, Biju Das <biju.das.jz@xxxxxxxxxxxxxx> wrote:
> > On Fri, 19 Jun 2026 at 18:40, Biju <biju.das.au@xxxxxxxxx> wrote:
> > > From: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> > >
> > > Add support for RZ/G3L DSI mux that supports 2 duty cycles.
> > >
> > > Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>

> > > --- a/drivers/clk/renesas/rzg2l-cpg.c
> > > +++ b/drivers/clk/renesas/rzg2l-cpg.c
> > > @@ -120,6 +120,11 @@
> > >  #define RZG3L_PLL7_FSTD_DIV_MR_MIN     (8 * MEGA)
> > >  #define RZG3L_PLL7_FSTD_DIV_MR_MAX     (16 * MEGA)
> > >
> > > +#define CPG_PLLDSI_SMUX_LVDS_DUTY_NUM          4
> > > +#define CPG_PLLDSI_SMUX_LVDS_DUTY_DEN          7
> > > +#define CPG_PLLDSI_SMUX_DSI_RGB_DUTY_NUM       1
> > > +#define CPG_PLLDSI_SMUX_DSI_RGB_DUTY_DEN       2
> >
> > I am wondering if these macros are worthwhile, as IMO they make it harder to understand the code.
>
> OK, you mean find a way to determine the parent based on some variable.

No, I mean to use the actual duty numbers in the code:

    - duty->num = CPG_PLLDSI_SMUX_LVDS_DUTY_NUM;
    - duty->den = CPG_PLLDSI_SMUX_LVDS_DUTY_DEN;
    + duty->num = 4;
    + duty->den = 7;

But probably this can go away anyway...

> > > +
> > >  /**
> > >   * struct clk_hw_data - clock hardware data
> > >   * @hw: clock hw
> >
> > > +static int rzg3l_cpg_dsi_smux_get_duty_cycle(struct clk_hw *hw,
> > > +                                            struct clk_duty *duty) {
> > > +       u8 parent = clk_mux_ops.get_parent(hw);
> > > +
> > > +       /*
> > > +        * CDIV7_DSIx_CLK - LVDS path (div7) - duty 4/7.
> > > +        * CSDIV_DSIx - DSI/RGB path (csdiv) - duty 1/2.
> > > +        */
> > > +       if (parent == 0) {
> > > +               duty->num = CPG_PLLDSI_SMUX_LVDS_DUTY_NUM;
> > > +               duty->den = CPG_PLLDSI_SMUX_LVDS_DUTY_DEN;
> >
> > The 4/7 duty cycle is due to the division by 7 between M2 and M2_DIV7.
> > Perhaps it should be set by that divider clock instead?
>
> You mean define a custom clock for M2_DIV7 and call setduty for grand parent
> from LVDS bridge driver to distinguish LVDS from others ??

I mean to define a custom clock for M2_DIV7 which sets a duty cycle
of 4/7 (i.e. an odd divider clock which sets .num = (div + 1) / 2 and
.den = div).

Then you can just drop rzg3l_cpg_dsi_smux_get_duty_cycle(), as the
clock core will happily call into the parent.

In hindsight, I should have given the same comment when
rzv2h_cpg_plldsi_smux_get_duty_cycle() was introduced.

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