[PATCH] spi: davinci: Allow device tree devices to use DMA

[PATCH] spi: davinci: Allow device tree devices to use DMA

[David Lechner]

This makes SPI devices specified in a device tree use DMA when the master
controller has DMA configured.

Since device tree is supposed to only describe the hardware, adding a
configuration option to device tree to enable DMA per-device would not be
acceptable. So, this is the best we can do for now to get SPI devices
working with DMA when using device tree.

Unfortunately, this excludes the possibility of using one SPI device with
DMA and one without on the same master.

I have tested this on LEGO MINDSTORMS EV3 using the NOR flash. Reading the
flash memory would fail with -EIO when DMA is not enabled for the device.

Signed-off-by: David Lechner <david@lechnology.com>
---
 drivers/spi/spi-davinci.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/spi/spi-davinci.c b/drivers/spi/spi-davinci.c
index d36c11b..c6cf73a 100644
--- a/drivers/spi/spi-davinci.c
+++ b/drivers/spi/spi-davinci.c
@@ -388,6 +388,7 @@ static int davinci_spi_setup_transfer(struct spi_device *spi,
 static int davinci_spi_of_setup(struct spi_device *spi)
 {
 	struct davinci_spi_config *spicfg = spi->controller_data;
+	struct davinci_spi *dspi = spi_master_get_devdata(spi->master);
 	struct device_node *np = spi->dev.of_node;
 	u32 prop;
 
@@ -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)
+			spicfg->io_type = SPI_IO_TYPE_DMA;
 	}
 
 	return 0;
-- 
2.7.4

[PATCH] spi: davinci: Allow device tree devices to use DMA

[David Lechner]

This makes SPI devices specified in a device tree use DMA when the master
controller has DMA configured.

Since device tree is supposed to only describe the hardware, adding a
configuration option to device tree to enable DMA per-device would not be
acceptable. So, this is the best we can do for now to get SPI devices
working with DMA when using device tree.

Unfortunately, this excludes the possibility of using one SPI device with
DMA and one without on the same master.

I have tested this on LEGO MINDSTORMS EV3 using the NOR flash. Reading the
flash memory would fail with -EIO when DMA is not enabled for the device.

Signed-off-by: David Lechner <david-nq/r/kbU++upp/zk7JDF2g@public.gmane.org>
---
 drivers/spi/spi-davinci.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/spi/spi-davinci.c b/drivers/spi/spi-davinci.c
index d36c11b..c6cf73a 100644
--- a/drivers/spi/spi-davinci.c
+++ b/drivers/spi/spi-davinci.c
@@ -388,6 +388,7 @@ static int davinci_spi_setup_transfer(struct spi_device *spi,
 static int davinci_spi_of_setup(struct spi_device *spi)
 {
 	struct davinci_spi_config *spicfg = spi->controller_data;
+	struct davinci_spi *dspi = spi_master_get_devdata(spi->master);
 	struct device_node *np = spi->dev.of_node;
 	u32 prop;
 
@@ -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)
+			spicfg->io_type = SPI_IO_TYPE_DMA;
 	}
 
 	return 0;
-- 
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] spi: davinci: Allow device tree devices to use DMA

[David Lechner]

This makes SPI devices specified in a device tree use DMA when the master
controller has DMA configured.

Since device tree is supposed to only describe the hardware, adding a
configuration option to device tree to enable DMA per-device would not be
acceptable. So, this is the best we can do for now to get SPI devices
working with DMA when using device tree.

Unfortunately, this excludes the possibility of using one SPI device with
DMA and one without on the same master.

I have tested this on LEGO MINDSTORMS EV3 using the NOR flash. Reading the
flash memory would fail with -EIO when DMA is not enabled for the device.

Signed-off-by: David Lechner <david@lechnology.com>
---
 drivers/spi/spi-davinci.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/spi/spi-davinci.c b/drivers/spi/spi-davinci.c
