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

[Mark Brown]

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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: Digital signature
URL: <http://mailman.alsa-project.org/pipermail/alsa-devel/attachments/20150919/8f7a7647/attachment.sig>