Re: [alsa-devel] [PATCH V2 06/10] ASoC: img: Add driver for parallel output controller

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 1401 bytes --]

On Mon, Oct 12, 2015 at 01:40:33PM +0100, Damien Horsley wrote:

> +	spin_lock_irqsave(&prl->lock, flags);
> +	reg = img_prl_out_readl(prl, IMG_PRL_OUT_CTL);
> +	ucontrol->value.integer.value[0] = !!(reg & IMG_PRL_OUT_CTL_EDGE_MASK);
> +	spin_unlock_irqrestore(&prl->lock, flags);

Do you need to lock a single register read?

> +static struct snd_kcontrol_new img_prl_out_controls[] = {
> +	{
> +		.iface = SNDRV_CTL_ELEM_IFACE_PCM,
> +		.name = "Parallel Out Edge Falling",
> +		.info = img_prl_out_edge_info,
> +		.get = img_prl_out_get_edge,
> +		.put = img_prl_out_set_edge
> +	}
> +};

If this is a boolean control (it looked like one) it should be called
Switch but it's not clear to me what exactly is being controlled here or
why it's something that should be exposed to userspace.

> +	switch (cmd) {
> +	case SNDRV_PCM_TRIGGER_START:
> +	case SNDRV_PCM_TRIGGER_RESUME:
> +	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
> +		reg = img_prl_out_readl(prl, IMG_PRL_OUT_CTL);
> +		reg |= IMG_PRL_OUT_CTL_ME_MASK;
> +		img_prl_out_writel(prl, reg, IMG_PRL_OUT_CTL);
> +		prl->active = true;
> +		break;
> +	case SNDRV_PCM_TRIGGER_STOP:
> +	case SNDRV_PCM_TRIGGER_SUSPEND:
> +	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
> +		img_prl_out_reset(prl);
> +		prl->active = false;
> +		break;

No need for locking on the reset (and doesn't this mean that the control
state is going to be changed underneath userspace)?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]