[10/12] ASoC: AMD: add AMD ASoC ACP-I2S driver (v2)
diff mbox

Message ID 1438871112-25946-10-git-send-email-alexander.deucher@amd.com
State New
Headers show

Commit Message

Alex Deucher Aug. 6, 2015, 2:25 p.m. UTC
From: Maruthi Srinivas Bayyavarapu <Maruthi.Bayyavarapu@amd.com>

ACP IP block consists of dedicated DMA and I2S blocks. The PCM driver
provides the DMA and CPU DAI components to ALSA core. Machine driver
provides the audio functionality together with the PCM driver and rt286
codec driver.

v2: squash in Kconfig fix

Signed-off-by: Maruthi Bayyavarapu <maruthi.bayyavarapu@amd.com>
Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
Reviewed-by: Murali Krishna Vemuri <murali-krishna.vemuri@amd.com>
---
 sound/soc/Kconfig           |   2 +-
 sound/soc/Makefile          |   1 +
 sound/soc/amd/Kconfig       |  13 +
 sound/soc/amd/Makefile      |  11 +
 sound/soc/amd/acp-pcm-dma.c | 661 ++++++++++++++++++++++++++++++++++++++++++++
 sound/soc/amd/acp-rt286.c   | 265 ++++++++++++++++++
 6 files changed, 952 insertions(+), 1 deletion(-)
 create mode 100644 sound/soc/amd/Kconfig
 create mode 100644 sound/soc/amd/Makefile
 create mode 100644 sound/soc/amd/acp-pcm-dma.c
 create mode 100644 sound/soc/amd/acp-rt286.c

Comments

Mark Brown Aug. 6, 2015, 7:22 p.m. UTC | #1
On Thu, Aug 06, 2015 at 10:25:10AM -0400, Alex Deucher wrote:
> From: Maruthi Srinivas Bayyavarapu <Maruthi.Bayyavarapu@amd.com>
> 
> ACP IP block consists of dedicated DMA and I2S blocks. The PCM driver
> provides the DMA and CPU DAI components to ALSA core. Machine driver
> provides the audio functionality together with the PCM driver and rt286
> codec driver.

Quite a few comments below, a lot of them fairly simple stylistic and
process ones but there seem to be some really big issues in the way the
machine driver is implemented and in particular with things being
manually registered rather than instantiated with ACPI.  I'm also
worried about the unusual licensing.

Please also coordinate with Intel on ACPI bindings for ASoC style audio
subsystems, I've CCed in Vinod who's been working on it (and Liam is
also at Intel).

> 
> v2: squash in Kconfig fix

Please follow the patch submission process in SubmittingPatches: put any
versioning in the subject line inside the [] and put noise like inter
version changelogs after the ---.

> --- a/sound/soc/Makefile
> +++ b/sound/soc/Makefile
> @@ -41,3 +41,4 @@ obj-$(CONFIG_SND_SOC)	+= txx9/
>  obj-$(CONFIG_SND_SOC)	+= ux500/
>  obj-$(CONFIG_SND_SOC)	+= xtensa/
>  obj-$(CONFIG_SND_SOC)	+= zte/
> +obj-$(CONFIG_SND_SOC)   += amd/

Please keep the Makefile sorted as well as the Kconfig.

> diff --git a/sound/soc/amd/Kconfig b/sound/soc/amd/Kconfig
> new file mode 100644
> index 0000000..07677de
> --- /dev/null
> +++ b/sound/soc/amd/Kconfig
> @@ -0,0 +1,13 @@
> + config SND_SOC_AMD_CZ_RT286_MACH
> +        tristate "AMD ASoC Audio driver for Carrizo with rt286 codec"
> +	select SND_SOC_RT286
> +	select SND_SOC_AMD_ACP
> +        depends on I2C_DESIGNWARE_PLATFORM
> +        help
> +           This option enables AMD I2S Audio support on Carrizo
> +	   with ALC288 codec.

It looks like you've got tab/space here.  You're also adding a machine
driver in the same patch as the base driver support, please don't do
that - send one patch per driver.

> new file mode 100644
> index 0000000..63b6f83
> --- /dev/null
> +++ b/sound/soc/amd/Makefile
> @@ -0,0 +1,11 @@
> +ccflags-y := -Iinclude/drm -Idrivers/gpu/drm/amdsoc/
> +ccflags-y += -Idrivers/gpu/drm/amdsoc/include/
> +ccflags-y += -Idrivers/gpu/drm/amd/include/bus/
> +ccflags-y += -Idrivers/gpu/drm/amd/acp/include
> +ccflags-y += -Idrivers/gpu/drm/amd/include/
> +ccflags-y += -Idrivers/gpu/drm/amd/include/asic_reg/acp

Eew, no - please put these headers in include.

> + * AMD ALSA SoC PCM Driver
> + *
> + * Copyright 2014-2015 Advanced Micro Devices, Inc.
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
> + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
> + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> + * OTHER DEALINGS IN THE SOFTWARE.

All the ASoC APIs are EXPORT_SYMBOL_GPL() and this doesn't explicitly
grant GPL rights.  Your MODULE_LICENSE() says

> +MODULE_LICENSE("GPL and additional rights");

but I'm not 100% sure that's the case - it looks like a separate MIT/BSD
license rather than a GPL grant.  This doesn't make me happy about the
licensing status.  The clearest thing would be either to just license
under the GPL, or to dual license.

> +	.fifo_size = 0,

No need to set static structures to 0.

> +static const struct snd_soc_component_driver dw_i2s_component = {
> +	.name = "dw-i2s.0",
> +};

.0?  What's going on here...

