Re: [PATCH 1/2] cx23885 Radio Support [was: cx23885: Add basic analog radio support]
Hi Hans El 20/12/13 06:54, Hans Verkuil escribió: Hi Alfredo, It's a rather late review for which I apologize. Anyway, this patch needs more work, see my comments below. Hans thank you very much for your comments. I'll have to wait until January to work with your suggestions, because I'm very busy right now. Thanks again, Alfredo -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] cx23885 Radio Support [was: cx23885: Add basic analog radio support]
Hi Alfredo,
It's a rather late review for which I apologize.
Anyway, this patch needs more work, see my comments below.
On 11/13/2013 03:52 PM, Alfredo Jesús Delaiti wrote:
> Hi Mauro and all
>
> El 31/10/13 07:12, Mauro Carvalho Chehab escribió:
>>
>> Hi Alfredo,
>>
>> My understanding is that the patch you've enclosed is incomplete and
>> depends on Miroslav's patch.
>>
>> As he have you his ack to rework on it, could you please prepare a
>> patch series addressing the above comments for us to review?
>>
>> Than
>
> This patch supports only radio is for CX23885
>
> I've only applied to current git version of V4L, and I've also removed the
> lines that support two cards in particular.
>
> The original patch is: https://linuxtv.org/patch/9498/
>
> I found the following issue
>
> Details of the issue:
>
> Some warning when compiling
>
> ...
>CC [M] /home/alfredo/ISDB/Nuevo_Driver/git/media_build/v4l/cx23885-video.o
> /home/alfredo/ISDB/Nuevo_Driver/git/media_build/v4l/cx23885-video.c:1910:8: :
> initialization from incompatible pointer type [enabled by default]
> /home/alfredo/ISDB/Nuevo_Driver/git/media_build/v4l/cx23885-video.c:1910:8:
> warning: (near initialization for 'radio_ioctl_ops.vidioc_s_tuner') [enabled
> by default]
> /home/alfredo/ISDB/Nuevo_Driver/git/media_build/v4l/cx23885-video.c:1911:8:
> warning: initialization from incompatible pointer type [enabled by default]
> /home/alfredo/ISDB/Nuevo_Driver/git/media_build/v4l/cx23885-video.c:1911:8:
> warning: (near initialization for 'radio_ioctl_ops.vidioc_s_audio') [enabled
> by default]
>CC [M] /home/alfredo/ISDB/Nuevo_Driver/git/media_build/v4l/cx23885-vbi.o
> ...
>
>
> static const struct v4l2_ioctl_ops radio_ioctl_ops = {
>
> .vidioc_s_tuner = radio_s_tuner, /* line 1910 */
> .vidioc_s_audio = radio_s_audio, /* line 1911 */
>
>
>
>
>
> Signed-off-by: Miroslav Slugen
> Tested-by: Alfredo J. Delaiti
>
> diff --git a/drivers/media/pci/cx23885/cx23885-video.c
> b/drivers/media/pci/cx23885/cx23885-video.c
> index 7891f34..9eed6fe 100644
> --- a/drivers/media/pci/cx23885/cx23885-video.c
> +++ b/drivers/media/pci/cx23885/cx23885-video.c
> @@ -889,6 +889,8 @@ static int video_open(struct file *file)
> fh->width= 320;
> fh->height = 240;
> fh->fmt = format_by_fourcc(V4L2_PIX_FMT_YUYV);
> +
> + mutex_lock(&dev->lock);
>
> videobuf_queue_sg_init(&fh->vidq, &cx23885_video_qops,
> &dev->pci->dev, &dev->slock,
> @@ -904,6 +906,14 @@ static int video_open(struct file *file)
> sizeof(struct cx23885_buffer),
> fh, NULL);
>
> + if (fh->radio) {
> + dprintk(1,"video_open: setting radio device\n");
> + cx_write(GPIO_0, cx23885_boards[dev->board].radio.gpio0);
> + call_all(dev, tuner, s_radio);
> + }
> +
> + dev->users++;
> + mutex_unlock(&dev->lock);
>
> dprintk(1, "post videobuf_queue_init()\n");
>
> @@ -1001,15 +1011,26 @@ static int video_release(struct file *file)
>
> videobuf_mmap_free(&fh->vidq);
> videobuf_mmap_free(&fh->vbiq);
> +
> + mutex_lock(&dev->lock);
>
> file->private_data = NULL;
> kfree(fh);
> +
> + dev->users--;
>
> /* We are not putting the tuner to sleep here on exit, because
>* we want to use the mpeg encoder in another session to capture
>* tuner video. Closing this will result in no video to the encoder.
>*/
>
> +#if 0
> + if (!dev->users)
> + call_all(dev, core, s_power, 0);
> + #endif
Please intend #endif at the beginning of the line, I originally missed it when
reviewing the code because it didn't line up with the #if 0.
Why is it #if 0 anyway? Should it perhaps just be removed altogether?
> +
> + mutex_unlock(&dev->lock);
> +
> return 0;
> }
>
> @@ -1635,6 +1656,106 @@ static int vidioc_s_frequency(struct file *file, void
> *priv,
>
> /* --- */
>
> +/* RADIO ESPECIFIC IOCTLS */
> +
> +static int radio_querycap (struct file *file, void *priv,
> + struct v4l2_capability *cap)
> +{
> + struct cx23885_dev *dev = ((struct cx23885_fh *)priv)->dev;
> +
> + strcpy(cap->driver, "cx23885");
> + strlcpy(cap->card, cx23885_boards[dev->board].name,
> sizeof(cap->card));
> + sprintf(cap->bus_info,"PCIe:%s", pci_name(dev->pci));
> + cap->capabilities = V4L2_CAP_TUNER;
Add V4L2_CAP_RADIO as well.
> + return 0;
> +}
> +
> +static int radio_g_tuner (struct file *file, void *priv,
> + struct v4l2_tuner *t)
> +{
> + struct cx23885_dev *dev = ((struct cx23885_fh *)priv)->dev;
> +
[PATCH 1/2] cx23885 Radio Support [was: cx23885: Add basic analog radio support]
Hi Mauro and all
El 31/10/13 07:12, Mauro Carvalho Chehab escribió:
>
> Hi Alfredo,
>
> My understanding is that the patch you've enclosed is incomplete and
> depends on Miroslav's patch.
>
> As he have you his ack to rework on it, could you please prepare a
> patch series addressing the above comments for us to review?
>
> Than
This patch supports only radio is for CX23885
I've only applied to current git version of V4L, and I've also removed the
lines that support two cards in particular.
The original patch is: https://linuxtv.org/patch/9498/
I found the following issue
Details of the issue:
Some warning when compiling
...
CC [M] /home/alfredo/ISDB/Nuevo_Driver/git/media_build/v4l/cx23885-video.o
/home/alfredo/ISDB/Nuevo_Driver/git/media_build/v4l/cx23885-video.c:1910:8: :
initialization from incompatible pointer type [enabled by default]
/home/alfredo/ISDB/Nuevo_Driver/git/media_build/v4l/cx23885-video.c:1910:8:
warning: (near initialization for 'radio_ioctl_ops.vidioc_s_tuner') [enabled by
default]
/home/alfredo/ISDB/Nuevo_Driver/git/media_build/v4l/cx23885-video.c:1911:8:
warning: initialization from incompatible pointer type [enabled by default]
/home/alfredo/ISDB/Nuevo_Driver/git/media_build/v4l/cx23885-video.c:1911:8:
warning: (near initialization for 'radio_ioctl_ops.vidioc_s_audio') [enabled by
default]
CC [M] /home/alfredo/ISDB/Nuevo_Driver/git/media_build/v4l/cx23885-vbi.o
...
static const struct v4l2_ioctl_ops radio_ioctl_ops = {
.vidioc_s_tuner = radio_s_tuner, /* line 1910 */
.vidioc_s_audio = radio_s_audio, /* line 1911 */
Signed-off-by: Miroslav Slugen
Tested-by: Alfredo J. Delaiti
diff --git a/drivers/media/pci/cx23885/cx23885-video.c b/drivers/media/pci/cx23885/cx23885-video.c
index 7891f34..9eed6fe 100644
--- a/drivers/media/pci/cx23885/cx23885-video.c
+++ b/drivers/media/pci/cx23885/cx23885-video.c
@@ -889,6 +889,8 @@ static int video_open(struct file *file)
fh->width= 320;
fh->height = 240;
fh->fmt = format_by_fourcc(V4L2_PIX_FMT_YUYV);
+
+ mutex_lock(&dev->lock);
videobuf_queue_sg_init(&fh->vidq, &cx23885_video_qops,
&dev->pci->dev, &dev->slock,
@@ -904,6 +906,14 @@ static int video_open(struct file *file)
sizeof(struct cx23885_buffer),
fh, NULL);
+ if (fh->radio) {
+ dprintk(1,"video_open: setting radio device\n");
+ cx_write(GPIO_0, cx23885_boards[dev->board].radio.gpio0);
+ call_all(dev, tuner, s_radio);
+ }
+
+ dev->users++;
+ mutex_unlock(&dev->lock);
dprintk(1, "post videobuf_queue_init()\n");
@@ -1001,15 +1011,26 @@ static int video_release(struct file *file)
videobuf_mmap_free(&fh->vidq);
videobuf_mmap_free(&fh->vbiq);
+
+ mutex_lock(&dev->lock);
file->private_data = NULL;
kfree(fh);
+
+ dev->users--;
/* We are not putting the tuner to sleep here on exit, because
* we want to use the mpeg encoder in another session to capture
* tuner video. Closing this will result in no video to the encoder.
*/
+#if 0
+ if (!dev->users)
+ call_all(dev, core, s_power, 0);
+ #endif
+
+ mutex_unlock(&dev->lock);
+
return 0;
}
@@ -1635,6 +1656,106 @@ static int vidioc_s_frequency(struct file *file, void *priv,
/* --- */
+/* RADIO ESPECIFIC IOCTLS */
+
+static int radio_querycap (struct file *file, void *priv,
+ struct v4l2_capability *cap)
+{
+ struct cx23885_dev *dev = ((struct cx23885_fh *)priv)->dev;
+
+ strcpy(cap->driver, "cx23885");
+ strlcpy(cap->card, cx23885_boards[dev->board].name, sizeof(cap->card));
+ sprintf(cap->bus_info,"PCIe:%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)
+{
+ struct cx23885_dev *dev = ((struct cx23885_fh *)priv)->dev;
+
+ if (unlikely(t->index > 0))
+ return -EINVAL;
+
+ strcpy(t->name, "Radio");
+ t->type = V4L2_TUNER_RADIO;
+
+ call_all(dev, tuner, g_tuner, t);
+ return 0;
+}
+
+static int radio_enum_input (struct file *file, void *priv,
+ struct v4l2_input *i)
+{
+ if (i->index != 0)
+ return -EINVAL;
+ strcpy(i->name,"Radio");
+ i->type = V4L2_INPUT_TYPE_TUNER;
+
+ return 0;
+}
+
+static int radio_g_audio (struct file *file, void *priv, struct v4l2_audio *a)
+{
+ if (unlikely(a->index))
+ return -EINVAL;
+
+ strcpy(a->name,"Radio");
+ return 0;
+}
+
+/* FIXME: Should add a standard for radio */
+
+static int radio_s_tuner (struct file *file, void *priv,
+
