[alsa-devel] [PATCH v2 0/1] *** SUBJECT HERE ***
Hello.
And thank you for all the comments.
Comments to comments:
+#undef DEBUG
This shouldn't be here.
Removed.
wl1273->mode = ucontrol->value.integer.value[0];You're not doing any validation of the supplied value here.
Validation added.
Don't put your enums in arrays, it just makes things harder to maintain since you have to reference an index into an array rather than a named value.
Yes using one-element arrays is kind of meaningless, so arrays removed.
My major comment about this driver is that it'd probably make sense to redo it on top of Liam's multi-component branch, though it shouldn't be a pressing concern for merge.
I didn't do this yet. But could you give me the git URL and then I'll resend?
Wouldn't it be nicer to do this stuff with the ALSA constraint APIs rather than futzing with the constant data?
I principle I of course agree. But it seems - also discussed with the local ALSA specialist - that using static constraints does not work here. HALF_DUPLEX is not one of the SNDRV_PCM_HW_PARAM_'s... or something like that. As a ALSA non-specialist I'd be willing to this leave as it is...
+static int wl1273_set_dai_fmt(struct snd_soc_dai *codec_dai,
unsigned int fmt)+{
return 0;+}
Remove unused functions.
I tried leaving this out but then the codec stopped working. I guess I should mention that my test environment is 2.6.32. I'm only able to test that the codec compiles under 2.6.35. Anyway I left the function in.
+enum wl1273_mode wl1273_get_codec_mode(struct snd_soc_codec *codec) +{
struct wl1273_priv *wl1273 = snd_soc_codec_get_drvdata(codec);return wl1273->mode;+} +EXPORT_SYMBOL_GPL(wl1273_get_codec_mode);
Why is this being exported?
This is because the soc_card driver needs to know the codec mode and accessing the internals of the codec struct is ugly.
+static int wl1273_soc_suspend(struct platform_device *pdev, pm_message_t state) +{
return 0;+}
+static int wl1273_soc_resume(struct platform_device *pdev) +{
return 0;+}
Remove unused functions.
Dropping these caused no problems, so removed...
Cheers, Matti A.
Matti J. Aaltonen (1): ASoC: TI WL1273 FM Radio Codec.
sound/soc/codecs/wl1273.c | 586 +++++++++++++++++++++++++++++++++++++++++++++ sound/soc/codecs/wl1273.h | 42 ++++ 2 files changed, 628 insertions(+), 0 deletions(-) create mode 100644 sound/soc/codecs/wl1273.c create mode 100644 sound/soc/codecs/wl1273.h
This is an ALSA codec for the Texas Instruments WL1273 FM Radio.
Signed-off-by: Matti J. Aaltonen matti.j.aaltonen@nokia.com --- sound/soc/codecs/wl1273.c | 586 +++++++++++++++++++++++++++++++++++++++++++++ sound/soc/codecs/wl1273.h | 42 ++++ 2 files changed, 628 insertions(+), 0 deletions(-) create mode 100644 sound/soc/codecs/wl1273.c create mode 100644 sound/soc/codecs/wl1273.h
diff --git a/sound/soc/codecs/wl1273.c b/sound/soc/codecs/wl1273.c new file mode 100644 index 0000000..ca3e654 --- /dev/null +++ b/sound/soc/codecs/wl1273.c @@ -0,0 +1,586 @@ +/* + * ALSA SoC WL1273 codec driver + * + * Author: Matti Aaltonen, matti.j.aaltonen@nokia.com + * + * Copyright: (C) 2010 Nokia Corporation + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License + * version 2 as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, but + * WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA + * 02110-1301 USA + * + */ + +#include <linux/mfd/wl1273-core.h> +#include <linux/slab.h> +#include <sound/pcm.h> +#include <sound/pcm_params.h> +#include <sound/soc-dapm.h> +#include <sound/initval.h> + +#include "wl1273.h" + +static int snd_wl1273_fm_set_i2s_mode(struct wl1273_core *core, + int rate, int width) +{ + struct device *dev = &core->i2c_dev->dev; + int r = 0; + u16 mode; + + dev_dbg(dev, "rate: %d\n", rate); + dev_dbg(dev, "width: %d\n", width); + + mutex_lock(&core->lock); + + mode = core->i2s_mode & ~WL1273_IS2_WIDTH & ~WL1273_IS2_RATE; + + switch (rate) { + case 48000: + mode |= WL1273_IS2_RATE_48K; + break; + case 44100: + mode |= WL1273_IS2_RATE_44_1K; + break; + case 32000: + mode |= WL1273_IS2_RATE_32K; + break; + case 22050: + mode |= WL1273_IS2_RATE_22_05K; + break; + case 16000: + mode |= WL1273_IS2_RATE_16K; + break; + case 12000: + mode |= WL1273_IS2_RATE_12K; + break; + case 11025: + mode |= WL1273_IS2_RATE_11_025; + break; + case 8000: + mode |= WL1273_IS2_RATE_8K; + break; + default: + dev_err(dev, "Sampling rate: %d not supported\n", rate); + r = -EINVAL; + goto out; + } + + switch (width) { + case 16: + mode |= WL1273_IS2_WIDTH_32; + break; + case 20: + mode |= WL1273_IS2_WIDTH_40; + break; + case 24: + mode |= WL1273_IS2_WIDTH_48; + break; + case 25: + mode |= WL1273_IS2_WIDTH_50; + break; + case 30: + mode |= WL1273_IS2_WIDTH_60; + break; + case 32: + mode |= WL1273_IS2_WIDTH_64; + break; + case 40: + mode |= WL1273_IS2_WIDTH_80; + break; + case 48: + mode |= WL1273_IS2_WIDTH_96; + break; + case 64: + mode |= WL1273_IS2_WIDTH_128; + break; + default: + dev_err(dev, "Data width: %d not supported\n", width); + r = -EINVAL; + goto out; + } + + dev_dbg(dev, "WL1273_I2S_DEF_MODE: 0x%04x\n", WL1273_I2S_DEF_MODE); + dev_dbg(dev, "core->i2s_mode: 0x%04x\n", core->i2s_mode); + dev_dbg(dev, "mode: 0x%04x\n", mode); + + if (core->i2s_mode != mode) { + r = wl1273_fm_write_cmd(core, WL1273_I2S_MODE_CONFIG_SET, + mode); + if (!r) + core->i2s_mode = mode; + + r = wl1273_fm_write_cmd(core, WL1273_AUDIO_ENABLE, + WL1273_AUDIO_ENABLE_I2S); + if (r) + goto out; + } +out: + mutex_unlock(&core->lock); + + return r; +} + +static int snd_wl1273_fm_set_channel_number(struct wl1273_core *core, + int channel_number) +{ + struct i2c_client *client = core->i2c_dev; + struct device *dev = &client->dev; + int r = 0; + + dev_dbg(dev, "%s\n", __func__); + + mutex_lock(&core->lock); + + if (core->channel_number == channel_number) + goto out; + + if (channel_number == 1 && core->mode == WL1273_MODE_RX) + r = wl1273_fm_write_cmd(core, WL1273_MOST_MODE_SET, + WL1273_RX_MONO); + else if (channel_number == 1 && core->mode == WL1273_MODE_TX) + r = wl1273_fm_write_cmd(core, WL1273_MONO_SET, + WL1273_TX_MONO); + else if (channel_number == 2 && core->mode == WL1273_MODE_RX) + r = wl1273_fm_write_cmd(core, WL1273_MOST_MODE_SET, + WL1273_RX_STEREO); + else if (channel_number == 2 && core->mode == WL1273_MODE_TX) + r = wl1273_fm_write_cmd(core, WL1273_MONO_SET, + WL1273_TX_STEREO); + else + r = -EINVAL; +out: + mutex_unlock(&core->lock); + + return r; +} + +static int snd_wl1273_get_audio_route(struct snd_kcontrol *kcontrol, + struct snd_ctl_elem_value *ucontrol) +{ + struct snd_soc_codec *codec = snd_kcontrol_chip(kcontrol); + struct wl1273_priv *wl1273 = snd_soc_codec_get_drvdata(codec); + + ucontrol->value.integer.value[0] = wl1273->mode; + + return 0; +} + +static const char *wl1273_audio_route[] = { "Bt", "FmRx", "FmTx" }; + +static int snd_wl1273_set_audio_route(struct snd_kcontrol *kcontrol, + struct snd_ctl_elem_value *ucontrol) +{ + struct snd_soc_codec *codec = snd_kcontrol_chip(kcontrol); + struct wl1273_priv *wl1273 = snd_soc_codec_get_drvdata(codec); + + /* Do not allow changes while stream is running */ + if (codec->active) + return -EPERM; + + if (ucontrol->value.integer.value[0] < 0 || + ucontrol->value.integer.value[0] >= ARRAY_SIZE(wl1273_audio_route)) + return -EINVAL; + + wl1273->mode = ucontrol->value.integer.value[0]; + + return 1; +} + +static const struct soc_enum wl1273_enum = + SOC_ENUM_SINGLE_EXT(ARRAY_SIZE(wl1273_audio_route), wl1273_audio_route); + +static int snd_wl1273_fm_audio_get(struct snd_kcontrol *kcontrol, + struct snd_ctl_elem_value *ucontrol) +{ + struct snd_soc_codec *codec = snd_kcontrol_chip(kcontrol); + struct wl1273_priv *wl1273 = snd_soc_codec_get_drvdata(codec); + + dev_dbg(codec->dev, "%s: enter.\n", __func__); + + ucontrol->value.integer.value[0] = wl1273->core->audio_mode; + + return 0; +} + +static int snd_wl1273_fm_audio_put(struct snd_kcontrol *kcontrol, + struct snd_ctl_elem_value *ucontrol) +{ + struct snd_soc_codec *codec = snd_kcontrol_chip(kcontrol); + struct wl1273_priv *wl1273 = snd_soc_codec_get_drvdata(codec); + int val, r = 0; + + dev_dbg(codec->dev, "%s: enter.\n", __func__); + + val = ucontrol->value.integer.value[0]; + if (wl1273->core->audio_mode == val) + return 0; + + r = wl1273_fm_set_audio(wl1273->core, val); + if (r < 0) + return r; + + return 1; +} + +static const char *wl1273_audio_strings[] = { "Digital", "Analog" }; + +static const struct soc_enum wl1273_audio_enum = + SOC_ENUM_SINGLE_EXT(ARRAY_SIZE(wl1273_audio_strings), + wl1273_audio_strings); + +static int snd_wl1273_fm_volume_get(struct snd_kcontrol *kcontrol, + struct snd_ctl_elem_value *ucontrol) +{ + struct snd_soc_codec *codec = snd_kcontrol_chip(kcontrol); + struct wl1273_priv *wl1273 = snd_soc_codec_get_drvdata(codec); + + dev_dbg(codec->dev, "%s: enter.\n", __func__); + + ucontrol->value.integer.value[0] = wl1273->core->volume; + + return 0; +} + +static int snd_wl1273_fm_volume_put(struct snd_kcontrol *kcontrol, + struct snd_ctl_elem_value *ucontrol) +{ + struct snd_soc_codec *codec = snd_kcontrol_chip(kcontrol); + struct wl1273_priv *wl1273 = snd_soc_codec_get_drvdata(codec); + int r; + + dev_dbg(codec->dev, "%s: enter.\n", __func__); + + r = wl1273_fm_set_volume(wl1273->core, + ucontrol->value.integer.value[0]); + if (r) + return r; + + return 1; +} + +static const struct snd_kcontrol_new wl1273_controls[] = { + SOC_ENUM_EXT("Codec Mode", wl1273_enum, + snd_wl1273_get_audio_route, snd_wl1273_set_audio_route), + SOC_ENUM_EXT("Audio Switch", wl1273_audio_enum, + snd_wl1273_fm_audio_get, snd_wl1273_fm_audio_put), + SOC_SINGLE_EXT("Volume", 0, 0, WL1273_MAX_VOLUME, 0, + snd_wl1273_fm_volume_get, snd_wl1273_fm_volume_put), +}; + +static int wl1273_add_controls(struct snd_soc_codec *codec) +{ + return snd_soc_add_controls(codec, wl1273_controls, + ARRAY_SIZE(wl1273_controls)); +} + +static int wl1273_startup(struct snd_pcm_substream *substream, + struct snd_soc_dai *dai) +{ + struct snd_soc_pcm_runtime *rtd = substream->private_data; + struct snd_soc_device *socdev = rtd->socdev; + struct snd_pcm *pcm = socdev->card->dai_link->pcm; + struct snd_soc_codec *codec = socdev->card->codec; + struct wl1273_priv *wl1273 = snd_soc_codec_get_drvdata(codec); + + switch (wl1273->mode) { + case WL1273_MODE_BT: + pcm->info_flags &= ~SNDRV_PCM_INFO_HALF_DUPLEX; + break; + case WL1273_MODE_FM_RX: + if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) { + pr_err("Cannot play in RX mode.\n"); + return -EINVAL; + } + pcm->info_flags |= SNDRV_PCM_INFO_HALF_DUPLEX; + break; + case WL1273_MODE_FM_TX: + if (substream->stream == SNDRV_PCM_STREAM_CAPTURE) { + pr_err("Cannot capture in TX mode.\n"); + return -EINVAL; + } + pcm->info_flags |= SNDRV_PCM_INFO_HALF_DUPLEX; + break; + default: + return -EINVAL; + break; + } + + return 0; +} + +static int wl1273_hw_params(struct snd_pcm_substream *substream, + struct snd_pcm_hw_params *params, + struct snd_soc_dai *dai) +{ + struct snd_soc_pcm_runtime *rtd = substream->private_data; + struct snd_soc_device *socdev = rtd->socdev; + struct snd_soc_codec *codec = socdev->card->codec; + struct wl1273_priv *wl1273 = snd_soc_codec_get_drvdata(codec); + struct wl1273_core *core = wl1273->core; + unsigned int rate, width, r; + + if (params_format(params) != SNDRV_PCM_FORMAT_S16_LE) { + pr_err("Only SNDRV_PCM_FORMAT_S16_LE supported.\n"); + return -EINVAL; + } + + rate = params_rate(params); + width = hw_param_interval(params, SNDRV_PCM_HW_PARAM_SAMPLE_BITS)->min; + + if (wl1273->mode == WL1273_MODE_BT) { + if (rate != 8000) { + pr_err("Rate %d not supported.\n", params_rate(params)); + return -EINVAL; + } + + if (params_channels(params) != 1) { + pr_err("Only mono supported.\n"); + return -EINVAL; + } + + return 0; + } + + if (wl1273->mode == WL1273_MODE_FM_TX && + substream->stream == SNDRV_PCM_STREAM_CAPTURE) { + pr_err("Only playback supported with TX.\n"); + return -EINVAL; + } + + if (wl1273->mode == WL1273_MODE_FM_RX && + substream->stream == SNDRV_PCM_STREAM_PLAYBACK) { + pr_err("Only capture supported with RX.\n"); + return -EINVAL; + } + + if (wl1273->mode != WL1273_MODE_FM_RX && + wl1273->mode != WL1273_MODE_FM_TX) { + pr_err("Unexpected mode: %d.\n", wl1273->mode); + return -EINVAL; + } + + r = snd_wl1273_fm_set_i2s_mode(core, rate, width); + if (r) + return r; + + r = snd_wl1273_fm_set_channel_number(core, (params_channels(params))); + if (r) + return r; + + return 0; +} + +static int wl1273_set_dai_fmt(struct snd_soc_dai *codec_dai, + unsigned int fmt) +{ + return 0; +} + +static struct snd_soc_dai_ops wl1273_dai_ops = { + .startup = wl1273_startup, + .hw_params = wl1273_hw_params, + .set_fmt = wl1273_set_dai_fmt, +}; + +struct snd_soc_dai wl1273_dai = { + .name = "WL1273 BT/FM codec", + .playback = { + .stream_name = "Playback", + .channels_min = 1, + .channels_max = 2, + .rates = SNDRV_PCM_RATE_8000 | SNDRV_PCM_RATE_48000, + .formats = SNDRV_PCM_FMTBIT_S16_LE}, + .capture = { + .stream_name = "Capture", + .channels_min = 1, + .channels_max = 2, + .rates = SNDRV_PCM_RATE_8000 | SNDRV_PCM_RATE_48000, + .formats = SNDRV_PCM_FMTBIT_S16_LE}, + .ops = &wl1273_dai_ops, +}; +EXPORT_SYMBOL_GPL(wl1273_dai); + +/* + * This function is exported because the soc_card driver needs to + * know the mode. + */ +enum wl1273_mode wl1273_get_codec_mode(struct snd_soc_codec *codec) +{ + struct wl1273_priv *wl1273 = snd_soc_codec_get_drvdata(codec); + return wl1273->mode; +} +EXPORT_SYMBOL_GPL(wl1273_get_codec_mode); + +static struct snd_soc_codec *wl1273_codec; + +/* + * initialize the driver + * register the mixer and dsp interfaces with the kernel + */ +static int wl1273_soc_probe(struct platform_device *pdev) +{ + struct snd_soc_device *socdev = platform_get_drvdata(pdev); + struct snd_soc_codec *codec; + struct wl1273_priv *wl1273; + int r = 0; + + dev_dbg(&pdev->dev, "%s.\n", __func__); + + codec = wl1273_codec; + wl1273 = snd_soc_codec_get_drvdata(codec); + socdev->card->codec = codec; + + codec->name = "wl1273"; + codec->owner = THIS_MODULE; + codec->dai = &wl1273_dai; + codec->num_dai = 1; + + /* register pcms */ + r = snd_soc_new_pcms(socdev, SNDRV_DEFAULT_IDX1, SNDRV_DEFAULT_STR1); + if (r < 0) { + dev_err(&pdev->dev, "Wl1273: failed to create pcms.\n"); + goto err2; + } + + r = wl1273_add_controls(codec); + if (r < 0) { + dev_err(&pdev->dev, "Wl1273: failed to add contols.\n"); + goto err1; + } + + return r; +err1: + snd_soc_free_pcms(socdev); +err2: + return r; +} + +static int wl1273_soc_remove(struct platform_device *pdev) +{ + struct snd_soc_device *socdev = platform_get_drvdata(pdev); + + snd_soc_free_pcms(socdev); + + return 0; +} + +static int __devinit wl1273_codec_probe(struct platform_device *pdev) +{ + struct wl1273_core **pdata = pdev->dev.platform_data; + struct snd_soc_codec *codec; + struct wl1273_priv *wl1273; + int r; + + dev_dbg(&pdev->dev, "%s.\n", __func__); + + if (!pdata) { + dev_err(&pdev->dev, "Platform data is missing.\n"); + return -EINVAL; + } + + wl1273 = kzalloc(sizeof(struct wl1273_priv), GFP_KERNEL); + if (wl1273 == NULL) { + dev_err(&pdev->dev, "Cannot allocate memory.\n"); + return -ENOMEM; + } + + wl1273->mode = WL1273_MODE_BT; + wl1273->core = *pdata; + + codec = &wl1273->codec; + snd_soc_codec_set_drvdata(codec, wl1273); + codec->dev = &pdev->dev; + wl1273_dai.dev = &pdev->dev; + + mutex_init(&codec->mutex); + INIT_LIST_HEAD(&codec->dapm_widgets); + INIT_LIST_HEAD(&codec->dapm_paths); + + codec->name = "wl1273"; + codec->owner = THIS_MODULE; + codec->dai = &wl1273_dai; + codec->num_dai = 1; + + platform_set_drvdata(pdev, wl1273); + wl1273_codec = codec; + + codec->bias_level = SND_SOC_BIAS_OFF; + + r = snd_soc_register_codec(codec); + if (r != 0) { + dev_err(codec->dev, "Failed to register codec: %d\n", r); + goto err2; + } + + r = snd_soc_register_dai(&wl1273_dai); + if (r != 0) { + dev_err(codec->dev, "Failed to register DAIs: %d\n", r); + goto err1; + } + + return 0; + +err1: + snd_soc_unregister_codec(codec); +err2: + kfree(wl1273); + return r; +} + +static int __devexit wl1273_codec_remove(struct platform_device *pdev) +{ + struct wl1273_priv *wl1273 = platform_get_drvdata(pdev); + + dev_dbg(&pdev->dev, "%s\n", __func__); + + snd_soc_unregister_dai(&wl1273_dai); + snd_soc_unregister_codec(&wl1273->codec); + + kfree(wl1273); + wl1273_codec = NULL; + + return 0; +} + +MODULE_ALIAS("platform:wl1273_codec_audio"); + +static struct platform_driver wl1273_codec_driver = { + .probe = wl1273_codec_probe, + .remove = __devexit_p(wl1273_codec_remove), + .driver = { + .name = "wl1273_codec_audio", + .owner = THIS_MODULE, + }, +}; + +static int __init wl1273_modinit(void) +{ + return platform_driver_register(&wl1273_codec_driver); +} +module_init(wl1273_modinit); + +static void __exit wl1273_exit(void) +{ + platform_driver_unregister(&wl1273_codec_driver); +} +module_exit(wl1273_exit); + +struct snd_soc_codec_device soc_codec_dev_wl1273 = { + .probe = wl1273_soc_probe, + .remove = wl1273_soc_remove, +}; +EXPORT_SYMBOL_GPL(soc_codec_dev_wl1273); + +MODULE_AUTHOR("Matti Aaltonen matti.j.aaltonen@nokia.com"); +MODULE_DESCRIPTION("ASoC WL1273 codec driver"); +MODULE_LICENSE("GPL"); diff --git a/sound/soc/codecs/wl1273.h b/sound/soc/codecs/wl1273.h new file mode 100644 index 0000000..8fb50ab --- /dev/null +++ b/sound/soc/codecs/wl1273.h @@ -0,0 +1,42 @@ +/* + * sound/soc/codec/wl1273.h + * + * ALSA SoC WL1273 codec driver + * + * Copyright (C) Nokia Corporation + * Author: Matti Aaltonen matti.j.aaltonen@nokia.com + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License + * version 2 as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, but + * WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA + * 02110-1301 USA + * + */ + +#ifndef __WL1273_CODEC_H__ +#define __WL1273_CODEC_H__ + +enum wl1273_mode { WL1273_MODE_BT, WL1273_MODE_FM_RX, WL1273_MODE_FM_TX }; + +/* codec private data */ +struct wl1273_priv { + struct snd_soc_codec codec; + enum wl1273_mode mode; + struct wl1273_core *core; +}; + +extern struct snd_soc_dai wl1273_dai; +extern struct snd_soc_codec_device soc_codec_dev_wl1273; + +enum wl1273_mode wl1273_get_codec_mode(struct snd_soc_codec *codec); + +#endif /* End of __WL1273_CODEC_H__ */
On Thu, Jul 22, 2010 at 12:38:53PM +0300, Matti J. Aaltonen wrote:
My major comment about this driver is that it'd probably make sense to redo it on top of Liam's multi-component branch, though it shouldn't be a pressing concern for merge.
I didn't do this yet. But could you give me the git URL and then I'll resend?
git://git.kernel.org/pub/scm/linux/kernel/git/lrg/sound-2.6.git
Wouldn't it be nicer to do this stuff with the ALSA constraint APIs rather than futzing with the constant data?
I principle I of course agree. But it seems - also discussed with the local ALSA specialist - that using static constraints does not work here. HALF_DUPLEX is not one of the SNDRV_PCM_HW_PARAM_'s... or something like that. As a ALSA non-specialist I'd be willing to this leave as it is...
Could you please get whoever the expert is to get involved in this thread? If this is not currently possible it should probably be.
+static int wl1273_set_dai_fmt(struct snd_soc_dai *codec_dai,
unsigned int fmt)+{
return 0;+}
Remove unused functions.
I tried leaving this out but then the codec stopped working. I guess I should mention that my test environment is 2.6.32. I'm only able to test that the codec compiles under 2.6.35. Anyway I left the function in.
Could you please be a little more specific - what do you mean when you say "the CODEC stopped working? The set_dai_fmt() function has always been optional.
+enum wl1273_mode wl1273_get_codec_mode(struct snd_soc_codec *codec) +{
struct wl1273_priv *wl1273 = snd_soc_codec_get_drvdata(codec);return wl1273->mode;+} +EXPORT_SYMBOL_GPL(wl1273_get_codec_mode);
Why is this being exported?
This is because the soc_card driver needs to know the codec mode and accessing the internals of the codec struct is ugly.
So why do you believe that the soc_card driver needs to know the CODEC mode? This was the gist of my original question...
Hi,
On Thursday 22 July 2010 12:48:32 ext Mark Brown wrote:
I principle I of course agree. But it seems - also discussed with the local ALSA specialist - that using static constraints does not work here. HALF_DUPLEX is not one of the SNDRV_PCM_HW_PARAM_'s... or something like that. As a ALSA non-specialist I'd be willing to this leave as it is...
Could you please get whoever the expert is to get involved in this thread? If this is not currently possible it should probably be.
I think the whole SNDRV_PCM_INFO_HALF_DUPLEX bit cleaning/setting can be left out as such. My point is, that in wl1273_startup, the driver anyway will refuse the non correct stream for FM mode. It is checking, that only playback can be started, when the FM is in TX mode, and allows only capture, if the chip has been configured to FM RX mode. In BT mode the codec can work in full duplex.
And to the issue: I'm not really sure about it, but at least I have not seen the constraint API used for anything, but for hw_params (access mode, formats, sizes, times)... As I looked at the ALSA core, I don't really noticed, that we can use the constraint API for this.
So let's ask Takashi: Is it possible to use the constraint API for placing HALF_DUPLEX on a stream (modifying bits in the info_flags)?
On Thu, Jul 22, 2010 at 01:57:16PM +0300, Peter Ujfalusi wrote:
I think the whole SNDRV_PCM_INFO_HALF_DUPLEX bit cleaning/setting can be left out as such.
That would be even simpler, of course!
My point is, that in wl1273_startup, the driver anyway will refuse the non correct stream for FM mode. It is checking, that only playback can be started, when the FM is in TX mode, and allows only capture, if the chip has been configured to FM RX mode. In BT mode the codec can work in full duplex.
...and hopefully userspace won't be trying to do any of the unsupported things anyway at a higher level than ALSA so that's probably OK. I guess in principle the device can support full duplex even with FM, it's just that the output is going to get discarded so it's not *useful* so there's not such a need to make sure userspace is told about the restriction anyway - the CPU driver can do the DMA which is all that's directly visible to the application.
Hi,
to give Takashi the context ;)
On Thursday 22 July 2010 13:57:16 Ujfalusi Peter (Nokia-MS/Tampere) wrote:
So let's ask Takashi: Is it possible to use the constraint API for placing HALF_DUPLEX on a stream (modifying bits in the info_flags)?
Is there a clean way to do:
+ switch (wl1273->mode) { + case WL1273_MODE_BT: + pcm->info_flags &= ~SNDRV_PCM_INFO_HALF_DUPLEX; + break; + case WL1273_MODE_FM_RX: + if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) { + pr_err("Cannot play in RX mode.\n"); + return -EINVAL; + } + pcm->info_flags |= SNDRV_PCM_INFO_HALF_DUPLEX; ^ This
+ break; + case WL1273_MODE_FM_TX: + if (substream->stream == SNDRV_PCM_STREAM_CAPTURE) { + pr_err("Cannot capture in TX mode.\n"); + return -EINVAL; + } + pcm->info_flags |= SNDRV_PCM_INFO_HALF_DUPLEX; ^ and this
+ break; + default:
with for example the ALSA constraint API?
The info_flags are modified at stream open time.
Thanks, Péter
At Thu, 22 Jul 2010 14:07:18 +0300, Peter Ujfalusi wrote:
Hi,
to give Takashi the context ;)
On Thursday 22 July 2010 13:57:16 Ujfalusi Peter (Nokia-MS/Tampere) wrote:
So let's ask Takashi: Is it possible to use the constraint API for placing HALF_DUPLEX on a stream (modifying bits in the info_flags)?
Is there a clean way to do:
switch (wl1273->mode) {case WL1273_MODE_BT:pcm->info_flags &= ~SNDRV_PCM_INFO_HALF_DUPLEX;break;case WL1273_MODE_FM_RX:if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {pr_err("Cannot play in RX mode.\n");return -EINVAL;}pcm->info_flags |= SNDRV_PCM_INFO_HALF_DUPLEX; ^ Thisbreak;case WL1273_MODE_FM_TX:if (substream->stream == SNDRV_PCM_STREAM_CAPTURE) {pr_err("Cannot capture in TX mode.\n");return -EINVAL;}pcm->info_flags |= SNDRV_PCM_INFO_HALF_DUPLEX; ^ and thisbreak;default:with for example the ALSA constraint API?
I don't think so. Because...
The info_flags are modified at stream open time.
The HALF_DUPLEX flag is referred in the PCM open itself, but before the driver's open callback is called (snd_pcm_attach_substream()). So, changing it in the open callback is too late -- at least for filtering the duplex access in the common open call. Of course, the modified info can be propagated via ioctl to user-space, but the driver doesn't block the full-duplex.
The hw constrains are basically for hw_params setup. So, the question is whether do we want to make open successful but further setup fails, or do we give the error at open time? IMO, the latter looks straightforward. That is, changing the flag before open call is needed, or we need to check explicitly in the open callback itself.
Or, let me know if I misunderstand the discussion here...
thanks,
Takashi
On Thu, Jul 22, 2010 at 01:30:23PM +0200, Takashi Iwai wrote:
The hw constrains are basically for hw_params setup. So, the question is whether do we want to make open successful but further setup fails, or do we give the error at open time? IMO, the latter looks straightforward. That is, changing the flag before open call is needed, or we need to check explicitly in the open callback itself.
Or, let me know if I misunderstand the discussion here...
I think you understand correctly but we decided we can just totally ignore this setting since from the point of view of application software it can happily do full duplex operations with this device, the device will just discard the data sent to it in some configurations.
Hi.
Thanks for the comments. ________________________________________ From: ext Mark Brown [broonie@opensource.wolfsonmicro.com] Sent: Thursday, July 22, 2010 11:48 AM To: Aaltonen Matti.J (Nokia-MS/Tampere) Cc: alsa-devel@alsa-project.org; lrg@slimlogic.co.uk Subject: Re: [PATCH v2 0/1] *** SUBJECT HERE ***
On Thu, Jul 22, 2010 at 12:38:53PM +0300, Matti J. Aaltonen wrote:
My major comment about this driver is that it'd probably make sense to redo it on top of Liam's multi-component branch, though it shouldn't be a pressing concern for merge.
I didn't do this yet. But could you give me the git URL and then I'll resend?
git://git.kernel.org/pub/scm/linux/kernel/git/lrg/sound-2.6.git
Thanks...
Wouldn't it be nicer to do this stuff with the ALSA constraint APIs rather than futzing with the constant data?
I principle I of course agree. But it seems - also discussed with the local ALSA specialist - that using static constraints does not work here. HALF_DUPLEX is not one of the SNDRV_PCM_HW_PARAM_'s... or something like that. As a ALSA non-specialist I'd be willing to this leave as it is...
Could you please get whoever the expert is to get involved in this thread? If this is not currently possible it should probably be.
Yes, he's being CCed...
+static int wl1273_set_dai_fmt(struct snd_soc_dai *codec_dai,
unsigned int fmt)+{
return 0;+}
Remove unused functions.
I tried leaving this out but then the codec stopped working. I guess I should mention that my test environment is 2.6.32. I'm only able to test that the codec compiles under 2.6.35. Anyway I left the function in.
Could you please be a little more specific - what do you mean when you say "the CODEC stopped working? The set_dai_fmt() function has always been optional.
I get this error message:
aplay -f dat -Dhw:wl1273 /root/sumppi.wav [ 297.179626] Can't set codec DAI configuration: -22 [ 297.184539] asoc: BT/FM PCM startup failed
+enum wl1273_mode wl1273_get_codec_mode(struct snd_soc_codec *codec) +{
struct wl1273_priv *wl1273 = snd_soc_codec_get_drvdata(codec);return wl1273->mode;+} +EXPORT_SYMBOL_GPL(wl1273_get_codec_mode);
Why is this being exported?
This is because the soc_card driver needs to know the codec mode and accessing the internals of the codec struct is ugly.
So why do you believe that the soc_card driver needs to know the CODEC mode? This was the gist of my original question...
OK, sorry... The problem in a sense is that the codec supports three different modes: BT, FM RX and FM TX. All digital audio goes over a single McBSP connection. The BT part handles the audio routing, which is completely external to the radio driver and this codec. And the user of the codec is in principle responsible for keeping the codec mode in sync with the route setting. And the soc_card driver needs to know the setting also to do its thing.
Thanks, Matti
On Thursday 22 July 2010 14:18:32 Aaltonen Matti.J (Nokia-MS/Tampere) wrote:
Could you please be a little more specific - what do you mean when you say "the CODEC stopped working? The set_dai_fmt() function has always been optional.
I get this error message:
aplay -f dat -Dhw:wl1273 /root/sumppi.wav [ 297.179626] Can't set codec DAI configuration: -22 [ 297.184539] asoc: BT/FM PCM startup failed
If you remove the wl1273_set_dai_fmt, than you should not call snd_soc_dai_set_fmt(codec_dai, ...); in the machine driver.
The snd_soc_dai_set_fmt checks, if the codec provides the callback, and if it is missing, than it returns -EINVAL.
Hi.
________________________________________
From: Ujfalusi Peter (Nokia-MS/Tampere) Sent: Thursday, July 22, 2010 1:25 PM To: Aaltonen Matti.J (Nokia-MS/Tampere) Cc: ext Mark Brown; alsa-devel@alsa-project.org; lrg@slimlogic.co.uk Subject: Re: [PATCH v2 0/1] *** SUBJECT HERE ***
On Thursday 22 July 2010 14:18:32 Aaltonen Matti.J (Nokia-MS/Tampere) wrote:
Could you please be a little more specific - what do you mean when you say "the CODEC stopped working? The set_dai_fmt() function has always been optional.
I get this error message:
aplay -f dat -Dhw:wl1273 /root/sumppi.wav [ 297.179626] Can't set codec DAI configuration: -22 [ 297.184539] asoc: BT/FM PCM startup failed
If you remove the wl1273_set_dai_fmt, than you should not call snd_soc_dai_set_fmt(codec_dai, ...); in the machine driver.
The snd_soc_dai_set_fmt checks, if the codec provides the callback, and if it is missing, than it returns -EINVAL.
Yes, I fixed the soc_card driver and now it works.
Thank you...
Matti A.
-- Péter
On Thu, Jul 22, 2010 at 01:18:32PM +0200, matti.j.aaltonen@nokia.com wrote:
From: ext Mark Brown [broonie@opensource.wolfsonmicro.com]
+static int wl1273_set_dai_fmt(struct snd_soc_dai *codec_dai,
unsigned int fmt)+{
return 0;+}
Remove unused functions.
I tried leaving this out but then the codec stopped working. I guess I should mention that my test environment is 2.6.32. I'm only able to test that the codec compiles under 2.6.35. Anyway I left the function in.
Could you please be a little more specific - what do you mean when you say "the CODEC stopped working? The set_dai_fmt() function has always been optional.
I get this error message:
aplay -f dat -Dhw:wl1273 /root/sumppi.wav [ 297.179626] Can't set codec DAI configuration: -22 [ 297.184539] asoc: BT/FM PCM startup failed
So, obviously if your machine driver tries to configure the DAI format on a device that doesn't support configuring the DAI format that's not going to work. You should just remove the attempt to do something unsupported from the machine driver rather than adding a null function. If there is a function there it needs to at least do validation of the parameters.
So why do you believe that the soc_card driver needs to know the CODEC mode? This was the gist of my original question...
[Reflowed into 80 colummns, please fix your MUA configuration.]
OK, sorry... The problem in a sense is that the codec supports three different modes: BT, FM RX and FM TX. All digital audio goes over a single McBSP connection. The BT part handles the audio routing, which is completely external to the radio driver and this codec. And the user of the codec is in principle responsible for keeping the codec mode in sync with the route setting. And the soc_card driver needs to know the setting also to do its thing.
Which is...
You've still not provided any information about what use this will be put to.
________________________________________ From: alsa-devel-bounces@alsa-project.org [alsa-devel-bounces@alsa-project.org] On Behalf Of ext Mark Brown [broonie@opensource.wolfsonmicro.com] Sent: Thursday, July 22, 2010 2:02 PM To: Aaltonen Matti.J (Nokia-MS/Tampere) Cc: alsa-devel@alsa-project.org; Ujfalusi Peter (Nokia-MS/Tampere); lrg@slimlogic.co.uk Subject: Re: [alsa-devel] [PATCH v2 0/1] *** SUBJECT HERE ***
On Thu, Jul 22, 2010 at 01:18:32PM +0200, matti.j.aaltonen@nokia.com wrote:
From: ext Mark Brown [broonie@opensource.wolfsonmicro.com]
OK, sorry... The problem in a sense is that the codec supports three different modes: BT, FM RX and FM TX. All digital audio goes over a single McBSP connection. The BT part handles the audio routing, which is completely external to the radio driver and this codec. And the user of the codec is in principle responsible for keeping the codec mode in sync with the route setting. And the soc_card driver needs to know the setting also to do its thing.
Which is...
You've still not provided any information about what use this will be put to.
Yes, sorry I didn't get your point. I guess I tought that it's irrelevant, what the soc_card driver actually does. But it actually does this:
switch (wl1273_get_codec_mode(codec)) { case WL1273_MODE_FM_RX: case WL1273_MODE_FM_TX: fmt = SND_SOC_DAIFMT_I2S | SND_SOC_DAIFMT_NB_NF | SND_SOC_DAIFMT_CBM_CFM;
break; case WL1273_MODE_BT: fmt = SND_SOC_DAIFMT_DSP_A | SND_SOC_DAIFMT_IB_NF | SND_SOC_DAIFMT_CBM_CFM;
break; default: return -EINVAL; }
r = snd_soc_dai_set_fmt(cpu_dai, fmt);
Cheers, Matti A.
_______________________________________________ Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
On Thu, Jul 22, 2010 at 02:11:27PM +0200, matti.j.aaltonen@nokia.com wrote:
You've still not provided any information about what use this will be put to.
Yes, sorry I didn't get your point. I guess I tought that it's irrelevant, what the soc_card driver actually does. But it actually does this:
OK, so the DAI format for the CODEC changes dependant on the mode. For this I'd suggest just changing the function to actually export the DAI format directly rather than requring all users to replicate the lookup you've currently got in your driver. This makes it clear what the purpose of providing the export is and avoids external drivers needing to know too much about the internals of this one.
In terms of what's relevant if someone's asking questions like this they're normally trying to figure out what your high level goal is because your code looks suspicous and are trying to understand the purpose of the code so they can suggest a way of doing things that is more in line with the intentions of the surrounding code.
Hello.
On Thu, 2010-07-22 at 15:05 +0200, ext Mark Brown wrote:
On Thu, Jul 22, 2010 at 02:11:27PM +0200, matti.j.aaltonen@nokia.com wrote:
You've still not provided any information about what use this will be put to.
Yes, sorry I didn't get your point. I guess I tought that it's irrelevant, what the soc_card driver actually does. But it actually does this:
OK, so the DAI format for the CODEC changes dependant on the mode. For this I'd suggest just changing the function to actually export the DAI format directly rather than requring all users to replicate the lookup you've currently got in your driver. This makes it clear what the purpose of providing the export is and avoids external drivers needing to know too much about the internals of this one.
Ok, that makes perfect sense...
In terms of what's relevant if someone's asking questions like this they're normally trying to figure out what your high level goal is because your code looks suspicous and are trying to understand the purpose of the code so they can suggest a way of doing things that is more in line with the intentions of the surrounding code.
Sorry, I didn't mean to be difficult... If this was my personal project I'd have sent you every bit my code at once.
Cheers, Matti A.
participants (5)
-
Mark Brown -
Matti J. Aaltonen -
matti.j.aaltonen＠nokia.com
-
Peter Ujfalusi -
Takashi Iwai