| Message ID | 20201030063654.25877-3-brent.lu@intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add rt1015 support to CML boards | expand |
On 10/30/20 1:36 AM, Brent Lu wrote: > This DMI product family string of this board is "Google_Hatch" so the > DMI quirk will take place. However, this board is using rt1015 speaker > amp instead of max98357a specified in the quirk. Therefore, we need an > new DMI quirk for this board. Do you actually need a DMI quirk for this platform? the .driver_data below uses the exact same settings as what you would use with the generic solution based on ACPI IDs, see below. Wondering if patch1 would be enough? > > Signed-off-by: Brent Lu <brent.lu@intel.com> > --- > sound/soc/intel/boards/sof_rt5682.c | 14 ++++++++++++++ > 1 file changed, 14 insertions(+) > > diff --git a/sound/soc/intel/boards/sof_rt5682.c b/sound/soc/intel/boards/sof_rt5682.c > index 7701957e0eb7..dfcdf6d4b6c8 100644 > --- a/sound/soc/intel/boards/sof_rt5682.c > +++ b/sound/soc/intel/boards/sof_rt5682.c > @@ -100,6 +100,20 @@ static const struct dmi_system_id sof_rt5682_quirk_table[] = { > SOF_RT5682_MCLK_24MHZ | > SOF_RT5682_SSP_CODEC(1)), > }, > + { > + .callback = sof_rt5682_quirk_cb, > + .matches = { > + DMI_MATCH(DMI_SYS_VENDOR, "HP"), > + DMI_MATCH(DMI_PRODUCT_NAME, "Dooly"), > + }, > + .driver_data = (void *)(SOF_RT5682_MCLK_EN | > + SOF_RT5682_MCLK_24MHZ | > + SOF_RT5682_SSP_CODEC(0) | > + SOF_SPEAKER_AMP_PRESENT | > + SOF_RT1015_SPEAKER_AMP_PRESENT | > + SOF_RT1015_SPEAKER_AMP_100FS | > + SOF_RT5682_SSP_AMP(1)), > + }, is this really needed? it's the same as the .driver_data below: @@ -875,6 +901,16 @@ static const struct platform_device_id board_ids[] = { SOF_MAX98360A_SPEAKER_AMP_PRESENT | SOF_RT5682_SSP_AMP(1)), }, + { + .name = "cml_rt1015_rt5682", + .driver_data = (kernel_ulong_t)(SOF_RT5682_MCLK_EN | + SOF_RT5682_MCLK_24MHZ | + SOF_RT5682_SSP_CODEC(0) | + SOF_SPEAKER_AMP_PRESENT | + SOF_RT1015_SPEAKER_AMP_PRESENT | + SOF_RT1015_SPEAKER_AMP_100FS | + SOF_RT5682_SSP_AMP(1)), + },
, Brent Lu wrote: > > This DMI product family string of this board is "Google_Hatch" so the > > DMI quirk will take place. However, this board is using rt1015 speaker > > amp instead of max98357a specified in the quirk. Therefore, we need an > > new DMI quirk for this board. > > Do you actually need a DMI quirk for this platform? > > the .driver_data below uses the exact same settings as what you would use > with the generic solution based on ACPI IDs, see below. > > Wondering if patch1 would be enough? > Dooly has DMI family string " Google_Hatch" so the DMI quirk will overwrite the driver_data. I asked google but they prefer not removing this string so it seems to me that one extra DMI quirk is needed. { .callback = sof_rt5682_quirk_cb, .matches = { DMI_MATCH(DMI_PRODUCT_FAMILY, "Google_Hatch"), }, .driver_data = (void *)(SOF_RT5682_MCLK_EN | SOF_RT5682_MCLK_24MHZ | SOF_RT5682_SSP_CODEC(0) | SOF_SPEAKER_AMP_PRESENT | SOF_RT5682_SSP_AMP(1)), }, The other way is using acpi_dev_present() in probe to update the quirk with correct codec setting. Which one do you think is better? Regards, Brent
On 10/30/20 11:44 AM, Lu, Brent wrote: > , Brent Lu wrote: >>> This DMI product family string of this board is "Google_Hatch" so the >>> DMI quirk will take place. However, this board is using rt1015 speaker >>> amp instead of max98357a specified in the quirk. Therefore, we need an >>> new DMI quirk for this board. >> >> Do you actually need a DMI quirk for this platform? >> >> the .driver_data below uses the exact same settings as what you would use >> with the generic solution based on ACPI IDs, see below. >> >> Wondering if patch1 would be enough? >> > > Dooly has DMI family string " Google_Hatch" so the DMI quirk will overwrite the > driver_data. I asked google but they prefer not removing this string so it seems to > me that one extra DMI quirk is needed. I find this pretty funny. The PRODUCT_FAMILY was added to reduce the number of quirks, but of course there's a variant that has nothing to do with this 'FAMILY'. You should add a comment on this, to make sure this information remains in the code and we don't lose it during code cleanups. > > { > .callback = sof_rt5682_quirk_cb, > .matches = { > DMI_MATCH(DMI_PRODUCT_FAMILY, "Google_Hatch"), > }, > .driver_data = (void *)(SOF_RT5682_MCLK_EN | > SOF_RT5682_MCLK_24MHZ | > SOF_RT5682_SSP_CODEC(0) | > SOF_SPEAKER_AMP_PRESENT | > SOF_RT5682_SSP_AMP(1)), > }, > > The other way is using acpi_dev_present() in probe to update the quirk with correct > codec setting. Which one do you think is better? The DMI quirk you added is probably better for now, I don't know if the odds of getting things right with acpi_dev_present() are that high or if we are going to get even more variants on top of this variant (e.g. tweeter/booster cases...). If we get too many quirks we'll see later if we can simplify. So if you don't mind adding a comment on the 'Dooly' quirk in a v3 that series is good to go. Thank you!
On Fri, Oct 30, 2020 at 04:44:17PM +0000, Lu, Brent wrote: > , Brent Lu wrote: > > Wondering if patch1 would be enough? > Dooly has DMI family string " Google_Hatch" so the DMI quirk will overwrite the > driver_data. I asked google but they prefer not removing this string so it seems to > me that one extra DMI quirk is needed. I think this needs at least a comment otherwise someone might come along later and clean it up. > The other way is using acpi_dev_present() in probe to update the quirk with correct > codec setting. Which one do you think is better? I don't have a strong opinion either way.
diff --git a/sound/soc/intel/boards/sof_rt5682.c b/sound/soc/intel/boards/sof_rt5682.c index 7701957e0eb7..dfcdf6d4b6c8 100644 --- a/sound/soc/intel/boards/sof_rt5682.c +++ b/sound/soc/intel/boards/sof_rt5682.c @@ -100,6 +100,20 @@ static const struct dmi_system_id sof_rt5682_quirk_table[] = { SOF_RT5682_MCLK_24MHZ | SOF_RT5682_SSP_CODEC(1)), }, + { + .callback = sof_rt5682_quirk_cb, + .matches = { + DMI_MATCH(DMI_SYS_VENDOR, "HP"), + DMI_MATCH(DMI_PRODUCT_NAME, "Dooly"), + }, + .driver_data = (void *)(SOF_RT5682_MCLK_EN | + SOF_RT5682_MCLK_24MHZ | + SOF_RT5682_SSP_CODEC(0) | + SOF_SPEAKER_AMP_PRESENT | + SOF_RT1015_SPEAKER_AMP_PRESENT | + SOF_RT1015_SPEAKER_AMP_100FS | + SOF_RT5682_SSP_AMP(1)), + }, { .callback = sof_rt5682_quirk_cb, .matches = {
This DMI product family string of this board is "Google_Hatch" so the DMI quirk will take place. However, this board is using rt1015 speaker amp instead of max98357a specified in the quirk. Therefore, we need an new DMI quirk for this board. Signed-off-by: Brent Lu <brent.lu@intel.com> --- sound/soc/intel/boards/sof_rt5682.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+)