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

Uwe Kleine-König
2025-03-10T09:14:28Z

[-- Attachment #1: Type: text/plain, Size: 1635 bytes --]

Hello,

On Mon, Mar 10, 2025 at 10:21:32AM +0900, Dominique Martinet wrote:
> From: Makoto Sato <makoto.sato@atmark-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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]