index d36c11b..c6cf73a 100644
--- a/drivers/spi/spi-davinci.c
+++ b/drivers/spi/spi-davinci.c
@@ -388,6 +388,7 @@ static int davinci_spi_setup_transfer(struct spi_device *spi,
 static int davinci_spi_of_setup(struct spi_device *spi)
 {
 	struct davinci_spi_config *spicfg = spi->controller_data;
+	struct davinci_spi *dspi = spi_master_get_devdata(spi->master);
 	struct device_node *np = spi->dev.of_node;
 	u32 prop;
 
@@ -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)
+			spicfg->io_type = SPI_IO_TYPE_DMA;
 	}
 
 	return 0;
-- 
2.7.4

Re: [PATCH] spi: davinci: Allow device tree devices to use DMA

[Sekhar Nori]

On Saturday 19 November 2016 10:11 AM, David Lechner wrote:
> This makes SPI devices specified in a device tree use DMA when the master
> controller has DMA configured.
> 
> Since device tree is supposed to only describe the hardware, adding a
> configuration option to device tree to enable DMA per-device would not be
> acceptable. So, this is the best we can do for now to get SPI devices
> working with DMA when using device tree.
> 
> Unfortunately, this excludes the possibility of using one SPI device with
> DMA and one without on the same master.
> 
> I have tested this on LEGO MINDSTORMS EV3 using the NOR flash. Reading the
> flash memory would fail with -EIO when DMA is not enabled for the device.
> 
> Signed-off-by: David Lechner <david@lechnology.com>
> ---
>  drivers/spi/spi-davinci.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/spi/spi-davinci.c b/drivers/spi/spi-davinci.c
> index d36c11b..c6cf73a 100644
> --- a/drivers/spi/spi-davinci.c
> +++ b/drivers/spi/spi-davinci.c
> @@ -388,6 +388,7 @@ static int davinci_spi_setup_transfer(struct spi_device *spi,
>  static int davinci_spi_of_setup(struct spi_device *spi)
>  {
>  	struct davinci_spi_config *spicfg = spi->controller_data;
> +	struct davinci_spi *dspi = spi_master_get_devdata(spi->master);
>  	struct device_node *np = spi->dev.of_node;
>  	u32 prop;
>  
> @@ -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))

> +			spicfg->io_type = SPI_IO_TYPE_DMA;

Otherwise looks good to me.

Thanks,
Sekhar

Re: [PATCH] spi: davinci: Allow device tree devices to use DMA

[Sekhar Nori]

On Saturday 19 November 2016 10:11 AM, David Lechner wrote:
> This makes SPI devices specified in a device tree use DMA when the master
> controller has DMA configured.
> 
> Since device tree is supposed to only describe the hardware, adding a
> configuration option to device tree to enable DMA per-device would not be
> acceptable. So, this is the best we can do for now to get SPI devices
> working with DMA when using device tree.
> 
> Unfortunately, this excludes the possibility of using one SPI device with
> DMA and one without on the same master.
> 
> I have tested this on LEGO MINDSTORMS EV3 using the NOR flash. Reading the
> flash memory would fail with -EIO when DMA is not enabled for the device.
> 
> Signed-off-by: David Lechner <david-nq/r/kbU++upp/zk7JDF2g@public.gmane.org>
> ---
>  drivers/spi/spi-davinci.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/spi/spi-davinci.c b/drivers/spi/spi-davinci.c
> index d36c11b..c6cf73a 100644
> --- a/drivers/spi/spi-davinci.c
> +++ b/drivers/spi/spi-davinci.c
> @@ -388,6 +388,7 @@ static int davinci_spi_setup_transfer(struct spi_device *spi,
>  static int davinci_spi_of_setup(struct spi_device *spi)
>  {
>  	struct davinci_spi_config *spicfg = spi->controller_data;
> +	struct davinci_spi *dspi = spi_master_get_devdata(spi->master);
>  	struct device_node *np = spi->dev.of_node;
>  	u32 prop;
>  
> @@ -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))

> +			spicfg->io_type = SPI_IO_TYPE_DMA;

