[RFC PATCH 0/7] saa7134: improve v4l2-compliance

[RFC PATCH 0/7] saa7134: improve v4l2-compliance

[Ondrej Zary]

Hello,
this patch series improves v4l2-compliance of saa7134 driver. There are still
some problems. Controls require conversion to control framework which I was
unable to finish (because the driver accesses other controls and also the
file handle from within s_ctrl).

Radio is now OK except for controls.
Video has problems with controls, debugging, formats and buffers:
Debug ioctls:
        test VIDIOC_DBG_G_CHIP_IDENT: OK (Not Supported)
                fail: v4l2-test-debug.cpp(84): doioctl(node, VIDIOC_DBG_G_CHIP_IDENT, &chip)
Format ioctls:
        test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK
                fail: v4l2-test-formats.cpp(836): !cap->readbuffers
        test VIDIOC_G/S_PARM: FAIL
                fail: v4l2-test-formats.cpp(335): !fmt.width || !fmt.height
        test VIDIOC_G_FBUF: FAIL
                fail: v4l2-test-formats.cpp(382): !pix.colorspace
Buffer ioctls:
                fail: v4l2-test-buffers.cpp(109): can_stream && !mmap_valid && !userptr_valid && !dmabuf_valid
        test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: FAIL


Haven't looked into VBI yet:
Format ioctls:
                fail: v4l2-test-formats.cpp(914): G/S_PARM is only allowed for video capture/output
        test VIDIOC_G/S_PARM: FAIL
                fail: v4l2-test-formats.cpp(432): vbi.reserved not zeroed
                fail: v4l2-test-formats.cpp(570): VBI Capture is valid, but TRY_FMT failed to return a format
        test VIDIOC_TRY_FMT: FAIL
                fail: v4l2-test-formats.cpp(432): vbi.reserved not zeroed
                fail: v4l2-test-formats.cpp(728): VBI Capture is valid, but no S_FMT was implemented
        test VIDIOC_S_FMT: FAIL

-- 
Ondrej Zary

[PATCH 1/7] saa7134: v4l2-compliance: implement V4L2_CAP_DEVICE_CAPS

[Ondrej Zary]

Make saa7134 driver more V4L2 compliant: implement V4L2_CAP_DEVICE_CAPS support
and fix all capabilities problems reported by v4l2-compliance.

Signed-off-by: Ondrej Zary <linux@rainbow-software.org>
---
 drivers/media/pci/saa7134/saa7134-video.c |   55 ++++++++++++++++------------
 1 files changed, 31 insertions(+), 24 deletions(-)

