[PATCH v4 1/2] dt/bindings: Add bindings for PIC32 SPI peripheral

[PATCH v4 1/2] dt/bindings: Add bindings for PIC32 SPI peripheral

[Purna Chandra Mandal]

Document the devicetree bindings for the SPI peripheral found
on Microchip PIC32 class devices.

Signed-off-by: Purna Chandra Mandal <purna.mandal@microchip.com>
Acked-by: Rob Herring <robh@kernel.org>

---

Changes in v4: None
Changes in v3: None
Changes in v2:
 - fix indentation
 - add space after comma
 - moved 'cs-gpios' section under 'required' properties.

 .../bindings/spi/microchip,spi-pic32.txt           | 34 ++++++++++++++++++++++
 1 file changed, 34 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/spi/microchip,spi-pic32.txt

diff --git a/Documentation/devicetree/bindings/spi/microchip,spi-pic32.txt b/Documentation/devicetree/bindings/spi/microchip,spi-pic32.txt
new file mode 100644
index 0000000..79de379f
--- /dev/null
+++ b/Documentation/devicetree/bindings/spi/microchip,spi-pic32.txt
@@ -0,0 +1,34 @@
+Microchip PIC32 SPI Master controller
+
+Required properties:
+- compatible: Should be "microchip,pic32mzda-spi".
+- reg: Address and length of register space for the device.
+- interrupts: Should contain all three spi interrupts in sequence
+              of <fault-irq>, <receive-irq>, <transmit-irq>.
+- interrupt-names: Should be "fault", "rx", "tx" in order.
+- clocks: Phandle of the clock generating SPI clock on the bus.
+- clock-names: Should be "mck0".
+- cs-gpios: Specifies the gpio pins to be used for chipselects.
+            See: Documentation/devicetree/bindings/spi/spi-bus.txt
+
+Optional properties:
+- dmas: Two or more DMA channel specifiers following the convention outlined
+        in Documentation/devicetree/bindings/dma/dma.txt
+- dma-names: Names for the dma channels. There must be at least one channel
+             named "spi-tx" for transmit and named "spi-rx" for receive.
+
+Example:
+
+spi1: spi@1f821000 {
+        compatible = "microchip,pic32mzda-spi";
+        reg = <0x1f821000 0x200>;
+        interrupts = <109 IRQ_TYPE_LEVEL_HIGH>,
+                     <110 IRQ_TYPE_LEVEL_HIGH>,
+                     <111 IRQ_TYPE_LEVEL_HIGH>;
+        interrupt-names = "fault", "rx", "tx";
+        clocks = <&PBCLK2>;
+        clock-names = "mck0";
+        cs-gpios = <&gpio3 4 GPIO_ACTIVE_LOW>;
+        dmas = <&dma 134>, <&dma 135>;
+        dma-names = "spi-rx", "spi-tx";
+};
-- 
1.8.3.1

[PATCH v4 1/2] dt/bindings: Add bindings for PIC32 SPI peripheral

[Purna Chandra Mandal]

Document the devicetree bindings for the SPI peripheral found
on Microchip PIC32 class devices.

Signed-off-by: Purna Chandra Mandal <purna.mandal-UWL1GkI3JZL3oGB3hsPCZA@public.gmane.org>
Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>

---

Changes in v4: None
Changes in v3: None
Changes in v2:
 - fix indentation
 - add space after comma
 - moved 'cs-gpios' section under 'required' properties.

 .../bindings/spi/microchip,spi-pic32.txt           | 34 ++++++++++++++++++++++
 1 file changed, 34 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/spi/microchip,spi-pic32.txt

diff --git a/Documentation/devicetree/bindings/spi/microchip,spi-pic32.txt b/Documentation/devicetree/bindings/spi/microchip,spi-pic32.txt
new file mode 100644
index 0000000..79de379f
--- /dev/null
+++ b/Documentation/devicetree/bindings/spi/microchip,spi-pic32.txt
@@ -0,0 +1,34 @@
+Microchip PIC32 SPI Master controller
+
+Required properties:
+- compatible: Should be "microchip,pic32mzda-spi".
+- reg: Address and length of register space for the device.
+- interrupts: Should contain all three spi interrupts in sequence
+              of <fault-irq>, <receive-irq>, <transmit-irq>.
+- interrupt-names: Should be "fault", "rx", "tx" in order.
+- clocks: Phandle of the clock generating SPI clock on the bus.
+- clock-names: Should be "mck0".
+- cs-gpios: Specifies the gpio pins to be used for chipselects.
+            See: Documentation/devicetree/bindings/spi/spi-bus.txt
+
+Optional properties:
+- dmas: Two or more DMA channel specifiers following the convention outlined
+        in Documentation/devicetree/bindings/dma/dma.txt
+- dma-names: Names for the dma channels. There must be at least one channel
+             named "spi-tx" for transmit and named "spi-rx" for receive.
+
+Example:
+
+spi1: spi@1f821000 {
+        compatible = "microchip,pic32mzda-spi";
+        reg = <0x1f821000 0x200>;
+        interrupts = <109 IRQ_TYPE_LEVEL_HIGH>,
+                     <110 IRQ_TYPE_LEVEL_HIGH>,
+                     <111 IRQ_TYPE_LEVEL_HIGH>;
+        interrupt-names = "fault", "rx", "tx";
+        clocks = <&PBCLK2>;
+        clock-names = "mck0";
+        cs-gpios = <&gpio3 4 GPIO_ACTIVE_LOW>;
+        dmas = <&dma 134>, <&dma 135>;
+        dma-names = "spi-rx", "spi-tx";
+};
-- 
1.8.3.1

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 1/2] dt/bindings: Add bindings for PIC32 SPI peripheral

[Purna Chandra Mandal]

Document the devicetree bindings for the SPI peripheral found
on Microchip PIC32 class devices.

Signed-off-by: Purna Chandra Mandal <purna.mandal-UWL1GkI3JZL3oGB3hsPCZA@public.gmane.org>
Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>

---

Changes in v4: None
Changes in v3: None
Changes in v2:
 - fix indentation
 - add space after comma
 - moved 'cs-gpios' section under 'required' properties.

 .../bindings/spi/microchip,spi-pic32.txt           | 34 ++++++++++++++++++++++
 1 file changed, 34 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/spi/microchip,spi-pic32.txt

diff --git a/Documentation/devicetree/bindings/spi/microchip,spi-pic32.txt b/Documentation/devicetree/bindings/spi/microchip,spi-pic32.txt
new file mode 100644
index 0000000..79de379f
--- /dev/null
+++ b/Documentation/devicetree/bindings/spi/microchip,spi-pic32.txt
@@ -0,0 +1,34 @@
+Microchip PIC32 SPI Master controller
+
+Required properties:
+- compatible: Should be "microchip,pic32mzda-spi".
+- reg: Address and length of register space for the device.
+- interrupts: Should contain all three spi interrupts in sequence
+              of <fault-irq>, <receive-irq>, <transmit-irq>.
+- interrupt-names: Should be "fault", "rx", "tx" in order.
+- clocks: Phandle of the clock generating SPI clock on the bus.
+- clock-names: Should be "mck0".
+- cs-gpios: Specifies the gpio pins to be used for chipselects.
+            See: Documentation/devicetree/bindings/spi/spi-bus.txt
+
+Optional properties:
+- dmas: Two or more DMA channel specifiers following the convention outlined
+        in Documentation/devicetree/bindings/dma/dma.txt
+- dma-names: Names for the dma channels. There must be at least one channel
+             named "spi-tx" for transmit and named "spi-rx" for receive.
+
+Example:
+
+spi1: spi@1f821000 {
+        compatible = "microchip,pic32mzda-spi";
+        reg = <0x1f821000 0x200>;
+        interrupts = <109 IRQ_TYPE_LEVEL_HIGH>,
+                     <110 IRQ_TYPE_LEVEL_HIGH>,
+                     <111 IRQ_TYPE_LEVEL_HIGH>;
+        interrupt-names = "fault", "rx", "tx";
+        clocks = <&PBCLK2>;
+        clock-names = "mck0";
+        cs-gpios = <&gpio3 4 GPIO_ACTIVE_LOW>;
+        dmas = <&dma 134>, <&dma 135>;
+        dma-names = "spi-rx", "spi-tx";
+};
-- 
1.8.3.1

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 1/2] dt/bindings: Add bindings for PIC32 SPI peripheral

[Purna Chandra Mandal]

Document the devicetree bindings for the SPI peripheral found
on Microchip PIC32 class devices.

Signed-off-by: Purna Chandra Mandal <purna.mandal@microchip.com>
Acked-by: Rob Herring <robh@kernel.org>

---

Changes in v4: None
Changes in v3: None
Changes in v2:
 - fix indentation
 - add space after comma
 - moved 'cs-gpios' section under 'required' properties.

 .../bindings/spi/microchip,spi-pic32.txt           | 34 ++++++++++++++++++++++
 1 file changed, 34 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/spi/microchip,spi-pic32.txt

