Re: [PATCH v2 2/2] media: ov2680: Add Omnivision OV2680 sensor driver

[Sakari Ailus]

On Tue, Mar 13, 2018 at 11:16:33AM +, Rui Miguel Silva wrote:
...
> > > +static int ov2680_gain_set(struct ov2680_dev *sensor, bool
> > > auto_gain)
> > > +{
> > > + struct ov2680_ctrls *ctrls = >ctrls;
> > > + u32 gain;
> > > + int ret;
> > > +
> > > + ret = ov2680_mod_reg(sensor, OV2680_REG_R_MANUAL, BIT(1),
> > > +  auto_gain ? 0 : BIT(1));
> > > + if (ret < 0)
> > > + return ret;
> > > +
> > > + if (auto_gain || !ctrls->gain->is_new)
> > > + return 0;
> > > +
> > > + gain = ctrls->gain->val;
> > > +
> > > + ret = ov2680_write_reg16(sensor, OV2680_REG_GAIN_PK, gain);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static int ov2680_gain_get(struct ov2680_dev *sensor)
> > > +{
> > > + u32 gain;
> > > + int ret;
> > > +
> > > + ret = ov2680_read_reg16(sensor, OV2680_REG_GAIN_PK, );
> > > + if (ret)
> > > + return ret;
> > > +
> > > + return gain;
> > > +}
> > > +
> > > +static int ov2680_auto_gain_enable(struct ov2680_dev *sensor)
> > > +{
> > > + return ov2680_gain_set(sensor, true);
> > 
> > Just call ov2780_gain_set() in the caller.
> 
> Here if you do not mind I would like to have it this way, on the caller
> side makes explicit that we are disabling/enabling the auto gain,
> instead of going and search what that bool parameter means.
> 
> but, if you do mind... ;)

How about renaming that function as e.g. ov2680_autogain_set()? That's what
it does, doesn't it?

Right now you have these functions doing nothing really useful, cluttering
up the code.

...

> > > +static const struct i2c_device_id ov2680_id[] = {
> > > + {"ov2680", 0},
> > > + { },
> > 
> > You can remove the i2c_device_id table if you use the probe_new callback
> > (instead of probe) below.
> 
> Well this one was hard to debug, so removing the device id table made
> the module not to be auto loaded, and after some debug I found the root
> cause and it looks it is addressed by this [0]. With that patch removing
> the device_id and use probe_new works as expected.
> 
> I will be sending v3 soon.

Great, thanks!

-- 
Sakari Ailus
sakari.ai...@linux.intel.com

Re: [PATCH v2 2/2] media: ov2680: Add Omnivision OV2680 sensor driver

[Rui Miguel Silva]

Hi Sakari,
Thanks for the review...

On Fri 09 Mar 2018 at 09:21, Sakari Ailus wrote:

Hi Miguel,

Thanks for the update.

On Wed, Feb 28, 2018 at 03:27:23PM +, Rui Miguel Silva 
wrote:

This patch adds V4L2 sub-device driver for OV2680 image sensor.
The OV2680 is a 1/5" CMOS color sensor from Omnivision.
Supports output format: 10-bit Raw RGB.
The OV2680 has a single lane MIPI interface.

The driver exposes following V4L2 controls:
- auto/manual exposure,
- exposure,
- auto/manual gain,
- gain,
- horizontal/vertical flip,
- test pattern menu.
Supported resolution are only: QUXGA, 720P, UXGA.

Signed-off-by: Rui Miguel Silva 
---
 drivers/media/i2c/Kconfig  |   12 +
 drivers/media/i2c/Makefile |1 +
 drivers/media/i2c/ov2680.c | 1094 
 

 3 files changed, 1107 insertions(+)
 create mode 100644 drivers/media/i2c/ov2680.c

diff --git a/drivers/media/i2c/Kconfig 
b/drivers/media/i2c/Kconfig

index 9f18cd296841..39dc9f236ffa 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -586,6 +586,18 @@ config VIDEO_OV2659
  To compile this driver as a module, choose M here: the
  module will be called ov2659.
 
+config VIDEO_OV2680

+   tristate "OmniVision OV2680 sensor support"
+	depends on GPIOLIB && VIDEO_V4L2 && I2C && 
VIDEO_V4L2_SUBDEV_API

+   depends on MEDIA_CAMERA_SUPPORT
+   select V4L2_FWNODE
+   ---help---
+	  This is a Video4Linux2 sensor-level driver for the 
OmniVision

+ OV2680 camera sensor with a MIPI CSI-2 interface.
+
+ To compile this driver as a module, choose M here: the
+ module will be called ov2680.
+
 config VIDEO_OV5640
tristate "OmniVision OV5640 sensor support"
depends on OF
diff --git a/drivers/media/i2c/Makefile 
b/drivers/media/i2c/Makefile

index c0f94cd8d56d..d0aba4d37b8d 100644
--- a/drivers/media/i2c/Makefile
+++ b/drivers/media/i2c/Makefile
@@ -61,6 +61,7 @@ obj-$(CONFIG_VIDEO_SONY_BTF_MPX) += 
sony-btf-mpx.o

 obj-$(CONFIG_VIDEO_UPD64031A) += upd64031a.o
 obj-$(CONFIG_VIDEO_UPD64083) += upd64083.o
 obj-$(CONFIG_VIDEO_OV2640) += ov2640.o
+obj-$(CONFIG_VIDEO_OV2680) += ov2680.o
 obj-$(CONFIG_VIDEO_OV5640) += ov5640.o
 obj-$(CONFIG_VIDEO_OV5645) += ov5645.o
 obj-$(CONFIG_VIDEO_OV5647) += ov5647.o
diff --git a/drivers/media/i2c/ov2680.c 
b/drivers/media/i2c/ov2680.c

new file mode 100644
index ..78f2be27a8f5
--- /dev/null
+++ b/drivers/media/i2c/ov2680.c
@@ -0,0 +1,1094 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Omnivision OV2680 CMOS Image Sensor driver
+ *
+ * Copyright (C) 2018 Linaro Ltd
+ *
+ * Based on OV5640 Sensor Driver
+ * Copyright (C) 2011-2013 Freescale Semiconductor, Inc. All 
Rights Reserved.

+ * Copyright (C) 2014-2017 Mentor Graphics Inc.
+ *
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+#include 
+#include 
+
+#define OV2680_XVCLK_MIN   600
+#define OV2680_XVCLK_MAX   2400
+
+#define OV2680_CHIP_ID 0x2680
+
+#define OV2680_REG_STREAM_CTRL 0x0100
+#define OV2680_REG_SOFT_RESET  0x0103
+
+#define OV2680_REG_CHIP_ID_HIGH0x300a
+#define OV2680_REG_CHIP_ID_LOW 0x300b
+
+#define OV2680_REG_R_MANUAL0x3503
+#define OV2680_REG_GAIN_PK 0x350a
+#define OV2680_REG_EXPOSURE_PK_HIGH0x3500
+#define OV2680_REG_TIMING_HTS  0x380c
+#define OV2680_REG_TIMING_VTS  0x380e
+#define OV2680_REG_FORMAT1 0x3820
+#define OV2680_REG_FORMAT2 0x3821
+
+#define OV2680_REG_ISP_CTRL00  0x5080
+
+#define OV2680_FRAME_RATE  30
+
+#define OV2680_REG_VALUE_8BIT  1
+#define OV2680_REG_VALUE_16BIT 2
+#define OV2680_REG_VALUE_24BIT 3
+
+enum ov2680_mode_id {
+   OV2680_MODE_QUXGA_800_600,
+   OV2680_MODE_720P_1280_720,
+   OV2680_MODE_UXGA_1600_1200,
+   OV2680_MODE_MAX,
+};
+
+struct reg_value {
+   u16 reg_addr;
+   u8 val;
+};
+
+struct ov2680_mode_info {
+   const char *name;
+   enum ov2680_mode_id id;
+   u32 width;
+   u32 height;
+   const struct reg_value *reg_data;
+   u32 reg_data_size;
+};
+
+struct ov2680_ctrls {
+   struct v4l2_ctrl_handler handler;
+   struct {
+   struct v4l2_ctrl *auto_exp;
+   struct v4l2_ctrl *exposure;
+   };
+   struct {
+   struct v4l2_ctrl *auto_gain;
+   struct v4l2_ctrl *gain;
+   };
+
+   struct v4l2_ctrl *hflip;
+   struct v4l2_ctrl *vflip;
+   struct v4l2_ctrl *test_pattern;
+};
+
+struct ov2680_dev {
+   struct i2c_client   *i2c_client;
+   struct v4l2_subdev  sd;
+
+   struct media_padpad;
+   struct clk  *xvclk;
+   u32 xvclk_freq;
+
+   struct gpio_desc*pwdn_gpio;
+	struct mutex			lock; 

Re: [PATCH v2 2/2] media: ov2680: Add Omnivision OV2680 sensor driver

[Sakari Ailus]

Hi Miguel,

Thanks for the update.

On Wed, Feb 28, 2018 at 03:27:23PM +, Rui Miguel Silva wrote:
> This patch adds V4L2 sub-device driver for OV2680 image sensor.
> The OV2680 is a 1/5" CMOS color sensor from Omnivision.
> Supports output format: 10-bit Raw RGB.
> The OV2680 has a single lane MIPI interface.
> 
> The driver exposes following V4L2 controls:
> - auto/manual exposure,
> - exposure,
> - auto/manual gain,
> - gain,
> - horizontal/vertical flip,
> - test pattern menu.
> Supported resolution are only: QUXGA, 720P, UXGA.
> 
> Signed-off-by: Rui Miguel Silva 
> ---
>  drivers/media/i2c/Kconfig  |   12 +
>  drivers/media/i2c/Makefile |1 +
>  drivers/media/i2c/ov2680.c | 1094 
> 
>  3 files changed, 1107 insertions(+)
>  create mode 100644 drivers/media/i2c/ov2680.c
> 
> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> index 9f18cd296841..39dc9f236ffa 100644
> --- a/drivers/media/i2c/Kconfig
> +++ b/drivers/media/i2c/Kconfig
> @@ -586,6 +586,18 @@ config VIDEO_OV2659
> To compile this driver as a module, choose M here: the
> module will be called ov2659.
>  
> +config VIDEO_OV2680
> + tristate "OmniVision OV2680 sensor support"
> + depends on GPIOLIB && VIDEO_V4L2 && I2C && VIDEO_V4L2_SUBDEV_API
> + depends on MEDIA_CAMERA_SUPPORT
> + select V4L2_FWNODE
> + ---help---
> +   This is a Video4Linux2 sensor-level driver for the OmniVision
> +   OV2680 camera sensor with a MIPI CSI-2 interface.
> +
> +   To compile this driver as a module, choose M here: the
> +   module will be called ov2680.
> +
>  config VIDEO_OV5640
>   tristate "OmniVision OV5640 sensor support"
>   depends on OF
> diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
> index c0f94cd8d56d..d0aba4d37b8d 100644
> --- a/drivers/media/i2c/Makefile
> +++ b/drivers/media/i2c/Makefile
> @@ -61,6 +61,7 @@ obj-$(CONFIG_VIDEO_SONY_BTF_MPX) += sony-btf-mpx.o
>  obj-$(CONFIG_VIDEO_UPD64031A) += upd64031a.o
>  obj-$(CONFIG_VIDEO_UPD64083) += upd64083.o
>  obj-$(CONFIG_VIDEO_OV2640) += ov2640.o
> +obj-$(CONFIG_VIDEO_OV2680) += ov2680.o
>  obj-$(CONFIG_VIDEO_OV5640) += ov5640.o
>  obj-$(CONFIG_VIDEO_OV5645) += ov5645.o
>  obj-$(CONFIG_VIDEO_OV5647) += ov5647.o
> diff --git a/drivers/media/i2c/ov2680.c b/drivers/media/i2c/ov2680.c
> new file mode 100644
> index ..78f2be27a8f5
> --- /dev/null
> +++ b/drivers/media/i2c/ov2680.c
> @@ -0,0 +1,1094 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Omnivision OV2680 CMOS Image Sensor driver
> + *
> + * Copyright (C) 2018 Linaro Ltd
> + *
> + * Based on OV5640 Sensor Driver
> + * Copyright (C) 2011-2013 Freescale Semiconductor, Inc. All Rights Reserved.
> + * Copyright (C) 2014-2017 Mentor Graphics Inc.
> + *
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include 
> +#include 
> +#include 
> +
> +#define OV2680_XVCLK_MIN 600
> +#define OV2680_XVCLK_MAX 2400
> +
> +#define OV2680_CHIP_ID   0x2680
> +
> +#define OV2680_REG_STREAM_CTRL   0x0100
> +#define OV2680_REG_SOFT_RESET0x0103
> +
> +#define OV2680_REG_CHIP_ID_HIGH  0x300a
> +#define OV2680_REG_CHIP_ID_LOW   0x300b
> +
> +#define OV2680_REG_R_MANUAL  0x3503
> +#define OV2680_REG_GAIN_PK   0x350a
> +#define OV2680_REG_EXPOSURE_PK_HIGH  0x3500
> +#define OV2680_REG_TIMING_HTS0x380c
> +#define OV2680_REG_TIMING_VTS0x380e
> +#define OV2680_REG_FORMAT1   0x3820
> +#define OV2680_REG_FORMAT2   0x3821
> +
> +#define OV2680_REG_ISP_CTRL000x5080
> +
> +#define OV2680_FRAME_RATE30
> +
> +#define OV2680_REG_VALUE_8BIT1
> +#define OV2680_REG_VALUE_16BIT   2
> +#define OV2680_REG_VALUE_24BIT   3
> +
> +enum ov2680_mode_id {
> + OV2680_MODE_QUXGA_800_600,
> + OV2680_MODE_720P_1280_720,
> + OV2680_MODE_UXGA_1600_1200,
> + OV2680_MODE_MAX,
> +};
> +
> +struct reg_value {
> + u16 reg_addr;
> + u8 val;
> +};
> +
> +struct ov2680_mode_info {
> + const char *name;
> + enum ov2680_mode_id id;
> + u32 width;
> + u32 height;
> + const struct reg_value *reg_data;
> + u32 reg_data_size;
> +};
> +
> +struct ov2680_ctrls {
> + struct v4l2_ctrl_handler handler;
> + struct {
> + struct v4l2_ctrl *auto_exp;
> + struct v4l2_ctrl *exposure;
> + };
> + struct {
> + struct v4l2_ctrl *auto_gain;
> + struct v4l2_ctrl *gain;
> + };
> +
> + struct v4l2_ctrl *hflip;
> + struct v4l2_ctrl *vflip;
> + struct v4l2_ctrl *test_pattern;
> +};
> +
> +struct ov2680_dev {
> + struct i2c_client   *i2c_client;
> + struct v4l2_subdev  sd;
> +
> + struct media_pad