Re: [PATCH 4/8] ASoC: AMD: added condition checks for CZ specific code

From: Mark Brown <broonie@kernel.org>
To: Alex Deucher <alexdeucher@gmail.com>
Cc: alsa-devel@alsa-project.org, tiwai@suse.de, lgirdwood@gmail.com,
	amd-gfx@lists.freedesktop.org, rajeevkumar.linux@gmail.com,
	dri-devel@lists.freedesktop.org, Vijendar.Mukunda@amd.com,
	Alex Deucher <alexander.deucher@amd.com>,
	airlied@gmail.com
Subject: Re: [PATCH 4/8] ASoC: AMD: added condition checks for CZ specific code
Date: Wed, 28 Jun 2017 19:05:39 +0100	[thread overview]
Message-ID: <20170628180539.3uns3nudnjrjsx7o@sirena.org.uk> (raw)
In-Reply-To: <1498235706-31111-5-git-send-email-alexander.deucher@amd.com>

[-- Attachment #1.1: Type: text/plain, Size: 975 bytes --]

On Fri, Jun 23, 2017 at 12:35:02PM -0400, Alex Deucher wrote:

> Reviewed-by: Alex Deucher <alexander.deucher@amd.com>

> index dcbf997..e48ae5d 100644
> --- a/sound/soc/amd/acp-pcm-dma.c
> +++ b/sound/soc/amd/acp-pcm-dma.c
> @@ -34,6 +34,8 @@
>  
>  #define MAX_BUFFER (PLAYBACK_MAX_PERIOD_SIZE * PLAYBACK_MAX_NUM_PERIODS)
>  #define MIN_BUFFER MAX_BUFFER
> +#define CHIP_STONEY 14
> +#define CHIP_CARRIZO 13

These defines are being added in the middle of a file but CHIP_STONEY is
also used in another file in the previous patch (and apparently
extensively throughout the DRM driver already).  This is obviously not
good, we shouldn't have multiple copies of the definition.

> -	} else {
> +		if (adata->asic_type == CHIP_CARRIZO) {
> +			for (bank = 1; bank <= 4; bank++)
> +				acp_set_sram_bank_state(adata->acp_mmio, bank,
> +				false);

I'm not seeing any poweroff cases for other chips being added, and again
switch statements please.

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

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]