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

Laurent Pinchart
2012-03-04T10:14:11Z

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