Re: [PATCH] ASoC: atmel: add wm8904 based audio machine driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Hi Mark,

On 07/18/2013 07:17 PM, Mark Brown wrote:
On Thu, Jul 18, 2013 at 03:28:03PM +0800, Bo Shen wrote:
add wm8904 based audio machine driver

the following ek board based on it
   - at91sam9n12ek
   - sama5d3xek (d31, d33, d34, d35)

Signed-off-by: Bo Shen <voice.shen@xxxxxxxxx>

Looks pretty good, a few fairly small issues below.

The binding document should be CCed to devicetree-discuss for review.

OK, I will cc the devicetree-discuss next version.

+	atmel,audio-routing =
+		"Headphone Jack", "HPOUTL",
+		"Headphone Jack", "HPOUTR",
+		"IN2L", "Line In Jack",
+		"IN2R", "Line In Jack",
+		"Mic", "MICBIAS",
+		"IN1L", "Mic";

The widgets defined by the board should be documented in this binding.
Those from the CODEC should be documented in the CODEC binding.

OK, I will separate this and put need information in this binding document.

+#define MCLK_RATE 32768
+static struct clk *mclk;

This should be in driver data for the board.  Shouldn't the clock be
referenced using the clock bindings?

I will try to implement this in next version.

+static int atmel_asoc_wm8904_init(struct snd_soc_pcm_runtime *rtd)
+	struct snd_soc_dai *codec_dai = rtd->codec_dai;
+	int ret;
+	ret = snd_soc_dai_set_sysclk(codec_dai, WM8904_CLK_FLL,
+			12000000, SND_SOC_CLOCK_IN);
+	if (ret < 0) {
+		pr_err("%s -failed to set wm8904 SYSCLK\n", __func__);
+		return ret;
+	}

This is an odd thing to set the clock rate to and limits the sample
rates the board can play back.  It would be better to set the clock rate
based on the requested sample rate in hw_params - you're using the FLL
so may as well take advantage of the ability it offers to support all
sample rates.
+static int atmel_asoc_wm8904_params(struct snd_pcm_substream *substream,
+		struct snd_pcm_hw_params *params)
+	struct snd_soc_pcm_runtime *rtd = substream->private_data;
+	struct snd_soc_dai *codec_dai = rtd->codec_dai;
+	int ret;
+	ret = snd_soc_dai_set_pll(codec_dai, WM8904_FLL_MCLK, WM8904_FLL_MCLK,
+		32768, params_rate(params) * 256);

Ah, in fact you do actually do that - it'd be clearer to put the
set_sysclk() after this.

OK, I will put set_sysclk here.

+	switch (level) {
+		if (!mclk_on) {
+			ret = clk_prepare_enable(mclk);
+			if (ret == 0)
+				mclk_on = 1;
+		}
+		break;
+		if (mclk_on)
+			clk_disable_unprepare(mclk);
+		mclk_on = 0;
+		break;
+	}

It's better to write this using the previous state than to have the
mclk_on flag - you want to enable on the standby->prepare transition and
disable on the standby->off transition.

OK, I will implement it in next version.

+static int atmel_asoc_wm8904_dt_init(struct platform_device *pdev)
+	struct device_node *np = pdev->dev.of_node;
+	struct device_node *codec_np, *cpu_np;
+	struct snd_soc_card *card = &atmel_asoc_wm8904_card;
+	struct snd_soc_dai_link *dailink = &atmel_asoc_wm8904_dailink;
+	int ret;
+	if (!np)
+		return -1;

Return a real error code here.



Best Reards,
Bo Shen

To unsubscribe from this list: send the line "unsubscribe linux-sound" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at

[Index of Archives]     [Pulseaudio]     [Linux Audio Users]     [ALSA Devel]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]

  Powered by Linux