On Thu, May 3, 2018 at 10:35 PM Agrawal, Akshu Akshu.Agrawal@amd.com wrote:
On 5/3/2018 10:10 PM, Daniel Kurtz wrote:
On Thu, May 3, 2018 at 1:33 AM Mukunda,Vijendar <
vijendar.mukunda@amd.com>
wrote:
On Thursday 03 May 2018 11:13 AM, Daniel Kurtz wrote:
Some checkpatch nits below...
On Tue, May 1, 2018 at 2:53 PM Vijendar Mukunda <
Vijendar.Mukunda@amd.com>
wrote:
With in ACP, There are three I2S controllers can be configured/enabled ( I2S SP, I2S MICSP, I2S BT). Default enabled I2S controller instance is I2S SP. This patch provides required changes to support I2S BT controller Instance.
Signed-off-by: Vijendar Mukunda Vijendar.Mukunda@amd.com
v1->v2: defined i2s instance macros in acp header file v2->v3: sqaushed previous patch series and spilt changes into multiple patches (acp dma driver code cleanup patches and bt i2s instance specific changes) sound/soc/amd/acp-da7219-max98357a.c | 23 ++++ sound/soc/amd/acp-pcm-dma.c | 256
+++++++++++++++++++++++++++--------
sound/soc/amd/acp.h | 40 ++++++ 3 files changed, 262 insertions(+), 57 deletions(-)diff --git a/sound/soc/amd/acp-da7219-max98357a.c
b/sound/soc/amd/acp-da7219-max98357a.c
index 133139d..b3184ab 100644 --- a/sound/soc/amd/acp-da7219-max98357a.c +++ b/sound/soc/amd/acp-da7219-max98357a.c @@ -36,6 +36,7 @@ #include <linux/input.h> #include <linux/acpi.h>
+#include "acp.h" #include "../codecs/da7219.h" #include "../codecs/da7219-aad.h"
@@ -44,6 +45,7 @@
static struct snd_soc_jack cz_jack; static struct clk *da7219_dai_clk;+extern int bt_pad_enable;
WARNING: externs should be avoided in .c files
We don't have .h file for machine driver and It can be ignored for one variable.
static int cz_da7219_init(struct snd_soc_pcm_runtime *rtd) {@@ -132,6 +134,9 @@ static const struct snd_pcm_hw_constraint_list
constraints_channels = {
static int cz_da7219_startup(struct snd_pcm_substream
*substream)
{ struct snd_pcm_runtime *runtime = substream->runtime;
struct snd_soc_pcm_runtime *rtd = substream->private_data;struct snd_soc_card *card = rtd->card;struct acp_platform_info *machine =snd_soc_card_get_drvdata(card);
/* * On this platform for PCM device we support stereo@@ -143,6 +148,7 @@ static int cz_da7219_startup(struct
snd_pcm_substream
*substream)
snd_pcm_hw_constraint_list(runtime, 0,SNDRV_PCM_HW_PARAM_RATE,
&constraints_rates);
machine->i2s_instance = I2S_BT_INSTANCE;I'm not a big fan of this approach, but I don't know any other way to
tell
a single "platform" driver (acp-pcm-dma) which of two channels (ST/BT)
to
use via the pcm_open() callback.
Mark, can you recommend any other way of doing this?
Hi Dan,
There have been couple of approaches worked upon this earlier.
- To compare cpu dai name to get the I2S instance value in
acp_dma_open() call.
But, Mark suggested not to implement this approach as we are comparing dynamically generated cpu dai names.
- We added i2s_instance parameter as platform data to dwc driver.
By querying dwc driver platform data in acp dma driver, current i2s instance was programmed in acp_dma_open ().
But Mark's latest comment was to implement platform specific changes in machine driver. Machine driver and Dma driver should exchange the data regarding this. We accepted this and current approach is based on the same comment. Below is the reference. https://lkml.org/lkml/2018/4/18/597
Yes, I saw Mark's previous comment, but what we are trying to implement here is the SoC specific binding between i2s channel and acp-dma
channel.
This is a feature of the SoC, not of the i2s controller, but also not a feature of the audio configuration on the board.
The binding of channel and dma is soc specific but codec to channel is board specific. These linkages can change from one board to another. Machine driver is specific to a board (grunt here) and dma driver being common for various boards (but specific to a platform like ST/CZ here). Tomorrow there can be some other board with x codec linked to BT and y to SP. Hence, machine driver should send this board specific link information to dma driver for it to dma on the correct channel as per the board. For these SoCs, the link
between DMA registers & I2S-channel is hard-coded. The machine driver
is
already specifying which i2s channel to use when it configures, for example, '.cpu_dai_name = "designware-i2s.2.auto"'. The i2s channel selection already implies a particular DMA configuration. It seems redundant to create this separate out-of-band infrastructure to make the machine driver also tell its platform driver '.platform_name = "acp_audio_dma.0.auto"' which i2s channel it is using.
We could have decided on the basis of "cpu_dai_name" but the decision would have been based on dynamically generated name. Though maybe redundant in nature, but machine driver is just sending info to dma driver that this codec is playing so play on this instance.
Perhaps the acp-pcm-dma.c should register two different platform
drivers,
and let the machine driver pick the appropriate one (ie, acp_audio_dma.0.auto or acp_audio_dma.1.auto) depending on which i2s
port
it needs? Then each of these would have its own snd_soc_platform_driver->ops->open() that could setup snd_pcm_runtime->private_data appropriately?
I guess it would be overkill to have 2 platform drivers one for each instance but actually for a single platform.
Or is there another standardway to have a single snd_soc_platform_driver handle multiple channels?
Don't know if there is a standard but would be interesting to know how other platforms handle multiple instances.
Hmm, there may be another way that we don't see yet, but in any case, I see no harm in merging this patch set as is. So, for the series, feel free to add:
Reviewed-by: Daniel Kurtz djkurtz@chromium.org
Thanks, Akshu
-Dan