Re: [PATCH] phy: freescale: fsl-samsung-hdmi: fix build error in fsl_samsung_hdmi_phy_configure_pll_lock_det

[Dominique Martinet <dominique.martinet@atmark-techno.com>]

Adam Ford wrote on Mon, Dec 30, 2024 at 08:11:16PM -0600:
> > index 5eac70a1e858..3e4d1a5160ea 100644
> > --- a/drivers/phy/freescale/phy-fsl-samsung-hdmi.c
> > +++ b/drivers/phy/freescale/phy-fsl-samsung-hdmi.c
> > @@ -341,7 +341,7 @@ fsl_samsung_hdmi_phy_configure_pll_lock_det(struct fsl_samsung_hdmi_phy *phy,
> >                         break;
> >         }
> >
> > -       writeb(FIELD_PREP(REG12_CK_DIV_MASK, div), phy->regs + PHY_REG(12));
> > +       writeb(FIELD_PREP(REG12_CK_DIV_MASK, div == 4 ? div - 1 : div), phy->regs + PHY_REG(12));
> 
> The for-loop above this line states:   for (div = 0; div < 4; div++)
> How could this ever reach 4?  If it did reach 4, the calculation for
> int_pllclk would need to be recalculated since int_pllclk = pclk / (1
> << div);

But... for (div = 0; div < 4; div++) does reach 4, if the break
condition didn't match, which is something the compiler cannot ensure
here.

The old code would just fall out of any of the switch cases and fallback
to div = 1 if pixclk > 297000000, which is likely incorrect, so in that
sense just padding this through `& 3` and pretending it will never
happen is probably acceptable, but this ought to have a better comment
than what Pei just sent.
(this was correct with the old lookup tables, I'm not sure if we can't
compute any higher frequencies now?)

My preference would be to actually check and handle this somehow since I
don't think this part of the code is that performance critical that we
can't afford an extra instruction, e.g. something like that:
if (WARN_ONCE(div == 4, "pixclk %u out of range", pclk))
	(appropriate fallback or return?)

but I haven't spent the time to actually check so will leave that up to
you.

Thank you both,
-- 
Dominique