[alsa-devel] SoC scenarii API

[Mark Brown]

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.

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.  

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
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?  For dealing with overheating I *expect* that you only need to
have the machine driver prevent particular combinations of outputs being
simultaneously enabled.

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?  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.

> 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".

> /**
>  * 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.

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.  Please also use a number of elements
parameter for consistency with the rest of the API (both here and ASoC
wide).

> 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...

> 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.

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.

> /**
>  * 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 :)