OV5647 Sensor driver
Subsystems (4)
v3 cover OV5647 Sensor driver
Hello,
This patch adds support for the Omnivision OV5647 sensor.
At the moment it only supports 640x480 in Raw 8.
v3: Re-sending after no reply to previous patch.
Ramiro Oliveira (2):
Add OV5647 device tree documentation
Add support for Omnivision OV5647
.../devicetree/bindings/media/i2c/ov5647.txt | 19 + MAINTAINERS | 7 + drivers/media/i2c/Kconfig | 12 + drivers/media/i2c/Makefile | 1 + drivers/media/i2c/ov5647.c | 891 +++++++++++++++++++++ 5 files changed, 930 insertions(+) create mode 100644 Documentation/devicetree/bindings/media/i2c/ov5647.txt create mode 100644 drivers/media/i2c/ov5647.c
Signature · 1 line
2.9.3
PATCH v3 1/2 Add OV5647 device tree documentation
--- .../devicetree/bindings/media/i2c/ov5647.txt | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) create mode 100644 Documentation/devicetree/bindings/media/i2c/ov5647.txt
Documentation/devicetree/bindings/media/i2c/ov5647.txt new file +19 -0
@@ -0,0 +1,19 @@ +Omnivision OV5647 raw image sensor +--------------------------------- + +OV5647 is a raw image sensor with MIPI CSI-2 and CCP2 image data interfaces +and CCI (I2C compatible) control bus. + +Required properties: + +- compatible : "ovti,ov5647"; +- reg : I2C slave address of the sensor; + +The common video interfaces bindings (see video-interfaces.txt) should be +used to specify link to the image data receiver. The OV5647 device +node should contain one 'port' child node with an 'endpoint' subnode. + +Following properties are valid for the endpoint node: + +- data-lanes : (optional) specifies MIPI CSI-2 data lanes as covered in + video-interfaces.txt. The sensor supports only two data lanes.
Signature · 1 line
2.9.3
Rob Herring
maintainerRe: [PATCH v3 1/2] Add OV5647 device tree documentation
Quoted 21 lines · Ramiro Oliveira
Signed-off-by Ramiro Oliveira <[email protected]>--- .../devicetree/bindings/media/i2c/ov5647.txt | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) create mode 100644 Documentation/devicetree/bindings/media/i2c/ov5647.txtDocumentation/devicetree/bindings/media/i2c/ov5647.txt new file +9
-0@@ -0,0 +1,19 @@ +Omnivision OV5647 raw image sensor +--------------------------------- + +OV5647 is a raw image sensor with MIPI CSI-2 and CCP2 image data interfaces +and CCI (I2C compatible) control bus. + +Required properties: + +- compatible : "ovti,ov5647";
Documentation/devicetree/bindings/media/i2c/ov5647.txt line 19
Quoted 10 lines
Documentation/devicetree/bindings/media/i2c/ov5647.txt new file +10
-0+- reg : I2C slave address of the sensor; + +The common video interfaces bindings (see video-interfaces.txt) should be +used to specify link to the image data receiver. The OV5647 device +node should contain one 'port' child node with an 'endpoint' subnode. + +Following properties are valid for the endpoint node: + +- data-lanes : (optional) specifies MIPI CSI-2 data lanes as covered in + video-interfaces.txt. The sensor supports only two data lanes.
What's the default if not present?
Quoted 4 lines
Signature · 1 line
2.9.3
Re: [PATCH v3 1/2] Add OV5647 device tree documentation
Quoted 25 lines · Rob Herring
Quoted 24 lines · Ramiro Oliveira
Signed-off-by Ramiro Oliveira <[email protected]>--- .../devicetree/bindings/media/i2c/ov5647.txt | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) create mode 100644 Documentation/devicetree/bindings/media/i2c/ov5647.txtDocumentation/devicetree/bindings/media/i2c/ov5647.txt new file +12
-0@@ -0,0 +1,19 @@ +Omnivision OV5647 raw image sensor +--------------------------------- + +OV5647 is a raw image sensor with MIPI CSI-2 and CCP2 image data interfaces +and CCI (I2C compatible) control bus. + +Required properties: + +- compatible : "ovti,ov5647"; +- reg : I2C slave address of the sensor; + +The common video interfaces bindings (see video-interfaces.txt) should be
Quoted 8 lines
Documentation/devicetree/bindings/media/i2c/ov5647.txt line 19
Quoted 7 lines
Documentation/devicetree/bindings/media/i2c/ov5647.txt new file +7
-0+used to specify link to the image data receiver. The OV5647 device +node should contain one 'port' child node with an 'endpoint' subnode. + +Following properties are valid for the endpoint node: + +- data-lanes : (optional) specifies MIPI CSI-2 data lanes as covered in + video-interfaces.txt. The sensor supports only two data lanes.What's the default if not present?
Since the sensor only supports 2 data lanes, this information will not be used, even if it is available in the DT. So the default will be 2.
Quoted 5 lines
Quoted 5 lines
Signature · 2 lines
-- 2.9.3
PATCH v3 2/2 Add support for Omnivision OV5647
In reply to Ramiro Oliveira--- MAINTAINERS | 7 + drivers/media/i2c/Kconfig | 12 + drivers/media/i2c/Makefile | 1 + drivers/media/i2c/ov5647.c | 891 +++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 911 insertions(+) create mode 100644 drivers/media/i2c/ov5647.c
MAINTAINERS +7 -0
@@ -8661,6 +8661,13 @@ M: Harald Welte <[email protected]> S: Maintained F: drivers/char/pcmcia/cm4040_cs.* +OMNIVISION OV5647 SENSOR DRIVER +M: Ramiro Oliveira <roliveir-HKixBCOQz3hWk0Htik3J/[email protected]> +L: [email protected] +T: git git://linuxtv.org/media_tree.git +S: Maintained +F: drivers/media/i2c/ov5647.c + OMNIVISION OV7670 SENSOR DRIVER M: Jonathan Corbet <[email protected]> L: [email protected]
drivers/media/i2c/Kconfig +12 -0
@@ -531,6 +531,18 @@ config VIDEO_OV2659 To compile this driver as a module, choose M here: the module will be called ov2659. +config VIDEO_OV5647 + tristate "OmniVision OV5647 sensor support" + depends on OF + depends on I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API + depends on MEDIA_CAMERA_SUPPORT + ---help--- + This is a Video4Linux2 sensor-level driver for the OmniVision + OV5647 camera. + + To compile this driver as a module, choose M here: the + module will be called ov5647. + config VIDEO_OV7640 tristate "OmniVision OV7640 sensor support" depends on I2C && VIDEO_V4L2
drivers/media/i2c/Makefile +1 -0
@@ -82,3 +82,4 @@ obj-$(CONFIG_VIDEO_IR_I2C) += ir-kbd-i2c.o obj-$(CONFIG_VIDEO_ML86V7667) += ml86v7667.o obj-$(CONFIG_VIDEO_OV2659) += ov2659.o obj-$(CONFIG_VIDEO_TC358743) += tc358743.o +obj-$(CONFIG_VIDEO_OV5647) += ov5647.o
drivers/media/i2c/ov5647.c new file +891 -0
891 lines not on this page · raw message
Signature · 7 lines
2.9.3 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 2/2] Add support for Omnivision OV5647
Hi!
Quoted 4 lines
+/*
+ * A V4L2 driver for OmniVision OV5647 cameras.
+ *
+ * Based on Samsung S5K6AAFX SXGA 1/6" 1.3M CMOS Image Sensor driver
Quoted 8 lines
+ * Copyright (C) 2011 Sylwester Nawrocki <[email protected]>
+ *
+ * Based on Omnivision OV7670 Camera Driver
+ * Copyright (C) 2006-7 Jonathan Corbet <[email protected]>
+ *
+ * Copyright (C) 2016, Synopsys, Inc.
+ *
+ */
If you do copyrights, you should also specify license.
drivers/media/i2c/ov5647.c line 28
Quoted 3 lines
drivers/media/i2c/ov5647.c new file +3
-0+static bool debug; +module_param (debug, bool,0644 ); +MODULE_PARM_DESC (debug,"Debug level (0-1)" );
Please remove, we have generic infrastructure for debugging.
Quoted 4 lines
drivers/media/i2c/ov5647.c new file +4
-0+/* +* The ov5647 sits on i2c with ID 0x6c +*/ +#define OV5647_I2C_ADDR 0x6c
drivers/media/i2c/ov5647.c line 43
Quoted 10 lines
drivers/media/i2c/ov5647.c new file +10
-0+#define SENSOR_NAME "ov5647" + +#define OV5647_REG_CHIPID_H 0x300A +#define OV5647_REG_CHIPID_L 0x300B + +#define REG_TERM 0xfffe +#define VAL_TERM 0xfe +#define REG_DLY 0xffff + +/*define the voltage level of control signal*/
"/* ", " */".
Quoted 8 lines
drivers/media/i2c/ov5647.c new file +8
-0+#define CSI_STBY_ON 1 +#define CSI_STBY_OFF 0 +#define CSI_RST_ON 0 +#define CSI_RST_OFF 1 +#define CSI_PWR_ON 1 +#define CSI_PWR_OFF 0 +#define CSI_AF_PWR_ON 1 +#define CSI_AF_PWR_OFF 0
...
drivers/media/i2c/ov5647.c line 77
Quoted 4 lines
drivers/media/i2c/ov5647.c new file +4
-0+enum power_seq_cmd { +CSI_SUBDEV_PWR_OFF =0x00 , +CSI_SUBDEV_PWR_ON =0x01 , +};
Pick one style for defines/enums?
drivers/media/i2c/ov5647.c line 82
Quoted 4 lines
drivers/media/i2c/ov5647.c new file +4
-0+struct regval_list { +uint16_t addr; +uint8_t data; +};
u8/u16?
drivers/media/i2c/ov5647.c line 239
Quoted 7 lines
drivers/media/i2c/ov5647.c new file +7
-0+/** +* @short I2C Write operation +* @param[in] i2c_client I2C client +* @param[in] reg register address +* @param[in] val value to write +* @return Error code +*/
" *"?
drivers/media/i2c/ov5647.c line 243
Quoted 4 lines
drivers/media/i2c/ov5647.c new file +4
-0+static int ov5647_write (struct v4l2_subdev *sd,uint16_t reg,uint8_t val) +{ +int ret; +unsigned char data[3 ] = { reg >>8 , reg &0xff , val};
" }".
Quoted 4 lines
drivers/media/i2c/ov5647.c new file +4
-0+static int ov5647_write_array (struct v4l2_subdev *subdev, +struct regval_list *regs,int array_size) +{ +int i =0 ;
drivers/media/i2c/ov5647.c line 301
Quoted 10 lines
drivers/media/i2c/ov5647.c new file +10
-0+int ret =0 ; + +if (!regs) +return -EINVAL ; + +while (i < array_size) { +if (regs->addr ==REG_DLY ) +mdelay (regs->data); +else + ret =ov5647_write (subdev, regs->addr, regs->data);
The "REG_DLY" is never used AFAICT? Remove?
drivers/media/i2c/ov5647.c line 305
Quoted 3 lines
drivers/media/i2c/ov5647.c new file +3
-0+if (ret == -EIO ) +return ret; +
ov5647_write() can return errors other then EIO. Are they handled correctly?
Quoted 10 lines
drivers/media/i2c/ov5647.c new file +10
-0+/** +* @short Set SW standby +* @param[in] subdev v4l2 subdev +* @param[in] on_off standby on or off +* @return Error code +*/ +static int sensor_s_sw_stby (struct v4l2_subdev *subdev,int on_off) +{ +int ret; +unsigned char rdval;
drivers/media/i2c/ov5647.c line 367
Quoted 10 lines
drivers/media/i2c/ov5647.c new file +10
-0+ + ret =ov5647_read (subdev,0x0100 , &rdval); +if (ret !=0 ) +return ret; + +if (on_off ==CSI_STBY_ON ) + ret =ov5647_write (subdev,0x0100 , rdval&0xfe ); + +else + ret =ov5647_write (subdev,0x0100 , rdval|0x01 );
I'd get rid of CSI_STBY_ON, and convert arg to bool, as you don't really handle other values. Plus, naming it "set_sw_standby()" would make core slightly more readable. Also kill the empty line before else.
drivers/media/i2c/ov5647.c line 404
Quoted 11 lines
drivers/media/i2c/ov5647.c new file +11
-0+/** +* @short Initialize sensor +* @param[in] subdev v4l2 subdev +* @param[in] val not used +* @return Error code +*/ +static int __sensor_init (struct v4l2_subdev *subdev) +{ +int ret; +uint8_t resetval; +unsigned char rdval;
u8 for both?
drivers/media/i2c/ov5647.c line 427
Quoted 4 lines
drivers/media/i2c/ov5647.c new file +4
-0+ov5647_read (subdev,0x0100 , &resetval); +if (!resetval&0x01 ) { +v4l2_dbg (1 , debug, subdev, +"DEVICE WAS IN SOFTWARE STANDBY" );
No shouting please? If it is important maybe it should have higher priority?
Quoted 10 lines
drivers/media/i2c/ov5647.c new file +10
-0+static int sensor_power (struct v4l2_subdev *subdev,int on) +{ +int ret; +struct ov5647 *ov5647 =to_state (subdev); + + ret =0 ; +mutex_lock (&ov5647->lock); + +switch (on) { +case CSI_SUBDEV_PWR_OFF :
...
Quoted 1 line
drivers/media/i2c/ov5647.c new file +1
-0+case CSI_SUBDEV_PWR_ON :
...
drivers/media/i2c/ov5647.c line 486
Quoted 5 lines
drivers/media/i2c/ov5647.c new file +5
-0+default : +return -EINVAL ; + } + +mutex_unlock (&ov5647->lock);
I'd really convert this to bool. Note how it returns with lock held?
Quoted 8 lines
drivers/media/i2c/ov5647.c new file +8
-0+int ov5647_detect (struct v4l2_subdev *sd) +{ +unsigned char v; +int ret; + + ret =sensor_power (sd,1 ); +if (ret <0 ) +return ret;
drivers/media/i2c/ov5647.c line 725
Quoted 10 lines
drivers/media/i2c/ov5647.c new file +10
-0+ ret =ov5647_read (sd,OV5647_REG_CHIPID_H , &v); +if (ret <0 ) +return ret; +if (v !=0x56 )/* OV manuf. id. */ +return -ENODEV ; + ret =ov5647_read (sd,OV5647_REG_CHIPID_L , &v); +if (ret <0 ) +return ret; +if (v !=0x47 ) +return -ENODEV ;
I guess invalid chipid deserves a printk?
Quoted 1 line
+* Refer to Linux errors.
Useful?
drivers/media/i2c/ov5647.c line 859
Quoted 4 lines
drivers/media/i2c/ov5647.c new file +4
-0+/** +* @short of_device_id structure +*/ +static const struct i2c_device_id ov5647_id[] = {
Umm. The comment looks useless and wrong.
Best regards,
Pavel
Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Re: [PATCH v3 2/2] Add support for Omnivision OV5647
Hi!
Quoted 52 lines · Pavel Machek
Hi!
Quoted 4 lines
+/*
+ * A V4L2 driver for OmniVision OV5647 cameras.
+ *
+ * Based on Samsung S5K6AAFX SXGA 1/6" 1.3M CMOS Image Sensor driverQuoted 8 lines
+ * Copyright (C) 2011 Sylwester Nawrocki <[email protected]>
+ *
+ * Based on Omnivision OV7670 Camera Driver
+ * Copyright (C) 2006-7 Jonathan Corbet <[email protected]>
+ *
+ * Copyright (C) 2016, Synopsys, Inc.
+ *
+ */If you do copyrights, you should also specify license.
drivers/media/i2c/ov5647.c line 28
Quoted 3 lines
drivers/media/i2c/ov5647.c new file +3
-0+static bool debug; +module_param (debug, bool,0644 ); +MODULE_PARM_DESC (debug,"Debug level (0-1)" );Please remove, we have generic infrastructure for debugging.
Quoted 4 lines
drivers/media/i2c/ov5647.c new file +4
-0+/* +* The ov5647 sits on i2c with ID 0x6c +*/ +#define OV5647_I2C_ADDR 0x6cdrivers/media/i2c/ov5647.c line 43
Quoted 10 lines
drivers/media/i2c/ov5647.c new file +10
-0+#define SENSOR_NAME "ov5647" + +#define OV5647_REG_CHIPID_H 0x300A +#define OV5647_REG_CHIPID_L 0x300B + +#define REG_TERM 0xfffe +#define VAL_TERM 0xfe +#define REG_DLY 0xffff + +/*define the voltage level of control signal*/ "/* ", " */".
Quoted 8 lines
drivers/media/i2c/ov5647.c new file +8
-0+#define CSI_STBY_ON 1 +#define CSI_STBY_OFF 0 +#define CSI_RST_ON 0 +#define CSI_RST_OFF 1 +#define CSI_PWR_ON 1 +#define CSI_PWR_OFF 0 +#define CSI_AF_PWR_ON 1 +#define CSI_AF_PWR_OFF 0...
drivers/media/i2c/ov5647.c line 77
Quoted 4 lines
drivers/media/i2c/ov5647.c new file +4
-0+enum power_seq_cmd { +CSI_SUBDEV_PWR_OFF =0x00 , +CSI_SUBDEV_PWR_ON =0x01 , +};Pick one style for defines/enums?
Quoted 5 lines
drivers/media/i2c/ov5647.c line 82
Quoted 4 lines
drivers/media/i2c/ov5647.c new file +4
-0+struct regval_list { +uint16_t addr; +uint8_t data; +};u8/u16?
This sensor uses 16 bits for addresses and 8 for data, so I think it makes sense to keep it this way.
Quoted 126 lines
drivers/media/i2c/ov5647.c line 239
Quoted 8 lines
drivers/media/i2c/ov5647.c new file +7
-0+/** +* @short I2C Write operation +* @param[in] i2c_client I2C client +* @param[in] reg register address +* @param[in] val value to write +* @return Error code +*/ " *"?
drivers/media/i2c/ov5647.c line 243
Quoted 4 lines
drivers/media/i2c/ov5647.c new file +4
-0+static int ov5647_write (struct v4l2_subdev *sd,uint16_t reg,uint8_t val) +{ +int ret; +unsigned char data[3 ] = { reg >>8 , reg &0xff , val};" }".
Quoted 4 lines
drivers/media/i2c/ov5647.c new file +4
-0+static int ov5647_write_array (struct v4l2_subdev *subdev, +struct regval_list *regs,int array_size) +{ +int i =0 ;drivers/media/i2c/ov5647.c line 301
Quoted 10 lines
drivers/media/i2c/ov5647.c new file +10
-0+int ret =0 ; + +if (!regs) +return -EINVAL ; + +while (i < array_size) { +if (regs->addr ==REG_DLY ) +mdelay (regs->data); +else + ret =ov5647_write (subdev, regs->addr, regs->data);The "
REG_DLY" is never used AFAICT? Remove?drivers/media/i2c/ov5647.c line 305
Quoted 3 lines
drivers/media/i2c/ov5647.c new file +3
-0+if (ret == -EIO ) +return ret; +
ov5647_write()can return errors other then EIO. Are they handled correctly?Quoted 10 lines
drivers/media/i2c/ov5647.c new file +10
-0+/** +* @short Set SW standby +* @param[in] subdev v4l2 subdev +* @param[in] on_off standby on or off +* @return Error code +*/ +static int sensor_s_sw_stby (struct v4l2_subdev *subdev,int on_off) +{ +int ret; +unsigned char rdval;drivers/media/i2c/ov5647.c line 367
Quoted 10 lines
drivers/media/i2c/ov5647.c new file +10
-0+ + ret =ov5647_read (subdev,0x0100 , &rdval); +if (ret !=0 ) +return ret; + +if (on_off ==CSI_STBY_ON ) + ret =ov5647_write (subdev,0x0100 , rdval&0xfe ); + +else + ret =ov5647_write (subdev,0x0100 , rdval|0x01 );I'd get rid of
CSI_STBY_ON, and convert arg to bool, as you don't really handle other values. Plus, naming it "set_sw_standby()" would make core slightly more readable. Also kill the empty line before else.drivers/media/i2c/ov5647.c line 404
Quoted 11 lines
drivers/media/i2c/ov5647.c new file +11
-0+/** +* @short Initialize sensor +* @param[in] subdev v4l2 subdev +* @param[in] val not used +* @return Error code +*/ +static int __sensor_init (struct v4l2_subdev *subdev) +{ +int ret; +uint8_t resetval; +unsigned char rdval;u8 for both?
drivers/media/i2c/ov5647.c line 427
Quoted 4 lines
drivers/media/i2c/ov5647.c new file +4
-0+ov5647_read (subdev,0x0100 , &resetval); +if (!resetval&0x01 ) { +v4l2_dbg (1 , debug, subdev, +"DEVICE WAS IN SOFTWARE STANDBY" );No shouting please? If it is important maybe it should have higher priority?
Quoted 10 lines
drivers/media/i2c/ov5647.c new file +10
-0+static int sensor_power (struct v4l2_subdev *subdev,int on) +{ +int ret; +struct ov5647 *ov5647 =to_state (subdev); + + ret =0 ; +mutex_lock (&ov5647->lock); + +switch (on) { +case CSI_SUBDEV_PWR_OFF :...
Quoted 1 line
drivers/media/i2c/ov5647.c new file +1
-0+case CSI_SUBDEV_PWR_ON :...
drivers/media/i2c/ov5647.c line 486
Quoted 5 lines
drivers/media/i2c/ov5647.c new file +5
-0+default : +return -EINVAL ; + } + +mutex_unlock (&ov5647->lock);I'd really convert this to bool. Note how it returns with lock held?
Quoted 8 lines
drivers/media/i2c/ov5647.c new file +8
-0+int ov5647_detect (struct v4l2_subdev *sd) +{ +unsigned char v; +int ret; + + ret =sensor_power (sd,1 ); +if (ret <0 ) +return ret;drivers/media/i2c/ov5647.c line 725
Quoted 10 lines
drivers/media/i2c/ov5647.c new file +10
-0+ ret =ov5647_read (sd,OV5647_REG_CHIPID_H , &v); +if (ret <0 ) +return ret; +if (v !=0x56 )/* OV manuf. id. */ +return -ENODEV ; + ret =ov5647_read (sd,OV5647_REG_CHIPID_L , &v); +if (ret <0 ) +return ret; +if (v !=0x47 ) +return -ENODEV ;I guess invalid chipid deserves a printk?
Quoted 1 line
+* Refer to Linux errors.
Useful?
Quoted 8 lines
drivers/media/i2c/ov5647.c line 859
Quoted 4 lines
drivers/media/i2c/ov5647.c new file +4
-0+/** +* @short of_device_id structure +*/ +static const struct i2c_device_id ov5647_id[] = {Umm. The comment looks useless and wrong.
Best regards,
Pavel
Thanks for the feedback. I agree with most of your suggestions, and I commented with the one I didn't agree.
Thanks,
Ramiro
Re: [PATCH v3 2/2] Add support for Omnivision OV5647
Hi!
Quoted 8 lines
Quoted 5 lines
drivers/media/i2c/ov5647.c line 82
Quoted 4 lines
drivers/media/i2c/ov5647.c new file +4
-0+struct regval_list { +uint16_t addr; +uint8_t data; +};u8/u16?
This sensor uses 16 bits for addresses and 8 for data, so I think it makes sense to keep it this way.
Yes, you can do it. But please use u8/u16 types (also elsewhere in the driver), they are more common in ther kernel.
Quoted 2 lines
Thanks for the feedback. I agree with most of your suggestions, and I commented with the one I didn't agree.
You are welcome,
Pavel
Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html