Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752476AbaJQNCk (ORCPT ); Fri, 17 Oct 2014 09:02:40 -0400 Received: from mail-pa0-f54.google.com ([209.85.220.54]:41342 "EHLO mail-pa0-f54.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751251AbaJQNCi (ORCPT ); Fri, 17 Oct 2014 09:02:38 -0400 From: Kevin Hilman To: Wenyou Yang Cc: , , , , Subject: Re: [PATCH] spi/atmel: add support for runtime PM References: <1413424160-21180-1-git-send-email-wenyou.yang@atmel.com> Date: Fri, 17 Oct 2014 06:02:35 -0700 In-Reply-To: <1413424160-21180-1-git-send-email-wenyou.yang@atmel.com> (Wenyou Yang's message of "Thu, 16 Oct 2014 09:49:20 +0800") Message-ID: <7hk33yhms4.fsf@deeprootsystems.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Wenyou Yang 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 > --- > 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 > #include > #include > +#include > > /* 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/