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
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
