[PATCH] tuner-core: return tuner name with ioctl VIDIOC_G_TUNER

[PATCH] tuner-core: return tuner name with ioctl VIDIOC_G_TUNER

[Frank Schäfer]

tuner_g_tuner() is supposed to fill struct v4l2_tuner passed by ioctl
VIDIOC_G_TUNER, but misses setting the name field.

Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
Cc: stable@kernel.org
---
 drivers/media/v4l2-core/tuner-core.c |    1 +
 1 Datei geändert, 1 Zeile hinzugefügt(+)

diff --git a/drivers/media/v4l2-core/tuner-core.c b/drivers/media/v4l2-core/tuner-core.c
index b5a819a..95a47cf 100644
--- a/drivers/media/v4l2-core/tuner-core.c
+++ b/drivers/media/v4l2-core/tuner-core.c
@@ -1187,6 +1187,7 @@ static int tuner_g_tuner(struct v4l2_subdev *sd, struct v4l2_tuner *vt)
 
 	if (check_mode(t, vt->type) == -EINVAL)
 		return 0;
+	strcpy(vt->name, t->name);
 	if (vt->type == t->mode && analog_ops->get_afc)
 		vt->afc = analog_ops->get_afc(&t->fe);
 	if (analog_ops->has_signal)
-- 
1.7.10.4

Re: [PATCH] tuner-core: return tuner name with ioctl VIDIOC_G_TUNER

[Hans Verkuil]

On Tue January 22 2013 20:46:21 Frank Schäfer wrote:
> tuner_g_tuner() is supposed to fill struct v4l2_tuner passed by ioctl
> VIDIOC_G_TUNER, but misses setting the name field.
> 
> Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
> Cc: stable@kernel.org
> ---
>  drivers/media/v4l2-core/tuner-core.c |    1 +
>  1 Datei geändert, 1 Zeile hinzugefügt(+)
> 
> diff --git a/drivers/media/v4l2-core/tuner-core.c b/drivers/media/v4l2-core/tuner-core.c
> index b5a819a..95a47cf 100644
> --- a/drivers/media/v4l2-core/tuner-core.c
> +++ b/drivers/media/v4l2-core/tuner-core.c
> @@ -1187,6 +1187,7 @@ static int tuner_g_tuner(struct v4l2_subdev *sd, struct v4l2_tuner *vt)
>  
>  	if (check_mode(t, vt->type) == -EINVAL)
>  		return 0;
> +	strcpy(vt->name, t->name);
>  	if (vt->type == t->mode && analog_ops->get_afc)
>  		vt->afc = analog_ops->get_afc(&t->fe);
>  	if (analog_ops->has_signal)
> 

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

And the reason is that the tuner field should be filled in by the bridge
driver. That's because you may have multiple tuners and it's only the
bridge driver that will know which tuner is which and what name to give
it.

Regards,

	Hans

Re: [PATCH] tuner-core: return tuner name with ioctl VIDIOC_G_TUNER

[Frank Schäfer]

Am 23.01.2013 08:35, schrieb Hans Verkuil:
> On Tue January 22 2013 20:46:21 Frank Schäfer wrote:
>> tuner_g_tuner() is supposed to fill struct v4l2_tuner passed by ioctl
>> VIDIOC_G_TUNER, but misses setting the name field.
>>
>> Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
>> Cc: stable@kernel.org
>> ---
>>  drivers/media/v4l2-core/tuner-core.c |    1 +
>>  1 Datei geändert, 1 Zeile hinzugefügt(+)
>>
>> diff --git a/drivers/media/v4l2-core/tuner-core.c b/drivers/media/v4l2-core/tuner-core.c
>> index b5a819a..95a47cf 100644
>> --- a/drivers/media/v4l2-core/tuner-core.c
>> +++ b/drivers/media/v4l2-core/tuner-core.c
>> @@ -1187,6 +1187,7 @@ static int tuner_g_tuner(struct v4l2_subdev *sd, struct v4l2_tuner *vt)
>>  
>>  	if (check_mode(t, vt->type) == -EINVAL)
>>  		return 0;
>> +	strcpy(vt->name, t->name);
>>  	if (vt->type == t->mode && analog_ops->get_afc)
>>  		vt->afc = analog_ops->get_afc(&t->fe);
>>  	if (analog_ops->has_signal)
>>
> Nacked-by: Hans Verkuil <hans.verkuil@cisco.com>
>
> And the reason is that the tuner field should be filled in by the bridge
> driver. That's because you may have multiple tuners and it's only the
> bridge driver that will know which tuner is which and what name to give
> it.

Hmmm... I don't understand.
Isn't his a per-tuner (subdev) operation ? If a device has multiple
tuners (subdevs) it is called for each of them.
So how can the returned tuner name be wrong and why should the bridge
driver know better than the subdevice itself which name is correct ?

Regards,
Frank


> Regards,
>
> 	Hans

Re: [PATCH] tuner-core: return tuner name with ioctl VIDIOC_G_TUNER

[Hans Verkuil]

On Wed January 23 2013 16:57:40 Frank Schäfer wrote:
> Am 23.01.2013 08:35, schrieb Hans Verkuil:
> > On Tue January 22 2013 20:46:21 Frank Schäfer wrote:
> >> tuner_g_tuner() is supposed to fill struct v4l2_tuner passed by ioctl
> >> VIDIOC_G_TUNER, but misses setting the name field.
> >>
> >> Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
> >> Cc: stable@kernel.org
> >> ---
> >>  drivers/media/v4l2-core/tuner-core.c |    1 +
> >>  1 Datei geändert, 1 Zeile hinzugefügt(+)
> >>
> >> diff --git a/drivers/media/v4l2-core/tuner-core.c b/drivers/media/v4l2-core/tuner-core.c
> >> index b5a819a..95a47cf 100644
> >> --- a/drivers/media/v4l2-core/tuner-core.c
> >> +++ b/drivers/media/v4l2-core/tuner-core.c
> >> @@ -1187,6 +1187,7 @@ static int tuner_g_tuner(struct v4l2_subdev *sd, struct v4l2_tuner *vt)
> >>  
> >>  	if (check_mode(t, vt->type) == -EINVAL)
> >>  		return 0;
> >> +	strcpy(vt->name, t->name);
> >>  	if (vt->type == t->mode && analog_ops->get_afc)
> >>  		vt->afc = analog_ops->get_afc(&t->fe);
> >>  	if (analog_ops->has_signal)
> >>
> > Nacked-by: Hans Verkuil <hans.verkuil@cisco.com>
> >
> > And the reason is that the tuner field should be filled in by the bridge
> > driver. That's because you may have multiple tuners and it's only the
> > bridge driver that will know which tuner is which and what name to give
> > it.
> 
> Hmmm... I don't understand.
> Isn't his a per-tuner (subdev) operation ? If a device has multiple
> tuners (subdevs) it is called for each of them.
> So how can the returned tuner name be wrong and why should the bridge
> driver know better than the subdevice itself which name is correct ?

The name that's filled in is exposed to userspace, so it should be something
meaningful and not some internal name. In the case of multiple tuners that
means that the name should be something like 'TV 1' or 'TV 2', where the name
matches a name (label) of a tuner input of the product. There is no way a
subdev driver can know that, only the bridge driver knows what product it is
and thus what the labels on the inputs are.

It is somewhat theoretical since we don't have any multi-tuner devices (yet),
so for now names like 'Radio' and 'TV' are sufficient, but we made the same
mistake (letting subdevs set the name) in the past for regular video/audio
inputs and outputs and it took a lot of work to fix that.

The golden rule is that sub-devices should not assume anything about how they
are hooked up in the actual product.

Regards,

	Hans

Re: [PATCH] tuner-core: return tuner name with ioctl VIDIOC_G_TUNER

[Frank Schäfer]

