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 Regards Manju On Tue, Jun 19, 2012 at 7:36 AM, Hans de Goede <hdegoede@xxxxxxxxxx> 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.verkuil@xxxxxxxxx> >>>>> >>>>> Signed-off-by: Hans Verkuil <hans.verkuil@xxxxxxxxx> >>>>> Acked-by: Hans de Goede <hdegoede@xxxxxxxxxx> >>>>> --- >>>>> 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 majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
