[PATCH RFC v2 0/7] V4L2 SDR API

[PATCH RFC v2 0/7] V4L2 SDR API

[Antti Palosaari]

Some changes done suggested by Hans.

regards
Antti

Antti Palosaari (7):
  v4l: don't clear VIDIOC_G_FREQUENCY tuner type
  v4l: add device type for Software Defined Radio
  v4l: add new tuner types for SDR
  v4l: 1 Hz resolution flag for tuners
  v4l: add stream format for SDR receiver
  v4l: enable some IOCTLs for SDR receiver
  v4l: define own IOCTL ops for SDR FMT

 drivers/media/v4l2-core/v4l2-dev.c   | 32 +++++++++++++--
 drivers/media/v4l2-core/v4l2-ioctl.c | 80 ++++++++++++++++++++++++++++++------
 include/media/v4l2-dev.h             |  3 +-
 include/media/v4l2-ioctl.h           |  8 ++++
 include/trace/events/v4l2.h          |  1 +
 include/uapi/linux/videodev2.h       | 14 +++++++
 6 files changed, 122 insertions(+), 16 deletions(-)

-- 
1.8.4.2

[PATCH RFC v2 1/7] v4l: don't clear VIDIOC_G_FREQUENCY tuner type

[Antti Palosaari]

No need to clear as it will be overridden in every case when
v4l_g_frequency() is called. We will need that value later when
new tuner types are defined.

Cc: Hans Verkuil <hverkuil@xs4all.nl>
Signed-off-by: Antti Palosaari <crope@iki.fi>
---
 drivers/media/v4l2-core/v4l2-ioctl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
index 68e6b5e..bc10684 100644
--- a/drivers/media/v4l2-core/v4l2-ioctl.c
+++ b/drivers/media/v4l2-core/v4l2-ioctl.c
@@ -2013,7 +2013,7 @@ static struct v4l2_ioctl_info v4l2_ioctls[] = {
 	IOCTL_INFO_STD(VIDIOC_S_AUDOUT, vidioc_s_audout, v4l_print_audioout, INFO_FL_PRIO),
 	IOCTL_INFO_FNC(VIDIOC_G_MODULATOR, v4l_g_modulator, v4l_print_modulator, INFO_FL_CLEAR(v4l2_modulator, index)),
 	IOCTL_INFO_STD(VIDIOC_S_MODULATOR, vidioc_s_modulator, v4l_print_modulator, INFO_FL_PRIO),
-	IOCTL_INFO_FNC(VIDIOC_G_FREQUENCY, v4l_g_frequency, v4l_print_frequency, INFO_FL_CLEAR(v4l2_frequency, tuner)),
+	IOCTL_INFO_FNC(VIDIOC_G_FREQUENCY, v4l_g_frequency, v4l_print_frequency, INFO_FL_CLEAR(v4l2_frequency, type)),
 	IOCTL_INFO_FNC(VIDIOC_S_FREQUENCY, v4l_s_frequency, v4l_print_frequency, INFO_FL_PRIO),
 	IOCTL_INFO_FNC(VIDIOC_CROPCAP, v4l_cropcap, v4l_print_cropcap, INFO_FL_CLEAR(v4l2_cropcap, type)),
 	IOCTL_INFO_FNC(VIDIOC_G_CROP, v4l_g_crop, v4l_print_crop, INFO_FL_CLEAR(v4l2_crop, type)),
-- 
1.8.4.2

[PATCH RFC v2 2/7] v4l: add device type for Software Defined Radio

[Antti Palosaari]

Add new V4L device type VFL_TYPE_SDR for Software Defined Radio.
It is registered as /dev/sdr0

Cc: Hans Verkuil <hverkuil@xs4all.nl>
Signed-off-by: Antti Palosaari <crope@iki.fi>
---
 drivers/media/v4l2-core/v4l2-dev.c | 5 +++++
 include/media/v4l2-dev.h           | 3 ++-
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
index 1cc1749..c9cf54c 100644
--- a/drivers/media/v4l2-core/v4l2-dev.c
+++ b/drivers/media/v4l2-core/v4l2-dev.c
@@ -767,6 +767,8 @@ static void determine_valid_ioctls(struct video_device *vdev)
  *	%VFL_TYPE_RADIO - A radio card
  *
  *	%VFL_TYPE_SUBDEV - A subdevice
+ *
+ *	%VFL_TYPE_SDR - Software Defined Radio
  */
 int __video_register_device(struct video_device *vdev, int type, int nr,
 		int warn_if_nr_in_use, struct module *owner)
@@ -806,6 +808,9 @@ int __video_register_device(struct video_device *vdev, int type, int nr,
 	case VFL_TYPE_SUBDEV:
 		name_base = "v4l-subdev";
 		break;
+	case VFL_TYPE_SDR:
+		name_base = "sdr";
+		break;
 	default:
 		printk(KERN_ERR "%s called with unknown type: %d\n",
 		       __func__, type);
diff --git a/include/media/v4l2-dev.h b/include/media/v4l2-dev.h
index c768c9f..eec6e46 100644
--- a/include/media/v4l2-dev.h
+++ b/include/media/v4l2-dev.h
@@ -24,7 +24,8 @@
 #define VFL_TYPE_VBI		1
 #define VFL_TYPE_RADIO		2
 #define VFL_TYPE_SUBDEV		3
-#define VFL_TYPE_MAX		4
+#define VFL_TYPE_SDR		4
+#define VFL_TYPE_MAX		5
 
 /* Is this a receiver, transmitter or mem-to-mem? */
 /* Ignored for VFL_TYPE_SUBDEV. */
-- 
1.8.4.2

[PATCH RFC v2 3/7] v4l: add new tuner types for SDR

[Antti Palosaari]

Define tuner types V4L2_TUNER_ADC and V4L2_TUNER_RF for SDR usage.

ADC is used for setting sampling rate (sampling frequency) to SDR
device.

Another tuner type, named as V4L2_TUNER_RF, is possible RF tuner.
Is is used to down-convert RF frequency to range ADC could sample.
Having RF tuner is optional, whilst in practice it is almost always
there.

Also add checks to VIDIOC_G_FREQUENCY, VIDIOC_S_FREQUENCY and
VIDIOC_ENUM_FREQ_BANDS only allow these two tuner types when device
type is SDR (VFL_TYPE_SDR).

Prohibit VIDIOC_S_HW_FREQ_SEEK explicitly when device type is SDR,
as device cannot do hardware seek without a hardware demodulator.

Cc: Hans Verkuil <hverkuil@xs4all.nl>
Signed-off-by: Antti Palosaari <crope@iki.fi>
---
 drivers/media/v4l2-core/v4l2-ioctl.c | 42 ++++++++++++++++++++++++++----------
 include/uapi/linux/videodev2.h       |  2 ++
 2 files changed, 33 insertions(+), 11 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
index bc10684..6b72bd8 100644
--- a/drivers/media/v4l2-core/v4l2-ioctl.c
+++ b/drivers/media/v4l2-core/v4l2-ioctl.c
@@ -1288,8 +1288,13 @@ static int v4l_g_frequency(const struct v4l2_ioctl_ops *ops,
 	struct video_device *vfd = video_devdata(file);
 	struct v4l2_frequency *p = arg;
 
-	p->type = (vfd->vfl_type == VFL_TYPE_RADIO) ?
-			V4L2_TUNER_RADIO : V4L2_TUNER_ANALOG_TV;
+	if (vfd->vfl_type == VFL_TYPE_SDR) {
+		if (p->type != V4L2_TUNER_ADC && p->type != V4L2_TUNER_RF)
+			return -EINVAL;
+	} else {
+		p->type = (vfd->vfl_type == VFL_TYPE_RADIO) ?
+				V4L2_TUNER_RADIO : V4L2_TUNER_ANALOG_TV;
+	}
 	return ops->vidioc_g_frequency(file, fh, p);
 }
 
@@ -1300,10 +1305,16 @@ static int v4l_s_frequency(const struct v4l2_ioctl_ops *ops,
 	const struct v4l2_frequency *p = arg;
 	enum v4l2_tuner_type type;
 
-	type = (vfd->vfl_type == VFL_TYPE_RADIO) ?
-			V4L2_TUNER_RADIO : V4L2_TUNER_ANALOG_TV;
-	if (p->type != type)
-		return -EINVAL;
+	if (vfd->vfl_type == VFL_TYPE_SDR) {
+		if (p->type != V4L2_TUNER_ADC && p->type != V4L2_TUNER_RF)
+			return -EINVAL;
+		type = p->type;
+	} else {
+		type = (vfd->vfl_type == VFL_TYPE_RADIO) ?
+				V4L2_TUNER_RADIO : V4L2_TUNER_ANALOG_TV;
+		if (type != p->type)
+			return -EINVAL;
+	}
 	return ops->vidioc_s_frequency(file, fh, p);
 }
 
@@ -1383,6 +1394,10 @@ static int v4l_s_hw_freq_seek(const struct v4l2_ioctl_ops *ops,
 	struct v4l2_hw_freq_seek *p = arg;
 	enum v4l2_tuner_type type;
 
+	/* s_hw_freq_seek is not supported for SDR for now */
+	if (vfd->vfl_type == VFL_TYPE_SDR)
+		return -EINVAL;
+
 	type = (vfd->vfl_type == VFL_TYPE_RADIO) ?
 		V4L2_TUNER_RADIO : V4L2_TUNER_ANALOG_TV;
 	if (p->type != type)
@@ -1882,11 +1897,16 @@ static int v4l_enum_freq_bands(const struct v4l2_ioctl_ops *ops,
 	enum v4l2_tuner_type type;
 	int err;
 
-	type = (vfd->vfl_type == VFL_TYPE_RADIO) ?
-			V4L2_TUNER_RADIO : V4L2_TUNER_ANALOG_TV;
-
-	if (type != p->type)
-		return -EINVAL;
+	if (vfd->vfl_type == VFL_TYPE_SDR) {
+		if (p->type != V4L2_TUNER_ADC && p->type != V4L2_TUNER_RF)
+			return -EINVAL;
+		type = p->type;
+	} else {
+		type = (vfd->vfl_type == VFL_TYPE_RADIO) ?
+				V4L2_TUNER_RADIO : V4L2_TUNER_ANALOG_TV;
+		if (type != p->type)
+			return -EINVAL;
+	}
 	if (ops->vidioc_enum_freq_bands)
 		return ops->vidioc_enum_freq_bands(file, fh, p);
 	if (is_valid_ioctl(vfd, VIDIOC_G_TUNER)) {
diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index b8ee9048..c3e7780 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -159,6 +159,8 @@ enum v4l2_tuner_type {
 	V4L2_TUNER_RADIO	     = 1,
 	V4L2_TUNER_ANALOG_TV	     = 2,
 	V4L2_TUNER_DIGITAL_TV	     = 3,
+	V4L2_TUNER_ADC               = 4,
+	V4L2_TUNER_RF                = 5,
 };
 
 enum v4l2_memory {
-- 
1.8.4.2

[PATCH RFC v2 4/7] v4l: 1 Hz resolution flag for tuners

[Antti Palosaari]

Add V4L2_TUNER_CAP_1HZ for 1 Hz resolution.

Cc: Hans Verkuil <hverkuil@xs4all.nl>
Signed-off-by: Antti Palosaari <crope@iki.fi>
---
 include/uapi/linux/videodev2.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index c3e7780..619f83f 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -1349,6 +1349,7 @@ struct v4l2_modulator {
 #define V4L2_TUNER_CAP_RDS_CONTROLS	0x0200
 #define V4L2_TUNER_CAP_FREQ_BANDS	0x0400
 #define V4L2_TUNER_CAP_HWSEEK_PROG_LIM	0x0800
+#define V4L2_TUNER_CAP_1HZ		0x1000
 
 /*  Flags for the 'rxsubchans' field */
 #define V4L2_TUNER_SUB_MONO		0x0001
-- 
1.8.4.2

[PATCH RFC v2 5/7] v4l: add stream format for SDR receiver

[Antti Palosaari]

Add new V4L2 stream format definition, V4L2_BUF_TYPE_SDR_CAPTURE,
for SDR receiver.

Cc: Hans Verkuil <hverkuil@xs4all.nl>
Signed-off-by: Antti Palosaari <crope@iki.fi>
---
 drivers/media/v4l2-core/v4l2-ioctl.c |  1 +
 include/trace/events/v4l2.h          |  1 +
 include/uapi/linux/videodev2.h       | 11 +++++++++++
 3 files changed, 13 insertions(+)

diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
index 6b72bd8..edb0a4c 100644
--- a/drivers/media/v4l2-core/v4l2-ioctl.c
+++ b/drivers/media/v4l2-core/v4l2-ioctl.c
@@ -149,6 +149,7 @@ const char *v4l2_type_names[] = {
 	[V4L2_BUF_TYPE_VIDEO_OUTPUT_OVERLAY] = "vid-out-overlay",
 	[V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE] = "vid-cap-mplane",
 	[V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE] = "vid-out-mplane",
+	[V4L2_BUF_TYPE_SDR_CAPTURE]        = "sdr-cap",
 };
 EXPORT_SYMBOL(v4l2_type_names);
 
diff --git a/include/trace/events/v4l2.h b/include/trace/events/v4l2.h
index ef94eca..b9bb1f2 100644
--- a/include/trace/events/v4l2.h
+++ b/include/trace/events/v4l2.h
@@ -18,6 +18,7 @@
 		{ V4L2_BUF_TYPE_VIDEO_OUTPUT_OVERLAY, "VIDEO_OUTPUT_OVERLAY" },\
 		{ V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE, "VIDEO_CAPTURE_MPLANE" },\
 		{ V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE,  "VIDEO_OUTPUT_MPLANE" }, \
+		{ V4L2_BUF_TYPE_SDR_CAPTURE,          "SDR_CAPTURE" },         \
 		{ V4L2_BUF_TYPE_PRIVATE,	      "PRIVATE" })
 
 #define show_field(field)						\
diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index 619f83f..52c570e 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -139,6 +139,7 @@ enum v4l2_buf_type {
 #endif
 	V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE = 9,
 	V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE  = 10,
+	V4L2_BUF_TYPE_SDR_CAPTURE          = 11,
 	/* Deprecated, do not use */
 	V4L2_BUF_TYPE_PRIVATE              = 0x80,
 };
@@ -1703,6 +1704,15 @@ struct v4l2_pix_format_mplane {
 } __attribute__ ((packed));
 
 /**
+ * struct v4l2_format_sdr - SDR format definition
+ * @pixelformat:	little endian four character code (fourcc)
+ */
+struct v4l2_format_sdr {
+	__u32				pixelformat;
+	__u8				reserved[28];
+} __attribute__ ((packed));
+
+/**
  * struct v4l2_format - stream data format
  * @type:	enum v4l2_buf_type; type of the data stream
  * @pix:	definition of an image format
@@ -1720,6 +1730,7 @@ struct v4l2_format {
 		struct v4l2_window		win;     /* V4L2_BUF_TYPE_VIDEO_OVERLAY */
 		struct v4l2_vbi_format		vbi;     /* V4L2_BUF_TYPE_VBI_CAPTURE */
 		struct v4l2_sliced_vbi_format	sliced;  /* V4L2_BUF_TYPE_SLICED_VBI_CAPTURE */
+		struct v4l2_format_sdr		sdr;     /* V4L2_BUF_TYPE_SDR_CAPTURE */
 		__u8	raw_data[200];                   /* user-defined */
 	} fmt;
 };
-- 
1.8.4.2

[PATCH RFC v2 6/7] v4l: enable some IOCTLs for SDR receiver

[Antti Palosaari]

Enable stream format (FMT) IOCTLs for SDR use. These are used for negotiate
used data stream format.

Enable input IOCTLs, VIDIOC_ENUMINPUT, VIDIOC_G_INPUT, VIDIOC_S_INPUT.
These are used to select possible antenna connector.

Reorganise some some IOCTL selection logic.

Cc: Hans Verkuil <hverkuil@xs4all.nl>
Signed-off-by: Antti Palosaari <crope@iki.fi>
---
 drivers/media/v4l2-core/v4l2-dev.c   | 27 ++++++++++++++++++++++++---
 drivers/media/v4l2-core/v4l2-ioctl.c | 35 +++++++++++++++++++++++++++++++++++
 2 files changed, 59 insertions(+), 3 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
index c9cf54c..9f15e25 100644
--- a/drivers/media/v4l2-core/v4l2-dev.c
+++ b/drivers/media/v4l2-core/v4l2-dev.c
@@ -562,7 +562,7 @@ static void determine_valid_ioctls(struct video_device *vdev)
 	const struct v4l2_ioctl_ops *ops = vdev->ioctl_ops;
 	bool is_vid = vdev->vfl_type == VFL_TYPE_GRABBER;
 	bool is_vbi = vdev->vfl_type == VFL_TYPE_VBI;
-	bool is_radio = vdev->vfl_type == VFL_TYPE_RADIO;
+	bool is_sdr = vdev->vfl_type == VFL_TYPE_SDR;
 	bool is_rx = vdev->vfl_dir != VFL_DIR_TX;
 	bool is_tx = vdev->vfl_dir != VFL_DIR_RX;
 
@@ -671,9 +671,26 @@ static void determine_valid_ioctls(struct video_device *vdev)
 			       ops->vidioc_try_fmt_sliced_vbi_out)))
 			set_bit(_IOC_NR(VIDIOC_TRY_FMT), valid_ioctls);
 		SET_VALID_IOCTL(ops, VIDIOC_G_SLICED_VBI_CAP, vidioc_g_sliced_vbi_cap);
+	} else if (is_sdr) {
+		/* SDR specific ioctls */
+		if (ops->vidioc_enum_fmt_vid_cap)
+			set_bit(_IOC_NR(VIDIOC_ENUM_FMT), valid_ioctls);
+		if (ops->vidioc_g_fmt_vid_cap)
+			set_bit(_IOC_NR(VIDIOC_G_FMT), valid_ioctls);
+		if (ops->vidioc_s_fmt_vid_cap)
+			set_bit(_IOC_NR(VIDIOC_S_FMT), valid_ioctls);
+		if (ops->vidioc_try_fmt_vid_cap)
+			set_bit(_IOC_NR(VIDIOC_TRY_FMT), valid_ioctls);
+
+		if (is_rx) {
+			SET_VALID_IOCTL(ops, VIDIOC_ENUMINPUT, vidioc_enum_input);
+			SET_VALID_IOCTL(ops, VIDIOC_G_INPUT, vidioc_g_input);
+			SET_VALID_IOCTL(ops, VIDIOC_S_INPUT, vidioc_s_input);
+		}
 	}
