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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <v57uy3gddzcoeg3refyv7h6j3ypx23mobctybt27xzdyqy6bgb@atzdlqlytz3c>
Date: Mon, 10 Mar 2025 10:14:23 +0100
From: Uwe Kleine-König <u.kleine-koenig@...libre.com>
To: Dominique Martinet <dominique.martinet@...ark-techno.com>
Cc: Vinod Koul <vkoul@...nel.org>, 
	Kishon Vijay Abraham I <kishon@...nel.org>, Adam Ford <aford173@...il.com>, 
	Frieder Schrempf <frieder.schrempf@...tron.de>, Marco Felsch <m.felsch@...gutronix.de>, 
	Lucas Stach <l.stach@...gutronix.de>, linux-phy@...ts.infradead.org, linux-kernel@...r.kernel.org, 
	Makoto Sato <makoto.sato@...ark-techno.com>
Subject: Re: [PATCH] phy: freescale: fsl-samsung-hdmi: return closest rate
 instead LUT

Hello,

On Mon, Mar 10, 2025 at 10:21:32AM +0900, Dominique Martinet wrote:
> From: Makoto Sato <makoto.sato@...ark-techno.com>
> 
> If the requested rate is not an exact match of the integer divider
> phy_clk_round_rate() would return the look up table value,
> but phy_clk_set_rate() can still use the integer divider if it results
> in a frequency that is closer than the look up table.
> 
> In particular, not returning the actually used value here made the hdmi
> bridge driver reject a frequency that has an integer divider rate
> within 0.5% of the target:
> 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?

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?

Ideally the implementation would look conceptually like:

	static long phy_clk_round_rate(..., unsigned long rate, ...)
	{
		hw = phy_determine_register_settings_for(rate);
		if (hw_is_error(hw))
			return someerror;

		return phy_get_rate_from(hw);
	}

	static int phy_clk_set_rate(..., unsigned long rate, ...)
	{
		hw = phy_determine_register_settings_for(rate);
		if (hw_is_error(hw))
			return someerror;

		return phy_apply(hw);
	}

Best regards
Uwe

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)