Re: [PATCH 2/3] Add driver for OmniVision OV9640 sensor
Dne Pá 11. září 2009 23:51:44 Guennadi Liakhovetski napsal(a): On Fri, 4 Sep 2009, Marek Vasut wrote: Dne Po 24. srpna 2009 20:06:29 Guennadi Liakhovetski napsal(a): Hi Marek On Sat, 22 Aug 2009, Marek Vasut wrote: From 479aafc9a6540efec8a691a84aff166eb0218a72 Mon Sep 17 00:00:00 2001 From: Marek Vasut marek.va...@gmail.com Date: Sat, 22 Aug 2009 05:14:23 +0200 Subject: [PATCH 2/3] Add driver for OmniVision OV9640 sensor Signed-off-by: Marek Vasut marek.va...@gmail.com Ok, I see, you rebased your patches on my soc-camera patch-stack, this is good, thanks. But you missed a couple of my comments - you still have a few static functions in the ov9640.c marked inline: ov9640_set_crop() is not used at all, ov9640_set_bus_param() gets assigned to a struct member, so, cannot be inline. ov9640_alter_regs() is indeed only called at one location, but see Chapter 15 in Documentation/CodingStyle. You left at least one multi-line comment wrongly formatted. You left some broken printk format lines, etc. You moved your header under drivers/... - that's good. But, please, address all my comments that I provided in a private review, or explain why you do not agree. Otherwise I feel like I wasted my time. Besides, your mailer has wrapped lines. Please, read Documentation/email-clients.txt on how to configure your email client to send patches properly. In the worst case, you can inline your patches while reviewing, and then attach them for a final submission. This is, however, discouraged, because it makes review and test of your intermediate patches with wrapped lines more difficult. Also, please check your patches with scripts/checkpatch.pl. Fixed patch follows, my mailer is fixed as much as possible (working on getting git-email to work, but that's still to be done). Please consider applying, thanks. Ok, a couple more simple questions / remarks, In principle, there's just one principle objection - if we agree, that the correct format is BGR565 and RGB565X, then we should change that. There's no BGR565 format currently in the kernel, so, we'd have to add it (and the documentation for the mercurial tree). Thanks Guennadi Add driver for OmniVision OV9640 sensor Signed-off-by: Marek Vasut marek.va...@gmail.com --- drivers/media/video/Kconfig |6 + drivers/media/video/Makefile|1 + drivers/media/video/ov9640.c| 811 +++ drivers/media/video/ov9640.h| 206 ++ include/media/v4l2-chip-ident.h |1 + 5 files changed, 1025 insertions(+), 0 deletions(-) create mode 100644 drivers/media/video/ov9640.c create mode 100644 drivers/media/video/ov9640.h diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig index 84b6fc1..fddd654 100644 --- a/drivers/media/video/Kconfig +++ b/drivers/media/video/Kconfig @@ -772,6 +772,12 @@ config SOC_CAMERA_OV772X help This is a ov772x camera driver +config SOC_CAMERA_OV9640 + tristate ov9640 camera support + depends on SOC_CAMERA I2C + help + This is a ov9640 camera driver + config MX1_VIDEO bool diff --git a/drivers/media/video/Makefile b/drivers/media/video/Makefile index 9f2e321..e18efd5 100644 --- a/drivers/media/video/Makefile +++ b/drivers/media/video/Makefile @@ -76,6 +76,7 @@ obj-$(CONFIG_SOC_CAMERA_MT9M111) += mt9m111.o obj-$(CONFIG_SOC_CAMERA_MT9T031) += mt9t031.o obj-$(CONFIG_SOC_CAMERA_MT9V022) += mt9v022.o obj-$(CONFIG_SOC_CAMERA_OV772X)+= ov772x.o +obj-$(CONFIG_SOC_CAMERA_OV9640)+= ov9640.o obj-$(CONFIG_SOC_CAMERA_TW9910)+= tw9910.o # And now the v4l2 drivers: diff --git a/drivers/media/video/ov9640.c b/drivers/media/video/ov9640.c new file mode 100644 index 000..289e82d --- /dev/null +++ b/drivers/media/video/ov9640.c @@ -0,0 +1,811 @@ +/* + * OmniVision OV96xx Camera Driver + * + * Copyright (C) 2009 Marek Vasut marek.va...@gmail.com + * + * Based on ov772x camera driver: + * + * Copyright (C) 2008 Renesas Solutions Corp. + * Kuninori Morimoto morimoto.kunin...@renesas.com + * + * Based on ov7670 and soc_camera_platform driver, + * + * Copyright 2006-7 Jonathan Corbet cor...@lwn.net + * Copyright (C) 2008 Magnus Damm + * Copyright (C) 2008, Guennadi Liakhovetski ker...@pengutronix.de + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + */ + +#include linux/init.h +#include linux/module.h +#include linux/i2c.h +#include linux/slab.h +#include linux/delay.h +#include linux/videodev2.h +#include media/v4l2-chip-ident.h +#include media/v4l2-common.h +#include media/soc_camera.h + +#include ov9640.h + +/* default
Re: [PATCH 2/3] Add driver for OmniVision OV9640 sensor
On Mon, 14 Sep 2009, Marek Vasut wrote:
[snip]
+/* NOTE: The RGB555X format still has issues, so it's left out. */
+static const struct soc_camera_data_format ov9640_fmt_lists[] = {
+{
+ .name = VYUY,
+ .fourcc = V4L2_PIX_FMT_VYUY,
+ .depth = 16,
+ .colorspace = V4L2_COLORSPACE_JPEG,
+},
+{
+ .name = RGB565X,
+ .fourcc = V4L2_PIX_FMT_RGB565X,
Hm, so, do we keep RGB565X here or do we add a BGR565? We'll anyway have
to do that when converting to imagebus, so, better do it right straight
away, to avoid having to modify user-space apps.
I changed this, but the RGB can still possibly be broken (can be fixed
later).
Well, no, sorry, I do not think it's a good idea to commit code, that we
believe does not work. Then, please, remove RGB codes and add a TODO comment.
The yuv works fine (and with the register tweak I did it's even more standard-
conformant).
Ok.
+/* read a register */
+static int ov9640_reg_read(struct i2c_client *client, u8 reg, u8 *val)
+{
+ int ret;
+ u8 data = reg;
+ struct i2c_msg msg = {
+ .addr = client-addr,
+ .flags = 0,
+ .len= 1,
+ .buf= data,
+ };
+
+ ret = i2c_transfer(client-adapter, msg, 1);
Are there any advantages in using raw i2c operations over smbus calls? The
latter look much simpler, cf., e.g., mt9m001:
static int reg_read(struct i2c_client *client, const u8 reg)
{
s32 data = i2c_smbus_read_word_data(client, reg);
return data 0 ? data : swab16(data);
}
static int reg_write(struct i2c_client *client, const u8 reg,
const u16 data)
{
return i2c_smbus_write_word_data(client, reg, swab16(data));
}
Ok, going through smbus layer takes a bit of time, but that's just done
during configuration.
Yes, the sensor doesnt work with SMBUS calls, that's why those are there. It
took me a while to figure it out.
Ok, then we should remove the check for SMBUS in probing.
+/* program default register values */
+static int ov9640_prog_dflt(struct i2c_client *client)
+{
+ int i, ret;
+
+ for (i = 0; i ARRAY_SIZE(ov9640_regs_dflt); i++) {
+ ret = ov9640_reg_write(client, ov9640_regs_dflt[i].reg,
+ ov9640_regs_dflt[i].val);
+ if (ret)
+ return ret;
+ }
+
+ /* wait for the changes to actually happen */
+ mdelay(150);
I still think that's a lot. Have you tried any lower values? I would try
50ms, if that works, try 10ms...
You are free to try, I'm not doing anything about this. It doesnt work with
lower values, period.
So, you did try it? Ok, then at least add a comment specifying whichlargest
value didn't work for you.
+ /*
+ * We must have a parent by now. And it cannot be a wrong one.
+ * So this entire test is completely redundant.
+ */
+ if (!icd-dev.parent ||
+ to_soc_camera_host(icd-dev.parent)-nr != icd-iface) {
+ dev_err(icd-dev, Parent missing or invalid!\n);
Please, see
http://git.kernel.org/?p=linux/kernel/git/next/linux-next.git;a=commitdiff;
h=d680a88e84792f84310042919065c90c5eb87423
Please adjust your dev_{dbg,err,warn,info} calls according to that patch.
+/*
+ * i2c_driver function
+ */
+static int ov9640_probe(struct i2c_client *client,
+ const struct i2c_device_id *did)
+{
+ struct ov9640_priv *priv;
+ struct i2c_adapter *adapter = to_i2c_adapter(client-dev.parent);
+ struct soc_camera_device *icd = client-dev.platform_data;
+ struct soc_camera_link *icl;
+ int ret;
+
+ if (!icd) {
+ dev_err(client-dev, Missing soc-camera data!\n);
+ return -EINVAL;
+ }
+
+ icl = to_soc_camera_link(icd);
+ if (!icl) {
+ dev_err(client-dev, Missing platform_data for driver\n);
+ return -EINVAL;
+ }
+
+ if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA)) {
You're not using smbus calls, so, don't need this check. Or switch to
using them:-)
And remove this.
Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] Add driver for OmniVision OV9640 sensor
Dne Po 14. září 2009 16:45:50 Guennadi Liakhovetski napsal(a): Ok, you were faster than I:-) If you agree, I can just remove those two RGB formats myself, changing your comment to a TODO, We can #ifdef them so others dont have to re-add them by hand if they want to try fixing those. and modify the comment next to msleep(150) 140 didn't (if you could tell me what value didn't work, that would be appreciated) and push it out. Thanks Guennadi Cheers --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] Add driver for OmniVision OV9640 sensor
Dne Ne 13. září 2009 21:13:32 Guennadi Liakhovetski napsal(a): On Sun, 13 Sep 2009, Marek Vasut wrote: Dne Pá 11. záÅà 2009 23:51:44 Guennadi Liakhovetski napsal(a): Ok, a couple more simple questions / remarks, In principle, there's just one principle objection - if we agree, that the correct format is BGR565 and RGB565X, then we should change that. There's no BGR565 format currently in the kernel, so, we'd have to add it (and the documentation for the mercurial tree). I dont think I understand you at all. Your patch used RGB565X, which is the same as RGB565 but with _bytes_ swapped, whereas earlier you confirmed, that it's not a byte-swapped RGB565 but rather R and B _colours_ are swapped: http://marc.info/?l=linux-arm-kernelm=125220918005429w=2 , i.e., it is a BGR565. Now it looks like you don't change anything in your RGB processing code, but you declare it as plain RGB555 and RGB565 codes. So, are these really the normal unswapped formats or am I missing something? And you replaced VYUY with UYVY while also modifying register configuration, so, I hope, this has settled now and your current configuration works properly with the unmodified pxa270 for you, right? Oh, damn, I see now, I put my signature above the patch, so, you didn't look below it, and there were a couple more comments there:-( Sorry! All of them should be pretty easy to fix, so, please have a look at them. Inlined is a new version of the patch (I did some lookup through the datasheet). I might not need the BGR formats for now. btw. weren't you planning to merge the ov96xx drivers into .31? I havent seen any of them there. Which ov96xx drivers? Do you mean the ov9655 driver from Stefan Herbrechtsmeier? My doubt with the latter one was (and still is), that we already have two drivers in the mainline (gspca/sn9c20x.c and gspca/ov534.c) that seem to claim support for that chip, so, I wanted to see, if we could reuse them. There's also an ov7670 from Jonathan Cameron with the same issue. And for that merge we have to come closer to v4l2-subdev, which is happening just now. Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ Here's a new patch ... YUV works, RGB might be still broken (but that can be fixed later when imagebus arrives). From: Marek Vasut marek.va...@gmail.com Date: Sat, 22 Aug 2009 05:14:23 +0200 Subject: [PATCH] Add driver for OmniVision OV9640 sensor Signed-off-by: Marek Vasut marek.va...@gmail.com --- drivers/media/video/Kconfig |6 + drivers/media/video/Makefile|1 + drivers/media/video/ov9640.c| 812 +++ drivers/media/video/ov9640.h| 209 ++ include/media/v4l2-chip-ident.h |1 + 5 files changed, 1029 insertions(+), 0 deletions(-) create mode 100644 drivers/media/video/ov9640.c create mode 100644 drivers/media/video/ov9640.h diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig index 84b6fc1..fddd654 100644 --- a/drivers/media/video/Kconfig +++ b/drivers/media/video/Kconfig @@ -772,6 +772,12 @@ config SOC_CAMERA_OV772X help This is a ov772x camera driver +config SOC_CAMERA_OV9640 + tristate ov9640 camera support + depends on SOC_CAMERA I2C + help + This is a ov9640 camera driver + config MX1_VIDEO bool diff --git a/drivers/media/video/Makefile b/drivers/media/video/Makefile index 9f2e321..e18efd5 100644 --- a/drivers/media/video/Makefile +++ b/drivers/media/video/Makefile @@ -76,6 +76,7 @@ obj-$(CONFIG_SOC_CAMERA_MT9M111) += mt9m111.o obj-$(CONFIG_SOC_CAMERA_MT9T031) += mt9t031.o obj-$(CONFIG_SOC_CAMERA_MT9V022) += mt9v022.o obj-$(CONFIG_SOC_CAMERA_OV772X)+= ov772x.o +obj-$(CONFIG_SOC_CAMERA_OV9640)+= ov9640.o obj-$(CONFIG_SOC_CAMERA_TW9910)+= tw9910.o # And now the v4l2 drivers: diff --git a/drivers/media/video/ov9640.c b/drivers/media/video/ov9640.c new file mode 100644 index 000..a651d2b --- /dev/null +++ b/drivers/media/video/ov9640.c @@ -0,0 +1,812 @@ +/* + * OmniVision OV96xx Camera Driver + * + * Copyright (C) 2009 Marek Vasut marek.va...@gmail.com + * + * Based on ov772x camera driver: + * + * Copyright (C) 2008 Renesas Solutions Corp. + * Kuninori Morimoto morimoto.kunin...@renesas.com + * + * Based on ov7670 and soc_camera_platform driver, + * + * Copyright 2006-7 Jonathan Corbet cor...@lwn.net + * Copyright (C) 2008 Magnus Damm + * Copyright (C) 2008, Guennadi Liakhovetski ker...@pengutronix.de + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + */ + +#include linux/init.h +#include linux/module.h +#include linux/i2c.h +#include linux/slab.h +#include linux/delay.h +#include linux/videodev2.h
Re: [PATCH 2/3] Add driver for OmniVision OV9640 sensor
Ok, you were faster than I:-) If you agree, I can just remove those two RGB formats myself, changing your comment to a TODO, and modify the comment next to msleep(150) (if you could tell me what value didn't work, that would be appreciated) and push it out. Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] Add driver for OmniVision OV9640 sensor
Dne Po 14. září 2009 16:35:24 Marek Vasut napsal(a): Dne Ne 13. září 2009 21:13:32 Guennadi Liakhovetski napsal(a): On Sun, 13 Sep 2009, Marek Vasut wrote: Dne Pá 11. záÅà 2009 23:51:44 Guennadi Liakhovetski napsal(a): Ok, a couple more simple questions / remarks, In principle, there's just one principle objection - if we agree, that the correct format is BGR565 and RGB565X, then we should change that. There's no BGR565 format currently in the kernel, so, we'd have to add it (and the documentation for the mercurial tree). I dont think I understand you at all. Your patch used RGB565X, which is the same as RGB565 but with _bytes_ swapped, whereas earlier you confirmed, that it's not a byte-swapped RGB565 but rather R and B _colours_ are swapped: http://marc.info/?l=linux-arm-kernelm=125220918005429w=2 , i.e., it is a BGR565. Now it looks like you don't change anything in your RGB processing code, but you declare it as plain RGB555 and RGB565 codes. So, are these really the normal unswapped formats or am I missing something? And you replaced VYUY with UYVY while also modifying register configuration, so, I hope, this has settled now and your current configuration works properly with the unmodified pxa270 for you, right? Oh, damn, I see now, I put my signature above the patch, so, you didn't look below it, and there were a couple more comments there:-( Sorry! All of them should be pretty easy to fix, so, please have a look at them. Inlined is a new version of the patch (I did some lookup through the datasheet). I might not need the BGR formats for now. btw. weren't you planning to merge the ov96xx drivers into .31? I havent seen any of them there. Which ov96xx drivers? Do you mean the ov9655 driver from Stefan Herbrechtsmeier? My doubt with the latter one was (and still is), that we already have two drivers in the mainline (gspca/sn9c20x.c and gspca/ov534.c) that seem to claim support for that chip, so, I wanted to see, if we could reuse them. There's also an ov7670 from Jonathan Cameron with the same issue. And for that merge we have to come closer to v4l2-subdev, which is happening just now. Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ Here's a new patch ... YUV works, RGB might be still broken (but that can be fixed later when imagebus arrives). Tab fixed... From: Marek Vasut marek.va...@gmail.com Date: Sat, 22 Aug 2009 05:14:23 +0200 Subject: [PATCH] Add driver for OmniVision OV9640 sensor Signed-off-by: Marek Vasut marek.va...@gmail.com --- drivers/media/video/Kconfig |6 + drivers/media/video/Makefile|1 + drivers/media/video/ov9640.c| 812 +++ drivers/media/video/ov9640.h| 209 ++ include/media/v4l2-chip-ident.h |1 + 5 files changed, 1029 insertions(+), 0 deletions(-) create mode 100644 drivers/media/video/ov9640.c create mode 100644 drivers/media/video/ov9640.h diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig index 84b6fc1..fddd654 100644 --- a/drivers/media/video/Kconfig +++ b/drivers/media/video/Kconfig @@ -772,6 +772,12 @@ config SOC_CAMERA_OV772X help This is a ov772x camera driver +config SOC_CAMERA_OV9640 + tristate ov9640 camera support + depends on SOC_CAMERA I2C + help + This is a ov9640 camera driver + config MX1_VIDEO bool diff --git a/drivers/media/video/Makefile b/drivers/media/video/Makefile index 9f2e321..e18efd5 100644 --- a/drivers/media/video/Makefile +++ b/drivers/media/video/Makefile @@ -76,6 +76,7 @@ obj-$(CONFIG_SOC_CAMERA_MT9M111) += mt9m111.o obj-$(CONFIG_SOC_CAMERA_MT9T031) += mt9t031.o obj-$(CONFIG_SOC_CAMERA_MT9V022) += mt9v022.o obj-$(CONFIG_SOC_CAMERA_OV772X)+= ov772x.o +obj-$(CONFIG_SOC_CAMERA_OV9640)+= ov9640.o obj-$(CONFIG_SOC_CAMERA_TW9910)+= tw9910.o # And now the v4l2 drivers: diff --git a/drivers/media/video/ov9640.c b/drivers/media/video/ov9640.c new file mode 100644 index 000..a651d2b --- /dev/null +++ b/drivers/media/video/ov9640.c @@ -0,0 +1,812 @@ +/* + * OmniVision OV96xx Camera Driver + * + * Copyright (C) 2009 Marek Vasut marek.va...@gmail.com + * + * Based on ov772x camera driver: + * + * Copyright (C) 2008 Renesas Solutions Corp. + * Kuninori Morimoto morimoto.kunin...@renesas.com + * + * Based on ov7670 and soc_camera_platform driver, + * + * Copyright 2006-7 Jonathan Corbet cor...@lwn.net + * Copyright (C) 2008 Magnus Damm + * Copyright (C) 2008, Guennadi Liakhovetski ker...@pengutronix.de + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + */ + +#include linux/init.h +#include
Re: [PATCH 2/3] Add driver for OmniVision OV9640 sensor
Dne Pá 11. září 2009 23:51:44 Guennadi Liakhovetski napsal(a): On Fri, 4 Sep 2009, Marek Vasut wrote: Dne Po 24. srpna 2009 20:06:29 Guennadi Liakhovetski napsal(a): Hi Marek On Sat, 22 Aug 2009, Marek Vasut wrote: From 479aafc9a6540efec8a691a84aff166eb0218a72 Mon Sep 17 00:00:00 2001 From: Marek Vasut marek.va...@gmail.com Date: Sat, 22 Aug 2009 05:14:23 +0200 Subject: [PATCH 2/3] Add driver for OmniVision OV9640 sensor Signed-off-by: Marek Vasut marek.va...@gmail.com Ok, I see, you rebased your patches on my soc-camera patch-stack, this is good, thanks. But you missed a couple of my comments - you still have a few static functions in the ov9640.c marked inline: ov9640_set_crop() is not used at all, ov9640_set_bus_param() gets assigned to a struct member, so, cannot be inline. ov9640_alter_regs() is indeed only called at one location, but see Chapter 15 in Documentation/CodingStyle. You left at least one multi-line comment wrongly formatted. You left some broken printk format lines, etc. You moved your header under drivers/... - that's good. But, please, address all my comments that I provided in a private review, or explain why you do not agree. Otherwise I feel like I wasted my time. Besides, your mailer has wrapped lines. Please, read Documentation/email-clients.txt on how to configure your email client to send patches properly. In the worst case, you can inline your patches while reviewing, and then attach them for a final submission. This is, however, discouraged, because it makes review and test of your intermediate patches with wrapped lines more difficult. Also, please check your patches with scripts/checkpatch.pl. Fixed patch follows, my mailer is fixed as much as possible (working on getting git-email to work, but that's still to be done). Please consider applying, thanks. Ok, a couple more simple questions / remarks, In principle, there's just one principle objection - if we agree, that the correct format is BGR565 and RGB565X, then we should change that. There's no BGR565 format currently in the kernel, so, we'd have to add it (and the documentation for the mercurial tree). I dont think I understand you at all. Thanks Guennadi Inlined is a new version of the patch (I did some lookup through the datasheet). I might not need the BGR formats for now. btw. weren't you planning to merge the ov96xx drivers into .31? I havent seen any of them there. Cheers! From: Marek Vasut marek.va...@gmail.com Date: Sat, 22 Aug 2009 05:14:23 +0200 Subject: [PATCH] Add driver for OmniVision OV9640 sensor Signed-off-by: Marek Vasut marek.va...@gmail.com --- drivers/media/video/Kconfig |6 + drivers/media/video/Makefile|1 + drivers/media/video/ov9640.c| 818 +++ drivers/media/video/ov9640.h| 209 ++ include/media/v4l2-chip-ident.h |1 + 5 files changed, 1035 insertions(+), 0 deletions(-) create mode 100644 drivers/media/video/ov9640.c create mode 100644 drivers/media/video/ov9640.h diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig index 84b6fc1..fddd654 100644 --- a/drivers/media/video/Kconfig +++ b/drivers/media/video/Kconfig @@ -772,6 +772,12 @@ config SOC_CAMERA_OV772X help This is a ov772x camera driver +config SOC_CAMERA_OV9640 + tristate ov9640 camera support + depends on SOC_CAMERA I2C + help + This is a ov9640 camera driver + config MX1_VIDEO bool diff --git a/drivers/media/video/Makefile b/drivers/media/video/Makefile index 9f2e321..e18efd5 100644 --- a/drivers/media/video/Makefile +++ b/drivers/media/video/Makefile @@ -76,6 +76,7 @@ obj-$(CONFIG_SOC_CAMERA_MT9M111) += mt9m111.o obj-$(CONFIG_SOC_CAMERA_MT9T031) += mt9t031.o obj-$(CONFIG_SOC_CAMERA_MT9V022) += mt9v022.o obj-$(CONFIG_SOC_CAMERA_OV772X)+= ov772x.o +obj-$(CONFIG_SOC_CAMERA_OV9640)+= ov9640.o obj-$(CONFIG_SOC_CAMERA_TW9910)+= tw9910.o # And now the v4l2 drivers: diff --git a/drivers/media/video/ov9640.c b/drivers/media/video/ov9640.c new file mode 100644 index 000..0701247 --- /dev/null +++ b/drivers/media/video/ov9640.c @@ -0,0 +1,818 @@ +/* + * OmniVision OV96xx Camera Driver + * + * Copyright (C) 2009 Marek Vasut marek.va...@gmail.com + * + * Based on ov772x camera driver: + * + * Copyright (C) 2008 Renesas Solutions Corp. + * Kuninori Morimoto morimoto.kunin...@renesas.com + * + * Based on ov7670 and soc_camera_platform driver, + * + * Copyright 2006-7 Jonathan Corbet cor...@lwn.net + * Copyright (C) 2008 Magnus Damm + * Copyright (C) 2008, Guennadi Liakhovetski ker...@pengutronix.de + * + * 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
Re: [PATCH 2/3] Add driver for OmniVision OV9640 sensor
On Sun, 13 Sep 2009, Marek Vasut wrote: Dne Pá 11. záÅà 2009 23:51:44 Guennadi Liakhovetski napsal(a): Ok, a couple more simple questions / remarks, In principle, there's just one principle objection - if we agree, that the correct format is BGR565 and RGB565X, then we should change that. There's no BGR565 format currently in the kernel, so, we'd have to add it (and the documentation for the mercurial tree). I dont think I understand you at all. Your patch used RGB565X, which is the same as RGB565 but with _bytes_ swapped, whereas earlier you confirmed, that it's not a byte-swapped RGB565 but rather R and B _colours_ are swapped: http://marc.info/?l=linux-arm-kernelm=125220918005429w=2 , i.e., it is a BGR565. Now it looks like you don't change anything in your RGB processing code, but you declare it as plain RGB555 and RGB565 codes. So, are these really the normal unswapped formats or am I missing something? And you replaced VYUY with UYVY while also modifying register configuration, so, I hope, this has settled now and your current configuration works properly with the unmodified pxa270 for you, right? Oh, damn, I see now, I put my signature above the patch, so, you didn't look below it, and there were a couple more comments there:-( Sorry! All of them should be pretty easy to fix, so, please have a look at them. Inlined is a new version of the patch (I did some lookup through the datasheet). I might not need the BGR formats for now. btw. weren't you planning to merge the ov96xx drivers into .31? I havent seen any of them there. Which ov96xx drivers? Do you mean the ov9655 driver from Stefan Herbrechtsmeier? My doubt with the latter one was (and still is), that we already have two drivers in the mainline (gspca/sn9c20x.c and gspca/ov534.c) that seem to claim support for that chip, so, I wanted to see, if we could reuse them. There's also an ov7670 from Jonathan Cameron with the same issue. And for that merge we have to come closer to v4l2-subdev, which is happening just now. Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] Add driver for OmniVision OV9640 sensor
On Fri, 4 Sep 2009, Marek Vasut wrote:
Dne Po 24. srpna 2009 20:06:29 Guennadi Liakhovetski napsal(a):
Hi Marek
On Sat, 22 Aug 2009, Marek Vasut wrote:
From 479aafc9a6540efec8a691a84aff166eb0218a72 Mon Sep 17 00:00:00 2001
From: Marek Vasut marek.va...@gmail.com
Date: Sat, 22 Aug 2009 05:14:23 +0200
Subject: [PATCH 2/3] Add driver for OmniVision OV9640 sensor
Signed-off-by: Marek Vasut marek.va...@gmail.com
Ok, I see, you rebased your patches on my soc-camera patch-stack, this is
good, thanks. But you missed a couple of my comments - you still have a
few static functions in the ov9640.c marked inline: ov9640_set_crop() is
not used at all, ov9640_set_bus_param() gets assigned to a struct member,
so, cannot be inline. ov9640_alter_regs() is indeed only called at one
location, but see Chapter 15 in Documentation/CodingStyle. You left at
least one multi-line comment wrongly formatted. You left some broken
printk format lines, etc. You moved your header under drivers/... - that's
good. But, please, address all my comments that I provided in a private
review, or explain why you do not agree. Otherwise I feel like I wasted my
time. Besides, your mailer has wrapped lines. Please, read
Documentation/email-clients.txt on how to configure your email client to
send patches properly. In the worst case, you can inline your patches
while reviewing, and then attach them for a final submission. This is,
however, discouraged, because it makes review and test of your
intermediate patches with wrapped lines more difficult. Also, please check
your patches with scripts/checkpatch.pl.
Fixed patch follows, my mailer is fixed as much as possible (working on
getting
git-email to work, but that's still to be done). Please consider applying,
thanks.
Ok, a couple more simple questions / remarks, In principle, there's just
one principle objection - if we agree, that the correct format is BGR565
and RGB565X, then we should change that. There's no BGR565 format
currently in the kernel, so, we'd have to add it (and the documentation
for the mercurial tree).
Thanks
Guennadi
Add driver for OmniVision OV9640 sensor
Signed-off-by: Marek Vasut marek.va...@gmail.com
---
drivers/media/video/Kconfig |6 +
drivers/media/video/Makefile|1 +
drivers/media/video/ov9640.c| 811
+++
drivers/media/video/ov9640.h| 206 ++
include/media/v4l2-chip-ident.h |1 +
5 files changed, 1025 insertions(+), 0 deletions(-)
create mode 100644 drivers/media/video/ov9640.c
create mode 100644 drivers/media/video/ov9640.h
diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig
index 84b6fc1..fddd654 100644
--- a/drivers/media/video/Kconfig
+++ b/drivers/media/video/Kconfig
@@ -772,6 +772,12 @@ config SOC_CAMERA_OV772X
help
This is a ov772x camera driver
+config SOC_CAMERA_OV9640
+ tristate ov9640 camera support
+ depends on SOC_CAMERA I2C
+ help
+ This is a ov9640 camera driver
+
config MX1_VIDEO
bool
diff --git a/drivers/media/video/Makefile b/drivers/media/video/Makefile
index 9f2e321..e18efd5 100644
--- a/drivers/media/video/Makefile
+++ b/drivers/media/video/Makefile
@@ -76,6 +76,7 @@ obj-$(CONFIG_SOC_CAMERA_MT9M111)+= mt9m111.o
obj-$(CONFIG_SOC_CAMERA_MT9T031) += mt9t031.o
obj-$(CONFIG_SOC_CAMERA_MT9V022) += mt9v022.o
obj-$(CONFIG_SOC_CAMERA_OV772X) += ov772x.o
+obj-$(CONFIG_SOC_CAMERA_OV9640) += ov9640.o
obj-$(CONFIG_SOC_CAMERA_TW9910) += tw9910.o
# And now the v4l2 drivers:
diff --git a/drivers/media/video/ov9640.c b/drivers/media/video/ov9640.c
new file mode 100644
index 000..289e82d
--- /dev/null
+++ b/drivers/media/video/ov9640.c
@@ -0,0 +1,811 @@
+/*
+ * OmniVision OV96xx Camera Driver
+ *
+ * Copyright (C) 2009 Marek Vasut marek.va...@gmail.com
+ *
+ * Based on ov772x camera driver:
+ *
+ * Copyright (C) 2008 Renesas Solutions Corp.
+ * Kuninori Morimoto morimoto.kunin...@renesas.com
+ *
+ * Based on ov7670 and soc_camera_platform driver,
+ *
+ * Copyright 2006-7 Jonathan Corbet cor...@lwn.net
+ * Copyright (C) 2008 Magnus Damm
+ * Copyright (C) 2008, Guennadi Liakhovetski ker...@pengutronix.de
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include linux/init.h
+#include linux/module.h
+#include linux/i2c.h
+#include linux/slab.h
+#include linux/delay.h
+#include linux/videodev2.h
+#include media/v4l2-chip-ident.h
+#include media/v4l2-common.h
+#include media/soc_camera.h
+
+#include ov9640.h
+
+/* default register setup */
+static const struct ov9640_reg ov9640_regs_dflt[] = {
+ { OV9640_COM5, OV9640_COM5_SYSCLK | OV9640_COM5_LONGEXP },
+
Re: [PATCH 2/3] Add driver for OmniVision OV9640 sensor
Dne Po 24. srpna 2009 20:06:29 Guennadi Liakhovetski napsal(a):
Hi Marek
On Sat, 22 Aug 2009, Marek Vasut wrote:
From 479aafc9a6540efec8a691a84aff166eb0218a72 Mon Sep 17 00:00:00 2001
From: Marek Vasut marek.va...@gmail.com
Date: Sat, 22 Aug 2009 05:14:23 +0200
Subject: [PATCH 2/3] Add driver for OmniVision OV9640 sensor
Signed-off-by: Marek Vasut marek.va...@gmail.com
Ok, I see, you rebased your patches on my soc-camera patch-stack, this is
good, thanks. But you missed a couple of my comments - you still have a
few static functions in the ov9640.c marked inline: ov9640_set_crop() is
not used at all, ov9640_set_bus_param() gets assigned to a struct member,
so, cannot be inline. ov9640_alter_regs() is indeed only called at one
location, but see Chapter 15 in Documentation/CodingStyle. You left at
least one multi-line comment wrongly formatted. You left some broken
printk format lines, etc. You moved your header under drivers/... - that's
good. But, please, address all my comments that I provided in a private
review, or explain why you do not agree. Otherwise I feel like I wasted my
time. Besides, your mailer has wrapped lines. Please, read
Documentation/email-clients.txt on how to configure your email client to
send patches properly. In the worst case, you can inline your patches
while reviewing, and then attach them for a final submission. This is,
however, discouraged, because it makes review and test of your
intermediate patches with wrapped lines more difficult. Also, please check
your patches with scripts/checkpatch.pl.
Fixed patch follows, my mailer is fixed as much as possible (working on getting
git-email to work, but that's still to be done). Please consider applying,
thanks.
Add driver for OmniVision OV9640 sensor
Signed-off-by: Marek Vasut marek.va...@gmail.com
---
drivers/media/video/Kconfig |6 +
drivers/media/video/Makefile|1 +
drivers/media/video/ov9640.c| 811 +++
drivers/media/video/ov9640.h| 206 ++
include/media/v4l2-chip-ident.h |1 +
5 files changed, 1025 insertions(+), 0 deletions(-)
create mode 100644 drivers/media/video/ov9640.c
create mode 100644 drivers/media/video/ov9640.h
diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig
index 84b6fc1..fddd654 100644
--- a/drivers/media/video/Kconfig
+++ b/drivers/media/video/Kconfig
@@ -772,6 +772,12 @@ config SOC_CAMERA_OV772X
help
This is a ov772x camera driver
+config SOC_CAMERA_OV9640
+ tristate ov9640 camera support
+ depends on SOC_CAMERA I2C
+ help
+ This is a ov9640 camera driver
+
config MX1_VIDEO
bool
diff --git a/drivers/media/video/Makefile b/drivers/media/video/Makefile
index 9f2e321..e18efd5 100644
--- a/drivers/media/video/Makefile
+++ b/drivers/media/video/Makefile
@@ -76,6 +76,7 @@ obj-$(CONFIG_SOC_CAMERA_MT9M111) += mt9m111.o
obj-$(CONFIG_SOC_CAMERA_MT9T031) += mt9t031.o
obj-$(CONFIG_SOC_CAMERA_MT9V022) += mt9v022.o
obj-$(CONFIG_SOC_CAMERA_OV772X)+= ov772x.o
+obj-$(CONFIG_SOC_CAMERA_OV9640)+= ov9640.o
obj-$(CONFIG_SOC_CAMERA_TW9910)+= tw9910.o
# And now the v4l2 drivers:
diff --git a/drivers/media/video/ov9640.c b/drivers/media/video/ov9640.c
new file mode 100644
index 000..289e82d
--- /dev/null
+++ b/drivers/media/video/ov9640.c
@@ -0,0 +1,811 @@
+/*
+ * OmniVision OV96xx Camera Driver
+ *
+ * Copyright (C) 2009 Marek Vasut marek.va...@gmail.com
+ *
+ * Based on ov772x camera driver:
+ *
+ * Copyright (C) 2008 Renesas Solutions Corp.
+ * Kuninori Morimoto morimoto.kunin...@renesas.com
+ *
+ * Based on ov7670 and soc_camera_platform driver,
+ *
+ * Copyright 2006-7 Jonathan Corbet cor...@lwn.net
+ * Copyright (C) 2008 Magnus Damm
+ * Copyright (C) 2008, Guennadi Liakhovetski ker...@pengutronix.de
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include linux/init.h
+#include linux/module.h
+#include linux/i2c.h
+#include linux/slab.h
+#include linux/delay.h
+#include linux/videodev2.h
+#include media/v4l2-chip-ident.h
+#include media/v4l2-common.h
+#include media/soc_camera.h
+
+#include ov9640.h
+
+/* default register setup */
+static const struct ov9640_reg ov9640_regs_dflt[] = {
+ { OV9640_COM5, OV9640_COM5_SYSCLK | OV9640_COM5_LONGEXP },
+ { OV9640_COM6, OV9640_COM6_OPT_BLC | OV9640_COM6_ADBLC_BIAS |
+ OV9640_COM6_FMT_RST | OV9640_COM6_ADBLC_OPTEN },
+ { OV9640_PSHFT, OV9640_PSHFT_VAL(0x01) },
+ { OV9640_ACOM, OV9640_ACOM_2X_ANALOG | OV9640_ACOM_RSVD },
+ { OV9640_COM16, OV9640_COM16_RB_AVG },
+
+ /* Gamma curve P */
+ { 0x6c, 0x40 }, { 0x6d, 0x30 }, { 0x6e, 0x4b }, { 0x6f, 0x60 },
+ { 0x70, 0x70 }, { 0x71, 0x70 }, { 0x72, 0x70 }, {
Re: [PATCH 2/3] Add driver for OmniVision OV9640 sensor
Hi Marek On Sat, 22 Aug 2009, Marek Vasut wrote: From 479aafc9a6540efec8a691a84aff166eb0218a72 Mon Sep 17 00:00:00 2001 From: Marek Vasut marek.va...@gmail.com Date: Sat, 22 Aug 2009 05:14:23 +0200 Subject: [PATCH 2/3] Add driver for OmniVision OV9640 sensor Signed-off-by: Marek Vasut marek.va...@gmail.com Ok, I see, you rebased your patches on my soc-camera patch-stack, this is good, thanks. But you missed a couple of my comments - you still have a few static functions in the ov9640.c marked inline: ov9640_set_crop() is not used at all, ov9640_set_bus_param() gets assigned to a struct member, so, cannot be inline. ov9640_alter_regs() is indeed only called at one location, but see Chapter 15 in Documentation/CodingStyle. You left at least one multi-line comment wrongly formatted. You left some broken printk format lines, etc. You moved your header under drivers/... - that's good. But, please, address all my comments that I provided in a private review, or explain why you do not agree. Otherwise I feel like I wasted my time. Besides, your mailer has wrapped lines. Please, read Documentation/email-clients.txt on how to configure your email client to send patches properly. In the worst case, you can inline your patches while reviewing, and then attach them for a final submission. This is, however, discouraged, because it makes review and test of your intermediate patches with wrapped lines more difficult. Also, please check your patches with scripts/checkpatch.pl. [patch skipped] Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
