linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 0/4]  ASoC: Dell IoT Gateway audio support
@ 2017-01-12 12:00 Shrirang Bagul
  2017-01-12 12:00 ` [PATCH v2 1/4] ASoC: rt5660: Add ACPI support Shrirang Bagul
                   ` (3 more replies)
  0 siblings, 4 replies; 10+ messages in thread
From: Shrirang Bagul @ 2017-01-12 12:00 UTC (permalink / raw)
  To: alsa-devel; +Cc: linux-kernel

This work is based on for-next branch of:
git://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git

These patches add basic audio support on new Dell Edge IoT Gateways.
RT5660 codec is connected to SSP2 port on Intel Atom Baytrail E3815 SoC.
The machine driver is based on bytcr_rt5640. These devices have line-in and
line-out jacks with line-out mute enable gpio described in ACPI tables.

Since v1:
* rt5660: Use enumerations to define GPIO's described ACPI DSDT
* Initialize rt5660 controls from card struct
* Re-use existing bytcr_rt5640 machine driver to support rt5660 codec based
  audio

Shrirang Bagul (4):
  ASoC: rt5660: Add ACPI support
  ASoC: Intel: bytcr_rt5640: move codec clks to card driver data
  ASoC: Intel: Support rt5660 codec for Baytrail
  ASoC: Intel: bytcr_rt5640: Support line-out mute gpio

 sound/soc/codecs/rt5660.c             |  32 ++++++
 sound/soc/intel/Kconfig               |  11 +-
 sound/soc/intel/atom/sst/sst_acpi.c   |   2 +
 sound/soc/intel/boards/bytcr_rt5640.c | 211 ++++++++++++++++++++++++++++++----
 4 files changed, 228 insertions(+), 28 deletions(-)

-- 
2.9.3

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

* [PATCH v2 1/4] ASoC: rt5660: Add ACPI support
  2017-01-12 12:00 [PATCH v2 0/4] ASoC: Dell IoT Gateway audio support Shrirang Bagul
@ 2017-01-12 12:00 ` Shrirang Bagul
  2017-01-13  2:53   ` Bard Liao
  2017-01-12 12:00 ` [PATCH v2 2/4] ASoC: Intel: bytcr_rt5640: move codec clks to card driver data Shrirang Bagul
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 10+ messages in thread
From: Shrirang Bagul @ 2017-01-12 12:00 UTC (permalink / raw)
  To: alsa-devel
  Cc: linux-kernel, Bard Liao, Oder Chiou, Liam Girdwood, Mark Brown,
	Jaroslav Kysela, Takashi Iwai

On Dell IoT Gateways, RT5660 codec is available with ACPI ID 10EC3277.
Also, GPIO's are only available by index, so we register mappings to allow
machine drivers to access them by name.

Signed-off-by: Shrirang Bagul <shrirang.bagul@canonical.com>
---
 sound/soc/codecs/rt5660.c | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/sound/soc/codecs/rt5660.c b/sound/soc/codecs/rt5660.c
index 76cf76a..06cdcec 100644
--- a/sound/soc/codecs/rt5660.c
+++ b/sound/soc/codecs/rt5660.c
@@ -9,6 +9,7 @@
  * published by the Free Software Foundation.
  */
 
+#include <linux/acpi.h>
 #include <linux/module.h>
 #include <linux/moduleparam.h>
 #include <linux/init.h>
@@ -40,6 +41,12 @@
 
 #define RT5660_PR_BASE (RT5660_PR_RANGE_BASE + (0 * RT5660_PR_SPACING))
 
+/* GPIO indexes defined by ACPI */
+enum {
+	RT5660_GPIO_WAKE_INTR		= 0,
+	RT5660_GPIO_LINEOUT_MUTE	= 1,
+};
+
 static const struct regmap_range_cfg rt5660_ranges[] = {
 	{ .name = "PR", .range_min = RT5660_PR_BASE,
 	  .range_max = RT5660_PR_BASE + 0xf3,
@@ -1245,10 +1252,31 @@ MODULE_DEVICE_TABLE(of, rt5660_of_match);
 
 static const struct acpi_device_id rt5660_acpi_match[] = {
 	{ "10EC5660", 0 },
+	{ "10EC3277", 0 },
 	{ },
 };
 MODULE_DEVICE_TABLE(acpi, rt5660_acpi_match);
 
+static const struct acpi_gpio_params audio_wake_intr_gpio = { RT5660_GPIO_WAKE_INTR, 0, false };
+static const struct acpi_gpio_params lineout_mute_gpio = { RT5660_GPIO_LINEOUT_MUTE, 0, true };
+
+static const struct acpi_gpio_mapping byt_rt5660_gpios[] = {
+	{ "audio-wake-intr-gpios", &audio_wake_intr_gpio, 1 },
+	{ "lineout-mute-gpios", &lineout_mute_gpio, 1 },
+	{ NULL },
+};
+
+static void rt5660_read_acpi_properties(struct rt5660_priv *rt5660,
+		struct device *dev)
+{
+	int ret;
+
+	ret = acpi_dev_add_driver_gpios(ACPI_COMPANION(dev),
+			byt_rt5660_gpios);
+	if (ret)
+		dev_warn(dev, "Failed to add driver gpios\n");
+}
+
 static int rt5660_parse_dt(struct rt5660_priv *rt5660, struct device *dev)
 {
 	rt5660->pdata.in1_diff = device_property_read_bool(dev,
@@ -1288,6 +1316,10 @@ static int rt5660_i2c_probe(struct i2c_client *i2c,
 		rt5660->pdata = *pdata;
 	else if (i2c->dev.of_node)
 		rt5660_parse_dt(rt5660, &i2c->dev);
+	else if (ACPI_HANDLE(&i2c->dev))
+		rt5660_read_acpi_properties(rt5660, &i2c->dev);
+	else
+		return -EINVAL;
 
 	rt5660->regmap = devm_regmap_init_i2c(i2c, &rt5660_regmap);
 	if (IS_ERR(rt5660->regmap)) {
-- 
2.9.3

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

* [PATCH v2 2/4] ASoC: Intel: bytcr_rt5640: move codec clks to card driver data
  2017-01-12 12:00 [PATCH v2 0/4] ASoC: Dell IoT Gateway audio support Shrirang Bagul
  2017-01-12 12:00 ` [PATCH v2 1/4] ASoC: rt5660: Add ACPI support Shrirang Bagul
@ 2017-01-12 12:00 ` Shrirang Bagul
  2017-01-12 12:01 ` [PATCH v2 3/4] ASoC: Intel: Support rt5660 codec for Baytrail Shrirang Bagul
  2017-01-12 12:01 ` [PATCH v2 4/4] ASoC: Intel: bytcr_rt5640: Support line-out mute gpio Shrirang Bagul
  3 siblings, 0 replies; 10+ messages in thread
From: Shrirang Bagul @ 2017-01-12 12:00 UTC (permalink / raw)
  To: alsa-devel
  Cc: linux-kernel, Liam Girdwood, Mark Brown, Jaroslav Kysela,
	Takashi Iwai, Pierre-Louis Bossart, Shrirang Bagul, Irina Tirdea

This patch moves codec specific clock indices to card driver data.
Functions configuring clocks can be re-used for other codec chips with
similar features (viz. RT5660)

Signed-off-by: Shrirang Bagul <shrirang.bagul@canonical.com>
---
 sound/soc/intel/boards/bytcr_rt5640.c | 29 +++++++++++++++++++++++------
 1 file changed, 23 insertions(+), 6 deletions(-)

diff --git a/sound/soc/intel/boards/bytcr_rt5640.c b/sound/soc/intel/boards/bytcr_rt5640.c
index 1bd985f..f6fd397 100644
--- a/sound/soc/intel/boards/bytcr_rt5640.c
+++ b/sound/soc/intel/boards/bytcr_rt5640.c
@@ -53,8 +53,23 @@ enum {
 #define BYT_RT5640_MCLK_EN	BIT(22)
 #define BYT_RT5640_MCLK_25MHZ	BIT(23)
 
+enum {
+	SCLK_PLL1 = 0,
+	SCLK_RCCLK,
+	PLL1_BCLK,
+	PLL1_MCLK,
+};
+
 struct byt_rt5640_private {
 	struct clk *mclk;
+	int *clks;
+};
+
+static int byt_rt5640_clks[] = {
+	RT5640_SCLK_S_PLL1,
+	RT5640_SCLK_S_RCCLK,
+	RT5640_PLL1_S_BCLK1,
+	RT5640_PLL1_S_MCLK
 };
 
 static unsigned long byt_rt5640_quirk = BYT_RT5640_MCLK_EN;
@@ -132,7 +147,7 @@ static int platform_clock_control(struct snd_soc_dapm_widget *w,
 				return ret;
 			}
 		}
-		ret = snd_soc_dai_set_sysclk(codec_dai, RT5640_SCLK_S_PLL1,
+		ret = snd_soc_dai_set_sysclk(codec_dai, priv->clks[SCLK_PLL1],
 					     48000 * 512,
 					     SND_SOC_CLOCK_IN);
 	} else {
@@ -141,7 +156,7 @@ static int platform_clock_control(struct snd_soc_dapm_widget *w,
 		 * turning off the platform clock. Codec needs clock
 		 * for Jack detection and button press
 		 */
-		ret = snd_soc_dai_set_sysclk(codec_dai, RT5640_SCLK_S_RCCLK,
+		ret = snd_soc_dai_set_sysclk(codec_dai, priv->clks[SCLK_RCCLK],
 					     48000 * 512,
 					     SND_SOC_CLOCK_IN);
 		if (!ret) {
@@ -259,6 +274,7 @@ static int byt_rt5640_aif1_hw_params(struct snd_pcm_substream *substream,
 {
 	struct snd_soc_pcm_runtime *rtd = substream->private_data;
 	struct snd_soc_dai *codec_dai = rtd->codec_dai;
+	struct byt_rt5640_private *priv = snd_soc_card_get_drvdata(rtd->card);
 	int ret;
 
 	ret = snd_soc_dai_set_sysclk(codec_dai, RT5640_SCLK_S_PLL1,
@@ -277,25 +293,25 @@ static int byt_rt5640_aif1_hw_params(struct snd_pcm_substream *substream,
 
 			/* 2x16 bit slots on SSP0 */
 			ret = snd_soc_dai_set_pll(codec_dai, 0,
-						RT5640_PLL1_S_BCLK1,
+						priv->clks[PLL1_BCLK],
 						params_rate(params) * 32,
 						params_rate(params) * 512);
 		} else {
 			/* 2x15 bit slots on SSP2 */
 			ret = snd_soc_dai_set_pll(codec_dai, 0,
-						RT5640_PLL1_S_BCLK1,
+						priv->clks[PLL1_BCLK],
 						params_rate(params) * 50,
 						params_rate(params) * 512);
 		}
 	} else {
 		if (byt_rt5640_quirk & BYT_RT5640_MCLK_25MHZ) {
 			ret = snd_soc_dai_set_pll(codec_dai, 0,
-						RT5640_PLL1_S_MCLK,
+						priv->clks[PLL1_MCLK],
 						25000000,
 						params_rate(params) * 512);
 		} else {
 			ret = snd_soc_dai_set_pll(codec_dai, 0,
-						RT5640_PLL1_S_MCLK,
+						priv->clks[PLL1_MCLK],
 						19200000,
 						params_rate(params) * 512);
 		}
@@ -717,6 +733,7 @@ static int snd_byt_rt5640_mc_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	/* register the soc card */
+	priv->clks = byt_rt5640_clks;
 	byt_rt5640_card.dev = &pdev->dev;
 	mach = byt_rt5640_card.dev->platform_data;
 	snd_soc_card_set_drvdata(&byt_rt5640_card, priv);
-- 
2.9.3

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

* [PATCH v2 3/4] ASoC: Intel: Support rt5660 codec for Baytrail
  2017-01-12 12:00 [PATCH v2 0/4] ASoC: Dell IoT Gateway audio support Shrirang Bagul
  2017-01-12 12:00 ` [PATCH v2 1/4] ASoC: rt5660: Add ACPI support Shrirang Bagul
  2017-01-12 12:00 ` [PATCH v2 2/4] ASoC: Intel: bytcr_rt5640: move codec clks to card driver data Shrirang Bagul
@ 2017-01-12 12:01 ` Shrirang Bagul
  2017-01-12 14:40   ` [alsa-devel] " Pierre-Louis Bossart
  2017-01-12 12:01 ` [PATCH v2 4/4] ASoC: Intel: bytcr_rt5640: Support line-out mute gpio Shrirang Bagul
  3 siblings, 1 reply; 10+ messages in thread
