[alsa-devel] [PATCH V5 3/3] ASoC: AMD: add AMD ASoC ACP-I2S driver

[Mark Brown]

On Thu, Aug 20, 2015 at 05:36:34PM -0400, Alex Deucher wrote:
> From: Maruthi Srinivas Bayyavarapu <Maruthi.Bayyavarapu at 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.

This is flagged as patch 3/3 but appears to be sent as a single patch.
Please don't do this, the purpose of numbering patches is to help people
tell which order they are in.  Numbering only makes sense when you send
more than one patch, if you are sending a single patch there is no need
to number.

> +++ b/sound/soc/amd/Kconfig
> @@ -0,0 +1,5 @@
> +config SND_SOC_AMD_ACP
> +	tristate "AMD Audio Coprocessor support"
> +	depends on MFD_CORE

What is the dependency on the MFD core?

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

Still not happy with the licensing.

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

We already have a driver for the DesignWare I2S controller.  To repeat
the concern I raised in a slightly different bit of the code last time:

| This doesn't appear to be a designware-i2s driver, we already have one
| of those?  

Like I say please stop ignoring review comments.  The hw_params()
certainly seems to bear more than a passing resemblance.

> +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 = dev_get_drvdata(dev);
> +
> +	/* Inform ALSA about the period elapsed (one out of two periods) */
> +	if (play_intr)
> +		substream = irq_data->play_stream;
> +	else
> +		substream = irq_data->capture_stream;

So you've replaced the two if statements with an if/else which means
subsstream is now guaranteed to be initialized but perhaps not in the
way the caller intended...  I did find the caller (which got moved into
another patch in this version) and it appears that the two u16s are
actually used to pass boolean flags.  The whole interface here now
seems odd, what is going on here?

> +	if (NULL != pg) {

We still normally write these checks more like (pg != NULL) or even just
(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->pg = pg;
> +		rtd->size = size;
> +		rtd->num_of_pages = num_of_pages;
> +		rtd->direction = substream->stream;
> +
> +		acp_dev->config_dma(acp_dev, rtd);
> +		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);
> +}

hw_free() doesn't seem to undo everything that we did on allocation?

> +		/* 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 struct snd_soc_dai_driver i2s_dai_driver = {
> +	.playback = {
> +		     .stream_name = "I2S Playback",
> +		     .channels_min = 2,
> +		     .channels_max = 2,

Elsewhere support for 8 channels was declared and handled.

> +		     .rates = SNDRV_PCM_RATE_8000_96000,
> +		     .formats = SNDRV_PCM_FMTBIT_S24_LE |
> +				SNDRV_PCM_FMTBIT_S32_LE,

Elsewhere there was 16 bit support declared and handled.

> +	pm_rts = pm_runtime_status_suspended(dev);
> +	if (pm_rts == true) {

There seem to be a lot of variables in here that have only one
assignment and one user immediately next to them.  I'm not sure it's
adding much.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: Digital signature
URL: <http://mailman.alsa-project.org/pipermail/alsa-devel/attachments/20150820/03ad110f/attachment.sig>