>>> @@ -464,16 +488,79 @@ static int acp63_sdw_platform_probe(struct platform_device *pdev) >>> status = devm_snd_soc_register_component(&pdev->dev, >>> &acp63_sdw_component, >>> NULL, 0); >>> - if (status) >>> + if (status) { >>> dev_err(&pdev->dev, "Fail to register sdw dma component\n"); >>> + return status; >>> + } >>> + pm_runtime_set_autosuspend_delay(&pdev->dev, ACP_SUSPEND_DELAY_MS); >>> + pm_runtime_use_autosuspend(&pdev->dev); >>> + pm_runtime_enable(&pdev->dev); >>> + pm_runtime_allow(&pdev->dev); >> Can you remind me why you need the pm_runtime_allow()? I can't recall >> where the _forbid() is done. > We have used pm_runtime_allow() to allow the device immediately > enter runtime suspend state. Yes you are correct. If we use pm_runtime_allow(), > then in remove sequence we should use pm_runtime_forbid call. >> Also is there not a pm_runtime_set_active() missing? > > > We will change the sequence as mentioned below. > > in probe sequence , we will use > > pm_runtime_set_autosuspend_delay(&pdev->dev, ACP_SUSPEND_DELAY_MS); > pm_runtime_use_autosuspend(&pdev->dev); > pm_runtime_mark_last_busy(&pdev->dev); > pm_runtime_set_active(&pdev->dev); > pm_runtime_enable(&pdev->dev); > > In remove sequence > > pm_runtime_disable(&pdev->dev); sounds about right. >> >>> + return 0; >>> +} >>> >>> - return status; >>> +static int acp63_sdw_platform_remove(struct platform_device *pdev) >>> +{ >>> + pm_runtime_disable(&pdev->dev); >>> + return 0; >>> } >>> >>> +static int __maybe_unused acp63_sdw_pcm_resume(struct device *dev) >>> +{ >>> + struct sdw_dma_dev_data *sdw_data; >>> + struct acp_sdw_dma_stream *stream; >>> + struct snd_pcm_runtime *runtime; >>> + u32 period_bytes, buf_size, water_mark_size_reg; >>> + int ret; >>> + int index; >>> + >>> + sdw_data = dev_get_drvdata(dev); >>> + for (index = 0; index < ACP63_SDW0_DMA_MAX_STREAMS; index++) { >>> + if (sdw_data->sdw0_dma_stream[index] && >>> + sdw_data->sdw0_dma_stream[index]->runtime) { >>> + water_mark_size_reg = sdw0_dma_ring_buf_reg[index].water_mark_size_reg; >>> + runtime = sdw_data->sdw0_dma_stream[index]->runtime; >>> + stream = runtime->private_data; >>> + period_bytes = frames_to_bytes(runtime, runtime->period_size); >>> + buf_size = frames_to_bytes(runtime, runtime->buffer_size); >>> + acp63_config_dma(stream, sdw_data->acp_base, index); >>> + ret = acp63_configure_sdw_ringbuffer(sdw_data->acp_base, index, >>> + buf_size, ACP_SDW0); >>> + if (ret) >>> + return ret; >>> + writel(period_bytes, sdw_data->acp_base + water_mark_size_reg); >>> + } >>> + } >>> + for (index = 0; index < ACP63_SDW1_DMA_MAX_STREAMS; index++) { >>> + if (sdw_data->sdw1_dma_stream[index] && >>> + sdw_data->sdw1_dma_stream[index]->runtime) { >>> + water_mark_size_reg = sdw1_dma_ring_buf_reg[index].water_mark_size_reg; >>> + runtime = sdw_data->sdw1_dma_stream[index]->runtime; >>> + stream = runtime->private_data; >>> + period_bytes = frames_to_bytes(runtime, runtime->period_size); >>> + buf_size = frames_to_bytes(runtime, runtime->buffer_size); >>> + acp63_config_dma(stream, sdw_data->acp_base, index); >>> + ret = acp63_configure_sdw_ringbuffer(sdw_data->acp_base, index, >>> + buf_size, ACP_SDW1); >>> + if (ret) >>> + return ret; >>> + writel(period_bytes, sdw_data->acp_base + water_mark_size_reg); >>> + } >>> + } >> Isn't this set of configurations something that needs to be done already >> somewhere else, i.e. could there be a common helper? > In hw_params() callback, we are setting period_bytes and buf_size from > params structure. We are extracting same variables from runtime structures > in resume() callback. > We can implement a helper function to further simplify above logic > instead of having two separate loops. ok