Am 23.01.2013 17:13, schrieb Hans Verkuil:
> On Wed January 23 2013 16:57:40 Frank Schäfer wrote:
>> Am 23.01.2013 08:35, schrieb Hans Verkuil:
>>> On Tue January 22 2013 20:46:21 Frank Schäfer wrote:
>>>> tuner_g_tuner() is supposed to fill struct v4l2_tuner passed by ioctl
>>>> VIDIOC_G_TUNER, but misses setting the name field.
>>>>
>>>> Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
>>>> Cc: stable@kernel.org
>>>> ---
>>>>  drivers/media/v4l2-core/tuner-core.c |    1 +
>>>>  1 Datei geändert, 1 Zeile hinzugefügt(+)
>>>>
>>>> diff --git a/drivers/media/v4l2-core/tuner-core.c b/drivers/media/v4l2-core/tuner-core.c
>>>> index b5a819a..95a47cf 100644
>>>> --- a/drivers/media/v4l2-core/tuner-core.c
>>>> +++ b/drivers/media/v4l2-core/tuner-core.c
>>>> @@ -1187,6 +1187,7 @@ static int tuner_g_tuner(struct v4l2_subdev *sd, struct v4l2_tuner *vt)
>>>>  
>>>>  	if (check_mode(t, vt->type) == -EINVAL)
>>>>  		return 0;
>>>> +	strcpy(vt->name, t->name);
>>>>  	if (vt->type == t->mode && analog_ops->get_afc)
>>>>  		vt->afc = analog_ops->get_afc(&t->fe);
>>>>  	if (analog_ops->has_signal)
>>>>
>>> Nacked-by: Hans Verkuil <hans.verkuil@cisco.com>
>>>
>>> And the reason is that the tuner field should be filled in by the bridge
>>> driver. That's because you may have multiple tuners and it's only the
>>> bridge driver that will know which tuner is which and what name to give
>>> it.
>> Hmmm... I don't understand.
>> Isn't his a per-tuner (subdev) operation ? If a device has multiple
>> tuners (subdevs) it is called for each of them.
>> So how can the returned tuner name be wrong and why should the bridge
>> driver know better than the subdevice itself which name is correct ?
> The name that's filled in is exposed to userspace, so it should be something
> meaningful and not some internal name. In the case of multiple tuners that
> means that the name should be something like 'TV 1' or 'TV 2', where the name
> matches a name (label) of a tuner input of the product. There is no way a
> subdev driver can know that, only the bridge driver knows what product it is
> and thus what the labels on the inputs are.

Hmm... I assume there is a misunderstanding about what the tuner "name"
means and the corresponding field in struct v42l_tuner should contain...
With the patch, the returned tuner names are e.g. "Temic PAL (4002
FH5)", "LG PAL_BG+FM (TPI8PSB01D)" etc, which is what I expect / think
is a correct tuner name.
If it is a TV or radio tuner is defined by the "type" field and if it is
the first / second / third / ... in the device is indeed the
responsibility of the bridge driver. My understanding is, that both
informations do not belong into the "name" field.
If userspace isn't interested in the actual tuner name, it can easily
create a simple name using the tuner "type" and "index" fields of the
struct.

> ...
>
> The golden rule is that sub-devices should not assume anything about how they
> are hooked up in the actual product.

Yeah, sure, but I can't see that we make such an assumption here !?

Regards,
Frank

>
> Regards,
>
> 	Hans

Re: [PATCH] tuner-core: return tuner name with ioctl VIDIOC_G_TUNER

[Hans Verkuil]

