Re: [PATCH 4/9] ASoC: Intel: Skylake: Add FE and BE hw_params handling

From: Mark Brown <broonie@kernel.org>
To: Vinod Koul <vinod.koul@intel.com>
Cc: alsa-devel@alsa-project.org, tiwai@suse.de,
	liam.r.girdwood@linux.intel.com, patches.audio@intel.com,
	Jeeja KP <jeeja.kp@intel.com>,
	"Subhransu S. Prusty" <subhransu.s.prusty@intel.com>
Subject: Re: [PATCH 4/9] ASoC: Intel: Skylake: Add FE and BE hw_params handling
Date: Sat, 15 Aug 2015 07:46:46 -0700	[thread overview]
Message-ID: <20150815144646.GD10748@sirena.org.uk> (raw)
In-Reply-To: <20150815140014.GE13546@localhost>

[-- Attachment #1.1: Type: text/plain, Size: 2790 bytes --]

On Sat, Aug 15, 2015 at 07:30:14PM +0530, Vinod Koul wrote:
> On Fri, Aug 14, 2015 at 10:53:10PM +0100, Mark Brown wrote:
> > On Sat, Aug 08, 2015 at 01:06:19AM +0530, Subhransu S. Prusty wrote:

> > > +	/*
> > > +	 * 16 bit is 16 bit container whereas 24 bit is in 32 bit container so
> > > +	 * update bit depth accordingly
> > > +	 */
> > > +	if (format->valid_bit_depth == SKL_DEPTH_16BIT)
> > > +		format->bit_depth = format->valid_bit_depth;
> > > +	else if (format->valid_bit_depth == SKL_DEPTH_24BIT)
> > > +		format->bit_depth = SKL_DEPTH_32BIT;
> > > +

> > What if the depth is neither 16 bit nor 24 bit?  Shouldn't this just be
> > a simple override for the 24 bit case?

> These are the only two DSP supports, so am not sure that overriding will
> work with DSP. I think we should add else and error out.

Then write a switch statement.  :/

> > > +static int skl_tplg_be_set_params(struct snd_soc_dai *dai,
> > > +					struct snd_soc_dapm_widget *w,
> > > +					struct skl_pipe_params *params)
> > > +{
> > > +	if (!w->power) {
> > > +		dev_dbg(dai->dev, "set params for widget=%s\n", w->name);
> > > +		return skl_tplg_be_fill_pipe_params(dai, w->priv, params);
> > > +	}
> > > +
> > > +	return -EINVAL;
> > 
> > Shouldn't that be -EBUSY?  The normal idiom would be to check to see if
> > we were busy and error out rather than writing it as only configuing if
> > not busy (which looks like an error case now).
> 
> Not EBUSY. So here we are trying to set params for the pipe. So we would
> expect that pipe (widget) should be powered up by DAPM, but if that is not
> the case then we are in bad state. I am thinking EIO might suit this one better

Right, that's why I'm saying -EBUSY - we can't configure the pipe
because it is current in use.

> > There's some more common code here and the same patterns as above...  :/

> well the problem here is sink, source. Former was for source pipe using
> p->source, here we have sink pipe and using p->sink.
> Same here too that we either set params or for sink pipe

> Jeeja did try to make this common but it becomes quite hard, if you have
> a trick up your sleeve for this, am all ears :)

Well, I don't have the code any more.

> > > +	if (params->stream == SNDRV_PCM_STREAM_CAPTURE) {
> > > +		w = dai->capture_widget;
> > > +
> > > +		dev_dbg(dai->dev, "Stream name=%s\n", w->name);
> > > +		return skl_tplg_be_set_sink_pipe_params(dai, w, params);
> > > +	}

> > Normally that'd be written as an if/else, and the only difference
> > between the two cases here is which widget we pick...

> Yes we can change to else. Btw should we rather do if and if else, and else
> for error check, in case the value became bad. I am thinking latter.

Either works.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]