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

From: Mark Brown <broonie@kernel.org>
To: Vinod Koul <vinod.koul@intel.com>
Cc: patches.audio@intel.com, liam.r.girdwood@linux.intel.com,
	alsa-devel@alsa-project.org,
	"Subhransu S. Prusty" <subhransu.s.prusty@intel.com>,
	Jeeja KP <jeeja.kp@intel.com>
Subject: Re: [PATCH v2 1/9] ASoC: Intel: Skylake: Add pipe and modules handlers
Date: Sat, 19 Sep 2015 09:00:23 -0700	[thread overview]
Message-ID: <20150919160023.GC30445@sirena.org.uk> (raw)
In-Reply-To: <1439832404-12424-2-git-send-email-vinod.koul@intel.com>

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

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?).

I'm sure I've raised this before.

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

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

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

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

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

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