Otherwise looks good to me.

Thanks,
Sekhar
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] spi: davinci: Allow device tree devices to use DMA

[Sekhar Nori]

On Saturday 19 November 2016 10:11 AM, David Lechner wrote:
> This makes SPI devices specified in a device tree use DMA when the master
> controller has DMA configured.
> 
> Since device tree is supposed to only describe the hardware, adding a
> configuration option to device tree to enable DMA per-device would not be
> acceptable. So, this is the best we can do for now to get SPI devices
> working with DMA when using device tree.
> 
> Unfortunately, this excludes the possibility of using one SPI device with
> DMA and one without on the same master.
> 
> I have tested this on LEGO MINDSTORMS EV3 using the NOR flash. Reading the
> flash memory would fail with -EIO when DMA is not enabled for the device.
> 
> Signed-off-by: David Lechner <david@lechnology.com>
> ---
>  drivers/spi/spi-davinci.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/spi/spi-davinci.c b/drivers/spi/spi-davinci.c
> index d36c11b..c6cf73a 100644
> --- a/drivers/spi/spi-davinci.c
> +++ b/drivers/spi/spi-davinci.c
> @@ -388,6 +388,7 @@ static int davinci_spi_setup_transfer(struct spi_device *spi,
>  static int davinci_spi_of_setup(struct spi_device *spi)
>  {
>  	struct davinci_spi_config *spicfg = spi->controller_data;
> +	struct davinci_spi *dspi = spi_master_get_devdata(spi->master);
>  	struct device_node *np = spi->dev.of_node;
>  	u32 prop;
>  
> @@ -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))

> +			spicfg->io_type = SPI_IO_TYPE_DMA;

Otherwise looks good to me.

Thanks,
Sekhar

Re: [PATCH] spi: davinci: Allow device tree devices to use DMA

[David Lechner]