On Wed January 23 2013 18:03:33 Frank Schäfer wrote:
> Am 23.01.2013 17:13, schrieb Hans Verkuil:
> > On Wed January 23 2013 16:57:40 Frank Schäfer wrote:
> >> Am 23.01.2013 08:35, schrieb Hans Verkuil:
> >>> On Tue January 22 2013 20:46:21 Frank Schäfer wrote:
> >>>> tuner_g_tuner() is supposed to fill struct v4l2_tuner passed by ioctl
> >>>> VIDIOC_G_TUNER, but misses setting the name field.
> >>>>
> >>>> Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
> >>>> Cc: stable@kernel.org
> >>>> ---
> >>>>  drivers/media/v4l2-core/tuner-core.c |    1 +
> >>>>  1 Datei geändert, 1 Zeile hinzugefügt(+)
> >>>>
> >>>> diff --git a/drivers/media/v4l2-core/tuner-core.c b/drivers/media/v4l2-core/tuner-core.c
> >>>> index b5a819a..95a47cf 100644
> >>>> --- a/drivers/media/v4l2-core/tuner-core.c
> >>>> +++ b/drivers/media/v4l2-core/tuner-core.c
> >>>> @@ -1187,6 +1187,7 @@ static int tuner_g_tuner(struct v4l2_subdev *sd, struct v4l2_tuner *vt)
> >>>>  
> >>>>  	if (check_mode(t, vt->type) == -EINVAL)
> >>>>  		return 0;
> >>>> +	strcpy(vt->name, t->name);
> >>>>  	if (vt->type == t->mode && analog_ops->get_afc)
> >>>>  		vt->afc = analog_ops->get_afc(&t->fe);
> >>>>  	if (analog_ops->has_signal)
> >>>>
> >>> Nacked-by: Hans Verkuil <hans.verkuil@cisco.com>
> >>>
> >>> And the reason is that the tuner field should be filled in by the bridge
> >>> driver. That's because you may have multiple tuners and it's only the
> >>> bridge driver that will know which tuner is which and what name to give
> >>> it.
> >> Hmmm... I don't understand.
> >> Isn't his a per-tuner (subdev) operation ? If a device has multiple
> >> tuners (subdevs) it is called for each of them.
> >> So how can the returned tuner name be wrong and why should the bridge
> >> driver know better than the subdevice itself which name is correct ?
> > The name that's filled in is exposed to userspace, so it should be something
> > meaningful and not some internal name. In the case of multiple tuners that
> > means that the name should be something like 'TV 1' or 'TV 2', where the name
> > matches a name (label) of a tuner input of the product. There is no way a
> > subdev driver can know that, only the bridge driver knows what product it is
> > and thus what the labels on the inputs are.
> 
> Hmm... I assume there is a misunderstanding about what the tuner "name"
> means and the corresponding field in struct v42l_tuner should contain...
> With the patch, the returned tuner names are e.g. "Temic PAL (4002
> FH5)", "LG PAL_BG+FM (TPI8PSB01D)" etc, which is what I expect / think
> is a correct tuner name.

That's the name of the tuner device. That's like reporting to the user
that the video input is named "saa7115 4-0021". That's not useful. It
*is* useful for us as developers, which is why such names are seen in the
kernel log.

> If it is a TV or radio tuner is defined by the "type" field and if it is
> the first / second / third / ... in the device is indeed the
> responsibility of the bridge driver. My understanding is, that both
> informations do not belong into the "name" field.
> If userspace isn't interested in the actual tuner name, it can easily
> create a simple name using the tuner "type" and "index" fields of the
> struct.

That's like saying that you don't need the name field in v4l2_input since
you can just say "Input 1", "Input 2", etc. In the case of multiple tuners
the tuner name should have some relationship with an antenna input on the
product (which may be internal as well, BTW).

Note that the only time I can see multiple tuners work is for radio receivers:
one tuner might be the current tuner, while a second tuner is testing whether
'alternate' frequencies as provided by an RDS feed have a better signal. Or
if you have different tuners each with different frequency ranges.

> 
> > ...
> >
> > The golden rule is that sub-devices should not assume anything about how they
> > are hooked up in the actual product.
> 
> Yeah, sure, but I can't see that we make such an assumption here !?

In this case it is more in the opposite direction: a subdev internal name is
exposed to userspace.

Regards,

	Hans

Re: [PATCH] tuner-core: return tuner name with ioctl VIDIOC_G_TUNER

[Frank Schäfer]

