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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20120314143941.GS3133@opensource.wolfsonmicro.com>
Date:	Wed, 14 Mar 2012 14:39:41 +0000
From:	Mark Brown <broonie@...nsource.wolfsonmicro.com>
To:	Tomoya MORINAGA <tomoya.rohm@...il.com>
Cc:	Liam Girdwood <lrg@...com>, Jaroslav Kysela <perex@...ex.cz>,
	Takashi Iwai <tiwai@...e.de>, alsa-devel@...a-project.org,
	linux-kernel@...r.kernel.org, qi.wang@...el.com,
	yong.y.wang@...el.com, joel.clark@...el.com, kok.howg.ewe@...el.com
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...

Download attachment "signature.asc" of type "application/pgp-signature" (837 bytes)