Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Thu, 26 Sep 2019 11:15:38 -0700
From:   Mark Brown <broonie@kernel.org>
To:     Ravulapati Vishnu vardhan rao 
        <Vishnuvardhanrao.Ravulapati@amd.com>
Cc:     Alexander.Deucher@amd.com, Liam Girdwood <lgirdwood@gmail.com>,
        Jaroslav Kysela <perex@perex.cz>,
        Takashi Iwai <tiwai@suse.com>,
        Vijendar Mukunda <vijendar.mukunda@amd.com>,
        Maruthi Bayyavarapu <maruthi.bayyavarapu@amd.com>,
        YueHaibing <yuehaibing@huawei.com>,
        "Gustavo A. R. Silva" <gustavo@embeddedor.com>,
        "moderated list:SOUND - SOC LAYER / DYNAMIC AUDIO POWER MANAGEM..." 
        <alsa-devel@alsa-project.org>,
        open list <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 3/5] ASoC: amd: Enabling two I2S instances
Message-ID: <20190926181538.GC2036@sirena.org.uk>
References: <1569539290-756-1-git-send-email-Vishnuvardhanrao.Ravulapati@amd.com>
 <1569539290-756-3-git-send-email-Vishnuvardhanrao.Ravulapati@amd.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
        protocol="application/pgp-signature"; boundary="8H5+QmNTcR9iGQhm"
Content-Disposition: inline
In-Reply-To: <1569539290-756-3-git-send-email-Vishnuvardhanrao.Ravulapati@amd.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


--8H5+QmNTcR9iGQhm
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Fri, Sep 27, 2019 at 04:37:37AM +0530, Ravulapati Vishnu vardhan rao wro=
te:
> RAVEN has multiple I2S instances:BT and SP.But only BT is enabled.
> Now I2S SP instance also gets enabled with this patch.

This is extremely difficult to review as is, the patch is very
large and the description very brief so it's hard to tell exactly
what issues there are that must be fixed to enable multiple I2S
interfaces.  My suggestion would be that this should be split
into a number of smaller patches, each making one logical change
with a clear description of what that specific change is.

A few specific comments below but really I didn't get very far
into the code due to the difficulty figuring out what's going on:

> @@ -46,10 +28,10 @@ static int acp3x_i2s_set_fmt(struct snd_soc_dai *cpu_=
dai, unsigned int fmt)
> =20
>  	case SND_SOC_DAIFMT_I2S:
>  		adata->tdm_mode =3D false;
> -		break;
> +	break;
>  	case SND_SOC_DAIFMT_DSP_A:
> -			adata->tdm_mode =3D true;
> -			break;
> +		adata->tdm_mode =3D true;
> +	break;
>  	default:
>  		return -EINVAL;
>  	}

For example this is a pure formatting change (one that moves
things away from the normal Linux coding style) and clearly not
related to the changelog.

> @@ -87,9 +69,16 @@ static int acp3x_i2s_set_tdm_slot(struct snd_soc_dai *=
cpu_dai, u32 tx_mask,
>  	val =3D rv_readl(adata->acp3x_base + mmACP_BTTDM_IRER);
>  	rv_writel((val | 0x2), adata->acp3x_base + mmACP_BTTDM_IRER);
> =20
> +	val =3D rv_readl(adata->acp3x_base + mmACP_I2STDM_ITER);
> +	rv_writel((val | 0x2), adata->acp3x_base + mmACP_I2STDM_ITER);
> +	val =3D rv_readl(adata->acp3x_base + mmACP_I2STDM_IRER);
> +	rv_writel((val | 0x2), adata->acp3x_base + mmACP_I2STDM_IRER);
> +
>  	val =3D (FRM_LEN | (slots << 15) | (slot_len << 18));
>  	rv_writel(val, adata->acp3x_base + mmACP_BTTDM_TXFRMT);
>  	rv_writel(val, adata->acp3x_base + mmACP_BTTDM_RXFRMT);
> +	rv_writel(val, adata->acp3x_base + mmACP_I2STDM_TXFRMT);
> +	rv_writel(val, adata->acp3x_base + mmACP_I2STDM_RXFRMT);
> =20
>  	adata->tdm_fmt =3D val;
>  	return 0;

Won't this configure all the interfaces identically?

--8H5+QmNTcR9iGQhm
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQEzBAABCgAdFiEEreZoqmdXGLWf4p/qJNaLcl1Uh9AFAl2NAEkACgkQJNaLcl1U
h9A/nQf/aCxpiLEUD1Kxl47cIVbozOcJU4QYNdUFGgPrUnoYCKI013RSszByaTzt
50c9wKCPY+rsQwVa0Vn9v7LiCDiq50OuEcmknCIFhKqtersy52YJLJB7pu9XNkTD
rfTJiMEIEgaFaa90HAtACaBEvtw9y8K9jqdiOvffmNJUWeMassdjR8GHEJ63U6aJ
OHjcOF3U2jQ49F04lt7EEioY7UNso5Ru4WbYlF4WPvDnZjzzReW9zWiXL0dhitHy
Iw478TOWlDmWbbOcLkzJi8EIqhfshzKxBOmU3JX649T+XghFqhI0s+9ZwWgzSIMF
bw5dMN3uhrrF0ExH7SRRoZPg2T3arg==
=Y62R
-----END PGP SIGNATURE-----

--8H5+QmNTcR9iGQhm--