[RFC PATCH 0/5] Use control framework in cafe_ccic and s_config removal

[RFC PATCH 0/5] Use control framework in cafe_ccic and s_config removal

[Hans Verkuil]

Hi Jon, Laurent,

This patch series converts the OLPC cafe_ccic driver to the new control
framework. It turned out that this depended on the removal of the legacy
s_config subdev operation. I originally created the ov7670 controls in
s_config, but it turned out that s_config is called after v4l2_device_register_subdev,
so v4l2_device_register_subdev is unable to 'inherit' the ov7670 controls
in cafe_ccic since there aren't any yet.

Another reason why s_config is a bad idea.

So the first patch removes s_config and v4l2_i2c_new_subdev_cfg and converts
any users (cafe_ccic/ov7670 among them) to v4l2_i2c_new_subdev_board, which is
what God (i.e. Jean Delvare) intended. :-)

The second patch adds the (un)register internal ops allowing subdev drivers to
do some work after the subdevice is registered with a v4l2_device. So here the
sd->v4l2_dev pointer is set. Laurent, is this sufficient for the upcoming omap3
sub-devices?

The third patch fixes a small bug in v4l2_ctrl_handler_setup() that is required
for the ov7670 conversion. This bug does not affect any existing drivers.

The last two patches convert ov7670 and cafe_ccic to the V4L2 control framework.

This saves over a 100 lines of code and adds full support of the control API
(including extended controls) to the drivers.

This has been extensively tested on my humble OLPC laptop (and it took me 4-5
hours just to get the damn thing up and running with these drivers).

Special attention was given to the handling of gain/autogain and
exposure/autoexposure.

The way this works is that setting the gain on its own will turn off autogain
(this conforms to the current behavior of ov7670), setting autogain and gain
atomically will only set the gain if autogain is set to manual.

Ditto for exposure/autoexposure.

The only question is: is the current behavior of implicitly turning off autogain
when setting a new gain value correct? I think setting the gain in that case
should do nothing.

On the other hand, I don't know if there is any code that depends on this
behavior, so perhaps we should just leave it as is.

Jon, can you take a look and let me know if it is OK with you?

Regards,

	Hans

 ov7670.c |  296 ++++++++++++++++++++++++---------------------------------------
 1 file changed, 116 insertions(+), 180 deletions(-)
 cafe_ccic.c |   59 +++++++++++------------------------------------------------
 1 file changed, 11 insertions(+), 48 deletions(-)

[RFC PATCH 1/5] v4l2-subdev: remove core.s_config and v4l2_i2c_new_subdev_cfg()

[Hans Verkuil]

The core.s_config op was meant for legacy drivers that needed to work with old
pre-2.6.26 kernels. This is no longer relevant. Unfortunately, this op was
incorrectly called from several drivers.

Replace those occurences with proper i2c_board_info structs and call
v4l2_i2c_new_subdev_board.

After these changes v4l2_i2c_new_subdev_cfg() was no longer used, so remove
that function as well.

Signed-off-by: Hans Verkuil <hverkuil@xs4all.nl>
---
 drivers/media/video/cafe_ccic.c            |   11 +++-
 drivers/media/video/cx25840/cx25840-core.c |   22 ++------
 drivers/media/video/em28xx/em28xx-cards.c  |   18 ++++---
 drivers/media/video/ivtv/ivtv-i2c.c        |    9 +++-
 drivers/media/video/mt9v011.c              |   29 ++++-------
 drivers/media/video/mt9v011.h              |   36 -------------
 drivers/media/video/mt9v011_regs.h         |   36 +++++++++++++
 drivers/media/video/ov7670.c               |   74 ++++++++++++----------------
 drivers/media/video/sr030pc30.c            |   10 ----
 drivers/media/video/v4l2-common.c          |   19 +------
 include/media/mt9v011.h                    |   17 ++++++
 include/media/v4l2-common.h                |   13 +-----
 include/media/v4l2-subdev.h                |    6 +--
 13 files changed, 130 insertions(+), 170 deletions(-)
 delete mode 100644 drivers/media/video/mt9v011.h
 create mode 100644 drivers/media/video/mt9v011_regs.h
 create mode 100644 include/media/mt9v011.h

diff --git a/drivers/media/video/cafe_ccic.c b/drivers/media/video/cafe_ccic.c
index 789087c..7488b47 100644
--- a/drivers/media/video/cafe_ccic.c
+++ b/drivers/media/video/cafe_ccic.c
@@ -2001,6 +2001,11 @@ static int cafe_pci_probe(struct pci_dev *pdev,
 		.min_width = 320,
 		.min_height = 240,
 	};
+	struct i2c_board_info ov7670_info = {
+		.type = "ov7670",
+		.addr = 0x42,
+		.platform_data = &sensor_cfg,
+	};
 
 	/*
 	 * Start putting together one of our big camera structures.
@@ -2062,9 +2067,9 @@ static int cafe_pci_probe(struct pci_dev *pdev,
 	if (dmi_check_system(olpc_xo1_dmi))
 		sensor_cfg.clock_speed = 45;
 
-	cam->sensor_addr = 0x42;
-	cam->sensor = v4l2_i2c_new_subdev_cfg(&cam->v4l2_dev, &cam->i2c_adapter,
-			"ov7670", 0, &sensor_cfg, cam->sensor_addr, NULL);
+	cam->sensor_addr = ov7670_info.addr;
+	cam->sensor = v4l2_i2c_new_subdev_board(&cam->v4l2_dev, &cam->i2c_adapter,
+			&ov7670_info, NULL);
 	if (cam->sensor == NULL) {
 		ret = -ENODEV;
 		goto out_smbus;
diff --git a/drivers/media/video/cx25840/cx25840-core.c b/drivers/media/video/cx25840/cx25840-core.c
index dfb198d..9b384ac 100644
--- a/drivers/media/video/cx25840/cx25840-core.c
+++ b/drivers/media/video/cx25840/cx25840-core.c
@@ -1682,20 +1682,6 @@ static int cx25840_log_status(struct v4l2_subdev *sd)
 	return 0;
 }
 
-static int cx25840_s_config(struct v4l2_subdev *sd, int irq, void *platform_data)
-{
-	struct cx25840_state *state = to_state(sd);
-	struct i2c_client *client = v4l2_get_subdevdata(sd);
-
-	if (platform_data) {
-		struct cx25840_platform_data *pdata = platform_data;
-
-		state->pvr150_workaround = pdata->pvr150_workaround;
-		set_input(client, state->vid_input, state->aud_input);
-	}
-	return 0;
-}
-
 static int cx23885_irq_handler(struct v4l2_subdev *sd, u32 status,
 			       bool *handled)
 {
@@ -1787,7 +1773,6 @@ static const struct v4l2_ctrl_ops cx25840_ctrl_ops = {
 
 static const struct v4l2_subdev_core_ops cx25840_core_ops = {
 	.log_status = cx25840_log_status,
-	.s_config = cx25840_s_config,
 	.g_chip_ident = cx25840_g_chip_ident,
 	.g_ctrl = v4l2_subdev_g_ctrl,
 	.s_ctrl = v4l2_subdev_s_ctrl,
@@ -1974,7 +1959,6 @@ static int cx25840_probe(struct i2c_client *client,
 	state->vid_input = CX25840_COMPOSITE7;
 	state->aud_input = CX25840_AUDIO8;
 	state->audclk_freq = 48000;
-	state->pvr150_workaround = 0;
 	state->audmode = V4L2_TUNER_MODE_LANG1;
 	state->vbi_line_offset = 8;
 	state->id = id;
@@ -2019,6 +2003,12 @@ static int cx25840_probe(struct i2c_client *client,
 	v4l2_ctrl_cluster(2, &state->volume);
 	v4l2_ctrl_handler_setup(&state->hdl);
 
+	if (client->dev.platform_data) {
+		struct cx25840_platform_data *pdata = client->dev.platform_data;
+
+		state->pvr150_workaround = pdata->pvr150_workaround;
+	}
+
 	cx25840_ir_probe(sd);
 	return 0;
 }
diff --git a/drivers/media/video/em28xx/em28xx-cards.c b/drivers/media/video/em28xx/em28xx-cards.c
index 8af302b..2822aeb 100644
--- a/drivers/media/video/em28xx/em28xx-cards.c
+++ b/drivers/media/video/em28xx/em28xx-cards.c
@@ -33,6 +33,7 @@
 #include <media/saa7115.h>
 #include <media/tvp5150.h>
 #include <media/tvaudio.h>
+#include <media/mt9v011.h>
 #include <media/i2c-addr.h>
 #include <media/tveeprom.h>
 #include <media/v4l2-common.h>
@@ -1917,11 +1918,6 @@ static unsigned short tvp5150_addrs[] = {
 	I2C_CLIENT_END
 };
 
-static unsigned short mt9v011_addrs[] = {
-	0xba >> 1,
-	I2C_CLIENT_END
-};
-
 static unsigned short msp3400_addrs[] = {
 	0x80 >> 1,
 	0x88 >> 1,
@@ -2623,11 +2619,17 @@ void em28xx_card_setup(struct em28xx *dev)
 			"tvp5150", 0, tvp5150_addrs);
 
 	if (dev->em28xx_sensor == EM28XX_MT9V011) {
+		struct mt9v011_platform_data pdata;
+		struct i2c_board_info mt9v011_info = {
+			.type = "mt9v011",
+			.addr = 0xba >> 1,
+			.platform_data = &pdata,
+		};
 		struct v4l2_subdev *sd;
 
-		sd = v4l2_i2c_new_subdev(&dev->v4l2_dev,
-			 &dev->i2c_adap, "mt9v011", 0, mt9v011_addrs);
-		v4l2_subdev_call(sd, core, s_config, 0, &dev->sensor_xtal);
+		pdata.xtal = dev->sensor_xtal;
+		sd = v4l2_i2c_new_subdev_board(&dev->v4l2_dev, &dev->i2c_adap,
+				&mt9v011_info, NULL);
 	}
 
 
diff --git a/drivers/media/video/ivtv/ivtv-i2c.c b/drivers/media/video/ivtv/ivtv-i2c.c
index e103b8f..9fb86a0 100644
--- a/drivers/media/video/ivtv/ivtv-i2c.c
+++ b/drivers/media/video/ivtv/ivtv-i2c.c
@@ -300,10 +300,15 @@ int ivtv_i2c_register(struct ivtv *itv, unsigned idx)
 				adap, type, 0, I2C_ADDRS(hw_addrs[idx]));
 	} else if (hw == IVTV_HW_CX25840) {
 		struct cx25840_platform_data pdata;
+		struct i2c_board_info cx25840_info = {
+			.type = "cx25840",
+			.addr = hw_addrs[idx],
+			.platform_data = &pdata,
+		};
 
 		pdata.pvr150_workaround = itv->pvr150_workaround;
-		sd = v4l2_i2c_new_subdev_cfg(&itv->v4l2_dev,
-				adap, type, 0, &pdata, hw_addrs[idx], NULL);
+		sd = v4l2_i2c_new_subdev_board(&itv->v4l2_dev, adap,
+				&cx25840_info, NULL);
 	} else {
 		sd = v4l2_i2c_new_subdev(&itv->v4l2_dev,
 				adap, type, hw_addrs[idx], NULL);
diff --git a/drivers/media/video/mt9v011.c b/drivers/media/video/mt9v011.c
index 209ff97..f3f73d3 100644
--- a/drivers/media/video/mt9v011.c
+++ b/drivers/media/video/mt9v011.c
@@ -12,7 +12,8 @@
 #include <asm/div64.h>
 #include <media/v4l2-device.h>
 #include <media/v4l2-chip-ident.h>
-#include "mt9v011.h"
+#include <media/mt9v011.h>
+#include "mt9v011_regs.h"
 
 MODULE_DESCRIPTION("Micron mt9v011 sensor driver");
 MODULE_AUTHOR("Mauro Carvalho Chehab <mchehab@redhat.com>");
@@ -469,23 +470,6 @@ static int mt9v011_s_mbus_fmt(struct v4l2_subdev *sd, struct v4l2_mbus_framefmt
 	return 0;
 }
 
-static int mt9v011_s_config(struct v4l2_subdev *sd, int dumb, void *data)
-{
-	struct mt9v011 *core = to_mt9v011(sd);
-	unsigned *xtal = data;
-
-	v4l2_dbg(1, debug, sd, "s_config called\n");
-
-	if (xtal) {
-		core->xtal = *xtal;
-		v4l2_dbg(1, debug, sd, "xtal set to %d.%03d MHz\n",
-			 *xtal / 1000000, (*xtal / 1000) % 1000);
-	}
-
-	return 0;
-}
-
-
 #ifdef CONFIG_VIDEO_ADV_DEBUG
 static int mt9v011_g_register(struct v4l2_subdev *sd,
 			      struct v4l2_dbg_register *reg)
@@ -536,7 +520,6 @@ static const struct v4l2_subdev_core_ops mt9v011_core_ops = {
 	.g_ctrl = mt9v011_g_ctrl,
 	.s_ctrl = mt9v011_s_ctrl,
 	.reset = mt9v011_reset,
-	.s_config = mt9v011_s_config,
 	.g_chip_ident = mt9v011_g_chip_ident,
 #ifdef CONFIG_VIDEO_ADV_DEBUG
 	.g_register = mt9v011_g_register,
@@ -596,6 +579,14 @@ static int mt9v011_probe(struct i2c_client *c,
 	core->height = 480;
 	core->xtal = 27000000;	/* Hz */
 
+	if (c->dev.platform_data) {
+		struct mt9v011_platform_data *pdata = c->dev.platform_data;
+
+		core->xtal = pdata->xtal;
+		v4l2_dbg(1, debug, sd, "xtal set to %d.%03d MHz\n",
+			core->xtal / 1000000, (core->xtal / 1000) % 1000);
+	}
+
 	v4l_info(c, "chip found @ 0x%02x (%s - chip version 0x%04x)\n",
 		 c->addr << 1, c->adapter->name, version);
 
diff --git a/drivers/media/video/mt9v011.h b/drivers/media/video/mt9v011.h
deleted file mode 100644
index 3350fd6..0000000
--- a/drivers/media/video/mt9v011.h
+++ /dev/null
@@ -1,36 +0,0 @@
-/*
- * mt9v011 -Micron 1/4-Inch VGA Digital Image Sensor
- *
- * Copyright (c) 2009 Mauro Carvalho Chehab (mchehab@redhat.com)
- * This code is placed under the terms of the GNU General Public License v2
- */
-
-#ifndef MT9V011_H_
-#define MT9V011_H_
-
-#define R00_MT9V011_CHIP_VERSION	0x00
-#define R01_MT9V011_ROWSTART		0x01
-#define R02_MT9V011_COLSTART		0x02
-#define R03_MT9V011_HEIGHT		0x03
-#define R04_MT9V011_WIDTH		0x04
-#define R05_MT9V011_HBLANK		0x05
-#define R06_MT9V011_VBLANK		0x06
-#define R07_MT9V011_OUT_CTRL		0x07
-#define R09_MT9V011_SHUTTER_WIDTH	0x09
-#define R0A_MT9V011_CLK_SPEED		0x0a
-#define R0B_MT9V011_RESTART		0x0b
-#define R0C_MT9V011_SHUTTER_DELAY	0x0c
-#define R0D_MT9V011_RESET		0x0d
-#define R1E_MT9V011_DIGITAL_ZOOM	0x1e
-#define R20_MT9V011_READ_MODE		0x20
-#define R2B_MT9V011_GREEN_1_GAIN	0x2b
-#define R2C_MT9V011_BLUE_GAIN		0x2c
-#define R2D_MT9V011_RED_GAIN		0x2d
-#define R2E_MT9V011_GREEN_2_GAIN	0x2e
-#define R35_MT9V011_GLOBAL_GAIN		0x35
-#define RF1_MT9V011_CHIP_ENABLE		0xf1
-
-#define MT9V011_VERSION			0x8232
-#define MT9V011_REV_B_VERSION		0x8243
-
-#endif
diff --git a/drivers/media/video/mt9v011_regs.h b/drivers/media/video/mt9v011_regs.h
new file mode 100644
index 0000000..d6a5a08
--- /dev/null
+++ b/drivers/media/video/mt9v011_regs.h
@@ -0,0 +1,36 @@
+/*
+ * mt9v011 -Micron 1/4-Inch VGA Digital Image Sensor
+ *
+ * Copyright (c) 2009 Mauro Carvalho Chehab (mchehab@redhat.com)
+ * This code is placed under the terms of the GNU General Public License v2
+ */
+
+#ifndef MT9V011_REGS_H_
+#define MT9V011_REGS_H_
+
+#define R00_MT9V011_CHIP_VERSION	0x00
+#define R01_MT9V011_ROWSTART		0x01
+#define R02_MT9V011_COLSTART		0x02
+#define R03_MT9V011_HEIGHT		0x03
+#define R04_MT9V011_WIDTH		0x04
+#define R05_MT9V011_HBLANK		0x05
+#define R06_MT9V011_VBLANK		0x06
+#define R07_MT9V011_OUT_CTRL		0x07
+#define R09_MT9V011_SHUTTER_WIDTH	0x09
+#define R0A_MT9V011_CLK_SPEED		0x0a
+#define R0B_MT9V011_RESTART		0x0b
+#define R0C_MT9V011_SHUTTER_DELAY	0x0c
+#define R0D_MT9V011_RESET		0x0d
+#define R1E_MT9V011_DIGITAL_ZOOM	0x1e
+#define R20_MT9V011_READ_MODE		0x20
+#define R2B_MT9V011_GREEN_1_GAIN	0x2b
+#define R2C_MT9V011_BLUE_GAIN		0x2c
+#define R2D_MT9V011_RED_GAIN		0x2d
+#define R2E_MT9V011_GREEN_2_GAIN	0x2e
+#define R35_MT9V011_GLOBAL_GAIN		0x35
+#define RF1_MT9V011_CHIP_ENABLE		0xf1
+
+#define MT9V011_VERSION			0x8232
+#define MT9V011_REV_B_VERSION		0x8243
+
+#endif
diff --git a/drivers/media/video/ov7670.c b/drivers/media/video/ov7670.c
index c881a64..d4e7c11 100644
--- a/drivers/media/video/ov7670.c
+++ b/drivers/media/video/ov7670.c
@@ -1449,47 +1449,6 @@ static int ov7670_g_chip_ident(struct v4l2_subdev *sd,
 	return v4l2_chip_ident_i2c_client(client, chip, V4L2_IDENT_OV7670, 0);
 }
 
