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.