Re: [PATCH v5 0/5] OMAP3 ISP driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thursday, January 27, 2011 13:32:16 Laurent Pinchart wrote:
> Hi everybody,
> 
> Here's the fifth version of the OMAP3 ISP driver patches, updated to
> 2.6.37 and the latest changes in the media controller and sub-device APIs.

Hmm, patch 5/5 is missing. It's probably too big.

Anyway, I got the patch from your git tree and did a review. It's always hard
to review over 21000 lines of driver code :-), so I limited myself to the V4L2
API parts. I can't really comment on the OMAP3 specific parts anyway.

The first issue I found was related to controls: it seems you set up control
handlers for subdevs that don't have any controls. You can just leave
sd->ctrl_handler to NULL in that case and you don't need to use a control handler
at all.

There is also no need to set the core ctrl ops:

+       .queryctrl = v4l2_subdev_queryctrl,
+       .querymenu = v4l2_subdev_querymenu,
+       .g_ctrl = v4l2_subdev_g_ctrl,
+       .s_ctrl = v4l2_subdev_s_ctrl,
+       .g_ext_ctrls = v4l2_subdev_g_ext_ctrls,
+       .try_ext_ctrls = v4l2_subdev_try_ext_ctrls,
+       .s_ext_ctrls = v4l2_subdev_s_ext_ctrls,

These are only necessary if the master driver doesn't use the control
framework but called core.queryctrl directly. That shouldn't be the case
for this driver.

What isn't clear to me is whether the /dev/videoX nodes should give access
to the subdev controls as well. As far as I can see the ctrl_handler pointer
of neither v4l2_device nor video_device is ever set, so that means that the
controls are only accessible through /dev/v4l-subdevX.

I'm not sure whether that is intentional or not.

The other comment I have is regarding include/linux/omap3isp.h: both the
ioctls and the events need to be documented there. A one-liner for each is
probably enough. I also see that struct omap3isp_stat_data has a deprecated
field: perhaps when creating the final pull request the time is right to
remove it?

Finally, I noticed that OMAP3 has its own implementation of videobuf. Are
there plans to move to vb2?

Regards,

	Hans

