| Message ID | 20211216115743.2130622-5-cezary.rojewski@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ASoC: Intel: catpt: Dma-transfer fix and couple | expand |
> +struct snd_soc_acpi_mach lpt_machines[] = { > + { > + .id = "INT33CA", > + .drv_name = "haswell-audio", > + }, > + {} > +}; > + > +struct snd_soc_acpi_mach wpt_machines[] = { > + { > + .id = "INT343A", > + .drv_name = "broadwell-audio", > + }, > + { > + .id = "10EC5650", > + .drv_name = "bdw-rt5650", > + }, > + { > + .id = "RT5677CE", > + .drv_name = "bdw-rt5677", > + }, > + { > + .id = "INT33CA", > + .drv_name = "haswell-audio", > + }, > + {} > +}; The intent of soc-acpi files is to establish a match between ACPI _HID and machine driver, this is now duplicated, and it makes limited sense to add machine driver dependencies in a platform driver. Nothing was broken with the existing code.
On 2021-12-16 3:11 PM, Pierre-Louis Bossart wrote: > The intent of soc-acpi files is to establish a match between ACPI _HID > and machine driver, this is now duplicated, and it makes limited sense > to add machine driver dependencies in a platform driver. > > Nothing was broken with the existing code. Hello, Yes, nothing is broken in the existing code. The intention is different - be cohesive about what is actually used by the driver. PCI-ids table is duplicated already for the Intel audio drivers. And it's OK to do so - one knows which ids are covered by given driver and how. Here, it's clear that haswell_machines are only used by catpt-driver and so are some fields for broadwell_machines. In time I believe that we will be able to reduce the number of fields for struct snd_soc_acpi_mach i.e. have a single fw_filename and single tplg_filename field without some driver-specific duplicates. About the last, there could be a case where no topology file is available for certain configuration and given entry should not be taken into account. While catpt-driver does not make use of soc-topology feature, that isn't true for other drivers. Regards, Czarek
On 12/16/21 8:37 AM, Cezary Rojewski wrote: > On 2021-12-16 3:11 PM, Pierre-Louis Bossart wrote: >> The intent of soc-acpi files is to establish a match between ACPI _HID >> and machine driver, this is now duplicated, and it makes limited sense >> to add machine driver dependencies in a platform driver. >> >> Nothing was broken with the existing code. > > Hello, > > Yes, nothing is broken in the existing code. The intention is different > - be cohesive about what is actually used by the driver. > > PCI-ids table is duplicated already for the Intel audio drivers. And > it's OK to do so - one knows which ids are covered by given driver and > how. Here, it's clear that haswell_machines are only used by > catpt-driver and so are some fields for broadwell_machines. In time I > believe that we will be able to reduce the number of fields for struct > snd_soc_acpi_mach i.e. have a single fw_filename and single > tplg_filename field without some driver-specific duplicates. I don't really see the point about the number of fields, this is a generic descriptor used for I2S/SoundWire devices so mechanically there are things are are not used in all platforms. Another example is the quirks field, it's only meant to be used when there's actually a quirk. Note that I am planning to remove the sof_fw_filename field since it's redundant with what is part of the PCI descriptor, but the topology will remain there: it has to match with the machine driver. > About the last, there could be a case where no topology file is > available for certain configuration and given entry should not be taken > into account. While catpt-driver does not make use of soc-topology > feature, that isn't true for other drivers. Again if a feature is not needed/not supported, the field can remain empty.
On 2021-12-16 4:13 PM, Pierre-Louis Bossart wrote: ... > I don't really see the point about the number of fields, this is a > generic descriptor used for I2S/SoundWire devices so mechanically there > are things are are not used in all platforms. > > Another example is the quirks field, it's only meant to be used when > there's actually a quirk. > > Note that I am planning to remove the sof_fw_filename field since it's > redundant with what is part of the PCI descriptor, but the topology will > remain there: it has to match with the machine driver. That's why no new struct is declared. Simply the tables are moved locally, and there is nothing wrong with that. Cohesiveness and readability outweighs the duplication of ACPI _HID. After thinking about this again, perhaps in the future, this generic descriptor should be split into more specific bits e.g.: like the struct pci_driver which wraps struct driver. i2s_mach = { // some i2s-specific fields e.g.: .acpi-id = XXX, .mach = { // some generic fields e.g.: .drv_name = "a machine board name", }, }; Regards, Czarek
>> I don't really see the point about the number of fields, this is a >> generic descriptor used for I2S/SoundWire devices so mechanically there >> are things are are not used in all platforms. >> >> Another example is the quirks field, it's only meant to be used when >> there's actually a quirk. >> >> Note that I am planning to remove the sof_fw_filename field since it's >> redundant with what is part of the PCI descriptor, but the topology will >> remain there: it has to match with the machine driver. > > That's why no new struct is declared. Simply the tables are moved > locally, and there is nothing wrong with that. Cohesiveness and > readability outweighs the duplication of ACPI _HID. If I follow your logic, I could move all the tables for glk, cnl, cfl, cml, icl, jsl, tgl, ehl, adl into the SOF driver. That really doesn't seem sensible.
On 2021-12-16 5:29 PM, Pierre-Louis Bossart wrote: >>> I don't really see the point about the number of fields, this is a >>> generic descriptor used for I2S/SoundWire devices so mechanically there >>> are things are are not used in all platforms. >>> >>> Another example is the quirks field, it's only meant to be used when >>> there's actually a quirk. >>> >>> Note that I am planning to remove the sof_fw_filename field since it's >>> redundant with what is part of the PCI descriptor, but the topology will >>> remain there: it has to match with the machine driver. >> >> That's why no new struct is declared. Simply the tables are moved >> locally, and there is nothing wrong with that. Cohesiveness and >> readability outweighs the duplication of ACPI _HID. > > If I follow your logic, I could move all the tables for glk, cnl, cfl, > cml, icl, jsl, tgl, ehl, adl into the SOF driver. That really doesn't > seem sensible. Hmm.. doesn't it really? Are the glk/cnl/cfl/cml/icl/jsl/tgl/ehl/adl tables "common" for the Intel audio drivers? There are not used by anything except for SOF. It seems reasonable to have them present locally too. SOF solution becomes more cohesively organized in such case, just like catpt is after this patch. Regards, Czarek
>>>> I don't really see the point about the number of fields, this is a >>>> generic descriptor used for I2S/SoundWire devices so mechanically there >>>> are things are are not used in all platforms. >>>> >>>> Another example is the quirks field, it's only meant to be used when >>>> there's actually a quirk. >>>> >>>> Note that I am planning to remove the sof_fw_filename field since it's >>>> redundant with what is part of the PCI descriptor, but the topology >>>> will >>>> remain there: it has to match with the machine driver. >>> >>> That's why no new struct is declared. Simply the tables are moved >>> locally, and there is nothing wrong with that. Cohesiveness and >>> readability outweighs the duplication of ACPI _HID. >> >> If I follow your logic, I could move all the tables for glk, cnl, cfl, >> cml, icl, jsl, tgl, ehl, adl into the SOF driver. That really doesn't >> seem sensible. > > Hmm.. doesn't it really? Are the glk/cnl/cfl/cml/icl/jsl/tgl/ehl/adl > tables "common" for the Intel audio drivers? There are not used by > anything except for SOF. It seems reasonable to have them present > locally too. SOF solution becomes more cohesively organized in such > case, just like catpt is after this patch. We could also move the boards/ while we're at it, on the grounds they are not all used by all drivers. My take is that unless a new feature is added that warrants moving the tables around, let's keep the existing code as is.
On 2021-12-16 7:04 PM, Pierre-Louis Bossart wrote: ... >> Hmm.. doesn't it really? Are the glk/cnl/cfl/cml/icl/jsl/tgl/ehl/adl >> tables "common" for the Intel audio drivers? There are not used by >> anything except for SOF. It seems reasonable to have them present >> locally too. SOF solution becomes more cohesively organized in such >> case, just like catpt is after this patch. > > We could also move the boards/ while we're at it, on the grounds they > are not all used by all drivers. > > My take is that unless a new feature is added that warrants moving the > tables around, let's keep the existing code as is. > I agree with the relocation of the boards! In general, improving the maintainability is worth it, even if it takes a larger number of patches. Hmm.. avs-driver related boards are kind of an argument in this discussion. I can delay patches 4/5 and 5/5 until they're on the list, no problem. Mark, Should I re-send the series as 3, or is it fine as is and simply last two patches could be omitted? Regards, Czarek
diff --git a/sound/soc/intel/Kconfig b/sound/soc/intel/Kconfig index f3a4a907b29d..0423009a186e 100644 --- a/sound/soc/intel/Kconfig +++ b/sound/soc/intel/Kconfig @@ -23,7 +23,7 @@ config SND_SOC_INTEL_CATPT depends on ACPI || COMPILE_TEST depends on DMADEVICES && SND_DMA_SGBUF select DW_DMAC_CORE - select SND_SOC_ACPI_INTEL_MATCH + select SND_SOC_ACPI select WANT_DEV_COREDUMP select SND_INTEL_DSP_CONFIG help diff --git a/sound/soc/intel/catpt/device.c b/sound/soc/intel/catpt/device.c index 85a34e37316d..81c7cb94b68b 100644 --- a/sound/soc/intel/catpt/device.c +++ b/sound/soc/intel/catpt/device.c @@ -22,7 +22,6 @@ #include <sound/intel-dsp-config.h> #include <sound/soc.h> #include <sound/soc-acpi.h> -#include <sound/soc-acpi-intel-match.h> #include "core.h" #include "registers.h" @@ -313,8 +312,36 @@ static int catpt_acpi_remove(struct platform_device *pdev) return 0; } +struct snd_soc_acpi_mach lpt_machines[] = { + { + .id = "INT33CA", + .drv_name = "haswell-audio", + }, + {} +}; + +struct snd_soc_acpi_mach wpt_machines[] = { + { + .id = "INT343A", + .drv_name = "broadwell-audio", + }, + { + .id = "10EC5650", + .drv_name = "bdw-rt5650", + }, + { + .id = "RT5677CE", + .drv_name = "bdw-rt5677", + }, + { + .id = "INT33CA", + .drv_name = "haswell-audio", + }, + {} +}; + static struct catpt_spec lpt_desc = { - .machines = snd_soc_acpi_intel_haswell_machines, + .machines = lpt_machines, .core_id = 0x01, .host_dram_offset = 0x000000, .host_iram_offset = 0x080000, @@ -329,7 +356,7 @@ static struct catpt_spec lpt_desc = { }; static struct catpt_spec wpt_desc = { - .machines = snd_soc_acpi_intel_broadwell_machines, + .machines = wpt_machines, .core_id = 0x02, .host_dram_offset = 0x000000, .host_iram_offset = 0x0A0000,
catpt-driver does not make use of most of the fields found in the descriptor table and is the sole user of haswell machines list. Move the tables to local directory and clean them up so it's clear what's actually used by the solution. Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com> --- sound/soc/intel/Kconfig | 2 +- sound/soc/intel/catpt/device.c | 33 ++++++++++++++++++++++++++++++--- 2 files changed, 31 insertions(+), 4 deletions(-)