[patch 0/6] kirkwood openrd client audio support - v2

[patch 0/6] kirkwood openrd client audio support - v2

[apatard at mandriva.com]

[ added forgotten linux-arm-kernel cc: ]

Hi,

It's a new try to get merged support for openrd client audio.
I've tried to fix all comments, I hope I didn't miss something.

In the changes, there's :
- s/kirkwood/orion/
- Added a comment about data centered around 0 in the new control entry
- fixed quilt diffstat header braindamage
- added dapm support to cs42l51 codec
- use ASoC register cache support
- modified i2s driver to be a platform driver and move resource stuff to
  the right place


Arnaud Patard

[patch 1/6] orion/kirkwood: add audio functions.

[apatard at mandriva.com]

An embedded and charset-unspecified text was scrubbed...
Name: kirkwood_add_audio_dev.patch
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20100515/c7c3436f/attachment.el>

[patch 2/6] openrd-client: initialise audio

[apatard at mandriva.com]

An embedded and charset-unspecified text was scrubbed...
Name: openrd_init_audio_dev.patch
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20100515/e9a2ef36/attachment.el>

[patch 3/6] ASoC: Add SOC_DOUBLE_R_SX_TLV control

[apatard at mandriva.com]

An embedded and charset-unspecified text was scrubbed...
Name: add_soc_double_r_sx_tlv.patch
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20100515/1b796907/attachment.el>

[patch 4/6] cs42l51: add asoc driver

[apatard at mandriva.com]

An embedded and charset-unspecified text was scrubbed...
Name: cs42l51_support.patch
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20100515/0f759772/attachment.el>

[patch 5/6] orion/kirkwood: Add i2s support

[apatard at mandriva.com]

An embedded and charset-unspecified text was scrubbed...
Name: kirkwood_i2s.patch
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20100515/a27f0bcf/attachment.el>

[patch 6/6] kirkwood: Add audio support to openrd client platforms

[apatard at mandriva.com]

An embedded and charset-unspecified text was scrubbed...
Name: openrd_audio.patch
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20100515/7476679e/attachment.el>

[patch 6/6] kirkwood: Add audio support to openrd?client platforms

[Alexander Clouter]

apatard at mandriva.com wrote:
> +
> +static int openrd_client_hw_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->dai->codec_dai;
> +       struct snd_soc_dai *cpu_dai = rtd->dai->cpu_dai;
> +       int ret;
> +       unsigned int freq, fmt;
> +
> +       fmt = SND_SOC_DAIFMT_I2S | SND_SOC_DAIFMT_CBM_CFM;
> +       ret = snd_soc_dai_set_fmt(cpu_dai, fmt);
> +       if (ret < 0)
> +               return ret;
> +
> +       ret = snd_soc_dai_set_fmt(codec_dai, fmt);
> +       if (ret < 0)
> +               return ret;
> +
> +       switch (params_rate(params)) {
> +       default:
> +       case 44100:
> +               freq = 11289600;
> +               break;
> +       case 48000:
> +               freq = 12288000;
> +               break;
> +       case 96000:
> +               freq = 24576000;
> +               break;
> +       }
> +
> +       return snd_soc_dai_set_sysclk(codec_dai, 0, freq, SND_SOC_CLOCK_IN);
> +
>
The switch() could be replaced with a 'freq = params_rate(params) * 
256'.

Cheers

-- 
Alexander Clouter
.sigmonster says: NOBODY EXPECTS THE SPANISH INQUISITION!

[patch 3/6] ASoC: Add SOC_DOUBLE_R_SX_TLV control

[Mark Brown]

On Sat, May 15, 2010 at 05:30:01PM +0200, apatard at mandriva.com wrote:
> This patch is adding a new control which has the following capabilities:
> - tlv
> - variable data size (for instance, 7 ou 8 bit)
> - double mixer
> - data range centered around 0
> 
> Signed-off-by: Arnaud Patard <apatard@mandriva.com>

I've applied this one.  Please remember to CC both myself and Liam on
ASoc patches.

[patch 4/6] cs42l51: add asoc driver

[Mark Brown]

