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: 3889 bytes --]

On Sat, Aug 15, 2015 at 07:36:38AM -0700, Mark Brown wrote:
> On Sat, Aug 15, 2015 at 07:12:08PM +0530, Vinod Koul wrote:
> > On Fri, Aug 14, 2015 at 10:43:36PM +0100, Mark Brown wrote:
> 
> > > > +	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.
> 
> And we couldn't connect in any other way?  This case is fine but I'm
> worrying about what happens if that connection gets made later - we
> won't get another PMU event since the widget is already powered.

Yes you are right this won't be handled here, but will be handled in source
pipe it is connecting to. The DAPM event will be called for that pipe, and
there we check if source or sink pipe is already running which is clue to
driver that we should do bind there rather than here

I will add that part as well in event handler comments

> > > > +			/* 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
> 
> This is the sort of thing where the whole lack of documentation thing
> that I keep going on about becomes really important - there's a limited
> amount of time I can spend on any individual patch series and so things
> that need to be reverse engineered are just going to get queried a lot
> of the time (and even if they get reverse engineered the feedback is
> often going to be that the code needs to be clearer).

Okay, I did try to add comments to help understand but looks like I still
have more work to do. Will try to add these details as well

> > 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...
> 
> So this is about generating a notification to say which routes are being
> disconnected?  That seems like a fairly general thing, I can imagine
> that being quite common for DSPs - indeed it seems essential given that
> we won't always trigger any power changes on disconnection.

Okay fair point. Would it be okay to carry this approach here for now and
with later DAPM updates for next kernel cycle?

> 
> > > > +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.
> 
> There's certainly a lot of visual similarity there.

Oh yes, they do look a lot similar...


-- 
~Vinod

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

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