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

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

[wangyuhang]

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


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH V2 2/2] spi: dual and quad support(device tree)

[wangyuhang]

Add spi-tmax-nbits and spi-rmax-nbits for spi slave node.
Modify the related dt document(spi-bus.txt)
 spi-tmax-nbits:Max number of bits slave will use for MOSI(writting)
 spi-rmax-nbits:Max number of bits slave will use for MISO(reading)
Support for spi-tx/rmax-nbits in SPI framework has been picked[1].
[1]: http://comments.gmane.org/gmane.linux.kernel.spi.devel/14420
Commit Id:f477b7fb13df2b843997559ff34e87d054ba6538

Signed-off-by: wangyuhang <wangyuhang2014@gmail.com>
---
 Documentation/devicetree/bindings/spi/spi-bus.txt |   16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/Documentation/devicetree/bindings/spi/spi-bus.txt b/Documentation/devicetree/bindings/spi/spi-bus.txt
index 296015e..211336c 100644
--- a/Documentation/devicetree/bindings/spi/spi-bus.txt
+++ b/Documentation/devicetree/bindings/spi/spi-bus.txt
@@ -55,6 +55,22 @@ contain the following properties.
     		chip select active high
 - spi-3wire       - (optional) Empty property indicating device requires
     		    3-wire mode.
+- spi-tmax-nbits  - (optional) Max number of bits slave will use for
+    		    MOSI(writting)
+- spi-rmax-nbits  - (optional) Max number of bits slave will use for
+    		    MISO(reading)
+
+So if for example the slave has 4 wires for writting and 2 wires for reading,
+and the spi-tx/rx-nbits property should be set as follows:
+
+spi-tmax-nbits = <4>;
+spi-rmax-nbits = <2>;
+
+Now the value that spi-tmax-nbits and spi-rmax-nbits can receive is only
+1(single), 2(dual) and 4(quad). If you don't set spi-tmax-nbits or spi-rmax-nbits,
+spi_device mode will be set in single(1 wire) as default. Another point, if
+property:spi-3wire is set, spi-t/rmax-nbits is forbidden to set to <2 or 4>,
+otherwise, an errro will return.
 
 If a gpio chipselect is used for the SPI slave the gpio number will be passed
 via the cs_gpio
-- 
1.7.9.5


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

RE: [PATCH V2 2/2] spi: dual and quad support(device tree)

[Gupta, Pekon]

> 
> Add spi-tmax-nbits and spi-rmax-nbits for spi slave node.
> Modify the related dt document(spi-bus.txt)
>  spi-tmax-nbits:Max number of bits slave will use for MOSI(writting)
>  spi-rmax-nbits:Max number of bits slave will use for MISO(reading)
> Support for spi-tx/rmax-nbits in SPI framework has been picked[1].
> [1]: http://comments.gmane.org/gmane.linux.kernel.spi.devel/14420
> Commit Id:f477b7fb13df2b843997559ff34e87d054ba6538
> 
> Signed-off-by: wangyuhang <wangyuhang2014@gmail.com>
> ---
>  Documentation/devicetree/bindings/spi/spi-bus.txt |   16
> ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/spi/spi-bus.txt
> b/Documentation/devicetree/bindings/spi/spi-bus.txt
> index 296015e..211336c 100644
> --- a/Documentation/devicetree/bindings/spi/spi-bus.txt
> +++ b/Documentation/devicetree/bindings/spi/spi-bus.txt
> @@ -55,6 +55,22 @@ contain the following properties.
>      		chip select active high
>  - spi-3wire       - (optional) Empty property indicating device requires
>      		    3-wire mode.
> +- spi-tmax-nbits  - (optional) Max number of bits slave will use for
> +    		    MOSI(writting)
> +- spi-rmax-nbits  - (optional) Max number of bits slave will use for
> +    		    MISO(reading)
> +
> +So if for example the slave has 4 wires for writting and 2 wires for reading,
> +and the spi-tx/rx-nbits property should be set as follows:
> +
> +spi-tmax-nbits = <4>;
> +spi-rmax-nbits = <2>;
> +
> +Now the value that spi-tmax-nbits and spi-rmax-nbits can receive is only
> +1(single), 2(dual) and 4(quad). If you don't set spi-tmax-nbits or spi-rmax-
> nbits,
> +spi_device mode will be set in single(1 wire) as default. Another point, if
> +property:spi-3wire is set, spi-t/rmax-nbits is forbidden to set to <2 or 4>,
> +otherwise, an errro will return.
> 
>  If a gpio chipselect is used for the SPI slave the gpio number will be passed
>  via the cs_gpio
> --
> 1.7.9.5

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

