[PATCH v4 0/2] spi: microchip-core: Code improvements (part 2)

[PATCH v4 0/2] spi: microchip-core: Code improvements (part 2)

[Andy Shevchenko]

Here is the second part of the set of refactoring and cleaning it up.

Changelog v4:
- collected tags (Prajna)
- dropped applied patches
- added a new patch 2

v3: <20251127190031.2998705-1-andriy.shevchenko@linux.intel.com>

Changelog v3:
- collected tags (Prajna)
- restored dummy read in TX-only transfers

v2: <20251126075558.2035012-1-andriy.shevchenko@linux.intel.com>

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

Andy Shevchenko (2):
  spi: microchip-core: Refactor FIFO read and write handlers
  spi: microchip-core: use XOR instead of ANDNOT to simplify the logic

 drivers/spi/spi-microchip-core-spi.c | 33 +++++++++++-----------------
 1 file changed, 13 insertions(+), 20 deletions(-)

-- 
2.50.1

[PATCH v4 1/2] 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>
Reviewed-by: Prajna Rajendra Kumar <prajna.rajendrakumar@microchip.com>
---
 drivers/spi/spi-microchip-core-spi.c | 31 +++++++++++-----------------
 1 file changed, 12 insertions(+), 19 deletions(-)

diff --git a/drivers/spi/spi-microchip-core-spi.c b/drivers/spi/spi-microchip-core-spi.c
index 892f066f0074..98bf0e6cd00e 100644
--- a/drivers/spi/spi-microchip-core-spi.c
+++ b/drivers/spi/spi-microchip-core-spi.c
@@ -97,15 +97,12 @@ static inline void mchp_corespi_read_fifo(struct mchp_corespi *spi, u32 fifo_max
 		       MCHP_CORESPI_STATUS_RXFIFO_EMPTY)
 			;
 
+		/* On TX-only transfers always perform a dummy read */
 		data = readb(spi->regs + MCHP_CORESPI_REG_RXDATA);
+		if (spi->rx_buf)
+			*spi->rx_buf++ = data;
 
 		spi->rx_len--;
-		if (!spi->rx_buf)
-			continue;
-
-		*spi->rx_buf = data;
-
-		spi->rx_buf++;
 	}
 }
 
@@ -127,23 +124,19 @@ 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;
 
+		/* On RX-only transfers always perform a dummy write */
 		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 v4 2/2] spi: microchip-core: use XOR instead of ANDNOT to simplify the logic

[Andy Shevchenko]

Use XOR instead of ANDNOT to simplify the logic. The current approach
with (foo & BAR & ~baz) is harder to process than more usual pattern
for the comparing misconfiguration using ((foo ^ baz) & BAR) which
can be read as "find all different bits between foo and baz that are
related to BAR (mask)". Besides that it makes binary code shorter.

Function                                     old     new   delta
mchp_corespi_setup                           103      99      -4

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

diff --git a/drivers/spi/spi-microchip-core-spi.c b/drivers/spi/spi-microchip-core-spi.c
index 98bf0e6cd00e..2f4b21ad56a7 100644
--- a/drivers/spi/spi-microchip-core-spi.c
+++ b/drivers/spi/spi-microchip-core-spi.c
@@ -161,7 +161,7 @@ static int mchp_corespi_setup(struct spi_device *spi)
 		return -EOPNOTSUPP;
 	}
 
