[PATCH v4 1/7] sound: sam9x5_wm8731: machine driver for at91sam9x5 wm8731 boards
From: Nicolas Ferre 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 Signed-off-by: Uwe Kleine-König Signed-off-by: Richard Genoud --- sound/soc/atmel/Kconfig | 10 ++ sound/soc/atmel/Makefile|2 + sound/soc/atmel/sam9x5_wm8731.c | 238 +++ 3 files changed, 250 insertions(+) create mode 100644 sound/soc/atmel/sam9x5_wm8731.c 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 000..c56e36b --- /dev/null +++ b/sound/soc/atmel/sam9x5_wm8731.c @@ -0,0 +1,238 @@ +/* + * sam9x5_wm8731 -- SoC audio for AT91SAM9X5-based boards + * that are using WM8731 as codec. + * + * Copyright (C) 2011 Atmel, + * Nicolas Ferre + * + * Copyright (C) 2013 Paratronic, + * Richard Genoud + * + * Based on sam9g20_wm8731.c by: + * Sedji Gaouaou + * + * This program is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License as published by the + * Free Software Foundation; either version 2 of the License, or (at your + * option) any later version. + * + */ + +#include +#include +#include +#include +#include +#include +#include +#include + +#include + +#include +#include +#include +#include + +#include +#include +#include + +#include "../codecs/wm8731.h" +#include "atmel-pcm.h" +#include "atmel_ssc_dai.h" + +#define MCLK_RATE 12288000 + +#define DRV_NAME "sam9x5-snd-wm8731" + +/* + * Authorized rates are: + * Rate = MCLK_RATE / (n * 2) + * Where n is in [1..4095] + * (cf register SSC_CMR) + */ +static unsigned int rates[] = { + 8000, + 16000, + 32000, + 48000, + 64000, + 96000, +}; + +static struct snd_pcm_hw_constraint_list hw_rates = { + .count = ARRAY_SIZE(rates), + .list = rates, +}; + +/* + * 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_dai *codec_dai = rtd->codec_dai; + struct device *dev = rtd->dev; + int ret; + + dev_dbg(dev, "ASoC: %s called\n", __func__); + + /* 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; + } + + return 0; +} + +static int sam9x5ek_wm8731_startup(struct snd_pcm_substream *substream) +{ + struct snd_pcm_runtime *runtime = substream->runtime; + + runtime->hw.rate_min = hw_rates.list[0]; + runtime->hw.rate_max = hw_rates.list[hw_rates.count - 1]; + runtime->hw.rates = SNDRV_PCM_RATE_KNOT; + + return snd_pcm_hw_constraint_list(runtime, 0, SNDRV_PCM_HW_PARAM_RATE, + &hw_rates); +} + +/* + * Audio paths on at91sam9x5ek board: + * + * |A| > | | ---R> Headphone Jack + * |T| <\| WM | ---L--/ + * |9| ---> CLK <--> | 8731 | <--R- Line In Jack
Re: [PATCH v4 1/7] sound: sam9x5_wm8731: machine driver for at91sam9x5 wm8731 boards
On Tue, Jul 09, 2013 at 04:25:27PM +0200, Richard Genoud wrote: > +/* > + * Authorized rates are: > + * Rate = MCLK_RATE / (n * 2) > + * Where n is in [1..4095] > + * (cf register SSC_CMR) > + */ > +static unsigned int rates[] = { > + 8000, > + 16000, > + 32000, > + 48000, > + 64000, > + 96000, > +}; Shouldn't the SSC driver be enforcing this constraint if it comes from the SSC hardware? If the clock is reprogrammable the usual convention for drivers is to not constrain if the clock is set to zero so a machine driver could remove the constraint. > + 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; > + } Shouldn't this be a parameter in the DT too? > + 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; After all we're looking things up in the DT... > + at91sam9x5ek_dai.dai_fmt = snd_soc_of_parse_daifmt(np, "atmel,"); Is this really something that machines would want to reconfigure? If so why? signature.asc Description: Digital signature ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v4 1/7] sound: sam9x5_wm8731: machine driver for at91sam9x5 wm8731 boards
Hi Richard, On 7/9/2013 22:25, Richard Genoud wrote: [snip] +#include +#include +#include +#include +#include +#include +#include +#include + +#include + +#include +#include +#include +#include + +#include +#include +#include + +#include "../codecs/wm8731.h" +#include "atmel-pcm.h" +#include "atmel_ssc_dai.h" I think some of the header file include is not needed. I keep them as simple as following: ---8>--- #include #include #include #include #include #include #include "../codecs/wm8731.h" #include "atmel_ssc_dai.h" ---<8--- +#define MCLK_RATE 12288000 + +#define DRV_NAME "sam9x5-snd-wm8731" + +/* + * Authorized rates are: + * Rate = MCLK_RATE / (n * 2) + * Where n is in [1..4095] + * (cf register SSC_CMR) + */ +static unsigned int rates[] = { + 8000, + 16000, + 32000, + 48000, + 64000, + 96000, +}; This is decided by the codec, while not ssc when ssc in slave mode. +static struct snd_pcm_hw_constraint_list hw_rates = { + .count = ARRAY_SIZE(rates), + .list = rates, +}; + [snip] + + at91sam9x5ek_dai.dai_fmt = snd_soc_of_parse_daifmt(np, "atmel,"); We can put this into at91sam9x5ek_dai directly, not need to parse it then. example as following: ---8>--- .dai_fmt = SND_SOC_DAIFMT_I2S | SND_SOC_DAIFMT_NB_NF | SND_SOC_DAIFMT_CBM_CFM, ---<8--- Best Regards, Bo Shen ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v4 1/7] sound: sam9x5_wm8731: machine driver for at91sam9x5 wm8731 boards
2013/7/9 Mark Brown : > On Tue, Jul 09, 2013 at 04:25:27PM +0200, Richard Genoud wrote: > >> +/* >> + * Authorized rates are: >> + * Rate = MCLK_RATE / (n * 2) >> + * Where n is in [1..4095] >> + * (cf register SSC_CMR) >> + */ >> +static unsigned int rates[] = { >> + 8000, >> + 16000, >> + 32000, >> + 48000, >> + 64000, >> + 96000, >> +}; > > Shouldn't the SSC driver be enforcing this constraint if it comes from > the SSC hardware? If the clock is reprogrammable the usual convention > for drivers is to not constrain if the clock is set to zero so a machine > driver could remove the constraint. Actually, my comment is buggy here (or at least, unrelated to the authorized rates). The "MCLK_RATE" is the master clock of the wm8731 codec (a 12.288MHz crystal). According to the datasheet of wm8731, when a 12.288 crystal is used, the authorized rates are 8, 32, 48 and 96kHz (I have to remove 16 and 64kHz). So, is this the right place for the rates ? >> + 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; >> + } > > Shouldn't this be a parameter in the DT too? Yes, I'll add that to the DT. >> + 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; > > After all we're looking things up in the DT... > >> + at91sam9x5ek_dai.dai_fmt = snd_soc_of_parse_daifmt(np, "atmel,"); > > Is this really something that machines would want to reconfigure? If so > why? That's right. There's no point reconfiguring that because I2S is the only possible interface. Thanks for your comments ! Richard. ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v4 1/7] sound: sam9x5_wm8731: machine driver for at91sam9x5 wm8731 boards
2013/7/10 Bo Shen : > Hi Richard, Hi ! > On 7/9/2013 22:25, Richard Genoud wrote: > [snip] > > >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> +#include >> + >> +#include >> +#include >> +#include >> +#include >> + >> +#include >> +#include >> +#include >> + >> +#include "../codecs/wm8731.h" >> +#include "atmel-pcm.h" >> +#include "atmel_ssc_dai.h" > > > I think some of the header file include is not needed. I keep them as simple > as following: > ---8>--- > #include > #include > #include > #include > #include > > #include > > #include "../codecs/wm8731.h" > #include "atmel_ssc_dai.h" > ---<8--- ooopps ! I forgot to do some cleaning in those after the file rework. Thanks ! > >> +#define MCLK_RATE 12288000 >> + >> +#define DRV_NAME "sam9x5-snd-wm8731" >> + >> +/* >> + * Authorized rates are: >> + * Rate = MCLK_RATE / (n * 2) >> + * Where n is in [1..4095] >> + * (cf register SSC_CMR) >> + */ >> +static unsigned int rates[] = { >> + 8000, >> + 16000, >> + 32000, >> + 48000, >> + 64000, >> + 96000, >> +}; > > > This is decided by the codec, while not ssc when ssc in slave mode. yes. >> +static struct snd_pcm_hw_constraint_list hw_rates = { >> + .count = ARRAY_SIZE(rates), >> + .list = rates, >> +}; >> + > > > [snip] > > >> + >> + at91sam9x5ek_dai.dai_fmt = snd_soc_of_parse_daifmt(np, "atmel,"); > > > We can put this into at91sam9x5ek_dai directly, not need to parse it then. > example as following: > ---8>--- > .dai_fmt = SND_SOC_DAIFMT_I2S > | SND_SOC_DAIFMT_NB_NF > | SND_SOC_DAIFMT_CBM_CFM, > ---<8--- yes, I removed that for the older machine driver, without thinking that much. It's better hardcorded like that. Thanks for your comments ! ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v4 1/7] sound: sam9x5_wm8731: machine driver for at91sam9x5 wm8731 boards
On Wed, Jul 10, 2013 at 11:25:37AM +0200, Richard Genoud wrote: > 2013/7/9 Mark Brown : > > Shouldn't the SSC driver be enforcing this constraint if it comes from > > the SSC hardware? If the clock is reprogrammable the usual convention > > for drivers is to not constrain if the clock is set to zero so a machine > > driver could remove the constraint. > Actually, my comment is buggy here (or at least, unrelated to the > authorized rates). > The "MCLK_RATE" is the master clock of the wm8731 codec (a 12.288MHz crystal). > According to the datasheet of wm8731, when a 12.288 crystal is used, > the authorized rates are 8, 32, 48 and 96kHz (I have to remove 16 and > 64kHz). > So, is this the right place for the rates ? No, the CODEC driver should be enforcing the restrictions if that's where they come from. signature.asc Description: Digital signature ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss