[PATCH v1 0/5] spi: pxa2xx: Cleanup (part 2)

[PATCH v1 0/5] spi: pxa2xx: Cleanup (part 2)

[Andy Shevchenko]

Here is the additional cleanup of the driver based on the fact of
the linux/spi/pxa2xx_spi.h being a local (to drivers/spi/) header.
This means it's based on top of "spi: pxa2xx: Drop linux/spi/pxa2xx_spi.h"
(20240327193138.2385910-1-andriy.shevchenko@linux.intel.com).

Andy Shevchenko (5):
  spi: pxa2xx: Move number of CS pins validation out of condition
  spi: pxa2xx: Drop struct pxa2xx_spi_chip
  spi: pxa2xx: Remove DMA parameters from struct chip_data
  spi: pxa2xx: Remove timeout field from struct chip_data
  spi: pxa2xx: Don't provide struct chip_data for others

 drivers/spi/spi-pxa2xx-dma.c | 27 +----------
 drivers/spi/spi-pxa2xx.c     | 92 +++++++++---------------------------
 drivers/spi/spi-pxa2xx.h     | 31 ------------
 3 files changed, 25 insertions(+), 125 deletions(-)

-- 
2.43.0.rc1.1.gbec44491f096

[PATCH v1 1/5] spi: pxa2xx: Move number of CS pins validation out of condition

[Andy Shevchenko]

There is no need to allocate chip_data and then validate number of
CS pins as it will have the same effect. Hence move number of CS pins
validation out of condition in setup().

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/spi/spi-pxa2xx.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/drivers/spi/spi-pxa2xx.c b/drivers/spi/spi-pxa2xx.c
index 1348249f8178..cc0e54f8d2c3 100644
--- a/drivers/spi/spi-pxa2xx.c
+++ b/drivers/spi/spi-pxa2xx.c
@@ -1200,6 +1200,13 @@ static int setup(struct spi_device *spi)
 		break;
 	}
 
+	if (drv_data->ssp_type == CE4100_SSP) {
+		if (spi_get_chipselect(spi, 0) > 4) {
+			dev_err(&spi->dev, "failed setup: cs number must not be > 4.\n");
+			return -EINVAL;
+		}
+	}
+
 	/* Only allocate on the first setup */
 	chip = spi_get_ctldata(spi);
 	if (!chip) {
@@ -1207,14 +1214,6 @@ static int setup(struct spi_device *spi)
 		if (!chip)
 			return -ENOMEM;
 
-		if (drv_data->ssp_type == CE4100_SSP) {
-			if (spi_get_chipselect(spi, 0) > 4) {
-				dev_err(&spi->dev,
-					"failed setup: cs number must not be > 4.\n");
-				kfree(chip);
-				return -EINVAL;
-			}
-		}
 		chip->enable_dma = drv_data->controller_info->enable_dma;
 		chip->timeout = TIMOUT_DFLT;
 	}
-- 
2.43.0.rc1.1.gbec44491f096

[PATCH v1 2/5] spi: pxa2xx: Drop struct pxa2xx_spi_chip

[Andy Shevchenko]

No more users.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/spi/spi-pxa2xx-dma.c |  9 ++-------
 drivers/spi/spi-pxa2xx.c     | 25 -------------------------
 drivers/spi/spi-pxa2xx.h     | 14 --------------
 3 files changed, 2 insertions(+), 46 deletions(-)

