>>> +struct sdw_dma_dev_data { >>> + void __iomem *acp_base; >>> + struct mutex *acp_lock; /* used to protect acp common register access */ >>> +}; >>> + >>> /** >>> * struct acp63_dev_data - acp pci driver context >>> * @acp63_base: acp mmio base >>> diff --git a/sound/soc/amd/ps/ps-sdw-dma.c b/sound/soc/amd/ps/ps-sdw-dma.c >>> new file mode 100644 >>> index 000000000000..f41849fd035c >>> --- /dev/null >>> +++ b/sound/soc/amd/ps/ps-sdw-dma.c >>> @@ -0,0 +1,70 @@ >>> +// SPDX-License-Identifier: GPL-2.0+ >>> +/* >>> + * AMD ALSA SoC Pink Sardine SoundWire DMA Driver >>> + * >>> + * Copyright 2023 Advanced Micro Devices, Inc. >>> + */ >>> + >>> +#include <linux/err.h> >>> +#include <linux/io.h> >>> +#include <linux/module.h> >>> +#include <linux/platform_device.h> >>> +#include <sound/pcm_params.h> >>> +#include <sound/soc.h> >>> +#include <sound/soc-dai.h> >>> +#include "acp63.h" >>> + >>> +#define DRV_NAME "amd_ps_sdw_dma" >>> + >>> +static const struct snd_soc_component_driver acp63_sdw_component = { >>> + .name = DRV_NAME, >>> +}; >>> + >>> +static int acp63_sdw_platform_probe(struct platform_device *pdev) >>> +{ >>> + struct resource *res; >>> + struct sdw_dma_dev_data *sdw_data; >>> + int status; >>> + >>> + if (!pdev->dev.platform_data) { >>> + dev_err(&pdev->dev, "platform_data not retrieved\n"); >>> + return -ENODEV; >>> + } >>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >>> + if (!res) { >>> + dev_err(&pdev->dev, "IORESOURCE_MEM FAILED\n"); >>> + return -ENODEV; >>> + } >>> + >>> + sdw_data = devm_kzalloc(&pdev->dev, sizeof(*sdw_data), GFP_KERNEL); >>> + if (!sdw_data) >>> + return -ENOMEM; >>> + >>> + sdw_data->acp_base = devm_ioremap(&pdev->dev, res->start, resource_size(res)); >>> + if (!sdw_data->acp_base) >>> + return -ENOMEM; >>> + >>> + sdw_data->acp_lock = pdev->dev.platform_data; >> so you are sharing the same lock between parent and child platform device? > Initially, we thought of sharing the same lock between parent and child > platform devices. Later we have observed, mutex hang issues observed. If the goal is a global lock, then the platform data should contain a pointer to the lock. We used this for Intel, see .e.g. the shim_mask in drivers/soundwire/intel_init.c, where the same pointer is used by all children. > > We have avoided critical section code and removed acp_lock from > ACP SoundWire DMA driver while accessing ACP common registers. > We will remove mutex lock from ACP SoundWire DMA driver code. >> Does this work? IIRC the platform_data is copied, you do not point >> directly to the initial data provided by the parent. We had issues with >> SoundWire when we used platform devices, with the 'wrong' pointer used. > Till now, we haven't observed mutex hang issues due to > ACP PDM driver mutex lock changes. > Agreed. We will remove the mutex code from ACP PDM driver as > well and we will refactor code. > In SoundWire manager driver, we are sharing the same copy for two > manager instances. We haven't observed any issue. What's the benefit of passing this lock as platform_data, if the goal is to perform mutual exclusion between the two manager instances? Why not just create the lock as part of the SoundWire probe? If there was no need for a lock, then please remove it :-) If it's needed, please describe what it protects, which agents rely on it and how the lock is shared. >> >> The documentation does make mention of a copy.... >> >> /** >> * platform_device_add_data - add platform-specific data to a platform >> device >> * @pdev: platform device allocated by platform_device_alloc to add >> resources to >> * @data: platform specific data for this platform device >> * @size: size of platform specific data >> * >> * Add a copy of platform specific data to the platform device's >> * platform_data pointer. The memory associated with the platform data >> * will be freed when the platform device is released. >> */ >>> + dev_set_drvdata(&pdev->dev, sdw_data); >>> + status = devm_snd_soc_register_component(&pdev->dev, >>> + &acp63_sdw_component, >>> + NULL, 0); >>> + if (status) >>> + dev_err(&pdev->dev, "Fail to register sdw dma component\n"); >>> + >>> + return status; >>> +}