LinuxLists.cc - [PATCH] Add driver for Analog Devices ADAU1701 SigmaDSP

[permalink] [raw]

Subject: Re: [Device-drivers-devel] [PATCH] Add driver for Analog Devices ADAU1701 SigmaDSP

On Sun, Mar 6, 2011 at 20:11, <[email protected]> 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

2011-03-07 11:01:48

by Liam Girdwood

[permalink] [raw]

Subject: Re: [alsa-devel] [PATCH] Add driver for Analog Devices ADAU1701 SigmaDSP

On Mon, 2011-03-07 at 09:11 +0800, [email protected] wrote:
> From: Cliff Cai <[email protected]>
>

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 Cai<[email protected]>
> ---
> 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

2011-03-07 11:41:49

[permalink] [raw]

Subject: Re: [PATCH] Add driver for Analog Devices ADAU1701 SigmaDSP

On Mon, Mar 07, 2011 at 09:11:42AM +0800, [email protected] wrote:

> From: Cliff Cai <[email protected]>

> 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.

2011-03-07 11:44:10

[permalink] [raw]

Subject: Re: [alsa-devel] [Device-drivers-devel] [PATCH] Add driver for Analog Devices ADAU1701 SigmaDSP

On Sun, Mar 06, 2011 at 09:44:49PM -0500, Mike Frysinger wrote:
> On Sun, Mar 6, 2011 at 20:11, <[email protected]> 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.

2011-03-07 11:50:37

[permalink] [raw]

Subject: Re: [Device-drivers-devel] [PATCH] Add driver for Analog Devices ADAU1701 SigmaDSP

On Mon, Mar 7, 2011 at 06:41, Mark Brown wrote:
> On Mon, Mar 07, 2011 at 09:11:42AM +0800, [email protected] wrote:
>> From: Cliff Cai <[email protected]>
>> 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 I2C
>
> 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

>> +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.

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

2011-03-07 12:15:09

[permalink] [raw]

Subject: Re: [Device-drivers-devel] [PATCH] Add driver for Analog Devices ADAU1701 SigmaDSP

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.

2011-03-07 12:29:34

[permalink] [raw]

Subject: Re: [Device-drivers-devel] [PATCH] Add driver for Analog Devices ADAU1701 SigmaDSP

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

2011-03-07 14:59:38

[permalink] [raw]

Subject: Re: [Device-drivers-devel] [PATCH] Add driver for Analog Devices ADAU1701 SigmaDSP

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.

2011-03-07 15:33:46

[permalink] [raw]

Subject: Re: [Device-drivers-devel] [PATCH] Add driver for Analog Devices ADAU1701 SigmaDSP

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

2011-03-07 15:55:15

[permalink] [raw]

Subject: Re: [Device-drivers-devel] [PATCH] Add driver for Analog Devices ADAU1701 SigmaDSP

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.

2011-03-09 06:08:27

[permalink] [raw]

Subject: Re: [Device-drivers-devel] [PATCH] Add driver for Analog Devices ADAU1701 SigmaDSP

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

2011-03-09 07:04:09

by Cliff Cai

[permalink] [raw]

Subject: Re: [alsa-devel] [PATCH] Add driver for Analog Devices ADAU1701 SigmaDSP

On Mon, Mar 7, 2011 at 7:41 PM, Mark Brown
<[email protected]> wrote:
> On Mon, Mar 07, 2011 at 09:11:42AM +0800, [email protected] wrote:
>
>> From: Cliff Cai <[email protected]>
>
>> 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
> [email protected]
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
>

2011-03-09 07:25:12

by Cliff Cai

[permalink] [raw]

Subject: Re: [alsa-devel] [Device-drivers-devel] [PATCH] Add driver for Analog Devices ADAU1701 SigmaDSP

On Mon, Mar 7, 2011 at 8:29 PM, Mike Frysinger <[email protected]> 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
> [email protected]
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
>

2011-03-09 07:39:07

by Cliff Cai

[permalink] [raw]

Subject: Re: [alsa-devel] [Device-drivers-devel] [PATCH] Add driver for Analog Devices ADAU1701 SigmaDSP

On Wed, Mar 9, 2011 at 2:08 PM, Mike Frysinger <[email protected]> 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
> [email protected]
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
>

2011-03-09 09:55:12

[permalink] [raw]

Subject: Re: [Device-drivers-devel] [PATCH] Add driver for Analog Devices ADAU1701 SigmaDSP

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.

2011-03-09 10:00:58

[permalink] [raw]

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 <[email protected]> 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.

2011-03-09 10:12:25

[permalink] [raw]

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, [email protected] 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.

2011-03-10 00:36:14

[permalink] [raw]

Subject: Re: [alsa-devel] [Device-drivers-devel] [PATCH] Add driver for Analog Devices ADAU1701 SigmaDSP

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

2011-03-10 09:41:27

[permalink] [raw]

Subject: RE: [alsa-devel] [Device-drivers-devel] [PATCH] Add driver for Analog Devices ADAU1701 SigmaDSP

>-----Original Message-----
>From: Mark Brown [mailto:[email protected]]
>Sent: 2011年3月9日 18:01
>To: Cliff Cai
>Cc: Mike Frysinger; Cai, Cliff; [email protected];
>[email protected];
>[email protected];
>[email protected]; [email protected]
>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
><[email protected]> 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
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay?ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m????????????I?

2011-03-10 10:00:18

[permalink] [raw]

Subject: RE: [alsa-devel] [PATCH] Add driver for Analog Devices ADAU1701 SigmaDSP

>-----Original Message-----
>From: Mark Brown [mailto:[email protected]]
>Sent: 2011年3月9日 18:12
>To: Cliff Cai
>Cc: Cai, Cliff; [email protected];
>[email protected]; [email protected];
>[email protected]; [email protected]
>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,
>[email protected] 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
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay?ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m????????????I?

2011-03-10 13:46:09

[permalink] [raw]

Subject: Re: [alsa-devel] [Device-drivers-devel] [PATCH] Add driver for Analog Devices ADAU1701 SigmaDSP

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).

2011-03-11 07:50:37

[permalink] [raw]

Subject: RE: [alsa-devel] [Device-drivers-devel] [PATCH] Add driver for Analog Devices ADAU1701 SigmaDSP

>-----Original Message-----
>From: Mark Brown [mailto:[email protected]]
>Sent: 2011??3??10?? 21:46
>To: Cai, Cliff
>Cc: Cliff Cai; Mike Frysinger; [email protected];
>[email protected];
>[email protected];
>[email protected]; [email protected]
>Subject: Re: [alsa-devel] [Device-drivers-devel] [PATCH] Add
>driver for Analog Devices ADAU1701 SigmaDSP
>
>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).

1.The default firmware is used when the chip is just powered-up,
Once driver gets the control,it should load the custom firmware to replace
The default one,
2.we don't need to worry about this case,the driver which needs loading firmware
Has to be compiled as a module,and of cause,we load it after the filesystem has been mounted.

Cliff
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay?ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m????????????I?

2011-03-11 11:53:52

[permalink] [raw]

Subject: Re: [alsa-devel] [Device-drivers-devel] [PATCH] Add driver for Analog Devices ADAU1701 SigmaDSP

On Fri, Mar 11, 2011 at 02:54:42AM -0500, Cai, Cliff wrote:

> 1.The default firmware is used when the chip is just powered-up,
> Once driver gets the control,it should load the custom firmware to replace
> The default one,
> 2.we don't need to worry about this case,the driver which needs loading firmware
> Has to be compiled as a module,and of cause,we load it after the filesystem has been mounted.

Forcing the driver to be build modular isn't great for usability either,
though - it's not very general and some systems don't use modules at all
for various reasons (eg, simplicity or difficulties in keeping the fs
and the kernel partitions in sync).

I'm not sure this gels well with the approach you're taking with the
driver just passing the firmware through. On the one hand you don't
want the driver to know anything about the firmware or join up the
firmware handling with the audio subsystem but on the other hand the
driver is requiring that a firmware be loaded as soon as the driver is
even though it can provide some very basic functionality without it.
It'd seem better to just let the user load firmware at their leisure.

2011-03-14 02:18:55

[permalink] [raw]

Subject: RE: [alsa-devel] [Device-drivers-devel] [PATCH] Add driver for Analog Devices ADAU1701 SigmaDSP

>-----Original Message-----
>From: Mark Brown [mailto:[email protected]]
>Sent: 2011??3??11?? 19:54
>To: Cai, Cliff
>Cc: Cliff Cai; Mike Frysinger; [email protected];
>[email protected];
>[email protected];
>[email protected]; [email protected]
>Subject: Re: [alsa-devel] [Device-drivers-devel] [PATCH] Add
>driver for Analog Devices ADAU1701 SigmaDSP
>
>On Fri, Mar 11, 2011 at 02:54:42AM -0500, Cai, Cliff wrote:
>
>> 1.The default firmware is used when the chip is just
>powered-up, Once
>> driver gets the control,it should load the custom firmware
>to replace
>> The default one, 2.we don't need to worry about this case,the driver
>> which needs loading firmware Has to be compiled as a module,and of
>> cause,we load it after the filesystem has been mounted.
>
>Forcing the driver to be build modular isn't great for
>usability either, though - it's not very general and some
>systems don't use modules at all for various reasons (eg,
>simplicity or difficulties in keeping the fs and the kernel
>partitions in sync).
>
>I'm not sure this gels well with the approach you're taking
>with the driver just passing the firmware through. On the one
>hand you don't want the driver to know anything about the
>firmware or join up the firmware handling with the audio
>subsystem but on the other hand the driver is requiring that a
>firmware be loaded as soon as the driver is even though it can
>provide some very basic functionality without it.
>It'd seem better to just let the user load firmware at their leisure.
>

Hi Mark,

I think I need to have a check of the requirement from customer.
Anyway,if I modified the driver according to the comments here,
Would you like to accept it currently or we need to wait until the missing patch
Has been merged in mainline?
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay?ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m????????????I?

2011-03-14 11:29:30

[permalink] [raw]

Subject: Re: [alsa-devel] [Device-drivers-devel] [PATCH] Add driver for Analog Devices ADAU1701 SigmaDSP

On Sun, Mar 13, 2011 at 10:23:19PM -0400, Cai, Cliff wrote:

> I think I need to have a check of the requirement from customer.
> Anyway,if I modified the driver according to the comments here,
> Would you like to accept it currently or we need to wait until the missing patch
> Has been merged in mainline?

Please repost, there were quite a few issues raised with this revision
even outside the firmware. For the purpose of getting things into
mainline I'd recommend splitting out the functionaltiy which allows
firmware to be loaded dynamically at runtime as a second patch.

2011-04-19 00:15:12

by Andres Salomon

[permalink] [raw]

Subject: Re: [Device-drivers-devel] [PATCH] Add driver for Analog Devices ADAU1701 SigmaDSP

On Mon, 7 Mar 2011 14:59:31 +0000
Mark Brown <[email protected]> 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).

2011-04-19 08:06:26