Patchwork [V2,1/2] spi: dual and quad support(device tree)

login
register
mail settings
Submitter 王宇航
Date Aug. 27, 2013, 12:50 a.m.
Message ID <1377564603-4030-1-git-send-email-wangyuhang2014@gmail.com>
Download mbox | patch
Permalink /patch/270004/
State New
Headers show

Comments

王宇航 - Aug. 27, 2013, 12:50 a.m.
fix two things in patch:
commit id:f477b7fb13df2b843997559ff34e87d054ba6538

1.change the name of property: spi-tx-nbits and spi-rx-nbits to
spi-tmax-nbits and spi-rmax-nbits, aimed to make that name different
from the member of spi_transfer(tx_nbits and rx_nbits). using tmax
and rmax here to describe that it is the max transfer bits that the
members in spi_transfer(tx_nbits and rx_nbits) can reach.
2.change property spi-tmax-nbits and spi-rmax-nbits to optional.
If User don't set spi-tmax-nbits or spi-rmax-nbits, spi device mode
should be regarded as SINGLE, not return an error. In such case, user
don't have to modify the old dts files to fit the new spi framework.

Signed-off-by: wangyuhang <wangyuhang2014@gmail.com>
---
 drivers/spi/spi.c |   72 +++++++++++++++++++++++++----------------------------
 1 file changed, 34 insertions(+), 38 deletions(-)
pekon gupta - Aug. 27, 2013, 4:09 a.m.
> 
> fix two things in patch:
> commit id:f477b7fb13df2b843997559ff34e87d054ba6538
> 
> 1.change the name of property: spi-tx-nbits and spi-rx-nbits to
> spi-tmax-nbits and spi-rmax-nbits, aimed to make that name different
> from the member of spi_transfer(tx_nbits and rx_nbits). using tmax
> and rmax here to describe that it is the max transfer bits that the
> members in spi_transfer(tx_nbits and rx_nbits) can reach.
> 2.change property spi-tmax-nbits and spi-rmax-nbits to optional.
> If User don't set spi-tmax-nbits or spi-rmax-nbits, spi device mode
> should be regarded as SINGLE, not return an error. In such case, user
> don't have to modify the old dts files to fit the new spi framework.
> 
> Signed-off-by: wangyuhang <wangyuhang2014@gmail.com>
> ---
>  drivers/spi/spi.c |   72 +++++++++++++++++++++++++--------------------------
> --
>  1 file changed, 34 insertions(+), 38 deletions(-)
> 
> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> index 50f7fc3..8d191f2 100644
> --- a/drivers/spi/spi.c
> +++ b/drivers/spi/spi.c
> @@ -870,48 +870,44 @@ static void of_register_spi_devices(struct
> spi_master *master)
>  			spi->mode |= SPI_3WIRE;
> 
>  		/* Device DUAL/QUAD mode */
> -		prop = of_get_property(nc, "spi-tx-nbits", &len);
> +		prop = of_get_property(nc, "spi-tmax-nbits", &len);
>  		if (!prop || len < sizeof(*prop)) {
> -			dev_err(&master->dev, "%s has no 'spi-tx-nbits'
> property\n",
> -				nc->full_name);
> -			spi_dev_put(spi);
> -			continue;
> -		}
> -		switch (be32_to_cpup(prop)) {
> -		case SPI_NBITS_SINGLE:
> -			break;
> -		case SPI_NBITS_DUAL:
> -			spi->mode |= SPI_TX_DUAL;
> -			break;
> -		case SPI_NBITS_QUAD:
> -			spi->mode |= SPI_TX_QUAD;
> -			break;
> -		default:
> -			dev_err(&master->dev, "spi-tx-nbits value is not
> supported\n");
> -			spi_dev_put(spi);
> -			continue;
> +			/* set tx mode in SINGLE as default */
> +		} else {
> +			switch (be32_to_cpup(prop)) {
> +			case SPI_NBITS_SINGLE:
> +				break;
> +			case SPI_NBITS_DUAL:
> +				spi->mode |= SPI_TX_DUAL;
> +				break;
> +			case SPI_NBITS_QUAD:
> +				spi->mode |= SPI_TX_QUAD;
> +				break;
> +			default:
> +				dev_err(&master->dev,
> +					"spi-tmax-nbits value is not
> supported\n");
> +				spi_dev_put(spi);
> +				continue;
>  		}
> 
> -		prop = of_get_property(nc, "spi-rx-nbits", &len);
> +		prop = of_get_property(nc, "spi-rmax-nbits", &len);
>  		if (!prop || len < sizeof(*prop)) {
> -			dev_err(&master->dev, "%s has no 'spi-rx-nbits'
> property\n",
> -				nc->full_name);
> -			spi_dev_put(spi);
> -			continue;
> -		}
> -		switch (be32_to_cpup(prop)) {
> -		case SPI_NBITS_SINGLE:
> -			break;
> -		case SPI_NBITS_DUAL:
> -			spi->mode |= SPI_RX_DUAL;
> -			break;
> -		case SPI_NBITS_QUAD:
> -			spi->mode |= SPI_RX_QUAD;
> -			break;
> -		default:
> -			dev_err(&master->dev, "spi-rx-nbits value is not
> supported\n");
> -			spi_dev_put(spi);
> -			continue;
> +			/* set rx mode in SINGLE as default */
> +		} else {
> +			switch (be32_to_cpup(prop)) {
> +			case SPI_NBITS_SINGLE:
> +				break;
> +			case SPI_NBITS_DUAL:
> +				spi->mode |= SPI_RX_DUAL;
> +				break;
> +			case SPI_NBITS_QUAD:
> +				spi->mode |= SPI_RX_QUAD;
> +				break;
> +			default:
> +				dev_err(&master->dev,
> +					"spi-rmax-nbits value is not
> supported\n");
> +				spi_dev_put(spi);
> +				continue;
>  		}
> 
>  		/* Device speed */
> --
> 1.7.9.5

You missed out comments provided earlier..
http://lists.infradead.org/pipermail/linux-mtd/2013-August/048374.html

Remember .. this compatibility check is between spi_device->mode
v/s spi_master->mode_bits. This is different from ur V3 patch
which checks for spi_transfer->tx_nbits v/s spi_device->mode.

+ if ((xfer->tx_nbits == SPI_NBITS_DUAL) &&
+ !(spi->mode & (SPI_TX_DUAL | SPI_TX_QUAD)))
+ return -EINVAL;
+ if ((xfer->tx_nbits == SPI_NBITS_QUAD) &&
+ !(spi->mode & SPI_TX_QUAD))
+ return -EINVAL;


with regards, pekon
王宇航 - Aug. 27, 2013, 4:58 a.m.
Hi, Pekon

2013/8/27 Gupta, Pekon <pekon@ti.com>:
>>
>> fix two things in patch:
>> commit id:f477b7fb13df2b843997559ff34e87d054ba6538
>>
>> 1.change the name of property: spi-tx-nbits and spi-rx-nbits to
>> spi-tmax-nbits and spi-rmax-nbits, aimed to make that name different
>> from the member of spi_transfer(tx_nbits and rx_nbits). using tmax
>> and rmax here to describe that it is the max transfer bits that the
>> members in spi_transfer(tx_nbits and rx_nbits) can reach.
>> 2.change property spi-tmax-nbits and spi-rmax-nbits to optional.
>> If User don't set spi-tmax-nbits or spi-rmax-nbits, spi device mode
>> should be regarded as SINGLE, not return an error. In such case, user
>> don't have to modify the old dts files to fit the new spi framework.
>>
>> Signed-off-by: wangyuhang <wangyuhang2014@gmail.com>
>> ---
>>  drivers/spi/spi.c |   72 +++++++++++++++++++++++++--------------------------
>> --
>>  1 file changed, 34 insertions(+), 38 deletions(-)
>>
>> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
>> index 50f7fc3..8d191f2 100644
>> --- a/drivers/spi/spi.c
>> +++ b/drivers/spi/spi.c
>> @@ -870,48 +870,44 @@ static void of_register_spi_devices(struct
>> spi_master *master)
>>                       spi->mode |= SPI_3WIRE;
>>
>>               /* Device DUAL/QUAD mode */
>> -             prop = of_get_property(nc, "spi-tx-nbits", &len);
>> +             prop = of_get_property(nc, "spi-tmax-nbits", &len);
>>               if (!prop || len < sizeof(*prop)) {
>> -                     dev_err(&master->dev, "%s has no 'spi-tx-nbits'
>> property\n",
>> -                             nc->full_name);
>> -                     spi_dev_put(spi);
>> -                     continue;
>> -             }
>> -             switch (be32_to_cpup(prop)) {
>> -             case SPI_NBITS_SINGLE:
>> -                     break;
>> -             case SPI_NBITS_DUAL:
>> -                     spi->mode |= SPI_TX_DUAL;
>> -                     break;
>> -             case SPI_NBITS_QUAD:
>> -                     spi->mode |= SPI_TX_QUAD;
>> -                     break;
>> -             default:
>> -                     dev_err(&master->dev, "spi-tx-nbits value is not
>> supported\n");
>> -                     spi_dev_put(spi);
>> -                     continue;
>> +                     /* set tx mode in SINGLE as default */
>> +             } else {
>> +                     switch (be32_to_cpup(prop)) {
>> +                     case SPI_NBITS_SINGLE:
>> +                             break;
>> +                     case SPI_NBITS_DUAL:
>> +                             spi->mode |= SPI_TX_DUAL;
>> +                             break;
>> +                     case SPI_NBITS_QUAD:
>> +                             spi->mode |= SPI_TX_QUAD;
>> +                             break;
>> +                     default:
>> +                             dev_err(&master->dev,
>> +                                     "spi-tmax-nbits value is not
>> supported\n");
>> +                             spi_dev_put(spi);
>> +                             continue;
>>               }
>>
>> -             prop = of_get_property(nc, "spi-rx-nbits", &len);
>> +             prop = of_get_property(nc, "spi-rmax-nbits", &len);
>>               if (!prop || len < sizeof(*prop)) {
>> -                     dev_err(&master->dev, "%s has no 'spi-rx-nbits'
>> property\n",
>> -                             nc->full_name);
>> -                     spi_dev_put(spi);
>> -                     continue;
>> -             }
>> -             switch (be32_to_cpup(prop)) {
>> -             case SPI_NBITS_SINGLE:
>> -                     break;
>> -             case SPI_NBITS_DUAL:
>> -                     spi->mode |= SPI_RX_DUAL;
>> -                     break;
>> -             case SPI_NBITS_QUAD:
>> -                     spi->mode |= SPI_RX_QUAD;
>> -                     break;
>> -             default:
>> -                     dev_err(&master->dev, "spi-rx-nbits value is not
>> supported\n");
>> -                     spi_dev_put(spi);
>> -                     continue;
>> +                     /* set rx mode in SINGLE as default */
>> +             } else {
>> +                     switch (be32_to_cpup(prop)) {
>> +                     case SPI_NBITS_SINGLE:
>> +                             break;
>> +                     case SPI_NBITS_DUAL:
>> +                             spi->mode |= SPI_RX_DUAL;
>> +                             break;
>> +                     case SPI_NBITS_QUAD:
>> +                             spi->mode |= SPI_RX_QUAD;
>> +                             break;
>> +                     default:
>> +                             dev_err(&master->dev,
>> +                                     "spi-rmax-nbits value is not
>> supported\n");
>> +                             spi_dev_put(spi);
>> +                             continue;
>>               }
>>
>>               /* Device speed */
>> --
>> 1.7.9.5
>
> You missed out comments provided earlier..
> http://lists.infradead.org/pipermail/linux-mtd/2013-August/048374.html
>
> Remember .. this compatibility check is between spi_device->mode
> v/s spi_master->mode_bits. This is different from ur V3 patch
> which checks for spi_transfer->tx_nbits v/s spi_device->mode.
>
> + if ((xfer->tx_nbits == SPI_NBITS_DUAL) &&
> + !(spi->mode & (SPI_TX_DUAL | SPI_TX_QUAD)))
> + return -EINVAL;
> + if ((xfer->tx_nbits == SPI_NBITS_QUAD) &&
> + !(spi->mode & SPI_TX_QUAD))
> + return -EINVAL;
>
>
Sorry for my mis-understanding. But here I want to regard DUAL / QUAD
mode just as other mode, such as CPHA, CPOL and so on. To other mode,
spi framework didn't do such check, example:
---------------------------------------------------------------------------------
/* Mode (clock phase/polarity/etc.) */
if (of_find_property(nc, "spi-cpha", NULL))
    spi->mode |= SPI_CPHA;
if (of_find_property(nc, "spi-cpol", NULL))
    spi->mode |= SPI_CPOL;
---------------------------------------------------------------------------------

Spi framework do this check in spi_setup as follow:
---------------------------------------------------------------------------------
bad_bits = spi->mode & ~spi->master->mode_bits;
if (bad_bits) {
    dev_err(&spi->dev, "setup: unsupported mode bits %x\n",
          bad_bits);
    return -EINVAL;
}
---------------------------------------------------------------------------------
So to keep that consistant, I don't think it is necessary to add the
check when setting the mode.

Best regards
王宇航 - Aug. 27, 2013, 7:56 a.m.
Hi, Pekon

What about the comment below, do I still need to check the mode before setting.

>> You missed out comments provided earlier..
>> http://lists.infradead.org/pipermail/linux-mtd/2013-August/048374.html
>>
>> Remember .. this compatibility check is between spi_device->mode
>> v/s spi_master->mode_bits. This is different from ur V3 patch
>> which checks for spi_transfer->tx_nbits v/s spi_device->mode.
>>
>> + if ((xfer->tx_nbits == SPI_NBITS_DUAL) &&
>> + !(spi->mode & (SPI_TX_DUAL | SPI_TX_QUAD)))
>> + return -EINVAL;
>> + if ((xfer->tx_nbits == SPI_NBITS_QUAD) &&
>> + !(spi->mode & SPI_TX_QUAD))
>> + return -EINVAL;
>>
>>
> Sorry for my mis-understanding. But here I want to regard DUAL / QUAD
> mode just as other mode, such as CPHA, CPOL and so on. To other mode,
> spi framework didn't do such check, example:
> ---------------------------------------------------------------------------------
> /* Mode (clock phase/polarity/etc.) */
> if (of_find_property(nc, "spi-cpha", NULL))
>     spi->mode |= SPI_CPHA;
> if (of_find_property(nc, "spi-cpol", NULL))
>     spi->mode |= SPI_CPOL;
> ---------------------------------------------------------------------------------
>
> Spi framework do this check in spi_setup as follow:
> ---------------------------------------------------------------------------------
> bad_bits = spi->mode & ~spi->master->mode_bits;
> if (bad_bits) {
>     dev_err(&spi->dev, "setup: unsupported mode bits %x\n",
>           bad_bits);
>     return -EINVAL;
> }
> ---------------------------------------------------------------------------------
> So to keep that consistant, I don't think it is necessary to add the
> check when setting the mode.
>
> Best regards
pekon gupta - Aug. 27, 2013, 8:37 a.m.
> 
> Hi, Pekon
> 
> What about the comment below, do I still need to check the mode before
> setting.
> 
I would suggest so :-)
 (a) 'mode == CPHA, CPOL'
(b) 'mode==SPI_TX_QUAD | SPI_TX_DUAL'

Though I would have suggested to add compatibility checks for both (a)
and (b), so that all mis-matches are identified during device probe.
But, as you patch newly adds (b), it would look complete if it adds
compatibility for at-least (b).

But if you are not keen, its okay. For your current patch..

Acked-by: Pekon Gupta <pekon@ti.com>


with regards, pekon

> >> You missed out comments provided earlier..
> >> http://lists.infradead.org/pipermail/linux-mtd/2013-August/048374.html
> >>
> >> Remember .. this compatibility check is between spi_device->mode
> >> v/s spi_master->mode_bits. This is different from ur V3 patch
> >> which checks for spi_transfer->tx_nbits v/s spi_device->mode.
> >>
> >> + if ((xfer->tx_nbits == SPI_NBITS_DUAL) &&
> >> + !(spi->mode & (SPI_TX_DUAL | SPI_TX_QUAD)))
> >> + return -EINVAL;
> >> + if ((xfer->tx_nbits == SPI_NBITS_QUAD) &&
> >> + !(spi->mode & SPI_TX_QUAD))
> >> + return -EINVAL;
> >>
> >>
> > Sorry for my mis-understanding. But here I want to regard DUAL / QUAD
> > mode just as other mode, such as CPHA, CPOL and so on. To other mode,
> > spi framework didn't do such check, example:
> > ---------------------------------------------------------------------------------
> > /* Mode (clock phase/polarity/etc.) */
> > if (of_find_property(nc, "spi-cpha", NULL))
> >     spi->mode |= SPI_CPHA;
> > if (of_find_property(nc, "spi-cpol", NULL))
> >     spi->mode |= SPI_CPOL;
> > ---------------------------------------------------------------------------------
> >
> > Spi framework do this check in spi_setup as follow:
> > ---------------------------------------------------------------------------------
> > bad_bits = spi->mode & ~spi->master->mode_bits;
> > if (bad_bits) {
> >     dev_err(&spi->dev, "setup: unsupported mode bits %x\n",
> >           bad_bits);
> >     return -EINVAL;
> > }
> > ---------------------------------------------------------------------------------
> > So to keep that consistant, I don't think it is necessary to add the
> > check when setting the mode.
> >
> > Best regards
Stephen Warren - Aug. 30, 2013, 9:20 p.m.
On 08/26/2013 06:50 PM, wangyuhang wrote:
> fix two things in patch:
> commit id:f477b7fb13df2b843997559ff34e87d054ba6538
> 
> 1.change the name of property: spi-tx-nbits and spi-rx-nbits to
> spi-tmax-nbits and spi-rmax-nbits, aimed to make that name different
> from the member of spi_transfer(tx_nbits and rx_nbits). using tmax
> and rmax here to describe that it is the max transfer bits that the
> members in spi_transfer(tx_nbits and rx_nbits) can reach.
> 2.change property spi-tmax-nbits and spi-rmax-nbits to optional.
> If User don't set spi-tmax-nbits or spi-rmax-nbits, spi device mode
> should be regarded as SINGLE, not return an error. In such case, user
> don't have to modify the old dts files to fit the new spi framework.

> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c

> +		} else {
> +			switch (be32_to_cpup(prop)) {
> +			case SPI_NBITS_SINGLE:
> +				break;
> +			case SPI_NBITS_DUAL:
> +				spi->mode |= SPI_TX_DUAL;
> +				break;
> +			case SPI_NBITS_QUAD:
> +				spi->mode |= SPI_TX_QUAD;
> +				break;
> +			default:
> +				dev_err(&master->dev,
> +					"spi-tmax-nbits value is not supported\n");
> +				spi_dev_put(spi);
> +				continue;

You're missing a closing brace here; this patch doesn't compile...

>  		}

Patch

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 50f7fc3..8d191f2 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -870,48 +870,44 @@  static void of_register_spi_devices(struct spi_master *master)
 			spi->mode |= SPI_3WIRE;
 
 		/* Device DUAL/QUAD mode */
-		prop = of_get_property(nc, "spi-tx-nbits", &len);
+		prop = of_get_property(nc, "spi-tmax-nbits", &len);
 		if (!prop || len < sizeof(*prop)) {
-			dev_err(&master->dev, "%s has no 'spi-tx-nbits' property\n",
-				nc->full_name);
-			spi_dev_put(spi);
-			continue;
-		}
-		switch (be32_to_cpup(prop)) {
-		case SPI_NBITS_SINGLE:
-			break;
-		case SPI_NBITS_DUAL:
-			spi->mode |= SPI_TX_DUAL;
-			break;
-		case SPI_NBITS_QUAD:
-			spi->mode |= SPI_TX_QUAD;
-			break;
-		default:
-			dev_err(&master->dev, "spi-tx-nbits value is not supported\n");
-			spi_dev_put(spi);
-			continue;
+			/* set tx mode in SINGLE as default */
+		} else {
+			switch (be32_to_cpup(prop)) {
+			case SPI_NBITS_SINGLE:
+				break;
+			case SPI_NBITS_DUAL:
+				spi->mode |= SPI_TX_DUAL;
+				break;
+			case SPI_NBITS_QUAD:
+				spi->mode |= SPI_TX_QUAD;
+				break;
+			default:
+				dev_err(&master->dev,
+					"spi-tmax-nbits value is not supported\n");
+				spi_dev_put(spi);
+				continue;
 		}
 
-		prop = of_get_property(nc, "spi-rx-nbits", &len);
+		prop = of_get_property(nc, "spi-rmax-nbits", &len);
 		if (!prop || len < sizeof(*prop)) {
-			dev_err(&master->dev, "%s has no 'spi-rx-nbits' property\n",
-				nc->full_name);
-			spi_dev_put(spi);
-			continue;
-		}
-		switch (be32_to_cpup(prop)) {
-		case SPI_NBITS_SINGLE:
-			break;
-		case SPI_NBITS_DUAL:
-			spi->mode |= SPI_RX_DUAL;
-			break;
-		case SPI_NBITS_QUAD:
-			spi->mode |= SPI_RX_QUAD;
-			break;
-		default:
-			dev_err(&master->dev, "spi-rx-nbits value is not supported\n");
-			spi_dev_put(spi);
-			continue;
+			/* set rx mode in SINGLE as default */
+		} else {
+			switch (be32_to_cpup(prop)) {
+			case SPI_NBITS_SINGLE:
+				break;
+			case SPI_NBITS_DUAL:
+				spi->mode |= SPI_RX_DUAL;
+				break;
+			case SPI_NBITS_QUAD:
+				spi->mode |= SPI_RX_QUAD;
+				break;
+			default:
+				dev_err(&master->dev,
+					"spi-rmax-nbits value is not supported\n");
+				spi_dev_put(spi);
+				continue;
 		}
 
 		/* Device speed */