subject:"\[PATCH v7\] media\: imx258\: Add imx258 camera sensor driver"

RE: [PATCH v7] media: imx258: Add imx258 camera sensor driver

2018-03-14 Thread Yeh, Andy

Hi Tomasz,

Thanks for the comments. OKAY as inline.
Please check the Patch V8.

Regards, Andy

-Original Message-
From: Tomasz Figa [mailto:tf...@chromium.org] 
Sent: Monday, March 12, 2018 3:33 PM
To: Yeh, Andy <andy@intel.com>
Cc: Linux Media Mailing List <linux-media@vger.kernel.org>; Sakari Ailus 
<sakari.ai...@linux.intel.com>; Chen, JasonX Z <jasonx.z.c...@intel.com>; 
Chiang, AlanX <alanx.chi...@intel.com>; Lai, Jim <jim....@intel.com>
Subject: Re: [PATCH v7] media: imx258: Add imx258 camera sensor driver

Hi Andy,

On Mon, Mar 12, 2018 at 1:01 AM, Andy Yeh <andy@intel.com> wrote:
>> Add a V4L2 sub-device driver for the Sony IMX258 image sensor.
>> This is a camera sensor using the I2C bus for control and the
>> CSI-2 bus for data.
>>
>> Signed-off-by: Jason Chen <jasonx.z.c...@intel.com>
>> Signed-off-by: Alan Chiang <alanx.chi...@intel.com>
>> ---
>> since v2:
>> -- Update the streaming function to remove SW_STANDBY in the beginning.
>> -- Adjust the delay time from 1ms to 12ms before set stream-on register.
>> since v3:
>> -- fix the sd.entity to make code be compiled on the mainline kernel.
>> since v4:
>> -- Enabled AG, DG, and Exposure time control correctly.
>> since v5:
>> -- Sensor vendor provided a new setting to fix different CLK issue
>> -- Add one more resolution for 1048x780, used for VGA streaming since 
>> v6:
>> -- improved i2c read/write function to support writing 2 registers
>> -- modified i2c reg read/write function with a more portable way
>> -- utilized v4l2_find_nearest_size instead of the local find_best_fit 
>> function
>> -- defined an enum for the link freq entries for explicit indexing
>
>Thanks for the patch. Looks almost good now. Just two new comments inline.
>
>[snip]
>> +   /* Set Orientation be 180 degree */
>> +   ret = imx258_write_reg(imx258, REG_MIRROR_FLIP_CONTROL,
>> +   IMX258_REG_VALUE_08BIT, 
>> REG_CONFIG_MIRROR_FLIP);
>> +   if (ret) {
>> +   dev_err(>dev, "%s failed to set orientation\n",
>> +   __func__);
>> +   return ret;
>> +   }
>> +
>> +   /* Apply customized values from user */
>> +   ret =  __v4l2_ctrl_handler_setup(imx258->sd.ctrl_handler);
>> +   if (ret)
>> +   return ret;
>> +
>> +   /*
>> +* Per sensor datasheet:
>> +* These are the minimum 12ms delays for accessing the sensor through
>> +* I2C and enabling streaming after lifting the device from reset.
>> +*/
>> +   usleep_range(12000, 13000);
>
>Hmm, the code above already accesses the sensor through I2C. If this works, is 
>this sleep perhaps already done by ACPI code or it is only needed for 
>streaming?
>

Okay.
usleep removed since confirmed sufficient delay implemented in coreboot. Stress 
test passed.

