RE: [PATCH 2/2] mem2mem_testdev: Code cleanup
> -Original Message- > From: Pawel Osciak [mailto:p.osc...@samsung.com] > Sent: Thursday, April 01, 2010 3:39 PM > To: Hiremath, Vaibhav > Cc: linux-media@vger.kernel.org; Marek Szyprowski; kyungmin.p...@samsung.com > Subject: RE: [PATCH 2/2] mem2mem_testdev: Code cleanup > > Hi again, > > > Vaibhav Hiremath wrote: > >From: Vaibhav Hiremath > > > > > >Signed-off-by: Vaibhav Hiremath > >--- > > drivers/media/video/mem2mem_testdev.c | 58 ++--- > --- > > 1 files changed, 25 insertions(+), 33 deletions(-) > > > >diff --git a/drivers/media/video/mem2mem_testdev.c > b/drivers/media/video/mem2mem_testdev.c > >index 05630e3..1f35b7e 100644 > >--- a/drivers/media/video/mem2mem_testdev.c > >+++ b/drivers/media/video/mem2mem_testdev.c > >@@ -98,11 +98,10 @@ static struct m2mtest_fmt formats[] = { > > }; > > > > /* Per-queue, driver-specific private data */ > >-struct m2mtest_q_data > >-{ > >-unsigned intwidth; > >-unsigned intheight; > >-unsigned intsizeimage; > >+struct m2mtest_q_data { > >+u32 width; > >+u32 height; > >+u32 sizeimage; > > struct m2mtest_fmt *fmt; > > }; > > Could you explain this change? > [Hiremath, Vaibhav] No specific reason for this change, this is normal practice I learned from very first patch. > [...] > > >@@ -158,7 +156,7 @@ static struct v4l2_queryctrl m2mtest_ctrls[] = { > > static struct m2mtest_fmt *find_format(struct v4l2_format *f) > > { > > struct m2mtest_fmt *fmt; > >-unsigned int k; > >+u32 k; > > This is a loop index... Is there any reason for using u32? > [Hiremath, Vaibhav] Same as above. > [...] > > >@@ -535,8 +532,8 @@ static int vidioc_s_fmt(struct m2mtest_ctx *ctx, struct > v4l2_format *f) > > > > if (videobuf_queue_is_busy(vq)) { > > v4l2_err(&ctx->dev->v4l2_dev, "%s queue busy\n", __func__); > >-ret = -EBUSY; > >-goto out; > >+mutex_unlock(&vq->vb_lock); > >+return -EBUSY; > > } > > > > q_data->fmt = find_format(f); > >@@ -550,9 +547,7 @@ static int vidioc_s_fmt(struct m2mtest_ctx *ctx, struct > v4l2_format *f) > > "Setting format for type %d, wxh: %dx%d, fmt: %d\n", > > f->type, q_data->width, q_data->height, q_data->fmt->fourcc); > > > >-out: > >-mutex_unlock(&vq->vb_lock); > >-return ret; > >+return 0; > > } > > > > Unless I'm somehow misreading patch output, aren't you removing mutex_unlock > for the path > that reaches the end of the function? > [Hiremath, Vaibhav] Oops, how did this got merged to patch? I had removed it. Just remove/ignore this change while merging. Thanks, Vaibhav > [...] > > > Best regards > -- > Pawel Osciak > Linux Platform Group > Samsung Poland R&D Center > > > > -- 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
RE: [PATCH 2/2] mem2mem_testdev: Code cleanup
Hi again, > Vaibhav Hiremath wrote: >From: Vaibhav Hiremath > > >Signed-off-by: Vaibhav Hiremath >--- > drivers/media/video/mem2mem_testdev.c | 58 ++-- > 1 files changed, 25 insertions(+), 33 deletions(-) > >diff --git a/drivers/media/video/mem2mem_testdev.c >b/drivers/media/video/mem2mem_testdev.c >index 05630e3..1f35b7e 100644 >--- a/drivers/media/video/mem2mem_testdev.c >+++ b/drivers/media/video/mem2mem_testdev.c >@@ -98,11 +98,10 @@ static struct m2mtest_fmt formats[] = { > }; > > /* Per-queue, driver-specific private data */ >-struct m2mtest_q_data >-{ >- unsigned intwidth; >- unsigned intheight; >- unsigned intsizeimage; >+struct m2mtest_q_data { >+ u32 width; >+ u32 height; >+ u32 sizeimage; > struct m2mtest_fmt *fmt; > }; Could you explain this change? [...] >@@ -158,7 +156,7 @@ static struct v4l2_queryctrl m2mtest_ctrls[] = { > static struct m2mtest_fmt *find_format(struct v4l2_format *f) > { > struct m2mtest_fmt *fmt; >- unsigned int k; >+ u32 k; This is a loop index... Is there any reason for using u32? [...] >@@ -535,8 +532,8 @@ static int vidioc_s_fmt(struct m2mtest_ctx *ctx, struct >v4l2_format *f) > > if (videobuf_queue_is_busy(vq)) { > v4l2_err(&ctx->dev->v4l2_dev, "%s queue busy\n", __func__); >- ret = -EBUSY; >- goto out; >+ mutex_unlock(&vq->vb_lock); >+ return -EBUSY; > } > > q_data->fmt = find_format(f); >@@ -550,9 +547,7 @@ static int vidioc_s_fmt(struct m2mtest_ctx *ctx, struct >v4l2_format *f) > "Setting format for type %d, wxh: %dx%d, fmt: %d\n", > f->type, q_data->width, q_data->height, q_data->fmt->fourcc); > >-out: >- mutex_unlock(&vq->vb_lock); >- return ret; >+ return 0; > } > Unless I'm somehow misreading patch output, aren't you removing mutex_unlock for the path that reaches the end of the function? [...] Best regards -- Pawel Osciak Linux Platform Group Samsung Poland R&D Center -- 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
[PATCH 2/2] mem2mem_testdev: Code cleanup
From: Vaibhav Hiremath Signed-off-by: Vaibhav Hiremath --- drivers/media/video/mem2mem_testdev.c | 58 ++-- 1 files changed, 25 insertions(+), 33 deletions(-) diff --git a/drivers/media/video/mem2mem_testdev.c b/drivers/media/video/mem2mem_testdev.c index 05630e3..1f35b7e 100644 --- a/drivers/media/video/mem2mem_testdev.c +++ b/drivers/media/video/mem2mem_testdev.c @@ -98,11 +98,10 @@ static struct m2mtest_fmt formats[] = { }; /* Per-queue, driver-specific private data */ -struct m2mtest_q_data -{ - unsigned intwidth; - unsigned intheight; - unsigned intsizeimage; +struct m2mtest_q_data { + u32 width; + u32 height; + u32 sizeimage; struct m2mtest_fmt *fmt; }; @@ -123,11 +122,10 @@ static struct m2mtest_q_data *get_q_data(enum v4l2_buf_type type) return &q_data[V4L2_M2M_DST]; default: BUG(); - return NULL; } + return NULL; } - #define V4L2_CID_TRANS_TIME_MSEC V4L2_CID_PRIVATE_BASE #define V4L2_CID_TRANS_NUM_BUFS(V4L2_CID_PRIVATE_BASE + 1) @@ -158,7 +156,7 @@ static struct v4l2_queryctrl m2mtest_ctrls[] = { static struct m2mtest_fmt *find_format(struct v4l2_format *f) { struct m2mtest_fmt *fmt; - unsigned int k; + u32 k; for (k = 0; k < NUM_FORMATS; k++) { fmt = &formats[k]; @@ -237,12 +235,12 @@ static int device_process(struct m2mtest_ctx *ctx, if (!p_in || !p_out) { v4l2_err(&dev->v4l2_dev, "Acquiring kernel pointers to buffers failed\n"); - return 1; + return -EFAULT; } if (in_buf->vb.size < out_buf->vb.size) { v4l2_err(&dev->v4l2_dev, "Output buffer is too small\n"); - return 1; + return -EINVAL; } tile_w = (in_buf->vb.width * (q_data[V4L2_M2M_DST].fmt->depth >> 3)) @@ -361,8 +359,6 @@ static void device_isr(unsigned long priv) spin_unlock_irqrestore(&m2mtest_dev->irqlock, flags); device_run(curr_ctx); } - - return; } @@ -384,10 +380,7 @@ static int vidioc_querycap(struct file *file, void *priv, static int enum_fmt(struct v4l2_fmtdesc *f, u32 type) { - int i, num; - struct m2mtest_fmt *fmt; - - num = 0; + int i, num = 0; for (i = 0; i < NUM_FORMATS; ++i) { if (formats[i].types & type) { @@ -402,9 +395,9 @@ static int enum_fmt(struct v4l2_fmtdesc *f, u32 type) if (i < NUM_FORMATS) { /* Format found */ - fmt = &formats[i]; - strncpy(f->description, fmt->name, sizeof(f->description) - 1); - f->pixelformat = fmt->fourcc; + strncpy(f->description, formats[i].name, + sizeof(f->description) - 1); + f->pixelformat = formats[i].fourcc; return 0; } @@ -430,6 +423,9 @@ static int vidioc_g_fmt(struct m2mtest_ctx *ctx, struct v4l2_format *f) struct m2mtest_q_data *q_data; vq = v4l2_m2m_get_vq(ctx->m2m_ctx, f->type); + if (!vq) + return -EINVAL; + q_data = get_q_data(f->type); f->fmt.pix.width= q_data->width; @@ -524,9 +520,10 @@ static int vidioc_s_fmt(struct m2mtest_ctx *ctx, struct v4l2_format *f) { struct m2mtest_q_data *q_data; struct videobuf_queue *vq; - int ret = 0; vq = v4l2_m2m_get_vq(ctx->m2m_ctx, f->type); + if (!vq) + return -EINVAL; q_data = get_q_data(f->type); if (!q_data) return -EINVAL; @@ -535,8 +532,8 @@ static int vidioc_s_fmt(struct m2mtest_ctx *ctx, struct v4l2_format *f) if (videobuf_queue_is_busy(vq)) { v4l2_err(&ctx->dev->v4l2_dev, "%s queue busy\n", __func__); - ret = -EBUSY; - goto out; + mutex_unlock(&vq->vb_lock); + return -EBUSY; } q_data->fmt = find_format(f); @@ -550,9 +547,7 @@ static int vidioc_s_fmt(struct m2mtest_ctx *ctx, struct v4l2_format *f) "Setting format for type %d, wxh: %dx%d, fmt: %d\n", f->type, q_data->width, q_data->height, q_data->fmt->fourcc); -out: - mutex_unlock(&vq->vb_lock); - return ret; + return 0; } static int vidioc_s_fmt_vid_cap(struct file *file, void *priv, @@ -857,13 +852,9 @@ static int m2mtest_open(struct file *file) struct m2mtest_dev *dev = video_drvdata(file); struct m2mtest_ctx *ctx = NULL; - atomic_inc(&dev->num_inst); - ctx = kzalloc(sizeof *ctx, GFP_KERNEL); - if (!ctx) { - atomic_dec(&dev->num_inst); + if (!ctx) return -EN