RE: [PATCH v4 3/3] intel-ipu3: cio2: Add new MIPI-CSI2 driver

[Zhi, Yong]

Hi, Sakari,

Thanks for the review.

> -Original Message-
> From: linux-media-ow...@vger.kernel.org [mailto:linux-media-
> ow...@vger.kernel.org] On Behalf Of Sakari Ailus
> Sent: Tuesday, July 11, 2017 3:34 AM
> To: Zhi, Yong <yong@intel.com>
> Cc: linux-media@vger.kernel.org; sakari.ai...@linux.intel.com;
> hans.verk...@cisco.com; Zheng, Jian Xu <jian.xu.zh...@intel.com>;
> tf...@chromium.org; Mani, Rajmohan <rajmohan.m...@intel.com>;
> Toivonen, Tuukka <tuukka.toivo...@intel.com>; Yang, Hyungwoo
> <hyungwoo.y...@intel.com>; Vijaykumar, Ramya
> <ramya.vijayku...@intel.com>
> Subject: Re: [PATCH v4 3/3] intel-ipu3: cio2: Add new MIPI-CSI2 driver
> 
> Hi Yong,
> 
> Thanks for the update! This looks pretty good in general, still some more
> comments below.
> 
> Do you happen to have a todo list for the changes that are still planned?
> AFAIR we should have two items in the list at least ---
> 
> 1. Extend the format example to include the DMA word boundary and
> 
> 2. Separate formats on the sub-device and on the video node.
> 

Will include above in v5 update.

> On Mon, Jul 10, 2017 at 06:43:34PM -0500, Yong Zhi wrote:
> > This patch adds CIO2 CSI-2 device driver for
> > Intel's IPU3 camera sub-system support.
> >
> > Signed-off-by: Yong Zhi <yong@intel.com>
> > Signed-off-by: Hyungwoo Yang <hyungwoo.y...@intel.com>
> > Signed-off-by: Ramya Vijaykumar <ramya.vijayku...@intel.com>
> > ---
> >  drivers/media/pci/Kconfig|2 +
> >  drivers/media/pci/Makefile   |3 +-
> >  drivers/media/pci/intel/Makefile |5 +
> >  drivers/media/pci/intel/ipu3/Kconfig |   17 +
> >  drivers/media/pci/intel/ipu3/Makefile|1 +
> >  drivers/media/pci/intel/ipu3/ipu3-cio2.c | 1873
> ++
> >  drivers/media/pci/intel/ipu3/ipu3-cio2.h |  442 +++
> >  7 files changed, 2342 insertions(+), 1 deletion(-)
> >  create mode 100644 drivers/media/pci/intel/Makefile
> >  create mode 100644 drivers/media/pci/intel/ipu3/Kconfig
> >  create mode 100644 drivers/media/pci/intel/ipu3/Makefile
> >  create mode 100644 drivers/media/pci/intel/ipu3/ipu3-cio2.c
> >  create mode 100644 drivers/media/pci/intel/ipu3/ipu3-cio2.h
> >
> > diff --git a/drivers/media/pci/Kconfig b/drivers/media/pci/Kconfig
> > index da28e68..5932e22 100644
> > --- a/drivers/media/pci/Kconfig
> > +++ b/drivers/media/pci/Kconfig
> > @@ -54,5 +54,7 @@ source "drivers/media/pci/smipcie/Kconfig"
> >  source "drivers/media/pci/netup_unidvb/Kconfig"
> >  endif
> >
> > +source "drivers/media/pci/intel/ipu3/Kconfig"
> > +
> >  endif #MEDIA_PCI_SUPPORT
> >  endif #PCI
> > diff --git a/drivers/media/pci/Makefile b/drivers/media/pci/Makefile
> > index a7e8af0..d8f9843 100644
> > --- a/drivers/media/pci/Makefile
> > +++ b/drivers/media/pci/Makefile
> > @@ -13,7 +13,8 @@ obj-y+=   ttpci/  \
> > ddbridge/   \
> > saa7146/\
> > smipcie/\
> > -   netup_unidvb/
> > +   netup_unidvb/   \
> > +   intel/
> >
> >  obj-$(CONFIG_VIDEO_IVTV) += ivtv/
> >  obj-$(CONFIG_VIDEO_ZORAN) += zoran/
> > diff --git a/drivers/media/pci/intel/Makefile
> b/drivers/media/pci/intel/Makefile
> > new file mode 100644
> > index 000..745c8b2
> > --- /dev/null
> > +++ b/drivers/media/pci/intel/Makefile
> > @@ -0,0 +1,5 @@
> > +#
> > +# Makefile for the IPU3 cio2 and ImGU drivers
> > +#
> > +
> > +obj-y  += ipu3/
> > diff --git a/drivers/media/pci/intel/ipu3/Kconfig
> b/drivers/media/pci/intel/ipu3/Kconfig
> > new file mode 100644
> > index 000..2a895d6
> > --- /dev/null
> > +++ b/drivers/media/pci/intel/ipu3/Kconfig
> > @@ -0,0 +1,17 @@
> > +config VIDEO_IPU3_CIO2
> > +   tristate "Intel ipu3-cio2 driver"
> > +   depends on VIDEO_V4L2 && PCI
> > +   depends on MEDIA_CONTROLLER
> 
> Could you also add VIDEO_V4L2_SUBDEV_API, please?

