All of lore.kernel.org
 help / color / mirror / Atom feed
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


  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: link
Be 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.