On Sun March 24 2013 11:07:03 Mauro Carvalho Chehab wrote:
> 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?
It was using volatile before and as I told Laurent as well, I don't want to
mix changing volatile removal with adding const support.
> 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.
OK, and I'll add a separate patch in that case to remove the volatile part.
> Also, having it on a separate patch helps to bisect it, if it ever brings
> any problem.
Regards,
Hans
--
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
