Messages in this thread | | | Date | Wed, 14 Mar 2012 14:39:41 +0000 | From | Mark Brown <> | Subject | Re: [PATCH v7] sound/soc/codecs: add LAPIS Semiconductor ML26124 |
| |
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... [unhandled content-type:application/pgp-signature] | |