[PATCHv2,06/10] sysreset: Add poweroff-gpio driver

Add GPIO poweroff driver, which is based on the Linux
driver and uses the same DT binding.

Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
---
 drivers/sysreset/Kconfig         |  6 +++
 drivers/sysreset/Makefile        |  1 +
 drivers/sysreset/poweroff_gpio.c | 84 ++++++++++++++++++++++++++++++++
 3 files changed, 91 insertions(+)
 create mode 100644 drivers/sysreset/poweroff_gpio.c

Hi Sebastian,

On Thu, 13 Aug 2020 at 02:28, Sebastian Reichel
<sebastian.reichel@collabora.com> wrote:
>
> Add GPIO poweroff driver, which is based on the Linux
> driver and uses the same DT binding.
>
> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> ---
>  drivers/sysreset/Kconfig         |  6 +++
>  drivers/sysreset/Makefile        |  1 +
>  drivers/sysreset/poweroff_gpio.c | 84 ++++++++++++++++++++++++++++++++
>  3 files changed, 91 insertions(+)
>  create mode 100644 drivers/sysreset/poweroff_gpio.c
>

Reviewed-by: Simon Glass <sjg@chromium.org>

nits below

> diff --git a/drivers/sysreset/Kconfig b/drivers/sysreset/Kconfig
> index 6ebc90e1d33b..82af8db47f65 100644
> --- a/drivers/sysreset/Kconfig
> +++ b/drivers/sysreset/Kconfig
> @@ -43,6 +43,12 @@ config SYSRESET_CMD_POWEROFF
>
>  endif
>
> +config POWEROFF_GPIO
> +       bool "Enable support for GPIO poweroff driver"
> +       select DM_GPIO
> +       help
> +         Poweroff support via GPIO pin connected reset logic.

Does this mean GPIO-pin-connected reset logic? Can you please add the
hyphens or rephrase to clarify the meaning?

Also could use a bit more detail.

> +
>  config SYSRESET_GPIO
>         bool "Enable support for GPIO reset driver"
>         select DM_GPIO
> diff --git a/drivers/sysreset/Makefile b/drivers/sysreset/Makefile
> index df2293b8489d..90a9b26abef4 100644
> --- a/drivers/sysreset/Makefile
> +++ b/drivers/sysreset/Makefile
> @@ -7,6 +7,7 @@ obj-$(CONFIG_ARCH_ASPEED) += sysreset_ast.o
>  obj-$(CONFIG_ARCH_ROCKCHIP) += sysreset_rockchip.o
>  obj-$(CONFIG_ARCH_STI) += sysreset_sti.o
>  obj-$(CONFIG_SANDBOX) += sysreset_sandbox.o
> +obj-$(CONFIG_POWEROFF_GPIO) += poweroff_gpio.o
>  obj-$(CONFIG_SYSRESET_GPIO) += sysreset_gpio.o
>  obj-$(CONFIG_SYSRESET_MPC83XX) += sysreset_mpc83xx.o
>  obj-$(CONFIG_SYSRESET_MICROBLAZE) += sysreset_microblaze.o
> diff --git a/drivers/sysreset/poweroff_gpio.c b/drivers/sysreset/poweroff_gpio.c
> new file mode 100644
> index 000000000000..0eb2426d7574
> --- /dev/null
> +++ b/drivers/sysreset/poweroff_gpio.c
> @@ -0,0 +1,84 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Toggles a GPIO pin to power down a device
> + *
> + * Created using the Linux driver as reference, which
> + * has been written by:
> + *
> + * Jamie Lentin <jm@lentin.co.uk>
> + * Andrew Lunn <andrew@lunn.ch>
> + *
> + * Copyright (C) 2012 Jamie Lentin
> + */
> +
> +#include <common.h>
> +#include <asm/gpio.h>

Check order.

> +#include <dm.h>
> +#include <errno.h>
> +#include <linux/delay.h>
> +#include <log.h>
> +#include <sysreset.h>
> +
> +struct poweroff_gpio_info {
> +       struct gpio_desc gpio;
> +       u32 active_delay;
> +       u32 inactive_delay;
> +       u32 timeout;

timeout_ms so units are clear?
> +};
> +
> +static int poweroff_gpio_request(struct udevice *dev, enum sysreset_t type)
> +{
> +       struct poweroff_gpio_info *priv = dev_get_priv(dev);
> +
> +       if (type != SYSRESET_POWER_OFF)
> +               return -ENOSYS;
> +
> +       debug("GPIO poweroff\n");
> +
> +       /* drive it active, also inactive->active edge */
> +       dm_gpio_set_value(&priv->gpio, 1);

Should check return values in this function

> +       mdelay(priv->active_delay);
> +
> +       /* drive inactive, also active->inactive edge */
> +       dm_gpio_set_value(&priv->gpio, 0);
> +       mdelay(priv->inactive_delay);
> +
> +       /* drive it active, also inactive->active edge */
> +       dm_gpio_set_value(&priv->gpio, 1);
> +
> +       /* give it some time */
> +       mdelay(priv->timeout);
> +
> +       return -ETIMEDOUT;

-EINPROGRESS since it may still happen later, right?


> +}
> +
> +static int poweroff_gpio_probe(struct udevice *dev)
> +{
> +       struct poweroff_gpio_info *priv = dev_get_priv(dev);
> +       int flags;
> +
> +       flags = dev_read_bool(dev, "input") ? GPIOD_IS_IN : GPIOD_IS_OUT;
> +       priv->active_delay = dev_read_u32_default(dev, "active-delay-ms", 100);
> +       priv->inactive_delay = dev_read_u32_default(dev, "inactive-delay-ms", 100);
> +       priv->timeout = dev_read_u32_default(dev, "timeout-ms", 3000);
> +
> +       return gpio_request_by_name(dev, "gpios", 0, &priv->gpio, flags);
> +}
> +
> +static struct sysreset_ops poweroff_gpio_ops = {
> +       .request = poweroff_gpio_request,
> +};
> +
> +static const struct udevice_id poweroff_gpio_ids[] = {
> +       { .compatible = "gpio-poweroff", },
> +       {},
> +};
> +
> +U_BOOT_DRIVER(poweroff_gpio) = {
> +       .name           = "poweroff-gpio",
> +       .id             = UCLASS_SYSRESET,
> +       .ops            = &poweroff_gpio_ops,
> +       .probe          = poweroff_gpio_probe,
> +       .priv_auto_alloc_size = sizeof(struct poweroff_gpio_info),
> +       .of_match       = poweroff_gpio_ids,
> +};
> --
> 2.28.0
>

Regards,
Simon