diff mbox series

[09/13] ASoC: amd: acp: add machine driver support for pdm use case

Message ID 20231021145110.478744-9-Syed.SabaKareem@amd.com
State New
Headers
Series [01/13] ASoC: amd: acp: Add acp6.3 pci legacy driver support |

Commit Message

Saba Kareem, Syed Oct. 21, 2023, 2:50 p.m. UTC 
  add pdm use case machine driver support

Signed-off-by: Syed Saba Kareem <Syed.SabaKareem@amd.com>
---
 sound/soc/amd/acp/acp-legacy-mach.c | 12 ++++++++++++
 sound/soc/amd/acp/acp-platform.c    | 29 ++++++++++++++++++-----------
 2 files changed, 30 insertions(+), 11 deletions(-)

Comments

Krzysztof Kozlowski Oct. 27, 2023, 8:49 a.m. UTC | #1 
On 21/10/2023 16:50, Syed Saba Kareem wrote:
> add pdm use case machine driver support
> 
> Signed-off-by: Syed Saba Kareem <Syed.SabaKareem@amd.com>
> ---


>  	dmi_id = dmi_first_match(acp_quirk_table);
>  	if (dmi_id && dmi_id->driver_data)
> @@ -214,6 +221,10 @@ static const struct platform_device_id board_ids[] = {
>  		.name = "rmb-rt5682s-rt1019",
>  		.driver_data = (kernel_ulong_t)&rt5682s_rt1019_rmb_data,
>  	},
> +	{
> +		.name = "acp-pdm-mach",
> +		.driver_data = (kernel_ulong_t)&acp_dmic_data,
> +	},
>  	{ }
>  };
>  static struct platform_driver acp_asoc_audio = {
> @@ -235,4 +246,5 @@ MODULE_ALIAS("platform:acp3xalc5682s1019");
>  MODULE_ALIAS("platform:acp3x-es83xx");
>  MODULE_ALIAS("platform:rmb-nau8825-max");
>  MODULE_ALIAS("platform:rmb-rt5682s-rt1019");
> +MODULE_ALIAS("platform:acp-pdm-mach");

Please stop growing the aliases. Module alias is not a substitute for
missing MODULE_DEVICE_TABLE.

Best regards,
Krzysztof
Mario Limonciello Oct. 27, 2023, 3:28 p.m. UTC | #2 
On 10/27/2023 03:49, Krzysztof Kozlowski wrote:
> On 21/10/2023 16:50, Syed Saba Kareem wrote:
>> add pdm use case machine driver support
>>
>> Signed-off-by: Syed Saba Kareem <Syed.SabaKareem@amd.com>
>> ---
> 
> 
>>   	dmi_id = dmi_first_match(acp_quirk_table);
>>   	if (dmi_id && dmi_id->driver_data)
>> @@ -214,6 +221,10 @@ static const struct platform_device_id board_ids[] = {
>>   		.name = "rmb-rt5682s-rt1019",
>>   		.driver_data = (kernel_ulong_t)&rt5682s_rt1019_rmb_data,
>>   	},
>> +	{
>> +		.name = "acp-pdm-mach",
>> +		.driver_data = (kernel_ulong_t)&acp_dmic_data,
>> +	},
>>   	{ }
>>   };
>>   static struct platform_driver acp_asoc_audio = {
>> @@ -235,4 +246,5 @@ MODULE_ALIAS("platform:acp3xalc5682s1019");
>>   MODULE_ALIAS("platform:acp3x-es83xx");
>>   MODULE_ALIAS("platform:rmb-nau8825-max");
>>   MODULE_ALIAS("platform:rmb-rt5682s-rt1019");
>> +MODULE_ALIAS("platform:acp-pdm-mach");
> 
> Please stop growing the aliases. Module alias is not a substitute for
> missing MODULE_DEVICE_TABLE.
> 
> Best regards,
> Krzysztof
> 

I thought the way that this works is that top level ACP driver (IE 
acp-pci.c) will have MODULE_DEVICE_TABLE.  This is how that module gets 
loaded.

Then it creates platform devices based on the detected needs for the 
situation and the creation of those platform devices triggers a uevent 
which due to MODULE_ALIAS will get appropriate other platform drivers 
like this one loaded.
Krzysztof Kozlowski Oct. 27, 2023, 3:51 p.m. UTC | #3 
On 27/10/2023 17:28, Mario Limonciello wrote:
> On 10/27/2023 03:49, Krzysztof Kozlowski wrote:
>> On 21/10/2023 16:50, Syed Saba Kareem wrote:
>>> add pdm use case machine driver support
>>>
>>> Signed-off-by: Syed Saba Kareem <Syed.SabaKareem@amd.com>
>>> ---
>>
>>
>>>   	dmi_id = dmi_first_match(acp_quirk_table);
>>>   	if (dmi_id && dmi_id->driver_data)
>>> @@ -214,6 +221,10 @@ static const struct platform_device_id board_ids[] = {
>>>   		.name = "rmb-rt5682s-rt1019",
>>>   		.driver_data = (kernel_ulong_t)&rt5682s_rt1019_rmb_data,
>>>   	},
>>> +	{
>>> +		.name = "acp-pdm-mach",
>>> +		.driver_data = (kernel_ulong_t)&acp_dmic_data,
>>> +	},
>>>   	{ }
>>>   };
>>>   static struct platform_driver acp_asoc_audio = {
>>> @@ -235,4 +246,5 @@ MODULE_ALIAS("platform:acp3xalc5682s1019");
>>>   MODULE_ALIAS("platform:acp3x-es83xx");
>>>   MODULE_ALIAS("platform:rmb-nau8825-max");
>>>   MODULE_ALIAS("platform:rmb-rt5682s-rt1019");
>>> +MODULE_ALIAS("platform:acp-pdm-mach");
>>
>> Please stop growing the aliases. Module alias is not a substitute for
>> missing MODULE_DEVICE_TABLE.
>>
>> Best regards,
>> Krzysztof
>>
> 
> I thought the way that this works is that top level ACP driver (IE 
> acp-pci.c) will have MODULE_DEVICE_TABLE.  This is how that module gets 
> loaded.
> 
> Then it creates platform devices based on the detected needs for the 
> situation and the creation of those platform devices triggers a uevent 
> which due to MODULE_ALIAS will get appropriate other platform drivers 
> like this one loaded.

And why you cannot use MODULE_DEVICE_TABLE here? IOW, why do you need to
manually duplicate entire table and re-invent MODULE_DEVICE_TABLE with
MODULE_ALIAS?

Best regards,
Krzysztof
Mario Limonciello Oct. 27, 2023, 3:54 p.m. UTC | #4 
On 10/27/2023 10:51, Krzysztof Kozlowski wrote:
> On 27/10/2023 17:28, Mario Limonciello wrote:
>> On 10/27/2023 03:49, Krzysztof Kozlowski wrote:
>>> On 21/10/2023 16:50, Syed Saba Kareem wrote:
>>>> add pdm use case machine driver support
>>>>
>>>> Signed-off-by: Syed Saba Kareem <Syed.SabaKareem@amd.com>
>>>> ---
>>>
>>>
>>>>    	dmi_id = dmi_first_match(acp_quirk_table);
>>>>    	if (dmi_id && dmi_id->driver_data)
>>>> @@ -214,6 +221,10 @@ static const struct platform_device_id board_ids[] = {
>>>>    		.name = "rmb-rt5682s-rt1019",
>>>>    		.driver_data = (kernel_ulong_t)&rt5682s_rt1019_rmb_data,
>>>>    	},
>>>> +	{
>>>> +		.name = "acp-pdm-mach",
>>>> +		.driver_data = (kernel_ulong_t)&acp_dmic_data,
>>>> +	},
>>>>    	{ }
>>>>    };
>>>>    static struct platform_driver acp_asoc_audio = {
>>>> @@ -235,4 +246,5 @@ MODULE_ALIAS("platform:acp3xalc5682s1019");
>>>>    MODULE_ALIAS("platform:acp3x-es83xx");
>>>>    MODULE_ALIAS("platform:rmb-nau8825-max");
>>>>    MODULE_ALIAS("platform:rmb-rt5682s-rt1019");
>>>> +MODULE_ALIAS("platform:acp-pdm-mach");
>>>
>>> Please stop growing the aliases. Module alias is not a substitute for
>>> missing MODULE_DEVICE_TABLE.
>>>
>>> Best regards,
>>> Krzysztof
>>>
>>
>> I thought the way that this works is that top level ACP driver (IE
>> acp-pci.c) will have MODULE_DEVICE_TABLE.  This is how that module gets
>> loaded.
>>
>> Then it creates platform devices based on the detected needs for the
>> situation and the creation of those platform devices triggers a uevent
>> which due to MODULE_ALIAS will get appropriate other platform drivers
>> like this one loaded.
> 
> And why you cannot use MODULE_DEVICE_TABLE here? IOW, why do you need to
> manually duplicate entire table and re-invent MODULE_DEVICE_TABLE with
> MODULE_ALIAS?

What would actually go into MODULE_DEVICE_TABLE?

The platform devices created are contingent upon what was found during 
the top level ACP driver probe.  You don't want all the "child" platform 
drivers to load unless they're needed.
Krzysztof Kozlowski Oct. 27, 2023, 3:57 p.m. UTC | #5 
On 27/10/2023 17:54, Mario Limonciello wrote:
> On 10/27/2023 10:51, Krzysztof Kozlowski wrote:
>> On 27/10/2023 17:28, Mario Limonciello wrote:
>>> On 10/27/2023 03:49, Krzysztof Kozlowski wrote:
>>>> On 21/10/2023 16:50, Syed Saba Kareem wrote:
>>>>> add pdm use case machine driver support
>>>>>
>>>>> Signed-off-by: Syed Saba Kareem <Syed.SabaKareem@amd.com>
>>>>> ---
>>>>
>>>>
>>>>>    	dmi_id = dmi_first_match(acp_quirk_table);
>>>>>    	if (dmi_id && dmi_id->driver_data)
>>>>> @@ -214,6 +221,10 @@ static const struct platform_device_id board_ids[] = {
>>>>>    		.name = "rmb-rt5682s-rt1019",
>>>>>    		.driver_data = (kernel_ulong_t)&rt5682s_rt1019_rmb_data,
>>>>>    	},
>>>>> +	{
>>>>> +		.name = "acp-pdm-mach",
>>>>> +		.driver_data = (kernel_ulong_t)&acp_dmic_data,
>>>>> +	},
>>>>>    	{ }
>>>>>    };
>>>>>    static struct platform_driver acp_asoc_audio = {
>>>>> @@ -235,4 +246,5 @@ MODULE_ALIAS("platform:acp3xalc5682s1019");
>>>>>    MODULE_ALIAS("platform:acp3x-es83xx");
>>>>>    MODULE_ALIAS("platform:rmb-nau8825-max");
>>>>>    MODULE_ALIAS("platform:rmb-rt5682s-rt1019");
>>>>> +MODULE_ALIAS("platform:acp-pdm-mach");
>>>>
>>>> Please stop growing the aliases. Module alias is not a substitute for
>>>> missing MODULE_DEVICE_TABLE.
>>>>
>>>> Best regards,
>>>> Krzysztof
>>>>
>>>
>>> I thought the way that this works is that top level ACP driver (IE
>>> acp-pci.c) will have MODULE_DEVICE_TABLE.  This is how that module gets
>>> loaded.
>>>
>>> Then it creates platform devices based on the detected needs for the
>>> situation and the creation of those platform devices triggers a uevent
>>> which due to MODULE_ALIAS will get appropriate other platform drivers
>>> like this one loaded.
>>
>> And why you cannot use MODULE_DEVICE_TABLE here? IOW, why do you need to
>> manually duplicate entire table and re-invent MODULE_DEVICE_TABLE with
>> MODULE_ALIAS?
> 
> What would actually go into MODULE_DEVICE_TABLE?

The table you have few lines above aliases.

> 
> The platform devices created are contingent upon what was found during 
> the top level ACP driver probe.  You don't want all the "child" platform 
> drivers to load unless they're needed.

How static alias differs here from static device ID table? Both are
built into the module and always there. I don't even understand what
does it mean by "loading child platform drivers". Why would unneeded
driver be loaded?

Best regards,
Krzysztof
Mark Brown Oct. 27, 2023, 4:23 p.m. UTC | #6 
On Fri, Oct 27, 2023 at 10:54:47AM -0500, Mario Limonciello wrote:

> What would actually go into MODULE_DEVICE_TABLE?

> The platform devices created are contingent upon what was found during the
> top level ACP driver probe.  You don't want all the "child" platform drivers
> to load unless they're needed.

You want

	MODULE_DEVICE_TABLE(platform, board_ids);

which is effectively the same as all the MODULE_ALIAS items you have
there (which can be removed).
syed saba kareem Oct. 27, 2023, 5:32 p.m. UTC | #7 
On 10/27/23 21:53, Mark Brown wrote:
> On Fri, Oct 27, 2023 at 10:54:47AM -0500, Mario Limonciello wrote:
>
>> What would actually go into MODULE_DEVICE_TABLE?
>> The platform devices created are contingent upon what was found during the
>> top level ACP driver probe.  You don't want all the "child" platform drivers
>> to load unless they're needed.
> You want
>
> 	MODULE_DEVICE_TABLE(platform, board_ids);
>
> which is effectively the same as all the MODULE_ALIAS items you have
> there (which can be removed).

@krzk:as Mark Brown explained we can use platform device id table

instead of MODULE_ALIAS. As effectively there is no difference between

using platform device id table and MODULE_ALIAS.

If you are still expecting us to use platform id table instead of 
MODULE_ALIAS

we will provide the changes as an incremental patch.
Krzysztof Kozlowski Oct. 28, 2023, 9:14 a.m. UTC | #8 
On 27/10/2023 19:32, syed saba kareem wrote:
> 
> On 10/27/23 21:53, Mark Brown wrote:
>> On Fri, Oct 27, 2023 at 10:54:47AM -0500, Mario Limonciello wrote:
>>
>>> What would actually go into MODULE_DEVICE_TABLE?
>>> The platform devices created are contingent upon what was found during the
>>> top level ACP driver probe.  You don't want all the "child" platform drivers
>>> to load unless they're needed.
>> You want
>>
>> 	MODULE_DEVICE_TABLE(platform, board_ids);
>>
>> which is effectively the same as all the MODULE_ALIAS items you have
>> there (which can be removed).
> 
> @krzk:as Mark Brown explained we can use platform device id table
> 
> instead of MODULE_ALIAS. As effectively there is no difference between
> 
> using platform device id table and MODULE_ALIAS.

There is a difference. MODULE_DEVICE_TABLE solves the problem and you do
not need to spread aliases all over. This code is not equivalent. What's
more, DEVICE_TABLE could be used for other purposes like dependency
detection or ordering or whatever. ALIAS not.

> 
> If you are still expecting us to use platform id table instead of 
> MODULE_ALIAS

Yes, I asked this first time.

> 
> we will provide the changes as an incremental patch.

Fix existing driver before adding new aliases. Then don't add ALIAS, how
I asked already two times before.

Best regards,
Krzysztof
diff mbox series

Patch

diff --git a/sound/soc/amd/acp/acp-legacy-mach.c b/sound/soc/amd/acp/acp-legacy-mach.c
index 1ab3edffe0ce..47c3b5f167f5 100644
--- a/sound/soc/amd/acp/acp-legacy-mach.c
+++ b/sound/soc/amd/acp/acp-legacy-mach.c
@@ -84,6 +84,11 @@  static struct acp_card_drvdata rt5682s_rt1019_rmb_data = {
 	.tdm_mode = false,
 };
 
+static struct acp_card_drvdata acp_dmic_data = {
+	.dmic_cpu_id = DMIC,
+	.dmic_codec_id = DMIC,
+};
+
 static bool acp_asoc_init_ops(struct acp_card_drvdata *priv)
 {
 	bool has_ops = false;
@@ -165,6 +170,8 @@  static int acp_asoc_probe(struct platform_device *pdev)
 			card->name, ret);
 		goto out;
 	}
