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
