lkml.org 
[lkml]   [2015]   [Oct]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [alsa-devel] [PATCH V2 06/10] ASoC: img: Add driver for parallel output controller
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)?
[unhandled content-type:application/pgp-signature]
\
 
 \ /
  Last update: 2015-10-19 20:21    [W:0.164 / U:0.028 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site