2018-04-18 11:04:07

by Vijendar Mukunda

[permalink] [raw]
Subject: [PATCH v2 1/3] ASoC: dwc: I2S Controller instance param added

When multiple I2S controller instances created,
i2s_instance parameter refers to i2s controller instance value.

Signed-off-by: Vijendar Mukunda <[email protected]>
---
v1->v2: removed acp dma driver specific macros from dwc header file
include/sound/designware_i2s.h | 4 ++++
sound/soc/dwc/dwc-i2s.c | 1 +
sound/soc/dwc/local.h | 1 +
3 files changed, 6 insertions(+)

diff --git a/include/sound/designware_i2s.h b/include/sound/designware_i2s.h
index 830f5ca..2a94e7c 100644
--- a/include/sound/designware_i2s.h
+++ b/include/sound/designware_i2s.h
@@ -44,6 +44,10 @@ struct i2s_platform_data {
int channel;
u32 snd_fmts;
u32 snd_rates;
+ /* i2s_instance parameter returns I2S controller instance value
+ * when multiple I2S controllers instantiated
+ */
+ u32 i2s_instance;

#define DW_I2S_QUIRK_COMP_REG_OFFSET (1 << 0)
#define DW_I2S_QUIRK_COMP_PARAM1 (1 << 1)
diff --git a/sound/soc/dwc/dwc-i2s.c b/sound/soc/dwc/dwc-i2s.c
index 65112b9..58f81a4 100644
--- a/sound/soc/dwc/dwc-i2s.c
+++ b/sound/soc/dwc/dwc-i2s.c
@@ -553,6 +553,7 @@ static int dw_configure_dai_by_pd(struct dw_i2s_dev *dev,

if (dev->quirks & DW_I2S_QUIRK_16BIT_IDX_OVERRIDE)
idx = 1;
+ dev->i2s_instance = pdata->i2s_instance;
/* Set DMA slaves info */
dev->play_dma_data.pd.data = pdata->play_dma_data;
dev->capture_dma_data.pd.data = pdata->capture_dma_data;
diff --git a/sound/soc/dwc/local.h b/sound/soc/dwc/local.h
index 91dc70a..e89e464 100644
--- a/sound/soc/dwc/local.h
+++ b/sound/soc/dwc/local.h
@@ -98,6 +98,7 @@ struct dw_i2s_dev {
u32 ccr;
u32 xfer_resolution;
u32 fifo_th;
+ u32 i2s_instance;

/* data related to DMA transfers b/w i2s and DMAC */
union dw_i2s_snd_dma_data play_dma_data;
--
2.7.4



2018-04-18 11:26:44

by Mark Brown

[permalink] [raw]
Subject: Re: [PATCH v2 1/3] ASoC: dwc: I2S Controller instance param added

On Wed, Apr 18, 2018 at 04:34:52PM +0530, Vijendar Mukunda wrote:
> When multiple I2S controller instances created,
> i2s_instance parameter refers to i2s controller instance value.

You're missing the point here a bit - it's not just the defines for the
magic numbers that are the problem, it's the whole idea of passing
instance numbers around like this that's the big problem. Whatever you
are trying to do here is most likely better accomplished at the machine
driver level. If I'm missing something here and this is a useful
concept to have in the driver it really needs to be articulated much
more clearly than in your very brief changelog, and most likely done at
the subsystem level (though the fact that we've managed to get this far
without needing it is a bit of a red flag).


Attachments:
(No filename) (802.00 B)
signature.asc (499.00 B)
Download all attachments

2018-04-18 12:47:23

by Vijendar Mukunda

[permalink] [raw]
Subject: Re: [PATCH v2 1/3] ASoC: dwc: I2S Controller instance param added



On Wednesday 18 April 2018 04:54 PM, Mark Brown wrote:
> On Wed, Apr 18, 2018 at 04:34:52PM +0530, Vijendar Mukunda wrote:
>> When multiple I2S controller instances created,
>> i2s_instance parameter refers to i2s controller instance value.
>
> You're missing the point here a bit - it's not just the defines for the
> magic numbers that are the problem, it's the whole idea of passing
> instance numbers around like this that's the big problem. Whatever you
> are trying to do here is most likely better accomplished at the machine
> driver level. If I'm missing something here and this is a useful
> concept to have in the driver it really needs to be articulated much
> more clearly than in your very brief changelog, and most likely done at
> the subsystem level (though the fact that we've managed to get this far
> without needing it is a bit of a red flag).
>

In Audio Coprocessor (ACP), There are three I2S controllers can be
configured/enabled.(I2S SP, I2S MICSP, BT I2S)
Default enabled I2S controller instance is I2S SP instance.
There is a requirement to enable BT I2S controller Instance along with
I2S SP controller instance in one of our platforms Which has multiple
codecs connected to each instance.

AMD GPU ACP driver creates devices for Playback and capture devices for
both the I2S Controller instances using MFD framework.
Designware driver probe call gets invoked for every device creation with
resource information and platform data provided by GPU driver.

We have added one more parameter i2s instance to dwc platform data.
So that AMDGPU ACP Driver will pass I2S controller instance value to dwc
driver while creating device nodes for I2S Controllers.

In ACP DMA Driver acp_dma_open () call, We are retrieving dwc
controller dev data as mentioned below.
dw_i2s_dev *dev = snd_soc_dai_get_drvdata(prtd->cpu_dai);

From dev->i2s_instance , ACP DMA Driver gets to know current I2S
controller instance value.
We want to make ACP DMA driver platform independent one so that it will
work across all platforms.

This is a generic implementation. Any platform which uses Designware I2S
controller can use this implementation when multiple I2S controller
instances are created.
This patch stores the I2S controller instance value in platform data.
Please suggest us, if there is any better way to handle it.

2018-04-18 17:03:10

by Mark Brown

[permalink] [raw]
Subject: Re: [PATCH v2 1/3] ASoC: dwc: I2S Controller instance param added

On Wed, Apr 18, 2018 at 06:18:44PM +0530, Mukunda,Vijendar wrote:
> On Wednesday 18 April 2018 04:54 PM, Mark Brown wrote:
> > On Wed, Apr 18, 2018 at 04:34:52PM +0530, Vijendar Mukunda wrote:

> > > When multiple I2S controller instances created,
> > > i2s_instance parameter refers to i2s controller instance value.

> > You're missing the point here a bit - it's not just the defines for the
> > magic numbers that are the problem, it's the whole idea of passing
> > instance numbers around like this that's the big problem. Whatever you

> In Audio Coprocessor (ACP), There are three I2S controllers can be
> configured/enabled.(I2S SP, I2S MICSP, BT I2S)
> Default enabled I2S controller instance is I2S SP instance.
> There is a requirement to enable BT I2S controller Instance along with
> I2S SP controller instance in one of our platforms Which has multiple codecs
> connected to each instance.

Describing how the system is wired up is the function of the machine
driver. The individual drivers should just take configuration from the
machine driver.

> We have added one more parameter i2s instance to dwc platform data.
> So that AMDGPU ACP Driver will pass I2S controller instance value to dwc
> driver while creating device nodes for I2S Controllers.

This is not the way to do this - you can already get the struct device
for all the children from when they were instantiated, you should be
able to use this to reference them so you can bind the machine driver.
There's facilities for doing this (mostly serializing into dev_name() as
this was mostly used by board file based machines) which are already
used, if there's problems we should extend those rather than hacking
things into every single driver.


Attachments:
(No filename) (1.71 kB)
signature.asc (499.00 B)
Download all attachments