Re: [PATCH v2 1/9] ASoC: Intel: Skylake: Add pipe and modules handlers

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

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

On Sat, Sep 19, 2015 at 09:00:23AM -0700, Mark Brown wrote:
> On Mon, Aug 17, 2015 at 10:56:36PM +0530, Vinod Koul wrote:
> 
> > +static int skl_tplg_bind_unbind_pipes(struct skl_module_cfg *src_module,
> > +	struct skl_module_cfg *sink_module, struct skl_sst *ctx, bool bind)
> > +{
> > +	int ret;
> > +
> > +	if (!bind) {
> > +		ret = skl_stop_pipe(ctx, src_module->pipe);
> > +		if (ret < 0)
> > +			return ret;
> > +
> > +		ret = skl_unbind_modules(ctx, src_module, sink_module);
> > +	} else {
> > +		ret = skl_bind_modules(ctx, src_module, sink_module);
> > +	}
> > +
> > +	return ret;
> > +}
> 
> Can we *please* stop having these functions which optionally do several
> different things with nothing at all shared between the different paths?
> It just adds a layer of indentation in the function and makes everything
> harder to read (including at the call site - how does the reader know if
> a given call will bind or unbind?).

Well while reading based on the argument bind we would know if we are doing
bind or unbind. While binding we call only bind. On unbind we have to stop
and then unbind.

I will remove this and few other wrappers like this which will help in
readability and reviews

> 
> I'm sure I've raised this before.
Sorry to miss that, will fix this time for sure


> > +static bool skl_tplg_is_pipe_mem_available(struct skl *skl,
> > +                               struct skl_module_cfg *mconfig)
> 
> I'm not seeing any users of this function (unlike the mcps checker).

It is called in skl_tplg_mixer_dapm_pre_pmu_event() which is in Patch 3 of
this series.

> > +	if (skl->resource.mem + mconfig->pipe->memory_pages > skl->resource.max_mem) {
> > +		dev_err(ctx->dev, "exceeds ppl memory available=%d > mem=%d\n",
> > +				skl->resource.max_mem, skl->resource.mem);
> > +		return false;
> > +	}
> 
> I'm not sure how the user is going to be able to tell what exceeded the
> maximum memory here.  

Module Id and Instance Id are printed before this but yes that is debug, so
merging the two to error alone makes sense, will do that here


> > +static bool skl_tplg_is_pipe_mcps_available(struct skl *skl,
> > +				struct skl_module_cfg *mconfig)
> 
> This looks an awful lot like the memory check.  Can we make a struct or
> ideally array for the constraints and then have a single function which
> checks them all?

No it only two checks, one for MCPS and one for memory. We cannot have
single function as they are checked at two places. Due to FW construction.
MCPS is property of each module whereas memory is for complete pipe.
For each module while initialization we check MCPS whereas for pipe creation
we check memory


> > +	list_for_each_entry(p, &w->sinks, list_source) {
> > +		if ((p->sink->priv == NULL)
> > +			&& (!is_skl_dsp_widget_type(w)))
> > +			continue;
> > +
> > +		if ((p->sink->priv != NULL) && (p->connect)
> > +			&& (is_skl_dsp_widget_type(p->sink))) {
> 
> This is harder to read with the extra brackets.
Will fix this

-- 
~Vinod

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

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