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)
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?
> > 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[].
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.)
Are you willing to pursue any of these 3 options and submit a
patch to the v4l folks?
(I wouldn't realistically be able to get a patch to them in the
proper form until the FC5 guys get a 2.6.19 kernel rpm released
as I'm working on my household's primary machine that's always
in high demand.)
My testing and pages 13 and 16 of this datasheet:
http://dl.ivtvdriver.org/datasheets/tuners/TAPE-H091F_MK3.pdf
can be used as justification for changing the flags.
Page 6 of:
http://www.lginnotek.com/data/item/A4152TAPEseries/TAPE-Hseries.pdf
can be used to show why the TAPE-H091F and TAPE-H001F are electrically
the same, if required. (BTW page 4 has a fixed block diagram.)
BTW, someone may wish to edit this wiki page:
http://www.ivtvdriver.org/index.php/Howto:Radio_tuner
Thanks,
Andy
_______________________________________________
ivtv-devel mailing list
[email protected]
http://ivtvdriver.org/mailman/listinfo/ivtv-devel
