Patchwork [v2,1/5] sound: sam9x5_wm8731: machine driver for at91sam9x5 wm8731 boards

login
register
mail settings
Submitter Richard Genoud
Date July 8, 2013, 1:29 p.m.
Message ID <1373290193-25300-2-git-send-email-richard.genoud@gmail.com>
Download mbox | patch
Permalink /patch/257559/
State New
Headers show

Comments

Richard Genoud - July 8, 2013, 1:29 p.m.
From: Nicolas Ferre <nicolas.ferre@atmel.com>

Description of the Asoc machine driver for an at91sam9x5 based board
with a wm8731 audio DAC. Wm8731 is clocked by a crystal and used as a
master on the SSC/I2S interface. Its connections are a headphone jack
and an Line input jack.

[Richard: this is based on an old patch from Nicolas that I forward
ported and reworked to use only device tree]

Signed-off-by: Nicolas Ferre <nicolas.ferre@atmel.com>
Signed-off-by: Uwe Kleine-K├Ânig <u.kleine-koenig@pengutronix.de>
Signed-off-by: Richard Genoud <richard.genoud@gmail.com>
---
 sound/soc/atmel/Kconfig         |   10 ++
 sound/soc/atmel/Makefile        |    2 +
 sound/soc/atmel/sam9x5_wm8731.c |  206 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 218 insertions(+)
 create mode 100644 sound/soc/atmel/sam9x5_wm8731.c
Lars-Peter Clausen - July 8, 2013, 2:39 p.m.
On 07/08/2013 03:29 PM, Richard Genoud wrote:
[...]
> +/*
> + * Logic for a wm8731 as connected on a at91sam9x5 based board.
> + */
> +static int at91sam9x5ek_wm8731_init(struct snd_soc_pcm_runtime *rtd)
> +{
[...]
> +	codec_dai->driver->playback.rates &= SNDRV_PCM_RATE_8000 |
> +		SNDRV_PCM_RATE_32000 |
> +		SNDRV_PCM_RATE_48000 |
> +		SNDRV_PCM_RATE_96000;
> +	codec_dai->driver->capture.rates &= SNDRV_PCM_RATE_8000 |
> +		SNDRV_PCM_RATE_32000 |
> +		SNDRV_PCM_RATE_48000 |
> +		SNDRV_PCM_RATE_96000;

That's not right. The driver structure is shared between all instances of
the codec, a single instance should not modify it. If you need to constrain
the list of supported rates use snd_pcm_hw_constraint_list().

> +
> +	/* set the codec system clock for DAC and ADC */
> +	ret = snd_soc_dai_set_sysclk(codec_dai, WM8731_SYSCLK_XTAL,
> +				     MCLK_RATE, SND_SOC_CLOCK_IN);
> +	if (ret < 0) {
> +		dev_err(dev, "ASoC: Failed to set WM8731 SYSCLK: %d\n", ret);
> +		return ret;
> +	}
> +
> +	/* signal a DAPM event */
> +	snd_soc_dapm_sync(dapm);

This should not be necessary.

> +	return 0;
> +}
Mark Brown - July 8, 2013, 3:07 p.m.
On Mon, Jul 08, 2013 at 03:29:49PM +0200, Richard Genoud wrote:

> + *		  Nicolas Ferre <nicolas.ferre@atmel.com>
> + *
> + * Based on sam9g20_wm8731.c by:
> + * Sedji Gaouaou <sedji.gaouaou@atmel.com>

The obvious question here is of course if we can use the same driver for
both of them.

> +	codec_dai->driver->playback.rates &= SNDRV_PCM_RATE_8000 |
> +		SNDRV_PCM_RATE_32000 |
> +		SNDRV_PCM_RATE_48000 |
> +		SNDRV_PCM_RATE_96000;
> +	codec_dai->driver->capture.rates &= SNDRV_PCM_RATE_8000 |
> +		SNDRV_PCM_RATE_32000 |
> +		SNDRV_PCM_RATE_48000 |
> +		SNDRV_PCM_RATE_96000;

