[PATCH RESEND 0/3] Intel-rt5670 updates

[PATCH RESEND 0/3] Intel-rt5670 updates

[Pierre-Louis Bossart]

[when I started cleaning-up some patches reviewed by Andy Shevchenko,
I realized this initial batch has not been merged, although both
Takashi and Vinod reviewed them, see
http://mailman.alsa-project.org/pipermail/alsa-devel/2017-September/125331.html
no change except for the addition of Takashi's Reviewed-by and Vinod's
Acked-by.  I don't mean to be pushy or anything, just checking if
something is missing ]

Instead of submitting all my patches at ones, I broke them by
platforms/codecs. This series is only for Intel-rt5670 platforms. It
fixes a mistake on the Dell Venue 5585 - corrected with the help from
Realtek and Dell who kindly provided schematics and data sheet
information needed to fix non-working DMIC capture. Since there are
still some reports of non-functional audio on some platforms, I added
quirks that can be changed from the kernel command line (and later
added as DMI-based quirks). The last two patches are minor corrections
to fix ACPI HID naming issues and use a more meaningful card name.


Pierre-Louis Bossart (3):
  ASoC: rt5670: refactor DMI quirks and fix Dell Venue settings
  ASoC: Intel: cht_bsw_rt5672: use actual HID in suspend/resume
  ASoC: Intel: cht_bsw_rt5672: fix card name

 sound/soc/codecs/rt5670.c               | 124 +++++++++++++++++++++++++++-----
 sound/soc/intel/boards/cht_bsw_rt5672.c |  10 ++-
 2 files changed, 112 insertions(+), 22 deletions(-)

-- 
2.11.0

[PATCH RESEND 1/3] ASoC: rt5670: refactor DMI quirks and fix Dell Venue settings

[Pierre-Louis Bossart]

Additional checks exposed a mistake in the quirk for the Dell Venue
Pro 5855 (Dmic2 instead of Dmic1). Rather than adding quirk tables,
merge all quirks in a single table and use flags to differentiate
platforms.  Also add a parameter override to help support additional
platforms using this codec

CC: Bard Liao <bardliao@realtek.com>
Reviewed-by: Takashi Iwai <tiwai@suse.de>
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 sound/soc/codecs/rt5670.c | 124 +++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 105 insertions(+), 19 deletions(-)

diff --git a/sound/soc/codecs/rt5670.c b/sound/soc/codecs/rt5670.c
index 9545764ef3eb..be945b345b43 100644
--- a/sound/soc/codecs/rt5670.c
+++ b/sound/soc/codecs/rt5670.c
@@ -34,6 +34,24 @@
 #include "rt5670.h"
 #include "rt5670-dsp.h"
 
+#define RT5670_DEV_GPIO     BIT(0)
+#define RT5670_IN2_DIFF     BIT(1)
+#define RT5670_DMIC_EN      BIT(2)
+#define RT5670_DMIC1_IN2P   BIT(3)
+#define RT5670_DMIC1_GPIO6  BIT(4)
+#define RT5670_DMIC1_GPIO7  BIT(5)
+#define RT5670_DMIC2_INR    BIT(6)
+#define RT5670_DMIC2_GPIO8  BIT(7)
+#define RT5670_DMIC3_GPIO5  BIT(8)
+#define RT5670_JD_MODE1     BIT(9)
+#define RT5670_JD_MODE2     BIT(10)
+#define RT5670_JD_MODE3     BIT(11)
+
+static unsigned long rt5670_quirk;
+static unsigned int quirk_override;
+module_param_named(quirk, quirk_override, uint, 0444);
+MODULE_PARM_DESC(quirk, "Board-specific quirk override");
+
 #define RT5670_DEVICE_ID 0x6271
 
 #define RT5670_PR_RANGE_BASE (0xff + 1)
