Re: [PATCH] spi: tegra: add spi driver for SLINK controller

From: Mark Brown
Date: Tue Oct 23 2012 - 05:17:07 EST

On Mon, Oct 22, 2012 at 02:02:47PM -0600, Stephen Warren wrote:
> On 10/18/2012 04:47 AM, Laxman Dewangan wrote:

> > + /* Read back register to make sure that register writes completed */
> > + if (reg != SLINK_TX_FIFO)
> > + readl(tspi->base + SLINK_MAS_DATA);

> Is that really necessary on every single write, or only for certain
> specific operations? This seems rather heavy-weight.

I saw that myself - I figured that since SPI is (relatively) slow itself
the simplicity gained from doing the flush unconditionally was
reasonable, further optimisation can always be done later.

> > + val = tegra_slink_readl(tspi, SLINK_STATUS);

> > + /* Write 1 to clear status register */
> > + val_write = SLINK_RDY;
> > + val_write |= (val & SLINK_FIFO_ERROR);

> Why not just always set SLINK_FIFO_ERROR; does it have to be set in the
> write only if the status was previously asserted? If that is true, how
> do you avoid a race condition where the bit gets set in SLINK_STATUS
> after you read it but before you write to clear it?

The whole point with this sort of interrupt scheme is usually that the
interrupt is level triggered and will just continue to be asserted if
some source within the interrupt isn't acked; the race is usually the
other way around where you may ack a status you didn't see.

> > +static unsigned int tegra_slink_read_rx_fifo_to_client_rxbuf(
> > + bits_per_word = t->bits_per_word ? t->bits_per_word :
> > + tspi->cur_spi->bits_per_word;

> That logic is repeated a few times. Shouldn't there be a macro to avoid
> cut/paste. Perhaps even in the SPI core rather than this driver.

Yes, should be in the SPI core as this pattern exists for all SPI

> Doesn't this function need to parse all the other standardized
> properties from spi-bus.txt, or does the SPI core handle that?

No, it doesn't. But perhaps it should (see the thing above about the
per-transfer max frequency too...). In general SPI hasn't done much
about factoring code out of drivers until very recently.

Attachment: signature.asc
Description: Digital signature