On Saturday 02 December 2006 19:37, Andy Walls wrote:
> On Sat, 2006-12-02 at 14:29 +0100, Hans Verkuil wrote:
> > On Saturday 02 December 2006 03:51, Andy Walls wrote:
> > > Hmmm. My first cut at fixing my problem was this exact patch,
> > > but I didn't like it for a few reasons:
> > >
> > > 1. It didn't address all the TDA9887 port (OP1 & OP2) specifics
> > > for FM radio for the TAPE-H0x1x. I don't know if they're
> > > critical, but I wanted to make the tuner entry "right". I live
> > > over 75 miles (121 km) from the closest major city, so FM
> > > sensitivity is important.
> >
> > That's already set correctly through the port1_fm_high_sensitivity
> > flag.
>
> I can't quite follow the code to see how this is the default behavior
> (I may be missing something through all the logic inversions though).
> The original tuner_lg_ntsc_tape_params[] doesn't have the following
> set:
>
> .cb_first_if_lower_freq = 1,
> .has_tda9887 = 1,
> .port1_active = 1,
> .port2_active = 1,
> .port1_fm_high_sensitivity = 1,
>
> (the .port2_invert_for_secam_lc = 1 can safely be ignored as you
> know)
It's all set at the end of the default_set_radio_freq() function in
tuner-simple.c. It's a bit tricky but it really works :-)
>
> What hindsight does show is that not only are the
> tuner_fm1236_mk3_ntsc_ranges[] compatible with the
> tuner_lg_ntsc_tape_ranges[], as the original code indicated, but also
> that the tuner_lg_ntsc_tape_params[] should actually be identical to
> the tuner_fm1236_mk3_params[].
>
> Maybe it would be better to eliminate tuner_lg_ntsc_tape_ranges[] and
> tuner_lg_ntsc_tape_params[] from the file and fix the
> tuners[TUNER_LG_NTSC_TAPE] entry to point to
> tuner_fm1236_mk3_params[]?
>
> Your thoughts?
Agreed.
>
> > > 2. The FM radio configuration in the tuner module is spread
> > > between a configuration look-up and special case handling
> > > peppered through the code. Experience has taught me that lookup
> > > tables are better for long term maintenance than exceptional
> > > condition handling distributed through the code.
> >
> > Yes, I agree that would be better, but that is something you should
> > discuss separately on the video4linux mailinglist. It is also good
> > to see this as two separate issues: fixing radio support for this
> > tuner and adding table lookup for the radio support. Don't mix
> > them, as that will make it much harder to get your fixes in.
>
> Makes sense to me. I probably won't have the time to follow through
> on table lookup for radio support though. (It looks like someone
> else is eventually planning to tackle it anyway.)
>
> > > 3. It seemed like there were bigger plans for the tuner
> > > entries in tuner-types.c to support multi-standard tuners,
> > > so I thought I'd follow that lead. Also since my tuner's
> > > front end has a different bandswitch byte for stereo vs forced
> > > mono, I thought that the range entries in the table might be a
> > > good place to store that configuration knowledge.
> >
> > Again, I agree that that is probably not a bad idea, but this too
> > is a separate issue.
>
> Agree. I probably won't have time to follow up on this either. I'd
> only use Mono to try and tune an incredibly weak station which was
> running a radio program that I desperately wanted to hear. I have a
> standalone FM radio for those rare occasions. ;)
>
> > You see, fixing radio support for this tuner is a one-liner: it
> > will go in without discussion for 2.6.20, and can be added as a
> > dot-release for 2.6.19. The other changes will certainly not be
> > accepted for a 2.6.19 dot release and will probably run into
> > difficulties on the 2.6.20 front: in particular (and this would be
> > my criticism if I put my v4l maintainer hat on) it deals with only
> > one radio tuner. If you want to get such improvements in then you
> > should convert the other existing 'simple' radio tuners as well.
> >
> >
> > Don't let this discourage you,
> > the idea is good and improvements in this area will be appreciated.
> > It just takes a bit more effort.
>
> I'm not feeling discouraged, but I really only have 1 dog in this
> fight (my PVR-150) and so little time to devote. I used to get paid
> to write code, and I do like to exercise my skills now and then.
> However life goes on and wife, kids, job, home maintenance, etc. all
> seem to have higher priority when allocating my time.
>
> > It's up to you: I can still push through the one-liner change now,
> > and you can post the table lookup changes later to the video4linux
> > list. It's my preferred option since this will fix the really
> > serious bug quickly. Besides, I think I introduced this bug anyway
> > when I merged the ivtv tuner module with the kernel tuner module
> > and missed this one difference.
>
> Thank you for the offer, I agree with your reasoning and would like
> you to proceed, but with perhaps slight modification.
>
> Below are 3 alternatives for a fix I'd like to see,
> that I think serve both our interests and the user community as
> a whole:
>
> 1: a. Your one-liner to tuner-simple.c
> b. Change tuner_lg_ntsc_tape_params[] to have the flags
> match tuner_fm1236_mk3_params[]'s flags in tuner-types.c
>
> or
>
> 2: a. Your one-liner to tuner-simple.c
> b. Remove tuner_lg_ntsc_tape_params[] from tuner-types.c
> c. Changing tuners[TUNER_LG_NTSC_TAPE] in tuner-types.c
> to refer to tuner_fm1236_mk3_params[].
Yes, this is the best solution.
>
> or
>
> 3: a. Your one-liner to tuner-simple.c
> b. and a clue for me as to where in the code fm sensitivity
> is "already set correctly through the
> port1_fm_high_sensitivity flag" with the original
> tuner_lg_ntsc_tape_params[] definition?
> (Maybe I'm blinded by obviousness; it's happened before.)
No, it's my fault: I now see that the current tuner_lg_ntsc_tape_params
doesn't contain the extra flags. I missed that :-)
>
>
> Are you willing to pursue any of these 3 options and submit a
> patch to the v4l folks?
I've attached a patch for option 2. Let me know if this is OK by you and
I'll submit it.
Thanks,
Hans
diff -r 69852a58d02c linux/drivers/media/video/tuner-simple.c
--- a/linux/drivers/media/video/tuner-simple.c Fri Dec 01 18:55:37 2006 -0200
+++ b/linux/drivers/media/video/tuner-simple.c Sat Dec 02 20:38:49 2006 +0100
@@ -117,6 +117,7 @@ static int tuner_stereo(struct i2c_clien
case TUNER_PHILIPS_FM1216ME_MK3:
case TUNER_PHILIPS_FM1236_MK3:
case TUNER_PHILIPS_FM1256_IH3:
+ case TUNER_LG_NTSC_TAPE:
stereo = ((status & TUNER_SIGNAL) == TUNER_STEREO_MK3);
break;
default:
@@ -457,6 +458,7 @@ static void default_set_radio_freq(struc
case TUNER_PHILIPS_FM1216ME_MK3:
case TUNER_PHILIPS_FM1236_MK3:
case TUNER_PHILIPS_FMD1216ME_MK3:
+ case TUNER_LG_NTSC_TAPE:
buffer[3] = 0x19;
break;
case TUNER_TNF_5335MF:
diff -r 69852a58d02c linux/drivers/media/video/tuner-types.c
--- a/linux/drivers/media/video/tuner-types.c Fri Dec 01 18:55:37 2006 -0200
+++ b/linux/drivers/media/video/tuner-types.c Sat Dec 02 20:37:54 2006 +0100
@@ -673,16 +673,6 @@ static struct tuner_params tuner_panason
},
};
-/* ------------ TUNER_LG_NTSC_TAPE - LGINNOTEK NTSC ------------ */
-
-static struct tuner_params tuner_lg_ntsc_tape_params[] = {
- {
- .type = TUNER_PARAM_TYPE_NTSC,
- .ranges = tuner_fm1236_mk3_ntsc_ranges,
- .count = ARRAY_SIZE(tuner_fm1236_mk3_ntsc_ranges),
- },
-};
-
/* ------------ TUNER_TNF_8831BGFF - Philips PAL ------------ */
static struct tuner_range tuner_tnf_8831bgff_pal_ranges[] = {
@@ -1379,8 +1369,8 @@ struct tunertype tuners[] = {
},
[TUNER_LG_NTSC_TAPE] = { /* LGINNOTEK NTSC */
.name = "LG NTSC (TAPE series)",
- .params = tuner_lg_ntsc_tape_params,
- .count = ARRAY_SIZE(tuner_lg_ntsc_tape_params),
+ .params = tuner_fm1236_mk3_params,
+ .count = ARRAY_SIZE(tuner_fm1236_mk3_params),
},
[TUNER_TNF_8831BGFF] = { /* Philips PAL */
.name = "Tenna TNF 8831 BGFF)",
_______________________________________________
ivtv-devel mailing list
[email protected]
http://ivtvdriver.org/mailman/listinfo/ivtv-devel
