Re: [REVIEW PATCH 3/5] v4l2: pass std by value to the write-only s_std ioctl.

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Hans,

Thanks for the patch.

On Friday 15 March 2013 11:27:23 Hans Verkuil wrote:
> From: Hans Verkuil <hans.verkuil@cisco.com>
> 
> This ioctl is defined as IOW, so pass the argument by value instead of by
> reference. I could have chosen to add const instead, but this is 1) easier
> to handle in drivers and 2) consistent with the s_std subdev operation.
> 
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>

Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

[snip]

> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c
> b/drivers/media/v4l2-core/v4l2-ioctl.c index 8ec8abe..d80d8af 100644
> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> @@ -1383,15 +1383,15 @@ static int v4l_s_std(const struct v4l2_ioctl_ops
> *ops, struct file *file, void *fh, void *arg)
>  {
>  	struct video_device *vfd = video_devdata(file);
> -	v4l2_std_id *id = arg, norm;
> +	v4l2_std_id id = *(v4l2_std_id *)arg, norm;

This is getting a bit hard to read, I would have split the declaration of norm 
to a separate line, but that's just nit-picking.

>  	int ret;
> 
> -	norm = (*id) & vfd->tvnorms;
> +	norm = id & vfd->tvnorms;
>  	if (vfd->tvnorms && !norm)	/* Check if std is supported */
>  		return -EINVAL;
> 
>  	/* Calls the specific handler */
> -	ret = ops->vidioc_s_std(file, fh, &norm);
> +	ret = ops->vidioc_s_std(file, fh, norm);
> 
>  	/* Updates standard information */
>  	if (ret >= 0)

-- 
Regards,

Laurent Pinchart