[PATCH 7/8 v2] ASoC: fsi: remove slave_id settings for DMAEngine

[PATCH 7/8 v2] ASoC: fsi: remove slave_id settings for DMAEngine

[Kuninori Morimoto]

From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>

Current fsi sets dma_slave_config :: slave_id field for DMAEngine,
but it is no longer needed. Let's remove it.

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
v1 -> v2

 - separate into shdma and others

 sound/soc/sh/fsi.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/sound/soc/sh/fsi.c b/sound/soc/sh/fsi.c
index 8869971..c4eb234 100644
--- a/sound/soc/sh/fsi.c
+++ b/sound/soc/sh/fsi.c
@@ -1374,10 +1374,9 @@ static int fsi_dma_probe(struct fsi_priv *fsi, struct fsi_stream *io, struct dev
 				shdma_chan_filter, (void *)io->dma_id,
 				dev, is_play ? "tx" : "rx");
 	if (io->chan) {
-		struct dma_slave_config cfg;
+		struct dma_slave_config cfg = {};
 		int ret;
 
-		cfg.slave_id	= io->dma_id;
 		cfg.dst_addr	= 0; /* use default addr */
 		cfg.src_addr	= 0; /* use default addr */
 		cfg.direction	= is_play ? DMA_MEM_TO_DEV : DMA_DEV_TO_MEM;
-- 
1.7.9.5

Re: [PATCH 7/8 v2] ASoC: fsi: remove slave_id settings for DMAEngine

[Arnd Bergmann]

On Tuesday 20 January 2015 04:18:17 Kuninori Morimoto wrote:
> diff --git a/sound/soc/sh/fsi.c b/sound/soc/sh/fsi.c
> index 8869971..c4eb234 100644
> --- a/sound/soc/sh/fsi.c
> +++ b/sound/soc/sh/fsi.c
> @@ -1374,10 +1374,9 @@ static int fsi_dma_probe(struct fsi_priv *fsi, struct fsi_stream *io, struct dev
>                                 shdma_chan_filter, (void *)io->dma_id,
>                                 dev, is_play ? "tx" : "rx");
>         if (io->chan) {
> -               struct dma_slave_config cfg;
> +               struct dma_slave_config cfg = {};
>                 int ret;
>  
> -               cfg.slave_id    = io->dma_id;
>                 cfg.dst_addr    = 0; /* use default addr */
>                 cfg.src_addr    = 0; /* use default addr */
>                 cfg.direction   = is_play ? DMA_MEM_TO_DEV : DMA_DEV_TO_MEM;

As the dma_slave_config structure is now initialized to all-zeroes, the
two address assignments are not strictly required any more.

However, I also suspect that that particular initialization to zero
is also incorrect: When booting with DT, the dst_addr/src_addr fields
are no longer passed in the platform data for the DMA engine but
are expected to be set by the slave driver.

Also set the correct address width, which has the same problem.

	Arnd

8<---
ASoC: fsi: use correct FIFO address for DMA

Passing the FIFO address using platform data is deprecated and does
not work with devicetree due to the lack of that platform data, so
the fsi driver should instead fill the correct address itself.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>

diff --git a/sound/soc/sh/fsi.c b/sound/soc/sh/fsi.c
index b87b22e88e43..9d5f1f7d0b8c 100644
--- a/sound/soc/sh/fsi.c
+++ b/sound/soc/sh/fsi.c
@@ -250,6 +250,7 @@ struct fsi_clk {
 
 struct fsi_priv {
 	void __iomem *base;
+	resource_size_t phys;
 	struct fsi_master *master;
 
 	struct fsi_stream playback;
@@ -294,6 +295,7 @@ struct fsi_core {
 
 struct fsi_master {
 	void __iomem *base;
+	resource_size_t phys;
 	struct fsi_priv fsia;
 	struct fsi_priv fsib;
 	const struct fsi_core *core;
@@ -1375,8 +1377,10 @@ static int fsi_dma_probe(struct fsi_priv *fsi, struct fsi_stream *io, struct dev
 		int ret;
 
 		cfg.slave_id	= io->dma_id;
-		cfg.dst_addr	= 0; /* use default addr */
-		cfg.src_addr	= 0; /* use default addr */
+		cfg.dst_addr	= is_play ? fsi->phys + REG_DODT : 0;
+		cfg.src_addr	= is_play ? 0 : fsi->phys + REG_DIDT;
+		cfg.dst_addr_width = is_play ? DMA_SLAVE_BUSWIDTH_4_BYTES : 0;
+		cfg.src_addr_width = is_play ? 0 : DMA_SLAVE_BUSWIDTH_4_BYTES;
 		cfg.direction	= is_play ? DMA_MEM_TO_DEV : DMA_DEV_TO_MEM;
 
 		ret = dmaengine_slave_config(io->chan, &cfg);
@@ -1929,6 +1931,7 @@ static int fsi_probe(struct platform_device *pdev)
 
 	master->base = devm_ioremap_nocache(&pdev->dev,
 					    res->start, resource_size(res));
+	master->phys = res->start;
 	if (!master->base) {
 		dev_err(&pdev->dev, "Unable to ioremap FSI registers.\n");
 		return -ENXIO;
@@ -1941,6 +1944,7 @@ static int fsi_probe(struct platform_device *pdev)
 	/* FSI A setting */
 	fsi		= &master->fsia;
 	fsi->base	= master->base;
+	fsi->phys	= master->phys;
 	fsi->master	= master;
 	fsi_port_info_init(fsi, &info.port_a);
 	fsi_handler_init(fsi, &info.port_a);
@@ -1953,6 +1957,7 @@ static int fsi_probe(struct platform_device *pdev)
 	/* FSI B setting */
 	fsi		= &master->fsib;
 	fsi->base	= master->base + 0x40;
+	fsi->phys	= master->phys + 0x40;
 	fsi->master	= master;
 	fsi_port_info_init(fsi, &info.port_b);
 	fsi_handler_init(fsi, &info.port_b);

Re: [PATCH 7/8 v2] ASoC: fsi: remove slave_id settings for DMAEngine

[Kuninori Morimoto]

Hi Arnd

> > diff --git a/sound/soc/sh/fsi.c b/sound/soc/sh/fsi.c
> > index 8869971..c4eb234 100644
> > --- a/sound/soc/sh/fsi.c
> > +++ b/sound/soc/sh/fsi.c
> > @@ -1374,10 +1374,9 @@ static int fsi_dma_probe(struct fsi_priv *fsi, struct fsi_stream *io, struct dev
> >                                 shdma_chan_filter, (void *)io->dma_id,
> >                                 dev, is_play ? "tx" : "rx");
> >         if (io->chan) {
> > -               struct dma_slave_config cfg;
> > +               struct dma_slave_config cfg = {};
> >                 int ret;
> >  
> > -               cfg.slave_id    = io->dma_id;
> >                 cfg.dst_addr    = 0; /* use default addr */
> >                 cfg.src_addr    = 0; /* use default addr */
> >                 cfg.direction   = is_play ? DMA_MEM_TO_DEV : DMA_DEV_TO_MEM;
> 
> As the dma_slave_config structure is now initialized to all-zeroes, the
> two address assignments are not strictly required any more.
> 
> However, I also suspect that that particular initialization to zero
> is also incorrect: When booting with DT, the dst_addr/src_addr fields
> are no longer passed in the platform data for the DMA engine but
> are expected to be set by the slave driver.
> 
> Also set the correct address width, which has the same problem.

Thank you for pointing it.
But, unfortunately, this FSI driver is not used from DT.
(It is supporting DT probe, but no one use it)
So, it is assuming that dst_addr/src_addr fields are come
from platform data.

Best regards
---
Kuninori Morimoto

Re: [PATCH 7/8 v2] ASoC: fsi: remove slave_id settings for DMAEngine

[Arnd Bergmann]

On Wednesday 21 January 2015 01:33:10 Kuninori Morimoto wrote:
> Hi Arnd
> 
> > > diff --git a/sound/soc/sh/fsi.c b/sound/soc/sh/fsi.c
> > > index 8869971..c4eb234 100644
> > > --- a/sound/soc/sh/fsi.c
> > > +++ b/sound/soc/sh/fsi.c
> > > @@ -1374,10 +1374,9 @@ static int fsi_dma_probe(struct fsi_priv *fsi, struct fsi_stream *io, struct dev
> > >                                 shdma_chan_filter, (void *)io->dma_id,
> > >                                 dev, is_play ? "tx" : "rx");
> > >         if (io->chan) {
> > > -               struct dma_slave_config cfg;
> > > +               struct dma_slave_config cfg = {};
> > >                 int ret;
> > >  
> > > -               cfg.slave_id    = io->dma_id;
> > >                 cfg.dst_addr    = 0; /* use default addr */
> > >                 cfg.src_addr    = 0; /* use default addr */
> > >                 cfg.direction   = is_play ? DMA_MEM_TO_DEV : DMA_DEV_TO_MEM;
> > 
> > As the dma_slave_config structure is now initialized to all-zeroes, the
> > two address assignments are not strictly required any more.
> > 
> > However, I also suspect that that particular initialization to zero
> > is also incorrect: When booting with DT, the dst_addr/src_addr fields
> > are no longer passed in the platform data for the DMA engine but
> > are expected to be set by the slave driver.
> > 
> > Also set the correct address width, which has the same problem.
> 
> Thank you for pointing it.
> But, unfortunately, this FSI driver is not used from DT.
> (It is supporting DT probe, but no one use it)
> So, it is assuming that dst_addr/src_addr fields are come
> from platform data.

I see. My patch would still be correct, right?

I think the way that Guennadi Liakhovetski started the conversion of
shmobile dmaengine drivers, each slave driver either uses a custom
filter function that relies on the platform data and will not work
with DT, or it uses dma_request_slave_channel_compat() which will
work on both, and then uses dmaengine_slave_config() to pass the
extra settings. This driver is the one exception you have since it
uses a combination of the two approaches.

	Arnd

Re: [PATCH 7/8 v2] ASoC: fsi: remove slave_id settings for DMAEngine

[Kuninori Morimoto]

Hi Arnd

> > Thank you for pointing it.
> > But, unfortunately, this FSI driver is not used from DT.
> > (It is supporting DT probe, but no one use it)
> > So, it is assuming that dst_addr/src_addr fields are come
> > from platform data.
> 
> I see. My patch would still be correct, right?
> 
> I think the way that Guennadi Liakhovetski started the conversion of
> shmobile dmaengine drivers, each slave driver either uses a custom
> filter function that relies on the platform data and will not work
> with DT, or it uses dma_request_slave_channel_compat() which will
> work on both, and then uses dmaengine_slave_config() to pass the
> extra settings. This driver is the one exception you have since it
> uses a combination of the two approaches.

I understand.
will fix in v3 patch

Best regards
---
Kuninori Morimoto

Re: [PATCH 7/8 v2] ASoC: fsi: remove slave_id settings for DMAEngine

[Kuninori Morimoto]

Hi Arnd, again

> > > Thank you for pointing it.
> > > But, unfortunately, this FSI driver is not used from DT.
> > > (It is supporting DT probe, but no one use it)
> > > So, it is assuming that dst_addr/src_addr fields are come
> > > from platform data.
> > 
> > I see. My patch would still be correct, right?
> > 
> > I think the way that Guennadi Liakhovetski started the conversion of
> > shmobile dmaengine drivers, each slave driver either uses a custom
> > filter function that relies on the platform data and will not work
> > with DT, or it uses dma_request_slave_channel_compat() which will
> > work on both, and then uses dmaengine_slave_config() to pass the
> > extra settings. This driver is the one exception you have since it
> > uses a combination of the two approaches.
> 
> I understand.
> will fix in v3 patch

I understand, but it is different feature patch.
I want to focus to "remove slave_id" on this patch series.
And, I send fixup above dst_addr/src_addr in other patch.
Is this OK ?

Best regards
---
Kuninori Morimoto

Re: [PATCH 7/8 v2] ASoC: fsi: remove slave_id settings for DMAEngine

[Arnd Bergmann]

On Thursday 22 January 2015 01:24:04 Kuninori Morimoto wrote:
> Hi Arnd, again
> 
> > > > Thank you for pointing it.
> > > > But, unfortunately, this FSI driver is not used from DT.
> > > > (It is supporting DT probe, but no one use it)
> > > > So, it is assuming that dst_addr/src_addr fields are come
> > > > from platform data.
> > > 
> > > I see. My patch would still be correct, right?
> > > 
> > > I think the way that Guennadi Liakhovetski started the conversion of
> > > shmobile dmaengine drivers, each slave driver either uses a custom
> > > filter function that relies on the platform data and will not work
> > > with DT, or it uses dma_request_slave_channel_compat() which will
> > > work on both, and then uses dmaengine_slave_config() to pass the
> > > extra settings. This driver is the one exception you have since it
> > > uses a combination of the two approaches.
> > 
> > I understand.
> > will fix in v3 patch
> 
> I understand, but it is different feature patch.
> I want to focus to "remove slave_id" on this patch series.
> And, I send fixup above dst_addr/src_addr in other patch.
> Is this OK ?

Yes, good idea. I was mentioning it mainly so it does not
get lost, but it is unrelated as you say.

	Arnd