Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 20 Jul 2022 12:44:18 +0100
From:   Paul Cercueil <paul@crapouillou.net>
Subject: Re: [PATCH v4 01/11] ASoC: jz4740-i2s: Handle independent FIFO flush
 bits
To:     Aidan MacDonald <aidanmacdonald.0x0@gmail.com>
Cc:     lgirdwood@gmail.com, broonie@kernel.org, perex@perex.cz,
        tiwai@suse.com, linux-mips@vger.kernel.org,
        alsa-devel@alsa-project.org, linux-kernel@vger.kernel.org
Message-Id: <UXHBFR.6W2XPUNX040K1@crapouillou.net>
In-Reply-To: <20220708160244.21933-2-aidanmacdonald.0x0@gmail.com>
References: <20220708160244.21933-1-aidanmacdonald.0x0@gmail.com>
        <20220708160244.21933-2-aidanmacdonald.0x0@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1; format=flowed
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Hi Aidan,

Le ven., juil. 8 2022 at 17:02:34 +0100, Aidan MacDonald=20
<aidanmacdonald.0x0@gmail.com> a =E9crit :
> 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.
>=20
> 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.
>=20
> 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(-)
>=20
> diff --git a/sound/soc/jz4740/jz4740-i2s.c=20
> 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)
>=20
> +#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=20
JZ_AIC_CTRL_RLUSH.

> +
>  #define JZ_AIC_CTRL_OUTPUT_SAMPLE_SIZE_OFFSET 19
>  #define JZ_AIC_CTRL_INPUT_SAMPLE_SIZE_OFFSET  16
>=20
> @@ -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;
>  };
>=20
>  struct jz4740_i2s {
> @@ -124,12 +129,33 @@ static int jz4740_i2s_startup(struct=20
> snd_pcm_substream *substream,
>  	uint32_t conf, ctrl;
>  	int ret;
>=20
> +	/*
> +	 * When we can flush FIFOs independently, only flush the
> +	 * FIFO that is starting up.
> +	 */
> +	if (!i2s->soc_info->shared_fifo_flush) {
> +		ctrl =3D jz4740_i2s_read(i2s, JZ_REG_AIC_CTRL);
> +
> +		if (substream->stream =3D=3D SNDRV_PCM_STREAM_PLAYBACK)
> +			ctrl |=3D JZ4760_AIC_CTRL_TFLUSH;
> +		else
> +			ctrl |=3D 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=20
before the (snd_soc_dai_active(dai)) check, and the other is checked=20
after?

You could do something like this:

ctrl =3D jz4740_i2s_read(i2s, JZ_REG_AIC_CTRL);

if (i2s->soc_info->shared_fifo_flush ||
    substream->stream =3D=3D SNDRV_PCM_STREAM_PLAYBACK) {
    ctrl |=3D JZ_AIC_CTRL_TFLUSH;
} else {
    ctrl |=3D JZ_AIC_CTRL_RFLUSH;
}

jz4740_i2s_write(i2s, JZ_REG_AIC_CTRL, ctrl);

Cheers,
-Paul

> +
>  	if (snd_soc_dai_active(dai))
>  		return 0;
>=20
> -	ctrl =3D jz4740_i2s_read(i2s, JZ_REG_AIC_CTRL);
> -	ctrl |=3D 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 =3D jz4740_i2s_read(i2s, JZ_REG_AIC_CTRL);
> +		ctrl |=3D JZ_AIC_CTRL_FLUSH;
> +		jz4740_i2s_write(i2s, JZ_REG_AIC_CTRL, ctrl);
> +	}
>=20
>  	ret =3D clk_prepare_enable(i2s->clk_i2s);
>  	if (ret)
> @@ -444,6 +470,7 @@ static struct snd_soc_dai_driver jz4740_i2s_dai =3D=20
> {
>  static const struct i2s_soc_info jz4740_i2s_soc_info =3D {
>  	.version =3D JZ_I2S_JZ4740,
>  	.dai =3D &jz4740_i2s_dai,
> +	.shared_fifo_flush =3D true,
>  };
>=20
>  static const struct i2s_soc_info jz4760_i2s_soc_info =3D {
> --
> 2.35.1
>=20