[PATCH][RFC] Add mt9p031 sensor support.

[PATCH][RFC] Add mt9p031 sensor support.

[Javier Martin]

This RFC includes a power management implementation that causes
the sensor to show images with horizontal artifacts (usually
monochrome lines that appear on the image randomly).

Signed-off-by: Javier Martin <javier.martin@vista-silicon.com>
---
 drivers/media/video/Kconfig   |    7 +
 drivers/media/video/Makefile  |    1 +
 drivers/media/video/mt9p031.c |  841 +++++++++++++++++++++++++++++++++++++++++
 include/media/mt9p031.h       |   11 +
 4 files changed, 860 insertions(+), 0 deletions(-)
 create mode 100644 drivers/media/video/mt9p031.c
 create mode 100644 include/media/mt9p031.h

diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig
index 00f51dd..8a596cc 100644
--- a/drivers/media/video/Kconfig
+++ b/drivers/media/video/Kconfig
@@ -329,6 +329,13 @@ config VIDEO_OV7670
 	  OV7670 VGA camera.  It currently only works with the M88ALP01
 	  controller.
 
+config VIDEO_MT9P031
+	tristate "Aptina MT9P031 support"
+	depends on I2C && VIDEO_V4L2
+	---help---
+	 This is a Video4Linux2 sensor-level driver for the Aptina
+	 (Micron) mt9p031 5 Mpixel camera.
+
 config VIDEO_MT9V011
 	tristate "Micron mt9v011 sensor support"
 	depends on I2C && VIDEO_V4L2
diff --git a/drivers/media/video/Makefile b/drivers/media/video/Makefile
index ace5d8b..912b29b 100644
--- a/drivers/media/video/Makefile
+++ b/drivers/media/video/Makefile
@@ -65,6 +65,7 @@ obj-$(CONFIG_VIDEO_UPD64083) += upd64083.o
 obj-$(CONFIG_VIDEO_OV7670) 	+= ov7670.o
 obj-$(CONFIG_VIDEO_TCM825X) += tcm825x.o
 obj-$(CONFIG_VIDEO_TVEEPROM) += tveeprom.o
+obj-$(CONFIG_VIDEO_MT9P031) += mt9p031.o
 obj-$(CONFIG_VIDEO_MT9V011) += mt9v011.o
 obj-$(CONFIG_VIDEO_SR030PC30)	+= sr030pc30.o
 obj-$(CONFIG_VIDEO_NOON010PC30)	+= noon010pc30.o
