Re: [PATCH] ASoC: SOF: Intel: avoid reverse module dependency

[Takashi Iwai <tiwai@suse.de>]

On Mon, 11 Jan 2021 20:54:17 +0100,
Pierre-Louis Bossart wrote:
> 
> 
> 
> On 1/5/21 1:07 PM, Arnd Bergmann wrote:
> > From: Arnd Bergmann <arnd@arndb.de>
> >
> > The SOF-ACPI driver is backwards from the normal Linux model, it has a
> > generic driver that knows about all the specific drivers, as opposed to
> > having hardware specific drivers that link against a common framework.
> >
> > This requires ugly Kconfig magic and leads to missed dependencies as
> > seen in this link error:
> >
> > arm-linux-gnueabi-ld: sound/soc/sof/sof-pci-dev.o: in function `sof_acpi_probe':
> > sof-pci-dev.c:(.text+0x1c): undefined reference to `snd_intel_dsp_driver_probe'
> >
> > Change it to use the normal probe order of starting with a specific
> > device in a driver, turning the sof-acpi-dev.c driver into a library.
> 
> Thanks Arnd for reporting all this, much appreciated.
> 
> The initial design was that we would have one generic platform_driver
> (ACPI) and one generic PCI driver that would deal with all known IDs,
> with descriptors that would point ops and callbacks defined in
> device-specific drivers. It's how all Intel drivers worked so far,
> from HDaudio to Atom/SST and Skylake.
> 
> It's not that ugly, but to Arnd's point we do have a lot of #if
> IS_ENABLED at the top level with a larger and larger table of IDs,
> along with Kconfig magic indeed to propagate constraints from
> top-level to device-specific drivers. The error with DSP_CONFIG comes
> from the fact that this never belonged at the top-level, or should
> have been conditionally invoked, as noted by Takashi.
> 
> That said, the initial design which dates from 2017 can be revisited
> now that we start having quite a few platforms and more coming. What
> Arnd suggests isn't without merits, it would indeed turn the generic
> code into generic helpers, and have all the platform IDs maintained in
> device-specific drivers. It's a more distributed/scalable solution,
> the only minor drawback I see is that it would require multiple
> instances of the 'platform_driver' and 'pci_driver' structures.
> 
> I would also want to keep the top-level selection so that ACPI/PCI/DT
> modules can be disabled in one shot, that would mean an additional
> change to the Makefiles since e.g.
> obj-$(CONFIG_SND_SOC_SOF_ACPI) += snd-sof-acpi.o
> would need to be set somehow.
> 
> Since this is going to be a really invasive change, and past
> experience shows that mucking with Kconfigs will invariably raise a
> number of broken corner cases, if there is support from
> Mark/Takashi/Jaroslav on this idea, we should first test it in the SOF
> tree so that we get a good test coverage and don't break too many eggs
> in Mark's tree. We would also need to concurrently change our CI
> scripts which are dependent on module names.

I'm in favor of the way Arnd proposed.  It's more straightforward and
less code.

If you find the number of modules or the too much cutting out being
problematic, you can create a module snd-sof-intel-acpi and
snd-sof-intel-pci containing the driver table entries for all Intel
devices, too.  In the case, you'll still need some conditional calls
of intel-dsp-config there, but it's a good step for reducing the
Kconfig complexity.

> Also maybe in a first pass we can remove the compilation error with
> IS_REACHABLE and in a second pass do more invasive surgery?

Agreed, we'd like to keep less changes for 5.11 for now.


thanks,

Takashi