| Message ID | 1348152226-13588-11-git-send-email-mporter@ti.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
* Matt Porter <mporter@ti.com> [120920 07:43]: > For platforms with DT populated, use dma_request_slave_channel() > to acquire the DMA channel. For !DT platforms, we fall back to > explicitly passing the omap_dma_filter_fn() to dma_request_channel(). > Once all platforms boot from DT, the dma_request_channel() path can > be dropped. > > Signed-off-by: Matt Porter <mporter@ti.com> > --- > drivers/spi/spi-omap2-mcspi.c | 68 +++++++++++++++++++++++++++++------------ > 1 file changed, 48 insertions(+), 20 deletions(-) > > diff --git a/drivers/spi/spi-omap2-mcspi.c b/drivers/spi/spi-omap2-mcspi.c > index 9502566..1cf1072 100644 > --- a/drivers/spi/spi-omap2-mcspi.c > +++ b/drivers/spi/spi-omap2-mcspi.c > @@ -104,6 +104,9 @@ struct omap2_mcspi_dma { > > struct completion dma_tx_completion; > struct completion dma_rx_completion; > + > + char dma_rx_ch_name[14]; > + char dma_tx_ch_name[14]; > }; > > /* use PIO for small transfers, avoiding DMA setup/teardown overhead and > @@ -798,14 +801,26 @@ static int omap2_mcspi_request_dma(struct spi_device *spi) > dma_cap_zero(mask); > dma_cap_set(DMA_SLAVE, mask); > sig = mcspi_dma->dma_rx_sync_dev; > - mcspi_dma->dma_rx = dma_request_channel(mask, omap_dma_filter_fn, &sig); > + if (spi->dev.of_node) > + mcspi_dma->dma_rx = > + dma_request_slave_channel(&master->dev, > + mcspi_dma->dma_rx_ch_name); > + else > + mcspi_dma->dma_rx = > + dma_request_channel(mask, omap_dma_filter_fn, &sig); > if (!mcspi_dma->dma_rx) { > dev_err(&spi->dev, "no RX DMA engine channel for McSPI\n"); > return -EAGAIN; > } > Hmm this does not look nice.. We should be able to somehow not to care about the configuration at the mcspi driver level. Regards, Tony
On Thursday 20 September 2012, Tony Lindgren wrote: > > /* use PIO for small transfers, avoiding DMA setup/teardown overhead and > > @@ -798,14 +801,26 @@ static int omap2_mcspi_request_dma(struct spi_device *spi) > > dma_cap_zero(mask); > > dma_cap_set(DMA_SLAVE, mask); > > sig = mcspi_dma->dma_rx_sync_dev; > > - mcspi_dma->dma_rx = dma_request_channel(mask, omap_dma_filter_fn, &sig); > > + if (spi->dev.of_node) > > + mcspi_dma->dma_rx = > > + dma_request_slave_channel(&master->dev, > > + mcspi_dma->dma_rx_ch_name); > > + else > > + mcspi_dma->dma_rx = > > + dma_request_channel(mask, omap_dma_filter_fn, &sig); > > if (!mcspi_dma->dma_rx) { > > dev_err(&spi->dev, "no RX DMA engine channel for McSPI\n"); > > return -EAGAIN; > > } > > > > Hmm this does not look nice.. We should be able to somehow not to care about > the configuration at the mcspi driver level. I agree, but as far as I understand Vinod's plans, we would actually move all drivers over to dma_request_slave_channel() when we have an interface to register the lookup tables from platform code. I think the above is ok for a transitional phase and we can remove the fallback path when we have converted all platforms using this driver to either use DT or move to the new style way of passing the channel configuration. Arnd
* Arnd Bergmann <arnd@arndb.de> [120921 02:19]: > On Thursday 20 September 2012, Tony Lindgren wrote: > > > /* use PIO for small transfers, avoiding DMA setup/teardown overhead and > > > @@ -798,14 +801,26 @@ static int omap2_mcspi_request_dma(struct spi_device *spi) > > > dma_cap_zero(mask); > > > dma_cap_set(DMA_SLAVE, mask); > > > sig = mcspi_dma->dma_rx_sync_dev; > > > - mcspi_dma->dma_rx = dma_request_channel(mask, omap_dma_filter_fn, &sig); > > > + if (spi->dev.of_node) > > > + mcspi_dma->dma_rx = > > > + dma_request_slave_channel(&master->dev, > > > + mcspi_dma->dma_rx_ch_name); > > > + else > > > + mcspi_dma->dma_rx = > > > + dma_request_channel(mask, omap_dma_filter_fn, &sig); > > > if (!mcspi_dma->dma_rx) { > > > dev_err(&spi->dev, "no RX DMA engine channel for McSPI\n"); > > > return -EAGAIN; > > > } > > > > > > > Hmm this does not look nice.. We should be able to somehow not to care about > > the configuration at the mcspi driver level. > > I agree, but as far as I understand Vinod's plans, we would actually move > all drivers over to dma_request_slave_channel() when we have an interface > to register the lookup tables from platform code. > > I think the above is ok for a transitional phase and we can remove the > fallback path when we have converted all platforms using this driver > to either use DT or move to the new style way of passing the channel > configuration. Can't we come up with a version of dma_request_slave_channel that works both ways for now: mcspi_dma->dma_rx = dma_request_slave_channel_compat(mask, omap_dma_filter_fn, &sig, &master->dev, mcspi_dma->dma_rx_ch_name); ... Then it's just question of patching away two lines later on rather than having to add all this if else to all the drivers first, then patching it away again. Regards, Tony
On Fri, Sep 21, 2012 at 08:42:47AM -0700, Tony Lindgren wrote: > * Arnd Bergmann <arnd@arndb.de> [120921 02:19]: > > On Thursday 20 September 2012, Tony Lindgren wrote: > > > > /* use PIO for small transfers, avoiding DMA setup/teardown overhead and > > > > @@ -798,14 +801,26 @@ static int omap2_mcspi_request_dma(struct spi_device *spi) > > > > dma_cap_zero(mask); > > > > dma_cap_set(DMA_SLAVE, mask); > > > > sig = mcspi_dma->dma_rx_sync_dev; > > > > - mcspi_dma->dma_rx = dma_request_channel(mask, omap_dma_filter_fn, &sig); > > > > + if (spi->dev.of_node) > > > > + mcspi_dma->dma_rx = > > > > + dma_request_slave_channel(&master->dev, > > > > + mcspi_dma->dma_rx_ch_name); > > > > + else > > > > + mcspi_dma->dma_rx = > > > > + dma_request_channel(mask, omap_dma_filter_fn, &sig); > > > > if (!mcspi_dma->dma_rx) { > > > > dev_err(&spi->dev, "no RX DMA engine channel for McSPI\n"); > > > > return -EAGAIN; > > > > } > > > > > > > > > > Hmm this does not look nice.. We should be able to somehow not to care about > > > the configuration at the mcspi driver level. > > > > I agree, but as far as I understand Vinod's plans, we would actually move > > all drivers over to dma_request_slave_channel() when we have an interface > > to register the lookup tables from platform code. > > > > I think the above is ok for a transitional phase and we can remove the > > fallback path when we have converted all platforms using this driver > > to either use DT or move to the new style way of passing the channel > > configuration. > > Can't we come up with a version of dma_request_slave_channel that works > both ways for now: > > mcspi_dma->dma_rx = > dma_request_slave_channel_compat(mask, omap_dma_filter_fn, &sig, > &master->dev, mcspi_dma->dma_rx_ch_name); > ... > > Then it's just question of patching away two lines later on rather than > having to add all this if else to all the drivers first, then patching > it away again. I think that something like that is workable with the implementation simply checking for of_node to do the right thing. -Matt
On Fri, 2012-09-21 at 14:37 -0400, Matt Porter wrote: > On Fri, Sep 21, 2012 at 08:42:47AM -0700, Tony Lindgren wrote: > > > > Can't we come up with a version of dma_request_slave_channel that works > > both ways for now: > > > > mcspi_dma->dma_rx = > > dma_request_slave_channel_compat(mask, omap_dma_filter_fn, &sig, > > &master->dev, mcspi_dma->dma_rx_ch_name); > > ... > > > > Then it's just question of patching away two lines later on rather than > > having to add all this if else to all the drivers first, then patching > > it away again. > > I think that something like that is workable with the implementation > simply checking for of_node to do the right thing. Yes, I think it would be better to have common API but underneath two implementations in transitional phase.
On Thu, Sep 27, 2012 at 03:06:34PM +0530, Vinod Koul wrote: > On Fri, 2012-09-21 at 14:37 -0400, Matt Porter wrote: > > On Fri, Sep 21, 2012 at 08:42:47AM -0700, Tony Lindgren wrote: > > > > > > Can't we come up with a version of dma_request_slave_channel that works > > > both ways for now: > > > > > > mcspi_dma->dma_rx = > > > dma_request_slave_channel_compat(mask, omap_dma_filter_fn, &sig, > > > &master->dev, mcspi_dma->dma_rx_ch_name); > > > ... > > > > > > Then it's just question of patching away two lines later on rather than > > > having to add all this if else to all the drivers first, then patching > > > it away again. > > > > I think that something like that is workable with the implementation > > simply checking for of_node to do the right thing. > Yes, I think it would be better to have common API but underneath two > implementations in transitional phase. Ok, I'll implement something for discussion in the v2 series. -Matt
diff --git a/drivers/spi/spi-omap2-mcspi.c b/drivers/spi/spi-omap2-mcspi.c index 9502566..1cf1072 100644 --- a/drivers/spi/spi-omap2-mcspi.c +++ b/drivers/spi/spi-omap2-mcspi.c @@ -104,6 +104,9 @@ struct omap2_mcspi_dma { struct completion dma_tx_completion; struct completion dma_rx_completion; + + char dma_rx_ch_name[14]; + char dma_tx_ch_name[14]; }; /* use PIO for small transfers, avoiding DMA setup/teardown overhead and @@ -798,14 +801,26 @@ static int omap2_mcspi_request_dma(struct spi_device *spi) dma_cap_zero(mask); dma_cap_set(DMA_SLAVE, mask); sig = mcspi_dma->dma_rx_sync_dev; - mcspi_dma->dma_rx = dma_request_channel(mask, omap_dma_filter_fn, &sig); + if (spi->dev.of_node) + mcspi_dma->dma_rx = + dma_request_slave_channel(&master->dev, + mcspi_dma->dma_rx_ch_name); + else + mcspi_dma->dma_rx = + dma_request_channel(mask, omap_dma_filter_fn, &sig); if (!mcspi_dma->dma_rx) { dev_err(&spi->dev, "no RX DMA engine channel for McSPI\n"); return -EAGAIN; } sig = mcspi_dma->dma_tx_sync_dev; - mcspi_dma->dma_tx = dma_request_channel(mask, omap_dma_filter_fn, &sig); + if (spi->dev.of_node) + mcspi_dma->dma_tx = + dma_request_slave_channel(&master->dev, + mcspi_dma->dma_tx_ch_name); + else + mcspi_dma->dma_tx = + dma_request_channel(mask, omap_dma_filter_fn, &sig); if (!mcspi_dma->dma_tx) { dev_err(&spi->dev, "no TX DMA engine channel for McSPI\n"); dma_release_channel(mcspi_dma->dma_rx); @@ -1194,29 +1209,42 @@ static int __devinit omap2_mcspi_probe(struct platform_device *pdev) goto free_master; for (i = 0; i < master->num_chipselect; i++) { - char dma_ch_name[14]; + char *dma_rx_ch_name = mcspi->dma_channels[i].dma_rx_ch_name; + char *dma_tx_ch_name = mcspi->dma_channels[i].dma_tx_ch_name; struct resource *dma_res; - sprintf(dma_ch_name, "rx%d", i); - dma_res = platform_get_resource_byname(pdev, IORESOURCE_DMA, - dma_ch_name); - if (!dma_res) { - dev_dbg(&pdev->dev, "cannot get DMA RX channel\n"); - status = -ENODEV; - break; - } + sprintf(dma_rx_ch_name, "rx%d", i); + if (!pdev->dev.of_node) { + dma_res = + platform_get_resource_byname(pdev, + IORESOURCE_DMA, + dma_rx_ch_name); + if (!dma_res) { + dev_dbg(&pdev->dev, + "cannot get DMA RX channel\n"); + status = -ENODEV; + break; + } - mcspi->dma_channels[i].dma_rx_sync_dev = dma_res->start; - sprintf(dma_ch_name, "tx%d", i); - dma_res = platform_get_resource_byname(pdev, IORESOURCE_DMA, - dma_ch_name); - if (!dma_res) { - dev_dbg(&pdev->dev, "cannot get DMA TX channel\n"); - status = -ENODEV; - break; + mcspi->dma_channels[i].dma_rx_sync_dev = + dma_res->start; } + sprintf(dma_tx_ch_name, "tx%d", i); + if (!pdev->dev.of_node) { + dma_res = + platform_get_resource_byname(pdev, + IORESOURCE_DMA, + dma_tx_ch_name); + if (!dma_res) { + dev_dbg(&pdev->dev, + "cannot get DMA TX channel\n"); + status = -ENODEV; + break; + } - mcspi->dma_channels[i].dma_tx_sync_dev = dma_res->start; + mcspi->dma_channels[i].dma_tx_sync_dev = + dma_res->start; + } } if (status < 0)
For platforms with DT populated, use dma_request_slave_channel() to acquire the DMA channel. For !DT platforms, we fall back to explicitly passing the omap_dma_filter_fn() to dma_request_channel(). Once all platforms boot from DT, the dma_request_channel() path can be dropped. Signed-off-by: Matt Porter <mporter@ti.com> --- drivers/spi/spi-omap2-mcspi.c | 68 +++++++++++++++++++++++++++++------------ 1 file changed, 48 insertions(+), 20 deletions(-)