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

["Agrawal, Akshu" <Akshu.Agrawal@amd.com>]

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.
>> 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.
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 standard
> way 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.

Thanks,
Akshu
> -Dan
>