Messages in this thread |  | | Date | Wed, 26 Nov 2025 13:17:34 +0200 | | From | Andy Shevchenko <> | | Subject | Re: [PATCH v2 2/6] spi: microchip-core: Refactor FIFO read and write handlers |
| |
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
|  |