Re: [PATCH 00/14] ASoC: Intel/SOF: extend run-time driver selection to ACPI devices

[Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>]

> +static inline bool snd_soc_acpi_sof_parent(struct device *dev)
> +{
> +	return dev->parent && dev->parent->driver && dev->parent->driver->name &&
> +		!strcmp(dev->parent->driver->name, "sof-audio-acpi");
> +}
> +
> 
> -and-
> 
> +	/* set pm ops */
> +	if (sof_parent)
> +		pdev->dev.driver->pm = &snd_soc_pm_ops;
> +

The legacy Baytrail/Cherrytrail driver uses its own power management 
flow instead of the ASoC one. This patch does not cause the problem, it 
recognizes instead that this legacy driver cannot use the same pm ops.

I wish we didn't have to do this but there's just no alternative.

Dynamically assigning the .pm ops is not illegal and has been done in 
other drivers.

> -and-
> 
> +	/* set card and driver name */
> +	if (snd_soc_acpi_sof_parent(&pdev->dev)) {
> +		bdw_rt5650_card.name = SOF_CARD_NAME;
> +		bdw_rt5650_card.driver_name = SOF_DRIVER_NAME;
> +	} else {
> +		bdw_rt5650_card.name = CARD_NAME;
> +		bdw_rt5650_card.driver_name = DRIVER_NAME;
> +	}
> +

That is also intentional. We modified the card names based on Jaroslav's 
feedback, and this code change is just the mirror of what happened 
before with build-time code changes. Should we have changed the legacy 
card names? Maybe, but that might have added issues for users so we left 
the names untouched.

> pieces that are appearing several times throughout the series.
> ASoC is a framework on its own. It is by all means an extension to an
> older, more general ALSA framework. With every passing month SOF code
> found in /sound/soc/sof gets closer to becoming yet another framework,
> one that is placed on top of ASoC. Samples taken from this series
> augment this statement. If ASoC framework is missing means for its child
> drivers to do specific things, it's better to update the framework than
> creating yet another one.

There are no ASoC devices or drivers, we use platform devices/drivers. I 
don't see any need for an ASoC extension here.

> Explicit 'ifs' asking whether we're dealing with SOF or other solution
> is at best a code smell. I believe this is unnecessary complexity added
> to the code especially once you realize user needs to play with module
> parameters to switch between solutions. If we assume user is able to
> play with module parameters then why not simply make use of blacklist
> mechanism?

Been there, done that. We don't want to use either denylist of kernel 
parameters.

denylists create confusion for users, it's an endless stream of false 
errors and time lost in bug reports.

The use of the kernel parameter is ONLY for expert users who want to 
tinker with the system or debug issues, the average user should not have 
to play with either denylists or kernel parameters.

> And last but not least: intel-dsp-config is (as already stated) a mean
> for solving the HDA-runtime-driver-selection problem. Mixing it with
> ACPI devices is another layer of confusion.

Why? Who said it was PCI only? We already take care of DMIC, SoundWire, 
Google Chromebooks, open platforms, why not ACPI? It's just one API to 
be used when more than one driver can register to the same device.

>> Exactly. As Pierre-Louis mentions the Intel team does not have most
>> of the affected hardware. Since I've been working on making BYT and CHT
>> based devices work better with Linux as a side project for the last
>> couple of years I have been buying these kinda devices 2nd hand when ever
> ...
>> As Pierre-Louis mentioned I've already been working with him on getting
>> ready to move everything BYT/CHT related over to SOF. I've already been
>> testing SOF on various devices with mostly ok results so far.
>> But this is a process not a switch which we can simply throw.
> 
> Hans, thanks for sharing your concerns.
> 
> I'm afraid it's basically impossible to be fully prepared as you and
> Pierre pointed out. Even when speaking about catpt and BDW, we too
> didn't have all the available production stuff.
> 
> And thus I don't believe there will ever be a "good moment" to switch.
> Once internal validation confirms driver is stable it's better to switch
> entirely to the new solution with CI and devs on standby - ready to
> address any upcoming reports. Don't believe /atom/ has clean slide
> anyway given the patches and issues being posted from time to time
> related to said solution.

You refer to tests made by Intel when we are talking about community 
based tests here. We precisely do not have 'CI and devs on standby', the 
transition will be made by distributions themselves.

Besides, CI cannot test jack detection and all the flavors of 
microphone/speaker placements, which are the source of 99% of the issues.

> I understand you're here for atom-based products yet the patchset also
> touches on catpt aka hsw/bdw. While to my knowledge old /atom/ has no
> active CI running - so the switch is desirable - the same cannot be said
> about catpt. Because of that, I don't see any reason for appending any
> catpt-related changes here. Leave no place for confusion. One solution
> for one architecture that's recommended and maintained.

There is no confusion, the wording is explicit

"
SOF does not fully support Broadwell and has limitations related to
+	  DMA and suspend-resume, this is not a recommended option for
+	  distributions.
"

But there are niche users who prefer it for their own experiments, so 
what's the issue in making their life simpler?