Hi Hugo,
Thank you again for the review.
:)
On Thu, Oct 23, 2025 2:32 PM, Hugo Villeneuve wrote:
> > + if ((b + 1) << a == dsi_div_ab) {
> > + priv->mux_dsi_div_params.dsi_div_a = a;
> > + priv->mux_dsi_div_params.dsi_div_b = b;
> > +
> > + goto calc_pll_clk;
> > + }
> > + }
> > + }
>
> If we arrive at this point, it seems that these values:
> priv->mux_dsi_div_params.dsi_div_a
> priv->mux_dsi_div_params.dsi_div_b
>
> were not initialised by the previous loop. Is this expected? If yes, maybe a
> comment would help?
Actually, I think I want to print out a warning.
dev_warn(priv->dev, "Failed to calculate DIV_DSI A,B\n");
I could not find a monitor that would cause this, but it's possible.
So to be kind to the users, I will print out a message so if something does not
work, at least
they will have an idea what is wrong.
> > +calc_pll_clk:
> > + /*
> > + * Below conditions must be set for PLL5 parameters:
> > + * - REFDIV must be between 1 and 2.
>
> I am assuming this means PLL5_POSTDIV_MIN and PLL5_POSTDIV_MAX? If these
> macros change, then that mean you would also need to change your comment, not
> very practical and error-prone. I would suggest to remove this comment
> altogether.
"REFDIV" is the term used in the hardware manual to reference the signal.
> > + * - POSTDIV1/2 must be between 1 and 7.
> > + * - INTIN must be between 20 and 320.
> > + * - FOUTVCO must be between 800MHz and 3000MHz.
>
> Same here.
While I doubt the hardware design will change, your point is valid that I'm not
giving the reader
any more info than they could get from the code.
Kind of like the classic source code comment:
value = 5; /* Set value to 5 */
I'll remove it.
> + /* Set defaults since valid clock was not found */
> + params->pl5_intin = PLL5_INTIN_DEF;
> + params->pl5_fracin = PLL5_FRACIN_DEF;
> + params->pl5_refdiv = PLL5_REFDIV_DEF;
> + params->pl5_postdiv1 = PLL5_POSTDIV_DEF;
> + params->pl5_postdiv2 = PLL5_POSTDIV_DEF;
I'm going to print out a warning here too.
dev_warn(priv->dev, "Failed to calculate exact PLL5 settings\n");
> > - foutpostdiv_rate = DIV_ROUND_CLOSEST_ULL(foutvco_rate,
> > - params->pl5_postdiv1 *
> > params->pl5_postdiv2);
> > +
> > + /* If foutvco is above 1.5GHz, change parent and recalculate */
>
> Similar suggestion for hardcoded values in comments, maybe replace "above
> 1.5GHz" with "too high"...
This one I'm OK with because that's the design specification of the hardware IP
that's used in all the devices.
If for some reason they re-design the hardware in future devices, something
going to have to change and
the driver will need to be updated. So we'll deal with it at that point.
Cheers
Chris
