Messages in this thread Patch in this message | | | Date | Wed, 20 Jul 2022 12:44:18 +0100 | From | Paul Cercueil <> | Subject | Re: [PATCH v4 01/11] ASoC: jz4740-i2s: Handle independent FIFO flush bits |
| |
Hi Aidan,
Le ven., juil. 8 2022 at 17:02:34 +0100, Aidan MacDonald <aidanmacdonald.0x0@gmail.com> a écrit : > On the JZ4740, there is a single bit that flushes (empties) both > the transmit and receive FIFO. Later SoCs have independent flush > bits for each FIFO, which allows us to flush the right FIFO when > starting up a stream. > > This also fixes a bug: since we were only setting the JZ4740's > flush bit, which corresponds to the TX FIFO flush bit on other > SoCs, other SoCs were not having their RX FIFO flushed at all. > > Fixes: 967beb2e8777 ("ASoC: jz4740: Add jz4780 support") > Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com> > --- > sound/soc/jz4740/jz4740-i2s.c | 33 ++++++++++++++++++++++++++++++--- > 1 file changed, 30 insertions(+), 3 deletions(-) > > diff --git a/sound/soc/jz4740/jz4740-i2s.c > b/sound/soc/jz4740/jz4740-i2s.c > index ecd8df70d39c..576f31f9d734 100644 > --- a/sound/soc/jz4740/jz4740-i2s.c > +++ b/sound/soc/jz4740/jz4740-i2s.c > @@ -64,6 +64,9 @@ > #define JZ_AIC_CTRL_ENABLE_PLAYBACK BIT(1) > #define JZ_AIC_CTRL_ENABLE_CAPTURE BIT(0) > > +#define JZ4760_AIC_CTRL_TFLUSH BIT(8) > +#define JZ4760_AIC_CTRL_RFLUSH BIT(7)
Just rename JZ_AIC_CTRL_FLUSH to JZ_AIC_CTRL_TFLUSH and introduce JZ_AIC_CTRL_RLUSH.
> + > #define JZ_AIC_CTRL_OUTPUT_SAMPLE_SIZE_OFFSET 19 > #define JZ_AIC_CTRL_INPUT_SAMPLE_SIZE_OFFSET 16 > > @@ -90,6 +93,8 @@ enum jz47xx_i2s_version { > struct i2s_soc_info { > enum jz47xx_i2s_version version; > struct snd_soc_dai_driver *dai; > + > + bool shared_fifo_flush; > }; > > struct jz4740_i2s { > @@ -124,12 +129,33 @@ static int jz4740_i2s_startup(struct > snd_pcm_substream *substream, > uint32_t conf, ctrl; > int ret; > > + /* > + * When we can flush FIFOs independently, only flush the > + * FIFO that is starting up. > + */ > + if (!i2s->soc_info->shared_fifo_flush) { > + ctrl = jz4740_i2s_read(i2s, JZ_REG_AIC_CTRL); > + > + if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) > + ctrl |= JZ4760_AIC_CTRL_TFLUSH; > + else > + ctrl |= JZ4760_AIC_CTRL_RFLUSH; > + > + jz4740_i2s_write(i2s, JZ_REG_AIC_CTRL, ctrl); > + }
Wouldn't it be simpler to do one single if/else? And hy is one checked before the (snd_soc_dai_active(dai)) check, and the other is checked after?
You could do something like this:
ctrl = jz4740_i2s_read(i2s, JZ_REG_AIC_CTRL);
if (i2s->soc_info->shared_fifo_flush || substream->stream == SNDRV_PCM_STREAM_PLAYBACK) { ctrl |= JZ_AIC_CTRL_TFLUSH; } else { ctrl |= JZ_AIC_CTRL_RFLUSH; }
jz4740_i2s_write(i2s, JZ_REG_AIC_CTRL, ctrl);
Cheers, -Paul
> + > if (snd_soc_dai_active(dai)) > return 0; > > - ctrl = jz4740_i2s_read(i2s, JZ_REG_AIC_CTRL); > - ctrl |= JZ_AIC_CTRL_FLUSH; > - jz4740_i2s_write(i2s, JZ_REG_AIC_CTRL, ctrl); > + /* > + * When there is a shared flush bit for both FIFOs we can > + * only flush the FIFOs if no other stream has started. > + */ > + if (i2s->soc_info->shared_fifo_flush) { > + ctrl = jz4740_i2s_read(i2s, JZ_REG_AIC_CTRL); > + ctrl |= JZ_AIC_CTRL_FLUSH; > + jz4740_i2s_write(i2s, JZ_REG_AIC_CTRL, ctrl); > + } > > ret = clk_prepare_enable(i2s->clk_i2s); > if (ret) > @@ -444,6 +470,7 @@ static struct snd_soc_dai_driver jz4740_i2s_dai = > { > static const struct i2s_soc_info jz4740_i2s_soc_info = { > .version = JZ_I2S_JZ4740, > .dai = &jz4740_i2s_dai, > + .shared_fifo_flush = true, > }; > > static const struct i2s_soc_info jz4760_i2s_soc_info = { > -- > 2.35.1 >
| |