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

Pierre-Louis Bossart pierre-louis.bossart at linux.intel.com
Wed Jan 2 17:39:43 CET 2019


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 at 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 at alsa-project.org
>>> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel at alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel


More information about the Alsa-devel mailing list