[PATCH 0/3] ASoC: Intel: ACPI-related fixes

[PATCH 0/3] ASoC: Intel: ACPI-related fixes

[Pierre-Louis Bossart]

Here are 3 patches that were provided earlier on the mailing list or
in Bugzilla entries, I rebased and ported them on top of the latest
ACPI/KConfig fixes.

Jeremy Cline (1):
  ASoC: Replace snd_soc_acpi_check_hid with acpi_dev_present

Pierre-Louis Bossart (2):
  ASoC: acpi: fix machine driver selection based on quirk
  ASoC: Intel: bytcht_es8316: fix HID handling

 include/sound/soc-acpi.h               |  3 ---
 sound/soc/intel/boards/Kconfig         |  1 +
 sound/soc/intel/boards/bytcht_es8316.c | 26 ++++++++++++++++++++++-
 sound/soc/soc-acpi.c                   | 38 ++++------------------------------
 4 files changed, 30 insertions(+), 38 deletions(-)

-- 
2.14.1

[PATCH 1/3] [PATCH] ASoC: acpi: fix machine driver selection based on quirk

[Pierre-Louis Bossart]

The ACPI/machine-driver code refactoring introduced in 4.13 introduced
a regression for cases where we need a DMI-based quirk to select the
machine driver (the BIOS reports an invalid HID). The fix is just to
make sure the results of the quirk are actually used.

Fixes: 54746dabf770 ('ASoC: Improve machine driver selection based on quirk data')
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=96691
Tested-by: Nicole Færber <nicole.faerber@dpin.de>
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 sound/soc/soc-acpi.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/sound/soc/soc-acpi.c b/sound/soc/soc-acpi.c
index f21df28bc28e..e103f3571b55 100644
--- a/sound/soc/soc-acpi.c
+++ b/sound/soc/soc-acpi.c
@@ -84,11 +84,9 @@ snd_soc_acpi_find_machine(struct snd_soc_acpi_mach *machines)
 
 	for (mach = machines; mach->id[0]; mach++) {
 		if (snd_soc_acpi_check_hid(mach->id) == true) {
-			if (mach->machine_quirk == NULL)
-				return mach;
-
 			if (mach->machine_quirk(mach) != NULL)
-				return mach;
+				mach = mach->machine_quirk(mach);
+			return mach;
 		}
 	}
 	return NULL;
-- 
2.14.1

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH 1/3] [PATCH] ASoC: acpi: fix machine driver selection based on quirk

[Andy Shevchenko]

On Wed, 2018-01-03 at 11:02 -0600, Pierre-Louis Bossart wrote:
> The ACPI/machine-driver code refactoring introduced in 4.13 introduced
> a regression for cases where we need a DMI-based quirk to select the
> machine driver (the BIOS reports an invalid HID). The fix is just to
> make sure the results of the quirk are actually used.
> 

>  	for (mach = machines; mach->id[0]; mach++) {
>  		if (snd_soc_acpi_check_hid(mach->id) == true) {
>  			if (mach->machine_quirk(mach) != NULL)

Just a nit: perhaps use more natural style, i.e. drop " != NULL" part
off?

> +				mach = mach->machine_quirk(mach);
> +			return mach;
>  		}
>  	}
>  	return NULL;

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH 1/3] [PATCH] ASoC: acpi: fix machine driver selection based on quirk

[Pierre-Louis Bossart]

On 1/4/18 7:08 AM, Andy Shevchenko wrote:
> On Wed, 2018-01-03 at 11:02 -0600, Pierre-Louis Bossart wrote:
>> The ACPI/machine-driver code refactoring introduced in 4.13 introduced
>> a regression for cases where we need a DMI-based quirk to select the
>> machine driver (the BIOS reports an invalid HID). The fix is just to
>> make sure the results of the quirk are actually used.
>>
> 
>>   	for (mach = machines; mach->id[0]; mach++) {
>>   		if (snd_soc_acpi_check_hid(mach->id) == true) {
>>   			if (mach->machine_quirk(mach) != NULL)
> 
> Just a nit: perhaps use more natural style, i.e. drop " != NULL" part
> off?

ok.

> 
>> +				mach = mach->machine_quirk(mach);
>> +			return mach;
>>   		}
>>   	}
>>   	return NULL;
> 

