[PATCH 0/6] Add more configuration and regmap support for spi-altera

[PATCH 0/6] Add more configuration and regmap support for spi-altera

[Xu Yilun]

This patchset adds platform_data for spi-altera, to enable more IP
configurations, and creating specific spi client devices. It also adds
regmap support, to enable the indirect access to this IP.

We have a PCIE based FPGA platform which integrates this IP to communicate
with a BMC chip (Intel MAX10) over SPI. The IP is configured as 32bit data
width. There is also an indirect access interface in FPGA for host to
access the registers of this IP. This patchset enables this use case.

Matthew Gerlach (1):
  spi: altera: fix size mismatch on 64 bit processors

Xu Yilun (5):
  spi: altera: add 32bit data width transfer support.
  spi: altera: add SPI core parameters support via platform data.
  spi: altera: add platform data for slave information.
  spi: altera: use regmap instead of direct mmio register access
  spi: altera: move driver name string to header file

 drivers/spi/Kconfig        |   1 +
 drivers/spi/spi-altera.c   | 161 +++++++++++++++++++++++++++++++++++++--------
 include/linux/spi/altera.h |  37 +++++++++++
 3 files changed, 171 insertions(+), 28 deletions(-)
 create mode 100644 include/linux/spi/altera.h

-- 
2.7.4

[PATCH 1/6] spi: altera: add 32bit data width transfer support.

[Xu Yilun]

Add support for 32bit width data register, then it supports 32bit
data width spi slave device and spi transfers.

Signed-off-by: Xu Yilun <yilun.xu@intel.com>
Signed-off-by: Wu Hao <hao.wu@intel.com>
Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
Signed-off-by: Russ Weight <russell.h.weight@intel.com>
---
 drivers/spi/spi-altera.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/spi/spi-altera.c b/drivers/spi/spi-altera.c
index 41d71ba..d5fa0c5 100644
--- a/drivers/spi/spi-altera.c
+++ b/drivers/spi/spi-altera.c
@@ -86,6 +86,13 @@ static void altera_spi_tx_word(struct altera_spi *hw)
 			txd = (hw->tx[hw->count * 2]
 				| (hw->tx[hw->count * 2 + 1] << 8));
 			break;
+		case 4:
+			txd = (hw->tx[hw->count * 4]
+				| (hw->tx[hw->count * 4 + 1] << 8)
+				| (hw->tx[hw->count * 4 + 2] << 16)
+				| (hw->tx[hw->count * 4 + 3] << 24));
+			break;
+
 		}
 	}
 
@@ -106,6 +113,13 @@ static void altera_spi_rx_word(struct altera_spi *hw)
 			hw->rx[hw->count * 2] = rxd;
 			hw->rx[hw->count * 2 + 1] = rxd >> 8;
 			break;
+		case 4:
+			hw->rx[hw->count * 4] = rxd;
+			hw->rx[hw->count * 4 + 1] = rxd >> 8;
+			hw->rx[hw->count * 4 + 2] = rxd >> 16;
+			hw->rx[hw->count * 4 + 3] = rxd >> 24;
+			break;
+
 		}
 	}
 
-- 
2.7.4

[PATCH 2/6] spi: altera: add SPI core parameters support via platform data.

[Xu Yilun]

This patch introduced SPI core parameters in platform data, it
allows passing these SPI core parameters via platform data.

Signed-off-by: Wu Hao <hao.wu@intel.com>
Signed-off-by: Xu Yilun <yilun.xu@intel.com>
Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
Signed-off-by: Russ Weight <russell.h.weight@intel.com>
---
 drivers/spi/spi-altera.c   | 25 ++++++++++++++++++++++---
 include/linux/spi/altera.h | 24 ++++++++++++++++++++++++
 2 files changed, 46 insertions(+), 3 deletions(-)
 create mode 100644 include/linux/spi/altera.h

diff --git a/drivers/spi/spi-altera.c b/drivers/spi/spi-altera.c
index d5fa0c5..e6e6708 100644
--- a/drivers/spi/spi-altera.c
+++ b/drivers/spi/spi-altera.c
@@ -14,6 +14,7 @@
 #include <linux/errno.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
+#include <linux/spi/altera.h>
 #include <linux/spi/spi.h>
 #include <linux/io.h>
 #include <linux/of.h>
@@ -40,6 +41,8 @@
 #define ALTERA_SPI_CONTROL_IE_MSK	0x100
 #define ALTERA_SPI_CONTROL_SSO_MSK	0x400
 
