[PATCH v2 0/2] Add SPI support for Diolan DLN2

[PATCH v2 0/2] Add SPI support for Diolan DLN2

[Laurentiu Palcu]

Changes in v2:
 * fixed an indentation issue;
 * removed redundant call to spi_master_put() in _remove();

Hi,

This patchset adds support for the SPI module of the Diolan DLN2 USB to
I2C/SPI/GPIO bridge [1].

This patch depends on another patchset, [1], not yet merged, which adds support
for the other two modules: I2C and GPIO.

[1] http://www.diolan.com/products/dln2/features.html
[2] http://marc.info/?l=linux-kernel&m=141528172903850&w=2

laurentiu

Laurentiu Palcu (2):
  spi: add support for DLN-2 USB-SPI adapter
  mfd: dln2: add support for USB-SPI module

 drivers/mfd/dln2.c     |  12 +
 drivers/spi/Kconfig    |  10 +
 drivers/spi/Makefile   |   1 +
 drivers/spi/spi-dln2.c | 793 +++++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 816 insertions(+)
 create mode 100644 drivers/spi/spi-dln2.c

-- 
1.9.1

[PATCH v2 1/2] spi: add support for DLN-2 USB-SPI adapter

[Laurentiu Palcu]

This adds support for Diolan DLN2 USB-SPI adapter.

Information about the USB protocol interface can be found in the
Programmer's Reference Manual [1], see section 5.4.6 for the SPI
master module commands and responses.

[1] https://www.diolan.com/downloads/dln-api-manual.pdf

Signed-off-by: Laurentiu Palcu <laurentiu.palcu@intel.com>
---
 drivers/spi/Kconfig    |  10 +
 drivers/spi/Makefile   |   1 +
 drivers/spi/spi-dln2.c | 793 +++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 804 insertions(+)
 create mode 100644 drivers/spi/spi-dln2.c

diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index 62e2242..a52a910 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -176,6 +176,16 @@ config SPI_DAVINCI
 	help
 	  SPI master controller for DaVinci/DA8x/OMAP-L/AM1x SPI modules.
 
+config SPI_DLN2
+       tristate "Diolan DLN-2 USB SPI adapter"
+       depends on MFD_DLN2
+       help
+         If you say yes to this option, support will be included for Diolan
+         DLN2, a USB to SPI interface.
+
+         This driver can also be built as a module.  If so, the module
+         will be called spi-dln2.
+
 config SPI_EFM32
 	tristate "EFM32 SPI controller"
 	depends on OF && ARM && (ARCH_EFM32 || COMPILE_TEST)
diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
index 762da07..b315da2 100644
--- a/drivers/spi/Makefile
+++ b/drivers/spi/Makefile
@@ -26,6 +26,7 @@ obj-$(CONFIG_SPI_CADENCE)		+= spi-cadence.o
 obj-$(CONFIG_SPI_CLPS711X)		+= spi-clps711x.o
 obj-$(CONFIG_SPI_COLDFIRE_QSPI)		+= spi-coldfire-qspi.o
 obj-$(CONFIG_SPI_DAVINCI)		+= spi-davinci.o
+obj-$(CONFIG_SPI_DLN2)			+= spi-dln2.o
 obj-$(CONFIG_SPI_DESIGNWARE)		+= spi-dw.o
 obj-$(CONFIG_SPI_DW_MMIO)		+= spi-dw-mmio.o
 obj-$(CONFIG_SPI_DW_PCI)		+= spi-dw-midpci.o
diff --git a/drivers/spi/spi-dln2.c b/drivers/spi/spi-dln2.c
new file mode 100644
index 0000000..8277294
--- /dev/null
+++ b/drivers/spi/spi-dln2.c
@@ -0,0 +1,793 @@
+/*
+ * Driver for the Diolan DLN-2 USB-SPI adapter
+ *
+ * Copyright (c) 2014 Intel Corporation
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation, version 2.
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/mfd/dln2.h>
+#include <linux/spi/spi.h>
+
+#define DLN2_SPI_MODULE_ID		0x02
+#define DLN2_SPI_CMD(cmd)		DLN2_CMD(cmd, DLN2_SPI_MODULE_ID)
+
+/* SPI commands */
+#define DLN2_SPI_GET_PORT_COUNT			DLN2_SPI_CMD(0x00)
+#define DLN2_SPI_ENABLE				DLN2_SPI_CMD(0x11)
+#define DLN2_SPI_DISABLE			DLN2_SPI_CMD(0x12)
+#define DLN2_SPI_IS_ENABLED			DLN2_SPI_CMD(0x13)
+#define DLN2_SPI_SET_MODE			DLN2_SPI_CMD(0x14)
+#define DLN2_SPI_GET_MODE			DLN2_SPI_CMD(0x15)
+#define DLN2_SPI_SET_FRAME_SIZE			DLN2_SPI_CMD(0x16)
+#define DLN2_SPI_GET_FRAME_SIZE			DLN2_SPI_CMD(0x17)
+#define DLN2_SPI_SET_FREQUENCY			DLN2_SPI_CMD(0x18)
+#define DLN2_SPI_GET_FREQUENCY			DLN2_SPI_CMD(0x19)
+#define DLN2_SPI_READ_WRITE			DLN2_SPI_CMD(0x1A)
+#define DLN2_SPI_READ				DLN2_SPI_CMD(0x1B)
+#define DLN2_SPI_WRITE				DLN2_SPI_CMD(0x1C)
+#define DLN2_SPI_SET_DELAY_BETWEEN_SS		DLN2_SPI_CMD(0x20)
+#define DLN2_SPI_GET_DELAY_BETWEEN_SS		DLN2_SPI_CMD(0x21)
+#define DLN2_SPI_SET_DELAY_AFTER_SS		DLN2_SPI_CMD(0x22)
+#define DLN2_SPI_GET_DELAY_AFTER_SS		DLN2_SPI_CMD(0x23)
+#define DLN2_SPI_SET_DELAY_BETWEEN_FRAMES	DLN2_SPI_CMD(0x24)
+#define DLN2_SPI_GET_DELAY_BETWEEN_FRAMES	DLN2_SPI_CMD(0x25)
+#define DLN2_SPI_SET_SS				DLN2_SPI_CMD(0x26)
+#define DLN2_SPI_GET_SS				DLN2_SPI_CMD(0x27)
+#define DLN2_SPI_RELEASE_SS			DLN2_SPI_CMD(0x28)
+#define DLN2_SPI_SS_VARIABLE_ENABLE		DLN2_SPI_CMD(0x2B)
+#define DLN2_SPI_SS_VARIABLE_DISABLE		DLN2_SPI_CMD(0x2C)
+#define DLN2_SPI_SS_VARIABLE_IS_ENABLED		DLN2_SPI_CMD(0x2D)
+#define DLN2_SPI_SS_AAT_ENABLE			DLN2_SPI_CMD(0x2E)
+#define DLN2_SPI_SS_AAT_DISABLE			DLN2_SPI_CMD(0x2F)
+#define DLN2_SPI_SS_AAT_IS_ENABLED		DLN2_SPI_CMD(0x30)
+#define DLN2_SPI_SS_BETWEEN_FRAMES_ENABLE	DLN2_SPI_CMD(0x31)
+#define DLN2_SPI_SS_BETWEEN_FRAMES_DISABLE	DLN2_SPI_CMD(0x32)
+#define DLN2_SPI_SS_BETWEEN_FRAMES_IS_ENABLED	DLN2_SPI_CMD(0x33)
+#define DLN2_SPI_SET_CPHA			DLN2_SPI_CMD(0x34)
+#define DLN2_SPI_GET_CPHA			DLN2_SPI_CMD(0x35)
+#define DLN2_SPI_SET_CPOL			DLN2_SPI_CMD(0x36)
+#define DLN2_SPI_GET_CPOL			DLN2_SPI_CMD(0x37)
+#define DLN2_SPI_SS_MULTI_ENABLE		DLN2_SPI_CMD(0x38)
+#define DLN2_SPI_SS_MULTI_DISABLE		DLN2_SPI_CMD(0x39)
+#define DLN2_SPI_SS_MULTI_IS_ENABLED		DLN2_SPI_CMD(0x3A)
+#define DLN2_SPI_GET_SUPPORTED_MODES		DLN2_SPI_CMD(0x40)
+#define DLN2_SPI_GET_SUPPORTED_CPHA_VALUES	DLN2_SPI_CMD(0x41)
+#define DLN2_SPI_GET_SUPPORTED_CPOL_VALUES	DLN2_SPI_CMD(0x42)
+#define DLN2_SPI_GET_SUPPORTED_FRAME_SIZES	DLN2_SPI_CMD(0x43)
+#define DLN2_SPI_GET_SS_COUNT			DLN2_SPI_CMD(0x44)
+#define DLN2_SPI_GET_MIN_FREQUENCY		DLN2_SPI_CMD(0x45)
+#define DLN2_SPI_GET_MAX_FREQUENCY		DLN2_SPI_CMD(0x46)
+#define DLN2_SPI_GET_MIN_DELAY_BETWEEN_SS	DLN2_SPI_CMD(0x47)
+#define DLN2_SPI_GET_MAX_DELAY_BETWEEN_SS	DLN2_SPI_CMD(0x48)
+#define DLN2_SPI_GET_MIN_DELAY_AFTER_SS		DLN2_SPI_CMD(0x49)
+#define DLN2_SPI_GET_MAX_DELAY_AFTER_SS		DLN2_SPI_CMD(0x4A)
+#define DLN2_SPI_GET_MIN_DELAY_BETWEEN_FRAMES	DLN2_SPI_CMD(0x4B)
+#define DLN2_SPI_GET_MAX_DELAY_BETWEEN_FRAMES	DLN2_SPI_CMD(0x4C)
+
+#define DLN2_SPI_MAX_XFER_SIZE			256
+#define DLN2_SPI_BUF_SIZE			(DLN2_SPI_MAX_XFER_SIZE + 16)
+#define DLN2_SPI_ATTR_LEAVE_SS_LOW		BIT(0)
+#define DLN2_TRANSFERS_WAIT_COMPLETE		1
+#define DLN2_TRANSFERS_CANCEL			0
+
+struct dln2_spi {
+	struct platform_device *pdev;
+	struct spi_master *master;
+	u8 port;
+
+	void *buf;
+
+	u8 bpw;
+	u32 speed;
+	u16 mode;
+	u8 cs;
+};
+
+/*
+ * Enable/Disable SPI module. The disable command will wait for transfers to
+ * complete first.
+ */
+static int dln2_spi_enable(struct dln2_spi *dln2, bool enable)
+{
+	int ret;
+	u16 cmd;
+	struct {
+		u8 port;
+		u8 wait_for_completion;
+	} __packed tx;
+	unsigned len = sizeof(tx);
+
+	tx.port = dln2->port;
+
+	if (enable) {
+		cmd = DLN2_SPI_ENABLE;
+		len -= sizeof(tx.wait_for_completion);
+	} else {
+		tx.wait_for_completion = DLN2_TRANSFERS_WAIT_COMPLETE;
+		cmd = DLN2_SPI_DISABLE;
+	}
+
+	ret = dln2_transfer_tx(dln2->pdev, cmd, &tx, len);
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+
+/*
+ * Select/unselect multiple CS lines. The selected lines will be automatically
+ * toggled LOW/HIGH by the board firmware during transfers, provided they're
+ * enabled first.
+ *
+ * Ex: cs_mask = 0x03 -> CS0 & CS1 will be selected and the next WR/RD operation
+ *                       will toggle the lines LOW/HIGH automatically.
+ */
+static int dln2_spi_cs_set(struct dln2_spi *dln2, u8 cs_mask)
+{
+	struct {
+		u8 port;
+		u8 cs;
+	} __packed tx;
+
+	tx.port = dln2->port;
+
+	/* According to Diolan docs, "a slave device can be selected by changing
+	 * the corresponding bit value to 0". The rest must be set to 1. Hence
+	 * the bitwise NOT in front.
+	 */
+	tx.cs = ~cs_mask;
+
+	return dln2_transfer_tx(dln2->pdev, DLN2_SPI_SET_SS, &tx, sizeof(tx));
+}
+
+/*
+ * Select one CS line. The other lines will be un-selected.
+ */
+static int dln2_spi_cs_set_one(struct dln2_spi *dln2, u8 cs)
+{
+	return dln2_spi_cs_set(dln2, BIT(cs));
+}
+
+/*
+ * Enable/disable CS lines for usage. The module has to be disabled first.
+ */
+static int dln2_spi_cs_enable(struct dln2_spi *dln2, u8 cs_mask, bool enable)
+{
+	struct {
+		u8 port;
+		u8 cs;
+	} __packed tx;
+	u16 cmd;
+
+	tx.port = dln2->port;
+	tx.cs = cs_mask;
+	cmd = enable ? DLN2_SPI_SS_MULTI_ENABLE : DLN2_SPI_SS_MULTI_DISABLE;
+
+	return dln2_transfer_tx(dln2->pdev, cmd, &tx, sizeof(tx));
+}
+
+static int dln2_spi_cs_enable_all(struct dln2_spi *dln2, bool enable)
+{
+	u8 cs_mask = GENMASK(dln2->master->num_chipselect - 1, 0);
+
+	return dln2_spi_cs_enable(dln2, cs_mask, enable);
+}
+
+static int dln2_spi_get_cs_num(struct dln2_spi *dln2, u16 *cs_num)
+{
+	int ret;
+	struct {
+		u8 port;
+	} tx;
+	struct {
+		__le16 cs_count;
+	} *rx = dln2->buf;
+	unsigned rx_len = sizeof(*rx);
+
+	tx.port = dln2->port;
+	ret = dln2_transfer(dln2->pdev, DLN2_SPI_GET_SS_COUNT, &tx, sizeof(tx),
+			    rx, &rx_len);
+	if (ret < 0)
+		return ret;
+	if (rx_len < sizeof(*rx))
+		return -EPROTO;
+
+	*cs_num = le16_to_cpu(rx->cs_count);
+
+	dev_dbg(&dln2->pdev->dev, "cs_num = %d\n", *cs_num);
+
+	return 0;
+}
+
+/*
+ * Get bus min/max frequencies.
+ */
+static int dln2_spi_get_speed_range(struct dln2_spi *dln2, u32 *fmin, u32 *fmax)
+{
+	int ret;
+	struct {
+		u8 port;
+	} tx;
+	struct {
+		__le32 speed;
+	} *rx = dln2->buf;
+	unsigned rx_len = sizeof(*rx);
+	int cmd[2] = {DLN2_SPI_GET_MIN_FREQUENCY, DLN2_SPI_GET_MAX_FREQUENCY};
+	u32 *speed[2] = {fmin, fmax};
+	int i;
+
+	tx.port = dln2->port;
+
+	for (i = 0; i < 2; i++) {
+		ret = dln2_transfer(dln2->pdev, cmd[i], &tx, sizeof(tx),
+				    rx, &rx_len);
+		if (ret < 0)
+			return ret;
+		if (rx_len < sizeof(*rx))
+			return -EPROTO;
+
+		*speed[i] = le32_to_cpu(rx->speed);
+	}
+
+	dev_dbg(&dln2->pdev->dev, "freq_min = %d, freq_max = %d\n",
+		*fmin, *fmax);
+
+	return 0;
+}
+
+/*
+ * Set the bus speed. The module will automatically round down to the closest
+ * available frequency and returns it. The module has to be disabled first.
+ */
+static int dln2_spi_set_speed(struct dln2_spi *dln2, u32 speed,
+			      u32 *probed_speed)
+{
+	int ret;
+	struct {
+		u8 port;
+		__le32 speed;
+	} __packed tx;
+	struct {
+		__le32 speed;
+	} __packed *rx = dln2->buf;
+	int rx_len = sizeof(*rx);
+
+	tx.port = dln2->port;
+	tx.speed = cpu_to_le32(speed);
+
+	ret = dln2_transfer(dln2->pdev, DLN2_SPI_SET_FREQUENCY, &tx, sizeof(tx),
+			    rx, &rx_len);
+	if (ret < 0)
+		return ret;
+	if (rx_len < sizeof(*rx))
+		return -EPROTO;
+
+	if (probed_speed)
+		*probed_speed = le32_to_cpu(rx->speed);
+
+	return 0;
+}
+
+/*
+ * Change CPOL & CPHA. The module has to be disabled first.
+ */
+static int dln2_spi_set_mode(struct dln2_spi *dln2, u8 mode)
+{
+	struct {
+		u8 port;
+		u8 mode;
+	} __packed tx;
+
+	tx.port = dln2->port;
+	tx.mode = mode;
+
+	return dln2_transfer_tx(dln2->pdev, DLN2_SPI_SET_MODE, &tx, sizeof(tx));
+}
+
+/*
+ * Change frame size. The module has to be disabled first.
+ */
+static int dln2_spi_set_bpw(struct dln2_spi *dln2, u8 bpw)
+{
+	struct {
+		u8 port;
+		u8 bpw;
+	} __packed tx;
+
+	tx.port = dln2->port;
+	tx.bpw = bpw;
+
+	return dln2_transfer_tx(dln2->pdev, DLN2_SPI_SET_FRAME_SIZE,
+				&tx, sizeof(tx));
+}
+
+static int dln2_spi_get_supported_frame_sizes(struct dln2_spi *dln2,
+					      u32 *bpw_mask)
+{
+	int ret;
+	struct {
+		u8 port;
+	} tx;
+	struct {
+		u8 count;
+		u8 frame_sizes[36];
+	} __packed *rx = dln2->buf;
+	unsigned rx_len = sizeof(*rx);
+	int i;
+
+	tx.port = dln2->port;
+
+	ret = dln2_transfer(dln2->pdev, DLN2_SPI_GET_SUPPORTED_FRAME_SIZES,
+			    &tx, sizeof(tx), rx, &rx_len);
+	if (ret < 0)
+		return ret;
+	if (rx_len  < sizeof(*rx))
+		return -EPROTO;
+
+	*bpw_mask = 0;
+	for (i = 0; i < rx->count; i++)
+		*bpw_mask |= BIT(rx->frame_sizes[i] - 1);
+
+	dev_dbg(&dln2->pdev->dev, "bpw_mask = 0x%X\n", *bpw_mask);
+
+	return 0;
+}
+
+/*
+ * Copy the data to DLN2 buffer and change the alignment to LE, requested by
+ * DLN2 module. SPI core makes sure that the data length is a multiple of word
+ * size.
+ */
+static int dln2_spi_copy_to_buf(u8 *dln2_buf, const u8 *src, u16 len, u8 bpw)
+{
+#ifdef __LITTLE_ENDIAN
+	memcpy(dln2_buf, src, len);
+#else
+	if (bpw <= 8)
+		memcpy(dln2_buf, src, len);
+	else if (bpw <= 16) {
+		__le16 *d = (__le16 *) dln2_buf;
+		u16 *s = (u16 *) src;
+
+		len = len / 2;
+		while (len--)
+			*d++ = cpu_to_le16p(s++);
+	} else {
+		__le32 *d = (__le32 *) dln2_buf;
+		u32 *s = (u32 *) src;
+
+		len = len / 4;
+		while (len--)
+			*d++ = cpu_to_le32p(s++);
+	}
+#endif
+
+	return 0;
+}
+
+/*
+ * Copy the data from DLN2 buffer and convert to CPU alignment since the DLN2
+ * buffer is LE aligned. SPI core makes sure that the data length is a multiple
+ * of word size.
+ */
+static int dln2_spi_copy_from_buf(u8 *dest, const u8 *dln2_buf, u16 len, u8 bpw)
+{
+#ifdef __LITTLE_ENDIAN
+	memcpy(dest, dln2_buf, len);
+#else
+	if (bpw <= 8)
+		memcpy(dest, dln2_buf, len);
+	else if (bpw <= 16) {
+		u16 *d = (u16 *) dest;
+		__le16 *s = (__le16 *) dln2_buf;
+
+		len = len / 2;
+		while (len--)
+			*d++ = le16_to_cpup(s++);
+	} else {
+		u32 *d = (u32 *) dest;
+		__le32 *s = (__le32 *) dln2_buf;
+
+		len = len / 4;
+		while (len--)
+			*d++ = le32_to_cpup(s++);
+	}
+#endif
+
+	return 0;
+}
+
+/*
+ * Perform one write operation.
+ */
+static int dln2_spi_write_one(struct dln2_spi *dln2, const u8 *data,
+			      u16 data_len, u8 attr)
+{
+	struct {
+		u8 port;
+		__le16 size;
+		u8 attr;
+		u8 buf[DLN2_SPI_MAX_XFER_SIZE];
+	} __packed *tx = dln2->buf;
+	unsigned tx_len;
+
+	BUILD_BUG_ON(sizeof(*tx) > DLN2_SPI_BUF_SIZE);
+
+	if (data_len > DLN2_SPI_MAX_XFER_SIZE)
+		return -EINVAL;
+
+	tx->port = dln2->port;
+	tx->size = cpu_to_le16(data_len);
+	tx->attr = attr;
+
+	dln2_spi_copy_to_buf(tx->buf, data, data_len, dln2->bpw);
+
+	tx_len = sizeof(*tx) + data_len - DLN2_SPI_MAX_XFER_SIZE;
+	return dln2_transfer_tx(dln2->pdev, DLN2_SPI_WRITE, tx, tx_len);
+}
+
+/*
+ * Perform one read operation.
+ */
+static int dln2_spi_read_one(struct dln2_spi *dln2, u8 *data,
+			     u16 data_len, u8 attr)
+{
+	int ret;
+	struct {
+		u8 port;
+		__le16 size;
+		u8 attr;
+	} __packed tx;
+	struct {
+		__le16 size;
+		u8 buf[DLN2_SPI_MAX_XFER_SIZE];
+	} __packed *rx = dln2->buf;
+	unsigned rx_len = sizeof(*rx);
+
+	BUILD_BUG_ON(sizeof(*rx) > DLN2_SPI_BUF_SIZE);
+
+	if (data_len > DLN2_SPI_MAX_XFER_SIZE)
+		return -EINVAL;
+
+	tx.port = dln2->port;
+	tx.size = cpu_to_le16(data_len);
+	tx.attr = attr;
+
+	ret = dln2_transfer(dln2->pdev, DLN2_SPI_READ, &tx, sizeof(tx),
+			    rx, &rx_len);
+	if (ret < 0)
+		return ret;
+	if (rx_len < sizeof(rx->size) + data_len)
+		return -EPROTO;
+	if (le16_to_cpu(rx->size) != data_len)
+		return -EPROTO;
+
+	dln2_spi_copy_from_buf(data, rx->buf, data_len, dln2->bpw);
+
+	return 0;
+}
+
+/*
+ * Perform one write & read operation.
+ */
+static int dln2_spi_read_write_one(struct dln2_spi *dln2, const u8 *tx_data,
+				   u8 *rx_data, u16 data_len, u8 attr)
+{
+	int ret;
+	struct {
+		u8 port;
+		__le16 size;
+		u8 attr;
+		u8 buf[DLN2_SPI_MAX_XFER_SIZE];
+	} __packed *tx;
+	struct {
+		__le16 size;
+		u8 buf[DLN2_SPI_MAX_XFER_SIZE];
+	} __packed *rx;
+	unsigned tx_len, rx_len;
+
+	BUILD_BUG_ON(sizeof(*tx) > DLN2_SPI_BUF_SIZE ||
+		     sizeof(*rx) > DLN2_SPI_BUF_SIZE);
+
+	if (data_len > DLN2_SPI_MAX_XFER_SIZE)
+		return -EINVAL;
+
+	/* Since this is a pseudo full-duplex communication, we're perfectly
+	 * safe to use the same buffer for both tx and rx. When DLN2 sends the
+	 * response back, with the rx data, we don't need the tx buffer anymore.
+	 */
+	tx = dln2->buf;
+	rx = dln2->buf;
+
+	tx->port = dln2->port;
+	tx->size = cpu_to_le16(data_len);
+	tx->attr = attr;
+
+	dln2_spi_copy_to_buf(tx->buf, tx_data, data_len, dln2->bpw);
+
+	tx_len = sizeof(*tx) + data_len - DLN2_SPI_MAX_XFER_SIZE;
+	rx_len = sizeof(*rx);
+
+	ret = dln2_transfer(dln2->pdev, DLN2_SPI_READ_WRITE, tx, tx_len,
+			    rx, &rx_len);
+	if (ret < 0)
+		return ret;
+	if (rx_len < sizeof(rx->size) + data_len)
+		return -EPROTO;
+	if (le16_to_cpu(rx->size) != data_len)
+		return -EPROTO;
+
+	dln2_spi_copy_from_buf(rx_data, rx->buf, data_len, dln2->bpw);
+
+	return 0;
+}
+
+/*
+ * Read/Write wrapper. It will automatically split an operation into multiple
+ * single ones due to device buffer constraints.
+ */
+static int dln2_spi_rdwr(struct dln2_spi *dln2, const u8 *tx_data,
+			 u8 *rx_data, u16 data_len, u8 attr) {
+	int ret;
+	u16 len;
+	u8 temp_attr;
+	u16 remaining = data_len;
+	u16 offset;
+
+	do {
+		if (remaining > DLN2_SPI_MAX_XFER_SIZE) {
+			len = DLN2_SPI_MAX_XFER_SIZE;
+			temp_attr = DLN2_SPI_ATTR_LEAVE_SS_LOW;
+		} else {
+			len = remaining;
+			temp_attr = attr;
+		}
+
+		offset = data_len - remaining;
+
+		if (tx_data && rx_data)
+			ret = dln2_spi_read_write_one(dln2,
+						      tx_data + offset,
+						      rx_data + offset,
+						      len, temp_attr);
+		else if (tx_data)
+			ret = dln2_spi_write_one(dln2,
+						 tx_data + offset,
+						 len, temp_attr);
+		else if (rx_data)
+			ret = dln2_spi_read_one(dln2,
+						rx_data + offset,
+						len, temp_attr);
+		else
+			return -EINVAL;
+
+		if (ret < 0)
+			return ret;
+
+		remaining -= len;
+	} while (remaining);
+
+	return 0;
+}
+
+static int dln2_spi_prepare_message(struct spi_master *master,
+				    struct spi_message *message)
+{
+	int ret;
+	struct dln2_spi *dln2 = spi_master_get_devdata(master);
+	struct spi_device *spi = message->spi;
+
+	if (dln2->cs != spi->chip_select) {
+		ret = dln2_spi_cs_set_one(dln2, spi->chip_select);
+		if (ret < 0)
+			return ret;
+
+		dln2->cs = spi->chip_select;
+	}
+
+	return 0;
+}
+
+static int dln2_spi_transfer_setup(struct dln2_spi *dln2, u32 speed,
+				   u8 bpw, u8 mode)
+{
+	int ret;
+	bool bus_setup_change;
+
+	bus_setup_change = dln2->speed != speed || dln2->mode != mode ||
+			   dln2->bpw != bpw;
+
+	if (!bus_setup_change)
+		return 0;
+
+	ret = dln2_spi_enable(dln2, false);
+	if (ret < 0)
+		return ret;
+
+	if (dln2->speed != speed) {
+		ret = dln2_spi_set_speed(dln2, speed, NULL);
+		if (ret < 0)
+			return ret;
+
+		dln2->speed = speed;
+	}
+
+	if (dln2->mode != mode) {
+		ret = dln2_spi_set_mode(dln2, mode & 0x3);
+		if (ret < 0)
+			return ret;
+
+		dln2->mode = mode;
+	}
+
+	if (dln2->bpw != bpw) {
+		ret = dln2_spi_set_bpw(dln2, bpw);
+		if (ret < 0)
+			return ret;
+
+		dln2->bpw = bpw;
+	}
+
+	ret = dln2_spi_enable(dln2, true);
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+
+static int dln2_spi_transfer_one_message(struct spi_master *master,
+					 struct spi_message *msg)
+{
+	struct dln2_spi *dln2 = spi_master_get_devdata(master);
+	struct spi_device *spi = msg->spi;
+	struct spi_transfer *xfer;
+	int status;
+
+	list_for_each_entry(xfer, &msg->transfers, transfer_list) {
+		u8 attr = 0;
+
+		status = dln2_spi_transfer_setup(dln2, xfer->speed_hz,
+						 xfer->bits_per_word,
+						 spi->mode);
+		if (status < 0) {
+			dev_err(&dln2->pdev->dev, "Cannot setup transfer\n");
+			break;
+		}
+
+		if (!xfer->cs_change &&
+		    !list_is_last(&xfer->transfer_list, &msg->transfers))
+			attr = xfer->cs_change ? 0 : DLN2_SPI_ATTR_LEAVE_SS_LOW;
+
+		status = dln2_spi_rdwr(dln2, xfer->tx_buf, xfer->rx_buf,
+				       xfer->len, attr);
+		if (status < 0) {
+			dev_err(&dln2->pdev->dev, "write/read failed!\n");
+			break;
+		}
+
+		status = 0;
+	}
+
+	msg->status = status;
+	spi_finalize_current_message(master);
+	return status;
+}
+
+static int dln2_spi_probe(struct platform_device *pdev)
+{
+	struct spi_master *master;
+	struct dln2_spi *dln2;
+	struct dln2_platform_data *pdata = dev_get_platdata(&pdev->dev);
+	int ret;
+
+	master = spi_alloc_master(&pdev->dev, sizeof(*dln2));
+	if (!master)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, master);
+
+	dln2 = spi_master_get_devdata(master);
+
+	dln2->buf = devm_kmalloc(&pdev->dev, DLN2_SPI_BUF_SIZE, GFP_KERNEL);
+	if (!dln2->buf) {
+		ret = -ENOMEM;
+		goto exit_free_master;
+	}
+
+	dln2->master = master;
+	dln2->pdev = pdev;
+	dln2->port = pdata->port;
+	/* cs number can never be 0xff, so the first transfer will set the cs */
+	dln2->cs = 0xff;
+
+	/* disable SPI module before continuing with the setup */
+	ret = dln2_spi_enable(dln2, false);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "Failed to disable SPI module\n");
+		goto exit_free_master;
+	}
+
+	ret = dln2_spi_get_cs_num(dln2, &master->num_chipselect);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "Failed to get number of CS pins\n");
+		goto exit_free_master;
+	}
+
+	ret = dln2_spi_get_speed_range(dln2,
+				       &master->min_speed_hz,
+				       &master->max_speed_hz);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "Failed to read bus min/max freqs\n");
+		goto exit_free_master;
+	}
+
+	ret = dln2_spi_get_supported_frame_sizes(dln2,
+						 &master->bits_per_word_mask);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "Failed to read supported frame sizes\n");
+		goto exit_free_master;
+	}
+
+	ret = dln2_spi_cs_enable_all(dln2, true);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "Failed to enable CS pins\n");
+		goto exit_free_master;
+	}
+
+	master->bus_num = -1;
+	master->mode_bits = SPI_CPOL | SPI_CPHA;
+	master->prepare_message = dln2_spi_prepare_message;
+	master->transfer_one_message = dln2_spi_transfer_one_message;
+
+	/* enable SPI module, we're good to go */
+	ret = dln2_spi_enable(dln2, true);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "Failed to enable SPI module\n");
+		goto exit_free_master;
+	}
+
+	ret = devm_spi_register_master(&pdev->dev, master);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "Failed to register master\n");
+		goto exit_register;
+	}
+
+	return ret;
+
+exit_register:
+	if (dln2_spi_enable(dln2, false) < 0)
+		dev_err(&pdev->dev, "Failed to disable SPI module\n");
+exit_free_master:
+	spi_master_put(master);
+
+	return ret;
+}
+
+static int dln2_spi_remove(struct platform_device *pdev)
+{
+	struct spi_master *master = spi_master_get(platform_get_drvdata(pdev));
+	struct dln2_spi *dln2 = spi_master_get_devdata(master);
+
+	if (dln2_spi_enable(dln2, false) < 0)
+		dev_err(&pdev->dev, "Failed to disable SPI module\n");
+
+	return 0;
+}
+
+static struct platform_driver spi_dln2_driver = {
+	.driver.name	= "dln2-spi",
+	.probe		= dln2_spi_probe,
+	.remove		= dln2_spi_remove,
+};
+module_platform_driver(spi_dln2_driver);
+
+MODULE_DESCRIPTION("Driver for the Diolan DLN2 SPI master interface");
+MODULE_AUTHOR("Laurentiu Palcu <laurentiu.palcu@intel.com>");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:dln2-spi");
-- 
1.9.1

