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

[Sakari Ailus <sakari.ailus@linux.intel.com>]

Hi Laurent,

On Mon, Apr 21, 2025 at 11:24:29PM +0300, Laurent Pinchart wrote:
> 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.

Comparing with the value assigned in the variable initialisation seems most
straightforward to me. But the value could be U16_MAX, that'd be nicer than
SHRT_MAX >> 1. U16_MAX was added before the first instance of this was
merged. :-)

I'll add a new patch to change this, after this set.

> The result should be the same though, so either way,
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Thank you!

-- 
Regards,

Sakari Ailus