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

[Alex Deucher]

On Tue, Jan 5, 2016 at 1:43 PM, Mark Brown <broonie at kernel.org> wrote:
> 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?

No dependencies.  The ACP device discovery is triggered from GPU
driver, but the ACP driver has no direct dependencies on it.
Theoretically it could act as a standalone driver.

Alex

>
>> +
>> +     /* 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.