On Sat, May 15, 2010 at 05:30:02PM +0200, apatard at mandriva.com wrote:
> This patch is adding a ASoC driver for the cs42l51 from Cirrus Logic.
> Master mode and spi mode are not supported.
> 
> Signed-off-by: Arnaud Patard <apatard@mandriva.com>

This seems basically OK but there are a few things of varying severity
to fix below.  The main one is the namespacing of the constants in the
header file.

> +	/* route microphone */
> +	ret = snd_soc_write(codec, ADC_INPUT, 0xF0);
> +	if (ret < 0)
> +		goto error_alloc;

This really should be either chip default or user visible, but hopefully
when explicit control gets added users will be able to work out which
non-default value they were using so it'll be OK.

> +static const char *mic_boost[] = { "+16dB", "+32dB"};

> +	SOC_ENUM("Mic Boost", cs42l51_mic_boost),

This really should be a SOC_SINGLE_TLV() - that will mean that userspace
software like Pulse that tries to automate gain control over the full
path can include the gain from the boost amplifier.

> +static const char *cs42l51_dac_names[] = {"PCM->DAC",
> +	"PCM->SPE->DAC", "ADC->DAC"};

Conventionally these would have different names - something like "Direct
PCM", "DSP PCM" and "ADC" - since it looks a little nicer in the UI of
apps but it doesn't really matter since these are only ever likely to be
seen by people configuring systems and not end users.

> +#define DAPM_EVENT_PRE_POST_PMD	(SND_SOC_DAPM_PRE_PMD | SND_SOC_DAPM_POST_PMD)

This really ought to get pushed into the main DAPM header file if it's
going to stick, though I'd be surprised to see other users so it's not
terribly urgent.

> +	if (!rates) {
> +		printk(KERN_ERR "cs42l51: could not find a valid sample rate\n");
> +		return -EINVAL;
> +	}

You could use dev_() printks based on codec->dev or dai->dev.

> +	dev_info(&pdev->dev, "CS42L51 ALSA SoC Codec\n");

Not a big fan of chatty boots but it's no big deal.

> +#define CHIP_ID			0x1B
> +#define CHIP_REV_A		0x00
> +#define CHIP_REV_B		0x01

The macros in the header file should all be namespaced - they're likely
to collide with other things when the header is included in machine
drivers (eg, the registers for the CODEC).  Some of them are moderately
safe but things like these ones seem likely to run into trouble and it'd
be better to be consistent.

[patch 5/6] orion/kirkwood: Add i2s support

[Mark Brown]

On Sat, May 15, 2010 at 05:30:03PM +0200, apatard at mandriva.com wrote:

> This patch enables support for the i2s controller available on orion/kirkwood platforms
> 
> Signed-off-by: Arnaud Patard <apatard@mandriva.com>

All looks sensible enough, CCing in Liam.  One thing...

> +	if (status & ~(ORION_INT_CAUSE_PLAY_BYTES | \
> +			ORION_INT_CAUSE_REC_BYTES)) {
> +		printk(KERN_WARNING "%s: unexpected interrupt %lx\n",
> +			__func__, status);
> +		return IRQ_HANDLED;
> +	}

Did the driver really handle the interrupt here?  It's not even written
an ack back so IRQ_NONE might be a better response (the problem with
using IRQ_NONE before was that the driver may have handled some transfer
interrupts before it got the empty status).

[patch 6/6] kirkwood: Add audio support to openrd client platforms

[Mark Brown]

On Sat, May 15, 2010 at 05:30:04PM +0200, apatard at mandriva.com wrote:
> This patch is adding support for openrd client platforms. It's using
> the cs42l51 codec and has one mic and one speaker plugs.
> 
> Signed-off-by: Arnaud Patard <apatard@mandriva.com>

This all looks OK - CCing in Liam (and obviously this depends on both
the CODEC and CPU drivers).

> Index: sound-2.6/sound/soc/orion/Kconfig
> ===================================================================
> --- sound-2.6.orig/sound/soc/orion/Kconfig	2010-05-15 17:06:16.890586343 +0200
> +++ sound-2.6/sound/soc/orion/Kconfig	2010-05-15 17:07:30.990586832 +0200
> @@ -9,3 +9,12 @@ config SND_ORION_SOC
>  config SND_ORION_SOC_I2S
>  	tristate
>  
> +config SND_KIRKWOOD_SOC_OPENRD
> +	tristate "SoC Audio support for Kirkwood Openrd Client"
> +	depends on SND_ORION_SOC && MACH_OPENRD_CLIENT
> +	select SND_ORION_SOC_I2S
> +	select SND_SOC_CS42L51
> +	help
> +	  Say Y if you want to add support for SoC audio on
> +	  Openrd Client.
> +
> Index: sound-2.6/sound/soc/orion/Makefile
> ===================================================================
> --- sound-2.6.orig/sound/soc/orion/Makefile	2010-05-15 17:06:16.962086209 +0200
> +++ sound-2.6/sound/soc/orion/Makefile	2010-05-15 17:07:31.006586283 +0200
> @@ -4,3 +4,6 @@ snd-soc-orion-i2s-objs := orion-i2s.o
>  obj-$(CONFIG_SND_ORION_SOC) += snd-soc-orion.o
>  obj-$(CONFIG_SND_ORION_SOC_I2S) += snd-soc-orion-i2s.o
>  
> +snd-soc-openrd-objs := kirkwood-openrd.o
> +
> +obj-$(CONFIG_SND_KIRKWOOD_SOC_OPENRD) += snd-soc-openrd.o
> Index: sound-2.6/sound/soc/orion/kirkwood-openrd.c
> ===================================================================
> --- /dev/null	1970-01-01 00:00:00.000000000 +0000
> +++ sound-2.6/sound/soc/orion/kirkwood-openrd.c	2010-05-15 17:07:31.046586004 +0200
> @@ -0,0 +1,126 @@
> +/*
> + * kirkwood-openrd.c
> + *
> + * (c) 2010 Arnaud Patard <apatard@mandriva.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/interrupt.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +#include <sound/soc.h>
> +#include <mach/kirkwood.h>
> +#include <plat/audio.h>
> +#include <asm/mach-types.h>
> +#include "orion-i2s.h"
> +#include "orion-dma.h"
> +#include "../codecs/cs42l51.h"
> +
> +static int openrd_client_hw_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->dai->codec_dai;
> +	struct snd_soc_dai *cpu_dai = rtd->dai->cpu_dai;
> +	int ret;
> +	unsigned int freq, fmt;
> +
> +	fmt = SND_SOC_DAIFMT_I2S | SND_SOC_DAIFMT_CBM_CFM;
> +	ret = snd_soc_dai_set_fmt(cpu_dai, fmt);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = snd_soc_dai_set_fmt(codec_dai, fmt);
> +	if (ret < 0)
> +		return ret;
> +
> +	switch (params_rate(params)) {
> +	default:
> +	case 44100:
> +		freq = 11289600;
> +		break;
> +	case 48000:
> +		freq = 12288000;
> +		break;
> +	case 96000:
> +		freq = 24576000;
> +		break;
> +	}
> +
> +	return snd_soc_dai_set_sysclk(codec_dai, 0, freq, SND_SOC_CLOCK_IN);
> +
> +}
> +
> +static struct snd_soc_ops openrd_client_ops = {
> +	.hw_params = openrd_client_hw_params,
> +};
> +
> +
> +static struct snd_soc_dai_link openrd_client_dai[] = {
> +{
> +	.name = "CS42L51",
> +	.stream_name = "CS42L51 HiFi",
> +	.cpu_dai = &orion_i2s_dai,
> +	.codec_dai = &cs42l51_dai,
> +	.ops = &openrd_client_ops,
> +},
> +};
> +
> +
> +static struct snd_soc_card openrd_client = {
> +	.name = "OpenRD Client",
> +	.platform = &orion_soc_platform,
> +	.dai_link = openrd_client_dai,
> +	.num_links = ARRAY_SIZE(openrd_client_dai),
> +};
> +
> +static struct snd_soc_device openrd_client_snd_devdata = {
> +	.card = &openrd_client,
> +	.codec_dev = &soc_codec_device_cs42l51,
> +};
> +
> +static struct platform_device *openrd_client_snd_device;
> +
> +static int __init openrd_client_init(void)
> +{
> +	int ret;
> +
> +	if (!machine_is_openrd_client())
> +		return 0;
> +
> +	openrd_client_snd_device = platform_device_alloc("soc-audio", -1);
> +	if (!openrd_client_snd_device)
> +		return -ENOMEM;
> +
> +	platform_set_drvdata(openrd_client_snd_device,
> +			&openrd_client_snd_devdata);
> +	openrd_client_snd_devdata.dev = &openrd_client_snd_device->dev;
> +
> +	ret = platform_device_add(openrd_client_snd_device);
> +	if (ret) {
> +		printk(KERN_ERR "%s: platform_device_add failed\n", __func__);
> +		platform_device_put(openrd_client_snd_device);
> +	}
> +
> +	return ret;
> +}
> +
> +static void __exit openrd_client_exit(void)
> +{
> +	platform_device_unregister(openrd_client_snd_device);
> +}
> +
> +module_init(openrd_client_init);
> +module_exit(openrd_client_exit);
> +
> +/* Module information */
> +MODULE_AUTHOR("Arnaud Patard <apatard@mandriva.com>");
> +MODULE_DESCRIPTION("ALSA SoC OpenRD Client");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:soc-audio");
> 

[patch 1/6] orion/kirkwood: add audio functions.

[Mark Brown]

On Sat, May 15, 2010 at 05:29:59PM +0200, apatard at mandriva.com wrote:
> This patch add audio related definitions and functions

> Signed-off-by: Arnaud Patard <apatard@mandriva.com>

This patch looks OK but obviously I'm not really reviewing the arch
side.  The CPU side ASoC drivers need do depend on this, though, so
the patches for that will need to go in through the same tree or do a
merge of some kind.  I don't know what the Orion maintainer's thoughts
are there?

[patch 2/6] openrd-client: initialise audio

[Mark Brown]

On Sat, May 15, 2010 at 05:30:00PM +0200, apatard at mandriva.com wrote:
> This patch is reponsible for enabling audio on the openrd client board
> 
> Signed-off-by: Arnaud Patard <apatard@mandriva.com>

This is OK by me and there's no cross-tree dependencies for the merge so
should go via the Orion tree I guess?  For what it's worth

Acked-by: Mark Brown <broonie@opensource.wolfsonmicro.com>

[alsa-devel] [patch 5/6] orion/kirkwood: Add i2s support

[saeed bishara]

> + ? ? ? /* First disable and clear windows */
> + ? ? ? for (win_num = 0; win_num < ORION_MAX_AUDIO_WIN; win_num++) {
> + ? ? ? ? ? ? ? writel(0, base + ORION_AUDIO_WIN_CTRL_REG(win_num));
> + ? ? ? ? ? ? ? writel(0, base + ORION_AUDIO_WIN_BASE_REG(win_num));
> + ? ? ? }
> +
> + ? ? ? /* Setup windows for DDR */
> + ? ? ? for (win_num = 0; win_num < ORION_MAX_AUDIO_WIN; win_num++) {
> + ? ? ? ? ? ? ? /* We will set the Window to DRAM_CS1 in default */
> + ? ? ? ? ? ? ? struct mbus_dram_window *cs = &dram->cs[1];
> + ? ? ? ? ? ? ? writel(cs->base & 0xffff0000,
> + ? ? ? ? ? ? ? ? ? ? ?base + ORION_AUDIO_WIN_BASE_REG(win_num));
> + ? ? ? ? ? ? ? writel(((cs->size - 1) & 0xffff0000) |
> + ? ? ? ? ? ? ? ? ? ? ?(cs->mbus_attr << 8) |
> + ? ? ? ? ? ? ? ? ? ? ?(dram->mbus_dram_target_id << 4) | 1,
> + ? ? ? ? ? ? ? ? ? ? ? base + ORION_AUDIO_WIN_CTRL_REG(win_num));
> + ? ? ? }
> +}
The i2s controller has one window for playback, so that window must be
set according to the current dma address, i.e, if the dma address
falls into csX then the window should be configured to csX. same for
record.
sorry for the late comment.
saeed