[PATCH v3 0/3] GPIO driver to turn power off

[PATCH v3 0/3] GPIO driver to turn power off

[Andrew Lunn]

A few of the Kirkwood systems use a GPIO line to turn the power off on
shutdown. They request the GPIO line in the board setup code,
optionally setting it as an output and driving it inactive. They then
register a function for pm_power_off. At the end of the shutdown, this
function is called. The GPIO line is configured as an output, if not
already so, and then driven active. In order to cover the use case of
edges rather then levels, triggering power off, the GPIO line is then
toggled active and later inactive. This should allow the driver to be
used by PXA which also has boards using the same scheme.

The driver code was initially developed by Jamie Lentin and extended
to cover the PXA case by Andrew Lunn.

v1 -> v2: Moved to drivers/power/reset
v2 -> v3: Fixed polarity of IB62x0 GPIO
          Increased delay before WARN if the power does not go off.

Andrew Lunn (2):
  ARM: Kirkwood: Convert DNSKW to use gpio-poweroff.
  ARM: Kirkwood: Convert IB62x0 to use gpio-poweroff.

Jamie Lentin (1):
  power: Add simple poweroff-gpio driver

 .../devicetree/bindings/gpio/gpio-poweroff.txt     |   22 ++++
 arch/arm/boot/dts/kirkwood-dnskw.dtsi              |    5 +
 arch/arm/boot/dts/kirkwood-ib62x0.dts              |    6 +
 arch/arm/mach-kirkwood/Kconfig                     |    4 +
 arch/arm/mach-kirkwood/board-dnskw.c               |   12 --
 arch/arm/mach-kirkwood/board-ib62x0.c              |   13 --
 drivers/power/Kconfig                              |    3 +
 drivers/power/Makefile                             |    1 +
 drivers/power/reset/Kconfig                        |   15 +++
 drivers/power/reset/Makefile                       |    1 +
 drivers/power/reset/gpio-poweroff.c                |  129 ++++++++++++++++++++
 11 files changed, 186 insertions(+), 25 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/gpio/gpio-poweroff.txt
 create mode 100644 drivers/power/reset/Kconfig
 create mode 100644 drivers/power/reset/Makefile
 create mode 100644 drivers/power/reset/gpio-poweroff.c

-- 
1.7.10.4

[PATCH v3 1/3] power: Add simple poweroff-gpio driver

[Andrew Lunn]

From: Jamie Lentin <jm-Pj/HzkgeCk7QXOPxS62xeg@public.gmane.org>

Given appropriate devicetree bindings, this driver registers a
pm_power_off function to set a GPIO line high/low to power down
your board.

Signed-off-by: Jamie Lentin <jm-Pj/HzkgeCk7QXOPxS62xeg@public.gmane.org>
Signed-off-by: Andrew Lunn <andrew-g2DYL2Zd6BY@public.gmane.org>
Tested-by:Simon Baatz <gmbnomis-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 .../devicetree/bindings/gpio/gpio-poweroff.txt     |   22 ++++
 drivers/power/Kconfig                              |    3 +
 drivers/power/Makefile                             |    1 +
 drivers/power/reset/Kconfig                        |   15 +++
 drivers/power/reset/Makefile                       |    1 +
 drivers/power/reset/gpio-poweroff.c                |  129 ++++++++++++++++++++
 6 files changed, 171 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/gpio-poweroff.txt
 create mode 100644 drivers/power/reset/Kconfig
 create mode 100644 drivers/power/reset/Makefile
 create mode 100644 drivers/power/reset/gpio-poweroff.c

diff --git a/Documentation/devicetree/bindings/gpio/gpio-poweroff.txt b/Documentation/devicetree/bindings/gpio/gpio-poweroff.txt
new file mode 100644
index 0000000..558cdf3
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/gpio-poweroff.txt
@@ -0,0 +1,22 @@
+GPIO line that should be set high/low to power off a device
+
+Required properties:
+- compatible : should be "gpio-poweroff".
+- gpios : The GPIO to set high/low, see "gpios property" in
+  Documentation/devicetree/bindings/gpio/gpio.txt. If the pin should be
+  low to power down the board set it to "Active Low", otherwise set
+  gpio to "Active High".
+
+Optional properties:
+- input : Initially configure the GPIO line as an input. Only reconfigure
+  it to an output when the pm_power_off function is called. If this optional
+  property is not specified, the GPIO is initialized as an output in its
+  inactive state.
+
+
+Examples:
+
+gpio-poweroff {
+	compatible = "gpio-poweroff";
+	gpios = <&gpio 4 0>; /* GPIO 4 Active Low */
+};
diff --git a/drivers/power/Kconfig b/drivers/power/Kconfig
index 49a8939..b1d956d 100644
--- a/drivers/power/Kconfig
+++ b/drivers/power/Kconfig
@@ -335,6 +335,9 @@ config AB8500_BATTERY_THERM_ON_BATCTRL
 	help
 	  Say Y to enable battery temperature measurements using
 	  thermistor connected on BATCTRL ADC.
+
+source "drivers/power/reset/Kconfig"
+
 endif # POWER_SUPPLY
 
 source "drivers/power/avs/Kconfig"