+#define ALTERA_SPI_MAX_CS		32
+
 struct altera_spi {
 	void __iomem *base;
 	int irq;
@@ -182,6 +185,7 @@ static irqreturn_t altera_spi_irq(int irq, void *dev)
 
 static int altera_spi_probe(struct platform_device *pdev)
 {
+	struct altera_spi_platform_data *pdata = dev_get_platdata(&pdev->dev);
 	struct altera_spi *hw;
 	struct spi_master *master;
 	int err = -ENODEV;
@@ -192,9 +196,24 @@ static int altera_spi_probe(struct platform_device *pdev)
 
 	/* setup the master state. */
 	master->bus_num = pdev->id;
-	master->num_chipselect = 16;
-	master->mode_bits = SPI_CS_HIGH;
-	master->bits_per_word_mask = SPI_BPW_RANGE_MASK(1, 16);
+
+	if (pdata) {
+		if (pdata->num_chipselect > ALTERA_SPI_MAX_CS) {
+			dev_err(&pdev->dev,
+				"Invalid number of chipselect: %hu\n",
+				pdata->num_chipselect);
+			return -EINVAL;
+		}
+
+		master->num_chipselect = pdata->num_chipselect;
+		master->mode_bits = pdata->mode_bits;
+		master->bits_per_word_mask = pdata->bits_per_word_mask;
+	} else {
+		master->num_chipselect = 16;
+		master->mode_bits = SPI_CS_HIGH;
+		master->bits_per_word_mask = SPI_BPW_RANGE_MASK(1, 16);
+	}
+
 	master->dev.of_node = pdev->dev.of_node;
 	master->transfer_one = altera_spi_txrx;
 	master->set_cs = altera_spi_set_cs;
diff --git a/include/linux/spi/altera.h b/include/linux/spi/altera.h
new file mode 100644
index 0000000..344a3fc
--- /dev/null
+++ b/include/linux/spi/altera.h
@@ -0,0 +1,24 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Header File for Altera SPI Driver.
+ */
+#ifndef __LINUX_SPI_ALTERA_H
+#define __LINUX_SPI_ALTERA_H
+
+#include <linux/regmap.h>
+#include <linux/spi/spi.h>
+#include <linux/types.h>
+
+/**
+ * struct altera_spi_platform_data - Platform data of the Altera SPI driver
+ * @mode_bits:		Mode bits of SPI master.
+ * @num_chipselect:	Number of chipselects.
+ * @bits_per_word_mask:	bitmask of supported bits_per_word for transfers.
+ */
+struct altera_spi_platform_data {
+	u16				mode_bits;
+	u16				num_chipselect;
+	u32				bits_per_word_mask;
+};
+
+#endif /* __LINUX_SPI_ALTERA_H */
-- 
2.7.4

[PATCH 3/6] spi: altera: add platform data for slave information.

[Xu Yilun]

This patch introduces platform data for slave information, it allows
spi-altera to add new spi devices once master registration is done.

Signed-off-by: Wu Hao <hao.wu@intel.com>
Signed-off-by: Xu Yilun <yilun.xu@intel.com>
Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
Signed-off-by: Russ Weight <russell.h.weight@intel.com>
---
 drivers/spi/spi-altera.c   | 11 +++++++++++
 include/linux/spi/altera.h |  5 +++++
 2 files changed, 16 insertions(+)

diff --git a/drivers/spi/spi-altera.c b/drivers/spi/spi-altera.c
index e6e6708..aa9d1a2 100644
--- a/drivers/spi/spi-altera.c
+++ b/drivers/spi/spi-altera.c
@@ -189,6 +189,7 @@ static int altera_spi_probe(struct platform_device *pdev)
 	struct altera_spi *hw;
 	struct spi_master *master;
 	int err = -ENODEV;
+	u16 i;
 
 	master = spi_alloc_master(&pdev->dev, sizeof(struct altera_spi));
 	if (!master)
@@ -244,6 +245,16 @@ static int altera_spi_probe(struct platform_device *pdev)
 	err = devm_spi_register_master(&pdev->dev, master);
 	if (err)
 		goto exit;
+
+	if (pdata) {
+		for (i = 0; i < pdata->num_devices; i++) {
+			if (!spi_new_device(master, pdata->devices + i))
+				dev_warn(&pdev->dev,
+					 "unable to create SPI device: %s\n",
+					 pdata->devices[i].modalias);
+		}
+	}
+
 	dev_info(&pdev->dev, "base %p, irq %d\n", hw->base, hw->irq);
 
 	return 0;
diff --git a/include/linux/spi/altera.h b/include/linux/spi/altera.h
index 344a3fc..2d42641 100644
--- a/include/linux/spi/altera.h
+++ b/include/linux/spi/altera.h
@@ -14,11 +14,16 @@
  * @mode_bits:		Mode bits of SPI master.
  * @num_chipselect:	Number of chipselects.
  * @bits_per_word_mask:	bitmask of supported bits_per_word for transfers.
+ * @num_devices:	Number of devices that shall be added when the driver
+ *			is probed.
+ * @devices:		The devices to add.
  */
 struct altera_spi_platform_data {
 	u16				mode_bits;
 	u16				num_chipselect;
 	u32				bits_per_word_mask;
+	u16				num_devices;
+	struct spi_board_info		*devices;
 };
 
 #endif /* __LINUX_SPI_ALTERA_H */
-- 
2.7.4

[PATCH 4/6] spi: altera: use regmap instead of direct mmio register access

[Xu Yilun]

This patch adds support for regmap. It allows this driver to
be compatible if low layer register access method is changed
in some cases.

Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
Signed-off-by: Wu Hao <hao.wu@intel.com>
Signed-off-by: Xu Yilun <yilun.xu@intel.com>
Signed-off-by: Russ Weight <russell.h.weight@intel.com>
---
 drivers/spi/Kconfig        |   1 +
 drivers/spi/spi-altera.c   | 103 ++++++++++++++++++++++++++++++++++++---------
 include/linux/spi/altera.h |   6 +++
 3 files changed, 90 insertions(+), 20 deletions(-)

diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index 8f1f8fc..6d79fc7 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -59,6 +59,7 @@ comment "SPI Master Controller Drivers"
 
 config SPI_ALTERA
 	tristate "Altera SPI Controller"
+	select REGMAP_MMIO
 	help
 	  This is the driver for the Altera SPI Controller.
 
diff --git a/drivers/spi/spi-altera.c b/drivers/spi/spi-altera.c
index aa9d1a2..8d47c57 100644
--- a/drivers/spi/spi-altera.c
+++ b/drivers/spi/spi-altera.c
@@ -44,7 +44,6 @@
 #define ALTERA_SPI_MAX_CS		32
 
 struct altera_spi {
-	void __iomem *base;
 	int irq;
 	int len;
 	int count;
@@ -54,8 +53,45 @@ struct altera_spi {
 	/* data buffers */
 	const unsigned char *tx;
 	unsigned char *rx;
+
+	struct regmap *regmap;
+	u32 base;
+
+	struct device *dev;
+};
+
+static const struct regmap_config spi_altera_config = {
+	.reg_bits = 32,
+	.reg_stride = 4,
+	.val_bits = 32,
+	.fast_io = true,
 };
 
+static int altr_spi_writel(struct altera_spi *hw, unsigned int reg,
+			   unsigned int val)
+{
+	int ret;
+
+	ret = regmap_write(hw->regmap, hw->base + reg, val);
+	if (ret)
+		dev_err(hw->dev, "fail to write reg 0x%x val 0x%x: %d\n",
+			reg, val, ret);
+
+	return ret;
+}
+
+static int altr_spi_readl(struct altera_spi *hw, unsigned int reg,
+			  unsigned int *val)
+{
+	int ret;
+
+	ret = regmap_read(hw->regmap, hw->base + reg, val);
+	if (ret)
+		dev_err(hw->dev, "fail to read reg 0x%x: %d\n", reg, ret);
+
+	return ret;
+}
+
 static inline struct altera_spi *altera_spi_to_hw(struct spi_device *sdev)
 {
 	return spi_master_get_devdata(sdev->master);
@@ -67,12 +103,13 @@ static void altera_spi_set_cs(struct spi_device *spi, bool is_high)
 
 	if (is_high) {
 		hw->imr &= ~ALTERA_SPI_CONTROL_SSO_MSK;
-		writel(hw->imr, hw->base + ALTERA_SPI_CONTROL);
-		writel(0, hw->base + ALTERA_SPI_SLAVE_SEL);
+		altr_spi_writel(hw, ALTERA_SPI_CONTROL, hw->imr);
+		altr_spi_writel(hw, ALTERA_SPI_SLAVE_SEL, 0);
 	} else {
-		writel(BIT(spi->chip_select), hw->base + ALTERA_SPI_SLAVE_SEL);
+		altr_spi_writel(hw, ALTERA_SPI_SLAVE_SEL,
+				BIT(spi->chip_select));
 		hw->imr |= ALTERA_SPI_CONTROL_SSO_MSK;
-		writel(hw->imr, hw->base + ALTERA_SPI_CONTROL);
+		altr_spi_writel(hw, ALTERA_SPI_CONTROL, hw->imr);
 	}
 }
 
@@ -99,14 +136,14 @@ static void altera_spi_tx_word(struct altera_spi *hw)
 		}
 	}
 
-	writel(txd, hw->base + ALTERA_SPI_TXDATA);
+	altr_spi_writel(hw, ALTERA_SPI_TXDATA, txd);
 }
 
 static void altera_spi_rx_word(struct altera_spi *hw)
 {
 	unsigned int rxd;
 
-	rxd = readl(hw->base + ALTERA_SPI_RXDATA);
+	altr_spi_readl(hw, ALTERA_SPI_RXDATA, &rxd);
 	if (hw->rx) {
 		switch (hw->bytes_per_word) {
 		case 1:
@@ -133,6 +170,7 @@ static int altera_spi_txrx(struct spi_master *master,
 	struct spi_device *spi, struct spi_transfer *t)
 {
 	struct altera_spi *hw = spi_master_get_devdata(master);
+	u32 val;
 
 	hw->tx = t->tx_buf;
 	hw->rx = t->rx_buf;
@@ -143,7 +181,7 @@ static int altera_spi_txrx(struct spi_master *master,
 	if (hw->irq >= 0) {
 		/* enable receive interrupt */
 		hw->imr |= ALTERA_SPI_CONTROL_IRRDY_MSK;
-		writel(hw->imr, hw->base + ALTERA_SPI_CONTROL);
+		altr_spi_writel(hw, ALTERA_SPI_CONTROL, hw->imr);
 
 		/* send the first byte */
 		altera_spi_tx_word(hw);
@@ -151,9 +189,13 @@ static int altera_spi_txrx(struct spi_master *master,
 		while (hw->count < hw->len) {
 			altera_spi_tx_word(hw);
 
-			while (!(readl(hw->base + ALTERA_SPI_STATUS) &
-				 ALTERA_SPI_STATUS_RRDY_MSK))
+			for (;;) {
+				altr_spi_readl(hw, ALTERA_SPI_STATUS, &val);
+				if (val & ALTERA_SPI_STATUS_RRDY_MSK)
+					break;
+
 				cpu_relax();
+			}
 
 			altera_spi_rx_word(hw);
 		}
@@ -175,7 +217,7 @@ static irqreturn_t altera_spi_irq(int irq, void *dev)
 	} else {
 		/* disable receive interrupt */
 		hw->imr &= ~ALTERA_SPI_CONTROL_IRRDY_MSK;
-		writel(hw->imr, hw->base + ALTERA_SPI_CONTROL);
+		altr_spi_writel(hw, ALTERA_SPI_CONTROL, hw->imr);
 
 		spi_finalize_current_transfer(master);
 	}
@@ -188,7 +230,9 @@ static int altera_spi_probe(struct platform_device *pdev)
 	struct altera_spi_platform_data *pdata = dev_get_platdata(&pdev->dev);
 	struct altera_spi *hw;
 	struct spi_master *master;
+	void __iomem *res;
 	int err = -ENODEV;
+	u32 val;
 	u16 i;
 
 	master = spi_alloc_master(&pdev->dev, sizeof(struct altera_spi));
@@ -220,19 +264,38 @@ static int altera_spi_probe(struct platform_device *pdev)
 	master->set_cs = altera_spi_set_cs;
 
 	hw = spi_master_get_devdata(master);
+	hw->dev = &pdev->dev;
 
 	/* find and map our resources */
-	hw->base = devm_platform_ioremap_resource(pdev, 0);
-	if (IS_ERR(hw->base)) {
-		err = PTR_ERR(hw->base);
-		goto exit;
+	if (pdata && pdata->use_parent_regmap) {
+		hw->regmap = dev_get_regmap(pdev->dev.parent, NULL);
+		if (!hw->regmap) {
+			dev_err(&pdev->dev, "get regmap failed\n");
+			goto exit;
+		}
+		hw->base = pdata->regoff;
+	} else {
+		res = devm_platform_ioremap_resource(pdev, 0);
+		if (IS_ERR(res)) {
+			err = PTR_ERR(res);
+			goto exit;
+		}
+
+		hw->regmap = devm_regmap_init_mmio(&pdev->dev, res,
+						   &spi_altera_config);
+		if (IS_ERR(hw->regmap)) {
+			dev_err(&pdev->dev, "regmap mmio init failed\n");
+			err = PTR_ERR(hw->regmap);
+			goto exit;
+		}
 	}
 	/* program defaults into the registers */
 	hw->imr = 0;		/* disable spi interrupts */
-	writel(hw->imr, hw->base + ALTERA_SPI_CONTROL);
-	writel(0, hw->base + ALTERA_SPI_STATUS);	/* clear status reg */
-	if (readl(hw->base + ALTERA_SPI_STATUS) & ALTERA_SPI_STATUS_RRDY_MSK)
-		readl(hw->base + ALTERA_SPI_RXDATA);	/* flush rxdata */
+	altr_spi_writel(hw, ALTERA_SPI_CONTROL, hw->imr);
+	altr_spi_writel(hw, ALTERA_SPI_STATUS, 0);	/* clear status reg */
+	altr_spi_readl(hw, ALTERA_SPI_STATUS, &val);
+	if (val & ALTERA_SPI_STATUS_RRDY_MSK)
+		altr_spi_readl(hw, ALTERA_SPI_RXDATA, &val); /* flush rxdata */
 	/* irq is optional */
 	hw->irq = platform_get_irq(pdev, 0);
 	if (hw->irq >= 0) {
@@ -255,7 +318,7 @@ static int altera_spi_probe(struct platform_device *pdev)
 		}
 	}
 
-	dev_info(&pdev->dev, "base %p, irq %d\n", hw->base, hw->irq);
+	dev_info(&pdev->dev, "base %u, irq %d\n", hw->base, hw->irq);
 
 	return 0;
 exit:
diff --git a/include/linux/spi/altera.h b/include/linux/spi/altera.h
index 2d42641..164ab7b 100644
--- a/include/linux/spi/altera.h
+++ b/include/linux/spi/altera.h
@@ -17,6 +17,10 @@
  * @num_devices:	Number of devices that shall be added when the driver
  *			is probed.
  * @devices:		The devices to add.
+ * @use_parent_regmap:	If true, device uses parent regmap to access its
+ *			registers. Otherwise try map platform mmio resources.
+ * @regoff:		Offset of the device register base in parent regmap.
+ *			This field is ignored when use_parent_regmap == false.
  */
 struct altera_spi_platform_data {
 	u16				mode_bits;
@@ -24,6 +28,8 @@ struct altera_spi_platform_data {
 	u32				bits_per_word_mask;
 	u16				num_devices;
 	struct spi_board_info		*devices;
+	bool				use_parent_regmap;
+	u32				regoff;
 };
 
 #endif /* __LINUX_SPI_ALTERA_H */
-- 
2.7.4

[PATCH 5/6] spi: altera: move driver name string to header file

[Xu Yilun]

This allows other driver to reuse the name string for spi-altera
platform device creation.

Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
Signed-off-by: Wu Hao <hao.wu@intel.com>
Signed-off-by: Xu Yilun <yilun.xu@intel.com>
Signed-off-by: Russ Weight <russell.h.weight@intel.com>
---
 drivers/spi/spi-altera.c   | 6 ++----
 include/linux/spi/altera.h | 2 ++
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/spi/spi-altera.c b/drivers/spi/spi-altera.c
index 8d47c57..2c12c7a 100644
--- a/drivers/spi/spi-altera.c
+++ b/drivers/spi/spi-altera.c
@@ -19,8 +19,6 @@
 #include <linux/io.h>
 #include <linux/of.h>
 
-#define DRV_NAME "spi_altera"
-
 #define ALTERA_SPI_RXDATA	0
 #define ALTERA_SPI_TXDATA	4
 #define ALTERA_SPI_STATUS	8
@@ -338,7 +336,7 @@ MODULE_DEVICE_TABLE(of, altera_spi_match);
 static struct platform_driver altera_spi_driver = {
 	.probe = altera_spi_probe,
 	.driver = {
-		.name = DRV_NAME,
+		.name = ALTERA_SPI_DRV_NAME,
 		.pm = NULL,
 		.of_match_table = of_match_ptr(altera_spi_match),
 	},
@@ -348,4 +346,4 @@ module_platform_driver(altera_spi_driver);
 MODULE_DESCRIPTION("Altera SPI driver");
 MODULE_AUTHOR("Thomas Chou <thomas@wytron.com.tw>");
 MODULE_LICENSE("GPL");
-MODULE_ALIAS("platform:" DRV_NAME);
+MODULE_ALIAS("platform:" ALTERA_SPI_DRV_NAME);
diff --git a/include/linux/spi/altera.h b/include/linux/spi/altera.h
index 164ab7b..6539371 100644
--- a/include/linux/spi/altera.h
+++ b/include/linux/spi/altera.h
@@ -9,6 +9,8 @@
 #include <linux/spi/spi.h>
 #include <linux/types.h>
 
+#define ALTERA_SPI_DRV_NAME	"spi-altera"
+
 /**
  * struct altera_spi_platform_data - Platform data of the Altera SPI driver
  * @mode_bits:		Mode bits of SPI master.
-- 
2.7.4

[PATCH 6/6] spi: altera: fix size mismatch on 64 bit processors

[Xu Yilun]

From: Matthew Gerlach <matthew.gerlach@linux.intel.com>

The spi-altera driver was originally written with a 32
bit processor, where sizeof(unsigned long) is 4.  On a
64 bit processor sizeof(unsigned long) is 8.  Change the structure
member to u32 to match the actual size of the control
register.

Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
---
 drivers/spi/spi-altera.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/spi/spi-altera.c b/drivers/spi/spi-altera.c
index 2c12c7a..468fbd5 100644
--- a/drivers/spi/spi-altera.c
+++ b/drivers/spi/spi-altera.c
@@ -46,7 +46,7 @@ struct altera_spi {
 	int len;
 	int count;
 	int bytes_per_word;
-	unsigned long imr;
+	u32 imr;
 
 	/* data buffers */
 	const unsigned char *tx;
-- 
2.7.4

Re: [PATCH 4/6] spi: altera: use regmap instead of direct mmio register access

[Mark Brown]

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

On Thu, Jun 11, 2020 at 11:25:09AM +0800, Xu Yilun wrote:

> +	if (pdata && pdata->use_parent_regmap) {
> +		hw->regmap = dev_get_regmap(pdev->dev.parent, NULL);
> +		if (!hw->regmap) {
> +			dev_err(&pdev->dev, "get regmap failed\n");
> +			goto exit;
> +		}
> +		hw->base = pdata->regoff;

This seems very confused - there's some abstraction problem here.  This
looks like the driver should know about whatever is causing it to use
the parent regmap, it shouldn't just be "use the parent regmap" directly
since that is too much of an implementation detail.  This new feature
also seems like a separate change which should be in a separate patch,
the changelog only talked about converting to use regmap which I'd have
expected to be a straight 1:1 replacement of non-regmap stuff with
regmap stuff.

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

Re: [PATCH 6/6] spi: altera: fix size mismatch on 64 bit processors

[Mark Brown]

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

On Thu, Jun 11, 2020 at 11:25:11AM +0800, Xu Yilun wrote:
> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> 
> The spi-altera driver was originally written with a 32
> bit processor, where sizeof(unsigned long) is 4.  On a
> 64 bit processor sizeof(unsigned long) is 8.  Change the structure
> member to u32 to match the actual size of the control
> register.
> 
> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> ---

You've not provided a Signed-off-by for this, I can't do anything with
it.  For details on what Signed-off-by means and why it's important
please see Documentation/process/submitting-patches.rst.

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

Re: [PATCH 0/6] Add more configuration and regmap support for spi-altera

[Tom Rix]

This patchset looks good to me.

Reviewed-by: Tom Rix <trix@redhat.com>

Thanks,

Tom

On 6/10/20 8:25 PM, Xu Yilun wrote:
> This patchset adds platform_data for spi-altera, to enable more IP
> configurations, and creating specific spi client devices. It also adds
> regmap support, to enable the indirect access to this IP.
>
> We have a PCIE based FPGA platform which integrates this IP to communicate
> with a BMC chip (Intel MAX10) over SPI. The IP is configured as 32bit data
> width. There is also an indirect access interface in FPGA for host to
> access the registers of this IP. This patchset enables this use case.
>
> Matthew Gerlach (1):
>   spi: altera: fix size mismatch on 64 bit processors
>
> Xu Yilun (5):
>   spi: altera: add 32bit data width transfer support.
>   spi: altera: add SPI core parameters support via platform data.
>   spi: altera: add platform data for slave information.
>   spi: altera: use regmap instead of direct mmio register access
>   spi: altera: move driver name string to header file
>
>  drivers/spi/Kconfig        |   1 +
>  drivers/spi/spi-altera.c   | 161 +++++++++++++++++++++++++++++++++++++--------
>  include/linux/spi/altera.h |  37 +++++++++++
>  3 files changed, 171 insertions(+), 28 deletions(-)
>  create mode 100644 include/linux/spi/altera.h
>

Re: [PATCH 5/6] spi: altera: move driver name string to header file

[Mark Brown]

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

On Thu, Jun 11, 2020 at 11:25:10AM +0800, Xu Yilun wrote:
> This allows other driver to reuse the name string for spi-altera
> platform device creation.

This is a very unusual thing to do, normally we just have the users use
the strong directly.  It feels like if we are going to change this idiom
we should do so globally but that seems like far more trouble thanit's
worth.

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

Re: [PATCH 5/6] spi: altera: move driver name string to header file

[Xu Yilun]

On Thu, Jun 11, 2020 at 03:03:01PM +0100, Mark Brown wrote:
> On Thu, Jun 11, 2020 at 11:25:10AM +0800, Xu Yilun wrote:
> > This allows other driver to reuse the name string for spi-altera
> > platform device creation.
> 
> This is a very unusual thing to do, normally we just have the users use
> the strong directly.  It feels like if we are going to change this idiom
> we should do so globally but that seems like far more trouble thanit's
> worth.

Sure, I'll discard this patch and just use string for spi-altera device
creation.

Thanks,
Yilun.

Re: [PATCH 6/6] spi: altera: fix size mismatch on 64 bit processors

[Xu Yilun]

On Thu, Jun 11, 2020 at 12:04:08PM +0100, Mark Brown wrote:
> On Thu, Jun 11, 2020 at 11:25:11AM +0800, Xu Yilun wrote:
> > From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> > 
> > The spi-altera driver was originally written with a 32
> > bit processor, where sizeof(unsigned long) is 4.  On a
> > 64 bit processor sizeof(unsigned long) is 8.  Change the structure
> > member to u32 to match the actual size of the control
> > register.
> > 
> > Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> > ---
> 
> You've not provided a Signed-off-by for this, I can't do anything with
> it.  For details on what Signed-off-by means and why it's important
> please see Documentation/process/submitting-patches.rst.

Thanks for your explanation. I'll add my Signed-off-by.

Re: [PATCH 4/6] spi: altera: use regmap instead of direct mmio register access

[Xu Yilun]

On Thu, Jun 11, 2020 at 12:02:11PM +0100, Mark Brown wrote:
> On Thu, Jun 11, 2020 at 11:25:09AM +0800, Xu Yilun wrote:
> 
> > +	if (pdata && pdata->use_parent_regmap) {
> > +		hw->regmap = dev_get_regmap(pdev->dev.parent, NULL);
> > +		if (!hw->regmap) {
> > +			dev_err(&pdev->dev, "get regmap failed\n");
> > +			goto exit;
> > +		}
> > +		hw->base = pdata->regoff;
> 
> This seems very confused - there's some abstraction problem here.  This
> looks like the driver should know about whatever is causing it to use
> the parent regmap, it shouldn't just be "use the parent regmap" directly
> since that is too much of an implementation detail.  This new feature

Our usecase is that, we have the PCIE card which integrates this
spi-altera soft IP. So It seems reasonable we reuse this driver. But the
IP registers are not directly accessed by MMIO. It is accessed via an
indirect access interfaces, SW need to operate on the indirect access
interfaces to RW the spi-altera registers. like the following:

+------+   +------------+   +------------+
| PCIE |---| indirect   |---| spi-altera |
+------+   | access     |   +------------+
           | interfaces |
           +------------+
           | SPI params |
           +------------+

So we think of creating regmap to abstract the actually register accessing
detail. The parent device driver creates the regmap of indirect access,
and it creates the spi-altera platform device as child. Spi-altera
driver could just get the regmap from parent, don't have to care about
the indirect access detail.

It seems your concern is how to gracefully let spi-altera driver get the
regmap. or not using it. Since our platform doesn't enable device tree
support, seems the only way to talk to platform device is the
platform_data.

Do you have any suggestion on that?


I think the driver may need to figure out the role of the device in
system, whether it is a subdev of other device (like MFD? Many mfd subdev
driver will get parent regmap by default), or it is an independent mmio
device. But I'm not sure how to do it in right way.

> also seems like a separate change which should be in a separate patch,
> the changelog only talked about converting to use regmap which I'd have
> expected to be a straight 1:1 replacement of non-regmap stuff with
> regmap stuff.

Understood. I should make a patch to do 1:1 replacement of regmap first,
then a second patch to handle the parent regmap.

Re: [PATCH 4/6] spi: altera: use regmap instead of direct mmio register access

[Mark Brown]

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

On Fri, Jun 12, 2020 at 12:43:46PM +0800, Xu Yilun wrote:

> So we think of creating regmap to abstract the actually register accessing
> detail. The parent device driver creates the regmap of indirect access,
> and it creates the spi-altera platform device as child. Spi-altera
> driver could just get the regmap from parent, don't have to care about
> the indirect access detail.

To be clear there's absolutely no problem with the end result, my
concern is the way that we're getting there.

> It seems your concern is how to gracefully let spi-altera driver get the
> regmap. or not using it. Since our platform doesn't enable device tree
> support, seems the only way to talk to platform device is the
> platform_data.

No, the problem is with how that platform data is structured.  Based on
what you're saying I'd suggest adding another device ID for this - you
can use the id_table field in struct platform_driver to have more than
one ID like you can have more than one ACPI ID or OF compatible.  That
would mirror how this would be handled if things were enumerated through
firmware.

> I think the driver may need to figure out the role of the device in
> system, whether it is a subdev of other device (like MFD? Many mfd subdev
> driver will get parent regmap by default), or it is an independent mmio
> device. But I'm not sure how to do it in right way.

Yes, it sounds like this card is a MFD.

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

Re: [PATCH 4/6] spi: altera: use regmap instead of direct mmio register access

[Xu Yilun]

On Fri, Jun 12, 2020 at 12:52:02PM +0100, Mark Brown wrote:
> On Fri, Jun 12, 2020 at 12:43:46PM +0800, Xu Yilun wrote:
> 
> > So we think of creating regmap to abstract the actually register accessing
> > detail. The parent device driver creates the regmap of indirect access,
> > and it creates the spi-altera platform device as child. Spi-altera
> > driver could just get the regmap from parent, don't have to care about
> > the indirect access detail.
> 
> To be clear there's absolutely no problem with the end result, my
> concern is the way that we're getting there.
> 
> > It seems your concern is how to gracefully let spi-altera driver get the
> > regmap. or not using it. Since our platform doesn't enable device tree
> > support, seems the only way to talk to platform device is the
> > platform_data.
> 
> No, the problem is with how that platform data is structured.  Based on
> what you're saying I'd suggest adding another device ID for this - you
> can use the id_table field in struct platform_driver to have more than
> one ID like you can have more than one ACPI ID or OF compatible.  That
> would mirror how this would be handled if things were enumerated through
> firmware.

Thanks for your quick response. It's very clear and makes sense. I'll
change it.

> 
> > I think the driver may need to figure out the role of the device in
> > system, whether it is a subdev of other device (like MFD? Many mfd subdev
> > driver will get parent regmap by default), or it is an independent mmio
> > device. But I'm not sure how to do it in right way.
> 
> Yes, it sounds like this card is a MFD.

Re: [PATCH 0/6] Add more configuration and regmap support for spi-altera

[Mark Brown]

On Thu, 11 Jun 2020 11:25:05 +0800, Xu Yilun wrote:
> This patchset adds platform_data for spi-altera, to enable more IP
> configurations, and creating specific spi client devices. It also adds
> regmap support, to enable the indirect access to this IP.
> 
> We have a PCIE based FPGA platform which integrates this IP to communicate
> with a BMC chip (Intel MAX10) over SPI. The IP is configured as 32bit data
> width. There is also an indirect access interface in FPGA for host to
> access the registers of this IP. This patchset enables this use case.
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next

Thanks!

[1/3] spi: altera: add 32bit data width transfer support.
      commit: 3011d314751535782508a86bbd8de415ea99909f
[2/3] spi: altera: add SPI core parameters support via platform data.
      commit: 8e04187c1bc7953f6dfad3400c58b1b0b0ad767b
[3/3] spi: altera: add platform data for slave information.
      commit: 1fccd182a4694a848f2d6f3b1820d6fc71d9c99d

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark