On 19/10/15 19:07, Mark Brown wrote:
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?
Between the calls to reset_control_assert and reset_control_deassert, the block is held in reset. During this time, no register access will succeed. All register access that may occur concurrently with the reset needs to be locked
+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.
This controls the edge (rising/falling) of the frame clock that the samples are generated on. Should I create a set_fmt function and use SND_SOC_DAIFMT_NB_NF / SND_SOC_DAIFMT_NB_IF to set this instead? I wasn't sure if those formats were just for I2S or not
- 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)?
The reset function restores the setting after the reset