Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 11 Jan 2023 13:04:46 +0000
From:   Mark Brown <broonie@kernel.org>
To:     Vijendar Mukunda <Vijendar.Mukunda@amd.com>
Cc:     vkoul@kernel.org, alsa-devel@alsa-project.org,
        Basavaraj.Hiregoudar@amd.com, Sunil-kumar.Dommati@amd.com,
        Mario.Limonciello@amd.com, Mastan.Katragadda@amd.com,
        arungopal.kondaveeti@amd.com, Liam Girdwood <lgirdwood@gmail.com>,
        Jaroslav Kysela <perex@perex.cz>,
        Takashi Iwai <tiwai@suse.com>,
        Syed Saba Kareem <Syed.SabaKareem@amd.com>,
        open list <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 08/19] ASoC: amd: ps: add soundwire dma driver dma ops
Message-ID: <Y76z7uYb2/2BUSfl@sirena.org.uk>
References: <20230111090222.2016499-1-Vijendar.Mukunda@amd.com>
 <20230111090222.2016499-9-Vijendar.Mukunda@amd.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
        protocol="application/pgp-signature"; boundary="ag3m81g3IJWqkKTr"
Content-Disposition: inline
In-Reply-To: <20230111090222.2016499-9-Vijendar.Mukunda@amd.com>
Precedence: bulk


--ag3m81g3IJWqkKTr
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Wed, Jan 11, 2023 at 02:32:11PM +0530, Vijendar Mukunda wrote:

> +static int acp63_sdw_dma_start(struct snd_pcm_substream *stream)
> +{
> +	struct sdw_stream_instance *sdw_ins;
> +	struct snd_soc_pcm_runtime *prtd;
> +	u32 stream_id;
> +	u32 sdw_dma_reg;
> +	u32 sdw_dma_en_stat_reg;
> +	u32 sdw_dma_stat;
> +	u32 val;
> +	int timeout = 0;
> +
> +	sdw_ins = stream->runtime->private_data;
> +	prtd = stream->private_data;
> +	stream_id = sdw_ins->stream_id;
> +	switch (stream_id) {
> +	case ACP_SDW_AUDIO_TX:
> +		sdw_dma_reg = ACP_SW_AUDIO_TX_EN;
> +		sdw_dma_en_stat_reg = ACP_SW_AUDIO_TX_EN_STATUS;
> +		break;

Not super urgent but if you're respinning then it looks like the
register selection here is the same in _dma_stop() so they could be
shared.  Indeed it generally feels like it might be nicer to have a
table of structs listing the registers needed per stream so all these
switch statements can be more like

	sdw_dma_reg = stream_registers[sdw_ins->stream_id];

That'd make each function smaller and I'd expect it'd be a bit easier to
add new streams for new hardware that way.

--ag3m81g3IJWqkKTr
Content-Type: application/pgp-signature; name="signature.asc"

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

iQEzBAABCgAdFiEEreZoqmdXGLWf4p/qJNaLcl1Uh9AFAmO+s+4ACgkQJNaLcl1U
h9DLbAf/Y4NTcfbaVytJnLOHnw0v/nbCxi1SzVDhzKx3adrIi2mK0rjmw1rH8Ic/
OX1xBbmsovdG69oGTI3m7stFElnvH8/xOhKPjyzN1KcDIe9fxCUTmvblRGdBfkE3
jWUSbT6JI+d7b8LxYy7aEclwebLOmbhvDZiGYTasbF9POtj1msqx9J1w++XVpRj0
PS8pKMs/VIGrO+dsyYxmQwR/jkadmkecR1vpcnhy7cYGDJC0UBlh//LEwiM4E6Ta
sIRPq6ExQl3Vt21Xq7be5DFHueF3hgnqochCD5Kx/UmudA1b5iPzvIApPoTmNPUm
j4jObBg8qnHUvv03RkbfqEBsuGGAjg==
=PM/9
-----END PGP SIGNATURE-----

--ag3m81g3IJWqkKTr--