| Message ID | 1373379933-32749-2-git-send-email-richard.genoud@gmail.com |
|---|---|
| State | New |
| Headers | show |
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?
Hi Richard, On 7/9/2013 22:25, Richard Genoud wrote: [snip] > +#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" I think some of the header file include is not needed. I keep them as simple as following: ---8>--- #include <linux/clk.h> #include <linux/module.h> #include <linux/of.h> #include <linux/of_device.h> #include <linux/pinctrl/consumer.h> #include <sound/soc.h> #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
2013/7/9 Mark Brown <broonie@kernel.org>: > 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.
2013/7/10 Bo Shen <voice.shen@atmel.com>: > Hi Richard, Hi ! > On 7/9/2013 22:25, Richard Genoud wrote: > [snip] > > >> +#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" > > > I think some of the header file include is not needed. I keep them as simple > as following: > ---8>--- > #include <linux/clk.h> > #include <linux/module.h> > #include <linux/of.h> > #include <linux/of_device.h> > #include <linux/pinctrl/consumer.h> > > #include <sound/soc.h> > > #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 !
On Wed, Jul 10, 2013 at 11:25:37AM +0200, Richard Genoud wrote: > 2013/7/9 Mark Brown <broonie@kernel.org>: > > 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.
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..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 <nicolas.ferre@atmel.com> + * + * Copyright (C) 2013 Paratronic, + * Richard Genoud <richard.genoud@gmail.com> + * + * Based on sam9g20_wm8731.c by: + * Sedji Gaouaou <sedji.gaouaou@atmel.com> + * + * 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 <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" + +/* + * 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 + * |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_ops sam9x5ek_wm8731_ops = { + .startup = sam9x5ek_wm8731_startup, +}; + +static struct snd_soc_dai_link at91sam9x5ek_dai = { + .name = "WM8731", + .stream_name = "WM8731 PCM", + .codec_dai_name = "wm8731-hifi", + .init = at91sam9x5ek_wm8731_init, + .ops = &sam9x5ek_wm8731_ops, +}; + +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-wm8731-audio", }, + {}, +}; +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);