RE: [PATCH v7 00/16] Intel IPU3 ImgU patchset

[Mani, Rajmohan]

Hi Laurent,

> > Hi Rajmohan,
> >
> > On Tuesday, 4 December 2018 18:07:16 EET Mani, Rajmohan wrote:
> > > >> On Thursday, 29 November 2018 21:51:32 EET Tomasz Figa wrote:
> > > >>> On Thu, Nov 29, 2018 at 6:43 AM Laurent Pinchart wrote:
> > > >>>> On Tuesday, 30 October 2018 00:22:54 EET Yong Zhi wrote:
> > > >>
> > > >> [snip]
> > > >>
> > > >>>>> 1. Link pad flag of video nodes (i.e. ipu3-imgu 0 output) need
> > > >>>>> to be enabled prior to the test.
> > > >>>>> 2. Stream tests are not performed since it requires
> > > >>>>> pre-configuration for each case.
> > > >>>>
> > > >>>> And that's a bit of an issue. I've tested the driver with a
> > > >>>> small script based on media-ctl to configure links and yavta to
> > > >>>> interface with the video nodes, and got the following oops:
> >
> > [snip]
> >
> > > >>>> The script can be found at
> > > >>>> https://lists.libcamera.org/pipermail/libcamera-devel/2018-Nove
> > > >>>> mb
> > > >>>> e
> > > >>>> r/40.html.
> > > >>>>
> > > >>>> I may be doing something wrong (and I probably am), but in any
> > > >>>> case, the driver shouldn't crash. Could you please have a look ?
> > > >>>
> > > >>> It looks like the driver doesn't have the default state
> > > >>> initialized correctly somewhere and it ends up using 0 as the
> > > >>> divisor in some calculation? Something to fix indeed.
> > > >>
> > > >> That's probably the case. I'll trust Intel to fix that in v8 :-)
> > > >
> > > > Ack.
> > >
> > > Thanks for catching this.
> > > I was able to reproduce this error and I see that error handling is
> > > missing, leading to the panic.
> > >
> > > https://git.linuxtv.org/sailus/media_tree.git/tree/drivers/media
> > > /pci/intel/ipu3/ipu3-css-params.c?h=ipu3-v7=
> > > 19cee7329ca2d0156043cac6afcde535e93310af#n433
> > >
> > > is where the -EINVAL is returned.
> > >
> > > Setting the return type as int for the following function and all
> > > its callers to use the return value properly to error out, makes the
> > > panic go away.
> >
> > I assume that I will still not be able to process frames through the
> > pipe then, as I'll get an error :-) Could you tell me why the
> > configuration is incorrect and how I can fix it ?
> >
> 
> :)
> Let me look into this more and get back.
> Thanks for your patience.

I can see a couple of steps missing in the script below.
(https://lists.libcamera.org/pipermail/libcamera-devel/2018-November/40.html)

>From patch 02 of this v7 series "doc-rst: Add Intel IPU3 documentation", under 
>section
"Configuring ImgU V4L2 subdev for image processing"...

1. The pipe mode needs to be configured for the V4L2 subdev.

Also the pipe mode of the corresponding V4L2 subdev should be set as 
desired (e.g 0 for video mode or 1 for still mode) through the control 
id 0x009819a1 as below.

e.g v4l2n -d /dev/v4l-subdev7 --ctrl=0x009819A1=1

2. ImgU pipeline needs to be configured for image processing as below.

RAW bayer frames go through the following ISP pipeline HW blocks to 
have the processed image output to the DDR memory.

RAW bayer frame -> Input Feeder -> Bayer Down Scaling (BDS) -> 
Geometric Distortion Correction (GDC) -> DDR

The ImgU V4L2 subdev has to be configured with the supported 
resolutions in all the above HW blocks, for a given input resolution.

For a given supported resolution for an input frame, the Input Feeder, 
Bayer Down Scaling and GDC blocks should be configured with the 
supported resolutions. This information can be obtained by looking at 
the following IPU3 ISP configuration table for ov5670 sensor.

https://chromium.googlesource.com/chromiumos/overlays/board-overlays/+/master
/baseboard-poppy/media-libs/cros-camera-hal-configs-poppy/files/gcss
/graph_settings_ov5670.xml

For the ov5670 example, for an input frame with a resolution of 
2592x1944 (which is input to the ImgU subdev pad 0), the corresponding 
resolutions for input feeder, BDS and GDC are 2592x1944, 2592x1944 and 
2560x1920 respectively.

The following steps prepare the ImgU ISP pipeline for the image processing.

1. The ImgU V4L2 subdev data format should be set by using the 
VIDIOC_SUBDEV_S_FMT on pad 0, using the GDC width and height obtained above.

2. The ImgU V4L2 subdev cropping should be set by using the 
VIDIOC_SUBDEV_S_SELECTION on pad 0, with V4L2_SEL_TGT_CROP as the 
target, using the input feeder height and width.

3. The ImgU V4L2 subdev composing should be set by using the 
VIDIOC_SUBDEV_S_SELECTION on pad 0, with V4L2_SEL_TGT_COMPOSE as the 
target, using the BDS height and width.

Once these 2 steps are done, the raw bayer frames can be input to the 
ImgU V4L2 subdev for processing.

> Raj
> 
> > > ipu3_css_osys_calc_frame_and_stripe_params()
> > >
> > > Will include the fix in v8.
> >
> > Thank you.
> >
> > > Thanks for catching this.
> >
> > You're welcome.
> >
> > --
> > Regards,
> >
> > Laurent Pinchart
> >
> >

RE: [PATCH v7 00/16] Intel IPU3 ImgU patchset

[Mani, Rajmohan]

Hi Laurent,

> -Original Message-
> From: Laurent Pinchart [mailto:laurent.pinch...@ideasonboard.com]
> Sent: Tuesday, December 04, 2018 8:42 AM
> To: Mani, Rajmohan 
> Cc: Tomasz Figa ; Zhi, Yong ;
> Linux Media Mailing List ; Sakari Ailus
> ; Mauro Carvalho Chehab
> ; Hans Verkuil ; Zheng, Jian
> Xu ; Hu, Jerry W ;
> Toivonen, Tuukka ; Qiu, Tian Shu
> ; Cao, Bingbu 
> Subject: Re: [PATCH v7 00/16] Intel IPU3 ImgU patchset
> 
> Hi Rajmohan,
> 
> On Tuesday, 4 December 2018 18:07:16 EET Mani, Rajmohan wrote:
> > >> On Thursday, 29 November 2018 21:51:32 EET Tomasz Figa wrote:
> > >>> On Thu, Nov 29, 2018 at 6:43 AM Laurent Pinchart wrote:
> > >>>> On Tuesday, 30 October 2018 00:22:54 EET Yong Zhi wrote:
> > >>
> > >> [snip]
> > >>
> > >>>>> 1. Link pad flag of video nodes (i.e. ipu3-imgu 0 output) need
> > >>>>> to be enabled prior to the test.
> > >>>>> 2. Stream tests are not performed since it requires
> > >>>>> pre-configuration for each case.
> > >>>>
> > >>>> And that's a bit of an issue. I've tested the driver with a small
> > >>>> script based on media-ctl to configure links and yavta to
> > >>>> interface with the video nodes, and got the following oops:
> 
> [snip]
> 
> > >>>> The script can be found at
> > >>>> https://lists.libcamera.org/pipermail/libcamera-devel/2018-Novemb
> > >>>> e
> > >>>> r/40.html.
> > >>>>
> > >>>> I may be doing something wrong (and I probably am), but in any
> > >>>> case, the driver shouldn't crash. Could you please have a look ?
> > >>>
> > >>> It looks like the driver doesn't have the default state
> > >>> initialized correctly somewhere and it ends up using 0 as the
> > >>> divisor in some calculation? Something to fix indeed.
> > >>
> > >> That's probably the case. I'll trust Intel to fix that in v8 :-)
> > >
> > > Ack.
> >
> > Thanks for catching this.
> > I was able to reproduce this error and I see that error handling is
> > missing, leading to the panic.
> >
> > https://git.linuxtv.org/sailus/media_tree.git/tree/drivers/media
> > /pci/intel/ipu3/ipu3-css-params.c?h=ipu3-v7=
> > 19cee7329ca2d0156043cac6afcde535e93310af#n433
> >
> > is where the -EINVAL is returned.
> >
> > Setting the return type as int for the following function and all its
> > callers to use the return value properly to error out, makes the panic
> > go away.
> 
> I assume that I will still not be able to process frames through the pipe 
> then, as
> I'll get an error :-) Could you tell me why the configuration is incorrect 
> and how
> I can fix it ?
> 

:)
Let me look into this more and get back.
Thanks for your patience.

Raj

> > ipu3_css_osys_calc_frame_and_stripe_params()
> >
> > Will include the fix in v8.
> 
> Thank you.
> 
> > Thanks for catching this.
> 
> You're welcome.
> 
> --
> Regards,
> 
> Laurent Pinchart
> 
> 

RE: [PATCH v7 00/16] Intel IPU3 ImgU patchset

[Mani, Rajmohan]

Hi Laurent, Tomasz,

> 
> Thanks for the reviews.
> 
> > Subject: Re: [PATCH v7 00/16] Intel IPU3 ImgU patchset
> >
> > Hi Tomasz,
> >
> > On Thursday, 29 November 2018 21:51:32 EET Tomasz Figa wrote:
> > > On Thu, Nov 29, 2018 at 6:43 AM Laurent Pinchart wrote:
> > > > On Tuesday, 30 October 2018 00:22:54 EET Yong Zhi wrote:
> >
> > [snip]
> >
> > > >> 1. Link pad flag of video nodes (i.e. ipu3-imgu 0 output) need to
> > > >> be enabled prior to the test.
> > > >> 2. Stream tests are not performed since it requires
> > > >> pre-configuration for each case.
> > > >
> > > > And that's a bit of an issue. I've tested the driver with a small
> > > > script based on media-ctl to configure links and yavta to
> > > > interface with the video nodes, and got the following oops:
> > > >
> > > > [  136.927788] divide error:  [#1] PREEMPT SMP PTI [
> > > > 136.927801] CPU: 2 PID: 2069 Comm: yavta Not tainted 4.20.0-rc1+
> > > > #9 [  136.927806] Hardware name: HP Soraka/Soraka, BIOS
> > > > 08/30/2018 [ 136.927820] RIP: 0010:ipu3_css_osys_calc+0xc54/0xe14
> > > > [ipu3_imgu] [ 136.927825] Code: 89 44 24 28 42 8b 44 86 6c f7 54
> > > > 24 04 81 64 24 28
> > > > 00 fd ff ff 81 64 24 04 00 03 00 00 8d 44 03 ff 81 44 24 28 80 03
> > > > 00
> > > > 00 99  fb 0f af c3 bb 20 00 00 00 99 f7 fb 8b 5c 24 40 83 fd
> > > > 01
> > > > 19 d2 [  136.927830] RSP: 0018:9af2c0b837c8 EFLAGS: 00010202 [
> > > > 136.927835] RAX:  RBX:  RCX:
> > > > 9af2c3e353c0
> > > > [  136.927839] RDX:  RSI: 9af2c0b838e0 RDI:
> > > > 9af2c3e353c0
> > > > [  136.927843] RBP: 0001 R08:  R09:
> > > > 9af2c0b83880
> > > > [  136.927846] R10: 9af2c3e353c0 R11: 9af2c3e357c0 R12:
> > > > 03a0
> > > > [  136.927849] R13: 00025a0a R14:  R15:
> > > > 
> > > > [  136.927854] FS:  7f1eca167700()
> > > > GS:8c19fab0()
> > > > knlGS:
> > > > 
> > > > [  136.927858] CS:  0010 DS:  ES:  CR0: 80050033 [
> > > > 136.927862] CR2: 7f1ec776c000 CR3: 0001312a4003 CR4:
> > > > 003606e0
> > > > [  136.927865] Call Trace:
> > > > [  136.927884]  ? __accumulate_pelt_segments+0x29/0x3a
> > > > [  136.927892]  ? __switch_to_asm+0x40/0x70 [  136.927899]  ?
> > > > alloc_vmap_area+0x78/0x2f6 [  136.927903]  ?
> > > > __switch_to_asm+0x40/0x70 [  136.927907]  ?
> > > > __switch_to_asm+0x34/0x70 [  136.927911]  ?
> > > > __switch_to_asm+0x40/0x70 [  136.927915]  ?
> > > > __switch_to_asm+0x34/0x70 [  136.927923]  ?
> > > > __inc_numa_state+0x28/0x70 [  136.927929]  ?
> > > > preempt_latency_start+0x1e/0x3d [  136.927936]  ?
> > > > get_page_from_freelist+0x821/0xb62
> > > > [  136.927943]  ? slab_pre_alloc_hook+0x12/0x3b [  136.927948]  ?
> > > > kmem_cache_alloc_node_trace+0xf6/0x108
> > > > [  136.927954]  ? alloc_vmap_area+0x78/0x2f6
> > >
> > > Is it just me or the backtrace above doesn't seem to make sense? I
> > > don't see any allocations inside ipu3_css_cfg_acc().
> >
> > I suppose that's why it's prefixed with '?' :-)
> >
> > > > [  136.927965]  ipu3_css_cfg_acc+0xa0/0x1b5f [ipu3_imgu] [
> > > > 136.927981]  ipu3_css_set_parameters+0x286/0x6e7 [ipu3_imgu] [
> > > > 136.927995]  ipu3_css_start_streaming+0x1230/0x130a [ipu3_imgu] [
> > > > 136.928010]  imgu_s_stream+0x104/0x2f7 [ipu3_imgu] [  136.928022]
> > > > ipu3_vb2_start_streaming+0x168/0x1bd [ipu3_imgu] [  136.928034]
> > > > vb2_start_streaming+0x6c/0xf2 [videobuf2_common] [  136.928044]
> > > > vb2_core_streamon+0xcf/0x109 [videobuf2_common] [  136.928061]
> > > > __video_do_ioctl+0x239/0x388 [videodev] [  136.928081]
> > > > video_usercopy+0x25d/0x47a [videodev] [  136.928097]  ?
> > > > copy_overflow+0x14/0x14 [videodev] [  136.928115]
> > > > v4l2_ioctl+0x4d/0x58 [videodev] [  136.928123]
> > > > vfs_ioctl+0x1b/0x28 [  136.928130]  do_vfs_ioctl+0x4de/0x566 [
> > > > 136.928139]
> > > > ksys_ioctl+0x50/0x70 [  136.928146]  __x64_sys_ioctl+0x16/0x19 [
> > > > 136.928152]  do_syscall_64+0x4d/0x5a [  136.928158]
> > > > entry_SYSCALL_64_after_hwframe+0x44/0xa9
> > > > [  136.928164] RIP: 0033:0x7f1ec9a84f47 [  136.928169] Code: 00 00
> > > > 00 48 8b 05 51 6f 2c 00 64 c7 00 26 00 00 00 48
> > > > c7 c0 ff ff ff ff c3 66 2e 0f 1f 84 00 00 00 00 00 b8 10 00 00 00
> > > > 0f
> > > > 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 21 6f 2c 00 f7 d8 64 89
> > > > 01
> > > > 48 [  136.928173] RSP: 002b:7ffe279e6188 EFLAGS: 0246
> > ORIG_RAX:
> > > > 0010
> > > > [  136.928178] RAX: ffda RBX: 0007 RCX:
> > > > 7f1ec9a84f47
> > > > [  136.928181] RDX: 7ffe279e6194 RSI: 40045612 RDI:
> > > > 0003
> > > > [  136.928184] RBP:  R08: 7f1ec776d000 R09:
> > > > 
> > > > [  136.928188] R10: 0020 R11: 0246 R12:
> > > > 7ffe279e6360
> > > > [  136.928191] R13: 0004 R14: 

RE: [PATCH v7 00/16] Intel IPU3 ImgU patchset

[Mani, Rajmohan]

Hi Laurent, Tomasz,

Thanks for the reviews.

> Subject: Re: [PATCH v7 00/16] Intel IPU3 ImgU patchset
> 
> Hi Tomasz,
> 
> On Thursday, 29 November 2018 21:51:32 EET Tomasz Figa wrote:
> > On Thu, Nov 29, 2018 at 6:43 AM Laurent Pinchart wrote:
> > > On Tuesday, 30 October 2018 00:22:54 EET Yong Zhi wrote:
> 
> [snip]
> 
> > >> 1. Link pad flag of video nodes (i.e. ipu3-imgu 0 output) need to
> > >> be enabled prior to the test.
> > >> 2. Stream tests are not performed since it requires
> > >> pre-configuration for each case.
> > >
> > > And that's a bit of an issue. I've tested the driver with a small
> > > script based on media-ctl to configure links and yavta to interface
> > > with the video nodes, and got the following oops:
> > >
> > > [  136.927788] divide error:  [#1] PREEMPT SMP PTI [
> > > 136.927801] CPU: 2 PID: 2069 Comm: yavta Not tainted 4.20.0-rc1+ #9
> > > [  136.927806] Hardware name: HP Soraka/Soraka, BIOS  08/30/2018 [
> > > 136.927820] RIP: 0010:ipu3_css_osys_calc+0xc54/0xe14 [ipu3_imgu] [
> > > 136.927825] Code: 89 44 24 28 42 8b 44 86 6c f7 54 24 04 81 64 24 28
> > > 00 fd ff ff 81 64 24 04 00 03 00 00 8d 44 03 ff 81 44 24 28 80 03 00
> > > 00 99  fb 0f af c3 bb 20 00 00 00 99 f7 fb 8b 5c 24 40 83 fd 01
> > > 19 d2 [  136.927830] RSP: 0018:9af2c0b837c8 EFLAGS: 00010202 [
> > > 136.927835] RAX:  RBX:  RCX:
> > > 9af2c3e353c0
> > > [  136.927839] RDX:  RSI: 9af2c0b838e0 RDI:
> > > 9af2c3e353c0
> > > [  136.927843] RBP: 0001 R08:  R09:
> > > 9af2c0b83880
> > > [  136.927846] R10: 9af2c3e353c0 R11: 9af2c3e357c0 R12:
> > > 03a0
> > > [  136.927849] R13: 00025a0a R14:  R15:
> > > 
> > > [  136.927854] FS:  7f1eca167700() GS:8c19fab0()
> > > knlGS:
> > > 
> > > [  136.927858] CS:  0010 DS:  ES:  CR0: 80050033 [
> > > 136.927862] CR2: 7f1ec776c000 CR3: 0001312a4003 CR4:
> > > 003606e0
> > > [  136.927865] Call Trace:
> > > [  136.927884]  ? __accumulate_pelt_segments+0x29/0x3a
> > > [  136.927892]  ? __switch_to_asm+0x40/0x70 [  136.927899]  ?
> > > alloc_vmap_area+0x78/0x2f6 [  136.927903]  ?
> > > __switch_to_asm+0x40/0x70 [  136.927907]  ?
> > > __switch_to_asm+0x34/0x70 [  136.927911]  ?
> > > __switch_to_asm+0x40/0x70 [  136.927915]  ?
> > > __switch_to_asm+0x34/0x70 [  136.927923]  ?
> > > __inc_numa_state+0x28/0x70 [  136.927929]  ?
> > > preempt_latency_start+0x1e/0x3d [  136.927936]  ?
> > > get_page_from_freelist+0x821/0xb62
> > > [  136.927943]  ? slab_pre_alloc_hook+0x12/0x3b [  136.927948]  ?
> > > kmem_cache_alloc_node_trace+0xf6/0x108
> > > [  136.927954]  ? alloc_vmap_area+0x78/0x2f6
> >
> > Is it just me or the backtrace above doesn't seem to make sense? I
> > don't see any allocations inside ipu3_css_cfg_acc().
> 
> I suppose that's why it's prefixed with '?' :-)
> 
> > > [  136.927965]  ipu3_css_cfg_acc+0xa0/0x1b5f [ipu3_imgu] [
> > > 136.927981]  ipu3_css_set_parameters+0x286/0x6e7 [ipu3_imgu] [
> > > 136.927995]  ipu3_css_start_streaming+0x1230/0x130a [ipu3_imgu] [
> > > 136.928010]  imgu_s_stream+0x104/0x2f7 [ipu3_imgu] [  136.928022]
> > > ipu3_vb2_start_streaming+0x168/0x1bd [ipu3_imgu] [  136.928034]
> > > vb2_start_streaming+0x6c/0xf2 [videobuf2_common] [  136.928044]
> > > vb2_core_streamon+0xcf/0x109 [videobuf2_common] [  136.928061]
> > > __video_do_ioctl+0x239/0x388 [videodev] [  136.928081]
> > > video_usercopy+0x25d/0x47a [videodev] [  136.928097]  ?
> > > copy_overflow+0x14/0x14 [videodev] [  136.928115]
> > > v4l2_ioctl+0x4d/0x58 [videodev] [  136.928123]  vfs_ioctl+0x1b/0x28
> > > [  136.928130]  do_vfs_ioctl+0x4de/0x566 [  136.928139]
> > > ksys_ioctl+0x50/0x70 [  136.928146]  __x64_sys_ioctl+0x16/0x19 [
> > > 136.928152]  do_syscall_64+0x4d/0x5a [  136.928158]
> > > entry_SYSCALL_64_after_hwframe+0x44/0xa9
> > > [  136.928164] RIP: 0033:0x7f1ec9a84f47 [  136.928169] Code: 00 00
> > > 00 48 8b 05 51 6f 2c 00 64 c7 00 26 00 00 00 48
> > > c7 c0 ff ff ff ff c3 66 2e 0f 1f 84 00 00 00 00 00 b8 10 00 00 00 0f
> > > 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 21 6f 2c 00 f7 d8 64 89 01
> > > 48 [  136.928173] RSP: 002b:7ffe279e6188 EFLAGS: 0246
> ORIG_RAX:
> > > 0010
> > > [  136.928178] RAX: ffda RBX: 0007 RCX:
> > > 7f1ec9a84f47
> > > [  136.928181] RDX: 7ffe279e6194 RSI: 40045612 RDI:
> > > 0003
> > > [  136.928184] RBP:  R08: 7f1ec776d000 R09:
> > > 
> > > [  136.928188] R10: 0020 R11: 0246 R12:
> > > 7ffe279e6360
> > > [  136.928191] R13: 0004 R14: 7ffe279e6360 R15:
> > > 7ffe279e8826
> > > [  136.928198] Modules linked in: ccm zram arc4 iwlmvm mac80211
> > > intel_rapl x86_pkg_temp_thermal intel_powerclamp coretemp iwlwifi
> > > cfg80211 hid_multitouch ipu3_imgu ipu3_cio2 8250_dw

RE: [PATCH v7 03/16] v4l: Add Intel IPU3 meta data uAPI

[Mani, Rajmohan]

Hi Hans,

> Subject: Re: [PATCH v7 03/16] v4l: Add Intel IPU3 meta data uAPI
> 
> On 11/07/18 00:27, Mani, Rajmohan wrote:
> > Hi Mauro,
> >
> > Thanks for the reviews.
> >
> >> Subject: Re: [PATCH v7 03/16] v4l: Add Intel IPU3 meta data uAPI
> >>
> >> Hi Mauro,
> >>
> >> On Fri, Nov 2, 2018 at 10:49 PM Mauro Carvalho Chehab
> >>  wrote:
> >>>
> >>> Em Mon, 29 Oct 2018 15:22:57 -0700
> >>> Yong Zhi  escreveu:
> >> [snip]
> >>>> +struct ipu3_uapi_awb_config_s {
> >>>> + __u16 rgbs_thr_gr;
> >>>> + __u16 rgbs_thr_r;
> >>>> + __u16 rgbs_thr_gb;
> >>>> + __u16 rgbs_thr_b;
> >>>> + struct ipu3_uapi_grid_config grid; }
> >>>> +__attribute__((aligned(32))) __packed;
> >>>
> >>> Hmm... Kernel defines a macro for aligned attribute:
> >>>
> >>> include/linux/compiler_types.h:#define __aligned(x)
> >> __attribute__((aligned(x)))
> >>>
> >>
> >> First, thanks for review!
> >>
> >> Maybe I missed something, but last time I checked, it wasn't
> >> accessible from UAPI headers in userspace.
> >
> > Ack. We see that's still the case.
> >
> >>
> >>> I'm not a gcc expert, but it sounds weird to first ask it to align
> >>> with 32 bits and then have __packed (with means that pads should be
> >>> removed).
> >>>
> >>> In other words, I *guess* is it should either be __packed or
> >>> __aligned(32).
> >>>
> >>> Not that it would do any difference, in practice, as this specific
> >>> struct has a size with is multiple of 32 bits, but let's do the
> >>> right annotation here, not mixing two incompatible alignment
> requirements.
> >>>
> >>
> >> My understanding was that __packed makes the compiler not insert any
> >> alignment between particular fields of the struct, while __aligned
> >> makes the whole struct be aligned at given boundary, if placed in
> >> another struct. If I didn't miss anything, having both should make perfect
> sense here.
> >
> > Ack
> >
> > I also recall that as part of addressing review comments  (from Hans
> > and Sakari), on earlier versions of this patch series, we added
> > __packed attribute to all structs to ensure the size of the structs remains 
> > the
> same between 32 and 64 bit builds.
> >
> > The addition of structure members of the name padding[x] in some of
> > the structs ensures that respective members are aligned at 32 byte
> > boundaries, while the overall size of the structs remain the same between 32
> and 64 bit builds.
> 
> I recommend that this is documented in the header. It's not a common
> construction so an explanation will help.

Ack.

> 
> Regards,
> 
>   Hans
> 
> >
> > Thanks
> > Raj
> >
> >>
> >> Best regards,
> >> Tomasz

RE: [PATCH v7 05/16] intel-ipu3: abi: Add structs

[Mani, Rajmohan]

Hi Sakari,

