Re: [PATCH v3 22/24] media: imx: Add MIPI CSI-2 OV5640 sensor subdev driver

2017-02-12 Thread Steve Longerbeam 

(resending text only)


On 02/02/2017 02:36 AM, Laurent Pinchart wrote:

Hi Steve,

Thank you for the patch. Many of the comments below apply to the ov5642 driver
too, please take them into account when reworking patch 23/24.

On Friday 06 Jan 2017 18:11:40 Steve Longerbeam wrote:

This driver is based on ov5640_mipi.c from Freescale imx_3.10.17_1.0.0_beta
branch, modified heavily to bring forward to latest interfaces and code
cleanup.

Signed-off-by: Steve Longerbeam
---
  drivers/staging/media/imx/Kconfig   |8 +
  drivers/staging/media/imx/Makefile  |2 +
  drivers/staging/media/imx/ov5640-mipi.c | 2348 

You're missing DT bindings.


Done, created Documentation/devicetree/bindings/media/i2c/ov5640.txt.


The driver should go to drivers/media/i2c/ as it should not be specific to the
i.MX6, and you can just call it ov5640.c.


Done.


diff --git a/drivers/staging/media/imx/Kconfig
b/drivers/staging/media/imx/Kconfig index ce2d2c8..09f373d 100644
--- a/drivers/staging/media/imx/Kconfig
+++ b/drivers/staging/media/imx/Kconfig
@@ -17,5 +17,13 @@ config VIDEO_IMX_CAMERA
---help---
  A video4linux camera capture driver for i.MX5/6.

+config IMX_OV5640_MIPI
+   tristate "OmniVision OV5640 MIPI CSI-2 camera support"
+   depends on GPIOLIB && VIDEO_IMX_CAMERA

The sensor driver is generic, it shouldn't depend on IMX. It should however
depend on at least I2C and OF by the look of it.


Done.




+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 

Pet peeve of mine, please sort the headers alphabetically. It makes it easier
to locate duplicated.


Fixed.


+
+#define OV5640_CHIP_ID  0x300A
+#define OV5640_SLAVE_ID 0x3100
+#define OV5640_DEFAULT_SLAVE_ID 0x3c

You're mixing lower-case and upper-case hex constants. Let's pick one. Kernel
code usually favours lower-case.


Fixed.


Please define macros for all the other numerical constants you use in the
driver (register addresses and values). The large registers tables can be an
exception if you don't have access to the information, but for registers
written manually, hardcoding numerical values isn't good.


Done.


+
+#define OV5640_MAX_CONTROLS 64
+
+enum ov5640_mode {
+   ov5640_mode_MIN = 0,
+   ov5640_mode_QCIF_176_144 = 0,
+   ov5640_mode_QVGA_320_240,
+   ov5640_mode_VGA_640_480,
+   ov5640_mode_NTSC_720_480,
+   ov5640_mode_PAL_720_576,
+   ov5640_mode_XGA_1024_768,
+   ov5640_mode_720P_1280_720,
+   ov5640_mode_1080P_1920_1080,
+   ov5640_mode_QSXGA_2592_1944,
+   ov5640_num_modes,
+   ov5640_mode_INIT = 0xff, /*only for sensor init*/

Please add spaces after /* and before */.

Enumerated values should be all upper-case.


Fixed (and ov5640_mode_INIT is removed).

+
+/* image size under 1280 * 960 are SUBSAMPLING
+ * image size upper 1280 * 960 are SCALING
+ */

The kernel multi-line comment style is

/*
  * text
  * text
  */


Fixed.


+
+struct ov5640_dev {
+   struct i2c_client *i2c_client;
+   struct device *dev;
+   struct v4l2_subdev sd;
+   struct media_pad pad;
+   struct v4l2_ctrl_handler ctrl_hdl;
+   struct v4l2_of_endpoint ep; /* the parsed DT endpoint info */
+   struct v4l2_mbus_framefmt fmt;
+   struct v4l2_captureparm streamcap;
+   struct clk *xclk; /* system clock to OV5640 */
+   int xclk_freq;/* requested xclk freq from devicetree */
+
+   enum ov5640_mode current_mode;

Store a (const) pointer to the corresponding ov5640_mode_info instead, it will
simplify the code and allow you to get rid of the ov5640_mode enum.


Done.


+
+   int prev_sysclk, prev_hts;
+   int ae_low, ae_high, ae_target;

Can't these be unsigned int ?


yep, an old left-over Freescale-ism, fixed.


+
+static void ov5640_power(struct ov5640_dev *sensor, bool enable);
+static void ov5640_reset(struct ov5640_dev *sensor);
+static int ov5640_restore_ctrls(struct ov5640_dev *sensor);
+static int ov5640_set_agc(struct ov5640_dev *sensor, int value);
+static int ov5640_set_exposure(struct ov5640_dev *sensor, int value);
+static int ov5640_get_exposure(struct ov5640_dev *sensor);
+static int ov5640_set_gain(struct ov5640_dev *sensor, int value);
+static int ov5640_get_gain(struct ov5640_dev *sensor);

No forward declarations please. You should reorder functions as needed (and of
course still group related functions together).


Fixed.


+static struct reg_value ov5640_init_setting_30fps_VGA[] = {
+

+   {0x3a1f, 0x14, 0, 0}, {0x3008, 0x02, 0, 0}, {0x3c00, 0x04, 0, 300},
+};

You only use the delay feature of the registers tables twice, once after
writing the first two registers (to select the clock source and perform a
software reset) and once at the very end.


There is a delay in other places as well. There is a

Re: [PATCH v3 22/24] media: imx: Add MIPI CSI-2 OV5640 sensor subdev driver

2017-02-02 Thread Laurent Pinchart 
Hi Steve,

Thank you for the patch. Many of the comments below apply to the ov5642 driver 
too, please take them into account when reworking patch 23/24.

On Friday 06 Jan 2017 18:11:40 Steve Longerbeam wrote:
> This driver is based on ov5640_mipi.c from Freescale imx_3.10.17_1.0.0_beta
> branch, modified heavily to bring forward to latest interfaces and code
> cleanup.
> 
> Signed-off-by: Steve Longerbeam 
> ---
>  drivers/staging/media/imx/Kconfig   |8 +
>  drivers/staging/media/imx/Makefile  |2 +
>  drivers/staging/media/imx/ov5640-mipi.c | 2348 

You're missing DT bindings.

The driver should go to drivers/media/i2c/ as it should not be specific to the 
i.MX6, and you can just call it ov5640.c.

>  3 files changed, 2358 insertions(+)
>  create mode 100644 drivers/staging/media/imx/ov5640-mipi.c
> 
> diff --git a/drivers/staging/media/imx/Kconfig
> b/drivers/staging/media/imx/Kconfig index ce2d2c8..09f373d 100644
> --- a/drivers/staging/media/imx/Kconfig
> +++ b/drivers/staging/media/imx/Kconfig
> @@ -17,5 +17,13 @@ config VIDEO_IMX_CAMERA
>   ---help---
> A video4linux camera capture driver for i.MX5/6.
> 
> +config IMX_OV5640_MIPI
> +   tristate "OmniVision OV5640 MIPI CSI-2 camera support"
> +   depends on GPIOLIB && VIDEO_IMX_CAMERA

The sensor driver is generic, it shouldn't depend on IMX. It should however 
depend on at least I2C and OF by the look of it.

> +   select IMX_MIPI_CSI2
> +   default y
> +   ---help---
> + MIPI CSI-2 OV5640 Camera support.
> +
>  endmenu
>  endif

[snip]

> diff --git a/drivers/staging/media/imx/ov5640-mipi.c
> b/drivers/staging/media/imx/ov5640-mipi.c new file mode 100644
> index 000..54647a7
> --- /dev/null
> +++ b/drivers/staging/media/imx/ov5640-mipi.c
> @@ -0,0 +1,2348 @@
> +/*
> + * Copyright (c) 2014 Mentor Graphics Inc.
> + * Copyright (C) 2011-2013 Freescale Semiconductor, Inc. All Rights
> Reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 

Pet peeve of mine, please sort the headers alphabetically. It makes it easier 
to locate duplicated.

> +
> +#define OV5640_VOLTAGE_ANALOG   280
> +#define OV5640_VOLTAGE_DIGITAL_CORE 150
> +#define OV5640_VOLTAGE_DIGITAL_IO   180
> +
> +#define MIN_FPS 15
> +#define MAX_FPS 30
> +#define DEFAULT_FPS 30
> +
> +/* min/typical/max system clock (xclk) frequencies */
> +#define OV5640_XCLK_MIN  600
> +#define OV5640_XCLK_TYP 2400
> +#define OV5640_XCLK_MAX 5400
> +
> +/* min/typical/max pixel clock (mclk) frequencies */
> +#define OV5640_MCLK_MIN 4800
> +#define OV5640_MCLK_TYP 4800
> +#define OV5640_MCLK_MAX 9600
> +
> +#define OV5640_CHIP_ID  0x300A
> +#define OV5640_SLAVE_ID 0x3100
> +#define OV5640_DEFAULT_SLAVE_ID 0x3c

You're mixing lower-case and upper-case hex constants. Let's pick one. Kernel 
code usually favours lower-case.

Please define macros for all the other numerical constants you use in the 
driver (register addresses and values). The large registers tables can be an 
exception if you don't have access to the information, but for registers 
written manually, hardcoding numerical values isn't good.

> +
> +#define OV5640_MAX_CONTROLS 64
> +
> +enum ov5640_mode {
> + ov5640_mode_MIN = 0,
> + ov5640_mode_QCIF_176_144 = 0,
> + ov5640_mode_QVGA_320_240,
> + ov5640_mode_VGA_640_480,
> + ov5640_mode_NTSC_720_480,
> + ov5640_mode_PAL_720_576,
> + ov5640_mode_XGA_1024_768,
> + ov5640_mode_720P_1280_720,
> + ov5640_mode_1080P_1920_1080,
> + ov5640_mode_QSXGA_2592_1944,
> + ov5640_num_modes,
> + ov5640_mode_INIT = 0xff, /*only for sensor init*/

Please add spaces after /* and before */.

Enumerated values should be all upper-case.

> +};
> +
> +enum ov5640_frame_rate {
> + ov5640_15_fps,
> + ov5640_30_fps
> +};
> +
> +static int ov5640_framerates[] = {
> + [ov5640_15_fps] = 15,
> + [ov5640_30_fps] = 30,
> +};
> +#define ov5640_num_framerates ARRAY_SIZE(ov5640_framerates)
> +
> +/* image size under 1280 * 960 are SUBSAMPLING
> + * image size upper 1280 * 960 are SCALING
> + */

The kernel multi-line comment style is

/*
 * text
 * text
 */

> +enum ov5640_downsize_mode {
> + SUBSAMPLING,
> + SCALING,
> +};
> +
> +struct reg_value {
> + u16 reg_addr;
> + u8 val;
> + u8 mask;
> + u32 delay_ms;
> +};
> +
> +struct ov5640_mode_info {
> + enum ov5640_mode mode;
> + enum

Re: [PATCH v3 22/24] media: imx: Add MIPI CSI-2 OV5640 sensor subdev driver

2017-01-30 Thread Steve Longerbeam 



On 01/30/2017 03:29 PM, Russell King - ARM Linux wrote:

On Fri, Jan 06, 2017 at 06:11:40PM -0800, Steve Longerbeam wrote:

+config IMX_OV5640_MIPI
+   tristate "OmniVision OV5640 MIPI CSI-2 camera support"
+   depends on GPIOLIB && VIDEO_IMX_CAMERA
+   select IMX_MIPI_CSI2
+   default y

Why is this defaulting to y?  New drivers should not default to enabled
unless they are replacing some already pre-existing functionality.
Ditto for the other camera driver.


The ov564x sensors are required for the SabreSD and SabreLite/Nitrogen
reference boards (if VIDEO_IMX_CAMERA is enabled that is). But they're not
required for other platforms, so you're right I shouldn't have defaulted 
these

to yes. Fixed.


Steve

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 22/24] media: imx: Add MIPI CSI-2 OV5640 sensor subdev driver

2017-01-30 Thread Russell King - ARM Linux 
On Fri, Jan 06, 2017 at 06:11:40PM -0800, Steve Longerbeam wrote:
> +config IMX_OV5640_MIPI
> +   tristate "OmniVision OV5640 MIPI CSI-2 camera support"
> +   depends on GPIOLIB && VIDEO_IMX_CAMERA
> +   select IMX_MIPI_CSI2
> +   default y

Why is this defaulting to y?  New drivers should not default to enabled
unless they are replacing some already pre-existing functionality.
Ditto for the other camera driver.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 22/24] media: imx: Add MIPI CSI-2 OV5640 sensor subdev driver

2017-01-25 Thread Steve Longerbeam 



On 01/20/2017 06:48 AM, Hans Verkuil wrote:

On 01/07/2017 03:11 AM, Steve Longerbeam wrote:

+
+   /* cached control settings */
+   int ctrl_cache[OV5640_MAX_CONTROLS];

This is just duplicating the cached value in the control framework. I think 
this can be dropped.


done, see below.




+
+static struct ov5640_control ov5640_ctrls[] = {
+   {
+   .set = ov5640_set_agc,
+   .ctrl = {
+   .id = V4L2_CID_AUTOGAIN,
+   .name = "Auto Gain/Exposure Control",
+   .minimum = 0,
+   .maximum = 1,
+   .step = 1,
+   .default_value = 1,
+   .type = V4L2_CTRL_TYPE_BOOLEAN,
+   },
+   }, {
+   .set = ov5640_set_exposure,
+   .ctrl = {
+   .id = V4L2_CID_EXPOSURE,
+   .name = "Exposure",
+   .minimum = 0,
+   .maximum = 65535,
+   .step = 1,
+   .default_value = 0,
+   .type = V4L2_CTRL_TYPE_INTEGER,
+   },
+   }, {
+   .set = ov5640_set_gain,
+   .ctrl = {
+   .id = V4L2_CID_GAIN,
+   .name = "Gain",
+   .minimum = 0,
+   .maximum = 1023,
+   .step = 1,
+   .default_value = 0,
+   .type = V4L2_CTRL_TYPE_INTEGER,
+   },
+   }, {
+   .set = ov5640_set_hue,
+   .ctrl = {
+   .id = V4L2_CID_HUE,
+   .name = "Hue",
+   .minimum = 0,
+   .maximum = 359,
+   .step = 1,
+   .default_value = 0,
+   .type = V4L2_CTRL_TYPE_INTEGER,
+   },
+   }, {
+   .set = ov5640_set_contrast,
+   .ctrl = {
+   .id = V4L2_CID_CONTRAST,
+   .name = "Contrast",
+   .minimum = 0,
+   .maximum = 255,
+   .step = 1,
+   .default_value = 0,
+   .type = V4L2_CTRL_TYPE_INTEGER,
+   },
+   }, {
+   .set = ov5640_set_saturation,
+   .ctrl = {
+   .id = V4L2_CID_SATURATION,
+   .name = "Saturation",
+   .minimum = 0,
+   .maximum = 255,
+   .step = 1,
+   .default_value = 64,
+   .type = V4L2_CTRL_TYPE_INTEGER,
+   },
+   }, {
+   .set = ov5640_set_awb,
+   .ctrl = {
+   .id = V4L2_CID_AUTO_WHITE_BALANCE,
+   .name = "Auto White Balance",
+   .minimum = 0,
+   .maximum = 1,
+   .step = 1,
+   .default_value = 1,
+   .type = V4L2_CTRL_TYPE_BOOLEAN,
+   },
+   }, {
+   .set = ov5640_set_red_balance,
+   .ctrl = {
+   .id = V4L2_CID_RED_BALANCE,
+   .name = "Red Balance",
+   .minimum = 0,
+   .maximum = 4095,
+   .step = 1,
+   .default_value = 0,
+   .type = V4L2_CTRL_TYPE_INTEGER,
+   },
+   }, {
+   .set = ov5640_set_blue_balance,
+   .ctrl = {
+   .id = V4L2_CID_BLUE_BALANCE,
+   .name = "Blue Balance",
+   .minimum = 0,
+   .maximum = 4095,
+   .step = 1,
+   .default_value = 0,
+   .type = V4L2_CTRL_TYPE_INTEGER,
+   },
+   },
+};
+#define OV5640_NUM_CONTROLS ARRAY_SIZE(ov5640_ctrls)

This should use v4l2_ctrl_new_std() instead of this array.
Just put a switch on ctrl->id in s_ctrl, and each case calls the corresponding
set function.


In this case, because there are lots of controls, my preference is to use
table lookup rather than a large switch statement. However I did remove
.name and .type from the table entries, leaving only .def, .min, .max, .step
as required to pass to v4l2_ctrl_new_std(). Also converted to 'struct 
v4l2_ctrl_config'

in the table.





+
+static int ov5640_restore_ctrls(struct ov5640_dev *sensor)
+{
+   struct ov5640_control *c;
+   int i;
+
+   for (i = 0; i < OV5640_NUM_CONTROLS; i++) {
+   c = _ctrls[i];
+   c->set(sensor, sensor->ctrl_cache[i]);
+   }
+
+   return 0;
+}

This does the same as v4l2_ctrl_handler_setup() if I understand the code 
correctly.


yes thanks. I

Re: [PATCH v3 22/24] media: imx: Add MIPI CSI-2 OV5640 sensor subdev driver

2017-01-20 Thread Hans Verkuil 
On 01/07/2017 03:11 AM, Steve Longerbeam wrote:
> This driver is based on ov5640_mipi.c from Freescale imx_3.10.17_1.0.0_beta
> branch, modified heavily to bring forward to latest interfaces and code
> cleanup.
> 
> Signed-off-by: Steve Longerbeam 
> ---
>  drivers/staging/media/imx/Kconfig   |8 +
>  drivers/staging/media/imx/Makefile  |2 +
>  drivers/staging/media/imx/ov5640-mipi.c | 2348 
> +++
>  3 files changed, 2358 insertions(+)
>  create mode 100644 drivers/staging/media/imx/ov5640-mipi.c
> 
> diff --git a/drivers/staging/media/imx/Kconfig 
> b/drivers/staging/media/imx/Kconfig
> index ce2d2c8..09f373d 100644
> --- a/drivers/staging/media/imx/Kconfig
> +++ b/drivers/staging/media/imx/Kconfig
> @@ -17,5 +17,13 @@ config VIDEO_IMX_CAMERA
>   ---help---
> A video4linux camera capture driver for i.MX5/6.
>  
> +config IMX_OV5640_MIPI
> +   tristate "OmniVision OV5640 MIPI CSI-2 camera support"
> +   depends on GPIOLIB && VIDEO_IMX_CAMERA
> +   select IMX_MIPI_CSI2
> +   default y
> +   ---help---
> + MIPI CSI-2 OV5640 Camera support.
> +
>  endmenu
>  endif
> diff --git a/drivers/staging/media/imx/Makefile 
> b/drivers/staging/media/imx/Makefile
> index 0decef7..aa954c1 100644
> --- a/drivers/staging/media/imx/Makefile
> +++ b/drivers/staging/media/imx/Makefile
> @@ -10,3 +10,5 @@ obj-$(CONFIG_VIDEO_IMX_CAMERA) += imx-csi.o
>  obj-$(CONFIG_VIDEO_IMX_CAMERA) += imx-smfc.o
>  obj-$(CONFIG_VIDEO_IMX_CAMERA) += imx-camif.o
>  obj-$(CONFIG_VIDEO_IMX_CAMERA) += imx-mipi-csi2.o
> +
> +obj-$(CONFIG_IMX_OV5640_MIPI) += ov5640-mipi.o
> diff --git a/drivers/staging/media/imx/ov5640-mipi.c 
> b/drivers/staging/media/imx/ov5640-mipi.c
> new file mode 100644
> index 000..54647a7
> --- /dev/null
> +++ b/drivers/staging/media/imx/ov5640-mipi.c
> @@ -0,0 +1,2348 @@
> +/*
> + * Copyright (c) 2014 Mentor Graphics Inc.
> + * Copyright (C) 2011-2013 Freescale Semiconductor, Inc. All Rights Reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#define OV5640_VOLTAGE_ANALOG   280
> +#define OV5640_VOLTAGE_DIGITAL_CORE 150
> +#define OV5640_VOLTAGE_DIGITAL_IO   180
> +
> +#define MIN_FPS 15
> +#define MAX_FPS 30
> +#define DEFAULT_FPS 30
> +
> +/* min/typical/max system clock (xclk) frequencies */
> +#define OV5640_XCLK_MIN  600
> +#define OV5640_XCLK_TYP 2400
> +#define OV5640_XCLK_MAX 5400
> +
> +/* min/typical/max pixel clock (mclk) frequencies */
> +#define OV5640_MCLK_MIN 4800
> +#define OV5640_MCLK_TYP 4800
> +#define OV5640_MCLK_MAX 9600
> +
> +#define OV5640_CHIP_ID  0x300A
> +#define OV5640_SLAVE_ID 0x3100
> +#define OV5640_DEFAULT_SLAVE_ID 0x3c
> +
> +#define OV5640_MAX_CONTROLS 64
> +
> +enum ov5640_mode {
> + ov5640_mode_MIN = 0,
> + ov5640_mode_QCIF_176_144 = 0,
> + ov5640_mode_QVGA_320_240,
> + ov5640_mode_VGA_640_480,
> + ov5640_mode_NTSC_720_480,
> + ov5640_mode_PAL_720_576,
> + ov5640_mode_XGA_1024_768,
> + ov5640_mode_720P_1280_720,
> + ov5640_mode_1080P_1920_1080,
> + ov5640_mode_QSXGA_2592_1944,
> + ov5640_num_modes,
> + ov5640_mode_INIT = 0xff, /*only for sensor init*/
> +};
> +
> +enum ov5640_frame_rate {
> + ov5640_15_fps,
> + ov5640_30_fps
> +};
> +
> +static int ov5640_framerates[] = {
> + [ov5640_15_fps] = 15,
> + [ov5640_30_fps] = 30,
> +};
> +#define ov5640_num_framerates ARRAY_SIZE(ov5640_framerates)
> +
> +/* image size under 1280 * 960 are SUBSAMPLING
> + * image size upper 1280 * 960 are SCALING
> + */
> +enum ov5640_downsize_mode {
> + SUBSAMPLING,
> + SCALING,
> +};
> +
> +struct reg_value {
> + u16 reg_addr;
> + u8 val;
> + u8 mask;
> + u32 delay_ms;
> +};
> +
> +struct ov5640_mode_info {
> + enum ov5640_mode mode;
> + enum ov5640_downsize_mode dn_mode;
> + u32 width;
> + u32 height;
> + struct reg_value *init_data_ptr;
> + u32 init_data_size;
> +};
> +
> +struct ov5640_dev {
> + struct i2c_client *i2c_client;
> + struct device *dev;
> + struct v4l2_subdev sd;
> + struct media_pad pad;
> + struct v4l2_ctrl_handler ctrl_hdl;
> + struct v4l2_of_endpoint ep; /* the parsed DT endpoint info */
> + struct v4l2_mbus_framefmt fmt;
> + struct v4l2_captureparm streamcap;
> + struct clk *xclk; /* system clock to OV5640 */
> + int xclk_freq;/* requested xclk freq from devicetree */
> +
> +

[PATCH v3 22/24] media: imx: Add MIPI CSI-2 OV5640 sensor subdev driver

2017-01-06 Thread Steve Longerbeam 
This driver is based on ov5640_mipi.c from Freescale imx_3.10.17_1.0.0_beta
branch, modified heavily to bring forward to latest interfaces and code
cleanup.

Signed-off-by: Steve Longerbeam 
---
 drivers/staging/media/imx/Kconfig   |8 +
 drivers/staging/media/imx/Makefile  |2 +
 drivers/staging/media/imx/ov5640-mipi.c | 2348 +++
 3 files changed, 2358 insertions(+)
 create mode 100644 drivers/staging/media/imx/ov5640-mipi.c

diff --git a/drivers/staging/media/imx/Kconfig 
b/drivers/staging/media/imx/Kconfig
index ce2d2c8..09f373d 100644
--- a/drivers/staging/media/imx/Kconfig
+++ b/drivers/staging/media/imx/Kconfig
@@ -17,5 +17,13 @@ config VIDEO_IMX_CAMERA
---help---
  A video4linux camera capture driver for i.MX5/6.
 
+config IMX_OV5640_MIPI
+   tristate "OmniVision OV5640 MIPI CSI-2 camera support"
+   depends on GPIOLIB && VIDEO_IMX_CAMERA
+   select IMX_MIPI_CSI2
+   default y
+   ---help---
+ MIPI CSI-2 OV5640 Camera support.
+
 endmenu
 endif
diff --git a/drivers/staging/media/imx/Makefile 
b/drivers/staging/media/imx/Makefile
index 0decef7..aa954c1 100644
--- a/drivers/staging/media/imx/Makefile
+++ b/drivers/staging/media/imx/Makefile
@@ -10,3 +10,5 @@ obj-$(CONFIG_VIDEO_IMX_CAMERA) += imx-csi.o
 obj-$(CONFIG_VIDEO_IMX_CAMERA) += imx-smfc.o
 obj-$(CONFIG_VIDEO_IMX_CAMERA) += imx-camif.o
 obj-$(CONFIG_VIDEO_IMX_CAMERA) += imx-mipi-csi2.o
+
+obj-$(CONFIG_IMX_OV5640_MIPI) += ov5640-mipi.o
diff --git a/drivers/staging/media/imx/ov5640-mipi.c 
b/drivers/staging/media/imx/ov5640-mipi.c
new file mode 100644
index 000..54647a7
--- /dev/null
+++ b/drivers/staging/media/imx/ov5640-mipi.c
@@ -0,0 +1,2348 @@
+/*
+ * Copyright (c) 2014 Mentor Graphics Inc.
+ * Copyright (C) 2011-2013 Freescale Semiconductor, Inc. All Rights Reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#define OV5640_VOLTAGE_ANALOG   280
+#define OV5640_VOLTAGE_DIGITAL_CORE 150
+#define OV5640_VOLTAGE_DIGITAL_IO   180
+
+#define MIN_FPS 15
+#define MAX_FPS 30
+#define DEFAULT_FPS 30
+
+/* min/typical/max system clock (xclk) frequencies */
+#define OV5640_XCLK_MIN  600
+#define OV5640_XCLK_TYP 2400
+#define OV5640_XCLK_MAX 5400
+
+/* min/typical/max pixel clock (mclk) frequencies */
+#define OV5640_MCLK_MIN 4800
+#define OV5640_MCLK_TYP 4800
+#define OV5640_MCLK_MAX 9600
+
+#define OV5640_CHIP_ID  0x300A
+#define OV5640_SLAVE_ID 0x3100
+#define OV5640_DEFAULT_SLAVE_ID 0x3c
+
+#define OV5640_MAX_CONTROLS 64
+
+enum ov5640_mode {
+   ov5640_mode_MIN = 0,
+   ov5640_mode_QCIF_176_144 = 0,
+   ov5640_mode_QVGA_320_240,
+   ov5640_mode_VGA_640_480,
+   ov5640_mode_NTSC_720_480,
+   ov5640_mode_PAL_720_576,
+   ov5640_mode_XGA_1024_768,
+   ov5640_mode_720P_1280_720,
+   ov5640_mode_1080P_1920_1080,
+   ov5640_mode_QSXGA_2592_1944,
+   ov5640_num_modes,
+   ov5640_mode_INIT = 0xff, /*only for sensor init*/
+};
+
+enum ov5640_frame_rate {
+   ov5640_15_fps,
+   ov5640_30_fps
+};
+
+static int ov5640_framerates[] = {
+   [ov5640_15_fps] = 15,
+   [ov5640_30_fps] = 30,
+};
+#define ov5640_num_framerates ARRAY_SIZE(ov5640_framerates)
+
+/* image size under 1280 * 960 are SUBSAMPLING
+ * image size upper 1280 * 960 are SCALING
+ */
+enum ov5640_downsize_mode {
+   SUBSAMPLING,
+   SCALING,
+};
+
+struct reg_value {
+   u16 reg_addr;
+   u8 val;
+   u8 mask;
+   u32 delay_ms;
+};
+
+struct ov5640_mode_info {
+   enum ov5640_mode mode;
+   enum ov5640_downsize_mode dn_mode;
+   u32 width;
+   u32 height;
+   struct reg_value *init_data_ptr;
+   u32 init_data_size;
+};
+
+struct ov5640_dev {
+   struct i2c_client *i2c_client;
+   struct device *dev;
+   struct v4l2_subdev sd;
+   struct media_pad pad;
+   struct v4l2_ctrl_handler ctrl_hdl;
+   struct v4l2_of_endpoint ep; /* the parsed DT endpoint info */
+   struct v4l2_mbus_framefmt fmt;
+   struct v4l2_captureparm streamcap;
+   struct clk *xclk; /* system clock to OV5640 */
+   int xclk_freq;/* requested xclk freq from devicetree */
+
+   enum ov5640_mode current_mode;
+   enum ov5640_frame_rate current_fr;
+
+   bool on;
+   bool awb_on;
+   bool agc_on;
+
+   /* cached control settings */
+   int ctrl_cache[OV5640_MAX_CONTROLS];
+
+   struct gpio_desc *reset_gpio;
+   struct gpio_desc *pwdn_gpio;
+