[AMD Official Use Only - Internal Distribution Only] > -----Original Message----- > From: Mark Brown <broonie@xxxxxxxxxx> > Sent: Tuesday, May 19, 2020 5:10 PM > To: Mukunda, Vijendar <Vijendar.Mukunda@xxxxxxx> > Cc: alsa-devel@xxxxxxxxxxxxxxxx; tiwai@xxxxxxx; Deucher, Alexander > <Alexander.Deucher@xxxxxxx> > 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.
