lkml.org 
[lkml]   [2021]   [Sep]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v3 2/2] spi: cadence: add support for Cadence XSPI controller
On Wed, Sep 01, 2021 at 02:37:38PM +0200, Parshuram Thombare wrote:

> +++ b/drivers/spi/spi-cadence-xspi.c
> @@ -0,0 +1,837 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Cadence XSPI flash controller driver

Please make the entire comment a C++ so things look more intentional.

> +static int cdns_xspi_setup(struct spi_device *spi_dev)
> +{
> + if (spi_dev->chip_select > spi_dev->master->num_chipselect) {
> + dev_err(&spi_dev->dev,
> + "%d chip-select is out of range\n",
> + spi_dev->chip_select);
> + return -EINVAL;
> + }
> +
> + return 0;
> +}

The core already validates this, are you seeing it happen? If so we
should fix the core and either way just remove setup() entirely.

> + if (irq_status) {
> + writel(irq_status,
> + cdns_xspi->iobase + CDNS_XSPI_INTR_STATUS_REG);
> +
> + if (irq_status & CDNS_XSPI_SDMA_ERROR) {
> + dev_err(cdns_xspi->dev,
> + "Slave DMA transaction error\n");
> + cdns_xspi->sdma_error = true;
> + complete(&cdns_xspi->sdma_complete);
> + }
> +
> + if (irq_status & CDNS_XSPI_SDMA_TRIGGER)
> + complete(&cdns_xspi->sdma_complete);
> +
> + if (irq_status & CDNS_XSPI_STIG_DONE)
> + complete(&cdns_xspi->cmd_complete);
> +
> + result = IRQ_HANDLED;
> + }

We will just silently ignore any unknown interrupts here. It would be
better to either only ack known interrupts (so genirq can notice if
there's a problem with other interrupts) or at least log that we're
seeing unexpected interrupts. The current code will cause trouble if
this is deployed in a system with the interrupt line shared (which the
driver claims to support), or if something goes wrong and the IP starts
asserting some interrupt that isn't expected.

> + master->mode_bits = SPI_3WIRE | SPI_TX_DUAL | SPI_TX_QUAD |
> + SPI_RX_DUAL | SPI_RX_QUAD | SPI_TX_OCTAL | SPI_RX_OCTAL |
> + SPI_MODE_0 | SPI_MODE_3;

I don't see any handling of these in the code?
[unhandled content-type:application/pgp-signature]
\
 
 \ /
  Last update: 2021-09-02 16:40    [W:1.089 / U:0.720 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site