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

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

 



Hi Hans,

Thanks for the review.

On Friday 04 February 2011 12:55:50 Hans Verkuil wrote:
> 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.

Yes it is. I forgot to mention that in the cover letter.

> 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.

OK. It was a leftover.

> 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.

OK.

> 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.

It's intentional.

> 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?

OK.

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

Yes, but not before 2.6.39 :-)

-- 
Regards,

Laurent Pinchart
--
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