Skip to contentSkip to conversation

spi: DUAL and QUAD support

PATCH Applied wangyuhang 7 participants 22 messages Aug 11, 2013 – Sep 26, 2013 1 message not in the archive

Subsystems (3)
Device tree bindings Memory technology devices (MTD) SPI
lore
Share & tools
lore

wangyuhang

author
#0 lore raw

PATCH v3 1/2 spi: DUAL and QUAD support

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 <[email protected]>
---
 drivers/spi/spi.c       |   96 +++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/spi/spi.h |   22 ++++++++++-
 2 files changed, 116 insertions(+), 2 deletions(-)
drivers/spi/spi.c +96 -0
index 004b10f..78f257a 100644
@@ -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;
include/linux/spi/spi.h +20 -2
index 38c2b92..3dc0a86 100644
@@ -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
Signature · 9 lines
1.7.9.5


------------------------------------------------------------------------------
Get 100% visibility into Java/.NET code with AppDynamics Lite!
It's a free troubleshooting tool designed for production.
Get down to code-level detail for bottlenecks, with <2% overhead. 
Download for free and get started troubleshooting in minutes. 
http://pubads.g.doubleclick.net/gampad/clk?id=48897031&iu=/4140/ostg.clktrk
maintainer Applied
#1 lore raw

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:

Quoted 4 lines

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.

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

#2 lore raw
In reply to wangyuhang

Hi Mark, Wang,

Quoted 108 lines · 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<[email protected]>
---
  drivers/spi/spi.c       |   96 +++++++++++++++++++++++++++++++++++++++++++++++
  include/linux/spi/spi.h |   22 ++++++++++-
  2 files changed, 116 insertions(+), 2 deletions(-)
drivers/spi/spi.c +60 -0
index 004b10f..78f257a 100644
@@ -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)

drivers/spi/spi.c static int __spi_async(struct spi_device *spi, struct spi_message *message) · line 1447

Quoted 10 lines
drivers/spi/spi.c +9 -0
  			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...

drivers/spi/spi.c static int __spi_async(struct spi_device *spi, struct spi_message *message) · line 1460

Quoted 13 lines
drivers/spi/spi.c +13 -0
+		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

Quoted 4 lines
drivers/spi/spi.c +4 -0
+		/* check transfer rx_nbits */
+		if (xfer->rx_nbits != SPI_NBITS_SINGLE&&
+			xfer->rx_nbits != SPI_NBITS_DUAL&&
+			xfer->rx_nbits != SPI_NBITS_QUAD)

drivers/spi/spi.c static int __spi_async(struct spi_device *spi, struct spi_message *message) · line 1474

Quoted 10 lines
drivers/spi/spi.c +10 -0
+			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) {
.....
}

Quoted 63 lines
drivers/spi/spi.c +0 -0
  	}
 
  	message->spi = spi;
include/linux/spi/spi.h +19 -1
index 38c2b92..3dc0a86 100644
@@ -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;