Re: [PATCH 1/3] [PATCH] ASoC: acpi: fix machine driver selection based on quirk

[Pierre-Louis Bossart]

On 01/04/2018 09:18 AM, Pierre-Louis Bossart wrote:
> On 1/4/18 7:08 AM, Andy Shevchenko wrote:
>> On Wed, 2018-01-03 at 11:02 -0600, Pierre-Louis Bossart wrote:
>>> The ACPI/machine-driver code refactoring introduced in 4.13 introduced
>>> a regression for cases where we need a DMI-based quirk to select the
>>> machine driver (the BIOS reports an invalid HID). The fix is just to
>>> make sure the results of the quirk are actually used.
>>>
>>
>>>       for (mach = machines; mach->id[0]; mach++) {
>>>           if (snd_soc_acpi_check_hid(mach->id) == true) {
>>>               if (mach->machine_quirk(mach) != NULL)
>>
>> Just a nit: perhaps use more natural style, i.e. drop " != NULL" part
>> off?
>
> ok.
I found out while testing Andy's patches that there was a 
merge/copy-paste mistake here which wasn't detected in my tests - not 
sure how. it should be (as in the initial patch on bugzilla)

if (mach->machine_quirk)
                 mach = mach->machine_quirk(mach);

Will fix in the next version

>
>>
>>> +                mach = mach->machine_quirk(mach);
>>> +            return mach;
>>>           }
>>>       }
>>>       return NULL;
>>
>
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

[PATCH 2/3] ASoC: Replace snd_soc_acpi_check_hid with acpi_dev_present

[Pierre-Louis Bossart]

From: Jeremy Cline <jeremy@jcline.org>

Replace snd_soc_acpi_check_hid() with the generic acpi_dev_present()
and remove the now unused snd_soc_acpi_check_hid function. This should
have no functional change.

Signed-off-by: Jeremy Cline <jeremy@jcline.org>
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 include/sound/soc-acpi.h |  3 ---
 sound/soc/soc-acpi.c     | 32 ++------------------------------
 2 files changed, 2 insertions(+), 33 deletions(-)

