Re: [PATCH v6 05/10] ASoC: Intel: mrfld: add DSP core controls

[Mark Brown <broonie@kernel.org>]

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

On Tue, Sep 09, 2014 at 03:11:28PM +0530, Subhransu S. Prusty wrote:

> +	mutex_lock(&drv->lock);
> +	map = is_tx ? sst_ssp_channel_map : sst_ssp_slot_map;
> +
> +	val = 1 << ctl_no;
> +	mux = ucontrol->value.enumerated.item[0];
> +	if (mux > e->max - 1) {
> +		mutex_unlock(&drv->lock);
> +		return -EINVAL;
> +	}

This is still in parameter validation so we don't need to have the lock
yet at all.

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

This is returning with the lock still held AFAICT.  I'm a bit surprised
that we don't need to interact with the hardware if we've disabled
everything, shouldn't this have some effect on the hardware?

Also the coding style thing with the comments again.

> +static void sst_find_and_send_pipe_algo(struct sst_data *drv,
> +					const char *pipe, struct sst_ids *ids)
> +{
> +	struct sst_algo_control *bc;
> +	struct sst_module *algo = NULL;
> +
> +	dev_dbg(&drv->pdev->dev, "Enter: widget=%s\n", pipe);
> +
> +	list_for_each_entry(algo, &ids->algo_list, node) {
> +		bc = (void *)algo->kctl->private_value;
> +
> +		dev_dbg(&drv->pdev->dev, "Found algo control name=%s pipe=%s\n",
> +				algo->kctl->id.name, pipe);
> +		sst_send_algo_cmd(drv, bc);
> +	}
> +}

I'm not seeing any handling if we failed to find anything here.

> +	[SST_IP_MEDIA2]		= SST_SWM_IN_MEDIA2,
> +	[SST_IP_MEDIA3]		= SST_SWM_IN_MEDIA3,
> +};
> +static void sst_set_pipe_gain(struct sst_ids *ids,

Coding style - no blank line.  This is quite common in this patch and
something that's been an issue in previous versions of this series too.

> +	if (enable)
> +		sst->ops->power(sst->dev, true);
> +
> +	mutex_lock(&drv->lock);
> +	if (enable)
> +		timer_usage++;
> +	else
> +		timer_usage--;

This seems a bit strange.  We're doing the enable outside the lock, then
taking the lock, then doing some refcounting outside the lock.

> +	mutex_unlock(&drv->lock);
> +
> +	if (!enable)
> +		sst->ops->power(sst->dev, false);

Same thing here.  How does the power management work here, it doesn't
seem joined up?  I'd really like to see some comments explaining what's
going on with this stuff.

> +static bool is_sst_dapm_widget(struct snd_soc_dapm_widget *w)
> +{
> +	if ((w->id == snd_soc_dapm_pga) ||
> +	    (w->id == snd_soc_dapm_aif_in) ||
> +	    (w->id == snd_soc_dapm_aif_out) ||
> +	    (w->id == snd_soc_dapm_input) ||
> +	    (w->id == snd_soc_dapm_output) ||
> +	    (w->id == snd_soc_dapm_mixer))
> +		return true;
> +	else
> +		return false;
> +}

Use a switch statement please.

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

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