Re: [PATCH 7/8] ASoC: AMD: add pm ops

From: Mark Brown <broonie@kernel.org>
To: Alex Deucher <alexdeucher@gmail.com>
Cc: alsa-devel@alsa-project.org, maruthi.bayyavarapu@amd.com,
	lgirdwood@gmail.com, dri-devel@lists.freedesktop.org,
	rajeevkumar.linux@gmail.com,
	Alex Deucher <alexander.deucher@amd.com>,
	perex@perex.cz
Subject: Re: [PATCH 7/8] ASoC: AMD: add pm ops
Date: Tue, 5 Jan 2016 18:59:29 +0000	[thread overview]
Message-ID: <20160105185929.GN6588@sirena.org.uk> (raw)
In-Reply-To: <1450897275-17152-7-git-send-email-alexander.deucher@amd.com>

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

On Wed, Dec 23, 2015 at 02:01:14PM -0500, Alex Deucher wrote:

> +	stream = adata->play_stream;
> +	rtd = stream ? stream->runtime : NULL;
> +	if (rtd != NULL) {
> +		/* Resume playback stream from a suspended state */
> +		sdata = rtd->private_data;
> +		config_acp_dma(adata->acp_mmio, sdata);
> +	}

Please don't use the ternery operator like this, it's not bad to write
if statements and they're much more legible.  I was having to think far
too much about the various variables and if we were safely handling all
of them before I realised the next block overwrites them all anyway.
This is really

	stream = adata->play_stream;
	if (stream && stream->rtd)
		config_acp_dma(adata->acp_mmio, stream->rtd->private_data);

or a couple of nested if statements.  Otherwise this patch looks OK.

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

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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel