From: Hans de Goede <hdegoede@redhat.com> To: Charles Keepax <ckeepax@opensource.cirrus.com> Cc: Lee Jones <lee.jones@linaro.org>, MyungJoo Ham <myungjoo.ham@samsung.com>, Chanwoo Choi <cw00.choi@samsung.com>, Cezary Rojewski <cezary.rojewski@intel.com>, Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>, Liam Girdwood <liam.r.girdwood@linux.intel.com>, Jie Yang <yang.jie@linux.intel.com>, Mark Brown <broonie@kernel.org>, patches@opensource.cirrus.com, linux-kernel@vger.kernel.org, alsa-devel@alsa-project.org Subject: Re: [PATCH 04/14] mfd: arizona: Allow building arizona MFD-core as module Date: Mon, 11 Jan 2021 20:12:27 +0100 [thread overview] Message-ID: <d80e90c1-143e-2655-b053-9361458f3df9@redhat.com> (raw) In-Reply-To: <20201229120039.GI9673@ediswmail.ad.cirrus.com> Hi, First of all thank you for the review and for all your comments (also in the other part of the thread). On 12/29/20 1:00 PM, Charles Keepax wrote: > On Sun, Dec 27, 2020 at 10:12:22PM +0100, Hans de Goede wrote: >> There is no reason why the arizona core,irq and codec model specific >> regmap bits cannot be build as a module. All they do is export symbols >> which are used by the arizona-spi and/or arizona-i2c modules, which >> themselves can be built as module. >> >> Change the Kconfig and Makefile arizona bits so that the arizona MFD-core >> can be built as a module. >> >> This is especially useful on x86 platforms with a WM5102 codec, this >> allows the arizona MFD driver necessary for the WM5102 codec to be >> enabled in generic distro-kernels without growing the base kernel-image >> size. >> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> --- > > I think this patch might still cause some issues. ASoC has an > idiom where the machine driver does a select on the necessary > CODEC drivers. Select doesn't take care of dependencies etc. So I > believe if you build the machine driver as built in, it then > selects the CODEC as built in. If you have the MFD as a module > the build then fails due to the the CODEC calling some Arizona > functions. The rules for using select in Kconfig say that the Kconfig symbol doing the selecting must either have a "requires on or a "select" on any dependencies of the Kconfig symbol which it is selecting. Looking at the new machine-driver which this series adds: config SND_SOC_INTEL_BYTCR_WM5102_MACH tristate "Baytrail and Baytrail-CR with WM5102 codec" depends on SPI && ACPI depends on X86_INTEL_LPSS || COMPILE_TEST select SND_SOC_ACPI select SND_SOC_WM5102 help This adds support for ASoC machine driver for Intel(R) Baytrail and Baytrail-CR platforms with WM5102 audio codec. Say Y if you have such a device. If unsure select "N". I now see that I'm breaking that rule myself and this is missing a "depends on MFD_WM5102" I do see a "depends on MFD_WM5102" here: config SND_SOC_WM5102 tristate depends on MFD_WM5102 So I would expect some sort of error if I unselect MFD_WM5102 and indeed the following happens if I unselect MFD_WM5102: [hans@x1 linux]$ make oldconfig WARNING: unmet direct dependencies detected for SND_SOC_WM5102 Depends on [n]: SOUND [=m] && !UML && SND [=m] && SND_SOC [=m] && MFD_WM5102 [=n] Selected by [m]: - SND_SOC_INTEL_BYTCR_WM5102_MACH [=m] && SOUND [=m] && !UML && SND [=m] && SND_SOC [=m] && SND_SOC_INTEL_MACH [=y] && (SND_SST_ATOM_HIFI2_PLATFORM [=m] || SND_SOC_SOF_BAYTRAIL [=m]) && SPI [=y] && ACPI [=y] && (X86_INTEL_LPSS [=y] || COMPILE_TEST [=n]) Which of course is not supposed to happen and is caused by the config SND_SOC_INTEL_BYTCR_WM5102_MACH missing "depends on MFD_WM5102", which is my bad. But typing this (rubber ducky effect) has made me realized what the problem is, the codec Kconfig symbols depend on the: MFD_WM5102, MFD_WM5110, etc. Kconfig symbols. But those are booleans which enable/disable support for those codecs inside arizona-core.c. So you are right that this ("mfd: arizona: Allow building arizona MFD-core as module") could cause a scenario where the core is built as a module and the codec driver is builtin. But I do not think that the solution is to inline all the functions used from the codec drivers. The solution is to extend: config SND_SOC_WM5102 tristate depends on MFD_WM5102 to: config SND_SOC_WM5102 tristate depends on MFD_WM5102 depends on MFD_ARIZONA And to update machine drivers to still match the: "The Kconfig symbol doing the selecting must either have a "requires on" or a "select" on any dependencies of the Kconfig symbol which it is selecting." rule which I formulated above. (and idem for all the other codecs which use symbols from MFD_ARIZONA) > arizona_request_irq, arizona_free_irq, arizona_set_irq_wake > > On Madera we made the equivalents inline functions to avoid the > issue, the same should work here. > > include/linux/irqchip/irq-madera.h Yes that will work too, but I would prefer to solve what is a Kconfig issue with Kconfig changes. Note I will simply drop this patch for the next version of this series (*) since the whole jack rework thing we discussed is really orthogonal to this and that one will be interesting enough the review all by itself :) Regards, Hans *) I will resubmit a fixed version later after the other stuff has landed
WARNING: multiple messages have this Message-ID (diff)
From: Hans de Goede <hdegoede@redhat.com> To: Charles Keepax <ckeepax@opensource.cirrus.com> Cc: Cezary Rojewski <cezary.rojewski@intel.com>, alsa-devel@alsa-project.org, patches@opensource.cirrus.com, Mark Brown <broonie@kernel.org>, Jie Yang <yang.jie@linux.intel.com>, Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>, linux-kernel@vger.kernel.org, Liam Girdwood <liam.r.girdwood@linux.intel.com>, Chanwoo Choi <cw00.choi@samsung.com>, MyungJoo Ham <myungjoo.ham@samsung.com>, Lee Jones <lee.jones@linaro.org> Subject: Re: [PATCH 04/14] mfd: arizona: Allow building arizona MFD-core as module Date: Mon, 11 Jan 2021 20:12:27 +0100 [thread overview] Message-ID: <d80e90c1-143e-2655-b053-9361458f3df9@redhat.com> (raw) In-Reply-To: <20201229120039.GI9673@ediswmail.ad.cirrus.com> Hi, First of all thank you for the review and for all your comments (also in the other part of the thread). On 12/29/20 1:00 PM, Charles Keepax wrote: > On Sun, Dec 27, 2020 at 10:12:22PM +0100, Hans de Goede wrote: >> There is no reason why the arizona core,irq and codec model specific >> regmap bits cannot be build as a module. All they do is export symbols >> which are used by the arizona-spi and/or arizona-i2c modules, which >> themselves can be built as module. >> >> Change the Kconfig and Makefile arizona bits so that the arizona MFD-core >> can be built as a module. >> >> This is especially useful on x86 platforms with a WM5102 codec, this >> allows the arizona MFD driver necessary for the WM5102 codec to be >> enabled in generic distro-kernels without growing the base kernel-image >> size. >> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> --- > > I think this patch might still cause some issues. ASoC has an > idiom where the machine driver does a select on the necessary > CODEC drivers. Select doesn't take care of dependencies etc. So I > believe if you build the machine driver as built in, it then > selects the CODEC as built in. If you have the MFD as a module > the build then fails due to the the CODEC calling some Arizona > functions. The rules for using select in Kconfig say that the Kconfig symbol doing the selecting must either have a "requires on or a "select" on any dependencies of the Kconfig symbol which it is selecting. Looking at the new machine-driver which this series adds: config SND_SOC_INTEL_BYTCR_WM5102_MACH tristate "Baytrail and Baytrail-CR with WM5102 codec" depends on SPI && ACPI depends on X86_INTEL_LPSS || COMPILE_TEST select SND_SOC_ACPI select SND_SOC_WM5102 help This adds support for ASoC machine driver for Intel(R) Baytrail and Baytrail-CR platforms with WM5102 audio codec. Say Y if you have such a device. If unsure select "N". I now see that I'm breaking that rule myself and this is missing a "depends on MFD_WM5102" I do see a "depends on MFD_WM5102" here: config SND_SOC_WM5102 tristate depends on MFD_WM5102 So I would expect some sort of error if I unselect MFD_WM5102 and indeed the following happens if I unselect MFD_WM5102: [hans@x1 linux]$ make oldconfig WARNING: unmet direct dependencies detected for SND_SOC_WM5102 Depends on [n]: SOUND [=m] && !UML && SND [=m] && SND_SOC [=m] && MFD_WM5102 [=n] Selected by [m]: - SND_SOC_INTEL_BYTCR_WM5102_MACH [=m] && SOUND [=m] && !UML && SND [=m] && SND_SOC [=m] && SND_SOC_INTEL_MACH [=y] && (SND_SST_ATOM_HIFI2_PLATFORM [=m] || SND_SOC_SOF_BAYTRAIL [=m]) && SPI [=y] && ACPI [=y] && (X86_INTEL_LPSS [=y] || COMPILE_TEST [=n]) Which of course is not supposed to happen and is caused by the config SND_SOC_INTEL_BYTCR_WM5102_MACH missing "depends on MFD_WM5102", which is my bad. But typing this (rubber ducky effect) has made me realized what the problem is, the codec Kconfig symbols depend on the: MFD_WM5102, MFD_WM5110, etc. Kconfig symbols. But those are booleans which enable/disable support for those codecs inside arizona-core.c. So you are right that this ("mfd: arizona: Allow building arizona MFD-core as module") could cause a scenario where the core is built as a module and the codec driver is builtin. But I do not think that the solution is to inline all the functions used from the codec drivers. The solution is to extend: config SND_SOC_WM5102 tristate depends on MFD_WM5102 to: config SND_SOC_WM5102 tristate depends on MFD_WM5102 depends on MFD_ARIZONA And to update machine drivers to still match the: "The Kconfig symbol doing the selecting must either have a "requires on" or a "select" on any dependencies of the Kconfig symbol which it is selecting." rule which I formulated above. (and idem for all the other codecs which use symbols from MFD_ARIZONA) > arizona_request_irq, arizona_free_irq, arizona_set_irq_wake > > On Madera we made the equivalents inline functions to avoid the > issue, the same should work here. > > include/linux/irqchip/irq-madera.h Yes that will work too, but I would prefer to solve what is a Kconfig issue with Kconfig changes. Note I will simply drop this patch for the next version of this series (*) since the whole jack rework thing we discussed is really orthogonal to this and that one will be interesting enough the review all by itself :) Regards, Hans *) I will resubmit a fixed version later after the other stuff has landed
next prev parent reply other threads:[~2021-01-11 19:14 UTC|newest] Thread overview: 106+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-12-27 21:12 [PATCH 00/14] MFD/extcon/ASoC: Add support for Intel Bay Trail boards with WM5102 codec Hans de Goede 2020-12-27 21:12 ` Hans de Goede 2020-12-27 21:12 ` [PATCH 01/14] mfd: arizona: Add jack pointer to struct arizona Hans de Goede 2020-12-27 21:12 ` Hans de Goede 2020-12-28 12:21 ` Mark Brown 2020-12-28 12:21 ` Mark Brown 2020-12-28 13:16 ` Hans de Goede 2020-12-28 13:16 ` Hans de Goede 2020-12-28 16:28 ` Mark Brown 2020-12-28 16:28 ` Mark Brown 2020-12-29 13:06 ` Charles Keepax 2020-12-29 13:06 ` Charles Keepax 2020-12-29 13:57 ` Hans de Goede 2020-12-29 13:57 ` Hans de Goede 2020-12-29 15:06 ` Charles Keepax 2020-12-29 15:06 ` Charles Keepax 2020-12-29 15:15 ` Mark Brown 2020-12-29 15:15 ` Mark Brown 2020-12-29 15:40 ` Hans de Goede 2020-12-29 15:40 ` Hans de Goede 2020-12-29 16:51 ` Richard Fitzgerald 2020-12-29 16:51 ` Richard Fitzgerald 2020-12-30 11:04 ` Hans de Goede 2020-12-30 11:04 ` Hans de Goede 2020-12-30 11:23 ` Richard Fitzgerald 2020-12-30 11:23 ` Richard Fitzgerald 2020-12-30 12:01 ` Hans de Goede 2020-12-30 12:01 ` Hans de Goede 2020-12-30 13:16 ` Mark Brown 2020-12-30 13:16 ` Mark Brown 2020-12-29 16:43 ` Richard Fitzgerald 2020-12-29 16:43 ` Richard Fitzgerald 2020-12-29 15:08 ` Mark Brown 2020-12-29 15:08 ` Mark Brown 2020-12-29 15:33 ` Hans de Goede 2020-12-29 15:33 ` Hans de Goede 2020-12-30 13:38 ` Mark Brown 2020-12-30 13:38 ` Mark Brown 2021-01-01 13:24 ` Hans de Goede 2021-01-01 13:24 ` Hans de Goede 2020-12-27 21:12 ` [PATCH 02/14] mfd: arizona: Add MODULE_SOFTDEP("pre: arizona_ldo1") Hans de Goede 2020-12-27 21:12 ` Hans de Goede 2020-12-29 11:40 ` Charles Keepax 2020-12-29 11:40 ` Charles Keepax 2020-12-27 21:12 ` [PATCH 03/14] mfd: arizona: Add support for ACPI enumeration of WM5102 connected over SPI Hans de Goede 2020-12-27 21:12 ` Hans de Goede 2020-12-28 1:21 ` kernel test robot 2020-12-28 1:21 ` kernel test robot 2020-12-28 1:21 ` [RFC PATCH] mfd: arizona: ldoena_gpios can be static kernel test robot 2020-12-28 1:21 ` kernel test robot 2020-12-28 14:14 ` [PATCH 03/14] mfd: arizona: Add support for ACPI enumeration of WM5102 connected over SPI Andy Shevchenko 2020-12-28 14:14 ` Andy Shevchenko 2021-01-16 14:46 ` Hans de Goede 2021-01-16 14:46 ` Hans de Goede 2020-12-28 22:11 ` kernel test robot 2020-12-28 22:11 ` kernel test robot 2020-12-28 22:29 ` kernel test robot 2020-12-28 22:29 ` kernel test robot 2020-12-27 21:12 ` [PATCH 04/14] mfd: arizona: Allow building arizona MFD-core as module Hans de Goede 2020-12-27 21:12 ` Hans de Goede 2020-12-29 12:00 ` Charles Keepax 2020-12-29 12:00 ` Charles Keepax 2021-01-11 19:12 ` Hans de Goede [this message] 2021-01-11 19:12 ` Hans de Goede 2020-12-27 21:12 ` [PATCH 05/14] extcon: arizona: Fix some issues when HPDET IRQ fires after the jack has been unplugged Hans de Goede 2020-12-27 21:12 ` Hans de Goede 2020-12-27 21:12 ` [PATCH 06/14] extcon: arizona: Fix various races on driver unbind Hans de Goede 2020-12-27 21:12 ` Hans de Goede 2020-12-29 12:10 ` Charles Keepax 2020-12-29 12:10 ` Charles Keepax 2020-12-27 21:12 ` [PATCH 07/14] extcon: arizona: Fix modalias Hans de Goede 2020-12-27 21:12 ` Hans de Goede 2020-12-29 12:10 ` Charles Keepax 2020-12-29 12:10 ` Charles Keepax 2020-12-27 21:12 ` [PATCH 08/14] extcon: arizona: Fix flags parameter to the gpiod_get("wlf,micd-pol") call Hans de Goede 2020-12-27 21:12 ` [PATCH 08/14] extcon: arizona: Fix flags parameter to the gpiod_get("wlf, micd-pol") call Hans de Goede 2020-12-29 12:12 ` [PATCH 08/14] extcon: arizona: Fix flags parameter to the gpiod_get("wlf,micd-pol") call Charles Keepax 2020-12-29 12:12 ` Charles Keepax 2020-12-27 21:12 ` [PATCH 09/14] extcon: arizona: Add arizona_set_extcon_state() helper Hans de Goede 2020-12-27 21:12 ` Hans de Goede 2020-12-29 12:57 ` Charles Keepax 2020-12-29 12:57 ` Charles Keepax 2020-12-27 21:12 ` [PATCH 10/14] extcon: arizona: Also report jack state through snd_soc_jack_report() Hans de Goede 2020-12-27 21:12 ` Hans de Goede 2020-12-28 14:16 ` Andy Shevchenko 2020-12-28 14:16 ` Andy Shevchenko 2020-12-27 21:12 ` [PATCH 11/14] extcon: arizona: Use ASoC jack input-device when available Hans de Goede 2020-12-27 21:12 ` Hans de Goede 2020-12-27 21:12 ` [PATCH 12/14] ASoC: Intel: Add DMI quirk table to soc_intel_is_byt_cr() Hans de Goede 2020-12-27 21:12 ` Hans de Goede 2021-01-11 17:52 ` Pierre-Louis Bossart 2021-01-11 17:52 ` Pierre-Louis Bossart 2020-12-27 21:12 ` [PATCH 13/14] ASoC: Intel: bytcr_wm5102: Add machine driver for BYT/WM5102 Hans de Goede 2020-12-27 21:12 ` Hans de Goede 2020-12-29 13:58 ` Charles Keepax 2020-12-29 13:58 ` Charles Keepax 2021-01-11 17:54 ` Pierre-Louis Bossart 2021-01-11 17:54 ` Pierre-Louis Bossart 2021-01-16 16:49 ` Hans de Goede 2021-01-16 16:49 ` Hans de Goede 2020-12-27 21:12 ` [PATCH 14/14] ASoC: Intel: bytcr_wm5102: Add jack detect support Hans de Goede 2020-12-27 21:12 ` Hans de Goede 2020-12-28 14:19 ` [PATCH 00/14] MFD/extcon/ASoC: Add support for Intel Bay Trail boards with WM5102 codec Andy Shevchenko 2020-12-28 14:19 ` Andy Shevchenko 2021-01-11 18:54 ` Hans de Goede 2021-01-11 18:54 ` Hans de Goede
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=d80e90c1-143e-2655-b053-9361458f3df9@redhat.com \ --to=hdegoede@redhat.com \ --cc=alsa-devel@alsa-project.org \ --cc=broonie@kernel.org \ --cc=cezary.rojewski@intel.com \ --cc=ckeepax@opensource.cirrus.com \ --cc=cw00.choi@samsung.com \ --cc=lee.jones@linaro.org \ --cc=liam.r.girdwood@linux.intel.com \ --cc=linux-kernel@vger.kernel.org \ --cc=myungjoo.ham@samsung.com \ --cc=patches@opensource.cirrus.com \ --cc=pierre-louis.bossart@linux.intel.com \ --cc=yang.jie@linux.intel.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
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.