Re: [PATCH 12/13] ASoC: amd: acp: Add pci legacy driver support for acp7.0 platform

[syed saba kareem <ssabakar@amd.com>]

On 10/23/23 13:20, Amadeusz Sławiński wrote:
> On 10/21/2023 4:50 PM, Syed Saba Kareem wrote:
>> Add pci legacy driver support and create platform driver for
>> acp7.0 platform.
>>
>> Signed-off-by: Syed Saba Kareem <Syed.SabaKareem@amd.com>
>> ---
>
> ...
>
>> +
>> +static struct snd_soc_dai_driver acp70_dai[] = {
>> +{
>> +    .name = "acp-i2s-sp",
>> +    .id = I2S_SP_INSTANCE,
>> +    .playback = {
>> +        .stream_name = "I2S SP Playback",
>> +        .rates = SNDRV_PCM_RATE_8000_96000,
>> +        .formats = SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S8 |
>> +               SNDRV_PCM_FMTBIT_U8 | SNDRV_PCM_FMTBIT_S32_LE,
>
> Any reason to not go from lowest bit width to higher? Similarly in 
> further definitions.
>  This has to be corrected, will push the changes as an incremental patch.
>> +        .channels_min = 2,
>> +        .channels_max = 8,
>> +        .rate_min = 8000,
>> +        .rate_max = 96000,
>> +    },
>
> ...
>
>> +
>> +static int __maybe_unused acp70_pcm_resume(struct device *dev)
>> +{
>> +    struct acp_dev_data *adata = dev_get_drvdata(dev);
>> +    struct acp_stream *stream;
>> +    struct snd_pcm_substream *substream;
>> +    snd_pcm_uframes_t buf_in_frames;
>> +    u64 buf_size;
>> +
>> +    spin_lock(&adata->acp_lock);
>> +    list_for_each_entry(stream, &adata->stream_list, list) {
>> +        if (stream) {
>> +            substream = stream->substream;
>> +            if (substream && substream->runtime) {
>> +                buf_in_frames = (substream->runtime->buffer_size);
>> +                buf_size = frames_to_bytes(substream->runtime, 
>> buf_in_frames);
>> +                config_pte_for_stream(adata, stream);
>> +                config_acp_dma(adata, stream, buf_size);
>> +                if (stream->dai_id)
>> +                    restore_acp_i2s_params(substream, adata, stream);
>> +                else
>> +                    restore_acp_pdm_params(substream, adata);
>> +            }
>> +        }
>> +    }
>> +        spin_unlock(&adata->acp_lock);
>> +        return 0;
>
> Indentation is wrong in above two lines.
>   This has to be corrected, will push the changes as an incremental 
> patch.
>> +}
>> +
>> +static const struct dev_pm_ops acp70_dma_pm_ops = {
>> +    SET_SYSTEM_SLEEP_PM_OPS(NULL, acp70_pcm_resume)
>> +};
>> +
>
>