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

[Sakari Ailus <sakari.ailus@iki.fi>]

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