[alsa-devel] [PATCH 6/6] ASoC: TWL4030: Add analog loopback support

[Mark Brown]

On Tue, Jan 27, 2009 at 11:29:44AM +0200, Peter Ujfalusi wrote:

> It seams that the analog loopback needs the DAC powered on on the channel,
> where the loopback is selected. The switch for the DACs has been moved from

Oh, well...

> After the loopback registers are changed, the event callback will update the
> bypass bitfield, so we will know if any of the bypass paths are
> enabled. If the codec is in STANDBY mode, the decision is made to keep/switch
> the codec off or keep/switch the codec on to allow the bypass.
> Also after the codec switches from ON to STANDBY the same check will be done.

The original implementation was done that way due to the lack of DAPM -
it did *all* power management in the bias configuration.  Now that you
have DAPM support it should be possible for the driver to function more
normally and avoid these checks.  What most drivers do is bring all the
chip wide power up when coming into standby then power on the rest under
DAPM.

Support for turning the codec completely off should really be provided
by the core as an optional thing that can be done when the subsystem has
been idle for a while using the regular bias level stuff like the
existing delay before falling back to standby.  It needs to be optional
since won't be appropriate for all applications since bringing the bias
levels up cleanly may take too long for some systems.

A few more specific issues in the rest of the code below.  I've also got
a patch I'm going to push to Takashi later today which conflicts with
this one, see the for-2.6.30 branch of:

	git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound-2.6.git

>  static void twl4030_codec_enable(struct snd_soc_codec *codec, int enable)
>  {
> +       struct twl4030_priv *twl4030 =
> +               (struct twl4030_priv *)codec->private_data;

Casts away from void aren't needed, there's quite a few of those here
and in the rest of the patch code.  Please fix these - they're usually a
sign that something is wrong if they're needed.

>         codec = kzalloc(sizeof(struct snd_soc_codec), GFP_KERNEL);
>         if (codec == NULL)
>                 return -ENOMEM;
> 
> +       twl4030 = kzalloc(sizeof(struct twl4030_priv), GFP_KERNEL);
> +       if (twl4030 == NULL) {

Not essential but you could collapse these into a single allocation.