Re: [PATCH] ASoC: codecs: Add AB8500 codec-driver

[Mark Brown <broonie@opensource.wolfsonmicro.com>]

[-- Attachment #1.1: Type: text/plain, Size: 2284 bytes --]

On Fri, Jun 01, 2012 at 12:50:34PM +0200, Ola Lilja wrote:

> +/*
> + * Extended interface for codec-driver
> + */

So, a few issues below.  Could you please submit a version with this
extended API removed or made driver internal as much as possible?  The
rest of the driver looks good so it'd be good to split this stuff out
and review separately.

> +int ab8500_audio_init_audioblock(struct snd_soc_codec *codec)
> +{
> +	int status;
> +
> +	dev_dbg(codec->dev, "%s: Enter.\n", __func__);
> +
> +	/* Reset audio-registers and disable 32kHz-clock output 2 */
> +	status = ab8500_sysctrl_write(AB8500_STW4500CTRL3,
> +				AB8500_STW4500CTRL3_CLK32KOUT2DIS |
> +					AB8500_STW4500CTRL3_RESETAUDN,
> +				AB8500_STW4500CTRL3_RESETAUDN);
> +	if (status < 0)
> +		return status;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(ab8500_audio_init_audioblock);

This is really odd (I'm sure I've queried this on previous versions of
the driver) - why is something else reinitialising the device?
Shouldn't the CODEC driver be doing this by itself - if nothing else I'd
expect it to cause confusion if the device is reinitialised underneath
the driver?

I didn't register this last time as I'd assumed that the issue was a
missing static rather than something else calling the function.

> +int ab8500_audio_setup_mics(struct snd_soc_codec *codec,
> +			struct amic_settings *amics)
> +{

Similar thing here, I'd expect this to just be platform data.

> +int ab8500_audio_set_ear_cmv(struct snd_soc_codec *codec,
> +			enum ear_cm_voltage ear_cmv)
> +{
> +	char *cmv_str;
> +
> +	switch (ear_cmv) {
> +	case EAR_CMV_0_95V:
> +		cmv_str = "0.95V";
> +		break;
> +	case EAR_CMV_1_10V:
> +		cmv_str = "1.10V";
> +		break;
> +	case EAR_CMV_1_27V:
> +		cmv_str = "1.27V";
> +		break;
> +	case EAR_CMV_1_58V:
> +		cmv_str = "1.58V";
> +		break;
> +	default:
> +		dev_err(codec->dev,
> +			"%s: Unknown earpiece CM-voltage (%d)!\n",
> +			__func__, (int)ear_cmv);
> +		return -EINVAL;
> +	}
> +	dev_dbg(codec->dev, "%s: Earpiece CM-voltage: %s\n", __func__,
> +		cmv_str);
> +	snd_soc_update_bits(codec, AB8500_ANACONF1, AB8500_ANACONF1_EARSELCM,
> +			ear_cmv);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(ab8500_audio_set_ear_cmv);

This is *possibly* OK, though again it does look platform dataish.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]