spi: soft_spi: Support NULL din/dout buffers
PATCH 1/1 spi: soft_spi: Support NULL din/dout buffers
This mirrors the conventions used in other SPI drivers (kirkwood, davinci, atmel, et al) where the din/dout buffer can be NULL when the received/transmitted data isn't important. This reduces the need for allocating additional buffers when write-only/read-only functionality is needed.
In the din == NULL case, the received data is simply not stored. In the dout == NULL case, zeroes are transmitted.
---
So going through and cleaning up some of my trees and determining which patches have not been taken. A little more justification for this patch - I've gone through all of the current SPI implementations and have determined what (if any) support the driver has for half duplex operation by giving NULL for one of the two buffers (din/dout). This greatly reduces the need for temporary buffers for writing or reading large amounts of half-duplex data to devices over SPI. Here's the list:
altera_spi.c - SUPPORTS NULL din or dout andes_spi.c - REQUIRES NULL din or dout armada100_spi.c - SUPPORTS NULL din or dout atmel_spi.c - SUPPORTS NULL din or dout bfin_spi6xx.c - SUPPORTS NULL din or dout bfin_spi.c - SUPPORTS NULL din or dout cf_qspi.c - SUPPORTS NULL din or dout cf_spi.c - SUPPORTS NULL din or dout davinci_spi.c - SUPPORTS NULL din or dout exynos_spi.c - SUPPORTS NULL din or dout fdt_spi.c-tegra20_sf - SUPPORTS NULL din or dout fdt_spi.c-tegra20_sl - SUPPORTS NULL din or dout fdt_spi.c-tegra114 - SUPPORTS NULL din or dout fsl_espi.c - SUPPORTS NULL din (NOT dout) ftssp010_spi.c - SUPPORTS NULL din or dout ich.c - SUPPORTS NULL din or dout kirkwood_spi.c - SUPPORTS NULL din or dout mpc52xx_spi.c - NO SUPPORT mpc8xxx_spi.c - NO SUPPORT mxc_spi.c - NO SUPPORT mxs_spi.c - REQUIRES NULL din or dout oc_tiny_spi.c - SUPPORTS NULL din or dout omap3_spi.c - SUPPORTS NULL din or dout sandbox_spi.c - SUPPORTS NULL din or dout sh_qspi.c - SUPPORTS NULL din or dout sh_spi.c - SUPPORTS NULL din or dout soft_spi.c - NO SUPPORT ti_qspi.c - SUPPORTS NULL din or dout xilinx_spi.c - SUPPORTS NULL din or dout zynq_spi.c - SUPPORTS NULL din or dout
Furthermore, this would not affect any board already using temporary buffers and it would continue to work as usual. The *only* obscure corner case that this will not work is if NULL (0x0) is a valid buffer address in your system and you are transferring SPI to/from it. This seems very unlikely and would probably break in other ways from library functions that check their arguments.
drivers/spi/soft_spi.c | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-)
drivers/spi/soft_spi.c +13 -5
@@ -137,9 +137,15 @@ int spi_xfer(struct spi_slave *slave, unsigned int bitlen,* Check if it is time to work on a new byte. */ if ((j %8 ) ==0 ) { - tmpdout = *txd++; +if (txd) { + tmpdout = *txd++; + }else { + tmpdout =0 ; + }if (j !=0 ) { - *rxd++ = tmpdin; +if (rxd) { + *rxd++ = tmpdin; + } } tmpdin =0 ; }
@@ -164,9 +170,11 @@ int spi_xfer(struct spi_slave *slave, unsigned int bitlen,* bits over to left-justify them. Then store the last byte * read in. */ -if ((bitlen %8 ) !=0 ) - tmpdin <<=8 - (bitlen %8 ); - *rxd++ = tmpdin; +if (rxd) { +if ((bitlen %8 ) !=0 ) + tmpdin <<=8 - (bitlen %8 ); + *rxd++ = tmpdin; + }if (flags &SPI_XFER_END )spi_cs_deactivate (slave);
Signature · 1 line
1.9.0.rc3.12.gbc97e2d
PATCH 1/1 spi: soft_spi: Support NULL din/dout buffers
I further justify this with a list of drivers that depend on half-duplex SPI operation:
drivers/mmc/mmc_spi.c: spi_xfer(spi, 2 * 8, tok, NULL, 0); drivers/mtd/spi/eeprom_m95xxx.c: if (spi_xfer(slave, 8, buf, NULL, SPI_XFER_BEGIN | SPI_XFER_END)) drivers/mtd/spi/sf_ops.c: ret = spi_xfer(spi, 8, NULL, &status, 0); drivers/net/e1000_spi.c: return e1000_spi_xfer(hw, 8*sizeof(op), op, NULL, intr); drivers/net/enc28j60.c: spi_xfer(enc->slave, 2 * 8, dout, NULL, drivers/rtc/m41t94.c: ret = spi_xfer(slave, 64, buf, NULL, SPI_XFER_BEGIN | SPI_XFER_END);
And an old email first highlighting this issue in 2010 with the soft_spi driver:
http://article.gmane.org/gmane.comp.boot-loaders.u-boot/75839/match=soft_spi
PATCH 1/1 spi: soft_spi: Support NULL din/dout buffers
In reply to Andrew RuderQuoted 52 lines · Andrew Ruder
This mirrors the conventions used in other SPI drivers (kirkwood, davinci, atmel, et al) where the din/dout buffer can be NULL when the received/transmitted data isn't important. This reduces the need for allocating additional buffers when write-only/read-only functionality is needed.
In the din == NULL case, the received data is simply not stored. In the dout == NULL case, zeroes are transmitted.
Signed-off-by Andrew Ruder <[email protected]>Cc Jean-Christophe PLAGNIOL-VILLARD <[email protected]>Cc Jagan Teki <[email protected]>---
So going through and cleaning up some of my trees and determining which patches have not been taken. A little more justification for this patch - I've gone through all of the current SPI implementations and have determined what (if any) support the driver has for half duplex operation by giving NULL for one of the two buffers (din/dout). This greatly reduces the need for temporary buffers for writing or reading large amounts of half-duplex data to devices over SPI. Here's the list:altera_spi.c - SUPPORTS NULL din or dout andes_spi.c - REQUIRES NULL din or dout armada100_spi.c - SUPPORTS NULL din or dout atmel_spi.c - SUPPORTS NULL din or dout bfin_spi6xx.c - SUPPORTS NULL din or dout bfin_spi.c - SUPPORTS NULL din or dout cf_qspi.c - SUPPORTS NULL din or dout cf_spi.c - SUPPORTS NULL din or dout davinci_spi.c - SUPPORTS NULL din or dout exynos_spi.c - SUPPORTS NULL din or dout fdt_spi.c-tegra20_sf - SUPPORTS NULL din or dout fdt_spi.c-tegra20_sl - SUPPORTS NULL din or dout fdt_spi.c-tegra114 - SUPPORTS NULL din or dout fsl_espi.c - SUPPORTS NULL din (NOT dout) ftssp010_spi.c - SUPPORTS NULL din or dout ich.c - SUPPORTS NULL din or dout kirkwood_spi.c - SUPPORTS NULL din or dout mpc52xx_spi.c - NO SUPPORT mpc8xxx_spi.c - NO SUPPORT mxc_spi.c - NO SUPPORT mxs_spi.c - REQUIRES NULL din or dout oc_tiny_spi.c - SUPPORTS NULL din or dout omap3_spi.c - SUPPORTS NULL din or dout sandbox_spi.c - SUPPORTS NULL din or dout sh_qspi.c - SUPPORTS NULL din or dout sh_spi.c - SUPPORTS NULL din or dout soft_spi.c - NO SUPPORT ti_qspi.c - SUPPORTS NULL din or dout xilinx_spi.c - SUPPORTS NULL din or dout zynq_spi.c - SUPPORTS NULL din or dout
Quoted 6 lines
Furthermore, this would not affect any board already using temporary buffers and it would continue to work as usual. The *only* obscure corner case that this will not work is if NULL (
0x0) is a valid buffer address in your system and you are transferring SPI to/from it. This seems very unlikely and would probably break in other ways from library functions that check their arguments.
At-least from zynq_spi.c case - I wouldn't find that obscure case as you pointed and even with dout NULL which is status poll from (sf_ops.c).
Let me come back again and will show my results for zynq_spi.c
It would be great if you mentioned issue scenario for status poll casedrivers/mtd/spi/sf_ops.c: ret = spi_xfer(spi, 8, NULL,
&status, 0);
Quoted 42 lines
drivers/spi/soft_spi.c | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-)drivers/spi/soft_spi.c +13
-6@@ -137,9 +137,15 @@ int spi_xfer(struct spi_slave *slave, unsigned int bitlen,* Check if it is time to work on a new byte. */ if ((j %8 ) ==0 ) { - tmpdout = *txd++; +if (txd) { + tmpdout = *txd++; + }else { + tmpdout =0 ; + }if (j !=0 ) { - *rxd++ = tmpdin; +if (rxd) { + *rxd++ = tmpdin; + } } tmpdin =0 ; }@@ -164,9 +170,11 @@ int spi_xfer(struct spi_slave *slave, unsigned int bitlen,* bits over to left-justify them. Then store the last byte * read in. */ -if ((bitlen %8 ) !=0 ) - tmpdin <<=8 - (bitlen %8 ); - *rxd++ = tmpdin; +if (rxd) { +if ((bitlen %8 ) !=0 ) + tmpdin <<=8 - (bitlen %8 ); + *rxd++ = tmpdin; + }if (flags &SPI_XFER_END )spi_cs_deactivate (slave); --
Quoted 2 lines
1.9.0.rc3.12.gbc97e2d
thanks!
Signature · 1 line
Jagan.
PATCH 1/1 spi: soft_spi: Support NULL din/dout buffers
Quoted 2 lines · Jagan Teki
At-least from
zynq_spi.ccase - I wouldn't find that obscure case as you pointed and even with dout NULL which is status poll from (sf_ops.c).
zynq_spi is correct, soft_spi is not.
zynq_spi.c:
int spi_xfer(struct spi_slave *slave, unsigned int bitlen, const void *dout,
void *din, unsigned long flags)
{
const u8 *tx_buf = dout;
u8 *rx_buf = din, buf;
[...]
if (tx_buf)
buf = *tx_buf++;
else
buf = 0;
[...]
if (rx_buf)
*rx_buf++ = buf;
[...]Quoted 3 lines
It would be great if you mentioned issue scenario for status poll case
drivers/mtd/spi/sf_ops.c: ret = spi_xfer(spi, 8, NULL,
&status, 0);
That breaks on soft_spi, hence the patch.
Cheers,
Andy
PATCH 1/1 spi: soft_spi: Support NULL din/dout buffers
Quoted 22 lines · Andrew Ruder
Quoted 2 lines · Jagan Teki
At-least from
zynq_spi.ccase - I wouldn't find that obscure case as you pointed and even with dout NULL which is status poll from (sf_ops.c).
zynq_spiis correct,soft_spiis not.
zynq_spi.c:
int spi_xfer(structspi_slave*slave, unsigned int bitlen, const void *dout,
void *din, unsigned long flags){ const u8 *tx_buf = dout; u8 *rx_buf = din, buf; [...] if (tx_buf) buf = *tx_buf++; else buf = 0; [...] if (rx_buf) *rx_buf++ = buf; [...]
Quoted 5 lines
Quoted 3 lines
It would be great if you mentioned issue scenario for status poll case
drivers/mtd/spi/sf_ops.c: ret = spi_xfer(spi, 8, NULL,
&status, 0);That breaks on
soft_spi, hence the patch.
OK - means issue only with soft_spi.c is it?
Can you share the issue log or typical use case scenario w.r.t soft_spi.c, I need to understand how this got resolved with your change.
I understand you assigned '0' when dout is NULL and you took the buf only when din is !NULL.
thanks!
Signature · 1 line
Jagan.
PATCH 1/1 spi: soft_spi: Support NULL din/dout buffers
Quoted 4 lines · Jagan Teki
Quoted 3 lines
Quoted 3 lines
It would be great if you mentioned issue scenario for status poll case
drivers/mtd/spi/sf_ops.c: ret = spi_xfer(spi, 8, NULL,
&status, 0);OK - means issue only with
soft_spi.cis it?
Yes, and a couple other drivers.
Quoted 2 lines
Can you share the issue log or typical use case scenario w.r.t
soft_spi.c, I need to understand how this got resolved with your change.
Yes, you actually posted one such case that will not work "correctly" with soft_spi. In the line of code you posted above, soft_spi will actually perform a read from address 0x0. In most cases, the read side isn't a huge deal, but on the write side it can cause all kinds of surprises.
Quoted 2 lines
I understand you assigned '0' when dout is NULL and you took the buf only when din is !NULL.
Yes, just handling NULL case to be how most drivers are handling it and how apparently most users of the spi modules (like mtd/spi/sf_ops) are clearly expecting it to work.
- Andy
PATCH 1/1 spi: soft_spi: Support NULL din/dout buffers
Quoted 17 lines · Andrew Ruder
Quoted 4 lines · Jagan Teki
Quoted 3 lines
Quoted 3 lines
It would be great if you mentioned issue scenario for status poll case
drivers/mtd/spi/sf_ops.c: ret = spi_xfer(spi, 8, NULL,
&status, 0);OK - means issue only with
soft_spi.cis it?Yes, and a couple other drivers.
Quoted 2 lines
Can you share the issue log or typical use case scenario w.r.t
soft_spi.c, I need to understand how this got resolved with your change.Yes, you actually posted one such case that will not work "correctly" with
soft_spi. In the line of code you posted above,soft_spiwill actually perform a read from address0x0. In most cases, the read side isn't a huge deal, but on the write side it can cause all kinds of surprises.
Quoted 6 lines
Quoted 2 lines
I understand you assigned '0' when dout is NULL and you took the buf only when din is !NULL.
Yes, just handling NULL case to be how most drivers are handling it and how apparently most users of the spi modules (like mtd/spi/sf_ops) are clearly expecting it to work.
I saw some check-patch issues, can you fix - plan to apply.
thanks!
Signature · 1 line
Jagan.