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

Hi Geert,

Thanks for the feedback.

> -----Original Message-----
> From: Geert Uytterhoeven <[email protected]>
> Sent: 10 August 2026 13:57
> Subject: Re: [PATCH 4/6] clk: renesas: rzg2l: Add support for RZ/G3L DSI mux
> 
> Hi Biju,
> 
> On Tue, 28 Jul 2026 at 14:35, Biju Das <[email protected]> wrote:
> > > On Fri, 19 Jun 2026 at 18:40, Biju <[email protected]> wrote:
> > > > From: Biju Das <[email protected]>
> > > >
> > > > Add support for RZ/G3L DSI mux that supports 2 duty cycles.
> > > >
> > > > Signed-off-by: Biju Das <[email protected]>
> 
> > > > --- 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...

OK.

> 
> > > > +
> > > >  /**
> > > >   * 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.

OK, will add FF clock with .get_duty_cycle, that will set 4:7 duty cycle during clk registration.


Cheers,
Biju