Mark Brown broonie@sirena.org.uk writes:
On Tue, Jan 13, 2009 at 12:36:13PM +0100, Robert Jarzmik wrote:
As mioa701 submission was stopped due to the need of a generic scenarii API, this a first attempt to design such an API in order to unblock mioa701 submission.
You can still carry on submitting the core machine support and then patch it to add the scenario stuff later. That'd probably make review slightly easier too since it'd identify the new stuff explicitly.
Mmm ... at the risk of having another hardware incident ... why not ? Let's take chances and see if another mio overheats.
In general I'd like to see more exploration of the use cases that this is intended to satisfy, including in terms of the mioa701 itself. The documentation should make it clear that this is not intended to be a scalable solution and is only intended to be useful for hardware that is very limited.
More comments then ? You know how poor my english is, you'll have to cope with it, sorry ... I'll add some information at the beginning.
What are you using for user space - is it one of the standard stacks like FSO? Looking at the features you've got I'm a bit concerned that
No. Userspace is Trolltech's Qtopia over alsa-lib.
the scenarios may get limiting in future: with both bluetooth and GSM things are already pretty complex. The only thing it's missing compared to OpenMoko is WiFi. For example, with your current scenarios I'm not sure if it'd be possible to record a call or do simultaneous record and playback?
You're right. That functionnality is not available. That's the price to pay, somehow.
For dealing with overheating I *expect* that you only need to have the machine driver prevent particular combinations of outputs being simultaneously enabled.
Ah, I feel you're right. The problem is, we don't have the specification, so we cannot be sure what creates the overheating.
That said, this looks mostly reasonable as a scenario API. I assume that it creates new controls for the master volume and for selecting the scenario?
Yes, namely "SoC Volume" and "SoC Mode".
The main issues I can see are with how state transitions are managed and with how machine drivers interact with this if they need to update the configuration at run time.
Ah, the missing pre_scenario() and post_scenario() handlers in snd_soc_scenario I guess. I thought about these and forgot them. Will these deal with your concern ?
struct setup_mixmux { char *mixname; /* Codec Mixer or Muxer name */ int val; /* Codec Mixer or Muxer value to enforce */ };
This structure needs namespacing. Also, "Mux" not "Muxer".
Right, I'll amend that.
/**
- struct soc_scenario - describes one sound usecase
snd_soc_scenario.
- @name: Scenario name, value as will be seen in alsa "SoC Mode" alsa control
- @pin_setup: Pin configuration to perform on scenario activation
- Table of all pins, as they should be configured. Each elements is a
- pin_change value, describing how to handle a specific pin.
Table size must be the same as in snd_soc_scenario_init().The table size ought to be specified in only one place.
- @mixer_cleanup: Mixers/muxes to set up in phase (b)
- Table of all mixers/muxes to modify, NULL terminated.
- @mixer_setup: Mixers/muxers to set up in phase (c)
- Table of all mixers/muxes to modify, NULL terminated
Hrm. This all feels either too flexible or too inflexible WRT state transitions. If you do need to impose ordering beyond what DAPM can figure out for itself then I'd expect to see any number of steps being allowed. If that isn't required then the intermediate step could just be dropped. If there were going to be some sort of "idle" state to transition through I'd expect to just see that identified and then the switchover to do a state->idle->state transition.
As you wish. mixer_cleanup can be removed, as a state->idle->state will do the same thing.
This might also be better with snd_ctl_elem_values so that it can cope with any control - there's definitely a need for configuring PGAs per-scenario, for example.
Right, I'll amend that.
Please also use a number of elements parameter for consistency with the rest of the API (both here and ASoC wide).
OK.
struct soc_scenario { const char *name; /* scenario name */ const unsigned char pin_setup[]; /* pin_change for pins */ const struct setup_mixmux mixes_cleanup[]; /* mix cleanup */ const struct setup_mixmux mixes_setup[]; /* mix setup */
More natural would be pointers to arrays...
Maybe. I was thinking of some macro magic to define each soc_scenario (thus the const). Let me activate thing a bit about it.
int snd_soc_scenario_init(struct snd_soc_codec *codec, struct soc_scenario *scenario, int nb_scenarii,
Please make this take a snd_soc_card rather than a snd_soc_codec. Most of the card-wide APIs currently take a codec but this is in the process of being fixed. Also, scenarios is the more usual plural in English. For consistency with the rest of the API it'd be nice to use num rather than nb.
OK.
Some of the documentation about the situations where this API should be used should go with this function.
char *pin_names[], int nb_pins);I'm not sure why the pin names are specified separately here? Would it not be better to just use the pin lists in the scenarios.
There are _no_ pin lists in scenarios. The scenarios only define a transition for each pin index. The actual pins are initialized once in the init function (ie. pin_names[0] = "Rear Speaker" for example). Then, in each scenario, pin_setup[0] will tell what to do to "Rear Speaker" : leave it, activate it or deactivate it.
/**
- snd_soc_scenario_init - initialize soc scenarii
- @codec: codec used for the init
*/ void snd_soc_scenario_release(struct snd_soc_codec *code);
I sense bitrot :)
Yes ...
I'll send an update soon.
-- Robert