> 
> You can find the patches in http://git.linuxtv.org/pinchartl/media.git as
> usual (media-0005-omap3isp).
> 
> Laurent Pinchart (2):
>   omap3: Add function to register omap3isp platform device structure
>   OMAP3 ISP driver
> 
> Sergio Aguirre (2):
>   omap3: Remove unusued ISP CBUFF resource
>   omap2: Fix camera resources for multiomap
> 
> Tuukka Toivonen (1):
>   ARM: OMAP3: Update Camera ISP definitions for OMAP3630
> 
>  arch/arm/mach-omap2/devices.c                |   64 +-
>  arch/arm/mach-omap2/devices.h                |   17 +
>  arch/arm/plat-omap/include/plat/omap34xx.h   |   16 +-
>  drivers/media/video/Kconfig                  |   13 +
>  drivers/media/video/Makefile                 |    2 +
>  drivers/media/video/isp/Makefile             |   13 +
>  drivers/media/video/isp/cfa_coef_table.h     |  601 +++++++
>  drivers/media/video/isp/gamma_table.h        |   90 +
>  drivers/media/video/isp/isp.c                | 2221 +++++++++++++++++++++++++
>  drivers/media/video/isp/isp.h                |  427 +++++
>  drivers/media/video/isp/ispccdc.c            | 2280 ++++++++++++++++++++++++++
>  drivers/media/video/isp/ispccdc.h            |  223 +++
>  drivers/media/video/isp/ispccp2.c            | 1189 ++++++++++++++
>  drivers/media/video/isp/ispccp2.h            |  101 ++
>  drivers/media/video/isp/ispcsi2.c            | 1332 +++++++++++++++
>  drivers/media/video/isp/ispcsi2.h            |  169 ++
>  drivers/media/video/isp/ispcsiphy.c          |  247 +++
>  drivers/media/video/isp/ispcsiphy.h          |   74 +
>  drivers/media/video/isp/isph3a.h             |  117 ++
>  drivers/media/video/isp/isph3a_aewb.c        |  374 +++++
>  drivers/media/video/isp/isph3a_af.c          |  429 +++++
>  drivers/media/video/isp/isphist.c            |  520 ++++++
>  drivers/media/video/isp/isphist.h            |   40 +
>  drivers/media/video/isp/isppreview.c         | 2120 ++++++++++++++++++++++++
>  drivers/media/video/isp/isppreview.h         |  214 +++
>  drivers/media/video/isp/ispqueue.c           | 1136 +++++++++++++
>  drivers/media/video/isp/ispqueue.h           |  185 +++
>  drivers/media/video/isp/ispreg.h             | 1589 ++++++++++++++++++
>  drivers/media/video/isp/ispresizer.c         | 1710 +++++++++++++++++++
>  drivers/media/video/isp/ispresizer.h         |  150 ++
>  drivers/media/video/isp/ispstat.c            | 1100 +++++++++++++
>  drivers/media/video/isp/ispstat.h            |  169 ++
>  drivers/media/video/isp/ispvideo.c           | 1264 ++++++++++++++
>  drivers/media/video/isp/ispvideo.h           |  202 +++
>  drivers/media/video/isp/luma_enhance_table.h |  154 ++
>  drivers/media/video/isp/noise_filter_table.h |   90 +
>  include/linux/Kbuild                         |    1 +
>  include/linux/omap3isp.h                     |  631 +++++++
>  38 files changed, 21246 insertions(+), 28 deletions(-)
>  create mode 100644 arch/arm/mach-omap2/devices.h
>  create mode 100644 drivers/media/video/isp/Makefile
>  create mode 100644 drivers/media/video/isp/cfa_coef_table.h
>  create mode 100644 drivers/media/video/isp/gamma_table.h
>  create mode 100644 drivers/media/video/isp/isp.c
>  create mode 100644 drivers/media/video/isp/isp.h
>  create mode 100644 drivers/media/video/isp/ispccdc.c
>  create mode 100644 drivers/media/video/isp/ispccdc.h
>  create mode 100644 drivers/media/video/isp/ispccp2.c
>  create mode 100644 drivers/media/video/isp/ispccp2.h
>  create mode 100644 drivers/media/video/isp/ispcsi2.c
>  create mode 100644 drivers/media/video/isp/ispcsi2.h
>  create mode 100644 drivers/media/video/isp/ispcsiphy.c
>  create mode 100644 drivers/media/video/isp/ispcsiphy.h
>  create mode 100644 drivers/media/video/isp/isph3a.h
>  create mode 100644 drivers/media/video/isp/isph3a_aewb.c
>  create mode 100644 drivers/media/video/isp/isph3a_af.c
>  create mode 100644 drivers/media/video/isp/isphist.c
>  create mode 100644 drivers/media/video/isp/isphist.h
>  create mode 100644 drivers/media/video/isp/isppreview.c
>  create mode 100644 drivers/media/video/isp/isppreview.h
>  create mode 100644 drivers/media/video/isp/ispqueue.c
>  create mode 100644 drivers/media/video/isp/ispqueue.h
>  create mode 100644 drivers/media/video/isp/ispreg.h
>  create mode 100644 drivers/media/video/isp/ispresizer.c
>  create mode 100644 drivers/media/video/isp/ispresizer.h
>  create mode 100644 drivers/media/video/isp/ispstat.c
>  create mode 100644 drivers/media/video/isp/ispstat.h
>  create mode 100644 drivers/media/video/isp/ispvideo.c
>  create mode 100644 drivers/media/video/isp/ispvideo.h
>  create mode 100644 drivers/media/video/isp/luma_enhance_table.h
>  create mode 100644 drivers/media/video/isp/noise_filter_table.h
>  create mode 100644 include/linux/omap3isp.h
> 
> 

-- 
Hans Verkuil - video4linux developer - sponsored by Cisco
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [Linux Arm (vger)]     [ARM Kernel]     [ARM MSM]     [Linux Tegra]     [Linux WPAN Networking]     [Linux Wireless Networking]     [Maemo Users]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Trails]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux