Hi Chris,
On Thu, 13 Nov 2025 00:39:59 +0000
Chris Brandt <[email protected]> wrote:
> 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:
Good idea.
>
> -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???
No, int is good for that enum.
>
>
> > > +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.
Agreed.
>
>
> > > + /* 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.
I think -EINVAL is good, as there is already, a few lines above, a
check for:
if (!rate)
return -EINVAL;
So it will be consistent.
>
>
> > > - 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.
Ok.
Hugo.
