From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from mx1.redhat.com ([209.132.183.28]:13804 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753287Ab3CXKIQ (ORCPT ); Sun, 24 Mar 2013 06:08:16 -0400 Date: Sun, 24 Mar 2013 07:07:03 -0300 From: Mauro Carvalho Chehab To: Hans Verkuil Cc: linux-media@vger.kernel.org, Scott Jiang , Laurent Pinchart , Jonathan Corbet , Guennadi Liakhovetski , Andy Walls , Prabhakar Lad , Kyungmin Park , Tomasz Stanislawski , Alexey Klimov , Hans de Goede , Brian Johnson , Mike Isely , Ezequiel Garcia , Huang Shijie , Ismael Luceno , Takashi Iwai , Ondrej Zary , Hans Verkuil Subject: Re: [REVIEWv2 PATCH 4/6] v4l2: add const to argument of write-only s_register ioctl. Message-ID: <20130324070703.63a4e918@redhat.com> In-Reply-To: <1363615925-19507-5-git-send-email-hverkuil@xs4all.nl> References: <1363615925-19507-1-git-send-email-hverkuil@xs4all.nl> <1363615925-19507-5-git-send-email-hverkuil@xs4all.nl> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-media-owner@vger.kernel.org List-ID: Em Mon, 18 Mar 2013 15:12:03 +0100 Hans Verkuil escreveu: > From: Hans Verkuil > > This ioctl is defined as IOW, so pass the argument as const. > > Signed-off-by: Hans Verkuil > Acked-by: Guennadi Liakhovetski > Acked-by: Lad, Prabhakar ... > 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