with regards, pekon
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

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

[Gupta, Pekon]

> 
> 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
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

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

[yuhang wang]

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

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

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

[yuhang wang]

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

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

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

[Gupta, Pekon]

> 
> 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
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

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

[Stephen Warren]

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...

>  		}


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH V2 2/2] spi: dual and quad support(device tree)

[Stephen Warren]

On 08/26/2013 06:50 PM, wangyuhang wrote:
> Add spi-tmax-nbits and spi-rmax-nbits for spi slave node.
> Modify the related dt document(spi-bus.txt)
>  spi-tmax-nbits:Max number of bits slave will use for MOSI(writting)
>  spi-rmax-nbits:Max number of bits slave will use for MISO(reading)
> Support for spi-tx/rmax-nbits in SPI framework has been picked[1].
> [1]: http://comments.gmane.org/gmane.linux.kernel.spi.devel/14420
> Commit Id:f477b7fb13df2b843997559ff34e87d054ba6538

> diff --git a/Documentation/devicetree/bindings/spi/spi-bus.txt b/Documentation/devicetree/bindings/spi/spi-bus.txt

> +- spi-tmax-nbits  - (optional) Max number of bits slave will use for
> +    		    MOSI(writting)
> +- spi-rmax-nbits  - (optional) Max number of bits slave will use for
> +    		    MISO(reading)

I would suggest the property names

spi-tx-num-wires, spi-rx-num-wires

or:

spi-tx-bus-width, spi-rx-bus-width (this option from Tomasz Figa on IRC)

Those name alone self-document their purpose much more clearly to me.

I would also change the descriptions a bit, resulting in the following
overall:

==========
- spi-tx-num-wires  - (optional) The number of data wires that
                      transfer data from the SPI controller to the
                      SPI device. Defaults to 1 if not present.
- spi-rx-num-wires  - (optional) The number of data wires that
                      transfer data from the SPI device to the
                      SPI controller. Defaults to 1 if not present.
==========

> +So if for example the slave has 4 wires for writting and 2 wires for reading,
> +and the spi-tx/rx-nbits property should be set as follows:
> +
> +spi-tmax-nbits = <4>;
> +spi-rmax-nbits = <2>;
> +
> +Now the value that spi-tmax-nbits and spi-rmax-nbits can receive is only
> +1(single), 2(dual) and 4(quad). If you don't set spi-tmax-nbits or spi-rmax-nbits,
> +spi_device mode will be set in single(1 wire) as default. Another point, if
> +property:spi-3wire is set, spi-t/rmax-nbits is forbidden to set to <2 or 4>,
> +otherwise, an errro will return.

I think most of that explanation can be removed if you use the text I
wrote above.

I'm not sure you should ban spi-3wire if those properties are specified;
can't all those properties be present together, if the num-wires are set
to 1? Actually, isn't spi-3write the default; what does that property mean?

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH V2 2/2] spi: dual and quad support(device tree)

[Tomasz Figa]

Hi,

[Ccing DT binding maintainers]

On Tuesday 27 of August 2013 08:50:03 wangyuhang wrote:
> Add spi-tmax-nbits and spi-rmax-nbits for spi slave node.
> Modify the related dt document(spi-bus.txt)
>  spi-tmax-nbits:Max number of bits slave will use for MOSI(writting)
>  spi-rmax-nbits:Max number of bits slave will use for MISO(reading)
> Support for spi-tx/rmax-nbits in SPI framework has been picked[1].
> [1]: http://comments.gmane.org/gmane.linux.kernel.spi.devel/14420
> Commit Id:f477b7fb13df2b843997559ff34e87d054ba6538
> 
> Signed-off-by: wangyuhang <wangyuhang2014@gmail.com>
> ---
>  Documentation/devicetree/bindings/spi/spi-bus.txt |   16
> ++++++++++++++++ 1 file changed, 16 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/spi/spi-bus.txt
> b/Documentation/devicetree/bindings/spi/spi-bus.txt index
> 296015e..211336c 100644
> --- a/Documentation/devicetree/bindings/spi/spi-bus.txt
> +++ b/Documentation/devicetree/bindings/spi/spi-bus.txt
> @@ -55,6 +55,22 @@ contain the following properties.
>      		chip select active high
>  - spi-3wire       - (optional) Empty property indicating device
> requires 3-wire mode.
> +- spi-tmax-nbits  - (optional) Max number of bits slave will use for
> +    		    MOSI(writting)
> +- spi-rmax-nbits  - (optional) Max number of bits slave will use for
> +    		    MISO(reading)

