[PATCH v2 00/10] MT9M032 sensor driver

[PATCH v2 00/10] MT9M032 sensor driver

[Laurent Pinchart]

Hi everybody,

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

Compared to the first version, I've squashed two patches together that were
not supposed to be split in the first place and fixed a couple of 80-columns
checkpatch warnings.

The first patch is still Martin's original driver untouched for ease of
review. I can squash some of the other patches onto it if needed after review
is complete.

Patch 09/10 adds a generic PLL setup code for several Aptina sensors. I will
post a patch for the MT9P031 sensor driver to use that code separately from
this set.

I would still like to push the patches in time for v3.4. Martin, could you
please review the set ? Guennadi, to you have any MT9M032 hardware you can use
to test the 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 |  120 ++++++
 drivers/media/video/aptina-pll.h |   55 +++
 drivers/media/video/mt9m032.c    |  827 ++++++++++++++++++++++++++++++++++++++
 include/media/mt9m032.h          |   36 ++
 6 files changed, 1054 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 v2 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 v2 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 v2 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 v2 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 v2 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 v2 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 v2 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      |    4 +
 drivers/media/video/Makefile     |    4 +
 drivers/media/video/aptina-pll.c |  120 ++++++++++++++++++++++++++++++++++++++
 drivers/media/video/aptina-pll.h |   55 +++++++++++++++++
 4 files changed, 183 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..e1dfdbc 100644
--- a/drivers/media/video/Kconfig
+++ b/drivers/media/video/Kconfig
@@ -133,6 +133,9 @@ menu "Encoders, decoders, sensors and other helper chips"
 
 comment "Audio decoders, processors and mixers"
 
+config VIDEO_APTINA_PLL
+	tristate
+
 config VIDEO_TVAUDIO
 	tristate "Simple audio decoder chips"
 	depends on VIDEO_V4L2 && I2C
@@ -946,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/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..e4df9ec
--- /dev/null
+++ b/drivers/media/video/aptina-pll.c
@@ -0,0 +1,120 @@
+/*
+ * 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/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 mf;
+	unsigned int clock;
+	unsigned int div;
+	unsigned int p1;
+	unsigned int n;
+	unsigned int m;
+
+	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;
+	}
+
+	/* Find the highest acceptable P1 value and compute the corresponding N
+	 * divisor. Make sure the P1 value is even.
+	 */
+	for (mf = mf_min; mf <= mf_max; ++mf) {
+		m = pll->m * mf;
+
+		for (p1 = limits->p1_max & ~1; p1 > limits->p1_min; p1 -= 2) {
+			if ((div * mf) % p1)
+				continue;
+
+			n = div * mf / p1;
+
+			clock = pll->ext_clock / n;
+			if (clock < limits->int_clock_min ||
+			    clock > limits->int_clock_max)
+				continue;
+
+			clock *= m;
+			if (clock < limits->out_clock_min ||
+			    clock > limits->out_clock_max)
+				continue;
+
+			goto found;
+		}
+	}
+
+	dev_err(dev, "pll: no valid N and P1 divisors found.\n");
+	return -EINVAL;
+
+found:
+	pll->n = n;
+	pll->m = m;
+	pll->p1 = p1;
+
+	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);
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

