Hi Chris,
On Tue, 28 Oct 2025 16:17:51 +0000
Chris Brandt <[email protected]> wrote:
> 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?
So are the uninitialised values valid at all?
>
> 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.
So is it ok to continue in this case?
If yes, then maybe your message could specify: "Failed to calculate
DIV_DSI A,B, using defaults\n" ?
>
> > > +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.
Ok :)
>
> > + /* 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");
Similar to my comment above, would it be a good idea to add something
like "Failed to calculate exact PLL5 settings, using defaults\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
>
--
Hugo Villeneuve
