| Message ID | 20181222144708.121732-1-stephan@gerhold.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] ASoC: Intel: sst: Fallback to BYT-CR if IRQ 5 is missing | expand |
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; > }
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; > > }
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
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
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
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; }
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(-)