[PATCH 1/2] ASoC: Intel: sst: Fallback to BYT-CR if IRQ 5 is missing

[PATCH 1/2] ASoC: Intel: sst: Fallback to BYT-CR if IRQ 5 is missing

[Stephan Gerhold]

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.html
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;
+	}
+
 	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;
 	}
-- 
2.20.1

[PATCH 2/2] ASoC: Intel: bytcr_rt5640: Add quirks for ASUS MeMO Pad 7 (ME176C)

[Stephan Gerhold]

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

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

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

Re: [PATCH 2/2] ASoC: Intel: bytcr_rt5640: Add quirks for ASUS MeMO Pad 7 (ME176C)

[Stephan Gerhold]

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
> 

Re: [PATCH 2/2] ASoC: Intel: bytcr_rt5640: Add quirks for ASUS MeMO Pad 7 (ME176C)

[Hans de Goede]

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.

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

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

Re: [PATCH 2/2] ASoC: Intel: bytcr_rt5640: Add quirks for ASUS MeMO Pad 7 (ME176C)

[Stephan Gerhold]

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

Re: [PATCH 2/2] ASoC: Intel: bytcr_rt5640: Add quirks for ASUS MeMO Pad 7 (ME176C)

[Hans de Goede]

Hi,

On 24-12-18 10:53, Stephan Gerhold wrote:
> 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?

No there is no need for that, I just wanted to make sure that it was not
necessary.

Regards,

Hans



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

Re: [PATCH 1/2] ASoC: Intel: sst: Fallback to BYT-CR if IRQ 5 is missing

[Pierre-Louis Bossart]

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.html
> 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?


>   	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;
>   	}

Re: [PATCH 1/2] ASoC: Intel: sst: Fallback to BYT-CR if IRQ 5 is missing

[Stephan Gerhold]

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

[1]: http://mailman.alsa-project.org/pipermail/alsa-devel/2018-December/143339.html

> 
> >   	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;
> >   	}

Re: [PATCH 1/2] ASoC: Intel: sst: Fallback to BYT-CR if IRQ 5 is missing

[Pierre-Louis Bossart]

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.html
>>> 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;
  }




>
> [1]: http://mailman.alsa-project.org/pipermail/alsa-devel/2018-December/143339.html
>
>>>    	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

Re: [PATCH 1/2] ASoC: Intel: sst: Fallback to BYT-CR if IRQ 5 is missing

[Stephan Gerhold]

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

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?

> 
> 
> > 
> > [1]: http://mailman.alsa-project.org/pipermail/alsa-devel/2018-December/143339.html
> > 
> > > >    	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

Re: [PATCH 1/2] ASoC: Intel: sst: Fallback to BYT-CR if IRQ 5 is missing

[Pierre-Louis Bossart]

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.html
>>>>> 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?
>
>>
>>> [1]: http://mailman.alsa-project.org/pipermail/alsa-devel/2018-December/143339.html
>>>
>>>>>     	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
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel