Re: [PATCH v2 09/11] media: ccs-pll: Better validate VT PLL branch

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Sakari,

Thank you for the patch.

On Thu, Apr 17, 2025 at 09:53:52AM +0300, Sakari Ailus wrote:
> Check that the VT PLL dividers are actually found, don't trust they always
> are even though they should be.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  drivers/media/i2c/ccs-pll.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/i2c/ccs-pll.c b/drivers/media/i2c/ccs-pll.c
> index 3f8153fb4af0..fc6f8aff5fd8 100644
> --- a/drivers/media/i2c/ccs-pll.c
> +++ b/drivers/media/i2c/ccs-pll.c
> @@ -449,7 +449,7 @@ static int ccs_pll_calculate_vt_tree(struct device *dev,
>  	return -EINVAL;
>  }
>  
> -static void
> +static int
>  ccs_pll_calculate_vt(struct device *dev, const struct ccs_pll_limits *lim,
>  		     const struct ccs_pll_branch_limits_bk *op_lim_bk,
>  		     struct ccs_pll *pll, struct ccs_pll_branch_fr *pll_fr,
> @@ -572,6 +572,8 @@ ccs_pll_calculate_vt(struct device *dev, const struct ccs_pll_limits *lim,
>  		if (best_pix_div < SHRT_MAX >> 1)
>  			break;
>  	}
> +	if (best_pix_div == SHRT_MAX >> 1)
> +		return -EINVAL;

I think I would have written

	if (vt_div > max_vt_div)
		return -EINVAL;

to match the for loop condition, this seems a bit more readable to me.
The result should be the same though, so either way,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>  
>  	pll->vt_bk.sys_clk_div = DIV_ROUND_UP(vt_div, best_pix_div);
>  	pll->vt_bk.pix_clk_div = best_pix_div;
> @@ -584,6 +586,8 @@ ccs_pll_calculate_vt(struct device *dev, const struct ccs_pll_limits *lim,
>  out_calc_pixel_rate:
>  	pll->pixel_rate_pixel_array =
>  		pll->vt_bk.pix_clk_freq_hz * pll->vt_lanes;
> +
> +	return 0;
>  }
>  
>  /*
> @@ -863,8 +867,10 @@ int ccs_pll_calculate(struct device *dev, const struct ccs_pll_limits *lim,
>  		if (pll->flags & CCS_PLL_FLAG_DUAL_PLL)
>  			break;
>  
> -		ccs_pll_calculate_vt(dev, lim, op_lim_bk, pll, op_pll_fr,
> -				     op_pll_bk, cphy, phy_const);
> +		rval = ccs_pll_calculate_vt(dev, lim, op_lim_bk, pll, op_pll_fr,
> +					    op_pll_bk, cphy, phy_const);
> +		if (rval)
> +			continue;
>  
>  		rval = check_bk_bounds(dev, lim, pll, PLL_VT);
>  		if (rval)

-- 
Regards,

Laurent Pinchart