Re: [PATCH v3] media: imx319: Add imx319 camera sensor driver

[jacopo mondi]

Hi Tomasz,

On Fri, May 25, 2018 at 04:31:07PM +0900, Tomasz Figa wrote:
> On Fri, May 25, 2018 at 4:12 PM jacopo mondi  wrote:
>
> > Hi Tomasz,
>

[snip]

> > > the controls set before powering on will actually be programmed into the
> > > hardware registers.
>
> > Thanks, I had missed that part.
>
> > I quickly tried searching for 's_ctrl' calls in the v4l2-core/ code
> > and I've found nothing that invokes that in response to a streaming
> > start operation. Just if you happen to have any reference handy, could
> > you please point me to that part, just for my better understanding?
>
> The driver does it itself by calling __v4l2_ctrl_handler_setup() from its
> .start_streaming() callback.

Thanks, I'm not sure how I've missed that part. Thanks and sorry for
the fuss!



signature.asc
Description: PGP signature

Re: [PATCH v3] media: imx319: Add imx319 camera sensor driver

[Tomasz Figa]

On Fri, May 25, 2018 at 4:12 PM jacopo mondi  wrote:

> Hi Tomasz,

> On Fri, May 25, 2018 at 03:18:38PM +0900, Tomasz Figa wrote:
> > On Fri, May 25, 2018 at 5:47 AM Sakari Ailus <
sakari.ai...@linux.intel.com>
> > wrote:
> >
> > > Hi Jacopo,
> >
> > > On Thu, May 24, 2018 at 10:07:38PM +0200, jacopo mondi wrote:
> > > ...
> > > > > > about that, but I wonder why setting controls should be enabled
only
> > > > > > when streaming. I would have expected runtime_pm_get/put in
> > subdevices
> > > > > > node open/close functions not only when streaming. Am I missing
> > something?
> > > > >
> > > > > You can do it either way. If powering on the sensor takes a long
> > time, then
> > > > > doing that in the open callback may be helpful as the user space
has
> > a way
> > > > > to keep the device powered.
> > > >
> > > > Ok, so I assume my comment could be ignored, assuming is fine not
> > > > being able to set control if the sensor is not streaming. Is it?
> >
> > > I'd say so. From the user's point of view, the sensor doesn't really
do
> > > anything when it's in software standby mode.
> >
> > Just to make sure we're on the same page, the code actually does nothing
> > when the sensor is not in streaming mode (i.e. powered off). When
stream is
> > being started, the V4L2 control framework will call s_ctrl for all the
> > controls in the handler to synchronize hardware state and this is when
all
> > the controls set before powering on will actually be programmed into the
> > hardware registers.

> Thanks, I had missed that part.

> I quickly tried searching for 's_ctrl' calls in the v4l2-core/ code
> and I've found nothing that invokes that in response to a streaming
> start operation. Just if you happen to have any reference handy, could
> you please point me to that part, just for my better understanding?

The driver does it itself by calling __v4l2_ctrl_handler_setup() from its
.start_streaming() callback.

Best regards,
Tomasz

Re: [PATCH v3] media: imx319: Add imx319 camera sensor driver

[jacopo mondi]

Hi Tomasz,

On Fri, May 25, 2018 at 03:18:38PM +0900, Tomasz Figa wrote:
> On Fri, May 25, 2018 at 5:47 AM Sakari Ailus 
> wrote:
>
> > Hi Jacopo,
>
> > On Thu, May 24, 2018 at 10:07:38PM +0200, jacopo mondi wrote:
> > ...
> > > > > about that, but I wonder why setting controls should be enabled only
> > > > > when streaming. I would have expected runtime_pm_get/put in
> subdevices
> > > > > node open/close functions not only when streaming. Am I missing
> something?
> > > >
> > > > You can do it either way. If powering on the sensor takes a long
> time, then
> > > > doing that in the open callback may be helpful as the user space has
> a way
> > > > to keep the device powered.
> > >
> > > Ok, so I assume my comment could be ignored, assuming is fine not
> > > being able to set control if the sensor is not streaming. Is it?
>
> > I'd say so. From the user's point of view, the sensor doesn't really do
> > anything when it's in software standby mode.
>
> Just to make sure we're on the same page, the code actually does nothing
> when the sensor is not in streaming mode (i.e. powered off). When stream is
> being started, the V4L2 control framework will call s_ctrl for all the
> controls in the handler to synchronize hardware state and this is when all
> the controls set before powering on will actually be programmed into the
> hardware registers.

Thanks, I had missed that part.

I quickly tried searching for 's_ctrl' calls in the v4l2-core/ code
and I've found nothing that invokes that in response to a streaming
start operation. Just if you happen to have any reference handy, could
you please point me to that part, just for my better understanding?

Thanks
   j

>
> Best regards,
> Tomasz


signature.asc
Description: PGP signature

Re: [PATCH v3] media: imx319: Add imx319 camera sensor driver

[Tomasz Figa]

On Fri, May 25, 2018 at 5:47 AM Sakari Ailus 
wrote:

> Hi Jacopo,

> On Thu, May 24, 2018 at 10:07:38PM +0200, jacopo mondi wrote:
> ...
> > > > about that, but I wonder why setting controls should be enabled only
> > > > when streaming. I would have expected runtime_pm_get/put in
subdevices
> > > > node open/close functions not only when streaming. Am I missing
something?
> > >
> > > You can do it either way. If powering on the sensor takes a long
time, then
> > > doing that in the open callback may be helpful as the user space has
a way
> > > to keep the device powered.
> >
> > Ok, so I assume my comment could be ignored, assuming is fine not
> > being able to set control if the sensor is not streaming. Is it?

> I'd say so. From the user's point of view, the sensor doesn't really do
> anything when it's in software standby mode.

Just to make sure we're on the same page, the code actually does nothing
when the sensor is not in streaming mode (i.e. powered off). When stream is
being started, the V4L2 control framework will call s_ctrl for all the
controls in the handler to synchronize hardware state and this is when all
the controls set before powering on will actually be programmed into the
hardware registers.

Best regards,
Tomasz

Re: [PATCH v3] media: imx319: Add imx319 camera sensor driver

[Sakari Ailus]

Hi Jacopo,

On Thu, May 24, 2018 at 10:07:38PM +0200, jacopo mondi wrote:
...
> > > about that, but I wonder why setting controls should be enabled only
> > > when streaming. I would have expected runtime_pm_get/put in subdevices
> > > node open/close functions not only when streaming. Am I missing something?
> >
> > You can do it either way. If powering on the sensor takes a long time, then
> > doing that in the open callback may be helpful as the user space has a way
> > to keep the device powered.
> 
> Ok, so I assume my comment could be ignored, assuming is fine not
> being able to set control if the sensor is not streaming. Is it?

I'd say so. From the user's point of view, the sensor doesn't really do
anything when it's in software standby mode.

...

