Re: [PATCH v5 2/2] media:imx274 V4l2 driver for Sony imx274 CMOS sensor

2017-09-18 Thread Sakari Ailus 
Hi Leon,

Thanks for the update. Please see my comments below.

On Fri, Sep 15, 2017 at 03:49:52PM +0800, Leon Luo wrote:
> The imx274 is a Sony CMOS image sensor that has 1/2.5 image size.
> It supports up to 3840x2160 (4K) 60fps, 1080p 120fps. The interface
> is 4-lane MIPI CSI-2 running at 1.44Gbps each.
> 
> This driver has been tested on Xilinx ZCU102 platform with a Leopard
> LI-IMX274MIPI-FMC camera board.
> 
> Support for the following features:
> -Resolutions: 3840x2160, 1920x1080, 1280x720
> -Frame rate: 3840x2160 : 5 – 60fps
> 1920x1080 : 5 – 120fps
> 1280x720 : 5 – 120fps
> -Exposure time: 16 – (frame interval) micro-seconds
> -Gain: 1x - 180x
> -VFLIP: enable/disabledrivers/media/i2c/imx274.c
> -Test pattern: 12 test patterns
> 
> Signed-off-by: Leon Luo 
> Tested-by: Sören Brinkmann 
> ---
> v5:
>  - no changes
> v4:
>  - use 32-bit data type to avoid __divdi3 compile error for i386
>  - clean up OR together error codes
> v3:
>  - clean up header files
>  - use struct directly instead of alias #define
>  - use v4l2_ctrl_s_ctrl instead of v4l2_s_ctrl
>  - revise debug output
>  - move static helpers closer to their call site
>  - don't OR toegether error codes
>  - use closest valid gain value instead of erroring out
>  - assigne lock to the control handler and omit explicit locking
>  - acquire mutex lock for imx274_get_fmt
>  - remove format->pad check in imx274_set_fmt since the pad is always 0
>  - pass struct v4l2_ctrl pointer in gain/exposure/vlip/test pattern controls
>  - remove priv->ctrls.vflip->val = val in imx274_set_vflip()
>  - remove priv->ctrls.test_pattern->val = val in imx274_set_test_pattern()
>  - remove empty open/close callbacks
>  - remove empty core ops
>  - add more error labels in probe
>  - use v4l2_async_unregister_subdev instead of v4l2_device_unregister_subdev
>  - use dynamic debug
>  - split start_stream to two steps: imx274_mode_regs() and 
> imx274_start_stream()
>frame rate & exposure can be updated
>between imx274_mode_regs() & imx274_start_stream()
> 
> v2:
>  - Fix Kconfig to not remove existing options
> ---
>  drivers/media/i2c/Kconfig  |7 +
>  drivers/media/i2c/Makefile |1 +
>  drivers/media/i2c/imx274.c | 1808 
> 
>  3 files changed, 1816 insertions(+)
>  create mode 100644 drivers/media/i2c/imx274.c
> 
> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> index 94153895fcd4..ad2e70a02363 100644
> --- a/drivers/media/i2c/Kconfig
> +++ b/drivers/media/i2c/Kconfig
> @@ -547,6 +547,13 @@ config VIDEO_APTINA_PLL
>  config VIDEO_SMIAPP_PLL
>   tristate
>  
> +config VIDEO_IMX274
> + tristate "Sony IMX274 sensor support"
> + depends on I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API
> + ---help---
> +   This is a V4L2 sensor-level driver for the Sony IMX274
> +   CMOS image sensor.
> +
>  config VIDEO_OV2640
>   tristate "OmniVision OV2640 sensor support"
>   depends on VIDEO_V4L2 && I2C
> diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
> index c843c181dfb9..f8d57e453936 100644
> --- a/drivers/media/i2c/Makefile
> +++ b/drivers/media/i2c/Makefile
> @@ -92,5 +92,6 @@ 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_IMX274)   += imx274.o
>  
>  obj-$(CONFIG_SDR_MAX2175) += max2175.o
> diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
> new file mode 100644
> index ..66cbfa81c030
> --- /dev/null
> +++ b/drivers/media/i2c/imx274.c
> @@ -0,0 +1,1808 @@
> +/*
> + * imx274.c - IMX274 CMOS Image Sensor driver
> + *
> + * Copyright (C) 2017, Leopard Imaging, Inc.
> + *
> + * Leon Luo 
> + * Edwin Zou 
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see .
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include 
> +#include 
> +#include 

Alphabetical order, please.

> +#include 

Do you need this header?

> +
> +/*
> + * See "SHR, SVR Setting" in datasheet
> + */
> +#define IMX274_DEFAULT_FRAME_LENGTH  (4550)
> +#define IMX274_MAX_FRAME_LENGTH  (0x000f)
> +
> +/*
> + * See "Frame Rate Adjustment" in datasheet

[PATCH v5 2/2] media:imx274 V4l2 driver for Sony imx274 CMOS sensor

2017-09-15 Thread Leon Luo 
The imx274 is a Sony CMOS image sensor that has 1/2.5 image size.
It supports up to 3840x2160 (4K) 60fps, 1080p 120fps. The interface
is 4-lane MIPI CSI-2 running at 1.44Gbps each.

This driver has been tested on Xilinx ZCU102 platform with a Leopard
LI-IMX274MIPI-FMC camera board.

Support for the following features:
-Resolutions: 3840x2160, 1920x1080, 1280x720
-Frame rate: 3840x2160 : 5 – 60fps
1920x1080 : 5 – 120fps
1280x720 : 5 – 120fps
-Exposure time: 16 – (frame interval) micro-seconds
-Gain: 1x - 180x
-VFLIP: enable/disabledrivers/media/i2c/imx274.c
-Test pattern: 12 test patterns

Signed-off-by: Leon Luo 
Tested-by: Sören Brinkmann 
---
v5:
 - no changes
v4:
 - use 32-bit data type to avoid __divdi3 compile error for i386
 - clean up OR together error codes
v3:
 - clean up header files
 - use struct directly instead of alias #define
 - use v4l2_ctrl_s_ctrl instead of v4l2_s_ctrl
 - revise debug output
 - move static helpers closer to their call site
 - don't OR toegether error codes
 - use closest valid gain value instead of erroring out
 - assigne lock to the control handler and omit explicit locking
 - acquire mutex lock for imx274_get_fmt
 - remove format->pad check in imx274_set_fmt since the pad is always 0
 - pass struct v4l2_ctrl pointer in gain/exposure/vlip/test pattern controls
 - remove priv->ctrls.vflip->val = val in imx274_set_vflip()
 - remove priv->ctrls.test_pattern->val = val in imx274_set_test_pattern()
 - remove empty open/close callbacks
 - remove empty core ops
 - add more error labels in probe
 - use v4l2_async_unregister_subdev instead of v4l2_device_unregister_subdev
 - use dynamic debug
 - split start_stream to two steps: imx274_mode_regs() and imx274_start_stream()
   frame rate & exposure can be updated
   between imx274_mode_regs() & imx274_start_stream()

v2:
 - Fix Kconfig to not remove existing options
---
 drivers/media/i2c/Kconfig  |7 +
 drivers/media/i2c/Makefile |1 +
 drivers/media/i2c/imx274.c | 1808 
 3 files changed, 1816 insertions(+)
 create mode 100644 drivers/media/i2c/imx274.c

diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
index 94153895fcd4..ad2e70a02363 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -547,6 +547,13 @@ config VIDEO_APTINA_PLL
 config VIDEO_SMIAPP_PLL
tristate
 
+config VIDEO_IMX274
+   tristate "Sony IMX274 sensor support"
+   depends on I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API
+   ---help---
+ This is a V4L2 sensor-level driver for the Sony IMX274
+ CMOS image sensor.
+
 config VIDEO_OV2640
tristate "OmniVision OV2640 sensor support"
depends on VIDEO_V4L2 && I2C
diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
index c843c181dfb9..f8d57e453936 100644
--- a/drivers/media/i2c/Makefile
+++ b/drivers/media/i2c/Makefile
@@ -92,5 +92,6 @@ 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_IMX274) += imx274.o
 
 obj-$(CONFIG_SDR_MAX2175) += max2175.o
diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
new file mode 100644
index ..66cbfa81c030
--- /dev/null
+++ b/drivers/media/i2c/imx274.c
@@ -0,0 +1,1808 @@
+/*
+ * imx274.c - IMX274 CMOS Image Sensor driver
+ *
+ * Copyright (C) 2017, Leopard Imaging, Inc.
+ *
+ * Leon Luo 
+ * Edwin Zou 
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see .
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+#include 
+#include 
+#include 
+
+/*
+ * See "SHR, SVR Setting" in datasheet
+ */
+#define IMX274_DEFAULT_FRAME_LENGTH(4550)
+#define IMX274_MAX_FRAME_LENGTH(0x000f)
+
+/*
+ * See "Frame Rate Adjustment" in datasheet
+ */
+#define IMX274_PIXCLK_CONST1   (7200)
+#define IMX274_PIXCLK_CONST2   (100)
+
+/*
+ * The input gain is shifted by IMX274_GAIN_SHIFT to get
+ * decimal number. The real gain is
+ * (float)input_gain_value / (1 << IMX274_GAIN_SHIFT)
+ */
+#define IMX274_GAIN_SHIFT  (8)
+#define IMX274_GAIN_SHIFT_MASK ((1 << IMX274_GAIN_SHIFT) - 1)
+
+/*
+ * See "Analog Gain" a