Message-ID: <a70760ea-50e9-229e-9f89-d56136e1872d@linux.intel.com>
Date:   Thu, 16 Nov 2017 14:27:48 -0600
From:   Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>
To:     Takashi Iwai <tiwai@...e.de>
Cc:     Linus Torvalds <torvalds@...ux-foundation.org>,
        Vinod Koul <vinod.koul@...el.com>,
        Mark Brown <broonie@...nel.org>,
        Liam Girdwood <liam.r.girdwood@...ux.intel.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        alsa-devel@...a-project.org
Subject: Re: [GIT PULL] sound updates for 4.15-rc1



On 11/16/2017 09:10 AM, Takashi Iwai wrote:
> On Thu, 16 Nov 2017 15:55:35 +0100,
> Pierre-Louis Bossart wrote:
>> On 11/15/17 1:34 AM, Takashi Iwai wrote:
>>> [Adding more people and alsa-devel to Cc]
>>>
>>> On Wed, 15 Nov 2017 03:40:09 +0100,
>>> Linus Torvalds wrote:
>>>> On Tue, Nov 14, 2017 at 6:51 AM, Takashi Iwai <tiwai@...e.de> wrote:
>>>>> please pull sound updates for v4.15-rc1 from:
>>>> Hmm. Making "oldconfig" on my laptop with this, my
>>>> SND_SOC_INTEL_SKYLAKE went away.
>>>>
>>>> And the reason seems to be that new SND_SOC_INTEL_SST_TOPLEVEL config option.
>>>>
>>>> Which has no help associated with it.
>>>>
>>>> This is not a friendly thing to do to people. It basically breaks
>>>> existing setups for no documented reason, and with no explanation.
>>>>
>>>> Please fix the config situation. At the very least, add documentation.
>>> Sorry about that.  I saw Vinod already submitted a patch to add the
>>> help text to CONFIG_SND_SOC_INTEL_SST_TOPLEVEL, so the least fix
>>> should go in soon.
>>>
>>> But now looking at these changes, I noticed a few things, too:
>>>
>>> - With the introduction of SND_SOC_INTEL_SST_TOPLEVEL, keeping
>>>     SND_SOC_INTEL_COMMON and SND_SOC_INTEL_MACH individually doesn't
>>>     make much sense.  They can be dropped and replaced with
>>>     SND_SOC_INTEL_SST_TOPLEVEL as a further cleanup.
>>>
>>> - ... or, make SND_SOC_INTEL_SST_TOPLEVEL=y as default, if this is
>>>     considered to be a top-level filter config (like the network vendor
>>>     kconfig items).  In that case, the reverse-selection of
>>>     SND_SOC_INTEL_COMMON and SND_SOC_INTEL_MACH should be avoided, but
>>>     they should be selected from the actual drivers instead.
>> This level was introduced as a shortcut to help select the platform
>> drivers associated with the closed-source audio firmware.
>> There will be a follow-up set of patches coming soon, and we'll need
>> to have the corresponding top-level selector for the drivers handling
>> the open-source audio firmware. Please don't make too many assumptions
>> on this SND_SOC_INTEL_SST_TOPLEVEL, it is not going to be true for
>> everyone moving forward.
> OK, but *before* moving to that direction, please fix the current mess
> at first.  If CONFIG_SND_SOC_INTEL_SST_TOPLEVEL is the entry to filter
> the Intel ASoC drivers, it should be the one with default=y and
> doesn't select / enable others as itself.  OTOH, if some explicit
> selection by user becomes mandatory while it wasn't, it's already a
> bad step.
ok, so i will accept that there was a gap in the validation and that I 
shouldn't have broken Linus' audio. My bad, working on it.
The sound/soc/intel/boards/Kconfig file can indeed be simplified, I 
tried to avoid changing too many things at the same time, but I can 
provide a patch if people want a nicer solution now if the if/endif and 
default m. I'll send a proposal later today. I guess I'll play with 
defconfig, oldconfig and olddefconfig for a quick sanity check.

That said, it looks like we are not aligned on what the patches tried to do:
SND_SOC_INTEL_COMMON contains the matching code used by both SST and SOF 
drivers and it needs to remain as a separate module
SND_SOC_INTEL_MACH is a way to make the machine drivers appear, both 
from SST and SOF drivers (we want common machine drivers)
the two are completely unrelated.

I also don't see why we are talking about a reverse selection, the flow 
is the same top-down selection in both cases

config SND_SOC_INTEL_SST_TOPLEVEL
     tristate "Intel ASoC SST drivers"
     select SND_SOC_INTEL_MACH
     select SND_SOC_INTEL_COMMON

config SND_SOC_SOF_INTEL
     tristate "SOF support for Intel audio DSPs"
     depends on SND_SOC_SOF << probably more dependencies needed with 
randconfig.
     select SND_SOC_INTEL_COMMON
     select SND_SOC_INTEL_MACH

then you select the platform drivers needed and the relevant machine 
drivers are visible.

I am also not sure if there should be hard-coded 'y' default. If you use 
make defconfig, there is currently nothing selected at all for any of 
the ASoC drivers, should we instead use 'default m' for all the sensible 
drivers (i.e. default n for deprecated stuff) and automagically select 
them when X86+ASoC is true?
>> If you want look at the Kconfig setup, the latest code cherry-picked
>> on top of v4.14 is here
>> https://github.com/plbossart/sound/tree/backport/intel-audio-stable-v4.14
> Thanks.  But I see only the very same content in sound/soc/intel/*...?
sorry, please look here:
https://github.com/plbossart/sound/tree/topic/sof-1.0-dev-rc2
>
>
> Takashi
>
>> the menuconfig options are still in device drivers/sound/ASoC but now
>> you have SST and SOF menus.
>>
>>>
>>> And I believe there are a few more possible cleanups / fixes in the
>>> messy Intel ASoC Kconfigs.  For example, SND_SOC_INTEL_SST is almost
>>> always set.  The only exception is via SND_SST_ATOM_HIFI2_PLATFORM.
>>> But all machine drivers using Atom Hifi2 do set SND_SST_IPC_ACPI,
>>> which also requires SND_SOC_INTEL_SST.
>>>
>>> Further looking at this, we see that the only entry that does *not*
>>> require SND_SOC_INTEL_SST is the case with SND_MFLD_MACHINE in
>>> sound/soc/intel/boards.  And now more interesting part -- there is no
>>> corresponding entry in Makefile.  That is, this kconfig is effectively
>>> dead!  The source code mfld_machine.c exists, but it's just a place
>>> holder now.  The code was supposed to be integrated into atom
>>> directory by the commit b97169da0699, but it seems forgotten to be
>>> updated.
>>>
>>> Hmm...
>>>
>>>
>>> Takashi
>>>

Powered by blists - more mailing lists