[PATCH 0/4] soc-camera: setting the buswidth of camera sensors

[PATCH 0/4] soc-camera: setting the buswidth of camera sensors

[Sascha Hauer]

Hi,

The following patches change the handling of the bus width
for camera sensors so that a board can overwrite a sensors
native bus width

Sascha

Sascha Hauer (4):
  soc-camera: add board hook to specify the buswidth for camera sensors
  pcm990 baseboard: add camera bus width switch setting
  mt9m001: allow setting of bus width from board code
  mt9v022: allow setting of bus width from board code

 arch/arm/mach-pxa/pcm990-baseboard.c |   50 +++++++++++---
 drivers/media/video/Kconfig          |   14 ----
 drivers/media/video/mt9m001.c        |  130 ++++++++--------------------------
 drivers/media/video/mt9v022.c        |   97 +++----------------------
 include/media/soc_camera.h           |    6 ++
 5 files changed, 88 insertions(+), 209 deletions(-)

[PATCH 1/4] soc-camera: add board hook to specify the buswidth for camera sensors

[Sascha Hauer]

Camera sensors have a native bus width say support, but on some
boards not all sensor data lines are connected to the image
interface and thus support a different bus width than the sensors
native one. Some boards even have a bus driver which dynamically
switches between different bus widths with a GPIO.

This patch adds a hook which board code can use to support different
bus widths.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 include/media/soc_camera.h |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/include/media/soc_camera.h b/include/media/soc_camera.h
index 7440d92..d68959c 100644
--- a/include/media/soc_camera.h
+++ b/include/media/soc_camera.h
@@ -100,6 +100,12 @@ struct soc_camera_link {
 	/* Optional callbacks to power on or off and reset the sensor */
 	int (*power)(struct device *, int);
 	int (*reset)(struct device *);
+	/* some platforms may support different data widths than the sensors
+	 * native ones due to different data line routing. Let the board code
+	 * overwrite the width flags.
+	 */
+	int (*set_bus_param)(struct device *, unsigned long flags);
+	unsigned long (*query_bus_param)(struct device *);
 };
 
 static inline struct soc_camera_device *to_soc_camera_dev(struct device *dev)
-- 
1.5.6.5

[PATCH 2/4] pcm990 baseboard: add camera bus width switch setting

[Sascha Hauer]

Some Phytec cameras have a I2C GPIO expander which allows it to
switch between different sensor bus widths. This was previously
handled in the camera driver. Since handling of this switch
varies on several boards the cameras are used on, the board
support seems a better place to handle the switch

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 arch/arm/mach-pxa/pcm990-baseboard.c |   50 +++++++++++++++++++++++++++------
 1 files changed, 41 insertions(+), 9 deletions(-)

diff --git a/arch/arm/mach-pxa/pcm990-baseboard.c b/arch/arm/mach-pxa/pcm990-baseboard.c
index 34841c7..e9feb89 100644
--- a/arch/arm/mach-pxa/pcm990-baseboard.c
+++ b/arch/arm/mach-pxa/pcm990-baseboard.c
@@ -381,14 +381,46 @@ static struct pca953x_platform_data pca9536_data = {
 	.gpio_base	= NR_BUILTIN_GPIO + 1,
 };
 
-static struct soc_camera_link iclink[] = {
-	{
-		.bus_id	= 0, /* Must match with the camera ID above */
-		.gpio	= NR_BUILTIN_GPIO + 1,
-	}, {
-		.bus_id	= 0, /* Must match with the camera ID above */
-		.gpio	= -ENXIO,
+static int gpio_bus_switch;
+
+static int pcm990_camera_set_bus_param(struct device *dev,
+		unsigned long flags)
+{
+	if (gpio_bus_switch <= 0)
+		return 0;
+
+	if (flags & SOCAM_DATAWIDTH_8)
+		gpio_set_value(NR_BUILTIN_GPIO + 1, 1);
+	else
+		gpio_set_value(NR_BUILTIN_GPIO + 1, 0);
+
+	return 0;
+}
+
+static unsigned long pcm990_camera_query_bus_param(struct device *dev)
+{
+	int ret;
+
+	if (!gpio_bus_switch) {
+		ret = gpio_request(NR_BUILTIN_GPIO + 1, "camera");
+		if (!ret) {
+			gpio_bus_switch = NR_BUILTIN_GPIO + 1;
+			gpio_direction_output(gpio_bus_switch, 0);
+		} else
+			gpio_bus_switch = -1;
 	}
+
+	if (gpio_bus_switch > 0)
+		return SOCAM_DATAWIDTH_8 | SOCAM_DATAWIDTH_10;
+	else
+		return SOCAM_DATAWIDTH_10;
+}
+
+static struct soc_camera_link iclink = {
+	.bus_id	= 0, /* Must match with the camera ID above */
+	.query_bus_param = pcm990_camera_query_bus_param,
+	.set_bus_param = pcm990_camera_set_bus_param,
+	.gpio	= NR_BUILTIN_GPIO + 1,
 };
 
 /* Board I2C devices. */
@@ -399,10 +431,10 @@ static struct i2c_board_info __initdata pcm990_i2c_devices[] = {
 		.platform_data = &pca9536_data,
 	}, {
 		I2C_BOARD_INFO("mt9v022", 0x48),
-		.platform_data = &iclink[0], /* With extender */
+		.platform_data = &iclink, /* With extender */
 	}, {
 		I2C_BOARD_INFO("mt9m001", 0x5d),
-		.platform_data = &iclink[0], /* With extender */
+		.platform_data = &iclink, /* With extender */
 	},
 };
 #endif /* CONFIG_VIDEO_PXA27x ||CONFIG_VIDEO_PXA27x_MODULE */
-- 
1.5.6.5

[PATCH 3/4] mt9m001: allow setting of bus width from board code

[Sascha Hauer]

This patch removes the phytec specific setting of the bus width
and switches to the more generic query_bus_param/set_bus_param
hooks

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/media/video/Kconfig   |    7 --
 drivers/media/video/mt9m001.c |  130 +++++++++-------------------------------
 2 files changed, 30 insertions(+), 107 deletions(-)

diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig
index 19cf3b8..5fc1531 100644
--- a/drivers/media/video/Kconfig
+++ b/drivers/media/video/Kconfig
@@ -728,13 +728,6 @@ config SOC_CAMERA_MT9M001
 	  This driver supports MT9M001 cameras from Micron, monochrome
 	  and colour models.
 
-config MT9M001_PCA9536_SWITCH
-	bool "pca9536 datawidth switch for mt9m001"
-	depends on SOC_CAMERA_MT9M001 && GENERIC_GPIO
-	help
-	  Select this if your MT9M001 camera uses a PCA9536 I2C GPIO
-	  extender to switch between 8 and 10 bit datawidth modes
-
 config SOC_CAMERA_MT9M111
 	tristate "mt9m111 and mt9m112 support"
 	depends on SOC_CAMERA && I2C
diff --git a/drivers/media/video/mt9m001.c b/drivers/media/video/mt9m001.c
index c1bf75e..112c612 100644
--- a/drivers/media/video/mt9m001.c
+++ b/drivers/media/video/mt9m001.c
@@ -75,7 +75,6 @@ struct mt9m001 {
 	int model;	/* V4L2_IDENT_MT9M001* codes from v4l2-chip-ident.h */
 	int switch_gpio;
 	unsigned char autoexposure;
-	unsigned char datawidth;
 };
 
 static int reg_read(struct soc_camera_device *icd, const u8 reg)
@@ -181,90 +180,17 @@ static int mt9m001_stop_capture(struct soc_camera_device *icd)
 	return 0;
 }
 
