Re: [PATCH 2/2] ASoC: Intel: sof_rt5682: quirk auto detection

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

On 10/26/22 19:13, Lu, Brent wrote:
>>
>> This is a bit confusing: this quirk does not work for Volteer
>>
>> 	{
>> 		.callback = sof_rt5682_quirk_cb,
>> 		.matches = {
>> 			DMI_MATCH(DMI_PRODUCT_FAMILY,
>> "Google_Volteer"),
>> 			DMI_MATCH(DMI_OEM_STRING, "AUDIO-
>> MAX98373_ALC5682I_I2S_UP4"),
>> 		},
>> 		.driver_data = (void *)(SOF_RT5682_MCLK_EN |
>> 					SOF_RT5682_SSP_CODEC(0) |
>> 					SOF_SPEAKER_AMP_PRESENT |
>>
>> 	SOF_MAX98373_SPEAKER_AMP_PRESENT |
>> 					SOF_RT5682_SSP_AMP(2) |
>> 					SOF_RT5682_NUM_HDMIDEV(4)),
>> 	},
> 
> I checked Volteer reference kit, it should use SSP1 for amplifier. It seems to me 
> this quirk is for some customer variants which implements MAX98373 on SSP2.
> 
>>
>> Same for Brya and all usages of SSP_AMP(2)
>>
>>
> 
> It's a compromise that Google implements amplifiers on SSP2 on Brya so they can 
> connect SDW codec to SSP1 pins, but we asked customers to implement amplifiers 
> on SSP1 to reserve BT offload capability.

I appreciate what you are trying to suggest, and I am all for trying to
put this proliferation of quirks under control, but the autodetection
based on rules seems out-of-reach if we don't first cleanup the existing
quirks.

If "volteer" and "brya" mean different things to different people, and
reference and derivatives are not well identified, the odds of
regressions seem very high to me.

> 
>>> -	{
>>> -		.name = "adl_rt1019_rt5682s",
>>> -		.driver_data = (kernel_ulong_t)(SOF_RT5682_MCLK_EN |
>>> -					SOF_RT5682_SSP_CODEC(0) |
>>> -
>> 	SOF_RT5682S_HEADPHONE_CODEC_PRESENT |
>>
>> and HEADPHONE_CODEC_PRESENT is not handled either.
>>
> 
> Headphone type will be detected later in the sof_audio_probe().
> 
>>> -					SOF_SPEAKER_AMP_PRESENT |
>>> -					SOF_RT1019_SPEAKER_AMP_PRESENT
>> |
>>> -					SOF_RT5682_SSP_AMP(1) |
>>> -					SOF_RT5682_NUM_HDMIDEV(4)),
>>> -	},
>>
>> Overall I doubt that the SOC alone can tell you what the quirk is.
>>
>> Maybe it's a default to avoid repeats of the same baseline configuration, but
>> there's not much else that can be infer from an SOC definition in light of the
>> creativity of our hardware friends who routinely swap interfaces.
> 
> I'm thinking about using kernel module parameters for those boards which do not
> use default SSP port allocation. Not sure it's doable for machine driver module.

That's not a working solution IMHO, the kernel parameters should be used
by expert developers only for specific and short-term debug. It's not
possible to add a dependency on kernel parameters, that would prevent a
kernel update.