On Sat, Dec 22, 2018 at 03:47:12PM +0100, Stephan Gerhold wrote:

Add quirks to select the correct input map, jack-detect options and channel map to make sound work on the ASUS MeMO Pad 7 (ME176C).

Everything seems to work fine with the quirks below, although I have to say that the internal microphone quality is really bad. There are always high-pitched sounds in the background. Not sure if the hardware is just really bad or if there is any way to improve it. (I never use it so I have not investigated much further..)

Note: Although sound works out of the box, jack detection currently requires overriding the ACPI DSDT table. This is necessary because the rt5640 ACPI device (10EC5640) has the wrong GPIO listed as interrupt (one of the Bluetooth GPIOs). The correct GPIO is GPO2 0x0004 (listed as the first GPIO in the Intel(R) Audio Machine Driver - AMCR0F28 device).

At some point it might be possible to add a workaround for this using that AMCR0F28 device which has the correct GPIO. However, at the moment I still need the DSDT override for Bluetooth to work (with no simple workaround), so it's probably easiest if we just document it here for now. Eventually I will investigate this later..

Signed-off-by: Stephan Gerhold stephan@gerhold.net

sound/soc/intel/boards/bytcr_rt5640.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)

diff --git a/sound/soc/intel/boards/bytcr_rt5640.c b/sound/soc/intel/boards/bytcr_rt5640.c index a22366ce33c4..ca8b4d5ff70f 100644 --- a/sound/soc/intel/boards/bytcr_rt5640.c +++ b/sound/soc/intel/boards/bytcr_rt5640.c @@ -428,6 +428,18 @@ static const struct dmi_system_id byt_rt5640_quirk_table[] = { BYT_RT5640_SSP0_AIF1 | BYT_RT5640_MCLK_EN), },

• {

• .matches = {
  

• 	DMI_EXACT_MATCH(DMI_SYS_VENDOR, "ASUSTeK COMPUTER INC."),
  

• 	DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "ME176C"),
  

• },
  

• .driver_data = (void *)(BYT_RT5640_IN1_MAP |
  

• 			BYT_RT5640_JD_SRC_JD2_IN4N |
  

• 			BYT_RT5640_OVCD_TH_2000UA |
  

• 			BYT_RT5640_OVCD_SF_0P75 |
  

• 			BYT_RT5640_SSP0_AIF1 |
  

• 			BYT_RT5640_MCLK_EN),
  

• }, { .matches = { DMI_EXACT_MATCH(DMI_SYS_VENDOR, "ASUSTeK COMPUTER INC."),

-- 2.20.1

On Mon, Dec 24, 2018 at 10:01:53AM +0100, Hans de Goede wrote:

H,

On 22-12-18 15:59, Stephan Gerhold wrote:

...

On Sat, Dec 22, 2018 at 03:47:12PM +0100, Stephan Gerhold wrote:

...

Add quirks to select the correct input map, jack-detect options and channel map to make sound work on the ASUS MeMO Pad 7 (ME176C).

Everything seems to work fine with the quirks below, although I have to say that the internal microphone quality is really bad. There are always high-pitched sounds in the background. Not sure if the hardware is just really bad or if there is any way to improve it. (I never use it so I have not investigated much further..)

One possible way of improving it which is worthwhile testing is adding the BYT_RT5640_DIFF_MIC quirk.

Note almost all quirk table entries not using a DMIC mapping have this set (it is also set in INPUT_DEFAULTS) because it does not really hurt to enable it, while likely only a few models really have a diff mic.

I thought that the DIFF_MIC quirk might help, so I tested it shortly before sending this patch: I was not able to hear any difference in the recorded sample, which is why I didn't add it to the quirks.

Would you prefer to have it enabled even if it (seemingly) makes no difference?

...

...

Note: Although sound works out of the box, jack detection currently requires overriding the ACPI DSDT table. This is necessary because the rt5640 ACPI device (10EC5640) has the wrong GPIO listed as interrupt (one of the Bluetooth GPIOs). The correct GPIO is GPO2 0x0004 (listed as the first GPIO in the Intel(R) Audio Machine Driver - AMCR0F28 device).

At some point it might be possible to add a workaround for this using that AMCR0F28 device which has the correct GPIO. However, at the moment I still need the DSDT override for Bluetooth to work (with no simple workaround), so it's probably easiest if we just document it here for now. Eventually I will investigate this later..

+1 for just documenting, as long as this device needs a DSDT override anyways I don't think it is a good idea to add code to the bytcr_rt5640.c machine driver just to avoid that. If the machine driver is the only one needing the DSDT override and some code can avoid it then the balance changes, but for now I believe that just documenting it is best.

Agreed. :)

Regards,

Hans

...

...

Signed-off-by: Stephan Gerhold stephan@gerhold.net

sound/soc/intel/boards/bytcr_rt5640.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)

