Re: [PATCH v3 09/10] v4l: Aptina-style sensor PLL support

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

Hi Sakari,

On Sunday 04 March 2012 00:37:07 Sakari Ailus wrote:
> On Sat, Mar 03, 2012 at 04:28:14PM +0100, Laurent Pinchart wrote:
> > Add a generic helper function to compute PLL parameters for PLL found in
> > several Aptina sensors.

[snip]

> > diff --git a/drivers/media/video/aptina-pll.c
> > b/drivers/media/video/aptina-pll.c new file mode 100644
> > index 0000000..55e4a40
> > --- /dev/null
> > +++ b/drivers/media/video/aptina-pll.c

[snip]

> > +int aptina_pll_configure(struct device *dev, struct aptina_pll *pll,
> > +			 const struct aptina_pll_limits *limits)
> 
> I've done the same to the SMIA++ PLL: it can be used separately from the
> driver now; it'll be part of the next patchset.
> 
> Do you think it could make sense to swap pll and limits parameters?

Why ? :-)

> I call the function smiapp_pll_calculate().

I've renamed the function to aptina_pll_calculate().

> > +{
> > +	unsigned int mf_min;
> > +	unsigned int mf_max;
> > +	unsigned int p1_min;
> > +	unsigned int p1_max;
> > +	unsigned int p1;
> > +	unsigned int div;
> > +
> > +	if (pll->ext_clock < limits->ext_clock_min ||
> > +	    pll->ext_clock > limits->ext_clock_max) {
> > +		dev_err(dev, "pll: invalid external clock frequency.\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	if (pll->pix_clock > limits->pix_clock_max) {
> > +		dev_err(dev, "pll: invalid pixel clock frequency.\n");
> > +		return -EINVAL;
> > +	}
> 
> You could check that pix_clock isn't zero.

OK.

[snip]

> > +	for (p1 = p1_max & ~1; p1 >= p1_min; p1 -= 2) {
> > +		unsigned int mf_inc = lcm(div, p1) / div;
> 
> I think you could avoid division by using p1 * gcd(div, p1) instead.

That's not the same. lcm(div, p1) / div == p1 / gcd(div, p1). There's still a 
division, but it's slightly better, so I'll use that.

> > +		unsigned int mf_high;
> > +		unsigned int mf_low;
> > +
> > +		mf_low = max(roundup(mf_min, mf_inc),
> > +			     DIV_ROUND_UP(pll->ext_clock * p1,
> > +			       limits->int_clock_max * div));
> > +		mf_high = min(mf_max, pll->ext_clock * p1 /
> > +			      (limits->int_clock_min * div));
> > +
> > +		if (mf_low <= mf_high) {
> > +			pll->n = div * mf_low / p1;
> > +			pll->m *= mf_low;
> > +			pll->p1 = p1;
> > +			break;
> 
> You could return already here.

OK.

> > +		}
> > +	}
> > +
> > +	if (p1 < p1_min) {
> > +		dev_err(dev, "pll: no valid N and P1 divisors found.\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	dev_dbg(dev, "PLL: ext clock %u N %u M %u P1 %u pix clock %u\n",
> > +		 pll->ext_clock, pll->n, pll->m, pll->p1, pll->pix_clock);
> > +
> > +	return 0;
> > +}

-- 
Regards,

Laurent Pinchart