[PATCH v2 0/2] Add audio support for Microsoft Surface 3 tablet

[PATCH v2 0/2] Add audio support for Microsoft Surface 3 tablet

[Stephen Just]

This series adds audio support for the Surface 3 tablet.

This tablet identifes an rt5645 audio codec using the ACPI
identifer 10EC5640, which is already associated with the rt5640,
thus a quirk is required to have the correct drivers loaded.

Additionally, in order for jack-detection and the tablet's internal
microphone to work, the Surface 3 must be configured in the audio codec
driver similarly to other devices.

Resolves: https://bugzilla.kernel.org/show_bug.cgi?id=98001

Changes in v2: Remove alsa-lib changes from series

Stephen Just (2):
  ASoC: rt5645: Add support for Surface 3 tablet
  ASoC: Intel: Atom: add quirk for CHT w/ RT5645

 sound/soc/codecs/rt5645.c               |  7 +++++++
 sound/soc/intel/atom/sst/sst_acpi.c     | 29 ++++++++++++++++++++++++++++-
 sound/soc/intel/boards/cht_bsw_rt5645.c |  1 +
 3 files changed, 36 insertions(+), 1 deletion(-)

-- 
2.7.4

[PATCH v2 1/2] ASoC: rt5645: Add support for Surface 3 tablet

[Stephen Just]

* Add a dmi match for Surface 3
* Add ACPI match "10EC5640"

The Surface 3 tablet identifies the codec in ACPI as
10EC5640, but other devices use this identifier to match
the rt5640 codec. However, the probe code for each driver
will filter out mismatches.

Cc: Mark Brown <broonie@kernel.org>
Cc: Vinod Koul <vinod.koul@intel.com>
Signed-off-by: Stephen Just <stephenjust@gmail.com>
---
 sound/soc/codecs/rt5645.c | 7 +++++++
 1 file changed, 7 insertions(+)

v1 -> v2: No change

diff --git a/sound/soc/codecs/rt5645.c b/sound/soc/codecs/rt5645.c
index 3c6594d..761ca88 100644
--- a/sound/soc/codecs/rt5645.c
+++ b/sound/soc/codecs/rt5645.c
@@ -3531,6 +3531,7 @@ MODULE_DEVICE_TABLE(i2c, rt5645_i2c_id);
 static const struct acpi_device_id rt5645_acpi_match[] = {
 	{ "10EC5645", 0 },
 	{ "10EC5650", 0 },
+	{ "10EC5640", 0 },
 	{},
 };
 MODULE_DEVICE_TABLE(acpi, rt5645_acpi_match);
@@ -3561,6 +3562,12 @@ static const struct dmi_system_id dmi_platform_intel_braswell[] = {
 			DMI_MATCH(DMI_PRODUCT_NAME, "Setzer"),
 		},
 	},
+	{
+		.ident = "Microsoft Surface 3",
+		.matches = {
+			DMI_MATCH(DMI_PRODUCT_NAME, "Surface 3"),
+		},
+	},
 	{ }
 };
 
-- 
2.7.4

Re: [PATCH v2 1/2] ASoC: rt5645: Add support for Surface 3 tablet

[Vinod Koul]

On Mon, Jul 04, 2016 at 10:53:47AM -0600, Stephen Just wrote:
> * Add a dmi match for Surface 3
> * Add ACPI match "10EC5640"
> 
> The Surface 3 tablet identifies the codec in ACPI as
> 10EC5640, but other devices use this identifier to match
> the rt5640 codec. However, the probe code for each driver
> will filter out mismatches.
> 
> Cc: Mark Brown <broonie@kernel.org>
> Cc: Vinod Koul <vinod.koul@intel.com>
> Signed-off-by: Stephen Just <stephenjust@gmail.com>
> ---
>  sound/soc/codecs/rt5645.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> v1 -> v2: No change

That is wrong!

> 
> diff --git a/sound/soc/codecs/rt5645.c b/sound/soc/codecs/rt5645.c
> index 3c6594d..761ca88 100644
> --- a/sound/soc/codecs/rt5645.c
> +++ b/sound/soc/codecs/rt5645.c
> @@ -3531,6 +3531,7 @@ MODULE_DEVICE_TABLE(i2c, rt5645_i2c_id);
>  static const struct acpi_device_id rt5645_acpi_match[] = {
>  	{ "10EC5645", 0 },
>  	{ "10EC5650", 0 },
> +	{ "10EC5640", 0 },

AT least here you have a change, but earlier rev was the one I posted in BZ. It is
NOT nice to take other peoples change and post them as your patches.

>  	{},
>  };
>  MODULE_DEVICE_TABLE(acpi, rt5645_acpi_match);
> @@ -3561,6 +3562,12 @@ static const struct dmi_system_id dmi_platform_intel_braswell[] = {
>  			DMI_MATCH(DMI_PRODUCT_NAME, "Setzer"),
>  		},
>  	},
> +	{
> +		.ident = "Microsoft Surface 3",
> +		.matches = {
> +			DMI_MATCH(DMI_PRODUCT_NAME, "Surface 3"),
> +		},
> +	},
>  	{ }
>  };
>  
> -- 
> 2.7.4
> 

-- 
~Vinod

Re: [PATCH v2 1/2] ASoC: rt5645: Add support for Surface 3 tablet

[Takashi Iwai]

On Mon, 04 Jul 2016 18:53:47 +0200,
Stephen Just wrote:
> 
> * Add a dmi match for Surface 3
> * Add ACPI match "10EC5640"
> 
> The Surface 3 tablet identifies the codec in ACPI as
> 10EC5640, but other devices use this identifier to match
> the rt5640 codec. However, the probe code for each driver
> will filter out mismatches.
> 
> Cc: Mark Brown <broonie@kernel.org>
> Cc: Vinod Koul <vinod.koul@intel.com>
> Signed-off-by: Stephen Just <stephenjust@gmail.com>
> ---
>  sound/soc/codecs/rt5645.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> v1 -> v2: No change
> 
> diff --git a/sound/soc/codecs/rt5645.c b/sound/soc/codecs/rt5645.c
> index 3c6594d..761ca88 100644
> --- a/sound/soc/codecs/rt5645.c
> +++ b/sound/soc/codecs/rt5645.c
> @@ -3531,6 +3531,7 @@ MODULE_DEVICE_TABLE(i2c, rt5645_i2c_id);
>  static const struct acpi_device_id rt5645_acpi_match[] = {
>  	{ "10EC5645", 0 },
>  	{ "10EC5650", 0 },
> +	{ "10EC5640", 0 },

Better to put in the right order.


Takashi

[PATCH v2 2/2] ASoC: Intel: Atom: add quirk for CHT w/ RT5645

[Stephen Just]

The Surface 3 uses an RT5645 audio codec, but the ACPI
identifier used is that for the RT5640. Use a DMI match
to override the driver_data provided by ACPI to workaround
ID conflict.

Cc: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Cc: Mark Brown <broonie@kernel.org>
Cc: Vinod Koul <vinod.koul@intel.com>
Signed-off-by: Stephen Just <stephenjust@gmail.com>
---
 sound/soc/intel/atom/sst/sst_acpi.c     | 29 ++++++++++++++++++++++++++++-
 sound/soc/intel/boards/cht_bsw_rt5645.c |  1 +
 2 files changed, 29 insertions(+), 1 deletion(-)

v1 -> v2: No change

diff --git a/sound/soc/intel/atom/sst/sst_acpi.c b/sound/soc/intel/atom/sst/sst_acpi.c
index 3bc4b63..ef40608 100644
--- a/sound/soc/intel/atom/sst/sst_acpi.c
+++ b/sound/soc/intel/atom/sst/sst_acpi.c
@@ -19,6 +19,7 @@
  */
 
 #include <linux/module.h>
+#include <linux/dmi.h>
 #include <linux/fs.h>
 #include <linux/interrupt.h>
 #include <linux/slab.h>
@@ -131,6 +132,8 @@ static struct sst_platform_info chv_platform_data = {
 	.platform = "sst-mfld-platform",
 };
 
+static const struct dmi_system_id dmi_platform_quirk_table[];
+
 static int sst_platform_get_resources(struct intel_sst_drv *ctx)
 {
 	struct resource *rsrc;
@@ -224,6 +227,7 @@ static int sst_acpi_probe(struct platform_device *pdev)
 	struct platform_device *mdev;
 	struct platform_device *plat_dev;
 	struct sst_platform_info *pdata;
+	const struct dmi_system_id *dmi_match;
 	unsigned int dev_id;
 
 	id = acpi_match_device(dev->driver->acpi_match_table, dev);
@@ -231,7 +235,13 @@ static int sst_acpi_probe(struct platform_device *pdev)
 		return -ENODEV;
 	dev_dbg(dev, "for %s", id->id);
 
-	mach = (struct sst_acpi_mach *)id->driver_data;
+	/* check for quirk when getting driver_data */
+	dmi_match = dmi_first_match(dmi_platform_quirk_table);
+	if (dmi_match)
+		mach = (struct sst_acpi_mach *)dmi_match->driver_data;
+	else
+		mach = (struct sst_acpi_mach *)id->driver_data;
+
 	mach = sst_acpi_find_machine(mach);
 	if (mach == NULL) {
 		dev_err(dev, "No matching machine driver found\n");
@@ -345,7 +355,24 @@ static struct sst_acpi_mach sst_acpi_chv[] = {
 	/* some CHT-T platforms rely on RT5640, use Baytrail machine driver */
 	{"10EC5640", "bytcr_rt5640", "intel/fw_sst_22a8.bin", "bytcr_rt5640", NULL,
 						&chv_platform_data },
+	{},
+};
 
+static struct sst_acpi_mach sst_acpi_chv_quirk[] = {
+	/* some CHT-T platforms rely on RT5645 but use the ID from RT5640 */
+	{"10EC5640", "cht-bsw-rt5645", "intel/fw_sst_22a8.bin", "cht-bsw", NULL,
+						&chv_platform_data },
+	{},
+};
+
+static const struct dmi_system_id dmi_platform_quirk_table[] = {
+	{
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "Microsoft Corporation"),
+			DMI_MATCH(DMI_PRODUCT_NAME, "Surface 3"),
+		},
+		.driver_data = (unsigned long *) &sst_acpi_chv_quirk,
+	},
 	{},
 };
 
diff --git a/sound/soc/intel/boards/cht_bsw_rt5645.c b/sound/soc/intel/boards/cht_bsw_rt5645.c
index d7ef292..bf99efb 100644
--- a/sound/soc/intel/boards/cht_bsw_rt5645.c
+++ b/sound/soc/intel/boards/cht_bsw_rt5645.c
@@ -340,6 +340,7 @@ static struct snd_soc_card snd_soc_card_chtrt5650 = {
 };
 
 static struct cht_acpi_card snd_soc_cards[] = {
+	{"10EC5640", CODEC_TYPE_RT5645, &snd_soc_card_chtrt5645},
 	{"10EC5645", CODEC_TYPE_RT5645, &snd_soc_card_chtrt5645},
 	{"10EC5650", CODEC_TYPE_RT5650, &snd_soc_card_chtrt5650},
 };
-- 
2.7.4

Re: [PATCH v2 2/2] ASoC: Intel: Atom: add quirk for CHT w/ RT5645

[Vinod Koul]

On Mon, Jul 04, 2016 at 10:53:48AM -0600, Stephen Just wrote:
> The Surface 3 uses an RT5645 audio codec, but the ACPI
> identifier used is that for the RT5640. Use a DMI match
> to override the driver_data provided by ACPI to workaround
> ID conflict.
> 
> Cc: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> Cc: Mark Brown <broonie@kernel.org>
> Cc: Vinod Koul <vinod.koul@intel.com>
> Signed-off-by: Stephen Just <stephenjust@gmail.com>
> ---
>  sound/soc/intel/atom/sst/sst_acpi.c     | 29 ++++++++++++++++++++++++++++-
>  sound/soc/intel/boards/cht_bsw_rt5645.c |  1 +
>  2 files changed, 29 insertions(+), 1 deletion(-)
> 
> v1 -> v2: No change
> 
> diff --git a/sound/soc/intel/atom/sst/sst_acpi.c b/sound/soc/intel/atom/sst/sst_acpi.c
> index 3bc4b63..ef40608 100644
> --- a/sound/soc/intel/atom/sst/sst_acpi.c
> +++ b/sound/soc/intel/atom/sst/sst_acpi.c
> @@ -19,6 +19,7 @@
>   */
>  
>  #include <linux/module.h>
> +#include <linux/dmi.h>
>  #include <linux/fs.h>
>  #include <linux/interrupt.h>
>  #include <linux/slab.h>
> @@ -131,6 +132,8 @@ static struct sst_platform_info chv_platform_data = {
>  	.platform = "sst-mfld-platform",
>  };
>  
> +static const struct dmi_system_id dmi_platform_quirk_table[];
> +
>  static int sst_platform_get_resources(struct intel_sst_drv *ctx)
>  {
>  	struct resource *rsrc;
> @@ -224,6 +227,7 @@ static int sst_acpi_probe(struct platform_device *pdev)
>  	struct platform_device *mdev;
>  	struct platform_device *plat_dev;
>  	struct sst_platform_info *pdata;
> +	const struct dmi_system_id *dmi_match;
>  	unsigned int dev_id;
>  
>  	id = acpi_match_device(dev->driver->acpi_match_table, dev);
> @@ -231,7 +235,13 @@ static int sst_acpi_probe(struct platform_device *pdev)
>  		return -ENODEV;
>  	dev_dbg(dev, "for %s", id->id);
>  
> -	mach = (struct sst_acpi_mach *)id->driver_data;
> +	/* check for quirk when getting driver_data */
> +	dmi_match = dmi_first_match(dmi_platform_quirk_table);
> +	if (dmi_match)
> +		mach = (struct sst_acpi_mach *)dmi_match->driver_data;
> +	else
> +		mach = (struct sst_acpi_mach *)id->driver_data;
> +
>  	mach = sst_acpi_find_machine(mach);
>  	if (mach == NULL) {
>  		dev_err(dev, "No matching machine driver found\n");
> @@ -345,7 +355,24 @@ static struct sst_acpi_mach sst_acpi_chv[] = {
>  	/* some CHT-T platforms rely on RT5640, use Baytrail machine driver */
>  	{"10EC5640", "bytcr_rt5640", "intel/fw_sst_22a8.bin", "bytcr_rt5640", NULL,
>  						&chv_platform_data },
> +	{},
> +};
>  
> +static struct sst_acpi_mach sst_acpi_chv_quirk[] = {
> +	/* some CHT-T platforms rely on RT5645 but use the ID from RT5640 */
> +	{"10EC5640", "cht-bsw-rt5645", "intel/fw_sst_22a8.bin", "cht-bsw", NULL,
> +						&chv_platform_data },
> +	{},
> +};
> +
> +static const struct dmi_system_id dmi_platform_quirk_table[] = {
> +	{
> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR, "Microsoft Corporation"),
> +			DMI_MATCH(DMI_PRODUCT_NAME, "Surface 3"),
> +		},
> +		.driver_data = (unsigned long *) &sst_acpi_chv_quirk,
> +	},
>  	{},
>  };

This seeems a bit better than the one I posted on BZ, so I will take it.
But I would like to hold off a few days for upstreaming this as there are
few other devices with similar configuration and we need solutions for those
as well.. I would hate to redo the patches already applied.

Also you should add BZ id in the changelog.

-- 
~Vinod

Re: [PATCH v2 2/2] ASoC: Intel: Atom: add quirk for CHT w/ RT5645

[Takashi Iwai]

On Mon, 04 Jul 2016 18:53:48 +0200,
Stephen Just wrote:
> 
> The Surface 3 uses an RT5645 audio codec, but the ACPI
> identifier used is that for the RT5640. Use a DMI match
> to override the driver_data provided by ACPI to workaround
> ID conflict.
> 
> Cc: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> Cc: Mark Brown <broonie@kernel.org>
> Cc: Vinod Koul <vinod.koul@intel.com>
> Signed-off-by: Stephen Just <stephenjust@gmail.com>
> ---
>  sound/soc/intel/atom/sst/sst_acpi.c     | 29 ++++++++++++++++++++++++++++-
>  sound/soc/intel/boards/cht_bsw_rt5645.c |  1 +
>  2 files changed, 29 insertions(+), 1 deletion(-)
> 
> v1 -> v2: No change
> 
> diff --git a/sound/soc/intel/atom/sst/sst_acpi.c b/sound/soc/intel/atom/sst/sst_acpi.c
> index 3bc4b63..ef40608 100644
> --- a/sound/soc/intel/atom/sst/sst_acpi.c
> +++ b/sound/soc/intel/atom/sst/sst_acpi.c
> @@ -19,6 +19,7 @@
>   */
>  
>  #include <linux/module.h>
> +#include <linux/dmi.h>
>  #include <linux/fs.h>
>  #include <linux/interrupt.h>
>  #include <linux/slab.h>
> @@ -131,6 +132,8 @@ static struct sst_platform_info chv_platform_data = {
>  	.platform = "sst-mfld-platform",
>  };
>  
> +static const struct dmi_system_id dmi_platform_quirk_table[];
> +
>  static int sst_platform_get_resources(struct intel_sst_drv *ctx)
>  {
>  	struct resource *rsrc;
> @@ -224,6 +227,7 @@ static int sst_acpi_probe(struct platform_device *pdev)
>  	struct platform_device *mdev;
>  	struct platform_device *plat_dev;
>  	struct sst_platform_info *pdata;
> +	const struct dmi_system_id *dmi_match;
>  	unsigned int dev_id;
>  
>  	id = acpi_match_device(dev->driver->acpi_match_table, dev);
> @@ -231,7 +235,13 @@ static int sst_acpi_probe(struct platform_device *pdev)
>  		return -ENODEV;
>  	dev_dbg(dev, "for %s", id->id);
>  
> -	mach = (struct sst_acpi_mach *)id->driver_data;
> +	/* check for quirk when getting driver_data */
> +	dmi_match = dmi_first_match(dmi_platform_quirk_table);
> +	if (dmi_match)
> +		mach = (struct sst_acpi_mach *)dmi_match->driver_data;

dmi_system_id.driver_data is void*, so you need no cast here (unlike
acpi_device_id below).


> +	else
> +		mach = (struct sst_acpi_mach *)id->driver_data;
> +
>  	mach = sst_acpi_find_machine(mach);
>  	if (mach == NULL) {
>  		dev_err(dev, "No matching machine driver found\n");
> @@ -345,7 +355,24 @@ static struct sst_acpi_mach sst_acpi_chv[] = {
>  	/* some CHT-T platforms rely on RT5640, use Baytrail machine driver */
>  	{"10EC5640", "bytcr_rt5640", "intel/fw_sst_22a8.bin", "bytcr_rt5640", NULL,
>  						&chv_platform_data },
> +	{},
> +};
>  
> +static struct sst_acpi_mach sst_acpi_chv_quirk[] = {
> +	/* some CHT-T platforms rely on RT5645 but use the ID from RT5640 */
> +	{"10EC5640", "cht-bsw-rt5645", "intel/fw_sst_22a8.bin", "cht-bsw", NULL,
> +						&chv_platform_data },
> +	{},
> +};
> +
> +static const struct dmi_system_id dmi_platform_quirk_table[] = {
> +	{
> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR, "Microsoft Corporation"),
> +			DMI_MATCH(DMI_PRODUCT_NAME, "Surface 3"),
> +		},
> +		.driver_data = (unsigned long *) &sst_acpi_chv_quirk,

Why cast to unsigned long pointer...?


Takashi