linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v5 0/2] ASoC: qcom: add support to apq8016 sbc machine driver
@ 2015-06-09 12:58 Srinivas Kandagatla
  2015-06-09 12:59 ` [PATCH v5 1/2] ASoC: qcom: document apq8016 sbc machine driver bindings Srinivas Kandagatla
  2015-06-09 12:59 ` [PATCH v5 2/2] ASoC: qcom: add apq8016 sound card support Srinivas Kandagatla
  0 siblings, 2 replies; 11+ messages in thread
From: Srinivas Kandagatla @ 2015-06-09 12:58 UTC (permalink / raw)
  To: Mark Brown, alsa-devel
  Cc: Rob Herring, Patrick Lai, Banajit Goswami, Liam Girdwood,
	Jaroslav Kysela, Takashi Iwai, devicetree, linux-kernel,
	kwestfie, linux-arm-msm, Srinivas Kandagatla

This patchset adds sound card support to APQ8016 SBC board aka DB410c.
APQ8016 has 4 MI2S( Primary, Secondary, Tertiary, Quaternary) which can be routed
to internal wcd codec or external codecs. This routing and various board specifics
are controlled by 2 mux registers.

All these patches are tested for HDMI audio via adv7533 bridge and Analog audio
on APQ8016-SBC, msm8916-mtp boards.

Changes since v4 (https://lkml.org/lkml/2015/5/22/628)
  - moved all the dt parsing to single function, spotted by Mark.
  - removed unnecessary setting of card->dev = NULL spotted by Mark.
  - renamed the dai links to the correct codec names.
  - removed the special case for EPROBE_DEFER in probe, spotted by Mark.
  - Dropped all the Changes since comments as most of the patches
   in last series are merged to topic/qcom branch.

--srini

Srinivas Kandagatla (2):
  ASoC: qcom: document apq8016 sbc machine driver bindings
  ASoC: qcom: add apq8016 sound card support

 .../devicetree/bindings/sound/qcom,apq8016-sbc.txt |  61 ++++++
 sound/soc/qcom/Kconfig                             |   9 +
 sound/soc/qcom/Makefile                            |   2 +
 sound/soc/qcom/apq8016_sbc.c                       | 208 +++++++++++++++++++++
 4 files changed, 280 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/sound/qcom,apq8016-sbc.txt
 create mode 100644 sound/soc/qcom/apq8016_sbc.c

-- 
1.9.1


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v5 1/2] ASoC: qcom: document apq8016 sbc machine driver bindings
  2015-06-09 12:58 [PATCH v5 0/2] ASoC: qcom: add support to apq8016 sbc machine driver Srinivas Kandagatla