> Subject: Re: [PATCH v7 05/16] intel-ipu3: abi: Add structs
> 
> Hi Raj,
> 
> On Mon, Nov 05, 2018 at 07:05:53PM +, Mani, Rajmohan wrote:
> > Hi Sakari,
> >
> > > Subject: Re: [PATCH v7 05/16] intel-ipu3: abi: Add structs
> > >
> > > Hi Yong,
> > >
> > > On Mon, Oct 29, 2018 at 03:22:59PM -0700, Yong Zhi wrote:
> > > > This add all the structs of IPU3 firmware ABI.
> > > >
> > > > Signed-off-by: Yong Zhi 
> > > > Signed-off-by: Rajmohan Mani 
> > >
> > > ...
> > >
> > > > +struct imgu_abi_shd_intra_frame_operations_data {
> > > > +   struct imgu_abi_acc_operation
> > > > +   operation_list[IMGU_ABI_SHD_MAX_OPERATIONS]
> > > __attribute__((aligned(32)));
> > > > +   struct imgu_abi_acc_process_lines_cmd_data
> > > > +   process_lines_data[IMGU_ABI_SHD_MAX_PROCESS_LINES]
> > > __attribute__((aligned(32)));
> > > > +   struct imgu_abi_shd_transfer_luts_set_data
> > > > +   transfer_data[IMGU_ABI_SHD_MAX_TRANSFERS]
> > > > +__attribute__((aligned(32)));
> > >
> > > Could you replace this wth __aligned(32), please? The same for the
> > > rest of the header.
> > >
> >
> > Using __aligned(32) in the uAPI header resulted in compilation errors
> > in user space / camera HAL code.
> >
> > e.g
> > ../../../../../../../../usr/include/linux/intel-ipu3.h:464:57: error: 
> > expected ';'
> > at end of declaration list
> >  __u8 bayer_table[IPU3_UAPI_AWB_FR_BAYER_TABLE_MAX_SIZE]
> > __aligned(32);
> >
> > So we ended up using __attribute__((aligned(32))) format in uAPI
> > header and to be consistent, we followed the same format in ABI header as
> well.
> >
> > Let us know if it's okay to deviate between uAPI and ABI header for
> > this alignment qualifier.
> 
> There's a reason for using __attribute__((aligned(32))) in the uAPI header, 
> but
> not in the in-kernel headers where __aligned(32) is preferred.
> 
> I have a patch for addressing this for the uAPI headers as well so
> __aligned(32) could be used there, too; I'll submit it soon. Let's see...
> there are kerneldoc issues still in this area.
> 

Great. Thanks for the patch to address this need.

> --
> Regards,
> 
> Sakari Ailus
> sakari.ai...@linux.intel.com

RE: [PATCH v7 03/16] v4l: Add Intel IPU3 meta data uAPI

[Mani, Rajmohan]

Hi Mauro,

Thanks for the reviews.

> Subject: Re: [PATCH v7 03/16] v4l: Add Intel IPU3 meta data uAPI
> 
> Hi Mauro,
> 
> On Fri, Nov 2, 2018 at 10:49 PM Mauro Carvalho Chehab
>  wrote:
> >
> > Em Mon, 29 Oct 2018 15:22:57 -0700
> > Yong Zhi  escreveu:
> [snip]
> > > +struct ipu3_uapi_awb_config_s {
> > > + __u16 rgbs_thr_gr;
> > > + __u16 rgbs_thr_r;
> > > + __u16 rgbs_thr_gb;
> > > + __u16 rgbs_thr_b;
> > > + struct ipu3_uapi_grid_config grid; }
> > > +__attribute__((aligned(32))) __packed;
> >
> > Hmm... Kernel defines a macro for aligned attribute:
> >
> > include/linux/compiler_types.h:#define __aligned(x)
> __attribute__((aligned(x)))
> >
> 
> First, thanks for review!
> 
> Maybe I missed something, but last time I checked, it wasn't accessible from
> UAPI headers in userspace.

Ack. We see that's still the case.

> 
> > I'm not a gcc expert, but it sounds weird to first ask it to align
> > with 32 bits and then have __packed (with means that pads should be
> > removed).
> >
> > In other words, I *guess* is it should either be __packed or
> > __aligned(32).
> >
> > Not that it would do any difference, in practice, as this specific
> > struct has a size with is multiple of 32 bits, but let's do the right
> > annotation here, not mixing two incompatible alignment requirements.
> >
> 
> My understanding was that __packed makes the compiler not insert any
> alignment between particular fields of the struct, while __aligned makes the
> whole struct be aligned at given boundary, if placed in another struct. If I
> didn't miss anything, having both should make perfect sense here.

Ack

I also recall that as part of addressing review comments  (from Hans and 
Sakari),
on earlier versions of this patch series, we added __packed attribute to all 
structs
to ensure the size of the structs remains the same between 32 and 64 bit builds.

The addition of structure members of the name padding[x] in some of the structs
ensures that respective members are aligned at 32 byte boundaries, while the
overall size of the structs remain the same between 32 and 64 bit builds.

Thanks
Raj

> 
> Best regards,
> Tomasz

RE: [PATCH v7 05/16] intel-ipu3: abi: Add structs

[Mani, Rajmohan]

Hi Sakari,

> -Original Message-
> From: Sakari Ailus [mailto:sakari.ai...@linux.intel.com]
> Sent: Monday, November 05, 2018 12:28 AM
> To: Zhi, Yong 
> Cc: linux-media@vger.kernel.org; tf...@chromium.org; mche...@kernel.org;
> hans.verk...@cisco.com; laurent.pinch...@ideasonboard.com; Mani,
> Rajmohan ; Zheng, Jian Xu
> ; Hu, Jerry W ; Toivonen,
> Tuukka ; Qiu, Tian Shu
> ; Cao, Bingbu 
> Subject: Re: [PATCH v7 05/16] intel-ipu3: abi: Add structs
> 
> Hi Yong,
> 
> On Mon, Oct 29, 2018 at 03:22:59PM -0700, Yong Zhi wrote:
> > This add all the structs of IPU3 firmware ABI.
> >
> > Signed-off-by: Yong Zhi 
> > Signed-off-by: Rajmohan Mani 
> 
> ...
> 
> > +struct imgu_abi_shd_intra_frame_operations_data {
> > +   struct imgu_abi_acc_operation
> > +   operation_list[IMGU_ABI_SHD_MAX_OPERATIONS]
> __attribute__((aligned(32)));
> > +   struct imgu_abi_acc_process_lines_cmd_data
> > +   process_lines_data[IMGU_ABI_SHD_MAX_PROCESS_LINES]
> __attribute__((aligned(32)));
> > +   struct imgu_abi_shd_transfer_luts_set_data
> > +   transfer_data[IMGU_ABI_SHD_MAX_TRANSFERS]
> > +__attribute__((aligned(32)));
> 
> Could you replace this wth __aligned(32), please? The same for the rest of the
> header.
> 

Using __aligned(32) in the uAPI header resulted in compilation errors in
user space / camera HAL code.

e.g
../../../../../../../../usr/include/linux/intel-ipu3.h:464:57: error: expected 
';' 
at end of declaration list
 __u8 bayer_table[IPU3_UAPI_AWB_FR_BAYER_TABLE_MAX_SIZE] __aligned(32);

So we ended up using __attribute__((aligned(32))) format in uAPI header and
to be consistent, we followed the same format in ABI header as well.

Let us know if it's okay to deviate between uAPI and ABI header for this
alignment qualifier.

> --
> Regards,
> 
> Sakari Ailus
> sakari.ai...@linux.intel.com

RE: [PATCH v1 2/2] v4l: Document Intel IPU3 meta data uAPI

[Mani, Rajmohan]

Hi Sakari,

> -Original Message-
> From: sakari.ai...@linux.intel.com [mailto:sakari.ai...@linux.intel.com]
> Sent: Thursday, October 04, 2018 12:09 AM
> To: Mani, Rajmohan 
> Cc: Mauro Carvalho Chehab ; Hans Verkuil
> ; Zhi, Yong ; linux-
> me...@vger.kernel.org; tf...@chromium.org; mche...@kernel.org;
> hans.verk...@cisco.com; laurent.pinch...@ideasonboard.com; Zheng, Jian Xu
> ; Hu, Jerry W ; Li, Chao C
> ; Qiu, Tian Shu 
> Subject: Re: [PATCH v1 2/2] v4l: Document Intel IPU3 meta data uAPI
> 
> Hi Raj,
> 
> On Wed, Oct 03, 2018 at 10:56:19PM +, Mani, Rajmohan wrote:
> ...
> > > From some comment you had later,
> > > I guess you're meaning that only 3 or 7 are the valid values.
> > >
> > > Yet, you're listing from 2^3 to 2^7, and that's confusing. Perhaps
> > > you want to say, instead, that the valid values are at the 3..7 range?
> > > If so, please use something like "values at the [3..7] range".
> > >
> >
> > As Sakari pointed / preferred in the other thread, we will use the
> > format [3, 7] to represent all integers between 3 and 7, including 3 and 7.
> 
> Feel free to add a reference to this in the format documentation:
> 
> https://en.wikipedia.org/wiki/Interval_(mathematics)>
> 
> I guess the right place would be the top parameter format ReST document.
> 

Ack

> ...
> 
> > > > > + * All above has precision u0.4, range [0..0xF].
> > >
> > > again, what do you mean by u0.4?
> >
> > unsigned integer with 0 bits used for representing whole number, with
> > 4 least significant bits used to represent the fractional part.
> 
> You could refer to this:
> 
> https://en.wikipedia.org/wiki/Q_(number_format)>
> 
> The ux.y notation is more common in the context of software but I couldn't
> find any decent document to refer to.
> 

Ack.
Will stick with ux.y notation with the relevant documentation (unless there
Is a preference for Q format).

> --
> Regards,
> 
> Sakari Ailus
> sakari.ai...@linux.intel.com

RE: [PATCH v1 2/2] v4l: Document Intel IPU3 meta data uAPI

[Mani, Rajmohan]

Hi Mauro,

Thanks for the reviews.

> Subject: Re: [PATCH v1 2/2] v4l: Document Intel IPU3 meta data uAPI
> 
> Em Mon, 13 Aug 2018 15:42:34 +0200
> Hans Verkuil  escreveu:
> 
> > On 15/06/18 05:29, Yong Zhi wrote:
> > > These meta formats are used on Intel IPU3 ImgU video queues
> > > to carry 3A statistics and ISP pipeline parameters.
> > >
> > > V4L2_META_FMT_IPU3_3A
> > > V4L2_META_FMT_IPU3_PARAMS
> > >
> > > Signed-off-by: Yong Zhi 
> > > Signed-off-by: Chao C Li 
> > > Signed-off-by: Rajmohan Mani 
> > > ---
> > >  Documentation/media/uapi/v4l/meta-formats.rst  |1 +
> > >  .../media/uapi/v4l/pixfmt-meta-intel-ipu3.rst  |  174 ++
> > >  include/uapi/linux/intel-ipu3.h| 2816 
> > > 
> > >  3 files changed, 2991 insertions(+)
> > >  create mode 100644 Documentation/media/uapi/v4l/pixfmt-meta-intel-
> ipu3.rst
> > >  create mode 100644 include/uapi/linux/intel-ipu3.h
> > >
> > > diff --git a/Documentation/media/uapi/v4l/meta-formats.rst
> b/Documentation/media/uapi/v4l/meta-formats.rst
> > > index 0c4e1ec..b887fca 100644
> > > --- a/Documentation/media/uapi/v4l/meta-formats.rst
> > > +++ b/Documentation/media/uapi/v4l/meta-formats.rst
> > > @@ -12,6 +12,7 @@ These formats are used for the :ref:`metadata`
> interface only.
> > >  .. toctree::
> > >  :maxdepth: 1
> > >
> > > +pixfmt-meta-intel-ipu3
> > >  pixfmt-meta-uvc
> > >  pixfmt-meta-vsp1-hgo
> > >  pixfmt-meta-vsp1-hgt
> > > diff --git a/Documentation/media/uapi/v4l/pixfmt-meta-intel-ipu3.rst
> b/Documentation/media/uapi/v4l/pixfmt-meta-intel-ipu3.rst
> > > new file mode 100644
> > > index 000..5c050e6
> > > --- /dev/null
> > > +++ b/Documentation/media/uapi/v4l/pixfmt-meta-intel-ipu3.rst
> > > @@ -0,0 +1,174 @@
> > > +.. -*- coding: utf-8; mode: rst -*-
> > > +
> > > +.. _intel-ipu3:
> > > +
> > >
> +***
> ***
> > > +V4L2_META_FMT_IPU3_PARAMS ('ip3p'), V4L2_META_FMT_IPU3_3A
> ('ip3s')
> > >
> +***
> ***
> > > +
> > > +.. c:type:: ipu3_uapi_stats_3a
> > > +
> > > +3A statistics
> > > +=
> > > +
> > > +For IPU3 ImgU, the 3A statistics accelerators collect different 
> > > statistics over
> > > +an input bayer frame. Those statistics, defined in data struct
> > > +:c:type:`ipu3_uapi_stats_3a`, are meta output obtained from "ipu3-imgu
> 3a stat"
> > > +video node, which are then passed to user space for statistics analysis
> > > +using :c:type:`v4l2_meta_format` interface.
> > > +
> > > +The statistics collected are AWB (Auto-white balance) RGBS cells, AWB
> filter
> 
> Just like you did with AWB, AF and AE, please place the full name in
> parenthesis
> for RGBS and AWB.
> 

Ack

> > > +response, AF (Auto-focus) filter response, and AE (Auto-exposure)
> histogram.
> > > +
> > > +struct :c:type:`ipu3_uapi_4a_config` saves configurable parameters for 
> > > all
> above.
> > > +
> > > +
> > > +.. code-block:: c
> > > +
> > > +
> > > + struct ipu3_uapi_stats_3a {
> > > + IPU3_ALIGN struct ipu3_uapi_awb_raw_buffer awb_raw_buffer;
> >
> > IPU3_ALIGN? What's that?
> >
> > OK, after reading the header I see what it does, but I think you should
> > drop it in the documentation since it doesn't help the reader.
> 
> Yeah, that IPU3_ALIGN is confusing.
> 
> Yet, instead of just dropping, I would replace it by a comment
> to explain that the struct is 32-bytes aligned.
> 

Ack

> On a separate (but related) comment, you're declaring it as:
> 
>   #define IPU3_ALIGN
>   __attribute__((aligned(IPU3_UAPI_ISP_WORD_BYTES)))
> 
> This is a gcc-specific dialect. Better to use, instead, __aligned(x)
> which is defined as:
> 
> #define __aligned(x)  __attribute__((aligned(x)))
> 

As discussed in the other thread, the following will be used directly,
as applicable for required data structures.

__attribute__((aligned(32)))

[snip]

> > > +
> > > +#define IPU3_UAPI_ISP_WORD_BYTES 32
> > > +#define IMGU_ABI_PAD __aligned(IPU3_UAPI_ISP_WORD_BYTES)
> > > +#define IPU3_ALIGN
>   __attribute__((aligned(IPU3_UAPI_ISP_WORD_BYTES)))
> >
> > That's weird. Isn't __aligned identical to __attribute__((aligned( ?
> > If so, then these last two defines are also identical.
> 
> Yes, AFAIKT they're identical and __aligned() is the preferred form.
> 

As discussed in the other thread, the following will be used directly,
as applicable for required data structures.

__attribute__((aligned(x)))

> >
> > Why is it aligned to a 32 bytes? Is it a hardware requirement?
> >
> > > +
> > > +/*** ipu3_uapi_stats_3a ***/
> > > +
> > > +#define IPU3_UAPI_MAX_STRIPES2
> > > +#define IPU3_UAPI_MAX_BUBBLE_SIZE10
> > > +
> > > +#define IPU3_UAPI_GRID_START_MASK(BIT(12) - 1)
> > > +#define IPU3_UAPI_GRID_Y_START_ENBIT(15)
> > > +

RE: [PATCH v1 2/2] v4l: Document Intel IPU3 meta data uAPI

[Mani, Rajmohan]

Hi Sakari,

> -Original Message-
> From: Sakari Ailus [mailto:sakari.ai...@linux.intel.com]
> Sent: Thursday, September 13, 2018 12:37 AM
> To: Mani, Rajmohan 
> Cc: Tomasz Figa ; Mauro Carvalho Chehab
> ; Hans Verkuil ; Zhi,
> Yong ; Linux Media Mailing List  me...@vger.kernel.org>; Mauro Carvalho Chehab ;
> Hans Verkuil ; Laurent Pinchart
> ; Zheng, Jian Xu
> ; Hu, Jerry W ; Li, Chao C
> ; Qiu, Tian Shu 
> Subject: Re: [PATCH v1 2/2] v4l: Document Intel IPU3 meta data uAPI
> 
> Hi Raj, Mauro,
> 
> On Fri, Aug 31, 2018 at 10:40:22PM +, Mani, Rajmohan wrote:
> > Hi Sakari, Mauro,
> >
> > > -Original Message-
> > > From: Sakari Ailus [mailto:sakari.ai...@linux.intel.com]
> > > Sent: Tuesday, August 28, 2018 2:16 AM
> > > To: Tomasz Figa 
> > > Cc: Mauro Carvalho Chehab ; Hans
> Verkuil
> > > ; Mani, Rajmohan ; Zhi,
> > > Yong ; Linux Media Mailing List  > > me...@vger.kernel.org>; Mauro Carvalho Chehab
> ;
> > > Hans Verkuil ; Laurent Pinchart
> > > ; Zheng, Jian Xu
> > > ; Hu, Jerry W ; Li,
> > > Chao C ; Qiu, Tian Shu 
> > > Subject: Re: [PATCH v1 2/2] v4l: Document Intel IPU3 meta data uAPI
> > >
> > > Hi Tomasz,
> > >
> > > On Tue, Aug 28, 2018 at 05:56:37PM +0900, Tomasz Figa wrote:
> > > > On Tue, Aug 14, 2018 at 5:50 AM Mauro Carvalho Chehab
> > > >  wrote:
> > > > >
> > > > > Em Mon, 13 Aug 2018 15:42:34 +0200 Hans Verkuil
> > > > >  escreveu:
> > > > >
> > > > > > On 15/06/18 05:29, Yong Zhi wrote:
> > > > > > > These meta formats are used on Intel IPU3 ImgU video queues
> > > > > > > to carry 3A statistics and ISP pipeline parameters.
> > > > > > >
> > > > > > > V4L2_META_FMT_IPU3_3A
> > > > > > > V4L2_META_FMT_IPU3_PARAMS
> > > > > > >
> > > > > > > Signed-off-by: Yong Zhi 
> > > > > > > Signed-off-by: Chao C Li 
> > > > > > > Signed-off-by: Rajmohan Mani 
> > > > > > > ---
> > > > > > >  Documentation/media/uapi/v4l/meta-formats.rst  |1 +
> > > > > > >  .../media/uapi/v4l/pixfmt-meta-intel-ipu3.rst  |  174 ++
> > > > > > >  include/uapi/linux/intel-ipu3.h| 2816
> > > 
> > > > > > >  3 files changed, 2991 insertions(+)  create mode 100644
> > > > > > > Documentation/media/uapi/v4l/pixfmt-meta-intel-ipu3.rst
> > > > > > >  create mode 100644 include/uapi/linux/intel-ipu3.h
> > > > > > >
> > > > > > > diff --git a/Documentation/media/uapi/v4l/meta-formats.rst
> > > > > > > b/Documentation/media/uapi/v4l/meta-formats.rst
> > > > > > > index 0c4e1ec..b887fca 100644
> > > > > > > --- a/Documentation/media/uapi/v4l/meta-formats.rst
> > > > > > > +++ b/Documentation/media/uapi/v4l/meta-formats.rst
> > > > > > > @@ -12,6 +12,7 @@ These formats are used for the
> > > > > > > :ref:`metadata`
> > > interface only.
> > > > > > >  .. toctree::
> > > > > > >  :maxdepth: 1
> > > > > > >
> > > > > > > +pixfmt-meta-intel-ipu3
> > > > > > >  pixfmt-meta-uvc
> > > > > > >  pixfmt-meta-vsp1-hgo
> > > > > > >  pixfmt-meta-vsp1-hgt
> > > > > > > diff --git
> > > > > > > a/Documentation/media/uapi/v4l/pixfmt-meta-intel-ipu3.rst
> > > > > > > b/Documentation/media/uapi/v4l/pixfmt-meta-intel-ipu3.rst
> > > > > > > new file mode 100644
> > > > > > > index 000..5c050e6
> > > > > > > --- /dev/null
> > > > > > > +++ b/Documentation/media/uapi/v4l/pixfmt-meta-intel-ipu3.rs
> > > > > > > +++ t
> > > > > > > @@ -0,0 +1,174 @@
> > > > > > > +.. -*- coding: utf-8; mode: rst -*-
> > > > > > > +
> > > > > > > +.. _intel-ipu3:
> > > > > > > +
> > > > > > >
> > >
> +***
> > > > > > > +*** V4L2_META_FMT_IPU3_PARAMS ('ip3p'),
> > > V4L2_META_FMT_IPU3_3A
> > > > 

RE: [PATCH v1 2/2] v4l: Document Intel IPU3 meta data uAPI

[Mani, Rajmohan]

Hi Sakari,

> -Original Message-
> From: sakari.ai...@linux.intel.com [mailto:sakari.ai...@linux.intel.com]
> Sent: Thursday, September 13, 2018 4:38 AM
> To: Mani, Rajmohan 
> Cc: Hans Verkuil ; Zhi, Yong ; linux-
> me...@vger.kernel.org; tf...@chromium.org; mche...@kernel.org;
> hans.verk...@cisco.com; laurent.pinch...@ideasonboard.com; Zheng, Jian Xu
> ; Hu, Jerry W ; Li, Chao C
> ; Qiu, Tian Shu 
> Subject: Re: [PATCH v1 2/2] v4l: Document Intel IPU3 meta data uAPI
> 
> Hi Raj,
> 
> My apologies for the delayed reply.
> 
> On Fri, Aug 31, 2018 at 11:39:54PM +, Mani, Rajmohan wrote:
> ...
> > > > +struct ipu3_uapi_af_meta_data {
> > > > +   __u8 y_table[IPU3_UAPI_AF_Y_TABLE_MAX_SIZE] IPU3_ALIGN;
> > >
> > > Here IPU3_ALIGN is put at the end...
> > >
> > > > +} __packed;
> > > > +
> > > > +/**
> > > > + * struct ipu3_uapi_af_raw_buffer - AF raw buffer
> > > > + *
> > > > + * @meta_data: raw buffer _uapi_af_meta_data for auto focus
> > > > +meta
> > > data.
> > > > + */
> > > > +struct ipu3_uapi_af_raw_buffer {
> > > > +   IPU3_ALIGN struct ipu3_uapi_af_meta_data meta_data;
> > >
> > > ... and here at the start. Is that due to the difference between an
> > > array and a struct?
> > >
> >
> > No.
> >
> > When preparing uAPI kernelDoc using "make htmldocs", the kernel-doc
> > encounters two type of error/warnings caused by IPU3_ALIGN.
> >
> > case 1:
> > struct IPU3_ALIGN ipu3_uapi_dummy {
> > ...
> > } __packed;
> >
> > "error: Cannot parse struct or union!"
> >
> > case 2:
> > struct ipu3_uapi_dummy {
> > struct ipu3_uapi_x x IPU3_ALIGN;
> > } __packed;
> >
> > "warning: Function parameter or member 'IPU3_ALIGN' not described in
> > 'ipu3_uapi_dummy'"
> >
> > Positioned the attribute syntax without altering the mem layout of the
> > structs, while also making "make htmldocs" to compile fine.
> >
> > Let us know if it's okay to ignore Sphinx warnings.
> 
> I looked a bit at different options for handling this in scripts/kernel-doc 
> but the
> difficulty in macro substitution comes in determining where to do the
> substitution and where not to. That seems unaddressable in the kernel-doc
> script; most of the time you want the definitions as-is while this is likely 
> the
> only case where something else might be appropriate. Making IPU3_ALIGN a
> special case probably wouldn't really fly either.
> 
> In this particular case I'd just write open the alignment requirement so 
> kernel-
> doc can correctly parse it.

Ack.

> 
> --
> Kind regards,
> 
> Sakari Ailus
> sakari.ai...@linux.intel.com

RE: [PATCH v1 2/2] v4l: Document Intel IPU3 meta data uAPI

[Mani, Rajmohan]

Hi Sakari, Mauro,

> -Original Message-
> From: Sakari Ailus [mailto:sakari.ai...@linux.intel.com]
> Sent: Tuesday, August 28, 2018 2:16 AM
> To: Tomasz Figa 
> Cc: Mauro Carvalho Chehab ; Hans Verkuil
> ; Mani, Rajmohan ; Zhi,
> Yong ; Linux Media Mailing List  me...@vger.kernel.org>; Mauro Carvalho Chehab ;
> Hans Verkuil ; Laurent Pinchart
> ; Zheng, Jian Xu
> ; Hu, Jerry W ; Li, Chao C
> ; Qiu, Tian Shu 
> Subject: Re: [PATCH v1 2/2] v4l: Document Intel IPU3 meta data uAPI
> 
> Hi Tomasz,
> 
> On Tue, Aug 28, 2018 at 05:56:37PM +0900, Tomasz Figa wrote:
> > On Tue, Aug 14, 2018 at 5:50 AM Mauro Carvalho Chehab
> >  wrote:
> > >
> > > Em Mon, 13 Aug 2018 15:42:34 +0200
> > > Hans Verkuil  escreveu:
> > >
> > > > On 15/06/18 05:29, Yong Zhi wrote:
> > > > > These meta formats are used on Intel IPU3 ImgU video queues to
> > > > > carry 3A statistics and ISP pipeline parameters.
> > > > >
> > > > > V4L2_META_FMT_IPU3_3A
> > > > > V4L2_META_FMT_IPU3_PARAMS
> > > > >
> > > > > Signed-off-by: Yong Zhi 
> > > > > Signed-off-by: Chao C Li 
> > > > > Signed-off-by: Rajmohan Mani 
> > > > > ---
> > > > >  Documentation/media/uapi/v4l/meta-formats.rst  |1 +
> > > > >  .../media/uapi/v4l/pixfmt-meta-intel-ipu3.rst  |  174 ++
> > > > >  include/uapi/linux/intel-ipu3.h| 2816
> 
> > > > >  3 files changed, 2991 insertions(+)  create mode 100644
> > > > > Documentation/media/uapi/v4l/pixfmt-meta-intel-ipu3.rst
> > > > >  create mode 100644 include/uapi/linux/intel-ipu3.h
> > > > >
> > > > > diff --git a/Documentation/media/uapi/v4l/meta-formats.rst
> > > > > b/Documentation/media/uapi/v4l/meta-formats.rst
> > > > > index 0c4e1ec..b887fca 100644
> > > > > --- a/Documentation/media/uapi/v4l/meta-formats.rst
> > > > > +++ b/Documentation/media/uapi/v4l/meta-formats.rst
> > > > > @@ -12,6 +12,7 @@ These formats are used for the :ref:`metadata`
> interface only.
> > > > >  .. toctree::
> > > > >  :maxdepth: 1
> > > > >
> > > > > +pixfmt-meta-intel-ipu3
> > > > >  pixfmt-meta-uvc
> > > > >  pixfmt-meta-vsp1-hgo
> > > > >  pixfmt-meta-vsp1-hgt
> > > > > diff --git
> > > > > a/Documentation/media/uapi/v4l/pixfmt-meta-intel-ipu3.rst
> > > > > b/Documentation/media/uapi/v4l/pixfmt-meta-intel-ipu3.rst
> > > > > new file mode 100644
> > > > > index 000..5c050e6
> > > > > --- /dev/null
> > > > > +++ b/Documentation/media/uapi/v4l/pixfmt-meta-intel-ipu3.rst
> > > > > @@ -0,0 +1,174 @@
> > > > > +.. -*- coding: utf-8; mode: rst -*-
> > > > > +
> > > > > +.. _intel-ipu3:
> > > > > +
> > > > >
> +***
> > > > > +*** V4L2_META_FMT_IPU3_PARAMS ('ip3p'),
> V4L2_META_FMT_IPU3_3A
> > > > > +('ip3s')
> > > > >
> +***
> > > > > +***
> > > > > +
> > > > > +.. c:type:: ipu3_uapi_stats_3a
> > > > > +
> > > > > +3A statistics
> > > > > +=
> > > > > +
> > > > > +For IPU3 ImgU, the 3A statistics accelerators collect different
> > > > > +statistics over an input bayer frame. Those statistics, defined
> > > > > +in data struct :c:type:`ipu3_uapi_stats_3a`, are meta output obtained
> from "ipu3-imgu 3a stat"
> > > > > +video node, which are then passed to user space for statistics
> > > > > +analysis using :c:type:`v4l2_meta_format` interface.
> > > > > +
> > > > > +The statistics collected are AWB (Auto-white balance) RGBS
> > > > > +cells, AWB filter
> > >
> > > Just like you did with AWB, AF and AE, please place the full name in
> > > parenthesis for RGBS and AWB.
> > >
> > > > > +response, AF (Auto-focus) filter response, and AE (Auto-exposure)
> histogram.
> > > > > +
> > > > > +struct :c:type:`ipu3_uapi_4a_config` saves configurable parameters 
> > > > > for
> all above.
> > > > &

RE: [PATCH v1 2/2] v4l: Document Intel IPU3 meta data uAPI

[Mani, Rajmohan]

Hi Mauro, Hans,

> -Original Message-
> From: Tomasz Figa [mailto:tf...@chromium.org]
> Sent: Wednesday, July 18, 2018 7:53 AM
> To: Hans Verkuil ; Mauro Carvalho Chehab
> 
> Cc: Zhi, Yong ; Sakari Ailus
> ; Linux Media Mailing List  me...@vger.kernel.org>; Laurent Pinchart
> ; Mani, Rajmohan
> ; Zheng, Jian Xu ; Hu,
> Jerry W ; Li, Chao C ; Qiu, Tian
> Shu 
> Subject: Re: [PATCH v1 2/2] v4l: Document Intel IPU3 meta data uAPI
> 
> Hi Mauro, Hans,
> 
> On Fri, Jun 15, 2018 at 12:30 PM Yong Zhi  wrote:
> >
> > These meta formats are used on Intel IPU3 ImgU video queues to carry
> > 3A statistics and ISP pipeline parameters.
> >
> > V4L2_META_FMT_IPU3_3A
> > V4L2_META_FMT_IPU3_PARAMS
> >
> > Signed-off-by: Yong Zhi 
> > Signed-off-by: Chao C Li 
> > Signed-off-by: Rajmohan Mani 
> > ---
> >  Documentation/media/uapi/v4l/meta-formats.rst  |1 +
> >  .../media/uapi/v4l/pixfmt-meta-intel-ipu3.rst  |  174 ++
> >  include/uapi/linux/intel-ipu3.h| 2816 
> > 
> 
> The documentation seems to be quite extensive in current version. Do you
> think it's more acceptable now? Would you be able to take a look?

Hope you had a chance to look into this current version of IPU3 documentation.
Please share your thoughts.

Thanks
Raj

> 
> We obviously need to keep working on the user space framework (and we're in
> process of figuring out how we can proceed further), but having the driver 
> bit-
> rotting downstream might not be a very encouraging factor. ;)
> 
> Best regards,
> Tomasz

RE: [PATCH v3 2/2] media: ak7375: Add ak7375 lens voice coil driver

[Mani, Rajmohan]

Hi Bingbu,

> -Original Message-
> From: Bing Bu Cao [mailto:bingbu@linux.intel.com]
> Sent: Monday, June 04, 2018 11:27 PM
> To: Mani, Rajmohan ; Cao, Bingbu
> ; linux-media@vger.kernel.org
> Cc: sakari.ai...@linux.intel.com; tf...@google.com; jac...@jmondi.org; Qiu,
> Tian Shu ; Zheng, Jian Xu 
> Subject: Re: [PATCH v3 2/2] media: ak7375: Add ak7375 lens voice coil driver
> 
> 
> 
> On 20180605 08:23, Mani, Rajmohan wrote:
> > Hi Bingbu,
> >
> > Please see a couple of comments below.
> >
> >> -Original Message-
> >> From: Cao, Bingbu
> >> Sent: Monday, June 04, 2018 2:00 AM
> >> To: linux-media@vger.kernel.org
> >> Cc: sakari.ai...@linux.intel.com; tf...@google.com;
> >> jac...@jmondi.org; Mani, Rajmohan ;
> >> bingbu@linux.intel.com; Qiu, Tian Shu ;
> >> Zheng, Jian Xu 
> >> Subject: [PATCH v3 2/2] media: ak7375: Add ak7375 lens voice coil
> >> driver
> >>
> >> From: Bingbu Cao 
> >>
> >> Add a v4l2 sub-device driver for the ak7375 lens voice coil.
> >> This is a voice coil module using the i2c bus to control the focus 
> >> position.
> >>
> >> ak7375 can write multiple bytes of data at a time. If more data is
> >> received instead of the stop condition after receiving one byte of
> >> data, the address inside the chip is automatically incremented and
> >> the data is written into the next address.
> >>
> >> The ak7375 can control the position with 12 bits value and consists
> >> of two 8 bit registers show as below:
> >> register 0x00(AK7375_REG_POSITION):
> >>  +---+---+---+---+---+---+---+---+
> >>  |D11|D10|D09|D08|D07|D06|D05|D04|
> >>  +---+---+---+---+---+---+---+---+ register 0x01:
> >>  +---+---+---+---+---+---+---+---+
> >>  |D03|D02|D01|D00|---|---|---|---|
> >>  +---+---+---+---+---+---+---+---+
> >>
> >> This driver support :
> >>  - set ak7375 to standby mode once suspend and
> >>turn it back to active if resume
> >>  - set the position via V4L2_CID_FOCUS_ABSOLUTE ctrl
> >>
> >> Signed-off-by: Tianshu Qiu 
> >> Signed-off-by: Bingbu Cao 
> >>
> >> ---
> >> Changes from v1:
> >>  - correct i2c write
> >>  - add media_entity_pads_init() into probe
> >>  - move the MAINTAINERs change into dt-bindings change
> >>  - correct the compatible string
> >> ---
> >> ---
> >>   drivers/media/i2c/Kconfig  |  10 ++
> >>   drivers/media/i2c/Makefile |   1 +
> >>   drivers/media/i2c/ak7375.c | 278
> >> +
> >>   3 files changed, 289 insertions(+)
> >>   create mode 100644 drivers/media/i2c/ak7375.c
> >>
> >> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> >> index
> >> 341452fe98df..ff3cb5afb0e1 100644
> >> --- a/drivers/media/i2c/Kconfig
> >> +++ b/drivers/media/i2c/Kconfig
> >> @@ -326,6 +326,16 @@ config VIDEO_AD5820
> >>  This is a driver for the AD5820 camera lens voice coil.
> >>  It is used for example in Nokia N900 (RX-51).
> >>
> >> +config VIDEO_AK7375
> >> +  tristate "AK7375 lens voice coil support"
> >> +  depends on I2C && VIDEO_V4L2 && MEDIA_CONTROLLER
> >> +  depends on VIDEO_V4L2_SUBDEV_API
> >> +  help
> >> +This is a driver for the AK7375 camera lens voice coil.
> >> +AK7375 is a 12 bit DAC with 120mA output current sink
> >> +capability. This is designed for linear control of
> >> +voice coil motors, controlled via I2C serial interface.
> >> +
> >>   config VIDEO_DW9714
> >>tristate "DW9714 lens voice coil support"
> >>depends on I2C && VIDEO_V4L2 && MEDIA_CONTROLLER diff --git
> >> a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile index
> >> d679d57cd3b3..05b97e319ea9 100644
> >> --- a/drivers/media/i2c/Makefile
> >> +++ b/drivers/media/i2c/Makefile
> >> @@ -23,6 +23,7 @@ obj-$(CONFIG_VIDEO_SAA7127) += saa7127.o
> >>   obj-$(CONFIG_VIDEO_SAA7185) += saa7185.o
> >>   obj-$(CONFIG_VIDEO_SAA6752HS) += saa6752hs.o
> >>   obj-$(CONFIG_VIDEO_AD5820)  += ad5820.o
> >> +obj-$(CONFIG_VIDEO_AK7375)  += ak7375.o
> >>   obj-$(CONFIG_VIDEO_DW9714)  += dw9714.o
> >>   obj-$(CONFIG_VIDEO_ADV7170) += adv7170.o
> >

RE: [PATCH v3 2/2] media: ak7375: Add ak7375 lens voice coil driver

[Mani, Rajmohan]

Hi Bingbu,

Please see a couple of comments below.

> -Original Message-
> From: Cao, Bingbu
> Sent: Monday, June 04, 2018 2:00 AM
> To: linux-media@vger.kernel.org
> Cc: sakari.ai...@linux.intel.com; tf...@google.com; jac...@jmondi.org;
> Mani, Rajmohan ; bingbu@linux.intel.com;
> Qiu, Tian Shu ; Zheng, Jian Xu
> 
> Subject: [PATCH v3 2/2] media: ak7375: Add ak7375 lens voice coil driver
> 
> From: Bingbu Cao 
> 
> Add a v4l2 sub-device driver for the ak7375 lens voice coil.
> This is a voice coil module using the i2c bus to control the focus position.
> 
> ak7375 can write multiple bytes of data at a time. If more data is received
> instead of the stop condition after receiving one byte of data, the address
> inside the chip is automatically incremented and the data is written into the
> next address.
> 
> The ak7375 can control the position with 12 bits value and consists of two 8 
> bit
> registers show as below:
> register 0x00(AK7375_REG_POSITION):
> +---+---+---+---+---+---+---+---+
> |D11|D10|D09|D08|D07|D06|D05|D04|
> +---+---+---+---+---+---+---+---+
> register 0x01:
> +---+---+---+---+---+---+---+---+
> |D03|D02|D01|D00|---|---|---|---|
> +---+---+---+---+---+---+---+---+
> 
> This driver support :
> - set ak7375 to standby mode once suspend and
>   turn it back to active if resume
> - set the position via V4L2_CID_FOCUS_ABSOLUTE ctrl
> 
> Signed-off-by: Tianshu Qiu 
> Signed-off-by: Bingbu Cao 
> 
> ---
> Changes from v1:
> - correct i2c write
> - add media_entity_pads_init() into probe
> - move the MAINTAINERs change into dt-bindings change
> - correct the compatible string
> ---
> ---
>  drivers/media/i2c/Kconfig  |  10 ++
>  drivers/media/i2c/Makefile |   1 +
>  drivers/media/i2c/ak7375.c | 278
> +
>  3 files changed, 289 insertions(+)
>  create mode 100644 drivers/media/i2c/ak7375.c
> 
> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig index
> 341452fe98df..ff3cb5afb0e1 100644
> --- a/drivers/media/i2c/Kconfig
> +++ b/drivers/media/i2c/Kconfig
> @@ -326,6 +326,16 @@ config VIDEO_AD5820
> This is a driver for the AD5820 camera lens voice coil.
> It is used for example in Nokia N900 (RX-51).
> 
> +config VIDEO_AK7375
> + tristate "AK7375 lens voice coil support"
> + depends on I2C && VIDEO_V4L2 && MEDIA_CONTROLLER
> + depends on VIDEO_V4L2_SUBDEV_API
> + help
> +   This is a driver for the AK7375 camera lens voice coil.
> +   AK7375 is a 12 bit DAC with 120mA output current sink
> +   capability. This is designed for linear control of
> +   voice coil motors, controlled via I2C serial interface.
> +
>  config VIDEO_DW9714
>   tristate "DW9714 lens voice coil support"
>   depends on I2C && VIDEO_V4L2 && MEDIA_CONTROLLER diff --git
> a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile index
> d679d57cd3b3..05b97e319ea9 100644
> --- a/drivers/media/i2c/Makefile
> +++ b/drivers/media/i2c/Makefile
> @@ -23,6 +23,7 @@ obj-$(CONFIG_VIDEO_SAA7127) += saa7127.o
>  obj-$(CONFIG_VIDEO_SAA7185) += saa7185.o
>  obj-$(CONFIG_VIDEO_SAA6752HS) += saa6752hs.o
>  obj-$(CONFIG_VIDEO_AD5820)  += ad5820.o
> +obj-$(CONFIG_VIDEO_AK7375)  += ak7375.o
>  obj-$(CONFIG_VIDEO_DW9714)  += dw9714.o
>  obj-$(CONFIG_VIDEO_ADV7170) += adv7170.o
>  obj-$(CONFIG_VIDEO_ADV7175) += adv7175.o diff --git
> a/drivers/media/i2c/ak7375.c b/drivers/media/i2c/ak7375.c new file mode
> 100644 index ..94bcadae4258
> --- /dev/null
> +++ b/drivers/media/i2c/ak7375.c
> @@ -0,0 +1,278 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (C) 2018 Intel Corporation
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#define AK7375_MAX_FOCUS_POS 4095
> +/*
> + * This sets the minimum granularity for the focus positions.
> + * A value of 1 gives maximum accuracy for a desired focus position  */
> +#define AK7375_FOCUS_STEPS   1
> +/*
> + * This acts as the minimum granularity of lens movement.
> + * Keep this value power of 2, so the control steps can be
> + * uniformly adjusted for gradual lens movement, with desired
> + * number of control steps.
> + */
> +#define AK7375_CTRL_STEPS64
> +#define AK7375_CTRL_DELAY_US 1000
> +
> +#define AK7375_REG_POSITION  0x0
> +#define AK7375_REG_CONT  0x2
> +#define AK7375_MODE_ACTIVE   0x0
> +#define AK7375_MODE_STANDBY  0x40
> +
> +/* ak7375 device structure */
> +struct ak7375_device {
> + struct v4l2_ctrl

RE: [PATCH v2] [media] MAINTAINERS: Update entry for Intel IPU3 cio2 driver

[Mani, Rajmohan]

Hi Yong,

> -Original Message-
> From: Zhi, Yong
> Sent: Wednesday, May 30, 2018 3:10 PM
> To: linux-media@vger.kernel.org; sakari.ai...@linux.intel.com
> Cc: Mani, Rajmohan ; Zheng, Jian Xu
> ; Qiu, Tian Shu ; Cao,
> Bingbu ; Zhi, Yong 
> Subject: [PATCH v2] [media] MAINTAINERS: Update entry for Intel IPU3 cio2
> driver
> 
> This patch adds three more maintainers to the IPU3 CIO2 driver.

nit:  The above line should be changed to reflect this v2 patch.

> 
> Signed-off-by: Yong Zhi 
> ---
>  MAINTAINERS | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index a38e24a..3dd530e 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -7157,6 +7157,9 @@ F:  drivers/dma/iop-adma.c
>  INTEL IPU3 CSI-2 CIO2 DRIVER
>  M:   Yong Zhi 
>  M:   Sakari Ailus 
> +M:   Bingbu Cao 
> +R:   Tian Shu Qiu 
> +R:   Jian Xu Zheng 
>  L:   linux-media@vger.kernel.org
>  S:   Maintained
>  F:   drivers/media/pci/intel/ipu3/
> --
> 2.7.4

RE: [PATCH v4 00/12] Intel IPU3 ImgU patchset

[Mani, Rajmohan]

Hi Mauro and all,

> > Subject: RE: [PATCH v4 00/12] Intel IPU3 ImgU patchset
> >
> > Hi Mauro,
> >
> > > > > Subject: Re: [PATCH v4 00/12] Intel IPU3 ImgU patchset
> > > > >
> > > > > Hi,
> > > > >
> > > > > Em Fri, 17 Nov 2017 02:58:56 + "Mani, Rajmohan"
> > > > > <rajmohan.m...@intel.com> escreveu:
> > > > >
> > > > > > Here is an update on the IPU3 documentation that we are
> > > > > > currently working
> > > > > on.
> > > > > >
> > > > > > Image processing in IPU3 relies on the following.
> > > > > >
> > > > > > 1) HW configuration to enable ISP and
> > > > > > 2) setting customer specific 3A Tuning / Algorithm Parameters
> > > > > > to achieve
> > > > > desired image quality.
> > > > > >
> > > > > > We intend to provide documentation on ImgU driver programming
> > > > > > interface
> > > > > to help users of this driver to configure and enable ISP HW to
> > > > > meet their needs.  This documentation will include details on
> > > > > complete
> > > > > V4L2 Kernel driver interface and IO-Control parameters, except
> > > > > for the ISP internal algorithm and its parameters (which is
> > > > > Intel proprietary
> > IP).
> > > > >
> > > > > Sakari asked me to take a look on this thread, specifically on
> > > > > this email. I took a look on the other e-mails from this thread
> > > > > that are discussing about this IP block.
> > > > >
> > > > > I understand that Intel wants to keep their internal 3A
> > > > > algorithm protected, just like other vendors protect their own
> > > > > algos. It was never a requirement to open whatever algorithm are
> > > > > used inside a hardware (or firmware). The only requirement is
> > > > > that firmwares should be licensed with redistribution
> > > > > permission, ideally merged at linux-firmware
> > > > git tree.
> > > > >
> > > > > Yet, what I don't understand is why Intel also wants to also
> > > > > protect the interface for such 3A hardware/firmware algorithm.
> > > > > The parameters that are passed from an userspace application to
> > > > > Intel ISP logic doesn't contain the algorithm itself. What's the
> > > > > issue of documenting the meaning of each parameter?
> > > > >
> > > >
> > > > Thanks for looking into this.
> > > >
> > > > To achieve improved image quality using IPU3, 3A (Auto White
> > > > balance, Auto Focus and Auto Exposure) Tuning parameters specific
> > > > to a given camera sensor module, are converted to Intel ISP
> > > > algorithm parameters in user space camera HAL using AIC (Automatic
> > > > ISP
> > Configuration) library.
> > > >
> > > > As a unique design of Intel ISP, it exposes very detailed
> > > > algorithm parameters (~ 1 parameters) to configure ISP's image
> > > > processing algorithm per each image fame in runtime. Typical
> > > > Camera SW developers (including those at
> > > > Intel) are not expected to fully understand and directly set these
> > > > parameters to configure the ISP algorithm blocks. Due to the
> > > > above, a user space AIC library (in binary form) is provided to
> > > > generate ISP Algorithm specific parameters, for a given set of 3A
> > > > tuning parameters. It significantly reduces the efforts of SW
> > > > development in ISP HW
> > > configuration.
> > > >
> > > > On the other hand, the ISP algorithm details could be deduced
> > > > readily through these detailed parameters by other ISP experts
> > > > outside
> > Intel.
> > > > This is the reason that we want to keep these parameter
> > > > definitions as Intel
> > > proprietary IP.
> > > >
> > > > We are fully aware of your concerns on how to enable open source
> > > > developers to use Intel ISP through up-streamed Kernel Driver.
> > > > Internally, we are working on the license for this AIC library
> > > > release now (as Hans said NDA license is not acceptable). We
> > > > believe this will be more efficient way to help open source developers.
> > > >
> > > > This AIC library release would be a binary-only release. This AIC
> > > > library does not use any kernel uAPIs directly. The user space
> > > > Camera HAL that uses kernel uAPIs is available at
> > > > https://chromium.googlesource.com/chromiumos/platform/arc-
> > > > camera/+/master
> > > >
> >
> > The AIC library (in binary form) is available here.
> > https://storage.googleapis.com/chromeos-localmirror/distfiles/intel-3a
> > -libs-
> > bin-2017.09.27.tbz2
> >
> > Licensing information can be found in ./LICENSE.intel_3a_library file
> > after unzipping the tar file.
> >
> > >
> > > Just pinging to know your thoughts on this.
> > >

To follow up further on this, we have posted the preliminary documentation on
bnr (bayer noise reduction) parameters as below.

https://www.mail-archive.com/linux-media@vger.kernel.org/msg128683.html

We would like hear your early feedback and apply the same towards the
documentation of remaining IPU3 parameters.

Thanks
Raj

RE: [PATCH v4 00/12] Intel IPU3 ImgU patchset

[Mani, Rajmohan]

Adding Tomasz...

> -Original Message-
> From: Mani, Rajmohan
> Sent: Tuesday, February 20, 2018 5:56 PM
> To: 'Mauro Carvalho Chehab' <mche...@s-opensource.com>
> Cc: Zhi, Yong <yong@intel.com>; 'linux-media@vger.kernel.org'  me...@vger.kernel.org>; 'sakari.ai...@linux.intel.com'
> <sakari.ai...@linux.intel.com>; Zheng, Jian Xu <jian.xu.zh...@intel.com>;
> Toivonen, Tuukka <tuukka.toivo...@intel.com>; Hu, Jerry W
> <jerry.w...@intel.com>; 'a...@arndb.de' <a...@arndb.de>; 'h...@lst.de'
> <h...@lst.de>; 'robin.mur...@arm.com' <robin.mur...@arm.com>;
> 'io...@lists.linux-foundation.org' <io...@lists.linux-foundation.org>
> Subject: RE: [PATCH v4 00/12] Intel IPU3 ImgU patchset
> 
> Hi Mauro,
> 
> > > > Subject: Re: [PATCH v4 00/12] Intel IPU3 ImgU patchset
> > > >
> > > > Hi,
> > > >
> > > > Em Fri, 17 Nov 2017 02:58:56 + "Mani, Rajmohan"
> > > > <rajmohan.m...@intel.com> escreveu:
> > > >
> > > > > Here is an update on the IPU3 documentation that we are
> > > > > currently working
> > > > on.
> > > > >
> > > > > Image processing in IPU3 relies on the following.
> > > > >
> > > > > 1) HW configuration to enable ISP and
> > > > > 2) setting customer specific 3A Tuning / Algorithm Parameters to
> > > > > achieve
> > > > desired image quality.
> > > > >
> > > > > We intend to provide documentation on ImgU driver programming
> > > > > interface
> > > > to help users of this driver to configure and enable ISP HW to
> > > > meet their needs.  This documentation will include details on
> > > > complete
> > > > V4L2 Kernel driver interface and IO-Control parameters, except for
> > > > the ISP internal algorithm and its parameters (which is Intel 
> > > > proprietary
> IP).
> > > >
> > > > Sakari asked me to take a look on this thread, specifically on
> > > > this email. I took a look on the other e-mails from this thread
> > > > that are discussing about this IP block.
> > > >
> > > > I understand that Intel wants to keep their internal 3A algorithm
> > > > protected, just like other vendors protect their own algos. It was
> > > > never a requirement to open whatever algorithm are used inside a
> > > > hardware (or firmware). The only requirement is that firmwares
> > > > should be licensed with redistribution permission, ideally merged
> > > > at linux-firmware
> > > git tree.
> > > >
> > > > Yet, what I don't understand is why Intel also wants to also
> > > > protect the interface for such 3A hardware/firmware algorithm. The
> > > > parameters that are passed from an userspace application to Intel
> > > > ISP logic doesn't contain the algorithm itself. What's the issue
> > > > of documenting the meaning of each parameter?
> > > >
> > >
> > > Thanks for looking into this.
> > >
> > > To achieve improved image quality using IPU3, 3A (Auto White
> > > balance, Auto Focus and Auto Exposure) Tuning parameters specific to
> > > a given camera sensor module, are converted to Intel ISP algorithm
> > > parameters in user space camera HAL using AIC (Automatic ISP
> Configuration) library.
> > >
> > > As a unique design of Intel ISP, it exposes very detailed algorithm
> > > parameters (~ 1 parameters) to configure ISP's image processing
> > > algorithm per each image fame in runtime. Typical Camera SW
> > > developers (including those at
> > > Intel) are not expected to fully understand and directly set these
> > > parameters to configure the ISP algorithm blocks. Due to the above,
> > > a user space AIC library (in binary form) is provided to generate
> > > ISP Algorithm specific parameters, for a given set of 3A tuning
> > > parameters. It significantly reduces the efforts of SW development
> > > in ISP HW
> > configuration.
> > >
> > > On the other hand, the ISP algorithm details could be deduced
> > > readily through these detailed parameters by other ISP experts outside
> Intel.
> > > This is the reason that we want to keep these parameter definitions
> > > as Intel
> > proprietary IP.
> > >
> > > We are fully aware of your concerns on how to enable open source
> > > developers to use Intel ISP through up-streamed Kernel Driver.
> > > Internally, we are working on the license for this AIC library
> > > release now (as Hans said NDA license is not acceptable). We believe
> > > this will be more efficient way to help open source developers.
> > >
> > > This AIC library release would be a binary-only release. This AIC
> > > library does not use any kernel uAPIs directly. The user space
> > > Camera HAL that uses kernel uAPIs is available at
> > > https://chromium.googlesource.com/chromiumos/platform/arc-
> > > camera/+/master
> > >
> 
> The AIC library (in binary form) is available here.
> https://storage.googleapis.com/chromeos-localmirror/distfiles/intel-3a-libs-
> bin-2017.09.27.tbz2
> 
> Licensing information can be found in ./LICENSE.intel_3a_library file after
> unzipping the tar file.
> 
> >
> > Just pinging to know your thoughts on this.
> >
> > Thanks
> > Raj

RE: [PATCH v4 00/12] Intel IPU3 ImgU patchset

[Mani, Rajmohan]

Hi Mauro,

> > > Subject: Re: [PATCH v4 00/12] Intel IPU3 ImgU patchset
> > >
> > > Hi,
> > >
> > > Em Fri, 17 Nov 2017 02:58:56 +
> > > "Mani, Rajmohan" <rajmohan.m...@intel.com> escreveu:
> > >
> > > > Here is an update on the IPU3 documentation that we are currently
> > > > working
> > > on.
> > > >
> > > > Image processing in IPU3 relies on the following.
> > > >
> > > > 1) HW configuration to enable ISP and
> > > > 2) setting customer specific 3A Tuning / Algorithm Parameters to
> > > > achieve
> > > desired image quality.
> > > >
> > > > We intend to provide documentation on ImgU driver programming
> > > > interface
> > > to help users of this driver to configure and enable ISP HW to meet
> > > their needs.  This documentation will include details on complete
> > > V4L2 Kernel driver interface and IO-Control parameters, except for
> > > the ISP internal algorithm and its parameters (which is Intel proprietary 
> > > IP).
> > >
> > > Sakari asked me to take a look on this thread, specifically on this
> > > email. I took a look on the other e-mails from this thread that are
> > > discussing about this IP block.
> > >
> > > I understand that Intel wants to keep their internal 3A algorithm
> > > protected, just like other vendors protect their own algos. It was
> > > never a requirement to open whatever algorithm are used inside a
> > > hardware (or firmware). The only requirement is that firmwares
> > > should be licensed with redistribution permission, ideally merged at
> > > linux-firmware
> > git tree.
> > >
> > > Yet, what I don't understand is why Intel also wants to also protect
> > > the interface for such 3A hardware/firmware algorithm. The
> > > parameters that are passed from an userspace application to Intel
> > > ISP logic doesn't contain the algorithm itself. What's the issue of
> > > documenting the meaning of each parameter?
> > >
> >
> > Thanks for looking into this.
> >
> > To achieve improved image quality using IPU3, 3A (Auto White balance,
> > Auto Focus and Auto Exposure) Tuning parameters specific to a given
> > camera sensor module, are converted to Intel ISP algorithm parameters
> > in user space camera HAL using AIC (Automatic ISP Configuration) library.
> >
> > As a unique design of Intel ISP, it exposes very detailed algorithm
> > parameters (~ 1 parameters) to configure ISP's image processing
> > algorithm per each image fame in runtime. Typical Camera SW developers
> > (including those at
> > Intel) are not expected to fully understand and directly set these
> > parameters to configure the ISP algorithm blocks. Due to the above, a
> > user space AIC library (in binary form) is provided to generate ISP
> > Algorithm specific parameters, for a given set of 3A tuning
> > parameters. It significantly reduces the efforts of SW development in ISP HW
> configuration.
> >
> > On the other hand, the ISP algorithm details could be deduced readily
> > through these detailed parameters by other ISP experts outside Intel.
> > This is the reason that we want to keep these parameter definitions as Intel
> proprietary IP.
> >
> > We are fully aware of your concerns on how to enable open source
> > developers to use Intel ISP through up-streamed Kernel Driver.
> > Internally, we are working on the license for this AIC library release
> > now (as Hans said NDA license is not acceptable). We believe this will
> > be more efficient way to help open source developers.
> >
> > This AIC library release would be a binary-only release. This AIC
> > library does not use any kernel uAPIs directly. The user space Camera
> > HAL that uses kernel uAPIs is available at
> > https://chromium.googlesource.com/chromiumos/platform/arc-
> > camera/+/master
> >

The AIC library (in binary form) is available here.
https://storage.googleapis.com/chromeos-localmirror/distfiles/intel-3a-libs-bin-2017.09.27.tbz2

Licensing information can be found in ./LICENSE.intel_3a_library file after 
unzipping the tar file.

> 
> Just pinging to know your thoughts on this.
> 
> Thanks
> Raj

RE: [PATCH v4 00/12] Intel IPU3 ImgU patchset

[Mani, Rajmohan]

Hi Mauro,

> -Original Message-
> From: Mani, Rajmohan
> Sent: Tuesday, December 26, 2017 2:31 PM
> To: 'Mauro Carvalho Chehab' <mche...@s-opensource.com>
> Cc: Zhi, Yong <yong@intel.com>; linux-media@vger.kernel.org;
> sakari.ai...@linux.intel.com; Zheng, Jian Xu <jian.xu.zh...@intel.com>;
> Toivonen, Tuukka <tuukka.toivo...@intel.com>; Hu, Jerry W
> <jerry.w...@intel.com>; a...@arndb.de; h...@lst.de;
> robin.mur...@arm.com; io...@lists.linux-foundation.org
> Subject: RE: [PATCH v4 00/12] Intel IPU3 ImgU patchset
> 
> Hi Mauro,
> 
> > -Original Message-
> > From: Mauro Carvalho Chehab [mailto:mche...@s-opensource.com]
> > Sent: Wednesday, December 20, 2017 5:58 AM
> > To: Mani, Rajmohan <rajmohan.m...@intel.com>
> > Cc: Zhi, Yong <yong@intel.com>; linux-media@vger.kernel.org;
> > sakari.ai...@linux.intel.com; Zheng, Jian Xu
> > <jian.xu.zh...@intel.com>; Toivonen, Tuukka
> > <tuukka.toivo...@intel.com>; Hu, Jerry W <jerry.w...@intel.com>;
> > a...@arndb.de; h...@lst.de; robin.mur...@arm.com;
> > io...@lists.linux-foundation.org
> > Subject: Re: [PATCH v4 00/12] Intel IPU3 ImgU patchset
> >
> > Hi,
> >
> > Em Fri, 17 Nov 2017 02:58:56 +
> > "Mani, Rajmohan" <rajmohan.m...@intel.com> escreveu:
> >
> > > Here is an update on the IPU3 documentation that we are currently
> > > working
> > on.
> > >
> > > Image processing in IPU3 relies on the following.
> > >
> > > 1) HW configuration to enable ISP and
> > > 2) setting customer specific 3A Tuning / Algorithm Parameters to
> > > achieve
> > desired image quality.
> > >
> > > We intend to provide documentation on ImgU driver programming
> > > interface
> > to help users of this driver to configure and enable ISP HW to meet
> > their needs.  This documentation will include details on complete V4L2
> > Kernel driver interface and IO-Control parameters, except for the ISP
> > internal algorithm and its parameters (which is Intel proprietary IP).
> >
> > Sakari asked me to take a look on this thread, specifically on this
> > email. I took a look on the other e-mails from this thread that are
> > discussing about this IP block.
> >
> > I understand that Intel wants to keep their internal 3A algorithm
> > protected, just like other vendors protect their own algos. It was
> > never a requirement to open whatever algorithm are used inside a
> > hardware (or firmware). The only requirement is that firmwares should
> > be licensed with redistribution permission, ideally merged at linux-firmware
> git tree.
> >
> > Yet, what I don't understand is why Intel also wants to also protect
> > the interface for such 3A hardware/firmware algorithm. The parameters
> > that are passed from an userspace application to Intel ISP logic
> > doesn't contain the algorithm itself. What's the issue of documenting
> > the meaning of each parameter?
> >
> 
> Thanks for looking into this.
> 
> To achieve improved image quality using IPU3, 3A (Auto White balance, Auto
> Focus and Auto Exposure) Tuning parameters specific to a given camera sensor
> module, are converted to Intel ISP algorithm parameters in user space camera
> HAL using AIC (Automatic ISP Configuration) library.
> 
> As a unique design of Intel ISP, it exposes very detailed algorithm parameters
> (~ 1 parameters) to configure ISP's image processing algorithm per each
> image fame in runtime. Typical Camera SW developers (including those at
> Intel) are not expected to fully understand and directly set these parameters 
> to
> configure the ISP algorithm blocks. Due to the above, a user space AIC library
> (in binary form) is provided to generate ISP Algorithm specific parameters, 
> for a
> given set of 3A tuning parameters. It significantly reduces the efforts of SW
> development in ISP HW configuration.
> 
> On the other hand, the ISP algorithm details could be deduced readily through
> these detailed parameters by other ISP experts outside Intel. This is the 
> reason
> that we want to keep these parameter definitions as Intel proprietary IP.
> 
> We are fully aware of your concerns on how to enable open source developers
> to use Intel ISP through up-streamed Kernel Driver. Internally, we are working
> on the license for this AIC library release now (as Hans said NDA license is 
> not
> acceptable). We believe this will be more efficient way to help open source
> developers.
> 
> This AIC library release would be a binary-only release. This AIC library 
> does not
> use any kernel uAPIs directly. The user space Camera HAL that uses kernel
> uAPIs is available at
> https://chromium.googlesource.com/chromiumos/platform/arc-
> camera/+/master
> 

