Re: [alsa-devel] [PATCH V2 02/10] ASoC: img: Add driver for I2S input controller

Re: [alsa-devel] [PATCH V2 02/10] ASoC: img: Add driver for I2S input controller

[Mark Brown]

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

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

> +static inline u32 img_i2s_in_ch_disable(struct img_i2s_in *i2s, u32 chan)
> +{
> +	u32 reg;
> +
> +	reg = img_i2s_in_ch_readl(i2s, chan, IMG_I2S_IN_CH_CTL);
> +	reg &= ~IMG_I2S_IN_CH_CTL_ME_MASK;
> +	img_i2s_in_ch_writel(i2s, chan, reg, IMG_I2S_IN_CH_CTL);
> +
> +	return reg;
> +}
> +
> +static inline void img_i2s_in_ch_enable(struct img_i2s_in *i2s, u32 chan,
> +					u32 reg)
> +{
> +	reg |= IMG_I2S_IN_CH_CTL_ME_MASK;
> +	img_i2s_in_ch_writel(i2s, chan, reg, IMG_I2S_IN_CH_CTL);
> +}

The APIs here all seem a bit odd - for example the enable API taking a
register value as an argument (normally reg is a register address BTW)
and returning a value but the disable API doing a read/modify/write
cycle.

> +static inline void img_i2s_in_flush(struct img_i2s_in *i2s)
> +{
> +	int i;
> +	u32 reg;
> +
> +	for (i = 0; i < i2s->active_channels; i++) {
> +		reg = img_i2s_in_ch_disable(i2s, i);
> +		reg |= IMG_I2S_IN_CH_CTL_FIFO_FLUSH_MASK;
> +		img_i2s_in_ch_writel(i2s, i, reg, IMG_I2S_IN_CH_CTL);
> +		reg &= ~IMG_I2S_IN_CH_CTL_FIFO_FLUSH_MASK;
> +		img_i2s_in_ch_writel(i2s, i, reg, IMG_I2S_IN_CH_CTL);
> +		img_i2s_in_ch_enable(i2s, i, reg);
> +	}
> +}

This all seems to be connected to this, which is itself slightly funky
especially in the context of the only user...

> +	case SNDRV_PCM_TRIGGER_STOP:
> +	case SNDRV_PCM_TRIGGER_SUSPEND:
> +	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
> +		reg = img_i2s_in_readl(i2s, IMG_I2S_IN_CTL);
> +		reg &= ~IMG_I2S_IN_CTL_ME_MASK;
> +		img_i2s_in_writel(i2s, reg, IMG_I2S_IN_CTL);
> +		img_i2s_in_flush(i2s);
> +		break;

...which looks like it'll enable everything, then disable and reenable.
Plus needing to do a flush on trigger seems weird.

> +	if ((channels < 2) ||
> +			(channels > (i2s->max_i2s_chan * 2)) ||
> +			(channels % 2))
> +		return -EINVAL;

This indentation is very weird.

> +	control_mask = (u32)(~IMG_I2S_IN_CTL_16PACK_MASK &
> +			~IMG_I2S_IN_CTL_ACTIVE_CHAN_MASK);

> +	chan_control_mask = (u32)(~IMG_I2S_IN_CH_CTL_16PACK_MASK &
> +			~IMG_I2S_IN_CH_CTL_FEN_MASK &
> +			~IMG_I2S_IN_CH_CTL_FMODE_MASK &
> +			~IMG_I2S_IN_CH_CTL_SW_MASK &
> +			~IMG_I2S_IN_CH_CTL_FW_MASK &
> +			~IMG_I2S_IN_CH_CTL_PACKH_MASK);

This also looks very odd.  Normally we'd write masks as being the valid
bits and or them together.

> +	i2s->clk_sys = devm_clk_get(dev, "sys");
> +	if (IS_ERR(i2s->clk_sys))
> +		return PTR_ERR(i2s->clk_sys);

Please print an error message so people can tell why things failed.

