Hi Martin,
On Saturday 17 December 2011 10:50:51 martin@xxxxxxxxxxxxxxxxxxxxxx wrote:
> On Thu, Dec 15, 2011 at 12:11:13AM +0200, Sakari Ailus wrote:
> > 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.
That's what I was referring to, yes. The idea would be to pass the input
(external) clock frequency to the driver, as well as the desired pixel clock.
Both are properties of the board hardware, so they will be included in DT
bindings. The PLL parameters are not properties of the board hardware, so they
should be computed automatically.
> 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.htm
> > l>
>
> Sounds sensible for the future. But let's not hold this driver until
> agreement is reached about the details?
--
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
