|
Rockbox mail archiveSubject: Re: Radio power handlingRe: Radio power handling
From: Boris Gjenero <boris.gjenero_at_gmail.com>
Date: Thu, 02 Feb 2012 14:02:37 -0500 On 12-02-01 12:59 PM, Amaury Pouly wrote: > Hi all, > While working on the fuze+ radio (which is quite picky), I ran into the > mess known as our radio code and one issue in particular which is power > handling. Currently our code works with something I consider problematic > choice: > - a tuner API: tuner_get and tuner_set > - one power handling function: tuner_power > The problem is that tuner_get/set is handled by the driver and > tuner_power is device specific but they are used as the same level. To > put it differently, the power handling is completely bypassing the > driver. Currently this works because for most (if not all) devices, > tuner_power does not actually control the tuner power but the tuner chip > enable or something similar. A corollary is that tuner_power does not > reset the device and the registers. > If one looks carefully at the si4700 driver for example, it is clear > that it assumes the registers are never destroyed and thus that > tuner_power does not really control the power. I agree that bypassing drivers like that is a bad idea. Power management for a device must be via the driver. It must know when power has been turned off, and it should also get a chance to do a clean shutdown before power is cut. > Furthermore, nothing > prevents our radio code from calling tuner_set(RADIO_TUNE,x) while > "powered off" which of course makes no sense (I find the radio code > complicated to follow but this is not a surprise I guess). Such a thing > could be handled properly if the power state was part of the driver. I don't think this is a significant problem. There usually are some requirements about how an API is used, and this is okay as long as they're documented. Automatically powering on the tuner adds a bit of complexity and creates an opportunity for bugs which leave power turned on. > 1) Remove all direct calls to tuner_power and move them to the drivers > and replace all tuner_power(ed) calls from other parts of the code by > calls to tuner_set/get by introducing two new properties: > RADIO_POWER(ED). So concretely tuner_power(en) -> tuner_set(RADIO_POWER, > en) and tuner_powered() -> tuner_get(RADIO_POWERED). > > 2) In drivers like the si4700, implement tuner_set(RADIO_POWER,x) the > proper way by cleanly shutting show/powering up the radio before/after > calling tuner_power; thus removing the assumption that tuner_power > doesn't reset the registers. For the other drivers, we could simply > forward tuner_set(RADIO_POWER,x) to tuner_power. It seems that outside of the tuner drivers, tuner_power() is only used in apps/radio/radio.c. Furthermore, tuner_set(RADIO_SLEEP, 0) is called after tuner_power(true) and tuner_set(RADIO_SLEEP, 1) is called before tuner_power(false). You could simply remove the tuner_power() calls from radio.c and put them in code handling RADIO_SLEEP. Note that there's also tuner_set(RADIO_SLEEP, 2); which is used when the radio is paused. The tuner-specific driver can decide what power saving measures are appropriate. A simpler solution affecting just the Fuze+ would be to assume that power will be turned off after tuner_set(RADIO_SLEEP, 1) and to assume power has been off but is now on when handling tuner_set(RADIO_SLEEP, 0). I think moving the tuner_power() calls into code handling RADIO_SLEEP is better because it removes those assumptions, tuner drivers already use tuner_power(), and this would allow tuner_power() calls to be removed from radio.c. There doesn't seem to be any need for something like tuner_get(RADIO_POWERED). In radio.c and other higher level code, this information seems unnecessary. The tuner drivers can continue to use tuner_powered() if they need it. Regards, Boris Received on 2012-02-02 Page template was last modified "Tue Sep 7 00:00:02 2021" The Rockbox Crew -- Privacy Policy |