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

[Alex Deucher]

On Thu, Aug 20, 2015 at 7:18 PM, Mark Brown <broonie at kernel.org> wrote:
> 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.

Sorry, I just saw this reply now.  I just resent the patch with an
improved change log.  You an ignore that one.

Maruthi, can you comment on the points below and respin the patches as
necessary?

Thanks,

Alex

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