Hi Sebastian,
On Thu, 13 Aug 2020 at 02:28, Sebastian Reichel
<sebastian.reic...@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.reic...@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 <s...@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 <j...@lentin.co.uk>
> + * Andrew Lunn <and...@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
