Messages in this thread | ![/](/images/icornerl.gif) | | Date | Sat, 24 Oct 2015 07:57:23 +0900 | From | Mark Brown <> | Subject | Re: [alsa-devel] [PATCH V2 02/10] ASoC: img: Add driver for I2S input controller |
| |
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. [unhandled content-type:application/pgp-signature] | ![\](/images/icornerr.gif) |