>[snip]
>> +/* Initialize control handlers */
>> +static int imx258_init_controls(struct imx258 *imx258) {
>> +   struct i2c_client *client = v4l2_get_subdevdata(>sd);
>> +   struct v4l2_ctrl_handler *ctrl_hdlr;
>> +   s64 exposure_max;
>> +   s64 vblank_def;
>> +   s64 vblank_min;
>> +   s64 pixel_rate_min;
>> +   s64 pixel_rate_max;
>> +   int ret;
>> +
>> +   ctrl_hdlr = >ctrl_handler;
>> +   ret = v4l2_ctrl_handler_init(ctrl_hdlr, 8);
>> +   if (ret)
>> +   return ret;
>> +
>> +   mutex_init(>mutex);
>> +   ctrl_hdlr->lock = >mutex;
>> +   imx258->link_freq = v4l2_ctrl_new_int_menu(ctrl_hdlr,
>> +   _ctrl_ops,
>> +   V4L2_CID_LINK_FREQ,
>> +   ARRAY_SIZE(link_freq_menu_items) - 1,
>> +   0,
>> +   link_freq_menu_items);
>> +
>> +   if (!imx258->link_freq) {
>> +   ret = -EINVAL;
>> +   goto error;
>> +   }
>> +
>> +   imx258->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
>
>nit: As discussed earlier with Sakari, I think we agreed on making this, and 
>other controls that need dereferencing here, as follows:
>
>imx258->link_freq = v4l2_ctrl_new_int_menu(ctrl_hdlr,
>_ctrl_ops,
>V4L2_CID_LINK_FREQ,
>ARRAY_SIZE(link_freq_menu_items) - 1,
>0,
>link_freq_menu_items);
>if (imx258->link_freq)
>imx258->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
>
>The error will be reported at the end of this function, through the check for 
>ctrl_hdlr->error.
>
>
OKAY


Best regards,
Tomasz

Re: [PATCH v7] media: imx258: Add imx258 camera sensor driver

2018-03-12 Thread Tomasz Figa

Hi Andy,

On Mon, Mar 12, 2018 at 1:01 AM, Andy Yeh  wrote:
> Add a V4L2 sub-device driver for the Sony IMX258 image sensor.
> This is a camera sensor using the I2C bus for control and the
> CSI-2 bus for data.
>
> Signed-off-by: Jason Chen 
> Signed-off-by: Alan Chiang 
> ---
> since v2:
> -- Update the streaming function to remove SW_STANDBY in the beginning.
> -- Adjust the delay time from 1ms to 12ms before set stream-on register.
> since v3:
> -- fix the sd.entity to make code be compiled on the mainline kernel.
> since v4:
> -- Enabled AG, DG, and Exposure time control correctly.
> since v5:
> -- Sensor vendor provided a new setting to fix different CLK issue
> -- Add one more resolution for 1048x780, used for VGA streaming
> since v6:
> -- improved i2c read/write function to support writing 2 registers
> -- modified i2c reg read/write function with a more portable way
> -- utilized v4l2_find_nearest_size instead of the local find_best_fit function
> -- defined an enum for the link freq entries for explicit indexing

Thanks for the patch. Looks almost good now. Just two new comments inline.

[snip]
> +   /* Set Orientation be 180 degree */
> +   ret = imx258_write_reg(imx258, REG_MIRROR_FLIP_CONTROL,
> +   IMX258_REG_VALUE_08BIT, 
> REG_CONFIG_MIRROR_FLIP);
> +   if (ret) {
> +   dev_err(>dev, "%s failed to set orientation\n",
> +   __func__);
> +   return ret;
> +   }
> +
> +   /* Apply customized values from user */
> +   ret =  __v4l2_ctrl_handler_setup(imx258->sd.ctrl_handler);
> +   if (ret)
> +   return ret;
> +
> +   /*
> +* Per sensor datasheet:
> +* These are the minimum 12ms delays for accessing the sensor through
> +* I2C and enabling streaming after lifting the device from reset.
> +*/
> +   usleep_range(12000, 13000);

Hmm, the code above already accesses the sensor through I2C. If this
works, is this sleep perhaps already done by ACPI code or it is only
needed for streaming?

[snip]
> +/* Initialize control handlers */
> +static int imx258_init_controls(struct imx258 *imx258)
> +{
> +   struct i2c_client *client = v4l2_get_subdevdata(>sd);
> +   struct v4l2_ctrl_handler *ctrl_hdlr;
> +   s64 exposure_max;
> +   s64 vblank_def;
> +   s64 vblank_min;
> +   s64 pixel_rate_min;
> +   s64 pixel_rate_max;
> +   int ret;
> +
> +   ctrl_hdlr = >ctrl_handler;
> +   ret = v4l2_ctrl_handler_init(ctrl_hdlr, 8);
> +   if (ret)
> +   return ret;
> +
> +   mutex_init(>mutex);
> +   ctrl_hdlr->lock = >mutex;
> +   imx258->link_freq = v4l2_ctrl_new_int_menu(ctrl_hdlr,
> +   _ctrl_ops,
> +   V4L2_CID_LINK_FREQ,
> +   ARRAY_SIZE(link_freq_menu_items) - 1,
> +   0,
> +   link_freq_menu_items);
> +
> +   if (!imx258->link_freq) {
> +   ret = -EINVAL;
> +   goto error;
> +   }
> +
> +   imx258->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;

nit: As discussed earlier with Sakari, I think we agreed on making
this, and other controls that need dereferencing here, as follows:

imx258->link_freq = v4l2_ctrl_new_int_menu(ctrl_hdlr,
_ctrl_ops,
V4L2_CID_LINK_FREQ,
ARRAY_SIZE(link_freq_menu_items) - 1,
0,
link_freq_menu_items);
if (imx258->link_freq)
imx258->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;

The error will be reported at the end of this function, through the
check for ctrl_hdlr->error.

Best regards,
Tomasz

RE: [PATCH v7] media: imx258: Add imx258 camera sensor driver

2018-03-11 Thread Yeh, Andy

Dear all,

Please kindly check this v7.
I noticed many incorrect line breaks again in this mail which was not identical 
to the original patch. Not sure how it was resulted.
Such as 
-- Add one more resolution for 1048x780, used for VGA streaming "\n" since v6:
-- improved i2c read/write function to support writing 2 registers

+/* Mode : resolution and related config */ "\n" struct imx258_mode {

Therefore please help check the patch on the list, which is with correct line 
break.
https://patchwork.linuxtv.org/patch/47869/

Sorry to bring you inconvenience.


Regards, Andy

-Original Message-
From: Yeh, Andy 
Sent: Monday, March 12, 2018 12:02 AM
To: linux-media@vger.kernel.org; tf...@chromium.org
Cc: sakari.ai...@linux.intel.com; Yeh, Andy <andy@intel.com>; Chen, JasonX 
Z <jasonx.z.c...@intel.com>; Chiang, AlanX <alanx.chi...@intel.com>; Lai, Jim 
<jim....@intel.com>
Subject: [PATCH v7] media: imx258: Add imx258 camera sensor driver

Add a V4L2 sub-device driver for the Sony IMX258 image sensor.
This is a camera sensor using the I2C bus for control and the
CSI-2 bus for data.

Signed-off-by: Jason Chen <jasonx.z.c...@intel.com>
Signed-off-by: Alan Chiang <alanx.chi...@intel.com>
---
since v2:
-- Update the streaming function to remove SW_STANDBY in the beginning.
-- Adjust the delay time from 1ms to 12ms before set stream-on register.
since v3:
-- fix the sd.entity to make code be compiled on the mainline kernel.
since v4:
-- Enabled AG, DG, and Exposure time control correctly.
since v5:
-- Sensor vendor provided a new setting to fix different CLK issue
-- Add one more resolution for 1048x780, used for VGA streaming since v6:
-- improved i2c read/write function to support writing 2 registers
-- modified i2c reg read/write function with a more portable way
-- utilized v4l2_find_nearest_size instead of the local find_best_fit function
-- defined an enum for the link freq entries for explicit indexing

 MAINTAINERS|7 +
 drivers/media/i2c/Kconfig  |   11 +
 drivers/media/i2c/Makefile |1 +
 drivers/media/i2c/imx258.c | 1299 
 4 files changed, 1318 insertions(+)
 create mode 100644 drivers/media/i2c/imx258.c

diff --git a/MAINTAINERS b/MAINTAINERS
index a339bb5..9f75510 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12646,6 +12646,13 @@ S: Maintained
 F: drivers/ssb/
 F: include/linux/ssb/
 
+SONY IMX258 SENSOR DRIVER
+M: Sakari Ailus <sakari.ai...@linux.intel.com>
+L: linux-media@vger.kernel.org
+T: git git://linuxtv.org/media_tree.git
+S: Maintained
+F: drivers/media/i2c/imx258.c
+
 SONY IMX274 SENSOR DRIVER
 M: Leon Luo <le...@leopardimaging.com>
 L: linux-media@vger.kernel.org
diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig index 
fd01842..bcd4bf1 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -565,6 +565,17 @@ config VIDEO_APTINA_PLL  config VIDEO_SMIAPP_PLL
tristate
 
+config VIDEO_IMX258
+   tristate "Sony IMX258 sensor support"
+   depends on I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API
+   depends on MEDIA_CAMERA_SUPPORT
+   ---help---
+ This is a Video4Linux2 sensor-level driver for the Sony
+ IMX258 camera.
+
+ To compile this driver as a module, choose M here: the
+ module will be called imx258.
+
 config VIDEO_IMX274
tristate "Sony IMX274 sensor support"
depends on I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API diff --git 
a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile index 
1b62639..4bf7d00 100644
--- a/drivers/media/i2c/Makefile
+++ b/drivers/media/i2c/Makefile
@@ -94,6 +94,7 @@ 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_IMX258) += imx258.o
 obj-$(CONFIG_VIDEO_IMX274) += imx274.o
 
 obj-$(CONFIG_SDR_MAX2175) += max2175.o
diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c new file 
mode 100644 index 000..6e695f1
--- /dev/null
+++ b/drivers/media/i2c/imx258.c
@@ -0,0 +1,1299 @@
+// Copyright (C) 2018 Intel Corporation // SPDX-License-Identifier: 
+GPL-2.0
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#define IMX258_REG_VALUE_08BIT 1
+#define IMX258_REG_VALUE_16BIT 2
+
+#define IMX258_REG_MODE_SELECT 0x0100
+#define IMX258_MODE_STANDBY0x00
+#define IMX258_MODE_STREAMING  0x01
+
+/* Chip ID */
+#define IMX258_REG_CHIP_ID 0x0016
+#define IMX258_CHIP_ID 0x0258
+
+/* V_TIMING internal */
+#define IMX258_REG_VTS 0x0340
+#define IMX258_VTS_30FPS   0x0c98
+#define IMX258_VTS_MAX 0x
+
+/*Fr

[PATCH v7] media: imx258: Add imx258 camera sensor driver

2018-03-11 Thread Andy Yeh

Add a V4L2 sub-device driver for the Sony IMX258 image sensor.
This is a camera sensor using the I2C bus for control and the
CSI-2 bus for data.

Signed-off-by: Jason Chen 
Signed-off-by: Alan Chiang 
---
since v2:
-- Update the streaming function to remove SW_STANDBY in the beginning.
-- Adjust the delay time from 1ms to 12ms before set stream-on register.
since v3:
-- fix the sd.entity to make code be compiled on the mainline kernel.
since v4:
-- Enabled AG, DG, and Exposure time control correctly.
since v5:
-- Sensor vendor provided a new setting to fix different CLK issue
-- Add one more resolution for 1048x780, used for VGA streaming
since v6:
-- improved i2c read/write function to support writing 2 registers
-- modified i2c reg read/write function with a more portable way
-- utilized v4l2_find_nearest_size instead of the local find_best_fit function
-- defined an enum for the link freq entries for explicit indexing

 MAINTAINERS|7 +
 drivers/media/i2c/Kconfig  |   11 +
 drivers/media/i2c/Makefile |1 +
 drivers/media/i2c/imx258.c | 1299 
 4 files changed, 1318 insertions(+)
 create mode 100644 drivers/media/i2c/imx258.c

diff --git a/MAINTAINERS b/MAINTAINERS
index a339bb5..9f75510 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12646,6 +12646,13 @@ S: Maintained
 F: drivers/ssb/
 F: include/linux/ssb/
 
+SONY IMX258 SENSOR DRIVER
+M: Sakari Ailus 
+L: linux-media@vger.kernel.org
+T: git git://linuxtv.org/media_tree.git
+S: Maintained
+F: drivers/media/i2c/imx258.c
+
 SONY IMX274 SENSOR DRIVER
 M: Leon Luo 
 L: linux-media@vger.kernel.org
diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
index fd01842..bcd4bf1 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -565,6 +565,17 @@ config VIDEO_APTINA_PLL
 config VIDEO_SMIAPP_PLL
tristate
 
+config VIDEO_IMX258
+   tristate "Sony IMX258 sensor support"
+   depends on I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API
+   depends on MEDIA_CAMERA_SUPPORT
+   ---help---
+ This is a Video4Linux2 sensor-level driver for the Sony
+ IMX258 camera.
+
+ To compile this driver as a module, choose M here: the
+ module will be called imx258.
+
 config VIDEO_IMX274
tristate "Sony IMX274 sensor support"
depends on I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API
diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
index 1b62639..4bf7d00 100644
--- a/drivers/media/i2c/Makefile
+++ b/drivers/media/i2c/Makefile
@@ -94,6 +94,7 @@ 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_IMX258) += imx258.o
 obj-$(CONFIG_VIDEO_IMX274) += imx274.o
 
 obj-$(CONFIG_SDR_MAX2175) += max2175.o
diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c
new file mode 100644
index 000..6e695f1
--- /dev/null
+++ b/drivers/media/i2c/imx258.c
@@ -0,0 +1,1299 @@
+// Copyright (C) 2018 Intel Corporation
+// SPDX-License-Identifier: GPL-2.0
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#define IMX258_REG_VALUE_08BIT 1
+#define IMX258_REG_VALUE_16BIT 2
+
+#define IMX258_REG_MODE_SELECT 0x0100
+#define IMX258_MODE_STANDBY0x00
+#define IMX258_MODE_STREAMING  0x01
+
+/* Chip ID */
+#define IMX258_REG_CHIP_ID 0x0016
+#define IMX258_CHIP_ID 0x0258
+
+/* V_TIMING internal */
+#define IMX258_REG_VTS 0x0340
+#define IMX258_VTS_30FPS   0x0c98
+#define IMX258_VTS_MAX 0x
+
+/*Frame Length Line*/
+#define IMX258_FLL_MIN 0x08a6
+#define IMX258_FLL_MAX 0x
+#define IMX258_FLL_STEP1
+#define IMX258_FLL_DEFAULT 0x0c98
+
+/* HBLANK control - read only */
+#define IMX258_PPL_DEFAULT 5352
+
+/* Exposure control */
+#define IMX258_REG_EXPOSURE0x0202
+#define IMX258_EXPOSURE_MIN4
+#define IMX258_EXPOSURE_STEP   1
+#define IMX258_EXPOSURE_DEFAULT0x640
+#define IMX258_EXPOSURE_MAX65535
+
+/* Analog gain control */
+#define IMX258_REG_ANALOG_GAIN 0x0204
+#define IMX258_ANA_GAIN_MIN0
+#define IMX258_ANA_GAIN_MAX0x1fff
+#define IMX258_ANA_GAIN_STEP   1
+#define IMX258_ANA_GAIN_DEFAULT0x0
+
+/* Digital gain control */
+#define IMX258_REG_GR_DIGITAL_GAIN 0x020e
+#define IMX258_REG_R_DIGITAL_GAIN  0x0210
+#define IMX258_REG_B_DIGITAL_GAIN  0x0212
+#define IMX258_REG_GB_DIGITAL_GAIN 0x0214
+#define IMX258_DGTL_GAIN_MIN   0
+#define IMX258_DGTL_GAIN_MAX

RE: [PATCH v7] media: imx258: Add imx258 camera sensor driver

Re: [PATCH v7] media: imx258: Add imx258 camera sensor driver

RE: [PATCH v7] media: imx258: Add imx258 camera sensor driver

[PATCH v7] media: imx258: Add imx258 camera sensor driver

4 matches

Site Navigation

Mail list logo

Footer information