Ack.

> 
> > +   depends on HAS_DMA
> > +   depends on ACPI
> > +   select V4L2_FWNODE
> > +   select VIDEOBUF2_DMA_SG
> > +
> > +   ---help---
> > +   This is the Intel IPU3 CIO2 CSI-2 receiver unit, found in Intel
> > +   Skylake and Kaby Lake SoCs and used for capturing images and
> > +   video from a camera sensor.
> > +
> > +   Say Y or M here if you have a Skylake/Kaby Lake SoC with MIPI CSI-2
> > +   connected came

Re: [PATCH v4 3/3] intel-ipu3: cio2: Add new MIPI-CSI2 driver

[Sakari Ailus]

Hi Tomasz,

On Thu, Jul 13, 2017 at 05:31:33PM +0900, Tomasz Figa wrote:
> On Thu, Jul 13, 2017 at 5:21 PM, Sakari Ailus  wrote:
> >> >> + ret = v4l2_async_notifier_register(>v4l2_dev, 
> >> >> >notifier);
> >> >> + if (ret) {
> >> >> + cio2->notifier.num_subdevs = 0;
> >> >
> >> > No need to assign num_subdevs as 0.
> >> >
> >> > [YZ] _notifier_exit() will call _unregister() if this is not 0.
> >>
> >> You shouldn't call _exit() if _init() failed. I noticed that many
> >> error paths in your code does this. Please fix it.
> >
> > In general most functions that initialise and clean up something are
> > implemented so that the cleanup function can be called without calling the
> > corresponding init function. This eases driver implementation by reducing
> > complexity in error paths that are difficult to implement and test to begin
> > with, so I don't see anything wrong with that, quite the contrary.
> >
> > I.e. in this case you should call v4l2_async_notifier_unregister() without
> > checking the number of async sub-devices.
> >
> > There are exceptions to that though; not all the framework functions behave
> > this way. Of kernel APIs, e.g. kmalloc() and kfree() do this --- you can
> > pass a NULL pointer to kfree() and it does nothing.
> 
> I'd argue that most of the cleanup paths I've seen in the kernel are
> the opposite. If you properly check for errors in your code, it's
> actually very unlikely that you need to call a cleanup function
> without the init function called... That said, I've seen the pattern
> you describe too, so probably either there is no strict rule or it's
> not strictly enforced. (Still, judging by
> https://www.kernel.org/doc/html/v4.10/process/coding-style.html#centralized-exiting-of-functions,
> which mentions "one err bugs" and suggests "to split it up into two
> error labels", the pattern I'm arguing for might be the recommended
> default.)

I don't really see a problem here; the another label in the example is
needed to avoid referencing a NULL pointer. If there's a reason to add a
label, then a label is just added. :-)