-	if (spi->mode & SPI_MODE_X_MASK & ~spi->controller->mode_bits) {
+	if ((spi->mode ^ spi->controller->mode_bits) & SPI_MODE_X_MASK) {
 		dev_err(&spi->dev, "incompatible CPOL/CPHA, must match controller's Motorola mode\n");
 		return -EINVAL;
 	}
-- 
2.50.1

Re: [PATCH v4 2/2] spi: microchip-core: use XOR instead of ANDNOT to simplify the logic

[Jonas Gorski]

On Fri, Nov 28, 2025 at 7:56 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> Use XOR instead of ANDNOT to simplify the logic. The current approach
> with (foo & BAR & ~baz) is harder to process than more usual pattern
> for the comparing misconfiguration using ((foo ^ baz) & BAR) which
> can be read as "find all different bits between foo and baz that are
> related to BAR (mask)". Besides that it makes binary code shorter.
>
> Function                                     old     new   delta
> mchp_corespi_setup                           103      99      -4
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/spi/spi-microchip-core-spi.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/spi/spi-microchip-core-spi.c b/drivers/spi/spi-microchip-core-spi.c
> index 98bf0e6cd00e..2f4b21ad56a7 100644
> --- a/drivers/spi/spi-microchip-core-spi.c
> +++ b/drivers/spi/spi-microchip-core-spi.c
> @@ -161,7 +161,7 @@ static int mchp_corespi_setup(struct spi_device *spi)
>                 return -EOPNOTSUPP;
>         }
>
> -       if (spi->mode & SPI_MODE_X_MASK & ~spi->controller->mode_bits) {
> +       if ((spi->mode ^ spi->controller->mode_bits) & SPI_MODE_X_MASK) {

This changes the behavior: if a bit isn't set in spi->mode that is set
in mode_bits, it would have been previously accepted, now it's
refused. E.g. controller has (SPI_CPOL | SPI_CPHA), device only
SPI_CPOL. 0x1 & 0x3 & ~0x3 => 0, vs (0x1 ^ 0x3) & 0x3 => 0x2

If this is the actually intended behavior here, it is a fix and should
carry a Fixes tag (the message below implies that).

>                 dev_err(&spi->dev, "incompatible CPOL/CPHA, must match controller's Motorola mode\n");
>                 return -EINVAL;
>         }

Best regards,
Jonas

Re: [PATCH v4 2/2] spi: microchip-core: use XOR instead of ANDNOT to simplify the logic

[Andy Shevchenko]

On Fri, Nov 28, 2025 at 08:30:43PM +0100, Jonas Gorski wrote:
> On Fri, Nov 28, 2025 at 7:56 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:

...

> > -       if (spi->mode & SPI_MODE_X_MASK & ~spi->controller->mode_bits) {
> > +       if ((spi->mode ^ spi->controller->mode_bits) & SPI_MODE_X_MASK) {
> 
> This changes the behavior: if a bit isn't set in spi->mode that is set
> in mode_bits, it would have been previously accepted, now it's
> refused. E.g. controller has (SPI_CPOL | SPI_CPHA), device only
> SPI_CPOL. 0x1 & 0x3 & ~0x3 => 0, vs (0x1 ^ 0x3) & 0x3 => 0x2
> 
> If this is the actually intended behavior here, it is a fix and should
> carry a Fixes tag (the message below implies that).

Yeah, yesterday I was thinking about the same and I was confused by the logic
behind. As far as I understood the comments regarding mode provided by DT is
that the mode is configured in IP and may not be changed. And you are right
about the fix, but let's wait for Microchip to elaborate on the expected
behaviour.

> >                 dev_err(&spi->dev, "incompatible CPOL/CPHA, must match controller's Motorola mode\n");
> >                 return -EINVAL;
> >         }

Thanks for the review!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 2/2] spi: microchip-core: use XOR instead of ANDNOT to simplify the logic

[Conor Dooley]

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

On Sat, Nov 29, 2025 at 10:19:00AM +0200, Andy Shevchenko wrote:
> On Fri, Nov 28, 2025 at 08:30:43PM +0100, Jonas Gorski wrote:
> > On Fri, Nov 28, 2025 at 7:56 PM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> 
> ...
> 
> > > -       if (spi->mode & SPI_MODE_X_MASK & ~spi->controller->mode_bits) {
> > > +       if ((spi->mode ^ spi->controller->mode_bits) & SPI_MODE_X_MASK) {
> > 
> > This changes the behavior: if a bit isn't set in spi->mode that is set
> > in mode_bits, it would have been previously accepted, now it's
> > refused. E.g. controller has (SPI_CPOL | SPI_CPHA), device only
> > SPI_CPOL. 0x1 & 0x3 & ~0x3 => 0, vs (0x1 ^ 0x3) & 0x3 => 0x2
> > 
> > If this is the actually intended behavior here, it is a fix and should
> > carry a Fixes tag (the message below implies that).
> 
> Yeah, yesterday I was thinking about the same and I was confused by the logic
> behind. As far as I understood the comments regarding mode provided by DT is
> that the mode is configured in IP and may not be changed. And you are right
> about the fix, but let's wait for Microchip to elaborate on the expected
> behaviour.

Prajna is on holiday and I don't have a setup to actually test this on,
but I'm 99% sure that you're both right and the original behaviour was
wrong. There's a verilog parameter to the IP block that determines which
motorola mode it is and a device that's not an exact match won't work.
FWIW:
Reviewed-by: Conor Dooley <conor.dooley@microchip.com>

Cheers,
Conor.

> 
> > >                 dev_err(&spi->dev, "incompatible CPOL/CPHA, must match controller's Motorola mode\n");
> > >                 return -EINVAL;
> > >         }
> 
> Thanks for the review!
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 

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

Re: [PATCH v4 2/2] spi: microchip-core: use XOR instead of ANDNOT to simplify the logic

[Andy Shevchenko]

On Mon, Dec 01, 2025 at 04:08:57PM +0000, Conor Dooley wrote:
> On Sat, Nov 29, 2025 at 10:19:00AM +0200, Andy Shevchenko wrote:
> > On Fri, Nov 28, 2025 at 08:30:43PM +0100, Jonas Gorski wrote:
> > > On Fri, Nov 28, 2025 at 7:56 PM Andy Shevchenko
> > > <andriy.shevchenko@linux.intel.com> wrote:

...

> > > > -       if (spi->mode & SPI_MODE_X_MASK & ~spi->controller->mode_bits) {
> > > > +       if ((spi->mode ^ spi->controller->mode_bits) & SPI_MODE_X_MASK) {
> > > 
> > > This changes the behavior: if a bit isn't set in spi->mode that is set
> > > in mode_bits, it would have been previously accepted, now it's
> > > refused. E.g. controller has (SPI_CPOL | SPI_CPHA), device only
> > > SPI_CPOL. 0x1 & 0x3 & ~0x3 => 0, vs (0x1 ^ 0x3) & 0x3 => 0x2
> > > 
> > > If this is the actually intended behavior here, it is a fix and should
> > > carry a Fixes tag (the message below implies that).
> > 
> > Yeah, yesterday I was thinking about the same and I was confused by the logic
> > behind. As far as I understood the comments regarding mode provided by DT is
> > that the mode is configured in IP and may not be changed. And you are right
> > about the fix, but let's wait for Microchip to elaborate on the expected
> > behaviour.
> 
> Prajna is on holiday and I don't have a setup to actually test this on,
> but I'm 99% sure that you're both right and the original behaviour was
> wrong. There's a verilog parameter to the IP block that determines which
> motorola mode it is and a device that's not an exact match won't work.

Okay, let's not hurry up with this and wait for testing results.

> FWIW:
> Reviewed-by: Conor Dooley <conor.dooley@microchip.com>

> > > >                 dev_err(&spi->dev, "incompatible CPOL/CPHA, must match controller's Motorola mode\n");
> > > >                 return -EINVAL;
> > > >         }

Thanks for the review!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 2/2] spi: microchip-core: use XOR instead of ANDNOT to simplify the logic

[Prajna Rajendra Kumar]

On 01/12/2025 17:57, Andy Shevchenko wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> On Mon, Dec 01, 2025 at 04:08:57PM +0000, Conor Dooley wrote:
>> On Sat, Nov 29, 2025 at 10:19:00AM +0200, Andy Shevchenko wrote:
>>> On Fri, Nov 28, 2025 at 08:30:43PM +0100, Jonas Gorski wrote:
>>>> On Fri, Nov 28, 2025 at 7:56 PM Andy Shevchenko
>>>> <andriy.shevchenko@linux.intel.com> wrote:
> ...
>
>>>>> -       if (spi->mode & SPI_MODE_X_MASK & ~spi->controller->mode_bits) {
>>>>> +       if ((spi->mode ^ spi->controller->mode_bits) & SPI_MODE_X_MASK) {
>>>> This changes the behavior: if a bit isn't set in spi->mode that is set
>>>> in mode_bits, it would have been previously accepted, now it's
>>>> refused. E.g. controller has (SPI_CPOL | SPI_CPHA), device only
>>>> SPI_CPOL. 0x1 & 0x3 & ~0x3 => 0, vs (0x1 ^ 0x3) & 0x3 => 0x2
>>>>
>>>> If this is the actually intended behavior here, it is a fix and should
>>>> carry a Fixes tag (the message below implies that).
>>> Yeah, yesterday I was thinking about the same and I was confused by the logic
>>> behind. As far as I understood the comments regarding mode provided by DT is
>>> that the mode is configured in IP and may not be changed. And you are right
>>> about the fix, but let's wait for Microchip to elaborate on the expected
>>> behaviour.
>> Prajna is on holiday and I don't have a setup to actually test this on,
>> but I'm 99% sure that you're both right and the original behaviour was
>> wrong. There's a verilog parameter to the IP block that determines which
>> motorola mode it is and a device that's not an exact match won't work.
> Okay, let's not hurry up with this and wait for testing results.
>
>> FWIW:
>> Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
>>>>>                  dev_err(&spi->dev, "incompatible CPOL/CPHA, must match controller's Motorola mode\n");
>>>>>                  return -EINVAL;
>>>>>          }
> Thanks for the review!
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
Hi,

I've tested this on my setup and XOR check matches how the controller
behaves. The SPI mode is fixed in hardware, so the previous logic was
wrong.

Tested-by: Prajna Rajendra Kumar <prajna.rajendrakumar@microchip.com>

Re: [PATCH v4 2/2] spi: microchip-core: use XOR instead of ANDNOT to simplify the logic

[Andy Shevchenko]

On Thu, Jan 08, 2026 at 01:00:39PM +0000, Prajna Rajendra Kumar wrote:
> On 01/12/2025 17:57, Andy Shevchenko wrote:
> > On Mon, Dec 01, 2025 at 04:08:57PM +0000, Conor Dooley wrote:
> > > On Sat, Nov 29, 2025 at 10:19:00AM +0200, Andy Shevchenko wrote:
> > > > On Fri, Nov 28, 2025 at 08:30:43PM +0100, Jonas Gorski wrote:
> > > > > On Fri, Nov 28, 2025 at 7:56 PM Andy Shevchenko
> > > > > <andriy.shevchenko@linux.intel.com> wrote:

...

> > > > > > -       if (spi->mode & SPI_MODE_X_MASK & ~spi->controller->mode_bits) {
> > > > > > +       if ((spi->mode ^ spi->controller->mode_bits) & SPI_MODE_X_MASK) {
> > > > > This changes the behavior: if a bit isn't set in spi->mode that is set
> > > > > in mode_bits, it would have been previously accepted, now it's
> > > > > refused. E.g. controller has (SPI_CPOL | SPI_CPHA), device only
> > > > > SPI_CPOL. 0x1 & 0x3 & ~0x3 => 0, vs (0x1 ^ 0x3) & 0x3 => 0x2
> > > > > 
> > > > > If this is the actually intended behavior here, it is a fix and should
> > > > > carry a Fixes tag (the message below implies that).
> > > > Yeah, yesterday I was thinking about the same and I was confused by the logic
> > > > behind. As far as I understood the comments regarding mode provided by DT is
> > > > that the mode is configured in IP and may not be changed. And you are right
> > > > about the fix, but let's wait for Microchip to elaborate on the expected
> > > > behaviour.
> > > Prajna is on holiday and I don't have a setup to actually test this on,
> > > but I'm 99% sure that you're both right and the original behaviour was
> > > wrong. There's a verilog parameter to the IP block that determines which
> > > motorola mode it is and a device that's not an exact match won't work.
> > Okay, let's not hurry up with this and wait for testing results.
> > 
> > > FWIW:
> > > Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
> > > > > >                  dev_err(&spi->dev, "incompatible CPOL/CPHA, must match controller's Motorola mode\n");
> > > > > >                  return -EINVAL;
> > > > > >          }
> > Thanks for the review!
> 
> I've tested this on my setup and XOR check matches how the controller
> behaves. The SPI mode is fixed in hardware, so the previous logic was
> wrong.
> 
> Tested-by: Prajna Rajendra Kumar <prajna.rajendrakumar@microchip.com>

Thank you! I just sent a new version of this patch with updated tags and commit
message.

-- 
With Best Regards,
Andy Shevchenko