[PATCH v3 1/2]spi: DUAL and QUAD support

[PATCH v3 1/2]spi: DUAL and QUAD support

[wangyuhang]

fix the previous patch some mistake below:
1. DT in slave node, use "spi-tx-nbits = <1/2/4>" in place of using
   "spi-tx-dual, spi-tx-quad" directly, same to rx. So correct the
   previous way to get the property in @of_register_spi_devices().
2. Change the value of transfer bit macro(SPI_NBITS_SINGLE, SPI_NBITS_DUAL
   SPI_NBITS_QUAD) to 0x01, 0x02 and 0x04 to match the actual wires.
3. Add the following check
   (1)keep the tx_nbits and rx_nbits in spi_transfer is not beyond the
      single, dual and quad.
   (2)keep tx_nbits and rx_nbits are contained by @spi_device->mode
      example: if @spi_device->mode = DUAL, then tx/rx_nbits can not be set
               to QUAD(SPI_NBITS_QUAD)
   (3)if "@spi_device->mode & SPI_3WIRE", then tx/rx_nbits should be in
      single(SPI_NBITS_SINGLE)

Signed-off-by: wangyuhang <wangyuhang2014@gmail.com>
---
 drivers/spi/spi.c       |   96 +++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/spi/spi.h |   22 ++++++++++-
 2 files changed, 116 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 004b10f..78f257a 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -869,6 +869,51 @@ static void of_register_spi_devices(struct spi_master *master)
 		if (of_find_property(nc, "spi-3wire", NULL))
 			spi->mode |= SPI_3WIRE;
 
+		/* Device DUAL/QUAD mode */
+		prop = of_get_property(nc, "spi-tx-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;
+		}
+		
+		prop = of_get_property(nc, "spi-rx-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;
+		}
+
 		/* Device speed */
 		prop = of_get_property(nc, "spi-max-frequency", &len);
 		if (!prop || len < sizeof(*prop)) {
@@ -1313,6 +1358,19 @@ int spi_setup(struct spi_device *spi)
 	unsigned	bad_bits;
 	int		status = 0;
 
+	/* check mode to prevent that DUAL and QUAD set at the same time
+	 */
+	if (((spi->mode & SPI_TX_DUAL) && (spi->mode & SPI_TX_QUAD)) ||
+		((spi->mode & SPI_RX_DUAL) && (spi->mode & SPI_RX_QUAD))) {
+		dev_err(&spi->dev,
+		"setup: can not select dual and quad at the same time\n");
+		return -EINVAL;
+	}
+	/* if it is SPI_3WIRE mode, DUAL and QUAD should be forbidden
+	 */
+	if ((spi->mode & SPI_3WIRE) && (spi->mode &
+		(SPI_TX_DUAL | SPI_TX_QUAD | SPI_RX_DUAL | SPI_RX_QUAD)))
+		return -EINVAL;
 	/* help drivers fail *cleanly* when they need options
 	 * that aren't supported with their current master
 	 */
@@ -1370,12 +1428,50 @@ static int __spi_async(struct spi_device *spi, struct spi_message *message)
 	/**
 	 * Set transfer bits_per_word and max speed as spi device default if
 	 * it is not set for this transfer.
+	 * Set transfer tx_nbits and rx_nbits as single transfer default
+	 * (SPI_NBITS_SINGLE) if it is not set for this transfer.
 	 */
 	list_for_each_entry(xfer, &message->transfers, transfer_list) {
 		if (!xfer->bits_per_word)
 			xfer->bits_per_word = spi->bits_per_word;
 		if (!xfer->speed_hz)
 			xfer->speed_hz = spi->max_speed_hz;
+		if (xfer->tx_buf && !xfer->tx_nbits)
+			xfer->tx_nbits = SPI_NBITS_SINGLE;
+		if (xfer->rx_buf && !xfer->rx_nbits)
+			xfer->rx_nbits = SPI_NBITS_SINGLE;
+		/* check transfer tx/rx_nbits:
+		 * 1. keep the value is not out of single, dual and quad
+		 * 2. keep tx/rx_nbits is contained by mode in spi_device
+		 * 3. if SPI_3WIRE, tx/rx_nbits should be in single
+		 */
+		if (xfer->tx_nbits != SPI_NBITS_SINGLE &&
+			xfer->tx_nbits != SPI_NBITS_DUAL &&
+			xfer->tx_nbits != SPI_NBITS_QUAD)
+			return -EINVAL;
+		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;
+		if ((spi->mode & SPI_3WIRE) &&
+			(xfer->tx_nbits != SPI_NBITS_SINGLE))
+			return -EINVAL;
+		/* check transfer rx_nbits */
+		if (xfer->rx_nbits != SPI_NBITS_SINGLE &&
+			xfer->rx_nbits != SPI_NBITS_DUAL &&
+			xfer->rx_nbits != SPI_NBITS_QUAD)
+			return -EINVAL;
+		if ((xfer->rx_nbits == SPI_NBITS_DUAL) &&
+			!(spi->mode & (SPI_RX_DUAL | SPI_RX_QUAD)))
+			return -EINVAL;
+		if ((xfer->rx_nbits == SPI_NBITS_QUAD) &&
+			!(spi->mode & SPI_RX_QUAD))
+			return -EINVAL;
+		if ((spi->mode & SPI_3WIRE) &&
+			(xfer->rx_nbits != SPI_NBITS_SINGLE))
+			return -EINVAL;
 	}
 
 	message->spi = spi;
diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
index 38c2b92..3dc0a86 100644
--- a/include/linux/spi/spi.h
+++ b/include/linux/spi/spi.h
@@ -74,7 +74,7 @@ struct spi_device {
 	struct spi_master	*master;
 	u32			max_speed_hz;
 	u8			chip_select;
-	u8			mode;
+	u16			mode;
 #define	SPI_CPHA	0x01			/* clock phase */
 #define	SPI_CPOL	0x02			/* clock polarity */
 #define	SPI_MODE_0	(0|0)			/* (original MicroWire) */
@@ -87,6 +87,10 @@ struct spi_device {
 #define	SPI_LOOP	0x20			/* loopback mode */
 #define	SPI_NO_CS	0x40			/* 1 dev/bus, no chipselect */
 #define	SPI_READY	0x80			/* slave pulls low to pause */
+#define	SPI_TX_DUAL	0x100			/* transmit with 2 wires */
+#define	SPI_TX_QUAD	0x200			/* transmit with 4 wires */
+#define	SPI_RX_DUAL	0x400			/* receive with 2 wires */
+#define	SPI_RX_QUAD	0x800			/* receive with 4 wires */
 	u8			bits_per_word;
 	int			irq;
 	void			*controller_state;
@@ -437,6 +441,10 @@ extern struct spi_master *spi_busnum_to_master(u16 busnum);
  * @rx_buf: data to be read (dma-safe memory), or NULL
  * @tx_dma: DMA address of tx_buf, if @spi_message.is_dma_mapped
  * @rx_dma: DMA address of rx_buf, if @spi_message.is_dma_mapped
+ * @tx_nbits: number of bits used for writting. If 0 the default
+ *      (SPI_NBITS_SINGLE) is used.
+ * @rx_nbits: number of bits used for reading. If 0 the default
+ *      (SPI_NBITS_SINGLE) is used.
  * @len: size of rx and tx buffers (in bytes)
  * @speed_hz: Select a speed other than the device default for this
  *      transfer. If 0 the default (from @spi_device) is used.
@@ -491,6 +499,11 @@ extern struct spi_master *spi_busnum_to_master(u16 busnum);
  * by the results of previous messages and where the whole transaction
  * ends when the chipselect goes intactive.
  *
+ * When SPI can transfer in 1x,2x or 4x. It can get this tranfer information
+ * from device through @tx_nbits and @rx_nbits. In Bi-direction, these
+ * two should both be set. User can set transfer mode with SPI_NBITS_SINGLE(1x)
+ * SPI_NBITS_DUAL(2x) and SPI_NBITS_QUAD(4x) to support these three transfer.
+ *
  * The code that submits an spi_message (and its spi_transfers)
  * to the lower layers is responsible for managing its memory.
  * Zero-initialize every field you don't set up explicitly, to
@@ -511,6 +524,11 @@ struct spi_transfer {
 	dma_addr_t	rx_dma;
 
 	unsigned	cs_change:1;
+	u8		tx_nbits;
+	u8		rx_nbits;
+#define	SPI_NBITS_SINGLE	0x01 /* 1bit transfer */
+#define	SPI_NBITS_DUAL		0x02 /* 2bits transfer */
+#define	SPI_NBITS_QUAD		0x04 /* 4bits transfer */
 	u8		bits_per_word;
 	u16		delay_usecs;
 	u32		speed_hz;
@@ -858,7 +876,7 @@ struct spi_board_info {
 	/* mode becomes spi_device.mode, and is essential for chips
 	 * where the default of SPI_CS_HIGH = 0 is wrong.
 	 */
-	u8		mode;
+	u16		mode;
 
 	/* ... may need additional spi_device chip config data here.
 	 * avoid stuff protocol drivers can set; but include stuff
-- 
1.7.9.5

Re: [PATCH v3 1/2]spi: DUAL and QUAD support

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 614 bytes --]

On Sun, Aug 11, 2013 at 06:15:17PM +0800, wangyuhang wrote:

Applied, this looks good thanks.  A few coding style issues which I'll
fix up myself but one thing:

> fix the previous patch some mistake below:
> 1. DT in slave node, use "spi-tx-nbits = <1/2/4>" in place of using
>    "spi-tx-dual, spi-tx-quad" directly, same to rx. So correct the
>    previous way to get the property in @of_register_spi_devices().

This new DT stuff needs to be reviewed on the devicetree list and
documented in the generic SPI bindings.  Can you please send a patch
doing that?

Please also note that the SPI list moved to vger.

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

Re: [PATCH v3 1/2]spi: DUAL and QUAD support

[Sourav Poddar]

Hi Mark, Wang,
On Sunday 11 August 2013 03:45 PM, wangyuhang wrote:
> fix the previous patch some mistake below:
> 1. DT in slave node, use "spi-tx-nbits =<1/2/4>" in place of using
>     "spi-tx-dual, spi-tx-quad" directly, same to rx. So correct the
>     previous way to get the property in @of_register_spi_devices().
> 2. Change the value of transfer bit macro(SPI_NBITS_SINGLE, SPI_NBITS_DUAL
>     SPI_NBITS_QUAD) to 0x01, 0x02 and 0x04 to match the actual wires.
> 3. Add the following check
>     (1)keep the tx_nbits and rx_nbits in spi_transfer is not beyond the
>        single, dual and quad.
>     (2)keep tx_nbits and rx_nbits are contained by @spi_device->mode
>        example: if @spi_device->mode = DUAL, then tx/rx_nbits can not be set
>                 to QUAD(SPI_NBITS_QUAD)
>     (3)if "@spi_device->mode&  SPI_3WIRE", then tx/rx_nbits should be in
>        single(SPI_NBITS_SINGLE)
>
> Signed-off-by: wangyuhang<wangyuhang2014@gmail.com>
> ---
>   drivers/spi/spi.c       |   96 +++++++++++++++++++++++++++++++++++++++++++++++
>   include/linux/spi/spi.h |   22 ++++++++++-
>   2 files changed, 116 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> index 004b10f..78f257a 100644
> --- a/drivers/spi/spi.c
> +++ b/drivers/spi/spi.c
> @@ -869,6 +869,51 @@ static void of_register_spi_devices(struct spi_master *master)
>   		if (of_find_property(nc, "spi-3wire", NULL))
>   			spi->mode |= SPI_3WIRE;
>
> +		/* Device DUAL/QUAD mode */
> +		prop = of_get_property(nc, "spi-tx-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;
> +		}
> +		
> +		prop = of_get_property(nc, "spi-rx-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;
> +		}
> +
>   		/* Device speed */
>   		prop = of_get_property(nc, "spi-max-frequency",&len);
>   		if (!prop || len<  sizeof(*prop)) {
> @@ -1313,6 +1358,19 @@ int spi_setup(struct spi_device *spi)
>   	unsigned	bad_bits;
>   	int		status = 0;
>
> +	/* check mode to prevent that DUAL and QUAD set at the same time
> +	 */
> +	if (((spi->mode&  SPI_TX_DUAL)&&  (spi->mode&  SPI_TX_QUAD)) ||
> +		((spi->mode&  SPI_RX_DUAL)&&  (spi->mode&  SPI_RX_QUAD))) {
> +		dev_err(&spi->dev,
> +		"setup: can not select dual and quad at the same time\n");
> +		return -EINVAL;
> +	}
> +	/* if it is SPI_3WIRE mode, DUAL and QUAD should be forbidden
> +	 */
> +	if ((spi->mode&  SPI_3WIRE)&&  (spi->mode&
> +		(SPI_TX_DUAL | SPI_TX_QUAD | SPI_RX_DUAL | SPI_RX_QUAD)))
> +		return -EINVAL;
>   	/* help drivers fail *cleanly* when they need options
>   	 * that aren't supported with their current master
>   	 */
> @@ -1370,12 +1428,50 @@ static int __spi_async(struct spi_device *spi, struct spi_message *message)
>   	/**
>   	 * Set transfer bits_per_word and max speed as spi device default if
>   	 * it is not set for this transfer.
> +	 * Set transfer tx_nbits and rx_nbits as single transfer default
> +	 * (SPI_NBITS_SINGLE) if it is not set for this transfer.
>   	 */
>   	list_for_each_entry(xfer,&message->transfers, transfer_list) {
>   		if (!xfer->bits_per_word)
>   			xfer->bits_per_word = spi->bits_per_word;
>   		if (!xfer->speed_hz)
>   			xfer->speed_hz = spi->max_speed_hz;
> +		if (xfer->tx_buf&&  !xfer->tx_nbits)
> +			xfer->tx_nbits = SPI_NBITS_SINGLE;
> +		if (xfer->rx_buf&&  !xfer->rx_nbits)
> +			xfer->rx_nbits = SPI_NBITS_SINGLE;
> +		/* check transfer tx/rx_nbits:
> +		 * 1. keep the value is not out of single, dual and quad
> +		 * 2. keep tx/rx_nbits is contained by mode in spi_device
> +		 * 3. if SPI_3WIRE, tx/rx_nbits should be in single
> +		 */
Sorry for pitchin in a bit late on this.
I was trying to use this for my use case today. I realise this check 
should be under...

> +		if (xfer->tx_nbits != SPI_NBITS_SINGLE&&
> +			xfer->tx_nbits != SPI_NBITS_DUAL&&
> +			xfer->tx_nbits != SPI_NBITS_QUAD)
> +			return -EINVAL;
> +		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;
> +		if ((spi->mode&  SPI_3WIRE)&&
> +			(xfer->tx_nbits != SPI_NBITS_SINGLE))
> +			return -EINVAL;
if (xfer->tx_buf) {
      ......

} and
> +		/* check transfer rx_nbits */
> +		if (xfer->rx_nbits != SPI_NBITS_SINGLE&&
> +			xfer->rx_nbits != SPI_NBITS_DUAL&&
> +			xfer->rx_nbits != SPI_NBITS_QUAD)
> +			return -EINVAL;
> +		if ((xfer->rx_nbits == SPI_NBITS_DUAL)&&
> +			!(spi->mode&  (SPI_RX_DUAL | SPI_RX_QUAD)))
> +			return -EINVAL;
> +		if ((xfer->rx_nbits == SPI_NBITS_QUAD)&&
> +			!(spi->mode&  SPI_RX_QUAD))
> +			return -EINVAL;
> +		if ((spi->mode&  SPI_3WIRE)&&
> +			(xfer->rx_nbits != SPI_NBITS_SINGLE))
> +			return -EINVAL;
this should be under
  if (xfer->rx_buf) {
.....
}
>   	}
>
>   	message->spi = spi;
> diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
> index 38c2b92..3dc0a86 100644
> --- a/include/linux/spi/spi.h
> +++ b/include/linux/spi/spi.h
> @@ -74,7 +74,7 @@ struct spi_device {
>   	struct spi_master	*master;
>   	u32			max_speed_hz;
>   	u8			chip_select;
> -	u8			mode;
> +	u16			mode;
>   #define	SPI_CPHA	0x01			/* clock phase */
>   #define	SPI_CPOL	0x02			/* clock polarity */
>   #define	SPI_MODE_0	(0|0)			/* (original MicroWire) */
> @@ -87,6 +87,10 @@ struct spi_device {
>   #define	SPI_LOOP	0x20			/* loopback mode */
>   #define	SPI_NO_CS	0x40			/* 1 dev/bus, no chipselect */
>   #define	SPI_READY	0x80			/* slave pulls low to pause */
> +#define	SPI_TX_DUAL	0x100			/* transmit with 2 wires */
> +#define	SPI_TX_QUAD	0x200			/* transmit with 4 wires */
> +#define	SPI_RX_DUAL	0x400			/* receive with 2 wires */
> +#define	SPI_RX_QUAD	0x800			/* receive with 4 wires */
>   	u8			bits_per_word;
>   	int			irq;
>   	void			*controller_state;
> @@ -437,6 +441,10 @@ extern struct spi_master *spi_busnum_to_master(u16 busnum);
>    * @rx_buf: data to be read (dma-safe memory), or NULL
>    * @tx_dma: DMA address of tx_buf, if @spi_message.is_dma_mapped
>    * @rx_dma: DMA address of rx_buf, if @spi_message.is_dma_mapped
> + * @tx_nbits: number of bits used for writting. If 0 the default
> + *      (SPI_NBITS_SINGLE) is used.
> + * @rx_nbits: number of bits used for reading. If 0 the default
> + *      (SPI_NBITS_SINGLE) is used.
>    * @len: size of rx and tx buffers (in bytes)
>    * @speed_hz: Select a speed other than the device default for this
>    *      transfer. If 0 the default (from @spi_device) is used.
> @@ -491,6 +499,11 @@ extern struct spi_master *spi_busnum_to_master(u16 busnum);
>    * by the results of previous messages and where the whole transaction
>    * ends when the chipselect goes intactive.
>    *
> + * When SPI can transfer in 1x,2x or 4x. It can get this tranfer information
> + * from device through @tx_nbits and @rx_nbits. In Bi-direction, these
> + * two should both be set. User can set transfer mode with SPI_NBITS_SINGLE(1x)
> + * SPI_NBITS_DUAL(2x) and SPI_NBITS_QUAD(4x) to support these three transfer.
> + *
>    * The code that submits an spi_message (and its spi_transfers)
>    * to the lower layers is responsible for managing its memory.
>    * Zero-initialize every field you don't set up explicitly, to
> @@ -511,6 +524,11 @@ struct spi_transfer {
>   	dma_addr_t	rx_dma;
>
>   	unsigned	cs_change:1;
> +	u8		tx_nbits;
> +	u8		rx_nbits;
> +#define	SPI_NBITS_SINGLE	0x01 /* 1bit transfer */
> +#define	SPI_NBITS_DUAL		0x02 /* 2bits transfer */
> +#define	SPI_NBITS_QUAD		0x04 /* 4bits transfer */
>   	u8		bits_per_word;
>   	u16		delay_usecs;
>   	u32		speed_hz;
> @@ -858,7 +876,7 @@ struct spi_board_info {
>   	/* mode becomes spi_device.mode, and is essential for chips
>   	 * where the default of SPI_CS_HIGH = 0 is wrong.
>   	 */
> -	u8		mode;
> +	u16		mode;
>
>   	/* ... may need additional spi_device chip config data here.
>   	 * avoid stuff protocol drivers can set; but include stuff

Re: [PATCH v3 1/2]spi: DUAL and QUAD support

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 360 bytes --]

On Sun, Aug 11, 2013 at 06:15:17PM +0800, wangyuhang wrote:
> fix the previous patch some mistake below:

One other thing: this wasn't sent against current code, it seems to have
been generated against an old kernel version.  Please make sure when you
submit that you submit against either Linus' tree or ideally the latest
development code for the subsystem.

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

Re: [PATCH v3 1/2]spi: DUAL and QUAD support

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 526 bytes --]

On Thu, Aug 22, 2013 at 06:05:11PM +0530, Sourav Poddar wrote:
> On Sunday 11 August 2013 03:45 PM, wangyuhang wrote:

> >+		/* check transfer tx/rx_nbits:
> >+		 * 1. keep the value is not out of single, dual and quad
> >+		 * 2. keep tx/rx_nbits is contained by mode in spi_device
> >+		 * 3. if SPI_3WIRE, tx/rx_nbits should be in single
> >+		 */

> Sorry for pitchin in a bit late on this.
> I was trying to use this for my use case today. I realise this check
> should be under...

Can you send a patch for this please?

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

Re: [PATCH v3 1/2]spi: DUAL and QUAD support

[Sourav Poddar]

On Thursday 22 August 2013 06:51 PM, Mark Brown wrote:
> On Thu, Aug 22, 2013 at 06:05:11PM +0530, Sourav Poddar wrote:
>> On Sunday 11 August 2013 03:45 PM, wangyuhang wrote:
>>> +		/* check transfer tx/rx_nbits:
>>> +		 * 1. keep the value is not out of single, dual and quad
>>> +		 * 2. keep tx/rx_nbits is contained by mode in spi_device
>>> +		 * 3. if SPI_3WIRE, tx/rx_nbits should be in single
>>> +		 */
>> Sorry for pitchin in a bit late on this.
>> I was trying to use this for my use case today. I realise this check
>> should be under...
> Can you send a patch for this please?
Yes, I will send it.

[PATCH] spi: quad: fix the name of DT property

[wangyuhang]

spi: quad: fix the name of DT property in patch

The previous property name spi-tx-nbits and spi-rx-nbits looks not
human-readable. To make it consistent with other devices, using property
name spi-tx-bus-width and spi-rx-bus-width instead of the previous one
specify the number of data wires that spi controller will work in.
Add the specification in spi-bus.txt.

Signed-off-by: wangyuhang <wangyuhang2014@gmail.com>
---
 Documentation/devicetree/bindings/spi/spi-bus.txt |   10 ++++++++++
 drivers/spi/spi.c                                 |    8 ++++----
 2 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/spi/spi-bus.txt b/Documentation/devicetree/bindings/spi/spi-bus.txt
index 296015e..800dafe 100644
--- a/Documentation/devicetree/bindings/spi/spi-bus.txt
+++ b/Documentation/devicetree/bindings/spi/spi-bus.txt
@@ -55,6 +55,16 @@ contain the following properties.
     		chip select active high
 - spi-3wire       - (optional) Empty property indicating device requires
     		    3-wire mode.
+- spi-tx-bus-width - (optional) The bus width(number of data wires) that
+                      used for MOSI. Defaults to 1 if not present.
+- spi-rx-bus-width - (optional) The bus width(number of data wires) that
+                      used for MISO. Defaults to 1 if not present.
+
+Some SPI controllers and devices support Dual and Quad SPI transfer mode.
+It allows data in SPI system transfered in 2 wires(DUAL) or 4 wires(QUAD).
+Now the value that spi-tx-bus-width and spi-rx-bus-width can receive is
+only 1(SINGLE), 2(DUAL) and 4(QUAD).
+Dual/Quad mode is not allowed when 3-wire mode is used.
 
 If a gpio chipselect is used for the SPI slave the gpio number will be passed
 via the cs_gpio
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 7557f61..0075318 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -870,7 +870,7 @@ 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-tx-bus-width", &len);
 		if (prop && len == sizeof(*prop)) {
 			switch (be32_to_cpup(prop)) {
 			case SPI_NBITS_SINGLE:
@@ -883,14 +883,14 @@ static void of_register_spi_devices(struct spi_master *master)
 				break;
 			default:
 				dev_err(&master->dev,
-					"spi-tx-nbits %d not supported\n",
+					"spi-tx-bus-width %d not supported\n",
 					be32_to_cpup(prop));
 				spi_dev_put(spi);
 				continue;
 			}
 		}
 
-		prop = of_get_property(nc, "spi-rx-nbits", &len);
+		prop = of_get_property(nc, "spi-rx-bus-width", &len);
 		if (prop && len == sizeof(*prop)) {
 			switch (be32_to_cpup(prop)) {
 			case SPI_NBITS_SINGLE:
@@ -903,7 +903,7 @@ static void of_register_spi_devices(struct spi_master *master)
 				break;
 			default:
 				dev_err(&master->dev,
-					"spi-rx-nbits %d not supported\n",
+					"spi-rx-bus-width %d not supported\n",
 					be32_to_cpup(prop));
 				spi_dev_put(spi);
 				continue;
-- 
1.7.9.5

Re: [PATCH] spi: quad: fix the name of DT property

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 648 bytes --]

On Sun, Sep 01, 2013 at 05:36:21PM +0800, wangyuhang wrote:
> spi: quad: fix the name of DT property in patch
> 
> The previous property name spi-tx-nbits and spi-rx-nbits looks not
> human-readable. To make it consistent with other devices, using property
> name spi-tx-bus-width and spi-rx-bus-width instead of the previous one
> specify the number of data wires that spi controller will work in.
> Add the specification in spi-bus.txt.

Applied, thanks - if there's disagreement about the DT bindings (or
indeed other problems) then I can not send this in the final pull
request but if it is OK I wanted it to be in -next on Monday.

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

Re: [PATCH] spi: quad: fix the name of DT property

[Stephen Warren]

On 09/01/2013 03:36 AM, wangyuhang wrote:
> spi: quad: fix the name of DT property in patch
> 
> The previous property name spi-tx-nbits and spi-rx-nbits looks not
> human-readable. To make it consistent with other devices, using property
> name spi-tx-bus-width and spi-rx-bus-width instead of the previous one
> specify the number of data wires that spi controller will work in.
> Add the specification in spi-bus.txt.

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

>  - spi-3wire       - (optional) Empty property indicating device requires
>      		    3-wire mode.
> +- spi-tx-bus-width - (optional) The bus width(number of data wires) that
> +                      used for MOSI. Defaults to 1 if not present.
> +- spi-rx-bus-width - (optional) The bus width(number of data wires) that
> +                      used for MISO. Defaults to 1 if not present.

The binding,
Acked-by: Stephen Warren <swarren@nvidia.com>

I would have preferred my original wording rather than the unqualified
"MOSI"/"MISO", since the meaning of those terms depends on whether
you're looking at the host controller or the device, but I guess we can
assume that since this is documentation for the host controller binding,
the naming is in terms of the host controller's signals.

Re: [PATCH] spi: quad: fix the name of DT property

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 449 bytes --]

On Tue, Sep 03, 2013 at 10:53:11AM -0600, Stephen Warren wrote:

> I would have preferred my original wording rather than the unqualified
> "MOSI"/"MISO", since the meaning of those terms depends on whether
> you're looking at the host controller or the device, but I guess we can

No, MOSI and MISO have fixed meanings at all ends - they expand to
Master Out Slave In and Master In Slave Out so it's always clear which
side should be transmitting.

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

Re: [PATCH v3 1/2]spi: DUAL and QUAD support

[Trent Piepho]

On Sun, Aug 11, 2013 at 3:15 AM, wangyuhang <wangyuhang2014@gmail.com> wrote:
> fix the previous patch some mistake below:

This isn't a very good commit message.  "the previous patch" will have
no meaning in the kernel git repo.  The rest of the message only
describes changes since a previous version of the patch and not the
actual patch in full.

>
> +               /* Device DUAL/QUAD mode */
> +               prop = of_get_property(nc, "spi-tx-nbits", &len);

Why not use of_property_read_u32() here?

> +               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;

So if no spi-tx-nbits property is supplied, the device is rejected and
the loop continues to the next device entry?  This means ALL EXISTING
DEVICE TREES with SPI devices will be rejected, since none of them
have this new property!  Was this patch tested at all with any system
before being accepted?

> +       /* check mode to prevent that DUAL and QUAD set at the same time
> +        */
> +       if (((spi->mode & SPI_TX_DUAL) && (spi->mode & SPI_TX_QUAD)) ||
> +               ((spi->mode & SPI_RX_DUAL) && (spi->mode & SPI_RX_QUAD))) {
> +               dev_err(&spi->dev,
> +               "setup: can not select dual and quad at the same time\n");
> +               return -EINVAL;

This test can be done with fewer operations as:
if ((spi->mode & (SPI_TX_DUAL|SPI_TX_QUAD)) == (SPI_TX_DUAL|SPI_TX_QUAD) ||
    (spi->mode & (SPI_RX_DUAL|SPI_RX_QUAD)) == (SPI_RX_DUAL|SPI_RX_QUAD)) {


> +       }
> +       /* if it is SPI_3WIRE mode, DUAL and QUAD should be forbidden
> +        */
> +       if ((spi->mode & SPI_3WIRE) && (spi->mode &
> +               (SPI_TX_DUAL | SPI_TX_QUAD | SPI_RX_DUAL | SPI_RX_QUAD)))
> +               return -EINVAL;

No dev_err message for this possibility?  What's different than the
previous check that does produce a message?

>         /* help drivers fail *cleanly* when they need options
>          * that aren't supported with their current master
>          */
Following this comment is the code:
        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;
        }

Won't this always trigger for anything that sets one of the dual or quad bits?

> @@ -1370,12 +1428,50 @@ static int __spi_async(struct spi_device *spi, struct spi_message *message)
>         /**
>          * Set transfer bits_per_word and max speed as spi device default if
>          * it is not set for this transfer.
> +        * Set transfer tx_nbits and rx_nbits as single transfer default
> +        * (SPI_NBITS_SINGLE) if it is not set for this transfer.
>          */
>         list_for_each_entry(xfer, &message->transfers, transfer_list) {
>                 if (!xfer->bits_per_word)
>                         xfer->bits_per_word = spi->bits_per_word;
>                 if (!xfer->speed_hz)
>                         xfer->speed_hz = spi->max_speed_hz;
> +               if (xfer->tx_buf && !xfer->tx_nbits)
> +                       xfer->tx_nbits = SPI_NBITS_SINGLE;
> +               if (xfer->rx_buf && !xfer->rx_nbits)
> +                       xfer->rx_nbits = SPI_NBITS_SINGLE;
> +               /* check transfer tx/rx_nbits:
> +                * 1. keep the value is not out of single, dual and quad
> +                * 2. keep tx/rx_nbits is contained by mode in spi_device
> +                * 3. if SPI_3WIRE, tx/rx_nbits should be in single
> +                */
> +               if (xfer->tx_nbits != SPI_NBITS_SINGLE &&
> +                       xfer->tx_nbits != SPI_NBITS_DUAL &&
> +                       xfer->tx_nbits != SPI_NBITS_QUAD)
> +                       return -EINVAL;
> +               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;
> +               if ((spi->mode & SPI_3WIRE) &&
> +                       (xfer->tx_nbits != SPI_NBITS_SINGLE))
> +                       return -EINVAL;
> +               /* check transfer rx_nbits */
> +               if (xfer->rx_nbits != SPI_NBITS_SINGLE &&
> +                       xfer->rx_nbits != SPI_NBITS_DUAL &&
> +                       xfer->rx_nbits != SPI_NBITS_QUAD)
> +                       return -EINVAL;
> +               if ((xfer->rx_nbits == SPI_NBITS_DUAL) &&
> +                       !(spi->mode & (SPI_RX_DUAL | SPI_RX_QUAD)))
> +                       return -EINVAL;
> +               if ((xfer->rx_nbits == SPI_NBITS_QUAD) &&
> +                       !(spi->mode & SPI_RX_QUAD))
> +                       return -EINVAL;
> +               if ((spi->mode & SPI_3WIRE) &&
> +                       (xfer->rx_nbits != SPI_NBITS_SINGLE))
> +                       return -EINVAL;
>         }

What a lot of code to check the transfer bits.  It really needs this much?

> @@ -511,6 +524,11 @@ struct spi_transfer {
>         dma_addr_t      rx_dma;
>
>         unsigned        cs_change:1;
> +       u8              tx_nbits;
> +       u8              rx_nbits;
> +#define        SPI_NBITS_SINGLE        0x01 /* 1bit transfer */
> +#define        SPI_NBITS_DUAL          0x02 /* 2bits transfer */
> +#define        SPI_NBITS_QUAD          0x04 /* 4bits transfer */

These fields increase the size of a spi_transfer by 4 bytes.  If you
used bitfields instead it wouldn't increase the size at all since
there are still 7 bits left after cs_change.

Re: [PATCH v3 1/2]spi: DUAL and QUAD support

[yuhang wang]

2013/9/4 Trent Piepho <tpiepho@gmail.com>:
> On Sun, Aug 11, 2013 at 3:15 AM, wangyuhang <wangyuhang2014@gmail.com> wrote:
>> fix the previous patch some mistake below:
>
> This isn't a very good commit message.  "the previous patch" will have
> no meaning in the kernel git repo.  The rest of the message only
> describes changes since a previous version of the patch and not the
> actual patch in full.
>
>>
>> +               /* Device DUAL/QUAD mode */
>> +               prop = of_get_property(nc, "spi-tx-nbits", &len);
>
> Why not use of_property_read_u32() here?
>
>> +               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;
>
> So if no spi-tx-nbits property is supplied, the device is rejected and
> the loop continues to the next device entry?  This means ALL EXISTING
> DEVICE TREES with SPI devices will be rejected, since none of them
> have this new property!  Was this patch tested at all with any system
> before being accepted?
>
This part has been fixed in patch[commit
id:a822e99c70f448c4068ea85bb195dac0b2eb3afe]

>> +       /* check mode to prevent that DUAL and QUAD set at the same time
>> +        */
>> +       if (((spi->mode & SPI_TX_DUAL) && (spi->mode & SPI_TX_QUAD)) ||
>> +               ((spi->mode & SPI_RX_DUAL) && (spi->mode & SPI_RX_QUAD))) {
>> +               dev_err(&spi->dev,
>> +               "setup: can not select dual and quad at the same time\n");
>> +               return -EINVAL;
>
> This test can be done with fewer operations as:
> if ((spi->mode & (SPI_TX_DUAL|SPI_TX_QUAD)) == (SPI_TX_DUAL|SPI_TX_QUAD) ||
>     (spi->mode & (SPI_RX_DUAL|SPI_RX_QUAD)) == (SPI_RX_DUAL|SPI_RX_QUAD)) {
>
>
>> +       }
>> +       /* if it is SPI_3WIRE mode, DUAL and QUAD should be forbidden
>> +        */
>> +       if ((spi->mode & SPI_3WIRE) && (spi->mode &
>> +               (SPI_TX_DUAL | SPI_TX_QUAD | SPI_RX_DUAL | SPI_RX_QUAD)))
>> +               return -EINVAL;
>
> No dev_err message for this possibility?  What's different than the
> previous check that does produce a message?
>
OK, should be added. Thanks.

>>         /* help drivers fail *cleanly* when they need options
>>          * that aren't supported with their current master
>>          */
> Following this comment is the code:
>         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;
>         }
>
> Won't this always trigger for anything that sets one of the dual or quad bits?
>
I can't get your idea clearly. Please provide more infomation.

>> @@ -1370,12 +1428,50 @@ static int __spi_async(struct spi_device *spi, struct spi_message *message)
>>         /**
>>          * Set transfer bits_per_word and max speed as spi device default if
>>          * it is not set for this transfer.
>> +        * Set transfer tx_nbits and rx_nbits as single transfer default
>> +        * (SPI_NBITS_SINGLE) if it is not set for this transfer.
>>          */
>>         list_for_each_entry(xfer, &message->transfers, transfer_list) {
>>                 if (!xfer->bits_per_word)
>>                         xfer->bits_per_word = spi->bits_per_word;
>>                 if (!xfer->speed_hz)
>>                         xfer->speed_hz = spi->max_speed_hz;
>> +               if (xfer->tx_buf && !xfer->tx_nbits)
>> +                       xfer->tx_nbits = SPI_NBITS_SINGLE;
>> +               if (xfer->rx_buf && !xfer->rx_nbits)
>> +                       xfer->rx_nbits = SPI_NBITS_SINGLE;
>> +               /* check transfer tx/rx_nbits:
>> +                * 1. keep the value is not out of single, dual and quad
>> +                * 2. keep tx/rx_nbits is contained by mode in spi_device
>> +                * 3. if SPI_3WIRE, tx/rx_nbits should be in single
>> +                */
>> +               if (xfer->tx_nbits != SPI_NBITS_SINGLE &&
>> +                       xfer->tx_nbits != SPI_NBITS_DUAL &&
>> +                       xfer->tx_nbits != SPI_NBITS_QUAD)
>> +                       return -EINVAL;
>> +               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;
>> +               if ((spi->mode & SPI_3WIRE) &&
>> +                       (xfer->tx_nbits != SPI_NBITS_SINGLE))
>> +                       return -EINVAL;
>> +               /* check transfer rx_nbits */
>> +               if (xfer->rx_nbits != SPI_NBITS_SINGLE &&
>> +                       xfer->rx_nbits != SPI_NBITS_DUAL &&
>> +                       xfer->rx_nbits != SPI_NBITS_QUAD)
>> +                       return -EINVAL;
>> +               if ((xfer->rx_nbits == SPI_NBITS_DUAL) &&
>> +                       !(spi->mode & (SPI_RX_DUAL | SPI_RX_QUAD)))
>> +                       return -EINVAL;
>> +               if ((xfer->rx_nbits == SPI_NBITS_QUAD) &&
>> +                       !(spi->mode & SPI_RX_QUAD))
>> +                       return -EINVAL;
>> +               if ((spi->mode & SPI_3WIRE) &&
>> +                       (xfer->rx_nbits != SPI_NBITS_SINGLE))
>> +                       return -EINVAL;
>>         }
>
> What a lot of code to check the transfer bits.  It really needs this much?
>
This part is also corrected in patch [commit
id:db90a44177ac39fc22b2da5235b231fccdd4c673].

>> @@ -511,6 +524,11 @@ struct spi_transfer {
>>         dma_addr_t      rx_dma;
>>
>>         unsigned        cs_change:1;
>> +       u8              tx_nbits;
>> +       u8              rx_nbits;
>> +#define        SPI_NBITS_SINGLE        0x01 /* 1bit transfer */
>> +#define        SPI_NBITS_DUAL          0x02 /* 2bits transfer */
>> +#define        SPI_NBITS_QUAD          0x04 /* 4bits transfer */
>
> These fields increase the size of a spi_transfer by 4 bytes.  If you
> used bitfields instead it wouldn't increase the size at all since
> there are still 7 bits left after cs_change.
Yes, it will increase the size by 4 bytes, but using bitfields here
will make it logically weird. Bitfields may mean that really have the
restriction of number of bits. If just for saving memory, then what
about the other members such as bits_per_world, delay_usecs.

Re: [PATCH v3 1/2]spi: DUAL and QUAD support

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 431 bytes --]

On Tue, Sep 03, 2013 at 08:24:08PM -0700, Trent Piepho wrote:

> So if no spi-tx-nbits property is supplied, the device is rejected and
> the loop continues to the next device entry?  This means ALL EXISTING
> DEVICE TREES with SPI devices will be rejected, since none of them
> have this new property!  Was this patch tested at all with any system
> before being accepted?

This particular issue is already fixed properly anyway.

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

Re: [PATCH v3 1/2]spi: DUAL and QUAD support

[Trent Piepho]

On Wed, Sep 4, 2013 at 12:07 AM, yuhang wang <wangyuhang2014@gmail.com> wrote:
>>>         dma_addr_t      rx_dma;
>>>
>>>         unsigned        cs_change:1;
>>> +       u8              tx_nbits;
>>> +       u8              rx_nbits;
>>> +#define        SPI_NBITS_SINGLE        0x01 /* 1bit transfer */
>>> +#define        SPI_NBITS_DUAL          0x02 /* 2bits transfer */
>>> +#define        SPI_NBITS_QUAD          0x04 /* 4bits transfer */
>>
>> These fields increase the size of a spi_transfer by 4 bytes.  If you
>> used bitfields instead it wouldn't increase the size at all since
>> there are still 7 bits left after cs_change.
> Yes, it will increase the size by 4 bytes, but using bitfields here
> will make it logically weird. Bitfields may mean that really have the
> restriction of number of bits. If just for saving memory, then what
> about the other members such as bits_per_world, delay_usecs.

Rather than just talk about it, I'll make patches to do it.  Then it
will be clear.

On Wed, Sep 4, 2013 at 12:07 AM, yuhang wang <wangyuhang2014@gmail.com> wrote:
> 2013/9/4 Trent Piepho <tpiepho@gmail.com>:
>> On Sun, Aug 11, 2013 at 3:15 AM, wangyuhang <wangyuhang2014@gmail.com> wrote:
>>> fix the previous patch some mistake below:
>>
>> This isn't a very good commit message.  "the previous patch" will have
>> no meaning in the kernel git repo.  The rest of the message only
>> describes changes since a previous version of the patch and not the
>> actual patch in full.
>>
>>>
>>> +               /* Device DUAL/QUAD mode */
>>> +               prop = of_get_property(nc, "spi-tx-nbits", &len);
>>
>> Why not use of_property_read_u32() here?
>>
>>> +               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;
>>
>> So if no spi-tx-nbits property is supplied, the device is rejected and
>> the loop continues to the next device entry?  This means ALL EXISTING
>> DEVICE TREES with SPI devices will be rejected, since none of them
>> have this new property!  Was this patch tested at all with any system
>> before being accepted?
>>
> This part has been fixed in patch[commit
> id:a822e99c70f448c4068ea85bb195dac0b2eb3afe]
>
>>> +       /* check mode to prevent that DUAL and QUAD set at the same time
>>> +        */
>>> +       if (((spi->mode & SPI_TX_DUAL) && (spi->mode & SPI_TX_QUAD)) ||
>>> +               ((spi->mode & SPI_RX_DUAL) && (spi->mode & SPI_RX_QUAD))) {
>>> +               dev_err(&spi->dev,
>>> +               "setup: can not select dual and quad at the same time\n");
>>> +               return -EINVAL;
>>
>> This test can be done with fewer operations as:
>> if ((spi->mode & (SPI_TX_DUAL|SPI_TX_QUAD)) == (SPI_TX_DUAL|SPI_TX_QUAD) ||
>>     (spi->mode & (SPI_RX_DUAL|SPI_RX_QUAD)) == (SPI_RX_DUAL|SPI_RX_QUAD)) {
>>
>>
>>> +       }
>>> +       /* if it is SPI_3WIRE mode, DUAL and QUAD should be forbidden
>>> +        */
>>> +       if ((spi->mode & SPI_3WIRE) && (spi->mode &
>>> +               (SPI_TX_DUAL | SPI_TX_QUAD | SPI_RX_DUAL | SPI_RX_QUAD)))
>>> +               return -EINVAL;
>>
>> No dev_err message for this possibility?  What's different than the
>> previous check that does produce a message?
>>
> OK, should be added. Thanks.
>
>>>         /* help drivers fail *cleanly* when they need options
>>>          * that aren't supported with their current master
>>>          */
>> Following this comment is the code:
>>         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;
>>         }
>>
>> Won't this always trigger for anything that sets one of the dual or quad bits?
>>
> I can't get your idea clearly. Please provide more infomation.
>
>>> @@ -1370,12 +1428,50 @@ static int __spi_async(struct spi_device *spi, struct spi_message *message)
>>>         /**
>>>          * Set transfer bits_per_word and max speed as spi device default if
>>>          * it is not set for this transfer.
>>> +        * Set transfer tx_nbits and rx_nbits as single transfer default
>>> +        * (SPI_NBITS_SINGLE) if it is not set for this transfer.
>>>          */
>>>         list_for_each_entry(xfer, &message->transfers, transfer_list) {
>>>                 if (!xfer->bits_per_word)
>>>                         xfer->bits_per_word = spi->bits_per_word;
>>>                 if (!xfer->speed_hz)
>>>                         xfer->speed_hz = spi->max_speed_hz;
>>> +               if (xfer->tx_buf && !xfer->tx_nbits)
>>> +                       xfer->tx_nbits = SPI_NBITS_SINGLE;
>>> +               if (xfer->rx_buf && !xfer->rx_nbits)
>>> +                       xfer->rx_nbits = SPI_NBITS_SINGLE;
>>> +               /* check transfer tx/rx_nbits:
>>> +                * 1. keep the value is not out of single, dual and quad
>>> +                * 2. keep tx/rx_nbits is contained by mode in spi_device
>>> +                * 3. if SPI_3WIRE, tx/rx_nbits should be in single
>>> +                */
>>> +               if (xfer->tx_nbits != SPI_NBITS_SINGLE &&
>>> +                       xfer->tx_nbits != SPI_NBITS_DUAL &&
>>> +                       xfer->tx_nbits != SPI_NBITS_QUAD)
>>> +                       return -EINVAL;
>>> +               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;
>>> +               if ((spi->mode & SPI_3WIRE) &&
>>> +                       (xfer->tx_nbits != SPI_NBITS_SINGLE))
>>> +                       return -EINVAL;
>>> +               /* check transfer rx_nbits */
>>> +               if (xfer->rx_nbits != SPI_NBITS_SINGLE &&
>>> +                       xfer->rx_nbits != SPI_NBITS_DUAL &&
>>> +                       xfer->rx_nbits != SPI_NBITS_QUAD)
>>> +                       return -EINVAL;
>>> +               if ((xfer->rx_nbits == SPI_NBITS_DUAL) &&
>>> +                       !(spi->mode & (SPI_RX_DUAL | SPI_RX_QUAD)))
>>> +                       return -EINVAL;
>>> +               if ((xfer->rx_nbits == SPI_NBITS_QUAD) &&
>>> +                       !(spi->mode & SPI_RX_QUAD))
>>> +                       return -EINVAL;
>>> +               if ((spi->mode & SPI_3WIRE) &&
>>> +                       (xfer->rx_nbits != SPI_NBITS_SINGLE))
>>> +                       return -EINVAL;
>>>         }
>>
>> What a lot of code to check the transfer bits.  It really needs this much?
>>
> This part is also corrected in patch [commit
> id:db90a44177ac39fc22b2da5235b231fccdd4c673].
>
>>> @@ -511,6 +524,11 @@ struct spi_transfer {
>>>         dma_addr_t      rx_dma;
>>>
>>>         unsigned        cs_change:1;
>>> +       u8              tx_nbits;
>>> +       u8              rx_nbits;
>>> +#define        SPI_NBITS_SINGLE        0x01 /* 1bit transfer */
>>> +#define        SPI_NBITS_DUAL          0x02 /* 2bits transfer */
>>> +#define        SPI_NBITS_QUAD          0x04 /* 4bits transfer */
>>
>> These fields increase the size of a spi_transfer by 4 bytes.  If you
>> used bitfields instead it wouldn't increase the size at all since
>> there are still 7 bits left after cs_change.
> Yes, it will increase the size by 4 bytes, but using bitfields here
> will make it logically weird. Bitfields may mean that really have the
> restriction of number of bits. If just for saving memory, then what
> about the other members such as bits_per_world, delay_usecs.