Messages in this thread | | | Subject | Re: [PATCH] ASoC: Intel: sof_rt5682: Add rt1015p speaker amp support | From | Pierre-Louis Bossart <> | Date | Mon, 15 Mar 2021 09:26:26 -0500 |
| |
I am not a big fan of the code partition you've selected.
> +void sof_rt1015p_set_share_en_spk(void) > +{ > + /* Two amps share one en pin so there is only one device in acpi > + * table > + */ > + rt1015p_quirk |= RT1015P_SHARE_EN_SPK; > +}
This is a function now used in the machine driver, see below [1] This adds a new interface between machine and realtek common code, which we are trying to move to a module with well-defined APIs.
> +void sof_rt1015p_dai_link(struct snd_soc_dai_link *link) > +{ > + link->codecs = rt1015p_dai_link_components; > + if (rt1015p_quirk & RT1015P_SHARE_EN_SPK) > + link->num_codecs = 1; > + else > + link->num_codecs = ARRAY_SIZE(rt1015p_dai_link_components); > + link->init = rt1015p_init; > + link->ops = &rt1015p_ops; > +} > + > +void sof_rt1015p_codec_conf(struct snd_soc_card *card) > +{ > + if (rt1015p_quirk & RT1015P_SHARE_EN_SPK) > + return; > + > + card->codec_conf = rt1015p_codec_confs; > + card->num_configs = ARRAY_SIZE(rt1015p_codec_confs); > +}
could we not handle this quirk inside one of the two dai_link or codec_conf functions above? The machine driver does not care about this RT1015P_SHARE_EN_SPK quirk, it's only used in those two functions so should be set in this scope. There's no need to make it visible to the machine driver, is there?
> + /* setup amp quirk if any */ > + if (sof_rt5682_quirk & SOF_RT1015P_SPEAKER_AMP_PRESENT) { > + /* There may be only one device instance if two amps > + * sharing one en pin > + */ > + if (!acpi_dev_present("RTL1015", "1", -1)) { > + dev_dbg(&pdev->dev, "Device %s not exist\n", > + RT1015P_DEV1_NAME); > + sof_rt1015p_set_share_en_spk(); > + } > + } > +
[1]
I see no problem using the _UID (Unique ID) to check if a second amplifier is present or not. This seems to follow the ACPI spec Section 6.1.12, as long as "the _UID must be unique across all devices with either a common _HID or _CID"
> + else if (sof_rt5682_quirk & SOF_RT1015P_SPEAKER_AMP_PRESENT) > + sof_rt1015p_codec_conf(&sof_audio_card_rt5682);
| |