Re: [PATCH v7] sound/soc/codecs: add LAPIS Semiconductor ML26124

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

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

On Fri, Mar 09, 2012 at 03:37:06PM +0900, Tomoya MORINAGA wrote:

A substantial improvement on previous versions but there's still a few
issues, though they're now fairly small.

> +	/* SAI configuration */
> +	if (priv->channels == 1) {
> +		ml26124_update_bits(codec, ML26124_SAI_TRANS_CTL, mask,
> +				ML26124_SAI_NO_DELAY | ML26124_SAI_FRAME_SYNC);
> +		ml26124_update_bits(codec, ML26124_SAI_RCV_CTL, mask,
> +				ML26124_SAI_NO_DELAY | ML26124_SAI_FRAME_SYNC);
> +	} else {
> +		ml26124_update_bits(codec, ML26124_SAI_TRANS_CTL, mask,
> +				    ML26124_SAI_NO_DELAY);
> +		ml26124_update_bits(codec, ML26124_SAI_RCV_CTL, mask,
> +				    ML26124_SAI_NO_DELAY);
> +	}

What happens if the system switches between 1 channel mode and
multi-channel mode at runtime?  It looks like _SYNC will never be
cleared.

> +static int ml26124_set_dai_sysclk(struct snd_soc_dai *codec_dai,
> +		int clk_id, unsigned int freq, int dir)
> +{
> +	struct snd_soc_codec *codec = codec_dai->codec;
> +	struct ml26124_priv *priv = snd_soc_codec_get_drvdata(codec);
> +
> +	switch (clk_id) {
> +	case ML26124_USE_PLL:
> +		ml26124_update_bits(codec, ML26124_CLK_CTL,
> +				    BIT(0) | BIT(1), 0);
> +		break;
> +	case ML26124_USE_MCLKI_256FS:
> +		ml26124_update_bits(codec, ML26124_CLK_CTL,
> +				    BIT(0) | BIT(1), 1);
> +		break;
> +	case ML26124_USE_MCLKI_512FS:
> +		ml26124_update_bits(codec, ML26124_CLK_CTL,
> +				    BIT(0) | BIT(1), 2);
> +		break;
> +	case ML26124_USE_MCLKI_1024FS:
> +		ml26124_update_bits(codec, ML26124_CLK_CTL,
> +				    BIT(0) | BIT(1), 3);
> +		break;

Why not just specify the MCLK rate and then calculate the division based
on the sample rate?

> +	switch (freq) {
> +	case 12288000:
> +		priv->mclk = freq;
> +		break;

This is especially true given that only one MCLK rate is supported.

> +	case SND_SOC_BIAS_STANDBY:
> +		/* VMID ON */
> +		ml26124_update_bits(codec, ML26124_PW_REF_PW_MNG,
> +				    ML26124_VMID, ML26124_VMID);
> +		msleep(500);

This will sleep for 500ms when powering down which probably isn't what's
desired...

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

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