[alsa-devel] [Pull request] Support for wm9705 codec and two machines that use it.
The following changes since commit 6d3455ecb7d564789e8dd493fb876f4a8706e3b2: Mark Brown (1): Fix leftover merge issue.
are available in the git repository at:
git://git.mnementh.co.uk/linux-2.6-im.git asoc
Ian Molton (3): ASoc: codec: Driver for wm9705 AC97 codec. ASoC: machine driver for Toshiba e750 ASoC: machine driver for Toshiba e800
arch/arm/mach-pxa/e750.c | 5 + arch/arm/mach-pxa/include/mach/eseries-gpio.h | 10 + sound/soc/codecs/Kconfig | 4 + sound/soc/codecs/Makefile | 2 + sound/soc/codecs/wm9705.c | 401 ++++++++++++++++++ sound/soc/codecs/wm9705.h | 14 + sound/soc/pxa/Kconfig | 9 + sound/soc/pxa/Makefile | 2 + sound/soc/pxa/e750_wm9705.c | 185 ++++++++++++ sound/soc/pxa/e800_wm9712.c | 115 +++++++- 10 files changed, 733 insertions(+), 14 deletions(-) create mode 100644 sound/soc/codecs/wm9705.c create mode 100644 sound/soc/codecs/wm9705.h create mode 100644 sound/soc/pxa/e750_wm9705.c
Apologies for the misleading subject line - two machines, one using wm9705. the other is wm9712 based.
TTFN.
At Thu, 15 Jan 2009 01:13:52 +0000, Ian Molton wrote:
The following changes since commit 6d3455ecb7d564789e8dd493fb876f4a8706e3b2: Mark Brown (1): Fix leftover merge issue.
are available in the git repository at:
git://git.mnementh.co.uk/linux-2.6-im.git asoc
We need reviews. Could you post patches as well?
thanks,
Takashi
Ian Molton (3): ASoc: codec: Driver for wm9705 AC97 codec. ASoC: machine driver for Toshiba e750 ASoC: machine driver for Toshiba e800
arch/arm/mach-pxa/e750.c | 5 + arch/arm/mach-pxa/include/mach/eseries-gpio.h | 10 + sound/soc/codecs/Kconfig | 4 + sound/soc/codecs/Makefile | 2 + sound/soc/codecs/wm9705.c | 401 ++++++++++++++++++ sound/soc/codecs/wm9705.h | 14 + sound/soc/pxa/Kconfig | 9 + sound/soc/pxa/Makefile | 2 + sound/soc/pxa/e750_wm9705.c | 185 ++++++++++++ sound/soc/pxa/e800_wm9712.c | 115 +++++++- 10 files changed, 733 insertions(+), 14 deletions(-) create mode 100644 sound/soc/codecs/wm9705.c create mode 100644 sound/soc/codecs/wm9705.h create mode 100644 sound/soc/pxa/e750_wm9705.c _______________________________________________ Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
Takashi Iwai wrote:
We need reviews. Could you post patches as well?
Sure - attached below:
I also noticed that I forgot to run it by checkpatch. It wasn't bad, but now checkpatch is silent.
Side note: I wish diffstat on these patches would keep lines under 80 chars...
sound/soc/codecs/Kconfig | 4 + sound/soc/codecs/Makefile | 2 + sound/soc/codecs/wm9705.c | 401 +++++++++++++++++++++++++++++++++++++++++++++ sound/soc/codecs/wm9705.h | 14 ++ 4 files changed, 421 insertions(+), 0 deletions(-) create mode 100644 sound/soc/codecs/wm9705.c create mode 100644 sound/soc/codecs/wm9705.h
diff --git a/sound/soc/codecs/Kconfig b/sound/soc/codecs/Kconfig index b32a2b5..9f33c07 100644 --- a/sound/soc/codecs/Kconfig +++ b/sound/soc/codecs/Kconfig @@ -46,6 +46,7 @@ config SND_SOC_ALL_CODECS select SND_SOC_WM8980 if I2C select SND_SOC_WM8990 if I2C select SND_SOC_WM8991 if I2C + select SND_SOC_WM9705 if SND_SOC_AC97_BUS select SND_SOC_WM9712 if SND_SOC_AC97_BUS select SND_SOC_WM9713 if SND_SOC_AC97_BUS help @@ -190,6 +191,9 @@ config SND_SOC_WM8990 config SND_SOC_WM8991 tristate
+config SND_SOC_WM9705 + tristate + config SND_SOC_WM9712 tristate
diff --git a/sound/soc/codecs/Makefile b/sound/soc/codecs/Makefile index 0a0c9dd..9c61037 100644 --- a/sound/soc/codecs/Makefile +++ b/sound/soc/codecs/Makefile @@ -37,6 +37,7 @@ snd-soc-wm8978-objs := wm8978.o snd-soc-wm8980-objs := wm8980.o snd-soc-wm8990-objs := wm8990.o snd-soc-wm8991-objs := wm8991.o +snd-soc-wm9705-objs := wm9705.o snd-soc-wm9712-objs := wm9712.o snd-soc-wm9713-objs := wm9713.o
@@ -78,5 +79,6 @@ obj-$(CONFIG_SND_SOC_WM8978) += snd-soc-wm8978.o obj-$(CONFIG_SND_SOC_WM8980) += snd-soc-wm8980.o obj-$(CONFIG_SND_SOC_WM8990) += snd-soc-wm8990.o obj-$(CONFIG_SND_SOC_WM8991) += snd-soc-wm8991.o +obj-$(CONFIG_SND_SOC_WM9705) += snd-soc-wm9705.o obj-$(CONFIG_SND_SOC_WM9712) += snd-soc-wm9712.o obj-$(CONFIG_SND_SOC_WM9713) += snd-soc-wm9713.o diff --git a/sound/soc/codecs/wm9705.c b/sound/soc/codecs/wm9705.c new file mode 100644 index 0000000..4ff6a84 --- /dev/null +++ b/sound/soc/codecs/wm9705.c @@ -0,0 +1,401 @@ +/* + * wm9705.c -- ALSA Soc WM9705 codec support + * + * Copyright 2008 Ian Molton spyro@f2s.com + * + * This program is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License as published by the + * Free Software Foundation; Version 2 of the License only. + * + */ + +#include <linux/init.h> +#include <linux/module.h> +#include <linux/version.h> +#include <linux/kernel.h> +#include <linux/device.h> +#include <sound/core.h> +#include <sound/pcm.h> +#include <sound/ac97_codec.h> +#include <sound/initval.h> +#include <sound/soc.h> +#include <sound/soc-dapm.h> + +/* + * WM9705 register cache + */ +static const u16 wm9705_reg[] = { + 0x6174, 0x8000, 0x8000, 0x8000, /* 0x0 */ + 0x0000, 0x8000, 0x8008, 0x8008, /* 0x8 */ + 0x8808, 0x8808, 0x8808, 0x8808, /* 0x10 */ + 0x8808, 0x0000, 0x8000, 0x0000, /* 0x18 */ + 0x0000, 0x0000, 0x0000, 0x000f, /* 0x20 */ + 0x0605, 0x0000, 0xbb80, 0x0000, /* 0x28 */ + 0x0000, 0xbb80, 0x0000, 0x0000, /* 0x30 */ + 0x0000, 0x2000, 0x0000, 0x0000, /* 0x38 */ + 0x0000, 0x0000, 0x0000, 0x0000, /* 0x40 */ + 0x0000, 0x0000, 0x0000, 0x0000, /* 0x48 */ + 0x0000, 0x0000, 0x0000, 0x0000, /* 0x50 */ + 0x0000, 0x0000, 0x0000, 0x0000, /* 0x58 */ + 0x0000, 0x0000, 0x0000, 0x0000, /* 0x60 */ + 0x0000, 0x0000, 0x0000, 0x0000, /* 0x68 */ + 0x0000, 0x0808, 0x0000, 0x0006, /* 0x70 */ + 0x0000, 0x0000, 0x574d, 0x4c05, /* 0x78 */ +}; + +static const struct snd_kcontrol_new wm9705_snd_ac97_controls[] = { + SOC_DOUBLE("Master Playback Volume", AC97_MASTER, 8, 0, 31, 1), + SOC_SINGLE("Master Playback Switch", AC97_MASTER, 15, 1, 1), + SOC_DOUBLE("Headphone Playback Volume", AC97_HEADPHONE, 8, 0, 31, 1), + SOC_SINGLE("Headphone Playback Switch", AC97_HEADPHONE, 15, 1, 1), + SOC_DOUBLE("PCM Playback Volume", AC97_PCM, 8, 0, 31, 1), + SOC_SINGLE("PCM Playback Switch", AC97_PCM, 15, 1, 1), + SOC_SINGLE("Mono Playback Volume", AC97_MASTER_MONO, 0, 31, 1), + SOC_SINGLE("Mono Playback Switch", AC97_MASTER_MONO, 15, 1, 1), + SOC_SINGLE("PCBeep Playback Volume", AC97_PC_BEEP, 1, 15, 1), + SOC_SINGLE("Phone Playback Volume", AC97_PHONE, 0, 31, 1), + SOC_DOUBLE("Line Playback Volume", AC97_LINE, 8, 0, 31, 1), + SOC_DOUBLE("CD Playback Volume", AC97_CD, 8, 0, 31, 1), + SOC_SINGLE("Mic Playback Volume", AC97_MIC, 0, 31, 1), + SOC_SINGLE("Mic 20dB Boost Switch", AC97_MIC, 6, 1, 0), + SOC_DOUBLE("PCM Capture Volume", AC97_REC_GAIN, 8, 0, 15, 0), + SOC_SINGLE("PCM Capture Switch", AC97_REC_GAIN, 15, 1, 1), +}; + +static const char *wm9705_mic[] = {"Mic 1", "Mic 2"}; +static const char *wm9705_rec_sel[] = {"Mic", "CD", "NC", "NC", + "Line", "Stereo Mix", "Mono Mix", "Phone"}; + +static const struct soc_enum wm9705_enum_mic = + SOC_ENUM_SINGLE(AC97_GENERAL_PURPOSE, 8, 2, wm9705_mic); +static const struct soc_enum wm9705_enum_rec_l = + SOC_ENUM_SINGLE(AC97_REC_SEL, 8, 8, wm9705_rec_sel); +static const struct soc_enum wm9705_enum_rec_r = + SOC_ENUM_SINGLE(AC97_REC_SEL, 0, 8, wm9705_rec_sel); + +/* Headphone Mixer */ +static const struct snd_kcontrol_new wm9705_hp_mixer_controls[] = { + SOC_DAPM_SINGLE("PCBeep Playback Switch", AC97_PC_BEEP, 15, 1, 1), + SOC_DAPM_SINGLE("CD Playback Switch", AC97_CD, 15, 1, 1), + SOC_DAPM_SINGLE("Mic Playback Switch", AC97_MIC, 15, 1, 1), + SOC_DAPM_SINGLE("Phone Playback Switch", AC97_PHONE, 15, 1, 1), + SOC_DAPM_SINGLE("Line Playback Switch", AC97_LINE, 15, 1, 1), +}; + +/* Mic source */ +static const struct snd_kcontrol_new wm9705_mic_src_controls = + SOC_DAPM_ENUM("Route", wm9705_enum_mic); + +/* Capture source */ +static const struct snd_kcontrol_new wm9705_capture_selectl_controls = + SOC_DAPM_ENUM("Route", wm9705_enum_rec_l); +static const struct snd_kcontrol_new wm9705_capture_selectr_controls = + SOC_DAPM_ENUM("Route", wm9705_enum_rec_r); + +/* DAPM widgets */ +static const struct snd_soc_dapm_widget wm9705_dapm_widgets[] = { + SND_SOC_DAPM_MUX("Mic Source", SND_SOC_NOPM, 0, 0, + &wm9705_mic_src_controls), + SND_SOC_DAPM_MUX("Left Capture Source", SND_SOC_NOPM, 0, 0, + &wm9705_capture_selectl_controls), + SND_SOC_DAPM_MUX("Right Capture Source", SND_SOC_NOPM, 0, 0, + &wm9705_capture_selectr_controls), + SND_SOC_DAPM_DAC("Left DAC", "Left HiFi Playback", + SND_SOC_NOPM, 0, 0), + SND_SOC_DAPM_DAC("Right DAC", "Right HiFi Playback", + SND_SOC_NOPM, 0, 0), + SND_SOC_DAPM_MIXER_NAMED_CTL("HP Mixer", SND_SOC_NOPM, 0, 0, + &wm9705_hp_mixer_controls[0], + ARRAY_SIZE(wm9705_hp_mixer_controls)), + SND_SOC_DAPM_MIXER("Mono Mixer", SND_SOC_NOPM, 0, 0, NULL, 0), + SND_SOC_DAPM_ADC("Left ADC", "Left HiFi Capture", SND_SOC_NOPM, 0, 0), + SND_SOC_DAPM_ADC("Right ADC", "Right HiFi Capture", SND_SOC_NOPM, 0, 0), + SND_SOC_DAPM_PGA("Headphone PGA", SND_SOC_NOPM, 0, 0, NULL, 0), + SND_SOC_DAPM_PGA("Speaker PGA", SND_SOC_NOPM, 0, 0, NULL, 0), + SND_SOC_DAPM_PGA("Line PGA", SND_SOC_NOPM, 0, 0, NULL, 0), + SND_SOC_DAPM_PGA("Line out PGA", SND_SOC_NOPM, 0, 0, NULL, 0), + SND_SOC_DAPM_PGA("Mono PGA", SND_SOC_NOPM, 0, 0, NULL, 0), + SND_SOC_DAPM_PGA("Phone PGA", SND_SOC_NOPM, 0, 0, NULL, 0), + SND_SOC_DAPM_PGA("Mic PGA", SND_SOC_NOPM, 0, 0, NULL, 0), + SND_SOC_DAPM_PGA("PCBEEP PGA", SND_SOC_NOPM, 0, 0, NULL, 0), + SND_SOC_DAPM_PGA("CD PGA", SND_SOC_NOPM, 0, 0, NULL, 0), + SND_SOC_DAPM_PGA("ADC PGA", SND_SOC_NOPM, 0, 0, NULL, 0), + SND_SOC_DAPM_OUTPUT("HPOUTL"), + SND_SOC_DAPM_OUTPUT("HPOUTR"), + SND_SOC_DAPM_OUTPUT("LOUT"), + SND_SOC_DAPM_OUTPUT("ROUT"), + SND_SOC_DAPM_OUTPUT("MONOOUT"), + SND_SOC_DAPM_INPUT("PHONE"), + SND_SOC_DAPM_INPUT("LINEINL"), + SND_SOC_DAPM_INPUT("LINEINR"), + SND_SOC_DAPM_INPUT("CDINL"), + SND_SOC_DAPM_INPUT("CDINR"), + SND_SOC_DAPM_INPUT("PCBEEP"), + SND_SOC_DAPM_INPUT("MIC1"), + SND_SOC_DAPM_INPUT("MIC2"), +}; + +/* Audio map + * WM9705 has no switches to disable the route from the inputs to the HP mixer + * so in order to prevent active inputs from forcing the audio outputs to be + * constantly enabled, we use the mutes on those inputs to simulate such + * controls. + */ +static const struct snd_soc_dapm_route audio_map[] = { + /* HP mixer */ + {"HP Mixer", "PCBeep Playback Switch", "PCBEEP PGA"}, + {"HP Mixer", "CD Playback Switch", "CD PGA"}, + {"HP Mixer", "Mic Playback Switch", "Mic PGA"}, + {"HP Mixer", "Phone Playback Switch", "Phone PGA"}, + {"HP Mixer", "Line Playback Switch", "Line PGA"}, + {"HP Mixer", NULL, "Left DAC"}, + {"HP Mixer", NULL, "Right DAC"}, + + /* mono mixer */ + {"Mono Mixer", NULL, "HP Mixer"}, + + /* outputs */ + {"Headphone PGA", NULL, "HP Mixer"}, + {"HPOUTL", NULL, "Headphone PGA"}, + {"HPOUTR", NULL, "Headphone PGA"}, + {"Line out PGA", NULL, "HP Mixer"}, + {"LOUT", NULL, "Line out PGA"}, + {"ROUT", NULL, "Line out PGA"}, + {"Mono PGA", NULL, "Mono Mixer"}, + {"MONOOUT", NULL, "Mono PGA"}, + + /* inputs */ + {"CD PGA", NULL, "CDINL"}, + {"CD PGA", NULL, "CDINR"}, + {"Line PGA", NULL, "LINEINL"}, + {"Line PGA", NULL, "LINEINR"}, + {"Phone PGA", NULL, "PHONE"}, + {"Mic Source", "Mic 1", "MIC1"}, + {"Mic Source", "Mic 2", "MIC2"}, + {"Mic PGA", NULL, "Mic Source"}, + {"PCBEEP PGA", NULL, "PCBEEP"}, + + /* Left capture selector */ + {"Left Capture Source", "Mic", "Mic Source"}, + {"Left Capture Source", "CD", "CDINL"}, + {"Left Capture Source", "Line", "LINEINL"}, + {"Left Capture Source", "Stereo Mix", "HP Mixer"}, + {"Left Capture Source", "Mono Mix", "HP Mixer"}, + {"Left Capture Source", "Phone", "PHONE"}, + + /* Right capture source */ + {"Right Capture Source", "Mic", "Mic Source"}, + {"Right Capture Source", "CD", "CDINR"}, + {"Right Capture Source", "Line", "LINEINR"}, + {"Right Capture Source", "Stereo Mix", "HP Mixer"}, + {"Right Capture Source", "Mono Mix", "HP Mixer"}, + {"Right Capture Source", "Phone", "PHONE"}, + + {"ADC PGA", NULL, "Left Capture Source"}, + {"ADC PGA", NULL, "Right Capture Source"}, + + /* ADC's */ + {"Left ADC", NULL, "ADC PGA"}, + {"Right ADC", NULL, "ADC PGA"}, +}; + +static int wm9705_add_widgets(struct snd_soc_codec *codec) +{ + snd_soc_dapm_new_controls(codec, wm9705_dapm_widgets, + ARRAY_SIZE(wm9705_dapm_widgets)); + snd_soc_dapm_add_routes(codec, audio_map, ARRAY_SIZE(audio_map)); + snd_soc_dapm_new_widgets(codec); + + return 0; +} + +/* We use a register cache to enhance read performance. */ +static unsigned int ac97_read(struct snd_soc_codec *codec, unsigned int reg) +{ + u16 *cache = codec->reg_cache; + + switch (reg) { + case AC97_RESET: + case AC97_VENDOR_ID1: + case AC97_VENDOR_ID2: + return soc_ac97_ops.read(codec->ac97, reg); + default: + reg = reg >> 1; + + if (reg > (ARRAY_SIZE(wm9705_reg))) + return -EIO; + + return cache[reg]; + } +} + +static int ac97_write(struct snd_soc_codec *codec, unsigned int reg, + unsigned int val) +{ + u16 *cache = codec->reg_cache; + + soc_ac97_ops.write(codec->ac97, reg, val); + reg = reg >> 1; + if (reg <= (ARRAY_SIZE(wm9705_reg))) + cache[reg] = val; + + return 0; +} + +static int ac97_prepare(struct snd_pcm_substream *substream, + struct snd_soc_dai *dai) +{ + struct snd_pcm_runtime *runtime = substream->runtime; + struct snd_soc_pcm_runtime *rtd = substream->private_data; + struct snd_soc_device *socdev = rtd->socdev; + struct snd_soc_codec *codec = socdev->codec; + int reg; + u16 vra; + + vra = ac97_read(codec, AC97_EXTENDED_STATUS); + ac97_write(codec, AC97_EXTENDED_STATUS, vra | 0x1); + + if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) + reg = AC97_PCM_FRONT_DAC_RATE; + else + reg = AC97_PCM_LR_ADC_RATE; + + return ac97_write(codec, reg, runtime->rate); +} + +#define WM9705_AC97_RATES (SNDRV_PCM_RATE_8000 | SNDRV_PCM_RATE_11025 | \ + SNDRV_PCM_RATE_16000 | SNDRV_PCM_RATE_22050 | \ + SNDRV_PCM_RATE_32000 | SNDRV_PCM_RATE_44100 | \ + SNDRV_PCM_RATE_48000) + +struct snd_soc_dai wm9705_dai[] = { + { + .name = "AC97 HiFi", + .ac97_control = 1, + .playback = { + .stream_name = "HiFi Playback", + .channels_min = 1, + .channels_max = 2, + .rates = WM9705_AC97_RATES, + .formats = SNDRV_PCM_FMTBIT_S16_LE, + }, + .capture = { + .stream_name = "HiFi Capture", + .channels_min = 1, + .channels_max = 2, + .rates = WM9705_AC97_RATES, + .formats = SNDRV_PCM_FMTBIT_S16_LE, + }, + .ops = { + .prepare = ac97_prepare, + }, + }, + { + .name = "AC97 Aux", + .playback = { + .stream_name = "Aux Playback", + .channels_min = 1, + .channels_max = 1, + .rates = WM9705_AC97_RATES, + .formats = SNDRV_PCM_FMTBIT_S16_LE, + }, + } +}; +EXPORT_SYMBOL_GPL(wm9705_dai); + +static int wm9705_soc_probe(struct platform_device *pdev) +{ + struct snd_soc_device *socdev = platform_get_drvdata(pdev); + struct snd_soc_codec *codec; + int ret = 0; + + printk(KERN_INFO "WM9705 SoC Audio Codec\n"); + + socdev->codec = kzalloc(sizeof(struct snd_soc_codec), GFP_KERNEL); + if (socdev->codec == NULL) + return -ENOMEM; + codec = socdev->codec; + mutex_init(&codec->mutex); + + codec->reg_cache = + kzalloc(sizeof(u16) * ARRAY_SIZE(wm9705_reg), GFP_KERNEL); + if (codec->reg_cache == NULL) { + ret = -ENOMEM; + goto cache_err; + } + memcpy(codec->reg_cache, wm9705_reg, + sizeof(u16) * ARRAY_SIZE(wm9705_reg)); + codec->reg_cache_size = sizeof(u16) * ARRAY_SIZE(wm9705_reg); + codec->reg_cache_step = 2; + + codec->name = "WM9705"; + codec->owner = THIS_MODULE; + codec->dai = wm9705_dai; + codec->num_dai = ARRAY_SIZE(wm9705_dai); + codec->write = ac97_write; + codec->read = ac97_read; + INIT_LIST_HEAD(&codec->dapm_widgets); + INIT_LIST_HEAD(&codec->dapm_paths); + + ret = snd_soc_new_ac97_codec(codec, &soc_ac97_ops, 0); + if (ret < 0) { + printk(KERN_ERR "wm9705: failed to register AC97 codec\n"); + goto codec_err; + } + + /* register pcms */ + ret = snd_soc_new_pcms(socdev, SNDRV_DEFAULT_IDX1, SNDRV_DEFAULT_STR1); + if (ret < 0) + goto pcm_err; + + soc_ac97_ops.reset(codec->ac97); + + snd_soc_add_controls(codec, wm9705_snd_ac97_controls, + ARRAY_SIZE(wm9705_snd_ac97_controls)); + wm9705_add_widgets(codec); + + ret = snd_soc_init_card(socdev); + if (ret < 0) { + printk(KERN_ERR "wm9705: failed to register card\n"); + goto pcm_err; + } + + return 0; + +pcm_err: + snd_soc_free_ac97_codec(codec); + +codec_err: + kfree(codec->reg_cache); + +cache_err: + kfree(socdev->codec); + socdev->codec = NULL; + return ret; +} + +static int wm9705_soc_remove(struct platform_device *pdev) +{ + struct snd_soc_device *socdev = platform_get_drvdata(pdev); + struct snd_soc_codec *codec = socdev->codec; + + if (codec == NULL) + return 0; + + snd_soc_dapm_free(socdev); + snd_soc_free_pcms(socdev); + snd_soc_free_ac97_codec(codec); + kfree(codec->reg_cache); + kfree(codec); + return 0; +} + +struct snd_soc_codec_device soc_codec_dev_wm9705 = { + .probe = wm9705_soc_probe, + .remove = wm9705_soc_remove, +}; +EXPORT_SYMBOL_GPL(soc_codec_dev_wm9705); + +MODULE_DESCRIPTION("ASoC WM9705 driver"); +MODULE_AUTHOR("Ian Molton"); +MODULE_LICENSE("GPL v2"); diff --git a/sound/soc/codecs/wm9705.h b/sound/soc/codecs/wm9705.h new file mode 100644 index 0000000..0f46e66 --- /dev/null +++ b/sound/soc/codecs/wm9705.h @@ -0,0 +1,14 @@ +/* + * wm9705.h -- WM9705 Soc Audio driver + */ + +#ifndef _WM9705_H +#define _WM9705_H + +#define WM9705_DAI_AC97_HIFI 0 +#define WM9705_DAI_AC97_AUX 1 + +extern struct snd_soc_dai wm9705_dai[2]; +extern struct snd_soc_codec_device soc_codec_dev_wm9705; + +#endif
Only one comment from me...
On Thu, Jan 15, 2009 at 10:06:53AM +0000, Ian Molton wrote:
diff --git a/sound/soc/codecs/wm9705.c b/sound/soc/codecs/wm9705.c new file mode 100644 index 0000000..4ff6a84 --- /dev/null +++ b/sound/soc/codecs/wm9705.c @@ -0,0 +1,401 @@ +/*
- wm9705.c -- ALSA Soc WM9705 codec support
- Copyright 2008 Ian Molton spyro@f2s.com
- This program is free software; you can redistribute it and/or
modify it
- under the terms of the GNU General Public License as published
by the
- Free Software Foundation; Version 2 of the License only.
- */
+#include <linux/init.h> +#include <linux/module.h> +#include <linux/version.h>
Please do not include linux/version.h except where it is really needed (I don't see anything requiring it in this file.)
On Thu, Jan 15, 2009 at 10:06:53AM +0000, Ian Molton wrote:
Takashi Iwai wrote:
We need reviews. Could you post patches as well?
Sure - attached below:
Always do this for ALSA patch submissions (the same thing will apply to the majority of kernel subsystems). Please also CC at least me on ASoC patches. I do read the list but it helps to make sure things don't get missed.
These all look fine apart from one small thing in the codec driver and the issue Russell already pointed out.
- soc_ac97_ops.reset(codec->ac97);
This should check the return value, resets can and do fail.
Mark Brown wrote:
Always do this for ALSA patch submissions (the same thing will apply to the majority of kernel subsystems). Please also CC at least me on ASoC patches.
My bad, sorry.
I do read the list but it helps to make sure things don't get missed.
Sure.
These all look fine apart from one small thing in the codec driver
- soc_ac97_ops.reset(codec->ac97);
This should check the return value, resets can and do fail.
This call returns void. So no.
I cant see a way to determine the outcome of a reset on wm9705. I could program a register, then check to see it is set to its reset value, but if the codec is hung, this could cause lockups.
Russell's isue has been fixed.
TTFN!
-Ian
On Thu, Jan 15, 2009 at 10:16:10PM +0000, Ian Molton wrote:
Mark Brown wrote:
- soc_ac97_ops.reset(codec->ac97);
This should check the return value, resets can and do fail.
This call returns void. So no.
Oh, sorry - thinko. You should read back the ID register and verify the value.
I cant see a way to determine the outcome of a reset on wm9705. I could program a register, then check to see it is set to its reset value, but if the codec is hung, this could cause lockups.
It shouldn't, the AC97 bus driver should time out. The goal is to make sure that the codec is live and the expected device.
Mark Brown wrote:
On Thu, Jan 15, 2009 at 10:16:10PM +0000, Ian Molton wrote:
Mark Brown wrote:
- soc_ac97_ops.reset(codec->ac97);
This should check the return value, resets can and do fail.
This call returns void. So no.
Oh, sorry - thinko. You should read back the ID register and verify the value.
Implemented, tested, checkpatch passed.
Pushed out and available at:
git://git.mnementh.co.uk/linux-2.6-im.git asoc
gitweb at:
http://git.mnementh.co.uk/cgi-bin/gitweb.cgi?p=linux-2.6-im.git;a=shortlog;h...
Thanks for the review,
-Ian
On Thu, Jan 15, 2009 at 11:58:36PM +0000, Ian Molton wrote:
Pushed out and available at:
git://git.mnementh.co.uk/linux-2.6-im.git asoc
Err... that branch is based off the ASoC dev branch so can't be pulled for mainline. I've cherry picked the patches over since the conflicts were trivial but if you are going to send pull requests please base things for upstream off the ASoC branch of Takashi's repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound-2.6.git
As previously mentioned you really should post patches rather than just pull requests (pull requests are fine but you should post the patches as well) - it really is much easier to work with.
At Thu, 15 Jan 2009 10:06:53 +0000, Ian Molton wrote:
+static unsigned int ac97_read(struct snd_soc_codec *codec, unsigned int reg) +{
- u16 *cache = codec->reg_cache;
- switch (reg) {
- case AC97_RESET:
- case AC97_VENDOR_ID1:
- case AC97_VENDOR_ID2:
return soc_ac97_ops.read(codec->ac97, reg);- default:
reg = reg >> 1;if (reg > (ARRAY_SIZE(wm9705_reg)))return -EIO;
Shouldn't be this "reg >= ARRAY_SIZE(wm9705_reg)" ?
+static int ac97_write(struct snd_soc_codec *codec, unsigned int reg,
- unsigned int val)
+{
- u16 *cache = codec->reg_cache;
- soc_ac97_ops.write(codec->ac97, reg, val);
- reg = reg >> 1;
- if (reg <= (ARRAY_SIZE(wm9705_reg)))
cache[reg] = val;
Ditto, should be "reg < ARRAY_SIZE(wm9705_reg)".
+static int wm9705_soc_probe(struct platform_device *pdev) +{
- struct snd_soc_device *socdev = platform_get_drvdata(pdev);
- struct snd_soc_codec *codec;
- int ret = 0;
- printk(KERN_INFO "WM9705 SoC Audio Codec\n");
- socdev->codec = kzalloc(sizeof(struct snd_soc_codec), GFP_KERNEL);
- if (socdev->codec == NULL)
return -ENOMEM;- codec = socdev->codec;
- mutex_init(&codec->mutex);
- codec->reg_cache =
kzalloc(sizeof(u16) * ARRAY_SIZE(wm9705_reg), GFP_KERNEL);- if (codec->reg_cache == NULL) {
ret = -ENOMEM;goto cache_err;- }
- memcpy(codec->reg_cache, wm9705_reg,
sizeof(u16) * ARRAY_SIZE(wm9705_reg));
You can use kmemdup() here.
thanks,
Takashi
Takashi Iwai wrote:
At Thu, 15 Jan 2009 10:06:53 +0000, Ian Molton wrote:
Shouldn't be this "reg >= ARRAY_SIZE(wm9705_reg)" ?
Looks that way. Will fix.
I've spotted the same error in a couple of other drivers too.
It looks like register caching is a very common thing, would you be interested in a patch that consolidates the cache handling in soc-core, rather than having multiple possibly broken implementations around?
Also, I could be wrong, but wm8980 caching looks completely broken (array is type u16 but there is no register shift applied, AFAICT)
fixed in wm9705 for now anyway.
Ditto, should be "reg < ARRAY_SIZE(wm9705_reg)".
Fixed.
You can use kmemdup() here.
Changed.
TTFN,
-Ian
On Fri, Jan 16, 2009 at 11:34:41AM +0000, Ian Molton wrote:
It looks like register caching is a very common thing, would you be interested in a patch that consolidates the cache handling in soc-core, rather than having multiple possibly broken implementations around?
There's gotchas with variable register sizes and volatile bits (this was originally done more in the core but pushed out). I've looked at it before and it looked like the best way to handle it was to do it along with a factoring out of common I/O routines. Possibly best to leave it for now.
Also, I could be wrong, but wm8980 caching looks completely broken (array is type u16 but there is no register shift applied, AFAICT)
There's normally a reason for the non-mainline drivers being that way but in this case that's not it - the chip has 16 bit registers and the cache is being worked with as u16 * so natural array indexing should DTRT.
Please post an incremental patch with your other changes.
Mark Brown wrote:
On Fri, Jan 16, 2009 at 11:34:41AM +0000, Ian Molton wrote:
It looks like register caching is a very common thing, would you be interested in a patch that consolidates the cache handling in soc-core, rather than having multiple possibly broken implementations around?
There's gotchas with variable register sizes and volatile bits (this was originally done more in the core but pushed out). I've looked at it before and it looked like the best way to handle it was to do it along with a factoring out of common I/O routines. Possibly best to leave it for now.
It looks to me like most chips could be handled with little difficulty - a reg mask for some chips that have eg. 9 bit registers represented in a u16, a reg_step to allow easy handling of different reg sizes, and an array of register numbers for which the core should not cache values.
Easily 10 chips presently supported could use that scheme, whilst leaving the function pointer based system available for the few really weird chips.
Also, I could be wrong, but wm8980 caching looks completely broken (array is type u16 but there is no register shift applied, AFAICT)
There's normally a reason for the non-mainline drivers being that way but in this case that's not it - the chip has 16 bit registers and the cache is being worked with as u16 * so natural array indexing should DTRT.
I cant quickly check that chip that now because I've rebased to the for-tiwai branch which it isnt in.
What does seem to be clear though is that there seems to be some confusion as to what the 'reg' parameter to codec->read should be - should it be a register _address_, or a register _number_.
If the former, then the caching in my wm9705 and a few other drivers is broken. These treat reg as a register _address_ and shift it right by 1 to get the reg number and thus index into the reg cache.
If the latter, then other drivers may be making flawed assumptions, eg. wm8753 which uses an array of u16 for the cache and treats 'reg' as a register _number_ and uses it directly to index it, without a shift.
Please post an incremental patch with your other changes.
Attached below for review.
I haven't added my SoB: as I havent tested it.
As you can see, the cache handling is done in several ways by different codecs, with inconsistent semantics and return codes (some BUG, some return 1 some -1 some -EIO. Some wont allow undefined reg access, some do, but only uncached.) This seems like ripe territory for bugs (albeit fairly harmless ones, but bugs nontheless).
ac97.c seems to have cookie-cutter-copied code to free a cache that it doesnt even have...
From 93188b0b342bd849bc6aa62dca4ed33f71fcba0e Mon Sep 17 00:00:00 2001 From: Ian Molton ian@mnementh.co.uk Date: Fri, 16 Jan 2009 13:31:52 +0000 Subject: [PATCH] ASoC: fixes to caching implementations
This patch takes fixes a number of bugs in the caching code used by several ASoC codec drivers. Mostly off-by-one fixes.
--- sound/soc/codecs/ac97.c | 2 -- sound/soc/codecs/ad1980.c | 4 ++-- sound/soc/codecs/twl4030.c | 3 +++ sound/soc/codecs/wm8580.c | 4 ++-- sound/soc/codecs/wm8728.c | 4 ++-- sound/soc/codecs/wm8753.c | 4 ++-- sound/soc/codecs/wm8990.c | 4 ++-- sound/soc/codecs/wm9705.c | 4 ++-- sound/soc/codecs/wm9712.c | 4 ++-- sound/soc/codecs/wm9713.c | 4 ++-- 10 files changed, 19 insertions(+), 18 deletions(-)
diff --git a/sound/soc/codecs/ac97.c b/sound/soc/codecs/ac97.c index fb53e65..89d4127 100644 --- a/sound/soc/codecs/ac97.c +++ b/sound/soc/codecs/ac97.c @@ -123,7 +123,6 @@ bus_err: snd_soc_free_pcms(socdev);
err: - kfree(socdev->codec->reg_cache); kfree(socdev->codec); socdev->codec = NULL; return ret; @@ -138,7 +137,6 @@ static int ac97_soc_remove(struct platform_device *pdev) return 0;
snd_soc_free_pcms(socdev); - kfree(socdev->codec->reg_cache); kfree(socdev->codec);
return 0; diff --git a/sound/soc/codecs/ad1980.c b/sound/soc/codecs/ad1980.c index c3c5d0e..faf3587 100644 --- a/sound/soc/codecs/ad1980.c +++ b/sound/soc/codecs/ad1980.c @@ -109,7 +109,7 @@ static unsigned int ac97_read(struct snd_soc_codec *codec, default: reg = reg >> 1;
- if (reg >= (ARRAY_SIZE(ad1980_reg))) + if (reg >= ARRAY_SIZE(ad1980_reg)) return -EINVAL;
return cache[reg]; @@ -123,7 +123,7 @@ static int ac97_write(struct snd_soc_codec *codec, unsigned int reg,
soc_ac97_ops.write(codec->ac97, reg, val); reg = reg >> 1; - if (reg < (ARRAY_SIZE(ad1980_reg))) + if (reg < ARRAY_SIZE(ad1980_reg)) cache[reg] = val;
return 0; diff --git a/sound/soc/codecs/twl4030.c b/sound/soc/codecs/twl4030.c index ddc9f37..f530c1e 100644 --- a/sound/soc/codecs/twl4030.c +++ b/sound/soc/codecs/twl4030.c @@ -125,6 +125,9 @@ static inline unsigned int twl4030_read_reg_cache(struct snd_soc_codec *codec, { u8 *cache = codec->reg_cache;
+ if (reg >= TWL4030_CACHEREGNUM) + return -EIO; + return cache[reg]; }
diff --git a/sound/soc/codecs/wm8580.c b/sound/soc/codecs/wm8580.c index 9b75a37..3faf0e7 100644 --- a/sound/soc/codecs/wm8580.c +++ b/sound/soc/codecs/wm8580.c @@ -200,7 +200,7 @@ static inline unsigned int wm8580_read_reg_cache(struct snd_soc_codec *codec, unsigned int reg) { u16 *cache = codec->reg_cache; - BUG_ON(reg > ARRAY_SIZE(wm8580_reg)); + BUG_ON(reg >= ARRAY_SIZE(wm8580_reg)); return cache[reg]; }
@@ -223,7 +223,7 @@ static int wm8580_write(struct snd_soc_codec *codec, unsigned int reg, { u8 data[2];
- BUG_ON(reg > ARRAY_SIZE(wm8580_reg)); + BUG_ON(reg >= ARRAY_SIZE(wm8580_reg));
/* Registers are 9 bits wide */ value &= 0x1ff; diff --git a/sound/soc/codecs/wm8728.c b/sound/soc/codecs/wm8728.c index defa310..f90dc52 100644 --- a/sound/soc/codecs/wm8728.c +++ b/sound/soc/codecs/wm8728.c @@ -47,7 +47,7 @@ static inline unsigned int wm8728_read_reg_cache(struct snd_soc_codec *codec, unsigned int reg) { u16 *cache = codec->reg_cache; - BUG_ON(reg > ARRAY_SIZE(wm8728_reg_defaults)); + BUG_ON(reg >= ARRAY_SIZE(wm8728_reg_defaults)); return cache[reg]; }
@@ -55,7 +55,7 @@ static inline void wm8728_write_reg_cache(struct snd_soc_codec *codec, u16 reg, unsigned int value) { u16 *cache = codec->reg_cache; - BUG_ON(reg > ARRAY_SIZE(wm8728_reg_defaults)); + BUG_ON(reg >= ARRAY_SIZE(wm8728_reg_defaults)); cache[reg] = value; }
diff --git a/sound/soc/codecs/wm8753.c b/sound/soc/codecs/wm8753.c index 7283178..5a1c1fc 100644 --- a/sound/soc/codecs/wm8753.c +++ b/sound/soc/codecs/wm8753.c @@ -97,7 +97,7 @@ static inline unsigned int wm8753_read_reg_cache(struct snd_soc_codec *codec, unsigned int reg) { u16 *cache = codec->reg_cache; - if (reg < 1 || reg > (ARRAY_SIZE(wm8753_reg) + 1)) + if (reg < 1 || reg >= (ARRAY_SIZE(wm8753_reg) + 1)) return -1; return cache[reg - 1]; } @@ -109,7 +109,7 @@ static inline void wm8753_write_reg_cache(struct snd_soc_codec *codec, unsigned int reg, unsigned int value) { u16 *cache = codec->reg_cache; - if (reg < 1 || reg > 0x3f) + if (reg < 1 || reg >= (ARRAY_SIZE(wm8753_reg) + 1)) return; cache[reg - 1] = value; } diff --git a/sound/soc/codecs/wm8990.c b/sound/soc/codecs/wm8990.c index 6b27786..f93c095 100644 --- a/sound/soc/codecs/wm8990.c +++ b/sound/soc/codecs/wm8990.c @@ -116,7 +116,7 @@ static inline unsigned int wm8990_read_reg_cache(struct snd_soc_codec *codec, unsigned int reg) { u16 *cache = codec->reg_cache; - BUG_ON(reg > (ARRAY_SIZE(wm8990_reg)) - 1); + BUG_ON(reg >= ARRAY_SIZE(wm8990_reg)); return cache[reg]; }
@@ -129,7 +129,7 @@ static inline void wm8990_write_reg_cache(struct snd_soc_codec *codec, u16 *cache = codec->reg_cache;
/* Reset register and reserved registers are uncached */ - if (reg == 0 || reg > ARRAY_SIZE(wm8990_reg) - 1) + if (reg == 0 || reg >= ARRAY_SIZE(wm8990_reg)) return;
cache[reg] = value; diff --git a/sound/soc/codecs/wm9705.c b/sound/soc/codecs/wm9705.c index c6b7dc2..021b8ab 100644 --- a/sound/soc/codecs/wm9705.c +++ b/sound/soc/codecs/wm9705.c @@ -221,7 +221,7 @@ static unsigned int ac97_read(struct snd_soc_codec *codec, unsigned int reg) default: reg = reg >> 1;
- if (reg > (ARRAY_SIZE(wm9705_reg))) + if (reg >= (ARRAY_SIZE(wm9705_reg))) return -EIO;
return cache[reg]; @@ -235,7 +235,7 @@ static int ac97_write(struct snd_soc_codec *codec, unsigned int reg,
soc_ac97_ops.write(codec->ac97, reg, val); reg = reg >> 1; - if (reg <= (ARRAY_SIZE(wm9705_reg))) + if (reg < (ARRAY_SIZE(wm9705_reg))) cache[reg] = val;
return 0; diff --git a/sound/soc/codecs/wm9712.c b/sound/soc/codecs/wm9712.c index 1b0ace0..4dc90d6 100644 --- a/sound/soc/codecs/wm9712.c +++ b/sound/soc/codecs/wm9712.c @@ -452,7 +452,7 @@ static unsigned int ac97_read(struct snd_soc_codec *codec, else { reg = reg >> 1;
- if (reg > (ARRAY_SIZE(wm9712_reg))) + if (reg >= (ARRAY_SIZE(wm9712_reg))) return -EIO;
return cache[reg]; @@ -466,7 +466,7 @@ static int ac97_write(struct snd_soc_codec *codec, unsigned int reg,
soc_ac97_ops.write(codec->ac97, reg, val); reg = reg >> 1; - if (reg <= (ARRAY_SIZE(wm9712_reg))) + if (reg < (ARRAY_SIZE(wm9712_reg))) cache[reg] = val;
return 0; diff --git a/sound/soc/codecs/wm9713.c b/sound/soc/codecs/wm9713.c index a456226..d8ddca9 100644 --- a/sound/soc/codecs/wm9713.c +++ b/sound/soc/codecs/wm9713.c @@ -621,7 +621,7 @@ static unsigned int ac97_read(struct snd_soc_codec *codec, else { reg = reg >> 1;
- if (reg > (ARRAY_SIZE(wm9713_reg))) + if (reg >= (ARRAY_SIZE(wm9713_reg))) return -EIO;
return cache[reg]; @@ -635,7 +635,7 @@ static int ac97_write(struct snd_soc_codec *codec, unsigned int reg, if (reg < 0x7c) soc_ac97_ops.write(codec->ac97, reg, val); reg = reg >> 1; - if (reg <= (ARRAY_SIZE(wm9713_reg))) + if (reg < (ARRAY_SIZE(wm9713_reg))) cache[reg] = val;
return 0;
On Fri, Jan 16, 2009 at 01:39:45PM +0000, Ian Molton wrote:
It looks to me like most chips could be handled with little difficulty - a reg mask for some chips that have eg. 9 bit registers represented in a u16, a reg_step to allow easy handling of different reg sizes, and an array of register numbers for which the core should not cache values.
Easily 10 chips presently supported could use that scheme, whilst leaving the function pointer based system available for the few really weird chips.
Oh, there's a lot of stuff that could be factored out but the whole register access area needs a good looking at. It's just not the highest priority cleanup - that's getting the card registration API converted over to V2 so we can have multiple cards in one system.
What does seem to be clear though is that there seems to be some confusion as to what the 'reg' parameter to codec->read should be - should it be a register _address_, or a register _number_.
It should be the register address as you'd see it in the datasheet unless there's a *very* good reason for it to be something else. It's entirely private to the codec driver what the register parameter means, the rest of the code only works with values that were given to it by the codec driver.
If the former, then the caching in my wm9705 and a few other drivers is broken. These treat reg as a register _address_ and shift it right by 1 to get the reg number and thus index into the reg cache.
Neither scheme is broken. Most codecs have no gaps in the register space and therefore just use a straight array with a 1:1 mapping between registers and cache slots. The AC97 parts are special because AC97 only has even numbered registers so they're saving a bit of memory by only having cache space for registers that actually exist and cutting down on noise by only showing those registers in the debug output. It's not a big saving but it's there.
Please post an incremental patch with your other changes.
Attached below for review.
Doesn't seem to include the kmemdup() fix for WM9705 - if you could post the WM9705 changes only together with your signoff I can push the queue I'm sitting on to Takashi?
I haven't added my SoB: as I havent tested it.
As you can see, the cache handling is done in several ways by different codecs, with inconsistent semantics and return codes (some BUG, some
Yes. Broadly, writing outside of the register space the driver wants to support is a clear bug, but the driver can choose to cache some or none of the registers. This makes the specifics of the error handling less important for the system as a whole.
return 1 some -1 some -EIO. Some wont allow undefined reg access, some do, but only uncached.) This seems like ripe territory for bugs (albeit
Undefined register access is often a deliberate decision in order to allow access to functionality of the chip outside the fully documented sections of the register space. Having the cache there may not be safe depending on the behaviour of the undocumented registers.
fairly harmless ones, but bugs nontheless).
Yes, though it's less bad than it looks since each codec driver only has to agree with itself.
This is why I'm saying the whole area needs going over fairly carefully - there's useful cleanups that can be done but it needs a degree of care due to the way the code is now in order to ensure that existing functionality doesn't get broken. Fortunately much of the care should scale well over doing cleanups of the whole area since everything is private to the individual codec drivers.
This patch takes fixes a number of bugs in the caching code used by several ASoC codec drivers. Mostly off-by-one fixes.
This looks reasonable, though I didn't go through it completely carefully due to the lack of signoff.
Mark Brown wrote:
On Fri, Jan 16, 2009 at 01:39:45PM +0000, Ian Molton wrote:
Oh, there's a lot of stuff that could be factored out but the whole register access area needs a good looking at.
I dont mind consolidating the cache handling code if its worth the effort (IOW its not scheduled to get totally re-written)
It should be the register address as you'd see it in the datasheet unless there's a *very* good reason for it to be something else. It's entirely private to the codec driver what the register parameter means, the rest of the code only works with values that were given to it by the codec driver.
some of the core code for dumping the codecs reg_cache looks quite broken... at least its only debug code, but broken debug code is worse than useless...
If the former, then the caching in my wm9705 and a few other drivers is broken. These treat reg as a register _address_ and shift it right by 1 to get the reg number and thus index into the reg cache.
Neither scheme is broken.
I guess not as its codec specific (wit the exception noted above)
The AC97 parts are special because AC97 only has even numbered registers so they're saving a bit of memory by only having cache space for registers that actually exist and cutting down on noise by only showing those registers in the debug output. It's not a big saving but it's there.
Actually they arent. the regs are 16 bit anyway for the most part so there is no saving - Just confusion over the meaning of the 'reg' parameter.
Basically we've got two codec types. BOTH have 16 bit regs, but on some the 'reg' parameter is a register address, and on some its the register number.
This is fine I supppose if the semantics for the underlying bus are the same, eg. the PXA AC97 bus takes reg as an address.
I havent checked to see if any AC97 codecs are passing reg as a register number - if they are then thats a clear bug as its not what the AC97 bus driver expects.
Please post an incremental patch with your other changes.
Attached below for review.
Doesn't seem to include the kmemdup() fix for WM9705
Whoops. Fixed.
- if you could post
the WM9705 changes only together with your signoff I can push the queue I'm sitting on to Takashi?
Sure, but I really think the other changes noted below need to go in ASAP as AFAICT they are clearly bogus.
Undefined register access is often a deliberate decision in order to allow access to functionality of the chip outside the fully documented sections of the register space.
Yes, I understand that.
Having the cache there may not be safe depending on the behaviour of the undocumented registers.
Of course, but drivers calling ac97_write need to get clear errors back, IMHO.
Yes, though it's less bad than it looks since each codec driver only has to agree with itself.
True.
This patch takes fixes a number of bugs in the caching code used by several ASoC codec drivers. Mostly off-by-one fixes.
This looks reasonable, though I didn't go through it completely carefully due to the lack of signoff.
I think it needs to get in ASAP. I'll send it along with the wm9705 stuff again, with my SOB (I've read through it again), but as a seperate patchset.
Patches to follow shortly.
-Ian
On Fri, Jan 16, 2009 at 03:00:35PM +0000, Ian Molton wrote:
Mark Brown wrote:
On Fri, Jan 16, 2009 at 01:39:45PM +0000, Ian Molton wrote:
Oh, there's a lot of stuff that could be factored out but the whole register access area needs a good looking at.
I dont mind consolidating the cache handling code if its worth the effort (IOW its not scheduled to get totally re-written)
It's not likely to be rewritten dramatically, like I say it's more about the effort taken to do it - I know there's a bunch of people who really want multiple cards.
some of the core code for dumping the codecs reg_cache looks quite broken... at least its only debug code, but broken debug code is worse than useless...
Could you be more specific, please? The read interface is in rather active use. The only thing I'm aware of is that only things marked as cached are visible. The read interface certainly gets a lot of use, the write interface less so but still.
The AC97 parts are special because AC97 only has even numbered registers so they're saving a bit of memory by only having cache space for registers that actually exist and cutting down on noise by only showing those registers in the debug output. It's not a big saving but it's there.
Actually they arent. the regs are 16 bit anyway for the most part so there is no saving - Just confusion over the meaning of the 'reg' parameter.
There is. The natural layout would be to just have an array of u16s which gives us an array of 16 bit register slots like this:
[0][1][2][3][4][5]..
but we're actually storing:
[0][2][4]...
since we know that the odd numbered registers don't exist.
Basically we've got two codec types. BOTH have 16 bit regs, but on some the 'reg' parameter is a register address, and on some its the register number.
The registers referred to by ASoC are *always* register numbers as you'd see in the chip documentation; the storage method used for any register cache is entirely up to the codec drivers and is not something that the core concerns itself with. The core is also unaware of the actual width of the registers.
You're confusing yourself here by thinking of these things memory mapped devices - they aren't. How software chooses to lay out any view it has of the registers in memory has no meaning in hardware since there is, in general, no way to interact with the hardware with a resolution less than a full register.
This is fine I supppose if the semantics for the underlying bus are the same, eg. the PXA AC97 bus takes reg as an address.
For marshalling purposes all the I2C and SPI codecs are their own "bus driver" - the only external thing that cares is the hardware itself, the I2C and SPI bus drivers just deal in byte streams. AC97 is the only actual bus in use here and it has the same register number interface that ASoC uses.
I havent checked to see if any AC97 codecs are passing reg as a register number - if they are then thats a clear bug as its not what the AC97 bus driver expects.
All the AC97 codecs are using reg as a register number because that is what both ASoC and the AC97 bus drivers expect. Note that the AC97 bus drivers have no idea that the codecs have caches, they just work with register/value pairs.
the WM9705 changes only together with your signoff I can push the queue I'm sitting on to Takashi?
Sure, but I really think the other changes noted below need to go in ASAP as AFAICT they are clearly bogus.
They're not *that* urgent given how long they've been there - it's a nice cleanup not something I'd push to Linus unless they fixed issues on live systems (in which case I'd really expect to see patches to fix the callers as well). The risk/reward doesn't seem worth it.
Having the cache there may not be safe depending on the behaviour of the undocumented registers.
Of course, but drivers calling ac97_write need to get clear errors back, IMHO.
Hardware I/O is a different matter - the error codes there are more interesting since that could realistically go wrong at runtime. Accessing invalid registers is a clear coding error, but it's possible that something is relying on the fact that errors aren't reported (probably by completely ignoring the return values) so it needs a bit of care to actually change anything.
Hi,
Mark, the changes requested are implemented below. There are two patchsets, one for wm9705 and one for the other codec issues I found.
also available at:
git://git.mnementh.co.uk/linux-2.6-im.git asoc-pu
From 8e89f1f1a94bc697d1ce1c07ab8426d372b0d6fe Mon Sep 17 00:00:00 2001 From: Ian Molton ian@mnementh.co.uk Date: Fri, 16 Jan 2009 15:37:22 +0000 Subject: [PATCH] ASoC: codec: wm9705 misc cleanup
This patch fixes an off-by-one error in the wm9705 reg_cache implementation and replaces a kzalloc/copy with kmemdup().
Signed-off-by: Ian Molton ian@mnementh.co.uk --- sound/soc/codecs/wm9705.c | 11 ++++------- 1 files changed, 4 insertions(+), 7 deletions(-)
diff --git a/sound/soc/codecs/wm9705.c b/sound/soc/codecs/wm9705.c index c6b7dc2..cb26b6a 100644 --- a/sound/soc/codecs/wm9705.c +++ b/sound/soc/codecs/wm9705.c @@ -221,7 +221,7 @@ static unsigned int ac97_read(struct snd_soc_codec *codec, unsigned int reg) default: reg = reg >> 1;
- if (reg > (ARRAY_SIZE(wm9705_reg))) + if (reg >= (ARRAY_SIZE(wm9705_reg))) return -EIO;
return cache[reg]; @@ -235,7 +235,7 @@ static int ac97_write(struct snd_soc_codec *codec, unsigned int reg,
soc_ac97_ops.write(codec->ac97, reg, val); reg = reg >> 1; - if (reg <= (ARRAY_SIZE(wm9705_reg))) + if (reg < (ARRAY_SIZE(wm9705_reg))) cache[reg] = val;
return 0; @@ -327,15 +327,12 @@ static int wm9705_soc_probe(struct platform_device *pdev) codec = socdev->codec; mutex_init(&codec->mutex);
- codec->reg_cache = - kzalloc(sizeof(u16) * ARRAY_SIZE(wm9705_reg), GFP_KERNEL); + codec->reg_cache = kmemdup(wm9705_reg, sizeof(wm9705_reg), GFP_KERNEL); if (codec->reg_cache == NULL) { ret = -ENOMEM; goto cache_err; } - memcpy(codec->reg_cache, wm9705_reg, - sizeof(u16) * ARRAY_SIZE(wm9705_reg)); - codec->reg_cache_size = sizeof(u16) * ARRAY_SIZE(wm9705_reg); + codec->reg_cache_size = sizeof(wm9705_reg); codec->reg_cache_step = 2;
codec->name = "WM9705";
On Fri, Jan 16, 2009 at 03:46:19PM +0000, Ian Molton wrote:
Mark, the changes requested are implemented below. There are two patchsets, one for wm9705 and one for the other codec issues I found.
also available at:
git://git.mnementh.co.uk/linux-2.6-im.git asoc-pu
Thanks, I've applied the first and squashed it down, will look at the second one later.
It looks like your MUA has badly whitespace damaged the patches - it's word wrapped them and it looks like tabs and spaces have been confused. Might be worth using git send-email to send patches.
On Fri, Jan 16, 2009 at 03:46:19PM +0000, Ian Molton wrote:
Mark, the changes requested are implemented below. There are two patchsets, one for wm9705 and one for the other codec issues I found.
This looks good, thanks - I've applied it. A couple of the changes appear to be pure formatting changes:
--- a/sound/soc/codecs/ad1980.c +++ b/sound/soc/codecs/ad1980.c @@ -109,7 +109,7 @@ static unsigned int ac97_read(struct snd_soc_codec *codec, default: reg = reg >> 1;
if (reg >= (ARRAY_SIZE(ad1980_reg)))
if (reg >= ARRAY_SIZE(ad1980_reg)) return -EINVAL;return cache[reg];
Formatting changes only in this driver? Not sure I really like the extra brackets.
diff --git a/sound/soc/codecs/wm8990.c b/sound/soc/codecs/wm8990.c index 6b27786..f93c095 100644 --- a/sound/soc/codecs/wm8990.c +++ b/sound/soc/codecs/wm8990.c @@ -116,7 +116,7 @@ static inline unsigned int wm8990_read_reg_cache(struct snd_soc_codec *codec, unsigned int reg) { u16 *cache = codec->reg_cache;
- BUG_ON(reg > (ARRAY_SIZE(wm8990_reg)) - 1);
- BUG_ON(reg >= ARRAY_SIZE(wm8990_reg)); return cache[reg];
}
These two bits of code are equivalent...
Mark Brown wrote:
On Fri, Jan 16, 2009 at 03:46:19PM +0000, Ian Molton wrote:
Mark, the changes requested are implemented below. There are two patchsets, one for wm9705 and one for the other codec issues I found.
This looks good, thanks - I've applied it. A couple of the changes appear to be pure formatting changes:
Thanks,
Comments below on your comments:
--- a/sound/soc/codecs/ad1980.c +++ b/sound/soc/codecs/ad1980.c @@ -109,7 +109,7 @@ static unsigned int ac97_read(struct snd_soc_codec *codec, default: reg = reg >> 1;
if (reg >= (ARRAY_SIZE(ad1980_reg)))
if (reg >= ARRAY_SIZE(ad1980_reg)) return -EINVAL;return cache[reg];
Formatting changes only in this driver? Not sure I really like the extra brackets.
Exactly - nor did I, so Iremoved them :-)
diff --git a/sound/soc/codecs/wm8990.c b/sound/soc/codecs/wm8990.c index 6b27786..f93c095 100644 --- a/sound/soc/codecs/wm8990.c +++ b/sound/soc/codecs/wm8990.c @@ -116,7 +116,7 @@ static inline unsigned int wm8990_read_reg_cache(struct snd_soc_codec *codec, unsigned int reg) { u16 *cache = codec->reg_cache;
- BUG_ON(reg > (ARRAY_SIZE(wm8990_reg)) - 1);
- BUG_ON(reg >= ARRAY_SIZE(wm8990_reg)); return cache[reg];
}
These two bits of code are equivalent...
Yes but one is clearer and more commonly used. Testing for 'more than one less than the array size' is just a bit... meh :-)
(plus its inconsistent with all the other drivers as it was)
-Ian
participants (4)
-
Ian Molton -
Mark Brown -
Russell King - ARM Linux -
Takashi Iwai