Just pinging to know your thoughts on this.

Thanks
Raj

RE: [PATCH 1/2] media: intel-ipu3: cio2: fix a crash with out-of-bounds access

[Mani, Rajmohan]

Hi Yong,

> -Original Message-
> From: Zhi, Yong
> Sent: Wednesday, January 03, 2018 6:57 PM
> To: linux-media@vger.kernel.org; sakari.ai...@linux.intel.com
> Cc: tf...@chromium.org; Mani, Rajmohan <rajmohan.m...@intel.com>; Zhi,
> Yong <yong@intel.com>; Cao, Bingbu <bingbu@intel.com>
> Subject: [PATCH 1/2] media: intel-ipu3: cio2: fix a crash with out-of-bounds
> access
> 
> When dmabuf is used for BLOB type frame, the frame buffers allocated by
> gralloc will hold more pages than the valid frame data due to height 
> alignment.
> 
> In this case, the page numbers in sg list could exceed the FBPT upper limit
> value - max_lops(8)*1024 to cause crash.
> 
> Limit the LOP access to the valid data length to avoid FBPT sub-entries
> overflow.
> 
> Signed-off-by: Yong Zhi <yong@intel.com>
> Signed-off-by: Cao Bing Bu <bingbu@intel.com>
> ---
>  drivers/media/pci/intel/ipu3/ipu3-cio2.c | 7 +--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> index 941caa987dab..949f43d206ad 100644
> --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> @@ -838,8 +838,9 @@ static int cio2_vb2_buf_init(struct vb2_buffer *vb)
>   container_of(vb, struct cio2_buffer, vbb.vb2_buf);
>   static const unsigned int entries_per_page =
>   CIO2_PAGE_SIZE / sizeof(u32);
> - unsigned int pages = DIV_ROUND_UP(vb->planes[0].length,
> CIO2_PAGE_SIZE);
> - unsigned int lops = DIV_ROUND_UP(pages + 1, entries_per_page);
> + unsigned int pages = DIV_ROUND_UP(vb->planes[0].length,
> +   CIO2_PAGE_SIZE) + 1;
> + unsigned int lops = DIV_ROUND_UP(pages, entries_per_page);
>   struct sg_table *sg;
>   struct sg_page_iter sg_iter;
>   int i, j;
> @@ -869,6 +870,8 @@ static int cio2_vb2_buf_init(struct vb2_buffer *vb)
> 
>   i = j = 0;

