[PATCH 1/3] spi: pl022: Drop custom per-chip cs_control

[PATCH 1/3] spi: pl022: Drop custom per-chip cs_control

[Linus Walleij]

Drop the custom cs_control() assigned through platform data,
we have no in-tree users and the only out-of-tree use I have
ever seen of this facility is to pull GPIO lines, which is
something the driver can already do for us.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/spi/spi-pl022.c    | 26 ++------------------------
 include/linux/amba/pl022.h |  3 ---
 2 files changed, 2 insertions(+), 27 deletions(-)

diff --git a/drivers/spi/spi-pl022.c b/drivers/spi/spi-pl022.c
index faaca7373b40..e5dd7756c2ea 100644
--- a/drivers/spi/spi-pl022.c
+++ b/drivers/spi/spi-pl022.c
@@ -412,7 +412,6 @@ struct pl022 {
  * @enable_dma: Whether to enable DMA or not
  * @read: function ptr to be used to read when doing xfer for this chip
  * @write: function ptr to be used to write when doing xfer for this chip
- * @cs_control: chip select callback provided by chip
  * @xfer_type: polling/interrupt/DMA
  *
  * Runtime state of the SSP controller, maintained per chip,
@@ -427,22 +426,9 @@ struct chip_data {
 	bool enable_dma;
 	enum ssp_reading read;
 	enum ssp_writing write;
-	void (*cs_control) (u32 command);
 	int xfer_type;
 };
 
-/**
- * null_cs_control - Dummy chip select function
- * @command: select/delect the chip
- *
- * If no chip select function is provided by client this is used as dummy
- * chip select
- */
-static void null_cs_control(u32 command)
-{
-	pr_debug("pl022: dummy chip select control, CS=0x%x\n", command);
-}
-
 /**
  * internal_cs_control - Control chip select signals via SSP_CSR.
  * @pl022: SSP driver private data structure
@@ -470,8 +456,6 @@ static void pl022_cs_control(struct pl022 *pl022, u32 command)
 		internal_cs_control(pl022, command);
 	else if (gpio_is_valid(pl022->cur_cs))
 		gpio_set_value(pl022->cur_cs, command);
-	else
-		pl022->cur_chip->cs_control(command);
 }
 
 /**
@@ -1829,7 +1813,6 @@ static const struct pl022_config_chip pl022_default_chip_info = {
 	.ctrl_len = SSP_BITS_8,
 	.wait_state = SSP_MWIRE_WAIT_ZERO,
 	.duplex = SSP_MICROWIRE_CHANNEL_FULL_DUPLEX,
-	.cs_control = null_cs_control,
 };
 
 /**
@@ -1940,13 +1923,8 @@ static int pl022_setup(struct spi_device *spi)
 
 	/* Now set controller state based on controller data */
 	chip->xfer_type = chip_info->com_mode;
-	if (!chip_info->cs_control) {
-		chip->cs_control = null_cs_control;
-		if (!gpio_is_valid(pl022->chipselects[spi->chip_select]))
-			dev_warn(&spi->dev,
-				 "invalid chip select\n");
-	} else
-		chip->cs_control = chip_info->cs_control;
+	if (!gpio_is_valid(pl022->chipselects[spi->chip_select]))
+		dev_warn(&spi->dev, "invalid chip select\n");
 
 	/* Check bits per word with vendor specific range */
 	if ((bits <= 3) || (bits > pl022->vendor->max_bpw)) {
diff --git a/include/linux/amba/pl022.h b/include/linux/amba/pl022.h
index 131b27c97209..29274cedefde 100644
--- a/include/linux/amba/pl022.h
+++ b/include/linux/amba/pl022.h
@@ -265,8 +265,6 @@ struct pl022_ssp_controller {
  * @duplex: Microwire interface: Full/Half duplex
  * @clkdelay: on the PL023 variant, the delay in feeback clock cycles
  * before sampling the incoming line
- * @cs_control: function pointer to board-specific function to
- * assert/deassert I/O port to control HW generation of devices chip-select.
  */
 struct pl022_config_chip {
 	enum ssp_interface iface;
@@ -280,7 +278,6 @@ struct pl022_config_chip {
 	enum ssp_microwire_wait_state wait_state;
 	enum ssp_duplex duplex;
 	enum ssp_clkdelay clkdelay;
-	void (*cs_control) (u32 control);
 };
 
 #endif /* _SSP_PL022_H */
-- 
2.29.2

[PATCH 2/3] spi: pl022: Use GPIOs looked up by the core

[Linus Walleij]

The SPI core looks up GPIO lines from the device tree,
so let's stop trying to do that on our own and rely
on the core to do this for us.

In addition to the GPIO line we also need to keep
track of the chip select index separately, as the native
chip select needs this index. The driver was reusing
the same GPIO array for native chip select indices,
so keep this in a separate state variable instead.

The facility to pass in custom GPIO lines from the
platform data can go, because even if we do have
out-of-tree code that want to use platform data, they
can soon pass in GPIOs using machine GPIO descriptor
tables which will be available after the next step
when we convert the driver to using GPIO descriptors.

The implicit inclusion of <linux/of.h> is made
explicit as we no longer need to include <linux/of_gpio.h>.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/spi/spi-pl022.c    | 70 ++++++--------------------------------
 include/linux/amba/pl022.h |  7 ----
 2 files changed, 10 insertions(+), 67 deletions(-)

diff --git a/drivers/spi/spi-pl022.c b/drivers/spi/spi-pl022.c
index e5dd7756c2ea..84ffed2a3554 100644
--- a/drivers/spi/spi-pl022.c
+++ b/drivers/spi/spi-pl022.c
@@ -32,7 +32,7 @@
 #include <linux/scatterlist.h>
 #include <linux/pm_runtime.h>
 #include <linux/gpio.h>
-#include <linux/of_gpio.h>
+#include <linux/of.h>
 #include <linux/pinctrl/consumer.h>
 
 /*
@@ -362,8 +362,8 @@ struct vendor_data {
  * @sgt_tx: scattertable for the TX transfer
  * @dummypage: a dummy page used for driving data on the bus with DMA
  * @dma_running: indicates whether DMA is in operation
- * @cur_cs: current chip select (gpio)
- * @chipselects: list of chipselects (gpios)
+ * @cur_cs: current chip select index
+ * @cur_gpio: current chip select GPIO line
  */
 struct pl022 {
 	struct amba_device		*adev;
@@ -398,7 +398,7 @@ struct pl022 {
 	bool				dma_running;
 #endif
 	int cur_cs;
-	int *chipselects;
+	int cur_gpio;
 };
 
 /**
@@ -454,8 +454,8 @@ static void pl022_cs_control(struct pl022 *pl022, u32 command)
 {
 	if (pl022->vendor->internal_cs_ctrl)
 		internal_cs_control(pl022, command);
-	else if (gpio_is_valid(pl022->cur_cs))
-		gpio_set_value(pl022->cur_cs, command);
+	else if (gpio_is_valid(pl022->cur_gpio))
+		gpio_set_value(pl022->cur_gpio, command);
 }
 
 /**
@@ -1580,7 +1580,9 @@ static int pl022_transfer_one_message(struct spi_master *master,
 
 	/* Setup the SPI using the per chip configuration */
 	pl022->cur_chip = spi_get_ctldata(msg->spi);
-	pl022->cur_cs = pl022->chipselects[msg->spi->chip_select];
+	pl022->cur_cs = msg->spi->chip_select;
+	/* This is always available but may be set to -ENOENT */
+	pl022->cur_gpio = msg->spi->cs_gpio;
 
 	restore_state(pl022);
 	flush(pl022);
@@ -1923,8 +1925,6 @@ static int pl022_setup(struct spi_device *spi)
 
 	/* Now set controller state based on controller data */
 	chip->xfer_type = chip_info->com_mode;
-	if (!gpio_is_valid(pl022->chipselects[spi->chip_select]))
-		dev_warn(&spi->dev, "invalid chip select\n");
 
 	/* Check bits per word with vendor specific range */
 	if ((bits <= 3) || (bits > pl022->vendor->max_bpw)) {
@@ -2072,7 +2072,6 @@ pl022_platform_data_dt_get(struct device *dev)
 {
 	struct device_node *np = dev->of_node;
 	struct pl022_ssp_controller *pd;
-	u32 tmp = 0;
 
 	if (!np) {
 		dev_err(dev, "no dt node defined\n");
@@ -2085,8 +2084,6 @@ pl022_platform_data_dt_get(struct device *dev)
 
 	pd->bus_id = -1;
 	pd->enable_dma = 1;
-	of_property_read_u32(np, "num-cs", &tmp);
-	pd->num_chipselect = tmp;
 	of_property_read_u32(np, "pl022,autosuspend-delay",
 			     &pd->autosuspend_delay);
 	pd->rt = of_property_read_bool(np, "pl022,rt");
@@ -2101,8 +2098,7 @@ static int pl022_probe(struct amba_device *adev, const struct amba_id *id)
 			dev_get_platdata(&adev->dev);
 	struct spi_master *master;
 	struct pl022 *pl022 = NULL;	/*Data for this driver */
-	struct device_node *np = adev->dev.of_node;
-	int status = 0, i, num_cs;
+	int status = 0;
 
 	dev_info(&adev->dev,
 		 "ARM PL022 driver, device ID: 0x%08x\n", adev->periphid);
@@ -2114,13 +2110,6 @@ static int pl022_probe(struct amba_device *adev, const struct amba_id *id)
 		return -ENODEV;
 	}
 
-	if (platform_info->num_chipselect) {
-		num_cs = platform_info->num_chipselect;
-	} else {
-		dev_err(dev, "probe: no chip select defined\n");
-		return -ENODEV;
-	}
-
 	/* Allocate master with space for data */
 	master = spi_alloc_master(dev, sizeof(struct pl022));
 	if (master == NULL) {
@@ -2133,19 +2122,12 @@ static int pl022_probe(struct amba_device *adev, const struct amba_id *id)
 	pl022->master_info = platform_info;
 	pl022->adev = adev;
 	pl022->vendor = id->data;
-	pl022->chipselects = devm_kcalloc(dev, num_cs, sizeof(int),
-					  GFP_KERNEL);
-	if (!pl022->chipselects) {
-		status = -ENOMEM;
-		goto err_no_mem;
-	}
 
 	/*
 	 * Bus Number Which has been Assigned to this SSP controller
 	 * on this board
 	 */
 	master->bus_num = platform_info->bus_id;
-	master->num_chipselect = num_cs;
 	master->cleanup = pl022_cleanup;
 	master->setup = pl022_setup;
 	master->auto_runtime_pm = true;
@@ -2154,36 +2136,6 @@ static int pl022_probe(struct amba_device *adev, const struct amba_id *id)
 	master->rt = platform_info->rt;
 	master->dev.of_node = dev->of_node;
 
-	if (platform_info->num_chipselect && platform_info->chipselects) {
-		for (i = 0; i < num_cs; i++)
-			pl022->chipselects[i] = platform_info->chipselects[i];
-	} else if (pl022->vendor->internal_cs_ctrl) {
-		for (i = 0; i < num_cs; i++)
-			pl022->chipselects[i] = i;
-	} else if (IS_ENABLED(CONFIG_OF)) {
-		for (i = 0; i < num_cs; i++) {
-			int cs_gpio = of_get_named_gpio(np, "cs-gpios", i);
-
-			if (cs_gpio == -EPROBE_DEFER) {
-				status = -EPROBE_DEFER;
-				goto err_no_gpio;
-			}
-
-			pl022->chipselects[i] = cs_gpio;
-
-			if (gpio_is_valid(cs_gpio)) {
-				if (devm_gpio_request(dev, cs_gpio, "ssp-pl022"))
-					dev_err(&adev->dev,
-						"could not request %d gpio\n",
-						cs_gpio);
-				else if (gpio_direction_output(cs_gpio, 1))
-					dev_err(&adev->dev,
-						"could not set gpio %d as output\n",
-						cs_gpio);
-			}
-		}
-	}
-
 	/*
 	 * Supports mode 0-3, loopback, and active low CS. Transfers are
 	 * always MS bit first on the original pl022.
@@ -2286,8 +2238,6 @@ static int pl022_probe(struct amba_device *adev, const struct amba_id *id)
  err_no_ioremap:
 	amba_release_regions(adev);
  err_no_ioregion:
- err_no_gpio:
- err_no_mem:
 	spi_master_put(master);
 	return status;
 }
diff --git a/include/linux/amba/pl022.h b/include/linux/amba/pl022.h
index 29274cedefde..9bf58aac0df2 100644
--- a/include/linux/amba/pl022.h
+++ b/include/linux/amba/pl022.h
@@ -223,10 +223,6 @@ struct dma_chan;
 /**
  * struct pl022_ssp_master - device.platform_data for SPI controller devices.
  * @bus_id: identifier for this bus
- * @num_chipselect: chipselects are used to distinguish individual
- *     SPI slaves, and are numbered from zero to num_chipselects - 1.
- *     each slave has a chipselect signal, but it's common that not
- *     every chipselect is connected to a slave.
  * @enable_dma: if true enables DMA driven transfers.
  * @dma_rx_param: parameter to locate an RX DMA channel.
  * @dma_tx_param: parameter to locate a TX DMA channel.
@@ -235,18 +231,15 @@ struct dma_chan;
  *     indicates no delay and the device will be suspended immediately.
  * @rt: indicates the controller should run the message pump with realtime
  *     priority to minimise the transfer latency on the bus.
- * @chipselects: list of <num_chipselects> chip select gpios
  */
 struct pl022_ssp_controller {
 	u16 bus_id;
-	u8 num_chipselect;
 	u8 enable_dma:1;
 	bool (*dma_filter)(struct dma_chan *chan, void *filter_param);
 	void *dma_rx_param;
 	void *dma_tx_param;
 	int autosuspend_delay;
 	bool rt;
-	int *chipselects;
 };
 
 /**
-- 
2.29.2

[PATCH 3/3] spi: pl022: Convert to use GPIO descriptors

[Linus Walleij]

This converts the PL022 driver to use GPIO descriptors
instead of the old global GPIO numberspace. Since the
driver handles messages on its own it needs to manage
the GPIO descriptor directly.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/spi/spi-pl022.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/drivers/spi/spi-pl022.c b/drivers/spi/spi-pl022.c
index 84ffed2a3554..fda025a5ec06 100644
--- a/drivers/spi/spi-pl022.c
+++ b/drivers/spi/spi-pl022.c
@@ -31,7 +31,6 @@
 #include <linux/dma-mapping.h>
 #include <linux/scatterlist.h>
 #include <linux/pm_runtime.h>
-#include <linux/gpio.h>
 #include <linux/of.h>
 #include <linux/pinctrl/consumer.h>
 
@@ -363,7 +362,7 @@ struct vendor_data {
  * @dummypage: a dummy page used for driving data on the bus with DMA
  * @dma_running: indicates whether DMA is in operation
  * @cur_cs: current chip select index
- * @cur_gpio: current chip select GPIO line
+ * @cur_gpiod: current chip select GPIO descriptor
  */
 struct pl022 {
 	struct amba_device		*adev;
@@ -398,7 +397,7 @@ struct pl022 {
 	bool				dma_running;
 #endif
 	int cur_cs;
-	int cur_gpio;
+	struct gpio_desc *cur_gpiod;
 };
 
 /**
@@ -454,8 +453,16 @@ static void pl022_cs_control(struct pl022 *pl022, u32 command)
 {
 	if (pl022->vendor->internal_cs_ctrl)
 		internal_cs_control(pl022, command);
-	else if (gpio_is_valid(pl022->cur_gpio))
-		gpio_set_value(pl022->cur_gpio, command);
+	else if (pl022->cur_gpiod)
+		/*
+		 * This needs to be inverted since with GPIOLIB in
+		 * control, the inversion will be handled by
+		 * GPIOLIB's active low handling. The "command"
+		 * passed into this function will be SSP_CHIP_SELECT
+		 * which is enum:ed to 0, so we need the inverse
+		 * (1) to activate chip select.
+		 */
+		gpiod_set_value(pl022->cur_gpiod, !command);
 }
 
 /**
@@ -1582,7 +1589,7 @@ static int pl022_transfer_one_message(struct spi_master *master,
 	pl022->cur_chip = spi_get_ctldata(msg->spi);
 	pl022->cur_cs = msg->spi->chip_select;
 	/* This is always available but may be set to -ENOENT */
-	pl022->cur_gpio = msg->spi->cs_gpio;
+	pl022->cur_gpiod = msg->spi->cs_gpiod;
 
 	restore_state(pl022);
 	flush(pl022);
@@ -2135,6 +2142,7 @@ static int pl022_probe(struct amba_device *adev, const struct amba_id *id)
 	master->unprepare_transfer_hardware = pl022_unprepare_transfer_hardware;
 	master->rt = platform_info->rt;
 	master->dev.of_node = dev->of_node;
+	master->use_gpio_descriptors = true;
 
 	/*
 	 * Supports mode 0-3, loopback, and active low CS. Transfers are
-- 
2.29.2

Re: [PATCH 1/3] spi: pl022: Drop custom per-chip cs_control

[Mark Brown]

On Tue, 30 Mar 2021 18:49:05 +0200, Linus Walleij wrote:
> Drop the custom cs_control() assigned through platform data,
> we have no in-tree users and the only out-of-tree use I have
> ever seen of this facility is to pull GPIO lines, which is
> something the driver can already do for us.

Applied to

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

Thanks!

[1/3] spi: pl022: Drop custom per-chip cs_control
      commit: 4179e576b56d82e5ce007b9f548efb90605e2713
[2/3] spi: pl022: Use GPIOs looked up by the core
      commit: 77f983a9df421fa00ca6a2f494dc79f8afca75a2
[3/3] spi: pl022: Convert to use GPIO descriptors
      commit: 8bb2dbf1e14d05e92a23e03bcbd1c27f7ee937f7

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