Re: [REVIEW PATCH 4/5] v4l2: add const to argument of write-only s_register ioctl.

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

Hi Hans,

Thanks for the patch.

On Friday 15 March 2013 11:27:24 Hans Verkuil wrote:
> From: Hans Verkuil <hans.verkuil@cisco.com>
> 
> This ioctl is defined as IOW, so pass the argument as const.
> 
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>

[snip]

> diff --git a/drivers/media/pci/ivtv/ivtv-ioctl.c
> b/drivers/media/pci/ivtv/ivtv-ioctl.c index 080f179..15e08aa 100644
> --- a/drivers/media/pci/ivtv/ivtv-ioctl.c
> +++ b/drivers/media/pci/ivtv/ivtv-ioctl.c
> @@ -711,49 +711,50 @@ static int ivtv_g_chip_ident(struct file *file, void
> *fh, struct v4l2_dbg_chip_i }
> 
>  #ifdef CONFIG_VIDEO_ADV_DEBUG
> -static int ivtv_itvc(struct ivtv *itv, unsigned int cmd, void *arg)
> +static volatile u8 __iomem *ivtv_itvc_start(struct ivtv *itv,
> +		const struct v4l2_dbg_register *regs)

As you use the proper __iomem accessors I don't think you need a volatile.

>  {
> -	struct v4l2_dbg_register *regs = arg;
> -	volatile u8 __iomem *reg_start;
> -
> -	if (!capable(CAP_SYS_ADMIN))
> -		return -EPERM;
>  	if (regs->reg >= IVTV_REG_OFFSET && regs->reg < IVTV_REG_OFFSET +
> IVTV_REG_SIZE) -		reg_start = itv->reg_mem - IVTV_REG_OFFSET;
> -	else if (itv->has_cx23415 && regs->reg >= IVTV_DECODER_OFFSET &&
> +		return itv->reg_mem - IVTV_REG_OFFSET;
> +	if (itv->has_cx23415 && regs->reg >= IVTV_DECODER_OFFSET &&
>  			regs->reg < IVTV_DECODER_OFFSET + IVTV_DECODER_SIZE)
> -		reg_start = itv->dec_mem - IVTV_DECODER_OFFSET;
> -	else if (regs->reg < IVTV_ENCODER_SIZE)
> -		reg_start = itv->enc_mem;
> -	else
> -		return -EINVAL;
> -
> -	regs->size = 4;
> -	if (cmd == VIDIOC_DBG_G_REGISTER)
> -		regs->val = readl(regs->reg + reg_start);
> -	else
> -		writel(regs->val, regs->reg + reg_start);
> -	return 0;
> +		return itv->dec_mem - IVTV_DECODER_OFFSET;
> +	if (regs->reg < IVTV_ENCODER_SIZE)
> +		return itv->enc_mem;
> +	return NULL;
>  }
> 
>  static int ivtv_g_register(struct file *file, void *fh, struct
> v4l2_dbg_register *reg) {
>  	struct ivtv *itv = fh2id(fh)->itv;
> 
> -	if (v4l2_chip_match_host(&reg->match))
> -		return ivtv_itvc(itv, VIDIOC_DBG_G_REGISTER, reg);
> +	if (v4l2_chip_match_host(&reg->match)) {
> +		volatile u8 __iomem *reg_start = ivtv_itvc_start(itv, reg);
> +
> +		if (reg_start == NULL)
> +			return -EINVAL;
> +		reg->size = 4;
> +		reg->val = readl(reg->reg + reg_start);
> +		return 0;
> +	}
>  	/* TODO: subdev errors should not be ignored, this should become a
>  	   subdev helper function. */
>  	ivtv_call_all(itv, core, g_register, reg);
>  	return 0;
>  }
> 
> -static int ivtv_s_register(struct file *file, void *fh, struct
> v4l2_dbg_register *reg) +static int ivtv_s_register(struct file *file, void
> *fh, const struct v4l2_dbg_register *reg) {
>  	struct ivtv *itv = fh2id(fh)->itv;
> 
> -	if (v4l2_chip_match_host(&reg->match))
> -		return ivtv_itvc(itv, VIDIOC_DBG_S_REGISTER, reg);
> +	if (v4l2_chip_match_host(&reg->match)) {
> +		volatile u8 __iomem *reg_start = ivtv_itvc_start(itv, reg);
> +
> +		if (reg_start == NULL)
> +			return -EINVAL;
> +		writel(reg->val, reg->reg + reg_start);
> +		return 0;
> +	}
>  	/* TODO: subdev errors should not be ignored, this should become a
>  	   subdev helper function. */
>  	ivtv_call_all(itv, core, s_register, reg);

-- 
Regards,

Laurent Pinchart