Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH 04/14] mfd: arizona: Allow building arizona MFD-core as
 module
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
References: <20201227211232.117801-1-hdegoede@redhat.com>
 <20201227211232.117801-5-hdegoede@redhat.com>
 <20201229120039.GI9673@ediswmail.ad.cirrus.com>
From:   Hans de Goede <hdegoede@redhat.com>
Message-ID: <d80e90c1-143e-2655-b053-9361458f3df9@redhat.com>
Date:   Mon, 11 Jan 2021 20:12:27 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.4.0
MIME-Version: 1.0
In-Reply-To: <20201229120039.GI9673@ediswmail.ad.cirrus.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

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