From: Shrirang Bagul @ 2017-01-12 12:01 UTC (permalink / raw)
  To: alsa-devel
  Cc: linux-kernel, Liam Girdwood, Mark Brown, Jaroslav Kysela,
	Takashi Iwai, Vinod Koul, Jeeja KP, Pierre-Louis Bossart,
	Sathyanarayana Nujella, Shrirang Bagul, Ramesh Babu,
	John Keeping, Arnd Bergmann, Colin Ian King, Wei Yongjun,
	Irina Tirdea

rt5660 and rt5640 are similar codecs so reuse the bytcr_rt5640 driver.
RT5660 codec is used on Dell Edge IoT Gateways with ACPI ID 10EC3277.
These devices sport only Line-In and Line-Out jacks.

Signed-off-by: Shrirang Bagul <shrirang.bagul@canonical.com>
---
 sound/soc/intel/Kconfig               |  11 +--
 sound/soc/intel/atom/sst/sst_acpi.c   |   2 +
 sound/soc/intel/boards/bytcr_rt5640.c | 156 ++++++++++++++++++++++++++++++----
 3 files changed, 147 insertions(+), 22 deletions(-)

diff --git a/sound/soc/intel/Kconfig b/sound/soc/intel/Kconfig
index fd5d1e0..0b43b6a 100644
--- a/sound/soc/intel/Kconfig
+++ b/sound/soc/intel/Kconfig
@@ -147,17 +147,18 @@ config SND_SOC_INTEL_BROADWELL_MACH
 	  If unsure select "N".
 
 config SND_SOC_INTEL_BYTCR_RT5640_MACH
-        tristate "ASoC Audio driver for Intel Baytrail and Baytrail-CR with RT5640 codec"
+	tristate "ASoC Audio driver for Intel Baytrail and Baytrail-CR with RT5640/5660 codec"
 	depends on X86 && I2C && ACPI
 	select SND_SOC_RT5640
+	select SND_SOC_RT5660
 	select SND_SST_MFLD_PLATFORM
 	select SND_SST_IPC_ACPI
 	select SND_SOC_INTEL_SST_MATCH if ACPI
 	help
-          This adds support for ASoC machine driver for Intel(R) Baytrail and Baytrail-CR
-          platforms with RT5640 audio codec.
-          Say Y if you have such a device.
-          If unsure select "N".
+	  This adds support for ASoC machine driver for Intel(R) Baytrail and Baytrail-CR
+	  platforms with RT5640, RT5460 audio codec.
+	  Say Y if you have such a device.
+	  If unsure select "N".
 
 config SND_SOC_INTEL_BYTCR_RT5651_MACH
         tristate "ASoC Audio driver for Intel Baytrail and Baytrail-CR with RT5651 codec"
diff --git a/sound/soc/intel/atom/sst/sst_acpi.c b/sound/soc/intel/atom/sst/sst_acpi.c
index f4d92bb..d401457f 100644
--- a/sound/soc/intel/atom/sst/sst_acpi.c
+++ b/sound/soc/intel/atom/sst/sst_acpi.c
@@ -441,6 +441,8 @@ static struct sst_acpi_mach sst_acpi_bytcr[] = {
 						&byt_rvp_platform_data },
 	{"10EC5642", "bytcr_rt5640", "intel/fw_sst_0f28.bin", "bytcr_rt5640", NULL,
 						&byt_rvp_platform_data },
