Skip to contentSkip to conversation

OV7670: getting it working with soc-camera.

PATCH Jonathan Cameron 2 participants 5 messages Jun 17, 2009 – Jun 19, 2009 Media (V4L/DVB)

lore
Share & tools
lore

Jonathan Cameron

author
#0 lore raw

This is purely for info of anyone else wanting to use the ov7670 with Guennadi's recent work on converted soc-camera to v4l2-subdevs.

It may not be completely minimal, but it's letting me take pictures ;)

Couple of minor queries:

Currently it is assumed that there is a means of telling the chip to use particular bus params. In the case of this one it doesn't support anything other than 8 bit. Stuff may get added down the line, but in meantime does anyone mind if we make icd->ops->set_bus_param optional in soc-camera?

Is there any reason (or advantage) in not specifying the i2c address in the driver? Or for that matter why the address is right shifted by 1 in:

v4l_info(client, "chip found @ 0x%02x (%s)\n",

	 client->addr << 1, client->adapter->name);

Admittedly the data sheet uses an 'unusual' convention for the address (separate write and read address which correspond to a single address of 0x21 with the relevant write bit set as appropriate).

As ever any comments welcome. Thanks to Guennadi Liakhovetski for his soc-camera work and Hans Verkuil for conversion of the ov7670 to soc-dev.

Tested against a merge of todays v4l-next tree and Linus' current with the minor pxa-camera bug I posted earlier fixed and Guennadi's extensive patch set applied (this requires a few hand merges, but nothing too nasty).

---

include/media/ov7670_soc.h new file +21 -0
new file mode 100644
index 0000000..2f264b2
@@ -0,0 +1,21 @@
+/* ov7670_soc Camera
+ *
+ * Copyright (C) 2009 Jonathan Cameron <[email protected]>
+ *
+ * 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 __OV7670_SOC_H__
+#define __OV7670_SOC_H__
+
+#include <media/soc_camera.h>
+
+struct ov7670_soc_camera_info {
+	int gpio_pwr;
+	int gpio_reset;
+	struct soc_camera_link link;
+};
+
+#endif
drivers/media/video/ov7670.c +88 -1

124 lines not on this page · raw message

#1 lore raw
Quoted 4 lines · Jonathan Cameron

This is purely for info of anyone else wanting to use the ov7670 with Guennadi's recent work on converted soc-camera to v4l2-subdevs.

It may not be completely minimal, but it's letting me take pictures ;)

Cool, I like it! Not the pictures, but the fact that the required patch turned out to be so small. Of course, you understand this is not what we'll eventually commit, but, I think, this is a good start. In principle, if a device has all parameters fixed, there's no merit in trying to set them.

Quoted 7 lines

Couple of minor queries:

Currently it is assumed that there is a means of telling the chip to use particular bus params. In the case of this one it doesn't support anything other than 8 bit. Stuff may get added down the line, but in meantime does anyone mind if we make icd->ops->set_bus_param optional in soc-camera?

struct soc_camera_ops will disappear completely anyway, and we don't know yet what the v4l2-subdev counterpart will look like.

Quoted 2 lines

Is there any reason (or advantage) in not specifying the i2c address in the driver?

Some i2c devices can be configured to respond to one of several i2c addresses.

Quoted 10 lines

Or for that matter why the address is right shifted by 1 in:

v4l_info(client, "chip found @ 0x%02x (%s)\n",

	 client->addr << 1, client->adapter->name);

Admittedly the data sheet uses an 'unusual' convention for the address (separate write and read address which correspond to a single address of 0x21 with the relevant write bit set as appropriate).

That's exactly the reason, I think. Many (or most?) datasheets specify i2c addresses which are a double of Linux i2c address. IIRC this is just a Linux convention to use the shifted address.

Quoted 8 lines

As ever any comments welcome. Thanks to Guennadi Liakhovetski for his soc-camera work and Hans Verkuil for conversion of the ov7670 to soc-dev.

Tested against a merge of todays v4l-next tree and Linus' current with the minor pxa-camera bug I posted earlier fixed and Guennadi's extensive patch set applied (this requires a few hand merges, but nothing too nasty).

Good to know.

A couple of comments:

Quoted 13 lines
include/media/ov7670_soc.h new file +7 -0
new file mode 100644
index 0000000..2f264b2
@@ -0,0 +1,21 @@
+/* ov7670_soc Camera
+ *
+ * Copyright (C) 2009 Jonathan Cameron <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.

include/media/ov7670_soc.h line 17

Quoted 10 lines
include/media/ov7670_soc.h new file +10 -0
+ */
+
+#ifndef __OV7670_SOC_H__
+#define __OV7670_SOC_H__
+
+#include <media/soc_camera.h>
+
+struct ov7670_soc_camera_info {
+	int gpio_pwr;
+	int gpio_reset;

You should not need these GPIOs...

Quoted 9 lines
include/media/ov7670_soc.h new file +4 -0
+	struct soc_camera_link link;
+};
+
+#endif
drivers/media/video/ov7670.c +0 -0
index 0e2184e..51d432e 100644

