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
> - 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
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
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.
Received on 2012-02-02