* [PATCH] ASoC: Intel: avs: Depend on CONFIG_ACPI @ 2022-04-26 11:54 Amadeusz Sławiński 2022-04-26 13:33 ` Pierre-Louis Bossart 0 siblings, 1 reply; 10+ messages in thread From: Amadeusz Sławiński @ 2022-04-26 11:54 UTC (permalink / raw) To: Liam Girdwood, Mark Brown Cc: Pierre-Louis Bossart, Cezary Rojewski, kernel test robot, alsa-devel, Takashi Iwai, Amadeusz Sławiński Apparently the assumption that driver can build without ACPI was too optimistic as avs selects SND_SOC_ACPI which requires working ACPI implementation. So depend on ACPI without COMPILE_TEST alternative. Fixes: 47a1886a610a ("ASoC: Intel: avs: Enable AVS driver only on x86 platforms") Reported-by: kernel test robot <lkp@intel.com> Reviewed-by: Cezary Rojewski <cezary.rojewski@intel.com> Signed-off-by: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com> --- sound/soc/intel/Kconfig | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/sound/soc/intel/Kconfig b/sound/soc/intel/Kconfig index be42c4eff165..9fdd6e32951f 100644 --- a/sound/soc/intel/Kconfig +++ b/sound/soc/intel/Kconfig @@ -211,8 +211,8 @@ config SND_SOC_INTEL_KEEMBAY config SND_SOC_INTEL_AVS tristate "Intel AVS driver" - depends on (X86 && ACPI) || COMPILE_TEST - depends on PCI + depends on X86 || COMPILE_TEST + depends on PCI && ACPI depends on COMMON_CLK select SND_SOC_ACPI select SND_SOC_TOPOLOGY -- 2.25.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] ASoC: Intel: avs: Depend on CONFIG_ACPI 2022-04-26 11:54 [PATCH] ASoC: Intel: avs: Depend on CONFIG_ACPI Amadeusz Sławiński @ 2022-04-26 13:33 ` Pierre-Louis Bossart 2022-04-26 17:08 ` Mark Brown 0 siblings, 1 reply; 10+ messages in thread From: Pierre-Louis Bossart @ 2022-04-26 13:33 UTC (permalink / raw) To: Amadeusz Sławiński, Liam Girdwood, Mark Brown Cc: kernel test robot, Cezary Rojewski, Takashi Iwai, alsa-devel On 4/26/22 06:54, Amadeusz Sławiński wrote: > Apparently the assumption that driver can build without ACPI was too > optimistic as avs selects SND_SOC_ACPI which requires working ACPI > implementation. So depend on ACPI without COMPILE_TEST alternative. > > Fixes: 47a1886a610a ("ASoC: Intel: avs: Enable AVS driver only on x86 platforms") > Reported-by: kernel test robot <lkp@intel.com> > Reviewed-by: Cezary Rojewski <cezary.rojewski@intel.com> > Signed-off-by: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com> > --- > sound/soc/intel/Kconfig | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/sound/soc/intel/Kconfig b/sound/soc/intel/Kconfig > index be42c4eff165..9fdd6e32951f 100644 > --- a/sound/soc/intel/Kconfig > +++ b/sound/soc/intel/Kconfig > @@ -211,8 +211,8 @@ config SND_SOC_INTEL_KEEMBAY > > config SND_SOC_INTEL_AVS > tristate "Intel AVS driver" > - depends on (X86 && ACPI) || COMPILE_TEST > - depends on PCI > + depends on X86 || COMPILE_TEST > + depends on PCI && ACPI > depends on COMMON_CLK > select SND_SOC_ACPI select SND_SOC_ACPI if ACPI should work, that's what we do for SOF. > select SND_SOC_TOPOLOGY ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] ASoC: Intel: avs: Depend on CONFIG_ACPI 2022-04-26 13:33 ` Pierre-Louis Bossart @ 2022-04-26 17:08 ` Mark Brown 2022-04-26 18:07 ` Cezary Rojewski 0 siblings, 1 reply; 10+ messages in thread From: Mark Brown @ 2022-04-26 17:08 UTC (permalink / raw) To: Pierre-Louis Bossart Cc: Cezary Rojewski, kernel test robot, alsa-devel, Liam Girdwood, Takashi Iwai, Amadeusz Sławiński [-- Attachment #1: Type: text/plain, Size: 569 bytes --] On Tue, Apr 26, 2022 at 08:33:57AM -0500, Pierre-Louis Bossart wrote: > On 4/26/22 06:54, Amadeusz Sławiński wrote: > > - depends on (X86 && ACPI) || COMPILE_TEST > > - depends on PCI > > + depends on X86 || COMPILE_TEST > > + depends on PCI && ACPI > > depends on COMMON_CLK > > select SND_SOC_ACPI > select SND_SOC_ACPI if ACPI should work, that's what we do for SOF. That also looks sensible - it's probably good if both users do the same thing here so can someone send a patch for that please? Sorry for not noticing that earlier Amadeusz. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] ASoC: Intel: avs: Depend on CONFIG_ACPI 2022-04-26 17:08 ` Mark Brown @ 2022-04-26 18:07 ` Cezary Rojewski 2022-04-26 18:25 ` Mark Brown 0 siblings, 1 reply; 10+ messages in thread From: Cezary Rojewski @ 2022-04-26 18:07 UTC (permalink / raw) To: Mark Brown, Pierre-Louis Bossart Cc: alsa-devel, kernel test robot, Liam Girdwood, Takashi Iwai, Amadeusz Sławiński On 2022-04-26 7:08 PM, Mark Brown wrote: > On Tue, Apr 26, 2022 at 08:33:57AM -0500, Pierre-Louis Bossart wrote: >> On 4/26/22 06:54, Amadeusz Sławiński wrote: > >>> - depends on (X86 && ACPI) || COMPILE_TEST >>> - depends on PCI >>> + depends on X86 || COMPILE_TEST >>> + depends on PCI && ACPI >>> depends on COMMON_CLK >>> select SND_SOC_ACPI > >> select SND_SOC_ACPI if ACPI should work, that's what we do for SOF. > > That also looks sensible - it's probably good if both users do the same > thing here so can someone send a patch for that please? Sorry for not > noticing that earlier Amadeusz. Hello, Would you like the change proposed by Pierre to be part of this patch or a separate one? Also, I assume the above code does not change, just: s/select SND_SOC_ACPI/select SND_SOC_ACPI if ACPI/ is applied on top of what's already in the patch. Is my deduction correct? Regards, Czarek ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] ASoC: Intel: avs: Depend on CONFIG_ACPI 2022-04-26 18:07 ` Cezary Rojewski @ 2022-04-26 18:25 ` Mark Brown 2022-04-26 18:26 ` Pierre-Louis Bossart 0 siblings, 1 reply; 10+ messages in thread From: Mark Brown @ 2022-04-26 18:25 UTC (permalink / raw) To: Cezary Rojewski Cc: Pierre-Louis Bossart, alsa-devel, kernel test robot, Takashi Iwai, Liam Girdwood, Amadeusz Sławiński [-- Attachment #1: Type: text/plain, Size: 496 bytes --] On Tue, Apr 26, 2022 at 08:07:22PM +0200, Cezary Rojewski wrote: > Would you like the change proposed by Pierre to be part of this patch or a > separate one? Either is fine. > Also, I assume the above code does not change, just: > s/select SND_SOC_ACPI/select SND_SOC_ACPI if ACPI/ > is applied on top of what's already in the patch. Is my deduction correct? AIUI the current patch and associated hard dependency on ACPI could be dropped but I've not checked or thought too deeply about it. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] ASoC: Intel: avs: Depend on CONFIG_ACPI 2022-04-26 18:25 ` Mark Brown @ 2022-04-26 18:26 ` Pierre-Louis Bossart 2022-04-26 18:38 ` Mark Brown 2022-04-26 19:07 ` Amadeusz Sławiński 0 siblings, 2 replies; 10+ messages in thread From: Pierre-Louis Bossart @ 2022-04-26 18:26 UTC (permalink / raw) To: Mark Brown, Cezary Rojewski Cc: alsa-devel, kernel test robot, Liam Girdwood, Takashi Iwai, Amadeusz Sławiński On 4/26/22 13:25, Mark Brown wrote: > On Tue, Apr 26, 2022 at 08:07:22PM +0200, Cezary Rojewski wrote: > >> Would you like the change proposed by Pierre to be part of this patch or a >> separate one? > > Either is fine. > >> Also, I assume the above code does not change, just: >> s/select SND_SOC_ACPI/select SND_SOC_ACPI if ACPI/ > >> is applied on top of what's already in the patch. Is my deduction correct? > > AIUI the current patch and associated hard dependency on ACPI could be > dropped but I've not checked or thought too deeply about it my suggestion was the following: depends on PCI depends on COMMON_CLK select SND_SOC_ACPI if ACPI ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] ASoC: Intel: avs: Depend on CONFIG_ACPI 2022-04-26 18:26 ` Pierre-Louis Bossart @ 2022-04-26 18:38 ` Mark Brown 2022-04-26 18:42 ` Pierre-Louis Bossart 2022-04-26 19:07 ` Amadeusz Sławiński 1 sibling, 1 reply; 10+ messages in thread From: Mark Brown @ 2022-04-26 18:38 UTC (permalink / raw) To: Pierre-Louis Bossart Cc: Cezary Rojewski, kernel test robot, alsa-devel, Liam Girdwood, Takashi Iwai, Amadeusz Sławiński [-- Attachment #1: Type: text/plain, Size: 302 bytes --] On Tue, Apr 26, 2022 at 01:26:08PM -0500, Pierre-Louis Bossart wrote: > my suggestion was the following: > depends on PCI > depends on COMMON_CLK > select SND_SOC_ACPI if ACPI The X86 || COMPILE_TEST that's there currently is also sensible given that this can only actually be used on x86 hardware. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] ASoC: Intel: avs: Depend on CONFIG_ACPI 2022-04-26 18:38 ` Mark Brown @ 2022-04-26 18:42 ` Pierre-Louis Bossart 0 siblings, 0 replies; 10+ messages in thread From: Pierre-Louis Bossart @ 2022-04-26 18:42 UTC (permalink / raw) To: Mark Brown Cc: Cezary Rojewski, kernel test robot, alsa-devel, Liam Girdwood, Takashi Iwai, Amadeusz Sławiński On 4/26/22 13:38, Mark Brown wrote: > On Tue, Apr 26, 2022 at 01:26:08PM -0500, Pierre-Louis Bossart wrote: > >> my suggestion was the following: > >> depends on PCI >> depends on COMMON_CLK >> select SND_SOC_ACPI if ACPI > > The X86 || COMPILE_TEST that's there currently is also sensible given > that this can only actually be used on x86 hardware. right, I only commented on the ACPI dependency. X86 || COMPILE_TEST is also what we use for SOF. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] ASoC: Intel: avs: Depend on CONFIG_ACPI 2022-04-26 18:26 ` Pierre-Louis Bossart 2022-04-26 18:38 ` Mark Brown @ 2022-04-26 19:07 ` Amadeusz Sławiński 2022-04-26 19:16 ` Pierre-Louis Bossart 1 sibling, 1 reply; 10+ messages in thread From: Amadeusz Sławiński @ 2022-04-26 19:07 UTC (permalink / raw) To: Pierre-Louis Bossart, Mark Brown, Cezary Rojewski Cc: alsa-devel, Takashi Iwai, kernel test robot, Liam Girdwood On 4/26/2022 8:26 PM, Pierre-Louis Bossart wrote: > > > On 4/26/22 13:25, Mark Brown wrote: >> On Tue, Apr 26, 2022 at 08:07:22PM +0200, Cezary Rojewski wrote: >> >>> Would you like the change proposed by Pierre to be part of this patch or a >>> separate one? >> >> Either is fine. >> >>> Also, I assume the above code does not change, just: >>> s/select SND_SOC_ACPI/select SND_SOC_ACPI if ACPI/ >> >>> is applied on top of what's already in the patch. Is my deduction correct? >> >> AIUI the current patch and associated hard dependency on ACPI could be >> dropped but I've not checked or thought too deeply about it > > > my suggestion was the following: > > depends on PCI > depends on COMMON_CLK > select SND_SOC_ACPI if ACPI Thanks for suggestion, I tested with it, but it doesn't work as we access fields from struct nhlt_specific_cfg which is available only if ACPI support is enabled: sound/soc/intel/avs/path.c: In function ‘avs_copier_create’: sound/soc/intel/avs/path.c:186:17: error: ‘struct nhlt_specific_cfg’ has no member named ‘caps’ 186 | data = ep_blob->caps; | ^~ sound/soc/intel/avs/path.c:187:22: error: ‘struct nhlt_specific_cfg’ has no member named ‘size’ 187 | data_size = ep_blob->size; | ^~ sound/soc/intel/avs/path.c:210:17: error: ‘struct nhlt_specific_cfg’ has no member named ‘caps’ 210 | data = ep_blob->caps; | ^~ sound/soc/intel/avs/path.c:211:22: error: ‘struct nhlt_specific_cfg’ has no member named ‘size’ 211 | data_size = ep_blob->size; | ^~ Now there are few ways this can be solved: 1. Move the struct nhlt_specific_cfg definition out of define check... in include/sound/intel-nhlt.h, but that seems kind of sloppy to me, even if it makes sense. 2. NHLT is only needed for I2S and DMIC configuration, so we could in theory add #ifdef ACPI around configuration for those endpoints in sound/soc/intel/avs/path.c - but it introduces potential problems on x86 platforms with users doing some weird config. 3. Depend directly on ACPI (current patch) causing NHLT support to be always enabled. And then apply change suggested by Pierre when we rewrite NHLT support to use NHLT structures in include/acpi/actbl2.h header as it as far as I can tell exposes all structures unconditionally. I've discussed with Cezary and we are leaning towards option 3. but if there is any other preference we can prepare patch for that option. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] ASoC: Intel: avs: Depend on CONFIG_ACPI 2022-04-26 19:07 ` Amadeusz Sławiński @ 2022-04-26 19:16 ` Pierre-Louis Bossart 0 siblings, 0 replies; 10+ messages in thread From: Pierre-Louis Bossart @ 2022-04-26 19:16 UTC (permalink / raw) To: Amadeusz Sławiński, Mark Brown, Cezary Rojewski Cc: Liam Girdwood, alsa-devel, Takashi Iwai, kernel test robot On 4/26/22 14:07, Amadeusz Sławiński wrote: > On 4/26/2022 8:26 PM, Pierre-Louis Bossart wrote: >> >> >> On 4/26/22 13:25, Mark Brown wrote: >>> On Tue, Apr 26, 2022 at 08:07:22PM +0200, Cezary Rojewski wrote: >>> >>>> Would you like the change proposed by Pierre to be part of this patch or a >>>> separate one? >>> >>> Either is fine. >>> >>>> Also, I assume the above code does not change, just: >>>> s/select SND_SOC_ACPI/select SND_SOC_ACPI if ACPI/ >>> >>>> is applied on top of what's already in the patch. Is my deduction correct? >>> >>> AIUI the current patch and associated hard dependency on ACPI could be >>> dropped but I've not checked or thought too deeply about it >> >> >> my suggestion was the following: >> >> depends on PCI >> depends on COMMON_CLK >> select SND_SOC_ACPI if ACPI > > Thanks for suggestion, I tested with it, but it doesn't work as we access fields from struct nhlt_specific_cfg which is available only if ACPI support is enabled: > > sound/soc/intel/avs/path.c: In function ‘avs_copier_create’: > sound/soc/intel/avs/path.c:186:17: error: ‘struct nhlt_specific_cfg’ has no member named ‘caps’ > 186 | data = ep_blob->caps; > | ^~ > sound/soc/intel/avs/path.c:187:22: error: ‘struct nhlt_specific_cfg’ has no member named ‘size’ > 187 | data_size = ep_blob->size; > | ^~ > sound/soc/intel/avs/path.c:210:17: error: ‘struct nhlt_specific_cfg’ has no member named ‘caps’ > 210 | data = ep_blob->caps; > | ^~ > sound/soc/intel/avs/path.c:211:22: error: ‘struct nhlt_specific_cfg’ has no member named ‘size’ > 211 | data_size = ep_blob->size; > | ^~ > > Now there are few ways this can be solved: > 1. Move the struct nhlt_specific_cfg definition out of define check... in include/sound/intel-nhlt.h, but that seems kind of sloppy to me, even if it makes sense. That would be my preference. There's no real reason to hide those definitions that are pure C, and only have some enums exposed and others not. diff --git a/include/sound/intel-nhlt.h b/include/sound/intel-nhlt.h index 6fb2d5e378fdd..340584d9515f4 100644 --- a/include/sound/intel-nhlt.h +++ b/include/sound/intel-nhlt.h @@ -25,8 +25,6 @@ enum nhlt_device_type { NHLT_DEVICE_INVALID }; -#if IS_ENABLED(CONFIG_ACPI) && IS_ENABLED(CONFIG_SND_INTEL_NHLT) - struct wav_fmt { u16 fmt_tag; u16 channels; @@ -126,6 +124,8 @@ enum { NHLT_MIC_ARRAY_VENDOR_DEFINED = 0xf, }; +#if IS_ENABLED(CONFIG_ACPI) && IS_ENABLED(CONFIG_SND_INTEL_NHLT) + struct nhlt_acpi_table *intel_nhlt_init(struct device *dev); void intel_nhlt_free(struct nhlt_acpi_table *addr); > 2. NHLT is only needed for I2S and DMIC configuration, so we could in theory add #ifdef ACPI around configuration for those endpoints in sound/soc/intel/avs/path.c - but it introduces potential problems on x86 platforms with users doing some weird config. > 3. Depend directly on ACPI (current patch) causing NHLT support to be always enabled. And then apply change suggested by Pierre when we rewrite NHLT support to use NHLT structures in include/acpi/actbl2.h header as it as far as I can tell exposes all structures unconditionally. > > I've discussed with Cezary and we are leaning towards option 3. but if there is any other preference we can prepare patch for that option. ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2022-04-26 19:18 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-04-26 11:54 [PATCH] ASoC: Intel: avs: Depend on CONFIG_ACPI Amadeusz Sławiński 2022-04-26 13:33 ` Pierre-Louis Bossart 2022-04-26 17:08 ` Mark Brown 2022-04-26 18:07 ` Cezary Rojewski 2022-04-26 18:25 ` Mark Brown 2022-04-26 18:26 ` Pierre-Louis Bossart 2022-04-26 18:38 ` Mark Brown 2022-04-26 18:42 ` Pierre-Louis Bossart 2022-04-26 19:07 ` Amadeusz Sławiński 2022-04-26 19:16 ` Pierre-Louis Bossart
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.