[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <7hk33yhms4.fsf@deeprootsystems.com>
Date: Fri, 17 Oct 2014 06:02:35 -0700
From: Kevin Hilman <khilman@...nel.org>
To: Wenyou Yang <wenyou.yang@...el.com>
Cc: <broonie@...nel.org>, <linux-spi@...r.kernel.org>,
<linux-kernel@...r.kernel.org>, <nicolas.ferre@...el.com>,
<linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH] spi/atmel: add support for runtime PM
Wenyou Yang <wenyou.yang@...el.com> writes:
> Drivers should put the device into low power states proactively whenever the
> device is not in use. Thus implement support for runtime PM and use the
> autosuspend feature to make sure that we can still perform well in case we see
> lots of SPI traffic within short period of time.
>
> Signed-off-by: Wenyou Yang <wenyou.yang@...el.com>
> ---
> drivers/spi/spi-atmel.c | 62 +++++++++++++++++++++++++++++++++++++++++------
> 1 file changed, 55 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/spi/spi-atmel.c b/drivers/spi/spi-atmel.c
> index 649dcb5..aca285e 100644
> --- a/drivers/spi/spi-atmel.c
> +++ b/drivers/spi/spi-atmel.c
> @@ -26,6 +26,7 @@
> #include <linux/io.h>
> #include <linux/gpio.h>
> #include <linux/pinctrl/consumer.h>
> +#include <linux/pm_runtime.h>
>
> /* SPI register offsets */
> #define SPI_CR 0x0000
> @@ -191,6 +192,8 @@
>
> #define SPI_DMA_TIMEOUT (msecs_to_jiffies(1000))
>
> +#define AUTOSUSPEND_TIMEOUT 2000
> +
> struct atmel_spi_dma {
> struct dma_chan *chan_rx;
> struct dma_chan *chan_tx;
> @@ -1313,6 +1316,7 @@ static int atmel_spi_probe(struct platform_device *pdev)
> master->setup = atmel_spi_setup;
> master->transfer_one_message = atmel_spi_transfer_one_message;
> master->cleanup = atmel_spi_cleanup;
> + master->auto_runtime_pm = true;
> platform_set_drvdata(pdev, master);
>
> as = spi_master_get_devdata(master);
> @@ -1385,6 +1389,11 @@ static int atmel_spi_probe(struct platform_device *pdev)
> dev_info(&pdev->dev, "Atmel SPI Controller at 0x%08lx (irq %d)\n",
> (unsigned long)regs->start, irq);
>
> + pm_runtime_set_autosuspend_delay(&pdev->dev, AUTOSUSPEND_TIMEOUT);
> + pm_runtime_use_autosuspend(&pdev->dev);
> + pm_runtime_set_active(&pdev->dev);
> + pm_runtime_enable(&pdev->dev);
> +
> ret = devm_spi_register_master(&pdev->dev, master);
> if (ret)
> goto out_free_dma;
> @@ -1392,6 +1401,9 @@ static int atmel_spi_probe(struct platform_device *pdev)
> return 0;
>
> out_free_dma:
> + pm_runtime_disable(&pdev->dev);
> + pm_runtime_set_suspended(&pdev->dev);
> +
> if (as->use_dma)
> atmel_spi_release_dma(as);
>
> @@ -1413,6 +1425,8 @@ static int atmel_spi_remove(struct platform_device *pdev)
> struct spi_master *master = platform_get_drvdata(pdev);
> struct atmel_spi *as = spi_master_get_devdata(master);
>
> + pm_runtime_get_sync(&pdev->dev);
> +
> /* reset the hardware and block queue progress */
> spin_lock_irq(&as->lock);
> if (as->use_dma) {
> @@ -1430,9 +1444,13 @@ static int atmel_spi_remove(struct platform_device *pdev)
>
> clk_disable_unprepare(as->clk);
>
> + pm_runtime_put_noidle(&pdev->dev);
Why the _noidle version?
> + pm_runtime_disable(&pdev->dev);
> +
> return 0;
> }
>
> +#ifdef CONFIG_PM
Why CONFIG_PM? CONFIG_PM_SLEEP causes CONFIG_PM=y, so this seems
redundant.
> #ifdef CONFIG_PM_SLEEP
> static int atmel_spi_suspend(struct device *dev)
> {
> @@ -1447,9 +1465,10 @@ static int atmel_spi_suspend(struct device *dev)
> return ret;
> }
>
> - clk_disable_unprepare(as->clk);
> -
> - pinctrl_pm_select_sleep_state(dev);
> + if (!pm_runtime_suspended(dev)) {
> + clk_disable_unprepare(as->clk);
> + pinctrl_pm_select_sleep_state(dev);
> + }
a.k.a. pm_runtime_put_sync() since the ->runtime_suspend() callback does
the same thing.
> return 0;
> }
> @@ -1460,9 +1479,10 @@ static int atmel_spi_resume(struct device *dev)
> struct atmel_spi *as = spi_master_get_devdata(master);
> int ret;
>
> - pinctrl_pm_select_default_state(dev);
> -
> - clk_prepare_enable(as->clk);
> + if (!pm_runtime_suspended(dev)) {
> + pinctrl_pm_select_default_state(dev);
> + clk_prepare_enable(as->clk);
> + }
a.k.a. pm_runtime_get_sync()
Kevin
> /* Start the queue running */
> ret = spi_master_resume(master);
> @@ -1471,9 +1491,37 @@ static int atmel_spi_resume(struct device *dev)
>
> return ret;
> }
> +#endif
>
> -static SIMPLE_DEV_PM_OPS(atmel_spi_pm_ops, atmel_spi_suspend, atmel_spi_resume);
> +#ifdef CONFIG_PM_RUNTIME
> +static int atmel_spi_runtime_suspend(struct device *dev)
> +{
> + struct spi_master *master = dev_get_drvdata(dev);
> + struct atmel_spi *as = spi_master_get_devdata(master);
>
> + clk_disable_unprepare(as->clk);
> + pinctrl_pm_select_sleep_state(dev);
> +
> + return 0;
> +}
> +
> +static int atmel_spi_runtime_resume(struct device *dev)
> +{
> + struct spi_master *master = dev_get_drvdata(dev);
> + struct atmel_spi *as = spi_master_get_devdata(master);
> +
> + pinctrl_pm_select_default_state(dev);
> + clk_prepare_enable(as->clk);
> +
> + return 0;
> +}
> +#endif
> +
> +static const struct dev_pm_ops atmel_spi_pm_ops = {
> + SET_SYSTEM_SLEEP_PM_OPS(atmel_spi_suspend, atmel_spi_resume)
> + SET_RUNTIME_PM_OPS(atmel_spi_runtime_suspend,
> + atmel_spi_runtime_resume, NULL)
> +};
> #define ATMEL_SPI_PM_OPS (&atmel_spi_pm_ops)
> #else
> #define ATMEL_SPI_PM_OPS NULL
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists