[alsa-devel] [PATCH v4] ASoC: tlv320aic31xx: Add basic codec driver implementation

[Mark Brown]

On Mon, Mar 10, 2014 at 10:52:21AM +0200, Jyri Sarha wrote:

> +- ai31xx-micbias-vg - MicBias Voltage setting
> +        0 or MICBIAS_OFF - MICBIAS output it not powered

So, on every other version of this patch set I've suggested removing
this as there's no reason why the bias would be wired up but disabled.
Each time you seem to agree that the option should be removed yet here
it is again...

> +static int aic31xx_dapm_power_event(struct snd_soc_dapm_widget *w,
> +				    struct snd_kcontrol *kcontrol, int event)
> +{
> +	struct aic31xx_priv *aic31xx = snd_soc_codec_get_drvdata(w->codec);
> +	unsigned int reg = AIC31XX_DACFLAG1;
> +	unsigned int mask;
> +
> +	dev_dbg(w->codec->dev, "%s -> %s (event: %d)\n", w->name,
> +		event == SND_SOC_DAPM_POST_PMU ? "on" :
> +		event == SND_SOC_DAPM_POST_PMD ? "off" :
> +		"(unsupported event)", event);

The core should already be logging well enough for this.

> +	switch (event) {
> +	case SND_SOC_DAPM_POST_PMU:
> +		return aic31xx_wait_bits(aic31xx, reg, mask, mask, 5000, 100);
> +	case SND_SOC_DAPM_POST_PMD:
> +		return aic31xx_wait_bits(aic31xx, reg, mask, 0, 5000, 100);
> +	}
> +
> +	dev_dbg(w->codec->dev, "Unhandled dapm widget event %d from %s\n",
> +		event, w->name);

switch with a default would be more idiomatic here.

> +static int aic31xx_add_controls(struct snd_soc_codec *codec)
> +{
> +	int err = 0;
> +	struct aic31xx_priv *aic31xx = snd_soc_codec_get_drvdata(codec);
> +
> +	if (aic31xx->pdata.codec_type & AIC31XX_STEREO_CLASS_D_BIT) {
> +		err = snd_soc_add_codec_controls(
> +			codec, aic311x_snd_controls,
> +			ARRAY_SIZE(aic311x_snd_controls));
> +		if (err < 0)
> +			dev_dbg(codec->dev, "Invalid control\n");
> +
> +	} else {
> +		err = snd_soc_add_codec_controls(
> +			codec, aic310x_snd_controls,
> +			ARRAY_SIZE(aic310x_snd_controls));
> +		if (err < 0)
> +			dev_dbg(codec->dev, "Invalid Control\n");
> +	}

Those error messages are real errors - you should print them out and
also return the error code.  Similarly for DAPM.

> +	if (event & REGULATOR_EVENT_DISABLE) {
> +		/*
> +		 * Put codec to reset and as at least one of the
> +		 * supplies was disabled. It may be to late to try
> +		 * soft reset, but it should not do any harm.
> +		 * Resetting the codec helps in minimizing leakage
> +		 * in case some of the regulators remains turned on.
> +		 */
> +		if (gpio_is_valid(aic31xx->pdata.gpio_reset))
> +			gpio_set_value(aic31xx->pdata.gpio_reset, 0);
> +		else
> +			snd_soc_write(aic31xx->codec, AIC31XX_RESET, 0x01);

I'd expect this write may fail (potentially loudly and annoyingly) if
the device really has just had its power pulled.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://mailman.alsa-project.org/pipermail/alsa-devel/attachments/20140310/1822d349/attachment.sig>