On 11/20/2016 06:59 AM, Sekhar Nori wrote:
> On Saturday 19 November 2016 10:11 AM, David Lechner wrote:
>> This makes SPI devices specified in a device tree use DMA when the master
>> controller has DMA configured.
>>
>> Since device tree is supposed to only describe the hardware, adding a
>> configuration option to device tree to enable DMA per-device would not be
>> acceptable. So, this is the best we can do for now to get SPI devices
>> working with DMA when using device tree.
>>
>> Unfortunately, this excludes the possibility of using one SPI device with
>> DMA and one without on the same master.
>>
>> I have tested this on LEGO MINDSTORMS EV3 using the NOR flash. Reading the
>> flash memory would fail with -EIO when DMA is not enabled for the device.
>>
>> Signed-off-by: David Lechner <david@lechnology.com>
>> ---
>>  drivers/spi/spi-davinci.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/spi/spi-davinci.c b/drivers/spi/spi-davinci.c
>> index d36c11b..c6cf73a 100644
>> --- a/drivers/spi/spi-davinci.c
>> +++ b/drivers/spi/spi-davinci.c
>> @@ -388,6 +388,7 @@ static int davinci_spi_setup_transfer(struct spi_device *spi,
>>  static int davinci_spi_of_setup(struct spi_device *spi)
>>  {
>>  	struct davinci_spi_config *spicfg = spi->controller_data;
>> +	struct davinci_spi *dspi = spi_master_get_devdata(spi->master);
>>  	struct device_node *np = spi->dev.of_node;
>>  	u32 prop;
>>
>> @@ -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.

>
>> +			spicfg->io_type = SPI_IO_TYPE_DMA;
>
> Otherwise looks good to me.
>
> Thanks,
> Sekhar
>

Re: [PATCH] spi: davinci: Allow device tree devices to use DMA

[David Lechner]

On 11/20/2016 06:59 AM, Sekhar Nori wrote:
> On Saturday 19 November 2016 10:11 AM, David Lechner wrote:
>> This makes SPI devices specified in a device tree use DMA when the master
>> controller has DMA configured.
>>
>> Since device tree is supposed to only describe the hardware, adding a
>> configuration option to device tree to enable DMA per-device would not be
>> acceptable. So, this is the best we can do for now to get SPI devices
>> working with DMA when using device tree.
>>
>> Unfortunately, this excludes the possibility of using one SPI device with
>> DMA and one without on the same master.
>>
>> I have tested this on LEGO MINDSTORMS EV3 using the NOR flash. Reading the
>> flash memory would fail with -EIO when DMA is not enabled for the device.
>>
>> Signed-off-by: David Lechner <david-nq/r/kbU++upp/zk7JDF2g@public.gmane.org>
>> ---
>>  drivers/spi/spi-davinci.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/spi/spi-davinci.c b/drivers/spi/spi-davinci.c
>> index d36c11b..c6cf73a 100644
>> --- a/drivers/spi/spi-davinci.c
>> +++ b/drivers/spi/spi-davinci.c
>> @@ -388,6 +388,7 @@ static int davinci_spi_setup_transfer(struct spi_device *spi,
>>  static int davinci_spi_of_setup(struct spi_device *spi)
>>  {
>>  	struct davinci_spi_config *spicfg = spi->controller_data;
>> +	struct davinci_spi *dspi = spi_master_get_devdata(spi->master);
>>  	struct device_node *np = spi->dev.of_node;
>>  	u32 prop;
>>
>> @@ -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.

>
>> +			spicfg->io_type = SPI_IO_TYPE_DMA;
>
> Otherwise looks good to me.
>
> Thanks,
> Sekhar
>

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] spi: davinci: Allow device tree devices to use DMA

[David Lechner]

On 11/20/2016 06:59 AM, Sekhar Nori wrote:
> On Saturday 19 November 2016 10:11 AM, David Lechner wrote:
>> This makes SPI devices specified in a device tree use DMA when the master
>> controller has DMA configured.
>>
>> Since device tree is supposed to only describe the hardware, adding a
>> configuration option to device tree to enable DMA per-device would not be
>> acceptable. So, this is the best we can do for now to get SPI devices
>> working with DMA when using device tree.
>>
>> Unfortunately, this excludes the possibility of using one SPI device with
>> DMA and one without on the same master.
>>
>> I have tested this on LEGO MINDSTORMS EV3 using the NOR flash. Reading the
>> flash memory would fail with -EIO when DMA is not enabled for the device.
>>
>> Signed-off-by: David Lechner <david@lechnology.com>
>> ---
>>  drivers/spi/spi-davinci.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/spi/spi-davinci.c b/drivers/spi/spi-davinci.c
>> index d36c11b..c6cf73a 100644
>> --- a/drivers/spi/spi-davinci.c
>> +++ b/drivers/spi/spi-davinci.c
>> @@ -388,6 +388,7 @@ static int davinci_spi_setup_transfer(struct spi_device *spi,
>>  static int davinci_spi_of_setup(struct spi_device *spi)
>>  {
>>  	struct davinci_spi_config *spicfg = spi->controller_data;
>> +	struct davinci_spi *dspi = spi_master_get_devdata(spi->master);
>>  	struct device_node *np = spi->dev.of_node;
>>  	u32 prop;
>>
>> @@ -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.

>
>> +			spicfg->io_type = SPI_IO_TYPE_DMA;
>
> Otherwise looks good to me.
>
> Thanks,
> Sekhar
>

Re: [PATCH] spi: davinci: Allow device tree devices to use DMA

[Sekhar Nori]

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

Re: [PATCH] spi: davinci: Allow device tree devices to use DMA

[Sekhar Nori]

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
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] spi: davinci: Allow device tree devices to use DMA

[Sekhar Nori]

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

Re: [PATCH] spi: davinci: Allow device tree devices to use DMA

[David Lechner]

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.

Re: [PATCH] spi: davinci: Allow device tree devices to use DMA

[David Lechner]

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.


--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] spi: davinci: Allow device tree devices to use DMA

[David Lechner]

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.