Re: [PATCH v5 2/4] media: ov5695: add support for OV5695 sensor
Hi Jacopo, On 2018年01月10日 17:08, jacopo mondi wrote: Hello Shunqian, On Wed, Jan 10, 2018 at 10:06:05AM +0800, Shunqian Zheng wrote: [snip] +static int __ov5695_start_stream(struct ov5695 *ov5695) +{ + int ret; + + ret = ov5695_write_array(ov5695->client, ov5695_global_regs); + if (ret) + return ret; + ret = ov5695_write_array(ov5695->client, ov5695->cur_mode->reg_list); + if (ret) + return ret; + + /* In case these controls are set before streaming */ + ret = __v4l2_ctrl_handler_setup(&ov5695->ctrl_handler); + if (ret) + return ret; + + return ov5695_write_reg(ov5695->client, OV5695_REG_CTRL_MODE, + OV5695_REG_VALUE_08BIT, OV5695_MODE_STREAMING); +} + +static int __ov5695_stop_stream(struct ov5695 *ov5695) +{ + return ov5695_write_reg(ov5695->client, OV5695_REG_CTRL_MODE, + OV5695_REG_VALUE_08BIT, OV5695_MODE_SW_STANDBY); +} + +static int ov5695_s_stream(struct v4l2_subdev *sd, int on) +{ + struct ov5695 *ov5695 = to_ov5695(sd); + struct i2c_client *client = ov5695->client; + int ret = 0; + + mutex_lock(&ov5695->mutex); + on = !!on; + if (on == ov5695->streaming) + goto unlock_and_return; + + if (on) { + ret = pm_runtime_get_sync(&client->dev); + if (ret < 0) { + pm_runtime_put_noidle(&client->dev); + goto unlock_and_return; + } + + ret = __ov5695_start_stream(ov5695); + if (ret) { + v4l2_err(sd, "start stream failed while write regs\n"); + pm_runtime_put(&client->dev); + goto unlock_and_return; + } + } else { + __ov5695_stop_stream(ov5695); + ret = pm_runtime_put(&client->dev); I would return the result of __ov5695_stop_stream() instead of pm_runtime_put(). I know I asked for this, but if the first s_stream(0) fails, the sensor may not have been stopped but the interface will be put in "streaming = 0" state, preventing a second s_stream(0) to be issued because of your check "on == ov5695->streaming" a few lines above. I can't tell how bad this is. Imho is acceptable but I would like to hear someone else opinion here :) How about not checking the return values of s_stream(0) branch? It seems not much this driver can do if pm_runtime_put() fails. + } + + ov5695->streaming = on; + +unlock_and_return: + mutex_unlock(&ov5695->mutex); + + return ret; +} + + [snip] +static const struct of_device_id ov5695_of_match[] = { + { .compatible = "ovti,ov5695" }, + {}, +}; If you don't list CONFIG_OF as a dependecy for this driver (which you should not imho), please guard this with: #if IS_ENABLED(CONFIG_OF) #endif + +static struct i2c_driver ov5695_i2c_driver = { + .driver = { + .name = "ov5695", + .owner = THIS_MODULE, + .pm = &ov5695_pm_ops, + .of_match_table = ov5695_of_match + }, + .probe = &ov5695_probe, + .remove = &ov5695_remove, +}; + +module_i2c_driver(ov5695_i2c_driver); + +MODULE_DESCRIPTION("OmniVision ov5695 sensor driver"); +MODULE_LICENSE("GPL v2"); As you've fixed my comments on v1, and with the above bits addressed: Reviewed-by: Jacopo Mondi Thank you very much~ Shunqian Thanks j -- 1.9.1
Re: [PATCH v5 2/4] media: ov5695: add support for OV5695 sensor
Hello Shunqian, On Wed, Jan 10, 2018 at 10:06:05AM +0800, Shunqian Zheng wrote: [snip] > +static int __ov5695_start_stream(struct ov5695 *ov5695) > +{ > + int ret; > + > + ret = ov5695_write_array(ov5695->client, ov5695_global_regs); > + if (ret) > + return ret; > + ret = ov5695_write_array(ov5695->client, ov5695->cur_mode->reg_list); > + if (ret) > + return ret; > + > + /* In case these controls are set before streaming */ > + ret = __v4l2_ctrl_handler_setup(&ov5695->ctrl_handler); > + if (ret) > + return ret; > + > + return ov5695_write_reg(ov5695->client, OV5695_REG_CTRL_MODE, > + OV5695_REG_VALUE_08BIT, OV5695_MODE_STREAMING); > +} > + > +static int __ov5695_stop_stream(struct ov5695 *ov5695) > +{ > + return ov5695_write_reg(ov5695->client, OV5695_REG_CTRL_MODE, > + OV5695_REG_VALUE_08BIT, OV5695_MODE_SW_STANDBY); > +} > + > +static int ov5695_s_stream(struct v4l2_subdev *sd, int on) > +{ > + struct ov5695 *ov5695 = to_ov5695(sd); > + struct i2c_client *client = ov5695->client; > + int ret = 0; > + > + mutex_lock(&ov5695->mutex); > + on = !!on; > + if (on == ov5695->streaming) > + goto unlock_and_return; > + > + if (on) { > + ret = pm_runtime_get_sync(&client->dev); > + if (ret < 0) { > + pm_runtime_put_noidle(&client->dev); > + goto unlock_and_return; > + } > + > + ret = __ov5695_start_stream(ov5695); > + if (ret) { > + v4l2_err(sd, "start stream failed while write regs\n"); > + pm_runtime_put(&client->dev); > + goto unlock_and_return; > + } > + } else { > + __ov5695_stop_stream(ov5695); > + ret = pm_runtime_put(&client->dev); I would return the result of __ov5695_stop_stream() instead of pm_runtime_put(). I know I asked for this, but if the first s_stream(0) fails, the sensor may not have been stopped but the interface will be put in "streaming = 0" state, preventing a second s_stream(0) to be issued because of your check "on == ov5695->streaming" a few lines above. I can't tell how bad this is. Imho is acceptable but I would like to hear someone else opinion here :) > + } > + > + ov5695->streaming = on; > + > +unlock_and_return: > + mutex_unlock(&ov5695->mutex); > + > + return ret; > +} > + > + [snip] > +static const struct of_device_id ov5695_of_match[] = { > + { .compatible = "ovti,ov5695" }, > + {}, > +}; If you don't list CONFIG_OF as a dependecy for this driver (which you should not imho), please guard this with: #if IS_ENABLED(CONFIG_OF) #endif > + > +static struct i2c_driver ov5695_i2c_driver = { > + .driver = { > + .name = "ov5695", > + .owner = THIS_MODULE, > + .pm = &ov5695_pm_ops, > + .of_match_table = ov5695_of_match > + }, > + .probe = &ov5695_probe, > + .remove = &ov5695_remove, > +}; > + > +module_i2c_driver(ov5695_i2c_driver); > + > +MODULE_DESCRIPTION("OmniVision ov5695 sensor driver"); > +MODULE_LICENSE("GPL v2"); As you've fixed my comments on v1, and with the above bits addressed: Reviewed-by: Jacopo Mondi Thanks j > -- > 1.9.1 >
[PATCH v5 2/4] media: ov5695: add support for OV5695 sensor
This patch adds driver for Omnivision's ov5695 sensor, the driver supports following features: - supported resolutions + 2592x1944 at 30fps + 1920x1080 at 30fps + 1296x972 at 60fps + 1280x720 at 30fps + 640x480 at 120fps - test patterns - manual exposure/gain(analog and digital) control - vblank and hblank - media controller - runtime pm Signed-off-by: Shunqian Zheng --- MAINTAINERS|7 + drivers/media/i2c/Kconfig | 11 + drivers/media/i2c/Makefile |1 + drivers/media/i2c/ov5695.c | 1396 4 files changed, 1415 insertions(+) create mode 100644 drivers/media/i2c/ov5695.c diff --git a/MAINTAINERS b/MAINTAINERS index 85773bf..cca5b1c 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -10046,6 +10046,13 @@ T: git git://linuxtv.org/media_tree.git S: Maintained F: drivers/media/i2c/ov5647.c +OMNIVISION OV5695 SENSOR DRIVER +M: Shunqian Zheng +L: linux-media@vger.kernel.org +T: git git://linuxtv.org/media_tree.git +S: Maintained +F: drivers/media/i2c/ov5695.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 3c6d642..55b37c8 100644 --- a/drivers/media/i2c/Kconfig +++ b/drivers/media/i2c/Kconfig @@ -645,6 +645,17 @@ config VIDEO_OV5670 To compile this driver as a module, choose M here: the module will be called ov5670. +config VIDEO_OV5695 + tristate "OmniVision OV5695 sensor support" + depends on I2C && VIDEO_V4L2 + depends on MEDIA_CAMERA_SUPPORT + ---help--- + This is a Video4Linux2 sensor-level driver for the OmniVision + OV5695 camera. + + To compile this driver as a module, choose M here: the + module will be called ov5695. + 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 548a9ef..a063030 100644 --- a/drivers/media/i2c/Makefile +++ b/drivers/media/i2c/Makefile @@ -65,6 +65,7 @@ obj-$(CONFIG_VIDEO_OV5640) += ov5640.o obj-$(CONFIG_VIDEO_OV5645) += ov5645.o obj-$(CONFIG_VIDEO_OV5647) += ov5647.o obj-$(CONFIG_VIDEO_OV5670) += ov5670.o +obj-$(CONFIG_VIDEO_OV5695) += ov5695.o obj-$(CONFIG_VIDEO_OV6650) += ov6650.o obj-$(CONFIG_VIDEO_OV7640) += ov7640.o obj-$(CONFIG_VIDEO_OV7670) += ov7670.o diff --git a/drivers/media/i2c/ov5695.c b/drivers/media/i2c/ov5695.c new file mode 100644 index 000..c2e0462 --- /dev/null +++ b/drivers/media/i2c/ov5695.c @@ -0,0 +1,1396 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * ov5695 driver + * + * Copyright (C) 2017 Fuzhou Rockchip Electronics Co., Ltd. + */ + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#ifndef V4L2_CID_DIGITAL_GAIN +#define V4L2_CID_DIGITAL_GAIN V4L2_CID_GAIN +#endif + +/* 45Mhz * 4 Binning */ +#define OV5695_PIXEL_RATE (45 * 1000 * 1000 * 4) +#define OV5695_XVCLK_FREQ 2400 + +#define CHIP_ID0x005695 +#define OV5695_REG_CHIP_ID 0x300a + +#define OV5695_REG_CTRL_MODE 0x0100 +#define OV5695_MODE_SW_STANDBY 0x0 +#define OV5695_MODE_STREAMING BIT(0) + +#define OV5695_REG_EXPOSURE0x3500 +#defineOV5695_EXPOSURE_MIN 4 +#defineOV5695_EXPOSURE_STEP1 +#define OV5695_VTS_MAX 0x7fff + +#define OV5695_REG_ANALOG_GAIN 0x3509 +#defineANALOG_GAIN_MIN 0x10 +#defineANALOG_GAIN_MAX 0xf8 +#defineANALOG_GAIN_STEP1 +#defineANALOG_GAIN_DEFAULT 0xf8 + +#define OV5695_REG_DIGI_GAIN_H 0x350a +#define OV5695_REG_DIGI_GAIN_L 0x350b +#define OV5695_DIGI_GAIN_L_MASK0x3f +#define OV5695_DIGI_GAIN_H_SHIFT 6 +#define OV5695_DIGI_GAIN_MIN 0 +#define OV5695_DIGI_GAIN_MAX (0x4000 - 1) +#define OV5695_DIGI_GAIN_STEP 1 +#define OV5695_DIGI_GAIN_DEFAULT 1024 + +#define OV5695_REG_TEST_PATTERN0x4503 +#defineOV5695_TEST_PATTERN_ENABLE 0x80 +#defineOV5695_TEST_PATTERN_DISABLE 0x0 + +#define OV5695_REG_VTS 0x380e + +#define REG_NULL 0x + +#define OV5695_REG_VALUE_08BIT 1 +#define OV5695_REG_VALUE_16BIT 2 +#define OV5695_REG_VALUE_24BIT 3 + +#define OV5695_LANES 2 +#define OV5695_BITS_PER_SAMPLE 10 + +static const char * const ov5695_supply_names[] = { + "avdd", /* Analog power */ + "dovdd",/* Digital I/O power */ + "dvdd", /* Digital core power */ +}; + +#define OV5695_NUM_SUPPLIES ARRAY_SIZE(ov5695_supply_names) + +struct regva