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. Regards, Mauro > > 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 >> >> >> >> >> >> >> > > > -- 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