@@ -2808,56 +2826,84 @@ static const struct acpi_device_id rt5670_acpi_match[] = {
 MODULE_DEVICE_TABLE(acpi, rt5670_acpi_match);
 #endif
 
-static const struct dmi_system_id dmi_platform_intel_braswell[] = {
+static int rt5670_quirk_cb(const struct dmi_system_id *id)
+{
+	rt5670_quirk = (unsigned long)id->driver_data;
+	return 1;
+}
+
+static const struct dmi_system_id dmi_platform_intel_quirks[] = {
 	{
+		.callback = rt5670_quirk_cb,
 		.ident = "Intel Braswell",
 		.matches = {
 			DMI_MATCH(DMI_SYS_VENDOR, "Intel Corporation"),
 			DMI_MATCH(DMI_BOARD_NAME, "Braswell CRB"),
 		},
+		.driver_data = (unsigned long *)(RT5670_DMIC_EN |
+						 RT5670_DMIC1_IN2P |
+						 RT5670_DEV_GPIO |
+						 RT5670_JD_MODE1),
 	},
 	{
+		.callback = rt5670_quirk_cb,
 		.ident = "Dell Wyse 3040",
 		.matches = {
 			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
 			DMI_MATCH(DMI_PRODUCT_NAME, "Wyse 3040"),
 		},
+		.driver_data = (unsigned long *)(RT5670_DMIC_EN |
+						 RT5670_DMIC1_IN2P |
+						 RT5670_DEV_GPIO |
+						 RT5670_JD_MODE1),
 	},
 	{
+		.callback = rt5670_quirk_cb,
 		.ident = "Lenovo Thinkpad Tablet 10",
 		.matches = {
 			DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
 			DMI_MATCH(DMI_PRODUCT_VERSION, "ThinkPad 10"),
 		},
+		.driver_data = (unsigned long *)(RT5670_DMIC_EN |
+						 RT5670_DMIC1_IN2P |
+						 RT5670_DEV_GPIO |
+						 RT5670_JD_MODE1),
 	},
 	{
+		.callback = rt5670_quirk_cb,
 		.ident = "Lenovo Thinkpad Tablet 10",
 		.matches = {
 			DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
 			DMI_MATCH(DMI_PRODUCT_VERSION, "ThinkPad Tablet B"),
 		},
+		.driver_data = (unsigned long *)(RT5670_DMIC_EN |
+						 RT5670_DMIC1_IN2P |
+						 RT5670_DEV_GPIO |
+						 RT5670_JD_MODE1),
 	},
-	{}
-};
-
-static const struct dmi_system_id dmi_platform_intel_bytcht_jdmode2[] = {
 	{
+		.callback = rt5670_quirk_cb,
 		.ident = "Lenovo Thinkpad Tablet 10",
 		.matches = {
 			DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
 			DMI_MATCH(DMI_PRODUCT_VERSION, "Lenovo Miix 2 10"),
 		},
+		.driver_data = (unsigned long *)(RT5670_DMIC_EN |
+						 RT5670_DMIC1_IN2P |
+						 RT5670_DEV_GPIO |
+						 RT5670_JD_MODE2),
 	},
-	{}
-};
-
-static const struct dmi_system_id dmi_platform_intel_bytcht_jdmode3[] = {
 	{
+		.callback = rt5670_quirk_cb,
 		.ident = "Dell Venue 8 Pro 5855",
 		.matches = {
 			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
 			DMI_MATCH(DMI_PRODUCT_NAME, "Venue 8 Pro 5855"),
 		},
+		.driver_data = (unsigned long *)(RT5670_DMIC_EN |
+						 RT5670_DMIC2_INR |
+						 RT5670_DEV_GPIO |
+						 RT5670_JD_MODE3),
 	},
 	{}
 };
@@ -2881,21 +2927,61 @@ static int rt5670_i2c_probe(struct i2c_client *i2c,
 	if (pdata)
 		rt5670->pdata = *pdata;
 
-	if (dmi_check_system(dmi_platform_intel_braswell)) {
-		rt5670->pdata.dmic_en = true;
-		rt5670->pdata.dmic1_data_pin = RT5670_DMIC_DATA_IN2P;
+	dmi_check_system(dmi_platform_intel_quirks);
+	if (quirk_override) {
+		dev_info(&i2c->dev, "Overriding quirk 0x%x => 0x%x\n",
+			 (unsigned int)rt5670_quirk, quirk_override);
+		rt5670_quirk = quirk_override;
+	}
+
+	if (rt5670_quirk & RT5670_DEV_GPIO) {
 		rt5670->pdata.dev_gpio = true;
-		rt5670->pdata.jd_mode = 1;
-	} else if (dmi_check_system(dmi_platform_intel_bytcht_jdmode2)) {
+		dev_info(&i2c->dev, "quirk dev_gpio\n");
+	}
+	if (rt5670_quirk & RT5670_IN2_DIFF) {
+		rt5670->pdata.in2_diff = true;
+		dev_info(&i2c->dev, "quirk IN2_DIFF\n");
+	}
+	if (rt5670_quirk & RT5670_DMIC_EN) {
 		rt5670->pdata.dmic_en = true;
+		dev_info(&i2c->dev, "quirk DMIC enabled\n");
+	}
+	if (rt5670_quirk & RT5670_DMIC1_IN2P) {
 		rt5670->pdata.dmic1_data_pin = RT5670_DMIC_DATA_IN2P;
-		rt5670->pdata.dev_gpio = true;
+		dev_info(&i2c->dev, "quirk DMIC1 on IN2P pin\n");
+	}
+	if (rt5670_quirk & RT5670_DMIC1_GPIO6) {
+		rt5670->pdata.dmic1_data_pin = RT5670_DMIC_DATA_GPIO6;
+		dev_info(&i2c->dev, "quirk DMIC1 on GPIO6 pin\n");
+	}
+	if (rt5670_quirk & RT5670_DMIC1_GPIO7) {
+		rt5670->pdata.dmic1_data_pin = RT5670_DMIC_DATA_GPIO7;
+		dev_info(&i2c->dev, "quirk DMIC1 on GPIO7 pin\n");
+	}
+	if (rt5670_quirk & RT5670_DMIC2_INR) {
+		rt5670->pdata.dmic2_data_pin = RT5670_DMIC_DATA_IN3N;
+		dev_info(&i2c->dev, "quirk DMIC2 on INR pin\n");
+	}
+	if (rt5670_quirk & RT5670_DMIC2_GPIO8) {
+		rt5670->pdata.dmic2_data_pin = RT5670_DMIC_DATA_GPIO8;
+		dev_info(&i2c->dev, "quirk DMIC2 on GPIO8 pin\n");
+	}
+	if (rt5670_quirk & RT5670_DMIC3_GPIO5) {
+		rt5670->pdata.dmic3_data_pin = RT5670_DMIC_DATA_GPIO5;
+		dev_info(&i2c->dev, "quirk DMIC3 on GPIO5 pin\n");
+	}
+
+	if (rt5670_quirk & RT5670_JD_MODE1) {
+		rt5670->pdata.jd_mode = 1;
+		dev_info(&i2c->dev, "quirk JD mode 1\n");
+	}
+	if (rt5670_quirk & RT5670_JD_MODE2) {
 		rt5670->pdata.jd_mode = 2;
-	} else if (dmi_check_system(dmi_platform_intel_bytcht_jdmode3)) {
-		rt5670->pdata.dmic_en = true;
-		rt5670->pdata.dmic1_data_pin = RT5670_DMIC_DATA_IN2P;
-		rt5670->pdata.dev_gpio = true;
+		dev_info(&i2c->dev, "quirk JD mode 2\n");
+	}
+	if (rt5670_quirk & RT5670_JD_MODE3) {
 		rt5670->pdata.jd_mode = 3;
+		dev_info(&i2c->dev, "quirk JD mode 3\n");
 	}
 
 	rt5670->regmap = devm_regmap_init_i2c(i2c, &rt5670_regmap);
-- 
2.11.0

[PATCH RESEND 2/3] ASoC: Intel: cht_bsw_rt5672: use actual HID in suspend/resume

[Pierre-Louis Bossart]

The code scans all components looking for the default name
i2c-10EC5670:00, which of course doesn't work in platforms
where the BIOS uses a different HID such as Dell 5585

Since we already have the correct information available, just
use the actual codec name and length.

Acked-by: Vinod Koul <vinod.koul@intel.com>
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 sound/soc/intel/boards/cht_bsw_rt5672.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/sound/soc/intel/boards/cht_bsw_rt5672.c b/sound/soc/intel/boards/cht_bsw_rt5672.c
index a0e60bc1f84f..5b1e3ee965a7 100644
--- a/sound/soc/intel/boards/cht_bsw_rt5672.c
+++ b/sound/soc/intel/boards/cht_bsw_rt5672.c
@@ -347,9 +347,11 @@ static struct snd_soc_dai_link cht_dailink[] = {
 static int cht_suspend_pre(struct snd_soc_card *card)
 {
 	struct snd_soc_component *component;
+	struct cht_mc_private *ctx = snd_soc_card_get_drvdata(card);
 
 	list_for_each_entry(component, &card->component_dev_list, card_list) {
-		if (!strcmp(component->name, "i2c-10EC5670:00")) {
+		if (!strncmp(component->name,
+			     ctx->codec_name, sizeof(ctx->codec_name))) {
 			struct snd_soc_codec *codec = snd_soc_component_to_codec(component);
 
 			dev_dbg(codec->dev, "disabling jack detect before going to suspend.\n");
@@ -363,9 +365,11 @@ static int cht_suspend_pre(struct snd_soc_card *card)
 static int cht_resume_post(struct snd_soc_card *card)
 {
 	struct snd_soc_component *component;
+	struct cht_mc_private *ctx = snd_soc_card_get_drvdata(card);
 
 	list_for_each_entry(component, &card->component_dev_list, card_list) {
-		if (!strcmp(component->name, "i2c-10EC5670:00")) {
+		if (!strncmp(component->name,
+			     ctx->codec_name, sizeof(ctx->codec_name))) {
 			struct snd_soc_codec *codec = snd_soc_component_to_codec(component);
 
 			dev_dbg(codec->dev, "enabling jack detect for resume.\n");
-- 
2.11.0

[PATCH RESEND 3/3] ASoC: Intel: cht_bsw_rt5672: fix card name

[Pierre-Louis Bossart]

For now reason the current card name is a ridiculous
'cherrytrailcraudio'. This isn't very useful or self-explanatory,
change to driver name cht-bsw-rt5672.

[Additional comment from Takashi: One bonus by this change would be to
avoid inconsistent names by the driver's name string size.  Since it's
16 bytes including \0, the name is shortened as cherrytrailcrau or
such in some fields.  With the new name this shouldn't happen ]

Reviewed-by: Takashi Iwai <tiwai@suse.de>
Acked-by: Vinod Koul <vinod.koul@intel.com>
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 sound/soc/intel/boards/cht_bsw_rt5672.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/soc/intel/boards/cht_bsw_rt5672.c b/sound/soc/intel/boards/cht_bsw_rt5672.c
index 5b1e3ee965a7..f799b76fdedf 100644
--- a/sound/soc/intel/boards/cht_bsw_rt5672.c
+++ b/sound/soc/intel/boards/cht_bsw_rt5672.c
@@ -383,7 +383,7 @@ static int cht_resume_post(struct snd_soc_card *card)
 
 /* SoC card */
 static struct snd_soc_card snd_soc_card_cht = {
-	.name = "cherrytrailcraudio",
+	.name = "cht-bsw-rt5672",
 	.owner = THIS_MODULE,
 	.dai_link = cht_dailink,
 	.num_links = ARRAY_SIZE(cht_dailink),
-- 
2.11.0

Re: [PATCH RESEND 0/3] Intel-rt5670 updates

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 632 bytes --]

On Tue, Sep 19, 2017 at 07:34:50PM -0500, Pierre-Louis Bossart wrote:
> [when I started cleaning-up some patches reviewed by Andy Shevchenko,
> I realized this initial batch has not been merged, although both
> Takashi and Vinod reviewed them, see
> http://mailman.alsa-project.org/pipermail/alsa-devel/2017-September/125331.html

Please allow a reasonable time for review.  People get busy, go on
holiday, attend conferences and so on so unless there is some reason for
urgency (like critical bug fixes) please allow at least a couple of
weeks for review.  It's been less than two weeks, including both the
merge window and LPC...

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH RESEND 0/3] Intel-rt5670 updates

[Pierre-Louis Bossart]

On 9/20/17 5:41 AM, Mark Brown wrote:
> On Tue, Sep 19, 2017 at 07:34:50PM -0500, Pierre-Louis Bossart wrote:
>> [when I started cleaning-up some patches reviewed by Andy Shevchenko,
>> I realized this initial batch has not been merged, although both
>> Takashi and Vinod reviewed them, see
>> http://mailman.alsa-project.org/pipermail/alsa-devel/2017-September/125331.html
> 
> Please allow a reasonable time for review.  People get busy, go on
> holiday, attend conferences and so on so unless there is some reason for
> urgency (like critical bug fixes) please allow at least a couple of
> weeks for review.  It's been less than two weeks, including both the
> merge window and LPC...

Sure. it's just that you already merged a number of patches sent *after* 
this batch and they aren't queued in your topic/intel branch either.

Re: [PATCH RESEND 0/3] Intel-rt5670 updates

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 635 bytes --]

On Wed, Sep 20, 2017 at 08:17:09AM -0500, Pierre-Louis Bossart wrote:
> On 9/20/17 5:41 AM, Mark Brown wrote:

> > Please allow a reasonable time for review.  People get busy, go on
> > holiday, attend conferences and so on so unless there is some reason for
> > urgency (like critical bug fixes) please allow at least a couple of
> > weeks for review.  It's been less than two weeks, including both the
> > merge window and LPC...

> Sure. it's just that you already merged a number of patches sent *after*
> this batch and they aren't queued in your topic/intel branch either.

You really shouldn't expect things to be handled FIFO.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]