[spi,for-5.4,13/14] spi: spi-fsl-dspi: Reduce indentation level in dspi_interrupt
diff mbox series

Message ID 20190818180115.31114-14-olteanv@gmail.com
State New
Headers show
Series
  • NXP DSPI driver cleanup
Related show

Commit Message

Vladimir Oltean Aug. 18, 2019, 6:01 p.m. UTC
There is no point in checking which interrupt source was triggered in
SPI_SR, since only EOQ and TCFQ modes trigger interrupts anyway (see the
writes to SPI_RSER). In DMA mode, the RSER is configured for RFDF_DIRS=1
and TFFF_DIRS=1, aka FIFO events generate eDMA requests and not CPU
interrupts.

Signed-off-by: Vladimir Oltean <olteanv@gmail.com>
---
 drivers/spi/spi-fsl-dspi.c | 76 +++++++++++++++++++-------------------
 1 file changed, 37 insertions(+), 39 deletions(-)

Comments

Mark Brown Aug. 20, 2019, 1:02 p.m. UTC | #1
On Sun, Aug 18, 2019 at 09:01:14PM +0300, Vladimir Oltean wrote:
> There is no point in checking which interrupt source was triggered in
> SPI_SR, since only EOQ and TCFQ modes trigger interrupts anyway (see the
> writes to SPI_RSER). In DMA mode, the RSER is configured for RFDF_DIRS=1
> and TFFF_DIRS=1, aka FIFO events generate eDMA requests and not CPU
> interrupts.

It's also good to check interrupt sources in case the interrupt line
might be shared, that means that it's possible that the interrupt
handler will be called when there's no interrupt at all from the IP.  It
also helps with robustness in case there's some system error though
that's (hopefully!) a lot less common.  This driver does set IRQF_SHARED
so it does support that, I don't know how many systems could do it but
it seems a shame to remove the support from the driver.
Vladimir Oltean Aug. 20, 2019, 10:43 p.m. UTC | #2
On Tue, 20 Aug 2019 at 16:02, Mark Brown <broonie@kernel.org> wrote:
>
> On Sun, Aug 18, 2019 at 09:01:14PM +0300, Vladimir Oltean wrote:
> > There is no point in checking which interrupt source was triggered in
> > SPI_SR, since only EOQ and TCFQ modes trigger interrupts anyway (see the
> > writes to SPI_RSER). In DMA mode, the RSER is configured for RFDF_DIRS=1
> > and TFFF_DIRS=1, aka FIFO events generate eDMA requests and not CPU
> > interrupts.
>
> It's also good to check interrupt sources in case the interrupt line
> might be shared, that means that it's possible that the interrupt
> handler will be called when there's no interrupt at all from the IP.  It
> also helps with robustness in case there's some system error though
> that's (hopefully!) a lot less common.  This driver does set IRQF_SHARED
> so it does support that, I don't know how many systems could do it but
> it seems a shame to remove the support from the driver.

Ok, I hadn't thought of that, I'll add the check back.
Vladimir Oltean Aug. 20, 2019, 10:49 p.m. UTC | #3
On Wed, 21 Aug 2019 at 01:43, Vladimir Oltean <olteanv@gmail.com> wrote:
>
> On Tue, 20 Aug 2019 at 16:02, Mark Brown <broonie@kernel.org> wrote:
> >
> > On Sun, Aug 18, 2019 at 09:01:14PM +0300, Vladimir Oltean wrote:
> > > There is no point in checking which interrupt source was triggered in
> > > SPI_SR, since only EOQ and TCFQ modes trigger interrupts anyway (see the
> > > writes to SPI_RSER). In DMA mode, the RSER is configured for RFDF_DIRS=1
> > > and TFFF_DIRS=1, aka FIFO events generate eDMA requests and not CPU
> > > interrupts.
> >
> > It's also good to check interrupt sources in case the interrupt line
> > might be shared, that means that it's possible that the interrupt
> > handler will be called when there's no interrupt at all from the IP.  It
> > also helps with robustness in case there's some system error though
> > that's (hopefully!) a lot less common.  This driver does set IRQF_SHARED
> > so it does support that, I don't know how many systems could do it but
> > it seems a shame to remove the support from the driver.
>
> Ok, I hadn't thought of that, I'll add the check back.

