[alsa-devel] [PATCH v2 4/8] ASoC: Intel: add kcontrol to enable/disable sound effect module waves

[Mark Brown]

On Wed, Mar 11, 2015 at 07:07:12AM +0000, Lu, Han wrote:

Again, please fix your mailer to word wrap within paragraphs - this mail
is very difficult to read.

> > On Tue, Mar 10, 2015 at 10:41:23AM +0800, han.lu at intel.com wrote:
> > 
> > > +static int hsw_waves_switch_put(struct snd_kcontrol *kcontrol,
> > > +				struct snd_ctl_elem_value *ucontrol) {
> > > +	struct snd_soc_platform *platform =
> > snd_soc_kcontrol_platform(kcontrol);
> > > +	struct hsw_priv_data *pdata =
> > snd_soc_platform_get_drvdata(platform);
> > > +	struct sst_hsw *hsw = pdata->hsw;
> > > +	int ret = 0;
> > > +	enum sst_hsw_module_id id = SST_HSW_MODULE_WAVES;
> > > +	bool switch_on = (bool)ucontrol->value.integer.value[0];

> > > +	if (sst_hsw_is_module_loaded(hsw, id)) {
> > > +		if (switch_on == sst_hsw_is_module_active(hsw, id))
> > > +			return 0;
> > > +
> > > +		/* ipc is valid only when module is loaded */
> > > +		if (switch_on)
> > > +			ret = sst_hsw_module_enable(hsw, id, 0);
> > > +		else
> > > +			ret = sst_hsw_module_disable(hsw, id, 0);
> > > +	}

> > I'm not clear how this setting is saved when the module is not in RAM on the
> > DSP.  If the module enable/disable calls do that that's fine but I'd expect the
> > function to return an error if the module isn't loaded (or better yet for the
> > control not to be made visible to userspace at all) - we shouldn't be silently
> > ignoring what the user is setting.

> [Han] if module is not in RAM on the DSP, sst_hsw_is_module_loaded()
> will return false. Also sst_hsw_module_enable() and
> sst_hsw_module_disable() will check the module status in DSP before
> send IPC commands to DSP.  In case sst_hsw_is_module_loaded() return
> false, (eg. System go RTD3 and modules in DSP RAM be unloaded) the
> user setting will be stored in variant u32 enabled_modules_rtd3, which
> is implemented in patch 0005. So which is better for next version of
> patch, to add error return here, merge patch 0005 with 0004, or  do
> not change this part? Thanks.

It's better to merge the two patches so that it's clear that everything
is working fine - in general the idea is that things should work after
every patch and people shouldn't be finding problems in the patch series
which are fixed later on in the same series.
-------------- 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/20150311/cab9130c/attachment.sig>