> +static void acp_pcm_period_elapsed(struct device *dev, u16 play_intr,
> +							u16 capture_intr)
> +{
> +	struct snd_pcm_substream *substream;
> +	struct audio_drv_data *irq_data =
> +	    (struct audio_drv_data *)dev_get_drvdata(dev);

No need to cast away from void.

> +
> +	/* Inform ALSA about the period elapsed (one out of two periods) */
> +	if (play_intr)
> +		substream = irq_data->play_stream;
> +	else if (capture_intr)
> +		substream = irq_data->capture_stream;
> +
> +	if (substream->runtime && snd_pcm_running(substream))

What if both play_intr and capture_intr or set, or if neither of them is
set?

> +	adata->dma_config =
> +			kzalloc(sizeof(struct acp_dma_config), GFP_KERNEL);
> +	if (adata->dma_config == NULL) {
> +		kfree(adata);
> +		return -ENOMEM;
> +	}

Why are all these structs allocated separately and not just embedded
into adata?

> +	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
> +		runtime->hw = acp_pcm_hardware_playback;
> +	else if (substream->stream == SNDRV_PCM_STREAM_CAPTURE)
> +		runtime->hw = acp_pcm_hardware_capture;
> +	else {
> +		pr_err("Error in stream type\n");
> +		return -EINVAL;
> +	}

You should have { } on all branches of an if statement if you need it on
one.  The usual idiom for this check is just to do "if (playback)" and
not have the third error case.

> +	ret = snd_pcm_hw_constraint_integer(runtime,
> +					    SNDRV_PCM_HW_PARAM_PERIODS);
> +	if (ret < 0) {
> +		pr_err("snd_pcm_hw_constraint_integer failed\n");

Please use dev_ prints so people can tell more easily what the source of
the message is.

> +	if (WARN_ON(!substream))
> +		return -EINVAL;

The subsystem will check this for you.

> +	memset(substream->runtime->dma_area, 0, params_buffer_bytes(params));
> +	pg = virt_to_page(substream->dma_buffer.area);
> +
> +	if (NULL != pg) {
> +		/* Save for runtime private data */
> +		rtd->pg = pg;
> +		rtd->order = get_order(size);
> +
> +		/*Let ACP know the Allocated memory */
> +		num_of_pages = PAGE_ALIGN(size) >> PAGE_SHIFT;

Spaces in the comments.

> +		/* Fill ACP SRAM with zeros from System RAM which is zero-ed
> +		 * in hw_params */
> +		ret = acp_dev->dma_start(rtd->acp_dev,
> +						SYSRAM_TO_ACP_CH_NUM, false);
> +		if (ret < 0)
> +			ret = -EFAULT;
> +
> +		/* Now configure DMA to transfer only first half of System RAM
> +		 * buffer before playback is triggered. This will overwrite
> +		 * zero-ed second half of SRAM buffer */
> +		acp_dev->config_dma_channel(acp_dev, SYSRAM_TO_ACP_CH_NUM,
> +					PLAYBACK_START_DMA_DESCR_CH12,
> +					1, 0);

Why?  The comments describe what's happening but it's not clear why it's
happening.

> +static int acp_dma_trigger(struct snd_pcm_substream *substream, int cmd)
> +{
> +	struct snd_pcm_runtime *runtime = substream->runtime;
> +	struct audio_substream_data *rtd = runtime->private_data;
> +	struct amd_acp_device *acp_dev = rtd->acp_dev;
> +
> +	int ret = -EIO;

This means the compiler won't be able to spot missing initialisation
problems - do we need to do it?  Also I notice you've got a *lot* of
blocks of variable declarations separated by spaces which is really
unusual for the kernel.

> +	if (!rtd)
> +		return -EINVAL;
> +	switch (cmd) {
> +	case SNDRV_PCM_TRIGGER_START:
> +	case SNDRV_PCM_TRIGGER_RESUME:
> +	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
> +		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
> +			ret = acp_dev->dma_start(rtd->acp_dev,
> +						SYSRAM_TO_ACP_CH_NUM, false);
> +			if (ret < 0)
> +				ret = -EFAULT;

Don't ignore the error code you got, pass it back.

> +	default:
> +		dev_err(dev, "designware-i2s: unsuppted PCM fmt");
> +		return -EINVAL;

This doesn't appear to be a designware-i2s driver, we already have one
of those?  It's also better to print out the value that we're erroring
on, it can help people figure out problems.

> +static int acp_alsa_register(struct device *dev, struct amd_acp_device *acp_dev,
> +				struct amd_gnb_bus_dev *adev)
> +{
> +	int status;
> +
> +	status = snd_soc_register_platform(dev, &acp_asoc_platform);
> +	if (STATUS_SUCCESS != status) {

STATUS_SUCCESS!?  It's also very unusual to have the variable and the
constant this way round in the kernel - I am noticing a lot of style
issues in here.

> +static void __exit amdsoc_bus_acp_dma_driver_exit(void)
> +{
> +	pr_info("ACP: PCM driver exit\n");

Don't include noise like this in the kernel logs, it's not adding
anything.

> +	amd_gnb_bus_unregister_driver(&acp_dma_driver);
> +}
> +
> +module_init(amdsoc_bus_acp_dma_driver_init);
> +module_exit(amdsoc_bus_acp_dma_driver_exit);
> +
> +MODULE_AUTHOR("Maruthi.Bayyavarapu@amd.com");
> +MODULE_DESCRIPTION("AMD ACP PCM Driver");
> +MODULE_LICENSE("GPL and additional rights");

How will this module get autoloaded?

> +#include "../codecs/rt286.h"
> +
> +#ifdef CONFIG_PINCTRL_AMD
> +
> +#define CZ_HPJACK_GPIO  7

Hard coded system wide magic numbers?  Please don't do this.

> +	err = snd_soc_dai_set_fmt(codec_dai, SND_SOC_DAIFMT_I2S |
> +				SND_SOC_DAIFMT_NB_NF |
> +				SND_SOC_DAIFMT_CBM_CFM);
> +	if (err < 0) {
> +		dev_err(card->dev, "unable to set codec dai format\n");
> +		return err;
> +	}

Set this in the dai_link.

> +	err = snd_soc_dai_set_sysclk(codec_dai, RT286_SCLK_S_PLL, 24000000,
> +					SND_SOC_CLOCK_OUT);
> +	if (err < 0) {
> +		dev_err(card->dev, "unable to set codec dai clock\n");
> +		return err;
> +	}

Set this in the link init function, no need to reset it each time we
configure since it never changes.

> +static int carrizo_init(struct snd_soc_pcm_runtime *rtd)
> +{
> +	/* TODO: check whether dapm widgets needs to be
> +	 * dsiconnected initially. */

They don't.

> +static struct snd_soc_dai_link carrizo_dai_rt286 = {
> +	.name = "amd-rt286",
> +	.stream_name = "RT286_AIF1",
> +	.platform_name = "acp_pcm_dev",
> +	.cpu_dai_name = "acp_pcm_dev",
> +	.codec_dai_name = "rt286-aif1",
> +	.codec_name = "rt286.3-001c",
> +	.dai_fmt = SND_SOC_DAIFMT_I2S | SND_SOC_DAIFMT_NB_NF
> +			| SND_SOC_DAIFMT_CBM_CFM,

Oh, you did initialise this in the dai_link :(

> +	.ignore_suspend = 1,

Why is this being set - this doesn't look like it's a CODEC<->CODEC
link?

> +	ret = snd_soc_register_card(card);
> +	if (ret) {

devm_snd_soc_register_card()

> +static const struct acpi_device_id cz_audio_acpi_match[] = {
> +	{ "I2SC1002", 0 },
> +	{},
> +};

ACPI has bindings for GPIOs, you should be able to use those to discover 
the detection GPIO.

> +static int __init cz_audio_init(void)
> +{
> +	int ret;
> +	struct i2c_adapter *adapter;
> +	struct i2c_board_info cz_board_info;
> +	const char *codec_acpi_name = "rt288";
> +
> +	adapter = i2c_get_adapter(CZ_CODEC_I2C_ADAPTER_ID);
> +	if (!adapter)
> +		return -ENODEV;
> +
> +	memset(&cz_board_info, 0, sizeof(struct i2c_board_info));
> +	cz_board_info.addr = CZ_CODEC_I2C_ADDR;
> +	strlcpy(cz_board_info.type, codec_acpi_name, I2C_NAME_SIZE);
> +
> +#ifdef CONFIG_PINCTRL_AMD
> +	if (gpio_is_valid(CZ_HPJACK_GPIO)) {
> +		ret = gpio_request_one(CZ_HPJACK_GPIO, GPIOF_DIR_IN |
> +						GPIOF_EXPORT, "hp-gpio");
> +		if (ret != 0)
> +			pr_err("gpio_request_one failed : err %d\n", ret);

As well as the whole thing with getting this from ACPI rather than
defining magic numbers this should be done in the card init not in the
module init (like other card drivers do).  This then means you don't
need any global variables.

> +#endif
> +	i2c_client = i2c_new_device(adapter, &cz_board_info);
> +	i2c_put_adapter(adapter);
> +	if (!i2c_client)
> +		return -ENODEV;

No, definitely not - ACPI has perfectly good bindings for instantiating
I2C devices (indeed the laptop I'm typing this on actually uses them to
instantiate exactly the same RT286 audio CODEC you're using here),
please use them.
Maruthi Srinivas Bayyavarapu Aug. 7, 2015, 9:27 a.m. UTC | #2
Hi Mark,

Thanks, I will work on the review feedback.

Regards,
Maruthi

-----Original Message-----
From: Mark Brown [mailto:broonie@kernel.org] 
Sent: 07 August 2015 00:52
To: Alex Deucher
Cc: airlied@gmail.com; dri-devel@lists.freedesktop.org; alsa-devel@alsa-project.org; tiwai@suse.de; perex@perex.cz; lgirdwood@gmail.com; Bayyavarapu, Maruthi; Vinod Koul
Subject: Re: [PATCH 10/12] ASoC: AMD: add AMD ASoC ACP-I2S driver (v2)

On Thu, Aug 06, 2015 at 10:25:10AM -0400, Alex Deucher wrote:
> From: Maruthi Srinivas Bayyavarapu <Maruthi.Bayyavarapu@amd.com>
> 
> ACP IP block consists of dedicated DMA and I2S blocks. The PCM driver 
> provides the DMA and CPU DAI components to ALSA core. Machine driver 
> provides the audio functionality together with the PCM driver and 
> rt286 codec driver.

Quite a few comments below, a lot of them fairly simple stylistic and process ones but there seem to be some really big issues in the way the machine driver is implemented and in particular with things being manually registered rather than instantiated with ACPI.  I'm also worried about the unusual licensing.

Please also coordinate with Intel on ACPI bindings for ASoC style audio subsystems, I've CCed in Vinod who's been working on it (and Liam is also at Intel).

> 
> v2: squash in Kconfig fix

Please follow the patch submission process in SubmittingPatches: put any versioning in the subject line inside the [] and put noise like inter version changelogs after the ---.

> --- a/sound/soc/Makefile
> +++ b/sound/soc/Makefile
> @@ -41,3 +41,4 @@ obj-$(CONFIG_SND_SOC)	+= txx9/
>  obj-$(CONFIG_SND_SOC)	+= ux500/
>  obj-$(CONFIG_SND_SOC)	+= xtensa/
>  obj-$(CONFIG_SND_SOC)	+= zte/
> +obj-$(CONFIG_SND_SOC)   += amd/

Please keep the Makefile sorted as well as the Kconfig.

> diff --git a/sound/soc/amd/Kconfig b/sound/soc/amd/Kconfig new file 
> mode 100644 index 0000000..07677de
> --- /dev/null
> +++ b/sound/soc/amd/Kconfig
> @@ -0,0 +1,13 @@
> + config SND_SOC_AMD_CZ_RT286_MACH
> +        tristate "AMD ASoC Audio driver for Carrizo with rt286 codec"
> +	select SND_SOC_RT286
> +	select SND_SOC_AMD_ACP
> +        depends on I2C_DESIGNWARE_PLATFORM
> +        help
> +           This option enables AMD I2S Audio support on Carrizo
> +	   with ALC288 codec.

It looks like you've got tab/space here.  You're also adding a machine driver in the same patch as the base driver support, please don't do that - send one patch per driver.

> new file mode 100644
> index 0000000..63b6f83
> --- /dev/null
> +++ b/sound/soc/amd/Makefile
> @@ -0,0 +1,11 @@
> +ccflags-y := -Iinclude/drm -Idrivers/gpu/drm/amdsoc/ ccflags-y += 
> +-Idrivers/gpu/drm/amdsoc/include/
> +ccflags-y += -Idrivers/gpu/drm/amd/include/bus/
> +ccflags-y += -Idrivers/gpu/drm/amd/acp/include ccflags-y += 
> +-Idrivers/gpu/drm/amd/include/ ccflags-y += 
> +-Idrivers/gpu/drm/amd/include/asic_reg/acp

Eew, no - please put these headers in include.

> + * AMD ALSA SoC PCM Driver
> + *
> + * Copyright 2014-2015 Advanced Micro Devices, Inc.
> + *
> + * Permission is hereby granted, free of charge, to any person 
> + obtaining a
> + * copy of this software and associated documentation files (the 
> + "Software"),
> + * to deal in the Software without restriction, including without 
> + limitation
> + * the rights to use, copy, modify, merge, publish, distribute, 
> + sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom 
> + the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be 
> + included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, 
> + EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF 
> + MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT 
> + SHALL
> + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, 
> + DAMAGES OR
> + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR 
> + OTHERWISE,
> + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE 
> + OR
> + * OTHER DEALINGS IN THE SOFTWARE.

All the ASoC APIs are EXPORT_SYMBOL_GPL() and this doesn't explicitly grant GPL rights.  Your MODULE_LICENSE() says

> +MODULE_LICENSE("GPL and additional rights");

but I'm not 100% sure that's the case - it looks like a separate MIT/BSD license rather than a GPL grant.  This doesn't make me happy about the licensing status.  The clearest thing would be either to just license under the GPL, or to dual license.

> +	.fifo_size = 0,

No need to set static structures to 0.

> +static const struct snd_soc_component_driver dw_i2s_component = {
> +	.name = "dw-i2s.0",
> +};

.0?  What's going on here...

> +static void acp_pcm_period_elapsed(struct device *dev, u16 play_intr,
> +							u16 capture_intr)
> +{
> +	struct snd_pcm_substream *substream;
> +	struct audio_drv_data *irq_data =
> +	    (struct audio_drv_data *)dev_get_drvdata(dev);

No need to cast away from void.

> +
> +	/* Inform ALSA about the period elapsed (one out of two periods) */
> +	if (play_intr)
> +		substream = irq_data->play_stream;
> +	else if (capture_intr)
> +		substream = irq_data->capture_stream;
> +
> +	if (substream->runtime && snd_pcm_running(substream))

What if both play_intr and capture_intr or set, or if neither of them is set?

> +	adata->dma_config =
> +			kzalloc(sizeof(struct acp_dma_config), GFP_KERNEL);
> +	if (adata->dma_config == NULL) {
> +		kfree(adata);
> +		return -ENOMEM;
> +	}

Why are all these structs allocated separately and not just embedded into adata?

> +	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
> +		runtime->hw = acp_pcm_hardware_playback;
> +	else if (substream->stream == SNDRV_PCM_STREAM_CAPTURE)
> +		runtime->hw = acp_pcm_hardware_capture;
> +	else {
> +		pr_err("Error in stream type\n");
> +		return -EINVAL;
> +	}

You should have { } on all branches of an if statement if you need it on one.  The usual idiom for this check is just to do "if (playback)" and not have the third error case.

> +	ret = snd_pcm_hw_constraint_integer(runtime,
> +					    SNDRV_PCM_HW_PARAM_PERIODS);
> +	if (ret < 0) {
> +		pr_err("snd_pcm_hw_constraint_integer failed\n");

Please use dev_ prints so people can tell more easily what the source of the message is.

> +	if (WARN_ON(!substream))
> +		return -EINVAL;

The subsystem will check this for you.

> +	memset(substream->runtime->dma_area, 0, params_buffer_bytes(params));
> +	pg = virt_to_page(substream->dma_buffer.area);
> +
> +	if (NULL != pg) {
> +		/* Save for runtime private data */
> +		rtd->pg = pg;
> +		rtd->order = get_order(size);
> +
> +		/*Let ACP know the Allocated memory */
> +		num_of_pages = PAGE_ALIGN(size) >> PAGE_SHIFT;

Spaces in the comments.

> +		/* Fill ACP SRAM with zeros from System RAM which is zero-ed
> +		 * in hw_params */
> +		ret = acp_dev->dma_start(rtd->acp_dev,
> +						SYSRAM_TO_ACP_CH_NUM, false);
> +		if (ret < 0)
> +			ret = -EFAULT;
> +
> +		/* Now configure DMA to transfer only first half of System RAM
> +		 * buffer before playback is triggered. This will overwrite
> +		 * zero-ed second half of SRAM buffer */
> +		acp_dev->config_dma_channel(acp_dev, SYSRAM_TO_ACP_CH_NUM,
> +					PLAYBACK_START_DMA_DESCR_CH12,
> +					1, 0);

Why?  The comments describe what's happening but it's not clear why it's happening.

> +static int acp_dma_trigger(struct snd_pcm_substream *substream, int 
> +cmd) {
> +	struct snd_pcm_runtime *runtime = substream->runtime;
> +	struct audio_substream_data *rtd = runtime->private_data;
> +	struct amd_acp_device *acp_dev = rtd->acp_dev;
> +
> +	int ret = -EIO;

This means the compiler won't be able to spot missing initialisation problems - do we need to do it?  Also I notice you've got a *lot* of blocks of variable declarations separated by spaces which is really unusual for the kernel.

> +	if (!rtd)
> +		return -EINVAL;
> +	switch (cmd) {
> +	case SNDRV_PCM_TRIGGER_START:
> +	case SNDRV_PCM_TRIGGER_RESUME:
> +	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
> +		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
> +			ret = acp_dev->dma_start(rtd->acp_dev,
> +						SYSRAM_TO_ACP_CH_NUM, false);
> +			if (ret < 0)
> +				ret = -EFAULT;

Don't ignore the error code you got, pass it back.

> +	default:
> +		dev_err(dev, "designware-i2s: unsuppted PCM fmt");
> +		return -EINVAL;

This doesn't appear to be a designware-i2s driver, we already have one of those?  It's also better to print out the value that we're erroring on, it can help people figure out problems.

> +static int acp_alsa_register(struct device *dev, struct amd_acp_device *acp_dev,
> +				struct amd_gnb_bus_dev *adev)
> +{
> +	int status;
> +
> +	status = snd_soc_register_platform(dev, &acp_asoc_platform);
> +	if (STATUS_SUCCESS != status) {

STATUS_SUCCESS!?  It's also very unusual to have the variable and the constant this way round in the kernel - I am noticing a lot of style issues in here.

> +static void __exit amdsoc_bus_acp_dma_driver_exit(void)
> +{
> +	pr_info("ACP: PCM driver exit\n");

Don't include noise like this in the kernel logs, it's not adding anything.

> +	amd_gnb_bus_unregister_driver(&acp_dma_driver);
> +}
> +
> +module_init(amdsoc_bus_acp_dma_driver_init);
> +module_exit(amdsoc_bus_acp_dma_driver_exit);
> +
> +MODULE_AUTHOR("Maruthi.Bayyavarapu@amd.com");
> +MODULE_DESCRIPTION("AMD ACP PCM Driver"); MODULE_LICENSE("GPL and 
> +additional rights");

How will this module get autoloaded?

> +#include "../codecs/rt286.h"
> +
> +#ifdef CONFIG_PINCTRL_AMD
> +
> +#define CZ_HPJACK_GPIO  7

Hard coded system wide magic numbers?  Please don't do this.

> +	err = snd_soc_dai_set_fmt(codec_dai, SND_SOC_DAIFMT_I2S |
> +				SND_SOC_DAIFMT_NB_NF |
> +				SND_SOC_DAIFMT_CBM_CFM);
> +	if (err < 0) {
> +		dev_err(card->dev, "unable to set codec dai format\n");
> +		return err;
> +	}

Set this in the dai_link.

> +	err = snd_soc_dai_set_sysclk(codec_dai, RT286_SCLK_S_PLL, 24000000,
> +					SND_SOC_CLOCK_OUT);
> +	if (err < 0) {
> +		dev_err(card->dev, "unable to set codec dai clock\n");
> +		return err;
> +	}

Set this in the link init function, no need to reset it each time we configure since it never changes.

> +static int carrizo_init(struct snd_soc_pcm_runtime *rtd) {
> +	/* TODO: check whether dapm widgets needs to be
> +	 * dsiconnected initially. */

They don't.

> +static struct snd_soc_dai_link carrizo_dai_rt286 = {
> +	.name = "amd-rt286",
> +	.stream_name = "RT286_AIF1",
> +	.platform_name = "acp_pcm_dev",
> +	.cpu_dai_name = "acp_pcm_dev",
> +	.codec_dai_name = "rt286-aif1",
> +	.codec_name = "rt286.3-001c",
> +	.dai_fmt = SND_SOC_DAIFMT_I2S | SND_SOC_DAIFMT_NB_NF
> +			| SND_SOC_DAIFMT_CBM_CFM,

Oh, you did initialise this in the dai_link :(

> +	.ignore_suspend = 1,

Why is this being set - this doesn't look like it's a CODEC<->CODEC link?

> +	ret = snd_soc_register_card(card);
> +	if (ret) {

devm_snd_soc_register_card()

> +static const struct acpi_device_id cz_audio_acpi_match[] = {
> +	{ "I2SC1002", 0 },
> +	{},
> +};

ACPI has bindings for GPIOs, you should be able to use those to discover the detection GPIO.

> +static int __init cz_audio_init(void) {
> +	int ret;
> +	struct i2c_adapter *adapter;
> +	struct i2c_board_info cz_board_info;
> +	const char *codec_acpi_name = "rt288";
> +
> +	adapter = i2c_get_adapter(CZ_CODEC_I2C_ADAPTER_ID);
> +	if (!adapter)
> +		return -ENODEV;
> +
> +	memset(&cz_board_info, 0, sizeof(struct i2c_board_info));
> +	cz_board_info.addr = CZ_CODEC_I2C_ADDR;
> +	strlcpy(cz_board_info.type, codec_acpi_name, I2C_NAME_SIZE);
> +
> +#ifdef CONFIG_PINCTRL_AMD
> +	if (gpio_is_valid(CZ_HPJACK_GPIO)) {
> +		ret = gpio_request_one(CZ_HPJACK_GPIO, GPIOF_DIR_IN |
> +						GPIOF_EXPORT, "hp-gpio");
> +		if (ret != 0)
> +			pr_err("gpio_request_one failed : err %d\n", ret);

As well as the whole thing with getting this from ACPI rather than defining magic numbers this should be done in the card init not in the module init (like other card drivers do).  This then means you don't need any global variables.

> +#endif
> +	i2c_client = i2c_new_device(adapter, &cz_board_info);
> +	i2c_put_adapter(adapter);
> +	if (!i2c_client)
> +		return -ENODEV;

No, definitely not - ACPI has perfectly good bindings for instantiating I2C devices (indeed the laptop I'm typing this on actually uses them to instantiate exactly the same RT286 audio CODEC you're using here), please use them.

Patch
diff mbox

diff --git a/sound/soc/Kconfig b/sound/soc/Kconfig
index 2ae9619..5965657 100644
--- a/sound/soc/Kconfig
+++ b/sound/soc/Kconfig
@@ -32,6 +32,7 @@  config SND_SOC_GENERIC_DMAENGINE_PCM
 
 # All the supported SoCs
 source "sound/soc/adi/Kconfig"
+source "sound/soc/amd/Kconfig"
 source "sound/soc/atmel/Kconfig"
 source "sound/soc/au1x/Kconfig"
 source "sound/soc/bcm/Kconfig"
@@ -67,4 +68,3 @@  source "sound/soc/codecs/Kconfig"
 source "sound/soc/generic/Kconfig"
 
 endif	# SND_SOC
-
diff --git a/sound/soc/Makefile b/sound/soc/Makefile
index e189903..e2b017a 100644
--- a/sound/soc/Makefile
+++ b/sound/soc/Makefile
@@ -41,3 +41,4 @@  obj-$(CONFIG_SND_SOC)	+= txx9/
 obj-$(CONFIG_SND_SOC)	+= ux500/
 obj-$(CONFIG_SND_SOC)	+= xtensa/
 obj-$(CONFIG_SND_SOC)	+= zte/
+obj-$(CONFIG_SND_SOC)   += amd/
diff --git a/sound/soc/amd/Kconfig b/sound/soc/amd/Kconfig
new file mode 100644
index 0000000..07677de
--- /dev/null
+++ b/sound/soc/amd/Kconfig
@@ -0,0 +1,13 @@ 
+ config SND_SOC_AMD_CZ_RT286_MACH
+        tristate "AMD ASoC Audio driver for Carrizo with rt286 codec"
+	select SND_SOC_RT286
+	select SND_SOC_AMD_ACP
+        depends on I2C_DESIGNWARE_PLATFORM
+        help
+           This option enables AMD I2S Audio support on Carrizo
+	   with ALC288 codec.
+ config SND_SOC_AMD_ACP
+        tristate "AMD Audio Coprocessor support"
+        depends on DRM_AMD_GNB_BUS
+        help
+          This option enables ACP support (DMA,I2S) on AMD platforms.
diff --git a/sound/soc/amd/Makefile b/sound/soc/amd/Makefile
new file mode 100644
index 0000000..63b6f83
--- /dev/null
+++ b/sound/soc/amd/Makefile
@@ -0,0 +1,11 @@ 
+ccflags-y := -Iinclude/drm -Idrivers/gpu/drm/amdsoc/
+ccflags-y += -Idrivers/gpu/drm/amdsoc/include/
+ccflags-y += -Idrivers/gpu/drm/amd/include/bus/
+ccflags-y += -Idrivers/gpu/drm/amd/acp/include
+ccflags-y += -Idrivers/gpu/drm/amd/include/
+ccflags-y += -Idrivers/gpu/drm/amd/include/asic_reg/acp
+snd-soc-acp-pcm-objs	:= acp-pcm-dma.o
+snd-soc-acp-rt286-mach-objs := acp-rt286.o
+
+obj-$(CONFIG_SND_SOC_AMD_ACP) += snd-soc-acp-pcm.o
+obj-$(CONFIG_SND_SOC_AMD_CZ_RT286_MACH) += snd-soc-acp-rt286-mach.o
diff --git a/sound/soc/amd/acp-pcm-dma.c b/sound/soc/amd/acp-pcm-dma.c
new file mode 100644
index 0000000..0f58957
--- /dev/null
+++ b/sound/soc/amd/acp-pcm-dma.c
@@ -0,0 +1,661 @@ 
+/*
+ * AMD ALSA SoC PCM Driver
+ *
+ * Copyright 2014-2015 Advanced Micro Devices, Inc.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
+ * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
+ * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
+ * OTHER DEALINGS IN THE SOFTWARE.
+ *
+ *
+ */
+
+#include <linux/interrupt.h>
+#include <linux/platform_device.h>
+#include <linux/dma-mapping.h>
+#include <linux/slab.h>
+#include <linux/module.h>
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/pci.h>
+
+#include <sound/pcm.h>
+#include <sound/pcm_params.h>
+#include <sound/soc.h>
+
+#include "amd_acp.h"
+#include "amd_gnb_bus.h"
+
+#define PLAYBACK_MIN_NUM_PERIODS    2
+#define PLAYBACK_MAX_NUM_PERIODS    2
+#define PLAYBACK_MAX_PERIOD_SIZE    16384
+#define PLAYBACK_MIN_PERIOD_SIZE    1024
+#define CAPTURE_MIN_NUM_PERIODS     2
+#define CAPTURE_MAX_NUM_PERIODS     2
+#define CAPTURE_MAX_PERIOD_SIZE     16384
+#define CAPTURE_MIN_PERIOD_SIZE     1024
+
+#define NUM_DSCRS_PER_CHANNEL 2
+
+#define MAX_BUFFER (PLAYBACK_MAX_PERIOD_SIZE * PLAYBACK_MAX_NUM_PERIODS)
+#define MIN_BUFFER MAX_BUFFER
+
+#define TWO_CHANNEL_SUPPORT     2	/* up to 2.0 */
+#define FOUR_CHANNEL_SUPPORT    4	/* up to 3.1 */
+#define SIX_CHANNEL_SUPPORT     6	/* up to 5.1 */
+#define EIGHT_CHANNEL_SUPPORT   8	/* up to 7.1 */
+
+
+static const struct snd_pcm_hardware acp_pcm_hardware_playback = {
+	.info = SNDRV_PCM_INFO_INTERLEAVED |
+		SNDRV_PCM_INFO_BLOCK_TRANSFER | SNDRV_PCM_INFO_MMAP |
+		SNDRV_PCM_INFO_MMAP_VALID | SNDRV_PCM_INFO_BATCH |
+		SNDRV_PCM_INFO_PAUSE | SNDRV_PCM_INFO_RESUME,
+	/* formats,rates,channels  based on i2s doc. */
+	.formats = SNDRV_PCM_FMTBIT_S16_LE |
+		SNDRV_PCM_FMTBIT_S24_LE | SNDRV_PCM_FMTBIT_S32_LE,
+	.channels_min = 1,
+	.channels_max = 8,
+	.rates = SNDRV_PCM_RATE_8000_96000,
+	.rate_min = 8000,
+	.rate_max = 96000,
+	.buffer_bytes_max = PLAYBACK_MAX_NUM_PERIODS * PLAYBACK_MAX_PERIOD_SIZE,
+	.period_bytes_min = PLAYBACK_MIN_PERIOD_SIZE,
+	.period_bytes_max = PLAYBACK_MAX_PERIOD_SIZE,
+	.periods_min = PLAYBACK_MIN_NUM_PERIODS,
+	.periods_max = PLAYBACK_MAX_NUM_PERIODS,
+	.fifo_size = 0,
+};
+
+static const struct snd_pcm_hardware acp_pcm_hardware_capture = {
+	.info = SNDRV_PCM_INFO_INTERLEAVED |
+		SNDRV_PCM_INFO_BLOCK_TRANSFER | SNDRV_PCM_INFO_MMAP |
+		SNDRV_PCM_INFO_MMAP_VALID | SNDRV_PCM_INFO_BATCH |
+	    SNDRV_PCM_INFO_PAUSE | SNDRV_PCM_INFO_RESUME,
+	/* formats,rates,channels  based on i2s doc. */
+	.formats = SNDRV_PCM_FMTBIT_S16_LE |
+		SNDRV_PCM_FMTBIT_S24_LE | SNDRV_PCM_FMTBIT_S32_LE,
+	.channels_min = 1,
+	.channels_max = 2,
+	.rates = SNDRV_PCM_RATE_8000_48000,
+	.rate_min = 8000,
+	.rate_max = 48000,
+	.buffer_bytes_max = CAPTURE_MAX_NUM_PERIODS * CAPTURE_MAX_PERIOD_SIZE,
+	.period_bytes_min = CAPTURE_MIN_PERIOD_SIZE,
+	.period_bytes_max = CAPTURE_MAX_PERIOD_SIZE,
+	.periods_min = CAPTURE_MIN_NUM_PERIODS,
+	.periods_max = CAPTURE_MAX_NUM_PERIODS,
+	.fifo_size = 0,
+};
+
+struct audio_drv_data {
+	struct snd_pcm_substream *play_stream;
+	struct snd_pcm_substream *capture_stream;
+	struct amd_acp_device *acp_dev;
+	struct acp_irq_prv *iprv;
+};
+
+struct audio_substream_data {
+	struct amd_acp_device *acp_dev;
+	struct page *pg;
+	struct acp_dma_config *dma_config;
+	struct acp_i2s_config  *i2s_config;
+	unsigned int order;
+};
+
+static const struct snd_soc_component_driver dw_i2s_component = {
+	.name = "dw-i2s.0",
+};
+
+static void acp_pcm_period_elapsed(struct device *dev, u16 play_intr,
+							u16 capture_intr)
+{
+	struct snd_pcm_substream *substream;
+	struct audio_drv_data *irq_data =
+	    (struct audio_drv_data *)dev_get_drvdata(dev);
+
+	/* Inform ALSA about the period elapsed (one out of two periods) */
+	if (play_intr)
+		substream = irq_data->play_stream;
+	else if (capture_intr)
+		substream = irq_data->capture_stream;
+
+	if (substream->runtime && snd_pcm_running(substream))
+		snd_pcm_period_elapsed(substream);
+}
+
+static int acp_dma_open(struct snd_pcm_substream *substream)
+{
+	int ret = 0;
+	struct snd_pcm_runtime *runtime = substream->runtime;
+	struct snd_soc_pcm_runtime *prtd = substream->private_data;
+	struct audio_drv_data *intr_data =
+	    (struct audio_drv_data *)dev_get_drvdata(prtd->platform->dev);
+
+	struct audio_substream_data *adata =
+		kzalloc(sizeof(struct audio_substream_data), GFP_KERNEL);
+	if (adata == NULL)
+		return -ENOMEM;
+
+	adata->dma_config =
+			kzalloc(sizeof(struct acp_dma_config), GFP_KERNEL);
+	if (adata->dma_config == NULL) {
+		kfree(adata);
+		return -ENOMEM;
+	}
+
+	adata->i2s_config =
+			kzalloc(sizeof(struct acp_i2s_config), GFP_KERNEL);
+	if (adata->i2s_config == NULL) {
+		kfree(adata->dma_config);
+		kfree(adata);
+		return -ENOMEM;
+	}
+
+	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
+		runtime->hw = acp_pcm_hardware_playback;
+	else if (substream->stream == SNDRV_PCM_STREAM_CAPTURE)
+		runtime->hw = acp_pcm_hardware_capture;
+	else {
+		pr_err("Error in stream type\n");
+		return -EINVAL;
+	}
+
+	ret = snd_pcm_hw_constraint_integer(runtime,
+					    SNDRV_PCM_HW_PARAM_PERIODS);
+	if (ret < 0) {
+		pr_err("snd_pcm_hw_constraint_integer failed\n");
+		return ret;
+	}
+
+	adata->acp_dev = intr_data->acp_dev;
+	runtime->private_data = adata;
+
+	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
+		intr_data->play_stream = substream;
+	else
+		intr_data->capture_stream = substream;
+
+	return 0;
+}
+
+static int acp_dma_hw_params(struct snd_pcm_substream *substream,
+			     struct snd_pcm_hw_params *params)
+{
+	int status;
+	uint64_t size;
+	struct snd_dma_buffer *dma_buffer;
+	struct page *pg;
+	u16 num_of_pages;
+
+	struct snd_pcm_runtime *runtime;
+	struct audio_substream_data *rtd;
+	struct amd_acp_device *acp_dev;
+
+	if (WARN_ON(!substream))
+		return -EINVAL;
+
+	dma_buffer = &substream->dma_buffer;
+
+	runtime = substream->runtime;
+	rtd = runtime->private_data;
+
+	if (WARN_ON(!rtd))
+		return -EINVAL;
+	acp_dev = rtd->acp_dev;
+
+	size = params_buffer_bytes(params);
+	status = snd_pcm_lib_malloc_pages(substream, size);
+	if (status < 0)
+		return status;
+
+	memset(substream->runtime->dma_area, 0, params_buffer_bytes(params));
+	pg = virt_to_page(substream->dma_buffer.area);
+
+	if (NULL != pg) {
+		/* Save for runtime private data */
+		rtd->pg = pg;
+		rtd->order = get_order(size);
+
+		/*Let ACP know the Allocated memory */
+		num_of_pages = PAGE_ALIGN(size) >> PAGE_SHIFT;
+
+		/* Fill the page table entries in ACP SRAM */
+
+		rtd->dma_config->pg = pg;
+		rtd->dma_config->size = size;
+		rtd->dma_config->num_of_pages = num_of_pages;
+		rtd->dma_config->direction = substream->stream;
+
+		acp_dev->config_dma(acp_dev, rtd->dma_config);
+
+		status = 0;
+	} else {
+		status = -ENOMEM;
+	}
+	return status;
+}
+
+static int acp_dma_hw_free(struct snd_pcm_substream *substream)
+{
+	return snd_pcm_lib_free_pages(substream);
+}
+
+static snd_pcm_uframes_t acp_dma_pointer(struct snd_pcm_substream *substream)
+{
+	u32 pos = 0;
+	struct snd_pcm_runtime *runtime = substream->runtime;
+	struct audio_substream_data *rtd = runtime->private_data;
+	struct amd_acp_device *acp_dev = rtd->acp_dev;
+
+	pos = acp_dev->update_dma_pointer(acp_dev, substream->stream,
+				frames_to_bytes(runtime, runtime->period_size));
+	return bytes_to_frames(runtime, pos);
+
+}
+
+static int acp_dma_mmap(struct snd_pcm_substream *substream,
+			struct vm_area_struct *vma)
+{
+	return snd_pcm_lib_default_mmap(substream, vma);
+}
+
+static int acp_dma_prepare(struct snd_pcm_substream *substream)
+{
+	int ret;
+	struct snd_pcm_runtime *runtime = substream->runtime;
+	struct audio_substream_data *rtd = runtime->private_data;
+	struct amd_acp_device *acp_dev = rtd->acp_dev;
+
+	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
+
+		acp_dev->config_dma_channel(acp_dev, SYSRAM_TO_ACP_CH_NUM,
+					PLAYBACK_START_DMA_DESCR_CH12,
+					NUM_DSCRS_PER_CHANNEL, 0);
+		acp_dev->config_dma_channel(acp_dev, ACP_TO_I2S_DMA_CH_NUM,
+					PLAYBACK_START_DMA_DESCR_CH13,
+					NUM_DSCRS_PER_CHANNEL, 0);
+		/* Fill ACP SRAM with zeros from System RAM which is zero-ed
+		 * in hw_params */
+		ret = acp_dev->dma_start(rtd->acp_dev,
+						SYSRAM_TO_ACP_CH_NUM, false);
+		if (ret < 0)
+			ret = -EFAULT;
+
+		/* Now configure DMA to transfer only first half of System RAM
+		 * buffer before playback is triggered. This will overwrite
+		 * zero-ed second half of SRAM buffer */
+		acp_dev->config_dma_channel(acp_dev, SYSRAM_TO_ACP_CH_NUM,
+					PLAYBACK_START_DMA_DESCR_CH12,
+					1, 0);
+	} else {
+		acp_dev->config_dma_channel(acp_dev, ACP_TO_SYSRAM_CH_NUM,
+					CAPTURE_START_DMA_DESCR_CH14,
+					NUM_DSCRS_PER_CHANNEL, 0);
+		acp_dev->config_dma_channel(acp_dev, I2S_TO_ACP_DMA_CH_NUM,
+					CAPTURE_START_DMA_DESCR_CH15,
+					NUM_DSCRS_PER_CHANNEL, 0);
+	}
+	return 0;
+}
+
+static int acp_dma_trigger(struct snd_pcm_substream *substream, int cmd)
+{
+	struct snd_pcm_runtime *runtime = substream->runtime;
+	struct audio_substream_data *rtd = runtime->private_data;
+	struct amd_acp_device *acp_dev = rtd->acp_dev;
+
+	int ret = -EIO;
+
+	if (!rtd)
+		return -EINVAL;
+	switch (cmd) {
+	case SNDRV_PCM_TRIGGER_START:
+	case SNDRV_PCM_TRIGGER_RESUME:
+	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
+		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
+			ret = acp_dev->dma_start(rtd->acp_dev,
+						SYSRAM_TO_ACP_CH_NUM, false);
+			if (ret < 0)
+				ret = -EFAULT;
+			else
+				acp_dev->prebuffer_audio(rtd->acp_dev);
+
+			ret = acp_dev->dma_start(acp_dev,
+					    ACP_TO_I2S_DMA_CH_NUM, true);
+		} else if (substream->stream == SNDRV_PCM_STREAM_CAPTURE)
+			ret = acp_dev->dma_start(acp_dev,
+					    I2S_TO_ACP_DMA_CH_NUM, true);
+		break;
+	case SNDRV_PCM_TRIGGER_STOP:
+	case SNDRV_PCM_TRIGGER_SUSPEND:
+	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
+		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
+			ret = acp_dev->dma_stop(acp_dev, SYSRAM_TO_ACP_CH_NUM);
+			if (0 == ret)
+				ret = acp_dev->dma_stop(acp_dev,
+						   ACP_TO_I2S_DMA_CH_NUM);
+		} else if (substream->stream == SNDRV_PCM_STREAM_CAPTURE) {
+			ret = acp_dev->dma_stop(acp_dev, I2S_TO_ACP_DMA_CH_NUM);
+			if (0 == ret)
+				ret = acp_dev->dma_stop(acp_dev,
+						ACP_TO_SYSRAM_CH_NUM);
+		}
+		break;
+	default:
+		ret = -EINVAL;
+
+	}
+	return ret;
+}
+
+static int acp_dma_new(struct snd_soc_pcm_runtime *rtd)
+{
+	int ret;
+	struct snd_pcm *pcm;
+
+	pcm = rtd->pcm;
+	ret = snd_pcm_lib_preallocate_pages_for_all(pcm, SNDRV_DMA_TYPE_DEV,
+					NULL, MIN_BUFFER, MAX_BUFFER);
+	return ret;
+}
+
+static int acp_dma_close(struct snd_pcm_substream *substream)
+{
+	struct snd_pcm_runtime *runtime = substream->runtime;
+	struct audio_substream_data *rtd = runtime->private_data;
+
+	kfree(rtd->dma_config);
+	kfree(rtd->i2s_config);
+	kfree(rtd);
+
+	return 0;
+}
+
+static int acp_dai_i2s_hwparams(struct snd_pcm_substream *substream,
+				struct snd_pcm_hw_params *params,
+				struct snd_soc_dai *dai)
+{
+	struct snd_pcm_runtime *runtime = substream->runtime;
+	struct snd_soc_pcm_runtime *prtd = substream->private_data;
+
+	struct audio_substream_data *rtd = runtime->private_data;
+	struct amd_acp_device *acp_dev = rtd->acp_dev;
+	struct device *dev = prtd->platform->dev;
+
+	u32 chan_nr;
+
+	switch (params_format(params)) {
+	case SNDRV_PCM_FORMAT_S16_LE:
+		rtd->i2s_config->xfer_resolution = 0x02;
+		break;
+
+	case SNDRV_PCM_FORMAT_S24_LE:
+		rtd->i2s_config->xfer_resolution = 0x04;
+		break;
+
+	case SNDRV_PCM_FORMAT_S32_LE:
+		rtd->i2s_config->xfer_resolution = 0x05;
+		break;
+
+	default:
+		dev_err(dev, "designware-i2s: unsuppted PCM fmt");
+		return -EINVAL;
+	}
+
+	chan_nr = params_channels(params);
+
+	switch (chan_nr) {
+	case EIGHT_CHANNEL_SUPPORT:
+		rtd->i2s_config->ch_reg = 3;
+		break;
+	case SIX_CHANNEL_SUPPORT:
+		rtd->i2s_config->ch_reg = 2;
+		break;
+	case FOUR_CHANNEL_SUPPORT:
+		rtd->i2s_config->ch_reg = 1;
+		break;
+	case TWO_CHANNEL_SUPPORT:
+		rtd->i2s_config->ch_reg = 0;
+		break;
+	default:
+		dev_err(dev, "channel not supported\n");
+		return -EINVAL;
+	}
+
+	rtd->i2s_config->direction = substream->stream;
+
+	acp_dev->config_i2s(acp_dev, rtd->i2s_config);
+
+	return 0;
+}
+
+static int acp_dai_i2s_trigger(struct snd_pcm_substream *substream,
+			       int cmd, struct snd_soc_dai *dai)
+{
+	struct snd_pcm_runtime *runtime = substream->runtime;
+	struct audio_substream_data *rtd = runtime->private_data;
+	struct amd_acp_device *acp_dev = rtd->acp_dev;
+
+	int ret = 0;
+
+	switch (cmd) {
+	case SNDRV_PCM_TRIGGER_START:
+	case SNDRV_PCM_TRIGGER_RESUME:
+	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
+		acp_dev->i2s_start(acp_dev, substream->stream);
+		break;
+
+	case SNDRV_PCM_TRIGGER_STOP:
+	case SNDRV_PCM_TRIGGER_SUSPEND:
+	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
+		acp_dev->i2s_stop(acp_dev, substream->stream);
+		break;
+	default:
+		ret = -EINVAL;
+		break;
+	}
+
+	return ret;
+}
+
+static int acp_dai_i2s_prepare(struct snd_pcm_substream *substream,
+			       struct snd_soc_dai *dai)
+{
+	struct snd_pcm_runtime *runtime = substream->runtime;
+	struct audio_substream_data *rtd = runtime->private_data;
+	struct amd_acp_device *acp_dev = rtd->acp_dev;
+
+	acp_dev->i2s_reset(acp_dev, substream->stream);
+	return 0;
+}
+
+static struct snd_pcm_ops acp_dma_ops = {
+	.open = acp_dma_open,
+	.close = acp_dma_close,
+	.ioctl = snd_pcm_lib_ioctl,
+	.hw_params = acp_dma_hw_params,
+	.hw_free = acp_dma_hw_free,
+	.trigger = acp_dma_trigger,
+	.pointer = acp_dma_pointer,
+	.mmap = acp_dma_mmap,
+	.prepare = acp_dma_prepare,
+};
+
+struct snd_soc_dai_ops acp_dai_i2s_ops = {
+	.prepare = acp_dai_i2s_prepare,
+	.hw_params = acp_dai_i2s_hwparams,
+	.trigger = acp_dai_i2s_trigger,
+};
+
+/* CZ i2s configuration */
+static struct snd_soc_dai_driver i2s_dai_driver_cz = {
+	.playback = {
+		     .stream_name = "I2S Playback",
+		     .channels_min = 2,
+		     .channels_max = 2,
+		     .rates = SNDRV_PCM_RATE_8000_96000,
+		     .formats = SNDRV_PCM_FMTBIT_S24_LE |
+				SNDRV_PCM_FMTBIT_S32_LE,
+
+		     .rate_min = 8000,
+		     .rate_max = 96000,
+		     },
+	.capture = {
+		    .stream_name = "I2S Capture",
+		    .channels_min = 2,
+		    .channels_max = 2,
+		    .rates = SNDRV_PCM_RATE_8000_48000,
+		    .formats = SNDRV_PCM_FMTBIT_S24_LE |
+				SNDRV_PCM_FMTBIT_S32_LE,
+		    .rate_min = 8000,
+		    .rate_max = 48000,
+		    },
+	.ops = &acp_dai_i2s_ops,
+};
+
+static struct snd_soc_platform_driver acp_asoc_platform = {
+	.ops = &acp_dma_ops,
+	.pcm_new = acp_dma_new,
+};
+
+static int acp_alsa_register(struct device *dev, struct amd_acp_device *acp_dev,
+				struct amd_gnb_bus_dev *adev)
+{
+	int status;
+
+	status = snd_soc_register_platform(dev, &acp_asoc_platform);
+	if (STATUS_SUCCESS != status) {
+		dev_err(dev, "Unable to register ALSA platform device\n");
+		goto exit_platform;
+	} else {
+		 /* SNDRV_PCM_FMTBIT_S16_LE is not supported in CZ */
+		status = snd_soc_register_component(dev,
+					&dw_i2s_component,
+					&i2s_dai_driver_cz, 1);
+
+		if (STATUS_SUCCESS != status) {
+			dev_err(dev, "Unable to register i2s dai\n");
+			goto exit_dai;
+		} else {
+			dev_info(dev, "ACP device registered with ALSA\n");
+			return status;
+		}
+	}
+
+exit_dai:
+	snd_soc_unregister_platform(dev);
+exit_platform:
+	acp_dev->fini(acp_dev);
+	return status;
+}
+
+static int acp_amdsoc_probe(struct amd_gnb_bus_dev *adev)
+{
+	int status;
+	struct audio_drv_data *audio_drv_data;
+	struct amd_acp_device *acp_dev = adev->private_data;
+
+	if (AMD_GNB_IP_ACP_PCM != adev->ip) {
+		dev_err(&adev->dev, "Not an ACP Device on AMD GNB bus\n");
+		return -ENODEV;
+	}
+
+	audio_drv_data = devm_kzalloc(&adev->dev,
+						sizeof(struct audio_drv_data),
+						GFP_KERNEL);
+	if (audio_drv_data == NULL)
+		return -ENOMEM;
+
+	audio_drv_data->iprv = devm_kzalloc(&adev->dev,
+						sizeof(struct acp_irq_prv),
+						GFP_KERNEL);
+	if (audio_drv_data->iprv == NULL)
+		return -ENOMEM;
+
+	/* The following members gets populated in device 'open'
+	 * function. Till then interrupts are disabled in 'acp_hw_init'
+	 * and device doesn't generate any interrupts.
+	 */
+
+	audio_drv_data->play_stream = NULL;
+	audio_drv_data->capture_stream = NULL;
+	audio_drv_data->acp_dev = acp_dev;
+
+	audio_drv_data->iprv->dev = &adev->dev;
+	audio_drv_data->iprv->acp_dev = acp_dev;
+	audio_drv_data->iprv->set_elapsed = acp_pcm_period_elapsed;
+
+	dev_set_drvdata(&adev->dev, audio_drv_data);
+
+	/* Initialize the ACP */
+	status = acp_dev->init(acp_dev, audio_drv_data->iprv);
+
+	if (STATUS_SUCCESS == status)
+		status = acp_alsa_register(&adev->dev, acp_dev, adev);
+	else
+		pr_err("ACP initialization Failed\n");
+
+	return status;
+}
+
+static int acp_amdsoc_remove(struct amd_gnb_bus_dev *adev)
+{
+	struct amd_acp_device *acp_dev = adev->private_data;
+
+	snd_soc_unregister_component(&adev->dev);
+	snd_soc_unregister_platform(&adev->dev);
+
+	acp_dev->fini(acp_dev);
+
+	return 0;
+}
+
+static struct amd_gnb_bus_driver acp_dma_driver = {
+	.name = "acp-pcm-driver",
+	.ip = AMD_GNB_IP_ACP_PCM,
+	.probe = acp_amdsoc_probe,
+	.remove = acp_amdsoc_remove,
+};
+
+static int __init amdsoc_bus_acp_dma_driver_init(void)
+{
+	int ret = 0;
+
+	ret = amd_gnb_bus_register_driver(&acp_dma_driver,
+					 THIS_MODULE, "acp-pcm-driver");
+	if (ret) {
+		pr_err("ACP: Unable to register with AMD GNB BUS!\n");
+		return ret;
+	}
+
+	return 0;
+
+}
+
+static void __exit amdsoc_bus_acp_dma_driver_exit(void)
+{
+	pr_info("ACP: PCM driver exit\n");
+	amd_gnb_bus_unregister_driver(&acp_dma_driver);
+}
+
+module_init(amdsoc_bus_acp_dma_driver_init);
+module_exit(amdsoc_bus_acp_dma_driver_exit);
+
+MODULE_AUTHOR("Maruthi.Bayyavarapu@amd.com");
+MODULE_DESCRIPTION("AMD ACP PCM Driver");
+MODULE_LICENSE("GPL and additional rights");
diff --git a/sound/soc/amd/acp-rt286.c b/sound/soc/amd/acp-rt286.c
new file mode 100644
index 0000000..7d4bdf9
--- /dev/null
+++ b/sound/soc/amd/acp-rt286.c
@@ -0,0 +1,265 @@ 
+/*
+ * Machine driver for AMD ACP Audio engine using Realtek RT286 codec
+ *
+ * Copyright 2014-2015 Advanced Micro Devices, Inc.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
+ * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
+ * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
+ * OTHER DEALINGS IN THE SOFTWARE.
+ *
+ *
+ */
+
+#include <sound/core.h>
+#include <sound/soc.h>
+#include <sound/pcm.h>
+#include <sound/pcm_params.h>
+#include <sound/soc-dapm.h>
+#include <sound/jack.h>
+#include <linux/gpio.h>
+#include <linux/module.h>
+#include <linux/i2c.h>
+#include <linux/acpi.h>
+
+#include "../codecs/rt286.h"
+
+#ifdef CONFIG_PINCTRL_AMD
+
+#define CZ_HPJACK_GPIO  7
+#define CZ_HPJACK_DEBOUNCE 150
+
+#endif
+
+#define CZ_CODEC_I2C_ADDR 0x1c
+#define CZ_CODEC_I2C_ADAPTER_ID 3
+
+struct i2c_client *i2c_client;
+
+struct acp_rt286 {
+	int gpio_hp_det;
+};
+
+static struct snd_soc_jack cz_jack;
+static struct snd_soc_jack_pin cz_pins[] = {
+	{
+		.pin = "Analog Mic",
+		.mask = SND_JACK_MICROPHONE,
+	},
+	{
+		.pin = "Headphones",
+		.mask = SND_JACK_HEADPHONE,
+	},
+};
+
+static int carrizo_hw_params(struct snd_pcm_substream *substream,
+				struct snd_pcm_hw_params *params)
+{
+	struct snd_soc_pcm_runtime *rtd = substream->private_data;
+	struct snd_soc_dai *codec_dai = rtd->codec_dai;
+	struct snd_soc_card *card = rtd->card;
+	int sample_rate;
+	int err;
+
+	err = snd_soc_dai_set_fmt(codec_dai, SND_SOC_DAIFMT_I2S |
+				SND_SOC_DAIFMT_NB_NF |
+				SND_SOC_DAIFMT_CBM_CFM);
+	if (err < 0) {
+		dev_err(card->dev, "unable to set codec dai format\n");
+		return err;
+	}
+
+	sample_rate = params_rate(params);
+
+	err = snd_soc_dai_set_sysclk(codec_dai, RT286_SCLK_S_PLL, 24000000,
+					SND_SOC_CLOCK_OUT);
+	if (err < 0) {
+		dev_err(card->dev, "unable to set codec dai clock\n");
+		return err;
+	}
+
+	return 0;
+
+}
+
+static struct snd_soc_ops carrizo_rt286_ops = {
+	.hw_params = carrizo_hw_params,
+};
+
+static int carrizo_init(struct snd_soc_pcm_runtime *rtd)
+{
+	/* TODO: check whether dapm widgets needs to be
+	 * dsiconnected initially. */
+	int ret;
+	struct snd_soc_card *card;
+	struct snd_soc_codec *codec;
+
+	codec = rtd->codec;
+	card = rtd->card;
+	ret = snd_soc_card_jack_new(card, "Headset",
+		SND_JACK_HEADSET, &cz_jack, cz_pins, ARRAY_SIZE(cz_pins));
+
+	if (ret)
+		return ret;
+
+	rt286_mic_detect(codec, &cz_jack);
+	return 0;
+}
+
+
+static struct snd_soc_dai_link carrizo_dai_rt286 = {
+	.name = "amd-rt286",
+	.stream_name = "RT286_AIF1",
+	.platform_name = "acp_pcm_dev",
+	.cpu_dai_name = "acp_pcm_dev",
+	.codec_dai_name = "rt286-aif1",
+	.codec_name = "rt286.3-001c",
+	.dai_fmt = SND_SOC_DAIFMT_I2S | SND_SOC_DAIFMT_NB_NF
+			| SND_SOC_DAIFMT_CBM_CFM,
+	.ignore_suspend = 1,
+	.ops = &carrizo_rt286_ops,
+	.init = carrizo_init,
+};
+
+static const struct snd_soc_dapm_widget cz_widgets[] = {
+	SND_SOC_DAPM_HP("Headphones", NULL),
+	SND_SOC_DAPM_MIC("Analog Mic", NULL),
+};
+
+static const struct snd_soc_dapm_route cz_audio_route[] = {
+	{"Headphones", NULL, "HPO L"},
+	{"Headphones", NULL, "HPO R"},
+	{"MIC1", NULL, "Analog Mic"},
+};
+
+static struct snd_soc_card carrizo_card = {
+	.name = "acp-rt286",
+	.owner = THIS_MODULE,
+	.dai_link = &carrizo_dai_rt286,
+	.num_links = 1,
+
+	.dapm_widgets = cz_widgets,
+	.num_dapm_widgets = ARRAY_SIZE(cz_widgets),
+	.dapm_routes = cz_audio_route,
+	.num_dapm_routes = ARRAY_SIZE(cz_audio_route),
+};
+
+static int carrizo_probe(struct platform_device *pdev)
+{
+	int ret;
+	struct acp_rt286 *machine;
+	struct snd_soc_card *card;
+
+	machine = devm_kzalloc(&pdev->dev, sizeof(struct acp_rt286),
+				GFP_KERNEL);
+	if (!machine)
+		return -ENOMEM;
+
+	card = &carrizo_card;
+	carrizo_card.dev = &pdev->dev;
+
+	platform_set_drvdata(pdev, card);
+	snd_soc_card_set_drvdata(card, machine);
+
+	ret = snd_soc_register_card(card);
+	if (ret) {
+		dev_err(&pdev->dev,
+				"snd_soc_register_card(%s) failed: %d\n",
+				carrizo_card.name, ret);
+		return ret;
+	}
+	return 0;
+}
+
+static int carrizo_remove(struct platform_device *pdev)
+{
+	struct snd_soc_card *card;
+
+	card = platform_get_drvdata(pdev);
+	snd_soc_unregister_card(card);
+
+	return 0;
+}
+
+static const struct acpi_device_id cz_audio_acpi_match[] = {
+	{ "I2SC1002", 0 },
+	{},
+};
+
+static struct platform_driver carrizo_pcm_driver = {
+	.driver = {
+		.name = "carrizo_i2s_audio",
+		.acpi_match_table = ACPI_PTR(cz_audio_acpi_match),
+		.owner = THIS_MODULE,
+		.pm = &snd_soc_pm_ops,
+	},
+	.probe = carrizo_probe,
+	.remove = carrizo_remove,
+};
+
+static int __init cz_audio_init(void)
+{
+	int ret;
+	struct i2c_adapter *adapter;
+	struct i2c_board_info cz_board_info;
+	const char *codec_acpi_name = "rt288";
+
+	adapter = i2c_get_adapter(CZ_CODEC_I2C_ADAPTER_ID);
+	if (!adapter)
+		return -ENODEV;
+
+	memset(&cz_board_info, 0, sizeof(struct i2c_board_info));
+	cz_board_info.addr = CZ_CODEC_I2C_ADDR;
+	strlcpy(cz_board_info.type, codec_acpi_name, I2C_NAME_SIZE);
+
+#ifdef CONFIG_PINCTRL_AMD
+	if (gpio_is_valid(CZ_HPJACK_GPIO)) {
+		ret = gpio_request_one(CZ_HPJACK_GPIO, GPIOF_DIR_IN |
+						GPIOF_EXPORT, "hp-gpio");
+		if (ret != 0)
+			pr_err("gpio_request_one failed : err %d\n", ret);
+
+		cz_board_info.irq = gpio_to_irq(CZ_HPJACK_GPIO);
+
+		gpio_set_debounce(CZ_HPJACK_GPIO, CZ_HPJACK_DEBOUNCE);
+	}
+#endif
+	i2c_client = i2c_new_device(adapter, &cz_board_info);
+	i2c_put_adapter(adapter);
+	if (!i2c_client)
+		return -ENODEV;
+
+	platform_driver_register(&carrizo_pcm_driver);
+	return 0;
+}
+
+static void __exit cz_audio_exit(void)
+{
+#ifdef CONFIG_PINCTRL_AMD
+	if (gpio_is_valid(CZ_HPJACK_GPIO))
+		gpio_free(CZ_HPJACK_GPIO);
+#endif
+	i2c_unregister_device(i2c_client);
+
+	platform_driver_unregister(&carrizo_pcm_driver);
+}
+
+module_init(cz_audio_init);
+module_exit(cz_audio_exit);
+
+MODULE_AUTHOR("Maruthi.Bayyavarapu@amd.com");
+MODULE_DESCRIPTION("CZ-rt288 Audio Support");
+MODULE_LICENSE("GPL and additional rights");