FS#11152 - FM Tuner isn't stopped during at init

Attached to Project: Rockbox
Opened by Doru Barbu (fragilematter) - Tuesday, 30 March 2010, 08:02 GMT
Last edited by Rafaël Carré (funman) - Tuesday, 30 March 2010, 19:21 GMT
Task Type Bugs
Category FM Tuner
Status Closed
Assigned To No-one
Operating System All players
Severity Low
Priority Normal
Reported Version Daily build (which?)
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
Votes 0
Private No


radio_stop() doesn't actually do anything when it's called by radio_init(), because of faulty variable initialization,

Here's the rundown:
at the beginning of apps/recorder/radio.c, radio_status is declared and initialized with FMRADIO_OFF. When rockbox starts up, radio_init() is called, which in turn calls tuner_init() [to take care of the hardware bits] and radio_stop() [to power off the radio].
However, radio_stop() checks checks if radio_status == FMRADIO_OFF, and if it is the procedure exits without doing anything.

initialize radio_status with FMRADIO_PLAYING.
Then radio_stop() will work as intended, and in the process it will reset radio_status to FMRADIO_OFF.

Tested and working on an iRiver H10 and a Sansa E200v2. Also, the fm radio regs on the iRiver show that proper initialization is occuring.
This task depends upon

Closed by  Rafaël Carré (funman)
Tuesday, 30 March 2010, 19:21 GMT
Reason for closing:  Accepted
Additional comments about closing:  r25403
Comment by Rafaël Carré (funman) - Tuesday, 30 March 2010, 12:30 GMT
in the same file, radio_start() has:

if(radio_status == FMRADIO_PLAYING)
if(radio_status == FMRADIO_OFF)

So you're going to skip tuner_power(), right ?

Both h10 & e200v2 do not have tuner_power() (it's present but has no effect)

I think I had tried this same patch when looking at disabling clipv1 FM and went another way because of tuner_power() : I put the chip in the disabled state in tuner_init() instead.
Comment by Doru Barbu (fragilematter) - Tuesday, 30 March 2010, 16:39 GMT
No, it's about the fact that, with the current init sequence (and with radio_stop() essentially skipped), there is a chance that the tuner will be left in an inconsistent state at start.
On the H10, if you enable "Force Mono" for FM, the tuner will power-up with only the mute bit set and it will add noise to the audio path, even during playback of music. That state can only be reset by listening to the radio, thus ensuring that the radio will be muted afterwards.
Comment by Rafaël Carré (funman) - Tuesday, 30 March 2010, 17:06 GMT
I just wanted to say that the patch is incomplete, it will not run tuner_power() on targets where it's needed
Comment by Doru Barbu (fragilematter) - Tuesday, 30 March 2010, 17:24 GMT
If I understand it correctly, radio_init() is only to be called during rockbox start-up. On the other hand, radio_start() is run only when the user accesses the radio.
tuner_power() is called in the section of radio_stop() that gets executed if radio_status == FMRADIO_OFF fails the comparison.

What am I missing?
Comment by Rafaël Carré (funman) - Tuesday, 30 March 2010, 17:28 GMT
I don't know much the code, but tuner_power(true) at the start of radio_init() + your patch might be enough?
Comment by Doru Barbu (fragilematter) - Tuesday, 30 March 2010, 17:39 GMT
Well, unless radio_init() is called in some other place other than when the player boots, it should be ok. Tests on other devices are encouraged, though :D
Comment by Rafaël Carré (funman) - Tuesday, 30 March 2010, 17:53 GMT
It's only called once (as INIT_ATTR suggests)

So we'd just need someone with a device that has a tuner power to test.

Do you agree with my previous comment : add tuner_power(true) at the start of radio_init() ?
Comment by Doru Barbu (fragilematter) - Tuesday, 30 March 2010, 18:10 GMT
radio_stop() already has a tuner_power(false) command.
Isn't the tuner supposed to stay powered off if possible, until the user actually accesses the radio function? So radio_init() turns off the radio from the moment the device boots to the moment when someone starts the radio. Then radio_start() will call tuner_power(true) and everything should work out.

[boot] radio_status = FMRADIO_PLAYING
tuner_init() -> radio_stop() -> radio_status = FMRADIO_OFF && tuner_power(false);

[user wants radio] radio_start() -> tuner_power(true) && radio_status = FMRADIO_PLAYING;
Comment by Rafaël Carré (funman) - Tuesday, 30 March 2010, 18:29 GMT
I still don't get how tuner_init() can work without a tuner_power(true) before it, but that's unrelated to the patch.

I tried your patch on Clipv1 and it's OK.

Can you add a comment next to radio_status to explain why you set it to playing ?
Comment by Doru Barbu (fragilematter) - Tuesday, 30 March 2010, 19:04 GMT
At that point when rockbox has just started up, the tuner is in whatever state the physical power-up sequence puts it in.
With the variable being set the way it was previously, rockbox only initialized the tuner and left it that way (without making sure that it's muted, powered off or in whatever state rockbox would want it to be), unless the code to do that was in tuner_init().
Now tuner_init() gets all the hardware bits in the state that they should be in for the tuner to work and, by initializing radio_status with FMRADIO_PLAYING, we also make sure that radio_stop() mutes the tuner or, for the targets that support it, powers it off,

I stumbled upon this when I enabled force mono for fm. When rockbox started, it only loaded the setting and set the apropriate bit of the tuner (bit 4 of byte 3 set to 1 for the TEA5767), and left all the other regs to 0x0. In most of the targets this is just fine and the radio remains silent, even for others that have the TEA5767 apparently. But on (my) H10, this woke up the tuner and, since it wasn't muted, allowed it to output noise over the player's audio.
By explicitly passing through the whole radio_stop() function, the PLLs are initialized and, more importantly, the tuner is muted.
For other targets that support power, this also means that the tuner is formally powered off, without extra code to make that happen in the various targets' tuner_init().

Commented patch added.