You could check foo as well under a single label. I'd say that approach
generally scales better: if you can handle a difference in error handling
locally in error handling code, this is easier to maintain and easier to
get right in complex error handling code (whereas the example is very
simple); spreading that knowledge over much of the function has the
opposite effect: it's easy to e.g. to go to a wrong label and there have
been plenty of such bugs in the past.

I think we're getting to fine details here. :-)

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk

Re: [PATCH v4 3/3] intel-ipu3: cio2: Add new MIPI-CSI2 driver

[Tomasz Figa]

On Thu, Jul 13, 2017 at 5:21 PM, Sakari Ailus  wrote:
>> >> + ret = v4l2_async_notifier_register(>v4l2_dev, 
>> >> >notifier);
>> >> + if (ret) {
>> >> + cio2->notifier.num_subdevs = 0;
>> >
>> > No need to assign num_subdevs as 0.
>> >
>> > [YZ] _notifier_exit() will call _unregister() if this is not 0.
>>
>> You shouldn't call _exit() if _init() failed. I noticed that many
>> error paths in your code does this. Please fix it.
>
> In general most functions that initialise and clean up something are
> implemented so that the cleanup function can be called without calling the
> corresponding init function. This eases driver implementation by reducing
> complexity in error paths that are difficult to implement and test to begin
> with, so I don't see anything wrong with that, quite the contrary.
>
> I.e. in this case you should call v4l2_async_notifier_unregister() without
> checking the number of async sub-devices.
>
> There are exceptions to that though; not all the framework functions behave
> this way. Of kernel APIs, e.g. kmalloc() and kfree() do this --- you can
> pass a NULL pointer to kfree() and it does nothing.

I'd argue that most of the cleanup paths I've seen in the kernel are
the opposite. If you properly check for errors in your code, it's
actually very unlikely that you need to call a cleanup function
without the init function called... That said, I've seen the pattern
you describe too, so probably either there is no strict rule or it's
not strictly enforced. (Still, judging by
https://www.kernel.org/doc/html/v4.10/process/coding-style.html#centralized-exiting-of-functions,
which mentions "one err bugs" and suggests "to split it up into two
error labels", the pattern I'm arguing for might be the recommended
default.)

Best regards,
Tomasz

Re: [PATCH v4 3/3] intel-ipu3: cio2: Add new MIPI-CSI2 driver

[Sakari Ailus]

Hi Tomasz and Yong,

On Thu, Jul 13, 2017 at 01:51:18PM +0900, Tomasz Figa wrote:
> Hi Yong,
> 
> On Thu, Jul 13, 2017 at 8:20 AM, Zhi, Yong  wrote:
> > Hi, Sakari,
> >
> > Thanks for the time spent on code review, acks to all the comments, except 
> > two places:
> >
> >> +/* .complete() is called after all subdevices have been located */
> >> +static int cio2_notifier_complete(struct v4l2_async_notifier *notifier)
> >> +{
> >> + struct cio2_device *cio2 = container_of(notifier, struct cio2_device,
> >> + notifier);
> >> + struct sensor_async_subdev *s_asd;
> >> + struct fwnode_handle *fwn_remote, *fwn_endpt, *fwn_remote_endpt;
> >> + struct cio2_queue *q;
> >> + struct fwnode_endpoint remote_endpt;
> >> + unsigned int i, pad;
> >> + int ret;
> >> +
> >> + for (i = 0; i < notifier->num_subdevs; i++) {
> >> + s_asd = container_of(cio2->notifier.subdevs[i],
> >> + struct sensor_async_subdev,
> >> + asd);
> >> +
> >> + fwn_remote = s_asd->asd.match.fwnode.fwnode;
> >> + fwn_endpt = (struct fwnode_handle *)
> >> + s_asd->vfwn_endpt.base.local_fwnode;
> >
> > Why do you need a cast?
> >
> > [YZ] With a cast results in compilation warning:
> 
> (I think you mean "without".)
> 
> >
> > drivers/media/pci/ipu3/ipu3-cio2.c:1298:13: warning: assignment discards 
> > ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
> >fwn_endpt = /*(struct fwnode_handle *)*/
> 
> This is a sign that the code is doing something wrong (in this case
> probably trying to write to a const pointer), so casting just silences
> the unfixed error.

Indeed. The right thing to do would be to make fwn_endpt const.

In that case though I don't think that variable is really used for anything
useful; the same goes for remote_endpt and fwn_remote_endpt. This looks
like leftovers from development time.

Could these be removed, or did I miss something?

> 
> >
> >> + ret = v4l2_async_notifier_register(>v4l2_dev, >notifier);
> >> + if (ret) {
> >> + cio2->notifier.num_subdevs = 0;
> >
> > No need to assign num_subdevs as 0.
> >
> > [YZ] _notifier_exit() will call _unregister() if this is not 0.
> 
> You shouldn't call _exit() if _init() failed. I noticed that many
> error paths in your code does this. Please fix it.

In general most functions that initialise and clean up something are
implemented so that the cleanup function can be called without calling the
corresponding init function. This eases driver implementation by reducing
complexity in error paths that are difficult to implement and test to begin
with, so I don't see anything wrong with that, quite the contrary.

I.e. in this case you should call v4l2_async_notifier_unregister() without
checking the number of async sub-devices.

There are exceptions to that though; not all the framework functions behave
this way. Of kernel APIs, e.g. kmalloc() and kfree() do this --- you can
pass a NULL pointer to kfree() and it does nothing.

-- 
Regards,

Sakari Ailus
e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk

Re: [PATCH v4 3/3] intel-ipu3: cio2: Add new MIPI-CSI2 driver

[Tomasz Figa]

Hi Yong,

On Thu, Jul 13, 2017 at 8:20 AM, Zhi, Yong  wrote:
> Hi, Sakari,
>
> Thanks for the time spent on code review, acks to all the comments, except 
> two places:
>
>> +/* .complete() is called after all subdevices have been located */
>> +static int cio2_notifier_complete(struct v4l2_async_notifier *notifier)
>> +{
>> + struct cio2_device *cio2 = container_of(notifier, struct cio2_device,
>> + notifier);
>> + struct sensor_async_subdev *s_asd;
>> + struct fwnode_handle *fwn_remote, *fwn_endpt, *fwn_remote_endpt;
>> + struct cio2_queue *q;
>> + struct fwnode_endpoint remote_endpt;
>> + unsigned int i, pad;
>> + int ret;
>> +
>> + for (i = 0; i < notifier->num_subdevs; i++) {
>> + s_asd = container_of(cio2->notifier.subdevs[i],
>> + struct sensor_async_subdev,
>> + asd);
>> +
>> + fwn_remote = s_asd->asd.match.fwnode.fwnode;
>> + fwn_endpt = (struct fwnode_handle *)
>> + s_asd->vfwn_endpt.base.local_fwnode;
>
> Why do you need a cast?
>
> [YZ] With a cast results in compilation warning:

(I think you mean "without".)

>
> drivers/media/pci/ipu3/ipu3-cio2.c:1298:13: warning: assignment discards 
> ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
>fwn_endpt = /*(struct fwnode_handle *)*/

This is a sign that the code is doing something wrong (in this case
probably trying to write to a const pointer), so casting just silences
the unfixed error.

>
>> + ret = v4l2_async_notifier_register(>v4l2_dev, >notifier);
>> + if (ret) {
>> + cio2->notifier.num_subdevs = 0;
>
> No need to assign num_subdevs as 0.
>
> [YZ] _notifier_exit() will call _unregister() if this is not 0.

You shouldn't call _exit() if _init() failed. I noticed that many
error paths in your code does this. Please fix it.

Best regards,
Tomasz

RE: [PATCH v4 3/3] intel-ipu3: cio2: Add new MIPI-CSI2 driver

[Zhi, Yong]

Hi, Sakari,

Thanks for the time spent on code review, acks to all the comments, except two 
places:

> +/* .complete() is called after all subdevices have been located */
> +static int cio2_notifier_complete(struct v4l2_async_notifier *notifier)
> +{
> + struct cio2_device *cio2 = container_of(notifier, struct cio2_device,
> + notifier);
> + struct sensor_async_subdev *s_asd;
> + struct fwnode_handle *fwn_remote, *fwn_endpt, *fwn_remote_endpt;
> + struct cio2_queue *q;
> + struct fwnode_endpoint remote_endpt;
> + unsigned int i, pad;
> + int ret;
> +
> + for (i = 0; i < notifier->num_subdevs; i++) {
> + s_asd = container_of(cio2->notifier.subdevs[i],
> + struct sensor_async_subdev,
> + asd);
> +
> + fwn_remote = s_asd->asd.match.fwnode.fwnode;
> + fwn_endpt = (struct fwnode_handle *)
> + s_asd->vfwn_endpt.base.local_fwnode;

Why do you need a cast?

[YZ] With a cast results in compilation warning:

drivers/media/pci/ipu3/ipu3-cio2.c:1298:13: warning: assignment discards 
‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
   fwn_endpt = /*(struct fwnode_handle *)*/

> + ret = v4l2_async_notifier_register(>v4l2_dev, >notifier);
> + if (ret) {
> + cio2->notifier.num_subdevs = 0;

No need to assign num_subdevs as 0.

[YZ] _notifier_exit() will call _unregister() if this is not 0.

Regards,

Yong


From: linux-media-ow...@vger.kernel.org [linux-media-ow...@vger.kernel.org] on 
behalf of Sakari Ailus [sakari.ai...@iki.fi]
Sent: Tuesday, July 11, 2017 3:33 AM
To: Zhi, Yong
Cc: linux-media@vger.kernel.org; sakari.ai...@linux.intel.com; 
hans.verk...@cisco.com; Zheng, Jian Xu; tf...@chromium.org; Mani, Rajmohan; 
Toivonen, Tuukka; Yang, Hyungwoo; Vijaykumar, Ramya
Subject: Re: [PATCH v4 3/3] intel-ipu3: cio2: Add new MIPI-CSI2 driver

Hi Yong,

Thanks for the update! This looks pretty good in general, still some more
comments below.

Do you happen to have a todo list for the changes that are still planned?
AFAIR we should have two items in the list at least ---

1. Extend the format example to include the DMA word boundary and

2. Separate formats on the sub-device and on the video node.

On Mon, Jul 10, 2017 at 06:43:34PM -0500, Yong Zhi wrote:
> This patch adds CIO2 CSI-2 device driver for
> Intel's IPU3 camera sub-system support.
>
> Signed-off-by: Yong Zhi <yong@intel.com>
> Signed-off-by: Hyungwoo Yang <hyungwoo.y...@intel.com>
> Signed-off-by: Ramya Vijaykumar <ramya.vijayku...@intel.com>
> ---
>  drivers/media/pci/Kconfig|2 +
>  drivers/media/pci/Makefile   |3 +-
>  drivers/media/pci/intel/Makefile |5 +
>  drivers/media/pci/intel/ipu3/Kconfig |   17 +
>  drivers/media/pci/intel/ipu3/Makefile|1 +
>  drivers/media/pci/intel/ipu3/ipu3-cio2.c | 1873 
> ++
>  drivers/media/pci/intel/ipu3/ipu3-cio2.h |  442 +++
>  7 files changed, 2342 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/media/pci/intel/Makefile
>  create mode 100644 drivers/media/pci/intel/ipu3/Kconfig
>  create mode 100644 drivers/media/pci/intel/ipu3/Makefile
>  create mode 100644 drivers/media/pci/intel/ipu3/ipu3-cio2.c
>  create mode 100644 drivers/media/pci/intel/ipu3/ipu3-cio2.h
>
> diff --git a/drivers/media/pci/Kconfig b/drivers/media/pci/Kconfig
> index da28e68..5932e22 100644
> --- a/drivers/media/pci/Kconfig
> +++ b/drivers/media/pci/Kconfig
> @@ -54,5 +54,7 @@ source "drivers/media/pci/smipcie/Kconfig"
>  source "drivers/media/pci/netup_unidvb/Kconfig"
>  endif
>
> +source "drivers/media/pci/intel/ipu3/Kconfig"
> +
>  endif #MEDIA_PCI_SUPPORT
>  endif #PCI
> diff --git a/drivers/media/pci/Makefile b/drivers/media/pci/Makefile
> index a7e8af0..d8f9843 100644
> --- a/drivers/media/pci/Makefile
> +++ b/drivers/media/pci/Makefile
> @@ -13,7 +13,8 @@ obj-y+= ttpci/  \
>   ddbridge/   \
>   saa7146/\
>   smipcie/\
> - netup_unidvb/
> + netup_unidvb/   \
> + intel/
>
>  obj-$(CONFIG_VIDEO_IVTV) += ivtv/
>  obj-$(CONFIG_VIDEO_ZORAN) += zoran/
> diff --git a/drivers/media/pci/intel/Makefile 
> b/drivers/media/pci/intel/Makefile
> new file mode 100644
> index 000..745c8b2
> --- /dev/null
> +++ b/drivers/media/pci/intel/Makefile
> @@ -0,0 +1,5 @@
> +#
> +# Makefile for the IPU3 cio2 and ImGU drivers
> +#
> +
> +obj-y 

Re: [PATCH v4 3/3] intel-ipu3: cio2: Add new MIPI-CSI2 driver

[Sakari Ailus]

Hi Yong,

Thanks for the update! This looks pretty good in general, still some more
comments below.

Do you happen to have a todo list for the changes that are still planned?
AFAIR we should have two items in the list at least ---

1. Extend the format example to include the DMA word boundary and

2. Separate formats on the sub-device and on the video node.

On Mon, Jul 10, 2017 at 06:43:34PM -0500, Yong Zhi wrote:
> This patch adds CIO2 CSI-2 device driver for
> Intel's IPU3 camera sub-system support.
> 
> Signed-off-by: Yong Zhi 
> Signed-off-by: Hyungwoo Yang 
> Signed-off-by: Ramya Vijaykumar 
> ---
>  drivers/media/pci/Kconfig|2 +
>  drivers/media/pci/Makefile   |3 +-
>  drivers/media/pci/intel/Makefile |5 +
>  drivers/media/pci/intel/ipu3/Kconfig |   17 +
>  drivers/media/pci/intel/ipu3/Makefile|1 +
>  drivers/media/pci/intel/ipu3/ipu3-cio2.c | 1873 
> ++
>  drivers/media/pci/intel/ipu3/ipu3-cio2.h |  442 +++
>  7 files changed, 2342 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/media/pci/intel/Makefile
>  create mode 100644 drivers/media/pci/intel/ipu3/Kconfig
>  create mode 100644 drivers/media/pci/intel/ipu3/Makefile
>  create mode 100644 drivers/media/pci/intel/ipu3/ipu3-cio2.c
>  create mode 100644 drivers/media/pci/intel/ipu3/ipu3-cio2.h
> 
> diff --git a/drivers/media/pci/Kconfig b/drivers/media/pci/Kconfig
> index da28e68..5932e22 100644
> --- a/drivers/media/pci/Kconfig
> +++ b/drivers/media/pci/Kconfig
> @@ -54,5 +54,7 @@ source "drivers/media/pci/smipcie/Kconfig"
>  source "drivers/media/pci/netup_unidvb/Kconfig"
>  endif
> 
> +source "drivers/media/pci/intel/ipu3/Kconfig"
> +
>  endif #MEDIA_PCI_SUPPORT
>  endif #PCI
> diff --git a/drivers/media/pci/Makefile b/drivers/media/pci/Makefile
> index a7e8af0..d8f9843 100644
> --- a/drivers/media/pci/Makefile
> +++ b/drivers/media/pci/Makefile
> @@ -13,7 +13,8 @@ obj-y+= ttpci/  \
>   ddbridge/   \
>   saa7146/\
>   smipcie/\
> - netup_unidvb/
> + netup_unidvb/   \
> + intel/
> 
>  obj-$(CONFIG_VIDEO_IVTV) += ivtv/
>  obj-$(CONFIG_VIDEO_ZORAN) += zoran/
> diff --git a/drivers/media/pci/intel/Makefile 
> b/drivers/media/pci/intel/Makefile
> new file mode 100644
> index 000..745c8b2
> --- /dev/null
> +++ b/drivers/media/pci/intel/Makefile
> @@ -0,0 +1,5 @@
> +#
> +# Makefile for the IPU3 cio2 and ImGU drivers
> +#
> +
> +obj-y+= ipu3/
> diff --git a/drivers/media/pci/intel/ipu3/Kconfig 
> b/drivers/media/pci/intel/ipu3/Kconfig
> new file mode 100644
> index 000..2a895d6
> --- /dev/null
> +++ b/drivers/media/pci/intel/ipu3/Kconfig
> @@ -0,0 +1,17 @@
> +config VIDEO_IPU3_CIO2
> + tristate "Intel ipu3-cio2 driver"
> + depends on VIDEO_V4L2 && PCI
> + depends on MEDIA_CONTROLLER

Could you also add VIDEO_V4L2_SUBDEV_API, please?

> + depends on HAS_DMA
> + depends on ACPI
> + select V4L2_FWNODE
> + select VIDEOBUF2_DMA_SG
> +
> + ---help---
> + This is the Intel IPU3 CIO2 CSI-2 receiver unit, found in Intel
> + Skylake and Kaby Lake SoCs and used for capturing images and
> + video from a camera sensor.
> +
> + Say Y or M here if you have a Skylake/Kaby Lake SoC with MIPI CSI-2
> + connected camera.
> + The module will be called ipu3-cio2.
> diff --git a/drivers/media/pci/intel/ipu3/Makefile 
> b/drivers/media/pci/intel/ipu3/Makefile
> new file mode 100644
> index 000..20186e3
> --- /dev/null
> +++ b/drivers/media/pci/intel/ipu3/Makefile
> @@ -0,0 +1 @@
> +obj-$(CONFIG_VIDEO_IPU3_CIO2) += ipu3-cio2.o
> diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c 
> b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> new file mode 100644
> index 000..8ecb17f
> --- /dev/null
> +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> @@ -0,0 +1,1873 @@
> +/*
> + * Copyright (c) 2017 Intel Corporation.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License version
> + * 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that 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.
> + *
> + * Based partially on Intel IPU4 driver written by
> + *  Sakari Ailus 
> + *  Samu Onkalo 
> + *  Jouni Högander 
> + *  Jouni Ukkonen 
> + *  Antti Laakso 
> + * et al.
> + *
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> 

[PATCH v4 3/3] intel-ipu3: cio2: Add new MIPI-CSI2 driver

[Yong Zhi]

This patch adds CIO2 CSI-2 device driver for
Intel's IPU3 camera sub-system support.

Signed-off-by: Yong Zhi 
Signed-off-by: Hyungwoo Yang 
Signed-off-by: Ramya Vijaykumar 
---
 drivers/media/pci/Kconfig|2 +
 drivers/media/pci/Makefile   |3 +-
 drivers/media/pci/intel/Makefile |5 +
 drivers/media/pci/intel/ipu3/Kconfig |   17 +
 drivers/media/pci/intel/ipu3/Makefile|1 +
 drivers/media/pci/intel/ipu3/ipu3-cio2.c | 1873 ++
 drivers/media/pci/intel/ipu3/ipu3-cio2.h |  442 +++
 7 files changed, 2342 insertions(+), 1 deletion(-)
 create mode 100644 drivers/media/pci/intel/Makefile
 create mode 100644 drivers/media/pci/intel/ipu3/Kconfig
 create mode 100644 drivers/media/pci/intel/ipu3/Makefile
 create mode 100644 drivers/media/pci/intel/ipu3/ipu3-cio2.c
 create mode 100644 drivers/media/pci/intel/ipu3/ipu3-cio2.h

diff --git a/drivers/media/pci/Kconfig b/drivers/media/pci/Kconfig
index da28e68..5932e22 100644
--- a/drivers/media/pci/Kconfig
+++ b/drivers/media/pci/Kconfig
@@ -54,5 +54,7 @@ source "drivers/media/pci/smipcie/Kconfig"
 source "drivers/media/pci/netup_unidvb/Kconfig"
 endif

+source "drivers/media/pci/intel/ipu3/Kconfig"
+
 endif #MEDIA_PCI_SUPPORT
 endif #PCI
diff --git a/drivers/media/pci/Makefile b/drivers/media/pci/Makefile
index a7e8af0..d8f9843 100644
--- a/drivers/media/pci/Makefile
+++ b/drivers/media/pci/Makefile
@@ -13,7 +13,8 @@ obj-y+=   ttpci/  \
ddbridge/   \
saa7146/\
smipcie/\
-   netup_unidvb/
+   netup_unidvb/   \
+   intel/

 obj-$(CONFIG_VIDEO_IVTV) += ivtv/
 obj-$(CONFIG_VIDEO_ZORAN) += zoran/
diff --git a/drivers/media/pci/intel/Makefile b/drivers/media/pci/intel/Makefile
new file mode 100644
index 000..745c8b2
--- /dev/null
+++ b/drivers/media/pci/intel/Makefile
@@ -0,0 +1,5 @@
+#
+# Makefile for the IPU3 cio2 and ImGU drivers
+#
+
+obj-y  += ipu3/
diff --git a/drivers/media/pci/intel/ipu3/Kconfig 
b/drivers/media/pci/intel/ipu3/Kconfig
new file mode 100644
index 000..2a895d6
--- /dev/null
+++ b/drivers/media/pci/intel/ipu3/Kconfig
@@ -0,0 +1,17 @@
+config VIDEO_IPU3_CIO2
+   tristate "Intel ipu3-cio2 driver"
+   depends on VIDEO_V4L2 && PCI
+   depends on MEDIA_CONTROLLER
+   depends on HAS_DMA
+   depends on ACPI
+   select V4L2_FWNODE
+   select VIDEOBUF2_DMA_SG
+
+   ---help---
+   This is the Intel IPU3 CIO2 CSI-2 receiver unit, found in Intel
+   Skylake and Kaby Lake SoCs and used for capturing images and
+   video from a camera sensor.
+
+   Say Y or M here if you have a Skylake/Kaby Lake SoC with MIPI CSI-2
+   connected camera.
+   The module will be called ipu3-cio2.
diff --git a/drivers/media/pci/intel/ipu3/Makefile 
b/drivers/media/pci/intel/ipu3/Makefile
new file mode 100644
index 000..20186e3
--- /dev/null
+++ b/drivers/media/pci/intel/ipu3/Makefile
@@ -0,0 +1 @@
+obj-$(CONFIG_VIDEO_IPU3_CIO2) += ipu3-cio2.o
diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c 
b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
new file mode 100644
index 000..8ecb17f
--- /dev/null
+++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
@@ -0,0 +1,1873 @@
+/*
+ * Copyright (c) 2017 Intel Corporation.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License version
+ * 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that 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.
+ *
+ * Based partially on Intel IPU4 driver written by
+ *  Sakari Ailus 
+ *  Samu Onkalo 
+ *  Jouni Högander 
+ *  Jouni Ukkonen 
+ *  Antti Laakso 
+ * et al.
+ *
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include "ipu3-cio2.h"
+
+/*
+ * These are raw formats used in Intel's third generation of
+ * Image Processing Unit known as IPU3.
+ * 10bit raw bayer packed, 32 bytes for every 25 pixels,
+ * last LSB 6 bits unused.
+ */
+static const u32 cio2_csi2_fmts[] = {
+   V4L2_PIX_FMT_IPU3_SRGGB10,
+   V4L2_PIX_FMT_IPU3_SGBRG10,
+   V4L2_PIX_FMT_IPU3_SGRBG10,
+   V4L2_PIX_FMT_IPU3_SBGGR10,
+};
+
+static inline u32 cio2_bytesperline(const unsigned int width)
+{
+   /*
+* 64 bytes for every 50 pixels, the line length
+* in bytes is multiple of 64 (line end alignment).
+*/
+   return DIV_ROUND_UP(width, 50) * 64;
+}
+