diff --git a/drivers/media/video/mt9p031.c b/drivers/media/video/mt9p031.c
new file mode 100644
index 0000000..04d8812
--- /dev/null
+++ b/drivers/media/video/mt9p031.c
@@ -0,0 +1,841 @@
+/*
+ * Driver for MT9P031 CMOS Image Sensor from Aptina
+ *
+ * Copyright (C) 2011, Javier Martin <javier.martin@vista-silicon.com>
+ *
+ * Copyright (C) 2011, Guennadi Liakhovetski <g.liakhovetski@gmx.de>
+ *
+ * Based on the MT9V032 driver and Bastian Hecht's code.
+ *
+ * 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.
+ */
+
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/i2c.h>
+#include <linux/log2.h>
+#include <linux/pm.h>
+#include <linux/regulator/consumer.h>
+#include <linux/slab.h>
+#include <media/v4l2-subdev.h>
+#include <linux/videodev2.h>
+
+#include <media/mt9p031.h>
+#include <media/v4l2-chip-ident.h>
+#include <media/v4l2-subdev.h>
+#include <media/v4l2-device.h>
+
+#define MT9P031_PIXCLK_FREQ			54000000
+
+/* mt9p031 selected register addresses */
+#define MT9P031_CHIP_VERSION			0x00
+#define		MT9P031_CHIP_VERSION_VALUE	0x1801
+#define MT9P031_ROW_START			0x01
+#define		MT9P031_ROW_START_DEF		54
+#define MT9P031_COLUMN_START			0x02
+#define		MT9P031_COLUMN_START_DEF	16
+#define MT9P031_WINDOW_HEIGHT			0x03
+#define MT9P031_WINDOW_WIDTH			0x04
+#define MT9P031_H_BLANKING			0x05
+#define		MT9P031_H_BLANKING_VALUE	0
+#define MT9P031_V_BLANKING			0x06
+#define		MT9P031_V_BLANKING_VALUE	25
+#define MT9P031_OUTPUT_CONTROL			0x07
+#define		MT9P031_OUTPUT_CONTROL_CEN	2
+#define		MT9P031_OUTPUT_CONTROL_SYN	1
+#define MT9P031_SHUTTER_WIDTH_UPPER		0x08
+#define MT9P031_SHUTTER_WIDTH			0x09
+#define MT9P031_PIXEL_CLOCK_CONTROL		0x0a
+#define MT9P031_FRAME_RESTART			0x0b
+#define MT9P031_SHUTTER_DELAY			0x0c
+#define MT9P031_RST				0x0d
+#define		MT9P031_RST_ENABLE		1
+#define		MT9P031_RST_DISABLE		0
+#define MT9P031_READ_MODE_1			0x1e
+#define MT9P031_READ_MODE_2			0x20
+#define		MT9P031_READ_MODE_2_ROW_MIR	0x8000
+#define		MT9P031_READ_MODE_2_COL_MIR	0x4000
+#define MT9P031_ROW_ADDRESS_MODE		0x22
+#define MT9P031_COLUMN_ADDRESS_MODE		0x23
+#define MT9P031_GLOBAL_GAIN			0x35
+
+#define MT9P031_WINDOW_HEIGHT_MAX		1944
+#define MT9P031_WINDOW_WIDTH_MAX		2592
+#define MT9P031_WINDOW_HEIGHT_MIN		2
+#define MT9P031_WINDOW_WIDTH_MIN		18
+
+struct mt9p031 {
+	struct v4l2_subdev subdev;
+	struct media_pad pad;
+	struct v4l2_rect rect;	/* Sensor window */
+	struct v4l2_mbus_framefmt format;
+	struct mt9p031_platform_data *pdata;
+	struct mutex power_lock; /* lock to protect power_count */
+	int power_count;
+	u16 xskip;
+	u16 yskip;
+	/* cache register values */
+	u16 output_control;
+	u16 h_blanking;
+	u16 v_blanking;
+	u16 column_address_mode;
+	u16 row_address_mode;
+	u16 column_start;
+	u16 row_start;
+	u16 window_width;
+	u16 window_height;
+	struct regulator *reg_1v8;
+	struct regulator *reg_2v8;
+};
+
+static struct mt9p031 *to_mt9p031(const struct i2c_client *client)
+{
+	return container_of(i2c_get_clientdata(client), struct mt9p031, subdev);
+}
+
+static int reg_read(struct i2c_client *client, const u8 reg)
+{
+	s32 data = i2c_smbus_read_word_data(client, reg);
+	return data < 0 ? data : swab16(data);
+}
+
+static int reg_write(struct i2c_client *client, const u8 reg,
+			const u16 data)
+{
+	return i2c_smbus_write_word_data(client, reg, swab16(data));
+}
+
+static int reg_write_cached(struct i2c_client *client, const u8 reg,
+			const u16 data, u16 *cache)
+{
+	int ret;
+
+	ret = reg_write(client, reg, data);
+	if (ret < 0)
+		return ret;
+	*cache = data;
+	return 0;
+}
+
+static int mt9p031_set_output_control(struct mt9p031 *mt9p031, u16 clear,
+				      u16 set)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(&mt9p031->subdev);
+	u16 value = (mt9p031->output_control & ~clear) | set;
+
+	return reg_write_cached(client, MT9P031_OUTPUT_CONTROL, value,
+				&mt9p031->output_control);
+}
+
+static int restore_registers(struct i2c_client *client)
+{
+	int ret;
+	struct mt9p031 *mt9p031 = to_mt9p031(client);
+
+	/* Disable register update, reconfigure atomically */
+	ret = mt9p031_set_output_control(mt9p031, 0,
+					MT9P031_OUTPUT_CONTROL_SYN);
+	if (ret < 0)
+		return ret;
+
+	/* Blanking and start values - default... */
+	ret = reg_write(client, MT9P031_H_BLANKING, mt9p031->h_blanking);
+	if (ret < 0)
+		return ret;
+
+	ret = reg_write(client, MT9P031_V_BLANKING, mt9p031->v_blanking);
+	if (ret < 0)
+		return ret;
+
+	ret = reg_write(client, MT9P031_COLUMN_ADDRESS_MODE,
+				mt9p031->column_address_mode);
+	if (ret < 0)
+		return ret;
+
+	ret = reg_write(client, MT9P031_ROW_ADDRESS_MODE,
+				mt9p031->row_address_mode);
+	if (ret < 0)
+		return ret;
+
+	ret = reg_write(client, MT9P031_COLUMN_START,
+				mt9p031->column_start);
+	if (ret < 0)
+		return ret;
+
+	ret = reg_write(client, MT9P031_ROW_START,
+				mt9p031->row_start);
+	if (ret < 0)
+		return ret;
+
+	ret = reg_write(client, MT9P031_WINDOW_WIDTH,
+				mt9p031->window_width);
+	if (ret < 0)
+		return ret;
+
+	ret = reg_write(client, MT9P031_WINDOW_HEIGHT,
+				mt9p031->window_height);
+	if (ret < 0)
+		return ret;
+
+	/* Re-enable register update, commit all changes */
+	ret = mt9p031_set_output_control(mt9p031,
+					MT9P031_OUTPUT_CONTROL_SYN, 0);
+	if (ret < 0)
+		return ret;
+	return 0;
+}
+
+static int mt9p031_reset(struct i2c_client *client)
+{
+	struct mt9p031 *mt9p031 = to_mt9p031(client);
+	int ret;
+
+	/* Disable chip output, synchronous option update */
+	ret = reg_write(client, MT9P031_RST, MT9P031_RST_ENABLE);
+	if (ret < 0)
+		return ret;
+	ret = reg_write(client, MT9P031_RST, MT9P031_RST_DISABLE);
+	if (ret < 0)
+		return ret;
+	return mt9p031_set_output_control(mt9p031,
+					MT9P031_OUTPUT_CONTROL_CEN, 0);
+}
+
+static int mt9p031_power_on(struct mt9p031 *mt9p031)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(&mt9p031->subdev);
+	int ret;
+
+	/* Ensure RESET_BAR is low */
+	if (mt9p031->pdata->reset)
+		mt9p031->pdata->reset(&mt9p031->subdev, 1);
+	/* turn on digital supply first */
+	ret = regulator_enable(mt9p031->reg_1v8);
+	if (ret) {
+		dev_err(&client->dev,
+			"Failed to enable 1.8v regulator: %d\n", ret);
+		goto err_1v8;
+	}
+	/* now turn on analog supply */
+	ret = regulator_enable(mt9p031->reg_2v8);
+	if (ret) {
+		dev_err(&client->dev,
+			"Failed to enable 2.8v regulator: %d\n", ret);
+		goto err_rst;
+	}
+	/* Now RESET_BAR must be high */
+	if (mt9p031->pdata->reset)
+		mt9p031->pdata->reset(&mt9p031->subdev, 0);
+	
+	if (mt9p031->pdata->set_xclk)
+		mt9p031->pdata->set_xclk(&mt9p031->subdev, MT9P031_PIXCLK_FREQ);
+
+	/* soft reset */
+	ret = mt9p031_reset(client);
+	if (ret < 0) {
+		dev_err(&client->dev, "Failed to reset the camera\n");
+		goto err_rst;
+	}
+
+	ret = restore_registers(client);
+	if (ret < 0) {
+		dev_err(&client->dev, "Failed to restore registers\n");
+		goto err_rst;
+	}
+
+	return 0;
+err_rst:
+	regulator_disable(mt9p031->reg_1v8);
+err_1v8:
+	return ret;
+	
+}
+
+static void mt9p031_power_off(struct mt9p031 *mt9p031)
+{
+	if (mt9p031->pdata->set_xclk)
+		mt9p031->pdata->set_xclk(&mt9p031->subdev, 0);
+	if (mt9p031->pdata->reset)
+		mt9p031->pdata->reset(&mt9p031->subdev, 1);
+	regulator_disable(mt9p031->reg_1v8);
+	regulator_disable(mt9p031->reg_2v8);
+}
+
+static int mt9p031_enum_mbus_code(struct v4l2_subdev *sd,
+				struct v4l2_subdev_fh *fh,
+				struct v4l2_subdev_mbus_code_enum *code)
+{
+	struct mt9p031 *mt9p031 = container_of(sd, struct mt9p031, subdev);
+
+	if (code->pad || code->index)
+		return -EINVAL;
+
+	code->code = mt9p031->format.code;
+
+	return 0;
+}
+
+static struct v4l2_mbus_framefmt *mt9p031_get_pad_format(
+			struct mt9p031 *mt9p031,
+			struct v4l2_subdev_fh *fh,
+			unsigned int pad, u32 which)
+{
+	switch (which) {
+	case V4L2_SUBDEV_FORMAT_TRY:
+		return v4l2_subdev_get_try_format(fh, pad);
+	case V4L2_SUBDEV_FORMAT_ACTIVE:
+		return &mt9p031->format;
+	default:
+		return NULL;
+	}
+}
+
+static struct v4l2_rect *mt9p031_get_pad_crop(struct mt9p031 *mt9p031,
+			struct v4l2_subdev_fh *fh, unsigned int pad, u32 which)
+{
+	switch (which) {
+	case V4L2_SUBDEV_FORMAT_TRY:
+		return v4l2_subdev_get_try_crop(fh, pad);
+	case V4L2_SUBDEV_FORMAT_ACTIVE:
+		return &mt9p031->rect;
+	default:
+		return NULL;
+	}
+}
+
+static int mt9p031_get_crop(struct v4l2_subdev *sd,
+				struct v4l2_subdev_fh *fh,
+				struct v4l2_subdev_crop *crop)
+{
+	struct mt9p031 *mt9p031 = container_of(sd, struct mt9p031, subdev);
+	struct v4l2_rect *rect = mt9p031_get_pad_crop(mt9p031, fh, crop->pad,
+							crop->which);
+	if (!rect)
+		return -EINVAL;
+
+	crop->rect = *rect;
+
+	return 0;
+}
+
+static u16 mt9p031_skip_for_crop(s32 source, s32 *target, s32 max_skip)
+{
+	unsigned int skip;
+
+	if (source - source / 4 < *target) {
+		*target = source;
+		return 1;
+	}
+
+	skip = DIV_ROUND_CLOSEST(source, *target);
+	if (skip > max_skip)
+		skip = max_skip;
+	*target = 2 * DIV_ROUND_UP(source, 2 * skip);
+
+	return skip;
+}
+
+static int mt9p031_set_params(struct i2c_client *client,
+			      struct v4l2_rect *rect, u16 xskip, u16 yskip)
+{
+	struct mt9p031 *mt9p031 = to_mt9p031(client);
+	int ret;
+	u16 xbin, ybin;
+	const u16 hblank = MT9P031_H_BLANKING_VALUE,
+		vblank = MT9P031_V_BLANKING_VALUE;
+	__s32 left, top, width, height;
+
+	/*
+	 * TODO: Attention! When implementing horizontal flipping, adjust
+	 * alignment according to R2 "Column Start" description in the datasheet
+	 */
+	if (xskip & 1) {
+		xbin = 1;
+		left = rect->left & (~3);
+	} else if (xskip & 2) {
+		xbin = 2;
+		left = rect->left & (~7);
+	} else {
+		xbin = 4;
+		left = rect->left & (~15);
+	}
+	top = rect->top & (~1);
+	width = rect->width;
+	height = rect->height;
+
+	ybin = min(yskip, (u16)4);
+
+	/* Disable register update, reconfigure atomically */
+	ret = mt9p031_set_output_control(mt9p031, 0,
+					MT9P031_OUTPUT_CONTROL_SYN);
+	if (ret < 0)
+		return ret;
+
+	dev_dbg(&client->dev, "skip %u:%u, rect %ux%u@%u:%u\n",
+		xskip, yskip, rect->width, rect->height, rect->left, rect->top);
+
+	/* Blanking and start values - default... */
+	ret = reg_write_cached(client, MT9P031_H_BLANKING, hblank,
+				&mt9p031->h_blanking);
+	if (ret < 0)
+		return ret;
+	ret = reg_write_cached(client, MT9P031_V_BLANKING, vblank,
+				&mt9p031->v_blanking);
+	if (ret < 0)
+		return ret;
+
+	ret = reg_write_cached(client, MT9P031_COLUMN_ADDRESS_MODE,
+				((xbin - 1) << 4) | (xskip - 1),
+				&mt9p031->column_address_mode);
+	if (ret < 0)
+		return ret;
+	ret = reg_write_cached(client, MT9P031_ROW_ADDRESS_MODE,
+				((ybin - 1) << 4) | (yskip - 1),
+				&mt9p031->row_address_mode);
+	if (ret < 0)
+		return ret;
+
+	dev_dbg(&client->dev, "new physical left %u, top %u\n",
+		rect->left, rect->top);
+
+	ret = reg_write_cached(client, MT9P031_COLUMN_START,
+				rect->left + MT9P031_COLUMN_START_DEF,
+				&mt9p031->column_start);
+	if (ret < 0)
+		return ret;
+	ret = reg_write_cached(client, MT9P031_ROW_START,
+				rect->top + MT9P031_ROW_START_DEF,
+				&mt9p031->row_start);
+	if (ret < 0)
+		return ret;
+	ret = reg_write_cached(client, MT9P031_WINDOW_WIDTH,
+				rect->width - 1,
+				&mt9p031->window_width);
+	if (ret < 0)
+		return ret;
+	ret = reg_write_cached(client, MT9P031_WINDOW_HEIGHT,
+				rect->height - 1,
+				&mt9p031->window_height);
+	if (ret < 0)
+		return ret;
+
+	/* Re-enable register update, commit all changes */
+	ret = mt9p031_set_output_control(mt9p031,
+					MT9P031_OUTPUT_CONTROL_SYN, 0);
+	if (ret < 0)
+		return ret;
+
+	mt9p031->xskip = xskip;
+	mt9p031->yskip = yskip;
+	return ret;
+}
+
+static int mt9p031_set_crop(struct v4l2_subdev *sd,
+				struct v4l2_subdev_fh *fh,
+				struct v4l2_subdev_crop *crop)
+{
+	struct mt9p031 *mt9p031 = container_of(sd, struct mt9p031, subdev);
+	struct v4l2_mbus_framefmt *f;
+	struct v4l2_rect *c;
+	struct v4l2_rect rect;
+	u16 xskip, yskip;
+	s32 width, height;
+
+	dev_dbg(mt9p031->subdev.v4l2_dev->dev, "%s(%ux%u@%u:%u : %u)\n",
+			__func__, crop->rect.width, crop->rect.height,
+			crop->rect.left, crop->rect.top, crop->which);
+
+	/*
+	 * Clamp the crop rectangle boundaries and align them to a multiple of 2
+	 * pixels.
+	 */
+	rect.width = ALIGN(clamp(crop->rect.width,
+				 MT9P031_WINDOW_WIDTH_MIN, MT9P031_WINDOW_WIDTH_MAX), 2);
+	rect.height = ALIGN(clamp(crop->rect.height,
+				  MT9P031_WINDOW_HEIGHT_MIN, MT9P031_WINDOW_HEIGHT_MAX), 2);
+	rect.left = ALIGN(clamp(crop->rect.left,
+				0, MT9P031_WINDOW_WIDTH_MAX - rect.width), 2);
+	rect.top = ALIGN(clamp(crop->rect.top,
+			       0, MT9P031_WINDOW_HEIGHT_MAX - rect.height), 2);
+
+	c = mt9p031_get_pad_crop(mt9p031, fh, crop->pad, crop->which);
+
+	if (rect.width != c->width || rect.height != c->height) {
+		/*
+		 * Reset the output image size if the crop rectangle size has
+		 * been modified.
+		 */
+		f = mt9p031_get_pad_format(mt9p031, fh, crop->pad,
+						    crop->which);
+		width = f->width;
+		height = f->height;
+
+		xskip = mt9p031_skip_for_crop(rect.width, &width, 7);
+		yskip = mt9p031_skip_for_crop(rect.height, &height, 8);
+	} else {
+		xskip = mt9p031->xskip;
+		yskip = mt9p031->yskip;
+		f = NULL;
+	}
+	if (f) {
+		f->width = width;
+		f->height = height;
+	}
+
+	*c = rect;
+	crop->rect = rect;
+
+	mt9p031->xskip = xskip;
+	mt9p031->yskip = yskip;
+	mt9p031->rect = *c;
+	return 0;
+}
+
+static int mt9p031_get_format(struct v4l2_subdev *sd,
+				struct v4l2_subdev_fh *fh,
+				struct v4l2_subdev_format *fmt)
+{
+	struct mt9p031 *mt9p031 = container_of(sd, struct mt9p031, subdev);
+
+	fmt->format =
+		*mt9p031_get_pad_format(mt9p031, fh, fmt->pad, fmt->which);
+	return 0;
+}
+
+static u16 mt9p031_skip_for_scale(s32 *source, s32 target,
+					s32 max_skip, s32 max)
+{
+	unsigned int skip;
+
+	if (*source - *source / 4 < target) {
+		*source = target;
+		return 1;
+	}
+
+	skip = min(max, *source + target / 2) / target;
+	if (skip > max_skip)
+		skip = max_skip;
+	*source = target * skip;
+
+	return skip;
+}
+
+static int mt9p031_fmt_validate(struct v4l2_subdev *sd,
+				struct v4l2_subdev_format *fmt)
+{
+	struct v4l2_mbus_framefmt *format = &fmt->format;
+
+	/* Hardcode code and colorspace as sensor only supports one */
+	format->code = V4L2_MBUS_FMT_SGRBG12_1X12;
+	format->colorspace = V4L2_COLORSPACE_SRGB;
+	
+	format->width = clamp_t(int, ALIGN(format->width, 2), 2,
+						MT9P031_WINDOW_WIDTH_MAX);
+	format->height = clamp_t(int, ALIGN(format->height, 2), 2,
+						MT9P031_WINDOW_HEIGHT_MAX);
+	format->field = V4L2_FIELD_NONE;
+
+	return 0;
+}
+
+static int mt9p031_set_format(struct v4l2_subdev *sd,
+				struct v4l2_subdev_fh *fh,
+				struct v4l2_subdev_format *format)
+{
+	struct v4l2_subdev_format sdf = *format;
+	struct v4l2_mbus_framefmt *__format, *format_bak = &sdf.format;
+	struct v4l2_rect *__crop, rect;
+	struct mt9p031 *mt9p031 = container_of(sd, struct mt9p031, subdev);
+	u16 xskip, yskip;
+	int ret;
+
+	__crop = mt9p031_get_pad_crop(mt9p031, fh, format->pad, format->which);
+
+	ret = mt9p031_fmt_validate(sd, &sdf);
+	if (ret < 0)
+		return ret;
+	rect.width = __crop->width;
+	rect.height = __crop->height;
+
+	xskip = mt9p031_skip_for_scale(&rect.width, format_bak->width, 7,
+				       MT9P031_WINDOW_WIDTH_MAX);
+	if (rect.width + __crop->left > MT9P031_WINDOW_WIDTH_MAX)
+		rect.left = (MT9P031_WINDOW_WIDTH_MAX - rect.width) / 2;
+	else
+		rect.left = __crop->left;
+	yskip = mt9p031_skip_for_scale(&rect.height, format_bak->height, 8,
+				       MT9P031_WINDOW_HEIGHT_MAX);
+	if (rect.height + __crop->top > MT9P031_WINDOW_HEIGHT_MAX)
+		rect.top = (MT9P031_WINDOW_HEIGHT_MAX - rect.height) / 2;
+	else
+		rect.top = __crop->top;
+
+	dev_dbg(mt9p031->subdev.v4l2_dev->dev, "%s(%ux%u : %u)\n", __func__,
+		format_bak->width, format_bak->height, format->which);
+	if (__crop)
+		*__crop = rect;
+
+	__format = mt9p031_get_pad_format(mt9p031, fh, format->pad, format->which);
+	*__format = *format_bak;
+	format->format = *format_bak;
+
+	mt9p031->xskip = xskip;
+	mt9p031->yskip = yskip;
+	mt9p031->rect = *__crop;
+	return 0;
+}
+
+static int mt9p031_s_stream(struct v4l2_subdev *sd, int enable)
+{
+	struct mt9p031 *mt9p031 = container_of(sd, struct mt9p031, subdev);
+	struct i2c_client *client = v4l2_get_subdevdata(&mt9p031->subdev);
+	struct v4l2_rect rect = mt9p031->rect;
+	u16 xskip = mt9p031->xskip;
+	u16 yskip = mt9p031->yskip;
+	int ret;
+
+	if (enable) {
+		ret = mt9p031_set_params(client, &rect, xskip, yskip);
+		if (ret < 0)
+			return ret;
+		/* Switch to master "normal" mode */
+		ret = mt9p031_set_output_control(mt9p031, 0,
+						MT9P031_OUTPUT_CONTROL_CEN);
+	} else {
+		/* Stop sensor readout */
+		ret = mt9p031_set_output_control(mt9p031,
+						MT9P031_OUTPUT_CONTROL_CEN, 0);
+	}
+	return ret;
+}
+
+static int mt9p031_video_probe(struct i2c_client *client)
+{
+	s32 data;
+
+	/* Read out the chip version register */
+	data = reg_read(client, MT9P031_CHIP_VERSION);
+	if (data != MT9P031_CHIP_VERSION_VALUE) {
+		dev_err(&client->dev,
+			"No MT9P031 chip detected, register read %x\n", data);
+		return -ENODEV;
+	}
+
+	dev_info(&client->dev, "Detected a MT9P031 chip ID %x\n", data);
+
+	return 0;
+}
+
+static int mt9p031_set_power(struct v4l2_subdev *sd, int on)
+{
+	struct mt9p031 *mt9p031 = container_of(sd, struct mt9p031, subdev);
+	int ret = 0;
+
+	mutex_lock(&mt9p031->power_lock);
+
+	/*
+	 * If the power count is modified from 0 to != 0 or from != 0 to 0,
+	 * update the power state.
+	 */
+	if (mt9p031->power_count == !on) {
+		if (on) {
+			ret = mt9p031_power_on(mt9p031);
+			if (ret) {
+				dev_err(mt9p031->subdev.v4l2_dev->dev,
+				"Failed to enable 2.8v regulator: %d\n", ret);
+				goto out;
+			}
+		} else {
+			mt9p031_power_off(mt9p031);
+		}
+	}
+
+	/* Update the power count. */
+	mt9p031->power_count += on ? 1 : -1;
+	WARN_ON(mt9p031->power_count < 0);
+
+out:
+	mutex_unlock(&mt9p031->power_lock);
+	return ret;
+}
+
+static int mt9p031_registered(struct v4l2_subdev *sd)
+{
+	struct mt9p031 *mt9p031 = container_of(sd, struct mt9p031, subdev);
+	struct i2c_client *client = v4l2_get_subdevdata(&mt9p031->subdev);
+	int ret;
+
+	ret = mt9p031_set_power(&mt9p031->subdev, 1);
+	if (ret) {
+		dev_err(&client->dev,
+			"Failed to power on device: %d\n", ret);
+		goto err_pwron;
+	}
+
+	ret = mt9p031_video_probe(client);
+	if (ret)
+		goto err_evprobe;
+
+	mt9p031->pad.flags = MEDIA_PAD_FL_SOURCE;
+	ret = media_entity_init(&mt9p031->subdev.entity, 1, &mt9p031->pad, 0);
+	if (ret)
+		goto err_evprobe;
+
+	mt9p031->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
+	mt9p031_set_power(&mt9p031->subdev, 0);
+
+	return 0;
+err_evprobe:
+	mt9p031_set_power(&mt9p031->subdev, 0);
+err_pwron:
+	return ret;
+}
+
+static int mt9p031_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
+{
+	struct mt9p031 *mt9p031;
+	mt9p031 = container_of(sd, struct mt9p031, subdev);
+
+	mt9p031->rect.width	= MT9P031_WINDOW_WIDTH_MAX;
+	mt9p031->rect.height	= MT9P031_WINDOW_HEIGHT_MAX;
+	mt9p031->rect.left	= MT9P031_COLUMN_START_DEF;
+	mt9p031->rect.top	= MT9P031_ROW_START_DEF;
+
+	mt9p031->format.code = V4L2_MBUS_FMT_SGRBG12_1X12;
+	mt9p031->format.width = MT9P031_WINDOW_WIDTH_MAX;
+	mt9p031->format.height = MT9P031_WINDOW_HEIGHT_MAX;
+	mt9p031->format.field = V4L2_FIELD_NONE;
+	mt9p031->format.colorspace = V4L2_COLORSPACE_SRGB;
+
+	mt9p031->xskip = 1;
+	mt9p031->yskip = 1;
+	return mt9p031_set_power(sd, 1);
+}
+
+static int mt9p031_close(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
+{
+	return mt9p031_set_power(sd, 0);
+}
+
+static struct v4l2_subdev_core_ops mt9p031_subdev_core_ops = {
+	.s_power	= mt9p031_set_power,
+};
+
+static struct v4l2_subdev_video_ops mt9p031_subdev_video_ops = {
+	.s_stream	= mt9p031_s_stream,
+};
+
+static struct v4l2_subdev_pad_ops mt9p031_subdev_pad_ops = {
+	.enum_mbus_code = mt9p031_enum_mbus_code,
+	.get_fmt = mt9p031_get_format,
+	.set_fmt = mt9p031_set_format,
+	.get_crop = mt9p031_get_crop,
+	.set_crop = mt9p031_set_crop,
+};
+
+static struct v4l2_subdev_ops mt9p031_subdev_ops = {
+	.core	= &mt9p031_subdev_core_ops,
+	.video	= &mt9p031_subdev_video_ops,
+	.pad	= &mt9p031_subdev_pad_ops,
+};
+
+static const struct v4l2_subdev_internal_ops mt9p031_subdev_internal_ops = {
+	.registered = mt9p031_registered,
+	.open = mt9p031_open,
+	.close = mt9p031_close,
+};
+
+static int mt9p031_probe(struct i2c_client *client,
+			 const struct i2c_device_id *did)
+{
+	struct mt9p031 *mt9p031;
+	struct mt9p031_platform_data *pdata = client->dev.platform_data;
+	struct i2c_adapter *adapter = to_i2c_adapter(client->dev.parent);
+	int ret;
+
+	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_WORD_DATA)) {
+		dev_warn(&adapter->dev,
+			 "I2C-Adapter doesn't support I2C_FUNC_SMBUS_WORD\n");
+		return -EIO;
+	}
+
+	mt9p031 = kzalloc(sizeof(struct mt9p031), GFP_KERNEL);
+	if (!mt9p031)
+		return -ENOMEM;
+
+	mutex_init(&mt9p031->power_lock);
+	v4l2_i2c_subdev_init(&mt9p031->subdev, client, &mt9p031_subdev_ops);
+	mt9p031->subdev.internal_ops = &mt9p031_subdev_internal_ops;
+
+	mt9p031->pdata		= pdata;
+
+	mt9p031->reg_1v8 = regulator_get(NULL, "cam_1v8");
+	if (IS_ERR(mt9p031->reg_1v8)) {
+		ret = PTR_ERR(mt9p031->reg_1v8);
+		dev_err(mt9p031->subdev.v4l2_dev->dev,
+			"Failed 1.8v regulator: %d\n", ret);
+		goto err_e1v8;
+	}
+
+	mt9p031->reg_2v8 = regulator_get(NULL, "cam_2v8");
+	if (IS_ERR(mt9p031->reg_2v8)) {
+		ret = PTR_ERR(mt9p031->reg_2v8);
+		dev_err(mt9p031->subdev.v4l2_dev->dev,
+			"Failed 2.8v regulator: %d\n", ret);
+		goto err_e2v8;
+	}
+	return 0;
+err_e2v8:
+	regulator_put(mt9p031->reg_1v8);
+err_e1v8:
+	kfree(mt9p031);
+	return ret;
+}
+
+static int mt9p031_remove(struct i2c_client *client)
+{
+	struct v4l2_subdev *sd = i2c_get_clientdata(client);
+	struct mt9p031 *mt9p031 = container_of(sd, struct mt9p031, subdev);
+
+	v4l2_device_unregister_subdev(sd);
+	media_entity_cleanup(&sd->entity);
+	regulator_put(mt9p031->reg_2v8);
+	regulator_put(mt9p031->reg_1v8);
+	kfree(mt9p031);
+
+	return 0;
+}
+
+static const struct i2c_device_id mt9p031_id[] = {
+	{ "mt9p031", 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, mt9p031_id);
+
+static struct i2c_driver mt9p031_i2c_driver = {
+	.driver = {
+		.name = "mt9p031",
+	},
+	.probe		= mt9p031_probe,
+	.remove		= mt9p031_remove,
+	.id_table	= mt9p031_id,
+};
+
+static int __init mt9p031_mod_init(void)
+{
+	return i2c_add_driver(&mt9p031_i2c_driver);
+}
+
+static void __exit mt9p031_mod_exit(void)
+{
+	i2c_del_driver(&mt9p031_i2c_driver);
+}
+
+module_init(mt9p031_mod_init);
+module_exit(mt9p031_mod_exit);
+
+MODULE_DESCRIPTION("Aptina MT9P031 Camera driver");
+MODULE_AUTHOR("Bastian Hecht <hechtb@gmail.com>");
+MODULE_LICENSE("GPL v2");
diff --git a/include/media/mt9p031.h b/include/media/mt9p031.h
new file mode 100644
index 0000000..ad37eb3
--- /dev/null
+++ b/include/media/mt9p031.h
@@ -0,0 +1,11 @@
+#ifndef MT9P031_H
+#define MT9P031_H
+
+struct v4l2_subdev;
+
+struct mt9p031_platform_data {
+	int (*set_xclk)(struct v4l2_subdev *subdev, int hz);
+	int (*reset)(struct v4l2_subdev *subdev, int active);
+};
+
+#endif
-- 
1.7.0.4

[PATCH][RFC] Add mt9p031 sensor support.

[Javier Martin]

This RFC includes a power management implementation that causes
the sensor to show images with horizontal artifacts (usually
monochrome lines that appear on the image randomly).

Signed-off-by: Javier Martin <javier.martin@vista-silicon.com>
---
 drivers/media/video/Kconfig   |    7 +
 drivers/media/video/Makefile  |    1 +
 drivers/media/video/mt9p031.c |  841 +++++++++++++++++++++++++++++++++++++++++
 include/media/mt9p031.h       |   11 +
 4 files changed, 860 insertions(+), 0 deletions(-)
 create mode 100644 drivers/media/video/mt9p031.c
 create mode 100644 include/media/mt9p031.h

diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig
index 00f51dd..8a596cc 100644
--- a/drivers/media/video/Kconfig
+++ b/drivers/media/video/Kconfig
@@ -329,6 +329,13 @@ config VIDEO_OV7670
 	  OV7670 VGA camera.  It currently only works with the M88ALP01
 	  controller.
 
+config VIDEO_MT9P031
+	tristate "Aptina MT9P031 support"
+	depends on I2C && VIDEO_V4L2
+	---help---
+	 This is a Video4Linux2 sensor-level driver for the Aptina
+	 (Micron) mt9p031 5 Mpixel camera.
+
 config VIDEO_MT9V011
 	tristate "Micron mt9v011 sensor support"
 	depends on I2C && VIDEO_V4L2
diff --git a/drivers/media/video/Makefile b/drivers/media/video/Makefile
index ace5d8b..912b29b 100644
--- a/drivers/media/video/Makefile
+++ b/drivers/media/video/Makefile
@@ -65,6 +65,7 @@ obj-$(CONFIG_VIDEO_UPD64083) += upd64083.o
 obj-$(CONFIG_VIDEO_OV7670) 	+= ov7670.o
 obj-$(CONFIG_VIDEO_TCM825X) += tcm825x.o
 obj-$(CONFIG_VIDEO_TVEEPROM) += tveeprom.o
+obj-$(CONFIG_VIDEO_MT9P031) += mt9p031.o
 obj-$(CONFIG_VIDEO_MT9V011) += mt9v011.o
 obj-$(CONFIG_VIDEO_SR030PC30)	+= sr030pc30.o
 obj-$(CONFIG_VIDEO_NOON010PC30)	+= noon010pc30.o
diff --git a/drivers/media/video/mt9p031.c b/drivers/media/video/mt9p031.c
new file mode 100644
index 0000000..04d8812
--- /dev/null
+++ b/drivers/media/video/mt9p031.c
@@ -0,0 +1,841 @@
+/*
+ * Driver for MT9P031 CMOS Image Sensor from Aptina
+ *
+ * Copyright (C) 2011, Javier Martin <javier.martin@vista-silicon.com>
+ *
+ * Copyright (C) 2011, Guennadi Liakhovetski <g.liakhovetski@gmx.de>
+ *
+ * Based on the MT9V032 driver and Bastian Hecht's code.
+ *
+ * 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.
+ */
+
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/i2c.h>
+#include <linux/log2.h>
+#include <linux/pm.h>
+#include <linux/regulator/consumer.h>
+#include <linux/slab.h>
+#include <media/v4l2-subdev.h>
+#include <linux/videodev2.h>
+
+#include <media/mt9p031.h>
+#include <media/v4l2-chip-ident.h>
+#include <media/v4l2-subdev.h>
+#include <media/v4l2-device.h>
+
+#define MT9P031_PIXCLK_FREQ			54000000
+
+/* mt9p031 selected register addresses */
+#define MT9P031_CHIP_VERSION			0x00
+#define		MT9P031_CHIP_VERSION_VALUE	0x1801
+#define MT9P031_ROW_START			0x01
+#define		MT9P031_ROW_START_DEF		54
+#define MT9P031_COLUMN_START			0x02
+#define		MT9P031_COLUMN_START_DEF	16
+#define MT9P031_WINDOW_HEIGHT			0x03
+#define MT9P031_WINDOW_WIDTH			0x04
+#define MT9P031_H_BLANKING			0x05
+#define		MT9P031_H_BLANKING_VALUE	0
+#define MT9P031_V_BLANKING			0x06
+#define		MT9P031_V_BLANKING_VALUE	25
+#define MT9P031_OUTPUT_CONTROL			0x07
+#define		MT9P031_OUTPUT_CONTROL_CEN	2
+#define		MT9P031_OUTPUT_CONTROL_SYN	1
+#define MT9P031_SHUTTER_WIDTH_UPPER		0x08
+#define MT9P031_SHUTTER_WIDTH			0x09
+#define MT9P031_PIXEL_CLOCK_CONTROL		0x0a
+#define MT9P031_FRAME_RESTART			0x0b
+#define MT9P031_SHUTTER_DELAY			0x0c
+#define MT9P031_RST				0x0d
+#define		MT9P031_RST_ENABLE		1
+#define		MT9P031_RST_DISABLE		0
+#define MT9P031_READ_MODE_1			0x1e
+#define MT9P031_READ_MODE_2			0x20
+#define		MT9P031_READ_MODE_2_ROW_MIR	0x8000
+#define		MT9P031_READ_MODE_2_COL_MIR	0x4000
+#define MT9P031_ROW_ADDRESS_MODE		0x22
+#define MT9P031_COLUMN_ADDRESS_MODE		0x23
+#define MT9P031_GLOBAL_GAIN			0x35
+
+#define MT9P031_WINDOW_HEIGHT_MAX		1944
+#define MT9P031_WINDOW_WIDTH_MAX		2592
+#define MT9P031_WINDOW_HEIGHT_MIN		2
+#define MT9P031_WINDOW_WIDTH_MIN		18
+
+struct mt9p031 {
+	struct v4l2_subdev subdev;
+	struct media_pad pad;
+	struct v4l2_rect rect;	/* Sensor window */
+	struct v4l2_mbus_framefmt format;
+	struct mt9p031_platform_data *pdata;
+	struct mutex power_lock; /* lock to protect power_count */
+	int power_count;
+	u16 xskip;
+	u16 yskip;
+	/* cache register values */
+	u16 output_control;
+	u16 h_blanking;
+	u16 v_blanking;
+	u16 column_address_mode;
+	u16 row_address_mode;
+	u16 column_start;
+	u16 row_start;
+	u16 window_width;
+	u16 window_height;
+	struct regulator *reg_1v8;
+	struct regulator *reg_2v8;
+};
+
+static struct mt9p031 *to_mt9p031(const struct i2c_client *client)
+{
+	return container_of(i2c_get_clientdata(client), struct mt9p031, subdev);
+}
+
+static int reg_read(struct i2c_client *client, const u8 reg)
+{
+	s32 data = i2c_smbus_read_word_data(client, reg);
+	return data < 0 ? data : swab16(data);
+}
+
+static int reg_write(struct i2c_client *client, const u8 reg,
+			const u16 data)
+{
+	return i2c_smbus_write_word_data(client, reg, swab16(data));
+}
+
+static int reg_write_cached(struct i2c_client *client, const u8 reg,
+			const u16 data, u16 *cache)
+{
+	int ret;
+
+	ret = reg_write(client, reg, data);
+	if (ret < 0)
+		return ret;
+	*cache = data;
+	return 0;
+}
+
+static int mt9p031_set_output_control(struct mt9p031 *mt9p031, u16 clear,
+				      u16 set)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(&mt9p031->subdev);
+	u16 value = (mt9p031->output_control & ~clear) | set;
+
+	return reg_write_cached(client, MT9P031_OUTPUT_CONTROL, value,
+				&mt9p031->output_control);
+}
+
+static int restore_registers(struct i2c_client *client)
+{
+	int ret;
+	struct mt9p031 *mt9p031 = to_mt9p031(client);
+
+	/* Disable register update, reconfigure atomically */
+	ret = mt9p031_set_output_control(mt9p031, 0,
+					MT9P031_OUTPUT_CONTROL_SYN);
+	if (ret < 0)
+		return ret;
+
+	/* Blanking and start values - default... */
+	ret = reg_write(client, MT9P031_H_BLANKING, mt9p031->h_blanking);
+	if (ret < 0)
+		return ret;
+
+	ret = reg_write(client, MT9P031_V_BLANKING, mt9p031->v_blanking);
+	if (ret < 0)
+		return ret;
+
+	ret = reg_write(client, MT9P031_COLUMN_ADDRESS_MODE,
+				mt9p031->column_address_mode);
+	if (ret < 0)
+		return ret;
+
+	ret = reg_write(client, MT9P031_ROW_ADDRESS_MODE,
+				mt9p031->row_address_mode);
+	if (ret < 0)
+		return ret;
+
+	ret = reg_write(client, MT9P031_COLUMN_START,
+				mt9p031->column_start);
+	if (ret < 0)
+		return ret;
+
+	ret = reg_write(client, MT9P031_ROW_START,
+				mt9p031->row_start);
+	if (ret < 0)
+		return ret;
+
+	ret = reg_write(client, MT9P031_WINDOW_WIDTH,
+				mt9p031->window_width);
+	if (ret < 0)
+		return ret;
+
+	ret = reg_write(client, MT9P031_WINDOW_HEIGHT,
+				mt9p031->window_height);
+	if (ret < 0)
+		return ret;
+
+	/* Re-enable register update, commit all changes */
+	ret = mt9p031_set_output_control(mt9p031,
+					MT9P031_OUTPUT_CONTROL_SYN, 0);
+	if (ret < 0)
+		return ret;
+	return 0;
+}
+
+static int mt9p031_reset(struct i2c_client *client)
+{
+	struct mt9p031 *mt9p031 = to_mt9p031(client);
+	int ret;
+
+	/* Disable chip output, synchronous option update */
+	ret = reg_write(client, MT9P031_RST, MT9P031_RST_ENABLE);
+	if (ret < 0)
+		return ret;
+	ret = reg_write(client, MT9P031_RST, MT9P031_RST_DISABLE);
+	if (ret < 0)
+		return ret;
+	return mt9p031_set_output_control(mt9p031,
+					MT9P031_OUTPUT_CONTROL_CEN, 0);
+}
+
+static int mt9p031_power_on(struct mt9p031 *mt9p031)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(&mt9p031->subdev);
+	int ret;
+
+	/* Ensure RESET_BAR is low */
+	if (mt9p031->pdata->reset)
+		mt9p031->pdata->reset(&mt9p031->subdev, 1);
+	/* turn on digital supply first */
+	ret = regulator_enable(mt9p031->reg_1v8);
+	if (ret) {
+		dev_err(&client->dev,
+			"Failed to enable 1.8v regulator: %d\n", ret);
+		goto err_1v8;
+	}
+	/* now turn on analog supply */
+	ret = regulator_enable(mt9p031->reg_2v8);
+	if (ret) {
+		dev_err(&client->dev,
+			"Failed to enable 2.8v regulator: %d\n", ret);
+		goto err_rst;
+	}
+	/* Now RESET_BAR must be high */
+	if (mt9p031->pdata->reset)
+		mt9p031->pdata->reset(&mt9p031->subdev, 0);
+	
+	if (mt9p031->pdata->set_xclk)
+		mt9p031->pdata->set_xclk(&mt9p031->subdev, MT9P031_PIXCLK_FREQ);
+
+	/* soft reset */
+	ret = mt9p031_reset(client);
+	if (ret < 0) {
+		dev_err(&client->dev, "Failed to reset the camera\n");
+		goto err_rst;
+	}
+
+	ret = restore_registers(client);
+	if (ret < 0) {
+		dev_err(&client->dev, "Failed to restore registers\n");
+		goto err_rst;
+	}
+
+	return 0;
+err_rst:
+	regulator_disable(mt9p031->reg_1v8);
+err_1v8:
+	return ret;
+	
+}
+
+static void mt9p031_power_off(struct mt9p031 *mt9p031)
+{
+	if (mt9p031->pdata->set_xclk)
+		mt9p031->pdata->set_xclk(&mt9p031->subdev, 0);
+	if (mt9p031->pdata->reset)
+		mt9p031->pdata->reset(&mt9p031->subdev, 1);
+	regulator_disable(mt9p031->reg_1v8);
+	regulator_disable(mt9p031->reg_2v8);
+}
+
+static int mt9p031_enum_mbus_code(struct v4l2_subdev *sd,
+				struct v4l2_subdev_fh *fh,
+				struct v4l2_subdev_mbus_code_enum *code)
+{
+	struct mt9p031 *mt9p031 = container_of(sd, struct mt9p031, subdev);
+
+	if (code->pad || code->index)
+		return -EINVAL;
+
+	code->code = mt9p031->format.code;
+
+	return 0;
+}
+
+static struct v4l2_mbus_framefmt *mt9p031_get_pad_format(
+			struct mt9p031 *mt9p031,
+			struct v4l2_subdev_fh *fh,
+			unsigned int pad, u32 which)
+{
+	switch (which) {
+	case V4L2_SUBDEV_FORMAT_TRY:
+		return v4l2_subdev_get_try_format(fh, pad);
+	case V4L2_SUBDEV_FORMAT_ACTIVE:
+		return &mt9p031->format;
+	default:
+		return NULL;
+	}
+}
+
+static struct v4l2_rect *mt9p031_get_pad_crop(struct mt9p031 *mt9p031,
+			struct v4l2_subdev_fh *fh, unsigned int pad, u32 which)
+{
+	switch (which) {
+	case V4L2_SUBDEV_FORMAT_TRY:
+		return v4l2_subdev_get_try_crop(fh, pad);
+	case V4L2_SUBDEV_FORMAT_ACTIVE:
+		return &mt9p031->rect;
+	default:
+		return NULL;
+	}
+}
+
+static int mt9p031_get_crop(struct v4l2_subdev *sd,
+				struct v4l2_subdev_fh *fh,
+				struct v4l2_subdev_crop *crop)
+{
+	struct mt9p031 *mt9p031 = container_of(sd, struct mt9p031, subdev);
+	struct v4l2_rect *rect = mt9p031_get_pad_crop(mt9p031, fh, crop->pad,
+							crop->which);
+	if (!rect)
+		return -EINVAL;
+
+	crop->rect = *rect;
+
+	return 0;
+}
+
+static u16 mt9p031_skip_for_crop(s32 source, s32 *target, s32 max_skip)
+{
+	unsigned int skip;
+
+	if (source - source / 4 < *target) {
+		*target = source;
+		return 1;
+	}
+
+	skip = DIV_ROUND_CLOSEST(source, *target);
+	if (skip > max_skip)
+		skip = max_skip;
+	*target = 2 * DIV_ROUND_UP(source, 2 * skip);
+
+	return skip;
+}
+
+static int mt9p031_set_params(struct i2c_client *client,
+			      struct v4l2_rect *rect, u16 xskip, u16 yskip)
+{
+	struct mt9p031 *mt9p031 = to_mt9p031(client);
+	int ret;
+	u16 xbin, ybin;
+	const u16 hblank = MT9P031_H_BLANKING_VALUE,
+		vblank = MT9P031_V_BLANKING_VALUE;
+	__s32 left, top, width, height;
+
+	/*
+	 * TODO: Attention! When implementing horizontal flipping, adjust
+	 * alignment according to R2 "Column Start" description in the datasheet
+	 */
+	if (xskip & 1) {
+		xbin = 1;
+		left = rect->left & (~3);
+	} else if (xskip & 2) {
+		xbin = 2;
+		left = rect->left & (~7);
+	} else {
+		xbin = 4;
+		left = rect->left & (~15);
+	}
+	top = rect->top & (~1);
+	width = rect->width;
+	height = rect->height;
+
+	ybin = min(yskip, (u16)4);
+
+	/* Disable register update, reconfigure atomically */
+	ret = mt9p031_set_output_control(mt9p031, 0,
+					MT9P031_OUTPUT_CONTROL_SYN);
+	if (ret < 0)
+		return ret;
+
+	dev_dbg(&client->dev, "skip %u:%u, rect %ux%u@%u:%u\n",
+		xskip, yskip, rect->width, rect->height, rect->left, rect->top);
+
+	/* Blanking and start values - default... */
+	ret = reg_write_cached(client, MT9P031_H_BLANKING, hblank,
+				&mt9p031->h_blanking);
+	if (ret < 0)
+		return ret;
+	ret = reg_write_cached(client, MT9P031_V_BLANKING, vblank,
+				&mt9p031->v_blanking);
+	if (ret < 0)
+		return ret;
+
+	ret = reg_write_cached(client, MT9P031_COLUMN_ADDRESS_MODE,
+				((xbin - 1) << 4) | (xskip - 1),
+				&mt9p031->column_address_mode);
+	if (ret < 0)
+		return ret;
+	ret = reg_write_cached(client, MT9P031_ROW_ADDRESS_MODE,
+				((ybin - 1) << 4) | (yskip - 1),
+				&mt9p031->row_address_mode);
+	if (ret < 0)
+		return ret;
+
+	dev_dbg(&client->dev, "new physical left %u, top %u\n",
+		rect->left, rect->top);
+
+	ret = reg_write_cached(client, MT9P031_COLUMN_START,
+				rect->left + MT9P031_COLUMN_START_DEF,
+				&mt9p031->column_start);
+	if (ret < 0)
+		return ret;
+	ret = reg_write_cached(client, MT9P031_ROW_START,
+				rect->top + MT9P031_ROW_START_DEF,
+				&mt9p031->row_start);
+	if (ret < 0)
+		return ret;
+	ret = reg_write_cached(client, MT9P031_WINDOW_WIDTH,
+				rect->width - 1,
+				&mt9p031->window_width);
+	if (ret < 0)
+		return ret;
+	ret = reg_write_cached(client, MT9P031_WINDOW_HEIGHT,
+				rect->height - 1,
+				&mt9p031->window_height);
+	if (ret < 0)
+		return ret;
+
+	/* Re-enable register update, commit all changes */
+	ret = mt9p031_set_output_control(mt9p031,
+					MT9P031_OUTPUT_CONTROL_SYN, 0);
+	if (ret < 0)
+		return ret;
+
+	mt9p031->xskip = xskip;
+	mt9p031->yskip = yskip;
+	return ret;
+}
+
+static int mt9p031_set_crop(struct v4l2_subdev *sd,
+				struct v4l2_subdev_fh *fh,
+				struct v4l2_subdev_crop *crop)
+{
+	struct mt9p031 *mt9p031 = container_of(sd, struct mt9p031, subdev);
+	struct v4l2_mbus_framefmt *f;
+	struct v4l2_rect *c;
+	struct v4l2_rect rect;
+	u16 xskip, yskip;
+	s32 width, height;
+
+	dev_dbg(mt9p031->subdev.v4l2_dev->dev, "%s(%ux%u@%u:%u : %u)\n",
+			__func__, crop->rect.width, crop->rect.height,
+			crop->rect.left, crop->rect.top, crop->which);
+
+	/*
+	 * Clamp the crop rectangle boundaries and align them to a multiple of 2
+	 * pixels.
+	 */
+	rect.width = ALIGN(clamp(crop->rect.width,
+				 MT9P031_WINDOW_WIDTH_MIN, MT9P031_WINDOW_WIDTH_MAX), 2);
+	rect.height = ALIGN(clamp(crop->rect.height,
+				  MT9P031_WINDOW_HEIGHT_MIN, MT9P031_WINDOW_HEIGHT_MAX), 2);
+	rect.left = ALIGN(clamp(crop->rect.left,
+				0, MT9P031_WINDOW_WIDTH_MAX - rect.width), 2);
+	rect.top = ALIGN(clamp(crop->rect.top,
+			       0, MT9P031_WINDOW_HEIGHT_MAX - rect.height), 2);
+
+	c = mt9p031_get_pad_crop(mt9p031, fh, crop->pad, crop->which);
+
+	if (rect.width != c->width || rect.height != c->height) {
+		/*
+		 * Reset the output image size if the crop rectangle size has
+		 * been modified.
+		 */
+		f = mt9p031_get_pad_format(mt9p031, fh, crop->pad,
+						    crop->which);
+		width = f->width;
+		height = f->height;
+
+		xskip = mt9p031_skip_for_crop(rect.width, &width, 7);
+		yskip = mt9p031_skip_for_crop(rect.height, &height, 8);
+	} else {
+		xskip = mt9p031->xskip;
+		yskip = mt9p031->yskip;
+		f = NULL;
+	}
+	if (f) {
+		f->width = width;
+		f->height = height;
+	}
+
+	*c = rect;
+	crop->rect = rect;
+
+	mt9p031->xskip = xskip;
+	mt9p031->yskip = yskip;
+	mt9p031->rect = *c;
+	return 0;
+}
+
+static int mt9p031_get_format(struct v4l2_subdev *sd,
+				struct v4l2_subdev_fh *fh,
+				struct v4l2_subdev_format *fmt)
+{
+	struct mt9p031 *mt9p031 = container_of(sd, struct mt9p031, subdev);
+
+	fmt->format =
+		*mt9p031_get_pad_format(mt9p031, fh, fmt->pad, fmt->which);
+	return 0;
+}
+
+static u16 mt9p031_skip_for_scale(s32 *source, s32 target,
+					s32 max_skip, s32 max)
+{
+	unsigned int skip;
+
+	if (*source - *source / 4 < target) {
+		*source = target;
+		return 1;
+	}
+
+	skip = min(max, *source + target / 2) / target;
+	if (skip > max_skip)
+		skip = max_skip;
+	*source = target * skip;
+
+	return skip;
+}
+
+static int mt9p031_fmt_validate(struct v4l2_subdev *sd,
+				struct v4l2_subdev_format *fmt)
+{
+	struct v4l2_mbus_framefmt *format = &fmt->format;
+
+	/* Hardcode code and colorspace as sensor only supports one */
+	format->code = V4L2_MBUS_FMT_SGRBG12_1X12;
+	format->colorspace = V4L2_COLORSPACE_SRGB;
+	
+	format->width = clamp_t(int, ALIGN(format->width, 2), 2,
+						MT9P031_WINDOW_WIDTH_MAX);
+	format->height = clamp_t(int, ALIGN(format->height, 2), 2,
+						MT9P031_WINDOW_HEIGHT_MAX);
+	format->field = V4L2_FIELD_NONE;
+
+	return 0;
+}
+
+static int mt9p031_set_format(struct v4l2_subdev *sd,
+				struct v4l2_subdev_fh *fh,
+				struct v4l2_subdev_format *format)
+{
+	struct v4l2_subdev_format sdf = *format;
+	struct v4l2_mbus_framefmt *__format, *format_bak = &sdf.format;
+	struct v4l2_rect *__crop, rect;
+	struct mt9p031 *mt9p031 = container_of(sd, struct mt9p031, subdev);
+	u16 xskip, yskip;
+	int ret;
+
+	__crop = mt9p031_get_pad_crop(mt9p031, fh, format->pad, format->which);
+
+	ret = mt9p031_fmt_validate(sd, &sdf);
+	if (ret < 0)
+		return ret;
+	rect.width = __crop->width;
+	rect.height = __crop->height;
+
+	xskip = mt9p031_skip_for_scale(&rect.width, format_bak->width, 7,
+				       MT9P031_WINDOW_WIDTH_MAX);
+	if (rect.width + __crop->left > MT9P031_WINDOW_WIDTH_MAX)
+		rect.left = (MT9P031_WINDOW_WIDTH_MAX - rect.width) / 2;
+	else
+		rect.left = __crop->left;
+	yskip = mt9p031_skip_for_scale(&rect.height, format_bak->height, 8,
+				       MT9P031_WINDOW_HEIGHT_MAX);
+	if (rect.height + __crop->top > MT9P031_WINDOW_HEIGHT_MAX)
+		rect.top = (MT9P031_WINDOW_HEIGHT_MAX - rect.height) / 2;
+	else
+		rect.top = __crop->top;
+
+	dev_dbg(mt9p031->subdev.v4l2_dev->dev, "%s(%ux%u : %u)\n", __func__,
+		format_bak->width, format_bak->height, format->which);
+	if (__crop)
+		*__crop = rect;
+
+	__format = mt9p031_get_pad_format(mt9p031, fh, format->pad, format->which);
+	*__format = *format_bak;
+	format->format = *format_bak;
+
+	mt9p031->xskip = xskip;
+	mt9p031->yskip = yskip;
+	mt9p031->rect = *__crop;
+	return 0;
+}
+
+static int mt9p031_s_stream(struct v4l2_subdev *sd, int enable)
+{
+	struct mt9p031 *mt9p031 = container_of(sd, struct mt9p031, subdev);
+	struct i2c_client *client = v4l2_get_subdevdata(&mt9p031->subdev);
+	struct v4l2_rect rect = mt9p031->rect;
+	u16 xskip = mt9p031->xskip;
+	u16 yskip = mt9p031->yskip;
+	int ret;
+
+	if (enable) {
+		ret = mt9p031_set_params(client, &rect, xskip, yskip);
+		if (ret < 0)
+			return ret;
+		/* Switch to master "normal" mode */
+		ret = mt9p031_set_output_control(mt9p031, 0,
+						MT9P031_OUTPUT_CONTROL_CEN);
+	} else {
+		/* Stop sensor readout */
+		ret = mt9p031_set_output_control(mt9p031,
+						MT9P031_OUTPUT_CONTROL_CEN, 0);
+	}
+	return ret;
+}
+
+static int mt9p031_video_probe(struct i2c_client *client)
+{
+	s32 data;
+
+	/* Read out the chip version register */
+	data = reg_read(client, MT9P031_CHIP_VERSION);
+	if (data != MT9P031_CHIP_VERSION_VALUE) {
+		dev_err(&client->dev,
+			"No MT9P031 chip detected, register read %x\n", data);
+		return -ENODEV;
+	}
+
+	dev_info(&client->dev, "Detected a MT9P031 chip ID %x\n", data);
+
+	return 0;
+}
+
+static int mt9p031_set_power(struct v4l2_subdev *sd, int on)
+{
+	struct mt9p031 *mt9p031 = container_of(sd, struct mt9p031, subdev);
+	int ret = 0;
+
+	mutex_lock(&mt9p031->power_lock);
+
+	/*
+	 * If the power count is modified from 0 to != 0 or from != 0 to 0,
+	 * update the power state.
+	 */
+	if (mt9p031->power_count == !on) {
+		if (on) {
+			ret = mt9p031_power_on(mt9p031);
+			if (ret) {
+				dev_err(mt9p031->subdev.v4l2_dev->dev,
+				"Failed to enable 2.8v regulator: %d\n", ret);
+				goto out;
+			}
+		} else {
+			mt9p031_power_off(mt9p031);
+		}
+	}
+
+	/* Update the power count. */
+	mt9p031->power_count += on ? 1 : -1;
+	WARN_ON(mt9p031->power_count < 0);
+
+out:
+	mutex_unlock(&mt9p031->power_lock);
+	return ret;
+}
+
+static int mt9p031_registered(struct v4l2_subdev *sd)
+{
+	struct mt9p031 *mt9p031 = container_of(sd, struct mt9p031, subdev);
+	struct i2c_client *client = v4l2_get_subdevdata(&mt9p031->subdev);
+	int ret;
+
+	ret = mt9p031_set_power(&mt9p031->subdev, 1);
+	if (ret) {
+		dev_err(&client->dev,
+			"Failed to power on device: %d\n", ret);
+		goto err_pwron;
+	}
+
+	ret = mt9p031_video_probe(client);
+	if (ret)
+		goto err_evprobe;
+
+	mt9p031->pad.flags = MEDIA_PAD_FL_SOURCE;
+	ret = media_entity_init(&mt9p031->subdev.entity, 1, &mt9p031->pad, 0);
+	if (ret)
+		goto err_evprobe;
+
+	mt9p031->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
+	mt9p031_set_power(&mt9p031->subdev, 0);
+
+	return 0;
+err_evprobe:
+	mt9p031_set_power(&mt9p031->subdev, 0);
+err_pwron:
+	return ret;
+}
+
+static int mt9p031_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
+{
+	struct mt9p031 *mt9p031;
+	mt9p031 = container_of(sd, struct mt9p031, subdev);
+
+	mt9p031->rect.width	= MT9P031_WINDOW_WIDTH_MAX;
+	mt9p031->rect.height	= MT9P031_WINDOW_HEIGHT_MAX;
+	mt9p031->rect.left	= MT9P031_COLUMN_START_DEF;
+	mt9p031->rect.top	= MT9P031_ROW_START_DEF;
+
+	mt9p031->format.code = V4L2_MBUS_FMT_SGRBG12_1X12;
+	mt9p031->format.width = MT9P031_WINDOW_WIDTH_MAX;
+	mt9p031->format.height = MT9P031_WINDOW_HEIGHT_MAX;
+	mt9p031->format.field = V4L2_FIELD_NONE;
+	mt9p031->format.colorspace = V4L2_COLORSPACE_SRGB;
+
+	mt9p031->xskip = 1;
+	mt9p031->yskip = 1;
+	return mt9p031_set_power(sd, 1);
+}
+
+static int mt9p031_close(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
+{
+	return mt9p031_set_power(sd, 0);
+}
+
+static struct v4l2_subdev_core_ops mt9p031_subdev_core_ops = {
+	.s_power	= mt9p031_set_power,
+};
+
+static struct v4l2_subdev_video_ops mt9p031_subdev_video_ops = {
+	.s_stream	= mt9p031_s_stream,
+};
+
+static struct v4l2_subdev_pad_ops mt9p031_subdev_pad_ops = {
+	.enum_mbus_code = mt9p031_enum_mbus_code,
+	.get_fmt = mt9p031_get_format,
+	.set_fmt = mt9p031_set_format,
+	.get_crop = mt9p031_get_crop,
+	.set_crop = mt9p031_set_crop,
+};
+
+static struct v4l2_subdev_ops mt9p031_subdev_ops = {
+	.core	= &mt9p031_subdev_core_ops,
+	.video	= &mt9p031_subdev_video_ops,
+	.pad	= &mt9p031_subdev_pad_ops,
+};
+
+static const struct v4l2_subdev_internal_ops mt9p031_subdev_internal_ops = {
+	.registered = mt9p031_registered,
+	.open = mt9p031_open,
+	.close = mt9p031_close,
+};
+
+static int mt9p031_probe(struct i2c_client *client,
+			 const struct i2c_device_id *did)
+{
+	struct mt9p031 *mt9p031;
+	struct mt9p031_platform_data *pdata = client->dev.platform_data;
+	struct i2c_adapter *adapter = to_i2c_adapter(client->dev.parent);
+	int ret;
+
+	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_WORD_DATA)) {
+		dev_warn(&adapter->dev,
+			 "I2C-Adapter doesn't support I2C_FUNC_SMBUS_WORD\n");
+		return -EIO;
+	}
+
+	mt9p031 = kzalloc(sizeof(struct mt9p031), GFP_KERNEL);
+	if (!mt9p031)
+		return -ENOMEM;
+
+	mutex_init(&mt9p031->power_lock);
+	v4l2_i2c_subdev_init(&mt9p031->subdev, client, &mt9p031_subdev_ops);
+	mt9p031->subdev.internal_ops = &mt9p031_subdev_internal_ops;
+
+	mt9p031->pdata		= pdata;
+
+	mt9p031->reg_1v8 = regulator_get(NULL, "cam_1v8");
+	if (IS_ERR(mt9p031->reg_1v8)) {
+		ret = PTR_ERR(mt9p031->reg_1v8);
+		dev_err(mt9p031->subdev.v4l2_dev->dev,
+			"Failed 1.8v regulator: %d\n", ret);
+		goto err_e1v8;
+	}
+
+	mt9p031->reg_2v8 = regulator_get(NULL, "cam_2v8");
+	if (IS_ERR(mt9p031->reg_2v8)) {
+		ret = PTR_ERR(mt9p031->reg_2v8);
+		dev_err(mt9p031->subdev.v4l2_dev->dev,
+			"Failed 2.8v regulator: %d\n", ret);
+		goto err_e2v8;
+	}
+	return 0;
+err_e2v8:
+	regulator_put(mt9p031->reg_1v8);
+err_e1v8:
+	kfree(mt9p031);
+	return ret;
+}
+
+static int mt9p031_remove(struct i2c_client *client)
+{
+	struct v4l2_subdev *sd = i2c_get_clientdata(client);
+	struct mt9p031 *mt9p031 = container_of(sd, struct mt9p031, subdev);
+
+	v4l2_device_unregister_subdev(sd);
+	media_entity_cleanup(&sd->entity);
+	regulator_put(mt9p031->reg_2v8);
+	regulator_put(mt9p031->reg_1v8);
+	kfree(mt9p031);
+
+	return 0;
+}
+
+static const struct i2c_device_id mt9p031_id[] = {
+	{ "mt9p031", 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, mt9p031_id);
+
+static struct i2c_driver mt9p031_i2c_driver = {
+	.driver = {
+		.name = "mt9p031",
+	},
+	.probe		= mt9p031_probe,
+	.remove		= mt9p031_remove,
+	.id_table	= mt9p031_id,
+};
+
+static int __init mt9p031_mod_init(void)
+{
+	return i2c_add_driver(&mt9p031_i2c_driver);
+}
+
+static void __exit mt9p031_mod_exit(void)
+{
+	i2c_del_driver(&mt9p031_i2c_driver);
+}
+
+module_init(mt9p031_mod_init);
+module_exit(mt9p031_mod_exit);
+
+MODULE_DESCRIPTION("Aptina MT9P031 Camera driver");
+MODULE_AUTHOR("Bastian Hecht <hechtb@gmail.com>");
+MODULE_LICENSE("GPL v2");
diff --git a/include/media/mt9p031.h b/include/media/mt9p031.h
new file mode 100644
index 0000000..ad37eb3
--- /dev/null
+++ b/include/media/mt9p031.h
@@ -0,0 +1,11 @@
+#ifndef MT9P031_H
+#define MT9P031_H
+
+struct v4l2_subdev;
+
+struct mt9p031_platform_data {
+	int (*set_xclk)(struct v4l2_subdev *subdev, int hz);
+	int (*reset)(struct v4l2_subdev *subdev, int active);
+};
+
+#endif
-- 
1.7.0.4

Re: [PATCH][RFC] Add mt9p031 sensor support.

[Laurent Pinchart]

Hi Javier,

Thanks for the patch. Here's a review of the power handling code.

On Tuesday 24 May 2011 16:30:43 Javier Martin wrote:
> This RFC includes a power management implementation that causes
> the sensor to show images with horizontal artifacts (usually
> monochrome lines that appear on the image randomly).
> 
> Signed-off-by: Javier Martin <javier.martin@vista-silicon.com>

[snip]

> diff --git a/drivers/media/video/mt9p031.c b/drivers/media/video/mt9p031.c
> new file mode 100644
> index 0000000..04d8812
> --- /dev/null
> +++ b/drivers/media/video/mt9p031.c

[snip]


> @@ -0,0 +1,841 @@
> +/*
> + * Driver for MT9P031 CMOS Image Sensor from Aptina
> + *
> + * Copyright (C) 2011, Javier Martin <javier.martin@vista-silicon.com>
> + *
> + * Copyright (C) 2011, Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> + *
> + * Based on the MT9V032 driver and Bastian Hecht's code.
> + *
> + * 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.
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/i2c.h>
> +#include <linux/log2.h>
> +#include <linux/pm.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/slab.h>
> +#include <media/v4l2-subdev.h>
> +#include <linux/videodev2.h>
> +
> +#include <media/mt9p031.h>
> +#include <media/v4l2-chip-ident.h>

This header is not needed anymore.

> +#include <media/v4l2-subdev.h>
> +#include <media/v4l2-device.h>
> +
> +#define MT9P031_PIXCLK_FREQ			54000000
> +
> +/* mt9p031 selected register addresses */
> +#define MT9P031_CHIP_VERSION			0x00
> +#define		MT9P031_CHIP_VERSION_VALUE	0x1801
> +#define MT9P031_ROW_START			0x01
> +#define		MT9P031_ROW_START_DEF		54
> +#define MT9P031_COLUMN_START			0x02
> +#define		MT9P031_COLUMN_START_DEF	16
> +#define MT9P031_WINDOW_HEIGHT			0x03
> +#define MT9P031_WINDOW_WIDTH			0x04
> +#define MT9P031_H_BLANKING			0x05
> +#define		MT9P031_H_BLANKING_VALUE	0
> +#define MT9P031_V_BLANKING			0x06
> +#define		MT9P031_V_BLANKING_VALUE	25
> +#define MT9P031_OUTPUT_CONTROL			0x07
> +#define		MT9P031_OUTPUT_CONTROL_CEN	2
> +#define		MT9P031_OUTPUT_CONTROL_SYN	1
> +#define MT9P031_SHUTTER_WIDTH_UPPER		0x08
> +#define MT9P031_SHUTTER_WIDTH			0x09
> +#define MT9P031_PIXEL_CLOCK_CONTROL		0x0a
> +#define MT9P031_FRAME_RESTART			0x0b
> +#define MT9P031_SHUTTER_DELAY			0x0c
> +#define MT9P031_RST				0x0d
> +#define		MT9P031_RST_ENABLE		1
> +#define		MT9P031_RST_DISABLE		0
> +#define MT9P031_READ_MODE_1			0x1e
> +#define MT9P031_READ_MODE_2			0x20
> +#define		MT9P031_READ_MODE_2_ROW_MIR	0x8000
> +#define		MT9P031_READ_MODE_2_COL_MIR	0x4000
> +#define MT9P031_ROW_ADDRESS_MODE		0x22
> +#define MT9P031_COLUMN_ADDRESS_MODE		0x23
> +#define MT9P031_GLOBAL_GAIN			0x35
> +
> +#define MT9P031_WINDOW_HEIGHT_MAX		1944
> +#define MT9P031_WINDOW_WIDTH_MAX		2592
> +#define MT9P031_WINDOW_HEIGHT_MIN		2
> +#define MT9P031_WINDOW_WIDTH_MIN		18

Can you move those 4 constants right below MT9P031_WINDOW_HEIGHT and 
MT9P031_WINDOW_WIDTH ? The max values are not correct, according to the 
datasheet they should be 2005 and 2751. You can define *_DEF constants for the 
default width and height.

> +struct mt9p031 {
> +	struct v4l2_subdev subdev;
> +	struct media_pad pad;
> +	struct v4l2_rect rect;	/* Sensor window */
> +	struct v4l2_mbus_framefmt format;
> +	struct mt9p031_platform_data *pdata;
> +	struct mutex power_lock; /* lock to protect power_count */
> +	int power_count;
> +	u16 xskip;
> +	u16 yskip;
> +	/* cache register values */
> +	u16 output_control;
> +	u16 h_blanking;
> +	u16 v_blanking;
> +	u16 column_address_mode;
> +	u16 row_address_mode;
> +	u16 column_start;
> +	u16 row_start;
> +	u16 window_width;
> +	u16 window_height;
> +	struct regulator *reg_1v8;
> +	struct regulator *reg_2v8;
> +};

[snip]

> +static int restore_registers(struct i2c_client *client)
> +{
> +	int ret;
> +	struct mt9p031 *mt9p031 = to_mt9p031(client);
> +
> +	/* Disable register update, reconfigure atomically */
> +	ret = mt9p031_set_output_control(mt9p031, 0,
> +					MT9P031_OUTPUT_CONTROL_SYN);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* Blanking and start values - default... */
> +	ret = reg_write(client, MT9P031_H_BLANKING, mt9p031->h_blanking);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = reg_write(client, MT9P031_V_BLANKING, mt9p031->v_blanking);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = reg_write(client, MT9P031_COLUMN_ADDRESS_MODE,
> +				mt9p031->column_address_mode);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = reg_write(client, MT9P031_ROW_ADDRESS_MODE,
> +				mt9p031->row_address_mode);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = reg_write(client, MT9P031_COLUMN_START,
> +				mt9p031->column_start);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = reg_write(client, MT9P031_ROW_START,
> +				mt9p031->row_start);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = reg_write(client, MT9P031_WINDOW_WIDTH,
> +				mt9p031->window_width);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = reg_write(client, MT9P031_WINDOW_HEIGHT,
> +				mt9p031->window_height);
> +	if (ret < 0)
> +		return ret;

All those registers will be written to in mt9p031_s_stream(), there's no need 
to restore them when powering the chip up. You can remove this function 
completely, as well as the register cache.

> +	/* Re-enable register update, commit all changes */
> +	ret = mt9p031_set_output_control(mt9p031,
> +					MT9P031_OUTPUT_CONTROL_SYN, 0);
> +	if (ret < 0)
> +		return ret;
> +	return 0;
> +}

[snip]

> +static int mt9p031_power_on(struct mt9p031 *mt9p031)
> +{
> +	struct i2c_client *client = v4l2_get_subdevdata(&mt9p031->subdev);
> +	int ret;
> +
> +	/* Ensure RESET_BAR is low */
> +	if (mt9p031->pdata->reset)
> +		mt9p031->pdata->reset(&mt9p031->subdev, 1);
> +	/* turn on digital supply first */
> +	ret = regulator_enable(mt9p031->reg_1v8);
> +	if (ret) {
> +		dev_err(&client->dev,
> +			"Failed to enable 1.8v regulator: %d\n", ret);
> +		goto err_1v8;

You can return ret immediately.

> +	}

According to the datasheet (see figure 31) you need a delay of minimum 1ms 
here. regulator_enable() probably doesn't wait for the power to stabilize 
before returning (correct me if I'm wrong), so a slightly longer delay would 
be safer.

> +	/* now turn on analog supply */
> +	ret = regulator_enable(mt9p031->reg_2v8);
> +	if (ret) {
> +		dev_err(&client->dev,
> +			"Failed to enable 2.8v regulator: %d\n", ret);
> +		goto err_rst;
> +	}
>
> +	/* Now RESET_BAR must be high */
> +	if (mt9p031->pdata->reset)

Similarly, you need to wait for the 2.8V power supply to stabilize before de-
asserting the reset signal. A short delay is probably required. You can put it 
inside the 'if', as the delay is not needed if the reset signal can't be 
controlled.

> +		mt9p031->pdata->reset(&mt9p031->subdev, 0);
> +
> +	if (mt9p031->pdata->set_xclk)
> +		mt9p031->pdata->set_xclk(&mt9p031->subdev, MT9P031_PIXCLK_FREQ);
> +
> +	/* soft reset */
> +	ret = mt9p031_reset(client);
> +	if (ret < 0) {
> +		dev_err(&client->dev, "Failed to reset the camera\n");
> +		goto err_rst;
> +	}
> +
> +	ret = restore_registers(client);
> +	if (ret < 0) {
> +		dev_err(&client->dev, "Failed to restore registers\n");
> +		goto err_rst;
> +	}
> +
> +	return 0;
> +err_rst:
> +	regulator_disable(mt9p031->reg_1v8);

You should disable both regulators here, and only the 1.8V regulator if 
enabling the 2.8V regulator fails.

> +err_1v8:
> +	return ret;
> +
> +}

[snip]

> +static int mt9p031_set_power(struct v4l2_subdev *sd, int on)
> +{
> +	struct mt9p031 *mt9p031 = container_of(sd, struct mt9p031, subdev);
> +	int ret = 0;
> +
> +	mutex_lock(&mt9p031->power_lock);
> +
> +	/*
> +	 * If the power count is modified from 0 to != 0 or from != 0 to 0,
> +	 * update the power state.
> +	 */
> +	if (mt9p031->power_count == !on) {
> +		if (on) {
> +			ret = mt9p031_power_on(mt9p031);
> +			if (ret) {
> +				dev_err(mt9p031->subdev.v4l2_dev->dev,
> +				"Failed to enable 2.8v regulator: %d\n", ret);

This message isn't correct anymore, as mt9p031_power_on() now handles two 
regulators. As mt9p031_power_on() already prints an error message in the case 
of failure, you can remove this message.

> +				goto out;
> +			}
> +		} else {
> +			mt9p031_power_off(mt9p031);
> +		}
> +	}
> +
> +	/* Update the power count. */
> +	mt9p031->power_count += on ? 1 : -1;
> +	WARN_ON(mt9p031->power_count < 0);
> +
> +out:
> +	mutex_unlock(&mt9p031->power_lock);
> +	return ret;
> +}
> +
> +static int mt9p031_registered(struct v4l2_subdev *sd)
> +{
> +	struct mt9p031 *mt9p031 = container_of(sd, struct mt9p031, subdev);
> +	struct i2c_client *client = v4l2_get_subdevdata(&mt9p031->subdev);
> +	int ret;
> +
> +	ret = mt9p031_set_power(&mt9p031->subdev, 1);
> +	if (ret) {
> +		dev_err(&client->dev,
> +			"Failed to power on device: %d\n", ret);
> +		goto err_pwron;

You can return ret directly.

> +	}
> +
> +	ret = mt9p031_video_probe(client);
> +	if (ret)
> +		goto err_evprobe;
> +

Code from here

> +	mt9p031->pad.flags = MEDIA_PAD_FL_SOURCE;
> +	ret = media_entity_init(&mt9p031->subdev.entity, 1, &mt9p031->pad, 0);
> +	if (ret)
> +		goto err_evprobe;
> +
> +	mt9p031->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;

to here can be moved to the probe() function, it will simplify 
mt9p031_registered(). The function will become so small that you can directly 
inline mt9p031_video_probe() inside it.

> +	mt9p031_set_power(&mt9p031->subdev, 0);
> +
> +	return 0;
> +err_evprobe:
> +	mt9p031_set_power(&mt9p031->subdev, 0);
> +err_pwron:
> +	return ret;
> +}

-- 
Regards,

Laurent Pinchart

[PATCH][RFC] Add mt9p031 sensor support.

[Laurent Pinchart]

Hi Javier,

Thanks for the patch. Here's a review of the power handling code.

On Tuesday 24 May 2011 16:30:43 Javier Martin wrote:
> This RFC includes a power management implementation that causes
> the sensor to show images with horizontal artifacts (usually
> monochrome lines that appear on the image randomly).
> 
> Signed-off-by: Javier Martin <javier.martin@vista-silicon.com>

[snip]

> diff --git a/drivers/media/video/mt9p031.c b/drivers/media/video/mt9p031.c
> new file mode 100644
> index 0000000..04d8812
> --- /dev/null
> +++ b/drivers/media/video/mt9p031.c

[snip]


> @@ -0,0 +1,841 @@
> +/*
> + * Driver for MT9P031 CMOS Image Sensor from Aptina
> + *
> + * Copyright (C) 2011, Javier Martin <javier.martin@vista-silicon.com>
> + *
> + * Copyright (C) 2011, Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> + *
> + * Based on the MT9V032 driver and Bastian Hecht's code.
> + *
> + * 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.
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/i2c.h>
> +#include <linux/log2.h>
> +#include <linux/pm.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/slab.h>
> +#include <media/v4l2-subdev.h>
> +#include <linux/videodev2.h>
> +
> +#include <media/mt9p031.h>
> +#include <media/v4l2-chip-ident.h>

This header is not needed anymore.

> +#include <media/v4l2-subdev.h>
> +#include <media/v4l2-device.h>
> +
> +#define MT9P031_PIXCLK_FREQ			54000000
> +
> +/* mt9p031 selected register addresses */
> +#define MT9P031_CHIP_VERSION			0x00
> +#define		MT9P031_CHIP_VERSION_VALUE	0x1801
> +#define MT9P031_ROW_START			0x01
> +#define		MT9P031_ROW_START_DEF		54
> +#define MT9P031_COLUMN_START			0x02
> +#define		MT9P031_COLUMN_START_DEF	16
> +#define MT9P031_WINDOW_HEIGHT			0x03
> +#define MT9P031_WINDOW_WIDTH			0x04
> +#define MT9P031_H_BLANKING			0x05
> +#define		MT9P031_H_BLANKING_VALUE	0
> +#define MT9P031_V_BLANKING			0x06
> +#define		MT9P031_V_BLANKING_VALUE	25
> +#define MT9P031_OUTPUT_CONTROL			0x07
> +#define		MT9P031_OUTPUT_CONTROL_CEN	2
> +#define		MT9P031_OUTPUT_CONTROL_SYN	1
> +#define MT9P031_SHUTTER_WIDTH_UPPER		0x08
> +#define MT9P031_SHUTTER_WIDTH			0x09
> +#define MT9P031_PIXEL_CLOCK_CONTROL		0x0a
> +#define MT9P031_FRAME_RESTART			0x0b
> +#define MT9P031_SHUTTER_DELAY			0x0c
> +#define MT9P031_RST				0x0d
> +#define		MT9P031_RST_ENABLE		1
> +#define		MT9P031_RST_DISABLE		0
> +#define MT9P031_READ_MODE_1			0x1e
> +#define MT9P031_READ_MODE_2			0x20
> +#define		MT9P031_READ_MODE_2_ROW_MIR	0x8000
> +#define		MT9P031_READ_MODE_2_COL_MIR	0x4000
> +#define MT9P031_ROW_ADDRESS_MODE		0x22
> +#define MT9P031_COLUMN_ADDRESS_MODE		0x23
> +#define MT9P031_GLOBAL_GAIN			0x35
> +
> +#define MT9P031_WINDOW_HEIGHT_MAX		1944
> +#define MT9P031_WINDOW_WIDTH_MAX		2592
> +#define MT9P031_WINDOW_HEIGHT_MIN		2
> +#define MT9P031_WINDOW_WIDTH_MIN		18

Can you move those 4 constants right below MT9P031_WINDOW_HEIGHT and 
MT9P031_WINDOW_WIDTH ? The max values are not correct, according to the 
datasheet they should be 2005 and 2751. You can define *_DEF constants for the 
default width and height.

> +struct mt9p031 {
> +	struct v4l2_subdev subdev;
> +	struct media_pad pad;
> +	struct v4l2_rect rect;	/* Sensor window */
> +	struct v4l2_mbus_framefmt format;
> +	struct mt9p031_platform_data *pdata;
> +	struct mutex power_lock; /* lock to protect power_count */
> +	int power_count;
> +	u16 xskip;
> +	u16 yskip;
> +	/* cache register values */
> +	u16 output_control;
> +	u16 h_blanking;
> +	u16 v_blanking;
> +	u16 column_address_mode;
> +	u16 row_address_mode;
> +	u16 column_start;
> +	u16 row_start;
> +	u16 window_width;
> +	u16 window_height;
> +	struct regulator *reg_1v8;
> +	struct regulator *reg_2v8;
> +};

[snip]

> +static int restore_registers(struct i2c_client *client)
> +{
> +	int ret;
> +	struct mt9p031 *mt9p031 = to_mt9p031(client);
> +
> +	/* Disable register update, reconfigure atomically */
> +	ret = mt9p031_set_output_control(mt9p031, 0,
> +					MT9P031_OUTPUT_CONTROL_SYN);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* Blanking and start values - default... */
> +	ret = reg_write(client, MT9P031_H_BLANKING, mt9p031->h_blanking);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = reg_write(client, MT9P031_V_BLANKING, mt9p031->v_blanking);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = reg_write(client, MT9P031_COLUMN_ADDRESS_MODE,
> +				mt9p031->column_address_mode);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = reg_write(client, MT9P031_ROW_ADDRESS_MODE,
> +				mt9p031->row_address_mode);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = reg_write(client, MT9P031_COLUMN_START,
> +				mt9p031->column_start);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = reg_write(client, MT9P031_ROW_START,
> +				mt9p031->row_start);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = reg_write(client, MT9P031_WINDOW_WIDTH,
> +				mt9p031->window_width);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = reg_write(client, MT9P031_WINDOW_HEIGHT,
> +				mt9p031->window_height);
> +	if (ret < 0)
> +		return ret;

All those registers will be written to in mt9p031_s_stream(), there's no need 
to restore them when powering the chip up. You can remove this function 
completely, as well as the register cache.

> +	/* Re-enable register update, commit all changes */
> +	ret = mt9p031_set_output_control(mt9p031,
> +					MT9P031_OUTPUT_CONTROL_SYN, 0);
> +	if (ret < 0)
> +		return ret;
> +	return 0;
> +}

[snip]

> +static int mt9p031_power_on(struct mt9p031 *mt9p031)
> +{
> +	struct i2c_client *client = v4l2_get_subdevdata(&mt9p031->subdev);
> +	int ret;
> +
> +	/* Ensure RESET_BAR is low */
> +	if (mt9p031->pdata->reset)
> +		mt9p031->pdata->reset(&mt9p031->subdev, 1);
> +	/* turn on digital supply first */
> +	ret = regulator_enable(mt9p031->reg_1v8);
> +	if (ret) {
> +		dev_err(&client->dev,
> +			"Failed to enable 1.8v regulator: %d\n", ret);
> +		goto err_1v8;

You can return ret immediately.

> +	}

According to the datasheet (see figure 31) you need a delay of minimum 1ms 
here. regulator_enable() probably doesn't wait for the power to stabilize 
before returning (correct me if I'm wrong), so a slightly longer delay would 
be safer.

> +	/* now turn on analog supply */
> +	ret = regulator_enable(mt9p031->reg_2v8);
> +	if (ret) {
> +		dev_err(&client->dev,
> +			"Failed to enable 2.8v regulator: %d\n", ret);
> +		goto err_rst;
> +	}
>
> +	/* Now RESET_BAR must be high */
> +	if (mt9p031->pdata->reset)

Similarly, you need to wait for the 2.8V power supply to stabilize before de-
asserting the reset signal. A short delay is probably required. You can put it 
inside the 'if', as the delay is not needed if the reset signal can't be 
controlled.

> +		mt9p031->pdata->reset(&mt9p031->subdev, 0);
> +
> +	if (mt9p031->pdata->set_xclk)
> +		mt9p031->pdata->set_xclk(&mt9p031->subdev, MT9P031_PIXCLK_FREQ);
> +
> +	/* soft reset */
> +	ret = mt9p031_reset(client);
> +	if (ret < 0) {
> +		dev_err(&client->dev, "Failed to reset the camera\n");
> +		goto err_rst;
> +	}
> +
> +	ret = restore_registers(client);
> +	if (ret < 0) {
> +		dev_err(&client->dev, "Failed to restore registers\n");
> +		goto err_rst;
> +	}
> +
> +	return 0;
> +err_rst:
> +	regulator_disable(mt9p031->reg_1v8);

You should disable both regulators here, and only the 1.8V regulator if 
enabling the 2.8V regulator fails.

> +err_1v8:
> +	return ret;
> +
> +}

[snip]

> +static int mt9p031_set_power(struct v4l2_subdev *sd, int on)
> +{
> +	struct mt9p031 *mt9p031 = container_of(sd, struct mt9p031, subdev);
> +	int ret = 0;
> +
> +	mutex_lock(&mt9p031->power_lock);
> +
> +	/*
> +	 * If the power count is modified from 0 to != 0 or from != 0 to 0,
> +	 * update the power state.
> +	 */
> +	if (mt9p031->power_count == !on) {
> +		if (on) {
> +			ret = mt9p031_power_on(mt9p031);
> +			if (ret) {
> +				dev_err(mt9p031->subdev.v4l2_dev->dev,
> +				"Failed to enable 2.8v regulator: %d\n", ret);

This message isn't correct anymore, as mt9p031_power_on() now handles two 
regulators. As mt9p031_power_on() already prints an error message in the case 
of failure, you can remove this message.

> +				goto out;
> +			}
> +		} else {
> +			mt9p031_power_off(mt9p031);
> +		}
> +	}
> +
> +	/* Update the power count. */
> +	mt9p031->power_count += on ? 1 : -1;
> +	WARN_ON(mt9p031->power_count < 0);
> +
> +out:
> +	mutex_unlock(&mt9p031->power_lock);
> +	return ret;
> +}
> +
> +static int mt9p031_registered(struct v4l2_subdev *sd)
> +{
> +	struct mt9p031 *mt9p031 = container_of(sd, struct mt9p031, subdev);
> +	struct i2c_client *client = v4l2_get_subdevdata(&mt9p031->subdev);
> +	int ret;
> +
> +	ret = mt9p031_set_power(&mt9p031->subdev, 1);
> +	if (ret) {
> +		dev_err(&client->dev,
> +			"Failed to power on device: %d\n", ret);
> +		goto err_pwron;

You can return ret directly.

> +	}
> +
> +	ret = mt9p031_video_probe(client);
> +	if (ret)
> +		goto err_evprobe;
> +

Code from here

> +	mt9p031->pad.flags = MEDIA_PAD_FL_SOURCE;
> +	ret = media_entity_init(&mt9p031->subdev.entity, 1, &mt9p031->pad, 0);
> +	if (ret)
> +		goto err_evprobe;
> +
> +	mt9p031->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;

to here can be moved to the probe() function, it will simplify 
mt9p031_registered(). The function will become so small that you can directly 
inline mt9p031_video_probe() inside it.

> +	mt9p031_set_power(&mt9p031->subdev, 0);
> +
> +	return 0;
> +err_evprobe:
> +	mt9p031_set_power(&mt9p031->subdev, 0);
> +err_pwron:
> +	return ret;
> +}

-- 
Regards,

Laurent Pinchart

Re: [PATCH][RFC] Add mt9p031 sensor support.

[javier Martin]

Hi,
thank you for the review, I agree with you on all the suggested
changes except on this one:

On 25 May 2011 10:05, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> Hi Javier,
>
> Thanks for the patch. Here's a review of the power handling code.
>
> On Tuesday 24 May 2011 16:30:43 Javier Martin wrote:
>> This RFC includes a power management implementation that causes
>> the sensor to show images with horizontal artifacts (usually
>> monochrome lines that appear on the image randomly).
>>
>> Signed-off-by: Javier Martin <javier.martin@vista-silicon.com>
>
> [snip]
>
>> diff --git a/drivers/media/video/mt9p031.c b/drivers/media/video/mt9p031.c
>> new file mode 100644
>> index 0000000..04d8812
>> --- /dev/null
>> +++ b/drivers/media/video/mt9p031.c
>
> [snip]
>> +#define MT9P031_WINDOW_HEIGHT_MAX            1944
>> +#define MT9P031_WINDOW_WIDTH_MAX             2592
>> +#define MT9P031_WINDOW_HEIGHT_MIN            2
>> +#define MT9P031_WINDOW_WIDTH_MIN             18
>
> Can you move those 4 constants right below MT9P031_WINDOW_HEIGHT and
> MT9P031_WINDOW_WIDTH ? The max values are not correct, according to the
> datasheet they should be 2005 and 2751.

In figure 4, it says active image size is 2592 x 1944
Why should I include active boundary and dark pixels?




-- 
Javier Martin
Vista Silicon S.L.
CDTUC - FASE C - Oficina S-345
Avda de los Castros s/n
39005- Santander. Cantabria. Spain
+34 942 25 32 60
www.vista-silicon.com

[PATCH][RFC] Add mt9p031 sensor support.

[javier Martin]

Hi,
thank you for the review, I agree with you on all the suggested
changes except on this one:

On 25 May 2011 10:05, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> Hi Javier,
>
> Thanks for the patch. Here's a review of the power handling code.
>
> On Tuesday 24 May 2011 16:30:43 Javier Martin wrote:
>> This RFC includes a power management implementation that causes
>> the sensor to show images with horizontal artifacts (usually
>> monochrome lines that appear on the image randomly).
>>
>> Signed-off-by: Javier Martin <javier.martin@vista-silicon.com>
>
> [snip]
>
>> diff --git a/drivers/media/video/mt9p031.c b/drivers/media/video/mt9p031.c
>> new file mode 100644
>> index 0000000..04d8812
>> --- /dev/null
>> +++ b/drivers/media/video/mt9p031.c
>
> [snip]
>> +#define MT9P031_WINDOW_HEIGHT_MAX ? ? ? ? ? ?1944
>> +#define MT9P031_WINDOW_WIDTH_MAX ? ? ? ? ? ? 2592
>> +#define MT9P031_WINDOW_HEIGHT_MIN ? ? ? ? ? ?2
>> +#define MT9P031_WINDOW_WIDTH_MIN ? ? ? ? ? ? 18
>
> Can you move those 4 constants right below MT9P031_WINDOW_HEIGHT and
> MT9P031_WINDOW_WIDTH ? The max values are not correct, according to the
> datasheet they should be 2005 and 2751.

In figure 4, it says active image size is 2592 x 1944
Why should I include active boundary and dark pixels?




-- 
Javier Martin
Vista Silicon S.L.
CDTUC - FASE C - Oficina S-345
Avda de los Castros s/n
39005- Santander. Cantabria. Spain
+34 942 25 32 60
www.vista-silicon.com

Re: [PATCH][RFC] Add mt9p031 sensor support.

[Laurent Pinchart]

Hi Javier,

On Wednesday 25 May 2011 11:41:42 javier Martin wrote:
> Hi,
> thank you for the review, I agree with you on all the suggested
> changes except on this one:
> 
> On 25 May 2011 10:05, Laurent Pinchart wrote:
> > On Tuesday 24 May 2011 16:30:43 Javier Martin wrote:
> >> This RFC includes a power management implementation that causes
> >> the sensor to show images with horizontal artifacts (usually
> >> monochrome lines that appear on the image randomly).
> >> 
> >> Signed-off-by: Javier Martin <javier.martin@vista-silicon.com>
> > 
> > [snip]
> > 
> >> diff --git a/drivers/media/video/mt9p031.c
> >> b/drivers/media/video/mt9p031.c new file mode 100644
> >> index 0000000..04d8812
> >> --- /dev/null
> >> +++ b/drivers/media/video/mt9p031.c
> > 
> > [snip]
> > 
> >> +#define MT9P031_WINDOW_HEIGHT_MAX            1944
> >> +#define MT9P031_WINDOW_WIDTH_MAX             2592
> >> +#define MT9P031_WINDOW_HEIGHT_MIN            2
> >> +#define MT9P031_WINDOW_WIDTH_MIN             18
> > 
> > Can you move those 4 constants right below MT9P031_WINDOW_HEIGHT and
> > MT9P031_WINDOW_WIDTH ? The max values are not correct, according to the
> > datasheet they should be 2005 and 2751.
> 
> In figure 4, it says active image size is 2592 x 1944
> Why should I include active boundary and dark pixels?

Users might want to get the dark pixels for black level compensation purpose. 
As the chip allows for that, it should be supported. The default should of 
course be the active area of 2592 x 1944 pixels.

-- 
Regards,

Laurent Pinchart

[PATCH][RFC] Add mt9p031 sensor support.

[Laurent Pinchart]

Hi Javier,

On Wednesday 25 May 2011 11:41:42 javier Martin wrote:
> Hi,
> thank you for the review, I agree with you on all the suggested
> changes except on this one:
> 
> On 25 May 2011 10:05, Laurent Pinchart wrote:
> > On Tuesday 24 May 2011 16:30:43 Javier Martin wrote:
> >> This RFC includes a power management implementation that causes
> >> the sensor to show images with horizontal artifacts (usually
> >> monochrome lines that appear on the image randomly).
> >> 
> >> Signed-off-by: Javier Martin <javier.martin@vista-silicon.com>
> > 
> > [snip]
> > 
> >> diff --git a/drivers/media/video/mt9p031.c
> >> b/drivers/media/video/mt9p031.c new file mode 100644
> >> index 0000000..04d8812
> >> --- /dev/null
> >> +++ b/drivers/media/video/mt9p031.c
> > 
> > [snip]
> > 
> >> +#define MT9P031_WINDOW_HEIGHT_MAX            1944
> >> +#define MT9P031_WINDOW_WIDTH_MAX             2592
> >> +#define MT9P031_WINDOW_HEIGHT_MIN            2
> >> +#define MT9P031_WINDOW_WIDTH_MIN             18
> > 
> > Can you move those 4 constants right below MT9P031_WINDOW_HEIGHT and
> > MT9P031_WINDOW_WIDTH ? The max values are not correct, according to the
> > datasheet they should be 2005 and 2751.
> 
> In figure 4, it says active image size is 2592 x 1944
> Why should I include active boundary and dark pixels?

Users might want to get the dark pixels for black level compensation purpose. 
As the chip allows for that, it should be supported. The default should of 
course be the active area of 2592 x 1944 pixels.

-- 
Regards,

Laurent Pinchart

Re: [PATCH][RFC] Add mt9p031 sensor support.

[javier Martin]

On 25 May 2011 11:43, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> Hi Javier,
>
> On Wednesday 25 May 2011 11:41:42 javier Martin wrote:
>> Hi,
>> thank you for the review, I agree with you on all the suggested
>> changes except on this one:
>>
>> On 25 May 2011 10:05, Laurent Pinchart wrote:
>> > On Tuesday 24 May 2011 16:30:43 Javier Martin wrote:
>> >> This RFC includes a power management implementation that causes
>> >> the sensor to show images with horizontal artifacts (usually
>> >> monochrome lines that appear on the image randomly).
>> >>
>> >> Signed-off-by: Javier Martin <javier.martin@vista-silicon.com>
>> >
>> > [snip]
>> >
>> >> diff --git a/drivers/media/video/mt9p031.c
>> >> b/drivers/media/video/mt9p031.c new file mode 100644
>> >> index 0000000..04d8812
>> >> --- /dev/null
>> >> +++ b/drivers/media/video/mt9p031.c
>> >
>> > [snip]
>> >
>> >> +#define MT9P031_WINDOW_HEIGHT_MAX            1944
>> >> +#define MT9P031_WINDOW_WIDTH_MAX             2592
>> >> +#define MT9P031_WINDOW_HEIGHT_MIN            2
>> >> +#define MT9P031_WINDOW_WIDTH_MIN             18
>> >
>> > Can you move those 4 constants right below MT9P031_WINDOW_HEIGHT and
>> > MT9P031_WINDOW_WIDTH ? The max values are not correct, according to the
>> > datasheet they should be 2005 and 2751.
>>
>> In figure 4, it says active image size is 2592 x 1944
>> Why should I include active boundary and dark pixels?
>
> Users might want to get the dark pixels for black level compensation purpose.
> As the chip allows for that, it should be supported. The default should of
> course be the active area of 2592 x 1944 pixels.
>

OK, that sounds reasonable. However, that would include black pixels
that are located at the beginning of the array (0,0) to (16, 54),
which means that users would have to specify a crop value of (15,54)
to eliminate those initial black level pixels. Which seems quite
unnatural to me.
Another option could be setting (16,54) as default values and allowing
to introduce negative cropping values. Is this possible?
And finally, the most sensible idea IMHO could be not letting the user
to see pixels from (0,0) to (15,54) (setting 15,54 as minimum and
default )and, for black level compensation, ending pixels could be
used (2608,1998) to (2751, 2003).

What do you think?


-- 
Javier Martin
Vista Silicon S.L.
CDTUC - FASE C - Oficina S-345
Avda de los Castros s/n
39005- Santander. Cantabria. Spain
+34 942 25 32 60
www.vista-silicon.com

[PATCH][RFC] Add mt9p031 sensor support.

[javier Martin]

On 25 May 2011 11:43, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> Hi Javier,
>
> On Wednesday 25 May 2011 11:41:42 javier Martin wrote:
>> Hi,
>> thank you for the review, I agree with you on all the suggested
>> changes except on this one:
>>
>> On 25 May 2011 10:05, Laurent Pinchart wrote:
>> > On Tuesday 24 May 2011 16:30:43 Javier Martin wrote:
>> >> This RFC includes a power management implementation that causes
>> >> the sensor to show images with horizontal artifacts (usually
>> >> monochrome lines that appear on the image randomly).
>> >>
>> >> Signed-off-by: Javier Martin <javier.martin@vista-silicon.com>
>> >
>> > [snip]
>> >
>> >> diff --git a/drivers/media/video/mt9p031.c
>> >> b/drivers/media/video/mt9p031.c new file mode 100644
>> >> index 0000000..04d8812
>> >> --- /dev/null
>> >> +++ b/drivers/media/video/mt9p031.c
>> >
>> > [snip]
>> >
>> >> +#define MT9P031_WINDOW_HEIGHT_MAX ? ? ? ? ? ?1944
>> >> +#define MT9P031_WINDOW_WIDTH_MAX ? ? ? ? ? ? 2592
>> >> +#define MT9P031_WINDOW_HEIGHT_MIN ? ? ? ? ? ?2
>> >> +#define MT9P031_WINDOW_WIDTH_MIN ? ? ? ? ? ? 18
>> >
>> > Can you move those 4 constants right below MT9P031_WINDOW_HEIGHT and
>> > MT9P031_WINDOW_WIDTH ? The max values are not correct, according to the
>> > datasheet they should be 2005 and 2751.
>>
>> In figure 4, it says active image size is 2592 x 1944
>> Why should I include active boundary and dark pixels?
>
> Users might want to get the dark pixels for black level compensation purpose.
> As the chip allows for that, it should be supported. The default should of
> course be the active area of 2592 x 1944 pixels.
>

OK, that sounds reasonable. However, that would include black pixels
that are located at the beginning of the array (0,0) to (16, 54),
which means that users would have to specify a crop value of (15,54)
to eliminate those initial black level pixels. Which seems quite
unnatural to me.
Another option could be setting (16,54) as default values and allowing
to introduce negative cropping values. Is this possible?
And finally, the most sensible idea IMHO could be not letting the user
to see pixels from (0,0) to (15,54) (setting 15,54 as minimum and
default )and, for black level compensation, ending pixels could be
used (2608,1998) to (2751, 2003).

What do you think?


-- 
Javier Martin
Vista Silicon S.L.
CDTUC - FASE C - Oficina S-345
Avda de los Castros s/n
39005- Santander. Cantabria. Spain
+34 942 25 32 60
www.vista-silicon.com

Re: [PATCH][RFC] Add mt9p031 sensor support.

[Guennadi Liakhovetski]

On Fri, 27 May 2011, javier Martin wrote:

> On 25 May 2011 11:43, Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
> > Hi Javier,
> >
> > On Wednesday 25 May 2011 11:41:42 javier Martin wrote:
> >> Hi,
> >> thank you for the review, I agree with you on all the suggested
> >> changes except on this one:
> >>
> >> On 25 May 2011 10:05, Laurent Pinchart wrote:
> >> > On Tuesday 24 May 2011 16:30:43 Javier Martin wrote:
> >> >> This RFC includes a power management implementation that causes
> >> >> the sensor to show images with horizontal artifacts (usually
> >> >> monochrome lines that appear on the image randomly).
> >> >>
> >> >> Signed-off-by: Javier Martin <javier.martin@vista-silicon.com>
> >> >
> >> > [snip]
> >> >
> >> >> diff --git a/drivers/media/video/mt9p031.c
> >> >> b/drivers/media/video/mt9p031.c new file mode 100644
> >> >> index 0000000..04d8812
> >> >> --- /dev/null
> >> >> +++ b/drivers/media/video/mt9p031.c
> >> >
> >> > [snip]
> >> >
> >> >> +#define MT9P031_WINDOW_HEIGHT_MAX            1944
> >> >> +#define MT9P031_WINDOW_WIDTH_MAX             2592
> >> >> +#define MT9P031_WINDOW_HEIGHT_MIN            2
> >> >> +#define MT9P031_WINDOW_WIDTH_MIN             18
> >> >
> >> > Can you move those 4 constants right below MT9P031_WINDOW_HEIGHT and
> >> > MT9P031_WINDOW_WIDTH ? The max values are not correct, according to the
> >> > datasheet they should be 2005 and 2751.
> >>
> >> In figure 4, it says active image size is 2592 x 1944
> >> Why should I include active boundary and dark pixels?
> >
> > Users might want to get the dark pixels for black level compensation purpose.
> > As the chip allows for that, it should be supported. The default should of
> > course be the active area of 2592 x 1944 pixels.
> >
> 
> OK, that sounds reasonable. However, that would include black pixels
> that are located at the beginning of the array (0,0) to (16, 54),
> which means that users would have to specify a crop value of (15,54)
> to eliminate those initial black level pixels. Which seems quite
> unnatural to me.
> Another option could be setting (16,54) as default values and allowing
> to introduce negative cropping values. Is this possible?
> And finally, the most sensible idea IMHO could be not letting the user
> to see pixels from (0,0) to (15,54) (setting 15,54 as minimum and
> default )and, for black level compensation, ending pixels could be
> used (2608,1998) to (2751, 2003).

No, you set your crop bounds to (0,0)-... but your default rectangle to 
(15,54)-... and that's also what you set if noone issues an S_CROP.

Thanks
Guennadi

> What do you think?
> 
> 
> -- 
> Javier Martin
> Vista Silicon S.L.
> CDTUC - FASE C - Oficina S-345
> Avda de los Castros s/n
> 39005- Santander. Cantabria. Spain
> +34 942 25 32 60
> www.vista-silicon.com
> 

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

[PATCH][RFC] Add mt9p031 sensor support.

[Guennadi Liakhovetski]

On Fri, 27 May 2011, javier Martin wrote:

> On 25 May 2011 11:43, Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
> > Hi Javier,
> >
> > On Wednesday 25 May 2011 11:41:42 javier Martin wrote:
> >> Hi,
> >> thank you for the review, I agree with you on all the suggested
> >> changes except on this one:
> >>
> >> On 25 May 2011 10:05, Laurent Pinchart wrote:
> >> > On Tuesday 24 May 2011 16:30:43 Javier Martin wrote:
> >> >> This RFC includes a power management implementation that causes
> >> >> the sensor to show images with horizontal artifacts (usually
> >> >> monochrome lines that appear on the image randomly).
> >> >>
> >> >> Signed-off-by: Javier Martin <javier.martin@vista-silicon.com>
> >> >
> >> > [snip]
> >> >
> >> >> diff --git a/drivers/media/video/mt9p031.c
> >> >> b/drivers/media/video/mt9p031.c new file mode 100644
> >> >> index 0000000..04d8812
> >> >> --- /dev/null
> >> >> +++ b/drivers/media/video/mt9p031.c
> >> >
> >> > [snip]
> >> >
> >> >> +#define MT9P031_WINDOW_HEIGHT_MAX ? ? ? ? ? ?1944
> >> >> +#define MT9P031_WINDOW_WIDTH_MAX ? ? ? ? ? ? 2592
> >> >> +#define MT9P031_WINDOW_HEIGHT_MIN ? ? ? ? ? ?2
> >> >> +#define MT9P031_WINDOW_WIDTH_MIN ? ? ? ? ? ? 18
> >> >
> >> > Can you move those 4 constants right below MT9P031_WINDOW_HEIGHT and
> >> > MT9P031_WINDOW_WIDTH ? The max values are not correct, according to the
> >> > datasheet they should be 2005 and 2751.
> >>
> >> In figure 4, it says active image size is 2592 x 1944
> >> Why should I include active boundary and dark pixels?
> >
> > Users might want to get the dark pixels for black level compensation purpose.
> > As the chip allows for that, it should be supported. The default should of
> > course be the active area of 2592 x 1944 pixels.
> >
> 
> OK, that sounds reasonable. However, that would include black pixels
> that are located at the beginning of the array (0,0) to (16, 54),
> which means that users would have to specify a crop value of (15,54)
> to eliminate those initial black level pixels. Which seems quite
> unnatural to me.
> Another option could be setting (16,54) as default values and allowing
> to introduce negative cropping values. Is this possible?
> And finally, the most sensible idea IMHO could be not letting the user
> to see pixels from (0,0) to (15,54) (setting 15,54 as minimum and
> default )and, for black level compensation, ending pixels could be
> used (2608,1998) to (2751, 2003).

No, you set your crop bounds to (0,0)-... but your default rectangle to 
(15,54)-... and that's also what you set if noone issues an S_CROP.

Thanks
Guennadi

> What do you think?
> 
> 
> -- 
> Javier Martin
> Vista Silicon S.L.
> CDTUC - FASE C - Oficina S-345
> Avda de los Castros s/n
> 39005- Santander. Cantabria. Spain
> +34 942 25 32 60
> www.vista-silicon.com
> 

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/