[PATCH v3 08/14] ASoC: amd: add ACP PDM DMA driver dai ops

[Mukunda, Vijendar]

[AMD Official Use Only - Internal Distribution Only]



> -----Original Message-----
> From: Mark Brown <broonie at kernel.org>
> Sent: Tuesday, May 19, 2020 5:10 PM
> To: Mukunda, Vijendar <Vijendar.Mukunda at amd.com>
> Cc: alsa-devel at alsa-project.org; tiwai at suse.de; Deucher, Alexander
> <Alexander.Deucher at amd.com>
> Subject: Re: [PATCH v3 08/14] ASoC: amd: add ACP PDM DMA driver dai ops
> 
> On Tue, May 19, 2020 at 11:37:39AM +0000, Mukunda, Vijendar wrote:
> 
> > > > +	case TWO_CH:
> > > > +	default:
> > > > +		ch_mask = 0x00;
> > > > +		break;
> > > > +	}
> 
> > > The TWO_CH define isn't adding anything, and I'd expect there to be
> > > invalid channel configurations this is rejecting - at the minute this
> > > just boils down to an assignment.
> 
> > Currently we have added two channel support.
> > As of today, as we restricted no of channels to 2 , there is no point
> > to check invalid configuration.
> > It kept for future expansion to support more than two channels.
> 
> You should still return an error here, if nothing else it ensures that
> this gets updated when support for other configurations is added.

Will fix it and submit the incremental patch.
> 
> > > > +	config_pdm_stream_params(ch_mask, rtd->acp_base);
> 
> > > Does this function have any other callers - is there a need for it to be
> > > a separate function?
> 
> > Current ask is only to support 48Khz, 2 Channel streams.
> > This is kept for future reference.
> > This API works as place holder to expand the logic to support multiple
> > sample rates and no of channels.
> > Even we can discard this API , do in it calling API itself.
> 
> Even when you support more configurations these will be configured from
> hw_params().

Agreed. Will fix it and post a incremental patch.