Re: [PATCH V7 4/5] phy: freescale: fsl-samsung-hdmi: Use closest divider

[Frieder Schrempf]

On 11.09.24 3:28 AM, Adam Ford wrote:
> Currently, if the clock values cannot be set to the exact rate,
> the round_rate and set_rate functions use the closest value found in
> the look-up-table.  In preparation of removing values from the LUT
> that can be calculated evenly with the integer calculator, it's
> necessary to ensure to check both the look-up-table and the integer
> divider clock values to get the closest values to the requested
> value.  It does this by measuring the difference between the
> requested clock value and the closest value in both integer divider
> calucator and the fractional clock look-up-table.

  ^ calculator

> 
> Which ever has the smallest difference between them is returned as
> the closest rate.
> 
> Signed-off-by: Adam Ford <aford173@xxxxxxxxx>
> ---
> V7:  Because of the previous patch refactoring, the flow of this patch
>      changed quite a bit to use more help functions and goto statements
>      to hopefully make the code flow better and improve comment
>      readability.  Because of the change, I removed s-o-b and r-b,
>      and t-b tags.
> 
> V6:  Simplify the calculation of the closest rate and fix
>      a situation where the integer divider values may not be properly
>      setup before they are used.
>      Fixup some comments
> V5:  No Change
> V4:  New to series
> ---
>  drivers/phy/freescale/phy-fsl-samsung-hdmi.c | 40 ++++++++++++++------
>  1 file changed, 28 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/phy/freescale/phy-fsl-samsung-hdmi.c b/drivers/phy/freescale/phy-fsl-samsung-hdmi.c
> index 49317a96f767..67a28aac9c45 100644
> --- a/drivers/phy/freescale/phy-fsl-samsung-hdmi.c
> +++ b/drivers/phy/freescale/phy-fsl-samsung-hdmi.c
> @@ -577,6 +577,16 @@ static void fsl_samsung_hdmi_calculate_phy(struct phy_config *cal_phy, unsigned
>  	/* pll_div_regs 3-6 are fixed and pre-defined already */
>  }
>  
> +static u32 fsl_samsung_hdmi_phy_get_closest_rate(unsigned long rate,
> +						 u32 int_div_clk, u32 frac_div_clk)
> +{
> +	/* The int_div_clk may be greater than rate, so cast it and use ABS */
> +	if (abs((long)rate - (long)int_div_clk) < (rate - frac_div_clk))
> +		return int_div_clk;
> +
> +	return frac_div_clk;
> +}
> +
>  static long phy_clk_round_rate(struct clk_hw *hw,
>  			       unsigned long rate, unsigned long *parent_rate)
>  {
> @@ -624,27 +634,33 @@ static int phy_clk_set_rate(struct clk_hw *hw,
>  		goto use_fract_div;
>  
>  	/*
> -	 * If the rate from the fractional divder is not exact, check the integer divider,
> +	 * If the rate from the fractional divider is not exact, check the integer divider,
>  	 * and use it if that value is an exact match.
>  	 */
>  	int_div_clk = fsl_samsung_hdmi_phy_find_pms(rate, &p, &m, &s);
> +	fsl_samsung_hdmi_calculate_phy(&calculated_phy_pll_cfg, int_div_clk, p, m, s);
>  	if (int_div_clk == rate) {
> -		dev_dbg(phy->dev, "fsl_samsung_hdmi_phy: integer divider rate = %u\n",
> -				   int_div_clk);
> -
> -		fsl_samsung_hdmi_calculate_phy(&calculated_phy_pll_cfg, int_div_clk, p, m, s);
> -		phy->cur_cfg  = &calculated_phy_pll_cfg;
> -		return fsl_samsung_hdmi_phy_configure(phy, phy->cur_cfg);
> +		goto use_int_div;
>  	}
>  
>  	/*
> -	 * If neither the fractional divder nor the integer divder can find an exact value
> -	 * fall back to using the fractional divider
> +	 * Compare the difference between the integer clock and the fractional clock against
> +	 * the desired clock and which whichever is closest,
>  	 */
> +	if (fsl_samsung_hdmi_phy_get_closest_rate(rate, int_div_clk,
> +						  fract_div_phy->pixclk) == fract_div_phy->pixclk)
> +		goto use_fract_div;
> +
> +use_int_div:
> +	dev_dbg(phy->dev, "fsl_samsung_hdmi_phy: integer divider rate = %u\n", int_div_clk);
> +	phy->cur_cfg  = &calculated_phy_pll_cfg;
> +	goto end;

You are jumping into this part of the code and then back out again to
the end of the function. In my opinion this is beyond what is generally
considered "good" use of goto. It makes the code hard to read.

Can we rewrite/rearrange this to avoid goto in this function? Just
creating additional functions as necessary and calling them in the right
places should do the job without having duplicated code, no?

> +
>  use_fract_div:
> -	phy->cur_cfg = fract_div_phy;
> -	dev_dbg(phy->dev, "fsl_samsung_hdmi_phy: using fractional divider rate = %u\n",
> -			   phy->cur_cfg->pixclk);
> +	 phy->cur_cfg = fract_div_phy;
> +	 dev_dbg(phy->dev, "fsl_samsung_hdmi_phy: using fractional divider rate = %u\n",
> +		   phy->cur_cfg->pixclk);
> +end:
>  	return fsl_samsung_hdmi_phy_configure(phy, phy->cur_cfg);
>  }
>