> +	rst = devm_reset_control_get(dev, "rst");
> +	if (IS_ERR(rst)) {
> +		dev_dbg(dev, "No top level reset found\n");

You should check for -EPROBE_DEFER here and just return the error here
if you get it (on the basis that the reset framework ought to be using a
different error if there's nothing bound in DT).

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

Re: [alsa-devel] [PATCH V2 03/10] ASoC: img: Add binding document for I2S output controller

[Mark Brown]

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

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

> +Optional Properties:

> +  - interrupts : Contains the I2S out interrupts. Depending on
> +	the configuration, there may be no interrupts, one interrupt,
> +	or an interrupt per I2S channel

If there is an interrupt per channel how should they be described in DT?

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

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

[Mark Brown]

[-- 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 --]

Re: [alsa-devel] [PATCH V2 08/10] ASoC: img: Add driver for SPDIF input controller

[Mark Brown]

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

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

> +static int img_spdif_in_get_lock_acquire(struct snd_kcontrol *kcontrol,
> +				  struct snd_ctl_elem_value *ucontrol)
> +{
> +	struct snd_soc_dai *cpu_dai = snd_kcontrol_chip(kcontrol);
> +	struct img_spdif_in *spdif = snd_soc_dai_get_drvdata(cpu_dai);
> +
> +	ucontrol->value.integer.value[0] = spdif->lock_acquire;
> +
> +	return 0;
> +}

It would be nice to get some documentation of what these controls are...

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

Re: [alsa-devel] [PATCH V2 00/10] Add support for Imagination Technologies audio controllers

[Mark Brown]

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

On Mon, Oct 12, 2015 at 01:40:27PM +0100, Damien Horsley wrote:
> From: "Damien.Horsley" <Damien.Horsley@imgtec.com>
> 
> Add drivers and binding documents for the following Imagination Technologies audio controllers:

This all looks mostly good, I had a few fairly minor comments which
should be fairly easy to address, mostly stylistic.

Please also fix your mail client to word wrap within paragraphs at
something substantially less than 80 columns.  Doing this makes your
messages much easier to read and reply to.

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

Re: [alsa-devel] [PATCH V2 02/10] ASoC: img: Add driver for I2S input controller

[Damien Horsley]

On 19/10/15 18:47, Mark Brown wrote:
> On Mon, Oct 12, 2015 at 01:40:29PM +0100, Damien Horsley wrote:
> 
>> +static inline u32 img_i2s_in_ch_disable(struct img_i2s_in *i2s, u32 chan)
>> +{
>> +	u32 reg;
>> +
>> +	reg = img_i2s_in_ch_readl(i2s, chan, IMG_I2S_IN_CH_CTL);
>> +	reg &= ~IMG_I2S_IN_CH_CTL_ME_MASK;
>> +	img_i2s_in_ch_writel(i2s, chan, reg, IMG_I2S_IN_CH_CTL);
>> +
>> +	return reg;
>> +}
>> +
>> +static inline void img_i2s_in_ch_enable(struct img_i2s_in *i2s, u32 chan,
>> +					u32 reg)
>> +{
>> +	reg |= IMG_I2S_IN_CH_CTL_ME_MASK;
>> +	img_i2s_in_ch_writel(i2s, chan, reg, IMG_I2S_IN_CH_CTL);
>> +}
> 
> The APIs here all seem a bit odd - for example the enable API taking a
> register value as an argument (normally reg is a register address BTW)
> and returning a value but the disable API doing a read/modify/write
> cycle.
>

Sure. It reduces the number of register accesses this way, but the
difference in execution time is not significant. Would you prefer these
to both do read-modify-writes?

>> +static inline void img_i2s_in_flush(struct img_i2s_in *i2s)
>> +{
>> +	int i;
>> +	u32 reg;
>> +
>> +	for (i = 0; i < i2s->active_channels; i++) {
>> +		reg = img_i2s_in_ch_disable(i2s, i);
>> +		reg |= IMG_I2S_IN_CH_CTL_FIFO_FLUSH_MASK;
>> +		img_i2s_in_ch_writel(i2s, i, reg, IMG_I2S_IN_CH_CTL);
>> +		reg &= ~IMG_I2S_IN_CH_CTL_FIFO_FLUSH_MASK;
>> +		img_i2s_in_ch_writel(i2s, i, reg, IMG_I2S_IN_CH_CTL);
>> +		img_i2s_in_ch_enable(i2s, i, reg);
>> +	}
>> +}
> 
> This all seems to be connected to this, which is itself slightly funky
> especially in the context of the only user...
> 

They are also used during hw_params and set_format.

>> +	case SNDRV_PCM_TRIGGER_STOP:
>> +	case SNDRV_PCM_TRIGGER_SUSPEND:
>> +	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
>> +		reg = img_i2s_in_readl(i2s, IMG_I2S_IN_CTL);
>> +		reg &= ~IMG_I2S_IN_CTL_ME_MASK;
>> +		img_i2s_in_writel(i2s, reg, IMG_I2S_IN_CTL);
>> +		img_i2s_in_flush(i2s);
>> +		break;
> 
> ...which looks like it'll enable everything, then disable and reenable.
> Plus needing to do a flush on trigger seems weird.
> 

If the FIFOs are not flushed, some samples from the previous stream will
be transferred to the user application when the block is started again

>> +	if ((channels < 2) ||
>> +			(channels > (i2s->max_i2s_chan * 2)) ||
>> +			(channels % 2))
>> +		return -EINVAL;
> 
> This indentation is very weird.
> 

Ok. What is the correct indentation for this?

>> +	control_mask = (u32)(~IMG_I2S_IN_CTL_16PACK_MASK &
>> +			~IMG_I2S_IN_CTL_ACTIVE_CHAN_MASK);
> 
>> +	chan_control_mask = (u32)(~IMG_I2S_IN_CH_CTL_16PACK_MASK &
>> +			~IMG_I2S_IN_CH_CTL_FEN_MASK &
>> +			~IMG_I2S_IN_CH_CTL_FMODE_MASK &
>> +			~IMG_I2S_IN_CH_CTL_SW_MASK &
>> +			~IMG_I2S_IN_CH_CTL_FW_MASK &
>> +			~IMG_I2S_IN_CH_CTL_PACKH_MASK);
> 
> This also looks very odd.  Normally we'd write masks as being the valid
> bits and or them together.
> 

Ok

>> +	i2s->clk_sys = devm_clk_get(dev, "sys");
>> +	if (IS_ERR(i2s->clk_sys))
>> +		return PTR_ERR(i2s->clk_sys);
> 
> Please print an error message so people can tell why things failed.
>

Ok


>> +	rst = devm_reset_control_get(dev, "rst");
>> +	if (IS_ERR(rst)) {
>> +		dev_dbg(dev, "No top level reset found\n");
> 
> You should check for -EPROBE_DEFER here and just return the error here
> if you get it (on the basis that the reset framework ought to be using a
> different error if there's nothing bound in DT).
> 

Ok

Re: [alsa-devel] [PATCH V2 03/10] ASoC: img: Add binding document for I2S output controller

[Damien Horsley]

On 19/10/15 18:56, Mark Brown wrote:
> On Mon, Oct 12, 2015 at 01:40:30PM +0100, Damien Horsley wrote:
> 
>> +Optional Properties:
> 
>> +  - interrupts : Contains the I2S out interrupts. Depending on
>> +	the configuration, there may be no interrupts, one interrupt,
>> +	or an interrupt per I2S channel
> 
> If there is an interrupt per channel how should they be described in DT?
> 

In ascending channel order. I will clarify this in the binding document.

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

[Damien Horsley]

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

Re: [alsa-devel] [PATCH V2 08/10] ASoC: img: Add driver for SPDIF input controller

[Damien Horsley]

On 19/10/15 19:27, Mark Brown wrote:
> On Mon, Oct 12, 2015 at 01:40:35PM +0100, Damien Horsley wrote:
> 
>> +static int img_spdif_in_get_lock_acquire(struct snd_kcontrol *kcontrol,
>> +				  struct snd_ctl_elem_value *ucontrol)
>> +{
>> +	struct snd_soc_dai *cpu_dai = snd_kcontrol_chip(kcontrol);
>> +	struct img_spdif_in *spdif = snd_soc_dai_get_drvdata(cpu_dai);
>> +
>> +	ucontrol->value.integer.value[0] = spdif->lock_acquire;
>> +
>> +	return 0;
>> +}
> 
> It would be nice to get some documentation of what these controls are...
> 

Ok

Re: [alsa-devel] [PATCH V2 02/10] ASoC: img: Add driver for I2S input controller

[Mark Brown]

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

On Thu, Oct 22, 2015 at 08:09:38PM +0100, Damien Horsley wrote:
> On 19/10/15 18:47, Mark Brown wrote:
> > On Mon, Oct 12, 2015 at 01:40:29PM +0100, Damien Horsley wrote:

> > The APIs here all seem a bit odd - for example the enable API taking a
> > register value as an argument (normally reg is a register address BTW)
> > and returning a value but the disable API doing a read/modify/write
> > cycle.

> Sure. It reduces the number of register accesses this way, but the
> difference in execution time is not significant. Would you prefer these
> to both do read-modify-writes?

I would prefer that the functions look consistent with each other and
ideally resemble common register acceess idioms in the kernel.

> >> +static inline void img_i2s_in_flush(struct img_i2s_in *i2s)
> >> +{
> >> +	int i;
> >> +	u32 reg;
> >> +
> >> +	for (i = 0; i < i2s->active_channels; i++) {
> >> +		reg = img_i2s_in_ch_disable(i2s, i);
> >> +		reg |= IMG_I2S_IN_CH_CTL_FIFO_FLUSH_MASK;
> >> +		img_i2s_in_ch_writel(i2s, i, reg, IMG_I2S_IN_CH_CTL);
> >> +		reg &= ~IMG_I2S_IN_CH_CTL_FIFO_FLUSH_MASK;
> >> +		img_i2s_in_ch_writel(i2s, i, reg, IMG_I2S_IN_CH_CTL);
> >> +		img_i2s_in_ch_enable(i2s, i, reg);
> >> +	}
> >> +}

> > This all seems to be connected to this, which is itself slightly funky
> > especially in the context of the only user...

> They are also used during hw_params and set_format.

My point is that the flush function has only one user.

> >> +	case SNDRV_PCM_TRIGGER_STOP:
> >> +	case SNDRV_PCM_TRIGGER_SUSPEND:
> >> +	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
> >> +		reg = img_i2s_in_readl(i2s, IMG_I2S_IN_CTL);
> >> +		reg &= ~IMG_I2S_IN_CTL_ME_MASK;
> >> +		img_i2s_in_writel(i2s, reg, IMG_I2S_IN_CTL);
> >> +		img_i2s_in_flush(i2s);
> >> +		break;

> > ...which looks like it'll enable everything, then disable and reenable.
> > Plus needing to do a flush on trigger seems weird.

> If the FIFOs are not flushed, some samples from the previous stream will
> be transferred to the user application when the block is started again

Shouldn't we be doing that flush on stream close instead?  If nothing
else the flush is going to discard a bit of data if the stream is just
paused.

> >> +	if ((channels < 2) ||
> >> +			(channels > (i2s->max_i2s_chan * 2)) ||
> >> +			(channels % 2))
> >> +		return -EINVAL;

> > This indentation is very weird.

> Ok. What is the correct indentation for this?

Align the continuation lines of the if condition with the first line.

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

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

[Mark Brown]

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

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.

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

Re: [alsa-devel] [PATCH V2 02/10] ASoC: img: Add driver for I2S input controller

[Damien Horsley]

On 23/10/15 23:57, Mark Brown wrote:
> On Thu, Oct 22, 2015 at 08:09:38PM +0100, Damien Horsley wrote:
>> On 19/10/15 18:47, Mark Brown wrote:
>>> On Mon, Oct 12, 2015 at 01:40:29PM +0100, Damien Horsley wrote:
> 
>>> The APIs here all seem a bit odd - for example the enable API taking a
>>> register value as an argument (normally reg is a register address BTW)
>>> and returning a value but the disable API doing a read/modify/write
>>> cycle.
> 
>> Sure. It reduces the number of register accesses this way, but the
>> difference in execution time is not significant. Would you prefer these
>> to both do read-modify-writes?
> 
> I would prefer that the functions look consistent with each other and
> ideally resemble common register acceess idioms in the kernel.
> 

Ok.

>>>> +static inline void img_i2s_in_flush(struct img_i2s_in *i2s)
>>>> +{
>>>> +	int i;
>>>> +	u32 reg;
>>>> +
>>>> +	for (i = 0; i < i2s->active_channels; i++) {
>>>> +		reg = img_i2s_in_ch_disable(i2s, i);
>>>> +		reg |= IMG_I2S_IN_CH_CTL_FIFO_FLUSH_MASK;
>>>> +		img_i2s_in_ch_writel(i2s, i, reg, IMG_I2S_IN_CH_CTL);
>>>> +		reg &= ~IMG_I2S_IN_CH_CTL_FIFO_FLUSH_MASK;
>>>> +		img_i2s_in_ch_writel(i2s, i, reg, IMG_I2S_IN_CH_CTL);
>>>> +		img_i2s_in_ch_enable(i2s, i, reg);
>>>> +	}
>>>> +}
> 
>>> This all seems to be connected to this, which is itself slightly funky
>>> especially in the context of the only user...
> 
>> They are also used during hw_params and set_format.
> 
> My point is that the flush function has only one user.
> 
>>>> +	case SNDRV_PCM_TRIGGER_STOP:
>>>> +	case SNDRV_PCM_TRIGGER_SUSPEND:
>>>> +	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
>>>> +		reg = img_i2s_in_readl(i2s, IMG_I2S_IN_CTL);
>>>> +		reg &= ~IMG_I2S_IN_CTL_ME_MASK;
>>>> +		img_i2s_in_writel(i2s, reg, IMG_I2S_IN_CTL);
>>>> +		img_i2s_in_flush(i2s);
>>>> +		break;
> 
>>> ...which looks like it'll enable everything, then disable and reenable.
>>> Plus needing to do a flush on trigger seems weird.
> 
>> If the FIFOs are not flushed, some samples from the previous stream will
>> be transferred to the user application when the block is started again
> 
> Shouldn't we be doing that flush on stream close instead?  If nothing
> else the flush is going to discard a bit of data if the stream is just
> paused.
> 

The FIFOs are only 8 frames in size, so I am not sure there is an
issue with these frames being lost.

I think it also makes sense to keep the blocks consistent with each
other. The spdif (out and in), and parallel out, all flush automatically
when stopped, and the fifo for the i2s out block is cleared when the
reset is asserted.

>>>> +	if ((channels < 2) ||
>>>> +			(channels > (i2s->max_i2s_chan * 2)) ||
>>>> +			(channels % 2))
>>>> +		return -EINVAL;
> 
>>> This indentation is very weird.
> 
>> Ok. What is the correct indentation for this?
> 
> Align the continuation lines of the if condition with the first line.
> 

Ok

Re: [alsa-devel] [PATCH V2 02/10] ASoC: img: Add driver for I2S input controller

[Mark Brown]

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

On Tue, Oct 27, 2015 at 01:55:27PM +0000, Damien Horsley wrote:
> On 23/10/15 23:57, Mark Brown wrote:

> > Shouldn't we be doing that flush on stream close instead?  If nothing
> > else the flush is going to discard a bit of data if the stream is just
> > paused.

> The FIFOs are only 8 frames in size, so I am not sure there is an
> issue with these frames being lost.

> I think it also makes sense to keep the blocks consistent with each
> other. The spdif (out and in), and parallel out, all flush automatically
> when stopped, and the fifo for the i2s out block is cleared when the
> reset is asserted.

This seems like an issue that got missed in the other drivers then.  I'd
expect the trigger operation to be a minimal operation which starts and
stops the data transfer, not doing anything else.

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

Re: [alsa-devel] [PATCH V2 02/10] ASoC: img: Add driver for I2S input controller

[Damien Horsley]

On 28/10/15 01:04, Mark Brown wrote:
> On Tue, Oct 27, 2015 at 01:55:27PM +0000, Damien Horsley wrote:
>> On 23/10/15 23:57, Mark Brown wrote:
> 
>>> Shouldn't we be doing that flush on stream close instead?  If nothing
>>> else the flush is going to discard a bit of data if the stream is just
>>> paused.
> 
>> The FIFOs are only 8 frames in size, so I am not sure there is an
>> issue with these frames being lost.
> 
>> I think it also makes sense to keep the blocks consistent with each
>> other. The spdif (out and in), and parallel out, all flush automatically
>> when stopped, and the fifo for the i2s out block is cleared when the
>> reset is asserted.
> 
> This seems like an issue that got missed in the other drivers then.  I'd
> expect the trigger operation to be a minimal operation which starts and
> stops the data transfer, not doing anything else.
> 

The spdif out, spdif in, and parallel out blocks auto-flush whenever
they are stopped. It is not possible for software to prevent this behavior.

Re: [alsa-devel] [PATCH V2 02/10] ASoC: img: Add driver for I2S input controller

[Mark Brown]

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

On Wed, Oct 28, 2015 at 09:18:20PM +0000, Damien Horsley wrote:
> On 28/10/15 01:04, Mark Brown wrote:

> >> I think it also makes sense to keep the blocks consistent with each
> >> other. The spdif (out and in), and parallel out, all flush automatically
> >> when stopped, and the fifo for the i2s out block is cleared when the
> >> reset is asserted.

> > This seems like an issue that got missed in the other drivers then.  I'd
> > expect the trigger operation to be a minimal operation which starts and
> > stops the data transfer, not doing anything else.

> The spdif out, spdif in, and parallel out blocks auto-flush whenever
> they are stopped. It is not possible for software to prevent this behavior.

Oh, so this isn't the drivers doing this?  In that case it's fine for
them to do that, if it's what the hardware does it's what the hardware
does.  It sounded like you were saying that there was similar code in
the other drivers.

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

Re: [alsa-devel] [PATCH V2 02/10] ASoC: img: Add driver for I2S input controller

[Damien Horsley]

On 28/10/15 23:43, Mark Brown wrote:
> On Wed, Oct 28, 2015 at 09:18:20PM +0000, Damien Horsley wrote:
>> On 28/10/15 01:04, Mark Brown wrote:
> 
>>>> I think it also makes sense to keep the blocks consistent with each
>>>> other. The spdif (out and in), and parallel out, all flush automatically
>>>> when stopped, and the fifo for the i2s out block is cleared when the
>>>> reset is asserted.
> 
>>> This seems like an issue that got missed in the other drivers then.  I'd
>>> expect the trigger operation to be a minimal operation which starts and
>>> stops the data transfer, not doing anything else.
> 
>> The spdif out, spdif in, and parallel out blocks auto-flush whenever
>> they are stopped. It is not possible for software to prevent this behavior.
> 
> Oh, so this isn't the drivers doing this?  In that case it's fine for
> them to do that, if it's what the hardware does it's what the hardware
> does.  It sounded like you were saying that there was similar code in
> the other drivers.
> 

For the I2S In, there is another issue with flushing on stream close. If
the stream is stopped, then reconfigured to use a larger number of
channels (without the stream being closed), then the per-channel fifos
will become inconsistent with each other. The new channels will have no
samples in their FIFOs, while the others may contain samples from the
previous stream.

Would hw_params be the correct place to flush instead?

Re: [alsa-devel] [PATCH V2 02/10] ASoC: img: Add driver for I2S input controller

[Mark Brown]

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

On Thu, Oct 29, 2015 at 03:42:59PM +0000, Damien Horsley wrote:

> For the I2S In, there is another issue with flushing on stream close. If
> the stream is stopped, then reconfigured to use a larger number of
> channels (without the stream being closed), then the per-channel fifos
> will become inconsistent with each other. The new channels will have no
> samples in their FIFOs, while the others may contain samples from the
> previous stream.

> Would hw_params be the correct place to flush instead?

Yes, you could flush there (or in both places for that matter).

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