Messages in this thread | | | Date | Tue, 14 Apr 2020 12:16:46 +0100 | From | Mark Brown <> | Subject | Re: [PATCH] spi: spi-amd: Add AMD SPI controller driver support |
| |
On Sun, Apr 12, 2020 at 02:28:31PM -0500, Sanjay R Mehta wrote:
> +++ b/drivers/spi/spi-amd.c > @@ -0,0 +1,341 @@ > +// SPDX-License-Identifier: GPL-2.0 OR BSD-3-Clause > +/* > + * AMD SPI controller driver > + *
Please make the entire comment a C++ one so things look more intentional.
> +#define DRIVER_NAME "amd_spi"
This is unused.
> +/* M_CMD OP codes for SPI */ > +#define SPI_XFER_TX 1 > +#define SPI_XFER_RX 2
These constants should be namespaced, they're likely to collide with generic additions.
> +static void amd_spi_execute_opcode(struct spi_master *master) > +{ > + struct amd_spi *amd_spi = spi_master_get_devdata(master); > + bool spi_busy; > + > + /* Set ExecuteOpCode bit in the CTRL0 register */ > + amd_spi_setclear_reg32(master, AMD_SPI_CTRL0_REG, AMD_SPI_EXEC_CMD, > + AMD_SPI_EXEC_CMD); > + > + /* poll for SPI bus to become idle */ > + spi_busy = (ioread32((u8 __iomem *)amd_spi->io_remap_addr + > + AMD_SPI_CTRL0_REG) & AMD_SPI_BUSY) == AMD_SPI_BUSY; > + while (spi_busy) { > + set_current_state(TASK_INTERRUPTIBLE); > + schedule(); > + set_current_state(TASK_RUNNING); > + spi_busy = (ioread32((u8 __iomem *)amd_spi->io_remap_addr + > + AMD_SPI_CTRL0_REG) & AMD_SPI_BUSY) == AMD_SPI_BUSY; > + }
This is a weird way to busy wait - usually you'd use a cpu_relax() rather than a schedule(). There's also no timeout here so we could busy wait for ever if something goes wrong.
> +static int amd_spi_master_setup(struct spi_device *spi) > +{ > + struct spi_master *master = spi->master; > + struct amd_spi *amd_spi = spi_master_get_devdata(master); > + > + amd_spi->chip_select = spi->chip_select; > + amd_spi_select_chip(master);
This looks like it will potentially affect devices other than the current one. setup() may be called while other devices are active it shouldn't do that.
> + } else if (m_cmd & SPI_XFER_RX) { > + /* Store no. of bytes to be received from > + * FIFO > + */ > + rx_len = xfer->len; > + buffer = (u8 *)xfer->rx_buf;
> + /* Read data from FIFO to receive buffer */ > + for (i = 0; i < rx_len; i++) > + buffer[i] = ioread8((u8 __iomem *)amd_spi->io_remap_addr > + + AMD_SPI_FIFO_BASE > + + tx_len + i);
This will only work for messages with a single receive transfer, if there are multiple transfers then you'll need to store multiple buffers and their lengths.
> +static int amd_spi_master_transfer(struct spi_master *master, > + struct spi_message *msg) > +{ > + struct amd_spi *amd_spi = spi_master_get_devdata(master); > + > + /* > + * Extract spi_transfers from the spi message and > + * program the controller. > + */ > + amd_spi_fifo_xfer(amd_spi, msg); > + > + return 0; > +}
This function is completely redundant, just inline amd_spi_fifo_xfer(). It also ignores all errors which isn't great.
> + /* Initialize the spi_master fields */ > + master->bus_num = 0; > + master->num_chipselect = 4; > + master->mode_bits = 0; > + master->flags = 0;
This device is single duplex so should flag that.
> + err = spi_register_master(master); > + if (err) { > + dev_err(dev, "error registering SPI controller\n"); > + goto err_iounmap;
It's best to print the error code to help people debug things. [unhandled content-type:application/pgp-signature] | |