[PATCH v3 0/2] Add support for ltc2952 poweroff

[PATCH v3 0/2] Add support for ltc2952 poweroff

[Frans Klaver]

This is version 3 of the series that adds support for the ltc2952 powerpath chip.

This series adds a driver for the LTC2952, an external power control chip, which
signals the OS to shut down. Additionally this driver lets the kernel power
down the board.

This driver was tested with:
  - kernel version 3.15.8, 3.17-rc7
  - ARM based processor (TI AM335x series)

v2..v3:
  - Allow absence of button input pin (and document that)
  - Fix some whitespace issues
  - Straighten out control flow here and there
  - Get rid of gpio array. This was only used in setup/cleanup cases. It never mattered elsewhere
  - Add some explanation to the commit message that introduces the driver
  - Add other contributors
  - Update René's e-mail; his Xsens e-mail is no longer valid
  - Now also tested on v3.17-rc7

René Moll (2):
  power: reset: add LTC2952 poweroff support
  power: reset: document LTC2952 poweroff support

 .../bindings/power/reset/ltc2952-poweroff.txt      |  31 ++
 drivers/power/reset/Kconfig                        |  10 +
 drivers/power/reset/Makefile                       |   1 +
 drivers/power/reset/ltc2952-poweroff.c             | 381 +++++++++++++++++++++
 4 files changed, 423 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/power/reset/ltc2952-poweroff.txt
 create mode 100644 drivers/power/reset/ltc2952-poweroff.c

-- 
2.1.0

[PATCH v3 1/2] power: reset: add LTC2952 poweroff support

[Frans Klaver]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="UTF-8", Size: 13018 bytes --]

From: René Moll <linux@r-moll.nl>

The LTC2952 allows control over system power using a push button. It also
supports powering down the board from a controller on the board.

If the power button is pushed, the ltc2952 starts a 400ms window to
properly shut down the software. This window can be stretched by
toggling a watchdog line. This driver always uses this watchdog line.
The power button input is optional. This allows the system to use a
custom shutdown sequence on the push button.

On software shutdown the driver sets the kill signal, starting the same
sequence as if the push button was used.

Signed-off-by: René Moll <linux@r-moll.nl>
Signed-off-by: Tjerk Hofmeijer <tjerk.hofmeijer@xsens.com>
Signed-off-by: Frans Klaver <frans.klaver@xsens.com>
---
 drivers/power/reset/Kconfig            |  10 +
 drivers/power/reset/Makefile           |   1 +
 drivers/power/reset/ltc2952-poweroff.c | 381 +++++++++++++++++++++++++++++++++
 3 files changed, 392 insertions(+)
 create mode 100644 drivers/power/reset/ltc2952-poweroff.c

diff --git a/drivers/power/reset/Kconfig b/drivers/power/reset/Kconfig
index ca41523..a0f68da 100644
--- a/drivers/power/reset/Kconfig
+++ b/drivers/power/reset/Kconfig
@@ -45,6 +45,16 @@ config POWER_RESET_HISI
 	help
 	  Reboot support for Hisilicon boards.
 
+config POWER_RESET_LTC2952
+	bool "LTC2952 PowerPath power-off driver"
+	depends on OF_GPIO && POWER_RESET
+	help
+	  This driver supports an external powerdown trigger and board
+	  power down via the LTC2952.
+
+	  If your board uses an LTC2952, say  Y and create a binding
+	  in the device tree.
+
 config POWER_RESET_MSM
 	bool "Qualcomm MSM power-off driver"
 	depends on POWER_RESET && ARCH_QCOM
diff --git a/drivers/power/reset/Makefile b/drivers/power/reset/Makefile
index a42e70e..3cbb7c3 100644
--- a/drivers/power/reset/Makefile
+++ b/drivers/power/reset/Makefile
@@ -3,6 +3,7 @@ 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_HISI) += hisi-reboot.o
+obj-$(CONFIG_POWER_RESET_LTC2952) += ltc2952-poweroff.o
 obj-$(CONFIG_POWER_RESET_MSM) += msm-poweroff.o
 obj-$(CONFIG_POWER_RESET_QNAP) += qnap-poweroff.o
 obj-$(CONFIG_POWER_RESET_RESTART) += restart-poweroff.o