Am 23.01.2013 19:33, schrieb Hans Verkuil:
> On Wed January 23 2013 18:03:33 Frank Schäfer wrote:
>> Am 23.01.2013 17:13, schrieb Hans Verkuil:
>>> On Wed January 23 2013 16:57:40 Frank Schäfer wrote:
>>>> Am 23.01.2013 08:35, schrieb Hans Verkuil:
>>>>> On Tue January 22 2013 20:46:21 Frank Schäfer wrote:
>>>>>> tuner_g_tuner() is supposed to fill struct v4l2_tuner passed by ioctl
>>>>>> VIDIOC_G_TUNER, but misses setting the name field.
>>>>>>
>>>>>> Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
>>>>>> Cc: stable@kernel.org
>>>>>> ---
>>>>>>  drivers/media/v4l2-core/tuner-core.c |    1 +
>>>>>>  1 Datei geändert, 1 Zeile hinzugefügt(+)
>>>>>>
>>>>>> diff --git a/drivers/media/v4l2-core/tuner-core.c b/drivers/media/v4l2-core/tuner-core.c
>>>>>> index b5a819a..95a47cf 100644
>>>>>> --- a/drivers/media/v4l2-core/tuner-core.c
>>>>>> +++ b/drivers/media/v4l2-core/tuner-core.c
>>>>>> @@ -1187,6 +1187,7 @@ static int tuner_g_tuner(struct v4l2_subdev *sd, struct v4l2_tuner *vt)
>>>>>>  
>>>>>>  	if (check_mode(t, vt->type) == -EINVAL)
>>>>>>  		return 0;
>>>>>> +	strcpy(vt->name, t->name);
>>>>>>  	if (vt->type == t->mode && analog_ops->get_afc)
>>>>>>  		vt->afc = analog_ops->get_afc(&t->fe);
>>>>>>  	if (analog_ops->has_signal)
>>>>>>
>>>>> Nacked-by: Hans Verkuil <hans.verkuil@cisco.com>
>>>>>
>>>>> And the reason is that the tuner field should be filled in by the bridge
>>>>> driver. That's because you may have multiple tuners and it's only the
>>>>> bridge driver that will know which tuner is which and what name to give
>>>>> it.
>>>> Hmmm... I don't understand.
>>>> Isn't his a per-tuner (subdev) operation ? If a device has multiple
>>>> tuners (subdevs) it is called for each of them.
>>>> So how can the returned tuner name be wrong and why should the bridge
>>>> driver know better than the subdevice itself which name is correct ?
>>> The name that's filled in is exposed to userspace, so it should be something
>>> meaningful and not some internal name. In the case of multiple tuners that
>>> means that the name should be something like 'TV 1' or 'TV 2', where the name
>>> matches a name (label) of a tuner input of the product. There is no way a
>>> subdev driver can know that, only the bridge driver knows what product it is
>>> and thus what the labels on the inputs are.
>> Hmm... I assume there is a misunderstanding about what the tuner "name"
>> means and the corresponding field in struct v42l_tuner should contain...
>> With the patch, the returned tuner names are e.g. "Temic PAL (4002
>> FH5)", "LG PAL_BG+FM (TPI8PSB01D)" etc, which is what I expect / think
>> is a correct tuner name.
> That's the name of the tuner device. That's like reporting to the user
> that the video input is named "saa7115 4-0021". That's not useful. It
> *is* useful for us as developers, which is why such names are seen in the
> kernel log.

Not sure if the saa7115 driver could provide better input names, but I
agree that letting the bridge driver set it's own names for inputs makes
sense.
But I would say for the tuner names, the situation is different.
Looking at tuner-types.c, I can say that these names are exaclty what I
expect to get with the VIDIOC_G_TUNER ioctrl.
Btw, bridge drivers can still overwrite the tuner name they get from the
subdevice. ;)
And getting the full device name from the subdevice is definitely better
than getting no device name at all. (I would also say it's somehow
inconsistent).

...
>>
>>> ...
>>>
>>> The golden rule is that sub-devices should not assume anything about how they
>>> are hooked up in the actual product.
>> Yeah, sure, but I can't see that we make such an assumption here !?
> In this case it is more in the opposite direction: a subdev internal name is
> exposed to userspace.

It doesn't matter where the name comes from, the important thing is that
it matches what the API spec says / userspace expects.
So if we can pass the name from the subdevice to userspace directly
because it matches these requirements, why not ?
I think overwriting names etc. retrieved via a subdevice call should be
the exception, not the rule.


I actually don't care too much about the content of the tuner name
field. I can't remember having seen any applications displaying it.
I'm just trying to understand the API and I have a pending patch (as
part of an ioctl fixes series) for the em28xx drivers that is supposed
to unify the tuner name handing for video/vbi and radio devices (in
whatever direction).

Regards,
Frank

>
> Regards,
>
> 	Hans