You definitely shouldn't be fiddling with a driver's constant static
data.  You want to be using snd_pcm_hw_constraint() APIs to set
additional constraints intead.
Richard Genoud - July 9, 2013, 7:37 a.m.
2013/7/8 Lars-Peter Clausen <lars@metafoo.de>:
> On 07/08/2013 03:29 PM, Richard Genoud wrote:
> [...]
>> +/*
>> + * Logic for a wm8731 as connected on a at91sam9x5 based board.
>> + */
>> +static int at91sam9x5ek_wm8731_init(struct snd_soc_pcm_runtime *rtd)
>> +{
> [...]
>> +     codec_dai->driver->playback.rates &= SNDRV_PCM_RATE_8000 |
>> +             SNDRV_PCM_RATE_32000 |
>> +             SNDRV_PCM_RATE_48000 |
>> +             SNDRV_PCM_RATE_96000;
>> +     codec_dai->driver->capture.rates &= SNDRV_PCM_RATE_8000 |
>> +             SNDRV_PCM_RATE_32000 |
>> +             SNDRV_PCM_RATE_48000 |
>> +             SNDRV_PCM_RATE_96000;
>
> That's not right. The driver structure is shared between all instances of
> the codec, a single instance should not modify it. If you need to constrain
> the list of supported rates use snd_pcm_hw_constraint_list().
Ok, I'll do that.

>
>> +
>> +     /* set the codec system clock for DAC and ADC */
>> +     ret = snd_soc_dai_set_sysclk(codec_dai, WM8731_SYSCLK_XTAL,
>> +                                  MCLK_RATE, SND_SOC_CLOCK_IN);
>> +     if (ret < 0) {
>> +             dev_err(dev, "ASoC: Failed to set WM8731 SYSCLK: %d\n", ret);
>> +             return ret;
>> +     }
>> +
>> +     /* signal a DAPM event */
>> +     snd_soc_dapm_sync(dapm);
>
> This should not be necessary.
ok
>
>> +     return 0;
>> +}
>

Thanks !
Richard Genoud - July 9, 2013, 8:19 a.m.
2013/7/8 Mark Brown <broonie@kernel.org>:
> On Mon, Jul 08, 2013 at 03:29:49PM +0200, Richard Genoud wrote:
>
>> + *             Nicolas Ferre <nicolas.ferre@atmel.com>
>> + *
>> + * Based on sam9g20_wm8731.c by:
>> + * Sedji Gaouaou <sedji.gaouaou@atmel.com>
>
> The obvious question here is of course if we can use the same driver for
> both of them.
I haven't got a g20 to test that, but that's the goal.
For now, g20 is still non-DT, so I think it's best to have a DT-only
driver like this one for the 9x5 family (9g15, 9g25, 9x25, 9g35,
9x25).
When the g20 will move to DT completely, we can drop sam9g20_wm8731.c
and adjust sam9x5_wm8731.c (mainly master clock and widgets it seems)
By the way, maybe g45 could use that also (and SAMA5 ?)

>
>> +     codec_dai->driver->playback.rates &= SNDRV_PCM_RATE_8000 |
>> +             SNDRV_PCM_RATE_32000 |
>> +             SNDRV_PCM_RATE_48000 |
>> +             SNDRV_PCM_RATE_96000;
>> +     codec_dai->driver->capture.rates &= SNDRV_PCM_RATE_8000 |
>> +             SNDRV_PCM_RATE_32000 |
>> +             SNDRV_PCM_RATE_48000 |
>> +             SNDRV_PCM_RATE_96000;
>
> You definitely shouldn't be fiddling with a driver's constant static
> data.  You want to be using snd_pcm_hw_constraint() APIs to set
> additional constraints intead.
Ok, I'll change that.


Thanks !
Bo Shen - July 9, 2013, 8:37 a.m.
Hi Richard,

On 7/9/2013 16:19, Richard Genoud wrote:
> 2013/7/8 Mark Brown <broonie@kernel.org>:
>> On Mon, Jul 08, 2013 at 03:29:49PM +0200, Richard Genoud wrote:
>>
>>> + *             Nicolas Ferre <nicolas.ferre@atmel.com>
>>> + *
>>> + * Based on sam9g20_wm8731.c by:
>>> + * Sedji Gaouaou <sedji.gaouaou@atmel.com>
>>
>> The obvious question here is of course if we can use the same driver for
>> both of them.
> I haven't got a g20 to test that, but that's the goal.
> For now, g20 is still non-DT, so I think it's best to have a DT-only
> driver like this one for the 9x5 family (9g15, 9g25, 9x25, 9g35,
> 9x25).
> When the g20 will move to DT completely, we can drop sam9g20_wm8731.c
> and adjust sam9x5_wm8731.c (mainly master clock and widgets it seems)

The at91sam9g20ek board can work in DT mode, and the sound has support 
in DT mode already.

Sure, the mainly thing we need deal with is the master clock and 
widgets. If put them together will make code ugly or need many many 
#ifdef, I suggest to keep them separately.

> By the way, maybe g45 could use that also (and SAMA5 ?)

For g45ek board, it use AC97 interface, not the case.

>>
>>> +     codec_dai->driver->playback.rates &= SNDRV_PCM_RATE_8000 |
>>> +             SNDRV_PCM_RATE_32000 |
>>> +             SNDRV_PCM_RATE_48000 |
>>> +             SNDRV_PCM_RATE_96000;
>>> +     codec_dai->driver->capture.rates &= SNDRV_PCM_RATE_8000 |
>>> +             SNDRV_PCM_RATE_32000 |
>>> +             SNDRV_PCM_RATE_48000 |
>>> +             SNDRV_PCM_RATE_96000;
>>
>> You definitely shouldn't be fiddling with a driver's constant static
>> data.  You want to be using snd_pcm_hw_constraint() APIs to set
>> additional constraints intead.
> Ok, I'll change that.
>
>
> Thanks !
>

Best Regards,
Bo Shen
Mark Brown - July 9, 2013, 9:29 a.m.
On Tue, Jul 09, 2013 at 10:19:45AM +0200, Richard Genoud wrote:
> 2013/7/8 Mark Brown <broonie@kernel.org>:

> > The obvious question here is of course if we can use the same driver for
> > both of them.

> I haven't got a g20 to test that, but that's the goal.

I think I do somewhere.

> For now, g20 is still non-DT, so I think it's best to have a DT-only
> driver like this one for the 9x5 family (9g15, 9g25, 9x25, 9g35,
> 9x25).
> When the g20 will move to DT completely, we can drop sam9g20_wm8731.c
> and adjust sam9x5_wm8731.c (mainly master clock and widgets it seems)
> By the way, maybe g45 could use that also (and SAMA5 ?)

If this is the goal then this driver needs a more generic name.
Richard Genoud - July 9, 2013, 10:27 a.m.
2013/7/9 Mark Brown <broonie@kernel.org>:
> On Tue, Jul 09, 2013 at 10:19:45AM +0200, Richard Genoud wrote:
>> 2013/7/8 Mark Brown <broonie@kernel.org>:
>
>> > The obvious question here is of course if we can use the same driver for
>> > both of them.
>
>> I haven't got a g20 to test that, but that's the goal.
>
> I think I do somewhere.
>
>> For now, g20 is still non-DT, so I think it's best to have a DT-only
>> driver like this one for the 9x5 family (9g15, 9g25, 9x25, 9g35,
>> 9x25).
>> When the g20 will move to DT completely, we can drop sam9g20_wm8731.c
>> and adjust sam9x5_wm8731.c (mainly master clock and widgets it seems)
>> By the way, maybe g45 could use that also (and SAMA5 ?)
>
> If this is the goal then this driver needs a more generic name.
Hum. I guess we could call it atmel-ssc-sound.c, since the ultimate
goal would be to make it codec agnostic, but there's quite some work
to achieve that (handle the widgets, mclk and rates by DT).
But I really don't know if it's better to keep this name as long as
g20 is not supported, and change it after or have a name that is not
reflecting what the machine driver is doing right now.
Maybe Bo or Nicolas can give their feeling on that (since I'm not
Atmel, I don't know every SoC or what their Big Plan is for sound
machine drivers)

Richard.

Patch

diff --git a/sound/soc/atmel/Kconfig b/sound/soc/atmel/Kconfig
index 1c0b185..9eb8c49 100644
--- a/sound/soc/atmel/Kconfig
+++ b/sound/soc/atmel/Kconfig
@@ -33,6 +33,16 @@  config SND_AT91_SOC_SAM9G20_WM8731
 	  Say Y if you want to add support for SoC audio on WM8731-based
 	  AT91sam9g20 evaluation board.
 
+config SND_AT91_SOC_SAM9X5_WM8731
+	tristate "SoC Audio support for WM8731-based at91sam9x5 board"
+	depends on ATMEL_SSC && SND_ATMEL_SOC && SOC_AT91SAM9X5
+	select SND_ATMEL_SOC_SSC
+	select SND_ATMEL_SOC_DMA
+	select SND_SOC_WM8731
+	help
+	  Say Y if you want to add support for audio SoC on an
+	  at91sam9x5 based board that is using WM8731 codec.
+
 config SND_AT91_SOC_AFEB9260
 	tristate "SoC Audio support for AFEB9260 board"
 	depends on ARCH_AT91 && ATMEL_SSC && ARCH_AT91 && MACH_AFEB9260 && SND_ATMEL_SOC
