| Message ID | 4060965.ckzs3UAxrx@wuerfel (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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);