[PATCH 0/2] soundwire: fix Kconfig select/depend issues

[PATCH 0/2] soundwire: fix Kconfig select/depend issues

[Pierre-Louis Bossart]

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(-)

-- 
2.17.1

[PATCH 1/2] regmap: soundwire: fix Kconfig select/depend issue

[Pierre-Louis Bossart]

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
-- 
2.17.1

Re: [PATCH 1/2] regmap: soundwire: fix Kconfig select/depend issue

[Takashi Iwai]

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
> 

Re: [PATCH 1/2] regmap: soundwire: fix Kconfig select/depend issue

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 536 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [alsa-devel] [PATCH 1/2] regmap: soundwire: fix Kconfig select/depend issue

[Pierre-Louis Bossart]

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

Re: [alsa-devel] [PATCH 1/2] regmap: soundwire: fix Kconfig select/depend issue

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 388 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [alsa-devel] [PATCH 1/2] regmap: soundwire: fix Kconfig select/depend issue

[Takashi Iwai]

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

Re: [alsa-devel] [PATCH 1/2] regmap: soundwire: fix Kconfig select/depend issue

[Vinod Koul]

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
-- 
~Vinod

[PATCH 2/2] soundwire: fix SOUNDWIRE_BUS option

[Pierre-Louis Bossart]

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
-- 
2.17.1

Re: [PATCH 0/2] soundwire: fix Kconfig select/depend issues

[Srinivas Kandagatla]

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(-)
> 

Re: [alsa-devel] [PATCH 0/2] soundwire: fix Kconfig select/depend issues

[Pierre-Louis Bossart]

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

Re: [alsa-devel] [PATCH 0/2] soundwire: fix Kconfig select/depend issues

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 2042 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [alsa-devel] [PATCH 0/2] soundwire: fix Kconfig select/depend issues

[Pierre-Louis Bossart]

>>> 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.

Re: [PATCH 0/2] soundwire: fix Kconfig select/depend issues

[Vinod Koul]

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

-- 
~Vinod

Re: [alsa-devel] [PATCH 0/2] soundwire: fix Kconfig select/depend issues

[Pierre-Louis Bossart]

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'