[PATCH] power: reset: add PowerPath poweroff support

[PATCH] power: reset: add PowerPath poweroff support

[René Moll]

Signed-off-by: René Moll <rene.moll@xsens.com>

---
 drivers/power/reset/Kconfig              |   9 +
 drivers/power/reset/Makefile             |   1 +
 drivers/power/reset/powerpath-poweroff.c | 418 +++++++++++++++++++++++++++++++
 3 files changed, 428 insertions(+)
 create mode 100644 drivers/power/reset/powerpath-poweroff.c

diff --git a/drivers/power/reset/Kconfig b/drivers/power/reset/Kconfig
index 6d452a7..83ce507 100644
--- a/drivers/power/reset/Kconfig
+++ b/drivers/power/reset/Kconfig
@@ -26,6 +26,15 @@ config POWER_RESET_MSM
 	help
 	  Power off and restart support for Qualcomm boards.
 
+config POWER_RESET_POWERPATH
+	tristate "PowerPath power-off driver"
+	depends on OF_GPIO && POWER_RESET
+	help
+	  Supports powerdown with an external PowerPath IC.
+
 config POWER_RESET_QNAP
 	bool "QNAP power-off driver"
 	depends on OF_GPIO && POWER_RESET && PLAT_ORION
diff --git a/drivers/power/reset/Makefile b/drivers/power/reset/Makefile
index a5b4a77..54a1f53 100644
--- a/drivers/power/reset/Makefile
+++ b/drivers/power/reset/Makefile
@@ -1,6 +1,7 @@
 obj-$(CONFIG_POWER_RESET_AS3722) += as3722-poweroff.o
 obj-$(CONFIG_POWER_RESET_GPIO) += gpio-poweroff.o
 obj-$(CONFIG_POWER_RESET_MSM) += msm-poweroff.o
+obj-$(CONFIG_POWER_RESET_POWERPATH) += powerpath-poweroff.o
 obj-$(CONFIG_POWER_RESET_QNAP) += qnap-poweroff.o
 obj-$(CONFIG_POWER_RESET_RESTART) += restart-poweroff.o
 obj-$(CONFIG_POWER_RESET_VEXPRESS) += vexpress-poweroff.o
diff --git a/drivers/power/reset/powerpath-poweroff.c b/drivers/power/reset/powerpath-poweroff.c
new file mode 100644
index 0000000..c902827
--- /dev/null
+++ b/drivers/power/reset/powerpath-poweroff.c
@@ -0,0 +1,418 @@
+/*
+ * PowerPath driver
+ *
+ * Copyright (C) 2014, Xsens Technologies BV <info@xsens.com>
+ * Maintainer: René Moll <linux@r-moll.nl>
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * 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.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * ----------------------------------------
+ * - Description
+ * ----------------------------------------
+ *
+ * This driver is to be used with an external PowerPath Controller.
+ * Its function is to determine when a external shut down is triggered
+ * and react by properly shutting down the system.
+ *
+ * This driver expects a device tree with a powerpath entry for pin mapping.
+ *
+ * Example:
+ *
+ * powerpath {
+ *	compatible = "powerpath,ltc2952";
+ *
+ *	trigger-gpios = <&gpio0 16 GPIO_ACTIVE_LOW>;
+ *	watchdog-gpios = <&gpio3 4 GPIO_ACTIVE_HIGH>;
+ *	kill-gpios = <&gpio0 2 GPIO_ACTIVE_LOW>;
+ * };
+ *
+ * ----------------------------------------
+ * - GPIO
+ * ----------------------------------------
+ *
+ * The following GPIOs are used:
+ * - trigger (input)
+ *     A level change indicates the shut-down trigger. If it's state reverts
+ *     within the time-out defined by trigger_delay, the shut down is not
+ *     executed.
+ *
+ * - watchdog (output)
+ *     Once a shut down is triggered, the driver will toggle this signal,
+ *     with an internal (wde_interval) to stall the hardware shut down.
+ *
+ * - kill (output)
+ *     The last action during shut down is triggering this signalling, such
+ *     that the PowerPath Control will power down the hardware.
+ *
+ * ----------------------------------------
+ * - Interrupts
+ * ----------------------------------------
+ *
+ * The driver requires a non-shared, edge-triggered interrupt on the trigger
+ * GPIO.
+ *
+ */
+
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/device.h>
+#include <linux/platform_device.h>
+#include <linux/ktime.h>
+#include <linux/slab.h>
+#include <linux/kmod.h>
+#include <linux/module.h>
+#include <linux/gpio/consumer.h>
+#include <linux/reboot.h>
+
+static int powerpath_poweroff_panic;
+
+struct powerpath_poweroff_data {
+	struct hrtimer timer_trigger;
+	struct hrtimer timer_wde;
+
+	ktime_t trigger_delay;
+	ktime_t wde_interval;
+
+	struct device *dev;
+
+	unsigned int virq;
+
+	/**
+	 * 0: trigger
+	 * 1: watchdog
+	 * 2: kill
+	 */
+	struct gpio_desc *gpio[3];
+};
+
+#define POWERPATH_IO_TRIGGER	0
+#define POWERPATH_IO_WATCHDOG	1
+#define POWERPATH_IO_KILL	2
+
+/**
+ * powerpath_poweroff_timer_wde - Timer callback
+ * Toggles the watchdog reset signal each wde_interval
+ *
+ * @timer: corresponding timer
+ *
+ * Returns HRTIMER_RESTART for an infinite loop which will only stop when the
+ * machine actually shuts down
+ */
+static enum hrtimer_restart powerpath_poweroff_timer_wde(struct hrtimer *timer)
+{
+	ktime_t now;
+	int state;
+	unsigned long overruns;
+	struct powerpath_poweroff_data *data = container_of(timer,
+		struct powerpath_poweroff_data, timer_wde);
+
+	if (powerpath_poweroff_panic)
+		return HRTIMER_NORESTART;
+
+	state = gpiod_get_value(data->gpio[POWERPATH_IO_WATCHDOG]);
+	gpiod_set_value(data->gpio[POWERPATH_IO_WATCHDOG], !state);
+
+	now = hrtimer_cb_get_time(timer);
+	overruns = hrtimer_forward(timer, now, data->wde_interval);
+
+	return HRTIMER_RESTART;
+}
+
+static enum hrtimer_restart powerpath_poweroff_timer_trigger(
+	struct hrtimer *timer)
+{
+	int ret;
+	struct powerpath_poweroff_data *data = container_of(timer,
+		struct powerpath_poweroff_data, timer_trigger);
+
+	/* Using halt here as shutdown does not work properly under BusyBox */
+	static char *shutdown_argv[] = { "/sbin/halt", NULL };
+	static char *shutdown_envp[] = {
+		"HOME=/",
+		"TERM=linux",
+		"PATH=/sbin:/bin:/usr/sbin:/usr/bin", NULL
+	};
+
+	ret = hrtimer_start(&data->timer_wde, data->wde_interval,
+		HRTIMER_MODE_REL);
+
+	if (ret) {
+		dev_err(data->dev, "unable to start the timer\n");
+		/*
+		 * The device will not toggle the watchdog reset,
+		 * thus shut down is only safe if the PowerPath controller
+		 * has a long enough time-off before triggering a hardware
+		 * power-off.
+		 *
+		 * Only sending a warning as the system will power-off anyway
+		 */
+	}
+
+	dev_info(data->dev, "executing shutdown\n");
+
+	call_usermodehelper(shutdown_argv[0], shutdown_argv,
+		shutdown_envp, UMH_NO_WAIT);
+
+	return HRTIMER_NORESTART;
+}
+
+/**
+ * powerpath_poweroff_handler - Interrupt handler
+ * Triggered each time the trigger signal changes state and (de)activates a
+ * time-out (timer_trigger). Once the time-out is actually reached the shut
+ * down is executed.
+ *
+ * @irq: IRQ number
+ * @dev_id: pointer to the main data structure
+ */
+static irqreturn_t powerpath_poweroff_handler(int irq, void *dev_id)
+{
+	int ret;
+	struct powerpath_poweroff_data *data = dev_id;
+
+	if (powerpath_poweroff_panic)
+		return IRQ_HANDLED;
+
+	if (hrtimer_active(&data->timer_wde)) {
+		/* shutdown is already triggered, nothing to do any more */
+		return IRQ_HANDLED;
+	}
+
+	if (!hrtimer_active(&data->timer_trigger)) {
+		ret = hrtimer_start(&data->timer_trigger, data->trigger_delay,
+			HRTIMER_MODE_REL);
+
+		if (ret)
+			dev_err(data->dev, "unable to start the wait timer\n");
+
+		return IRQ_HANDLED;
+	} else {
+		ret = hrtimer_cancel(&data->timer_trigger);
+		/* omitting return value check, timer should have been valid */
+
+		return IRQ_HANDLED;
+	}
+
+	return IRQ_NONE;
+}
+
+static void powerpath_poweroff_kill(struct platform_device *pdev)
+{
+	struct powerpath_poweroff_data *data = platform_get_drvdata(pdev);
+
+	gpiod_set_value(data->gpio[POWERPATH_IO_KILL], 1);
+}
+
+static void powerpath_poweroff_shutdown(struct platform_device *pdev)
+{
+	if (system_state == SYSTEM_RESTART || system_state == SYSTEM_BOOTING)
+		return;
+
+	powerpath_poweroff_kill(pdev);
+}
+
+static int powerpath_poweroff_suspend(struct platform_device *pdev,
+	pm_message_t state)
+{
+	return -ENOSYS;
+}
+
+static int powerpath_poweroff_resume(struct platform_device *pdev)
+{
+	return -ENOSYS;
+}
+
+static void powerpath_poweroff_default(struct powerpath_poweroff_data *data)
+{
+	unsigned int i;
+
+	for (i = 0; i < ARRAY_SIZE(data->gpio); i++)
+		data->gpio[i] = NULL;
+
+	data->wde_interval = ktime_set(0, 300L*1E6L);
+	data->trigger_delay = ktime_set(2, 500L*1E6L);
+
+	hrtimer_init(&data->timer_trigger, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
+	data->timer_trigger.function = &powerpath_poweroff_timer_trigger;
+
+	hrtimer_init(&data->timer_wde, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
+	data->timer_wde.function = &powerpath_poweroff_timer_wde;
+}
+
+static int powerpath_poweroff_init(struct platform_device *pdev,
+	struct powerpath_poweroff_data *data)
+{
+	int ret, virq;
+	unsigned int i;
+
+	static char *name[] = {
+		"trigger",
+		"watchdog",
+		"kill",
+		NULL
+	};
+
+	powerpath_poweroff_default(data);
+
+	for (i = 0; i < ARRAY_SIZE(data->gpio); i++) {
+		data->gpio[i] = gpiod_get(&pdev->dev, name[i]);
+
+		if (IS_ERR(data->gpio[i])) {
+			ret = PTR_ERR(data->gpio[i]);
+			dev_err(&pdev->dev,
+				"unable to claim the following gpio: %s\n",
+				name[i]);
+			goto err;
+		}
+	}
+
+	ret = gpiod_direction_output(data->gpio[POWERPATH_IO_WATCHDOG], 0);
+	if (ret) {
+		dev_err(&pdev->dev, "unable to use watchdog-gpio as output\n");
+		goto err_io;
+	}
+
+	ret = gpiod_direction_output(data->gpio[POWERPATH_IO_KILL], 0);
+	if (ret) {
+		dev_err(&pdev->dev, "unable to use kill-gpio as output\n");
+		goto err_io;
+	}
+
+	virq = gpiod_to_irq(data->gpio[POWERPATH_IO_TRIGGER]);
+	if (virq < 0) {
+		dev_err(&pdev->dev, "cannot map GPIO as interrupt");
+		goto err_io;
+	}
+
+	data->virq = virq;
+	ret = request_irq(virq,
+		powerpath_poweroff_handler,
+		(IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING),
+		"powerpath-poweroff",
+		data
+	);
+
+	if (ret) {
+		dev_err(&pdev->dev, "cannot configure an interrupt handler\n");
+		goto err_io;
+	}
+
+	return 0;
+
+err_io:
+	for (i = 0; i < ARRAY_SIZE(data->gpio); i++)
+		gpiod_put(data->gpio[i]);
+
+err:
+	return ret;
+}
+
+static int powerpath_poweroff_probe(struct platform_device *pdev)
+{
+	int ret;
+	struct powerpath_poweroff_data *data;
+
+	data = kzalloc(sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, data);
+	data->dev = &pdev->dev;
+
+	ret = powerpath_poweroff_init(pdev, data);
+	if (ret)
+		goto err;
+
+	dev_info(&pdev->dev, "probe successful\n");
+
+	return 0;
+
+err:
+	kfree(data);
+	return ret;
+}
+
+static int powerpath_poweroff_remove(struct platform_device *pdev)
+{
+	unsigned int i;
+	struct powerpath_poweroff_data *data = platform_get_drvdata(pdev);
+
+	if (data) {
+		free_irq(data->virq, data);
+
+		for (i = 0; i < ARRAY_SIZE(data->gpio); i++)
+			gpiod_put(data->gpio[i]);
+
+		kfree(data);
+	}
+
+	return 0;
+}
+
+static const struct of_device_id of_powerpath_poweroff_match[] = {
+	{ .compatible = "powerpath,ltc2952" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, of_powerpath_poweroff_match);
+
+static struct platform_driver powerpath_poweroff_driver = {
+	.probe = powerpath_poweroff_probe,
+	.remove = powerpath_poweroff_remove,
+	.driver = {
+		.name = "powerpath-poweroff",
+		.owner = THIS_MODULE,
+		.of_match_table = of_powerpath_poweroff_match,
+	},
+	.suspend = powerpath_poweroff_suspend,
+	.resume = powerpath_poweroff_resume,
+	.shutdown = powerpath_poweroff_shutdown,
+};
+
+static int powerpath_poweroff_notify_panic(struct notifier_block *nb,
+	unsigned long code, void *unused)
+{
+	powerpath_poweroff_panic = 1;
+	return NOTIFY_DONE;
+}
+
+static struct notifier_block powerpath_poweroff_panic_nb = {
+	.notifier_call = powerpath_poweroff_notify_panic,
+};
+
+static int __init powerpath_poweroff_platform_init(void)
+{
+	powerpath_poweroff_panic = 0;
+
+	atomic_notifier_chain_register(&panic_notifier_list,
+		&powerpath_poweroff_panic_nb);
+
+	return platform_driver_register(&powerpath_poweroff_driver);
+}
+
+static void __exit powerpath_poweroff_platform_exit(void)
+{
+	atomic_notifier_chain_unregister(&panic_notifier_list,
+		&powerpath_poweroff_panic_nb);
+
+	platform_driver_unregister(&powerpath_poweroff_driver);
+}
+
+module_init(powerpath_poweroff_platform_init);
+module_exit(powerpath_poweroff_platform_exit);
+
+MODULE_AUTHOR("René Moll <rene.moll@xsens.com>");
+MODULE_DESCRIPTION("PowerPath power-off driver");
+MODULE_LICENSE("GPL v2");
+
-- 
1.9.1

Re: [PATCH] power: reset: add PowerPath poweroff support

[Mikko Perttunen]

Not my subsystem, but some general points..

On 07/08/14 15:44, René Moll wrote:
>
> Signed-off-by: René Moll <rene.moll@xsens.com>

Please add a commit message. You should describe the hardware, it's 
capabilities and what features this driver implements.

>
> ---
>   drivers/power/reset/Kconfig              |   9 +
>   drivers/power/reset/Makefile             |   1 +
>   drivers/power/reset/powerpath-poweroff.c | 418 +++++++++++++++++++++++++++++++
>   3 files changed, 428 insertions(+)
>   create mode 100644 drivers/power/reset/powerpath-poweroff.c
>
> diff --git a/drivers/power/reset/Kconfig b/drivers/power/reset/Kconfig
> index 6d452a7..83ce507 100644
> --- a/drivers/power/reset/Kconfig
> +++ b/drivers/power/reset/Kconfig
> @@ -26,6 +26,15 @@ config POWER_RESET_MSM
>          help
>            Power off and restart support for Qualcomm boards.
>
> +config POWER_RESET_POWERPATH
> +       tristate "PowerPath power-off driver"
> +       depends on OF_GPIO && POWER_RESET
> +       help
> +         Supports powerdown with an external PowerPath IC.
> +
>   config POWER_RESET_QNAP
>          bool "QNAP power-off driver"
>          depends on OF_GPIO && POWER_RESET && PLAT_ORION
> diff --git a/drivers/power/reset/Makefile b/drivers/power/reset/Makefile
> index a5b4a77..54a1f53 100644
> --- a/drivers/power/reset/Makefile
> +++ b/drivers/power/reset/Makefile
> @@ -1,6 +1,7 @@
>   obj-$(CONFIG_POWER_RESET_AS3722) += as3722-poweroff.o
>   obj-$(CONFIG_POWER_RESET_GPIO) += gpio-poweroff.o
>   obj-$(CONFIG_POWER_RESET_MSM) += msm-poweroff.o
> +obj-$(CONFIG_POWER_RESET_POWERPATH) += powerpath-poweroff.o
>   obj-$(CONFIG_POWER_RESET_QNAP) += qnap-poweroff.o
>   obj-$(CONFIG_POWER_RESET_RESTART) += restart-poweroff.o
>   obj-$(CONFIG_POWER_RESET_VEXPRESS) += vexpress-poweroff.o
> diff --git a/drivers/power/reset/powerpath-poweroff.c b/drivers/power/reset/powerpath-poweroff.c
> new file mode 100644
> index 0000000..c902827
> --- /dev/null
> +++ b/drivers/power/reset/powerpath-poweroff.c
> @@ -0,0 +1,418 @@
> +/*
> + * PowerPath driver
> + *
> + * Copyright (C) 2014, Xsens Technologies BV <info@xsens.com>
> + * Maintainer: René Moll <linux@r-moll.nl>
> + *
> + * This program is free software: you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation, either version 3 of the License, or
> + * (at your option) any later version.

The kernel is GPL v2.

> + *
> + * 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.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
> + *
> + * ----------------------------------------
> + * - Description
> + * ----------------------------------------
> + *
> + * This driver is to be used with an external PowerPath Controller.
> + * Its function is to determine when a external shut down is triggered
> + * and react by properly shutting down the system.
> + *
> + * This driver expects a device tree with a powerpath entry for pin mapping.
> + *
> + * Example:
> + *
> + * powerpath {
> + *     compatible = "powerpath,ltc2952";

Looks like this chip is made by Linear Technology, for which the vendor 
prefix is "lltc". So the compatibility string should be "lltc,ltc2952".

> + *
> + *     trigger-gpios = <&gpio0 16 GPIO_ACTIVE_LOW>;
> + *     watchdog-gpios = <&gpio3 4 GPIO_ACTIVE_HIGH>;
> + *     kill-gpios = <&gpio0 2 GPIO_ACTIVE_LOW>;
> + * };
> + *
> + * ----------------------------------------
> + * - GPIO
> + * ----------------------------------------
> + *
> + * The following GPIOs are used:
> + * - trigger (input)
> + *     A level change indicates the shut-down trigger. If it's state reverts
> + *     within the time-out defined by trigger_delay, the shut down is not
> + *     executed.
> + *
> + * - watchdog (output)
> + *     Once a shut down is triggered, the driver will toggle this signal,
> + *     with an internal (wde_interval) to stall the hardware shut down.
> + *
> + * - kill (output)
> + *     The last action during shut down is triggering this signalling, such
> + *     that the PowerPath Control will power down the hardware.
> + *
> + * ----------------------------------------
> + * - Interrupts
> + * ----------------------------------------
> + *
> + * The driver requires a non-shared, edge-triggered interrupt on the trigger
> + * GPIO.

You should add a description of the hardware and device tree node along 
with properties (like this one!) under Documentation/devicetree/bindings/...
Don't refer to Linux-specific driver implementation details, though.

> + *
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/device.h>
> +#include <linux/platform_device.h>
> +#include <linux/ktime.h>
> +#include <linux/slab.h>
> +#include <linux/kmod.h>
> +#include <linux/module.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/reboot.h>
> +
> +static int powerpath_poweroff_panic;
> +
> +struct powerpath_poweroff_data {
> +       struct hrtimer timer_trigger;
> +       struct hrtimer timer_wde;
> +
> +       ktime_t trigger_delay;
> +       ktime_t wde_interval;
> +
> +       struct device *dev;
> +
> +       unsigned int virq;
> +
> +       /**
> +        * 0: trigger
> +        * 1: watchdog
> +        * 2: kill
> +        */
> +       struct gpio_desc *gpio[3];
> +};
> +
> +#define POWERPATH_IO_TRIGGER   0
> +#define POWERPATH_IO_WATCHDOG  1
> +#define POWERPATH_IO_KILL      2
> +
> +/**
> + * powerpath_poweroff_timer_wde - Timer callback
> + * Toggles the watchdog reset signal each wde_interval
> + *
> + * @timer: corresponding timer
> + *
> + * Returns HRTIMER_RESTART for an infinite loop which will only stop when the
> + * machine actually shuts down
> + */
> +static enum hrtimer_restart powerpath_poweroff_timer_wde(struct hrtimer *timer)
> +{
> +       ktime_t now;
> +       int state;
> +       unsigned long overruns;
> +       struct powerpath_poweroff_data *data = container_of(timer,
> +               struct powerpath_poweroff_data, timer_wde);
> +
> +       if (powerpath_poweroff_panic)
> +               return HRTIMER_NORESTART;
> +
> +       state = gpiod_get_value(data->gpio[POWERPATH_IO_WATCHDOG]);
> +       gpiod_set_value(data->gpio[POWERPATH_IO_WATCHDOG], !state);
> +
> +       now = hrtimer_cb_get_time(timer);
> +       overruns = hrtimer_forward(timer, now, data->wde_interval);
> +
> +       return HRTIMER_RESTART;
> +}
> +
> +static enum hrtimer_restart powerpath_poweroff_timer_trigger(
> +       struct hrtimer *timer)
> +{
> +       int ret;
> +       struct powerpath_poweroff_data *data = container_of(timer,
> +               struct powerpath_poweroff_data, timer_trigger);
> +
> +       /* Using halt here as shutdown does not work properly under BusyBox */
> +       static char *shutdown_argv[] = { "/sbin/halt", NULL };
> +       static char *shutdown_envp[] = {
> +               "HOME=/",
> +               "TERM=linux",
> +               "PATH=/sbin:/bin:/usr/sbin:/usr/bin", NULL
> +       };
> +
> +       ret = hrtimer_start(&data->timer_wde, data->wde_interval,
> +               HRTIMER_MODE_REL);
> +
> +       if (ret) {
> +               dev_err(data->dev, "unable to start the timer\n");
> +               /*
> +                * The device will not toggle the watchdog reset,
> +                * thus shut down is only safe if the PowerPath controller
> +                * has a long enough time-off before triggering a hardware
> +                * power-off.
> +                *
> +                * Only sending a warning as the system will power-off anyway
> +                */
> +       }
> +
> +       dev_info(data->dev, "executing shutdown\n");
> +
> +       call_usermodehelper(shutdown_argv[0], shutdown_argv,
> +               shutdown_envp, UMH_NO_WAIT);

Any reason not to use orderly_poweroff()?

> +
> +       return HRTIMER_NORESTART;
> +}
> +
> +/**
> + * powerpath_poweroff_handler - Interrupt handler
> + * Triggered each time the trigger signal changes state and (de)activates a
> + * time-out (timer_trigger). Once the time-out is actually reached the shut
> + * down is executed.
> + *
> + * @irq: IRQ number
> + * @dev_id: pointer to the main data structure
> + */
> +static irqreturn_t powerpath_poweroff_handler(int irq, void *dev_id)
> +{
> +       int ret;
> +       struct powerpath_poweroff_data *data = dev_id;
> +
> +       if (powerpath_poweroff_panic)
> +               return IRQ_HANDLED;
> +
> +       if (hrtimer_active(&data->timer_wde)) {
> +               /* shutdown is already triggered, nothing to do any more */
> +               return IRQ_HANDLED;
> +       }
> +
> +       if (!hrtimer_active(&data->timer_trigger)) {
> +               ret = hrtimer_start(&data->timer_trigger, data->trigger_delay,
> +                       HRTIMER_MODE_REL);
> +
> +               if (ret)
> +                       dev_err(data->dev, "unable to start the wait timer\n");
> +
> +               return IRQ_HANDLED;
> +       } else {
> +               ret = hrtimer_cancel(&data->timer_trigger);
> +               /* omitting return value check, timer should have been valid */
> +
> +               return IRQ_HANDLED;
> +       }
> +
> +       return IRQ_NONE;
> +}

Looks like this function always returns IRQ_HANDLED, so you could 
restructure it a bit.

> +
> +static void powerpath_poweroff_kill(struct platform_device *pdev)
> +{
> +       struct powerpath_poweroff_data *data = platform_get_drvdata(pdev);
> +
> +       gpiod_set_value(data->gpio[POWERPATH_IO_KILL], 1);
> +}
> +
> +static void powerpath_poweroff_shutdown(struct platform_device *pdev)
> +{
> +       if (system_state == SYSTEM_RESTART || system_state == SYSTEM_BOOTING)
> +               return;
> +
> +       powerpath_poweroff_kill(pdev);
> +}
> +
> +static int powerpath_poweroff_suspend(struct platform_device *pdev,
> +       pm_message_t state)
> +{
> +       return -ENOSYS;
> +}
> +
> +static int powerpath_poweroff_resume(struct platform_device *pdev)
> +{
> +       return -ENOSYS;
> +}
> +
> +static void powerpath_poweroff_default(struct powerpath_poweroff_data *data)
> +{
> +       unsigned int i;
> +
> +       for (i = 0; i < ARRAY_SIZE(data->gpio); i++)
> +               data->gpio[i] = NULL;
> +
> +       data->wde_interval = ktime_set(0, 300L*1E6L);
> +       data->trigger_delay = ktime_set(2, 500L*1E6L);
> +
> +       hrtimer_init(&data->timer_trigger, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
> +       data->timer_trigger.function = &powerpath_poweroff_timer_trigger;
> +
> +       hrtimer_init(&data->timer_wde, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
> +       data->timer_wde.function = &powerpath_poweroff_timer_wde;
> +}
> +
> +static int powerpath_poweroff_init(struct platform_device *pdev,
> +       struct powerpath_poweroff_data *data)
> +{
> +       int ret, virq;
> +       unsigned int i;
> +
> +       static char *name[] = {
> +               "trigger",
> +               "watchdog",
> +               "kill",
> +               NULL
> +       };
> +
> +       powerpath_poweroff_default(data);
> +
> +       for (i = 0; i < ARRAY_SIZE(data->gpio); i++) {
> +               data->gpio[i] = gpiod_get(&pdev->dev, name[i]);
> +
> +               if (IS_ERR(data->gpio[i])) {
> +                       ret = PTR_ERR(data->gpio[i]);
> +                       dev_err(&pdev->dev,
> +                               "unable to claim the following gpio: %s\n",
> +                               name[i]);
> +                       goto err;
> +               }
> +       }

If an error occurs mid-loop, this can leave some gpios get'ted. I 
recommend switching to devm_gpiod_get.

> +
> +       ret = gpiod_direction_output(data->gpio[POWERPATH_IO_WATCHDOG], 0);
> +       if (ret) {
> +               dev_err(&pdev->dev, "unable to use watchdog-gpio as output\n");
> +               goto err_io;
> +       }
> +
> +       ret = gpiod_direction_output(data->gpio[POWERPATH_IO_KILL], 0);
> +       if (ret) {
> +               dev_err(&pdev->dev, "unable to use kill-gpio as output\n");
> +               goto err_io;
> +       }
> +
> +       virq = gpiod_to_irq(data->gpio[POWERPATH_IO_TRIGGER]);
> +       if (virq < 0) {
> +               dev_err(&pdev->dev, "cannot map GPIO as interrupt");
> +               goto err_io;
> +       }
> +
> +       data->virq = virq;
> +       ret = request_irq(virq,
> +               powerpath_poweroff_handler,
> +               (IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING),
> +               "powerpath-poweroff",
> +               data
> +       );
> +
> +       if (ret) {
> +               dev_err(&pdev->dev, "cannot configure an interrupt handler\n");
> +               goto err_io;
> +       }
> +
> +       return 0;
> +
> +err_io:
> +       for (i = 0; i < ARRAY_SIZE(data->gpio); i++)
> +               gpiod_put(data->gpio[i]);
> +
> +err:
> +       return ret;
> +}
> +
> +static int powerpath_poweroff_probe(struct platform_device *pdev)
> +{
> +       int ret;
> +       struct powerpath_poweroff_data *data;
> +
> +       data = kzalloc(sizeof(*data), GFP_KERNEL);

I'd use devm_kzalloc, devm_gpiod_get and devm_request_irq. That would 
clean up the error handling and remove the need for the _remove function.

> +       if (!data)
> +               return -ENOMEM;
> +
> +       platform_set_drvdata(pdev, data);
> +       data->dev = &pdev->dev;
> +
> +       ret = powerpath_poweroff_init(pdev, data);
> +       if (ret)
> +               goto err;
> +
> +       dev_info(&pdev->dev, "probe successful\n");
> +
> +       return 0;
> +
> +err:
> +       kfree(data);
> +       return ret;
> +}
> +
> +static int powerpath_poweroff_remove(struct platform_device *pdev)
> +{
> +       unsigned int i;
> +       struct powerpath_poweroff_data *data = platform_get_drvdata(pdev);
> +
> +       if (data) {
> +               free_irq(data->virq, data);
> +
> +               for (i = 0; i < ARRAY_SIZE(data->gpio); i++)
> +                       gpiod_put(data->gpio[i]);
> +
> +               kfree(data);
> +       }
> +
> +       return 0;
> +}
> +
> +static const struct of_device_id of_powerpath_poweroff_match[] = {
> +       { .compatible = "powerpath,ltc2952" },
> +       {},
> +};
> +MODULE_DEVICE_TABLE(of, of_powerpath_poweroff_match);
> +
> +static struct platform_driver powerpath_poweroff_driver = {
> +       .probe = powerpath_poweroff_probe,
> +       .remove = powerpath_poweroff_remove,
> +       .driver = {
> +               .name = "powerpath-poweroff",
> +               .owner = THIS_MODULE,
> +               .of_match_table = of_powerpath_poweroff_match,
> +       },
> +       .suspend = powerpath_poweroff_suspend,
> +       .resume = powerpath_poweroff_resume,
> +       .shutdown = powerpath_poweroff_shutdown,

Based on other drivers, looks like what you're supposed to do is store a 
function pointer in `pm_power_off'. Also, no need to specify suspend and 
resume if you don't implement them anyway.

> +};
> +
> +static int powerpath_poweroff_notify_panic(struct notifier_block *nb,
> +       unsigned long code, void *unused)
> +{
> +       powerpath_poweroff_panic = 1;
> +       return NOTIFY_DONE;
> +}
> +
> +static struct notifier_block powerpath_poweroff_panic_nb = {
> +       .notifier_call = powerpath_poweroff_notify_panic,
> +};
> +
> +static int __init powerpath_poweroff_platform_init(void)
> +{
> +       powerpath_poweroff_panic = 0;
> +
> +       atomic_notifier_chain_register(&panic_notifier_list,
> +               &powerpath_poweroff_panic_nb);
> +
> +       return platform_driver_register(&powerpath_poweroff_driver);
> +}
> +
> +static void __exit powerpath_poweroff_platform_exit(void)
> +{
> +       atomic_notifier_chain_unregister(&panic_notifier_list,
> +               &powerpath_poweroff_panic_nb);
> +
> +       platform_driver_unregister(&powerpath_poweroff_driver);
> +}
> +
> +module_init(powerpath_poweroff_platform_init);
> +module_exit(powerpath_poweroff_platform_exit);

You can leave a lot of this out by using module_platform_driver()

> +
> +MODULE_AUTHOR("René Moll <rene.moll@xsens.com>");
> +MODULE_DESCRIPTION("PowerPath power-off driver");
> +MODULE_LICENSE("GPL v2");
> +
> --
> 1.9.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Thanks for contributing!

Cheers,
Mikko

RE: [PATCH] power: reset: add PowerPath poweroff support

[René Moll]

Thanks for your response, I will do some refactoring based on your
comments. Also see the inline replies.

> -----Original Message-----
> From: Mikko Perttunen [mailto:mperttunen@nvidia.com]
> Sent: Thursday, August 07, 2014 3:58 PM
> To: René Moll; linux-pm@vger.kernel.org
> Cc: Sebastian Reichel; Dmitry Eremin-Solenikov; David Woodhouse; Frans
> Klaver
> Subject: Re: [PATCH] power: reset: add PowerPath poweroff support
> 
> Not my subsystem, but some general points..
> 
> On 07/08/14 15:44, René Moll wrote:
> >
> > Signed-off-by: René Moll <rene.moll@xsens.com>
> 
> Please add a commit message. You should describe the hardware, it's
> capabilities and what features this driver implements.
> 

Since the driver already has documentation embedded,
this seemed a bit overkill to me.

> > + * the Free Software Foundation, either version 3 of the License, or
> > + * (at your option) any later version.
> 
> The kernel is GPL v2.
> 

Ok, I will correct this.

> > + * The driver requires a non-shared, edge-triggered interrupt on the
> > + trigger
> > + * GPIO.
> 
> You should add a description of the hardware and device tree node along
> with properties (like this one!) under Documentation/devicetree/bindings/...
> Don't refer to Linux-specific driver implementation details, though.
> 

Good point, will add this.

> > +       dev_info(data->dev, "executing shutdown\n");
> > +
> > +       call_usermodehelper(shutdown_argv[0], shutdown_argv,
> > +               shutdown_envp, UMH_NO_WAIT);
> 
> Any reason not to use orderly_poweroff()?
> 

I was not aware of that function, which indeed does exactly what I need.
I will change this.

> > +       powerpath_poweroff_default(data);
> > +
> > +       for (i = 0; i < ARRAY_SIZE(data->gpio); i++) {
> > +               data->gpio[i] = gpiod_get(&pdev->dev, name[i]);
> > +
> > +               if (IS_ERR(data->gpio[i])) {
> > +                       ret = PTR_ERR(data->gpio[i]);
> > +                       dev_err(&pdev->dev,
> > +                               "unable to claim the following gpio: %s\n",
> > +                               name[i]);
> > +                       goto err;
> > +               }
> > +       }
> 
> If an error occurs mid-loop, this can leave some gpios get'ted. I recommend
> switching to devm_gpiod_get.
> 

devm_* functions do not seem to common to me and are sparsely documented,
therefore I preferred the regular functions. This error jump should go to 
err_io and the clean-up loop must check if the gpio entry is actually filled in

> > +static struct platform_driver powerpath_poweroff_driver = {
> > +       .probe = powerpath_poweroff_probe,
> > +       .remove = powerpath_poweroff_remove,
> > +       .driver = {
> > +               .name = "powerpath-poweroff",
> > +               .owner = THIS_MODULE,
> > +               .of_match_table = of_powerpath_poweroff_match,
> > +       },
> > +       .suspend = powerpath_poweroff_suspend,
> > +       .resume = powerpath_poweroff_resume,
> > +       .shutdown = powerpath_poweroff_shutdown,
> 
> Based on other drivers, looks like what you're supposed to do is store a
> function pointer in `pm_power_off'. 

I will use that instead of the shutdown pointer.

> [...] Also, no need to specify suspend and
> resume if you don't implement them anyway.
> 

Actually, this is recommended in Documentation/SubmittingDrivers.txt

> > +static int powerpath_poweroff_notify_panic(struct notifier_block *nb,
> > +       unsigned long code, void *unused) {
> > +       powerpath_poweroff_panic = 1;
> > +       return NOTIFY_DONE;
> > +}
> > +
> > +static struct notifier_block powerpath_poweroff_panic_nb = {
> > +       .notifier_call = powerpath_poweroff_notify_panic, };
> > +
> > +static int __init powerpath_poweroff_platform_init(void)
> > +{
> > +       powerpath_poweroff_panic = 0;
> > +
> > +       atomic_notifier_chain_register(&panic_notifier_list,
> > +               &powerpath_poweroff_panic_nb);
> > +
> > +       return platform_driver_register(&powerpath_poweroff_driver);
> > +}
> > +
> > +static void __exit powerpath_poweroff_platform_exit(void)
> > +{
> > +       atomic_notifier_chain_unregister(&panic_notifier_list,
> > +               &powerpath_poweroff_panic_nb);
> > +
> > +       platform_driver_unregister(&powerpath_poweroff_driver);
> > +}
> > +
> > +module_init(powerpath_poweroff_platform_init);
> > +module_exit(powerpath_poweroff_platform_exit);
> 
> You can leave a lot of this out by using module_platform_driver()
> 

I am aware of that macro, however where to place the notifier calls.
The register call could be placed in the probe function. The unregister call
should then go to the remove function, which you suggested to remove :)

Personally I think this is neater as a panic is a system state, which we
want to know and react upon, but is not part of the driver itself.

> > +
> > +MODULE_AUTHOR("René Moll <rene.moll@xsens.com>");
> > +MODULE_DESCRIPTION("PowerPath power-off driver");
> MODULE_LICENSE("GPL
> > +v2");
> > +
> > --
> > 1.9.1
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> > the body of a message to majordomo@vger.kernel.org More majordomo
> info
> > at  http://vger.kernel.org/majordomo-info.html
> >
> 
> Thanks for contributing!
> 
> Cheers,
> Mikko