Re: [PATCH v2 11/13] ASoC: Intel: mrfld: add the DSP DAPM widgets

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

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

On Fri, Jul 18, 2014 at 07:56:23PM +0530, Vinod Koul wrote:
> On Fri, Jul 18, 2014 at 01:19:20PM +0100, Mark Brown wrote:
> > On Thu, Jul 10, 2014 at 10:14:55AM +0530, Subhransu S. Prusty wrote:
> > 
> > > +	pr_debug("%s: widget = %s\n", __func__, w->name);
> > > +	for (i = 0; i < w->num_kcontrols; i++) {
> > > +		if (dapm_kcontrol_get_value(w->kcontrols[i])) {
> > > +			mc = (struct soc_mixer_control *)(w->kcontrols[i])->private_value;
> > > +			val |= 1 << mc->shift;
> > > +		}
> > > +	}
> > 
> > So, this is the usage of dapm_kcontrol_get_value() (quite a way away
> > from the patch exporting it!).  The usage here looks *very* strange.
> > We're calling the function but treating the result as a boolean and
> > manually decoding the DAPM data structures in order to get the control
> > shift...  that's odd to say the least.
> > 
> > > +	SST_FILL_DESTINATION(2, cmd.output_id,
> > > +			     ids->location_id, SST_DEFAULT_MODULE_ID);
> > > +	cmd.nb_inputs =	fill_swm_input(&cmd.input[0], val);
> > 
> > So what we're doing here is parsing the controls to get which inputs are
> > enabled...  it's not altogether clear to me that we shouldn't be doing
> > this at control update time.  Presumably we'll also need to be sending
> > these messages when the controls are updated to account for changes that
> > happen while streams are active.
> yes thats the idea. Here we send as mixer path is On. Yes the mixer update
> if On does send this as well.

Wanted to check if there is anything else required to be done for thsi
series, if not can you pls merge the ones before this in series, if okay.

Thanks
-- 
~Vinod

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

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