Re: [PATCH] spi/pl022: Enable clock in probe an use runtime_idle

[Russell King - ARM Linux <linux-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org>]

On Fri, Oct 21, 2011 at 04:08:44PM +0200, Ulf Hansson wrote:
> Since we are always runtime resumed when leaving probe
> the clock must be enabled. To accomplish that we are able
> to be runtime suspended after probe in the case when no
> request is going to be recieved, a runtime_idle function
> has been implemented.
> 
> Change-Id: I6cb86f2cad30ecaab16f512daf4674b039b18213
> Signed-off-by: Ulf Hansson <ulf.hansson-0IS4wlFg1OjSUeElwK9/Pw@public.gmane.org>
> Reviewed-on: http://gerrit.lud.stericsson.com/gerrit/34447

Sorry Ulf, this patch has issues.

>  drivers/spi/spi-pl022.c |   18 +++++++++++++++++-
>  1 files changed, 17 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/spi/spi-pl022.c b/drivers/spi/spi-pl022.c
> index f103e47..ad48fba 100644
> --- a/drivers/spi/spi-pl022.c
> +++ b/drivers/spi/spi-pl022.c
> @@ -2184,6 +2184,12 @@ pl022_probe(struct amba_device *adev, const struct amba_id *id)
>  		goto  err_clk_prep;
>  	}
>  
> +	status = clk_enable(pl022->clk);
> +	if (status) {
> +		dev_err(&adev->dev, "could not enable SSP/SPI bus clock\n");
> +		goto err_no_clk_en;
> +	}
> +
>  	/* Disable SSP */
>  	writew((readw(SSP_CR1(pl022->virtbase)) & (~SSP_CR1_MASK_SSE)),
>  	       SSP_CR1(pl022->virtbase));

Ok - this is a bug fix.

> @@ -2237,6 +2243,8 @@ pl022_probe(struct amba_device *adev, const struct amba_id *id)
>  
>  	free_irq(adev->irq[0], pl022);
>   err_no_irq:
> +	clk_disable(pl022->clk);
> + err_no_clk_en:
>  	clk_unprepare(pl022->clk);
>   err_clk_prep:
>  	clk_put(pl022->clk);

As is this.

> @@ -2342,11 +2350,19 @@ static int pl022_runtime_resume(struct device *dev)
>  
>  	return 0;
>  }
> +
> +static int pl022_runtime_idle(struct device *dev)
> +{
> +	pm_runtime_suspend(dev);
> +	return 0;
> +}
>  #endif
>  
>  static const struct dev_pm_ops pl022_dev_pm_ops = {
>  	SET_SYSTEM_SLEEP_PM_OPS(pl022_suspend, pl022_resume)
> -	SET_RUNTIME_PM_OPS(pl022_runtime_suspend, pl022_runtime_resume, NULL)
> +	SET_RUNTIME_PM_OPS(pl022_runtime_suspend,
> +			   pl022_runtime_resume,
> +			   pl022_runtime_idle)

This is an unnecessary change.

The bus-level ops runtime PM ops call pm_generic_runtime_idle() when
its 'runtime_idle' operation is invoked.  Let's look at the code
there:

int pm_generic_runtime_idle(struct device *dev)
{
        const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;

        if (pm && pm->runtime_idle) {
                int ret = pm->runtime_idle(dev);
                if (ret)
                        return ret;
        }

        pm_runtime_suspend(dev);
        return 0;
}

If the driver has a NULL runtime idle, then generic code will call
pm_runtime_suspend() for the device.  So, adding a runtime_idle callback
to a driver to explicitly call pm_runtime_suspend() is not required.

------------------------------------------------------------------------------
RSA&#174; Conference 2012
Save $700 by Nov 18
Register now&#33;
http://p.sf.net/sfu/rsa-sfdev2dev1