Date:   Tue, 23 Nov 2021 13:48:27 +0100
From:   Lukas Wunner <lukas@wunner.de>
To:     "LH.Kuo" <lhjeff911@gmail.com>
Cc:     p.zabel@pengutronix.de, broonie@kernel.org, robh+dt@kernel.org,
        linux-spi@vger.kernel.org, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org, dvorkin@tibbo.com,
        qinjian@cqplus1.com, wells.lu@sunplus.com,
        "LH.Kuo" <lh.kuo@sunplus.com>
Subject: Re: [PATCH v3 1/2] SPI: Add SPI driver for Sunplus SP7021
Message-ID: <20211123124827.GA22253@wunner.de>
References: <cover.1637547799.git.lh.kuo@sunplus.com>
 <e5f2549224cf875d81306ef5f6e98db1cfd81c2e.1637547799.git.lh.kuo@sunplus.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <e5f2549224cf875d81306ef5f6e98db1cfd81c2e.1637547799.git.lh.kuo@sunplus.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
Precedence: bulk

On Mon, Nov 22, 2021 at 10:33:32AM +0800, LH.Kuo wrote:
> +static int sp7021_spi_controller_probe(struct platform_device *pdev)
> +{
[...]
> +	ret = devm_spi_register_controller(&pdev->dev, ctlr);
> +	if (ret != 0) {
> +		dev_err(&pdev->dev, "spi_register_master fail\n");
> +		goto disable_runtime_pm;
> +	}

You need to use spi_register_controller() here (*not* the devm_ variant)
because you're using spi_unregister_controller() in
sp7021_spi_controller_remove().

> +
> +	// clk
> +	pspim->spi_clk = devm_clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(pspim->spi_clk)) {
> +		return dev_err_probe(&pdev->dev, PTR_ERR(pspim->spi_clk),
> +				     "devm_clk_get fail\n");
> +	}
> +
> +	// reset
> +	pspim->rstc = devm_reset_control_get_exclusive(&pdev->dev, NULL);
> +	dev_dbg(&pdev->dev, "pspim->rstc : 0x%x\n", (unsigned int)pspim->rstc);
> +	if (IS_ERR(pspim->rstc)) {
> +		return dev_err_probe(&pdev->dev, PTR_ERR(pspim->rstc),
> +				     "devm_rst_get fail\n");
> +	}
> +
> +	ret = clk_prepare_enable(pspim->spi_clk);
> +	if (ret)
> +		return dev_err_probe(&pdev->dev, ret,
> +			"failed to enable clk\n");
> +
> +	ret = reset_control_deassert(pspim->rstc);
> +	if (ret) {
> +		dev_err_probe(&pdev->dev, ret,
> +			"failed to deassert reset\n");
> +		goto free_reset_assert;
> +
> +	}

You need to move the above steps *before* the call to
spi_register_controller().  Once spi_register_controller() returns,
it must be able to perform SPI transactions.  So you have to enable
all required clocks before calling it.  You also have to perform the
reset step before registration to avoid interfering with an ongoing
transaction.  The order of these steps must mirror the order in
sp7021_spi_controller_remove():  There you're unregistering the
controller *before* disabling the clock and asserting reset,
so the order must be inverted here.


> +static int sp7021_spi_controller_remove(struct platform_device *pdev)
> +{
> +	struct spi_controller *ctlr = dev_get_drvdata(&pdev->dev);
> +	struct sp7021_spi_ctlr *pspim = spi_master_get_devdata(ctlr);
> +
> +	pm_runtime_disable(&pdev->dev);
> +	pm_runtime_set_suspended(&pdev->dev);
> +
> +	spi_unregister_controller(pspim->ctlr);
> +	clk_disable_unprepare(pspim->spi_clk);
> +	reset_control_assert(pspim->rstc);
> +
> +	return 0;
> +}

I think the two calls to pm_runtime_* should be moved after
spi_unregister_controller() but that's probably not critical.

Thanks,

Lukas