[PATCH v2 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/mt9m032.c |   61 ++++++++++++++++++++++++++---------------
 include/media/mt9m032.h       |    4 +--
 2 files changed, 40 insertions(+), 25 deletions(-)

diff --git a/drivers/media/video/mt9m032.c b/drivers/media/video/mt9m032.c
index 4cde779..45e8c68 100644
--- a/drivers/media/video/mt9m032.c
+++ b/drivers/media/video/mt9m032.c
@@ -35,6 +35,8 @@
 #include <media/v4l2-device.h>
 #include <media/v4l2-subdev.h>
 
+#include "aptina-pll.h"
+
 #define MT9M032_CHIP_VERSION			0x00
 #define     MT9M032_CHIP_VERSION_VALUE		0x1402
 #define MT9M032_ROW_START			0x01
@@ -61,6 +63,7 @@
 #define MT9M032_GAIN_BLUE			0x2c
 #define MT9M032_GAIN_RED			0x2d
 #define MT9M032_GAIN_GREEN2			0x2e
+
 /* write only */
 #define MT9M032_GAIN_ALL			0x35
 #define     MT9M032_GAIN_DIGITAL_MASK		0x7f
@@ -83,6 +86,8 @@
 #define     MT9P031_PLL_CONTROL_PWROFF		0x0050
 #define     MT9P031_PLL_CONTROL_PWRON		0x0051
 #define     MT9P031_PLL_CONTROL_USEPLL		0x0052
+#define MT9P031_PLL_CONFIG2			0x11
+#define     MT9P031_PLL_CONFIG2_P1_DIV_MASK	0x1f
 
 /*
  * width and height include active boundry and black parts
@@ -223,31 +228,43 @@ static int update_formatter2(struct mt9m032 *sensor, bool streaming)
 static int mt9m032_setup_pll(struct mt9m032 *sensor)
 {
 	struct i2c_client *client = v4l2_get_subdevdata(&sensor->subdev);
-	struct mt9m032_platform_data* pdata = sensor->pdata;
-	u16 reg_pll1;
-	unsigned int pre_div;
+	struct mt9m032_platform_data *pdata = sensor->pdata;
+	struct aptina_pll_limits limits;
+	struct aptina_pll pll;
 	int ret;
 
-	/* TODO: also support other pre-div values */
-	if (pdata->pll_pre_div != 6) {
-		dev_warn(to_dev(sensor),
-			"Unsupported PLL pre-divisor value %u, using default 6\n",
-			pdata->pll_pre_div);
-	}
-	pre_div = 6;
-
-	sensor->pix_clock = pdata->ext_clock * pdata->pll_mul /
-		(pre_div * pdata->pll_out_div);
+	limits.ext_clock_min = 8000000;
+	limits.ext_clock_max = 16500000;
+	limits.int_clock_min = 2000000;
+	limits.int_clock_max = 24000000;
+	limits.out_clock_min = 322000000;
+	limits.out_clock_max = 693000000;
+	limits.pix_clock_max = 99000000;
+	limits.n_min = 1;
+	limits.n_max = 64;
+	limits.m_min = 16;
+	limits.m_max = 255;
+	limits.p1_min = 1;
+	limits.p1_max = 128;
+
+	pll.ext_clock = pdata->ext_clock;
+	pll.pix_clock = pdata->pix_clock;
+
+	ret = aptina_pll_configure(&client->dev, &pll, &limits);
+	if (ret < 0)
+		return ret;
 
-	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

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

[Sakari Ailus]

Hi Laurent,

Thanks for the patch.

On Fri, Mar 02, 2012 at 11:44:06AM +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      |    4 +
>  drivers/media/video/Makefile     |    4 +
>  drivers/media/video/aptina-pll.c |  120 ++++++++++++++++++++++++++++++++++++++
>  drivers/media/video/aptina-pll.h |   55 +++++++++++++++++
>  4 files changed, 183 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..e1dfdbc 100644
> --- a/drivers/media/video/Kconfig
> +++ b/drivers/media/video/Kconfig
> @@ -133,6 +133,9 @@ menu "Encoders, decoders, sensors and other helper chips"
>  
>  comment "Audio decoders, processors and mixers"
>  
> +config VIDEO_APTINA_PLL
> +	tristate
> +
>  config VIDEO_TVAUDIO
>  	tristate "Simple audio decoder chips"
>  	depends on VIDEO_V4L2 && I2C

Wouldn't it make sense to create another section for these to separate them
from the reset? This isn't audio decoder, processor nor mixer. :-)

> @@ -946,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.

This should be in another patch, shouldn't it?

> 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..e4df9ec
> --- /dev/null
> +++ b/drivers/media/video/aptina-pll.c
> @@ -0,0 +1,120 @@
> +/*
> + * 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/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 mf;
> +	unsigned int clock;
> +	unsigned int div;
> +	unsigned int p1;
> +	unsigned int n;
> +	unsigned int m;
> +
> +	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));

I'd split this line as you've split the rest.

> +	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;
> +	}
> +
> +	/* Find the highest acceptable P1 value and compute the corresponding N
> +	 * divisor. Make sure the P1 value is even.
> +	 */
> +	for (mf = mf_min; mf <= mf_max; ++mf) {
> +		m = pll->m * mf;
> +
> +		for (p1 = limits->p1_max & ~1; p1 > limits->p1_min; p1 -= 2) {

Can't p1 be equal to limits->p1_min?

What are typical values for p1_min and p1_max?

> +			if ((div * mf) % p1)
> +				continue;

I think you could calculate the valid iteration change for mf and avoid
extra time spent in the loop. You could swap the for loops which gives you
constant divider in the inner loop, which should be helpful.

Example (not fully tested nor thought out):

mf_min = ALIGN(p1, lcm(div, p1) / div)

mf += lcm(div, p1) / div

> +			n = div * mf / p1;
> +
> +			clock = pll->ext_clock / n;
> +			if (clock < limits->int_clock_min ||
> +			    clock > limits->int_clock_max)
> +				continue;
> +
> +			clock *= m;
> +			if (clock < limits->out_clock_min ||
> +			    clock > limits->out_clock_max)
> +				continue;

Same goes with clock values: now you can calculate which range of mf_min and
mf_max you need to check. Your inner loop becomes a simple check for p1_min
<= p1_max.

> +			goto found;
> +		}
> +	}
> +
> +	dev_err(dev, "pll: no valid N and P1 divisors found.\n");
> +	return -EINVAL;
> +
> +found:
> +	pll->n = n;
> +	pll->m = m;
> +	pll->p1 = p1;
> +
> +	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);
> 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;

Is there no minimum for pix_clock?

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

Regards,

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

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

[Sakari Ailus]

Hi Laurent,

Thanks for the patch.

On Fri, Mar 02, 2012 at 11:44:07AM +0100, Laurent Pinchart wrote:
> Compute the PLL parameters at runtime using the generic Aptina PLL
> helper.
> 
> Remove the PLL parameters from platform data and pass the external clock
> and desired internal clock frequencies instead.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  drivers/media/video/mt9m032.c |   61 ++++++++++++++++++++++++++---------------
>  include/media/mt9m032.h       |    4 +--
>  2 files changed, 40 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/media/video/mt9m032.c b/drivers/media/video/mt9m032.c
> index 4cde779..45e8c68 100644
> --- a/drivers/media/video/mt9m032.c
> +++ b/drivers/media/video/mt9m032.c
> @@ -35,6 +35,8 @@
>  #include <media/v4l2-device.h>
>  #include <media/v4l2-subdev.h>
>  
> +#include "aptina-pll.h"
> +
>  #define MT9M032_CHIP_VERSION			0x00
>  #define     MT9M032_CHIP_VERSION_VALUE		0x1402
>  #define MT9M032_ROW_START			0x01
> @@ -61,6 +63,7 @@
>  #define MT9M032_GAIN_BLUE			0x2c
>  #define MT9M032_GAIN_RED			0x2d
>  #define MT9M032_GAIN_GREEN2			0x2e
> +
>  /* write only */
>  #define MT9M032_GAIN_ALL			0x35
>  #define     MT9M032_GAIN_DIGITAL_MASK		0x7f
> @@ -83,6 +86,8 @@
>  #define     MT9P031_PLL_CONTROL_PWROFF		0x0050
>  #define     MT9P031_PLL_CONTROL_PWRON		0x0051
>  #define     MT9P031_PLL_CONTROL_USEPLL		0x0052
> +#define MT9P031_PLL_CONFIG2			0x11
> +#define     MT9P031_PLL_CONFIG2_P1_DIV_MASK	0x1f
>  
>  /*
>   * width and height include active boundry and black parts
> @@ -223,31 +228,43 @@ static int update_formatter2(struct mt9m032 *sensor, bool streaming)
>  static int mt9m032_setup_pll(struct mt9m032 *sensor)
>  {
>  	struct i2c_client *client = v4l2_get_subdevdata(&sensor->subdev);
> -	struct mt9m032_platform_data* pdata = sensor->pdata;
> -	u16 reg_pll1;
> -	unsigned int pre_div;
> +	struct mt9m032_platform_data *pdata = sensor->pdata;
> +	struct aptina_pll_limits limits;
> +	struct aptina_pll pll;
>  	int ret;
>  
> -	/* TODO: also support other pre-div values */
> -	if (pdata->pll_pre_div != 6) {
> -		dev_warn(to_dev(sensor),
> -			"Unsupported PLL pre-divisor value %u, using default 6\n",
> -			pdata->pll_pre_div);
> -	}
> -	pre_div = 6;
> -
> -	sensor->pix_clock = pdata->ext_clock * pdata->pll_mul /
> -		(pre_div * pdata->pll_out_div);
> +	limits.ext_clock_min = 8000000;
> +	limits.ext_clock_max = 16500000;
> +	limits.int_clock_min = 2000000;
> +	limits.int_clock_max = 24000000;
> +	limits.out_clock_min = 322000000;
> +	limits.out_clock_max = 693000000;
> +	limits.pix_clock_max = 99000000;
> +	limits.n_min = 1;
> +	limits.n_max = 64;
> +	limits.m_min = 16;
> +	limits.m_max = 255;
> +	limits.p1_min = 1;
> +	limits.p1_max = 128;

You could make limits const and static.

> +	pll.ext_clock = pdata->ext_clock;
> +	pll.pix_clock = pdata->pix_clock;
> +
> +	ret = aptina_pll_configure(&client->dev, &pll, &limits);
> +	if (ret < 0)
> +		return ret;

Now that you're handling timing information in the sensor driver, including
blanking --- AFAIU, what would you think about implementing the new sensor
control interface for this one?

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

Regards,

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

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

[Laurent Pinchart]

Hi Sakari,

Thanks for the review.

On Friday 02 March 2012 20:46:21 Sakari Ailus wrote:
> On Fri, Mar 02, 2012 at 11:44:06AM +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      |    4 +
> >  drivers/media/video/Makefile     |    4 +
> >  drivers/media/video/aptina-pll.c |  120
> >  ++++++++++++++++++++++++++++++++++++++ drivers/media/video/aptina-pll.h
> >  |   55 +++++++++++++++++
> >  4 files changed, 183 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..e1dfdbc 100644
> > --- a/drivers/media/video/Kconfig
> > +++ b/drivers/media/video/Kconfig
> > @@ -133,6 +133,9 @@ menu "Encoders, decoders, sensors and other helper
> > chips"> 
> >  comment "Audio decoders, processors and mixers"
> > 
> > +config VIDEO_APTINA_PLL
> > +	tristate
> > +
> > 
> >  config VIDEO_TVAUDIO
> >  
> >  	tristate "Simple audio decoder chips"
> >  	depends on VIDEO_V4L2 && I2C
> 
> Wouldn't it make sense to create another section for these to separate them
> from the reset? This isn't audio decoder, processor nor mixer. :-)

I'm not sure if a separate section is really needed. The option won't show up 
during configuration, it will be selected automatically by other drivers. I'll 
move it to the "Camera sensor devices" section anyway, as it's more logical to 
group it with the sensor drivers.

> > @@ -946,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.
> 
> This should be in another patch, shouldn't it?

Yes, That's already fixed :-)

> > 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..e4df9ec
> > --- /dev/null
> > +++ b/drivers/media/video/aptina-pll.c
> > @@ -0,0 +1,120 @@
> > +/*
> > + * 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/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 mf;
> > +	unsigned int clock;
> > +	unsigned int div;
> > +	unsigned int p1;
> > +	unsigned int n;
> > +	unsigned int m;
> > +
> > +	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));
> 
> I'd split this line as you've split the rest.

The other lines were split to keep lines shorter than 81 columns. This one is 
short enough.

> > +	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;
> > +	}
> > +
> > +	/* Find the highest acceptable P1 value and compute the corresponding 
N
> > +	 * divisor. Make sure the P1 value is even.
> > +	 */
> > +	for (mf = mf_min; mf <= mf_max; ++mf) {
> > +		m = pll->m * mf;
> > +
> > +		for (p1 = limits->p1_max & ~1; p1 > limits->p1_min; p1 -= 2) {
> 
> Can't p1 be equal to limits->p1_min?
> 
> What are typical values for p1_min and p1_max?

Typical values are 1 and 128. As p1 should be even, it will never be equal to 
p1_min. However, in the general case, it could, so I'll fix this. There's a 
risk of inifinite loop if the caller sets p1_min to 0, so I'll add a check.

> > +			if ((div * mf) % p1)
> > +				continue;
> 
> I think you could calculate the valid iteration change for mf and avoid
> extra time spent in the loop. You could swap the for loops which gives you
> constant divider in the inner loop, which should be helpful.
> 
> Example (not fully tested nor thought out):
> 
> mf_min = ALIGN(p1, lcm(div, p1) / div)
> 
> mf += lcm(div, p1) / div
>
> > +			n = div * mf / p1;
> > +
> > +			clock = pll->ext_clock / n;
> > +			if (clock < limits->int_clock_min ||
> > +			    clock > limits->int_clock_max)
> > +				continue;
> > +
> > +			clock *= m;
> > +			if (clock < limits->out_clock_min ||
> > +			    clock > limits->out_clock_max)
> > +				continue;
> 
> Same goes with clock values: now you can calculate which range of mf_min and
> mf_max you need to check. Your inner loop becomes a simple check for p1_min
> <= p1_max.

I've found a possible solution, that gets rid of the inner loop. See v3 :-)

> > +			goto found;
> > +		}
> > +	}
> > +
> > +	dev_err(dev, "pll: no valid N and P1 divisors found.\n");
> > +	return -EINVAL;
> > +
> > +found:
> > +	pll->n = n;
> > +	pll->m = m;
> > +	pll->p1 = p1;
> > +
> > +	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);
> > 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;
> 
> Is there no minimum for pix_clock?

Not in the datasheets I have.

> > +	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 */

-- 
Regards,

Laurent Pinchart

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

[Laurent Pinchart]

Hi Sakari,

Thanks for the review.

On Saturday 03 March 2012 00:41:26 Sakari Ailus wrote:
> On Fri, Mar 02, 2012 at 11:44:07AM +0100, Laurent Pinchart wrote:
> > Compute the PLL parameters at runtime using the generic Aptina PLL
> > helper.
> > 
> > Remove the PLL parameters from platform data and pass the external clock
> > and desired internal clock frequencies instead.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> > 
> >  drivers/media/video/mt9m032.c |   61
> >  ++++++++++++++++++++++++++--------------- include/media/mt9m032.h      
> >  |    4 +--
> >  2 files changed, 40 insertions(+), 25 deletions(-)
> > 
> > diff --git a/drivers/media/video/mt9m032.c b/drivers/media/video/mt9m032.c
> > index 4cde779..45e8c68 100644
> > --- a/drivers/media/video/mt9m032.c
> > +++ b/drivers/media/video/mt9m032.c
> > @@ -35,6 +35,8 @@
> > 
> >  #include <media/v4l2-device.h>
> >  #include <media/v4l2-subdev.h>
> > 
> > +#include "aptina-pll.h"
> > +
> > 
> >  #define MT9M032_CHIP_VERSION			0x00
> >  #define     MT9M032_CHIP_VERSION_VALUE		0x1402
> >  #define MT9M032_ROW_START			0x01
> > 
> > @@ -61,6 +63,7 @@
> > 
> >  #define MT9M032_GAIN_BLUE			0x2c
> >  #define MT9M032_GAIN_RED			0x2d
> >  #define MT9M032_GAIN_GREEN2			0x2e
> > 
> > +
> > 
> >  /* write only */
> >  #define MT9M032_GAIN_ALL			0x35
> >  #define     MT9M032_GAIN_DIGITAL_MASK		0x7f
> > 
> > @@ -83,6 +86,8 @@
> > 
> >  #define     MT9P031_PLL_CONTROL_PWROFF		0x0050
> >  #define     MT9P031_PLL_CONTROL_PWRON		0x0051
> >  #define     MT9P031_PLL_CONTROL_USEPLL		0x0052
> > 
> > +#define MT9P031_PLL_CONFIG2			0x11
> > +#define     MT9P031_PLL_CONFIG2_P1_DIV_MASK	0x1f
> > 
> >  /*
> >  
> >   * width and height include active boundry and black parts
> > 
> > @@ -223,31 +228,43 @@ static int update_formatter2(struct mt9m032 *sensor,
> > bool streaming)> 
> >  static int mt9m032_setup_pll(struct mt9m032 *sensor)
> >  {
> >  
> >  	struct i2c_client *client = v4l2_get_subdevdata(&sensor->subdev);
> > 
> > -	struct mt9m032_platform_data* pdata = sensor->pdata;
> > -	u16 reg_pll1;
> > -	unsigned int pre_div;
> > +	struct mt9m032_platform_data *pdata = sensor->pdata;
> > +	struct aptina_pll_limits limits;
> > +	struct aptina_pll pll;
> > 
> >  	int ret;
> > 
> > -	/* TODO: also support other pre-div values */
> > -	if (pdata->pll_pre_div != 6) {
> > -		dev_warn(to_dev(sensor),
> > -			"Unsupported PLL pre-divisor value %u, using default 6\n",
> > -			pdata->pll_pre_div);
> > -	}
> > -	pre_div = 6;
> > -
> > -	sensor->pix_clock = pdata->ext_clock * pdata->pll_mul /
> > -		(pre_div * pdata->pll_out_div);
> > +	limits.ext_clock_min = 8000000;
> > +	limits.ext_clock_max = 16500000;
> > +	limits.int_clock_min = 2000000;
> > +	limits.int_clock_max = 24000000;
> > +	limits.out_clock_min = 322000000;
> > +	limits.out_clock_max = 693000000;
> > +	limits.pix_clock_max = 99000000;
> > +	limits.n_min = 1;
> > +	limits.n_max = 64;
> > +	limits.m_min = 16;
> > +	limits.m_max = 255;
> > +	limits.p1_min = 1;
> > +	limits.p1_max = 128;
> 
> You could make limits const and static.

Done.

> > +	pll.ext_clock = pdata->ext_clock;
> > +	pll.pix_clock = pdata->pix_clock;
> > +
> > +	ret = aptina_pll_configure(&client->dev, &pll, &limits);
> > +	if (ret < 0)
> > +		return ret;
> 
> Now that you're handling timing information in the sensor driver, including
> blanking --- AFAIU, what would you think about implementing the new sensor
> control interface for this one?

Sure, but in a further patch :-)

> > -	reg_pll1 = ((pdata->pll_out_div - 1) & 
MT9M032_PLL_CONFIG1_OUTDIV_MASK)
> > -		   | pdata->pll_mul << MT9M032_PLL_CONFIG1_MUL_SHIFT;
> > +	sensor->pix_clock = pll.pix_clock;
> > 
> > -	ret = mt9m032_write_reg(client, MT9M032_PLL_CONFIG1, reg_pll1);
> > +	ret = mt9m032_write_reg(client, MT9M032_PLL_CONFIG1,
> > +				(pll.m << MT9M032_PLL_CONFIG1_MUL_SHIFT)
> > +				| (pll.p1 - 1));
> > 
> >  	if (!ret)
> > 
> > -		ret = mt9m032_write_reg(client,
> > -					MT9P031_PLL_CONTROL,
> > -					MT9P031_PLL_CONTROL_PWRON | 
MT9P031_PLL_CONTROL_USEPLL);
> > -
> > +		ret = mt9m032_write_reg(client, MT9P031_PLL_CONFIG2, pll.n - 1);
> > +	if (!ret)
> > +		ret = mt9m032_write_reg(client, MT9P031_PLL_CONTROL,
> > +					MT9P031_PLL_CONTROL_PWRON |
> > +					MT9P031_PLL_CONTROL_USEPLL);
> > 
> >  	if (!ret)
> >  	
> >  		ret = mt9m032_write_reg(client, MT9M032_READ_MODE1, 0x8006);
> >  		
> >  							/* more reserved, Continuous */
> > 
> > diff --git a/include/media/mt9m032.h b/include/media/mt9m032.h
> > index 94cefc5..804e0a5 100644
> > --- a/include/media/mt9m032.h
> > +++ b/include/media/mt9m032.h
> > @@ -29,9 +29,7 @@
> > 
> >  struct mt9m032_platform_data {
> >  
> >  	u32 ext_clock;
> > 
> > -	u32 pll_pre_div;
> > -	u32 pll_mul;
> > -	u32 pll_out_div;
> > +	u32 pix_clock;
> > 
> >  	int invert_pixclock;
> >  
> >  };

-- 
Regards,

Laurent Pinchart