i.MX27: add suspend to RAM support

PATCH Superseded Eric Bénard 11 participants 33 messages May 27, 2010 – Jul 1, 2010

Subsystems (10)

Networking Network drivers Arm (32-bit) Arm sub-architectures +6 more subsystems

lore

Share & tools

lore

Eric Bénard

author

2010-05-27 06:33 UTC #0 lore raw

PATCH v2 1/8 i.MX27: add suspend to RAM support

Signed-off-by Eric B?nard <[email protected]>

---
 arch/arm/mach-mx2/Makefile   |    2 +-
 arch/arm/mach-mx2/pm-imx27.c |   46 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 47 insertions(+), 1 deletions(-)
 create mode 100644 arch/arm/mach-mx2/pm-imx27.c

arch/arm/mach-mx2/Makefile +1 -1

index e3254fa..27d496c 100644
@@ -8,7 +8,7 @@ obj-y	:=  devices.o serial.o
 
 obj-$(CONFIG_MACH_MX21) += clock_imx21.o mm-imx21.o
 
-obj-$(CONFIG_MACH_MX27) += cpu_imx27.o
+obj-$(CONFIG_MACH_MX27) += cpu_imx27.o pm-imx27.o
 obj-$(CONFIG_MACH_MX27) += clock_imx27.o mm-imx27.o
 
 obj-$(CONFIG_MACH_MX21ADS) += mach-mx21ads.o

arch/arm/mach-mx2/pm-imx27.c new file +46 -0

46 lines not on this page · raw message

Signature · 1 line

1.6.3.3

Eric Bénard

author

2010-05-27 06:33 UTC #1 lore raw

PATCH v2 2/8 mxc_nand: fix PM

* when resuming, we get the following log :
nand_resume called for a chip which is not in suspended state
* this happens because mtd->suspend and mtd->resume are already called from the mtd layer, thus they were called a second time in mxc_nand so mtd complains when trying to resume for the second time
* the fix is to remove these call and thus remove the supend and resume functions of mxc_nand

Signed-off-by Eric B?nard <[email protected]>

---
 drivers/mtd/nand/mxc_nand.c |   44 +------------------------------------------
 1 files changed, 1 insertions(+), 43 deletions(-)

drivers/mtd/nand/mxc_nand.c +1 ~~-43~~

index 314e2bf..217b247 100644
@@ -892,53 +892,11 @@ static int __devexit mxcnd_remove(struct platform_device *pdev)
 	return 0;
 }
 
-#ifdef CONFIG_PM
-static int mxcnd_suspend(struct platform_device *pdev, pm_message_t state)
-{
-	struct mtd_info *mtd = platform_get_drvdata(pdev);
-	struct nand_chip *nand_chip = mtd->priv;
-	struct mxc_nand_host *host = nand_chip->priv;
-	int ret = 0;
-
-	DEBUG(MTD_DEBUG_LEVEL0, "MXC_ND : NAND suspend\n");
-
-	ret = mtd->suspend(mtd);
-
-	/*
-	 * nand_suspend locks the device for exclusive access, so
-	 * the clock must already be off.
-	 */
-	BUG_ON(!ret && host->clk_act);
-
-	return ret;
-}
-
-static int mxcnd_resume(struct platform_device *pdev)
-{
-	struct mtd_info *mtd = platform_get_drvdata(pdev);
-	struct nand_chip *nand_chip = mtd->priv;
-	struct mxc_nand_host *host = nand_chip->priv;
-	int ret = 0;
-
-	DEBUG(MTD_DEBUG_LEVEL0, "MXC_ND : NAND resume\n");
-
-	mtd->resume(mtd);
-
-	return ret;
-}
-
-#else
-# define mxcnd_suspend   NULL
-# define mxcnd_resume    NULL
-#endif				/* CONFIG_PM */
-
 static struct platform_driver mxcnd_driver = {
 	.driver = {
 		   .name = DRIVER_NAME,
-		   },
+	},
 	.remove = __devexit_p(mxcnd_remove),
-	.suspend = mxcnd_suspend,
-	.resume = mxcnd_resume,
 };
 
 static int __init mxc_nd_init(void)

Signature · 1 line

1.6.3.3

Eric Bénard

author

2010-05-27 06:33 UTC #2 lore raw

PATCH v2 3/8 imx_keypad: add PM support

* add suspend/resume support
* use keypad_data to enable wakeup from platform data
* suspend/resume tested when keypad is not in use and when it's is use (in which case we immediatly get the key pressed for wakeup)

Signed-off-by Eric B?nard <[email protected]>

---
 drivers/input/keyboard/imx_keypad.c |   56 +++++++++++++++++++++++++++++++++-
 1 files changed, 54 insertions(+), 2 deletions(-)

drivers/input/keyboard/imx_keypad.c +54 -2

index d92c15c..b303cc9 100644
@@ -364,6 +364,55 @@ static void imx_keypad_inhibit(struct imx_keypad *keypad)
 	writew(0xff00, keypad->mmio_base + KPCR);
 }
 
+static int mxc_kpp_suspend(struct platform_device *pdev, pm_message_t state)
+{
+	struct imx_keypad *keypad = platform_get_drvdata(pdev);
+	unsigned short reg_val;
+
+	clk_disable(keypad->clk);
+
+	if (device_may_wakeup(&pdev->dev)) {
+		reg_val = readw(keypad->mmio_base + KPSR);
+		if ((reg_val & KBD_STAT_KDIE) == 0) {
+			/* If no depress interrupt enable the release interrupt */
+			reg_val |= KBD_STAT_KRIE;
+			writew(reg_val, keypad->mmio_base + KPSR);
+		}
+		enable_irq_wake(keypad->irq);
+	} else {
+		keypad->enabled = false;
+		synchronize_irq(keypad->irq);
+		disable_irq(keypad->irq);
+	}
+
+	return 0;
+}
+
+/*!
+ * This function brings the Keypad controller back from low-power state.
+ * If Keypad is enabled as a wake source(i.e. it can resume the system
+ * from suspend mode), the Keypad controller doesn't enter low-power state.
+ *
+ * @param   pdev  the device structure used to give information on Keypad
+ *                to resume
+ *
+ * @return  The function always returns 0.
+ */
+static int mxc_kpp_resume(struct platform_device *pdev)
+{
+	struct imx_keypad *keypad = platform_get_drvdata(pdev);
+
+	clk_enable(keypad->clk);
+	if (device_may_wakeup(&pdev->dev)) {
+		/* The irq routine already cleared KRIE if it was set */
+	} else {
+		keypad->enabled = true;
+		enable_irq(keypad->irq);
+	}
+
+	return 0;
+}
+
 static void imx_keypad_close(struct input_dev *dev)
 {
 	struct imx_keypad *keypad = input_get_drvdata(dev);

@@ -410,7 +459,8 @@ open_err:
 
 static int __devinit imx_keypad_probe(struct platform_device *pdev)
 {
-	const struct matrix_keymap_data *keymap_data = pdev->dev.platform_data;
+	const struct matrix_keypad_platform_data *keypad_data = pdev->dev.platform_data;
+	const struct matrix_keymap_data *keymap_data = keypad_data->keymap_data;
 	struct imx_keypad *keypad;
 	struct input_dev *input_dev;
 	struct resource *res;

@@ -525,7 +575,7 @@ static int __devinit imx_keypad_probe(struct platform_device *pdev)
 	}
 
 	platform_set_drvdata(pdev, keypad);
-	device_init_wakeup(&pdev->dev, 1);
+	device_init_wakeup(&pdev->dev, keypad_data->wakeup);
 
 	return 0;

@@ -574,6 +624,8 @@ static struct platform_driver imx_keypad_driver = {
 	},
 	.probe		= imx_keypad_probe,
 	.remove		= __devexit_p(imx_keypad_remove),
+	.suspend = mxc_kpp_suspend,
+	.resume = mxc_kpp_resume,
 };
 
 static int __init imx_keypad_init(void)

Signature · 1 line

1.6.3.3

Eric Bénard

author

2010-05-27 06:33 UTC #3 lore raw

PATCH v2 4/8 eukrea_mbimx27: update keyboard platform data

* use matrix_keypad_platform_data
* enable wakeup

Signed-off-by Eric B?nard <[email protected]>

---
 arch/arm/mach-mx2/eukrea_mbimx27-baseboard.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

arch/arm/mach-mx2/eukrea_mbimx27-baseboard.c +6 -1

index 34bdc26..9038e1f 100644
@@ -107,6 +107,11 @@ static struct matrix_keymap_data eukrea_mbimx27_keymap_data = {
 	.keymap_size    = ARRAY_SIZE(eukrea_mbimx27_keymap),
 };
 
+static struct matrix_keypad_platform_data eukrea_mbimx27_keypad_data = {
+	.keymap_data	= &eukrea_mbimx27_keymap_data,
+	.wakeup		= 1,
+};
+
 static struct gpio_led gpio_leds[] = {
 	{
 		.name			= "led1",

@@ -350,7 +355,7 @@ void __init eukrea_mbimx27_baseboard_init(void)
 	gpio_request(GPIO_PORTA | 25, "lcd_enable");
 	platform_device_register(&eukrea_mbimx27_lcd_powerdev);
 
-	mxc_register_device(&imx_kpp_device, &eukrea_mbimx27_keymap_data);
+	mxc_register_device(&imx_kpp_device, &eukrea_mbimx27_keypad_data);
 
 	platform_add_devices(platform_devices, ARRAY_SIZE(platform_devices));
 }

Signature · 1 line

1.6.3.3

Eric Bénard

author

2010-05-27 06:33 UTC #4 lore raw

PATCH v2 5/8 serial/imx.c: fix suspend/resume

* when uart is used as a kernel's console, its baudrate is wrong after resume :
when resuming, serial_core call set_termios which fails to get the right baudrate.

Signed-off-by Eric B?nard <[email protected]>

---
 drivers/serial/imx.c |   16 ++++++++++++++++
 1 files changed, 16 insertions(+), 0 deletions(-)

drivers/serial/imx.c +16 -0

index eacb588..936f810 100644
@@ -1020,6 +1020,17 @@ imx_verify_port(struct uart_port *port, struct serial_struct *ser)
 	return ret;
 }
 
+static void
+imx_suspend(struct uart_port *port, unsigned int state, unsigned int oldstate)
+{
+	struct imx_port *sport = (struct imx_port *)port;
+
+	if (!state)
+		clk_enable(sport->clk);
+	else
+		clk_disable(sport->clk);
+}
+
 static struct uart_ops imx_pops = {
 	.tx_empty	= imx_tx_empty,
 	.set_mctrl	= imx_set_mctrl,

@@ -1037,6 +1048,7 @@ static struct uart_ops imx_pops = {
 	.request_port	= imx_request_port,
 	.config_port	= imx_config_port,
 	.verify_port	= imx_verify_port,
+	.pm		= imx_suspend,
 };
 
 static struct imx_port *imx_ports[UART_NR];

@@ -1207,6 +1219,7 @@ static struct uart_driver imx_reg = {
 	.cons           = IMX_CONSOLE,
 };
 
+#ifdef CONFIG_PM
 static int serial_imx_suspend(struct platform_device *dev, pm_message_t state)
 {
 	struct imx_port *sport = platform_get_drvdata(dev);

@@ -1226,6 +1239,7 @@ static int serial_imx_resume(struct platform_device *dev)
 
 	return 0;
 }
+#endif
 
 static int serial_imx_probe(struct platform_device *pdev)
 {

@@ -1343,8 +1357,10 @@ static struct platform_driver serial_imx_driver = {
 	.probe		= serial_imx_probe,
 	.remove		= serial_imx_remove,
 
+#ifdef CONFIG_PM
 	.suspend	= serial_imx_suspend,
 	.resume		= serial_imx_resume,
+#endif
 	.driver		= {
 		.name	= "imx-uart",
 		.owner	= THIS_MODULE,

Signature · 1 line

1.6.3.3

Eric Bénard

author

2010-05-27 06:33 UTC #5 lore raw

PATCH v2 6/8 serial/imx: drvdata is sport and not &sport->port

even if that's the same pointer this is more readable like this

Signed-off-by Eric B?nard <[email protected]>

---
 drivers/serial/imx.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

drivers/serial/imx.c +1 -1

index 936f810..a0ebe91 100644
@@ -1311,7 +1311,7 @@ static int serial_imx_probe(struct platform_device *pdev)
 	ret = uart_add_one_port(&imx_reg, &sport->port);
 	if (ret)
 		goto deinit;
-	platform_set_drvdata(pdev, &sport->port);
+	platform_set_drvdata(pdev, sport);
 
 	return 0;
 deinit:

Signature · 1 line

1.6.3.3

Eric Bénard

author

2010-05-27 06:33 UTC #6 lore raw

PATCH v2 7/8 net/fec: fix pm to survive to suspend/resume

* with this patch, if the connection if active before suspend, it will be active after resume.
* before this patch, it was necessary to close the interface and reopen it to recover the connection.

Signed-off-by Eric B?nard <[email protected]>

---
 drivers/net/fec.c |    9 ++++++---
 1 files changed, 6 insertions(+), 3 deletions(-)

drivers/net/fec.c +6 -3

index 9b4e8f7..654abaf 100644
@@ -1971,13 +1971,14 @@ static int
 fec_suspend(struct platform_device *dev, pm_message_t state)
 {
 	struct net_device *ndev = platform_get_drvdata(dev);
-	struct fec_enet_private *fep;
+	struct fec_enet_private *fep = netdev_priv(ndev);
 
 	if (ndev) {
 		fep = netdev_priv(ndev);
 		if (netif_running(ndev)) {
+			netif_stop_queue(ndev);
 			netif_device_detach(ndev);
-			fec_stop(ndev);
+			clk_disable(fep->clk);
 		}
 	}
 	return 0;

@@ -1987,11 +1988,13 @@ static int
 fec_resume(struct platform_device *dev)
 {
 	struct net_device *ndev = platform_get_drvdata(dev);
+	struct fec_enet_private *fep = netdev_priv(ndev);
 
 	if (ndev) {
 		if (netif_running(ndev)) {
-			fec_enet_init(ndev, 0);
+			clk_enable(fep->clk);
 			netif_device_attach(ndev);
+			netif_start_queue(ndev);
 		}
 	}
 	return 0;

Signature · 1 line

1.6.3.3

Eric Bénard

author

2010-05-27 06:33 UTC #7 lore raw

PATCH v2 8/8 imxfb: fix PM

* if the screen is blanked, the clock is already disabled, so handle this case
* before this patch, the following log was triggered :

WARNING: at arch/arm/plat-mxc/clock.c:59 __clk_disable+0x68/0x74()
Modules linked in:
[<c002d200>] (unwind_backtrace+0x0/0xf8) from [<c0046008>] (warn_slowpath_common+0x48/0x78)
[<c0046008>] (warn_slowpath_common+0x48/0x78) from [<c0031e28>] (__clk_disable+0x68/0x74)
[<c0031e28>] (__clk_disable+0x68/0x74) from [<c0031ddc>] (__clk_disable+0x1c/0x74)
[<c0031ddc>] (__clk_disable+0x1c/0x74) from [<c0031e58>] (clk_disable+0x24/0x34)
[<c0031e58>] (clk_disable+0x24/0x34) from [<c019808c>] (imxfb_disable_controller+0x38/0x48)
[<c019808c>] (imxfb_disable_controller+0x38/0x48) from [<c01980b0>] (imxfb_suspend+0x14/0x1c)

Signed-off-by Eric B?nard <[email protected]>

---
 drivers/video/imxfb.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

drivers/video/imxfb.c +2 -2

index b4b6dec..6cbfbff 100644
@@ -597,8 +597,8 @@ static int imxfb_suspend(struct platform_device *dev, pm_message_t state)
 	struct imxfb_info *fbi = info->par;
 
 	pr_debug("%s\n", __func__);
-
-	imxfb_disable_controller(fbi);
+	if (readl(fbi->regs + LCDC_RMCR))
+		imxfb_disable_controller(fbi);
 	return 0;
 }

Signature · 1 line

1.6.3.3

Sergei Shtylyov

2010-05-27 10:37 UTC #8 lore raw

PATCH v2 7/8 net/fec: fix pm to survive to suspend/resume

In reply to Eric Bénard

Hello.

Quoted 12 lines · Eric B?nard

* with this patch, if the connection if active before suspend, it will be active after resume.
* before this patch, it was necessary to close the interface and reopen it to recover the connection.
Signed-off-by Eric B?nard <[email protected]>
drivers/net/fec.c +0 -0
index 9b4e8f7..654abaf 100644

drivers/net/fec.c static int · line 1974

Quoted 5 lines

drivers/net/fec.c +1 -1

@@ -1971,13 +1971,14 @@ static int
 fec_suspend(struct platform_device *dev, pm_message_t state)
 {
 	struct net_device *ndev = platform_get_drvdata(dev);
-	struct fec_enet_private *fep;
+	struct fec_enet_private *fep = netdev_priv(ndev);

Can netdev_priv() be called if 'ndev' is NULL (and can it be really
NULL?)?

drivers/net/fec.c static int · line 1977

Quoted 3 lines

drivers/net/fec.c +0 -0

 	if (ndev) {
 		fep = netdev_priv(ndev);

Duplicated now.

WBR, Sergei

Eric Bénard

author

2010-05-27 14:19 UTC #9 lore raw

PATCH v3 1/1 net/fec: fix pm to survive to suspend/resume

* with this patch, if the connection if active before suspend, it will be active after resume.
* before this patch, it was necessary to close the interface and reopen it to recover the connection.

Signed-off-by Eric B?nard <[email protected]>

---
v3 : fix following Sergei Shtylyov's comments

 drivers/net/fec.c |   10 +++++++---
 1 files changed, 7 insertions(+), 3 deletions(-)

drivers/net/fec.c +7 -3

index 9b4e8f7..1521972 100644
@@ -1974,10 +1974,11 @@ fec_suspend(struct platform_device *dev, pm_message_t state)
 	struct fec_enet_private *fep;
 
 	if (ndev) {
-		fep = netdev_priv(ndev);
 		if (netif_running(ndev)) {
+			fep = netdev_priv(ndev);
+			netif_stop_queue(ndev);
 			netif_device_detach(ndev);
-			fec_stop(ndev);
+			clk_disable(fep->clk);
 		}
 	}
 	return 0;

@@ -1987,11 +1988,14 @@ static int
 fec_resume(struct platform_device *dev)
 {
 	struct net_device *ndev = platform_get_drvdata(dev);
+	struct fec_enet_private *fep;
 
 	if (ndev) {
 		if (netif_running(ndev)) {
-			fec_enet_init(ndev, 0);
+			fep = netdev_priv(ndev);
+			clk_enable(fep->clk);
 			netif_device_attach(ndev);
+			netif_start_queue(ndev);
 		}
 	}
 	return 0;

Signature · 1 line

1.6.3.3

Fabio Estevam

2010-05-27 17:09 UTC #10 lore raw

PATCH v3 1/1 net/fec: fix pm to survive to suspend/resume

Eric,

drivers/net/fec.c

Quoted 23 lines · Eric B?nard

From: Eric B?nard <[email protected]>
Subject: [PATCH v3] net/fec: fix pm to survive to suspend/resume
To: linux-arm-kernel at lists.infradead.org
Cc: s.hauer at pengutronix.de, sshtylyov at mvista.com
Date: Thursday, May 27, 2010, 11:19 AM
* with this patch, if the connection
if active before suspend, it
will be active after resume.
* before this patch, it was necessary to close the interface and
reopen it to recover the connection.
Signed-off-by Eric B?nard <[email protected]>
---
v3 : fix following Sergei Shtylyov's comments
drivers/net/fec.c |???10 +++++++---
 1 files changed, 7 insertions(+), 3 deletions(-)
drivers/net/fec.c +0 -0
index 9b4e8f7..1521972 100644

I think you should have CC'ed the linux-netdev list also.

Regards,

Fabio Estevam

Eric Bénard

author

2010-05-27 17:53 UTC #11 lore raw

PATCH 1/1 net/fec: fix pm to survive to suspend/resume

* with this patch, if the connection if active before suspend, it will be active after resume.
* before this patch, it was necessary to close the interface and reopen it to recover the connection.

Signed-off-by Eric B?nard <[email protected]>

Cc s.hauer at pengutronix.de

Cc sshtylyov at mvista.com

Cc linux-arm-kernel at lists.infradead.org

Cc fabioestevam at yahoo.com

Cc netdev at vger.kernel.org

---
 drivers/net/fec.c |   10 +++++++---
 1 files changed, 7 insertions(+), 3 deletions(-)

drivers/net/fec.c +7 -3

index 9b4e8f7..1521972 100644
@@ -1974,10 +1974,11 @@ fec_suspend(struct platform_device *dev, pm_message_t state)
 	struct fec_enet_private *fep;
 
 	if (ndev) {
-		fep = netdev_priv(ndev);
 		if (netif_running(ndev)) {
+			fep = netdev_priv(ndev);
+			netif_stop_queue(ndev);
 			netif_device_detach(ndev);
-			fec_stop(ndev);
+			clk_disable(fep->clk);
 		}
 	}
 	return 0;

@@ -1987,11 +1988,14 @@ static int
 fec_resume(struct platform_device *dev)
 {
 	struct net_device *ndev = platform_get_drvdata(dev);
+	struct fec_enet_private *fep;
 
 	if (ndev) {
 		if (netif_running(ndev)) {
-			fec_enet_init(ndev, 0);
+			fep = netdev_priv(ndev);
+			clk_enable(fep->clk);
 			netif_device_attach(ndev);
+			netif_start_queue(ndev);
 		}
 	}
 	return 0;

Signature · 1 line

1.6.3.3

David Miller

maintainer

2010-05-29 07:15 UTC #12 lore raw

Re: [PATCH] net/fec: fix pm to survive to suspend/resume

From: Eric Bénard <[email protected]>
Date: Thu, 27 May 2010 19:53:48 +0200

Quoted 6 lines

* with this patch, if the connection if active before suspend, it will be active after resume.
* before this patch, it was necessary to close the interface and reopen it to recover the connection.
Signed-off-by Eric Bénard <[email protected]>

On resume the PHY should be reset and the link renegotiated (or set to a fixed speed/duplex if set by the user via ethtool) just like as happens on ->open().

You seem to be avoiding that here.

If the PHY is not functioning properly after the ->resume() triggered reset, fix that instead.

I'm not applying this patch, it doesn't look the right thing to do at all.

Eric Bénard

author

2010-05-29 20:04 UTC #13 lore raw

PATCH v2 1/1 net/fec: fix pm to survive to suspend/resume

* in the actual driver, calling fec_stop and fec_enet_init doesn't allow to have a working network interface at resume (where a ifconfig down and up is required to recover the interface)
* by using fec_enet_close and fec_enet_open, this patch solves this problem and handle the case where the link changed between suspend and resume
* this patch also disable clock at suspend and reenable it at resume

Signed-off-by Eric B?nard <[email protected]>

Cc s.hauer at pengutronix.de

Cc sshtylyov at mvista.com

Cc linux-arm-kernel at lists.infradead.org

Cc fabioestevam at yahoo.com

Cc davem at davemloft.net

---
 drivers/net/fec.c |   16 ++++++++--------
 1 files changed, 8 insertions(+), 8 deletions(-)

drivers/net/fec.c +8 -8

index 9b4e8f7..40ffdb8 100644
@@ -1975,10 +1975,9 @@ fec_suspend(struct platform_device *dev, pm_message_t state)
 
 	if (ndev) {
 		fep = netdev_priv(ndev);
-		if (netif_running(ndev)) {
-			netif_device_detach(ndev);
-			fec_stop(ndev);
-		}
+		if (netif_running(ndev))
+			fec_enet_close(ndev);
+		clk_disable(fep->clk);
 	}
 	return 0;
 }

@@ -1987,12 +1986,13 @@ static int
 fec_resume(struct platform_device *dev)
 {
 	struct net_device *ndev = platform_get_drvdata(dev);
+	struct fec_enet_private *fep;
 
 	if (ndev) {
-		if (netif_running(ndev)) {
-			fec_enet_init(ndev, 0);
-			netif_device_attach(ndev);
-		}
+		fep = netdev_priv(ndev);
+		clk_enable(fep->clk);
+		if (netif_running(ndev))
+			fec_enet_open(ndev);
 	}
 	return 0;
 }

Signature · 1 line

1.6.3.3

David Miller

maintainer Applied

2010-06-02 13:13 UTC #14 lore raw

Re: [PATCH v2] net/fec: fix pm to survive to suspend/resume

From: Eric Bénard <[email protected]>
Date: Sat, 29 May 2010 22:04:50 +0200

Quoted 9 lines

* in the actual driver, calling fec_stop and fec_enet_init doesn't allow to have a working network interface at resume (where a ifconfig down and up is required to recover the interface)
* by using fec_enet_close and fec_enet_open, this patch solves this problem and handle the case where the link changed between suspend and resume
* this patch also disable clock at suspend and reenable it at resume
Signed-off-by Eric Bénard <[email protected]>

Applied, thanks.

Alberto Panizzo

2010-05-29 12:23 UTC #15 lore raw

PATCH v2 3/8 imx_keypad: add PM support

In reply to Eric Bénard

Eric,

Quoted 91 lines · Eric B?nard

Signed-off-by Eric B?nard <[email protected]>

---
 drivers/input/keyboard/imx_keypad.c |   56 +++++++++++++++++++++++++++++++++-
 1 files changed, 54 insertions(+), 2 deletions(-)

drivers/input/keyboard/imx_keypad.c +52 -2

index d92c15c..b303cc9 100644
@@ -364,6 +364,55 @@ static void imx_keypad_inhibit(struct imx_keypad *keypad)
 	writew(0xff00, keypad->mmio_base + KPCR);
 }
 
+static int mxc_kpp_suspend(struct platform_device *pdev, pm_message_t state)
+{
+	struct imx_keypad *keypad = platform_get_drvdata(pdev);
+	unsigned short reg_val;
+
+	clk_disable(keypad->clk);
+
+	if (device_may_wakeup(&pdev->dev)) {
+		reg_val = readw(keypad->mmio_base + KPSR);
+		if ((reg_val & KBD_STAT_KDIE) == 0) {
+			/* If no depress interrupt enable the release interrupt */
+			reg_val |= KBD_STAT_KRIE;
+			writew(reg_val, keypad->mmio_base + KPSR);
+		}
+		enable_irq_wake(keypad->irq);
+	} else {
+		keypad->enabled = false;
+		synchronize_irq(keypad->irq);
+		disable_irq(keypad->irq);
+	}
+
+	return 0;
+}
+
+/*!
+ * This function brings the Keypad controller back from low-power state.
+ * If Keypad is enabled as a wake source(i.e. it can resume the system
+ * from suspend mode), the Keypad controller doesn't enter low-power state.
+ *
+ * @param   pdev  the device structure used to give information on Keypad
+ *                to resume
+ *
+ * @return  The function always returns 0.
+ */
+static int mxc_kpp_resume(struct platform_device *pdev)
+{
+	struct imx_keypad *keypad = platform_get_drvdata(pdev);
+
+	clk_enable(keypad->clk);
+	if (device_may_wakeup(&pdev->dev)) {
+		/* The irq routine already cleared KRIE if it was set */
+	} else {
+		keypad->enabled = true;
+		enable_irq(keypad->irq);
+	}
+
+	return 0;
+}
+
 static void imx_keypad_close(struct input_dev *dev)
 {
 	struct imx_keypad *keypad = input_get_drvdata(dev);

@@ -410,7 +459,8 @@ open_err:
 
 static int __devinit imx_keypad_probe(struct platform_device *pdev)
 {
-	const struct matrix_keymap_data *keymap_data = pdev->dev.platform_data;
+	const struct matrix_keypad_platform_data *keypad_data = pdev->dev.platform_data;
+	const struct matrix_keymap_data *keymap_data = keypad_data->keymap_data;
 	struct imx_keypad *keypad;
 	struct input_dev *input_dev;
 	struct resource *res;

@@ -525,7 +575,7 @@ static int __devinit imx_keypad_probe(struct platform_device *pdev)
 	}
 
 	platform_set_drvdata(pdev, keypad);
-	device_init_wakeup(&pdev->dev, 1);
+	device_init_wakeup(&pdev->dev, keypad_data->wakeup);
 
 	return 0;

drivers/input/keyboard/imx_keypad.c static struct platform_driver imx_keypad_driver = { · line 631

Quoted 8 lines

drivers/input/keyboard/imx_keypad.c +2 -0

@@ -574,6 +624,8 @@ static struct platform_driver imx_keypad_driver = {
 	},
 	.probe		= imx_keypad_probe,
 	.remove		= __devexit_p(imx_keypad_remove),
+	.suspend = mxc_kpp_suspend,
+	.resume = mxc_kpp_resume,
 };
 
 static int __init imx_keypad_init(void)

This patch need to follow the path through linux-input.

Generally, to know what are the addresses to add in CC when you send a patch you can do as follow:

Quoted 1 line

From a patch:

linux_root$ ./scripts/get_maintainer.pl patch_name.patch

Quoted 1 line

From a file:

linux_root$ ./scripts/get_maintainer.pl -f drivers/input/keyboard/imx_keypad.c

The result contains: right mailing lists, the maintainers of the touched subsystems and all the people that had contribute on the files touched by the patch.

Best regards

Signature · 4 lines

Alberto!

        Be Persistent!
                - Greg Kroah-Hartman (FOSDEM 2010)

Eric Bénard

author

2010-05-30 08:51 UTC #16 lore raw

PATCH 1/1 imx_keypad: add PM support

Signed-off-by Eric B?nard <[email protected]>

---
 drivers/input/keyboard/imx_keypad.c |   56 +++++++++++++++++++++++++++++++++-
 1 files changed, 54 insertions(+), 2 deletions(-)

drivers/input/keyboard/imx_keypad.c +54 -2

index d92c15c..b303cc9 100644
@@ -364,6 +364,55 @@ static void imx_keypad_inhibit(struct imx_keypad *keypad)
 	writew(0xff00, keypad->mmio_base + KPCR);
 }
 
+static int mxc_kpp_suspend(struct platform_device *pdev, pm_message_t state)
+{
+	struct imx_keypad *keypad = platform_get_drvdata(pdev);
+	unsigned short reg_val;
+
+	clk_disable(keypad->clk);
+
+	if (device_may_wakeup(&pdev->dev)) {
+		reg_val = readw(keypad->mmio_base + KPSR);
+		if ((reg_val & KBD_STAT_KDIE) == 0) {
+			/* If no depress interrupt enable the release interrupt */
+			reg_val |= KBD_STAT_KRIE;
+			writew(reg_val, keypad->mmio_base + KPSR);
+		}
+		enable_irq_wake(keypad->irq);
+	} else {
+		keypad->enabled = false;
+		synchronize_irq(keypad->irq);
+		disable_irq(keypad->irq);
+	}
+
+	return 0;
+}
+
+/*!
+ * This function brings the Keypad controller back from low-power state.
+ * If Keypad is enabled as a wake source(i.e. it can resume the system
+ * from suspend mode), the Keypad controller doesn't enter low-power state.
+ *
+ * @param   pdev  the device structure used to give information on Keypad
+ *                to resume
+ *
+ * @return  The function always returns 0.
+ */
+static int mxc_kpp_resume(struct platform_device *pdev)
+{
+	struct imx_keypad *keypad = platform_get_drvdata(pdev);
+
+	clk_enable(keypad->clk);
+	if (device_may_wakeup(&pdev->dev)) {
+		/* The irq routine already cleared KRIE if it was set */
+	} else {
+		keypad->enabled = true;
+		enable_irq(keypad->irq);
+	}
+
+	return 0;
+}
+
 static void imx_keypad_close(struct input_dev *dev)
 {
 	struct imx_keypad *keypad = input_get_drvdata(dev);

@@ -410,7 +459,8 @@ open_err:
 
 static int __devinit imx_keypad_probe(struct platform_device *pdev)
 {
-	const struct matrix_keymap_data *keymap_data = pdev->dev.platform_data;
+	const struct matrix_keypad_platform_data *keypad_data = pdev->dev.platform_data;
+	const struct matrix_keymap_data *keymap_data = keypad_data->keymap_data;
 	struct imx_keypad *keypad;
 	struct input_dev *input_dev;
 	struct resource *res;

@@ -525,7 +575,7 @@ static int __devinit imx_keypad_probe(struct platform_device *pdev)
 	}
 
 	platform_set_drvdata(pdev, keypad);
-	device_init_wakeup(&pdev->dev, 1);
+	device_init_wakeup(&pdev->dev, keypad_data->wakeup);
 
 	return 0;

@@ -574,6 +624,8 @@ static struct platform_driver imx_keypad_driver = {
 	},
 	.probe		= imx_keypad_probe,
 	.remove		= __devexit_p(imx_keypad_remove),
+	.suspend = mxc_kpp_suspend,
+	.resume = mxc_kpp_resume,
 };
 
 static int __init imx_keypad_init(void)

Signature · 1 line

1.6.3.3

Dmitry Torokhov

maintainer

2010-06-02 08:02 UTC #17 lore raw

PATCH 1/1 imx_keypad: add PM support

Hi Eric,

Quoted 28 lines · Eric B?nard

* add suspend/resume support
* use keypad_data to enable wakeup from platform data
* suspend/resume tested when keypad is not in use and when it's is use (in which case we immediatly get the key pressed for wakeup)
Signed-off-by Eric B?nard <[email protected]>
---
 drivers/input/keyboard/imx_keypad.c |   56 +++++++++++++++++++++++++++++++++-
 1 files changed, 54 insertions(+), 2 deletions(-)
drivers/input/keyboard/imx_keypad.c +9 -0
index d92c15c..b303cc9 100644
@@ -364,6 +364,55 @@ static void imx_keypad_inhibit(struct imx_keypad *keypad)
 	writew(0xff00, keypad->mmio_base + KPCR);
 }
 
+static int mxc_kpp_suspend(struct platform_device *pdev, pm_message_t state)
+{
+	struct imx_keypad *keypad = platform_get_drvdata(pdev);
+	unsigned short reg_val;
+
+	clk_disable(keypad->clk);
+
+	if (device_may_wakeup(&pdev->dev)) {
+		reg_val = readw(keypad->mmio_base + KPSR);

drivers/input/keyboard/imx_keypad.c static void imx_keypad_inhibit(struct imx_keypad *keypad) · line 385

Quoted 10 lines

drivers/input/keyboard/imx_keypad.c +10 -0

+		if ((reg_val & KBD_STAT_KDIE) == 0) {
+			/* If no depress interrupt enable the release interrupt */
+			reg_val |= KBD_STAT_KRIE;
+			writew(reg_val, keypad->mmio_base + KPSR);
+		}
+		enable_irq_wake(keypad->irq);
+	} else {
+		keypad->enabled = false;
+		synchronize_irq(keypad->irq);
+		disable_irq(keypad->irq);

I'd factor it out as it is used in both suspend and close.

Does everythig (clk_disable, eit) work well in case when device is still closed when it get ssuspended?

Also, do you really need such an elaborate setup when device is/is not wakeup source? Do you really loose keypress if you always dosable IRQ in suspend and enable it in resume?

Quoted 5 lines

drivers/input/keyboard/imx_keypad.c +5 -0
+	}
+
+	return 0;
+}
+

drivers/input/keyboard/imx_keypad.c static void imx_keypad_inhibit(struct imx_keypad *keypad) · line 400

Quoted 10 lines

drivers/input/keyboard/imx_keypad.c +10 -0

+/*!
+ * This function brings the Keypad controller back from low-power state.
+ * If Keypad is enabled as a wake source(i.e. it can resume the system
+ * from suspend mode), the Keypad controller doesn't enter low-power state.
+ *
+ * @param   pdev  the device structure used to give information on Keypad
+ *                to resume
+ *
+ * @return  The function always returns 0.
+ */

This is not the standard kerneldoc markup.

Quoted 38 lines

drivers/input/keyboard/imx_keypad.c +18 -2

+static int mxc_kpp_resume(struct platform_device *pdev)
+{
+	struct imx_keypad *keypad = platform_get_drvdata(pdev);
+
+	clk_enable(keypad->clk);
+	if (device_may_wakeup(&pdev->dev)) {
+		/* The irq routine already cleared KRIE if it was set */
+	} else {
+		keypad->enabled = true;
+		enable_irq(keypad->irq);
+	}
+
+	return 0;
+}
+
 static void imx_keypad_close(struct input_dev *dev)
 {
 	struct imx_keypad *keypad = input_get_drvdata(dev);

@@ -410,7 +459,8 @@ open_err:
 
 static int __devinit imx_keypad_probe(struct platform_device *pdev)
 {
-	const struct matrix_keymap_data *keymap_data = pdev->dev.platform_data;
+	const struct matrix_keypad_platform_data *keypad_data = pdev->dev.platform_data;
+	const struct matrix_keymap_data *keymap_data = keypad_data->keymap_data;
 	struct imx_keypad *keypad;
 	struct input_dev *input_dev;
 	struct resource *res;

@@ -525,7 +575,7 @@ static int __devinit imx_keypad_probe(struct platform_device *pdev)
 	}
 
 	platform_set_drvdata(pdev, keypad);
-	device_init_wakeup(&pdev->dev, 1);
+	device_init_wakeup(&pdev->dev, keypad_data->wakeup);
 
 	return 0;

drivers/input/keyboard/imx_keypad.c static struct platform_driver imx_keypad_driver = { · line 628

Quoted 5 lines

drivers/input/keyboard/imx_keypad.c +2 -0

@@ -574,6 +624,8 @@ static struct platform_driver imx_keypad_driver = {
 	},
 	.probe		= imx_keypad_probe,
 	.remove		= __devexit_p(imx_keypad_remove),
+	.suspend = mxc_kpp_suspend,
+	.resume = mxc_kpp_resume,

Please use pm_ops.

Thanks.

Signature · 1 line

Dmitry

Eric Bénard

author

2010-05-27 17:55 UTC #18 lore raw

PATCH 1/1 mxc_nand: fix PM

In reply to Eric Bénard

Signed-off-by Eric B?nard <[email protected]>

Cc s.hauer at pengutronix.de

Cc linux-arm-kernel at lists.infradead.org

Cc linux-mtd at lists.infradead.org

---
 drivers/mtd/nand/mxc_nand.c |   44 +------------------------------------------
 1 files changed, 1 insertions(+), 43 deletions(-)

drivers/mtd/nand/mxc_nand.c +1 ~~-43~~

index 314e2bf..217b247 100644
@@ -892,53 +892,11 @@ static int __devexit mxcnd_remove(struct platform_device *pdev)
 	return 0;
 }
 
-#ifdef CONFIG_PM
-static int mxcnd_suspend(struct platform_device *pdev, pm_message_t state)
-{
-	struct mtd_info *mtd = platform_get_drvdata(pdev);
-	struct nand_chip *nand_chip = mtd->priv;
-	struct mxc_nand_host *host = nand_chip->priv;
-	int ret = 0;
-
-	DEBUG(MTD_DEBUG_LEVEL0, "MXC_ND : NAND suspend\n");
-
-	ret = mtd->suspend(mtd);
-
-	/*
-	 * nand_suspend locks the device for exclusive access, so
-	 * the clock must already be off.
-	 */
-	BUG_ON(!ret && host->clk_act);
-
-	return ret;
-}
-
-static int mxcnd_resume(struct platform_device *pdev)
-{
-	struct mtd_info *mtd = platform_get_drvdata(pdev);
-	struct nand_chip *nand_chip = mtd->priv;
-	struct mxc_nand_host *host = nand_chip->priv;
-	int ret = 0;
-
-	DEBUG(MTD_DEBUG_LEVEL0, "MXC_ND : NAND resume\n");
-
-	mtd->resume(mtd);
-
-	return ret;
-}
-
-#else
-# define mxcnd_suspend   NULL
-# define mxcnd_resume    NULL
-#endif				/* CONFIG_PM */
-
 static struct platform_driver mxcnd_driver = {
 	.driver = {
 		   .name = DRIVER_NAME,
-		   },
+	},
 	.remove = __devexit_p(mxcnd_remove),
-	.suspend = mxcnd_suspend,
-	.resume = mxcnd_resume,
 };
 
 static int __init mxc_nd_init(void)

Signature · 1 line

1.6.3.3

Eric Bénard

author

2010-06-14 07:07 UTC #19 lore raw

PATCH 1/1 mxc_nand: fix PM

Hi Sascha,

Le 27/05/2010 19:55, Eric B?nard a ?crit :

Quoted 9 lines

* when resuming, we get the following log :
nand_resume called for a chip which is not in suspended state
* this happens because mtd->suspend and mtd->resume are already called from the mtd layer, thus they were called a second time in mxc_nand so mtd complains when trying to resume for the second time
* the fix is to remove these call and thus remove the supend and resume functions of mxc_nand

any news concerning this patch ?

Eric

Quoted 67 lines

Signed-off-by Eric B?nard<[email protected]>

Cc s.hauer at pengutronix.de

Cc linux-arm-kernel at lists.infradead.org

Cc linux-mtd at lists.infradead.org

---
  drivers/mtd/nand/mxc_nand.c |   44 +------------------------------------------
  1 files changed, 1 insertions(+), 43 deletions(-)

drivers/mtd/nand/mxc_nand.c +1 ~~-43~~

index 314e2bf..217b247 100644
@@ -892,53 +892,11 @@ static int __devexit mxcnd_remove(struct platform_device *pdev)
  	return 0;
  }
 
-#ifdef CONFIG_PM
-static int mxcnd_suspend(struct platform_device *pdev, pm_message_t state)
-{
-	struct mtd_info *mtd = platform_get_drvdata(pdev);
-	struct nand_chip *nand_chip = mtd->priv;
-	struct mxc_nand_host *host = nand_chip->priv;
-	int ret = 0;
-
-	DEBUG(MTD_DEBUG_LEVEL0, "MXC_ND : NAND suspend\n");
-
-	ret = mtd->suspend(mtd);
-
-	/*
-	 * nand_suspend locks the device for exclusive access, so
-	 * the clock must already be off.
-	 */
-	BUG_ON(!ret&&  host->clk_act);
-
-	return ret;
-}
-
-static int mxcnd_resume(struct platform_device *pdev)
-{
-	struct mtd_info *mtd = platform_get_drvdata(pdev);
-	struct nand_chip *nand_chip = mtd->priv;
-	struct mxc_nand_host *host = nand_chip->priv;
-	int ret = 0;
-
-	DEBUG(MTD_DEBUG_LEVEL0, "MXC_ND : NAND resume\n");
-
-	mtd->resume(mtd);
-
-	return ret;
-}
-
-#else
-# define mxcnd_suspend   NULL
-# define mxcnd_resume    NULL
-#endif				/* CONFIG_PM */
-
  static struct platform_driver mxcnd_driver = {
  	.driver = {
  		   .name = DRIVER_NAME,
-		   },
+	},
  	.remove = __devexit_p(mxcnd_remove),
-	.suspend = mxcnd_suspend,
-	.resume = mxcnd_resume,
  };
 
  static int __init mxc_nd_init(void)

Artem Bityutskiy

Applied

2010-06-29 06:41 UTC #20 lore raw

PATCH 1/1 mxc_nand: fix PM

Quoted 12 lines · Eric B?nard

Hi Sascha,
Le 27/05/2010 19:55, Eric B?nard a ?crit :
Quoted 9 lines
* when resuming, we get the following log :
nand_resume called for a chip which is not in suspended state
* this happens because mtd->suspend and mtd->resume are already called from the mtd layer, thus they were called a second time in mxc_nand so mtd complains when trying to resume for the second time
* the fix is to remove these call and thus remove the supend and resume functions of mxc_nand

Quoted 2 lines

any news concerning this patch ?

Pushed to l2-mtd-2.6.git / dunno

Signature · 2 lines

Best Regards,
Artem Bityutskiy (????? ????????)

Artem Bityutskiy

Applied

2010-07-01 03:47 UTC #21 lore raw

PATCH 1/1 mxc_nand: fix PM

In reply to Eric Bénard

Quoted 12 lines · Eric B?nard

Hi Sascha,
Le 27/05/2010 19:55, Eric B?nard a ?crit :
Quoted 9 lines
* when resuming, we get the following log :
nand_resume called for a chip which is not in suspended state
* this happens because mtd->suspend and mtd->resume are already called from the mtd layer, thus they were called a second time in mxc_nand so mtd complains when trying to resume for the second time
* the fix is to remove these call and thus remove the supend and resume functions of mxc_nand

Quoted 1 line

any news concerning this patch ?

Pushed to l2-mtd-2.6.git / dunno

Signature · 2 lines

Best Regards,
Artem Bityutskiy (????? ????????)

Uwe Kleine-König

2010-06-14 08:43 UTC #22 lore raw

PATCH 1/1 mxc_nand: fix PM

In reply to Eric Bénard

Hello Eric,

Quoted 8 lines · Eric B?nard

* when resuming, we get the following log :
nand_resume called for a chip which is not in suspended state
* this happens because mtd->suspend and mtd->resume are already called from the mtd layer, thus they were called a second time in mxc_nand so mtd complains when trying to resume for the second time
* the fix is to remove these call and thus remove the supend and resume functions of mxc_nand

the commit log looks IMHO ugly. Better make it (including Subject):

mtd/mxc_nand: don't call mtd->{suspend,resume} explicitly

The mtd layer is responsible to call mtd->{suspend,resume}. Doing it again in the driver is wrong and results in a warning:

nand_resume called for a chip which is not in suspended state

at resume time. Removing the calls from the resume and suspend functions makes them empty allowing them to be deleted completely.

Signed-off-by Eric B?nard <[email protected]>

Acked-by Uwe Kleine-K?nig <[email protected]>

Cc Sascha Hauer <[email protected]>

Cc linux-arm-kernel at lists.infradead.org

Cc linux-mtd at lists.infradead.org

Best regards
Uwe

Signature · 2 lines

Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Eric Bénard

author

2010-05-27 14:13 UTC #23 lore raw

PATCH v3 1/1 i.MX27: add suspend to RAM support

In reply to Eric Bénard

Signed-off-by Eric B?nard <[email protected]>

---
v3 :
return -EINVAL instead of -1

 arch/arm/mach-mx2/Makefile   |    2 +-
 arch/arm/mach-mx2/pm-imx27.c |   46 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 47 insertions(+), 1 deletions(-)
 create mode 100644 arch/arm/mach-mx2/pm-imx27.c

arch/arm/mach-mx2/Makefile +1 -1

index e3254fa..27d496c 100644
@@ -8,7 +8,7 @@ obj-y	:=  devices.o serial.o
 
 obj-$(CONFIG_MACH_MX21) += clock_imx21.o mm-imx21.o
 
-obj-$(CONFIG_MACH_MX27) += cpu_imx27.o
+obj-$(CONFIG_MACH_MX27) += cpu_imx27.o pm-imx27.o
 obj-$(CONFIG_MACH_MX27) += clock_imx27.o mm-imx27.o
 
 obj-$(CONFIG_MACH_MX21ADS) += mach-mx21ads.o

arch/arm/mach-mx2/pm-imx27.c new file +46 -0

46 lines not on this page · raw message

Signature · 1 line

1.6.3.3

Russell King - ARM Linux

2010-05-27 19:43 UTC #24 lore raw

PATCH v3 1/1 i.MX27: add suspend to RAM support

arch/arm/mach-mx2/pm-imx27.c line 12

Quoted 3 lines · Eric B?nard

arch/arm/mach-mx2/pm-imx27.c new file +3 -0
+#include <linux/kernel.h>
+#include <linux/suspend.h>
+#include <asm/io.h>

linux/io.h

(checkpatch.pl will tell you this, although the general rule that "if there's a linux/foo.h and an asm/foo.h, linux/foo.h should be used" doesn't always apply - there are some exceptions.)

Eric Bénard

author

2010-05-27 20:06 UTC #25 lore raw

PATCH v3 1/1 i.MX27: add suspend to RAM support

Hi Russell,

Le 27/05/2010 21:43, Russell King - ARM Linux a ?crit :

Quoted 7 lines · Eric B?nard

Review
Quoted 3 lines
(quoted diff) +3 -0
+#include<linux/kernel.h>
+#include<linux/suspend.h>
+#include<asm/io.h>
linux/io.h

OK sorry for this one, you already told me this at the first review and I forget to fix it.

Quoted 4 lines

(checkpatch.pl will tell you this, although the general rule that "if there's a linux/foo.h and an asm/foo.h, linux/foo.h should be used" doesn't always apply - there are some exceptions.)

In fact checkpatch only returned the following warning (which I didn't fixed as I didn't found any platform_suspend_ops which is const). Do you think I should also fix this warning ?

./linux-2.6/scripts/checkpatch.pl 0001-i.MX27-add-suspend-to-RAM-support.patch
WARNING: struct platform_suspend_ops should normally be const #72: FILE: arch/arm/mach-mx2/pm-imx27.c:35:
+static struct platform_suspend_ops mx27_suspend_ops = {

total: 0 errors, 1 warnings, 54 lines checked

0001-i.MX27-add-suspend-to-RAM-support.patch has style problems, please review. If any of these errors
are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS.

Eric

Russell King - ARM Linux

2010-05-27 20:16 UTC #26 lore raw

PATCH v3 1/1 i.MX27: add suspend to RAM support

Quoted 4 lines · Eric B?nard

Le 27/05/2010 21:43, Russell King - ARM Linux a ?crit :
Quoted 1 line · Eric B?nard
OK sorry for this one, you already told me this at the first review and I forget to fix it.

np.

Quoted 6 lines

Quoted 4 lines
(checkpatch.pl will tell you this, although the general rule that "if there's a linux/foo.h and an asm/foo.h, linux/foo.h should be used" doesn't always apply - there are some exceptions.)
In fact checkpatch only returned the following warning (which I didn't fixed as I didn't found any platform_suspend_ops which is const).

It seems to be classified as a "--subjective" check - I suspect because there's some variance between arch maintainers on their policy on this issue, and also because there are some exceptions as I've said above.

Eg, we have asm/memory.h, and there is a linux/memory.h, but the two are completely unrelated - so the checkpatch warnings about "use linux/memory.h rather than asm/memory.h" are rather misleading.

Quoted 7 lines

Do you think I should also fix this warning ?
./linux-2.6/scripts/checkpatch.pl 0001-i.MX27-add-suspend-to-RAM-support.patch
WARNING: struct platform_suspend_ops should normally be const #72: FILE: arch/arm/mach-mx2/pm-imx27.c:35:
+static struct platform_suspend_ops mx27_suspend_ops = {

It's trivial enough to fix, and doesn't cause any issues by doing so, so I'd suggest adding the const.

Eric Bénard

author

2010-05-27 20:48 UTC #27 lore raw

PATCH v4 1/1 i.MX27: add suspend to RAM support

Signed-off-by Eric B?nard <[email protected]>

---
v4:
fixed include + const struct
v3:
return -EINVAL instead of -1

 arch/arm/mach-mx2/Makefile   |    2 +-
 arch/arm/mach-mx2/pm-imx27.c |   46 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 47 insertions(+), 1 deletions(-)
 create mode 100644 arch/arm/mach-mx2/pm-imx27.c

arch/arm/mach-mx2/Makefile +1 -1

index e3254fa..27d496c 100644
@@ -8,7 +8,7 @@ obj-y	:=  devices.o serial.o
 
 obj-$(CONFIG_MACH_MX21) += clock_imx21.o mm-imx21.o
 
-obj-$(CONFIG_MACH_MX27) += cpu_imx27.o
+obj-$(CONFIG_MACH_MX27) += cpu_imx27.o pm-imx27.o
 obj-$(CONFIG_MACH_MX27) += clock_imx27.o mm-imx27.o
 
 obj-$(CONFIG_MACH_MX21ADS) += mach-mx21ads.o

arch/arm/mach-mx2/pm-imx27.c new file +46 -0

46 lines not on this page · raw message

Signature · 1 line

1.6.3.3

Eric Bénard

author

2010-05-27 20:54 UTC #28 lore raw

PATCH v3 1/1 i.MX27: add suspend to RAM support

In reply to Russell King - ARM Linux

Le 27/05/2010 22:16, Russell King - ARM Linux a ?crit :

Quoted 12 lines · Eric B?nard

Quoted 7 lines
Do you think I should also fix this warning ?
./linux-2.6/scripts/checkpatch.pl 0001-i.MX27-add-suspend-to-RAM-support.patch
WARNING: struct platform_suspend_ops should normally be const #72: FILE: arch/arm/mach-mx2/pm-imx27.c:35:
+static struct platform_suspend_ops mx27_suspend_ops = {
It's trivial enough to fix, and doesn't cause any issues by doing so, so I'd suggest adding the const.

well now this triggers the following warning so forget the patch v4, checkpatch seems wrong on this case :
CC arch/arm/mach-mx2/pm-imx27.o
arch/arm/mach-mx2/pm-imx27.c: In function 'mx27_pm_init':
arch/arm/mach-mx2/pm-imx27.c:42: warning: passing argument 1 of 'suspend_set_ops' discards qualifiers from pointer target type

Eric

Russell King - ARM Linux

2010-05-27 21:46 UTC #29 lore raw

PATCH v3 1/1 i.MX27: add suspend to RAM support

Quoted 13 lines · Eric B?nard

Le 27/05/2010 22:16, Russell King - ARM Linux a ?crit :
Quoted 12 lines · Eric B?nard
Quoted 7 lines
Do you think I should also fix this warning ?
./linux-2.6/scripts/checkpatch.pl 0001-i.MX27-add-suspend-to-RAM-support.patch
WARNING: struct platform_suspend_ops should normally be const #72: FILE: arch/arm/mach-mx2/pm-imx27.c:35:
+static struct platform_suspend_ops mx27_suspend_ops = {
It's trivial enough to fix, and doesn't cause any issues by doing so, so I'd suggest adding the const.

Quoted 6 lines

well now this triggers the following warning so forget the patch v4, checkpatch seems wrong on this case :
CC arch/arm/mach-mx2/pm-imx27.o
arch/arm/mach-mx2/pm-imx27.c: In function 'mx27_pm_init':
arch/arm/mach-mx2/pm-imx27.c:42: warning: passing argument 1 of 'suspend_set_ops' discards qualifiers from pointer target type

Hmm, seems that checkpatch is incorrect on this point then. include/linux/suspend.h has:

extern void suspend_set_ops(struct platform_suspend_ops *ops); static inline void suspend_set_ops(struct platform_suspend_ops *ops) {}

and I can't find any suggestion that these ever took a const pointer.

Eric Bénard

author

2010-06-01 19:57 UTC #30 lore raw

PATCH v5 1/1 i.MX27: add suspend to RAM support

Signed-off-by Eric B?nard <[email protected]>

---
v5:
remove const struct
v4:
fixed include + const struct
v3:
return -EINVAL instead of -1

 arch/arm/mach-mx2/Makefile   |    2 +-
 arch/arm/mach-mx2/pm-imx27.c |   46 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 47 insertions(+), 1 deletions(-)
 create mode 100644 arch/arm/mach-mx2/pm-imx27.c

arch/arm/mach-mx2/Makefile +1 -1

index e3254fa..27d496c 100644
@@ -8,7 +8,7 @@ obj-y	:=  devices.o serial.o
 
 obj-$(CONFIG_MACH_MX21) += clock_imx21.o mm-imx21.o
 
-obj-$(CONFIG_MACH_MX27) += cpu_imx27.o
+obj-$(CONFIG_MACH_MX27) += cpu_imx27.o pm-imx27.o
 obj-$(CONFIG_MACH_MX27) += clock_imx27.o mm-imx27.o
 
 obj-$(CONFIG_MACH_MX21ADS) += mach-mx21ads.o

arch/arm/mach-mx2/pm-imx27.c new file +46 -0

46 lines not on this page · raw message

Signature · 1 line

1.6.3.3

Sascha Hauer

maintainer

2010-06-03 08:28 UTC #31 lore raw

PATCH v5 1/1 i.MX27: add suspend to RAM support

Quoted 9 lines · Eric B?nard

Signed-off-by Eric B?nard <[email protected]>
---
v5:
remove const struct
v4:
fixed include + const struct
v3:
return -EINVAL instead of -1

Quoted 4 lines

 arch/arm/mach-mx2/Makefile   |    2 +-
 arch/arm/mach-mx2/pm-imx27.c |   46 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 47 insertions(+), 1 deletions(-)
 create mode 100644 arch/arm/mach-mx2/pm-imx27.c

Ok, applied

Sascha

Signature · 4 lines

Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Wolfram Sang

2010-05-27 21:13 UTC #32 lore raw

PATCH v3 1/1 i.MX27: add suspend to RAM support

In reply to Russell King - ARM Linux

Quoted 3 lines

Eg, we have asm/memory.h, and there is a linux/memory.h, but the two are completely unrelated - so the checkpatch warnings about "use linux/memory.h rather than asm/memory.h" are rather misleading.

checkpatch can blacklist .h-files for this warning (as it does for irq.h at the moment). Do you think adding memory.h there is worthwhile? I'd prepare a patch, then.

Signature · 9 lines

Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20100527/86c6e21d/attachment.sig>