On Tue, Jun 19, 2012 at 10:41 AM, Mauro Carvalho Chehab
<mche...@redhat.com> wrote:
> Em 19-06-2012 10:31, halli manjunatha escreveu:
>> Hi Mauro,
>>
>> Please take the Patch-set 7 which I submitted by removing my set_band
>> implementation (as per Hans V suggestion).
>>
>> https://lkml.org/lkml/2012/5/21/294
>
> Manju,
>
> That doesn't solve the issue.
>
> As I pointed on my previous email, the ranges aren't consistent among the
> radio devices. The best, IMHO, would be to use several g/s_tuner ranges,
> one for each supported one.
>
> An alternative would be to write a set of ioctls specific for radio that
> would do the same that g/s_tuner does at radio-cadet, but, IMHO, this is
> is overdesign.
>
> In any case, we should not apply a patch for it without having a consensus
> about the right way.
I agree with you that we should not apply a patch till we come to a
conclusion about the design.
But the patches which I have sent (PATCH-SET 7) that doesn't deal with
FM band selection, instead it adds
few other features like below
1) FM RX RDS AF turn ON/OFF
2) FM RX De-Emphasis mode set/get
3) FM TX Alternate Frequency set/get
So since these are other features which are not related to Band
selection I think you can merge these to K3.6 kernel.
>
> Regards,
> Mauro
>
>>
>> Regards
>> Manju
>>
>> On Tue, Jun 19, 2012 at 7:36 AM, Hans de Goede <hdego...@redhat.com> wrote:
>>> Hi,
>>>
>>>
>>> On 06/19/2012 01:09 PM, Mauro Carvalho Chehab wrote:
>>>>
>>>> Em 19-06-2012 05:27, Hans de Goede escreveu:
>>>>>
>>>>> Hi,
>>>>>
>>>>> On 06/19/2012 02:47 AM, Mauro Carvalho Chehab wrote:
>>>>>>
>>>>>> Em 28-05-2012 07:46, Hans Verkuil escreveu:
>>>>>>>
>>>>>>> From: Hans Verkuil <hans.verk...@cisco.com>
>>>>>>>
>>>>>>> Signed-off-by: Hans Verkuil <hans.verk...@cisco.com>
>>>>>>> Acked-by: Hans de Goede <hdego...@redhat.com>
>>>>>>> ---
>>>>>>> include/linux/videodev2.h | 19 +++++++++++++++++--
>>>>>>> 1 file changed, 17 insertions(+), 2 deletions(-)
>>>>>>>
>>>>>>> diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
>>>>>>> index 2339678..013ee46 100644
>>>>>>> --- a/include/linux/videodev2.h
>>>>>>> +++ b/include/linux/videodev2.h
>>>>>>> @@ -2023,7 +2023,8 @@ struct v4l2_tuner {
>>>>>>> __u32 audmode;
>>>>>>> __s32 signal;
>>>>>>> __s32 afc;
>>>>>>> - __u32 reserved[4];
>>>>>>> + __u32 band;
>>>>>>> + __u32 reserved[3];
>>>>>>> };
>>>>>>>
>>>>>>> struct v4l2_modulator {
>>>>>>> @@ -2033,7 +2034,8 @@ struct v4l2_modulator {
>>>>>>> __u32 rangelow;
>>>>>>> __u32 rangehigh;
>>>>>>> __u32 txsubchans;
>>>>>>> - __u32 reserved[4];
>>>>>>> + __u32 band;
>>>>>>> + __u32 reserved[3];
>>>>>>> };
>>>>>>>
>>>>>>> /* Flags for the 'capability' field */
>>>>>>> @@ -2048,6 +2050,11 @@ struct v4l2_modulator {
>>>>>>> #define V4L2_TUNER_CAP_RDS 0x0080
>>>>>>> #define V4L2_TUNER_CAP_RDS_BLOCK_IO 0x0100
>>>>>>> #define V4L2_TUNER_CAP_RDS_CONTROLS 0x0200
>>>>>>> +#define V4L2_TUNER_CAP_BAND_FM_EUROPE_US 0x00010000
>>>>>>> +#define V4L2_TUNER_CAP_BAND_FM_JAPAN 0x00020000
>>>>>>> +#define V4L2_TUNER_CAP_BAND_FM_RUSSIAN 0x00040000
>>>>>>> +#define V4L2_TUNER_CAP_BAND_FM_WEATHER 0x00080000
>>>>>>> +#define V4L2_TUNER_CAP_BAND_AM_MW 0x00100000
>>>>>>
>>>>>>
>>>>>> Frequency band is already specified by rangelow/rangehigh.
>>>>>>
>>>>>> Why do you need to duplicate this information?
>>>>>
>>>>>
>>>>> Because radio tuners may support multiple non overlapping
>>>>> bands, this is why this patch also adds a band member
>>>>> to the tuner struct, which can be used to set/get
>>>>> the current band.
>>>>>
>>>>> One example of this are the tea5757 / tea5759
>>>>> radio tuner chips:
>>>>>
>>>>> FM:
>>>>> tea5757 87.5 - 108 MHz
>>>>
>>>>
>>>> rangelow = 87.5 * 62500;
>>>> rangehigh = 108 * 62500;
>>>>
>>>>> tea5759 76 - 91 MHz
>>>>
>>>>
>>>> rangelow = 76 * 62500;
>>>> rangehigh = 91 * 62500;
>>>>
>>>>> AM:
>>>>> Both: 530 - 1710 kHz
>>>>
>>>>
>>>> rangelow = 0.530 * 62500;
>>>> rangehigh = 0.1710 * 62500;
>>>>
>>>>
>>>> See radio-cadet.c:
>>>>
>>>> static int vidioc_g_tuner(struct file *file, void *priv,
>>>> struct v4l2_tuner *v)
>>>> {
>>>> struct cadet *dev = video_drvdata(file);
>>>>
>>>> v->type = V4L2_TUNER_RADIO;
>>>> switch (v->index) {
>>>> case 0:
>>>> strlcpy(v->name, "FM", sizeof(v->name));
>>>> v->capability = V4L2_TUNER_CAP_STEREO | V4L2_TUNER_CAP_RDS
>>>> |
>>>> V4L2_TUNER_CAP_RDS_BLOCK_IO;
>>>> v->rangelow = 1400; /* 87.5 MHz */
>>>> v->rangehigh = 1728; /* 108.0 MHz */
>>>> v->rxsubchans = cadet_getstereo(dev);
>>>> switch (v->rxsubchans) {
>>>> case V4L2_TUNER_SUB_MONO:
>>>> v->audmode = V4L2_TUNER_MODE_MONO;
>>>> break;
>>>> case V4L2_TUNER_SUB_STEREO:
>>>> v->audmode = V4L2_TUNER_MODE_STEREO;
>>>> break;
>>>> default:
>>>> break;
>>>> }
>>>> v->rxsubchans |= V4L2_TUNER_SUB_RDS;
>>>> break;
>>>> case 1:
>>>> strlcpy(v->name, "AM", sizeof(v->name));
>>>> v->capability = V4L2_TUNER_CAP_LOW;
>>>> v->rangelow = 8320; /* 520 kHz */
>>>> v->rangehigh = 26400; /* 1650 kHz */
>>>> v->rxsubchans = V4L2_TUNER_SUB_MONO;
>>>> v->audmode = V4L2_TUNER_MODE_MONO;
>>>> break;
>>>> default:
>>>> return -EINVAL;
>>>> }
>>>> v->signal = dev->sigstrength; /* We might need to modify scaling of
>>>> this
>>>> */
>>>> return 0;
>>>> }
>>>> static int vidioc_s_tuner(struct file *file, void *priv,
>>>> struct v4l2_tuner *v)
>>>> {
>>>> struct cadet *dev = video_drvdata(file);
>>>>
>>>> if (v->index != 0 && v->index != 1)
>>>> return -EINVAL;
>>>> dev->curtuner = v->index;
>>>> return 0;
>>>> }
>>>>
>>>> Band switching are made via g_tuner/s_tuner calls. If a device have
>>>> several non-overlapping bands, just implement it there. There's no
>>>> need for a new API.
>>>
>>>
>>> <sigh>, this has been discussed extensively between me, Hans V and
>>> Halli Manjunatha on both irc and on the list. What the cadet driver is
>>> doing is an ugly hack, and really a poor match for what we want.
>>>
>>> Not to mention that it is a clear violation of the v4l2 spec:
>>> http://linuxtv.org/downloads/v4l-dvb-apis/tuner.html
>>>
>>> "Radio input devices have exactly one tuner with index zero, no video
>>> inputs."
>>>
>>> So there is supposed to be only one tuner, and s_tuner / g_tuner
>>> on radio devices always expect a tuner index of 0.
>>>
>>> Also from the same page:
>>> "Note that VIDIOC_S_TUNER does not switch the current tuner, when there is
>>> more than one at all."
>>>
>>> So if we model discontinuous ranges as multiple tuners how do we
>>> select the right tuner? Certainly *not* though s_tuner, as that would
>>> violate the spec. Note that changing the spec here is not really an option,
>>> S_TUNER is expected to change the properties of the tuner selected through
>>> the index, and is *not* expected to change the active tuner , esp. since
>>> changing the active tuner would raise the question, change the active tuner
>>> for which input ? The spec is clear on this:
>>> "The tuner is solely determined by the current video input."
>>>
>>> iow s_tuner sets tuner parameters (such as the band of a multi-band tuner),
>>> but it does not select a tuner. Making s_tuner actually select 1 of multiple
>>> tuners for radio devices, would cause a large discrepancy between radio and
>>> tv tuners.
>>>
>>> For tv tuners we've a 1:1 mapping between tuners and inputs, which makes
>>> sense, because
>>> there are actual dual tuner devices, and the purpose of those is to be able
>>> to watch /
>>> record 2 "shows" at the same time. This is simply not the case with these
>>> radio devices,
>>> they can tune both AM and FM but *not* at the same time, so they have a
>>> *single*
>>> *multiple-band* tuner.
>>>
>>> Modeling this as multiple tuners is just wrong. Not only have we already
>>> discussed
>>> this in a long discussion, I've patches to extend the tea575x driver with AM
>>> support,
>>> and the initial revision used the multiple tuner model, but that just does
>>> not work
>>> well, and I'm bad Hans V. intervened and pointed out Halli Manjunatha's
>>> patchset for
>>> limiting hw-freq seek ranges, after which all of this has been discussed
>>> extensively!
>>>
>>>
>>>> Also, this is generic enough to cover even devices with non-standard
>>>> frequency ranges.
>>>>
>>>> All bands can easily be detected via a g_tuner loop, and band switching
>>>> is done via s_tuner.
>>>>
>>>> Each band range can have its name ("AM", "FM", "AM-SW", "FM-Japan", ...),
>>>> and this is a way more generic than what's being proposed.
>>>
>>>
>>> It is also very very wrong, there is only a single tuner on these devices,
>>> modeling this as multiple tuners is just wrong!
>>>
>>>
>>>> It likely makes sense to standardize the band names inside the radio core,
>>>> in order to avoid having the same band called with two different names
>>>> inside
>>>> the drivers.
>>>>
>>>> It should also be noticed that each band may have different properties.
>>>> On the above, the FM band can do stereo/mono and RDS, while AM is just
>>>> mono So, a change like what's proposed would keep requiring two entries.
>>>
>>>
>>> With FM we already have a situation where some channels are mono and other
>>> stereo, with AM/FM the tuner capabilities would reflect what the tuner can
>>> do on some bands-frequency combinations, just like it now reflects what
>>> it can do on some frequencies.
>>>
>>> <snip>
>>>
>>>
>>>>> 87.5 - 108 MHz is very close to 88 - 108 MHz, I don't think it is worth
>>>>> creating 2 band defines for this.
>>>>
>>>>
>>>> Yes, it is very close, but Countries that added the extra 500 kHz
>>>> bandwidth
>>>> added stations there. On those, older devices can't tune into the new
>>>> channels.
>>>
>>>
>>> On those older devices rangelow would get reported as 88 rather then 87.5,
>>> the
>>> band selection mechanism is there to select a certain range approximately,
>>> the exact resulting range will be hw specific and reported in rangelow /'
>>> rangehigh, as the patch documenting the new fields clearly documents.
>>>
>>> <snip>
>>>
>>>
>>>>> This would be covered by the V4L2_TUNER_BAND_FM_UNIVERSAL, however,
>>>>> on some devices V4L2_TUNER_BAND_FM_UNIVERSAL may include the weather
>>>>> band,
>>>>> thus going all the way from 76 - 163 Mhz, so I guess we should add a
>>>>> V4L2_TUNER_BAND_FM_JAPAN_WIDE for this. Note that the si470x already
>>>>> supports this, and indeed calls it "Japan wide band"
>>>>
>>>>
>>>> That's why giving them name via defines is a bad thing: the concept of
>>>> "universal" changes from time to time: 15 years ago, an "universal" radio
>>>> is a device that were able to tune at AM-SW, AM-MW, AM-HW and FM
>>>> (88-108MHz).
>>>>
>>>> An "universal FM" radio used to be 76-108 MHz, but, with the weather band,
>>>> it is now 76-163 Mhz.
>>>>
>>>> If a band like that is described as "FM" with a frequency range from 76
>>>> to 163 MHz, this is clearer than calling it as "FM unversal".
>>>
>>>
>>> We will still have rangelow and rangehigh to report the actual implemented
>>> band. So there is no problem here. An app can select universal and then
>>> figure out what universal is on the specific device it is using with a
>>> G_TUNER.
>>>
>>> <snip>
>>>
>>>
>>>>> So lets get back to the basis, for AM/FM switching / limiting hw-freq
>>>>> seeking, and on some devices likely even just to be able to tune to
>>>>> certain frequencies we need to select a band with various radio devices.
>>>>>
>>>>> On some radio devices we may be able to just program the seek range, but
>>>>> on
>>>>> most it is hardcoded based on a band selection register.
>>>>
>>>>
>>>> Except due to regulatory requirements, the driver could just expose the
>>>> broadest range. That's what I did with tea5767, as it allows using either
>>>> an "universal" range from 76 to 108 MHz, or to limit it to 88.5-108MHz.
>>>>
>>>>> So we need some way of naming the bands, with approx. expected ranges
>>>>> (the real range supported by the specific device will be reported on a
>>>>> G_TUNER).
>>>>>
>>>>> Looking at:
>>>>> http://en.wikipedia.org/wiki/FM_broadcast_band
>>>>>
>>>>> I suggest naming the bands after their standards, except for the Japanese
>>>>> bands which are special and I suggest just naming them after their
>>>>> country, resulting in:
>>>>>
>>>>> #define V4L2_TUNER_BAND_FM_CCIR 1 /* 87.5 - 108 Mhz */
>>>>
>>>>
>>>> CCIR is a bad (and obsolete) name.
>>>
>>>
>>> Ok, so we call it V4L2_TUNER_BAND_FM_STANDARD, since it seems to
>>> be what most of the world is either using or moving too (most of the
>>> former USSR has also moved to a range of 87.5 - 108, rather then the
>>> OIRT bands).
>>>
>>>
>>>> It is a bad name because it is the name of the Radio committee of the ITU,
>>>> and this committee standardizes all radio ranges, not only the above.
>>>>
>>>> It is an obsolete name, as CCIR was renamed to ITU-R, back in 1992[1].
>>>>
>>>> Btw, take a look at ITU-R BS.450-3 spec, table 1a[2]: it defines several
>>>> ranges there:
>>>> 87.5-108
>>>> 88-108
>>>> 88-100 (Norway)
>>>
>>>
>>> Standard
>>>
>>>> 66-73 (Gambia)
>>>> 66-74 (Lithuania)
>>>
>>>
>>> OIRT
>>>
>>>> 87.8-108 (US)
>>>> 100-108 (India)
>>>
>>>
>>> Standard
>>>
>>>> 76-90 (Japan)
>>>
>>>
>>> Japan
>>>
>>> Note that currently several drivers already implement a band concept in some
>>> way, ie in the tea5767 driver, you expose this through a config flag called
>>> japan_band,
>>> and that at least the saa7134 and cx88 cards code adds a tea5767 tuner
>>> with the japan_band flag set to 0, resulting in not getting the wide band,
>>> but the
>>> small band, and thus likely not working in japan. Also note that since the
>>> tea5767
>>> radio tuner driver uses the standard tuner framework, it reports a hardcoded
>>> range
>>> of 65-108 (radio_range in drivers/media/video/tuner-core.c) independent of
>>> the
>>> japan_band parameter.
>>>
>>> The si470x driver has a band *module* parameter instead, note though that in
>>> both cases
>>> the (average) user ends up with a hardcoded band, where he should be able to
>>> adjust it
>>> to match the country/regio he is in...
>>>
>>> So we really need some way to enumerate and set radio-bands, not
>>> radio-tuners, but
>>> radio-bands, and that is exactly what the proposed API gives us in a nice
>>> and simple
>>> way.
>>>
>>> Regards,
>>>
>>> Hans
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>
>>
>>
>
>
--
Regards
Halli
--
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
