Re: [PATCH 1/9] ASoC: SOF: imx: introduce more common structures and functions

[Laurentiu Mihalcea <laurentiumihalcea111@gmail.com>]

On 2/3/2025 9:52 PM, Frank Li wrote:
> On Mon, Feb 03, 2025 at 12:18:00PM -0500, Laurentiu Mihalcea wrote:
>> From: Laurentiu Mihalcea <laurentiu.mihalcea@nxp.com>
>>
>> The SOF drivers for imx chips have a lot of duplicate code
>> and routines/code snippets that could certainly be reused
>> among drivers.
>>
>> As such, introduce a new set of structures and functions
>> that will help eliminate the redundancy and code size of
>> the drivers.
> please wrap at 75 chars

sure, fix in v2


>
>> Signed-off-by: Laurentiu Mihalcea <laurentiu.mihalcea@nxp.com>
>> Reviewed-by: Daniel Baluta <daniel.baluta@nxp.com>
>> Reviewed-by: Iuliana Prodan <iuliana.prodan@nxp.com>
>> ---
>>  sound/soc/sof/imx/imx-common.c | 419 ++++++++++++++++++++++++++++++++-
>>  sound/soc/sof/imx/imx-common.h | 149 ++++++++++++
>>  2 files changed, 567 insertions(+), 1 deletion(-)
>>
>> diff --git a/sound/soc/sof/imx/imx-common.c b/sound/soc/sof/imx/imx-common.c
>> index fce6d9cf6a6b..5921900335c8 100644
>> --- a/sound/soc/sof/imx/imx-common.c
>> +++ b/sound/soc/sof/imx/imx-common.c
>> @@ -1,11 +1,16 @@
>>  // SPDX-License-Identifier: (GPL-2.0-only OR BSD-3-Clause)
>>  //
>> -// Copyright 2020 NXP
>> +// Copyright 2020-2025 NXP
>>  //
>>  // Common helpers for the audio DSP on i.MX8
>>
>> +#include <linux/firmware/imx/dsp.h>
>>  #include <linux/module.h>
>> +#include <linux/of_reserved_mem.h>
>> +#include <linux/of_address.h>
> keep alphabet order

ok

>
>> +#include <linux/pm_domain.h>
>>  #include <sound/sof/xtensa.h>
>> +
>>  #include "../ops.h"
>>
>>  #include "imx-common.h"
>> @@ -74,5 +79,417 @@ void imx8_dump(struct snd_sof_dev *sdev, u32 flags)
>>  }
>>  EXPORT_SYMBOL(imx8_dump);
>>
>> +static void imx_handle_reply(struct imx_dsp_ipc *ipc)
>> +{
>> +	unsigned long flags;
>> +	struct snd_sof_dev *sdev = imx_dsp_get_data(ipc);
>> +
>> +	spin_lock_irqsave(&sdev->ipc_lock, flags);
>> +	snd_sof_ipc_process_reply(sdev, 0);
>> +	spin_unlock_irqrestore(&sdev->ipc_lock, flags);
> Are you sure have to use spin_lock?

not sure, this definition was taken from previous drivers. I'd say keep it for now
as removing it would require some more testing which will take some time.

(snip)

>> +static int imx_suspend(struct snd_sof_dev *sdev, unsigned int target_state)
>> +{
>> +	int ret;
>> +	const struct sof_dsp_power_state target_power_state = {
>> +		.state = target_state,
>> +	};
>> +
>> +	if (!pm_runtime_suspended(sdev->dev)) {
>> +		ret = imx_common_suspend(sdev);
>> +		if (ret < 0) {
>> +			dev_err(sdev->dev, "failed to common suspend: %d\n", ret);
>> +			return ret;
>> +		}
>> +	}
>> +
>> +	return snd_sof_dsp_set_power_state(sdev, &target_power_state);
> does pm_runtime_force_suspend()/pm_runtime_force_resume() work?

no, these functions are not called directly by the PM core, but rather by the SOF core.
Using the proposed functions would result in the SOF core PM functions (i.e: sof_resume/sof_suspend)
being called twice in the case of system suspend/resume, which is wrong.

(snip)

>> +
>> +static int imx_probe(struct snd_sof_dev *sdev)
>> +{
>> +	int ret;
>> +	struct platform_device *pdev;
>> +	struct imx_common_data *common;
>> +	struct dev_pm_domain_attach_data domain_data = {
>> +		.pd_names = NULL, /* no filtering */
>> +		.pd_flags = PD_FLAG_DEV_LINK_ON,
>> +	};
> try keep reverse Christmas tree.

sure, fix in V2

>
>> +
>> +	pdev = to_platform_device(sdev->dev);
>> +
>> +	common = devm_kzalloc(sdev->dev, sizeof(*common), GFP_KERNEL);
>> +	if (!common)
>> +		return dev_err_probe(sdev->dev, -ENOMEM,
>> +				     "failed to allocate common data\n");
>> +
>> +	common->ipc_dev = platform_device_register_data(sdev->dev, "imx-dsp",
>> +							PLATFORM_DEVID_NONE,
>> +							pdev, sizeof(*pdev));
>> +	if (IS_ERR(common->ipc_dev))
>> +		return dev_err_probe(sdev->dev, PTR_ERR(common->ipc_dev),
>> +				     "failed to create IPC device\n");
>> +
>> +	/* let the devres API take care of unregistering this platform
>> +	 * driver when no longer required.
>> +	 */
> I remember only network subsystem use this type comments style.

will fix in V2

>
>> +	ret = devm_add_action_or_reset(sdev->dev,
>> +				       imx_unregister_action,
>> +				       common->ipc_dev);
>> +	if (ret)
>> +		return dev_err_probe(sdev->dev, ret, "failed to add devm action\n");
>> +
>> +	common->ipc_handle = dev_get_drvdata(&common->ipc_dev->dev);
>> +	if (!common->ipc_handle)
>> +		return dev_err_probe(sdev->dev, -EPROBE_DEFER,
>> +				     "failed to fetch IPC handle\n");
>> +
>> +	ret = imx_parse_ioremap_memory(sdev);
>> +	if (ret < 0)
>> +		return dev_err_probe(sdev->dev, ret,
>> +				     "failed to parse/ioremap memory regions\n");
>> +
>> +	if (get_chip_info(sdev)->has_dma_reserved) {
>> +		ret = of_reserved_mem_device_init_by_name(sdev->dev,
>> +							  pdev->dev.of_node,
>> +							  "dma");
> do you need put "of_reserved_mem_device_release()" at imx_unregister_action?
>
> The below error path will miss call of_reserved_mem_device_release().

good catch, thx. Fix in V2.