subject:"\[PATCH v5 2\/4\] media\: ov5695\: add support for OV5695 sensor"

Re: [PATCH v5 2/4] media: ov5695: add support for OV5695 sensor

2018-01-10 Thread Shunqian Zheng


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(>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(>mutex);
+   on = !!on;
+   if (on == ov5695->streaming)
+   goto unlock_and_return;
+
+   if (on) {
+   ret = pm_runtime_get_sync(>dev);
+   if (ret < 0) {
+   pm_runtime_put_noidle(>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(>dev);
+   goto unlock_and_return;
+   }
+   } else {
+   __ov5695_stop_stream(ov5695);
+   ret = pm_runtime_put(>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(>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 = _pm_ops,
+   .of_match_table = ov5695_of_match
+   },
+   .probe  = _probe,
+   .remove = _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

2018-01-10 Thread jacopo mondi

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(>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(>mutex);
> + on = !!on;
> + if (on == ov5695->streaming)
> + goto unlock_and_return;
> +
> + if (on) {
> + ret = pm_runtime_get_sync(>dev);
> + if (ret < 0) {
> + pm_runtime_put_noidle(>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(>dev);
> + goto unlock_and_return;
> + }
> + } else {
> + __ov5695_stop_stream(ov5695);
> + ret = pm_runtime_put(>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(>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 = _pm_ops,
> + .of_match_table = ov5695_of_match
> + },
> + .probe  = _probe,
> + .remove = _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

2018-01-09 Thread Shunqian Zheng

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

Re: [PATCH v5 2/4] media: ov5695: add support for OV5695 sensor

Re: [PATCH v5 2/4] media: ov5695: add support for OV5695 sensor

[PATCH v5 2/4] media: ov5695: add support for OV5695 sensor

3 matches

Site Navigation

Mail list logo

Footer information