+	{"10EC3277", "bytcr_rt5640", "intel/fw_sst_0f28.bin", "bytcr_rt5640", NULL,
+						&byt_rvp_platform_data },
 	{"INTCCFFD", "bytcr_rt5640", "intel/fw_sst_0f28.bin", "bytcr_rt5640", NULL,
 						&byt_rvp_platform_data },
 	{"10EC5651", "bytcr_rt5651", "intel/fw_sst_0f28.bin", "bytcr_rt5651", NULL,
diff --git a/sound/soc/intel/boards/bytcr_rt5640.c b/sound/soc/intel/boards/bytcr_rt5640.c
index f6fd397..e8c9a01 100644
--- a/sound/soc/intel/boards/bytcr_rt5640.c
+++ b/sound/soc/intel/boards/bytcr_rt5640.c
@@ -32,11 +32,17 @@
 #include <sound/soc.h>
 #include <sound/jack.h>
 #include "../../codecs/rt5640.h"
+#include "../../codecs/rt5660.h"
 #include "../atom/sst-atom-controls.h"
 #include "../common/sst-acpi.h"
 #include "../common/sst-dsp.h"
 
 enum {
+	CODEC_TYPE_RT5640,
+	CODEC_TYPE_RT5660,
+};
+
+enum {
 	BYT_RT5640_DMIC1_MAP,
 	BYT_RT5640_DMIC2_MAP,
 	BYT_RT5640_IN1_MAP,
@@ -60,8 +66,16 @@ enum {
 	PLL1_MCLK,
 };
 
+struct byt_acpi_card {
+	char *codec_id;
+	int codec_type;
+	struct snd_soc_card *soc_card;
+};
+
 struct byt_rt5640_private {
+	struct byt_acpi_card *acpi_card;
 	struct clk *mclk;
+	char codec_name[16];
 	int *clks;
 };
 
@@ -72,6 +86,13 @@ static int byt_rt5640_clks[] = {
 	RT5640_PLL1_S_MCLK
 };
 
+static int byt_rt5660_clks[] = {
+	RT5660_SCLK_S_PLL1,
+	RT5660_SCLK_S_RCCLK,
+	RT5660_PLL1_S_BCLK,
+	RT5660_PLL1_S_MCLK
+};
+
 static unsigned long byt_rt5640_quirk = BYT_RT5640_MCLK_EN;
 
 static void log_quirks(struct device *dev)
@@ -105,6 +126,7 @@ static void log_quirks(struct device *dev)
 
 #define BYT_CODEC_DAI1	"rt5640-aif1"
 #define BYT_CODEC_DAI2	"rt5640-aif2"
+#define BYT_CODEC_DAI3	"rt5660-aif1"
 
 static inline struct snd_soc_dai *byt_get_codec_dai(struct snd_soc_card *card)
 {
@@ -117,6 +139,9 @@ static inline struct snd_soc_dai *byt_get_codec_dai(struct snd_soc_card *card)
 		if (!strncmp(rtd->codec_dai->name, BYT_CODEC_DAI2,
 				strlen(BYT_CODEC_DAI2)))
 			return rtd->codec_dai;
+		if (!strncmp(rtd->codec_dai->name, BYT_CODEC_DAI3,
+				strlen(BYT_CODEC_DAI3)))
+			return rtd->codec_dai;
 
 	}
 	return NULL;
@@ -269,6 +294,29 @@ static const struct snd_kcontrol_new byt_rt5640_controls[] = {
 	SOC_DAPM_PIN_SWITCH("Speaker"),
 };
 
+static const struct snd_soc_dapm_widget byt_rt5660_widgets[] = {
+	SND_SOC_DAPM_MIC("Line In", NULL),
+	SND_SOC_DAPM_LINE("Line Out", NULL),
+	SND_SOC_DAPM_SUPPLY("Platform Clock", SND_SOC_NOPM, 0, 0,
+			platform_clock_control, SND_SOC_DAPM_PRE_PMU |
+			SND_SOC_DAPM_POST_PMD),
+};
+
+static const struct snd_soc_dapm_route byt_rt5660_audio_map[] = {
+	{"IN1P", NULL, "Line In"},
+	{"IN2P", NULL, "Line In"},
+	{"Line Out", NULL, "LOUTR"},
+	{"Line Out", NULL, "LOUTL"},
+
+	{"Line In", NULL, "Platform Clock"},
+	{"Line Out", NULL, "Platform Clock"},
+};
+
+static const struct snd_kcontrol_new byt_rt5660_controls[] = {
+	SOC_DAPM_PIN_SWITCH("Line In"),
+	SOC_DAPM_PIN_SWITCH("Line Out"),
+};
+
 static int byt_rt5640_aif1_hw_params(struct snd_pcm_substream *substream,
 					struct snd_pcm_hw_params *params)
 {
@@ -422,11 +470,8 @@ static int byt_rt5640_init(struct snd_soc_pcm_runtime *runtime)
 	struct snd_soc_codec *codec = runtime->codec;
 	struct snd_soc_card *card = runtime->card;
 	const struct snd_soc_dapm_route *custom_map;
-	struct byt_rt5640_private *priv = snd_soc_card_get_drvdata(card);
 	int num_routes;
 
-	card->dapm.idle_bias_off = true;
-
 	rt5640_sel_asrc_clk_src(codec,
 				RT5640_DA_STEREO_FILTER |
 				RT5640_DA_MONO_L_FILTER	|
@@ -511,6 +556,39 @@ static int byt_rt5640_init(struct snd_soc_pcm_runtime *runtime)
 	snd_soc_dapm_ignore_suspend(&card->dapm, "Headphone");
 	snd_soc_dapm_ignore_suspend(&card->dapm, "Speaker");
 
+	return ret;
+}
+
+static int byt_rt5660_init(struct snd_soc_pcm_runtime *runtime)
+{
+	int ret;
+	struct snd_soc_card *card = runtime->card;
+
+	ret = snd_soc_dapm_add_routes(&card->dapm,
+			byt_rt5640_ssp2_aif1_map,
+			ARRAY_SIZE(byt_rt5640_ssp2_aif1_map));
+	if (ret)
+		return ret;
+
+	snd_soc_dapm_enable_pin(&card->dapm, "Line In");
+	snd_soc_dapm_enable_pin(&card->dapm, "Line Out");
+
+	return ret;
+}
+
+static int byt_rt56x0_init(struct snd_soc_pcm_runtime *runtime)
+{
+	int ret;
+	struct snd_soc_card *card = runtime->card;
+	struct byt_rt5640_private *priv = snd_soc_card_get_drvdata(card);
+
+	card->dapm.idle_bias_off = true;
+
+	if (priv->acpi_card->codec_type == CODEC_TYPE_RT5640)
+		ret = byt_rt5640_init(runtime);
+	else
+		ret = byt_rt5660_init(runtime);
+
 	if ((byt_rt5640_quirk & BYT_RT5640_MCLK_EN) && priv->mclk) {
 		/*
 		 * The firmware might enable the clock at
@@ -679,7 +757,7 @@ static struct snd_soc_dai_link byt_rt5640_dais[] = {
 		.ignore_suspend = 1,
 		.dpcm_playback = 1,
 		.dpcm_capture = 1,
-		.init = byt_rt5640_init,
+		.init = byt_rt56x0_init,
 		.ops = &byt_rt5640_be_ssp2_ops,
 	},
 };
@@ -697,6 +775,25 @@ static struct snd_soc_card byt_rt5640_card = {
 	.fully_routed = true,
 };
 
+static struct snd_soc_card byt_rt5660_card = {
+	.name = "bytcr-rt5660",
+	.owner = THIS_MODULE,
+	.dai_link = byt_rt5640_dais,
+	.num_links = ARRAY_SIZE(byt_rt5640_dais),
+	.controls = byt_rt5660_controls,
+	.num_controls = ARRAY_SIZE(byt_rt5660_controls),
+	.dapm_widgets = byt_rt5660_widgets,
+	.num_dapm_widgets = ARRAY_SIZE(byt_rt5660_widgets),
+	.dapm_routes = byt_rt5660_audio_map,
+	.num_dapm_routes = ARRAY_SIZE(byt_rt5660_audio_map),
+	.fully_routed = true,
+};
+
+static struct byt_acpi_card snd_soc_cards[] = {
+	{"10EC5640", CODEC_TYPE_RT5640, &byt_rt5640_card},
+	{"10EC3277", CODEC_TYPE_RT5660, &byt_rt5660_card},
+};
+
 static char byt_rt5640_codec_name[16]; /* i2c-<HID>:00 with HID being 8 chars */
 static char byt_rt5640_codec_aif_name[12]; /*  = "rt5640-aif[1|2]" */
 static char byt_rt5640_cpu_dai_name[10]; /*  = "ssp[0|2]-port" */
@@ -721,41 +818,51 @@ struct acpi_chan_package {   /* ACPICA seems to require 64 bit integers */
 static int snd_byt_rt5640_mc_probe(struct platform_device *pdev)
 {
 	int ret_val = 0;
-	struct sst_acpi_mach *mach;
-	const char *i2c_name = NULL;
 	int i;
-	int dai_index;
 	struct byt_rt5640_private *priv;
+	struct snd_soc_card *card = snd_soc_cards[0].soc_card;
+	struct sst_acpi_mach *mach;
+	const char *i2c_name = NULL;
+	int dai_index = 0;
 	bool is_bytcr = false;
 
 	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_ATOMIC);
 	if (!priv)
 		return -ENOMEM;
 
+	for (i = 0; i < ARRAY_SIZE(snd_soc_cards); i++) {
+		if (acpi_dev_found(snd_soc_cards[i].codec_id)) {
+			dev_dbg(&pdev->dev,
+				"found codec %s\n", snd_soc_cards[i].codec_id);
+			card = snd_soc_cards[i].soc_card;
+			priv->acpi_card = &snd_soc_cards[i];
+			break;
+		}
+	}
+
 	/* register the soc card */
 	priv->clks = byt_rt5640_clks;
-	byt_rt5640_card.dev = &pdev->dev;
-	mach = byt_rt5640_card.dev->platform_data;
-	snd_soc_card_set_drvdata(&byt_rt5640_card, priv);
+	card->dev = &pdev->dev;
+	mach = card->dev->platform_data;
+	sprintf(priv->codec_name, "i2c-%s:00", priv->acpi_card->codec_id);
 
 	/* fix index of codec dai */
-	dai_index = MERR_DPCM_COMPR + 1;
-	for (i = 0; i < ARRAY_SIZE(byt_rt5640_dais); i++) {
+	for (i = 0; i < ARRAY_SIZE(byt_rt5640_dais); i++)
 		if (!strcmp(byt_rt5640_dais[i].codec_name, "i2c-10EC5640:00")) {
+			card->dai_link[i].codec_name = priv->codec_name;
 			dai_index = i;
-			break;
 		}
-	}
 
 	/* fixup codec name based on HID */
 	i2c_name = sst_acpi_find_name_from_hid(mach->id);
 	if (i2c_name != NULL) {
 		snprintf(byt_rt5640_codec_name, sizeof(byt_rt5640_codec_name),
 			"%s%s", "i2c-", i2c_name);
-
 		byt_rt5640_dais[dai_index].codec_name = byt_rt5640_codec_name;
 	}
 
+	snd_soc_card_set_drvdata(card, priv);
+
 	/*
 	 * swap SSP0 if bytcr is detected
 	 * (will be overridden if DMI quirk is detected)
@@ -821,6 +928,21 @@ static int snd_byt_rt5640_mc_probe(struct platform_device *pdev)
 				BYT_RT5640_DMIC_EN);
 	}
 
+	if (priv->acpi_card->codec_type == CODEC_TYPE_RT5660) {
+		priv->clks = byt_rt5660_clks;
+
+		/* fixup codec aif name for rt5660 */
+		snprintf(byt_rt5640_codec_aif_name,
+			sizeof(byt_rt5640_codec_aif_name),
+			"%s", "rt5660-aif1");
+
+		byt_rt5640_dais[dai_index].codec_dai_name =
+			byt_rt5640_codec_aif_name;
+
+		/* setup codec quirks for rt5660 */
+		byt_rt5640_quirk = BYT_RT5640_MCLK_EN;
+	}
+
 	/* check quirks before creating card */
 	dmi_check_system(byt_rt5640_quirk_table);
 	log_quirks(&pdev->dev);
@@ -869,14 +991,14 @@ static int snd_byt_rt5640_mc_probe(struct platform_device *pdev)
 		}
 	}
 
-	ret_val = devm_snd_soc_register_card(&pdev->dev, &byt_rt5640_card);
+	ret_val = devm_snd_soc_register_card(&pdev->dev, card);
 
 	if (ret_val) {
 		dev_err(&pdev->dev, "devm_snd_soc_register_card failed %d\n",
 			ret_val);
 		return ret_val;
 	}
-	platform_set_drvdata(pdev, &byt_rt5640_card);
+	platform_set_drvdata(pdev, card);
 	return ret_val;
 }
 
-- 
2.9.3

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

* [PATCH v2 4/4] ASoC: Intel: bytcr_rt5640: Support line-out mute gpio
  2017-01-12 12:00 [PATCH v2 0/4] ASoC: Dell IoT Gateway audio support Shrirang Bagul
                   ` (2 preceding siblings ...)
  2017-01-12 12:01 ` [PATCH v2 3/4] ASoC: Intel: Support rt5660 codec for Baytrail Shrirang Bagul
@ 2017-01-12 12:01 ` Shrirang Bagul
  3 siblings, 0 replies; 10+ messages in thread
From: Shrirang Bagul @ 2017-01-12 12:01 UTC (permalink / raw)
  To: alsa-devel
  Cc: linux-kernel, Liam Girdwood, Mark Brown, Jaroslav Kysela,
	Takashi Iwai, Pierre-Louis Bossart, Shrirang Bagul, Irina Tirdea

This patch adds support to toggle mute on 'line-out' pin on Dell Edge
IoT Gateways. A dedicated GPIO on the SoC is used to control the amplifier.
This GPIO is described in the BIOS DSD table.

Signed-off-by: Shrirang Bagul <shrirang.bagul@canonical.com>
---
 sound/soc/intel/boards/bytcr_rt5640.c | 28 +++++++++++++++++++++++++++-
 1 file changed, 27 insertions(+), 1 deletion(-)

diff --git a/sound/soc/intel/boards/bytcr_rt5640.c b/sound/soc/intel/boards/bytcr_rt5640.c
index e8c9a01..ae53a29 100644
--- a/sound/soc/intel/boards/bytcr_rt5640.c
+++ b/sound/soc/intel/boards/bytcr_rt5640.c
@@ -20,6 +20,7 @@
 #include <linux/init.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
+#include <linux/gpio/consumer.h>
 #include <linux/acpi.h>
 #include <linux/device.h>
 #include <linux/dmi.h>
@@ -74,6 +75,7 @@ struct byt_acpi_card {
 
 struct byt_rt5640_private {
 	struct byt_acpi_card *acpi_card;
+	struct gpio_desc *gpio_lo_mute;
 	struct clk *mclk;
 	char codec_name[16];
 	int *clks;
@@ -198,6 +200,19 @@ static int platform_clock_control(struct snd_soc_dapm_widget *w,
 	return 0;
 }
 
+static int byt_rt5660_event_lineout(struct snd_soc_dapm_widget *w,
+			struct snd_kcontrol *k, int event)
+{
+	struct snd_soc_dapm_context *dapm = w->dapm;
+	struct snd_soc_card *card = dapm->card;
+	struct byt_rt5640_private *priv = snd_soc_card_get_drvdata(card);
+
+	gpiod_set_value_cansleep(priv->gpio_lo_mute,
+			!(SND_SOC_DAPM_EVENT_ON(event)));
+
+	return 0;
+}
+
 static const struct snd_soc_dapm_widget byt_rt5640_widgets[] = {
 	SND_SOC_DAPM_HP("Headphone", NULL),
 	SND_SOC_DAPM_MIC("Headset Mic", NULL),
@@ -296,7 +311,7 @@ static const struct snd_kcontrol_new byt_rt5640_controls[] = {
 
 static const struct snd_soc_dapm_widget byt_rt5660_widgets[] = {
 	SND_SOC_DAPM_MIC("Line In", NULL),
-	SND_SOC_DAPM_LINE("Line Out", NULL),
+	SND_SOC_DAPM_LINE("Line Out", byt_rt5660_event_lineout),
 	SND_SOC_DAPM_SUPPLY("Platform Clock", SND_SOC_NOPM, 0, 0,
 			platform_clock_control, SND_SOC_DAPM_PRE_PMU |
 			SND_SOC_DAPM_POST_PMD),
@@ -562,7 +577,18 @@ static int byt_rt5640_init(struct snd_soc_pcm_runtime *runtime)
 static int byt_rt5660_init(struct snd_soc_pcm_runtime *runtime)
 {
 	int ret;
+	struct snd_soc_codec *codec = runtime->codec;
 	struct snd_soc_card *card = runtime->card;
+	struct byt_rt5640_private *priv = snd_soc_card_get_drvdata(card);
+
+	/* Request rt5660 GPIO for lineout mute control */
+	priv->gpio_lo_mute = devm_gpiod_get_index(codec->dev,
+			"lineout-mute", 0, 0);
+	if (IS_ERR(priv->gpio_lo_mute)) {
+		dev_err(card->dev, "Can't find GPIO_MUTE# gpio\n");
+		return PTR_ERR(priv->gpio_lo_mute);
+	}
+	gpiod_direction_output(priv->gpio_lo_mute, 1);
 
 	ret = snd_soc_dapm_add_routes(&card->dapm,
 			byt_rt5640_ssp2_aif1_map,
-- 
2.9.3

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

* Re: [alsa-devel] [PATCH v2 3/4] ASoC: Intel: Support rt5660 codec for Baytrail
  2017-01-12 12:01 ` [PATCH v2 3/4] ASoC: Intel: Support rt5660 codec for Baytrail Shrirang Bagul
@ 2017-01-12 14:40   ` Pierre-Louis Bossart
  2017-01-16  7:45     ` Shrirang Bagul
  0 siblings, 1 reply; 10+ messages in thread
From: Pierre-Louis Bossart @ 2017-01-12 14:40 UTC (permalink / raw)
  To: Shrirang Bagul, alsa-devel
  Cc: Arnd Bergmann, Liam Girdwood, Vinod Koul, linux-kernel,
	Irina Tirdea, Takashi Iwai, Ramesh Babu, Mark Brown,
	John Keeping, Sathyanarayana Nujella, Wei Yongjun,
	Colin Ian King, Jeeja KP

On 1/12/17 6:01 AM, Shrirang Bagul wrote:
> rt5660 and rt5640 are similar codecs so reuse the bytcr_rt5640 driver.
> RT5660 codec is used on Dell Edge IoT Gateways with ACPI ID 10EC3277.
> These devices sport only Line-In and Line-Out jacks.

While it would be nice to avoid copy/pasting everytime we add a new 
codec and refactor the code, I am not comfortable with a series of 
changes below.
Also if we do this refactoring then we might as well do it for rt5651 
which is similar and only relies on I2S. other machine drivers enable 
TDM mode when possible.
And last this change has a lot of impact on how we deal with UCM files. 
The name of the card should reflect which codec is used, and the quirks 
be added to the long name. If you lump everything with a single name 
then you will make it really hard for userspace to figure out which 
controls need to be set.

So nice idea but too early to merge. NAK.

>
> Signed-off-by: Shrirang Bagul <shrirang.bagul@canonical.com>
> ---
>  sound/soc/intel/Kconfig               |  11 +--
>  sound/soc/intel/atom/sst/sst_acpi.c   |   2 +
>  sound/soc/intel/boards/bytcr_rt5640.c | 156 ++++++++++++++++++++++++++++++----
>  3 files changed, 147 insertions(+), 22 deletions(-)
>
> diff --git a/sound/soc/intel/Kconfig b/sound/soc/intel/Kconfig
> index fd5d1e0..0b43b6a 100644
> --- a/sound/soc/intel/Kconfig
> +++ b/sound/soc/intel/Kconfig
> @@ -147,17 +147,18 @@ config SND_SOC_INTEL_BROADWELL_MACH
>  	  If unsure select "N".
>
>  config SND_SOC_INTEL_BYTCR_RT5640_MACH
> -        tristate "ASoC Audio driver for Intel Baytrail and Baytrail-CR with RT5640 codec"
> +	tristate "ASoC Audio driver for Intel Baytrail and Baytrail-CR with RT5640/5660 codec"
>  	depends on X86 && I2C && ACPI
>  	select SND_SOC_RT5640
> +	select SND_SOC_RT5660
>  	select SND_SST_MFLD_PLATFORM
>  	select SND_SST_IPC_ACPI
>  	select SND_SOC_INTEL_SST_MATCH if ACPI
>  	help
> -          This adds support for ASoC machine driver for Intel(R) Baytrail and Baytrail-CR
> -          platforms with RT5640 audio codec.
> -          Say Y if you have such a device.
> -          If unsure select "N".
> +	  This adds support for ASoC machine driver for Intel(R) Baytrail and Baytrail-CR
> +	  platforms with RT5640, RT5460 audio codec.
> +	  Say Y if you have such a device.
> +	  If unsure select "N".
>
>  config SND_SOC_INTEL_BYTCR_RT5651_MACH
>          tristate "ASoC Audio driver for Intel Baytrail and Baytrail-CR with RT5651 codec"
> diff --git a/sound/soc/intel/atom/sst/sst_acpi.c b/sound/soc/intel/atom/sst/sst_acpi.c
> index f4d92bb..d401457f 100644
> --- a/sound/soc/intel/atom/sst/sst_acpi.c
> +++ b/sound/soc/intel/atom/sst/sst_acpi.c
> @@ -441,6 +441,8 @@ static struct sst_acpi_mach sst_acpi_bytcr[] = {
>  						&byt_rvp_platform_data },
>  	{"10EC5642", "bytcr_rt5640", "intel/fw_sst_0f28.bin", "bytcr_rt5640", NULL,
>  						&byt_rvp_platform_data },
> +	{"10EC3277", "bytcr_rt5640", "intel/fw_sst_0f28.bin", "bytcr_rt5640", NULL,
> +						&byt_rvp_platform_data },

so right there you add an HID in the platform driver and you need the 
same in the platform driver to determine which codec type this is...

>  	{"INTCCFFD", "bytcr_rt5640", "intel/fw_sst_0f28.bin", "bytcr_rt5640", NULL,
>  						&byt_rvp_platform_data },
>  	{"10EC5651", "bytcr_rt5651", "intel/fw_sst_0f28.bin", "bytcr_rt5651", NULL,
> diff --git a/sound/soc/intel/boards/bytcr_rt5640.c b/sound/soc/intel/boards/bytcr_rt5640.c
> index f6fd397..e8c9a01 100644
> --- a/sound/soc/intel/boards/bytcr_rt5640.c
> +++ b/sound/soc/intel/boards/bytcr_rt5640.c
> @@ -32,11 +32,17 @@
>  #include <sound/soc.h>
>  #include <sound/jack.h>
>  #include "../../codecs/rt5640.h"
> +#include "../../codecs/rt5660.h"
>  #include "../atom/sst-atom-controls.h"
>  #include "../common/sst-acpi.h"
>  #include "../common/sst-dsp.h"
>
>  enum {
> +	CODEC_TYPE_RT5640,
> +	CODEC_TYPE_RT5660,
> +};
> +
> +enum {
>  	BYT_RT5640_DMIC1_MAP,
>  	BYT_RT5640_DMIC2_MAP,
>  	BYT_RT5640_IN1_MAP,
> @@ -60,8 +66,16 @@ enum {
>  	PLL1_MCLK,
>  };
>
> +struct byt_acpi_card {
> +	char *codec_id;
> +	int codec_type;
> +	struct snd_soc_card *soc_card;
> +};
> +
>  struct byt_rt5640_private {
> +	struct byt_acpi_card *acpi_card;
>  	struct clk *mclk;
> +	char codec_name[16];
>  	int *clks;
>  };
>
> @@ -72,6 +86,13 @@ static int byt_rt5640_clks[] = {
>  	RT5640_PLL1_S_MCLK
>  };
>
> +static int byt_rt5660_clks[] = {
> +	RT5660_SCLK_S_PLL1,
> +	RT5660_SCLK_S_RCCLK,
> +	RT5660_PLL1_S_BCLK,
> +	RT5660_PLL1_S_MCLK
> +};
> +
>  static unsigned long byt_rt5640_quirk = BYT_RT5640_MCLK_EN;

the quirks would need to be isolated and made dependent on codec type

>
>  static void log_quirks(struct device *dev)
> @@ -105,6 +126,7 @@ static void log_quirks(struct device *dev)
>
>  #define BYT_CODEC_DAI1	"rt5640-aif1"
>  #define BYT_CODEC_DAI2	"rt5640-aif2"
> +#define BYT_CODEC_DAI3	"rt5660-aif1"
>
>  static inline struct snd_soc_dai *byt_get_codec_dai(struct snd_soc_card *card)
>  {
> @@ -117,6 +139,9 @@ static inline struct snd_soc_dai *byt_get_codec_dai(struct snd_soc_card *card)
>  		if (!strncmp(rtd->codec_dai->name, BYT_CODEC_DAI2,
>  				strlen(BYT_CODEC_DAI2)))
>  			return rtd->codec_dai;
> +		if (!strncmp(rtd->codec_dai->name, BYT_CODEC_DAI3,
> +				strlen(BYT_CODEC_DAI3)))
> +			return rtd->codec_dai;

not very good to go look for a DAI that doesn't exist for a specific 
codec. this would need to be dependent on codec type.

>
>  	}
>  	return NULL;
> @@ -269,6 +294,29 @@ static const struct snd_kcontrol_new byt_rt5640_controls[] = {
>  	SOC_DAPM_PIN_SWITCH("Speaker"),
>  };
>
> +static const struct snd_soc_dapm_widget byt_rt5660_widgets[] = {
> +	SND_SOC_DAPM_MIC("Line In", NULL),
> +	SND_SOC_DAPM_LINE("Line Out", NULL),
> +	SND_SOC_DAPM_SUPPLY("Platform Clock", SND_SOC_NOPM, 0, 0,
> +			platform_clock_control, SND_SOC_DAPM_PRE_PMU |
> +			SND_SOC_DAPM_POST_PMD),
> +};
> +
> +static const struct snd_soc_dapm_route byt_rt5660_audio_map[] = {
> +	{"IN1P", NULL, "Line In"},
> +	{"IN2P", NULL, "Line In"},
> +	{"Line Out", NULL, "LOUTR"},
> +	{"Line Out", NULL, "LOUTL"},
> +
> +	{"Line In", NULL, "Platform Clock"},
> +	{"Line Out", NULL, "Platform Clock"},
> +};
> +
> +static const struct snd_kcontrol_new byt_rt5660_controls[] = {
> +	SOC_DAPM_PIN_SWITCH("Line In"),
> +	SOC_DAPM_PIN_SWITCH("Line Out"),
> +};
> +
>  static int byt_rt5640_aif1_hw_params(struct snd_pcm_substream *substream,
>  					struct snd_pcm_hw_params *params)
>  {
> @@ -422,11 +470,8 @@ static int byt_rt5640_init(struct snd_soc_pcm_runtime *runtime)
>  	struct snd_soc_codec *codec = runtime->codec;
>  	struct snd_soc_card *card = runtime->card;
>  	const struct snd_soc_dapm_route *custom_map;
> -	struct byt_rt5640_private *priv = snd_soc_card_get_drvdata(card);
>  	int num_routes;
>
> -	card->dapm.idle_bias_off = true;
> -
>  	rt5640_sel_asrc_clk_src(codec,
>  				RT5640_DA_STEREO_FILTER |
>  				RT5640_DA_MONO_L_FILTER	|
> @@ -511,6 +556,39 @@ static int byt_rt5640_init(struct snd_soc_pcm_runtime *runtime)
>  	snd_soc_dapm_ignore_suspend(&card->dapm, "Headphone");
>  	snd_soc_dapm_ignore_suspend(&card->dapm, "Speaker");
>
> +	return ret;
> +}
> +
> +static int byt_rt5660_init(struct snd_soc_pcm_runtime *runtime)
> +{
> +	int ret;
> +	struct snd_soc_card *card = runtime->card;
> +
> +	ret = snd_soc_dapm_add_routes(&card->dapm,
> +			byt_rt5640_ssp2_aif1_map,
> +			ARRAY_SIZE(byt_rt5640_ssp2_aif1_map));
> +	if (ret)
> +		return ret;
> +
> +	snd_soc_dapm_enable_pin(&card->dapm, "Line In");
> +	snd_soc_dapm_enable_pin(&card->dapm, "Line Out");
> +
> +	return ret;
> +}
> +
> +static int byt_rt56x0_init(struct snd_soc_pcm_runtime *runtime)
> +{
> +	int ret;
> +	struct snd_soc_card *card = runtime->card;
> +	struct byt_rt5640_private *priv = snd_soc_card_get_drvdata(card);
> +
> +	card->dapm.idle_bias_off = true;
> +
> +	if (priv->acpi_card->codec_type == CODEC_TYPE_RT5640)
> +		ret = byt_rt5640_init(runtime);
> +	else
> +		ret = byt_rt5660_init(runtime);
> +
>  	if ((byt_rt5640_quirk & BYT_RT5640_MCLK_EN) && priv->mclk) {
>  		/*
>  		 * The firmware might enable the clock at
> @@ -679,7 +757,7 @@ static struct snd_soc_dai_link byt_rt5640_dais[] = {
>  		.ignore_suspend = 1,
>  		.dpcm_playback = 1,
>  		.dpcm_capture = 1,
> -		.init = byt_rt5640_init,
> +		.init = byt_rt56x0_init,
>  		.ops = &byt_rt5640_be_ssp2_ops,
>  	},
>  };
> @@ -697,6 +775,25 @@ static struct snd_soc_card byt_rt5640_card = {
>  	.fully_routed = true,
>  };
>
> +static struct snd_soc_card byt_rt5660_card = {
> +	.name = "bytcr-rt5660",
> +	.owner = THIS_MODULE,
> +	.dai_link = byt_rt5640_dais,
> +	.num_links = ARRAY_SIZE(byt_rt5640_dais),
> +	.controls = byt_rt5660_controls,
> +	.num_controls = ARRAY_SIZE(byt_rt5660_controls),
> +	.dapm_widgets = byt_rt5660_widgets,
> +	.num_dapm_widgets = ARRAY_SIZE(byt_rt5660_widgets),
> +	.dapm_routes = byt_rt5660_audio_map,
> +	.num_dapm_routes = ARRAY_SIZE(byt_rt5660_audio_map),
> +	.fully_routed = true,
> +};
> +
> +static struct byt_acpi_card snd_soc_cards[] = {
> +	{"10EC5640", CODEC_TYPE_RT5640, &byt_rt5640_card},
> +	{"10EC3277", CODEC_TYPE_RT5660, &byt_rt5660_card},
> +};

I know this is what's used for rt5645/50 but I don't like it and don't 
think it should be the baseline for how we deal with codecs. This forces 
the addition of additional quirks.

> +
>  static char byt_rt5640_codec_name[16]; /* i2c-<HID>:00 with HID being 8 chars */
>  static char byt_rt5640_codec_aif_name[12]; /*  = "rt5640-aif[1|2]" */
>  static char byt_rt5640_cpu_dai_name[10]; /*  = "ssp[0|2]-port" */
> @@ -721,41 +818,51 @@ struct acpi_chan_package {   /* ACPICA seems to require 64 bit integers */
>  static int snd_byt_rt5640_mc_probe(struct platform_device *pdev)
>  {
>  	int ret_val = 0;
> -	struct sst_acpi_mach *mach;
> -	const char *i2c_name = NULL;
>  	int i;
> -	int dai_index;
>  	struct byt_rt5640_private *priv;
> +	struct snd_soc_card *card = snd_soc_cards[0].soc_card;
> +	struct sst_acpi_mach *mach;
> +	const char *i2c_name = NULL;
> +	int dai_index = 0;
>  	bool is_bytcr = false;
>
>  	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_ATOMIC);
>  	if (!priv)
>  		return -ENOMEM;
>
> +	for (i = 0; i < ARRAY_SIZE(snd_soc_cards); i++) {
> +		if (acpi_dev_found(snd_soc_cards[i].codec_id)) {
> +			dev_dbg(&pdev->dev,
> +				"found codec %s\n", snd_soc_cards[i].codec_id);
> +			card = snd_soc_cards[i].soc_card;
> +			priv->acpi_card = &snd_soc_cards[i];
> +			break;
> +		}
> +	}
> +
>  	/* register the soc card */
>  	priv->clks = byt_rt5640_clks;
> -	byt_rt5640_card.dev = &pdev->dev;
> -	mach = byt_rt5640_card.dev->platform_data;
> -	snd_soc_card_set_drvdata(&byt_rt5640_card, priv);
> +	card->dev = &pdev->dev;
> +	mach = card->dev->platform_data;
> +	sprintf(priv->codec_name, "i2c-%s:00", priv->acpi_card->codec_id);
>
>  	/* fix index of codec dai */
> -	dai_index = MERR_DPCM_COMPR + 1;
> -	for (i = 0; i < ARRAY_SIZE(byt_rt5640_dais); i++) {
> +	for (i = 0; i < ARRAY_SIZE(byt_rt5640_dais); i++)
>  		if (!strcmp(byt_rt5640_dais[i].codec_name, "i2c-10EC5640:00")) {
> +			card->dai_link[i].codec_name = priv->codec_name;
>  			dai_index = i;
> -			break;
>  		}
> -	}
>
>  	/* fixup codec name based on HID */
>  	i2c_name = sst_acpi_find_name_from_hid(mach->id);
>  	if (i2c_name != NULL) {
>  		snprintf(byt_rt5640_codec_name, sizeof(byt_rt5640_codec_name),
>  			"%s%s", "i2c-", i2c_name);
> -
>  		byt_rt5640_dais[dai_index].codec_name = byt_rt5640_codec_name;
>  	}
>
> +	snd_soc_card_set_drvdata(card, priv);
> +
>  	/*
>  	 * swap SSP0 if bytcr is detected
>  	 * (will be overridden if DMI quirk is detected)
> @@ -821,6 +928,21 @@ static int snd_byt_rt5640_mc_probe(struct platform_device *pdev)
>  				BYT_RT5640_DMIC_EN);
>  	}
>
> +	if (priv->acpi_card->codec_type == CODEC_TYPE_RT5660) {
> +		priv->clks = byt_rt5660_clks;
> +
> +		/* fixup codec aif name for rt5660 */
> +		snprintf(byt_rt5640_codec_aif_name,
> +			sizeof(byt_rt5640_codec_aif_name),
> +			"%s", "rt5660-aif1");
> +
> +		byt_rt5640_dais[dai_index].codec_dai_name =
> +			byt_rt5640_codec_aif_name;
> +
> +		/* setup codec quirks for rt5660 */
> +		byt_rt5640_quirk = BYT_RT5640_MCLK_EN;
> +	}
> +
>  	/* check quirks before creating card */
>  	dmi_check_system(byt_rt5640_quirk_table);

the quirks have to be separate.

>  	log_quirks(&pdev->dev);
> @@ -869,14 +991,14 @@ static int snd_byt_rt5640_mc_probe(struct platform_device *pdev)
>  		}
>  	}
>
> -	ret_val = devm_snd_soc_register_card(&pdev->dev, &byt_rt5640_card);
> +	ret_val = devm_snd_soc_register_card(&pdev->dev, card);
>
>  	if (ret_val) {
>  		dev_err(&pdev->dev, "devm_snd_soc_register_card failed %d\n",
>  			ret_val);
>  		return ret_val;
>  	}
> -	platform_set_drvdata(pdev, &byt_rt5640_card);
> +	platform_set_drvdata(pdev, card);
>  	return ret_val;
>  }
>
>

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

* RE: [PATCH v2 1/4] ASoC: rt5660: Add ACPI support
  2017-01-12 12:00 ` [PATCH v2 1/4] ASoC: rt5660: Add ACPI support Shrirang Bagul
@ 2017-01-13  2:53   ` Bard Liao
  2017-01-13 22:30     ` Andy Shevchenko
  0 siblings, 1 reply; 10+ messages in thread
From: Bard Liao @ 2017-01-13  2:53 UTC (permalink / raw)
  To: Shrirang Bagul, alsa-devel
  Cc: linux-kernel, Oder Chiou, Liam Girdwood, Mark Brown,
	Jaroslav Kysela, Takashi Iwai


> -----Original Message-----
> From: Shrirang Bagul [mailto:shrirang.bagul@canonical.com]
> Sent: Thursday, January 12, 2017 8:01 PM
> To: alsa-devel@alsa-project.org
> Cc: linux-kernel@vger.kernel.org; Bard Liao; Oder Chiou; Liam Girdwood; Mark
> Brown; Jaroslav Kysela; Takashi Iwai
> Subject: [PATCH v2 1/4] ASoC: rt5660: Add ACPI support
> 
> On Dell IoT Gateways, RT5660 codec is available with ACPI ID 10EC3277.
> Also, GPIO's are only available by index, so we register mappings to allow
> machine drivers to access them by name.
> 
> Signed-off-by: Shrirang Bagul <shrirang.bagul@canonical.com>
> ---
> +static const struct acpi_gpio_params audio_wake_intr_gpio =
> { RT5660_GPIO_WAKE_INTR, 0, false };
> +static const struct acpi_gpio_params lineout_mute_gpio =
> { RT5660_GPIO_LINEOUT_MUTE, 0, true };
> +
> +static const struct acpi_gpio_mapping byt_rt5660_gpios[] = {
> +	{ "audio-wake-intr-gpios", &audio_wake_intr_gpio, 1 },
> +	{ "lineout-mute-gpios", &lineout_mute_gpio, 1 },
> +	{ NULL },
> +};
> +

I am thinking if it is more suitable to move the gpio params to
machine driver? They are not codec's gpios and are only used
by machine driver. 

> --
> 2.9.3

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

* Re: [PATCH v2 1/4] ASoC: rt5660: Add ACPI support
  2017-01-13  2:53   ` Bard Liao
@ 2017-01-13 22:30     ` Andy Shevchenko
  0 siblings, 0 replies; 10+ messages in thread
From: Andy Shevchenko @ 2017-01-13 22:30 UTC (permalink / raw)
  To: Bard Liao
  Cc: Shrirang Bagul, alsa-devel, linux-kernel, Oder Chiou,
	Liam Girdwood, Mark Brown, Jaroslav Kysela, Takashi Iwai

On Fri, Jan 13, 2017 at 4:53 AM, Bard Liao <bardliao@realtek.com> wrote:
>> +static const struct acpi_gpio_mapping byt_rt5660_gpios[] = {
>> +     { "audio-wake-intr-gpios", &audio_wake_intr_gpio, 1 },
>> +     { "lineout-mute-gpios", &lineout_mute_gpio, 1 },
>> +     { NULL },
>> +};
>> +
>
> I am thinking if it is more suitable to move the gpio params to
> machine driver? They are not codec's gpios and are only used
> by machine driver.

Generally speaking those properties should come per ACPI ID. So, they
should share the same module which has MODULE_DEVICE_TABLE(acpi, ...)
defined.

-- 
With Best Regards,
Andy Shevchenko

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

* Re: [alsa-devel] [PATCH v2 3/4] ASoC: Intel: Support rt5660 codec for Baytrail
  2017-01-12 14:40   ` [alsa-devel] " Pierre-Louis Bossart
@ 2017-01-16  7:45     ` Shrirang Bagul
  2017-01-16 15:10       ` Pierre-Louis Bossart
  0 siblings, 1 reply; 10+ messages in thread
From: Shrirang Bagul @ 2017-01-16  7:45 UTC (permalink / raw)
  To: Pierre-Louis Bossart, alsa-devel
  Cc: Arnd Bergmann, Liam Girdwood, Vinod Koul, linux-kernel,
	Irina Tirdea, Takashi Iwai, Ramesh Babu, Mark Brown,
	John Keeping, Sathyanarayana Nujella, Wei Yongjun,
	Colin Ian King, Jeeja KP

On Thu, 2017-01-12 at 08:40 -0600, Pierre-Louis Bossart wrote:
> On 1/12/17 6:01 AM, Shrirang Bagul wrote:
> > rt5660 and rt5640 are similar codecs so reuse the bytcr_rt5640 driver.
> > RT5660 codec is used on Dell Edge IoT Gateways with ACPI ID 10EC3277.
> > These devices sport only Line-In and Line-Out jacks.
> 
> While it would be nice to avoid copy/pasting everytime we add a new 
> codec and refactor the code, I am not comfortable with a series of 
> changes below.
> Also if we do this refactoring then we might as well do it for rt5651 
> which is similar and only relies on I2S. other machine drivers enable 
> TDM mode when possible.
> And last this change has a lot of impact on how we deal with UCM files. 
> The name of the card should reflect which codec is used, and the quirks 
> be added to the long name. If you lump everything with a single name 
> then you will make it really hard for userspace to figure out which 
> controls need to be set.
> 
> So nice idea but too early to merge. NAK.
Thank you for the review, will address these comments in the next version. When
you it be appropriate to re-submit? Are we waiting for any patches which are
queued to be merged soon?
> 
> > 
> > Signed-off-by: Shrirang Bagul <shrirang.bagul@canonical.com>
> > ---
> >  sound/soc/intel/Kconfig               |  11 +--
> >  sound/soc/intel/atom/sst/sst_acpi.c   |   2 +
> >  sound/soc/intel/boards/bytcr_rt5640.c | 156 ++++++++++++++++++++++++++++++-
> > ---
> >  3 files changed, 147 insertions(+), 22 deletions(-)
> > 
> > diff --git a/sound/soc/intel/Kconfig b/sound/soc/intel/Kconfig
> > index fd5d1e0..0b43b6a 100644
> > --- a/sound/soc/intel/Kconfig
> > +++ b/sound/soc/intel/Kconfig
> > @@ -147,17 +147,18 @@ config SND_SOC_INTEL_BROADWELL_MACH
> >  	  If unsure select "N".
> > 
> >  config SND_SOC_INTEL_BYTCR_RT5640_MACH
> > -        tristate "ASoC Audio driver for Intel Baytrail and Baytrail-CR with
> > RT5640 codec"
> > +	tristate "ASoC Audio driver for Intel Baytrail and Baytrail-CR with
> > RT5640/5660 codec"
> >  	depends on X86 && I2C && ACPI
> >  	select SND_SOC_RT5640
> > +	select SND_SOC_RT5660
> >  	select SND_SST_MFLD_PLATFORM
> >  	select SND_SST_IPC_ACPI
> >  	select SND_SOC_INTEL_SST_MATCH if ACPI
> >  	help
> > -          This adds support for ASoC machine driver for Intel(R) Baytrail
> > and Baytrail-CR
> > -          platforms with RT5640 audio codec.
> > -          Say Y if you have such a device.
> > -          If unsure select "N".
> > +	  This adds support for ASoC machine driver for Intel(R) Baytrail
> > and Baytrail-CR
> > +	  platforms with RT5640, RT5460 audio codec.
> > +	  Say Y if you have such a device.
> > +	  If unsure select "N".
> > 
> >  config SND_SOC_INTEL_BYTCR_RT5651_MACH
> >          tristate "ASoC Audio driver for Intel Baytrail and Baytrail-CR with
> > RT5651 codec"
> > diff --git a/sound/soc/intel/atom/sst/sst_acpi.c
> > b/sound/soc/intel/atom/sst/sst_acpi.c
> > index f4d92bb..d401457f 100644
> > --- a/sound/soc/intel/atom/sst/sst_acpi.c
> > +++ b/sound/soc/intel/atom/sst/sst_acpi.c
> > @@ -441,6 +441,8 @@ static struct sst_acpi_mach sst_acpi_bytcr[] = {
> >  						&byt_rvp_platform_data },
> >  	{"10EC5642", "bytcr_rt5640", "intel/fw_sst_0f28.bin",
> > "bytcr_rt5640", NULL,
> >  						&byt_rvp_platform_data },
> > +	{"10EC3277", "bytcr_rt5640", "intel/fw_sst_0f28.bin",
> > "bytcr_rt5640", NULL,
> > +						&byt_rvp_platform_data },
> 
> so right there you add an HID in the platform driver and you need the 
> same in the platform driver to determine which codec type this is...
> 
> >  	{"INTCCFFD", "bytcr_rt5640", "intel/fw_sst_0f28.bin",
> > "bytcr_rt5640", NULL,
> >  						&byt_rvp_platform_data },
> >  	{"10EC5651", "bytcr_rt5651", "intel/fw_sst_0f28.bin",
> > "bytcr_rt5651", NULL,
> > diff --git a/sound/soc/intel/boards/bytcr_rt5640.c
> > b/sound/soc/intel/boards/bytcr_rt5640.c
> > index f6fd397..e8c9a01 100644
> > --- a/sound/soc/intel/boards/bytcr_rt5640.c
> > +++ b/sound/soc/intel/boards/bytcr_rt5640.c
> > @@ -32,11 +32,17 @@
> >  #include <sound/soc.h>
> >  #include <sound/jack.h>
> >  #include "../../codecs/rt5640.h"
> > +#include "../../codecs/rt5660.h"
> >  #include "../atom/sst-atom-controls.h"
> >  #include "../common/sst-acpi.h"
> >  #include "../common/sst-dsp.h"
> > 
> >  enum {
> > +	CODEC_TYPE_RT5640,
> > +	CODEC_TYPE_RT5660,
> > +};
> > +
> > +enum {
> >  	BYT_RT5640_DMIC1_MAP,
> >  	BYT_RT5640_DMIC2_MAP,
> >  	BYT_RT5640_IN1_MAP,
> > @@ -60,8 +66,16 @@ enum {
> >  	PLL1_MCLK,
> >  };
> > 
> > +struct byt_acpi_card {
> > +	char *codec_id;
> > +	int codec_type;
> > +	struct snd_soc_card *soc_card;
> > +};
> > +
> >  struct byt_rt5640_private {
> > +	struct byt_acpi_card *acpi_card;
> >  	struct clk *mclk;
> > +	char codec_name[16];
> >  	int *clks;
> >  };
> > 
> > @@ -72,6 +86,13 @@ static int byt_rt5640_clks[] = {
> >  	RT5640_PLL1_S_MCLK
> >  };
> > 
> > +static int byt_rt5660_clks[] = {
> > +	RT5660_SCLK_S_PLL1,
> > +	RT5660_SCLK_S_RCCLK,
> > +	RT5660_PLL1_S_BCLK,
> > +	RT5660_PLL1_S_MCLK
> > +};
> > +
> >  static unsigned long byt_rt5640_quirk = BYT_RT5640_MCLK_EN;
> 
> the quirks would need to be isolated and made dependent on codec type
Will fix in next version of the patch.
> 
> > 
> >  static void log_quirks(struct device *dev)
> > @@ -105,6 +126,7 @@ static void log_quirks(struct device *dev)
> > 
> >  #define BYT_CODEC_DAI1	"rt5640-aif1"
> >  #define BYT_CODEC_DAI2	"rt5640-aif2"
> > +#define BYT_CODEC_DAI3	"rt5660-aif1"
> > 
> >  static inline struct snd_soc_dai *byt_get_codec_dai(struct snd_soc_card
> > *card)
> >  {
> > @@ -117,6 +139,9 @@ static inline struct snd_soc_dai
> > *byt_get_codec_dai(struct snd_soc_card *card)
> >  		if (!strncmp(rtd->codec_dai->name, BYT_CODEC_DAI2,
> >  				strlen(BYT_CODEC_DAI2)))
> >  			return rtd->codec_dai;
> > +		if (!strncmp(rtd->codec_dai->name, BYT_CODEC_DAI3,
> > +				strlen(BYT_CODEC_DAI3)))
> > +			return rtd->codec_dai;
> 
> not very good to go look for a DAI that doesn't exist for a specific 
> codec. this would need to be dependent on codec type.
Understood, will fix this in next version.
> 
> > 
> >  	}
> >  	return NULL;
> > @@ -269,6 +294,29 @@ static const struct snd_kcontrol_new
> > byt_rt5640_controls[] = {
> >  	SOC_DAPM_PIN_SWITCH("Speaker"),
> >  };
> > 
> > +static const struct snd_soc_dapm_widget byt_rt5660_widgets[] = {
> > +	SND_SOC_DAPM_MIC("Line In", NULL),
> > +	SND_SOC_DAPM_LINE("Line Out", NULL),
> > +	SND_SOC_DAPM_SUPPLY("Platform Clock", SND_SOC_NOPM, 0, 0,
> > +			platform_clock_control, SND_SOC_DAPM_PRE_PMU |
> > +			SND_SOC_DAPM_POST_PMD),
> > +};
> > +
> > +static const struct snd_soc_dapm_route byt_rt5660_audio_map[] = {
> > +	{"IN1P", NULL, "Line In"},
> > +	{"IN2P", NULL, "Line In"},
> > +	{"Line Out", NULL, "LOUTR"},
> > +	{"Line Out", NULL, "LOUTL"},
> > +
> > +	{"Line In", NULL, "Platform Clock"},
> > +	{"Line Out", NULL, "Platform Clock"},
> > +};
> > +
> > +static const struct snd_kcontrol_new byt_rt5660_controls[] = {
> > +	SOC_DAPM_PIN_SWITCH("Line In"),
> > +	SOC_DAPM_PIN_SWITCH("Line Out"),
> > +};
> > +
> >  static int byt_rt5640_aif1_hw_params(struct snd_pcm_substream *substream,
> >  					struct snd_pcm_hw_params *params)
> >  {
> > @@ -422,11 +470,8 @@ static int byt_rt5640_init(struct snd_soc_pcm_runtime
> > *runtime)
> >  	struct snd_soc_codec *codec = runtime->codec;
> >  	struct snd_soc_card *card = runtime->card;
> >  	const struct snd_soc_dapm_route *custom_map;
> > -	struct byt_rt5640_private *priv = snd_soc_card_get_drvdata(card);
> >  	int num_routes;
> > 
> > -	card->dapm.idle_bias_off = true;
> > -
> >  	rt5640_sel_asrc_clk_src(codec,
> >  				RT5640_DA_STEREO_FILTER |
> >  				RT5640_DA_MONO_L_FILTER	|
> > @@ -511,6 +556,39 @@ static int byt_rt5640_init(struct snd_soc_pcm_runtime
> > *runtime)
> >  	snd_soc_dapm_ignore_suspend(&card->dapm, "Headphone");
> >  	snd_soc_dapm_ignore_suspend(&card->dapm, "Speaker");
> > 
> > +	return ret;
> > +}
> > +
> > +static int byt_rt5660_init(struct snd_soc_pcm_runtime *runtime)
> > +{
> > +	int ret;
> > +	struct snd_soc_card *card = runtime->card;
> > +
> > +	ret = snd_soc_dapm_add_routes(&card->dapm,
> > +			byt_rt5640_ssp2_aif1_map,
> > +			ARRAY_SIZE(byt_rt5640_ssp2_aif1_map));
> > +	if (ret)
> > +		return ret;
> > +
> > +	snd_soc_dapm_enable_pin(&card->dapm, "Line In");
> > +	snd_soc_dapm_enable_pin(&card->dapm, "Line Out");
> > +
> > +	return ret;
> > +}
> > +
> > +static int byt_rt56x0_init(struct snd_soc_pcm_runtime *runtime)
> > +{
> > +	int ret;
> > +	struct snd_soc_card *card = runtime->card;
> > +	struct byt_rt5640_private *priv = snd_soc_card_get_drvdata(card);
> > +
> > +	card->dapm.idle_bias_off = true;
> > +
> > +	if (priv->acpi_card->codec_type == CODEC_TYPE_RT5640)
> > +		ret = byt_rt5640_init(runtime);
> > +	else
> > +		ret = byt_rt5660_init(runtime);
> > +
> >  	if ((byt_rt5640_quirk & BYT_RT5640_MCLK_EN) && priv->mclk) {
> >  		/*
> >  		 * The firmware might enable the clock at
> > @@ -679,7 +757,7 @@ static struct snd_soc_dai_link byt_rt5640_dais[] = {
> >  		.ignore_suspend = 1,
> >  		.dpcm_playback = 1,
> >  		.dpcm_capture = 1,
> > -		.init = byt_rt5640_init,
> > +		.init = byt_rt56x0_init,
> >  		.ops = &byt_rt5640_be_ssp2_ops,
> >  	},
> >  };
> > @@ -697,6 +775,25 @@ static struct snd_soc_card byt_rt5640_card = {
> >  	.fully_routed = true,
> >  };
> > 
> > +static struct snd_soc_card byt_rt5660_card = {
> > +	.name = "bytcr-rt5660",
> > +	.owner = THIS_MODULE,
> > +	.dai_link = byt_rt5640_dais,
> > +	.num_links = ARRAY_SIZE(byt_rt5640_dais),
> > +	.controls = byt_rt5660_controls,
> > +	.num_controls = ARRAY_SIZE(byt_rt5660_controls),
> > +	.dapm_widgets = byt_rt5660_widgets,
> > +	.num_dapm_widgets = ARRAY_SIZE(byt_rt5660_widgets),
> > +	.dapm_routes = byt_rt5660_audio_map,
> > +	.num_dapm_routes = ARRAY_SIZE(byt_rt5660_audio_map),
> > +	.fully_routed = true,
> > +};
> > +
> > +static struct byt_acpi_card snd_soc_cards[] = {
> > +	{"10EC5640", CODEC_TYPE_RT5640, &byt_rt5640_card},
> > +	{"10EC3277", CODEC_TYPE_RT5660, &byt_rt5660_card},
> > +};
> 
> I know this is what's used for rt5645/50 but I don't like it and don't 
> think it should be the baseline for how we deal with codecs. This forces 
> the addition of additional quirks.
Is there a better baseline to start from or none exists and we ought to come-up
with one?
> 
> > +
> >  static char byt_rt5640_codec_name[16]; /* i2c-<HID>:00 with HID being 8
> > chars */
> >  static char byt_rt5640_codec_aif_name[12]; /*  = "rt5640-aif[1|2]" */
> >  static char byt_rt5640_cpu_dai_name[10]; /*  = "ssp[0|2]-port" */
> > @@ -721,41 +818,51 @@ struct acpi_chan_package {   /* ACPICA seems to
> > require 64 bit integers */
> >  static int snd_byt_rt5640_mc_probe(struct platform_device *pdev)
> >  {
> >  	int ret_val = 0;
> > -	struct sst_acpi_mach *mach;
> > -	const char *i2c_name = NULL;
> >  	int i;
> > -	int dai_index;
> >  	struct byt_rt5640_private *priv;
> > +	struct snd_soc_card *card = snd_soc_cards[0].soc_card;
> > +	struct sst_acpi_mach *mach;
> > +	const char *i2c_name = NULL;
> > +	int dai_index = 0;
> >  	bool is_bytcr = false;
> > 
> >  	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_ATOMIC);
> >  	if (!priv)
> >  		return -ENOMEM;
> > 
> > +	for (i = 0; i < ARRAY_SIZE(snd_soc_cards); i++) {
> > +		if (acpi_dev_found(snd_soc_cards[i].codec_id)) {
> > +			dev_dbg(&pdev->dev,
> > +				"found codec %s\n",
> > snd_soc_cards[i].codec_id);
> > +			card = snd_soc_cards[i].soc_card;
> > +			priv->acpi_card = &snd_soc_cards[i];
> > +			break;
> > +		}
> > +	}
> > +
> >  	/* register the soc card */
> >  	priv->clks = byt_rt5640_clks;
> > -	byt_rt5640_card.dev = &pdev->dev;
> > -	mach = byt_rt5640_card.dev->platform_data;
> > -	snd_soc_card_set_drvdata(&byt_rt5640_card, priv);
> > +	card->dev = &pdev->dev;
> > +	mach = card->dev->platform_data;
> > +	sprintf(priv->codec_name, "i2c-%s:00", priv->acpi_card->codec_id);
> > 
> >  	/* fix index of codec dai */
> > -	dai_index = MERR_DPCM_COMPR + 1;
> > -	for (i = 0; i < ARRAY_SIZE(byt_rt5640_dais); i++) {
> > +	for (i = 0; i < ARRAY_SIZE(byt_rt5640_dais); i++)
> >  		if (!strcmp(byt_rt5640_dais[i].codec_name, "i2c-
> > 10EC5640:00")) {
> > +			card->dai_link[i].codec_name = priv->codec_name;
> >  			dai_index = i;
> > -			break;
> >  		}
> > -	}
> > 
> >  	/* fixup codec name based on HID */
> >  	i2c_name = sst_acpi_find_name_from_hid(mach->id);
> >  	if (i2c_name != NULL) {
> >  		snprintf(byt_rt5640_codec_name,
> > sizeof(byt_rt5640_codec_name),
> >  			"%s%s", "i2c-", i2c_name);
> > -
> >  		byt_rt5640_dais[dai_index].codec_name =
> > byt_rt5640_codec_name;
> >  	}
> > 
> > +	snd_soc_card_set_drvdata(card, priv);
> > +
> >  	/*
> >  	 * swap SSP0 if bytcr is detected
> >  	 * (will be overridden if DMI quirk is detected)
> > @@ -821,6 +928,21 @@ static int snd_byt_rt5640_mc_probe(struct
> > platform_device *pdev)
> >  				BYT_RT5640_DMIC_EN);
> >  	}
> > 
> > +	if (priv->acpi_card->codec_type == CODEC_TYPE_RT5660) {
> > +		priv->clks = byt_rt5660_clks;
> > +
> > +		/* fixup codec aif name for rt5660 */
> > +		snprintf(byt_rt5640_codec_aif_name,
> > +			sizeof(byt_rt5640_codec_aif_name),
> > +			"%s", "rt5660-aif1");
> > +
> > +		byt_rt5640_dais[dai_index].codec_dai_name =
> > +			byt_rt5640_codec_aif_name;
> > +
> > +		/* setup codec quirks for rt5660 */
> > +		byt_rt5640_quirk = BYT_RT5640_MCLK_EN;
> > +	}
> > +
> >  	/* check quirks before creating card */
> >  	dmi_check_system(byt_rt5640_quirk_table);
> 
> the quirks have to be separate.
> 
> >  	log_quirks(&pdev->dev);
> > @@ -869,14 +991,14 @@ static int snd_byt_rt5640_mc_probe(struct
> > platform_device *pdev)
> >  		}
> >  	}
> > 
> > -	ret_val = devm_snd_soc_register_card(&pdev->dev, &byt_rt5640_card);
> > +	ret_val = devm_snd_soc_register_card(&pdev->dev, card);
> > 
> >  	if (ret_val) {
> >  		dev_err(&pdev->dev, "devm_snd_soc_register_card failed
> > %d\n",
> >  			ret_val);
> >  		return ret_val;
> >  	}
> > -	platform_set_drvdata(pdev, &byt_rt5640_card);
> > +	platform_set_drvdata(pdev, card);
> >  	return ret_val;
> >  }
> > 
> > 
> 
> 

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

* Re: [alsa-devel] [PATCH v2 3/4] ASoC: Intel: Support rt5660 codec for Baytrail
  2017-01-16  7:45     ` Shrirang Bagul
@ 2017-01-16 15:10       ` Pierre-Louis Bossart
  0 siblings, 0 replies; 10+ messages in thread
From: Pierre-Louis Bossart @ 2017-01-16 15:10 UTC (permalink / raw)
  To: Shrirang Bagul, alsa-devel
  Cc: Jeeja KP, Arnd Bergmann, Irina Tirdea, Vinod Koul, linux-kernel,
	Takashi Iwai, Ramesh Babu, Liam Girdwood, Mark Brown,
	John Keeping, Sathyanarayana Nujella, Colin Ian King,
	Wei Yongjun

On 1/16/17 1:45 AM, Shrirang Bagul wrote:
> On Thu, 2017-01-12 at 08:40 -0600, Pierre-Louis Bossart wrote:
>> On 1/12/17 6:01 AM, Shrirang Bagul wrote:
>>> rt5660 and rt5640 are similar codecs so reuse the bytcr_rt5640 driver.
>>> RT5660 codec is used on Dell Edge IoT Gateways with ACPI ID 10EC3277.
>>> These devices sport only Line-In and Line-Out jacks.
>>
>> While it would be nice to avoid copy/pasting everytime we add a new
>> codec and refactor the code, I am not comfortable with a series of
>> changes below.
>> Also if we do this refactoring then we might as well do it for rt5651
>> which is similar and only relies on I2S. other machine drivers enable
>> TDM mode when possible.
>> And last this change has a lot of impact on how we deal with UCM files.
>> The name of the card should reflect which codec is used, and the quirks
>> be added to the long name. If you lump everything with a single name
>> then you will make it really hard for userspace to figure out which
>> controls need to be set.
>>
>> So nice idea but too early to merge. NAK.
> Thank you for the review, will address these comments in the next version. When
> you it be appropriate to re-submit? Are we waiting for any patches which are
> queued to be merged soon?

I have a set of 5640/5651 corrections that will be sent out very soon 
(fixes for existing platforms).

I am ok with factorizing some of the common code, however the quirk 
management can't really be fused and the platform names need to remain 
separate. The dailinks and dapm routes need to remain separate, same for 
the platform clock control. In the end, I am not sure if it's better to 
have a single file with all the codecs, or if we should keep separate 
files and use a common set of helper functions. The latter might be 
wiser if we want to add another codec, it'd avoid complicated if/else 
cases in the middle of the common code.

>>
>>>
>>> Signed-off-by: Shrirang Bagul <shrirang.bagul@canonical.com>
>>> ---
>>>  sound/soc/intel/Kconfig               |  11 +--
>>>  sound/soc/intel/atom/sst/sst_acpi.c   |   2 +
>>>  sound/soc/intel/boards/bytcr_rt5640.c | 156 ++++++++++++++++++++++++++++++-
>>> ---
>>>  3 files changed, 147 insertions(+), 22 deletions(-)
>>>
>>> diff --git a/sound/soc/intel/Kconfig b/sound/soc/intel/Kconfig
>>> index fd5d1e0..0b43b6a 100644
>>> --- a/sound/soc/intel/Kconfig
>>> +++ b/sound/soc/intel/Kconfig
>>> @@ -147,17 +147,18 @@ config SND_SOC_INTEL_BROADWELL_MACH
>>>  	  If unsure select "N".
>>>
>>>  config SND_SOC_INTEL_BYTCR_RT5640_MACH
>>> -        tristate "ASoC Audio driver for Intel Baytrail and Baytrail-CR with
>>> RT5640 codec"
>>> +	tristate "ASoC Audio driver for Intel Baytrail and Baytrail-CR with
>>> RT5640/5660 codec"
>>>  	depends on X86 && I2C && ACPI
>>>  	select SND_SOC_RT5640
>>> +	select SND_SOC_RT5660
>>>  	select SND_SST_MFLD_PLATFORM
>>>  	select SND_SST_IPC_ACPI
>>>  	select SND_SOC_INTEL_SST_MATCH if ACPI
>>>  	help
>>> -          This adds support for ASoC machine driver for Intel(R) Baytrail
>>> and Baytrail-CR
>>> -          platforms with RT5640 audio codec.
>>> -          Say Y if you have such a device.
>>> -          If unsure select "N".
>>> +	  This adds support for ASoC machine driver for Intel(R) Baytrail
>>> and Baytrail-CR
>>> +	  platforms with RT5640, RT5460 audio codec.
>>> +	  Say Y if you have such a device.
>>> +	  If unsure select "N".
>>>
>>>  config SND_SOC_INTEL_BYTCR_RT5651_MACH
>>>          tristate "ASoC Audio driver for Intel Baytrail and Baytrail-CR with
>>> RT5651 codec"
>>> diff --git a/sound/soc/intel/atom/sst/sst_acpi.c
>>> b/sound/soc/intel/atom/sst/sst_acpi.c
>>> index f4d92bb..d401457f 100644
>>> --- a/sound/soc/intel/atom/sst/sst_acpi.c
>>> +++ b/sound/soc/intel/atom/sst/sst_acpi.c
>>> @@ -441,6 +441,8 @@ static struct sst_acpi_mach sst_acpi_bytcr[] = {
>>>  						&byt_rvp_platform_data },
>>>  	{"10EC5642", "bytcr_rt5640", "intel/fw_sst_0f28.bin",
>>> "bytcr_rt5640", NULL,
>>>  						&byt_rvp_platform_data },
>>> +	{"10EC3277", "bytcr_rt5640", "intel/fw_sst_0f28.bin",
>>> "bytcr_rt5640", NULL,
>>> +						&byt_rvp_platform_data },
>>
>> so right there you add an HID in the platform driver and you need the
>> same in the platform driver to determine which codec type this is...
>>
>>>  	{"INTCCFFD", "bytcr_rt5640", "intel/fw_sst_0f28.bin",
>>> "bytcr_rt5640", NULL,
>>>  						&byt_rvp_platform_data },
>>>  	{"10EC5651", "bytcr_rt5651", "intel/fw_sst_0f28.bin",
>>> "bytcr_rt5651", NULL,
>>> diff --git a/sound/soc/intel/boards/bytcr_rt5640.c
>>> b/sound/soc/intel/boards/bytcr_rt5640.c
>>> index f6fd397..e8c9a01 100644
>>> --- a/sound/soc/intel/boards/bytcr_rt5640.c
>>> +++ b/sound/soc/intel/boards/bytcr_rt5640.c
>>> @@ -32,11 +32,17 @@
>>>  #include <sound/soc.h>
>>>  #include <sound/jack.h>
>>>  #include "../../codecs/rt5640.h"
>>> +#include "../../codecs/rt5660.h"
>>>  #include "../atom/sst-atom-controls.h"
>>>  #include "../common/sst-acpi.h"
>>>  #include "../common/sst-dsp.h"
>>>
>>>  enum {
>>> +	CODEC_TYPE_RT5640,
>>> +	CODEC_TYPE_RT5660,
>>> +};
>>> +
>>> +enum {
>>>  	BYT_RT5640_DMIC1_MAP,
>>>  	BYT_RT5640_DMIC2_MAP,
>>>  	BYT_RT5640_IN1_MAP,
>>> @@ -60,8 +66,16 @@ enum {
>>>  	PLL1_MCLK,
>>>  };
>>>
>>> +struct byt_acpi_card {
>>> +	char *codec_id;
>>> +	int codec_type;
>>> +	struct snd_soc_card *soc_card;
>>> +};
>>> +
>>>  struct byt_rt5640_private {
>>> +	struct byt_acpi_card *acpi_card;
>>>  	struct clk *mclk;
>>> +	char codec_name[16];
>>>  	int *clks;
>>>  };
>>>
>>> @@ -72,6 +86,13 @@ static int byt_rt5640_clks[] = {
>>>  	RT5640_PLL1_S_MCLK
>>>  };
>>>
>>> +static int byt_rt5660_clks[] = {
>>> +	RT5660_SCLK_S_PLL1,
>>> +	RT5660_SCLK_S_RCCLK,
>>> +	RT5660_PLL1_S_BCLK,
>>> +	RT5660_PLL1_S_MCLK
>>> +};
>>> +
>>>  static unsigned long byt_rt5640_quirk = BYT_RT5640_MCLK_EN;
>>
>> the quirks would need to be isolated and made dependent on codec type
> Will fix in next version of the patch.
>>
>>>
>>>  static void log_quirks(struct device *dev)
>>> @@ -105,6 +126,7 @@ static void log_quirks(struct device *dev)
>>>
>>>  #define BYT_CODEC_DAI1	"rt5640-aif1"
>>>  #define BYT_CODEC_DAI2	"rt5640-aif2"
>>> +#define BYT_CODEC_DAI3	"rt5660-aif1"
>>>
>>>  static inline struct snd_soc_dai *byt_get_codec_dai(struct snd_soc_card
>>> *card)
>>>  {
>>> @@ -117,6 +139,9 @@ static inline struct snd_soc_dai
>>> *byt_get_codec_dai(struct snd_soc_card *card)
>>>  		if (!strncmp(rtd->codec_dai->name, BYT_CODEC_DAI2,
>>>  				strlen(BYT_CODEC_DAI2)))
>>>  			return rtd->codec_dai;
>>> +		if (!strncmp(rtd->codec_dai->name, BYT_CODEC_DAI3,
>>> +				strlen(BYT_CODEC_DAI3)))
>>> +			return rtd->codec_dai;
>>
>> not very good to go look for a DAI that doesn't exist for a specific
>> codec. this would need to be dependent on codec type.
> Understood, will fix this in next version.
>>
>>>
>>>  	}
>>>  	return NULL;
>>> @@ -269,6 +294,29 @@ static const struct snd_kcontrol_new
>>> byt_rt5640_controls[] = {
>>>  	SOC_DAPM_PIN_SWITCH("Speaker"),
>>>  };
>>>
>>> +static const struct snd_soc_dapm_widget byt_rt5660_widgets[] = {
>>> +	SND_SOC_DAPM_MIC("Line In", NULL),
>>> +	SND_SOC_DAPM_LINE("Line Out", NULL),
>>> +	SND_SOC_DAPM_SUPPLY("Platform Clock", SND_SOC_NOPM, 0, 0,
>>> +			platform_clock_control, SND_SOC_DAPM_PRE_PMU |
>>> +			SND_SOC_DAPM_POST_PMD),
>>> +};
>>> +
>>> +static const struct snd_soc_dapm_route byt_rt5660_audio_map[] = {
>>> +	{"IN1P", NULL, "Line In"},
>>> +	{"IN2P", NULL, "Line In"},
>>> +	{"Line Out", NULL, "LOUTR"},
>>> +	{"Line Out", NULL, "LOUTL"},
>>> +
>>> +	{"Line In", NULL, "Platform Clock"},
>>> +	{"Line Out", NULL, "Platform Clock"},
>>> +};
>>> +
>>> +static const struct snd_kcontrol_new byt_rt5660_controls[] = {
>>> +	SOC_DAPM_PIN_SWITCH("Line In"),
>>> +	SOC_DAPM_PIN_SWITCH("Line Out"),
>>> +};
>>> +
>>>  static int byt_rt5640_aif1_hw_params(struct snd_pcm_substream *substream,
>>>  					struct snd_pcm_hw_params *params)
>>>  {
>>> @@ -422,11 +470,8 @@ static int byt_rt5640_init(struct snd_soc_pcm_runtime
>>> *runtime)
>>>  	struct snd_soc_codec *codec = runtime->codec;
>>>  	struct snd_soc_card *card = runtime->card;
>>>  	const struct snd_soc_dapm_route *custom_map;
>>> -	struct byt_rt5640_private *priv = snd_soc_card_get_drvdata(card);
>>>  	int num_routes;
>>>
>>> -	card->dapm.idle_bias_off = true;
>>> -
>>>  	rt5640_sel_asrc_clk_src(codec,
>>>  				RT5640_DA_STEREO_FILTER |
>>>  				RT5640_DA_MONO_L_FILTER	|
>>> @@ -511,6 +556,39 @@ static int byt_rt5640_init(struct snd_soc_pcm_runtime
>>> *runtime)
>>>  	snd_soc_dapm_ignore_suspend(&card->dapm, "Headphone");
>>>  	snd_soc_dapm_ignore_suspend(&card->dapm, "Speaker");
>>>
>>> +	return ret;
>>> +}
>>> +
>>> +static int byt_rt5660_init(struct snd_soc_pcm_runtime *runtime)
>>> +{
>>> +	int ret;
>>> +	struct snd_soc_card *card = runtime->card;
>>> +
>>> +	ret = snd_soc_dapm_add_routes(&card->dapm,
>>> +			byt_rt5640_ssp2_aif1_map,
>>> +			ARRAY_SIZE(byt_rt5640_ssp2_aif1_map));
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	snd_soc_dapm_enable_pin(&card->dapm, "Line In");
>>> +	snd_soc_dapm_enable_pin(&card->dapm, "Line Out");
>>> +
>>> +	return ret;
>>> +}
>>> +
>>> +static int byt_rt56x0_init(struct snd_soc_pcm_runtime *runtime)
>>> +{
>>> +	int ret;
>>> +	struct snd_soc_card *card = runtime->card;
>>> +	struct byt_rt5640_private *priv = snd_soc_card_get_drvdata(card);
>>> +
>>> +	card->dapm.idle_bias_off = true;
>>> +
>>> +	if (priv->acpi_card->codec_type == CODEC_TYPE_RT5640)
>>> +		ret = byt_rt5640_init(runtime);
>>> +	else
>>> +		ret = byt_rt5660_init(runtime);
>>> +
>>>  	if ((byt_rt5640_quirk & BYT_RT5640_MCLK_EN) && priv->mclk) {
>>>  		/*
>>>  		 * The firmware might enable the clock at
>>> @@ -679,7 +757,7 @@ static struct snd_soc_dai_link byt_rt5640_dais[] = {
>>>  		.ignore_suspend = 1,
>>>  		.dpcm_playback = 1,
>>>  		.dpcm_capture = 1,
>>> -		.init = byt_rt5640_init,
>>> +		.init = byt_rt56x0_init,
>>>  		.ops = &byt_rt5640_be_ssp2_ops,
>>>  	},
>>>  };
>>> @@ -697,6 +775,25 @@ static struct snd_soc_card byt_rt5640_card = {
>>>  	.fully_routed = true,
>>>  };
>>>
>>> +static struct snd_soc_card byt_rt5660_card = {
>>> +	.name = "bytcr-rt5660",
>>> +	.owner = THIS_MODULE,
>>> +	.dai_link = byt_rt5640_dais,
>>> +	.num_links = ARRAY_SIZE(byt_rt5640_dais),
>>> +	.controls = byt_rt5660_controls,
>>> +	.num_controls = ARRAY_SIZE(byt_rt5660_controls),
>>> +	.dapm_widgets = byt_rt5660_widgets,
>>> +	.num_dapm_widgets = ARRAY_SIZE(byt_rt5660_widgets),
>>> +	.dapm_routes = byt_rt5660_audio_map,
>>> +	.num_dapm_routes = ARRAY_SIZE(byt_rt5660_audio_map),
>>> +	.fully_routed = true,
>>> +};
>>> +
>>> +static struct byt_acpi_card snd_soc_cards[] = {
>>> +	{"10EC5640", CODEC_TYPE_RT5640, &byt_rt5640_card},
>>> +	{"10EC3277", CODEC_TYPE_RT5660, &byt_rt5660_card},
>>> +};
>>
>> I know this is what's used for rt5645/50 but I don't like it and don't
>> think it should be the baseline for how we deal with codecs. This forces
>> the addition of additional quirks.
> Is there a better baseline to start from or none exists and we ought to come-up
> with one?
>>
>>> +
>>>  static char byt_rt5640_codec_name[16]; /* i2c-<HID>:00 with HID being 8
>>> chars */
>>>  static char byt_rt5640_codec_aif_name[12]; /*  = "rt5640-aif[1|2]" */
>>>  static char byt_rt5640_cpu_dai_name[10]; /*  = "ssp[0|2]-port" */
>>> @@ -721,41 +818,51 @@ struct acpi_chan_package {   /* ACPICA seems to
>>> require 64 bit integers */
>>>  static int snd_byt_rt5640_mc_probe(struct platform_device *pdev)
>>>  {
>>>  	int ret_val = 0;
>>> -	struct sst_acpi_mach *mach;
>>> -	const char *i2c_name = NULL;
>>>  	int i;
>>> -	int dai_index;
>>>  	struct byt_rt5640_private *priv;
>>> +	struct snd_soc_card *card = snd_soc_cards[0].soc_card;
>>> +	struct sst_acpi_mach *mach;
>>> +	const char *i2c_name = NULL;
>>> +	int dai_index = 0;
>>>  	bool is_bytcr = false;
>>>
>>>  	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_ATOMIC);
>>>  	if (!priv)
>>>  		return -ENOMEM;
>>>
>>> +	for (i = 0; i < ARRAY_SIZE(snd_soc_cards); i++) {
>>> +		if (acpi_dev_found(snd_soc_cards[i].codec_id)) {
>>> +			dev_dbg(&pdev->dev,
>>> +				"found codec %s\n",
>>> snd_soc_cards[i].codec_id);
>>> +			card = snd_soc_cards[i].soc_card;
>>> +			priv->acpi_card = &snd_soc_cards[i];
>>> +			break;
>>> +		}
>>> +	}
>>> +
>>>  	/* register the soc card */
>>>  	priv->clks = byt_rt5640_clks;
>>> -	byt_rt5640_card.dev = &pdev->dev;
>>> -	mach = byt_rt5640_card.dev->platform_data;
>>> -	snd_soc_card_set_drvdata(&byt_rt5640_card, priv);
>>> +	card->dev = &pdev->dev;
>>> +	mach = card->dev->platform_data;
>>> +	sprintf(priv->codec_name, "i2c-%s:00", priv->acpi_card->codec_id);
>>>
>>>  	/* fix index of codec dai */
>>> -	dai_index = MERR_DPCM_COMPR + 1;
>>> -	for (i = 0; i < ARRAY_SIZE(byt_rt5640_dais); i++) {
>>> +	for (i = 0; i < ARRAY_SIZE(byt_rt5640_dais); i++)
>>>  		if (!strcmp(byt_rt5640_dais[i].codec_name, "i2c-
>>> 10EC5640:00")) {
>>> +			card->dai_link[i].codec_name = priv->codec_name;
>>>  			dai_index = i;
>>> -			break;
>>>  		}
>>> -	}
>>>
>>>  	/* fixup codec name based on HID */
>>>  	i2c_name = sst_acpi_find_name_from_hid(mach->id);
>>>  	if (i2c_name != NULL) {
>>>  		snprintf(byt_rt5640_codec_name,
>>> sizeof(byt_rt5640_codec_name),
>>>  			"%s%s", "i2c-", i2c_name);
>>> -
>>>  		byt_rt5640_dais[dai_index].codec_name =
>>> byt_rt5640_codec_name;
>>>  	}
>>>
>>> +	snd_soc_card_set_drvdata(card, priv);
>>> +
>>>  	/*
>>>  	 * swap SSP0 if bytcr is detected
>>>  	 * (will be overridden if DMI quirk is detected)
>>> @@ -821,6 +928,21 @@ static int snd_byt_rt5640_mc_probe(struct
>>> platform_device *pdev)
>>>  				BYT_RT5640_DMIC_EN);
>>>  	}
>>>
>>> +	if (priv->acpi_card->codec_type == CODEC_TYPE_RT5660) {
>>> +		priv->clks = byt_rt5660_clks;
>>> +
>>> +		/* fixup codec aif name for rt5660 */
>>> +		snprintf(byt_rt5640_codec_aif_name,
>>> +			sizeof(byt_rt5640_codec_aif_name),
>>> +			"%s", "rt5660-aif1");
>>> +
>>> +		byt_rt5640_dais[dai_index].codec_dai_name =
>>> +			byt_rt5640_codec_aif_name;
>>> +
>>> +		/* setup codec quirks for rt5660 */
>>> +		byt_rt5640_quirk = BYT_RT5640_MCLK_EN;
>>> +	}
>>> +
>>>  	/* check quirks before creating card */
>>>  	dmi_check_system(byt_rt5640_quirk_table);
>>
>> the quirks have to be separate.
>>
>>>  	log_quirks(&pdev->dev);
>>> @@ -869,14 +991,14 @@ static int snd_byt_rt5640_mc_probe(struct
>>> platform_device *pdev)
>>>  		}
>>>  	}
>>>
>>> -	ret_val = devm_snd_soc_register_card(&pdev->dev, &byt_rt5640_card);
>>> +	ret_val = devm_snd_soc_register_card(&pdev->dev, card);
>>>
>>>  	if (ret_val) {
>>>  		dev_err(&pdev->dev, "devm_snd_soc_register_card failed
>>> %d\n",
>>>  			ret_val);
>>>  		return ret_val;
>>>  	}
>>> -	platform_set_drvdata(pdev, &byt_rt5640_card);
>>> +	platform_set_drvdata(pdev, card);
>>>  	return ret_val;
>>>  }
>>>
>>>
>>
>>
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
>

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

end of thread, other threads:[~2017-01-16 15:10 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-01-12 12:00 [PATCH v2 0/4] ASoC: Dell IoT Gateway audio support Shrirang Bagul
2017-01-12 12:00 ` [PATCH v2 1/4] ASoC: rt5660: Add ACPI support Shrirang Bagul
2017-01-13  2:53   ` Bard Liao
2017-01-13 22:30     ` Andy Shevchenko
2017-01-12 12:00 ` [PATCH v2 2/4] ASoC: Intel: bytcr_rt5640: move codec clks to card driver data Shrirang Bagul
2017-01-12 12:01 ` [PATCH v2 3/4] ASoC: Intel: Support rt5660 codec for Baytrail Shrirang Bagul
2017-01-12 14:40   ` [alsa-devel] " Pierre-Louis Bossart
2017-01-16  7:45     ` Shrirang Bagul
2017-01-16 15:10       ` Pierre-Louis Bossart
2017-01-12 12:01 ` [PATCH v2 4/4] ASoC: Intel: bytcr_rt5640: Support line-out mute gpio Shrirang Bagul

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).