v4l2: add hardware frequency seek ioctl interface

Seems fine to me to have an specific ioctl for doing radio frequency seeks.

Yes. API should describe how this works.

It is good to have a patch to a real driver, implementing this feature. I don't like the idea of implementing newer ioctls at the API without having an in-kernel driver using. Having the driver ioctl implementation helps other developers that may need to use this interface on other places.

Cheers,
Mauro

--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Hi,

That seems to be a good approach. Is there any documentation on how the interface is currently implemented?

I would be very happy to add this functionality to my radio-si470x.c driver. Seek support can be parameterized with this device in many ways.
This is all described in a document from Silabs (available via Google) called AN284Rev0_1.pdf

These parameters are not only used when seeking up/down, but also during frequency tuning. They are already implemented as module parameters,
as they are country specific and should not be changed during normal operation:
- Band selection (upper/lower frequency limits)
- Spacing selection
- De-emphasis selection

Additionally these parameters are only used by the seek algorithm:
- RSSI Seek Threshold (range: 0..254, 254=highest threshold)
- Signal-Noise-Ratio (range: 0..15, 15=higest SNR ratio)
- FM-Impulse Noise Detection Counter (range: 0..15, 15=best audio quality)

Propably it is wise to give the user space applications the possiblity to change these parameters at run time (ioctl). Else I'll implement them as module parameters, too.

Bye,
Toby

--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Please don't implement things like this as module parameters. What happens when you have two device plugged in and want different values?

Example case: Drivers that use module parameters for devices without EEPROMs. What happens when I have two different models without EEPROMs in my box at the same time.

It would be far better to see these things defined per device in sysfs if you can't manage an IOCTL. However, in this case it should be an IOCTL.

Cheers,

Brandon

--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Agreed.

The better would be to use VIDIOC_[G/S/ENUM]_CTRL to allow specifying those parameters.

Cheers,
Mauro

--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Hi Keith,

I fully agree, that having something different then an ioctl to control the seek behavior is not the right way.

That sounds, like there's already a proposal.
Can you provide me with the current patches and documentation?
They don't seem to be part of the V4L2 documentation, the mercurial V4L or the Linux git repository yet.

The answer to that question depends on what's already planned or probably already implemented.

As said, the SI470x has several options to change the seek behavior:

I would implement these features as recommended by Mauro using VIDIOC_[G/S/ENUM]_CTRL. Especially I need some additional V4L2_CID_* for it.
When named corresponding to the register names, they would look like this:
#define V4L2_CID_SEEKTH (V4L2_CID_BASE+24) /* RSSI Seek Threshold */ #define V4L2_CID_SKSNR (V4L2_CID_BASE+25) /* Seek SNR Threshold */ #define V4L2_CID_SKCNT (V4L2_CID_BASE+26) /* Seek FM Impulse Detection Threshold */

The implementation is then rather simple and will provide proper default settings for these registers. I guess, the patch has less then 50 code lines.

More problematic is to patch one of the radio applications to test the new seek behavior.

Another question: If we implement a dedicated ioctl for seek operations,
will we also implement a compatibility function, that can be used for all devices,
that doesn't have hardware seek functionality?
I mean, a radio application should be able to just call the seek ioctl, independent of whether the hardware supports it or not. If a specific version is available to support the device's hardware seek functionality, then that's fine and we use it. If the hardware doesn't support seek operations, then we have the compatibility function,
that executes the usual "tune and check for signal strength" operation, as currently done in every application. So we basically should move the functionality from the application to the kernel.

Bye,
Toby

--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Please don't emulate hardware features in the Kernel.

The V4L layer should do its best to expose all features of a device. However, if a device needs emulation, translation or conversion of any sort it should be done in userspace- preferably in a library.

Plus, I am sure the application developers would love to see a libradio for V4L radio devices to simplify their apps :D

Cheers,

Brandon

--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Hi Keith,

Sorry, I have not found it on the v4l mailing list.

Therefore I would not implement this as ioctls, but using VIDIOC_[G/S/ENUM]_CTRL. #define V4L2_CID_SEEKTH (V4L2_CID_BASE+24) /* RSSI Seek Threshold */ #define V4L2_CID_SKSNR (V4L2_CID_BASE+25) /* Seek SNR Threshold */ #define V4L2_CID_SKCNT (V4L2_CID_BASE+26) /* Seek FM Impulse Detection Threshold */ But this is not done in the current implementation of the driver radio-si470x. The device is currently just initialized using the default/recommended parameters.

@@ -301,6 +301,8 @@
                                        struct v4l2_frequency *a);
        int (*vidioc_s_frequency)      (struct file *file, void *fh,
                                        struct v4l2_frequency *a);
+       int (*vidioc_s_hw_freq_seek)   (struct file *file, void *fh,
+                                       struct v4l2_hw_freq_seek *a);
 
        /* Sliced VBI cap */
        int (*vidioc_g_sliced_vbi_cap) (struct file *file, void *fh,