[PATCH 0/2] ASoC: SOF: Introduce of_machine_select

[PATCH 0/2] ASoC: SOF: Introduce of_machine_select

[Chunxu Li]

From: "chunxu.li" <chunxu.li@mediatek.com>

In these patches, we introduce of_machine_select for SOF

Chunxu Li (2):
  ASoC: SOF: Introduce optional callback of_machine_select
  ASoC: SOF: mediatek: Add .of_machine_select field for mt8186

 include/sound/sof.h                    |  2 ++
 sound/soc/sof/mediatek/mt8186/mt8186.c | 11 +++++++++++
 sound/soc/sof/ops.h                    |  9 +++++++++
 sound/soc/sof/pcm.c                    |  8 +++++++-
 sound/soc/sof/sof-audio.c              |  7 +++++++
 sound/soc/sof/sof-of-dev.c             | 23 +++++++++++++++++++++++
 sound/soc/sof/sof-of-dev.h             |  8 ++++++++
 sound/soc/sof/sof-priv.h               |  1 +
 8 files changed, 68 insertions(+), 1 deletion(-)

-- 
2.25.1

[PATCH 1/2] ASoC: SOF: Introduce optional callback of_machine_select

[Chunxu Li]

From current design in sof_machine_check and snd_sof_new_platform_drv,
the SOF can only support ACPI type machine.

1. In sof_machine_check if there is no ACPI machine exist, the function
will return -ENODEV directly, that's we don't expected if we do not
base on ACPI machine.

2. In snd_sof_new_platform_drv the component driver need a driver name
to do ignore_machine, currently the driver name is obtained from
machine->drv_name, and the type of machine is snd_soc_acpi_mach.

So we add a new function snd_sof_of_machine_select that we can pass
sof_machine_check and obtain info required by snd_sof_new_platform_drv.
this callback is optional, each machine that do not based on ACPI can
use common code implemented in sof-of-dev.c or implement their customized
specific routine.

Signed-off-by: Chunxu Li <chunxu.li@mediatek.com>
---
 include/sound/sof.h        |  2 ++
 sound/soc/sof/ops.h        |  9 +++++++++
 sound/soc/sof/pcm.c        |  8 +++++++-
 sound/soc/sof/sof-audio.c  |  7 +++++++
 sound/soc/sof/sof-of-dev.c | 23 +++++++++++++++++++++++
 sound/soc/sof/sof-of-dev.h |  8 ++++++++
 sound/soc/sof/sof-priv.h   |  1 +
 7 files changed, 57 insertions(+), 1 deletion(-)

