[PATCH v2 0/6] spi: microchip-core: Code improvements

[PATCH v2 0/6] spi: microchip-core: Code improvements

[Andy Shevchenko]

While reading some other stuff, I noticed that this driver may
be improved. Here is the set of refactoring and cleaning it up.

Changelog v2:
- dropped device property agnostic API conversion change (Mark)

Andy Shevchenko (6):
  spi: microchip-core: use min() instead of min_t()
  spi: microchip-core: Refactor FIFO read and write handlers
  spi: microchip-core: Replace dead code (-ENOMEM error message)
  spi: microchip-core: Utilise temporary variable for struct device
  spi: microchip-core: Use SPI_MODE_X_MASK
  spi: microchip-core: Remove unneeded PM related macro

 drivers/spi/spi-microchip-core-spi.c | 96 +++++++++++-----------------
 1 file changed, 38 insertions(+), 58 deletions(-)

-- 
2.50.1

[PATCH v2 1/6] spi: microchip-core: use min() instead of min_t()

[Andy Shevchenko]

min_t(int, a, b) casts an 'unsigned int' to 'int'. This might lead
to the cases when big number is wrongly chosen. On the other hand,
the SPI transfer length is unsigned and driver uses signed type for
an unknown reason. Change the type of the transfer length to be
unsigned and convert use min() instead of min_t().

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/spi/spi-microchip-core-spi.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/spi/spi-microchip-core-spi.c b/drivers/spi/spi-microchip-core-spi.c
index 16e0885474a0..08ccdc5f0cc9 100644
--- a/drivers/spi/spi-microchip-core-spi.c
+++ b/drivers/spi/spi-microchip-core-spi.c
@@ -74,8 +74,8 @@ struct mchp_corespi {
 	u8 *rx_buf;
 	u32 clk_gen;
 	int irq;
-	int tx_len;
-	int rx_len;
+	unsigned int tx_len;
+	unsigned int rx_len;
 	u32 fifo_depth;
 };
 
@@ -214,7 +214,7 @@ static irqreturn_t mchp_corespi_interrupt(int irq, void *dev_id)
 		       spi->regs + MCHP_CORESPI_REG_INTCLEAR);
 		finalise = true;
 		dev_err(&host->dev,
-			"RX OVERFLOW: rxlen: %d, txlen: %d\n",
+			"RX OVERFLOW: rxlen: %u, txlen: %u\n",
 			spi->rx_len, spi->tx_len);
 	}
 
@@ -223,7 +223,7 @@ static irqreturn_t mchp_corespi_interrupt(int irq, void *dev_id)
 		       spi->regs + MCHP_CORESPI_REG_INTCLEAR);
 		finalise = true;
 		dev_err(&host->dev,
-			"TX UNDERFLOW: rxlen: %d, txlen: %d\n",
+			"TX UNDERFLOW: rxlen: %u, txlen: %u\n",
 			spi->rx_len, spi->tx_len);
 	}
 
