diff mbox series

[4/5] ASoC: Intel: catpt: Drop SND_SOC_ACPI_INTEL_MATCH dependency

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

Commit Message

Cezary Rojewski Dec. 16, 2021, 11:57 a.m. UTC
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(-)

Comments

Pierre-Louis Bossart Dec. 16, 2021, 2:11 p.m. UTC | #1
> +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.
Cezary Rojewski Dec. 16, 2021, 2:37 p.m. UTC | #2
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
Pierre-Louis Bossart Dec. 16, 2021, 3:13 p.m. UTC | #3
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.
Cezary Rojewski Dec. 16, 2021, 3:59 p.m. UTC | #4
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
Pierre-Louis Bossart Dec. 16, 2021, 4:29 p.m. UTC | #5
>> 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.
Cezary Rojewski Dec. 16, 2021, 4:50 p.m. UTC | #6
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
Pierre-Louis Bossart Dec. 16, 2021, 6:04 p.m. UTC | #7
>>>> 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.
Cezary Rojewski Dec. 17, 2021, 9:06 a.m. UTC | #8
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 mbox series

Patch

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,