+	if (!strcmp(pdev->name, "acp-pdm-mach"))
+		acp_card_drvdata->platform =  *((int *)dev->platform_data);
 
 	dmi_id = dmi_first_match(acp_quirk_table);
 	if (dmi_id && dmi_id->driver_data)
@@ -214,6 +221,10 @@  static const struct platform_device_id board_ids[] = {
 		.name = "rmb-rt5682s-rt1019",
 		.driver_data = (kernel_ulong_t)&rt5682s_rt1019_rmb_data,
 	},
+	{
+		.name = "acp-pdm-mach",
+		.driver_data = (kernel_ulong_t)&acp_dmic_data,
+	},
 	{ }
 };
 static struct platform_driver acp_asoc_audio = {
@@ -235,4 +246,5 @@  MODULE_ALIAS("platform:acp3xalc5682s1019");
 MODULE_ALIAS("platform:acp3x-es83xx");
 MODULE_ALIAS("platform:rmb-nau8825-max");
 MODULE_ALIAS("platform:rmb-rt5682s-rt1019");
+MODULE_ALIAS("platform:acp-pdm-mach");
 MODULE_LICENSE("GPL v2");
diff --git a/sound/soc/amd/acp/acp-platform.c b/sound/soc/amd/acp/acp-platform.c
index f516daf6fef4..aaac8aa744cb 100644
--- a/sound/soc/amd/acp/acp-platform.c
+++ b/sound/soc/amd/acp/acp-platform.c
@@ -21,6 +21,8 @@ 
 #include <linux/dma-mapping.h>
 
 #include "amd.h"
+#include "../mach-config.h"
+#include "acp-mach.h"
 
 #define DRV_NAME "acp_i2s_dma"
 
@@ -69,20 +71,25 @@  static const struct snd_pcm_hardware acp_pcm_hardware_capture = {
 int acp_machine_select(struct acp_dev_data *adata)
 {
 	struct snd_soc_acpi_mach *mach;
-	int size;
-
-	size = sizeof(*adata->machines);
-	mach = snd_soc_acpi_find_machine(adata->machines);
-	if (!mach) {
-		dev_err(adata->dev, "warning: No matching ASoC machine driver found\n");
-		return -EINVAL;
+	int size, platform;
+
+	if (adata->flag == FLAG_AMD_LEGACY_ONLY_DMIC) {
+		platform = adata->platform;
+		adata->mach_dev = platform_device_register_data(adata->dev, "acp-pdm-mach",
+								PLATFORM_DEVID_NONE, &platform,
+								sizeof(platform));
+	} else {
+		size = sizeof(*adata->machines);
+		mach = snd_soc_acpi_find_machine(adata->machines);
+		if (!mach) {
+			dev_err(adata->dev, "warning: No matching ASoC machine driver found\n");
+			return -EINVAL;
+		}
+		adata->mach_dev = platform_device_register_data(adata->dev, mach->drv_name,
+								PLATFORM_DEVID_NONE, mach, size);
 	}
-
-	adata->mach_dev = platform_device_register_data(adata->dev, mach->drv_name,
-							PLATFORM_DEVID_NONE, mach, size);
 	if (IS_ERR(adata->mach_dev))
 		dev_warn(adata->dev, "Unable to register Machine device\n");
-
 	return 0;
 }
 EXPORT_SYMBOL_NS_GPL(acp_machine_select, SND_SOC_ACP_COMMON);