On Tue, May 12, 2015 at 10:38:13AM -0700, Jonathan Richardson wrote:
> + /* Wait for interrupt indicating transfer is complete. */
> + if (!wait_for_completion_timeout(&mspi->xfer_done,
> + msecs_to_jiffies(10))) {
What if we have a large transfer on a slow bus?
> + while (bytes_processed < transfer->len) {
> + /* Start transfer and wait until complete. */
> + err = bcm_mspi_start_transfer(mspi, !last_slot, slot);
> + if (err)
> + return err;
This isn't really start_transfer() is it? It's doing the entire
operation.
> + /* Delay requested amount before next transfer. */
> + udelay(transfer->delay_usecs);
> + }
This is buggy, it's applying the per-transfer delay every timme we fill
the FIFO.
> + /* The rx data will go into RXRAM0/1 + last tx length. */
> + if (slot + 1 >= NUM_SLOTS)
> + mspi->rx_slot = 0;
> + else
> + mspi->rx_slot = slot + 1;
How is this going to work for full duplex transfers if we had to fill
the FIFO more than once?
> +static int bcm_mspi_transfer_one(struct spi_master *master,
> + struct spi_device *spidev, struct spi_transfer *transfer)
> +{
> + int err;
> +
> + /* 8 bit transfers only are currently supported. */
> + if (transfer->bits_per_word > 8)
> + return -ENOTSUPP;
Tell the core what the device supports and it will check for you.
> +
> + err = bcm_mspi_tx_data(master, spidev, transfer);
> + if (err)
> + return err;
> +
> + err = bcm_mspi_rx_data(master, spidev, transfer);
> + if (err)
> + return err;
> +
> + return 0;
> +}
I would expect the recieve and transmit operations to be running in
parallel, not executed one after another, given the need to keep
manually filling and draining the FIFOs.
> + struct spi_master *master;
> + struct device *dev = &pdev->dev;
> + int err;
> + struct resource *res;
> + unsigned int irq;
> +
> + dev_info(dev, "Initializing BCM MSPI\n");
Don't spam the logs like this, there's no content in this message.
> + master = spi_alloc_master(dev, sizeof(*data));
> + if (!master) {
devm_spi_alloc_master().
> + /* Map base memory address. */
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + data->base = devm_ioremap_resource(dev, res);
> + if (IS_ERR(data->base)) {
> + dev_err(&pdev->dev, "unable to map base address\n");
devm_ioremap_resource() will complain for you.
Attachment:
signature.asc
Description: Digital signature
