Mark Brown <broonie@xxxxxxxxxxxxx> 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
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
