On Tue, 7 Aug 2018 14:16:52 +0200 Stefan Roese sr@denx.de wrote:
Some SPI controller do not support full-duplex SPI transfers. This patch changes the SPI transfer into 2 separate transfers - or 1, if no data is to transmitted.
With this change, no buffers need to be allocated anymore. We use the TX and RX buffers that are passed to spi_mem_exec_op() directly.
Signed-off-by: Stefan Roese sr@denx.de Suggested-by: Boris Brezillon boris.brezillon@bootlin.com Cc: Miquel Raynal miquel.raynal@bootlin.com Cc: Boris Brezillon boris.brezillon@bootlin.com Cc: Jagan Teki jagan@openedev.com
Looks good overall, just a few comments (that you might chose to ignore if you disagree).
Reviewed-by: Boris Brezillon boris.brezillon@bootlin.com
v2:
- Replaces patch "spi: spi-mem: Add optional half-duplex SPI transfer mode" from first patchset version
- No compile-time option but the default to use 2 separate SPI messages to transfer the command and data
drivers/spi/spi-mem.c | 74 +++++++++++++++++++++++-------------------- 1 file changed, 39 insertions(+), 35 deletions(-)
diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c index 07ce799170..84e33aa979 100644 --- a/drivers/spi/spi-mem.c +++ b/drivers/spi/spi-mem.c @@ -15,6 +15,8 @@ #include <spi-mem.h> #endif
+#define OP_BUFFER_SIZE_MAX 16 /* Max size for cmd + addr + dummy */
#ifndef __UBOOT__ /**
- spi_controller_dma_map_mem_op_data() - DMA-map the buffer attached to a
@@ -200,8 +202,12 @@ int spi_mem_exec_op(struct spi_slave *slave, const struct spi_mem_op *op) bool tx_data = op->data.nbytes && (op->data.dir == SPI_MEM_DATA_OUT); struct udevice *bus = slave->dev->parent; struct dm_spi_ops *ops = spi_get_ops(bus);
- unsigned int xfer_len, pos = 0;
- u8 *tx_buf, *rx_buf = NULL;
- unsigned int pos = 0;
- const u8 *tx_buf;
- u8 *rx_buf;
- u8 op_buf[OP_BUFFER_SIZE_MAX];
u8 op_buf[OP_BUFFER_SIZE_MAX] = { };
and you can get rid of the memset(0) in the code.
- int op_len;
- u32 flag; int ret; int i;
@@ -330,67 +336,65 @@ int spi_mem_exec_op(struct spi_slave *slave, const struct spi_mem_op *op) return -ENOTSUPP; }
- xfer_len = sizeof(op->cmd.opcode) + op->addr.nbytes +
op->dummy.nbytes + op->data.nbytes;
- tx_buf = op->data.buf.out;
- rx_buf = op->data.buf.in;
I think you can get rid of rx/tx_data and just keep rx/tx_buf. Initialize them to NULL at declaration time and then do:
if (op->data.nbytes) { if (op->data->dir == SPI_MEM_DATA_IN) rx_buf = op->data.buf.in; else tx_buf = op->data.buf.out; }
- /*
* Allocate a buffer to transmit the CMD, ADDR cycles with kmalloc() so* we're guaranteed that this buffer is DMA-able, as required by the* SPI layer.*/- tx_buf = kzalloc(xfer_len, GFP_KERNEL);
- if (!tx_buf)
return -ENOMEM;- if (rx_data) {
rx_buf = kzalloc(xfer_len, GFP_KERNEL);if (!rx_buf)return -ENOMEM;
- op_len = sizeof(op->cmd.opcode) + op->addr.nbytes + op->dummy.nbytes;
- if (op_len > OP_BUFFER_SIZE_MAX) {
printf("Length for cmd+addr+dummy too big (%d)\n", op_len);return -EIO;
While I agree we shouldn't exceed the 16 bytes for cmd, addr and dummy cycles, I'm not a big fan of those hardcoded limitations that needs to be adjusted every time we realize the initial choice was too restrictive.
Do you have a good reason for avoiding kzalloc() here (perfs, dynamic allocation not available in some cases?)?
memset(op_buf, 0x00, op_len);
ret = spi_claim_bus(slave); if (ret < 0) return ret;
- tx_buf[pos++] = op->cmd.opcode;
op_buf[pos++] = op->cmd.opcode;
if (op->addr.nbytes) { for (i = 0; i < op->addr.nbytes; i++)
tx_buf[pos + i] = op->addr.val >>(8 * (op->addr.nbytes - i - 1));
op_buf[pos + i] = op->addr.val >>(8 * (op->addr.nbytes - i - 1));pos += op->addr.nbytes; }
- if (op->dummy.nbytes) {
memset(tx_buf + pos, 0xff, op->dummy.nbytes);pos += op->dummy.nbytes;
- if (op->dummy.nbytes)
memset(op_buf + pos, 0xff, op->dummy.nbytes);- /* 1st transfer: opcode + address + dummy cycles */
- flag = SPI_XFER_BEGIN;
- /* Make sure to set END bit if no tx or rx data messages follow */
- if (!tx_data && !rx_data)
flag |= SPI_XFER_END;
I'd add a blank line here.
- ret = spi_xfer(slave, op_len * 8, op_buf, NULL, flag);
You should check ret here.
- if (tx_data) {
/* 2nd transfer a: tx data path */ret = spi_xfer(slave, op->data.nbytes * 8, tx_buf, NULL,SPI_XFER_END);
and here
}
- if (tx_data)
memcpy(tx_buf + pos, op->data.buf.out, op->data.nbytes);
- if (rx_data) {
/* 2nd transfer b: rx data path */ret = spi_xfer(slave, op->data.nbytes * 8, NULL, rx_buf,SPI_XFER_END);
and here
- }
If you've initialized rx/tx_buf as suggested above, you can do:
if (tx_buf || rx_buf) { ret = spi_xfer(slave, op->data.nbytes * 8, tx_buf, rx_buf, SPI_XFER_END); if (ret) return ret; }
ret = spi_xfer(slave, xfer_len * 8, tx_buf, rx_buf,
SPI_XFER_BEGIN | SPI_XFER_END);spi_release_bus(slave);
for (i = 0; i < pos; i++)
debug("%02x ", tx_buf[i]);
debug("%02x ", op_buf[i]);
- for (; i < xfer_len; i++)
for (i = 0; i < op->data.nbytes; i++) debug("%02x ", tx_data ? tx_buf[i] : rx_buf[i]); debug("[ret %d]\n", ret);
if (ret < 0) return ret;
- if (rx_data)
memcpy(op->data.buf.in, rx_buf + pos, op->data.nbytes);- kfree(tx_buf);
- kfree(rx_buf);
#endif /* __UBOOT__ */
return 0;