diff --git a/drivers/spi/spi-pxa2xx-dma.c b/drivers/spi/spi-pxa2xx-dma.c
index 751cb0f77b62..d77279c3acd8 100644
--- a/drivers/spi/spi-pxa2xx-dma.c
+++ b/drivers/spi/spi-pxa2xx-dma.c
@@ -231,16 +231,11 @@ int pxa2xx_spi_set_dma_burst_and_threshold(struct chip_data *chip,
 					   u8 bits_per_word, u32 *burst_code,
 					   u32 *threshold)
 {
-	struct pxa2xx_spi_chip *chip_info = spi->controller_data;
 	struct driver_data *drv_data = spi_controller_get_devdata(spi->controller);
 	u32 dma_burst_size = drv_data->controller_info->dma_burst_size;
 
-	/*
-	 * If the DMA burst size is given in chip_info we use that,
-	 * otherwise we use the default. Also we use the default FIFO
-	 * thresholds for now.
-	 */
-	*burst_code = chip_info ? chip_info->dma_burst_size : dma_burst_size;
+	/* We use the default the DMA burst size and FIFO thresholds for now */
+	*burst_code = dma_burst_size;
 	*threshold = SSCR1_RxTresh(RX_THRESH_DFLT)
 		   | SSCR1_TxTresh(TX_THRESH_DFLT);
 
diff --git a/drivers/spi/spi-pxa2xx.c b/drivers/spi/spi-pxa2xx.c
index cc0e54f8d2c3..00aa33c937bf 100644
--- a/drivers/spi/spi-pxa2xx.c
+++ b/drivers/spi/spi-pxa2xx.c
@@ -1154,7 +1154,6 @@ static int pxa2xx_spi_unprepare_transfer(struct spi_controller *controller)
 
 static int setup(struct spi_device *spi)
 {
-	struct pxa2xx_spi_chip *chip_info;
 	struct chip_data *chip;
 	const struct lpss_config *config;
 	struct driver_data *drv_data =
@@ -1218,25 +1217,6 @@ static int setup(struct spi_device *spi)
 		chip->timeout = TIMOUT_DFLT;
 	}
 
-	/*
-	 * Protocol drivers may change the chip settings, so...
-	 * if chip_info exists, use it.
-	 */
-	chip_info = spi->controller_data;
-
-	/* chip_info isn't always needed */
-	if (chip_info) {
-		if (chip_info->timeout)
-			chip->timeout = chip_info->timeout;
-		if (chip_info->tx_threshold)
-			tx_thres = chip_info->tx_threshold;
-		if (chip_info->tx_hi_threshold)
-			tx_hi_thres = chip_info->tx_hi_threshold;
-		if (chip_info->rx_threshold)
-			rx_thres = chip_info->rx_threshold;
-		chip->dma_threshold = 0;
-	}
-
 	chip->cr1 = 0;
 	if (spi_controller_is_target(drv_data->controller)) {
 		chip->cr1 |= SSCR1_SCFR;
@@ -1256,11 +1236,6 @@ static int setup(struct spi_device *spi)
 		chip->lpss_tx_threshold = tx_thres;
 	}
 
-	/*
-	 * Set DMA burst and threshold outside of chip_info path so that if
-	 * chip_info goes away after setting chip->enable_dma, the burst and
-	 * threshold can still respond to changes in bits_per_word.
-	 */
 	if (chip->enable_dma) {
 		/* Set up legal burst and threshold for DMA */
 		if (pxa2xx_spi_set_dma_burst_and_threshold(chip, spi,
diff --git a/drivers/spi/spi-pxa2xx.h b/drivers/spi/spi-pxa2xx.h
index f1097c96c50f..ae9c99bc9f6c 100644
--- a/drivers/spi/spi-pxa2xx.h
+++ b/drivers/spi/spi-pxa2xx.h
@@ -35,20 +35,6 @@ struct pxa2xx_spi_controller {
 	struct ssp_device ssp;
 };
 
-/*
- * The controller specific data for SPI target devices
- * (resides in spi_board_info.controller_data),
- * copied to spi_device.platform_data ... mostly for
- * DMA tuning.
- */
-struct pxa2xx_spi_chip {
-	u8 tx_threshold;
-	u8 tx_hi_threshold;
-	u8 rx_threshold;
-	u8 dma_burst_size;
-	u32 timeout;
-};
-
 struct spi_controller;
 struct spi_device;
 struct spi_transfer;
-- 
2.43.0.rc1.1.gbec44491f096

[PATCH v1 3/5] spi: pxa2xx: Remove DMA parameters from struct chip_data

[Andy Shevchenko]

The DMA related fields are set once and never modified. It effectively
repeats the content of the same fields in struct pxa2xx_spi_controller.
With that, remove DMA parameters from struct chip_data.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/spi/spi-pxa2xx-dma.c | 22 ++------------------
 drivers/spi/spi-pxa2xx.c     | 40 +++++++-----------------------------
 drivers/spi/spi-pxa2xx.h     |  8 --------
 3 files changed, 9 insertions(+), 61 deletions(-)

diff --git a/drivers/spi/spi-pxa2xx-dma.c b/drivers/spi/spi-pxa2xx-dma.c
index d77279c3acd8..08cb6e96ac94 100644
--- a/drivers/spi/spi-pxa2xx-dma.c
+++ b/drivers/spi/spi-pxa2xx-dma.c
@@ -68,8 +68,6 @@ pxa2xx_spi_dma_prepare_one(struct driver_data *drv_data,
 			   enum dma_transfer_direction dir,
 			   struct spi_transfer *xfer)
 {
-	struct chip_data *chip =
-		spi_get_ctldata(drv_data->controller->cur_msg->spi);
 	enum dma_slave_buswidth width;
 	struct dma_slave_config cfg;
 	struct dma_chan *chan;
@@ -94,14 +92,14 @@ pxa2xx_spi_dma_prepare_one(struct driver_data *drv_data,
 	if (dir == DMA_MEM_TO_DEV) {
 		cfg.dst_addr = drv_data->ssp->phys_base + SSDR;
 		cfg.dst_addr_width = width;
-		cfg.dst_maxburst = chip->dma_burst_size;
+		cfg.dst_maxburst = drv_data->controller_info->dma_burst_size;
 
 		sgt = &xfer->tx_sg;
 		chan = drv_data->controller->dma_tx;
 	} else {
 		cfg.src_addr = drv_data->ssp->phys_base + SSDR;
 		cfg.src_addr_width = width;
-		cfg.src_maxburst = chip->dma_burst_size;
+		cfg.src_maxburst = drv_data->controller_info->dma_burst_size;
 
 		sgt = &xfer->rx_sg;
 		chan = drv_data->controller->dma_rx;
@@ -225,19 +223,3 @@ void pxa2xx_spi_dma_release(struct driver_data *drv_data)
 		controller->dma_tx = NULL;
 	}
 }
-
-int pxa2xx_spi_set_dma_burst_and_threshold(struct chip_data *chip,
-					   struct spi_device *spi,
-					   u8 bits_per_word, u32 *burst_code,
-					   u32 *threshold)
-{
-	struct driver_data *drv_data = spi_controller_get_devdata(spi->controller);
-	u32 dma_burst_size = drv_data->controller_info->dma_burst_size;
-
-	/* We use the default the DMA burst size and FIFO thresholds for now */
-	*burst_code = dma_burst_size;
-	*threshold = SSCR1_RxTresh(RX_THRESH_DFLT)
-		   | SSCR1_TxTresh(TX_THRESH_DFLT);
-
-	return 0;
-}
diff --git a/drivers/spi/spi-pxa2xx.c b/drivers/spi/spi-pxa2xx.c
index 00aa33c937bf..3ba0f5816f7f 100644
--- a/drivers/spi/spi-pxa2xx.c
+++ b/drivers/spi/spi-pxa2xx.c
@@ -934,11 +934,11 @@ static bool pxa2xx_spi_can_dma(struct spi_controller *controller,
 			       struct spi_device *spi,
 			       struct spi_transfer *xfer)
 {
-	struct chip_data *chip = spi_get_ctldata(spi);
+	struct driver_data *drv_data = spi_controller_get_devdata(controller);
 
-	return chip->enable_dma &&
+	return drv_data->controller_info->enable_dma &&
 	       xfer->len <= MAX_DMA_LEN &&
-	       xfer->len >= chip->dma_burst_size;
+	       xfer->len >= drv_data->controller_info->dma_burst_size;
 }
 
 static int pxa2xx_spi_transfer_one(struct spi_controller *controller,
@@ -947,9 +947,8 @@ static int pxa2xx_spi_transfer_one(struct spi_controller *controller,
 {
 	struct driver_data *drv_data = spi_controller_get_devdata(controller);
 	struct chip_data *chip = spi_get_ctldata(spi);
-	u32 dma_thresh = chip->dma_threshold;
-	u32 dma_burst = chip->dma_burst_size;
 	u32 change_mask = pxa2xx_spi_get_ssrc1_change_mask(drv_data);
+	u32 dma_thresh;
 	u32 clk_div;
 	u8 bits;
 	u32 speed;
@@ -959,7 +958,7 @@ static int pxa2xx_spi_transfer_one(struct spi_controller *controller,
 	int dma_mapped;
 
 	/* Check if we can DMA this transfer */
-	if (transfer->len > MAX_DMA_LEN && chip->enable_dma) {
+	if (transfer->len > MAX_DMA_LEN && drv_data->controller_info->enable_dma) {
 		/* Warn ... we force this to PIO mode */
 		dev_warn_ratelimited(&spi->dev,
 				     "DMA disabled for transfer length %u greater than %d\n",
@@ -995,19 +994,8 @@ static int pxa2xx_spi_transfer_one(struct spi_controller *controller,
 		drv_data->read = drv_data->rx ? u32_reader : null_reader;
 		drv_data->write = drv_data->tx ? u32_writer : null_writer;
 	}
-	/*
-	 * If bits per word is changed in DMA mode, then must check
-	 * the thresholds and burst also.
-	 */
-	if (chip->enable_dma) {
-		if (pxa2xx_spi_set_dma_burst_and_threshold(chip,
-						spi,
-						bits, &dma_burst,
-						&dma_thresh))
-			dev_warn_ratelimited(&spi->dev,
-					     "DMA burst size reduced to match bits_per_word\n");
-	}
 
+	dma_thresh = SSCR1_RxTresh(RX_THRESH_DFLT) | SSCR1_TxTresh(TX_THRESH_DFLT);
 	dma_mapped = controller->can_dma &&
 		     controller->can_dma(controller, spi, transfer) &&
 		     controller->cur_msg_mapped;
@@ -1213,7 +1201,6 @@ static int setup(struct spi_device *spi)
 		if (!chip)
 			return -ENOMEM;
 
-		chip->enable_dma = drv_data->controller_info->enable_dma;
 		chip->timeout = TIMOUT_DFLT;
 	}
 
@@ -1236,20 +1223,6 @@ static int setup(struct spi_device *spi)
 		chip->lpss_tx_threshold = tx_thres;
 	}
 
-	if (chip->enable_dma) {
-		/* Set up legal burst and threshold for DMA */
-		if (pxa2xx_spi_set_dma_burst_and_threshold(chip, spi,
-						spi->bits_per_word,
-						&chip->dma_burst_size,
-						&chip->dma_threshold)) {
-			dev_warn(&spi->dev,
-				 "in setup: DMA burst size reduced to match bits_per_word\n");
-		}
-		dev_dbg(&spi->dev,
-			"in setup: DMA burst size set to %u\n",
-			chip->dma_burst_size);
-	}
-
 	switch (drv_data->ssp_type) {
 	case QUARK_X1000_SSP:
 		chip->threshold = (QUARK_X1000_SSCR1_RxTresh(rx_thres)
@@ -1439,6 +1412,7 @@ static int pxa2xx_spi_probe(struct platform_device *pdev)
 		if (IS_ERR(platform_info))
 			return dev_err_probe(dev, PTR_ERR(platform_info), "missing platform data\n");
 	}
+	dev_dbg(dev, "DMA burst size set to %u\n", platform_info->dma_burst_size);
 
 	ssp = pxa_ssp_request(pdev->id, pdev->name);
 	if (!ssp)
diff --git a/drivers/spi/spi-pxa2xx.h b/drivers/spi/spi-pxa2xx.h
index ae9c99bc9f6c..10294ef209d9 100644
--- a/drivers/spi/spi-pxa2xx.h
+++ b/drivers/spi/spi-pxa2xx.h
@@ -79,9 +79,6 @@ struct chip_data {
 	u32 cr1;
 	u32 dds_rate;
 	u32 timeout;
-	u8 enable_dma;
-	u32 dma_burst_size;
-	u32 dma_threshold;
 	u32 threshold;
 	u16 lpss_rx_threshold;
 	u16 lpss_tx_threshold;
@@ -142,10 +139,5 @@ extern void pxa2xx_spi_dma_start(struct driver_data *drv_data);
 extern void pxa2xx_spi_dma_stop(struct driver_data *drv_data);
 extern int pxa2xx_spi_dma_setup(struct driver_data *drv_data);
 extern void pxa2xx_spi_dma_release(struct driver_data *drv_data);
-extern int pxa2xx_spi_set_dma_burst_and_threshold(struct chip_data *chip,
-						  struct spi_device *spi,
-						  u8 bits_per_word,
-						  u32 *burst_code,
-						  u32 *threshold);
 
 #endif /* SPI_PXA2XX_H */
-- 
2.43.0.rc1.1.gbec44491f096

[PATCH v1 4/5] spi: pxa2xx: Remove timeout field from struct chip_data

[Andy Shevchenko]

The timeout field is used only once and assigned to a predefined
constant. Replace all that by using the constant directly.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/spi/spi-pxa2xx.c | 4 +---
 drivers/spi/spi-pxa2xx.h | 1 -
 2 files changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/spi/spi-pxa2xx.c b/drivers/spi/spi-pxa2xx.c
index 3ba0f5816f7f..030afb17e606 100644
--- a/drivers/spi/spi-pxa2xx.c
+++ b/drivers/spi/spi-pxa2xx.c
@@ -1058,7 +1058,7 @@ static int pxa2xx_spi_transfer_one(struct spi_controller *controller,
 		pxa_ssp_disable(drv_data->ssp);
 
 	if (!pxa25x_ssp_comp(drv_data))
-		pxa2xx_spi_write(drv_data, SSTO, chip->timeout);
+		pxa2xx_spi_write(drv_data, SSTO, TIMOUT_DFLT);
 
 	/* First set CR1 without interrupt and service enables */
 	pxa2xx_spi_update(drv_data, SSCR1, change_mask, cr1);
@@ -1200,8 +1200,6 @@ static int setup(struct spi_device *spi)
 		chip = kzalloc(sizeof(struct chip_data), GFP_KERNEL);
 		if (!chip)
 			return -ENOMEM;
-
-		chip->timeout = TIMOUT_DFLT;
 	}
 
 	chip->cr1 = 0;
diff --git a/drivers/spi/spi-pxa2xx.h b/drivers/spi/spi-pxa2xx.h
index 10294ef209d9..5f741bb30240 100644
--- a/drivers/spi/spi-pxa2xx.h
+++ b/drivers/spi/spi-pxa2xx.h
@@ -78,7 +78,6 @@ struct driver_data {
 struct chip_data {
 	u32 cr1;
 	u32 dds_rate;
-	u32 timeout;
 	u32 threshold;
 	u16 lpss_rx_threshold;
 	u16 lpss_tx_threshold;
-- 
2.43.0.rc1.1.gbec44491f096

[PATCH v1 5/5] spi: pxa2xx: Don't provide struct chip_data for others

[Andy Shevchenko]

Now the struct chip_data is local to spi-pxa2xx.c, move
its definition to the C file. This will slightly speed up
a build and also hide badly named data type (too generic).

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/spi/spi-pxa2xx.c | 8 ++++++++
 drivers/spi/spi-pxa2xx.h | 8 --------
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/spi/spi-pxa2xx.c b/drivers/spi/spi-pxa2xx.c
index 030afb17e606..efe76d0c21bb 100644
--- a/drivers/spi/spi-pxa2xx.c
+++ b/drivers/spi/spi-pxa2xx.c
@@ -66,6 +66,14 @@ MODULE_ALIAS("platform:pxa2xx-spi");
 				| CE4100_SSCR1_RFT | CE4100_SSCR1_TFT | SSCR1_MWDS \
 				| SSCR1_SPH | SSCR1_SPO | SSCR1_LBM)
 
+struct chip_data {
+	u32 cr1;
+	u32 dds_rate;
+	u32 threshold;
+	u16 lpss_rx_threshold;
+	u16 lpss_tx_threshold;
+};
+
 #define LPSS_GENERAL_REG_RXTO_HOLDOFF_DISABLE	BIT(24)
 #define LPSS_CS_CONTROL_SW_MODE			BIT(0)
 #define LPSS_CS_CONTROL_CS_HIGH			BIT(1)
diff --git a/drivers/spi/spi-pxa2xx.h b/drivers/spi/spi-pxa2xx.h
index 5f741bb30240..93e1e471e1c6 100644
--- a/drivers/spi/spi-pxa2xx.h
+++ b/drivers/spi/spi-pxa2xx.h
@@ -75,14 +75,6 @@ struct driver_data {
 	struct gpio_desc *gpiod_ready;
 };
 
-struct chip_data {
-	u32 cr1;
-	u32 dds_rate;
-	u32 threshold;
-	u16 lpss_rx_threshold;
-	u16 lpss_tx_threshold;
-};
-
 static inline u32 pxa2xx_spi_read(const struct driver_data *drv_data, u32 reg)
 {
 	return pxa_ssp_read_reg(drv_data->ssp, reg);
-- 
2.43.0.rc1.1.gbec44491f096

Re: [PATCH v1 0/5] spi: pxa2xx: Cleanup (part 2)

[Mark Brown]

On Wed, 03 Apr 2024 20:06:34 +0300, Andy Shevchenko wrote:
> Here is the additional cleanup of the driver based on the fact of
> the linux/spi/pxa2xx_spi.h being a local (to drivers/spi/) header.
> This means it's based on top of "spi: pxa2xx: Drop linux/spi/pxa2xx_spi.h"
> (20240327193138.2385910-1-andriy.shevchenko@linux.intel.com).
> 
> Andy Shevchenko (5):
>   spi: pxa2xx: Move number of CS pins validation out of condition
>   spi: pxa2xx: Drop struct pxa2xx_spi_chip
>   spi: pxa2xx: Remove DMA parameters from struct chip_data
>   spi: pxa2xx: Remove timeout field from struct chip_data
>   spi: pxa2xx: Don't provide struct chip_data for others
> 
> [...]

Applied to

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

Thanks!

[1/5] spi: pxa2xx: Move number of CS pins validation out of condition
      commit: df3431fd379dcc3b231bd109a55948c27474478d
[2/5] spi: pxa2xx: Drop struct pxa2xx_spi_chip
      (no commit info)
[3/5] spi: pxa2xx: Remove DMA parameters from struct chip_data
      (no commit info)
[4/5] spi: pxa2xx: Remove timeout field from struct chip_data
      (no commit info)
[5/5] spi: pxa2xx: Don't provide struct chip_data for others
      (no commit info)

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