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

Groups > linux.kernel > #1255045

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

Show all headers | View raw

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

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.

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

Re: [alsa-devel] [PATCH V2 02/10] ASoC: img: Add driver for I2S  input controller Mark Brown <broonie@kernel.org> - 2015-10-19 19:50 +0200
  Re: [alsa-devel] [PATCH V2 02/10] ASoC: img: Add driver for I2S input  controller Damien Horsley <Damien.Horsley@imgtec.com> - 2015-10-22 21:10 +0200
    Re: [alsa-devel] [PATCH V2 02/10] ASoC: img: Add driver for I2S  input controller Mark Brown <broonie@kernel.org> - 2015-10-24 01:00 +0200
      Re: [alsa-devel] [PATCH V2 02/10] ASoC: img: Add driver for I2S input  controller Damien Horsley <Damien.Horsley@imgtec.com> - 2015-10-27 15:00 +0100
        Re: [alsa-devel] [PATCH V2 02/10] ASoC: img: Add driver for I2S  input controller Mark Brown <broonie@kernel.org> - 2015-10-28 02:10 +0100
          Re: [alsa-devel] [PATCH V2 02/10] ASoC: img: Add driver for I2S input  controller Damien Horsley <Damien.Horsley@imgtec.com> - 2015-10-28 22:20 +0100
            Re: [alsa-devel] [PATCH V2 02/10] ASoC: img: Add driver for I2S  input controller Mark Brown <broonie@kernel.org> - 2015-10-29 00:50 +0100
              Re: [alsa-devel] [PATCH V2 02/10] ASoC: img: Add driver for I2S input  controller Damien Horsley <Damien.Horsley@imgtec.com> - 2015-10-29 16:50 +0100
                Re: [alsa-devel] [PATCH V2 02/10] ASoC: img: Add driver for I2S  input controller Mark Brown <broonie@kernel.org> - 2015-10-30 02:30 +0100

csiph-web