Skip to contentSkip to conversation

ov9740: Initial submit of OV9740 driver.

PATCH Superseded [email protected] 2 participants 2 messages Feb 16, 2011 – Feb 17, 2011 Media (V4L/DVB)

v3 of 4 Superseded Latest is v4 →

lore
Share & tools
lore
#0 lore raw

PATCH v3 1/1 ov9740: Initial submit of OV9740 driver.

From: Andrew Chew <[email protected]>

This soc_camera driver is for Omnivision's OV9740 sensor. This initial submission provides support for YUV422 output at 1280x720 (720p), which is the sensor's native resolution. 640x480 (VGA) is also supported, with cropping and scaling performed by the sensor's ISP.

This driver is heavily based off of the existing OV9640 driver.

Change-Id I2f3b731e667a8f16df44e03eb5187176d3abb1f4

---
Applied all of the suggestions from Guennadi Liakhovetski, to wit:

Renamed to_ov9740_sensor macro to to_ov9740.
Added spaces to the sides of comments.
Some renaming of 1280x720 to 720P, and 640x480 to VGA.
Added global array of supported resolutions and their extents. Fixed spacing problem with curly braces.
Removed register arrays for the start and stop streaming register pokes. Removed IS_ERR_VALUE uses.
Changed a set of tiny memcpy's to assignment operations.
Renamed "ary" to "array" in all cases.
Removed a comment accidentally carried over from the OV9640 regarding bus width. Renamed ov9740_set_res_code() to ov9740_set_res().
Changed ov9740_try_fmt() to not return an error if format is not supported. Removed __devinit and __devexit.
Fixed an endianness issue with reading the 16-bit model.
Removed unnecessary "devname" variable in probe call.
Implemented cropcap() and g_crop() methods.

 drivers/media/video/Kconfig     |    6 +
 drivers/media/video/Makefile    |    1 +
 drivers/media/video/ov9740.c    | 1013 +++++++++++++++++++++++++++++++++++++++
 include/media/v4l2-chip-ident.h |    1 +
 4 files changed, 1021 insertions(+), 0 deletions(-)
 create mode 100644 drivers/media/video/ov9740.c
drivers/media/video/Kconfig +6 -0
index d40a8fc..52b6271 100644
@@ -837,6 +837,12 @@ config SOC_CAMERA_OV9640
 	help
 	  This is a ov9640 camera driver
 
+config SOC_CAMERA_OV9740
+	tristate "ov9740 camera support"
+	depends on SOC_CAMERA && I2C
+	help
+	  This is a ov9740 camera driver
+
 config MX1_VIDEO
 	bool
 
drivers/media/video/Makefile +1 -0
index 251b7ca..ac54652 100644
@@ -79,6 +79,7 @@ obj-$(CONFIG_SOC_CAMERA_OV2640)		+= ov2640.o
 obj-$(CONFIG_SOC_CAMERA_OV6650)		+= ov6650.o
 obj-$(CONFIG_SOC_CAMERA_OV772X)		+= ov772x.o
 obj-$(CONFIG_SOC_CAMERA_OV9640)		+= ov9640.o
+obj-$(CONFIG_SOC_CAMERA_OV9740)		+= ov9740.o
 obj-$(CONFIG_SOC_CAMERA_RJ54N1)		+= rj54n1cb0c.o
 obj-$(CONFIG_SOC_CAMERA_TW9910)		+= tw9910.o
 
drivers/media/video/ov9740.c new file +1013 -0

1,013 lines not on this page · raw message

include/media/v4l2-chip-ident.h +1 -0
index ff4a52c..b3edb67 100644
@@ -75,6 +75,7 @@ enum {
 	V4L2_IDENT_OV9640 = 257,
 	V4L2_IDENT_OV6650 = 258,
 	V4L2_IDENT_OV2640 = 259,
+	V4L2_IDENT_OV9740 = 260,
 
 	/* module saa7146: reserved range 300-309 */
 	V4L2_IDENT_SAA7146 = 300,
Signature · 1 line
1.7.0.4
#1 lore raw

Hi Andrew

Thanks for an update, looks much better now, still, a couple of things _have_ to be corrected before we can push it:

Quoted 49 lines · On Wed, 16 Feb 2011, [email protected] wrote:

From: Andrew Chew <[email protected]>

This soc_camera driver is for Omnivision's OV9740 sensor. This initial submission provides support for YUV422 output at 1280x720 (720p), which is the sensor's native resolution. 640x480 (VGA) is also supported, with cropping and scaling performed by the sensor's ISP.

This driver is heavily based off of the existing OV9640 driver.

Change-Id I2f3b731e667a8f16df44e03eb5187176d3abb1f4

---
Applied all of the suggestions from Guennadi Liakhovetski, to wit:

Renamed to_ov9740_sensor macro to to_ov9740.
Added spaces to the sides of comments.
Some renaming of 1280x720 to 720P, and 640x480 to VGA.
Added global array of supported resolutions and their extents. Fixed spacing problem with curly braces.
Removed register arrays for the start and stop streaming register pokes. Removed IS_ERR_VALUE uses.
Changed a set of tiny memcpy's to assignment operations.
Renamed "ary" to "array" in all cases.
Removed a comment accidentally carried over from the OV9640 regarding bus width. Renamed ov9740_set_res_code() to ov9740_set_res().
Changed ov9740_try_fmt() to not return an error if format is not supported. Removed __devinit and __devexit.
Fixed an endianness issue with reading the 16-bit model.
Removed unnecessary "devname" variable in probe call.
Implemented cropcap() and g_crop() methods.

 drivers/media/video/Kconfig     |    6 +
 drivers/media/video/Makefile    |    1 +
 drivers/media/video/ov9740.c    | 1013 +++++++++++++++++++++++++++++++++++++++
 include/media/v4l2-chip-ident.h |    1 +
 4 files changed, 1021 insertions(+), 0 deletions(-)
 create mode 100644 drivers/media/video/ov9740.c
drivers/media/video/Kconfig +0 -0
index d40a8fc..52b6271 100644
drivers/media/video/ov9740.c new file +0 -0
new file mode 100644
index 0000000..e5eee66
@@ -0,0 +1,1013 @@

[snip]

Quoted 14 lines
drivers/media/video/ov9740.c new file +14 -0
+/* Start/Stop streaming from the device */
+static int ov9740_s_stream(struct v4l2_subdev *sd, int enable)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(sd);
+	struct ov9740_priv *priv = to_ov9740(sd);
+	int ret;
+
+	/* Program orientation register. */
+	if (priv->flag_vflip)
+		ret = ov9740_reg_rmw(client, OV9740_IMAGE_ORT, 0x2, 0);
+	else
+		ret = ov9740_reg_rmw(client, OV9740_IMAGE_ORT, 0, 0x2);
+	if (ret < 0)
+		return ret;

drivers/media/video/ov9740.c line 597

Quoted 10 lines
drivers/media/video/ov9740.c new file +10 -0
+
+	if (priv->flag_hflip)
+		ret = ov9740_reg_rmw(client, OV9740_IMAGE_ORT, 0x1, 0);
+	else
+		ret = ov9740_reg_rmw(client, OV9740_IMAGE_ORT, 0, 0x1);
+	if (ret < 0)
+		return ret;
+
+	if (enable) {
+		dev_info(&client->dev, "Enabling Streaming\n");

This

drivers/media/video/ov9740.c line 601

Quoted 4 lines
drivers/media/video/ov9740.c new file +4 -0
+		ret = ov9740_reg_write(client, OV9740_MODE_SELECT, 0x01);
+
+	} else {
+		dev_info(&client->dev, "Disabling Streaming\n");

and this dev_info() you might want to change to dev_dbg(). You don't want the driver to be too noisy. A couple more occasions below

drivers/media/video/ov9740.c line 605

Quoted 4 lines
drivers/media/video/ov9740.c new file +4 -0
+		ret = ov9740_reg_write(client, OV9740_SOFTWARE_RESET, 0x01);
+		if (ret < 0)
+			return ret;
+		ret = ov9740_reg_write(client, OV9740_MODE_SELECT, 0x00);

In the previous version you had comments for streamoff::

+ /* Software Reset */
and
+ /* Setting Streaming to Standby */

you can still use them now. Besides, the following is a bit shorter and I find it slightly better readable:

+ ret = ov9740_reg_write(client, OV9740_SOFTWARE_RESET, 0x01);
+ if (!ret)
+ ret = ov9740_reg_write(client, OV9740_MODE_SELECT, 0x00);

Quoted 10 lines
drivers/media/video/ov9740.c new file +10 -0
+	}
+
+	return ret;
+}
+
+/* Alter bus settings on camera side */
+static int ov9740_set_bus_param(struct soc_camera_device *icd,
+				unsigned long flags)
+{
+	return 0;

drivers/media/video/ov9740.c line 625

Quoted 10 lines
drivers/media/video/ov9740.c new file +10 -0
+}
+
+/* Request bus settings on camera side */
+static unsigned long ov9740_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_DATA_ACTIVE_HIGH | SOCAM_DATAWIDTH_8;

