[PATCH v3 00/10] MT9M032 sensor driver

[PATCH v3 00/10] MT9M032 sensor driver

[Laurent Pinchart]

Hi everybody,

Here's the third version of the Aptina MT9M032 sensor driver, originally
written by Martin Hostettler.

Compared to the second version, I've optimized the PLL code to get rid of the
inner loop. The limits are now declared as static const in the mt9m032 driver.

Laurent Pinchart (9):
  mt9m032: Reorder code into section and whitespace cleanups
  mt9m032: Make get/set format/crop operations consistent across
    drivers
  mt9m032: Use module_i2c_driver() macro
  mt9m032: Enclose to_dev() macro argument in brackets
  mt9m032: Pass an i2c_client pointer to the register read/write
    functions
  mt9m032: Put HFLIP and VFLIP controls in a cluster
  mt9m032: Remove unneeded register read
  v4l: Aptina-style sensor PLL support
  mt9m032: Use generic PLL setup code

Martin Hostettler (1):
  v4l: Add driver for Micron MT9M032 camera sensor

 drivers/media/video/Kconfig      |   11 +
 drivers/media/video/Makefile     |    5 +
 drivers/media/video/aptina-pll.c |  175 ++++++++
 drivers/media/video/aptina-pll.h |   55 +++
 drivers/media/video/mt9m032.c    |  828 ++++++++++++++++++++++++++++++++++++++
 include/media/mt9m032.h          |   36 ++
 6 files changed, 1110 insertions(+), 0 deletions(-)
 create mode 100644 drivers/media/video/aptina-pll.c
 create mode 100644 drivers/media/video/aptina-pll.h
 create mode 100644 drivers/media/video/mt9m032.c
 create mode 100644 include/media/mt9m032.h

-- 
Regards,

Laurent Pinchart

[PATCH v3 01/10] v4l: Add driver for Micron MT9M032 camera sensor

[Laurent Pinchart]

From: Martin Hostettler <martin@neutronstar.dyndns.org>

The MT9M032 is a parallel 1.6MP sensor from Micron controlled through I2C.

The driver creates a V4L2 subdevice. It currently supports cropping, gain,
exposure and v/h flipping controls in monochrome mode with an
external pixel clock.

Signed-off-by: Martin Hostettler <martin@neutronstar.dyndns.org>
---
 drivers/media/video/Kconfig   |    7 +
 drivers/media/video/Makefile  |    1 +
 drivers/media/video/mt9m032.c |  832 +++++++++++++++++++++++++++++++++++++++++
 include/media/mt9m032.h       |   38 ++
 4 files changed, 878 insertions(+), 0 deletions(-)
 create mode 100644 drivers/media/video/mt9m032.c
 create mode 100644 include/media/mt9m032.h

diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig
index 9495b6a..80acb78 100644
--- a/drivers/media/video/Kconfig
+++ b/drivers/media/video/Kconfig
@@ -943,6 +943,13 @@ config SOC_CAMERA_MT9M001
 	  This driver supports MT9M001 cameras from Micron, monochrome
 	  and colour models.
 
+config VIDEO_MT9M032
+	tristate "MT9M032 camera sensor support"
+	depends on I2C && VIDEO_V4L2
+	help
+	  This driver supports MT9M032 cameras from Micron, monochrome
+	  models only.
+
 config SOC_CAMERA_MT9M111
 	tristate "mt9m111, mt9m112 and mt9m131 support"
 	depends on SOC_CAMERA && I2C
diff --git a/drivers/media/video/Makefile b/drivers/media/video/Makefile
index 563443c..9b19533 100644
--- a/drivers/media/video/Makefile
+++ b/drivers/media/video/Makefile
@@ -78,6 +78,7 @@ obj-$(CONFIG_VIDEO_AS3645A)	+= as3645a.o
 
 obj-$(CONFIG_SOC_CAMERA_IMX074)		+= imx074.o
 obj-$(CONFIG_SOC_CAMERA_MT9M001)	+= mt9m001.o
+obj-$(CONFIG_VIDEO_MT9M032)             += mt9m032.o
 obj-$(CONFIG_SOC_CAMERA_MT9M111)	+= mt9m111.o
 obj-$(CONFIG_SOC_CAMERA_MT9T031)	+= mt9t031.o
 obj-$(CONFIG_SOC_CAMERA_MT9T112)	+= mt9t112.o
diff --git a/drivers/media/video/mt9m032.c b/drivers/media/video/mt9m032.c
new file mode 100644
index 0000000..92495f7
--- /dev/null
+++ b/drivers/media/video/mt9m032.c
@@ -0,0 +1,832 @@
+/*
+ * Driver for MT9M032 CMOS Image Sensor from Micron
+ *
+ * Copyright (C) 2010-2011 Lund Engineering
+ * Contact: Gil Lund <gwlund@lundeng.com>
+ * Author: Martin Hostettler <martin@neutronstar.dyndns.org>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
+ * 02110-1301 USA
+ */
+
+#include <linux/delay.h>
+#include <linux/i2c.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/math64.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/v4l2-mediabus.h>
+
+#include <media/media-entity.h>
+#include <media/v4l2-ctrls.h>
+#include <media/v4l2-device.h>
+#include <media/v4l2-subdev.h>
+
+#include <media/mt9m032.h>
+
+#define MT9M032_CHIP_VERSION			0x00
+#define     MT9M032_CHIP_VERSION_VALUE		0x1402
+#define MT9M032_ROW_START			0x01
+#define MT9M032_COLUMN_START			0x02
+#define MT9M032_ROW_SIZE			0x03
+#define MT9M032_COLUMN_SIZE			0x04
+#define MT9M032_HBLANK				0x05
+#define MT9M032_VBLANK				0x06
+#define MT9M032_SHUTTER_WIDTH_HIGH		0x08
+#define MT9M032_SHUTTER_WIDTH_LOW		0x09
+#define MT9M032_PIX_CLK_CTRL			0x0a
+#define     MT9M032_PIX_CLK_CTRL_INV_PIXCLK	0x8000
+#define MT9M032_RESTART				0x0b
+#define MT9M032_RESET				0x0d
+#define MT9M032_PLL_CONFIG1			0x11
+#define     MT9M032_PLL_CONFIG1_OUTDIV_MASK	0x3f
+#define     MT9M032_PLL_CONFIG1_MUL_SHIFT	8
+#define MT9M032_READ_MODE1			0x1e
+#define MT9M032_READ_MODE2			0x20
+#define     MT9M032_READ_MODE2_VFLIP_SHIFT	15
+#define     MT9M032_READ_MODE2_HFLIP_SHIFT	14
+#define     MT9M032_READ_MODE2_ROW_BLC		0x40
+#define MT9M032_GAIN_GREEN1			0x2b
+#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
+#define     MT9M032_GAIN_DIGITAL_SHIFT		8
+#define     MT9M032_GAIN_AMUL_SHIFT		6
+#define     MT9M032_GAIN_ANALOG_MASK		0x3f
+#define MT9M032_FORMATTER1			0x9e
+#define MT9M032_FORMATTER2			0x9f
+#define     MT9M032_FORMATTER2_DOUT_EN		0x1000
+#define     MT9M032_FORMATTER2_PIXCLK_EN	0x2000
+
+#define MT9M032_MAX_BLANKING_ROWS		0x7ff
+
+
+/*
+ * The availible MT9M032 datasheet is missing documentation for register 0x10
+ * MT9P031 seems to be close enough, so use constants from that datasheet for
+ * now.
+ * But keep the name MT9P031 to remind us, that this isn't really confirmed
+ * for this sensor.
+ */
+
+#define MT9P031_PLL_CONTROL			0x10
+#define     MT9P031_PLL_CONTROL_PWROFF		0x0050
+#define     MT9P031_PLL_CONTROL_PWRON		0x0051
+#define     MT9P031_PLL_CONTROL_USEPLL		0x0052
+
+
+
+/*
+ * width and height include active boundry and black parts
+ *
+ * column    0-  15 active boundry
+ * column   16-1455 image
+ * column 1456-1471 active boundry
+ * column 1472-1599 black
+ *
+ * row       0-  51 black
+ * row      53-  59 active boundry
+ * row      60-1139 image
+ * row    1140-1147 active boundry
+ * row    1148-1151 black
+ */
+#define MT9M032_WIDTH				1600
+#define MT9M032_HEIGHT				1152
+#define MT9M032_MINIMALSIZE			32
+
+#define to_mt9m032(sd)	container_of(sd, struct mt9m032, subdev)
+#define to_dev(sensor)	&((struct i2c_client *)v4l2_get_subdevdata(&sensor->subdev))->dev
+
+struct mt9m032 {
+	struct v4l2_subdev subdev;
+	struct media_pad pad;
+	struct mt9m032_platform_data *pdata;
+	struct v4l2_ctrl_handler ctrls;
+
+	bool streaming;
+
+	int pix_clock;
+
+	struct v4l2_mbus_framefmt format;	/* height and width always the same as in crop */
+	struct v4l2_rect crop;
+	struct v4l2_fract frame_interval;
+
+	struct v4l2_ctrl *hflip, *vflip;
+};
+
+
+static int mt9m032_read_reg(struct mt9m032 *sensor, u8 reg)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(&sensor->subdev);
+
+	return i2c_smbus_read_word_swapped(client, reg);
+}
+
+static int mt9m032_write_reg(struct mt9m032 *sensor, u8 reg,
+		     const u16 data)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(&sensor->subdev);
+
+	return i2c_smbus_write_word_swapped(client, reg, data);
+}
+
+
+static unsigned long mt9m032_row_time(struct mt9m032 *sensor, int width)
+{
+	int effective_width;
+	u64 ns;
+
+	effective_width = width + 716; /* emperical value */
+	ns = div_u64(((u64)1000000000) * effective_width, sensor->pix_clock);
+	dev_dbg(to_dev(sensor),	"MT9M032 line time: %llu ns\n", ns);
+	return ns;
+}
+
+static int mt9m032_update_timing(struct mt9m032 *sensor,
+				 struct v4l2_fract *interval,
+				 const struct v4l2_rect *crop)
+{
+	unsigned long row_time;
+	int additional_blanking_rows;
+	int min_blank;
+
+	if (!interval)
+		interval = &sensor->frame_interval;
+	if (!crop)
+		crop = &sensor->crop;
+
+	row_time = mt9m032_row_time(sensor, crop->width);
+
+	additional_blanking_rows = div_u64(((u64)1000000000) * interval->numerator,
+	                                  ((u64)interval->denominator) * row_time)
+	                           - crop->height;
+
+	if (additional_blanking_rows > MT9M032_MAX_BLANKING_ROWS) {
+		/* hardware limits to 11 bit values */
+		interval->denominator = 1000;
+		interval->numerator = div_u64((crop->height + MT9M032_MAX_BLANKING_ROWS)
+		                              * ((u64)row_time) * interval->denominator,
+					      1000000000);
+		additional_blanking_rows = div_u64(((u64)1000000000) * interval->numerator,
+	                                  ((u64)interval->denominator) * row_time)
+	                           - crop->height;
+	}
+	/* enforce minimal 1.6ms blanking time. */
+	min_blank = 1600000 / row_time;
+	additional_blanking_rows = clamp(additional_blanking_rows,
+	                                 min_blank, MT9M032_MAX_BLANKING_ROWS);
+
+	return mt9m032_write_reg(sensor, MT9M032_VBLANK, additional_blanking_rows);
+}
+
+static int mt9m032_update_geom_timing(struct mt9m032 *sensor,
+				 const struct v4l2_rect *crop)
+{
+	int ret;
+
+	ret = mt9m032_write_reg(sensor, MT9M032_COLUMN_SIZE, crop->width - 1);
+	if (!ret)
+		ret = mt9m032_write_reg(sensor, MT9M032_ROW_SIZE, crop->height - 1);
+	/* offsets compensate for black border */
+	if (!ret)
+		ret = mt9m032_write_reg(sensor, MT9M032_COLUMN_START, crop->left);
+	if (!ret)
+		ret = mt9m032_write_reg(sensor, MT9M032_ROW_START, crop->top);
+	if (!ret)
+		ret = mt9m032_update_timing(sensor, NULL, crop);
+	return ret;
+}
+
+static int update_formatter2(struct mt9m032 *sensor, bool streaming)
+{
+	u16 reg_val =   MT9M032_FORMATTER2_DOUT_EN
+		      | 0x0070;  /* parts reserved! */
+				 /* possibly for changing to 14-bit mode */
+
+	if (streaming)
+		reg_val |= MT9M032_FORMATTER2_PIXCLK_EN;   /* pixclock enable */
+
+	return mt9m032_write_reg(sensor, MT9M032_FORMATTER2, reg_val);
+}
+
+static int mt9m032_s_stream(struct v4l2_subdev *subdev, int streaming)
+{
+	struct mt9m032 *sensor = to_mt9m032(subdev);
+	int ret;
+
+	ret = update_formatter2(sensor, streaming);
+	if (!ret)
+		sensor->streaming = streaming;
+	return ret;
+}
+
+static int mt9m032_enum_mbus_code(struct v4l2_subdev *subdev,
+				  struct v4l2_subdev_fh *fh,
+				  struct v4l2_subdev_mbus_code_enum *code)
+{
+	if (code->index != 0 || code->pad != 0)
+		return -EINVAL;
+	code->code = V4L2_MBUS_FMT_Y8_1X8;
+	return 0;
+}
+
+static int mt9m032_enum_frame_size(struct v4l2_subdev *subdev,
+				   struct v4l2_subdev_fh *fh,
+				   struct v4l2_subdev_frame_size_enum *fse)
+{
+	if (fse->index != 0 || fse->code != V4L2_MBUS_FMT_Y8_1X8 || fse->pad != 0)
+		return -EINVAL;
+
+	fse->min_width = MT9M032_WIDTH;
+	fse->max_width = MT9M032_WIDTH;
+	fse->min_height = MT9M032_HEIGHT;
+	fse->max_height = MT9M032_HEIGHT;
+
+	return 0;
+}
+
+/**
+ * __mt9m032_get_pad_crop() - get crop rect
+ * @sensor:	pointer to the sensor struct
+ * @fh:	filehandle for getting the try crop rect from
+ * @which:	select try or active crop rect
+ * Returns a pointer the current active or fh relative try crop rect
+ */
+static struct v4l2_rect *__mt9m032_get_pad_crop(struct mt9m032 *sensor,
+						struct v4l2_subdev_fh *fh,
+						u32 which)
+{
+	switch (which) {
+	case V4L2_SUBDEV_FORMAT_TRY:
+		return v4l2_subdev_get_try_crop(fh, 0);
+	case V4L2_SUBDEV_FORMAT_ACTIVE:
+		return &sensor->crop;
+	default:
+		return NULL;
+	}
+}
+
+/**
+ * __mt9m032_get_pad_format() - get format
+ * @sensor:	pointer to the sensor struct
+ * @fh:	filehandle for getting the try format from
+ * @which:	select try or active format
+ * Returns a pointer the current active or fh relative try format
+ */
+static struct v4l2_mbus_framefmt *__mt9m032_get_pad_format(struct mt9m032 *sensor,
+							   struct v4l2_subdev_fh *fh,
+							   u32 which)
+{
+	switch (which) {
+	case V4L2_SUBDEV_FORMAT_TRY:
+		return v4l2_subdev_get_try_format(fh, 0);
+	case V4L2_SUBDEV_FORMAT_ACTIVE:
+		return &sensor->format;
+	default:
+		return NULL;
+	}
+}
+
+static int mt9m032_get_pad_format(struct v4l2_subdev *subdev,
+				  struct v4l2_subdev_fh *fh,
+				  struct v4l2_subdev_format *fmt)
+{
+	struct mt9m032 *sensor = to_mt9m032(subdev);
+	struct v4l2_mbus_framefmt *format;
+
+	format = __mt9m032_get_pad_format(sensor, fh, fmt->which);
+
+	fmt->format = *format;
+
+	return 0;
+}
+
+static int mt9m032_set_pad_format(struct v4l2_subdev *subdev,
+				  struct v4l2_subdev_fh *fh,
+				  struct v4l2_subdev_format *fmt)
+{
+	struct mt9m032 *sensor = to_mt9m032(subdev);
+
+	if (sensor->streaming)
+		return -EBUSY;
+
+	/*
+	 * fmt->format.colorspace, fmt->format.code and fmt->format.field are ignored
+	 * and thus forced to fixed values by the get call below.
+	 *
+	 * fmt->format.width, fmt->format.height are forced to the values set via crop
+	 */
+
+	return mt9m032_get_pad_format(subdev, fh, fmt);
+}
+
+static int mt9m032_get_crop(struct v4l2_subdev *subdev, struct v4l2_subdev_fh *fh,
+			    struct v4l2_subdev_crop *crop)
+{
+	struct mt9m032 *sensor = to_mt9m032(subdev);
+	struct v4l2_rect *curcrop;
+
+	curcrop = __mt9m032_get_pad_crop(sensor, fh, crop->which);
+
+	crop->rect = *curcrop;
+
+	return 0;
+}
+
+static int mt9m032_set_crop(struct v4l2_subdev *subdev, struct v4l2_subdev_fh *fh,
+		     struct v4l2_subdev_crop *crop)
+{
+	struct mt9m032 *sensor = to_mt9m032(subdev);
+	struct v4l2_mbus_framefmt tmp_format;
+	struct v4l2_rect tmp_crop_rect;
+	struct v4l2_mbus_framefmt *format;
+	struct v4l2_rect *crop_rect;
+	int ret = 0;
+
+	if (sensor->streaming)
+		return -EBUSY;
+
+	format = __mt9m032_get_pad_format(sensor, fh, crop->which);
+	crop_rect = __mt9m032_get_pad_crop(sensor, fh, crop->which);
+	if (crop->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
+		tmp_crop_rect = *crop_rect;
+		tmp_format = *format;
+		format = &tmp_format;
+		crop_rect = &tmp_crop_rect;
+	}
+
+	crop_rect->top = clamp(crop->rect.top, 0,
+			       MT9M032_HEIGHT - MT9M032_MINIMALSIZE) & ~1;
+	crop_rect->left = clamp(crop->rect.left, 0,
+			       MT9M032_WIDTH - MT9M032_MINIMALSIZE);
+	crop_rect->height = clamp(crop->rect.height, MT9M032_MINIMALSIZE,
+				  MT9M032_HEIGHT - crop_rect->top);
+	crop_rect->width = clamp(crop->rect.width, MT9M032_MINIMALSIZE,
+				 MT9M032_WIDTH - crop_rect->left) & ~1;
+
+	format->height = crop_rect->height;
+	format->width = crop_rect->width;
+
+	if (crop->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
+		ret = mt9m032_update_geom_timing(sensor, crop_rect);
+
+		if (!ret) {
+			sensor->crop = tmp_crop_rect;
+			sensor->format = tmp_format;
+		}
+		return ret;
+	}
+
+	return ret;
+}
+
+static int mt9m032_get_frame_interval(struct v4l2_subdev *subdev,
+				      struct v4l2_subdev_frame_interval *fi)
+{
+	struct mt9m032 *sensor = to_mt9m032(subdev);
+
+	fi->pad = 0;
+	memset(fi->reserved, 0, sizeof(fi->reserved));
+	fi->interval = sensor->frame_interval;
+
+	return 0;
+}
+
+static int mt9m032_set_frame_interval(struct v4l2_subdev *subdev,
+				      struct v4l2_subdev_frame_interval *fi)
+{
+	struct mt9m032 *sensor = to_mt9m032(subdev);
+	int ret;
+
+	if (sensor->streaming)
+		return -EBUSY;
+
+	memset(fi->reserved, 0, sizeof(fi->reserved));
+
+	ret = mt9m032_update_timing(sensor, &fi->interval, NULL);
+	if (!ret)
+		sensor->frame_interval = fi->interval;
+	return ret;
+}
+
+#ifdef CONFIG_VIDEO_ADV_DEBUG
+static int mt9m032_g_register(struct v4l2_subdev *sd,
+			      struct v4l2_dbg_register *reg)
+{
+	struct mt9m032 *sensor = to_mt9m032(sd);
+	struct i2c_client *client = v4l2_get_subdevdata(&sensor->subdev);
+	int val;
+
+	if (reg->match.type != V4L2_CHIP_MATCH_I2C_ADDR || reg->reg > 0xff)
+		return -EINVAL;
+	if (reg->match.addr != client->addr)
+		return -ENODEV;
+
+	val = mt9m032_read_reg(sensor, reg->reg);
+	if (val < 0)
+		return -EIO;
+
+	reg->size = 2;
+	reg->val = val;
+
+	return 0;
+}
+
+static int mt9m032_s_register(struct v4l2_subdev *sd,
+			      struct v4l2_dbg_register *reg)
+{
+	struct mt9m032 *sensor = to_mt9m032(sd);
+	struct i2c_client *client = v4l2_get_subdevdata(&sensor->subdev);
+
+	if (reg->match.type != V4L2_CHIP_MATCH_I2C_ADDR || reg->reg > 0xff)
+		return -EINVAL;
+
+	if (reg->match.addr != client->addr)
+		return -ENODEV;
+
+	if (mt9m032_write_reg(sensor, reg->reg, reg->val) < 0)
+		return -EIO;
+
+	return 0;
+}
+#endif
+
+static int update_read_mode2(struct mt9m032 *sensor, bool vflip, bool hflip)
+{
+	int reg_val = (!!vflip) << MT9M032_READ_MODE2_VFLIP_SHIFT
+		      | (!!hflip) << MT9M032_READ_MODE2_HFLIP_SHIFT
+		      | MT9M032_READ_MODE2_ROW_BLC
+		      | 0x0007;
+
+	return mt9m032_write_reg(sensor, MT9M032_READ_MODE2, reg_val);
+}
+
+static int mt9m032_set_hflip(struct mt9m032 *sensor, s32 val)
+{
+	return update_read_mode2(sensor, sensor->vflip->cur.val, val);
+}
+
+static int mt9m032_set_vflip(struct mt9m032 *sensor, s32 val)
+{
+	return update_read_mode2(sensor, val, sensor->hflip->cur.val);
+}
+
+static int mt9m032_set_exposure(struct mt9m032 *sensor, s32 val)
+{
+	int shutter_width;
+	u16 high_val, low_val;
+	int ret;
+
+	/* shutter width is in row times */
+	shutter_width = (val * 1000) / mt9m032_row_time(sensor, sensor->crop.width);
+
+	high_val = (shutter_width >> 16) & 0xf;
+	low_val = shutter_width & 0xffff;
+
+	ret = mt9m032_write_reg(sensor, MT9M032_SHUTTER_WIDTH_HIGH, high_val);
+	if (!ret)
+		ret = mt9m032_write_reg(sensor, MT9M032_SHUTTER_WIDTH_LOW, low_val);
+
+	return ret;
+}
+
+static int mt9m032_set_gain(struct mt9m032 *sensor, s32 val)
+{
+	int digital_gain_val;	/* in 1/8th (0..127) */
+	int analog_mul;		/* 0 or 1 */
+	int analog_gain_val;	/* in 1/16th. (0..63) */
+	u16 reg_val;
+
+	digital_gain_val = 51; /* from setup example */
+
+	if (val < 63) {
+		analog_mul = 0;
+		analog_gain_val = val;
+	} else {
+		analog_mul = 1;
+		analog_gain_val = val / 2;
+	}
+
+	/* a_gain = (1+analog_mul) + (analog_gain_val+1)/16 */
+	/* overall_gain = a_gain * (1 + digital_gain_val / 8) */
+
+	reg_val = (digital_gain_val & MT9M032_GAIN_DIGITAL_MASK) << MT9M032_GAIN_DIGITAL_SHIFT
+		  | (analog_mul & 1) << MT9M032_GAIN_AMUL_SHIFT
+		  | (analog_gain_val & MT9M032_GAIN_ANALOG_MASK);
+
+	return mt9m032_write_reg(sensor, MT9M032_GAIN_ALL, reg_val);
+}
+
+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 */
+	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);
+
+	reg_pll1 = ((pdata->pll_out_div - 1) & MT9M032_PLL_CONFIG1_OUTDIV_MASK)
+		   | pdata->pll_mul << MT9M032_PLL_CONFIG1_MUL_SHIFT;
+
+	ret = mt9m032_write_reg(sensor, MT9M032_PLL_CONFIG1, reg_pll1);
+	if (!ret)
+		ret = mt9m032_write_reg(sensor,
+		                        MT9P031_PLL_CONTROL,
+		                        MT9P031_PLL_CONTROL_PWRON | MT9P031_PLL_CONTROL_USEPLL);
+
+	if (!ret)
+		ret = mt9m032_write_reg(sensor, MT9M032_READ_MODE1, 0x8006);
+							/* more reserved, Continuous */
+							/* Master Mode */
+	if (!ret)
+		res = mt9m032_read_reg(sensor, MT9M032_READ_MODE1);
+
+	if (!ret)
+		ret = mt9m032_write_reg(sensor, MT9M032_FORMATTER1, 0x111e);
+					/* Set 14-bit mode, select 7 divider */
+
+	return ret;
+}
+
+static int mt9m032_try_ctrl(struct v4l2_ctrl *ctrl)
+{
+	if (ctrl->id == V4L2_CID_GAIN && ctrl->val >= 63) {
+		 /* round because of multiplier used for values >= 63 */
+		ctrl->val &= ~1;
+	}
+
+	return 0;
+}
+
+static int mt9m032_set_ctrl(struct v4l2_ctrl *ctrl)
+{
+	struct mt9m032 *sensor = container_of(ctrl->handler, struct mt9m032, ctrls);
+
+	switch (ctrl->id) {
+	case V4L2_CID_GAIN:
+		return mt9m032_set_gain(sensor, ctrl->val);
+
+	case V4L2_CID_HFLIP:
+		return mt9m032_set_hflip(sensor, ctrl->val);
+
+	case V4L2_CID_VFLIP:
+		return mt9m032_set_vflip(sensor, ctrl->val);
+
+	case V4L2_CID_EXPOSURE:
+		return mt9m032_set_exposure(sensor, ctrl->val);
+
+	default:
+		return -EINVAL;
+	}
+}
+
+static const struct v4l2_subdev_video_ops mt9m032_video_ops = {
+	.s_stream = mt9m032_s_stream,
+	.g_frame_interval = mt9m032_get_frame_interval,
+	.s_frame_interval = mt9m032_set_frame_interval,
+};
+
+static struct v4l2_ctrl_ops mt9m032_ctrl_ops = {
+	.s_ctrl = mt9m032_set_ctrl,
+	.try_ctrl = mt9m032_try_ctrl,
+};
+
+
+static const struct v4l2_subdev_core_ops mt9m032_core_ops = {
+#ifdef CONFIG_VIDEO_ADV_DEBUG
+	.g_register = mt9m032_g_register,
+	.s_register = mt9m032_s_register,
+#endif
+};
+
+static const struct v4l2_subdev_pad_ops mt9m032_pad_ops = {
+	.enum_mbus_code = mt9m032_enum_mbus_code,
+	.enum_frame_size = mt9m032_enum_frame_size,
+	.get_fmt = mt9m032_get_pad_format,
+	.set_fmt = mt9m032_set_pad_format,
+	.set_crop = mt9m032_set_crop,
+	.get_crop = mt9m032_get_crop,
+};
+
+static const struct v4l2_subdev_ops mt9m032_ops = {
+	.core = &mt9m032_core_ops,
+	.video = &mt9m032_video_ops,
+	.pad = &mt9m032_pad_ops,
+};
+
+static int mt9m032_probe(struct i2c_client *client,
+			 const struct i2c_device_id *devid)
+{
+	struct mt9m032 *sensor;
+	int chip_version;
+	int res, ret;
+
+	if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_WORD_DATA)) {
+		dev_warn(&client->adapter->dev,
+			 "I2C-Adapter doesn't support I2C_FUNC_SMBUS_WORD\n");
+		return -EIO;
+	}
+
+	if (!client->dev.platform_data)
+		return -ENODEV;
+
+	sensor = kzalloc(sizeof(*sensor), GFP_KERNEL);
+	if (sensor == NULL)
+		return -ENOMEM;
+
+	sensor->pdata = client->dev.platform_data;
+
+	v4l2_i2c_subdev_init(&sensor->subdev, client, &mt9m032_ops);
+	sensor->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
+
+	/*
+	 * This driver was developed with a camera module with seperate external
+	 * pix clock. For setups which use the clock from the camera interface
+	 * the code will need to be extended with the appropriate platform
+	 * callback to setup the clock.
+	 */
+	chip_version = mt9m032_read_reg(sensor, MT9M032_CHIP_VERSION);
+	if (chip_version == MT9M032_CHIP_VERSION_VALUE) {
+		dev_info(&client->dev, "mt9m032: detected sensor.\n");
+	} else {
+		dev_warn(&client->dev, "mt9m032: error: detected unsupported chip version 0x%x\n",
+			 chip_version);
+		ret = -ENODEV;
+		goto free_sensor;
+	}
+
+	sensor->frame_interval.numerator = 1;
+	sensor->frame_interval.denominator = 30;
+
+	sensor->crop.left = 416;
+	sensor->crop.top = 360;
+	sensor->crop.width = 640;
+	sensor->crop.height = 480;
+
+	sensor->format.width = sensor->crop.width;
+	sensor->format.height = sensor->crop.height;
+	sensor->format.code = V4L2_MBUS_FMT_Y8_1X8;
+	sensor->format.field = V4L2_FIELD_NONE;
+	sensor->format.colorspace = V4L2_COLORSPACE_SRGB;
+
+	v4l2_ctrl_handler_init(&sensor->ctrls, 4);
+
+	v4l2_ctrl_new_std(&sensor->ctrls, &mt9m032_ctrl_ops,
+			  V4L2_CID_GAIN, 0, 127, 1, 64);
+
+	sensor->hflip = v4l2_ctrl_new_std(&sensor->ctrls, &mt9m032_ctrl_ops,
+			  V4L2_CID_HFLIP, 0, 1, 1, 0);
+	sensor->vflip = v4l2_ctrl_new_std(&sensor->ctrls, &mt9m032_ctrl_ops,
+			  V4L2_CID_VFLIP, 0, 1, 1, 0);
+	v4l2_ctrl_new_std(&sensor->ctrls, &mt9m032_ctrl_ops,
+			  V4L2_CID_EXPOSURE, 0, 8000, 1, 1700);    /* 1.7ms */
+
+
+	if (sensor->ctrls.error) {
+		ret = sensor->ctrls.error;
+		dev_err(&client->dev, "control initialization error %d\n", ret);
+		goto free_ctrl;
+	}
+
+	sensor->subdev.ctrl_handler = &sensor->ctrls;
+	sensor->pad.flags = MEDIA_PAD_FL_SOURCE;
+	ret = media_entity_init(&sensor->subdev.entity, 1, &sensor->pad, 0);
+	if (ret < 0)
+		goto free_ctrl;
+
+	ret = mt9m032_write_reg(sensor, MT9M032_RESET, 1);	/* reset on */
+	if (ret < 0)
+		goto free_ctrl;
+	mt9m032_write_reg(sensor, MT9M032_RESET, 0);	/* reset off */
+	if (ret < 0)
+		goto free_ctrl;
+
+	ret = mt9m032_setup_pll(sensor);
+	if (ret < 0)
+		goto free_ctrl;
+	msleep(10);
+
+	v4l2_ctrl_handler_setup(&sensor->ctrls);
+
+	/* SIZE */
+	ret = mt9m032_update_geom_timing(sensor, &sensor->crop);
+	if (ret < 0)
+		goto free_ctrl;
+
+	ret = mt9m032_write_reg(sensor, 0x41, 0x0000);	/* reserved !!! */
+	if (ret < 0)
+		goto free_ctrl;
+	ret = mt9m032_write_reg(sensor, 0x42, 0x0003);	/* reserved !!! */
+	if (ret < 0)
+		goto free_ctrl;
+	ret = mt9m032_write_reg(sensor, 0x43, 0x0003);	/* reserved !!! */
+	if (ret < 0)
+		goto free_ctrl;
+	ret = mt9m032_write_reg(sensor, 0x7f, 0x0000);	/* reserved !!! */
+	if (ret < 0)
+		goto free_ctrl;
+	if (sensor->pdata->invert_pixclock) {
+		mt9m032_write_reg(sensor, MT9M032_PIX_CLK_CTRL, MT9M032_PIX_CLK_CTRL_INV_PIXCLK);
+		if (ret < 0)
+			goto free_ctrl;
+	}
+
+	res = mt9m032_read_reg(sensor, MT9M032_PIX_CLK_CTRL);
+
+	ret = mt9m032_write_reg(sensor, MT9M032_RESTART, 1); /* Restart on */
+	if (ret < 0)
+		goto free_ctrl;
+	msleep(100);
+	ret = mt9m032_write_reg(sensor, MT9M032_RESTART, 0); /* Restart off */
+	if (ret < 0)
+		goto free_ctrl;
+	msleep(100);
+	ret = update_formatter2(sensor, false);
+	if (ret < 0)
+		goto free_ctrl;
+
+	return ret;
+
+free_ctrl:
+	v4l2_ctrl_handler_free(&sensor->ctrls);
+
+free_sensor:
+	kfree(sensor);
+	return ret;
+}
+
+static int mt9m032_remove(struct i2c_client *client)
+{
+	struct v4l2_subdev *subdev = i2c_get_clientdata(client);
+	struct mt9m032 *sensor = to_mt9m032(subdev);
+
+	v4l2_device_unregister_subdev(&sensor->subdev);
+	v4l2_ctrl_handler_free(&sensor->ctrls);
+	media_entity_cleanup(&sensor->subdev.entity);
+	kfree(sensor);
+	return 0;
+}
+
+static const struct i2c_device_id mt9m032_id_table[] = {
+	{MT9M032_NAME, 0},
+	{}
+};
+
+MODULE_DEVICE_TABLE(i2c, mt9m032_id_table);
+
+static struct i2c_driver mt9m032_i2c_driver = {
+	.driver = {
+		   .name = MT9M032_NAME,
+		   },
+	.probe = mt9m032_probe,
+	.remove = mt9m032_remove,
+	.id_table = mt9m032_id_table,
+};
+
+static int __init mt9m032_init(void)
+{
+	int rval;
+
+	rval = i2c_add_driver(&mt9m032_i2c_driver);
+	if (rval)
+		pr_err("%s: failed registering " MT9M032_NAME "\n", __func__);
+
+	return rval;
+}
+
+static void mt9m032_exit(void)
+{
+	i2c_del_driver(&mt9m032_i2c_driver);
+}
+
+module_init(mt9m032_init);
+module_exit(mt9m032_exit);
+
+MODULE_AUTHOR("Martin Hostettler <martin@neutronstar.dyndns.org>");
+MODULE_DESCRIPTION("MT9M032 camera sensor driver");
+MODULE_LICENSE("GPL v2");
diff --git a/include/media/mt9m032.h b/include/media/mt9m032.h
new file mode 100644
index 0000000..94cefc5
--- /dev/null
+++ b/include/media/mt9m032.h
@@ -0,0 +1,38 @@
+/*
+ * Driver for MT9M032 CMOS Image Sensor from Micron
+ *
+ * Copyright (C) 2010-2011 Lund Engineering
+ * Contact: Gil Lund <gwlund@lundeng.com>
+ * Author: Martin Hostettler <martin@neutronstar.dyndns.org>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
+ * 02110-1301 USA
+ *
+ */
+
+#ifndef MT9M032_H
+#define MT9M032_H
+
+#define MT9M032_NAME		"mt9m032"
+#define MT9M032_I2C_ADDR	(0xb8 >> 1)
+
+struct mt9m032_platform_data {
+	u32 ext_clock;
+	u32 pll_pre_div;
+	u32 pll_mul;
+	u32 pll_out_div;
+	int invert_pixclock;
+
+};
+#endif /* MT9M032_H */
-- 
1.7.3.4

[PATCH v3 02/10] mt9m032: Reorder code into section and whitespace cleanups

[Laurent Pinchart]

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/video/mt9m032.c |  162 +++++++++++++++++++++-------------------
 1 files changed, 85 insertions(+), 77 deletions(-)

diff --git a/drivers/media/video/mt9m032.c b/drivers/media/video/mt9m032.c
index 92495f7..b6ef69f 100644
--- a/drivers/media/video/mt9m032.c
+++ b/drivers/media/video/mt9m032.c
@@ -30,12 +30,11 @@
 #include <linux/v4l2-mediabus.h>
 
 #include <media/media-entity.h>
+#include <media/mt9m032.h>
 #include <media/v4l2-ctrls.h>
 #include <media/v4l2-device.h>
 #include <media/v4l2-subdev.h>
 
-#include <media/mt9m032.h>
-
 #define MT9M032_CHIP_VERSION			0x00
 #define     MT9M032_CHIP_VERSION_VALUE		0x1402
 #define MT9M032_ROW_START			0x01
@@ -73,24 +72,18 @@
 #define     MT9M032_FORMATTER2_DOUT_EN		0x1000
 #define     MT9M032_FORMATTER2_PIXCLK_EN	0x2000
 
-#define MT9M032_MAX_BLANKING_ROWS		0x7ff
-
-
 /*
- * The availible MT9M032 datasheet is missing documentation for register 0x10
+ * The available MT9M032 datasheet is missing documentation for register 0x10
  * MT9P031 seems to be close enough, so use constants from that datasheet for
  * now.
  * But keep the name MT9P031 to remind us, that this isn't really confirmed
  * for this sensor.
  */
-
 #define MT9P031_PLL_CONTROL			0x10
 #define     MT9P031_PLL_CONTROL_PWROFF		0x0050
 #define     MT9P031_PLL_CONTROL_PWRON		0x0051
 #define     MT9P031_PLL_CONTROL_USEPLL		0x0052
 
-
-
 /*
  * width and height include active boundry and black parts
  *
@@ -108,9 +101,7 @@
 #define MT9M032_WIDTH				1600
 #define MT9M032_HEIGHT				1152
 #define MT9M032_MINIMALSIZE			32
-
-#define to_mt9m032(sd)	container_of(sd, struct mt9m032, subdev)
-#define to_dev(sensor)	&((struct i2c_client *)v4l2_get_subdevdata(&sensor->subdev))->dev
+#define MT9M032_MAX_BLANKING_ROWS		0x7ff
 
 struct mt9m032 {
 	struct v4l2_subdev subdev;
@@ -129,6 +120,8 @@ struct mt9m032 {
 	struct v4l2_ctrl *hflip, *vflip;
 };
 
+#define to_mt9m032(sd)	container_of(sd, struct mt9m032, subdev)
+#define to_dev(sensor)	&((struct i2c_client *)v4l2_get_subdevdata(&sensor->subdev))->dev
 
 static int mt9m032_read_reg(struct mt9m032 *sensor, u8 reg)
 {
@@ -145,7 +138,6 @@ static int mt9m032_write_reg(struct mt9m032 *sensor, u8 reg,
 	return i2c_smbus_write_word_swapped(client, reg, data);
 }
 
-
 static unsigned long mt9m032_row_time(struct mt9m032 *sensor, int width)
 {
 	int effective_width;
@@ -173,23 +165,23 @@ static int mt9m032_update_timing(struct mt9m032 *sensor,
 	row_time = mt9m032_row_time(sensor, crop->width);
 
 	additional_blanking_rows = div_u64(((u64)1000000000) * interval->numerator,
-	                                  ((u64)interval->denominator) * row_time)
-	                           - crop->height;
+					  ((u64)interval->denominator) * row_time)
+				   - crop->height;
 
 	if (additional_blanking_rows > MT9M032_MAX_BLANKING_ROWS) {
 		/* hardware limits to 11 bit values */
 		interval->denominator = 1000;
 		interval->numerator = div_u64((crop->height + MT9M032_MAX_BLANKING_ROWS)
-		                              * ((u64)row_time) * interval->denominator,
+					      * ((u64)row_time) * interval->denominator,
 					      1000000000);
 		additional_blanking_rows = div_u64(((u64)1000000000) * interval->numerator,
-	                                  ((u64)interval->denominator) * row_time)
-	                           - crop->height;
+					  ((u64)interval->denominator) * row_time)
+				   - crop->height;
 	}
 	/* enforce minimal 1.6ms blanking time. */
 	min_blank = 1600000 / row_time;
 	additional_blanking_rows = clamp(additional_blanking_rows,
-	                                 min_blank, MT9M032_MAX_BLANKING_ROWS);
+					 min_blank, MT9M032_MAX_BLANKING_ROWS);
 
 	return mt9m032_write_reg(sensor, MT9M032_VBLANK, additional_blanking_rows);
 }
@@ -224,17 +216,51 @@ static int update_formatter2(struct mt9m032 *sensor, bool streaming)
 	return mt9m032_write_reg(sensor, MT9M032_FORMATTER2, reg_val);
 }
 
-static int mt9m032_s_stream(struct v4l2_subdev *subdev, int streaming)
+static int mt9m032_setup_pll(struct mt9m032 *sensor)
 {
-	struct mt9m032 *sensor = to_mt9m032(subdev);
-	int ret;
+	struct mt9m032_platform_data* pdata = sensor->pdata;
+	u16 reg_pll1;
+	unsigned int pre_div;
+	int res, ret;
 
-	ret = update_formatter2(sensor, streaming);
+	/* 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);
+
+	reg_pll1 = ((pdata->pll_out_div - 1) & MT9M032_PLL_CONFIG1_OUTDIV_MASK)
+		   | pdata->pll_mul << MT9M032_PLL_CONFIG1_MUL_SHIFT;
+
+	ret = mt9m032_write_reg(sensor, MT9M032_PLL_CONFIG1, reg_pll1);
 	if (!ret)
-		sensor->streaming = streaming;
+		ret = mt9m032_write_reg(sensor,
+					MT9P031_PLL_CONTROL,
+					MT9P031_PLL_CONTROL_PWRON | MT9P031_PLL_CONTROL_USEPLL);
+
+	if (!ret)
+		ret = mt9m032_write_reg(sensor, MT9M032_READ_MODE1, 0x8006);
+							/* more reserved, Continuous */
+							/* Master Mode */
+	if (!ret)
+		res = mt9m032_read_reg(sensor, MT9M032_READ_MODE1);
+
+	if (!ret)
+		ret = mt9m032_write_reg(sensor, MT9M032_FORMATTER1, 0x111e);
+					/* Set 14-bit mode, select 7 divider */
+
 	return ret;
 }
 
+/* -----------------------------------------------------------------------------
+ * Subdev pad operations
+ */
+
 static int mt9m032_enum_mbus_code(struct v4l2_subdev *subdev,
 				  struct v4l2_subdev_fh *fh,
 				  struct v4l2_subdev_mbus_code_enum *code)
@@ -424,6 +450,21 @@ static int mt9m032_set_frame_interval(struct v4l2_subdev *subdev,
 	return ret;
 }
 
+static int mt9m032_s_stream(struct v4l2_subdev *subdev, int streaming)
+{
+	struct mt9m032 *sensor = to_mt9m032(subdev);
+	int ret;
+
+	ret = update_formatter2(sensor, streaming);
+	if (!ret)
+		sensor->streaming = streaming;
+	return ret;
+}
+
+/* -----------------------------------------------------------------------------
+ * V4L2 subdev core operations
+ */
+
 #ifdef CONFIG_VIDEO_ADV_DEBUG
 static int mt9m032_g_register(struct v4l2_subdev *sd,
 			      struct v4l2_dbg_register *reg)
@@ -466,6 +507,10 @@ static int mt9m032_s_register(struct v4l2_subdev *sd,
 }
 #endif
 
+/* -----------------------------------------------------------------------------
+ * V4L2 subdev control operations
+ */
+
 static int update_read_mode2(struct mt9m032 *sensor, bool vflip, bool hflip)
 {
 	int reg_val = (!!vflip) << MT9M032_READ_MODE2_VFLIP_SHIFT
@@ -532,51 +577,10 @@ static int mt9m032_set_gain(struct mt9m032 *sensor, s32 val)
 	return mt9m032_write_reg(sensor, MT9M032_GAIN_ALL, reg_val);
 }
 
-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 */
-	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);
-
-	reg_pll1 = ((pdata->pll_out_div - 1) & MT9M032_PLL_CONFIG1_OUTDIV_MASK)
-		   | pdata->pll_mul << MT9M032_PLL_CONFIG1_MUL_SHIFT;
-
-	ret = mt9m032_write_reg(sensor, MT9M032_PLL_CONFIG1, reg_pll1);
-	if (!ret)
-		ret = mt9m032_write_reg(sensor,
-		                        MT9P031_PLL_CONTROL,
-		                        MT9P031_PLL_CONTROL_PWRON | MT9P031_PLL_CONTROL_USEPLL);
-
-	if (!ret)
-		ret = mt9m032_write_reg(sensor, MT9M032_READ_MODE1, 0x8006);
-							/* more reserved, Continuous */
-							/* Master Mode */
-	if (!ret)
-		res = mt9m032_read_reg(sensor, MT9M032_READ_MODE1);
-
-	if (!ret)
-		ret = mt9m032_write_reg(sensor, MT9M032_FORMATTER1, 0x111e);
-					/* Set 14-bit mode, select 7 divider */
-
-	return ret;
-}
-
 static int mt9m032_try_ctrl(struct v4l2_ctrl *ctrl)
 {
 	if (ctrl->id == V4L2_CID_GAIN && ctrl->val >= 63) {
-		 /* round because of multiplier used for values >= 63 */
+		/* round because of multiplier used for values >= 63 */
 		ctrl->val &= ~1;
 	}
 
@@ -605,17 +609,12 @@ static int mt9m032_set_ctrl(struct v4l2_ctrl *ctrl)
 	}
 }
 
-static const struct v4l2_subdev_video_ops mt9m032_video_ops = {
-	.s_stream = mt9m032_s_stream,
-	.g_frame_interval = mt9m032_get_frame_interval,
-	.s_frame_interval = mt9m032_set_frame_interval,
-};
-
 static struct v4l2_ctrl_ops mt9m032_ctrl_ops = {
 	.s_ctrl = mt9m032_set_ctrl,
 	.try_ctrl = mt9m032_try_ctrl,
 };
 
+/* -------------------------------------------------------------------------- */
 
 static const struct v4l2_subdev_core_ops mt9m032_core_ops = {
 #ifdef CONFIG_VIDEO_ADV_DEBUG
@@ -624,6 +623,12 @@ static const struct v4l2_subdev_core_ops mt9m032_core_ops = {
 #endif
 };
 
+static const struct v4l2_subdev_video_ops mt9m032_video_ops = {
+	.s_stream = mt9m032_s_stream,
+	.g_frame_interval = mt9m032_get_frame_interval,
+	.s_frame_interval = mt9m032_set_frame_interval,
+};
+
 static const struct v4l2_subdev_pad_ops mt9m032_pad_ops = {
 	.enum_mbus_code = mt9m032_enum_mbus_code,
 	.enum_frame_size = mt9m032_enum_frame_size,
@@ -639,6 +644,10 @@ static const struct v4l2_subdev_ops mt9m032_ops = {
 	.pad = &mt9m032_pad_ops,
 };
 
+/* -----------------------------------------------------------------------------
+ * Driver initialization and probing
+ */
+
 static int mt9m032_probe(struct i2c_client *client,
 			 const struct i2c_device_id *devid)
 {
@@ -706,7 +715,6 @@ static int mt9m032_probe(struct i2c_client *client,
 	v4l2_ctrl_new_std(&sensor->ctrls, &mt9m032_ctrl_ops,
 			  V4L2_CID_EXPOSURE, 0, 8000, 1, 1700);    /* 1.7ms */
 
-
 	if (sensor->ctrls.error) {
 		ret = sensor->ctrls.error;
 		dev_err(&client->dev, "control initialization error %d\n", ret);
@@ -793,16 +801,16 @@ static int mt9m032_remove(struct i2c_client *client)
 }
 
 static const struct i2c_device_id mt9m032_id_table[] = {
-	{MT9M032_NAME, 0},
-	{}
+	{ MT9M032_NAME, 0 },
+	{ }
 };
 
 MODULE_DEVICE_TABLE(i2c, mt9m032_id_table);
 
 static struct i2c_driver mt9m032_i2c_driver = {
 	.driver = {
-		   .name = MT9M032_NAME,
-		   },
+		.name = MT9M032_NAME,
+	},
 	.probe = mt9m032_probe,
 	.remove = mt9m032_remove,
 	.id_table = mt9m032_id_table,
-- 
1.7.3.4

[PATCH v3 03/10] mt9m032: Make get/set format/crop operations consistent across drivers

[Laurent Pinchart]

Modify the get/set format/crop operation handlers to match the existing
pad-aware Aptina sensor drivers.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/video/mt9m032.c |  132 +++++++++++++++++++----------------------
 1 files changed, 61 insertions(+), 71 deletions(-)

diff --git a/drivers/media/video/mt9m032.c b/drivers/media/video/mt9m032.c
index b6ef69f..e09f9a5 100644
--- a/drivers/media/video/mt9m032.c
+++ b/drivers/media/video/mt9m032.c
@@ -150,17 +150,15 @@ static unsigned long mt9m032_row_time(struct mt9m032 *sensor, int width)
 }
 
 static int mt9m032_update_timing(struct mt9m032 *sensor,
-				 struct v4l2_fract *interval,
-				 const struct v4l2_rect *crop)
+				 struct v4l2_fract *interval)
 {
+	struct v4l2_rect *crop = &sensor->crop;
 	unsigned long row_time;
 	int additional_blanking_rows;
 	int min_blank;
 
 	if (!interval)
 		interval = &sensor->frame_interval;
-	if (!crop)
-		crop = &sensor->crop;
 
 	row_time = mt9m032_row_time(sensor, crop->width);
 
@@ -186,21 +184,24 @@ static int mt9m032_update_timing(struct mt9m032 *sensor,
 	return mt9m032_write_reg(sensor, MT9M032_VBLANK, additional_blanking_rows);
 }
 
-static int mt9m032_update_geom_timing(struct mt9m032 *sensor,
-				 const struct v4l2_rect *crop)
+static int mt9m032_update_geom_timing(struct mt9m032 *sensor)
 {
 	int ret;
 
-	ret = mt9m032_write_reg(sensor, MT9M032_COLUMN_SIZE, crop->width - 1);
+	ret = mt9m032_write_reg(sensor, MT9M032_COLUMN_SIZE,
+				sensor->crop.width - 1);
 	if (!ret)
-		ret = mt9m032_write_reg(sensor, MT9M032_ROW_SIZE, crop->height - 1);
+		ret = mt9m032_write_reg(sensor, MT9M032_ROW_SIZE,
+					sensor->crop.height - 1);
 	/* offsets compensate for black border */
 	if (!ret)
-		ret = mt9m032_write_reg(sensor, MT9M032_COLUMN_START, crop->left);
+		ret = mt9m032_write_reg(sensor, MT9M032_COLUMN_START,
+					sensor->crop.left);
 	if (!ret)
-		ret = mt9m032_write_reg(sensor, MT9M032_ROW_START, crop->top);
+		ret = mt9m032_write_reg(sensor, MT9M032_ROW_START,
+					sensor->crop.top);
 	if (!ret)
-		ret = mt9m032_update_timing(sensor, NULL, crop);
+		ret = mt9m032_update_timing(sensor, NULL);
 	return ret;
 }
 
@@ -265,8 +266,9 @@ static int mt9m032_enum_mbus_code(struct v4l2_subdev *subdev,
 				  struct v4l2_subdev_fh *fh,
 				  struct v4l2_subdev_mbus_code_enum *code)
 {
-	if (code->index != 0 || code->pad != 0)
+	if (code->index != 0)
 		return -EINVAL;
+
 	code->code = V4L2_MBUS_FMT_Y8_1X8;
 	return 0;
 }
@@ -275,7 +277,7 @@ static int mt9m032_enum_frame_size(struct v4l2_subdev *subdev,
 				   struct v4l2_subdev_fh *fh,
 				   struct v4l2_subdev_frame_size_enum *fse)
 {
-	if (fse->index != 0 || fse->code != V4L2_MBUS_FMT_Y8_1X8 || fse->pad != 0)
+	if (fse->index != 0 || fse->code != V4L2_MBUS_FMT_Y8_1X8)
 		return -EINVAL;
 
 	fse->min_width = MT9M032_WIDTH;
@@ -288,14 +290,15 @@ static int mt9m032_enum_frame_size(struct v4l2_subdev *subdev,
 
 /**
  * __mt9m032_get_pad_crop() - get crop rect
- * @sensor:	pointer to the sensor struct
- * @fh:	filehandle for getting the try crop rect from
- * @which:	select try or active crop rect
+ * @sensor: pointer to the sensor struct
+ * @fh: filehandle for getting the try crop rect from
+ * @which: select try or active crop rect
+ *
  * Returns a pointer the current active or fh relative try crop rect
  */
-static struct v4l2_rect *__mt9m032_get_pad_crop(struct mt9m032 *sensor,
-						struct v4l2_subdev_fh *fh,
-						u32 which)
+static struct v4l2_rect *
+__mt9m032_get_pad_crop(struct mt9m032 *sensor, struct v4l2_subdev_fh *fh,
+		       enum v4l2_subdev_format_whence which)
 {
 	switch (which) {
 	case V4L2_SUBDEV_FORMAT_TRY:
@@ -309,14 +312,15 @@ static struct v4l2_rect *__mt9m032_get_pad_crop(struct mt9m032 *sensor,
 
 /**
  * __mt9m032_get_pad_format() - get format
- * @sensor:	pointer to the sensor struct
- * @fh:	filehandle for getting the try format from
- * @which:	select try or active format
+ * @sensor: pointer to the sensor struct
+ * @fh: filehandle for getting the try format from
+ * @which: select try or active format
+ *
  * Returns a pointer the current active or fh relative try format
  */
-static struct v4l2_mbus_framefmt *__mt9m032_get_pad_format(struct mt9m032 *sensor,
-							   struct v4l2_subdev_fh *fh,
-							   u32 which)
+static struct v4l2_mbus_framefmt *
+__mt9m032_get_pad_format(struct mt9m032 *sensor, struct v4l2_subdev_fh *fh,
+			 enum v4l2_subdev_format_whence which)
 {
 	switch (which) {
 	case V4L2_SUBDEV_FORMAT_TRY:
@@ -333,12 +337,8 @@ static int mt9m032_get_pad_format(struct v4l2_subdev *subdev,
 				  struct v4l2_subdev_format *fmt)
 {
 	struct mt9m032 *sensor = to_mt9m032(subdev);
-	struct v4l2_mbus_framefmt *format;
-
-	format = __mt9m032_get_pad_format(sensor, fh, fmt->which);
-
-	fmt->format = *format;
 
+	fmt->format = *__mt9m032_get_pad_format(sensor, fh, fmt->which);
 	return 0;
 }
 
@@ -365,11 +365,8 @@ static int mt9m032_get_crop(struct v4l2_subdev *subdev, struct v4l2_subdev_fh *f
 			    struct v4l2_subdev_crop *crop)
 {
 	struct mt9m032 *sensor = to_mt9m032(subdev);
-	struct v4l2_rect *curcrop;
 
-	curcrop = __mt9m032_get_pad_crop(sensor, fh, crop->which);
-
-	crop->rect = *curcrop;
+	crop->rect = *__mt9m032_get_pad_crop(sensor, fh, crop->which);
 
 	return 0;
 }
@@ -378,47 +375,40 @@ static int mt9m032_set_crop(struct v4l2_subdev *subdev, struct v4l2_subdev_fh *f
 		     struct v4l2_subdev_crop *crop)
 {
 	struct mt9m032 *sensor = to_mt9m032(subdev);
-	struct v4l2_mbus_framefmt tmp_format;
-	struct v4l2_rect tmp_crop_rect;
 	struct v4l2_mbus_framefmt *format;
-	struct v4l2_rect *crop_rect;
-	int ret = 0;
+	struct v4l2_rect *__crop;
+	struct v4l2_rect rect;
 
 	if (sensor->streaming)
 		return -EBUSY;
 
-	format = __mt9m032_get_pad_format(sensor, fh, crop->which);
-	crop_rect = __mt9m032_get_pad_crop(sensor, fh, crop->which);
-	if (crop->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
-		tmp_crop_rect = *crop_rect;
-		tmp_format = *format;
-		format = &tmp_format;
-		crop_rect = &tmp_crop_rect;
+	rect.top = clamp(crop->rect.top, 0,
+			 MT9M032_HEIGHT - MT9M032_MINIMALSIZE) & ~1;
+	rect.left = clamp(crop->rect.left, 0,
+			  MT9M032_WIDTH - MT9M032_MINIMALSIZE);
+	rect.height = clamp(crop->rect.height, MT9M032_MINIMALSIZE,
+			    MT9M032_HEIGHT - rect.top);
+	rect.width = clamp(crop->rect.width, MT9M032_MINIMALSIZE,
+			   MT9M032_WIDTH - rect.left) & ~1;
+
+	__crop = __mt9m032_get_pad_crop(sensor, fh, crop->which);
+
+	if (rect.width != __crop->width || rect.height != __crop->height) {
+		/* Reset the output image size if the crop rectangle size has
+		 * been modified.
+		 */
+		format = __mt9m032_get_pad_format(sensor, fh, crop->which);
+		format->width = rect.width;
+		format->height = rect.height;
 	}
 
-	crop_rect->top = clamp(crop->rect.top, 0,
-			       MT9M032_HEIGHT - MT9M032_MINIMALSIZE) & ~1;
-	crop_rect->left = clamp(crop->rect.left, 0,
-			       MT9M032_WIDTH - MT9M032_MINIMALSIZE);
-	crop_rect->height = clamp(crop->rect.height, MT9M032_MINIMALSIZE,
-				  MT9M032_HEIGHT - crop_rect->top);
-	crop_rect->width = clamp(crop->rect.width, MT9M032_MINIMALSIZE,
-				 MT9M032_WIDTH - crop_rect->left) & ~1;
-
-	format->height = crop_rect->height;
-	format->width = crop_rect->width;
-
-	if (crop->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
-		ret = mt9m032_update_geom_timing(sensor, crop_rect);
-
-		if (!ret) {
-			sensor->crop = tmp_crop_rect;
-			sensor->format = tmp_format;
-		}
-		return ret;
-	}
+	*__crop = rect;
+	crop->rect = rect;
 
-	return ret;
+	if (crop->which != V4L2_SUBDEV_FORMAT_ACTIVE)
+		return 0;
+
+	return mt9m032_update_geom_timing(sensor);
 }
 
 static int mt9m032_get_frame_interval(struct v4l2_subdev *subdev,
@@ -426,8 +416,7 @@ static int mt9m032_get_frame_interval(struct v4l2_subdev *subdev,
 {
 	struct mt9m032 *sensor = to_mt9m032(subdev);
 
-	fi->pad = 0;
-	memset(fi->reserved, 0, sizeof(fi->reserved));
+	memset(fi, 0, sizeof(*fi));
 	fi->interval = sensor->frame_interval;
 
 	return 0;
@@ -444,9 +433,10 @@ static int mt9m032_set_frame_interval(struct v4l2_subdev *subdev,
 
 	memset(fi->reserved, 0, sizeof(fi->reserved));
 
-	ret = mt9m032_update_timing(sensor, &fi->interval, NULL);
+	ret = mt9m032_update_timing(sensor, &fi->interval);
 	if (!ret)
 		sensor->frame_interval = fi->interval;
+
 	return ret;
 }
 
@@ -742,7 +732,7 @@ static int mt9m032_probe(struct i2c_client *client,
 	v4l2_ctrl_handler_setup(&sensor->ctrls);
 
 	/* SIZE */
-	ret = mt9m032_update_geom_timing(sensor, &sensor->crop);
+	ret = mt9m032_update_geom_timing(sensor);
 	if (ret < 0)
 		goto free_ctrl;
 
-- 
1.7.3.4

[PATCH v3 04/10] mt9m032: Use module_i2c_driver() macro

[Laurent Pinchart]

Replace the custom driver init and exit functions by
module_i2c_driver().

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/video/mt9m032.c |   19 +------------------
 1 files changed, 1 insertions(+), 18 deletions(-)

diff --git a/drivers/media/video/mt9m032.c b/drivers/media/video/mt9m032.c
index e09f9a5..a821b91 100644
--- a/drivers/media/video/mt9m032.c
+++ b/drivers/media/video/mt9m032.c
@@ -806,24 +806,7 @@ static struct i2c_driver mt9m032_i2c_driver = {
 	.id_table = mt9m032_id_table,
 };
 
-static int __init mt9m032_init(void)
-{
-	int rval;
-
-	rval = i2c_add_driver(&mt9m032_i2c_driver);
-	if (rval)
-		pr_err("%s: failed registering " MT9M032_NAME "\n", __func__);
-
-	return rval;
-}
-
-static void mt9m032_exit(void)
-{
-	i2c_del_driver(&mt9m032_i2c_driver);
-}
-
-module_init(mt9m032_init);
-module_exit(mt9m032_exit);
+module_i2c_driver(mt9m032_i2c_driver);
 
 MODULE_AUTHOR("Martin Hostettler <martin@neutronstar.dyndns.org>");
 MODULE_DESCRIPTION("MT9M032 camera sensor driver");
-- 
1.7.3.4

[PATCH v3 05/10] mt9m032: Enclose to_dev() macro argument in brackets

[Laurent Pinchart]

To make the macro safer to use, enclose its argument in brackets in the
macro's body.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/video/mt9m032.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/media/video/mt9m032.c b/drivers/media/video/mt9m032.c
index a821b91..74f0cdd 100644
--- a/drivers/media/video/mt9m032.c
+++ b/drivers/media/video/mt9m032.c
@@ -121,7 +121,8 @@ struct mt9m032 {
 };
 
 #define to_mt9m032(sd)	container_of(sd, struct mt9m032, subdev)
-#define to_dev(sensor)	&((struct i2c_client *)v4l2_get_subdevdata(&sensor->subdev))->dev
+#define to_dev(sensor) \
+	(&((struct i2c_client *)v4l2_get_subdevdata(&(sensor)->subdev))->dev)
 
 static int mt9m032_read_reg(struct mt9m032 *sensor, u8 reg)
 {
-- 
1.7.3.4

[PATCH v3 06/10] mt9m032: Pass an i2c_client pointer to the register read/write functions

[Laurent Pinchart]

Replace the mt9m032 * argument to register read/write functions with an
i2c_client *. As the register access functions are often called several
times in a single location, this removes several casts at runtime.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/video/mt9m032.c |   78 +++++++++++++++++++++--------------------
 1 files changed, 40 insertions(+), 38 deletions(-)

diff --git a/drivers/media/video/mt9m032.c b/drivers/media/video/mt9m032.c
index 74f0cdd..cfed53a 100644
--- a/drivers/media/video/mt9m032.c
+++ b/drivers/media/video/mt9m032.c
@@ -124,18 +124,13 @@ struct mt9m032 {
 #define to_dev(sensor) \
 	(&((struct i2c_client *)v4l2_get_subdevdata(&(sensor)->subdev))->dev)
 
-static int mt9m032_read_reg(struct mt9m032 *sensor, u8 reg)
+static int mt9m032_read_reg(struct i2c_client *client, u8 reg)
 {
-	struct i2c_client *client = v4l2_get_subdevdata(&sensor->subdev);
-
 	return i2c_smbus_read_word_swapped(client, reg);
 }
 
-static int mt9m032_write_reg(struct mt9m032 *sensor, u8 reg,
-		     const u16 data)
+static int mt9m032_write_reg(struct i2c_client *client, u8 reg, const u16 data)
 {
-	struct i2c_client *client = v4l2_get_subdevdata(&sensor->subdev);
-
 	return i2c_smbus_write_word_swapped(client, reg, data);
 }
 
@@ -153,6 +148,7 @@ static unsigned long mt9m032_row_time(struct mt9m032 *sensor, int width)
 static int mt9m032_update_timing(struct mt9m032 *sensor,
 				 struct v4l2_fract *interval)
 {
+	struct i2c_client *client = v4l2_get_subdevdata(&sensor->subdev);
 	struct v4l2_rect *crop = &sensor->crop;
 	unsigned long row_time;
 	int additional_blanking_rows;
@@ -182,24 +178,26 @@ static int mt9m032_update_timing(struct mt9m032 *sensor,
 	additional_blanking_rows = clamp(additional_blanking_rows,
 					 min_blank, MT9M032_MAX_BLANKING_ROWS);
 
-	return mt9m032_write_reg(sensor, MT9M032_VBLANK, additional_blanking_rows);
+	return mt9m032_write_reg(client, MT9M032_VBLANK,
+				 additional_blanking_rows);
 }
 
 static int mt9m032_update_geom_timing(struct mt9m032 *sensor)
 {
+	struct i2c_client *client = v4l2_get_subdevdata(&sensor->subdev);
 	int ret;
 
-	ret = mt9m032_write_reg(sensor, MT9M032_COLUMN_SIZE,
+	ret = mt9m032_write_reg(client, MT9M032_COLUMN_SIZE,
 				sensor->crop.width - 1);
 	if (!ret)
-		ret = mt9m032_write_reg(sensor, MT9M032_ROW_SIZE,
+		ret = mt9m032_write_reg(client, MT9M032_ROW_SIZE,
 					sensor->crop.height - 1);
 	/* offsets compensate for black border */
 	if (!ret)
-		ret = mt9m032_write_reg(sensor, MT9M032_COLUMN_START,
+		ret = mt9m032_write_reg(client, MT9M032_COLUMN_START,
 					sensor->crop.left);
 	if (!ret)
-		ret = mt9m032_write_reg(sensor, MT9M032_ROW_START,
+		ret = mt9m032_write_reg(client, MT9M032_ROW_START,
 					sensor->crop.top);
 	if (!ret)
 		ret = mt9m032_update_timing(sensor, NULL);
@@ -208,6 +206,7 @@ static int mt9m032_update_geom_timing(struct mt9m032 *sensor)
 
 static int update_formatter2(struct mt9m032 *sensor, bool streaming)
 {
+	struct i2c_client *client = v4l2_get_subdevdata(&sensor->subdev);
 	u16 reg_val =   MT9M032_FORMATTER2_DOUT_EN
 		      | 0x0070;  /* parts reserved! */
 				 /* possibly for changing to 14-bit mode */
@@ -215,11 +214,12 @@ static int update_formatter2(struct mt9m032 *sensor, bool streaming)
 	if (streaming)
 		reg_val |= MT9M032_FORMATTER2_PIXCLK_EN;   /* pixclock enable */
 
-	return mt9m032_write_reg(sensor, MT9M032_FORMATTER2, reg_val);
+	return mt9m032_write_reg(client, MT9M032_FORMATTER2, reg_val);
 }
 
 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;
@@ -239,21 +239,21 @@ static int mt9m032_setup_pll(struct mt9m032 *sensor)
 	reg_pll1 = ((pdata->pll_out_div - 1) & MT9M032_PLL_CONFIG1_OUTDIV_MASK)
 		   | pdata->pll_mul << MT9M032_PLL_CONFIG1_MUL_SHIFT;
 
-	ret = mt9m032_write_reg(sensor, MT9M032_PLL_CONFIG1, reg_pll1);
+	ret = mt9m032_write_reg(client, MT9M032_PLL_CONFIG1, reg_pll1);
 	if (!ret)
-		ret = mt9m032_write_reg(sensor,
+		ret = mt9m032_write_reg(client,
 					MT9P031_PLL_CONTROL,
 					MT9P031_PLL_CONTROL_PWRON | MT9P031_PLL_CONTROL_USEPLL);
 
 	if (!ret)
-		ret = mt9m032_write_reg(sensor, MT9M032_READ_MODE1, 0x8006);
+		ret = mt9m032_write_reg(client, MT9M032_READ_MODE1, 0x8006);
 							/* more reserved, Continuous */
 							/* Master Mode */
 	if (!ret)
-		res = mt9m032_read_reg(sensor, MT9M032_READ_MODE1);
+		res = mt9m032_read_reg(client, MT9M032_READ_MODE1);
 
 	if (!ret)
-		ret = mt9m032_write_reg(sensor, MT9M032_FORMATTER1, 0x111e);
+		ret = mt9m032_write_reg(client, MT9M032_FORMATTER1, 0x111e);
 					/* Set 14-bit mode, select 7 divider */
 
 	return ret;
@@ -469,7 +469,7 @@ static int mt9m032_g_register(struct v4l2_subdev *sd,
 	if (reg->match.addr != client->addr)
 		return -ENODEV;
 
-	val = mt9m032_read_reg(sensor, reg->reg);
+	val = mt9m032_read_reg(client, reg->reg);
 	if (val < 0)
 		return -EIO;
 
@@ -491,10 +491,7 @@ static int mt9m032_s_register(struct v4l2_subdev *sd,
 	if (reg->match.addr != client->addr)
 		return -ENODEV;
 
-	if (mt9m032_write_reg(sensor, reg->reg, reg->val) < 0)
-		return -EIO;
-
-	return 0;
+	return mt9m032_write_reg(client, reg->reg, reg->val);
 }
 #endif
 
@@ -504,12 +501,13 @@ static int mt9m032_s_register(struct v4l2_subdev *sd,
 
 static int update_read_mode2(struct mt9m032 *sensor, bool vflip, bool hflip)
 {
+	struct i2c_client *client = v4l2_get_subdevdata(&sensor->subdev);
 	int reg_val = (!!vflip) << MT9M032_READ_MODE2_VFLIP_SHIFT
 		      | (!!hflip) << MT9M032_READ_MODE2_HFLIP_SHIFT
 		      | MT9M032_READ_MODE2_ROW_BLC
 		      | 0x0007;
 
-	return mt9m032_write_reg(sensor, MT9M032_READ_MODE2, reg_val);
+	return mt9m032_write_reg(client, MT9M032_READ_MODE2, reg_val);
 }
 
 static int mt9m032_set_hflip(struct mt9m032 *sensor, s32 val)
@@ -524,6 +522,7 @@ static int mt9m032_set_vflip(struct mt9m032 *sensor, s32 val)
 
 static int mt9m032_set_exposure(struct mt9m032 *sensor, s32 val)
 {
+	struct i2c_client *client = v4l2_get_subdevdata(&sensor->subdev);
 	int shutter_width;
 	u16 high_val, low_val;
 	int ret;
@@ -534,15 +533,17 @@ static int mt9m032_set_exposure(struct mt9m032 *sensor, s32 val)
 	high_val = (shutter_width >> 16) & 0xf;
 	low_val = shutter_width & 0xffff;
 
-	ret = mt9m032_write_reg(sensor, MT9M032_SHUTTER_WIDTH_HIGH, high_val);
+	ret = mt9m032_write_reg(client, MT9M032_SHUTTER_WIDTH_HIGH, high_val);
 	if (!ret)
-		ret = mt9m032_write_reg(sensor, MT9M032_SHUTTER_WIDTH_LOW, low_val);
+		ret = mt9m032_write_reg(client, MT9M032_SHUTTER_WIDTH_LOW,
+					low_val);
 
 	return ret;
 }
 
 static int mt9m032_set_gain(struct mt9m032 *sensor, s32 val)
 {
+	struct i2c_client *client = v4l2_get_subdevdata(&sensor->subdev);
 	int digital_gain_val;	/* in 1/8th (0..127) */
 	int analog_mul;		/* 0 or 1 */
 	int analog_gain_val;	/* in 1/16th. (0..63) */
@@ -565,7 +566,7 @@ static int mt9m032_set_gain(struct mt9m032 *sensor, s32 val)
 		  | (analog_mul & 1) << MT9M032_GAIN_AMUL_SHIFT
 		  | (analog_gain_val & MT9M032_GAIN_ANALOG_MASK);
 
-	return mt9m032_write_reg(sensor, MT9M032_GAIN_ALL, reg_val);
+	return mt9m032_write_reg(client, MT9M032_GAIN_ALL, reg_val);
 }
 
 static int mt9m032_try_ctrl(struct v4l2_ctrl *ctrl)
@@ -670,7 +671,7 @@ static int mt9m032_probe(struct i2c_client *client,
 	 * the code will need to be extended with the appropriate platform
 	 * callback to setup the clock.
 	 */
-	chip_version = mt9m032_read_reg(sensor, MT9M032_CHIP_VERSION);
+	chip_version = mt9m032_read_reg(client, MT9M032_CHIP_VERSION);
 	if (chip_version == MT9M032_CHIP_VERSION_VALUE) {
 		dev_info(&client->dev, "mt9m032: detected sensor.\n");
 	} else {
@@ -718,10 +719,10 @@ static int mt9m032_probe(struct i2c_client *client,
 	if (ret < 0)
 		goto free_ctrl;
 
-	ret = mt9m032_write_reg(sensor, MT9M032_RESET, 1);	/* reset on */
+	ret = mt9m032_write_reg(client, MT9M032_RESET, 1);	/* reset on */
 	if (ret < 0)
 		goto free_ctrl;
-	mt9m032_write_reg(sensor, MT9M032_RESET, 0);	/* reset off */
+	mt9m032_write_reg(client, MT9M032_RESET, 0);	/* reset off */
 	if (ret < 0)
 		goto free_ctrl;
 
@@ -737,31 +738,32 @@ static int mt9m032_probe(struct i2c_client *client,
 	if (ret < 0)
 		goto free_ctrl;
 
-	ret = mt9m032_write_reg(sensor, 0x41, 0x0000);	/* reserved !!! */
+	ret = mt9m032_write_reg(client, 0x41, 0x0000);	/* reserved !!! */
 	if (ret < 0)
 		goto free_ctrl;
-	ret = mt9m032_write_reg(sensor, 0x42, 0x0003);	/* reserved !!! */
+	ret = mt9m032_write_reg(client, 0x42, 0x0003);	/* reserved !!! */
 	if (ret < 0)
 		goto free_ctrl;
-	ret = mt9m032_write_reg(sensor, 0x43, 0x0003);	/* reserved !!! */
+	ret = mt9m032_write_reg(client, 0x43, 0x0003);	/* reserved !!! */
 	if (ret < 0)
 		goto free_ctrl;
-	ret = mt9m032_write_reg(sensor, 0x7f, 0x0000);	/* reserved !!! */
+	ret = mt9m032_write_reg(client, 0x7f, 0x0000);	/* reserved !!! */
 	if (ret < 0)
 		goto free_ctrl;
 	if (sensor->pdata->invert_pixclock) {
-		mt9m032_write_reg(sensor, MT9M032_PIX_CLK_CTRL, MT9M032_PIX_CLK_CTRL_INV_PIXCLK);
+		mt9m032_write_reg(client, MT9M032_PIX_CLK_CTRL,
+				  MT9M032_PIX_CLK_CTRL_INV_PIXCLK);
 		if (ret < 0)
 			goto free_ctrl;
 	}
 
-	res = mt9m032_read_reg(sensor, MT9M032_PIX_CLK_CTRL);
+	res = mt9m032_read_reg(client, MT9M032_PIX_CLK_CTRL);
 
-	ret = mt9m032_write_reg(sensor, MT9M032_RESTART, 1); /* Restart on */
+	ret = mt9m032_write_reg(client, MT9M032_RESTART, 1); /* Restart on */
 	if (ret < 0)
 		goto free_ctrl;
 	msleep(100);
-	ret = mt9m032_write_reg(sensor, MT9M032_RESTART, 0); /* Restart off */
+	ret = mt9m032_write_reg(client, MT9M032_RESTART, 0); /* Restart off */
 	if (ret < 0)
 		goto free_ctrl;
 	msleep(100);
-- 
1.7.3.4

[PATCH v3 07/10] mt9m032: Put HFLIP and VFLIP controls in a cluster

[Laurent Pinchart]

HFLIP and VFLIP are often set together to rotate the image by 180°.
Putting the controls in a cluster makes sure they will always be applied
together, getting rid of a race condition that could result in one bad
frame.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/video/mt9m032.c |   43 +++++++++++++++++++---------------------
 1 files changed, 20 insertions(+), 23 deletions(-)

diff --git a/drivers/media/video/mt9m032.c b/drivers/media/video/mt9m032.c
index cfed53a..6bd4280 100644
--- a/drivers/media/video/mt9m032.c
+++ b/drivers/media/video/mt9m032.c
@@ -107,7 +107,12 @@ struct mt9m032 {
 	struct v4l2_subdev subdev;
 	struct media_pad pad;
 	struct mt9m032_platform_data *pdata;
+
 	struct v4l2_ctrl_handler ctrls;
+	struct {
+		struct v4l2_ctrl *hflip;
+		struct v4l2_ctrl *vflip;
+	};
 
 	bool streaming;
 
@@ -116,8 +121,6 @@ struct mt9m032 {
 	struct v4l2_mbus_framefmt format;	/* height and width always the same as in crop */
 	struct v4l2_rect crop;
 	struct v4l2_fract frame_interval;
-
-	struct v4l2_ctrl *hflip, *vflip;
 };
 
 #define to_mt9m032(sd)	container_of(sd, struct mt9m032, subdev)
@@ -503,23 +506,13 @@ static int update_read_mode2(struct mt9m032 *sensor, bool vflip, bool hflip)
 {
 	struct i2c_client *client = v4l2_get_subdevdata(&sensor->subdev);
 	int reg_val = (!!vflip) << MT9M032_READ_MODE2_VFLIP_SHIFT
-		      | (!!hflip) << MT9M032_READ_MODE2_HFLIP_SHIFT
-		      | MT9M032_READ_MODE2_ROW_BLC
-		      | 0x0007;
+		    | (!!hflip) << MT9M032_READ_MODE2_HFLIP_SHIFT
+		    | MT9M032_READ_MODE2_ROW_BLC
+		    | 0x0007;
 
 	return mt9m032_write_reg(client, MT9M032_READ_MODE2, reg_val);
 }
 
-static int mt9m032_set_hflip(struct mt9m032 *sensor, s32 val)
-{
-	return update_read_mode2(sensor, sensor->vflip->cur.val, val);
-}
-
-static int mt9m032_set_vflip(struct mt9m032 *sensor, s32 val)
-{
-	return update_read_mode2(sensor, val, sensor->hflip->cur.val);
-}
-
 static int mt9m032_set_exposure(struct mt9m032 *sensor, s32 val)
 {
 	struct i2c_client *client = v4l2_get_subdevdata(&sensor->subdev);
@@ -581,17 +574,17 @@ static int mt9m032_try_ctrl(struct v4l2_ctrl *ctrl)
 
 static int mt9m032_set_ctrl(struct v4l2_ctrl *ctrl)
 {
-	struct mt9m032 *sensor = container_of(ctrl->handler, struct mt9m032, ctrls);
+	struct mt9m032 *sensor =
+		container_of(ctrl->handler, struct mt9m032, ctrls);
 
 	switch (ctrl->id) {
 	case V4L2_CID_GAIN:
 		return mt9m032_set_gain(sensor, ctrl->val);
 
 	case V4L2_CID_HFLIP:
-		return mt9m032_set_hflip(sensor, ctrl->val);
-
 	case V4L2_CID_VFLIP:
-		return mt9m032_set_vflip(sensor, ctrl->val);
+		return update_read_mode2(sensor, sensor->vflip->val,
+					 sensor->hflip->val);
 
 	case V4L2_CID_EXPOSURE:
 		return mt9m032_set_exposure(sensor, ctrl->val);
@@ -700,10 +693,14 @@ static int mt9m032_probe(struct i2c_client *client,
 	v4l2_ctrl_new_std(&sensor->ctrls, &mt9m032_ctrl_ops,
 			  V4L2_CID_GAIN, 0, 127, 1, 64);
 
-	sensor->hflip = v4l2_ctrl_new_std(&sensor->ctrls, &mt9m032_ctrl_ops,
-			  V4L2_CID_HFLIP, 0, 1, 1, 0);
-	sensor->vflip = v4l2_ctrl_new_std(&sensor->ctrls, &mt9m032_ctrl_ops,
-			  V4L2_CID_VFLIP, 0, 1, 1, 0);
+	sensor->hflip = v4l2_ctrl_new_std(&sensor->ctrls,
+					  &mt9m032_ctrl_ops,
+					  V4L2_CID_HFLIP, 0, 1, 1, 0);
+	sensor->vflip = v4l2_ctrl_new_std(&sensor->ctrls,
+					  &mt9m032_ctrl_ops,
+					  V4L2_CID_VFLIP, 0, 1, 1, 0);
+	v4l2_ctrl_cluster(2, &sensor->hflip);
+
 	v4l2_ctrl_new_std(&sensor->ctrls, &mt9m032_ctrl_ops,
 			  V4L2_CID_EXPOSURE, 0, 8000, 1, 1700);    /* 1.7ms */
 
-- 
1.7.3.4

[PATCH v3 08/10] mt9m032: Remove unneeded register read

[Laurent Pinchart]

There's not need to read register MT9M032_READ_MODE1 when setting up the
PLL. Remove the read call.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/video/mt9m032.c |    5 +----
 1 files changed, 1 insertions(+), 4 deletions(-)

diff --git a/drivers/media/video/mt9m032.c b/drivers/media/video/mt9m032.c
index 6bd4280..4cde779 100644
--- a/drivers/media/video/mt9m032.c
+++ b/drivers/media/video/mt9m032.c
@@ -226,7 +226,7 @@ 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;
+	int ret;
 
 	/* TODO: also support other pre-div values */
 	if (pdata->pll_pre_div != 6) {
@@ -253,9 +253,6 @@ static int mt9m032_setup_pll(struct mt9m032 *sensor)
 							/* more reserved, Continuous */
 							/* Master Mode */
 	if (!ret)
-		res = mt9m032_read_reg(client, MT9M032_READ_MODE1);
-
-	if (!ret)
 		ret = mt9m032_write_reg(client, MT9M032_FORMATTER1, 0x111e);
 					/* Set 14-bit mode, select 7 divider */
 
-- 
1.7.3.4

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

[Laurent Pinchart]

Add a generic helper function to compute PLL parameters for PLL found in
several Aptina sensors.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/video/Kconfig      |    3 +
 drivers/media/video/Makefile     |    4 +
 drivers/media/video/aptina-pll.c |  175 ++++++++++++++++++++++++++++++++++++++
 drivers/media/video/aptina-pll.h |   55 ++++++++++++
 4 files changed, 237 insertions(+), 0 deletions(-)
 create mode 100644 drivers/media/video/aptina-pll.c
 create mode 100644 drivers/media/video/aptina-pll.h

diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig
index 80acb78..410baf2 100644
--- a/drivers/media/video/Kconfig
+++ b/drivers/media/video/Kconfig
@@ -459,6 +459,9 @@ config VIDEO_AK881X
 
 comment "Camera sensor devices"
 
+config VIDEO_APTINA_PLL
+	tristate
+
 config VIDEO_OV7670
 	tristate "OmniVision OV7670 sensor support"
 	depends on I2C && VIDEO_V4L2
diff --git a/drivers/media/video/Makefile b/drivers/media/video/Makefile
index 9b19533..8e037e9 100644
--- a/drivers/media/video/Makefile
+++ b/drivers/media/video/Makefile
@@ -22,6 +22,10 @@ endif
 
 obj-$(CONFIG_VIDEO_V4L2_COMMON) += v4l2-common.o
 
+# Helper modules
+
+obj-$(CONFIG_VIDEO_APTINA_PLL) += aptina-pll.o
+
 # All i2c modules must come first:
 
 obj-$(CONFIG_VIDEO_TUNER) += tuner.o
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
@@ -0,0 +1,175 @@
+/*
+ * Aptina Sensor PLL Configuration
+ *
+ * Copyright (C) 2012 Laurent Pinchart <laurent.pinchart@ideasonboard.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
+ * 02110-1301 USA
+ */
+
+#include <linux/device.h>
+#include <linux/gcd.h>
+#include <linux/kernel.h>
+#include <linux/lcm.h>
+#include <linux/module.h>
+
+#include "aptina-pll.h"
+
+int aptina_pll_configure(struct device *dev, struct aptina_pll *pll,
+			 const struct aptina_pll_limits *limits)
+{
+	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;
+	}
+
+	/* Compute the multiplier M and combined N*P1 divisor. */
+	div = gcd(pll->pix_clock, pll->ext_clock);
+	pll->m = pll->pix_clock / div;
+	div = pll->ext_clock / div;
+
+	/* We now have the smallest M and N*P1 values that will result in the
+	 * desired pixel clock frequency, but they might be out of the valid
+	 * range. Compute the factor by which we should multiply them given the
+	 * following constraints:
+	 *
+	 * - minimum/maximum multiplier
+	 * - minimum/maximum multiplier output clock frequency assuming the
+	 *   minimum/maximum N value
+	 * - minimum/maximum combined N*P1 divisor
+	 */
+	mf_min = DIV_ROUND_UP(limits->m_min, pll->m);
+	mf_min = max(mf_min, limits->out_clock_min /
+		     (pll->ext_clock / limits->n_min * pll->m));
+	mf_min = max(mf_min, limits->n_min * limits->p1_min / div);
+	mf_max = limits->m_max / pll->m;
+	mf_max = min(mf_max, limits->out_clock_max /
+		    (pll->ext_clock / limits->n_max * pll->m));
+	mf_max = min(mf_max, DIV_ROUND_UP(limits->n_max * limits->p1_max, div));
+
+	dev_dbg(dev, "pll: mf min %u max %u\n", mf_min, mf_max);
+	if (mf_min > mf_max) {
+		dev_err(dev, "pll: no valid combined N*P1 divisor.\n");
+		return -EINVAL;
+	}
+
+	/*
+	 * We're looking for the highest acceptable P1 value for which a
+	 * multiplier factor MF exists that fulfills the following conditions:
+	 *
+	 * 1. p1 is in the [p1_min, p1_max] range given by the limits and is
+	 *    even
+	 * 2. mf is in the [mf_min, mf_max] range computed above
+	 * 3. div * mf is a multiple of p1, in order to compute
+	 *	n = div * mf / p1
+	 *	m = pll->m * mf
+	 * 4. the internal clock frequency, given by ext_clock / n, is in the
+	 *    [int_clock_min, int_clock_max] range given by the limits
+	 * 5. the output clock frequency, given by ext_clock / n * m, is in the
+	 *    [out_clock_min, out_clock_max] range given by the limits
+	 *
+	 * The first naive approach is to iterate over all p1 values acceptable
+	 * according to (1) and all mf values acceptable according to (2), and
+	 * stop at the first combination that fulfills (3), (4) and (5). This
+	 * has a O(n^2) complexity.
+	 *
+	 * Instead of iterating over all mf values in the [mf_min, mf_max] range
+	 * we can compute the mf increment between two acceptable values
+	 * according to (3) with
+	 *
+	 *	mf_inc = lcm(div, p1) / div			(6)
+	 *
+	 * and round the minimum up to the nearest multiple of mf_inc. This will
+	 * restrict the number of mf values to be checked.
+	 *
+	 * Furthermore, conditions (4) and (5) only restrict the range of
+	 * acceptable p1 and mf values by modifying the minimum and maximum
+	 * limits. (5) can be expressed as
+	 *
+	 *	ext_clock / (div * mf / p1) * m * mf >= out_clock_min
+	 *	ext_clock / (div * mf / p1) * m * mf <= out_clock_max
+	 *
+	 * or
+	 *
+	 *	p1 >= out_clock_min * div / (ext_clock * m)	(7)
+	 *	p1 <= out_clock_max * div / (ext_clock * m)
+	 *
+	 * Similarly, (4) can be expressed as
+	 *
+	 *	mf >= ext_clock * p1 / (int_clock_max * div)	(8)
+	 *	mf <= ext_clock * p1 / (int_clock_min * div)
+	 *
+	 * We can thus iterate over the restricted p1 range defined by the
+	 * combination of (1) and (7), and then compute the restricted mf range
+	 * defined by the combination of (2), (6) and (8). If the resulting mf
+	 * range is not empty, any value in the mf range is acceptable. We thus
+	 * select the mf lwoer bound and the corresponding p1 value.
+	 */
+	if (limits->p1_min == 0) {
+		dev_err(dev, "pll: P1 minimum value must be >0.\n");
+		return -EINVAL;
+	}
+
+	p1_min = max(limits->p1_min, DIV_ROUND_UP(limits->out_clock_min * div,
+		     pll->ext_clock * pll->m));
+	p1_max = min(limits->p1_max, limits->out_clock_max * div /
+		     (pll->ext_clock * pll->m));
+
+	for (p1 = p1_max & ~1; p1 >= p1_min; p1 -= 2) {
+		unsigned int mf_inc = lcm(div, p1) / div;
+		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;
+		}
+	}
+
+	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;
+}
+EXPORT_SYMBOL_GPL(aptina_pll_configure);
+
+MODULE_DESCRIPTION("Aptina PLL Helpers");
+MODULE_AUTHOR("Laurent Pinchart <laurent.pinchart@ideasonboard.com>");
+MODULE_LICENSE("GPL v2");
diff --git a/drivers/media/video/aptina-pll.h b/drivers/media/video/aptina-pll.h
new file mode 100644
index 0000000..36a9363
--- /dev/null
+++ b/drivers/media/video/aptina-pll.h
@@ -0,0 +1,55 @@
+/*
+ * Aptina Sensor PLL Configuration
+ *
+ * Copyright (C) 2012 Laurent Pinchart <laurent.pinchart@ideasonboard.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
+ * 02110-1301 USA
+ */
+
+#ifndef __APTINA_PLL_H
+#define __APTINA_PLL_H
+
+struct aptina_pll {
+	unsigned int ext_clock;
+	unsigned int pix_clock;
+
+	unsigned int n;
+	unsigned int m;
+	unsigned int p1;
+};
+
+struct aptina_pll_limits {
+	unsigned int ext_clock_min;
+	unsigned int ext_clock_max;
+	unsigned int int_clock_min;
+	unsigned int int_clock_max;
+	unsigned int out_clock_min;
+	unsigned int out_clock_max;
+	unsigned int pix_clock_max;
+
+	unsigned int n_min;
+	unsigned int n_max;
+	unsigned int m_min;
+	unsigned int m_max;
+	unsigned int p1_min;
+	unsigned int p1_max;
+};
+
+struct device;
+
+int aptina_pll_configure(struct device *dev, struct aptina_pll *pll,
+			 const struct aptina_pll_limits *limits);
+
+#endif /* __APTINA_PLL_H */
-- 
1.7.3.4

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

[Andy Walls]

On Sat, 2012-03-03 at 16:28 +0100, Laurent Pinchart wrote:
> Add a generic helper function to compute PLL parameters for PLL found in
> several Aptina sensors.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  drivers/media/video/Kconfig      |    3 +
>  drivers/media/video/Makefile     |    4 +
>  drivers/media/video/aptina-pll.c |  175 ++++++++++++++++++++++++++++++++++++++
>  drivers/media/video/aptina-pll.h |   55 ++++++++++++
>  4 files changed, 237 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/media/video/aptina-pll.c
>  create mode 100644 drivers/media/video/aptina-pll.h
> 
> diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig
> index 80acb78..410baf2 100644
> --- a/drivers/media/video/Kconfig
> +++ b/drivers/media/video/Kconfig
> @@ -459,6 +459,9 @@ config VIDEO_AK881X
>  
>  comment "Camera sensor devices"
>  
> +config VIDEO_APTINA_PLL
> +	tristate
> +
>  config VIDEO_OV7670
>  	tristate "OmniVision OV7670 sensor support"
>  	depends on I2C && VIDEO_V4L2
> diff --git a/drivers/media/video/Makefile b/drivers/media/video/Makefile
> index 9b19533..8e037e9 100644
> --- a/drivers/media/video/Makefile
> +++ b/drivers/media/video/Makefile
> @@ -22,6 +22,10 @@ endif
>  
>  obj-$(CONFIG_VIDEO_V4L2_COMMON) += v4l2-common.o
>  
> +# Helper modules
> +
> +obj-$(CONFIG_VIDEO_APTINA_PLL) += aptina-pll.o
> +
>  # All i2c modules must come first:
>  
>  obj-$(CONFIG_VIDEO_TUNER) += tuner.o
> 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
> @@ -0,0 +1,175 @@
> +/*
> + * Aptina Sensor PLL Configuration
> + *
> + * Copyright (C) 2012 Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful, but
> + * WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
> + * 02110-1301 USA
> + */
> +
> +#include <linux/device.h>
> +#include <linux/gcd.h>
> +#include <linux/kernel.h>
> +#include <linux/lcm.h>
> +#include <linux/module.h>
> +
> +#include "aptina-pll.h"
> +
> +int aptina_pll_configure(struct device *dev, struct aptina_pll *pll,
> +			 const struct aptina_pll_limits *limits)
> +{
> +	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;
> +	}
> +
> +	/* Compute the multiplier M and combined N*P1 divisor. */
> +	div = gcd(pll->pix_clock, pll->ext_clock);
> +	pll->m = pll->pix_clock / div;
> +	div = pll->ext_clock / div;
> +
> +	/* We now have the smallest M and N*P1 values that will result in the
> +	 * desired pixel clock frequency, but they might be out of the valid
> +	 * range. Compute the factor by which we should multiply them given the
> +	 * following constraints:
> +	 *
> +	 * - minimum/maximum multiplier
> +	 * - minimum/maximum multiplier output clock frequency assuming the
> +	 *   minimum/maximum N value
> +	 * - minimum/maximum combined N*P1 divisor
> +	 */
> +	mf_min = DIV_ROUND_UP(limits->m_min, pll->m);
> +	mf_min = max(mf_min, limits->out_clock_min /
> +		     (pll->ext_clock / limits->n_min * pll->m));
> +	mf_min = max(mf_min, limits->n_min * limits->p1_min / div);
> +	mf_max = limits->m_max / pll->m;
> +	mf_max = min(mf_max, limits->out_clock_max /
> +		    (pll->ext_clock / limits->n_max * pll->m));
> +	mf_max = min(mf_max, DIV_ROUND_UP(limits->n_max * limits->p1_max, div));
> +
> +	dev_dbg(dev, "pll: mf min %u max %u\n", mf_min, mf_max);
> +	if (mf_min > mf_max) {
> +		dev_err(dev, "pll: no valid combined N*P1 divisor.\n");
> +		return -EINVAL;
> +	}
> +
> +	/*
> +	 * We're looking for the highest acceptable P1 value 

Why the *highest* acceptable post-divide (P1) value?

> for which a
> +	 * multiplier factor MF exists that fulfills the following conditions:
> +	 *
> +	 * 1. p1 is in the [p1_min, p1_max] range given by the limits and is
> +	 *    even
> +	 * 2. mf is in the [mf_min, mf_max] range computed above
> +	 * 3. div * mf is a multiple of p1, in order to compute
> +	 *	n = div * mf / p1
> +	 *	m = pll->m * mf
> +	 * 4. the internal clock frequency, given by ext_clock / n, is in the
> +	 *    [int_clock_min, int_clock_max] range given by the limits
> +	 * 5. the output clock frequency, given by ext_clock / n * m, is in the
> +	 *    [out_clock_min, out_clock_max] range given by the limits
> +	 *

So just to make your constrained optimzation problem even more complex:

I would imagine you would get faster PLL lock and less phase noise by
having the VCO operate near its center frequency.

If you think that is a sensible constraint, then that translates to
having the PLL output before post-divide (i.e. ext_clock / n * m), to be
as close as possible to the center frequency of the VCO (i.e.
(out_clock_max - out_clock_min) / 2 ).

Regards,
Andy


> +	 * The first naive approach is to iterate over all p1 values acceptable
> +	 * according to (1) and all mf values acceptable according to (2), and
> +	 * stop at the first combination that fulfills (3), (4) and (5). This
> +	 * has a O(n^2) complexity.
> +	 *
> +	 * Instead of iterating over all mf values in the [mf_min, mf_max] range
> +	 * we can compute the mf increment between two acceptable values
> +	 * according to (3) with
> +	 *
> +	 *	mf_inc = lcm(div, p1) / div			(6)
> +	 *
> +	 * and round the minimum up to the nearest multiple of mf_inc. This will
> +	 * restrict the number of mf values to be checked.
> +	 *
> +	 * Furthermore, conditions (4) and (5) only restrict the range of
> +	 * acceptable p1 and mf values by modifying the minimum and maximum
> +	 * limits. (5) can be expressed as
> +	 *
> +	 *	ext_clock / (div * mf / p1) * m * mf >= out_clock_min
> +	 *	ext_clock / (div * mf / p1) * m * mf <= out_clock_max
> +	 *
> +	 * or
> +	 *
> +	 *	p1 >= out_clock_min * div / (ext_clock * m)	(7)
> +	 *	p1 <= out_clock_max * div / (ext_clock * m)
> +	 *
> +	 * Similarly, (4) can be expressed as
> +	 *
> +	 *	mf >= ext_clock * p1 / (int_clock_max * div)	(8)
> +	 *	mf <= ext_clock * p1 / (int_clock_min * div)
> +	 *
> +	 * We can thus iterate over the restricted p1 range defined by the
> +	 * combination of (1) and (7), and then compute the restricted mf range
> +	 * defined by the combination of (2), (6) and (8). If the resulting mf
> +	 * range is not empty, any value in the mf range is acceptable. We thus
> +	 * select the mf lwoer bound and the corresponding p1 value.
> +	 */
> +	if (limits->p1_min == 0) {
> +		dev_err(dev, "pll: P1 minimum value must be >0.\n");
> +		return -EINVAL;
> +	}
> +
> +	p1_min = max(limits->p1_min, DIV_ROUND_UP(limits->out_clock_min * div,
> +		     pll->ext_clock * pll->m));
> +	p1_max = min(limits->p1_max, limits->out_clock_max * div /
> +		     (pll->ext_clock * pll->m));
> +
> +	for (p1 = p1_max & ~1; p1 >= p1_min; p1 -= 2) {
> +		unsigned int mf_inc = lcm(div, p1) / div;
> +		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;
> +		}
> +	}
> +
> +	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;
> +}
> +EXPORT_SYMBOL_GPL(aptina_pll_configure);
> +
> +MODULE_DESCRIPTION("Aptina PLL Helpers");
> +MODULE_AUTHOR("Laurent Pinchart <laurent.pinchart@ideasonboard.com>");
> +MODULE_LICENSE("GPL v2");
> diff --git a/drivers/media/video/aptina-pll.h b/drivers/media/video/aptina-pll.h
> new file mode 100644
> index 0000000..36a9363
> --- /dev/null
> +++ b/drivers/media/video/aptina-pll.h
> @@ -0,0 +1,55 @@
> +/*
> + * Aptina Sensor PLL Configuration
> + *
> + * Copyright (C) 2012 Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful, but
> + * WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
> + * 02110-1301 USA
> + */
> +
> +#ifndef __APTINA_PLL_H
> +#define __APTINA_PLL_H
> +
> +struct aptina_pll {
> +	unsigned int ext_clock;
> +	unsigned int pix_clock;
> +
> +	unsigned int n;
> +	unsigned int m;
> +	unsigned int p1;
> +};
> +
> +struct aptina_pll_limits {
> +	unsigned int ext_clock_min;
> +	unsigned int ext_clock_max;
> +	unsigned int int_clock_min;
> +	unsigned int int_clock_max;
> +	unsigned int out_clock_min;
> +	unsigned int out_clock_max;
> +	unsigned int pix_clock_max;
> +
> +	unsigned int n_min;
> +	unsigned int n_max;
> +	unsigned int m_min;
> +	unsigned int m_max;
> +	unsigned int p1_min;
> +	unsigned int p1_max;
> +};
> +
> +struct device;
> +
> +int aptina_pll_configure(struct device *dev, struct aptina_pll *pll,
> +			 const struct aptina_pll_limits *limits);
> +
> +#endif /* __APTINA_PLL_H */

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

[Andy Walls]

On Sat, 2012-03-03 at 12:35 -0500, Andy Walls wrote:
> On Sat, 2012-03-03 at 16:28 +0100, Laurent Pinchart wrote:
> > Add a generic helper function to compute PLL parameters for PLL found in
> > several Aptina sensors.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >  drivers/media/video/Kconfig      |    3 +
> >  drivers/media/video/Makefile     |    4 +
> >  drivers/media/video/aptina-pll.c |  175 ++++++++++++++++++++++++++++++++++++++
> >  drivers/media/video/aptina-pll.h |   55 ++++++++++++
> >  4 files changed, 237 insertions(+), 0 deletions(-)
> >  create mode 100644 drivers/media/video/aptina-pll.c
> >  create mode 100644 drivers/media/video/aptina-pll.h
> > 
> > diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig
> > index 80acb78..410baf2 100644
> > --- a/drivers/media/video/Kconfig
> > +++ b/drivers/media/video/Kconfig
> > @@ -459,6 +459,9 @@ config VIDEO_AK881X
> >  
> >  comment "Camera sensor devices"
> >  
> > +config VIDEO_APTINA_PLL
> > +	tristate
> > +
> >  config VIDEO_OV7670
> >  	tristate "OmniVision OV7670 sensor support"
> >  	depends on I2C && VIDEO_V4L2
> > diff --git a/drivers/media/video/Makefile b/drivers/media/video/Makefile
> > index 9b19533..8e037e9 100644
> > --- a/drivers/media/video/Makefile
> > +++ b/drivers/media/video/Makefile
> > @@ -22,6 +22,10 @@ endif
> >  
> >  obj-$(CONFIG_VIDEO_V4L2_COMMON) += v4l2-common.o
> >  
> > +# Helper modules
> > +
> > +obj-$(CONFIG_VIDEO_APTINA_PLL) += aptina-pll.o
> > +
> >  # All i2c modules must come first:
> >  
> >  obj-$(CONFIG_VIDEO_TUNER) += tuner.o
> > 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
> > @@ -0,0 +1,175 @@
> > +/*
> > + * Aptina Sensor PLL Configuration
> > + *
> > + * Copyright (C) 2012 Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License
> > + * version 2 as published by the Free Software Foundation.
> > + *
> > + * This program is distributed in the hope that it will be useful, but
> > + * WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> > + * General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU General Public License
> > + * along with this program; if not, write to the Free Software
> > + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
> > + * 02110-1301 USA
> > + */
> > +
> > +#include <linux/device.h>
> > +#include <linux/gcd.h>
> > +#include <linux/kernel.h>
> > +#include <linux/lcm.h>
> > +#include <linux/module.h>
> > +
> > +#include "aptina-pll.h"
> > +
> > +int aptina_pll_configure(struct device *dev, struct aptina_pll *pll,
> > +			 const struct aptina_pll_limits *limits)
> > +{
> > +	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;
> > +	}
> > +
> > +	/* Compute the multiplier M and combined N*P1 divisor. */
> > +	div = gcd(pll->pix_clock, pll->ext_clock);
> > +	pll->m = pll->pix_clock / div;
> > +	div = pll->ext_clock / div;
> > +
> > +	/* We now have the smallest M and N*P1 values that will result in the
> > +	 * desired pixel clock frequency, but they might be out of the valid
> > +	 * range. Compute the factor by which we should multiply them given the
> > +	 * following constraints:
> > +	 *
> > +	 * - minimum/maximum multiplier
> > +	 * - minimum/maximum multiplier output clock frequency assuming the
> > +	 *   minimum/maximum N value
> > +	 * - minimum/maximum combined N*P1 divisor
> > +	 */
> > +	mf_min = DIV_ROUND_UP(limits->m_min, pll->m);
> > +	mf_min = max(mf_min, limits->out_clock_min /
> > +		     (pll->ext_clock / limits->n_min * pll->m));
> > +	mf_min = max(mf_min, limits->n_min * limits->p1_min / div);
> > +	mf_max = limits->m_max / pll->m;
> > +	mf_max = min(mf_max, limits->out_clock_max /
> > +		    (pll->ext_clock / limits->n_max * pll->m));
> > +	mf_max = min(mf_max, DIV_ROUND_UP(limits->n_max * limits->p1_max, div));
> > +
> > +	dev_dbg(dev, "pll: mf min %u max %u\n", mf_min, mf_max);
> > +	if (mf_min > mf_max) {
> > +		dev_err(dev, "pll: no valid combined N*P1 divisor.\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	/*
> > +	 * We're looking for the highest acceptable P1 value 
> 
> Why the *highest* acceptable post-divide (P1) value?
> 
> > for which a
> > +	 * multiplier factor MF exists that fulfills the following conditions:
> > +	 *
> > +	 * 1. p1 is in the [p1_min, p1_max] range given by the limits and is
> > +	 *    even
> > +	 * 2. mf is in the [mf_min, mf_max] range computed above
> > +	 * 3. div * mf is a multiple of p1, in order to compute
> > +	 *	n = div * mf / p1
> > +	 *	m = pll->m * mf
> > +	 * 4. the internal clock frequency, given by ext_clock / n, is in the
> > +	 *    [int_clock_min, int_clock_max] range given by the limits
> > +	 * 5. the output clock frequency, given by ext_clock / n * m, is in the
> > +	 *    [out_clock_min, out_clock_max] range given by the limits
> > +	 *
> 
> So just to make your constrained optimzation problem even more complex:
> 
> I would imagine you would get faster PLL lock and less phase noise by
> having the VCO operate near its center frequency.
> 
> If you think that is a sensible constraint, then that translates to
> having the PLL output before post-divide (i.e. ext_clock / n * m), to be
> as close as possible to the center frequency of the VCO (i.e.
> (out_clock_max - out_clock_min) / 2 ).

Oops, I hate math in public.  :P

I obviously meant

	(out_clock_max + out_clock_min) / 2

for the center frequency of the VCO.

Regards,
Andy

> Regards,
> Andy
> 
> 
> > +	 * The first naive approach is to iterate over all p1 values acceptable
> > +	 * according to (1) and all mf values acceptable according to (2), and
> > +	 * stop at the first combination that fulfills (3), (4) and (5). This
> > +	 * has a O(n^2) complexity.
> > +	 *
> > +	 * Instead of iterating over all mf values in the [mf_min, mf_max] range
> > +	 * we can compute the mf increment between two acceptable values
> > +	 * according to (3) with
> > +	 *
> > +	 *	mf_inc = lcm(div, p1) / div			(6)
> > +	 *
> > +	 * and round the minimum up to the nearest multiple of mf_inc. This will
> > +	 * restrict the number of mf values to be checked.
> > +	 *
> > +	 * Furthermore, conditions (4) and (5) only restrict the range of
> > +	 * acceptable p1 and mf values by modifying the minimum and maximum
> > +	 * limits. (5) can be expressed as
> > +	 *
> > +	 *	ext_clock / (div * mf / p1) * m * mf >= out_clock_min
> > +	 *	ext_clock / (div * mf / p1) * m * mf <= out_clock_max
> > +	 *
> > +	 * or
> > +	 *
> > +	 *	p1 >= out_clock_min * div / (ext_clock * m)	(7)
> > +	 *	p1 <= out_clock_max * div / (ext_clock * m)
> > +	 *
> > +	 * Similarly, (4) can be expressed as
> > +	 *
> > +	 *	mf >= ext_clock * p1 / (int_clock_max * div)	(8)
> > +	 *	mf <= ext_clock * p1 / (int_clock_min * div)
> > +	 *
> > +	 * We can thus iterate over the restricted p1 range defined by the
> > +	 * combination of (1) and (7), and then compute the restricted mf range
> > +	 * defined by the combination of (2), (6) and (8). If the resulting mf
> > +	 * range is not empty, any value in the mf range is acceptable. We thus
> > +	 * select the mf lwoer bound and the corresponding p1 value.
> > +	 */
> > +	if (limits->p1_min == 0) {
> > +		dev_err(dev, "pll: P1 minimum value must be >0.\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	p1_min = max(limits->p1_min, DIV_ROUND_UP(limits->out_clock_min * div,
> > +		     pll->ext_clock * pll->m));
> > +	p1_max = min(limits->p1_max, limits->out_clock_max * div /
> > +		     (pll->ext_clock * pll->m));
> > +
> > +	for (p1 = p1_max & ~1; p1 >= p1_min; p1 -= 2) {
> > +		unsigned int mf_inc = lcm(div, p1) / div;
> > +		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;
> > +		}
> > +	}
> > +
> > +	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;
> > +}
> > +EXPORT_SYMBOL_GPL(aptina_pll_configure);
> > +
> > +MODULE_DESCRIPTION("Aptina PLL Helpers");
> > +MODULE_AUTHOR("Laurent Pinchart <laurent.pinchart@ideasonboard.com>");
> > +MODULE_LICENSE("GPL v2");
> > diff --git a/drivers/media/video/aptina-pll.h b/drivers/media/video/aptina-pll.h
> > new file mode 100644
> > index 0000000..36a9363
> > --- /dev/null
> > +++ b/drivers/media/video/aptina-pll.h
> > @@ -0,0 +1,55 @@
> > +/*
> > + * Aptina Sensor PLL Configuration
> > + *
> > + * Copyright (C) 2012 Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License
> > + * version 2 as published by the Free Software Foundation.
> > + *
> > + * This program is distributed in the hope that it will be useful, but
> > + * WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> > + * General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU General Public License
> > + * along with this program; if not, write to the Free Software
> > + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
> > + * 02110-1301 USA
> > + */
> > +
> > +#ifndef __APTINA_PLL_H
> > +#define __APTINA_PLL_H
> > +
> > +struct aptina_pll {
> > +	unsigned int ext_clock;
> > +	unsigned int pix_clock;
> > +
> > +	unsigned int n;
> > +	unsigned int m;
> > +	unsigned int p1;
> > +};
> > +
> > +struct aptina_pll_limits {
> > +	unsigned int ext_clock_min;
> > +	unsigned int ext_clock_max;
> > +	unsigned int int_clock_min;
> > +	unsigned int int_clock_max;
> > +	unsigned int out_clock_min;
> > +	unsigned int out_clock_max;
> > +	unsigned int pix_clock_max;
> > +
> > +	unsigned int n_min;
> > +	unsigned int n_max;
> > +	unsigned int m_min;
> > +	unsigned int m_max;
> > +	unsigned int p1_min;
> > +	unsigned int p1_max;
> > +};
> > +
> > +struct device;
> > +
> > +int aptina_pll_configure(struct device *dev, struct aptina_pll *pll,
> > +			 const struct aptina_pll_limits *limits);
> > +
> > +#endif /* __APTINA_PLL_H */
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Laurent Pinchart]

Hi Andy,

Thanks for the review.

On Saturday 03 March 2012 12:35:09 Andy Walls wrote:
> On Sat, 2012-03-03 at 16:28 +0100, Laurent Pinchart wrote:
> > Add a generic helper function to compute PLL parameters for PLL found in
> > several Aptina sensors.

[snip]

> > +int aptina_pll_configure(struct device *dev, struct aptina_pll *pll,
> > +			 const struct aptina_pll_limits *limits)
> > +{
> > +	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;
> > +	}
> > +
> > +	/* Compute the multiplier M and combined N*P1 divisor. */
> > +	div = gcd(pll->pix_clock, pll->ext_clock);
> > +	pll->m = pll->pix_clock / div;
> > +	div = pll->ext_clock / div;
> > +
> > +	/* We now have the smallest M and N*P1 values that will result in the
> > +	 * desired pixel clock frequency, but they might be out of the valid
> > +	 * range. Compute the factor by which we should multiply them given
> > +	 * the following constraints:
> > +	 *
> > +	 * - minimum/maximum multiplier
> > +	 * - minimum/maximum multiplier output clock frequency assuming the
> > +	 *   minimum/maximum N value
> > +	 * - minimum/maximum combined N*P1 divisor
> > +	 */
> > +	mf_min = DIV_ROUND_UP(limits->m_min, pll->m);
> > +	mf_min = max(mf_min, limits->out_clock_min /
> > +		     (pll->ext_clock / limits->n_min * pll->m));
> > +	mf_min = max(mf_min, limits->n_min * limits->p1_min / div);
> > +	mf_max = limits->m_max / pll->m;
> > +	mf_max = min(mf_max, limits->out_clock_max /
> > +		    (pll->ext_clock / limits->n_max * pll->m));
> > +	mf_max = min(mf_max, DIV_ROUND_UP(limits->n_max * limits->p1_max, 
div));
> > +
> > +	dev_dbg(dev, "pll: mf min %u max %u\n", mf_min, mf_max);
> > +	if (mf_min > mf_max) {
> > +		dev_err(dev, "pll: no valid combined N*P1 divisor.\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	/*
> > +	 * We're looking for the highest acceptable P1 value
> 
> Why the *highest* acceptable post-divide (P1) value?

According to the Aptina datasheets, "it is desirable to keep (fEXTCLK / n) as 
large as possible within the limits".

> > for which a
> > +	 * multiplier factor MF exists that fulfills the following 
conditions:
> > +	 *
> > +	 * 1. p1 is in the [p1_min, p1_max] range given by the limits and is
> > +	 *    even
> > +	 * 2. mf is in the [mf_min, mf_max] range computed above
> > +	 * 3. div * mf is a multiple of p1, in order to compute
> > +	 *	n = div * mf / p1
> > +	 *	m = pll->m * mf
> > +	 * 4. the internal clock frequency, given by ext_clock / n, is in the
> > +	 *    [int_clock_min, int_clock_max] range given by the limits
> > +	 * 5. the output clock frequency, given by ext_clock / n * m, is in 
the
> > +	 *    [out_clock_min, out_clock_max] range given by the limits
> > +	 *
> 
> So just to make your constrained optimzation problem even more complex:
> 
> I would imagine you would get faster PLL lock and less phase noise by
> having the VCO operate near its center frequency.
> 
> If you think that is a sensible constraint, then that translates to
> having the PLL output before post-divide (i.e. ext_clock / n * m), to be
> as close as possible to the center frequency of the VCO (i.e.
> (out_clock_max - out_clock_min) / 2 ).

Good point. But... *ouch* :-)

Do you think it's worth it ?

-- 
Regards,

Laurent Pinchart

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

[Andy Walls]

On Sun, 2012-03-04 at 11:20 +0100, Laurent Pinchart wrote:
> Hi Andy,
> 
> Thanks for the review.
> 
> On Saturday 03 March 2012 12:35:09 Andy Walls wrote:
> > On Sat, 2012-03-03 at 16:28 +0100, Laurent Pinchart wrote:
> > > Add a generic helper function to compute PLL parameters for PLL found in
> > > several Aptina sensors.
> 
> [snip]
> 
> > > +int aptina_pll_configure(struct device *dev, struct aptina_pll *pll,
> > > +			 const struct aptina_pll_limits *limits)
> > > +{
> > > +	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;
> > > +	}
> > > +
> > > +	/* Compute the multiplier M and combined N*P1 divisor. */
> > > +	div = gcd(pll->pix_clock, pll->ext_clock);
> > > +	pll->m = pll->pix_clock / div;
> > > +	div = pll->ext_clock / div;
> > > +
> > > +	/* We now have the smallest M and N*P1 values that will result in the
> > > +	 * desired pixel clock frequency, but they might be out of the valid
> > > +	 * range. Compute the factor by which we should multiply them given
> > > +	 * the following constraints:
> > > +	 *
> > > +	 * - minimum/maximum multiplier
> > > +	 * - minimum/maximum multiplier output clock frequency assuming the
> > > +	 *   minimum/maximum N value
> > > +	 * - minimum/maximum combined N*P1 divisor
> > > +	 */
> > > +	mf_min = DIV_ROUND_UP(limits->m_min, pll->m);
> > > +	mf_min = max(mf_min, limits->out_clock_min /
> > > +		     (pll->ext_clock / limits->n_min * pll->m));
> > > +	mf_min = max(mf_min, limits->n_min * limits->p1_min / div);
> > > +	mf_max = limits->m_max / pll->m;
> > > +	mf_max = min(mf_max, limits->out_clock_max /
> > > +		    (pll->ext_clock / limits->n_max * pll->m));
> > > +	mf_max = min(mf_max, DIV_ROUND_UP(limits->n_max * limits->p1_max, 
> div));
> > > +
> > > +	dev_dbg(dev, "pll: mf min %u max %u\n", mf_min, mf_max);
> > > +	if (mf_min > mf_max) {
> > > +		dev_err(dev, "pll: no valid combined N*P1 divisor.\n");
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	/*
> > > +	 * We're looking for the highest acceptable P1 value
> > 
> > Why the *highest* acceptable post-divide (P1) value?
> 
> According to the Aptina datasheets, "it is desirable to keep (fEXTCLK / n) as 
> large as possible within the limits".

OK.  I'm looking ath the MT9P031 datasheet now.

The PLL implemented looks like a Classical Digital PLL (DPLL) with
- a Phase/Frequency Detector (PFD) phase detector
- a prescaler (divide by n) of the external reference (ext_clock) in
front of of the PFD
- a post-divider (divide by p1) for dividing the VCO's operating
frequency down (out_clock) for use as an output
- a divider in the feedback loop to multiply (by m) the locked operating
frequency of the VCO compared to the prescaled ext_clock (ext_clock/n)


Aptina's recommendation to keep ext_clock/n as large as possible for
best performance makes sense intuitively: more frequent phase error
measurements probably leads to better phase tracking.  


Since
	pix_clock = ext_clock / n * m / p1
 
I guess the objective is really to minimize m/p1 in order to meet the
recommendation.

> > for which a
> > > +	 * multiplier factor MF exists that fulfills the following 
> conditions:
> > > +	 *
> > > +	 * 1. p1 is in the [p1_min, p1_max] range given by the limits and is
> > > +	 *    even
> > > +	 * 2. mf is in the [mf_min, mf_max] range computed above
> > > +	 * 3. div * mf is a multiple of p1, in order to compute
> > > +	 *	n = div * mf / p1
> > > +	 *	m = pll->m * mf
> > > +	 * 4. the internal clock frequency, given by ext_clock / n, is in the
> > > +	 *    [int_clock_min, int_clock_max] range given by the limits
> > > +	 * 5. the output clock frequency, given by ext_clock / n * m, is in 
> the
> > > +	 *    [out_clock_min, out_clock_max] range given by the limits
> > > +	 *
> > 
> > So just to make your constrained optimzation problem even more complex:
> > 
> > I would imagine you would get faster PLL lock and less phase noise by
> > having the VCO operate near its center frequency.
> > 
> > If you think that is a sensible constraint, then that translates to
> > having the PLL output before post-divide (i.e. ext_clock / n * m), to be
> > as close as possible to the center frequency of the VCO (i.e.
> > (out_clock_max - out_clock_min) / 2 ).
> 
> Good point. But... *ouch* :-)

Sorry.  However, upon more research and reflection, I'll revise my
suggestion to be: set m as close as possible to m_mean =
sqrt(m_min*m_max), as that may be optimal for the PLL's operation.  (See
below).

> Do you think it's worth it ?

Well, that depends on the chip design details, but I'm guessing the
answer is "No". ;)

For frequency synthesis, one normally doesn't worry much about phase
noise, unless clock jitter will somehow adversely affect the results of
the processing downstream.

For frequency sythesis, one normally does care about DPLL response to
changes in the external frequency or the programmed frequency, in order
to quickly lock-in or pull-in to a change.  That may not matter for this
sensor, if the DPLL and external clock are only set up at startup, or
very infrequently.


Above I suggested you might try to pick m as close to m_mean =
sqrt(m_min * m_max) as practical.  In the absence of detailed design
information, I will guess that is where the DPLL's dynamic response is
"optimal".  

The damping factor, zeta, of the phase transfer function of the PLL, is
inversely proportional to 1/sqrt(m).  So zeta_min = k/sqrt(m_max) and
zeta_max = k/sqrt(m_min).  A DPLL with zeta == 1 is critically damped.
As zeta becomes < 1, the transient response of the PLL to a change
becomes increasingly oscillatory.  As zeta grows > 1, the response of
the PLL to a change becomes increasingly overdamped (sluggish).

A zeta = 1/sqrt(2) ~= 0.7071 provides an optimally flat, phase transfer
function response for the PLL.  (According to the textbook "Phase-Locked
Loops: Theory, Design, and Applications, Second Edition" by Roland E.
Best, 1993).  I will simply guess that the DPLLs in the Aptina devices
were designed with zeta_mean = k/sqrt(m_mean) = 0.7071.

For the MT9P031, m can range from [16, 256), so m_mean = 64. Assuming
zeta_mean = 0.7071, then zeta_min = 0.3536 and zeta_max = 1.414.

>From here, one could go on, making some assumptions about the data in
the Aptina datasheet and compute the lock-range of the PLL.  The
lock-range is the range of frequencies around the VCO center freq where
the PLL will lock to the reference freq within the time of 1 beat note
between ext_clock/n and out_clock (i.e. very fast).  Note that the
lock-range around the VCO center frequency increases proportionally to
1/m.

Anyway, I've rambled enough.

Regards,
Andy

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

[Laurent Pinchart]

Hi Andy,

On Sunday 04 March 2012 21:38:54 Andy Walls wrote:
> On Sun, 2012-03-04 at 11:20 +0100, Laurent Pinchart wrote:
> > On Saturday 03 March 2012 12:35:09 Andy Walls wrote:
> > > On Sat, 2012-03-03 at 16:28 +0100, Laurent Pinchart wrote:
> > > > Add a generic helper function to compute PLL parameters for PLL found
> > > > in
> > > > several Aptina sensors.
> > 
> > [snip]
> > 
> > > > +int aptina_pll_configure(struct device *dev, struct aptina_pll *pll,
> > > > +			 const struct aptina_pll_limits *limits)
> > > > +{
> > > > +	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;
> > > > +	}
> > > > +
> > > > +	/* Compute the multiplier M and combined N*P1 divisor. */
> > > > +	div = gcd(pll->pix_clock, pll->ext_clock);
> > > > +	pll->m = pll->pix_clock / div;
> > > > +	div = pll->ext_clock / div;
> > > > +
> > > > +	/* We now have the smallest M and N*P1 values that will result in
> > > > the
> > > > +	 * desired pixel clock frequency, but they might be out of the valid
> > > > +	 * range. Compute the factor by which we should multiply them given
> > > > +	 * the following constraints:
> > > > +	 *
> > > > +	 * - minimum/maximum multiplier
> > > > +	 * - minimum/maximum multiplier output clock frequency assuming the
> > > > +	 *   minimum/maximum N value
> > > > +	 * - minimum/maximum combined N*P1 divisor
> > > > +	 */
> > > > +	mf_min = DIV_ROUND_UP(limits->m_min, pll->m);
> > > > +	mf_min = max(mf_min, limits->out_clock_min /
> > > > +		     (pll->ext_clock / limits->n_min * pll->m));
> > > > +	mf_min = max(mf_min, limits->n_min * limits->p1_min / div);
> > > > +	mf_max = limits->m_max / pll->m;
> > > > +	mf_max = min(mf_max, limits->out_clock_max /
> > > > +		    (pll->ext_clock / limits->n_max * pll->m));
> > > > +	mf_max = min(mf_max, DIV_ROUND_UP(limits->n_max * limits->p1_max,
> > 
> > div));
> > 
> > > > +
> > > > +	dev_dbg(dev, "pll: mf min %u max %u\n", mf_min, mf_max);
> > > > +	if (mf_min > mf_max) {
> > > > +		dev_err(dev, "pll: no valid combined N*P1 divisor.\n");
> > > > +		return -EINVAL;
> > > > +	}
> > > > +
> > > > +	/*
> > > > +	 * We're looking for the highest acceptable P1 value
> > > 
> > > Why the *highest* acceptable post-divide (P1) value?
> > 
> > According to the Aptina datasheets, "it is desirable to keep (fEXTCLK / n)
> > as large as possible within the limits".
> 
> OK.  I'm looking ath the MT9P031 datasheet now.
> 
> The PLL implemented looks like a Classical Digital PLL (DPLL) with
> - a Phase/Frequency Detector (PFD) phase detector
> - a prescaler (divide by n) of the external reference (ext_clock) in
> front of of the PFD
> - a post-divider (divide by p1) for dividing the VCO's operating
> frequency down (out_clock) for use as an output
> - a divider in the feedback loop to multiply (by m) the locked operating
> frequency of the VCO compared to the prescaled ext_clock (ext_clock/n)
> 
> Aptina's recommendation to keep ext_clock/n as large as possible for
> best performance makes sense intuitively: more frequent phase error
> measurements probably leads to better phase tracking.
> 
> Since
> 	pix_clock = ext_clock / n * m / p1
> 
> I guess the objective is really to minimize m/p1 in order to meet the
> recommendation.

I agree.

> > > for which a
> > > 
> > > > +	 * multiplier factor MF exists that fulfills the following
> > 
> > conditions:
> > > > +	 *
> > > > +	 * 1. p1 is in the [p1_min, p1_max] range given by the limits and is
> > > > +	 *    even
> > > > +	 * 2. mf is in the [mf_min, mf_max] range computed above
> > > > +	 * 3. div * mf is a multiple of p1, in order to compute
> > > > +	 *	n = div * mf / p1
> > > > +	 *	m = pll->m * mf
> > > > +	 * 4. the internal clock frequency, given by ext_clock / n, is in
> > > > the
> > > > +	 *    [int_clock_min, int_clock_max] range given by the limits
> > > > +	 * 5. the output clock frequency, given by ext_clock / n * m, is in
> > 
> > the
> > 
> > > > +	 *    [out_clock_min, out_clock_max] range given by the limits
> > > > +	 *
> > > 
> > > So just to make your constrained optimzation problem even more complex:
> > > 
> > > I would imagine you would get faster PLL lock and less phase noise by
> > > having the VCO operate near its center frequency.
> > > 
> > > If you think that is a sensible constraint, then that translates to
> > > having the PLL output before post-divide (i.e. ext_clock / n * m), to be
> > > as close as possible to the center frequency of the VCO (i.e.
> > > (out_clock_max - out_clock_min) / 2 ).
> > 
> > Good point. But... *ouch* :-)
> 
> Sorry.  However, upon more research and reflection, I'll revise my
> suggestion to be: set m as close as possible to m_mean =
> sqrt(m_min*m_max), as that may be optimal for the PLL's operation.  (See
> below).
> 
> > Do you think it's worth it ?
> 
> Well, that depends on the chip design details, but I'm guessing the
> answer is "No". ;)

I really like that answer :-)

> For frequency synthesis, one normally doesn't worry much about phase
> noise, unless clock jitter will somehow adversely affect the results of
> the processing downstream.
> 
> For frequency sythesis, one normally does care about DPLL response to
> changes in the external frequency or the programmed frequency, in order
> to quickly lock-in or pull-in to a change.  That may not matter for this
> sensor, if the DPLL and external clock are only set up at startup, or
> very infrequently.

Yes, that's the case here. The external frequency is fixed (drift/jitter of 
course come into play), and the PLL configuration isn't changed at runtime.

> Above I suggested you might try to pick m as close to m_mean =
> sqrt(m_min * m_max) as practical.  In the absence of detailed design
> information, I will guess that is where the DPLL's dynamic response is
> "optimal".
> 
> The damping factor, zeta, of the phase transfer function of the PLL, is
> inversely proportional to 1/sqrt(m).  So zeta_min = k/sqrt(m_max) and
> zeta_max = k/sqrt(m_min).  A DPLL with zeta == 1 is critically damped.
> As zeta becomes < 1, the transient response of the PLL to a change
> becomes increasingly oscillatory.  As zeta grows > 1, the response of
> the PLL to a change becomes increasingly overdamped (sluggish).
> 
> A zeta = 1/sqrt(2) ~= 0.7071 provides an optimally flat, phase transfer
> function response for the PLL.  (According to the textbook "Phase-Locked
> Loops: Theory, Design, and Applications, Second Edition" by Roland E.
> Best, 1993).  I will simply guess that the DPLLs in the Aptina devices
> were designed with zeta_mean = k/sqrt(m_mean) = 0.7071.
> 
> For the MT9P031, m can range from [16, 256), so m_mean = 64. Assuming
> zeta_mean = 0.7071, then zeta_min = 0.3536 and zeta_max = 1.414.
> 
> From here, one could go on, making some assumptions about the data in
> the Aptina datasheet and compute the lock-range of the PLL.  The
> lock-range is the range of frequencies around the VCO center freq where
> the PLL will lock to the reference freq within the time of 1 beat note
> between ext_clock/n and out_clock (i.e. very fast).  Note that the
> lock-range around the VCO center frequency increases proportionally to
> 1/m.
> 
> Anyway, I've rambled enough.

Thanks for the rambling, it's very informative. It reminded me of my telecom 
classes. PLL was actually my oral exam question :-) Only analog PLLs were part 
of the class though.

-- 
Regards,

Laurent Pinchart

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

[Sakari Ailus]

Hi Laurent,

Thanks for the patch!

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.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  drivers/media/video/Kconfig      |    3 +
>  drivers/media/video/Makefile     |    4 +
>  drivers/media/video/aptina-pll.c |  175 ++++++++++++++++++++++++++++++++++++++
>  drivers/media/video/aptina-pll.h |   55 ++++++++++++
>  4 files changed, 237 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/media/video/aptina-pll.c
>  create mode 100644 drivers/media/video/aptina-pll.h
> 
> diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig
> index 80acb78..410baf2 100644
> --- a/drivers/media/video/Kconfig
> +++ b/drivers/media/video/Kconfig
> @@ -459,6 +459,9 @@ config VIDEO_AK881X
>  
>  comment "Camera sensor devices"
>  
> +config VIDEO_APTINA_PLL
> +	tristate
> +

That's a much better place for this.

>  config VIDEO_OV7670
>  	tristate "OmniVision OV7670 sensor support"
>  	depends on I2C && VIDEO_V4L2
> diff --git a/drivers/media/video/Makefile b/drivers/media/video/Makefile
> index 9b19533..8e037e9 100644
> --- a/drivers/media/video/Makefile
> +++ b/drivers/media/video/Makefile
> @@ -22,6 +22,10 @@ endif
>  
>  obj-$(CONFIG_VIDEO_V4L2_COMMON) += v4l2-common.o
>  
> +# Helper modules
> +
> +obj-$(CONFIG_VIDEO_APTINA_PLL) += aptina-pll.o
> +
>  # All i2c modules must come first:
>  
>  obj-$(CONFIG_VIDEO_TUNER) += tuner.o
> 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
> @@ -0,0 +1,175 @@
> +/*
> + * Aptina Sensor PLL Configuration
> + *
> + * Copyright (C) 2012 Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful, but
> + * WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
> + * 02110-1301 USA
> + */
> +
> +#include <linux/device.h>
> +#include <linux/gcd.h>
> +#include <linux/kernel.h>
> +#include <linux/lcm.h>
> +#include <linux/module.h>
> +
> +#include "aptina-pll.h"
> +
> +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?

I call the function smiapp_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.

> +	/* Compute the multiplier M and combined N*P1 divisor. */
> +	div = gcd(pll->pix_clock, pll->ext_clock);
> +	pll->m = pll->pix_clock / div;
> +	div = pll->ext_clock / div;
> +
> +	/* We now have the smallest M and N*P1 values that will result in the
> +	 * desired pixel clock frequency, but they might be out of the valid
> +	 * range. Compute the factor by which we should multiply them given the
> +	 * following constraints:
> +	 *
> +	 * - minimum/maximum multiplier
> +	 * - minimum/maximum multiplier output clock frequency assuming the
> +	 *   minimum/maximum N value
> +	 * - minimum/maximum combined N*P1 divisor
> +	 */
> +	mf_min = DIV_ROUND_UP(limits->m_min, pll->m);
> +	mf_min = max(mf_min, limits->out_clock_min /
> +		     (pll->ext_clock / limits->n_min * pll->m));
> +	mf_min = max(mf_min, limits->n_min * limits->p1_min / div);
> +	mf_max = limits->m_max / pll->m;
> +	mf_max = min(mf_max, limits->out_clock_max /
> +		    (pll->ext_clock / limits->n_max * pll->m));
> +	mf_max = min(mf_max, DIV_ROUND_UP(limits->n_max * limits->p1_max, div));
> +
> +	dev_dbg(dev, "pll: mf min %u max %u\n", mf_min, mf_max);
> +	if (mf_min > mf_max) {
> +		dev_err(dev, "pll: no valid combined N*P1 divisor.\n");
> +		return -EINVAL;
> +	}
> +
> +	/*
> +	 * We're looking for the highest acceptable P1 value for which a
> +	 * multiplier factor MF exists that fulfills the following conditions:
> +	 *
> +	 * 1. p1 is in the [p1_min, p1_max] range given by the limits and is
> +	 *    even
> +	 * 2. mf is in the [mf_min, mf_max] range computed above
> +	 * 3. div * mf is a multiple of p1, in order to compute
> +	 *	n = div * mf / p1
> +	 *	m = pll->m * mf
> +	 * 4. the internal clock frequency, given by ext_clock / n, is in the
> +	 *    [int_clock_min, int_clock_max] range given by the limits
> +	 * 5. the output clock frequency, given by ext_clock / n * m, is in the
> +	 *    [out_clock_min, out_clock_max] range given by the limits
> +	 *
> +	 * The first naive approach is to iterate over all p1 values acceptable
> +	 * according to (1) and all mf values acceptable according to (2), and
> +	 * stop at the first combination that fulfills (3), (4) and (5). This
> +	 * has a O(n^2) complexity.
> +	 *
> +	 * Instead of iterating over all mf values in the [mf_min, mf_max] range
> +	 * we can compute the mf increment between two acceptable values
> +	 * according to (3) with
> +	 *
> +	 *	mf_inc = lcm(div, p1) / div			(6)
> +	 *
> +	 * and round the minimum up to the nearest multiple of mf_inc. This will
> +	 * restrict the number of mf values to be checked.
> +	 *
> +	 * Furthermore, conditions (4) and (5) only restrict the range of
> +	 * acceptable p1 and mf values by modifying the minimum and maximum
> +	 * limits. (5) can be expressed as
> +	 *
> +	 *	ext_clock / (div * mf / p1) * m * mf >= out_clock_min
> +	 *	ext_clock / (div * mf / p1) * m * mf <= out_clock_max
> +	 *
> +	 * or
> +	 *
> +	 *	p1 >= out_clock_min * div / (ext_clock * m)	(7)
> +	 *	p1 <= out_clock_max * div / (ext_clock * m)
> +	 *
> +	 * Similarly, (4) can be expressed as
> +	 *
> +	 *	mf >= ext_clock * p1 / (int_clock_max * div)	(8)
> +	 *	mf <= ext_clock * p1 / (int_clock_min * div)
> +	 *
> +	 * We can thus iterate over the restricted p1 range defined by the
> +	 * combination of (1) and (7), and then compute the restricted mf range
> +	 * defined by the combination of (2), (6) and (8). If the resulting mf
> +	 * range is not empty, any value in the mf range is acceptable. We thus
> +	 * select the mf lwoer bound and the corresponding p1 value.
> +	 */
> +	if (limits->p1_min == 0) {
> +		dev_err(dev, "pll: P1 minimum value must be >0.\n");
> +		return -EINVAL;
> +	}
> +
> +	p1_min = max(limits->p1_min, DIV_ROUND_UP(limits->out_clock_min * div,
> +		     pll->ext_clock * pll->m));
> +	p1_max = min(limits->p1_max, limits->out_clock_max * div /
> +		     (pll->ext_clock * pll->m));
> +
> +	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.

> +		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.

> +		}
> +	}
> +
> +	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;
> +}
> +EXPORT_SYMBOL_GPL(aptina_pll_configure);
> +
> +MODULE_DESCRIPTION("Aptina PLL Helpers");
> +MODULE_AUTHOR("Laurent Pinchart <laurent.pinchart@ideasonboard.com>");
> +MODULE_LICENSE("GPL v2");
> diff --git a/drivers/media/video/aptina-pll.h b/drivers/media/video/aptina-pll.h
> new file mode 100644
> index 0000000..36a9363
> --- /dev/null
> +++ b/drivers/media/video/aptina-pll.h
> @@ -0,0 +1,55 @@
> +/*
> + * Aptina Sensor PLL Configuration
> + *
> + * Copyright (C) 2012 Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful, but
> + * WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
> + * 02110-1301 USA
> + */
> +
> +#ifndef __APTINA_PLL_H
> +#define __APTINA_PLL_H
> +
> +struct aptina_pll {
> +	unsigned int ext_clock;
> +	unsigned int pix_clock;
> +
> +	unsigned int n;
> +	unsigned int m;
> +	unsigned int p1;
> +};
> +
> +struct aptina_pll_limits {
> +	unsigned int ext_clock_min;
> +	unsigned int ext_clock_max;
> +	unsigned int int_clock_min;
> +	unsigned int int_clock_max;
> +	unsigned int out_clock_min;
> +	unsigned int out_clock_max;
> +	unsigned int pix_clock_max;
> +
> +	unsigned int n_min;
> +	unsigned int n_max;
> +	unsigned int m_min;
> +	unsigned int m_max;
> +	unsigned int p1_min;
> +	unsigned int p1_max;
> +};
> +
> +struct device;
> +
> +int aptina_pll_configure(struct device *dev, struct aptina_pll *pll,
> +			 const struct aptina_pll_limits *limits);
> +
> +#endif /* __APTINA_PLL_H */
> -- 
> 1.7.3.4
> 

Cheers,

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi	jabber/XMPP/Gmail: sailus@retiisi.org.uk

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

[Laurent Pinchart]

Hi Sakari,

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 ? :-)

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

> > +		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.

> > +		}
> > +	}
> > +
> > +	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;
> > +}

-- 
Regards,

Laurent Pinchart

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

[Sakari Ailus]

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

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

[Laurent Pinchart]

Hi Sakari,

On Sunday 04 March 2012 17:01:30 Sakari Ailus wrote:
> 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.

You're lucky, I'm in a good mood, I'll change that ;-)

-- 
Regards,

Laurent Pinchart

[PATCH v3 10/10] mt9m032: Use generic PLL setup code

[Laurent Pinchart]

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@ideasonboard.com>
---
 drivers/media/video/Kconfig   |    1 +
 drivers/media/video/mt9m032.c |   56 +++++++++++++++++++++++++++--------------
 include/media/mt9m032.h       |    4 +--
 3 files changed, 39 insertions(+), 22 deletions(-)

diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig
index 410baf2..2b3f4b1 100644
--- a/drivers/media/video/Kconfig
+++ b/drivers/media/video/Kconfig
@@ -949,6 +949,7 @@ config SOC_CAMERA_MT9M001
 config VIDEO_MT9M032
 	tristate "MT9M032 camera sensor support"
 	depends on I2C && VIDEO_V4L2
+	select VIDEO_APTINA_PLL
 	help
 	  This driver supports MT9M032 cameras from Micron, monochrome
 	  models only.
diff --git a/drivers/media/video/mt9m032.c b/drivers/media/video/mt9m032.c
index 4cde779..da69e12 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
@@ -222,32 +227,45 @@ static int update_formatter2(struct mt9m032 *sensor, bool streaming)
 
 static int mt9m032_setup_pll(struct mt9m032 *sensor)
 {
+	static const struct aptina_pll_limits limits = {
+		.ext_clock_min = 8000000,
+		.ext_clock_max = 16500000,
+		.int_clock_min = 2000000,
+		.int_clock_max = 24000000,
+		.out_clock_min = 322000000,
+		.out_clock_max = 693000000,
+		.pix_clock_max = 99000000,
+		.n_min = 1,
+		.n_max = 64,
+		.m_min = 16,
+		.m_max = 255,
+		.p1_min = 1,
+		.p1_max = 128,
+	};
+
 	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 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;
+	pll.ext_clock = pdata->ext_clock;
+	pll.pix_clock = pdata->pix_clock;
 
-	sensor->pix_clock = pdata->ext_clock * pdata->pll_mul /
-		(pre_div * pdata->pll_out_div);
+	ret = aptina_pll_configure(&client->dev, &pll, &limits);
+	if (ret < 0)
+		return ret;
 
-	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;
 
 };
-- 
1.7.3.4