-	if (!is_radio) {
-		/* ioctls valid for video or vbi */
+
+	if (is_vid || is_vbi || is_sdr) {
+		/* ioctls valid for video, vbi or sdr */
 		SET_VALID_IOCTL(ops, VIDIOC_REQBUFS, vidioc_reqbufs);
 		SET_VALID_IOCTL(ops, VIDIOC_QUERYBUF, vidioc_querybuf);
 		SET_VALID_IOCTL(ops, VIDIOC_QBUF, vidioc_qbuf);
@@ -681,6 +698,10 @@ static void determine_valid_ioctls(struct video_device *vdev)
 		SET_VALID_IOCTL(ops, VIDIOC_DQBUF, vidioc_dqbuf);
 		SET_VALID_IOCTL(ops, VIDIOC_CREATE_BUFS, vidioc_create_bufs);
 		SET_VALID_IOCTL(ops, VIDIOC_PREPARE_BUF, vidioc_prepare_buf);
+	}
+
+	if (is_vid || is_vbi) {
+		/* ioctls valid for video or vbi */
 		if (ops->vidioc_s_std)
 			set_bit(_IOC_NR(VIDIOC_ENUMSTD), valid_ioctls);
 		SET_VALID_IOCTL(ops, VIDIOC_S_STD, vidioc_s_std);
diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
index edb0a4c..a7e6b52 100644
--- a/drivers/media/v4l2-core/v4l2-ioctl.c
+++ b/drivers/media/v4l2-core/v4l2-ioctl.c
@@ -243,6 +243,7 @@ static void v4l_print_format(const void *arg, bool write_only)
 	const struct v4l2_vbi_format *vbi;
 	const struct v4l2_sliced_vbi_format *sliced;
 	const struct v4l2_window *win;
+	const struct v4l2_format_sdr *sdr;
 	unsigned i;
 
 	pr_cont("type=%s", prt_names(p->type, v4l2_type_names));
@@ -316,6 +317,14 @@ static void v4l_print_format(const void *arg, bool write_only)
 				sliced->service_lines[0][i],
 				sliced->service_lines[1][i]);
 		break;
+	case V4L2_BUF_TYPE_SDR_CAPTURE:
+		sdr = &p->fmt.sdr;
+		pr_cont(", pixelformat=%c%c%c%c\n",
+			(sdr->pixelformat >>  0) & 0xff,
+			(sdr->pixelformat >>  8) & 0xff,
+			(sdr->pixelformat >> 16) & 0xff,
+			(sdr->pixelformat >> 24) & 0xff);
+		break;
 	}
 }
 
@@ -879,6 +888,7 @@ static int check_fmt(struct file *file, enum v4l2_buf_type type)
 	const struct v4l2_ioctl_ops *ops = vfd->ioctl_ops;
 	bool is_vid = vfd->vfl_type == VFL_TYPE_GRABBER;
 	bool is_vbi = vfd->vfl_type == VFL_TYPE_VBI;
+	bool is_sdr = vfd->vfl_type == VFL_TYPE_SDR;
 	bool is_rx = vfd->vfl_dir != VFL_DIR_TX;
 	bool is_tx = vfd->vfl_dir != VFL_DIR_RX;
 
@@ -928,6 +938,10 @@ static int check_fmt(struct file *file, enum v4l2_buf_type type)
 		if (is_vbi && is_tx && ops->vidioc_g_fmt_sliced_vbi_out)
 			return 0;
 		break;
+	case V4L2_BUF_TYPE_SDR_CAPTURE:
+		if (is_sdr && is_rx && ops->vidioc_g_fmt_vid_cap)
+			return 0;
+		break;
 	default:
 		break;
 	}
@@ -1047,6 +1061,10 @@ static int v4l_enum_fmt(const struct v4l2_ioctl_ops *ops,
 		if (unlikely(!is_tx || !ops->vidioc_enum_fmt_vid_out_mplane))
 			break;
 		return ops->vidioc_enum_fmt_vid_out_mplane(file, fh, arg);
+	case V4L2_BUF_TYPE_SDR_CAPTURE:
+		if (unlikely(!is_rx || !ops->vidioc_enum_fmt_vid_cap))
+			break;
+		return ops->vidioc_enum_fmt_vid_cap(file, fh, arg);
 	}
 	return -EINVAL;
 }
@@ -1057,6 +1075,7 @@ static int v4l_g_fmt(const struct v4l2_ioctl_ops *ops,
 	struct v4l2_format *p = arg;
 	struct video_device *vfd = video_devdata(file);
 	bool is_vid = vfd->vfl_type == VFL_TYPE_GRABBER;
+	bool is_sdr = vfd->vfl_type == VFL_TYPE_SDR;
 	bool is_rx = vfd->vfl_dir != VFL_DIR_TX;
 	bool is_tx = vfd->vfl_dir != VFL_DIR_RX;
 
@@ -1101,6 +1120,10 @@ static int v4l_g_fmt(const struct v4l2_ioctl_ops *ops,
 		if (unlikely(!is_tx || is_vid || !ops->vidioc_g_fmt_sliced_vbi_out))
 			break;
 		return ops->vidioc_g_fmt_sliced_vbi_out(file, fh, arg);
+	case V4L2_BUF_TYPE_SDR_CAPTURE:
+		if (unlikely(!is_rx || !is_sdr || !ops->vidioc_g_fmt_vid_cap))
+			break;
+		return ops->vidioc_g_fmt_vid_cap(file, fh, arg);
 	}
 	return -EINVAL;
 }
@@ -1111,6 +1134,7 @@ static int v4l_s_fmt(const struct v4l2_ioctl_ops *ops,
 	struct v4l2_format *p = arg;
 	struct video_device *vfd = video_devdata(file);
 	bool is_vid = vfd->vfl_type == VFL_TYPE_GRABBER;
+	bool is_sdr = vfd->vfl_type == VFL_TYPE_SDR;
 	bool is_rx = vfd->vfl_dir != VFL_DIR_TX;
 	bool is_tx = vfd->vfl_dir != VFL_DIR_RX;
 
@@ -1165,6 +1189,11 @@ static int v4l_s_fmt(const struct v4l2_ioctl_ops *ops,
 			break;
 		CLEAR_AFTER_FIELD(p, fmt.sliced);
 		return ops->vidioc_s_fmt_sliced_vbi_out(file, fh, arg);
+	case V4L2_BUF_TYPE_SDR_CAPTURE:
+		if (unlikely(!is_rx || !is_sdr || !ops->vidioc_s_fmt_vid_cap))
+			break;
+		CLEAR_AFTER_FIELD(p, fmt.sdr);
+		return ops->vidioc_s_fmt_vid_cap(file, fh, arg);
 	}
 	return -EINVAL;
 }
@@ -1175,6 +1204,7 @@ static int v4l_try_fmt(const struct v4l2_ioctl_ops *ops,
 	struct v4l2_format *p = arg;
 	struct video_device *vfd = video_devdata(file);
 	bool is_vid = vfd->vfl_type == VFL_TYPE_GRABBER;
+	bool is_sdr = vfd->vfl_type == VFL_TYPE_SDR;
 	bool is_rx = vfd->vfl_dir != VFL_DIR_TX;
 	bool is_tx = vfd->vfl_dir != VFL_DIR_RX;
 
@@ -1229,6 +1259,11 @@ static int v4l_try_fmt(const struct v4l2_ioctl_ops *ops,
 			break;
 		CLEAR_AFTER_FIELD(p, fmt.sliced);
 		return ops->vidioc_try_fmt_sliced_vbi_out(file, fh, arg);
+	case V4L2_BUF_TYPE_SDR_CAPTURE:
+		if (unlikely(!is_rx || !is_sdr || !ops->vidioc_try_fmt_vid_cap))
+			break;
+		CLEAR_AFTER_FIELD(p, fmt.sdr);
+		return ops->vidioc_try_fmt_vid_cap(file, fh, arg);
 	}
 	return -EINVAL;
 }