-static int bus_switch_request(struct mt9m001 *mt9m001,
-			      struct soc_camera_link *icl)
-{
-#ifdef CONFIG_MT9M001_PCA9536_SWITCH
-	int ret;
-	unsigned int gpio = icl->gpio;
-
-	if (gpio_is_valid(gpio)) {
-		/* We have a data bus switch. */
-		ret = gpio_request(gpio, "mt9m001");
-		if (ret < 0) {
-			dev_err(&mt9m001->client->dev, "Cannot get GPIO %u\n",
-				gpio);
-			return ret;
-		}
-
-		ret = gpio_direction_output(gpio, 0);
-		if (ret < 0) {
-			dev_err(&mt9m001->client->dev,
-				"Cannot set GPIO %u to output\n", gpio);
-			gpio_free(gpio);
-			return ret;
-		}
-	}
-
-	mt9m001->switch_gpio = gpio;
-#else
-	mt9m001->switch_gpio = -EINVAL;
-#endif
-	return 0;
-}
-
-static void bus_switch_release(struct mt9m001 *mt9m001)
-{
-#ifdef CONFIG_MT9M001_PCA9536_SWITCH
-	if (gpio_is_valid(mt9m001->switch_gpio))
-		gpio_free(mt9m001->switch_gpio);
-#endif
-}
-
-static int bus_switch_act(struct mt9m001 *mt9m001, int go8bit)
-{
-#ifdef CONFIG_MT9M001_PCA9536_SWITCH
-	if (!gpio_is_valid(mt9m001->switch_gpio))
-		return -ENODEV;
-
-	gpio_set_value_cansleep(mt9m001->switch_gpio, go8bit);
-	return 0;
-#else
-	return -ENODEV;
-#endif
-}
-
-static int bus_switch_possible(struct mt9m001 *mt9m001)
-{
-#ifdef CONFIG_MT9M001_PCA9536_SWITCH
-	return gpio_is_valid(mt9m001->switch_gpio);
-#else
-	return 0;
-#endif
-}
-
 static int mt9m001_set_bus_param(struct soc_camera_device *icd,
 				 unsigned long flags)
 {
 	struct mt9m001 *mt9m001 = container_of(icd, struct mt9m001, icd);
-	unsigned int width_flag = flags & SOCAM_DATAWIDTH_MASK;
-	int ret;
+	struct soc_camera_link *icl = mt9m001->client->dev.platform_data;
 
 	/* Flags validity verified in test_bus_param */
 
-	if ((mt9m001->datawidth != 10 && (width_flag == SOCAM_DATAWIDTH_10)) ||
-	    (mt9m001->datawidth != 9  && (width_flag == SOCAM_DATAWIDTH_9)) ||
-	    (mt9m001->datawidth != 8  && (width_flag == SOCAM_DATAWIDTH_8))) {
-		/* Well, we actually only can do 10 or 8 bits... */
-		if (width_flag == SOCAM_DATAWIDTH_9)
-			return -EINVAL;
-		ret = bus_switch_act(mt9m001,
-				     width_flag == SOCAM_DATAWIDTH_8);
-		if (ret < 0)
-			return ret;
-
-		mt9m001->datawidth = width_flag == SOCAM_DATAWIDTH_8 ? 8 : 10;
-	}
+	if (icl->set_bus_param)
+		return icl->set_bus_param(&mt9m001->client->dev,
+				flags & SOCAM_DATAWIDTH_MASK);
 
 	return 0;
 }
@@ -274,12 +200,15 @@ static unsigned long mt9m001_query_bus_param(struct soc_camera_device *icd)
 	struct mt9m001 *mt9m001 = container_of(icd, struct mt9m001, icd);
 	struct soc_camera_link *icl = mt9m001->client->dev.platform_data;
 	/* MT9M001 has all capture_format parameters fixed */
-	unsigned long flags = SOCAM_DATAWIDTH_10 | SOCAM_PCLK_SAMPLE_RISING |
+	unsigned long flags = SOCAM_PCLK_SAMPLE_RISING |
 		SOCAM_HSYNC_ACTIVE_HIGH | SOCAM_VSYNC_ACTIVE_HIGH |
 		SOCAM_MASTER;
 
-	if (bus_switch_possible(mt9m001))
-		flags |= SOCAM_DATAWIDTH_8;
+	if (icl->query_bus_param)
+		flags |= icl->query_bus_param(&mt9m001->client->dev) &
+			SOCAM_DATAWIDTH_MASK;
+	else
+		flags |= SOCAM_DATAWIDTH_10;
 
 	return soc_camera_apply_sensor_flags(icl, flags);
 }
@@ -583,6 +512,7 @@ static int mt9m001_video_probe(struct soc_camera_device *icd)
 	struct soc_camera_link *icl = mt9m001->client->dev.platform_data;
 	s32 data;
 	int ret;
+	unsigned long flags;
 
 	/* We must have a parent by now. And it cannot be a wrong one.
 	 * So this entire test is completely redundant. */
@@ -603,18 +533,10 @@ static int mt9m001_video_probe(struct soc_camera_device *icd)
 	case 0x8421:
 		mt9m001->model = V4L2_IDENT_MT9M001C12ST;
 		icd->formats = mt9m001_colour_formats;
-		if (gpio_is_valid(icl->gpio))
-			icd->num_formats = ARRAY_SIZE(mt9m001_colour_formats);
-		else
-			icd->num_formats = 1;
 		break;
 	case 0x8431:
 		mt9m001->model = V4L2_IDENT_MT9M001C12STM;
 		icd->formats = mt9m001_monochrome_formats;
-		if (gpio_is_valid(icl->gpio))
-			icd->num_formats = ARRAY_SIZE(mt9m001_monochrome_formats);
-		else
-			icd->num_formats = 1;
 		break;
 	default:
 		ret = -ENODEV;
@@ -623,6 +545,25 @@ static int mt9m001_video_probe(struct soc_camera_device *icd)
 		goto ei2c;
 	}
 
+	icd->num_formats = 0;
+
+	/* This is a 10bit sensor, so by default we only allow 10bit.
+	 * The platform may support different bus widths due to
+	 * different routing of the data lines.
+	 */
+	if (icl->query_bus_param)
+		flags = icl->query_bus_param(&mt9m001->client->dev);
+	else
+		flags = SOCAM_DATAWIDTH_10;
+
+	if (flags & SOCAM_DATAWIDTH_10)
+		icd->num_formats++;
+	else
+		icd->formats++;
+
+	if (flags & SOCAM_DATAWIDTH_8)
+		icd->num_formats++;
+
 	dev_info(&icd->dev, "Detected a MT9M001 chip ID %x (%s)\n", data,
 		 data == 0x8431 ? "C12STM" : "C12ST");
 
@@ -688,18 +629,10 @@ static int mt9m001_probe(struct i2c_client *client,
 	icd->height_max	= 1024;
 	icd->y_skip_top	= 1;
 	icd->iface	= icl->bus_id;
-	/* Default datawidth - this is the only width this camera (normally)
-	 * supports. It is only with extra logic that it can support
-	 * other widths. Therefore it seems to be a sensible default. */
-	mt9m001->datawidth = 10;
 	/* Simulated autoexposure. If enabled, we calculate shutter width
 	 * ourselves in the driver based on vertical blanking and frame width */
 	mt9m001->autoexposure = 1;
 
-	ret = bus_switch_request(mt9m001, icl);
-	if (ret)
-		goto eswinit;
-
 	ret = soc_camera_device_register(icd);
 	if (ret)
 		goto eisdr;
@@ -707,8 +640,6 @@ static int mt9m001_probe(struct i2c_client *client,
 	return 0;
 
 eisdr:
-	bus_switch_release(mt9m001);
-eswinit:
 	kfree(mt9m001);
 	return ret;
 }
@@ -718,7 +649,6 @@ static int mt9m001_remove(struct i2c_client *client)
 	struct mt9m001 *mt9m001 = i2c_get_clientdata(client);
 
 	soc_camera_device_unregister(&mt9m001->icd);
-	bus_switch_release(mt9m001);
 	kfree(mt9m001);
 
 	return 0;
-- 
1.5.6.5

[PATCH 4/4] mt9v022: allow setting of bus width from board code

[Sascha Hauer]

This patch removes the phytec specific setting of the bus width
and switches to the more generic query_bus_param/set_bus_param
hooks

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/media/video/Kconfig   |    7 ---
 drivers/media/video/mt9v022.c |   97 +++++------------------------------------
 2 files changed, 11 insertions(+), 93 deletions(-)

diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig
index 5fc1531..071d66f 100644
--- a/drivers/media/video/Kconfig
+++ b/drivers/media/video/Kconfig
@@ -747,13 +747,6 @@ config SOC_CAMERA_MT9V022
 	help
 	  This driver supports MT9V022 cameras from Micron
 
-config MT9V022_PCA9536_SWITCH
-	bool "pca9536 datawidth switch for mt9v022"
-	depends on SOC_CAMERA_MT9V022 && GENERIC_GPIO
-	help
-	  Select this if your MT9V022 camera uses a PCA9536 I2C GPIO
-	  extender to switch between 8 and 10 bit datawidth modes
-
 config SOC_CAMERA_TW9910
 	tristate "tw9910 support"
 	depends on SOC_CAMERA && I2C
diff --git a/drivers/media/video/mt9v022.c b/drivers/media/video/mt9v022.c
index b04c8cb..26f97eb 100644
--- a/drivers/media/video/mt9v022.c
+++ b/drivers/media/video/mt9v022.c
@@ -209,66 +209,6 @@ static int mt9v022_stop_capture(struct soc_camera_device *icd)
 	return 0;
 }
 
-static int bus_switch_request(struct mt9v022 *mt9v022, struct soc_camera_link *icl)
-{
-#ifdef CONFIG_MT9V022_PCA9536_SWITCH
-	int ret;
-	unsigned int gpio = icl->gpio;
-
-	if (gpio_is_valid(gpio)) {
-		/* We have a data bus switch. */
-		ret = gpio_request(gpio, "mt9v022");
-		if (ret < 0) {
-			dev_err(&mt9v022->client->dev, "Cannot get GPIO %u\n", gpio);
-			return ret;
-		}
-
-		ret = gpio_direction_output(gpio, 0);
-		if (ret < 0) {
-			dev_err(&mt9v022->client->dev,
-				"Cannot set GPIO %u to output\n", gpio);
-			gpio_free(gpio);
-			return ret;
-		}
-	}
-
-	mt9v022->switch_gpio = gpio;
-#else
-	mt9v022->switch_gpio = -EINVAL;
-#endif
-	return 0;
-}
-
-static void bus_switch_release(struct mt9v022 *mt9v022)
-{
-#ifdef CONFIG_MT9V022_PCA9536_SWITCH
-	if (gpio_is_valid(mt9v022->switch_gpio))
-		gpio_free(mt9v022->switch_gpio);
-#endif
-}
-
-static int bus_switch_act(struct mt9v022 *mt9v022, int go8bit)
-{
-#ifdef CONFIG_MT9V022_PCA9536_SWITCH
-	if (!gpio_is_valid(mt9v022->switch_gpio))
-		return -ENODEV;
-
-	gpio_set_value_cansleep(mt9v022->switch_gpio, go8bit);
-	return 0;
-#else
-	return -ENODEV;
-#endif
-}
-
-static int bus_switch_possible(struct mt9v022 *mt9v022)
-{
-#ifdef CONFIG_MT9V022_PCA9536_SWITCH
-	return gpio_is_valid(mt9v022->switch_gpio);
-#else
-	return 0;
-#endif
-}
-
 static int mt9v022_set_bus_param(struct soc_camera_device *icd,
 				 unsigned long flags)
 {
@@ -282,19 +222,10 @@ static int mt9v022_set_bus_param(struct soc_camera_device *icd,
 	if (!is_power_of_2(width_flag))
 		return -EINVAL;
 
-	if ((mt9v022->datawidth != 10 && (width_flag == SOCAM_DATAWIDTH_10)) ||
-	    (mt9v022->datawidth != 9  && (width_flag == SOCAM_DATAWIDTH_9)) ||
-	    (mt9v022->datawidth != 8  && (width_flag == SOCAM_DATAWIDTH_8))) {
-		/* Well, we actually only can do 10 or 8 bits... */
-		if (width_flag == SOCAM_DATAWIDTH_9)
-			return -EINVAL;
-
-		ret = bus_switch_act(mt9v022,
-				     width_flag == SOCAM_DATAWIDTH_8);
-		if (ret < 0)
+	if (icl->set_bus_param) {
+		ret = icl->set_bus_param(&mt9v022->client->dev, width_flag);
+		if (ret)
 			return ret;
-
-		mt9v022->datawidth = width_flag == SOCAM_DATAWIDTH_8 ? 8 : 10;
 	}
 
 	flags = soc_camera_apply_sensor_flags(icl, flags);
@@ -328,10 +259,14 @@ static int mt9v022_set_bus_param(struct soc_camera_device *icd,
 static unsigned long mt9v022_query_bus_param(struct soc_camera_device *icd)
 {
 	struct mt9v022 *mt9v022 = container_of(icd, struct mt9v022, icd);
-	unsigned int width_flag = SOCAM_DATAWIDTH_10;
+	struct soc_camera_link *icl = mt9v022->client->dev.platform_data;
+	unsigned int width_flag;
 
-	if (bus_switch_possible(mt9v022))
-		width_flag |= SOCAM_DATAWIDTH_8;
+	if (icl->query_bus_param)
+		width_flag = icl->query_bus_param(&mt9v022->client->dev) &
+			SOCAM_DATAWIDTH_MASK;
+	else
+		width_flag = SOCAM_DATAWIDTH_10;
 
 	return SOCAM_PCLK_SAMPLE_RISING | SOCAM_PCLK_SAMPLE_FALLING |
 		SOCAM_HSYNC_ACTIVE_HIGH | SOCAM_HSYNC_ACTIVE_LOW |
@@ -729,6 +664,7 @@ static int mt9v022_video_probe(struct soc_camera_device *icd)
 	/* Set monochrome or colour sensor type */
 	if (sensor_type && (!strcmp("colour", sensor_type) ||
 			    !strcmp("color", sensor_type))) {
+	if (1) {
 		ret = reg_write(icd, MT9V022_PIXEL_OPERATION_MODE, 4 | 0x11);
 		mt9v022->model = V4L2_IDENT_MT9V022IX7ATC;
 		icd->formats = mt9v022_colour_formats;
@@ -812,14 +748,6 @@ static int mt9v022_probe(struct i2c_client *client,
 	icd->height_max	= 480;
 	icd->y_skip_top	= 1;
 	icd->iface	= icl->bus_id;
-	/* Default datawidth - this is the only width this camera (normally)
-	 * supports. It is only with extra logic that it can support
-	 * other widths. Therefore it seems to be a sensible default. */
-	mt9v022->datawidth = 10;
-
-	ret = bus_switch_request(mt9v022, icl);
-	if (ret)
-		goto eswinit;
 
 	ret = soc_camera_device_register(icd);
 	if (ret)
@@ -828,8 +756,6 @@ static int mt9v022_probe(struct i2c_client *client,
 	return 0;
 
 eisdr:
-	bus_switch_release(mt9v022);
-eswinit:
 	kfree(mt9v022);
 	return ret;
 }
@@ -839,7 +765,6 @@ static int mt9v022_remove(struct i2c_client *client)
 	struct mt9v022 *mt9v022 = i2c_get_clientdata(client);
 
 	soc_camera_device_unregister(&mt9v022->icd);
-	bus_switch_release(mt9v022);
 	kfree(mt9v022);
 
 	return 0;
-- 
1.5.6.5

Re: [PATCH 1/4] soc-camera: add board hook to specify the buswidth for camera sensors

[Guennadi Liakhovetski]

On Wed, 11 Mar 2009, Sascha Hauer wrote:

> Camera sensors have a native bus width say support, but on some
> boards not all sensor data lines are connected to the image
> interface and thus support a different bus width than the sensors
> native one. Some boards even have a bus driver which dynamically
> switches between different bus widths with a GPIO.
> 
> This patch adds a hook which board code can use to support different
> bus widths.
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>  include/media/soc_camera.h |    6 ++++++
>  1 files changed, 6 insertions(+), 0 deletions(-)
> 
> diff --git a/include/media/soc_camera.h b/include/media/soc_camera.h
> index 7440d92..d68959c 100644
> --- a/include/media/soc_camera.h
> +++ b/include/media/soc_camera.h
> @@ -100,6 +100,12 @@ struct soc_camera_link {
>  	/* Optional callbacks to power on or off and reset the sensor */
>  	int (*power)(struct device *, int);
>  	int (*reset)(struct device *);
> +	/* some platforms may support different data widths than the sensors
> +	 * native ones due to different data line routing. Let the board code
> +	 * overwrite the width flags.
> +	 */

Please, format the comment according to CodingStyle:

	/*
	 * some
	 * comment
	 */

I know, I have some non-conforming (similar to yours) comments in 
soc-camera .c files, but this header is "clean" so far:-) Let's keep it 
that way.

> +	int (*set_bus_param)(struct device *, unsigned long flags);
> +	unsigned long (*query_bus_param)(struct device *);

Wouldn't the first parameter of type "struct soc_camera_link *" make more 
sense? I'll also then change .power and .reset similarly.

>  };
>  
>  static inline struct soc_camera_device *to_soc_camera_dev(struct device *dev)
> -- 
> 1.5.6.5
> 

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer

Re: [PATCH 2/4] pcm990 baseboard: add camera bus width switch setting

[Guennadi Liakhovetski]

On Wed, 11 Mar 2009, Sascha Hauer wrote:

> Some Phytec cameras have a I2C GPIO expander which allows it to
> switch between different sensor bus widths. This was previously
> handled in the camera driver. Since handling of this switch
> varies on several boards the cameras are used on, the board
> support seems a better place to handle the switch
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>  arch/arm/mach-pxa/pcm990-baseboard.c |   50 +++++++++++++++++++++++++++------
>  1 files changed, 41 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/arm/mach-pxa/pcm990-baseboard.c b/arch/arm/mach-pxa/pcm990-baseboard.c
> index 34841c7..e9feb89 100644
> --- a/arch/arm/mach-pxa/pcm990-baseboard.c
> +++ b/arch/arm/mach-pxa/pcm990-baseboard.c
> @@ -381,14 +381,46 @@ static struct pca953x_platform_data pca9536_data = {
>  	.gpio_base	= NR_BUILTIN_GPIO + 1,
>  };
>  
> -static struct soc_camera_link iclink[] = {
> -	{
> -		.bus_id	= 0, /* Must match with the camera ID above */
> -		.gpio	= NR_BUILTIN_GPIO + 1,
> -	}, {
> -		.bus_id	= 0, /* Must match with the camera ID above */
> -		.gpio	= -ENXIO,
> +static int gpio_bus_switch;
> +
> +static int pcm990_camera_set_bus_param(struct device *dev,

The prototype will change to use "struct soc_camera_link *"

> +		unsigned long flags)
> +{
> +	if (gpio_bus_switch <= 0)
> +		return 0;
> +
> +	if (flags & SOCAM_DATAWIDTH_8)
> +		gpio_set_value(NR_BUILTIN_GPIO + 1, 1);
> +	else
> +		gpio_set_value(NR_BUILTIN_GPIO + 1, 0);

You wanted to use gpio_bus_switch for these.

> +
> +	return 0;
> +}
> +
> +static unsigned long pcm990_camera_query_bus_param(struct device *dev)
> +{
> +	int ret;
> +
> +	if (!gpio_bus_switch) {
> +		ret = gpio_request(NR_BUILTIN_GPIO + 1, "camera");
> +		if (!ret) {
> +			gpio_bus_switch = NR_BUILTIN_GPIO + 1;
> +			gpio_direction_output(gpio_bus_switch, 0);
> +		} else
> +			gpio_bus_switch = -1;

This is a purely internal variable, so, I won't insist if you disagree, 
but, I think, a scheme "non-negative for a valid value or a negative error 
code" looks better, cf.

If you want to initialize a structure with an invalid GPIO number, use
some negative number (perhaps "-EINVAL"); that will never be valid.

(Documentation/gpio.txt). "-1" looks like you're going to perform 
calculations with it.

>  	}
> +
> +	if (gpio_bus_switch > 0)
> +		return SOCAM_DATAWIDTH_8 | SOCAM_DATAWIDTH_10;
> +	else
> +		return SOCAM_DATAWIDTH_10;
> +}
> +
> +static struct soc_camera_link iclink = {
> +	.bus_id	= 0, /* Must match with the camera ID above */
> +	.query_bus_param = pcm990_camera_query_bus_param,
> +	.set_bus_param = pcm990_camera_set_bus_param,
> +	.gpio	= NR_BUILTIN_GPIO + 1,

There's one patch missing in your patch series:

[PATCH 5/5] Remove the "gpio" member from the struct soc_camera_link

>  };
>  
>  /* Board I2C devices. */
> @@ -399,10 +431,10 @@ static struct i2c_board_info __initdata pcm990_i2c_devices[] = {
>  		.platform_data = &pca9536_data,
>  	}, {
>  		I2C_BOARD_INFO("mt9v022", 0x48),
> -		.platform_data = &iclink[0], /* With extender */
> +		.platform_data = &iclink, /* With extender */
>  	}, {
>  		I2C_BOARD_INFO("mt9m001", 0x5d),
> -		.platform_data = &iclink[0], /* With extender */
> +		.platform_data = &iclink, /* With extender */
>  	},
>  };
>  #endif /* CONFIG_VIDEO_PXA27x ||CONFIG_VIDEO_PXA27x_MODULE */
> -- 
> 1.5.6.5
> 

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer

Re: [PATCH 2/4] pcm990 baseboard: add camera bus width switch setting

[Sascha Hauer]

On Thu, Mar 12, 2009 at 09:31:55AM +0100, Guennadi Liakhovetski wrote:
> On Wed, 11 Mar 2009, Sascha Hauer wrote:
> 
> > Some Phytec cameras have a I2C GPIO expander which allows it to
> > switch between different sensor bus widths. This was previously
> > handled in the camera driver. Since handling of this switch
> > varies on several boards the cameras are used on, the board
> > support seems a better place to handle the switch
> > 
> > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > ---
> >  arch/arm/mach-pxa/pcm990-baseboard.c |   50 +++++++++++++++++++++++++++------
> >  1 files changed, 41 insertions(+), 9 deletions(-)
> > 
> > diff --git a/arch/arm/mach-pxa/pcm990-baseboard.c b/arch/arm/mach-pxa/pcm990-baseboard.c
> > index 34841c7..e9feb89 100644
> > --- a/arch/arm/mach-pxa/pcm990-baseboard.c
> > +++ b/arch/arm/mach-pxa/pcm990-baseboard.c
> > @@ -381,14 +381,46 @@ static struct pca953x_platform_data pca9536_data = {
> >  	.gpio_base	= NR_BUILTIN_GPIO + 1,
> >  };
> >  
> > -static struct soc_camera_link iclink[] = {
> > -	{
> > -		.bus_id	= 0, /* Must match with the camera ID above */
> > -		.gpio	= NR_BUILTIN_GPIO + 1,
> > -	}, {
> > -		.bus_id	= 0, /* Must match with the camera ID above */
> > -		.gpio	= -ENXIO,
> > +static int gpio_bus_switch;
> > +
> > +static int pcm990_camera_set_bus_param(struct device *dev,
> 
> The prototype will change to use "struct soc_camera_link *"

OK

> 
> > +		unsigned long flags)
> > +{
> > +	if (gpio_bus_switch <= 0)
> > +		return 0;
> > +
> > +	if (flags & SOCAM_DATAWIDTH_8)
> > +		gpio_set_value(NR_BUILTIN_GPIO + 1, 1);
> > +	else
> > +		gpio_set_value(NR_BUILTIN_GPIO + 1, 0);
> 
> You wanted to use gpio_bus_switch for these.

s/wanted to/should/?

OK

> 
> > +
> > +	return 0;
> > +}
> > +
> > +static unsigned long pcm990_camera_query_bus_param(struct device *dev)
> > +{
> > +	int ret;
> > +
> > +	if (!gpio_bus_switch) {
> > +		ret = gpio_request(NR_BUILTIN_GPIO + 1, "camera");
> > +		if (!ret) {
> > +			gpio_bus_switch = NR_BUILTIN_GPIO + 1;
> > +			gpio_direction_output(gpio_bus_switch, 0);
> > +		} else
> > +			gpio_bus_switch = -1;
> 
> This is a purely internal variable, so, I won't insist if you disagree, 
> but, I think, a scheme "non-negative for a valid value or a negative error 
> code" looks better, cf.
> 
> If you want to initialize a structure with an invalid GPIO number, use
> some negative number (perhaps "-EINVAL"); that will never be valid.
> 
> (Documentation/gpio.txt). "-1" looks like you're going to perform 
> calculations with it.

OK

> 
> >  	}
> > +
> > +	if (gpio_bus_switch > 0)
> > +		return SOCAM_DATAWIDTH_8 | SOCAM_DATAWIDTH_10;
> > +	else
> > +		return SOCAM_DATAWIDTH_10;
> > +}
> > +
> > +static struct soc_camera_link iclink = {
> > +	.bus_id	= 0, /* Must match with the camera ID above */
> > +	.query_bus_param = pcm990_camera_query_bus_param,
> > +	.set_bus_param = pcm990_camera_set_bus_param,
> > +	.gpio	= NR_BUILTIN_GPIO + 1,
> 
> There's one patch missing in your patch series:
> 
> [PATCH 5/5] Remove the "gpio" member from the struct soc_camera_link

OK. I saw this member is unnecessary now and forgot it a minute later...

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 2/4] pcm990 baseboard: add camera bus width switch setting

[Guennadi Liakhovetski]

One more thing I noticed while looking at your patch 3/4:

> +static int pcm990_camera_set_bus_param(struct device *dev,
> +		unsigned long flags)
> +{
> +	if (gpio_bus_switch <= 0)
> +		return 0;
> +
> +	if (flags & SOCAM_DATAWIDTH_8)
> +		gpio_set_value(NR_BUILTIN_GPIO + 1, 1);
> +	else
> +		gpio_set_value(NR_BUILTIN_GPIO + 1, 0);

Originally the logic here was "only if flags == SOCAM_DATAWIDTH_8, switch 
to 8 bits, otherwise do 10 bits. I.e., if flags == SOCAM_DATAWIDTH_8 | 
SOCAM_DATAWIDTH_10, it would still do the default (and wider) 10 bits. Do 
you have any reason to change that logic?

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer

Re: [PATCH 2/4] pcm990 baseboard: add camera bus width switch setting

[Sascha Hauer]

On Thu, Mar 12, 2009 at 09:40:46AM +0100, Guennadi Liakhovetski wrote:
> One more thing I noticed while looking at your patch 3/4:
> 
> > +static int pcm990_camera_set_bus_param(struct device *dev,
> > +		unsigned long flags)
> > +{
> > +	if (gpio_bus_switch <= 0)
> > +		return 0;
> > +
> > +	if (flags & SOCAM_DATAWIDTH_8)
> > +		gpio_set_value(NR_BUILTIN_GPIO + 1, 1);
> > +	else
> > +		gpio_set_value(NR_BUILTIN_GPIO + 1, 0);
> 
> Originally the logic here was "only if flags == SOCAM_DATAWIDTH_8, switch 
> to 8 bits, otherwise do 10 bits. I.e., if flags == SOCAM_DATAWIDTH_8 | 
> SOCAM_DATAWIDTH_10, it would still do the default (and wider) 10 bits. Do 
> you have any reason to change that logic?

I was not aware that I changed any logic. I thought I would get here
with only one of the SOCAM_DATAWIDTH_* set. Isn't it a bug when we get
here with more than one width flags set?

The mt9v022 driver has this in set_bus_param:

>
>	/* Only one width bit may be set */
>	if (!is_power_of_2(width_flag))
>		return -EINVAL;
>

And I think it makes sense.

Sascha


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 3/4] mt9m001: allow setting of bus width from board code

[Guennadi Liakhovetski]

On Wed, 11 Mar 2009, Sascha Hauer wrote:

> This patch removes the phytec specific setting of the bus width
> and switches to the more generic query_bus_param/set_bus_param
> hooks
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>  drivers/media/video/Kconfig   |    7 --
>  drivers/media/video/mt9m001.c |  130 +++++++++-------------------------------
>  2 files changed, 30 insertions(+), 107 deletions(-)
> 
> diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig
> index 19cf3b8..5fc1531 100644
> --- a/drivers/media/video/Kconfig
> +++ b/drivers/media/video/Kconfig
> @@ -728,13 +728,6 @@ config SOC_CAMERA_MT9M001
>  	  This driver supports MT9M001 cameras from Micron, monochrome
>  	  and colour models.
>  
> -config MT9M001_PCA9536_SWITCH
> -	bool "pca9536 datawidth switch for mt9m001"
> -	depends on SOC_CAMERA_MT9M001 && GENERIC_GPIO
> -	help
> -	  Select this if your MT9M001 camera uses a PCA9536 I2C GPIO
> -	  extender to switch between 8 and 10 bit datawidth modes
> -
>  config SOC_CAMERA_MT9M111
>  	tristate "mt9m111 and mt9m112 support"
>  	depends on SOC_CAMERA && I2C
> diff --git a/drivers/media/video/mt9m001.c b/drivers/media/video/mt9m001.c
> index c1bf75e..112c612 100644
> --- a/drivers/media/video/mt9m001.c
> +++ b/drivers/media/video/mt9m001.c
> @@ -75,7 +75,6 @@ struct mt9m001 {
>  	int model;	/* V4L2_IDENT_MT9M001* codes from v4l2-chip-ident.h */
>  	int switch_gpio;
>  	unsigned char autoexposure;
> -	unsigned char datawidth;
>  };
>  
>  static int reg_read(struct soc_camera_device *icd, const u8 reg)
> @@ -181,90 +180,17 @@ static int mt9m001_stop_capture(struct soc_camera_device *icd)
>  	return 0;
>  }
>  
> -static int bus_switch_request(struct mt9m001 *mt9m001,
> -			      struct soc_camera_link *icl)
> -{
> -#ifdef CONFIG_MT9M001_PCA9536_SWITCH
> -	int ret;
> -	unsigned int gpio = icl->gpio;
> -
> -	if (gpio_is_valid(gpio)) {
> -		/* We have a data bus switch. */
> -		ret = gpio_request(gpio, "mt9m001");
> -		if (ret < 0) {
> -			dev_err(&mt9m001->client->dev, "Cannot get GPIO %u\n",
> -				gpio);
> -			return ret;
> -		}
> -
> -		ret = gpio_direction_output(gpio, 0);
> -		if (ret < 0) {
> -			dev_err(&mt9m001->client->dev,
> -				"Cannot set GPIO %u to output\n", gpio);
> -			gpio_free(gpio);
> -			return ret;
> -		}
> -	}
> -
> -	mt9m001->switch_gpio = gpio;
> -#else
> -	mt9m001->switch_gpio = -EINVAL;
> -#endif
> -	return 0;
> -}
> -
> -static void bus_switch_release(struct mt9m001 *mt9m001)
> -{
> -#ifdef CONFIG_MT9M001_PCA9536_SWITCH
> -	if (gpio_is_valid(mt9m001->switch_gpio))
> -		gpio_free(mt9m001->switch_gpio);
> -#endif
> -}
> -
> -static int bus_switch_act(struct mt9m001 *mt9m001, int go8bit)
> -{
> -#ifdef CONFIG_MT9M001_PCA9536_SWITCH
> -	if (!gpio_is_valid(mt9m001->switch_gpio))
> -		return -ENODEV;
> -
> -	gpio_set_value_cansleep(mt9m001->switch_gpio, go8bit);
> -	return 0;
> -#else
> -	return -ENODEV;
> -#endif
> -}
> -
> -static int bus_switch_possible(struct mt9m001 *mt9m001)
> -{
> -#ifdef CONFIG_MT9M001_PCA9536_SWITCH
> -	return gpio_is_valid(mt9m001->switch_gpio);
> -#else
> -	return 0;
> -#endif
> -}
> -
>  static int mt9m001_set_bus_param(struct soc_camera_device *icd,
>  				 unsigned long flags)
>  {
>  	struct mt9m001 *mt9m001 = container_of(icd, struct mt9m001, icd);
> -	unsigned int width_flag = flags & SOCAM_DATAWIDTH_MASK;
> -	int ret;
> +	struct soc_camera_link *icl = mt9m001->client->dev.platform_data;
>  
>  	/* Flags validity verified in test_bus_param */
>  
> -	if ((mt9m001->datawidth != 10 && (width_flag == SOCAM_DATAWIDTH_10)) ||
> -	    (mt9m001->datawidth != 9  && (width_flag == SOCAM_DATAWIDTH_9)) ||
> -	    (mt9m001->datawidth != 8  && (width_flag == SOCAM_DATAWIDTH_8))) {
> -		/* Well, we actually only can do 10 or 8 bits... */
> -		if (width_flag == SOCAM_DATAWIDTH_9)
> -			return -EINVAL;
> -		ret = bus_switch_act(mt9m001,
> -				     width_flag == SOCAM_DATAWIDTH_8);
> -		if (ret < 0)
> -			return ret;
> -
> -		mt9m001->datawidth = width_flag == SOCAM_DATAWIDTH_8 ? 8 : 10;
> -	}
> +	if (icl->set_bus_param)
> +		return icl->set_bus_param(&mt9m001->client->dev,
> +				flags & SOCAM_DATAWIDTH_MASK);

Not quite. Look at the original code. If no change is requested - just 
return 0. If a change is requested, but switching is impossible - return 
an error - and this is not, what you are doing in 2/4, please fix. So, you 
might still want to preserve ".datawidth" for the verification.

Calls to camera-device ->query_bus_param() and ->set_bus_param() methods 
are currently internal to specific host drivers. So, it is better to be 
prepared to handle invalid or unexpected parameters.

>  
>  	return 0;
>  }
> @@ -274,12 +200,15 @@ static unsigned long mt9m001_query_bus_param(struct soc_camera_device *icd)
>  	struct mt9m001 *mt9m001 = container_of(icd, struct mt9m001, icd);
>  	struct soc_camera_link *icl = mt9m001->client->dev.platform_data;
>  	/* MT9M001 has all capture_format parameters fixed */
> -	unsigned long flags = SOCAM_DATAWIDTH_10 | SOCAM_PCLK_SAMPLE_RISING |
> +	unsigned long flags = SOCAM_PCLK_SAMPLE_RISING |
>  		SOCAM_HSYNC_ACTIVE_HIGH | SOCAM_VSYNC_ACTIVE_HIGH |
>  		SOCAM_MASTER;
>  
> -	if (bus_switch_possible(mt9m001))
> -		flags |= SOCAM_DATAWIDTH_8;
> +	if (icl->query_bus_param)
> +		flags |= icl->query_bus_param(&mt9m001->client->dev) &
> +			SOCAM_DATAWIDTH_MASK;
> +	else
> +		flags |= SOCAM_DATAWIDTH_10;
>  
>  	return soc_camera_apply_sensor_flags(icl, flags);
>  }
> @@ -583,6 +512,7 @@ static int mt9m001_video_probe(struct soc_camera_device *icd)
>  	struct soc_camera_link *icl = mt9m001->client->dev.platform_data;
>  	s32 data;
>  	int ret;
> +	unsigned long flags;
>  
>  	/* We must have a parent by now. And it cannot be a wrong one.
>  	 * So this entire test is completely redundant. */
> @@ -603,18 +533,10 @@ static int mt9m001_video_probe(struct soc_camera_device *icd)
>  	case 0x8421:
>  		mt9m001->model = V4L2_IDENT_MT9M001C12ST;
>  		icd->formats = mt9m001_colour_formats;
> -		if (gpio_is_valid(icl->gpio))
> -			icd->num_formats = ARRAY_SIZE(mt9m001_colour_formats);
> -		else
> -			icd->num_formats = 1;
>  		break;
>  	case 0x8431:
>  		mt9m001->model = V4L2_IDENT_MT9M001C12STM;
>  		icd->formats = mt9m001_monochrome_formats;
> -		if (gpio_is_valid(icl->gpio))
> -			icd->num_formats = ARRAY_SIZE(mt9m001_monochrome_formats);
> -		else
> -			icd->num_formats = 1;
>  		break;
>  	default:
>  		ret = -ENODEV;
> @@ -623,6 +545,25 @@ static int mt9m001_video_probe(struct soc_camera_device *icd)
>  		goto ei2c;
>  	}
>  
> +	icd->num_formats = 0;
> +
> +	/* This is a 10bit sensor, so by default we only allow 10bit.
> +	 * The platform may support different bus widths due to
> +	 * different routing of the data lines.
> +	 */
> +	if (icl->query_bus_param)
> +		flags = icl->query_bus_param(&mt9m001->client->dev);
> +	else
> +		flags = SOCAM_DATAWIDTH_10;
> +
> +	if (flags & SOCAM_DATAWIDTH_10)
> +		icd->num_formats++;
> +	else
> +		icd->formats++;
> +
> +	if (flags & SOCAM_DATAWIDTH_8)
> +		icd->num_formats++;
> +
>  	dev_info(&icd->dev, "Detected a MT9M001 chip ID %x (%s)\n", data,
>  		 data == 0x8431 ? "C12STM" : "C12ST");
>  
> @@ -688,18 +629,10 @@ static int mt9m001_probe(struct i2c_client *client,
>  	icd->height_max	= 1024;
>  	icd->y_skip_top	= 1;
>  	icd->iface	= icl->bus_id;
> -	/* Default datawidth - this is the only width this camera (normally)
> -	 * supports. It is only with extra logic that it can support
> -	 * other widths. Therefore it seems to be a sensible default. */
> -	mt9m001->datawidth = 10;
>  	/* Simulated autoexposure. If enabled, we calculate shutter width
>  	 * ourselves in the driver based on vertical blanking and frame width */
>  	mt9m001->autoexposure = 1;
>  
> -	ret = bus_switch_request(mt9m001, icl);
> -	if (ret)
> -		goto eswinit;
> -
>  	ret = soc_camera_device_register(icd);
>  	if (ret)
>  		goto eisdr;
> @@ -707,8 +640,6 @@ static int mt9m001_probe(struct i2c_client *client,
>  	return 0;
>  
>  eisdr:
> -	bus_switch_release(mt9m001);
> -eswinit:
>  	kfree(mt9m001);
>  	return ret;
>  }
> @@ -718,7 +649,6 @@ static int mt9m001_remove(struct i2c_client *client)
>  	struct mt9m001 *mt9m001 = i2c_get_clientdata(client);
>  
>  	soc_camera_device_unregister(&mt9m001->icd);
> -	bus_switch_release(mt9m001);
>  	kfree(mt9m001);
>  
>  	return 0;
> -- 
> 1.5.6.5
> 

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer

Re: [PATCH 2/4] pcm990 baseboard: add camera bus width switch setting

[Guennadi Liakhovetski]

On Thu, 12 Mar 2009, Sascha Hauer wrote:

> On Thu, Mar 12, 2009 at 09:40:46AM +0100, Guennadi Liakhovetski wrote:
> > One more thing I noticed while looking at your patch 3/4:
> > 
> > > +static int pcm990_camera_set_bus_param(struct device *dev,
> > > +		unsigned long flags)
> > > +{
> > > +	if (gpio_bus_switch <= 0)
> > > +		return 0;
> > > +
> > > +	if (flags & SOCAM_DATAWIDTH_8)
> > > +		gpio_set_value(NR_BUILTIN_GPIO + 1, 1);
> > > +	else
> > > +		gpio_set_value(NR_BUILTIN_GPIO + 1, 0);
> > 
> > Originally the logic here was "only if flags == SOCAM_DATAWIDTH_8, switch 
> > to 8 bits, otherwise do 10 bits. I.e., if flags == SOCAM_DATAWIDTH_8 | 
> > SOCAM_DATAWIDTH_10, it would still do the default (and wider) 10 bits. Do 
> > you have any reason to change that logic?
> 
> I was not aware that I changed any logic. I thought I would get here
> with only one of the SOCAM_DATAWIDTH_* set. Isn't it a bug when we get
> here with more than one width flags set?
> 
> The mt9v022 driver has this in set_bus_param:
> 
> >
> >	/* Only one width bit may be set */
> >	if (!is_power_of_2(width_flag))
> >		return -EINVAL;
> >
> 
> And I think it makes sense.

Ok, then, could you, please add the same test to mt9m001? And, as I 
mentioned in a comment to 3/4, please, return an error if switching is 
requested but unsupported.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer

Re: [PATCH 4/4] mt9v022: allow setting of bus width from board code

[Guennadi Liakhovetski]

On Wed, 11 Mar 2009, Sascha Hauer wrote:

> This patch removes the phytec specific setting of the bus width
> and switches to the more generic query_bus_param/set_bus_param
> hooks
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>  drivers/media/video/Kconfig   |    7 ---
>  drivers/media/video/mt9v022.c |   97 +++++------------------------------------
>  2 files changed, 11 insertions(+), 93 deletions(-)
> 
> diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig
> index 5fc1531..071d66f 100644
> --- a/drivers/media/video/Kconfig
> +++ b/drivers/media/video/Kconfig
> @@ -747,13 +747,6 @@ config SOC_CAMERA_MT9V022
>  	help
>  	  This driver supports MT9V022 cameras from Micron
>  
> -config MT9V022_PCA9536_SWITCH
> -	bool "pca9536 datawidth switch for mt9v022"
> -	depends on SOC_CAMERA_MT9V022 && GENERIC_GPIO
> -	help
> -	  Select this if your MT9V022 camera uses a PCA9536 I2C GPIO
> -	  extender to switch between 8 and 10 bit datawidth modes
> -
>  config SOC_CAMERA_TW9910
>  	tristate "tw9910 support"
>  	depends on SOC_CAMERA && I2C
> diff --git a/drivers/media/video/mt9v022.c b/drivers/media/video/mt9v022.c
> index b04c8cb..26f97eb 100644
> --- a/drivers/media/video/mt9v022.c
> +++ b/drivers/media/video/mt9v022.c
> @@ -209,66 +209,6 @@ static int mt9v022_stop_capture(struct soc_camera_device *icd)
>  	return 0;
>  }
>  
> -static int bus_switch_request(struct mt9v022 *mt9v022, struct soc_camera_link *icl)
> -{
> -#ifdef CONFIG_MT9V022_PCA9536_SWITCH
> -	int ret;
> -	unsigned int gpio = icl->gpio;
> -
> -	if (gpio_is_valid(gpio)) {
> -		/* We have a data bus switch. */
> -		ret = gpio_request(gpio, "mt9v022");
> -		if (ret < 0) {
> -			dev_err(&mt9v022->client->dev, "Cannot get GPIO %u\n", gpio);
> -			return ret;
> -		}
> -
> -		ret = gpio_direction_output(gpio, 0);
> -		if (ret < 0) {
> -			dev_err(&mt9v022->client->dev,
> -				"Cannot set GPIO %u to output\n", gpio);
> -			gpio_free(gpio);
> -			return ret;
> -		}
> -	}
> -
> -	mt9v022->switch_gpio = gpio;
> -#else
> -	mt9v022->switch_gpio = -EINVAL;
> -#endif
> -	return 0;
> -}
> -
> -static void bus_switch_release(struct mt9v022 *mt9v022)
> -{
> -#ifdef CONFIG_MT9V022_PCA9536_SWITCH
> -	if (gpio_is_valid(mt9v022->switch_gpio))
> -		gpio_free(mt9v022->switch_gpio);
> -#endif
> -}
> -
> -static int bus_switch_act(struct mt9v022 *mt9v022, int go8bit)
> -{
> -#ifdef CONFIG_MT9V022_PCA9536_SWITCH
> -	if (!gpio_is_valid(mt9v022->switch_gpio))
> -		return -ENODEV;
> -
> -	gpio_set_value_cansleep(mt9v022->switch_gpio, go8bit);
> -	return 0;
> -#else
> -	return -ENODEV;
> -#endif
> -}
> -
> -static int bus_switch_possible(struct mt9v022 *mt9v022)
> -{
> -#ifdef CONFIG_MT9V022_PCA9536_SWITCH
> -	return gpio_is_valid(mt9v022->switch_gpio);
> -#else
> -	return 0;
> -#endif
> -}
> -
>  static int mt9v022_set_bus_param(struct soc_camera_device *icd,
>  				 unsigned long flags)
>  {
> @@ -282,19 +222,10 @@ static int mt9v022_set_bus_param(struct soc_camera_device *icd,
>  	if (!is_power_of_2(width_flag))
>  		return -EINVAL;
>  
> -	if ((mt9v022->datawidth != 10 && (width_flag == SOCAM_DATAWIDTH_10)) ||
> -	    (mt9v022->datawidth != 9  && (width_flag == SOCAM_DATAWIDTH_9)) ||
> -	    (mt9v022->datawidth != 8  && (width_flag == SOCAM_DATAWIDTH_8))) {
> -		/* Well, we actually only can do 10 or 8 bits... */
> -		if (width_flag == SOCAM_DATAWIDTH_9)
> -			return -EINVAL;
> -
> -		ret = bus_switch_act(mt9v022,
> -				     width_flag == SOCAM_DATAWIDTH_8);
> -		if (ret < 0)
> +	if (icl->set_bus_param) {
> +		ret = icl->set_bus_param(&mt9v022->client->dev, width_flag);
> +		if (ret)
>  			return ret;
> -
> -		mt9v022->datawidth = width_flag == SOCAM_DATAWIDTH_8 ? 8 : 10;
>  	}
>  
>  	flags = soc_camera_apply_sensor_flags(icl, flags);
> @@ -328,10 +259,14 @@ static int mt9v022_set_bus_param(struct soc_camera_device *icd,
>  static unsigned long mt9v022_query_bus_param(struct soc_camera_device *icd)
>  {
>  	struct mt9v022 *mt9v022 = container_of(icd, struct mt9v022, icd);
> -	unsigned int width_flag = SOCAM_DATAWIDTH_10;
> +	struct soc_camera_link *icl = mt9v022->client->dev.platform_data;
> +	unsigned int width_flag;
>  
> -	if (bus_switch_possible(mt9v022))
> -		width_flag |= SOCAM_DATAWIDTH_8;
> +	if (icl->query_bus_param)
> +		width_flag = icl->query_bus_param(&mt9v022->client->dev) &
> +			SOCAM_DATAWIDTH_MASK;
> +	else
> +		width_flag = SOCAM_DATAWIDTH_10;
>  
>  	return SOCAM_PCLK_SAMPLE_RISING | SOCAM_PCLK_SAMPLE_FALLING |
>  		SOCAM_HSYNC_ACTIVE_HIGH | SOCAM_HSYNC_ACTIVE_LOW |
> @@ -729,6 +664,7 @@ static int mt9v022_video_probe(struct soc_camera_device *icd)
>  	/* Set monochrome or colour sensor type */
>  	if (sensor_type && (!strcmp("colour", sensor_type) ||
>  			    !strcmp("color", sensor_type))) {
> +	if (1) {

Already pointed out by Hans.

>  		ret = reg_write(icd, MT9V022_PIXEL_OPERATION_MODE, 4 | 0x11);
>  		mt9v022->model = V4L2_IDENT_MT9V022IX7ATC;
>  		icd->formats = mt9v022_colour_formats;
> @@ -812,14 +748,6 @@ static int mt9v022_probe(struct i2c_client *client,
>  	icd->height_max	= 480;
>  	icd->y_skip_top	= 1;
>  	icd->iface	= icl->bus_id;
> -	/* Default datawidth - this is the only width this camera (normally)
> -	 * supports. It is only with extra logic that it can support
> -	 * other widths. Therefore it seems to be a sensible default. */
> -	mt9v022->datawidth = 10;
> -
> -	ret = bus_switch_request(mt9v022, icl);
> -	if (ret)
> -		goto eswinit;
>  
>  	ret = soc_camera_device_register(icd);
>  	if (ret)

As you remove ->gpio from struct soc_camera_link, you'll have to change

		if (gpio_is_valid(icl->gpio))

tests here too. Maybe make the format-selection in mt9v022 similar to 
mt9m001, even though I am not quite sure, that hard-coding the knowledge 
of the internal layout of the two format arrays (mt9m001_colour_formats 
and mt9m001_monochrome_formats) was a very good idea, but, well, I had 
already some assumptions about that in the code. But at least, maybe it is 
better to keep mt9m001 and mt9v022 look similar.

> @@ -828,8 +756,6 @@ static int mt9v022_probe(struct i2c_client *client,
>  	return 0;
>  
>  eisdr:
> -	bus_switch_release(mt9v022);
> -eswinit:
>  	kfree(mt9v022);
>  	return ret;
>  }
> @@ -839,7 +765,6 @@ static int mt9v022_remove(struct i2c_client *client)
>  	struct mt9v022 *mt9v022 = i2c_get_clientdata(client);
>  
>  	soc_camera_device_unregister(&mt9v022->icd);
> -	bus_switch_release(mt9v022);
>  	kfree(mt9v022);
>  
>  	return 0;
> -- 
> 1.5.6.5
> 

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer

Re: [PATCH 2/4] pcm990 baseboard: add camera bus width switch setting

[Sascha Hauer]

On Thu, Mar 12, 2009 at 10:25:35AM +0100, Guennadi Liakhovetski wrote:
> On Thu, 12 Mar 2009, Sascha Hauer wrote:
> 
> > On Thu, Mar 12, 2009 at 09:40:46AM +0100, Guennadi Liakhovetski wrote:
> > > One more thing I noticed while looking at your patch 3/4:
> > > 
> > > > +static int pcm990_camera_set_bus_param(struct device *dev,
> > > > +		unsigned long flags)
> > > > +{
> > > > +	if (gpio_bus_switch <= 0)
> > > > +		return 0;
> > > > +
> > > > +	if (flags & SOCAM_DATAWIDTH_8)
> > > > +		gpio_set_value(NR_BUILTIN_GPIO + 1, 1);
> > > > +	else
> > > > +		gpio_set_value(NR_BUILTIN_GPIO + 1, 0);
> > > 
> > > Originally the logic here was "only if flags == SOCAM_DATAWIDTH_8, switch 
> > > to 8 bits, otherwise do 10 bits. I.e., if flags == SOCAM_DATAWIDTH_8 | 
> > > SOCAM_DATAWIDTH_10, it would still do the default (and wider) 10 bits. Do 
> > > you have any reason to change that logic?
> > 
> > I was not aware that I changed any logic. I thought I would get here
> > with only one of the SOCAM_DATAWIDTH_* set. Isn't it a bug when we get
> > here with more than one width flags set?
> > 
> > The mt9v022 driver has this in set_bus_param:
> > 
> > >
> > >	/* Only one width bit may be set */
> > >	if (!is_power_of_2(width_flag))
> > >		return -EINVAL;
> > >
> > 
> > And I think it makes sense.
> 
> Ok, then, could you, please add the same test to mt9m001? And, as I 
> mentioned in a comment to 3/4, please, return an error if switching is 
> requested but unsupported.

Ok, will do.

It may be that we need a different approach to the bus width setting in
the longer term. For example the mt9m001 can support any thinkable bus
width of 1 to inf. bits. This depends on the hardware designer who wired
up the connection and not the physical count of data lines the chip has.
I have no idea how to implement this though.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 4/4] mt9v022: allow setting of bus width from board code

[Sascha Hauer]

On Wed, Mar 11, 2009 at 11:16:58AM +0100, Hans Verkuil wrote:
> 
> > This patch removes the phytec specific setting of the bus width
> > and switches to the more generic query_bus_param/set_bus_param
> > hooks
> >
> > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > ---
> >  drivers/media/video/Kconfig   |    7 ---
> >  drivers/media/video/mt9v022.c |   97
> > +++++------------------------------------
> >  2 files changed, 11 insertions(+), 93 deletions(-)
> >
> > diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig
> > index 5fc1531..071d66f 100644
> > --- a/drivers/media/video/Kconfig
> > +++ b/drivers/media/video/Kconfig
> > @@ -729,6 +664,7 @@ static int mt9v022_video_probe(struct
> > soc_camera_device *icd)
> >  	/* Set monochrome or colour sensor type */
> >  	if (sensor_type && (!strcmp("colour", sensor_type) ||
> >  			    !strcmp("color", sensor_type))) {
> > +	if (1) {
> >  		ret = reg_write(icd, MT9V022_PIXEL_OPERATION_MODE, 4 | 0x11);
> >  		mt9v022->model = V4L2_IDENT_MT9V022IX7ATC;
> >  		icd->formats = mt9v022_colour_formats;
> 
> 'if (1) {': some left-over debugging?

oops, yes.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 4/4] mt9v022: allow setting of bus width from board code

[Hans Verkuil]

> This patch removes the phytec specific setting of the bus width
> and switches to the more generic query_bus_param/set_bus_param
> hooks
>
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>  drivers/media/video/Kconfig   |    7 ---
>  drivers/media/video/mt9v022.c |   97
> +++++------------------------------------
>  2 files changed, 11 insertions(+), 93 deletions(-)
>
> diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig
> index 5fc1531..071d66f 100644
> --- a/drivers/media/video/Kconfig
> +++ b/drivers/media/video/Kconfig
> @@ -729,6 +664,7 @@ static int mt9v022_video_probe(struct
> soc_camera_device *icd)
>  	/* Set monochrome or colour sensor type */
>  	if (sensor_type && (!strcmp("colour", sensor_type) ||
>  			    !strcmp("color", sensor_type))) {
> +	if (1) {
>  		ret = reg_write(icd, MT9V022_PIXEL_OPERATION_MODE, 4 | 0x11);
>  		mt9v022->model = V4L2_IDENT_MT9V022IX7ATC;
>  		icd->formats = mt9v022_colour_formats;

'if (1) {': some left-over debugging?

Regards,

     Hans

-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG