Messages in this thread | | | Subject | Re: [PATCH] spi: davinci: Allow device tree devices to use DMA | From | David Lechner <> | Date | Tue, 6 Dec 2016 20:28:10 -0600 |
| |
On 11/21/2016 02:37 AM, Sekhar Nori wrote: > On Sunday 20 November 2016 10:31 PM, David Lechner wrote: > >> On 11/20/2016 06:59 AM, Sekhar Nori wrote: > >>> On Saturday 19 November 2016 10:11 AM, David Lechner wrote: > >>>> @@ -400,6 +401,9 @@ static int davinci_spi_of_setup(struct spi_device >>>> *spi) >>>> if (!of_property_read_u32(np, "ti,spi-wdelay", &prop)) >>>> spicfg->wdelay = (u8)prop; >>>> spi->controller_data = spicfg; >>>> + /* Use DMA for device if master supports it */ >>>> + if (dspi->dma_rx) >>> >>> This should be >>> >>> if (!(IS_ERR(dpsi->dma_rx) || IS_ERR(dspi->dma_tx)) >> >> >> There is the following code in davinci_spi_probe(): >> >> ret = davinci_spi_request_dma(dspi); >> if (ret == -EPROBE_DEFER) { >> goto free_clk; >> } else if (ret) { >> dev_info(&pdev->dev, "DMA is not supported (%d)\n", ret); >> dspi->dma_rx = NULL; >> dspi->dma_tx = NULL; >> } >> >> So, I does not look like it is possible to get anything other than NULL >> or a valid pointer for dpsi->dma_rx and that checking dpsi->dma_tx is >> not necessary. >> >> So, I think if (dspi->dma_rx) is sufficient. In fact the same check is >> used during unwinding if the probe function fails. > > You are right, I see it now. Setting dma_rx to NULL overriding the error > value is confusing since dma_request_chan() itself does not use NULL as > an error value. > > I think it is better to fix the existing code to remove the NULL > overwrite and use IS_ERR() instead. You should probably wait for some > feedback from the SPI maintainer though. > > Thanks, > Sekhar >
There don't seem to be any further comments. Using NULL here makes more sense to me, so I am inclined to leave this patch as-is.
| |