diff --git a/sound/soc/atmel/Makefile b/sound/soc/atmel/Makefile
index 41967cc..7784c09 100644
--- a/sound/soc/atmel/Makefile
+++ b/sound/soc/atmel/Makefile
@@ -11,6 +11,8 @@  obj-$(CONFIG_SND_ATMEL_SOC_SSC) += snd-soc-atmel_ssc_dai.o
 
 # AT91 Machine Support
 snd-soc-sam9g20-wm8731-objs := sam9g20_wm8731.o
+snd-soc-sam9x5-wm8731-objs := sam9x5_wm8731.o
 
 obj-$(CONFIG_SND_AT91_SOC_SAM9G20_WM8731) += snd-soc-sam9g20-wm8731.o
+obj-$(CONFIG_SND_AT91_SOC_SAM9X5_WM8731) += snd-soc-sam9x5-wm8731.o
 obj-$(CONFIG_SND_AT91_SOC_AFEB9260) += snd-soc-afeb9260.o
diff --git a/sound/soc/atmel/sam9x5_wm8731.c b/sound/soc/atmel/sam9x5_wm8731.c
new file mode 100644
index 0000000..3e5a9f6
--- /dev/null
+++ b/sound/soc/atmel/sam9x5_wm8731.c
@@ -0,0 +1,206 @@ 
+/*
+ * sam9x5_wm8731   --	SoC audio for AT91SAM9X5-based boards
+ *			that are using WM8731 as codec.
+ *
+ *  Copyright (C) 2011 Atmel,
+ *		  Nicolas Ferre <nicolas.ferre@atmel.com>
+ *
+ * Based on sam9g20_wm8731.c by:
+ * Sedji Gaouaou <sedji.gaouaou@atmel.com>
+ *
+ * GPL
+ */
+#include <linux/module.h>
+#include <linux/moduleparam.h>
+#include <linux/kernel.h>
+#include <linux/clk.h>
+#include <linux/timer.h>
+#include <linux/interrupt.h>
+#include <linux/platform_device.h>
+#include <linux/i2c.h>
+
+#include <linux/atmel-ssc.h>
+
+#include <sound/core.h>
+#include <sound/pcm.h>
+#include <sound/pcm_params.h>
+#include <sound/soc.h>
+
+#include <asm/mach-types.h>
+#include <mach/hardware.h>
+#include <mach/gpio.h>
+
+#include "../codecs/wm8731.h"
+#include "atmel-pcm.h"
+#include "atmel_ssc_dai.h"
+
+#define MCLK_RATE 12288000
+
+#define DRV_NAME "sam9x5-snd-wm8731"
+
+/*
+ * Logic for a wm8731 as connected on a at91sam9x5 based board.
+ */
+static int at91sam9x5ek_wm8731_init(struct snd_soc_pcm_runtime *rtd)
+{
+	struct snd_soc_codec *codec = rtd->codec;
+	struct snd_soc_dai *codec_dai = rtd->codec_dai;
+	struct snd_soc_dapm_context *dapm = &codec->dapm;
+	struct device *dev = rtd->dev;
+	int ret;
+
+	dev_dbg(dev, "ASoC: %s called\n", __func__);
+
+	codec_dai->driver->playback.rates &= SNDRV_PCM_RATE_8000 |
+		SNDRV_PCM_RATE_32000 |
+		SNDRV_PCM_RATE_48000 |
+		SNDRV_PCM_RATE_96000;
+	codec_dai->driver->capture.rates &= SNDRV_PCM_RATE_8000 |
+		SNDRV_PCM_RATE_32000 |
+		SNDRV_PCM_RATE_48000 |
+		SNDRV_PCM_RATE_96000;
+
+	/* set the codec system clock for DAC and ADC */
+	ret = snd_soc_dai_set_sysclk(codec_dai, WM8731_SYSCLK_XTAL,
+				     MCLK_RATE, SND_SOC_CLOCK_IN);
+	if (ret < 0) {
+		dev_err(dev, "ASoC: Failed to set WM8731 SYSCLK: %d\n", ret);
+		return ret;
+	}
+
+	/* signal a DAPM event */
+	snd_soc_dapm_sync(dapm);
+	return 0;
+}
+
+/*
+ * Audio paths on at91sam9x5ek board:
+ *
+ *  |A| ------------> |      | ---R----> Headphone Jack
+ *  |T| <----\        |  WM  | ---L--/
+ *  |9| ---> CLK <--> | 8751 | <--R----- Line In Jack
+ *  |1| <------------ |      | <--L--/
+ */
+static const struct snd_soc_dapm_widget at91sam9x5ek_dapm_widgets[] = {
+	SND_SOC_DAPM_HP("Headphone Jack", NULL),
+	SND_SOC_DAPM_LINE("Line In Jack", NULL),
+};
+
+static struct snd_soc_dai_link at91sam9x5ek_dai = {
+	.name = "WM8731",
+	.stream_name = "WM8731 PCM",
+	.codec_dai_name = "wm8731-hifi",
+	.init = at91sam9x5ek_wm8731_init,
+};
+
+static struct snd_soc_card snd_soc_at91sam9x5ek = {
+	.owner = THIS_MODULE,
+	.dai_link = &at91sam9x5ek_dai,
+	.num_links = 1,
+	.dapm_widgets = at91sam9x5ek_dapm_widgets,
+	.num_dapm_widgets = ARRAY_SIZE(at91sam9x5ek_dapm_widgets),
+};
+
+static int sam9x5_wm8731_driver_probe(struct platform_device *pdev)
+{
+	struct device_node *np = pdev->dev.of_node;
+	struct device_node *codec_np, *cpu_np;
+	int ret;
+
+	if (!np) {
+		dev_err(&pdev->dev, "No device node supplied\n");
+		return -EINVAL;
+	}
+
+	ret = atmel_ssc_set_audio(0);
+	if (ret != 0) {
+		dev_err(&pdev->dev,
+			"ASoC: Failed to set SSC 0 for audio: %d\n", ret);
+		return ret;
+	}
+
+	snd_soc_at91sam9x5ek.dev = &pdev->dev;
+
+	ret = snd_soc_of_parse_card_name(&snd_soc_at91sam9x5ek, "atmel,model");
+	if (ret)
+		goto out;
+
+	ret = snd_soc_of_parse_audio_routing(&snd_soc_at91sam9x5ek,
+					     "atmel,audio-routing");
+	if (ret)
+		goto out;
+
+	codec_np = of_parse_phandle(np, "atmel,audio-codec", 0);
+	if (!codec_np) {
+		dev_err(&pdev->dev, "codec info missing\n");
+		ret = -EINVAL;
+		goto out;
+	}
+
+	at91sam9x5ek_dai.codec_of_node = codec_np;
+
+	cpu_np = of_parse_phandle(np, "atmel,ssc-controller", 0);
+	if (!cpu_np) {
+		dev_err(&pdev->dev, "ssc controller node missing\n");
+		ret = -EINVAL;
+		goto out;
+	}
+	at91sam9x5ek_dai.cpu_of_node = cpu_np;
+	at91sam9x5ek_dai.platform_of_node = cpu_np;
+
+	of_node_put(codec_np);
+	of_node_put(cpu_np);
+
+	at91sam9x5ek_dai.dai_fmt = snd_soc_of_parse_daifmt(np, "atmel,");
+
+	ret = snd_soc_register_card(&snd_soc_at91sam9x5ek);
+	if (ret) {
+		dev_err(&pdev->dev,
+			"ASoC: Platform device allocation failed\n");
+		goto out;
+	}
+
+	platform_set_drvdata(pdev, &snd_soc_at91sam9x5ek);
+
+	dev_dbg(&pdev->dev, "ASoC: %s ok\n", __func__);
+
+	return ret;
+
+out:
+	atmel_ssc_put_audio(0);
+	return ret;
+}
+
+static int sam9x5_wm8731_driver_remove(struct platform_device *pdev)
+{
+	struct snd_soc_card *card = platform_get_drvdata(pdev);
+
+	snd_soc_unregister_card(card);
+	atmel_ssc_put_audio(0);
+
+	return 0;
+}
+
+static const struct of_device_id sam9x5_wm8731_of_match[] = {
+	{ .compatible = "atmel,sam9x5-audio-wm8731", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, sam9x5_wm8731_of_match);
+
+static struct platform_driver sam9x5_wm8731_driver = {
+	.driver = {
+		.name = DRV_NAME,
+		.owner = THIS_MODULE,
+		.of_match_table = of_match_ptr(sam9x5_wm8731_of_match),
+	},
+	.probe = sam9x5_wm8731_driver_probe,
+	.remove = sam9x5_wm8731_driver_remove,
+};
+module_platform_driver(sam9x5_wm8731_driver);
+
+/* Module information */
+MODULE_AUTHOR("Nicolas Ferre <nicolas.ferre@atmel.com>");
+MODULE_AUTHOR("Richard Genoud <richard.genoud@gmail.com>");
+MODULE_DESCRIPTION("ALSA SoC machine driver for AT91SAM9x5 - WM8731");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:" DRV_NAME);