But shouldn't it be returning IRQ_NONE in that case? Right now it's
returning IRQ_HANDLED.
Mark Brown Aug. 21, 2019, 10:01 a.m. UTC | #4
On Wed, Aug 21, 2019 at 01:49:15AM +0300, Vladimir Oltean wrote:

> But shouldn't it be returning IRQ_NONE in that case? Right now it's
> returning IRQ_HANDLED.

Yes, that's a bug that should be fixed.

Patch
diff mbox series

diff --git a/drivers/spi/spi-fsl-dspi.c b/drivers/spi/spi-fsl-dspi.c
index 790cb02fc181..2993d15f640e 100644
--- a/drivers/spi/spi-fsl-dspi.c
+++ b/drivers/spi/spi-fsl-dspi.c
@@ -658,47 +658,45 @@  static irqreturn_t dspi_interrupt(int irq, void *dev_id)
 	regmap_read(dspi->regmap, SPI_SR, &spi_sr);
 	regmap_write(dspi->regmap, SPI_SR, spi_sr);
 
+	/* Get transfer counter (in number of SPI transfers). It was
+	 * reset to 0 when transfer(s) were started.
+	 */
+	regmap_read(dspi->regmap, SPI_TCR, &spi_tcr);
+	spi_tcnt = SPI_TCR_GET_TCNT(spi_tcr);
+	/* Update total number of bytes that were transferred */
+	msg->actual_length += spi_tcnt * dspi->bytes_per_word;
+
+	trans_mode = dspi->devtype_data->trans_mode;
+	switch (trans_mode) {
+	case DSPI_EOQ_MODE:
+		dspi_eoq_read(dspi);
+		break;
+	case DSPI_TCFQ_MODE:
+		dspi_tcfq_read(dspi);
+		break;
+	default:
+		dev_err(&dspi->pdev->dev, "unsupported trans_mode %u\n",
+			trans_mode);
+			return IRQ_HANDLED;
+	}
 
-	if (spi_sr & (SPI_SR_EOQF | SPI_SR_TCFQF)) {
-		/* Get transfer counter (in number of SPI transfers). It was
-		 * reset to 0 when transfer(s) were started.
-		 */
-		regmap_read(dspi->regmap, SPI_TCR, &spi_tcr);
-		spi_tcnt = SPI_TCR_GET_TCNT(spi_tcr);
-		/* Update total number of bytes that were transferred */
-		msg->actual_length += spi_tcnt * dspi->bytes_per_word;
-
-		trans_mode = dspi->devtype_data->trans_mode;
-		switch (trans_mode) {
-		case DSPI_EOQ_MODE:
-			dspi_eoq_read(dspi);
-			break;
-		case DSPI_TCFQ_MODE:
-			dspi_tcfq_read(dspi);
-			break;
-		default:
-			dev_err(&dspi->pdev->dev, "unsupported trans_mode %u\n",
-				trans_mode);
-				return IRQ_HANDLED;
-		}
+	if (!dspi->len) {
+		dspi->waitflags = 1;
+		wake_up_interruptible(&dspi->waitq);
+		return IRQ_HANDLED;
+	}
 
-		if (!dspi->len) {
-			dspi->waitflags = 1;
-			wake_up_interruptible(&dspi->waitq);
-		} else {
-			switch (trans_mode) {
-			case DSPI_EOQ_MODE:
-				dspi_eoq_write(dspi);
-				break;
-			case DSPI_TCFQ_MODE:
-				dspi_tcfq_write(dspi);
-				break;
-			default:
-				dev_err(&dspi->pdev->dev,
-					"unsupported trans_mode %u\n",
-					trans_mode);
-			}
-		}
+	switch (trans_mode) {
+	case DSPI_EOQ_MODE:
+		dspi_eoq_write(dspi);
+		break;
+	case DSPI_TCFQ_MODE:
+		dspi_tcfq_write(dspi);
+		break;
+	default:
+		dev_err(&dspi->pdev->dev,
+			"unsupported trans_mode %u\n",
+			trans_mode);
 	}
 
 	return IRQ_HANDLED;