On 11/04/2011 07:33 AM, Zhao Chenhui wrote:
> +/* Cast the ccsrbar to 64-bit parameter so that the assembly
> + * code can be compatible with both 32-bit & 36-bit */
> +extern void mpc85xx_enter_deep_sleep(u64 ccsrbar, u32 powmgtreq);

/*
 * Please use proper
 * Linux multi-line comment format.
 */

>  static int pmc_suspend_enter(suspend_state_t state)
>  {
>       int ret;
> +     u32 powmgtreq = 0x00500000;

Where does this 0x00500000 come from?  Please symbolically define
individual bits.

The comment in the asm code says it should be 0x00100000, BTW.

> +
> +     switch (state) {
> +     case PM_SUSPEND_MEM:
> +#ifdef CONFIG_SPE
> +             enable_kernel_spe();
> +#endif

Should comment that currently only e500v2 hardware supports deep sleep
-- else we'd need to save normal FP here.

> +             pr_debug("Entering deep sleep\n");
> +
> +             local_irq_disable();
> +             mpc85xx_enter_deep_sleep(get_immrbase(),
> +                             powmgtreq);
> +             pr_debug("Resumed from deep sleep\n");
> +
> +             return 0;
> +
> +     /* else fall-through */
> +     case PM_SUSPEND_STANDBY:

What fall-through?  You just returned.

> +     }
>  
> -     /* Upon resume, wait for SLP bit to be clear. */
> -     ret = spin_event_timeout((in_be32(&pmc_regs->pmcsr) & PMCSR_SLP) == 0,
> -                              10000, 10) ? 0 : -ETIMEDOUT;
> -     if (ret)
> -             dev_err(pmc_dev, "tired waiting for SLP bit to clear\n");
> -     return ret;
>  }

Remove that blank line as well.

> @@ -58,13 +101,23 @@ static const struct platform_suspend_ops pmc_suspend_ops 
> = {
>       .enter = pmc_suspend_enter,
>  };
>  
> -static int pmc_probe(struct platform_device *ofdev)
> +static int pmc_probe(struct platform_device *pdev)
>  {
> -     pmc_regs = of_iomap(ofdev->dev.of_node, 0);
> +     struct device_node *np = pdev->dev.of_node;
> +
> +     pmc_regs = of_iomap(pdev->dev.of_node, 0);
>       if (!pmc_regs)
>               return -ENOMEM;
>  
> -     pmc_dev = &ofdev->dev;
> +     has_deep_sleep = 0;
> +     if (of_device_is_compatible(np, "fsl,mpc8536-pmc"))
> +             has_deep_sleep = 1;
> +
> +     has_lossless = 0;
> +     if (of_device_is_compatible(np, "fsl,p1022-pmc"))
> +             has_lossless = 1;
> +

You never use has_lossless.

-Scott

_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Reply via email to