From mboxrd@z Thu Jan 1 00:00:00 1970 From: Sergei Shtylyov Date: Sat, 18 Oct 2014 14:46:53 +0000 Subject: Re: [PATCH v2 2/3] media: soc_camera: rcar_vin: Add capture width check for NV16 format Message-Id: <54427D5D.70603@cogentembedded.com> List-Id: References: <1413439968-6349-1-git-send-email-ykaneko0929@gmail.com> <1413439968-6349-3-git-send-email-ykaneko0929@gmail.com> In-Reply-To: <1413439968-6349-3-git-send-email-ykaneko0929@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: Yoshihiro Kaneko , linux-media@vger.kernel.org Cc: Guennadi Liakhovetski , Simon Horman , Magnus Damm , linux-sh@vger.kernel.org Hello. On 10/16/2014 10:12 AM, Yoshihiro Kaneko wrote: > From: Koji Matsuoka > At the time of NV16 capture format, the user has to specify the > capture output width of the multiple of 32 for H/W specification. > At the time of using NV16 format by ioctl of VIDIOC_S_FMT, > this patch adds align check and the error handling to forbid > specification of the capture output width which is not a multiple of 32. > Signed-off-by: Koji Matsuoka > Signed-off-by: Yoshihiro Kaneko > --- > v2 [Yoshihiro Kaneko] > * use u32 instead of unsigned long > drivers/media/platform/soc_camera/rcar_vin.c | 24 ++++++++++++++++++++++-- > 1 file changed, 22 insertions(+), 2 deletions(-) > diff --git a/drivers/media/platform/soc_camera/rcar_vin.c b/drivers/media/platform/soc_camera/rcar_vin.c > index 34d5b80..ff5f80a 100644 > --- a/drivers/media/platform/soc_camera/rcar_vin.c > +++ b/drivers/media/platform/soc_camera/rcar_vin.c [...] > @@ -1087,6 +1091,7 @@ static int rcar_vin_set_rect(struct soc_camera_device *icd) > unsigned char dsize = 0; > struct v4l2_rect *cam_subrect = &cam->subrect; > u32 value; > + u32 imgstr; Quite strange name, given the variable's use... what does it stands for? > > dev_dbg(icd->parent, "Crop %ux%u@%u:%u\n", > icd->user_width, icd->user_height, cam->vin_left, cam->vin_top); > @@ -1164,7 +1169,11 @@ static int rcar_vin_set_rect(struct soc_camera_device *icd) > break; > } > > - iowrite32(ALIGN(cam->out_width, 0x10), priv->base + VNIS_REG); > + if (icd->current_fmt->host_fmt->fourcc = V4L2_PIX_FMT_NV16) > + imgstr = ALIGN(cam->out_width, 0x20); > + else > + imgstr = ALIGN(cam->out_width, 0x10); > + iowrite32(imgstr, priv->base + VNIS_REG); I'd call the variable 'vnis' as it gets written to the VNIS register... > > return 0; > } > @@ -1606,6 +1615,17 @@ static int rcar_vin_set_fmt(struct soc_camera_device *icd, > dev_dbg(dev, "S_FMT(pix=0x%x, %ux%u)\n", > pixfmt, pix->width, pix->height); > > + /* At the time of NV16 capture format, the user has to specify the > + width of the multiple of 32 for H/W specification. */ The preferred multi-line comment format is this: /* * bla * bla */ > + if (priv->error_flag = false) > + priv->error_flag = true; I don't see where else you check this flag, it seems rather useless. I also don't see where you clear it, other than the "add/remove device" paths. > + else { > + if ((pixfmt = V4L2_PIX_FMT_NV16) && (pix->width & 0x1F)) { > + dev_err(icd->parent, "Specified width error in NV16 format.\n"); > + return -EINVAL; > + } > + } > + WBR, Sergei