diff --git a/include/sound/soc-acpi.h b/include/sound/soc-acpi.h
index a93436089bf5..d1aaf876cd26 100644
--- a/include/sound/soc-acpi.h
+++ b/include/sound/soc-acpi.h
@@ -50,9 +50,6 @@ snd_soc_acpi_find_package_from_hid(const u8 hid[ACPI_ID_LEN],
 struct snd_soc_acpi_mach *
 snd_soc_acpi_find_machine(struct snd_soc_acpi_mach *machines);
 
-/* acpi check hid */
-bool snd_soc_acpi_check_hid(const u8 hid[ACPI_ID_LEN]);
-
 /**
  * snd_soc_acpi_mach: ACPI-based machine descriptor. Most of the fields are
  * related to the hardware, except for the firmware and topology file names.
diff --git a/sound/soc/soc-acpi.c b/sound/soc/soc-acpi.c
index e103f3571b55..06d2aaccdb32 100644
--- a/sound/soc/soc-acpi.c
+++ b/sound/soc/soc-acpi.c
@@ -49,41 +49,13 @@ const char *snd_soc_acpi_find_name_from_hid(const u8 hid[ACPI_ID_LEN])
 }
 EXPORT_SYMBOL_GPL(snd_soc_acpi_find_name_from_hid);
 
-static acpi_status snd_soc_acpi_mach_match(acpi_handle handle, u32 level,
-				       void *context, void **ret)
-{
-	unsigned long long sta;
-	acpi_status status;
-
-	*(bool *)context = true;
-	status = acpi_evaluate_integer(handle, "_STA", NULL, &sta);
-	if (ACPI_FAILURE(status) || !(sta & ACPI_STA_DEVICE_PRESENT))
-		*(bool *)context = false;
-
-	return AE_OK;
-}
-
-bool snd_soc_acpi_check_hid(const u8 hid[ACPI_ID_LEN])
-{
-	acpi_status status;
-	bool found = false;
-
-	status = acpi_get_devices(hid, snd_soc_acpi_mach_match, &found, NULL);
-
-	if (ACPI_FAILURE(status))
-		return false;
-
-	return found;
-}
-EXPORT_SYMBOL_GPL(snd_soc_acpi_check_hid);
-
 struct snd_soc_acpi_mach *
 snd_soc_acpi_find_machine(struct snd_soc_acpi_mach *machines)
 {
 	struct snd_soc_acpi_mach *mach;
 
 	for (mach = machines; mach->id[0]; mach++) {
-		if (snd_soc_acpi_check_hid(mach->id) == true) {
+		if (acpi_dev_present(mach->id, NULL, -1)) {
 			if (mach->machine_quirk(mach) != NULL)
 				mach = mach->machine_quirk(mach);
 			return mach;
@@ -161,7 +133,7 @@ struct snd_soc_acpi_mach *snd_soc_acpi_codec_list(void *arg)
 		return mach;
 
 	for (i = 0; i < codec_list->num_codecs; i++) {
-		if (snd_soc_acpi_check_hid(codec_list->codecs[i]) != true)
+		if (!acpi_dev_present(codec_list->codecs[i], NULL, -1))
 			return NULL;
 	}
 
-- 
2.14.1

Re: [PATCH 2/3] ASoC: Replace snd_soc_acpi_check_hid with acpi_dev_present

[Andy Shevchenko]

On Wed, 2018-01-03 at 11:02 -0600, Pierre-Louis Bossart wrote:
> From: Jeremy Cline <jeremy@jcline.org>
> 
> Replace snd_soc_acpi_check_hid() with the generic acpi_dev_present()
> and remove the now unused snd_soc_acpi_check_hid function. This should
> 
> have no functional change.

Nice catch!

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> 
> Signed-off-by: Jeremy Cline <jeremy@jcline.org>
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.
> com>
> ---
>  include/sound/soc-acpi.h |  3 ---
>  sound/soc/soc-acpi.c     | 32 ++------------------------------
>  2 files changed, 2 insertions(+), 33 deletions(-)
> 
> diff --git a/include/sound/soc-acpi.h b/include/sound/soc-acpi.h
> index a93436089bf5..d1aaf876cd26 100644
> --- a/include/sound/soc-acpi.h
> +++ b/include/sound/soc-acpi.h
> @@ -50,9 +50,6 @@ snd_soc_acpi_find_package_from_hid(const u8
> hid[ACPI_ID_LEN],
>  struct snd_soc_acpi_mach *
>  snd_soc_acpi_find_machine(struct snd_soc_acpi_mach *machines);
>  
> -/* acpi check hid */
> -bool snd_soc_acpi_check_hid(const u8 hid[ACPI_ID_LEN]);
> -
>  /**
>   * snd_soc_acpi_mach: ACPI-based machine descriptor. Most of the
> fields are
>   * related to the hardware, except for the firmware and topology file
> names.
> diff --git a/sound/soc/soc-acpi.c b/sound/soc/soc-acpi.c
> index e103f3571b55..06d2aaccdb32 100644
> --- a/sound/soc/soc-acpi.c
> +++ b/sound/soc/soc-acpi.c
> @@ -49,41 +49,13 @@ const char *snd_soc_acpi_find_name_from_hid(const
> u8 hid[ACPI_ID_LEN])
>  }
>  EXPORT_SYMBOL_GPL(snd_soc_acpi_find_name_from_hid);
>  
> -static acpi_status snd_soc_acpi_mach_match(acpi_handle handle, u32
> level,
> -				       void *context, void **ret)
> -{
> -	unsigned long long sta;
> -	acpi_status status;
> -
> -	*(bool *)context = true;
> -	status = acpi_evaluate_integer(handle, "_STA", NULL, &sta);
> -	if (ACPI_FAILURE(status) || !(sta & ACPI_STA_DEVICE_PRESENT))
> -		*(bool *)context = false;
> -
> -	return AE_OK;
> -}
> -
> -bool snd_soc_acpi_check_hid(const u8 hid[ACPI_ID_LEN])
> -{
> -	acpi_status status;
> -	bool found = false;
> -
> -	status = acpi_get_devices(hid, snd_soc_acpi_mach_match,
> &found, NULL);
> -
> -	if (ACPI_FAILURE(status))
> -		return false;
> -
> -	return found;
> -}
> -EXPORT_SYMBOL_GPL(snd_soc_acpi_check_hid);
> -
>  struct snd_soc_acpi_mach *
>  snd_soc_acpi_find_machine(struct snd_soc_acpi_mach *machines)
>  {
>  	struct snd_soc_acpi_mach *mach;
>  
>  	for (mach = machines; mach->id[0]; mach++) {
> -		if (snd_soc_acpi_check_hid(mach->id) == true) {
> +		if (acpi_dev_present(mach->id, NULL, -1)) {
>  			if (mach->machine_quirk(mach) != NULL)
>  				mach = mach->machine_quirk(mach);
>  			return mach;
> @@ -161,7 +133,7 @@ struct snd_soc_acpi_mach
> *snd_soc_acpi_codec_list(void *arg)
>  		return mach;
>  
>  	for (i = 0; i < codec_list->num_codecs; i++) {
> -		if (snd_soc_acpi_check_hid(codec_list->codecs[i]) !=
> true)
> +		if (!acpi_dev_present(codec_list->codecs[i], NULL,
> -1))
>  			return NULL;
>  	}
>  

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

[PATCH 3/3] ASoC: Intel: bytcht_es8316: fix HID handling

[Pierre-Louis Bossart]

Same problem as with previous machine drivers, the codec dai
uses a hard-coded name of "i2c-ESSX8316:00" but ACPI provides
"i2c-ESSX8316:01" in some systems.

Fix by overriding the hard-coded value with the codec name derived
from the HID information

Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=189261
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 sound/soc/intel/boards/Kconfig         |  1 +
 sound/soc/intel/boards/bytcht_es8316.c | 26 +++++++++++++++++++++++++-
 2 files changed, 26 insertions(+), 1 deletion(-)

diff --git a/sound/soc/intel/boards/Kconfig b/sound/soc/intel/boards/Kconfig
index 89a73e3d9d2d..4af2393160bf 100644
--- a/sound/soc/intel/boards/Kconfig
+++ b/sound/soc/intel/boards/Kconfig
@@ -153,6 +153,7 @@ config SND_SOC_INTEL_BYT_CHT_DA7213_MACH
 config SND_SOC_INTEL_BYT_CHT_ES8316_MACH
 	tristate "Baytrail & Cherrytrail with ES8316 codec"
 	depends on X86_INTEL_LPSS && I2C && ACPI
+	select SND_SOC_ACPI
 	select SND_SOC_ES8316
 	help
 	  This adds support for ASoC machine driver for Intel(R) Baytrail &
diff --git a/sound/soc/intel/boards/bytcht_es8316.c b/sound/soc/intel/boards/bytcht_es8316.c
index 8088396717e3..ae24f6205f05 100644
--- a/sound/soc/intel/boards/bytcht_es8316.c
+++ b/sound/soc/intel/boards/bytcht_es8316.c
@@ -232,15 +232,39 @@ static struct snd_soc_card byt_cht_es8316_card = {
 	.fully_routed = true,
 };
 
+static char codec_name[16]; /* i2c-<HID>:00 with HID being 8 chars */
+
 static int snd_byt_cht_es8316_mc_probe(struct platform_device *pdev)
 {
-	int ret = 0;
 	struct byt_cht_es8316_private *priv;
+	struct snd_soc_acpi_mach *mach;
+	const char *i2c_name = NULL;
+	int dai_index = 0;
+	int i;
+	int ret = 0;
 
 	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_ATOMIC);
 	if (!priv)
 		return -ENOMEM;
 
+	mach = (&pdev->dev)->platform_data;
+	/* fix index of codec dai */
+	for (i = 0; i < ARRAY_SIZE(byt_cht_es8316_dais); i++) {
+		if (!strcmp(byt_cht_es8316_dais[i].codec_name,
+			    "i2c-ESSX8316:00")) {
+			dai_index = i;
+			break;
+		}
+	}
+
+	/* fixup codec name based on HID */
+	i2c_name = snd_soc_acpi_find_name_from_hid(mach->id);
+	if (i2c_name) {
+		snprintf(codec_name, sizeof(codec_name),
+			"%s%s", "i2c-", i2c_name);
+		byt_cht_es8316_dais[dai_index].codec_name = codec_name;
+	}
+
 	/* register the soc card */
 	byt_cht_es8316_card.dev = &pdev->dev;
 	snd_soc_card_set_drvdata(&byt_cht_es8316_card, priv);
-- 
2.14.1

Re: [PATCH 3/3] ASoC: Intel: bytcht_es8316: fix HID handling

[Andy Shevchenko]

On Wed, 2018-01-03 at 11:02 -0600, Pierre-Louis Bossart wrote:
> Same problem as with previous machine drivers, the codec dai
> uses a hard-coded name of "i2c-ESSX8316:00" but ACPI provides
> "i2c-ESSX8316:01" in some systems.

Yeah, that's why using device instances is fragile...

> 
> Fix by overriding the hard-coded value with the codec name derived
> from the HID information

The patch makes me think about introducing acpi_dev_get_dev_name() and
utilize it here since I need something similar to have in one of GPIO
drivers.

> 
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=189261
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.
> com>
> ---
>  sound/soc/intel/boards/Kconfig         |  1 +
>  sound/soc/intel/boards/bytcht_es8316.c | 26
> +++++++++++++++++++++++++-
>  2 files changed, 26 insertions(+), 1 deletion(-)
> 
> diff --git a/sound/soc/intel/boards/Kconfig
> b/sound/soc/intel/boards/Kconfig
> index 89a73e3d9d2d..4af2393160bf 100644
> --- a/sound/soc/intel/boards/Kconfig
> +++ b/sound/soc/intel/boards/Kconfig
> @@ -153,6 +153,7 @@ config SND_SOC_INTEL_BYT_CHT_DA7213_MACH
>  config SND_SOC_INTEL_BYT_CHT_ES8316_MACH
>  	tristate "Baytrail & Cherrytrail with ES8316 codec"
>  	depends on X86_INTEL_LPSS && I2C && ACPI
> +	select SND_SOC_ACPI
>  	select SND_SOC_ES8316
>  	help
>  	  This adds support for ASoC machine driver for Intel(R)
> Baytrail &
> diff --git a/sound/soc/intel/boards/bytcht_es8316.c
> b/sound/soc/intel/boards/bytcht_es8316.c
> index 8088396717e3..ae24f6205f05 100644
> --- a/sound/soc/intel/boards/bytcht_es8316.c
> +++ b/sound/soc/intel/boards/bytcht_es8316.c
> @@ -232,15 +232,39 @@ static struct snd_soc_card byt_cht_es8316_card =
> {
>  	.fully_routed = true,
>  };
>  
> +static char codec_name[16]; /* i2c-<HID>:00 with HID being 8 chars */
> +

I would rather do use 4 + ACPI_ID_LEN + 3 /* or 1 + 2 */ + 1 and explain
each part in the comment above.

>  static int snd_byt_cht_es8316_mc_probe(struct platform_device *pdev)
>  {
> -	int ret = 0;
>  	struct byt_cht_es8316_private *priv;
> +	struct snd_soc_acpi_mach *mach;
> +	const char *i2c_name = NULL;
> +	int dai_index = 0;
> +	int i;
> +	int ret = 0;
>  
>  	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_ATOMIC);
>  	if (!priv)
>  		return -ENOMEM;
>  
> +	mach = (&pdev->dev)->platform_data;
> +	/* fix index of codec dai */
> +	for (i = 0; i < ARRAY_SIZE(byt_cht_es8316_dais); i++) {
> +		if (!strcmp(byt_cht_es8316_dais[i].codec_name,
> +			    "i2c-ESSX8316:00")) {
> +			dai_index = i;
> +			break;
> +		}

Perhaps at some point you can do such in more generic (across Intel
ASoCs) ?

> +	}
> +
> +	/* fixup codec name based on HID */
> +	i2c_name = snd_soc_acpi_find_name_from_hid(mach->id);
> +	if (i2c_name) {
> +		snprintf(codec_name, sizeof(codec_name),
> +			"%s%s", "i2c-", i2c_name);
> +		byt_cht_es8316_dais[dai_index].codec_name =
> codec_name;
> +	}
> +
>  	/* register the soc card */
>  	byt_cht_es8316_card.dev = &pdev->dev;
>  	snd_soc_card_set_drvdata(&byt_cht_es8316_card, priv);

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH 3/3] ASoC: Intel: bytcht_es8316: fix HID handling