diff --git a/drivers/power/Makefile b/drivers/power/Makefile
index b949cf8..f1d99f4 100644
--- a/drivers/power/Makefile
+++ b/drivers/power/Makefile
@@ -49,3 +49,4 @@ obj-$(CONFIG_CHARGER_MAX8997)	+= max8997_charger.o
 obj-$(CONFIG_CHARGER_MAX8998)	+= max8998_charger.o
 obj-$(CONFIG_POWER_AVS)		+= avs/
 obj-$(CONFIG_CHARGER_SMB347)	+= smb347-charger.o
+obj-$(CONFIG_POWER_RESET)	+= reset/
diff --git a/drivers/power/reset/Kconfig b/drivers/power/reset/Kconfig
new file mode 100644
index 0000000..6461b48
--- /dev/null
+++ b/drivers/power/reset/Kconfig
@@ -0,0 +1,15 @@
+menuconfig POWER_RESET
+	bool "Board level reset or power off"
+	help
+	  Provides a number of drivers which either reset a complete board
+	  or shut it down, by manipulating the main power supply on the board.
+
+	  Say Y here to enable board reset and power off
+
+config POWER_RESET_GPIO
+	bool "GPIO power-off driver"
+	depends on OF_GPIO && POWER_RESET
+	help
+	  This driver supports turning off your board via a GPIO line.
+	  If your board needs a GPIO high/low to power down, say Y and
+	  create a binding in your devicetree.
diff --git a/drivers/power/reset/Makefile b/drivers/power/reset/Makefile
new file mode 100644
index 0000000..751488a
--- /dev/null
+++ b/drivers/power/reset/Makefile
@@ -0,0 +1 @@
+obj-$(CONFIG_POWER_RESET_GPIO) += gpio-poweroff.o
diff --git a/drivers/power/reset/gpio-poweroff.c b/drivers/power/reset/gpio-poweroff.c
new file mode 100644
index 0000000..0491e53
--- /dev/null
+++ b/drivers/power/reset/gpio-poweroff.c
@@ -0,0 +1,129 @@
+/*
+ * Toggles a GPIO pin to power down a device
+ *
+ * Jamie Lentin <jm-Pj/HzkgeCk7QXOPxS62xeg@public.gmane.org>
+ * Andrew Lunn <andrew-g2DYL2Zd6BY@public.gmane.org>
+ *
+ * Copyright (C) 2012 Jamie Lentin
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ */
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/delay.h>
+#include <linux/platform_device.h>
+#include <linux/gpio.h>
+#include <linux/of_platform.h>
+#include <linux/of_gpio.h>
+#include <linux/module.h>
+
+/*
+ * Hold configuration here, cannot be more than one instance of the driver
+ * since pm_power_off itself is global.
+ */
+static int gpio_num = -1;
+static int gpio_active_low;
+
+static void gpio_poweroff_do_poweroff(void)
+{
+	BUG_ON(gpio_num == -1);
+
+	/* drive it active */
+	gpio_direction_output(gpio_num, !gpio_active_low);
+	mdelay(100);
+	/* rising edge or drive inactive */
+	gpio_set_value(gpio_num, gpio_active_low);
+	mdelay(100);
+	/* falling edge */
+	gpio_set_value(gpio_num, !gpio_active_low);
+
+	/* give it some time */
+	mdelay(3000);
+
+	WARN_ON(1);
+}
+
+static int __devinit gpio_poweroff_probe(struct platform_device *pdev)
+{
+	enum of_gpio_flags flags;
+	bool input = false;
+	int ret;
+
+	/* If a pm_power_off function has already been added, leave it alone */
+	if (pm_power_off != NULL) {
+		pr_err("%s: pm_power_off function already registered",
+		       __func__);
+		return -EBUSY;
+	}
+
+	gpio_num = of_get_gpio_flags(pdev->dev.of_node, 0, &flags);
+	if (gpio_num < 0) {
+		pr_err("%s: Could not get GPIO configuration: %d",
+		       __func__, gpio_num);
+		return -ENODEV;
+	}
+	gpio_active_low = flags & OF_GPIO_ACTIVE_LOW;
+
+	if (of_get_property(pdev->dev.of_node, "input", NULL))
+		input = true;
+
+	ret = gpio_request(gpio_num, "poweroff-gpio");
+	if (ret) {
+		pr_err("%s: Could not get GPIO %d", __func__, gpio_num);
+		return ret;
+	}
+	if (input) {
+		if (gpio_direction_input(gpio_num)) {
+			pr_err("Could not set direction of GPIO %d to input",
+			       gpio_num);
+			goto err;
+		}
+	} else {
+		if (gpio_direction_output(gpio_num, gpio_active_low)) {
+			pr_err("Could not set direction of GPIO %d", gpio_num);
+			goto err;
+		}
+	}
+
+	pm_power_off = &gpio_poweroff_do_poweroff;
+	return 0;
+
+err:
+	gpio_free(gpio_num);
+	return -ENODEV;
+}
+
+static int __devexit gpio_poweroff_remove(struct platform_device *pdev)
+{
+	if (gpio_num != -1)
+		gpio_free(gpio_num);
+	if (pm_power_off == &gpio_poweroff_do_poweroff)
+		pm_power_off = NULL;
+
+	return 0;
+}
+
+static const struct of_device_id of_gpio_poweroff_match[] = {
+	{ .compatible = "gpio-poweroff", },
+	{},
+};
+
+static struct platform_driver gpio_poweroff_driver = {
+	.probe = gpio_poweroff_probe,
+	.remove = __devexit_p(gpio_poweroff_remove),
+	.driver = {
+		   .name = "poweroff-gpio",
+		   .owner = THIS_MODULE,
+		   .of_match_table = of_gpio_poweroff_match,
+		   },
+};
+
+module_platform_driver(gpio_poweroff_driver);
+
+MODULE_AUTHOR("Jamie Lentin <jm-Pj/HzkgeCk7QXOPxS62xeg@public.gmane.org>");
+MODULE_DESCRIPTION("GPIO poweroff driver");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:poweroff-gpio");
-- 
1.7.10.4

Re: [PATCH v3 1/3] power: Add simple poweroff-gpio driver

[Stephen Warren]

On 11/17/2012 01:51 AM, Andrew Lunn wrote:
> Given appropriate devicetree bindings, this driver registers a
> pm_power_off function to set a GPIO line high/low to power down
> your board.

> diff --git a/drivers/power/reset/Kconfig b/drivers/power/reset/Kconfig

> +menuconfig POWER_RESET
> +	bool "Board level reset or power off"
> +	help
> +	  Provides a number of drivers which either reset a complete board
> +	  or shut it down, by manipulating the main power supply on the board.
> +
> +	  Say Y here to enable board reset and power off
> +
> +config POWER_RESET_GPIO
> +	bool "GPIO power-off driver"
> +	depends on OF_GPIO && POWER_RESET

I assume CONFIG_POWER_RESET won't really provide any/much
infra-structure itself. So, does it make sense to put all the individual
drives inside "if POWER_RESET" or a menu definition, so they don't all
have to depend on POWER_RESET explicitly?

> diff --git a/drivers/power/reset/gpio-poweroff.c b/drivers/power/reset/gpio-poweroff.c

> +static void gpio_poweroff_do_poweroff(void)
> +{
> +	BUG_ON(gpio_num == -1);

Perhaps use gpio_is_valid() here?

> +	/* drive it active */
> +	gpio_direction_output(gpio_num, !gpio_active_low);

The rest of the code below doesn't make a lot of sense to me. From
reading the binding documentation, it seems like the GPIO is expected to
be level-sensitive, and as such the above gpio_direction_output() call
should be all that's needed. What is the code below doing? If this
driver supports either a level-sensitive GPIO or generating a pulse on a
GPIO, I think the binding should be enhanced to specify which signalling
type is required. Also, all the delays should be specified as DT properties.

> +	mdelay(100);
> +	/* rising edge or drive inactive */

You can't assume that an active->inactive edge is a rising edge if you
allow the polarity to be configurable; I would remove that part of the
comment. Same below for "falling edge".

> +	gpio_set_value(gpio_num, gpio_active_low);
> +	mdelay(100);
> +	/* falling edge */
> +	gpio_set_value(gpio_num, !gpio_active_low);
> +
> +	/* give it some time */
> +	mdelay(3000);

> +static int __devinit gpio_poweroff_probe(struct platform_device *pdev)

> +	gpio_num = of_get_gpio_flags(pdev->dev.of_node, 0, &flags);
> +	if (gpio_num < 0) {

Perhaps use gpio_is_valid() here?

> +		pr_err("%s: Could not get GPIO configuration: %d",
> +		       __func__, gpio_num);
> +		return -ENODEV;
> +	}

This doesn't handle deferred probe correctly; if gpio_num ==
-EPROBE_DEFER, then this function needs to return -EPROBE_DEFER too; why
not "return gpio_num" rather than "return -ENODEV" above?

> +	gpio_active_low = flags & OF_GPIO_ACTIVE_LOW;
> +
> +	if (of_get_property(pdev->dev.of_node, "input", NULL))
> +		input = true;

Perhaps use of_property_read_bool() here.

> +static int __devexit gpio_poweroff_remove(struct platform_device *pdev)
> +{
> +	if (gpio_num != -1)
> +		gpio_free(gpio_num);

Perhaps use gpio_is_valid() here? Actually, how could this happen;
presumably if the GPIO is valid, probe() never succeeded, so you should
always just free the GPIO.

> +static struct platform_driver gpio_poweroff_driver = {
> +	.probe = gpio_poweroff_probe,
> +	.remove = __devexit_p(gpio_poweroff_remove),
> +	.driver = {
> +		   .name = "poweroff-gpio",
> +		   .owner = THIS_MODULE,
> +		   .of_match_table = of_gpio_poweroff_match,
> +		   },

There seems to be a mix of TAB/space indents there, and the closing }
should probably be aligned with the start of ".driver"?

> +MODULE_LICENSE("GPL");

That should be "GPL v2".

Re: [PATCH v3 1/3] power: Add simple poweroff-gpio driver

[Andrew Lunn]

Hi Jason

These are good comments from Stephan that i want to address. However,
i also don't want to delay the pull-requests direction arm-soc, the
merge window is getting close. Both Linus and Anton have Acked the
current version, so please go with what you have and i will produce a
patch over the top. If its available before Arnd pulls, you can squash
it, otherwise send it upstream as a standalone patch.

    Thanks
	Andrew


On Mon, Nov 19, 2012 at 10:38:06AM -0700, Stephen Warren wrote:
> On 11/17/2012 01:51 AM, Andrew Lunn wrote:
> > Given appropriate devicetree bindings, this driver registers a
> > pm_power_off function to set a GPIO line high/low to power down
> > your board.
> 
> > diff --git a/drivers/power/reset/Kconfig b/drivers/power/reset/Kconfig
> 
> > +menuconfig POWER_RESET
> > +	bool "Board level reset or power off"
> > +	help
> > +	  Provides a number of drivers which either reset a complete board
> > +	  or shut it down, by manipulating the main power supply on the board.
> > +
> > +	  Say Y here to enable board reset and power off
> > +
> > +config POWER_RESET_GPIO
> > +	bool "GPIO power-off driver"
> > +	depends on OF_GPIO && POWER_RESET
> 
> I assume CONFIG_POWER_RESET won't really provide any/much
> infra-structure itself. So, does it make sense to put all the individual
> drives inside "if POWER_RESET" or a menu definition, so they don't all
> have to depend on POWER_RESET explicitly?
> 
> > diff --git a/drivers/power/reset/gpio-poweroff.c b/drivers/power/reset/gpio-poweroff.c
> 
> > +static void gpio_poweroff_do_poweroff(void)
> > +{
> > +	BUG_ON(gpio_num == -1);
> 
> Perhaps use gpio_is_valid() here?
> 
> > +	/* drive it active */
> > +	gpio_direction_output(gpio_num, !gpio_active_low);
> 
> The rest of the code below doesn't make a lot of sense to me. From
> reading the binding documentation, it seems like the GPIO is expected to
> be level-sensitive, and as such the above gpio_direction_output() call
> should be all that's needed. What is the code below doing? If this
> driver supports either a level-sensitive GPIO or generating a pulse on a
> GPIO, I think the binding should be enhanced to specify which signalling
> type is required. Also, all the delays should be specified as DT properties.
> 
> > +	mdelay(100);
> > +	/* rising edge or drive inactive */
> 
> You can't assume that an active->inactive edge is a rising edge if you
> allow the polarity to be configurable; I would remove that part of the
> comment. Same below for "falling edge".
> 
> > +	gpio_set_value(gpio_num, gpio_active_low);
> > +	mdelay(100);
> > +	/* falling edge */
> > +	gpio_set_value(gpio_num, !gpio_active_low);
> > +
> > +	/* give it some time */
> > +	mdelay(3000);
> 
> > +static int __devinit gpio_poweroff_probe(struct platform_device *pdev)
> 
> > +	gpio_num = of_get_gpio_flags(pdev->dev.of_node, 0, &flags);
> > +	if (gpio_num < 0) {
> 
> Perhaps use gpio_is_valid() here?
> 
> > +		pr_err("%s: Could not get GPIO configuration: %d",
> > +		       __func__, gpio_num);
> > +		return -ENODEV;
> > +	}
> 
> This doesn't handle deferred probe correctly; if gpio_num ==
> -EPROBE_DEFER, then this function needs to return -EPROBE_DEFER too; why
> not "return gpio_num" rather than "return -ENODEV" above?
> 
> > +	gpio_active_low = flags & OF_GPIO_ACTIVE_LOW;
> > +
> > +	if (of_get_property(pdev->dev.of_node, "input", NULL))
> > +		input = true;
> 
> Perhaps use of_property_read_bool() here.
> 
> > +static int __devexit gpio_poweroff_remove(struct platform_device *pdev)
> > +{
> > +	if (gpio_num != -1)
> > +		gpio_free(gpio_num);
> 
> Perhaps use gpio_is_valid() here? Actually, how could this happen;
> presumably if the GPIO is valid, probe() never succeeded, so you should
> always just free the GPIO.
> 
> > +static struct platform_driver gpio_poweroff_driver = {
> > +	.probe = gpio_poweroff_probe,
> > +	.remove = __devexit_p(gpio_poweroff_remove),
> > +	.driver = {
> > +		   .name = "poweroff-gpio",
> > +		   .owner = THIS_MODULE,
> > +		   .of_match_table = of_gpio_poweroff_match,
> > +		   },
> 
> There seems to be a mix of TAB/space indents there, and the closing }
> should probably be aligned with the start of ".driver"?
> 
> > +MODULE_LICENSE("GPL");
> 
> That should be "GPL v2".
> 

Re: [PATCH v3 1/3] power: Add simple poweroff-gpio driver

[Anton Vorontsov]

On Tue, Nov 20, 2012 at 09:37:49AM +0100, Andrew Lunn wrote:
> These are good comments from Stephan that i want to address. However,
> i also don't want to delay the pull-requests direction arm-soc, the
> merge window is getting close. Both Linus and Anton have Acked the
> current version, so please go with what you have and i will produce a
> patch over the top. If its available before Arnd pulls, you can squash
> it, otherwise send it upstream as a standalone patch.

I agree. Furthermore, for the fixup patch, if it goes separately, you
might want to add

 Suggested-by: Stephen Warren <swarren@wwwdotorg.org>

to give a credit for the valuable comments. :)

Re: [PATCH v3 1/3] power: Add simple poweroff-gpio driver

[Stephen Warren]

On 11/20/2012 01:37 AM, Andrew Lunn wrote:
> Hi Jason
> 
> These are good comments from Stephan that i want to address. However,
> i also don't want to delay the pull-requests direction arm-soc, the
> merge window is getting close. Both Linus and Anton have Acked the
> current version, so please go with what you have and i will produce a
> patch over the top. If its available before Arnd pulls, you can squash
> it, otherwise send it upstream as a standalone patch.

I'm not sure I agree here; the comments I made re: the delays and
pulse-vs-level may affect the definition of the DT binding, and that's
something that should be correct from the start.

The implementation of gpio_poweroff_do_poweroff() really doesn't seem to
make sense; related to the above.

Also, probe deferral doesn't work, which will likely make this code
completely ineffective.

Re: [PATCH v3 1/3] power: Add simple poweroff-gpio driver

[Andrew Lunn]

On Tue, Nov 20, 2012 at 10:11:18AM -0700, Stephen Warren wrote:
> On 11/20/2012 01:37 AM, Andrew Lunn wrote:
> > Hi Jason
> > 
> > These are good comments from Stephan that i want to address. However,
> > i also don't want to delay the pull-requests direction arm-soc, the
> > merge window is getting close. Both Linus and Anton have Acked the
> > current version, so please go with what you have and i will produce a
> > patch over the top. If its available before Arnd pulls, you can squash
> > it, otherwise send it upstream as a standalone patch.
> 
> I'm not sure I agree here; the comments I made re: the delays and
> pulse-vs-level may affect the definition of the DT binding, and that's
> something that should be correct from the start.

Hi Stephen

There should be no need to change the binding is a none-backwards
compatible way. The current delays work for all current users. If some
other user comes along which needs longer delays, they can add an
optional property which specified a longer delay.

The pulse-vs-level is needed for the potential PXA use case, which is
where i took this code from. When we first proposed this driver,
Russel King pointed to the PXA reset.c code and requested we create
something which PXA could use. The configuring the GPIO as an input
also come from PXA, which the current two uses user don't need.

I agree it need better documentation, but the current code covers all
bases. It will turn off a level based system, it will also turn off an
edge based system. There is no need to specify in the DT which is
needed, it will just work.

> The implementation of gpio_poweroff_do_poweroff() really doesn't seem to
> make sense; related to the above.
> 
> Also, probe deferral doesn't work, which will likely make this code
> completely ineffective.

This code has been tested by the two current users. It works. However,
i agree about your comment here, and i will fix it in the follow up
patch.

	Andrew

Re: [PATCH v3 1/3] power: Add simple poweroff-gpio driver

[Stephen Warren]

On 11/20/2012 01:31 PM, Andrew Lunn wrote:
> On Tue, Nov 20, 2012 at 10:11:18AM -0700, Stephen Warren wrote:
>> On 11/20/2012 01:37 AM, Andrew Lunn wrote:
>>> Hi Jason
>>>
>>> These are good comments from Stephan that i want to address. However,
>>> i also don't want to delay the pull-requests direction arm-soc, the
>>> merge window is getting close. Both Linus and Anton have Acked the
>>> current version, so please go with what you have and i will produce a
>>> patch over the top. If its available before Arnd pulls, you can squash
>>> it, otherwise send it upstream as a standalone patch.
>>
>> I'm not sure I agree here; the comments I made re: the delays and
>> pulse-vs-level may affect the definition of the DT binding, and that's
>> something that should be correct from the start.
> 
> Hi Stephen
> 
> There should be no need to change the binding is a none-backwards
> compatible way. The current delays work for all current users.

Well, the binding isn't written for current users, the description is
just a generic "GPIO to turn off the system".

> If some
> other user comes along which needs longer delays, they can add an
> optional property which specified a longer delay.

The DT maintainers have in the past expressed a wish to define DT
bindings completely, rather than incrementally adding stuff if at all
possible. I guess they weren't CC'd on this patch; I've added them here
for comment. I'll defer to whatever they think here.

> The pulse-vs-level is needed for the potential PXA use case, which is
> where i took this code from. When we first proposed this driver,
> Russel King pointed to the PXA reset.c code and requested we create
> something which PXA could use. The configuring the GPIO as an input
> also come from PXA, which the current two uses user don't need.
> 
> I agree it need better documentation, but the current code covers all
> bases. It will turn off a level based system, it will also turn off an
> edge based system. There is no need to specify in the DT which is
> needed, it will just work.

I'm not convinced the pulse logic can be correct for arbitrary users who
expect a level-sensitive GPIO; what if the MPU (or whatever the GPIO is
connected to) is confused by the pulse and fails to power down when it
sees a pulse (e.g. as a safety measure), but would have worked fine with
just a regular level?

>> The implementation of gpio_poweroff_do_poweroff() really doesn't seem to
>> make sense; related to the above.
>>
>> Also, probe deferral doesn't work, which will likely make this code
>> completely ineffective.
> 
> This code has been tested by the two current users. It works. However,
> i agree about your comment here, and i will fix it in the follow up
> patch.

If this were a specific driver just for certain platforms, I think it
might be OK. Yet the driver purports to be generic. As such, I think we
need to make it explicitly work for the general case, not just happen to
work for the platforms that have been tested so far.

Re: [PATCH v3 1/3] power: Add simple poweroff-gpio driver

[Andrew Lunn]

On Tue, Nov 20, 2012 at 02:17:24PM -0700, Stephen Warren wrote:
> On 11/20/2012 01:31 PM, Andrew Lunn wrote:
> > On Tue, Nov 20, 2012 at 10:11:18AM -0700, Stephen Warren wrote:
> >> On 11/20/2012 01:37 AM, Andrew Lunn wrote:
> >>> Hi Jason
> >>>
> >>> These are good comments from Stephan that i want to address. However,
> >>> i also don't want to delay the pull-requests direction arm-soc, the
> >>> merge window is getting close. Both Linus and Anton have Acked the
> >>> current version, so please go with what you have and i will produce a
> >>> patch over the top. If its available before Arnd pulls, you can squash
> >>> it, otherwise send it upstream as a standalone patch.
> >>
> >> I'm not sure I agree here; the comments I made re: the delays and
> >> pulse-vs-level may affect the definition of the DT binding, and that's
> >> something that should be correct from the start.
> > 
> > Hi Stephen
> > 
> > There should be no need to change the binding is a none-backwards
> > compatible way. The current delays work for all current users.
> 
> Well, the binding isn't written for current users, the description is
> just a generic "GPIO to turn off the system".

The current binding documentation is maybe lacking. But the binding
itself fits the current users.

> > If some
> > other user comes along which needs longer delays, they can add an
> > optional property which specified a longer delay.
> 
> The DT maintainers have in the past expressed a wish to define DT
> bindings completely, rather than incrementally adding stuff if at all
> possible. I guess they weren't CC'd on this patch; I've added them here
> for comment. I'll defer to whatever they think here.

They were CC: I learn't the hard way with a USB binding....

> > The pulse-vs-level is needed for the potential PXA use case, which is
> > where i took this code from. When we first proposed this driver,
> > Russel King pointed to the PXA reset.c code and requested we create
> > something which PXA could use. The configuring the GPIO as an input
> > also come from PXA, which the current two uses user don't need.
> > 
> > I agree it need better documentation, but the current code covers all
> > bases. It will turn off a level based system, it will also turn off an
> > edge based system. There is no need to specify in the DT which is
> > needed, it will just work.
> 
> I'm not convinced the pulse logic can be correct for arbitrary users who
> expect a level-sensitive GPIO; what if the MPU (or whatever the GPIO is
> connected to) is confused by the pulse and fails to power down when it
> sees a pulse (e.g. as a safety measure), but would have worked fine with
> just a regular level?

Lets go through the sequence for a regular level off.....

At driver load time, assuming the optional input properties is not
present, the GPIO line is driven inactive. At power down time, its
driven active. One of four things can happen:

1) The power goes off. End of story 

2) The delay is not long enough. In that case it goes inactive again,
there is a short pause, and it goes active again, and the power goes
off. End of story.

3) The delay is not long enough. In that case it goes inactive again,
there is a short pause, and it goes active again. There is another
pause, this time much longer.  Still the power does not go off and we
hit the WARN_ON(1); Some time after that, the power goes off. Not so
nice, but at least the power went off eventually.

4) The delay is not long enough. In that case it goes inactive again,
there is a short pause, and it goes active again. There is another
pause, this time much longer.  Still the power does not go off and we
hit the WARN_ON(1); The power remains on, until somebody pulls the
plug.

Your scenario is that a 100ms pulse confuses the MPU, and after that,
it won't turn off given a much longer period of the gpio being
active. We run into 4). Is that really likely? a 100ms is a long
time. I could imaging a MPU ignoring a 1us pulse, or even a 1ms
pulse. But a 100ms pulse? Also, after such a pulse, it simply refuses
to turn off all together after 3000ms of active GPIO? Is that a
general purpose piece of hardware which can be driven by a general
purpose driver, or is it a specific piece of hardware which needs its
own driver?

> If this were a specific driver just for certain platforms, I think it
> might be OK. Yet the driver purports to be generic. As such, I think we
> need to make it explicitly work for the general case, not just happen to
> work for the platforms that have been tested so far.

And as i keep saying, i will submit a patch to fix this issue. It
won't be tomorrow, but it won't be longer than a week.

      Andrew

Re: [PATCH v3 1/3] power: Add simple poweroff-gpio driver

[Stephen Warren]

On 11/20/2012 03:05 PM, Andrew Lunn wrote:
> On Tue, Nov 20, 2012 at 02:17:24PM -0700, Stephen Warren wrote:
>> On 11/20/2012 01:31 PM, Andrew Lunn wrote:
>>> On Tue, Nov 20, 2012 at 10:11:18AM -0700, Stephen Warren wrote:
>>>> On 11/20/2012 01:37 AM, Andrew Lunn wrote:
>>>>> Hi Jason
>>>>>
>>>>> These are good comments from Stephan that i want to address. However,
>>>>> i also don't want to delay the pull-requests direction arm-soc, the
>>>>> merge window is getting close. Both Linus and Anton have Acked the
>>>>> current version, so please go with what you have and i will produce a
>>>>> patch over the top. If its available before Arnd pulls, you can squash
>>>>> it, otherwise send it upstream as a standalone patch.
>>>>
>>>> I'm not sure I agree here; the comments I made re: the delays and
>>>> pulse-vs-level may affect the definition of the DT binding, and that's
>>>> something that should be correct from the start.
>>>
>>> Hi Stephen
>>>
>>> There should be no need to change the binding is a none-backwards
>>> compatible way. The current delays work for all current users.
>>
>> Well, the binding isn't written for current users, the description is
>> just a generic "GPIO to turn off the system".
> 
> The current binding documentation is maybe lacking. But the binding
> itself fits the current users.
> 
>>> If some
>>> other user comes along which needs longer delays, they can add an
>>> optional property which specified a longer delay.
>>
>> The DT maintainers have in the past expressed a wish to define DT
>> bindings completely, rather than incrementally adding stuff if at all
>> possible. I guess they weren't CC'd on this patch; I've added them here
>> for comment. I'll defer to whatever they think here.
> 
> They were CC: I learn't the hard way with a USB binding....

Sorry to be picky, but I checked the original postings of the first rev
and V3 and they didn't appear to be CC'd. The mailing list was, but not
the people. I guess Grant did comment on the first rev though. He didn't
give an opinion on the DT binding that I can find.

>>> The pulse-vs-level is needed for the potential PXA use case, which is
>>> where i took this code from. When we first proposed this driver,
>>> Russel King pointed to the PXA reset.c code and requested we create
>>> something which PXA could use. The configuring the GPIO as an input
>>> also come from PXA, which the current two uses user don't need.
>>>
>>> I agree it need better documentation, but the current code covers all
>>> bases. It will turn off a level based system, it will also turn off an
>>> edge based system. There is no need to specify in the DT which is
>>> needed, it will just work.
>>
>> I'm not convinced the pulse logic can be correct for arbitrary users who
>> expect a level-sensitive GPIO; what if the MPU (or whatever the GPIO is
>> connected to) is confused by the pulse and fails to power down when it
>> sees a pulse (e.g. as a safety measure), but would have worked fine with
>> just a regular level?
> 
> Lets go through the sequence for a regular level off.....
> 
> At driver load time, assuming the optional input properties is not
> present, the GPIO line is driven inactive. At power down time, its
> driven active. One of four things can happen:
...
> 4) The delay is not long enough. In that case it goes inactive again,
> there is a short pause, and it goes active again. There is another
> pause, this time much longer.  Still the power does not go off and we
> hit the WARN_ON(1); The power remains on, until somebody pulls the
> plug.
> 
> Your scenario is that a 100ms pulse confuses the MPU, and after that,
> it won't turn off given a much longer period of the gpio being
> active. We run into 4). Is that really likely?

