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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxxxxxxxxxxxxxx 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@xxxxxxxxxxxxxxx/msg39798.html>

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

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


[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux