Re: [PATCH v3 26/32] usb: musb: omap2430: use dev_pm_ops structure

[Sergei Shtylyov <sshtylyov@xxxxxxxxxx>]

Hello.

On 07-12-2010 19:39, Felipe Balbi wrote:

> instead of using musb_platform_suspend/resume,
> we can use dev_pm_ops and let the platform_device
> core handle when to call musb_core's suspend and
> glue layer's suspend.

> Signed-off-by: Felipe Balbi<balbi@xxxxxx>
[...]
> diff --git a/drivers/usb/musb/omap2430.c b/drivers/usb/musb/omap2430.c
> index ef0f7fe..ed4ca41 100644
> --- a/drivers/usb/musb/omap2430.c
> +++ b/drivers/usb/musb/omap2430.c
[...]
> @@ -188,6 +187,33 @@ static int omap2430_musb_set_mode(struct musb *musb, u8 musb_mode)
>   	return 0;
>   }
>
> +static inline void omap2430_low_level_init(struct musb *musb)
> +{
> +	u32 l;
> +
> +	/* in any role */
> +	l = musb_readl(musb->mregs, OTG_FORCESTDBY);
> +	l |= ENABLEFORCE;	/* enable MSTANDBY */
> +	musb_writel(musb->mregs, OTG_FORCESTDBY, l);
> +
> +	l = musb_readl(musb->mregs, OTG_SYSCONFIG);
> +	l |= ENABLEWAKEUP;	/* enable wakeup */
> +	musb_writel(musb->mregs, OTG_SYSCONFIG, l);
> +}
> +
> +static inline void omap2430_low_level_exit(struct musb *musb)
> +{
> +	u32 l;
> +
> +	l = musb_readl(musb->mregs, OTG_SYSCONFIG);
> +	l &= ~ENABLEWAKEUP;	/* disable wakeup */
> +	musb_writel(musb->mregs, OTG_SYSCONFIG, l);
> +
> +	l = musb_readl(musb->mregs, OTG_FORCESTDBY);
> +	l &= ~ENABLEFORCE;	/* disable MSTANDBY */
> +	musb_writel(musb->mregs, OTG_FORCESTDBY, l);
> +}
> +
>   static int omap2430_musb_init(struct musb *musb)
>   {
>   	u32 l;
> @@ -205,7 +231,7 @@ static int omap2430_musb_init(struct musb *musb)
>   		return -ENODEV;
>   	}
>
> -	omap2430_musb_resume(musb);
> +	omap2430_low_level_init(musb);

   Wait! omap2430_musb_resume() disabled wakeup and MSTANDBY, while 
omap2430_low_level_init() enables them... :-/

>   	l = musb_readl(musb->mregs, OTG_SYSCONFIG);
>   	l &= ~ENABLEWAKEUP;	/* disable wakeup */

   Why disable what's already disabled?

> @@ -250,63 +276,12 @@ static int omap2430_musb_init(struct musb *musb)
>   	return 0;
>   }
>
> -static void omap2430_save_context(struct musb *musb)
> -{
> -	musb->context.otg_sysconfig = musb_readl(musb->mregs, OTG_SYSCONFIG);
> -	musb->context.otg_forcestandby = musb_readl(musb->mregs, OTG_FORCESTDBY);
> -}
> -
> -static void omap2430_restore_context(struct musb *musb)
> -{
> -	musb_writel(musb->mregs, OTG_SYSCONFIG, musb->context.otg_sysconfig);
> -	musb_writel(musb->mregs, OTG_FORCESTDBY, musb->context.otg_forcestandby);
> -}
> -
> -static int omap2430_musb_suspend(struct musb *musb)
> -{
> -	u32 l;
> -
> -	/* in any role */
> -	l = musb_readl(musb->mregs, OTG_FORCESTDBY);
> -	l |= ENABLEFORCE;	/* enable MSTANDBY */
> -	musb_writel(musb->mregs, OTG_FORCESTDBY, l);
> -
> -	l = musb_readl(musb->mregs, OTG_SYSCONFIG);
> -	l |= ENABLEWAKEUP;	/* enable wakeup */
> -	musb_writel(musb->mregs, OTG_SYSCONFIG, l);
> -
> -	omap2430_save_context(musb);
> -
> -	otg_set_suspend(musb->xceiv, 1);
> -
> -	return 0;
> -}
> -
> -static int omap2430_musb_resume(struct musb *musb)
> -{
> -	u32 l;
> -
> -	otg_set_suspend(musb->xceiv, 0);
> -
> -	omap2430_restore_context(musb);
> -
> -	l = musb_readl(musb->mregs, OTG_SYSCONFIG);
> -	l&= ~ENABLEWAKEUP;	/* disable wakeup */
> -	musb_writel(musb->mregs, OTG_SYSCONFIG, l);
> -
> -	l = musb_readl(musb->mregs, OTG_FORCESTDBY);
> -	l&= ~ENABLEFORCE;	/* disable MSTANDBY */
> -	musb_writel(musb->mregs, OTG_FORCESTDBY, l);
> -
> -	return 0;
> -}
> -
>   static int omap2430_musb_exit(struct musb *musb)
>   {
>
> -	omap2430_musb_suspend(musb);
> -
> +	omap2430_low_level_exit(musb);

   And vice versa here...

>   	otg_put_transceiver(musb->xceiv);
> +
>   	return 0;
>   }
>
> @@ -314,9 +289,6 @@ static const struct musb_platform_ops omap2430_ops = {
>   	.init		= omap2430_musb_init,
>   	.exit		= omap2430_musb_exit,
>
> -	.suspend	= omap2430_musb_suspend,
> -	.resume		= omap2430_musb_resume,
> -
>   	.set_mode	= omap2430_musb_set_mode,
>   	.try_idle	= omap2430_musb_try_idle,
>
> @@ -421,10 +393,66 @@ static int __exit omap2430_remove(struct platform_device *pdev)
>   	return 0;
>   }
>
> +#ifdef CONFIG_PM
> +static void omap2430_save_context(struct musb *musb)
> +{
> +	musb->context.otg_sysconfig = musb_readl(musb->mregs, OTG_SYSCONFIG);
> +	musb->context.otg_forcestandby = musb_readl(musb->mregs, OTG_FORCESTDBY);
> +}
> +
> +static void omap2430_restore_context(struct musb *musb)
> +{
> +	musb_writel(musb->mregs, OTG_SYSCONFIG, musb->context.otg_sysconfig);
> +	musb_writel(musb->mregs, OTG_FORCESTDBY, musb->context.otg_forcestandby);
> +}
> +
> +static int omap2430_suspend(struct device *dev)
> +{
> +	struct omap2430_glue		*glue = dev_get_drvdata(dev);
> +	struct musb			*musb = glue_to_musb(glue);
> +
> +	omap2430_low_level_exit(musb);

   Wait! You need to enable wakeup and MSTANDBY but you're siabling it here...

> +	otg_set_suspend(musb->xceiv, 1);
> +	omap2430_save_context(musb);
> +	clk_disable(glue->clk);
> +
> +	return 0;
> +}
> +
> +static int omap2430_resume(struct device *dev)
> +{
> +	struct omap2430_glue		*glue = dev_get_drvdata(dev);
> +	struct musb			*musb = glue_to_musb(glue);
> +	int				ret;
> +
> +	ret = clk_enable(glue->clk);
> +	if (ret) {
> +		dev_err(dev, "faled to enable clock\n");
> +		return ret;
> +	}
> +
> +	omap2430_low_level_init(musb);

   Vice versa here.

> +	omap2430_restore_context(musb);
> +	otg_set_suspend(musb->xceiv, 0);
> +
> +	return 0;
> +}
> +
> +static struct dev_pm_ops omap2430_pm_ops = {
> +	.suspend	= omap2430_suspend,
> +	.resume		= omap2430_resume,
> +};
> +
> +#define DEV_PM_OPS	&omap2430_pm_ops

   I've just read this should be giving scripts/checkpatch.pl error without 
parens...

WBR, Sergei
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html