drivers/media/video/ov7670.c line 28

Quoted 10 lines
drivers/media/video/ov7670.c +6 -0
@@ -19,7 +19,14 @@
 #include <media/v4l2-chip-ident.h>
 #include <media/v4l2-i2c-drv.h>
 
+#define OV7670_SOC
 
+
+#ifdef OV7670_SOC
+#include <media/ov7670_soc.h>
+#include <media/soc_camera.h>
+#include <linux/gpio.h>

...this header...

Quoted 72 lines
drivers/media/video/ov7670.c +57 -0
+#endif /* OV7670_SOC */
 MODULE_AUTHOR("Jonathan Corbet <[email protected]>");
 MODULE_DESCRIPTION("A low-level driver for OmniVision ov7670 sensors");
 MODULE_LICENSE("GPL");

@@ -1239,19 +1246,94 @@ static const struct v4l2_subdev_ops ov7670_ops = {
 };
 
 /* ----------------------------------------------------------------------- */
+#ifdef OV7670_SOC
+
+static unsigned long ov7670_soc_query_bus_param(struct soc_camera_device *icd)
+{
+	struct soc_camera_link *icl = to_soc_camera_link(icd);
+
+	unsigned long flags = SOCAM_PCLK_SAMPLE_RISING | SOCAM_MASTER |
+		SOCAM_VSYNC_ACTIVE_HIGH | SOCAM_HSYNC_ACTIVE_HIGH |
+		SOCAM_DATAWIDTH_8 | SOCAM_DATA_ACTIVE_HIGH;
+
+	return soc_camera_apply_sensor_flags(icl, flags);
+}
+/* This device only supports one bus option */
+static int ov7670_soc_set_bus_param(struct soc_camera_device *icd,
+				    unsigned long flags)
+{
+	return 0;
+}
+
+static struct soc_camera_ops ov7670_soc_ops = {
+	.set_bus_param = ov7670_soc_set_bus_param,
+	.query_bus_param = ov7670_soc_query_bus_param,
+};
+
+#define SETFOURCC(type) .name = (#type), .fourcc = (V4L2_PIX_FMT_ ## type)
+static const struct soc_camera_data_format ov7670_soc_fmt_lists[] = {
+	{
+		SETFOURCC(YUYV),
+		.depth = 16,
+		.colorspace = V4L2_COLORSPACE_JPEG,
+	}, {
+		SETFOURCC(RGB565),
+		.depth = 16,
+		.colorspace = V4L2_COLORSPACE_SRGB,
+	},
+};
 
+#endif
 static int ov7670_probe(struct i2c_client *client,
 			const struct i2c_device_id *id)
 {
+#ifdef OV7670_SOC
+	struct soc_camera_device *icd = client->dev.platform_data;
+	struct soc_camera_link *icl;
+	struct ov7670_soc_camera_info *board_info;
+#endif
 	struct v4l2_subdev *sd;
 	struct ov7670_info *info;
+
 	int ret;
 
+#ifdef OV7670_SOC
+	icl = to_soc_camera_link(icd);
+	if (!icl)
+		return -EINVAL;
+	board_info = container_of(icl, struct ov7670_soc_camera_info, link);
+
+	gpio_request(board_info->gpio_reset, "ov7670 soc reset");
+	gpio_request(board_info->gpio_pwr, "ov7670 soc power");
+
+	/* reset high for normal mode */
+	gpio_direction_output(board_info->gpio_reset, 1);
+	/* power down normal mode. */
+	gpio_direction_output(board_info->gpio_pwr, 0);

drivers/media/video/ov7670.c static const struct v4l2_subdev_ops ov7670_ops = { · line 1322

Quoted 10 lines
drivers/media/video/ov7670.c +10 -0
+	/* perform a hard reset as per tinyos code */
+	gpio_set_value(board_info->gpio_pwr, 1);
+	gpio_set_value(board_info->gpio_reset, 1);
+	mdelay(2);
+	gpio_set_value(board_info->gpio_pwr, 0);
+	gpio_set_value(board_info->gpio_reset, 0);
+	mdelay(2);
+	gpio_set_value(board_info->gpio_reset, 1);
+	mdelay(5);
+#endif

...and this switching. All this should be done in struct soc_camera_link .power() and .reset() methods in your platform code.

Quoted 18 lines
drivers/media/video/ov7670.c +9 -0
 	info = kzalloc(sizeof(struct ov7670_info), GFP_KERNEL);
 	if (info == NULL)
 		return -ENOMEM;
+	/* JIC; whole load of reset code may be needed */
+
 	sd = &info->sd;
 	v4l2_i2c_subdev_init(sd, client, &ov7670_ops);
+#ifdef OV7670_SOC
+	icd->ops = &ov7670_soc_ops;
+	icd->rect_max.width = VGA_WIDTH;
+	icd->rect_max.height = VGA_HEIGHT;
+	icd->formats = ov7670_soc_fmt_lists;
+	icd->num_formats = ARRAY_SIZE(ov7670_soc_fmt_lists);
+#endif
 
 	/* Make sure it's an ov7670 */
 	ret = ov7670_detect(sd);

drivers/media/video/ov7670.c static int ov7670_remove(struct i2c_client *client) · line 1371

Quoted 9 lines
drivers/media/video/ov7670.c +5 -1
@@ -1282,7 +1364,11 @@ static int ov7670_remove(struct i2c_client *client)
 }
 
 static const struct i2c_device_id ov7670_id[] = {
-	{ "ov7670", 0 },
+#ifdef OV7670_SOC
+	{ "ov7670", 0x21 },
+#else
+  	{ "ov7670", 0 },
+#endif

And you don't need this, just specify the address in platform code.

drivers/media/video/ov7670.c static int ov7670_remove(struct i2c_client *client) · line 1374

Quoted 3 lines
drivers/media/video/ov7670.c +0 -0
 	{ }
 };
 MODULE_DEVICE_TABLE(i2c, ov7670_id);

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

author
#2 lore raw
Quoted 5 lines · Guennadi Liakhovetski
Quoted 3 lines · Jonathan Cameron

This is purely for info of anyone else wanting to use the ov7670 with Guennadi's recent work on converted soc-camera to v4l2-subdevs.

Quoted 7 lines
Quoted 1 line

It may not be completely minimal, but it's letting me take pictures ;)

Cool, I like it! Not the pictures, but the fact that the required patch turned out to be so small. Of course, you understand this is not what we'll eventually commit, but, I think, this is a good start. In principle, if a device has all parameters fixed, there's no merit in trying to set them.

Yup, my intention is to slowly remove elements as they become unnecessary (and push any that actually make sense to the mailing list).

Quoted 12 lines
Quoted 8 lines

Couple of minor queries:

Currently it is assumed that there is a means of telling the chip to use particular bus params. In the case of this one it doesn't support anything other than 8 bit. Stuff may get added down the line, but in meantime does anyone mind if we make icd->ops->set_bus_param optional in soc-camera?

struct soc_camera_ops will disappear completely anyway, and we don't know yet what the v4l2-subdev counterpart will look like.

Sure, I'll wait and see whether this question is relevant down the line.

...

Quoted 6 lines
Quoted 6 lines

Or for that matter why the address is right shifted by 1 in:

v4l_info(client, "chip found @ 0x%02x (%s)\n",

	 client->addr << 1, client->adapter->name);
Quoted 8 lines
Quoted 4 lines

Admittedly the data sheet uses an 'unusual' convention for the address (separate write and read address which correspond to a single address of 0x21 with the relevant write bit set as appropriate).

That's exactly the reason, I think. Many (or most?) datasheets specify i2c addresses which are a double of Linux i2c address. IIRC this is just a Linux convention to use the shifted address.

Um. I'm not sure I agree with this. The convention when specifying the address in registration is to use correct one (without the write bit) and based on a lot of non video chips I've come across is about 50 / 50 on how they document it (with many using a delightful and random mix of the two) If you are going to have a registration scheme that requires the board code to specify the address as 0x21 then to my mind having the driver declare it as being on 0x42 seems rather odd and misleading.

This is particularly true here where the driver is using smbus calls as that specification is very clear indeed on the fact that addresses are 7 bit. Admittedly this chip uses the sccb bus protocol that just 'happens' to bare a startling resemblance to smbus / i2c.

Still this isn't exactly a crucial element of the driver!

Quoted 13 lines
Quoted 8 lines

As ever any comments welcome. Thanks to Guennadi Liakhovetski for his soc-camera work and Hans Verkuil for conversion of the ov7670 to soc-dev.

Tested against a merge of todays v4l-next tree and Linus' current with the minor pxa-camera bug I posted earlier fixed and Guennadi's extensive patch set applied (this requires a few hand merges, but nothing too nasty).

Good to know.

A couple of comments:

...

Quoted 4 lines
Quoted 1 line

+#endif

...and this switching. All this should be done in struct soc_camera_link .power() and .reset() methods in your platform code.

Ah, I'd missed those methods, thanks!

You are quite right about the i2c_device_table as well, not sure what got into me there.

Thanks,

Jonathan

author
#3 lore raw

Updated temporary patch to get ov7670 working with soc camera.

---
Basically this is the original patch with the changes Guennadi suggested. Again this is only for info, not a formal patch submission.

drivers/media/video/ov7670.c +51 -0
index 0e2184e..9bea804 100644
@@ -19,6 +19,12 @@
 #include <media/v4l2-chip-ident.h>
 #include <media/v4l2-i2c-drv.h>
 
+#define OV7670_SOC
+
+
+#ifdef OV7670_SOC
+#include <media/soc_camera.h>
+#endif /* OV7670_SOC */
 
 MODULE_AUTHOR("Jonathan Corbet <[email protected]>");
 MODULE_DESCRIPTION("A low-level driver for OmniVision ov7670 sensors");

@@ -1239,13 +1245,58 @@ static const struct v4l2_subdev_ops ov7670_ops = {
 };
 
 /* ----------------------------------------------------------------------- */
+#ifdef OV7670_SOC
+
+static unsigned long ov7670_soc_query_bus_param(struct soc_camera_device *icd)
+{
+	struct soc_camera_link *icl = to_soc_camera_link(icd);
+
+	unsigned long flags = SOCAM_PCLK_SAMPLE_RISING | SOCAM_MASTER |
+		SOCAM_VSYNC_ACTIVE_HIGH | SOCAM_HSYNC_ACTIVE_HIGH |
+		SOCAM_DATAWIDTH_8 | SOCAM_DATA_ACTIVE_HIGH;
+
+	return soc_camera_apply_sensor_flags(icl, flags);
+}
+/* This device only supports one bus option */
+static int ov7670_soc_set_bus_param(struct soc_camera_device *icd,
+				    unsigned long flags)
+{
+	return 0;
+}
+
+static struct soc_camera_ops ov7670_soc_ops = {
+	.set_bus_param = ov7670_soc_set_bus_param,
+	.query_bus_param = ov7670_soc_query_bus_param,
+};
 
+#define SETFOURCC(type) .name = (#type), .fourcc = (V4L2_PIX_FMT_ ## type)
+static const struct soc_camera_data_format ov7670_soc_fmt_lists[] = {
+	{
+		SETFOURCC(YUYV),
+		.depth = 16,
+		.colorspace = V4L2_COLORSPACE_JPEG,
+	}, {
+		SETFOURCC(RGB565),
+		.depth = 16,
+		.colorspace = V4L2_COLORSPACE_SRGB,
+	},
+};
+
+#endif
 static int ov7670_probe(struct i2c_client *client,
 			const struct i2c_device_id *id)
 {
 	struct v4l2_subdev *sd;
 	struct ov7670_info *info;
 	int ret;
+#ifdef OV7670_SOC
+	struct soc_camera_device *icd = client->dev.platform_data;
+	icd->ops = &ov7670_soc_ops;
+	icd->rect_max.width = VGA_WIDTH;
+	icd->rect_max.height = VGA_HEIGHT;
+	icd->formats = ov7670_soc_fmt_lists;
+	icd->num_formats = ARRAY_SIZE(ov7670_soc_fmt_lists);
+#endif
 
 	info = kzalloc(sizeof(struct ov7670_info), GFP_KERNEL);
 	if (info == NULL)
author
#4 lore raw

Hi All,

Turns out that, in addition the below, one more soc-camera ops function is still needed.

Init is required in my case to make a call to the subdev->ops->core_ops->init function in order to put the register contents back after the reset callback in soc_camera_open.

I'm guessing down the line it will make sense to call this directly in soc_camera or are the circumstances where it should call additional init functions?

Quoted 84 lines

Updated temporary patch to get ov7670 working with soc camera.

---
Basically this is the original patch with the changes Guennadi suggested. Again this is only for info, not a formal patch submission.

drivers/media/video/ov7670.c +51 -0
index 0e2184e..9bea804 100644
@@ -19,6 +19,12 @@
 #include <media/v4l2-chip-ident.h>
 #include <media/v4l2-i2c-drv.h>
 
+#define OV7670_SOC
+
+
+#ifdef OV7670_SOC
+#include <media/soc_camera.h>
+#endif /* OV7670_SOC */
 
 MODULE_AUTHOR("Jonathan Corbet <[email protected]>");
 MODULE_DESCRIPTION("A low-level driver for OmniVision ov7670 sensors");

@@ -1239,13 +1245,58 @@ static const struct v4l2_subdev_ops ov7670_ops = {
 };
 
 /* ----------------------------------------------------------------------- */
+#ifdef OV7670_SOC
+
+static unsigned long ov7670_soc_query_bus_param(struct soc_camera_device *icd)
+{
+	struct soc_camera_link *icl = to_soc_camera_link(icd);
+
+	unsigned long flags = SOCAM_PCLK_SAMPLE_RISING | SOCAM_MASTER |
+		SOCAM_VSYNC_ACTIVE_HIGH | SOCAM_HSYNC_ACTIVE_HIGH |
+		SOCAM_DATAWIDTH_8 | SOCAM_DATA_ACTIVE_HIGH;
+
+	return soc_camera_apply_sensor_flags(icl, flags);
+}
+/* This device only supports one bus option */
+static int ov7670_soc_set_bus_param(struct soc_camera_device *icd,
+				    unsigned long flags)
+{
+	return 0;
+}
+
+static struct soc_camera_ops ov7670_soc_ops = {
+	.set_bus_param = ov7670_soc_set_bus_param,
+	.query_bus_param = ov7670_soc_query_bus_param,
+};
 
+#define SETFOURCC(type) .name = (#type), .fourcc = (V4L2_PIX_FMT_ ## type)
+static const struct soc_camera_data_format ov7670_soc_fmt_lists[] = {
+	{
+		SETFOURCC(YUYV),
+		.depth = 16,
+		.colorspace = V4L2_COLORSPACE_JPEG,
+	}, {
+		SETFOURCC(RGB565),
+		.depth = 16,
+		.colorspace = V4L2_COLORSPACE_SRGB,
+	},
+};
+
+#endif
 static int ov7670_probe(struct i2c_client *client,
 			const struct i2c_device_id *id)
 {
 	struct v4l2_subdev *sd;
 	struct ov7670_info *info;
 	int ret;
+#ifdef OV7670_SOC
+	struct soc_camera_device *icd = client->dev.platform_data;
+	icd->ops = &ov7670_soc_ops;
+	icd->rect_max.width = VGA_WIDTH;
+	icd->rect_max.height = VGA_HEIGHT;
+	icd->formats = ov7670_soc_fmt_lists;
+	icd->num_formats = ARRAY_SIZE(ov7670_soc_fmt_lists);
+#endif
 
 	info = kzalloc(sizeof(struct ov7670_info), GFP_KERNEL);
 	if (info == NULL)