[Pierre-Louis Bossart]

On 1/4/18 8:57 AM, Andy Shevchenko wrote:
> On Wed, 2018-01-03 at 11:02 -0600, Pierre-Louis Bossart wrote:
>> Same problem as with previous machine drivers, the codec dai
>> uses a hard-coded name of "i2c-ESSX8316:00" but ACPI provides
>> "i2c-ESSX8316:01" in some systems.
> 
> Yeah, that's why using device instances is fragile...
> 
>>
>> Fix by overriding the hard-coded value with the codec name derived
>> from the HID information
> 
> The patch makes me think about introducing acpi_dev_get_dev_name() and
> utilize it here since I need something similar to have in one of GPIO
> drivers.

we use this: snd_soc_acpi_find_name_from_hid(mach->id)

When I started all this the recommendation was to do all this on the 
audio side of things, I have no objections to a move of the 
functionality in acpi.

[snip]

>> diff --git a/sound/soc/intel/boards/bytcht_es8316.c
>> b/sound/soc/intel/boards/bytcht_es8316.c
>> index 8088396717e3..ae24f6205f05 100644
>> --- a/sound/soc/intel/boards/bytcht_es8316.c
>> +++ b/sound/soc/intel/boards/bytcht_es8316.c
>> @@ -232,15 +232,39 @@ static struct snd_soc_card byt_cht_es8316_card =
>> {
>>   	.fully_routed = true,
>>   };
>>   
>> +static char codec_name[16]; /* i2c-<HID>:00 with HID being 8 chars */
>> +
> 
> I would rather do use 4 + ACPI_ID_LEN + 3 /* or 1 + 2 */ + 1 and explain
> each part in the comment above.

