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

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

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

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
> 
> 

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