diff --git a/drivers/power/reset/ltc2952-poweroff.c b/drivers/power/reset/ltc2952-poweroff.c
new file mode 100644
index 0000000..2cf146a
--- /dev/null
+++ b/drivers/power/reset/ltc2952-poweroff.c
@@ -0,0 +1,381 @@
+/*
+ * LTC2952 PowerPath TM driver
+ *
+ * Copyright (C) 2014, Xsens Technologies BV <info@xsens.com>
+ *
+ * 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 2
+ * 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.
+ *
+ * ----------------------------------------
+ * - Description
+ * ----------------------------------------
+ *
+ * This driver is to be used with an external LTC2952 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 ltc2952 entry for pin mapping.
+ *
+ * ----------------------------------------
+ * - GPIO
+ * ----------------------------------------
+ *
+ * The following GPIOs are used:
+ * - trigger (input, optional)
+ *     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. If no pin is assigned to this input the driver will start
+ *     the watchdog immediately and only a power off event will kill the ltc2952
+ *
+ * - 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>
+
+struct ltc2952_poweroff_data {
+	struct hrtimer timer_trigger;
+	struct hrtimer timer_wde;
+
+	ktime_t trigger_delay;
+	ktime_t wde_interval;
+
+	struct device *dev;
+
+	unsigned int virq;
+
+	struct gpio_desc *gpio_trigger;
+	struct gpio_desc *gpio_watchdog;
+	struct gpio_desc *gpio_kill;
+};
+
+static bool ltc2952_poweroff_panic;
+static struct ltc2952_poweroff_data *ltc2952_data;
+
+/**
+ * ltc2952_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 ltc2952_poweroff_timer_wde(struct hrtimer *timer)
+{
+	ktime_t now;
+	int state;
+	unsigned long overruns;
+
+	if (ltc2952_poweroff_panic)
+		return HRTIMER_NORESTART;
+
+	state = gpiod_get_value(ltc2952_data->gpio_watchdog);
+	gpiod_set_value(ltc2952_data->gpio_watchdog, !state);
+
+	now = hrtimer_cb_get_time(timer);
+	overruns = hrtimer_forward(timer, now, ltc2952_data->wde_interval);
+
+	return HRTIMER_RESTART;
+}
+
+static void ltc2952_poweroff_start_wde(void)
+{
+	int ret;
+
+	ret = hrtimer_start(&ltc2952_data->timer_wde,
+			    ltc2952_data->wde_interval, HRTIMER_MODE_REL);
+
+	if (ret) {
+		dev_err(ltc2952_data->dev,
+			"unable to start the watchdog 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
+		 */
+	}
+}
+
+static enum hrtimer_restart ltc2952_poweroff_timer_trigger(struct hrtimer *t)
+{
+	ltc2952_poweroff_start_wde();
+
+	dev_info(ltc2952_data->dev, "executing shutdown\n");
+
+	orderly_poweroff(true);
+
+	return HRTIMER_NORESTART;
+}
+
+/**
+ * ltc2952_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 ltc2952_poweroff_handler(int irq, void *dev_id)
+{
+	struct ltc2952_poweroff_data *data = dev_id;
+
+	if (ltc2952_poweroff_panic || hrtimer_active(&data->timer_wde))
+		/* we're either panicking, or already shutting down */
+		goto irq_ok;
+
+	if (hrtimer_active(&data->timer_trigger)) {
+		hrtimer_cancel(&data->timer_trigger);
+		goto irq_ok;
+	}
+
+	if (hrtimer_start(&data->timer_trigger, data->trigger_delay,
+			  HRTIMER_MODE_REL))
+		dev_err(data->dev, "unable to start the wait timer\n");
+
+irq_ok:
+	return IRQ_HANDLED;
+}
+
+static void ltc2952_poweroff_kill(void)
+{
+	gpiod_set_value(ltc2952_data->gpio_kill, 1);
+}
+
+static int ltc2952_poweroff_suspend(struct platform_device *pdev,
+				    pm_message_t state)
+{
+	return -ENOSYS;
+}
+
+static int ltc2952_poweroff_resume(struct platform_device *pdev)
+{
+	return -ENOSYS;
+}
+
+static void ltc2952_poweroff_default(struct ltc2952_poweroff_data *data)
+{
+	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 = &ltc2952_poweroff_timer_trigger;
+
+	hrtimer_init(&data->timer_wde, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
+	data->timer_wde.function = &ltc2952_poweroff_timer_wde;
+}
+
+static int ltc2952_poweroff_init(struct platform_device *pdev)
+{
+	int ret;
+
+	ltc2952_poweroff_default(ltc2952_data);
+
+	ltc2952_data->gpio_watchdog = gpiod_get(&pdev->dev, "watchdog");
+	if (IS_ERR(ltc2952_data->gpio_watchdog)) {
+		ret = PTR_ERR(ltc2952_data->gpio_watchdog);
+		dev_err(&pdev->dev, "unable to claim watchdog-gpio\n");
+		goto err;
+	}
+
+	ret = gpiod_direction_output(ltc2952_data->gpio_watchdog, 0);
+	if (ret) {
+		dev_err(&pdev->dev, "unable to use watchdog-gpio as output\n");
+		goto err_watchdog;
+	}
+
+	ltc2952_data->gpio_kill = gpiod_get(&pdev->dev, "kill");
+	if (IS_ERR(ltc2952_data->gpio_kill)) {
+		ret = PTR_ERR(ltc2952_data->gpio_kill);
+		dev_err(&pdev->dev, "unable to claim kill-gpio\n");
+		goto err_watchdog;
+	}
+
+	ret = gpiod_direction_output(ltc2952_data->gpio_kill, 0);
+	if (ret) {
+		dev_err(&pdev->dev, "unable to use kill-gpio as output\n");
+		goto err_kill;
+	}
+
+	ltc2952_data->gpio_trigger = gpiod_get(&pdev->dev, "trigger");
+	if (IS_ERR(ltc2952_data->gpio_trigger)) {
+		ret = PTR_ERR(ltc2952_data->gpio_trigger);
+
+		if (ret != -ENOENT) {
+			dev_err(&pdev->dev, "unable to claim trigger-gpio\n");
+			goto err_kill;
+		}
+
+		dev_info(&pdev->dev, "No trigger input defined\n");
+		ltc2952_data->gpio_trigger = NULL;
+		ltc2952_poweroff_start_wde();
+		return 0;
+	}
+
+	ltc2952_data->virq = gpiod_to_irq(ltc2952_data->gpio_trigger);
+	if (ltc2952_data->virq < 0) {
+		dev_err(&pdev->dev, "cannot map GPIO as interrupt");
+		goto err_trigger;
+	}
+
+	ret = request_irq(ltc2952_data->virq,
+			  ltc2952_poweroff_handler,
+			  (IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING),
+			  "ltc2952-poweroff",
+			  ltc2952_data);
+	if (ret) {
+		dev_err(&pdev->dev, "cannot configure an interrupt handler\n");
+		goto err_trigger;
+	}
+	return 0;
+
+err_trigger:
+	gpiod_put(ltc2952_data->gpio_trigger);
+err_kill:
+	gpiod_put(ltc2952_data->gpio_kill);
+err_watchdog:
+	gpiod_put(ltc2952_data->gpio_watchdog);
+err:
+	return ret;
+}
+
+static int ltc2952_poweroff_probe(struct platform_device *pdev)
+{
+	int ret;
+
+	if (pm_power_off) {
+		dev_err(&pdev->dev, "pm_power_off already registered");
+		return -EBUSY;
+	}
+
+	ltc2952_data = kzalloc(sizeof(*ltc2952_data), GFP_KERNEL);
+	if (!ltc2952_data)
+		return -ENOMEM;
+
+	ltc2952_data->dev = &pdev->dev;
+
+	ret = ltc2952_poweroff_init(pdev);
+	if (ret)
+		goto err;
+
+	pm_power_off = &ltc2952_poweroff_kill;
+
+	dev_info(&pdev->dev, "probe successful\n");
+
+	return 0;
+
+err:
+	kfree(ltc2952_data);
+	return ret;
+}
+
+static int ltc2952_poweroff_remove(struct platform_device *pdev)
+{
+	pm_power_off = NULL;
+
+	if (ltc2952_data) {
+		if (ltc2952_data->gpio_trigger != NULL)
+			free_irq(ltc2952_data->virq, ltc2952_data);
+
+		if (hrtimer_active(&ltc2952_data->timer_wde))
+			hrtimer_cancel(&ltc2952_data->timer_wde);
+
+		gpiod_put(ltc2952_data->gpio_watchdog);
+		gpiod_put(ltc2952_data->gpio_kill);
+		gpiod_put(ltc2952_data->gpio_trigger);
+
+		kfree(ltc2952_data);
+	}
+
+	return 0;
+}
+
+static const struct of_device_id of_ltc2952_poweroff_match[] = {
+	{ .compatible = "lltc,ltc2952"},
+	{},
+};
+MODULE_DEVICE_TABLE(of, of_ltc2952_poweroff_match);
+
+static struct platform_driver ltc2952_poweroff_driver = {
+	.probe = ltc2952_poweroff_probe,
+	.remove = ltc2952_poweroff_remove,
+	.driver = {
+		.name = "ltc2952-poweroff",
+		.owner = THIS_MODULE,
+		.of_match_table = of_ltc2952_poweroff_match,
+	},
+	.suspend = ltc2952_poweroff_suspend,
+	.resume = ltc2952_poweroff_resume,
+};
+
+static int ltc2952_poweroff_notify_panic(struct notifier_block *nb,
+					 unsigned long code,
+					 void *unused)
+{
+	ltc2952_poweroff_panic = true;
+	return NOTIFY_DONE;
+}
+
+static struct notifier_block ltc2952_poweroff_panic_nb = {
+	.notifier_call = ltc2952_poweroff_notify_panic,
+};
+
+static int __init ltc2952_poweroff_platform_init(void)
+{
+	ltc2952_poweroff_panic = false;
+
+	atomic_notifier_chain_register(&panic_notifier_list,
+		&ltc2952_poweroff_panic_nb);
+
+	return platform_driver_register(&ltc2952_poweroff_driver);
+}
+
+static void __exit ltc2952_poweroff_platform_exit(void)
+{
+	atomic_notifier_chain_unregister(&panic_notifier_list,
+		&ltc2952_poweroff_panic_nb);
+
+	platform_driver_unregister(&ltc2952_poweroff_driver);
+}
+
+module_init(ltc2952_poweroff_platform_init);
+module_exit(ltc2952_poweroff_platform_exit);
+
+MODULE_AUTHOR("René Moll <linux@r-moll.nl>");
+MODULE_AUTHOR("Frans KLaver <frans.klaver@xsens.com>");
+MODULE_AUTHOR("Tjerk Hofmeijer <tjerk.hofmeijer@xsens.com>");
+MODULE_DESCRIPTION("LTC PowerPath power-off driver");
+MODULE_LICENSE("GPL v2");
-- 
2.1.0

[PATCH v3 2/2] power: reset: document LTC2952 poweroff support

[Frans Klaver]

From: René Moll <linux@r-moll.nl>

Signed-off-by: René Moll <linux@r-moll.nl>
Signed-off-by: Tjerk Hofmeijer <tjerk.hofmeijer@xsens.com>
Signed-off-by: Frans Klaver <frans.klaver@xsens.com>
---
 .../bindings/power/reset/ltc2952-poweroff.txt      | 31 ++++++++++++++++++++++
 1 file changed, 31 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/power/reset/ltc2952-poweroff.txt

diff --git a/Documentation/devicetree/bindings/power/reset/ltc2952-poweroff.txt b/Documentation/devicetree/bindings/power/reset/ltc2952-poweroff.txt
new file mode 100644
index 0000000..3d15814
--- /dev/null
+++ b/Documentation/devicetree/bindings/power/reset/ltc2952-poweroff.txt
@@ -0,0 +1,31 @@
+Binding for the LTC2952 PowerPath controller
+
+This chip is used to externally trigger a system shut down. Once the trigger has
+been sent, the chips' watchdog has to be reset to gracefully shut down.
+If the Linux systems decides to shut down it powers off the platform via the
+kill signal.
+
+Required properties:
+
+- compatible:		Must contain: "lltc,ltc2952"
+- watchdog-gpios:	phandle + gpio-specifier for the GPIO connected to the
+			chip's watchdog line
+- kill-gpios:		phandle + gpio-specifier for the GPIO connected to the
+			chip's kill line
+
+Optional properties:
+
+- trigger-gpios:	phandle + gpio-specifier for the GPIO connected to the
+			chip's trigger line. If this property is not set, the
+			trigger function is ignored and the chip is kept alive
+			until an explicit kill signal is received
+
+Example:
+
+ltc2952 {
+	compatible = "lltc,ltc2952";
+
+	trigger-gpios = <&gpio0 1 GPIO_ACTIVE_LOW>;
+	watchdog-gpios = <&gpio1 2 GPIO_ACTIVE_HIGH>;
+	kill-gpios = <&gpio0 2 GPIO_ACTIVE_LOW>;
+};
-- 
2.1.0

Re: [PATCH v3 1/2] power: reset: add LTC2952 poweroff support

[Guenter Roeck]

On Mon, Oct 06, 2014 at 10:40:35AM +0200, Frans Klaver wrote:
> From: René Moll <linux@r-moll.nl>
> 
> The LTC2952 allows control over system power using a push button. It also
> supports powering down the board from a controller on the board.
> 
> If the power button is pushed, the ltc2952 starts a 400ms window to
> properly shut down the software. This window can be stretched by
> toggling a watchdog line. This driver always uses this watchdog line.
> The power button input is optional. This allows the system to use a
> custom shutdown sequence on the push button.
> 
> On software shutdown the driver sets the kill signal, starting the same
> sequence as if the push button was used.
> 
> Signed-off-by: Ren? Moll <linux@r-moll.nl>
> Signed-off-by: Tjerk Hofmeijer <tjerk.hofmeijer@xsens.com>
> Signed-off-by: Frans Klaver <frans.klaver@xsens.com>
> ---
>  drivers/power/reset/Kconfig            |  10 +
>  drivers/power/reset/Makefile           |   1 +
>  drivers/power/reset/ltc2952-poweroff.c | 381 +++++++++++++++++++++++++++++++++
>  3 files changed, 392 insertions(+)
>  create mode 100644 drivers/power/reset/ltc2952-poweroff.c
> 
> diff --git a/drivers/power/reset/Kconfig b/drivers/power/reset/Kconfig
> index ca41523..a0f68da 100644
> --- a/drivers/power/reset/Kconfig
> +++ b/drivers/power/reset/Kconfig
> @@ -45,6 +45,16 @@ config POWER_RESET_HISI
>  	help
>  	  Reboot support for Hisilicon boards.
>  
> +config POWER_RESET_LTC2952
> +	bool "LTC2952 PowerPath power-off driver"
> +	depends on OF_GPIO && POWER_RESET
> +	help
> +	  This driver supports an external powerdown trigger and board
> +	  power down via the LTC2952.
> +
> +	  If your board uses an LTC2952, say  Y and create a binding
> +	  in the device tree.
> +
>  config POWER_RESET_MSM
>  	bool "Qualcomm MSM power-off driver"
>  	depends on POWER_RESET && ARCH_QCOM
> diff --git a/drivers/power/reset/Makefile b/drivers/power/reset/Makefile
> index a42e70e..3cbb7c3 100644
> --- a/drivers/power/reset/Makefile
> +++ b/drivers/power/reset/Makefile
> @@ -3,6 +3,7 @@ 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_HISI) += hisi-reboot.o
> +obj-$(CONFIG_POWER_RESET_LTC2952) += ltc2952-poweroff.o
>  obj-$(CONFIG_POWER_RESET_MSM) += msm-poweroff.o
>  obj-$(CONFIG_POWER_RESET_QNAP) += qnap-poweroff.o
>  obj-$(CONFIG_POWER_RESET_RESTART) += restart-poweroff.o
> diff --git a/drivers/power/reset/ltc2952-poweroff.c b/drivers/power/reset/ltc2952-poweroff.c
> new file mode 100644
> index 0000000..2cf146a
> --- /dev/null
> +++ b/drivers/power/reset/ltc2952-poweroff.c
> @@ -0,0 +1,381 @@
> +/*
> + * LTC2952 PowerPath TM driver
> + *
> + * Copyright (C) 2014, Xsens Technologies BV <info@xsens.com>
> + *
> + * 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 2
> + * 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.
> + *
> + * ----------------------------------------
> + * - Description
> + * ----------------------------------------
> + *
> + * This driver is to be used with an external LTC2952 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 ltc2952 entry for pin mapping.
> + *
> + * ----------------------------------------
> + * - GPIO
> + * ----------------------------------------
> + *
> + * The following GPIOs are used:
> + * - trigger (input, optional)
> + *     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. If no pin is assigned to this input the driver will start
> + *     the watchdog immediately and only a power off event will kill the ltc2952
> + *
> + * - 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.
> + *

poweroff might be a better name.

> + * ----------------------------------------
> + * - 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>
> +
> +struct ltc2952_poweroff_data {
> +	struct hrtimer timer_trigger;
> +	struct hrtimer timer_wde;
> +
> +	ktime_t trigger_delay;
> +	ktime_t wde_interval;
> +
> +	struct device *dev;
> +
> +	unsigned int virq;
> +
> +	struct gpio_desc *gpio_trigger;
> +	struct gpio_desc *gpio_watchdog;
> +	struct gpio_desc *gpio_kill;
> +};
> +
> +static bool ltc2952_poweroff_panic;
> +static struct ltc2952_poweroff_data *ltc2952_data;
> +
> +/**
> + * ltc2952_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 ltc2952_poweroff_timer_wde(struct hrtimer *timer)
> +{
> +	ktime_t now;
> +	int state;
> +	unsigned long overruns;
> +
> +	if (ltc2952_poweroff_panic)
> +		return HRTIMER_NORESTART;
> +
> +	state = gpiod_get_value(ltc2952_data->gpio_watchdog);
> +	gpiod_set_value(ltc2952_data->gpio_watchdog, !state);
> +
> +	now = hrtimer_cb_get_time(timer);
> +	overruns = hrtimer_forward(timer, now, ltc2952_data->wde_interval);
> +
> +	return HRTIMER_RESTART;
> +}
> +
> +static void ltc2952_poweroff_start_wde(void)
> +{
> +	int ret;
> +
> +	ret = hrtimer_start(&ltc2952_data->timer_wde,
> +			    ltc2952_data->wde_interval, HRTIMER_MODE_REL);
> +
> +	if (ret) {
> +		dev_err(ltc2952_data->dev,
> +			"unable to start the watchdog 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
> +		 */
> +	}
> +}
> +
> +static enum hrtimer_restart ltc2952_poweroff_timer_trigger(struct hrtimer *t)
> +{
> +	ltc2952_poweroff_start_wde();
> +
> +	dev_info(ltc2952_data->dev, "executing shutdown\n");
> +
> +	orderly_poweroff(true);
> +
> +	return HRTIMER_NORESTART;
> +}
> +
> +/**
> + * ltc2952_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 ltc2952_poweroff_handler(int irq, void *dev_id)
> +{
> +	struct ltc2952_poweroff_data *data = dev_id;
> +
> +	if (ltc2952_poweroff_panic || hrtimer_active(&data->timer_wde))
> +		/* we're either panicking, or already shutting down */
> +		goto irq_ok;
> +
> +	if (hrtimer_active(&data->timer_trigger)) {
> +		hrtimer_cancel(&data->timer_trigger);
> +		goto irq_ok;
> +	}
> +
> +	if (hrtimer_start(&data->timer_trigger, data->trigger_delay,
> +			  HRTIMER_MODE_REL))
> +		dev_err(data->dev, "unable to start the wait timer\n");
> +
> +irq_ok:
> +	return IRQ_HANDLED;
> +}
> +
> +static void ltc2952_poweroff_kill(void)
> +{
> +	gpiod_set_value(ltc2952_data->gpio_kill, 1);
> +}
> +
> +static int ltc2952_poweroff_suspend(struct platform_device *pdev,
> +				    pm_message_t state)
> +{
> +	return -ENOSYS;
> +}
> +
> +static int ltc2952_poweroff_resume(struct platform_device *pdev)
> +{
> +	return -ENOSYS;
> +}
> +
What is the value of those functions if they don't do anything but return an
error ?

> +static void ltc2952_poweroff_default(struct ltc2952_poweroff_data *data)
> +{
> +	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 = &ltc2952_poweroff_timer_trigger;
> +
> +	hrtimer_init(&data->timer_wde, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
> +	data->timer_wde.function = &ltc2952_poweroff_timer_wde;
> +}
> +
> +static int ltc2952_poweroff_init(struct platform_device *pdev)
> +{
> +	int ret;
> +
> +	ltc2952_poweroff_default(ltc2952_data);
> +
> +	ltc2952_data->gpio_watchdog = gpiod_get(&pdev->dev, "watchdog");

Any reason for not using devm_gpiod_get() for those functions ?

> +	if (IS_ERR(ltc2952_data->gpio_watchdog)) {
> +		ret = PTR_ERR(ltc2952_data->gpio_watchdog);
> +		dev_err(&pdev->dev, "unable to claim watchdog-gpio\n");
> +		goto err;

Unnecessary goto. Just return the ERR_PTR directly.

> +	}
> +
> +	ret = gpiod_direction_output(ltc2952_data->gpio_watchdog, 0);
> +	if (ret) {
> +		dev_err(&pdev->dev, "unable to use watchdog-gpio as output\n");
> +		goto err_watchdog;
> +	}
> +
> +	ltc2952_data->gpio_kill = gpiod_get(&pdev->dev, "kill");
> +	if (IS_ERR(ltc2952_data->gpio_kill)) {
> +		ret = PTR_ERR(ltc2952_data->gpio_kill);
> +		dev_err(&pdev->dev, "unable to claim kill-gpio\n");
> +		goto err_watchdog;
> +	}
> +
> +	ret = gpiod_direction_output(ltc2952_data->gpio_kill, 0);
> +	if (ret) {
> +		dev_err(&pdev->dev, "unable to use kill-gpio as output\n");
> +		goto err_kill;
> +	}
> +
> +	ltc2952_data->gpio_trigger = gpiod_get(&pdev->dev, "trigger");
> +	if (IS_ERR(ltc2952_data->gpio_trigger)) {
> +		ret = PTR_ERR(ltc2952_data->gpio_trigger);
> +
> +		if (ret != -ENOENT) {
> +			dev_err(&pdev->dev, "unable to claim trigger-gpio\n");
> +			goto err_kill;
> +		}
> +
> +		dev_info(&pdev->dev, "No trigger input defined\n");
> +		ltc2952_data->gpio_trigger = NULL;
> +		ltc2952_poweroff_start_wde();

Can you explain this in more detail ? It is not entirely clear (at least not
to me) why you start the timer in this case.

> +		return 0;
> +	}
> +
> +	ltc2952_data->virq = gpiod_to_irq(ltc2952_data->gpio_trigger);
> +	if (ltc2952_data->virq < 0) {
> +		dev_err(&pdev->dev, "cannot map GPIO as interrupt");
> +		goto err_trigger;
> +	}
> +
> +	ret = request_irq(ltc2952_data->virq,
> +			  ltc2952_poweroff_handler,
> +			  (IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING),
> +			  "ltc2952-poweroff",
> +			  ltc2952_data);

Why not devm_request_irq ?

Overall, seems to me that using devm_ functions would simplify error cleanup
and removal code significantly.

> +	if (ret) {
> +		dev_err(&pdev->dev, "cannot configure an interrupt handler\n");
> +		goto err_trigger;

Wouldn't those cases be logically identical to the "have no trigger" case ?
Maybe not, but poweroff would presumably still work, and it is not entirely
clear to me why you would refuse to power off the system if triggered by the
"poweroff" command just because you failed to install an interrupt handler
for the trigger button.

> +	}
> +	return 0;
> +
> +err_trigger:
> +	gpiod_put(ltc2952_data->gpio_trigger);
> +err_kill:
> +	gpiod_put(ltc2952_data->gpio_kill);
> +err_watchdog:
> +	gpiod_put(ltc2952_data->gpio_watchdog);
> +err:
> +	return ret;
> +}
> +
> +static int ltc2952_poweroff_probe(struct platform_device *pdev)
> +{
> +	int ret;
> +
> +	if (pm_power_off) {
> +		dev_err(&pdev->dev, "pm_power_off already registered");
> +		return -EBUSY;
> +	}
> +
> +	ltc2952_data = kzalloc(sizeof(*ltc2952_data), GFP_KERNEL);

Why not devm_kzalloc ?

> +	if (!ltc2952_data)
> +		return -ENOMEM;
> +
> +	ltc2952_data->dev = &pdev->dev;
> +
> +	ret = ltc2952_poweroff_init(pdev);
> +	if (ret)
> +		goto err;
> +
> +	pm_power_off = &ltc2952_poweroff_kill;

That '&' is unnecessary.

> +
> +	dev_info(&pdev->dev, "probe successful\n");
> +
> +	return 0;
> +
> +err:
> +	kfree(ltc2952_data);
> +	return ret;
> +}
> +
> +static int ltc2952_poweroff_remove(struct platform_device *pdev)
> +{
> +	pm_power_off = NULL;
> +
> +	if (ltc2952_data) {
> +		if (ltc2952_data->gpio_trigger != NULL)
> +			free_irq(ltc2952_data->virq, ltc2952_data);
> +
> +		if (hrtimer_active(&ltc2952_data->timer_wde))
> +			hrtimer_cancel(&ltc2952_data->timer_wde);
> +
> +		gpiod_put(ltc2952_data->gpio_watchdog);
> +		gpiod_put(ltc2952_data->gpio_kill);
> +		gpiod_put(ltc2952_data->gpio_trigger);
> +
> +		kfree(ltc2952_data);
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id of_ltc2952_poweroff_match[] = {
> +	{ .compatible = "lltc,ltc2952"},
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, of_ltc2952_poweroff_match);
> +
> +static struct platform_driver ltc2952_poweroff_driver = {
> +	.probe = ltc2952_poweroff_probe,
> +	.remove = ltc2952_poweroff_remove,
> +	.driver = {
> +		.name = "ltc2952-poweroff",
> +		.owner = THIS_MODULE,
> +		.of_match_table = of_ltc2952_poweroff_match,
> +	},
> +	.suspend = ltc2952_poweroff_suspend,
> +	.resume = ltc2952_poweroff_resume,

I think you are supposed to put the suspend and resume calls into the driver
structure. The platform code names the callbacks here 'legacy'.

> +};
> +
> +static int ltc2952_poweroff_notify_panic(struct notifier_block *nb,
> +					 unsigned long code,
> +					 void *unused)
> +{
> +	ltc2952_poweroff_panic = true;
> +	return NOTIFY_DONE;
> +}
> +
> +static struct notifier_block ltc2952_poweroff_panic_nb = {
> +	.notifier_call = ltc2952_poweroff_notify_panic,
> +};
> +
> +static int __init ltc2952_poweroff_platform_init(void)
> +{
> +	ltc2952_poweroff_panic = false;
> +
> +	atomic_notifier_chain_register(&panic_notifier_list,
> +		&ltc2952_poweroff_panic_nb);
> +
> +	return platform_driver_register(&ltc2952_poweroff_driver);
> +}
> +
> +static void __exit ltc2952_poweroff_platform_exit(void)
> +{
> +	atomic_notifier_chain_unregister(&panic_notifier_list,
> +		&ltc2952_poweroff_panic_nb);
> +
> +	platform_driver_unregister(&ltc2952_poweroff_driver);
> +}
> +
> +module_init(ltc2952_poweroff_platform_init);
> +module_exit(ltc2952_poweroff_platform_exit);
> +
> +MODULE_AUTHOR("Ren? Moll <linux@r-moll.nl>");
> +MODULE_AUTHOR("Frans KLaver <frans.klaver@xsens.com>");
> +MODULE_AUTHOR("Tjerk Hofmeijer <tjerk.hofmeijer@xsens.com>");
> +MODULE_DESCRIPTION("LTC PowerPath power-off driver");
> +MODULE_LICENSE("GPL v2");
> -- 
> 2.1.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 
> 

Re: [PATCH v3 1/2] power: reset: add LTC2952 poweroff support

[Frans Klaver]

On Mon, Oct 06, 2014 at 02:32:10PM -0700, Guenter Roeck wrote:
> On Mon, Oct 06, 2014 at 10:40:35AM +0200, Frans Klaver wrote:
> > + * - kill (output)
> > + *     The last action during shut down is triggering this signalling, such
> > + *     that the PowerPath Control will power down the hardware.
> > + *
> 
> poweroff might be a better name.

Kill is the name used in the data sheet, but fair enough, poweroff does
describe the function better.


> > +static int ltc2952_poweroff_suspend(struct platform_device *pdev,
> > +				    pm_message_t state)
> > +{
> > +	return -ENOSYS;
> > +}
> > +
> > +static int ltc2952_poweroff_resume(struct platform_device *pdev)
> > +{
> > +	return -ENOSYS;
> > +}
> > +
> What is the value of those functions if they don't do anything but return an
> error ?

This is because of a passage in Documentation/SubmittingDrivers on PM
support:

  ... it should support basic power management by implementing, if
  necessary, the .suspend and .resume methods used during the
  system-wide suspend and resume transitions.  You should verify that
  your driver correctly handles the suspend and resume, but if you are
  unable to ensure that, please at least define the .suspend method
  returning the -ENOSYS ("Function not implemented") error.

I'm not sure what the actual difference is between doing this and just
not implementing them. I frankly don't care if they stay or go.


> > +static void ltc2952_poweroff_default(struct ltc2952_poweroff_data *data)
> > +{
> > +	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 = &ltc2952_poweroff_timer_trigger;
> > +
> > +	hrtimer_init(&data->timer_wde, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
> > +	data->timer_wde.function = &ltc2952_poweroff_timer_wde;
> > +}
> > +
> > +static int ltc2952_poweroff_init(struct platform_device *pdev)
> > +{
> > +	int ret;
> > +
> > +	ltc2952_poweroff_default(ltc2952_data);
> > +
> > +	ltc2952_data->gpio_watchdog = gpiod_get(&pdev->dev, "watchdog");
> 
> Any reason for not using devm_gpiod_get() for those functions ?

René said in the first review round:

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

I now unrolled these loops, and quite frankly I'm not that sure devm_*
functions are actually that sparsely used. The way to get around that is
to use them more. I'll see about using those instead, maybe add some
documentation as well.


> > +	if (IS_ERR(ltc2952_data->gpio_watchdog)) {
> > +		ret = PTR_ERR(ltc2952_data->gpio_watchdog);
> > +		dev_err(&pdev->dev, "unable to claim watchdog-gpio\n");
> > +		goto err;
> 
> Unnecessary goto. Just return the ERR_PTR directly.

Check.


> > +	ltc2952_data->gpio_trigger = gpiod_get(&pdev->dev, "trigger");
> > +	if (IS_ERR(ltc2952_data->gpio_trigger)) {
> > +		ret = PTR_ERR(ltc2952_data->gpio_trigger);
> > +
> > +		if (ret != -ENOENT) {
> > +			dev_err(&pdev->dev, "unable to claim trigger-gpio\n");
> > +			goto err_kill;
> > +		}
> > +
> > +		dev_info(&pdev->dev, "No trigger input defined\n");
> > +		ltc2952_data->gpio_trigger = NULL;
> > +		ltc2952_poweroff_start_wde();
> 
> Can you explain this in more detail ? It is not entirely clear (at least not
> to me) why you start the timer in this case.

The ltc2952 is designed around pushing a button for a configurable
length of time to trigger the powerdown. If we don't have the powerdown
trigger, we don't know when this happens. To be certain that the system
won't be powered down without us knowing about it, we have to start the
watchdog. If the ltc2952 gets into shutdown state, at least we know for
sure that the system will be kept alive. This is basically a way to
ignore the powerdown sequence imposed by the ltc2952.

I guess it could do with some explanation in the code.

 
> > +		return 0;
> > +	}
> > +
> > +	ltc2952_data->virq = gpiod_to_irq(ltc2952_data->gpio_trigger);
> > +	if (ltc2952_data->virq < 0) {
> > +		dev_err(&pdev->dev, "cannot map GPIO as interrupt");
> > +		goto err_trigger;
> > +	}
> > +
> > +	ret = request_irq(ltc2952_data->virq,
> > +			  ltc2952_poweroff_handler,
> > +			  (IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING),
> > +			  "ltc2952-poweroff",
> > +			  ltc2952_data);
> 
> Why not devm_request_irq ?
> 
> Overall, seems to me that using devm_ functions would simplify error cleanup
> and removal code significantly.

Refer to devm_gpiod_get()


> > +	if (ret) {
> > +		dev_err(&pdev->dev, "cannot configure an interrupt handler\n");
> > +		goto err_trigger;
> 
> Wouldn't those cases be logically identical to the "have no trigger" case ?
> Maybe not, but poweroff would presumably still work, and it is not entirely
> clear to me why you would refuse to power off the system if triggered by the
> "poweroff" command just because you failed to install an interrupt handler
> for the trigger button.

I think the reasoning was that once the gpio trigger is there, you
really want to respond to the interrupt. You're right though. We
shouldn't completely fail this function in this case and the poweroff
driver should just fall back to the no-gpio-trigger case.


> > +	}
> > +	return 0;
> > +
> > +err_trigger:
> > +	gpiod_put(ltc2952_data->gpio_trigger);
> > +err_kill:
> > +	gpiod_put(ltc2952_data->gpio_kill);
> > +err_watchdog:
> > +	gpiod_put(ltc2952_data->gpio_watchdog);
> > +err:
> > +	return ret;
> > +}
> > +
> > +static int ltc2952_poweroff_probe(struct platform_device *pdev)
> > +{
> > +	int ret;
> > +
> > +	if (pm_power_off) {
> > +		dev_err(&pdev->dev, "pm_power_off already registered");
> > +		return -EBUSY;
> > +	}
> > +
> > +	ltc2952_data = kzalloc(sizeof(*ltc2952_data), GFP_KERNEL);
> 
> Why not devm_kzalloc ?

See devm_gpiod_get() above.


> > +	if (!ltc2952_data)
> > +		return -ENOMEM;
> > +
> > +	ltc2952_data->dev = &pdev->dev;
> > +
> > +	ret = ltc2952_poweroff_init(pdev);
> > +	if (ret)
> > +		goto err;
> > +
> > +	pm_power_off = &ltc2952_poweroff_kill;
> 
> That '&' is unnecessary.

ooh, a c++ism...

Somewhat off-topic: In the next round I think I'll add a patch that will
use your new poweroff handler api. I like that approach better.


> > +static struct platform_driver ltc2952_poweroff_driver = {
> > +	.probe = ltc2952_poweroff_probe,
> > +	.remove = ltc2952_poweroff_remove,
> > +	.driver = {
> > +		.name = "ltc2952-poweroff",
> > +		.owner = THIS_MODULE,
> > +		.of_match_table = of_ltc2952_poweroff_match,
> > +	},
> > +	.suspend = ltc2952_poweroff_suspend,
> > +	.resume = ltc2952_poweroff_resume,
> 
> I think you are supposed to put the suspend and resume calls into the driver
> structure. The platform code names the callbacks here 'legacy'.

I'll check that out.

Thanks for the review,
Frans

Re: [PATCH v3 1/2] power: reset: add LTC2952 poweroff support

[Guenter Roeck]

On 10/07/2014 01:15 AM, Frans Klaver wrote:
> On Mon, Oct 06, 2014 at 02:32:10PM -0700, Guenter Roeck wrote:

>> Any reason for not using devm_gpiod_get() for those functions ?
>
> René said in the first review round:
>
>>>>   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
>
> I now unrolled these loops, and quite frankly I'm not that sure devm_*
> functions are actually that sparsely used. The way to get around that is
> to use them more. I'll see about using those instead, maybe add some
> documentation as well.
>

"git grep devm_| wc" on 3.17 returns 7344.

If that is not enough, nothing ever will.

Guenter

Re: [PATCH v3 1/2] power: reset: add LTC2952 poweroff support

[Frans Klaver]

On Tue, Oct 07, 2014 at 06:16:19AM -0700, Guenter Roeck wrote:
> On 10/07/2014 01:15 AM, Frans Klaver wrote:
> > On Mon, Oct 06, 2014 at 02:32:10PM -0700, Guenter Roeck wrote:
> 
> >> Any reason for not using devm_gpiod_get() for those functions ?
> >
> > René said in the first review round:
> >
> >>>>   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
> >
> > I now unrolled these loops, and quite frankly I'm not that sure devm_*
> > functions are actually that sparsely used. The way to get around that is
> > to use them more. I'll see about using those instead, maybe add some
> > documentation as well.
> >
> 
> "git grep devm_| wc" on 3.17 returns 7344.
> 
> If that is not enough, nothing ever will.

Heh, precisely. I've switched to the devm_* functions. Code's much
cleaner now.

Thanks,
Frans

Re: [PATCH v3 1/2] power: reset: add LTC2952 poweroff support

[Frans Klaver]

On Tue, Oct 07, 2014 at 10:15:44AM +0200, Frans Klaver wrote:
> On Mon, Oct 06, 2014 at 02:32:10PM -0700, Guenter Roeck wrote:
> > On Mon, Oct 06, 2014 at 10:40:35AM +0200, Frans Klaver wrote:
> > > +static struct platform_driver ltc2952_poweroff_driver = {
> > > +	.probe = ltc2952_poweroff_probe,
> > > +	.remove = ltc2952_poweroff_remove,
> > > +	.driver = {
> > > +		.name = "ltc2952-poweroff",
> > > +		.owner = THIS_MODULE,
> > > +		.of_match_table = of_ltc2952_poweroff_match,
> > > +	},
> > > +	.suspend = ltc2952_poweroff_suspend,
> > > +	.resume = ltc2952_poweroff_resume,
> > 
> > I think you are supposed to put the suspend and resume calls into the driver
> > structure. The platform code names the callbacks here 'legacy'.
> 
> I'll check that out.

They are supposed to go into a pm_ops structure, which is pointed at in
driver's pm pointer. I'll drop them in any case. It would lead to a
bunch of boiler plate for nothing.

Frans