diff --git a/drivers/media/pci/saa7134/saa7134-video.c b/drivers/media/pci/saa7134/saa7134-video.c
index 3abf527..ce15f1f 100644
--- a/drivers/media/pci/saa7134/saa7134-video.c
+++ b/drivers/media/pci/saa7134/saa7134-video.c
@@ -1805,6 +1805,8 @@ static int saa7134_querycap(struct file *file, void  *priv,
 {
 	struct saa7134_fh *fh = priv;
 	struct saa7134_dev *dev = fh->dev;
+	struct video_device *vdev = video_devdata(file);
+	u32 radio_caps, video_caps, vbi_caps;
 
 	unsigned int tuner_type = dev->tuner_type;
 
@@ -1812,19 +1814,37 @@ static int saa7134_querycap(struct file *file, void  *priv,
 	strlcpy(cap->card, saa7134_boards[dev->board].name,
 		sizeof(cap->card));
 	sprintf(cap->bus_info, "PCI:%s", pci_name(dev->pci));
-	cap->capabilities =
-		V4L2_CAP_VIDEO_CAPTURE |
-		V4L2_CAP_VBI_CAPTURE |
-		V4L2_CAP_READWRITE |
-		V4L2_CAP_STREAMING |
-		V4L2_CAP_TUNER;
+
+	cap->device_caps = V4L2_CAP_READWRITE | V4L2_CAP_STREAMING;
+	if ((tuner_type != TUNER_ABSENT) && (tuner_type != UNSET))
+		cap->device_caps |= V4L2_CAP_TUNER;
+
+	radio_caps = V4L2_CAP_RADIO;
 	if (dev->has_rds)
-		cap->capabilities |= V4L2_CAP_RDS_CAPTURE;
+		radio_caps |= V4L2_CAP_RDS_CAPTURE;
+
+	video_caps = V4L2_CAP_VIDEO_CAPTURE;
 	if (saa7134_no_overlay <= 0)
-		cap->capabilities |= V4L2_CAP_VIDEO_OVERLAY;
+		video_caps |= V4L2_CAP_VIDEO_OVERLAY;
+
+	vbi_caps = V4L2_CAP_VBI_CAPTURE;
+
+	switch (vdev->vfl_type) {
+	case VFL_TYPE_RADIO:
+		cap->device_caps |= radio_caps;
+		break;
+	case VFL_TYPE_GRABBER:
+		cap->device_caps |= video_caps;
+		break;
+	case VFL_TYPE_VBI:
+		cap->device_caps |= vbi_caps;
+		break;
+	}
+	cap->capabilities = radio_caps | video_caps | vbi_caps |
+		cap->device_caps | V4L2_CAP_DEVICE_CAPS;
+	if (vdev->vfl_type == VFL_TYPE_RADIO)
+		cap->device_caps &= ~(V4L2_CAP_READWRITE | V4L2_CAP_STREAMING);
 
-	if ((tuner_type == TUNER_ABSENT) || (tuner_type == UNSET))
-		cap->capabilities &= ~V4L2_CAP_TUNER;
 	return 0;
 }
 
@@ -2299,19 +2319,6 @@ static int vidioc_s_register (struct file *file, void *priv,
 }
 #endif
 
-static int radio_querycap(struct file *file, void *priv,
-					struct v4l2_capability *cap)
-{
-	struct saa7134_fh *fh = file->private_data;
-	struct saa7134_dev *dev = fh->dev;
-
-	strcpy(cap->driver, "saa7134");
-	strlcpy(cap->card, saa7134_boards[dev->board].name, sizeof(cap->card));
-	sprintf(cap->bus_info, "PCI:%s", pci_name(dev->pci));
-	cap->capabilities = V4L2_CAP_TUNER;
-	return 0;
-}
-
 static int radio_g_tuner(struct file *file, void *priv,
 					struct v4l2_tuner *t)
 {
@@ -2473,7 +2480,7 @@ static const struct v4l2_file_operations radio_fops = {
 };
 
 static const struct v4l2_ioctl_ops radio_ioctl_ops = {
-	.vidioc_querycap	= radio_querycap,
+	.vidioc_querycap	= saa7134_querycap,
 	.vidioc_g_tuner		= radio_g_tuner,
 	.vidioc_enum_input	= radio_enum_input,
 	.vidioc_g_audio		= radio_g_audio,
-- 
Ondrej Zary

[PATCH 2/7] saa7134: v4l2-compliance: don't report invalid audio modes for radio

[Ondrej Zary]

Make saa7134 driver more V4L2 compliant: filter audio modes that came from
tuner - keep only MONO/STEREO in radio mode

Signed-off-by: Ondrej Zary <linux@rainbow-software.org>
---
 drivers/media/pci/saa7134/saa7134-video.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/media/pci/saa7134/saa7134-video.c b/drivers/media/pci/saa7134/saa7134-video.c
index ce15f1f..db8da32 100644
--- a/drivers/media/pci/saa7134/saa7134-video.c
+++ b/drivers/media/pci/saa7134/saa7134-video.c
@@ -2333,6 +2333,7 @@ static int radio_g_tuner(struct file *file, void *priv,
 	t->type = V4L2_TUNER_RADIO;
 
 	saa_call_all(dev, tuner, g_tuner, t);
+	t->audmode &= V4L2_TUNER_MODE_MONO | V4L2_TUNER_MODE_STEREO;
 	if (dev->input->amux == TV) {
 		t->signal = 0xf800 - ((saa_readb(0x581) & 0x1f) << 11);
 		t->rxsubchans = (saa_readb(0x529) & 0x08) ?
-- 
Ondrej Zary

[PATCH 3/7] saa7134: v4l2-compliance: use v4l2_fh to fix priority handling

[Ondrej Zary]

Make saa7134 driver more V4L2 compliant: remove broken priority handling
and use v4l2_fh instead

Signed-off-by: Ondrej Zary <linux@rainbow-software.org>
---
 drivers/media/pci/saa7134/saa7134-core.c  |    3 +-
 drivers/media/pci/saa7134/saa7134-video.c |   61 +++-------------------------
 drivers/media/pci/saa7134/saa7134.h       |    4 +-
 3 files changed, 10 insertions(+), 58 deletions(-)

diff --git a/drivers/media/pci/saa7134/saa7134-core.c b/drivers/media/pci/saa7134/saa7134-core.c
index 8976d0e..ba08bd6 100644
--- a/drivers/media/pci/saa7134/saa7134-core.c
+++ b/drivers/media/pci/saa7134/saa7134-core.c
@@ -805,6 +805,7 @@ static struct video_device *vdev_init(struct saa7134_dev *dev,
 	vfd->debug   = video_debug;
 	snprintf(vfd->name, sizeof(vfd->name), "%s %s (%s)",
 		 dev->name, type, saa7134_boards[dev->board].name);
+	set_bit(V4L2_FL_USE_FH_PRIO, &vfd->flags);
 	video_set_drvdata(vfd, dev);
 	return vfd;
 }
@@ -1028,8 +1029,6 @@ static int __devinit saa7134_initdev(struct pci_dev *pci_dev,
 		}
 	}
 
-	v4l2_prio_init(&dev->prio);
-
 	mutex_lock(&saa7134_devlist_lock);
 	list_for_each_entry(mops, &mops_list, next)
 		mpeg_ops_attach(mops, dev);
diff --git a/drivers/media/pci/saa7134/saa7134-video.c b/drivers/media/pci/saa7134/saa7134-video.c
index db8da32..be745c0 100644
--- a/drivers/media/pci/saa7134/saa7134-video.c
+++ b/drivers/media/pci/saa7134/saa7134-video.c
@@ -1176,14 +1176,6 @@ int saa7134_s_ctrl_internal(struct saa7134_dev *dev,  struct saa7134_fh *fh, str
 	int restart_overlay = 0;
 	int err;
 
-	/* When called from the empress code fh == NULL.
-	   That needs to be fixed somehow, but for now this is
-	   good enough. */
-	if (fh) {
-		err = v4l2_prio_check(&dev->prio, fh->prio);
-		if (0 != err)
-			return err;
-	}
 	err = -EINVAL;
 
 	mutex_lock(&dev->lock);
@@ -1352,6 +1344,7 @@ static int video_open(struct file *file)
 	if (NULL == fh)
 		return -ENOMEM;
 
+	v4l2_fh_init(&fh->fh, vdev);
 	file->private_data = fh;
 	fh->dev      = dev;
 	fh->radio    = radio;
@@ -1359,7 +1352,6 @@ static int video_open(struct file *file)
 	fh->fmt      = format_by_fourcc(V4L2_PIX_FMT_BGR24);
 	fh->width    = 720;
 	fh->height   = 576;
-	v4l2_prio_open(&dev->prio, &fh->prio);
 
 	videobuf_queue_sg_init(&fh->cap, &video_qops,
 			    &dev->pci->dev, &dev->slock,
@@ -1384,6 +1376,8 @@ static int video_open(struct file *file)
 		/* switch to video/vbi mode */
 		video_mux(dev,dev->ctl_input);
 	}
+	v4l2_fh_add(&fh->fh);
+
 	return 0;
 }
 
@@ -1504,7 +1498,8 @@ static int video_release(struct file *file)
 	saa7134_pgtable_free(dev->pci,&fh->pt_cap);
 	saa7134_pgtable_free(dev->pci,&fh->pt_vbi);
 
-	v4l2_prio_close(&dev->prio, fh->prio);
+	v4l2_fh_del(&fh->fh);
+	v4l2_fh_exit(&fh->fh);
 	file->private_data = NULL;
 	kfree(fh);
 	return 0;
@@ -1784,11 +1779,6 @@ static int saa7134_s_input(struct file *file, void *priv, unsigned int i)
 {
 	struct saa7134_fh *fh = priv;
 	struct saa7134_dev *dev = fh->dev;
-	int err;
-
-	err = v4l2_prio_check(&dev->prio, fh->prio);
-	if (0 != err)
-		return err;
 
 	if (i >= SAA7134_INPUT_MAX)
 		return -EINVAL;
@@ -1853,16 +1843,8 @@ int saa7134_s_std_internal(struct saa7134_dev *dev, struct saa7134_fh *fh, v4l2_
 	unsigned long flags;
 	unsigned int i;
 	v4l2_std_id fixup;
-	int err;
 
-	/* When called from the empress code fh == NULL.
-	   That needs to be fixed somehow, but for now this is
-	   good enough. */
-	if (fh) {
-		err = v4l2_prio_check(&dev->prio, fh->prio);
-		if (0 != err)
-			return err;
-	} else if (res_locked(dev, RESOURCE_OVERLAY)) {
+	if (!fh && res_locked(dev, RESOURCE_OVERLAY)) {
 		/* Don't change the std from the mpeg device
 		   if overlay is active. */
 		return -EBUSY;
@@ -2047,11 +2029,7 @@ static int saa7134_s_tuner(struct file *file, void *priv,
 {
 	struct saa7134_fh *fh = priv;
 	struct saa7134_dev *dev = fh->dev;
-	int rx, mode, err;
-
-	err = v4l2_prio_check(&dev->prio, fh->prio);
-	if (0 != err)
-		return err;
+	int rx, mode;
 
 	mode = dev->thread.mode;
 	if (UNSET == mode) {
@@ -2081,11 +2059,6 @@ static int saa7134_s_frequency(struct file *file, void *priv,
 {
 	struct saa7134_fh *fh = priv;
 	struct saa7134_dev *dev = fh->dev;
-	int err;
-
-	err = v4l2_prio_check(&dev->prio, fh->prio);
-	if (0 != err)
-		return err;
 
 	if (0 != f->tuner)
 		return -EINVAL;
@@ -2114,24 +2087,6 @@ static int saa7134_s_audio(struct file *file, void *priv, const struct v4l2_audi
 	return 0;
 }
 
-static int saa7134_g_priority(struct file *file, void *f, enum v4l2_priority *p)
-{
-	struct saa7134_fh *fh = f;
-	struct saa7134_dev *dev = fh->dev;
-
-	*p = v4l2_prio_max(&dev->prio);
-	return 0;
-}
-
-static int saa7134_s_priority(struct file *file, void *f,
-					enum v4l2_priority prio)
-{
-	struct saa7134_fh *fh = f;
-	struct saa7134_dev *dev = fh->dev;
-
-	return v4l2_prio_change(&dev->prio, &fh->prio, prio);
-}
-
 static int saa7134_enum_fmt_vid_cap(struct file *file, void  *priv,
 					struct v4l2_fmtdesc *f)
 {
@@ -2460,8 +2415,6 @@ static const struct v4l2_ioctl_ops video_ioctl_ops = {
 	.vidioc_g_fbuf			= saa7134_g_fbuf,
 	.vidioc_s_fbuf			= saa7134_s_fbuf,
 	.vidioc_overlay			= saa7134_overlay,
-	.vidioc_g_priority		= saa7134_g_priority,
-	.vidioc_s_priority		= saa7134_s_priority,
 	.vidioc_g_parm			= saa7134_g_parm,
 	.vidioc_g_frequency		= saa7134_g_frequency,
 	.vidioc_s_frequency		= saa7134_s_frequency,
diff --git a/drivers/media/pci/saa7134/saa7134.h b/drivers/media/pci/saa7134/saa7134.h
index 9059d08..2ffe069 100644
--- a/drivers/media/pci/saa7134/saa7134.h
+++ b/drivers/media/pci/saa7134/saa7134.h
@@ -35,6 +35,7 @@
 #include <media/v4l2-common.h>
 #include <media/v4l2-ioctl.h>
 #include <media/v4l2-device.h>
+#include <media/v4l2-fh.h>
 #include <media/tuner.h>
 #include <media/rc-core.h>
 #include <media/ir-kbd-i2c.h>
@@ -466,11 +467,11 @@ struct saa7134_dmaqueue {
 
 /* video filehandle status */
 struct saa7134_fh {
+	struct v4l2_fh             fh;
 	struct saa7134_dev         *dev;
 	unsigned int               radio;
 	enum v4l2_buf_type         type;
 	unsigned int               resources;
-	enum v4l2_priority	   prio;
 
 	/* video overlay */
 	struct v4l2_window         win;
@@ -541,7 +542,6 @@ struct saa7134_dev {
 	struct list_head           devlist;
 	struct mutex               lock;
 	spinlock_t                 slock;
-	struct v4l2_prio_state     prio;
 	struct v4l2_device         v4l2_dev;
 	/* workstruct for loading modules */
 	struct work_struct request_module_wk;
-- 
Ondrej Zary

[PATCH 4/7] saa7134: v4l2-compliance: return real frequency

[Ondrej Zary]

Make saa7134 driver more V4L2 compliant: don't cache frequency in
s_frequency/g_frequency but return real one instead

Signed-off-by: Ondrej Zary <linux@rainbow-software.org>
---
 drivers/media/pci/saa7134/saa7134-video.c |    6 ++++--
 drivers/media/pci/saa7134/saa7134.h       |    1 -
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/media/pci/saa7134/saa7134-video.c b/drivers/media/pci/saa7134/saa7134-video.c
index be745c0..87b2b9e 100644
--- a/drivers/media/pci/saa7134/saa7134-video.c
+++ b/drivers/media/pci/saa7134/saa7134-video.c
@@ -2048,8 +2048,11 @@ static int saa7134_g_frequency(struct file *file, void *priv,
 	struct saa7134_fh *fh = priv;
 	struct saa7134_dev *dev = fh->dev;
 
+	if (0 != f->tuner)
+		return -EINVAL;
+
 	f->type = fh->radio ? V4L2_TUNER_RADIO : V4L2_TUNER_ANALOG_TV;
-	f->frequency = dev->ctl_freq;
+	saa_call_all(dev, tuner, g_frequency, f);
 
 	return 0;
 }
@@ -2067,7 +2070,6 @@ static int saa7134_s_frequency(struct file *file, void *priv,
 	if (1 == fh->radio && V4L2_TUNER_RADIO != f->type)
 		return -EINVAL;
 	mutex_lock(&dev->lock);
-	dev->ctl_freq = f->frequency;
 
 	saa_call_all(dev, tuner, s_frequency, f);
 
diff --git a/drivers/media/pci/saa7134/saa7134.h b/drivers/media/pci/saa7134/saa7134.h
index 2ffe069..d0ee05e 100644
--- a/drivers/media/pci/saa7134/saa7134.h
+++ b/drivers/media/pci/saa7134/saa7134.h
@@ -604,7 +604,6 @@ struct saa7134_dev {
 	int                        ctl_contrast;
 	int                        ctl_hue;
 	int                        ctl_saturation;
-	int                        ctl_freq;
 	int                        ctl_mute;             /* audio */
 	int                        ctl_volume;
 	int                        ctl_invert;           /* private */
-- 
Ondrej Zary

[PATCH 5/7] saa7134: v4l2-compliance: fix g_tuner/s_tuner

[Ondrej Zary]

Make saa7134 driver more V4L2 compliant: return real frequency range in
g_tuner and fail in s_tuner for non-zero tuner

Signed-off-by: Ondrej Zary <linux@rainbow-software.org>
---
 drivers/media/pci/saa7134/saa7134-video.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/drivers/media/pci/saa7134/saa7134-video.c b/drivers/media/pci/saa7134/saa7134-video.c
index 87b2b9e..0b42f0c 100644
--- a/drivers/media/pci/saa7134/saa7134-video.c
+++ b/drivers/media/pci/saa7134/saa7134-video.c
@@ -2011,11 +2011,11 @@ static int saa7134_g_tuner(struct file *file, void *priv,
 	if (NULL != card_in(dev, n).name) {
 		strcpy(t->name, "Television");
 		t->type = V4L2_TUNER_ANALOG_TV;
+		saa_call_all(dev, tuner, g_tuner, t);
 		t->capability = V4L2_TUNER_CAP_NORM |
 			V4L2_TUNER_CAP_STEREO |
 			V4L2_TUNER_CAP_LANG1 |
 			V4L2_TUNER_CAP_LANG2;
-		t->rangehigh = 0xffffffffUL;
 		t->rxsubchans = saa7134_tvaudio_getstereo(dev);
 		t->audmode = saa7134_tvaudio_rx2mode(t->rxsubchans);
 	}
@@ -2031,6 +2031,9 @@ static int saa7134_s_tuner(struct file *file, void *priv,
 	struct saa7134_dev *dev = fh->dev;
 	int rx, mode;
 
+	if (0 != t->index)
+		return -EINVAL;
+
 	mode = dev->thread.mode;
 	if (UNSET == mode) {
 		rx   = saa7134_tvaudio_getstereo(dev);
-- 
Ondrej Zary

[PATCH 6/7] saa7134: v4l2-compliance: remove V4L2_IN_ST_NO_SYNC from enum_input

[Ondrej Zary]

Make saa7134 driver more V4L2 compliant: don't set bogus V4L2_IN_ST_NO_SYNC
flag in enum_input as it's for digital video only

Signed-off-by: Ondrej Zary <linux@rainbow-software.org>
---
 drivers/media/pci/saa7134/saa7134-video.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/drivers/media/pci/saa7134/saa7134-video.c b/drivers/media/pci/saa7134/saa7134-video.c
index 0b42f0c..fff6735 100644
--- a/drivers/media/pci/saa7134/saa7134-video.c
+++ b/drivers/media/pci/saa7134/saa7134-video.c
@@ -1757,8 +1757,6 @@ static int saa7134_enum_input(struct file *file, void *priv,
 
 		if (0 != (v1 & 0x40))
 			i->status |= V4L2_IN_ST_NO_H_LOCK;
-		if (0 != (v2 & 0x40))
-			i->status |= V4L2_IN_ST_NO_SYNC;
 		if (0 != (v2 & 0x0e))
 			i->status |= V4L2_IN_ST_MACROVISION;
 	}
-- 
Ondrej Zary

[PATCH 7/7] saa7134: v4l2-compliance: remove bogus audio input support

[Ondrej Zary]

Make saa7134 driver more V4L2 compliant: remove empty g_audio and s_audio
functions and don't set audioset in enum_input

Signed-off-by: Ondrej Zary <linux@rainbow-software.org>
---
 drivers/media/pci/saa7134/saa7134-video.c |   30 -----------------------------
 1 files changed, 0 insertions(+), 30 deletions(-)

diff --git a/drivers/media/pci/saa7134/saa7134-video.c b/drivers/media/pci/saa7134/saa7134-video.c
index fff6735..b63cdad 100644
--- a/drivers/media/pci/saa7134/saa7134-video.c
+++ b/drivers/media/pci/saa7134/saa7134-video.c
@@ -1750,7 +1750,6 @@ static int saa7134_enum_input(struct file *file, void *priv,
 	strcpy(i->name, card_in(dev, n).name);
 	if (card_in(dev, n).tv)
 		i->type = V4L2_INPUT_TYPE_TUNER;
-	i->audioset = 1;
 	if (n == dev->ctl_input) {
 		int v1 = saa_readb(SAA7134_STATUS_VIDEO1);
 		int v2 = saa_readb(SAA7134_STATUS_VIDEO2);
@@ -2079,17 +2078,6 @@ static int saa7134_s_frequency(struct file *file, void *priv,
 	return 0;
 }
 
-static int saa7134_g_audio(struct file *file, void *priv, struct v4l2_audio *a)
-{
-	strcpy(a->name, "audio");
-	return 0;
-}
-
-static int saa7134_s_audio(struct file *file, void *priv, const struct v4l2_audio *a)
-{
-	return 0;
-}
-
 static int saa7134_enum_fmt_vid_cap(struct file *file, void  *priv,
 					struct v4l2_fmtdesc *f)
 {
@@ -2330,20 +2318,6 @@ static int radio_g_input(struct file *filp, void *priv, unsigned int *i)
 	return 0;
 }
 
-static int radio_g_audio(struct file *file, void *priv,
-					struct v4l2_audio *a)
-{
-	memset(a, 0, sizeof(*a));
-	strcpy(a->name, "Radio");
-	return 0;
-}
-
-static int radio_s_audio(struct file *file, void *priv,
-					const struct v4l2_audio *a)
-{
-	return 0;
-}
-
 static int radio_s_input(struct file *filp, void *priv, unsigned int i)
 {
 	return 0;
@@ -2394,8 +2368,6 @@ static const struct v4l2_ioctl_ops video_ioctl_ops = {
 	.vidioc_g_fmt_vbi_cap		= saa7134_try_get_set_fmt_vbi_cap,
 	.vidioc_try_fmt_vbi_cap		= saa7134_try_get_set_fmt_vbi_cap,
 	.vidioc_s_fmt_vbi_cap		= saa7134_try_get_set_fmt_vbi_cap,
-	.vidioc_g_audio			= saa7134_g_audio,
-	.vidioc_s_audio			= saa7134_s_audio,
 	.vidioc_cropcap			= saa7134_cropcap,
 	.vidioc_reqbufs			= saa7134_reqbufs,
 	.vidioc_querybuf		= saa7134_querybuf,
@@ -2440,9 +2412,7 @@ static const struct v4l2_ioctl_ops radio_ioctl_ops = {
 	.vidioc_querycap	= saa7134_querycap,
 	.vidioc_g_tuner		= radio_g_tuner,
 	.vidioc_enum_input	= radio_enum_input,
-	.vidioc_g_audio		= radio_g_audio,
 	.vidioc_s_tuner		= radio_s_tuner,
-	.vidioc_s_audio		= radio_s_audio,
 	.vidioc_s_input		= radio_s_input,
 	.vidioc_s_std		= radio_s_std,
 	.vidioc_queryctrl	= radio_queryctrl,
-- 
Ondrej Zary

Re: [PATCH 1/7] saa7134: v4l2-compliance: implement V4L2_CAP_DEVICE_CAPS

[Hans Verkuil]

On Sun January 27 2013 20:45:06 Ondrej Zary wrote:
> Make saa7134 driver more V4L2 compliant: implement V4L2_CAP_DEVICE_CAPS support
> and fix all capabilities problems reported by v4l2-compliance.
> 
> Signed-off-by: Ondrej Zary <linux@rainbow-software.org>
> ---
>  drivers/media/pci/saa7134/saa7134-video.c |   55 ++++++++++++++++------------
>  1 files changed, 31 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/media/pci/saa7134/saa7134-video.c b/drivers/media/pci/saa7134/saa7134-video.c
> index 3abf527..ce15f1f 100644
> --- a/drivers/media/pci/saa7134/saa7134-video.c
> +++ b/drivers/media/pci/saa7134/saa7134-video.c
> @@ -1805,6 +1805,8 @@ static int saa7134_querycap(struct file *file, void  *priv,
>  {
>  	struct saa7134_fh *fh = priv;
>  	struct saa7134_dev *dev = fh->dev;
> +	struct video_device *vdev = video_devdata(file);
> +	u32 radio_caps, video_caps, vbi_caps;
>  
>  	unsigned int tuner_type = dev->tuner_type;
>  
> @@ -1812,19 +1814,37 @@ static int saa7134_querycap(struct file *file, void  *priv,
>  	strlcpy(cap->card, saa7134_boards[dev->board].name,
>  		sizeof(cap->card));
>  	sprintf(cap->bus_info, "PCI:%s", pci_name(dev->pci));
> -	cap->capabilities =
> -		V4L2_CAP_VIDEO_CAPTURE |
> -		V4L2_CAP_VBI_CAPTURE |
> -		V4L2_CAP_READWRITE |
> -		V4L2_CAP_STREAMING |
> -		V4L2_CAP_TUNER;
> +
> +	cap->device_caps = V4L2_CAP_READWRITE | V4L2_CAP_STREAMING;
> +	if ((tuner_type != TUNER_ABSENT) && (tuner_type != UNSET))
> +		cap->device_caps |= V4L2_CAP_TUNER;
> +
> +	radio_caps = V4L2_CAP_RADIO;
>  	if (dev->has_rds)
> -		cap->capabilities |= V4L2_CAP_RDS_CAPTURE;
> +		radio_caps |= V4L2_CAP_RDS_CAPTURE;
> +
> +	video_caps = V4L2_CAP_VIDEO_CAPTURE;
>  	if (saa7134_no_overlay <= 0)
> -		cap->capabilities |= V4L2_CAP_VIDEO_OVERLAY;
> +		video_caps |= V4L2_CAP_VIDEO_OVERLAY;
> +
> +	vbi_caps = V4L2_CAP_VBI_CAPTURE;
> +
> +	switch (vdev->vfl_type) {
> +	case VFL_TYPE_RADIO:
> +		cap->device_caps |= radio_caps;
> +		break;
> +	case VFL_TYPE_GRABBER:
> +		cap->device_caps |= video_caps;
> +		break;
> +	case VFL_TYPE_VBI:
> +		cap->device_caps |= vbi_caps;
> +		break;
> +	}
> +	cap->capabilities = radio_caps | video_caps | vbi_caps |
> +		cap->device_caps | V4L2_CAP_DEVICE_CAPS;
> +	if (vdev->vfl_type == VFL_TYPE_RADIO)
> +		cap->device_caps &= ~(V4L2_CAP_READWRITE | V4L2_CAP_STREAMING);

Not quite right: if has_rds is true, then V4L2_CAP_READWRITE is valid for a
radio device.

>  
> -	if ((tuner_type == TUNER_ABSENT) || (tuner_type == UNSET))
> -		cap->capabilities &= ~V4L2_CAP_TUNER;
>  	return 0;
>  }
>  
> @@ -2299,19 +2319,6 @@ static int vidioc_s_register (struct file *file, void *priv,
>  }
>  #endif
>  
> -static int radio_querycap(struct file *file, void *priv,
> -					struct v4l2_capability *cap)
> -{
> -	struct saa7134_fh *fh = file->private_data;
> -	struct saa7134_dev *dev = fh->dev;
> -
> -	strcpy(cap->driver, "saa7134");
> -	strlcpy(cap->card, saa7134_boards[dev->board].name, sizeof(cap->card));
> -	sprintf(cap->bus_info, "PCI:%s", pci_name(dev->pci));
> -	cap->capabilities = V4L2_CAP_TUNER;
> -	return 0;
> -}
> -
>  static int radio_g_tuner(struct file *file, void *priv,
>  					struct v4l2_tuner *t)
>  {
> @@ -2473,7 +2480,7 @@ static const struct v4l2_file_operations radio_fops = {
>  };
>  
>  static const struct v4l2_ioctl_ops radio_ioctl_ops = {
> -	.vidioc_querycap	= radio_querycap,
> +	.vidioc_querycap	= saa7134_querycap,
>  	.vidioc_g_tuner		= radio_g_tuner,
>  	.vidioc_enum_input	= radio_enum_input,
>  	.vidioc_g_audio		= radio_g_audio,
> 

Regards,

	Hans

Re: [PATCH 2/7] saa7134: v4l2-compliance: don't report invalid audio modes for radio

[Hans Verkuil]

On Sun January 27 2013 20:45:07 Ondrej Zary wrote:
> Make saa7134 driver more V4L2 compliant: filter audio modes that came from
> tuner - keep only MONO/STEREO in radio mode
> 
> Signed-off-by: Ondrej Zary <linux@rainbow-software.org>

Acked-by: Hans Verkuil <hans.verkuil@cisco.com>

> ---
>  drivers/media/pci/saa7134/saa7134-video.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/media/pci/saa7134/saa7134-video.c b/drivers/media/pci/saa7134/saa7134-video.c
> index ce15f1f..db8da32 100644
> --- a/drivers/media/pci/saa7134/saa7134-video.c
> +++ b/drivers/media/pci/saa7134/saa7134-video.c
> @@ -2333,6 +2333,7 @@ static int radio_g_tuner(struct file *file, void *priv,
>  	t->type = V4L2_TUNER_RADIO;
>  
>  	saa_call_all(dev, tuner, g_tuner, t);
> +	t->audmode &= V4L2_TUNER_MODE_MONO | V4L2_TUNER_MODE_STEREO;
>  	if (dev->input->amux == TV) {
>  		t->signal = 0xf800 - ((saa_readb(0x581) & 0x1f) << 11);
>  		t->rxsubchans = (saa_readb(0x529) & 0x08) ?
> 

Re: [PATCH 3/7] saa7134: v4l2-compliance: use v4l2_fh to fix priority handling

[Hans Verkuil]

On Sun January 27 2013 20:45:08 Ondrej Zary wrote:
> Make saa7134 driver more V4L2 compliant: remove broken priority handling
> and use v4l2_fh instead
> 
> Signed-off-by: Ondrej Zary <linux@rainbow-software.org>

Acked-by: Hans Verkuil <hans.verkuil@cisco.com>

Nice cleanup!

Regards,

	Hans

> ---
>  drivers/media/pci/saa7134/saa7134-core.c  |    3 +-
>  drivers/media/pci/saa7134/saa7134-video.c |   61 +++-------------------------
>  drivers/media/pci/saa7134/saa7134.h       |    4 +-
>  3 files changed, 10 insertions(+), 58 deletions(-)
> 
> diff --git a/drivers/media/pci/saa7134/saa7134-core.c b/drivers/media/pci/saa7134/saa7134-core.c
> index 8976d0e..ba08bd6 100644
> --- a/drivers/media/pci/saa7134/saa7134-core.c
> +++ b/drivers/media/pci/saa7134/saa7134-core.c
> @@ -805,6 +805,7 @@ static struct video_device *vdev_init(struct saa7134_dev *dev,
>  	vfd->debug   = video_debug;
>  	snprintf(vfd->name, sizeof(vfd->name), "%s %s (%s)",
>  		 dev->name, type, saa7134_boards[dev->board].name);
> +	set_bit(V4L2_FL_USE_FH_PRIO, &vfd->flags);
>  	video_set_drvdata(vfd, dev);
>  	return vfd;
>  }
> @@ -1028,8 +1029,6 @@ static int __devinit saa7134_initdev(struct pci_dev *pci_dev,
>  		}
>  	}
>  
> -	v4l2_prio_init(&dev->prio);
> -
>  	mutex_lock(&saa7134_devlist_lock);
>  	list_for_each_entry(mops, &mops_list, next)
>  		mpeg_ops_attach(mops, dev);
> diff --git a/drivers/media/pci/saa7134/saa7134-video.c b/drivers/media/pci/saa7134/saa7134-video.c
> index db8da32..be745c0 100644
> --- a/drivers/media/pci/saa7134/saa7134-video.c
> +++ b/drivers/media/pci/saa7134/saa7134-video.c
> @@ -1176,14 +1176,6 @@ int saa7134_s_ctrl_internal(struct saa7134_dev *dev,  struct saa7134_fh *fh, str
>  	int restart_overlay = 0;
>  	int err;
>  
> -	/* When called from the empress code fh == NULL.
> -	   That needs to be fixed somehow, but for now this is
> -	   good enough. */
> -	if (fh) {
> -		err = v4l2_prio_check(&dev->prio, fh->prio);
> -		if (0 != err)
> -			return err;
> -	}
>  	err = -EINVAL;
>  
>  	mutex_lock(&dev->lock);
> @@ -1352,6 +1344,7 @@ static int video_open(struct file *file)
>  	if (NULL == fh)
>  		return -ENOMEM;
>  
> +	v4l2_fh_init(&fh->fh, vdev);
>  	file->private_data = fh;
>  	fh->dev      = dev;
>  	fh->radio    = radio;
> @@ -1359,7 +1352,6 @@ static int video_open(struct file *file)
>  	fh->fmt      = format_by_fourcc(V4L2_PIX_FMT_BGR24);
>  	fh->width    = 720;
>  	fh->height   = 576;
> -	v4l2_prio_open(&dev->prio, &fh->prio);
>  
>  	videobuf_queue_sg_init(&fh->cap, &video_qops,
>  			    &dev->pci->dev, &dev->slock,
> @@ -1384,6 +1376,8 @@ static int video_open(struct file *file)
>  		/* switch to video/vbi mode */
>  		video_mux(dev,dev->ctl_input);
>  	}
> +	v4l2_fh_add(&fh->fh);
> +
>  	return 0;
>  }
>  
> @@ -1504,7 +1498,8 @@ static int video_release(struct file *file)
>  	saa7134_pgtable_free(dev->pci,&fh->pt_cap);
>  	saa7134_pgtable_free(dev->pci,&fh->pt_vbi);
>  
> -	v4l2_prio_close(&dev->prio, fh->prio);
> +	v4l2_fh_del(&fh->fh);
> +	v4l2_fh_exit(&fh->fh);
>  	file->private_data = NULL;
>  	kfree(fh);
>  	return 0;
> @@ -1784,11 +1779,6 @@ static int saa7134_s_input(struct file *file, void *priv, unsigned int i)
>  {
>  	struct saa7134_fh *fh = priv;
>  	struct saa7134_dev *dev = fh->dev;
> -	int err;
> -
> -	err = v4l2_prio_check(&dev->prio, fh->prio);
> -	if (0 != err)
> -		return err;
>  
>  	if (i >= SAA7134_INPUT_MAX)
>  		return -EINVAL;
> @@ -1853,16 +1843,8 @@ int saa7134_s_std_internal(struct saa7134_dev *dev, struct saa7134_fh *fh, v4l2_
>  	unsigned long flags;
>  	unsigned int i;
>  	v4l2_std_id fixup;
> -	int err;
>  
> -	/* When called from the empress code fh == NULL.
> -	   That needs to be fixed somehow, but for now this is
> -	   good enough. */
> -	if (fh) {
> -		err = v4l2_prio_check(&dev->prio, fh->prio);
> -		if (0 != err)
> -			return err;
> -	} else if (res_locked(dev, RESOURCE_OVERLAY)) {
> +	if (!fh && res_locked(dev, RESOURCE_OVERLAY)) {
>  		/* Don't change the std from the mpeg device
>  		   if overlay is active. */
>  		return -EBUSY;
> @@ -2047,11 +2029,7 @@ static int saa7134_s_tuner(struct file *file, void *priv,
>  {
>  	struct saa7134_fh *fh = priv;
>  	struct saa7134_dev *dev = fh->dev;
> -	int rx, mode, err;
> -
> -	err = v4l2_prio_check(&dev->prio, fh->prio);
> -	if (0 != err)
> -		return err;
> +	int rx, mode;
>  
>  	mode = dev->thread.mode;
>  	if (UNSET == mode) {
> @@ -2081,11 +2059,6 @@ static int saa7134_s_frequency(struct file *file, void *priv,
>  {
>  	struct saa7134_fh *fh = priv;
>  	struct saa7134_dev *dev = fh->dev;
> -	int err;
> -
> -	err = v4l2_prio_check(&dev->prio, fh->prio);
> -	if (0 != err)
> -		return err;
>  
>  	if (0 != f->tuner)
>  		return -EINVAL;
> @@ -2114,24 +2087,6 @@ static int saa7134_s_audio(struct file *file, void *priv, const struct v4l2_audi
>  	return 0;
>  }
>  
> -static int saa7134_g_priority(struct file *file, void *f, enum v4l2_priority *p)
> -{
> -	struct saa7134_fh *fh = f;
> -	struct saa7134_dev *dev = fh->dev;
> -
> -	*p = v4l2_prio_max(&dev->prio);
> -	return 0;
> -}
> -
> -static int saa7134_s_priority(struct file *file, void *f,
> -					enum v4l2_priority prio)
> -{
> -	struct saa7134_fh *fh = f;
> -	struct saa7134_dev *dev = fh->dev;
> -
> -	return v4l2_prio_change(&dev->prio, &fh->prio, prio);
> -}
> -
>  static int saa7134_enum_fmt_vid_cap(struct file *file, void  *priv,
>  					struct v4l2_fmtdesc *f)
>  {
> @@ -2460,8 +2415,6 @@ static const struct v4l2_ioctl_ops video_ioctl_ops = {
>  	.vidioc_g_fbuf			= saa7134_g_fbuf,
>  	.vidioc_s_fbuf			= saa7134_s_fbuf,
>  	.vidioc_overlay			= saa7134_overlay,
> -	.vidioc_g_priority		= saa7134_g_priority,
> -	.vidioc_s_priority		= saa7134_s_priority,
>  	.vidioc_g_parm			= saa7134_g_parm,
>  	.vidioc_g_frequency		= saa7134_g_frequency,
>  	.vidioc_s_frequency		= saa7134_s_frequency,
> diff --git a/drivers/media/pci/saa7134/saa7134.h b/drivers/media/pci/saa7134/saa7134.h
> index 9059d08..2ffe069 100644
> --- a/drivers/media/pci/saa7134/saa7134.h
> +++ b/drivers/media/pci/saa7134/saa7134.h
> @@ -35,6 +35,7 @@
>  #include <media/v4l2-common.h>
>  #include <media/v4l2-ioctl.h>
>  #include <media/v4l2-device.h>
> +#include <media/v4l2-fh.h>
>  #include <media/tuner.h>
>  #include <media/rc-core.h>
>  #include <media/ir-kbd-i2c.h>
> @@ -466,11 +467,11 @@ struct saa7134_dmaqueue {
>  
>  /* video filehandle status */
>  struct saa7134_fh {
> +	struct v4l2_fh             fh;
>  	struct saa7134_dev         *dev;
>  	unsigned int               radio;
>  	enum v4l2_buf_type         type;
>  	unsigned int               resources;
> -	enum v4l2_priority	   prio;
>  
>  	/* video overlay */
>  	struct v4l2_window         win;
> @@ -541,7 +542,6 @@ struct saa7134_dev {
>  	struct list_head           devlist;
>  	struct mutex               lock;
>  	spinlock_t                 slock;
> -	struct v4l2_prio_state     prio;
>  	struct v4l2_device         v4l2_dev;
>  	/* workstruct for loading modules */
>  	struct work_struct request_module_wk;
> 

Re: [PATCH 4/7] saa7134: v4l2-compliance: return real frequency

[Hans Verkuil]

On Sun January 27 2013 20:45:09 Ondrej Zary wrote:
> Make saa7134 driver more V4L2 compliant: don't cache frequency in
> s_frequency/g_frequency but return real one instead
> 
> Signed-off-by: Ondrej Zary <linux@rainbow-software.org>

Acked-by: Hans Verkuil <hans.verkuil@cisco.com>


> ---
>  drivers/media/pci/saa7134/saa7134-video.c |    6 ++++--
>  drivers/media/pci/saa7134/saa7134.h       |    1 -
>  2 files changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/pci/saa7134/saa7134-video.c b/drivers/media/pci/saa7134/saa7134-video.c
> index be745c0..87b2b9e 100644
> --- a/drivers/media/pci/saa7134/saa7134-video.c
> +++ b/drivers/media/pci/saa7134/saa7134-video.c
> @@ -2048,8 +2048,11 @@ static int saa7134_g_frequency(struct file *file, void *priv,
>  	struct saa7134_fh *fh = priv;
>  	struct saa7134_dev *dev = fh->dev;
>  
> +	if (0 != f->tuner)
> +		return -EINVAL;
> +
>  	f->type = fh->radio ? V4L2_TUNER_RADIO : V4L2_TUNER_ANALOG_TV;
> -	f->frequency = dev->ctl_freq;
> +	saa_call_all(dev, tuner, g_frequency, f);
>  
>  	return 0;
>  }
> @@ -2067,7 +2070,6 @@ static int saa7134_s_frequency(struct file *file, void *priv,
>  	if (1 == fh->radio && V4L2_TUNER_RADIO != f->type)
>  		return -EINVAL;
>  	mutex_lock(&dev->lock);
> -	dev->ctl_freq = f->frequency;
>  
>  	saa_call_all(dev, tuner, s_frequency, f);
>  
> diff --git a/drivers/media/pci/saa7134/saa7134.h b/drivers/media/pci/saa7134/saa7134.h
> index 2ffe069..d0ee05e 100644
> --- a/drivers/media/pci/saa7134/saa7134.h
> +++ b/drivers/media/pci/saa7134/saa7134.h
> @@ -604,7 +604,6 @@ struct saa7134_dev {
>  	int                        ctl_contrast;
>  	int                        ctl_hue;
>  	int                        ctl_saturation;
> -	int                        ctl_freq;
>  	int                        ctl_mute;             /* audio */
>  	int                        ctl_volume;
>  	int                        ctl_invert;           /* private */
> 

Re: [PATCH 5/7] saa7134: v4l2-compliance: fix g_tuner/s_tuner

[Hans Verkuil]

On Sun January 27 2013 20:45:10 Ondrej Zary wrote:
> Make saa7134 driver more V4L2 compliant: return real frequency range in
> g_tuner and fail in s_tuner for non-zero tuner
> 
> Signed-off-by: Ondrej Zary <linux@rainbow-software.org>

Acked-by: Hans Verkuil <hans.verkuil@cisco.com>


> ---
>  drivers/media/pci/saa7134/saa7134-video.c |    5 ++++-
>  1 files changed, 4 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/media/pci/saa7134/saa7134-video.c b/drivers/media/pci/saa7134/saa7134-video.c
> index 87b2b9e..0b42f0c 100644
> --- a/drivers/media/pci/saa7134/saa7134-video.c
> +++ b/drivers/media/pci/saa7134/saa7134-video.c
> @@ -2011,11 +2011,11 @@ static int saa7134_g_tuner(struct file *file, void *priv,
>  	if (NULL != card_in(dev, n).name) {
>  		strcpy(t->name, "Television");
>  		t->type = V4L2_TUNER_ANALOG_TV;
> +		saa_call_all(dev, tuner, g_tuner, t);
>  		t->capability = V4L2_TUNER_CAP_NORM |
>  			V4L2_TUNER_CAP_STEREO |
>  			V4L2_TUNER_CAP_LANG1 |
>  			V4L2_TUNER_CAP_LANG2;
> -		t->rangehigh = 0xffffffffUL;
>  		t->rxsubchans = saa7134_tvaudio_getstereo(dev);
>  		t->audmode = saa7134_tvaudio_rx2mode(t->rxsubchans);
>  	}
> @@ -2031,6 +2031,9 @@ static int saa7134_s_tuner(struct file *file, void *priv,
>  	struct saa7134_dev *dev = fh->dev;
>  	int rx, mode;
>  
> +	if (0 != t->index)
> +		return -EINVAL;
> +
>  	mode = dev->thread.mode;
>  	if (UNSET == mode) {
>  		rx   = saa7134_tvaudio_getstereo(dev);
> 

Re: [PATCH 6/7] saa7134: v4l2-compliance: remove V4L2_IN_ST_NO_SYNC from enum_input

[Mauro Carvalho Chehab]

Em Sun, 27 Jan 2013 20:45:11 +0100
Ondrej Zary <linux@rainbow-software.org> escreveu:

> Make saa7134 driver more V4L2 compliant: don't set bogus V4L2_IN_ST_NO_SYNC
> flag in enum_input as it's for digital video only
> 
> Signed-off-by: Ondrej Zary <linux@rainbow-software.org>
> ---
>  drivers/media/pci/saa7134/saa7134-video.c |    2 --
>  1 files changed, 0 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/pci/saa7134/saa7134-video.c b/drivers/media/pci/saa7134/saa7134-video.c
> index 0b42f0c..fff6735 100644
> --- a/drivers/media/pci/saa7134/saa7134-video.c
> +++ b/drivers/media/pci/saa7134/saa7134-video.c
> @@ -1757,8 +1757,6 @@ static int saa7134_enum_input(struct file *file, void *priv,
>  
>  		if (0 != (v1 & 0x40))
>  			i->status |= V4L2_IN_ST_NO_H_LOCK;
> -		if (0 != (v2 & 0x40))
> -			i->status |= V4L2_IN_ST_NO_SYNC;
>  		if (0 != (v2 & 0x0e))
>  			i->status |= V4L2_IN_ST_MACROVISION;
>  	}


Hmm... I'm not sure about this one. Very few drivers use those definitions,
but I suspect that this might potentially break some userspace applications.

It sounds more likely that v4l-compliance is doing the wrong thing here,
if it is complaining about that.

-- 

Cheers,
Mauro

Re: [PATCH 6/7] saa7134: v4l2-compliance: remove V4L2_IN_ST_NO_SYNC from enum_input

[Hans Verkuil]

On Sun January 27 2013 20:45:11 Ondrej Zary wrote:
> Make saa7134 driver more V4L2 compliant: don't set bogus V4L2_IN_ST_NO_SYNC
> flag in enum_input as it's for digital video only

I think there is a lot to be said for allowing this particular flag for analog
as well.

There are two types of flags here: flags relating the digital tuners (those
should not be used) and flags relating to sync issues. V4L2_IN_ST_NO_SYNC
is perfectly valid as a way to report sync problems.

I think a better solution is to allow this flag and to reorganize the 'Input
Status Flags' documentation:

- rename the "Analog Video" header to "Video Sync"
- rename the "Digital Video" header to "Digital Tuner (Deprecated)"
- move V4L2_IN_ST_NO_SYNC to the "Video Sync" section

Basically you have a number of video sync states:

1) NO_POWER: the device is off
2) NO_SIGNAL: the device is on, but there is no incoming signal
3) NO_SYNC: there is a signal, but we can't sync at all
4) NO_H_LOCK: there is a signal, we detect video lines, but we can't
   get a horizontal sync
5) NO_COLOR: there is no color signal at all
6) COLOR_KILL: we see a color signal, but we can't lock to it.

Note that not a single driver uses COLOR_KILL at the moment, but I know it
can be used.

If you can think of a way of improving the documentation w.r.t. these sync
status flags, then that would be great. Perhaps the table shouldn't be in
the order of the value but in a more logical order.

Regards,

	Hans

> Signed-off-by: Ondrej Zary <linux@rainbow-software.org>
> ---
>  drivers/media/pci/saa7134/saa7134-video.c |    2 --
>  1 files changed, 0 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/pci/saa7134/saa7134-video.c b/drivers/media/pci/saa7134/saa7134-video.c
> index 0b42f0c..fff6735 100644
> --- a/drivers/media/pci/saa7134/saa7134-video.c
> +++ b/drivers/media/pci/saa7134/saa7134-video.c
> @@ -1757,8 +1757,6 @@ static int saa7134_enum_input(struct file *file, void *priv,
>  
>  		if (0 != (v1 & 0x40))
>  			i->status |= V4L2_IN_ST_NO_H_LOCK;
> -		if (0 != (v2 & 0x40))
> -			i->status |= V4L2_IN_ST_NO_SYNC;
>  		if (0 != (v2 & 0x0e))
>  			i->status |= V4L2_IN_ST_MACROVISION;
>  	}
> 

Re: [PATCH 7/7] saa7134: v4l2-compliance: remove bogus audio input support

[Hans Verkuil]

On Sun January 27 2013 20:45:12 Ondrej Zary wrote:
> Make saa7134 driver more V4L2 compliant: remove empty g_audio and s_audio
> functions and don't set audioset in enum_input
> 
> Signed-off-by: Ondrej Zary <linux@rainbow-software.org>

Acked-by: Hans Verkuil <hans.verkuil@cisco.com>

> ---
>  drivers/media/pci/saa7134/saa7134-video.c |   30 -----------------------------
>  1 files changed, 0 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/media/pci/saa7134/saa7134-video.c b/drivers/media/pci/saa7134/saa7134-video.c
> index fff6735..b63cdad 100644
> --- a/drivers/media/pci/saa7134/saa7134-video.c
> +++ b/drivers/media/pci/saa7134/saa7134-video.c
> @@ -1750,7 +1750,6 @@ static int saa7134_enum_input(struct file *file, void *priv,
>  	strcpy(i->name, card_in(dev, n).name);
>  	if (card_in(dev, n).tv)
>  		i->type = V4L2_INPUT_TYPE_TUNER;
> -	i->audioset = 1;
>  	if (n == dev->ctl_input) {
>  		int v1 = saa_readb(SAA7134_STATUS_VIDEO1);
>  		int v2 = saa_readb(SAA7134_STATUS_VIDEO2);
> @@ -2079,17 +2078,6 @@ static int saa7134_s_frequency(struct file *file, void *priv,
>  	return 0;
>  }
>  
> -static int saa7134_g_audio(struct file *file, void *priv, struct v4l2_audio *a)
> -{
> -	strcpy(a->name, "audio");
> -	return 0;
> -}
> -
> -static int saa7134_s_audio(struct file *file, void *priv, const struct v4l2_audio *a)
> -{
> -	return 0;
> -}
> -
>  static int saa7134_enum_fmt_vid_cap(struct file *file, void  *priv,
>  					struct v4l2_fmtdesc *f)
>  {
> @@ -2330,20 +2318,6 @@ static int radio_g_input(struct file *filp, void *priv, unsigned int *i)
>  	return 0;
>  }
>  
> -static int radio_g_audio(struct file *file, void *priv,
> -					struct v4l2_audio *a)
> -{
> -	memset(a, 0, sizeof(*a));
> -	strcpy(a->name, "Radio");
> -	return 0;
> -}
> -
> -static int radio_s_audio(struct file *file, void *priv,
> -					const struct v4l2_audio *a)
> -{
> -	return 0;
> -}
> -
>  static int radio_s_input(struct file *filp, void *priv, unsigned int i)
>  {
>  	return 0;
> @@ -2394,8 +2368,6 @@ static const struct v4l2_ioctl_ops video_ioctl_ops = {
>  	.vidioc_g_fmt_vbi_cap		= saa7134_try_get_set_fmt_vbi_cap,
>  	.vidioc_try_fmt_vbi_cap		= saa7134_try_get_set_fmt_vbi_cap,
>  	.vidioc_s_fmt_vbi_cap		= saa7134_try_get_set_fmt_vbi_cap,
> -	.vidioc_g_audio			= saa7134_g_audio,
> -	.vidioc_s_audio			= saa7134_s_audio,
>  	.vidioc_cropcap			= saa7134_cropcap,
>  	.vidioc_reqbufs			= saa7134_reqbufs,
>  	.vidioc_querybuf		= saa7134_querybuf,
> @@ -2440,9 +2412,7 @@ static const struct v4l2_ioctl_ops radio_ioctl_ops = {
>  	.vidioc_querycap	= saa7134_querycap,
>  	.vidioc_g_tuner		= radio_g_tuner,
>  	.vidioc_enum_input	= radio_enum_input,
> -	.vidioc_g_audio		= radio_g_audio,
>  	.vidioc_s_tuner		= radio_s_tuner,
> -	.vidioc_s_audio		= radio_s_audio,
>  	.vidioc_s_input		= radio_s_input,
>  	.vidioc_s_std		= radio_s_std,
>  	.vidioc_queryctrl	= radio_queryctrl,
> 

Re: [RFC PATCH 0/7] saa7134: improve v4l2-compliance

[Hans Verkuil]

On Sun January 27 2013 20:45:05 Ondrej Zary wrote:
> Hello,
> this patch series improves v4l2-compliance of saa7134 driver. There are still
> some problems. Controls require conversion to control framework which I was
> unable to finish (because the driver accesses other controls and also the
> file handle from within s_ctrl).

To convert to the control framework this driver needs quite a bit of work:
the saa6752hs driver should be done first (and moved to media/i2c as well as
it really doesn't belong here).

The filehandle shouldn't be a problem, I think after the prio conversion that's
no longer needed at all.

> 
> Radio is now OK except for controls.
> Video has problems with controls, debugging, formats and buffers:
> Debug ioctls:
>         test VIDIOC_DBG_G_CHIP_IDENT: OK (Not Supported)
>                 fail: v4l2-test-debug.cpp(84): doioctl(node, VIDIOC_DBG_G_CHIP_IDENT, &chip)
> Format ioctls:
>         test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK
>                 fail: v4l2-test-formats.cpp(836): !cap->readbuffers

That should be easy to fix. It's a pretty bogus field and I usually set it to
the minimum number of buffers (which is 2 for this driver).

>         test VIDIOC_G/S_PARM: FAIL
>                 fail: v4l2-test-formats.cpp(335): !fmt.width || !fmt.height
>         test VIDIOC_G_FBUF: FAIL
>                 fail: v4l2-test-formats.cpp(382): !pix.colorspace

That's easy enough to solve. Typically this should be set to V4L2_COLORSPACE_SMPTE170M.

But after solving this you'll probably get a bunch of other issues due to
a problem this driver shared with quite a few other related drivers: the
format state is stored in struct saa7134_fh instead of in the top-level
struct. These format states are all global and should never have been placed
in this struct.

In fact if I look at the fields in saa7134_fh then:

- radio and type can be removed (this info can be obtained from existing fields
  elsewhere)
- the fields win until pt_vbi should all be global fields
- I suspect resources and qos_request should also be global, but you would have
  to analyze that.

In fact, it is likely that the whole structure can be removed and only v4l2_fh
be used instead.

> Buffer ioctls:
>                 fail: v4l2-test-buffers.cpp(109): can_stream && !mmap_valid && !userptr_valid && !dmabuf_valid

Fixing this requires changing to vb2.

>         test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: FAIL
> 
> 
> Haven't looked into VBI yet:
> Format ioctls:
>                 fail: v4l2-test-formats.cpp(914): G/S_PARM is only allowed for video capture/output
>         test VIDIOC_G/S_PARM: FAIL
>                 fail: v4l2-test-formats.cpp(432): vbi.reserved not zeroed
>                 fail: v4l2-test-formats.cpp(570): VBI Capture is valid, but TRY_FMT failed to return a format
>         test VIDIOC_TRY_FMT: FAIL
>                 fail: v4l2-test-formats.cpp(432): vbi.reserved not zeroed
>                 fail: v4l2-test-formats.cpp(728): VBI Capture is valid, but no S_FMT was implemented
>         test VIDIOC_S_FMT: FAIL

These shouldn't be hard to fix.

Regards,

	Hans

Re: [PATCH 6/7] saa7134: v4l2-compliance: remove V4L2_IN_ST_NO_SYNC from enum_input

[Hans Verkuil]

On Mon January 28 2013 11:30:46 Mauro Carvalho Chehab wrote:
> Em Sun, 27 Jan 2013 20:45:11 +0100
> Ondrej Zary <linux@rainbow-software.org> escreveu:
> 
> > Make saa7134 driver more V4L2 compliant: don't set bogus V4L2_IN_ST_NO_SYNC
> > flag in enum_input as it's for digital video only
> > 
> > Signed-off-by: Ondrej Zary <linux@rainbow-software.org>
> > ---
> >  drivers/media/pci/saa7134/saa7134-video.c |    2 --
> >  1 files changed, 0 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/media/pci/saa7134/saa7134-video.c b/drivers/media/pci/saa7134/saa7134-video.c
> > index 0b42f0c..fff6735 100644
> > --- a/drivers/media/pci/saa7134/saa7134-video.c
> > +++ b/drivers/media/pci/saa7134/saa7134-video.c
> > @@ -1757,8 +1757,6 @@ static int saa7134_enum_input(struct file *file, void *priv,
> >  
> >  		if (0 != (v1 & 0x40))
> >  			i->status |= V4L2_IN_ST_NO_H_LOCK;
> > -		if (0 != (v2 & 0x40))
> > -			i->status |= V4L2_IN_ST_NO_SYNC;
> >  		if (0 != (v2 & 0x0e))
> >  			i->status |= V4L2_IN_ST_MACROVISION;
> >  	}
> 
> 
> Hmm... I'm not sure about this one. Very few drivers use those definitions,
> but I suspect that this might potentially break some userspace applications.
> 
> It sounds more likely that v4l-compliance is doing the wrong thing here,
> if it is complaining about that.

I agree. See my reply to this patch. I'd appreciate your input on that.

Regards,

	Hans

[PATCH 8/7] saa7134: v4l2-compliance: remove bogus g_parm

[Ondrej Zary]

Make saa7134 driver more V4L2 compliant: remove empty g_parm function

Signed-off-by: Ondrej Zary <linux@rainbow-software.org>
---
 drivers/media/pci/saa7134/saa7134-video.c |    7 -------
 1 files changed, 0 insertions(+), 7 deletions(-)

diff --git a/drivers/media/pci/saa7134/saa7134-video.c 
b/drivers/media/pci/saa7134/saa7134-video.c
index b63cdad..3e88041 100644
--- a/drivers/media/pci/saa7134/saa7134-video.c
+++ b/drivers/media/pci/saa7134/saa7134-video.c
@@ -2232,12 +2232,6 @@ static int saa7134_streamoff(struct file *file, void 
*priv,
 	return 0;
 }
 
-static int saa7134_g_parm(struct file *file, void *fh,
-				struct v4l2_streamparm *parm)
-{
-	return 0;
-}
-
 #ifdef CONFIG_VIDEO_ADV_DEBUG
 static int vidioc_g_register (struct file *file, void *priv,
 			      struct v4l2_dbg_register *reg)
@@ -2390,7 +2384,6 @@ static const struct v4l2_ioctl_ops video_ioctl_ops = {
 	.vidioc_g_fbuf			= saa7134_g_fbuf,
 	.vidioc_s_fbuf			= saa7134_s_fbuf,
 	.vidioc_overlay			= saa7134_overlay,
-	.vidioc_g_parm			= saa7134_g_parm,
 	.vidioc_g_frequency		= saa7134_g_frequency,
 	.vidioc_s_frequency		= saa7134_s_frequency,
 #ifdef CONFIG_VIDEO_ADV_DEBUG
-- 
Ondrej Zary

[PATCH 9/7] saa7134: v4l2-compliance: initialize VBI structure

[Ondrej Zary]

Make saa7134 driver more V4L2 compliant: clear VBI structure completely
before assigning values to make sure any reserved space is cleared

Signed-off-by: Ondrej Zary <linux@rainbow-software.org>
---
 drivers/media/pci/saa7134/saa7134-video.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/media/pci/saa7134/saa7134-video.c 
b/drivers/media/pci/saa7134/saa7134-video.c
index 3e88041..adb83b5 100644
--- a/drivers/media/pci/saa7134/saa7134-video.c
+++ b/drivers/media/pci/saa7134/saa7134-video.c
@@ -1552,6 +1552,7 @@ static int saa7134_try_get_set_fmt_vbi_cap(struct file 
*file, void *priv,
 	struct saa7134_dev *dev = fh->dev;
 	struct saa7134_tvnorm *norm = dev->tvnorm;
 
+	memset(&f->fmt.vbi, 0, sizeof(f->fmt.vbi));
 	f->fmt.vbi.sampling_rate = 6750000 * 4;
 	f->fmt.vbi.samples_per_line = 2048 /* VBI_LINE_LENGTH */;
 	f->fmt.vbi.sample_format = V4L2_PIX_FMT_GREY;
-- 
Ondrej Zary

Re: [PATCH 8/7] saa7134: v4l2-compliance: remove bogus g_parm

[Hans Verkuil]

On Mon January 28 2013 15:11:50 Ondrej Zary wrote:
> Make saa7134 driver more V4L2 compliant: remove empty g_parm function
> 
> Signed-off-by: Ondrej Zary <linux@rainbow-software.org>

Acked-by: Hans Verkuil <hans.verkuil@cisco.com>

> ---
>  drivers/media/pci/saa7134/saa7134-video.c |    7 -------
>  1 files changed, 0 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/media/pci/saa7134/saa7134-video.c 
> b/drivers/media/pci/saa7134/saa7134-video.c
> index b63cdad..3e88041 100644
> --- a/drivers/media/pci/saa7134/saa7134-video.c
> +++ b/drivers/media/pci/saa7134/saa7134-video.c
> @@ -2232,12 +2232,6 @@ static int saa7134_streamoff(struct file *file, void 
> *priv,
>  	return 0;
>  }
>  
> -static int saa7134_g_parm(struct file *file, void *fh,
> -				struct v4l2_streamparm *parm)
> -{
> -	return 0;
> -}
> -
>  #ifdef CONFIG_VIDEO_ADV_DEBUG
>  static int vidioc_g_register (struct file *file, void *priv,
>  			      struct v4l2_dbg_register *reg)
> @@ -2390,7 +2384,6 @@ static const struct v4l2_ioctl_ops video_ioctl_ops = {
>  	.vidioc_g_fbuf			= saa7134_g_fbuf,
>  	.vidioc_s_fbuf			= saa7134_s_fbuf,
>  	.vidioc_overlay			= saa7134_overlay,
> -	.vidioc_g_parm			= saa7134_g_parm,
>  	.vidioc_g_frequency		= saa7134_g_frequency,
>  	.vidioc_s_frequency		= saa7134_s_frequency,
>  #ifdef CONFIG_VIDEO_ADV_DEBUG
> 

Re: [PATCH 9/7] saa7134: v4l2-compliance: initialize VBI structure

[Hans Verkuil]

On Mon January 28 2013 15:11:53 Ondrej Zary wrote:
> Make saa7134 driver more V4L2 compliant: clear VBI structure completely
> before assigning values to make sure any reserved space is cleared
> 
> Signed-off-by: Ondrej Zary <linux@rainbow-software.org>
> ---
>  drivers/media/pci/saa7134/saa7134-video.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/media/pci/saa7134/saa7134-video.c 
> b/drivers/media/pci/saa7134/saa7134-video.c
> index 3e88041..adb83b5 100644
> --- a/drivers/media/pci/saa7134/saa7134-video.c
> +++ b/drivers/media/pci/saa7134/saa7134-video.c
> @@ -1552,6 +1552,7 @@ static int saa7134_try_get_set_fmt_vbi_cap(struct file 
> *file, void *priv,
>  	struct saa7134_dev *dev = fh->dev;
>  	struct saa7134_tvnorm *norm = dev->tvnorm;
>  
> +	memset(&f->fmt.vbi, 0, sizeof(f->fmt.vbi));

I prefer:

	memset(&f->fmt.vbi.reserved, 0, sizeof(f->fmt.vbi.reserved));

After all, that's the only thing that needs clearing.

Regards,

	Hans

>  	f->fmt.vbi.sampling_rate = 6750000 * 4;
>  	f->fmt.vbi.samples_per_line = 2048 /* VBI_LINE_LENGTH */;
>  	f->fmt.vbi.sample_format = V4L2_PIX_FMT_GREY;
> 

Re: [RFC PATCH 0/7] saa7134: improve v4l2-compliance

[Ondrej Zary]

On Monday 28 January 2013 11:56:59 Hans Verkuil wrote:
> On Sun January 27 2013 20:45:05 Ondrej Zary wrote:
> > Hello,
> > this patch series improves v4l2-compliance of saa7134 driver. There are
> > still some problems. Controls require conversion to control framework
> > which I was unable to finish (because the driver accesses other controls
> > and also the file handle from within s_ctrl).
>
> To convert to the control framework this driver needs quite a bit of work:
> the saa6752hs driver should be done first (and moved to media/i2c as well
> as it really doesn't belong here).
>
> The filehandle shouldn't be a problem, I think after the prio conversion
> that's no longer needed at all.
>
> > Radio is now OK except for controls.
> > Video has problems with controls, debugging, formats and buffers:
> > Debug ioctls:
> >         test VIDIOC_DBG_G_CHIP_IDENT: OK (Not Supported)
> >                 fail: v4l2-test-debug.cpp(84): doioctl(node,
> > VIDIOC_DBG_G_CHIP_IDENT, &chip) Format ioctls:
> >         test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK
> >                 fail: v4l2-test-formats.cpp(836): !cap->readbuffers
>
> That should be easy to fix. It's a pretty bogus field and I usually set it
> to the minimum number of buffers (which is 2 for this driver).
>
> >         test VIDIOC_G/S_PARM: FAIL
> >                 fail: v4l2-test-formats.cpp(335): !fmt.width ||
> > !fmt.height test VIDIOC_G_FBUF: FAIL
> >                 fail: v4l2-test-formats.cpp(382): !pix.colorspace
>
> That's easy enough to solve. Typically this should be set to
> V4L2_COLORSPACE_SMPTE170M.
>
> But after solving this you'll probably get a bunch of other issues due to
> a problem this driver shared with quite a few other related drivers: the
> format state is stored in struct saa7134_fh instead of in the top-level
> struct. These format states are all global and should never have been
> placed in this struct.

Got this after setting colorspace:
               fail: v4l2-test-formats.cpp(460): win.field == V4L2_FIELD_ANY

I don't know what win.field is supposed to be and where the value should came 
from. It's now taken from fh->win which is probably all zeros because no 
overlay is running?
The same problem is with g_fbuf - what should fmt.width and fmt.height be?

> In fact if I look at the fields in saa7134_fh then:
>
> - radio and type can be removed (this info can be obtained from existing
> fields elsewhere)
> - the fields win until pt_vbi should all be global fields
> - I suspect resources and qos_request should also be global, but you would
> have to analyze that.

There are two "resources" variables - one in saa7134_fh and one in 
saa7134_dev. They're used for some kind of double-locking (global and per 
file handle).

> In fact, it is likely that the whole structure can be removed and only
> v4l2_fh be used instead.

-- 
Ondrej Zary

Re: [RFC PATCH 0/7] saa7134: improve v4l2-compliance

[Hans Verkuil]

On Mon January 28 2013 23:40:59 Ondrej Zary wrote:
> On Monday 28 January 2013 11:56:59 Hans Verkuil wrote:
> > On Sun January 27 2013 20:45:05 Ondrej Zary wrote:
> > > Hello,
> > > this patch series improves v4l2-compliance of saa7134 driver. There are
> > > still some problems. Controls require conversion to control framework
> > > which I was unable to finish (because the driver accesses other controls
> > > and also the file handle from within s_ctrl).
> >
> > To convert to the control framework this driver needs quite a bit of work:
> > the saa6752hs driver should be done first (and moved to media/i2c as well
> > as it really doesn't belong here).
> >
> > The filehandle shouldn't be a problem, I think after the prio conversion
> > that's no longer needed at all.
> >
> > > Radio is now OK except for controls.
> > > Video has problems with controls, debugging, formats and buffers:
> > > Debug ioctls:
> > >         test VIDIOC_DBG_G_CHIP_IDENT: OK (Not Supported)
> > >                 fail: v4l2-test-debug.cpp(84): doioctl(node,
> > > VIDIOC_DBG_G_CHIP_IDENT, &chip) Format ioctls:
> > >         test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK
> > >                 fail: v4l2-test-formats.cpp(836): !cap->readbuffers
> >
> > That should be easy to fix. It's a pretty bogus field and I usually set it
> > to the minimum number of buffers (which is 2 for this driver).
> >
> > >         test VIDIOC_G/S_PARM: FAIL
> > >                 fail: v4l2-test-formats.cpp(335): !fmt.width ||
> > > !fmt.height test VIDIOC_G_FBUF: FAIL
> > >                 fail: v4l2-test-formats.cpp(382): !pix.colorspace
> >
> > That's easy enough to solve. Typically this should be set to
> > V4L2_COLORSPACE_SMPTE170M.
> >
> > But after solving this you'll probably get a bunch of other issues due to
> > a problem this driver shared with quite a few other related drivers: the
> > format state is stored in struct saa7134_fh instead of in the top-level
> > struct. These format states are all global and should never have been
> > placed in this struct.
> 
> Got this after setting colorspace:
>                fail: v4l2-test-formats.cpp(460): win.field == V4L2_FIELD_ANY
> 
> I don't know what win.field is supposed to be and where the value should came 
> from.

>From the spec:

"Applications set this field to determine which video field shall be overlaid,
typically one of V4L2_FIELD_ANY (0), V4L2_FIELD_TOP, V4L2_FIELD_BOTTOM or
V4L2_FIELD_INTERLACED. Drivers may have to choose a different field order
and return the actual setting here."

For this driver V4L2_FIELD_INTERLACED is almost certainly what should be
returned.

> It's now taken from fh->win which is probably all zeros because no 
> overlay is running?
> The same problem is with g_fbuf - what should fmt.width and fmt.height be?

The main problem here is that the win struct is in the filehandle. It should
be in a global struct and set to default values (usually the same size as the
current STD).

Regards,

	Hans