Hi Sakari,
Thanks for the review.
On Saturday 03 March 2012 00:41:26 Sakari Ailus wrote:
> On Fri, Mar 02, 2012 at 11:44:07AM +0100, Laurent Pinchart wrote:
> > Compute the PLL parameters at runtime using the generic Aptina PLL
> > helper.
> >
> > Remove the PLL parameters from platform data and pass the external clock
> > and desired internal clock frequencies instead.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> > ---
> >
> > drivers/media/video/mt9m032.c | 61
> > ++++++++++++++++++++++++++--------------- include/media/mt9m032.h
> > | 4 +--
> > 2 files changed, 40 insertions(+), 25 deletions(-)
> >
> > diff --git a/drivers/media/video/mt9m032.c b/drivers/media/video/mt9m032.c
> > index 4cde779..45e8c68 100644
> > --- a/drivers/media/video/mt9m032.c
> > +++ b/drivers/media/video/mt9m032.c
> > @@ -35,6 +35,8 @@
> >
> > #include <media/v4l2-device.h>
> > #include <media/v4l2-subdev.h>
> >
> > +#include "aptina-pll.h"
> > +
> >
> > #define MT9M032_CHIP_VERSION 0x00
> > #define MT9M032_CHIP_VERSION_VALUE 0x1402
> > #define MT9M032_ROW_START 0x01
> >
> > @@ -61,6 +63,7 @@
> >
> > #define MT9M032_GAIN_BLUE 0x2c
> > #define MT9M032_GAIN_RED 0x2d
> > #define MT9M032_GAIN_GREEN2 0x2e
> >
> > +
> >
> > /* write only */
> > #define MT9M032_GAIN_ALL 0x35
> > #define MT9M032_GAIN_DIGITAL_MASK 0x7f
> >
> > @@ -83,6 +86,8 @@
> >
> > #define MT9P031_PLL_CONTROL_PWROFF 0x0050
> > #define MT9P031_PLL_CONTROL_PWRON 0x0051
> > #define MT9P031_PLL_CONTROL_USEPLL 0x0052
> >
> > +#define MT9P031_PLL_CONFIG2 0x11
> > +#define MT9P031_PLL_CONFIG2_P1_DIV_MASK 0x1f
> >
> > /*
> >
> > * width and height include active boundry and black parts
> >
> > @@ -223,31 +228,43 @@ static int update_formatter2(struct mt9m032 *sensor,
> > bool streaming)>
> > static int mt9m032_setup_pll(struct mt9m032 *sensor)
> > {
> >
> > struct i2c_client *client = v4l2_get_subdevdata(&sensor->subdev);
> >
> > - struct mt9m032_platform_data* pdata = sensor->pdata;
> > - u16 reg_pll1;
> > - unsigned int pre_div;
> > + struct mt9m032_platform_data *pdata = sensor->pdata;
> > + struct aptina_pll_limits limits;
> > + struct aptina_pll pll;
> >
> > int ret;
> >
> > - /* TODO: also support other pre-div values */
> > - if (pdata->pll_pre_div != 6) {
> > - dev_warn(to_dev(sensor),
> > - "Unsupported PLL pre-divisor value %u, using default 6\n",
> > - pdata->pll_pre_div);
> > - }
> > - pre_div = 6;
> > -
> > - sensor->pix_clock = pdata->ext_clock * pdata->pll_mul /
> > - (pre_div * pdata->pll_out_div);
> > + limits.ext_clock_min = 8000000;
> > + limits.ext_clock_max = 16500000;
> > + limits.int_clock_min = 2000000;
> > + limits.int_clock_max = 24000000;
> > + limits.out_clock_min = 322000000;
> > + limits.out_clock_max = 693000000;
> > + limits.pix_clock_max = 99000000;
> > + limits.n_min = 1;
> > + limits.n_max = 64;
> > + limits.m_min = 16;
> > + limits.m_max = 255;
> > + limits.p1_min = 1;
> > + limits.p1_max = 128;
>
> You could make limits const and static.
Done.
> > + pll.ext_clock = pdata->ext_clock;
> > + pll.pix_clock = pdata->pix_clock;
> > +
> > + ret = aptina_pll_configure(&client->dev, &pll, &limits);
> > + if (ret < 0)
> > + return ret;
>
> Now that you're handling timing information in the sensor driver, including
> blanking --- AFAIU, what would you think about implementing the new sensor
> control interface for this one?
Sure, but in a further patch :-)
> > - reg_pll1 = ((pdata->pll_out_div - 1) &
MT9M032_PLL_CONFIG1_OUTDIV_MASK)
> > - | pdata->pll_mul << MT9M032_PLL_CONFIG1_MUL_SHIFT;
> > + sensor->pix_clock = pll.pix_clock;
> >
> > - ret = mt9m032_write_reg(client, MT9M032_PLL_CONFIG1, reg_pll1);
> > + ret = mt9m032_write_reg(client, MT9M032_PLL_CONFIG1,
> > + (pll.m << MT9M032_PLL_CONFIG1_MUL_SHIFT)
> > + | (pll.p1 - 1));
> >
> > if (!ret)
> >
> > - ret = mt9m032_write_reg(client,
> > - MT9P031_PLL_CONTROL,
> > - MT9P031_PLL_CONTROL_PWRON |
MT9P031_PLL_CONTROL_USEPLL);
> > -
> > + ret = mt9m032_write_reg(client, MT9P031_PLL_CONFIG2, pll.n - 1);
> > + if (!ret)
> > + ret = mt9m032_write_reg(client, MT9P031_PLL_CONTROL,
> > + MT9P031_PLL_CONTROL_PWRON |
> > + MT9P031_PLL_CONTROL_USEPLL);
> >
> > if (!ret)
> >
> > ret = mt9m032_write_reg(client, MT9M032_READ_MODE1, 0x8006);
> >
> > /* more reserved, Continuous */
> >
> > diff --git a/include/media/mt9m032.h b/include/media/mt9m032.h
> > index 94cefc5..804e0a5 100644
> > --- a/include/media/mt9m032.h
> > +++ b/include/media/mt9m032.h
> > @@ -29,9 +29,7 @@
> >
> > struct mt9m032_platform_data {
> >
> > u32 ext_clock;
> >
> > - u32 pll_pre_div;
> > - u32 pll_mul;
> > - u32 pll_out_div;
> > + u32 pix_clock;
> >
> > int invert_pixclock;
> >
> > };
--
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
