December 2012 - Alsa-devel - mailman.alsa-project.org

[alsa-devel] [PATCH RFT] ASoC: wm2200: Fix setting dai format in wm2200_set_fmt
by Axel Lin 21 Dec '12

21 Dec '12

According to the defines in wm2200.h: /* * R1284 (0x504) - Audio IF 1_5 */ #define WM2200_AIF1_FMT_MASK 0x0007 /* AIF1_FMT - [2:0] */ #define WM2200_AIF1_FMT_SHIFT 0 /* AIF1_FMT - [2:0] */ #define WM2200_AIF1_FMT_WIDTH 3 /* AIF1_FMT - [2:0] */ We should not left shift 1 bit for fmt_val when setting dai format. Signed-off-by: Axel Lin <axel.lin(a)ingics.com> --- Hi Mark, wm2200.h defines WM2200_AIF1_FMT_SHIFT to be 0, so current code looks wrong to me. I don't have the datasheet, so can you check if this patch is correct? Axel sound/soc/codecs/wm2200.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sound/soc/codecs/wm2200.c b/sound/soc/codecs/wm2200.c index afcf31d..a12fc2f 100644 --- a/sound/soc/codecs/wm2200.c +++ b/sound/soc/codecs/wm2200.c @@ -1626,7 +1626,7 @@ static int wm2200_set_fmt(struct snd_soc_dai *dai, unsigned int fmt) WM2200_AIF1TX_LRCLK_MSTR | WM2200_AIF1TX_LRCLK_INV, lrclk); snd_soc_update_bits(codec, WM2200_AUDIO_IF_1_5, - WM2200_AIF1_FMT_MASK << 1, fmt_val << 1); + WM2200_AIF1_FMT_MASK, fmt_val); return 0; } -- 1.7.9.5

2 1

[alsa-devel] [PATCH 0/3] ASoC: fsi: cleanup FSI driver
by Kuninori Morimoto 21 Dec '12

21 Dec '12

Hi Mark Cc Simon These patches changed platform setting to use FSI new style, and cleanup FSI driver. Kuninori Morimoto (3): ASoC: fsi: remove platform depended .set_rate() callback support ASoC: fsi: remove SH_FSI_xxx_INV flags ASoC: fsi: cleanup sh_fsi.h arch/arm/mach-shmobile/board-ap4evb.c | 11 +-- arch/arm/mach-shmobile/board-mackerel.c | 11 +-- arch/sh/boards/mach-ecovec24/setup.c | 12 +-- arch/sh/boards/mach-se/7724/setup.c | 12 +-- include/sound/sh_fsi.h | 70 +------------- sound/soc/sh/fsi.c | 156 +++---------------------------- 6 files changed, 27 insertions(+), 245 deletions(-) >> Simon #2 patch modifies both FSI driver and platform. So, it needs your Acked-by Best regards --- Kuninori Morimoto

1 5

[alsa-devel] [PATCHv2 0/9] ALSA: CA0132: DSP Update
by Ian Minett 21 Dec '12

21 Dec '12

From: Ian Minett <ian_minett(a)creativelabs.com> Thanks for the comments. This resubmit should help to correct many of the points raised. For example, use of C99-style init for structures, the previous large patch is now split into several subpatches, and there are more descriptions to the code and individual patchfiles. Patch Summary: 1: Add new definitions and structures that will be used by the DSP routines 2: Init chip registers, DSP parameters and mixer settings 3: Add the new DSP mixer kcontrols and helpers. 4: Update and add PCM callbacks and enhancements. 5: Add unsol handler for DSP response and jack detection reporting 6: Add tuning controls 7/8: Code shuffle and tidy, split into two for a cleaner diff. 9: Updates to the mic sample rate and module description, and fix potential errors on init. Thanks, - Ian Signed-off-by: Ian Minett <ian_minett(a)creativelabs.com>

1 9

Re: [alsa-devel] [PATCH 5/7] ASoC: tegra: add ac97 host driver
by Stephen Warren 20 Dec '12

20 Dec '12

