[alsa-devel] [PATCH V2] ASoC: fsl: add imx-wm8962 machine driver
This is the initial imx-wm8962 device-tree-only machine driver working with fsl_ssi driver. More features can be added on top of it later.
Signed-off-by: Nicolin Chen b42378@freescale.com --- ChangeLog: v1->v2: * fixed a typo in probe(): * data->dai.ops = &imx_hifi_ops, ==> data->dai.ops = &imx_hifi_ops; --- .../devicetree/bindings/sound/imx-audio-wm8962.txt | 61 +++ sound/soc/fsl/Kconfig | 12 + sound/soc/fsl/Makefile | 2 + sound/soc/fsl/imx-wm8962.c | 387 ++++++++++++++++++++ 4 files changed, 462 insertions(+), 0 deletions(-) create mode 100644 Documentation/devicetree/bindings/sound/imx-audio-wm8962.txt create mode 100644 sound/soc/fsl/imx-wm8962.c
diff --git a/Documentation/devicetree/bindings/sound/imx-audio-wm8962.txt b/Documentation/devicetree/bindings/sound/imx-audio-wm8962.txt new file mode 100644 index 0000000..cb188a6 --- /dev/null +++ b/Documentation/devicetree/bindings/sound/imx-audio-wm8962.txt @@ -0,0 +1,61 @@ +Freescale i.MX audio complex with WM8962 codec + +Required properties: +- compatible : "fsl,imx-audio-wm8962" +- model : The user-visible name of this sound complex +- ssi-controller : The phandle of the i.MX SSI controller +- audio-codec : The phandle of the WM8962 audio codec +- audio-routing : A list of the connections between audio components. + Each entry is a pair of strings, the first being the connection's sink, + the second being the connection's source. Valid names could be power + supplies, WM8962 pins, and the jacks on the board: + + Power supplies: + * Mic Bias + + WM8962 pins: + * HPOUTL + * HPOUTR + * SPKOUTL + * SPKOUTR + * MICBIAS + * IN3R + * DMIC + * DMICDAT + + Board connectors: + * Mic Jack + * Headphone Jack + * Ext Spk + +- mux-int-port : The internal port of the i.MX audio muxer (AUDMUX) +- mux-ext-port : The external port of the i.MX audio muxer + +- hp-det-gpios : The gpio port of Headphone detection. +- mic-det-gpios: The gpio port of Micphone detection. + +Note: The AUDMUX port numbering should start at 1, which is consistent with +hardware manual. + +Example: + +sound { + compatible = "fsl,imx6q-sabresd-wm8962", + "fsl,imx-audio-wm8962"; + model = "wm8962-audio"; + ssi-controller = <&ssi2>; + audio-codec = <&codec>; + audio-routing = + "Headphone Jack", "HPOUTL", + "Headphone Jack", "HPOUTR", + "Ext Spk", "SPKOUTL", + "Ext Spk", "SPKOUTR", + "MICBIAS", "AMIC", + "IN3R", "MICBIAS", + "DMIC", "MICBIAS", + "DMICDAT", "DMIC"; + mux-int-port = <2>; + mux-ext-port = <3>; + hp-det-gpios = <&gpio7 8 1>; /* active low */ + mic-det-gpios = <&gpio1 9 1>; /* active low */ +}; diff --git a/sound/soc/fsl/Kconfig b/sound/soc/fsl/Kconfig index 7860cc2..aa43854 100644 --- a/sound/soc/fsl/Kconfig +++ b/sound/soc/fsl/Kconfig @@ -168,6 +168,18 @@ config SND_SOC_EUKREA_TLV320 Enable I2S based access to the TLV320AIC23B codec attached to the SSI interface
+config SND_SOC_IMX_WM8962 + tristate "SoC Audio support for i.MX boards with wm8962" + depends on OF && I2C + select SND_SOC_WM8962 + select SND_SOC_IMX_PCM_DMA + select SND_SOC_IMX_AUDMUX + select SND_SOC_FSL_SSI + select SND_SOC_FSL_UTILS + help + Say Y if you want to add support for SoC audio on an i.MX board with + a wm8962 codec. + config SND_SOC_IMX_SGTL5000 tristate "SoC Audio support for i.MX boards with sgtl5000" depends on OF && I2C diff --git a/sound/soc/fsl/Makefile b/sound/soc/fsl/Makefile index 91883f8..d4b4aa8 100644 --- a/sound/soc/fsl/Makefile +++ b/sound/soc/fsl/Makefile @@ -42,6 +42,7 @@ snd-soc-phycore-ac97-objs := phycore-ac97.o snd-soc-mx27vis-aic32x4-objs := mx27vis-aic32x4.o snd-soc-wm1133-ev1-objs := wm1133-ev1.o snd-soc-imx-sgtl5000-objs := imx-sgtl5000.o +snd-soc-imx-wm8962-objs := imx-wm8962.o snd-soc-imx-mc13783-objs := imx-mc13783.o
obj-$(CONFIG_SND_SOC_EUKREA_TLV320) += snd-soc-eukrea-tlv320.o @@ -49,4 +50,5 @@ obj-$(CONFIG_SND_SOC_PHYCORE_AC97) += snd-soc-phycore-ac97.o obj-$(CONFIG_SND_SOC_MX27VIS_AIC32X4) += snd-soc-mx27vis-aic32x4.o obj-$(CONFIG_SND_MXC_SOC_WM1133_EV1) += snd-soc-wm1133-ev1.o obj-$(CONFIG_SND_SOC_IMX_SGTL5000) += snd-soc-imx-sgtl5000.o +obj-$(CONFIG_SND_SOC_IMX_WM8962) += snd-soc-imx-wm8962.o obj-$(CONFIG_SND_SOC_IMX_MC13783) += snd-soc-imx-mc13783.o diff --git a/sound/soc/fsl/imx-wm8962.c b/sound/soc/fsl/imx-wm8962.c new file mode 100644 index 0000000..cb54da4 --- /dev/null +++ b/sound/soc/fsl/imx-wm8962.c @@ -0,0 +1,387 @@ +/* + * Copyright 2013 Freescale Semiconductor, Inc. + * + * Based on imx-sgtl5000.c + * Copyright 2012 Freescale Semiconductor, Inc. + * Copyright 2012 Linaro Ltd. + * + * The code contained herein is licensed under the GNU General Public + * License. You may obtain a copy of the GNU General Public License + * Version 2 or later at the following locations: + * + * http://www.opensource.org/licenses/gpl-license.html + * http://www.gnu.org/copyleft/gpl.html + */ + +#include <linux/module.h> +#include <linux/of.h> +#include <linux/of_platform.h> +#include <linux/of_i2c.h> +#include <linux/of_gpio.h> +#include <linux/slab.h> +#include <linux/clk.h> +#include <linux/irq.h> +#include <sound/soc.h> +#include <sound/pcm_params.h> +#include <sound/soc-dapm.h> +#include <linux/pinctrl/consumer.h> + +#include "../codecs/wm8962.h" +#include "imx-audmux.h" + +#define DAI_NAME_SIZE 32 + +struct imx_wm8962_data { + struct snd_soc_dai_link dai; + struct snd_soc_card card; + char codec_dai_name[DAI_NAME_SIZE]; + char platform_name[DAI_NAME_SIZE]; + struct clk *codec_clk; + unsigned int clk_frequency; +}; + +struct imx_priv { + int hp_gpio; + int hp_active_low; + int hp_status; + int amic_gpio; + int amic_active_low; + int amic_status; + struct platform_device *pdev; + struct snd_pcm_substream *first_stream; + struct snd_pcm_substream *second_stream; +}; +static struct imx_priv card_priv; + +static const struct snd_soc_dapm_widget imx_wm8962_dapm_widgets[] = { + SND_SOC_DAPM_HP("Headphone Jack", NULL), + SND_SOC_DAPM_SPK("Ext Spk", NULL), + SND_SOC_DAPM_MIC("AMIC", NULL), + SND_SOC_DAPM_MIC("DMIC", NULL), +}; + +static int check_hw_params(struct snd_pcm_substream *substream, + snd_pcm_format_t sample_format, unsigned int sample_rate) +{ + struct imx_priv *priv = &card_priv; + struct device *dev = substream->pcm->card->dev; + + substream->runtime->sample_bits = + snd_pcm_format_physical_width(sample_format); + substream->runtime->rate = sample_rate; + substream->runtime->format = sample_format; + + if (!priv->first_stream) { + priv->first_stream = substream; + } else { + priv->second_stream = substream; + + if (priv->first_stream->runtime->rate != + priv->second_stream->runtime->rate) { + dev_err(dev, "KEEP THE SAME SAMPLE RATE: %d!\n", + priv->first_stream->runtime->rate); + return -EINVAL; + } + if (priv->first_stream->runtime->sample_bits != + priv->second_stream->runtime->sample_bits) { + snd_pcm_format_t first_format = + priv->first_stream->runtime->format; + + dev_err(dev, "KEEP THE SAME FORMAT: %s!\n", + snd_pcm_format_name(first_format)); + return -EINVAL; + } + } + + return 0; +} + +static int imx_hifi_hw_params(struct snd_pcm_substream *substream, + struct snd_pcm_hw_params *params) +{ + struct snd_soc_pcm_runtime *rtd = substream->private_data; + struct snd_soc_dai *codec_dai = rtd->codec_dai; + struct device *dev = substream->pcm->card->dev; + struct imx_priv *priv = &card_priv; + struct imx_wm8962_data *data = platform_get_drvdata(priv->pdev); + unsigned int sample_rate = params_rate(params); + unsigned int sample_format = params_format(params); + int ret = 0; + unsigned int pll_out; + + /* + * WM8962 doesn't support two substreams in different sample rates or + * sample formats. So check the two parameters of two substreams' when + * there're two substreams of playback and of capture in the same time. + */ + ret = check_hw_params(substream, sample_format, sample_rate); + if (ret < 0) { + dev_err(dev, "Failed to match hw params: %d\n", ret); + return ret; + } + + if (sample_format == SNDRV_PCM_FORMAT_S24_LE) + pll_out = sample_rate * 384; + else + pll_out = sample_rate * 256; + + ret = snd_soc_dai_set_pll(codec_dai, WM8962_FLL, WM8962_FLL_MCLK, + data->clk_frequency, pll_out); + if (ret < 0) { + dev_err(dev, "Failed to start FLL: %d\n", ret); + return ret; + } + + ret = snd_soc_dai_set_sysclk(codec_dai, WM8962_SYSCLK_FLL, + pll_out, SND_SOC_CLOCK_IN); + if (ret < 0) { + dev_err(dev, "Failed to set SYSCLK: %d\n", ret); + return ret; + } + + return 0; +} + +static int imx_hifi_hw_free(struct snd_pcm_substream *substream) +{ + struct snd_soc_pcm_runtime *rtd = substream->private_data; + struct snd_soc_dai *codec_dai = rtd->codec_dai; + struct device *dev = substream->pcm->card->dev; + struct imx_priv *priv = &card_priv; + struct imx_wm8962_data *data = platform_get_drvdata(priv->pdev); + int ret; + + /* We don't need to handle anything if there's no substream running */ + if (!priv->first_stream) + return 0; + + if (priv->first_stream == substream) + priv->first_stream = priv->second_stream; + priv->second_stream = NULL; + + if (!priv->first_stream) { + /* + * SYSCLK of wm8962's not disabled yet. If we wanna close FLL, + * we need to keep SYSCLK alive by changing its source to MCLK. + */ + ret = snd_soc_dai_set_sysclk(codec_dai, WM8962_SYSCLK_MCLK, + data->clk_frequency, SND_SOC_CLOCK_IN); + if (ret < 0) { + dev_err(dev, "Failed to set SYSCLK: %d\n", ret); + return ret; + } + + /* Disable FLL and let codec do pm_runtime_put() */ + ret = snd_soc_dai_set_pll(codec_dai, WM8962_FLL, 0, 0, 0); + if (ret < 0) { + dev_err(dev, "Failed to disable FLL: %d\n", ret); + return ret; + } + } + + return 0; +} + +static struct snd_soc_ops imx_hifi_ops = { + .hw_params = imx_hifi_hw_params, + .hw_free = imx_hifi_hw_free, +}; + +static int imx_wm8962_late_probe(struct snd_soc_card *card) +{ + struct snd_soc_dai *codec_dai = card->rtd[0].codec_dai; + struct imx_priv *priv = &card_priv; + struct imx_wm8962_data *data = platform_get_drvdata(priv->pdev); + int ret; + + ret = snd_soc_dai_set_sysclk(codec_dai, WM8962_SYSCLK_MCLK, + data->clk_frequency, SND_SOC_CLOCK_IN); + if (ret < 0) + dev_err(card->dev, "Failed to set sysclk in %s\n", __func__); + + return ret; +} + +static int imx_wm8962_probe(struct platform_device *pdev) +{ + struct device_node *np = pdev->dev.of_node; + struct device_node *ssi_np, *codec_np; + struct platform_device *ssi_pdev; + struct imx_priv *priv = &card_priv; + struct i2c_client *codec_dev; + struct imx_wm8962_data *data; + int int_port, ext_port; + int ret; + + priv->pdev = pdev; + + ret = of_property_read_u32(np, "mux-int-port", &int_port); + if (ret) { + dev_err(&pdev->dev, "mux-int-port missing or invalid\n"); + return ret; + } + ret = of_property_read_u32(np, "mux-ext-port", &ext_port); + if (ret) { + dev_err(&pdev->dev, "mux-ext-port missing or invalid\n"); + return ret; + } + + /* + * The port numbering in the hardware manual starts at 1, while + * the audmux API expects it starts at 0. + */ + int_port--; + ext_port--; + ret = imx_audmux_v2_configure_port(int_port, + IMX_AUDMUX_V2_PTCR_SYN | + IMX_AUDMUX_V2_PTCR_TFSEL(ext_port) | + IMX_AUDMUX_V2_PTCR_TCSEL(ext_port) | + IMX_AUDMUX_V2_PTCR_TFSDIR | + IMX_AUDMUX_V2_PTCR_TCLKDIR, + IMX_AUDMUX_V2_PDCR_RXDSEL(ext_port)); + if (ret) { + dev_err(&pdev->dev, "audmux internal port setup failed\n"); + return ret; + } + imx_audmux_v2_configure_port(ext_port, + IMX_AUDMUX_V2_PTCR_SYN, + IMX_AUDMUX_V2_PDCR_RXDSEL(int_port)); + if (ret) { + dev_err(&pdev->dev, "audmux external port setup failed\n"); + return ret; + } + + ssi_np = of_parse_phandle(pdev->dev.of_node, "ssi-controller", 0); + codec_np = of_parse_phandle(pdev->dev.of_node, "audio-codec", 0); + if (!ssi_np || !codec_np) { + dev_err(&pdev->dev, "phandle missing or invalid\n"); + ret = -EINVAL; + goto fail; + } + + ssi_pdev = of_find_device_by_node(ssi_np); + if (!ssi_pdev) { + dev_err(&pdev->dev, "failed to find SSI platform device\n"); + ret = -EINVAL; + goto fail; + } + codec_dev = of_find_i2c_device_by_node(codec_np); + if (!codec_dev || !codec_dev->driver) { + dev_err(&pdev->dev, "failed to find codec platform device\n"); + return -EINVAL; + } + + priv->hp_gpio = of_get_named_gpio_flags(np, "hp-det-gpios", 0, + (enum of_gpio_flags *)&priv->hp_active_low); + priv->amic_gpio = of_get_named_gpio_flags(np, "mic-det-gpios", 0, + (enum of_gpio_flags *)&priv->amic_active_low); + + priv->first_stream = NULL; + priv->second_stream = NULL; + + data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL); + if (!data) { + ret = -ENOMEM; + goto fail; + } + + data->codec_clk = clk_get(&codec_dev->dev, NULL); + if (IS_ERR(data->codec_clk)) { + /* assuming clock enabled by default */ + data->codec_clk = NULL; + ret = of_property_read_u32(codec_np, "clock-frequency", + &data->clk_frequency); + if (ret) { + dev_err(&codec_dev->dev, + "clock-frequency missing or invalid\n"); + goto fail; + } + } else { + data->clk_frequency = clk_get_rate(data->codec_clk); + clk_prepare_enable(data->codec_clk); + } + + data->dai.name = "HiFi"; + data->dai.stream_name = "HiFi"; + data->dai.codec_dai_name = "wm8962"; + data->dai.codec_of_node = codec_np; + data->dai.cpu_dai_name = dev_name(&ssi_pdev->dev); + data->dai.platform_of_node = ssi_np; + data->dai.ops = &imx_hifi_ops; + data->dai.dai_fmt = SND_SOC_DAIFMT_I2S | SND_SOC_DAIFMT_NB_NF | + SND_SOC_DAIFMT_CBM_CFM; + + data->card.dev = &pdev->dev; + ret = snd_soc_of_parse_card_name(&data->card, "model"); + if (ret) + goto clk_fail; + ret = snd_soc_of_parse_audio_routing(&data->card, "audio-routing"); + if (ret) + goto clk_fail; + data->card.num_links = 1; + data->card.dai_link = &data->dai; + data->card.dapm_widgets = imx_wm8962_dapm_widgets; + data->card.num_dapm_widgets = ARRAY_SIZE(imx_wm8962_dapm_widgets); + + data->card.late_probe = imx_wm8962_late_probe, + + ret = snd_soc_register_card(&data->card); + if (ret) { + dev_err(&pdev->dev, "snd_soc_register_card failed (%d)\n", ret); + goto clk_fail; + } + + platform_set_drvdata(pdev, data); + of_node_put(ssi_np); + of_node_put(codec_np); + + return 0; + +clk_fail: + if (data->codec_clk) { + clk_disable_unprepare(data->codec_clk); + clk_put(data->codec_clk); + } +fail: + if (ssi_np) + of_node_put(ssi_np); + if (codec_np) + of_node_put(codec_np); + + return ret; +} + +static int imx_wm8962_remove(struct platform_device *pdev) +{ + struct imx_wm8962_data *data = platform_get_drvdata(pdev); + + if (data->codec_clk) { + clk_disable_unprepare(data->codec_clk); + clk_put(data->codec_clk); + } + snd_soc_unregister_card(&data->card); + + return 0; +} + +static const struct of_device_id imx_wm8962_dt_ids[] = { + { .compatible = "fsl,imx-audio-wm8962", }, + { /* sentinel */ } +}; +MODULE_DEVICE_TABLE(of, imx_wm8962_dt_ids); + +static struct platform_driver imx_wm8962_driver = { + .driver = { + .name = "imx-wm8962", + .owner = THIS_MODULE, + .of_match_table = imx_wm8962_dt_ids, + }, + .probe = imx_wm8962_probe, + .remove = imx_wm8962_remove, +}; +module_platform_driver(imx_wm8962_driver); + +MODULE_AUTHOR("Freescale Semiconductor, Inc."); +MODULE_DESCRIPTION("Freescale i.MX WM8962 ASoC machine driver"); +MODULE_LICENSE("GPL v2"); +MODULE_ALIAS("platform:imx-wm8962");
On Wed, Jun 05, 2013 at 04:21:41PM +0800, Nicolin Chen wrote:
Looks pretty good, a few comments but they're all fairly minor.
- WM8962 pins:
- HPOUTL
- HPOUTR
- SPKOUTL
- SPKOUTR
- MICBIAS
- IN3R
- DMIC
- DMICDAT
No need to list all these, just reference the CODEC (and add them to the CODEC binding documentation if they're not there which they probably aren't). This is generally good practice if there's a family of devices sharing a driver and means that you don't have to worry about missing all the other input pins as you've done here. :)
+- hp-det-gpios : The gpio port of Headphone detection. +- mic-det-gpios: The gpio port of Micphone detection.
I'd say a bit more about these - obviously the CODEC has its own accessory detection here, and I rather suspect that in fact the HP detection you have there is really jack detection.
+static int check_hw_params(struct snd_pcm_substream *substream,
snd_pcm_format_t sample_format, unsigned int sample_rate)
+{
- struct imx_priv *priv = &card_priv;
- struct device *dev = substream->pcm->card->dev;
- substream->runtime->sample_bits =
snd_pcm_format_physical_width(sample_format);
- substream->runtime->rate = sample_rate;
- substream->runtime->format = sample_format;
- if (!priv->first_stream) {
priv->first_stream = substream;
- } else {
priv->second_stream = substream;
if (priv->first_stream->runtime->rate !=
priv->second_stream->runtime->rate) {
dev_err(dev, "KEEP THE SAME SAMPLE RATE: %d!\n",
priv->first_stream->runtime->rate);
return -EINVAL;
}
if (priv->first_stream->runtime->sample_bits !=
priv->second_stream->runtime->sample_bits) {
snd_pcm_format_t first_format =
priv->first_stream->runtime->format;
dev_err(dev, "KEEP THE SAME FORMAT: %s!\n",
snd_pcm_format_name(first_format));
return -EINVAL;
}
- }
The sample rate checking can be done with the symmmetric_rates feature in the core. The sample_bits check can't be but it's something that a lot of systems probably ought to have so it ought to be factored out into the core too rather than open coded in the driver.
/*
* SYSCLK of wm8962's not disabled yet. If we wanna close FLL,
No '.
ret = snd_soc_dai_set_sysclk(codec_dai, WM8962_SYSCLK_MCLK,
data->clk_frequency, SND_SOC_CLOCK_IN);
if (ret < 0) {
dev_err(dev, "Failed to set SYSCLK: %d\n", ret);
return ret;
}
/* Disable FLL and let codec do pm_runtime_put() */
ret = snd_soc_dai_set_pll(codec_dai, WM8962_FLL, 0, 0, 0);
if (ret < 0) {
dev_err(dev, "Failed to disable FLL: %d\n", ret);
return ret;
You're enabling and disabling the CODEC only while there's an audio stream active. This means that it's not possible to support analogue bypass paths (which the device can do) - you should probably also support enabling the FLL via set_bias_level() and use set_bias_level() to turn it off (which works in all cases).
- data->codec_clk = clk_get(&codec_dev->dev, NULL);
- if (IS_ERR(data->codec_clk)) {
devm_clk_get().
/* assuming clock enabled by default */
data->codec_clk = NULL;
ret = of_property_read_u32(codec_np, "clock-frequency",
&data->clk_frequency);
if (ret) {
dev_err(&codec_dev->dev,
"clock-frequency missing or invalid\n");
goto fail;
}
Since it's easy to define a fixed rate clock (there's a generic driver for that) I'd just require the user to provide a clock API clock and fix the rate using that. This is going to be less error prone and makes the code simpler.
- } else {
data->clk_frequency = clk_get_rate(data->codec_clk);
clk_prepare_enable(data->codec_clk);
Not needed immediately but if there is actually a clock we have control of we might want to vary the frequency...
Thanks you for the review. I'll revise the patch and send a [V3]
But please see my reply at some points.
On Wed, Jun 05, 2013 at 12:55:44PM +0100, Mark Brown wrote:
+- hp-det-gpios : The gpio port of Headphone detection. +- mic-det-gpios: The gpio port of Micphone detection.
I'd say a bit more about these - obviously the CODEC has its own accessory detection here, and I rather suspect that in fact the HP detection you have there is really jack detection.
[Nic]: i.MX6 SabreSD board uses these two extra pins for jack detection. You're exactly right that CODEC already has this function. But the hardware guy of Freescale, who put the extra pins here, has his own reason that Android needs not codec driver but machine driver to handle the detection and use an event to report pin status to Android system. Well this event code hasn't been included in this patch though. So since hardware has pins here, I think it's better to put in the binding doc as well. But I guess it'd be better to put it into OPTIONAL area?
+static int check_hw_params(struct snd_pcm_substream *substream,
snd_pcm_format_t sample_format, unsigned int sample_rate)
+{
- struct imx_priv *priv = &card_priv;
- struct device *dev = substream->pcm->card->dev;
- substream->runtime->sample_bits =
snd_pcm_format_physical_width(sample_format);
- substream->runtime->rate = sample_rate;
- substream->runtime->format = sample_format;
- if (!priv->first_stream) {
priv->first_stream = substream;
- } else {
priv->second_stream = substream;
if (priv->first_stream->runtime->rate !=
priv->second_stream->runtime->rate) {
dev_err(dev, "KEEP THE SAME SAMPLE RATE: %d!\n",
priv->first_stream->runtime->rate);
return -EINVAL;
}
if (priv->first_stream->runtime->sample_bits !=
priv->second_stream->runtime->sample_bits) {
snd_pcm_format_t first_format =
priv->first_stream->runtime->format;
dev_err(dev, "KEEP THE SAME FORMAT: %s!\n",
snd_pcm_format_name(first_format));
return -EINVAL;
}
- }
The sample rate checking can be done with the symmmetric_rates feature in the core. The sample_bits check can't be but it's something that a lot of systems probably ought to have so it ought to be factored out into the core too rather than open coded in the driver.
[Nic:] fsl_ssi.c uses symmmetric_rates, but last time I traced soc-pcm.c and found the core only cares about symmmetric_rates during pcm_open(), which means the startup() in dai/machine driver, while what I'm considering is something like "arecord -Dhw:0 -r 44100 | aplay -Dhw:0 -r 48000": The two startup() would run almost at the same time and each of them would be earlier than hw_param() -- the exact point we can get the sample rate from application, but before this point no sample-rate actually be set to make hw_constraint(). Then the two hw_param() would continue their code and successively call set_fll() twice with different sample rates. But I agree that it's kinda ugly to put code here. So I think maybe I need to drop this part of code and to think about another patch for the core?
ret = snd_soc_dai_set_sysclk(codec_dai, WM8962_SYSCLK_MCLK,
data->clk_frequency, SND_SOC_CLOCK_IN);
if (ret < 0) {
dev_err(dev, "Failed to set SYSCLK: %d\n", ret);
return ret;
}
/* Disable FLL and let codec do pm_runtime_put() */
ret = snd_soc_dai_set_pll(codec_dai, WM8962_FLL, 0, 0, 0);
if (ret < 0) {
dev_err(dev, "Failed to disable FLL: %d\n", ret);
return ret;
You're enabling and disabling the CODEC only while there's an audio stream active. This means that it's not possible to support analogue bypass paths (which the device can do) - you should probably also support enabling the FLL via set_bias_level() and use set_bias_level() to turn it off (which works in all cases).
[Nic] I've seen and tested set_bias_level() way as samsung/tobermory.c does. But I find a flaw to the method that if I play two and more wav files like 'aplay -Dhw:0 audio44Khz.wav audio48Khz.wav audio22Khz.wav', which needs to set_fll() before each playback and to keep SYSCLK valid by changing its source to MCLK after the playback, but I only found that hw_param() and hw_free() are symmetrically called before/after each playback in this case.
On Thu, Jun 06, 2013 at 12:39:20PM +0800, Nicolin Chen wrote:
On Wed, Jun 05, 2013 at 12:55:44PM +0100, Mark Brown wrote:
+- hp-det-gpios : The gpio port of Headphone detection. +- mic-det-gpios: The gpio port of Micphone detection.
I'd say a bit more about these - obviously the CODEC has its own accessory detection here, and I rather suspect that in fact the HP detection you have there is really jack detection.
hasn't been included in this patch though. So since hardware has pins here, I think it's better to put in the binding doc as well. But I guess it'd be better to put it into OPTIONAL area?
Yes, they need to be optional and you need to explain what the function of these pins is so people can tell what they are. As I said I really do expect that the pin you have labelled headphone detect is really a jack detect pin.
The sample rate checking can be done with the symmmetric_rates feature in the core. The sample_bits check can't be but it's something that a lot of systems probably ought to have so it ought to be factored out into the core too rather than open coded in the driver.
[Nic:] fsl_ssi.c uses symmmetric_rates, but last time I traced soc-pcm.c and found the core only cares about symmmetric_rates during pcm_open(), which means the startup() in dai/machine driver, while what I'm considering is something like "arecord -Dhw:0 -r 44100 | aplay -Dhw:0 -r 48000": The two startup() would run almost at the same time and each of them would be earlier than hw_param() -- the exact point we can get the sample rate from application, but before this point no sample-rate actually be set to make hw_constraint(). Then the two hw_param() would continue their code and successively call set_fll() twice with different sample rates. But I agree that it's kinda ugly to put code here. So I think maybe I need to drop this part of code and to think about another patch for the core?
Yes, you certainly need to drop this patch. There is no way of fixing this with the current ALSA APIs, they are fundamentally racy and you will always have the possibility of failures during simultaneous startup for hardware with these limits.
You're enabling and disabling the CODEC only while there's an audio stream active. This means that it's not possible to support analogue bypass paths (which the device can do) - you should probably also support enabling the FLL via set_bias_level() and use set_bias_level() to turn it off (which works in all cases).
[Nic] I've seen and tested set_bias_level() way as samsung/tobermory.c does. But I find a flaw to the method that if I play two and more wav files like 'aplay -Dhw:0 audio44Khz.wav audio48Khz.wav audio22Khz.wav', which needs to set_fll() before each playback and to keep SYSCLK valid by changing its source to MCLK after the playback, but I only found that hw_param() and hw_free() are symmetrically called before/after each playback in this case.
Two things here. One is that of course there's no reference counting on FLL enables, they just happen, and the other is that as you'd expect the bias level callbacks just won't happen for the second stream as the device is powered up. You only need to set the clocking up for the first stream as the clocking can't be changed when the device is active.
On Wed, Jun 05, 2013 at 12:55:44PM +0100, Mark Brown wrote:
/* assuming clock enabled by default */
data->codec_clk = NULL;
ret = of_property_read_u32(codec_np, "clock-frequency",
&data->clk_frequency);
if (ret) {
dev_err(&codec_dev->dev,
"clock-frequency missing or invalid\n");
goto fail;
}
Since it's easy to define a fixed rate clock (there's a generic driver for that) I'd just require the user to provide a clock API clock and fix the rate using that. This is going to be less error prone and makes the code simpler.
I tried to use fixed rate clock as below:
data->codec_clk = devm_clk_get(&codec_dev->dev, NULL); if (IS_ERR(data->codec_clk)) { of_fixed_clk_setup(codec_np); data->codec_clk = clk_get(NULL, codec_np->name); if (IS_ERR(data->codec_clk)) { dev_err(&pdev->dev, "failed to create fixed clk\n"); ret = IS_ERR(data->codec_clk); goto fail; } }
data->clk_frequency = clk_get_rate(data->codec_clk); clk_prepare_enable(data->codec_clk);
But I always got "failed to create fixed clk" error during system booting. So I think it's pretty different to get a fixed clock with normal since it's on the root_list of clock tree. How can I get the clock here, or more specifically, any way to get the rate of the fixed-rate-clk?
Thanks.
On Thu, Jun 06, 2013 at 08:49:53PM +0800, Nicolin Chen wrote:
On Wed, Jun 05, 2013 at 12:55:44PM +0100, Mark Brown wrote:
Since it's easy to define a fixed rate clock (there's a generic driver for that) I'd just require the user to provide a clock API clock and fix the rate using that. This is going to be less error prone and makes the code simpler.
I tried to use fixed rate clock as below:
data->codec_clk = devm_clk_get(&codec_dev->dev, NULL); if (IS_ERR(data->codec_clk)) { of_fixed_clk_setup(codec_np); data->codec_clk = clk_get(NULL, codec_np->name);
No, this is silly. Have the board define the clock in the DT.
Hi Nicolin,
On Thu, Jun 6, 2013 at 9:49 AM, Nicolin Chen b42378@freescale.com wrote:
But I always got "failed to create fixed clk" error during system booting. So I think it's pretty different to get a fixed clock with normal since it's on the root_list of clock tree. How can I get the clock here, or more specifically, any way to get the rate of the fixed-rate-clk?
You could try something like (pass the real clock in the device tree):
http://permalink.gmane.org/gmane.linux.alsa.devel/107614
(I will re-post this patch later)
Regards,
Fabio Estevam
On Thu, Jun 06, 2013 at 11:02:43AM -0300, Fabio Estevam wrote:
You could try something like (pass the real clock in the device tree):
(I will re-post this patch later)
That's exactly what I'm suggesting.
On Thu, Jun 06, 2013 at 11:02:43AM -0300, Fabio Estevam wrote:
You could try something like (pass the real clock in the device tree):
http://permalink.gmane.org/gmane.linux.alsa.devel/107614
(I will re-post this patch later)
Regards,
Fabio Estevam
Hi Fabio
Thank you so much. I'll refer to your patch.
participants (3)
-
Fabio Estevam
-
Mark Brown
-
Nicolin Chen