[PATCH 1/2] spi: atmel: convert to dma_request_slave_channel_compat()

[PATCH 1/2] spi: atmel: convert to dma_request_slave_channel_compat()

[Richard Genoud]

Use generic DMA DT helper.
Platforms booting with or without DT populated are both supported.

Based on Ludovic Desroches <ludovic.desroches@atmel.com> patchset
"ARM: at91: move to generic DMA device tree binding"

Signed-off-by: Richard Genoud <richard.genoud@gmail.com>
---
 drivers/spi/spi-atmel.c |   42 +++++++++++++++++++++++++++---------------
 1 file changed, 27 insertions(+), 15 deletions(-)

rebased on linux-next next-20130531

diff --git a/drivers/spi/spi-atmel.c b/drivers/spi/spi-atmel.c
index 31cfc87..ea1ec00 100644
--- a/drivers/spi/spi-atmel.c
+++ b/drivers/spi/spi-atmel.c
@@ -424,10 +424,15 @@ static int atmel_spi_dma_slave_config(struct atmel_spi *as,
 	return err;
 }
 
-static bool filter(struct dma_chan *chan, void *slave)
+static bool filter(struct dma_chan *chan, void *pdata)
 {
-	struct	at_dma_slave *sl = slave;
+	struct atmel_spi_dma *sl_pdata = pdata;
+	struct at_dma_slave *sl;
 
+	if (!sl_pdata)
+		return false;
+
+	sl = &sl_pdata->dma_slave;
 	if (sl->dma_dev == chan->device->dev) {
 		chan->private = sl;
 		return true;
@@ -438,24 +443,31 @@ static bool filter(struct dma_chan *chan, void *slave)
 
 static int atmel_spi_configure_dma(struct atmel_spi *as)
 {
-	struct at_dma_slave *sdata = &as->dma.dma_slave;
 	struct dma_slave_config	slave_config;
+	struct device *dev = &as->pdev->dev;
 	int err;
 
-	if (sdata && sdata->dma_dev) {
-		dma_cap_mask_t mask;
+	dma_cap_mask_t mask;
+	dma_cap_zero(mask);
+	dma_cap_set(DMA_SLAVE, mask);
 
-		/* Try to grab two DMA channels */
-		dma_cap_zero(mask);
-		dma_cap_set(DMA_SLAVE, mask);
-		as->dma.chan_tx = dma_request_channel(mask, filter, sdata);
-		if (as->dma.chan_tx)
-			as->dma.chan_rx =
-				dma_request_channel(mask, filter, sdata);
+	as->dma.chan_tx = dma_request_slave_channel_compat(mask, filter,
+							   &as->dma,
+							   dev, "tx");
+	if (!as->dma.chan_tx) {
+		dev_err(dev,
+			"DMA TX channel not available, SPI unable to use DMA\n");
+		err = -EBUSY;
+		goto error;
 	}
-	if (!as->dma.chan_rx || !as->dma.chan_tx) {
-		dev_err(&as->pdev->dev,
-			"DMA channel not available, SPI unable to use DMA\n");
+
+	as->dma.chan_rx = dma_request_slave_channel_compat(mask, filter,
+							   &as->dma,
+							   dev, "rx");
+
+	if (!as->dma.chan_rx) {
+		dev_err(dev,
+			"DMA RX channel not available, SPI unable to use DMA\n");
 		err = -EBUSY;
 		goto error;
 	}
-- 
1.7.10.4

[PATCH 2/2] ARM: at91: dt: at91sam9x5: add SPI DMA client infos

[Richard Genoud]

Signed-off-by: Richard Genoud <richard.genoud@gmail.com>
---
 arch/arm/boot/dts/at91sam9x5.dtsi |    6 ++++++
 1 file changed, 6 insertions(+)

rebased on linux-next next-20130531
plus Ludovic Desroches' patch "ARM: at91: dt: add header to define at_hdmac configuration"
to get the AT91_DMA_CFG_PER_ID() macro

diff --git a/arch/arm/boot/dts/at91sam9x5.dtsi b/arch/arm/boot/dts/at91sam9x5.dtsi
index af91599..a5942d0 100644
--- a/arch/arm/boot/dts/at91sam9x5.dtsi
+++ b/arch/arm/boot/dts/at91sam9x5.dtsi
@@ -629,6 +629,9 @@
 				compatible = "atmel,at91rm9200-spi";
 				reg = <0xf0000000 0x100>;
 				interrupts = <13 IRQ_TYPE_LEVEL_HIGH 3>;
+				dmas = <&dma0 1 AT91_DMA_CFG_PER_ID(1)>,
+				       <&dma0 1 AT91_DMA_CFG_PER_ID(2)>;
+				dma-names = "tx", "rx";
 				pinctrl-names = "default";
 				pinctrl-0 = <&pinctrl_spi0>;
 				status = "disabled";
@@ -640,6 +643,9 @@
 				compatible = "atmel,at91rm9200-spi";
 				reg = <0xf0004000 0x100>;
 				interrupts = <14 IRQ_TYPE_LEVEL_HIGH 3>;
+				dmas = <&dma1 1 AT91_DMA_CFG_PER_ID(1)>,
+				       <&dma1 1 AT91_DMA_CFG_PER_ID(2)>;
+				dma-names = "tx", "rx";
 				pinctrl-names = "default";
 				pinctrl-0 = <&pinctrl_spi1>;
 				status = "disabled";
-- 
1.7.10.4

[PATCH 1/2] spi: atmel: convert to dma_request_slave_channel_compat()

[Jean-Christophe PLAGNIOL-VILLARD]

On 17:01 Fri 31 May     , Richard Genoud wrote:
> Use generic DMA DT helper.
> Platforms booting with or without DT populated are both supported.
> 
> Based on Ludovic Desroches <ludovic.desroches@atmel.com> patchset
> "ARM: at91: move to generic DMA device tree binding"
> 
> Signed-off-by: Richard Genoud <richard.genoud@gmail.com>

Ludo please take a look but this looks ok to me

Best Regards,
J.
> ---
>  drivers/spi/spi-atmel.c |   42 +++++++++++++++++++++++++++---------------
>  1 file changed, 27 insertions(+), 15 deletions(-)
> 
> rebased on linux-next next-20130531
> 
> diff --git a/drivers/spi/spi-atmel.c b/drivers/spi/spi-atmel.c
> index 31cfc87..ea1ec00 100644
> --- a/drivers/spi/spi-atmel.c
> +++ b/drivers/spi/spi-atmel.c
> @@ -424,10 +424,15 @@ static int atmel_spi_dma_slave_config(struct atmel_spi *as,
>  	return err;
>  }
>  
> -static bool filter(struct dma_chan *chan, void *slave)
> +static bool filter(struct dma_chan *chan, void *pdata)
>  {
> -	struct	at_dma_slave *sl = slave;
> +	struct atmel_spi_dma *sl_pdata = pdata;
> +	struct at_dma_slave *sl;
>  
> +	if (!sl_pdata)
> +		return false;
> +
> +	sl = &sl_pdata->dma_slave;
>  	if (sl->dma_dev == chan->device->dev) {
>  		chan->private = sl;
>  		return true;
> @@ -438,24 +443,31 @@ static bool filter(struct dma_chan *chan, void *slave)
>  
>  static int atmel_spi_configure_dma(struct atmel_spi *as)
>  {
> -	struct at_dma_slave *sdata = &as->dma.dma_slave;
>  	struct dma_slave_config	slave_config;
> +	struct device *dev = &as->pdev->dev;
>  	int err;
>  
> -	if (sdata && sdata->dma_dev) {
> -		dma_cap_mask_t mask;
> +	dma_cap_mask_t mask;
> +	dma_cap_zero(mask);
> +	dma_cap_set(DMA_SLAVE, mask);
>  
> -		/* Try to grab two DMA channels */
> -		dma_cap_zero(mask);
> -		dma_cap_set(DMA_SLAVE, mask);
> -		as->dma.chan_tx = dma_request_channel(mask, filter, sdata);
> -		if (as->dma.chan_tx)
> -			as->dma.chan_rx =
> -				dma_request_channel(mask, filter, sdata);
> +	as->dma.chan_tx = dma_request_slave_channel_compat(mask, filter,
> +							   &as->dma,
> +							   dev, "tx");
> +	if (!as->dma.chan_tx) {
> +		dev_err(dev,
> +			"DMA TX channel not available, SPI unable to use DMA\n");
> +		err = -EBUSY;
> +		goto error;
>  	}
> -	if (!as->dma.chan_rx || !as->dma.chan_tx) {
> -		dev_err(&as->pdev->dev,
> -			"DMA channel not available, SPI unable to use DMA\n");
> +
> +	as->dma.chan_rx = dma_request_slave_channel_compat(mask, filter,
> +							   &as->dma,
> +							   dev, "rx");
> +
> +	if (!as->dma.chan_rx) {
> +		dev_err(dev,
> +			"DMA RX channel not available, SPI unable to use DMA\n");
>  		err = -EBUSY;
>  		goto error;
>  	}
> -- 
> 1.7.10.4
> 

[PATCH 1/2] spi: atmel: convert to dma_request_slave_channel_compat()

[Ludovic Desroches]

On Fri, May 31, 2013 at 05:01:59PM +0200, Richard Genoud wrote:
> Use generic DMA DT helper.
> Platforms booting with or without DT populated are both supported.
> 
> Based on Ludovic Desroches <ludovic.desroches@atmel.com> patchset
> "ARM: at91: move to generic DMA device tree binding"
> 
> Signed-off-by: Richard Genoud <richard.genoud@gmail.com>

Looks fine for me to so
Acked-by: Ludovic Desroches <ludovic.desroches@gmail.com>

One comment below

> ---
>  drivers/spi/spi-atmel.c |   42 +++++++++++++++++++++++++++---------------
>  1 file changed, 27 insertions(+), 15 deletions(-)
> 
> rebased on linux-next next-20130531
> 
> diff --git a/drivers/spi/spi-atmel.c b/drivers/spi/spi-atmel.c
> index 31cfc87..ea1ec00 100644
> --- a/drivers/spi/spi-atmel.c
> +++ b/drivers/spi/spi-atmel.c
> @@ -424,10 +424,15 @@ static int atmel_spi_dma_slave_config(struct atmel_spi *as,
>  	return err;
>  }
>  
> -static bool filter(struct dma_chan *chan, void *slave)
> +static bool filter(struct dma_chan *chan, void *pdata)
>  {
> -	struct	at_dma_slave *sl = slave;
> +	struct atmel_spi_dma *sl_pdata = pdata;
> +	struct at_dma_slave *sl;
>  
> +	if (!sl_pdata)
> +		return false;
> +
> +	sl = &sl_pdata->dma_slave;
>  	if (sl->dma_dev == chan->device->dev) {

I am wondering if a null pointer dereference can happen here. In
atmel_spi_configure_dma sdata was checked so maybe sl should be check
here.

>  		chan->private = sl;
>  		return true;
> @@ -438,24 +443,31 @@ static bool filter(struct dma_chan *chan, void *slave)
>  
>  static int atmel_spi_configure_dma(struct atmel_spi *as)
>  {
> -	struct at_dma_slave *sdata = &as->dma.dma_slave;
>  	struct dma_slave_config	slave_config;
> +	struct device *dev = &as->pdev->dev;
>  	int err;
>  
> -	if (sdata && sdata->dma_dev) {
> -		dma_cap_mask_t mask;
> +	dma_cap_mask_t mask;
> +	dma_cap_zero(mask);
> +	dma_cap_set(DMA_SLAVE, mask);
>  
> -		/* Try to grab two DMA channels */
> -		dma_cap_zero(mask);
> -		dma_cap_set(DMA_SLAVE, mask);
> -		as->dma.chan_tx = dma_request_channel(mask, filter, sdata);
> -		if (as->dma.chan_tx)
> -			as->dma.chan_rx =
> -				dma_request_channel(mask, filter, sdata);
> +	as->dma.chan_tx = dma_request_slave_channel_compat(mask, filter,
> +							   &as->dma,
> +							   dev, "tx");
> +	if (!as->dma.chan_tx) {
> +		dev_err(dev,
> +			"DMA TX channel not available, SPI unable to use DMA\n");
> +		err = -EBUSY;
> +		goto error;
>  	}
> -	if (!as->dma.chan_rx || !as->dma.chan_tx) {
> -		dev_err(&as->pdev->dev,
> -			"DMA channel not available, SPI unable to use DMA\n");
> +
> +	as->dma.chan_rx = dma_request_slave_channel_compat(mask, filter,
> +							   &as->dma,
> +							   dev, "rx");
> +
> +	if (!as->dma.chan_rx) {
> +		dev_err(dev,
> +			"DMA RX channel not available, SPI unable to use DMA\n");
>  		err = -EBUSY;
>  		goto error;
>  	}
> -- 
> 1.7.10.4
> 

[PATCH 1/2] spi: atmel: convert to dma_request_slave_channel_compat()

[Richard Genoud]

2013/6/3 Ludovic Desroches <ludovic.desroches@atmel.com>:
> On Fri, May 31, 2013 at 05:01:59PM +0200, Richard Genoud wrote:
>> Use generic DMA DT helper.
>> Platforms booting with or without DT populated are both supported.
>>
>> Based on Ludovic Desroches <ludovic.desroches@atmel.com> patchset
>> "ARM: at91: move to generic DMA device tree binding"
>>
>> Signed-off-by: Richard Genoud <richard.genoud@gmail.com>
>
> Looks fine for me to so
> Acked-by: Ludovic Desroches <ludovic.desroches@gmail.com>
>
> One comment below
>
Response below.
>> ---
>>  drivers/spi/spi-atmel.c |   42 +++++++++++++++++++++++++++---------------
>>  1 file changed, 27 insertions(+), 15 deletions(-)
>>
>> rebased on linux-next next-20130531
>>
>> diff --git a/drivers/spi/spi-atmel.c b/drivers/spi/spi-atmel.c
>> index 31cfc87..ea1ec00 100644
>> --- a/drivers/spi/spi-atmel.c
>> +++ b/drivers/spi/spi-atmel.c
>> @@ -424,10 +424,15 @@ static int atmel_spi_dma_slave_config(struct atmel_spi *as,
>>       return err;
>>  }
>>
>> -static bool filter(struct dma_chan *chan, void *slave)
>> +static bool filter(struct dma_chan *chan, void *pdata)
>>  {
>> -     struct  at_dma_slave *sl = slave;
>> +     struct atmel_spi_dma *sl_pdata = pdata;
>> +     struct at_dma_slave *sl;
>>
>> +     if (!sl_pdata)
>> +             return false;
>> +
>> +     sl = &sl_pdata->dma_slave;
>>       if (sl->dma_dev == chan->device->dev) {
>
> I am wondering if a null pointer dereference can happen here. In
> atmel_spi_configure_dma sdata was checked so maybe sl should be check
> here.

sdata was checked, but there was no point checking it:
if as is not null, as->dma.dma_slave is defined ; so its address is not null.
it's the same in the filter function now:
if sl_pdata is not null, sl_pdata->dma_slave is defined ; so its
address is not null.

I added a check on sl_pdata in this patch, maybe a (BUG|WARN)_ON()
would have been better, as it really should not happen.


>>               chan->private = sl;
>>               return true;
>> @@ -438,24 +443,31 @@ static bool filter(struct dma_chan *chan, void *slave)
>>
>>  static int atmel_spi_configure_dma(struct atmel_spi *as)
>>  {
>> -     struct at_dma_slave *sdata = &as->dma.dma_slave;
>>       struct dma_slave_config slave_config;
>> +     struct device *dev = &as->pdev->dev;
>>       int err;
>>
>> -     if (sdata && sdata->dma_dev) {
>> -             dma_cap_mask_t mask;
>> +     dma_cap_mask_t mask;
>> +     dma_cap_zero(mask);
>> +     dma_cap_set(DMA_SLAVE, mask);
>>
>> -             /* Try to grab two DMA channels */
>> -             dma_cap_zero(mask);
>> -             dma_cap_set(DMA_SLAVE, mask);
>> -             as->dma.chan_tx = dma_request_channel(mask, filter, sdata);
>> -             if (as->dma.chan_tx)
>> -                     as->dma.chan_rx =
>> -                             dma_request_channel(mask, filter, sdata);
>> +     as->dma.chan_tx = dma_request_slave_channel_compat(mask, filter,
>> +                                                        &as->dma,
>> +                                                        dev, "tx");
>> +     if (!as->dma.chan_tx) {
>> +             dev_err(dev,
>> +                     "DMA TX channel not available, SPI unable to use DMA\n");
>> +             err = -EBUSY;
>> +             goto error;
>>       }
>> -     if (!as->dma.chan_rx || !as->dma.chan_tx) {
>> -             dev_err(&as->pdev->dev,
>> -                     "DMA channel not available, SPI unable to use DMA\n");
>> +
>> +     as->dma.chan_rx = dma_request_slave_channel_compat(mask, filter,
>> +                                                        &as->dma,
>> +                                                        dev, "rx");
>> +
>> +     if (!as->dma.chan_rx) {
>> +             dev_err(dev,
>> +                     "DMA RX channel not available, SPI unable to use DMA\n");
>>               err = -EBUSY;
>>               goto error;
>>       }
>> --
>> 1.7.10.4
>>

Best regards,

Richard.

[PATCH 1/2] spi: atmel: convert to dma_request_slave_channel_compat()

[Mark Brown]

On Fri, May 31, 2013 at 05:01:59PM +0200, Richard Genoud wrote:
> Use generic DMA DT helper.
> Platforms booting with or without DT populated are both supported.
> 
> Based on Ludovic Desroches <ludovic.desroches@atmel.com> patchset
> "ARM: at91: move to generic DMA device tree binding"

Applied, thanks.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130603/6956c3f8/attachment.sig>

[PATCH 1/2] spi: atmel: convert to dma_request_slave_channel_compat()

[Yang Wenyou]

Richard Genoud wrote:
> Use generic DMA DT helper.
> Platforms booting with or without DT populated are both supported.
>
> Based on Ludovic Desroches <ludovic.desroches@atmel.com> patchset
> "ARM: at91: move to generic DMA device tree binding"
>
> Signed-off-by: Richard Genoud <richard.genoud@gmail.com>
> ---
>  drivers/spi/spi-atmel.c |   42 +++++++++++++++++++++++++++---------------
>  1 file changed, 27 insertions(+), 15 deletions(-)
>
> rebased on linux-next next-20130531
>   
Tested on at91sam9g25ek,
Based on linux-next, next-20130603
plus Ludovic Desroches' patch "ARM: at91: dt: add header to define 
at_hdmac configuration"

Tested-by: Wenyou Yang<wenyou.yang@atmel.com>
> diff --git a/drivers/spi/spi-atmel.c b/drivers/spi/spi-atmel.c
> index 31cfc87..ea1ec00 100644
> --- a/drivers/spi/spi-atmel.c
> +++ b/drivers/spi/spi-atmel.c
> @@ -424,10 +424,15 @@ static int atmel_spi_dma_slave_config(struct atmel_spi *as,
>  	return err;
>  }
>  
> -static bool filter(struct dma_chan *chan, void *slave)
> +static bool filter(struct dma_chan *chan, void *pdata)
>  {
> -	struct	at_dma_slave *sl = slave;
> +	struct atmel_spi_dma *sl_pdata = pdata;
> +	struct at_dma_slave *sl;
>  
> +	if (!sl_pdata)
> +		return false;
> +
> +	sl = &sl_pdata->dma_slave;
>  	if (sl->dma_dev == chan->device->dev) {
>  		chan->private = sl;
>  		return true;
> @@ -438,24 +443,31 @@ static bool filter(struct dma_chan *chan, void *slave)
>  
>  static int atmel_spi_configure_dma(struct atmel_spi *as)
>  {
> -	struct at_dma_slave *sdata = &as->dma.dma_slave;
>  	struct dma_slave_config	slave_config;
> +	struct device *dev = &as->pdev->dev;
>  	int err;
>  
> -	if (sdata && sdata->dma_dev) {
> -		dma_cap_mask_t mask;
> +	dma_cap_mask_t mask;
> +	dma_cap_zero(mask);
> +	dma_cap_set(DMA_SLAVE, mask);
>  
> -		/* Try to grab two DMA channels */
> -		dma_cap_zero(mask);
> -		dma_cap_set(DMA_SLAVE, mask);
> -		as->dma.chan_tx = dma_request_channel(mask, filter, sdata);
> -		if (as->dma.chan_tx)
> -			as->dma.chan_rx =
> -				dma_request_channel(mask, filter, sdata);
> +	as->dma.chan_tx = dma_request_slave_channel_compat(mask, filter,
> +							   &as->dma,
> +							   dev, "tx");
> +	if (!as->dma.chan_tx) {
> +		dev_err(dev,
> +			"DMA TX channel not available, SPI unable to use DMA\n");
> +		err = -EBUSY;
> +		goto error;
>  	}
> -	if (!as->dma.chan_rx || !as->dma.chan_tx) {
> -		dev_err(&as->pdev->dev,
> -			"DMA channel not available, SPI unable to use DMA\n");
> +
> +	as->dma.chan_rx = dma_request_slave_channel_compat(mask, filter,
> +							   &as->dma,
> +							   dev, "rx");
> +
> +	if (!as->dma.chan_rx) {
> +		dev_err(dev,
> +			"DMA RX channel not available, SPI unable to use DMA\n");
>  		err = -EBUSY;
>  		goto error;
>  	}
>