Em Mon, 18 Mar 2013 15:12:03 +0100
Hans Verkuil <hverkuil@xxxxxxxxx> escreveu:
> From: Hans Verkuil <hans.verkuil@xxxxxxxxx>
>
> This ioctl is defined as IOW, so pass the argument as const.
>
> Signed-off-by: Hans Verkuil <hans.verkuil@xxxxxxxxx>
> Acked-by: Guennadi Liakhovetski <g.liakhovetski@xxxxxx>
> Acked-by: Lad, Prabhakar <prabhakar.csengg@xxxxxxxxx>
...
> 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)
> {
> - 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(®->match))
> - return ivtv_itvc(itv, VIDIOC_DBG_G_REGISTER, reg);
> + if (v4l2_chip_match_host(®->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(®->match))
> - return ivtv_itvc(itv, VIDIOC_DBG_S_REGISTER, reg);
> + if (v4l2_chip_match_host(®->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);
I'm not convinced about the changes on ivtv. Why do you need volatile there?
Also, as you're doing changes there that aren't that trivial, and are not
just "add const argument", please split those non-trivial ivtv changes into
a separate patch, and properly describe what you're doing and why.
Also, having it on a separate patch helps to bisect it, if it ever brings
any problem.
--
Cheers,
Mauro
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
