[PATCH v1 0/1] gpio-restart restart handler

[PATCH v1 0/1] gpio-restart restart handler

[David Riley]

This driver builds upon Guenter Roeck's kernel restart handler patchset
to add a driver which registers a GPIO-based restart handler which restarts
the system by toggling a GPIO.

Changes are based off 3.17-rc2 with the following patches from v7 of
Guenter's patchset:
https://patchwork.kernel.org/patch/4746721/
https://patchwork.kernel.org/patch/4746731/
https://patchwork.kernel.org/patch/4747011/
https://patchwork.kernel.org/patch/4746741/
https://patchwork.kernel.org/patch/4746861/

David Riley (1):
  power: Add simple gpio-restart driver

 .../devicetree/bindings/gpio/gpio-restart.txt      |  48 +++++++
 drivers/power/reset/Kconfig                        |   8 ++
 drivers/power/reset/Makefile                       |   1 +
 drivers/power/reset/gpio-restart.c                 | 142 +++++++++++++++++++++
 4 files changed, 199 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/gpio-restart.txt
 create mode 100644 drivers/power/reset/gpio-restart.c

-- 
2.0.0

[PATCH v1 1/1] power: Add simple gpio-restart driver

[David Riley]

This driver registers a restart handler to set a GPIO line high/low
to reset a board based on devicetree bindings.

Signed-off-by: David Riley <davidriley@chromium.org>
---
 .../devicetree/bindings/gpio/gpio-restart.txt      |  48 +++++++
 drivers/power/reset/Kconfig                        |   8 ++
 drivers/power/reset/Makefile                       |   1 +
 drivers/power/reset/gpio-restart.c                 | 142 +++++++++++++++++++++
 4 files changed, 199 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/gpio-restart.txt
 create mode 100644 drivers/power/reset/gpio-restart.c

diff --git a/Documentation/devicetree/bindings/gpio/gpio-restart.txt b/Documentation/devicetree/bindings/gpio/gpio-restart.txt
new file mode 100644
index 0000000..7cd58788
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/gpio-restart.txt
@@ -0,0 +1,48 @@
+Driver a GPIO line that can be used to restart the system as a
+restart handler.
+
+The driver supports both level triggered and edge triggered power off.
+At driver load time, the driver will request the given gpio line and
+install a restart handler. If the optional properties 'input' is
+not found, the GPIO line will be driven in the inactive state.
+Otherwise its configured as an input.
+
+When do_kernel_restart is called the various restart handlers will be tried
+in order.  The gpio is configured as an output, and drive active, so
+triggering a level triggered power off condition. This will also cause an
+inactive->active edge condition, so triggering positive edge triggered
+power off. After a delay of 100ms, the GPIO is set to inactive, thus
+causing an active->inactive edge, triggering negative edge triggered power
+off. After another 100ms delay the GPIO is driver active again. If the
+power is still on and the CPU still running after a 3000ms delay, a
+WARN_ON(1) is emitted.
+
+Required properties:
+- compatible : should be "gpio-restart".
+- 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 machine_restart function is called. If this optional
+  property is not specified, the GPIO is initialized as an output in its
+  inactive state.
+- priority : A priority ranging from 0 to 255 (default 128) according to
+  the following guidelines:
+	0:	Restart handler of last resort, with limited restart
+		capabilities
+	128:	Default restart handler; use if no other restart handler is
+		expected to be available, and/or if restart functionality is
+		sufficient to restart the entire system
+	255:	Highest priority restart handler, will preempt all other
+		restart handlers
+
+Examples:
+
+gpio-restart {
+	compatible = "gpio-restart";
+	gpios = <&gpio 4 0>;
+	priority = /bits/ 8 <200>;
+};
diff --git a/drivers/power/reset/Kconfig b/drivers/power/reset/Kconfig
index ca41523..f07e26c 100644
--- a/drivers/power/reset/Kconfig
+++ b/drivers/power/reset/Kconfig
@@ -39,6 +39,14 @@ config POWER_RESET_GPIO
 	  If your board needs a GPIO high/low to power down, say Y and
 	  create a binding in your devicetree.
 
+config POWER_RESET_GPIO_RESTART
+	bool "GPIO restart driver"
+	depends on OF_GPIO && POWER_RESET
+	help
+	  This driver supports restarting your board via a GPIO line.
+	  If your board needs a GPIO high/low to restart, say Y and
+	  create a binding in your devicetree.
+
 config POWER_RESET_HISI
 	bool "Hisilicon power-off driver"
 	depends on POWER_RESET && ARCH_HISI
diff --git a/drivers/power/reset/Makefile b/drivers/power/reset/Makefile
index a42e70e..199cb6e 100644
--- a/drivers/power/reset/Makefile
+++ b/drivers/power/reset/Makefile
@@ -2,6 +2,7 @@ obj-$(CONFIG_POWER_RESET_AS3722) += as3722-poweroff.o
 obj-$(CONFIG_POWER_RESET_AXXIA) += axxia-reset.o
 obj-$(CONFIG_POWER_RESET_BRCMSTB) += brcmstb-reboot.o
 obj-$(CONFIG_POWER_RESET_GPIO) += gpio-poweroff.o
+obj-$(CONFIG_POWER_RESET_GPIO_RESTART) += gpio-restart.o
 obj-$(CONFIG_POWER_RESET_HISI) += hisi-reboot.o
 obj-$(CONFIG_POWER_RESET_MSM) += msm-poweroff.o
 obj-$(CONFIG_POWER_RESET_QNAP) += qnap-poweroff.o
diff --git a/drivers/power/reset/gpio-restart.c b/drivers/power/reset/gpio-restart.c
new file mode 100644
index 0000000..2cbff64
--- /dev/null
+++ b/drivers/power/reset/gpio-restart.c
@@ -0,0 +1,142 @@
+/*
+ * Toggles a GPIO pin to restart a device
+ *
+ * Copyright (C) 2014 Google, Inc.
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * Based on the gpio-poweroff driver.
+ */
+#include <linux/reboot.h>
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/delay.h>
+#include <linux/platform_device.h>
+#include <linux/gpio/consumer.h>
+#include <linux/of_platform.h>
+#include <linux/module.h>
+
+struct gpio_restart {
+	struct gpio_desc *reset_gpio;
+	struct notifier_block restart_handler;
+};
+
+static int gpio_restart_notify(struct notifier_block *this,
+				unsigned long mode, void *cmd)
+{
+	struct gpio_restart *gpio_restart =
+		container_of(this, struct gpio_restart, restart_handler);
+
+	BUG_ON(!gpio_restart->reset_gpio);
+
+	/* drive it active, also inactive->active edge */
+	gpiod_direction_output(gpio_restart->reset_gpio, 1);
+	mdelay(100);
+
+	/* drive inactive, also active->inactive edge */
+	gpiod_set_value(gpio_restart->reset_gpio, 0);
+	mdelay(100);
+
+	/* drive it active, also inactive->active edge */
+	gpiod_set_value(gpio_restart->reset_gpio, 1);
+
+	/* give it some time */
+	mdelay(3000);
+
+	WARN_ON(1);
+
+	return NOTIFY_DONE;
+}
+
+static int gpio_restart_probe(struct platform_device *pdev)
+{
+	struct gpio_restart *gpio_restart;
+	bool input = false;
+	u8 priority;
+	int ret;
+
+	gpio_restart = devm_kzalloc(&pdev->dev, sizeof(*gpio_restart),
+			GFP_KERNEL);
+	if (!gpio_restart)
+		return -ENOMEM;
+
+	gpio_restart->reset_gpio = devm_gpiod_get(&pdev->dev, NULL);
+	if (IS_ERR(gpio_restart->reset_gpio))
+		return PTR_ERR(gpio_restart->reset_gpio);
+
+	gpio_restart->restart_handler.notifier_call = gpio_restart_notify;
+	gpio_restart->restart_handler.priority = 128;
+
+	platform_set_drvdata(pdev, gpio_restart);
+
+	input = of_property_read_bool(pdev->dev.of_node, "input");
+	if (input) {
+		if (gpiod_direction_input(gpio_restart->reset_gpio)) {
+			dev_err(&pdev->dev,
+				"Could not set direction of reset GPIO to input\n");
+			return -ENODEV;
+		}
+	} else {
+		if (gpiod_direction_output(gpio_restart->reset_gpio, 0)) {
+			dev_err(&pdev->dev,
+				"Could not set direction of reset GPIO\n");
+			return -ENODEV;
+		}
+	}
+
+	if (!of_property_read_u8(pdev->dev.of_node, "priority", &priority))
+		gpio_restart->restart_handler.priority = priority;
+
+	ret = register_restart_handler(&gpio_restart->restart_handler);
+	if (ret) {
+		dev_err(&pdev->dev, "%s: cannot register restart handler, %d\n",
+				__func__, ret);
+		return -ENODEV;
+	}
+
+	return 0;
+}
+
+static int gpio_restart_remove(struct platform_device *pdev)
+{
+	struct gpio_restart *gpio_restart = platform_get_drvdata(pdev);
+	int ret;
+
+	ret = unregister_restart_handler(&gpio_restart->restart_handler);
+	if (ret) {
+		dev_err(&pdev->dev,
+				"%s: cannot unregister restart handler, %d\n",
+				__func__, ret);
+		return -ENODEV;
+	}
+
+	return 0;
+}
+
+static const struct of_device_id of_gpio_restart_match[] = {
+	{ .compatible = "gpio-restart", },
+	{},
+};
+
+static struct platform_driver gpio_restart_driver = {
+	.probe = gpio_restart_probe,
+	.remove = gpio_restart_remove,
+	.driver = {
+		.name = "restart-gpio",
+		.owner = THIS_MODULE,
+		.of_match_table = of_gpio_restart_match,
+	},
+};
+
+module_platform_driver(gpio_restart_driver);
+
+MODULE_AUTHOR("David Riley <davidriley@chromium.org>");
+MODULE_DESCRIPTION("GPIO restart driver");
+MODULE_LICENSE("GPL");
-- 
2.0.0

Re: [PATCH v1 1/1] power: Add simple gpio-restart driver

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 3431 bytes --]

Hi David,

On Tue, Aug 26, 2014 at 04:45:05PM -0700, David Riley wrote:
> This driver registers a restart handler to set a GPIO line high/low
> to reset a board based on devicetree bindings.

Driver looks fine to me. I have some comments about the
Documentation, though:

> [...]
> diff --git a/Documentation/devicetree/bindings/gpio/gpio-restart.txt b/Documentation/devicetree/bindings/gpio/gpio-restart.txt
> new file mode 100644
> index 0000000..7cd58788
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpio/gpio-restart.txt
> @@ -0,0 +1,48 @@
> +Driver a GPIO line that can be used to restart the system as a
> +restart handler.

Please fix the Typo (first word).

> [...]
> +
> +The driver supports both level triggered and edge triggered power off.
> +At driver load time, the driver will request the given gpio line and
> +install a restart handler.

The wording is too driver centric IMHO. You are supposed to document
the binding in a generic way. Maybe start with something like:

"This binding supports level and edge triggered reset."

(power off is the wrong word, since there is already gpio-poweroff).

> +If the optional properties 'input' is +not found, the GPIO line
> +will be driven in the inactive state. Otherwise its configured
> +as an input.

What is this needed for?

> +When do_kernel_restart is called the various restart handlers will be tried
> +in order.  The gpio is configured as an output, and drive active, so
> +triggering a level triggered power off condition. This will also cause an
> +inactive->active edge condition, so triggering positive edge triggered
> +power off. After a delay of 100ms, the GPIO is set to inactive, thus
> +causing an active->inactive edge, triggering negative edge triggered power
> +off. After another 100ms delay the GPIO is driver active again. If the
> +power is still on and the CPU still running after a 3000ms delay, a
> +WARN_ON(1) is emitted.

I really appreciate the description of the driver (it made it easier
to review it :)), but Documentation/devicetree should avoid
Linuxisms. In other words: this is the wrong location for the
description.

> +Required properties:
> +- compatible : should be "gpio-restart".
> +- 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 machine_restart function is called. If this optional
> +  property is not specified, the GPIO is initialized as an output in its
> +  inactive state.
> +- priority : A priority ranging from 0 to 255 (default 128) according to
> +  the following guidelines:
> +	0:	Restart handler of last resort, with limited restart
> +		capabilities
> +	128:	Default restart handler; use if no other restart handler is
> +		expected to be available, and/or if restart functionality is
> +		sufficient to restart the entire system
> +	255:	Highest priority restart handler, will preempt all other
> +		restart handlers

You should add a short information about the property type here
(e.g. "8 bit integer" for priority).

> +Examples:
> +
> +gpio-restart {
> +	compatible = "gpio-restart";
> +	gpios = <&gpio 4 0>;
> +	priority = /bits/ 8 <200>;
> +};
> [...]

-- Sebastian

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v1 1/1] power: Add simple gpio-restart driver

[Olof Johansson]

Hi,



On Tue, Aug 26, 2014 at 4:45 PM, David Riley <davidriley@chromium.org> wrote:
> This driver registers a restart handler to set a GPIO line high/low
> to reset a board based on devicetree bindings.
>
> Signed-off-by: David Riley <davidriley@chromium.org>
> ---
>  .../devicetree/bindings/gpio/gpio-restart.txt      |  48 +++++++
>  drivers/power/reset/Kconfig                        |   8 ++
>  drivers/power/reset/Makefile                       |   1 +
>  drivers/power/reset/gpio-restart.c                 | 142 +++++++++++++++++++++
>  4 files changed, 199 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/gpio/gpio-restart.txt
>  create mode 100644 drivers/power/reset/gpio-restart.c
>
> diff --git a/Documentation/devicetree/bindings/gpio/gpio-restart.txt b/Documentation/devicetree/bindings/gpio/gpio-restart.txt
> new file mode 100644
> index 0000000..7cd58788
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpio/gpio-restart.txt
> @@ -0,0 +1,48 @@
> +Driver a GPIO line that can be used to restart the system as a
> +restart handler.
> +
> +The driver supports both level triggered and edge triggered power off.
> +At driver load time, the driver will request the given gpio line and
> +install a restart handler. If the optional properties 'input' is
> +not found, the GPIO line will be driven in the inactive state.
> +Otherwise its configured as an input.
> +
> +When do_kernel_restart is called the various restart handlers will be tried
> +in order.

The above sentence documents the kernel behavior, not the hardware
description/binding.

> +The gpio is configured as an output, and drive active, so
> +triggering a level triggered power off condition. This will also cause an
> +inactive->active edge condition, so triggering positive edge triggered
> +power off.

> + After a delay of 100ms, the GPIO is set to inactive, thus
> +causing an active->inactive edge, triggering negative edge triggered power
> +off. After another 100ms delay the GPIO is driver active again. If the
> +power is still on and the CPU still running after a 3000ms delay, a
> +WARN_ON(1) is emitted.

It's possible that this behavior is inadequate for some hardware in
the future -- if so they can amend the binding (i.e. this comment is
an attempt at preemptive bikeshed avoidance :)

> +
> +Required properties:
> +- compatible : should be "gpio-restart".
> +- 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 machine_restart function is called. If this optional
> +  property is not specified, the GPIO is initialized as an output in its
> +  inactive state.

Isn't this the same as configuring the pin as tristate? I think that
should probably be controlled by pinmux setup instead?

> +- priority : A priority ranging from 0 to 255 (default 128) according to
> +  the following guidelines:
> +       0:      Restart handler of last resort, with limited restart
> +               capabilities
> +       128:    Default restart handler; use if no other restart handler is
> +               expected to be available, and/or if restart functionality is
> +               sufficient to restart the entire system
> +       255:    Highest priority restart handler, will preempt all other
> +               restart handlers

This is sort of leaking linux implementation, but it's also a useful
feature to have in the description. It seems sane enough to me to use.

> +
> +Examples:
> +
> +gpio-restart {
> +       compatible = "gpio-restart";
> +       gpios = <&gpio 4 0>;
> +       priority = /bits/ 8 <200>;

I think it makes sense to just have this as a regular cell instead of
doing an 8-bit value -- it's how we normally handle these elsewhere.


-Olof

Re: [PATCH v1 1/1] power: Add simple gpio-restart driver

[Guenter Roeck]

On 08/26/2014 04:45 PM, David Riley wrote:
> This driver registers a restart handler to set a GPIO line high/low
> to reset a board based on devicetree bindings.
>
> Signed-off-by: David Riley <davidriley@chromium.org>
> ---
>   .../devicetree/bindings/gpio/gpio-restart.txt      |  48 +++++++
>   drivers/power/reset/Kconfig                        |   8 ++
>   drivers/power/reset/Makefile                       |   1 +
>   drivers/power/reset/gpio-restart.c                 | 142 +++++++++++++++++++++
>   4 files changed, 199 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/gpio/gpio-restart.txt
>   create mode 100644 drivers/power/reset/gpio-restart.c
>
> diff --git a/Documentation/devicetree/bindings/gpio/gpio-restart.txt b/Documentation/devicetree/bindings/gpio/gpio-restart.txt
> new file mode 100644
> index 0000000..7cd58788
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpio/gpio-restart.txt
> @@ -0,0 +1,48 @@
> +Driver a GPIO line that can be used to restart the system as a
> +restart handler.
> +
> +The driver supports both level triggered and edge triggered power off.
> +At driver load time, the driver will request the given gpio line and
> +install a restart handler. If the optional properties 'input' is
> +not found, the GPIO line will be driven in the inactive state.
> +Otherwise its configured as an input.
> +
> +When do_kernel_restart is called the various restart handlers will be tried
> +in order.  The gpio is configured as an output, and drive active, so
> +triggering a level triggered power off condition. This will also cause an
> +inactive->active edge condition, so triggering positive edge triggered
> +power off. After a delay of 100ms, the GPIO is set to inactive, thus
> +causing an active->inactive edge, triggering negative edge triggered power
> +off. After another 100ms delay the GPIO is driver active again. If the
> +power is still on and the CPU still running after a 3000ms delay, a
> +WARN_ON(1) is emitted.
> +
> +Required properties:
> +- compatible : should be "gpio-restart".
> +- 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 machine_restart function is called. If this optional
> +  property is not specified, the GPIO is initialized as an output in its
> +  inactive state.

Maybe describe this as open source ?

> +- priority : A priority ranging from 0 to 255 (default 128) according to
> +  the following guidelines:
> +	0:	Restart handler of last resort, with limited restart
> +		capabilities
> +	128:	Default restart handler; use if no other restart handler is
> +		expected to be available, and/or if restart functionality is
> +		sufficient to restart the entire system
> +	255:	Highest priority restart handler, will preempt all other
> +		restart handlers
> +
> +Examples:
> +
> +gpio-restart {
> +	compatible = "gpio-restart";
> +	gpios = <&gpio 4 0>;
> +	priority = /bits/ 8 <200>;
> +};
> diff --git a/drivers/power/reset/Kconfig b/drivers/power/reset/Kconfig
> index ca41523..f07e26c 100644
> --- a/drivers/power/reset/Kconfig
> +++ b/drivers/power/reset/Kconfig
> @@ -39,6 +39,14 @@ config POWER_RESET_GPIO
>   	  If your board needs a GPIO high/low to power down, say Y and
>   	  create a binding in your devicetree.
>
> +config POWER_RESET_GPIO_RESTART
> +	bool "GPIO restart driver"
> +	depends on OF_GPIO && POWER_RESET
> +	help
> +	  This driver supports restarting your board via a GPIO line.
> +	  If your board needs a GPIO high/low to restart, say Y and
> +	  create a binding in your devicetree.
> +
>   config POWER_RESET_HISI
>   	bool "Hisilicon power-off driver"
>   	depends on POWER_RESET && ARCH_HISI
> diff --git a/drivers/power/reset/Makefile b/drivers/power/reset/Makefile
> index a42e70e..199cb6e 100644
> --- a/drivers/power/reset/Makefile
> +++ b/drivers/power/reset/Makefile
> @@ -2,6 +2,7 @@ obj-$(CONFIG_POWER_RESET_AS3722) += as3722-poweroff.o
>   obj-$(CONFIG_POWER_RESET_AXXIA) += axxia-reset.o
>   obj-$(CONFIG_POWER_RESET_BRCMSTB) += brcmstb-reboot.o
>   obj-$(CONFIG_POWER_RESET_GPIO) += gpio-poweroff.o
> +obj-$(CONFIG_POWER_RESET_GPIO_RESTART) += gpio-restart.o
>   obj-$(CONFIG_POWER_RESET_HISI) += hisi-reboot.o
>   obj-$(CONFIG_POWER_RESET_MSM) += msm-poweroff.o
>   obj-$(CONFIG_POWER_RESET_QNAP) += qnap-poweroff.o
> diff --git a/drivers/power/reset/gpio-restart.c b/drivers/power/reset/gpio-restart.c
> new file mode 100644
> index 0000000..2cbff64
> --- /dev/null
> +++ b/drivers/power/reset/gpio-restart.c
> @@ -0,0 +1,142 @@
> +/*
> + * Toggles a GPIO pin to restart a device
> + *
> + * Copyright (C) 2014 Google, Inc.
> + *
> + * This software is licensed under the terms of the GNU General Public
> + * License version 2, as published by the Free Software Foundation, and
> + * may be copied, distributed, and modified under those terms.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * Based on the gpio-poweroff driver.
> + */
> +#include <linux/reboot.h>
> +#include <linux/kernel.h>
> +#include <linux/init.h>
> +#include <linux/delay.h>
> +#include <linux/platform_device.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/of_platform.h>
> +#include <linux/module.h>
> +
> +struct gpio_restart {
> +	struct gpio_desc *reset_gpio;
> +	struct notifier_block restart_handler;
> +};
> +
> +static int gpio_restart_notify(struct notifier_block *this,
> +				unsigned long mode, void *cmd)
> +{
> +	struct gpio_restart *gpio_restart =
> +		container_of(this, struct gpio_restart, restart_handler);
> +
> +	BUG_ON(!gpio_restart->reset_gpio);
> +
Is this really necessary ? First, it can't really happen,
but if it happens anyway I am not sure if there might be
a possibility for recursiveness. After all, this is called
from machine_restart().

> +	/* drive it active, also inactive->active edge */
> +	gpiod_direction_output(gpio_restart->reset_gpio, 1);
> +	mdelay(100);
> +
> +	/* drive inactive, also active->inactive edge */
> +	gpiod_set_value(gpio_restart->reset_gpio, 0);
> +	mdelay(100);
> +
> +	/* drive it active, also inactive->active edge */
> +	gpiod_set_value(gpio_restart->reset_gpio, 1);
> +
> +	/* give it some time */
> +	mdelay(3000);
> +
Should those delays be parameters ?

> +	WARN_ON(1);
> +
> +	return NOTIFY_DONE;
> +}
> +
> +static int gpio_restart_probe(struct platform_device *pdev)
> +{
> +	struct gpio_restart *gpio_restart;
> +	bool input = false;
> +	u8 priority;
> +	int ret;
> +
> +	gpio_restart = devm_kzalloc(&pdev->dev, sizeof(*gpio_restart),
> +			GFP_KERNEL);
> +	if (!gpio_restart)
> +		return -ENOMEM;
> +
> +	gpio_restart->reset_gpio = devm_gpiod_get(&pdev->dev, NULL);
> +	if (IS_ERR(gpio_restart->reset_gpio))
> +		return PTR_ERR(gpio_restart->reset_gpio);
> +
There is now a flags parameter to devm_gpiod_get which should help
handling input/output with a single call.

> +	gpio_restart->restart_handler.notifier_call = gpio_restart_notify;
> +	gpio_restart->restart_handler.priority = 128;
> +
> +	platform_set_drvdata(pdev, gpio_restart);
> +
> +	input = of_property_read_bool(pdev->dev.of_node, "input");
> +	if (input) {
> +		if (gpiod_direction_input(gpio_restart->reset_gpio)) {
> +			dev_err(&pdev->dev,
> +				"Could not set direction of reset GPIO to input\n");
> +			return -ENODEV;
> +		}
> +	} else {
> +		if (gpiod_direction_output(gpio_restart->reset_gpio, 0)) {
> +			dev_err(&pdev->dev,
> +				"Could not set direction of reset GPIO\n");
> +			return -ENODEV;
> +		}
> +	}
> +
> +	if (!of_property_read_u8(pdev->dev.of_node, "priority", &priority))
> +		gpio_restart->restart_handler.priority = priority;
> +
> +	ret = register_restart_handler(&gpio_restart->restart_handler);
> +	if (ret) {
> +		dev_err(&pdev->dev, "%s: cannot register restart handler, %d\n",
> +				__func__, ret);
> +		return -ENODEV;
> +	}
> +
> +	return 0;
> +}
> +
> +static int gpio_restart_remove(struct platform_device *pdev)
> +{
> +	struct gpio_restart *gpio_restart = platform_get_drvdata(pdev);
> +	int ret;
> +
> +	ret = unregister_restart_handler(&gpio_restart->restart_handler);
> +	if (ret) {
> +		dev_err(&pdev->dev,
> +				"%s: cannot unregister restart handler, %d\n",
> +				__func__, ret);
> +		return -ENODEV;
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id of_gpio_restart_match[] = {
> +	{ .compatible = "gpio-restart", },
> +	{},
> +};
> +
> +static struct platform_driver gpio_restart_driver = {
> +	.probe = gpio_restart_probe,
> +	.remove = gpio_restart_remove,
> +	.driver = {
> +		.name = "restart-gpio",
> +		.owner = THIS_MODULE,
> +		.of_match_table = of_gpio_restart_match,
> +	},
> +};
> +
> +module_platform_driver(gpio_restart_driver);
> +
> +MODULE_AUTHOR("David Riley <davidriley@chromium.org>");
> +MODULE_DESCRIPTION("GPIO restart driver");
> +MODULE_LICENSE("GPL");
>

Re: [PATCH v1 1/1] power: Add simple gpio-restart driver

[David Riley]

Hi Sebastian,

Thanks for the feedback.

On Tue, Aug 26, 2014 at 6:43 PM, Sebastian Reichel <sre@kernel.org> wrote:
> Hi David,
>
> On Tue, Aug 26, 2014 at 04:45:05PM -0700, David Riley wrote:
>> This driver registers a restart handler to set a GPIO line high/low
>> to reset a board based on devicetree bindings.
>
> Driver looks fine to me. I have some comments about the
> Documentation, though:
>
>> [...]
>> diff --git a/Documentation/devicetree/bindings/gpio/gpio-restart.txt b/Documentation/devicetree/bindings/gpio/gpio-restart.txt
>> new file mode 100644
>> index 0000000..7cd58788
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/gpio/gpio-restart.txt
>> @@ -0,0 +1,48 @@
>> +Driver a GPIO line that can be used to restart the system as a
>> +restart handler.
>
> Please fix the Typo (first word).

Fixed.

>
>> [...]
>> +
>> +The driver supports both level triggered and edge triggered power off.
>> +At driver load time, the driver will request the given gpio line and
>> +install a restart handler.
>
> The wording is too driver centric IMHO. You are supposed to document
> the binding in a generic way. Maybe start with something like:
>
> "This binding supports level and edge triggered reset."
>
> (power off is the wrong word, since there is already gpio-poweroff).

I've cleaned this up for v2.

>> +If the optional properties 'input' is +not found, the GPIO line
>> +will be driven in the inactive state. Otherwise its configured
>> +as an input.
>
> What is this needed for?

This allows other hardware to be attached to the same line to reset
the system.  Carried forward from the gpio-poweroff implementation I
based this on.

>> +When do_kernel_restart is called the various restart handlers will be tried
>> +in order.  The gpio is configured as an output, and drive active, so
>> +triggering a level triggered power off condition. This will also cause an
>> +inactive->active edge condition, so triggering positive edge triggered
>> +power off. After a delay of 100ms, the GPIO is set to inactive, thus
>> +causing an active->inactive edge, triggering negative edge triggered power
>> +off. After another 100ms delay the GPIO is driver active again. If the
>> +power is still on and the CPU still running after a 3000ms delay, a
>> +WARN_ON(1) is emitted.
>
> I really appreciate the description of the driver (it made it easier
> to review it :)), but Documentation/devicetree should avoid
> Linuxisms. In other words: this is the wrong location for the
> description.

I've cleaned this up as well and made the explicit delays configurable.

>
>> +Required properties:
>> +- compatible : should be "gpio-restart".
>> +- 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 machine_restart function is called. If this optional
>> +  property is not specified, the GPIO is initialized as an output in its
>> +  inactive state.
>> +- priority : A priority ranging from 0 to 255 (default 128) according to
>> +  the following guidelines:
>> +     0:      Restart handler of last resort, with limited restart
>> +             capabilities
>> +     128:    Default restart handler; use if no other restart handler is
>> +             expected to be available, and/or if restart functionality is
>> +             sufficient to restart the entire system
>> +     255:    Highest priority restart handler, will preempt all other
>> +             restart handlers
>
> You should add a short information about the property type here
> (e.g. "8 bit integer" for priority).

As per Olof's comments I've just changed this to be a regular cell for
consistency with other bindings and will handle the range checking
internally.

>
>> +Examples:
>> +
>> +gpio-restart {
>> +     compatible = "gpio-restart";
>> +     gpios = <&gpio 4 0>;
>> +     priority = /bits/ 8 <200>;
>> +};
>> [...]
>
> -- Sebastian

Re: [PATCH v1 1/1] power: Add simple gpio-restart driver

[David Riley]

Hi Olof,

On Tue, Aug 26, 2014 at 7:14 PM, Olof Johansson <olof@lixom.net> wrote:
> Hi,
>
>
>
> On Tue, Aug 26, 2014 at 4:45 PM, David Riley <davidriley@chromium.org> wrote:
>> This driver registers a restart handler to set a GPIO line high/low
>> to reset a board based on devicetree bindings.
>>
>> Signed-off-by: David Riley <davidriley@chromium.org>
>> ---
>>  .../devicetree/bindings/gpio/gpio-restart.txt      |  48 +++++++
>>  drivers/power/reset/Kconfig                        |   8 ++
>>  drivers/power/reset/Makefile                       |   1 +
>>  drivers/power/reset/gpio-restart.c                 | 142 +++++++++++++++++++++
>>  4 files changed, 199 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/gpio/gpio-restart.txt
>>  create mode 100644 drivers/power/reset/gpio-restart.c
>>
>> diff --git a/Documentation/devicetree/bindings/gpio/gpio-restart.txt b/Documentation/devicetree/bindings/gpio/gpio-restart.txt
>> new file mode 100644
>> index 0000000..7cd58788
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/gpio/gpio-restart.txt
>> @@ -0,0 +1,48 @@
>> +Driver a GPIO line that can be used to restart the system as a
>> +restart handler.
>> +
>> +The driver supports both level triggered and edge triggered power off.
>> +At driver load time, the driver will request the given gpio line and
>> +install a restart handler. If the optional properties 'input' is
>> +not found, the GPIO line will be driven in the inactive state.
>> +Otherwise its configured as an input.
>> +
>> +When do_kernel_restart is called the various restart handlers will be tried
>> +in order.
>
> The above sentence documents the kernel behavior, not the hardware
> description/binding.

I've cleaned this up.

>
>> +The gpio is configured as an output, and drive active, so
>> +triggering a level triggered power off condition. This will also cause an
>> +inactive->active edge condition, so triggering positive edge triggered
>> +power off.
>
>> + After a delay of 100ms, the GPIO is set to inactive, thus
>> +causing an active->inactive edge, triggering negative edge triggered power
>> +off. After another 100ms delay the GPIO is driver active again. If the
>> +power is still on and the CPU still running after a 3000ms delay, a
>> +WARN_ON(1) is emitted.
>
> It's possible that this behavior is inadequate for some hardware in
> the future -- if so they can amend the binding (i.e. this comment is
> an attempt at preemptive bikeshed avoidance :)

Changed to be configurable at the request of Guenter.

>> +
>> +Required properties:
>> +- compatible : should be "gpio-restart".
>> +- 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 machine_restart function is called. If this optional
>> +  property is not specified, the GPIO is initialized as an output in its
>> +  inactive state.
>
> Isn't this the same as configuring the pin as tristate? I think that
> should probably be controlled by pinmux setup instead?

I think this still needs to be usable independently of pinctrl and it
keeps consistency with gpio-poweroff.

>
>> +- priority : A priority ranging from 0 to 255 (default 128) according to
>> +  the following guidelines:
>> +       0:      Restart handler of last resort, with limited restart
>> +               capabilities
>> +       128:    Default restart handler; use if no other restart handler is
>> +               expected to be available, and/or if restart functionality is
>> +               sufficient to restart the entire system
>> +       255:    Highest priority restart handler, will preempt all other
>> +               restart handlers
>
> This is sort of leaking linux implementation, but it's also a useful
> feature to have in the description. It seems sane enough to me to use.
>
>> +
>> +Examples:
>> +
>> +gpio-restart {
>> +       compatible = "gpio-restart";
>> +       gpios = <&gpio 4 0>;
>> +       priority = /bits/ 8 <200>;
>
> I think it makes sense to just have this as a regular cell instead of
> doing an 8-bit value -- it's how we normally handle these elsewhere.

Done.

>
>
> -Olof

Re: [PATCH v1 1/1] power: Add simple gpio-restart driver

[David Riley]

On Tue, Aug 26, 2014 at 7:40 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> On 08/26/2014 04:45 PM, David Riley wrote:
>>
>> This driver registers a restart handler to set a GPIO line high/low
>> to reset a board based on devicetree bindings.
>>
>> Signed-off-by: David Riley <davidriley@chromium.org>
>> ---
>>   .../devicetree/bindings/gpio/gpio-restart.txt      |  48 +++++++
>>   drivers/power/reset/Kconfig                        |   8 ++
>>   drivers/power/reset/Makefile                       |   1 +
>>   drivers/power/reset/gpio-restart.c                 | 142
>> +++++++++++++++++++++
>>   4 files changed, 199 insertions(+)
>>   create mode 100644
>> Documentation/devicetree/bindings/gpio/gpio-restart.txt
>>   create mode 100644 drivers/power/reset/gpio-restart.c
>>
>> diff --git a/Documentation/devicetree/bindings/gpio/gpio-restart.txt
>> b/Documentation/devicetree/bindings/gpio/gpio-restart.txt
>> new file mode 100644
>> index 0000000..7cd58788
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/gpio/gpio-restart.txt
>> @@ -0,0 +1,48 @@
>> +Driver a GPIO line that can be used to restart the system as a
>> +restart handler.
>> +
>> +The driver supports both level triggered and edge triggered power off.
>> +At driver load time, the driver will request the given gpio line and
>> +install a restart handler. If the optional properties 'input' is
>> +not found, the GPIO line will be driven in the inactive state.
>> +Otherwise its configured as an input.
>> +
>> +When do_kernel_restart is called the various restart handlers will be
>> tried
>> +in order.  The gpio is configured as an output, and drive active, so
>> +triggering a level triggered power off condition. This will also cause an
>> +inactive->active edge condition, so triggering positive edge triggered
>> +power off. After a delay of 100ms, the GPIO is set to inactive, thus
>> +causing an active->inactive edge, triggering negative edge triggered
>> power
>> +off. After another 100ms delay the GPIO is driver active again. If the
>> +power is still on and the CPU still running after a 3000ms delay, a
>> +WARN_ON(1) is emitted.
>> +
>> +Required properties:
>> +- compatible : should be "gpio-restart".
>> +- 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 machine_restart function is called. If this
>> optional
>> +  property is not specified, the GPIO is initialized as an output in its
>> +  inactive state.
>
>
> Maybe describe this as open source ?

Done.

>> +- priority : A priority ranging from 0 to 255 (default 128) according to
>> +  the following guidelines:
>> +       0:      Restart handler of last resort, with limited restart
>> +               capabilities
>> +       128:    Default restart handler; use if no other restart handler
>> is
>> +               expected to be available, and/or if restart functionality
>> is
>> +               sufficient to restart the entire system
>> +       255:    Highest priority restart handler, will preempt all other
>> +               restart handlers
>> +
>> +Examples:
>> +
>> +gpio-restart {
>> +       compatible = "gpio-restart";
>> +       gpios = <&gpio 4 0>;
>> +       priority = /bits/ 8 <200>;
>> +};
>> diff --git a/drivers/power/reset/Kconfig b/drivers/power/reset/Kconfig
>> index ca41523..f07e26c 100644
>> --- a/drivers/power/reset/Kconfig
>> +++ b/drivers/power/reset/Kconfig
>> @@ -39,6 +39,14 @@ config POWER_RESET_GPIO
>>           If your board needs a GPIO high/low to power down, say Y and
>>           create a binding in your devicetree.
>>
>> +config POWER_RESET_GPIO_RESTART
>> +       bool "GPIO restart driver"
>> +       depends on OF_GPIO && POWER_RESET
>> +       help
>> +         This driver supports restarting your board via a GPIO line.
>> +         If your board needs a GPIO high/low to restart, say Y and
>> +         create a binding in your devicetree.
>> +
>>   config POWER_RESET_HISI
>>         bool "Hisilicon power-off driver"
>>         depends on POWER_RESET && ARCH_HISI
>> diff --git a/drivers/power/reset/Makefile b/drivers/power/reset/Makefile
>> index a42e70e..199cb6e 100644
>> --- a/drivers/power/reset/Makefile
>> +++ b/drivers/power/reset/Makefile
>> @@ -2,6 +2,7 @@ obj-$(CONFIG_POWER_RESET_AS3722) += as3722-poweroff.o
>>   obj-$(CONFIG_POWER_RESET_AXXIA) += axxia-reset.o
>>   obj-$(CONFIG_POWER_RESET_BRCMSTB) += brcmstb-reboot.o
>>   obj-$(CONFIG_POWER_RESET_GPIO) += gpio-poweroff.o
>> +obj-$(CONFIG_POWER_RESET_GPIO_RESTART) += gpio-restart.o
>>   obj-$(CONFIG_POWER_RESET_HISI) += hisi-reboot.o
>>   obj-$(CONFIG_POWER_RESET_MSM) += msm-poweroff.o
>>   obj-$(CONFIG_POWER_RESET_QNAP) += qnap-poweroff.o
>> diff --git a/drivers/power/reset/gpio-restart.c
>> b/drivers/power/reset/gpio-restart.c
>> new file mode 100644
>> index 0000000..2cbff64
>> --- /dev/null
>> +++ b/drivers/power/reset/gpio-restart.c
>> @@ -0,0 +1,142 @@
>> +/*
>> + * Toggles a GPIO pin to restart a device
>> + *
>> + * Copyright (C) 2014 Google, Inc.
>> + *
>> + * This software is licensed under the terms of the GNU General Public
>> + * License version 2, as published by the Free Software Foundation, and
>> + * may be copied, distributed, and modified under those terms.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + *
>> + * Based on the gpio-poweroff driver.
>> + */
>> +#include <linux/reboot.h>
>> +#include <linux/kernel.h>
>> +#include <linux/init.h>
>> +#include <linux/delay.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/gpio/consumer.h>
>> +#include <linux/of_platform.h>
>> +#include <linux/module.h>
>> +
>> +struct gpio_restart {
>> +       struct gpio_desc *reset_gpio;
>> +       struct notifier_block restart_handler;
>> +};
>> +
>> +static int gpio_restart_notify(struct notifier_block *this,
>> +                               unsigned long mode, void *cmd)
>> +{
>> +       struct gpio_restart *gpio_restart =
>> +               container_of(this, struct gpio_restart, restart_handler);
>> +
>> +       BUG_ON(!gpio_restart->reset_gpio);
>> +
>
> Is this really necessary ? First, it can't really happen,
> but if it happens anyway I am not sure if there might be
> a possibility for recursiveness. After all, this is called
> from machine_restart().

I've removed it.

>> +       /* drive it active, also inactive->active edge */
>> +       gpiod_direction_output(gpio_restart->reset_gpio, 1);
>> +       mdelay(100);
>> +
>> +       /* drive inactive, also active->inactive edge */
>> +       gpiod_set_value(gpio_restart->reset_gpio, 0);
>> +       mdelay(100);
>> +
>> +       /* drive it active, also inactive->active edge */
>> +       gpiod_set_value(gpio_restart->reset_gpio, 1);
>> +
>> +       /* give it some time */
>> +       mdelay(3000);
>> +
>
> Should those delays be parameters ?

Done.

>
>
>> +       WARN_ON(1);
>> +
>> +       return NOTIFY_DONE;
>> +}
>> +
>> +static int gpio_restart_probe(struct platform_device *pdev)
>> +{
>> +       struct gpio_restart *gpio_restart;
>> +       bool input = false;
>> +       u8 priority;
>> +       int ret;
>> +
>> +       gpio_restart = devm_kzalloc(&pdev->dev, sizeof(*gpio_restart),
>> +                       GFP_KERNEL);
>> +       if (!gpio_restart)
>> +               return -ENOMEM;
>> +
>> +       gpio_restart->reset_gpio = devm_gpiod_get(&pdev->dev, NULL);
>> +       if (IS_ERR(gpio_restart->reset_gpio))
>> +               return PTR_ERR(gpio_restart->reset_gpio);
>> +
>
> There is now a flags parameter to devm_gpiod_get which should help
> handling input/output with a single call.

Done.

>
>> +       gpio_restart->restart_handler.notifier_call = gpio_restart_notify;
>> +       gpio_restart->restart_handler.priority = 128;
>> +
>> +       platform_set_drvdata(pdev, gpio_restart);
>> +
>> +       input = of_property_read_bool(pdev->dev.of_node, "input");
>> +       if (input) {
>> +               if (gpiod_direction_input(gpio_restart->reset_gpio)) {
>> +                       dev_err(&pdev->dev,
>> +                               "Could not set direction of reset GPIO to
>> input\n");
>> +                       return -ENODEV;
>> +               }
>> +       } else {
>> +               if (gpiod_direction_output(gpio_restart->reset_gpio, 0)) {
>> +                       dev_err(&pdev->dev,
>> +                               "Could not set direction of reset
>> GPIO\n");
>> +                       return -ENODEV;
>> +               }
>> +       }
>> +
>> +       if (!of_property_read_u8(pdev->dev.of_node, "priority",
>> &priority))
>> +               gpio_restart->restart_handler.priority = priority;
>> +
>> +       ret = register_restart_handler(&gpio_restart->restart_handler);
>> +       if (ret) {
>> +               dev_err(&pdev->dev, "%s: cannot register restart handler,
>> %d\n",
>> +                               __func__, ret);
>> +               return -ENODEV;
>> +       }
>> +
>> +       return 0;
>> +}
>> +
>> +static int gpio_restart_remove(struct platform_device *pdev)
>> +{
>> +       struct gpio_restart *gpio_restart = platform_get_drvdata(pdev);
>> +       int ret;
>> +
>> +       ret = unregister_restart_handler(&gpio_restart->restart_handler);
>> +       if (ret) {
>> +               dev_err(&pdev->dev,
>> +                               "%s: cannot unregister restart handler,
>> %d\n",
>> +                               __func__, ret);
>> +               return -ENODEV;
>> +       }
>> +
>> +       return 0;
>> +}
>> +
>> +static const struct of_device_id of_gpio_restart_match[] = {
>> +       { .compatible = "gpio-restart", },
>> +       {},
>> +};
>> +
>> +static struct platform_driver gpio_restart_driver = {
>> +       .probe = gpio_restart_probe,
>> +       .remove = gpio_restart_remove,
>> +       .driver = {
>> +               .name = "restart-gpio",
>> +               .owner = THIS_MODULE,
>> +               .of_match_table = of_gpio_restart_match,
>> +       },
>> +};
>> +
>> +module_platform_driver(gpio_restart_driver);
>> +
>> +MODULE_AUTHOR("David Riley <davidriley@chromium.org>");
>> +MODULE_DESCRIPTION("GPIO restart driver");
>> +MODULE_LICENSE("GPL");
>>
>

Re: [PATCH v1 1/1] power: Add simple gpio-restart driver

[Guenter Roeck]

On Wed, Aug 27, 2014 at 10:56:20AM -0700, David Riley wrote:
> Hi Sebastian,
> 
> Thanks for the feedback.
> 
> On Tue, Aug 26, 2014 at 6:43 PM, Sebastian Reichel <sre@kernel.org> wrote:
> > Hi David,
> >
> > On Tue, Aug 26, 2014 at 04:45:05PM -0700, David Riley wrote:
> >> This driver registers a restart handler to set a GPIO line high/low
> >> to reset a board based on devicetree bindings.
> >
> > Driver looks fine to me. I have some comments about the
> > Documentation, though:
> >
> >> [...]
> >> diff --git a/Documentation/devicetree/bindings/gpio/gpio-restart.txt b/Documentation/devicetree/bindings/gpio/gpio-restart.txt
> >> new file mode 100644
> >> index 0000000..7cd58788
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/gpio/gpio-restart.txt
> >> @@ -0,0 +1,48 @@
> >> +Driver a GPIO line that can be used to restart the system as a
> >> +restart handler.
> >
> > Please fix the Typo (first word).
> 
> Fixed.
> 
> >
> >> [...]
> >> +
> >> +The driver supports both level triggered and edge triggered power off.
> >> +At driver load time, the driver will request the given gpio line and
> >> +install a restart handler.
> >
> > The wording is too driver centric IMHO. You are supposed to document
> > the binding in a generic way. Maybe start with something like:
> >
> > "This binding supports level and edge triggered reset."
> >
> > (power off is the wrong word, since there is already gpio-poweroff).
> 
> I've cleaned this up for v2.
> 
> >> +If the optional properties 'input' is +not found, the GPIO line
> >> +will be driven in the inactive state. Otherwise its configured
> >> +as an input.
> >
> > What is this needed for?
> 
> This allows other hardware to be attached to the same line to reset
> the system.  Carried forward from the gpio-poweroff implementation I
> based this on.
> 
Maybe rephrase; the point isn't really that it is configured as input but
that it is an open source pin unless actively driven to reset the system.
That linux models the pin as input is really an implementation detail.

Thanks,
Guenter