[alsa-devel] [PATCH v7 3/7] ASoC: Intel: mrfld: add DSP core controls

[Mark Brown]

On Fri, Sep 19, 2014 at 04:46:04PM +0530, Subhransu S. Prusty wrote:

> +static int sst_slot_get(struct snd_kcontrol *kcontrol,
> +                       struct snd_ctl_elem_value *ucontrol)
> +{
> +       struct sst_enum *e = (void *)kcontrol->private_value;
> +       struct snd_soc_component *component = snd_kcontrol_chip(kcontrol);
> +       unsigned int ctl_no = e->reg;
> +       unsigned int is_tx = e->tx;
> +       unsigned int val, mux;
> +       u8 *map = is_tx ? sst_ssp_channel_map : sst_ssp_slot_map;

So the "channel" map is for transmit and the "slot" map is for receive?
That naming doesn't seem at all obvious, I'd expect some confusion and
resulting bugs there.  

This is a really big block of code and there's lots of things like this
in the code which may well *work* but don't seem robust - not flagging
up easy to detect errors for example - and I've been spotting things
like locking trouble as well.  This is all setting off alarm bells which
is worrying.

> +	mutex_lock(&drv->lock);
> +	/* first clear all registers of this bit */
> +	for (i = 0; i < e->max; i++)
> +		map[i] &= ~val;
> +
> +	if (mux == 0) {/* kctl set to 'none' */
> +		mutex_unlock(&drv->lock);
> +		return 0;
> +	}

It's still not at all clear to me why we don't need to interact with the
hardware if we're settinng this to zero.  AFAICT from the previous
discussion this comment at the top of the file:

> + *  In the dpcm driver modelling when a particular FE/BE/Mixer/Pipe is active
> + *  we forward the settings and parameters, rest we keep the values  in
> + *  driver and forward when DAPM enables them

is supposed to explain what's going on but since it's nowhere near the
code it's unlikely that people will have seen it and it's not terribly
easy to relate to the code here.  As far as I can tell the theory is
that something is going to trigger a power down of the DSP and it won't
matter but if this is the case it isn't at all clear from the immediate
code.

> +	if (type == SST_MODULE_GAIN) {
> +		struct sst_gain_mixer_control *mc = (void *)kctl->private_value;
> +
> +		mc->w = w;
> +		module->kctl = kctl;
> +		list_add_tail(&module->node, &ids->gain_list);
> +	} else if (type == SST_MODULE_ALGO) {
> +		struct sst_algo_control *bc = (void *)kctl->private_value;
> +
> +		bc->w = w;
> +		module->kctl = kctl;
> +		list_add_tail(&module->node, &ids->algo_list);
> +	}

This looks like it should be a switch statement with a default case to
trap any errors.

> +		if (idx == NULL)
> +			continue;
> +		index  = strlen(kctl->id.name) - strlen(idx);

I keep on seeing lots of code with random double instead of single
spaces.

> +		if (strstr(kctl->id.name, "Volume") &&
> +		    !strncmp(kctl->id.name, w->name, index))
> +			ret = sst_fill_module_list(kctl, w, SST_MODULE_GAIN);
> +
> +		else if (strstr(kctl->id.name, "params") &&
> +			 !strncmp(kctl->id.name, w->name, index))
> +			ret = sst_fill_module_list(kctl, w, SST_MODULE_ALGO);
> +
> +		else if (strstr(kctl->id.name, "Switch") &&
> +			 !strncmp(kctl->id.name, w->name, index)) {
> +			struct sst_gain_mixer_control *mc =
> +						(void *)kctl->private_value;
> +
> +			mc->w = w;
> +
> +		} else if (strstr(kctl->id.name, "interleaver") &&

Both or no branches of an if statement should use { }.

> +			 !strncmp(kctl->id.name, w->name, index)) {
> +			struct sst_enum *e = (void *)kctl->private_value;
> +
> +			e->w = w;
> +
> +		} else if (strstr(kctl->id.name, "deinterleaver") &&
> +			 !strncmp(kctl->id.name, w->name, index)) {
> +
> +			struct sst_enum *e = (void *)kctl->private_value;
> +
> +			e->w = w;
> +		}

Again no fallthrough case.
-------------- 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/20140925/165daf01/attachment-0001.sig>