| Message ID | 20090310062421.22328.68726.sendpatchset@rx1.opensource.se (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
Hi Magnus, > + > + Â Â Â device_init_wakeup(&pdev->dev, 1); > Â Â Â Â return 0; Please add device_init_wakeup(&pdev->dev, 0) in sh_keysc_remove. > Â err5: > Â Â Â Â free_irq(irq, pdev); > @@ -253,17 +255,36 @@ static int __devexit sh_keysc_remove(str > Â Â Â Â return 0; > Â } > > +static int sh_keysc_suspend(struct device *dev) > +{ > + Â Â Â struct platform_device *pdev; > + Â Â Â struct sh_keysc_priv *priv; > + Â Â Â unsigned short value; > + > + Â Â Â pdev = container_of(dev, struct platform_device, dev); > + Â Â Â priv = platform_get_drvdata(pdev); to_platform_device(...) ?
Hi Trilok, Thanks for your comments! On Tue, Mar 10, 2009 at 5:24 PM, Trilok Soni <soni.trilok@gmail.com> wrote: > Hi Magnus, > >> + >> + Â Â Â device_init_wakeup(&pdev->dev, 1); >> Â Â Â Â return 0; > > > Please add device_init_wakeup(&pdev->dev, 0) in sh_keysc_remove. Isn't this just flagging the device as wakeup capable? My first guess would be that the the wakeup state of a removed device is irrelevant since it won't be suspended anyway. Or am I misunderstanding? Please explain in more detail. >> Â err5: >> Â Â Â Â free_irq(irq, pdev); >> @@ -253,17 +255,36 @@ static int __devexit sh_keysc_remove(str >> Â Â Â Â return 0; >> Â } >> >> +static int sh_keysc_suspend(struct device *dev) >> +{ >> + Â Â Â struct platform_device *pdev; >> + Â Â Â struct sh_keysc_priv *priv; >> + Â Â Â unsigned short value; >> + >> + Â Â Â pdev = container_of(dev, struct platform_device, dev); >> + Â Â Â priv = platform_get_drvdata(pdev); > > to_platform_device(...) ? Thanks, much better! / magnus -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Magnus, >> >> Please add device_init_wakeup(&pdev->dev, 0) in sh_keysc_remove. > > Isn't this just flagging the device as wakeup capable? My first guess > would be that the the wakeup state of a removed device is irrelevant > since it won't be suspended anyway. Or am I misunderstanding? Please > explain in more detail. I may not have complete answer, but I am thinking of someone depending/reading on wakeup file state to determine the state of your device in userspace will get confused, so better to disable to wakeup capability on remove.
On Tue, Mar 10, 2009 at 7:43 PM, Trilok Soni <soni.trilok@gmail.com> wrote: >>> Please add device_init_wakeup(&pdev->dev, 0) in sh_keysc_remove. >> >> Isn't this just flagging the device as wakeup capable? My first guess >> would be that the the wakeup state of a removed device is irrelevant >> since it won't be suspended anyway. Or am I misunderstanding? Please >> explain in more detail. > > I may not have complete answer, but I am thinking of someone depending/reading > on wakeup file state to determine the state of your device in userspace will get > confused, so better to disable to wakeup capability on remove. I think the per device wakeup file will disappear from sysfs when the device gets removed, so resetting the flag in the remove() callback just makes the driver code more confusing in my opinion. =) Cheers, / magnus -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Magnus, On Tue, Mar 10, 2009 at 4:22 PM, Magnus Damm <magnus.damm@gmail.com> wrote: > On Tue, Mar 10, 2009 at 7:43 PM, Trilok Soni <soni.trilok@gmail.com> wrote: >>>> Please add device_init_wakeup(&pdev->dev, 0) in sh_keysc_remove. >>> >>> Isn't this just flagging the device as wakeup capable? My first guess >>> would be that the the wakeup state of a removed device is irrelevant >>> since it won't be suspended anyway. Or am I misunderstanding? Please >>> explain in more detail. >> >> I may not have complete answer, but I am thinking of someone depending/reading >> on wakeup file state to determine the state of your device in userspace will get >> confused, so better to disable to wakeup capability on remove. > > I think the per device wakeup file will disappear from sysfs when the > device gets removed, so resetting the flag in the remove() callback > just makes the driver code more confusing in my opinion. =) Yes, I thought the same but then I am surprised to find my drivers doing device_init_wakeup(...., 0) on driver removal. #grep "device_init_wakeup(&pdev->dev, 0)" -d recurse ./ | more ./usb/musb/musb_core.c: device_init_wakeup(&pdev->dev, 0); ./usb/gadget/at91_udc.c: device_init_wakeup(&pdev->dev, 0); ./usb/host/ohci-at91.c: device_init_wakeup(&pdev->dev, 0); ./serial/atmel_serial.c: device_init_wakeup(&pdev->dev, 0); ./input/keyboard/gpio_keys.c: device_init_wakeup(&pdev->dev, 0); ./rtc/rtc-at32ap700x.c: device_init_wakeup(&pdev->dev, 0); ./rtc/rtc-omap.c: device_init_wakeup(&pdev->dev, 0); ./rtc/rtc-omap.c: device_init_wakeup(&pdev->dev, 0); ./pcmcia/at91_cf.c: device_init_wakeup(&pdev->dev, 0); ./pcmcia/at91_cf.c: device_init_wakeup(&pdev->dev, 0); ./mfd/htc-egpio.c: device_init_wakeup(&pdev->dev, 0); ./mmc/host/at91_mci.c: device_init_wakeup(&pdev->dev, 0);
--- 0001/drivers/input/keyboard/sh_keysc.c +++ work/drivers/input/keyboard/sh_keysc.c 2009-03-09 19:14:47.000000000 +0900 @@ -219,6 +219,8 @@ static int __devinit sh_keysc_probe(stru pdata->scan_timing, priv->iomem_base + KYCR1_OFFS); iowrite16(0, priv->iomem_base + KYOUTDR_OFFS); iowrite16(KYCR2_IRQ_LEVEL, priv->iomem_base + KYCR2_OFFS); + + device_init_wakeup(&pdev->dev, 1); return 0; err5: free_irq(irq, pdev); @@ -253,17 +255,36 @@ static int __devexit sh_keysc_remove(str return 0; } +static int sh_keysc_suspend(struct device *dev) +{ + struct platform_device *pdev; + struct sh_keysc_priv *priv; + unsigned short value; + + pdev = container_of(dev, struct platform_device, dev); + priv = platform_get_drvdata(pdev); + + value = ioread16(priv->iomem_base + KYCR1_OFFS); + + if (device_may_wakeup(dev)) + value |= 0x80; + else + value &= ~0x80; -#define sh_keysc_suspend NULL -#define sh_keysc_resume NULL + iowrite16(value, priv->iomem_base + KYCR1_OFFS); + return 0; +} + +static struct dev_pm_ops sh_keysc_dev_pm_ops = { + .suspend = sh_keysc_suspend, +}; struct platform_driver sh_keysc_device_driver = { .probe = sh_keysc_probe, .remove = __devexit_p(sh_keysc_remove), - .suspend = sh_keysc_suspend, - .resume = sh_keysc_resume, .driver = { .name = "sh_keysc", + .pm = &sh_keysc_dev_pm_ops, } };