Re: [PATCH 1/3] spi: add xtfpga SPI controller driver

[Mark Brown]

On Tue, Mar 11, 2014 at 04:44:49PM +0400, Max Filippov wrote:

> +static inline int xtfpga_spi_wait_busy(struct xtfpga_spi *xspi)
> +{
> +	unsigned long timeout = jiffies + msecs_to_jiffies(100);
> +	while (xtfpga_spi_read32(xspi, XTFPGA_SPI_BUSY)) {
> +		if (!time_before(jiffies, timeout))
> +			return -EBUSY;
> +		else
> +			cpu_relax();
> +	}
> +	return 0;
> +}

So we'll busy wait for up to 100ms - that seems like an awfully long
time.  Perhaps fall back to msleep() if the delay is non-trivial (or
just reduce the timeout)?  If there are large enough FIFOs dead
reckoning a sleep for the expected transfer time might be helpful but it
seems like there's no meaningful FIFO.

> +/* Unused: this device controls its only CS automatically,
> + * deactivating it after every 16 bit transfer completion.
> + */

This is too limited to use with most SPI clients, they'll want to be
able to transmit more than one word (and the fact that only 16 bit words
are supported is also an issue, though that's easy enough to handle for
a bitbanging driver - I'd really strongly suggest supporting 8 bits per
word as well).  Clients are pretty much going to need to use GPIO based
chip select, you should make sure that's supported and covered in the
binding.

> +static void xtfpga_spi_chipselect(struct spi_device *spi, int is_on)
> +{
> +}

Omit this since it's empty.

Attachment: signature.asc
Description: Digital signature