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

[Biju Das]

Hi Geert,

Thanks for the feedback.

> -----Original Message-----
> From: Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>
> Sent: 10 July 2026 16:49
> Subject: Re: [PATCH 4/6] clk: renesas: rzg2l: Add support for RZ/G3L DSI mux
> 
> Hi Biju,
> 
> 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>
> 
> More comments...
> 
> > --- 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.

For eg: DSI sets "dsi_div_ab_desired" this can be used to differentiate
between DSI and LVDS, but DPI still use the same parent. So, we cannot
use this??

static int rzg3l_cpg_dsi_smux_determine_rate(struct clk_hw *hw,
                                             struct clk_rate_request *req)
 {
+       struct clk_hw *parent = NULL;
+
+       if (dsi_div_ab_desired) {
+               parent = clk_hw_get_parent_by_index(hw, 1);
+               req->best_parent_hw = parent;
+       }


}
> 
> > +
> >  /**
> >   * 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 ??

+static int rzg3l_lvds_attach(struct drm_bridge *bridge,
+			     struct drm_encoder *encoder,
+			     enum drm_bridge_attach_flags flags)
+{
+	struct rzg3l_lvds *lvds = bridge_to_rzg3l_lvds(bridge);
+	struct clk *clk_parent, *clk_grand_parent;
+
+	clk_parent = clk_get_parent(lvds->dotclk);
+     clk_grand_parent = clk_get_parent(clk_parent);
+	clk_set_duty_cycle(clk_grand_parent, 4, 7);
+
+	return drm_bridge_attach(encoder, lvds->bridge.next_bridge, bridge, flags);
+}

Cheers,
Biju