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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 26 Sep 2019 11:15:38 -0700
From:   Mark Brown <broonie@...nel.org>
To:     Ravulapati Vishnu vardhan rao 
        <Vishnuvardhanrao.Ravulapati@....com>
Cc:     Alexander.Deucher@....com, Liam Girdwood <lgirdwood@...il.com>,
        Jaroslav Kysela <perex@...ex.cz>,
        Takashi Iwai <tiwai@...e.com>,
        Vijendar Mukunda <vijendar.mukunda@....com>,
        Maruthi Bayyavarapu <maruthi.bayyavarapu@....com>,
        YueHaibing <yuehaibing@...wei.com>,
        "Gustavo A. R. Silva" <gustavo@...eddedor.com>,
        "moderated list:SOUND - SOC LAYER / DYNAMIC AUDIO POWER MANAGEM..." 
        <alsa-devel@...a-project.org>,
        open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 3/5] ASoC: amd: Enabling two I2S instances

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?

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)