On Saturday 02 December 2006 03:51, Andy Walls wrote:
> On Fri, 2006-12-01 at 17:32 +0100, Hans Verkuil wrote:
> > On Thursday 30 November 2006 08:04, Hans Verkuil wrote:
> > > On Thursday 30 November 2006 04:04, Andy Walls wrote:
> > > > I have a PVR-150 MCE on a AMD 64x2 Fedora Core 5 system and
> > > > couldn't get anything but noise (square wave with fundamental
> > > > frequencies of ~785 Hz and and some other higher frequency)
> > > > with ivtv-radio, ivtv-0.8.0 and kernel 2.6.18.
> > > >
> > > > I tracked the problem down to a bad "bandswitch byte" value
> > > > (0xa4) being sent to the front end of the TAPE-H001F tuner on
> > > > the card. FM band stereo required a value of 0x19 with this
> > > > tuner. The attached file is the fix for the kernel on my
> > > > machine (with too many comments for most people's taste I'm
> > > > sure).
> > > >
> > > > I'd like to say "Thank You!" to the fellow from France on this
> > > > list whose persistence at trying to get his FM radio to work on
> > > > his PVR inspired me to fix mine, and for letting me see that
> > > > someone else had FM radio, and not TV, as their primary
> > > > application as well.
> > > >
> > > > What follows are the details from my system.
> > > >
> > > > -Andy Walls
> > >
> > > Andy,
> > >
> > > Thank you for this patch! I'll add it to the ivtv-0.4 tuner
> > > module myself, but to get this patch into the kernel I recommend
> > > that you post it to the [email protected] mailinglist
> > > (see http://www.linuxtv.org/v4lwiki/index.php/Main_Page). You
> > > also need to add a 'Signed-off-by' line (see
> > > http://www.linuxtv.org/v4lwiki/index.php/SubmittingPatches).
> > >
> > > One suggestion: in the tuner_lg_ntsc_tape_params struct you
> > > should only put the '= 1' fields and omit the '= 0' fields as
> > > that is always the default. And the amount of comments is a bit
> > > over the top :-) Remember that most tuners are identical, except
> > > for bandwidth ranges and the bandswitch bytes. So only special,
> > > non-standard properties need to be documented.
> >
> > Andy,
> >
> > When I tried to fix this in ivtv-0.4.x I noticed that it was
> > handled correctly in tuner.c in ivtv-0.4. As far as I can tell this
> > is a change in tuner.c from ivtv-0.4 that was never merged into the
> > linux kernel tuner module. The (much smaller) diff below is all
> > that is needed for the linux kernel to make this tuner work again
> > as far as I can tell.
> >
> > If it's OK with you I'll have this diff committed to the v4l
> > repository. It will end up in 2.6.20 in that case.
> >
> > Thanks,
> >
> > Hans
>
> 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.
> 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. My fix was nudging the code in
> the lookup table direction in that the new default case would
> handle updated tuner definitions that specified FM range
> parameters without adding a new case to the switch.
> (I also plan to tweak the patch a bit more by not overriding
> the config byte to force 50 kHz, if the desired_type matches,
> as a proper FM range entry should already be set to 50 kHz).
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.
>
> 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. FM radio
> really doesn't need more than 1 range entry (20 MHz is small in
> TV tuner terms), "So why not use the range entry to codify stereo
> vs mono for each tuner type instead of special case handling in
> the code?" I thought. I was later going to look back at how to
> look up stereo vs mono in default_set_radio_freq() and choose
> range 0 or 1 as appropriate.
Again, I agree that that is probably not a bad idea, but this too is a
separate issue.
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.
> So reason 1 is the only technical one; reasons 2 & 3 are
> stylistic. So I'll caution that submitting your diff right
> now will get FM radio to work for the tuner, but results could
> be suboptimal for some users due to reason 1.
See my remark under reason 1.
> Now with all that said, it'll probably take me at least two weeks
> to submit a cleaned up patch due to personal reasons.
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.
>
> Thanks for taking the time to consider my input. Keep up the
> good work.
Thanks!
Hans
_______________________________________________
ivtv-devel mailing list
[email protected]
http://ivtvdriver.org/mailman/listinfo/ivtv-devel
