On Fri, Aug 21, 2015 at 4:48 AM, Mark Brown broonie@kernel.org wrote:
On Thu, Aug 20, 2015 at 05:36:34PM -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.
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.
Ok, sorry. I will be more careful next time. Actually, two patches are sent - one for DRM and other ASoC. It should have been represented as 2/2.
+++ b/sound/soc/amd/Kconfig @@ -0,0 +1,5 @@ +config SND_SOC_AMD_ACP
tristate "AMD Audio Coprocessor support"depends on MFD_COREWhat is the dependency on the MFD core?
None. Sorry, I forgot to remove this. Actually, AMD DRM driver depends on MFD_CORE to create a platform device for ASoC. This is already present in DRM patch.
+/*
- 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.
I will contact our internal teams and get back on this.
+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?
Our IP block includes few AMD IPs along with DesignWare I2S IP. I have reused code from Designware I2S controller from sound/soc/dwc/designware_i2s.c and because of the way IPs are coupled together, I couldn't use existing Designware I2S driver as is. I have given credit to the original author in DRM patch copyright header, where register I2S read/writes are made. Do I need to add the same header in ASoC driver too ?
Like I say please stop ignoring review comments. The hw_params() certainly seems to bear more than a passing resemblance.
Iam sorry, this mistake won't be repeated.
+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;elsesubstream = 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?
Agreed. I will modify this interface to use a bool, in next revision
if (NULL != pg) {We still normally write these checks more like (pg != NULL) or even just (pg).
Ok, will modify accordingly in next revision.
/* 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?
Oh! I freed in dma_close(), that were allocated in dma_open(). Rest of them used devm_*
/* 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.
|------------|----------| | A | B | DRAM |------------|----------| \ / \ / / DMA /\ |---------/-|---------| | C / | \ D | SRAM ---DMA ---------> I2S |-----------|----------|
buffer in System memory(DRAM) is divided into 2 periods - say A,B. internal memory inside ACP IP (SRAM) is also divided in similar way - say C,D. In hw_params() A and B are filled with zeros. In prepare() , content of A and B are DMA'ed to D and C respectively (as per DMA configuration). When ALSA core fills A and B with audio content (start threshold = A+B size), trigger() is called and content of A is transferred to D. C still holds zeros by now. The internal buffer (SRAM) which holds C+D will be DMA'ed to I2S in cyclic way starting from C.
At the end of C or D's DMA to I2S, irq is generated. In irq handler DMA is done from B to C or A to D depending on C or D' DMA completion respectively and snd_pcm_period_elapsed() is called.
The reason for doing this is : When C completes rendering, calls period_elapsed() and informs ALSA core there is free space to fill new data to system memory. In the same irq handler, B is DMA'ed to C to be ready by the time D completes rendering with prefetched data (in trigger()). when D completes rendering, new data fetched by ALSA core can be DMA'ed from A to D and rendering is continued with C. This is done cyclically.
If I don’t do A->D, B->C and instead do A->C, B->D, there would be timing problem to have new data ready to be rendered. The alternate approach would be implement 'copy' callback of 'snd_pcm_ops' in pcm driver. In that callback, I can use copy_from_user() to get new data and then only issue DMA transfers (A->C / B->D). This can solve the timing issue mentioned, but then I can't handle MMAP based playback/capture. Existing design can handle non-mmap and mmap based transfers, but the only issue(?) would be 1 period size of zeros will be played initially for any new playback usecase.
+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.
The board for which driver is developed, doesn't support more than 2 channels.
.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.
There is a bug in our I2S IP, which wrongly handles 16 bit and lower resolutions. So, I removed the support. All the players using the driver, will handle unsupported formats with the help of pulseaudio conversions.
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.
Ok, I will reuse variables, where required and remove extra ones in the next version.
Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel