input: add suspend wakeup support to sh_keysc
diff mbox

Message ID 20090310062421.22328.68726.sendpatchset@rx1.opensource.se
State Accepted
Headers show

Commit Message

Magnus Damm March 10, 2009, 6:24 a.m. UTC
From: Magnus Damm <damm@igel.co.jp>

This patch adds wakeup support to the sh_keysc driver.
With this feature the ".../power/wakeup" file can be
used to enable and disable if the device takes the
system out of suspend. Default is enabled.

Signed-off-by: Magnus Damm <damm@igel.co.jp>
---

 drivers/input/keyboard/sh_keysc.c |   29 +++++++++++++++++++++++++----
 1 file changed, 25 insertions(+), 4 deletions(-)

--
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

Comments

Trilok Soni March 10, 2009, 8:24 a.m. UTC | #1
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(...) ?
Magnus Damm March 10, 2009, 10:01 a.m. UTC | #2
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
Trilok Soni March 10, 2009, 10:43 a.m. UTC | #3
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.
Magnus Damm March 10, 2009, 10:52 a.m. UTC | #4
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
Trilok Soni March 10, 2009, 11:10 a.m. UTC | #5
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);

Patch
diff mbox

--- 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,
 	}
 };