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.1On 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)
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.
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
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
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
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.
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
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
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
© 2016 - 2025 Red Hat, Inc.