Yes, (4) is my concern.

Honestly, I don't know if it's likely. It's certainly theoretically
possible. Either way though, I'd rather the kernel explicitly drive the
signal in the fashion that the specific HW expects it to be driven,
rather than wiggling the GPIO in a way that by luck happens to work for
a variety of disparate requirements.

Anyway, I've made my opinion clear. Whoever would merge this patch can
choose to ignore it or not.

[PATCH v3 2/3] ARM: Kirkwood: Convert DNSKW to use gpio-poweroff.

[Andrew Lunn]

Also enable the gpio-poweroff driver when DT is used.

Signed-off-by: Andrew Lunn <andrew-g2DYL2Zd6BY@public.gmane.org>
Tested-by: Jamie Lentin <jm-Pj/HzkgeCk7QXOPxS62xeg@public.gmane.org>
---
 arch/arm/boot/dts/kirkwood-dnskw.dtsi |    5 +++++
 arch/arm/mach-kirkwood/Kconfig        |    4 ++++
 arch/arm/mach-kirkwood/board-dnskw.c  |   12 ------------
 3 files changed, 9 insertions(+), 12 deletions(-)

diff --git a/arch/arm/boot/dts/kirkwood-dnskw.dtsi b/arch/arm/boot/dts/kirkwood-dnskw.dtsi
index 9b32d02..22aa07a 100644
--- a/arch/arm/boot/dts/kirkwood-dnskw.dtsi
+++ b/arch/arm/boot/dts/kirkwood-dnskw.dtsi
@@ -35,6 +35,11 @@
 				      6000 2>;
 	};
 
