[PATCH 0/2] ASoC: SOF: Introduce of_machine_select
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(-)
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 */
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 = {
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...
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.
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?
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.
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 ... }
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",
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.
participants (3)
-
Chunxu Li
-
chunxu.li
-
Mark Brown