include/linux/spi/spi.h struct spi_board_info { · line 882

Quoted 8 lines
include/linux/spi/spi.h +1 -1
@@ -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

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

maintainer
#3 lore raw
Quoted 11 lines · Sourav Poddar

drivers/spi/spi.c static int __spi_async(struct spi_device *spi, struct spi_message *message) · line 1447

Quoted 5 lines · wangyuhang
drivers/spi/spi.c +5 -0
+		/* 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?

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

#4 lore raw
Quoted 11 lines · Mark Brown
Quoted 9 lines · Sourav Poddar

drivers/spi/spi.c static int __spi_async(struct spi_device *spi, struct spi_message *message) · line 1447

Quoted 5 lines · wangyuhang
drivers/spi/spi.c +5 -0
+		/* 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.

------------------------------------------------------------------------------
Introducing Performance Central, a new site from SourceForge and AppDynamics. Performance Central is your source for news, insights, analysis and resources for efficient Application Performance Management. Visit us today!
http://pubads.g.doubleclick.net/gampad/clk?id=48897511&iu=/4140/ostg.clktrk

maintainer
#5 lore raw
In reply to wangyuhang
Quoted 1 line · wangyuhang

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.

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

author
#6 lore raw

PATCH 1/1 spi: quad: fix the name of DT property

In reply to 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 <[email protected]>
---
 Documentation/devicetree/bindings/spi/spi-bus.txt |   10 ++++++++++
 drivers/spi/spi.c                                 |    8 ++++----
 2 files changed, 14 insertions(+), 4 deletions(-)
Documentation/devicetree/bindings/spi/spi-bus.txt +10 -0
index 296015e..800dafe 100644
@@ -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
drivers/spi/spi.c +4 -4
index 7557f61..0075318 100644
@@ -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;
Signature · 6 lines
1.7.9.5


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
maintainer Applied
#7 lore raw

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

Quoted 7 lines · 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.

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.

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

Acked-by on v1 1/1
#8 lore raw

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

In reply to wangyuhang
Quoted 9 lines · 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.

Documentation/devicetree/bindings/spi/spi-bus.txt contain the following properties. · line 61

Quoted 7 lines
Documentation/devicetree/bindings/spi/spi-bus.txt +4 -0
 
 - 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 <[email protected]>

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.

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

maintainer
#9 lore raw

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

Quoted 3 lines · Stephen Warren

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.

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

#10 lore raw
In reply to wangyuhang
Quoted 1 line · wangyuhang

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.

drivers/spi/spi.c static void of_register_spi_devices(struct spi_master *master) · line 873

Quoted 3 lines
drivers/spi/spi.c +2 -0
+               /* Device DUAL/QUAD mode */
+               prop = of_get_property(nc, "spi-tx-nbits", &len);

Why not use of_property_read_u32() here?

drivers/spi/spi.c static void of_register_spi_devices(struct spi_master *master) · line 878

Quoted 5 lines
drivers/spi/spi.c +5 -0
+               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?

drivers/spi/spi.c int spi_setup(struct spi_device *spi) · line 1367

Quoted 7 lines
drivers/spi/spi.c +7 -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;

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)) {

drivers/spi/spi.c int spi_setup(struct spi_device *spi) · line 1373

Quoted 6 lines
drivers/spi/spi.c +6 -0
+       }
+       /* 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?

Quoted 3 lines
        /* 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?

Quoted 39 lines
drivers/spi/spi.c +29 -0
@@ -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;

drivers/spi/spi.c static int __spi_async(struct spi_device *spi, struct spi_message *message) · line 1475

Quoted 10 lines
drivers/spi/spi.c +9 -0
+               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?

include/linux/spi/spi.h struct spi_transfer { · line 531

Quoted 9 lines
include/linux/spi/spi.h +5 -0
@@ -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.

------------------------------------------------------------------------------
Learn the latest--Visual Studio 2012, SharePoint 2013, SQL 2012, more! Discover the easy way to master current and previous Microsoft technologies and advance your career. Get an incredible 1,500+ hours of step-by-step tutorial videos with LearnDevNow. Subscribe today and save!
http://pubads.g.doubleclick.net/gampad/clk?id=58040911&iu=/4140/ostg.clktrk

author
#11 lore raw

2013/9/4 Trent Piepho <[email protected]>:

Quoted 20 lines · wangyuhang
Quoted 1 line

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.

drivers/spi/spi.c static void of_register_spi_devices(struct spi_master *master) · line 873

Quoted 3 lines
drivers/spi/spi.c +2 -0
+               /* Device DUAL/QUAD mode */
+               prop = of_get_property(nc, "spi-tx-nbits", &len);

Why not use of_property_read_u32() here?

drivers/spi/spi.c static void of_register_spi_devices(struct spi_master *master) · line 878

Quoted 5 lines
drivers/spi/spi.c +5 -0
+               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;
Quoted 6 lines

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]

Quoted 20 lines

drivers/spi/spi.c int spi_setup(struct spi_device *spi) · line 1367

Quoted 7 lines
drivers/spi/spi.c +7 -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;

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)) {

drivers/spi/spi.c int spi_setup(struct spi_device *spi) · line 1373

Quoted 6 lines
drivers/spi/spi.c +6 -0
+       }
+       /* 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;
Quoted 3 lines

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

OK, should be added. Thanks.

Quoted 4 lines
Quoted 3 lines
        /* help drivers fail *cleanly* when they need options
         * that aren't supported with their current master
         */

Following this comment is the code:

Quoted 9 lines
        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.

Quoted 43 lines
Quoted 43 lines
drivers/spi/spi.c +33 -0
@@ -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) &&
Quoted 9 lines

drivers/spi/spi.c static int __spi_async(struct spi_device *spi, struct spi_message *message) · line 1475

Quoted 6 lines
drivers/spi/spi.c +5 -0
+                       !(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].

Quoted 13 lines

include/linux/spi/spi.h struct spi_transfer { · line 531

Quoted 9 lines
include/linux/spi/spi.h +5 -0
@@ -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.

------------------------------------------------------------------------------
Learn the latest--Visual Studio 2012, SharePoint 2013, SQL 2012, more! Discover the easy way to master current and previous Microsoft technologies and advance your career. Get an incredible 1,500+ hours of step-by-step tutorial videos with LearnDevNow. Subscribe today and save!
http://pubads.g.doubleclick.net/gampad/clk?id=58040911&iu=/4140/ostg.clktrk

#12 lore raw
Quoted 12 lines · yuhang wang
Quoted 12 lines

include/linux/spi/spi.h struct spi_transfer { · line 531

Quoted 8 lines
        dma_addr_t      rx_dma;

        unsigned        cs_change:1;
include/linux/spi/spi.h +5 -0
+       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.

Quoted 4 lines

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.

Quoted 138 lines · yuhang wang

2013/9/4 Trent Piepho <[email protected]>:

Quoted 20 lines · wangyuhang
Quoted 1 line

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.

drivers/spi/spi.c static void of_register_spi_devices(struct spi_master *master) · line 873

Quoted 3 lines
drivers/spi/spi.c +2 -0
+               /* Device DUAL/QUAD mode */
+               prop = of_get_property(nc, "spi-tx-nbits", &len);

Why not use of_property_read_u32() here?

drivers/spi/spi.c static void of_register_spi_devices(struct spi_master *master) · line 878

Quoted 5 lines
drivers/spi/spi.c +5 -0
+               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;
Quoted 6 lines

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]

Quoted 20 lines

drivers/spi/spi.c int spi_setup(struct spi_device *spi) · line 1367

Quoted 7 lines
drivers/spi/spi.c +7 -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;

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)) {

drivers/spi/spi.c int spi_setup(struct spi_device *spi) · line 1373

Quoted 6 lines
drivers/spi/spi.c +6 -0
+       }
+       /* 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;
Quoted 3 lines

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

OK, should be added. Thanks.

Quoted 4 lines
Quoted 3 lines
        /* help drivers fail *cleanly* when they need options
         * that aren't supported with their current master
         */

