Hi Hugo
On Wed, Nov 19, 2025 12:11 AM, Hugo Villeneuve wrote:
> > + } else if (dsi_div_target == PLL5_TARGET_DPI) {
> > + /* Fixed settings for DPI */
> > + priv->mux_dsi_div_params.clksrc = 0;
> > + priv->mux_dsi_div_params.dsi_div_a = 3; /* Divided by 8 */
> > + priv->mux_dsi_div_params.dsi_div_b = 0; /* Divided by 1 */
> > + dsi_div_ab_desired = 8; /* (1 << a) * (b + 1) */
>
> As suggested in V4, use the new inline function:
>
> dsi_div_ab_desired = rzg2l_cpg_div_ab(priv->mux_dsi_div_params.dsi_div_a,
> priv->mux_dsi_div_params.dsi_div_b);
>
> You can then remove the comments re-explaining how it is computed.
>
> Then if you change a and/or b you have the new value automatically computed
> and not hard coded (less error-prone).
OK.
> > + params->pl5_fracin = div_u64(((u64)
> > + (foutvco_rate *
> > params->pl5_refdiv) %
> > + (EXTAL_FREQ_IN_MEGA_HZ *
> > MEGA)) << 24,
> > + EXTAL_FREQ_IN_MEGA_HZ *
> > MEGA);
>
> Something is not right here, because the pl5_fracin computation is exactly
> like in V2, where Geert in his comments suggested you move u64 outside of the
> modulo operation.
>
> It probably is because you had this code block duplicated in V3, but you
> removed the wrong block for V4:
Good catch! I totally delete the wrong block.
It's fixed....again.
> > - priv->mux_dsi_div_params.clksrc = 1; /* Use clk src 1 for DSI */
> > - priv->mux_dsi_div_params.dsi_div_a = 1; /* Divided by 2 */
> > - priv->mux_dsi_div_params.dsi_div_b = 2; /* Divided by 3 */
>
> Removing those lines make the "8" below hardcoded and hard to understand.
>
> But I am confused as a=1 and b=2 should give a div_ab value of 6 and not 8?
According to the hardware manual, valid dividers for that mode are 1/2, 1/4, 1/8
Since I see customers generally use smaller parallel LCDs, I went with the
larger divider (1/8).
> > + rzg2l_cpg_dsi_div_set_divider(8, PLL5_TARGET_DPI);
>
> Maybe use an intermediate variable like this:
>
> u8 div_ab;
> ...
> div_ab = rzg2l_cpg_div_ab(1, 2);
> rzg2l_cpg_dsi_div_set_divider(div_ab, PLL5_TARGET_DPI);
>
> Keeping the lines with the comments (setting a and b) would be even more
> clear, and simply reusing their values in rzg2l_cpg_div_ab...
The hardware manual says in DPI mode, "set divider to 1/2, 1/4, or 1/8".
So, then if you look at the code and it has:
rzg2l_cpg_dsi_div_set_divider(8, PLL5_TARGET_DPI);
That makes sense to me. It's doing exactly what the hardware manual told you to
do at a high level.
Other places later in the code it will figure out how to turn that into 'a' and
'b', but there is no need to worry about that yet.
That was my thinking.
Chris
