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

From: Mark Brown <broonie@kernel.org>
To: "Mukunda, Vijendar" <Vijendar.Mukunda@amd.com>
Cc: "tiwai@suse.de" <tiwai@suse.de>,
	"Deucher, Alexander" <Alexander.Deucher@amd.com>,
	"alsa-devel@alsa-project.org" <alsa-devel@alsa-project.org>
Subject: Re: [PATCH v3 08/14] ASoC: amd: add ACP PDM DMA driver dai ops
Date: Tue, 19 May 2020 12:40:12 +0100	[thread overview]
Message-ID: <20200519114012.GH4611@sirena.org.uk> (raw)
In-Reply-To: <DM6PR12MB2633954F20E044F44CA89D5497B90@DM6PR12MB2633.namprd12.prod.outlook.com>

[-- Attachment #1: Type: text/plain, Size: 1250 bytes --]

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.

> > > +	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().

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]