[alsa-devel] [PATCH 3/9] ASoC: Intel: Skylake: add DSP platform widget event handlers

[Mark Brown]

On Sat, Aug 08, 2015 at 01:06:18AM +0530, Subhransu S. Prusty wrote:

> +	/* If its not SKL DSP type dont handle */
> +	if (source->priv == NULL &&  (!is_skl_dsp_widget_type(w)))
> +		return 0;

Why would this event be assigned to a widget that is not a suitable
type, and what if it's an unsuitable widget type that happens ton have
private data?  I'm not sure I understand the priv check.

You've also got a typo with "don't" and an extra space after the &&.

> +	list_for_each_entry(p, &w->sinks, list_source) {
> +		if (!p->connect)
> +			continue;

Hrm.  Do we handle routing changes well?

> +			/* Add connected path to one global list */
> +			path_list->dapm_path = p;
> +			list_add_tail(&path_list->node, &skl->dapm_path_list);
> +			break;

We have our own private path list here which includes the DAPM path...
can't we outsource some of this to the topology and DAPM code?  I'm a
little fuzzy on what the list is for, it appears to be used mainly for
deallocation though it's called dapm_path_list (which is a bit worrying).

> +static int skl_tplg_mixer_dapm_post_pmu_event(struct snd_soc_dapm_widget *w,
> +								struct skl *skl)
> +{

This looks a lot like the previous function, there's similar duplication
in the other event functions.

> +	}
> +	if (src_pipe_started) {

Missing blank line here.

> +/*
> + * In modelling, we assume ther will be one mixer in a pipeline. If a second
> + * one is required that is created as another pipe entity.
> + * The mixer is resposible for pipe management and represent a pipeline
> + * instance
> + */

You mean to say you assume there will be *exactly* one mixer?
-------------- 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/20150814/7d0eb1dd/attachment.sig>