> > > > +   mode = v4l2_find_nearest_size(supported_modes,
> > > > +   ARRAY_SIZE(supported_modes), width, height,
> > > > +   fmt->format.width, fmt->format.height);
> > > > +   imx319_update_pad_format(imx319, mode, fmt);
> > > > +   if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
> > > > +   framefmt = v4l2_subdev_get_try_format(sd, cfg, 
> > > > fmt->pad);
> > > > +   *framefmt = fmt->format;
> > > > +   } else {
> > > > +   imx319->cur_mode = mode;
> > > > +   pixel_rate =
> > > > +   (link_freq_menu_items[0] * 2 * 4) / 10;
> > >
> > > This assumes a fixed link frequency and a fixed number of data lanes,
> > > and a fixed bpp value (but this is ok, as all the formats you have are
> > > 10bpp). In OF world those parameters come from DT, what about ACPI?
> >
> > I presume the driver only supports a particular number of lanes (4). ACPI
> > supports _DSD properties, i.e. the same can be done on ACPI.
> >
> > If the driver only supports these, then it should check this matches with
> > what the firmware (ACPI) has. The fwnode API is the same.
> 
> Thanks, so I assume those parameters represented in ACPI DSD nodes
> will be checked to be supported by the sensor in v2.

Agreed.

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

Re: [PATCH v3] media: imx319: Add imx319 camera sensor driver

[jacopo mondi]

Hi Sakari,

On Wed, May 23, 2018 at 10:38:33AM +0300, Sakari Ailus wrote:
> Hi Jacopo and Bingbu,
>
> On Tue, May 22, 2018 at 10:08:48PM +0200, jacopo mondi wrote:
> ...
> > > +/* Get bayer order based on flip setting. */
> > > +static __u32 imx319_get_format_code(struct imx319 *imx319)
> > > +{
> > > + /*
> > > +  * Only one bayer order is supported.
> > > +  * It depends on the flip settings.
> > > +  */
> > > + static const __u32 codes[2][2] = {
> > > + { MEDIA_BUS_FMT_SRGGB10_1X10, MEDIA_BUS_FMT_SGRBG10_1X10, },
> > > + { MEDIA_BUS_FMT_SGBRG10_1X10, MEDIA_BUS_FMT_SBGGR10_1X10, },
> > > + };
> > > +
> > > + return codes[imx319->vflip->val][imx319->hflip->val];
> > > +}
> >
> > I don't have any major comment actually, this is pretty good for a
> > first submission.
> >
> > This worries me a bit though. The media bus format depends on the
> > V/HFLIP value, I assume this is an hardware limitation. But if
> > changing the flip changes the reported media bus format, you could
> > trigger a -EPIPE error during pipeline format validation between two
> > streaming sessions with different flip settings. Isn't this a bit
> > dangerous?
>
> That's how it works on raw bayer sensors; you do have to configure the
> entire pipeline accordingly.
>
> What it also means is that the two controls may not be changed during
> streaming --- this needs to be prevented by the driver, and I think it's
> missing at the moment.

Thanks for explaining. At least my comment lead to something, as my
understanding is that VFLIP and HFLIP should be forbidden when
the sensor is streaming
>
> >
> > Below some minor comments.
> >
> > > +
> > > +/* Read registers up to 4 at a time */
> > > +static int imx319_read_reg(struct imx319 *imx319, u16 reg, u32 len, u32 
> > > *val)
> > > +{
> > > + struct i2c_client *client = v4l2_get_subdevdata(>sd);
> > > + struct i2c_msg msgs[2];
> > > + u8 addr_buf[2];
> > > + u8 data_buf[4] = { 0 };
> > > + int ret;
> > > +
> > > + if (len > 4)
> > > + return -EINVAL;
> > > +
> > > + put_unaligned_be16(reg, addr_buf);
> > > + /* Write register address */
> > > + msgs[0].addr = client->addr;
> > > + msgs[0].flags = 0;
> > > + msgs[0].len = ARRAY_SIZE(addr_buf);
> > > + msgs[0].buf = addr_buf;
> > > +
> > > + /* Read data from register */
> > > + msgs[1].addr = client->addr;
> > > + msgs[1].flags = I2C_M_RD;
> > > + msgs[1].len = len;
> > > + msgs[1].buf = _buf[4 - len];
> > > +
> > > + ret = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
> > > + if (ret != ARRAY_SIZE(msgs))
> > > + return -EIO;
> > > +
> > > + *val = get_unaligned_be32(data_buf);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +/* Write registers up to 4 at a time */
> > > +static int imx319_write_reg(struct imx319 *imx319, u16 reg, u32 len, u32 
> > > val)
> > > +{
> > > + struct i2c_client *client = v4l2_get_subdevdata(>sd);
> > > + u8 buf[6];
> > > +
> > > + if (len > 4)
> > > + return -EINVAL;
> > > +
> > > + put_unaligned_be16(reg, buf);
> > > + put_unaligned_be32(val << (8 * (4 - len)), buf + 2);
> > > + if (i2c_master_send(client, buf, len + 2) != len + 2)
> > > + return -EIO;
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +/* Write a list of registers */
> > > +static int imx319_write_regs(struct imx319 *imx319,
> > > +   const struct imx319_reg *regs, u32 len)
> > > +{
> > > + struct i2c_client *client = v4l2_get_subdevdata(>sd);
> > > + int ret;
> > > + u32 i;
> > > +
> > > + for (i = 0; i < len; i++) {
> > > + ret = imx319_write_reg(imx319, regs[i].address, 1,
> > > + regs[i].val);
> >
> > Unaligned to open parenthesis
> >
> > > + if (ret) {
> > > + dev_err_ratelimited(
> > > + >dev,
> > > + "Failed to write reg 0x%4.4x. error = %d\n",
> > > + regs[i].address, ret);
> >
> > No need to break line, align to open parenthesis, please.
> >
> > > +
> > > + return ret;
> > > + }
> > > + }
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +/* Open sub-device */
> > > +static int imx319_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
> > > +{
> > > + struct imx319 *imx319 = to_imx319(sd);
> > > + struct v4l2_mbus_framefmt *try_fmt =
> > > + v4l2_subdev_get_try_format(sd, fh->pad, 0);
> > > +
> > > + mutex_lock(>mutex);
> > > +
> > > + /* Initialize try_fmt */
> > > + try_fmt->width = imx319->cur_mode->width;
> > > + try_fmt->height = imx319->cur_mode->height;
> > > + try_fmt->code = imx319_get_format_code(imx319);
>
> The initial try format should reflect the default, not the current
> configuration.
>
> > > + try_fmt->field = V4L2_FIELD_NONE;
> > > +
> > > + mutex_unlock(>mutex);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static int imx319_update_digital_gain(struct imx319 *imx319, u32 d_gain)
> > > +{
> > > + int ret;
> > > +
> > > + ret = imx319_write_reg(imx319, 

Re: [PATCH v3] media: imx319: Add imx319 camera sensor driver

[Sakari Ailus]

Hi Jacopo and Bingbu,

On Tue, May 22, 2018 at 10:08:48PM +0200, jacopo mondi wrote:
...
> > +/* Get bayer order based on flip setting. */
> > +static __u32 imx319_get_format_code(struct imx319 *imx319)
> > +{
> > +   /*
> > +* Only one bayer order is supported.
> > +* It depends on the flip settings.
> > +*/
> > +   static const __u32 codes[2][2] = {
> > +   { MEDIA_BUS_FMT_SRGGB10_1X10, MEDIA_BUS_FMT_SGRBG10_1X10, },
> > +   { MEDIA_BUS_FMT_SGBRG10_1X10, MEDIA_BUS_FMT_SBGGR10_1X10, },
> > +   };
> > +
> > +   return codes[imx319->vflip->val][imx319->hflip->val];
> > +}
> 
> I don't have any major comment actually, this is pretty good for a
> first submission.
> 
> This worries me a bit though. The media bus format depends on the
> V/HFLIP value, I assume this is an hardware limitation. But if
> changing the flip changes the reported media bus format, you could
> trigger a -EPIPE error during pipeline format validation between two
> streaming sessions with different flip settings. Isn't this a bit
> dangerous?

That's how it works on raw bayer sensors; you do have to configure the
entire pipeline accordingly.

What it also means is that the two controls may not be changed during
streaming --- this needs to be prevented by the driver, and I think it's
missing at the moment.

> 
> Below some minor comments.
> 
> > +
> > +/* Read registers up to 4 at a time */
> > +static int imx319_read_reg(struct imx319 *imx319, u16 reg, u32 len, u32 
> > *val)
> > +{
> > +   struct i2c_client *client = v4l2_get_subdevdata(>sd);
> > +   struct i2c_msg msgs[2];
> > +   u8 addr_buf[2];
> > +   u8 data_buf[4] = { 0 };
> > +   int ret;
> > +
> > +   if (len > 4)
> > +   return -EINVAL;
> > +
> > +   put_unaligned_be16(reg, addr_buf);
> > +   /* Write register address */
> > +   msgs[0].addr = client->addr;
> > +   msgs[0].flags = 0;
> > +   msgs[0].len = ARRAY_SIZE(addr_buf);
> > +   msgs[0].buf = addr_buf;
> > +
> > +   /* Read data from register */
> > +   msgs[1].addr = client->addr;
> > +   msgs[1].flags = I2C_M_RD;
> > +   msgs[1].len = len;
> > +   msgs[1].buf = _buf[4 - len];
> > +
> > +   ret = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
> > +   if (ret != ARRAY_SIZE(msgs))
> > +   return -EIO;
> > +
> > +   *val = get_unaligned_be32(data_buf);
> > +
> > +   return 0;
> > +}
> > +
> > +/* Write registers up to 4 at a time */
> > +static int imx319_write_reg(struct imx319 *imx319, u16 reg, u32 len, u32 
> > val)
> > +{
> > +   struct i2c_client *client = v4l2_get_subdevdata(>sd);
> > +   u8 buf[6];
> > +
> > +   if (len > 4)
> > +   return -EINVAL;
> > +
> > +   put_unaligned_be16(reg, buf);
> > +   put_unaligned_be32(val << (8 * (4 - len)), buf + 2);
> > +   if (i2c_master_send(client, buf, len + 2) != len + 2)
> > +   return -EIO;
> > +
> > +   return 0;
> > +}
> > +
> > +/* Write a list of registers */
> > +static int imx319_write_regs(struct imx319 *imx319,
> > + const struct imx319_reg *regs, u32 len)
> > +{
> > +   struct i2c_client *client = v4l2_get_subdevdata(>sd);
> > +   int ret;
> > +   u32 i;
> > +
> > +   for (i = 0; i < len; i++) {
> > +   ret = imx319_write_reg(imx319, regs[i].address, 1,
> > +   regs[i].val);
> 
> Unaligned to open parenthesis
> 
> > +   if (ret) {
> > +   dev_err_ratelimited(
> > +   >dev,
> > +   "Failed to write reg 0x%4.4x. error = %d\n",
> > +   regs[i].address, ret);
> 
> No need to break line, align to open parenthesis, please.
> 
> > +
> > +   return ret;
> > +   }
> > +   }
> > +
> > +   return 0;
> > +}
> > +
> > +/* Open sub-device */
> > +static int imx319_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
> > +{
> > +   struct imx319 *imx319 = to_imx319(sd);
> > +   struct v4l2_mbus_framefmt *try_fmt =
> > +   v4l2_subdev_get_try_format(sd, fh->pad, 0);
> > +
> > +   mutex_lock(>mutex);
> > +
> > +   /* Initialize try_fmt */
> > +   try_fmt->width = imx319->cur_mode->width;
> > +   try_fmt->height = imx319->cur_mode->height;
> > +   try_fmt->code = imx319_get_format_code(imx319);

The initial try format should reflect the default, not the current
configuration.

> > +   try_fmt->field = V4L2_FIELD_NONE;
> > +
> > +   mutex_unlock(>mutex);
> > +
> > +   return 0;
> > +}
> > +
> > +static int imx319_update_digital_gain(struct imx319 *imx319, u32 d_gain)
> > +{
> > +   int ret;
> > +
> > +   ret = imx319_write_reg(imx319, IMX319_REG_DPGA_USE_GLOBAL_GAIN, 1, 1);
> > +   if (ret)
> > +   return ret;
> > +
> > +   /* Digital gain = (d_gain & 0xFF00) + (d_gain & 0xFF)/256 times */
> > +   return imx319_write_reg(imx319, IMX319_REG_DIG_GAIN_GLOBAL, 2, d_gain);
> > +}
> > +
> > +static int imx319_set_ctrl(struct v4l2_ctrl *ctrl)
> > +{
> > +   struct imx319 *imx319 = container_of(ctrl->handler,
> > +

Re: [PATCH v3] media: imx319: Add imx319 camera sensor driver

[Bing Bu Cao]

On 2018年05月23日 04:08, jacopo mondi wrote:

Hello Bingbu,
thanks for the patch

On Tue, May 22, 2018 at 12:33:01PM +0800, bingbu@intel.com wrote:

From: Bingbu Cao 

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

Could you please provide a changelog between versions? Also, I know
using the --in-reply-to option is tempting to keep all patch version
in a single thread, but most people finds it confusing and I rarely
see that.

Thanks, I will send the standalone v4 patch with change log .



Signed-off-by: Bingbu Cao 
Signed-off-by: Tianshu Qiu 
---
  MAINTAINERS|7 +
  drivers/media/i2c/Kconfig  |   11 +
  drivers/media/i2c/Makefile |1 +
  drivers/media/i2c/imx319.c | 2434 
  4 files changed, 2453 insertions(+)
  create mode 100644 drivers/media/i2c/imx319.c

diff --git a/MAINTAINERS b/MAINTAINERS
index e73a55a6a855..87b6c338d827 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -13084,6 +13084,13 @@ S: Maintained
  F:drivers/media/i2c/imx274.c
  F:Documentation/devicetree/bindings/media/i2c/imx274.txt

+SONY IMX319 SENSOR DRIVER
+M: Bingbu Cao 
+L: linux-media@vger.kernel.org
+T: git git://linuxtv.org/media_tree.git
+S: Maintained
+F: drivers/media/i2c/imx319.c
+
  SONY MEMORYSTICK CARD SUPPORT
  M:Alex Dubov 
  W:http://tifmxx.berlios.de/
diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
index 1f9d7c6aa31a..c3d279cc293e 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -604,6 +604,17 @@ config VIDEO_IMX274
  This is a V4L2 sensor-level driver for the Sony IMX274
  CMOS image sensor.

+config VIDEO_IMX319
+   tristate "Sony IMX319 sensor support"
+   depends on I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API
+   depends on MEDIA_CAMERA_SUPPORT
+   help
+ This is a Video4Linux2 sensor driver for the Sony
+ IMX319 camera.
+
+ To compile this driver as a module, choose M here: the
+ module will be called imx319.
+
  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 16fc34eda5cc..3adb3be4a486 100644
--- a/drivers/media/i2c/Makefile
+++ b/drivers/media/i2c/Makefile
@@ -104,5 +104,6 @@ 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_VIDEO_IMX319) += imx319.o

  obj-$(CONFIG_SDR_MAX2175) += max2175.o

This hunk does not apply nor on media tree master nor on Linus'
master. Could you please mention on which branch the patch is meant to
be applied in the cover letter? Maybe it's obvious for most people here,
but I failed to find it.

It is based on sakari media_tree branch.

I will rebase that in follow patches.



diff --git a/drivers/media/i2c/imx319.c b/drivers/media/i2c/imx319.c
new file mode 100644
index ..706bbafc75ec
--- /dev/null
+++ b/drivers/media/i2c/imx319.c
@@ -0,0 +1,2434 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (C) 2018 Intel Corporation
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#define IMX319_REG_MODE_SELECT 0x0100
+#define IMX319_MODE_STANDBY0x00
+#define IMX319_MODE_STREAMING  0x01
+
+/* Chip ID */
+#define IMX319_REG_CHIP_ID 0x0016
+#define IMX319_CHIP_ID 0x0319
+
+/* V_TIMING internal */
+#define IMX319_REG_FLL 0x0340
+#define IMX319_FLL_MAX 0x
+
+/* Exposure control */
+#define IMX319_REG_EXPOSURE0x0202
+#define IMX319_EXPOSURE_MIN1
+#define IMX319_EXPOSURE_STEP   1
+#define IMX319_EXPOSURE_DEFAULT0x04ee
+
+/* Analog gain control */
+#define IMX319_REG_ANALOG_GAIN 0x0204
+#define IMX319_ANA_GAIN_MIN0
+#define IMX319_ANA_GAIN_MAX960
+#define IMX319_ANA_GAIN_STEP   1
+#define IMX319_ANA_GAIN_DEFAULT0
+
+/* Digital gain control */
+#define IMX319_REG_DPGA_USE_GLOBAL_GAIN0x3ff9
+#define IMX319_REG_DIG_GAIN_GLOBAL 0x020e
+#define IMX319_DGTL_GAIN_MIN   256
+#define IMX319_DGTL_GAIN_MAX   4095
+#define IMX319_DGTL_GAIN_STEP  1
+#define IMX319_DGTL_GAIN_DEFAULT   256
+
+/* Test Pattern Control */
+#define IMX319_REG_TEST_PATTERN0x0600
+#define IMX319_TEST_PATTERN_DISABLED   0
+#define IMX319_TEST_PATTERN_SOLID_COLOR1
+#define IMX319_TEST_PATTERN_COLOR_BARS 2
+#define IMX319_TEST_PATTERN_GRAY_COLOR_BARS3

Re: [PATCH v3] media: imx319: Add imx319 camera sensor driver

[jacopo mondi]

Hello Bingbu,
   thanks for the patch

On Tue, May 22, 2018 at 12:33:01PM +0800, bingbu@intel.com wrote:
> From: Bingbu Cao 
>
> Add a V4L2 sub-device driver for the Sony IMX319 image sensor.
> This is a camera sensor using the I2C bus for control and the
> CSI-2 bus for data.

Could you please provide a changelog between versions? Also, I know
using the --in-reply-to option is tempting to keep all patch version
in a single thread, but most people finds it confusing and I rarely
see that.

>
> Signed-off-by: Bingbu Cao 
> Signed-off-by: Tianshu Qiu 
> ---
>  MAINTAINERS|7 +
>  drivers/media/i2c/Kconfig  |   11 +
>  drivers/media/i2c/Makefile |1 +
>  drivers/media/i2c/imx319.c | 2434 
> 
>  4 files changed, 2453 insertions(+)
>  create mode 100644 drivers/media/i2c/imx319.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index e73a55a6a855..87b6c338d827 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -13084,6 +13084,13 @@ S:   Maintained
>  F:   drivers/media/i2c/imx274.c
>  F:   Documentation/devicetree/bindings/media/i2c/imx274.txt
>
> +SONY IMX319 SENSOR DRIVER
> +M:   Bingbu Cao 
> +L:   linux-media@vger.kernel.org
> +T:   git git://linuxtv.org/media_tree.git
> +S:   Maintained
> +F:   drivers/media/i2c/imx319.c
> +
>  SONY MEMORYSTICK CARD SUPPORT
>  M:   Alex Dubov 
>  W:   http://tifmxx.berlios.de/
> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> index 1f9d7c6aa31a..c3d279cc293e 100644
> --- a/drivers/media/i2c/Kconfig
> +++ b/drivers/media/i2c/Kconfig
> @@ -604,6 +604,17 @@ config VIDEO_IMX274
> This is a V4L2 sensor-level driver for the Sony IMX274
> CMOS image sensor.
>
> +config VIDEO_IMX319
> + tristate "Sony IMX319 sensor support"
> + depends on I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API
> + depends on MEDIA_CAMERA_SUPPORT
> + help
> +   This is a Video4Linux2 sensor driver for the Sony
> +   IMX319 camera.
> +
> +   To compile this driver as a module, choose M here: the
> +   module will be called imx319.
> +
>  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 16fc34eda5cc..3adb3be4a486 100644
> --- a/drivers/media/i2c/Makefile
> +++ b/drivers/media/i2c/Makefile
> @@ -104,5 +104,6 @@ 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_VIDEO_IMX319)   += imx319.o
>
>  obj-$(CONFIG_SDR_MAX2175) += max2175.o

This hunk does not apply nor on media tree master nor on Linus'
master. Could you please mention on which branch the patch is meant to
be applied in the cover letter? Maybe it's obvious for most people here,
but I failed to find it.

> diff --git a/drivers/media/i2c/imx319.c b/drivers/media/i2c/imx319.c
> new file mode 100644
> index ..706bbafc75ec
> --- /dev/null
> +++ b/drivers/media/i2c/imx319.c
> @@ -0,0 +1,2434 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (C) 2018 Intel Corporation
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#define IMX319_REG_MODE_SELECT   0x0100
> +#define IMX319_MODE_STANDBY  0x00
> +#define IMX319_MODE_STREAMING0x01
> +
> +/* Chip ID */
> +#define IMX319_REG_CHIP_ID   0x0016
> +#define IMX319_CHIP_ID   0x0319
> +
> +/* V_TIMING internal */
> +#define IMX319_REG_FLL   0x0340
> +#define IMX319_FLL_MAX   0x
> +
> +/* Exposure control */
> +#define IMX319_REG_EXPOSURE  0x0202
> +#define IMX319_EXPOSURE_MIN  1
> +#define IMX319_EXPOSURE_STEP 1
> +#define IMX319_EXPOSURE_DEFAULT  0x04ee
> +
> +/* Analog gain control */
> +#define IMX319_REG_ANALOG_GAIN   0x0204
> +#define IMX319_ANA_GAIN_MIN  0
> +#define IMX319_ANA_GAIN_MAX  960
> +#define IMX319_ANA_GAIN_STEP 1
> +#define IMX319_ANA_GAIN_DEFAULT  0
> +
> +/* Digital gain control */
> +#define IMX319_REG_DPGA_USE_GLOBAL_GAIN  0x3ff9
> +#define IMX319_REG_DIG_GAIN_GLOBAL   0x020e
> +#define IMX319_DGTL_GAIN_MIN 256
> +#define IMX319_DGTL_GAIN_MAX 4095
> +#define IMX319_DGTL_GAIN_STEP1
> +#define IMX319_DGTL_GAIN_DEFAULT 256
> +
> +/* Test Pattern Control */
> +#define IMX319_REG_TEST_PATTERN  0x0600
> +#define IMX319_TEST_PATTERN_DISABLED 0
> +#define IMX319_TEST_PATTERN_SOLID_COLOR  1
> +#define IMX319_TEST_PATTERN_COLOR_BARS   2
> +#define IMX319_TEST_PATTERN_GRAY_COLOR_BARS  3
> +#define IMX319_TEST_PATTERN_PN9  4
> +
> +/*