[alsa-devel] [PATCH] Add driver for Analog Devices ADAU1701 SigmaDSP
From: Cliff Cai cliff.cai@analog.com
ADAU1701 is an SigmaDSP processor,it supports I2S audio interface. It needs to include "linux/sigma.h" which is still in Andrew Morton's tree.
Signed-off-by: Cliff Caicliff.cai@analog.com --- sound/soc/codecs/Kconfig | 4 + sound/soc/codecs/Makefile | 2 + sound/soc/codecs/adau1701.c | 417 +++++++++++++++++++++++++++++++++++++++++++ sound/soc/codecs/adau1701.h | 111 ++++++++++++ 4 files changed, 534 insertions(+), 0 deletions(-) create mode 100644 sound/soc/codecs/adau1701.c create mode 100644 sound/soc/codecs/adau1701.h
diff --git a/sound/soc/codecs/Kconfig b/sound/soc/codecs/Kconfig index bbc97fd..ba931c4 100644 --- a/sound/soc/codecs/Kconfig +++ b/sound/soc/codecs/Kconfig @@ -20,6 +20,7 @@ config SND_SOC_ALL_CODECS select SND_SOC_PCM3008 select SND_SOC_SPDIF select SND_SOC_SSM2602 if I2C + select SND_SOC_ADAU1701 if I2C select SND_SOC_STAC9766 if SND_SOC_AC97_BUS select SND_SOC_TLV320AIC23 if I2C select SND_SOC_TLV320AIC26 if SPI_MASTER @@ -98,6 +99,9 @@ config SND_SOC_SPDIF config SND_SOC_SSM2602 tristate
+config SND_SOC_ADAU1701 + tristate + config SND_SOC_STAC9766 tristate
diff --git a/sound/soc/codecs/Makefile b/sound/soc/codecs/Makefile index 8b75305..ed48581 100644 --- a/sound/soc/codecs/Makefile +++ b/sound/soc/codecs/Makefile @@ -8,6 +8,7 @@ snd-soc-l3-objs := l3.o snd-soc-pcm3008-objs := pcm3008.o snd-soc-spdif-objs := spdif_transciever.o snd-soc-ssm2602-objs := ssm2602.o +snd-soc-adau1701-objs := adau1701.o snd-soc-stac9766-objs := stac9766.o snd-soc-tlv320aic23-objs := tlv320aic23.o snd-soc-tlv320aic26-objs := tlv320aic26.o @@ -45,6 +46,7 @@ obj-$(CONFIG_SND_SOC_L3) += snd-soc-l3.o obj-$(CONFIG_SND_SOC_PCM3008) += snd-soc-pcm3008.o obj-$(CONFIG_SND_SOC_SPDIF) += snd-soc-spdif.o obj-$(CONFIG_SND_SOC_SSM2602) += snd-soc-ssm2602.o +obj-$(CONFIG_SND_SOC_ADAU1701) += snd-soc-adau1701.o obj-$(CONFIG_SND_SOC_STAC9766) += snd-soc-stac9766.o obj-$(CONFIG_SND_SOC_TLV320AIC23) += snd-soc-tlv320aic23.o obj-$(CONFIG_SND_SOC_TLV320AIC26) += snd-soc-tlv320aic26.o diff --git a/sound/soc/codecs/adau1701.c b/sound/soc/codecs/adau1701.c new file mode 100644 index 0000000..b7c671d --- /dev/null +++ b/sound/soc/codecs/adau1701.c @@ -0,0 +1,417 @@ +/* + * Driver for ADAU1701 SigmaDSP processor + * + * Copyright 2011 Analog Devices Inc. + * + * Licensed under the GPL-2 or later. + */ + +#include <linux/module.h> +#include <linux/moduleparam.h> +#include <linux/init.h> +#include <linux/delay.h> +#include <linux/pm.h> +#include <linux/i2c.h> +#include <linux/workqueue.h> +#include <linux/platform_device.h> +#include <linux/sigma.h> +#include <linux/sysfs.h> +#include <linux/slab.h> +#include <sound/core.h> +#include <sound/pcm.h> +#include <sound/pcm_params.h> +#include <sound/soc.h> +#include <sound/soc-dapm.h> +#include <sound/initval.h> + +#include "adau1701.h" + +#define AUDIO_NAME "adau1701" +#define ADAU1701_VERSION "0.10" +#define ADAU1701_FIRMWARE "SigmaDSP_fw.bin" + +/* codec private data */ +struct adau1701_priv { + struct snd_soc_codec *codec; + enum snd_soc_control_type control_type; +}; + +/* + * Write a ADAU1701 register,since the register length is from 1 to 5, + * So, use our own read/write functions instead of snd_soc_read/write. + */ +static int adau1701_write_register(struct snd_soc_codec *codec, + u16 reg_address, u8 length, u32 value) +{ + int ret; + int count = length + 2; /*data plus 16bit register address*/ + u8 buf[8] = {0, 0, 0, 0, 0, 0, 0, 0}; + + if (length == 0) + return -1; + buf[0] = (reg_address >> 8) & 0xFF; + buf[1] = reg_address & 0xFF; + if (length == 1) + buf[2] = value & 0xFF; + else if (length == 2) { + buf[2] = (value >> 8) & 0xFF; + buf[3] = value & 0xFF; + } else if (length == 3) { + buf[2] = (value >> 16) & 0xFF; + buf[3] = (value >> 8) & 0xFF; + buf[4] = value & 0xFF; + } + ret = i2c_master_send(codec->control_data, buf, count); + + return ret; + +} + +/* + * read ADAU1701 hw register + */ +static u32 adau1701_read_register(struct snd_soc_codec *codec, + u16 reg_address, u8 length) +{ + u8 addr[2]; + u8 buf[2]; + u32 value = 0; + int ret; + + if (reg_address < ADAU1701_FIRSTREG) + reg_address = reg_address + ADAU1701_FIRSTREG; + + if ((reg_address < ADAU1701_FIRSTREG) || (reg_address > ADAU1701_LASTREG)) + return -EIO; + + addr[0] = (reg_address >> 8) & 0xFF; + addr[1] = reg_address & 0xFF; + + /* write the 2byte read address */ + ret = i2c_master_send(codec->control_data, addr, 2); + if (ret) + return ret; + + if (length == 1) { + if (i2c_master_recv(codec->control_data, buf, 1) != 1) + return -EIO; + value = buf[0]; + } else if (length == 2) { + if (i2c_master_recv(codec->control_data, buf, 2) != 2) + return -EIO; + value = (buf[0] << 8) | buf[1]; + } + return value; +} + +static int adau1701_setprogram(struct snd_soc_codec *codec) +{ + int ret = 0; + + ret = process_sigma_firmware(codec->control_data, ADAU1701_FIRMWARE); + + return ret; +} + +static int adau1701_pcm_prepare(struct snd_pcm_substream *substream, + struct snd_soc_dai *dai) +{ + struct snd_soc_codec *codec = dai->codec; + int reg = 0; + + reg = SEROCTL_MASTER | SEROCTL_OBF16 | SEROCTL_OLF1024; + adau1701_write_register(codec, ADAU1701_SEROCTL, 2, reg); + + return 0; +} + +static void adau1701_shutdown(struct snd_pcm_substream *substream, + struct snd_soc_dai *dai) +{ + struct snd_soc_codec *codec = dai->codec; + + adau1701_write_register(codec, ADAU1701_SEROCTL, 2, 0); +} + +static int adau1701_mute(struct snd_soc_dai *dai, int mute) +{ + struct snd_soc_codec *codec = dai->codec; + u16 reg = 0; + + if (mute) { + /* mute inputs/outputs */ + reg = adau1701_read_register(codec, ADAU1701_AUXNPOW, 2); + reg |= AUXNPOW_AAPD | AUXNPOW_D0PD | AUXNPOW_D1PD | AUXNPOW_D2PD | AUXNPOW_D3PD; + adau1701_write_register(codec, ADAU1701_AUXNPOW, 2, reg); + } else { + /* unmute inputs/outputs */ + reg = adau1701_read_register(codec, ADAU1701_AUXNPOW, 2); + reg &= ~(AUXNPOW_AAPD | AUXNPOW_D0PD | AUXNPOW_D1PD | AUXNPOW_D2PD | AUXNPOW_D3PD); + adau1701_write_register(codec, ADAU1701_AUXNPOW, 2, reg); + } + + return 0; +} + +static int adau1701_set_dai_fmt(struct snd_soc_dai *codec_dai, + unsigned int fmt) +{ + struct snd_soc_codec *codec = codec_dai->codec; + u32 reg = 0; + + reg = adau1701_read_register(codec, ADAU1701_SERITL1, 1); + /* interface format */ + switch (fmt & SND_SOC_DAIFMT_FORMAT_MASK) { + case SND_SOC_DAIFMT_I2S: + break; + case SND_SOC_DAIFMT_LEFT_J: + reg |= SERITL1_LEFTJ; + break; + /* TODO: support TDM */ + default: + return 0; + } + + /* clock inversion */ + switch (fmt & SND_SOC_DAIFMT_INV_MASK) { + case SND_SOC_DAIFMT_NB_NF: + break; + /* TODO: support signal inversions */ + default: + return 0; + } + + /* set iface format*/ + adau1701_write_register(codec, ADAU1701_SERITL1, 1, reg); + return 0; +} + +static int adau1701_set_bias_level(struct snd_soc_codec *codec, + enum snd_soc_bias_level level) +{ + u16 reg; + switch (level) { + case SND_SOC_BIAS_ON: + reg = adau1701_read_register(codec, ADAU1701_AUXNPOW, 2); + reg &= ~(AUXNPOW_AAPD | AUXNPOW_D0PD | AUXNPOW_D1PD | AUXNPOW_D2PD | + AUXNPOW_D3PD | AUXNPOW_VBPD | AUXNPOW_VRPD); + adau1701_write_register(codec, ADAU1701_AUXNPOW, 2, reg); + break; + case SND_SOC_BIAS_PREPARE: + break; + case SND_SOC_BIAS_STANDBY: + break; + case SND_SOC_BIAS_OFF: + /* everything off, dac mute, inactive */ + reg = adau1701_read_register(codec, ADAU1701_AUXNPOW, 2); + reg |= AUXNPOW_AAPD | AUXNPOW_D0PD | AUXNPOW_D1PD | AUXNPOW_D2PD | + AUXNPOW_D3PD | AUXNPOW_VBPD | AUXNPOW_VRPD; + adau1701_write_register(codec, ADAU1701_AUXNPOW, 2, reg); + break; + + } + codec->bias_level = level; + return 0; +} + +#define ADAU1701_RATES SNDRV_PCM_RATE_48000 + +#define ADAU1701_FORMATS (SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S20_3LE |\ + SNDRV_PCM_FMTBIT_S24_LE) + +static struct snd_soc_dai_ops adau1701_dai_ops = { + .prepare = adau1701_pcm_prepare, + .shutdown = adau1701_shutdown, + .digital_mute = adau1701_mute, + .set_fmt = adau1701_set_dai_fmt, +}; + +struct snd_soc_dai_driver adau1701_dai = { + .name = "ADAU1701", + .playback = { + .stream_name = "Playback", + .channels_min = 2, + .channels_max = 2, + .rates = ADAU1701_RATES, + .formats = ADAU1701_FORMATS, + }, + .capture = { + .stream_name = "Capture", + .channels_min = 2, + .channels_max = 2, + .rates = ADAU1701_RATES, + .formats = ADAU1701_FORMATS, + }, + .ops = &adau1701_dai_ops, +}; +EXPORT_SYMBOL_GPL(adau1701_dai); + +static int adau1701_suspend(struct snd_soc_codec *codec, pm_message_t state) +{ + adau1701_set_bias_level(codec, SND_SOC_BIAS_OFF); + return 0; +} + +static int adau1701_resume(struct snd_soc_codec *codec) +{ + adau1701_set_bias_level(codec, SND_SOC_BIAS_STANDBY); + return 0; +} + +static ssize_t adau1371_dsp_load(struct device *dev, + struct device_attribute *attr, + const char *buf, size_t count) +{ + int ret = 0; + struct adau1701_priv *adau1701 = dev_get_drvdata(dev); + struct snd_soc_codec *codec = adau1701->codec; + ret = adau1701_setprogram(codec); + if (ret) + return ret; + else + return count; +} +static DEVICE_ATTR(dsp, 0644, NULL, adau1371_dsp_load); + +static int adau1701_reg_init(struct snd_soc_codec *codec) +{ + u32 reg; + int ret = 0; + + reg = DSPCTRL_DAM | DSPCTRL_ADM; + adau1701_write_register(codec, ADAU1701_DSPCTRL, 2, reg); + /* Load default program */ + ret = adau1701_setprogram(codec); + if (ret < 0) { + printk(KERN_ERR "Loading program data failed\n"); + goto error; + } + reg = DSPCTRL_DAM | DSPCTRL_ADM; + adau1701_write_register(codec, ADAU1701_DSPCTRL, 2, reg); + reg = 0x08; + adau1701_write_register(codec, ADAU1701_DSPRES, 1, reg); + adau1701_write_register(codec, ADAU1701_SEROCTL, 2, 0); + adau1701_write_register(codec, ADAU1701_SERITL1, 1, 0); + /* Configure the multipurpose pins as serial in/out pins */ + reg = MPCONF_SDATAP | MPCONF_SDATAP << 16 | MPCONF_SDATAP << 20; + adau1701_write_register(codec, ADAU1701_MPCONF0, 3, reg); + reg = MPCONF_AUXADC << 8 | MPCONF_SDATAP << 12 | MPCONF_SDATAP << 16 | + MPCONF_SDATAP << 20; + adau1701_write_register(codec, ADAU1701_MPCONF1, 3, reg); + adau1701_write_register(codec, ADAU1701_AUXNPOW, 2, 0); + reg = AUXADCE_AAEN; + adau1701_write_register(codec, ADAU1701_AUXADCE, 2, reg); + reg = DACSET_DACEN; + adau1701_write_register(codec, ADAU1701_DACSET, 2, reg); + reg = DSPCTRL_DAM | DSPCTRL_ADM | DSPCTRL_CR; + adau1701_write_register(codec, ADAU1701_DSPCTRL, 2, reg); + /* Power-up the oscillator */ + adau1701_write_register(codec, ADAU1701_OSCIPOW, 2, 0); +error: + return ret; +} + +static int adau1701_probe(struct snd_soc_codec *codec) +{ + int ret = 0; + + struct adau1701_priv *adau1701 = snd_soc_codec_get_drvdata(codec); + + adau1701->codec = codec; + ret = snd_soc_codec_set_cache_io(codec, 16, 16, adau1701->control_type); + if (ret < 0) { + dev_err(codec->dev, "Failed to set cache I/O: %d\n", ret); + return ret; + } + ret = adau1701_reg_init(codec); + if (ret < 0) { + dev_err(codec->dev, "failed to initialize\n"); + return ret; + } + ret = device_create_file(codec->dev, &dev_attr_dsp); + if (ret) + dev_err(codec->dev, "device_create_file() failed\n"); + + return ret; +} + +static int adau1701_remove(struct snd_soc_codec *codec) +{ + adau1701_set_bias_level(codec, SND_SOC_BIAS_OFF); + return 0; +} + +struct snd_soc_codec_driver soc_codec_dev_adau1701 = { + .probe = adau1701_probe, + .remove = adau1701_remove, + .suspend = adau1701_suspend, + .resume = adau1701_resume, + .set_bias_level = adau1701_set_bias_level, +}; + +static __devinit int adau1701_i2c_probe(struct i2c_client *i2c, + const struct i2c_device_id *id) +{ + struct adau1701_priv *adau1701; + int ret = 0; + + adau1701 = kzalloc(sizeof(struct adau1701_priv), GFP_KERNEL); + if (adau1701 == NULL) + return -ENOMEM; + + adau1701->control_type = SND_SOC_I2C; + i2c_set_clientdata(i2c, adau1701); + ret = snd_soc_register_codec(&i2c->dev, &soc_codec_dev_adau1701, &adau1701_dai, 1); + if (ret < 0) + kfree(adau1701); + + return ret; +} + +static __devexit int adau1701_i2c_remove(struct i2c_client *client) +{ + snd_soc_unregister_codec(&client->dev); + kfree(i2c_get_clientdata(client)); + return 0; +} + +static const struct i2c_device_id adau1701_i2c_id[] = { + { "adau1701", 0 }, + { } +}; +MODULE_DEVICE_TABLE(i2c, adau1701_i2c_id); + +/* corgi i2c codec control layer */ +static struct i2c_driver adau1701_i2c_driver = { + .driver = { + .name = "adau1701-codec", + .owner = THIS_MODULE, + }, + .probe = adau1701_i2c_probe, + .remove = __devexit_p(adau1701_i2c_remove), + .id_table = adau1701_i2c_id, +}; + +static int __init adau1701_modinit(void) +{ + int ret; + + ret = i2c_add_driver(&adau1701_i2c_driver); + if (ret != 0) { + printk(KERN_ERR "Failed to register adau1701 I2C driver: %d\n", + ret); + } + + return ret; +} +module_init(adau1701_modinit); + +static void __exit adau1701_exit(void) +{ + i2c_del_driver(&adau1701_i2c_driver); +} +module_exit(adau1701_exit); + +MODULE_DESCRIPTION("ASoC ADAU1701 SigmaDSP driver"); +MODULE_AUTHOR("Cliff Cai"); +MODULE_LICENSE("GPL"); diff --git a/sound/soc/codecs/adau1701.h b/sound/soc/codecs/adau1701.h new file mode 100644 index 0000000..174199e --- /dev/null +++ b/sound/soc/codecs/adau1701.h @@ -0,0 +1,111 @@ +/* + * header file for adau1701 SigmaDSP processor + * + * Copyright 2011 Analog Devices Inc. + * + * Licensed under the GPL-2 or later. + */ + +#ifndef _ADAU1701_H +#define _ADAU1701_H + +/* + * Register definition. + */ +#define ADAU1701_FIRSTREG 0x0800 +#define ADAU1701_LASTREG 0x0827 +#define ADAU1701_IFACE0 0x0800 +#define ADAU1701_IFACE1 0x0801 +#define ADAU1701_IFACE2 0x0802 +#define ADAU1701_IFACE3 0x0803 +#define ADAU1701_IFACE4 0x0804 +#define ADAU1701_IFACE5 0x0805 +#define ADAU1701_IFACE6 0x0806 +#define ADAU1701_IFACE7 0x0807 + +#define ADAU1701_GPIOSET 0x0808 +#define ADAU1701_AUXADC0 0x0809 +#define ADAU1701_AUXADC1 0x080A +#define ADAU1701_AUXADC2 0x080B +#define ADAU1701_AUXADC3 0x080C + +#define ADAU1701_SAFELD0 0x0810 +#define ADAU1701_SAFELD1 0x0811 +#define ADAU1701_SAFELD2 0x0812 +#define ADAU1701_SAFELD3 0x0813 +#define ADAU1701_SAFELD4 0x0814 + +#define ADAU1701_SLDADD0 0x0815 +#define ADAU1701_SLDADD1 0x0816 +#define ADAU1701_SLDADD2 0x0817 +#define ADAU1701_SLDADD3 0x0818 +#define ADAU1701_SLDADD4 0x0819 + +#define ADAU1701_DATCAP0 0x081A +#define ADAU1701_DATCAP1 0x081B + +#define ADAU1701_DSPCTRL 0x081C +#define ADAU1701_DSPRES 0x081D +#define ADAU1701_SEROCTL 0x081E +#define ADAU1701_SERITL1 0x081F + +#define ADAU1701_MPCONF0 0x0820 +#define ADAU1701_MPCONF1 0x0821 + +#define ADAU1701_AUXNPOW 0x0822 +#define ADAU1701_AUXADCE 0x0824 + +#define ADAU1701_OSCIPOW 0x0826 +#define ADAU1701_DACSET 0x0827 + + +#define ADAU1701_NUMCACHEREG 0x29 + +/* Bit fields */ +#define DSPCTRL_CR (1 << 2) +#define DSPCTRL_DAM (1 << 3) +#define DSPCTRL_ADM (1 << 4) +#define DSPCTRL_IST (1 << 5) +#define DSPCTRL_IFCW (1 << 6) +#define DSPCTRL_GPCW (1 << 7) +#define DSPCTRL_AACW (1 << 8) + +#define MPCONF_GPIOIDE (0) +#define MPCONF_GPIOINDE (1) +#define MPCONF_GPIOOPT (2) +#define MPCONF_OCOPT (3) +#define MPCONF_SDATAP (4) +#define MPCONF_GPIOIDEI (8) +#define MPCONF_GPIOINDEI (9) +#define MPCONF_GPIOOPTI (0xA) +#define MPCONF_OCOPTI (0xB) +#define MPCONF_SDATAPI (0xC) +#define MPCONF_AUXADC (0xF) + +#define SEROCTL_MASTER (0x0800) +#define SEROCTL_OBF16 (0x0000) +#define SEROCTL_OBF8 (0x0200) +#define SEROCTL_OBF4 (0x0400) +#define SEROCTL_OBF2 (0x0600) + +#define SEROCTL_OLF1024 (0x0000) +#define SEROCTL_OLF512 (0x0080) +#define SEROCTL_OLF256 (0x0100) +#define SEROCTL_OLFRSV (0x0180) + +#define AUXNPOW_AAPD (0x80) +#define AUXNPOW_VBPD (0x40) +#define AUXNPOW_VRPD (0x20) +#define AUXNPOW_D3PD (0x1) +#define AUXNPOW_D2PD (0x2) +#define AUXNPOW_D1PD (0x4) +#define AUXNPOW_D0PD (0x8) + +#define SERITL1_LEFTJ (1) +#define SERITL1_TDM (2) + +#define AUXADCE_AAEN (1 << 15) +#define OSCIPOW_OPD (0x04) +#define DACSET_DACEN (1) + +#endif
On Sun, Mar 6, 2011 at 20:11, cliff.cai@analog.com wrote:
+#define ADAU1701_FIRMWARE "SigmaDSP_fw.bin"
this probably needs to be more specific. like "adau1701.bin". otherwise it makes it a pain to work with diff codecs in the same system.
+static int adau1701_write_register(struct snd_soc_codec *codec,
- u16 reg_address, u8 length, u32 value)
+{
- int ret;
- int count = length + 2; /*data plus 16bit register address*/
- u8 buf[8] = {0, 0, 0, 0, 0, 0, 0, 0};
- if (length == 0)
- return -1;
- buf[0] = (reg_address >> 8) & 0xFF;
should be a blank line after that return
- buf[1] = reg_address & 0xFF;
- if (length == 1)
- buf[2] = value & 0xFF;
- else if (length == 2) {
- buf[2] = (value >> 8) & 0xFF;
- buf[3] = value & 0xFF;
- } else if (length == 3) {
- buf[2] = (value >> 16) & 0xFF;
- buf[3] = (value >> 8) & 0xFF;
- buf[4] = value & 0xFF;
- }
i think the buf[8] init forces gcc to generate bad code, and seems to be useless. shouldnt the code have an "else" brace to return -1 if length is greater than 3, and then change the decl to "u8 buf[5];" ?
also, all these 0xFF masks are useless -- you're assigning it to a u8 which implies bit truncation
ret = i2c_master_send(codec->control_data, buf, count);- return ret;
+}
the whole "int ret" seems kind of useless in this func. just return the i2c_master_send and drop the ret var completely. and drop the blank link after the return.
+/*
- read ADAU1701 hw register
- */
+static u32 adau1701_read_register(struct snd_soc_codec *codec,
- u16 reg_address, u8 length)
+{
- u8 addr[2];
- u8 buf[2];
- u32 value = 0;
- int ret;
- if (reg_address < ADAU1701_FIRSTREG)
- reg_address = reg_address + ADAU1701_FIRSTREG;
- if ((reg_address < ADAU1701_FIRSTREG) || (reg_address > ADAU1701_LASTREG))
- return -EIO;
this func has "u32" for ret, and you return the raw register value below. but here you try returning -EIO. shouldnt it be "int" so the caller can check for "< 0" ?
- addr[0] = (reg_address >> 8) & 0xFF;
- addr[1] = reg_address & 0xFF;
- /* write the 2byte read address */
- ret = i2c_master_send(codec->control_data, addr, 2);
- if (ret)
- return ret;
- if (length == 1) {
- if (i2c_master_recv(codec->control_data, buf, 1) != 1)
- return -EIO;
- value = buf[0];
- } else if (length == 2) {
- if (i2c_master_recv(codec->control_data, buf, 2) != 2)
- return -EIO;
seems like this can be combined into one simple code block where you replace "1" and '2" with "length"
+static int adau1701_setprogram(struct snd_soc_codec *codec) +{
- int ret = 0;
- ret = process_sigma_firmware(codec->control_data, ADAU1701_FIRMWARE);
- return ret;
+}
seems like this should be one statement -- a simple return
+static int adau1701_set_bias_level(struct snd_soc_codec *codec,
- enum snd_soc_bias_level level)
+{
- u16 reg;
- switch (level) {
- case SND_SOC_BIAS_ON:
- reg = adau1701_read_register(codec, ADAU1701_AUXNPOW, 2);
- reg &= ~(AUXNPOW_AAPD | AUXNPOW_D0PD | AUXNPOW_D1PD | AUXNPOW_D2PD |
- AUXNPOW_D3PD | AUXNPOW_VBPD | AUXNPOW_VRPD);
- adau1701_write_register(codec, ADAU1701_AUXNPOW, 2, reg);
- break;
- case SND_SOC_BIAS_PREPARE:
- break;
- case SND_SOC_BIAS_STANDBY:
- break;
- case SND_SOC_BIAS_OFF:
- /* everything off, dac mute, inactive */
- reg = adau1701_read_register(codec, ADAU1701_AUXNPOW, 2);
- reg |= AUXNPOW_AAPD | AUXNPOW_D0PD | AUXNPOW_D1PD | AUXNPOW_D2PD |
- AUXNPOW_D3PD | AUXNPOW_VBPD | AUXNPOW_VRPD;
- adau1701_write_register(codec, ADAU1701_AUXNPOW, 2, reg);
error checking missing on the read_register() call
- break;
- }
drop the blank line before the brace
+struct snd_soc_dai_driver adau1701_dai = {
- .name = "ADAU1701",
i think the standard is to use all lower case
+static int adau1701_suspend(struct snd_soc_codec *codec, pm_message_t state) +{
- adau1701_set_bias_level(codec, SND_SOC_BIAS_OFF);
- return 0;
+}
+static int adau1701_resume(struct snd_soc_codec *codec) +{
- adau1701_set_bias_level(codec, SND_SOC_BIAS_STANDBY);
- return 0;
+} ... +static int adau1701_remove(struct snd_soc_codec *codec) +{
adau1701_set_bias_level(codec, SND_SOC_BIAS_OFF);return 0;+}
why not return set_bias_level ?
+static int adau1701_reg_init(struct snd_soc_codec *codec) ...
- printk(KERN_ERR "Loading program data failed\n");
dev_err
+struct snd_soc_codec_driver soc_codec_dev_adau1701 = {
static
+static __devinit int adau1701_i2c_probe(struct i2c_client *i2c,
- const struct i2c_device_id *id)
+{
- struct adau1701_priv *adau1701;
- int ret = 0;
- adau1701 = kzalloc(sizeof(struct adau1701_priv), GFP_KERNEL);
sizeof(*adau1701)
+static int __init adau1701_modinit(void) +{
- int ret;
- ret = i2c_add_driver(&adau1701_i2c_driver);
- if (ret != 0) {
- printk(KERN_ERR "Failed to register adau1701 I2C driver: %d\n",
- ret);
- }
uesless braces, and should be pr_err
+MODULE_DESCRIPTION("ASoC ADAU1701 SigmaDSP driver"); +MODULE_AUTHOR("Cliff Cai"); +MODULE_LICENSE("GPL");
MODULE_ALIAS() ?
+++ b/sound/soc/codecs/adau1701.h ... +#define AUXADCE_AAEN (1 << 15) +#define OSCIPOW_OPD (0x04) +#define DACSET_DACEN (1)
looks like you got "#define<tab>" junk in here -mike
On Sun, Mar 06, 2011 at 09:44:49PM -0500, Mike Frysinger wrote:
On Sun, Mar 6, 2011 at 20:11, cliff.cai@analog.com wrote:
+static u32 adau1701_read_register(struct snd_soc_codec *codec,
- ?? ?? ?? u16 reg_address, u8 length)
+{
this func has "u32" for ret, and you return the raw register value below. but here you try returning -EIO. shouldnt it be "int" so the caller can check for "< 0" ?
This is the interface we have, unfortuantely. It's not ideal but it'd be a complete pain to change it and the actual benefit in production is questionable given our lack of options for dealing with them in typical systems beyond moaning in logs.
+struct snd_soc_dai_driver adau1701_dai = {
- ?? ?? ?? .name = "ADAU1701",
i think the standard is to use all lower case
Typically, yes.
+MODULE_DESCRIPTION("ASoC ADAU1701 SigmaDSP driver"); +MODULE_AUTHOR("Cliff Cai"); +MODULE_LICENSE("GPL");
MODULE_ALIAS() ?
That's not needed for I2C devices, it's handled by MODULE_DEVICE_TABLE() for I2C.
On Mon, 2011-03-07 at 09:11 +0800, cliff.cai@analog.com wrote:
From: Cliff Cai cliff.cai@analog.com
Had a quick look, mostly OK. Just a few cleanups required.
Thanks
Liam
ADAU1701 is an SigmaDSP processor,it supports I2S audio interface. It needs to include "linux/sigma.h" which is still in Andrew Morton's tree.
Signed-off-by: Cliff Caicliff.cai@analog.com
sound/soc/codecs/Kconfig | 4 + sound/soc/codecs/Makefile | 2 + sound/soc/codecs/adau1701.c | 417 +++++++++++++++++++++++++++++++++++++++++++ sound/soc/codecs/adau1701.h | 111 ++++++++++++ 4 files changed, 534 insertions(+), 0 deletions(-) create mode 100644 sound/soc/codecs/adau1701.c create mode 100644 sound/soc/codecs/adau1701.h
diff --git a/sound/soc/codecs/Kconfig b/sound/soc/codecs/Kconfig index bbc97fd..ba931c4 100644 --- a/sound/soc/codecs/Kconfig +++ b/sound/soc/codecs/Kconfig @@ -20,6 +20,7 @@ config SND_SOC_ALL_CODECS select SND_SOC_PCM3008 select SND_SOC_SPDIF select SND_SOC_SSM2602 if I2C
- select SND_SOC_ADAU1701 if I2C select SND_SOC_STAC9766 if SND_SOC_AC97_BUS select SND_SOC_TLV320AIC23 if I2C select SND_SOC_TLV320AIC26 if SPI_MASTER
@@ -98,6 +99,9 @@ config SND_SOC_SPDIF config SND_SOC_SSM2602 tristate
+config SND_SOC_ADAU1701
- tristate
config SND_SOC_STAC9766 tristate
diff --git a/sound/soc/codecs/Makefile b/sound/soc/codecs/Makefile index 8b75305..ed48581 100644 --- a/sound/soc/codecs/Makefile +++ b/sound/soc/codecs/Makefile @@ -8,6 +8,7 @@ snd-soc-l3-objs := l3.o snd-soc-pcm3008-objs := pcm3008.o snd-soc-spdif-objs := spdif_transciever.o snd-soc-ssm2602-objs := ssm2602.o +snd-soc-adau1701-objs := adau1701.o snd-soc-stac9766-objs := stac9766.o snd-soc-tlv320aic23-objs := tlv320aic23.o snd-soc-tlv320aic26-objs := tlv320aic26.o @@ -45,6 +46,7 @@ obj-$(CONFIG_SND_SOC_L3) += snd-soc-l3.o obj-$(CONFIG_SND_SOC_PCM3008) += snd-soc-pcm3008.o obj-$(CONFIG_SND_SOC_SPDIF) += snd-soc-spdif.o obj-$(CONFIG_SND_SOC_SSM2602) += snd-soc-ssm2602.o +obj-$(CONFIG_SND_SOC_ADAU1701) += snd-soc-adau1701.o obj-$(CONFIG_SND_SOC_STAC9766) += snd-soc-stac9766.o obj-$(CONFIG_SND_SOC_TLV320AIC23) += snd-soc-tlv320aic23.o obj-$(CONFIG_SND_SOC_TLV320AIC26) += snd-soc-tlv320aic26.o diff --git a/sound/soc/codecs/adau1701.c b/sound/soc/codecs/adau1701.c new file mode 100644 index 0000000..b7c671d --- /dev/null +++ b/sound/soc/codecs/adau1701.c @@ -0,0 +1,417 @@ +/*
- Driver for ADAU1701 SigmaDSP processor
- Copyright 2011 Analog Devices Inc.
- Licensed under the GPL-2 or later.
- */
+#include <linux/module.h> +#include <linux/moduleparam.h> +#include <linux/init.h> +#include <linux/delay.h> +#include <linux/pm.h> +#include <linux/i2c.h> +#include <linux/workqueue.h> +#include <linux/platform_device.h> +#include <linux/sigma.h> +#include <linux/sysfs.h> +#include <linux/slab.h> +#include <sound/core.h> +#include <sound/pcm.h> +#include <sound/pcm_params.h> +#include <sound/soc.h> +#include <sound/soc-dapm.h> +#include <sound/initval.h>
+#include "adau1701.h"
+#define AUDIO_NAME "adau1701" +#define ADAU1701_VERSION "0.10" +#define ADAU1701_FIRMWARE "SigmaDSP_fw.bin"
+/* codec private data */ +struct adau1701_priv {
- struct snd_soc_codec *codec;
- enum snd_soc_control_type control_type;
+};
+/*
- Write a ADAU1701 register,since the register length is from 1 to 5,
- So, use our own read/write functions instead of snd_soc_read/write.
- */
+static int adau1701_write_register(struct snd_soc_codec *codec,
- u16 reg_address, u8 length, u32 value)
+{
- int ret;
- int count = length + 2; /*data plus 16bit register address*/
- u8 buf[8] = {0, 0, 0, 0, 0, 0, 0, 0};
- if (length == 0)
return -1;- buf[0] = (reg_address >> 8) & 0xFF;
- buf[1] = reg_address & 0xFF;
- if (length == 1)
buf[2] = value & 0xFF;- else if (length == 2) {
buf[2] = (value >> 8) & 0xFF;buf[3] = value & 0xFF;- } else if (length == 3) {
buf[2] = (value >> 16) & 0xFF;buf[3] = (value >> 8) & 0xFF;buf[4] = value & 0xFF;- }
- ret = i2c_master_send(codec->control_data, buf, count);
- return ret;
Extra line
+}
+/*
- read ADAU1701 hw register
- */
+static u32 adau1701_read_register(struct snd_soc_codec *codec,
- u16 reg_address, u8 length)
+{
- u8 addr[2];
- u8 buf[2];
- u32 value = 0;
- int ret;
- if (reg_address < ADAU1701_FIRSTREG)
reg_address = reg_address + ADAU1701_FIRSTREG;- if ((reg_address < ADAU1701_FIRSTREG) || (reg_address > ADAU1701_LASTREG))
return -EIO;- addr[0] = (reg_address >> 8) & 0xFF;
- addr[1] = reg_address & 0xFF;
- /* write the 2byte read address */
- ret = i2c_master_send(codec->control_data, addr, 2);
- if (ret)
return ret;- if (length == 1) {
if (i2c_master_recv(codec->control_data, buf, 1) != 1)return -EIO;value = buf[0];- } else if (length == 2) {
if (i2c_master_recv(codec->control_data, buf, 2) != 2)return -EIO;value = (buf[0] << 8) | buf[1];- }
- return value;
+}
+static int adau1701_setprogram(struct snd_soc_codec *codec) +{
- int ret = 0;
Not necessary to set ret = 0 here.
- ret = process_sigma_firmware(codec->control_data, ADAU1701_FIRMWARE);
- return ret;
+}
+static int adau1701_pcm_prepare(struct snd_pcm_substream *substream,
struct snd_soc_dai *dai)+{
- struct snd_soc_codec *codec = dai->codec;
- int reg = 0;
ditto
- reg = SEROCTL_MASTER | SEROCTL_OBF16 | SEROCTL_OLF1024;
- adau1701_write_register(codec, ADAU1701_SEROCTL, 2, reg);
- return 0;
+}
+static void adau1701_shutdown(struct snd_pcm_substream *substream,
struct snd_soc_dai *dai)+{
- struct snd_soc_codec *codec = dai->codec;
- adau1701_write_register(codec, ADAU1701_SEROCTL, 2, 0);
+}
+static int adau1701_mute(struct snd_soc_dai *dai, int mute) +{
- struct snd_soc_codec *codec = dai->codec;
- u16 reg = 0;
ditto
- if (mute) {
/* mute inputs/outputs */reg = adau1701_read_register(codec, ADAU1701_AUXNPOW, 2);reg |= AUXNPOW_AAPD | AUXNPOW_D0PD | AUXNPOW_D1PD | AUXNPOW_D2PD | AUXNPOW_D3PD;adau1701_write_register(codec, ADAU1701_AUXNPOW, 2, reg);- } else {
/* unmute inputs/outputs */reg = adau1701_read_register(codec, ADAU1701_AUXNPOW, 2);reg &= ~(AUXNPOW_AAPD | AUXNPOW_D0PD | AUXNPOW_D1PD | AUXNPOW_D2PD | AUXNPOW_D3PD);adau1701_write_register(codec, ADAU1701_AUXNPOW, 2, reg);- }
- return 0;
+}
+static int adau1701_set_dai_fmt(struct snd_soc_dai *codec_dai,
unsigned int fmt)+{
- struct snd_soc_codec *codec = codec_dai->codec;
- u32 reg = 0;
ditto
- reg = adau1701_read_register(codec, ADAU1701_SERITL1, 1);
- /* interface format */
- switch (fmt & SND_SOC_DAIFMT_FORMAT_MASK) {
- case SND_SOC_DAIFMT_I2S:
break;- case SND_SOC_DAIFMT_LEFT_J:
reg |= SERITL1_LEFTJ;break;- /* TODO: support TDM */
- default:
return 0;- }
- /* clock inversion */
- switch (fmt & SND_SOC_DAIFMT_INV_MASK) {
- case SND_SOC_DAIFMT_NB_NF:
break;- /* TODO: support signal inversions */
- default:
return 0;
It's best to return an error if these are not supported atm.
- }
- /* set iface format*/
- adau1701_write_register(codec, ADAU1701_SERITL1, 1, reg);
- return 0;
+}
+static int adau1701_set_bias_level(struct snd_soc_codec *codec,
enum snd_soc_bias_level level)+{
- u16 reg;
- switch (level) {
- case SND_SOC_BIAS_ON:
reg = adau1701_read_register(codec, ADAU1701_AUXNPOW, 2);reg &= ~(AUXNPOW_AAPD | AUXNPOW_D0PD | AUXNPOW_D1PD | AUXNPOW_D2PD |AUXNPOW_D3PD | AUXNPOW_VBPD | AUXNPOW_VRPD);adau1701_write_register(codec, ADAU1701_AUXNPOW, 2, reg);break;- case SND_SOC_BIAS_PREPARE:
break;- case SND_SOC_BIAS_STANDBY:
break;- case SND_SOC_BIAS_OFF:
/* everything off, dac mute, inactive */reg = adau1701_read_register(codec, ADAU1701_AUXNPOW, 2);reg |= AUXNPOW_AAPD | AUXNPOW_D0PD | AUXNPOW_D1PD | AUXNPOW_D2PD |AUXNPOW_D3PD | AUXNPOW_VBPD | AUXNPOW_VRPD;adau1701_write_register(codec, ADAU1701_AUXNPOW, 2, reg);break;- }
- codec->bias_level = level;
- return 0;
+}
+#define ADAU1701_RATES SNDRV_PCM_RATE_48000
+#define ADAU1701_FORMATS (SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S20_3LE |\
- SNDRV_PCM_FMTBIT_S24_LE)
+static struct snd_soc_dai_ops adau1701_dai_ops = {
- .prepare = adau1701_pcm_prepare,
- .shutdown = adau1701_shutdown,
- .digital_mute = adau1701_mute,
- .set_fmt = adau1701_set_dai_fmt,
+};
+struct snd_soc_dai_driver adau1701_dai = {
- .name = "ADAU1701",
- .playback = {
.stream_name = "Playback",.channels_min = 2,.channels_max = 2,.rates = ADAU1701_RATES,.formats = ADAU1701_FORMATS,- },
- .capture = {
.stream_name = "Capture",.channels_min = 2,.channels_max = 2,.rates = ADAU1701_RATES,.formats = ADAU1701_FORMATS,- },
- .ops = &adau1701_dai_ops,
+}; +EXPORT_SYMBOL_GPL(adau1701_dai);
+static int adau1701_suspend(struct snd_soc_codec *codec, pm_message_t state) +{
- adau1701_set_bias_level(codec, SND_SOC_BIAS_OFF);
- return 0;
+}
+static int adau1701_resume(struct snd_soc_codec *codec) +{
- adau1701_set_bias_level(codec, SND_SOC_BIAS_STANDBY);
- return 0;
+}
+static ssize_t adau1371_dsp_load(struct device *dev,
struct device_attribute *attr,const char *buf, size_t count)+{
- int ret = 0;
No need for = 0.
- struct adau1701_priv *adau1701 = dev_get_drvdata(dev);
- struct snd_soc_codec *codec = adau1701->codec;
- ret = adau1701_setprogram(codec);
- if (ret)
return ret;- else
return count;+} +static DEVICE_ATTR(dsp, 0644, NULL, adau1371_dsp_load);
+static int adau1701_reg_init(struct snd_soc_codec *codec) +{
- u32 reg;
- int ret = 0;
ditto
- reg = DSPCTRL_DAM | DSPCTRL_ADM;
- adau1701_write_register(codec, ADAU1701_DSPCTRL, 2, reg);
- /* Load default program */
- ret = adau1701_setprogram(codec);
- if (ret < 0) {
printk(KERN_ERR "Loading program data failed\n");goto error;- }
- reg = DSPCTRL_DAM | DSPCTRL_ADM;
- adau1701_write_register(codec, ADAU1701_DSPCTRL, 2, reg);
- reg = 0x08;
- adau1701_write_register(codec, ADAU1701_DSPRES, 1, reg);
- adau1701_write_register(codec, ADAU1701_SEROCTL, 2, 0);
- adau1701_write_register(codec, ADAU1701_SERITL1, 1, 0);
- /* Configure the multipurpose pins as serial in/out pins */
- reg = MPCONF_SDATAP | MPCONF_SDATAP << 16 | MPCONF_SDATAP << 20;
- adau1701_write_register(codec, ADAU1701_MPCONF0, 3, reg);
- reg = MPCONF_AUXADC << 8 | MPCONF_SDATAP << 12 | MPCONF_SDATAP << 16 |
MPCONF_SDATAP << 20;- adau1701_write_register(codec, ADAU1701_MPCONF1, 3, reg);
- adau1701_write_register(codec, ADAU1701_AUXNPOW, 2, 0);
- reg = AUXADCE_AAEN;
- adau1701_write_register(codec, ADAU1701_AUXADCE, 2, reg);
- reg = DACSET_DACEN;
- adau1701_write_register(codec, ADAU1701_DACSET, 2, reg);
- reg = DSPCTRL_DAM | DSPCTRL_ADM | DSPCTRL_CR;
- adau1701_write_register(codec, ADAU1701_DSPCTRL, 2, reg);
- /* Power-up the oscillator */
- adau1701_write_register(codec, ADAU1701_OSCIPOW, 2, 0);
+error:
- return ret;
Probably best to either use the reg variable here for all writes or for none (my preference).
+}
+static int adau1701_probe(struct snd_soc_codec *codec) +{
- int ret = 0;
- struct adau1701_priv *adau1701 = snd_soc_codec_get_drvdata(codec);
- adau1701->codec = codec;
- ret = snd_soc_codec_set_cache_io(codec, 16, 16, adau1701->control_type);
- if (ret < 0) {
dev_err(codec->dev, "Failed to set cache I/O: %d\n", ret);return ret;- }
- ret = adau1701_reg_init(codec);
- if (ret < 0) {
dev_err(codec->dev, "failed to initialize\n");return ret;- }
- ret = device_create_file(codec->dev, &dev_attr_dsp);
- if (ret)
dev_err(codec->dev, "device_create_file() failed\n");- return ret;
+}
+static int adau1701_remove(struct snd_soc_codec *codec) +{
- adau1701_set_bias_level(codec, SND_SOC_BIAS_OFF);
- return 0;
+}
+struct snd_soc_codec_driver soc_codec_dev_adau1701 = {
- .probe = adau1701_probe,
- .remove = adau1701_remove,
- .suspend = adau1701_suspend,
- .resume = adau1701_resume,
- .set_bias_level = adau1701_set_bias_level,
+};
+static __devinit int adau1701_i2c_probe(struct i2c_client *i2c,
const struct i2c_device_id *id)+{
- struct adau1701_priv *adau1701;
- int ret = 0;
- adau1701 = kzalloc(sizeof(struct adau1701_priv), GFP_KERNEL);
- if (adau1701 == NULL)
return -ENOMEM;- adau1701->control_type = SND_SOC_I2C;
- i2c_set_clientdata(i2c, adau1701);
- ret = snd_soc_register_codec(&i2c->dev, &soc_codec_dev_adau1701, &adau1701_dai, 1);
- if (ret < 0)
kfree(adau1701);- return ret;
+}
+static __devexit int adau1701_i2c_remove(struct i2c_client *client) +{
- snd_soc_unregister_codec(&client->dev);
- kfree(i2c_get_clientdata(client));
- return 0;
+}
+static const struct i2c_device_id adau1701_i2c_id[] = {
- { "adau1701", 0 },
- { }
+}; +MODULE_DEVICE_TABLE(i2c, adau1701_i2c_id);
+/* corgi i2c codec control layer */ +static struct i2c_driver adau1701_i2c_driver = {
- .driver = {
.name = "adau1701-codec",.owner = THIS_MODULE,- },
- .probe = adau1701_i2c_probe,
- .remove = __devexit_p(adau1701_i2c_remove),
- .id_table = adau1701_i2c_id,
+};
+static int __init adau1701_modinit(void) +{
- int ret;
- ret = i2c_add_driver(&adau1701_i2c_driver);
- if (ret != 0) {
printk(KERN_ERR "Failed to register adau1701 I2C driver: %d\n",ret);- }
- return ret;
+} +module_init(adau1701_modinit);
+static void __exit adau1701_exit(void) +{
- i2c_del_driver(&adau1701_i2c_driver);
+} +module_exit(adau1701_exit);
+MODULE_DESCRIPTION("ASoC ADAU1701 SigmaDSP driver"); +MODULE_AUTHOR("Cliff Cai"); +MODULE_LICENSE("GPL"); diff --git a/sound/soc/codecs/adau1701.h b/sound/soc/codecs/adau1701.h new file mode 100644 index 0000000..174199e --- /dev/null +++ b/sound/soc/codecs/adau1701.h @@ -0,0 +1,111 @@ +/*
- header file for adau1701 SigmaDSP processor
- Copyright 2011 Analog Devices Inc.
- Licensed under the GPL-2 or later.
- */
+#ifndef _ADAU1701_H +#define _ADAU1701_H
+/*
- Register definition.
- */
+#define ADAU1701_FIRSTREG 0x0800 +#define ADAU1701_LASTREG 0x0827 +#define ADAU1701_IFACE0 0x0800 +#define ADAU1701_IFACE1 0x0801 +#define ADAU1701_IFACE2 0x0802 +#define ADAU1701_IFACE3 0x0803 +#define ADAU1701_IFACE4 0x0804 +#define ADAU1701_IFACE5 0x0805 +#define ADAU1701_IFACE6 0x0806 +#define ADAU1701_IFACE7 0x0807
+#define ADAU1701_GPIOSET 0x0808 +#define ADAU1701_AUXADC0 0x0809 +#define ADAU1701_AUXADC1 0x080A +#define ADAU1701_AUXADC2 0x080B +#define ADAU1701_AUXADC3 0x080C
+#define ADAU1701_SAFELD0 0x0810 +#define ADAU1701_SAFELD1 0x0811 +#define ADAU1701_SAFELD2 0x0812 +#define ADAU1701_SAFELD3 0x0813 +#define ADAU1701_SAFELD4 0x0814
+#define ADAU1701_SLDADD0 0x0815 +#define ADAU1701_SLDADD1 0x0816 +#define ADAU1701_SLDADD2 0x0817 +#define ADAU1701_SLDADD3 0x0818 +#define ADAU1701_SLDADD4 0x0819
+#define ADAU1701_DATCAP0 0x081A +#define ADAU1701_DATCAP1 0x081B
+#define ADAU1701_DSPCTRL 0x081C +#define ADAU1701_DSPRES 0x081D +#define ADAU1701_SEROCTL 0x081E +#define ADAU1701_SERITL1 0x081F
+#define ADAU1701_MPCONF0 0x0820 +#define ADAU1701_MPCONF1 0x0821
+#define ADAU1701_AUXNPOW 0x0822 +#define ADAU1701_AUXADCE 0x0824
+#define ADAU1701_OSCIPOW 0x0826 +#define ADAU1701_DACSET 0x0827
+#define ADAU1701_NUMCACHEREG 0x29
+/* Bit fields */ +#define DSPCTRL_CR (1 << 2) +#define DSPCTRL_DAM (1 << 3) +#define DSPCTRL_ADM (1 << 4) +#define DSPCTRL_IST (1 << 5) +#define DSPCTRL_IFCW (1 << 6) +#define DSPCTRL_GPCW (1 << 7) +#define DSPCTRL_AACW (1 << 8)
+#define MPCONF_GPIOIDE (0)
no need for () here and all but one case below.
+#define MPCONF_GPIOINDE (1) +#define MPCONF_GPIOOPT (2) +#define MPCONF_OCOPT (3) +#define MPCONF_SDATAP (4) +#define MPCONF_GPIOIDEI (8) +#define MPCONF_GPIOINDEI (9) +#define MPCONF_GPIOOPTI (0xA) +#define MPCONF_OCOPTI (0xB) +#define MPCONF_SDATAPI (0xC) +#define MPCONF_AUXADC (0xF)
+#define SEROCTL_MASTER (0x0800) +#define SEROCTL_OBF16 (0x0000) +#define SEROCTL_OBF8 (0x0200) +#define SEROCTL_OBF4 (0x0400) +#define SEROCTL_OBF2 (0x0600)
+#define SEROCTL_OLF1024 (0x0000) +#define SEROCTL_OLF512 (0x0080) +#define SEROCTL_OLF256 (0x0100) +#define SEROCTL_OLFRSV (0x0180)
+#define AUXNPOW_AAPD (0x80) +#define AUXNPOW_VBPD (0x40) +#define AUXNPOW_VRPD (0x20) +#define AUXNPOW_D3PD (0x1) +#define AUXNPOW_D2PD (0x2) +#define AUXNPOW_D1PD (0x4) +#define AUXNPOW_D0PD (0x8)
+#define SERITL1_LEFTJ (1) +#define SERITL1_TDM (2)
+#define AUXADCE_AAEN (1 << 15) +#define OSCIPOW_OPD (0x04) +#define DACSET_DACEN (1)
+#endif
On Mon, Mar 07, 2011 at 09:11:42AM +0800, cliff.cai@analog.com wrote:
From: Cliff Cai cliff.cai@analog.com
ADAU1701 is an SigmaDSP processor,it supports I2S audio interface.
As with previous submissions from you guys this won't compile with current ASoC versions. All new driver code submitted for mainline should be submitted against the development versions of the trees you're submitting against, in general -next contains everything relevant.
It needs to include "linux/sigma.h" which is still in Andrew Morton's tree.
Is this needed by other trees? I can't apply this driver until it's merged into ASoC via some means. I guess it'll go in during the merge window, though, and you do need to respin.
select SND_SOC_PCM3008 select SND_SOC_SPDIF select SND_SOC_SSM2602 if I2C
- select SND_SOC_ADAU1701 if I2C select SND_SOC_STAC9766 if SND_SOC_AC97_BUS select SND_SOC_TLV320AIC23 if I2C
This presumably also needs to select the DSP API otherwise it's not going to build.
As ever, keep the Kconfig and Makefile sorted.
+#define AUDIO_NAME "adau1701" +#define ADAU1701_VERSION "0.10"
These aren't referenced anywhere, remove them.
+/*
- Write a ADAU1701 register,since the register length is from 1 to 5,
- So, use our own read/write functions instead of snd_soc_read/write.
- */
+static int adau1701_write_register(struct snd_soc_codec *codec,
- u16 reg_address, u8 length, u32 value)
+{
It loooks like the register length is hard coded in every location that the register is referenced. This doesn't seem ideal - it'd be much nicer to have the register I/O functions work this out without the callers needing to.
+static int adau1701_pcm_prepare(struct snd_pcm_substream *substream,
struct snd_soc_dai *dai)+{
- struct snd_soc_codec *codec = dai->codec;
- int reg = 0;
- reg = SEROCTL_MASTER | SEROCTL_OBF16 | SEROCTL_OLF1024;
- adau1701_write_register(codec, ADAU1701_SEROCTL, 2, reg);
- return 0;
+}
This looks like some of it is DAI format and word length configuration?
+static void adau1701_shutdown(struct snd_pcm_substream *substream,
struct snd_soc_dai *dai)+{
- struct snd_soc_codec *codec = dai->codec;
- adau1701_write_register(codec, ADAU1701_SEROCTL, 2, 0);
+}
I suspect this isn't going to work for simultaneous playback and capture - it's not clear what the code does but I'd guess it will stop things completely.
+static int adau1701_set_dai_fmt(struct snd_soc_dai *codec_dai,
unsigned int fmt)+{
- struct snd_soc_codec *codec = codec_dai->codec;
- u32 reg = 0;
- reg = adau1701_read_register(codec, ADAU1701_SERITL1, 1);
- /* interface format */
- switch (fmt & SND_SOC_DAIFMT_FORMAT_MASK) {
- case SND_SOC_DAIFMT_I2S:
break;- case SND_SOC_DAIFMT_LEFT_J:
reg |= SERITL1_LEFTJ;break;- /* TODO: support TDM */
I'd expect that the I2S case should be clearing a bitmask.
- default:
return 0;- }
Return an error if you don't support something. The same issue applies elsewhere in the function.
+static int adau1701_set_bias_level(struct snd_soc_codec *codec,
enum snd_soc_bias_level level)+{
- u16 reg;
- switch (level) {
- case SND_SOC_BIAS_ON:
reg = adau1701_read_register(codec, ADAU1701_AUXNPOW, 2);reg &= ~(AUXNPOW_AAPD | AUXNPOW_D0PD | AUXNPOW_D1PD | AUXNPOW_D2PD |AUXNPOW_D3PD | AUXNPOW_VBPD | AUXNPOW_VRPD);adau1701_write_register(codec, ADAU1701_AUXNPOW, 2, reg);
You were also updating some of the same register bits in the mute function. This looks buggy.
break;- case SND_SOC_BIAS_PREPARE:
break;- case SND_SOC_BIAS_STANDBY:
break;- case SND_SOC_BIAS_OFF:
/* everything off, dac mute, inactive */reg = adau1701_read_register(codec, ADAU1701_AUXNPOW, 2);reg |= AUXNPOW_AAPD | AUXNPOW_D0PD | AUXNPOW_D1PD | AUXNPOW_D2PD |AUXNPOW_D3PD | AUXNPOW_VBPD | AUXNPOW_VRPD;adau1701_write_register(codec, ADAU1701_AUXNPOW, 2, reg);break;
It's very odd that the code only does anything in _OFF or _ON - what exactly are these updates doing?
.rates = ADAU1701_RATES,.formats = ADAU1701_FORMATS,- },
- .ops = &adau1701_dai_ops,
+}; +EXPORT_SYMBOL_GPL(adau1701_dai);
This is not required.
+static ssize_t adau1371_dsp_load(struct device *dev,
struct device_attribute *attr,const char *buf, size_t count)+{
- int ret = 0;
- struct adau1701_priv *adau1701 = dev_get_drvdata(dev);
- struct snd_soc_codec *codec = adau1701->codec;
- ret = adau1701_setprogram(codec);
- if (ret)
return ret;- else
return count;+} +static DEVICE_ATTR(dsp, 0644, NULL, adau1371_dsp_load);
You're marking the file as supporting both read and write but only providing write functionality, and the write functionality totally ignores the written data.
More generally this doesn't seem like a great interface - apparently the device requries that firmware be loaded but the whole firmware load process isn't at all joined up with the driver code meaning that the application layer has to figure out when firmware needs to be reloaded, there's no understanding on the part of the driver of what the firmware on the device is doing or how it's glued into the audio subsystem.
- ret = adau1701_setprogram(codec);
- if (ret < 0) {
printk(KERN_ERR "Loading program data failed\n");goto error;- }
- reg = DSPCTRL_DAM | DSPCTRL_ADM;
- adau1701_write_register(codec, ADAU1701_DSPCTRL, 2, reg);
- reg = 0x08;
- adau1701_write_register(codec, ADAU1701_DSPRES, 1, reg);
Should these DSP configuations things be part of downloading firmware?
- adau1701_write_register(codec, ADAU1701_AUXNPOW, 2, 0);
- reg = AUXADCE_AAEN;
- adau1701_write_register(codec, ADAU1701_AUXADCE, 2, reg);
- reg = DACSET_DACEN;
- adau1701_write_register(codec, ADAU1701_DACSET, 2, reg);
- reg = DSPCTRL_DAM | DSPCTRL_ADM | DSPCTRL_CR;
- adau1701_write_register(codec, ADAU1701_DSPCTRL, 2, reg);
- /* Power-up the oscillator */
- adau1701_write_register(codec, ADAU1701_OSCIPOW, 2, 0);
This looks like it's all power management which I'd expect to see handled in either the bias management functions or ideally DAPM. It also appears that the oscillator is an optional feature so it should be used conditionally.
- struct adau1701_priv *adau1701 = snd_soc_codec_get_drvdata(codec);
- adau1701->codec = codec;
You don't actually ever seem to reference the codec pointer you're storing, perhaps just loose it?
+/* Bit fields */ +#define DSPCTRL_CR (1 << 2) +#define DSPCTRL_DAM (1 << 3) +#define DSPCTRL_ADM (1 << 4) +#define DSPCTRL_IST (1 << 5) +#define DSPCTRL_IFCW (1 << 6) +#define DSPCTRL_GPCW (1 << 7) +#define DSPCTRL_AACW (1 << 8)
These and the other bitfield definitions in the header should all be namespaced.
On Mon, Mar 7, 2011 at 06:41, Mark Brown wrote:
On Mon, Mar 07, 2011 at 09:11:42AM +0800, cliff.cai@analog.com wrote:
From: Cliff Cai cliff.cai@analog.com It needs to include "linux/sigma.h" which is still in Andrew Morton's tree.
Is this needed by other trees?
no
I can't apply this driver until it's merged into ASoC via some means
and andrew has been holding off on merging the firmware driver until something in mainline needs it. once a driver gets to the point where you're fine with merging it, we can bug akpm to push the sigma firmware loader to linus.
select SND_SOC_PCM3008 select SND_SOC_SPDIF select SND_SOC_SSM2602 if I2C
select SND_SOC_ADAU1701 if I2C select SND_SOC_STAC9766 if SND_SOC_AC97_BUS select SND_SOC_TLV320AIC23 if I2CThis presumably also needs to select the DSP API otherwise it's not going to build.
by "DSP API" i'm guessing you mean the Sigma firmware loader
+static ssize_t adau1371_dsp_load(struct device *dev,
struct device_attribute *attr,const char *buf, size_t count)+{
int ret = 0;struct adau1701_priv *adau1701 = dev_get_drvdata(dev);struct snd_soc_codec *codec = adau1701->codec;ret = adau1701_setprogram(codec);if (ret)return ret;elsereturn count;+} +static DEVICE_ATTR(dsp, 0644, NULL, adau1371_dsp_load);
You're marking the file as supporting both read and write but only providing write functionality, and the write functionality totally ignores the written data.
More generally this doesn't seem like a great interface - apparently the device requries that firmware be loaded but the whole firmware load process isn't at all joined up with the driver code meaning that the application layer has to figure out when firmware needs to be reloaded, there's no understanding on the part of the driver of what the firmware on the device is doing or how it's glued into the audio subsystem.
the API should allow for userspace to specify the firmware name, but that's about it. it is up to the userspace application to arbitrarily load different firmware files on the fly into the codec without the codec caring what's going into it. often times the firmware is DSP code to do different post processing on the audio stream (cleanup or whatever). -mike
On Mon, Mar 07, 2011 at 06:50:15AM -0500, Mike Frysinger wrote:
On Mon, Mar 7, 2011 at 06:41, Mark Brown wrote:
Is this needed by other trees?
no
I can't apply this driver until it's merged into ASoC via some means
and andrew has been holding off on merging the firmware driver until something in mainline needs it. once a driver gets to the point where you're fine with merging it, we can bug akpm to push the sigma firmware loader to linus.
It'd seem easier to just merge the two patches together rather than trying to deal with cross-tree issues.
This presumably also needs to select the DSP API otherwise it's not going to build.
by "DSP API" i'm guessing you mean the Sigma firmware loader
Yes.
More generally this doesn't seem like a great interface - apparently the device requries that firmware be loaded but the whole firmware load process isn't at all joined up with the driver code meaning that the application layer has to figure out when firmware needs to be reloaded, there's no understanding on the part of the driver of what the firmware on the device is doing or how it's glued into the audio subsystem.
the API should allow for userspace to specify the firmware name, but that's about it. it is up to the userspace application to arbitrarily load different firmware files on the fly into the codec without the codec caring what's going into it. often times the firmware is DSP code to do different post processing on the audio stream (cleanup or whatever).
Right, and this isn't a particularly unusual requirement especially if the driver isn't going to have any ability to interact with the DSP (at which point it's just coefficient data, the fact that it's a DSP program is uninteresting). The problem is that this isn't a great interface for doing that.
At a really basic level the code is not at all integrated with either power management or stream handling in ASoC meaning that the user could try to download firmware in the middle of an active audio stream (which isn't going to be ideal). Obviously the driver doesn't really support any kind of power management at the minute but if it were improved in this area you'd also have issues with trying to download firmware while the device is off which probably isn't going to work either.
It's also not an interface which offers any kind of discoverability or enumerability, meaning that it's not suitable for integration into application layer code such as the use case manager. Applications need to be hard coded to know that a particular magic sysfs file needs to be poked. This would be a big step backwards in terms of the ability to run off the shelf application software.
On Mon, Mar 7, 2011 at 07:15, Mark Brown wrote:
On Mon, Mar 07, 2011 at 06:50:15AM -0500, Mike Frysinger wrote:
On Mon, Mar 7, 2011 at 06:41, Mark Brown wrote:
I can't apply this driver until it's merged into ASoC via some means
and andrew has been holding off on merging the firmware driver until something in mainline needs it. once a driver gets to the point where you're fine with merging it, we can bug akpm to push the sigma firmware loader to linus.
It'd seem easier to just merge the two patches together rather than trying to deal with cross-tree issues.
that sounds like a terrible idea. they're logically different changesets and we shouldnt be squashing them together simply to work around problems with process workflows.
More generally this doesn't seem like a great interface - apparently the device requries that firmware be loaded but the whole firmware load process isn't at all joined up with the driver code meaning that the application layer has to figure out when firmware needs to be reloaded, there's no understanding on the part of the driver of what the firmware on the device is doing or how it's glued into the audio subsystem.
the API should allow for userspace to specify the firmware name, but that's about it. it is up to the userspace application to arbitrarily load different firmware files on the fly into the codec without the codec caring what's going into it. often times the firmware is DSP code to do different post processing on the audio stream (cleanup or whatever).
Right, and this isn't a particularly unusual requirement especially if the driver isn't going to have any ability to interact with the DSP (at which point it's just coefficient data, the fact that it's a DSP program is uninteresting). The problem is that this isn't a great interface for doing that.
i dont see suggestions of a better interface anywhere ...
At a really basic level the code is not at all integrated with either power management or stream handling in ASoC meaning that the user could try to download firmware in the middle of an active audio stream (which isn't going to be ideal). Obviously the driver doesn't really support any kind of power management at the minute but if it were improved in this area you'd also have issues with trying to download firmware while the device is off which probably isn't going to work either.
wrt pm, that's a trivial programming issue that would be resolved in the driver. userspace need not care.
wrt stream flows, all the customers we've talked to are fine with this -- having stream control be an application issue. their application is driving the codec directly and knows when it needs to change the firmware, so it pauses its alsa flow, loads the new firmware, and moves on.
that said, i dont see anything in asoc today to address this, so if we're simply missing it, please highlight it for us.
It's also not an interface which offers any kind of discoverability or enumerability, meaning that it's not suitable for integration into application layer code such as the use case manager. Applications need to be hard coded to know that a particular magic sysfs file needs to be poked. This would be a big step backwards in terms of the ability to run off the shelf application software.
i dont see the issue here. the firmware is *optional* and does not impair basic audio output. further, the firmware is fully written/compiled/maintained by the end customer, just like the application. which means there is no "magic" here -- the end customer is the wizard.
there is no "stock" firmware that ADI or anyone provides for any of these SigmaDSP audio codecs. -mike
On Mon, Mar 07, 2011 at 07:29:12AM -0500, Mike Frysinger wrote:
On Mon, Mar 7, 2011 at 07:15, Mark Brown wrote:
It'd seem easier to just merge the two patches together rather than trying to deal with cross-tree issues.
that sounds like a terrible idea. they're logically different changesets and we shouldnt be squashing them together simply to work around problems with process workflows.
I'm not saying squash the changes, I'm saying apply them both via the same tree. From what you're saying the DSP code is used exclusively by audio devices anyway...
Right, and this isn't a particularly unusual requirement especially if the driver isn't going to have any ability to interact with the DSP (at which point it's just coefficient data, the fact that it's a DSP program is uninteresting). The problem is that this isn't a great interface for doing that.
i dont see suggestions of a better interface anywhere ...
It currently isn't and I'd encourage you to contribute to the discussion that's been going on on this, or even better help out with code. There was some discussion on the list recently (in the past month IIRC).
Whatever we do is going to require some additional APIs in alsa-lib, I'd expect. The key requirements I'm aware of are that we be able to support:
- Discoverable userspace interface. - Very large data sizes (megabytes have been mentioned). - Adding and removing parameter sets at runtime (for in system callibration).
Given the number of people looking at this from various angles I'd expect to see some sort of progress this year (probably in the next quater or so), but obviously no guarantees.
wrt pm, that's a trivial programming issue that would be resolved in the driver. userspace need not care.
That's the ideal, I mentioned this as several of my other review comments concerned issues with power management in the driver - addressing those will probably require that the integration occur.
wrt stream flows, all the customers we've talked to are fine with this -- having stream control be an application issue. their application is driving the codec directly and knows when it needs to change the firmware, so it pauses its alsa flow, loads the new firmware, and moves on.
I'd expect that the driver would at least error out if the user tried to do the wrong thing here, like I say currently the firmware code is just not joined up with anything else at all.
that said, i dont see anything in asoc today to address this, so if we're simply missing it, please highlight it for us.
There's handling of this in a bunch of drivers for things like EQ coefficients - the missing bit is a way of telling the driver that there are new coefficient sets available at runtime, at the minute everything that supports this enumerates the available coefficient sets in platform data and presents the user with an enumeration.
It's also not an interface which offers any kind of discoverability or enumerability, meaning that it's not suitable for integration into application layer code such as the use case manager. Applications need to be hard coded to know that a particular magic sysfs file needs to be poked. This would be a big step backwards in terms of the ability to run off the shelf application software.
i dont see the issue here. the firmware is *optional* and does not impair basic audio output. further, the firmware is fully
If the firmware is totally optional the driver needs updating - currently at startup it does:
| + /* Load default program */ | + ret = adau1701_setprogram(codec); | + if (ret < 0) { | + printk(KERN_ERR "Loading program data failed\n"); | + goto error; | + }
which will make the firmware mandatory. In any case, the interface issues are there if the firmware is optional or not.
written/compiled/maintained by the end customer, just like the application. which means there is no "magic" here -- the end customer is the wizard.
there is no "stock" firmware that ADI or anyone provides for any of these SigmaDSP audio codecs.
The question of where the DSP firmware (or coefficient data) comes from is orthogonal to the issue of runtime management of the data. The issue is how the application layer can tell if there are multiple coefficient sets and change between them, either directly or via something like UCM. Even if the system is using a custom application rather than an off the shelf distribution (which are becoming more and more common in embedded systems) there's no reason they shouldn't be able to rely on standard tools for managing their audio configurations.
At present userspace can enumerate and change the runtime configuration the system offers via the ALSA APIs (and this will get even better once the media controller API starts being used). This means that you can fairly easily write a userspace that'll run on pretty much any Linux audio hardware, adapting with pure configuration for which you can provide point and click tuning (realistically by allowing the user to configure via standard ALSA tools and offering a "save as use case" type interface). If we start adding backdoors to drivers we're taking a step back from where we are currently by requiring that the application layer know magic stuff about individual systems in order to work with them.
If this was an obscure system-specific feature it'd be much less of an issue but this is something which pretty much all modern CODECs can make use of.
On Mon, Mar 7, 2011 at 09:59, Mark Brown wrote:
On Mon, Mar 07, 2011 at 07:29:12AM -0500, Mike Frysinger wrote:
On Mon, Mar 7, 2011 at 07:15, Mark Brown wrote:
It'd seem easier to just merge the two patches together rather than trying to deal with cross-tree issues.
that sounds like a terrible idea. they're logically different changesets and we shouldnt be squashing them together simply to work around problems with process workflows.
I'm not saying squash the changes, I'm saying apply them both via the same tree.
merging trees != merging patches ;)
Right, and this isn't a particularly unusual requirement especially if the driver isn't going to have any ability to interact with the DSP (at which point it's just coefficient data, the fact that it's a DSP program is uninteresting). The problem is that this isn't a great interface for doing that.
i dont see suggestions of a better interface anywhere ...
It currently isn't and I'd encourage you to contribute to the discussion that's been going on on this, or even better help out with code. There was some discussion on the list recently (in the past month IIRC).
i'd be looking at Cliff/Michael for this. the ADI linux team has been restructured and audio parts are handled by Michael's new group now.
wrt stream flows, all the customers we've talked to are fine with this -- having stream control be an application issue. their application is driving the codec directly and knows when it needs to change the firmware, so it pauses its alsa flow, loads the new firmware, and moves on.
I'd expect that the driver would at least error out if the user tried to do the wrong thing here, like I say currently the firmware code is just not joined up with anything else at all.
i dont see how the driver can detect a "wrong" thing. the driver has no idea what arbitrary code the user is going to load and what that code is going to do, or validate the code in any way. this is why the firmware has a small crc header on it -- we only make sure that what the user compiled at build time matches what is loaded into the hardware.
that said, i dont see anything in asoc today to address this, so if we're simply missing it, please highlight it for us.
There's handling of this in a bunch of drivers for things like EQ coefficients - the missing bit is a way of telling the driver that there are new coefficient sets available at runtime, at the minute everything that supports this enumerates the available coefficient sets in platform data and presents the user with an enumeration.
i'm not sure that example is applicable ... or if it is, it's a pretty rough fit to the point of not being worthwhile
It's also not an interface which offers any kind of discoverability or enumerability, meaning that it's not suitable for integration into application layer code such as the use case manager. Applications need to be hard coded to know that a particular magic sysfs file needs to be poked. This would be a big step backwards in terms of the ability to run off the shelf application software.
i dont see the issue here. the firmware is *optional* and does not impair basic audio output. further, the firmware is fully
If the firmware is totally optional the driver needs updating - currently at startup it does:
| + /* Load default program */ | + ret = adau1701_setprogram(codec); | + if (ret < 0) { | + printk(KERN_ERR "Loading program data failed\n"); | + goto error; | + }
which will make the firmware mandatory. In any case, the interface issues are there if the firmware is optional or not.
i believe that is an error in the driver. Cliff can obviously correct me here since he's actually working on the codec.
written/compiled/maintained by the end customer, just like the application. which means there is no "magic" here -- the end customer is the wizard.
there is no "stock" firmware that ADI or anyone provides for any of these SigmaDSP audio codecs.
The question of where the DSP firmware (or coefficient data) comes from is orthogonal to the issue of runtime management of the data. The issue is how the application layer can tell if there are multiple coefficient sets and change between them, either directly or via something like UCM. Even if the system is using a custom application rather than an off the shelf distribution (which are becoming more and more common in embedded systems) there's no reason they shouldn't be able to rely on standard tools for managing their audio configurations.
if the standard tools existed today, i'd of course agree. but as you indicated there's nothing right now for us to bug off of. so how userspace "probes" for existing data would be however the end user chooses to manage things. it's not like the standard tools could really provide anything other than a simple string that indicates "some blob exists with name xxx". the meaning/metadata that surrounds xxx isnt really relevant from the kernel's pov.
At present userspace can enumerate and change the runtime configuration the system offers via the ALSA APIs (and this will get even better once the media controller API starts being used). This means that you can fairly easily write a userspace that'll run on pretty much any Linux audio hardware, adapting with pure configuration for which you can provide point and click tuning (realistically by allowing the user to configure via standard ALSA tools and offering a "save as use case" type interface). If we start adding backdoors to drivers we're taking a step back from where we are currently by requiring that the application layer know magic stuff about individual systems in order to work with them.
from how we've seen people using these codecs, this scenario doesnt make much sense. the different algorithms would be loaded on the fly by the application and its current operating needs, not a single algorithm selected by the ender user that wouldnt change for the life of the app. not saying the scenario would never come up, just that it isnt the one we'd be focused on. -mike
On Mon, Mar 07, 2011 at 10:33:25AM -0500, Mike Frysinger wrote:
On Mon, Mar 7, 2011 at 09:59, Mark Brown wrote:
Please at the start of capitalise sentences, it makes your text much more leigible.
It currently isn't and I'd encourage you to contribute to the discussion that's been going on on this, or even better help out with code. There was some discussion on the list recently (in the past month IIRC).
i'd be looking at Cliff/Michael for this. the ADI linux team has been restructured and audio parts are handled by Michael's new group now.
Sure, "you" in the above is Analog (and indeed anyone else who's looking at similar areas).
I'd expect that the driver would at least error out if the user tried to do the wrong thing here, like I say currently the firmware code is just not joined up with anything else at all.
i dont see how the driver can detect a "wrong" thing. the driver has no idea what arbitrary code the user is going to load and what that code is going to do, or validate the code in any way. this is why the firmware has a small crc header on it -- we only make sure that what the user compiled at build time matches what is loaded into the hardware.
At a bare minimum suddenly stopping and starting the firmware while audio is going through it is unlikely to work well (you'd most likely get a hard stop of the audio followed by a sudden hard start which sound very unpleasant to listeners) and so should be prevented. There's a bunch of options for doing this (including refusing to change, ensuring the DSP output is muted during the change or routing around the DSP while doing the change).
systems) there's no reason they shouldn't be able to rely on standard tools for managing their audio configurations.
if the standard tools existed today, i'd of course agree. but as you indicated there's nothing right now for us to bug off of. so how userspace "probes" for existing data would be however the end user chooses to manage things. it's not like the standard tools could really provide anything other than a simple string that indicates "some blob exists with name xxx". the meaning/metadata that surrounds xxx isnt really relevant from the kernel's pov.
The standard tools should also be able to manage the mechanics of actually getting the new data into the kernel at appropriate moments. This includes both offering control via UIs such as alsamixer and being able to include configuration of the data in UCM configurations.
At present userspace can enumerate and change the runtime configuration the system offers via the ALSA APIs (and this will get even better once the media controller API starts being used). This means that you can fairly easily write a userspace that'll run on pretty much any Linux audio hardware, adapting with pure configuration for which you can provide point and click tuning (realistically by allowing the user to configure via standard ALSA tools and offering a "save as use case" type interface). If we start adding backdoors to drivers we're taking a step back from where we are currently by requiring that the application layer know magic stuff about individual systems in order to work with them.
from how we've seen people using these codecs, this scenario doesnt make much sense. the different algorithms would be loaded on the fly by the application and its current operating needs, not a single algorithm selected by the ender user that wouldnt change for the life of the app. not saying the scenario would never come up, just that it isnt the one we'd be focused on.
I'm not sure you're following what's being said here. The above control discussed above full system configuration of all the control offered by the system, not tuning the parameters of an individual algorithm. This includes volume controls, routing controls, algorithms, coefficients and anything else that can be changed. A scenario where you want to change the set of algorithms the hardware can support is certainly included in that.
On Mon, Mar 7, 2011 at 10:55, Mark Brown wrote:
On Mon, Mar 07, 2011 at 10:33:25AM -0500, Mike Frysinger wrote:
On Mon, Mar 7, 2011 at 09:59, Mark Brown wrote:
I'd expect that the driver would at least error out if the user tried to do the wrong thing here, like I say currently the firmware code is just not joined up with anything else at all.
i dont see how the driver can detect a "wrong" thing. the driver has no idea what arbitrary code the user is going to load and what that code is going to do, or validate the code in any way. this is why the firmware has a small crc header on it -- we only make sure that what the user compiled at build time matches what is loaded into the hardware.
At a bare minimum suddenly stopping and starting the firmware while audio is going through it is unlikely to work well (you'd most likely get a hard stop of the audio followed by a sudden hard start which sound very unpleasant to listeners) and so should be prevented. There's a bunch of options for doing this (including refusing to change, ensuring the DSP output is muted during the change or routing around the DSP while doing the change).
you would probably get the "normal" clicks and pops, but i guess your view of "wrong" is much more strict than mine ;). i'm not sure our parts allow routing around the DSP (Cliff would have to comment). as for the rest, i think it'd be best to let the user space app dictate how they want to handle things. perhaps clicks/pops are fine with them. or they arent, and so the userspace app would make sure to pause/mute/whatever the stream. either way, this sounds like a policy that shouldnt be hard coded in the codec driver.
systems) there's no reason they shouldn't be able to rely on standard tools for managing their audio configurations.
if the standard tools existed today, i'd of course agree. but as you indicated there's nothing right now for us to bug off of. so how userspace "probes" for existing data would be however the end user chooses to manage things. it's not like the standard tools could really provide anything other than a simple string that indicates "some blob exists with name xxx". the meaning/metadata that surrounds xxx isnt really relevant from the kernel's pov.
The standard tools should also be able to manage the mechanics of actually getting the new data into the kernel at appropriate moments. This includes both offering control via UIs such as alsamixer and being able to include configuration of the data in UCM configurations.
exposing this via alsamixer and friends would be a useful debugging tool so people can toy around with known working configurations. and have code examples to see how to do it.
At present userspace can enumerate and change the runtime configuration the system offers via the ALSA APIs (and this will get even better once the media controller API starts being used). This means that you can fairly easily write a userspace that'll run on pretty much any Linux audio hardware, adapting with pure configuration for which you can provide point and click tuning (realistically by allowing the user to configure via standard ALSA tools and offering a "save as use case" type interface). If we start adding backdoors to drivers we're taking a step back from where we are currently by requiring that the application layer know magic stuff about individual systems in order to work with them.
from how we've seen people using these codecs, this scenario doesnt make much sense. the different algorithms would be loaded on the fly by the application and its current operating needs, not a single algorithm selected by the ender user that wouldnt change for the life of the app. not saying the scenario would never come up, just that it isnt the one we'd be focused on.
I'm not sure you're following what's being said here. The above control discussed above full system configuration of all the control offered by the system, not tuning the parameters of an individual algorithm. This includes volume controls, routing controls, algorithms, coefficients and anything else that can be changed. A scenario where you want to change the set of algorithms the hardware can support is certainly included in that.
i just meant that the use cases we've been dealing with involve the people developing the application taking care of picking which firmwares to load at any particular time. having the end user (the guy who buys the actual device) select firmwares doesnt make much sense. but this particular qualification probably is irrelevant to the framework you're proposing in the end. -mike
On Wed, Mar 9, 2011 at 2:08 PM, Mike Frysinger vapier.adi@gmail.com wrote:
On Mon, Mar 7, 2011 at 10:55, Mark Brown wrote:
On Mon, Mar 07, 2011 at 10:33:25AM -0500, Mike Frysinger wrote:
On Mon, Mar 7, 2011 at 09:59, Mark Brown wrote:
I'd expect that the driver would at least error out if the user tried to do the wrong thing here, like I say currently the firmware code is just not joined up with anything else at all.
i dont see how the driver can detect a "wrong" thing. the driver has no idea what arbitrary code the user is going to load and what that code is going to do, or validate the code in any way. this is why the firmware has a small crc header on it -- we only make sure that what the user compiled at build time matches what is loaded into the hardware.
At a bare minimum suddenly stopping and starting the firmware while audio is going through it is unlikely to work well (you'd most likely get a hard stop of the audio followed by a sudden hard start which sound very unpleasant to listeners) and so should be prevented. There's a bunch of options for doing this (including refusing to change, ensuring the DSP output is muted during the change or routing around the DSP while doing the change).
you would probably get the "normal" clicks and pops, but i guess your view of "wrong" is much more strict than mine ;). i'm not sure our parts allow routing around the DSP (Cliff would have to comment). as for the rest, i think it'd be best to let the user space app dictate how they want to handle things. perhaps clicks/pops are fine with them. or they arent, and so the userspace app would make sure to pause/mute/whatever the stream. either way, this sounds like a policy that shouldnt be hard coded in the codec driver.
again,this part is a DSP itself not an audio codec like adau1761 whose DSP core can be bypassed.
systems) there's no reason they shouldn't be able to rely on standard tools for managing their audio configurations.
if the standard tools existed today, i'd of course agree. but as you indicated there's nothing right now for us to bug off of. so how userspace "probes" for existing data would be however the end user chooses to manage things. it's not like the standard tools could really provide anything other than a simple string that indicates "some blob exists with name xxx". the meaning/metadata that surrounds xxx isnt really relevant from the kernel's pov.
The standard tools should also be able to manage the mechanics of actually getting the new data into the kernel at appropriate moments. This includes both offering control via UIs such as alsamixer and being able to include configuration of the data in UCM configurations.
exposing this via alsamixer and friends would be a useful debugging tool so people can toy around with known working configurations. and have code examples to see how to do it.
At present userspace can enumerate and change the runtime configuration the system offers via the ALSA APIs (and this will get even better once the media controller API starts being used). This means that you can fairly easily write a userspace that'll run on pretty much any Linux audio hardware, adapting with pure configuration for which you can provide point and click tuning (realistically by allowing the user to configure via standard ALSA tools and offering a "save as use case" type interface). If we start adding backdoors to drivers we're taking a step back from where we are currently by requiring that the application layer know magic stuff about individual systems in order to work with them.
from how we've seen people using these codecs, this scenario doesnt make much sense. the different algorithms would be loaded on the fly by the application and its current operating needs, not a single algorithm selected by the ender user that wouldnt change for the life of the app. not saying the scenario would never come up, just that it isnt the one we'd be focused on.
I'm not sure you're following what's being said here. The above control discussed above full system configuration of all the control offered by the system, not tuning the parameters of an individual algorithm. This includes volume controls, routing controls, algorithms, coefficients and anything else that can be changed. A scenario where you want to change the set of algorithms the hardware can support is certainly included in that.
i just meant that the use cases we've been dealing with involve the people developing the application taking care of picking which firmwares to load at any particular time. having the end user (the guy who buys the actual device) select firmwares doesnt make much sense. but this particular qualification probably is irrelevant to the framework you're proposing in the end. -mike _______________________________________________ Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
On Wed, Mar 09, 2011 at 01:08:03AM -0500, Mike Frysinger wrote:
On Mon, Mar 7, 2011 at 10:55, Mark Brown wrote:
At a bare minimum suddenly stopping and starting the firmware while audio is going through it is unlikely to work well (you'd most likely get a hard stop of the audio followed by a sudden hard start which sound very unpleasant to listeners) and so should be prevented. There's a bunch of options for doing this (including refusing to change, ensuring the DSP output is muted during the change or routing around the DSP while doing the change).
you would probably get the "normal" clicks and pops, but i guess your view of "wrong" is much more strict than mine ;). i'm not sure our parts allow routing around the DSP (Cliff would have to comment). as for the rest, i think it'd be best to let the user space app dictate how they want to handle things. perhaps clicks/pops are fine with them. or they arent, and so the userspace app would make sure to pause/mute/whatever the stream. either way, this sounds like a policy that shouldnt be hard coded in the codec driver.
Muting the DSP output during firmware reboots seems like an entirely reasonable way of doing this if there's no ability to route round the DSP or otherwise deal with the issue, and given how cheap the mute/unmute is it's hard to see where a user would find that a problem. Either they'll want to deal with the issue or they won't care.
Bear in mind that people are trying to do things like run off the shelf PulseAudio as their system audio manager in embedded systems - the kernel should make some effort to behave sanely.
The standard tools should also be able to manage the mechanics of actually getting the new data into the kernel at appropriate moments. This includes both offering control via UIs such as alsamixer and being able to include configuration of the data in UCM configurations.
exposing this via alsamixer and friends would be a useful debugging tool so people can toy around with known working configurations. and have code examples to see how to do it.
The ALSA APIs are also the userspace interface to the audio subsystem, any userspace application interacting with the audio hardware is expected to use them.
I'm not sure you're following what's being said here. The above control discussed above full system configuration of all the control offered by the system, not tuning the parameters of an individual algorithm. This includes volume controls, routing controls, algorithms, coefficients and anything else that can be changed. A scenario where you want to change the set of algorithms the hardware can support is certainly included in that.
i just meant that the use cases we've been dealing with involve the people developing the application taking care of picking which firmwares to load at any particular time. having the end user (the guy who buys the actual device) select firmwares doesnt make much sense. but this particular qualification probably is irrelevant to the framework you're proposing in the end.
In a modern Linux system the user will rarely see an application that exposes ALSA directly unless they go looking (in an embedded system they may not be able to go looking due to the limits of the UI), they'll see a higher level abstraction. In the desktop case the primary interface the user has is with GUIs that control PulseAudio which abstract away the actual control offered by the drivers. Embedded systems tend to either use Pulse or brew their own equivalent.
On Mon, 7 Mar 2011 14:59:31 +0000 Mark Brown broonie@opensource.wolfsonmicro.com wrote:
On Mon, Mar 07, 2011 at 07:29:12AM -0500, Mike Frysinger wrote:
On Mon, Mar 7, 2011 at 07:15, Mark Brown wrote:
[...]
Right, and this isn't a particularly unusual requirement especially if the driver isn't going to have any ability to interact with the DSP (at which point it's just coefficient data, the fact that it's a DSP program is uninteresting). The problem is that this isn't a great interface for doing that.
i dont see suggestions of a better interface anywhere ...
It currently isn't and I'd encourage you to contribute to the discussion that's been going on on this, or even better help out with code. There was some discussion on the list recently (in the past month IIRC).
Whatever we do is going to require some additional APIs in alsa-lib, I'd expect. The key requirements I'm aware of are that we be able to support:
- Discoverable userspace interface.
- Very large data sizes (megabytes have been mentioned).
- Adding and removing parameter sets at runtime (for in system callibration).
Given the number of people looking at this from various angles I'd expect to see some sort of progress this year (probably in the next quater or so), but obviously no guarantees.
Out of curiosity, has any progress been made on this?
wrt pm, that's a trivial programming issue that would be resolved in the driver. userspace need not care.
That's the ideal, I mentioned this as several of my other review comments concerned issues with power management in the driver - addressing those will probably require that the integration occur.
wrt stream flows, all the customers we've talked to are fine with this -- having stream control be an application issue. their application is driving the codec directly and knows when it needs to change the firmware, so it pauses its alsa flow, loads the new firmware, and moves on.
What would actually cause the firmware to change? Would the userspace application that's currently driving the codec know that it needs to load a new firmware, or will there be multiple applications potentially interacting with the codec (with only one driving it, and another deciding that the firmware should change)?
One can imagine two different types of APIs depending upon the answers to those questions. If the application driving the codec is also changing the firmware, a simple 'echo new_firmware_name.bin > /sys/.../filter_coeff' (or equivalent ALSA function) to cause something in alsa core to stop/pause/mute playback or capture and restart everything after the firmware has been loaded is all that's necessary. That is, userspace triggers the firmware loading. However, if multiple applications are going to be interacting with the codec, I'd imagine we'd need something more complex; perhaps a hook in snd_soc_dai_ops that's called anytime stream state changes? That would leave userspace no longer able to explicitly trigger a reload, but leave the driver smart enough to ensure that when a new firmware is available, it will be loaded once it is safe to do so (a break in userspace codec usage).
On Mon, Apr 18, 2011 at 05:15:05PM -0700, Andres Salomon wrote:
Mark Brown broonie@opensource.wolfsonmicro.com wrote:
Given the number of people looking at this from various angles I'd expect to see some sort of progress this year (probably in the next quater or so), but obviously no guarantees.
Out of curiosity, has any progress been made on this?
No, it's only been a month.
What would actually cause the firmware to change? Would the userspace application that's currently driving the codec know that it needs to load a new firmware, or will there be multiple applications potentially interacting with the codec (with only one driving it, and another deciding that the firmware should change)?
Clearly something in userspace will need to be able to do this.
One can imagine two different types of APIs depending upon the answers to those questions. If the application driving the codec is also changing the firmware, a simple 'echo new_firmware_name.bin > /sys/.../filter_coeff' (or equivalent ALSA function)
Using sysfs isn't going to fly for all the reasons discussed upthread - it's not enumerable and it's not going to cope with large coefficient sets.
However, if multiple applications are going to be interacting with the codec, I'd imagine we'd need something more complex; perhaps a hook in snd_soc_dai_ops that's called anytime stream state changes? That
Using the ops isn't going to work as there's no reason a coefficient set has to be associated with a DAI.
On Mon, Mar 7, 2011 at 8:29 PM, Mike Frysinger vapier.adi@gmail.com wrote:
On Mon, Mar 7, 2011 at 07:15, Mark Brown wrote:
On Mon, Mar 07, 2011 at 06:50:15AM -0500, Mike Frysinger wrote:
On Mon, Mar 7, 2011 at 06:41, Mark Brown wrote:
I can't apply this driver until it's merged into ASoC via some means
and andrew has been holding off on merging the firmware driver until something in mainline needs it. once a driver gets to the point where you're fine with merging it, we can bug akpm to push the sigma firmware loader to linus.
It'd seem easier to just merge the two patches together rather than trying to deal with cross-tree issues.
that sounds like a terrible idea. they're logically different changesets and we shouldnt be squashing them together simply to work around problems with process workflows.
More generally this doesn't seem like a great interface - apparently the device requries that firmware be loaded but the whole firmware load process isn't at all joined up with the driver code meaning that the application layer has to figure out when firmware needs to be reloaded, there's no understanding on the part of the driver of what the firmware on the device is doing or how it's glued into the audio subsystem.
the API should allow for userspace to specify the firmware name, but that's about it. it is up to the userspace application to arbitrarily load different firmware files on the fly into the codec without the codec caring what's going into it. often times the firmware is DSP code to do different post processing on the audio stream (cleanup or whatever).
Right, and this isn't a particularly unusual requirement especially if the driver isn't going to have any ability to interact with the DSP (at which point it's just coefficient data, the fact that it's a DSP program is uninteresting). The problem is that this isn't a great interface for doing that.
i dont see suggestions of a better interface anywhere ...
At a really basic level the code is not at all integrated with either power management or stream handling in ASoC meaning that the user could try to download firmware in the middle of an active audio stream (which isn't going to be ideal). Obviously the driver doesn't really support any kind of power management at the minute but if it were improved in this area you'd also have issues with trying to download firmware while the device is off which probably isn't going to work either.
wrt pm, that's a trivial programming issue that would be resolved in the driver. userspace need not care.
wrt stream flows, all the customers we've talked to are fine with this -- having stream control be an application issue. their application is driving the codec directly and knows when it needs to change the firmware, so it pauses its alsa flow, loads the new firmware, and moves on.
that said, i dont see anything in asoc today to address this, so if we're simply missing it, please highlight it for us.
It's also not an interface which offers any kind of discoverability or enumerability, meaning that it's not suitable for integration into application layer code such as the use case manager. Applications need to be hard coded to know that a particular magic sysfs file needs to be poked. This would be a big step backwards in terms of the ability to run off the shelf application software.
i dont see the issue here. the firmware is *optional* and does not impair basic audio output. further, the firmware is fully written/compiled/maintained by the end customer, just like the application. which means there is no "magic" here -- the end customer is the wizard.
it's a DSP,so firmware is not optional,actually there is default internal program can be used if no external firmware is downloaded,of cause, the internal program is only used to test analog audio pass-through.
there is no "stock" firmware that ADI or anyone provides for any of these SigmaDSP audio codecs. -mike _______________________________________________ Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
On Wed, Mar 09, 2011 at 03:25:05PM +0800, Cliff Cai wrote:
On Mon, Mar 7, 2011 at 8:29 PM, Mike Frysinger vapier.adi@gmail.com wrote:
i dont see the issue here. the firmware is *optional* and does not impair basic audio output. further, the firmware is fully written/compiled/maintained by the end customer, just like the application. which means there is no "magic" here -- the end customer is the wizard.
it's a DSP,so firmware is not optional,actually there is default internal program can be used if no external firmware is downloaded,of cause, the internal program is only used to test analog audio pass-through.
If there is default firmware then even if it's not particularly useful the kernel should probably not *require* that additional firmware is provided during driver startup - the system may wish to wait until later in the boot to provide it, for example because it needs to mount media to get to the firmware or because it needs to do additional work to decide what firmware is required for the current system setup.
On Wed, Mar 9, 2011 at 05:00, Mark Brown wrote:
On Wed, Mar 09, 2011 at 03:25:05PM +0800, Cliff Cai wrote:
On Mon, Mar 7, 2011 at 8:29 PM, Mike Frysinger wrote:
i dont see the issue here. the firmware is *optional* and does not impair basic audio output. further, the firmware is fully written/compiled/maintained by the end customer, just like the application. which means there is no "magic" here -- the end customer is the wizard.
it's a DSP,so firmware is not optional,actually there is default internal program can be used if no external firmware is downloaded,of cause, the internal program is only used to test analog audio pass-through.
If there is default firmware then even if it's not particularly useful the kernel should probably not *require* that additional firmware is provided during driver startup - the system may wish to wait until later in the boot to provide it, for example because it needs to mount media to get to the firmware or because it needs to do additional work to decide what firmware is required for the current system setup.
sorry, i was thinking this part was like the other ADI ones where the DSP is there as an enhancement. so many of my comments were tailored towards that.
there shouldnt be any problem with us providing a stub firmware with the kernel (in firmware/) that simply does a "pass through" or whatever. -mike
-----Original Message----- From: Mark Brown [mailto:broonie@opensource.wolfsonmicro.com] Sent: 2011年3月9日 18:01 To: Cliff Cai Cc: Mike Frysinger; Cai, Cliff; alsa-devel@alsa-project.org; linux-kernel@vger.kernel.org; device-drivers-devel@blackfin.uclinux.org; akpm@linux-foundation.org; lrg@slimlogic.co.uk Subject: Re: [alsa-devel] [Device-drivers-devel] [PATCH] Add driver for Analog Devices ADAU1701 SigmaDSP
On Wed, Mar 09, 2011 at 03:25:05PM +0800, Cliff Cai wrote:
On Mon, Mar 7, 2011 at 8:29 PM, Mike Frysinger
vapier.adi@gmail.com wrote:
i dont see the issue here. the firmware is *optional* and
does not
impair basic audio output. further, the firmware is fully written/compiled/maintained by the end customer, just like the application. which means there is no "magic" here -- the end customer is the wizard.
it's a DSP,so firmware is not optional,actually there is default internal program can be used if no external firmware is
downloaded,of
cause, the internal program is only used to test analog audio pass-through.
If there is default firmware then even if it's not particularly useful the kernel should probably not *require* that additional firmware is provided during driver startup - the system may wish to wait until later in the boot to provide it, for example because it needs to mount media to get to the firmware or because it needs to do additional work to decide what firmware is required for the current system setup.
I'm afraid it's not the real use case,the test mode just for test, In practice,people always want the DSP to run some algrithm at the beginning, Although they may change it later.
Cliff
On Thu, Mar 10, 2011 at 04:45:48AM -0500, Cai, Cliff wrote:
If there is default firmware then even if it's not particularly useful the kernel should probably not *require* that additional firmware is provided during driver startup -
I'm afraid it's not the real use case,the test mode just for test, In practice,people always want the DSP to run some algrithm at the beginning, Although they may change it later.
I'd be surprised if people wanted to actually play audio through a passthrough firmware in a shipping system but consider cases like board bringup (where just verifying that everything is physically connected is useful) and early boot (where you might not have mounted all filesystems yet but the driver is trying to start up).
On Mon, Mar 7, 2011 at 7:41 PM, Mark Brown broonie@opensource.wolfsonmicro.com wrote:
On Mon, Mar 07, 2011 at 09:11:42AM +0800, cliff.cai@analog.com wrote:
From: Cliff Cai cliff.cai@analog.com
ADAU1701 is an SigmaDSP processor,it supports I2S audio interface.
As with previous submissions from you guys this won't compile with current ASoC versions. All new driver code submitted for mainline should be submitted against the development versions of the trees you're submitting against, in general -next contains everything relevant.
It needs to include "linux/sigma.h" which is still in Andrew Morton's tree.
Is this needed by other trees? I can't apply this driver until it's merged into ASoC via some means. I guess it'll go in during the merge window, though, and you do need to respin.
select SND_SOC_PCM3008 select SND_SOC_SPDIF select SND_SOC_SSM2602 if I2C
- select SND_SOC_ADAU1701 if I2C
select SND_SOC_STAC9766 if SND_SOC_AC97_BUS select SND_SOC_TLV320AIC23 if I2C
This presumably also needs to select the DSP API otherwise it's not going to build.
As ever, keep the Kconfig and Makefile sorted.
+#define AUDIO_NAME "adau1701" +#define ADAU1701_VERSION "0.10"
These aren't referenced anywhere, remove them.
+/*
- Write a ADAU1701 register,since the register length is from 1 to 5,
- So, use our own read/write functions instead of snd_soc_read/write.
- */
+static int adau1701_write_register(struct snd_soc_codec *codec,
- u16 reg_address, u8 length, u32 value)
+{
It loooks like the register length is hard coded in every location that the register is referenced. This doesn't seem ideal - it'd be much nicer to have the register I/O functions work this out without the callers needing to.
I'm afraid it's not easy to do so.
+static int adau1701_pcm_prepare(struct snd_pcm_substream *substream,
- struct snd_soc_dai *dai)
+{
- struct snd_soc_codec *codec = dai->codec;
- int reg = 0;
- reg = SEROCTL_MASTER | SEROCTL_OBF16 | SEROCTL_OLF1024;
- adau1701_write_register(codec, ADAU1701_SEROCTL, 2, reg);
- return 0;
+}
This looks like some of it is DAI format and word length configuration?
no,it makes the processor work in master mode.and output bit clock and frame clock.
+static void adau1701_shutdown(struct snd_pcm_substream *substream,
- struct snd_soc_dai *dai)
+{
- struct snd_soc_codec *codec = dai->codec;
- adau1701_write_register(codec, ADAU1701_SEROCTL, 2, 0);
+}
I suspect this isn't going to work for simultaneous playback and capture
- it's not clear what the code does but I'd guess it will stop things
completely.
this SigmaDSP doesn't support duplex operation,it can choose either ADCs or serial port as input source.
+static int adau1701_set_dai_fmt(struct snd_soc_dai *codec_dai,
- unsigned int fmt)
+{
- struct snd_soc_codec *codec = codec_dai->codec;
- u32 reg = 0;
- reg = adau1701_read_register(codec, ADAU1701_SERITL1, 1);
- /* interface format */
- switch (fmt & SND_SOC_DAIFMT_FORMAT_MASK) {
- case SND_SOC_DAIFMT_I2S:
- break;
- case SND_SOC_DAIFMT_LEFT_J:
- reg |= SERITL1_LEFTJ;
- break;
- /* TODO: support TDM */
I'd expect that the I2S case should be clearing a bitmask.
- default:
- return 0;
- }
Return an error if you don't support something. The same issue applies elsewhere in the function.
+static int adau1701_set_bias_level(struct snd_soc_codec *codec,
- enum snd_soc_bias_level level)
+{
- u16 reg;
- switch (level) {
- case SND_SOC_BIAS_ON:
- reg = adau1701_read_register(codec, ADAU1701_AUXNPOW, 2);
- reg &= ~(AUXNPOW_AAPD | AUXNPOW_D0PD | AUXNPOW_D1PD | AUXNPOW_D2PD |
- AUXNPOW_D3PD | AUXNPOW_VBPD | AUXNPOW_VRPD);
- adau1701_write_register(codec, ADAU1701_AUXNPOW, 2, reg);
You were also updating some of the same register bits in the mute function. This looks buggy.
the processor has no switchs to mute or unmute ADCS/DACs,only thing we can do is turning them off or on.
- break;
- case SND_SOC_BIAS_PREPARE:
- break;
- case SND_SOC_BIAS_STANDBY:
- break;
- case SND_SOC_BIAS_OFF:
- /* everything off, dac mute, inactive */
- reg = adau1701_read_register(codec, ADAU1701_AUXNPOW, 2);
- reg |= AUXNPOW_AAPD | AUXNPOW_D0PD | AUXNPOW_D1PD | AUXNPOW_D2PD |
- AUXNPOW_D3PD | AUXNPOW_VBPD | AUXNPOW_VRPD;
- adau1701_write_register(codec, ADAU1701_AUXNPOW, 2, reg);
- break;
It's very odd that the code only does anything in _OFF or _ON - what exactly are these updates doing?
- .rates = ADAU1701_RATES,
- .formats = ADAU1701_FORMATS,
- },
- .ops = &adau1701_dai_ops,
+}; +EXPORT_SYMBOL_GPL(adau1701_dai);
This is not required.
+static ssize_t adau1371_dsp_load(struct device *dev,
- struct device_attribute *attr,
- const char *buf, size_t count)
+{
- int ret = 0;
- struct adau1701_priv *adau1701 = dev_get_drvdata(dev);
- struct snd_soc_codec *codec = adau1701->codec;
- ret = adau1701_setprogram(codec);
- if (ret)
- return ret;
- else
- return count;
+} +static DEVICE_ATTR(dsp, 0644, NULL, adau1371_dsp_load);
You're marking the file as supporting both read and write but only providing write functionality, and the write functionality totally ignores the written data.
More generally this doesn't seem like a great interface - apparently the device requries that firmware be loaded but the whole firmware load process isn't at all joined up with the driver code meaning that the application layer has to figure out when firmware needs to be reloaded, there's no understanding on the part of the driver of what the firmware on the device is doing or how it's glued into the audio subsystem.
- ret = adau1701_setprogram(codec);
- if (ret < 0) {
- printk(KERN_ERR "Loading program data failed\n");
- goto error;
- }
- reg = DSPCTRL_DAM | DSPCTRL_ADM;
- adau1701_write_register(codec, ADAU1701_DSPCTRL, 2, reg);
- reg = 0x08;
- adau1701_write_register(codec, ADAU1701_DSPRES, 1, reg);
Should these DSP configuations things be part of downloading firmware?
these configurations above loading firmware mainly used to avoid pops/clicks and cleanup some registers in the DSP core.
- adau1701_write_register(codec, ADAU1701_AUXNPOW, 2, 0);
- reg = AUXADCE_AAEN;
- adau1701_write_register(codec, ADAU1701_AUXADCE, 2, reg);
- reg = DACSET_DACEN;
- adau1701_write_register(codec, ADAU1701_DACSET, 2, reg);
- reg = DSPCTRL_DAM | DSPCTRL_ADM | DSPCTRL_CR;
- adau1701_write_register(codec, ADAU1701_DSPCTRL, 2, reg);
- /* Power-up the oscillator */
- adau1701_write_register(codec, ADAU1701_OSCIPOW, 2, 0);
This looks like it's all power management which I'd expect to see handled in either the bias management functions or ideally DAPM. It also appears that the oscillator is an optional feature so it should be used conditionally.
the processor can receive MCLK either from external clock source or crystal oscillator, currently we use the on board crystal,and it can't be turned off,otherwise the whole chip will be in an unpredicted status, only output clocks can be disabled.
- struct adau1701_priv *adau1701 = snd_soc_codec_get_drvdata(codec);
- adau1701->codec = codec;
You don't actually ever seem to reference the codec pointer you're storing, perhaps just loose it?
+/* Bit fields */ +#define DSPCTRL_CR (1 << 2) +#define DSPCTRL_DAM (1 << 3) +#define DSPCTRL_ADM (1 << 4) +#define DSPCTRL_IST (1 << 5) +#define DSPCTRL_IFCW (1 << 6) +#define DSPCTRL_GPCW (1 << 7) +#define DSPCTRL_AACW (1 << 8)
These and the other bitfield definitions in the header should all be namespaced. _______________________________________________ Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
On Wed, Mar 09, 2011 at 03:04:03PM +0800, Cliff Cai wrote:
On Mon, Mar 7, 2011 at 7:41 PM, Mark Brown
On Mon, Mar 07, 2011 at 09:11:42AM +0800, cliff.cai@analog.com wrote:
It loooks like the register length is hard coded in every location that the register is referenced. ?This doesn't seem ideal - it'd be much nicer to have the register I/O functions work this out without the callers needing to.
I'm afraid it's not easy to do so.
What's the issue? I'd expect something like a lookup table or switch statement going from register to register length - I'd be surprised if it were any more complicated than getting all the callers right.
+static int adau1701_pcm_prepare(struct snd_pcm_substream *substream,
- struct snd_soc_dai *dai)
+{
- struct snd_soc_codec *codec = dai->codec;
- int reg = 0;
- reg = SEROCTL_MASTER | SEROCTL_OBF16 | SEROCTL_OLF1024;
- adau1701_write_register(codec, ADAU1701_SEROCTL, 2, reg);
- return 0;
+}
This looks like some of it is DAI format and word length configuration?
no,it makes the processor work in master mode.and output bit clock and frame clock.
This controls the CODEC, not the CPU? Master/slave should be controlled by set_dai_fmt() rather than being hard coded.
+static void adau1701_shutdown(struct snd_pcm_substream *substream,
- struct snd_soc_dai *dai)
+{
- struct snd_soc_codec *codec = dai->codec;
- adau1701_write_register(codec, ADAU1701_SEROCTL, 2, 0);
+}
I suspect this isn't going to work for simultaneous playback and capture
- it's not clear what the code does but I'd guess it will stop things
completely.
this SigmaDSP doesn't support duplex operation,it can choose either ADCs or serial port as input source.
The driver should be enforcing this constraint, then.
+static int adau1701_set_bias_level(struct snd_soc_codec *codec,
- enum snd_soc_bias_level level)
+{
- u16 reg;
- switch (level) {
- case SND_SOC_BIAS_ON:
- reg = adau1701_read_register(codec, ADAU1701_AUXNPOW, 2);
- reg &= ~(AUXNPOW_AAPD | AUXNPOW_D0PD | AUXNPOW_D1PD | AUXNPOW_D2PD |
- AUXNPOW_D3PD | AUXNPOW_VBPD | AUXNPOW_VRPD);
- adau1701_write_register(codec, ADAU1701_AUXNPOW, 2, reg);
You were also updating some of the same register bits in the mute function. This looks buggy.
the processor has no switchs to mute or unmute ADCS/DACs,only thing we can do is turning them off or on.
If mute can't be implemented don't implement it. Right now the code is clearly not going to do what's expected as you've got two different bits of code trying to control the same thing - at least one set of register updates isn't going to do anything?
- ret = adau1701_setprogram(codec);
- if (ret < 0) {
- printk(KERN_ERR "Loading program data failed\n");
- goto error;
- }
- reg = DSPCTRL_DAM | DSPCTRL_ADM;
- adau1701_write_register(codec, ADAU1701_DSPCTRL, 2, reg);
- reg = 0x08;
- adau1701_write_register(codec, ADAU1701_DSPRES, 1, reg);
Should these DSP configuations things be part of downloading firmware?
these configurations above loading firmware mainly used to avoid pops/clicks and cleanup some registers in the DSP core.
Right, but is this something that's always useful when loading firmware (in which case the firmware load should just wrap it up so it always happens)?
- adau1701_write_register(codec, ADAU1701_AUXNPOW, 2, 0);
- reg = AUXADCE_AAEN;
- adau1701_write_register(codec, ADAU1701_AUXADCE, 2, reg);
- reg = DACSET_DACEN;
- adau1701_write_register(codec, ADAU1701_DACSET, 2, reg);
- reg = DSPCTRL_DAM | DSPCTRL_ADM | DSPCTRL_CR;
- adau1701_write_register(codec, ADAU1701_DSPCTRL, 2, reg);
- /* Power-up the oscillator */
- adau1701_write_register(codec, ADAU1701_OSCIPOW, 2, 0);
This looks like it's all power management which I'd expect to see handled in either the bias management functions or ideally DAPM. It also appears that the oscillator is an optional feature so it should be used conditionally.
the processor can receive MCLK either from external clock source or crystal oscillator, currently we use the on board crystal,and it can't be turned off,otherwise the whole chip will be in an unpredicted status, only output clocks can be disabled.
That's specific to your board design, though. Another board design might use a different clocking setup.
-----Original Message----- From: Mark Brown [mailto:broonie@opensource.wolfsonmicro.com] Sent: 2011年3月9日 18:12 To: Cliff Cai Cc: Cai, Cliff; device-drivers-devel@blackfin.uclinux.org; akpm@linux-foundation.org; linux-kernel@vger.kernel.org; alsa-devel@alsa-project.org; lrg@slimlogic.co.uk Subject: Re: [alsa-devel] [PATCH] Add driver for Analog Devices ADAU1701 SigmaDSP
On Wed, Mar 09, 2011 at 03:04:03PM +0800, Cliff Cai wrote:
On Mon, Mar 7, 2011 at 7:41 PM, Mark Brown
On Mon, Mar 07, 2011 at 09:11:42AM +0800,
cliff.cai@analog.com wrote:
It loooks like the register length is hard coded in every location that the register is referenced. ?This doesn't seem ideal
- it'd be
much nicer to have the register I/O functions work this
out without
the callers needing to.
I'm afraid it's not easy to do so.
What's the issue? I'd expect something like a lookup table or switch statement going from register to register length - I'd be surprised if it were any more complicated than getting all the callers right.
All right,just looks nicer.
+static int adau1701_pcm_prepare(struct snd_pcm_substream +*substream,
struct snd_soc_dai *dai) {struct snd_soc_codec *codec = dai->codec;int reg = 0;
reg = SEROCTL_MASTER | SEROCTL_OBF16 | SEROCTL_OLF1024;adau1701_write_register(codec, ADAU1701_SEROCTL, 2, reg);
return 0;+}
This looks like some of it is DAI format and word length
configuration?
no,it makes the processor work in master mode.and output bit
clock and
frame clock.
This controls the CODEC, not the CPU? Master/slave should be controlled by set_dai_fmt() rather than being hard coded.
Actually,if the DAI works in master mode,it will always output clocks. it's not we want,so we have to make it in slave mode when shutdown,and Make it in Master mode before starting to play.
+static void adau1701_shutdown(struct snd_pcm_substream
*substream,
struct snd_soc_dai *dai) {struct snd_soc_codec *codec = dai->codec;
adau1701_write_register(codec, ADAU1701_SEROCTL, 2, 0); }I suspect this isn't going to work for simultaneous playback and capture
- it's not clear what the code does but I'd guess it will stop
things completely.
this SigmaDSP doesn't support duplex operation,it can choose either ADCs or serial port as input source.
The driver should be enforcing this constraint, then.
It's the firmware that is in charge of this work.
+static int adau1701_set_bias_level(struct snd_soc_codec *codec,
enum snd_soc_bias_level level) {u16 reg;switch (level) {case SND_SOC_BIAS_ON:reg = adau1701_read_register(codec,ADAU1701_AUXNPOW,
+2);
reg &= ~(AUXNPOW_AAPD | AUXNPOW_D0PD |AUXNPOW_D1PD |
+AUXNPOW_D2PD |
AUXNPOW_D3PD | AUXNPOW_VBPD |AUXNPOW_VRPD);
adau1701_write_register(codec, ADAU1701_AUXNPOW, 2,+reg);
You were also updating some of the same register bits in the mute function. This looks buggy.
the processor has no switchs to mute or unmute
ADCS/DACs,only thing we
can do is turning them off or on.
If mute can't be implemented don't implement it. Right now the code is clearly not going to do what's expected as you've got two different bits of code trying to control the same thing - at least one set of register updates isn't going to do anything?
All right.
ret = adau1701_setprogram(codec);if (ret < 0) {printk(KERN_ERR "Loading program data failed\n");goto error;}reg = DSPCTRL_DAM | DSPCTRL_ADM;adau1701_write_register(codec, ADAU1701_DSPCTRL, 2, reg);reg = 0x08;adau1701_write_register(codec, ADAU1701_DSPRES, 1, reg);Should these DSP configuations things be part of
downloading firmware?
these configurations above loading firmware mainly used to avoid pops/clicks and cleanup some registers in the DSP core.
Right, but is this something that's always useful when loading firmware (in which case the firmware load should just wrap it up so it always happens)?
Sure,thanks.
adau1701_write_register(codec, ADAU1701_AUXNPOW, 2, 0);reg = AUXADCE_AAEN;adau1701_write_register(codec, ADAU1701_AUXADCE, 2, reg);reg = DACSET_DACEN;adau1701_write_register(codec, ADAU1701_DACSET, 2, reg);reg = DSPCTRL_DAM | DSPCTRL_ADM | DSPCTRL_CR;adau1701_write_register(codec, ADAU1701_DSPCTRL, 2, reg);/* Power-up the oscillator */adau1701_write_register(codec, ADAU1701_OSCIPOW, 2, 0);This looks like it's all power management which I'd expect to see handled in either the bias management functions or ideally
DAPM. It
also appears that the oscillator is an optional feature so
it should
be used conditionally.
the processor can receive MCLK either from external clock source or crystal oscillator, currently we use the on board
crystal,and it can't
be turned off,otherwise the whole chip will be in an unpredicted status, only output clocks can be disabled.
That's specific to your board design, though. Another board design might use a different clocking setup.
Yes,so I think I can add a MACRO to indicate it.
Cliff
participants (7)
-
Andres Salomon -
Cai, Cliff -
Cliff Cai -
cliff.cai＠analog.com
-
Liam Girdwood -
Mark Brown -
Mike Frysinger