-static int ov7670_s_config(struct v4l2_subdev *sd, int dumb, void *data)
-{
-	struct i2c_client *client = v4l2_get_subdevdata(sd);
-	struct ov7670_config *config = data;
-	struct ov7670_info *info = to_state(sd);
-	int ret;
-
-	info->clock_speed = 30; /* default: a guess */
-
-	/*
-	 * Must apply configuration before initializing device, because it
-	 * selects I/O method.
-	 */
-	if (config) {
-		info->min_width = config->min_width;
-		info->min_height = config->min_height;
-		info->use_smbus = config->use_smbus;
-
-		if (config->clock_speed)
-			info->clock_speed = config->clock_speed;
-	}
-
-	/* Make sure it's an ov7670 */
-	ret = ov7670_detect(sd);
-	if (ret) {
-		v4l_dbg(1, debug, client,
-			"chip found @ 0x%x (%s) is not an ov7670 chip.\n",
-			client->addr << 1, client->adapter->name);
-		kfree(info);
-		return ret;
-	}
-	v4l_info(client, "chip found @ 0x%02x (%s)\n",
-			client->addr << 1, client->adapter->name);
-
-	info->fmt = &ov7670_formats[0];
-	info->sat = 128;	/* Review this */
-	info->clkrc = info->clock_speed / 30;
-
-	return 0;
-}
-
 #ifdef CONFIG_VIDEO_ADV_DEBUG
 static int ov7670_g_register(struct v4l2_subdev *sd, struct v4l2_dbg_register *reg)
 {
@@ -1528,7 +1487,6 @@ static const struct v4l2_subdev_core_ops ov7670_core_ops = {
 	.s_ctrl = ov7670_s_ctrl,
 	.queryctrl = ov7670_queryctrl,
 	.reset = ov7670_reset,
-	.s_config = ov7670_s_config,
 	.init = ov7670_init,
 #ifdef CONFIG_VIDEO_ADV_DEBUG
 	.g_register = ov7670_g_register,
@@ -1558,6 +1516,7 @@ static int ov7670_probe(struct i2c_client *client,
 {
 	struct v4l2_subdev *sd;
 	struct ov7670_info *info;
+	int ret;
 
 	info = kzalloc(sizeof(struct ov7670_info), GFP_KERNEL);
 	if (info == NULL)
@@ -1565,6 +1524,37 @@ static int ov7670_probe(struct i2c_client *client,
 	sd = &info->sd;
 	v4l2_i2c_subdev_init(sd, client, &ov7670_ops);
 
+	info->clock_speed = 30; /* default: a guess */
+	if (client->dev.platform_data) {
+		struct ov7670_config *config = client->dev.platform_data;
+
+		/*
+		 * Must apply configuration before initializing device, because it
+		 * selects I/O method.
+		 */
+		info->min_width = config->min_width;
+		info->min_height = config->min_height;
+		info->use_smbus = config->use_smbus;
+
+		if (config->clock_speed)
+			info->clock_speed = config->clock_speed;
+	}
+
+	/* Make sure it's an ov7670 */
+	ret = ov7670_detect(sd);
+	if (ret) {
+		v4l_dbg(1, debug, client,
+			"chip found @ 0x%x (%s) is not an ov7670 chip.\n",
+			client->addr << 1, client->adapter->name);
+		kfree(info);
+		return ret;
+	}
+	v4l_info(client, "chip found @ 0x%02x (%s)\n",
+			client->addr << 1, client->adapter->name);
+
+	info->fmt = &ov7670_formats[0];
+	info->sat = 128;	/* Review this */
+	info->clkrc = info->clock_speed / 30;
 	return 0;
 }
 
diff --git a/drivers/media/video/sr030pc30.c b/drivers/media/video/sr030pc30.c
index 864696b..c901721 100644
--- a/drivers/media/video/sr030pc30.c
+++ b/drivers/media/video/sr030pc30.c
@@ -714,15 +714,6 @@ static int sr030pc30_base_config(struct v4l2_subdev *sd)
 	return ret;
 }
 
-static int sr030pc30_s_config(struct v4l2_subdev *sd,
-			      int irq, void *platform_data)
-{
-	struct sr030pc30_info *info = to_sr030pc30(sd);
-
-	info->pdata = platform_data;
-	return 0;
-}
-
 static int sr030pc30_s_stream(struct v4l2_subdev *sd, int enable)
 {
 	return 0;
@@ -763,7 +754,6 @@ static int sr030pc30_s_power(struct v4l2_subdev *sd, int on)
 }
 
 static const struct v4l2_subdev_core_ops sr030pc30_core_ops = {
-	.s_config	= sr030pc30_s_config,
 	.s_power	= sr030pc30_s_power,
 	.queryctrl	= sr030pc30_queryctrl,
 	.s_ctrl		= sr030pc30_s_ctrl,
diff --git a/drivers/media/video/v4l2-common.c b/drivers/media/video/v4l2-common.c
index 3f0871b..810eef4 100644
--- a/drivers/media/video/v4l2-common.c
+++ b/drivers/media/video/v4l2-common.c
@@ -407,18 +407,6 @@ struct v4l2_subdev *v4l2_i2c_new_subdev_board(struct v4l2_device *v4l2_dev,
 	/* Decrease the module use count to match the first try_module_get. */
 	module_put(client->driver->driver.owner);
 
-	if (sd) {
-		/* We return errors from v4l2_subdev_call only if we have the
-		   callback as the .s_config is not mandatory */
-		int err = v4l2_subdev_call(sd, core, s_config,
-				info->irq, info->platform_data);
-
-		if (err && err != -ENOIOCTLCMD) {
-			v4l2_device_unregister_subdev(sd);
-			sd = NULL;
-		}
-	}
-
 error:
 	/* If we have a client but no subdev, then something went wrong and
 	   we must unregister the client. */
@@ -428,9 +416,8 @@ error:
 }
 EXPORT_SYMBOL_GPL(v4l2_i2c_new_subdev_board);
 
-struct v4l2_subdev *v4l2_i2c_new_subdev_cfg(struct v4l2_device *v4l2_dev,
+struct v4l2_subdev *v4l2_i2c_new_subdev(struct v4l2_device *v4l2_dev,
 		struct i2c_adapter *adapter, const char *client_type,
-		int irq, void *platform_data,
 		u8 addr, const unsigned short *probe_addrs)
 {
 	struct i2c_board_info info;
@@ -440,12 +427,10 @@ struct v4l2_subdev *v4l2_i2c_new_subdev_cfg(struct v4l2_device *v4l2_dev,
 	memset(&info, 0, sizeof(info));
 	strlcpy(info.type, client_type, sizeof(info.type));
 	info.addr = addr;
-	info.irq = irq;
-	info.platform_data = platform_data;
 
 	return v4l2_i2c_new_subdev_board(v4l2_dev, adapter, &info, probe_addrs);
 }
-EXPORT_SYMBOL_GPL(v4l2_i2c_new_subdev_cfg);
+EXPORT_SYMBOL_GPL(v4l2_i2c_new_subdev);
 
 /* Return i2c client address of v4l2_subdev. */
 unsigned short v4l2_i2c_subdev_addr(struct v4l2_subdev *sd)
diff --git a/include/media/mt9v011.h b/include/media/mt9v011.h
new file mode 100644
index 0000000..ea29fc7
--- /dev/null
+++ b/include/media/mt9v011.h
@@ -0,0 +1,17 @@
+/* mt9v011 sensor
+ *
+ * Copyright (C) 2011 Hans Verkuil <hverkuil@xs4all.nl>
+ *
+ * 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.
+ */
+
+#ifndef __MT9V011_H__
+#define __MT9V011_H__
+
+struct mt9v011_platform_data {
+	unsigned xtal;	/* Hz */
+};
+
+#endif
diff --git a/include/media/v4l2-common.h b/include/media/v4l2-common.h
index 2d65b35..a659319 100644
--- a/include/media/v4l2-common.h
+++ b/include/media/v4l2-common.h
@@ -138,21 +138,10 @@ struct v4l2_subdev_ops;
 
 /* Load an i2c module and return an initialized v4l2_subdev struct.
    The client_type argument is the name of the chip that's on the adapter. */
-struct v4l2_subdev *v4l2_i2c_new_subdev_cfg(struct v4l2_device *v4l2_dev,
+struct v4l2_subdev *v4l2_i2c_new_subdev(struct v4l2_device *v4l2_dev,
 		struct i2c_adapter *adapter, const char *client_type,
-		int irq, void *platform_data,
 		u8 addr, const unsigned short *probe_addrs);
 
-/* Load an i2c module and return an initialized v4l2_subdev struct.
-   The client_type argument is the name of the chip that's on the adapter. */
-static inline struct v4l2_subdev *v4l2_i2c_new_subdev(struct v4l2_device *v4l2_dev,
-		struct i2c_adapter *adapter, const char *client_type,
-		u8 addr, const unsigned short *probe_addrs)
-{
-	return v4l2_i2c_new_subdev_cfg(v4l2_dev, adapter, client_type, 0, NULL,
-				       addr, probe_addrs);
-}
-
 struct i2c_board_info;
 
 struct v4l2_subdev *v4l2_i2c_new_subdev_board(struct v4l2_device *v4l2_dev,
diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
index b0316a7..42fbe46 100644
--- a/include/media/v4l2-subdev.h
+++ b/include/media/v4l2-subdev.h
@@ -106,10 +106,7 @@ struct v4l2_subdev_io_pin_config {
 	u8 strength;	/* Pin drive strength */
 };
 
-/* s_config: if set, then it is always called by the v4l2_i2c_new_subdev*
-	functions after the v4l2_subdev was registered. It is used to pass
-	platform data to the subdev which can be used during initialization.
-
+/*
    s_io_pin_config: configure one or more chip I/O pins for chips that
 	multiplex different internal signal pads out to IO pins.  This function
 	takes a pointer to an array of 'n' pin configuration entries, one for
@@ -141,7 +138,6 @@ struct v4l2_subdev_io_pin_config {
 struct v4l2_subdev_core_ops {
 	int (*g_chip_ident)(struct v4l2_subdev *sd, struct v4l2_dbg_chip_ident *chip);
 	int (*log_status)(struct v4l2_subdev *sd);
-	int (*s_config)(struct v4l2_subdev *sd, int irq, void *platform_data);
 	int (*s_io_pin_config)(struct v4l2_subdev *sd, size_t n,
 				      struct v4l2_subdev_io_pin_config *pincfg);
 	int (*init)(struct v4l2_subdev *sd, u32 val);
-- 
1.7.0.4

[RFC PATCH 2/5] v4l2-subdev: add (un)register internal ops

[Hans Verkuil]

Some subdevs need to call into the board code after they are registered
and have a valid struct v4l2_device pointer. The s_config op was abused
for this, but now that it is removed we need a cleaner way of solving this.

So this patch adds a struct with internal ops that the v4l2 core can call.

Currently only two ops exist: register and unregister. Subdevs can implement
these to call the board code and pass it the v4l2_device pointer, which the
board code can then use to get access to the struct that embeds the
v4l2_device.

It is expected that in the future open and close ops will also be added.

Signed-off-by: Hans Verkuil <hverkuil@xs4all.nl>
---
 drivers/media/video/v4l2-device.c |    4 ++++
 include/media/v4l2-subdev.h       |   17 +++++++++++++++++
 2 files changed, 21 insertions(+), 0 deletions(-)

diff --git a/drivers/media/video/v4l2-device.c b/drivers/media/video/v4l2-device.c
index 7fe6f92..0780c32 100644
--- a/drivers/media/video/v4l2-device.c
+++ b/drivers/media/video/v4l2-device.c
@@ -131,6 +131,8 @@ int v4l2_device_register_subdev(struct v4l2_device *v4l2_dev,
 	if (err)
 		return err;
 	sd->v4l2_dev = v4l2_dev;
+	if (sd->internal_ops && sd->internal_ops->registered)
+		sd->internal_ops->registered(sd);
 	spin_lock(&v4l2_dev->lock);
 	list_add_tail(&sd->list, &v4l2_dev->subdevs);
 	spin_unlock(&v4l2_dev->lock);
@@ -146,6 +148,8 @@ void v4l2_device_unregister_subdev(struct v4l2_subdev *sd)
 	spin_lock(&sd->v4l2_dev->lock);
 	list_del(&sd->list);
 	spin_unlock(&sd->v4l2_dev->lock);
+	if (sd->internal_ops && sd->internal_ops->unregistered)
+		sd->internal_ops->unregistered(sd);
 	sd->v4l2_dev = NULL;
 	module_put(sd->owner);
 }
diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
index 42fbe46..f559562 100644
--- a/include/media/v4l2-subdev.h
+++ b/include/media/v4l2-subdev.h
@@ -411,6 +411,21 @@ struct v4l2_subdev_ops {
 	const struct v4l2_subdev_sensor_ops	*sensor;
 };
 
+/*
+ * Internal ops. Never call this from drivers, only the v4l2 framework can call
+ * these ops.
+ *
+ * registered: called when this subdev is registered. When called the v4l2_dev
+ *	field is set to the correct v4l2_device.
+ *
+ * unregistered: called when this subdev is unregistered. When called the
+ *	v4l2_dev field is still set to the correct v4l2_device.
+ */
+struct v4l2_subdev_internal_ops {
+	int (*registered)(struct v4l2_subdev *sd);
+	int (*unregistered)(struct v4l2_subdev *sd);
+};
+
 #define V4L2_SUBDEV_NAME_SIZE 32
 
 /* Set this flag if this subdev is a i2c device. */
@@ -427,6 +442,8 @@ struct v4l2_subdev {
 	u32 flags;
 	struct v4l2_device *v4l2_dev;
 	const struct v4l2_subdev_ops *ops;
+	/* Never call these internal ops from within a driver! */
+	const struct v4l2_subdev_internal_ops *internal_ops;
 	/* The control handler of this subdev. May be NULL. */
 	struct v4l2_ctrl_handler *ctrl_handler;
 	/* name must be unique */
-- 
1.7.0.4

Re: [RFC PATCH 2/5] v4l2-subdev: add (un)register internal ops

[Laurent Pinchart]

Hi Hans,

Thanks for the patch.

On Friday 07 January 2011 13:47:32 Hans Verkuil wrote:
> Some subdevs need to call into the board code after they are registered
> and have a valid struct v4l2_device pointer. The s_config op was abused
> for this, but now that it is removed we need a cleaner way of solving this.
> 
> So this patch adds a struct with internal ops that the v4l2 core can call.
> 
> Currently only two ops exist: register and unregister. Subdevs can
> implement these to call the board code and pass it the v4l2_device
> pointer, which the board code can then use to get access to the struct
> that embeds the v4l2_device.
> 
> It is expected that in the future open and close ops will also be added.
> 
> Signed-off-by: Hans Verkuil <hverkuil@xs4all.nl>

Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

-- 
Regards,

Laurent Pinchart

[RFC PATCH 3/5] v4l2-ctrls: v4l2_ctrl_handler_setup must set has_new to 1

[Hans Verkuil]

Drivers can use the has_new field to determine if a new value was specified
for a control. The v4l2_ctrl_handler_setup() must always set this to 1 since
the setup has to force a full update of all controls.

Signed-off-by: Hans Verkuil <hverkuil@xs4all.nl>
---
 drivers/media/video/v4l2-ctrls.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/drivers/media/video/v4l2-ctrls.c b/drivers/media/video/v4l2-ctrls.c
index 8f81efc..64f56bb 100644
--- a/drivers/media/video/v4l2-ctrls.c
+++ b/drivers/media/video/v4l2-ctrls.c
@@ -1280,8 +1280,10 @@ int v4l2_ctrl_handler_setup(struct v4l2_ctrl_handler *hdl)
 		if (ctrl->done)
 			continue;
 
-		for (i = 0; i < master->ncontrols; i++)
+		for (i = 0; i < master->ncontrols; i++) {
 			cur_to_new(master->cluster[i]);
+			master->cluster[i]->has_new = 1;
+		}
 
 		/* Skip button controls and read-only controls. */
 		if (ctrl->type == V4L2_CTRL_TYPE_BUTTON ||
-- 
1.7.0.4

Re: [RFC PATCH 3/5] v4l2-ctrls: v4l2_ctrl_handler_setup must set has_new to 1

[Laurent Pinchart]

Hi Hans,

On Friday 07 January 2011 13:47:33 Hans Verkuil wrote:
> Drivers can use the has_new field to determine if a new value was specified
> for a control. The v4l2_ctrl_handler_setup() must always set this to 1
> since the setup has to force a full update of all controls.

According to include/media/v4l2-ctrls.h, has_new is a "Internal flag: set when 
there is a valid new value". Drivers should then not use it.

If you want to use the flag in a driver, the comment should be changed and its 
usage should be documented in Documentation/video4linux/v4l2-controls.txt.

> Signed-off-by: Hans Verkuil <hverkuil@xs4all.nl>
> ---
>  drivers/media/video/v4l2-ctrls.c |    4 +++-
>  1 files changed, 3 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/media/video/v4l2-ctrls.c
> b/drivers/media/video/v4l2-ctrls.c index 8f81efc..64f56bb 100644
> --- a/drivers/media/video/v4l2-ctrls.c
> +++ b/drivers/media/video/v4l2-ctrls.c
> @@ -1280,8 +1280,10 @@ int v4l2_ctrl_handler_setup(struct v4l2_ctrl_handler
> *hdl) if (ctrl->done)
>  			continue;
> 
> -		for (i = 0; i < master->ncontrols; i++)
> +		for (i = 0; i < master->ncontrols; i++) {
>  			cur_to_new(master->cluster[i]);
> +			master->cluster[i]->has_new = 1;
> +		}
> 
>  		/* Skip button controls and read-only controls. */
>  		if (ctrl->type == V4L2_CTRL_TYPE_BUTTON ||

-- 
Regards,

Laurent Pinchart

Re: [RFC PATCH 3/5] v4l2-ctrls: v4l2_ctrl_handler_setup must set has_new to 1

[Hans Verkuil]

On Friday, January 07, 2011 14:25:41 Laurent Pinchart wrote:
> Hi Hans,
> 
> On Friday 07 January 2011 13:47:33 Hans Verkuil wrote:
> > Drivers can use the has_new field to determine if a new value was specified
> > for a control. The v4l2_ctrl_handler_setup() must always set this to 1
> > since the setup has to force a full update of all controls.
> 
> According to include/media/v4l2-ctrls.h, has_new is a "Internal flag: set when 
> there is a valid new value". Drivers should then not use it.
> 
> If you want to use the flag in a driver, the comment should be changed and its 
> usage should be documented in Documentation/video4linux/v4l2-controls.txt.

Good catch. I'll document this.

Regards,

	Hans

> 
> > Signed-off-by: Hans Verkuil <hverkuil@xs4all.nl>
> > ---
> >  drivers/media/video/v4l2-ctrls.c |    4 +++-
> >  1 files changed, 3 insertions(+), 1 deletions(-)
> > 
> > diff --git a/drivers/media/video/v4l2-ctrls.c
> > b/drivers/media/video/v4l2-ctrls.c index 8f81efc..64f56bb 100644
> > --- a/drivers/media/video/v4l2-ctrls.c
> > +++ b/drivers/media/video/v4l2-ctrls.c
> > @@ -1280,8 +1280,10 @@ int v4l2_ctrl_handler_setup(struct v4l2_ctrl_handler
> > *hdl) if (ctrl->done)
> >  			continue;
> > 
> > -		for (i = 0; i < master->ncontrols; i++)
> > +		for (i = 0; i < master->ncontrols; i++) {
> >  			cur_to_new(master->cluster[i]);
> > +			master->cluster[i]->has_new = 1;
> > +		}
> > 
> >  		/* Skip button controls and read-only controls. */
> >  		if (ctrl->type == V4L2_CTRL_TYPE_BUTTON ||
> 
> 

-- 
Hans Verkuil - video4linux developer - sponsored by Cisco

[RFC PATCH 4/5] ov7670: use the control framework

[Hans Verkuil]

Signed-off-by: Hans Verkuil <hverkuil@xs4all.nl>
---
 drivers/media/video/ov7670.c |  296 ++++++++++++++++-------------------------
 1 files changed, 116 insertions(+), 180 deletions(-)

diff --git a/drivers/media/video/ov7670.c b/drivers/media/video/ov7670.c
index d4e7c11..4da8b62 100644
--- a/drivers/media/video/ov7670.c
+++ b/drivers/media/video/ov7670.c
@@ -19,6 +19,7 @@
 #include <media/v4l2-device.h>
 #include <media/v4l2-chip-ident.h>
 #include <media/v4l2-mediabus.h>
+#include <media/v4l2-ctrls.h>
 
 #include "ov7670.h"
 
@@ -191,9 +192,23 @@ MODULE_PARM_DESC(debug, "Debug level (0-1)");
 struct ov7670_format_struct;  /* coming later */
 struct ov7670_info {
 	struct v4l2_subdev sd;
+	struct v4l2_ctrl_handler hdl;
+	struct {
+		/* gain cluster */
+		struct v4l2_ctrl *auto_gain;
+		struct v4l2_ctrl *gain;
+	};
+	struct {
+		/* exposure cluster */
+		struct v4l2_ctrl *auto_exposure;
+		struct v4l2_ctrl *exposure;
+	};
+	struct {
+		/* saturation/hue cluster */
+		struct v4l2_ctrl *saturation;
+		struct v4l2_ctrl *hue;
+	};
 	struct ov7670_format_struct *fmt;  /* Current format */
-	unsigned char sat;		/* Saturation value */
-	int hue;			/* Hue value */
 	int min_width;			/* Filter out smaller sizes */
 	int min_height;			/* Filter out smaller sizes */
 	int clock_speed;		/* External clock speed (MHz) */
@@ -206,6 +221,11 @@ static inline struct ov7670_info *to_state(struct v4l2_subdev *sd)
 	return container_of(sd, struct ov7670_info, sd);
 }
 
+static inline struct v4l2_subdev *to_sd(struct v4l2_ctrl *ctrl)
+{
+	return &container_of(ctrl->handler, struct ov7670_info, hdl)->sd;
+}
+
 
 
 /*
@@ -1043,23 +1063,23 @@ static int ov7670_cosine(int theta)
 
 
 static void ov7670_calc_cmatrix(struct ov7670_info *info,
-		int matrix[CMATRIX_LEN])
+		int matrix[CMATRIX_LEN], int sat, int hue)
 {
 	int i;
 	/*
 	 * Apply the current saturation setting first.
 	 */
 	for (i = 0; i < CMATRIX_LEN; i++)
-		matrix[i] = (info->fmt->cmatrix[i]*info->sat) >> 7;
+		matrix[i] = (info->fmt->cmatrix[i] * sat) >> 7;
 	/*
 	 * Then, if need be, rotate the hue value.
 	 */
-	if (info->hue != 0) {
+	if (hue != 0) {
 		int sinth, costh, tmpmatrix[CMATRIX_LEN];
 
 		memcpy(tmpmatrix, matrix, CMATRIX_LEN*sizeof(int));
-		sinth = ov7670_sine(info->hue);
-		costh = ov7670_cosine(info->hue);
+		sinth = ov7670_sine(hue);
+		costh = ov7670_cosine(hue);
 
 		matrix[0] = (matrix[3]*sinth + matrix[0]*costh)/1000;
 		matrix[1] = (matrix[4]*sinth + matrix[1]*costh)/1000;
@@ -1072,60 +1092,21 @@ static void ov7670_calc_cmatrix(struct ov7670_info *info,
 
 
 
-static int ov7670_s_sat(struct v4l2_subdev *sd, int value)
-{
-	struct ov7670_info *info = to_state(sd);
-	int matrix[CMATRIX_LEN];
-	int ret;
-
-	info->sat = value;
-	ov7670_calc_cmatrix(info, matrix);
-	ret = ov7670_store_cmatrix(sd, matrix);
-	return ret;
-}
-
-static int ov7670_g_sat(struct v4l2_subdev *sd, __s32 *value)
-{
-	struct ov7670_info *info = to_state(sd);
-
-	*value = info->sat;
-	return 0;
-}
-
-static int ov7670_s_hue(struct v4l2_subdev *sd, int value)
+static int ov7670_s_sat_hue(struct v4l2_subdev *sd, int sat, int hue)
 {
 	struct ov7670_info *info = to_state(sd);
 	int matrix[CMATRIX_LEN];
 	int ret;
 
-	if (value < -180 || value > 180)
-		return -EINVAL;
-	info->hue = value;
-	ov7670_calc_cmatrix(info, matrix);
+	ov7670_calc_cmatrix(info, matrix, sat, hue);
 	ret = ov7670_store_cmatrix(sd, matrix);
 	return ret;
 }
 
 
-static int ov7670_g_hue(struct v4l2_subdev *sd, __s32 *value)
-{
-	struct ov7670_info *info = to_state(sd);
-
-	*value = info->hue;
-	return 0;
-}
-
-
 /*
  * Some weird registers seem to store values in a sign/magnitude format!
  */
-static unsigned char ov7670_sm_to_abs(unsigned char v)
-{
-	if ((v & 0x80) == 0)
-		return v + 128;
-	return 128 - (v & 0x7f);
-}
-
 
 static unsigned char ov7670_abs_to_sm(unsigned char v)
 {
@@ -1147,40 +1128,11 @@ static int ov7670_s_brightness(struct v4l2_subdev *sd, int value)
 	return ret;
 }
 
-static int ov7670_g_brightness(struct v4l2_subdev *sd, __s32 *value)
-{
-	unsigned char v = 0;
-	int ret = ov7670_read(sd, REG_BRIGHT, &v);
-
-	*value = ov7670_sm_to_abs(v);
-	return ret;
-}
-
 static int ov7670_s_contrast(struct v4l2_subdev *sd, int value)
 {
 	return ov7670_write(sd, REG_CONTRAS, (unsigned char) value);
 }
 
-static int ov7670_g_contrast(struct v4l2_subdev *sd, __s32 *value)
-{
-	unsigned char v = 0;
-	int ret = ov7670_read(sd, REG_CONTRAS, &v);
-
-	*value = v;
-	return ret;
-}
-
-static int ov7670_g_hflip(struct v4l2_subdev *sd, __s32 *value)
-{
-	int ret;
-	unsigned char v = 0;
-
-	ret = ov7670_read(sd, REG_MVFP, &v);
-	*value = (v & MVFP_MIRROR) == MVFP_MIRROR;
-	return ret;
-}
-
-
 static int ov7670_s_hflip(struct v4l2_subdev *sd, int value)
 {
 	unsigned char v = 0;
@@ -1196,19 +1148,6 @@ static int ov7670_s_hflip(struct v4l2_subdev *sd, int value)
 	return ret;
 }
 
-
-
-static int ov7670_g_vflip(struct v4l2_subdev *sd, __s32 *value)
-{
-	int ret;
-	unsigned char v = 0;
-
-	ret = ov7670_read(sd, REG_MVFP, &v);
-	*value = (v & MVFP_FLIP) == MVFP_FLIP;
-	return ret;
-}
-
-
 static int ov7670_s_vflip(struct v4l2_subdev *sd, int value)
 {
 	unsigned char v = 0;
@@ -1257,16 +1196,6 @@ static int ov7670_s_gain(struct v4l2_subdev *sd, int value)
 /*
  * Tweak autogain.
  */
-static int ov7670_g_autogain(struct v4l2_subdev *sd, __s32 *value)
-{
-	int ret;
-	unsigned char com8;
-
-	ret = ov7670_read(sd, REG_COM8, &com8);
-	*value = (com8 & COM8_AGC) != 0;
-	return ret;
-}
-
 static int ov7670_s_autogain(struct v4l2_subdev *sd, int value)
 {
 	int ret;
@@ -1325,20 +1254,6 @@ static int ov7670_s_exp(struct v4l2_subdev *sd, int value)
 /*
  * Tweak autoexposure.
  */
-static int ov7670_g_autoexp(struct v4l2_subdev *sd, __s32 *value)
-{
-	int ret;
-	unsigned char com8;
-	enum v4l2_exposure_auto_type *atype = (enum v4l2_exposure_auto_type *) value;
-
-	ret = ov7670_read(sd, REG_COM8, &com8);
-	if (com8 & COM8_AEC)
-		*atype = V4L2_EXPOSURE_AUTO;
-	else
-		*atype = V4L2_EXPOSURE_MANUAL;
-	return ret;
-}
-
 static int ov7670_s_autoexp(struct v4l2_subdev *sd,
 		enum v4l2_exposure_auto_type value)
 {
@@ -1357,90 +1272,69 @@ static int ov7670_s_autoexp(struct v4l2_subdev *sd,
 }
 
 
-
-static int ov7670_queryctrl(struct v4l2_subdev *sd,
-		struct v4l2_queryctrl *qc)
+static int ov7670_g_volatile_ctrl(struct v4l2_ctrl *ctrl)
 {
-	/* Fill in min, max, step and default value for these controls. */
-	switch (qc->id) {
-	case V4L2_CID_BRIGHTNESS:
-		return v4l2_ctrl_query_fill(qc, 0, 255, 1, 128);
-	case V4L2_CID_CONTRAST:
-		return v4l2_ctrl_query_fill(qc, 0, 127, 1, 64);
-	case V4L2_CID_VFLIP:
-	case V4L2_CID_HFLIP:
-		return v4l2_ctrl_query_fill(qc, 0, 1, 1, 0);
-	case V4L2_CID_SATURATION:
-		return v4l2_ctrl_query_fill(qc, 0, 256, 1, 128);
-	case V4L2_CID_HUE:
-		return v4l2_ctrl_query_fill(qc, -180, 180, 5, 0);
-	case V4L2_CID_GAIN:
-		return v4l2_ctrl_query_fill(qc, 0, 255, 1, 128);
-	case V4L2_CID_AUTOGAIN:
-		return v4l2_ctrl_query_fill(qc, 0, 1, 1, 1);
-	case V4L2_CID_EXPOSURE:
-		return v4l2_ctrl_query_fill(qc, 0, 65535, 1, 500);
-	case V4L2_CID_EXPOSURE_AUTO:
-		return v4l2_ctrl_query_fill(qc, 0, 1, 1, 0);
-	}
-	return -EINVAL;
-}
+	struct v4l2_subdev *sd = to_sd(ctrl);
+	struct ov7670_info *info = to_state(sd);
 
-static int ov7670_g_ctrl(struct v4l2_subdev *sd, struct v4l2_control *ctrl)
-{
 	switch (ctrl->id) {
-	case V4L2_CID_BRIGHTNESS:
-		return ov7670_g_brightness(sd, &ctrl->value);
-	case V4L2_CID_CONTRAST:
-		return ov7670_g_contrast(sd, &ctrl->value);
-	case V4L2_CID_SATURATION:
-		return ov7670_g_sat(sd, &ctrl->value);
-	case V4L2_CID_HUE:
-		return ov7670_g_hue(sd, &ctrl->value);
-	case V4L2_CID_VFLIP:
-		return ov7670_g_vflip(sd, &ctrl->value);
-	case V4L2_CID_HFLIP:
-		return ov7670_g_hflip(sd, &ctrl->value);
-	case V4L2_CID_GAIN:
-		return ov7670_g_gain(sd, &ctrl->value);
 	case V4L2_CID_AUTOGAIN:
-		return ov7670_g_autogain(sd, &ctrl->value);
-	case V4L2_CID_EXPOSURE:
-		return ov7670_g_exp(sd, &ctrl->value);
+		if (!ctrl->cur.val)
+			return 0;
+		return ov7670_g_gain(sd, &info->gain->cur.val);
 	case V4L2_CID_EXPOSURE_AUTO:
-		return ov7670_g_autoexp(sd, &ctrl->value);
+		if (ctrl->cur.val == V4L2_EXPOSURE_MANUAL)
+			return 0;
+		return ov7670_g_exp(sd, &info->exposure->cur.val);
 	}
 	return -EINVAL;
 }
 
-static int ov7670_s_ctrl(struct v4l2_subdev *sd, struct v4l2_control *ctrl)
+static int ov7670_s_ctrl(struct v4l2_ctrl *ctrl)
 {
+	struct v4l2_subdev *sd = to_sd(ctrl);
+	struct ov7670_info *info = to_state(sd);
+
 	switch (ctrl->id) {
 	case V4L2_CID_BRIGHTNESS:
-		return ov7670_s_brightness(sd, ctrl->value);
+		return ov7670_s_brightness(sd, ctrl->val);
 	case V4L2_CID_CONTRAST:
-		return ov7670_s_contrast(sd, ctrl->value);
+		return ov7670_s_contrast(sd, ctrl->val);
 	case V4L2_CID_SATURATION:
-		return ov7670_s_sat(sd, ctrl->value);
-	case V4L2_CID_HUE:
-		return ov7670_s_hue(sd, ctrl->value);
+		return ov7670_s_sat_hue(sd,
+				info->saturation->val, info->hue->val);
 	case V4L2_CID_VFLIP:
-		return ov7670_s_vflip(sd, ctrl->value);
+		return ov7670_s_vflip(sd, ctrl->val);
 	case V4L2_CID_HFLIP:
-		return ov7670_s_hflip(sd, ctrl->value);
-	case V4L2_CID_GAIN:
-		return ov7670_s_gain(sd, ctrl->value);
+		return ov7670_s_hflip(sd, ctrl->val);
 	case V4L2_CID_AUTOGAIN:
-		return ov7670_s_autogain(sd, ctrl->value);
-	case V4L2_CID_EXPOSURE:
-		return ov7670_s_exp(sd, ctrl->value);
+		/* Only set manual gain if auto gain is not explicitly
+		   turned on. */
+		if (!ctrl->has_new || !ctrl->val) {
+			ctrl->val = 0;
+			/* ov7670_s_gain turns off auto gain */
+			return ov7670_s_gain(sd, info->gain->val);
+		}
+		return ov7670_s_autogain(sd, ctrl->val);
 	case V4L2_CID_EXPOSURE_AUTO:
+		/* Only set manual exposure if auto exposure is not explicitly
+		   turned on. */
+		if (!ctrl->has_new || ctrl->val == V4L2_EXPOSURE_MANUAL) {
+			ctrl->val = V4L2_EXPOSURE_MANUAL;
+			/* ov7670_s_exp turns off auto exposure */
+			return ov7670_s_exp(sd, info->exposure->val);
+		}
 		return ov7670_s_autoexp(sd,
-				(enum v4l2_exposure_auto_type) ctrl->value);
+			(enum v4l2_exposure_auto_type)ctrl->val);
 	}
 	return -EINVAL;
 }
 
+static const struct v4l2_ctrl_ops ov7670_ctrl_ops = {
+	.s_ctrl = ov7670_s_ctrl,
+	.g_volatile_ctrl = ov7670_g_volatile_ctrl,
+};
+
 static int ov7670_g_chip_ident(struct v4l2_subdev *sd,
 		struct v4l2_dbg_chip_ident *chip)
 {
@@ -1483,9 +1377,13 @@ static int ov7670_s_register(struct v4l2_subdev *sd, struct v4l2_dbg_register *r
 
 static const struct v4l2_subdev_core_ops ov7670_core_ops = {
 	.g_chip_ident = ov7670_g_chip_ident,
-	.g_ctrl = ov7670_g_ctrl,
-	.s_ctrl = ov7670_s_ctrl,
-	.queryctrl = ov7670_queryctrl,
+	.g_ext_ctrls = v4l2_subdev_g_ext_ctrls,
+	.try_ext_ctrls = v4l2_subdev_try_ext_ctrls,
+	.s_ext_ctrls = v4l2_subdev_s_ext_ctrls,
+	.g_ctrl = v4l2_subdev_g_ctrl,
+	.s_ctrl = v4l2_subdev_s_ctrl,
+	.queryctrl = v4l2_subdev_queryctrl,
+	.querymenu = v4l2_subdev_querymenu,
 	.reset = ov7670_reset,
 	.init = ov7670_init,
 #ifdef CONFIG_VIDEO_ADV_DEBUG
@@ -1553,8 +1451,44 @@ static int ov7670_probe(struct i2c_client *client,
 			client->addr << 1, client->adapter->name);
 
 	info->fmt = &ov7670_formats[0];
-	info->sat = 128;	/* Review this */
 	info->clkrc = info->clock_speed / 30;
+
+	v4l2_ctrl_handler_init(&info->hdl, 10);
+	v4l2_ctrl_new_std(&info->hdl, &ov7670_ctrl_ops,
+			V4L2_CID_BRIGHTNESS, 0, 255, 1, 128);
+	v4l2_ctrl_new_std(&info->hdl, &ov7670_ctrl_ops,
+			V4L2_CID_CONTRAST, 0, 127, 1, 64);
+	v4l2_ctrl_new_std(&info->hdl, &ov7670_ctrl_ops,
+			V4L2_CID_VFLIP, 0, 1, 1, 0);
+	v4l2_ctrl_new_std(&info->hdl, &ov7670_ctrl_ops,
+			V4L2_CID_HFLIP, 0, 1, 1, 0);
+	info->saturation = v4l2_ctrl_new_std(&info->hdl, &ov7670_ctrl_ops,
+			V4L2_CID_SATURATION, 0, 256, 1, 128);
+	info->hue = v4l2_ctrl_new_std(&info->hdl, &ov7670_ctrl_ops,
+			V4L2_CID_HUE, -180, 180, 5, 0);
+	info->gain = v4l2_ctrl_new_std(&info->hdl, &ov7670_ctrl_ops,
+			V4L2_CID_GAIN, 0, 255, 1, 128);
+	info->auto_gain = v4l2_ctrl_new_std(&info->hdl, &ov7670_ctrl_ops,
+			V4L2_CID_AUTOGAIN, 0, 1, 1, 1);
+	info->exposure = v4l2_ctrl_new_std(&info->hdl, &ov7670_ctrl_ops,
+			V4L2_CID_EXPOSURE, 0, 65535, 1, 500);
+	info->auto_exposure = v4l2_ctrl_new_std_menu(&info->hdl, &ov7670_ctrl_ops,
+			V4L2_CID_EXPOSURE_AUTO, 1, 0, 0);
+	sd->ctrl_handler = &info->hdl;
+	if (info->hdl.error) {
+		int err = info->hdl.error;
+
+		v4l2_ctrl_handler_free(&info->hdl);
+		kfree(info);
+		return err;
+	}
+	info->gain->is_volatile = true;
+	info->exposure->is_volatile = true;
+	v4l2_ctrl_cluster(2, &info->auto_gain);
+	v4l2_ctrl_cluster(2, &info->auto_exposure);
+	v4l2_ctrl_cluster(2, &info->saturation);
+	v4l2_ctrl_handler_setup(&info->hdl);
+
 	return 0;
 }
 
@@ -1562,9 +1496,11 @@ static int ov7670_probe(struct i2c_client *client,
 static int ov7670_remove(struct i2c_client *client)
 {
 	struct v4l2_subdev *sd = i2c_get_clientdata(client);
+	struct ov7670_info *info = to_state(sd);
 
 	v4l2_device_unregister_subdev(sd);
-	kfree(to_state(sd));
+	v4l2_ctrl_handler_free(&info->hdl);
+	kfree(info);
 	return 0;
 }
 
-- 
1.7.0.4

[RFC PATCH 5/5] cafe_ccic: implement the control framework.

[Hans Verkuil]

And also swapped the out_free and out_unreg labels as they were in the
wrong order.

Tested with the OLPC laptop.

Signed-off-by: Hans Verkuil <hverkuil@xs4all.nl>
---
 drivers/media/video/cafe_ccic.c |   59 +++++++-------------------------------
 1 files changed, 11 insertions(+), 48 deletions(-)

diff --git a/drivers/media/video/cafe_ccic.c b/drivers/media/video/cafe_ccic.c
index 7488b47..44ec342 100644
--- a/drivers/media/video/cafe_ccic.c
+++ b/drivers/media/video/cafe_ccic.c
@@ -35,6 +35,7 @@
 #include <linux/slab.h>
 #include <media/v4l2-device.h>
 #include <media/v4l2-ioctl.h>
+#include <media/v4l2-ctrls.h>
 #include <media/v4l2-chip-ident.h>
 #include <linux/device.h>
 #include <linux/wait.h>
@@ -145,6 +146,7 @@ struct cafe_sio_buffer {
 struct cafe_camera
 {
 	struct v4l2_device v4l2_dev;
+	struct v4l2_ctrl_handler ctrl_handler;
 	enum cafe_state state;
 	unsigned long flags;   		/* Buffer status, mainly (dev_lock) */
 	int users;			/* How many open FDs */
@@ -1466,48 +1468,6 @@ static unsigned int cafe_v4l_poll(struct file *filp,
 
 
 
-static int cafe_vidioc_queryctrl(struct file *filp, void *priv,
-		struct v4l2_queryctrl *qc)
-{
-	struct cafe_camera *cam = priv;
-	int ret;
-
-	mutex_lock(&cam->s_mutex);
-	ret = sensor_call(cam, core, queryctrl, qc);
-	mutex_unlock(&cam->s_mutex);
-	return ret;
-}
-
-
-static int cafe_vidioc_g_ctrl(struct file *filp, void *priv,
-		struct v4l2_control *ctrl)
-{
-	struct cafe_camera *cam = priv;
-	int ret;
-
-	mutex_lock(&cam->s_mutex);
-	ret = sensor_call(cam, core, g_ctrl, ctrl);
-	mutex_unlock(&cam->s_mutex);
-	return ret;
-}
-
-
-static int cafe_vidioc_s_ctrl(struct file *filp, void *priv,
-		struct v4l2_control *ctrl)
-{
-	struct cafe_camera *cam = priv;
-	int ret;
-
-	mutex_lock(&cam->s_mutex);
-	ret = sensor_call(cam, core, s_ctrl, ctrl);
-	mutex_unlock(&cam->s_mutex);
-	return ret;
-}
-
-
-
-
-
 static int cafe_vidioc_querycap(struct file *file, void *priv,
 		struct v4l2_capability *cap)
 {
@@ -1792,9 +1752,6 @@ static const struct v4l2_ioctl_ops cafe_v4l_ioctl_ops = {
 	.vidioc_dqbuf		= cafe_vidioc_dqbuf,
 	.vidioc_streamon	= cafe_vidioc_streamon,
 	.vidioc_streamoff	= cafe_vidioc_streamoff,
-	.vidioc_queryctrl	= cafe_vidioc_queryctrl,
-	.vidioc_g_ctrl		= cafe_vidioc_g_ctrl,
-	.vidioc_s_ctrl		= cafe_vidioc_s_ctrl,
 	.vidioc_g_parm		= cafe_vidioc_g_parm,
 	.vidioc_s_parm		= cafe_vidioc_s_parm,
 	.vidioc_enum_framesizes = cafe_vidioc_enum_framesizes,
@@ -2031,6 +1988,10 @@ static int cafe_pci_probe(struct pci_dev *pdev,
 	INIT_LIST_HEAD(&cam->sb_avail);
 	INIT_LIST_HEAD(&cam->sb_full);
 	tasklet_init(&cam->s_tasklet, cafe_frame_tasklet, (unsigned long) cam);
+	ret = v4l2_ctrl_handler_init(&cam->ctrl_handler, 10);
+	if (ret)
+		goto out_unreg;
+	cam->v4l2_dev.ctrl_handler = &cam->ctrl_handler;
 	/*
 	 * Get set up on the PCI bus.
 	 */
@@ -2087,10 +2048,10 @@ static int cafe_pci_probe(struct pci_dev *pdev,
 	cam->vdev.debug = 0;
 /*	cam->vdev.debug = V4L2_DEBUG_IOCTL_ARG;*/
 	cam->vdev.v4l2_dev = &cam->v4l2_dev;
+	video_set_drvdata(&cam->vdev, cam);
 	ret = video_register_device(&cam->vdev, VFL_TYPE_GRABBER, -1);
 	if (ret)
 		goto out_unlock;
-	video_set_drvdata(&cam->vdev, cam);
 
 	/*
 	 * If so requested, try to get our DMA buffers now.
@@ -2113,9 +2074,10 @@ out_freeirq:
 	free_irq(pdev->irq, cam);
 out_iounmap:
 	pci_iounmap(pdev, cam->regs);
-out_free:
-	v4l2_device_unregister(&cam->v4l2_dev);
 out_unreg:
+	v4l2_ctrl_handler_free(&cam->ctrl_handler);
+	v4l2_device_unregister(&cam->v4l2_dev);
+out_free:
 	kfree(cam);
 out:
 	return ret;
@@ -2154,6 +2116,7 @@ static void cafe_pci_remove(struct pci_dev *pdev)
 	if (cam->users > 0)
 		cam_warn(cam, "Removing a device with users!\n");
 	cafe_shutdown(cam);
+	v4l2_ctrl_handler_free(&cam->ctrl_handler);
 	v4l2_device_unregister(&cam->v4l2_dev);
 	kfree(cam);
 /* No unlock - it no longer exists */
-- 
1.7.0.4

Re: [RFC PATCH 1/5] v4l2-subdev: remove core.s_config and v4l2_i2c_new_subdev_cfg()

[Laurent Pinchart]

Hi Hans,

On Friday 07 January 2011 13:47:31 Hans Verkuil wrote:

[snip]
 
> diff --git a/drivers/media/video/cx25840/cx25840-core.c
> b/drivers/media/video/cx25840/cx25840-core.c index dfb198d..9b384ac 100644
> --- a/drivers/media/video/cx25840/cx25840-core.c
> +++ b/drivers/media/video/cx25840/cx25840-core.c
> @@ -1682,20 +1682,6 @@ static int cx25840_log_status(struct v4l2_subdev
> *sd) return 0;
>  }
> 
> -static int cx25840_s_config(struct v4l2_subdev *sd, int irq, void
> *platform_data) -{
> -	struct cx25840_state *state = to_state(sd);
> -	struct i2c_client *client = v4l2_get_subdevdata(sd);
> -
> -	if (platform_data) {
> -		struct cx25840_platform_data *pdata = platform_data;
> -
> -		state->pvr150_workaround = pdata->pvr150_workaround;
> -		set_input(client, state->vid_input, state->aud_input);
> -	}
> -	return 0;
> -}
> -
>  static int cx23885_irq_handler(struct v4l2_subdev *sd, u32 status,
>  			       bool *handled)
>  {

[snip]

> @@ -2019,6 +2003,12 @@ static int cx25840_probe(struct i2c_client *client,
>  	v4l2_ctrl_cluster(2, &state->volume);
>  	v4l2_ctrl_handler_setup(&state->hdl);
> 
> +	if (client->dev.platform_data) {
> +		struct cx25840_platform_data *pdata = client->dev.platform_data;
> +
> +		state->pvr150_workaround = pdata->pvr150_workaround;
> +	}
> +

No need to call set_input() anymore ?

>  	cx25840_ir_probe(sd);
>  	return 0;
>  }

[snip]

> diff --git a/drivers/media/video/mt9v011_regs.h
> b/drivers/media/video/mt9v011_regs.h new file mode 100644
> index 0000000..d6a5a08
> --- /dev/null
> +++ b/drivers/media/video/mt9v011_regs.h
> @@ -0,0 +1,36 @@
> +/*
> + * mt9v011 -Micron 1/4-Inch VGA Digital Image Sensor
> + *
> + * Copyright (c) 2009 Mauro Carvalho Chehab (mchehab@redhat.com)
> + * This code is placed under the terms of the GNU General Public License
> v2 + */
> +
> +#ifndef MT9V011_REGS_H_
> +#define MT9V011_REGS_H_
> +
> +#define R00_MT9V011_CHIP_VERSION	0x00
> +#define R01_MT9V011_ROWSTART		0x01
> +#define R02_MT9V011_COLSTART		0x02
> +#define R03_MT9V011_HEIGHT		0x03
> +#define R04_MT9V011_WIDTH		0x04
> +#define R05_MT9V011_HBLANK		0x05
> +#define R06_MT9V011_VBLANK		0x06
> +#define R07_MT9V011_OUT_CTRL		0x07
> +#define R09_MT9V011_SHUTTER_WIDTH	0x09
> +#define R0A_MT9V011_CLK_SPEED		0x0a
> +#define R0B_MT9V011_RESTART		0x0b
> +#define R0C_MT9V011_SHUTTER_DELAY	0x0c
> +#define R0D_MT9V011_RESET		0x0d
> +#define R1E_MT9V011_DIGITAL_ZOOM	0x1e
> +#define R20_MT9V011_READ_MODE		0x20
> +#define R2B_MT9V011_GREEN_1_GAIN	0x2b
> +#define R2C_MT9V011_BLUE_GAIN		0x2c
> +#define R2D_MT9V011_RED_GAIN		0x2d
> +#define R2E_MT9V011_GREEN_2_GAIN	0x2e
> +#define R35_MT9V011_GLOBAL_GAIN		0x35
> +#define RF1_MT9V011_CHIP_ENABLE		0xf1
> +
> +#define MT9V011_VERSION			0x8232
> +#define MT9V011_REV_B_VERSION		0x8243

What about merging this into mt9v011.c ?

> +
> +#endif

-- 
Regards,

Laurent Pinchart

Re: [RFC PATCH 1/5] v4l2-subdev: remove core.s_config and v4l2_i2c_new_subdev_cfg()

[Hans Verkuil]

On Friday, January 07, 2011 14:53:06 Laurent Pinchart wrote:
> Hi Hans,
> 
> On Friday 07 January 2011 13:47:31 Hans Verkuil wrote:
> 
> [snip]
>  
> > diff --git a/drivers/media/video/cx25840/cx25840-core.c
> > b/drivers/media/video/cx25840/cx25840-core.c index dfb198d..9b384ac 100644
> > --- a/drivers/media/video/cx25840/cx25840-core.c
> > +++ b/drivers/media/video/cx25840/cx25840-core.c
> > @@ -1682,20 +1682,6 @@ static int cx25840_log_status(struct v4l2_subdev
> > *sd) return 0;
> >  }
> > 
> > -static int cx25840_s_config(struct v4l2_subdev *sd, int irq, void
> > *platform_data) -{
> > -	struct cx25840_state *state = to_state(sd);
> > -	struct i2c_client *client = v4l2_get_subdevdata(sd);
> > -
> > -	if (platform_data) {
> > -		struct cx25840_platform_data *pdata = platform_data;
> > -
> > -		state->pvr150_workaround = pdata->pvr150_workaround;
> > -		set_input(client, state->vid_input, state->aud_input);
> > -	}
> > -	return 0;
> > -}
> > -
> >  static int cx23885_irq_handler(struct v4l2_subdev *sd, u32 status,
> >  			       bool *handled)
> >  {
> 
> [snip]
> 
> > @@ -2019,6 +2003,12 @@ static int cx25840_probe(struct i2c_client *client,
> >  	v4l2_ctrl_cluster(2, &state->volume);
> >  	v4l2_ctrl_handler_setup(&state->hdl);
> > 
> > +	if (client->dev.platform_data) {
> > +		struct cx25840_platform_data *pdata = client->dev.platform_data;
> > +
> > +		state->pvr150_workaround = pdata->pvr150_workaround;
> > +	}
> > +
> 
> No need to call set_input() anymore ?

No. :-) This s_config was only called from ivtv, and in that particular case
the set_input wasn't needed at all.

> 
> >  	cx25840_ir_probe(sd);
> >  	return 0;
> >  }
> 
> [snip]
> 
> > diff --git a/drivers/media/video/mt9v011_regs.h
> > b/drivers/media/video/mt9v011_regs.h new file mode 100644
> > index 0000000..d6a5a08
> > --- /dev/null
> > +++ b/drivers/media/video/mt9v011_regs.h
> > @@ -0,0 +1,36 @@
> > +/*
> > + * mt9v011 -Micron 1/4-Inch VGA Digital Image Sensor
> > + *
> > + * Copyright (c) 2009 Mauro Carvalho Chehab (mchehab@redhat.com)
> > + * This code is placed under the terms of the GNU General Public License
> > v2 + */
> > +
> > +#ifndef MT9V011_REGS_H_
> > +#define MT9V011_REGS_H_
> > +
> > +#define R00_MT9V011_CHIP_VERSION	0x00
> > +#define R01_MT9V011_ROWSTART		0x01
> > +#define R02_MT9V011_COLSTART		0x02
> > +#define R03_MT9V011_HEIGHT		0x03
> > +#define R04_MT9V011_WIDTH		0x04
> > +#define R05_MT9V011_HBLANK		0x05
> > +#define R06_MT9V011_VBLANK		0x06
> > +#define R07_MT9V011_OUT_CTRL		0x07
> > +#define R09_MT9V011_SHUTTER_WIDTH	0x09
> > +#define R0A_MT9V011_CLK_SPEED		0x0a
> > +#define R0B_MT9V011_RESTART		0x0b
> > +#define R0C_MT9V011_SHUTTER_DELAY	0x0c
> > +#define R0D_MT9V011_RESET		0x0d
> > +#define R1E_MT9V011_DIGITAL_ZOOM	0x1e
> > +#define R20_MT9V011_READ_MODE		0x20
> > +#define R2B_MT9V011_GREEN_1_GAIN	0x2b
> > +#define R2C_MT9V011_BLUE_GAIN		0x2c
> > +#define R2D_MT9V011_RED_GAIN		0x2d
> > +#define R2E_MT9V011_GREEN_2_GAIN	0x2e
> > +#define R35_MT9V011_GLOBAL_GAIN		0x35
> > +#define RF1_MT9V011_CHIP_ENABLE		0xf1
> > +
> > +#define MT9V011_VERSION			0x8232
> > +#define MT9V011_REV_B_VERSION		0x8243
> 
> What about merging this into mt9v011.c ?

I went back and forth about this. If you think I should, then I can merge it.

Regards,

	Hans

-- 
Hans Verkuil - video4linux developer - sponsored by Cisco

Re: [RFC PATCH 1/5] v4l2-subdev: remove core.s_config and v4l2_i2c_new_subdev_cfg()

[Laurent Pinchart]

Hi Hans,

On Friday 07 January 2011 22:21:39 Hans Verkuil wrote:
> On Friday, January 07, 2011 14:53:06 Laurent Pinchart wrote:
> > On Friday 07 January 2011 13:47:31 Hans Verkuil wrote:

[snip]

> > > diff --git a/drivers/media/video/mt9v011_regs.h
> > > b/drivers/media/video/mt9v011_regs.h new file mode 100644
> > > index 0000000..d6a5a08
> > > --- /dev/null
> > > +++ b/drivers/media/video/mt9v011_regs.h
> > > @@ -0,0 +1,36 @@
> > > +/*
> > > + * mt9v011 -Micron 1/4-Inch VGA Digital Image Sensor
> > > + *
> > > + * Copyright (c) 2009 Mauro Carvalho Chehab (mchehab@redhat.com)
> > > + * This code is placed under the terms of the GNU General Public
> > > License v2 + */
> > > +
> > > +#ifndef MT9V011_REGS_H_
> > > +#define MT9V011_REGS_H_
> > > +
> > > +#define R00_MT9V011_CHIP_VERSION	0x00
> > > +#define R01_MT9V011_ROWSTART		0x01
> > > +#define R02_MT9V011_COLSTART		0x02
> > > +#define R03_MT9V011_HEIGHT		0x03
> > > +#define R04_MT9V011_WIDTH		0x04
> > > +#define R05_MT9V011_HBLANK		0x05
> > > +#define R06_MT9V011_VBLANK		0x06
> > > +#define R07_MT9V011_OUT_CTRL		0x07
> > > +#define R09_MT9V011_SHUTTER_WIDTH	0x09
> > > +#define R0A_MT9V011_CLK_SPEED		0x0a
> > > +#define R0B_MT9V011_RESTART		0x0b
> > > +#define R0C_MT9V011_SHUTTER_DELAY	0x0c
> > > +#define R0D_MT9V011_RESET		0x0d
> > > +#define R1E_MT9V011_DIGITAL_ZOOM	0x1e
> > > +#define R20_MT9V011_READ_MODE		0x20
> > > +#define R2B_MT9V011_GREEN_1_GAIN	0x2b
> > > +#define R2C_MT9V011_BLUE_GAIN		0x2c
> > > +#define R2D_MT9V011_RED_GAIN		0x2d
> > > +#define R2E_MT9V011_GREEN_2_GAIN	0x2e
> > > +#define R35_MT9V011_GLOBAL_GAIN		0x35
> > > +#define RF1_MT9V011_CHIP_ENABLE		0xf1
> > > +
> > > +#define MT9V011_VERSION			0x8232
> > > +#define MT9V011_REV_B_VERSION		0x8243
> > 
> > What about merging this into mt9v011.c ?
> 
> I went back and forth about this. If you think I should, then I can merge
> it.

There's few constants, and they're only used from a single source file. I 
would merge them there.

-- 
Regards,

Laurent Pinchart

Re: [RFC PATCH 1/5] v4l2-subdev: remove core.s_config and v4l2_i2c_new_subdev_cfg()

[Sylwester Nawrocki]

Hi Hans,

On 01/07/2011 01:47 PM, Hans Verkuil wrote:
> The core.s_config op was meant for legacy drivers that needed to work with old
> pre-2.6.26 kernels. This is no longer relevant. Unfortunately, this op was
> incorrectly called from several drivers.
>
> Replace those occurences with proper i2c_board_info structs and call
> v4l2_i2c_new_subdev_board.
>
> After these changes v4l2_i2c_new_subdev_cfg() was no longer used, so remove
> that function as well.
>
> Signed-off-by: Hans Verkuil<hverkuil@xs4all.nl>
> ---
>   drivers/media/video/cafe_ccic.c            |   11 +++-
>   drivers/media/video/cx25840/cx25840-core.c |   22 ++------
>   drivers/media/video/em28xx/em28xx-cards.c  |   18 ++++---
>   drivers/media/video/ivtv/ivtv-i2c.c        |    9 +++-
>   drivers/media/video/mt9v011.c              |   29 ++++-------
>   drivers/media/video/mt9v011.h              |   36 -------------
>   drivers/media/video/mt9v011_regs.h         |   36 +++++++++++++
>   drivers/media/video/ov7670.c               |   74 ++++++++++++----------------
>   drivers/media/video/sr030pc30.c            |   10 ----
>   drivers/media/video/v4l2-common.c          |   19 +------
>   include/media/mt9v011.h                    |   17 ++++++
>   include/media/v4l2-common.h                |   13 +-----
>   include/media/v4l2-subdev.h                |    6 +--
>   13 files changed, 130 insertions(+), 170 deletions(-)
>   delete mode 100644 drivers/media/video/mt9v011.h
>   create mode 100644 drivers/media/video/mt9v011_regs.h
>   create mode 100644 include/media/mt9v011.h
>
...
>
> diff --git a/drivers/media/video/sr030pc30.c b/drivers/media/video/sr030pc30.c
> index 864696b..c901721 100644
> --- a/drivers/media/video/sr030pc30.c
> +++ b/drivers/media/video/sr030pc30.c
> @@ -714,15 +714,6 @@ static int sr030pc30_base_config(struct v4l2_subdev *sd)
>   	return ret;
>   }
>
> -static int sr030pc30_s_config(struct v4l2_subdev *sd,
> -			      int irq, void *platform_data)
> -{
> -	struct sr030pc30_info *info = to_sr030pc30(sd);
> -
> -	info->pdata = platform_data;
> -	return 0;
> -}
> -
>   static int sr030pc30_s_stream(struct v4l2_subdev *sd, int enable)
>   {
>   	return 0;
> @@ -763,7 +754,6 @@ static int sr030pc30_s_power(struct v4l2_subdev *sd, int on)
>   }
>
>   static const struct v4l2_subdev_core_ops sr030pc30_core_ops = {
> -	.s_config	= sr030pc30_s_config,
>   	.s_power	= sr030pc30_s_power,
>   	.queryctrl	= sr030pc30_queryctrl,
>   	.s_ctrl		= sr030pc30_s_ctrl,
>

I've just had prepared a patch removing s_config as well as an empty 
s_stream op. So now there is only one left for me ;)
Thanks for handling that, and sorry for the trouble. I've got also
prepared a patch converting sr030pc30 driver to the control framework,
just need to find a time slot to test it.
An another one replacing the set_power callback with the regulator API.


Regards,
Sylwester