+	gpio_poweroff {
+		compatible = "gpio-poweroff";
+		gpios = <&gpio1 4 0>;
+	};
+
 	ocp@f1000000 {
 		sata@80000 {
 			status = "okay";
diff --git a/arch/arm/mach-kirkwood/Kconfig b/arch/arm/mach-kirkwood/Kconfig
index 50bca50..e0d26fe 100644
--- a/arch/arm/mach-kirkwood/Kconfig
+++ b/arch/arm/mach-kirkwood/Kconfig
@@ -46,7 +46,11 @@ config MACH_GURUPLUG
 
 config ARCH_KIRKWOOD_DT
 	bool "Marvell Kirkwood Flattened Device Tree"
+	select POWER_SUPPLY
+	select POWER_RESET
+	select POWER_RESET_GPIO
 	select USE_OF
+
 	help
 	  Say 'Y' here if you want your kernel to support the
 	  Marvell Kirkwood using flattened device tree.
diff --git a/arch/arm/mach-kirkwood/board-dnskw.c b/arch/arm/mach-kirkwood/board-dnskw.c
index 43d16d6..549369e 100644
--- a/arch/arm/mach-kirkwood/board-dnskw.c
+++ b/arch/arm/mach-kirkwood/board-dnskw.c
@@ -57,11 +57,6 @@ static unsigned int dnskw_mpp_config[] __initdata = {
 	0
 };
 
-static void dnskw_power_off(void)
-{
-	gpio_set_value(36, 1);
-}
-
 /* Register any GPIO for output and set the value */
 static void __init dnskw_gpio_register(unsigned gpio, char *name, int def)
 {
@@ -81,13 +76,6 @@ void __init dnskw_init(void)
 	kirkwood_ehci_init();
 	kirkwood_ge00_init(&dnskw_ge00_data);
 
-	/* Register power-off GPIO. */
-	if (gpio_request(36, "dnskw:power:off") == 0
-	    && gpio_direction_output(36, 0) == 0)
-		pm_power_off = dnskw_power_off;
-	else
-		pr_err("dnskw: failed to configure power-off GPIO\n");
-
 	/* Ensure power is supplied to both HDDs */
 	dnskw_gpio_register(39, "dnskw:power:sata0", 1);
 	dnskw_gpio_register(40, "dnskw:power:sata1", 1);
-- 
1.7.10.4

[PATCH v3 3/3] ARM: Kirkwood: Convert IB62x0 to use gpio-poweroff.

[Andrew Lunn]

Signed-off-by: Andrew Lunn <andrew-g2DYL2Zd6BY@public.gmane.org>
Tested-by: Simon Baatz <gmbnomis-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 arch/arm/boot/dts/kirkwood-ib62x0.dts |    6 ++++++
 arch/arm/mach-kirkwood/board-ib62x0.c |   13 -------------
 2 files changed, 6 insertions(+), 13 deletions(-)

diff --git a/arch/arm/boot/dts/kirkwood-ib62x0.dts b/arch/arm/boot/dts/kirkwood-ib62x0.dts
index 66794ed..6a9558d 100644
--- a/arch/arm/boot/dts/kirkwood-ib62x0.dts
+++ b/arch/arm/boot/dts/kirkwood-ib62x0.dts
@@ -79,4 +79,10 @@
 			gpios = <&gpio0 27 0>;
 		};
 	};
+	gpio_poweroff {
+		compatible = "gpio-poweroff";
+		gpios = <&gpio0 24 0>;
+	};
+
+
 };