Nit: separate assignments are preferred over multiple assignments

>   for_each_sg_page(sg->sgl, _iter, sg->nents, 0) {
> + if (!pages--)
> + break;
>   b->lop[i][j] = sg_page_iter_dma_address(_iter) >>
> PAGE_SHIFT;
>   j++;
>   if (j == entries_per_page) {
> --
> 2.7.4

RE: [RESEND PATCH 0/2] dw9714 fixes, cleanups

[Mani, Rajmohan]

Hi Sakari,

Thanks for the patches.

> -Original Message-
> From: Sakari Ailus [mailto:sakari.ai...@linux.intel.com]
> Sent: Tuesday, January 02, 2018 3:12 AM
> To: linux-media@vger.kernel.org
> Cc: Mani, Rajmohan <rajmohan.m...@intel.com>
> Subject: [RESEND PATCH 0/2] dw9714 fixes, cleanups
> 
> Hi,
> 
> (Fixed Raj's e-mail.)
> 
> The two patches address a small bug in dw9714 driver and clean it up a little,
> too.
> 
> Raj: could you let me know if they work for you? Thanks.
> 

These patches work fine.

> 
> Sakari Ailus (2):
>   dw9714: Call pm_runtime_idle() at the end of probe()
>   dw9714: Remove client field in driver's struct
> 
>  drivers/media/i2c/dw9714.c | 20 ++--
>  1 file changed, 6 insertions(+), 14 deletions(-)
> 
> --
> 2.7.4

RE: [PATCH v4 00/12] Intel IPU3 ImgU patchset

[Mani, Rajmohan]

Hi Mauro,

> -Original Message-
> From: Mauro Carvalho Chehab [mailto:mche...@s-opensource.com]
> Sent: Wednesday, December 20, 2017 5:58 AM
> To: Mani, Rajmohan <rajmohan.m...@intel.com>
> Cc: Zhi, Yong <yong@intel.com>; linux-media@vger.kernel.org;
> sakari.ai...@linux.intel.com; Zheng, Jian Xu <jian.xu.zh...@intel.com>;
> Toivonen, Tuukka <tuukka.toivo...@intel.com>; Hu, Jerry W
> <jerry.w...@intel.com>; a...@arndb.de; h...@lst.de;
> robin.mur...@arm.com; io...@lists.linux-foundation.org
> Subject: Re: [PATCH v4 00/12] Intel IPU3 ImgU patchset
> 
> Hi,
> 
> Em Fri, 17 Nov 2017 02:58:56 +
> "Mani, Rajmohan" <rajmohan.m...@intel.com> escreveu:
> 
> > Here is an update on the IPU3 documentation that we are currently working
> on.
> >
> > Image processing in IPU3 relies on the following.
> >
> > 1) HW configuration to enable ISP and
> > 2) setting customer specific 3A Tuning / Algorithm Parameters to achieve
> desired image quality.
> >
> > We intend to provide documentation on ImgU driver programming interface
> to help users of this driver to configure and enable ISP HW to meet their
> needs.  This documentation will include details on complete V4L2 Kernel driver
> interface and IO-Control parameters, except for the ISP internal algorithm and
> its parameters (which is Intel proprietary IP).
> 
> Sakari asked me to take a look on this thread, specifically on this email. I 
> took a
> look on the other e-mails from this thread that are discussing about this IP
> block.
> 
> I understand that Intel wants to keep their internal 3A algorithm protected,
> just like other vendors protect their own algos. It was never a requirement to
> open whatever algorithm are used inside a hardware (or firmware). The only
> requirement is that firmwares should be licensed with redistribution
> permission, ideally merged at linux-firmware git tree.
> 
> Yet, what I don't understand is why Intel also wants to also protect the
> interface for such 3A hardware/firmware algorithm. The parameters that are
> passed from an userspace application to Intel ISP logic doesn't contain the
> algorithm itself. What's the issue of documenting the meaning of each
> parameter?
> 

Thanks for looking into this.

To achieve improved image quality using IPU3, 3A (Auto White balance, Auto Focus
and Auto Exposure) Tuning parameters specific to a given camera sensor module,
are converted to Intel ISP algorithm parameters in user space camera HAL using
AIC (Automatic ISP Configuration) library.

As a unique design of Intel ISP, it exposes very detailed algorithm parameters
(~ 1 parameters) to configure ISP's image processing algorithm per each
image fame in runtime. Typical Camera SW developers (including those at Intel)
are not expected to fully understand and directly set these parameters to
configure the ISP algorithm blocks. Due to the above, a user space AIC library
(in binary form) is provided to generate ISP Algorithm specific parameters, for
a given set of 3A tuning parameters. It significantly reduces the efforts of SW
development in ISP HW configuration.

On the other hand, the ISP algorithm details could be deduced readily through
these detailed parameters by other ISP experts outside Intel. This is the reason
that we want to keep these parameter definitions as Intel proprietary IP.

We are fully aware of your concerns on how to enable open source developers
to use Intel ISP through up-streamed Kernel Driver. Internally, we are working
on the license for this AIC library release now (as Hans said NDA license is not
acceptable). We believe this will be more efficient way to help open source
developers.

This AIC library release would be a binary-only release. This AIC library does
not use any kernel uAPIs directly. The user space Camera HAL that uses kernel
uAPIs is available at 
https://chromium.googlesource.com/chromiumos/platform/arc-camera/+/master

Thanks
Raj

RE: [PATCH v4 00/12] Intel IPU3 ImgU patchset

[Mani, Rajmohan]

Hi Tomasz, Hans,

> Subject: Re: [PATCH v4 00/12] Intel IPU3 ImgU patchset
> 
> On 12/05/2017 04:22 AM, Tomasz Figa wrote:
> > Hi Raj,
> >
> > On Tue, Dec 5, 2017 at 9:13 AM, Mani, Rajmohan
> <rajmohan.m...@intel.com> wrote:
> >> Hi Hans,
> >>
> >> Thanks for your patience and sharing your thoughts on this.
> >>
> >>> Subject: Re: [PATCH v4 00/12] Intel IPU3 ImgU patchset
> >>>
> >>> Hi Rajmohan,
> >>>
> >>> On 11/17/2017 03:58 AM, Mani, Rajmohan wrote:
> >>>> Hi Sakari and all,
> >>>>
> >>>>> -Original Message-
> >>>>> From: Zhi, Yong
> >>>>> Sent: Tuesday, October 17, 2017 8:47 PM
> >>>>> To: linux-media@vger.kernel.org; sakari.ai...@linux.intel.com
> >>>>> Cc: Zheng, Jian Xu <jian.xu.zh...@intel.com>; Mani, Rajmohan
> >>>>> <rajmohan.m...@intel.com>; Toivonen, Tuukka
> >>>>> <tuukka.toivo...@intel.com>; Hu, Jerry W <jerry.w...@intel.com>;
> >>>>> a...@arndb.de; h...@lst.de; robin.mur...@arm.com;
> >>>>> iommu@lists.linux- foundation.org; Zhi, Yong <yong@intel.com>
> >>>>> Subject: [PATCH v4 00/12] Intel IPU3 ImgU patchset
> >>>>>
> >>>>> This patchset adds support for the Intel IPU3 (Image Processing
> >>>>> Unit) ImgU which is essentially a modern memory-to-memory ISP. It
> >>>>> implements raw Bayer to YUV image format conversion as well as a
> >>>>> large number of other pixel processing algorithms for improving
> >>>>> the image
> >>> quality.
> >>>>>
> >>>>> Meta data formats are defined for image statistics (3A, i.e.
> >>>>> automatic white balance, exposure and focus, histogram and local
> >>>>> area contrast
> >>>>> enhancement) as well as for the pixel processing algorithm parameters.
> >>>>> The documentation for these formats is currently not included in
> >>>>> the patchset but will be added in a future version of this set.
> >>>>>
> >>>>
> >>>> Here is an update on the IPU3 documentation that we are currently
> >>>> working
> >>> on.
> >>>>
> >>>> Image processing in IPU3 relies on the following.
> >>>>
> >>>> 1) HW configuration to enable ISP and
> >>>> 2) setting customer specific 3A Tuning / Algorithm Parameters to
> >>>> achieve
> >>> desired image quality.
> >>>>
> >>>> We intend to provide documentation on ImgU driver programming
> >>>> interface
> >>> to help users of this driver to configure and enable ISP HW to meet
> >>> their needs.  This documentation will include details on complete
> >>> V4L2 Kernel driver interface and IO-Control parameters, except for
> >>> the ISP internal algorithm and its parameters (which is Intel proprietary 
> >>> IP).
> >>>>
> >>>> We will also provide an user space library in binary form to help
> >>>> users of this
> >>> driver, to convert the public 3A tuning parameters to IPU3 algorithm
> >>> parameters. This tool will be released under NDA to the users of this
> driver.
> >>>
> >>> So I discussed this situation with Sakari in Prague during the ELCE.
> >>> I am not happy with adding a driver to the kernel that needs an NDA
> >>> to be usable. I can't agree to that. It's effectively the same as
> >>> firmware that's only available under an NDA and we wouldn't accept such
> drivers either.
> >>>
> >>
> >> Ack
> >>
> >>> There are a few options:
> >>>
> >>> 1) Document the ISP parameters and that format they are stored in
> >>> allowing for
> >>>open source implementations. While this is the ideal, I suspect that 
> >>> this
> is
> >>>a no-go for Intel.
> >>>
> >>
> >> Ack
> >>
> >>> 2) The driver can be used without using these ISP parameters and still
> achieve
> >>>'OK' quality. I.e., it's usable for basic webcam usage under normal 
> >>> lighting
> >>>conditions. I'm not sure if this is possible at all, though.
> >>>
> >>
> >> This is something that we have tri

RE: [PATCH v4 00/12] Intel IPU3 ImgU patchset

[Mani, Rajmohan]

Hi Hans,

Thanks for your patience and sharing your thoughts on this.

> Subject: Re: [PATCH v4 00/12] Intel IPU3 ImgU patchset
> 
> Hi Rajmohan,
> 
> On 11/17/2017 03:58 AM, Mani, Rajmohan wrote:
> > Hi Sakari and all,
> >
> >> -Original Message-
> >> From: Zhi, Yong
> >> Sent: Tuesday, October 17, 2017 8:47 PM
> >> To: linux-media@vger.kernel.org; sakari.ai...@linux.intel.com
> >> Cc: Zheng, Jian Xu <jian.xu.zh...@intel.com>; Mani, Rajmohan
> >> <rajmohan.m...@intel.com>; Toivonen, Tuukka
> >> <tuukka.toivo...@intel.com>; Hu, Jerry W <jerry.w...@intel.com>;
> >> a...@arndb.de; h...@lst.de; robin.mur...@arm.com; iommu@lists.linux-
> >> foundation.org; Zhi, Yong <yong@intel.com>
> >> Subject: [PATCH v4 00/12] Intel IPU3 ImgU patchset
> >>
> >> This patchset adds support for the Intel IPU3 (Image Processing Unit)
> >> ImgU which is essentially a modern memory-to-memory ISP. It
> >> implements raw Bayer to YUV image format conversion as well as a
> >> large number of other pixel processing algorithms for improving the image
> quality.
> >>
> >> Meta data formats are defined for image statistics (3A, i.e.
> >> automatic white balance, exposure and focus, histogram and local area
> >> contrast
> >> enhancement) as well as for the pixel processing algorithm parameters.
> >> The documentation for these formats is currently not included in the
> >> patchset but will be added in a future version of this set.
> >>
> >
> > Here is an update on the IPU3 documentation that we are currently working
> on.
> >
> > Image processing in IPU3 relies on the following.
> >
> > 1) HW configuration to enable ISP and
> > 2) setting customer specific 3A Tuning / Algorithm Parameters to achieve
> desired image quality.
> >
> > We intend to provide documentation on ImgU driver programming interface
> to help users of this driver to configure and enable ISP HW to meet their
> needs.  This documentation will include details on complete V4L2 Kernel driver
> interface and IO-Control parameters, except for the ISP internal algorithm and
> its parameters (which is Intel proprietary IP).
> >
> > We will also provide an user space library in binary form to help users of 
> > this
> driver, to convert the public 3A tuning parameters to IPU3 algorithm
> parameters. This tool will be released under NDA to the users of this driver.
> 
> So I discussed this situation with Sakari in Prague during the ELCE. I am not
> happy with adding a driver to the kernel that needs an NDA to be usable. I
> can't agree to that. It's effectively the same as firmware that's only 
> available
> under an NDA and we wouldn't accept such drivers either.
> 

Ack

> There are a few options:
> 
> 1) Document the ISP parameters and that format they are stored in allowing
> for
>open source implementations. While this is the ideal, I suspect that this 
> is
>a no-go for Intel.
> 

Ack

> 2) The driver can be used without using these ISP parameters and still achieve
>'OK' quality. I.e., it's usable for basic webcam usage under normal 
> lighting
>conditions. I'm not sure if this is possible at all, though.
> 

This is something that we have tried and are able to do image capture with
the default ISP parameters using ov5670 sensor.
Additionally we had to set optimal values for the ov5670 sensor's exposure and
gain settings.

Please see if the following image looks to meet the 'OK' quality.

git clone https://github.com/RajmohanMani/ipu3-misc.git
ipu3-misc/ov5670.jpg is the image file.

> 3) Make the library available without requiring an NDA.
> 

We are also actively exploring this option to see if this can be done.

> 4) Provide a non-NDA library (ideally open source) that achieves at minimum
>the quality as described in 2: i.e. usable for basic webcam.
> 

I see this is the same as option 3) + open sourcing the library.
Open sourcing the library does not look to be an option.
I will reconfirm this.

> 5) Other solutions I didn't think of?
> 
> I think 4 might be the best option, especially if it is open sourced and just 
> uses
> non-IP 3A algorithms. It could even be added to the v4l-utils git repo.
> 
> A closed source non-NDA library might also work, but you will need to check
> what Mauro thinks about that.

I believe this is option 3) that you are referring here.
Depending on the progress that we make on option 3), we can work on the
next steps.

> My problem is that such libraries tend to be
> abandoned after a few years and then bit-rot sets in. An open-source solution
> is much easier to maintain in the long term.
> 
> Regards,
> 
>   Hans
> 

RE: [PATCH v4 04/12] intel-ipu3: Add user space ABI definitions

[Mani, Rajmohan]

Hi Sakari,