@@ -283,7 +283,7 @@ static int mchp_corespi_transfer_one(struct spi_controller *host,
 	spi->rx_len = xfer->len;
 
 	while (spi->tx_len) {
-		int fifo_max = min_t(int, spi->tx_len, spi->fifo_depth);
+		unsigned int fifo_max = min(spi->tx_len, spi->fifo_depth);
 
 		mchp_corespi_write_fifo(spi, fifo_max);
 		mchp_corespi_read_fifo(spi, fifo_max);
-- 
2.50.1

[PATCH v2 2/6] spi: microchip-core: Refactor FIFO read and write handlers

[Andy Shevchenko]

Make both handlers to be shorter and easier to understand.
While at it, unify their style.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/spi/spi-microchip-core-spi.c | 32 +++++++++-------------------
 1 file changed, 10 insertions(+), 22 deletions(-)

diff --git a/drivers/spi/spi-microchip-core-spi.c b/drivers/spi/spi-microchip-core-spi.c
index 08ccdc5f0cc9..f8184b711272 100644
--- a/drivers/spi/spi-microchip-core-spi.c
+++ b/drivers/spi/spi-microchip-core-spi.c
@@ -91,21 +91,14 @@ static inline void mchp_corespi_disable(struct mchp_corespi *spi)
 static inline void mchp_corespi_read_fifo(struct mchp_corespi *spi, u32 fifo_max)
 {
 	for (int i = 0; i < fifo_max; i++) {
-		u32 data;
-
 		while (readb(spi->regs + MCHP_CORESPI_REG_STAT) &
 		       MCHP_CORESPI_STATUS_RXFIFO_EMPTY)
 			;
 
-		data = readb(spi->regs + MCHP_CORESPI_REG_RXDATA);
+		if (spi->rx_buf)
+			*spi->rx_buf++ = readb(spi->regs + MCHP_CORESPI_REG_RXDATA);
 
 		spi->rx_len--;
-		if (!spi->rx_buf)
-			continue;
-
-		*spi->rx_buf = data;
-
-		spi->rx_buf++;
 	}
 }
 
@@ -127,23 +120,18 @@ static void mchp_corespi_disable_ints(struct mchp_corespi *spi)
 
 static inline void mchp_corespi_write_fifo(struct mchp_corespi *spi, u32 fifo_max)
 {
-	int i = 0;
-
-	while ((i < fifo_max) &&
-	       !(readb(spi->regs + MCHP_CORESPI_REG_STAT) &
-		 MCHP_CORESPI_STATUS_TXFIFO_FULL)) {
-		u32 word;
-
-		word = spi->tx_buf ? *spi->tx_buf : 0xaa;
-		writeb(word, spi->regs + MCHP_CORESPI_REG_TXDATA);
+	for (int i = 0; i < fifo_max; i++) {
+		if (readb(spi->regs + MCHP_CORESPI_REG_STAT) &
+		    MCHP_CORESPI_STATUS_TXFIFO_FULL)
+			break;
 
 		if (spi->tx_buf)
-			spi->tx_buf++;
+			writeb(*spi->tx_buf++, spi->regs + MCHP_CORESPI_REG_TXDATA);
+		else
+			writeb(0xaa, spi->regs + MCHP_CORESPI_REG_TXDATA);
 
-		i++;
+		spi->tx_len--;
 	}
-
-	spi->tx_len -= i;
 }
 
 static void mchp_corespi_set_cs(struct spi_device *spi, bool disable)
-- 
2.50.1

[PATCH v2 3/6] spi: microchip-core: Replace dead code (-ENOMEM error message)

[Andy Shevchenko]

First of all, the convention in the kernel that we do not issue
error messages for -ENOMEM. Second, it's ignored by dev_err_probe().
Replace dead code by a simple return statement.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/spi/spi-microchip-core-spi.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/spi/spi-microchip-core-spi.c b/drivers/spi/spi-microchip-core-spi.c
index f8184b711272..8ea382c6fee7 100644
--- a/drivers/spi/spi-microchip-core-spi.c
+++ b/drivers/spi/spi-microchip-core-spi.c
@@ -293,8 +293,7 @@ static int mchp_corespi_probe(struct platform_device *pdev)
 
 	host = devm_spi_alloc_host(&pdev->dev, sizeof(*spi));
 	if (!host)
-		return dev_err_probe(&pdev->dev, -ENOMEM,
-				     "unable to allocate host for SPI controller\n");
+		return -ENOMEM;
 
 	platform_set_drvdata(pdev, host);
 
-- 
2.50.1

[PATCH v2 4/6] spi: microchip-core: Utilise temporary variable for struct device

[Andy Shevchenko]

Add a temporary variable to keep a pointer to struct device.
Utilise it where it makes sense.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/spi/spi-microchip-core-spi.c | 44 +++++++++++++---------------
 1 file changed, 21 insertions(+), 23 deletions(-)

diff --git a/drivers/spi/spi-microchip-core-spi.c b/drivers/spi/spi-microchip-core-spi.c
index 8ea382c6fee7..0dca46dcdc2f 100644
--- a/drivers/spi/spi-microchip-core-spi.c
+++ b/drivers/spi/spi-microchip-core-spi.c
@@ -284,6 +284,7 @@ static int mchp_corespi_transfer_one(struct spi_controller *host,
 static int mchp_corespi_probe(struct platform_device *pdev)
 {
 	const char *protocol = "motorola";
+	struct device *dev = &pdev->dev;
 	struct spi_controller *host;
 	struct mchp_corespi *spi;
 	struct resource *res;
@@ -291,13 +292,13 @@ static int mchp_corespi_probe(struct platform_device *pdev)
 	bool assert_ssel;
 	int ret = 0;
 
-	host = devm_spi_alloc_host(&pdev->dev, sizeof(*spi));
+	host = devm_spi_alloc_host(dev, sizeof(*spi));
 	if (!host)
 		return -ENOMEM;
 
 	platform_set_drvdata(pdev, host);
 
-	if (of_property_read_u32(pdev->dev.of_node, "num-cs", &num_cs))
+	if (of_property_read_u32(dev->of_node, "num-cs", &num_cs))
 		num_cs = MCHP_CORESPI_MAX_CS;
 
 	/*
@@ -305,12 +306,12 @@ static int mchp_corespi_probe(struct platform_device *pdev)
 	 * CoreSPI can be configured for Motorola, TI or NSC.
 	 * The current driver supports only Motorola mode.
 	 */
-	ret = of_property_read_string(pdev->dev.of_node, "microchip,protocol-configuration",
+	ret = of_property_read_string(dev->of_node, "microchip,protocol-configuration",
 				      &protocol);
 	if (ret && ret != -EINVAL)
-		return dev_err_probe(&pdev->dev, ret, "Error reading protocol-configuration\n");
+		return dev_err_probe(dev, ret, "Error reading protocol-configuration\n");
 	if (strcmp(protocol, "motorola") != 0)
-		return dev_err_probe(&pdev->dev, -EINVAL,
+		return dev_err_probe(dev, -EINVAL,
 				     "CoreSPI: protocol '%s' not supported by this driver\n",
 				      protocol);
 
@@ -318,11 +319,11 @@ static int mchp_corespi_probe(struct platform_device *pdev)
 	 * Motorola mode (0-3): CFG_MOT_MODE
 	 * Mode is fixed in the IP configurator.
 	 */
-	ret = of_property_read_u32(pdev->dev.of_node, "microchip,motorola-mode", &mode);
+	ret = of_property_read_u32(dev->of_node, "microchip,motorola-mode", &mode);
 	if (ret)
 		mode = MCHP_CORESPI_DEFAULT_MOTOROLA_MODE;
 	else if (mode > 3)
-		return dev_err_probe(&pdev->dev, -EINVAL,
+		return dev_err_probe(dev, -EINVAL,
 				     "invalid 'microchip,motorola-mode' value %u\n", mode);
 
 	/*
@@ -330,9 +331,9 @@ static int mchp_corespi_probe(struct platform_device *pdev)
 	 * The hardware allows frame sizes <= APB data width.
 	 * However, this driver currently only supports 8-bit frames.
 	 */
-	ret = of_property_read_u32(pdev->dev.of_node, "microchip,frame-size", &frame_size);
+	ret = of_property_read_u32(dev->of_node, "microchip,frame-size", &frame_size);
 	if (!ret && frame_size != 8)
-		return dev_err_probe(&pdev->dev, -EINVAL,
+		return dev_err_probe(dev, -EINVAL,
 				     "CoreSPI: frame size %u not supported by this driver\n",
 				     frame_size);
 
@@ -342,9 +343,9 @@ static int mchp_corespi_probe(struct platform_device *pdev)
 	 * To prevent CS deassertion when TX FIFO drains, the ssel-active property
 	 * keeps CS asserted for the full SPI transfer.
 	 */
-	assert_ssel = of_property_read_bool(pdev->dev.of_node, "microchip,ssel-active");
+	assert_ssel = of_property_read_bool(dev->of_node, "microchip,ssel-active");
 	if (!assert_ssel)
-		return dev_err_probe(&pdev->dev, -EINVAL,
+		return dev_err_probe(dev, -EINVAL,
 				     "hardware must enable 'microchip,ssel-active' to keep CS asserted for the SPI transfer\n");
 
 	spi = spi_controller_get_devdata(host);
@@ -356,9 +357,9 @@ static int mchp_corespi_probe(struct platform_device *pdev)
 	host->bits_per_word_mask = SPI_BPW_RANGE_MASK(4, 32);
 	host->transfer_one = mchp_corespi_transfer_one;
 	host->set_cs = mchp_corespi_set_cs;
-	host->dev.of_node = pdev->dev.of_node;
+	host->dev.of_node = dev->of_node;
 
-	ret = of_property_read_u32(pdev->dev.of_node, "fifo-depth", &spi->fifo_depth);
+	ret = of_property_read_u32(dev->of_node, "fifo-depth", &spi->fifo_depth);
 	if (ret)
 		spi->fifo_depth = MCHP_CORESPI_DEFAULT_FIFO_DEPTH;
 
@@ -370,24 +371,21 @@ static int mchp_corespi_probe(struct platform_device *pdev)
 	if (spi->irq < 0)
 		return spi->irq;
 
-	ret = devm_request_irq(&pdev->dev, spi->irq, mchp_corespi_interrupt,
-			       IRQF_SHARED, dev_name(&pdev->dev), host);
+	ret = devm_request_irq(dev, spi->irq, mchp_corespi_interrupt, IRQF_SHARED,
+			       dev_name(dev), host);
 	if (ret)
-		return dev_err_probe(&pdev->dev, ret,
-				     "could not request irq\n");
+		return dev_err_probe(dev, ret, "could not request irq\n");
 
-	spi->clk = devm_clk_get_enabled(&pdev->dev, NULL);
+	spi->clk = devm_clk_get_enabled(dev, NULL);
 	if (IS_ERR(spi->clk))
-		return dev_err_probe(&pdev->dev, PTR_ERR(spi->clk),
-				     "could not get clk\n");
+		return dev_err_probe(dev, PTR_ERR(spi->clk), "could not get clk\n");
 
 	mchp_corespi_init(host, spi);
 
-	ret = devm_spi_register_controller(&pdev->dev, host);
+	ret = devm_spi_register_controller(dev, host);
 	if (ret) {
 		mchp_corespi_disable(spi);
-		return dev_err_probe(&pdev->dev, ret,
-				     "unable to register host for CoreSPI controller\n");
+		return dev_err_probe(dev, ret, "unable to register host for CoreSPI controller\n");
 	}
 
 	return 0;
-- 
2.50.1

[PATCH v2 5/6] spi: microchip-core: Use SPI_MODE_X_MASK

[Andy Shevchenko]

Use SPI_MODE_X_MASK instead of open coded variant.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/spi/spi-microchip-core-spi.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/spi/spi-microchip-core-spi.c b/drivers/spi/spi-microchip-core-spi.c
index 0dca46dcdc2f..941b7e23eac3 100644
--- a/drivers/spi/spi-microchip-core-spi.c
+++ b/drivers/spi/spi-microchip-core-spi.c
@@ -148,8 +148,6 @@ static void mchp_corespi_set_cs(struct spi_device *spi, bool disable)
 
 static int mchp_corespi_setup(struct spi_device *spi)
 {
-	u32 dev_mode = spi->mode & (SPI_CPOL | SPI_CPHA);
-
 	if (spi_get_csgpiod(spi, 0))
 		return 0;
 
@@ -158,7 +156,7 @@ static int mchp_corespi_setup(struct spi_device *spi)
 		return -EOPNOTSUPP;
 	}
 
-	if (dev_mode & ~spi->controller->mode_bits) {
+	if (spi->mode & SPI_MODE_X_MASK & ~spi->controller->mode_bits) {
 		dev_err(&spi->dev, "incompatible CPOL/CPHA, must match controller's Motorola mode\n");
 		return -EINVAL;
 	}
-- 
2.50.1

[PATCH v2 6/6] spi: microchip-core: Remove unneeded PM related macro

[Andy Shevchenko]

Static declaration by default are 0 or NULL, no need to initialise
them explicitly. Remove unneeded PM related macro.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/spi/spi-microchip-core-spi.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/spi/spi-microchip-core-spi.c b/drivers/spi/spi-microchip-core-spi.c
index 941b7e23eac3..1e62af20d6f2 100644
--- a/drivers/spi/spi-microchip-core-spi.c
+++ b/drivers/spi/spi-microchip-core-spi.c
@@ -398,8 +398,6 @@ static void mchp_corespi_remove(struct platform_device *pdev)
 	mchp_corespi_disable(spi);
 }
 
-#define MICROCHIP_SPI_PM_OPS (NULL)
-
 /*
  * Platform driver data structure
  */
@@ -416,7 +414,6 @@ static struct platform_driver mchp_corespi_driver = {
 	.probe = mchp_corespi_probe,
 	.driver = {
 		.name = "microchip-corespi",
-		.pm = MICROCHIP_SPI_PM_OPS,
 		.of_match_table = of_match_ptr(mchp_corespi_dt_ids),
 	},
 	.remove = mchp_corespi_remove,
-- 
2.50.1

Re: [PATCH v2 2/6] spi: microchip-core: Refactor FIFO read and write handlers

[david laight]

On Wed, 26 Nov 2025 08:54:40 +0100
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> Make both handlers to be shorter and easier to understand.
> While at it, unify their style.

The read code looks dubious... (nothing new though)

> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/spi/spi-microchip-core-spi.c | 32 +++++++++-------------------
>  1 file changed, 10 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/spi/spi-microchip-core-spi.c b/drivers/spi/spi-microchip-core-spi.c
> index 08ccdc5f0cc9..f8184b711272 100644
> --- a/drivers/spi/spi-microchip-core-spi.c
> +++ b/drivers/spi/spi-microchip-core-spi.c
> @@ -91,21 +91,14 @@ static inline void mchp_corespi_disable(struct mchp_corespi *spi)
>  static inline void mchp_corespi_read_fifo(struct mchp_corespi *spi, u32 fifo_max)
>  {
>  	for (int i = 0; i < fifo_max; i++) {
> -		u32 data;
> -
>  		while (readb(spi->regs + MCHP_CORESPI_REG_STAT) &
>  		       MCHP_CORESPI_STATUS_RXFIFO_EMPTY)
>  			;

This is a hard spin until data is available.
I think 'spi' is a bit like 'i2c' (etc) so is quite slow.
Even if the code thinks there are 'fifo_max' bytes in the fifo it seems
wrong to spin indefinitely.
If the code is trying to read a response that is still arriving from the
physical hardware is is positively wrong.
If some hardware has a glitch that FIFO_EMPTY is temporarily incorrectly set
after a read - then maybe you need some recovery code.
Otherwise I suspect FIFO_EMPTY should generate a short read.

The write code (below) does an 'early terminate'' on fifo full.
Presumably it is woken by an interrupt to continue the write?

I actually doubt that transferring messages that are larger than the
device fifo is ever going to be completely reliable.
You'd need to guarantee the interrupt latency to update the fifo be short
enough to guarantee the fifo won't underflow/overflow.
(Unless the spi hardware 'clock stops' the physical interface when the fifo
if full/empty - which is effectively what happens when software 'bit-bangs'
these serial interfaces.)

>  
> -		data = readb(spi->regs + MCHP_CORESPI_REG_RXDATA);
> +		if (spi->rx_buf)
> +			*spi->rx_buf++ = readb(spi->regs + MCHP_CORESPI_REG_RXDATA);
>  
>  		spi->rx_len--;
> -		if (!spi->rx_buf)
> -			continue;
> -
> -		*spi->rx_buf = data;
> -
> -		spi->rx_buf++;
>  	}
>  }
>  
> @@ -127,23 +120,18 @@ static void mchp_corespi_disable_ints(struct mchp_corespi *spi)
>  
>  static inline void mchp_corespi_write_fifo(struct mchp_corespi *spi, u32 fifo_max)
>  {
> -	int i = 0;
> -
> -	while ((i < fifo_max) &&
> -	       !(readb(spi->regs + MCHP_CORESPI_REG_STAT) &
> -		 MCHP_CORESPI_STATUS_TXFIFO_FULL)) {
> -		u32 word;
> -
> -		word = spi->tx_buf ? *spi->tx_buf : 0xaa;
> -		writeb(word, spi->regs + MCHP_CORESPI_REG_TXDATA);
> +	for (int i = 0; i < fifo_max; i++) {

unsigned int or u32 ??

> +		if (readb(spi->regs + MCHP_CORESPI_REG_STAT) &
> +		    MCHP_CORESPI_STATUS_TXFIFO_FULL)
> +			break;
>  
>  		if (spi->tx_buf)
> -			spi->tx_buf++;
> +			writeb(*spi->tx_buf++, spi->regs + MCHP_CORESPI_REG_TXDATA);
> +		elsespi->regs + MCHP_CORESPI_REG_TXDATA);
> +			writeb(0xaa, spi->regs + MCHP_CORESPI_REG_TXDATA);

I'm not sure I don't prefer the version with one writeb() call.
How about:
		writeb(spi->tx_buf ? *spi->tx_buf++ : 0xaa,
			spi->regs + MCHP_CORESPI_REG_TXDATA);

	David
			
>  
> -		i++;
> +		spi->tx_len--;
>  	}
> -
> -	spi->tx_len -= i;
>  }
>  
>  static void mchp_corespi_set_cs(struct spi_device *spi, bool disable)

Re: [PATCH v2 1/6] spi: microchip-core: use min() instead of min_t()

[david laight]

On Wed, 26 Nov 2025 08:54:39 +0100
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> min_t(int, a, b) casts an 'unsigned int' to 'int'. This might lead
> to the cases when big number is wrongly chosen. On the other hand,
> the SPI transfer length is unsigned and driver uses signed type for
> an unknown reason. Change the type of the transfer length to be
> unsigned and convert use min() instead of min_t().
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Reviewed-by: David Laight <david.laight.linux@gmail.com>

> ---
>  drivers/spi/spi-microchip-core-spi.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/spi/spi-microchip-core-spi.c b/drivers/spi/spi-microchip-core-spi.c
> index 16e0885474a0..08ccdc5f0cc9 100644
> --- a/drivers/spi/spi-microchip-core-spi.c
> +++ b/drivers/spi/spi-microchip-core-spi.c
> @@ -74,8 +74,8 @@ struct mchp_corespi {
>  	u8 *rx_buf;
>  	u32 clk_gen;
>  	int irq;
> -	int tx_len;
> -	int rx_len;
> +	unsigned int tx_len;
> +	unsigned int rx_len;
>  	u32 fifo_depth;
>  };
>  
> @@ -214,7 +214,7 @@ static irqreturn_t mchp_corespi_interrupt(int irq, void *dev_id)
>  		       spi->regs + MCHP_CORESPI_REG_INTCLEAR);
>  		finalise = true;
>  		dev_err(&host->dev,
> -			"RX OVERFLOW: rxlen: %d, txlen: %d\n",
> +			"RX OVERFLOW: rxlen: %u, txlen: %u\n",
>  			spi->rx_len, spi->tx_len);
>  	}
>  
> @@ -223,7 +223,7 @@ static irqreturn_t mchp_corespi_interrupt(int irq, void *dev_id)
>  		       spi->regs + MCHP_CORESPI_REG_INTCLEAR);
>  		finalise = true;
>  		dev_err(&host->dev,
> -			"TX UNDERFLOW: rxlen: %d, txlen: %d\n",
> +			"TX UNDERFLOW: rxlen: %u, txlen: %u\n",
>  			spi->rx_len, spi->tx_len);
>  	}
>  
> @@ -283,7 +283,7 @@ static int mchp_corespi_transfer_one(struct spi_controller *host,
>  	spi->rx_len = xfer->len;
>  
>  	while (spi->tx_len) {
> -		int fifo_max = min_t(int, spi->tx_len, spi->fifo_depth);
> +		unsigned int fifo_max = min(spi->tx_len, spi->fifo_depth);
>  
>  		mchp_corespi_write_fifo(spi, fifo_max);
>  		mchp_corespi_read_fifo(spi, fifo_max);

Re: [PATCH v2 2/6] spi: microchip-core: Refactor FIFO read and write handlers

[Andy Shevchenko]

On Wed, Nov 26, 2025 at 09:21:45AM +0000, david laight wrote:
> On Wed, 26 Nov 2025 08:54:40 +0100
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> 
> > Make both handlers to be shorter and easier to understand.
> > While at it, unify their style.

...

> >  	for (int i = 0; i < fifo_max; i++) {

^^^	(1)

...

> >  		while (readb(spi->regs + MCHP_CORESPI_REG_STAT) &
> >  		       MCHP_CORESPI_STATUS_RXFIFO_EMPTY)
> >  			;
> 
> This is a hard spin until data is available.
> I think 'spi' is a bit like 'i2c' (etc) so is quite slow.
> Even if the code thinks there are 'fifo_max' bytes in the fifo it seems
> wrong to spin indefinitely.
> If the code is trying to read a response that is still arriving from the
> physical hardware is is positively wrong.
> If some hardware has a glitch that FIFO_EMPTY is temporarily incorrectly set
> after a read - then maybe you need some recovery code.
> Otherwise I suspect FIFO_EMPTY should generate a short read.
> 
> The write code (below) does an 'early terminate'' on fifo full.
> Presumably it is woken by an interrupt to continue the write?
> 
> I actually doubt that transferring messages that are larger than the
> device fifo is ever going to be completely reliable.
> You'd need to guarantee the interrupt latency to update the fifo be short
> enough to guarantee the fifo won't underflow/overflow.
> (Unless the spi hardware 'clock stops' the physical interface when the fifo
> if full/empty - which is effectively what happens when software 'bit-bangs'
> these serial interfaces.)

I also saw that code and it needs to be amended, but it's out of the scope of
this mini-series.

...

> > +	for (int i = 0; i < fifo_max; i++) {
> 
> unsigned int or u32 ??

int works as well and I won't to touch (1) above, less churn.

...

> > +			writeb(*spi->tx_buf++, spi->regs + MCHP_CORESPI_REG_TXDATA);
> > +		elsespi->regs + MCHP_CORESPI_REG_TXDATA);
> > +			writeb(0xaa, spi->regs + MCHP_CORESPI_REG_TXDATA);
> 
> I'm not sure I don't prefer the version with one writeb() call.
> How about:
> 		writeb(spi->tx_buf ? *spi->tx_buf++ : 0xaa,
> 			spi->regs + MCHP_CORESPI_REG_TXDATA);

I find ternary here is unreadable as regular if. With regular if we also see
the exact difference at a glance. I definitely prefer my variant.

...

Thanks for the review.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/6] spi: microchip-core: Refactor FIFO read and write handlers

[Mark Brown]

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

On Wed, Nov 26, 2025 at 09:21:45AM +0000, david laight wrote:

> I'm not sure I don't prefer the version with one writeb() call.
> How about:
> 		writeb(spi->tx_buf ? *spi->tx_buf++ : 0xaa,
> 			spi->regs + MCHP_CORESPI_REG_TXDATA);

Please don't abuse the ternery operator like this, just write normal
conditional statements.

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

Re: [PATCH v2 2/6] spi: microchip-core: Refactor FIFO read and write handlers

[Andy Shevchenko]

On Wed, Nov 26, 2025 at 12:05:22PM +0000, Mark Brown wrote:
> On Wed, Nov 26, 2025 at 09:21:45AM +0000, david laight wrote:

...

> > I'm not sure I don't prefer the version with one writeb() call.
> > How about:
> > 		writeb(spi->tx_buf ? *spi->tx_buf++ : 0xaa,
> > 			spi->regs + MCHP_CORESPI_REG_TXDATA);
> 
> Please don't abuse the ternery operator like this, just write normal
> conditional statements.

FWIW, that's what my patch does already :-)

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/6] spi: microchip-core: use min() instead of min_t()

[Prajna Rajendra Kumar]

On 26/11/2025 09:22, david laight wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> On Wed, 26 Nov 2025 08:54:39 +0100
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
>
>> min_t(int, a, b) casts an 'unsigned int' to 'int'. This might lead
>> to the cases when big number is wrongly chosen. On the other hand,
>> the SPI transfer length is unsigned and driver uses signed type for
>> an unknown reason. Change the type of the transfer length to be
>> unsigned and convert use min() instead of min_t().
>>
>> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Reviewed-by: David Laight <david.laight.linux@gmail.com>
Reviewed-by: Prajna Rajendra Kumar <prajna.rajendrakumar@microchip.com>
>
>> ---
>>   drivers/spi/spi-microchip-core-spi.c | 10 +++++-----
>>   1 file changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/spi/spi-microchip-core-spi.c b/drivers/spi/spi-microchip-core-spi.c
>> index 16e0885474a0..08ccdc5f0cc9 100644
>> --- a/drivers/spi/spi-microchip-core-spi.c
>> +++ b/drivers/spi/spi-microchip-core-spi.c
>> @@ -74,8 +74,8 @@ struct mchp_corespi {
>>        u8 *rx_buf;
>>        u32 clk_gen;
>>        int irq;
>> -     int tx_len;
>> -     int rx_len;
>> +     unsigned int tx_len;
>> +     unsigned int rx_len;
>>        u32 fifo_depth;
>>   };
>>
>> @@ -214,7 +214,7 @@ static irqreturn_t mchp_corespi_interrupt(int irq, void *dev_id)
>>                       spi->regs + MCHP_CORESPI_REG_INTCLEAR);
>>                finalise = true;
>>                dev_err(&host->dev,
>> -                     "RX OVERFLOW: rxlen: %d, txlen: %d\n",
>> +                     "RX OVERFLOW: rxlen: %u, txlen: %u\n",
>>                        spi->rx_len, spi->tx_len);
>>        }
>>
>> @@ -223,7 +223,7 @@ static irqreturn_t mchp_corespi_interrupt(int irq, void *dev_id)
>>                       spi->regs + MCHP_CORESPI_REG_INTCLEAR);
>>                finalise = true;
>>                dev_err(&host->dev,
>> -                     "TX UNDERFLOW: rxlen: %d, txlen: %d\n",
>> +                     "TX UNDERFLOW: rxlen: %u, txlen: %u\n",
>>                        spi->rx_len, spi->tx_len);
>>        }
>>
>> @@ -283,7 +283,7 @@ static int mchp_corespi_transfer_one(struct spi_controller *host,
>>        spi->rx_len = xfer->len;
>>
>>        while (spi->tx_len) {
>> -             int fifo_max = min_t(int, spi->tx_len, spi->fifo_depth);
>> +             unsigned int fifo_max = min(spi->tx_len, spi->fifo_depth);
>>
>>                mchp_corespi_write_fifo(spi, fifo_max);
>>                mchp_corespi_read_fifo(spi, fifo_max);

Re: [PATCH v2 3/6] spi: microchip-core: Replace dead code (-ENOMEM error message)

[Prajna Rajendra Kumar]

On 26/11/2025 07:54, Andy Shevchenko wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> First of all, the convention in the kernel that we do not issue
> error messages for -ENOMEM. Second, it's ignored by dev_err_probe().
> Replace dead code by a simple return statement.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Reviewed-by: Prajna Rajendra Kumar <prajna.rajendrakumar@microchip.com>
> ---
>   drivers/spi/spi-microchip-core-spi.c | 3 +--
>   1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/spi/spi-microchip-core-spi.c b/drivers/spi/spi-microchip-core-spi.c
> index f8184b711272..8ea382c6fee7 100644
> --- a/drivers/spi/spi-microchip-core-spi.c
> +++ b/drivers/spi/spi-microchip-core-spi.c
> @@ -293,8 +293,7 @@ static int mchp_corespi_probe(struct platform_device *pdev)
>
>          host = devm_spi_alloc_host(&pdev->dev, sizeof(*spi));
>          if (!host)
> -               return dev_err_probe(&pdev->dev, -ENOMEM,
> -                                    "unable to allocate host for SPI controller\n");
> +               return -ENOMEM;
>
>          platform_set_drvdata(pdev, host);
>
> --
> 2.50.1
>

Re: [PATCH v2 4/6] spi: microchip-core: Utilise temporary variable for struct device

[Prajna Rajendra Kumar]

On 26/11/2025 07:54, Andy Shevchenko wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> Add a temporary variable to keep a pointer to struct device.
> Utilise it where it makes sense.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Reviewed-by: Prajna Rajendra Kumar <prajna.rajendrakumar@microchip.com>
> ---
>   drivers/spi/spi-microchip-core-spi.c | 44 +++++++++++++---------------
>   1 file changed, 21 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/spi/spi-microchip-core-spi.c b/drivers/spi/spi-microchip-core-spi.c
> index 8ea382c6fee7..0dca46dcdc2f 100644
> --- a/drivers/spi/spi-microchip-core-spi.c
> +++ b/drivers/spi/spi-microchip-core-spi.c
> @@ -284,6 +284,7 @@ static int mchp_corespi_transfer_one(struct spi_controller *host,
>   static int mchp_corespi_probe(struct platform_device *pdev)
>   {
>          const char *protocol = "motorola";
> +       struct device *dev = &pdev->dev;
>          struct spi_controller *host;
>          struct mchp_corespi *spi;
>          struct resource *res;
> @@ -291,13 +292,13 @@ static int mchp_corespi_probe(struct platform_device *pdev)
>          bool assert_ssel;
>          int ret = 0;
>
> -       host = devm_spi_alloc_host(&pdev->dev, sizeof(*spi));
> +       host = devm_spi_alloc_host(dev, sizeof(*spi));
>          if (!host)
>                  return -ENOMEM;
>
>          platform_set_drvdata(pdev, host);
>
> -       if (of_property_read_u32(pdev->dev.of_node, "num-cs", &num_cs))
> +       if (of_property_read_u32(dev->of_node, "num-cs", &num_cs))
>                  num_cs = MCHP_CORESPI_MAX_CS;
>
>          /*
> @@ -305,12 +306,12 @@ static int mchp_corespi_probe(struct platform_device *pdev)
>           * CoreSPI can be configured for Motorola, TI or NSC.
>           * The current driver supports only Motorola mode.
>           */
> -       ret = of_property_read_string(pdev->dev.of_node, "microchip,protocol-configuration",
> +       ret = of_property_read_string(dev->of_node, "microchip,protocol-configuration",
>                                        &protocol);
>          if (ret && ret != -EINVAL)
> -               return dev_err_probe(&pdev->dev, ret, "Error reading protocol-configuration\n");
> +               return dev_err_probe(dev, ret, "Error reading protocol-configuration\n");
>          if (strcmp(protocol, "motorola") != 0)
> -               return dev_err_probe(&pdev->dev, -EINVAL,
> +               return dev_err_probe(dev, -EINVAL,
>                                       "CoreSPI: protocol '%s' not supported by this driver\n",
>                                        protocol);
>
> @@ -318,11 +319,11 @@ static int mchp_corespi_probe(struct platform_device *pdev)
>           * Motorola mode (0-3): CFG_MOT_MODE
>           * Mode is fixed in the IP configurator.
>           */
> -       ret = of_property_read_u32(pdev->dev.of_node, "microchip,motorola-mode", &mode);
> +       ret = of_property_read_u32(dev->of_node, "microchip,motorola-mode", &mode);
>          if (ret)
>                  mode = MCHP_CORESPI_DEFAULT_MOTOROLA_MODE;
>          else if (mode > 3)
> -               return dev_err_probe(&pdev->dev, -EINVAL,
> +               return dev_err_probe(dev, -EINVAL,
>                                       "invalid 'microchip,motorola-mode' value %u\n", mode);
>
>          /*
> @@ -330,9 +331,9 @@ static int mchp_corespi_probe(struct platform_device *pdev)
>           * The hardware allows frame sizes <= APB data width.
>           * However, this driver currently only supports 8-bit frames.
>           */
> -       ret = of_property_read_u32(pdev->dev.of_node, "microchip,frame-size", &frame_size);
> +       ret = of_property_read_u32(dev->of_node, "microchip,frame-size", &frame_size);
>          if (!ret && frame_size != 8)
> -               return dev_err_probe(&pdev->dev, -EINVAL,
> +               return dev_err_probe(dev, -EINVAL,
>                                       "CoreSPI: frame size %u not supported by this driver\n",
>                                       frame_size);
>
> @@ -342,9 +343,9 @@ static int mchp_corespi_probe(struct platform_device *pdev)
>           * To prevent CS deassertion when TX FIFO drains, the ssel-active property
>           * keeps CS asserted for the full SPI transfer.
>           */
> -       assert_ssel = of_property_read_bool(pdev->dev.of_node, "microchip,ssel-active");
> +       assert_ssel = of_property_read_bool(dev->of_node, "microchip,ssel-active");
>          if (!assert_ssel)
> -               return dev_err_probe(&pdev->dev, -EINVAL,
> +               return dev_err_probe(dev, -EINVAL,
>                                       "hardware must enable 'microchip,ssel-active' to keep CS asserted for the SPI transfer\n");
>
>          spi = spi_controller_get_devdata(host);
> @@ -356,9 +357,9 @@ static int mchp_corespi_probe(struct platform_device *pdev)
>          host->bits_per_word_mask = SPI_BPW_RANGE_MASK(4, 32);
>          host->transfer_one = mchp_corespi_transfer_one;
>          host->set_cs = mchp_corespi_set_cs;
> -       host->dev.of_node = pdev->dev.of_node;
> +       host->dev.of_node = dev->of_node;
>
> -       ret = of_property_read_u32(pdev->dev.of_node, "fifo-depth", &spi->fifo_depth);
> +       ret = of_property_read_u32(dev->of_node, "fifo-depth", &spi->fifo_depth);
>          if (ret)
>                  spi->fifo_depth = MCHP_CORESPI_DEFAULT_FIFO_DEPTH;
>
> @@ -370,24 +371,21 @@ static int mchp_corespi_probe(struct platform_device *pdev)
>          if (spi->irq < 0)
>                  return spi->irq;
>
> -       ret = devm_request_irq(&pdev->dev, spi->irq, mchp_corespi_interrupt,
> -                              IRQF_SHARED, dev_name(&pdev->dev), host);
> +       ret = devm_request_irq(dev, spi->irq, mchp_corespi_interrupt, IRQF_SHARED,
> +                              dev_name(dev), host);
>          if (ret)
> -               return dev_err_probe(&pdev->dev, ret,
> -                                    "could not request irq\n");
> +               return dev_err_probe(dev, ret, "could not request irq\n");
>
> -       spi->clk = devm_clk_get_enabled(&pdev->dev, NULL);
> +       spi->clk = devm_clk_get_enabled(dev, NULL);
>          if (IS_ERR(spi->clk))
> -               return dev_err_probe(&pdev->dev, PTR_ERR(spi->clk),
> -                                    "could not get clk\n");
> +               return dev_err_probe(dev, PTR_ERR(spi->clk), "could not get clk\n");
>
>          mchp_corespi_init(host, spi);
>
> -       ret = devm_spi_register_controller(&pdev->dev, host);
> +       ret = devm_spi_register_controller(dev, host);
>          if (ret) {
>                  mchp_corespi_disable(spi);
> -               return dev_err_probe(&pdev->dev, ret,
> -                                    "unable to register host for CoreSPI controller\n");
> +               return dev_err_probe(dev, ret, "unable to register host for CoreSPI controller\n");
>          }
>
>          return 0;
> --
> 2.50.1
>

Re: [PATCH v2 5/6] spi: microchip-core: Use SPI_MODE_X_MASK

[Prajna Rajendra Kumar]

On 26/11/2025 07:54, Andy Shevchenko wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> Use SPI_MODE_X_MASK instead of open coded variant.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Reviewed-by: Prajna Rajendra Kumar <prajna.rajendrakumar@microchip.com>
> ---
>   drivers/spi/spi-microchip-core-spi.c | 4 +---
>   1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/drivers/spi/spi-microchip-core-spi.c b/drivers/spi/spi-microchip-core-spi.c
> index 0dca46dcdc2f..941b7e23eac3 100644
> --- a/drivers/spi/spi-microchip-core-spi.c
> +++ b/drivers/spi/spi-microchip-core-spi.c
> @@ -148,8 +148,6 @@ static void mchp_corespi_set_cs(struct spi_device *spi, bool disable)
>
>   static int mchp_corespi_setup(struct spi_device *spi)
>   {
> -       u32 dev_mode = spi->mode & (SPI_CPOL | SPI_CPHA);
> -
>          if (spi_get_csgpiod(spi, 0))
>                  return 0;
>
> @@ -158,7 +156,7 @@ static int mchp_corespi_setup(struct spi_device *spi)
>                  return -EOPNOTSUPP;
>          }
>
> -       if (dev_mode & ~spi->controller->mode_bits) {
> +       if (spi->mode & SPI_MODE_X_MASK & ~spi->controller->mode_bits) {
>                  dev_err(&spi->dev, "incompatible CPOL/CPHA, must match controller's Motorola mode\n");
>                  return -EINVAL;
>          }
> --
> 2.50.1
>

Re: [PATCH v2 6/6] spi: microchip-core: Remove unneeded PM related macro

[Prajna Rajendra Kumar]

On 26/11/2025 07:54, Andy Shevchenko wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> Static declaration by default are 0 or NULL, no need to initialise
> them explicitly. Remove unneeded PM related macro.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Reviewed-by: Prajna Rajendra Kumar <prajna.rajendrakumar@microchip.com>
> ---
>   drivers/spi/spi-microchip-core-spi.c | 3 ---
>   1 file changed, 3 deletions(-)
>
> diff --git a/drivers/spi/spi-microchip-core-spi.c b/drivers/spi/spi-microchip-core-spi.c
> index 941b7e23eac3..1e62af20d6f2 100644
> --- a/drivers/spi/spi-microchip-core-spi.c
> +++ b/drivers/spi/spi-microchip-core-spi.c
> @@ -398,8 +398,6 @@ static void mchp_corespi_remove(struct platform_device *pdev)
>          mchp_corespi_disable(spi);
>   }
>
> -#define MICROCHIP_SPI_PM_OPS (NULL)
> -
>   /*
>    * Platform driver data structure
>    */
> @@ -416,7 +414,6 @@ static struct platform_driver mchp_corespi_driver = {
>          .probe = mchp_corespi_probe,
>          .driver = {
>                  .name = "microchip-corespi",
> -               .pm = MICROCHIP_SPI_PM_OPS,
>                  .of_match_table = of_match_ptr(mchp_corespi_dt_ids),
>          },
>          .remove = mchp_corespi_remove,
> --
> 2.50.1
>

Re: [PATCH v2 2/6] spi: microchip-core: Refactor FIFO read and write handlers

[Prajna Rajendra Kumar]

On 26/11/2025 12:13, Andy Shevchenko wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> On Wed, Nov 26, 2025 at 12:05:22PM +0000, Mark Brown wrote:
>> On Wed, Nov 26, 2025 at 09:21:45AM +0000, david laight wrote:
> ...
>
>>> I'm not sure I don't prefer the version with one writeb() call.
>>> How about:
>>>              writeb(spi->tx_buf ? *spi->tx_buf++ : 0xaa,
>>>                      spi->regs + MCHP_CORESPI_REG_TXDATA);
>> Please don't abuse the ternery operator like this, just write normal
>> conditional statements.
> FWIW, that's what my patch does already :-)
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
Hi Andy,

Thanks for the series. However, this particular patch appears to
introduce a regression. The SPI controller reads an incorrect
Device ID from the peripheral.

I’m investigating the root cause and will follow up.

Best regards,
Prajna

Re: [PATCH v2 2/6] spi: microchip-core: Refactor FIFO read and write handlers

[Andy Shevchenko]

On Thu, Nov 27, 2025 at 04:08:25PM +0000, Prajna Rajendra Kumar wrote:
> On 26/11/2025 12:13, Andy Shevchenko wrote:
> > On Wed, Nov 26, 2025 at 12:05:22PM +0000, Mark Brown wrote:
> > > On Wed, Nov 26, 2025 at 09:21:45AM +0000, david laight wrote:

...

> > > > I'm not sure I don't prefer the version with one writeb() call.
> > > > How about:
> > > >              writeb(spi->tx_buf ? *spi->tx_buf++ : 0xaa,
> > > >                      spi->regs + MCHP_CORESPI_REG_TXDATA);
> > > Please don't abuse the ternery operator like this, just write normal
> > > conditional statements.
> > FWIW, that's what my patch does already :-)
> 
> Thanks for the series. However, this particular patch appears to
> introduce a regression. The SPI controller reads an incorrect
> Device ID from the peripheral.

Hmm... This is interesting. The only thing I see is missed dummy byte read in
case of TX only transfers. Is this what you have?

> I’m investigating the root cause and will follow up.

Okay, I will be glad to know the cause and help to fix that.


Thank you for the review!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/6] spi: microchip-core: Refactor FIFO read and write handlers

[Andy Shevchenko]

On Thu, Nov 27, 2025 at 06:49:31PM +0200, Andy Shevchenko wrote:
> On Thu, Nov 27, 2025 at 04:08:25PM +0000, Prajna Rajendra Kumar wrote:
> > On 26/11/2025 12:13, Andy Shevchenko wrote:
> > > On Wed, Nov 26, 2025 at 12:05:22PM +0000, Mark Brown wrote:
> > > > On Wed, Nov 26, 2025 at 09:21:45AM +0000, david laight wrote:

...

> > > > > I'm not sure I don't prefer the version with one writeb() call.
> > > > > How about:
> > > > >              writeb(spi->tx_buf ? *spi->tx_buf++ : 0xaa,
> > > > >                      spi->regs + MCHP_CORESPI_REG_TXDATA);
> > > > Please don't abuse the ternery operator like this, just write normal
> > > > conditional statements.
> > > FWIW, that's what my patch does already :-)
> > 
> > Thanks for the series. However, this particular patch appears to
> > introduce a regression. The SPI controller reads an incorrect
> > Device ID from the peripheral.
> 
> Hmm... This is interesting. The only thing I see is missed dummy byte read in
> case of TX only transfers. Is this what you have?

So, the

	else
		readb(spi->regs + MCHP_CORESPI_REG_RXDATA);

should help if it's the case.

But the change can be updated to still have the "data" be always assigned as
in the original code.

> > I’m investigating the root cause and will follow up.
> 
> Okay, I will be glad to know the cause and help to fix that.
> 
> 
> Thank you for the review!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/6] spi: microchip-core: Refactor FIFO read and write handlers

[Conor Dooley]

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

On Thu, Nov 27, 2025 at 06:49:27PM +0200, Andy Shevchenko wrote:
> On Thu, Nov 27, 2025 at 04:08:25PM +0000, Prajna Rajendra Kumar wrote:
> > On 26/11/2025 12:13, Andy Shevchenko wrote:
> > > On Wed, Nov 26, 2025 at 12:05:22PM +0000, Mark Brown wrote:
> > > > On Wed, Nov 26, 2025 at 09:21:45AM +0000, david laight wrote:
> 
> ...
> 
> > > > > I'm not sure I don't prefer the version with one writeb() call.
> > > > > How about:
> > > > >              writeb(spi->tx_buf ? *spi->tx_buf++ : 0xaa,
> > > > >                      spi->regs + MCHP_CORESPI_REG_TXDATA);
> > > > Please don't abuse the ternery operator like this, just write normal
> > > > conditional statements.
> > > FWIW, that's what my patch does already :-)
> > 
> > Thanks for the series. However, this particular patch appears to
> > introduce a regression. The SPI controller reads an incorrect
> > Device ID from the peripheral.
> 
> Hmm... This is interesting. The only thing I see is missed dummy byte read in
> case of TX only transfers. Is this what you have?

Seems very likely, the hardware is pretty simple, so it has no concept
of whether bytes it receives are useful or any ability to operate on
transfers and discard data from the FIFOs when a new one starts. That's
why the unconditional read is there to make sure the rx FIFO is kept in
sync.

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

Re: [PATCH v2 2/6] spi: microchip-core: Refactor FIFO read and write handlers

[Andy Shevchenko]

On Thu, Nov 27, 2025 at 06:05:04PM +0000, Conor Dooley wrote:
> On Thu, Nov 27, 2025 at 06:49:27PM +0200, Andy Shevchenko wrote:
> > On Thu, Nov 27, 2025 at 04:08:25PM +0000, Prajna Rajendra Kumar wrote:
> > > On 26/11/2025 12:13, Andy Shevchenko wrote:
> > > > On Wed, Nov 26, 2025 at 12:05:22PM +0000, Mark Brown wrote:
> > > > > On Wed, Nov 26, 2025 at 09:21:45AM +0000, david laight wrote:

...

> > > > > > I'm not sure I don't prefer the version with one writeb() call.
> > > > > > How about:
> > > > > >              writeb(spi->tx_buf ? *spi->tx_buf++ : 0xaa,
> > > > > >                      spi->regs + MCHP_CORESPI_REG_TXDATA);
> > > > > Please don't abuse the ternery operator like this, just write normal
> > > > > conditional statements.
> > > > FWIW, that's what my patch does already :-)
> > > 
> > > Thanks for the series. However, this particular patch appears to
> > > introduce a regression. The SPI controller reads an incorrect
> > > Device ID from the peripheral.
> > 
> > Hmm... This is interesting. The only thing I see is missed dummy byte read in
> > case of TX only transfers. Is this what you have?
> 
> Seems very likely, the hardware is pretty simple, so it has no concept
> of whether bytes it receives are useful or any ability to operate on
> transfers and discard data from the FIFOs when a new one starts. That's
> why the unconditional read is there to make sure the rx FIFO is kept in
> sync.

I have just sent a v3, please, give it a try.

-- 
With Best Regards,
Andy Shevchenko

Re: (subset) [PATCH v2 0/6] spi: microchip-core: Code improvements

[Mark Brown]

On Wed, 26 Nov 2025 08:54:38 +0100, Andy Shevchenko wrote:
> While reading some other stuff, I noticed that this driver may
> be improved. Here is the set of refactoring and cleaning it up.
> 
> Changelog v2:
> - dropped device property agnostic API conversion change (Mark)
> 
> Andy Shevchenko (6):
>   spi: microchip-core: use min() instead of min_t()
>   spi: microchip-core: Refactor FIFO read and write handlers
>   spi: microchip-core: Replace dead code (-ENOMEM error message)
>   spi: microchip-core: Utilise temporary variable for struct device
>   spi: microchip-core: Use SPI_MODE_X_MASK
>   spi: microchip-core: Remove unneeded PM related macro
> 
> [...]

Applied to

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

Thanks!

[1/6] spi: microchip-core: use min() instead of min_t()
      commit: e29aca7038f3c292c18048922c5f4436a034da99
[3/6] spi: microchip-core: Replace dead code (-ENOMEM error message)
      commit: 274b3458af1f9c665faae70b560852461c30acef
[4/6] spi: microchip-core: Utilise temporary variable for struct device
      commit: 06b010d3c778075108041074a8fb785074231ac4
[5/6] spi: microchip-core: Use SPI_MODE_X_MASK
      commit: 4db5a0705b1e03abb6ff4e7d7789b32c31384429
[6/6] spi: microchip-core: Remove unneeded PM related macro
      commit: f458fc9b1946bc882a217d65bfe5ba50787f253f

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