Re: [PATCH v2 1/2] spi: add support for DLN-2 USB-SPI adapter

[Johan Havold]

On Wed, Nov 12, 2014 at 03:38:09PM +0200, Laurentiu Palcu wrote:
> This adds support for Diolan DLN2 USB-SPI adapter.
> 
> Information about the USB protocol interface can be found in the
> Programmer's Reference Manual [1], see section 5.4.6 for the SPI
> master module commands and responses.
> 
> [1] https://www.diolan.com/downloads/dln-api-manual.pdf
> 
> Signed-off-by: Laurentiu Palcu <laurentiu.palcu@intel.com>
> ---
>  drivers/spi/Kconfig    |  10 +
>  drivers/spi/Makefile   |   1 +
>  drivers/spi/spi-dln2.c | 793 +++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 804 insertions(+)
>  create mode 100644 drivers/spi/spi-dln2.c
> 
> diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
> index 62e2242..a52a910 100644
> --- a/drivers/spi/Kconfig
> +++ b/drivers/spi/Kconfig
> @@ -176,6 +176,16 @@ config SPI_DAVINCI
>  	help
>  	  SPI master controller for DaVinci/DA8x/OMAP-L/AM1x SPI modules.
>  
> +config SPI_DLN2
> +       tristate "Diolan DLN-2 USB SPI adapter"
> +       depends on MFD_DLN2
> +       help
> +         If you say yes to this option, support will be included for Diolan
> +         DLN2, a USB to SPI interface.
> +
> +         This driver can also be built as a module.  If so, the module
> +         will be called spi-dln2.
> +
>  config SPI_EFM32
>  	tristate "EFM32 SPI controller"
>  	depends on OF && ARM && (ARCH_EFM32 || COMPILE_TEST)
> diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
> index 762da07..b315da2 100644
> --- a/drivers/spi/Makefile
> +++ b/drivers/spi/Makefile
> @@ -26,6 +26,7 @@ obj-$(CONFIG_SPI_CADENCE)		+= spi-cadence.o
>  obj-$(CONFIG_SPI_CLPS711X)		+= spi-clps711x.o
>  obj-$(CONFIG_SPI_COLDFIRE_QSPI)		+= spi-coldfire-qspi.o
>  obj-$(CONFIG_SPI_DAVINCI)		+= spi-davinci.o
> +obj-$(CONFIG_SPI_DLN2)			+= spi-dln2.o
>  obj-$(CONFIG_SPI_DESIGNWARE)		+= spi-dw.o
>  obj-$(CONFIG_SPI_DW_MMIO)		+= spi-dw-mmio.o
>  obj-$(CONFIG_SPI_DW_PCI)		+= spi-dw-midpci.o
> diff --git a/drivers/spi/spi-dln2.c b/drivers/spi/spi-dln2.c
> new file mode 100644
> index 0000000..8277294
> --- /dev/null
> +++ b/drivers/spi/spi-dln2.c
> @@ -0,0 +1,793 @@
> +/*
> + * Driver for the Diolan DLN-2 USB-SPI adapter
> + *
> + * Copyright (c) 2014 Intel Corporation
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation, version 2.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/mfd/dln2.h>
> +#include <linux/spi/spi.h>
> +
> +#define DLN2_SPI_MODULE_ID		0x02
> +#define DLN2_SPI_CMD(cmd)		DLN2_CMD(cmd, DLN2_SPI_MODULE_ID)
> +
> +/* SPI commands */
> +#define DLN2_SPI_GET_PORT_COUNT			DLN2_SPI_CMD(0x00)
> +#define DLN2_SPI_ENABLE				DLN2_SPI_CMD(0x11)
> +#define DLN2_SPI_DISABLE			DLN2_SPI_CMD(0x12)
> +#define DLN2_SPI_IS_ENABLED			DLN2_SPI_CMD(0x13)
> +#define DLN2_SPI_SET_MODE			DLN2_SPI_CMD(0x14)
> +#define DLN2_SPI_GET_MODE			DLN2_SPI_CMD(0x15)
> +#define DLN2_SPI_SET_FRAME_SIZE			DLN2_SPI_CMD(0x16)
> +#define DLN2_SPI_GET_FRAME_SIZE			DLN2_SPI_CMD(0x17)
> +#define DLN2_SPI_SET_FREQUENCY			DLN2_SPI_CMD(0x18)
> +#define DLN2_SPI_GET_FREQUENCY			DLN2_SPI_CMD(0x19)
> +#define DLN2_SPI_READ_WRITE			DLN2_SPI_CMD(0x1A)
> +#define DLN2_SPI_READ				DLN2_SPI_CMD(0x1B)
> +#define DLN2_SPI_WRITE				DLN2_SPI_CMD(0x1C)
> +#define DLN2_SPI_SET_DELAY_BETWEEN_SS		DLN2_SPI_CMD(0x20)
> +#define DLN2_SPI_GET_DELAY_BETWEEN_SS		DLN2_SPI_CMD(0x21)
> +#define DLN2_SPI_SET_DELAY_AFTER_SS		DLN2_SPI_CMD(0x22)
> +#define DLN2_SPI_GET_DELAY_AFTER_SS		DLN2_SPI_CMD(0x23)
> +#define DLN2_SPI_SET_DELAY_BETWEEN_FRAMES	DLN2_SPI_CMD(0x24)
> +#define DLN2_SPI_GET_DELAY_BETWEEN_FRAMES	DLN2_SPI_CMD(0x25)
> +#define DLN2_SPI_SET_SS				DLN2_SPI_CMD(0x26)
> +#define DLN2_SPI_GET_SS				DLN2_SPI_CMD(0x27)
> +#define DLN2_SPI_RELEASE_SS			DLN2_SPI_CMD(0x28)
> +#define DLN2_SPI_SS_VARIABLE_ENABLE		DLN2_SPI_CMD(0x2B)
> +#define DLN2_SPI_SS_VARIABLE_DISABLE		DLN2_SPI_CMD(0x2C)
> +#define DLN2_SPI_SS_VARIABLE_IS_ENABLED		DLN2_SPI_CMD(0x2D)
> +#define DLN2_SPI_SS_AAT_ENABLE			DLN2_SPI_CMD(0x2E)
> +#define DLN2_SPI_SS_AAT_DISABLE			DLN2_SPI_CMD(0x2F)
> +#define DLN2_SPI_SS_AAT_IS_ENABLED		DLN2_SPI_CMD(0x30)
> +#define DLN2_SPI_SS_BETWEEN_FRAMES_ENABLE	DLN2_SPI_CMD(0x31)
> +#define DLN2_SPI_SS_BETWEEN_FRAMES_DISABLE	DLN2_SPI_CMD(0x32)
> +#define DLN2_SPI_SS_BETWEEN_FRAMES_IS_ENABLED	DLN2_SPI_CMD(0x33)
> +#define DLN2_SPI_SET_CPHA			DLN2_SPI_CMD(0x34)
> +#define DLN2_SPI_GET_CPHA			DLN2_SPI_CMD(0x35)
> +#define DLN2_SPI_SET_CPOL			DLN2_SPI_CMD(0x36)
> +#define DLN2_SPI_GET_CPOL			DLN2_SPI_CMD(0x37)
> +#define DLN2_SPI_SS_MULTI_ENABLE		DLN2_SPI_CMD(0x38)
> +#define DLN2_SPI_SS_MULTI_DISABLE		DLN2_SPI_CMD(0x39)
> +#define DLN2_SPI_SS_MULTI_IS_ENABLED		DLN2_SPI_CMD(0x3A)
> +#define DLN2_SPI_GET_SUPPORTED_MODES		DLN2_SPI_CMD(0x40)
> +#define DLN2_SPI_GET_SUPPORTED_CPHA_VALUES	DLN2_SPI_CMD(0x41)
> +#define DLN2_SPI_GET_SUPPORTED_CPOL_VALUES	DLN2_SPI_CMD(0x42)
> +#define DLN2_SPI_GET_SUPPORTED_FRAME_SIZES	DLN2_SPI_CMD(0x43)
> +#define DLN2_SPI_GET_SS_COUNT			DLN2_SPI_CMD(0x44)
> +#define DLN2_SPI_GET_MIN_FREQUENCY		DLN2_SPI_CMD(0x45)
> +#define DLN2_SPI_GET_MAX_FREQUENCY		DLN2_SPI_CMD(0x46)
> +#define DLN2_SPI_GET_MIN_DELAY_BETWEEN_SS	DLN2_SPI_CMD(0x47)
> +#define DLN2_SPI_GET_MAX_DELAY_BETWEEN_SS	DLN2_SPI_CMD(0x48)
> +#define DLN2_SPI_GET_MIN_DELAY_AFTER_SS		DLN2_SPI_CMD(0x49)
> +#define DLN2_SPI_GET_MAX_DELAY_AFTER_SS		DLN2_SPI_CMD(0x4A)
> +#define DLN2_SPI_GET_MIN_DELAY_BETWEEN_FRAMES	DLN2_SPI_CMD(0x4B)
> +#define DLN2_SPI_GET_MAX_DELAY_BETWEEN_FRAMES	DLN2_SPI_CMD(0x4C)
> +
> +#define DLN2_SPI_MAX_XFER_SIZE			256
> +#define DLN2_SPI_BUF_SIZE			(DLN2_SPI_MAX_XFER_SIZE + 16)
> +#define DLN2_SPI_ATTR_LEAVE_SS_LOW		BIT(0)
> +#define DLN2_TRANSFERS_WAIT_COMPLETE		1
> +#define DLN2_TRANSFERS_CANCEL			0
> +
> +struct dln2_spi {
> +	struct platform_device *pdev;
> +	struct spi_master *master;
> +	u8 port;
> +
> +	void *buf;

Add comment on what is protecting this buffer.

> +
> +	u8 bpw;
> +	u32 speed;
> +	u16 mode;
> +	u8 cs;
> +};
> +
> +/*
> + * Enable/Disable SPI module. The disable command will wait for transfers to
> + * complete first.
> + */
> +static int dln2_spi_enable(struct dln2_spi *dln2, bool enable)
> +{
> +	int ret;
> +	u16 cmd;
> +	struct {
> +		u8 port;
> +		u8 wait_for_completion;
> +	} __packed tx;

No need for __packed.

> +	unsigned len = sizeof(tx);
> +
> +	tx.port = dln2->port;
> +
> +	if (enable) {
> +		cmd = DLN2_SPI_ENABLE;
> +		len -= sizeof(tx.wait_for_completion);
> +	} else {
> +		tx.wait_for_completion = DLN2_TRANSFERS_WAIT_COMPLETE;
> +		cmd = DLN2_SPI_DISABLE;
> +	}
> +
> +	ret = dln2_transfer_tx(dln2->pdev, cmd, &tx, len);
> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +/*
> + * Select/unselect multiple CS lines. The selected lines will be automatically
> + * toggled LOW/HIGH by the board firmware during transfers, provided they're
> + * enabled first.
> + *
> + * Ex: cs_mask = 0x03 -> CS0 & CS1 will be selected and the next WR/RD operation

Seems you have several comments that wrap at >80 columns (81 above).

> + *                       will toggle the lines LOW/HIGH automatically.
> + */
> +static int dln2_spi_cs_set(struct dln2_spi *dln2, u8 cs_mask)
> +{
> +	struct {
> +		u8 port;
> +		u8 cs;
> +	} __packed tx;
> +

No __packed.

> +	tx.port = dln2->port;
> +
> +	/* According to Diolan docs, "a slave device can be selected by changing
> +	 * the corresponding bit value to 0". The rest must be set to 1. Hence
> +	 * the bitwise NOT in front.
> +	 */
> +	tx.cs = ~cs_mask;
> +
> +	return dln2_transfer_tx(dln2->pdev, DLN2_SPI_SET_SS, &tx, sizeof(tx));
> +}
> +
> +/*
> + * Select one CS line. The other lines will be un-selected.
> + */
> +static int dln2_spi_cs_set_one(struct dln2_spi *dln2, u8 cs)
> +{
> +	return dln2_spi_cs_set(dln2, BIT(cs));
> +}
> +
> +/*
> + * Enable/disable CS lines for usage. The module has to be disabled first.
> + */
> +static int dln2_spi_cs_enable(struct dln2_spi *dln2, u8 cs_mask, bool enable)
> +{
> +	struct {
> +		u8 port;
> +		u8 cs;
> +	} __packed tx;

No __packed.

> +	u16 cmd;
> +
> +	tx.port = dln2->port;
> +	tx.cs = cs_mask;
> +	cmd = enable ? DLN2_SPI_SS_MULTI_ENABLE : DLN2_SPI_SS_MULTI_DISABLE;
> +
> +	return dln2_transfer_tx(dln2->pdev, cmd, &tx, sizeof(tx));
> +}
> +
> +static int dln2_spi_cs_enable_all(struct dln2_spi *dln2, bool enable)
> +{
> +	u8 cs_mask = GENMASK(dln2->master->num_chipselect - 1, 0);
> +
> +	return dln2_spi_cs_enable(dln2, cs_mask, enable);
> +}
> +
> +static int dln2_spi_get_cs_num(struct dln2_spi *dln2, u16 *cs_num)
> +{
> +	int ret;
> +	struct {
> +		u8 port;
> +	} tx;
> +	struct {
> +		__le16 cs_count;
> +	} *rx = dln2->buf;

I don't think you want to use dln2->buf for all these small transfers.
And what would be protecting it from concurrent use?

Check this throughout.

> +	unsigned rx_len = sizeof(*rx);
> +
> +	tx.port = dln2->port;
> +	ret = dln2_transfer(dln2->pdev, DLN2_SPI_GET_SS_COUNT, &tx, sizeof(tx),
> +			    rx, &rx_len);
> +	if (ret < 0)
> +		return ret;
> +	if (rx_len < sizeof(*rx))
> +		return -EPROTO;
> +
> +	*cs_num = le16_to_cpu(rx->cs_count);
> +
> +	dev_dbg(&dln2->pdev->dev, "cs_num = %d\n", *cs_num);

Use the spi device for device messages throughout.

> +
> +	return 0;
> +}
> +
> +/*
> + * Get bus min/max frequencies.
> + */
> +static int dln2_spi_get_speed_range(struct dln2_spi *dln2, u32 *fmin, u32 *fmax)
> +{
> +	int ret;
> +	struct {
> +		u8 port;
> +	} tx;
> +	struct {
> +		__le32 speed;
> +	} *rx = dln2->buf;
> +	unsigned rx_len = sizeof(*rx);
> +	int cmd[2] = {DLN2_SPI_GET_MIN_FREQUENCY, DLN2_SPI_GET_MAX_FREQUENCY};
> +	u32 *speed[2] = {fmin, fmax};

Spaces after and before { and }.

> +	int i;
> +
> +	tx.port = dln2->port;
> +
> +	for (i = 0; i < 2; i++) {
> +		ret = dln2_transfer(dln2->pdev, cmd[i], &tx, sizeof(tx),
> +				    rx, &rx_len);
> +		if (ret < 0)
> +			return ret;
> +		if (rx_len < sizeof(*rx))
> +			return -EPROTO;
> +
> +		*speed[i] = le32_to_cpu(rx->speed);
> +	}

Add a helper to fetch a 32-bit value and call it twice instead of the
loop-construct above.

> +
> +	dev_dbg(&dln2->pdev->dev, "freq_min = %d, freq_max = %d\n",
> +		*fmin, *fmax);
> +
> +	return 0;
> +}
> +
> +/*
> + * Set the bus speed. The module will automatically round down to the closest
> + * available frequency and returns it. The module has to be disabled first.
> + */
> +static int dln2_spi_set_speed(struct dln2_spi *dln2, u32 speed,
> +			      u32 *probed_speed)
> +{
> +	int ret;
> +	struct {
> +		u8 port;
> +		__le32 speed;
> +	} __packed tx;
> +	struct {
> +		__le32 speed;
> +	} __packed *rx = dln2->buf;
> +	int rx_len = sizeof(*rx);
> +
> +	tx.port = dln2->port;
> +	tx.speed = cpu_to_le32(speed);
> +
> +	ret = dln2_transfer(dln2->pdev, DLN2_SPI_SET_FREQUENCY, &tx, sizeof(tx),
> +			    rx, &rx_len);
> +	if (ret < 0)
> +		return ret;
> +	if (rx_len < sizeof(*rx))
> +		return -EPROTO;
> +
> +	if (probed_speed)
> +		*probed_speed = le32_to_cpu(rx->speed);

You never use the probed_speed parameter in any call to this function.
Remove it if you don't need it.

> +
> +	return 0;
> +}
> +
> +/*
> + * Change CPOL & CPHA. The module has to be disabled first.
> + */
> +static int dln2_spi_set_mode(struct dln2_spi *dln2, u8 mode)
> +{
> +	struct {
> +		u8 port;
> +		u8 mode;
> +	} __packed tx;

No __packed.

> +
> +	tx.port = dln2->port;
> +	tx.mode = mode;
> +
> +	return dln2_transfer_tx(dln2->pdev, DLN2_SPI_SET_MODE, &tx, sizeof(tx));
> +}
> +
> +/*
> + * Change frame size. The module has to be disabled first.
> + */
> +static int dln2_spi_set_bpw(struct dln2_spi *dln2, u8 bpw)
> +{
> +	struct {
> +		u8 port;
> +		u8 bpw;
> +	} __packed tx;

Ditto.

> +
> +	tx.port = dln2->port;
> +	tx.bpw = bpw;
> +
> +	return dln2_transfer_tx(dln2->pdev, DLN2_SPI_SET_FRAME_SIZE,
> +				&tx, sizeof(tx));
> +}
> +
> +static int dln2_spi_get_supported_frame_sizes(struct dln2_spi *dln2,
> +					      u32 *bpw_mask)
> +{
> +	int ret;
> +	struct {
> +		u8 port;
> +	} tx;
> +	struct {
> +		u8 count;
> +		u8 frame_sizes[36];
> +	} __packed *rx = dln2->buf;
> +	unsigned rx_len = sizeof(*rx);
> +	int i;
> +
> +	tx.port = dln2->port;
> +
> +	ret = dln2_transfer(dln2->pdev, DLN2_SPI_GET_SUPPORTED_FRAME_SIZES,
> +			    &tx, sizeof(tx), rx, &rx_len);
> +	if (ret < 0)
> +		return ret;
> +	if (rx_len  < sizeof(*rx))
> +		return -EPROTO;
> +

You must verify rx->count.

> +	*bpw_mask = 0;
> +	for (i = 0; i < rx->count; i++)
> +		*bpw_mask |= BIT(rx->frame_sizes[i] - 1);
> +
> +	dev_dbg(&dln2->pdev->dev, "bpw_mask = 0x%X\n", *bpw_mask);
> +
> +	return 0;
> +}
> +
> +/*
> + * Copy the data to DLN2 buffer and change the alignment to LE, requested by
> + * DLN2 module. SPI core makes sure that the data length is a multiple of word
> + * size.

What about buffer alignment?

> + */
> +static int dln2_spi_copy_to_buf(u8 *dln2_buf, const u8 *src, u16 len, u8 bpw)
> +{
> +#ifdef __LITTLE_ENDIAN
> +	memcpy(dln2_buf, src, len);
> +#else
> +	if (bpw <= 8)
> +		memcpy(dln2_buf, src, len);

Missing braces.

> +	else if (bpw <= 16) {
> +		__le16 *d = (__le16 *) dln2_buf;

No spaces after casts.

> +		u16 *s = (u16 *) src;
> +
> +		len = len / 2;
> +		while (len--)
> +			*d++ = cpu_to_le16p(s++);
> +	} else {
> +		__le32 *d = (__le32 *) dln2_buf;
> +		u32 *s = (u32 *) src;
> +
> +		len = len / 4;
> +		while (len--)
> +			*d++ = cpu_to_le32p(s++);
> +	}
> +#endif
> +
> +	return 0;
> +}
> +
> +/*
> + * Copy the data from DLN2 buffer and convert to CPU alignment since the DLN2
> + * buffer is LE aligned. SPI core makes sure that the data length is a multiple
> + * of word size.
> + */
> +static int dln2_spi_copy_from_buf(u8 *dest, const u8 *dln2_buf, u16 len, u8 bpw)
> +{
> +#ifdef __LITTLE_ENDIAN
> +	memcpy(dest, dln2_buf, len);
> +#else
> +	if (bpw <= 8)
> +		memcpy(dest, dln2_buf, len);
> +	else if (bpw <= 16) {
> +		u16 *d = (u16 *) dest;
> +		__le16 *s = (__le16 *) dln2_buf;
> +
> +		len = len / 2;
> +		while (len--)
> +			*d++ = le16_to_cpup(s++);
> +	} else {
> +		u32 *d = (u32 *) dest;
> +		__le32 *s = (__le32 *) dln2_buf;
> +
> +		len = len / 4;
> +		while (len--)
> +			*d++ = le32_to_cpup(s++);
> +	}
> +#endif
> +
> +	return 0;
> +}
> +
> +/*
> + * Perform one write operation.
> + */
> +static int dln2_spi_write_one(struct dln2_spi *dln2, const u8 *data,
> +			      u16 data_len, u8 attr)
> +{
> +	struct {
> +		u8 port;
> +		__le16 size;
> +		u8 attr;
> +		u8 buf[DLN2_SPI_MAX_XFER_SIZE];
> +	} __packed *tx = dln2->buf;
> +	unsigned tx_len;
> +
> +	BUILD_BUG_ON(sizeof(*tx) > DLN2_SPI_BUF_SIZE);
> +
> +	if (data_len > DLN2_SPI_MAX_XFER_SIZE)
> +		return -EINVAL;
> +
> +	tx->port = dln2->port;
> +	tx->size = cpu_to_le16(data_len);
> +	tx->attr = attr;
> +
> +	dln2_spi_copy_to_buf(tx->buf, data, data_len, dln2->bpw);
> +
> +	tx_len = sizeof(*tx) + data_len - DLN2_SPI_MAX_XFER_SIZE;
> +	return dln2_transfer_tx(dln2->pdev, DLN2_SPI_WRITE, tx, tx_len);
> +}
> +
> +/*
> + * Perform one read operation.
> + */
> +static int dln2_spi_read_one(struct dln2_spi *dln2, u8 *data,
> +			     u16 data_len, u8 attr)
> +{
> +	int ret;
> +	struct {
> +		u8 port;
> +		__le16 size;
> +		u8 attr;
> +	} __packed tx;
> +	struct {
> +		__le16 size;
> +		u8 buf[DLN2_SPI_MAX_XFER_SIZE];
> +	} __packed *rx = dln2->buf;
> +	unsigned rx_len = sizeof(*rx);
> +
> +	BUILD_BUG_ON(sizeof(*rx) > DLN2_SPI_BUF_SIZE);
> +
> +	if (data_len > DLN2_SPI_MAX_XFER_SIZE)
> +		return -EINVAL;
> +
> +	tx.port = dln2->port;
> +	tx.size = cpu_to_le16(data_len);
> +	tx.attr = attr;
> +
> +	ret = dln2_transfer(dln2->pdev, DLN2_SPI_READ, &tx, sizeof(tx),
> +			    rx, &rx_len);
> +	if (ret < 0)
> +		return ret;
> +	if (rx_len < sizeof(rx->size) + data_len)
> +		return -EPROTO;
> +	if (le16_to_cpu(rx->size) != data_len)
> +		return -EPROTO;
> +
> +	dln2_spi_copy_from_buf(data, rx->buf, data_len, dln2->bpw);
> +
> +	return 0;
> +}
> +
> +/*
> + * Perform one write & read operation.
> + */
> +static int dln2_spi_read_write_one(struct dln2_spi *dln2, const u8 *tx_data,
> +				   u8 *rx_data, u16 data_len, u8 attr)
> +{
> +	int ret;
> +	struct {
> +		u8 port;
> +		__le16 size;
> +		u8 attr;
> +		u8 buf[DLN2_SPI_MAX_XFER_SIZE];
> +	} __packed *tx;
> +	struct {
> +		__le16 size;
> +		u8 buf[DLN2_SPI_MAX_XFER_SIZE];
> +	} __packed *rx;
> +	unsigned tx_len, rx_len;
> +
> +	BUILD_BUG_ON(sizeof(*tx) > DLN2_SPI_BUF_SIZE ||
> +		     sizeof(*rx) > DLN2_SPI_BUF_SIZE);
> +
> +	if (data_len > DLN2_SPI_MAX_XFER_SIZE)
> +		return -EINVAL;
> +
> +	/* Since this is a pseudo full-duplex communication, we're perfectly
> +	 * safe to use the same buffer for both tx and rx. When DLN2 sends the
> +	 * response back, with the rx data, we don't need the tx buffer anymore.
> +	 */
> +	tx = dln2->buf;
> +	rx = dln2->buf;
> +
> +	tx->port = dln2->port;
> +	tx->size = cpu_to_le16(data_len);
> +	tx->attr = attr;
> +
> +	dln2_spi_copy_to_buf(tx->buf, tx_data, data_len, dln2->bpw);
> +
> +	tx_len = sizeof(*tx) + data_len - DLN2_SPI_MAX_XFER_SIZE;
> +	rx_len = sizeof(*rx);
> +
> +	ret = dln2_transfer(dln2->pdev, DLN2_SPI_READ_WRITE, tx, tx_len,
> +			    rx, &rx_len);
> +	if (ret < 0)
> +		return ret;
> +	if (rx_len < sizeof(rx->size) + data_len)
> +		return -EPROTO;
> +	if (le16_to_cpu(rx->size) != data_len)
> +		return -EPROTO;
> +
> +	dln2_spi_copy_from_buf(rx_data, rx->buf, data_len, dln2->bpw);
> +
> +	return 0;
> +}
> +
> +/*
> + * Read/Write wrapper. It will automatically split an operation into multiple
> + * single ones due to device buffer constraints.
> + */
> +static int dln2_spi_rdwr(struct dln2_spi *dln2, const u8 *tx_data,
> +			 u8 *rx_data, u16 data_len, u8 attr) {
> +	int ret;
> +	u16 len;
> +	u8 temp_attr;
> +	u16 remaining = data_len;
> +	u16 offset;
> +
> +	do {
> +		if (remaining > DLN2_SPI_MAX_XFER_SIZE) {
> +			len = DLN2_SPI_MAX_XFER_SIZE;
> +			temp_attr = DLN2_SPI_ATTR_LEAVE_SS_LOW;
> +		} else {
> +			len = remaining;
> +			temp_attr = attr;
> +		}
> +
> +		offset = data_len - remaining;
> +
> +		if (tx_data && rx_data)
> +			ret = dln2_spi_read_write_one(dln2,
> +						      tx_data + offset,
> +						      rx_data + offset,
> +						      len, temp_attr);
> +		else if (tx_data)
> +			ret = dln2_spi_write_one(dln2,
> +						 tx_data + offset,
> +						 len, temp_attr);
> +		else if (rx_data)
> +			ret = dln2_spi_read_one(dln2,
> +						rx_data + offset,
> +						len, temp_attr);
> +		else
> +			return -EINVAL;

Braces, please.

> +
> +		if (ret < 0)
> +			return ret;
> +
> +		remaining -= len;
> +	} while (remaining);
> +
> +	return 0;
> +}
> +
> +static int dln2_spi_prepare_message(struct spi_master *master,
> +				    struct spi_message *message)
> +{
> +	int ret;
> +	struct dln2_spi *dln2 = spi_master_get_devdata(master);
> +	struct spi_device *spi = message->spi;
> +
> +	if (dln2->cs != spi->chip_select) {
> +		ret = dln2_spi_cs_set_one(dln2, spi->chip_select);
> +		if (ret < 0)
> +			return ret;
> +
> +		dln2->cs = spi->chip_select;
> +	}
> +
> +	return 0;
> +}
> +
> +static int dln2_spi_transfer_setup(struct dln2_spi *dln2, u32 speed,
> +				   u8 bpw, u8 mode)
> +{
> +	int ret;
> +	bool bus_setup_change;
> +
> +	bus_setup_change = dln2->speed != speed || dln2->mode != mode ||
> +			   dln2->bpw != bpw;
> +
> +	if (!bus_setup_change)
> +		return 0;
> +
> +	ret = dln2_spi_enable(dln2, false);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (dln2->speed != speed) {
> +		ret = dln2_spi_set_speed(dln2, speed, NULL);
> +		if (ret < 0)
> +			return ret;
> +
> +		dln2->speed = speed;
> +	}
> +
> +	if (dln2->mode != mode) {
> +		ret = dln2_spi_set_mode(dln2, mode & 0x3);
> +		if (ret < 0)
> +			return ret;
> +
> +		dln2->mode = mode;
> +	}
> +
> +	if (dln2->bpw != bpw) {
> +		ret = dln2_spi_set_bpw(dln2, bpw);
> +		if (ret < 0)
> +			return ret;
> +
> +		dln2->bpw = bpw;
> +	}
> +
> +	ret = dln2_spi_enable(dln2, true);
> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static int dln2_spi_transfer_one_message(struct spi_master *master,
> +					 struct spi_message *msg)
> +{
> +	struct dln2_spi *dln2 = spi_master_get_devdata(master);
> +	struct spi_device *spi = msg->spi;
> +	struct spi_transfer *xfer;
> +	int status;
> +
> +	list_for_each_entry(xfer, &msg->transfers, transfer_list) {
> +		u8 attr = 0;
> +
> +		status = dln2_spi_transfer_setup(dln2, xfer->speed_hz,
> +						 xfer->bits_per_word,
> +						 spi->mode);
> +		if (status < 0) {
> +			dev_err(&dln2->pdev->dev, "Cannot setup transfer\n");
> +			break;
> +		}
> +
> +		if (!xfer->cs_change &&
> +		    !list_is_last(&xfer->transfer_list, &msg->transfers))
> +			attr = xfer->cs_change ? 0 : DLN2_SPI_ATTR_LEAVE_SS_LOW;
> +
> +		status = dln2_spi_rdwr(dln2, xfer->tx_buf, xfer->rx_buf,
> +				       xfer->len, attr);
> +		if (status < 0) {
> +			dev_err(&dln2->pdev->dev, "write/read failed!\n");
> +			break;
> +		}
> +
> +		status = 0;
> +	}
> +
> +	msg->status = status;
> +	spi_finalize_current_message(master);
> +	return status;
> +}
> +
> +static int dln2_spi_probe(struct platform_device *pdev)
> +{
> +	struct spi_master *master;
> +	struct dln2_spi *dln2;
> +	struct dln2_platform_data *pdata = dev_get_platdata(&pdev->dev);
> +	int ret;
> +
> +	master = spi_alloc_master(&pdev->dev, sizeof(*dln2));
> +	if (!master)
> +		return -ENOMEM;
> +
> +	platform_set_drvdata(pdev, master);
> +
> +	dln2 = spi_master_get_devdata(master);
> +
> +	dln2->buf = devm_kmalloc(&pdev->dev, DLN2_SPI_BUF_SIZE, GFP_KERNEL);
> +	if (!dln2->buf) {
> +		ret = -ENOMEM;
> +		goto exit_free_master;
> +	}
> +
> +	dln2->master = master;
> +	dln2->pdev = pdev;
> +	dln2->port = pdata->port;
> +	/* cs number can never be 0xff, so the first transfer will set the cs */
> +	dln2->cs = 0xff;
> +
> +	/* disable SPI module before continuing with the setup */
> +	ret = dln2_spi_enable(dln2, false);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "Failed to disable SPI module\n");
> +		goto exit_free_master;
> +	}
> +
> +	ret = dln2_spi_get_cs_num(dln2, &master->num_chipselect);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "Failed to get number of CS pins\n");
> +		goto exit_free_master;
> +	}
> +
> +	ret = dln2_spi_get_speed_range(dln2,
> +				       &master->min_speed_hz,
> +				       &master->max_speed_hz);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "Failed to read bus min/max freqs\n");
> +		goto exit_free_master;
> +	}
> +
> +	ret = dln2_spi_get_supported_frame_sizes(dln2,
> +						 &master->bits_per_word_mask);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "Failed to read supported frame sizes\n");
> +		goto exit_free_master;
> +	}
> +
> +	ret = dln2_spi_cs_enable_all(dln2, true);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "Failed to enable CS pins\n");
> +		goto exit_free_master;
> +	}
> +
> +	master->bus_num = -1;
> +	master->mode_bits = SPI_CPOL | SPI_CPHA;
> +	master->prepare_message = dln2_spi_prepare_message;
> +	master->transfer_one_message = dln2_spi_transfer_one_message;
> +
> +	/* enable SPI module, we're good to go */
> +	ret = dln2_spi_enable(dln2, true);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "Failed to enable SPI module\n");
> +		goto exit_free_master;
> +	}
> +
> +	ret = devm_spi_register_master(&pdev->dev, master);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "Failed to register master\n");
> +		goto exit_register;
> +	}
> +
> +	return ret;
> +
> +exit_register:
> +	if (dln2_spi_enable(dln2, false) < 0)
> +		dev_err(&pdev->dev, "Failed to disable SPI module\n");
> +exit_free_master:
> +	spi_master_put(master);
> +
> +	return ret;
> +}
> +
> +static int dln2_spi_remove(struct platform_device *pdev)
> +{
> +	struct spi_master *master = spi_master_get(platform_get_drvdata(pdev));
> +	struct dln2_spi *dln2 = spi_master_get_devdata(master);
> +
> +	if (dln2_spi_enable(dln2, false) < 0)
> +		dev_err(&pdev->dev, "Failed to disable SPI module\n");
> +

Memory leak -- spi_master_put missing.

> +	return 0;
> +}
> +
> +static struct platform_driver spi_dln2_driver = {
> +	.driver.name	= "dln2-spi",
> +	.probe		= dln2_spi_probe,
> +	.remove		= dln2_spi_remove,
> +};
> +module_platform_driver(spi_dln2_driver);
> +
> +MODULE_DESCRIPTION("Driver for the Diolan DLN2 SPI master interface");
> +MODULE_AUTHOR("Laurentiu Palcu <laurentiu.palcu@intel.com>");
> +MODULE_LICENSE("GPL v2");
> +MODULE_ALIAS("platform:dln2-spi");

