Re: [PATCH v3 2/2] Add support for Omnivision OV5647
Hi! > >> +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. > 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 -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html signature.asc Description: Digital signature
Re: [PATCH v3 2/2] Add support for Omnivision OV5647
Hi! On 10/18/2016 7:31 PM, Pavel Machek wrote: > Hi! > >> +/* >> + * A V4L2 driver for OmniVision OV5647 cameras. >> + * >> + * Based on Samsung S5K6AAFX SXGA 1/6" 1.3M CMOS Image Sensor driver >> + * Copyright (C) 2011 Sylwester Nawrocki>> + * >> + * Based on Omnivision OV7670 Camera Driver >> + * Copyright (C) 2006-7 Jonathan Corbet >> + * >> + * Copyright (C) 2016, Synopsys, Inc. >> + * >> + */ > If you do copyrights, you should also specify license. > >> +static bool debug; >> +module_param(debug, bool, 0644); >> +MODULE_PARM_DESC(debug, "Debug level (0-1)"); > Please remove, we have generic infrastructure for debugging. > >> +/* >> + * The ov5647 sits on i2c with ID 0x6c >> + */ >> +#define OV5647_I2C_ADDR 0x6c >> +#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 0x >> + >> +/*define the voltage level of control signal*/ > "/* ", " */". > >> +#define CSI_STBY_ON 1 >> +#define CSI_STBY_OFF0 >> +#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 > ... >> +enum power_seq_cmd { >> +CSI_SUBDEV_PWR_OFF = 0x00, >> +CSI_SUBDEV_PWR_ON = 0x01, >> +}; > Pick one style for defines/enums? > >> +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. > >> +/** >> +* @short I2C Write operation >> +* @param[in] i2c_client I2C client >> +* @param[in] reg register address >> +* @param[in] val value to write >> +* @return Error code >> +*/ > " *"? > >> +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}; > " }". > >> +static int ov5647_write_array(struct v4l2_subdev *subdev, >> +struct regval_list *regs, int array_size) >> +{ >> +int i = 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? > >> +if (ret == -EIO) >> +return ret; >> + > ov5647_write() can return errors other then EIO. Are they handled correctly? > >> +/** >> + * @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; >> + >> +ret = ov5647_read(subdev, 0x0100, ); >> +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. > >> +/** >> +* @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? > >> +ov5647_read(subdev, 0x0100, ); >> +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? > >> +static int sensor_power(struct v4l2_subdev *subdev, int on) >> +{ >> +int ret; >> +struct ov5647 *ov5647 = to_state(subdev); >> + >> +ret = 0; >> +mutex_lock(>lock); >> + >> +switch (on) { >> +case CSI_SUBDEV_PWR_OFF: > ... >> +case CSI_SUBDEV_PWR_ON: > ... >> +default: >> +return -EINVAL; >> +} >> + >> +mutex_unlock(>lock); > I'd really convert this to bool. Note how it returns with lock held? > > >> +int ov5647_detect(struct v4l2_subdev *sd) >> +{ >> +unsigned char v; >> +int ret; >> + >> +ret = sensor_power(sd, 1); >> +if (ret < 0) >> +return ret; >> +ret = ov5647_read(sd, OV5647_REG_CHIPID_H, ); >> +if (ret < 0) >> +return ret; >> +if (v != 0x56) /* OV manuf. id. */ >> +return -ENODEV; >> +ret = ov5647_read(sd, OV5647_REG_CHIPID_L, ); >> +if (ret < 0) >> +return ret; >> +
Re: [PATCH v3 2/2] Add support for Omnivision OV5647
Hi! > +/* > + * A V4L2 driver for OmniVision OV5647 cameras. > + * > + * Based on Samsung S5K6AAFX SXGA 1/6" 1.3M CMOS Image Sensor driver > + * Copyright (C) 2011 Sylwester Nawrocki> + * > + * Based on Omnivision OV7670 Camera Driver > + * Copyright (C) 2006-7 Jonathan Corbet > + * > + * Copyright (C) 2016, Synopsys, Inc. > + * > + */ If you do copyrights, you should also specify license. > +static bool debug; > +module_param(debug, bool, 0644); > +MODULE_PARM_DESC(debug, "Debug level (0-1)"); Please remove, we have generic infrastructure for debugging. > +/* > + * The ov5647 sits on i2c with ID 0x6c > + */ > +#define OV5647_I2C_ADDR 0x6c > +#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 0x > + > +/*define the voltage level of control signal*/ "/* ", " */". > +#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_ON1 > +#define CSI_AF_PWR_OFF 0 ... > +enum power_seq_cmd { > + CSI_SUBDEV_PWR_OFF = 0x00, > + CSI_SUBDEV_PWR_ON = 0x01, > +}; Pick one style for defines/enums? > +struct regval_list { > + uint16_t addr; > + uint8_t data; > +}; u8/u16? > +/** > +* @short I2C Write operation > +* @param[in] i2c_client I2C client > +* @param[in] reg register address > +* @param[in] val value to write > +* @return Error code > +*/ " *"? > +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}; " }". > +static int ov5647_write_array(struct v4l2_subdev *subdev, > + struct regval_list *regs, int array_size) > +{ > + int i = 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? > + if (ret == -EIO) > + return ret; > + ov5647_write() can return errors other then EIO. Are they handled correctly? > +/** > + * @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; > + > + ret = ov5647_read(subdev, 0x0100, ); > + 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. > +/** > +* @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? > + ov5647_read(subdev, 0x0100, ); > + 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? > +static int sensor_power(struct v4l2_subdev *subdev, int on) > +{ > + int ret; > + struct ov5647 *ov5647 = to_state(subdev); > + > + ret = 0; > + mutex_lock(>lock); > + > + switch (on) { > + case CSI_SUBDEV_PWR_OFF: ... > + case CSI_SUBDEV_PWR_ON: ... > + default: > + return -EINVAL; > + } > + > + mutex_unlock(>lock); I'd really convert this to bool. Note how it returns with lock held? > +int ov5647_detect(struct v4l2_subdev *sd) > +{ > + unsigned char v; > + int ret; > + > + ret = sensor_power(sd, 1); > + if (ret < 0) > + return ret; > + ret = ov5647_read(sd, OV5647_REG_CHIPID_H, ); > + if (ret < 0) > + return ret; > + if (v != 0x56) /* OV manuf. id. */ > + return -ENODEV; > + ret = ov5647_read(sd, OV5647_REG_CHIPID_L, ); > + if (ret < 0) > + return ret; > + if (v != 0x47) > + return -ENODEV; I guess invalid chipid deserves a printk? > +* Refer to Linux errors. Useful? > +/** > +* @short of_device_id structure > +*/ > +static const struct i2c_device_id ov5647_id[] = { Umm. The comment looks useless and wrong.
[PATCH v3 2/2] Add support for Omnivision OV5647
Signed-off-by: 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 diff --git a/MAINTAINERS b/MAINTAINERS index 0560911..11c414b 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -8661,6 +8661,13 @@ M: Harald Welte S: Maintained F: drivers/char/pcmcia/cm4040_cs.* +OMNIVISION OV5647 SENSOR DRIVER +M: Ramiro Oliveira +L: linux-media@vger.kernel.org +T: git git://linuxtv.org/media_tree.git +S: Maintained +F: drivers/media/i2c/ov5647.c + OMNIVISION OV7670 SENSOR DRIVER M: Jonathan Corbet L: linux-media@vger.kernel.org diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig index 2669b4b..4237165 100644 --- a/drivers/media/i2c/Kconfig +++ b/drivers/media/i2c/Kconfig @@ -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 diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile index 92773b2..0d9014c 100644 --- a/drivers/media/i2c/Makefile +++ b/drivers/media/i2c/Makefile @@ -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 diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c new file mode 100644 index 000..ba9a874 --- /dev/null +++ b/drivers/media/i2c/ov5647.c @@ -0,0 +1,891 @@ +/* + * A V4L2 driver for OmniVision OV5647 cameras. + * + * Based on Samsung S5K6AAFX SXGA 1/6" 1.3M CMOS Image Sensor driver + * Copyright (C) 2011 Sylwester Nawrocki + * + * Based on Omnivision OV7670 Camera Driver + * Copyright (C) 2006-7 Jonathan Corbet + * + * Copyright (C) 2016, Synopsys, Inc. + * + */ +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +static bool debug; +module_param(debug, bool, 0644); +MODULE_PARM_DESC(debug, "Debug level (0-1)"); + +/* + * The ov5647 sits on i2c with ID 0x6c + */ +#define OV5647_I2C_ADDR 0x6c +#define SENSOR_NAME "ov5647" + +#define OV5647_REG_CHIPID_H0x300A +#define OV5647_REG_CHIPID_L0x300B + +#define REG_TERM 0xfffe +#define VAL_TERM 0xfe +#define REG_DLY 0x + +/*define the voltage level of control signal*/ +#define CSI_STBY_ON1 +#define CSI_STBY_OFF 0 +#define CSI_RST_ON 0 +#define CSI_RST_OFF1 +#define CSI_PWR_ON 1 +#define CSI_PWR_OFF0 +#define CSI_AF_PWR_ON 1 +#define CSI_AF_PWR_OFF 0 + + +#define OV5647_ROW_START 0x01 +#define OV5647_ROW_START_MIN 0 +#define OV5647_ROW_START_MAX 2004 +#define OV5647_ROW_START_DEF 54 + +#define OV5647_COLUMN_START0x02 +#define OV5647_COLUMN_START_MIN0 +#define OV5647_COLUMN_START_MAX2750 +#define OV5647_COLUMN_START_DEF16 + +#define OV5647_WINDOW_HEIGHT 0x03 +#define OV5647_WINDOW_HEIGHT_MIN 2 +#define OV5647_WINDOW_HEIGHT_MAX 2006 +#define OV5647_WINDOW_HEIGHT_DEF 1944 + +#define OV5647_WINDOW_WIDTH0x04 +#define OV5647_WINDOW_WIDTH_MIN2 +#define OV5647_WINDOW_WIDTH_MAX2752 +#define OV5647_WINDOW_WIDTH_DEF2592 + +enum power_seq_cmd { + CSI_SUBDEV_PWR_OFF = 0x00, + CSI_SUBDEV_PWR_ON = 0x01, +}; + +struct regval_list { + uint16_t addr; + uint8_t data; +}; + +struct cfg_array { + struct regval_list *regs; + int size; +}; + +struct sensor_win_size { + int width; + int height; + unsigned int hoffset; + unsigned int voffset; + unsigned int hts; + unsigned int vts; + unsigned int pclk; + unsigned int mipi_bps; + unsigned int fps_fixed; + unsigned int bin_factor; + unsigned int intg_min; + unsigned int intg_max; + void *regs; + int regs_size; + int (*set_size)(struct v4l2_subdev *subdev); +}; + + +struct ov5647 { +