> -Original Message-
> From: sakari.ai...@linux.intel.com [mailto:sakari.ai...@linux.intel.com]
> Sent: Tuesday, November 21, 2017 9:22 AM
> To: Mani, Rajmohan <rajmohan.m...@intel.com>
> Cc: Sakari Ailus <sakari.ai...@iki.fi>; Zhi, Yong <yong@intel.com>; linux-
> me...@vger.kernel.org; Zheng, Jian Xu <jian.xu.zh...@intel.com>; Toivonen,
> Tuukka <tuukka.toivo...@intel.com>; Hu, Jerry W <jerry.w...@intel.com>
> Subject: Re: [PATCH v4 04/12] intel-ipu3: Add user space ABI definitions
> 
> Hi Rajmohan,
> 
> My apologies for the late reply.
> 
> On Sat, Nov 11, 2017 at 04:07:22AM +, Mani, Rajmohan wrote:
> > Hi Sakari,
> >
> > > -Original Message-
> > > From: Sakari Ailus [mailto:sakari.ai...@iki.fi]
> > > Sent: Friday, October 20, 2017 2:30 AM
> > > To: Zhi, Yong <yong@intel.com>
> > > Cc: linux-media@vger.kernel.org; sakari.ai...@linux.intel.com;
> > > Zheng, Jian Xu <jian.xu.zh...@intel.com>; Mani, Rajmohan
> > > <rajmohan.m...@intel.com>; Toivonen, Tuukka
> > > <tuukka.toivo...@intel.com>; Hu, Jerry W <jerry.w...@intel.com>
> > > Subject: Re: [PATCH v4 04/12] intel-ipu3: Add user space ABI
> > > definitions
> > >
> > > Hi Yong,
> > >
> > > On Tue, Oct 17, 2017 at 10:54:49PM -0500, Yong Zhi wrote:
> 
> ...
> 
> > > > +struct ipu3_uapi_params {
> > > > +   __u32 fourcc;   /* V4L2_PIX_FMT_IPU3_PARAMS */
> > > > +   __u32 version;  /* Must be 0x100 */
> > >
> > > These were called padding1 and padding2 in the previous version.
> > > What happened?
> > >
> > > I'd just call them reserved, and maybe also make the use field the
> > > first member of the struct.
> > >
> >
> > These fields were repurposed after v3 of this patch series. Please see the 
> > user
> space code that uses these fields.
> > https://chromium.googlesource.com/chromiumos/platform/arc-
> camera/+/mas
> > ter/hal/intel/psl/ipu3/workers/IPU3AicToFwEncoder.cpp
> 
> They were fourcc and version in the beginning, and then replaced by
> padding1 and padding 2. Is there a particular reason for changing them back?
> 

We looked into this further to see that we have no compelling reasons to use 
these variables.
I will revert these names back to padding1 and padding2, along with the 
required user space code changes.

RE: [PATCH v4 00/12] Intel IPU3 ImgU patchset

[Mani, Rajmohan]

Hi Sakari and all,

> -Original Message-
> From: Zhi, Yong
> Sent: Tuesday, October 17, 2017 8:47 PM
> To: linux-media@vger.kernel.org; sakari.ai...@linux.intel.com
> Cc: Zheng, Jian Xu <jian.xu.zh...@intel.com>; Mani, Rajmohan
> <rajmohan.m...@intel.com>; Toivonen, Tuukka
> <tuukka.toivo...@intel.com>; Hu, Jerry W <jerry.w...@intel.com>;
> a...@arndb.de; h...@lst.de; robin.mur...@arm.com; iommu@lists.linux-
> foundation.org; Zhi, Yong <yong@intel.com>
> Subject: [PATCH v4 00/12] Intel IPU3 ImgU patchset
> 
> This patchset adds support for the Intel IPU3 (Image Processing Unit) ImgU
> which is essentially a modern memory-to-memory ISP. It implements raw
> Bayer to YUV image format conversion as well as a large number of other pixel
> processing algorithms for improving the image quality.
> 
> Meta data formats are defined for image statistics (3A, i.e. automatic white
> balance, exposure and focus, histogram and local area contrast
> enhancement) as well as for the pixel processing algorithm parameters.
> The documentation for these formats is currently not included in the patchset
> but will be added in a future version of this set.
> 

Here is an update on the IPU3 documentation that we are currently working on.

Image processing in IPU3 relies on the following.

1) HW configuration to enable ISP and
2) setting customer specific 3A Tuning / Algorithm Parameters to achieve 
desired image quality. 

We intend to provide documentation on ImgU driver programming interface to help 
users of this driver to configure and enable ISP HW to meet their needs.  This 
documentation will include details on complete V4L2 Kernel driver interface and 
IO-Control parameters, except for the ISP internal algorithm and its parameters 
(which is Intel proprietary IP).

We will also provide an user space library in binary form to help users of this 
driver, to convert the public 3A tuning parameters to IPU3 algorithm 
parameters. This tool will be released under NDA to the users of this driver.

