[PATCH] various bugfixes for 2.6.36-rc3

[PATCH] various bugfixes for 2.6.36-rc3

[Marek Szyprowski]

Hello,

This patch series fixes a bunch of minor bugs in s5p-fimc driver,
videobuf and si470x radio driver.

This patch series has been prepared against linus/2.6.36-rc3 kernel
tree. I would like to get it merged soon to be sure that they will be
included in the final 2.6.36 release.

The complete list of patches:

[PATCH 1/8] v4l: s5p-fimc: Fix return value on probe() failure
[PATCH 2/8] v4l: s5p-fimc: Fix 3-planar formats handling and pixel offset error on S5PV210 SoCs
[PATCH 3/8] v4l: s5p-fimc: Register definition cleanup
[PATCH 4/8] v4l: videobuf: Fail streamon on an output device when no buffers queued
[PATCH 5/8] v4l: videobuf: prevent passing a NULL to dma_free_coherent()
[PATCH 6/8] v4l: videobuf: remove unused is_userptr variable
[PATCH 7/8] v4l: Add EBUSY error description for VIDIOC_STREAMON
[PATCH 8/8] v4l: radio: si470x: fix unneeded free_irq() call

Best regards
-- 
Marek Szyprowski
Samsung Poland R&D Center

[PATCH 1/8] v4l: s5p-fimc: Fix return value on probe() failure

[Marek Szyprowski]

From: Pawel Osciak <p.osciak@samsung.com>

On failed create_workqueue() fimc_probe() was returning 0.

Signed-off-by: Pawel Osciak <p.osciak@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 drivers/media/video/s5p-fimc/fimc-core.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/drivers/media/video/s5p-fimc/fimc-core.c b/drivers/media/video/s5p-fimc/fimc-core.c
index b151c7b..b03b856 100644
--- a/drivers/media/video/s5p-fimc/fimc-core.c
+++ b/drivers/media/video/s5p-fimc/fimc-core.c
@@ -1414,8 +1414,10 @@ static int fimc_probe(struct platform_device *pdev)
 	}
 
 	fimc->work_queue = create_workqueue(dev_name(&fimc->pdev->dev));
-	if (!fimc->work_queue)
+	if (!fimc->work_queue) {
+		ret = -ENOMEM;
 		goto err_irq;
+	}
 
 	ret = fimc_register_m2m_device(fimc);
 	if (ret)
-- 
1.7.2.2

[PATCH 2/8] v4l: s5p-fimc: Fix 3-planar formats handling and pixel offset error on S5PV210 SoCs

[Marek Szyprowski]

From: Sylwester Nawrocki <s.nawrocki@samsung.com>

Fix DMA engine pixel offset calculation for 3-planar YUV formats.
On S5PV210 SoCs horizontal offset is applied as number of pixels,
not bytes per line.

Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 drivers/media/video/s5p-fimc/fimc-core.c |   87 +++++++++++++-----------------
 1 files changed, 37 insertions(+), 50 deletions(-)

diff --git a/drivers/media/video/s5p-fimc/fimc-core.c b/drivers/media/video/s5p-fimc/fimc-core.c
index b03b856..e00026b 100644
--- a/drivers/media/video/s5p-fimc/fimc-core.c
+++ b/drivers/media/video/s5p-fimc/fimc-core.c
@@ -393,6 +393,37 @@ static void fimc_set_yuv_order(struct fimc_ctx *ctx)
 	dbg("ctx->out_order_1p= %d", ctx->out_order_1p);
 }
 
+static void fimc_prepare_dma_offset(struct fimc_ctx *ctx, struct fimc_frame *f)
+{
+	struct samsung_fimc_variant *variant = ctx->fimc_dev->variant;
+
+	f->dma_offset.y_h = f->offs_h;
+	if (!variant->pix_hoff)
+		f->dma_offset.y_h *= (f->fmt->depth >> 3);
+
+	f->dma_offset.y_v = f->offs_v;
+
+	f->dma_offset.cb_h = f->offs_h;
+	f->dma_offset.cb_v = f->offs_v;
+
+	f->dma_offset.cr_h = f->offs_h;
+	f->dma_offset.cr_v = f->offs_v;
+
+	if (!variant->pix_hoff) {
+		if(f->fmt->planes_cnt == 3) {
+			f->dma_offset.cb_h >>= 1;
+			f->dma_offset.cr_h >>= 1;
+		}
+		if(f->fmt->color == S5P_FIMC_YCBCR420) {
+			f->dma_offset.cb_v >>= 1;
+			f->dma_offset.cr_v >>= 1;
+		}
+	}
+
+	dbg("in_offset: color= %d, y_h= %d, y_v= %d",
+	    f->fmt->color, f->dma_offset.y_h, f->dma_offset.y_v);
+}
+
 /**
  * fimc_prepare_config - check dimensions, operation and color mode
  *			 and pre-calculate offset and the scaling coefficients.
@@ -406,7 +437,6 @@ static int fimc_prepare_config(struct fimc_ctx *ctx, u32 flags)
 {
 	struct fimc_frame *s_frame, *d_frame;
 	struct fimc_vid_buffer *buf = NULL;
-	struct samsung_fimc_variant *variant = ctx->fimc_dev->variant;
 	int ret = 0;
 
 	s_frame = &ctx->s_frame;
@@ -419,61 +449,16 @@ static int fimc_prepare_config(struct fimc_ctx *ctx, u32 flags)
 			swap(d_frame->width, d_frame->height);
 		}
 
-		/* Prepare the output offset ratios for scaler. */
-		d_frame->dma_offset.y_h = d_frame->offs_h;
-		if (!variant->pix_hoff)
-			d_frame->dma_offset.y_h *= (d_frame->fmt->depth >> 3);
-
-		d_frame->dma_offset.y_v = d_frame->offs_v;
+		/* Prepare the DMA offset ratios for scaler. */
+		fimc_prepare_dma_offset(ctx, &ctx->s_frame);
+		fimc_prepare_dma_offset(ctx, &ctx->d_frame);
 
-		d_frame->dma_offset.cb_h = d_frame->offs_h;
-		d_frame->dma_offset.cb_v = d_frame->offs_v;
-
-		d_frame->dma_offset.cr_h = d_frame->offs_h;
-		d_frame->dma_offset.cr_v = d_frame->offs_v;
-
-		if (!variant->pix_hoff && d_frame->fmt->planes_cnt == 3) {
-			d_frame->dma_offset.cb_h >>= 1;
-			d_frame->dma_offset.cb_v >>= 1;
-			d_frame->dma_offset.cr_h >>= 1;
-			d_frame->dma_offset.cr_v >>= 1;
-		}
-
-		dbg("out offset: color= %d, y_h= %d, y_v= %d",
-			d_frame->fmt->color,
-			d_frame->dma_offset.y_h, d_frame->dma_offset.y_v);
-
-		/* Prepare the input offset ratios for scaler. */
-		s_frame->dma_offset.y_h = s_frame->offs_h;
-		if (!variant->pix_hoff)
-			s_frame->dma_offset.y_h *= (s_frame->fmt->depth >> 3);
-		s_frame->dma_offset.y_v = s_frame->offs_v;
-
-		s_frame->dma_offset.cb_h = s_frame->offs_h;
-		s_frame->dma_offset.cb_v = s_frame->offs_v;
-
-		s_frame->dma_offset.cr_h = s_frame->offs_h;
-		s_frame->dma_offset.cr_v = s_frame->offs_v;
-
-		if (!variant->pix_hoff && s_frame->fmt->planes_cnt == 3) {
-			s_frame->dma_offset.cb_h >>= 1;
-			s_frame->dma_offset.cb_v >>= 1;
-			s_frame->dma_offset.cr_h >>= 1;
-			s_frame->dma_offset.cr_v >>= 1;
-		}
-
-		dbg("in offset: color= %d, y_h= %d, y_v= %d",
-			s_frame->fmt->color, s_frame->dma_offset.y_h,
-			s_frame->dma_offset.y_v);
-
-		fimc_set_yuv_order(ctx);
-
-		/* Check against the scaler ratio. */
 		if (s_frame->height > (SCALER_MAX_VRATIO * d_frame->height) ||
 		    s_frame->width > (SCALER_MAX_HRATIO * d_frame->width)) {
 			err("out of scaler range");
 			return -EINVAL;
 		}
+		fimc_set_yuv_order(ctx);
 	}
 
 	/* Input DMA mode is not allowed when the scaler is disabled. */
@@ -1494,6 +1479,7 @@ static struct samsung_fimc_variant fimc2_variant_s5p = {
 };
 
 static struct samsung_fimc_variant fimc01_variant_s5pv210 = {
+	.pix_hoff	= 1,
 	.has_inp_rot	= 1,
 	.has_out_rot	= 1,
 	.min_inp_pixsize = 16,
@@ -1508,6 +1494,7 @@ static struct samsung_fimc_variant fimc01_variant_s5pv210 = {
 };
 
 static struct samsung_fimc_variant fimc2_variant_s5pv210 = {
+	.pix_hoff	 = 1,
 	.min_inp_pixsize = 16,
 	.min_out_pixsize = 32,
 
-- 
1.7.2.2

[PATCH 3/8] v4l: s5p-fimc: Register definition cleanup

[Marek Szyprowski]

From: Sylwester Nawrocki <s.nawrocki@samsung.com>

Prepare DMA address definitions for interlaced input frame mode.

Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 drivers/media/video/s5p-fimc/fimc-reg.c  |    6 ++--
 drivers/media/video/s5p-fimc/regs-fimc.h |   38 ++++++-----------------------
 2 files changed, 11 insertions(+), 33 deletions(-)

diff --git a/drivers/media/video/s5p-fimc/fimc-reg.c b/drivers/media/video/s5p-fimc/fimc-reg.c
index 5570f1c..70f29c5 100644
--- a/drivers/media/video/s5p-fimc/fimc-reg.c
+++ b/drivers/media/video/s5p-fimc/fimc-reg.c
@@ -507,9 +507,9 @@ void fimc_hw_set_input_addr(struct fimc_dev *dev, struct fimc_addr *paddr)
 	cfg |= S5P_CIREAL_ISIZE_ADDR_CH_DIS;
 	writel(cfg, dev->regs + S5P_CIREAL_ISIZE);
 
-	writel(paddr->y, dev->regs + S5P_CIIYSA0);
-	writel(paddr->cb, dev->regs + S5P_CIICBSA0);
-	writel(paddr->cr, dev->regs + S5P_CIICRSA0);
+	writel(paddr->y, dev->regs + S5P_CIIYSA(0));
+	writel(paddr->cb, dev->regs + S5P_CIICBSA(0));
+	writel(paddr->cr, dev->regs + S5P_CIICRSA(0));
 
 	cfg &= ~S5P_CIREAL_ISIZE_ADDR_CH_DIS;
 	writel(cfg, dev->regs + S5P_CIREAL_ISIZE);
diff --git a/drivers/media/video/s5p-fimc/regs-fimc.h b/drivers/media/video/s5p-fimc/regs-fimc.h
index a3cfe82..df8cdfb 100644
--- a/drivers/media/video/s5p-fimc/regs-fimc.h
+++ b/drivers/media/video/s5p-fimc/regs-fimc.h
@@ -11,10 +11,6 @@
 #ifndef REGS_FIMC_H_
 #define REGS_FIMC_H_
 
-#define S5P_CIOYSA(__x)			(0x18 + (__x) * 4)
-#define S5P_CIOCBSA(__x)		(0x28 + (__x) * 4)
-#define S5P_CIOCRSA(__x)		(0x38 + (__x) * 4)
-
 /* Input source format */
 #define S5P_CISRCFMT			0x00
 #define S5P_CISRCFMT_ITU601_8BIT	(1 << 31)
@@ -72,23 +68,10 @@
 #define S5P_CIWDOFST2_HOROFF(x)		((x) << 16)
 #define S5P_CIWDOFST2_VEROFF(x)		((x) << 0)
 
-/* Output DMA Y plane start address */
-#define S5P_CIOYSA1			0x18
-#define S5P_CIOYSA2			0x1c
-#define S5P_CIOYSA3			0x20
-#define S5P_CIOYSA4			0x24
-
-/* Output DMA Cb plane start address */
-#define S5P_CIOCBSA1			0x28
-#define S5P_CIOCBSA2			0x2c
-#define S5P_CIOCBSA3			0x30
-#define S5P_CIOCBSA4			0x34
-
-/* Output DMA Cr plane start address */
-#define S5P_CIOCRSA1			0x38
-#define S5P_CIOCRSA2			0x3c
-#define S5P_CIOCRSA3			0x40
-#define S5P_CIOCRSA4			0x44
+/* Output DMA Y/Cb/Cr plane start addresses */
+#define S5P_CIOYSA(n)			(0x18 + (n) * 4)
+#define S5P_CIOCBSA(n)			(0x28 + (n) * 4)
+#define S5P_CIOCRSA(n)			(0x38 + (n) * 4)
 
 /* Target image format */
 #define S5P_CITRGFMT			0x48
@@ -206,10 +189,10 @@
 #define S5P_CIIMGEFF_PAT_CB(x)		((x) << 13)
 #define S5P_CIIMGEFF_PAT_CR(x)		((x) << 0)
 
-/* Input DMA Y/Cb/Cr plane start address 0 */
-#define S5P_CIIYSA0			0xd4
-#define S5P_CIICBSA0			0xd8
-#define S5P_CIICRSA0			0xdc
+/* Input DMA Y/Cb/Cr plane start address 0/1 */
+#define S5P_CIIYSA(n)			(0xd4 + (n) * 0x70)
+#define S5P_CIICBSA(n)			(0xd8 + (n) * 0x70)
+#define S5P_CIICRSA(n)			(0xdc + (n) * 0x70)
 
 /* Real input DMA image size */
 #define S5P_CIREAL_ISIZE		0xf8
@@ -250,11 +233,6 @@
 #define S5P_MSCTRL_ENVID		(1 << 0)
 #define S5P_MSCTRL_FRAME_COUNT(x)	((x) << 24)
 
-/* Input DMA Y/Cb/Cr plane start address 1 */
-#define S5P_CIIYSA1			0x144
-#define S5P_CIICBSA1			0x148
-#define S5P_CIICRSA1			0x14c
-
 /* Output DMA Y/Cb/Cr offset */
 #define S5P_CIOYOFF			0x168
 #define S5P_CIOCBOFF			0x16c
-- 
1.7.2.2

[PATCH 4/8] v4l: videobuf: Fail streamon on an output device when no buffers queued

[Marek Szyprowski]

From: Pawel Osciak <p.osciak@samsung.com>

Streamon should return -EINVAL if called on an output device and no
buffers are queued.

Signed-off-by: Pawel Osciak <p.osciak@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 drivers/media/video/videobuf-core.c |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/drivers/media/video/videobuf-core.c b/drivers/media/video/videobuf-core.c
index ce1595b..2cdf8ed 100644
--- a/drivers/media/video/videobuf-core.c
+++ b/drivers/media/video/videobuf-core.c
@@ -725,6 +725,17 @@ int videobuf_streamon(struct videobuf_queue *q)
 	int retval;
 
 	mutex_lock(&q->vb_lock);
+
+	if (q->type == V4L2_BUF_TYPE_VIDEO_OUTPUT
+			|| q->type == V4L2_BUF_TYPE_VBI_OUTPUT
+			|| q->type == V4L2_BUF_TYPE_SLICED_VBI_OUTPUT) {
+		if (list_empty(&q->stream)) {
+			dprintk(1, "streamon: no output buffers queued\n");
+			retval = -EINVAL;
+			goto done;
+		}
+	}
+
 	retval = -EBUSY;
 	if (q->reading)
 		goto done;
-- 
1.7.2.2

[PATCH 5/8] v4l: videobuf: prevent passing a NULL to dma_free_coherent()

[Marek Szyprowski]

From: Pawel Osciak <p.osciak@samsung.com>

When a driver that uses videobuf-dma-contig is used with the USERPTR
memory access method a kernel oops might happen: a NULL address may be
passed to dma_free_coherent(). This happens when an application calls
REQBUFS and then exits without queuing any buffers. This patch fixes
that bug.

Signed-off-by: Pawel Osciak <p.osciak@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 drivers/media/video/videobuf-dma-contig.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/media/video/videobuf-dma-contig.c b/drivers/media/video/videobuf-dma-contig.c
index 372b87e..6ff9e4b 100644
--- a/drivers/media/video/videobuf-dma-contig.c
+++ b/drivers/media/video/videobuf-dma-contig.c
@@ -393,8 +393,10 @@ void videobuf_dma_contig_free(struct videobuf_queue *q,
 	}
 
 	/* read() method */
-	dma_free_coherent(q->dev, mem->size, mem->vaddr, mem->dma_handle);
-	mem->vaddr = NULL;
+	if (mem->vaddr) {
+		dma_free_coherent(q->dev, mem->size, mem->vaddr, mem->dma_handle);
+		mem->vaddr = NULL;
+	}
 }
 EXPORT_SYMBOL_GPL(videobuf_dma_contig_free);
 
-- 
1.7.2.2

[PATCH 6/8] v4l: videobuf: remove unused is_userptr variable

[Marek Szyprowski]

From: Pawel Osciak <p.osciak@samsung.com>

Remove unused is_userptr variable from videobuf-dma-contig.

Signed-off-by: Pawel Osciak <p.osciak@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 drivers/media/video/videobuf-dma-contig.c |    6 ------
 1 files changed, 0 insertions(+), 6 deletions(-)

diff --git a/drivers/media/video/videobuf-dma-contig.c b/drivers/media/video/videobuf-dma-contig.c
index 6ff9e4b..4b3f0e1 100644
--- a/drivers/media/video/videobuf-dma-contig.c
+++ b/drivers/media/video/videobuf-dma-contig.c
@@ -28,7 +28,6 @@ struct videobuf_dma_contig_memory {
 	void *vaddr;
 	dma_addr_t dma_handle;
 	unsigned long size;
-	int is_userptr;
 };
 
 #define MAGIC_DC_MEM 0x0733ac61
@@ -120,7 +119,6 @@ static const struct vm_operations_struct videobuf_vm_ops = {
  */
 static void videobuf_dma_contig_user_put(struct videobuf_dma_contig_memory *mem)
 {
-	mem->is_userptr = 0;
 	mem->dma_handle = 0;
 	mem->size = 0;
 }
@@ -147,7 +145,6 @@ static int videobuf_dma_contig_user_get(struct videobuf_dma_contig_memory *mem,
 
 	offset = vb->baddr & ~PAGE_MASK;
 	mem->size = PAGE_ALIGN(vb->size + offset);
-	mem->is_userptr = 0;
 	ret = -EINVAL;
 
 	down_read(&mm->mmap_sem);
@@ -181,9 +178,6 @@ static int videobuf_dma_contig_user_get(struct videobuf_dma_contig_memory *mem,
 		pages_done++;
 	}
 
-	if (!ret)
-		mem->is_userptr = 1;
-
  out_up:
 	up_read(&current->mm->mmap_sem);
 
-- 
1.7.2.2

[PATCH 7/8] v4l: Add EBUSY error description for VIDIOC_STREAMON

[Marek Szyprowski]

From: Pawel Osciak <p.osciak@samsung.com>

VIDIOC_STREAMON should return EBUSY if streaming is already active.

Signed-off-by: Pawel Osciak <p.osciak@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 Documentation/DocBook/v4l/vidioc-streamon.xml |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/Documentation/DocBook/v4l/vidioc-streamon.xml b/Documentation/DocBook/v4l/vidioc-streamon.xml
index e42bff1..fdbd8d8 100644
--- a/Documentation/DocBook/v4l/vidioc-streamon.xml
+++ b/Documentation/DocBook/v4l/vidioc-streamon.xml
@@ -93,6 +93,13 @@ synchronize with other events.</para>
 been allocated (memory mapping) or enqueued (output) yet.</para>
 	</listitem>
       </varlistentry>
+      <varlistentry>
+	<term><errorcode>EBUSY</errorcode></term>
+	<listitem>
+	  <para><constant>VIDIOC_STREAMON</constant> called, but
+	  streaming I/O already active.</para>
+	</listitem>
+      </varlistentry>
     </variablelist>
   </refsect1>
 </refentry>
-- 
1.7.2.2

Re: [PATCH 7/8] v4l: Add EBUSY error description for VIDIOC_STREAMON

[Mauro Carvalho Chehab]

Em 06-09-2010 03:53, Marek Szyprowski escreveu:
> From: Pawel Osciak <p.osciak@samsung.com>
> 
> VIDIOC_STREAMON should return EBUSY if streaming is already active.
> 
> Signed-off-by: Pawel Osciak <p.osciak@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
>  Documentation/DocBook/v4l/vidioc-streamon.xml |    7 +++++++
>  1 files changed, 7 insertions(+), 0 deletions(-)
> 
> diff --git a/Documentation/DocBook/v4l/vidioc-streamon.xml b/Documentation/DocBook/v4l/vidioc-streamon.xml
> index e42bff1..fdbd8d8 100644
> --- a/Documentation/DocBook/v4l/vidioc-streamon.xml
> +++ b/Documentation/DocBook/v4l/vidioc-streamon.xml
> @@ -93,6 +93,13 @@ synchronize with other events.</para>
>  been allocated (memory mapping) or enqueued (output) yet.</para>
>  	</listitem>
>        </varlistentry>
> +      <varlistentry>
> +	<term><errorcode>EBUSY</errorcode></term>
> +	<listitem>
> +	  <para><constant>VIDIOC_STREAMON</constant> called, but
> +	  streaming I/O already active.</para>
> +	</listitem>
> +      </varlistentry>
>      </variablelist>
>    </refsect1>
>  </refentry>

I'm in doubt about this patch. I don't see any problem on just return 0 if
stream is active.

Actually, I think that this patch may break some applications, as there are
some cases where stream may start even without streamon (like via read() method).

Cheers,
Mauro

Re: [PATCH 7/8] v4l: Add EBUSY error description for VIDIOC_STREAMON

[Pawel Osciak]

On Fri, Oct 22, 2010 at 17:18, Mauro Carvalho Chehab <mchehab@redhat.com> wrote:
> Em 06-09-2010 03:53, Marek Szyprowski escreveu:
>> From: Pawel Osciak <p.osciak@samsung.com>
>>
>> VIDIOC_STREAMON should return EBUSY if streaming is already active.
>>
>> Signed-off-by: Pawel Osciak <p.osciak@samsung.com>
>> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
>> ---
>>  Documentation/DocBook/v4l/vidioc-streamon.xml |    7 +++++++
>>  1 files changed, 7 insertions(+), 0 deletions(-)
>>
>> diff --git a/Documentation/DocBook/v4l/vidioc-streamon.xml b/Documentation/DocBook/v4l/vidioc-streamon.xml
>> index e42bff1..fdbd8d8 100644
>> --- a/Documentation/DocBook/v4l/vidioc-streamon.xml
>> +++ b/Documentation/DocBook/v4l/vidioc-streamon.xml
>> @@ -93,6 +93,13 @@ synchronize with other events.</para>
>>  been allocated (memory mapping) or enqueued (output) yet.</para>
>>       </listitem>
>>        </varlistentry>
>> +      <varlistentry>
>> +     <term><errorcode>EBUSY</errorcode></term>
>> +     <listitem>
>> +       <para><constant>VIDIOC_STREAMON</constant> called, but
>> +       streaming I/O already active.</para>
>> +     </listitem>
>> +      </varlistentry>
>>      </variablelist>
>>    </refsect1>
>>  </refentry>
>
> I'm in doubt about this patch. I don't see any problem on just return 0 if
> stream is active.
>
> Actually, I think that this patch may break some applications, as there are
> some cases where stream may start even without streamon (like via read() method).


A quick grep over the media directory reveals that many drivers
(including videobuf_streamon) return EBUSY for some cases. This patch
was not intended to introduce something new to the API. I just wanted
to document an undocumented return value. How should the EBUSY return
be interpreted? Or should we get rid of it?

-- 
Best regards,
Pawel Osciak

Re: [PATCH 7/8] v4l: Add EBUSY error description for VIDIOC_STREAMON

[Laurent Pinchart]

On Sunday 24 October 2010 18:52:09 Pawel Osciak wrote:
> On Fri, Oct 22, 2010 at 17:18, Mauro Carvalho Chehab <mchehab@redhat.com> 
wrote:
> > Em 06-09-2010 03:53, Marek Szyprowski escreveu:
> >> From: Pawel Osciak <p.osciak@samsung.com>
> >> 
> >> VIDIOC_STREAMON should return EBUSY if streaming is already active.
> >> 
> >> Signed-off-by: Pawel Osciak <p.osciak@samsung.com>
> >> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> >> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> >> ---
> >>  Documentation/DocBook/v4l/vidioc-streamon.xml |    7 +++++++
> >>  1 files changed, 7 insertions(+), 0 deletions(-)
> >> 
> >> diff --git a/Documentation/DocBook/v4l/vidioc-streamon.xml
> >> b/Documentation/DocBook/v4l/vidioc-streamon.xml index e42bff1..fdbd8d8
> >> 100644
> >> --- a/Documentation/DocBook/v4l/vidioc-streamon.xml
> >> +++ b/Documentation/DocBook/v4l/vidioc-streamon.xml
> >> @@ -93,6 +93,13 @@ synchronize with other events.</para>
> >>  been allocated (memory mapping) or enqueued (output) yet.</para>
> >>       </listitem>
> >>        </varlistentry>
> >> +      <varlistentry>
> >> +     <term><errorcode>EBUSY</errorcode></term>
> >> +     <listitem>
> >> +       <para><constant>VIDIOC_STREAMON</constant> called, but
> >> +       streaming I/O already active.</para>
> >> +     </listitem>
> >> +      </varlistentry>
> >>      </variablelist>
> >>    </refsect1>
> >>  </refentry>
> > 
> > I'm in doubt about this patch. I don't see any problem on just return 0
> > if stream is active.
> > 
> > Actually, I think that this patch may break some applications, as there
> > are some cases where stream may start even without streamon (like via
> > read() method).
> 
> A quick grep over the media directory reveals that many drivers
> (including videobuf_streamon) return EBUSY for some cases. This patch
> was not intended to introduce something new to the API. I just wanted
> to document an undocumented return value. How should the EBUSY return
> be interpreted? Or should we get rid of it?

I think the patch makes sense. As you mention many drivers already implement 
this behaviour, so this mostly clarifies the API. Calling VIDIOC_STREAMON on 
an already streaming file handle isn't something applications should do in the 
first place anyway.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 7/8] v4l: Add EBUSY error description for VIDIOC_STREAMON

[Devin Heitmueller]

On Sun, Oct 24, 2010 at 3:50 PM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> I think the patch makes sense. As you mention many drivers already implement
> this behaviour, so this mostly clarifies the API. Calling VIDIOC_STREAMON on
> an already streaming file handle isn't something applications should do in the
> first place anyway.

I don't disagree with this behavior in principle, but Pawel should
really try this out with some of the common applications to ensure it
doesn't cause breakage (e.g. tvtime, xawtv, mythtv).

Despite the fact that some drivers already do this, that doesn't mean
that those drivers are necessarily the ones commonly used with these
applications.

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com

[PATCH 8/8] v4l: radio: si470x: fix unneeded free_irq() call

[Marek Szyprowski]

In case of error during probe() the driver calls free_irq() function
on not yet allocated irq. This patches fixes the call sequence in case of
the error.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
CC: Tobias Lorenz <tobias.lorenz@gmx.net>
CC: Joonyoung Shim <jy0922.shim@samsung.com>
CC: Douglas Schilling Landgraf <dougsland@redhat.com>
CC: Jean Delvare <khali@linux-fr.org>
---
 drivers/media/radio/si470x/radio-si470x-i2c.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/media/radio/si470x/radio-si470x-i2c.c b/drivers/media/radio/si470x/radio-si470x-i2c.c
index 67a4ec8..4ce541a 100644
--- a/drivers/media/radio/si470x/radio-si470x-i2c.c
+++ b/drivers/media/radio/si470x/radio-si470x-i2c.c
@@ -395,7 +395,7 @@ static int __devinit si470x_i2c_probe(struct i2c_client *client,
 	radio->registers[POWERCFG] = POWERCFG_ENABLE;
 	if (si470x_set_register(radio, POWERCFG) < 0) {
 		retval = -EIO;
-		goto err_all;
+		goto err_video;
 	}
 	msleep(110);
 
-- 
1.7.2.2

Re: [PATCH 8/8] v4l: radio: si470x: fix unneeded free_irq() call

[Jean Delvare]

On Mon, 06 Sep 2010 08:53:50 +0200, Marek Szyprowski wrote:
> In case of error during probe() the driver calls free_irq() function
> on not yet allocated irq. This patches fixes the call sequence in case of
> the error.
> 
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> CC: Tobias Lorenz <tobias.lorenz@gmx.net>
> CC: Joonyoung Shim <jy0922.shim@samsung.com>
> CC: Douglas Schilling Landgraf <dougsland@redhat.com>
> CC: Jean Delvare <khali@linux-fr.org>
> ---
>  drivers/media/radio/si470x/radio-si470x-i2c.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/media/radio/si470x/radio-si470x-i2c.c b/drivers/media/radio/si470x/radio-si470x-i2c.c
> index 67a4ec8..4ce541a 100644
> --- a/drivers/media/radio/si470x/radio-si470x-i2c.c
> +++ b/drivers/media/radio/si470x/radio-si470x-i2c.c
> @@ -395,7 +395,7 @@ static int __devinit si470x_i2c_probe(struct i2c_client *client,
>  	radio->registers[POWERCFG] = POWERCFG_ENABLE;
>  	if (si470x_set_register(radio, POWERCFG) < 0) {
>  		retval = -EIO;
> -		goto err_all;
> +		goto err_video;
>  	}
>  	msleep(110);
>  

Good catch.

Acked-by: Jean Delvare <khali@linux-fr.org>

-- 
Jean Delvare

Re: [PATCH 8/8] v4l: radio: si470x: fix unneeded free_irq() call

[Joonyoung Shim]

On 2010-09-06 오후 3:53, Marek Szyprowski wrote:
> In case of error during probe() the driver calls free_irq() function
> on not yet allocated irq. This patches fixes the call sequence in case of
> the error.
>

I sent this fix patch but it didn't go to linux-media ML by certain
reason. Anyway this is good catch.

Acked-by: Joonyoung Shim <jy0922.shim@samsung.com>

> Signed-off-by: Marek Szyprowski<m.szyprowski@samsung.com>
> Signed-off-by: Kyungmin Park<kyungmin.park@samsung.com>
> CC: Tobias Lorenz<tobias.lorenz@gmx.net>
> CC: Joonyoung Shim<jy0922.shim@samsung.com>
> CC: Douglas Schilling Landgraf<dougsland@redhat.com>
> CC: Jean Delvare<khali@linux-fr.org>
> ---
>   drivers/media/radio/si470x/radio-si470x-i2c.c |    2 +-
>   1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/media/radio/si470x/radio-si470x-i2c.c b/drivers/media/radio/si470x/radio-si470x-i2c.c
> index 67a4ec8..4ce541a 100644
> --- a/drivers/media/radio/si470x/radio-si470x-i2c.c
> +++ b/drivers/media/radio/si470x/radio-si470x-i2c.c
> @@ -395,7 +395,7 @@ static int __devinit si470x_i2c_probe(struct i2c_client *client,
>   	radio->registers[POWERCFG] = POWERCFG_ENABLE;
>   	if (si470x_set_register(radio, POWERCFG)<  0) {
>   		retval = -EIO;
> -		goto err_all;
> +		goto err_video;
>   	}
>   	msleep(110);
>