Re: [PATCH 6/8] ASoC: AMD: add AMD ASoC ACP 2.x DMA driver

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 6/8] ASoC: AMD: add AMD ASoC ACP 2.x DMA driver
Date: Tue, 5 Jan 2016 18:43:43 +0000	[thread overview]
Message-ID: <20160105184343.GL6588@sirena.org.uk> (raw)
In-Reply-To: <1450897275-17152-6-git-send-email-alexander.deucher@amd.com>

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

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

> --- /dev/null
> +++ b/sound/soc/amd/Kconfig
> @@ -0,0 +1,4 @@
> +config SND_SOC_AMD_ACP
> +	tristate "AMD Audio Coprocessor support"
> +	help
> +	 This option enables ACP DMA support on AMD platform.

This has no dependencies?

> +
> +	/* Designware I2S driver requries proper capabilities
> +	 * from mmACP_I2SMICSP_COMP_PARAM_1 register. The register
> +	 * reports playback and capture capabilities though the
> +	 * MIC instance of DW I2S controller supports capture only
> +	 * Provide a workaround by masking the capability into a
> +	 * scratch register and provide scratch register offset as
> +	 * though it is mmACP_I2SMICSP_COMP_PARAM_1
> +	 */
> +
> +	val = acp_reg_read(acp_mmio, mmACP_I2SMICSP_COMP_PARAM_1);
> +	val = val & ~BIT(5);
> +	acp_reg_write(val, acp_mmio, mmACP_SCRATCH_REG_0);

Ugh, right.  So the hardware doesn't actually have the register moved at
all.  Why are we doing this, if the capabilities really are buggy the
more idiomatic thing would be to provide an override for the
capabilities via platform data?  Requiring some other driver to poke the
hardware to set the capabilities is a very roundabout way to deal with
things.

We should probably revert that quirk unless I'm missing something here...

> +++ b/sound/soc/amd/acp.h
> @@ -0,0 +1,119 @@
> +#ifndef __ACP_HW_H
> +#define __ACP_HW_H
> +
> +#include "include/acp_2_2_d.h"
> +#include "include/acp_2_2_sh_mask.h"

I can't find these headers anywhere in the kernel tree or earlier in
this patch series, this will break the build.  The fact that they've got
include in the filename is also a bit interesting...

> +#define PAGE_SIZE_4K				4096

SZ_4K exists for this.

> +#define PAGE_SIZE_4K_ENABLE			0x02
> +
> +#define PLAYBACK_PTE_OFFSET			10
> +#define CAPTURE_PTE_OFFSET			0

These defines could all use namespacing, as could some of the others
later than don't mention ACP.

[-- 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