I asked you to use the platform data here... If you don't do that, then at least, please, preserve your comment from the original version:

+ /*
+ * REVISIT: the camera probably can do 10 bit transfers, but I don't
+ * have those pins connected on my hardware.
+ */

Quoted 3 lines
drivers/media/video/ov9740.c new file +3 -0
+
+	return soc_camera_apply_sensor_flags(icl, flags);
+}

[snip]

drivers/media/video/ov9740.c line 734

Quoted 4 lines
drivers/media/video/ov9740.c new file +4 -0
+/* Setup registers according to resolution and color encoding */
+static int ov9740_set_res(struct i2c_client *client, u32 width,
+			  enum v4l2_mbus_pixelcode code)
+{

Why don't you just remove the "code" parameter as I proposed in my previous review? I don't see why you need it, and if it is ever needed, it can always be re-added.

drivers/media/video/ov9740.c line 739

Quoted 5 lines
drivers/media/video/ov9740.c new file +5 -0
+	int ret;
+
+	/* select register configuration for given resolution */
+	if (width == ov9740_resolutions[OV9740_VGA].width) {
+		dev_info(&client->dev, "Setting image size to 640x480\n");

dev_dbg()

drivers/media/video/ov9740.c line 743

Quoted 4 lines
drivers/media/video/ov9740.c new file +4 -0
+		ret = ov9740_reg_write_array(client, ov9740_regs_vga,
+					     ARRAY_SIZE(ov9740_regs_vga));
+	} else if (width == ov9740_resolutions[OV9740_720P].width) {
+		dev_info(&client->dev, "Setting image size to 1280x720\n");

dev_dbg()

Quoted 16 lines
drivers/media/video/ov9740.c new file +15 -0
+		ret = ov9740_reg_write_array(client, ov9740_regs_720p,
+					     ARRAY_SIZE(ov9740_regs_720p));
+	} else {
+		dev_err(&client->dev, "Failed to select resolution!\n");
+		return -EINVAL;
+	}
+
+	return ret;
+}
 
+static int ov9740_try_fmt(struct v4l2_subdev *sd,
+			  struct v4l2_mbus_framefmt *mf)
+{
+	ov9740_res_roundup(&mf->width, &mf->height);
+
+	mf->field = V4L2_FIELD_NONE;

drivers/media/video/ov9740.c line 803

Quoted 10 lines
drivers/media/video/ov9740.c new file +10 -0
+
+	switch (mf->code) {
+	case V4L2_MBUS_FMT_YUYV8_2X8:
+		mf->colorspace = V4L2_COLORSPACE_SRGB;
+		break;
+	default:
+		break;
+	}
+
+	return 0;

Ok, this is the important one: sorry, this is not what I meant. You shouldn't fail try_fmt(), that's right, but you should _always_ return valid data from it. In your case you just can set mf->code and mf->colorspace always to your fixed values and remove the switch completely.

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