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

Sakari Ailus
2012-03-04T15:01:34Z

Hi Laurent,

Laurent Pinchart wrote:
> 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 ? :-)

Uh, I have it that way. ;-) Also both dev and limits contain perhaps
less interesting or const information than pll, which contains both
input and output parameters.

>> 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.

Right; you can put that on the late hour I was writing this at. ;-)

>>> + 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.

Or even:

if (mf_low > mf_high)
continue;

dev_dbg(stuff);
return 0;

I find this often easier to read. It's up to you.

>>> + }
>>> + }
>>> +
>>> + 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;
>>> +}
>

--
Sakari Ailus
sakari.ailus@iki.fi