Hi Hugo,
Thank you for your review.
On Tue, Nov 11, 2025 11:41 PM, Hugo Villeneuve wrote:
> > +/* Required division ratio for MIPI D-PHY clock depending on number of
> > lanes and bpp. */
> >+static unsigned int dsi_div_ab_desired;
>
> giving a maximum of 24 * 2 / 1 = 48
>
> So change type to u8?
Done.
I also changed the API function:
-void rzg2l_cpg_dsi_div_set_divider(unsigned int divider, int target);
+void rzg2l_cpg_dsi_div_set_divider(u8 divider, int target);
But, I kept "target" as int because it is an enum.
Any comment on changing that to "u8" as well???
> > +rzg2l_cpg_get_foutpostdiv_rate(struct rzg2l_cpg_priv *priv,
> > + struct rzg2l_pll5_param *params,
> > unsigned long rate)
> > {
> > unsigned long foutpostdiv_rate, foutvco_rate;
> > + unsigned long hsclk;
> > + unsigned int a, b, odd;
> > + unsigned int dsi_div_ab_calc;
> > +
>
> Based on my tests, it seems we can arrive at this point with a
> non-initialized dsi_div_ab_desired (0). Since valid
> values are from 1 to 128, add a check for this before using it.
I think at first, we need to understand how that is happening and fix that code
path.
If this function is being called before the dsi_div_ab_desired is set to a
valid value, then the code might make bad calculations and decisions.
> > + /* Determine the correct clock source based on even/odd of the
> > divider */
> > + odd = dsi_div_ab_desired & 1;
> > + if (odd) {
> > + /* divider is odd */
>
> You can drop this comment, as your "odd" variable is self-explanatory.
>
> > + priv->mux_dsi_div_params.clksrc = 0; /* FOUTPOSTDIV
> > */
> > + dsi_div_ab_calc = dsi_div_ab_desired;
> > + } else {
> > + /* divider is even */
>
> ditto.
Done.
> > +
> > + /* FOUTPOSTDIV: DIV_DSI_A must always be 1/1 */
> > + if (odd && a != 0)
> > + continue;
>
> Use break instead of continue?
Done.
Changed to break;
> > +
> > + for (b = 0; b < 16; b++) {
> > + /* FOUTPOSTDIV: DIV_DSI_B must always be odd
> > divider 1/(b+1) */
> > + if (odd && b & 1)
> > + continue;
> > +
> > + if ((b + 1) << a == dsi_div_ab_calc) {
>
>
> It took me a while to decipher this :)
>
> Use an inline function to compute div_ab to improve readability, and you can
> reuse this function elsewhere instead of hardcoding the div_ab value (to for
> example):
>
> static inline u8 rzg2l_cpg_div_ab(u8 a, u8 b) {
> return (b + 1) << a;
> }
>
> and then:
>
> ...
> if (rzg2l_cpg_div_ab(a, b) == dsi_div_ab_calc) {
> ...
>
OK. I can live with that.
During the reviews, the code kept getting more and more optimized and trimmed
down, which then became harder to follow.
> > + 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) */
> > + }
>
> Here this block could be combined as an if/else-if:
>
> if (dsi_div_target == PLL5_TARGET_DPI) {
> ...
> } else if (dsi_div_target == PLL5_TARGET_DSI) {
> ...
Done.
> > + if (params->pl5_intin < PLL5_INTIN_MIN ||
> > + params->pl5_intin > PLL5_INTIN_MAX)
> > + continue;
>
> Insert line for readability
Done.
> > - foutpostdiv_rate = DIV_ROUND_CLOSEST_ULL(foutvco_rate,
> > - params->pl5_postdiv1 *
> > params->pl5_postdiv2);
> > +
> > + foutpostdiv_rate = DIV_ROUND_CLOSEST(foutvco_rate,
> > + params->pl5_postdiv1 *
> > params->pl5_postdiv2);
> >
> > return foutpostdiv_rate;
>
> You can drop foutpostdiv_rate intermediate variable and return directly, all
> on one line:
>
> return DIV_ROUND_CLOSEST(foutvco_rate, params->pl5_postdiv1 *
> params->pl5_postdiv2);
Done.
> > vclk_rate = rzg2l_cpg_get_vclk_rate(hw, rate);
> > sipll5->foutpostdiv_rate =
> > - rzg2l_cpg_get_foutpostdiv_rate(¶ms, vclk_rate);
> > + rzg2l_cpg_get_foutpostdiv_rate(priv, ¶ms, vclk_rate);
>
> Before this patch, rzg2l_cpg_get_foutpostdiv_rate() seemed to always return a
> valid rate. Therefore, no validation was done of the computed rate.
>
> Now with your patch it may return "0" if the rate is invalid. Therefore you
> need to check for this here and return a corresponding error code.
I saw for some clock drivers, if they could not get the correct rate, they just
return '0' as the rate.
But, I do see in some other drivers, they return -EINVAL, so I can do that as
well.
> > - 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 */
> > + /* Default 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) */
>
> Use inline function rzg2l_cpg_div_ab() previously suggested.
I plan on getting rid of this code block anyway.
It's not needed.
Cheers
Chris
