Re: [PATCH v2 2/2] hwmon: add generic GPIO brownout support

Guenter Roeck
2018-10-30T04:42:50Z

Hi Marco,

On Mon, Oct 29, 2018 at 03:35:21PM +0100, Marco Felsch wrote:
> Most the time low voltage detection happens on a external device
> e.g. a pmic or any other hw-logic. Some of such devices can pass the
> power state (good/bad) to the host via i2c or by toggling a gpio.
>
> This patch adds the support to evaluate a gpio line to determine
> the power good/bad state. The state is represented by the
> in0_lcrit_alarm. Furthermore the driver supports to release device from
> their driver upon a low voltage detection. This feature is supported by
> OF-based firmware only.
>

NACK, sorry.

hwmon is strictly about monitoring, not about taking any action, and much
less about removing devices from the system after some status change,
it be a gpio pin value or anything else. Other than that, the driver simply
maps a gpio pin to a (pretty much arbitrary) sysfs attribute, for which
a driver isn't really needed.

I don't know if there is a space in the kernel for handling the problem
you are trying to solve, but it isn't hwmon.

Guenter

> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> ---
> Documentation/hwmon/gpio-brownout | 14 +++
> drivers/hwmon/Kconfig | 23 ++++
> drivers/hwmon/Makefile | 1 +
> drivers/hwmon/gpio-brownout.c | 195 ++++++++++++++++++++++++++++++
> 4 files changed, 233 insertions(+)
> create mode 100644 Documentation/hwmon/gpio-brownout
> create mode 100644 drivers/hwmon/gpio-brownout.c
>
> diff --git a/Documentation/hwmon/gpio-brownout b/Documentation/hwmon/gpio-brownout
> new file mode 100644
> index 000000000000..61d6a781af47
> --- /dev/null
> +++ b/Documentation/hwmon/gpio-brownout
> @@ -0,0 +1,14 @@
> +Kernel driver gpio-brownout
> +===========================
> +
> +Author: Marco Felsch <kernel@pengutronix.de>
> +
> +Description
> +-----------
> +
> +This driver checks a GPIO line to detect a undervoltage condition.
> +
> +Sysfs entries
> +-------------
> +
> +in0_lcrit_alarm Undervoltage alarm
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 81da17a42dc9..a2712452ba8b 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -558,6 +558,29 @@ config SENSORS_G762
> This driver can also be built as a module. If so, the module
> will be called g762.
>
> +config SENSORS_GPIO_BROWNOUT
> + tristate "Generic GPIO Brownout detection support"
> + depends on GPIOLIB
> + help
> + If you say yes here you get support for GPIO based brownout detection.
> +
> + If unsure, say N.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called gpio-brownout.
> +
> +if SENSORS_GPIO_BROWNOUT
> +
> +config SENSORS_GPIO_BROWNOUT_UNBIND
> + bool "OF I2C/SPI device unbinding support"
> + depends on OF && I2C && SPI
> + help
> + Enable support to unbind devices upon a brownout detection.
> +
> + If unsure, say N.
> +
> +endif
> +
> config SENSORS_GPIO_FAN
> tristate "GPIO fan"
> depends on OF_GPIO
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index 93f7f41ea4ad..6b217b39e0e0 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -71,6 +71,7 @@ obj-$(CONFIG_SENSORS_G760A) += g760a.o
> obj-$(CONFIG_SENSORS_G762) += g762.o
> obj-$(CONFIG_SENSORS_GL518SM) += gl518sm.o
> obj-$(CONFIG_SENSORS_GL520SM) += gl520sm.o
> +obj-$(CONFIG_SENSORS_GPIO_BROWNOUT) += gpio-brownout.o
> obj-$(CONFIG_SENSORS_GPIO_FAN) += gpio-fan.o
> obj-$(CONFIG_SENSORS_HIH6130) += hih6130.o
> obj-$(CONFIG_SENSORS_ULTRA45) += ultra45_env.o
> diff --git a/drivers/hwmon/gpio-brownout.c b/drivers/hwmon/gpio-brownout.c
> new file mode 100644
> index 000000000000..00d6ff8b1490
> --- /dev/null
> +++ b/drivers/hwmon/gpio-brownout.c
> @@ -0,0 +1,195 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * gpio-brownout.c - gpio based low voltage detection
> + *
> + * Copyright (C) 2018 Pengutronix, Marco Felsch <kernel@pengutronix.de>
> + */
> +
> +#include <linux/gpio/consumer.h>
> +#include <linux/i2c.h>
> +#include <linux/hwmon.h>
> +#include <linux/hwmon-sysfs.h>
> +#include <linux/interrupt.h>
> +#include <linux/kernel.h>
> +#include <linux/list.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/spi/spi.h>
> +#include <linux/stat.h>
> +
> +#define GPIO_BROWNOUT_MOD_NAME "gpio-brownout"
> +
> +struct gpio_brownout_device {
> + struct list_head list;
> + struct device *dev;
> +};
> +
> +struct gpio_brownout {
> + struct device *hwmon_dev;
> + struct gpio_desc *gpio;
> + struct list_head devices;
> +};
> +
> +static irqreturn_t gpio_brownout_isr(int irq, void *dev_id)
> +{
> + struct gpio_brownout *gb = dev_id;
> + struct gpio_brownout_device *bout_dev, *tmp;
> +
> + list_for_each_entry_safe(bout_dev, tmp, &gb->devices, list) {
> + device_release_driver(bout_dev->dev);
> + list_del(&bout_dev->list);
> + }
> +
> + sysfs_notify(&gb->hwmon_dev->kobj, NULL, "in0_lcrit_alarm");
> +
> + return IRQ_HANDLED;
> +}
> +
> +static int gpio_brownout_probe_fw(struct gpio_brownout *gb)
> +{
> + struct device *pdev = gb->hwmon_dev->parent;
> +
> + gb->gpio = devm_gpiod_get(pdev, "gpio-brownout,sense", GPIOD_IN);
> + if (IS_ERR(gb->gpio))
> + return PTR_ERR(gb->gpio);
> +
> + /*
> + * Register all devices which should be unbinded upon a brownout
> + * detection. At the moment only i2c and spi devices are supported
> + */
> +
> + if (IS_ENABLED(SENSORS_GPIO_BROWNOUT_UNBIND)) {
> + struct device_node *np = gb->hwmon_dev->of_node;
> + struct of_phandle_iterator it;
> + struct gpio_brownout_device *elem;
> + struct i2c_client *i2c_c;
> + struct spi_device *spi_c;
> + int ret;
> +
> + of_for_each_phandle(&it, ret, np,
> + "gpio-brownout,dev-list", NULL, 0) {
> + i2c_c = of_find_i2c_device_by_node(it.node);
> + spi_c = of_find_spi_device_by_node(it.node);
> +
> + if (!i2c_c && !spi_c)
> + return -EPROBE_DEFER;
> + else if (i2c_c && spi_c)
> + return -EINVAL;
> +
> + elem = devm_kzalloc(pdev, sizeof(*elem), GFP_KERNEL);
> + if (!elem)
> + return -ENOMEM;
> +
> + elem->dev = i2c_c ? &i2c_c->dev : &spi_c->dev;
> +
> + INIT_LIST_HEAD(&elem->list);
> + list_add_tail(&elem->list, &gb->devices);
> + }
> + }
> +
> + return 0;
> +}
> +
> +const u32 gpio_brownout_in_config[] = {
> + HWMON_I_LCRIT_ALARM,
> + 0
> +};
> +
> +const struct hwmon_channel_info gpio_brownout_in = {
> + .type = hwmon_in,
> + .config = gpio_brownout_in_config,
> +};
> +
> +const struct hwmon_channel_info *gpio_brownout_ch_info[] = {
> + &gpio_brownout_in,
> + NULL
> +};
> +
> +static int gpio_brownout_read(struct device *dev, enum hwmon_sensor_types type,
> + u32 attr, int channel, long *val)
> +{
> + struct gpio_brownout *gb = dev_get_drvdata(dev);
> +
> + *val = gpiod_get_value(gb->gpio);
> + return 0;
> +}
> +
> +static umode_t gpio_brownout_is_visible(const void *drvdata,
> + enum hwmon_sensor_types type, u32 attr,
> + int channel)
> +{
> + return 0444;
> +}
> +
> +const struct hwmon_ops gpio_brownout_ops = {
> + .is_visible = gpio_brownout_is_visible,
> + .read = gpio_brownout_read,
> +
> +};
> +
> +const struct hwmon_chip_info gpio_brownout_info = {
> + .ops = &gpio_brownout_ops,
> + .info = gpio_brownout_ch_info,
> +};
> +
> +static int gpio_brownout_probe(struct platform_device *pdev)
> +{
> + struct gpio_brownout *gb;
> + struct device *hwmon;
> + unsigned long irq_flags;
> + int ret;
> +
> + gb = devm_kzalloc(&pdev->dev, sizeof(*gb), GFP_KERNEL);
> + if (!gb)
> + return -ENOMEM;
> +
> + hwmon = devm_hwmon_device_register_with_info(&pdev->dev, pdev->name, gb,
> + &gpio_brownout_info, NULL);
> + if (IS_ERR(hwmon))
> + return PTR_ERR(hwmon);
> +
> + gb->hwmon_dev = hwmon;
> +
> + INIT_LIST_HEAD(&gb->devices);
> +
> + ret = gpio_brownout_probe_fw(gb);
> + if (ret)
> + return ret;
> +
> + irq_flags = IRQF_ONESHOT;
> + if (gpiod_is_active_low(gb->gpio))
> + irq_flags |= IRQF_TRIGGER_FALLING;
> + else
> + irq_flags |= IRQF_TRIGGER_RISING;
> + ret = devm_request_threaded_irq(&pdev->dev, gpiod_to_irq(gb->gpio),
> + NULL, gpio_brownout_isr, irq_flags,
> + GPIO_BROWNOUT_MOD_NAME, gb);
> + if (ret < 0) {
> + dev_err(&pdev->dev, "IRQ request failed: %d\n", ret);
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static const struct of_device_id __maybe_unused gpio_brownout_of_match[] = {
> + { .compatible = GPIO_BROWNOUT_MOD_NAME, },
> + { },
> +};
> +MODULE_DEVICE_TABLE(of, arm_gpio_brownout_of_match);
> +
> +static struct platform_driver gpio_brownout_driver = {
> + .driver = {
> + .name = GPIO_BROWNOUT_MOD_NAME,
> + .of_match_table = of_match_ptr(gpio_brownout_of_match)
> + },
> + .probe = gpio_brownout_probe,
> +};
> +
> +module_platform_driver(gpio_brownout_driver);
> +
> +MODULE_AUTHOR("Marco Felsch <kernel@pengutronix.de>");
> +MODULE_DESCRIPTION("GPIO Brownout Detection");
> +MODULE_LICENSE("GPL v2");
> --
> 2.19.1
>