[alsa-devel] [PATCH 0/2] soundwire: fix Kconfig select/depend issues
0-day/Kbuild starts complaining about missed module dependencies and compilation issues. Since codecs and soc drivers need to be compilable independently, let's fix this using the following model:
SOUNDWIRE_INTEL ---- select ----------- | v REGMAP_SOUNDWIRE --- select ---> SOUNDWIRE_BUS
Pierre-Louis Bossart (2): regmap: soundwire: fix Kconfig select/depend issue soundwire: fix SOUNDWIRE_BUS option
drivers/base/regmap/Kconfig | 3 ++- drivers/soundwire/Kconfig | 1 - 2 files changed, 2 insertions(+), 2 deletions(-)
The mechanism should be
config CODEC_XYX_SDW depends on SOUNDWIRE select REGMAP_SOUNDWIRE
config REGMAP_SOUNDWIRE depends on SOUNDWIRE select SOUNDWIRE_BUS
SOUNDWIRE_BUS can be independently selected by the SOC driver. The SOC driver should not know or care about REGMAP_SOUNDWIRE.
Fixes: 7c22ce6e2184 ('03fc8746f7915b5a391d8227f7e1') Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- drivers/base/regmap/Kconfig | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/base/regmap/Kconfig b/drivers/base/regmap/Kconfig index 6ad5ef48b61e..4e422afe3c0d 100644 --- a/drivers/base/regmap/Kconfig +++ b/drivers/base/regmap/Kconfig @@ -44,7 +44,8 @@ config REGMAP_IRQ
config REGMAP_SOUNDWIRE tristate - depends on SOUNDWIRE_BUS + depends on SOUNDWIRE + select SOUNDWIRE_BUS
config REGMAP_SCCB tristate
On Thu, 11 Apr 2019 21:28:13 +0200, Pierre-Louis Bossart wrote:
The mechanism should be
config CODEC_XYX_SDW depends on SOUNDWIRE select REGMAP_SOUNDWIRE
config REGMAP_SOUNDWIRE depends on SOUNDWIRE select SOUNDWIRE_BUS
To be noted, in general you can't do put both depends-on and select. The select always wins. So the depends-on in REGMAP_SOUNDWIRE is more or less moot.
thanks,
Takashi
SOUNDWIRE_BUS can be independently selected by the SOC driver. The SOC driver should not know or care about REGMAP_SOUNDWIRE.
Fixes: 7c22ce6e2184 ('03fc8746f7915b5a391d8227f7e1') Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com
drivers/base/regmap/Kconfig | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/base/regmap/Kconfig b/drivers/base/regmap/Kconfig index 6ad5ef48b61e..4e422afe3c0d 100644 --- a/drivers/base/regmap/Kconfig +++ b/drivers/base/regmap/Kconfig @@ -44,7 +44,8 @@ config REGMAP_IRQ
config REGMAP_SOUNDWIRE tristate
- depends on SOUNDWIRE_BUS
- depends on SOUNDWIRE
- select SOUNDWIRE_BUS
config REGMAP_SCCB tristate -- 2.17.1
On Fri, Apr 12, 2019 at 10:38:19AM +0200, Takashi Iwai wrote:
Pierre-Louis Bossart wrote:
config REGMAP_SOUNDWIRE depends on SOUNDWIRE select SOUNDWIRE_BUS
To be noted, in general you can't do put both depends-on and select. The select always wins. So the depends-on in REGMAP_SOUNDWIRE is more or less moot.
To be clear the issue is that the dependencies of selected symbols just get ignored (IIRC their selects too). It can be useful for documentation but it does get a bit confusing sometimes.
Thanks for the reviews
The mechanism should be
config CODEC_XYX_SDW depends on SOUNDWIRE select REGMAP_SOUNDWIRE
config REGMAP_SOUNDWIRE depends on SOUNDWIRE select SOUNDWIRE_BUS
To be noted, in general you can't do put both depends-on and select. The select always wins. So the depends-on in REGMAP_SOUNDWIRE is more or less moot.
ok. To double-check, the example below would be legit, yes?
config CODEC_XYX_SDW tristate "XYZ SDW Codec" depends on SOUNDWIRE select REGMAP_SOUNDWIRE
config REGMAP_SOUNDWIRE select SOUNDWIRE_BUS
it'd follow the typical pattern seen in sound/soc/codecs/Kconfig:
config SND_SOC_CS4265 tristate "Cirrus Logic CS4265 CODEC" depends on I2C select REGMAP_I2C
Thanks -Pierre
On Fri, Apr 12, 2019 at 09:07:41AM -0500, Pierre-Louis Bossart wrote:
config CODEC_XYX_SDW tristate "XYZ SDW Codec" depends on SOUNDWIRE select REGMAP_SOUNDWIRE
That looks good.
config REGMAP_SOUNDWIRE select SOUNDWIRE_BUS
it'd follow the typical pattern seen in sound/soc/codecs/Kconfig:
IIRC the select here won't actually do anything.
On Fri, 12 Apr 2019 16:18:41 +0200, Mark Brown wrote:
On Fri, Apr 12, 2019 at 09:07:41AM -0500, Pierre-Louis Bossart wrote:
config CODEC_XYX_SDW tristate "XYZ SDW Codec" depends on SOUNDWIRE select REGMAP_SOUNDWIRE
That looks good.
config REGMAP_SOUNDWIRE select SOUNDWIRE_BUS
it'd follow the typical pattern seen in sound/soc/codecs/Kconfig:
IIRC the select here won't actually do anything.
I thought it'd do select SOUNDWIRE_BUS. The depends-on here would be ignored instead.
Takashi
On 12-04-19, 16:21, Takashi Iwai wrote:
On Fri, 12 Apr 2019 16:18:41 +0200, Mark Brown wrote:
On Fri, Apr 12, 2019 at 09:07:41AM -0500, Pierre-Louis Bossart wrote:
config CODEC_XYX_SDW tristate "XYZ SDW Codec" depends on SOUNDWIRE select REGMAP_SOUNDWIRE
That looks good.
config REGMAP_SOUNDWIRE select SOUNDWIRE_BUS
it'd follow the typical pattern seen in sound/soc/codecs/Kconfig:
IIRC the select here won't actually do anything.
I thought it'd do select SOUNDWIRE_BUS. The depends-on here would be ignored instead.
yeah this all is bit complicated and should not be so. As Srini pointed out, we have two symbols, SOUNDWIRE as a menuconfig item which enables the subsystem and then SOUNDWIRE_BUS which compiles in the code.
Unfortunately I dont seem to recall the advantages of this approach so it might be easier to drop SOUNDWIRE_BUS and then let codecs do select REGMAP_SOUNDWIRE and depends on SOUNDWIRE
Thanks
SOUNDWIRE_BUS can be selected independendly by the SOC driver (e.g. SOUNDWIRE_INTEL) or the codec driver (via REGMAP_SOUNDWIRE).
Remove wrong-way link between SOUNDWIRE_BUS and REGMAP_SOUNDWIRE
Fixes: 6c49b32d3c09 ('soundwire: select REGMAP_SOUNDWIRE') Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- drivers/soundwire/Kconfig | 1 - 1 file changed, 1 deletion(-)
diff --git a/drivers/soundwire/Kconfig b/drivers/soundwire/Kconfig index 84876a74874f..d382d80d2fe1 100644 --- a/drivers/soundwire/Kconfig +++ b/drivers/soundwire/Kconfig @@ -18,7 +18,6 @@ comment "SoundWire Devices"
config SOUNDWIRE_BUS tristate - select REGMAP_SOUNDWIRE
config SOUNDWIRE_CADENCE tristate
On 11/04/2019 20:28, Pierre-Louis Bossart wrote:
0-day/Kbuild starts complaining about missed module dependencies and compilation issues. Since codecs and soc drivers need to be compilable independently, let's fix this using the following model:
SOUNDWIRE_INTEL ---- select ----------- | v REGMAP_SOUNDWIRE --- select ---> SOUNDWIRE_BUS
Last time when I looked at this, Kconfig symbols SOUNDWIRE_BUS and SOUNDWIRE looked totally redundant and bit over done.
Removing SOUNDWIRE_BUS Kconfig did clean it up and made it bit more align with others. regarding REGMAP_SOUNDWIRE, It should be selected by the codec/soundwire slave drivers isn't it?
------------------------------><----------------------------------- diff --git a/drivers/base/regmap/Kconfig b/drivers/base/regmap/Kconfig index 6ad5ef48b61e..8cd2ac650b50 100644 --- a/drivers/base/regmap/Kconfig +++ b/drivers/base/regmap/Kconfig @@ -44,7 +44,7 @@ config REGMAP_IRQ
config REGMAP_SOUNDWIRE tristate - depends on SOUNDWIRE_BUS + depends on SOUNDWIRE
config REGMAP_SCCB tristate diff --git a/drivers/soundwire/Kconfig b/drivers/soundwire/Kconfig index 5c8677e20b49..bcee4b101c8a 100644 --- a/drivers/soundwire/Kconfig +++ b/drivers/soundwire/Kconfig @@ -3,7 +3,7 @@ #
menuconfig SOUNDWIRE - bool "SoundWire support" + tristate "SoundWire support" ---help--- SoundWire is a 2-Pin interface with data and clock line ratified by the MIPI Alliance. SoundWire is used for transporting data @@ -16,17 +16,12 @@ if SOUNDWIRE
comment "SoundWire Devices"
-config SOUNDWIRE_BUS - tristate - select REGMAP_SOUNDWIRE - config SOUNDWIRE_CADENCE tristate
config SOUNDWIRE_INTEL tristate "Intel SoundWire Master driver" select SOUNDWIRE_CADENCE - select SOUNDWIRE_BUS depends on X86 && ACPI && SND_SOC ---help--- SoundWire Intel Master driver. diff --git a/drivers/soundwire/Makefile b/drivers/soundwire/Makefile index f6fbb21d4e84..9d70d20ca3ce 100644 --- a/drivers/soundwire/Makefile +++ b/drivers/soundwire/Makefile @@ -1,10 +1,11 @@ # # Makefile for soundwire core #
#Bus Objs -soundwire-bus-objs := bus_type.o bus.o slave.o mipi_disco.o stream.o -obj-$(CONFIG_SOUNDWIRE_BUS) += soundwire-bus.o +soundwire-objs= bus_type.o bus.o slave.o mipi_disco.o stream.o +obj-$(CONFIG_SOUNDWIRE) += soundwire.o
#Cadence Objs soundwire-cadence-objs := cadence_master.o ------------------------------><-----------------------------------
--srini
Pierre-Louis Bossart (2): regmap: soundwire: fix Kconfig select/depend issue soundwire: fix SOUNDWIRE_BUS option
drivers/base/regmap/Kconfig | 3 ++- drivers/soundwire/Kconfig | 1 - 2 files changed, 2 insertions(+), 2 deletions(-)
On 4/12/19 5:06 AM, Srinivas Kandagatla wrote:
On 11/04/2019 20:28, Pierre-Louis Bossart wrote:
0-day/Kbuild starts complaining about missed module dependencies and compilation issues. Since codecs and soc drivers need to be compilable independently, let's fix this using the following model:
SOUNDWIRE_INTEL ---- select ----------- | v REGMAP_SOUNDWIRE --- select ---> SOUNDWIRE_BUS
Last time when I looked at this, Kconfig symbols SOUNDWIRE_BUS and SOUNDWIRE looked totally redundant and bit over done.
Removing SOUNDWIRE_BUS Kconfig did clean it up and made it bit more align with others
Good point, but no. This is intentional and follows the Kconfig pattern pattern described by Takashi at https://lkml.org/lkml/2017/11/17/47
yes, this SOUNDWIRE is overkill for now, but let's assume there is a second non-intel implementation (which I understand as very likely given all the threads on ARM64 support). In that case you'd really want a top-level selector option that has no actual compilation impact - not used in any Makefile or code - but enables the sub-options and let users/distros select the platforms they care about.
SOUNDWIRE_BUS is really the lowest-common denominator that will be used by all drivers at the end.
regarding REGMAP_SOUNDWIRE, It should be selected by the codec/soundwire slave drivers isn't it?
yes, that was the intent. Thanks -Pierre
On Fri, Apr 12, 2019 at 09:17:35AM -0500, Pierre-Louis Bossart wrote:
On 4/12/19 5:06 AM, Srinivas Kandagatla wrote:
On 11/04/2019 20:28, Pierre-Louis Bossart wrote:
0-day/Kbuild starts complaining about missed module dependencies and compilation issues. Since codecs and soc drivers need to be compilable independently, let's fix this using the following model:
SOUNDWIRE_INTEL ---- select ----------- | v REGMAP_SOUNDWIRE --- select ---> SOUNDWIRE_BUS
Last time when I looked at this, Kconfig symbols SOUNDWIRE_BUS and SOUNDWIRE looked totally redundant and bit over done.
Removing SOUNDWIRE_BUS Kconfig did clean it up and made it bit more align with others
Good point, but no. This is intentional and follows the Kconfig pattern pattern described by Takashi at https://lkml.org/lkml/2017/11/17/47
yes, this SOUNDWIRE is overkill for now, but let's assume there is a second non-intel implementation (which I understand as very likely given all the threads on ARM64 support). In that case you'd really want a top-level selector option that has no actual compilation impact - not used in any Makefile or code - but enables the sub-options and let users/distros select the platforms they care about.
I don't understand what you're saying here - what is the intended difference between SOUNDWIRE and SOUNDWIRE_BUS? Having the separate option for _INTEL for your controller makes sense but otherwise the normal pattern for a bus would be that you'd have the root config option for the bus (which would enable the core code for the bus) and then everything else is hidden behind that.
SOUNDWIRE_BUS is really the lowest-common denominator that will be used by all drivers at the end.
regarding REGMAP_SOUNDWIRE, It should be selected by the codec/soundwire slave drivers isn't it?
yes, that was the intent. Thanks -Pierre
Removing SOUNDWIRE_BUS Kconfig did clean it up and made it bit more align with others
Good point, but no. This is intentional and follows the Kconfig pattern pattern described by Takashi at https://lkml.org/lkml/2017/11/17/47
yes, this SOUNDWIRE is overkill for now, but let's assume there is a second non-intel implementation (which I understand as very likely given all the threads on ARM64 support). In that case you'd really want a top-level selector option that has no actual compilation impact - not used in any Makefile or code - but enables the sub-options and let users/distros select the platforms they care about.
I don't understand what you're saying here - what is the intended difference between SOUNDWIRE and SOUNDWIRE_BUS? Having the separate option for _INTEL for your controller makes sense but otherwise the normal pattern for a bus would be that you'd have the root config option for the bus (which would enable the core code for the bus) and then everything else is hidden behind that.
I was thinking of this pattern:
config SOUNDWIRE bool "SoundWire support"
if SOUNDWIRE
config SOUNDWIRE_INTEL tristate "SoundWire for Intel" select SOUNDWIRE_BUS
config SOUNDWIRE_XYZ_ARM64 tristate "SoundWire for XYZ platform" select SOUNDWIRE_BUS
config SOUNDWIRE_BUS tristate
endif
One could argue that SOUNDWIRE could select SOUNDWIRE_BUS directly, or merge the two, but then you could have a configuration where the bus is included with no actual users. Not to mention that the intent of the top-level selector is typically to have no impact on compilation as recommended by Linus.
On 11-04-19, 14:28, Pierre-Louis Bossart wrote:
0-day/Kbuild starts complaining about missed module dependencies and compilation issues. Since codecs and soc drivers need to be compilable independently, let's fix this using the following model:
I have not seen a build report on this one, is this some internal Intel report or on upstream tree?
SOUNDWIRE_INTEL ---- select ----------- | v REGMAP_SOUNDWIRE --- select ---> SOUNDWIRE_BUS
Pierre-Louis Bossart (2): regmap: soundwire: fix Kconfig select/depend issue soundwire: fix SOUNDWIRE_BUS option
drivers/base/regmap/Kconfig | 3 ++- drivers/soundwire/Kconfig | 1 - 2 files changed, 2 insertions(+), 2 deletions(-)
-- 2.17.1
On 4/14/19 5:13 AM, Vinod Koul wrote:
On 11-04-19, 14:28, Pierre-Louis Bossart wrote:
0-day/Kbuild starts complaining about missed module dependencies and compilation issues. Since codecs and soc drivers need to be compilable independently, let's fix this using the following model:
I have not seen a build report on this one, is this some internal Intel report or on upstream tree?
0day/kbuild generated this on my work tree:
tree: https://github.com/plbossart/sound fix/soundwire-dev head: d59242993bef53c301c0badda162c6bb3a5f15cb commit: 24fe4a80b2435446c52197cb828f4475aa817c79 [22/25] skl change for sdw config: x86_64-randconfig-l1-04110213 (attached as .config) compiler: gcc-5 (Debian 5.5.0-3) 5.4.1 20171010 reproduce: git checkout 24fe4a80b2435446c52197cb828f4475aa817c79 # save the attached .config to linux build tree make ARCH=x86_64
All errors (new ones prefixed by >>):
sound/soc/intel/skylake/skl-pcm.o: In function `skl_platform_pcm_prepare':
sound/soc/intel/skylake/skl-pcm.c:1364: undefined reference to `sdw_prepare_stream'
sound/soc/intel/skylake/skl-pcm.o: In function `skl_platform_pcm_hw_free':
sound/soc/intel/skylake/skl-pcm.c:1339: undefined reference to `sdw_deprepare_stream'
sound/soc/intel/skylake/skl-pcm.o: In function `skl_platform_shutdown':
sound/soc/intel/skylake/skl-pcm.c:1314: undefined reference to `sdw_release_stream'
sound/soc/intel/skylake/skl-pcm.o: In function `skl_sdw_stream_trigger':
sound/soc/intel/skylake/skl-pcm.c:1266: undefined reference to `sdw_enable_stream' sound/soc/intel/skylake/skl-pcm.c:1278: undefined reference to `sdw_disable_stream'
sound/soc/intel/skylake/skl-pcm.o: In function `skl_platform_open':
sound/soc/intel/skylake/skl-pcm.c:1136: undefined reference to `sdw_alloc_stream'
sound/soc/intel/skylake/skl-pcm.c:1163: undefined reference to `sdw_release_stream' sound/soc/intel/skylake/cnl-sst.o: In function `skl_sdw_init':
sound/soc/intel/skylake/cnl-sst.c:514: undefined reference to `sdw_intel_init'
participants (5)
-
Mark Brown -
Pierre-Louis Bossart -
Srinivas Kandagatla -
Takashi Iwai -
Vinod Koul