On Thu, Aug 20, 2015 at 7:18 PM, Mark Brown <broonie@xxxxxxxxxx> wrote:
> On Thu, Aug 20, 2015 at 05:36:34PM -0400, Alex Deucher wrote:
>> From: Maruthi Srinivas Bayyavarapu <Maruthi.Bayyavarapu@xxxxxxx>
>>
>> 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.
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