yes we could do this. this is the same code as in other machine drivers, 
so we should do the replacement across all of them in one shot in a 
separate patch.

> 
>>   static int snd_byt_cht_es8316_mc_probe(struct platform_device *pdev)
>>   {
>> -	int ret = 0;
>>   	struct byt_cht_es8316_private *priv;
>> +	struct snd_soc_acpi_mach *mach;
>> +	const char *i2c_name = NULL;
>> +	int dai_index = 0;
>> +	int i;
>> +	int ret = 0;
>>   
>>   	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_ATOMIC);
>>   	if (!priv)
>>   		return -ENOMEM;
>>   
>> +	mach = (&pdev->dev)->platform_data;
>> +	/* fix index of codec dai */
>> +	for (i = 0; i < ARRAY_SIZE(byt_cht_es8316_dais); i++) {
>> +		if (!strcmp(byt_cht_es8316_dais[i].codec_name,
>> +			    "i2c-ESSX8316:00")) {
>> +			dai_index = i;
>> +			break;
>> +		}
> 
> Perhaps at some point you can do such in more generic (across Intel
> ASoCs) ?

Not sure what you are hinting at here? Did you mean changing the name of 
the array? Or adding a helper function to do this?

Re: [PATCH 3/3] ASoC: Intel: bytcht_es8316: fix HID handling

[Andy Shevchenko]

On Thu, 2018-01-04 at 09:24 -0600, Pierre-Louis Bossart wrote:
> On 1/4/18 8:57 AM, Andy Shevchenko wrote:
> > On Wed, 2018-01-03 at 11:02 -0600, Pierre-Louis Bossart wrote:
> > > 
> > > Fix by overriding the hard-coded value with the codec name derived
> > > from the HID information
> > 
> > The patch makes me think about introducing acpi_dev_get_dev_name()
> > and
> > utilize it here since I need something similar to have in one of
> > GPIO
> > drivers.
> 
> we use this: snd_soc_acpi_find_name_from_hid(mach->id)
> 
> When I started all this the recommendation was to do all this on the 
> audio side of things, I have no objections to a move of the 
> functionality in acpi.

I almost done a patch. I will Cc you for the series.

> > > +static char codec_name[16]; /* i2c-<HID>:00 with HID being 8
> > > chars */
> > > +
> > 
> > I would rather do use 4 + ACPI_ID_LEN + 3 /* or 1 + 2 */ + 1 and
> > explain
> > each part in the comment above.
> 
> yes we could do this. this is the same code as in other machine
> drivers, 
> so we should do the replacement across all of them in one shot in a 
> separate patch.

Whatever you prefer, it's minor.
  
> > > +	mach = (&pdev->dev)->platform_data;
> > > +	/* fix index of codec dai */
> > > +	for (i = 0; i < ARRAY_SIZE(byt_cht_es8316_dais); i++) {
> > > +		if (!strcmp(byt_cht_es8316_dais[i].codec_name,
> > > +			    "i2c-ESSX8316:00")) {
> > > +			dai_index = i;
> > > +			break;
> > > +		}
> > 
> > Perhaps at some point you can do such in more generic (across Intel
> > ASoCs) ?
> 
> Not sure what you are hinting at here? Did you mean changing the name
> of 
> the array? Or adding a helper function to do this?

Suggestion is to create a helper out of similar for-loops-fix-name in
all current users.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH 3/3] ASoC: Intel: bytcht_es8316: fix HID handling

[Pierre-Louis Bossart]

On 1/4/18 10:00 AM, Andy Shevchenko wrote:
> On Thu, 2018-01-04 at 09:24 -0600, Pierre-Louis Bossart wrote:
>> On 1/4/18 8:57 AM, Andy Shevchenko wrote:
>>> On Wed, 2018-01-03 at 11:02 -0600, Pierre-Louis Bossart wrote:
>>>>
>>>> Fix by overriding the hard-coded value with the codec name derived
>>>> from the HID information
>>>
>>> The patch makes me think about introducing acpi_dev_get_dev_name()
>>> and
>>> utilize it here since I need something similar to have in one of
>>> GPIO
>>> drivers.
>>
>> we use this: snd_soc_acpi_find_name_from_hid(mach->id)
>>
>> When I started all this the recommendation was to do all this on the
>> audio side of things, I have no objections to a move of the
>> functionality in acpi.
> 
> I almost done a patch. I will Cc you for the series.

ok. The only concern I have is that this needs to be a somewhat 
contained change that doesn't depend on all sorts of other ACPI/GPIO 
changes. We will have to maintain a v4.14-based branch for a very very 
long time and the simpler we make it for people who back-port or 
cherry-pick the better.

> 
>>>> +static char codec_name[16]; /* i2c-<HID>:00 with HID being 8
>>>> chars */
>>>> +
>>>
>>> I would rather do use 4 + ACPI_ID_LEN + 3 /* or 1 + 2 */ + 1 and
>>> explain
>>> each part in the comment above.
>>
>> yes we could do this. this is the same code as in other machine
>> drivers,
>> so we should do the replacement across all of them in one shot in a
>> separate patch.
> 
> Whatever you prefer, it's minor.
>    
>>>> +	mach = (&pdev->dev)->platform_data;
>>>> +	/* fix index of codec dai */
>>>> +	for (i = 0; i < ARRAY_SIZE(byt_cht_es8316_dais); i++) {
>>>> +		if (!strcmp(byt_cht_es8316_dais[i].codec_name,
>>>> +			    "i2c-ESSX8316:00")) {
>>>> +			dai_index = i;
>>>> +			break;
>>>> +		}
>>>
>>> Perhaps at some point you can do such in more generic (across Intel
>>> ASoCs) ?
>>
>> Not sure what you are hinting at here? Did you mean changing the name
>> of
>> the array? Or adding a helper function to do this?
> 
> Suggestion is to create a helper out of similar for-loops-fix-name in
> all current users.

ok, I'll work on this. Thanks for the suggestion.

Re: [PATCH 3/3] ASoC: Intel: bytcht_es8316: fix HID handling

[Andy Shevchenko]

On Thu, 2018-01-04 at 11:06 -0600, Pierre-Louis Bossart wrote:
> On 1/4/18 10:00 AM, Andy Shevchenko wrote:
> > On Thu, 2018-01-04 at 09:24 -0600, Pierre-Louis Bossart wrote:

> > > we use this: snd_soc_acpi_find_name_from_hid(mach->id)
> > > 
> > > When I started all this the recommendation was to do all this on
> > > the
> > > audio side of things, I have no objections to a move of the
> > > functionality in acpi.
> > 
> > I almost done a patch. I will Cc you for the series.
> 
> ok. The only concern I have is that this needs to be a somewhat 
> contained change that doesn't depend on all sorts of other ACPI/GPIO 
> changes. We will have to maintain a v4.14-based branch for a very
> very 
> long time and the simpler we make it for people who back-port or 
> cherry-pick the better.

Yes, that is how series has been prepared. You basically need two first
patches out of three from it. It had been sent already.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Applied "ASoC: Intel: bytcht_es8316: fix HID handling" to the asoc tree

[Mark Brown]

The patch

   ASoC: Intel: bytcht_es8316: fix HID handling

has been applied to the asoc tree at

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git 

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

>From 3c22a73fb87366851dcf48d852357a6d808921cc Mon Sep 17 00:00:00 2001
From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Date: Thu, 11 Jan 2018 13:52:08 -0600
Subject: [PATCH] ASoC: Intel: bytcht_es8316: fix HID handling

Same problem as with previous machine drivers, the codec dai
uses a hard-coded name of "i2c-ESSX8316:00" but ACPI provides
"i2c-ESSX8316:01" in some systems.

Fix by overriding the hard-coded value with the codec name derived
from the HID information

Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=189261
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Acked-By: Vinod Koul <vinod.koul@intel.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 sound/soc/intel/boards/Kconfig         |  1 +
 sound/soc/intel/boards/bytcht_es8316.c | 26 +++++++++++++++++++++++++-
 2 files changed, 26 insertions(+), 1 deletion(-)

diff --git a/sound/soc/intel/boards/Kconfig b/sound/soc/intel/boards/Kconfig
index de598dcbef30..d4e103615f51 100644
--- a/sound/soc/intel/boards/Kconfig
+++ b/sound/soc/intel/boards/Kconfig
@@ -139,6 +139,7 @@ config SND_SOC_INTEL_BYT_CHT_DA7213_MACH
 config SND_SOC_INTEL_BYT_CHT_ES8316_MACH
 	tristate "Baytrail & Cherrytrail with ES8316 codec"
 	depends on X86_INTEL_LPSS && I2C && ACPI
+	select SND_SOC_ACPI
 	select SND_SOC_ES8316
 	help
 	  This adds support for ASoC machine driver for Intel(R) Baytrail &
diff --git a/sound/soc/intel/boards/bytcht_es8316.c b/sound/soc/intel/boards/bytcht_es8316.c
index 8088396717e3..ae24f6205f05 100644
--- a/sound/soc/intel/boards/bytcht_es8316.c
+++ b/sound/soc/intel/boards/bytcht_es8316.c
@@ -232,15 +232,39 @@ static struct snd_soc_card byt_cht_es8316_card = {
 	.fully_routed = true,
 };
 
+static char codec_name[16]; /* i2c-<HID>:00 with HID being 8 chars */
+
 static int snd_byt_cht_es8316_mc_probe(struct platform_device *pdev)
 {
-	int ret = 0;
 	struct byt_cht_es8316_private *priv;
+	struct snd_soc_acpi_mach *mach;
+	const char *i2c_name = NULL;
+	int dai_index = 0;
+	int i;
+	int ret = 0;
 
 	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_ATOMIC);
 	if (!priv)
 		return -ENOMEM;
 
+	mach = (&pdev->dev)->platform_data;
+	/* fix index of codec dai */
+	for (i = 0; i < ARRAY_SIZE(byt_cht_es8316_dais); i++) {
+		if (!strcmp(byt_cht_es8316_dais[i].codec_name,
+			    "i2c-ESSX8316:00")) {
+			dai_index = i;
+			break;
+		}
+	}
+
+	/* fixup codec name based on HID */
+	i2c_name = snd_soc_acpi_find_name_from_hid(mach->id);
+	if (i2c_name) {
+		snprintf(codec_name, sizeof(codec_name),
+			"%s%s", "i2c-", i2c_name);
+		byt_cht_es8316_dais[dai_index].codec_name = codec_name;
+	}
+
 	/* register the soc card */
 	byt_cht_es8316_card.dev = &pdev->dev;
 	snd_soc_card_set_drvdata(&byt_cht_es8316_card, priv);
-- 
2.15.1