diff mbox

[v4,2/3] ASoC: qcom: add apq8016 sound card support

Message ID 1432310047-5316-1-git-send-email-srinivas.kandagatla@linaro.org (mailing list archive)
State New, archived
Headers show

Commit Message

Srinivas Kandagatla May 22, 2015, 3:54 p.m. UTC
This patch adds apq8016 machine driver support. This patch was tested on
two apq8016-sbc and msm8916-mtp board for both hdmi and analog audio
features.

Tested-by: Kenneth Westfield <kwestfie@codeaurora.org>
Acked-by: Kenneth Westfield <kwestfie@codeaurora.org>
Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
 sound/soc/qcom/Kconfig       |   9 ++
 sound/soc/qcom/Makefile      |   2 +
 sound/soc/qcom/apq8016_sbc.c | 215 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 226 insertions(+)
 create mode 100644 sound/soc/qcom/apq8016_sbc.c

Comments

Mark Brown June 2, 2015, 7:55 p.m. UTC | #1
On Fri, May 22, 2015 at 04:54:07PM +0100, Srinivas Kandagatla wrote:

> +	if (cpu_dai->id == MI2S_QUATERNARY) {
> +		/* Configure the Quat MI2S to TLMM */
> +		writel(readl(pdata->mic_iomux) |
> +			MIC_CTRL_QUA_WS_SLAVE_SEL_10 |
> +			MIC_CTRL_TLMM_SCLK_EN,
> +			pdata->mic_iomux);
> +
> +		return 0;
> +	} else if (cpu_dai->id == MI2S_PRIMARY) {

This looks like you're trying to write a switch statement.  It's also
somewhat unclear to me that this should be in a machine driver and not
in a CODEC/aux driver that gets pulled in by a machine driver, I can't
be entirely sure what this is controlling.

> +		if (of_property_read_bool(np, "external"))
> +			name = "HDMI";
> +
> +		else
> +			name = "Headset";

Coding style.  I'm also a bit concerned about the binding here -
headsets sound external too?

> +	card->dev = dev;
> +	data = apq8016_sbc_parse_of(card);

We parse the DT here and then...

> +	ret = snd_soc_of_parse_card_name(card, "qcom,model");
> +	if (ret) {
> +		dev_err(&pdev->dev, "Error parsing card name: %d\n", ret);
> +		return ret;
> +	}

...this other bit of DT here.

> +	ret = devm_snd_soc_register_card(&pdev->dev, card);
> +	if (ret == -EPROBE_DEFER) {
> +		card->dev = NULL;
> +		return ret;
> +	} else if (ret) {
> +		dev_err(&pdev->dev, "Error registering soundcard: %d\n", ret);
> +		return ret;
> +	}

If setting card->dev does anything there something is broken, and in
general it's just better form to not special case probe deferral.
Srinivas Kandagatla June 3, 2015, 8:11 a.m. UTC | #2
On 02/06/15 20:55, Mark Brown wrote:
> On Fri, May 22, 2015 at 04:54:07PM +0100, Srinivas Kandagatla wrote:
>
>> +	if (cpu_dai->id == MI2S_QUATERNARY) {
>> +		/* Configure the Quat MI2S to TLMM */
>> +		writel(readl(pdata->mic_iomux) |
>> +			MIC_CTRL_QUA_WS_SLAVE_SEL_10 |
>> +			MIC_CTRL_TLMM_SCLK_EN,
>> +			pdata->mic_iomux);
>> +
>> +		return 0;
>> +	} else if (cpu_dai->id == MI2S_PRIMARY) {
>
> This looks like you're trying to write a switch statement.  It's also
I started of with switch case but, as this card only uses 
MI2S_QUATERNARY and MI2S_PRIMARY I converted it to if else statements to 
save few lines.
> somewhat unclear to me that this should be in a machine driver and not
> in a CODEC/aux driver that gets pulled in by a machine driver, I can't
> be entirely sure what this is controlling.

This bit of code is writing to a mux control register, which would be 
very much specific to board wiring. Moving this to machine level could 
would introduce lot of un-necessary interface.
>
>> +		if (of_property_read_bool(np, "external"))
>> +			name = "HDMI";
>> +
>> +		else
>> +			name = "Headset";
>
> Coding style.  I'm also a bit concerned about the binding here -
> headsets sound external too?
I agree, its confusing. The term external in this case describes the 
placement of codec w.r.t to SOC rather then the audio sink. on APQ8016 
we have an internal codec within the SOC which is wiredup to the Headset 
and on this board we have external ADV7533 codec which is connected to HDMI.

>
>> +	card->dev = dev;
>> +	data = apq8016_sbc_parse_of(card);
>
> We parse the DT here and then...
>
>> +	ret = snd_soc_of_parse_card_name(card, "qcom,model");
>> +	if (ret) {
>> +		dev_err(&pdev->dev, "Error parsing card name: %d\n", ret);
>> +		return ret;
>> +	}
>
> ...this other bit of DT here.
I will put this code to the apq8016_sbc_parse_of function.
>
>> +	ret = devm_snd_soc_register_card(&pdev->dev, card);
>> +	if (ret == -EPROBE_DEFER) {
>> +		card->dev = NULL;
>> +		return ret;
>> +	} else if (ret) {
>> +		dev_err(&pdev->dev, "Error registering soundcard: %d\n", ret);
>> +		return ret;
>> +	}
>
> If setting card->dev does anything there something is broken, and in
> general it's just better form to not special case probe deferral.
>
Yes, you are right, setting card->dev = NULL is really unnecessary, I 
will fix it and also remove the special casing the EPROBE_DEFER.
There are 2 other drivers which need similar cleanup, I will fix them too.

--srini
diff mbox

Patch

diff --git a/sound/soc/qcom/Kconfig b/sound/soc/qcom/Kconfig
index 29fff6d..6ecac6c 100644
--- a/sound/soc/qcom/Kconfig
+++ b/sound/soc/qcom/Kconfig
@@ -32,3 +32,12 @@  config SND_SOC_STORM
 	help
           Say Y or M if you want add support for SoC audio on the
           Qualcomm Technologies IPQ806X-based Storm board.
+
+config SND_SOC_APQ8016_SBC
+	tristate "SoC Audio support for APQ8016 SBC platforms"
+	depends on SND_SOC_QCOM && (ARCH_QCOM || COMPILE_TEST)
+	select SND_SOC_LPASS_APQ8016
+	help
+          Support for Qualcomm Technologies LPASS audio block in
+          APQ8016 SOC-based systems.
+          Say Y if you want to use audio devices on MI2S
diff --git a/sound/soc/qcom/Makefile b/sound/soc/qcom/Makefile
index ac76308..79e5c50 100644
--- a/sound/soc/qcom/Makefile
+++ b/sound/soc/qcom/Makefile
@@ -11,5 +11,7 @@  obj-$(CONFIG_SND_SOC_LPASS_APQ8016) += snd-soc-lpass-apq8016.o
 
 # Machine
 snd-soc-storm-objs := storm.o
+snd-soc-apq8016-sbc-objs := apq8016_sbc.o
 
 obj-$(CONFIG_SND_SOC_STORM) += snd-soc-storm.o
+obj-$(CONFIG_SND_SOC_APQ8016_SBC) += snd-soc-apq8016-sbc.o
diff --git a/sound/soc/qcom/apq8016_sbc.c b/sound/soc/qcom/apq8016_sbc.c
new file mode 100644
index 0000000..8958633
--- /dev/null
+++ b/sound/soc/qcom/apq8016_sbc.c
@@ -0,0 +1,215 @@ 
+/*
+ * Copyright (c) 2015 The Linux Foundation. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only 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.
+ *
+ */
+
+#include <linux/device.h>
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/io.h>
+#include <linux/of.h>
+#include <linux/clk.h>
+#include <linux/platform_device.h>
+#include <sound/pcm.h>
+#include <sound/pcm_params.h>
+#include <sound/soc.h>
+#include <dt-bindings/sound/apq8016-lpass.h>
+
+struct apq8016_sbc_data {
+	void __iomem *mic_iomux;
+	void __iomem *spkr_iomux;
+	struct snd_soc_dai_link dai_link[];	/* dynamically allocated */
+};
+
+#define MIC_CTRL_QUA_WS_SLAVE_SEL_10	BIT(17)
+#define MIC_CTRL_TLMM_SCLK_EN		BIT(1)
+#define	SPKR_CTL_PRI_WS_SLAVE_SEL_11	(BIT(17) | BIT(16))
+
+static int apq8016_sbc_startup(struct snd_pcm_substream *substream)
+{
+	struct snd_soc_pcm_runtime *rtd = substream->private_data;
+	struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
+	struct snd_soc_card *card = rtd->card;
+	struct apq8016_sbc_data *pdata = snd_soc_card_get_drvdata(card);
+
+	if (cpu_dai->id == MI2S_QUATERNARY) {
+		/* Configure the Quat MI2S to TLMM */
+		writel(readl(pdata->mic_iomux) |
+			MIC_CTRL_QUA_WS_SLAVE_SEL_10 |
+			MIC_CTRL_TLMM_SCLK_EN,
+			pdata->mic_iomux);
+
+		return 0;
+	} else if (cpu_dai->id == MI2S_PRIMARY) {
+		writel(readl(pdata->spkr_iomux) |
+			SPKR_CTL_PRI_WS_SLAVE_SEL_11,
+			pdata->spkr_iomux);
+
+		return 0;
+	}
+
+	dev_err(card->dev, "unsupported cpu dai configuration\n");
+
+	return -EINVAL;
+}
+
+static struct snd_soc_ops apq8016_sbc_soc_ops = {
+	.startup	= apq8016_sbc_startup,
+};
+
+static struct apq8016_sbc_data *apq8016_sbc_parse_of(struct snd_soc_card *card)
+{
+	int num_links;
+	struct device *dev = card->dev;
+	struct snd_soc_dai_link *dai_link;
+	struct device_node *np, *codec, *cpu, *node  = dev->of_node;
+	struct apq8016_sbc_data *data;
+	char *name;
+	int ret;
+
+	/* Populate links */
+	num_links = of_get_child_count(node);
+
+	/* Allocate the private data and the DAI link array */
+	data = devm_kzalloc(dev, sizeof(*data) + sizeof(*dai_link) * num_links,
+			    GFP_KERNEL);
+	if (!data)
+		return ERR_PTR(-ENOMEM);
+
+	card->dai_link	= &data->dai_link[0];
+	card->num_links	= num_links;
+
+	dai_link = data->dai_link;
+
+	for_each_child_of_node(node, np) {
+		cpu = of_get_child_by_name(np, "cpu");
+		codec = of_get_child_by_name(np, "codec");
+
+		if (!cpu || !codec) {
+			dev_err(dev, "Can't find cpu/codec DT node\n");
+			return ERR_PTR(-EINVAL);
+		}
+
+		dai_link->cpu_of_node = of_parse_phandle(cpu, "sound-dai", 0);
+		if (!dai_link->cpu_of_node) {
+			dev_err(card->dev, "error getting cpu phandle\n");
+			return ERR_PTR(-EINVAL);
+		}
+
+		dai_link->codec_of_node = of_parse_phandle(codec,
+							   "sound-dai",
+							   0);
+		if (!dai_link->codec_of_node) {
+			dev_err(card->dev, "error getting codec phandle\n");
+			return ERR_PTR(-EINVAL);
+		}
+
+		ret = snd_soc_of_get_dai_name(cpu, &dai_link->cpu_dai_name);
+		if (ret) {
+			dev_err(card->dev, "error getting cpu dai name\n");
+			return ERR_PTR(ret);
+		}
+
+		ret = snd_soc_of_get_dai_name(codec, &dai_link->codec_dai_name);
+		if (ret) {
+			dev_err(card->dev, "error getting codec dai name\n");
+			return ERR_PTR(ret);
+		}
+
+		dai_link->platform_of_node = dai_link->cpu_of_node;
+		/* For now we only support playback */
+		dai_link->playback_only = true;
+
+		if (of_property_read_bool(np, "external"))
+			name = "HDMI";
+
+		else
+			name = "Headset";
+
+		dai_link->ops = &apq8016_sbc_soc_ops;
+		dai_link->name = dai_link->stream_name = name;
+
+		dai_link++;
+	}
+
+	return data;
+}
+
+static int apq8016_sbc_platform_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct snd_soc_card *card;
+	struct apq8016_sbc_data *data;
+	struct resource *res;
+	int ret;
+
+	card = devm_kzalloc(dev, sizeof(*card), GFP_KERNEL);
+	if (!card)
+		return -ENOMEM;
+
+	card->dev = dev;
+	data = apq8016_sbc_parse_of(card);
+	if (IS_ERR(data)) {
+		dev_err(&pdev->dev, "Error resolving dai links: %ld\n",
+			PTR_ERR(data));
+		return PTR_ERR(data);
+	}
+
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "mic-iomux");
+	data->mic_iomux = devm_ioremap_resource(dev, res);
+	if (IS_ERR(data->mic_iomux))
+		return PTR_ERR(data->mic_iomux);
+
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "spkr-iomux");
+	data->spkr_iomux = devm_ioremap_resource(dev, res);
+	if (IS_ERR(data->spkr_iomux))
+		return PTR_ERR(data->spkr_iomux);
+
+	platform_set_drvdata(pdev, data);
+	snd_soc_card_set_drvdata(card, data);
+
+	ret = snd_soc_of_parse_card_name(card, "qcom,model");
+	if (ret) {
+		dev_err(&pdev->dev, "Error parsing card name: %d\n", ret);
+		return ret;
+	}
+
+	ret = devm_snd_soc_register_card(&pdev->dev, card);
+	if (ret == -EPROBE_DEFER) {
+		card->dev = NULL;
+		return ret;
+	} else if (ret) {
+		dev_err(&pdev->dev, "Error registering soundcard: %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static const struct of_device_id apq8016_sbc_device_id[]  = {
+	{ .compatible = "qcom,apq8016-sbc-sndcard" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, apq8016_sbc_device_id);
+
+static struct platform_driver apq8016_sbc_platform_driver = {
+	.driver = {
+		.name = "qcom-apq8016-sbc",
+		.of_match_table = of_match_ptr(apq8016_sbc_device_id),
+	},
+	.probe = apq8016_sbc_platform_probe,
+};
+module_platform_driver(apq8016_sbc_platform_driver);
+
+MODULE_AUTHOR("Srinivas Kandagatla <srinivas.kandagatla@linaro.org");
+MODULE_DESCRIPTION("APQ8016 ASoC Machine Driver");
+MODULE_LICENSE("GPL v2");