[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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 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


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

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

[Mark Brown]

[-- Attachment #1.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 #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

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

Re: [PATCH 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


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

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

[Mark Brown]

[-- Attachment #1.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 #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

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

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

[Mark Brown]

[-- Attachment #1.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 #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

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

Re: [PATCH 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.

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

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

[Trent Piepho]

On Sun, Aug 11, 2013 at 3:15 AM, wangyuhang <wangyuhang2014-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 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.

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

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

[yuhang wang]

2013/9/4 Trent Piepho <tpiepho-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>:
> On Sun, Aug 11, 2013 at 3:15 AM, wangyuhang <wangyuhang2014-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 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.

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

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

[Mark Brown]

[-- Attachment #1.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 #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

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

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

[Trent Piepho]

On Wed, Sep 4, 2013 at 12:07 AM, yuhang wang <wangyuhang2014-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> 2013/9/4 Trent Piepho <tpiepho-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>:
>> On Sun, Aug 11, 2013 at 3:15 AM, wangyuhang <wangyuhang2014-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 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.

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

[PATCH 1/4] spi: Use of_property_read_u32

[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 <tpiepho-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/spi/spi.c |   49 +++++++++++++++++++++++--------------------------
 1 file changed, 23 insertions(+), 26 deletions(-)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 9e039c6..8fcffe0 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -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

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

[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 <tpiepho-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 include/linux/spi/spi.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
index 887116d..beaffe8 100644
--- a/include/linux/spi/spi.h
+++ b/include/linux/spi/spi.h
@@ -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

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

[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 <tpiepho-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/spi/spi.c       |    6 ------
 include/linux/spi/spi.h |   27 ++++++++++++++-------------
 2 files changed, 14 insertions(+), 19 deletions(-)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 8fcffe0..0e8e5e8 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -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
diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
index beaffe8..4dbf40e 100644
--- a/include/linux/spi/spi.h
+++ b/include/linux/spi/spi.h
@@ -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

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

[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 <tpiepho-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/spi/spi.c       |   49 +++++++++++++++++++----------------------------
 include/linux/spi/spi.h |    2 ++
 2 files changed, 22 insertions(+), 29 deletions(-)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 0e8e5e8..fd79767 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -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;
 		}
 	}
diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
index 4dbf40e..ffccfdd 100644
--- a/include/linux/spi/spi.h
+++ b/include/linux/spi/spi.h
@@ -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

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

[Sourav Poddar]

On Thursday 26 September 2013 05:22 AM, Trent Piepho wrote:
> 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<tpiepho-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>   drivers/spi/spi.c |   49 +++++++++++++++++++++++--------------------------
>   1 file changed, 23 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> index 9e039c6..8fcffe0 100644
> --- a/drivers/spi/spi.c
> +++ b/drivers/spi/spi.c
> @@ -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);
>
Reviewed-by: Sourav Poddar<sourav.poddar-l0cyMroinI0@public.gmane.org>
Tested-by: Sourav Poddar<sourav.poddar-l0cyMroinI0@public.gmane.org>

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

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

[Sourav Poddar]

On Thursday 26 September 2013 05:22 AM, Trent Piepho wrote:
> 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<tpiepho-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>   include/linux/spi/spi.h |    2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
> index 887116d..beaffe8 100644
> --- a/include/linux/spi/spi.h
> +++ b/include/linux/spi/spi.h
> @@ -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;
>
Reviewed-by: Sourav Poddar<sourav.poddar-l0cyMroinI0@public.gmane.org>
Tested-by: Sourav Poddar<sourav.poddar-l0cyMroinI0@public.gmane.org>

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

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

[Sourav Poddar]

On Thursday 26 September 2013 05:22 AM, Trent Piepho wrote:
> 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<tpiepho-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>   drivers/spi/spi.c       |    6 ------
>   include/linux/spi/spi.h |   27 ++++++++++++++-------------
>   2 files changed, 14 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> index 8fcffe0..0e8e5e8 100644
> --- a/drivers/spi/spi.c
> +++ b/drivers/spi/spi.c
> @@ -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
> diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
> index beaffe8..4dbf40e 100644
> --- a/include/linux/spi/spi.h
> +++ b/include/linux/spi/spi.h
> @@ -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;
>
Reviewed-by: Sourav Poddar<sourav.poddar-l0cyMroinI0@public.gmane.org>
Tested-by: Sourav Poddar<sourav.poddar-l0cyMroinI0@public.gmane.org>

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

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

[Sourav Poddar]

On Thursday 26 September 2013 05:22 AM, Trent Piepho wrote:
> 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<tpiepho-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>   drivers/spi/spi.c       |   49 +++++++++++++++++++----------------------------
>   include/linux/spi/spi.h |    2 ++
>   2 files changed, 22 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> index 0e8e5e8..fd79767 100644
> --- a/drivers/spi/spi.c
> +++ b/drivers/spi/spi.c
> @@ -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;
>   		}
>   	}
> diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
> index 4dbf40e..ffccfdd 100644
> --- a/include/linux/spi/spi.h
> +++ b/include/linux/spi/spi.h
> @@ -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<sourav.poddar-l0cyMroinI0@public.gmane.org>
Tested-by: Sourav Poddar<sourav.poddar-l0cyMroinI0@public.gmane.org>

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