diff --git a/include/sound/sof.h b/include/sound/sof.h
index 367dccfea7ad..341fef19e612 100644
--- a/include/sound/sof.h
+++ b/include/sound/sof.h
@@ -89,6 +89,7 @@ struct snd_sof_pdata {
 	/* machine */
 	struct platform_device *pdev_mach;
 	const struct snd_soc_acpi_mach *machine;
+	const struct snd_sof_of_mach *of_machine;
 
 	void *hw_pdata;
 
@@ -102,6 +103,7 @@ struct snd_sof_pdata {
 struct sof_dev_desc {
 	/* list of machines using this configuration */
 	struct snd_soc_acpi_mach *machines;
+	struct snd_sof_of_mach *of_machines;
 
 	/* alternate list of machines using this configuration */
 	struct snd_soc_acpi_mach *alt_machines;
diff --git a/sound/soc/sof/ops.h b/sound/soc/sof/ops.h
index 55d43adb6a29..e20720c09744 100644
--- a/sound/soc/sof/ops.h
+++ b/sound/soc/sof/ops.h
@@ -522,6 +522,15 @@ snd_sof_set_mach_params(struct snd_soc_acpi_mach *mach,
 		sof_ops(sdev)->set_mach_params(mach, sdev);
 }
 
+static inline struct snd_sof_of_mach *
+snd_sof_of_machine_select(struct snd_sof_dev *sdev)
+{
+	if (sof_ops(sdev) && sof_ops(sdev)->of_machine_select)
+		return sof_ops(sdev)->of_machine_select(sdev);
+
+	return NULL;
+}
+
 /**
  * snd_sof_dsp_register_poll_timeout - Periodically poll an address
  * until a condition is met or a timeout occurs
diff --git a/sound/soc/sof/pcm.c b/sound/soc/sof/pcm.c
index 6cb6a432be5e..49f7cb049f62 100644
--- a/sound/soc/sof/pcm.c
+++ b/sound/soc/sof/pcm.c
@@ -13,6 +13,7 @@
 #include <linux/pm_runtime.h>
 #include <sound/pcm_params.h>
 #include <sound/sof.h>
+#include "sof-of-dev.h"
 #include "sof-priv.h"
 #include "sof-audio.h"
 #include "sof-utils.h"
@@ -655,7 +656,12 @@ void snd_sof_new_platform_drv(struct snd_sof_dev *sdev)
 	struct snd_sof_pdata *plat_data = sdev->pdata;
 	const char *drv_name;
 
-	drv_name = plat_data->machine->drv_name;
+	if (plat_data->machine)
+		drv_name = plat_data->machine->drv_name;
+	else if (plat_data->of_machine)
+		drv_name = plat_data->of_machine->drv_name;
+	else
+		drv_name = NULL;
 
 	pd->name = "sof-audio-component";
 	pd->probe = sof_pcm_probe;
diff --git a/sound/soc/sof/sof-audio.c b/sound/soc/sof/sof-audio.c
index 28976098a89e..7b3c99d1b2a4 100644
--- a/sound/soc/sof/sof-audio.c
+++ b/sound/soc/sof/sof-audio.c
@@ -794,6 +794,7 @@ int sof_machine_check(struct snd_sof_dev *sdev)
 	struct snd_soc_acpi_mach *mach;
 
 	if (!IS_ENABLED(CONFIG_SND_SOC_SOF_FORCE_NOCODEC_MODE)) {
+		const struct snd_sof_of_mach *of_mach;
 
 		/* find machine */
 		mach = snd_sof_machine_select(sdev);
@@ -803,6 +804,12 @@ int sof_machine_check(struct snd_sof_dev *sdev)
 			return 0;
 		}
 
+		of_mach = snd_sof_of_machine_select(sdev);
+		if (of_mach) {
+			sof_pdata->of_machine = of_mach;
+			return 0;
+		}
+
 		if (!IS_ENABLED(CONFIG_SND_SOC_SOF_NOCODEC)) {
 			dev_err(sdev->dev, "error: no matching ASoC machine driver found -
 aborting probe\n");
 			return -ENODEV;
diff --git a/sound/soc/sof/sof-of-dev.c b/sound/soc/sof/sof-of-dev.c
index 53faeccedd4f..8df588f9349a 100644
--- a/sound/soc/sof/sof-of-dev.c
+++ b/sound/soc/sof/sof-of-dev.c
@@ -31,6 +31,29 @@ const struct dev_pm_ops sof_of_pm = {
 };
 EXPORT_SYMBOL(sof_of_pm);
 
+struct snd_sof_of_mach *sof_of_machine_select(struct snd_sof_dev *sdev)
+{
+	struct snd_sof_pdata *sof_pdata = sdev->pdata;
+	const struct sof_dev_desc *desc = sof_pdata->desc;
+	struct snd_sof_of_mach *mach = desc->of_machines;
+
+	if (!mach)
+		return NULL;
+
+	for (; mach->board; mach++) {
+		if (of_machine_is_compatible(mach->board)) {
+			sof_pdata->tplg_filename = mach->sof_tplg_filename;
+			if (mach->fw_filename)
+				sof_pdata->fw_filename = mach->fw_filename;
+
+			return mach;
+		}
+	}
+
+	return NULL;
+}
+EXPORT_SYMBOL(sof_of_machine_select);
+
 static void sof_of_probe_complete(struct device *dev)
 {
 	/* allow runtime_pm */
diff --git a/sound/soc/sof/sof-of-dev.h b/sound/soc/sof/sof-of-dev.h
index fd950a222ba4..2ab81ced139d 100644
--- a/sound/soc/sof/sof-of-dev.h
+++ b/sound/soc/sof/sof-of-dev.h
@@ -9,8 +9,16 @@
 #ifndef __SOUND_SOC_SOF_OF_H
 #define __SOUND_SOC_SOF_OF_H
 
+struct snd_sof_of_mach {
+	const char *board;
+	const char *drv_name;
+	const char *fw_filename;
+	const char *sof_tplg_filename;
+};
+
 extern const struct dev_pm_ops sof_of_pm;
 
+struct snd_sof_of_mach *sof_of_machine_select(struct snd_sof_dev *sdev);
 int sof_of_probe(struct platform_device *pdev);
 int sof_of_remove(struct platform_device *pdev);
 void sof_of_shutdown(struct platform_device *pdev);
diff --git a/sound/soc/sof/sof-priv.h b/sound/soc/sof/sof-priv.h
index 823583086279..c5ed18f99d00 100644
--- a/sound/soc/sof/sof-priv.h
+++ b/sound/soc/sof/sof-priv.h
@@ -284,6 +284,7 @@ struct snd_sof_dsp_ops {
 	void (*machine_unregister)(struct snd_sof_dev *sdev,
 				   void *pdata); /* optional */
 	struct snd_soc_acpi_mach * (*machine_select)(struct snd_sof_dev *sdev); /*
 optional */
+	struct snd_sof_of_mach * (*of_machine_select)(struct snd_sof_dev *sdev);
 /* optional */
 	void (*set_mach_params)(struct snd_soc_acpi_mach *mach,
 				struct snd_sof_dev *sdev); /* optional */
 
-- 
2.25.1

Re: [PATCH 1/2] ASoC: SOF: Introduce optional callback of_machine_select

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 710 bytes --]

On Thu, Aug 04, 2022 at 05:13:58PM +0800, Chunxu Li wrote:

> @@ -284,6 +284,7 @@ struct snd_sof_dsp_ops {
>  	void (*machine_unregister)(struct snd_sof_dev *sdev,
>  				   void *pdata); /* optional */
>  	struct snd_soc_acpi_mach * (*machine_select)(struct snd_sof_dev *sdev); /*
>  optional */
> +	struct snd_sof_of_mach * (*of_machine_select)(struct snd_sof_dev *sdev);

I don't understand why we pass this in as a function when as far as I
can see it should always be the standard operation provided by the core
- why not just always call the function?  We can tell at runtime if the
system is using DT so there's no issue there and there shouldn't be any
concerns with ACPI or other firmware interfaces.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/2] ASoC: SOF: Introduce optional callback of_machine_select

[chunxu.li]

On Thu, 2022-08-04 at 14:17 +0100, Mark Brown wrote:
> On Thu, Aug 04, 2022 at 05:13:58PM +0800, Chunxu Li wrote:
> 
> > @@ -284,6 +284,7 @@ struct snd_sof_dsp_ops {
> >  	void (*machine_unregister)(struct snd_sof_dev *sdev,
> >  				   void *pdata); /* optional */
> >  	struct snd_soc_acpi_mach * (*machine_select)(struct snd_sof_dev
> > *sdev); /*
> >  optional */
> > +	struct snd_sof_of_mach * (*of_machine_select)(struct
> > snd_sof_dev *sdev);
> 
> I don't understand why we pass this in as a function when as far as I
> can see it should always be the standard operation provided by the
> core
> - why not just always call the function?  We can tell at runtime if
> the
> system is using DT so there's no issue there and there shouldn't be
> any
> concerns with ACPI or other firmware interfaces.

Thanks for you advice, I'll remove the callback function, and directly
call sof_of_machine_select() in sof_machine_check() as following.

int sof_machine_check(struct snd_sof_dev *sdev)
{
snip...
	const struct snd_sof_of_mach *of_mach;
	/* find machine */
	mach = snd_sof_machine_select(sdev);
	if (mach) {
		sof_pdata->machine = mach;
		snd_sof_set_mach_params(mach, sdev);
		return 0;
	}

	
+	of_mach = sof_of_machine_select(sdev);
+	if (of_mach) {
+		sof_pdata->of_machine = of_mach;
+		return 0;
+	}
snip ...
}

Re: [PATCH 1/2] ASoC: SOF: Introduce optional callback of_machine_select

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 421 bytes --]

On Thu, Aug 04, 2022 at 10:36:07PM +0800, chunxu.li wrote:

> Thanks for you advice, I'll remove the callback function, and directly
> call sof_of_machine_select() in sof_machine_check() as following.
> 
> int sof_machine_check(struct snd_sof_dev *sdev)
> {
> 	}
> 
> 	
> +	of_mach = sof_of_machine_select(sdev);
> +	if (of_mach) {
> +		sof_pdata->of_machine = of_mach;
> +		return 0;
> +	}

Looks good.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH 2/2] ASoC: SOF: mediatek: Add .of_machine_select field for mt8186

[Chunxu Li]

As the mt8186 do not use ACPI type machine, the of_machine_select
is used to distinguish different machines in sof_machine_check.

Signed-off-by: Chunxu Li <chunxu.li@mediatek.com>
---
 sound/soc/sof/mediatek/mt8186/mt8186.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/sound/soc/sof/mediatek/mt8186/mt8186.c
 b/sound/soc/sof/mediatek/mt8186/mt8186.c
index e006532caf2f..cb8bed383064 100644
--- a/sound/soc/sof/mediatek/mt8186/mt8186.c
+++ b/sound/soc/sof/mediatek/mt8186/mt8186.c
@@ -497,6 +497,8 @@ static struct snd_sof_dsp_ops sof_mt8186_ops = {
 	/* misc */
 	.get_bar_index	= mt8186_get_bar_index,
 
+	.of_machine_select = sof_of_machine_select,
+
 	/* firmware loading */
 	.load_firmware	= snd_sof_load_firmware_memcpy,
 
@@ -515,7 +517,16 @@ static struct snd_sof_dsp_ops sof_mt8186_ops = {
 			SNDRV_PCM_INFO_NO_PERIOD_WAKEUP,
 };
 
+static struct snd_sof_of_mach sof_mt8186_machs[] = {
+	{
+		.board = "mediatek,mt8186",
+		.sof_tplg_filename = "sof-mt8186.tplg",
+	},
+	{}
+};
+
 static const struct sof_dev_desc sof_of_mt8186_desc = {
+	.of_machines = sof_mt8186_machs,
 	.ipc_supported_mask	= BIT(SOF_IPC),
 	.ipc_default		= SOF_IPC,
 	.default_fw_path = {
-- 
2.25.1

Re: [PATCH 2/2] ASoC: SOF: mediatek: Add .of_machine_select field for mt8186

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 286 bytes --]

On Thu, Aug 04, 2022 at 05:13:59PM +0800, Chunxu Li wrote:

> +static struct snd_sof_of_mach sof_mt8186_machs[] = {
> +	{
> +		.board = "mediatek,mt8186",
> +		.sof_tplg_filename = "sof-mt8186.tplg",

The mediatek,mt8186 compatible looks like a SoC compatible not a board
compatible...

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 2/2] ASoC: SOF: mediatek: Add .of_machine_select field for mt8186

[chunxu.li]

On Thu, 2022-08-04 at 13:41 +0100, Mark Brown wrote:
> On Thu, Aug 04, 2022 at 05:13:59PM +0800, Chunxu Li wrote:
> 
> > +static struct snd_sof_of_mach sof_mt8186_machs[] = {
> > +	{
> > +		.board = "mediatek,mt8186",
> > +		.sof_tplg_filename = "sof-mt8186.tplg",
> 
> The mediatek,mt8186 compatible looks like a SoC compatible not a
> board
> compatible...

Our dts config as below:
kingler board:
compatible = "google,corsola", "google,kingler", "mediatek,mt8186";
krabby board:
compatible = "google,corsola", "google,krabby", "mediatek,mt8186";
they all use the same sof_tplg_filename, so i use "mediatek,mt8186" to
match these.

The original code looks like the following:
static struct snd_soc_acpi_mach sof_mt8186_machs[] = {
	{
		.board = "google,kingler",
		.sof_tplg_filename = "sof-mt8186.tplg",
	},
	{
		.board = "google,krabby",
		.sof_tplg_filename = "sof-mt8186.tplg",
	},
	{}
};

Which of the two approaches do you prefer?

Re: [PATCH 2/2] ASoC: SOF: mediatek: Add .of_machine_select field for mt8186

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 946 bytes --]

On Thu, Aug 04, 2022 at 09:21:37PM +0800, chunxu.li wrote:
> On Thu, 2022-08-04 at 13:41 +0100, Mark Brown wrote:
> > On Thu, Aug 04, 2022 at 05:13:59PM +0800, Chunxu Li wrote:

> > > +		.board = "mediatek,mt8186",
> > > +		.sof_tplg_filename = "sof-mt8186.tplg",

> > The mediatek,mt8186 compatible looks like a SoC compatible not a
> > board
> > compatible...

> Our dts config as below:
> kingler board:
> compatible = "google,corsola", "google,kingler", "mediatek,mt8186";
> krabby board:
> compatible = "google,corsola", "google,krabby", "mediatek,mt8186";

So the SoC is listed as a fallback for the board and things work out
fine.

> Which of the two approaches do you prefer?

I think it would be clearer to keep what's being matched the same but
rename the .board field to be .compatible, or possibly .system_compatible
or something if it's unclear what's being matched.  That'd be more
normal for specifying a compatible string anyway.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 2/2] ASoC: SOF: mediatek: Add .of_machine_select field for mt8186

[chunxu.li]

On Thu, 2022-08-04 at 14:33 +0100, Mark Brown wrote:
> On Thu, Aug 04, 2022 at 09:21:37PM +0800, chunxu.li wrote:
> > On Thu, 2022-08-04 at 13:41 +0100, Mark Brown wrote:
> > > On Thu, Aug 04, 2022 at 05:13:59PM +0800, Chunxu Li wrote:
> > > > +		.board = "mediatek,mt8186",
> > > > +		.sof_tplg_filename = "sof-mt8186.tplg",
> > > The mediatek,mt8186 compatible looks like a SoC compatible not a
> > > board
> > > compatible...
> > Our dts config as below:
> > kingler board:
> > compatible = "google,corsola", "google,kingler", "mediatek,mt8186";
> > krabby board:
> > compatible = "google,corsola", "google,krabby", "mediatek,mt8186";
> 
> So the SoC is listed as a fallback for the board and things work out
> fine.
> 
> > Which of the two approaches do you prefer?
> 
> I think it would be clearer to keep what's being matched the same but
> rename the .board field to be .compatible, or possibly
> .system_compatible
> or something if it's unclear what's being matched.  That'd be more
> normal for specifying a compatible string anyway.

Appreciated for you advice, I'll rename the .board to .compatible as
following:

+	.compatible = "mediatek,mt8186",
+	.sof_tplg_filename = "sof-mt8186.tplg",