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

[Laurent Pinchart]

Hi Hans,

Thanks for the patch.

On Monday 18 March 2013 15:12:03 Hans Verkuil wrote:
> From: Hans Verkuil <hans.verk...@cisco.com>
> 
> This ioctl is defined as IOW, so pass the argument as const.
> 
> Signed-off-by: Hans Verkuil <hans.verk...@cisco.com>
> Acked-by: Guennadi Liakhovetski <g.liakhovet...@gmx.de>
> Acked-by: Lad, Prabhakar <prabhakar.cse...@gmail.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)

I haven't changed my mind since v1, I still don't think you need a volatile 
here :-)

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

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html