Re: [PATCH v3] v4l: Add driver for Micron MT9M032 camera sensor

[martin@neutronstar.dyndns.org]

On Thu, Dec 15, 2011 at 12:11:13AM +0200, Sakari Ailus wrote:
> Hi Martin,
> 
> On Wed, Dec 14, 2011 at 07:58:45PM +0100, martin@neutronstar.dyndns.org wrote:
> ...
> > > > > > +static int mt9m032_setup_pll(struct mt9m032 *sensor)
> > > > > > +{
> > > > > > +	struct mt9m032_platform_data* pdata = sensor->pdata;
> > > > > > +	u16 reg_pll1;
> > > > > > +	unsigned int pre_div;
> > > > > > +	int res, ret;
> > > > > > +
> > > > > > +	/* TODO: also support other pre-div values */
> > > 
> > > I might already have mentioned this, but wouldn't it be time to work a on real 
> > > PLL setup code that compute the pre-divisor, multiplier and output divisor 
> > > dynamically from the input and output clock frequencies ?
> > 
> > I'm not sure what the implications for quality and stability of such a
> > generic setup would be. My gut feeling is most users go with known working
> > hardcoded values.
> 
> You'd get a lot better control of the sensor as a bonus in doing so. Also,
> you could program the sensor properly suitable for the host it is connected
> to, achieving optimal maximum frame rates for it.
> 
> These values tend to be relatively board / bridge dependent. On one board
> some frequencies might not be usable even if they do not exceed the maximum
> for the bridge.

Yes, that's why i have exported almost all of the pll details i'm reasonanly sure
that they are settable in the platform data. I think this gives maximum
flexibility in the board configuration. Maybe something with less values to fiddle
with in the normal case would be better. But not really by a large amount.

static struct mt9m032_platform_data mt9m032_platform_data = {
	.ext_clock = 13500000,
	.pll_pre_div = 6,
	.pll_mul = 120,
	.pll_out_div = 5,
	.invert_pixclock = 1,
};

I think what Laurent was talking about was moving to a setup where the board
doesn't need to specify the exact details. i.e. have pll_pre_div, pll_mul and
pll_out_div rolled into one. I'm not sure that's something that should be done.

And this driver does feel like it had it's share of tracking in development
interfaces.

> 
> Please also see this:
> 
> <URL:http://www.mail-archive.com/linux-media@vger.kernel.org/msg39798.html>

Sounds sensible for the future. But let's not hold this driver until agreement is
reached about the details?

 - Martin