Re: [PATCH v3 2/2] spi: cadence: add support for Cadence XSPI controller
From: Mark Brown <broonie@kernel.org>
Date: 2021-09-02 14:40:22
Also in:
linux-spi, lkml
On Wed, Sep 01, 2021 at 02:37:38PM +0200, Parshuram Thombare wrote:
quoted hunk ↗ jump to hunk
+++ 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?
Attachments
- signature.asc [application/pgp-signature] 488 bytes