Johan

Re: [PATCH v2 1/2] spi: add support for DLN-2 USB-SPI adapter

[Laurentiu Palcu]

Hi Johan,

On Thu, Nov 13, 2014 at 01:27:28PM +0100, Johan Havold wrote:
> On Wed, Nov 12, 2014 at 03:38:09PM +0200, Laurentiu Palcu wrote:
> > This adds support for Diolan DLN2 USB-SPI adapter.
> > 
> > Information about the USB protocol interface can be found in the
> > Programmer's Reference Manual [1], see section 5.4.6 for the SPI
> > master module commands and responses.
> > 
> > [1] https://www.diolan.com/downloads/dln-api-manual.pdf
> > 
> > Signed-off-by: Laurentiu Palcu <laurentiu.palcu@intel.com>
> > ---
> >  drivers/spi/Kconfig    |  10 +
> >  drivers/spi/Makefile   |   1 +
> >  drivers/spi/spi-dln2.c | 793 +++++++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 804 insertions(+)
> >  create mode 100644 drivers/spi/spi-dln2.c
> > 
> > diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
> > index 62e2242..a52a910 100644
> > --- a/drivers/spi/Kconfig
> > +++ b/drivers/spi/Kconfig
> > @@ -176,6 +176,16 @@ config SPI_DAVINCI
> >  	help
> >  	  SPI master controller for DaVinci/DA8x/OMAP-L/AM1x SPI modules.
> >  
> > +config SPI_DLN2
> > +       tristate "Diolan DLN-2 USB SPI adapter"
> > +       depends on MFD_DLN2
> > +       help
> > +         If you say yes to this option, support will be included for Diolan
> > +         DLN2, a USB to SPI interface.
> > +
> > +         This driver can also be built as a module.  If so, the module
> > +         will be called spi-dln2.
> > +
> >  config SPI_EFM32
> >  	tristate "EFM32 SPI controller"
> >  	depends on OF && ARM && (ARCH_EFM32 || COMPILE_TEST)
> > diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
> > index 762da07..b315da2 100644
> > --- a/drivers/spi/Makefile
> > +++ b/drivers/spi/Makefile
> > @@ -26,6 +26,7 @@ obj-$(CONFIG_SPI_CADENCE)		+= spi-cadence.o
> >  obj-$(CONFIG_SPI_CLPS711X)		+= spi-clps711x.o
> >  obj-$(CONFIG_SPI_COLDFIRE_QSPI)		+= spi-coldfire-qspi.o
> >  obj-$(CONFIG_SPI_DAVINCI)		+= spi-davinci.o
> > +obj-$(CONFIG_SPI_DLN2)			+= spi-dln2.o
> >  obj-$(CONFIG_SPI_DESIGNWARE)		+= spi-dw.o
> >  obj-$(CONFIG_SPI_DW_MMIO)		+= spi-dw-mmio.o
> >  obj-$(CONFIG_SPI_DW_PCI)		+= spi-dw-midpci.o
> > diff --git a/drivers/spi/spi-dln2.c b/drivers/spi/spi-dln2.c
> > new file mode 100644
> > index 0000000..8277294
> > --- /dev/null
> > +++ b/drivers/spi/spi-dln2.c
> > @@ -0,0 +1,793 @@
> > +/*
> > + * Driver for the Diolan DLN-2 USB-SPI adapter
> > + *
> > + * Copyright (c) 2014 Intel Corporation
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License as
> > + * published by the Free Software Foundation, version 2.
> > + */
> > +
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/mfd/dln2.h>
> > +#include <linux/spi/spi.h>
> > +
> > +#define DLN2_SPI_MODULE_ID		0x02
> > +#define DLN2_SPI_CMD(cmd)		DLN2_CMD(cmd, DLN2_SPI_MODULE_ID)
> > +
> > +/* SPI commands */
> > +#define DLN2_SPI_GET_PORT_COUNT			DLN2_SPI_CMD(0x00)
> > +#define DLN2_SPI_ENABLE				DLN2_SPI_CMD(0x11)
> > +#define DLN2_SPI_DISABLE			DLN2_SPI_CMD(0x12)
> > +#define DLN2_SPI_IS_ENABLED			DLN2_SPI_CMD(0x13)
> > +#define DLN2_SPI_SET_MODE			DLN2_SPI_CMD(0x14)
> > +#define DLN2_SPI_GET_MODE			DLN2_SPI_CMD(0x15)
> > +#define DLN2_SPI_SET_FRAME_SIZE			DLN2_SPI_CMD(0x16)
> > +#define DLN2_SPI_GET_FRAME_SIZE			DLN2_SPI_CMD(0x17)
> > +#define DLN2_SPI_SET_FREQUENCY			DLN2_SPI_CMD(0x18)
> > +#define DLN2_SPI_GET_FREQUENCY			DLN2_SPI_CMD(0x19)
> > +#define DLN2_SPI_READ_WRITE			DLN2_SPI_CMD(0x1A)
> > +#define DLN2_SPI_READ				DLN2_SPI_CMD(0x1B)
> > +#define DLN2_SPI_WRITE				DLN2_SPI_CMD(0x1C)
> > +#define DLN2_SPI_SET_DELAY_BETWEEN_SS		DLN2_SPI_CMD(0x20)
> > +#define DLN2_SPI_GET_DELAY_BETWEEN_SS		DLN2_SPI_CMD(0x21)
> > +#define DLN2_SPI_SET_DELAY_AFTER_SS		DLN2_SPI_CMD(0x22)
> > +#define DLN2_SPI_GET_DELAY_AFTER_SS		DLN2_SPI_CMD(0x23)
> > +#define DLN2_SPI_SET_DELAY_BETWEEN_FRAMES	DLN2_SPI_CMD(0x24)
> > +#define DLN2_SPI_GET_DELAY_BETWEEN_FRAMES	DLN2_SPI_CMD(0x25)
> > +#define DLN2_SPI_SET_SS				DLN2_SPI_CMD(0x26)
> > +#define DLN2_SPI_GET_SS				DLN2_SPI_CMD(0x27)
> > +#define DLN2_SPI_RELEASE_SS			DLN2_SPI_CMD(0x28)
> > +#define DLN2_SPI_SS_VARIABLE_ENABLE		DLN2_SPI_CMD(0x2B)
> > +#define DLN2_SPI_SS_VARIABLE_DISABLE		DLN2_SPI_CMD(0x2C)
> > +#define DLN2_SPI_SS_VARIABLE_IS_ENABLED		DLN2_SPI_CMD(0x2D)
> > +#define DLN2_SPI_SS_AAT_ENABLE			DLN2_SPI_CMD(0x2E)
> > +#define DLN2_SPI_SS_AAT_DISABLE			DLN2_SPI_CMD(0x2F)
> > +#define DLN2_SPI_SS_AAT_IS_ENABLED		DLN2_SPI_CMD(0x30)
> > +#define DLN2_SPI_SS_BETWEEN_FRAMES_ENABLE	DLN2_SPI_CMD(0x31)
> > +#define DLN2_SPI_SS_BETWEEN_FRAMES_DISABLE	DLN2_SPI_CMD(0x32)
> > +#define DLN2_SPI_SS_BETWEEN_FRAMES_IS_ENABLED	DLN2_SPI_CMD(0x33)
> > +#define DLN2_SPI_SET_CPHA			DLN2_SPI_CMD(0x34)
> > +#define DLN2_SPI_GET_CPHA			DLN2_SPI_CMD(0x35)
> > +#define DLN2_SPI_SET_CPOL			DLN2_SPI_CMD(0x36)
> > +#define DLN2_SPI_GET_CPOL			DLN2_SPI_CMD(0x37)
> > +#define DLN2_SPI_SS_MULTI_ENABLE		DLN2_SPI_CMD(0x38)
> > +#define DLN2_SPI_SS_MULTI_DISABLE		DLN2_SPI_CMD(0x39)
> > +#define DLN2_SPI_SS_MULTI_IS_ENABLED		DLN2_SPI_CMD(0x3A)
> > +#define DLN2_SPI_GET_SUPPORTED_MODES		DLN2_SPI_CMD(0x40)
> > +#define DLN2_SPI_GET_SUPPORTED_CPHA_VALUES	DLN2_SPI_CMD(0x41)
> > +#define DLN2_SPI_GET_SUPPORTED_CPOL_VALUES	DLN2_SPI_CMD(0x42)
> > +#define DLN2_SPI_GET_SUPPORTED_FRAME_SIZES	DLN2_SPI_CMD(0x43)
> > +#define DLN2_SPI_GET_SS_COUNT			DLN2_SPI_CMD(0x44)
> > +#define DLN2_SPI_GET_MIN_FREQUENCY		DLN2_SPI_CMD(0x45)
> > +#define DLN2_SPI_GET_MAX_FREQUENCY		DLN2_SPI_CMD(0x46)
> > +#define DLN2_SPI_GET_MIN_DELAY_BETWEEN_SS	DLN2_SPI_CMD(0x47)
> > +#define DLN2_SPI_GET_MAX_DELAY_BETWEEN_SS	DLN2_SPI_CMD(0x48)
> > +#define DLN2_SPI_GET_MIN_DELAY_AFTER_SS		DLN2_SPI_CMD(0x49)
> > +#define DLN2_SPI_GET_MAX_DELAY_AFTER_SS		DLN2_SPI_CMD(0x4A)
> > +#define DLN2_SPI_GET_MIN_DELAY_BETWEEN_FRAMES	DLN2_SPI_CMD(0x4B)
> > +#define DLN2_SPI_GET_MAX_DELAY_BETWEEN_FRAMES	DLN2_SPI_CMD(0x4C)
> > +
> > +#define DLN2_SPI_MAX_XFER_SIZE			256
> > +#define DLN2_SPI_BUF_SIZE			(DLN2_SPI_MAX_XFER_SIZE + 16)
> > +#define DLN2_SPI_ATTR_LEAVE_SS_LOW		BIT(0)
> > +#define DLN2_TRANSFERS_WAIT_COMPLETE		1
> > +#define DLN2_TRANSFERS_CANCEL			0
> > +
> > +struct dln2_spi {
> > +	struct platform_device *pdev;
> > +	struct spi_master *master;
> > +	u8 port;
> > +
> > +	void *buf;
> 
> Add comment on what is protecting this buffer.
No need to protect this buffer. First off, AFAICS, once we register the
master, a message queue is initialized by the spi core and the entire
communication with the SPI module goes through this queue. Secondly,
every TX/RX SPI operation with the Diolan is split in 2 parts: command
and response. Once we send the command out, the buffer can be safely
reused for the response.

Also, I guess this answers a couple of your comments below too.
> 
> > +
> > +	u8 bpw;
> > +	u32 speed;
> > +	u16 mode;
> > +	u8 cs;
> > +};
> > +
> > +/*
> > + * Enable/Disable SPI module. The disable command will wait for transfers to
> > + * complete first.
> > + */
> > +static int dln2_spi_enable(struct dln2_spi *dln2, bool enable)
> > +{
> > +	int ret;
> > +	u16 cmd;
> > +	struct {
> > +		u8 port;
> > +		u8 wait_for_completion;
> > +	} __packed tx;
> 
> No need for __packed.
True, will be removed, together with the other unnecessary ones below. I
guess I was overzealous with the __packed usage.
> 
> > +	unsigned len = sizeof(tx);
> > +
> > +	tx.port = dln2->port;
> > +
> > +	if (enable) {
> > +		cmd = DLN2_SPI_ENABLE;
> > +		len -= sizeof(tx.wait_for_completion);
> > +	} else {
> > +		tx.wait_for_completion = DLN2_TRANSFERS_WAIT_COMPLETE;
> > +		cmd = DLN2_SPI_DISABLE;
> > +	}
> > +
> > +	ret = dln2_transfer_tx(dln2->pdev, cmd, &tx, len);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	return 0;
> > +}
> > +
> > +/*
> > + * Select/unselect multiple CS lines. The selected lines will be automatically
> > + * toggled LOW/HIGH by the board firmware during transfers, provided they're
> > + * enabled first.
> > + *
> > + * Ex: cs_mask = 0x03 -> CS0 & CS1 will be selected and the next WR/RD operation
> 
> Seems you have several comments that wrap at >80 columns (81 above).
According to the kernel coding style, 80 columns is the limit, unless
readability is affected. The line above does not exceed this limit. Also,
checkpatch does not complain.
> 
> > + *                       will toggle the lines LOW/HIGH automatically.
> > + */
> > +static int dln2_spi_cs_set(struct dln2_spi *dln2, u8 cs_mask)
> > +{
> > +	struct {
> > +		u8 port;
> > +		u8 cs;
> > +	} __packed tx;
> > +
> 
> No __packed.
> 
> > +	tx.port = dln2->port;
> > +
> > +	/* According to Diolan docs, "a slave device can be selected by changing
> > +	 * the corresponding bit value to 0". The rest must be set to 1. Hence
> > +	 * the bitwise NOT in front.
> > +	 */
> > +	tx.cs = ~cs_mask;
> > +
> > +	return dln2_transfer_tx(dln2->pdev, DLN2_SPI_SET_SS, &tx, sizeof(tx));
> > +}
> > +
> > +/*
> > + * Select one CS line. The other lines will be un-selected.
> > + */
> > +static int dln2_spi_cs_set_one(struct dln2_spi *dln2, u8 cs)
> > +{
> > +	return dln2_spi_cs_set(dln2, BIT(cs));
> > +}
> > +
> > +/*
> > + * Enable/disable CS lines for usage. The module has to be disabled first.
> > + */
> > +static int dln2_spi_cs_enable(struct dln2_spi *dln2, u8 cs_mask, bool enable)
> > +{
> > +	struct {
> > +		u8 port;
> > +		u8 cs;
> > +	} __packed tx;
> 
> No __packed.
> 
> > +	u16 cmd;
> > +
> > +	tx.port = dln2->port;
> > +	tx.cs = cs_mask;
> > +	cmd = enable ? DLN2_SPI_SS_MULTI_ENABLE : DLN2_SPI_SS_MULTI_DISABLE;
> > +
> > +	return dln2_transfer_tx(dln2->pdev, cmd, &tx, sizeof(tx));
> > +}
> > +
> > +static int dln2_spi_cs_enable_all(struct dln2_spi *dln2, bool enable)
> > +{
> > +	u8 cs_mask = GENMASK(dln2->master->num_chipselect - 1, 0);
> > +
> > +	return dln2_spi_cs_enable(dln2, cs_mask, enable);
> > +}
> > +
> > +static int dln2_spi_get_cs_num(struct dln2_spi *dln2, u16 *cs_num)
> > +{
> > +	int ret;
> > +	struct {
> > +		u8 port;
> > +	} tx;
> > +	struct {
> > +		__le16 cs_count;
> > +	} *rx = dln2->buf;
> 
> I don't think you want to use dln2->buf for all these small transfers.
> And what would be protecting it from concurrent use?
> 
> Check this throughout.
I answered this above.
> 
> > +	unsigned rx_len = sizeof(*rx);
> > +
> > +	tx.port = dln2->port;
> > +	ret = dln2_transfer(dln2->pdev, DLN2_SPI_GET_SS_COUNT, &tx, sizeof(tx),
> > +			    rx, &rx_len);
> > +	if (ret < 0)
> > +		return ret;
> > +	if (rx_len < sizeof(*rx))
> > +		return -EPROTO;
> > +
> > +	*cs_num = le16_to_cpu(rx->cs_count);
> > +
> > +	dev_dbg(&dln2->pdev->dev, "cs_num = %d\n", *cs_num);
> 
> Use the spi device for device messages throughout.
Apparently, I'm consistent with the other SPI master controller drivers.
All of them use pdev->dev for printing messages. However, I tried it,
and the prefix is "spi_master (null):" when master->dev is used,
compared to "dln2-spi dln2-spi.2.auto:" when pdev->dev is used. It looks
like master->dev.init_name is not set... :/

> 
> > +
> > +	return 0;
> > +}
> > +
> > +/*
> > + * Get bus min/max frequencies.
> > + */
> > +static int dln2_spi_get_speed_range(struct dln2_spi *dln2, u32 *fmin, u32 *fmax)
> > +{
> > +	int ret;
> > +	struct {
> > +		u8 port;
> > +	} tx;
> > +	struct {
> > +		__le32 speed;
> > +	} *rx = dln2->buf;
> > +	unsigned rx_len = sizeof(*rx);
> > +	int cmd[2] = {DLN2_SPI_GET_MIN_FREQUENCY, DLN2_SPI_GET_MAX_FREQUENCY};
> > +	u32 *speed[2] = {fmin, fmax};
> 
> Spaces after and before { and }.
> 
> > +	int i;
> > +
> > +	tx.port = dln2->port;
> > +
> > +	for (i = 0; i < 2; i++) {
> > +		ret = dln2_transfer(dln2->pdev, cmd[i], &tx, sizeof(tx),
> > +				    rx, &rx_len);
> > +		if (ret < 0)
> > +			return ret;
> > +		if (rx_len < sizeof(*rx))
> > +			return -EPROTO;
> > +
> > +		*speed[i] = le32_to_cpu(rx->speed);
> > +	}
> 
> Add a helper to fetch a 32-bit value and call it twice instead of the
> loop-construct above.
Ok, I guess I could use a helper instead of this.

> 
> > +
> > +	dev_dbg(&dln2->pdev->dev, "freq_min = %d, freq_max = %d\n",
> > +		*fmin, *fmax);
> > +
> > +	return 0;
> > +}
> > +
> > +/*
> > + * Set the bus speed. The module will automatically round down to the closest
> > + * available frequency and returns it. The module has to be disabled first.
> > + */
> > +static int dln2_spi_set_speed(struct dln2_spi *dln2, u32 speed,
> > +			      u32 *probed_speed)
> > +{
> > +	int ret;
> > +	struct {
> > +		u8 port;
> > +		__le32 speed;
> > +	} __packed tx;
> > +	struct {
> > +		__le32 speed;
> > +	} __packed *rx = dln2->buf;
> > +	int rx_len = sizeof(*rx);
> > +
> > +	tx.port = dln2->port;
> > +	tx.speed = cpu_to_le32(speed);
> > +
> > +	ret = dln2_transfer(dln2->pdev, DLN2_SPI_SET_FREQUENCY, &tx, sizeof(tx),
> > +			    rx, &rx_len);
> > +	if (ret < 0)
> > +		return ret;
> > +	if (rx_len < sizeof(*rx))
> > +		return -EPROTO;
> > +
> > +	if (probed_speed)
> > +		*probed_speed = le32_to_cpu(rx->speed);
> 
> You never use the probed_speed parameter in any call to this function.
> Remove it if you don't need it.
ok

> 
> > +
> > +	return 0;
> > +}
> > +
> > +/*
> > + * Change CPOL & CPHA. The module has to be disabled first.
> > + */
> > +static int dln2_spi_set_mode(struct dln2_spi *dln2, u8 mode)
> > +{
> > +	struct {
> > +		u8 port;
> > +		u8 mode;
> > +	} __packed tx;
> 
> No __packed.
> 
> > +
> > +	tx.port = dln2->port;
> > +	tx.mode = mode;
> > +
> > +	return dln2_transfer_tx(dln2->pdev, DLN2_SPI_SET_MODE, &tx, sizeof(tx));
> > +}
> > +
> > +/*
> > + * Change frame size. The module has to be disabled first.
> > + */
> > +static int dln2_spi_set_bpw(struct dln2_spi *dln2, u8 bpw)
> > +{
> > +	struct {
> > +		u8 port;
> > +		u8 bpw;
> > +	} __packed tx;
> 
> Ditto.
> 
> > +
> > +	tx.port = dln2->port;
> > +	tx.bpw = bpw;
> > +
> > +	return dln2_transfer_tx(dln2->pdev, DLN2_SPI_SET_FRAME_SIZE,
> > +				&tx, sizeof(tx));
> > +}
> > +
> > +static int dln2_spi_get_supported_frame_sizes(struct dln2_spi *dln2,
> > +					      u32 *bpw_mask)
> > +{
> > +	int ret;
> > +	struct {
> > +		u8 port;
> > +	} tx;
> > +	struct {
> > +		u8 count;
> > +		u8 frame_sizes[36];
> > +	} __packed *rx = dln2->buf;
> > +	unsigned rx_len = sizeof(*rx);
> > +	int i;
> > +
> > +	tx.port = dln2->port;
> > +
> > +	ret = dln2_transfer(dln2->pdev, DLN2_SPI_GET_SUPPORTED_FRAME_SIZES,
> > +			    &tx, sizeof(tx), rx, &rx_len);
> > +	if (ret < 0)
> > +		return ret;
> > +	if (rx_len  < sizeof(*rx))
> > +		return -EPROTO;
> > +
> 
> You must verify rx->count.
True
> 
> > +	*bpw_mask = 0;
> > +	for (i = 0; i < rx->count; i++)
> > +		*bpw_mask |= BIT(rx->frame_sizes[i] - 1);
> > +
> > +	dev_dbg(&dln2->pdev->dev, "bpw_mask = 0x%X\n", *bpw_mask);
> > +
> > +	return 0;
> > +}
> > +
> > +/*
> > + * Copy the data to DLN2 buffer and change the alignment to LE, requested by
> > + * DLN2 module. SPI core makes sure that the data length is a multiple of word
> > + * size.
> 
> What about buffer alignment?
Buffer alignment? Why should the buffer be aligned? Now that you
mentioned it, I realized I should've used "byte order" instead of
alignment in the comment above...
> 
> > + */
> > +static int dln2_spi_copy_to_buf(u8 *dln2_buf, const u8 *src, u16 len, u8 bpw)
> > +{
> > +#ifdef __LITTLE_ENDIAN
> > +	memcpy(dln2_buf, src, len);
> > +#else
> > +	if (bpw <= 8)
> > +		memcpy(dln2_buf, src, len);
> 
> Missing braces.
ok

> 
> > +	else if (bpw <= 16) {
> > +		__le16 *d = (__le16 *) dln2_buf;
> 
> No spaces after casts.
ok

> 
> > +		u16 *s = (u16 *) src;
> > +
> > +		len = len / 2;
> > +		while (len--)
> > +			*d++ = cpu_to_le16p(s++);
> > +	} else {
> > +		__le32 *d = (__le32 *) dln2_buf;
> > +		u32 *s = (u32 *) src;
> > +
> > +		len = len / 4;
> > +		while (len--)
> > +			*d++ = cpu_to_le32p(s++);
> > +	}
> > +#endif
> > +
> > +	return 0;
> > +}
> > +
> > +/*
> > + * Copy the data from DLN2 buffer and convert to CPU alignment since the DLN2
> > + * buffer is LE aligned. SPI core makes sure that the data length is a multiple
> > + * of word size.
> > + */
> > +static int dln2_spi_copy_from_buf(u8 *dest, const u8 *dln2_buf, u16 len, u8 bpw)
> > +{
> > +#ifdef __LITTLE_ENDIAN
> > +	memcpy(dest, dln2_buf, len);
> > +#else
> > +	if (bpw <= 8)
> > +		memcpy(dest, dln2_buf, len);
> > +	else if (bpw <= 16) {
> > +		u16 *d = (u16 *) dest;
> > +		__le16 *s = (__le16 *) dln2_buf;
> > +
> > +		len = len / 2;
> > +		while (len--)
> > +			*d++ = le16_to_cpup(s++);
> > +	} else {
> > +		u32 *d = (u32 *) dest;
> > +		__le32 *s = (__le32 *) dln2_buf;
> > +
> > +		len = len / 4;
> > +		while (len--)
> > +			*d++ = le32_to_cpup(s++);
> > +	}
> > +#endif
> > +
> > +	return 0;
> > +}
> > +
> > +/*
> > + * Perform one write operation.
> > + */
> > +static int dln2_spi_write_one(struct dln2_spi *dln2, const u8 *data,
> > +			      u16 data_len, u8 attr)
> > +{
> > +	struct {
> > +		u8 port;
> > +		__le16 size;
> > +		u8 attr;
> > +		u8 buf[DLN2_SPI_MAX_XFER_SIZE];
> > +	} __packed *tx = dln2->buf;
> > +	unsigned tx_len;
> > +
> > +	BUILD_BUG_ON(sizeof(*tx) > DLN2_SPI_BUF_SIZE);
> > +
> > +	if (data_len > DLN2_SPI_MAX_XFER_SIZE)
> > +		return -EINVAL;
> > +
> > +	tx->port = dln2->port;
> > +	tx->size = cpu_to_le16(data_len);
> > +	tx->attr = attr;
> > +
> > +	dln2_spi_copy_to_buf(tx->buf, data, data_len, dln2->bpw);
> > +
> > +	tx_len = sizeof(*tx) + data_len - DLN2_SPI_MAX_XFER_SIZE;
> > +	return dln2_transfer_tx(dln2->pdev, DLN2_SPI_WRITE, tx, tx_len);
> > +}
> > +
> > +/*
> > + * Perform one read operation.
> > + */
> > +static int dln2_spi_read_one(struct dln2_spi *dln2, u8 *data,
> > +			     u16 data_len, u8 attr)
> > +{
> > +	int ret;
> > +	struct {
> > +		u8 port;
> > +		__le16 size;
> > +		u8 attr;
> > +	} __packed tx;
> > +	struct {
> > +		__le16 size;
> > +		u8 buf[DLN2_SPI_MAX_XFER_SIZE];
> > +	} __packed *rx = dln2->buf;
> > +	unsigned rx_len = sizeof(*rx);
> > +
> > +	BUILD_BUG_ON(sizeof(*rx) > DLN2_SPI_BUF_SIZE);
> > +
> > +	if (data_len > DLN2_SPI_MAX_XFER_SIZE)
> > +		return -EINVAL;
> > +
> > +	tx.port = dln2->port;
> > +	tx.size = cpu_to_le16(data_len);
> > +	tx.attr = attr;
> > +
> > +	ret = dln2_transfer(dln2->pdev, DLN2_SPI_READ, &tx, sizeof(tx),
> > +			    rx, &rx_len);
> > +	if (ret < 0)
> > +		return ret;
> > +	if (rx_len < sizeof(rx->size) + data_len)
> > +		return -EPROTO;
> > +	if (le16_to_cpu(rx->size) != data_len)
> > +		return -EPROTO;
> > +
> > +	dln2_spi_copy_from_buf(data, rx->buf, data_len, dln2->bpw);
> > +
> > +	return 0;
> > +}
> > +
> > +/*
> > + * Perform one write & read operation.
> > + */
> > +static int dln2_spi_read_write_one(struct dln2_spi *dln2, const u8 *tx_data,
> > +				   u8 *rx_data, u16 data_len, u8 attr)
> > +{
> > +	int ret;
> > +	struct {
> > +		u8 port;
> > +		__le16 size;
> > +		u8 attr;
> > +		u8 buf[DLN2_SPI_MAX_XFER_SIZE];
> > +	} __packed *tx;
> > +	struct {
> > +		__le16 size;
> > +		u8 buf[DLN2_SPI_MAX_XFER_SIZE];
> > +	} __packed *rx;
> > +	unsigned tx_len, rx_len;
> > +
> > +	BUILD_BUG_ON(sizeof(*tx) > DLN2_SPI_BUF_SIZE ||
> > +		     sizeof(*rx) > DLN2_SPI_BUF_SIZE);
> > +
> > +	if (data_len > DLN2_SPI_MAX_XFER_SIZE)
> > +		return -EINVAL;
> > +
> > +	/* Since this is a pseudo full-duplex communication, we're perfectly
> > +	 * safe to use the same buffer for both tx and rx. When DLN2 sends the
> > +	 * response back, with the rx data, we don't need the tx buffer anymore.
> > +	 */
> > +	tx = dln2->buf;
> > +	rx = dln2->buf;
> > +
> > +	tx->port = dln2->port;
> > +	tx->size = cpu_to_le16(data_len);
> > +	tx->attr = attr;
> > +
> > +	dln2_spi_copy_to_buf(tx->buf, tx_data, data_len, dln2->bpw);
> > +
> > +	tx_len = sizeof(*tx) + data_len - DLN2_SPI_MAX_XFER_SIZE;
> > +	rx_len = sizeof(*rx);
> > +
> > +	ret = dln2_transfer(dln2->pdev, DLN2_SPI_READ_WRITE, tx, tx_len,
> > +			    rx, &rx_len);
> > +	if (ret < 0)
> > +		return ret;
> > +	if (rx_len < sizeof(rx->size) + data_len)
> > +		return -EPROTO;
> > +	if (le16_to_cpu(rx->size) != data_len)
> > +		return -EPROTO;
> > +
> > +	dln2_spi_copy_from_buf(rx_data, rx->buf, data_len, dln2->bpw);
> > +
> > +	return 0;
> > +}
> > +
> > +/*
> > + * Read/Write wrapper. It will automatically split an operation into multiple
> > + * single ones due to device buffer constraints.
> > + */
> > +static int dln2_spi_rdwr(struct dln2_spi *dln2, const u8 *tx_data,
> > +			 u8 *rx_data, u16 data_len, u8 attr) {
> > +	int ret;
> > +	u16 len;
> > +	u8 temp_attr;
> > +	u16 remaining = data_len;
> > +	u16 offset;
> > +
> > +	do {
> > +		if (remaining > DLN2_SPI_MAX_XFER_SIZE) {
> > +			len = DLN2_SPI_MAX_XFER_SIZE;
> > +			temp_attr = DLN2_SPI_ATTR_LEAVE_SS_LOW;
> > +		} else {
> > +			len = remaining;
> > +			temp_attr = attr;
> > +		}
> > +
> > +		offset = data_len - remaining;
> > +
> > +		if (tx_data && rx_data)
> > +			ret = dln2_spi_read_write_one(dln2,
> > +						      tx_data + offset,
> > +						      rx_data + offset,
> > +						      len, temp_attr);
> > +		else if (tx_data)
> > +			ret = dln2_spi_write_one(dln2,
> > +						 tx_data + offset,
> > +						 len, temp_attr);
> > +		else if (rx_data)
> > +			ret = dln2_spi_read_one(dln2,
> > +						rx_data + offset,
> > +						len, temp_attr);
> > +		else
> > +			return -EINVAL;
> 
> Braces, please.
ok

> 
> > +
> > +		if (ret < 0)
> > +			return ret;
> > +
> > +		remaining -= len;
> > +	} while (remaining);
> > +
> > +	return 0;
> > +}
> > +
> > +static int dln2_spi_prepare_message(struct spi_master *master,
> > +				    struct spi_message *message)
> > +{
> > +	int ret;
> > +	struct dln2_spi *dln2 = spi_master_get_devdata(master);
> > +	struct spi_device *spi = message->spi;
> > +
> > +	if (dln2->cs != spi->chip_select) {
> > +		ret = dln2_spi_cs_set_one(dln2, spi->chip_select);
> > +		if (ret < 0)
> > +			return ret;
> > +
> > +		dln2->cs = spi->chip_select;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int dln2_spi_transfer_setup(struct dln2_spi *dln2, u32 speed,
> > +				   u8 bpw, u8 mode)
> > +{
> > +	int ret;
> > +	bool bus_setup_change;
> > +
> > +	bus_setup_change = dln2->speed != speed || dln2->mode != mode ||
> > +			   dln2->bpw != bpw;
> > +
> > +	if (!bus_setup_change)
> > +		return 0;
> > +
> > +	ret = dln2_spi_enable(dln2, false);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	if (dln2->speed != speed) {
> > +		ret = dln2_spi_set_speed(dln2, speed, NULL);
> > +		if (ret < 0)
> > +			return ret;
> > +
> > +		dln2->speed = speed;
> > +	}
> > +
> > +	if (dln2->mode != mode) {
> > +		ret = dln2_spi_set_mode(dln2, mode & 0x3);
> > +		if (ret < 0)
> > +			return ret;
> > +
> > +		dln2->mode = mode;
> > +	}
> > +
> > +	if (dln2->bpw != bpw) {
> > +		ret = dln2_spi_set_bpw(dln2, bpw);
> > +		if (ret < 0)
> > +			return ret;
> > +
> > +		dln2->bpw = bpw;
> > +	}
> > +
> > +	ret = dln2_spi_enable(dln2, true);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	return 0;
> > +}
> > +
> > +static int dln2_spi_transfer_one_message(struct spi_master *master,
> > +					 struct spi_message *msg)
> > +{
> > +	struct dln2_spi *dln2 = spi_master_get_devdata(master);
> > +	struct spi_device *spi = msg->spi;
> > +	struct spi_transfer *xfer;
> > +	int status;
> > +
> > +	list_for_each_entry(xfer, &msg->transfers, transfer_list) {
> > +		u8 attr = 0;
> > +
> > +		status = dln2_spi_transfer_setup(dln2, xfer->speed_hz,
> > +						 xfer->bits_per_word,
> > +						 spi->mode);
> > +		if (status < 0) {
> > +			dev_err(&dln2->pdev->dev, "Cannot setup transfer\n");
> > +			break;
> > +		}
> > +
> > +		if (!xfer->cs_change &&
> > +		    !list_is_last(&xfer->transfer_list, &msg->transfers))
> > +			attr = xfer->cs_change ? 0 : DLN2_SPI_ATTR_LEAVE_SS_LOW;
> > +
> > +		status = dln2_spi_rdwr(dln2, xfer->tx_buf, xfer->rx_buf,
> > +				       xfer->len, attr);
> > +		if (status < 0) {
> > +			dev_err(&dln2->pdev->dev, "write/read failed!\n");
> > +			break;
> > +		}
> > +
> > +		status = 0;
> > +	}
> > +
> > +	msg->status = status;
> > +	spi_finalize_current_message(master);
> > +	return status;
> > +}
> > +
> > +static int dln2_spi_probe(struct platform_device *pdev)
> > +{
> > +	struct spi_master *master;
> > +	struct dln2_spi *dln2;
> > +	struct dln2_platform_data *pdata = dev_get_platdata(&pdev->dev);
> > +	int ret;
> > +
> > +	master = spi_alloc_master(&pdev->dev, sizeof(*dln2));
> > +	if (!master)
> > +		return -ENOMEM;
> > +
> > +	platform_set_drvdata(pdev, master);
> > +
> > +	dln2 = spi_master_get_devdata(master);
> > +
> > +	dln2->buf = devm_kmalloc(&pdev->dev, DLN2_SPI_BUF_SIZE, GFP_KERNEL);
> > +	if (!dln2->buf) {
> > +		ret = -ENOMEM;
> > +		goto exit_free_master;
> > +	}
> > +
> > +	dln2->master = master;
> > +	dln2->pdev = pdev;
> > +	dln2->port = pdata->port;
> > +	/* cs number can never be 0xff, so the first transfer will set the cs */
> > +	dln2->cs = 0xff;
> > +
> > +	/* disable SPI module before continuing with the setup */
> > +	ret = dln2_spi_enable(dln2, false);
> > +	if (ret < 0) {
> > +		dev_err(&pdev->dev, "Failed to disable SPI module\n");
> > +		goto exit_free_master;
> > +	}
> > +
> > +	ret = dln2_spi_get_cs_num(dln2, &master->num_chipselect);
> > +	if (ret < 0) {
> > +		dev_err(&pdev->dev, "Failed to get number of CS pins\n");
> > +		goto exit_free_master;
> > +	}
> > +
> > +	ret = dln2_spi_get_speed_range(dln2,
> > +				       &master->min_speed_hz,
> > +				       &master->max_speed_hz);
> > +	if (ret < 0) {
> > +		dev_err(&pdev->dev, "Failed to read bus min/max freqs\n");
> > +		goto exit_free_master;
> > +	}
> > +
> > +	ret = dln2_spi_get_supported_frame_sizes(dln2,
> > +						 &master->bits_per_word_mask);
> > +	if (ret < 0) {
> > +		dev_err(&pdev->dev, "Failed to read supported frame sizes\n");
> > +		goto exit_free_master;
> > +	}
> > +
> > +	ret = dln2_spi_cs_enable_all(dln2, true);
> > +	if (ret < 0) {
> > +		dev_err(&pdev->dev, "Failed to enable CS pins\n");
> > +		goto exit_free_master;
> > +	}
> > +
> > +	master->bus_num = -1;
> > +	master->mode_bits = SPI_CPOL | SPI_CPHA;
> > +	master->prepare_message = dln2_spi_prepare_message;
> > +	master->transfer_one_message = dln2_spi_transfer_one_message;
> > +
> > +	/* enable SPI module, we're good to go */
> > +	ret = dln2_spi_enable(dln2, true);
> > +	if (ret < 0) {
> > +		dev_err(&pdev->dev, "Failed to enable SPI module\n");
> > +		goto exit_free_master;
> > +	}
> > +
> > +	ret = devm_spi_register_master(&pdev->dev, master);
> > +	if (ret < 0) {
> > +		dev_err(&pdev->dev, "Failed to register master\n");
> > +		goto exit_register;
> > +	}
> > +
> > +	return ret;
> > +
> > +exit_register:
> > +	if (dln2_spi_enable(dln2, false) < 0)
> > +		dev_err(&pdev->dev, "Failed to disable SPI module\n");
> > +exit_free_master:
> > +	spi_master_put(master);
> > +
> > +	return ret;
> > +}
> > +
> > +static int dln2_spi_remove(struct platform_device *pdev)
> > +{
> > +	struct spi_master *master = spi_master_get(platform_get_drvdata(pdev));
> > +	struct dln2_spi *dln2 = spi_master_get_devdata(master);
> > +
> > +	if (dln2_spi_enable(dln2, false) < 0)
> > +		dev_err(&pdev->dev, "Failed to disable SPI module\n");
> > +
> 
> Memory leak -- spi_master_put missing.
I used devm_spi_register_master(). No need for _put() here.
> 
> > +	return 0;
> > +}
> > +
> > +static struct platform_driver spi_dln2_driver = {
> > +	.driver.name	= "dln2-spi",
> > +	.probe		= dln2_spi_probe,
> > +	.remove		= dln2_spi_remove,
> > +};
> > +module_platform_driver(spi_dln2_driver);
> > +
> > +MODULE_DESCRIPTION("Driver for the Diolan DLN2 SPI master interface");
> > +MODULE_AUTHOR("Laurentiu Palcu <laurentiu.palcu@intel.com>");
> > +MODULE_LICENSE("GPL v2");
> > +MODULE_ALIAS("platform:dln2-spi");
> 
> Johan

Re: [PATCH v2 1/2] spi: add support for DLN-2 USB-SPI adapter

[Johan Havold]

On Thu, Nov 13, 2014 at 05:17:21PM +0200, Laurentiu Palcu wrote:
> Hi Johan,
> 
> On Thu, Nov 13, 2014 at 01:27:28PM +0100, Johan Havold wrote:
> > On Wed, Nov 12, 2014 at 03:38:09PM +0200, Laurentiu Palcu wrote:

> > > +struct dln2_spi {
> > > +	struct platform_device *pdev;
> > > +	struct spi_master *master;
> > > +	u8 port;
> > > +
> > > +	void *buf;
> > 
> > Add comment on what is protecting this buffer.
>
> No need to protect this buffer. First off, AFAICS, once we register the
> master, a message queue is initialized by the spi core and the entire
> communication with the SPI module goes through this queue. Secondly,
> every TX/RX SPI operation with the Diolan is split in 2 parts: command
> and response. Once we send the command out, the buffer can be safely
> reused for the response.

I didn't as for a lock, but for you to put a comment there explaining
why there's no need for one (e.g. buffer used in what functions that are
serialised by spi core).

> Also, I guess this answers a couple of your comments below too.

Not really. I still think you should use stack allocated buffer for
short control transfer.

> > > +
> > > +	u8 bpw;
> > > +	u32 speed;
> > > +	u16 mode;
> > > +	u8 cs;
>> > +};

> > > +/*
> > > + * Select/unselect multiple CS lines. The selected lines will be automatically
> > > + * toggled LOW/HIGH by the board firmware during transfers, provided they're
> > > + * enabled first.
> > > + *
> > > + * Ex: cs_mask = 0x03 -> CS0 & CS1 will be selected and the next WR/RD operation
> > 
> > Seems you have several comments that wrap at >80 columns (81 above).
>
> According to the kernel coding style, 80 columns is the limit, unless
> readability is affected. The line above does not exceed this limit. Also,
> checkpatch does not complain.

I noticed that checkpatch didn't complain, but 81 chars is still >80,
right? The newline counts as well (at least in vim).

> > > + *                       will toggle the lines LOW/HIGH automatically.
> > > + */
> > > +static int dln2_spi_cs_set(struct dln2_spi *dln2, u8 cs_mask)

[...]

> > > +static int dln2_spi_get_cs_num(struct dln2_spi *dln2, u16 *cs_num)
> > > +{
> > > +	int ret;
> > > +	struct {
> > > +		u8 port;
> > > +	} tx;
> > > +	struct {
> > > +		__le16 cs_count;
> > > +	} *rx = dln2->buf;
> > 
> > I don't think you want to use dln2->buf for all these small transfers.
> > And what would be protecting it from concurrent use?
> > 
> > Check this throughout.
>
> I answered this above.

So did I. Even though the transfer functions are serialised by spi-core
it's not obvious that all your helpers will be.

And since there's no gain from using the global buffer why not simply
use stack allocated ones here as well (e.g. for both tx and rx above)?

> > > +	unsigned rx_len = sizeof(*rx);
> > > +
> > > +	tx.port = dln2->port;
> > > +	ret = dln2_transfer(dln2->pdev, DLN2_SPI_GET_SS_COUNT, &tx, sizeof(tx),
> > > +			    rx, &rx_len);
> > > +	if (ret < 0)
> > > +		return ret;
> > > +	if (rx_len < sizeof(*rx))
> > > +		return -EPROTO;
> > > +
> > > +	*cs_num = le16_to_cpu(rx->cs_count);
> > > +
> > > +	dev_dbg(&dln2->pdev->dev, "cs_num = %d\n", *cs_num);
> > 
> > Use the spi device for device messages throughout.
>
> Apparently, I'm consistent with the other SPI master controller drivers.
> All of them use pdev->dev for printing messages. However, I tried it,
> and the prefix is "spi_master (null):" when master->dev is used,
> compared to "dln2-spi dln2-spi.2.auto:" when pdev->dev is used. It looks
> like master->dev.init_name is not set... :/

Ok.

> > > +
> > > +	return 0;
> > > +}

> > > +/*
> > > + * Copy the data to DLN2 buffer and change the alignment to LE, requested by
> > > + * DLN2 module. SPI core makes sure that the data length is a multiple of word
> > > + * size.
> > 
> > What about buffer alignment?
>
> Buffer alignment? Why should the buffer be aligned? Now that you
> mentioned it, I realized I should've used "byte order" instead of
> alignment in the comment above...

You can't just simply cast an unaligned buffer to a type that has a
minimum alignment requirement > 1. That's what the get/put_unaligned
helpers are for (see Documentation/unaligned-memory-access.txt).

Perhaps the buffer is properly aligned, just making sure you verified
this (e.g. what's the size of the header?).

And yes, fixing the wording would be nice as well.

> > > + */
> > > +static int dln2_spi_copy_to_buf(u8 *dln2_buf, const u8 *src, u16 len, u8 bpw)
> > > +{
> > > +#ifdef __LITTLE_ENDIAN
> > > +	memcpy(dln2_buf, src, len);
> > > +#else
> > > +	if (bpw <= 8)
> > > +		memcpy(dln2_buf, src, len);
> > 
> > Missing braces.
> ok
> 
> > 
> > > +	else if (bpw <= 16) {
> > > +		__le16 *d = (__le16 *) dln2_buf;
> > 
> > No spaces after casts.
> ok
> 
> > 
> > > +		u16 *s = (u16 *) src;
> > > +
> > > +		len = len / 2;
> > > +		while (len--)
> > > +			*d++ = cpu_to_le16p(s++);
> > > +	} else {
> > > +		__le32 *d = (__le32 *) dln2_buf;
> > > +		u32 *s = (u32 *) src;
> > > +
> > > +		len = len / 4;
> > > +		while (len--)
> > > +			*d++ = cpu_to_le32p(s++);
> > > +	}
> > > +#endif
> > > +
> > > +	return 0;
> > > +}

[...]

> > > +static int dln2_spi_probe(struct platform_device *pdev)
> > > +{
> > > +	struct spi_master *master;
> > > +	struct dln2_spi *dln2;
> > > +	struct dln2_platform_data *pdata = dev_get_platdata(&pdev->dev);
> > > +	int ret;
> > > +
> > > +	master = spi_alloc_master(&pdev->dev, sizeof(*dln2));
> > > +	if (!master)
> > > +		return -ENOMEM;
> > > +
> > > +	platform_set_drvdata(pdev, master);
> > > +
> > > +	dln2 = spi_master_get_devdata(master);
> > > +
> > > +	dln2->buf = devm_kmalloc(&pdev->dev, DLN2_SPI_BUF_SIZE, GFP_KERNEL);
> > > +	if (!dln2->buf) {
> > > +		ret = -ENOMEM;
> > > +		goto exit_free_master;
> > > +	}
> > > +
> > > +	dln2->master = master;
> > > +	dln2->pdev = pdev;
> > > +	dln2->port = pdata->port;
> > > +	/* cs number can never be 0xff, so the first transfer will set the cs */
> > > +	dln2->cs = 0xff;
> > > +
> > > +	/* disable SPI module before continuing with the setup */
> > > +	ret = dln2_spi_enable(dln2, false);
> > > +	if (ret < 0) {
> > > +		dev_err(&pdev->dev, "Failed to disable SPI module\n");
> > > +		goto exit_free_master;
> > > +	}
> > > +
> > > +	ret = dln2_spi_get_cs_num(dln2, &master->num_chipselect);
> > > +	if (ret < 0) {
> > > +		dev_err(&pdev->dev, "Failed to get number of CS pins\n");
> > > +		goto exit_free_master;
> > > +	}
> > > +
> > > +	ret = dln2_spi_get_speed_range(dln2,
> > > +				       &master->min_speed_hz,
> > > +				       &master->max_speed_hz);
> > > +	if (ret < 0) {
> > > +		dev_err(&pdev->dev, "Failed to read bus min/max freqs\n");
> > > +		goto exit_free_master;
> > > +	}
> > > +
> > > +	ret = dln2_spi_get_supported_frame_sizes(dln2,
> > > +						 &master->bits_per_word_mask);
> > > +	if (ret < 0) {
> > > +		dev_err(&pdev->dev, "Failed to read supported frame sizes\n");
> > > +		goto exit_free_master;
> > > +	}
> > > +
> > > +	ret = dln2_spi_cs_enable_all(dln2, true);
> > > +	if (ret < 0) {
> > > +		dev_err(&pdev->dev, "Failed to enable CS pins\n");
> > > +		goto exit_free_master;
> > > +	}
> > > +
> > > +	master->bus_num = -1;
> > > +	master->mode_bits = SPI_CPOL | SPI_CPHA;
> > > +	master->prepare_message = dln2_spi_prepare_message;
> > > +	master->transfer_one_message = dln2_spi_transfer_one_message;
> > > +
> > > +	/* enable SPI module, we're good to go */
> > > +	ret = dln2_spi_enable(dln2, true);
> > > +	if (ret < 0) {
> > > +		dev_err(&pdev->dev, "Failed to enable SPI module\n");
> > > +		goto exit_free_master;
> > > +	}
> > > +
> > > +	ret = devm_spi_register_master(&pdev->dev, master);
> > > +	if (ret < 0) {
> > > +		dev_err(&pdev->dev, "Failed to register master\n");
> > > +		goto exit_register;
> > > +	}
> > > +
> > > +	return ret;
> > > +
> > > +exit_register:
> > > +	if (dln2_spi_enable(dln2, false) < 0)
> > > +		dev_err(&pdev->dev, "Failed to disable SPI module\n");
> > > +exit_free_master:
> > > +	spi_master_put(master);
> > > +
> > > +	return ret;
> > > +}
> > > +
> > > +static int dln2_spi_remove(struct platform_device *pdev)
> > > +{
> > > +	struct spi_master *master = spi_master_get(platform_get_drvdata(pdev));
> > > +	struct dln2_spi *dln2 = spi_master_get_devdata(master);
> > > +
> > > +	if (dln2_spi_enable(dln2, false) < 0)
> > > +		dev_err(&pdev->dev, "Failed to disable SPI module\n");
> > > +
> > 
> > Memory leak -- spi_master_put missing.
>
> I used devm_spi_register_master(). No need for _put() here.

Ok, and master is freed as part of unregistration.

> > > +	return 0;
> > > +}

Johan

Re: [PATCH v2 1/2] spi: add support for DLN-2 USB-SPI adapter

[Laurentiu Palcu]

Hi Johan,

On Fri, Nov 14, 2014 at 11:56:45AM +0100, Johan Havold wrote:
> On Thu, Nov 13, 2014 at 05:17:21PM +0200, Laurentiu Palcu wrote:
> > Hi Johan,
> > 
> > On Thu, Nov 13, 2014 at 01:27:28PM +0100, Johan Havold wrote:
> > > On Wed, Nov 12, 2014 at 03:38:09PM +0200, Laurentiu Palcu wrote:
> 
> > > > +struct dln2_spi {
> > > > +	struct platform_device *pdev;
> > > > +	struct spi_master *master;
> > > > +	u8 port;
> > > > +
> > > > +	void *buf;
> > > 
> > > Add comment on what is protecting this buffer.
> >
> > No need to protect this buffer. First off, AFAICS, once we register the
> > master, a message queue is initialized by the spi core and the entire
> > communication with the SPI module goes through this queue. Secondly,
> > every TX/RX SPI operation with the Diolan is split in 2 parts: command
> > and response. Once we send the command out, the buffer can be safely
> > reused for the response.
> 
> I didn't as for a lock, but for you to put a comment there explaining
> why there's no need for one (e.g. buffer used in what functions that are
> serialised by spi core).
ok, I'll add a comment.

> 
> > Also, I guess this answers a couple of your comments below too.
> 
> Not really. I still think you should use stack allocated buffer for
> short control transfer.
> 
> > > > +
> > > > +	u8 bpw;
> > > > +	u32 speed;
> > > > +	u16 mode;
> > > > +	u8 cs;
> >> > +};
> 
> > > > +/*
> > > > + * Select/unselect multiple CS lines. The selected lines will be automatically
> > > > + * toggled LOW/HIGH by the board firmware during transfers, provided they're
> > > > + * enabled first.
> > > > + *
> > > > + * Ex: cs_mask = 0x03 -> CS0 & CS1 will be selected and the next WR/RD operation
> > > 
> > > Seems you have several comments that wrap at >80 columns (81 above).
> >
> > According to the kernel coding style, 80 columns is the limit, unless
> > readability is affected. The line above does not exceed this limit. Also,
> > checkpatch does not complain.
> 
> I noticed that checkpatch didn't complain, but 81 chars is still >80,
> right? The newline counts as well (at least in vim).
Isn't it all about visible characters? vim, which I use, does not count
the newline. I even have a special plugin for linux kernel development
that highlights the extra characters in red. Also, after looking at
other files in the kernel, it seems this file is compliant.

> 
> > > > + *                       will toggle the lines LOW/HIGH automatically.
> > > > + */
> > > > +static int dln2_spi_cs_set(struct dln2_spi *dln2, u8 cs_mask)
> 
> [...]
> 
> > > > +static int dln2_spi_get_cs_num(struct dln2_spi *dln2, u16 *cs_num)
> > > > +{
> > > > +	int ret;
> > > > +	struct {
> > > > +		u8 port;
> > > > +	} tx;
> > > > +	struct {
> > > > +		__le16 cs_count;
> > > > +	} *rx = dln2->buf;
> > > 
> > > I don't think you want to use dln2->buf for all these small transfers.
> > > And what would be protecting it from concurrent use?
> > > 
> > > Check this throughout.
> >
> > I answered this above.
> 
> So did I. Even though the transfer functions are serialised by spi-core
> it's not obvious that all your helpers will be.
I really looked this through and I couldn't find an example when one or
more helpers can be called in parallel. Most of the helpers are called
from the probe function and the rest are called from
dln2_spi_transfer_setup(), via dln2_spi_transfer_one_message(). The
dln2_spi_prepare_message() is called from SPI core's
spi_pump_messages(), just before calling transfer_one_message()... So,
it's still serial.

If there is a flaw in my logic, feel free to correct me.

> 
> And since there's no gain from using the global buffer why not simply
> use stack allocated ones here as well (e.g. for both tx and rx above)?
However, I will give this a shot though... Sounds reasonable and I could
probably lose the struct constructs too if the struct contains just
one field.

> > > > +
> > > > +	return 0;
> > > > +}
> 
> > > > +/*
> > > > + * Copy the data to DLN2 buffer and change the alignment to LE, requested by
> > > > + * DLN2 module. SPI core makes sure that the data length is a multiple of word
> > > > + * size.
> > > 
> > > What about buffer alignment?
> >
> > Buffer alignment? Why should the buffer be aligned? Now that you
> > mentioned it, I realized I should've used "byte order" instead of
> > alignment in the comment above...
> 
> You can't just simply cast an unaligned buffer to a type that has a
> minimum alignment requirement > 1. That's what the get/put_unaligned
> helpers are for (see Documentation/unaligned-memory-access.txt).
> 
> Perhaps the buffer is properly aligned, just making sure you verified
> this (e.g. what's the size of the header?).
> 
> And yes, fixing the wording would be nice as well.
>
You've got a good point here. I must've relied on the fact that x86 is
capable of dealing with unaligned access and didn't give it much thought
for other architectures... :/ Well, now that you raised the flag (thank
you for that), I checked the alignment for dln2 tx/rx buffer and appear
to be 4 byte aligned: for tx, the header is 4 bytes; for rx, it's 12
bytes (I'm counting also the response header that is stripped by
_dln2_transfer()).

The SPI transfer rx_buf and tx_buf, on the other hand, might not be
aligned so, thanks again for reminding me about this. :/

I'll send a v3 soon to address these.

laurentiu

Re: [PATCH v2 1/2] spi: add support for DLN-2 USB-SPI adapter

[Johan Hovold]

On Mon, Nov 17, 2014 at 12:33:01PM +0200, Laurentiu Palcu wrote:
> Hi Johan,
> 
> On Fri, Nov 14, 2014 at 11:56:45AM +0100, Johan Havold wrote:
> > On Thu, Nov 13, 2014 at 05:17:21PM +0200, Laurentiu Palcu wrote:
> > > Hi Johan,
> > > 
> > > On Thu, Nov 13, 2014 at 01:27:28PM +0100, Johan Havold wrote:
> > > > On Wed, Nov 12, 2014 at 03:38:09PM +0200, Laurentiu Palcu wrote:

> > I noticed that checkpatch didn't complain, but 81 chars is still >80,
> > right? The newline counts as well (at least in vim).
>
> Isn't it all about visible characters? vim, which I use, does not count
> the newline. I even have a special plugin for linux kernel development
> that highlights the extra characters in red. Also, after looking at
> other files in the kernel, it seems this file is compliant.

You're right. For some reason I had my vim textwidth set to 78. Sorry.

> > > > > +static int dln2_spi_get_cs_num(struct dln2_spi *dln2, u16 *cs_num)
> > > > > +{
> > > > > +	int ret;
> > > > > +	struct {
> > > > > +		u8 port;
> > > > > +	} tx;
> > > > > +	struct {
> > > > > +		__le16 cs_count;
> > > > > +	} *rx = dln2->buf;
> > > > 
> > > > I don't think you want to use dln2->buf for all these small transfers.
> > > > And what would be protecting it from concurrent use?
> > > > 
> > > > Check this throughout.
> > >
> > > I answered this above.
> > 
> > So did I. Even though the transfer functions are serialised by spi-core
> > it's not obvious that all your helpers will be.
>
> I really looked this through and I couldn't find an example when one or
> more helpers can be called in parallel. Most of the helpers are called
> from the probe function and the rest are called from
> dln2_spi_transfer_setup(), via dln2_spi_transfer_one_message(). The
> dln2_spi_prepare_message() is called from SPI core's
> spi_pump_messages(), just before calling transfer_one_message()... So,
> it's still serial.
> 
> If there is a flaw in my logic, feel free to correct me.

I'm not doubting that you got it right. My point is just that it's hard
for someone else to see whether you did (and someone adding a new helper
later on might not be as careful as you).

> > And since there's no gain from using the global buffer why not simply
> > use stack allocated ones here as well (e.g. for both tx and rx above)?
>
> However, I will give this a shot though... Sounds reasonable and I could
> probably lose the struct constructs too if the struct contains just
> one field.

You might want to keep the structs for single-field messages for
consistency reasons (also used in the other dln2 subdrivers).

Johan

Re: [PATCH v2 1/2] spi: add support for DLN-2 USB-SPI adapter

[Laurentiu Palcu]

On Mon, Nov 17, 2014 at 12:53:16PM +0100, Johan Hovold wrote:
> On Mon, Nov 17, 2014 at 12:33:01PM +0200, Laurentiu Palcu wrote:
> > Hi Johan,
> > 
> > On Fri, Nov 14, 2014 at 11:56:45AM +0100, Johan Havold wrote:
> > > On Thu, Nov 13, 2014 at 05:17:21PM +0200, Laurentiu Palcu wrote:
> > > > Hi Johan,
> > > > 
> > > > On Thu, Nov 13, 2014 at 01:27:28PM +0100, Johan Havold wrote:
> > > > > On Wed, Nov 12, 2014 at 03:38:09PM +0200, Laurentiu Palcu wrote:
> 
> > > And since there's no gain from using the global buffer why not simply
> > > use stack allocated ones here as well (e.g. for both tx and rx above)?
> >
> > However, I will give this a shot though... Sounds reasonable and I could
> > probably lose the struct constructs too if the struct contains just
> > one field.
> 
> You might want to keep the structs for single-field messages for
> consistency reasons (also used in the other dln2 subdrivers).
Ok, fair enough.

laurentiu

Re: [PATCH v2 1/2] spi: add support for DLN-2 USB-SPI adapter

[Mark Brown]

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

On Thu, Nov 13, 2014 at 01:27:28PM +0100, Johan Havold wrote:
> On Wed, Nov 12, 2014 at 03:38:09PM +0200, Laurentiu Palcu wrote:
> > This adds support for Diolan DLN2 USB-SPI adapter.
> > 
> > Information about the USB protocol interface can be found in the
> > Programmer's Reference Manual [1], see section 5.4.6 for the SPI
> > master module commands and responses.

John, please delete unneeded context from your replies - it makes it
much easier to find any content you've added.

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

[PATCH v2 2/2] mfd: dln2: add support for USB-SPI module

[Laurentiu Palcu]

Signed-off-by: Laurentiu Palcu <laurentiu.palcu@intel.com>
Acked-by: Mark Brown <broonie@kernel.org>
---
 drivers/mfd/dln2.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/mfd/dln2.c b/drivers/mfd/dln2.c
index 9765a17..0cdad2d 100644
--- a/drivers/mfd/dln2.c
+++ b/drivers/mfd/dln2.c
@@ -52,6 +52,7 @@ enum dln2_handle {
 	DLN2_HANDLE_CTRL,
 	DLN2_HANDLE_GPIO,
 	DLN2_HANDLE_I2C,
+	DLN2_HANDLE_SPI,
 	DLN2_HANDLES
 };
 
@@ -634,6 +635,12 @@ static struct dln2_platform_data dln2_pdata_i2c = {
 	.port = 0,
 };
 
+/* Only one SPI port supported */
+static struct dln2_platform_data dln2_pdata_spi = {
+	.handle = DLN2_HANDLE_SPI,
+	.port = 0,
+};
+
 static const struct mfd_cell dln2_devs[] = {
 	{
 		.name = "dln2-gpio",
@@ -645,6 +652,11 @@ static const struct mfd_cell dln2_devs[] = {
 		.platform_data = &dln2_pdata_i2c,
 		.pdata_size = sizeof(struct dln2_platform_data),
 	},
+	{
+		.name = "dln2-spi",
+		.platform_data = &dln2_pdata_spi,
+		.pdata_size = sizeof(struct dln2_platform_data),
+	},
 };
 
 static void dln2_disconnect(struct usb_interface *interface)
-- 
1.9.1

Re: [PATCH v2 2/2] mfd: dln2: add support for USB-SPI module

[Lee Jones]

On Wed, 12 Nov 2014, Laurentiu Palcu wrote:

> Signed-off-by: Laurentiu Palcu <laurentiu.palcu@intel.com>
> Acked-by: Mark Brown <broonie@kernel.org>
> ---
>  drivers/mfd/dln2.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)