Following this comment is the code:

Quoted 9 lines
        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.

Quoted 43 lines
Quoted 43 lines
drivers/spi/spi.c +33 -0
@@ -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) &&
Quoted 9 lines

drivers/spi/spi.c static int __spi_async(struct spi_device *spi, struct spi_message *message) · line 1475

Quoted 6 lines
drivers/spi/spi.c +5 -0
+                       !(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].

Quoted 13 lines

include/linux/spi/spi.h struct spi_transfer { · line 531

Quoted 9 lines
include/linux/spi/spi.h +5 -0
@@ -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.

Quoted 4 lines

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.

------------------------------------------------------------------------------
October Webinars: Code for Performance
Free Intel webinars can help you accelerate application performance. Explore tips for MPI, OpenMP, advanced profiling, and more. Get the most from the latest Intel processors and coprocessors. See abstracts and register >
http://pubads.g.doubleclick.net/gampad/clk?id=60133471&iu=/4140/ostg.clktrk

A message that is not in the archive (sent off-list or not yet received): CA+7tXigzKW8sydfnktr4F5uY-SKk0JAMjaxFn74XLbw_NTYVXg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org

#14 lore raw

PATCH 1/4 spi: Use of_property_read_u32

Instead of getting the raw property, checking the length, and doing endian conversion each time, use the OF function of_property_read_u32() that does all that.

Error messages are slightly improved with error codes from of_property_read_u32() for different ways the property may be invalid (missing, too short, etc.)

Signed-off-by Trent Piepho <[email protected]>
---
 drivers/spi/spi.c |   49 +++++++++++++++++++++++--------------------------
 1 file changed, 23 insertions(+), 26 deletions(-)
drivers/spi/spi.c +23 -26
index 9e039c6..8fcffe0 100644
@@ -838,10 +838,9 @@ static void of_register_spi_devices(struct spi_master *master)
 {
 	struct spi_device *spi;
 	struct device_node *nc;
-	const __be32 *prop;
 	char modalias[SPI_NAME_SIZE + 4];
 	int rc;
-	int len;
+	u32 value;
 
 	if (!master->dev.of_node)
 		return;

@@ -866,14 +865,14 @@ static void of_register_spi_devices(struct spi_master *master)
 		}
 
 		/* Device address */
-		prop = of_get_property(nc, "reg", &len);
-		if (!prop || len < sizeof(*prop)) {
-			dev_err(&master->dev, "%s has no 'reg' property\n",
-				nc->full_name);
+		rc = of_property_read_u32(nc, "reg", &value);
+		if (rc) {
+			dev_err(&master->dev, "%s has no valid 'reg' property (%d)\n",
+				nc->full_name, rc);
 			spi_dev_put(spi);
 			continue;
 		}
-		spi->chip_select = be32_to_cpup(prop);
+		spi->chip_select = value;
 
 		/* Mode (clock phase/polarity/etc.) */
 		if (of_find_property(nc, "spi-cpha", NULL))

@@ -886,55 +885,53 @@ 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-bus-width", &len);
-		if (prop && len == sizeof(*prop)) {
-			switch (be32_to_cpup(prop)) {
-			case SPI_NBITS_SINGLE:
+		if (!of_property_read_u32(nc, "spi-tx-bus-width", &value)) {
+			switch (value) {
+			case 1:
 				break;
-			case SPI_NBITS_DUAL:
+			case 2:
 				spi->mode |= SPI_TX_DUAL;
 				break;
-			case SPI_NBITS_QUAD:
+			case 4:
 				spi->mode |= SPI_TX_QUAD;
 				break;
 			default:
 				dev_err(&master->dev,
 					"spi-tx-bus-width %d not supported\n",
-					be32_to_cpup(prop));
+					value);
 				spi_dev_put(spi);
 				continue;
 			}
 		}
 
-		prop = of_get_property(nc, "spi-rx-bus-width", &len);
-		if (prop && len == sizeof(*prop)) {
-			switch (be32_to_cpup(prop)) {
-			case SPI_NBITS_SINGLE:
+		if (!of_property_read_u32(nc, "spi-rx-bus-width", &value)) {
+			switch (value) {
+			case 1:
 				break;
-			case SPI_NBITS_DUAL:
+			case 2:
 				spi->mode |= SPI_RX_DUAL;
 				break;
-			case SPI_NBITS_QUAD:
+			case 4:
 				spi->mode |= SPI_RX_QUAD;
 				break;
 			default:
 				dev_err(&master->dev,
 					"spi-rx-bus-width %d not supported\n",
-					be32_to_cpup(prop));
+					value);
 				spi_dev_put(spi);
 				continue;
 			}
 		}
 
 		/* Device speed */
-		prop = of_get_property(nc, "spi-max-frequency", &len);
-		if (!prop || len < sizeof(*prop)) {
-			dev_err(&master->dev, "%s has no 'spi-max-frequency' property\n",
-				nc->full_name);
+		rc = of_property_read_u32(nc, "spi-max-frequency", &value);
+		if (rc) {
+			dev_err(&master->dev, "%s has no valid 'spi-max-frequency' property (%d)\n",
+				nc->full_name, rc);
 			spi_dev_put(spi);
 			continue;
 		}
-		spi->max_speed_hz = be32_to_cpup(prop);
+		spi->max_speed_hz = value;
 
 		/* IRQ */
 		spi->irq = irq_of_parse_and_map(nc, 0);

------------------------------------------------------------------------------
October Webinars: Code for Performance
Free Intel webinars can help you accelerate application performance. Explore tips for MPI, OpenMP, advanced profiling, and more. Get the most from the latest Intel processors and coprocessors. See abstracts and register >
http://pubads.g.doubleclick.net/gampad/clk?id=60133471&iu=/4140/ostg.clktrk

Reviewed-by Tested-by on v1 1/4
#15 lore raw

Re: [PATCH 1/4] spi: Use of_property_read_u32

Quoted 113 lines · Trent Piepho

Instead of getting the raw property, checking the length, and doing endian conversion each time, use the OF function of_property_read_u32() that does all that.

Error messages are slightly improved with error codes from of_property_read_u32() for different ways the property may be invalid (missing, too short, etc.)

Signed-off-by Trent Piepho<[email protected]>
---
  drivers/spi/spi.c |   49 +++++++++++++++++++++++--------------------------
  1 file changed, 23 insertions(+), 26 deletions(-)
drivers/spi/spi.c +20 -25
index 9e039c6..8fcffe0 100644
@@ -838,10 +838,9 @@ static void of_register_spi_devices(struct spi_master *master)
  {
  	struct spi_device *spi;
  	struct device_node *nc;
-	const __be32 *prop;
  	char modalias[SPI_NAME_SIZE + 4];
  	int rc;
-	int len;
+	u32 value;
 
  	if (!master->dev.of_node)
  		return;

@@ -866,14 +865,14 @@ static void of_register_spi_devices(struct spi_master *master)
  		}
 
  		/* Device address */
-		prop = of_get_property(nc, "reg",&len);
-		if (!prop || len<  sizeof(*prop)) {
-			dev_err(&master->dev, "%s has no 'reg' property\n",
-				nc->full_name);
+		rc = of_property_read_u32(nc, "reg",&value);
+		if (rc) {
+			dev_err(&master->dev, "%s has no valid 'reg' property (%d)\n",
+				nc->full_name, rc);
  			spi_dev_put(spi);
  			continue;
  		}
-		spi->chip_select = be32_to_cpup(prop);
+		spi->chip_select = value;
 
  		/* Mode (clock phase/polarity/etc.) */
  		if (of_find_property(nc, "spi-cpha", NULL))

@@ -886,55 +885,53 @@ 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-bus-width",&len);
-		if (prop&&  len == sizeof(*prop)) {
-			switch (be32_to_cpup(prop)) {
-			case SPI_NBITS_SINGLE:
+		if (!of_property_read_u32(nc, "spi-tx-bus-width",&value)) {
+			switch (value) {
+			case 1:
  				break;
-			case SPI_NBITS_DUAL:
+			case 2:
  				spi->mode |= SPI_TX_DUAL;
  				break;
-			case SPI_NBITS_QUAD:
+			case 4:
  				spi->mode |= SPI_TX_QUAD;
  				break;
  			default:
  				dev_err(&master->dev,
  					"spi-tx-bus-width %d not supported\n",
-					be32_to_cpup(prop));
+					value);
  				spi_dev_put(spi);
  				continue;
  			}
  		}
 
-		prop = of_get_property(nc, "spi-rx-bus-width",&len);
-		if (prop&&  len == sizeof(*prop)) {
-			switch (be32_to_cpup(prop)) {
-			case SPI_NBITS_SINGLE:
+		if (!of_property_read_u32(nc, "spi-rx-bus-width",&value)) {
+			switch (value) {
+			case 1:
  				break;
-			case SPI_NBITS_DUAL:
+			case 2:
  				spi->mode |= SPI_RX_DUAL;
  				break;
-			case SPI_NBITS_QUAD:
+			case 4:
  				spi->mode |= SPI_RX_QUAD;
  				break;
  			default:
  				dev_err(&master->dev,
  					"spi-rx-bus-width %d not supported\n",
-					be32_to_cpup(prop));
+					value);
  				spi_dev_put(spi);
  				continue;
  			}
  		}
 
  		/* Device speed */
-		prop = of_get_property(nc, "spi-max-frequency",&len);
-		if (!prop || len<  sizeof(*prop)) {
-			dev_err(&master->dev, "%s has no 'spi-max-frequency' property\n",
-				nc->full_name);
+		rc = of_property_read_u32(nc, "spi-max-frequency",&value);
+		if (rc) {

drivers/spi/spi.c static void of_register_spi_devices(struct spi_master *master) · line 937

Quoted 10 lines
drivers/spi/spi.c +3 -1
+			dev_err(&master->dev, "%s has no valid 'spi-max-frequency' property (%d)\n",
+				nc->full_name, rc);
  			spi_dev_put(spi);
  			continue;
  		}
-		spi->max_speed_hz = be32_to_cpup(prop);
+		spi->max_speed_hz = value;
 
  		/* IRQ */
  		spi->irq = irq_of_parse_and_map(nc, 0);
Reviewed-by Sourav Poddar<[email protected]>
Tested-by Sourav Poddar<[email protected]>

------------------------------------------------------------------------------
October Webinars: Code for Performance
Free Intel webinars can help you accelerate application performance. Explore tips for MPI, OpenMP, advanced profiling, and more. Get the most from the latest Intel processors and coprocessors. See abstracts and register >
http://pubads.g.doubleclick.net/gampad/clk?id=60133471&iu=/4140/ostg.clktrk

#16 lore raw

PATCH 2/4 spi: Order fields in spi_device for better packing

In reply to a message not in the archive

Now that spi_device->mode is a u16, the chip_select, bits_per_mode, and mode fields pack poorly, taking 8 bytes: four data and four padding. By moving (u8)bits_per_word up one position, to after (u8)chip_select, they pack better and only use 4 bytes.

Signed-off-by Trent Piepho <[email protected]>
---
 include/linux/spi/spi.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
include/linux/spi/spi.h +1 -1
index 887116d..beaffe8 100644
@@ -74,6 +74,7 @@ struct spi_device {
 	struct spi_master	*master;
 	u32			max_speed_hz;
 	u8			chip_select;
+	u8			bits_per_word;
 	u16			mode;
 #define	SPI_CPHA	0x01			/* clock phase */
 #define	SPI_CPOL	0x02			/* clock polarity */

@@ -91,7 +92,6 @@ struct spi_device {
 #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;
 	void			*controller_data;

------------------------------------------------------------------------------
October Webinars: Code for Performance
Free Intel webinars can help you accelerate application performance. Explore tips for MPI, OpenMP, advanced profiling, and more. Get the most from the latest Intel processors and coprocessors. See abstracts and register >
http://pubads.g.doubleclick.net/gampad/clk?id=60133471&iu=/4140/ostg.clktrk

Reviewed-by Tested-by on v1 2/4
#17 lore raw

Re: [PATCH 2/4] spi: Order fields in spi_device for better packing

Quoted 24 lines · Trent Piepho

Now that spi_device->mode is a u16, the chip_select, bits_per_mode, and mode fields pack poorly, taking 8 bytes: four data and four padding. By moving (u8)bits_per_word up one position, to after (u8)chip_select, they pack better and only use 4 bytes.

Signed-off-by Trent Piepho<[email protected]>
---
  include/linux/spi/spi.h |    2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
include/linux/spi/spi.h +1 -0
index 887116d..beaffe8 100644
@@ -74,6 +74,7 @@ struct spi_device {
  	struct spi_master	*master;
  	u32			max_speed_hz;
  	u8			chip_select;
+	u8			bits_per_word;
  	u16			mode;
  #define	SPI_CPHA	0x01			/* clock phase */
  #define	SPI_CPOL	0x02			/* clock polarity */

include/linux/spi/spi.h struct spi_device { · line 97

Quoted 7 lines
include/linux/spi/spi.h +0 -1
@@ -91,7 +92,6 @@ struct spi_device {
  #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;
  	void			*controller_data;
Reviewed-by Sourav Poddar<[email protected]>
Tested-by Sourav Poddar<[email protected]>

------------------------------------------------------------------------------
October Webinars: Code for Performance
Free Intel webinars can help you accelerate application performance. Explore tips for MPI, OpenMP, advanced profiling, and more. Get the most from the latest Intel processors and coprocessors. See abstracts and register >
http://pubads.g.doubleclick.net/gampad/clk?id=60133471&iu=/4140/ostg.clktrk

#18 lore raw

PATCH 3/4 spi: Encode data width more efficiently

In reply to a message not in the archive

The TX and RX data bus width fields, [tr]x_nbits, use 8 bits each in every SPI transfer. Due to padding, this increases the size of a spi_transfer by 32 bits. Really, only two bits are needed to encode the three possible values and there are seven bits of padding after cs_change. By making [tr]x_nbits bitfields they can fit in this unused space and not increase the size of a spi_transfer at all.

The standard width of one bit is currently indicated by two values, 0 or SPI_NBITS_SINGLE (1). There is no need to have two different values for this. By making SPI_NBITS_SINGLE have the value 0, the code in __spi_async() that transforms 0 into SPI_NBITS_SINGLE becomes unnecessary.

Try to improve the grammar and clarity of the bit width documentation. Try to be clear this refers to the bit width and not the bit length, which is a different property.

Signed-off-by Trent Piepho <[email protected]>
---
 drivers/spi/spi.c       |    6 ------
 include/linux/spi/spi.h |   27 ++++++++++++++-------------
 2 files changed, 14 insertions(+), 19 deletions(-)
drivers/spi/spi.c +0 -6
index 8fcffe0..0e8e5e8 100644
@@ -1445,8 +1445,6 @@ 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) {
 		message->frame_length += xfer->len;

@@ -1475,10 +1473,6 @@ static int __spi_async(struct spi_device *spi, struct spi_message *message)
 		    xfer->speed_hz > master->max_speed_hz)
 			return -EINVAL;
 
-		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
include/linux/spi/spi.h +14 -13
index beaffe8..4dbf40e 100644
@@ -43,6 +43,9 @@ extern struct bus_type spi_bus_type;
  *	The "active low" default for chipselect mode can be overridden
  *	(by specifying SPI_CS_HIGH) as can the "MSB first" default for
  *	each word in a transfer (by specifying SPI_LSB_FIRST).
+ *	The data width mode bits do not indicate the current mode, as the
+ *	other mode bits do, but rather the maximum supported width.  The
+ *	actual width used is specified for each spi_transfer.
  * @bits_per_word: Data transfers involve one or more words; word sizes
  *	like eight or 12 bits are common.  In-memory wordsizes are
  *	powers of two bytes (e.g. 20 bit samples use 32 bits).

@@ -463,10 +466,8 @@ 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.
+ * @tx_nbits: number of data lines used for transmit.
+ * @rx_nbits: number of data lines used for receive.
  * @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.

@@ -521,10 +522,10 @@ 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.
+ * When the SPI bus has more than one data line, the bit-width of the tranfer,
+ * i.e. SPI_NBITS_SINGLE (1x), SPI_NBITS_DUAL (2x), or SPI_NBITS_QUAD (4x)
+ * mode, is specified with @tx_nbits and @rx_nbits.  In bi-directional mode,
+ * both should be set.  Setting these fields to zero selects normal (1x) mode.
  *
  * The code that submits an spi_message (and its spi_transfers)
  * to the lower layers is responsible for managing its memory.

@@ -546,11 +547,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 */
+	unsigned	tx_nbits:2;
+	unsigned	rx_nbits:2;
+#define	SPI_NBITS_SINGLE	0x00 /* 1x wide transfer */
+#define	SPI_NBITS_DUAL		0x01 /* 2x wide transfer */
+#define	SPI_NBITS_QUAD		0x02 /* 4x wide transfer */
 	u8		bits_per_word;
 	u16		delay_usecs;
 	u32		speed_hz;

------------------------------------------------------------------------------
October Webinars: Code for Performance
Free Intel webinars can help you accelerate application performance. Explore tips for MPI, OpenMP, advanced profiling, and more. Get the most from the latest Intel processors and coprocessors. See abstracts and register >
http://pubads.g.doubleclick.net/gampad/clk?id=60133471&iu=/4140/ostg.clktrk

Reviewed-by Tested-by on v1 3/4
#19 lore raw

Re: [PATCH 3/4] spi: Encode data width more efficiently

Quoted 96 lines · Trent Piepho

The TX and RX data bus width fields, [tr]x_nbits, use 8 bits each in every SPI transfer. Due to padding, this increases the size of a spi_transfer by 32 bits. Really, only two bits are needed to encode the three possible values and there are seven bits of padding after cs_change. By making [tr]x_nbits bitfields they can fit in this unused space and not increase the size of a spi_transfer at all.

The standard width of one bit is currently indicated by two values, 0 or SPI_NBITS_SINGLE (1). There is no need to have two different values for this. By making SPI_NBITS_SINGLE have the value 0, the code in __spi_async() that transforms 0 into SPI_NBITS_SINGLE becomes unnecessary.

Try to improve the grammar and clarity of the bit width documentation. Try to be clear this refers to the bit width and not the bit length, which is a different property.

Signed-off-by Trent Piepho<[email protected]>
---
  drivers/spi/spi.c       |    6 ------
  include/linux/spi/spi.h |   27 ++++++++++++++-------------
  2 files changed, 14 insertions(+), 19 deletions(-)
drivers/spi/spi.c +0 -6
index 8fcffe0..0e8e5e8 100644
@@ -1445,8 +1445,6 @@ 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) {
  		message->frame_length += xfer->len;

@@ -1475,10 +1473,6 @@ static int __spi_async(struct spi_device *spi, struct spi_message *message)
  		    xfer->speed_hz>  master->max_speed_hz)
  			return -EINVAL;
 
-		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
include/linux/spi/spi.h +9 -11
index beaffe8..4dbf40e 100644
@@ -43,6 +43,9 @@ extern struct bus_type spi_bus_type;
   *	The "active low" default for chipselect mode can be overridden
   *	(by specifying SPI_CS_HIGH) as can the "MSB first" default for
   *	each word in a transfer (by specifying SPI_LSB_FIRST).
+ *	The data width mode bits do not indicate the current mode, as the
+ *	other mode bits do, but rather the maximum supported width.  The
+ *	actual width used is specified for each spi_transfer.
   * @bits_per_word: Data transfers involve one or more words; word sizes
   *	like eight or 12 bits are common.  In-memory wordsizes are
   *	powers of two bytes (e.g. 20 bit samples use 32 bits).

@@ -463,10 +466,8 @@ 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.
+ * @tx_nbits: number of data lines used for transmit.
+ * @rx_nbits: number of data lines used for receive.
   * @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.

@@ -521,10 +522,10 @@ 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.
+ * When the SPI bus has more than one data line, the bit-width of the tranfer,
+ * i.e. SPI_NBITS_SINGLE (1x), SPI_NBITS_DUAL (2x), or SPI_NBITS_QUAD (4x)
+ * mode, is specified with @tx_nbits and @rx_nbits.  In bi-directional mode,
+ * both should be set.  Setting these fields to zero selects normal (1x) mode.
   *
   * The code that submits an spi_message (and its spi_transfers)
   * to the lower layers is responsible for managing its memory.

@@ -546,11 +547,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 */

include/linux/spi/spi.h struct spi_transfer { · line 557

Quoted 10 lines
include/linux/spi/spi.h +5 -2
-#define	SPI_NBITS_DUAL		0x02 /* 2bits transfer */
-#define	SPI_NBITS_QUAD		0x04 /* 4bits transfer */
+	unsigned	tx_nbits:2;
+	unsigned	rx_nbits:2;
+#define	SPI_NBITS_SINGLE	0x00 /* 1x wide transfer */
+#define	SPI_NBITS_DUAL		0x01 /* 2x wide transfer */
+#define	SPI_NBITS_QUAD		0x02 /* 4x wide transfer */
  	u8		bits_per_word;
  	u16		delay_usecs;
  	u32		speed_hz;
Reviewed-by Sourav Poddar<[email protected]>
Tested-by Sourav Poddar<[email protected]>

------------------------------------------------------------------------------
October Webinars: Code for Performance
Free Intel webinars can help you accelerate application performance. Explore tips for MPI, OpenMP, advanced profiling, and more. Get the most from the latest Intel processors and coprocessors. See abstracts and register >
http://pubads.g.doubleclick.net/gampad/clk?id=60133471&iu=/4140/ostg.clktrk

#20 lore raw

PATCH 4/4 spi: Simplify bit width checking code

In reply to a message not in the archive

The code that checks [tr]x_nbits for every transfer is more complex than it needs to be.

Checking for SPI_3WIRE isn't needed. spi_config() already prevents 3WIRE mode from being combined with DUAL or QUAD mode support. So there is no need to differentiate between a single bit device with SPI_3WIRE set and one with without. It doesn't change the allowed bit widths.

By using the sensible rule that the width codes should be sequential, it's possible to avoid checking for every single valid code individually and instead use a single comparison to check for codes greater than the largest valid code.

Signed-off-by Trent Piepho <[email protected]>
---
 drivers/spi/spi.c       |   49 +++++++++++++++++++----------------------------
 include/linux/spi/spi.h |    2 ++
 2 files changed, 22 insertions(+), 29 deletions(-)
drivers/spi/spi.c +20 -29
index 0e8e5e8..fd79767 100644
@@ -1447,6 +1447,8 @@ static int __spi_async(struct spi_device *spi, struct spi_message *message)
 	 * it is not set for this transfer.
 	 */
 	list_for_each_entry(xfer, &message->transfers, transfer_list) {
+		int max_width;
+
 		message->frame_length += xfer->len;
 		if (!xfer->bits_per_word)
 			xfer->bits_per_word = spi->bits_per_word;

@@ -1473,40 +1475,29 @@ static int __spi_async(struct spi_device *spi, struct spi_message *message)
 		    xfer->speed_hz > master->max_speed_hz)
 			return -EINVAL;
 
-		/* 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
+		/* Check transfer's tx/rx_nbits.  Do not allow a width greater
+		 * than the max the device supports.  As the width codes are
+		 * assigned sequentially, a non-existent width code will also be
+		 * greater than the largest allowed value.
 		 */
 		if (xfer->tx_buf) {
-			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))
+			if (spi->mode & SPI_TX_QUAD)
+				max_width = SPI_NBITS_QUAD;
+			else if (spi->mode & SPI_TX_DUAL)
+				max_width = SPI_NBITS_DUAL;
+			else
+				max_width = SPI_NBITS_SINGLE;
+			if (xfer->tx_nbits > max_width)
 				return -EINVAL;
 		}
-		/* check transfer rx_nbits */
 		if (xfer->rx_buf) {
-			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))
+			if (spi->mode & SPI_RX_QUAD)
+				max_width = SPI_NBITS_QUAD;
+			else if (spi->mode & SPI_RX_DUAL)
+				max_width = SPI_NBITS_DUAL;
+			else
+				max_width = SPI_NBITS_SINGLE;
+			if (xfer->rx_nbits > max_width)
 				return -EINVAL;
 		}
 	}
include/linux/spi/spi.h +2 -0
index 4dbf40e..ffccfdd 100644
@@ -549,6 +549,8 @@ struct spi_transfer {
 	unsigned	cs_change:1;
 	unsigned	tx_nbits:2;
 	unsigned	rx_nbits:2;
+/* These should be assigned sequentially, so that numeric comparisons produce
+ * the correct result, e.g. SPI_NBITS_DUAL < SPI_NBITS_QUAD.  */
 #define	SPI_NBITS_SINGLE	0x00 /* 1x wide transfer */
 #define	SPI_NBITS_DUAL		0x01 /* 2x wide transfer */
 #define	SPI_NBITS_QUAD		0x02 /* 4x wide transfer */

------------------------------------------------------------------------------
October Webinars: Code for Performance
Free Intel webinars can help you accelerate application performance. Explore tips for MPI, OpenMP, advanced profiling, and more. Get the most from the latest Intel processors and coprocessors. See abstracts and register >
http://pubads.g.doubleclick.net/gampad/clk?id=60133471&iu=/4140/ostg.clktrk

Reviewed-by Tested-by on v1 4/4
#21 lore raw

Re: [PATCH 4/4] spi: Simplify bit width checking code

Quoted 97 lines · Trent Piepho

The code that checks [tr]x_nbits for every transfer is more complex than it needs to be.

Checking for SPI_3WIRE isn't needed. spi_config() already prevents 3WIRE mode from being combined with DUAL or QUAD mode support. So there is no need to differentiate between a single bit device with SPI_3WIRE set and one with without. It doesn't change the allowed bit widths.

By using the sensible rule that the width codes should be sequential, it's possible to avoid checking for every single valid code individually and instead use a single comparison to check for codes greater than the largest valid code.

Signed-off-by Trent Piepho<[email protected]>
---
  drivers/spi/spi.c       |   49 +++++++++++++++++++----------------------------
  include/linux/spi/spi.h |    2 ++
  2 files changed, 22 insertions(+), 29 deletions(-)
drivers/spi/spi.c +20 -29
index 0e8e5e8..fd79767 100644
@@ -1447,6 +1447,8 @@ static int __spi_async(struct spi_device *spi, struct spi_message *message)
  	 * it is not set for this transfer.
  	 */
  	list_for_each_entry(xfer,&message->transfers, transfer_list) {
+		int max_width;
+
  		message->frame_length += xfer->len;
  		if (!xfer->bits_per_word)
  			xfer->bits_per_word = spi->bits_per_word;

@@ -1473,40 +1475,29 @@ static int __spi_async(struct spi_device *spi, struct spi_message *message)
  		    xfer->speed_hz>  master->max_speed_hz)
  			return -EINVAL;
 
-		/* 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
+		/* Check transfer's tx/rx_nbits.  Do not allow a width greater
+		 * than the max the device supports.  As the width codes are
+		 * assigned sequentially, a non-existent width code will also be
+		 * greater than the largest allowed value.
  		 */
  		if (xfer->tx_buf) {
-			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))
+			if (spi->mode&  SPI_TX_QUAD)
+				max_width = SPI_NBITS_QUAD;
+			else if (spi->mode&  SPI_TX_DUAL)
+				max_width = SPI_NBITS_DUAL;
+			else
+				max_width = SPI_NBITS_SINGLE;
+			if (xfer->tx_nbits>  max_width)
  				return -EINVAL;
  		}
-		/* check transfer rx_nbits */
  		if (xfer->rx_buf) {
-			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))
+			if (spi->mode&  SPI_RX_QUAD)
+				max_width = SPI_NBITS_QUAD;
+			else if (spi->mode&  SPI_RX_DUAL)
+				max_width = SPI_NBITS_DUAL;
+			else
+				max_width = SPI_NBITS_SINGLE;
+			if (xfer->rx_nbits>  max_width)
  				return -EINVAL;
  		}
  	}
include/linux/spi/spi.h +0 -0
index 4dbf40e..ffccfdd 100644

include/linux/spi/spi.h struct spi_transfer { · line 556

Quoted 8 lines
include/linux/spi/spi.h +2 -0
@@ -549,6 +549,8 @@ struct spi_transfer {
  	unsigned	cs_change:1;
  	unsigned	tx_nbits:2;
  	unsigned	rx_nbits:2;
+/* These should be assigned sequentially, so that numeric comparisons produce
+ * the correct result, e.g. SPI_NBITS_DUAL<  SPI_NBITS_QUAD.  */
  #define	SPI_NBITS_SINGLE	0x00 /* 1x wide transfer */
  #define	SPI_NBITS_DUAL		0x01 /* 2x wide transfer */
  #define	SPI_NBITS_QUAD		0x02 /* 4x wide transfer */
Reviewed-by Sourav Poddar<[email protected]>
Tested-by Sourav Poddar<[email protected]>

------------------------------------------------------------------------------
October Webinars: Code for Performance
Free Intel webinars can help you accelerate application performance. Explore tips for MPI, OpenMP, advanced profiling, and more. Get the most from the latest Intel processors and coprocessors. See abstracts and register >
http://pubads.g.doubleclick.net/gampad/clk?id=60133471&iu=/4140/ostg.clktrk

maintainer
#22 lore raw
In reply to Trent Piepho
Quoted 5 lines · Trent Piepho

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.

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