diff --git a/sound/soc/intel/boards/bytcr_rt5640.c b/sound/soc/intel/boards/bytcr_rt5640.c index a22366ce33c4..ca8b4d5ff70f 100644 --- a/sound/soc/intel/boards/bytcr_rt5640.c +++ b/sound/soc/intel/boards/bytcr_rt5640.c @@ -428,6 +428,18 @@ static const struct dmi_system_id byt_rt5640_quirk_table[] = { BYT_RT5640_SSP0_AIF1 | BYT_RT5640_MCLK_EN), },

• {

• .matches = {
  

• 	DMI_EXACT_MATCH(DMI_SYS_VENDOR, "ASUSTeK COMPUTER INC."),
  

• 	DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "ME176C"),
  

• },
  

• .driver_data = (void *)(BYT_RT5640_IN1_MAP |
  

• 			BYT_RT5640_JD_SRC_JD2_IN4N |
  

• 			BYT_RT5640_OVCD_TH_2000UA |
  

• 			BYT_RT5640_OVCD_SF_0P75 |
  

• 			BYT_RT5640_SSP0_AIF1 |
  

• 			BYT_RT5640_MCLK_EN),
  

• }, { .matches = { DMI_EXACT_MATCH(DMI_SYS_VENDOR, "ASUSTeK COMPUTER INC."),

-- 2.20.1

On 12/31/18 10:30 AM, Stephan Gerhold wrote:

On Mon, Dec 31, 2018 at 09:38:21AM -0600, Pierre-Louis Bossart wrote:

...

On 12/22/18 8:47 AM, Stephan Gerhold wrote:

...

Some devices detected as BYT-T by the PMIC-type based detection have only a single IRQ listed in the 80860F28 ACPI device. This causes -ENXIO later when attempting to get the IRQ at index 5. It turns out these devices behave more like BYT-CR devices, and using the IRQ at index 0 makes sound work correctly.

This patch adds a fallback for these devices to is_byt_cr(): If there is no IRQ resource at index 5, treating the device as BYT-T is guaranteed to fail later, so we can safely treat these devices as BYT-CR without breaking any working device.

Link: http://mailman.alsa-project.org/pipermail/alsa-devel/2018-December/143176.ht... Signed-off-by: Stephan Gerhold stephan@gerhold.net

---

Moved the "Detected Baytrail-CR platform" message to is_byt_cr() so we can log a different message if the fallback is used.

Tested this on my device as-is, and simulated a "normal" BYT-T and BYT-CR device (copied their IRQs to a custom DSDT).

sound/soc/intel/atom/sst/sst_acpi.c | 24 ++++++++++++++++++------ 1 file changed, 18 insertions(+), 6 deletions(-)

diff --git a/sound/soc/intel/atom/sst/sst_acpi.c b/sound/soc/intel/atom/sst/sst_acpi.c index 3a95ebbfc45d..755a396121ff 100644 --- a/sound/soc/intel/atom/sst/sst_acpi.c +++ b/sound/soc/intel/atom/sst/sst_acpi.c @@ -255,10 +255,22 @@ static int is_byt(void) return status; } -static int is_byt_cr(struct device *dev, bool *bytcr) +static int is_byt_cr(struct platform_device *pdev, bool *bytcr) {

• struct device *dev = &pdev->dev; int status = 0;
• if (platform_get_resource(pdev, IORESOURCE_IRQ, 5) == NULL) {

• /*
  

•  * Some devices detected as BYT-T have only a single IRQ listed,
  

•  * causing platform_get_irq with index 5 to return -ENXIO.
  

•  * The correct IRQ in this case is at index 0, as used on BYT-CR.
  

•  */
  

• dev_info(dev, "Falling back to Baytrail-CR platform\n");
  

• *bytcr = true;
  

• return status;
  

• }

Isn't this going to bypass the PMIC-based detection on all BYT-CR devices? Maybe move this code as a fallback used when the PMIC-based detection isn't positive?

Except for the message that is logged, it does not really make a difference. PMIC-based detection is still used for most BYT-CR devices, which usually have 6 IRQs listed. For the few that have not, the end result (bytcr = true) is the same, even if they now use the fallback.

I mentioned this in a previous mail when I asked you which option you would prefer (see [1]). Since is_byt_cr() has multiple returns, I cannot just put it last without refactoring the entire method. (Which is something I wanted to avoid...)

Ah yes, but there was a side thread with Andy Shevchenko where we discussed that the initial return can be simplified since there are wrappers for iosf_mbi_available even when CONFIG_IOSF_MBI is not enabled. The code could be something like

diff --git a/sound/soc/intel/atom/sst/sst_acpi.c b/sound/soc/intel/atom/sst/sst_acpi.c index ac542535b9d5..58e389a64c6a 100644 --- a/sound/soc/intel/atom/sst/sst_acpi.c +++ b/sound/soc/intel/atom/sst/sst_acpi.c @@ -255,17 +255,16 @@ static int is_byt(void)         return status;  }

-static int is_byt_cr(struct device *dev, bool *bytcr) +static int is_byt_cr(struct platform_device *pdev, bool *bytcr)  { +       struct device *dev = &pdev->dev; +       u32 bios_status;         int status = 0;

-       if (IS_ENABLED(CONFIG_IOSF_MBI)) { -               u32 bios_status; +       if (!is_byt()) +               return status;

-               if (!is_byt() || !iosf_mbi_available()) { -                       /* bail silently */ -                       return status; -               } +       if (iosf_mbi_available()) {

status = iosf_mbi_read(BT_MBI_UNIT_PMC, /* 0x04 PUNIT */                                        MBI_REG_READ, /* 0x10 */ @@ -286,6 +285,20 @@ static int is_byt_cr(struct device *dev, bool *bytcr)         } else {                 dev_info(dev, "IOSF_MBI not enabled, no BYT-CR detection\n");         } + +       if (*bytcr == false && +           platform_get_resource(pdev, IORESOURCE_IRQ, 5) == NULL) { +               /* +                * Some devices detected as BYT-T have only a single IRQ listed, +                * causing platform_get_irq with index 5 to return -ENXIO. +                * The correct IRQ in this case is at index 0, as used on +                * BYT-CR. +                */ +               dev_info(dev, "Falling back to Baytrail-CR platform\n"); +               status = 0; +               *bytcr = true; +       } +         return status;  }

...

...

if (IS_ENABLED(CONFIG_IOSF_MBI)) {
	u32 bios_status;

@@ -278,10 +290,12 @@ static int is_byt_cr(struct device *dev, bool *bytcr) /* bits 26:27 mirror PMIC options */ bios_status = (bios_status >> 26) & 3;

• 	if ((bios_status == 1) || (bios_status == 3))
  

• 	if ((bios_status == 1) || (bios_status == 3)) {
  

• 		dev_info(dev, "Detected Baytrail-CR platform\n");
  		*bytcr = true;
  

• 	else
  

• 	} else {
  		dev_info(dev, "BYT-CR not detected\n");
  

• 	}
  }
  

  } else { dev_info(dev, "IOSF_MBI not enabled, no BYT-CR detection\n");

@@ -333,10 +347,8 @@ static int sst_acpi_probe(struct platform_device *pdev) if (ret < 0) return ret;

• ret = is_byt_cr(dev, &bytcr);

• ret = is_byt_cr(pdev, &bytcr); if (!(ret < 0 || !bytcr)) {

• dev_info(dev, "Detected Baytrail-CR platform\n");
  

• /* override resource info */
  byt_rvp_platform_data.res_info = &bytcr_res_info;
  

  }

---

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

On 1/1/19 3:11 PM, Stephan Gerhold wrote:

On Mon, Dec 31, 2018 at 02:44:58PM -0600, Pierre-Louis Bossart wrote:

...

On 12/31/18 10:30 AM, Stephan Gerhold wrote:

...

On Mon, Dec 31, 2018 at 09:38:21AM -0600, Pierre-Louis Bossart wrote:

...

On 12/22/18 8:47 AM, Stephan Gerhold wrote:

...

Some devices detected as BYT-T by the PMIC-type based detection have only a single IRQ listed in the 80860F28 ACPI device. This causes -ENXIO later when attempting to get the IRQ at index 5. It turns out these devices behave more like BYT-CR devices, and using the IRQ at index 0 makes sound work correctly.

This patch adds a fallback for these devices to is_byt_cr(): If there is no IRQ resource at index 5, treating the device as BYT-T is guaranteed to fail later, so we can safely treat these devices as BYT-CR without breaking any working device.

Link: http://mailman.alsa-project.org/pipermail/alsa-devel/2018-December/143176.ht... Signed-off-by: Stephan Gerhold stephan@gerhold.net

---

Moved the "Detected Baytrail-CR platform" message to is_byt_cr() so we can log a different message if the fallback is used.

Tested this on my device as-is, and simulated a "normal" BYT-T and BYT-CR device (copied their IRQs to a custom DSDT).

sound/soc/intel/atom/sst/sst_acpi.c | 24 ++++++++++++++++++------
1 file changed, 18 insertions(+), 6 deletions(-)

diff --git a/sound/soc/intel/atom/sst/sst_acpi.c b/sound/soc/intel/atom/sst/sst_acpi.c index 3a95ebbfc45d..755a396121ff 100644 --- a/sound/soc/intel/atom/sst/sst_acpi.c +++ b/sound/soc/intel/atom/sst/sst_acpi.c @@ -255,10 +255,22 @@ static int is_byt(void) return status; } -static int is_byt_cr(struct device *dev, bool *bytcr) +static int is_byt_cr(struct platform_device *pdev, bool *bytcr) {

• struct device *dev = &pdev->dev; int status = 0;
• if (platform_get_resource(pdev, IORESOURCE_IRQ, 5) == NULL) {

• /*
  

•  * Some devices detected as BYT-T have only a single IRQ listed,
  

•  * causing platform_get_irq with index 5 to return -ENXIO.
  

•  * The correct IRQ in this case is at index 0, as used on BYT-CR.
  

•  */
  

• dev_info(dev, "Falling back to Baytrail-CR platform\n");
  

• *bytcr = true;
  

• return status;
  

• }

Isn't this going to bypass the PMIC-based detection on all BYT-CR devices? Maybe move this code as a fallback used when the PMIC-based detection isn't positive?

Except for the message that is logged, it does not really make a difference. PMIC-based detection is still used for most BYT-CR devices, which usually have 6 IRQs listed. For the few that have not, the end result (bytcr = true) is the same, even if they now use the fallback.

I mentioned this in a previous mail when I asked you which option you would prefer (see [1]). Since is_byt_cr() has multiple returns, I cannot just put it last without refactoring the entire method. (Which is something I wanted to avoid...)

Ah yes, but there was a side thread with Andy Shevchenko where we discussed that the initial return can be simplified since there are wrappers for iosf_mbi_available even when CONFIG_IOSF_MBI is not enabled. The code could be something like

diff --git a/sound/soc/intel/atom/sst/sst_acpi.c b/sound/soc/intel/atom/sst/sst_acpi.c index ac542535b9d5..58e389a64c6a 100644 --- a/sound/soc/intel/atom/sst/sst_acpi.c +++ b/sound/soc/intel/atom/sst/sst_acpi.c @@ -255,17 +255,16 @@ static int is_byt(void)         return status;  }

-static int is_byt_cr(struct device *dev, bool *bytcr) +static int is_byt_cr(struct platform_device *pdev, bool *bytcr)  { +       struct device *dev = &pdev->dev; +       u32 bios_status;         int status = 0;

-       if (IS_ENABLED(CONFIG_IOSF_MBI)) { -               u32 bios_status; +       if (!is_byt()) +               return status;

-               if (!is_byt() || !iosf_mbi_available()) { -                       /* bail silently */ -                       return status; -               } +       if (iosf_mbi_available()) {

status = iosf_mbi_read(BT_MBI_UNIT_PMC, /* 0x04 PUNIT */                                        MBI_REG_READ, /* 0x10 */ @@ -286,6 +285,20 @@ static int is_byt_cr(struct device *dev, bool *bytcr)         } else {                 dev_info(dev, "IOSF_MBI not enabled, no BYT-CR detection\n");         }

+       if (*bytcr == false && +           platform_get_resource(pdev, IORESOURCE_IRQ, 5) == NULL) { +               /* +                * Some devices detected as BYT-T have only a single IRQ listed, +                * causing platform_get_irq with index 5 to return -ENXIO. +                * The correct IRQ in this case is at index 0, as used on +                * BYT-CR. +                */ +               dev_info(dev, "Falling back to Baytrail-CR platform\n"); +               status = 0; +               *bytcr = true; +       }

return status;  }

Thanks! That looks fine to me. I will test it on my device and send a v2 shortly.

Speaking of simplifying is_byt_cr(): Especially its usage in

ret = is_byt_cr(pdev, &bytcr); if (!(ret < 0 || !bytcr)) { /* override resource info */ byt_rvp_platform_data.res_info = &bytcr_res_info; }

with the negated "or" has been rather confusing to read for me. In my opinion, it would be easier to understand as: if (ret == 0 && bytcr)

The return value (`ret`) is only used in this if statement. Since `bytcr` stays false when an error occurs in is_byt_cr(), we could further simplify this by returning the bool directly: if (is_byt_cr(pdev))

I like the suggested changes. This code evolved over time, IIRC the status was initially reporting some ACPI code but now a boolean will do. Good discussion, thanks!

Together with:

static bool is_byt_cr(struct platform_device *pdev) { struct device *dev = &pdev->dev;

if (!is_byt()) return false;

if (iosf_mbi_available()) { u32 bios_status; int status = iosf_mbi_read(BT_MBI_UNIT_PMC, /* 0x04 PUNIT */ MBI_REG_READ, /* 0x10 */ 0x006, /* BIOS_CONFIG */ &bios_status);

if (status) {
	dev_err(dev, "could not read PUNIT BIOS_CONFIG\n");
} else {
	/* bits 26:27 mirror PMIC options */
	bios_status = (bios_status >> 26) & 3;

	if ((bios_status == 1) || (bios_status == 3)) {
		dev_info(dev, "Detected Baytrail-CR platform\n");
		return true;
	} else {
		dev_info(dev, "BYT-CR not detected\n");
	}
}

} else { dev_info(dev, "IOSF_MBI not enabled, no BYT-CR detection\n"); }

if (platform_get_resource(pdev, IORESOURCE_IRQ, 5) == NULL) { /* * Some devices detected as BYT-T have only a single IRQ listed, * causing platform_get_irq with index 5 to return -ENXIO. * The correct IRQ in this case is at index 0, as used on BYT-CR. */ dev_info(dev, "Falling back to Baytrail-CR platform\n"); return true; }

return false; }

What do you think?

...

...

...

...

	if (IS_ENABLED(CONFIG_IOSF_MBI)) {
		u32 bios_status;

@@ -278,10 +290,12 @@ static int is_byt_cr(struct device *dev, bool *bytcr) /* bits 26:27 mirror PMIC options */ bios_status = (bios_status >> 26) & 3;

• 	if ((bios_status == 1) || (bios_status == 3))
  

• 	if ((bios_status == 1) || (bios_status == 3)) {
  

• 		dev_info(dev, "Detected Baytrail-CR platform\n");
  			*bytcr = true;
  

• 	else
  

• 	} else {
  			dev_info(dev, "BYT-CR not detected\n");
  

• 	}
  	}
  } else {
  	dev_info(dev, "IOSF_MBI not enabled, no BYT-CR detection\n");
  

@@ -333,10 +347,8 @@ static int sst_acpi_probe(struct platform_device *pdev) if (ret < 0) return ret;

• ret = is_byt_cr(dev, &bytcr);

• ret = is_byt_cr(pdev, &bytcr); if (!(ret < 0 || !bytcr)) {

• dev_info(dev, "Detected Baytrail-CR platform\n");
  

• 	/* override resource info */
  	byt_rvp_platform_data.res_info = &bytcr_res_info;
  }
  

---

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