May I ask for more human-readable names, please?

If we look around, there are already devices using bus-width property, so 
for consistency we could use tx-bus-width and rx-bus-width here or even a 
single bus-width property taking two cells, first tx and second rx?

Let's hear others' opinion on this as well.

Also the properties should be telling information about hardware, so 
probably in this case the meaning would be the number of wires that are 
physically wired on the board.

> +

One or two sentences about what this dual/quad thing is about would be 
nice here, e.g.

	Some SPI controllers and devices support Dual and/or Quad SPI
	mode, which is [...]. It allows [...], etc.

> +So if for example the slave has 4 wires for writting and 2 wires for
> reading, +and the spi-tx/rx-nbits property should be set as follows:
> +
> +spi-tmax-nbits = <4>;
> +spi-rmax-nbits = <2>;
> +
> +Now the value that spi-tmax-nbits and spi-rmax-nbits can receive is
> only +1(single), 2(dual) and 4(quad). If you don't set spi-tmax-nbits
> or spi-rmax-nbits, +spi_device mode will be set in single(1 wire) as
> default. Another point, if +property:spi-3wire is set, spi-t/rmax-nbits
> is forbidden to set to <2 or 4>, +otherwise, an errro will return.

typo: errro

Also an error or "returning an error" is an OS specific term. This 
statement should rather be something along

	Dual/Quad mode is not allowed when 3-wire mode is used.

Best regards,
Tomasz


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH V2 2/2] spi: dual and quad support(device tree)

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 336 bytes --]

On Fri, Aug 30, 2013 at 03:26:44PM -0600, Stephen Warren wrote:

> I would suggest the property names

> spi-tx-num-wires, spi-rx-num-wires

> or:

> spi-tx-bus-width, spi-rx-bus-width (this option from Tomasz Figa on IRC)

> Those name alone self-document their purpose much more clearly to me.

These both seem like good suggestions.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 144 bytes --]

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH V2 2/2] spi: dual and quad support(device tree)

[yuhang wang]

Hi, Stephen

2013/8/31 Stephen Warren <swarren@wwwdotorg.org>:
> On 08/26/2013 06:50 PM, wangyuhang wrote:
>> Add spi-tmax-nbits and spi-rmax-nbits for spi slave node.
>> Modify the related dt document(spi-bus.txt)
>>  spi-tmax-nbits:Max number of bits slave will use for MOSI(writting)
>>  spi-rmax-nbits:Max number of bits slave will use for MISO(reading)
>> Support for spi-tx/rmax-nbits in SPI framework has been picked[1].
>> [1]: http://comments.gmane.org/gmane.linux.kernel.spi.devel/14420
>> Commit Id:f477b7fb13df2b843997559ff34e87d054ba6538
>
>> diff --git a/Documentation/devicetree/bindings/spi/spi-bus.txt b/Documentation/devicetree/bindings/spi/spi-bus.txt
>
>> +- spi-tmax-nbits  - (optional) Max number of bits slave will use for
>> +                 MOSI(writting)
>> +- spi-rmax-nbits  - (optional) Max number of bits slave will use for
>> +                 MISO(reading)
>
> I would suggest the property names
>
> spi-tx-num-wires, spi-rx-num-wires
>
> or:
>
> spi-tx-bus-width, spi-rx-bus-width (this option from Tomasz Figa on IRC)
>
> Those name alone self-document their purpose much more clearly to me.
>
> I would also change the descriptions a bit, resulting in the following
> overall:
>
> ==========
> - spi-tx-num-wires  - (optional) The number of data wires that
>                       transfer data from the SPI controller to the
>                       SPI device. Defaults to 1 if not present.
> - spi-rx-num-wires  - (optional) The number of data wires that
>                       transfer data from the SPI device to the
>                       SPI controller. Defaults to 1 if not present.
> ==========
>

OK, that seems better.

>> +So if for example the slave has 4 wires for writting and 2 wires for reading,
>> +and the spi-tx/rx-nbits property should be set as follows:
>> +
>> +spi-tmax-nbits = <4>;
>> +spi-rmax-nbits = <2>;
>> +
>> +Now the value that spi-tmax-nbits and spi-rmax-nbits can receive is only
>> +1(single), 2(dual) and 4(quad). If you don't set spi-tmax-nbits or spi-rmax-nbits,
>> +spi_device mode will be set in single(1 wire) as default. Another point, if
>> +property:spi-3wire is set, spi-t/rmax-nbits is forbidden to set to <2 or 4>,
>> +otherwise, an errro will return.
>
> I think most of that explanation can be removed if you use the text I
> wrote above.
>
> I'm not sure you should ban spi-3wire if those properties are specified;
> can't all those properties be present together, if the num-wires are set
> to 1? Actually, isn't spi-3write the default; what does that property mean?

Property spi-3write means some controllers only have cs, clk and
sda(SI/SO shared) 3 wires. So if spi-3wire is set, then dual and quad
set should be forbidden. I didn't mean that spi-3wire can't be present
together with spi-tx/rx-num-wires, if set to <1> it's OK. Just can't
be <2 | 4>(dual and quad).

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH V2 2/2] spi: dual and quad support(device tree)

[yuhang wang]

Hi, Tomasz

2013/8/31 Tomasz Figa <tomasz.figa@gmail.com>:
> Hi,
>
> [Ccing DT binding maintainers]
>
> On Tuesday 27 of August 2013 08:50:03 wangyuhang wrote:
>> Add spi-tmax-nbits and spi-rmax-nbits for spi slave node.
>> Modify the related dt document(spi-bus.txt)
>>  spi-tmax-nbits:Max number of bits slave will use for MOSI(writting)
>>  spi-rmax-nbits:Max number of bits slave will use for MISO(reading)
>> Support for spi-tx/rmax-nbits in SPI framework has been picked[1].
>> [1]: http://comments.gmane.org/gmane.linux.kernel.spi.devel/14420
>> Commit Id:f477b7fb13df2b843997559ff34e87d054ba6538
>>
>> Signed-off-by: wangyuhang <wangyuhang2014@gmail.com>
>> ---
>>  Documentation/devicetree/bindings/spi/spi-bus.txt |   16
>> ++++++++++++++++ 1 file changed, 16 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/spi/spi-bus.txt
>> b/Documentation/devicetree/bindings/spi/spi-bus.txt index
>> 296015e..211336c 100644
>> --- a/Documentation/devicetree/bindings/spi/spi-bus.txt
>> +++ b/Documentation/devicetree/bindings/spi/spi-bus.txt
>> @@ -55,6 +55,22 @@ contain the following properties.
>>               chip select active high
>>  - spi-3wire       - (optional) Empty property indicating device
>> requires 3-wire mode.
>> +- spi-tmax-nbits  - (optional) Max number of bits slave will use for
>> +                 MOSI(writting)
>> +- spi-rmax-nbits  - (optional) Max number of bits slave will use for
>> +                 MISO(reading)
>
> May I ask for more human-readable names, please?
>
> If we look around, there are already devices using bus-width property, so
> for consistency we could use tx-bus-width and rx-bus-width here or even a
> single bus-width property taking two cells, first tx and second rx?
>
Yes, you are right. bus_width is really more widely used. So I will correct it.

> Let's hear others' opinion on this as well.
>
> Also the properties should be telling information about hardware, so
> probably in this case the meaning would be the number of wires that are
> physically wired on the board.
>
>> +
>
> One or two sentences about what this dual/quad thing is about would be
> nice here, e.g.
>
>         Some SPI controllers and devices support Dual and/or Quad SPI
>         mode, which is [...]. It allows [...], etc.
>
OK, Thanks.

>> +So if for example the slave has 4 wires for writting and 2 wires for
>> reading, +and the spi-tx/rx-nbits property should be set as follows:
>> +
>> +spi-tmax-nbits = <4>;
>> +spi-rmax-nbits = <2>;
>> +
>> +Now the value that spi-tmax-nbits and spi-rmax-nbits can receive is
>> only +1(single), 2(dual) and 4(quad). If you don't set spi-tmax-nbits
>> or spi-rmax-nbits, +spi_device mode will be set in single(1 wire) as
>> default. Another point, if +property:spi-3wire is set, spi-t/rmax-nbits
>> is forbidden to set to <2 or 4>, +otherwise, an errro will return.
>
> typo: errro
>
> Also an error or "returning an error" is an OS specific term. This
> statement should rather be something along
>
>         Dual/Quad mode is not allowed when 3-wire mode is used.
>
Got it, Thanks a lot.

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/