| Message ID | ec1741f29c44c436c1086877ff5b208e6b8af45d.1547559542.git.baolin.wang@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
On Tue, Jan 15, 2019 at 09:46:51PM +0800, Baolin Wang wrote: This looks good, just one small issue and a thing to check: > +static irqreturn_t sprd_spi_handle_irq(int irq, void *data) > +{ > + struct sprd_spi *ss = (struct sprd_spi *)data; > + u32 val = readl_relaxed(ss->base + SPRD_SPI_INT_MASK_STS); > + > + if (val & SPRD_SPI_MASK_TX_END) { > + writel_relaxed(SPRD_SPI_TX_END_CLR, ss->base + SPRD_SPI_INT_CLR); > + if (!(ss->trans_mode & SPRD_SPI_RX_MODE)) > + complete(&ss->xfer_completion); > + return IRQ_HANDLED; > + } > + > + if (val & SPRD_SPI_MASK_RX_END) { > + writel_relaxed(SPRD_SPI_RX_END_CLR, ss->base + SPRD_SPI_INT_CLR); > + complete(&ss->xfer_completion); > + } > + > + return IRQ_HANDLED; > +} This will return IRQ_HANDLED no matter if there was an interrupt actually handled. That means that if something goes wrong due to some bug or a hardware change (eg, a new version of the IP) and there's another interrupt fired we won't clear it and the interrupt core won't be able to detect that it's a spurious interrupt and use its own error handling. It's better to return IRQ_NONE in that case. > + ret = devm_request_irq(&pdev->dev, ss->irq, sprd_spi_handle_irq, > + 0, pdev->name, ss); > + if (ret) > + dev_err(&pdev->dev, "failed to request spi irq %d, ret = %d\n", > + ss->irq, ret); Are you sure it's safe to use devm_request_irq(), especially when unloading the driver? Using it means that we will only disable the interrupt after the driver's remove function has finished so there's a danger of an interrupt firing when some of the resources the hander has used are still in use. I didn't spot any issues, just something to check especially with the later patches building on top of this.
diff --git a/drivers/spi/spi-sprd.c b/drivers/spi/spi-sprd.c index fa324ce..bfba30b 100644 --- a/drivers/spi/spi-sprd.c +++ b/drivers/spi/spi-sprd.c @@ -133,6 +133,7 @@ struct sprd_spi { void __iomem *base; struct device *dev; struct clk *clk; + int irq; u32 src_clk; u32 hw_mode; u32 trans_len; @@ -141,6 +142,7 @@ struct sprd_spi { u32 hw_speed_hz; u32 len; int status; + struct completion xfer_completion; const void *tx_buf; void *rx_buf; int (*read_bufs)(struct sprd_spi *ss, u32 len); @@ -575,6 +577,45 @@ static int sprd_spi_transfer_one(struct spi_controller *sctlr, return ret; } +static irqreturn_t sprd_spi_handle_irq(int irq, void *data) +{ + struct sprd_spi *ss = (struct sprd_spi *)data; + u32 val = readl_relaxed(ss->base + SPRD_SPI_INT_MASK_STS); + + if (val & SPRD_SPI_MASK_TX_END) { + writel_relaxed(SPRD_SPI_TX_END_CLR, ss->base + SPRD_SPI_INT_CLR); + if (!(ss->trans_mode & SPRD_SPI_RX_MODE)) + complete(&ss->xfer_completion); + return IRQ_HANDLED; + } + + if (val & SPRD_SPI_MASK_RX_END) { + writel_relaxed(SPRD_SPI_RX_END_CLR, ss->base + SPRD_SPI_INT_CLR); + complete(&ss->xfer_completion); + } + + return IRQ_HANDLED; +} + +static int sprd_spi_irq_init(struct platform_device *pdev, struct sprd_spi *ss) +{ + int ret; + + ss->irq = platform_get_irq(pdev, 0); + if (ss->irq < 0) { + dev_err(&pdev->dev, "failed to get irq resource\n"); + return ss->irq; + } + + ret = devm_request_irq(&pdev->dev, ss->irq, sprd_spi_handle_irq, + 0, pdev->name, ss); + if (ret) + dev_err(&pdev->dev, "failed to request spi irq %d, ret = %d\n", + ss->irq, ret); + + return ret; +} + static int sprd_spi_clk_init(struct platform_device *pdev, struct sprd_spi *ss) { struct clk *clk_spi, *clk_parent; @@ -635,11 +676,16 @@ static int sprd_spi_probe(struct platform_device *pdev) sctlr->max_speed_hz = min_t(u32, ss->src_clk >> 1, SPRD_SPI_MAX_SPEED_HZ); + init_completion(&ss->xfer_completion); platform_set_drvdata(pdev, sctlr); ret = sprd_spi_clk_init(pdev, ss); if (ret) goto free_controller; + ret = sprd_spi_irq_init(pdev, ss); + if (ret) + goto free_controller; + ret = clk_prepare_enable(ss->clk); if (ret) goto free_controller;