> The algorithm parameters need to be considered specific to a given frame and
> typically a large number of these parameters change on frame to frame basis.
> Additionally, the parameters are highly structured (and not a flat space of
> independent configuration primitives). They also reflect the data structures
> used by the firmware and the hardware. On top of that, the algorithms require
> highly specialized user space to make meaningful use of them. For these
> reasons it has been chosen video buffers to pass the parameters to the device.
> 
> On individual patches:
> 
> The heart of ImgU is the CSS, or Camera Subsystem, which contains the image
> processors and HW accelerators.
> 
> The 3A statistics and other firmware parameter computation related functions
> are implemented in patch 8.
> 
> All h/w programming related code can be found in patch 9.
> 
> To access DDR via ImgU's own memory space, IPU3 is also equipped with its
> own MMU unit, the driver is implemented in patch 2.
> 
> Currently, the MMU driver has dependency on two exported symbols
> (iommu_group_ref_get and iommu_group_get_for_dev))to build as ko.
> 
> Patch 3 uses above IOMMU driver for DMA mem related functions.
> 
> Patch 5-10 are basically IPU3 CSS specific implementations:
> 
> 6 and 7 provide some utility functions and manage IPU3 fw download and
> install.
> 
> The firmware which is called ipu3-fw.bin can be downloaded from:
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/firmware/linux-firmware.git
> (commit 2c27b0cb02f18c022d8378e0e1abaf8b7ae8188f)
> 
> Patch 9 and 10 are of the same file, the latter implements interface functions
> for access fw & hw capabilities defined in patch 8.
> 
> Patch 11 has a dependency on Sakari's V4L2_BUF_TYPE_META_OUTPUT work:
> 
> <URL:https://patchwork.kernel.org/patch/9976293/>
> <URL:https://patchwork.kernel.org/patch/9976295/>
> 
> Patch 12 uses Kconfig and Makefile created by IPU3 cio2 patch series.
> 
> Link to user space implementation:
> 
> <URL:https://chromium.googlesource.com/chromiumos/platform/arc-
> camera/+/master>
> 
> Device topology:
> 
> ./media-ctl -d /dev/media0 -p
> Media controller API version 4.14.0
> 
> Media device information
> 
> driver  ipu3-imgu
> model   ipu3-imgu
> serial
> bus info:00:05.0
> hw revision 0x0
> driver version  4.14.0
> 
> Device topology
> - entity 1: ipu3-imgu:0 (8 pads, 8 links)
> type V4L2 subdev subtype Unknown flags 0
> device node name /dev/v4l-subdev0
>   pad0: Sink
>   [fmt:UYVY8_2X8/1920x1080 field:none colorspace:u

RE: [PATCH v4 04/12] intel-ipu3: Add user space ABI definitions

[Mani, Rajmohan]

Hi Sakari,

> -Original Message-
> From: Sakari Ailus [mailto:sakari.ai...@iki.fi]
> Sent: Friday, October 20, 2017 2:30 AM
> To: Zhi, Yong <yong@intel.com>
> Cc: linux-media@vger.kernel.org; sakari.ai...@linux.intel.com; Zheng, Jian Xu
> <jian.xu.zh...@intel.com>; Mani, Rajmohan <rajmohan.m...@intel.com>;
> Toivonen, Tuukka <tuukka.toivo...@intel.com>; Hu, Jerry W
> <jerry.w...@intel.com>
> Subject: Re: [PATCH v4 04/12] intel-ipu3: Add user space ABI definitions
> 
> Hi Yong,
> 
> On Tue, Oct 17, 2017 at 10:54:49PM -0500, Yong Zhi wrote:
> > The UAPI header defines the structures and macros
> > to be used by user space.
> >
> > Signed-off-by: Rajmohan Mani <rajmohan.m...@intel.com>
> > Signed-off-by: Yong Zhi <yong@intel.com>
> > ---
> >  include/uapi/linux/intel-ipu3.h | 2199
> +++
> >  1 file changed, 2199 insertions(+)
> >  create mode 100644 include/uapi/linux/intel-ipu3.h
> >
> > diff --git a/include/uapi/linux/intel-ipu3.h 
> > b/include/uapi/linux/intel-ipu3.h
> > new file mode 100644
> > index ..e27a449b4ec1
> > --- /dev/null
> > +++ b/include/uapi/linux/intel-ipu3.h
> > @@ -0,0 +1,2199 @@
> > +/*
> > + * 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.
> > + */
> > +
> > +#ifndef __IPU3_UAPI_H
> > +#define __IPU3_UAPI_H
> > +
> > +#include 
> > +
> > +#define IPU3_UAPI_ISP_VEC_ELEMS64
> > +
> > +#define IMGU_ABI_PAD   __aligned(IPU3_UAPI_ISP_WORD_BYTES)
> > +#define IPU3_ALIGN
>   __attribute__((aligned(IPU3_UAPI_ISP_WORD_BYTES)))
> > +
> > +#define IPU3_UAPI_ISP_WORD_BYTES   32
> > +#define IPU3_UAPI_MAX_STRIPES  2
> > +
> > +/*** ipu3_uapi_stats_3a ***/
> > +
> > +#define IPU3_UAPI_MAX_BUBBLE_SIZE  10
> > +
> > +#define IPU3_UAPI_AE_COLORS4
> > +#define IPU3_UAPI_AE_BINS  256
> > +
> > +#define IPU3_UAPI_AWB_MD_ITEM_SIZE 8
> > +#define IPU3_UAPI_AWB_MAX_SETS 60
> > +#define IPU3_UAPI_AWB_SET_SIZE 0x500
> > +#define IPU3_UAPI_AWB_SPARE_FOR_BUBBLES \
> > +   (IPU3_UAPI_MAX_BUBBLE_SIZE * IPU3_UAPI_MAX_STRIPES * \
> > +IPU3_UAPI_AWB_MD_ITEM_SIZE)
> > +#define IPU3_UAPI_AWB_MAX_BUFFER_SIZE \
> > +   (IPU3_UAPI_AWB_MAX_SETS * \
> > +(IPU3_UAPI_AWB_SET_SIZE +
> IPU3_UAPI_AWB_SPARE_FOR_BUBBLES))
> > +
> > +#define IPU3_UAPI_AF_MAX_SETS  24
> > +#define IPU3_UAPI_AF_MD_ITEM_SIZE  4
> > +#define IPU3_UAPI_AF_SPARE_FOR_BUBBLES \
> > +   (IPU3_UAPI_MAX_BUBBLE_SIZE * IPU3_UAPI_MAX_STRIPES * \
> > +IPU3_UAPI_AF_MD_ITEM_SIZE)
> > +#define IPU3_UAPI_AF_Y_TABLE_SET_SIZE  0x80
> > +#define IPU3_UAPI_AF_Y_TABLE_MAX_SIZE \
> > +   (IPU3_UAPI_AF_MAX_SETS * \
> > +(IPU3_UAPI_AF_Y_TABLE_SET_SIZE +
> IPU3_UAPI_AF_SPARE_FOR_BUBBLES) * \
> > +IPU3_UAPI_MAX_STRIPES)
> > +
> > +#define IPU3_UAPI_AWB_FR_MAX_SETS  24
> > +#define IPU3_UAPI_AWB_FR_MD_ITEM_SIZE  8
> > +#define IPU3_UAPI_AWB_FR_BAYER_TBL_SIZE0x100
> > +#define IPU3_UAPI_AWB_FR_SPARE_FOR_BUBBLES \
> > +   (IPU3_UAPI_MAX_BUBBLE_SIZE * IPU3_UAPI_MAX_STRIPES * \
> > +   IPU3_UAPI_AWB_FR_MD_ITEM_SIZE)
> > +#define IPU3_UAPI_AWB_FR_BAYER_TABLE_MAX_SIZE \
> > +   (IPU3_UAPI_AWB_FR_MAX_SETS * \
> > +   (IPU3_UAPI_AWB_FR_BAYER_TBL_SIZE + \
> > +IPU3_UAPI_AWB_FR_SPARE_FOR_BUBBLES) * \
> > +   IPU3_UAPI_MAX_STRIPES)
> > +
> > +struct ipu3_uapi_grid_config {
> > +   __u8 width; /* 6 or 7 (rgbs_grd_cfg) bits */
> > +   __u8 height;
> > +   __u16 block_width_log2:3;
> > +   __u16 block_height_log2:3;
> > +   __u16 height_per_slice:8; 

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

[Mani, Rajmohan]

Hi Sakari,

> -Original Message-
> From: Sakari Ailus [mailto:sakari.ai...@linux.intel.com]
> Sent: Monday, November 06, 2017 3:38 PM
> To: Mani, Rajmohan <rajmohan.m...@intel.com>
> Cc: Zhi, Yong <yong@intel.com>; linux-media@vger.kernel.org; Zheng, Jian
> Xu <jian.xu.zh...@intel.com>; tf...@chromium.org; Toivonen, Tuukka
> <tuukka.toivo...@intel.com>; Yang, Hyungwoo
> <hyungwoo.y...@intel.com>; Rapolu, Chiranjeevi
> <chiranjeevi.rap...@intel.com>; Hu, Jerry W <jerry.w...@intel.com>;
> Vijaykumar, Ramya <ramya.vijayku...@intel.com>
> Subject: Re: [PATCH v7 3/3] intel-ipu3: cio2: Add new MIPI-CSI2 driver
> 
> Hi Rajmohan,
> 
> On Mon, Nov 06, 2017 at 07:32:52PM +, Mani, Rajmohan wrote:
> > Hi Sakari, Yong,
> >
> > > Subject: Re: [PATCH v7 3/3] intel-ipu3: cio2: Add new MIPI-CSI2
> > > driver
> > >
> > > Hi Yong,
> > >
> > > Thanks for the update!
> > >
> > > I took a final glance, there are a few matters that still need to be
> > > addressed but then I think we're done. Please see below.
> > >
> > > Then we'll need an entry in the MAINTAINERS file in the kernel root
> directory.
> > > Could you add an entry for this driver in v8?
> > >
> > > On Thu, Nov 02, 2017 at 03:00:01PM -0500, Yong Zhi wrote:
> > > ...
> > > > +static int cio2_fbpt_rearrange(struct cio2_device *cio2, struct
> > > > +cio2_queue *q) {
> > > > +   unsigned int i, j;
> > > > +
> > > > +   for (i = 0, j = q->bufs_first; i < CIO2_MAX_BUFFERS;
> > > > +   i++, j = (j + 1) % CIO2_MAX_BUFFERS)
> > > > +   if (q->bufs[j])
> > > > +   break;
> > > > +
> > > > +   if (i == CIO2_MAX_BUFFERS)
> > > > +   return 0;
> > >
> > > You always return 0. The return type could be void.
> > >
> > > You could also move this function just above the function using it
> > > (it's a single location).
> > >
> > > > +
> > > > +   if (j) {
> > > > +   arrange(q->fbpt, sizeof(struct cio2_fbpt_entry) *
> > > CIO2_MAX_LOPS,
> > > > +   CIO2_MAX_BUFFERS, j);
> > > > +   arrange(q->bufs, sizeof(struct cio2_buffer *),
> > > > +   CIO2_MAX_BUFFERS, j);
> > > > +   }
> > > > +
> > > > +   /*
> > > > +* DMA clears the valid bit when accessing the buffer.
> > > > +* When stopping stream in suspend callback, some of the buffers
> > > > +* may be in invalid state. After resume, when DMA meets the 
> > > > invalid
> > > > +* buffer, it will halt and stop receiving new data.
> > > > +* To avoid DMA halting, set the valid bit for all buffers in 
> > > > FBPT.
> > > > +*/
> > > > +   for (i = 0; i < CIO2_MAX_BUFFERS; i++)
> > > > +   cio2_fbpt_entry_enable(cio2, q->fbpt + i * 
> > > > CIO2_MAX_LOPS);
> > > > +
> > > > +   return 0;
> > > > +}
> > >
> > > ...
> > >
> > > > +static int cio2_vb2_start_streaming(struct vb2_queue *vq,
> > > > +unsigned int count) {
> > > > +   struct cio2_queue *q = vb2q_to_cio2_queue(vq);
> > > > +   struct cio2_device *cio2 = vb2_get_drv_priv(vq);
> > > > +   int r;
> > > > +
> > > > +   cio2->cur_queue = q;
> > > > +   atomic_set(>frame_sequence, 0);
> > > > +
> > > > +   r = pm_runtime_get_sync(>pci_dev->dev);
> > > > +   if (r < 0) {
> > > > +   dev_info(>pci_dev->dev, "failed to set power 
> > > > %d\n", r);
> > > > +   pm_runtime_put(>pci_dev->dev);
> >
> > Shouldn't we use pm_runtime_put_noidle() here, so the usage_count gets
> decremented?
> 
> Yeah, pm_runtime_put_noidle() would be perhaps more appropriate. The
> difference is that the noidle variant won't suspend the device (but it was
> already suspended to begin with as resuming failed).
> 

Thanks for confirming.

> >
> > > > +   return r;
> > > > +   }
> > > > +
> > > > +   r = media_pipeline_start(>vdev.entity, >pipe);
> &g

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

[Mani, Rajmohan]

Hi Sakari, Yong,

> Subject: Re: [PATCH v7 3/3] intel-ipu3: cio2: Add new MIPI-CSI2 driver
> 
> Hi Yong,
> 
> Thanks for the update!
> 
> I took a final glance, there are a few matters that still need to be 
> addressed but
> then I think we're done. Please see below.
> 
> Then we'll need an entry in the MAINTAINERS file in the kernel root directory.
> Could you add an entry for this driver in v8?
> 
> On Thu, Nov 02, 2017 at 03:00:01PM -0500, Yong Zhi wrote:
> ...
> > +static int cio2_fbpt_rearrange(struct cio2_device *cio2, struct
> > +cio2_queue *q) {
> > +   unsigned int i, j;
> > +
> > +   for (i = 0, j = q->bufs_first; i < CIO2_MAX_BUFFERS;
> > +   i++, j = (j + 1) % CIO2_MAX_BUFFERS)
> > +   if (q->bufs[j])
> > +   break;
> > +
> > +   if (i == CIO2_MAX_BUFFERS)
> > +   return 0;
> 
> You always return 0. The return type could be void.
> 
> You could also move this function just above the function using it (it's a 
> single
> location).
> 
> > +
> > +   if (j) {
> > +   arrange(q->fbpt, sizeof(struct cio2_fbpt_entry) *
> CIO2_MAX_LOPS,
> > +   CIO2_MAX_BUFFERS, j);
> > +   arrange(q->bufs, sizeof(struct cio2_buffer *),
> > +   CIO2_MAX_BUFFERS, j);
> > +   }
> > +
> > +   /*
> > +* DMA clears the valid bit when accessing the buffer.
> > +* When stopping stream in suspend callback, some of the buffers
> > +* may be in invalid state. After resume, when DMA meets the invalid
> > +* buffer, it will halt and stop receiving new data.
> > +* To avoid DMA halting, set the valid bit for all buffers in FBPT.
> > +*/
> > +   for (i = 0; i < CIO2_MAX_BUFFERS; i++)
> > +   cio2_fbpt_entry_enable(cio2, q->fbpt + i * CIO2_MAX_LOPS);
> > +
> > +   return 0;
> > +}
> 
> ...
> 
> > +static int cio2_vb2_start_streaming(struct vb2_queue *vq, unsigned
> > +int count) {
> > +   struct cio2_queue *q = vb2q_to_cio2_queue(vq);
> > +   struct cio2_device *cio2 = vb2_get_drv_priv(vq);
> > +   int r;
> > +
> > +   cio2->cur_queue = q;
> > +   atomic_set(>frame_sequence, 0);
> > +
> > +   r = pm_runtime_get_sync(>pci_dev->dev);
> > +   if (r < 0) {
> > +   dev_info(>pci_dev->dev, "failed to set power %d\n", r);
> > +   pm_runtime_put(>pci_dev->dev);

Shouldn't we use pm_runtime_put_noidle() here, so the usage_count gets 
decremented?

> > +   return r;
> > +   }
> > +
> > +   r = media_pipeline_start(>vdev.entity, >pipe);
> > +   if (r)
> > +   goto fail_pipeline;
> > +
> > +   r = cio2_hw_init(cio2, q);
> > +   if (r)
> > +   goto fail_hw;
> > +
> > +   /* Start streaming on sensor */
> > +   r = v4l2_subdev_call(q->sensor, video, s_stream, 1);
> > +   if (r)
> > +   goto fail_csi2_subdev;
> > +
> > +   cio2->streaming = true;
> > +
> > +   return 0;
> > +
> > +fail_csi2_subdev:
> > +   cio2_hw_exit(cio2, q);
> > +fail_hw:
> > +   media_pipeline_stop(>vdev.entity);
> > +fail_pipeline:
> > +   dev_dbg(>pci_dev->dev, "failed to start streaming (%d)\n", r);
> > +   cio2_vb2_return_all_buffers(q);
> 
> I believe there should be
> 
>   pm_runtime_put(>pci_dev->dev);
> 
> here. You should also add a label and use goto from where you do
> pm_runtime_put() in error handling in this function in order to make this
> cleaner.
> 
> > +
> > +   return r;
> > +}
> 
> --
> Kind regards,
> 
> Sakari Ailus
> sakari.ai...@linux.intel.com

RE: [PATCH] [media] dw9714: Set the v4l2 focus ctrl step as 1

[Mani, Rajmohan]

Hi Sakari,

Thanks for the review.

> Subject: Re: [PATCH] [media] dw9714: Set the v4l2 focus ctrl step as 1
> 
> Hi Rajmohan,
> 
> On Wed, Aug 30, 2017 at 10:48:52AM -0700, Rajmohan Mani wrote:
> > Current v4l2 focus ctrl step value of 16, limits the minimum
> > granularity of focus positions to 16.
> > Setting this value as 1, enables more accurate focus positions.
> 
> Thanks for the patch.
> 
> The recommended limit for line length is 75, not 50 (or 25 or whatever) as it
> might be in certain Gerrit installations. :-) Please make good use of lines 
> in the
> future, I've rewrapped the text this time. Thanks.
> 

Ack. I have been overly cautious here.

RE: [GIT PULL] linux-firmware: intel: Add Kabylake IPU3 firmware

[Mani, Rajmohan]

Hi Kyle, Tomasz,

Thanks for following up on this.

It's now available in the master branch.
https://git.kernel.org/pub/scm/linux/kernel/git/firmware/linux-firmware.git/commit/?id=cb0ddd68413981694d94aab8446418f2e25423a8

Raj

> -Original Message-
> From: Kyle McMartin [mailto:k...@infradead.org]
> Sent: Wednesday, August 23, 2017 9:48 AM
> To: Mani, Rajmohan <rajmohan.m...@intel.com>
> Cc: linux-media@vger.kernel.org; sakari.ai...@linux.intel.com; Zheng, Jian Xu
> <jian.xu.zh...@intel.com>; tf...@chromium.org; Toivonen, Tuukka
> <tuukka.toivo...@intel.com>; Zhi, Yong <yong@intel.com>; linux-
> firmw...@kernel.org
> Subject: Re: [GIT PULL] linux-firmware: intel: Add Kabylake IPU3 firmware
> 
> On Sat, Aug 05, 2017 at 12:04:26AM +, Mani, Rajmohan wrote:
> > Hi,
> >
> > Please review this PULL request to add Kabylake IPU3 firmware to the linux-
> firmware repository.
> >
> >
> > The following changes since commit
> 7d2c913dcd1be083350d97a8cb1eba24cfacbc8a:
> >
> >   ath10k: update year in license (2017-06-22 12:06:02 -0700)
> >
> > are available in the git repository at:
> >
> >   https://github.com/RajmohanMani/linux-firmware.git tags/v1
> >
> 
> pulled, sorry for the delay, I was on vacation (and there's ostensibly two 
> other
> people...)
> 
> --Kyle

RE: [PATCH 0/3] DW9714 DT support

[Mani, Rajmohan]

Hi Sakari,

Thanks for the patches.

I have verified that the dw9714 driver gets probed via DT bindings, with these 
patches on 4.13-rc6 kernel.

Feel free to add

Tested-by: Rajmohan Mani <rajmohan.m...@intel.com>

Raj

> -Original Message-
> From: Sakari Ailus [mailto:sakari.ai...@linux.intel.com]
> Sent: Thursday, August 17, 2017 6:43 AM
> To: linux-media@vger.kernel.org
> Cc: devicet...@vger.kernel.org; Mani, Rajmohan
> <rajmohan.m...@intel.com>
> Subject: [PATCH 0/3] DW9714 DT support
> 
> Hi all,
> 
> This patchset adds DT bindings as well as DT support for DW9714. The unused
> ACPI match table is removed.
> 
> Sakari Ailus (3):
>   dt-bindings: Add bindings for Dongwoon DW9714 voice coil
>   dw9714: Add Devicetree support
>   dw9714: Remove ACPI match tables, convert to use probe_new
> 
>  .../bindings/media/i2c/dongwoon,dw9714.txt |  9 
>  .../devicetree/bindings/vendor-prefixes.txt|  1 +
>  drivers/media/i2c/dw9714.c | 26 
> +-
>  3 files changed, 21 insertions(+), 15 deletions(-)  create mode 100644
> Documentation/devicetree/bindings/media/i2c/dongwoon,dw9714.txt
> 
> --
> 2.7.4

[GIT PULL] linux-firmware: intel: Add Kabylake IPU3 firmware

[Mani, Rajmohan]

Hi,

Please review this PULL request to add Kabylake IPU3 firmware to the 
linux-firmware repository.


The following changes since commit 7d2c913dcd1be083350d97a8cb1eba24cfacbc8a:

  ath10k: update year in license (2017-06-22 12:06:02 -0700)

are available in the git repository at:

  https://github.com/RajmohanMani/linux-firmware.git tags/v1

for you to fetch changes up to 2c27b0cb02f18c022d8378e0e1abaf8b7ae8188f:

  linux-firmware: intel: Add Kabylake IPU3 firmware (2017-08-04 15:53:13 -0700)


IPU3 firmware version irci_irci_ecr-master_20161208_0213_20170112_1500


Rajmohan Mani (1):
  linux-firmware: intel: Add Kabylake IPU3 firmware

 LICENSE.ipu3_firmware  |  36 

 WHENCE |  11 +++
 intel/ipu3-fw.bin  |   1 +
 intel/irci_irci_ecr-master_20161208_0213_20170112_1500.bin | Bin 0 -> 1212984 
bytes
 4 files changed, 48 insertions(+)
 create mode 100644 LICENSE.ipu3_firmware
 create mode 12 intel/ipu3-fw.bin
 create mode 100644 intel/irci_irci_ecr-master_20161208_0213_20170112_1500.bin


Thanks
Raj

RE: [PATCH v4 1/1] i2c: Add Omnivision OV5670 5M sensor support

[Mani, Rajmohan]

Hi Chiran,

> +
> +/* Supported link frequencies */
> +#define OV5670_LINK_FREQ_840MBPS 84000
> +#define OV5670_LINK_FREQ_840MBPS_INDEX   0
> +static const struct ov5670_link_freq_config link_freq_configs[] = {
> + {
> + .pixel_rate = 33600,
> + .reg_list = {
> + .num_of_regs =
> ARRAY_SIZE(mipi_data_rate_840mbps),
> + .regs = mipi_data_rate_840mbps,
> + }
> + }
> +};
> +
> +static const const s64 link_freq_menu_items[] = {

 ^ duplicated const declaration

> + OV5670_LINK_FREQ_840MBPS
> +};
> +

RE: [PATCH v8] dw9714: Initial driver for dw9714 VCM

[Mani, Rajmohan]

Hi Sakari,

> >
> > On Sat, Jun 03, 2017 at 01:11:40AM -0700, Rajmohan Mani wrote:
> > > DW9714 is a 10 bit DAC, designed for linear control of voice coil
> > > motor.
> > >
> > > This driver creates a V4L2 subdevice and provides control to set the
> > > desired focus.
> > >
> > > Signed-off-by: Rajmohan Mani 
> >
> > Acked-by: Sakari Ailus 
> 
> Applied to my tree.
> 

Great. Thanks for the update.

RE: [PATCH v5] dw9714: Initial driver for dw9714 VCM

[Mani, Rajmohan]

Hi Sakari,

> -Original Message-
> From: Sakari Ailus [mailto:sakari.ai...@iki.fi]
> Sent: Friday, May 26, 2017 1:20 AM
> To: Mani, Rajmohan <rajmohan.m...@intel.com>
> Cc: linux-media@vger.kernel.org; mche...@kernel.org; hverk...@xs4all.nl;
> tf...@chromium.org; s.nawro...@samsung.com; Toivonen, Tuukka
> <tuukka.toivo...@intel.com>
> Subject: Re: [PATCH v5] dw9714: Initial driver for dw9714 VCM
> 
> Hi Rajmohan,
> 
> Thankd for the update. A few more comments I missed earlier.
> 
> On Thu, May 25, 2017 at 06:50:35PM -0700, Rajmohan Mani wrote:
> > DW9714 is a 10 bit DAC, designed for linear control of voice coil
> > motor.
> >
> > This driver creates a V4L2 subdevice and provides control to set the
> > desired focus.
> >
> > Signed-off-by: Rajmohan Mani <rajmohan.m...@intel.com>
> > ---
> > Changes in v5:
> > - Addressed review comments from Tomasz, Sakari and Sylwester on v4
> > of this patch
> > Changes in v4:
> > - Addressed review comments from Tomasz Changes in v3:
> > - Addressed most of the review comments from Sakari
> >   on v1 of this patch
> > Changes in v2:
> > - Addressed review comments from Hans Verkuil
> > - Fixed a debug message typo
> > - Got rid of a return variable
> > ---
> >  drivers/media/i2c/Kconfig  |   9 ++
> >  drivers/media/i2c/Makefile |   1 +
> >  drivers/media/i2c/dw9714.c | 292
> > +
> >  3 files changed, 302 insertions(+)
> >  create mode 100644 drivers/media/i2c/dw9714.c
> >
> > diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> > index fd181c9..516e2f2 100644
> > --- a/drivers/media/i2c/Kconfig
> > +++ b/drivers/media/i2c/Kconfig
> > @@ -300,6 +300,15 @@ config VIDEO_AD5820
> >   This is a driver for the AD5820 camera lens voice coil.
> >   It is used for example in Nokia N900 (RX-51).
> >
> > +config VIDEO_DW9714
> > +   tristate "DW9714 lens voice coil support"
> > +   depends on I2C && VIDEO_V4L2 && MEDIA_CONTROLLER &&
> > +VIDEO_V4L2_SUBDEV_API
> 
> This would be better split on two lines.
> 

Ack

> > +   ---help---
> > + This is a driver for the DW9714 camera lens voice coil.
> > + DW9714 is a 10 bit DAC with 120mA output current sink
> > + capability. This is designed for linear control of
> > + voice coil motors, controlled via I2C serial interface.
> > +
> >  config VIDEO_SAA7110
> > tristate "Philips SAA7110 video decoder"
> > depends on VIDEO_V4L2 && I2C
> > diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
> > index 62323ec..987bd1f 100644
> > --- a/drivers/media/i2c/Makefile
> > +++ b/drivers/media/i2c/Makefile
> > @@ -21,6 +21,7 @@ obj-$(CONFIG_VIDEO_SAA7127) += saa7127.o
> >  obj-$(CONFIG_VIDEO_SAA7185) += saa7185.o
> >  obj-$(CONFIG_VIDEO_SAA6752HS) += saa6752hs.o
> >  obj-$(CONFIG_VIDEO_AD5820)  += ad5820.o
> > +obj-$(CONFIG_VIDEO_DW9714)  += dw9714.o
> >  obj-$(CONFIG_VIDEO_ADV7170) += adv7170.o
> >  obj-$(CONFIG_VIDEO_ADV7175) += adv7175.o
> >  obj-$(CONFIG_VIDEO_ADV7180) += adv7180.o diff --git
> > a/drivers/media/i2c/dw9714.c b/drivers/media/i2c/dw9714.c new file
> > mode 100644 index 000..22c84de
> > --- /dev/null
> > +++ b/drivers/media/i2c/dw9714.c
> > @@ -0,0 +1,292 @@
> > +/*
> > + * Copyright (c) 2015--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.
> > + */
> > +
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +
> > +#define DW9714_NAME"dw9714"
> > +#define DW9714_MAX_FOCUS_POS   1023
> > +/*
> > + * This acts as the minimum granularity of lens movement.
> > + * Keep this value power of 2, so the control steps can be
> > + * uniformly adjusted for gradual lens movement, with desired
> > + * number of control steps.
> > + */
> > +#defi

RE: [PATCH] dw9714: Initial driver for dw9714 VCM

[Mani, Rajmohan]

Hi All,
Please follow v5 of this patch for the latest and ignore this patch.
Sorry for the noise.

Raj

> -Original Message-
> From: Mani, Rajmohan
> Sent: Thursday, May 25, 2017 6:50 PM
> To: linux-media@vger.kernel.org
> Cc: mche...@kernel.org; hverk...@xs4all.nl; tf...@chromium.org;
> sakari.ai...@iki.fi; s.nawro...@samsung.com; Toivonen, Tuukka
> <tuukka.toivo...@intel.com>; Mani, Rajmohan <rajmohan.m...@intel.com>
> Subject: [PATCH] dw9714: Initial driver for dw9714 VCM
> 
> DW9714 is a 10 bit DAC, designed for linear control of voice coil motor.
> 
> This driver creates a V4L2 subdevice and provides control to set the desired
> focus.
> 
> Signed-off-by: Rajmohan Mani <rajmohan.m...@intel.com>
> ---
>  drivers/media/i2c/Kconfig  |   9 ++
>  drivers/media/i2c/Makefile |   1 +
>  drivers/media/i2c/dw9714.c | 292
> +
>  3 files changed, 302 insertions(+)
>  create mode 100644 drivers/media/i2c/dw9714.c
> 
> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig index
> fd181c9..516e2f2 100644
> --- a/drivers/media/i2c/Kconfig
> +++ b/drivers/media/i2c/Kconfig
> @@ -300,6 +300,15 @@ config VIDEO_AD5820
> This is a driver for the AD5820 camera lens voice coil.
> It is used for example in Nokia N900 (RX-51).
> 
> +config VIDEO_DW9714
> + tristate "DW9714 lens voice coil support"
> + depends on I2C && VIDEO_V4L2 && MEDIA_CONTROLLER &&
> VIDEO_V4L2_SUBDEV_API
> + ---help---
> +   This is a driver for the DW9714 camera lens voice coil.
> +   DW9714 is a 10 bit DAC with 120mA output current sink
> +   capability. This is designed for linear control of
> +   voice coil motors, controlled via I2C serial interface.
> +
>  config VIDEO_SAA7110
>   tristate "Philips SAA7110 video decoder"
>   depends on VIDEO_V4L2 && I2C
> diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile index
> 62323ec..987bd1f 100644
> --- a/drivers/media/i2c/Makefile
> +++ b/drivers/media/i2c/Makefile
> @@ -21,6 +21,7 @@ obj-$(CONFIG_VIDEO_SAA7127) += saa7127.o
>  obj-$(CONFIG_VIDEO_SAA7185) += saa7185.o
>  obj-$(CONFIG_VIDEO_SAA6752HS) += saa6752hs.o
>  obj-$(CONFIG_VIDEO_AD5820)  += ad5820.o
> +obj-$(CONFIG_VIDEO_DW9714)  += dw9714.o
>  obj-$(CONFIG_VIDEO_ADV7170) += adv7170.o
>  obj-$(CONFIG_VIDEO_ADV7175) += adv7175.o
>  obj-$(CONFIG_VIDEO_ADV7180) += adv7180.o diff --git
> a/drivers/media/i2c/dw9714.c b/drivers/media/i2c/dw9714.c new file mode
> 100644 index 000..22c84de
> --- /dev/null
> +++ b/drivers/media/i2c/dw9714.c
> @@ -0,0 +1,292 @@
> +/*
> + * Copyright (c) 2015--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.
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#define DW9714_NAME  "dw9714"
> +#define DW9714_MAX_FOCUS_POS 1023
> +/*
> + * This acts as the minimum granularity of lens movement.
> + * Keep this value power of 2, so the control steps can be
> + * uniformly adjusted for gradual lens movement, with desired
> + * number of control steps.
> + */
> +#define DW9714_CTRL_STEPS16
> +#define DW9714_CTRL_DELAY_US 1000
> +/*
> + * S[3:2] = 0x00, codes per step for "Linear Slope Control"
> + * S[1:0] = 0x00, step period
> + */
> +#define DW9714_DEFAULT_S 0x0
> +#define DW9714_VAL(data, s) ((data) << 4 | (s))
> +
> +/* dw9714 device structure */
> +struct dw9714_device {
> + struct i2c_client *client;
> + struct v4l2_ctrl_handler ctrls_vcm;
> + struct v4l2_subdev sd;
> + u16 current_val;
> +};
> +
> +static inline struct dw9714_device *to_dw9714_vcm(struct v4l2_ctrl
> +*ctrl) {
> + return container_of(ctrl->handler, struct dw9714_device, ctrls_vcm); }
> +
> +static inline struct dw9714_device *sd_to_dw9714_vcm(struct v4l2_subdev
> +*subdev) {
> + return container_of(subdev, struct dw9714_device, sd); }
> +
> +static int dw9714_i2c_write(struct i2c_client *client, u16 data) {
> + int ret;
> + u16 val = cpu_to_be16(data);
> +
> + ret = i2c_master_send(client, (const char *), sizeof(val));
> + i

RE: [PATCH v4] dw9714: Initial driver for dw9714 VCM

[Mani, Rajmohan]

> > > +   if (ret)
> > > +   dev_err(dev, "%s I2C failure: %d", __func__,
> > > + ret);
> >
> > I think we should just return an error code here and fail the suspend.
> 
> The result from an error here is that the user would hear an audible click.
> I don't think it's worth failing system suspend. :-)
> 
> But as no action is taken based on the error code, there could be quite a few 
> of
> these messages. How about dev_err_once()? For resume I might use
> dev_err_ratelimited().
> 

Ack (addressed with v5 of this patch)

RE: [PATCH v4] dw9714: Initial driver for dw9714 VCM

[Mani, Rajmohan]

Hi Sakari, Sylwester,

> >
> > You're right, sorry. I'd expect such things to be better covered in
> > the API documentation.  Probably pm_runtime_put_noidle() is a better
> 
> Well, the documentation tells what the function does. It'd be good if it 
> pointed
> that the usage count needs to be decremented if the function fails.
> 
> I guess the reason is that it's just a synchronous variant of 
> pm_runtime_get(),
> which could not handle the error anyway.
> 
> > match for just decreasing usage_count.  Now many drivers appear to not
> > be balancing usage_count when when pm_runtime_get_sync() fails.
> 
> Ah, quite a few drivers seem to be using pm_runtime_put_noidle() which seems
> to be the correct thing to do as the device won't be on then anyway.
> 

Ack

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

RE: [PATCH v4] dw9714: Initial driver for dw9714 VCM

[Mani, Rajmohan]

Hi Sakari,

> -Original Message-
> From: Sakari Ailus [mailto:sakari.ai...@iki.fi]
> Sent: Thursday, May 11, 2017 12:41 AM
> To: Mani, Rajmohan <rajmohan.m...@intel.com>
> Cc: linux-media@vger.kernel.org; mche...@kernel.org; hverk...@xs4all.nl;
> tf...@chromium.org
> Subject: Re: [PATCH v4] dw9714: Initial driver for dw9714 VCM
> 
> Hi Rajmohan,
> 
> A few minor comments below.
> 
> On Wed, May 10, 2017 at 10:00:20PM -0700, Rajmohan Mani wrote:
> > DW9714 is a 10 bit DAC, designed for linear control of voice coil
> > motor.
> >
> > This driver creates a V4L2 subdevice and provides control to set the
> > desired focus.
> >
> > Signed-off-by: Rajmohan Mani <rajmohan.m...@intel.com>
> > ---
> > Changes in v4:
> > - Addressed review comments from Tomasz Changes in v3:
> > - Addressed most of the review comments from Sakari
> >   on v1 of this patch
> > Changes in v2:
> > - Addressed review comments from Hans Verkuil
> > - Fixed a debug message typo
> > - Got rid of a return variable
> > ---
> >  drivers/media/i2c/Kconfig  |   9 ++
> >  drivers/media/i2c/Makefile |   1 +
> >  drivers/media/i2c/dw9714.c | 332
> > +
> >  3 files changed, 342 insertions(+)
> >  create mode 100644 drivers/media/i2c/dw9714.c
> >
> > diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> > index fd181c9..516e2f2 100644
> > --- a/drivers/media/i2c/Kconfig
> > +++ b/drivers/media/i2c/Kconfig
> > @@ -300,6 +300,15 @@ config VIDEO_AD5820
> >   This is a driver for the AD5820 camera lens voice coil.
> >   It is used for example in Nokia N900 (RX-51).
> >
> > +config VIDEO_DW9714
> > +   tristate "DW9714 lens voice coil support"
> > +   depends on I2C && VIDEO_V4L2 && MEDIA_CONTROLLER &&
> VIDEO_V4L2_SUBDEV_API
> > +   ---help---
> > + This is a driver for the DW9714 camera lens voice coil.
> > + DW9714 is a 10 bit DAC with 120mA output current sink
> > + capability. This is designed for linear control of
> > + voice coil motors, controlled via I2C serial interface.
> > +
> >  config VIDEO_SAA7110
> > tristate "Philips SAA7110 video decoder"
> > depends on VIDEO_V4L2 && I2C
> > diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
> > index 62323ec..987bd1f 100644
> > --- a/drivers/media/i2c/Makefile
> > +++ b/drivers/media/i2c/Makefile
> > @@ -21,6 +21,7 @@ obj-$(CONFIG_VIDEO_SAA7127) += saa7127.o
> >  obj-$(CONFIG_VIDEO_SAA7185) += saa7185.o
> >  obj-$(CONFIG_VIDEO_SAA6752HS) += saa6752hs.o
> >  obj-$(CONFIG_VIDEO_AD5820)  += ad5820.o
> > +obj-$(CONFIG_VIDEO_DW9714)  += dw9714.o
> >  obj-$(CONFIG_VIDEO_ADV7170) += adv7170.o
> >  obj-$(CONFIG_VIDEO_ADV7175) += adv7175.o
> >  obj-$(CONFIG_VIDEO_ADV7180) += adv7180.o diff --git
> > a/drivers/media/i2c/dw9714.c b/drivers/media/i2c/dw9714.c new file
> > mode 100644 index 000..fefd5d2
> > --- /dev/null
> > +++ b/drivers/media/i2c/dw9714.c
> > @@ -0,0 +1,332 @@
> > +/*
> > + * Copyright (c) 2015--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.
> > + */
> > +
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +
> > +#define DW9714_NAME"dw9714"
> > +#define DW9714_MAX_FOCUS_POS   1023
> > +/*
> > + * This acts as the minimum granularity of lens movement.
> > + * Keep this value power of 2, so the control steps can be
> > + * uniformly adjusted for gradual lens movement, with desired
> > + * number of control steps.
> > + */
> > +#define DW9714_CTRL_STEPS  16
> > +#define DW9714_CTRL_DELAY_US   1000
> > +/*
> > + * S[3:2] = 0x00, codes per step for "Linear Slope Control"
> > + * S[1:0] = 0x00, step period
> > + */
> > +#define DW9714_DEFAULT_S 0x0
> > +#define DW9714_VAL(data, s) (u16)((data) <<

RE: [PATCH v4] dw9714: Initial driver for dw9714 VCM

[Mani, Rajmohan]

Hi Tomasz,

Thanks for the reviews. I have consolidated my responses to comments from all 
of you below and are addressed in v5 of this patch.

> -Original Message-
> From: Tomasz Figa [mailto:tf...@chromium.org]
> Sent: Wednesday, May 10, 2017 11:31 PM
> To: Mani, Rajmohan <rajmohan.m...@intel.com>
> Cc: linux-media@vger.kernel.org; mche...@kernel.org; Hans Verkuil
> <hverk...@xs4all.nl>; Sakari Ailus <sakari.ai...@iki.fi>
> Subject: Re: [PATCH v4] dw9714: Initial driver for dw9714 VCM
> 
> Hi Raj,
> 
> Thanks for re-spin. Still a bit more comments inline. (I missed few more 
> before,
> sorry.)
> 
> On Thu, May 11, 2017 at 1:00 PM, Rajmohan Mani
> <rajmohan.m...@intel.com> wrote:
> > DW9714 is a 10 bit DAC, designed for linear control of voice coil
> > motor.
> [snip]
> > +static int dw9714_i2c_write(struct i2c_client *client, u16 data) {
> > +   int ret;
> > +   u16 val = cpu_to_be16(data);
> > +   const int num_bytes = sizeof(val);
> > +
> > +   ret = i2c_master_send(client, (const char *) ,
> > + sizeof(val));
> 
> nit: No need for space between cast and casted value.
> 

Ack

> > +
> > +   /*One retry */
> > +   if (ret != num_bytes)
> > +   ret = i2c_master_send(client, (const char *) ,
> > + sizeof(val));
> 
> Why do we need this retry?
> 

This was found to be useful in the early bring up days of this vcm. I think 
this can be removed now.

> > +
> > +   if (ret != num_bytes) {
> > +   dev_err(>dev, "I2C write fail\n");
> > +   return -EIO;
> > +   }
> > +   return 0;
> > +}
> > +
> > +static int dw9714_t_focus_vcm(struct dw9714_device *dw9714_dev, u16
> > +val) {
> > +   struct i2c_client *client = dw9714_dev->client;
> > +
> > +   dw9714_dev->current_val = val;
> > +
> > +   return dw9714_i2c_write(client, DW9714_VAL(val,
> > + DW9714_DEFAULT_S));
> 
> This still doesn't seem to apply the control gradually as suspend and resume 
> do.
> 

Ack
As was discussed with Sakari on the follow up threads, we can leave it as it is 
here, so the user space can control this as needed.

> > +}
> [snip]
> > +static int dw9714_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh
> > +*fh) {
> > +   struct dw9714_device *dw9714_dev = container_of(sd,
> > +   struct 
> > dw9714_device,
> > +   sd);
> > +   struct device *dev = _dev->client->dev;
> > +   int rval;
> > +
> > +   rval = pm_runtime_get_sync(dev);
> > +   if (rval >= 0)
> > +   return 0;
> > +
> > +   pm_runtime_put(dev);
> > +   return rval;
> 
> nit: The typical coding style is to return early in case of a special case 
> and keep
> the common path linear, i.e.
> 

Ack

> rval = pm_runtime_get_sync(dev);
> if (rval < 0) {
> pm_runtime_put(dev);
> return rval;
> }
> 
> return 0;
> 
> > +}
> > +
> [snip]
> > +static int dw9714_remove(struct i2c_client *client) {
> > +   struct v4l2_subdev *sd = i2c_get_clientdata(client);
> > +   struct dw9714_device *dw9714_dev = container_of(sd,
> > +   struct 
> > dw9714_device,
> > +   sd);
> > +
> > +   pm_runtime_disable(>dev);
> > +   dw9714_subdev_cleanup(dw9714_dev);
> > +
> > +   return 0;
> > +}
> > +
> > +#ifdef CONFIG_PM
> 
> #if defined(CONFIG_PM) || defined(CONFIG_PM_SLEEP)
> 

Ack. __maybe_unused is added for vcm_*suspend/resume functions

> > +
> > +/*
> > + * This function sets the vcm position, so it consumes least current
> > + * The lens position is gradually moved in units of
> > +DW9714_CTRL_STEPS,
> > + * to make the movements smoothly.
> > + */
> > +static int dw9714_vcm_suspend(struct device *dev) {
> > +   struct i2c_client *client = to_i2c_client(dev);
> > +   struct v4l2_subdev *sd = i2c_get_clientdata(client);
> > +   struct dw9714_device *dw9714_dev = container_of(sd,
> > +   struct 
> > dw9714_device,
> > +   sd);
> > +   int ret, val;
> > +
> > +   for (val = dw9714_dev->current_val & ~(DW9714_CTRL_STEPS - 1)

RE: [PATCH v2] dw9714: Initial driver for dw9714 VCM

[Mani, Rajmohan]

Hi Tomasz,
Thanks for the reviews. Please see comments inline.

> -Original Message-
> From: Tomasz Figa [mailto:tf...@chromium.org]
> Sent: Tuesday, May 09, 2017 4:44 PM
> To: Mani, Rajmohan <rajmohan.m...@intel.com>
> Cc: linux-media@vger.kernel.org; mche...@kernel.org; Hans Verkuil
> <hverk...@xs4all.nl>
> Subject: Re: [PATCH v2] dw9714: Initial driver for dw9714 VCM
> 
> Hi Rajmohan,
> 
> Some comments below.
> 
> On Mon, May 8, 2017 at 10:36 PM, Rajmohan Mani
> <rajmohan.m...@intel.com> wrote:
> > DW9714 is a 10 bit DAC, designed for linear control of voice coil
> > motor.
> >
> > This driver creates a V4L2 subdevice and provides control to set the
> > desired focus.
> >
> > Signed-off-by: Rajmohan Mani <rajmohan.m...@intel.com>
> > ---
> > Changes in v2:
> > - Addressed review comments from Hans Verkuil
> > - Fixed a debug message typo
> > - Got rid of a return variable
> > ---
> >  drivers/media/i2c/Kconfig  |   9 ++
> >  drivers/media/i2c/Makefile |   1 +
> >  drivers/media/i2c/dw9714.c | 320
> > +
> >  3 files changed, 330 insertions(+)
> >  create mode 100644 drivers/media/i2c/dw9714.c
> [snip]
> > diff --git a/drivers/media/i2c/dw9714.c b/drivers/media/i2c/dw9714.c
> > new file mode 100644 index 000..cd6cde7
> > --- /dev/null
> > +++ b/drivers/media/i2c/dw9714.c
> > @@ -0,0 +1,320 @@
> > +/*
> > + * Copyright (c) 2015--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.
> > + */
> > +
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +
> > +#define DW9714_NAME"dw9714"
> > +#define DW9714_MAX_FOCUS_POS   1023
> > +#define DW9714_CTRL_STEPS  16  /* Keep this value power of 2 */
> 
> Because?
> 

This acts as the minimum granularity of lens movement.
Keep this value power of 2, so the control steps can be uniformly adjusted for 
gradual lens movement, with desired number of control steps.

> > +#define DW9714_CTRL_DELAY_US   1000
> > +/*
> > + * S[3:2] = 0x00, codes per step for "Linear Slope Control"
> > + * S[1:0] = 0x00, step period
> > + */
> > +#define DW9714_DEFAULT_S 0x0
> > +#define DW9714_VAL(data, s) (u16)((data) << 4 | (s))
> 
> Do we need this cast?
> 
Yes. This is a write to a 2 byte register

> > +
> > +/* dw9714 device structure */
> > +struct dw9714_device {
> > +   struct i2c_client *client;
> > +   struct v4l2_ctrl_handler ctrls_vcm;
> > +   struct v4l2_subdev sd;
> > +   u16 current_val;
> > +};
> > +
> > +#define to_dw9714_vcm(_ctrl)   \
> > +   container_of(_ctrl->handler, struct dw9714_device, ctrls_vcm)
> 
> Please use a static inline function for type safety.
> 

Ack

> > +
> > +static int dw9714_i2c_write(struct i2c_client *client, u16 data) {
> > +   const int num_msg = 1;
> > +   int ret;
> > +   u16 val = cpu_to_be16(data);
> > +   struct i2c_msg msg = {
> > +   .addr = client->addr,
> > +   .flags = 0,
> > +   .len = sizeof(val),
> > +   .buf = (u8 *) ,
> > +   };
> > +
> > +   ret = i2c_transfer(client->adapter, , num_msg);
> > +
> > +   /*One retry */
> > +   if (ret != num_msg)
> > +   ret = i2c_transfer(client->adapter, , num_msg);
> > +
> > +   if (ret != num_msg) {
> > +   dev_err(>dev, "I2C write fail\n");
> > +   return -EIO;
> > +   }
> 
> I believe i2c_master_send() would simplify this function significantly.
> 

Ack

> > +   return 0;
> > +}
> > +
> > +static int dw9714_t_focus_vcm(struct dw9714_device *dw9714_dev, u16
> > +val) {
> > +   struct i2c_client *client = dw9714_dev->client;
> > +
> > +   dev_dbg(>dev, "Setting new value VCM: %d\n&quo

RE: [PATCH] dw9714: Initial driver for dw9714 VCM

[Mani, Rajmohan]

Hi Sakari,
Please see comments inline below.

> -Original Message-
> From: Sakari Ailus [mailto:sakari.ai...@iki.fi]
> Sent: Tuesday, May 09, 2017 4:55 AM
> To: Mani, Rajmohan <rajmohan.m...@intel.com>
> Cc: linux-media@vger.kernel.org; mche...@kernel.org
> Subject: Re: [PATCH] dw9714: Initial driver for dw9714 VCM
> 
> Hi Rajmohan,
> 
> A few comments below...
> 
> On Sun, May 07, 2017 at 04:33:24AM -0700, rajmohan.m...@intel.com
> wrote:
> > From: Rajmohan Mani <rajmohan.m...@intel.com>
> >
> > DW9714 is a 10 bit DAC, designed for linear control of voice coil
> > motor.
> >
> > This driver creates a V4L2 subdevice and provides control to set the
> > desired focus.
> >
> > Signed-off-by: Rajmohan Mani <rajmohan.m...@intel.com>
> > ---
> >  drivers/media/i2c/Kconfig  |   9 ++
> >  drivers/media/i2c/Makefile |   1 +
> >  drivers/media/i2c/dw9714.c | 333
> > +
> >  3 files changed, 343 insertions(+)
> >  create mode 100644 drivers/media/i2c/dw9714.c
> >
> > diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> > index fd181c9..4f0a7ad 100644
> > --- a/drivers/media/i2c/Kconfig
> > +++ b/drivers/media/i2c/Kconfig
> > @@ -300,6 +300,15 @@ config VIDEO_AD5820
> >   This is a driver for the AD5820 camera lens voice coil.
> >   It is used for example in Nokia N900 (RX-51).
> >
> > +config VIDEO_DW9714
> > +   tristate "DW9714 lens voice coil support"
> > +   depends on I2C && VIDEO_V4L2 && MEDIA_CONTROLLER
> 
> You should also depend on VIDEO_V4L2_SUBDEV_API .

Ack

> 
> > +   ---help---
> > + This is a driver for the DW9714 camera lens voice coil.
> > + DW9714 is a 10 bit DAC with 120mA output current sink
> > + capability. This is designed for linear control of
> > + voice coil motors, controlled via I2C serial interface.
> > +
> >  config VIDEO_SAA7110
> > tristate "Philips SAA7110 video decoder"
> > depends on VIDEO_V4L2 && I2C
> > diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
> > index 62323ec..987bd1f 100644
> > --- a/drivers/media/i2c/Makefile
> > +++ b/drivers/media/i2c/Makefile
> > @@ -21,6 +21,7 @@ obj-$(CONFIG_VIDEO_SAA7127) += saa7127.o
> >  obj-$(CONFIG_VIDEO_SAA7185) += saa7185.o
> >  obj-$(CONFIG_VIDEO_SAA6752HS) += saa6752hs.o
> >  obj-$(CONFIG_VIDEO_AD5820)  += ad5820.o
> > +obj-$(CONFIG_VIDEO_DW9714)  += dw9714.o
> >  obj-$(CONFIG_VIDEO_ADV7170) += adv7170.o
> >  obj-$(CONFIG_VIDEO_ADV7175) += adv7175.o
> >  obj-$(CONFIG_VIDEO_ADV7180) += adv7180.o diff --git
> > a/drivers/media/i2c/dw9714.c b/drivers/media/i2c/dw9714.c new file
> > mode 100644 index 000..276d3f2
> > --- /dev/null
> > +++ b/drivers/media/i2c/dw9714.c
> > @@ -0,0 +1,333 @@
> > +/*
> > + * Copyright (c) 2015--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.
> > + */
> > +
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +
> > +#define DW9714_NAME"dw9714"
> > +#define DW9714_MAX_FOCUS_POS   1023
> > +#define DW9714_CTRL_STEPS  16  /* Keep this value power of 2
> */
> > +#define DW9714_CTRL_DELAY_US   1000
> > +/*
> > + * S[3:2] = 0x00, codes per step for "Linear Slope Control"
> > + * S[1:0] = 0x00, step period
> > + */
> > +#define DW9714_DEFAULT_S 0x0
> > +#define DW9714_VAL(data, s) (u16)((data) << 4 | (s))
> > +
> > +/* dw9714 device structure */
> > +struct dw9714_device {
> > +   struct i2c_client *client;
> > +   struct v4l2_ctrl_handler ctrls_vcm;
> > +   struct v4l2_subdev sd;
> > +   u16 current_val;
> > +};
> > +
> > +#define to_dw9714_vcm(_ctrl)   \
> > +   container_of(_ctrl->handler, struct dw9714_device, ctrls_vcm)
> > +
> > +static int dw9714_i2c_write(struct i2c_client *client, u16 data) {
> > +   const int num_msg = 

RE: [PATCH v2] dw9714: Initial driver for dw9714 VCM

[Mani, Rajmohan]

Hi Sakari,

> -Original Message-
> From: Sakari Ailus [mailto:sakari.ai...@iki.fi]
> Sent: Tuesday, May 09, 2017 4:56 AM
> To: Mani, Rajmohan <rajmohan.m...@intel.com>
> Cc: linux-media@vger.kernel.org; mche...@kernel.org; hverk...@xs4all.nl
> Subject: Re: [PATCH v2] dw9714: Initial driver for dw9714 VCM
> 
> On Mon, May 08, 2017 at 07:36:48AM -0700, Rajmohan Mani wrote:
> > +   dev_dbg(dev, "%s ret = %d\n", __func__, ret);
> 
> Please remove such debug prints.

I have removed all dev_dbg prints and this will be addressed in v3 of this 
patch.
> 
> --
> Sakari Ailus
> e-mail: sakari.ai...@iki.fi   XMPP: sai...@retiisi.org.uk

RE: [PATCH] dw9714: Initial driver for dw9714 VCM

[Mani, Rajmohan]

Hi Hans,

Thanks for the quick review. Please see my comments inline...
I will post the new v2 patch soon.

> -Original Message-
> From: Hans Verkuil [mailto:hverk...@xs4all.nl]
> Sent: Monday, May 08, 2017 6:59 PM
> To: Mani, Rajmohan <rajmohan.m...@intel.com>; linux-
> me...@vger.kernel.org; mche...@kernel.org
> Subject: Re: [PATCH] dw9714: Initial driver for dw9714 VCM
> 
> Hi Rajmohan,
> 
> Thanks for the patch!
> 
> A quick code review:
> 
> On 05/07/2017 01:33 PM, rajmohan.m...@intel.com wrote:
> > From: Rajmohan Mani <rajmohan.m...@intel.com>
> >
> > DW9714 is a 10 bit DAC, designed for linear control of voice coil
> > motor.
> >
> > This driver creates a V4L2 subdevice and provides control to set the
> > desired focus.
> >
> > Signed-off-by: Rajmohan Mani <rajmohan.m...@intel.com>
> > ---
> >  drivers/media/i2c/Kconfig  |   9 ++
> >  drivers/media/i2c/Makefile |   1 +
> >  drivers/media/i2c/dw9714.c | 333
> > +
> >  3 files changed, 343 insertions(+)
> >  create mode 100644 drivers/media/i2c/dw9714.c
> >
> > diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> > index fd181c9..4f0a7ad 100644
> > --- a/drivers/media/i2c/Kconfig
> > +++ b/drivers/media/i2c/Kconfig
> > @@ -300,6 +300,15 @@ config VIDEO_AD5820
> >   This is a driver for the AD5820 camera lens voice coil.
> >   It is used for example in Nokia N900 (RX-51).
> >
> > +config VIDEO_DW9714
> > +   tristate "DW9714 lens voice coil support"
> > +   depends on I2C && VIDEO_V4L2 && MEDIA_CONTROLLER
> > +   ---help---
> > + This is a driver for the DW9714 camera lens voice coil.
> > + DW9714 is a 10 bit DAC with 120mA output current sink
> > + capability. This is designed for linear control of
> > + voice coil motors, controlled via I2C serial interface.
> > +
> >  config VIDEO_SAA7110
> > tristate "Philips SAA7110 video decoder"
> > depends on VIDEO_V4L2 && I2C
> > diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
> > index 62323ec..987bd1f 100644
> > --- a/drivers/media/i2c/Makefile
> > +++ b/drivers/media/i2c/Makefile
> > @@ -21,6 +21,7 @@ obj-$(CONFIG_VIDEO_SAA7127) += saa7127.o
> >  obj-$(CONFIG_VIDEO_SAA7185) += saa7185.o
> >  obj-$(CONFIG_VIDEO_SAA6752HS) += saa6752hs.o
> >  obj-$(CONFIG_VIDEO_AD5820)  += ad5820.o
> > +obj-$(CONFIG_VIDEO_DW9714)  += dw9714.o
> >  obj-$(CONFIG_VIDEO_ADV7170) += adv7170.o
> >  obj-$(CONFIG_VIDEO_ADV7175) += adv7175.o
> >  obj-$(CONFIG_VIDEO_ADV7180) += adv7180.o diff --git
> > a/drivers/media/i2c/dw9714.c b/drivers/media/i2c/dw9714.c new file
> > mode 100644 index 000..276d3f2
> > --- /dev/null
> > +++ b/drivers/media/i2c/dw9714.c
> > @@ -0,0 +1,333 @@
> > +/*
> > + * Copyright (c) 2015--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.
> > + */
> > +
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +
> > +#define DW9714_NAME"dw9714"
> > +#define DW9714_MAX_FOCUS_POS   1023
> > +#define DW9714_CTRL_STEPS  16  /* Keep this value power of 2
> */
> > +#define DW9714_CTRL_DELAY_US   1000
> > +/*
> > + * S[3:2] = 0x00, codes per step for "Linear Slope Control"
> > + * S[1:0] = 0x00, step period
> > + */
> > +#define DW9714_DEFAULT_S 0x0
> > +#define DW9714_VAL(data, s) (u16)((data) << 4 | (s))
> > +
> > +/* dw9714 device structure */
> > +struct dw9714_device {
> > +   struct i2c_client *client;
> > +   struct v4l2_ctrl_handler ctrls_vcm;
> > +   struct v4l2_subdev sd;
> > +   u16 current_val;
> > +};
> > +
> > +#define to_dw9714_vcm(_ctrl)   \
> > +   container_of(_ctrl->handler, struct dw9714_device, ctrls_vcm)
> > +
> > +static int dw9714_i2c_write(struct i2c_client *client, u16 data) {
> > +