For my own reference:

Acked-by: Lee Jones <lee.jones@linaro.org>

So what's the plan for this set?

> diff --git a/drivers/mfd/dln2.c b/drivers/mfd/dln2.c
> index 9765a17..0cdad2d 100644
> --- a/drivers/mfd/dln2.c
> +++ b/drivers/mfd/dln2.c
> @@ -52,6 +52,7 @@ enum dln2_handle {
>  	DLN2_HANDLE_CTRL,
>  	DLN2_HANDLE_GPIO,
>  	DLN2_HANDLE_I2C,
> +	DLN2_HANDLE_SPI,
>  	DLN2_HANDLES
>  };
>  
> @@ -634,6 +635,12 @@ static struct dln2_platform_data dln2_pdata_i2c = {
>  	.port = 0,
>  };
>  
> +/* Only one SPI port supported */
> +static struct dln2_platform_data dln2_pdata_spi = {
> +	.handle = DLN2_HANDLE_SPI,
> +	.port = 0,
> +};
> +
>  static const struct mfd_cell dln2_devs[] = {
>  	{
>  		.name = "dln2-gpio",
> @@ -645,6 +652,11 @@ static const struct mfd_cell dln2_devs[] = {
>  		.platform_data = &dln2_pdata_i2c,
>  		.pdata_size = sizeof(struct dln2_platform_data),
>  	},
> +	{
> +		.name = "dln2-spi",
> +		.platform_data = &dln2_pdata_spi,
> +		.pdata_size = sizeof(struct dln2_platform_data),
> +	},
>  };
>  
>  static void dln2_disconnect(struct usb_interface *interface)

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v2 2/2] mfd: dln2: add support for USB-SPI module

[Laurentiu Palcu]

Hi Lee,

On Tue, Nov 18, 2014 at 02:28:16PM +0000, Lee Jones wrote:
> On Wed, 12 Nov 2014, Laurentiu Palcu wrote:
> 
> > Signed-off-by: Laurentiu Palcu <laurentiu.palcu@intel.com>
> > Acked-by: Mark Brown <broonie@kernel.org>
> > ---
> >  drivers/mfd/dln2.c | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> 
> For my own reference:
> 
> Acked-by: Lee Jones <lee.jones@linaro.org>
> 
> So what's the plan for this set?

I sent a v3 a few hours ago that addresses Johan comments for the 1st
patch. I also included this patch since I didn't find it in mfd next
branches.

laurentiu