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

[Vinod Koul <vinod.koul@intel.com>]

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

On Sat, Aug 15, 2015 at 07:46:46AM -0700, Mark Brown wrote:
> 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.  :/

Ok

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

Ok will update

> > > 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.
Ok will add latter

-- 
~Vinod

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

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