Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1250982 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [alsa-devel] [PATCH V2 06/10] ASoC: img: Add driver for parallel  output controller Mark Brown <broonie@kernel.org> - 2015-10-19 20:10 +0200
    Re: [alsa-devel] [PATCH V2 06/10] ASoC: img: Add driver for parallel  output controller Damien Horsley <Damien.Horsley@imgtec.com> - 2015-10-22 21:30 +0200
      Re: [alsa-devel] [PATCH V2 06/10] ASoC: img: Add driver for parallel  output controller Mark Brown <broonie@kernel.org> - 2015-10-24 01:00 +0200

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

[Multipart message — attachments visible in raw view] — view raw

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)?

[toc] | [next] | [standalone]

#1254131

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1255044

[Multipart message — attachments visible in raw view] — view raw

On Thu, Oct 22, 2015 at 08:21:03PM +0100, Damien Horsley wrote:
> 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

Add a comment so readers know this.

> >> +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

Yes, that's part of the DAI format - and we definitely don't want users
randomly changing this at runtime, the CODEC and CPU need to be
configured together.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web