Re: [PATCH 3/5] ASoC: amd: Enabling two I2S instances

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
Date: Thu, 26 Sep 2019 11:15:38 -0700	[thread overview]
Message-ID: <20190926181538.GC2036@sirena.org.uk> (raw)
In-Reply-To: <1569539290-756-3-git-send-email-Vishnuvardhanrao.Ravulapati@amd.com>

[-- Attachment #1: Type: text/plain, Size: 2135 bytes --]

On Fri, Sep 27, 2019 at 04:37:37AM +0530, Ravulapati Vishnu vardhan rao wrote:
> 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)
>  
>  	case SND_SOC_DAIFMT_I2S:
>  		adata->tdm_mode = false;
> -		break;
> +	break;
>  	case SND_SOC_DAIFMT_DSP_A:
> -			adata->tdm_mode = true;
> -			break;
> +		adata->tdm_mode = 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 = rv_readl(adata->acp3x_base + mmACP_BTTDM_IRER);
>  	rv_writel((val | 0x2), adata->acp3x_base + mmACP_BTTDM_IRER);
>  
> +	val = rv_readl(adata->acp3x_base + mmACP_I2STDM_ITER);
> +	rv_writel((val | 0x2), adata->acp3x_base + mmACP_I2STDM_ITER);
> +	val = rv_readl(adata->acp3x_base + mmACP_I2STDM_IRER);
> +	rv_writel((val | 0x2), adata->acp3x_base + mmACP_I2STDM_IRER);
> +
>  	val = (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);
>  
>  	adata->tdm_fmt = val;
>  	return 0;

Won't this configure all the interfaces identically?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]