diff --git a/arch/arm/mach-kirkwood/board-ib62x0.c b/arch/arm/mach-kirkwood/board-ib62x0.c
index cfc47f8..7b8adf5 100644
--- a/arch/arm/mach-kirkwood/board-ib62x0.c
+++ b/arch/arm/mach-kirkwood/board-ib62x0.c
@@ -17,7 +17,6 @@
 #include <linux/mtd/partitions.h>
 #include <linux/ata_platform.h>
 #include <linux/mv643xx_eth.h>
-#include <linux/gpio.h>
 #include <linux/input.h>
 #include <asm/mach-types.h>
 #include <asm/mach/arch.h>
@@ -25,8 +24,6 @@
 #include "common.h"
 #include "mpp.h"
 
-#define IB62X0_GPIO_POWER_OFF	24
-
 static struct mv643xx_eth_platform_data ib62x0_ge00_data = {
 	.phy_addr	= MV643XX_ETH_PHY_ADDR(8),
 };
@@ -49,11 +46,6 @@ static unsigned int ib62x0_mpp_config[] __initdata = {
 	0
 };
 
-static void ib62x0_power_off(void)
-{
-	gpio_set_value(IB62X0_GPIO_POWER_OFF, 1);
-}
-
 void __init ib62x0_init(void)
 {
 	/*
@@ -63,9 +55,4 @@ void __init ib62x0_init(void)
 
 	kirkwood_ehci_init();
 	kirkwood_ge00_init(&ib62x0_ge00_data);
-	if (gpio_request(IB62X0_GPIO_POWER_OFF, "ib62x0:power:off") == 0 &&
-	    gpio_direction_output(IB62X0_GPIO_POWER_OFF, 0) == 0)
-		pm_power_off = ib62x0_power_off;
-	else
-		pr_err("board-ib62x0: failed to configure power-off GPIO\n");
 }
-- 
1.7.10.4