Re: [PATCH V3 10/10] ASoC: amd: dma driver changes for bt i2s instance

[Daniel Kurtz <djkurtz@chromium.org>]

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.
> 1) 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.

> 2) 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.  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.

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?  Or is there another standard
way to have a single snd_soc_platform_driver handle multiple channels?

-Dan