| Message ID | 20211007133516.1464655-2-brent.lu@intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Multiple headphone codec driver support | expand |
On 2021-10-07 3:35 PM, Brent Lu wrote: ... > > +static bool snd_soc_acpi_id_present(struct snd_soc_acpi_mach *machine) > +{ > + struct snd_soc_acpi_codecs *comp_ids = machine->comp_ids; > + int i; > + > + if (machine->id[0]) { > + if (acpi_dev_present(machine->id, NULL, -1)) > + return true; > + } > + > + if (comp_ids) { > + for (i = 0; i < comp_ids->num_codecs; i++) { > + if (acpi_dev_present(comp_ids->codecs[i], NULL, -1)) > + return true; > + } > + } > + > + return false; > +} In cover letter you mention: "- can use 'comp_ids' field alone to enumerate driver" which leads me to an opinion that field 'id' should be removed, entirely. With 'comp_ids' added, 'id' is basically rendered optional/redundant. > + > struct snd_soc_acpi_mach * > snd_soc_acpi_find_machine(struct snd_soc_acpi_mach *machines) > { > struct snd_soc_acpi_mach *mach; > struct snd_soc_acpi_mach *mach_alt; > > - for (mach = machines; mach->id[0]; mach++) { > - if (acpi_dev_present(mach->id, NULL, -1)) { > + for (mach = machines; mach->id[0] || mach->comp_ids; mach++) { Such loops are hard to maintain i.e. 'comp_ids' acts here like a flex array that follows 'id'. Removal of 'id' field and streamlining code to only use 'comp_ids' should make this loop more intuitive. > + if (snd_soc_acpi_id_present(mach)) { > if (mach->machine_quirk) { > mach_alt = mach->machine_quirk(mach); > if (!mach_alt) >
>> struct snd_soc_acpi_mach * >> snd_soc_acpi_find_machine(struct snd_soc_acpi_mach *machines) >> { >> struct snd_soc_acpi_mach *mach; >> struct snd_soc_acpi_mach *mach_alt; >> - for (mach = machines; mach->id[0]; mach++) { >> - if (acpi_dev_present(mach->id, NULL, -1)) { >> + for (mach = machines; mach->id[0] || mach->comp_ids; mach++) { > > Such loops are hard to maintain i.e. 'comp_ids' acts here like a flex > array that follows 'id'. Removal of 'id' field and streamlining code to > only use 'comp_ids' should make this loop more intuitive. Changing all the tables adds more noise IMHO. There are 15 files and about 100 ids. This patch provides an opportunity to reduce duplication, that's good, but let's leave all the existing unique table entries alone, shall we?
On 2021-10-07 7:27 PM, Pierre-Louis Bossart wrote: > >>> struct snd_soc_acpi_mach * >>> snd_soc_acpi_find_machine(struct snd_soc_acpi_mach *machines) >>> { >>> struct snd_soc_acpi_mach *mach; >>> struct snd_soc_acpi_mach *mach_alt; >>> - for (mach = machines; mach->id[0]; mach++) { >>> - if (acpi_dev_present(mach->id, NULL, -1)) { >>> + for (mach = machines; mach->id[0] || mach->comp_ids; mach++) { >> >> Such loops are hard to maintain i.e. 'comp_ids' acts here like a flex >> array that follows 'id'. Removal of 'id' field and streamlining code to >> only use 'comp_ids' should make this loop more intuitive. > > Changing all the tables adds more noise IMHO. There are 15 files and > about 100 ids. > > This patch provides an opportunity to reduce duplication, that's good, > but let's leave all the existing unique table entries alone, shall we? > Well, we could have mentioned files updated in the follow up patches i.e. treat this patch as a 'preparation step'. In the long run, having two places for id initialization will cost us more than updating all those files. Have no problem with leaving current patch as is if the end goal is removal of 'id' field. In some future series I guess.. Czarek
diff --git a/include/sound/soc-acpi.h b/include/sound/soc-acpi.h index 2f3fa385c092..43cf520f145e 100644 --- a/include/sound/soc-acpi.h +++ b/include/sound/soc-acpi.h @@ -129,6 +129,7 @@ struct snd_soc_acpi_link_adr { * all firmware/topology related fields. * * @id: ACPI ID (usually the codec's) used to find a matching machine driver. + * @comp_ids: a list of audio codecs also used to find a matching machine driver. * @link_mask: describes required board layout, e.g. for SoundWire. * @links: array of link _ADR descriptors, null terminated. * @drv_name: machine driver name @@ -146,6 +147,7 @@ struct snd_soc_acpi_link_adr { /* Descriptor for SST ASoC machine driver */ struct snd_soc_acpi_mach { const u8 id[ACPI_ID_LEN]; + struct snd_soc_acpi_codecs *comp_ids; const u32 link_mask; const struct snd_soc_acpi_link_adr *links; const char *drv_name; diff --git a/sound/soc/soc-acpi.c b/sound/soc/soc-acpi.c index 395229bf5c51..aca7c2020567 100644 --- a/sound/soc/soc-acpi.c +++ b/sound/soc/soc-acpi.c @@ -8,14 +8,34 @@ #include <linux/module.h> #include <sound/soc-acpi.h> +static bool snd_soc_acpi_id_present(struct snd_soc_acpi_mach *machine) +{ + struct snd_soc_acpi_codecs *comp_ids = machine->comp_ids; + int i; + + if (machine->id[0]) { + if (acpi_dev_present(machine->id, NULL, -1)) + return true; + } + + if (comp_ids) { + for (i = 0; i < comp_ids->num_codecs; i++) { + if (acpi_dev_present(comp_ids->codecs[i], NULL, -1)) + return true; + } + } + + return false; +} + struct snd_soc_acpi_mach * snd_soc_acpi_find_machine(struct snd_soc_acpi_mach *machines) { struct snd_soc_acpi_mach *mach; struct snd_soc_acpi_mach *mach_alt; - for (mach = machines; mach->id[0]; mach++) { - if (acpi_dev_present(mach->id, NULL, -1)) { + for (mach = machines; mach->id[0] || mach->comp_ids; mach++) { + if (snd_soc_acpi_id_present(mach)) { if (mach->machine_quirk) { mach_alt = mach->machine_quirk(mach); if (!mach_alt)
A machine driver needs to be enumerated by more than one ACPI HID if it supports second headphone driver (i.e. rt5682 and rt5682s). However, the id field in snd_soc_acpi_mach structure could contain only one HID. By adding a 'comp_ids' field which can contain several HIDs, we can enumerate a machine driver by multiple ACPI HIDs. Signed-off-by: Brent Lu <brent.lu@intel.com> --- include/sound/soc-acpi.h | 2 ++ sound/soc/soc-acpi.c | 24 ++++++++++++++++++++++-- 2 files changed, 24 insertions(+), 2 deletions(-)