diff --git a/Documentation/devicetree/bindings/spi/microchip,spi-pic32.txt b/Documentation/devicetree/bindings/spi/microchip,spi-pic32.txt
new file mode 100644
index 0000000..79de379f
--- /dev/null
+++ b/Documentation/devicetree/bindings/spi/microchip,spi-pic32.txt
@@ -0,0 +1,34 @@
+Microchip PIC32 SPI Master controller
+
+Required properties:
+- compatible: Should be "microchip,pic32mzda-spi".
+- reg: Address and length of register space for the device.
+- interrupts: Should contain all three spi interrupts in sequence
+              of <fault-irq>, <receive-irq>, <transmit-irq>.
+- interrupt-names: Should be "fault", "rx", "tx" in order.
+- clocks: Phandle of the clock generating SPI clock on the bus.
+- clock-names: Should be "mck0".
+- cs-gpios: Specifies the gpio pins to be used for chipselects.
+            See: Documentation/devicetree/bindings/spi/spi-bus.txt
+
+Optional properties:
+- dmas: Two or more DMA channel specifiers following the convention outlined
+        in Documentation/devicetree/bindings/dma/dma.txt
+- dma-names: Names for the dma channels. There must be at least one channel
+             named "spi-tx" for transmit and named "spi-rx" for receive.
+
+Example:
+
+spi1: spi@1f821000 {
+        compatible = "microchip,pic32mzda-spi";
+        reg = <0x1f821000 0x200>;
+        interrupts = <109 IRQ_TYPE_LEVEL_HIGH>,
+                     <110 IRQ_TYPE_LEVEL_HIGH>,
+                     <111 IRQ_TYPE_LEVEL_HIGH>;
+        interrupt-names = "fault", "rx", "tx";
+        clocks = <&PBCLK2>;
+        clock-names = "mck0";
+        cs-gpios = <&gpio3 4 GPIO_ACTIVE_LOW>;
+        dmas = <&dma 134>, <&dma 135>;
+        dma-names = "spi-rx", "spi-tx";
+};
-- 
1.8.3.1

[PATCH v4 2/2] spi: spi-pic32: Add PIC32 SPI master driver

[Purna Chandra Mandal]

The PIC32 SPI driver is capable of performing SPI transfers
using PIO or external DMA engine. GPIO controlled /CS support
is made default in the driver for correct operation of the
controller. This can be enabled by adding "cs-gpios" property
of the SPI node in board dts file.

Signed-off-by: Purna Chandra Mandal <purna.mandal@microchip.com>


---

Changes in v4:
- report error and bailout in case of missing irq(s).
- remove fallback to PIO mode in case of failure in DMA transfer.
- remove polling based PIO completely.
- drop spinlock
- update error message and comments.

Changes in v3:
- drop 'owner' field in driver struct.
- fix compilation warning in dma_addr_t print.

Changes in v2:
- drop internal function drain_rx_fifo()
- merge wrapper functions with callers wherever applicable
- sort includes alphabetically
- Kconfig: sort SPI_PIC32 alphabetically and add || COMPILE_TEST
- Makefile: sort SPI_PIC32 alphabetically
- replace cpu_relax() with ndelay() in disable_chip()
- drop spi controller driven /CS management completely, use only
  GPIO controller /CS.
- rename function names starting with 'spi' to avoid namespace conflict
- use .one_transfer() callback instead of .one_message().
- drop /CS assert-deassert functions as core provides those.
- fix race while completing transfer before disabling interrupt.

 drivers/spi/Kconfig     |   6 +
 drivers/spi/Makefile    |   1 +
 drivers/spi/spi-pic32.c | 958 ++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 965 insertions(+)
 create mode 100644 drivers/spi/spi-pic32.c

diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index 7706416..22f973f 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -396,6 +396,12 @@ config SPI_ORION
 	help
 	  This enables using the SPI master controller on the Orion chips.
 
+config SPI_PIC32
+	tristate "Microchip PIC32 series SPI"
+	depends on MACH_PIC32 || COMPILE_TEST
+	help
+	  SPI driver for Microchip PIC32 SPI master controller.
+
 config SPI_PL022
 	tristate "ARM AMBA PL022 SSP controller"
 	depends on ARM_AMBA
diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
index 8991ffc..1bcb417 100644
--- a/drivers/spi/Makefile
+++ b/drivers/spi/Makefile
@@ -60,6 +60,7 @@ obj-$(CONFIG_SPI_OMAP_100K)		+= spi-omap-100k.o
 obj-$(CONFIG_SPI_OMAP24XX)		+= spi-omap2-mcspi.o
 obj-$(CONFIG_SPI_TI_QSPI)		+= spi-ti-qspi.o
 obj-$(CONFIG_SPI_ORION)			+= spi-orion.o
+obj-$(CONFIG_SPI_PIC32)			+= spi-pic32.o
 obj-$(CONFIG_SPI_PL022)			+= spi-pl022.o
 obj-$(CONFIG_SPI_PPC4xx)		+= spi-ppc4xx.o
 spi-pxa2xx-platform-objs		:= spi-pxa2xx.o
diff --git a/drivers/spi/spi-pic32.c b/drivers/spi/spi-pic32.c
new file mode 100644
index 0000000..78776f1
--- /dev/null
+++ b/drivers/spi/spi-pic32.c
@@ -0,0 +1,958 @@
+/*
+ * Microchip PIC32 SPI controller driver.
+ *
+ * Purna Chandra Mandal <purna.mandal@microchip.com>
+ * Copyright (c) 2016, Microchip Technology Inc.
+ *
+ * This program is free software; you can distribute it and/or modify it
+ * under the terms of the GNU General Public License (Version 2) as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * for more details.
+ */
+
+#include <linux/clk.h>
+#include <linux/clkdev.h>
+#include <linux/delay.h>
+#include <linux/dmaengine.h>
+#include <linux/dma-mapping.h>
+#include <linux/highmem.h>
+#include <linux/module.h>
+#include <linux/io.h>
+#include <linux/interrupt.h>
+#include <linux/of.h>
+#include <linux/of_irq.h>
+#include <linux/of_gpio.h>
+#include <linux/of_address.h>
+#include <linux/platform_device.h>
+#include <linux/spi/spi.h>
+
+/* SPI controller registers */
+struct pic32_spi_regs {
+	u32 ctrl;
+	u32 ctrl_clr;
+	u32 ctrl_set;
+	u32 ctrl_inv;
+	u32 status;
+	u32 status_clr;
+	u32 status_set;
+	u32 status_inv;
+	u32 buf;
+	u32 dontuse[3];
+	u32 baud;
+	u32 dontuse2[3];
+	u32 ctrl2;
+	u32 ctrl2_clr;
+	u32 ctrl2_set;
+	u32 ctrl2_inv;
+};
+
+/* Bit fields in SPI Control Register */
+#define CTRL_RX_INT_SHIFT	0  /* Rx interrupt generation */
+#define  RX_FIFO_EMTPY		0
+#define  RX_FIFO_NOT_EMPTY	1 /* not empty */
+#define  RX_FIFO_HALF_FULL	2 /* full by half or more */
+#define  RX_FIFO_FULL		3 /* completely full */
+
+#define CTRL_TX_INT_SHIFT	2  /* TX interrupt generation */
+#define  TX_FIFO_ALL_EMPTY	0 /* completely empty */
+#define  TX_FIFO_EMTPY		1 /* empty */
+#define  TX_FIFO_HALF_EMPTY	2 /* empty by half or more */
+#define  TX_FIFO_NOT_FULL	3 /* atleast one empty */
+
+#define CTRL_MSTEN	BIT(5) /* enable master mode */
+#define CTRL_CKP	BIT(6) /* active low */
+#define CTRL_CKE	BIT(8) /* Tx on falling edge */
+#define CTRL_SMP	BIT(9) /* Rx at middle or end of tx */
+#define CTRL_BPW_MASK	0x03   /* bits per word/sample */
+#define CTRL_BPW_SHIFT	10
+#define  PIC32_BPW_8	0
+#define  PIC32_BPW_16	1
+#define  PIC32_BPW_32	2
+#define CTRL_ON		BIT(15) /* enable macro */
+#define CTRL_ENHBUF	BIT(16) /* enable enhanced buffering */
+#define CTRL_MCLKSEL	BIT(23) /* select clock source */
+#define CTRL_MSSEN	BIT(28) /* macro driven /SS */
+#define CTRL_FRMEN	BIT(31) /* enable framing mode */
+
+/* Bit fields in SPI Status Register */
+#define STAT_RF_EMPTY	BIT(5) /* RX Fifo empty */
+#define STAT_RX_OV	BIT(6) /* err, s/w needs to clear */
+#define STAT_TX_UR	BIT(8) /* UR in Framed SPI modes */
+#define STAT_FRM_ERR	BIT(12) /* Multiple Frame Sync pulse */
+#define STAT_TF_LVL_MASK	0x1F
+#define STAT_TF_LVL_SHIFT	16
+#define STAT_RF_LVL_MASK	0x1F
+#define STAT_RF_LVL_SHIFT	24
+
+/* Bit fields in Baud Register */
+#define BAUD_MASK		0x1ff
+
+/* Bit fields in Control2 Register */
+#define CTRL2_TX_UR_EN		BIT(10) /* Enable int on Tx under-run */
+#define CTRL2_RX_OV_EN		BIT(11) /* Enable int on Rx over-run */
+#define CTRL2_FRM_ERR_EN	BIT(12) /* Enable frame err int */
+
+/* Minimum DMA transfer size */
+#define PIC32_DMA_LEN_MIN	64
+
+struct pic32_spi {
+	dma_addr_t		dma_base;
+	struct pic32_spi_regs __iomem *regs;
+	int			fault_irq;
+	int			rx_irq;
+	int			tx_irq;
+	u32			fifo_n_byte; /* FIFO depth in bytes */
+	struct clk		*clk;
+	struct spi_master	*master;
+	/* Current SPI device specific */
+	struct spi_device	*spi_dev;
+	u32			speed_hz; /* spi-clk rate */
+	u32			mode;
+#define PIC32F_DMA_PREP		0 /* DMA chnls configured */
+#define PIC32F_DMA_ISSUED	1 /* buffer mapped and DMA issued */
+	unsigned long		flags;
+	u32			fifo_n_elm; /* FIFO depth in words */
+	enum dma_slave_buswidth	dma_width;
+	/* Current transfer state */
+	struct spi_message	*mesg;
+	const void		*tx;
+	const void		*tx_end;
+	const void		*rx;
+	const void		*rx_end;
+	int			len;
+	struct completion	xfer_done;
+	/* SPI fifo accessors used in PIO transfer */
+	void (*rx_fifo)(struct pic32_spi *);
+	void (*tx_fifo)(struct pic32_spi *);
+};
+
+static bool pic32_spi_debug;
+
+static void pic32_spi_enable_chip(struct pic32_spi *pic32s)
+{
+	writel(CTRL_ON, &pic32s->regs->ctrl_set);
+}
+
+static void pic32_spi_disable_chip(struct pic32_spi *pic32s)
+{
+	writel(CTRL_ON, &pic32s->regs->ctrl_clr);
+
+	/* avoid read/write to SPI register at immediate next CPU clock */
+	ndelay(20);
+}
+
+static void pic32_spi_set_clk_rate(struct pic32_spi *pic32s, u32 spi_ck)
+{
+	u32 div;
+
+	/* div = (clk_in / (2 * spi_ck)) - 1 */
+	div = DIV_ROUND_CLOSEST(clk_get_rate(pic32s->clk), 2 * spi_ck) - 1;
+
+	writel(div & BAUD_MASK, &pic32s->regs->baud);
+}
+
+static u32 pic32_rx_fifo_level(struct pic32_spi *pic32s)
+{
+	u32 sr = readl(&pic32s->regs->status);
+
+	return (sr >> STAT_RF_LVL_SHIFT) & STAT_RF_LVL_MASK;
+}
+
+static u32 pic32_tx_fifo_level(struct pic32_spi *pic32s)
+{
+	u32 sr = readl(&pic32s->regs->status);
+
+	return (sr >> STAT_TF_LVL_SHIFT) & STAT_TF_LVL_MASK;
+}
+
+/* Return the max entries we can fill into tx fifo */
+static u32 pic32_tx_max(struct pic32_spi *pic32s, int n_bytes)
+{
+	u32 tx_left, tx_room, rxtx_gap;
+
+	tx_left = (pic32s->tx_end - pic32s->tx) / n_bytes;
+	tx_room = pic32s->fifo_n_elm - pic32_tx_fifo_level(pic32s);
+
+	/*
+	 * Another concern is about the tx/rx mismatch, we
+	 * though to use (pic32s->fifo_n_byte - rxfl - txfl) as
+	 * one maximum value for tx, but it doesn't cover the
+	 * data which is out of tx/rx fifo and inside the
+	 * shift registers. So a ctrl from sw point of
+	 * view is taken.
+	 */
+	rxtx_gap = ((pic32s->rx_end - pic32s->rx) -
+		    (pic32s->tx_end - pic32s->tx)) / n_bytes;
+	return min3(tx_left, tx_room, (u32)(pic32s->fifo_n_elm - rxtx_gap));
+}
+
+/* Return the max entries we should read out of rx fifo */
+static u32 pic32_rx_max(struct pic32_spi *pic32s, int n_bytes)
+{
+	u32 rx_left = (pic32s->rx_end - pic32s->rx) / n_bytes;
+
+	return min_t(u32, rx_left, pic32_rx_fifo_level(pic32s));
+}
+
+#define BUILD_SPI_FIFO_RW(__name, __type, __bwl)		\
+static void pic32_spi_rx_##__name(struct pic32_spi *pic32s)	\
+{								\
+	__type v;						\
+	u32 mx = pic32_rx_max(pic32s, sizeof(__type));		\
+	for (; mx; mx--) {					\
+		v = read##__bwl(&pic32s->regs->buf);		\
+		if (pic32s->rx_end - pic32s->len)		\
+			*(__type *)(pic32s->rx) = v;		\
+		pic32s->rx += sizeof(__type);			\
+	}							\
+}								\
+								\
+static void pic32_spi_tx_##__name(struct pic32_spi *pic32s)	\
+{								\
+	__type v;						\
+	u32 mx = pic32_tx_max(pic32s, sizeof(__type));		\
+	for (; mx ; mx--) {					\
+		v = (__type)~0U;				\
+		if (pic32s->tx_end - pic32s->len)		\
+			v = *(__type *)(pic32s->tx);		\
+		write##__bwl(v, &pic32s->regs->buf);		\
+		pic32s->tx += sizeof(__type);			\
+	}							\
+}
+
+BUILD_SPI_FIFO_RW(byte, u8, b);
+BUILD_SPI_FIFO_RW(word, u16, w);
+BUILD_SPI_FIFO_RW(dword, u32, l);
+
+static void pic32_err_stop(struct pic32_spi *pic32s, const char *msg)
+{
+	/* Stop h/w */
+	pic32_spi_disable_chip(pic32s);
+
+	/* disable all interrupts */
+	disable_irq_nosync(pic32s->fault_irq);
+	disable_irq_nosync(pic32s->rx_irq);
+	disable_irq_nosync(pic32s->tx_irq);
+
+	/* Show err message and abort xfer with err */
+	dev_err(&pic32s->master->dev, "%s\n", msg);
+	pic32s->mesg->status = -EIO;
+	complete(&pic32s->xfer_done);
+
+	dev_err(&pic32s->master->dev, "irq: disable all\n");
+}
+
+static irqreturn_t pic32_spi_fault_irq(int irq, void *dev_id)
+{
+	struct pic32_spi *pic32s = dev_id;
+	u32 status;
+
+	status = readl(&pic32s->regs->status);
+
+	/* Error handling */
+	if (status & (STAT_RX_OV | STAT_TX_UR)) {
+		writel(STAT_RX_OV, &pic32s->regs->status_clr);
+		writel(STAT_TX_UR, &pic32s->regs->status_clr);
+		pic32_err_stop(pic32s, "err_irq: fifo ov/ur-run\n");
+		return IRQ_HANDLED;
+	}
+
+	if (status & STAT_FRM_ERR) {
+		pic32_err_stop(pic32s, "err_irq: frame error");
+		return IRQ_HANDLED;
+	}
+
+	if (!pic32s->mesg) {
+		pic32_err_stop(pic32s, "err_irq: mesg error");
+		return IRQ_NONE;
+	}
+
+	return IRQ_NONE;
+}
+
+static irqreturn_t pic32_spi_rx_irq(int irq, void *dev_id)
+{
+	struct pic32_spi *pic32s = dev_id;
+
+	pic32s->rx_fifo(pic32s);
+
+	/* rx complete? */
+	if (pic32s->rx_end == pic32s->rx) {
+		/* mask & disable all interrupts */
+		disable_irq_nosync(pic32s->fault_irq);
+		disable_irq_nosync(pic32s->rx_irq);
+
+		/* complete current xfer */
+		complete(&pic32s->xfer_done);
+	}
+
+	return IRQ_HANDLED;
+}
+
+static irqreturn_t pic32_spi_tx_irq(int irq, void *dev_id)
+{
+	struct pic32_spi *pic32s = dev_id;
+
+	pic32s->tx_fifo(pic32s);
+
+	/* tx complete? mask and disable tx interrupt */
+	if (pic32s->tx_end == pic32s->tx)
+		disable_irq_nosync(pic32s->tx_irq);
+
+	return IRQ_HANDLED;
+}
+
+static void pic32_spi_dma_rx_notify(void *data)
+{
+	struct pic32_spi *pic32s = data;
+
+	complete(&pic32s->xfer_done);
+}
+
+static int pic32_spi_dma_transfer(struct pic32_spi *pic32s,
+				  struct spi_transfer *xfer)
+{
+	struct spi_master *master = pic32s->master;
+	struct dma_async_tx_descriptor *desc_rx;
+	struct dma_async_tx_descriptor *desc_tx;
+	dma_cookie_t cookie;
+	int ret;
+
+	if (!master->dma_rx || !master->dma_tx)
+		return -ENODEV;
+
+	desc_rx = dmaengine_prep_slave_sg(master->dma_rx,
+					  xfer->rx_sg.sgl,
+					  xfer->rx_sg.nents,
+					  DMA_FROM_DEVICE,
+					  DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
+	if (!desc_rx) {
+		ret = -EINVAL;
+		goto err_dma;
+	}
+
+	desc_tx = dmaengine_prep_slave_sg(master->dma_tx,
+					  xfer->tx_sg.sgl,
+					  xfer->tx_sg.nents,
+					  DMA_TO_DEVICE,
+					  DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
+	if (!desc_tx) {
+		ret = -EINVAL;
+		goto err_dma;
+	}
+
+	dev_vdbg(&master->dev, "dma_xfer %p: len %u, tx %p(0x%llx), rx %p(0x%llx)\n",
+		 xfer, xfer->len, xfer->tx_buf, (u64)xfer->tx_dma,
+		 xfer->rx_buf, (u64)xfer->rx_dma);
+
+	/* Put callback on the RX transfer, that should finish last */
+	desc_rx->callback = pic32_spi_dma_rx_notify;
+	desc_rx->callback_param = pic32s;
+
+	cookie = dmaengine_submit(desc_rx);
+	ret = dma_submit_error(cookie);
+	if (ret)
+		goto err_dma;
+
+	cookie = dmaengine_submit(desc_tx);
+	ret = dma_submit_error(cookie);
+	if (ret)
+		goto err_dma_tx;
+
+	dma_async_issue_pending(master->dma_rx);
+	dma_async_issue_pending(master->dma_tx);
+
+	return 0;
+
+err_dma_tx:
+	dmaengine_terminate_all(master->dma_rx);
+err_dma:
+	return ret;
+}
+
+static int pic32_spi_dma_config(struct pic32_spi *pic32s, u32 dma_width)
+{
+	int buf_offset = offsetof(struct pic32_spi_regs, buf);
+	struct spi_master *master = pic32s->master;
+	struct dma_slave_config cfg;
+	int err;
+
+	cfg.device_fc = true;
+	cfg.dst_addr = pic32s->dma_base + buf_offset;
+	cfg.dst_addr_width = dma_width;
+	cfg.dst_maxburst = pic32s->fifo_n_elm / 2; /* drain one-half */
+	cfg.src_addr = pic32s->dma_base + buf_offset;
+	cfg.src_addr_width = dma_width;
+	cfg.src_maxburst = pic32s->fifo_n_elm / 2; /* fill one-half */
+	cfg.slave_id = pic32s->tx_irq;
+	cfg.direction = DMA_MEM_TO_DEV;
+	err = dmaengine_slave_config(master->dma_tx, &cfg);
+	if (err) {
+		dev_err(&master->dev, "configure tx dma channel failed\n");
+		return err;
+	}
+
+	cfg.slave_id = pic32s->rx_irq;
+	cfg.direction = DMA_DEV_TO_MEM;
+	err = dmaengine_slave_config(master->dma_rx, &cfg);
+	if (err)
+		dev_err(&master->dev, "configure rx dma channel failed\n");
+
+	return err;
+}
+
+static void pic32_spi_set_word_size(struct pic32_spi *pic32s, u8 bits_per_word)
+{
+	enum dma_slave_buswidth dma_buswidth;
+	u32 spi_buswidth, v;
+
+	switch (bits_per_word) {
+	default:
+	case 8:
+		pic32s->rx_fifo = pic32_spi_rx_byte;
+		pic32s->tx_fifo = pic32_spi_tx_byte;
+		spi_buswidth = PIC32_BPW_8;
+		dma_buswidth = DMA_SLAVE_BUSWIDTH_1_BYTE;
+		break;
+	case 16:
+		pic32s->rx_fifo = pic32_spi_rx_word;
+		pic32s->tx_fifo = pic32_spi_tx_word;
+		spi_buswidth = PIC32_BPW_16;
+		dma_buswidth = DMA_SLAVE_BUSWIDTH_2_BYTES;
+		break;
+	case 32:
+		pic32s->rx_fifo = pic32_spi_rx_dword;
+		pic32s->tx_fifo = pic32_spi_tx_dword;
+		spi_buswidth = PIC32_BPW_32;
+		dma_buswidth = DMA_SLAVE_BUSWIDTH_4_BYTES;
+		break;
+	}
+
+	/* calculate maximum number of words fifos can hold */
+	pic32s->fifo_n_elm = DIV_ROUND_UP(pic32s->fifo_n_byte,
+					  bits_per_word / 8);
+	/* set word size */
+	v = readl(&pic32s->regs->ctrl);
+	v &= ~(CTRL_BPW_MASK << CTRL_BPW_SHIFT);
+	v |= spi_buswidth << CTRL_BPW_SHIFT;
+	writel(v, &pic32s->regs->ctrl);
+
+	/* re-configure dma width, if required */
+	if (test_bit(PIC32F_DMA_PREP, &pic32s->flags) &&
+	    (dma_buswidth != pic32s->dma_width)) {
+		pic32_spi_dma_config(pic32s, dma_buswidth);
+		pic32s->dma_width = dma_buswidth;
+	}
+}
+
+static int pic32_spi_prepare_message(struct spi_master *master,
+				     struct spi_message *msg)
+{
+	struct spi_device *spi = msg->spi;
+	struct spi_transfer *xfer;
+	struct pic32_spi *pic32s;
+
+	pic32s = spi_master_get_devdata(master);
+
+	pic32s->mesg = msg;
+	pic32_spi_disable_chip(pic32s);
+
+	if (!pic32_spi_debug)
+		return 0;
+
+	list_for_each_entry(xfer, &msg->transfers, transfer_list) {
+		dev_dbg(&spi->dev, "  xfer %p: len %u tx %p rx %p\n",
+			xfer, xfer->len, xfer->tx_buf, xfer->rx_buf);
+		print_hex_dump(KERN_DEBUG, "tx_buf ",
+			       DUMP_PREFIX_ADDRESS,
+			       16, 1, xfer->tx_buf,
+			       min_t(u32, xfer->len, 16), 1);
+	}
+
+	return 0;
+}
+
+static bool pic32_spi_can_dma(struct spi_master *master,
+			      struct spi_device *spi,
+			      struct spi_transfer *xfer)
+{
+	struct pic32_spi *pic32s = spi_master_get_devdata(master);
+
+	return (xfer->len >= PIC32_DMA_LEN_MIN) &&
+	       test_bit(PIC32F_DMA_PREP, &pic32s->flags);
+}
+
+static int pic32_spi_one_transfer(struct spi_master *master,
+				  struct spi_device *spi,
+				  struct spi_transfer *transfer)
+{
+	struct pic32_spi *pic32s;
+	int ret;
+
+	pic32s = spi_master_get_devdata(master);
+
+	/* handle speed change */
+	if (transfer->speed_hz && (transfer->speed_hz != pic32s->speed_hz)) {
+		pic32_spi_set_clk_rate(pic32s, transfer->speed_hz);
+		pic32s->speed_hz = transfer->speed_hz;
+	}
+
+	/* handle word size change */
+	if (transfer->bits_per_word)
+		pic32_spi_set_word_size(pic32s, transfer->bits_per_word);
+
+	pic32_spi_enable_chip(pic32s);
+	reinit_completion(&pic32s->xfer_done);
+
+	/* transact by DMA mode */
+	if (transfer->rx_sg.nents && transfer->tx_sg.nents) {
+		ret = pic32_spi_dma_transfer(pic32s, transfer);
+		if (ret) {
+			dev_err(&spi->dev, "dma submit error\n");
+			return ret;
+		}
+
+		/* DMA issued, wait for completion */
+		set_bit(PIC32F_DMA_ISSUED, &pic32s->flags);
+		goto out_wait_for_xfer;
+	}
+
+	/* set current transfer information */
+	pic32s->tx = (const void *)transfer->tx_buf;
+	pic32s->rx = (const void *)transfer->rx_buf;
+	pic32s->tx_end = pic32s->tx + transfer->len;
+	pic32s->rx_end = pic32s->rx + transfer->len;
+	pic32s->len = transfer->len;
+
+	/* transact by interrupt driven PIO */
+	enable_irq(pic32s->fault_irq);
+	enable_irq(pic32s->rx_irq);
+	enable_irq(pic32s->tx_irq);
+
+out_wait_for_xfer:
+	/* wait for completion */
+	ret = wait_for_completion_timeout(&pic32s->xfer_done, 2 * HZ);
+	if (ret <= 0) {
+		dev_err(&spi->dev, "wait error/timedout\n");
+		if (test_bit(PIC32F_DMA_ISSUED, &pic32s->flags)) {
+			dmaengine_terminate_all(master->dma_rx);
+			dmaengine_terminate_all(master->dma_rx);
+		}
+		ret = -ETIMEDOUT;
+	} else {
+		ret = 0;
+	}
+
+	clear_bit(PIC32F_DMA_ISSUED, &pic32s->flags);
+	return ret;
+}
+
+static int pic32_spi_unprepare_message(struct spi_master *master,
+				       struct spi_message *msg)
+{
+	struct spi_transfer *xfer;
+	struct pic32_spi *pic32s;
+
+	pic32s = spi_master_get_devdata(master);
+
+	pic32_spi_disable_chip(pic32s);
+
+	if (!pic32_spi_debug)
+		return 0;
+
+	list_for_each_entry(xfer, &msg->transfers, transfer_list)
+		print_hex_dump(KERN_DEBUG, "rx_buf ",
+			       DUMP_PREFIX_ADDRESS,
+			       16, 1, xfer->rx_buf,
+			       min_t(u32, xfer->len, 32), 1);
+
+	return 0;
+}
+
+static void pic32_spi_cleanup(struct spi_device *spi)
+{
+	struct pic32_spi *pic32s;
+	int cs_high;
+
+	pic32s = spi_master_get_devdata(spi->master);
+
+	/* disable chip */
+	pic32_spi_disable_chip(pic32s);
+
+	/* release cs-gpio */
+	cs_high = pic32s->mode & SPI_CS_HIGH;
+	gpio_direction_output(spi->cs_gpio, !cs_high);
+	gpio_free(spi->cs_gpio);
+
+	/* reset reference */
+	pic32s->spi_dev = NULL;
+	pic32s->speed_hz = 0;
+}
+
+/* This may be called multiple times by same spi dev */
+static int pic32_spi_setup(struct spi_device *spi)
+{
+	struct pic32_spi *pic32s;
+	int cs_high, ret;
+	u32 v;
+
+	pic32s = spi_master_get_devdata(spi->master);
+
+	/* SPI master supports only one spi-device at a time.
+	 * So multiple spi_setup() to different devices is not allowed.
+	 */
+	if (unlikely(pic32s->spi_dev && (pic32s->spi_dev != spi))) {
+		dev_err(&spi->dev, "spi-master already associated with %s\n",
+			dev_name(&pic32s->spi_dev->dev));
+		return -EBUSY;
+	}
+
+	/* But spi_setup() can be called multiple times by same device.
+	 * In that case stop current on-going transaction and release
+	 * resource(s).
+	 */
+	if (pic32s->spi_dev == spi)
+		pic32_spi_cleanup(spi);
+
+	pic32s->spi_dev = spi;
+
+	if (!spi->bits_per_word) {
+		dev_err(&spi->dev, "No bits_per_word defined\n");
+		return -EINVAL;
+	}
+
+	if (!spi->max_speed_hz) {
+		dev_err(&spi->dev, "No max speed HZ parameter\n");
+		return -EINVAL;
+	}
+
+	pic32_spi_disable_chip(pic32s);
+
+	pic32_spi_set_word_size(pic32s, spi->bits_per_word);
+
+	pic32s->speed_hz = spi->max_speed_hz;
+	pic32_spi_set_clk_rate(pic32s, spi->max_speed_hz);
+
+	/* set spi mode */
+	v = readl(&pic32s->regs->ctrl);
+
+	/* active low */
+	if (spi->mode & SPI_CPOL)
+		v |= CTRL_CKP;
+	else
+		v &= ~CTRL_CKP;
+
+	/* tx on rising edge */
+	if (spi->mode & SPI_CPHA)
+		v &= ~CTRL_CKE;
+	else
+		v |= CTRL_CKE;
+
+	/* rx at end of tx */
+	v |= CTRL_SMP;
+	writel(v, &pic32s->regs->ctrl);
+	pic32s->mode = spi->mode;
+
+	/* PIC32 spi controller can drive /CS during transfer depending
+	 * on tx fifo fill-level. /CS will stay asserted as long as TX
+	 * fifo is non-empty, else will be deasserted confirming
+	 * completion of the ongoing transfer. This might result into
+	 * unreliable/erroneous SPI transactions.
+	 * To avoid that we will manually handle /CS by toggling GPIO.
+	 */
+	if (!gpio_is_valid(spi->cs_gpio))
+		return -EINVAL;
+
+	cs_high = pic32s->mode & SPI_CS_HIGH;
+	ret = gpio_request(spi->cs_gpio, dev_name(&spi->dev));
+	if (!ret) {
+		gpio_direction_output(spi->cs_gpio, !cs_high);
+		dev_vdbg(&spi->dev,
+			 "gpio-%d configured for spics (%s)\n",
+			 spi->cs_gpio, cs_high ? "cs_high" : "cs_low");
+	}
+	writel(CTRL_MSSEN, &pic32s->regs->ctrl_clr);
+
+	dev_vdbg(&spi->master->dev,
+		 "successfully registered spi-device %s\n",
+		 dev_name(&spi->dev));
+	return 0;
+}
+
+static int pic32_spi_dma_prep(struct pic32_spi *pic32s, struct device *dev)
+{
+	struct spi_master *master = pic32s->master;
+	dma_cap_mask_t mask;
+	int err;
+
+	dma_cap_zero(mask);
+	dma_cap_set(DMA_SLAVE, mask);
+
+	master->dma_rx = dma_request_slave_channel_compat(mask, NULL, NULL,
+							  dev, "spi-rx");
+	if (!master->dma_rx) {
+		dev_err(dev, "RX channel not found, SPI unable to use DMA\n");
+		err = -EBUSY;
+		goto out_err;
+	}
+
+	master->dma_tx = dma_request_slave_channel_compat(mask, NULL, NULL,
+							  dev, "spi-tx");
+	if (!master->dma_tx) {
+		dev_err(dev, "TX channel not found, SPI unable to use DMA\n");
+		err = -EBUSY;
+		goto out_err;
+	}
+
+	pic32s->dma_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
+	err = pic32_spi_dma_config(pic32s, pic32s->dma_width);
+	if (err)
+		goto out_err;
+
+	/* DMA chnls allocated and prepared */
+	set_bit(PIC32F_DMA_PREP, &pic32s->flags);
+
+	dev_dbg(dev, "Using %s (tx) and %s (rx) for DMA transfers\n",
+		dma_chan_name(master->dma_tx),
+		dma_chan_name(master->dma_rx));
+	return 0;
+
+out_err:
+	if (master->dma_rx)
+		dma_release_channel(master->dma_rx);
+
+	if (master->dma_tx)
+		dma_release_channel(master->dma_tx);
+
+	return err;
+}
+
+static void pic32_spi_dma_unprep(struct pic32_spi *pic32s)
+{
+	if (!test_bit(PIC32F_DMA_PREP, &pic32s->flags))
+		return;
+
+	clear_bit(PIC32F_DMA_PREP, &pic32s->flags);
+	if (pic32s->master->dma_rx)
+		dma_release_channel(pic32s->master->dma_rx);
+
+	if (pic32s->master->dma_tx)
+		dma_release_channel(pic32s->master->dma_tx);
+}
+
+static void pic32_spi_hw_init(struct pic32_spi *pic32s)
+{
+	u32 v;
+
+	/* disable module */
+	pic32_spi_disable_chip(pic32s);
+
+	v = readl(&pic32s->regs->ctrl);
+	/* enable enhanced fifo of 128bit deep */
+	v |= CTRL_ENHBUF;
+	pic32s->fifo_n_byte = 16;
+
+	/* disable framing mode */
+	v &= ~CTRL_FRMEN;
+
+	/* enable master mode while disabled */
+	v |= CTRL_MSTEN;
+
+	/* set tx fifo threshold interrupt */
+	v &= ~(0x3 << CTRL_TX_INT_SHIFT);
+	v |= (TX_FIFO_HALF_EMPTY << CTRL_TX_INT_SHIFT);
+
+	/* set rx fifo threshold interrupt */
+	v &= ~(0x3 << CTRL_RX_INT_SHIFT);
+	v |= (RX_FIFO_NOT_EMPTY << CTRL_RX_INT_SHIFT);
+
+	/* select clk source */
+	v &= ~CTRL_MCLKSEL;
+
+	writel(v, &pic32s->regs->ctrl);
+
+	/* enable error reporting */
+	v = CTRL2_TX_UR_EN | CTRL2_RX_OV_EN | CTRL2_FRM_ERR_EN;
+	writel(v, &pic32s->regs->ctrl2_set);
+}
+
+static int pic32_spi_hw_probe(struct platform_device *pdev,
+			      struct pic32_spi *pic32s)
+{
+	struct resource *mem;
+	int ret;
+
+	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	pic32s->regs = devm_ioremap_resource(&pdev->dev, mem);
+	if (!pic32s->regs) {
+		dev_err(&pdev->dev, "ioremap() failed\n");
+		return -ENOMEM;
+	}
+	pic32s->dma_base = mem->start;
+
+	/* get irq resources: err-irq, rx-irq, tx-irq */
+	pic32s->fault_irq = platform_get_irq_byname(pdev, "fault");
+	if (pic32s->fault_irq < 0) {
+		dev_err(&pdev->dev, "fault-irq not found\n");
+		return pic32s->fault_irq;
+	}
+
+	pic32s->rx_irq = platform_get_irq_byname(pdev, "rx");
+	if (pic32s->rx_irq < 0) {
+		dev_err(&pdev->dev, "rx-irq not found\n");
+		return pic32s->rx_irq;
+	}
+
+	pic32s->tx_irq = platform_get_irq_byname(pdev, "tx");
+	if (pic32s->tx_irq < 0) {
+		dev_err(&pdev->dev, "tx-irq not found\n");
+		return pic32s->tx_irq;
+	}
+
+	/* get clock */
+	pic32s->clk = devm_clk_get(&pdev->dev, "mck0");
+	if (IS_ERR(pic32s->clk)) {
+		dev_err(&pdev->dev, "clk not found\n");
+		ret = PTR_ERR(pic32s->clk);
+		goto err_unmap_mem;
+	}
+
+	ret = clk_prepare_enable(pic32s->clk);
+	if (ret)
+		goto err_unmap_mem;
+
+	/* initialize module */
+	pic32_spi_hw_init(pic32s);
+
+	return 0;
+
+err_unmap_mem:
+	dev_err(&pdev->dev, "hw_probe failed, ret %d\n", ret);
+	return ret;
+}
+
+static int pic32_spi_probe(struct platform_device *pdev)
+{
+	struct spi_master *master;
+	struct pic32_spi *pic32s;
+	int ret;
+
+	master = spi_alloc_master(&pdev->dev, sizeof(*pic32s));
+	if (!master)
+		return -ENOMEM;
+
+	pic32s = spi_master_get_devdata(master);
+	pic32s->master = master;
+
+	ret = pic32_spi_hw_probe(pdev, pic32s);
+	if (ret) {
+		dev_err(&pdev->dev, "hw probe failed.\n");
+		goto err_master;
+	}
+
+	master->dev.of_node	= of_node_get(pdev->dev.of_node);
+	master->mode_bits	= SPI_MODE_3 | SPI_MODE_0 | SPI_CS_HIGH;
+	master->num_chipselect	= 1; /* single chip-select */
+	master->max_speed_hz	= clk_get_rate(pic32s->clk);
+	master->setup		= pic32_spi_setup;
+	master->cleanup		= pic32_spi_cleanup;
+	master->flags		= SPI_MASTER_MUST_TX | SPI_MASTER_MUST_RX;
+	master->bits_per_word_mask	= SPI_BPW_RANGE_MASK(8, 32);
+	master->prepare_message		= pic32_spi_prepare_message;
+	master->transfer_one		= pic32_spi_one_transfer;
+	master->unprepare_message	= pic32_spi_unprepare_message;
+
+	/* DMA support */
+	ret = pic32_spi_dma_prep(pic32s, &pdev->dev);
+	if (test_bit(PIC32F_DMA_PREP, &pic32s->flags))
+		master->can_dma	= pic32_spi_can_dma;
+
+	init_completion(&pic32s->xfer_done);
+
+	/* install irq handlers (with irq-disabled) */
+	irq_set_status_flags(pic32s->fault_irq, IRQ_NOAUTOEN);
+	ret = devm_request_irq(&pdev->dev, pic32s->fault_irq,
+			       pic32_spi_fault_irq, IRQF_NO_THREAD,
+			       dev_name(&pdev->dev), pic32s);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "request fault-irq %d\n", pic32s->rx_irq);
+		goto err_bailout;
+	}
+
+	/* receive interrupt handler */
+	irq_set_status_flags(pic32s->rx_irq, IRQ_NOAUTOEN);
+	ret = devm_request_irq(&pdev->dev, pic32s->rx_irq,
+			       pic32_spi_rx_irq, IRQF_NO_THREAD,
+			       dev_name(&pdev->dev), pic32s);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "request rx-irq %d\n", pic32s->rx_irq);
+		goto err_bailout;
+	}
+
+	/* transmit interrupt handler */
+	irq_set_status_flags(pic32s->tx_irq, IRQ_NOAUTOEN);
+	ret = devm_request_irq(&pdev->dev, pic32s->tx_irq,
+			       pic32_spi_tx_irq, IRQF_NO_THREAD,
+			       dev_name(&pdev->dev), pic32s);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "request tx-irq %d\n", pic32s->tx_irq);
+		goto err_bailout;
+	}
+
+	/* register master */
+	ret = devm_spi_register_master(&pdev->dev, master);
+	if (ret) {
+		dev_err(&master->dev, "failed registering spi master\n");
+		goto err_bailout;
+	}
+
+	platform_set_drvdata(pdev, pic32s);
+
+	return 0;
+
+err_bailout:
+	pic32_spi_disable_chip(pic32s);
+
+	if (!IS_ERR_OR_NULL(pic32s->clk))
+		clk_disable_unprepare(pic32s->clk);
+err_master:
+	spi_master_put(master);
+	return ret;
+}
+
+static int pic32_spi_remove(struct platform_device *pdev)
+{
+	struct pic32_spi *pic32s;
+
+	pic32s = platform_get_drvdata(pdev);
+	pic32_spi_disable_chip(pic32s);
+	clk_disable_unprepare(pic32s->clk);
+	pic32_spi_dma_unprep(pic32s);
+
+	return 0;
+}
+
+static const struct of_device_id pic32_spi_of_match[] = {
+	{.compatible = "microchip,pic32mzda-spi",},
+	{},
+};
+MODULE_DEVICE_TABLE(of, pic32_spi_of_match);
+
+static struct platform_driver pic32_spi_driver = {
+	.driver = {
+		.name = "spi-pic32",
+		.of_match_table = of_match_ptr(pic32_spi_of_match),
+	},
+	.probe = pic32_spi_probe,
+	.remove = pic32_spi_remove,
+};
+
+module_platform_driver(pic32_spi_driver);
+
+MODULE_AUTHOR("Purna Chandra Mandal <purna.mandal@microchip.com>");
+MODULE_DESCRIPTION("Microchip SPI driver for PIC32 SPI controller.");
+MODULE_LICENSE("GPL v2");
-- 
1.8.3.1

Re: [PATCH v4 2/2] spi: spi-pic32: Add PIC32 SPI master driver

[Mark Brown]

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

On Wed, Mar 23, 2016 at 07:12:56PM +0530, Purna Chandra Mandal wrote:

> +	switch (bits_per_word) {
> +	default:
> +	case 8:

Are you sure that all bits per word settings other than those explicitly
supported should be handled in exactly the same fashion as 8 bits per
word?  That doesn't seem entirely expected.  I'd expect the default to
be to return an error.

> +static int pic32_spi_prepare_message(struct spi_master *master,
> +				     struct spi_message *msg)
> +{
> +	struct spi_device *spi = msg->spi;
> +	struct spi_transfer *xfer;
> +	struct pic32_spi *pic32s;
> +
> +	pic32s = spi_master_get_devdata(master);
> +
> +	pic32s->mesg = msg;
> +	pic32_spi_disable_chip(pic32s);
> +
> +	if (!pic32_spi_debug)
> +		return 0;

This appears to be some half implemented debugging code which is never
enabled, please remove it.

> +	list_for_each_entry(xfer, &msg->transfers, transfer_list) {
> +		dev_dbg(&spi->dev, "  xfer %p: len %u tx %p rx %p\n",
> +			xfer, xfer->len, xfer->tx_buf, xfer->rx_buf);
> +		print_hex_dump(KERN_DEBUG, "tx_buf ",
> +			       DUMP_PREFIX_ADDRESS,
> +			       16, 1, xfer->tx_buf,
> +			       min_t(u32, xfer->len, 16), 1);
> +	}

Similarly here, the core already has extensive trace stuff in it which
you can use.

> +	/* transact by DMA mode */
> +	if (transfer->rx_sg.nents && transfer->tx_sg.nents) {
> +		ret = pic32_spi_dma_transfer(pic32s, transfer);
> +		if (ret) {
> +			dev_err(&spi->dev, "dma submit error\n");
> +			return ret;
> +		}
> +
> +		/* DMA issued, wait for completion */
> +		set_bit(PIC32F_DMA_ISSUED, &pic32s->flags);
> +		goto out_wait_for_xfer;
> +	}

...

> +
> +out_wait_for_xfer:

Please write normal code with an if/else rather than using gotos, it's a
lot easier to follow.

> +	pic32s = spi_master_get_devdata(master);
> +
> +	pic32_spi_disable_chip(pic32s);

Do we really need tod isable the hardware after every single message?
It's normally more efficient to just leave the hardware powered until it
goes idle and unprepare_transfer_hardware() is called.

> +	/* SPI master supports only one spi-device at a time.
> +	 * So multiple spi_setup() to different devices is not allowed.
> +	 */
> +	if (unlikely(pic32s->spi_dev && (pic32s->spi_dev != spi))) {

unlikely() is for fast paths, outside of them it's just noise.

> +	/* But spi_setup() can be called multiple times by same device.
> +	 * In that case stop current on-going transaction and release
> +	 * resource(s).
> +	 */
> +	if (pic32s->spi_dev == spi)
> +		pic32_spi_cleanup(spi);

This is broken, it will break in progress transfers.  setup() shouldn't
be doing anything that disrupts them, anything that can't be run when
other transfers are in progress needs to be deferred till we actually do
the transfers (or done earlier in probe).

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

Re: [PATCH v4 2/2] spi: spi-pic32: Add PIC32 SPI master driver

[Mark Brown]

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

On Wed, Mar 23, 2016 at 07:12:56PM +0530, Purna Chandra Mandal wrote:

> +	switch (bits_per_word) {
> +	default:
> +	case 8:

Are you sure that all bits per word settings other than those explicitly
supported should be handled in exactly the same fashion as 8 bits per
word?  That doesn't seem entirely expected.  I'd expect the default to
be to return an error.

> +static int pic32_spi_prepare_message(struct spi_master *master,
> +				     struct spi_message *msg)
> +{
> +	struct spi_device *spi = msg->spi;
> +	struct spi_transfer *xfer;
> +	struct pic32_spi *pic32s;
> +
> +	pic32s = spi_master_get_devdata(master);
> +
> +	pic32s->mesg = msg;
> +	pic32_spi_disable_chip(pic32s);
> +
> +	if (!pic32_spi_debug)
> +		return 0;

This appears to be some half implemented debugging code which is never
enabled, please remove it.

> +	list_for_each_entry(xfer, &msg->transfers, transfer_list) {
> +		dev_dbg(&spi->dev, "  xfer %p: len %u tx %p rx %p\n",
> +			xfer, xfer->len, xfer->tx_buf, xfer->rx_buf);
> +		print_hex_dump(KERN_DEBUG, "tx_buf ",
> +			       DUMP_PREFIX_ADDRESS,
> +			       16, 1, xfer->tx_buf,
> +			       min_t(u32, xfer->len, 16), 1);
> +	}

Similarly here, the core already has extensive trace stuff in it which
you can use.

> +	/* transact by DMA mode */
> +	if (transfer->rx_sg.nents && transfer->tx_sg.nents) {
> +		ret = pic32_spi_dma_transfer(pic32s, transfer);
> +		if (ret) {
> +			dev_err(&spi->dev, "dma submit error\n");
> +			return ret;
> +		}
> +
> +		/* DMA issued, wait for completion */
> +		set_bit(PIC32F_DMA_ISSUED, &pic32s->flags);
> +		goto out_wait_for_xfer;
> +	}

...

> +
> +out_wait_for_xfer:

Please write normal code with an if/else rather than using gotos, it's a
lot easier to follow.

> +	pic32s = spi_master_get_devdata(master);
> +
> +	pic32_spi_disable_chip(pic32s);

Do we really need tod isable the hardware after every single message?
It's normally more efficient to just leave the hardware powered until it
goes idle and unprepare_transfer_hardware() is called.

> +	/* SPI master supports only one spi-device at a time.
> +	 * So multiple spi_setup() to different devices is not allowed.
> +	 */
> +	if (unlikely(pic32s->spi_dev && (pic32s->spi_dev != spi))) {

unlikely() is for fast paths, outside of them it's just noise.

> +	/* But spi_setup() can be called multiple times by same device.
> +	 * In that case stop current on-going transaction and release
> +	 * resource(s).
> +	 */
> +	if (pic32s->spi_dev == spi)
> +		pic32_spi_cleanup(spi);

This is broken, it will break in progress transfers.  setup() shouldn't
be doing anything that disrupts them, anything that can't be run when
other transfers are in progress needs to be deferred till we actually do
the transfers (or done earlier in probe).

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

Re: [PATCH v4 2/2] spi: spi-pic32: Add PIC32 SPI master driver

[Purna Chandra Mandal]

On 03/29/2016 12:56 AM, Mark Brown wrote:

> On Wed, Mar 23, 2016 at 07:12:56PM +0530, Purna Chandra Mandal wrote:
>
>> +	switch (bits_per_word) {
>> +	default:
>> +	case 8:
> Are you sure that all bits per word settings other than those explicitly
> supported should be handled in exactly the same fashion as 8 bits per
> word?  That doesn't seem entirely expected.  I'd expect the default to
> be to return an error.

ack. Will treat default case as error.

>> +static int pic32_spi_prepare_message(struct spi_master *master,
>> +				     struct spi_message *msg)
>> +{
>> +	struct spi_device *spi = msg->spi;
>> +	struct spi_transfer *xfer;
>> +	struct pic32_spi *pic32s;
>> +
>> +	pic32s = spi_master_get_devdata(master);
>> +
>> +	pic32s->mesg = msg;
>> +	pic32_spi_disable_chip(pic32s);
>> +
>> +	if (!pic32_spi_debug)
>> +		return 0;
> This appears to be some half implemented debugging code which is never
> enabled, please remove it.

ack. Will remove the debug logic.

>> +	list_for_each_entry(xfer, &msg->transfers, transfer_list) {
>> +		dev_dbg(&spi->dev, "  xfer %p: len %u tx %p rx %p\n",
>> +			xfer, xfer->len, xfer->tx_buf, xfer->rx_buf);
>> +		print_hex_dump(KERN_DEBUG, "tx_buf ",
>> +			       DUMP_PREFIX_ADDRESS,
>> +			       16, 1, xfer->tx_buf,
>> +			       min_t(u32, xfer->len, 16), 1);
>> +	}
> Similarly here, the core already has extensive trace stuff in it which
> you can use.

Will remove.

>> +	/* transact by DMA mode */
>> +	if (transfer->rx_sg.nents && transfer->tx_sg.nents) {
>> +		ret = pic32_spi_dma_transfer(pic32s, transfer);
>> +		if (ret) {
>> +			dev_err(&spi->dev, "dma submit error\n");
>> +			return ret;
>> +		}
>> +
>> +		/* DMA issued, wait for completion */
>> +		set_bit(PIC32F_DMA_ISSUED, &pic32s->flags);
>> +		goto out_wait_for_xfer;
>> +	}
> ...
>
>> +
>> +out_wait_for_xfer:
> Please write normal code with an if/else rather than using gotos, it's a
> lot easier to follow.

Make sense. Will use if-else.

>> +	pic32s = spi_master_get_devdata(master);
>> +
>> +	pic32_spi_disable_chip(pic32s);
> Do we really need tod isable the hardware after every single message?
> It's normally more efficient to just leave the hardware powered until it
> goes idle and unprepare_transfer_hardware() is called.
>
Ack. There is no particular need to disable h/w per message.
Will instead implement prepare/unprepare_transfer_hardware() and add there.

>> +	/* SPI master supports only one spi-device at a time.
>> +	 * So multiple spi_setup() to different devices is not allowed.
>> +	 */
>> +	if (unlikely(pic32s->spi_dev && (pic32s->spi_dev != spi))) {
> unlikely() is for fast paths, outside of them it's just noise.

ack.

>> +	/* But spi_setup() can be called multiple times by same device.
>> +	 * In that case stop current on-going transaction and release
>> +	 * resource(s).
>> +	 */
>> +	if (pic32s->spi_dev == spi)
>> +		pic32_spi_cleanup(spi);
> This is broken, it will break in progress transfers.  setup() shouldn't
> be doing anything that disrupts them, anything that can't be run when
> other transfers are in progress needs to be deferred till we actually do
> the transfers (or done earlier in probe).

Normally setup() and cleanup() are called in-pair by spi-client driver.
In these cases the above condition won't met and will not be a problem.

It is required for MMC-over-SPI support. Linux MMC_SPI driver sometimes
(depending on some logic) want chip-select to be kept enabled (using
transfer.cs_change) even at the end of SPI message so that following
message(s) can also be made part of the running MMC transaction.
In this case if there is any error (and in some other cases as well)
MMC_SPI will have to terminate on-going MMC transactions and it is
done by calling setup(). It is assumed that setup() will always leave
the chip-select deactivated.

Reference from drivers/mmc/host/mmc_spi.c:
-------------------
/*
 * MMC-over-SPI protocol glue, used by the MMC stack interface
 */

static inline int mmc_cs_off(struct mmc_spi_host *host)
{
        /* chipselect will always be inactive after setup() */
        return spi_setup(host->spi);
}
----------------

Best thing I can add is wait for unprepare_message() before calling
cleanup().

If you could suggest better alternative that will be great.


--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 2/2] spi: spi-pic32: Add PIC32 SPI master driver

[Purna Chandra Mandal]

On 03/29/2016 12:56 AM, Mark Brown wrote:

> On Wed, Mar 23, 2016 at 07:12:56PM +0530, Purna Chandra Mandal wrote:
>
>> +	switch (bits_per_word) {
>> +	default:
>> +	case 8:
> Are you sure that all bits per word settings other than those explicitly
> supported should be handled in exactly the same fashion as 8 bits per
> word?  That doesn't seem entirely expected.  I'd expect the default to
> be to return an error.

ack. Will treat default case as error.

>> +static int pic32_spi_prepare_message(struct spi_master *master,
>> +				     struct spi_message *msg)
>> +{
>> +	struct spi_device *spi = msg->spi;
>> +	struct spi_transfer *xfer;
>> +	struct pic32_spi *pic32s;
>> +
>> +	pic32s = spi_master_get_devdata(master);
>> +
>> +	pic32s->mesg = msg;
>> +	pic32_spi_disable_chip(pic32s);
>> +
>> +	if (!pic32_spi_debug)
>> +		return 0;
> This appears to be some half implemented debugging code which is never
> enabled, please remove it.

ack. Will remove the debug logic.

>> +	list_for_each_entry(xfer, &msg->transfers, transfer_list) {
>> +		dev_dbg(&spi->dev, "  xfer %p: len %u tx %p rx %p\n",
>> +			xfer, xfer->len, xfer->tx_buf, xfer->rx_buf);
>> +		print_hex_dump(KERN_DEBUG, "tx_buf ",
>> +			       DUMP_PREFIX_ADDRESS,
>> +			       16, 1, xfer->tx_buf,
>> +			       min_t(u32, xfer->len, 16), 1);
>> +	}
> Similarly here, the core already has extensive trace stuff in it which
> you can use.

Will remove.

>> +	/* transact by DMA mode */
>> +	if (transfer->rx_sg.nents && transfer->tx_sg.nents) {
>> +		ret = pic32_spi_dma_transfer(pic32s, transfer);
>> +		if (ret) {
>> +			dev_err(&spi->dev, "dma submit error\n");
>> +			return ret;
>> +		}
>> +
>> +		/* DMA issued, wait for completion */
>> +		set_bit(PIC32F_DMA_ISSUED, &pic32s->flags);
>> +		goto out_wait_for_xfer;
>> +	}
> ...
>
>> +
>> +out_wait_for_xfer:
> Please write normal code with an if/else rather than using gotos, it's a
> lot easier to follow.

Make sense. Will use if-else.

>> +	pic32s = spi_master_get_devdata(master);
>> +
>> +	pic32_spi_disable_chip(pic32s);
> Do we really need tod isable the hardware after every single message?
> It's normally more efficient to just leave the hardware powered until it
> goes idle and unprepare_transfer_hardware() is called.
>
Ack. There is no particular need to disable h/w per message.
Will instead implement prepare/unprepare_transfer_hardware() and add there.

>> +	/* SPI master supports only one spi-device at a time.
>> +	 * So multiple spi_setup() to different devices is not allowed.
>> +	 */
>> +	if (unlikely(pic32s->spi_dev && (pic32s->spi_dev != spi))) {
> unlikely() is for fast paths, outside of them it's just noise.

ack.

>> +	/* But spi_setup() can be called multiple times by same device.
>> +	 * In that case stop current on-going transaction and release
>> +	 * resource(s).
>> +	 */
>> +	if (pic32s->spi_dev == spi)
>> +		pic32_spi_cleanup(spi);
> This is broken, it will break in progress transfers.  setup() shouldn't
> be doing anything that disrupts them, anything that can't be run when
> other transfers are in progress needs to be deferred till we actually do
> the transfers (or done earlier in probe).

Normally setup() and cleanup() are called in-pair by spi-client driver.
In these cases the above condition won't met and will not be a problem.

It is required for MMC-over-SPI support. Linux MMC_SPI driver sometimes
(depending on some logic) want chip-select to be kept enabled (using
transfer.cs_change) even at the end of SPI message so that following
message(s) can also be made part of the running MMC transaction.
In this case if there is any error (and in some other cases as well)
MMC_SPI will have to terminate on-going MMC transactions and it is
done by calling setup(). It is assumed that setup() will always leave
the chip-select deactivated.

Reference from drivers/mmc/host/mmc_spi.c:
-------------------
/*
 * MMC-over-SPI protocol glue, used by the MMC stack interface
 */

static inline int mmc_cs_off(struct mmc_spi_host *host)
{
        /* chipselect will always be inactive after setup() */
        return spi_setup(host->spi);
}
----------------

Best thing I can add is wait for unprepare_message() before calling
cleanup().

If you could suggest better alternative that will be great.

Re: [PATCH v4 2/2] spi: spi-pic32: Add PIC32 SPI master driver

[Mark Brown]

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

On Tue, Mar 29, 2016 at 05:32:41PM +0530, Purna Chandra Mandal wrote:

> It is required for MMC-over-SPI support. Linux MMC_SPI driver sometimes
> (depending on some logic) want chip-select to be kept enabled (using
> transfer.cs_change) even at the end of SPI message so that following
> message(s) can also be made part of the running MMC transaction.
> In this case if there is any error (and in some other cases as well)

If your hardware doesn't support per-/CS setup then your driver needs to
record whatever is configured via setup() and apply it later.

> MMC_SPI will have to terminate on-going MMC transactions and it is
> done by calling setup(). It is assumed that setup() will always leave
> the chip-select deactivated.

No, this is just completely broken - that's quite simply not what
setup() does.  See spi-summary.  There is *no* code in the core that
terminates ongoing transfers as a result of calling setup(), if that's
happening it's a result of triggering error handling.

> Best thing I can add is wait for unprepare_message() before calling
> cleanup().

The cleanup() function is for deallocating dynamically allocated data,
it will be called by the core.

> If you could suggest better alternative that will be great.

If you want to change the chip select you need to use the normal
interfaces for changing the chip select via message processing.

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

Re: [PATCH v4 2/2] spi: spi-pic32: Add PIC32 SPI master driver

[Mark Brown]

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

On Tue, Mar 29, 2016 at 05:32:41PM +0530, Purna Chandra Mandal wrote:

> It is required for MMC-over-SPI support. Linux MMC_SPI driver sometimes
> (depending on some logic) want chip-select to be kept enabled (using
> transfer.cs_change) even at the end of SPI message so that following
> message(s) can also be made part of the running MMC transaction.
> In this case if there is any error (and in some other cases as well)

If your hardware doesn't support per-/CS setup then your driver needs to
record whatever is configured via setup() and apply it later.

> MMC_SPI will have to terminate on-going MMC transactions and it is
> done by calling setup(). It is assumed that setup() will always leave
> the chip-select deactivated.

No, this is just completely broken - that's quite simply not what
setup() does.  See spi-summary.  There is *no* code in the core that
terminates ongoing transfers as a result of calling setup(), if that's
happening it's a result of triggering error handling.

> Best thing I can add is wait for unprepare_message() before calling
> cleanup().

The cleanup() function is for deallocating dynamically allocated data,
it will be called by the core.

> If you could suggest better alternative that will be great.

If you want to change the chip select you need to use the normal
interfaces for changing the chip select via message processing.

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

Re: [PATCH v4 2/2] spi: spi-pic32: Add PIC32 SPI master driver

[Purna Chandra Mandal]

On 03/29/2016 09:55 PM, Mark Brown wrote:

> On Tue, Mar 29, 2016 at 05:32:41PM +0530, Purna Chandra Mandal wrote:
>
>> It is required for MMC-over-SPI support. Linux MMC_SPI driver sometimes
>> (depending on some logic) want chip-select to be kept enabled (using
>> transfer.cs_change) even at the end of SPI message so that following
>> message(s) can also be made part of the running MMC transaction.
>> In this case if there is any error (and in some other cases as well)
> If your hardware doesn't support per-/CS setup then your driver needs to
> record whatever is configured via setup() and apply it later.

PIC32 SPI controller supports one /CS per instance. So per-/CS specific
logic might not be required.

>> MMC_SPI will have to terminate on-going MMC transactions and it is
>> done by calling setup(). It is assumed that setup() will always leave
>> the chip-select deactivated.
> No, this is just completely broken - that's quite simply not what
> setup() does.  See spi-summary.  There is *no* code in the core that
> terminates ongoing transfers as a result of calling setup(), if that's
> happening it's a result of triggering error handling.

Description of spi_setup() in spi.c clearly mentions that "When this
function returns, the spi device is deselected" and this is only
possible if (at least) chip-select is deactivated.

So based on spi-summary and spi_setup() description, except chip-select,
all other settings has to be recorded and applied later when device is
selected and data is transferred to or from the device.

Keeping this in mind I'll just retain cs-deselect logic in setup() and
apply the recorded setting in prepare_message() callback, if and when required.

>> Best thing I can add is wait for unprepare_message() before calling
>> cleanup().
> The cleanup() function is for deallocating dynamically allocated data,
> it will be called by the core.

ack.

>> If you could suggest better alternative that will be great.
> If you want to change the chip select you need to use the normal
> interfaces for changing the chip select via message processing.

ack.

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 2/2] spi: spi-pic32: Add PIC32 SPI master driver

[Purna Chandra Mandal]

On 03/29/2016 09:55 PM, Mark Brown wrote:

> On Tue, Mar 29, 2016 at 05:32:41PM +0530, Purna Chandra Mandal wrote:
>
>> It is required for MMC-over-SPI support. Linux MMC_SPI driver sometimes
>> (depending on some logic) want chip-select to be kept enabled (using
>> transfer.cs_change) even at the end of SPI message so that following
>> message(s) can also be made part of the running MMC transaction.
>> In this case if there is any error (and in some other cases as well)
> If your hardware doesn't support per-/CS setup then your driver needs to
> record whatever is configured via setup() and apply it later.

PIC32 SPI controller supports one /CS per instance. So per-/CS specific
logic might not be required.

>> MMC_SPI will have to terminate on-going MMC transactions and it is
>> done by calling setup(). It is assumed that setup() will always leave
>> the chip-select deactivated.
> No, this is just completely broken - that's quite simply not what
> setup() does.  See spi-summary.  There is *no* code in the core that
> terminates ongoing transfers as a result of calling setup(), if that's
> happening it's a result of triggering error handling.

Description of spi_setup() in spi.c clearly mentions that "When this
function returns, the spi device is deselected" and this is only
possible if (at least) chip-select is deactivated.

So based on spi-summary and spi_setup() description, except chip-select,
all other settings has to be recorded and applied later when device is
selected and data is transferred to or from the device.

Keeping this in mind I'll just retain cs-deselect logic in setup() and
apply the recorded setting in prepare_message() callback, if and when required.

>> Best thing I can add is wait for unprepare_message() before calling
>> cleanup().
> The cleanup() function is for deallocating dynamically allocated data,
> it will be called by the core.

ack.

>> If you could suggest better alternative that will be great.
> If you want to change the chip select you need to use the normal
> interfaces for changing the chip select via message processing.

ack.

Re: [PATCH v4 2/2] spi: spi-pic32: Add PIC32 SPI master driver

[Mark Brown]

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

On Wed, Mar 30, 2016 at 04:19:16PM +0530, Purna Chandra Mandal wrote:
> On 03/29/2016 09:55 PM, Mark Brown wrote:
> > On Tue, Mar 29, 2016 at 05:32:41PM +0530, Purna Chandra Mandal wrote:

> >> MMC_SPI will have to terminate on-going MMC transactions and it is
> >> done by calling setup(). It is assumed that setup() will always leave
> >> the chip-select deactivated.

> > No, this is just completely broken - that's quite simply not what
> > setup() does.  See spi-summary.  There is *no* code in the core that
> > terminates ongoing transfers as a result of calling setup(), if that's
> > happening it's a result of triggering error handling.

> Description of spi_setup() in spi.c clearly mentions that "When this
> function returns, the spi device is deselected" and this is only
> possible if (at least) chip-select is deactivated.

This doesn't say anything about any ongoing operations!  Trying to do a
setup() with active transfers on the device is just not sensible, it is
intended to be used on an idle device.  I would not expect it to work
reliably on an active device.

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

Re: [PATCH v4 2/2] spi: spi-pic32: Add PIC32 SPI master driver

[Mark Brown]

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

On Wed, Mar 30, 2016 at 04:19:16PM +0530, Purna Chandra Mandal wrote:
> On 03/29/2016 09:55 PM, Mark Brown wrote:
> > On Tue, Mar 29, 2016 at 05:32:41PM +0530, Purna Chandra Mandal wrote:

> >> MMC_SPI will have to terminate on-going MMC transactions and it is
> >> done by calling setup(). It is assumed that setup() will always leave
> >> the chip-select deactivated.

> > No, this is just completely broken - that's quite simply not what
> > setup() does.  See spi-summary.  There is *no* code in the core that
> > terminates ongoing transfers as a result of calling setup(), if that's
> > happening it's a result of triggering error handling.

> Description of spi_setup() in spi.c clearly mentions that "When this
> function returns, the spi device is deselected" and this is only
> possible if (at least) chip-select is deactivated.

This doesn't say anything about any ongoing operations!  Trying to do a
setup() with active transfers on the device is just not sensible, it is
intended to be used on an idle device.  I would not expect it to work
reliably on an active device.

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

Re: [PATCH v4 2/2] spi: spi-pic32: Add PIC32 SPI master driver

[Purna Chandra Mandal]

On 03/30/2016 09:18 PM, Mark Brown wrote:

> On Wed, Mar 30, 2016 at 04:19:16PM +0530, Purna Chandra Mandal wrote:
>> On 03/29/2016 09:55 PM, Mark Brown wrote:
>>> On Tue, Mar 29, 2016 at 05:32:41PM +0530, Purna Chandra Mandal wrote:
>>>> MMC_SPI will have to terminate on-going MMC transactions and it is
>>>> done by calling setup(). It is assumed that setup() will always leave
>>>> the chip-select deactivated.
>>> No, this is just completely broken - that's quite simply not what
>>> setup() does.  See spi-summary.  There is *no* code in the core that
>>> terminates ongoing transfers as a result of calling setup(), if that's
>>> happening it's a result of triggering error handling.
>> Description of spi_setup() in spi.c clearly mentions that "When this
>> function returns, the spi device is deselected" and this is only
>> possible if (at least) chip-select is deactivated.
> This doesn't say anything about any ongoing operations!  Trying to do a
> setup() with active transfers on the device is just not sensible, it is
> intended to be used on an idle device.  I would not expect it to work
> reliably on an active device.

agreed.


--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 2/2] spi: spi-pic32: Add PIC32 SPI master driver

[Purna Chandra Mandal]

On 03/30/2016 09:18 PM, Mark Brown wrote:

> On Wed, Mar 30, 2016 at 04:19:16PM +0530, Purna Chandra Mandal wrote:
>> On 03/29/2016 09:55 PM, Mark Brown wrote:
>>> On Tue, Mar 29, 2016 at 05:32:41PM +0530, Purna Chandra Mandal wrote:
>>>> MMC_SPI will have to terminate on-going MMC transactions and it is
>>>> done by calling setup(). It is assumed that setup() will always leave
>>>> the chip-select deactivated.
>>> No, this is just completely broken - that's quite simply not what
>>> setup() does.  See spi-summary.  There is *no* code in the core that
>>> terminates ongoing transfers as a result of calling setup(), if that's
>>> happening it's a result of triggering error handling.
>> Description of spi_setup() in spi.c clearly mentions that "When this
>> function returns, the spi device is deselected" and this is only
>> possible if (at least) chip-select is deactivated.
> This doesn't say anything about any ongoing operations!  Trying to do a
> setup() with active transfers on the device is just not sensible, it is
> intended to be used on an idle device.  I would not expect it to work
> reliably on an active device.

agreed.