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

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

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

On Sat, Sep 19, 2015 at 09:11:22AM -0700, Mark Brown wrote:
> On Mon, Aug 17, 2015 at 10:56:38PM +0530, Vinod Koul wrote:
> 
> > +	if (list_empty(&s_pipe->w_list)) {
> > +		ret = skl_tplg_get_pipe_widget(ctx->dev, w, s_pipe);
> > +		if (ret < 0)
> > +			return ret;
> > +	}
> 
> I'm not entirely clear what this is supposed to do or if it does it
> correctly - I suspect it's trying to always create the pipe widget which
> might be a bit clearer if it just created the pipe widget.  The fact
> that it's checking if a list is empty is a bit worrying, what if some
> but not all of the entries needed on that list are there?  Is just
> getting a widget really sufficient initialisation?

For quick reference, on first run of a pipe, we create a w_list of all
widgets in that pipe. This list is not freed on PMD event as widgets within
a pipe are static. This saves us cycles to get widgets in pipe every time.

So if we already initialized all the widgets of a pipeline we don't need
that anymore. I will add comment for this as well

> 
> > +	return skl_tplg_bind_unbind_pipe_modules(ctx, s_pipe, true);
> > +}
> > +/*
> > + * A PGA represents a module in a pipeline. So in the Pre-PMU event of PGA
> 
> Blank line between functions and the next thing.  I am getting fed up of
> seeing this.
Will fix these

Thanks
-- 
~Vinod

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

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