Messages in this thread | | | Date | Sat, 1 Jan 2022 08:25:48 +0100 | From | Lukas Wunner <> | Subject | Re: [PATCH 3/3] spi: apple: Add driver for Apple SPI controller |
| |
On Sun, Dec 12, 2021 at 12:47:26PM +0900, Hector Martin wrote: > + ret = devm_spi_register_controller(&pdev->dev, ctlr); [...] > +static int apple_spi_remove(struct platform_device *pdev) > +{ > + struct spi_controller *ctlr = platform_get_drvdata(pdev); > + struct apple_spi *spi = spi_controller_get_devdata(ctlr); > + > + pm_runtime_disable(&pdev->dev); > + > + /* Disable all the interrupts just in case */ > + reg_write(spi, APPLE_SPI_IE_FIFO, 0); > + reg_write(spi, APPLE_SPI_IE_XFER, 0); > + > + clk_disable_unprepare(spi->clk);
You need to use spi_register_controller() in apple_spi_probe() (*not* the devm variant) and invoke spi_unregister_controller() first thing in apple_spi_remove().
Otherwise you've got an ordering issue on driver unbind: __device_release_driver() first calls the ->remove hook and only then devres_release_all(). You're disabling the clock and interrupts in the ->remove hook, rendering the controller unusable. Yet the expectation is that until spi_unregister_controller() returns, the controller works and its slaves are accessible.
This is especially important if there are slaves attached to the controller which perform SPI transfers in their ->remove hooks, e.g. to quiesce interrupts on the slaves. Those transfers won't work the way you've structured the code now.
spi-sifive.c, from which you've derived this, is broken. As are a couple dozen other drivers. I've fixed some of them, but haven't gotten around to fixing them all. Just trying to prevent more brokenness from entering the codebase.
Thanks,
Lukas
| |