Hi Sakari, On Monday 04 November 2013 13:20:11 Sakari Ailus wrote: > Hi Laurent, > > Thanks for the patch. > > On Mon, Nov 04, 2013 at 11:07:48AM +0100, Laurent Pinchart wrote: > > The pad::get_fmt call must be protected by a mutex, but preparing its > > arguments doesn't need to be. Move the non-critical code out of the > > mutex-protected section. > > > > Signed-off-by: Laurent Pinchart <[email protected]> > > --- > > > > drivers/media/platform/omap3isp/ispvideo.c | 7 ++----- > > 1 file changed, 2 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/media/platform/omap3isp/ispvideo.c > > b/drivers/media/platform/omap3isp/ispvideo.c index a908d00..f6304bb > > 100644 > > --- a/drivers/media/platform/omap3isp/ispvideo.c > > +++ b/drivers/media/platform/omap3isp/ispvideo.c > > @@ -339,14 +339,11 @@ __isp_video_get_format(struct isp_video *video, > > struct v4l2_format *format)> > > if (subdev == NULL) > > > > return -EINVAL; > > > > - mutex_lock(&video->mutex); > > - > > > > fmt.pad = pad; > > fmt.which = V4L2_SUBDEV_FORMAT_ACTIVE; > > > > - ret = v4l2_subdev_call(subdev, pad, get_fmt, NULL, &fmt); > > - if (ret == -ENOIOCTLCMD) > > - ret = -EINVAL; > > By removing these lines, you're also returning -ENOIOCTLCMD to the caller. > Is this intentional? > > That return value will end up to at least one place which seems to be > isp_video_streamon() and, unless I'm mistaken, will cause > ioctl(VIDIOC_STREAMON) also return ENOTTY.
I should have split this in two patches, or at least explained the rationale in the commit message. The remote subdev is always an internal ISP subdev, the pad::get_fmt operation is thus guaranteed to be implemented. There's no need to check for ENOIOCTLCMD. > > + mutex_lock(&video->mutex); > > + ret = v4l2_subdev_call(subdev, pad, get_fmt, NULL, &fmt); > > > > mutex_unlock(&video->mutex); > > > > if (ret) -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