On 12/19/2012 04:17 PM, Lucas Stach wrote: > This adds the driver for the Tegra 2x AC97 host controller. The AC'97 DT binding file should really be added as part of this patch. I'm not at all familiar with AC'97, but this mostly looks fine. I have a bunch of comments below though, mostly for my own understanding. > diff --git a/sound/soc/tegra/Kconfig b/sound/soc/tegra/Kconfig > +config SND_SOC_TEGRA20_AC97 > + tristate > + depends on SND_SOC_TEGRA && ARCH_TEGRA_2x_SOC This should also select SND_SOC_TEGRA20_DAS, just like the I2S driver. > diff --git a/sound/soc/tegra/tegra20_ac97.c b/sound/soc/tegra/tegra20_ac97.c > +static struct tegra20_ac97 *workdata; Why not put that into the device's drvdata? Oh, is there no way to stash driver-private data into the struct snd_ac97? > +static void tegra20_ac97_codec_reset(struct snd_ac97 *ac97) > +{ > + u32 readback; > + unsigned long timeout; > + > + /* reset line is not driven by DAC pad group, have to toggle GPIO */ > + gpio_set_value(workdata->reset_gpio, 0); > + udelay(2); I'm not quite sure what that implies. Might some (Tegra) HW designs use a dedicated signal from the AC'97 HW to reset the CODEC and others not? That would make the GPIO optional. Or, does the comment mean that Tegra doesn't ever have a dedicated CODEC reset signal, so we always have to use a GPIO? If so, this comment might be more appropriate inside probe() where I asked my related questions. > +static void tegra20_ac97_codec_warm_reset(struct snd_ac97 *ac97) > +{ > + u32 readback; > + unsigned long timeout; > + > + /* > + * although sync line is driven by the DAC pad group warm reset using > + * the controller cmd is not working, have to toggle sync line > + * manually. > + */ > + gpio_request(workdata->sync_gpio, "codec-sync"); Hmm. There's an AC'97 command to reset the CODEC and we don't implement it? Uggh. > +static const struct snd_soc_dai_ops tegra20_ac97_dai_ops = { > + .trigger = tegra20_ac97_trigger, > +}; No .set_fmt or .hw_params? Does the AC'97 ASoC core code handle that somehow? I guess that's what soc_ac97_ops is for? > +static struct snd_soc_dai_driver tegra20_ac97_dai = { > + .name = "tegra-ac97-pcm", > + .ac97_control = 1, > + .probe = tegra20_ac97_probe, > + .playback = { > + .stream_name = "PCM Playback", Out of curiosity, why "PCM Playback" not just "Playback"? > +static bool tegra20_ac97_volatile_reg(struct device *dev, unsigned int reg) > +{ > + switch (reg) { > + case TEGRA20_AC97_CMD: ... > + return true; CMD is volatile; did we put status bits in there? > +static int tegra20_ac97_platform_probe(struct platform_device *pdev) > + ac97->reset_gpio = of_get_named_gpio(pdev->dev.of_node, > + "nvidia,codec-reset-gpio", 0); > + if (gpio_is_valid(ac97->reset_gpio)) { > + ret = devm_gpio_request_one(&pdev->dev, ac97->reset_gpio, > + GPIOF_OUT_INIT_HIGH, "codec-reset"); Shouldn't this get the flags from the GPIO specifier, and deal with active-high/active-low resets, or is the polarity set by the AC'97 spec? > + if (ret) { > + dev_err(&pdev->dev, "could not get codec-reset GPIO\n"); > + goto err_clk_put; > + } > + } else { > + dev_err(&pdev->dev, "no codec-reset GPIO supplied\n"); > + goto err_clk_put; > + } Is a reset GPIO necessarily required? What if a board arranged to reset the CODEC during power-on-reset, and there was no software control over the reset? > + ac97->sync_gpio = of_get_named_gpio(pdev->dev.of_node, > + "nvidia,codec-sync-gpio", 0); > + if (!gpio_is_valid(ac97->sync_gpio)) { > + dev_err(&pdev->dev, "no codec-sync GPIO supplied\n"); > + goto err_clk_put; > + } I don't know what this is, so I'll ask if it's strictly required too. > + ret = snd_soc_register_dais(&pdev->dev, &tegra20_ac97_dai, 1); snd_soc_register_dai() would be marginally simpler. > + ret = clk_prepare_enable(ac97->clk_ac97); > + if (ret) { > + dev_err(&pdev->dev, "clk_enable failed: %d\n", ret); > + goto err_asoc_utils_fini; > + } Can you do runtime PM instead? See the I2S driver for an example. > +MODULE_LICENSE("GPL"); "GPL v2"

2 1

Re: [alsa-devel] [PATCH 5/7] ASoC: tegra: add ac97 host driver
by Stephen Warren 20 Dec '12

20 Dec '12

On 12/20/2012 01:10 PM, Lucas Stach wrote: > Am Donnerstag, den 20.12.2012, 12:44 -0700 schrieb Stephen Warren: >> On 12/19/2012 04:17 PM, Lucas Stach wrote: >>> This adds the driver for the Tegra 2x AC97 host controller. >>> +static void tegra20_ac97_codec_warm_reset(struct snd_ac97 *ac97) >>> +{ >>> + u32 readback; >>> + unsigned long timeout; >>> + >>> + /* >>> + * although sync line is driven by the DAC pad group warm reset using >>> + * the controller cmd is not working, have to toggle sync line >>> + * manually. >>> + */ >>> + gpio_request(workdata->sync_gpio, "codec-sync"); >> >> Hmm. There's an AC'97 command to reset the CODEC and we don't implement >> it? Uggh. > > As far as I could figure from the downstream register doc there is a > command implemented in the host controller that's supposed to do the > warm reset, but in my testing it didn't work. Sadly I have not received > any documentation about the host controller from NVIDIA up until now, so > I don't now if we could make this work somehow. Bug Submission ID#: > 196075 What system was that bug submitted into? It's not a regular NVIDIA internal bug number. I'll see who I can ask about this feature... >>> + ac97->sync_gpio = of_get_named_gpio(pdev->dev.of_node, >>> + "nvidia,codec-sync-gpio", 0); >>> + if (!gpio_is_valid(ac97->sync_gpio)) { >>> + dev_err(&pdev->dev, "no codec-sync GPIO supplied\n"); >>> + goto err_clk_put; >>> + } >> >> I don't know what this is, so I'll ask if it's strictly required too. > > Until we manage to get the host controller warm reset command working > this is fixed to be the _FS line of the DAP connected to the codec, so > strictly required. I'd like to investigate whether the warm reset command can be made to work before actually committing to a DT binding that requires this GPIO to be specified; the DT binding is supposed to be basically static.

1 0

Re: [alsa-devel] [PATCH 7/7] ASoC: tegra: add machine driver using wm9712 codec
by Stephen Warren 20 Dec '12

20 Dec '12

On 12/19/2012 04:17 PM, Lucas Stach wrote: > This adds a simple machine driver using the Wolfson wm9712 AC97 > codec. If the AC'97 code is going to be reworked to probe the CODEC rather than manually specify it in DT, then will this machine driver become generic for any AC'97 system, and hence you could rename the DT binding to nvidia,tegra-audio-ac97? > diff --git a/Documentation/devicetree/bindings/sound/nvidia,tegra-audio-wm9712.txt b/Documentation/devicetree/bindings/sound/nvidia,tegra-audio-wm9712.txt > +- nvidia,audio-codec : The phandle of the WM9712 audio codec I wonder how you'll get that if you dynamically probe the AC'97 bus... > diff --git a/sound/soc/tegra/Kconfig b/sound/soc/tegra/Kconfig > +config SND_SOC_TEGRA_WM9712 > + tristate "SoC Audio support for Tegra boards using a WM9712 codec" > + depends on SND_SOC_TEGRA I'd suggest making this depend on the Tegra AC97 module, or ARCH_TEGRA_2x_SOC instead, since it can only run on Tegra20. The AC'97 module has been removed on Tegra30 and replaced with an HD Audio controller instead. > + select SND_SOC_TEGRA20_AC97 if ARCH_TEGRA_2x_SOC That select (or a dependency as mentioned above) is always required. > + select SND_SOC_AC97_BUS Shouldn'y the AC'97 driver select that? > diff --git a/sound/soc/tegra/tegra_wm9712.c b/sound/soc/tegra/tegra_wm9712.c > +static int tegra_wm9712_init(struct snd_soc_pcm_runtime *rtd) > + snd_soc_dapm_force_enable_pin(dapm, "Mic Bias"); Hmm. I guess that potentially ties this driver to whichever CODECs actually expose a widget with that exact name... > +static int tegra_wm9712_remove(struct snd_soc_card *card) > +{ > + return 0; > +} Do you need to implement this at all if it does nothing? > +MODULE_LICENSE("GPL"); "GPL v2"

2 1

Re: [alsa-devel] [PATCH 6/7] ASoC: tegra: add ac97 host controller to device tree
by Stephen Warren 20 Dec '12

20 Dec '12

On 12/19/2012 04:17 PM, Lucas Stach wrote: > This adds the DT bindings and a default entry for the Tegra 2x > AC97 host controller. Everything here except the .dtsi file should be part of the previous patch. > diff --git a/Documentation/devicetree/bindings/sound/nvidia,tegra20-ac97.txt b/Documentation/devicetree/bindings/sound/nvidia,tegra20-ac97.txt > +- nvidia,codec-sync-gpio : The Tegra GPIO controller's phandle and the number > + of the GPIO corresponding with the AC97 DAP _FS line Hmm, for I2S at least, FS (frame sync I assume) definitely is something that the audio controller itself drives. Are you absolutely sure that this should be represented as a GPIO here? > diff --git a/arch/arm/boot/dts/tegra20.dtsi b/arch/arm/boot/dts/tegra20.dtsi > + tegra_ac97: ac97@70002000 { There's only 1 node named ac97, so you can leave out the unit address since it's not required to generate a unique node name. In other words just "ac97".

1 0

Re: [alsa-devel] [PATCH 1/7] ARM: tegra: add ac97 clock
by Stephen Warren 20 Dec '12

20 Dec '12

On 12/19/2012 04:17 PM, Lucas Stach wrote: > Add clock used by the AC97 host controller. This is going to conflict with Prashant's clock driver rework. Can you rebase it on top of that once it's checked in. That might be a couple weeks out though.

1 0

Re: [alsa-devel] [PATCH 4/7] ASoC: tegra: add function to set ac97 rate
by Stephen Warren 20 Dec '12

20 Dec '12

On 12/19/2012 04:17 PM, Lucas Stach wrote: > AC97 uses a fixed rate, unrelated to the sample rate. Add a function to > make the setup more trivial. Acked-by: Stephen Warren <swarren(a)nvidia.com>

1 0

Re: [alsa-devel] [PATCH 3/7] ASoC: tegra: setup DAP3<->DAC3 connection by default
by Stephen Warren 20 Dec '12

20 Dec '12

On 12/19/2012 04:17 PM, Lucas Stach wrote: > This connection is used by the AC97 controller. Acked-by: Stephen Warren <swarren(a)nvidia.com>

1 0