Re: [PATCH] phy: freescale: fsl-samsung-hdmi: return closest rate instead LUT

From: Dominique Martinet <dominique.martinet@atmark-techno.com>
To: "Uwe Kleine-König" <u.kleine-koenig@baylibre.com>
Cc: Vinod Koul <vkoul@kernel.org>,
	Kishon Vijay Abraham I <kishon@kernel.org>,
	Adam Ford <aford173@gmail.com>,
	Frieder Schrempf <frieder.schrempf@kontron.de>,
	Marco Felsch <m.felsch@pengutronix.de>,
	Lucas Stach <l.stach@pengutronix.de>,
	linux-phy@lists.infradead.org, linux-kernel@vger.kernel.org,
	Makoto Sato <makoto.sato@atmark-techno.com>
Subject: Re: [PATCH] phy: freescale: fsl-samsung-hdmi: return closest rate instead LUT
Date: Mon, 10 Mar 2025 18:48:50 +0900	[thread overview]
Message-ID: <Z861gsaGY6bGSisf@atmark-techno.com> (raw)
In-Reply-To: <v57uy3gddzcoeg3refyv7h6j3ypx23mobctybt27xzdyqy6bgb@atzdlqlytz3c>

Uwe Kleine-König wrote on Mon, Mar 10, 2025 at 10:14:23AM +0100:
> > for 83.5mHz, the integer divider generates 83.2mHz (-0.36%), but the
> > next LUT value (82.5mHz) is 1.2% off which incorrectly rejects modes
> > requiring this frequency.
> 
> Is the unit here MHz or mHz? I suspect the former?

Err, yes MHz; I was still half asleep when I added that example to the
commit message..

> Without having looked in detail, I think it would be nice to reduce code
> duplication between phy_clk_round_rate() and phy_clk_set_rate(). The
> former has
> 
> 	if (rate > 297000000 || rate < 22250000)
> 		return -EINVAL;
> 
> which seems to be lacking from the latter so I suspect there are more
> differences between the two functions than fixed here?

For this particular rate check, I believe that if phy_clk_round_rate()
rejected the frequency then phy_clk_set_rate() cannot be called at all
with the said value (at least on this particular setup going through the
imx8mp-hdmi-tx bridge validating frequencies first before allowing
modes), not that it'd hurt to recheck.

In general though I agree these are doing pretty much the same thing,
with clk_round_rate() throwing away the pms values and there's more
duplication than ideal...
Unfortunately the code that computes registers for the integer divider
does it in a global variable so just computing everything in
round_rate() would forget what last setting was actually applied and
break e.g. resume, but yes that's just refactoring that should be done.

While we're here I also have no idea what recalc_rate() is supposed to
do but that 'return 74250000;' is definitely odd, and I'm sure there are
other improvements that could be made at the edges.

That's quite rude of me given I just sent the patch, but we probably
won't have time to rework this until mid-april after some urgent work
ends (this has actually been waiting for testing for 3 months
already...)
If this doesn't bother anyone else we can wait for a v2 then, but
otherwise it might be worth considering getting as is until refactoring
happens (and I pinky promise I'll find time before this summer -- I can
send a v2 with commit message fixed up as Frieder suggested if this is
acceptable to you)

Thanks for the quick feedback either way, and sorry for the long delay.
-- 
Dominique

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy