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

[Mikko Perttunen <mperttunen@nvidia.com>]

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