-- 
1.8.4.2

[PATCH RFC v2 7/7] v4l: define own IOCTL ops for SDR FMT

[Antti Palosaari]

Use own format ops for SDR data:
vidioc_enum_fmt_sdr_cap
vidioc_g_fmt_sdr_cap
vidioc_s_fmt_sdr_cap
vidioc_try_fmt_sdr_cap

Cc: Hans Verkuil <hverkuil@xs4all.nl>
Signed-off-by: Antti Palosaari <crope@iki.fi>
---
 drivers/media/v4l2-core/v4l2-dev.c   |  8 ++++----
 drivers/media/v4l2-core/v4l2-ioctl.c | 18 +++++++++---------
 include/media/v4l2-ioctl.h           |  8 ++++++++
 3 files changed, 21 insertions(+), 13 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
index 9f15e25..a84f4ea 100644
--- a/drivers/media/v4l2-core/v4l2-dev.c
+++ b/drivers/media/v4l2-core/v4l2-dev.c
@@ -673,13 +673,13 @@ static void determine_valid_ioctls(struct video_device *vdev)
 		SET_VALID_IOCTL(ops, VIDIOC_G_SLICED_VBI_CAP, vidioc_g_sliced_vbi_cap);
 	} else if (is_sdr) {
 		/* SDR specific ioctls */
-		if (ops->vidioc_enum_fmt_vid_cap)
+		if (ops->vidioc_enum_fmt_sdr_cap)
 			set_bit(_IOC_NR(VIDIOC_ENUM_FMT), valid_ioctls);
-		if (ops->vidioc_g_fmt_vid_cap)
+		if (ops->vidioc_g_fmt_sdr_cap)
 			set_bit(_IOC_NR(VIDIOC_G_FMT), valid_ioctls);
-		if (ops->vidioc_s_fmt_vid_cap)
+		if (ops->vidioc_s_fmt_sdr_cap)
 			set_bit(_IOC_NR(VIDIOC_S_FMT), valid_ioctls);
-		if (ops->vidioc_try_fmt_vid_cap)
+		if (ops->vidioc_try_fmt_sdr_cap)
 			set_bit(_IOC_NR(VIDIOC_TRY_FMT), valid_ioctls);
 
 		if (is_rx) {
diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
index a7e6b52..18aa36a 100644
--- a/drivers/media/v4l2-core/v4l2-ioctl.c
+++ b/drivers/media/v4l2-core/v4l2-ioctl.c
@@ -939,7 +939,7 @@ static int check_fmt(struct file *file, enum v4l2_buf_type type)
 			return 0;
 		break;
 	case V4L2_BUF_TYPE_SDR_CAPTURE:
-		if (is_sdr && is_rx && ops->vidioc_g_fmt_vid_cap)
+		if (is_sdr && is_rx && ops->vidioc_g_fmt_sdr_cap)
 			return 0;
 		break;
 	default:
@@ -1062,9 +1062,9 @@ static int v4l_enum_fmt(const struct v4l2_ioctl_ops *ops,
 			break;
 		return ops->vidioc_enum_fmt_vid_out_mplane(file, fh, arg);
 	case V4L2_BUF_TYPE_SDR_CAPTURE:
-		if (unlikely(!is_rx || !ops->vidioc_enum_fmt_vid_cap))
+		if (unlikely(!is_rx || !ops->vidioc_enum_fmt_sdr_cap))
 			break;
-		return ops->vidioc_enum_fmt_vid_cap(file, fh, arg);
+		return ops->vidioc_enum_fmt_sdr_cap(file, fh, arg);
 	}
 	return -EINVAL;
 }
@@ -1121,9 +1121,9 @@ static int v4l_g_fmt(const struct v4l2_ioctl_ops *ops,
 			break;
 		return ops->vidioc_g_fmt_sliced_vbi_out(file, fh, arg);
 	case V4L2_BUF_TYPE_SDR_CAPTURE:
-		if (unlikely(!is_rx || !is_sdr || !ops->vidioc_g_fmt_vid_cap))
+		if (unlikely(!is_rx || !is_sdr || !ops->vidioc_g_fmt_sdr_cap))
 			break;
-		return ops->vidioc_g_fmt_vid_cap(file, fh, arg);
+		return ops->vidioc_g_fmt_sdr_cap(file, fh, arg);
 	}
 	return -EINVAL;
 }
@@ -1190,10 +1190,10 @@ static int v4l_s_fmt(const struct v4l2_ioctl_ops *ops,
 		CLEAR_AFTER_FIELD(p, fmt.sliced);
 		return ops->vidioc_s_fmt_sliced_vbi_out(file, fh, arg);
 	case V4L2_BUF_TYPE_SDR_CAPTURE:
-		if (unlikely(!is_rx || !is_sdr || !ops->vidioc_s_fmt_vid_cap))
+		if (unlikely(!is_rx || !is_sdr || !ops->vidioc_s_fmt_sdr_cap))
 			break;
 		CLEAR_AFTER_FIELD(p, fmt.sdr);
-		return ops->vidioc_s_fmt_vid_cap(file, fh, arg);
+		return ops->vidioc_s_fmt_sdr_cap(file, fh, arg);
 	}
 	return -EINVAL;
 }
@@ -1260,10 +1260,10 @@ static int v4l_try_fmt(const struct v4l2_ioctl_ops *ops,
 		CLEAR_AFTER_FIELD(p, fmt.sliced);
 		return ops->vidioc_try_fmt_sliced_vbi_out(file, fh, arg);
 	case V4L2_BUF_TYPE_SDR_CAPTURE:
-		if (unlikely(!is_rx || !is_sdr || !ops->vidioc_try_fmt_vid_cap))
+		if (unlikely(!is_rx || !is_sdr || !ops->vidioc_try_fmt_sdr_cap))
 			break;
 		CLEAR_AFTER_FIELD(p, fmt.sdr);
-		return ops->vidioc_try_fmt_vid_cap(file, fh, arg);
+		return ops->vidioc_try_fmt_sdr_cap(file, fh, arg);
 	}
 	return -EINVAL;
 }
diff --git a/include/media/v4l2-ioctl.h b/include/media/v4l2-ioctl.h
index e0b74a4..8be32f5 100644
--- a/include/media/v4l2-ioctl.h
+++ b/include/media/v4l2-ioctl.h
@@ -40,6 +40,8 @@ struct v4l2_ioctl_ops {
 					      struct v4l2_fmtdesc *f);
 	int (*vidioc_enum_fmt_vid_out_mplane)(struct file *file, void *fh,
 					      struct v4l2_fmtdesc *f);
+	int (*vidioc_enum_fmt_sdr_cap)     (struct file *file, void *fh,
+					    struct v4l2_fmtdesc *f);
 
 	/* VIDIOC_G_FMT handlers */
 	int (*vidioc_g_fmt_vid_cap)    (struct file *file, void *fh,
@@ -62,6 +64,8 @@ struct v4l2_ioctl_ops {
 					   struct v4l2_format *f);
 	int (*vidioc_g_fmt_vid_out_mplane)(struct file *file, void *fh,
 					   struct v4l2_format *f);
+	int (*vidioc_g_fmt_sdr_cap)    (struct file *file, void *fh,
+					struct v4l2_format *f);
 
 	/* VIDIOC_S_FMT handlers */
 	int (*vidioc_s_fmt_vid_cap)    (struct file *file, void *fh,
@@ -84,6 +88,8 @@ struct v4l2_ioctl_ops {
 					   struct v4l2_format *f);
 	int (*vidioc_s_fmt_vid_out_mplane)(struct file *file, void *fh,
 					   struct v4l2_format *f);
+	int (*vidioc_s_fmt_sdr_cap)    (struct file *file, void *fh,
+					struct v4l2_format *f);
 
 	/* VIDIOC_TRY_FMT handlers */
 	int (*vidioc_try_fmt_vid_cap)    (struct file *file, void *fh,
@@ -106,6 +112,8 @@ struct v4l2_ioctl_ops {
 					     struct v4l2_format *f);
 	int (*vidioc_try_fmt_vid_out_mplane)(struct file *file, void *fh,
 					     struct v4l2_format *f);
+	int (*vidioc_try_fmt_sdr_cap)    (struct file *file, void *fh,
+					  struct v4l2_format *f);
 
 	/* Buffer handlers */
 	int (*vidioc_reqbufs) (struct file *file, void *fh, struct v4l2_requestbuffers *b);
-- 
1.8.4.2

Re: [PATCH RFC v2 7/7] v4l: define own IOCTL ops for SDR FMT

[Antti Palosaari]

Hello, Mauro, Hans,

On 14.12.2013 18:15, Antti Palosaari wrote:
> Use own format ops for SDR data:
> vidioc_enum_fmt_sdr_cap
> vidioc_g_fmt_sdr_cap
> vidioc_s_fmt_sdr_cap
> vidioc_try_fmt_sdr_cap

To be honest, I am a little bit against that patch. Is there any good 
reason we duplicate these FMT ops every-time when new stream format is 
added? For my eyes that is mostly just bloating the code without good 
reason.

regards
Antti


>
> Cc: Hans Verkuil <hverkuil@xs4all.nl>
> Signed-off-by: Antti Palosaari <crope@iki.fi>
> ---
>   drivers/media/v4l2-core/v4l2-dev.c   |  8 ++++----
>   drivers/media/v4l2-core/v4l2-ioctl.c | 18 +++++++++---------
>   include/media/v4l2-ioctl.h           |  8 ++++++++
>   3 files changed, 21 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
> index 9f15e25..a84f4ea 100644
> --- a/drivers/media/v4l2-core/v4l2-dev.c
> +++ b/drivers/media/v4l2-core/v4l2-dev.c
> @@ -673,13 +673,13 @@ static void determine_valid_ioctls(struct video_device *vdev)
>   		SET_VALID_IOCTL(ops, VIDIOC_G_SLICED_VBI_CAP, vidioc_g_sliced_vbi_cap);
>   	} else if (is_sdr) {
>   		/* SDR specific ioctls */
> -		if (ops->vidioc_enum_fmt_vid_cap)
> +		if (ops->vidioc_enum_fmt_sdr_cap)
>   			set_bit(_IOC_NR(VIDIOC_ENUM_FMT), valid_ioctls);
> -		if (ops->vidioc_g_fmt_vid_cap)
> +		if (ops->vidioc_g_fmt_sdr_cap)
>   			set_bit(_IOC_NR(VIDIOC_G_FMT), valid_ioctls);
> -		if (ops->vidioc_s_fmt_vid_cap)
> +		if (ops->vidioc_s_fmt_sdr_cap)
>   			set_bit(_IOC_NR(VIDIOC_S_FMT), valid_ioctls);
> -		if (ops->vidioc_try_fmt_vid_cap)
> +		if (ops->vidioc_try_fmt_sdr_cap)
>   			set_bit(_IOC_NR(VIDIOC_TRY_FMT), valid_ioctls);
>
>   		if (is_rx) {
> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
> index a7e6b52..18aa36a 100644
> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> @@ -939,7 +939,7 @@ static int check_fmt(struct file *file, enum v4l2_buf_type type)
>   			return 0;
>   		break;
>   	case V4L2_BUF_TYPE_SDR_CAPTURE:
> -		if (is_sdr && is_rx && ops->vidioc_g_fmt_vid_cap)
> +		if (is_sdr && is_rx && ops->vidioc_g_fmt_sdr_cap)
>   			return 0;
>   		break;
>   	default:
> @@ -1062,9 +1062,9 @@ static int v4l_enum_fmt(const struct v4l2_ioctl_ops *ops,
>   			break;
>   		return ops->vidioc_enum_fmt_vid_out_mplane(file, fh, arg);
>   	case V4L2_BUF_TYPE_SDR_CAPTURE:
> -		if (unlikely(!is_rx || !ops->vidioc_enum_fmt_vid_cap))
> +		if (unlikely(!is_rx || !ops->vidioc_enum_fmt_sdr_cap))
>   			break;
> -		return ops->vidioc_enum_fmt_vid_cap(file, fh, arg);
> +		return ops->vidioc_enum_fmt_sdr_cap(file, fh, arg);
>   	}
>   	return -EINVAL;
>   }
> @@ -1121,9 +1121,9 @@ static int v4l_g_fmt(const struct v4l2_ioctl_ops *ops,
>   			break;
>   		return ops->vidioc_g_fmt_sliced_vbi_out(file, fh, arg);
>   	case V4L2_BUF_TYPE_SDR_CAPTURE:
> -		if (unlikely(!is_rx || !is_sdr || !ops->vidioc_g_fmt_vid_cap))
> +		if (unlikely(!is_rx || !is_sdr || !ops->vidioc_g_fmt_sdr_cap))
>   			break;
> -		return ops->vidioc_g_fmt_vid_cap(file, fh, arg);
> +		return ops->vidioc_g_fmt_sdr_cap(file, fh, arg);
>   	}
>   	return -EINVAL;
>   }
> @@ -1190,10 +1190,10 @@ static int v4l_s_fmt(const struct v4l2_ioctl_ops *ops,
>   		CLEAR_AFTER_FIELD(p, fmt.sliced);
>   		return ops->vidioc_s_fmt_sliced_vbi_out(file, fh, arg);
>   	case V4L2_BUF_TYPE_SDR_CAPTURE:
> -		if (unlikely(!is_rx || !is_sdr || !ops->vidioc_s_fmt_vid_cap))
> +		if (unlikely(!is_rx || !is_sdr || !ops->vidioc_s_fmt_sdr_cap))
>   			break;
>   		CLEAR_AFTER_FIELD(p, fmt.sdr);
> -		return ops->vidioc_s_fmt_vid_cap(file, fh, arg);
> +		return ops->vidioc_s_fmt_sdr_cap(file, fh, arg);
>   	}
>   	return -EINVAL;
>   }
> @@ -1260,10 +1260,10 @@ static int v4l_try_fmt(const struct v4l2_ioctl_ops *ops,
>   		CLEAR_AFTER_FIELD(p, fmt.sliced);
>   		return ops->vidioc_try_fmt_sliced_vbi_out(file, fh, arg);
>   	case V4L2_BUF_TYPE_SDR_CAPTURE:
> -		if (unlikely(!is_rx || !is_sdr || !ops->vidioc_try_fmt_vid_cap))
> +		if (unlikely(!is_rx || !is_sdr || !ops->vidioc_try_fmt_sdr_cap))
>   			break;
>   		CLEAR_AFTER_FIELD(p, fmt.sdr);
> -		return ops->vidioc_try_fmt_vid_cap(file, fh, arg);
> +		return ops->vidioc_try_fmt_sdr_cap(file, fh, arg);
>   	}
>   	return -EINVAL;
>   }
> diff --git a/include/media/v4l2-ioctl.h b/include/media/v4l2-ioctl.h
> index e0b74a4..8be32f5 100644
> --- a/include/media/v4l2-ioctl.h
> +++ b/include/media/v4l2-ioctl.h
> @@ -40,6 +40,8 @@ struct v4l2_ioctl_ops {
>   					      struct v4l2_fmtdesc *f);
>   	int (*vidioc_enum_fmt_vid_out_mplane)(struct file *file, void *fh,
>   					      struct v4l2_fmtdesc *f);
> +	int (*vidioc_enum_fmt_sdr_cap)     (struct file *file, void *fh,
> +					    struct v4l2_fmtdesc *f);
>
>   	/* VIDIOC_G_FMT handlers */
>   	int (*vidioc_g_fmt_vid_cap)    (struct file *file, void *fh,
> @@ -62,6 +64,8 @@ struct v4l2_ioctl_ops {
>   					   struct v4l2_format *f);
>   	int (*vidioc_g_fmt_vid_out_mplane)(struct file *file, void *fh,
>   					   struct v4l2_format *f);
> +	int (*vidioc_g_fmt_sdr_cap)    (struct file *file, void *fh,
> +					struct v4l2_format *f);
>
>   	/* VIDIOC_S_FMT handlers */
>   	int (*vidioc_s_fmt_vid_cap)    (struct file *file, void *fh,
> @@ -84,6 +88,8 @@ struct v4l2_ioctl_ops {
>   					   struct v4l2_format *f);
>   	int (*vidioc_s_fmt_vid_out_mplane)(struct file *file, void *fh,
>   					   struct v4l2_format *f);
> +	int (*vidioc_s_fmt_sdr_cap)    (struct file *file, void *fh,
> +					struct v4l2_format *f);
>
>   	/* VIDIOC_TRY_FMT handlers */
>   	int (*vidioc_try_fmt_vid_cap)    (struct file *file, void *fh,
> @@ -106,6 +112,8 @@ struct v4l2_ioctl_ops {
>   					     struct v4l2_format *f);
>   	int (*vidioc_try_fmt_vid_out_mplane)(struct file *file, void *fh,
>   					     struct v4l2_format *f);
> +	int (*vidioc_try_fmt_sdr_cap)    (struct file *file, void *fh,
> +					  struct v4l2_format *f);
>
>   	/* Buffer handlers */
>   	int (*vidioc_reqbufs) (struct file *file, void *fh, struct v4l2_requestbuffers *b);
>


-- 
http://palosaari.fi/

Re: [PATCH RFC v2 0/7] V4L2 SDR API

[Antti Palosaari]

Hello
One possible problem I noticed is device node name.

Documentation/devices.txt

  65 block	SCSI disk devices (16-31)
		  0 = /dev/sdq		17th SCSI disk whole disk
		 16 = /dev/sdr		18th SCSI disk whole disk
		 32 = /dev/sds		19th SCSI disk whole disk
		    ...
		240 = /dev/sdaf		32nd SCSI disk whole disk

		Partitions are handled in the same way as for IDE
		disks (see major number 3) except that the limit on
		partitions is 15.


  81 char	video4linux
		  0 = /dev/video0	Video capture/overlay device
		    ...
		 63 = /dev/video63	Video capture/overlay device
		 64 = /dev/radio0	Radio device
		    ...
		127 = /dev/radio63	Radio device
		224 = /dev/vbi0		Vertical blank interrupt
		    ...
		255 = /dev/vbi31	Vertical blank interrupt


What I understand, /dev/sdr is not suitable node name as it conflicts 
with existing node name. Any ideas?

regards
Antti

-- 
http://palosaari.fi/

Re: [PATCH RFC v2 0/7] V4L2 SDR API

[Hans Verkuil]

On 12/14/2013 05:45 PM, Antti Palosaari wrote:
> Hello
> One possible problem I noticed is device node name.
> 
> Documentation/devices.txt
> 
>   65 block	SCSI disk devices (16-31)
> 		  0 = /dev/sdq		17th SCSI disk whole disk
> 		 16 = /dev/sdr		18th SCSI disk whole disk
> 		 32 = /dev/sds		19th SCSI disk whole disk
> 		    ...
> 		240 = /dev/sdaf		32nd SCSI disk whole disk
> 
> 		Partitions are handled in the same way as for IDE
> 		disks (see major number 3) except that the limit on
> 		partitions is 15.
> 
> 
>   81 char	video4linux
> 		  0 = /dev/video0	Video capture/overlay device
> 		    ...
> 		 63 = /dev/video63	Video capture/overlay device
> 		 64 = /dev/radio0	Radio device
> 		    ...
> 		127 = /dev/radio63	Radio device
> 		224 = /dev/vbi0		Vertical blank interrupt
> 		    ...
> 		255 = /dev/vbi31	Vertical blank interrupt
> 
> 
> What I understand, /dev/sdr is not suitable node name as it conflicts 
> with existing node name.

Good catch, that won't work :-)

> Any ideas?

/dev/sdradio?

Regards,

	Hans

Re: [PATCH RFC v2 0/7] V4L2 SDR API

[Antti Palosaari]

On 14.12.2013 19:05, Hans Verkuil wrote:
> On 12/14/2013 05:45 PM, Antti Palosaari wrote:
>> Hello
>> One possible problem I noticed is device node name.
>>
>> Documentation/devices.txt
>>
>>    65 block	SCSI disk devices (16-31)
>> 		  0 = /dev/sdq		17th SCSI disk whole disk
>> 		 16 = /dev/sdr		18th SCSI disk whole disk
>> 		 32 = /dev/sds		19th SCSI disk whole disk
>> 		    ...
>> 		240 = /dev/sdaf		32nd SCSI disk whole disk
>>
>> 		Partitions are handled in the same way as for IDE
>> 		disks (see major number 3) except that the limit on
>> 		partitions is 15.
>>
>>
>>    81 char	video4linux
>> 		  0 = /dev/video0	Video capture/overlay device
>> 		    ...
>> 		 63 = /dev/video63	Video capture/overlay device
>> 		 64 = /dev/radio0	Radio device
>> 		    ...
>> 		127 = /dev/radio63	Radio device
>> 		224 = /dev/vbi0		Vertical blank interrupt
>> 		    ...
>> 		255 = /dev/vbi31	Vertical blank interrupt
>>
>>
>> What I understand, /dev/sdr is not suitable node name as it conflicts
>> with existing node name.
>
> Good catch, that won't work :-)
>
>> Any ideas?
>
> /dev/sdradio?

/dev/swradio?


Lets do a small poll here. Everyone, but me, has a one vote ;)

regards
Antti
-- 
http://palosaari.fi/

Re: [PATCH RFC v2 7/7] v4l: define own IOCTL ops for SDR FMT

[Mauro Carvalho Chehab]

Em Sat, 14 Dec 2013 18:24:37 +0200
Antti Palosaari <crope@iki.fi> escreveu:

> Hello, Mauro, Hans,
> 
> On 14.12.2013 18:15, Antti Palosaari wrote:
> > Use own format ops for SDR data:
> > vidioc_enum_fmt_sdr_cap
> > vidioc_g_fmt_sdr_cap
> > vidioc_s_fmt_sdr_cap
> > vidioc_try_fmt_sdr_cap
> 
> To be honest, I am a little bit against that patch. Is there any good 
> reason we duplicate these FMT ops every-time when new stream format is 
> added? For my eyes that is mostly just bloating the code without good 
> reason.

The is one reason: when the same device can be used in both SDR and non
SDR mode (radio, video, vbi), then either the driver or the core would
need to select the right set of vidioc_*fmt_* ops.

In the past, all drivers had about the same logic for such tests.
Yet, as the implementations weren't the same, several of them were
implementing it wrong.

So, we ended by moving those validations to the core.

> 
> regards
> Antti
> 
> 
> >
> > Cc: Hans Verkuil <hverkuil@xs4all.nl>
> > Signed-off-by: Antti Palosaari <crope@iki.fi>
> > ---
> >   drivers/media/v4l2-core/v4l2-dev.c   |  8 ++++----
> >   drivers/media/v4l2-core/v4l2-ioctl.c | 18 +++++++++---------
> >   include/media/v4l2-ioctl.h           |  8 ++++++++
> >   3 files changed, 21 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
> > index 9f15e25..a84f4ea 100644
> > --- a/drivers/media/v4l2-core/v4l2-dev.c
> > +++ b/drivers/media/v4l2-core/v4l2-dev.c
> > @@ -673,13 +673,13 @@ static void determine_valid_ioctls(struct video_device *vdev)
> >   		SET_VALID_IOCTL(ops, VIDIOC_G_SLICED_VBI_CAP, vidioc_g_sliced_vbi_cap);
> >   	} else if (is_sdr) {
> >   		/* SDR specific ioctls */
> > -		if (ops->vidioc_enum_fmt_vid_cap)
> > +		if (ops->vidioc_enum_fmt_sdr_cap)
> >   			set_bit(_IOC_NR(VIDIOC_ENUM_FMT), valid_ioctls);
> > -		if (ops->vidioc_g_fmt_vid_cap)
> > +		if (ops->vidioc_g_fmt_sdr_cap)
> >   			set_bit(_IOC_NR(VIDIOC_G_FMT), valid_ioctls);
> > -		if (ops->vidioc_s_fmt_vid_cap)
> > +		if (ops->vidioc_s_fmt_sdr_cap)
> >   			set_bit(_IOC_NR(VIDIOC_S_FMT), valid_ioctls);
> > -		if (ops->vidioc_try_fmt_vid_cap)
> > +		if (ops->vidioc_try_fmt_sdr_cap)
> >   			set_bit(_IOC_NR(VIDIOC_TRY_FMT), valid_ioctls);
> >
> >   		if (is_rx) {
> > diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
> > index a7e6b52..18aa36a 100644
> > --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> > +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> > @@ -939,7 +939,7 @@ static int check_fmt(struct file *file, enum v4l2_buf_type type)
> >   			return 0;
> >   		break;
> >   	case V4L2_BUF_TYPE_SDR_CAPTURE:
> > -		if (is_sdr && is_rx && ops->vidioc_g_fmt_vid_cap)
> > +		if (is_sdr && is_rx && ops->vidioc_g_fmt_sdr_cap)
> >   			return 0;
> >   		break;
> >   	default:
> > @@ -1062,9 +1062,9 @@ static int v4l_enum_fmt(const struct v4l2_ioctl_ops *ops,
> >   			break;
> >   		return ops->vidioc_enum_fmt_vid_out_mplane(file, fh, arg);
> >   	case V4L2_BUF_TYPE_SDR_CAPTURE:
> > -		if (unlikely(!is_rx || !ops->vidioc_enum_fmt_vid_cap))
> > +		if (unlikely(!is_rx || !ops->vidioc_enum_fmt_sdr_cap))
> >   			break;
> > -		return ops->vidioc_enum_fmt_vid_cap(file, fh, arg);
> > +		return ops->vidioc_enum_fmt_sdr_cap(file, fh, arg);
> >   	}
> >   	return -EINVAL;
> >   }
> > @@ -1121,9 +1121,9 @@ static int v4l_g_fmt(const struct v4l2_ioctl_ops *ops,
> >   			break;
> >   		return ops->vidioc_g_fmt_sliced_vbi_out(file, fh, arg);
> >   	case V4L2_BUF_TYPE_SDR_CAPTURE:
> > -		if (unlikely(!is_rx || !is_sdr || !ops->vidioc_g_fmt_vid_cap))
> > +		if (unlikely(!is_rx || !is_sdr || !ops->vidioc_g_fmt_sdr_cap))
> >   			break;
> > -		return ops->vidioc_g_fmt_vid_cap(file, fh, arg);
> > +		return ops->vidioc_g_fmt_sdr_cap(file, fh, arg);
> >   	}
> >   	return -EINVAL;
> >   }
> > @@ -1190,10 +1190,10 @@ static int v4l_s_fmt(const struct v4l2_ioctl_ops *ops,
> >   		CLEAR_AFTER_FIELD(p, fmt.sliced);
> >   		return ops->vidioc_s_fmt_sliced_vbi_out(file, fh, arg);
> >   	case V4L2_BUF_TYPE_SDR_CAPTURE:
> > -		if (unlikely(!is_rx || !is_sdr || !ops->vidioc_s_fmt_vid_cap))
> > +		if (unlikely(!is_rx || !is_sdr || !ops->vidioc_s_fmt_sdr_cap))
> >   			break;
> >   		CLEAR_AFTER_FIELD(p, fmt.sdr);
> > -		return ops->vidioc_s_fmt_vid_cap(file, fh, arg);
> > +		return ops->vidioc_s_fmt_sdr_cap(file, fh, arg);
> >   	}
> >   	return -EINVAL;
> >   }
> > @@ -1260,10 +1260,10 @@ static int v4l_try_fmt(const struct v4l2_ioctl_ops *ops,
> >   		CLEAR_AFTER_FIELD(p, fmt.sliced);
> >   		return ops->vidioc_try_fmt_sliced_vbi_out(file, fh, arg);
> >   	case V4L2_BUF_TYPE_SDR_CAPTURE:
> > -		if (unlikely(!is_rx || !is_sdr || !ops->vidioc_try_fmt_vid_cap))
> > +		if (unlikely(!is_rx || !is_sdr || !ops->vidioc_try_fmt_sdr_cap))
> >   			break;
> >   		CLEAR_AFTER_FIELD(p, fmt.sdr);
> > -		return ops->vidioc_try_fmt_vid_cap(file, fh, arg);
> > +		return ops->vidioc_try_fmt_sdr_cap(file, fh, arg);
> >   	}
> >   	return -EINVAL;
> >   }
> > diff --git a/include/media/v4l2-ioctl.h b/include/media/v4l2-ioctl.h
> > index e0b74a4..8be32f5 100644
> > --- a/include/media/v4l2-ioctl.h
> > +++ b/include/media/v4l2-ioctl.h
> > @@ -40,6 +40,8 @@ struct v4l2_ioctl_ops {
> >   					      struct v4l2_fmtdesc *f);
> >   	int (*vidioc_enum_fmt_vid_out_mplane)(struct file *file, void *fh,
> >   					      struct v4l2_fmtdesc *f);
> > +	int (*vidioc_enum_fmt_sdr_cap)     (struct file *file, void *fh,
> > +					    struct v4l2_fmtdesc *f);
> >
> >   	/* VIDIOC_G_FMT handlers */
> >   	int (*vidioc_g_fmt_vid_cap)    (struct file *file, void *fh,
> > @@ -62,6 +64,8 @@ struct v4l2_ioctl_ops {
> >   					   struct v4l2_format *f);
> >   	int (*vidioc_g_fmt_vid_out_mplane)(struct file *file, void *fh,
> >   					   struct v4l2_format *f);
> > +	int (*vidioc_g_fmt_sdr_cap)    (struct file *file, void *fh,
> > +					struct v4l2_format *f);
> >
> >   	/* VIDIOC_S_FMT handlers */
> >   	int (*vidioc_s_fmt_vid_cap)    (struct file *file, void *fh,
> > @@ -84,6 +88,8 @@ struct v4l2_ioctl_ops {
> >   					   struct v4l2_format *f);
> >   	int (*vidioc_s_fmt_vid_out_mplane)(struct file *file, void *fh,
> >   					   struct v4l2_format *f);
> > +	int (*vidioc_s_fmt_sdr_cap)    (struct file *file, void *fh,
> > +					struct v4l2_format *f);
> >
> >   	/* VIDIOC_TRY_FMT handlers */
> >   	int (*vidioc_try_fmt_vid_cap)    (struct file *file, void *fh,
> > @@ -106,6 +112,8 @@ struct v4l2_ioctl_ops {
> >   					     struct v4l2_format *f);
> >   	int (*vidioc_try_fmt_vid_out_mplane)(struct file *file, void *fh,
> >   					     struct v4l2_format *f);
> > +	int (*vidioc_try_fmt_sdr_cap)    (struct file *file, void *fh,
> > +					  struct v4l2_format *f);
> >
> >   	/* Buffer handlers */
> >   	int (*vidioc_reqbufs) (struct file *file, void *fh, struct v4l2_requestbuffers *b);
> >
> 
> 


-- 

Cheers,
Mauro

Re: [PATCH RFC v2 0/7] V4L2 SDR API

[Mauro Carvalho Chehab]

Em Sat, 14 Dec 2013 19:47:45 +0200
Antti Palosaari <crope@iki.fi> escreveu:

> On 14.12.2013 19:05, Hans Verkuil wrote:
> > On 12/14/2013 05:45 PM, Antti Palosaari wrote:
> >> Hello
> >> One possible problem I noticed is device node name.
> >>
> >> Documentation/devices.txt
> >>
> >>    65 block	SCSI disk devices (16-31)
> >> 		  0 = /dev/sdq		17th SCSI disk whole disk
> >> 		 16 = /dev/sdr		18th SCSI disk whole disk
> >> 		 32 = /dev/sds		19th SCSI disk whole disk
> >> 		    ...
> >> 		240 = /dev/sdaf		32nd SCSI disk whole disk
> >>
> >> 		Partitions are handled in the same way as for IDE
> >> 		disks (see major number 3) except that the limit on
> >> 		partitions is 15.
> >>
> >>
> >>    81 char	video4linux
> >> 		  0 = /dev/video0	Video capture/overlay device
> >> 		    ...
> >> 		 63 = /dev/video63	Video capture/overlay device
> >> 		 64 = /dev/radio0	Radio device
> >> 		    ...
> >> 		127 = /dev/radio63	Radio device
> >> 		224 = /dev/vbi0		Vertical blank interrupt
> >> 		    ...
> >> 		255 = /dev/vbi31	Vertical blank interrupt
> >>
> >>
> >> What I understand, /dev/sdr is not suitable node name as it conflicts
> >> with existing node name.
> >
> > Good catch, that won't work :-)
> >
> >> Any ideas?
> >
> > /dev/sdradio?
> 
> /dev/swradio?
> 
> 
> Lets do a small poll here. Everyone, but me, has a one vote ;)

I vote for swradio.

The patches look ok to me, provided that you add the proper DocBook
bits on each of them.

I didn't like much that now have 3 ways to describe frequencies.
I think we should latter think on moving the frequency conversion to
the core, and use u64 with 1Hz step at the internal API, converting all
the drivers to use it.

IMHO, we should also provide a backward-compatible way that would allow
userspace to choose to use u64 1-Hz-stepping frequencies.

Of course the changes at the drivers is out of the scope, but perhaps
we should not apply patch 4/7, replacing it, instead, by some patch that
would move the frequency size to u64.

> 
> regards
> Antti


-- 

Cheers,
Mauro

Re: [PATCH RFC v2 7/7] v4l: define own IOCTL ops for SDR FMT

[Hans Verkuil]

On 12/15/2013 12:23 PM, Mauro Carvalho Chehab wrote:
> Em Sat, 14 Dec 2013 18:24:37 +0200
> Antti Palosaari <crope@iki.fi> escreveu:
> 
>> Hello, Mauro, Hans,
>>
>> On 14.12.2013 18:15, Antti Palosaari wrote:
>>> Use own format ops for SDR data:
>>> vidioc_enum_fmt_sdr_cap
>>> vidioc_g_fmt_sdr_cap
>>> vidioc_s_fmt_sdr_cap
>>> vidioc_try_fmt_sdr_cap
>>
>> To be honest, I am a little bit against that patch. Is there any good 
>> reason we duplicate these FMT ops every-time when new stream format is 
>> added? For my eyes that is mostly just bloating the code without good 
>> reason.
> 
> The is one reason: when the same device can be used in both SDR and non
> SDR mode (radio, video, vbi), then either the driver or the core would
> need to select the right set of vidioc_*fmt_* ops.
> 
> In the past, all drivers had about the same logic for such tests.
> Yet, as the implementations weren't the same, several of them were
> implementing it wrong.
> 
> So, we ended by moving those validations to the core.

I do think there is room for improvement here, though. Rather than
passing v4l2_format to the ops I would have preferred passing the appropriate
struct of the union instead.

And I never really liked it that try and set were split up. A 'try' boolean
would reduce the number of ops.

The first improvement is something that can be done at some point. It's too
late (and probably not worth it) to do anything about the second.

Regards,

	Hans

PS: Antti, I'll review the code in more detail tomorrow.

Re: [PATCH RFC v2 3/7] v4l: add new tuner types for SDR

[Hans Verkuil]

On 12/14/2013 05:15 PM, Antti Palosaari wrote:
> Define tuner types V4L2_TUNER_ADC and V4L2_TUNER_RF for SDR usage.
> 
> ADC is used for setting sampling rate (sampling frequency) to SDR
> device.
> 
> Another tuner type, named as V4L2_TUNER_RF, is possible RF tuner.
> Is is used to down-convert RF frequency to range ADC could sample.
> Having RF tuner is optional, whilst in practice it is almost always
> there.
> 
> Also add checks to VIDIOC_G_FREQUENCY, VIDIOC_S_FREQUENCY and
> VIDIOC_ENUM_FREQ_BANDS only allow these two tuner types when device
> type is SDR (VFL_TYPE_SDR).
> 
> Prohibit VIDIOC_S_HW_FREQ_SEEK explicitly when device type is SDR,
> as device cannot do hardware seek without a hardware demodulator.
> 
> Cc: Hans Verkuil <hverkuil@xs4all.nl>
> Signed-off-by: Antti Palosaari <crope@iki.fi>
> ---
>  drivers/media/v4l2-core/v4l2-ioctl.c | 42 ++++++++++++++++++++++++++----------
>  include/uapi/linux/videodev2.h       |  2 ++
>  2 files changed, 33 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
> index bc10684..6b72bd8 100644
> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> @@ -1288,8 +1288,13 @@ static int v4l_g_frequency(const struct v4l2_ioctl_ops *ops,
>  	struct video_device *vfd = video_devdata(file);
>  	struct v4l2_frequency *p = arg;
>  
> -	p->type = (vfd->vfl_type == VFL_TYPE_RADIO) ?
> -			V4L2_TUNER_RADIO : V4L2_TUNER_ANALOG_TV;
> +	if (vfd->vfl_type == VFL_TYPE_SDR) {
> +		if (p->type != V4L2_TUNER_ADC && p->type != V4L2_TUNER_RF)
> +			return -EINVAL;

This is wrong. As you mentioned in patch 1, the type field should always be set by
the driver. So type is not something that is set by the user.

I would just set type to V4L2_TUNER_ADC here (all SDR devices have at least an ADC
tuner), and let the driver change it to TUNER_RF if this tuner is really an RF
tuner. 

> +	} else {
> +		p->type = (vfd->vfl_type == VFL_TYPE_RADIO) ?
> +				V4L2_TUNER_RADIO : V4L2_TUNER_ANALOG_TV;
> +	}
>  	return ops->vidioc_g_frequency(file, fh, p);
>  }
>  
> @@ -1300,10 +1305,16 @@ static int v4l_s_frequency(const struct v4l2_ioctl_ops *ops,
>  	const struct v4l2_frequency *p = arg;
>  	enum v4l2_tuner_type type;
>  
> -	type = (vfd->vfl_type == VFL_TYPE_RADIO) ?
> -			V4L2_TUNER_RADIO : V4L2_TUNER_ANALOG_TV;
> -	if (p->type != type)
> -		return -EINVAL;
> +	if (vfd->vfl_type == VFL_TYPE_SDR) {
> +		if (p->type != V4L2_TUNER_ADC && p->type != V4L2_TUNER_RF)
> +			return -EINVAL;
> +		type = p->type;

No need to set type here. It isn't used.

> +	} else {
> +		type = (vfd->vfl_type == VFL_TYPE_RADIO) ?
> +				V4L2_TUNER_RADIO : V4L2_TUNER_ANALOG_TV;
> +		if (type != p->type)
> +			return -EINVAL;
> +	}
>  	return ops->vidioc_s_frequency(file, fh, p);
>  }
>  
> @@ -1383,6 +1394,10 @@ static int v4l_s_hw_freq_seek(const struct v4l2_ioctl_ops *ops,
>  	struct v4l2_hw_freq_seek *p = arg;
>  	enum v4l2_tuner_type type;
>  
> +	/* s_hw_freq_seek is not supported for SDR for now */
> +	if (vfd->vfl_type == VFL_TYPE_SDR)
> +		return -EINVAL;
> +
>  	type = (vfd->vfl_type == VFL_TYPE_RADIO) ?
>  		V4L2_TUNER_RADIO : V4L2_TUNER_ANALOG_TV;
>  	if (p->type != type)
> @@ -1882,11 +1897,16 @@ static int v4l_enum_freq_bands(const struct v4l2_ioctl_ops *ops,
>  	enum v4l2_tuner_type type;
>  	int err;
>  
> -	type = (vfd->vfl_type == VFL_TYPE_RADIO) ?
> -			V4L2_TUNER_RADIO : V4L2_TUNER_ANALOG_TV;
> -
> -	if (type != p->type)
> -		return -EINVAL;
> +	if (vfd->vfl_type == VFL_TYPE_SDR) {
> +		if (p->type != V4L2_TUNER_ADC && p->type != V4L2_TUNER_RF)
> +			return -EINVAL;
> +		type = p->type;

Ditto.

> +	} else {
> +		type = (vfd->vfl_type == VFL_TYPE_RADIO) ?
> +				V4L2_TUNER_RADIO : V4L2_TUNER_ANALOG_TV;
> +		if (type != p->type)
> +			return -EINVAL;
> +	}

Perhaps this type check should be moved to a separate function. It's after
all used by both enum_freq_bands and s_frequency.

>  	if (ops->vidioc_enum_freq_bands)
>  		return ops->vidioc_enum_freq_bands(file, fh, p);
>  	if (is_valid_ioctl(vfd, VIDIOC_G_TUNER)) {
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index b8ee9048..c3e7780 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -159,6 +159,8 @@ enum v4l2_tuner_type {
>  	V4L2_TUNER_RADIO	     = 1,
>  	V4L2_TUNER_ANALOG_TV	     = 2,
>  	V4L2_TUNER_DIGITAL_TV	     = 3,
> +	V4L2_TUNER_ADC               = 4,
> +	V4L2_TUNER_RF                = 5,
>  };
>  
>  enum v4l2_memory {
> 

Regards,

	Hans

Re: [PATCH RFC v2 7/7] v4l: define own IOCTL ops for SDR FMT

[Hans Verkuil]

On 12/14/2013 05:15 PM, Antti Palosaari wrote:
> Use own format ops for SDR data:
> vidioc_enum_fmt_sdr_cap
> vidioc_g_fmt_sdr_cap
> vidioc_s_fmt_sdr_cap
> vidioc_try_fmt_sdr_cap

The patch order is a bit weird. I would expect this patch to come before patch 6.

Regards,

	Hans

> 
> Cc: Hans Verkuil <hverkuil@xs4all.nl>
> Signed-off-by: Antti Palosaari <crope@iki.fi>
> ---
>  drivers/media/v4l2-core/v4l2-dev.c   |  8 ++++----
>  drivers/media/v4l2-core/v4l2-ioctl.c | 18 +++++++++---------
>  include/media/v4l2-ioctl.h           |  8 ++++++++
>  3 files changed, 21 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
> index 9f15e25..a84f4ea 100644
> --- a/drivers/media/v4l2-core/v4l2-dev.c
> +++ b/drivers/media/v4l2-core/v4l2-dev.c
> @@ -673,13 +673,13 @@ static void determine_valid_ioctls(struct video_device *vdev)
>  		SET_VALID_IOCTL(ops, VIDIOC_G_SLICED_VBI_CAP, vidioc_g_sliced_vbi_cap);
>  	} else if (is_sdr) {
>  		/* SDR specific ioctls */
> -		if (ops->vidioc_enum_fmt_vid_cap)
> +		if (ops->vidioc_enum_fmt_sdr_cap)
>  			set_bit(_IOC_NR(VIDIOC_ENUM_FMT), valid_ioctls);
> -		if (ops->vidioc_g_fmt_vid_cap)
> +		if (ops->vidioc_g_fmt_sdr_cap)
>  			set_bit(_IOC_NR(VIDIOC_G_FMT), valid_ioctls);
> -		if (ops->vidioc_s_fmt_vid_cap)
> +		if (ops->vidioc_s_fmt_sdr_cap)
>  			set_bit(_IOC_NR(VIDIOC_S_FMT), valid_ioctls);
> -		if (ops->vidioc_try_fmt_vid_cap)
> +		if (ops->vidioc_try_fmt_sdr_cap)
>  			set_bit(_IOC_NR(VIDIOC_TRY_FMT), valid_ioctls);
>  
>  		if (is_rx) {
> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
> index a7e6b52..18aa36a 100644
> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> @@ -939,7 +939,7 @@ static int check_fmt(struct file *file, enum v4l2_buf_type type)
>  			return 0;
>  		break;
>  	case V4L2_BUF_TYPE_SDR_CAPTURE:
> -		if (is_sdr && is_rx && ops->vidioc_g_fmt_vid_cap)
> +		if (is_sdr && is_rx && ops->vidioc_g_fmt_sdr_cap)
>  			return 0;
>  		break;
>  	default:
> @@ -1062,9 +1062,9 @@ static int v4l_enum_fmt(const struct v4l2_ioctl_ops *ops,
>  			break;
>  		return ops->vidioc_enum_fmt_vid_out_mplane(file, fh, arg);
>  	case V4L2_BUF_TYPE_SDR_CAPTURE:
> -		if (unlikely(!is_rx || !ops->vidioc_enum_fmt_vid_cap))
> +		if (unlikely(!is_rx || !ops->vidioc_enum_fmt_sdr_cap))
>  			break;
> -		return ops->vidioc_enum_fmt_vid_cap(file, fh, arg);
> +		return ops->vidioc_enum_fmt_sdr_cap(file, fh, arg);
>  	}
>  	return -EINVAL;
>  }
> @@ -1121,9 +1121,9 @@ static int v4l_g_fmt(const struct v4l2_ioctl_ops *ops,
>  			break;
>  		return ops->vidioc_g_fmt_sliced_vbi_out(file, fh, arg);
>  	case V4L2_BUF_TYPE_SDR_CAPTURE:
> -		if (unlikely(!is_rx || !is_sdr || !ops->vidioc_g_fmt_vid_cap))
> +		if (unlikely(!is_rx || !is_sdr || !ops->vidioc_g_fmt_sdr_cap))
>  			break;
> -		return ops->vidioc_g_fmt_vid_cap(file, fh, arg);
> +		return ops->vidioc_g_fmt_sdr_cap(file, fh, arg);
>  	}
>  	return -EINVAL;
>  }
> @@ -1190,10 +1190,10 @@ static int v4l_s_fmt(const struct v4l2_ioctl_ops *ops,
>  		CLEAR_AFTER_FIELD(p, fmt.sliced);
>  		return ops->vidioc_s_fmt_sliced_vbi_out(file, fh, arg);
>  	case V4L2_BUF_TYPE_SDR_CAPTURE:
> -		if (unlikely(!is_rx || !is_sdr || !ops->vidioc_s_fmt_vid_cap))
> +		if (unlikely(!is_rx || !is_sdr || !ops->vidioc_s_fmt_sdr_cap))
>  			break;
>  		CLEAR_AFTER_FIELD(p, fmt.sdr);
> -		return ops->vidioc_s_fmt_vid_cap(file, fh, arg);
> +		return ops->vidioc_s_fmt_sdr_cap(file, fh, arg);
>  	}
>  	return -EINVAL;
>  }
> @@ -1260,10 +1260,10 @@ static int v4l_try_fmt(const struct v4l2_ioctl_ops *ops,
>  		CLEAR_AFTER_FIELD(p, fmt.sliced);
>  		return ops->vidioc_try_fmt_sliced_vbi_out(file, fh, arg);
>  	case V4L2_BUF_TYPE_SDR_CAPTURE:
> -		if (unlikely(!is_rx || !is_sdr || !ops->vidioc_try_fmt_vid_cap))
> +		if (unlikely(!is_rx || !is_sdr || !ops->vidioc_try_fmt_sdr_cap))
>  			break;
>  		CLEAR_AFTER_FIELD(p, fmt.sdr);
> -		return ops->vidioc_try_fmt_vid_cap(file, fh, arg);
> +		return ops->vidioc_try_fmt_sdr_cap(file, fh, arg);
>  	}
>  	return -EINVAL;
>  }
> diff --git a/include/media/v4l2-ioctl.h b/include/media/v4l2-ioctl.h
> index e0b74a4..8be32f5 100644
> --- a/include/media/v4l2-ioctl.h
> +++ b/include/media/v4l2-ioctl.h
> @@ -40,6 +40,8 @@ struct v4l2_ioctl_ops {
>  					      struct v4l2_fmtdesc *f);
>  	int (*vidioc_enum_fmt_vid_out_mplane)(struct file *file, void *fh,
>  					      struct v4l2_fmtdesc *f);
> +	int (*vidioc_enum_fmt_sdr_cap)     (struct file *file, void *fh,
> +					    struct v4l2_fmtdesc *f);
>  
>  	/* VIDIOC_G_FMT handlers */
>  	int (*vidioc_g_fmt_vid_cap)    (struct file *file, void *fh,
> @@ -62,6 +64,8 @@ struct v4l2_ioctl_ops {
>  					   struct v4l2_format *f);
>  	int (*vidioc_g_fmt_vid_out_mplane)(struct file *file, void *fh,
>  					   struct v4l2_format *f);
> +	int (*vidioc_g_fmt_sdr_cap)    (struct file *file, void *fh,
> +					struct v4l2_format *f);
>  
>  	/* VIDIOC_S_FMT handlers */
>  	int (*vidioc_s_fmt_vid_cap)    (struct file *file, void *fh,
> @@ -84,6 +88,8 @@ struct v4l2_ioctl_ops {
>  					   struct v4l2_format *f);
>  	int (*vidioc_s_fmt_vid_out_mplane)(struct file *file, void *fh,
>  					   struct v4l2_format *f);
> +	int (*vidioc_s_fmt_sdr_cap)    (struct file *file, void *fh,
> +					struct v4l2_format *f);
>  
>  	/* VIDIOC_TRY_FMT handlers */
>  	int (*vidioc_try_fmt_vid_cap)    (struct file *file, void *fh,
> @@ -106,6 +112,8 @@ struct v4l2_ioctl_ops {
>  					     struct v4l2_format *f);
>  	int (*vidioc_try_fmt_vid_out_mplane)(struct file *file, void *fh,
>  					     struct v4l2_format *f);
> +	int (*vidioc_try_fmt_sdr_cap)    (struct file *file, void *fh,
> +					  struct v4l2_format *f);
>  
>  	/* Buffer handlers */
>  	int (*vidioc_reqbufs) (struct file *file, void *fh, struct v4l2_requestbuffers *b);
> 

Re: [PATCH RFC v2 0/7] V4L2 SDR API

[Hans Verkuil]

On 12/14/2013 06:47 PM, Antti Palosaari wrote:
> On 14.12.2013 19:05, Hans Verkuil wrote:
>> On 12/14/2013 05:45 PM, Antti Palosaari wrote:
>>> Hello
>>> One possible problem I noticed is device node name.
>>>
>>> Documentation/devices.txt
>>>
>>>    65 block	SCSI disk devices (16-31)
>>> 		  0 = /dev/sdq		17th SCSI disk whole disk
>>> 		 16 = /dev/sdr		18th SCSI disk whole disk
>>> 		 32 = /dev/sds		19th SCSI disk whole disk
>>> 		    ...
>>> 		240 = /dev/sdaf		32nd SCSI disk whole disk
>>>
>>> 		Partitions are handled in the same way as for IDE
>>> 		disks (see major number 3) except that the limit on
>>> 		partitions is 15.
>>>
>>>
>>>    81 char	video4linux
>>> 		  0 = /dev/video0	Video capture/overlay device
>>> 		    ...
>>> 		 63 = /dev/video63	Video capture/overlay device
>>> 		 64 = /dev/radio0	Radio device
>>> 		    ...
>>> 		127 = /dev/radio63	Radio device
>>> 		224 = /dev/vbi0		Vertical blank interrupt
>>> 		    ...
>>> 		255 = /dev/vbi31	Vertical blank interrupt
>>>
>>>
>>> What I understand, /dev/sdr is not suitable node name as it conflicts
>>> with existing node name.
>>
>> Good catch, that won't work :-)
>>
>>> Any ideas?
>>
>> /dev/sdradio?
> 
> /dev/swradio?
> 
> 
> Lets do a small poll here. Everyone, but me, has a one vote ;)

I'm happy with swradio as well. It's probably better than sdradio, namespace-wise.

Regards,

	Hans

Re: [PATCH RFC v2 3/7] v4l: add new tuner types for SDR

[Antti Palosaari]

On 16.12.2013 10:53, Hans Verkuil wrote:
> On 12/14/2013 05:15 PM, Antti Palosaari wrote:

>> @@ -1288,8 +1288,13 @@ static int v4l_g_frequency(const struct v4l2_ioctl_ops *ops,
>>   	struct video_device *vfd = video_devdata(file);
>>   	struct v4l2_frequency *p = arg;
>>
>> -	p->type = (vfd->vfl_type == VFL_TYPE_RADIO) ?
>> -			V4L2_TUNER_RADIO : V4L2_TUNER_ANALOG_TV;
>> +	if (vfd->vfl_type == VFL_TYPE_SDR) {
>> +		if (p->type != V4L2_TUNER_ADC && p->type != V4L2_TUNER_RF)
>> +			return -EINVAL;
>
> This is wrong. As you mentioned in patch 1, the type field should always be set by
> the driver. So type is not something that is set by the user.
>
> I would just set type to V4L2_TUNER_ADC here (all SDR devices have at least an ADC
> tuner), and let the driver change it to TUNER_RF if this tuner is really an RF
> tuner.

I don't think so. It sounds very stupid to handle tuner type with 
different meaning in that single case - it sounds just a is a mistake 
(and that SDR case mistakes are not needed continue as no regressions 
apply). I can say I was very puzzled what is the reason my tuner type is 
always changed to wrong, until finally found it was overridden here.

For me this looks more than it is just forced to "some" suitable value 
in a case app does not fill it correctly - not the way driver should 
return it to app. Tuner ID and type are here for Kernel driver could 
identify not the opposite and that is how it should be without unneeded 
exceptions.

Also, API does not specify that kind of different meaning for tuner type 
in a case of g_frequency.

Have to search some history where that odds is coming from...


regards
Antti

-- 
http://palosaari.fi/

Re: [PATCH RFC v2 3/7] v4l: add new tuner types for SDR

[Hans Verkuil]

On 12/16/2013 01:36 PM, Antti Palosaari wrote:
> On 16.12.2013 10:53, Hans Verkuil wrote:
>> On 12/14/2013 05:15 PM, Antti Palosaari wrote:
> 
>>> @@ -1288,8 +1288,13 @@ static int v4l_g_frequency(const struct v4l2_ioctl_ops *ops,
>>>   	struct video_device *vfd = video_devdata(file);
>>>   	struct v4l2_frequency *p = arg;
>>>
>>> -	p->type = (vfd->vfl_type == VFL_TYPE_RADIO) ?
>>> -			V4L2_TUNER_RADIO : V4L2_TUNER_ANALOG_TV;
>>> +	if (vfd->vfl_type == VFL_TYPE_SDR) {
>>> +		if (p->type != V4L2_TUNER_ADC && p->type != V4L2_TUNER_RF)
>>> +			return -EINVAL;
>>
>> This is wrong. As you mentioned in patch 1, the type field should always be set by
>> the driver. So type is not something that is set by the user.
>>
>> I would just set type to V4L2_TUNER_ADC here (all SDR devices have at least an ADC
>> tuner), and let the driver change it to TUNER_RF if this tuner is really an RF
>> tuner.
> 
> I don't think so. It sounds very stupid to handle tuner type with 
> different meaning in that single case - it sounds just a is a mistake 
> (and that SDR case mistakes are not needed continue as no regressions 
> apply). I can say I was very puzzled what is the reason my tuner type is 
> always changed to wrong, until finally found it was overridden here.
> 
> For me this looks more than it is just forced to "some" suitable value 
> in a case app does not fill it correctly - not the way driver should 
> return it to app. Tuner ID and type are here for Kernel driver could 
> identify not the opposite and that is how it should be without unneeded 
> exceptions.
> 
> Also, API does not specify that kind of different meaning for tuner type 
> in a case of g_frequency.
> 
> Have to search some history where that odds is coming from...

The application *does not set type* when calling G_FREQUENCY. The driver has
to fill that in. So the type field as received from the application is
uninitialized. That's the way the spec was defined, and that's the way
applications use G_FREQUENCY. There is nothing you can do about that.

So drivers have to fill in the type based on vfl_type and the tuner index.
Since drivers often didn't do that the vfl_type check has been moved to the
v4l2 core. In the case of SDR the type is actually dependent on the tuner
index, so the core cannot fully initialize the type field.

You can either leave it uninitialized for vfl_type SDR and leave it to the
SDR driver to fill in the type, or you can set it to ADC so the driver
only has to update the type field if the tuner index corresponds to the
RF tuner.

Regards,

	Hans

Re: [PATCH RFC v2 3/7] v4l: add new tuner types for SDR

[Antti Palosaari]

On 16.12.2013 14:50, Hans Verkuil wrote:
> On 12/16/2013 01:36 PM, Antti Palosaari wrote:
>> On 16.12.2013 10:53, Hans Verkuil wrote:
>>> On 12/14/2013 05:15 PM, Antti Palosaari wrote:
>>
>>>> @@ -1288,8 +1288,13 @@ static int v4l_g_frequency(const struct v4l2_ioctl_ops *ops,
>>>>    	struct video_device *vfd = video_devdata(file);
>>>>    	struct v4l2_frequency *p = arg;
>>>>
>>>> -	p->type = (vfd->vfl_type == VFL_TYPE_RADIO) ?
>>>> -			V4L2_TUNER_RADIO : V4L2_TUNER_ANALOG_TV;
>>>> +	if (vfd->vfl_type == VFL_TYPE_SDR) {
>>>> +		if (p->type != V4L2_TUNER_ADC && p->type != V4L2_TUNER_RF)
>>>> +			return -EINVAL;
>>>
>>> This is wrong. As you mentioned in patch 1, the type field should always be set by
>>> the driver. So type is not something that is set by the user.
>>>
>>> I would just set type to V4L2_TUNER_ADC here (all SDR devices have at least an ADC
>>> tuner), and let the driver change it to TUNER_RF if this tuner is really an RF
>>> tuner.
>>
>> I don't think so. It sounds very stupid to handle tuner type with
>> different meaning in that single case - it sounds just a is a mistake
>> (and that SDR case mistakes are not needed continue as no regressions
>> apply). I can say I was very puzzled what is the reason my tuner type is
>> always changed to wrong, until finally found it was overridden here.
>>
>> For me this looks more than it is just forced to "some" suitable value
>> in a case app does not fill it correctly - not the way driver should
>> return it to app. Tuner ID and type are here for Kernel driver could
>> identify not the opposite and that is how it should be without unneeded
>> exceptions.
>>
>> Also, API does not specify that kind of different meaning for tuner type
>> in a case of g_frequency.
>>
>> Have to search some history where that odds is coming from...
>
> The application *does not set type* when calling G_FREQUENCY. The driver has
> to fill that in. So the type field as received from the application is
> uninitialized. That's the way the spec was defined, and that's the way
> applications use G_FREQUENCY. There is nothing you can do about that.
>
> So drivers have to fill in the type based on vfl_type and the tuner index.
> Since drivers often didn't do that the vfl_type check has been moved to the
> v4l2 core. In the case of SDR the type is actually dependent on the tuner
> index, so the core cannot fully initialize the type field.
>
> You can either leave it uninitialized for vfl_type SDR and leave it to the
> SDR driver to fill in the type, or you can set it to ADC so the driver
> only has to update the type field if the tuner index corresponds to the
> RF tuner.


commit 227690df75382e46a4f6ea1bbc5df855a674b47f
Author: Hans Verkuil <hans.verkuil@cisco.com>
Date:   Sun Jun 12 06:36:41 2011 -0300

     [media] v4l2-ioctl.c: prefill tuner type for g_frequency and g/s_tuner

     The subdevs are supposed to receive a valid tuner type for the 
g_frequency
     and g/s_tuner subdev ops. Some drivers do this, others don't. So 
prefill
     this in v4l2-ioctl.c based on whether the device node from which 
this is
     called is a radio node or not.

     The spec does not require applications to fill in the type, and if they
     leave it at 0 then the 'check_mode' call in tuner-core.c will return
     an error and the ioctl does nothing.

     Cc: stable@kernel.org
     Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
     Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>


-- 
http://palosaari.fi/

Re: [PATCH RFC v2 3/7] v4l: add new tuner types for SDR

[Hans Verkuil]

On 12/16/2013 03:19 PM, Antti Palosaari wrote:
> On 16.12.2013 14:50, Hans Verkuil wrote:
>> On 12/16/2013 01:36 PM, Antti Palosaari wrote:
>>> On 16.12.2013 10:53, Hans Verkuil wrote:
>>>> On 12/14/2013 05:15 PM, Antti Palosaari wrote:
>>>
>>>>> @@ -1288,8 +1288,13 @@ static int v4l_g_frequency(const struct v4l2_ioctl_ops *ops,
>>>>>    	struct video_device *vfd = video_devdata(file);
>>>>>    	struct v4l2_frequency *p = arg;
>>>>>
>>>>> -	p->type = (vfd->vfl_type == VFL_TYPE_RADIO) ?
>>>>> -			V4L2_TUNER_RADIO : V4L2_TUNER_ANALOG_TV;
>>>>> +	if (vfd->vfl_type == VFL_TYPE_SDR) {
>>>>> +		if (p->type != V4L2_TUNER_ADC && p->type != V4L2_TUNER_RF)
>>>>> +			return -EINVAL;
>>>>
>>>> This is wrong. As you mentioned in patch 1, the type field should always be set by
>>>> the driver. So type is not something that is set by the user.
>>>>
>>>> I would just set type to V4L2_TUNER_ADC here (all SDR devices have at least an ADC
>>>> tuner), and let the driver change it to TUNER_RF if this tuner is really an RF
>>>> tuner.
>>>
>>> I don't think so. It sounds very stupid to handle tuner type with
>>> different meaning in that single case - it sounds just a is a mistake
>>> (and that SDR case mistakes are not needed continue as no regressions
>>> apply). I can say I was very puzzled what is the reason my tuner type is
>>> always changed to wrong, until finally found it was overridden here.
>>>
>>> For me this looks more than it is just forced to "some" suitable value
>>> in a case app does not fill it correctly - not the way driver should
>>> return it to app. Tuner ID and type are here for Kernel driver could
>>> identify not the opposite and that is how it should be without unneeded
>>> exceptions.
>>>
>>> Also, API does not specify that kind of different meaning for tuner type
>>> in a case of g_frequency.
>>>
>>> Have to search some history where that odds is coming from...
>>
>> The application *does not set type* when calling G_FREQUENCY. The driver has
>> to fill that in. So the type field as received from the application is
>> uninitialized. That's the way the spec was defined, and that's the way
>> applications use G_FREQUENCY. There is nothing you can do about that.
>>
>> So drivers have to fill in the type based on vfl_type and the tuner index.
>> Since drivers often didn't do that the vfl_type check has been moved to the
>> v4l2 core. In the case of SDR the type is actually dependent on the tuner
>> index, so the core cannot fully initialize the type field.
>>
>> You can either leave it uninitialized for vfl_type SDR and leave it to the
>> SDR driver to fill in the type, or you can set it to ADC so the driver
>> only has to update the type field if the tuner index corresponds to the
>> RF tuner.
> 
> 
> commit 227690df75382e46a4f6ea1bbc5df855a674b47f
> Author: Hans Verkuil <hans.verkuil@cisco.com>
> Date:   Sun Jun 12 06:36:41 2011 -0300
> 
>      [media] v4l2-ioctl.c: prefill tuner type for g_frequency and g/s_tuner
> 
>      The subdevs are supposed to receive a valid tuner type for the 
> g_frequency

This talks about the *subdevs*. The low-level tuner ops implemented by
sub-devices expected a valid type field. That field had to be filled in
by bridge drivers, and they often did not do that, or filled in the wrong
type.

>      and g/s_tuner subdev ops. Some drivers do this, others don't. So 
> prefill
>      this in v4l2-ioctl.c based on whether the device node from which 
> this is
>      called is a radio node or not.
> 
>      The spec does not require applications to fill in the type, and if they
       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

>      leave it at 0 then the 'check_mode' call in tuner-core.c will return
>      an error and the ioctl does nothing.
> 
>      Cc: stable@kernel.org
>      Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
>      Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
> 
> 

Regards,

	Hans

Re: [PATCH RFC v2 3/7] v4l: add new tuner types for SDR

[Antti Palosaari]

On 16.12.2013 16:25, Hans Verkuil wrote:
> On 12/16/2013 03:19 PM, Antti Palosaari wrote:
>> On 16.12.2013 14:50, Hans Verkuil wrote:
>>> On 12/16/2013 01:36 PM, Antti Palosaari wrote:
>>>> On 16.12.2013 10:53, Hans Verkuil wrote:
>>>>> On 12/14/2013 05:15 PM, Antti Palosaari wrote:
>>>>
>>>>>> @@ -1288,8 +1288,13 @@ static int v4l_g_frequency(const struct v4l2_ioctl_ops *ops,
>>>>>>     	struct video_device *vfd = video_devdata(file);
>>>>>>     	struct v4l2_frequency *p = arg;
>>>>>>
>>>>>> -	p->type = (vfd->vfl_type == VFL_TYPE_RADIO) ?
>>>>>> -			V4L2_TUNER_RADIO : V4L2_TUNER_ANALOG_TV;
>>>>>> +	if (vfd->vfl_type == VFL_TYPE_SDR) {
>>>>>> +		if (p->type != V4L2_TUNER_ADC && p->type != V4L2_TUNER_RF)
>>>>>> +			return -EINVAL;
>>>>>
>>>>> This is wrong. As you mentioned in patch 1, the type field should always be set by
>>>>> the driver. So type is not something that is set by the user.
>>>>>
>>>>> I would just set type to V4L2_TUNER_ADC here (all SDR devices have at least an ADC
>>>>> tuner), and let the driver change it to TUNER_RF if this tuner is really an RF
>>>>> tuner.
>>>>
>>>> I don't think so. It sounds very stupid to handle tuner type with
>>>> different meaning in that single case - it sounds just a is a mistake
>>>> (and that SDR case mistakes are not needed continue as no regressions
>>>> apply). I can say I was very puzzled what is the reason my tuner type is
>>>> always changed to wrong, until finally found it was overridden here.
>>>>
>>>> For me this looks more than it is just forced to "some" suitable value
>>>> in a case app does not fill it correctly - not the way driver should
>>>> return it to app. Tuner ID and type are here for Kernel driver could
>>>> identify not the opposite and that is how it should be without unneeded
>>>> exceptions.
>>>>
>>>> Also, API does not specify that kind of different meaning for tuner type
>>>> in a case of g_frequency.
>>>>
>>>> Have to search some history where that odds is coming from...
>>>
>>> The application *does not set type* when calling G_FREQUENCY. The driver has
>>> to fill that in. So the type field as received from the application is
>>> uninitialized. That's the way the spec was defined, and that's the way
>>> applications use G_FREQUENCY. There is nothing you can do about that.
>>>
>>> So drivers have to fill in the type based on vfl_type and the tuner index.
>>> Since drivers often didn't do that the vfl_type check has been moved to the
>>> v4l2 core. In the case of SDR the type is actually dependent on the tuner
>>> index, so the core cannot fully initialize the type field.
>>>
>>> You can either leave it uninitialized for vfl_type SDR and leave it to the
>>> SDR driver to fill in the type, or you can set it to ADC so the driver
>>> only has to update the type field if the tuner index corresponds to the
>>> RF tuner.
>>
>>
>> commit 227690df75382e46a4f6ea1bbc5df855a674b47f
>> Author: Hans Verkuil <hans.verkuil@cisco.com>
>> Date:   Sun Jun 12 06:36:41 2011 -0300
>>
>>       [media] v4l2-ioctl.c: prefill tuner type for g_frequency and g/s_tuner
>>
>>       The subdevs are supposed to receive a valid tuner type for the
>> g_frequency
>
> This talks about the *subdevs*. The low-level tuner ops implemented by
> sub-devices expected a valid type field. That field had to be filled in
> by bridge drivers, and they often did not do that, or filled in the wrong
> type.
>
>>       and g/s_tuner subdev ops. Some drivers do this, others don't. So
>> prefill
>>       this in v4l2-ioctl.c based on whether the device node from which
>> this is
>>       called is a radio node or not.
>>
>>       The spec does not require applications to fill in the type, and if they
>         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Yes, I saw it was not required. And thats why I saw now it is possible 
to fix in a case of SDR device. You can check if it is set correctly 
without fear of possible regressions. Implementation I made left 
functionality for any other devices than SDR to old "don't care about 
tuner type".

>>       leave it at 0 then the 'check_mode' call in tuner-core.c will return
>>       an error and the ioctl does nothing.
>>
>>       Cc: stable@kernel.org
>>       Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
>>       Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
>>

That is the patch where tuner type overridden (for g_frequency) was 
added. It does not say type is field aimed for reporting type to 
application, instead it says Kernel driver needs it and as applications 
are not setting always, just override some reasonable values.

I cannot see why you are against proper validation of tuner type got 
from app in a case of g_frequency (and it looks even more strange as 
type is validated for s_frequency).

regards
Antti

-- 
http://palosaari.fi/

Re: [PATCH RFC v2 3/7] v4l: add new tuner types for SDR

[Hans Verkuil]

On 12/16/2013 03:41 PM, Antti Palosaari wrote:
> On 16.12.2013 16:25, Hans Verkuil wrote:
>> On 12/16/2013 03:19 PM, Antti Palosaari wrote:
>>> On 16.12.2013 14:50, Hans Verkuil wrote:
>>>> On 12/16/2013 01:36 PM, Antti Palosaari wrote:
>>>>> On 16.12.2013 10:53, Hans Verkuil wrote:
>>>>>> On 12/14/2013 05:15 PM, Antti Palosaari wrote:
>>>>>
>>>>>>> @@ -1288,8 +1288,13 @@ static int v4l_g_frequency(const struct v4l2_ioctl_ops *ops,
>>>>>>>     	struct video_device *vfd = video_devdata(file);
>>>>>>>     	struct v4l2_frequency *p = arg;
>>>>>>>
>>>>>>> -	p->type = (vfd->vfl_type == VFL_TYPE_RADIO) ?
>>>>>>> -			V4L2_TUNER_RADIO : V4L2_TUNER_ANALOG_TV;
>>>>>>> +	if (vfd->vfl_type == VFL_TYPE_SDR) {
>>>>>>> +		if (p->type != V4L2_TUNER_ADC && p->type != V4L2_TUNER_RF)
>>>>>>> +			return -EINVAL;
>>>>>>
>>>>>> This is wrong. As you mentioned in patch 1, the type field should always be set by
>>>>>> the driver. So type is not something that is set by the user.
>>>>>>
>>>>>> I would just set type to V4L2_TUNER_ADC here (all SDR devices have at least an ADC
>>>>>> tuner), and let the driver change it to TUNER_RF if this tuner is really an RF
>>>>>> tuner.
>>>>>
>>>>> I don't think so. It sounds very stupid to handle tuner type with
>>>>> different meaning in that single case - it sounds just a is a mistake
>>>>> (and that SDR case mistakes are not needed continue as no regressions
>>>>> apply). I can say I was very puzzled what is the reason my tuner type is
>>>>> always changed to wrong, until finally found it was overridden here.
>>>>>
>>>>> For me this looks more than it is just forced to "some" suitable value
>>>>> in a case app does not fill it correctly - not the way driver should
>>>>> return it to app. Tuner ID and type are here for Kernel driver could
>>>>> identify not the opposite and that is how it should be without unneeded
>>>>> exceptions.
>>>>>
>>>>> Also, API does not specify that kind of different meaning for tuner type
>>>>> in a case of g_frequency.
>>>>>
>>>>> Have to search some history where that odds is coming from...
>>>>
>>>> The application *does not set type* when calling G_FREQUENCY. The driver has
>>>> to fill that in. So the type field as received from the application is
>>>> uninitialized. That's the way the spec was defined, and that's the way
>>>> applications use G_FREQUENCY. There is nothing you can do about that.
>>>>
>>>> So drivers have to fill in the type based on vfl_type and the tuner index.
>>>> Since drivers often didn't do that the vfl_type check has been moved to the
>>>> v4l2 core. In the case of SDR the type is actually dependent on the tuner
>>>> index, so the core cannot fully initialize the type field.
>>>>
>>>> You can either leave it uninitialized for vfl_type SDR and leave it to the
>>>> SDR driver to fill in the type, or you can set it to ADC so the driver
>>>> only has to update the type field if the tuner index corresponds to the
>>>> RF tuner.
>>>
>>>
>>> commit 227690df75382e46a4f6ea1bbc5df855a674b47f
>>> Author: Hans Verkuil <hans.verkuil@cisco.com>
>>> Date:   Sun Jun 12 06:36:41 2011 -0300
>>>
>>>       [media] v4l2-ioctl.c: prefill tuner type for g_frequency and g/s_tuner
>>>
>>>       The subdevs are supposed to receive a valid tuner type for the
>>> g_frequency
>>
>> This talks about the *subdevs*. The low-level tuner ops implemented by
>> sub-devices expected a valid type field. That field had to be filled in
>> by bridge drivers, and they often did not do that, or filled in the wrong
>> type.
>>
>>>       and g/s_tuner subdev ops. Some drivers do this, others don't. So
>>> prefill
>>>       this in v4l2-ioctl.c based on whether the device node from which
>>> this is
>>>       called is a radio node or not.
>>>
>>>       The spec does not require applications to fill in the type, and if they
>>         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 
> Yes, I saw it was not required. And thats why I saw now it is possible 
> to fix in a case of SDR device. You can check if it is set correctly 
> without fear of possible regressions. Implementation I made left 
> functionality for any other devices than SDR to old "don't care about 
> tuner type".

So what you are basically doing is that for non-SDR device nodes the
type field is just ignored (as per the spec) and filled-in by the core,
while for SDR device nodes you check what the application gives you.

In other words, the G_FREQUENCY ioctl behaves differently depending on
the device node you use (for sdr nodes it checks type, for others it
ignores type). That's highly unexpected and inconsistent, and that's
not the way to go.

> 
>>>       leave it at 0 then the 'check_mode' call in tuner-core.c will return
>>>       an error and the ioctl does nothing.
>>>
>>>       Cc: stable@kernel.org
>>>       Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
>>>       Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
>>>
> 
> That is the patch where tuner type overridden (for g_frequency) was 
> added. It does not say type is field aimed for reporting type to 
> application, instead it says Kernel driver needs it and as applications 
> are not setting always, just override some reasonable values.

That's not what it says. I quote: "The spec does not require applications to
fill in the type". So 'type' is *undefined*. But the low-level sub-device
drivers like tuners *do* expect type to be set by someone. That someone was the
bridge driver. That patch just sets the type in the core code, since it can
determine the type from vfl_type. So no bridge drivers had to be modified,
and sub-device drivers now receive a valid type.

> 
> I cannot see why you are against proper validation of tuner type got 
> from app in a case of g_frequency (and it looks even more strange as 
> type is validated for s_frequency).

Because for G_FREQUENCY the type field is not written by the application.
I can't help that, that's the way the spec was defined (and I agree that
it is weird).

It's been like that for many years, and we can't change it. And we don't
need to change it either, since your sdr driver can set the type based
on the tuner index.

You should drop patch 1, because that suggests that the type field as
passed in from the application can be used by the driver, and that's
simply not the case.

Regards,

	Hans

Re: [PATCH RFC v2 0/7] V4L2 SDR API

[Antti Palosaari]

On 15.12.2013 13:30, Mauro Carvalho Chehab wrote:
> Em Sat, 14 Dec 2013 19:47:45 +0200
> Antti Palosaari <crope@iki.fi> escreveu:
>
>> On 14.12.2013 19:05, Hans Verkuil wrote:
>>> On 12/14/2013 05:45 PM, Antti Palosaari wrote:

> I didn't like much that now have 3 ways to describe frequencies.
> I think we should latter think on moving the frequency conversion to
> the core, and use u64 with 1Hz step at the internal API, converting all
> the drivers to use it.
>
> IMHO, we should also provide a backward-compatible way that would allow
> userspace to choose to use u64 1-Hz-stepping frequencies.
>
> Of course the changes at the drivers is out of the scope, but perhaps
> we should not apply patch 4/7, replacing it, instead, by some patch that
> would move the frequency size to u64.

Frequency is defined by that structure.

struct v4l2_frequency {
	__u32	tuner;
	__u32	type;	/* enum v4l2_tuner_type */
	__u32	frequency;
	__u32	reserved[8];
};


Is it possible to somehow use reserved bytes to extend value to 64. Then 
change that 1-Hz flag (rename it) to signal it is 64?

Or add some info to that struct itself? Define both frequency and 
frequency64 and use the one which is not zero?

If implementation will not be very complex I could try to do it it the 
same time with other changes.

regards
Antti

-- 
http://palosaari.fi/

Re: [PATCH RFC v2 0/7] V4L2 SDR API

[Hans Verkuil]

On 12/16/2013 05:50 PM, Antti Palosaari wrote:
> On 15.12.2013 13:30, Mauro Carvalho Chehab wrote:
>> Em Sat, 14 Dec 2013 19:47:45 +0200
>> Antti Palosaari <crope@iki.fi> escreveu:
>>
>>> On 14.12.2013 19:05, Hans Verkuil wrote:
>>>> On 12/14/2013 05:45 PM, Antti Palosaari wrote:
> 
>> I didn't like much that now have 3 ways to describe frequencies.
>> I think we should latter think on moving the frequency conversion to
>> the core, and use u64 with 1Hz step at the internal API, converting all
>> the drivers to use it.
>>
>> IMHO, we should also provide a backward-compatible way that would allow
>> userspace to choose to use u64 1-Hz-stepping frequencies.
>>
>> Of course the changes at the drivers is out of the scope, but perhaps
>> we should not apply patch 4/7, replacing it, instead, by some patch that
>> would move the frequency size to u64.
> 
> Frequency is defined by that structure.
> 
> struct v4l2_frequency {
> 	__u32	tuner;
> 	__u32	type;	/* enum v4l2_tuner_type */
> 	__u32	frequency;
> 	__u32	reserved[8];
> };
> 
> 
> Is it possible to somehow use reserved bytes to extend value to 64. Then 
> change that 1-Hz flag (rename it) to signal it is 64?
> 
> Or add some info to that struct itself? Define both frequency and 
> frequency64 and use the one which is not zero?
> 
> If implementation will not be very complex I could try to do it it the 
> same time with other changes.

I'm inclined not to make any changes. If 32 bits becomes insufficient, then
I would just add a "__u32 frequency_high" field to store the top 32 bits. Or
would "frequency_msb" be a better name?

While I do like the idea to use a 64-bit frequency internally, I am afraid
of touching existing frequency calculation code. It is too easy to make
mistakes and introduce regressions when converting from 62.5 Hz or kHz
units to 1 Hz units.

Personally I do not think it is worth the effort. There is a clean way
of going to 64 bit frequencies should we need it in the future, but that's
not needed today. Note that going to 64 bit frequencies would also require
changes to struct v4l2_tuner and struct v4l2_frequency_band for the rangelow
and rangehigh frequencies.

There is room in both for rangelow_msb and rangehigh_msb fields, so we are
good there. Hmm, an _msb suffix would be better than a _high suffix:
rangehigh_high looks really weird :-)

Regards,

	Hans