RE: [PATCH v2 1/9] clk: renesas: rzg2l: Add DSI divider clock support for RZ/G3L

[Biju Das]

Hi Geert,

Thanks for the feedback.

> -----Original Message-----
> From: Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>
> Sent: 14 August 2026 13:32
> Subject: Re: [PATCH v2 1/9] clk: renesas: rzg2l: Add DSI divider clock support for RZ/G3L
> 
> Hi Biju,
> 
> On Tue, 11 Aug 2026 at 20:27, Biju <biju.das.au@xxxxxxxxx> wrote:
> > From: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> >
> > Add a new DSI divider clock type (CLK_TYPE_G3L_PLLDSI_DIV) for the
> > RZ/G3L SoC, which requires a different divider implementation than the
> > existing RZ/G2L DSI divider clock.
> >
> > The RZ/G3L DSI divider uses two cascaded dividers, DIV_DSI_A and
> > DIV_DSI_B, where the effective divider is:
> >
> >   rate = parent_rate / ((1 << div_a) * (div_b + 1))
> >
> > DIV_DSI_A is a power-of-two divider with values in the range [0, 5],
> > and DIV_DSI_B is a linear divider with values in the range [1, 16].
> >
> > Introduce the g3l_dsi_div_hw_data structure, rzg3l_cpg_dsi_div_ops,
> > and
> > rzg3l_cpg_dsi_div_clk_register() to implement the new clock type, and
> > add the DEF_G3L_PLLDSI_DIV() macro for use in clock table definitions.
> >
> > Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> > ---
> > v1->v2:
> >  * Introduced the macro DIV_DSI_{A,B}_SET and used inside
> >    rzg3l_cpg_dsi_div_recalc_rate() and rzg3l_cpg_dsi_div_set_rate()
> >  * Simplified the divider to "(div_b + 1) << div_a" in
> >    rzg3l_cpg_dsi_div_recalc_rate().
> >  * Replaced the data type u32->unsigned int for the variable divider in
> >    rzg3l_cpg_dsi_div_determine_rate()
> >  * Moved the declarations of the loop counters inside the
> > for()-statements
> >  * Updated the comment sections in rzg3l_cpg_dsi_div_determine_rate().
> >  * Updated loop logic in rzg3l_cpg_dsi_div_determine_rate().
> 
> Thanks for the update!
> 
> > --- a/drivers/clk/renesas/rzg2l-cpg.c
> > +++ b/drivers/clk/renesas/rzg2l-cpg.c
> 
> > +static int rzg3l_cpg_dsi_div_determine_rate(struct clk_hw *hw,
> > +                                           struct clk_rate_request
> > +*req) {
> > +       struct g3l_dsi_div_hw_data *dsi_div = to_g3l_dsi_div_hw_data(hw);
> > +       struct rzg2l_cpg_priv *priv = dsi_div->priv;
> > +       unsigned int divider = dsi_div_ab_desired;
> > +       bool divider_found = false;
> > +
> > +       if (dsi_div_target) {
> > +               /* Calculate DIV_DSI_A and DIV_DSI_B */
> > +               for (unsigned int div_a = 0; div_a <= 5 && !divider_found; div_a++) {
> > +                       unsigned int div_a_tmp = 5 - div_a;
> 
> So div_a_tmp is the actual value of div_a... That is confusing, so I'll do s/div_a/i/ and
> s/div_a_tmp/div_a/ while applying.

OK.

> 
> > +
> > +                       for (unsigned int div_b = 0; div_b < 16; div_b++) {
> > +                               divider = (div_b + 1) << div_a_tmp;
> > +                               if (divider == dsi_div_ab_desired) {
> > +                                       dsi_div->div_a = div_a_tmp;
> > +                                       dsi_div->div_b = div_b;
> > +                                       divider_found = true;
> > +                                       break;
> > +                               }
> > +                       }
> > +               }
> > +       } else {
> > +               dsi_div->div_b = 0;
> > +               /* Calculate DIV_DSI_A */
> > +               for (unsigned int div_a = 0; div_a <= 5 && !divider_found; div_a++) {
> > +                       unsigned int div_a_tmp = 5 - div_a;
> > +
> > +                       divider = 1 << div_a_tmp;
> > +                       if (divider == dsi_div_ab_desired) {
> > +                               dsi_div->div_a = div_a_tmp;
> > +                               divider_found = true;
> > +                               break;
> > +                       }
> > +               }
> 
> This is basically the same code as in the other branch, just looping over a single value of div_b:
> 
>     unsigned int div_b_max = dsi_div_target ? 15 : 0;

Yes, you are correct.

> 
>     for (unsigned int div_b = 0; div_b <= div_b_max; div_b++)
> 
> > +       }
> 
> Reviewed-by: Geert Uytterhoeven <geert+renesas@xxxxxxxxx> i.e. will queue in renesas-clk for v7.4, with
> the above fixed.

Thanks for taking care of this.

Cheers,
Biju