On Wed, Aug 19, 2015 at 10:39:16AM +0530, kamlakant.patel@xxxxxxxxxxxx wrote:
> +config SPI_XLP
> + tristate "Netlogic XLP SPI controller driver"
> + depends on CPU_XLP
Can we not have an || COMPILE_TEST here? There's no headers included
that look like a problem.
It also looks like you have tab/space damage here.
> +static int xlp_spi_setup(struct spi_device *spi)
> +{
> + struct xlp_spi_priv *xspi;
> + u32 fdiv, cfg;
> + int cs;
> +
> + if (!spi->bits_per_word)
> + spi->bits_per_word = 8;
The core will ensure that bits_per_word is always set, there is no need
to duplicate core functionality.
> + /*
> + * The value of fdiv must be between 4 and 65535.
> + */
> + fdiv = DIV_ROUND_UP(xspi->spi_clk, spi->max_speed_hz);
> + fdiv = fdiv < XLP_SPI_FDIV_MIN ? XLP_SPI_FDIV_MIN : fdiv;
> + fdiv = fdiv > XLP_SPI_FDIV_MAX ? XLP_SPI_FDIV_MAX : fdiv;
Please write if statements using the standard if construct, it's much
better for legibility.
> + while (rxfifo_cnt) {
> + rx_data = xlp_spi_reg_read(xspi, xspi->cs, XLP_SPI_RXDATA_FIFO);
> + j = 0;
> + nbytes = xspi->rx_len > 4 ? 4 : xspi->rx_len;
This is another (probably more serious) example of the problems with the
ternery operator.
> + txfifo_cnt = xlp_spi_reg_read(xspi, xspi->cs, XLP_SPI_FIFO_WCNT);
> + txfifo_cnt >>= XLP_SPI_TXFIFO_WCNT_SHIFT;
> + txfifo_cnt &= XLP_SPI_FIFO_WCNT_MASK;
This is a bit surprising - I'd expect a _MASK define to define the mask
for the register field so we'd mask then shift rather than shift then
mask.
> +static irqreturn_t xlp_spi_interrupt(int irq, void *dev_id)
> +{
> + struct xlp_spi_priv *xspi = dev_id;
> + u32 stat;
> +
> + stat = xlp_spi_reg_read(xspi, xspi->cs, XLP_SPI_STATUS);
> + if (xspi->tx_len) {
> + if (stat & XLP_SPI_TX_TH_OV)
> + xlp_spi_fill_txfifo(xspi);
> + if (stat & XLP_SPI_TX_UF)
> + xspi->txerrors++;
> + }
> + if (xspi->rx_len) {
> + if (stat & XLP_SPI_RX_TH_OV)
> + xlp_spi_read_rxfifo(xspi);
> + if (stat & XLP_SPI_RX_OF)
> + xspi->rxerrors++;
We should tell the user about errors more noticably to aid debugging -
I'd expect to see us printing an error message here.
> + }
> + /* ACK all interrupts */
> + xlp_spi_reg_write(xspi, xspi->cs, XLP_SPI_STATUS,
> + (stat & XLP_SPI_STAT_MASK));
This unconditionally acknowledges all interrupts even if we didn't
handle them.
> + if (stat & XLP_SPI_XFR_DONE)
> + complete(&xspi->done);
> +
> + return IRQ_HANDLED;
> +}
We're also unconditionally reporting that we handled an interrupt even
if we didn't which will break if the interrupt is ever shared and
prevent the kernel interrupt subsystem error handling working.
> + xlp_spi_send_cmd(xs, xfer_len, cmd_cont);
> + if (xs->tx_len)
> + xlp_spi_reg_write(xs, xs->cs, XLP_SPI_INTR_EN,
> + XLP_SPI_INTR_MASK);
> + else
> + xlp_spi_reg_write(xs, xs->cs, XLP_SPI_INTR_EN,
> + XLP_SPI_RXINTR_MASK);
So the device is single duplex?
> +static int xlp_spi_transfer_one(struct spi_master *master,
> + struct spi_message *msg)
> +{
This is called transfer_one() but it's actually a transfer_one_message()
operation. Though it looks like we should be able to convert it into a
transfer_one() operation and make greater use of core functionality.
> + if (t->transfer_list.next == &msg->transfers)
> + xspi->cmd_cont = 0;
> + else
> + xspi->cmd_cont = 1;
This is spi_tansfer_is_last() (and should've been using list_is_last()
anyway AFAICT).
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!res) {
> + dev_err(&pdev->dev, "can't get IOMEM resource\n");
> + return -ENODEV;
> + }
> + xspi->base = devm_ioremap_resource(&pdev->dev, res);
> + if (IS_ERR(xspi->base))
> + return PTR_ERR(xspi->base);
Just pass the resource to devm_ioremap_resource(), it'll handle a lookup
failure for you.
> + irq = platform_get_irq(pdev, 0);
> + if (irq < 0) {
> + dev_err(&pdev->dev, "Failed to get IRQ\n");
> + return irq;
> + }
Print any error codes you get to help users trying to fix problems.
Attachment:
signature.asc
Description: Digital signature