@ 2015-06-09 12:59 ` Srinivas Kandagatla
  2015-06-09 16:57   ` Mark Brown
  2015-06-09 12:59 ` [PATCH v5 2/2] ASoC: qcom: add apq8016 sound card support Srinivas Kandagatla
  1 sibling, 1 reply; 11+ messages in thread
From: Srinivas Kandagatla @ 2015-06-09 12:59 UTC (permalink / raw)
  To: Mark Brown, alsa-devel
  Cc: Rob Herring, Patrick Lai, Banajit Goswami, Liam Girdwood,
	Jaroslav Kysela, Takashi Iwai, devicetree, linux-kernel,
	kwestfie, linux-arm-msm, Srinivas Kandagatla

This patch adds bindings for apq8016 sbc machine driver.
APQ8016 has 4 MI2S which can be configured to different sinks like
internal codec/external codec, this connection and various parameters
are controlled via 2 iomux registers.

Acked-by: Kenneth Westfield <kwestfie@codeaurora.org>
Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
 .../devicetree/bindings/sound/qcom,apq8016-sbc.txt | 61 ++++++++++++++++++++++
 1 file changed, 61 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/sound/qcom,apq8016-sbc.txt

diff --git a/Documentation/devicetree/bindings/sound/qcom,apq8016-sbc.txt b/Documentation/devicetree/bindings/sound/qcom,apq8016-sbc.txt
new file mode 100644
index 0000000..01d62be
--- /dev/null
+++ b/Documentation/devicetree/bindings/sound/qcom,apq8016-sbc.txt
@@ -0,0 +1,61 @@
+* Qualcomm Technologies APQ8016 SBC ASoC machine driver
+
+This node models the Qualcomm Technologies APQ8016 SBC ASoC machine driver
+
+Required properties:
+
+- compatible		: "qcom,apq8016-sbc-sndcard"
+
+- pinctrl-N		: One property must exist for each entry in
+			  pinctrl-names.  See ../pinctrl/pinctrl-bindings.txt
+			  for details of the property values.
+- pinctrl-names		: Must contain a "default" entry.
+- reg			: Must contain an address for each entry in reg-names.
+- reg-names		: A list which must include the following entries:
+				* "mic-iomux"
+				* "spkr-iomux"
+- qcom,model		: Name of the sound card.
+
+Dai-link subnode properties and subnodes:
+
+Required dai-link subnodes:
+
+- cpu					: CPU   sub-node
+- codec					: CODEC sub-node
+
+Required CPU/CODEC subnodes properties:
+
+-sound-dai		: phandle and port of CPU/CODEC
+-capture-dai		: phandle and port of CPU/CODEC
+
+Optional CPU/CODEC subnodes properties:
+- external	: flag to indicate if the I2S is connected to external codec
+Example:
+
+sound: sound {
+	compatible = "qcom,apq8016-sbc-sndcard";
+	reg = <0x07702000 0x4>, <0x07702004 0x4>;
+	reg-names = "mic-iomux", "spkr-iomux";
+	qcom,model = "DB410c";
+
+	/* I2S - Internal codec */
+	internal-dai-link@0 {
+		cpu { /* PRIMARY */
+			sound-dai = <&lpass MI2S_PRIMARY>;
+		};
+		codec {
+			sound-dai = <&wcd_codec 0>;
+		};
+	};
+
+	/* External Primary or External Secondary -ADV7533 HDMI */
+	external-dai-link@0 {
+		external;
+		cpu { /* QUAT */
+			sound-dai = <&lpass MI2S_QUATERNARY>;
+		};
+		codec {
+			sound-dai = <&adv_bridge 0>;
+		};
+	};
+};
-- 
1.9.1


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH v5 2/2] ASoC: qcom: add apq8016 sound card support
  2015-06-09 12:58 [PATCH v5 0/2] ASoC: qcom: add support to apq8016 sbc machine driver Srinivas Kandagatla
  2015-06-09 12:59 ` [PATCH v5 1/2] ASoC: qcom: document apq8016 sbc machine driver bindings Srinivas Kandagatla
@ 2015-06-09 12:59 ` Srinivas Kandagatla
  2015-06-09 17:07   ` Mark Brown
  1 sibling, 1 reply; 11+ messages in thread
From: Srinivas Kandagatla @ 2015-06-09 12:59 UTC (permalink / raw)
  To: Mark Brown, alsa-devel
  Cc: Rob Herring, Patrick Lai, Banajit Goswami, Liam Girdwood,
	Jaroslav Kysela, Takashi Iwai, devicetree, linux-kernel,
	kwestfie, linux-arm-msm, Srinivas Kandagatla

This patch adds apq8016 machine driver support. This patch is tested on
DB410c and msm8916-mtp board for both hdmi and analog audio
features.

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 | 208 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 219 insertions(+)
 create mode 100644 sound/soc/qcom/apq8016_sbc.c

diff --git a/sound/soc/qcom/Kconfig b/sound/soc/qcom/Kconfig
index 938144c..807fedf 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..f7a5e76
--- /dev/null
+++ b/sound/soc/qcom/apq8016_sbc.c
@@ -0,0 +1,208 @@
+/*
+ * 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;
+
+	ret = snd_soc_of_parse_card_name(card, "qcom,model");
+	if (ret) {
+		dev_err(dev, "Error parsing card name: %d\n", ret);
+		return ERR_PTR(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;
+
+		/**
+		 * External codec is ADV7533
+		 * and internal codec digital part is inside apq8016
+		 * and analog part is in PMIC PM8916
+		 **/
+		if (of_property_read_bool(np, "external"))
+			name = "ADV7533";
+		else
+			name = "WCD";
+
+		dai_link->name = dai_link->stream_name = name;
+		dai_link->ops = &apq8016_sbc_soc_ops;
+		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;
+
+	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);
+
+	return devm_snd_soc_register_card(&pdev->dev, card);
+}
+
+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");
-- 
1.9.1


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH v5 1/2] ASoC: qcom: document apq8016 sbc machine driver bindings
  2015-06-09 12:59 ` [PATCH v5 1/2] ASoC: qcom: document apq8016 sbc machine driver bindings Srinivas Kandagatla
@ 2015-06-09 16:57   ` Mark Brown
  2015-06-09 17:08     ` Srinivas Kandagatla
  0 siblings, 1 reply; 11+ messages in thread
From: Mark Brown @ 2015-06-09 16:57 UTC (permalink / raw)
  To: Srinivas Kandagatla
  Cc: alsa-devel, Rob Herring, Patrick Lai, Banajit Goswami,
	Liam Girdwood, Jaroslav Kysela, Takashi Iwai, devicetree,
	linux-kernel, kwestfie, linux-arm-msm

[-- Attachment #1: Type: text/plain, Size: 419 bytes --]

On Tue, Jun 09, 2015 at 01:59:29PM +0100, Srinivas Kandagatla wrote:

> +Optional CPU/CODEC subnodes properties:
> +- external	: flag to indicate if the I2S is connected to external codec
> +Example:

Missing blank line between the property and the "Example:".  I'm still
not sure I understand why we need a boolean property indicating if an
external CODEC is in use - what is the consequence of setting this
property?

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v5 2/2] ASoC: qcom: add apq8016 sound card support
  2015-06-09 12:59 ` [PATCH v5 2/2] ASoC: qcom: add apq8016 sound card support Srinivas Kandagatla
@ 2015-06-09 17:07   ` Mark Brown
  2015-06-09 17:51     ` Srinivas Kandagatla
  0 siblings, 1 reply; 11+ messages in thread
From: Mark Brown @ 2015-06-09 17:07 UTC (permalink / raw)
  To: Srinivas Kandagatla
  Cc: alsa-devel, Rob Herring, Patrick Lai, Banajit Goswami,
	Liam Girdwood, Jaroslav Kysela, Takashi Iwai, devicetree,
	linux-kernel, kwestfie, linux-arm-msm

[-- Attachment #1: Type: text/plain, Size: 1392 bytes --]

On Tue, Jun 09, 2015 at 01:59:36PM +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) {
> +		writel(readl(pdata->spkr_iomux) |
> +			SPKR_CTL_PRI_WS_SLAVE_SEL_11,
> +			pdata->spkr_iomux);
> +
> +		return 0;
> +	}

Why not just do these one time at probe, we don't undo them when we shut
the DAI down?

> +
> +	dev_err(card->dev, "unsupported cpu dai configuration\n");
> +
> +	return -EINVAL;

It'd be clearer if this were part of the above code (which should still
be written as a switch statement) - I was just asking myself what
happens if we fall off the end of the if/else chain.

> +		/**
> +		 * External codec is ADV7533
> +		 * and internal codec digital part is inside apq8016
> +		 * and analog part is in PMIC PM8916
> +		 **/
> +		if (of_property_read_bool(np, "external"))
> +			name = "ADV7533";
> +		else
> +			name = "WCD";

If this is all the property is doing why not just put a link name
property in the DT rather than this?  That makes the driver a bit more
general and is more idiomatic.

> +		dai_link->name = dai_link->stream_name = name;

Write two assignment statements, it's clearer and more idiomatic.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v5 1/2] ASoC: qcom: document apq8016 sbc machine driver bindings
  2015-06-09 16:57   ` Mark Brown
@ 2015-06-09 17:08     ` Srinivas Kandagatla
  2015-06-09 17:13       ` Mark Brown
  0 siblings, 1 reply; 11+ messages in thread
From: Srinivas Kandagatla @ 2015-06-09 17:08 UTC (permalink / raw)
  To: Mark Brown
  Cc: alsa-devel, Rob Herring, Patrick Lai, Banajit Goswami,
	Liam Girdwood, Jaroslav Kysela, Takashi Iwai, devicetree,
	linux-kernel, kwestfie, linux-arm-msm



On 09/06/15 17:57, Mark Brown wrote:
> On Tue, Jun 09, 2015 at 01:59:29PM +0100, Srinivas Kandagatla wrote:
>
>> >+Optional CPU/CODEC subnodes properties:
>> >+- external	: flag to indicate if the I2S is connected to external codec
>> >+Example:
> Missing blank line between the property and the "Example:".  I'm still
I will fix it in next version.
> not sure I understand why we need a boolean property indicating if an
> external CODEC is in use - what is the consequence of setting this
> property?
As of today, the consequence of setting this flag is to setup correct 
dai_link names.
Also there are some limitations on which MI2S can be configured to 
external or internal codecs, this flag can be used in future to validate 
such configurations, if required.

--srini

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v5 1/2] ASoC: qcom: document apq8016 sbc machine driver bindings
  2015-06-09 17:08     ` Srinivas Kandagatla
@ 2015-06-09 17:13       ` Mark Brown
  2015-06-09 17:31         ` Srinivas Kandagatla
  0 siblings, 1 reply; 11+ messages in thread
From: Mark Brown @ 2015-06-09 17:13 UTC (permalink / raw)
  To: Srinivas Kandagatla
  Cc: alsa-devel, Rob Herring, Patrick Lai, Banajit Goswami,
	Liam Girdwood, Jaroslav Kysela, Takashi Iwai, devicetree,
	linux-kernel, kwestfie, linux-arm-msm

[-- Attachment #1: Type: text/plain, Size: 788 bytes --]

On Tue, Jun 09, 2015 at 06:08:24PM +0100, Srinivas Kandagatla wrote:

> >>>+- external	: flag to indicate if the I2S is connected to external codec

> >not sure I understand why we need a boolean property indicating if an
> >external CODEC is in use - what is the consequence of setting this
> >property?

> As of today, the consequence of setting this flag is to setup correct
> dai_link names.
> Also there are some limitations on which MI2S can be configured to external
> or internal codecs, this flag can be used in future to validate such
> configurations, if required.

That validation sounds like something that the SoC drivers should be
doing if required rather than the machine driver - otherwise every
machine driver using this SoC would have to implement the same
validation.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v5 1/2] ASoC: qcom: document apq8016 sbc machine driver bindings
  2015-06-09 17:13       ` Mark Brown
@ 2015-06-09 17:31         ` Srinivas Kandagatla
  0 siblings, 0 replies; 11+ messages in thread
From: Srinivas Kandagatla @ 2015-06-09 17:31 UTC (permalink / raw)
  To: Mark Brown
  Cc: alsa-devel, Rob Herring, Patrick Lai, Banajit Goswami,
	Liam Girdwood, Jaroslav Kysela, Takashi Iwai, devicetree,
	linux-kernel, kwestfie, linux-arm-msm



On 09/06/15 18:13, Mark Brown wrote:
> On Tue, Jun 09, 2015 at 06:08:24PM +0100, Srinivas Kandagatla wrote:
>
>>>>> +- external	: flag to indicate if the I2S is connected to external codec
>
>>> not sure I understand why we need a boolean property indicating if an
>>> external CODEC is in use - what is the consequence of setting this
>>> property?
>
>> As of today, the consequence of setting this flag is to setup correct
>> dai_link names.
>> Also there are some limitations on which MI2S can be configured to external
>> or internal codecs, this flag can be used in future to validate such
>> configurations, if required.
>
> That validation sounds like something that the SoC drivers should be
> doing if required rather than the machine driver - otherwise every
> machine driver using this SoC would have to implement the same
> validation.

Thats a valid point, for now I can move to using dai link name property 
instead of external flag, this should work for now. I will add the 
validation logic in SOC driver if required in future.
>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v5 2/2] ASoC: qcom: add apq8016 sound card support
  2015-06-09 17:07   ` Mark Brown
@ 2015-06-09 17:51     ` Srinivas Kandagatla
  2015-06-09 18:04       ` Mark Brown
  0 siblings, 1 reply; 11+ messages in thread
From: Srinivas Kandagatla @ 2015-06-09 17:51 UTC (permalink / raw)
  To: Mark Brown
  Cc: alsa-devel, Rob Herring, Patrick Lai, Banajit Goswami,
	Liam Girdwood, Jaroslav Kysela, Takashi Iwai, devicetree,
	linux-kernel, kwestfie, linux-arm-msm



On 09/06/15 18:07, Mark Brown wrote:
> On Tue, Jun 09, 2015 at 01:59:36PM +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) {
>> +		writel(readl(pdata->spkr_iomux) |
>> +			SPKR_CTL_PRI_WS_SLAVE_SEL_11,
>> +			pdata->spkr_iomux);
>> +
>> +		return 0;
>> +	}
>
> Why not just do these one time at probe, we don't undo them when we shut
> the DAI down?
If I do that Am afraid that the driver would loose the flexibility of 
selecting different MI2S from DT level. Hardcoding which MI2S can got to 
external or internal codec is something that I wanted to avoid from the 
start.

I will add the shutdown code to reset the configuration.

>
>> +
>> +	dev_err(card->dev, "unsupported cpu dai configuration\n");
>> +
>> +	return -EINVAL;
>
> It'd be clearer if this were part of the above code (which should still
> be written as a switch statement) - I was just asking myself what
> happens if we fall off the end of the if/else chain.

I will change this to switch case.

>
>> +		/**
>> +		 * External codec is ADV7533
>> +		 * and internal codec digital part is inside apq8016
>> +		 * and analog part is in PMIC PM8916
>> +		 **/
>> +		if (of_property_read_bool(np, "external"))
>> +			name = "ADV7533";
>> +		else
>> +			name = "WCD";
>
> If this is all the property is doing why not just put a link name
> property in the DT rather than this?  That makes the driver a bit more
> general and is more idiomatic.
Yes, it makes it more clear with link-name property. I will fix this in 
next version.
>
>> +		dai_link->name = dai_link->stream_name = name;
>
> Write two assignment statements, it's clearer and more idiomatic.
I will fix this too.
>
--srini

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v5 2/2] ASoC: qcom: add apq8016 sound card support
  2015-06-09 17:51     ` Srinivas Kandagatla
@ 2015-06-09 18:04       ` Mark Brown
  2015-06-09 18:22         ` Srinivas Kandagatla
  0 siblings, 1 reply; 11+ messages in thread
From: Mark Brown @ 2015-06-09 18:04 UTC (permalink / raw)
  To: Srinivas Kandagatla
  Cc: alsa-devel, Rob Herring, Patrick Lai, Banajit Goswami,
	Liam Girdwood, Jaroslav Kysela, Takashi Iwai, devicetree,
	linux-kernel, kwestfie, linux-arm-msm

[-- Attachment #1: Type: text/plain, Size: 607 bytes --]

On Tue, Jun 09, 2015 at 06:51:59PM +0100, Srinivas Kandagatla wrote:
> On 09/06/15 18:07, Mark Brown wrote:

> >Why not just do these one time at probe, we don't undo them when we shut
> >the DAI down?

> If I do that Am afraid that the driver would loose the flexibility of
> selecting different MI2S from DT level. Hardcoding which MI2S can got to
> external or internal codec is something that I wanted to avoid from the
> start.

I don't understand why we'd loose anything - we get init() callbacks on
the DAIs when they're instantiated?

> I will add the shutdown code to reset the configuration.

OK.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v5 2/2] ASoC: qcom: add apq8016 sound card support
  2015-06-09 18:04       ` Mark Brown
@ 2015-06-09 18:22         ` Srinivas Kandagatla
  0 siblings, 0 replies; 11+ messages in thread
From: Srinivas Kandagatla @ 2015-06-09 18:22 UTC (permalink / raw)
  To: Mark Brown
  Cc: alsa-devel, Rob Herring, Patrick Lai, Banajit Goswami,
	Liam Girdwood, Jaroslav Kysela, Takashi Iwai, devicetree,
	linux-kernel, kwestfie, linux-arm-msm



On 09/06/15 19:04, Mark Brown wrote:
> On Tue, Jun 09, 2015 at 06:51:59PM +0100, Srinivas Kandagatla wrote:
>> On 09/06/15 18:07, Mark Brown wrote:
>
>>> Why not just do these one time at probe, we don't undo them when we shut
>>> the DAI down?
>
>> If I do that Am afraid that the driver would loose the flexibility of
>> selecting different MI2S from DT level. Hardcoding which MI2S can got to
>> external or internal codec is something that I wanted to avoid from the
>> start.
>
> I don't understand why we'd loose anything - we get init() callbacks on
> the DAIs when they're instantiated?
>
Yes, got it. At dai_link init() level we can do it without losing any 
flexibility.

My bad, I thought you initially suggested me to add this to platform 
probe() level.

>> I will add the shutdown code to reset the configuration.
>
> OK.
>

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2015-06-09 18:30 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-06-09 12:58 [PATCH v5 0/2] ASoC: qcom: add support to apq8016 sbc machine driver Srinivas Kandagatla
2015-06-09 12:59 ` [PATCH v5 1/2] ASoC: qcom: document apq8016 sbc machine driver bindings Srinivas Kandagatla
2015-06-09 16:57   ` Mark Brown
2015-06-09 17:08     ` Srinivas Kandagatla
2015-06-09 17:13       ` Mark Brown
2015-06-09 17:31         ` Srinivas Kandagatla
2015-06-09 12:59 ` [PATCH v5 2/2] ASoC: qcom: add apq8016 sound card support Srinivas Kandagatla
2015-06-09 17:07   ` Mark Brown
2015-06-09 17:51     ` Srinivas Kandagatla
2015-06-09 18:04       ` Mark Brown
2015-06-09 18:22         ` Srinivas Kandagatla

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).