[patch 0/6] kirkwood openrd client audio support

[patch 0/6] kirkwood openrd client audio support

[apatard]

This patchset aims at adding ASoC support to ARM kirkwood systems and
more precisely to openrd client boards.


Arnaud Patard

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

[apatard]

[-- Attachment #1: kirkwood_add_audio_dev.patch --]
[-- Type: text/plain, Size: 3829 bytes --]

This patch add audio related definitions and functions


Signed-off-by: Arnaud Patard <apatard@mandriva.com>
---
 arch/arm/mach-kirkwood/common.c                |    9 	9 +	0 -	0 !
 arch/arm/mach-kirkwood/common.h                |    2 	2 +	0 -	0 !
 arch/arm/mach-kirkwood/include/mach/audio.h    |   13 	13 +	0 -	0 !
 arch/arm/mach-kirkwood/include/mach/kirkwood.h |    3 	3 +	0 -	0 !
 4 files changed, 27 insertions(+)
 create mode 100644 include/linux/mv88fx_audio.h
 create mode 100644 sound/soc/kirkwood/Kconfig
 create mode 100644 sound/soc/kirkwood/Makefile
 create mode 100644 sound/soc/kirkwood/cs42l51.c
 create mode 100644 sound/soc/kirkwood/cs42l51.h
 create mode 100644 sound/soc/kirkwood/kirkwood_audio_hal.c
 create mode 100644 sound/soc/kirkwood/kirkwood_audio_hal.h
 create mode 100644 sound/soc/kirkwood/kirkwood_audio_regs.h
 create mode 100644 sound/soc/kirkwood/kirkwood_pcm.c

Index: linux-2.6.33/arch/arm/mach-kirkwood/common.c
===================================================================
--- linux-2.6.33.orig/arch/arm/mach-kirkwood/common.c	2010-05-11 17:51:25.537649820 +0200
+++ linux-2.6.33/arch/arm/mach-kirkwood/common.c	2010-05-11 17:52:39.790150263 +0200
@@ -25,6 +25,7 @@
 #include <asm/mach/time.h>
 #include <mach/kirkwood.h>
 #include <mach/bridge-regs.h>
+#include <mach/audio.h>
 #include <plat/cache-feroceon-l2.h>
 #include <plat/ehci-orion.h>
 #include <plat/mvsdio.h>
@@ -977,3 +978,11 @@ static int __init kirkwood_clock_gate(vo
 	return 0;
 }
 late_initcall(kirkwood_clock_gate);
+
+/*****************************************************************************
+ * Audio
+ ****************************************************************************/
+void __init kirkwood_audio_init(struct kirkwood_soc_platform_data *audio_data)
+{
+	kirkwood_clk_ctrl |= CGC_AUDIO;
+}
Index: linux-2.6.33/arch/arm/mach-kirkwood/common.h
===================================================================
--- linux-2.6.33.orig/arch/arm/mach-kirkwood/common.h	2010-05-11 17:51:25.641649839 +0200
+++ linux-2.6.33/arch/arm/mach-kirkwood/common.h	2010-05-11 17:52:39.790150263 +0200
@@ -16,6 +16,7 @@ struct mv643xx_eth_platform_data;
 struct mv_sata_platform_data;
 struct mvsdio_platform_data;
 struct mtd_partition;
+struct kirkwood_soc_platform_data;
 
 /*
  * Basic Kirkwood init functions used early by machine-setup.
@@ -41,6 +42,7 @@ void kirkwood_i2c_init(void);
 void kirkwood_uart0_init(void);
 void kirkwood_uart1_init(void);
 void kirkwood_nand_init(struct mtd_partition *parts, int nr_parts, int delay);
+void kirkwood_audio_init(struct kirkwood_soc_platform_data *audio_data);
 
 extern int kirkwood_tclk;
 extern struct sys_timer kirkwood_timer;
Index: linux-2.6.33/arch/arm/mach-kirkwood/include/mach/kirkwood.h
===================================================================
--- linux-2.6.33.orig/arch/arm/mach-kirkwood/include/mach/kirkwood.h	2010-05-11 17:51:25.585649926 +0200
+++ linux-2.6.33/arch/arm/mach-kirkwood/include/mach/kirkwood.h	2010-05-11 17:52:39.790150263 +0200
@@ -96,6 +96,9 @@
 
 #define SDIO_PHYS_BASE		(KIRKWOOD_REGS_PHYS_BASE | 0x90000)
 
+#define AUDIO_PHYS_BASE		(KIRKWOOD_REGS_PHYS_BASE | 0xA0000)
+#define AUDIO_VIRT_BASE		(KIRKWOOD_REGS_VIRT_BASE | 0xA0000)
+
 /*
  * Supported devices and revisions.
  */
Index: linux-2.6.33/arch/arm/mach-kirkwood/include/mach/audio.h
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ linux-2.6.33/arch/arm/mach-kirkwood/include/mach/audio.h	2010-05-11 17:52:39.790150263 +0200
@@ -0,0 +1,13 @@
+#ifndef __MACH_AUDIO_H
+#define __MACH_AUDIO_H
+
+#include <linux/mbus.h>
+
+struct kirkwood_soc_platform_data {
+	u32 tclk;
+	struct mbus_dram_target_info *dram;
+};
+
+extern struct kirkwood_soc_platform_data kirkwood_soc_data;
+
+#endif

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

[Liam Girdwood]

On Tue, 2010-05-11 at 18:23 +0200, apatard@mandriva.com wrote:
> plain text document attachment (kirkwood_add_audio_dev.patch)
> This patch add audio related definitions and functions
> 
> 
> Signed-off-by: Arnaud Patard <apatard@mandriva.com>
> ---
>  arch/arm/mach-kirkwood/common.c                |    9 	9 +	0 -	0 !
>  arch/arm/mach-kirkwood/common.h                |    2 	2 +	0 -	0 !
>  arch/arm/mach-kirkwood/include/mach/audio.h    |   13 	13 +	0 -	0 !
>  arch/arm/mach-kirkwood/include/mach/kirkwood.h |    3 	3 +	0 -	0 !
>  4 files changed, 27 insertions(+)
>  create mode 100644 include/linux/mv88fx_audio.h
>  create mode 100644 sound/soc/kirkwood/Kconfig
>  create mode 100644 sound/soc/kirkwood/Makefile
>  create mode 100644 sound/soc/kirkwood/cs42l51.c
>  create mode 100644 sound/soc/kirkwood/cs42l51.h
>  create mode 100644 sound/soc/kirkwood/kirkwood_audio_hal.c
>  create mode 100644 sound/soc/kirkwood/kirkwood_audio_hal.h
>  create mode 100644 sound/soc/kirkwood/kirkwood_audio_regs.h
>  create mode 100644 sound/soc/kirkwood/kirkwood_pcm.c
> 

Some of the files above are missing from this patch series i.e.
kirkwood_audio_hal. Are they required ?

Liam

-- 
Freelance Developer, SlimLogic Ltd
ASoC and Voltage Regulator Maintainer.
http://www.slimlogic.co.uk

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

[apatard]

[-- Attachment #1: openrd_init_audio_dev.patch --]
[-- Type: text/plain, Size: 1356 bytes --]

This patch is reponsible for enabling audio on the openrd client board

Signed-off-by: Arnaud Patard <apatard@mandriva.com>
---
 arch/arm/mach-kirkwood/openrd-setup.c |   11 	11 +	0 -	0 !
 1 file changed, 11 insertions(+)

Index: linux-2.6.33/arch/arm/mach-kirkwood/openrd-setup.c
===================================================================
--- linux-2.6.33.orig/arch/arm/mach-kirkwood/openrd-setup.c	2010-05-11 17:53:34.753650304 +0200
+++ linux-2.6.33/arch/arm/mach-kirkwood/openrd-setup.c	2010-05-11 17:55:13.313650581 +0200
@@ -18,6 +18,7 @@
 #include <asm/mach-types.h>
 #include <asm/mach/arch.h>
 #include <mach/kirkwood.h>
+#include <mach/audio.h>
 #include <plat/mvsdio.h>
 #include "common.h"
 #include "mpp.h"
@@ -47,6 +48,11 @@ static struct mv643xx_eth_platform_data
 	.phy_addr	= MV643XX_ETH_PHY_ADDR(24),
 };
 
+struct kirkwood_soc_platform_data kirkwood_soc_data = {
+	.dram        = &kirkwood_mbus_dram_info,
+};
+EXPORT_SYMBOL_GPL(kirkwood_soc_data);
+
 static struct mv_sata_platform_data openrd_sata_data = {
 	.n_ports	= 2,
 };
@@ -80,6 +86,11 @@ static void __init openrd_init(void)
 	kirkwood_sdio_init(&openrd_mvsdio_data);
 
 	kirkwood_i2c_init();
+
+	if (machine_is_openrd_client()) {
+		kirkwood_soc_data.tclk = kirkwood_tclk,
+		kirkwood_audio_init(&kirkwood_soc_data);
+	}
 }
 
 static int __init openrd_pci_init(void)

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

[saeed bishara]

 <apatard <at> mandriva.com> writes:

> --- linux-2.6.33.orig/arch/arm/mach-kirkwood/openrd-setup.c	2010-05-11
17:53:34.753650304 +0200
> +++ linux-2.6.33/arch/arm/mach-kirkwood/openrd-setup.c	2010-05-11
17:55:13.313650581 +0200
> @@ -18,6 +18,7 @@
>  #include <asm/mach-types.h>
>  #include <asm/mach/arch.h>
>  #include <mach/kirkwood.h>
> +#include <mach/audio.h>
>  #include <plat/mvsdio.h>
>  #include "common.h"
>  #include "mpp.h"
> @@ -47,6 +48,11 @@ static struct mv643xx_eth_platform_data
>  	.phy_addr	= MV643XX_ETH_PHY_ADDR(24),
>  };
> 
> +struct kirkwood_soc_platform_data kirkwood_soc_data = {
> +	.dram        = &kirkwood_mbus_dram_info,
> +};
> +EXPORT_SYMBOL_GPL(kirkwood_soc_data);
> +
The kirkwood_soc_data is the same for all boards, so you can move it to the
mack-kirkwood/common.c, the tclk usually set at kirkwood_init().

saeed

[patch 3/6] asoc: Add SOC_DOUBLE_R_SX_TLV control

[apatard]

[-- Attachment #1: add_soc_double_r_sx_tlv.patch --]
[-- Type: text/plain, Size: 4975 bytes --]

This patch is adding a new control which has the following capabilities:
- tlv
- variable data size (for instance, 7 ou 8 bit)
- double mixer


Signed-off-by: Arnaud Patard <apatard@mandriva.com>
---
 include/sound/soc.h  |   22 	22 +	0 -	0 !
 sound/soc/soc-core.c |   95 	95 +	0 -	0 !
 2 files changed, 117 insertions(+)

Index: linux-2.6.33/include/sound/soc.h
===================================================================
--- linux-2.6.33.orig/include/sound/soc.h	2010-05-11 17:38:48.933650354 +0200
+++ linux-2.6.33/include/sound/soc.h	2010-05-11 17:57:31.369711347 +0200
@@ -168,6 +168,21 @@
 	.get = xhandler_get, .put = xhandler_put, \
 	.private_value = (unsigned long)&xenum }
 
+#define SOC_DOUBLE_R_SX_TLV(xname, xreg_left, xreg_right, xshift,\
+		xmin, xmax, tlv_array) \
+{	.iface = SNDRV_CTL_ELEM_IFACE_MIXER, .name = (xname), \
+	.access = SNDRV_CTL_ELEM_ACCESS_TLV_READ | \
+		  SNDRV_CTL_ELEM_ACCESS_READWRITE, \
+	.tlv.p = (tlv_array), \
+	.info = snd_soc_info_volsw_2r_sx, \
+	.get = snd_soc_get_volsw_2r_sx, \
+	.put = snd_soc_put_volsw_2r_sx, \
+	.private_value = (unsigned long)&(struct soc_mixer_control) \
+		{.reg = xreg_left, \
+		 .rreg = xreg_right, .shift = xshift, \
+		 .min = xmin, .max = xmax} }
+
+
 /*
  * Simplified versions of above macros, declaring a struct and calculating
  * ARRAY_SIZE internally
@@ -320,6 +335,13 @@ int snd_soc_get_volsw_s8(struct snd_kcon
 	struct snd_ctl_elem_value *ucontrol);
 int snd_soc_put_volsw_s8(struct snd_kcontrol *kcontrol,
 	struct snd_ctl_elem_value *ucontrol);
+int snd_soc_info_volsw_2r_sx(struct snd_kcontrol *kcontrol,
+	struct snd_ctl_elem_info *uinfo);
+int snd_soc_get_volsw_2r_sx(struct snd_kcontrol *kcontrol,
+	struct snd_ctl_elem_value *ucontrol);
+int snd_soc_put_volsw_2r_sx(struct snd_kcontrol *kcontrol,
+	struct snd_ctl_elem_value *ucontrol);
+
 
 /**
  * struct snd_soc_jack_pin - Describes a pin to update based on jack detection
Index: linux-2.6.33/sound/soc/soc-core.c
===================================================================
--- linux-2.6.33.orig/sound/soc/soc-core.c	2010-05-11 17:38:48.873649896 +0200
+++ linux-2.6.33/sound/soc/soc-core.c	2010-05-11 17:56:29.737650647 +0200
@@ -2190,6 +2190,101 @@ int snd_soc_put_volsw_s8(struct snd_kcon
 EXPORT_SYMBOL_GPL(snd_soc_put_volsw_s8);
 
 /**
+ * snd_soc_info_volsw_2r_sx - double with tlv and variable data size
+ *  mixer info callback
+ * @kcontrol: mixer control
+ * @uinfo: control element information
+ *
+ * Returns 0 for success.
+ */
+int snd_soc_info_volsw_2r_sx(struct snd_kcontrol *kcontrol,
+			struct snd_ctl_elem_info *uinfo)
+{
+	struct soc_mixer_control *mc =
+		(struct soc_mixer_control *)kcontrol->private_value;
+	int max = mc->max;
+	int min = mc->min;
+
+	uinfo->type = SNDRV_CTL_ELEM_TYPE_INTEGER;
+	uinfo->count = 2;
+	uinfo->value.integer.min = 0;
+	uinfo->value.integer.max = max-min;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(snd_soc_info_volsw_2r_sx);
+
+/**
+ * snd_soc_get_volsw_2r_sx - double with tlv and variable data size
+ *  mixer get callback
+ * @kcontrol: mixer control
+ * @uinfo: control element information
+ *
+ * Returns 0 for success.
+ */
+int snd_soc_get_volsw_2r_sx(struct snd_kcontrol *kcontrol,
+			struct snd_ctl_elem_value *ucontrol)
+{
+	struct soc_mixer_control *mc =
+		(struct soc_mixer_control *)kcontrol->private_value;
+	struct snd_soc_codec *codec = snd_kcontrol_chip(kcontrol);
+	unsigned int mask = (1<<mc->shift)-1;
+	int min = mc->min;
+	int val = snd_soc_read(codec, mc->reg) & mask;
+	int valr = snd_soc_read(codec, mc->rreg) & mask;
+
+	ucontrol->value.integer.value[0] = ((val & 0xff)-min);
+	ucontrol->value.integer.value[1] = ((valr & 0xff)-min);
+	return 0;
+}
+EXPORT_SYMBOL_GPL(snd_soc_get_volsw_2r_sx);
+
+/**
+ * snd_soc_put_volsw_2r_sx - double with tlv and variable data size
+ *  mixer put callback
+ * @kcontrol: mixer control
+ * @uinfo: control element information
+ *
+ * Returns 0 for success.
+ */
+int snd_soc_put_volsw_2r_sx(struct snd_kcontrol *kcontrol,
+			struct snd_ctl_elem_value *ucontrol)
+{
+	struct soc_mixer_control *mc =
+		(struct soc_mixer_control *)kcontrol->private_value;
+	struct snd_soc_codec *codec = snd_kcontrol_chip(kcontrol);
+	unsigned int mask = (1<<mc->shift)-1;
+	int min = mc->min;
+	int ret;
+	unsigned int val, valr, oval, ovalr;
+
+	val = ((ucontrol->value.integer.value[0]+min) & 0xff);
+	val &= mask;
+	valr = ((ucontrol->value.integer.value[1]+min) & 0xff);
+	valr &= mask;
+
+	oval = snd_soc_read(codec, mc->reg) & mask;
+	ovalr = snd_soc_read(codec, mc->rreg) & mask;
+
+	ret = 0;
+	if (oval != val) {
+		ret = snd_soc_write(codec, mc->reg, val);
+		if (ret < 0)
+			return 0;
+		ret = 1;
+	}
+	if (ovalr != valr) {
+		ret = snd_soc_write(codec, mc->rreg, valr);
+		if (ret < 0)
+			return 0;
+		ret = 1;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(snd_soc_put_volsw_2r_sx);
+
+/**
  * snd_soc_dai_set_sysclk - configure DAI system or master clock.
  * @dai: DAI
  * @clk_id: DAI specific clock ID

Re: [patch 3/6] asoc: Add SOC_DOUBLE_R_SX_TLV control

[Mark Brown]

On Tue, May 11, 2010 at 06:23:45PM +0200, apatard@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

The major new change here is that the control allows for ranges which
wrap around in the middle rather than continuously increasing or
decreasing.  The change itself looks OK but if you repost please make
the changelog clearer.

Oh, and ASoC, not asoc.

Re: [patch 3/6] asoc: Add SOC_DOUBLE_R_SX_TLV control

[Mark Brown]

On Tue, May 11, 2010 at 06:23:45PM +0200, apatard@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
> 
> 
> Signed-off-by: Arnaud Patard <apatard@mandriva.com>

So, after discussing with Liam on IRC I was going to rewrite the commit
message and apply this but it doesn't apply to current ASoC.  Please
make sure any patches for ASoC apply to:

  git://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound-2.6.git for-2.6.35

(or whatever the latest kernel version there's a branch for is.)

[patch 4/6] cs42l51: add asoc driver

[apatard]

[-- Attachment #1: cs42l51_support.patch --]
[-- Type: text/plain, Size: 25222 bytes --]

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>
---
 sound/soc/codecs/Kconfig   |    4 	4 +	0 -	0 !
 sound/soc/codecs/Makefile  |    2 	2 +	0 -	0 !
 sound/soc/codecs/cs42l51.c |  682 	682 +	0 -	0 !
 sound/soc/codecs/cs42l51.h |  167 	167 +	0 -	0 !
 4 files changed, 855 insertions(+)

Index: linux-2.6.33/sound/soc/codecs/cs42l51.c
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ linux-2.6.33/sound/soc/codecs/cs42l51.c	2010-05-11 18:03:07.717711495 +0200
@@ -0,0 +1,682 @@
+/*
+ * cs42l51.c
+ *
+ * ASoC Driver for Cirrus Logic CS42L51 codecs
+ *
+ * Copyright (c) 2010 Arnaud Patard <apatard@mandriva.com>
+ *
+ * Based on cs4270.c - Copyright (c) Timur Tabi <timur@freescale.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.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * For now:
+ *  - Only I2C is support. Not SPI
+ *  - master mode *NOT* supported
+ */
+
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <sound/core.h>
+#include <sound/soc.h>
+#include <sound/tlv.h>
+#include <sound/initval.h>
+#include <sound/pcm_params.h>
+#include <sound/pcm.h>
+#include <linux/i2c.h>
+
+#include "cs42l51.h"
+
+enum funct_mode {
+	MODE_SLAVE,
+	MODE_SLAVE_AUTO,
+	MODE_MASTER,
+};
+
+struct cs42l51_private {
+	unsigned int mclk;
+	unsigned int audio_mode;	/* The mode (I2S or left-justified) */
+	enum funct_mode func;		/* See funct_mode */
+	struct snd_soc_codec codec;
+	u16 reg_cache[CS42L51_NUMREGS];
+};
+
+static struct snd_soc_codec *cs42l51_codec;
+
+#define CS42L51_FORMATS ( \
+		SNDRV_PCM_FMTBIT_S16_LE  | SNDRV_PCM_FMTBIT_S16_BE  | \
+		SNDRV_PCM_FMTBIT_S18_3LE | SNDRV_PCM_FMTBIT_S18_3BE | \
+		SNDRV_PCM_FMTBIT_S20_3LE | SNDRV_PCM_FMTBIT_S20_3BE | \
+		SNDRV_PCM_FMTBIT_S24_LE  | SNDRV_PCM_FMTBIT_S24_BE)
+
+static int cs42l51_fill_cache(struct snd_soc_codec *codec)
+{
+	u8 *cache = codec->reg_cache;
+	struct i2c_client *i2c_client = codec->control_data;
+	s32 length;
+
+	length = i2c_smbus_read_i2c_block_data(i2c_client,
+			CS42L51_FIRSTREG | 0x80, CS42L51_NUMREGS, cache);
+	if (length != CS42L51_NUMREGS) {
+		dev_err(&i2c_client->dev,
+				"I2C read failure, addr=0x%x (ret=%d vs %d)\n",
+				i2c_client->addr, length, CS42L51_NUMREGS);
+		return -EIO;
+	}
+
+	return 0;
+}
+
+static unsigned int cs42l51_read_reg_cache(struct snd_soc_codec *codec,
+	unsigned int reg)
+{
+	u8 *cache = codec->reg_cache;
+
+	if ((reg < CS42L51_FIRSTREG) || (reg > CS42L51_LASTREG))
+		return -EIO;
+
+	return cache[reg - CS42L51_FIRSTREG];
+}
+
+static int cs42l51_i2c_write(struct snd_soc_codec *codec, unsigned int reg,
+	unsigned int value)
+{
+	u8 *cache = codec->reg_cache;
+	struct i2c_client *client = codec->control_data;
+
+	if ((reg < CS42L51_FIRSTREG) || (reg > CS42L51_LASTREG))
+		return -EIO;
+
+	/* Only perform an I2C operation if the new value is different */
+	if (cache[reg - CS42L51_FIRSTREG] != value) {
+		if (i2c_smbus_write_byte_data(client, reg, value)) {
+			dev_err(&client->dev, "I2C write failed\n");
+			return -EIO;
+		}
+		cache[reg - CS42L51_FIRSTREG] = value;
+	}
+
+	return 0;
+}
+
+
+static int cs42l51_i2c_probe(struct i2c_client *i2c_client,
+	const struct i2c_device_id *id)
+{
+	struct snd_soc_codec *codec;
+	struct cs42l51_private *cs42l51;
+	int ret = 0;
+	int reg;
+
+	if (cs42l51_codec)
+		return -EBUSY;
+
+	/* Verify that we have a CS42L51 */
+	ret = i2c_smbus_read_byte_data(i2c_client, CHIP_REV_ID);
+	if (ret < 0) {
+		dev_err(&i2c_client->dev, "failed to read I2C\n");
+		goto error;
+	}
+
+	if (ret != MK_CHIP_REV(CHIP_ID, CHIP_REV)) {
+		dev_err(&i2c_client->dev, "Invalid chip id\n");
+		ret = -ENODEV;
+		goto error;
+	}
+
+	dev_info(&i2c_client->dev, "found device at I2C address %X\n",
+				i2c_client->addr);
+
+	cs42l51 = kzalloc(sizeof(struct cs42l51_private), GFP_KERNEL);
+	if (!cs42l51) {
+		dev_err(&i2c_client->dev, "could not allocate codec\n");
+		return -ENOMEM;
+	}
+	codec = &cs42l51->codec;
+
+	mutex_init(&codec->mutex);
+	INIT_LIST_HEAD(&codec->dapm_widgets);
+	INIT_LIST_HEAD(&codec->dapm_paths);
+
+	codec->dev = &i2c_client->dev;
+	codec->name = "CS42L51";
+	codec->owner = THIS_MODULE;
+	codec->dai = &cs42l51_dai;
+	codec->num_dai = 1;
+	codec->private_data = cs42l51;
+
+	codec->control_data = i2c_client;
+	codec->read = cs42l51_read_reg_cache;
+	codec->write = cs42l51_i2c_write;
+	codec->reg_cache = cs42l51->reg_cache;
+	codec->reg_cache_size = CS42L51_NUMREGS;
+
+	ret = cs42l51_fill_cache(codec);
+	if (ret < 0) {
+		dev_err(&i2c_client->dev, "failed to fill register cache\n");
+		goto error;
+	}
+
+	i2c_set_clientdata(i2c_client, codec);
+
+	/*
+	 * DAC configuration (right place to do that ?)
+	 * - Use signal processor
+	 * - auto mute
+	 * - vol changes immediate
+	 * - no de-emphasize
+	 */
+	reg = DAC_CTL_DATA_SEL(1) | DAC_CTL_AMUTE | DAC_CTL_DACSZ(0);
+	ret = cs42l51_i2c_write(codec, DAC_CTL, reg);
+	if (ret < 0)
+		goto error;
+
+	/* Unmute PCM-A & PCM-B and set default vol to */
+	ret = cs42l51_i2c_write(codec, PCMA_VOL, 0x60);
+	if (ret < 0)
+		goto error;
+
+	ret = cs42l51_i2c_write(codec, PCMB_VOL, 0x60);
+	if (ret < 0)
+		goto error;
+
+	/* default for AOUTx */
+	ret = cs42l51_i2c_write(codec, AOUTA_VOL, 0x05);
+	if (ret < 0)
+		goto error;
+
+	ret = cs42l51_i2c_write(codec, AOUTB_VOL, 0x05);
+	if (ret < 0)
+		goto error;
+
+	/* route microphone */
+	ret = cs42l51_i2c_write(codec, ADC_INPUT, 0xF0);
+	if (ret < 0)
+		goto error;
+
+	cs42l51_dai.dev = codec->dev;
+	cs42l51_codec = codec;
+
+	ret = snd_soc_register_codec(codec);
+	if (ret != 0) {
+		dev_err(codec->dev, "Failed to register codec: %d\n", ret);
+		goto error;
+	}
+
+	ret = snd_soc_register_dai(&cs42l51_dai);
+	if (ret < 0) {
+		dev_err(&i2c_client->dev, "failed to register DAIe\n");
+		goto error_reg;
+	}
+
+	return 0;
+
+error_reg:
+	snd_soc_unregister_codec(codec);
+error:
+	return ret;
+}
+
+static int cs42l51_i2c_remove(struct i2c_client *client)
+{
+	struct cs42l51_private *cs42l51 = i2c_get_clientdata(client);
+	snd_soc_unregister_dai(&cs42l51_dai);
+	snd_soc_unregister_codec(&cs42l51->codec);
+	cs42l51_codec = NULL;
+	kfree(cs42l51);
+	return 0;
+}
+
+
+static const struct i2c_device_id cs42l51_id[] = {
+	{"cs42l51", 0},
+	{}
+};
+MODULE_DEVICE_TABLE(i2c, cs42l51_id);
+
+static struct i2c_driver cs42l51_i2c_driver = {
+	.driver = {
+		.name = "CS42L51 I2C",
+		.owner = THIS_MODULE,
+	},
+	.id_table = cs42l51_id,
+	.probe = cs42l51_i2c_probe,
+	.remove = cs42l51_i2c_remove,
+};
+
+static int cs42l51_get_chan_mix(struct snd_kcontrol *kcontrol,
+			struct snd_ctl_elem_value *ucontrol)
+{
+	struct snd_soc_codec *codec = snd_kcontrol_chip(kcontrol);
+	unsigned long value = snd_soc_read(codec, PCM_MIXER)&3;
+
+	switch (value) {
+	default:
+	case 0:
+		ucontrol->value.integer.value[0] = 0;
+		break;
+	case 1:
+	case 2:
+		ucontrol->value.integer.value[0] = 1;
+		break;
+	case 3:
+		ucontrol->value.integer.value[0] = 2;
+		break;
+	}
+
+	return 0;
+}
+
+#define CHAN_MIX_NORMAL	0x00
+#define CHAN_MIX_BOTH	0x55
+#define CHAN_MIX_SWAP	0xFF
+
+static int cs42l51_set_chan_mix(struct snd_kcontrol *kcontrol,
+			struct snd_ctl_elem_value *ucontrol)
+{
+	struct snd_soc_codec *codec = snd_kcontrol_chip(kcontrol);
+	unsigned char val;
+
+	switch (ucontrol->value.integer.value[0]) {
+	default:
+	case 0:
+		val = CHAN_MIX_NORMAL;
+		break;
+	case 1:
+		val = CHAN_MIX_BOTH;
+		break;
+	case 2:
+		val = CHAN_MIX_SWAP;
+		break;
+	}
+
+	snd_soc_write(codec, PCM_MIXER, val);
+
+	return 1;
+}
+
+static const DECLARE_TLV_DB_SCALE(adc_pcm_tlv, -5150, 50, 0);
+static const DECLARE_TLV_DB_SCALE(tone_tlv, -1050, 150, 0);
+/* This is a lie. after -102 db, it stays at -102 */
+/* maybe a range would be better */
+static const DECLARE_TLV_DB_SCALE(aout_tlv, -11550, 50, 0);
+
+static const char *mic_boost[] = { "+16dB", "+32dB"};
+
+static const char *chan_mix[] = {
+	"L R",
+	"L+R",
+	"R L",
+};
+
+static const struct soc_enum cs42l51_mix[] = {
+	SOC_ENUM_SINGLE_EXT(ARRAY_SIZE(chan_mix), chan_mix),
+	SOC_ENUM_DOUBLE(MIC_CTL, 0, 1, 2, mic_boost),
+};
+
+static const struct snd_kcontrol_new cs42l51_snd_controls[] = {
+	SOC_DOUBLE_R_SX_TLV("PCM Playback Volume",
+			PCMA_VOL, PCMB_VOL, 7, 0xffffff99, 0x18, adc_pcm_tlv),
+	SOC_DOUBLE_R("PCM Playback Switch", PCMA_VOL, PCMB_VOL, 7, 1, 1),
+	SOC_DOUBLE_R_SX_TLV("Analog Playback Volume",
+			AOUTA_VOL, AOUTB_VOL, 8, 0xffffff19, 0x18, aout_tlv),
+	SOC_DOUBLE_R_SX_TLV("ADC Mixer Volume",
+			ADCA_VOL, ADCB_VOL, 7, 0xffffff99, 0x18, adc_pcm_tlv),
+	SOC_DOUBLE_R("ADC Mixer Switch", PCMA_VOL, PCMB_VOL, 7, 1, 1),
+	SOC_SINGLE("Playback Deemphasis", DAC_CTL, 3, 1, 0),
+	SOC_SINGLE("Auto-Mute Switch", DAC_CTL, 2, 1, 0),
+	SOC_SINGLE("Soft Ramp Switch", DAC_CTL, 1, 1, 0),
+	SOC_SINGLE("Zero Cross Switch", DAC_CTL, 0, 0, 0),
+	SOC_SINGLE("Mic Bias", MIC_POWER_CTL, 1, 1, 1),
+	SOC_DOUBLE("Mic Powerdown", MIC_POWER_CTL, 2, 3, 1, 0),
+	SOC_ENUM("Mic Boost", cs42l51_mix[1]),
+	SOC_SINGLE_TLV("Bass Volume", TONE_CTL, 0, 0xf, 1, tone_tlv),
+	SOC_SINGLE_TLV("Treble Volume", TONE_CTL, 4, 0xf, 1, tone_tlv),
+	SOC_ENUM_EXT("PCM channel mixer",
+			cs42l51_mix[0],
+			cs42l51_get_chan_mix, cs42l51_set_chan_mix),
+};
+
+static int cs42l51_set_dai_fmt(struct snd_soc_dai *codec_dai,
+		unsigned int format)
+{
+	struct snd_soc_codec *codec = codec_dai->codec;
+	struct cs42l51_private *cs42l51 = codec->private_data;
+	int ret = 0;
+
+	switch (format & SND_SOC_DAIFMT_FORMAT_MASK) {
+	case SND_SOC_DAIFMT_I2S:
+	case SND_SOC_DAIFMT_LEFT_J:
+	case SND_SOC_DAIFMT_RIGHT_J:
+		cs42l51->audio_mode = format & SND_SOC_DAIFMT_FORMAT_MASK;
+		break;
+	default:
+		printk(KERN_ERR "cs4270: invalid DAI format\n");
+		ret = -EINVAL;
+	}
+
+	switch (SND_SOC_DAIFMT_MASTER_MASK) {
+	default:
+	case SND_SOC_DAIFMT_CBM_CFM:
+		cs42l51->func = MODE_SLAVE_AUTO;
+		break;
+	case SND_SOC_DAIFMT_CBS_CFS:
+		cs42l51->func = MODE_MASTER;
+		break;
+	}
+
+	return ret;
+}
+
+struct cs42l51_ratios {
+	unsigned int ratio;
+	unsigned char speed_mode;
+	unsigned char mclk;
+};
+
+static struct cs42l51_ratios slave_ratios[] = {
+	{  512, QSM_MODE, 0 }, {  768, QSM_MODE, 0 }, { 1024, QSM_MODE, 0 },
+	{ 1536, QSM_MODE, 0 }, { 2048, QSM_MODE, 0 }, { 3072, QSM_MODE, 0 },
+	{  256, HSM_MODE, 0 }, {  384, HSM_MODE, 0 }, {  512, HSM_MODE, 0 },
+	{  768, HSM_MODE, 0 }, { 1024, HSM_MODE, 0 }, { 1536, HSM_MODE, 0 },
+	{  128, SSM_MODE, 0 }, {  192, SSM_MODE, 0 }, {  256, SSM_MODE, 0 },
+	{  384, SSM_MODE, 0 }, {  512, SSM_MODE, 0 }, {  768, SSM_MODE, 0 },
+	{  128, DSM_MODE, 0 }, {  192, DSM_MODE, 0 }, {  256, DSM_MODE, 0 },
+	{  384, DSM_MODE, 0 },
+};
+
+static struct cs42l51_ratios slave_auto_ratios[] = {
+	{ 1024, QSM_MODE, 0 }, { 1536, QSM_MODE, 0 }, { 2048, QSM_MODE, 1 },
+	{ 3072, QSM_MODE, 1 },
+	{  512, HSM_MODE, 0 }, {  768, HSM_MODE, 0 }, { 1024, HSM_MODE, 1 },
+	{ 1536, HSM_MODE, 1 },
+	{  256, SSM_MODE, 0 }, {  384, SSM_MODE, 0 }, {  512, SSM_MODE, 1 },
+	{  768, SSM_MODE, 1 },
+	{  128, DSM_MODE, 0 }, {  192, DSM_MODE, 0 }, {  256, DSM_MODE, 1 },
+	{  384, DSM_MODE, 1 },
+};
+
+/* Fill me */
+static struct cs42l51_ratios master_ratios[] = { {},
+};
+
+static int cs42l51_set_dai_sysclk(struct snd_soc_dai *codec_dai,
+		int clk_id, unsigned int freq, int dir)
+{
+	struct snd_soc_codec *codec = codec_dai->codec;
+	struct cs42l51_private *cs42l51 = codec->private_data;
+	struct cs42l51_ratios *ratios = NULL;
+	int nr_ratios = 0;
+	unsigned int rates = 0;
+	unsigned int rate_min = -1;
+	unsigned int rate_max = 0;
+	int i;
+
+	cs42l51->mclk = freq;
+
+	switch (cs42l51->func) {
+	case MODE_MASTER:
+		ratios = master_ratios;
+		nr_ratios = ARRAY_SIZE(master_ratios);
+		break;
+	case MODE_SLAVE:
+		ratios = slave_ratios;
+		nr_ratios = ARRAY_SIZE(slave_ratios);
+		break;
+	case MODE_SLAVE_AUTO:
+		ratios = slave_auto_ratios;
+		nr_ratios = ARRAY_SIZE(slave_auto_ratios);
+		break;
+	}
+
+	for (i = 0; i < nr_ratios; i++) {
+		unsigned int rate = freq / ratios[i].ratio;
+		rates |= snd_pcm_rate_to_rate_bit(rate);
+		if (rate < rate_min)
+			rate_min = rate;
+		if (rate > rate_max)
+			rate_max = rate;
+	}
+	rates &= ~SNDRV_PCM_RATE_KNOT;
+
+	if (!rates) {
+		printk(KERN_ERR "cs42l51: could not find a valid sample rate\n");
+		return -EINVAL;
+	}
+
+	codec_dai->playback.rates = rates;
+	codec_dai->playback.rate_min = rate_min;
+	codec_dai->playback.rate_max = rate_max;
+
+	codec_dai->capture.rates = rates;
+	codec_dai->capture.rate_min = rate_min;
+	codec_dai->capture.rate_max = rate_max;
+
+	return 0;
+}
+
+static int cs42l51_hw_params(struct snd_pcm_substream *substream,
+		struct snd_pcm_hw_params *params,
+		struct snd_soc_dai *dai)
+{
+	struct snd_soc_pcm_runtime *rtd = substream->private_data;
+	struct snd_soc_device *socdev = rtd->socdev;
+	struct snd_soc_codec *codec = socdev->card->codec;
+	struct cs42l51_private *cs42l51 = codec->private_data;
+	int ret;
+	unsigned int i;
+	unsigned int rate;
+	unsigned int ratio;
+	struct cs42l51_ratios *ratios = NULL;
+	int nr_ratios = 0;
+	int intf_ctl, power_ctl;
+
+	switch (cs42l51->func) {
+	case MODE_MASTER:
+		ratios = master_ratios;
+		nr_ratios = ARRAY_SIZE(master_ratios);
+		break;
+	case MODE_SLAVE:
+		ratios = slave_ratios;
+		nr_ratios = ARRAY_SIZE(slave_ratios);
+		break;
+	case MODE_SLAVE_AUTO:
+		ratios = slave_auto_ratios;
+		nr_ratios = ARRAY_SIZE(slave_auto_ratios);
+		break;
+	}
+
+	/* Figure out which MCLK/LRCK ratio to use */
+	rate = params_rate(params);     /* Sampling rate, in Hz */
+	ratio = cs42l51->mclk / rate;    /* MCLK/LRCK ratio */
+	for (i = 0; i < nr_ratios; i++) {
+		if (ratios[i].ratio == ratio)
+			break;
+	}
+
+	if (i == nr_ratios) {
+		/* We did not find a matching ratio */
+		printk(KERN_ERR "cs42l51: could not find matching ratio\n");
+		return -EINVAL;
+	}
+
+	intf_ctl = snd_soc_read(codec, INTF_CTL);
+	power_ctl = snd_soc_read(codec, MIC_POWER_CTL);
+
+	intf_ctl &= ~(INTF_CTL_MASTER|INTF_CTL_ADC_I2S|INTF_CTL_DAC_FORMAT(7));
+	power_ctl &= ~(MIC_POWER_CTL_SPEED(3)|MIC_POWER_CTL_MCLK_DIV2);
+
+	switch (cs42l51->func) {
+	case MODE_MASTER:
+		intf_ctl |= INTF_CTL_MASTER;
+		power_ctl |= MIC_POWER_CTL_SPEED(ratios[i].speed_mode);
+		break;
+	case MODE_SLAVE:
+		power_ctl |= MIC_POWER_CTL_SPEED(ratios[i].speed_mode);
+		break;
+	case MODE_SLAVE_AUTO:
+		power_ctl |= MIC_POWER_CTL_AUTO;
+		break;
+	}
+
+	switch (cs42l51->audio_mode) {
+	case SND_SOC_DAIFMT_I2S:
+		intf_ctl |= INTF_CTL_ADC_I2S | INTF_CTL_DAC_FORMAT(DAC_DIF_I2S);
+		break;
+	case SND_SOC_DAIFMT_LEFT_J:
+		intf_ctl |= INTF_CTL_DAC_FORMAT(DAC_DIF_LJ24);
+		break;
+	case SND_SOC_DAIFMT_RIGHT_J:
+		switch (params_format(params)) {
+		case SNDRV_PCM_FORMAT_S16_LE:
+		case SNDRV_PCM_FORMAT_S16_BE:
+			intf_ctl |= INTF_CTL_DAC_FORMAT(DAC_DIF_RJ16);
+			break;
+		case SNDRV_PCM_FORMAT_S18_3LE:
+		case SNDRV_PCM_FORMAT_S18_3BE:
+			intf_ctl |= INTF_CTL_DAC_FORMAT(DAC_DIF_RJ18);
+			break;
+		case SNDRV_PCM_FORMAT_S20_3LE:
+		case SNDRV_PCM_FORMAT_S20_3BE:
+			intf_ctl |= INTF_CTL_DAC_FORMAT(DAC_DIF_RJ20);
+			break;
+		case SNDRV_PCM_FORMAT_S24_LE:
+		case SNDRV_PCM_FORMAT_S24_BE:
+			intf_ctl |= INTF_CTL_DAC_FORMAT(DAC_DIF_RJ24);
+			break;
+		default:
+			printk(KERN_ERR "cs42l51: unknown format\n");
+			return -EINVAL;
+		}
+		break;
+	default:
+		printk(KERN_ERR "cs42l51: unknown format\n");
+		return -EINVAL;
+	}
+
+	if (ratios[i].mclk)
+		power_ctl |= MIC_POWER_CTL_MCLK_DIV2;
+
+	ret = snd_soc_write(codec, INTF_CTL, intf_ctl);
+	if (ret < 0)
+		return ret;
+
+	ret = snd_soc_write(codec, MIC_POWER_CTL, power_ctl);
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+
+static struct snd_soc_dai_ops cs42l51_dai_ops = {
+	.hw_params      = cs42l51_hw_params,
+	.set_sysclk     = cs42l51_set_dai_sysclk,
+	.set_fmt        = cs42l51_set_dai_fmt,
+};
+
+struct snd_soc_dai cs42l51_dai = {
+	.name = "CS42L51 HiFi",
+	.playback = {
+		.stream_name = "Playback",
+		.channels_min = 1,
+		.channels_max = 2,
+		.rates = SNDRV_PCM_RATE_8000_96000,
+		.formats = CS42L51_FORMATS,
+	},
+	.capture = {
+		.stream_name = "Capture",
+		.channels_min = 1,
+		.channels_max = 2,
+		.rates = SNDRV_PCM_RATE_8000_96000,
+		.formats = CS42L51_FORMATS,
+	},
+	.ops = &cs42l51_dai_ops,
+};
+EXPORT_SYMBOL_GPL(cs42l51_dai);
+
+
+static int cs42l51_probe(struct platform_device *pdev)
+{
+	struct snd_soc_device *socdev = platform_get_drvdata(pdev);
+	struct snd_soc_codec *codec;
+	int ret = 0;
+	int i;
+
+	if (!cs42l51_codec) {
+		dev_err(&pdev->dev, "CS42L51 codec not yet registered\n");
+		return -EINVAL;
+	}
+
+	dev_info(&pdev->dev, "CS42L51 ALSA SoC Codec\n");
+
+	socdev->card->codec = cs42l51_codec;
+	codec = socdev->card->codec;
+
+	/* Register PCMs */
+	ret = snd_soc_new_pcms(socdev, SNDRV_DEFAULT_IDX1, SNDRV_DEFAULT_STR1);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "failed to create PCMs\n");
+		return ret;
+	}
+
+	for (i = 0; i < ARRAY_SIZE(cs42l51_snd_controls); i++) {
+		ret = snd_ctl_add(codec->card,
+				snd_soc_cnew(&cs42l51_snd_controls[i],
+				codec, NULL));
+		if (ret < 0) {
+			dev_err(&pdev->dev, "failed to add controls\n");
+			snd_soc_free_pcms(socdev);
+			return ret;
+		}
+	}
+
+	return 0;
+}
+
+
+static int cs42l51_remove(struct platform_device *pdev)
+{
+	struct snd_soc_device *socdev = platform_get_drvdata(pdev);
+
+	snd_soc_free_pcms(socdev);
+
+	return 0;
+}
+
+struct snd_soc_codec_device soc_codec_device_cs42l51 = {
+	.probe =	cs42l51_probe,
+	.remove =	cs42l51_remove
+};
+EXPORT_SYMBOL_GPL(soc_codec_device_cs42l51);
+
+static int __init cs42l51_init(void)
+{
+	int ret;
+
+	ret = i2c_add_driver(&cs42l51_i2c_driver);
+	if (ret != 0) {
+		printk(KERN_ERR "can't add i2c driver\n");
+		return ret;
+	}
+	return 0;
+}
+module_init(cs42l51_init);
+
+static void __exit cs42l51_exit(void)
+{
+	i2c_del_driver(&cs42l51_i2c_driver);
+}
+module_exit(cs42l51_exit);
+
+MODULE_AUTHOR("Arnaud Patard <apatard@mandriva.com>");
+MODULE_DESCRIPTION("Cirrus Logic CS42L51 ALSA SoC Codec Driver");
+MODULE_LICENSE("GPL");
+
Index: linux-2.6.33/sound/soc/codecs/cs42l51.h
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ linux-2.6.33/sound/soc/codecs/cs42l51.h	2010-05-11 17:59:30.745650160 +0200
@@ -0,0 +1,167 @@
+/*
+ * cs42l51.h
+ *
+ * ASoC Driver for Cirrus Logic CS42L51 codecs
+ *
+ * Copyright (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.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+#ifndef _CS42L51_H
+#define CS42L51_H
+
+#define CHIP_ID			0x1B
+#define CHIP_REV		0x01
+
+#define CHIP_REV_ID		0x01
+#define MK_CHIP_REV(a, b)	((a)<<3|(b))
+
+#define POWER_CTL1		0x02
+#define POWER_CTL1_PDN_DACB	(1<<6)
+#define POWER_CTL1_PDN_DACA	(1<<5)
+#define POWER_CTL1_PDN_PGAB	(1<<4)
+#define POWER_CTL1_PDN_PGAA	(1<<3)
+#define POWER_CTL1_PDN_ADCB	(1<<2)
+#define POWER_CTL1_PDN_ADCA	(1<<1)
+#define POWER_CTL1_PDN		(1<<0)
+
+#define MIC_POWER_CTL		0x03
+#define MIC_POWER_CTL_AUTO	(1<<7)
+#define MIC_POWER_CTL_SPEED(x)	(((x)&3)<<5)
+#define QSM_MODE		3
+#define HSM_MODE		2
+#define	SSM_MODE		1
+#define DSM_MODE		0
+#define MIC_POWER_CTL_3ST_SP	(1<<4)
+#define MIC_POWER_CTL_PDN_MICB	(1<<3)
+#define MIC_POWER_CTL_PDN_MICA	(1<<2)
+#define MIC_POWER_CTL_PDN_BIAS	(1<<1)
+#define MIC_POWER_CTL_MCLK_DIV2	(1<<0)
+
+#define INTF_CTL		0x04
+#define INTF_CTL_LOOPBACK	(1<<7)
+#define INTF_CTL_MASTER		(1<<6)
+#define INTF_CTL_DAC_FORMAT(x)	(((x)&7)<<3)
+#define DAC_DIF_LJ24		0x00
+#define DAC_DIF_I2S		0x01
+#define DAC_DIF_RJ24		0x02
+#define DAC_DIF_RJ20		0x03
+#define DAC_DIF_RJ18		0x04
+#define DAC_DIF_RJ16		0x05
+#define INTF_CTL_ADC_I2S	(1<<2)
+#define INTF_CTL_DIGMIX		(1<<1)
+#define INTF_CTL_MICMIX		(1<<0)
+
+#define MIC_CTL			0x05
+#define MIC_CTL_ADC_SNGVOL	(1<<7)
+#define MIC_CTL_ADCD_DBOOST	(1<<6)
+#define MIC_CTL_ADCA_DBOOST	(1<<5)
+#define MIC_CTL_MICBIAS_SEL	(1<<4)
+#define MIC_CTL_MICBIAS_LVL(x)	(((x)&3)<<2)
+#define MIC_CTL_MICB_BOOST	(1<<1)
+#define MIC_CTL_MICA_BOOST	(1<<0)
+
+#define ADC_CTL			0x06
+#define ADC_CTL_ADCB_HPFEN	(1<<7)
+#define ADC_CTL_ADCB_HPFRZ	(1<<6)
+#define ADC_CTL_ADCA_HPFEN	(1<<5)
+#define ADC_CTL_ADCA_HPFRZ	(1<<4)
+#define ADC_CTL_SOFTB		(1<<3)
+#define ADC_CTL_ZCROSSB		(1<<2)
+#define ADC_CTL_SOFTA		(1<<1)
+#define ADC_CTL_ZCROSSA		(1<<0)
+
+#define ADC_INPUT		0x07
+#define ADC_INPUT_AINB_MUX(x)	(((x)&3)<<6)
+#define ADC_INPUT_AINA_MUX(x)	(((x)&3)<<4)
+#define ADC_INPUT_INV_ADCB	(1<<3)
+#define ADC_INPUT_INV_ADCA	(1<<2)
+#define ADC_INPUT_ADCB_MUTE	(1<<1)
+#define ADC_INPUT_ADCA_MUTE	(1<<0)
+
+#define DAC_OUT_CTL		0x08
+#define DAC_OUT_CTL_HP_GAIN(x)	(((x)&7)<<5)
+#define DAC_OUT_CTL_DAC_SNGVOL	(1<<4)
+#define DAC_OUT_CTL_INV_PCMB	(1<<3)
+#define DAC_OUT_CTL_INV_PCMA	(1<<2)
+#define DAC_OUT_CTL_DACB_MUTE	(1<<1)
+#define DAC_OUT_CTL_DACA_MUTE	(1<<0)
+
+#define DAC_CTL			0x09
+#define DAC_CTL_DATA_SEL(x)	(((x)&3)<<6)
+#define DAC_CTL_FREEZE		(1<<5)
+#define DAC_CTL_DEEMPH		(1<<3)
+#define DAC_CTL_AMUTE		(1<<2)
+#define DAC_CTL_DACSZ(x)	(((x)&3)<<0)
+
+#define ALC_PGA_CTL		0x0A
+#define ALC_PGB_CTL		0x0B
+#define ALC_PGX_ALCX_SRDIS	(1<<7)
+#define ALC_PGX_ALCX_ZCDIS	(1<<6)
+#define ALC_PGX_PGX_VOL(x)	(((x)&0x1f)<<0)
+
+#define ADCA_ATT		0x0C
+#define ADCB_ATT		0x0D
+
+#define ADCA_VOL		0x0E
+#define ADCB_VOL		0x0F
+#define PCMA_VOL		0x10
+#define PCMB_VOL		0x11
+#define MIX_MUTE_ADCMIX		(1<<7)
+#define MIX_VOLUME(x)		(((x)&0x7f)<<0)
+
+#define BEEP_FREQ		0x12
+#define BEEP_VOL		0x13
+#define BEEP_CONF		0x14
+
+#define TONE_CTL		0x15
+#define TONE_CTL_TREB(x)	(((x)&0xf)<<4)
+#define TONE_CTL_BASS(x)	(((x)&0xf)<<0)
+
+#define AOUTA_VOL		0x16
+#define AOUTB_VOL		0x17
+#define PCM_MIXER		0x18
+#define LIMIT_THRES_DIS		0x19
+#define LIMIT_REL		0x1A
+#define LIMIT_ATT		0x1B
+#define ALC_EN			0x1C
+#define ALC_REL			0x1D
+#define ALC_THRES		0x1E
+#define NOISE_CONF		0x1F
+
+#define STATUS			0x20
+#define STATUS_SP_CLKERR	(1<<6)
+#define STATUS_SPEA_OVFL	(1<<5)
+#define STATUS_SPEB_OVFL	(1<<4)
+#define STATUS_PCMA_OVFL	(1<<3)
+#define STATUS_PCMB_OVFL	(1<<2)
+#define STATUS_ADCA_OVFL	(1<<1)
+#define STATUS_ADCB_OVFL	(1<<0)
+
+#define CHARGE_FREQ		0x21
+
+#define CS42L51_FIRSTREG	0x01
+/*
+ * Hack: with register 0x21, it makes 33 registers. Looks like someone in the
+ * i2c layer doesn't like i2c smbus block read of 33 regs. Workaround by using
+ * 32 regs
+ */
+#define CS42L51_LASTREG		0x20
+#define CS42L51_NUMREGS		(CS42L51_LASTREG - CS42L51_FIRSTREG + 1)
+
+struct cs42l51_setup_data {
+	int i2c_bus;
+	unsigned short i2c_address;
+};
+
+extern struct snd_soc_dai cs42l51_dai;
+extern struct snd_soc_codec_device soc_codec_device_cs42l51;
+#endif
Index: linux-2.6.33/sound/soc/codecs/Kconfig
===================================================================
--- linux-2.6.33.orig/sound/soc/codecs/Kconfig	2010-05-11 17:38:48.729650013 +0200
+++ linux-2.6.33/sound/soc/codecs/Kconfig	2010-05-11 17:59:30.745650160 +0200
@@ -21,6 +21,7 @@ config SND_SOC_ALL_CODECS
 	select SND_SOC_AK4535 if I2C
 	select SND_SOC_AK4642 if I2C
 	select SND_SOC_AK4671 if I2C
+	select SND_SOC_CS42L51 if I2C
 	select SND_SOC_CS4270 if I2C
 	select SND_SOC_MAX9877 if I2C
 	select SND_SOC_DA7210 if I2C
@@ -114,6 +115,9 @@ config SND_SOC_AK4642
 config SND_SOC_AK4671
 	tristate
 
+config SND_SOC_CS42L51
+	tristate
+
 # Cirrus Logic CS4270 Codec
 config SND_SOC_CS4270
 	tristate
Index: linux-2.6.33/sound/soc/codecs/Makefile
===================================================================
--- linux-2.6.33.orig/sound/soc/codecs/Makefile	2010-05-11 17:38:48.753650032 +0200
+++ linux-2.6.33/sound/soc/codecs/Makefile	2010-05-11 17:59:30.745650160 +0200
@@ -8,6 +8,7 @@ snd-soc-ak4104-objs := ak4104.o
 snd-soc-ak4535-objs := ak4535.o
 snd-soc-ak4642-objs := ak4642.o
 snd-soc-ak4671-objs := ak4671.o
+snd-soc-cs42l51-objs := cs42l51.o
 snd-soc-cs4270-objs := cs4270.o
 snd-soc-cx20442-objs := cx20442.o
 snd-soc-da7210-objs := da7210.o
@@ -70,6 +71,7 @@ obj-$(CONFIG_SND_SOC_AK4104)	+= snd-soc-
 obj-$(CONFIG_SND_SOC_AK4535)	+= snd-soc-ak4535.o
 obj-$(CONFIG_SND_SOC_AK4642)	+= snd-soc-ak4642.o
 obj-$(CONFIG_SND_SOC_AK4671)	+= snd-soc-ak4671.o
+obj-$(CONFIG_SND_SOC_CS42L51)	+= snd-soc-cs42l51.o
 obj-$(CONFIG_SND_SOC_CS4270)	+= snd-soc-cs4270.o
 obj-$(CONFIG_SND_SOC_CX20442)	+= snd-soc-cx20442.o
 obj-$(CONFIG_SND_SOC_DA7210)	+= snd-soc-da7210.o

Re: [patch 4/6] cs42l51: add asoc driver

[Timur Tabi]

On Tue, May 11, 2010 at 11:23 AM,  <apatard@mandriva.com> wrote:

> + * Copyright (c) 2010 Arnaud Patard <apatard@mandriva.com>
> + *
> + * Based on cs4270.c - Copyright (c) Timur Tabi <timur@freescale.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.

The cs4270.c driver is licensed under the GPL v2 only.  Since you
based your driver on mine, you cannot change the license to "v2 or
later".

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [patch 4/6] cs42l51: add asoc driver

[Mark Brown]

On Tue, May 11, 2010 at 06:23:46PM +0200, apatard@mandriva.com wrote:

> This patch is adding a ASoC driver for the cs42l51 from Cirrus Logic.
> Master mode and spi mode are not supported.

This is mostly OK - there's quite a few comments below but there's no
massive structural things in here, it's generally small, local things.

> + * Based on cs4270.c - Copyright (c) Timur Tabi <timur@freescale.com>

While we're picking up on the copyright stuff IIRC it's actually
Freescale copyright rather than Timur's personal copyright :)

> +enum funct_mode {
> +	MODE_SLAVE,
> +	MODE_SLAVE_AUTO,
> +	MODE_MASTER,
> +};

I'd prefer a more meaningful name than "funct_mode" here - I don't
really know what a funct is.

> +static unsigned int cs42l51_read_reg_cache(struct snd_soc_codec *codec,
> +	unsigned int reg)
> +{
> +	u8 *cache = codec->reg_cache;
> +
> +	if ((reg < CS42L51_FIRSTREG) || (reg > CS42L51_LASTREG))
> +		return -EIO;
> +
> +	return cache[reg - CS42L51_FIRSTREG];

Please just use the standard register cache access stuff in soc-cache.c.
The first register is 1 so you're only burning a single byte of RAM by
using the generic code.

> +	if (ret != MK_CHIP_REV(CHIP_ID, CHIP_REV)) {

Are you sure there's only one device revision in production?

> +		dev_err(&i2c_client->dev, "Invalid chip id\n");
> +		ret = -ENODEV;
> +		goto error;
> +	}

> +	dev_info(&i2c_client->dev, "found device at I2C address %X\n",
> +				i2c_client->addr);

The dev_() will already be including the I2C address in the log message
anyway.  I'd be inclined to just drop this, or make it log the device
revision.

> +	ret = cs42l51_fill_cache(codec);
> +	if (ret < 0) {
> +		dev_err(&i2c_client->dev, "failed to fill register cache\n");
> +		goto error;
> +	}

Normal practice is to reset the chip so it's in a known sensible state
when the driver starts trying to use it.  Is there a good reason not to
do this here?

> +	/*
> +	 * DAC configuration (right place to do that ?)

It's OK to do this for things that the user would unconditionally want.

> +	 * - Use signal processor

What does this mean? It sounds like something that would need to be
power managed via DAPM?

> +	 * - auto mute

This is probably OK, but exposing as a user visible control would be
better.

> +	 * - vol changes immediate

This is OK.

> +	 * - no de-emphasize

This is normally exposed as a user selectable switch.

> +	/* Unmute PCM-A & PCM-B and set default vol to */
> +	ret = cs42l51_i2c_write(codec, PCMA_VOL, 0x60);
> +	if (ret < 0)
> +		goto error;

This should not be done in the driver, leave it up to userspace.  This
driver will be used by all systems using this CODEC so just rely on the
hardware defaults to provide a sane power on configuration - the
designers will usually ensure that this is at least not harmful.

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

Ditto.

> +error_reg:
> +	snd_soc_unregister_codec(codec);
> +error:
> +	return ret;

You're missing some error handling here - there's no free of the device
data, for example.

> +static int cs42l51_get_chan_mix(struct snd_kcontrol *kcontrol,
> +			struct snd_ctl_elem_value *ucontrol)
> +{
> +	struct snd_soc_codec *codec = snd_kcontrol_chip(kcontrol);
> +	unsigned long value = snd_soc_read(codec, PCM_MIXER)&3;
> +
> +	switch (value) {
> +	default:
> +	case 0:
> +		ucontrol->value.integer.value[0] = 0;
> +		break;
> +	case 1:
> +	case 2:
> +		ucontrol->value.integer.value[0] = 1;
> +		break;
> +	case 3:
> +		ucontrol->value.integer.value[0] = 2;
> +		break;
> +	}
> +
> +	return 0;
> +}

Some comments here might be a little clearer...  Why is the difference
between 1 and 2 not interesting?

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

Could use TLV for this too.

> +static const struct soc_enum cs42l51_mix[] = {
> +	SOC_ENUM_SINGLE_EXT(ARRAY_SIZE(chan_mix), chan_mix),
> +	SOC_ENUM_DOUBLE(MIC_CTL, 0, 1, 2, mic_boost),
> +};

Don't do this, declare separate variables for each.  Indexing into a
table of mixer elements doesn't help leigibility and is asking for off
by one errors.  Some drivers do this for historical reasons but modern
drivers don't.

> +	SOC_DOUBLE_R("PCM Playback Switch", PCMA_VOL, PCMB_VOL, 7, 1, 1),

I suspect that this should be managed via the DAI mute function.

> +	SOC_SINGLE("Playback Deemphasis", DAC_CTL, 3, 1, 0),

Should have "Switch" at the end of the control name.

> +	SOC_SINGLE("Mic Bias", MIC_POWER_CTL, 1, 1, 1),

This should be a DAPM MICBIAS control.

> +	SOC_DOUBLE("Mic Powerdown", MIC_POWER_CTL, 2, 3, 1, 0),

What does this do?  Sounds like it should be DAPM controls.

> +	default:
> +		printk(KERN_ERR "cs4270: invalid DAI format\n");

Cut'n'paste.

> +/* Fill me */
> +static struct cs42l51_ratios master_ratios[] = { {},
> +};

Just remove this stuff for master mode if it's not implemented.

> +	for (i = 0; i < ARRAY_SIZE(cs42l51_snd_controls); i++) {
> +		ret = snd_ctl_add(codec->card,
> +				snd_soc_cnew(&cs42l51_snd_controls[i],
> +				codec, NULL));
> +		if (ret < 0) {
> +			dev_err(&pdev->dev, "failed to add controls\n");
> +			snd_soc_free_pcms(socdev);
> +			return ret;
> +		}
> +	}

snd_soc_add_controls() rather than open coding.

> +#define BEEP_FREQ		0x12
> +#define BEEP_VOL		0x13
> +#define BEEP_CONF		0x14

Please namespace everything in the header - things like this are asking
for namespace clashes.

Re: [patch 4/6] cs42l51: add asoc driver

[Arnaud Patard]

Mark Brown <broonie@opensource.wolfsonmicro.com> writes:

Hi,

> On Tue, May 11, 2010 at 06:23:46PM +0200, apatard@mandriva.com wrote:
>
>> This patch is adding a ASoC driver for the cs42l51 from Cirrus Logic.
>> Master mode and spi mode are not supported.
>
> This is mostly OK - there's quite a few comments below but there's no
> massive structural things in here, it's generally small, local things.
>
>> + * Based on cs4270.c - Copyright (c) Timur Tabi <timur@freescale.com>
>
> While we're picking up on the copyright stuff IIRC it's actually
> Freescale copyright rather than Timur's personal copyright :)
>
>> +enum funct_mode {
>> +	MODE_SLAVE,
>> +	MODE_SLAVE_AUTO,
>> +	MODE_MASTER,
>> +};
>
> I'd prefer a more meaningful name than "funct_mode" here - I don't
> really know what a funct is.
>
>> +static unsigned int cs42l51_read_reg_cache(struct snd_soc_codec *codec,
>> +	unsigned int reg)
>> +{
>> +	u8 *cache = codec->reg_cache;
>> +
>> +	if ((reg < CS42L51_FIRSTREG) || (reg > CS42L51_LASTREG))
>> +		return -EIO;
>> +
>> +	return cache[reg - CS42L51_FIRSTREG];
>
> Please just use the standard register cache access stuff in soc-cache.c.
> The first register is 1 so you're only burning a single byte of RAM by
> using the generic code.

I'm using i2c_smbus_read_i2c_block_data() and didn't notice it in the
soc-cache.c file. I didn't look further so it's possible that there's
actually something working for my case.

>
>> +	if (ret != MK_CHIP_REV(CHIP_ID, CHIP_REV)) {
>
> Are you sure there's only one device revision in production?

According to the specs there are 2 revs id, the A and B. As I've got a
rev B, I assumed that the rev A has not been on prod. I've changed the
check to handle both revs.

>
>> +		dev_err(&i2c_client->dev, "Invalid chip id\n");
>> +		ret = -ENODEV;
>> +		goto error;
>> +	}
>
>> +	dev_info(&i2c_client->dev, "found device at I2C address %X\n",
>> +				i2c_client->addr);
>
> The dev_() will already be including the I2C address in the log message
> anyway.  I'd be inclined to just drop this, or make it log the device
> revision.

I'll make it log the device revision then. I prefer having a message
telling us something was found than no message at all. It's usefull when
debugging.

>
>> +	ret = cs42l51_fill_cache(codec);
>> +	if (ret < 0) {
>> +		dev_err(&i2c_client->dev, "failed to fill register cache\n");
>> +		goto error;
>> +	}
>
> Normal practice is to reset the chip so it's in a known sensible state
> when the driver starts trying to use it.  Is there a good reason not to
> do this here?

The default reset state is good except for the bits I'm changing few
lines later.

>
>> +	/*
>> +	 * DAC configuration (right place to do that ?)
>
> It's OK to do this for things that the user would unconditionally want.
>
>> +	 * - Use signal processor
>
> What does this mean? It sounds like something that would need to be
> power managed via DAPM?

there are 3 options for DAC:
00 - PCM Serial Port to DAC
01 - Signal Processing Engine to DAC
10 - ADC Serial Port to DAC

but only the "signal processing engine to DAC" configuration is
useful. Using others are dropping some functionnalities.

>
>> +	 * - auto mute
>
> This is probably OK, but exposing as a user visible control would be
> better.

there's a control for that. I'm only setting the register in a known
good state.

>
>> +	 * - vol changes immediate
>
> This is OK.
>
>> +	 * - no de-emphasize
>
> This is normally exposed as a user selectable switch.

idem

>
>> +	/* Unmute PCM-A & PCM-B and set default vol to */
>> +	ret = cs42l51_i2c_write(codec, PCMA_VOL, 0x60);
>> +	if (ret < 0)
>> +		goto error;
>
> This should not be done in the driver, leave it up to userspace.  This
> driver will be used by all systems using this CODEC so just rely on the
> hardware defaults to provide a sane power on configuration - the
> designers will usually ensure that this is at least not harmful.
>

ok. Default is 0db with channel enabled so it's fine I guess.

>> +	/* route microphone */
>> +	ret = cs42l51_i2c_write(codec, ADC_INPUT, 0xF0);
>> +	if (ret < 0)
>> +		goto error;
>
> Ditto.

That's the only way to get microphone support, that's why I didn't
consider it as an option.

>
>> +error_reg:
>> +	snd_soc_unregister_codec(codec);
>> +error:
>> +	return ret;
>
> You're missing some error handling here - there's no free of the device
> data, for example.
>
>> +static int cs42l51_get_chan_mix(struct snd_kcontrol *kcontrol,
>> +			struct snd_ctl_elem_value *ucontrol)
>> +{
>> +	struct snd_soc_codec *codec = snd_kcontrol_chip(kcontrol);
>> +	unsigned long value = snd_soc_read(codec, PCM_MIXER)&3;
>> +
>> +	switch (value) {
>> +	default:
>> +	case 0:
>> +		ucontrol->value.integer.value[0] = 0;
>> +		break;
>> +	case 1:
>> +	case 2:
>> +		ucontrol->value.integer.value[0] = 1;
>> +		break;
>> +	case 3:
>> +		ucontrol->value.integer.value[0] = 2;
>> +		break;
>> +	}
>> +
>> +	return 0;
>> +}
>
> Some comments here might be a little clearer...  Why is the difference
> between 1 and 2 not interesting?

1 and 2 are 2 values for the same things. (L+R)/2 signal is sent to both channels.

>
>> +static const char *mic_boost[] = { "+16dB", "+32dB"};
>
> Could use TLV for this too.
>
>> +static const struct soc_enum cs42l51_mix[] = {
>> +	SOC_ENUM_SINGLE_EXT(ARRAY_SIZE(chan_mix), chan_mix),
>> +	SOC_ENUM_DOUBLE(MIC_CTL, 0, 1, 2, mic_boost),
>> +};
>
> Don't do this, declare separate variables for each.  Indexing into a
> table of mixer elements doesn't help leigibility and is asking for off
> by one errors.  Some drivers do this for historical reasons but modern
> drivers don't.
>
>> +	SOC_DOUBLE_R("PCM Playback Switch", PCMA_VOL, PCMB_VOL, 7, 1, 1),
>
> I suspect that this should be managed via the DAI mute function.

I wondered about this too and I choose to not use it. My thoughts was
that if I was to go and implement the DAI mute, it should rather mute
PCM playback and Analog playback channels at the same time I guess.

>
>> +	SOC_SINGLE("Playback Deemphasis", DAC_CTL, 3, 1, 0),
>
> Should have "Switch" at the end of the control name.
>
>> +	SOC_SINGLE("Mic Bias", MIC_POWER_CTL, 1, 1, 1),
>
> This should be a DAPM MICBIAS control.
>
>> +	SOC_DOUBLE("Mic Powerdown", MIC_POWER_CTL, 2, 3, 1, 0),
>
> What does this do?  Sounds like it should be DAPM controls.

It's to power down (or not) the 2 preamplifiers of the microphone
channels and not to power down the microphone. Maybe the name is not the
best one. Better names accepted.
With this new explanation, should I convert it to DAPM or is it fine as
mixer control ?


Arnaud

Re: [patch 4/6] cs42l51: add asoc driver

[Mark Brown]

On Wed, May 12, 2010 at 03:46:53PM +0200, Arnaud Patard wrote:
> Mark Brown <broonie@opensource.wolfsonmicro.com> writes:
> > On Tue, May 11, 2010 at 06:23:46PM +0200, apatard@mandriva.com wrote:

> >> +	if ((reg < CS42L51_FIRSTREG) || (reg > CS42L51_LASTREG))
> >> +		return -EIO;

> >> +	return cache[reg - CS42L51_FIRSTREG];

> > Please just use the standard register cache access stuff in soc-cache.c.
> > The first register is 1 so you're only burning a single byte of RAM by
> > using the generic code.

> I'm using i2c_smbus_read_i2c_block_data() and didn't notice it in the
> soc-cache.c file. I didn't look further so it's possible that there's
> actually something working for my case.

The soc-cache stuff doesn't deal with initialising the cache so you can
still use that to initialise the cache.  Generally people use the I2C
APIs rather than smbus ones, smbus is just an I2C subset.

> >> +	ret = cs42l51_fill_cache(codec);
> >> +	if (ret < 0) {
> >> +		dev_err(&i2c_client->dev, "failed to fill register cache\n");
> >> +		goto error;
> >> +	}

> > Normal practice is to reset the chip so it's in a known sensible state
> > when the driver starts trying to use it.  Is there a good reason not to
> > do this here?

> The default reset state is good except for the bits I'm changing few
> lines later.

In that case a table of register defaults plus doing a reset is likely
to be more robust.

> >> +	 * - Use signal processor

> > What does this mean? It sounds like something that would need to be
> > power managed via DAPM?

> there are 3 options for DAC:
> 00 - PCM Serial Port to DAC
> 01 - Signal Processing Engine to DAC
> 10 - ADC Serial Port to DAC

> but only the "signal processing engine to DAC" configuration is
> useful. Using others are dropping some functionnalities.

This sounds like useful stuff to offer to the user as a runtime option -
ADC->DAC can be used for bypass paths, and PCM to DAC will presumably
save power if the DSP is not in use.

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

> > Ditto.

> That's the only way to get microphone support, that's why I didn't
> consider it as an option.

The routing should be runtime configurable.  I've no idea what the
options the chip offers are, but presumably some of the other options
are usable.

> >> +	SOC_DOUBLE_R("PCM Playback Switch", PCMA_VOL, PCMB_VOL, 7, 1, 1),

> > I suspect that this should be managed via the DAI mute function.

> I wondered about this too and I choose to not use it. My thoughts was
> that if I was to go and implement the DAI mute, it should rather mute
> PCM playback and Analog playback channels at the same time I guess.

No, the purpose of the DAI mute is specifically to stop the data coming
out of the DAC while the digital audio interface is coming up - some
systems see noise there when starting up so we do the DAC unmute last in
the stream start to ensure that we don't play any of that back to the
user.

> >> +	SOC_DOUBLE("Mic Powerdown", MIC_POWER_CTL, 2, 3, 1, 0),

> > What does this do?  Sounds like it should be DAPM controls.

> It's to power down (or not) the 2 preamplifiers of the microphone
> channels and not to power down the microphone. Maybe the name is not the
> best one. Better names accepted.
> With this new explanation, should I convert it to DAPM or is it fine as
> mixer control ?

That's definitely something that should be managed via DAPM - we only
need to power up the amplifier when the signal is being used.

[patch 5/6] kirkwood: Add i2s support

[apatard]

[-- Attachment #1: kirkwood_i2s.patch --]
[-- Type: text/plain, Size: 33099 bytes --]

This patch enables support for the i2s codec available on kirkwood platforms

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

---
 sound/soc/Kconfig                 |    1 	1 +	0 -	0 !
 sound/soc/Makefile                |    1 	1 +	0 -	0 !
 sound/soc/kirkwood/Kconfig        |   11 	11 +	0 -	0 !
 sound/soc/kirkwood/Makefile       |    6 	6 +	0 -	0 !
 sound/soc/kirkwood/kirkwood-dma.c |  366 	366 +	0 -	0 !
 sound/soc/kirkwood/kirkwood-dma.h |   17 	17 +	0 -	0 !
 sound/soc/kirkwood/kirkwood-i2s.c |  471 	471 +	0 -	0 !
 sound/soc/kirkwood/kirkwood-i2s.h |   17 	17 +	0 -	0 !
 sound/soc/kirkwood/kirkwood.h     |  168 	168 +	0 -	0 !
 9 files changed, 1058 insertions(+)

Index: linux-2.6.33/sound/soc/kirkwood/Kconfig
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ linux-2.6.33/sound/soc/kirkwood/Kconfig	2010-05-11 18:05:14.981650112 +0200
@@ -0,0 +1,11 @@
+config SND_KIRKWOOD_SOC
+	tristate "SoC Audio for the Marvell Kirkwood chip"
+	depends on ARCH_KIRKWOOD
+	help
+	  Say Y or M if you want to add support for codecs attached to
+	  the Kirkwood I2S interface. You will also need
+	  to select the audio interfaces to support below.
+
+config SND_KIRKWOOD_SOC_I2S
+	tristate
+
Index: linux-2.6.33/sound/soc/kirkwood/kirkwood-dma.c
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ linux-2.6.33/sound/soc/kirkwood/kirkwood-dma.c	2010-05-11 18:09:38.361669930 +0200
@@ -0,0 +1,366 @@
+/*
+ * kirkwood-dma.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/init.h>
+#include <linux/module.h>
+#include <linux/device.h>
+#include <linux/io.h>
+#include <linux/slab.h>
+#include <linux/interrupt.h>
+#include <linux/dma-mapping.h>
+#include <linux/mbus.h>
+#include <sound/soc.h>
+#include "kirkwood-dma.h"
+#include "kirkwood.h"
+
+
+struct kirkwood_dma_priv {
+	struct snd_pcm_substream *play_stream;
+	struct snd_pcm_substream *rec_stream;
+	struct kirkwood_dma_data *data;
+};
+
+static struct snd_pcm_hardware kirkwood_dma_snd_hw = {
+	.info = (SNDRV_PCM_INFO_INTERLEAVED |
+		 SNDRV_PCM_INFO_MMAP |
+		 SNDRV_PCM_INFO_MMAP_VALID |
+		 SNDRV_PCM_INFO_BLOCK_TRANSFER |
+		 SNDRV_PCM_INFO_PAUSE),
+	.formats		= KIRKWOOD_FORMATS,
+	.rates			= KIRKWOOD_RATES,
+	.rate_min		= 44100,
+	.rate_max		= 96000,
+	.channels_min		= 1,
+	.channels_max		= 2,
+	.buffer_bytes_max	= KIRKWOOD_SND_MAX_PERIOD_BYTES * KIRKWOOD_SND_MAX_PERIODS,
+	.period_bytes_min	= KIRKWOOD_SND_MIN_PERIOD_BYTES,
+	.period_bytes_max	= KIRKWOOD_SND_MAX_PERIOD_BYTES,
+	.periods_min		= KIRKWOOD_SND_MIN_PERIODS,
+	.periods_max		= KIRKWOOD_SND_MAX_PERIODS,
+	.fifo_size		= 0,
+};
+
+static u64 kirkwood_dma_dmamask = 0xFFFFFFFFUL;
+
+static irqreturn_t kirkwood_dma_irq(int irq, void *dev_id)
+{
+	struct kirkwood_dma_priv *prdata = dev_id;
+	struct kirkwood_dma_data *priv = prdata->data;
+	unsigned long mask, status, cause;
+
+	mask = readl(priv->io + KIRKWOOD_INT_MASK);
+	status = readl(priv->io + KIRKWOOD_INT_CAUSE) & mask;
+
+	cause = readl(priv->io + KIRKWOOD_ERR_CAUSE);
+	if (unlikely(cause)) {
+		printk(KERN_WARNING "%s: got err interrupt 0x%lx\n",
+				__func__, cause);
+		writel(cause, priv->io + KIRKWOOD_ERR_CAUSE);
+		return IRQ_HANDLED;
+	}
+
+	do {
+		/* we've enabled only bytes interrupts ... */
+		if (status & ~(KIRKWOOD_INT_CAUSE_PLAY_BYTES | \
+				KIRKWOOD_INT_CAUSE_REC_BYTES))
+			return IRQ_NONE;
+
+		/* ack int */
+		writel(status, priv->io + KIRKWOOD_INT_CAUSE);
+
+		if (status & KIRKWOOD_INT_CAUSE_PLAY_BYTES)
+			snd_pcm_period_elapsed(prdata->play_stream);
+
+		if (status & KIRKWOOD_INT_CAUSE_REC_BYTES)
+			snd_pcm_period_elapsed(prdata->rec_stream);
+
+		mask = readl(priv->io + KIRKWOOD_INT_MASK);
+		status = readl(priv->io + KIRKWOOD_INT_CAUSE) & mask;
+	} while (status);
+
+	return IRQ_HANDLED;
+}
+
+static int kirkwood_dma_open(struct snd_pcm_substream *substream)
+{
+	int err;
+	struct snd_pcm_runtime *runtime = substream->runtime;
+	struct snd_soc_pcm_runtime *soc_runtime = substream->private_data;
+	struct snd_soc_dai *cpu_dai = soc_runtime->dai->cpu_dai;
+	struct kirkwood_dma_data *priv;
+	struct kirkwood_dma_priv *prdata = cpu_dai->private_data;
+
+	priv = snd_soc_dai_get_dma_data(cpu_dai, substream);
+	snd_soc_set_runtime_hwparams(substream, &kirkwood_dma_snd_hw);
+
+	/* Ensure that all constraints linked to dma burst are fullfilled */
+	err = snd_pcm_hw_constraint_minmax(runtime,
+			SNDRV_PCM_HW_PARAM_BUFFER_BYTES,
+			priv->burst * 2,
+			KIRKWOOD_AUDIO_BUF_MAX-1);
+	if (err < 0)
+		return err;
+
+	err = snd_pcm_hw_constraint_step(runtime, 0,
+			SNDRV_PCM_HW_PARAM_BUFFER_BYTES,
+			priv->burst);
+	if (err < 0)
+		return err;
+
+	err = snd_pcm_hw_constraint_step(substream->runtime, 0,
+			 SNDRV_PCM_HW_PARAM_PERIOD_BYTES,
+			 priv->burst);
+	if (err < 0)
+		return err;
+
+	if (soc_runtime->dai->cpu_dai->private_data == NULL) {
+		prdata = kzalloc(sizeof(struct kirkwood_dma_priv), GFP_KERNEL);
+		if (prdata == NULL)
+			return -ENOMEM;
+
+		prdata->data = priv;
+
+		err = request_irq(priv->irq, kirkwood_dma_irq, IRQF_SHARED,
+				  "kirkwood-dma", prdata);
+		if (err) {
+			kfree(prdata);
+			return -EBUSY;
+		}
+
+		soc_runtime->dai->cpu_dai->private_data = prdata;
+
+		/*
+		 * Enable Error interrupts. We're only ack'ing them but
+		 * it's usefull for diagnostics
+		 */
+		writel((unsigned long)-1, priv->io + KIRKWOOD_ERR_MASK);
+	}
+
+	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
+		prdata->play_stream = substream;
+	else
+		prdata->rec_stream = substream;
+
+	return 0;
+}
+
+static int kirkwood_dma_close(struct snd_pcm_substream *substream)
+{
+	struct snd_soc_pcm_runtime *soc_runtime = substream->private_data;
+	struct snd_soc_dai *cpu_dai = soc_runtime->dai->cpu_dai;
+	struct kirkwood_dma_priv *prdata = cpu_dai->private_data;
+	struct kirkwood_dma_data *priv;
+
+	priv = snd_soc_dai_get_dma_data(cpu_dai, substream);
+
+	if (!prdata || !priv)
+		return 0;
+
+	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
+		prdata->play_stream = NULL;
+	else
+		prdata->rec_stream = NULL;
+
+	if (!prdata->play_stream && !prdata->rec_stream) {
+		writel(0, priv->io + KIRKWOOD_ERR_MASK);
+		free_irq(priv->irq, prdata);
+		kfree(prdata);
+		soc_runtime->dai->cpu_dai->private_data = NULL;
+	}
+
+	return 0;
+}
+
+static int kirkwood_dma_hw_params(struct snd_pcm_substream *substream,
+				struct snd_pcm_hw_params *params)
+{
+	struct snd_pcm_runtime *runtime = substream->runtime;
+
+	snd_pcm_set_runtime_buffer(substream, &substream->dma_buffer);
+	runtime->dma_bytes = params_buffer_bytes(params);
+
+	return 0;
+}
+
+static int kirkwood_dma_hw_free(struct snd_pcm_substream *substream)
+{
+	snd_pcm_set_runtime_buffer(substream, NULL);
+	return 0;
+}
+
+static int kirkwood_dma_prepare(struct snd_pcm_substream *substream)
+{
+	struct snd_pcm_runtime *runtime = substream->runtime;
+	struct snd_soc_pcm_runtime *soc_runtime = substream->private_data;
+	struct snd_soc_dai *cpu_dai = soc_runtime->dai->cpu_dai;
+	struct kirkwood_dma_data *priv;
+	unsigned long size, count;
+
+	priv = snd_soc_dai_get_dma_data(cpu_dai, substream);
+
+	/* compute buffer size in term of "words" as requested in specs */
+	size = frames_to_bytes(runtime, runtime->buffer_size);
+	size = (size>>2)-1;
+	count = snd_pcm_lib_period_bytes(substream);
+
+	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
+		writel(count, priv->io + KIRKWOOD_PLAY_BYTE_INT_COUNT);
+		writel(runtime->dma_addr, priv->io + KIRKWOOD_PLAY_BUF_ADDR);
+		writel(size, priv->io + KIRKWOOD_PLAY_BUF_SIZE);
+	} else {
+		writel(count, priv->io + KIRKWOOD_REC_BYTE_INT_COUNT);
+		writel(runtime->dma_addr, priv->io + KIRKWOOD_REC_BUF_ADDR);
+		writel(size, priv->io + KIRKWOOD_REC_BUF_SIZE);
+	}
+
+
+	return 0;
+}
+
+static int kirkwood_dma_trigger(struct snd_pcm_substream *substream, int cmd)
+{
+	return 0;
+}
+
+static snd_pcm_uframes_t kirkwood_dma_pointer(struct snd_pcm_substream
+						*substream)
+{
+	struct snd_soc_pcm_runtime *soc_runtime = substream->private_data;
+	struct snd_soc_dai *cpu_dai = soc_runtime->dai->cpu_dai;
+	struct kirkwood_dma_data *priv;
+	snd_pcm_uframes_t count;
+
+	priv = snd_soc_dai_get_dma_data(cpu_dai, substream);
+
+	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
+		count = bytes_to_frames(substream->runtime,
+			readl(priv->io + KIRKWOOD_PLAY_BYTE_COUNT));
+	else
+		count = bytes_to_frames(substream->runtime,
+			readl(priv->io + KIRKWOOD_REC_BYTE_COUNT));
+
+	return count;
+}
+
+static int kirkwood_dma_mmap(struct snd_pcm_substream *substream,
+		struct vm_area_struct *vma)
+{
+	struct snd_pcm_runtime *runtime = substream->runtime;
+
+	return dma_mmap_coherent(substream->pcm->card->dev, vma,
+					runtime->dma_area,
+					runtime->dma_addr,
+					runtime->dma_bytes);
+}
+
+
+struct snd_pcm_ops kirkwood_dma_ops = {
+	.open =		kirkwood_dma_open,
+	.close =        kirkwood_dma_close,
+	.ioctl =	snd_pcm_lib_ioctl,
+	.hw_params =	kirkwood_dma_hw_params,
+	.hw_free =      kirkwood_dma_hw_free,
+	.prepare =      kirkwood_dma_prepare,
+	.trigger =	kirkwood_dma_trigger,
+	.pointer =	kirkwood_dma_pointer,
+	.mmap =		kirkwood_dma_mmap,
+};
+
+static int kirkwood_dma_preallocate_dma_buffer(struct snd_pcm *pcm,
+		int stream)
+{
+	struct snd_pcm_substream *substream = pcm->streams[stream].substream;
+	struct snd_dma_buffer *buf = &substream->dma_buffer;
+	size_t size = kirkwood_dma_snd_hw.buffer_bytes_max;
+
+	buf->dev.type = SNDRV_DMA_TYPE_DEV;
+	buf->dev.dev = pcm->card->dev;
+	buf->area = dma_alloc_coherent(pcm->card->dev, size,
+			&buf->addr, GFP_KERNEL);
+	if (!buf->area)
+		return -ENOMEM;
+	buf->bytes = size;
+	buf->private_data = NULL;
+
+	return 0;
+}
+
+static int kirkwood_dma_new(struct snd_card *card,
+		struct snd_soc_dai *dai, struct snd_pcm *pcm)
+{
+	int ret;
+
+	if (!card->dev->dma_mask)
+		card->dev->dma_mask = &kirkwood_dma_dmamask;
+	if (!card->dev->coherent_dma_mask)
+		card->dev->coherent_dma_mask = 0xffffffff;
+
+	if (dai->playback.channels_min) {
+		ret = kirkwood_dma_preallocate_dma_buffer(pcm,
+				SNDRV_PCM_STREAM_PLAYBACK);
+		if (ret)
+			return ret;
+	}
+
+	if (dai->capture.channels_min) {
+		ret = kirkwood_dma_preallocate_dma_buffer(pcm,
+				SNDRV_PCM_STREAM_CAPTURE);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+static void kirkwood_dma_free_dma_buffers(struct snd_pcm *pcm)
+{
+	struct snd_pcm_substream *substream;
+	struct snd_dma_buffer *buf;
+	int stream;
+
+	for (stream = 0; stream < 2; stream++) {
+		substream = pcm->streams[stream].substream;
+		if (!substream)
+			continue;
+		buf = &substream->dma_buffer;
+		if (!buf->area)
+			continue;
+
+		dma_free_coherent(pcm->card->dev, buf->bytes,
+				buf->area, buf->addr);
+		buf->area = NULL;
+	}
+}
+
+struct snd_soc_platform kirkwood_soc_platform = {
+	.name		= "kirkwood-dma",
+	.pcm_ops	= &kirkwood_dma_ops,
+	.pcm_new	= kirkwood_dma_new,
+	.pcm_free	= kirkwood_dma_free_dma_buffers,
+};
+EXPORT_SYMBOL_GPL(kirkwood_soc_platform);
+
+static int __init kirkwood_soc_platform_init(void)
+{
+	return snd_soc_register_platform(&kirkwood_soc_platform);
+}
+module_init(kirkwood_soc_platform_init);
+
+static void __exit kirkwood_soc_platform_exit(void)
+{
+	snd_soc_unregister_platform(&kirkwood_soc_platform);
+}
+module_exit(kirkwood_soc_platform_exit);
+
+MODULE_AUTHOR("Arnaud Patard <apatard@mandriva.com>");
+MODULE_DESCRIPTION("Marvell Kirkwood Audio DMA module");
+MODULE_LICENSE("GPL");
+
Index: linux-2.6.33/sound/soc/kirkwood/kirkwood.h
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ linux-2.6.33/sound/soc/kirkwood/kirkwood.h	2010-05-11 18:05:14.985650562 +0200
@@ -0,0 +1,168 @@
+/*
+ * kirkwood.h
+ *
+ * (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.
+ */
+
+#ifndef _KIRKWOOD_AUDIO_H
+#define _KIRKWOOD_AUDIO_H
+
+#define KIRKWOOD_RECORD_WIN			0
+#define KIRKWOOD_PLAYBACK_WIN			1
+#define KIRKWOOD_MAX_AUDIO_WIN			2
+
+#define AUDIO_WIN_BASE_REG(win)			(0xA00 + ((win)<<3))
+#define AUDIO_WIN_CTRL_REG(win)			(0xA04 + ((win)<<3))
+
+
+#define KIRKWOOD_RECCTL				0x1000
+#define KIRKWOOD_RECCTL_SPDIF_EN		(1<<11)
+#define KIRKWOOD_RECCTL_I2S_EN			(1<<10)
+#define KIRKWOOD_RECCTL_PAUSE			(1<<9)
+#define KIRKWOOD_RECCTL_MUTE			(1<<8)
+#define KIRKWOOD_RECCTL_BURST_MASK		(3<<5)
+#define KIRKWOOD_RECCTL_BURST_128		(2<<5)
+#define KIRKWOOD_RECCTL_BURST_32		(1<<5)
+#define KIRKWOOD_RECCTL_MONO			(1<<4)
+#define KIRKWOOD_RECCTL_MONO_CHAN_RIGHT		(1<<3)
+#define KIRKWOOD_RECCTL_MONO_CHAN_LEFT		(0<<3)
+#define KIRKWOOD_RECCTL_SIZE_MASK		(7<<0)
+#define KIRKWOOD_RECCTL_SIZE_16			(7<<0)
+#define KIRKWOOD_RECCTL_SIZE_16_C		(3<<0)
+#define KIRKWOOD_RECCTL_SIZE_20			(2<<0)
+#define KIRKWOOD_RECCTL_SIZE_24			(1<<0)
+#define KIRKWOOD_RECCTL_SIZE_32			(0<<0)
+
+#define KIRKWOOD_REC_BUF_ADDR			0x1004
+#define KIRKWOOD_REC_BUF_SIZE			0x1008
+#define KIRKWOOD_REC_BYTE_COUNT			0x100C
+
+#define KIRKWOOD_PLAYCTL			0x1100
+#define KIRKWOOD_PLAYCTL_PLAY_BUSY		(1<<16)
+#define KIRKWOOD_PLAYCTL_BURST_MASK		(3<<11)
+#define KIRKWOOD_PLAYCTL_BURST_128		(2<<11)
+#define KIRKWOOD_PLAYCTL_BURST_32		(1<<11)
+#define KIRKWOOD_PLAYCTL_PAUSE			(1<<9)
+#define KIRKWOOD_PLAYCTL_SPDIF_MUTE		(1<<8)
+#define KIRKWOOD_PLAYCTL_I2S_MUTE		(1<<7)
+#define KIRKWOOD_PLAYCTL_SPDIF_EN		(1<<4)
+#define KIRKWOOD_PLAYCTL_I2S_EN			(1<<3)
+#define KIRKWOOD_PLAYCTL_SIZE_MASK		(7<<0)
+#define KIRKWOOD_PLAYCTL_SIZE_16		(7<<0)
+#define KIRKWOOD_PLAYCTL_SIZE_16_C		(3<<0)
+#define KIRKWOOD_PLAYCTL_SIZE_20		(2<<0)
+#define KIRKWOOD_PLAYCTL_SIZE_24		(1<<0)
+#define KIRKWOOD_PLAYCTL_SIZE_32		(0<<0)
+
+#define KIRKWOOD_PLAY_BUF_ADDR			0x1104
+#define KIRKWOOD_PLAY_BUF_SIZE			0x1108
+#define KIRKWOOD_PLAY_BYTE_COUNT		0x110C
+
+#define KIRKWOOD_DCO_CTL			0x1204
+#define KIRKWOOD_DCO_CTL_OFFSET_MASK		(0xFFF<<2)
+#define KIRKWOOD_DCO_CTL_OFFSET_0		(0x800<<2)
+#define KIRKWOOD_DCO_CTL_FREQ_MASK		(3<<0)
+#define KIRKWOOD_DCO_CTL_FREQ_11		(0<<0)
+#define KIRKWOOD_DCO_CTL_FREQ_12		(1<<0)
+#define KIRKWOOD_DCO_CTL_FREQ_24		(2<<0)
+
+#define KIRKWOOD_DCO_SPCR_STATUS		0x120c
+#define KIRKWOOD_DCO_SPCR_STATUS_DCO_LOCK	(1<<16)
+
+#define KIRKWOOD_ERR_CAUSE			0x1300
+#define KIRKWOOD_ERR_MASK			0x1304
+
+#define KIRKWOOD_INT_CAUSE			0x1308
+#define KIRKWOOD_INT_MASK			0x130C
+#define KIRKWOOD_INT_CAUSE_PLAY_BYTES		(1<<14)
+#define KIRKWOOD_INT_CAUSE_REC_BYTES		(1<<13)
+#define KIRKWOOD_INT_CAUSE_DMA_PLAY_END		(1<<7)
+#define KIRKWOOD_INT_CAUSE_DMA_PLAY_3Q		(1<<6)
+#define KIRKWOOD_INT_CAUSE_DMA_PLAY_HALF	(1<<5)
+#define KIRKWOOD_INT_CAUSE_DMA_PLAY_1Q		(1<<4)
+#define KIRKWOOD_INT_CAUSE_DMA_REC_END		(1<<3)
+#define KIRKWOOD_INT_CAUSE_DMA_REC_3Q		(1<<2)
+#define KIRKWOOD_INT_CAUSE_DMA_REC_HALF		(1<<1)
+#define KIRKWOOD_INT_CAUSE_DMA_REC_1Q		(1<<0)
+
+#define KIRKWOOD_REC_BYTE_INT_COUNT		0x1310
+#define KIRKWOOD_PLAY_BYTE_INT_COUNT		0x1314
+#define KIRKWOOD_BYTE_INT_COUNT_MASK		0xffffff
+
+#define KIRKWOOD_I2S_PLAYCTL			0x2508
+#define KIRKWOOD_I2S_RECCTL			0x2408
+#define KIRKWOOD_I2S_CTL_JUST_MASK		(0xf<<26)
+#define KIRKWOOD_I2S_CTL_LJ			(0<<26)
+#define KIRKWOOD_I2S_CTL_I2S			(5<<26)
+#define KIRKWOOD_I2S_CTL_RJ			(8<<26)
+#define KIRKWOOD_I2S_CTL_SIZE_MASK		(3<<30)
+#define KIRKWOOD_I2S_CTL_SIZE_16		(3<<30)
+#define KIRKWOOD_I2S_CTL_SIZE_20		(2<<30)
+#define KIRKWOOD_I2S_CTL_SIZE_24		(1<<30)
+#define KIRKWOOD_I2S_CTL_SIZE_32		(0<<30)
+
+#define KIRKWOOD_AUDIO_BUF_MAX			(16*1024*1024)
+
+/* Theses values come from the marvell alsa driver */
+/* need to find where they come from               */
+#define KIRKWOOD_SND_MIN_PERIODS		8
+#define KIRKWOOD_SND_MAX_PERIODS		16
+#define	KIRKWOOD_SND_MIN_PERIOD_BYTES		0x4000
+#define	KIRKWOOD_SND_MAX_PERIOD_BYTES		0x4000
+
+
+#define KIRKWOOD_RATES \
+	(SNDRV_PCM_RATE_44100 | \
+	 SNDRV_PCM_RATE_48000 | SNDRV_PCM_RATE_96000)
+#define KIRKWOOD_FORMATS \
+	(SNDRV_PCM_FMTBIT_S16_LE | \
+	 SNDRV_PCM_FMTBIT_S24_LE | \
+	 SNDRV_PCM_FMTBIT_S32_LE)
+
+/* I2S formats and kirkwood dma are the same but it's not
+ * true for SPDIF (which can't do 32
+ */
+#define KIRKWOOD_I2S_RATES			KIRKWOOD_RATES
+#define KIRKWOOD_I2S_FORMATS			KIRKWOOD_FORMATS
+
+struct kirkwood_dma_data {
+	struct resource *mem;
+	void __iomem *io;
+	int irq;
+	int burst;
+	struct mbus_dram_target_info *dram;
+};
+
+static inline void kirkwood_set_dco(void __iomem *io, unsigned long rate)
+{
+	unsigned long value;
+
+	value = KIRKWOOD_DCO_CTL_OFFSET_0;
+	switch (rate) {
+	default:
+	case 44100:
+		value |= KIRKWOOD_DCO_CTL_FREQ_11;
+		break;
+	case 48000:
+		value |= KIRKWOOD_DCO_CTL_FREQ_12;
+		break;
+	case 96000:
+		value |= KIRKWOOD_DCO_CTL_FREQ_24;
+		break;
+	}
+	writel(value, io + KIRKWOOD_DCO_CTL);
+
+	/* wait for dco locked */
+	do {
+		cpu_relax();
+		value = readl(io + KIRKWOOD_DCO_SPCR_STATUS);
+		value &= KIRKWOOD_DCO_SPCR_STATUS;
+	} while (value == 0);
+}
+
+#endif
Index: linux-2.6.33/sound/soc/kirkwood/kirkwood-i2s.c
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ linux-2.6.33/sound/soc/kirkwood/kirkwood-i2s.c	2010-05-11 18:10:30.769691011 +0200
@@ -0,0 +1,471 @@
+/*
+ * kirkwood-i2s.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/init.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/io.h>
+#include <linux/slab.h>
+#include <linux/mbus.h>
+#include <sound/pcm.h>
+#include <sound/pcm_params.h>
+#include <sound/soc.h>
+#include <mach/audio.h>
+#include "kirkwood-i2s.h"
+#include "kirkwood.h"
+
+#define DRV_NAME	"kirkwood-i2s"
+
+struct snd_soc_dai kirkwood_i2s_dai;
+static struct kirkwood_dma_data *priv;
+
+static int kirkwood_i2s_set_fmt(struct snd_soc_dai *cpu_dai,
+		unsigned int fmt)
+{
+	unsigned long mask;
+	unsigned long value;
+
+	switch (fmt & SND_SOC_DAIFMT_FORMAT_MASK) {
+	case SND_SOC_DAIFMT_RIGHT_J:
+		mask = KIRKWOOD_I2S_CTL_RJ;
+		break;
+	case SND_SOC_DAIFMT_LEFT_J:
+		mask = KIRKWOOD_I2S_CTL_LJ;
+		break;
+	case SND_SOC_DAIFMT_I2S:
+		mask = KIRKWOOD_I2S_CTL_I2S;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	/*
+	 * Set same format for playback and record
+	 * This avoids some troubles.
+	 */
+	value = readl(priv->io+KIRKWOOD_I2S_PLAYCTL);
+	value &= ~KIRKWOOD_I2S_CTL_JUST_MASK;
+	value |= mask;
+	writel(value, priv->io+KIRKWOOD_I2S_PLAYCTL);
+
+	value = readl(priv->io+KIRKWOOD_I2S_RECCTL);
+	value &= ~KIRKWOOD_I2S_CTL_JUST_MASK;
+	value |= mask;
+	writel(value, priv->io+KIRKWOOD_I2S_RECCTL);
+
+	return 0;
+}
+
+static int kirkwood_i2s_hw_params(struct snd_pcm_substream *substream,
+				 struct snd_pcm_hw_params *params,
+				 struct snd_soc_dai *dai)
+{
+	unsigned int i2s_reg, reg;
+	unsigned long i2s_value, value;
+
+	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
+		i2s_reg = KIRKWOOD_I2S_PLAYCTL;
+		reg = KIRKWOOD_PLAYCTL;
+	} else {
+		i2s_reg = KIRKWOOD_I2S_RECCTL;
+		reg = KIRKWOOD_RECCTL;
+	}
+
+	/* set dco conf */
+	kirkwood_set_dco(priv->io, params_rate(params));
+
+	i2s_value = readl(priv->io+i2s_reg);
+	i2s_value &= ~KIRKWOOD_I2S_CTL_SIZE_MASK;
+
+	value = readl(priv->io+reg);
+	value &= ~KIRKWOOD_PLAYCTL_SIZE_MASK;
+
+	/*
+	 * Size settings in play/rec i2s control regs and play/rec control
+	 * regs must be the same.
+	 */
+	switch (params_format(params)) {
+	case SNDRV_PCM_FORMAT_S16_LE:
+		i2s_value |= KIRKWOOD_I2S_CTL_SIZE_16;
+		value |= KIRKWOOD_PLAYCTL_SIZE_16_C;
+		break;
+	/*
+	 * doesn't work... S20_3LE != kirkwood 20bit format ?
+	 *
+	case SNDRV_PCM_FORMAT_S20_3LE:
+		i2s_value |= KIRKWOOD_I2S_CTL_SIZE_20;
+		value |= KIRKWOOD_PLAYCTL_SIZE_20;
+		break;
+	*/
+	case SNDRV_PCM_FORMAT_S24_LE:
+		i2s_value |= KIRKWOOD_I2S_CTL_SIZE_24;
+		value |= KIRKWOOD_PLAYCTL_SIZE_24;
+		break;
+	case SNDRV_PCM_FORMAT_S32_LE:
+		i2s_value |= KIRKWOOD_I2S_CTL_SIZE_32;
+		value |= KIRKWOOD_PLAYCTL_SIZE_32;
+		break;
+	default:
+		return -EINVAL;
+	}
+	writel(i2s_value, priv->io+i2s_reg);
+	writel(value, priv->io+reg);
+
+	return 0;
+}
+
+static int kirkwood_i2s_play_trigger(struct snd_pcm_substream *substream,
+				int cmd, struct snd_soc_dai *dai)
+{
+	unsigned long value;
+
+	/*
+	 * specs says KIRKWOOD_PLAYCTL must be read 2 times before
+	 * changing it. So read 1 time here and 1 later.
+	 */
+	value = readl(priv->io + KIRKWOOD_PLAYCTL);
+
+	switch (cmd) {
+	case SNDRV_PCM_TRIGGER_START:
+		/* stop audio, enable interrupts */
+		value = readl(priv->io + KIRKWOOD_PLAYCTL);
+		value |= KIRKWOOD_PLAYCTL_PAUSE;
+		writel(value, priv->io + KIRKWOOD_PLAYCTL);
+
+		value = readl(priv->io + KIRKWOOD_INT_MASK);
+		value |= KIRKWOOD_INT_CAUSE_PLAY_BYTES;
+		writel(value, priv->io + KIRKWOOD_INT_MASK);
+
+		/* configure audio & enable i2s playback */
+		value = readl(priv->io + KIRKWOOD_PLAYCTL);
+		value &= ~KIRKWOOD_PLAYCTL_BURST_MASK;
+		value &= ~(KIRKWOOD_PLAYCTL_PAUSE|KIRKWOOD_PLAYCTL_SPDIF_EN);
+
+		if (priv->burst == 32)
+			value |= KIRKWOOD_PLAYCTL_BURST_32;
+		else
+			value |= KIRKWOOD_PLAYCTL_BURST_128;
+		value |= KIRKWOOD_PLAYCTL_I2S_EN;
+		writel(value, priv->io + KIRKWOOD_PLAYCTL);
+		break;
+
+	case SNDRV_PCM_TRIGGER_STOP:
+		/* stop audio, disable interrupts */
+		value = readl(priv->io + KIRKWOOD_PLAYCTL);
+		value |= KIRKWOOD_PLAYCTL_PAUSE;
+		writel(value, priv->io + KIRKWOOD_PLAYCTL);
+
+		value = readl(priv->io + KIRKWOOD_INT_MASK);
+		value &= ~KIRKWOOD_INT_CAUSE_PLAY_BYTES;
+		writel(value, priv->io + KIRKWOOD_INT_MASK);
+
+		/* disable all playbacks */
+		value = readl(priv->io + KIRKWOOD_PLAYCTL);
+		value &= ~(KIRKWOOD_PLAYCTL_I2S_EN | KIRKWOOD_PLAYCTL_SPDIF_EN);
+		writel(value, priv->io + KIRKWOOD_PLAYCTL);
+		break;
+
+	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
+	case SNDRV_PCM_TRIGGER_SUSPEND:
+		value = readl(priv->io + KIRKWOOD_PLAYCTL);
+		value |= KIRKWOOD_PLAYCTL_PAUSE;
+		writel(value, priv->io + KIRKWOOD_PLAYCTL);
+		break;
+
+	case SNDRV_PCM_TRIGGER_RESUME:
+	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
+		value = readl(priv->io + KIRKWOOD_PLAYCTL);
+		value &= ~KIRKWOOD_PLAYCTL_PAUSE;
+		writel(value, priv->io + KIRKWOOD_PLAYCTL);
+		break;
+
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int kirkwood_i2s_rec_trigger(struct snd_pcm_substream *substream,
+				int cmd, struct snd_soc_dai *dai)
+{
+	unsigned long value;
+
+	value = readl(priv->io + KIRKWOOD_RECCTL);
+
+	switch (cmd) {
+	case SNDRV_PCM_TRIGGER_START:
+		/* stop audio, enable interrupts */
+		value = readl(priv->io + KIRKWOOD_RECCTL);
+		value |= KIRKWOOD_RECCTL_PAUSE;
+		writel(value, priv->io + KIRKWOOD_RECCTL);
+
+		value = readl(priv->io + KIRKWOOD_INT_MASK);
+		value |= KIRKWOOD_INT_CAUSE_REC_BYTES;
+		writel(value, priv->io + KIRKWOOD_INT_MASK);
+
+		/* configure audio & enable i2s record */
+		value = readl(priv->io + KIRKWOOD_RECCTL);
+		value &= ~KIRKWOOD_RECCTL_BURST_MASK;
+		value &= ~KIRKWOOD_RECCTL_MONO;
+		value &= ~(KIRKWOOD_RECCTL_PAUSE | KIRKWOOD_RECCTL_SPDIF_EN);
+
+		if (priv->burst == 32)
+			value |= KIRKWOOD_RECCTL_BURST_32;
+		else
+			value |= KIRKWOOD_RECCTL_BURST_128;
+		value |= KIRKWOOD_RECCTL_I2S_EN;
+
+		writel(value, priv->io + KIRKWOOD_RECCTL);
+		break;
+
+	case SNDRV_PCM_TRIGGER_STOP:
+		/* stop audio, disable interrupts */
+		value = readl(priv->io + KIRKWOOD_RECCTL);
+		value |= KIRKWOOD_RECCTL_PAUSE;
+		writel(value, priv->io + KIRKWOOD_RECCTL);
+
+		value = readl(priv->io + KIRKWOOD_INT_MASK);
+		value &= ~KIRKWOOD_INT_CAUSE_REC_BYTES;
+		writel(value, priv->io + KIRKWOOD_INT_MASK);
+
+		/* disable all records */
+		value = readl(priv->io + KIRKWOOD_RECCTL);
+		value &= ~(KIRKWOOD_RECCTL_I2S_EN | KIRKWOOD_RECCTL_SPDIF_EN);
+		writel(value, priv->io + KIRKWOOD_RECCTL);
+		break;
+
+	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
+	case SNDRV_PCM_TRIGGER_SUSPEND:
+		value = readl(priv->io + KIRKWOOD_RECCTL);
+		value |= KIRKWOOD_RECCTL_PAUSE;
+		writel(value, priv->io + KIRKWOOD_RECCTL);
+		break;
+
+	case SNDRV_PCM_TRIGGER_RESUME:
+	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
+		value = readl(priv->io + KIRKWOOD_RECCTL);
+		value &= ~KIRKWOOD_RECCTL_PAUSE;
+		writel(value, priv->io + KIRKWOOD_RECCTL);
+		break;
+
+	default:
+		return -EINVAL;
+		break;
+	}
+
+	return 0;
+}
+
+static int kirkwood_i2s_trigger(struct snd_pcm_substream *substream, int cmd,
+			       struct snd_soc_dai *dai)
+{
+	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
+		return kirkwood_i2s_play_trigger(substream, cmd, dai);
+	else
+		return kirkwood_i2s_rec_trigger(substream, cmd, dai);
+
+	return 0;
+}
+
+static int kirkwood_i2s_set_sysclk(struct snd_soc_dai *cpu_dai,
+				int clk_id, unsigned int freq, int dir)
+{
+	return 0;
+}
+
+static void kirkwood_i2s_conf_mbus_windows(void __iomem *base,
+					struct mbus_dram_target_info *dram)
+{
+	int win_num;
+
+	/* First disable and clear windows */
+	for (win_num = 0; win_num < KIRKWOOD_MAX_AUDIO_WIN; win_num++) {
+		writel(0, base + AUDIO_WIN_CTRL_REG(win_num));
+		writel(0, base + AUDIO_WIN_BASE_REG(win_num));
+	}
+
+	/* Setup windows for DDR */
+	for (win_num = 0; win_num < KIRKWOOD_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 + AUDIO_WIN_BASE_REG(win_num));
+		writel(((cs->size - 1) & 0xffff0000) |
+		       (cs->mbus_attr << 8) |
+		       (dram->mbus_dram_target_id << 4) | 1,
+			base + AUDIO_WIN_CTRL_REG(win_num));
+	}
+}
+
+static void mv_audio_init(void __iomem *base)
+{
+	int timeout = 10000000;
+	unsigned int reg_data;
+
+	reg_data = readl(base + 0x1200);
+	reg_data &= (~(0x333FF8));
+	reg_data |= 0x111D18;
+	writel(reg_data, base + 0x1200);
+
+	do {
+		timeout--;
+	} while (timeout);
+
+	reg_data = readl(base + 0x1200);
+	reg_data &= (~(0x333FF8));
+	reg_data |= 0x111D18;
+
+	writel(reg_data, base + 0x1200);
+}
+
+static int kirkwood_i2s_probe(struct platform_device *pdev,
+			     struct snd_soc_dai *dai)
+{
+	struct resource *mem;
+	struct kirkwood_soc_platform_data *data =
+		pdev->dev.platform_data;
+	int err;
+	unsigned long value;
+
+	priv = kzalloc(sizeof(struct kirkwood_dma_data), GFP_KERNEL);
+	if (!priv) {
+		dev_err(&pdev->dev, "allocation failed\n");
+		err = -ENOMEM;
+		goto error;
+	}
+
+	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!mem) {
+		dev_err(&pdev->dev, "platform_get_resource failed\n");
+		err = -ENXIO;
+		goto err_alloc;
+	}
+
+	priv->mem = request_mem_region(mem->start, SZ_16K, DRV_NAME);
+	if (!priv->mem) {
+		dev_err(&pdev->dev, "request_mem_region failed\n");
+		err = -EBUSY;
+		goto error;
+	}
+
+	priv->io = ioremap(priv->mem->start, SZ_16K);
+	if (!priv->io) {
+		dev_err(&pdev->dev, "ioremap failed\n");
+		err = -ENOMEM;
+		goto err_iomem;
+	}
+
+	priv->irq = platform_get_irq(pdev, 0);
+	if (priv->irq <= 0) {
+		dev_err(&pdev->dev, "platform_get_irq failed\n");
+		err = -ENXIO;
+		goto err_ioremap;
+	}
+
+	if (!data || !data->dram) {
+		dev_err(&pdev->dev, "no platform data ?!\n");
+		err = -EINVAL;
+		goto err_ioremap;
+	}
+
+	kirkwood_i2s_conf_mbus_windows(priv->io, data->dram);
+	priv->dram = data->dram;
+
+	/* FIXME : should not be hardcoded */
+	priv->burst = 128;
+
+	kirkwood_i2s_dai.capture.dma_data = priv;
+	kirkwood_i2s_dai.playback.dma_data = priv;
+
+	/* put system in a "safe" state : */
+	/* disable audio interrupts */
+	writel(0xffffffff, priv->io + KIRKWOOD_INT_CAUSE);
+	writel(0, priv->io + KIRKWOOD_INT_MASK);
+
+	mv_audio_init(priv->io);
+
+	/* disable playback/record */
+	value = readl(priv->io + KIRKWOOD_PLAYCTL);
+	value &= ~(KIRKWOOD_PLAYCTL_I2S_EN|KIRKWOOD_PLAYCTL_SPDIF_EN);
+	writel(value, priv->io + KIRKWOOD_PLAYCTL);
+
+	value = readl(priv->io + KIRKWOOD_RECCTL);
+	value &= ~(KIRKWOOD_RECCTL_I2S_EN | KIRKWOOD_RECCTL_SPDIF_EN);
+	writel(value, priv->io + KIRKWOOD_RECCTL);
+
+	dev_info(&pdev->dev, "kirkwood-i2s driver loaded\n");
+
+	return 0;
+
+err_ioremap:
+	iounmap(priv->io);
+err_iomem:
+	release_mem_region(priv->mem->start, SZ_16K);
+err_alloc:
+	kfree(priv);
+error:
+	return err;
+}
+
+static void kirkwood_i2s_remove(struct platform_device *pdev,
+				struct snd_soc_dai *dai)
+{
+	if (priv) {
+		iounmap(priv->io);
+		release_mem_region(priv->mem->start, SZ_16K);
+		kfree(priv);
+	}
+}
+
+static struct snd_soc_dai_ops kirkwood_i2s_dai_ops = {
+	.trigger	= kirkwood_i2s_trigger,
+	.hw_params      = kirkwood_i2s_hw_params,
+	.set_sysclk     = kirkwood_i2s_set_sysclk,
+	.set_fmt        = kirkwood_i2s_set_fmt,
+};
+
+
+struct snd_soc_dai kirkwood_i2s_dai = {
+	.name = DRV_NAME,
+	.id = 0,
+	.probe = kirkwood_i2s_probe,
+	.remove = kirkwood_i2s_remove,
+	.playback = {
+		.channels_min = 1,
+		.channels_max = 2,
+		.rates = KIRKWOOD_I2S_RATES,
+		.formats = KIRKWOOD_I2S_FORMATS,},
+	.capture = {
+		.channels_min = 1,
+		.channels_max = 2,
+		.rates = KIRKWOOD_I2S_RATES,
+		.formats = KIRKWOOD_I2S_FORMATS,},
+	.ops = &kirkwood_i2s_dai_ops,
+};
+EXPORT_SYMBOL_GPL(kirkwood_i2s_dai);
+
+static int __init kirkwood_i2s_init(void)
+{
+	return snd_soc_register_dai(&kirkwood_i2s_dai);
+}
+module_init(kirkwood_i2s_init);
+
+static void __exit kirkwood_i2s_exit(void)
+{
+	snd_soc_unregister_dai(&kirkwood_i2s_dai);
+}
+module_exit(kirkwood_i2s_exit);
+
+/* Module information */
+MODULE_AUTHOR("Arnaud Patard, <apatard@mandriva.com>");
+MODULE_DESCRIPTION("Kirkwood I2S SoC Interface");
+MODULE_LICENSE("GPL");
Index: linux-2.6.33/sound/soc/kirkwood/Makefile
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ linux-2.6.33/sound/soc/kirkwood/Makefile	2010-05-11 18:05:14.985650562 +0200
@@ -0,0 +1,6 @@
+snd-soc-kirkwood-objs := kirkwood-dma.o
+snd-soc-kirkwood-i2s-objs := kirkwood-i2s.o
+
+obj-$(CONFIG_SND_KIRKWOOD_SOC) += snd-soc-kirkwood.o
+obj-$(CONFIG_SND_KIRKWOOD_SOC_I2S) += snd-soc-kirkwood-i2s.o
+
Index: linux-2.6.33/sound/soc/Makefile
===================================================================
--- linux-2.6.33.orig/sound/soc/Makefile	2010-05-11 17:38:48.533649884 +0200
+++ linux-2.6.33/sound/soc/Makefile	2010-05-11 18:05:14.985650562 +0200
@@ -8,6 +8,7 @@ obj-$(CONFIG_SND_SOC)	+= blackfin/
 obj-$(CONFIG_SND_SOC)	+= davinci/
 obj-$(CONFIG_SND_SOC)	+= fsl/
 obj-$(CONFIG_SND_SOC)   += imx/
+obj-$(CONFIG_SND_SOC)	+= kirkwood/
 obj-$(CONFIG_SND_SOC)	+= omap/
 obj-$(CONFIG_SND_SOC)	+= pxa/
 obj-$(CONFIG_SND_SOC)	+= s3c24xx/
Index: linux-2.6.33/sound/soc/kirkwood/kirkwood-dma.h
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ linux-2.6.33/sound/soc/kirkwood/kirkwood-dma.h	2010-05-11 18:05:14.985650562 +0200
@@ -0,0 +1,17 @@
+/*
+ * kirkwood-dma.h
+ *
+ * (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.
+ */
+
+#ifndef _KIRKWOOD_DMA_H
+#define _KIRKWOOD_DMA_H
+
+extern struct snd_soc_platform kirkwood_soc_platform;
+
+#endif
Index: linux-2.6.33/sound/soc/kirkwood/kirkwood-i2s.h
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ linux-2.6.33/sound/soc/kirkwood/kirkwood-i2s.h	2010-05-11 18:05:14.989649860 +0200
@@ -0,0 +1,17 @@
+/*
+ * kirkwood-i2s.h
+ *
+ * (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.
+ */
+
+#ifndef _KIRKWOOD_I2S_H
+#define _KIRKWOOD_I2S_H
+
+extern struct snd_soc_dai kirkwood_i2s_dai;
+
+#endif
Index: linux-2.6.33/sound/soc/Kconfig
===================================================================
--- linux-2.6.33.orig/sound/soc/Kconfig	2010-05-11 17:38:48.509650210 +0200
+++ linux-2.6.33/sound/soc/Kconfig	2010-05-11 18:05:14.989649860 +0200
@@ -30,6 +30,7 @@ source "sound/soc/blackfin/Kconfig"
 source "sound/soc/davinci/Kconfig"
 source "sound/soc/fsl/Kconfig"
 source "sound/soc/imx/Kconfig"
+source "sound/soc/kirkwood/Kconfig"
 source "sound/soc/omap/Kconfig"
 source "sound/soc/pxa/Kconfig"
 source "sound/soc/s3c24xx/Kconfig"

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

[Mark Brown]

On Tue, May 11, 2010 at 06:23:47PM +0200, apatard@mandriva.com wrote:
> This patch enables support for the i2s codec available on kirkwood platforms

It's not a CODEC, it's the I2S controller in the CPU.  A CODEC is a
device with DACs and ADCs in it.

> +	do {
> +		/* we've enabled only bytes interrupts ... */

You were checking for error interrupts further up the function?

> +		if (status & ~(KIRKWOOD_INT_CAUSE_PLAY_BYTES | \
> +				KIRKWOOD_INT_CAUSE_REC_BYTES))
> +			return IRQ_NONE;

Surely it'd make sense to log the unexpected interrupts for diagnostics?
You should never get here if there are no interrupts you've enabled.
Unless the interrupt is shared, but that'd be a bit surprising for a
SoC.

It also looks like it'd be more sensible to either drop the do/while
loop entirely (the IRQ core should handle the interrupt still being
asserted and it doesn't look like any of the handling should take so
long that interrupts reassert while it's running anyway) or move the
handling of error interrupts and the first read into the loop so you
don't have two slightly different paths.

> +		/* ack int */
> +		writel(status, priv->io + KIRKWOOD_INT_CAUSE);
> +
> +		if (status & KIRKWOOD_INT_CAUSE_PLAY_BYTES)
> +			snd_pcm_period_elapsed(prdata->play_stream);
> +
> +		if (status & KIRKWOOD_INT_CAUSE_REC_BYTES)
> +			snd_pcm_period_elapsed(prdata->rec_stream);
> +
> +		mask = readl(priv->io + KIRKWOOD_INT_MASK);
> +		status = readl(priv->io + KIRKWOOD_INT_CAUSE) & mask;

This masking didn't happen prior to the first entry into the loop.

> +       if (soc_runtime->dai->cpu_dai->private_data == NULL) {

It seems a bit icky to be managing the DAI private data here...

> +		err = request_irq(priv->irq, kirkwood_dma_irq, IRQF_SHARED,
> +				  "kirkwood-dma", prdata);

I'd suggest "kirkwood-i2s" as a name here.  Or take the name from the
DAI.

> +static int kirkwood_dma_trigger(struct snd_pcm_substream *substream, int cmd)
> +{
> +	return 0;
> +}

Remove this if it's not implemented.

> +static int kirkwood_dma_mmap(struct snd_pcm_substream *substream,
> +		struct vm_area_struct *vma)
> +{
> +	struct snd_pcm_runtime *runtime = substream->runtime;
> +
> +	return dma_mmap_coherent(substream->pcm->card->dev, vma,
> +					runtime->dma_area,
> +					runtime->dma_addr,
> +					runtime->dma_bytes);
> +}

Hrm, this looks like there should be a utility function to do it, it's
not terribly driver specific.

> +struct snd_soc_platform kirkwood_soc_platform = {
> +	.name		= "kirkwood-dma",

I'd also add an audio in here or something.

> +#define AUDIO_WIN_BASE_REG(win)			(0xA00 + ((win)<<3))
> +#define AUDIO_WIN_CTRL_REG(win)			(0xA04 + ((win)<<3))

Namespacing.  Or move into the driver, there seems little reason for
anything else to be peering at these.

> +#define KIRKWOOD_RATES \
> +	(SNDRV_PCM_RATE_44100 | \
> +	 SNDRV_PCM_RATE_48000 | SNDRV_PCM_RATE_96000)
> +#define KIRKWOOD_FORMATS \
> +	(SNDRV_PCM_FMTBIT_S16_LE | \
> +	 SNDRV_PCM_FMTBIT_S24_LE | \
> +	 SNDRV_PCM_FMTBIT_S32_LE)

Move these into the driver.

> +static inline void kirkwood_set_dco(void __iomem *io, unsigned long rate)
> +{
> +	unsigned long value;
> +
> +	value = KIRKWOOD_DCO_CTL_OFFSET_0;
> +	switch (rate) {
> +	default:
> +	case 44100:
> +		value |= KIRKWOOD_DCO_CTL_FREQ_11;
> +		break;
> +	case 48000:
> +		value |= KIRKWOOD_DCO_CTL_FREQ_12;
> +		break;
> +	case 96000:
> +		value |= KIRKWOOD_DCO_CTL_FREQ_24;
> +		break;
> +	}
> +	writel(value, io + KIRKWOOD_DCO_CTL);
> +
> +	/* wait for dco locked */
> +	do {
> +		cpu_relax();
> +		value = readl(io + KIRKWOOD_DCO_SPCR_STATUS);
> +		value &= KIRKWOOD_DCO_SPCR_STATUS;
> +	} while (value == 0);
> +}

Why is this an inline function in the header?

> +	 * Size settings in play/rec i2s control regs and play/rec control
> +	 * regs must be the same.
> +	 */

Ideally you'd be setting up constraints for this.

> +	/*
> +	 * specs says KIRKWOOD_PLAYCTL must be read 2 times before
> +	 * changing it. So read 1 time here and 1 later.
> +	 */
> +	value = readl(priv->io + KIRKWOOD_PLAYCTL);

Nice...

> +	switch (cmd) {
> +	case SNDRV_PCM_TRIGGER_START:
> +		/* stop audio, enable interrupts */

If audio is running when you're getting a start trigger something is
wrong...

> +static int kirkwood_i2s_set_sysclk(struct snd_soc_dai *cpu_dai,
> +				int clk_id, unsigned int freq, int dir)
> +{
> +	return 0;
> +}

Remove this.

> +static void mv_audio_init(void __iomem *base)
> +{
> +	int timeout = 10000000;
> +	unsigned int reg_data;
> +
> +	reg_data = readl(base + 0x1200);
> +	reg_data &= (~(0x333FF8));
> +	reg_data |= 0x111D18;
> +	writel(reg_data, base + 0x1200);

It's like magic!

> +
> +	do {
> +		timeout--;
> +	} while (timeout);

The driver is busy waiting here but not polling anything.  Just use a
sleep?

> +	/* FIXME : should not be hardcoded */
> +	priv->burst = 128;

Platform data?

> +static int kirkwood_i2s_probe(struct platform_device *pdev,
> +                            struct snd_soc_dai *dai)

This is the ASoC probe but...

+       mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);

...it's pulling all sorts of resources out of the device which are
specific to the I2S controller.  You should have the I2S controller have
a normal platform device which is probed on system startup and triggers
DAI registration, just as you're doing in the CODEC driver.  All the
resources should be there.  Apart from anything else this means you only
need to set up each I2S controller once in the arch/arm code for the
CPU.

> +	mv_audio_init(priv->io);

Seems a bit odd ot have the separate init function in the middle of a
bunch of other register writes.

> +	dev_info(&pdev->dev, "kirkwood-i2s driver loaded\n");

May as well remove this.  The core logs device registration anyway.

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

[Saeed Bishara]

Hi, 
	some of the dma stuff handled by the i2s! For example the i2s trigger function actually enables the dma, this should be moved to the dma driver. Also, the playback and capture trigger funtions looks alike except to registers offsets, maybe you can unify them.
	I suggest also to used devm_ functions in the i2s_probe (devm_request_mem_region, devm_kzalloc, devm_ioremap), this way you can remove the err_ cases. 


saeed
	  

>-----Original Message-----
>From: Mark Brown [mailto:broonie@opensource.wolfsonmicro.com] 
>Sent: Wednesday, May 12, 2010 1:24 PM
>To: apatard@mandriva.com
>Cc: alsa-devel@alsa-project.org; Saeed Bishara; 
>tbm@cyrius.com; nico@fluxnic.net
>Subject: Re: [patch 5/6] kirkwood: Add i2s support
>
>On Tue, May 11, 2010 at 06:23:47PM +0200, apatard@mandriva.com wrote:
>> This patch enables support for the i2s codec available on 
>kirkwood platforms
>
>It's not a CODEC, it's the I2S controller in the CPU.  A CODEC is a
>device with DACs and ADCs in it.
>
>> +	do {
>> +		/* we've enabled only bytes interrupts ... */
>
>You were checking for error interrupts further up the function?
>
>> +		if (status & ~(KIRKWOOD_INT_CAUSE_PLAY_BYTES | \
>> +				KIRKWOOD_INT_CAUSE_REC_BYTES))
>> +			return IRQ_NONE;
>
>Surely it'd make sense to log the unexpected interrupts for 
>diagnostics?
>You should never get here if there are no interrupts you've enabled.
>Unless the interrupt is shared, but that'd be a bit surprising for a
>SoC.
>
>It also looks like it'd be more sensible to either drop the do/while
>loop entirely (the IRQ core should handle the interrupt still being
>asserted and it doesn't look like any of the handling should take so
>long that interrupts reassert while it's running anyway) or move the
>handling of error interrupts and the first read into the loop so you
>don't have two slightly different paths.
>
>> +		/* ack int */
>> +		writel(status, priv->io + KIRKWOOD_INT_CAUSE);
>> +
>> +		if (status & KIRKWOOD_INT_CAUSE_PLAY_BYTES)
>> +			snd_pcm_period_elapsed(prdata->play_stream);
>> +
>> +		if (status & KIRKWOOD_INT_CAUSE_REC_BYTES)
>> +			snd_pcm_period_elapsed(prdata->rec_stream);
>> +
>> +		mask = readl(priv->io + KIRKWOOD_INT_MASK);
>> +		status = readl(priv->io + KIRKWOOD_INT_CAUSE) & mask;
>
>This masking didn't happen prior to the first entry into the loop.
>
>> +       if (soc_runtime->dai->cpu_dai->private_data == NULL) {
>
>It seems a bit icky to be managing the DAI private data here...
>
>> +		err = request_irq(priv->irq, kirkwood_dma_irq, 
>IRQF_SHARED,
>> +				  "kirkwood-dma", prdata);
>
>I'd suggest "kirkwood-i2s" as a name here.  Or take the name from the
>DAI.
>
>> +static int kirkwood_dma_trigger(struct snd_pcm_substream 
>*substream, int cmd)
>> +{
>> +	return 0;
>> +}
>
>Remove this if it's not implemented.
>
>> +static int kirkwood_dma_mmap(struct snd_pcm_substream *substream,
>> +		struct vm_area_struct *vma)
>> +{
>> +	struct snd_pcm_runtime *runtime = substream->runtime;
>> +
>> +	return dma_mmap_coherent(substream->pcm->card->dev, vma,
>> +					runtime->dma_area,
>> +					runtime->dma_addr,
>> +					runtime->dma_bytes);
>> +}
>
>Hrm, this looks like there should be a utility function to do it, it's
>not terribly driver specific.
>
>> +struct snd_soc_platform kirkwood_soc_platform = {
>> +	.name		= "kirkwood-dma",
>
>I'd also add an audio in here or something.
>
>> +#define AUDIO_WIN_BASE_REG(win)			(0xA00 
>+ ((win)<<3))
>> +#define AUDIO_WIN_CTRL_REG(win)			(0xA04 
>+ ((win)<<3))
>
>Namespacing.  Or move into the driver, there seems little reason for
>anything else to be peering at these.
>
>> +#define KIRKWOOD_RATES \
>> +	(SNDRV_PCM_RATE_44100 | \
>> +	 SNDRV_PCM_RATE_48000 | SNDRV_PCM_RATE_96000)
>> +#define KIRKWOOD_FORMATS \
>> +	(SNDRV_PCM_FMTBIT_S16_LE | \
>> +	 SNDRV_PCM_FMTBIT_S24_LE | \
>> +	 SNDRV_PCM_FMTBIT_S32_LE)
>
>Move these into the driver.
>
>> +static inline void kirkwood_set_dco(void __iomem *io, 
>unsigned long rate)
>> +{
>> +	unsigned long value;
>> +
>> +	value = KIRKWOOD_DCO_CTL_OFFSET_0;
>> +	switch (rate) {
>> +	default:
>> +	case 44100:
>> +		value |= KIRKWOOD_DCO_CTL_FREQ_11;
>> +		break;
>> +	case 48000:
>> +		value |= KIRKWOOD_DCO_CTL_FREQ_12;
>> +		break;
>> +	case 96000:
>> +		value |= KIRKWOOD_DCO_CTL_FREQ_24;
>> +		break;
>> +	}
>> +	writel(value, io + KIRKWOOD_DCO_CTL);
>> +
>> +	/* wait for dco locked */
>> +	do {
>> +		cpu_relax();
>> +		value = readl(io + KIRKWOOD_DCO_SPCR_STATUS);
>> +		value &= KIRKWOOD_DCO_SPCR_STATUS;
>> +	} while (value == 0);
>> +}
>
>Why is this an inline function in the header?
>
>> +	 * Size settings in play/rec i2s control regs and 
>play/rec control
>> +	 * regs must be the same.
>> +	 */
>
>Ideally you'd be setting up constraints for this.
>
>> +	/*
>> +	 * specs says KIRKWOOD_PLAYCTL must be read 2 times before
>> +	 * changing it. So read 1 time here and 1 later.
>> +	 */
>> +	value = readl(priv->io + KIRKWOOD_PLAYCTL);
>
>Nice...
>
>> +	switch (cmd) {
>> +	case SNDRV_PCM_TRIGGER_START:
>> +		/* stop audio, enable interrupts */
>
>If audio is running when you're getting a start trigger something is
>wrong...
>
>> +static int kirkwood_i2s_set_sysclk(struct snd_soc_dai *cpu_dai,
>> +				int clk_id, unsigned int freq, int dir)
>> +{
>> +	return 0;
>> +}
>
>Remove this.
>
>> +static void mv_audio_init(void __iomem *base)
>> +{
>> +	int timeout = 10000000;
>> +	unsigned int reg_data;
>> +
>> +	reg_data = readl(base + 0x1200);
>> +	reg_data &= (~(0x333FF8));
>> +	reg_data |= 0x111D18;
>> +	writel(reg_data, base + 0x1200);
>
>It's like magic!
>
>> +
>> +	do {
>> +		timeout--;
>> +	} while (timeout);
>
>The driver is busy waiting here but not polling anything.  Just use a
>sleep?
>
>> +	/* FIXME : should not be hardcoded */
>> +	priv->burst = 128;
>
>Platform data?
>
>> +static int kirkwood_i2s_probe(struct platform_device *pdev,
>> +                            struct snd_soc_dai *dai)
>
>This is the ASoC probe but...
>
>+       mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>
>...it's pulling all sorts of resources out of the device which are
>specific to the I2S controller.  You should have the I2S 
>controller have
>a normal platform device which is probed on system startup and triggers
>DAI registration, just as you're doing in the CODEC driver.  All the
>resources should be there.  Apart from anything else this 
>means you only
>need to set up each I2S controller once in the arch/arm code for the
>CPU.
>
>> +	mv_audio_init(priv->io);
>
>Seems a bit odd ot have the separate init function in the middle of a
>bunch of other register writes.
>
>> +	dev_info(&pdev->dev, "kirkwood-i2s driver loaded\n");
>
>May as well remove this.  The core logs device registration anyway.
>

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

[saeed bishara]

Hi,
	some of the dma stuff handled by the i2s! For example the i2s trigger
function actually enables the dma, this should be moved to the dma
driver. Also, the playback and capture trigger funtions looks alike
except to registers offsets, maybe you can unify them.
	I suggest also to used devm_ functions in the i2s_probe
(devm_request_mem_region, devm_kzalloc, devm_ioremap), this way you
can remove the err_ cases.


saeed


On Wed, May 12, 2010 at 1:24 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Tue, May 11, 2010 at 06:23:47PM +0200, apatard@mandriva.com wrote:
>> This patch enables support for the i2s codec available on kirkwood platforms
>
> It's not a CODEC, it's the I2S controller in the CPU.  A CODEC is a
> device with DACs and ADCs in it.
>
>> +     do {
>> +             /* we've enabled only bytes interrupts ... */
>
> You were checking for error interrupts further up the function?
>
>> +             if (status & ~(KIRKWOOD_INT_CAUSE_PLAY_BYTES | \
>> +                             KIRKWOOD_INT_CAUSE_REC_BYTES))
>> +                     return IRQ_NONE;
>
> Surely it'd make sense to log the unexpected interrupts for diagnostics?
> You should never get here if there are no interrupts you've enabled.
> Unless the interrupt is shared, but that'd be a bit surprising for a
> SoC.
>
> It also looks like it'd be more sensible to either drop the do/while
> loop entirely (the IRQ core should handle the interrupt still being
> asserted and it doesn't look like any of the handling should take so
> long that interrupts reassert while it's running anyway) or move the
> handling of error interrupts and the first read into the loop so you
> don't have two slightly different paths.
>
>> +             /* ack int */
>> +             writel(status, priv->io + KIRKWOOD_INT_CAUSE);
>> +
>> +             if (status & KIRKWOOD_INT_CAUSE_PLAY_BYTES)
>> +                     snd_pcm_period_elapsed(prdata->play_stream);
>> +
>> +             if (status & KIRKWOOD_INT_CAUSE_REC_BYTES)
>> +                     snd_pcm_period_elapsed(prdata->rec_stream);
>> +
>> +             mask = readl(priv->io + KIRKWOOD_INT_MASK);
>> +             status = readl(priv->io + KIRKWOOD_INT_CAUSE) & mask;
>
> This masking didn't happen prior to the first entry into the loop.
>
>> +       if (soc_runtime->dai->cpu_dai->private_data == NULL) {
>
> It seems a bit icky to be managing the DAI private data here...
>
>> +             err = request_irq(priv->irq, kirkwood_dma_irq, IRQF_SHARED,
>> +                               "kirkwood-dma", prdata);
>
> I'd suggest "kirkwood-i2s" as a name here.  Or take the name from the
> DAI.
>
>> +static int kirkwood_dma_trigger(struct snd_pcm_substream *substream, int cmd)
>> +{
>> +     return 0;
>> +}
>
> Remove this if it's not implemented.
>
>> +static int kirkwood_dma_mmap(struct snd_pcm_substream *substream,
>> +             struct vm_area_struct *vma)
>> +{
>> +     struct snd_pcm_runtime *runtime = substream->runtime;
>> +
>> +     return dma_mmap_coherent(substream->pcm->card->dev, vma,
>> +                                     runtime->dma_area,
>> +                                     runtime->dma_addr,
>> +                                     runtime->dma_bytes);
>> +}
>
> Hrm, this looks like there should be a utility function to do it, it's
> not terribly driver specific.
>
>> +struct snd_soc_platform kirkwood_soc_platform = {
>> +     .name           = "kirkwood-dma",
>
> I'd also add an audio in here or something.
>
>> +#define AUDIO_WIN_BASE_REG(win)                      (0xA00 + ((win)<<3))
>> +#define AUDIO_WIN_CTRL_REG(win)                      (0xA04 + ((win)<<3))
>
> Namespacing.  Or move into the driver, there seems little reason for
> anything else to be peering at these.
>
>> +#define KIRKWOOD_RATES \
>> +     (SNDRV_PCM_RATE_44100 | \
>> +      SNDRV_PCM_RATE_48000 | SNDRV_PCM_RATE_96000)
>> +#define KIRKWOOD_FORMATS \
>> +     (SNDRV_PCM_FMTBIT_S16_LE | \
>> +      SNDRV_PCM_FMTBIT_S24_LE | \
>> +      SNDRV_PCM_FMTBIT_S32_LE)
>
> Move these into the driver.
>
>> +static inline void kirkwood_set_dco(void __iomem *io, unsigned long rate)
>> +{
>> +     unsigned long value;
>> +
>> +     value = KIRKWOOD_DCO_CTL_OFFSET_0;
>> +     switch (rate) {
>> +     default:
>> +     case 44100:
>> +             value |= KIRKWOOD_DCO_CTL_FREQ_11;
>> +             break;
>> +     case 48000:
>> +             value |= KIRKWOOD_DCO_CTL_FREQ_12;
>> +             break;
>> +     case 96000:
>> +             value |= KIRKWOOD_DCO_CTL_FREQ_24;
>> +             break;
>> +     }
>> +     writel(value, io + KIRKWOOD_DCO_CTL);
>> +
>> +     /* wait for dco locked */
>> +     do {
>> +             cpu_relax();
>> +             value = readl(io + KIRKWOOD_DCO_SPCR_STATUS);
>> +             value &= KIRKWOOD_DCO_SPCR_STATUS;
>> +     } while (value == 0);
>> +}
>
> Why is this an inline function in the header?
>
>> +      * Size settings in play/rec i2s control regs and play/rec control
>> +      * regs must be the same.
>> +      */
>
> Ideally you'd be setting up constraints for this.
>
>> +     /*
>> +      * specs says KIRKWOOD_PLAYCTL must be read 2 times before
>> +      * changing it. So read 1 time here and 1 later.
>> +      */
>> +     value = readl(priv->io + KIRKWOOD_PLAYCTL);
>
> Nice...
>
>> +     switch (cmd) {
>> +     case SNDRV_PCM_TRIGGER_START:
>> +             /* stop audio, enable interrupts */
>
> If audio is running when you're getting a start trigger something is
> wrong...
>
>> +static int kirkwood_i2s_set_sysclk(struct snd_soc_dai *cpu_dai,
>> +                             int clk_id, unsigned int freq, int dir)
>> +{
>> +     return 0;
>> +}
>
> Remove this.
>
>> +static void mv_audio_init(void __iomem *base)
>> +{
>> +     int timeout = 10000000;
>> +     unsigned int reg_data;
>> +
>> +     reg_data = readl(base + 0x1200);
>> +     reg_data &= (~(0x333FF8));
>> +     reg_data |= 0x111D18;
>> +     writel(reg_data, base + 0x1200);
>
> It's like magic!
>
>> +
>> +     do {
>> +             timeout--;
>> +     } while (timeout);
>
> The driver is busy waiting here but not polling anything.  Just use a
> sleep?
>
>> +     /* FIXME : should not be hardcoded */
>> +     priv->burst = 128;
>
> Platform data?
>
>> +static int kirkwood_i2s_probe(struct platform_device *pdev,
>> +                            struct snd_soc_dai *dai)
>
> This is the ASoC probe but...
>
> +       mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>
> ...it's pulling all sorts of resources out of the device which are
> specific to the I2S controller.  You should have the I2S controller have
> a normal platform device which is probed on system startup and triggers
> DAI registration, just as you're doing in the CODEC driver.  All the
> resources should be there.  Apart from anything else this means you only
> need to set up each I2S controller once in the arch/arm code for the
> CPU.
>
>> +     mv_audio_init(priv->io);
>
> Seems a bit odd ot have the separate init function in the middle of a
> bunch of other register writes.
>
>> +     dev_info(&pdev->dev, "kirkwood-i2s driver loaded\n");
>
> May as well remove this.  The core logs device registration anyway.
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
>
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

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

[Mark Brown]

On Wed, May 12, 2010 at 01:48:57PM +0300, saeed bishara wrote:

Please don't top post.

> 	some of the dma stuff handled by the i2s! For example the i2s trigger
> function actually enables the dma, this should be moved to the dma
> driver. Also, the playback and capture trigger funtions looks alike

There's no real problem with doing this from an ASoC point of view, both
devices are part of the same chip so they're free to peer into each
other's implementations as much as makes sense for that hardware.

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

[Arnaud Patard]

Mark Brown <broonie@opensource.wolfsonmicro.com> writes:

> On Tue, May 11, 2010 at 06:23:47PM +0200, apatard@mandriva.com wrote:
>> This patch enables support for the i2s codec available on kirkwood platforms
>
> It's not a CODEC, it's the I2S controller in the CPU.  A CODEC is a
> device with DACs and ADCs in it.
>
>> +	do {
>> +		/* we've enabled only bytes interrupts ... */
>
> You were checking for error interrupts further up the function?
>
>> +		if (status & ~(KIRKWOOD_INT_CAUSE_PLAY_BYTES | \
>> +				KIRKWOOD_INT_CAUSE_REC_BYTES))
>> +			return IRQ_NONE;
>
> Surely it'd make sense to log the unexpected interrupts for diagnostics?
> You should never get here if there are no interrupts you've enabled.
> Unless the interrupt is shared, but that'd be a bit surprising for a
> SoC.

it's not shared iirc but there are different possible interrupt
cause. There used to be a message but I think it has been removed by
error. Should I had it back or remove the check ? (I prefer adding it
back but your point of view does matter).

>
> It also looks like it'd be more sensible to either drop the do/while
> loop entirely (the IRQ core should handle the interrupt still being
> asserted and it doesn't look like any of the handling should take so
> long that interrupts reassert while it's running anyway) or move the
> handling of error interrupts and the first read into the loop so you
> don't have two slightly different paths.

The loop is there to handle playback and record interrupts at the same
time. As regards the error interrupts, I preferred to put outside the
loop to differentiate "normal" interrupts from "error" interrupts.

>
>> +		/* ack int */
>> +		writel(status, priv->io + KIRKWOOD_INT_CAUSE);
>> +
>> +		if (status & KIRKWOOD_INT_CAUSE_PLAY_BYTES)
>> +			snd_pcm_period_elapsed(prdata->play_stream);
>> +
>> +		if (status & KIRKWOOD_INT_CAUSE_REC_BYTES)
>> +			snd_pcm_period_elapsed(prdata->rec_stream);
>> +
>> +		mask = readl(priv->io + KIRKWOOD_INT_MASK);
>> +		status = readl(priv->io + KIRKWOOD_INT_CAUSE) & mask;
>
> This masking didn't happen prior to the first entry into the loop.

Can you explain a little bit more ? at the beginning of the function,
there are the 2 same lines.

>
>> +       if (soc_runtime->dai->cpu_dai->private_data == NULL) {
>
> It seems a bit icky to be managing the DAI private data here...
>
>> +		err = request_irq(priv->irq, kirkwood_dma_irq, IRQF_SHARED,
>> +				  "kirkwood-dma", prdata);
>
> I'd suggest "kirkwood-i2s" as a name here.  Or take the name from the
> DAI.

ok

>
>> +static int kirkwood_dma_trigger(struct snd_pcm_substream *substream, int cmd)
>> +{
>> +	return 0;
>> +}
>
> Remove this if it's not implemented.
>
>> +static int kirkwood_dma_mmap(struct snd_pcm_substream *substream,
>> +		struct vm_area_struct *vma)
>> +{
>> +	struct snd_pcm_runtime *runtime = substream->runtime;
>> +
>> +	return dma_mmap_coherent(substream->pcm->card->dev, vma,
>> +					runtime->dma_area,
>> +					runtime->dma_addr,
>> +					runtime->dma_bytes);
>> +}
>
> Hrm, this looks like there should be a utility function to do it, it's
> not terribly driver specific.

hmm... I need to double check. I got some troubles with this kind of
stuff on my ST LS2E/F mips platforms, so I wrote something known to be
working.

>
>> +struct snd_soc_platform kirkwood_soc_platform = {
>> +	.name		= "kirkwood-dma",
>
> I'd also add an audio in here or something.
>
>> +#define AUDIO_WIN_BASE_REG(win)			(0xA00 + ((win)<<3))
>> +#define AUDIO_WIN_CTRL_REG(win)			(0xA04 + ((win)<<3))
>
> Namespacing.  Or move into the driver, there seems little reason for
> anything else to be peering at these.
>
>> +#define KIRKWOOD_RATES \
>> +	(SNDRV_PCM_RATE_44100 | \
>> +	 SNDRV_PCM_RATE_48000 | SNDRV_PCM_RATE_96000)
>> +#define KIRKWOOD_FORMATS \
>> +	(SNDRV_PCM_FMTBIT_S16_LE | \
>> +	 SNDRV_PCM_FMTBIT_S24_LE | \
>> +	 SNDRV_PCM_FMTBIT_S32_LE)
>
> Move these into the driver.
>
>> +static inline void kirkwood_set_dco(void __iomem *io, unsigned long rate)
>> +{
>> +	unsigned long value;
>> +
>> +	value = KIRKWOOD_DCO_CTL_OFFSET_0;
>> +	switch (rate) {
>> +	default:
>> +	case 44100:
>> +		value |= KIRKWOOD_DCO_CTL_FREQ_11;
>> +		break;
>> +	case 48000:
>> +		value |= KIRKWOOD_DCO_CTL_FREQ_12;
>> +		break;
>> +	case 96000:
>> +		value |= KIRKWOOD_DCO_CTL_FREQ_24;
>> +		break;
>> +	}
>> +	writel(value, io + KIRKWOOD_DCO_CTL);
>> +
>> +	/* wait for dco locked */
>> +	do {
>> +		cpu_relax();
>> +		value = readl(io + KIRKWOOD_DCO_SPCR_STATUS);
>> +		value &= KIRKWOOD_DCO_SPCR_STATUS;
>> +	} while (value == 0);
>> +}
>
> Why is this an inline function in the header?
>
>> +	 * Size settings in play/rec i2s control regs and play/rec control
>> +	 * regs must be the same.
>> +	 */
>
> Ideally you'd be setting up constraints for this.
>
>> +	/*
>> +	 * specs says KIRKWOOD_PLAYCTL must be read 2 times before
>> +	 * changing it. So read 1 time here and 1 later.
>> +	 */
>> +	value = readl(priv->io + KIRKWOOD_PLAYCTL);
>
> Nice...
>
>> +	switch (cmd) {
>> +	case SNDRV_PCM_TRIGGER_START:
>> +		/* stop audio, enable interrupts */
>
> If audio is running when you're getting a start trigger something is
> wrong...

I'm too paranoid ? here

>
>> +static int kirkwood_i2s_set_sysclk(struct snd_soc_dai *cpu_dai,
>> +				int clk_id, unsigned int freq, int dir)
>> +{
>> +	return 0;
>> +}
>
> Remove this.
>
>> +static void mv_audio_init(void __iomem *base)
>> +{
>> +	int timeout = 10000000;
>> +	unsigned int reg_data;
>> +
>> +	reg_data = readl(base + 0x1200);
>> +	reg_data &= (~(0x333FF8));
>> +	reg_data |= 0x111D18;
>> +	writel(reg_data, base + 0x1200);
>
> It's like magic!

yeah. I've not been able to find out any info in the specs about that
and last time I tried with that it was not working :(

>
>> +
>> +	do {
>> +		timeout--;
>> +	} while (timeout);
>
> The driver is busy waiting here but not polling anything.  Just use a
> sleep?
>
>> +	/* FIXME : should not be hardcoded */
>> +	priv->burst = 128;
>
> Platform data?
>
>> +static int kirkwood_i2s_probe(struct platform_device *pdev,
>> +                            struct snd_soc_dai *dai)
>
> This is the ASoC probe but...
>
> +       mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>
> ...it's pulling all sorts of resources out of the device which are
> specific to the I2S controller.  You should have the I2S controller have
> a normal platform device which is probed on system startup and triggers
> DAI registration, just as you're doing in the CODEC driver.  All the
> resources should be there.  Apart from anything else this means you only
> need to set up each I2S controller once in the arch/arm code for the
> CPU.

ok. Will look at that stuff.

>
>> +	mv_audio_init(priv->io);
>
> Seems a bit odd ot have the separate init function in the middle of a
> bunch of other register writes.

I want to have interrupts disable before calling it. I don't want bad
suprises.


Arnaud

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

[Mark Brown]

On Wed, May 12, 2010 at 04:38:25PM +0200, Arnaud Patard wrote:
> Mark Brown <broonie@opensource.wolfsonmicro.com> writes:
> > On Tue, May 11, 2010 at 06:23:47PM +0200, apatard@mandriva.com wrote:

> >> +		if (status & ~(KIRKWOOD_INT_CAUSE_PLAY_BYTES | \
> >> +				KIRKWOOD_INT_CAUSE_REC_BYTES))
> >> +			return IRQ_NONE;

> > Surely it'd make sense to log the unexpected interrupts for diagnostics?
> > You should never get here if there are no interrupts you've enabled.
> > Unless the interrupt is shared, but that'd be a bit surprising for a
> > SoC.

> it's not shared iirc but there are different possible interrupt
> cause. There used to be a message but I think it has been removed by
> error. Should I had it back or remove the check ? (I prefer adding it
> back but your point of view does matter).

Add it back and also log it, but also fix the IRQ_NONE thing - it's
defintiely going to be wrong if this is the second spin through the loop
for example.

> > It also looks like it'd be more sensible to either drop the do/while
> > loop entirely (the IRQ core should handle the interrupt still being
> > asserted and it doesn't look like any of the handling should take so
> > long that interrupts reassert while it's running anyway) or move the
> > handling of error interrupts and the first read into the loop so you
> > don't have two slightly different paths.

> The loop is there to handle playback and record interrupts at the same

Why is this required?  You check both interrupts in sequence anyway.

> time. As regards the error interrupts, I preferred to put outside the
> loop to differentiate "normal" interrupts from "error" interrupts.

Actually looking again they are in a separate status register too so I
suppose that's not quite so bad.  It is a bit odd that they all come in
on the same status line and can't be muxed out from a single register
but hardware designers are like that sometimes.

> >> +		mask = readl(priv->io + KIRKWOOD_INT_MASK);
> >> +		status = readl(priv->io + KIRKWOOD_INT_CAUSE) & mask;

> > This masking didn't happen prior to the first entry into the loop.

> Can you explain a little bit more ? at the beginning of the function,
> there are the 2 same lines.

I was looking at the read of cause there, I hadn't noticed the separate
status register.

> >> +static int kirkwood_dma_mmap(struct snd_pcm_substream *substream,
> >> +		struct vm_area_struct *vma)
> >> +{
> >> +	struct snd_pcm_runtime *runtime = substream->runtime;
> >> +
> >> +	return dma_mmap_coherent(substream->pcm->card->dev, vma,
> >> +					runtime->dma_area,
> >> +					runtime->dma_addr,
> >> +					runtime->dma_bytes);
> >> +}

> > Hrm, this looks like there should be a utility function to do it, it's
> > not terribly driver specific.

> hmm... I need to double check. I got some troubles with this kind of
> stuff on my ST LS2E/F mips platforms, so I wrote something known to be
> working.

The point is that it looks like either there should already be an
implementation of this function you can use or this should be put
somewhere were other folks are able to use it.

> >> +	mv_audio_init(priv->io);

> > Seems a bit odd ot have the separate init function in the middle of a
> > bunch of other register writes.

> I want to have interrupts disable before calling it. I don't want bad
> suprises.

My point is that you may as well just have the mv_audio_init() function
in line in the code here, jumping out into another function seems a bit
odd when all it is is more register writes.

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

[apatard]

[-- Attachment #1: openrd_audio.patch --]
[-- Type: text/plain, Size: 6319 bytes --]

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>
---
 sound/soc/kirkwood/Kconfig           |    9 	9 +	0 -	0 !
 sound/soc/kirkwood/Makefile          |    3 	3 +	0 -	0 !
 sound/soc/kirkwood/kirkwood-openrd.c |  176 	176 +	0 -	0 !
 3 files changed, 188 insertions(+)

Index: linux-2.6.33/sound/soc/kirkwood/Kconfig
===================================================================
--- linux-2.6.33.orig/sound/soc/kirkwood/Kconfig	2010-05-11 18:05:14.981650112 +0200
+++ linux-2.6.33/sound/soc/kirkwood/Kconfig	2010-05-11 18:11:50.097650543 +0200
@@ -9,3 +9,12 @@ config SND_KIRKWOOD_SOC
 config SND_KIRKWOOD_SOC_I2S
 	tristate
 
+config SND_KIRKWOOD_SOC_OPENRD
+	tristate "SoC Audio support for Kirkwood Openrd Client"
+	depends on SND_KIRKWOOD_SOC && MACH_OPENRD_CLIENT
+	select SND_KIRKWOOD_SOC_I2S
+	select SND_SOC_CS42L51
+	help
+	  Say Y if you want to add support for SoC audio on
+	  Openrd Client.
+
Index: linux-2.6.33/sound/soc/kirkwood/Makefile
===================================================================
--- linux-2.6.33.orig/sound/soc/kirkwood/Makefile	2010-05-11 18:05:14.985650562 +0200
+++ linux-2.6.33/sound/soc/kirkwood/Makefile	2010-05-11 18:11:50.097650543 +0200
@@ -4,3 +4,6 @@ snd-soc-kirkwood-i2s-objs := kirkwood-i2
 obj-$(CONFIG_SND_KIRKWOOD_SOC) += snd-soc-kirkwood.o
 obj-$(CONFIG_SND_KIRKWOOD_SOC_I2S) += snd-soc-kirkwood-i2s.o
 
+snd-soc-openrd-objs := kirkwood-openrd.o
+
+obj-$(CONFIG_SND_KIRKWOOD_SOC_OPENRD) += snd-soc-openrd.o
Index: linux-2.6.33/sound/soc/kirkwood/kirkwood-openrd.c
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ linux-2.6.33/sound/soc/kirkwood/kirkwood-openrd.c	2010-05-11 18:11:50.101650416 +0200
@@ -0,0 +1,176 @@
+/*
+ * 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/i2c.h>
+#include <linux/slab.h>
+#include <sound/soc.h>
+#include <mach/kirkwood.h>
+#include <mach/audio.h>
+#include <asm/mach-types.h>
+#include "kirkwood-i2s.h"
+#include "kirkwood-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 = &kirkwood_i2s_dai,
+	.codec_dai = &cs42l51_dai,
+	.ops = &openrd_client_ops,
+},
+};
+
+
+static struct snd_soc_card openrd_client = {
+	.name = "OpenRD Client",
+	.platform = &kirkwood_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 struct resource kirkwood_audio_resources[] = {
+	[0] = {
+		.start  = AUDIO_PHYS_BASE,
+		.end    = AUDIO_PHYS_BASE + SZ_16K - 1,
+		.flags  = IORESOURCE_MEM,
+	},
+	[1] = {
+		.start  = IRQ_KIRKWOOD_I2S,
+		.end    = IRQ_KIRKWOOD_I2S,
+		.flags  = IORESOURCE_IRQ,
+	},
+};
+
+static struct i2c_board_info i2c_board_info[] = {
+	{
+		I2C_BOARD_INFO("cs42l51", 0x4a),
+	},
+};
+
+static struct i2c_client *cs42l51_client;
+static int __init openrd_client_init(void)
+{
+	int ret;
+	struct kirkwood_soc_platform_data *data;
+	struct i2c_adapter *adapter;
+
+	if (!machine_is_openrd_client())
+		return 0;
+
+	adapter = i2c_get_adapter(0);
+	if (!adapter)
+		return -ENODEV;
+
+	cs42l51_client = i2c_new_device(i2c_get_adapter(0), i2c_board_info);
+	i2c_put_adapter(adapter);
+	if (!cs42l51_client)
+		return -ENODEV;
+
+	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;
+
+	data = kzalloc(sizeof(struct kirkwood_soc_platform_data), GFP_KERNEL);
+	if (!data) {
+		printk(KERN_ERR "%s: can't alloc data\n", __func__);
+		return -ENOMEM;
+	}
+	memcpy(data, &kirkwood_soc_data,
+		sizeof(struct kirkwood_soc_platform_data));
+	openrd_client_snd_device->dev.platform_data = data;
+
+	ret = platform_device_add_resources(openrd_client_snd_device,
+					kirkwood_audio_resources,
+					ARRAY_SIZE(kirkwood_audio_resources));
+	if (ret) {
+		printk(KERN_ERR "%s: Failed to add ressources\n", __func__);
+		return ret;
+	}
+
+	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);
+	i2c_unregister_device(cs42l51_client);
+}
+
+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");
+

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

[Mark Brown]

On Tue, May 11, 2010 at 06:23:48PM +0200, apatard@mandriva.com wrote:

> +	adapter = i2c_get_adapter(0);
> +	if (!adapter)
> +		return -ENODEV;
> +
> +	cs42l51_client = i2c_new_device(i2c_get_adapter(0), i2c_board_info);
> +	i2c_put_adapter(adapter);
> +	if (!cs42l51_client)
> +		return -ENODEV;

This should be in the arch/arm code.  There's no reason to dynamically
add this at runtime.

> +	ret = platform_device_add_resources(openrd_client_snd_device,
> +					kirkwood_audio_resources,
> +					ARRAY_SIZE(kirkwood_audio_resources));
> +	if (ret) {
> +		printk(KERN_ERR "%s: Failed to add ressources\n", __func__);
> +		return ret;
> +	}
> +
> +	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);
> +	}

As I said when reviewing the CPU drivers these should be part of the
setup of the I2S device in arch/arm.

Re: [patch 0/6] kirkwood openrd client audio support

[saeed bishara]

 <apatard <at> mandriva.com> writes:

> 
> 
> This patchset aims at adding ASoC support to ARM kirkwood systems and
> more precisely to openrd client boards.
> 
> Arnaud Patard
> 

Hi Arnaud,
   this i2s controller was first added into the Orion-1-90 (88F6183), and also
it's found on dove soc. so can you please rename to orion to make more generic?

saeed