[lkml]   [2022]   [Jan]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
SubjectRe: [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.



 \ /
  Last update: 2022-01-01 08:26    [W:0.287 / U:0.356 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site