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

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

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

On Fri, Aug 14, 2015 at 10:43:36PM +0100, Mark Brown wrote:
> 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.

Yes rethinking now I don't see why this would be called for events we don't
register as you pointed. I will recheck this bit and get back.

> You've also got a typo with "don't" and an extra space after the &&.
Oops, will fix. Thanks for pointing.

> > +	list_for_each_entry(p, &w->sinks, list_source) {
> > +		if (!p->connect)
> > +			continue;
> 
> Hrm.  Do we handle routing changes well?

Yes we do :)
This is the pmu handler for a mixer widget. Here we need to send Bind event
to firmware for the paths which are connected, so if a path is not connected
we don't need to send bind event so we skip that.

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

If you look at the usage of the dapm_path_list, it is used in pmd handler
for this mixer. For a mixer there can be many paths and we need to
disconnect the previously connected path. DAPM will disconnect and send
event, so we wont know which paths were disconnected and thats why we keep a
track if this by using this list. At PMU we keep adding and at PMD we check
the path which was disconnected and send the event for that.
This gives driver very quick lookup of which disconnect to send. I am not
sure if we should be changing framework for this...

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

Actually logic is quite different. The skl_tplg_mixer_dapm_post_pmu_event is
for mixer and is supposed to bind to source pipe and start
The previous one is for PGA and starts sink pipelines it is connected to and
then its own. It doesn't do bind.

Said that I will check and try to see if common code for checking and
starting can be factored out from these into a common helper

> 
> > +	}
> > +	if (src_pipe_started) {
> 
> Missing blank line here.
Will fix

> 
> > +/*
> > + * 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?
Yes :) and if we need another mixer then that will become another pipeline

Thanks
-- 
~Vinod

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

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