[PATCH v2 0/2] Add GPIO brownout detection support

[PATCH v2 0/2] Add GPIO brownout detection support

[Marco Felsch]

Hi,

this is my v2 of my v1 [1]. I converted the driver from the input to the
hwmon framework, as proposed by Trent [2]. Futhermore I droped the OF
dependency for the core functionality (sensing the gpio) and moved the
addtional feature (unbinding devices) to a additional config. So the
dependencies are much cleaner.

The spi patches are already applied by Mark so I dropped them in my v2.
You have to apply the patche from Marks Git:

https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git tags/spi-find-by-node

to get the patch stack compiled.

I rebased the series on top of Guenter's hwmon-next branch and checked
it by checkpatch. Unfortunately it complains about a missing help
paragraph for SENSORS_GPIO_BROWNOUT_UNBIND, but the help exists.

Regards,
Marco

[1] https://patchwork.kernel.org/cover/10613821/
[2] https://patchwork.kernel.org/patch/10613825/

Marco Felsch (2):
  dt-binding: hwmon: add gpio-brownout bindings
  hwmon: add generic GPIO brownout support

 .../bindings/hwmon/gpio-brownout.txt          |  32 +++
 Documentation/hwmon/gpio-brownout             |  14 ++
 drivers/hwmon/Kconfig                         |  23 +++
 drivers/hwmon/Makefile                        |   1 +
 drivers/hwmon/gpio-brownout.c                 | 195 ++++++++++++++++++
 5 files changed, 265 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/hwmon/gpio-brownout.txt
 create mode 100644 Documentation/hwmon/gpio-brownout
 create mode 100644 drivers/hwmon/gpio-brownout.c

-- 
2.19.1

[PATCH v2 1/2] dt-binding: hwmon: add gpio-brownout bindings

[Marco Felsch]

Add dt-bindings for gpio-brownout detection devices. Such a device
determines the voltage good/bad state by the gpio line level.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 .../bindings/hwmon/gpio-brownout.txt          | 32 +++++++++++++++++++
 1 file changed, 32 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/hwmon/gpio-brownout.txt

diff --git a/Documentation/devicetree/bindings/hwmon/gpio-brownout.txt b/Documentation/devicetree/bindings/hwmon/gpio-brownout.txt
new file mode 100644
index 000000000000..f3272befc8f8
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/gpio-brownout.txt
@@ -0,0 +1,32 @@
+Device-Tree bindings for GPIO line brownout detection
+
+Required properties:
+- compatible: Must be "gpio-brownout"
+- gpio-brownout,sense-gpios: This gpio is used to detect a brownout. The gpio
+  must support interrupts.
+
+Optional properties:
+- gpio-brownout,dev-list: A list of i2c or spi device phandles. All
+  listed devices will be released from their drivers in the order they listed
+  upon a brownout detection. This can be helpful to avoid a interrupt flood,
+  because some system designs power off all external devices immediately and
+  keep the host on for a certain time.
+
+Example:
+
+i2c3 {
+	temp_core: lm75@48 { };
+	temp_chassis: lm75@49 { };
+};
+
+spi1 {
+	ts: ad7879@1 { };
+};
+
+/ {
+	gpio_brownout {
+		compatible = "gpio-brownout";
+		gpio-brownout,sense-gpios = <&gpio3 3 GPIO_ACTIVE_LOW>;
+		gpio-brownout,dev-list = <&temp_core &ts>;
+	};
+};
-- 
2.19.1

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

[Marco Felsch]

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.

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

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

[Guenter Roeck]

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
> 

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

[Trent Piepho]

On Mon, 2018-10-29 at 12:52 -0700, Guenter Roeck wrote:
> 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.

If we ignore the ability to stop other devices, how is this not a hwmon
device with just alarm features?

It seems to map the hwmon interface quite directly.

Consider, what if we had a "classic" hwmon chip, but on this board the
I2C/LPC/SPI interface was not connected as an appropriate master was
not available, and the default configuration of the chip was
acceptable.  The chip's alarm outputs are connected to GPIOs, as it
normal for a hwmon chip with alarm outputs.

Are the alarms no longer appropriate to appear in hwmon?  But if we
connect the chip's I2C interface, then those same alarms should appear
in hwmon?

That just doesn't make sense to me.

Also consider the gpio-fan driver.  That's pretty much just an
interface to a gpio too.

Or consider the leds-gpio driver.  That allows a gpio controlled LED to
appear in the kernel's led subsystem.  It doesn't do anything besides
turn the gpio on and off.  It's hardly needed if all we cared about was
controlling the LED in some way.  Yet it's used quite often.

So it seems perfectly correct to me that a GPIO based hardware
monitoring alarm should appear in the kernel's hardware monitoring
subsystem with all the other hardware monitoring alarms.

The ability of the hwmon driver to cause certain other devices to be
removed is a different question.

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

[Guenter Roeck]

On 10/29/18 2:16 PM, Trent Piepho wrote:
> On Mon, 2018-10-29 at 12:52 -0700, Guenter Roeck wrote:
>> 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.
> 
> If we ignore the ability to stop other devices, how is this not a hwmon
> device with just alarm features?
> 
Possibly, but the ability to stop other devices is at the core of the driver
as submitted.

> It seems to map the hwmon interface quite directly.

Agreed.

> 
> Consider, what if we had a "classic" hwmon chip, but on this board the
> I2C/LPC/SPI interface was not connected as an appropriate master was
> not available, and the default configuration of the chip was
> acceptable.  The chip's alarm outputs are connected to GPIOs, as it
> normal for a hwmon chip with alarm outputs.
> 
"If we had" is theory. Do we ? We don't usually add code to the kernel
just in case the hardware it supports might be out there.

> Are the alarms no longer appropriate to appear in hwmon?  But if we
> connect the chip's I2C interface, then those same alarms should appear
> in hwmon?
> 
> That just doesn't make sense to me.
> 
> Also consider the gpio-fan driver.  That's pretty much just an
> interface to a gpio too.
> > Or consider the leds-gpio driver.  That allows a gpio controlled LED to
> appear in the kernel's led subsystem.  It doesn't do anything besides
> turn the gpio on and off.  It's hardly needed if all we cared about was
> controlling the LED in some way.  Yet it's used quite often.
> 

The difference here is that those are distinct drivers, with no other hardware
behind it to control the LEDs or to report fan speeds.

For voltage monitoring, that is not normally the case. It is much more likely
that there is in fact a hardware monitoring or power control chip, the
(or an) alarm output of that chip is connected to a gpio line, and its control
interface is connected. If so, the driver for that chip should be enhanced
to support interrupts, and to report the status with its own sysfs attributes.

Agreed, that might be more work for a given hardware, and it would have to
be repeated for each chip with that configuration which doesn't already
have interrupt support. Meaning, unfortunately, almost all hwmon drivers.
It might even be necessary to implement an i2c or spi controller driver
if that does not exist. But it would be the appropriate solution.

> So it seems perfectly correct to me that a GPIO based hardware
> monitoring alarm should appear in the kernel's hardware monitoring
> subsystem with all the other hardware monitoring alarms.
> 
We can only consider a driver reporting a specific attribute if there
is a board which doesn't support anything else.

> The ability of the hwmon driver to cause certain other devices to be
> removed is a different question.
> 
I disagree. That functionality is essential to the driver as submitted.

Guenter

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

[Marco Felsch]

Hi Guenter,

thanks for the quick response, please see my comments below.

On 18-10-29 18:12, Guenter Roeck wrote:
> On 10/29/18 2:16 PM, Trent Piepho wrote:
> > On Mon, 2018-10-29 at 12:52 -0700, Guenter Roeck wrote:
> > > 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.
> > 
> > If we ignore the ability to stop other devices, how is this not a hwmon
> > device with just alarm features?
> > 
> Possibly, but the ability to stop other devices is at the core of the driver
> as submitted.
> 
> > It seems to map the hwmon interface quite directly.
> 
> Agreed.
> 
> > 
> > Consider, what if we had a "classic" hwmon chip, but on this board the
> > I2C/LPC/SPI interface was not connected as an appropriate master was
> > not available, and the default configuration of the chip was
> > acceptable.  The chip's alarm outputs are connected to GPIOs, as it
> > normal for a hwmon chip with alarm outputs.
> > 
> "If we had" is theory. Do we ? We don't usually add code to the kernel
> just in case the hardware it supports might be out there.

Yes, there are "good old" hwmon chips without any management interface,
e.g. LTC694-3.3, LTC7862, ... also there might be a discrete hw circuit.
I think it's better to handle those "simple" chips by a generic hwmon
driver. By simple I mean chips which informs the host only by toggling a
gpio to report a state. For this purpose my driver (including name
convention) isn't generic enough, I think about a hwmon-simple device.
Such a device can support reporting states, no voltage/power/temp values.

> > Are the alarms no longer appropriate to appear in hwmon?  But if we
> > connect the chip's I2C interface, then those same alarms should appear
> > in hwmon?
> > 
> > That just doesn't make sense to me.
> > 
> > Also consider the gpio-fan driver.  That's pretty much just an
> > interface to a gpio too.
> > Or consider the leds-gpio driver.  That allows a gpio controlled LED to
> > appear in the kernel's led subsystem.  It doesn't do anything besides
> > turn the gpio on and off.  It's hardly needed if all we cared about was
> > controlling the LED in some way.  Yet it's used quite often.
> > 
> 
> The difference here is that those are distinct drivers, with no other hardware
> behind it to control the LEDs or to report fan speeds.
> 
> For voltage monitoring, that is not normally the case. It is much more likely
> that there is in fact a hardware monitoring or power control chip, the
> (or an) alarm output of that chip is connected to a gpio line, and its control
> interface is connected. If so, the driver for that chip should be enhanced
> to support interrupts, and to report the status with its own sysfs attributes.
> 
> Agreed, that might be more work for a given hardware, and it would have to
> be repeated for each chip with that configuration which doesn't already
> have interrupt support. Meaning, unfortunately, almost all hwmon drivers.
> It might even be necessary to implement an i2c or spi controller driver
> if that does not exist. But it would be the appropriate solution.

Please see my above comments.

> > So it seems perfectly correct to me that a GPIO based hardware
> > monitoring alarm should appear in the kernel's hardware monitoring
> > subsystem with all the other hardware monitoring alarms.
> > 
> We can only consider a driver reporting a specific attribute if there
> is a board which doesn't support anything else.
> 
> > The ability of the hwmon driver to cause certain other devices to be
> > removed is a different question.
> > 
> I disagree. That functionality is essential to the driver as submitted.

You're right thats important for my use-case, but I got you, thats not a
hwmon related problem. If I understood the device-model correctly there
are absolut no problems to hot-unplug devices, this is always the case
if we unbind a driver from user-space. So why we can't handle it
directly in the kernel. Are there any concerns? If not can you give me
some hints to get the logic at thr right place.

Thanks,
Marco

> Guenter
> 

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

[Guenter Roeck]

On 10/30/18 3:47 AM, Marco Felsch wrote:
> Hi Guenter,
> 
> thanks for the quick response, please see my comments below.
> 
> On 18-10-29 18:12, Guenter Roeck wrote:
>> On 10/29/18 2:16 PM, Trent Piepho wrote:
>>> On Mon, 2018-10-29 at 12:52 -0700, Guenter Roeck wrote:
>>>> 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.
>>>
>>> If we ignore the ability to stop other devices, how is this not a hwmon
>>> device with just alarm features?
>>>
>> Possibly, but the ability to stop other devices is at the core of the driver
>> as submitted.
>>
>>> It seems to map the hwmon interface quite directly.
>>
>> Agreed.
>>
>>>
>>> Consider, what if we had a "classic" hwmon chip, but on this board the
>>> I2C/LPC/SPI interface was not connected as an appropriate master was
>>> not available, and the default configuration of the chip was
>>> acceptable.  The chip's alarm outputs are connected to GPIOs, as it
>>> normal for a hwmon chip with alarm outputs.
>>>
>> "If we had" is theory. Do we ? We don't usually add code to the kernel
>> just in case the hardware it supports might be out there.
> 
> Yes, there are "good old" hwmon chips without any management interface,
> e.g. LTC694-3.3, LTC7862, ... also there might be a discrete hw circuit.
> I think it's better to handle those "simple" chips by a generic hwmon
> driver. By simple I mean chips which informs the host only by toggling a
> gpio to report a state. For this purpose my driver (including name
> convention) isn't generic enough, I think about a hwmon-simple device.
> Such a device can support reporting states, no voltage/power/temp values.
> 

Yes, I agree. It doesn't really make sense to keep adding single-attribute drivers.
hwmon-gpio-simple, maybe, to indicate that it gets its data from gpio ?
The most difficult part of such a driver would probably be to define acceptable
devicetree properties.

>>> Are the alarms no longer appropriate to appear in hwmon?  But if we
>>> connect the chip's I2C interface, then those same alarms should appear
>>> in hwmon?
>>>
>>> That just doesn't make sense to me.
>>>
>>> Also consider the gpio-fan driver.  That's pretty much just an
>>> interface to a gpio too.
>>> Or consider the leds-gpio driver.  That allows a gpio controlled LED to
>>> appear in the kernel's led subsystem.  It doesn't do anything besides
>>> turn the gpio on and off.  It's hardly needed if all we cared about was
>>> controlling the LED in some way.  Yet it's used quite often.
>>>
>>
>> The difference here is that those are distinct drivers, with no other hardware
>> behind it to control the LEDs or to report fan speeds.
>>
>> For voltage monitoring, that is not normally the case. It is much more likely
>> that there is in fact a hardware monitoring or power control chip, the
>> (or an) alarm output of that chip is connected to a gpio line, and its control
>> interface is connected. If so, the driver for that chip should be enhanced
>> to support interrupts, and to report the status with its own sysfs attributes.
>>
>> Agreed, that might be more work for a given hardware, and it would have to
>> be repeated for each chip with that configuration which doesn't already
>> have interrupt support. Meaning, unfortunately, almost all hwmon drivers.
>> It might even be necessary to implement an i2c or spi controller driver
>> if that does not exist. But it would be the appropriate solution.
> 
> Please see my above comments.
> 
>>> So it seems perfectly correct to me that a GPIO based hardware
>>> monitoring alarm should appear in the kernel's hardware monitoring
>>> subsystem with all the other hardware monitoring alarms.
>>>
>> We can only consider a driver reporting a specific attribute if there
>> is a board which doesn't support anything else.
>>
>>> The ability of the hwmon driver to cause certain other devices to be
>>> removed is a different question.
>>>
>> I disagree. That functionality is essential to the driver as submitted.
> 
> You're right thats important for my use-case, but I got you, thats not a
> hwmon related problem. If I understood the device-model correctly there
> are absolut no problems to hot-unplug devices, this is always the case
> if we unbind a driver from user-space. So why we can't handle it
> directly in the kernel. Are there any concerns? If not can you give me
> some hints to get the logic at thr right place.
> 
Well, the easiest solution would have been to do nothing code-wise and
register a gpio-keys instance on the gpio pin to create a KEY_POWER event.
Of course that might be considered an abuse of the input subsystem,
as Dmitry has pointed out, and is probably not acceptable. Now, if you
want that functionality, you could probably write some udev rules and
accomplish the same by listening for a change event from a sysfs attribute
supporting it (I think KEY_POWER is not really handled in the kernel,
but I am not entirely sure).

Handling the event in the kernel is more tricky. First, I don't think the
selective device removal as you have suggested would be acceptable anywhere;
it may cause all kinds of trouble. The thermal subsystem supports the
functionality to shut down the kernel in emergencies, but is limited
in its scope (or at least I think so) to thermal events. Anything else
would have to be discussed. I for my part prefer handling it from userspace.

Guenter

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

[Marco Felsch]

On 18-10-30 06:13, Guenter Roeck wrote:
> On 10/30/18 3:47 AM, Marco Felsch wrote:
> > On 18-10-29 18:12, Guenter Roeck wrote:
> > > On 10/29/18 2:16 PM, Trent Piepho wrote:

[Snip]

> > > > 
> > > > Consider, what if we had a "classic" hwmon chip, but on this board the
> > > > I2C/LPC/SPI interface was not connected as an appropriate master was
> > > > not available, and the default configuration of the chip was
> > > > acceptable.  The chip's alarm outputs are connected to GPIOs, as it
> > > > normal for a hwmon chip with alarm outputs.
> > > > 
> > > "If we had" is theory. Do we ? We don't usually add code to the kernel
> > > just in case the hardware it supports might be out there.
> > 
> > Yes, there are "good old" hwmon chips without any management interface,
> > e.g. LTC694-3.3, LTC7862, ... also there might be a discrete hw circuit.
> > I think it's better to handle those "simple" chips by a generic hwmon
> > driver. By simple I mean chips which informs the host only by toggling a
> > gpio to report a state. For this purpose my driver (including name
> > convention) isn't generic enough, I think about a hwmon-simple device.
> > Such a device can support reporting states, no voltage/power/temp values.
> > 
> 
> Yes, I agree. It doesn't really make sense to keep adding single-attribute drivers.
> hwmon-gpio-simple, maybe, to indicate that it gets its data from gpio ?

hwmon-gpio-simple sounds ok for me.

> The most difficult part of such a driver would probably be to define acceptable
> devicetree properties.

That's true! One possible solution could be:

hwmon_dev {
	compatible = "hwmon-gpio-simple";
	name = "gpio-generic-hwmon";
	update-interval-ms = 100;

	hwmon-gpio-simple,dev@0 {
		reg = <0>;
		gpio = <gpio3 15 GPIO_ACTIVE_LOW>;
		hwmon-gpio-simple,type = "in";
		hwmon-gpio-simple,report = "crit_alarm";
	};

	hwmon-gpio-simple,dev@1 {
		reg = <1>;
		gpio = <gpio3 19 GPIO_ACTIVE_LOW>;
		hwmon-gpio-simple,type = "temp";
		hwmon-gpio-simple,report = "alarm";
	};
};

[SNIP]

> > > > The ability of the hwmon driver to cause certain other devices to be
> > > > removed is a different question.
> > > > 
> > > I disagree. That functionality is essential to the driver as submitted.
> > 
> > You're right thats important for my use-case, but I got you, thats not a
> > hwmon related problem. If I understood the device-model correctly there
> > are absolut no problems to hot-unplug devices, this is always the case
> > if we unbind a driver from user-space. So why we can't handle it
> > directly in the kernel. Are there any concerns? If not can you give me
> > some hints to get the logic at thr right place.
> > 
> Well, the easiest solution would have been to do nothing code-wise and
> register a gpio-keys instance on the gpio pin to create a KEY_POWER event.
> Of course that might be considered an abuse of the input subsystem,
> as Dmitry has pointed out, and is probably not acceptable. Now, if you
> want that functionality, you could probably write some udev rules and
> accomplish the same by listening for a change event from a sysfs attribute
> supporting it (I think KEY_POWER is not really handled in the kernel,
> but I am not entirely sure).

I came from the input framework, as you pointed out.

> Handling the event in the kernel is more tricky. First, I don't think the
> selective device removal as you have suggested would be acceptable anywhere;
> it may cause all kinds of trouble. The thermal subsystem supports the
> functionality to shut down the kernel in emergencies, but is limited
> in its scope (or at least I think so) to thermal events. Anything else
> would have to be discussed. I for my part prefer handling it from userspace.

Imagine that use-case: There is a custom board design which power off
all external devices and leave the host on for a certain time (a few
seconds) upon a low-voltage detection. Now the pins from the external
devices are floating around and producing a huge amount of interrupts,
so the kernel is really busy handling those interrupts and can't handle
user-space processes anymore. The "host keep-on time" was intended to be
used to save all user data currently processed, but this never happen.
Furthermore there can be a high-priority userspace task which can't be
preempted and the exec_time for this task is greater than the "host
keep-on time". So the task which will unbind the devices gets never
scheduled. Hopefully you get me and understand my outlines and why we
need to do the unbinding within the kernel-space.

One solution could be to split the drivers into to: hwmon for measuring
and notifying, of-unbind to unbind registered devices. For this we need
to get a kernel_notification from the hwmon-framework, so the generic
unbinding driver gets informed. Are you agree with me?

Regards,
Marco

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

[Trent Piepho]

On Mon, 2018-10-29 at 18:12 -0700, Guenter Roeck wrote:
> On 10/29/18 2:16 PM, Trent Piepho wrote:
> > 
> > If we ignore the ability to stop other devices, how is this not a hwmon
> > device with just alarm features?
> > 
> 
> Possibly, but the ability to stop other devices is at the core of the driver
> as submitted.

I was thinking along the lines of a driver for gpio based hardware
alarms, that did not include the device stop feature.  Would that also
be quickly NACKed?

> > I2C/LPC/SPI interface was not connected as an appropriate master was
> > not available, and the default configuration of the chip was
> > acceptable.  The chip's alarm outputs are connected to GPIOs, as it
> > normal for a hwmon chip with alarm outputs.
> > 
> 
> "If we had" is theory. Do we ? We don't usually add code to the kernel
> just in case the hardware it supports might be out there.

What I was trying to do was reach the conclusion that a gpio hardware
alarm as a hwmon driver is appropriate via clear steps.

A classic hwmon chip should have a hwmon driver.  We all accept that.

Disconnect i2c interface, keep alarms, does the kernel interface need
to change?  Seems clear to me the answer is no, should still be hwmon.

Replace chip with discrete logic, e.g. an op amp and a few resistors
serving as a voltage comparator, which has the same behavior as the 
hwmon chip as far as the rest of the system is concerned.  Does the
kernel interface need to change now?  Again, seems like it shouldn't
change.

> 
> For voltage monitoring, that is not normally the case. It is much more likely
> that there is in fact a hardware monitoring or power control chip, the
> (or an) alarm output of that chip is connected to a gpio line, and its control
> interface is connected. If so, the driver for that chip should be enhanced
> to support interrupts, and to report the status with its own sysfs attributes.

I agree that writing a specialized driver that pretends a hwmon chip
with a control interface is just a gpio wouldn't be appropriate as an
upstreamable driver for the kernel.  It's more of a one off hack of
expediency.

But it's pretty easy to make a voltage alarm circuit with an op amp. 
Even a differential temperature sensor with hysteresis is just a few
components.

Would a hwmon driver for this be unacceptable?

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

[Trent Piepho]

On Tue, 2018-10-30 at 11:47 +0100, Marco Felsch wrote:
> 
> > 
> > I disagree. That functionality is essential to the driver as submitted.
> 
> You're right thats important for my use-case, but I got you, thats not a
> hwmon related problem. If I understood the device-model correctly there
> are absolut no problems to hot-unplug devices, this is always the case
> if we unbind a driver from user-space. So why we can't handle it
> directly in the kernel. Are there any concerns? If not can you give me
> some hints to get the logic at thr right place.

It is already common to have a userspace process that is signaled via
a hwmon alarm attribute.  Is there a reason one couldn't have a
userspace process poll() on the alarm and unbind or remote devices via
sysfs when it goes off?

Seems like this might also give the opportunity to stop using the
device in a more controlled way, instead of having it just disappear.

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

[Trent Piepho]

On Tue, 2018-10-30 at 18:00 +0100, Marco Felsch wrote:
> On 18-10-30 06:13, Guenter Roeck wrote:
> > On 10/30/18 3:47 AM, Marco Felsch wrote:
> > > 
> hwmon-gpio-simple sounds ok for me.
> 
> > The most difficult part of such a driver would probably be to define acceptable
> > devicetree properties.
> 
> That's true! One possible solution could be:
> 
> hwmon_dev {
> 	compatible = "hwmon-gpio-simple";
> 	name = "gpio-generic-hwmon";
> 	update-interval-ms = 100;
> 
> 	hwmon-gpio-simple,dev@0 {
> 		reg = <0>;
> 		gpio = <gpio3 15 GPIO_ACTIVE_LOW>;
> 		hwmon-gpio-simple,type = "in";
> 		hwmon-gpio-simple,report = "crit_alarm";
> 	};
> 
> 	hwmon-gpio-simple,dev@1 {
> 		reg = <1>;
> 		gpio = <gpio3 19 GPIO_ACTIVE_LOW>;
> 		hwmon-gpio-simple,type = "temp";
> 		hwmon-gpio-simple,report = "alarm";
> 	};
> };

Here's some options:

hwmon_dev {
	/* Orthogonal to existing "gpio-fan" binding. */
	compatible = "gpio-alarm";
	/* Standard DT property for GPIO users is [<name>-]gpios */
	alarm-gpios = <&gpio3 15 GPIO_ACTIVE_LOW>,
	              <&gpio3 19 GPIO_ACTIVE_LOW>;
	/* A <prop>-names property is also a DT standard */
        alarm-gpios-names = "in0", "temp0";
};

The driver can create hwmon alarm attribute(s) based on the name(s).  I
used "alarm" as it seemed to fit the pattern established by the "fan"
driver.  Both the gpio-fan and gpio-alarm driver use gpios, but I think
considering them one driver for that reason does not make sense.

The names are very Linuxy, something that is not liked in DT bindings. 
It also doesn't extend well if you need to add more attributes to each
alarm.  Here's something that's more like what I did for the gpio-leds
binding.

hwmon_dev {
	compatible = "gpio-alarm";
	voltage@0 {
		label = "Battery Voltage Low";
		type = "voltage";
		alarm-gpios = <&gpio3 15 GPIO_ACTIVE_LOW>;
	};
	cputemp@0 {
		label = "CPU Temperature Critical";
		type = "temperature";
		interrupt-parent = <&gpio3>;
		interrupts = <19 IRQ_TYPE_LEVEL_LOW>;
	};
};

Supporting interrupts instead of just a gpio would allow for edge
triggering.      

I can also see that someone might want to create some kind of time
based hysteresis for circuits that don't have that.  While it would be
very easy to add a "linux,debounce = <1000>;" property, I imagine that
would be rejected as configuration in the DT binding.

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

[Guenter Roeck]

On Tue, Oct 30, 2018 at 06:00:26PM +0100, Marco Felsch wrote:
> On 18-10-30 06:13, Guenter Roeck wrote:
> > On 10/30/18 3:47 AM, Marco Felsch wrote:
> > > On 18-10-29 18:12, Guenter Roeck wrote:
> > > > On 10/29/18 2:16 PM, Trent Piepho wrote:
> 
> [Snip]
> 
> > > > > 
> > > > > Consider, what if we had a "classic" hwmon chip, but on this board the
> > > > > I2C/LPC/SPI interface was not connected as an appropriate master was
> > > > > not available, and the default configuration of the chip was
> > > > > acceptable.  The chip's alarm outputs are connected to GPIOs, as it
> > > > > normal for a hwmon chip with alarm outputs.
> > > > > 
> > > > "If we had" is theory. Do we ? We don't usually add code to the kernel
> > > > just in case the hardware it supports might be out there.
> > > 
> > > Yes, there are "good old" hwmon chips without any management interface,
> > > e.g. LTC694-3.3, LTC7862, ... also there might be a discrete hw circuit.
> > > I think it's better to handle those "simple" chips by a generic hwmon
> > > driver. By simple I mean chips which informs the host only by toggling a
> > > gpio to report a state. For this purpose my driver (including name
> > > convention) isn't generic enough, I think about a hwmon-simple device.
> > > Such a device can support reporting states, no voltage/power/temp values.
> > > 
> > 
> > Yes, I agree. It doesn't really make sense to keep adding single-attribute drivers.
> > hwmon-gpio-simple, maybe, to indicate that it gets its data from gpio ?
> 
> hwmon-gpio-simple sounds ok for me.
> 
> > The most difficult part of such a driver would probably be to define acceptable
> > devicetree properties.
> 
> That's true! One possible solution could be:
> 
> hwmon_dev {
> 	compatible = "hwmon-gpio-simple";

This is unlikely to be acceptable for dt maintainers since "hwmon" is a Linuxism.
Not that I have a better idea for an acceptable "compatible" name.

> 	name = "gpio-generic-hwmon";
> 	update-interval-ms = 100;

We would not want to implement any polling in the kernel.

> 
> 	hwmon-gpio-simple,dev@0 {
> 		reg = <0>;
> 		gpio = <gpio3 15 GPIO_ACTIVE_LOW>;
> 		hwmon-gpio-simple,type = "in";
> 		hwmon-gpio-simple,report = "crit_alarm";
> 	};
> 
> 	hwmon-gpio-simple,dev@1 {
> 		reg = <1>;
> 		gpio = <gpio3 19 GPIO_ACTIVE_LOW>;
> 		hwmon-gpio-simple,type = "temp";
> 		hwmon-gpio-simple,report = "alarm";
> 	};
> };
> 
> [SNIP]
> 
> > > > > The ability of the hwmon driver to cause certain other devices to be
> > > > > removed is a different question.
> > > > > 
> > > > I disagree. That functionality is essential to the driver as submitted.
> > > 
> > > You're right thats important for my use-case, but I got you, thats not a
> > > hwmon related problem. If I understood the device-model correctly there
> > > are absolut no problems to hot-unplug devices, this is always the case
> > > if we unbind a driver from user-space. So why we can't handle it
> > > directly in the kernel. Are there any concerns? If not can you give me
> > > some hints to get the logic at thr right place.
> > > 
> > Well, the easiest solution would have been to do nothing code-wise and
> > register a gpio-keys instance on the gpio pin to create a KEY_POWER event.
> > Of course that might be considered an abuse of the input subsystem,
> > as Dmitry has pointed out, and is probably not acceptable. Now, if you
> > want that functionality, you could probably write some udev rules and
> > accomplish the same by listening for a change event from a sysfs attribute
> > supporting it (I think KEY_POWER is not really handled in the kernel,
> > but I am not entirely sure).
> 
> I came from the input framework, as you pointed out.
> 
> > Handling the event in the kernel is more tricky. First, I don't think the
> > selective device removal as you have suggested would be acceptable anywhere;
> > it may cause all kinds of trouble. The thermal subsystem supports the
> > functionality to shut down the kernel in emergencies, but is limited
> > in its scope (or at least I think so) to thermal events. Anything else
> > would have to be discussed. I for my part prefer handling it from userspace.
> 
> Imagine that use-case: There is a custom board design which power off
> all external devices and leave the host on for a certain time (a few
> seconds) upon a low-voltage detection. Now the pins from the external
> devices are floating around and producing a huge amount of interrupts,
> so the kernel is really busy handling those interrupts and can't handle
> user-space processes anymore. The "host keep-on time" was intended to be
> used to save all user data currently processed, but this never happen.
> Furthermore there can be a high-priority userspace task which can't be
> preempted and the exec_time for this task is greater than the "host
> keep-on time". So the task which will unbind the devices gets never
> scheduled. Hopefully you get me and understand my outlines and why we
> need to do the unbinding within the kernel-space.
> 
This would be better handled with a DT overlay and removal of the same
if power goes bad. The power good signal would then be tied to DT overlay
insertion and removal, ie it would be be handled like an "insert/remove"
pin.

> One solution could be to split the drivers into to: hwmon for measuring
> and notifying, of-unbind to unbind registered devices. For this we need
> to get a kernel_notification from the hwmon-framework, so the generic
> unbinding driver gets informed. Are you agree with me?
> 
At a previous employer we had a kernel module which would trigger DT overlay
insertion and removal for OIR capable cards with a number of devices on them.
That worked pretty well. A similar approach might work here. Maybe it is
even possible to integrate it into extcon-gpio.

Guenter

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

[Guenter Roeck]

On Tue, Oct 30, 2018 at 07:34:11PM +0000, Trent Piepho wrote:
> On Tue, 2018-10-30 at 18:00 +0100, Marco Felsch wrote:
> > On 18-10-30 06:13, Guenter Roeck wrote:
> > > On 10/30/18 3:47 AM, Marco Felsch wrote:
> > > > 
> > hwmon-gpio-simple sounds ok for me.
> > 
> > > The most difficult part of such a driver would probably be to define acceptable
> > > devicetree properties.
> > 
> > That's true! One possible solution could be:
> > 
> > hwmon_dev {
> > 	compatible = "hwmon-gpio-simple";
> > 	name = "gpio-generic-hwmon";
> > 	update-interval-ms = 100;
> > 
> > 	hwmon-gpio-simple,dev@0 {
> > 		reg = <0>;
> > 		gpio = <gpio3 15 GPIO_ACTIVE_LOW>;
> > 		hwmon-gpio-simple,type = "in";
> > 		hwmon-gpio-simple,report = "crit_alarm";
> > 	};
> > 
> > 	hwmon-gpio-simple,dev@1 {
> > 		reg = <1>;
> > 		gpio = <gpio3 19 GPIO_ACTIVE_LOW>;
> > 		hwmon-gpio-simple,type = "temp";
> > 		hwmon-gpio-simple,report = "alarm";
> > 	};
> > };
> 
> Here's some options:
> 
> hwmon_dev {
> 	/* Orthogonal to existing "gpio-fan" binding. */
> 	compatible = "gpio-alarm";
> 	/* Standard DT property for GPIO users is [<name>-]gpios */
> 	alarm-gpios = <&gpio3 15 GPIO_ACTIVE_LOW>,
> 	              <&gpio3 19 GPIO_ACTIVE_LOW>;
> 	/* A <prop>-names property is also a DT standard */
>         alarm-gpios-names = "in0", "temp0";

temp1, and it would have to specify which alarm, but, yes, that would
be better.

> };
> 
> The driver can create hwmon alarm attribute(s) based on the name(s).  I
> used "alarm" as it seemed to fit the pattern established by the "fan"
> driver.  Both the gpio-fan and gpio-alarm driver use gpios, but I think
> considering them one driver for that reason does not make sense.
> 
> The names are very Linuxy, something that is not liked in DT bindings. 
> It also doesn't extend well if you need to add more attributes to each
> alarm.  Here's something that's more like what I did for the gpio-leds
> binding.
> 
> hwmon_dev {
> 	compatible = "gpio-alarm";
> 	voltage@0 {
> 		label = "Battery Voltage Low";
> 		type = "voltage";
> 		alarm-gpios = <&gpio3 15 GPIO_ACTIVE_LOW>;
> 	};
> 	cputemp@0 {
> 		label = "CPU Temperature Critical";
> 		type = "temperature";
> 		interrupt-parent = <&gpio3>;
> 		interrupts = <19 IRQ_TYPE_LEVEL_LOW>;
> 	};

Even better, though the type of alarm (generic, min, max, lcrit, crit,
cap, emergency, fault) is still needed. That needs to be specified by
some explicit means, not with a label (though having a label is ok).

There could also be more than one alarm per sensor (eg in0_lcrit_alarm,
in0_min_alarm, in0_max_alarm, in0_crit_alarm), all of which would share
a single label. Something like

#define GPIO_ALARM_GENERIC	0
#define GPIO_ALARM_MIN		1
...

	voltage@0 {
		label = "Battery Voltage";
		type = "voltage";
		alarm-type = <GPIO_ALARM_LCRIT, GPIO_ALARM_CRIT>;
		alarm-gpios = <&gpio3 15 GPIO_ACTIVE_LOW
				&gpio3 16 GPIO_ACTIVE_LOW>;
	};

with some better (acceptable) values for "alarm-type" and the actual fields. 

Guenter

> };
> 
> Supporting interrupts instead of just a gpio would allow for edge
> triggering.      
> 
> I can also see that someone might want to create some kind of time
> based hysteresis for circuits that don't have that.  While it would be
> very easy to add a "linux,debounce = <1000>;" property, I imagine that
> would be rejected as configuration in the DT binding.

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

[Guenter Roeck]

On Tue, Oct 30, 2018 at 06:49:07PM +0000, Trent Piepho wrote:
> On Mon, 2018-10-29 at 18:12 -0700, Guenter Roeck wrote:
> > On 10/29/18 2:16 PM, Trent Piepho wrote:
> > > 
> > > If we ignore the ability to stop other devices, how is this not a hwmon
> > > device with just alarm features?
> > > 
> > 
> > Possibly, but the ability to stop other devices is at the core of the driver
> > as submitted.
> 
> I was thinking along the lines of a driver for gpio based hardware
> alarms, that did not include the device stop feature.  Would that also
> be quickly NACKed?
> 
I think we are beyond that.

Guenter

> > > I2C/LPC/SPI interface was not connected as an appropriate master was
> > > not available, and the default configuration of the chip was
> > > acceptable.  The chip's alarm outputs are connected to GPIOs, as it
> > > normal for a hwmon chip with alarm outputs.
> > > 
> > 
> > "If we had" is theory. Do we ? We don't usually add code to the kernel
> > just in case the hardware it supports might be out there.
> 
> What I was trying to do was reach the conclusion that a gpio hardware
> alarm as a hwmon driver is appropriate via clear steps.
> 
> A classic hwmon chip should have a hwmon driver.  We all accept that.
> 
> Disconnect i2c interface, keep alarms, does the kernel interface need
> to change?  Seems clear to me the answer is no, should still be hwmon.
> 
> Replace chip with discrete logic, e.g. an op amp and a few resistors
> serving as a voltage comparator, which has the same behavior as the 
> hwmon chip as far as the rest of the system is concerned.  Does the
> kernel interface need to change now?  Again, seems like it shouldn't
> change.
> 
> > 
> > For voltage monitoring, that is not normally the case. It is much more likely
> > that there is in fact a hardware monitoring or power control chip, the
> > (or an) alarm output of that chip is connected to a gpio line, and its control
> > interface is connected. If so, the driver for that chip should be enhanced
> > to support interrupts, and to report the status with its own sysfs attributes.
> 
> I agree that writing a specialized driver that pretends a hwmon chip
> with a control interface is just a gpio wouldn't be appropriate as an
> upstreamable driver for the kernel.  It's more of a one off hack of
> expediency.
> 
> But it's pretty easy to make a voltage alarm circuit with an op amp. 
> Even a differential temperature sensor with hysteresis is just a few
> components.
> 
> Would a hwmon driver for this be unacceptable?

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

[Marco Felsch]

On 18-10-30 12:56, Guenter Roeck wrote:
> On Tue, Oct 30, 2018 at 06:00:26PM +0100, Marco Felsch wrote:
> > On 18-10-30 06:13, Guenter Roeck wrote:
> > > On 10/30/18 3:47 AM, Marco Felsch wrote:
> > > > On 18-10-29 18:12, Guenter Roeck wrote:
> > > > > On 10/29/18 2:16 PM, Trent Piepho wrote:
> > 
> > [Snip]
> > 
> > > > > > 
> > > > > > Consider, what if we had a "classic" hwmon chip, but on this board the
> > > > > > I2C/LPC/SPI interface was not connected as an appropriate master was
> > > > > > not available, and the default configuration of the chip was
> > > > > > acceptable.  The chip's alarm outputs are connected to GPIOs, as it
> > > > > > normal for a hwmon chip with alarm outputs.
> > > > > > 
> > > > > "If we had" is theory. Do we ? We don't usually add code to the kernel
> > > > > just in case the hardware it supports might be out there.
> > > > 
> > > > Yes, there are "good old" hwmon chips without any management interface,
> > > > e.g. LTC694-3.3, LTC7862, ... also there might be a discrete hw circuit.
> > > > I think it's better to handle those "simple" chips by a generic hwmon
> > > > driver. By simple I mean chips which informs the host only by toggling a
> > > > gpio to report a state. For this purpose my driver (including name
> > > > convention) isn't generic enough, I think about a hwmon-simple device.
> > > > Such a device can support reporting states, no voltage/power/temp values.
> > > > 
> > > 
> > > Yes, I agree. It doesn't really make sense to keep adding single-attribute drivers.
> > > hwmon-gpio-simple, maybe, to indicate that it gets its data from gpio ?
> > 
> > hwmon-gpio-simple sounds ok for me.
> > 
> > > The most difficult part of such a driver would probably be to define acceptable
> > > devicetree properties.
> > 
> > That's true! One possible solution could be:
> > 
> > hwmon_dev {
> > 	compatible = "hwmon-gpio-simple";
> 
> This is unlikely to be acceptable for dt maintainers since "hwmon" is a Linuxism.
> Not that I have a better idea for an acceptable "compatible" name.
> 
> > 	name = "gpio-generic-hwmon";
> > 	update-interval-ms = 100;
> 
> We would not want to implement any polling in the kernel.
> 
> > 
> > 	hwmon-gpio-simple,dev@0 {
> > 		reg = <0>;
> > 		gpio = <gpio3 15 GPIO_ACTIVE_LOW>;
> > 		hwmon-gpio-simple,type = "in";
> > 		hwmon-gpio-simple,report = "crit_alarm";
> > 	};
> > 
> > 	hwmon-gpio-simple,dev@1 {
> > 		reg = <1>;
> > 		gpio = <gpio3 19 GPIO_ACTIVE_LOW>;
> > 		hwmon-gpio-simple,type = "temp";
> > 		hwmon-gpio-simple,report = "alarm";
> > 	};
> > };
> > 
> > [SNIP]
> > 
> > > > > > The ability of the hwmon driver to cause certain other devices to be
> > > > > > removed is a different question.
> > > > > > 
> > > > > I disagree. That functionality is essential to the driver as submitted.
> > > > 
> > > > You're right thats important for my use-case, but I got you, thats not a
> > > > hwmon related problem. If I understood the device-model correctly there
> > > > are absolut no problems to hot-unplug devices, this is always the case
> > > > if we unbind a driver from user-space. So why we can't handle it
> > > > directly in the kernel. Are there any concerns? If not can you give me
> > > > some hints to get the logic at thr right place.
> > > > 
> > > Well, the easiest solution would have been to do nothing code-wise and
> > > register a gpio-keys instance on the gpio pin to create a KEY_POWER event.
> > > Of course that might be considered an abuse of the input subsystem,
> > > as Dmitry has pointed out, and is probably not acceptable. Now, if you
> > > want that functionality, you could probably write some udev rules and
> > > accomplish the same by listening for a change event from a sysfs attribute
> > > supporting it (I think KEY_POWER is not really handled in the kernel,
> > > but I am not entirely sure).
> > 
> > I came from the input framework, as you pointed out.
> > 
> > > Handling the event in the kernel is more tricky. First, I don't think the
> > > selective device removal as you have suggested would be acceptable anywhere;
> > > it may cause all kinds of trouble. The thermal subsystem supports the
> > > functionality to shut down the kernel in emergencies, but is limited
> > > in its scope (or at least I think so) to thermal events. Anything else
> > > would have to be discussed. I for my part prefer handling it from userspace.
> > 
> > Imagine that use-case: There is a custom board design which power off
> > all external devices and leave the host on for a certain time (a few
> > seconds) upon a low-voltage detection. Now the pins from the external
> > devices are floating around and producing a huge amount of interrupts,
> > so the kernel is really busy handling those interrupts and can't handle
> > user-space processes anymore. The "host keep-on time" was intended to be
> > used to save all user data currently processed, but this never happen.
> > Furthermore there can be a high-priority userspace task which can't be
> > preempted and the exec_time for this task is greater than the "host
> > keep-on time". So the task which will unbind the devices gets never
> > scheduled. Hopefully you get me and understand my outlines and why we
> > need to do the unbinding within the kernel-space.
> > 
> This would be better handled with a DT overlay and removal of the same
> if power goes bad. The power good signal would then be tied to DT overlay
> insertion and removal, ie it would be be handled like an "insert/remove"
> pin.
> 
> > One solution could be to split the drivers into to: hwmon for measuring
> > and notifying, of-unbind to unbind registered devices. For this we need
> > to get a kernel_notification from the hwmon-framework, so the generic
> > unbinding driver gets informed. Are you agree with me?
> > 
> At a previous employer we had a kernel module which would trigger DT overlay
> insertion and removal for OIR capable cards with a number of devices on them.
> That worked pretty well. A similar approach might work here. Maybe it is
> even possible to integrate it into extcon-gpio.

Thanks for your hints.

Marco

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

[Marco Felsch]

On 18-10-30 13:11, Guenter Roeck wrote:
> On Tue, Oct 30, 2018 at 07:34:11PM +0000, Trent Piepho wrote:
> > On Tue, 2018-10-30 at 18:00 +0100, Marco Felsch wrote:
> > > On 18-10-30 06:13, Guenter Roeck wrote:
> > > > On 10/30/18 3:47 AM, Marco Felsch wrote:
> > > > > 
> > > hwmon-gpio-simple sounds ok for me.
> > > 
> > > > The most difficult part of such a driver would probably be to define acceptable
> > > > devicetree properties.
> > > 
> > > That's true! One possible solution could be:
> > > 
> > > hwmon_dev {
> > > 	compatible = "hwmon-gpio-simple";
> > > 	name = "gpio-generic-hwmon";
> > > 	update-interval-ms = 100;
> > > 
> > > 	hwmon-gpio-simple,dev@0 {
> > > 		reg = <0>;
> > > 		gpio = <gpio3 15 GPIO_ACTIVE_LOW>;
> > > 		hwmon-gpio-simple,type = "in";
> > > 		hwmon-gpio-simple,report = "crit_alarm";
> > > 	};
> > > 
> > > 	hwmon-gpio-simple,dev@1 {
> > > 		reg = <1>;
> > > 		gpio = <gpio3 19 GPIO_ACTIVE_LOW>;
> > > 		hwmon-gpio-simple,type = "temp";
> > > 		hwmon-gpio-simple,report = "alarm";
> > > 	};
> > > };
> > 
> > Here's some options:
> > 
> > hwmon_dev {
> > 	/* Orthogonal to existing "gpio-fan" binding. */
> > 	compatible = "gpio-alarm";
> > 	/* Standard DT property for GPIO users is [<name>-]gpios */
> > 	alarm-gpios = <&gpio3 15 GPIO_ACTIVE_LOW>,
> > 	              <&gpio3 19 GPIO_ACTIVE_LOW>;
> > 	/* A <prop>-names property is also a DT standard */
> >         alarm-gpios-names = "in0", "temp0";
> 
> temp1, and it would have to specify which alarm, but, yes, that would
> be better.
> 
> > };
> > 
> > The driver can create hwmon alarm attribute(s) based on the name(s).  I
> > used "alarm" as it seemed to fit the pattern established by the "fan"
> > driver.  Both the gpio-fan and gpio-alarm driver use gpios, but I think
> > considering them one driver for that reason does not make sense.
> > 
> > The names are very Linuxy, something that is not liked in DT bindings. 
> > It also doesn't extend well if you need to add more attributes to each
> > alarm.  Here's something that's more like what I did for the gpio-leds
> > binding.
> > 
> > hwmon_dev {
> > 	compatible = "gpio-alarm";
> > 	voltage@0 {
> > 		label = "Battery Voltage Low";
> > 		type = "voltage";
> > 		alarm-gpios = <&gpio3 15 GPIO_ACTIVE_LOW>;
> > 	};
> > 	cputemp@0 {
> > 		label = "CPU Temperature Critical";
> > 		type = "temperature";
> > 		interrupt-parent = <&gpio3>;
> > 		interrupts = <19 IRQ_TYPE_LEVEL_LOW>;
> > 	};
> 
> Even better, though the type of alarm (generic, min, max, lcrit, crit,
> cap, emergency, fault) is still needed. That needs to be specified by
> some explicit means, not with a label (though having a label is ok).

Thanks for your ideas, looks quite nice.

> There could also be more than one alarm per sensor (eg in0_lcrit_alarm,
> in0_min_alarm, in0_max_alarm, in0_crit_alarm), all of which would share
> a single label. Something like
> 
> #define GPIO_ALARM_GENERIC	0
> #define GPIO_ALARM_MIN		1
> ...
> 
> 	voltage@0 {

		reg = <0>;

I remember that we have to add a reg property if we want to use xyz@0.

> 		label = "Battery Voltage";
> 		type = "voltage";
> 		alarm-type = <GPIO_ALARM_LCRIT, GPIO_ALARM_CRIT>;
> 		alarm-gpios = <&gpio3 15 GPIO_ACTIVE_LOW
> 				&gpio3 16 GPIO_ACTIVE_LOW>;
> 	};
> 
> with some better (acceptable) values for "alarm-type" and the actual fields. 

Should we use the @<reg> suffix to map it to in<reg>_*_alarm or should
we do something like that:

hwmon_dev {
	compatible = "gpio-alarm";

	voltage {
		bat@0 {
			reg = <0>;

	 		label = "Battery Pack1 Voltage";
			alarm-type = <GPIO_ALARM_LCRIT, GPIO_ALARM_CRIT>;
			interrupt-parent = <&gpio3>;
			interrupts = <15 IRQ_TYPE_LEVEL_LOW>,
				     <16 IRQ_TYPE_EDGE_RISING>;
		
		};

		bat@1 {
			reg = <1>;

	 		label = "Battery Pack2 Voltage";
			alarm-type = <GPIO_ALARM_LCRIT, GPIO_ALARM_CRIT>;
			interrupts-extended = <&gpio3 17 IRQ_TYPE_EDGE_FALLING>,
					      <&gpio4 18 IRQ_TYPE_EDGE_FALLING>;
		
		};
	};

	temperature {
		cputemp {
			label = "CPU Temperature Critical";
			alarm-type = <GPIO_ALARM_CRIT>;
			interrupt-parent = <&gpio3>;
			interrupts = <20 IRQ_TYPE_EDGE_FALLING>;
		};
	};
};

Now the subnodes imply the type. Since the hwmon-gpio-simple should
work interrupt driven all the time we should replace the alarm-gpios by
the interrupt property, so we can use the already existing EDGE
flags, as Trent mentoined. Otherwise we have to asume if
the gpio is low-active then the interrupt should be triggered on a
falling edge.

Marco

> Guenter
> 
> > };
> > 
> > Supporting interrupts instead of just a gpio would allow for edge
> > triggering.      
> > 
> > I can also see that someone might want to create some kind of time
> > based hysteresis for circuits that don't have that.  While it would be
> > very easy to add a "linux,debounce = <1000>;" property, I imagine that
> > would be rejected as configuration in the DT binding.

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

[Guenter Roeck]

On 11/1/18 3:40 AM, Marco Felsch wrote:
> On 18-10-30 13:11, Guenter Roeck wrote:
>> On Tue, Oct 30, 2018 at 07:34:11PM +0000, Trent Piepho wrote:
>>> On Tue, 2018-10-30 at 18:00 +0100, Marco Felsch wrote:
>>>> On 18-10-30 06:13, Guenter Roeck wrote:
>>>>> On 10/30/18 3:47 AM, Marco Felsch wrote:
>>>>>>
>>>> hwmon-gpio-simple sounds ok for me.
>>>>
>>>>> The most difficult part of such a driver would probably be to define acceptable
>>>>> devicetree properties.
>>>>
>>>> That's true! One possible solution could be:
>>>>
>>>> hwmon_dev {
>>>> 	compatible = "hwmon-gpio-simple";
>>>> 	name = "gpio-generic-hwmon";
>>>> 	update-interval-ms = 100;
>>>>
>>>> 	hwmon-gpio-simple,dev@0 {
>>>> 		reg = <0>;
>>>> 		gpio = <gpio3 15 GPIO_ACTIVE_LOW>;
>>>> 		hwmon-gpio-simple,type = "in";
>>>> 		hwmon-gpio-simple,report = "crit_alarm";
>>>> 	};
>>>>
>>>> 	hwmon-gpio-simple,dev@1 {
>>>> 		reg = <1>;
>>>> 		gpio = <gpio3 19 GPIO_ACTIVE_LOW>;
>>>> 		hwmon-gpio-simple,type = "temp";
>>>> 		hwmon-gpio-simple,report = "alarm";
>>>> 	};
>>>> };
>>>
>>> Here's some options:
>>>
>>> hwmon_dev {
>>> 	/* Orthogonal to existing "gpio-fan" binding. */
>>> 	compatible = "gpio-alarm";
>>> 	/* Standard DT property for GPIO users is [<name>-]gpios */
>>> 	alarm-gpios = <&gpio3 15 GPIO_ACTIVE_LOW>,
>>> 	              <&gpio3 19 GPIO_ACTIVE_LOW>;
>>> 	/* A <prop>-names property is also a DT standard */
>>>          alarm-gpios-names = "in0", "temp0";
>>
>> temp1, and it would have to specify which alarm, but, yes, that would
>> be better.
>>
>>> };
>>>
>>> The driver can create hwmon alarm attribute(s) based on the name(s).  I
>>> used "alarm" as it seemed to fit the pattern established by the "fan"
>>> driver.  Both the gpio-fan and gpio-alarm driver use gpios, but I think
>>> considering them one driver for that reason does not make sense.
>>>
>>> The names are very Linuxy, something that is not liked in DT bindings.
>>> It also doesn't extend well if you need to add more attributes to each
>>> alarm.  Here's something that's more like what I did for the gpio-leds
>>> binding.
>>>
>>> hwmon_dev {
>>> 	compatible = "gpio-alarm";
>>> 	voltage@0 {
>>> 		label = "Battery Voltage Low";
>>> 		type = "voltage";
>>> 		alarm-gpios = <&gpio3 15 GPIO_ACTIVE_LOW>;
>>> 	};
>>> 	cputemp@0 {
>>> 		label = "CPU Temperature Critical";
>>> 		type = "temperature";
>>> 		interrupt-parent = <&gpio3>;
>>> 		interrupts = <19 IRQ_TYPE_LEVEL_LOW>;
>>> 	};
>>
>> Even better, though the type of alarm (generic, min, max, lcrit, crit,
>> cap, emergency, fault) is still needed. That needs to be specified by
>> some explicit means, not with a label (though having a label is ok).
> 
> Thanks for your ideas, looks quite nice.
> 
>> There could also be more than one alarm per sensor (eg in0_lcrit_alarm,
>> in0_min_alarm, in0_max_alarm, in0_crit_alarm), all of which would share
>> a single label. Something like
>>
>> #define GPIO_ALARM_GENERIC	0
>> #define GPIO_ALARM_MIN		1
>> ...
>>
>> 	voltage@0 {
> 
> 		reg = <0>;
> 
> I remember that we have to add a reg property if we want to use xyz@0.
> 
>> 		label = "Battery Voltage";
>> 		type = "voltage";
>> 		alarm-type = <GPIO_ALARM_LCRIT, GPIO_ALARM_CRIT>;
>> 		alarm-gpios = <&gpio3 15 GPIO_ACTIVE_LOW
>> 				&gpio3 16 GPIO_ACTIVE_LOW>;
>> 	};
>>
>> with some better (acceptable) values for "alarm-type" and the actual fields.
> 
> Should we use the @<reg> suffix to map it to in<reg>_*_alarm or should
> we do something like that:
> 
> hwmon_dev {
> 	compatible = "gpio-alarm";
> 
> 	voltage {
> 		bat@0 {
> 			reg = <0>;
> 
> 	 		label = "Battery Pack1 Voltage";
> 			alarm-type = <GPIO_ALARM_LCRIT, GPIO_ALARM_CRIT>;
> 			interrupt-parent = <&gpio3>;
> 			interrupts = <15 IRQ_TYPE_LEVEL_LOW>,
> 				     <16 IRQ_TYPE_EDGE_RISING>;
> 		
> 		};
> 
> 		bat@1 {
> 			reg = <1>;
> 
> 	 		label = "Battery Pack2 Voltage";
> 			alarm-type = <GPIO_ALARM_LCRIT, GPIO_ALARM_CRIT>;
> 			interrupts-extended = <&gpio3 17 IRQ_TYPE_EDGE_FALLING>,
> 					      <&gpio4 18 IRQ_TYPE_EDGE_FALLING>;
> 		
> 		};
> 	};
> 
> 	temperature {
> 		cputemp {
> 			label = "CPU Temperature Critical";
> 			alarm-type = <GPIO_ALARM_CRIT>;
> 			interrupt-parent = <&gpio3>;
> 			interrupts = <20 IRQ_TYPE_EDGE_FALLING>;
> 		};
> 	};
> };
> 
Works for me.

> Now the subnodes imply the type. Since the hwmon-gpio-simple should
> work interrupt driven all the time we should replace the alarm-gpios by
> the interrupt property, so we can use the already existing EDGE
> flags, as Trent mentoined. Otherwise we have to asume if
> the gpio is low-active then the interrupt should be triggered on a
> falling edge.
> 

Isn't that configurable with devicetree flags ? I don't think a driver
should get involved in deciding the active edge.

Guenter


> Marco
> 
>> Guenter
>>
>>> };
>>>
>>> Supporting interrupts instead of just a gpio would allow for edge
>>> triggering.
>>>
>>> I can also see that someone might want to create some kind of time
>>> based hysteresis for circuits that don't have that.  While it would be
>>> very easy to add a "linux,debounce = <1000>;" property, I imagine that
>>> would be rejected as configuration in the DT binding.
> 

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

[Guenter Roeck]

On 11/1/18 3:40 AM, Marco Felsch wrote:
> On 18-10-30 13:11, Guenter Roeck wrote:
>> On Tue, Oct 30, 2018 at 07:34:11PM +0000, Trent Piepho wrote:
>>> On Tue, 2018-10-30 at 18:00 +0100, Marco Felsch wrote:
>>>> On 18-10-30 06:13, Guenter Roeck wrote:
>>>>> On 10/30/18 3:47 AM, Marco Felsch wrote:
>>>>>>
>>>> hwmon-gpio-simple sounds ok for me.
>>>>
>>>>> The most difficult part of such a driver would probably be to define acceptable
>>>>> devicetree properties.
>>>>
>>>> That's true! One possible solution could be:
>>>>
>>>> hwmon_dev {
>>>> 	compatible = "hwmon-gpio-simple";
>>>> 	name = "gpio-generic-hwmon";
>>>> 	update-interval-ms = 100;
>>>>
>>>> 	hwmon-gpio-simple,dev@0 {
>>>> 		reg = <0>;
>>>> 		gpio = <gpio3 15 GPIO_ACTIVE_LOW>;
>>>> 		hwmon-gpio-simple,type = "in";
>>>> 		hwmon-gpio-simple,report = "crit_alarm";
>>>> 	};
>>>>
>>>> 	hwmon-gpio-simple,dev@1 {
>>>> 		reg = <1>;
>>>> 		gpio = <gpio3 19 GPIO_ACTIVE_LOW>;
>>>> 		hwmon-gpio-simple,type = "temp";
>>>> 		hwmon-gpio-simple,report = "alarm";
>>>> 	};
>>>> };
>>>
>>> Here's some options:
>>>
>>> hwmon_dev {
>>> 	/* Orthogonal to existing "gpio-fan" binding. */
>>> 	compatible = "gpio-alarm";
>>> 	/* Standard DT property for GPIO users is [<name>-]gpios */
>>> 	alarm-gpios = <&gpio3 15 GPIO_ACTIVE_LOW>,
>>> 	              <&gpio3 19 GPIO_ACTIVE_LOW>;
>>> 	/* A <prop>-names property is also a DT standard */
>>>          alarm-gpios-names = "in0", "temp0";
>>
>> temp1, and it would have to specify which alarm, but, yes, that would
>> be better.
>>
>>> };
>>>
>>> The driver can create hwmon alarm attribute(s) based on the name(s).  I
>>> used "alarm" as it seemed to fit the pattern established by the "fan"
>>> driver.  Both the gpio-fan and gpio-alarm driver use gpios, but I think
>>> considering them one driver for that reason does not make sense.
>>>
>>> The names are very Linuxy, something that is not liked in DT bindings.
>>> It also doesn't extend well if you need to add more attributes to each
>>> alarm.  Here's something that's more like what I did for the gpio-leds
>>> binding.
>>>
>>> hwmon_dev {
>>> 	compatible = "gpio-alarm";
>>> 	voltage@0 {
>>> 		label = "Battery Voltage Low";
>>> 		type = "voltage";
>>> 		alarm-gpios = <&gpio3 15 GPIO_ACTIVE_LOW>;
>>> 	};
>>> 	cputemp@0 {
>>> 		label = "CPU Temperature Critical";
>>> 		type = "temperature";
>>> 		interrupt-parent = <&gpio3>;
>>> 		interrupts = <19 IRQ_TYPE_LEVEL_LOW>;
>>> 	};
>>
>> Even better, though the type of alarm (generic, min, max, lcrit, crit,
>> cap, emergency, fault) is still needed. That needs to be specified by
>> some explicit means, not with a label (though having a label is ok).
> 
> Thanks for your ideas, looks quite nice.
> 
>> There could also be more than one alarm per sensor (eg in0_lcrit_alarm,
>> in0_min_alarm, in0_max_alarm, in0_crit_alarm), all of which would share
>> a single label. Something like
>>
>> #define GPIO_ALARM_GENERIC	0
>> #define GPIO_ALARM_MIN		1
>> ...
>>
>> 	voltage@0 {
> 
> 		reg = <0>;
> 
> I remember that we have to add a reg property if we want to use xyz@0.
> 
>> 		label = "Battery Voltage";
>> 		type = "voltage";
>> 		alarm-type = <GPIO_ALARM_LCRIT, GPIO_ALARM_CRIT>;
>> 		alarm-gpios = <&gpio3 15 GPIO_ACTIVE_LOW
>> 				&gpio3 16 GPIO_ACTIVE_LOW>;
>> 	};
>>
>> with some better (acceptable) values for "alarm-type" and the actual fields.
> 
> Should we use the @<reg> suffix to map it to in<reg>_*_alarm or should
> we do something like that:
> 
> hwmon_dev {
> 	compatible = "gpio-alarm";
> 
> 	voltage {
> 		bat@0 {
> 			reg = <0>;
> 
> 	 		label = "Battery Pack1 Voltage";
> 			alarm-type = <GPIO_ALARM_LCRIT, GPIO_ALARM_CRIT>;
> 			interrupt-parent = <&gpio3>;
> 			interrupts = <15 IRQ_TYPE_LEVEL_LOW>,
> 				     <16 IRQ_TYPE_EDGE_RISING>;
> 		
> 		};
> 
> 		bat@1 {
> 			reg = <1>;
> 
> 	 		label = "Battery Pack2 Voltage";
> 			alarm-type = <GPIO_ALARM_LCRIT, GPIO_ALARM_CRIT>;
> 			interrupts-extended = <&gpio3 17 IRQ_TYPE_EDGE_FALLING>,
> 					      <&gpio4 18 IRQ_TYPE_EDGE_FALLING>;
> 		
> 		};
> 	};
> 
> 	temperature {
> 		cputemp {
> 			label = "CPU Temperature Critical";
> 			alarm-type = <GPIO_ALARM_CRIT>;
> 			interrupt-parent = <&gpio3>;
> 			interrupts = <20 IRQ_TYPE_EDGE_FALLING>;
> 		};
> 	};
> };
> 
> Now the subnodes imply the type. Since the hwmon-gpio-simple should
> work interrupt driven all the time we should replace the alarm-gpios by

Note that this isn't entirely correct: If the gpio pin doesn't support
interrupts, the driver would just report the state of the pin.

Guenter

> the interrupt property, so we can use the already existing EDGE
> flags, as Trent mentoined. Otherwise we have to asume if
> the gpio is low-active then the interrupt should be triggered on a
> falling edge.
> 
> Marco
> 
>> Guenter
>>
>>> };
>>>
>>> Supporting interrupts instead of just a gpio would allow for edge
>>> triggering.
>>>
>>> I can also see that someone might want to create some kind of time
>>> based hysteresis for circuits that don't have that.  While it would be
>>> very easy to add a "linux,debounce = <1000>;" property, I imagine that
>>> would be rejected as configuration in the DT binding.
> 

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

[Marco Felsch]

On 18-11-01 06:01, Guenter Roeck wrote:
> On 11/1/18 3:40 AM, Marco Felsch wrote:
> > On 18-10-30 13:11, Guenter Roeck wrote:
> > > On Tue, Oct 30, 2018 at 07:34:11PM +0000, Trent Piepho wrote:
> > > > On Tue, 2018-10-30 at 18:00 +0100, Marco Felsch wrote:
> > > > > On 18-10-30 06:13, Guenter Roeck wrote:
> > > > > > On 10/30/18 3:47 AM, Marco Felsch wrote:
> > > > > > > 
> > > > > hwmon-gpio-simple sounds ok for me.
> > > > > 
> > > > > > The most difficult part of such a driver would probably be to define acceptable
> > > > > > devicetree properties.
> > > > > 
> > > > > That's true! One possible solution could be:
> > > > > 
> > > > > hwmon_dev {
> > > > > 	compatible = "hwmon-gpio-simple";
> > > > > 	name = "gpio-generic-hwmon";
> > > > > 	update-interval-ms = 100;
> > > > > 
> > > > > 	hwmon-gpio-simple,dev@0 {
> > > > > 		reg = <0>;
> > > > > 		gpio = <gpio3 15 GPIO_ACTIVE_LOW>;
> > > > > 		hwmon-gpio-simple,type = "in";
> > > > > 		hwmon-gpio-simple,report = "crit_alarm";
> > > > > 	};
> > > > > 
> > > > > 	hwmon-gpio-simple,dev@1 {
> > > > > 		reg = <1>;
> > > > > 		gpio = <gpio3 19 GPIO_ACTIVE_LOW>;
> > > > > 		hwmon-gpio-simple,type = "temp";
> > > > > 		hwmon-gpio-simple,report = "alarm";
> > > > > 	};
> > > > > };
> > > > 
> > > > Here's some options:
> > > > 
> > > > hwmon_dev {
> > > > 	/* Orthogonal to existing "gpio-fan" binding. */
> > > > 	compatible = "gpio-alarm";
> > > > 	/* Standard DT property for GPIO users is [<name>-]gpios */
> > > > 	alarm-gpios = <&gpio3 15 GPIO_ACTIVE_LOW>,
> > > > 	              <&gpio3 19 GPIO_ACTIVE_LOW>;
> > > > 	/* A <prop>-names property is also a DT standard */
> > > >          alarm-gpios-names = "in0", "temp0";
> > > 
> > > temp1, and it would have to specify which alarm, but, yes, that would
> > > be better.
> > > 
> > > > };
> > > > 
> > > > The driver can create hwmon alarm attribute(s) based on the name(s).  I
> > > > used "alarm" as it seemed to fit the pattern established by the "fan"
> > > > driver.  Both the gpio-fan and gpio-alarm driver use gpios, but I think
> > > > considering them one driver for that reason does not make sense.
> > > > 
> > > > The names are very Linuxy, something that is not liked in DT bindings.
> > > > It also doesn't extend well if you need to add more attributes to each
> > > > alarm.  Here's something that's more like what I did for the gpio-leds
> > > > binding.
> > > > 
> > > > hwmon_dev {
> > > > 	compatible = "gpio-alarm";
> > > > 	voltage@0 {
> > > > 		label = "Battery Voltage Low";
> > > > 		type = "voltage";
> > > > 		alarm-gpios = <&gpio3 15 GPIO_ACTIVE_LOW>;
> > > > 	};
> > > > 	cputemp@0 {
> > > > 		label = "CPU Temperature Critical";
> > > > 		type = "temperature";
> > > > 		interrupt-parent = <&gpio3>;
> > > > 		interrupts = <19 IRQ_TYPE_LEVEL_LOW>;
> > > > 	};
> > > 
> > > Even better, though the type of alarm (generic, min, max, lcrit, crit,
> > > cap, emergency, fault) is still needed. That needs to be specified by
> > > some explicit means, not with a label (though having a label is ok).
> > 
> > Thanks for your ideas, looks quite nice.
> > 
> > > There could also be more than one alarm per sensor (eg in0_lcrit_alarm,
> > > in0_min_alarm, in0_max_alarm, in0_crit_alarm), all of which would share
> > > a single label. Something like
> > > 
> > > #define GPIO_ALARM_GENERIC	0
> > > #define GPIO_ALARM_MIN		1
> > > ...
> > > 
> > > 	voltage@0 {
> > 
> > 		reg = <0>;
> > 
> > I remember that we have to add a reg property if we want to use xyz@0.
> > 
> > > 		label = "Battery Voltage";
> > > 		type = "voltage";
> > > 		alarm-type = <GPIO_ALARM_LCRIT, GPIO_ALARM_CRIT>;
> > > 		alarm-gpios = <&gpio3 15 GPIO_ACTIVE_LOW
> > > 				&gpio3 16 GPIO_ACTIVE_LOW>;
> > > 	};
> > > 
> > > with some better (acceptable) values for "alarm-type" and the actual fields.
> > 
> > Should we use the @<reg> suffix to map it to in<reg>_*_alarm or should
> > we do something like that:
> > 
> > hwmon_dev {
> > 	compatible = "gpio-alarm";
> > 
> > 	voltage {
> > 		bat@0 {
> > 			reg = <0>;
> > 
> > 	 		label = "Battery Pack1 Voltage";
> > 			alarm-type = <GPIO_ALARM_LCRIT, GPIO_ALARM_CRIT>;
> > 			interrupt-parent = <&gpio3>;
> > 			interrupts = <15 IRQ_TYPE_LEVEL_LOW>,
> > 				     <16 IRQ_TYPE_EDGE_RISING>;
> > 		
> > 		};
> > 
> > 		bat@1 {
> > 			reg = <1>;
> > 
> > 	 		label = "Battery Pack2 Voltage";
> > 			alarm-type = <GPIO_ALARM_LCRIT, GPIO_ALARM_CRIT>;
> > 			interrupts-extended = <&gpio3 17 IRQ_TYPE_EDGE_FALLING>,
> > 					      <&gpio4 18 IRQ_TYPE_EDGE_FALLING>;
> > 		
> > 		};
> > 	};
> > 
> > 	temperature {
> > 		cputemp {
> > 			label = "CPU Temperature Critical";
> > 			alarm-type = <GPIO_ALARM_CRIT>;
> > 			interrupt-parent = <&gpio3>;
> > 			interrupts = <20 IRQ_TYPE_EDGE_FALLING>;
> > 		};
> > 	};
> > };
> > 
> Works for me.
> 
> > Now the subnodes imply the type. Since the hwmon-gpio-simple should
> > work interrupt driven all the time we should replace the alarm-gpios by
> > the interrupt property, so we can use the already existing EDGE
> > flags, as Trent mentoined. Otherwise we have to asume if
> > the gpio is low-active then the interrupt should be triggered on a
> > falling edge.
> > 
> 
> Isn't that configurable with devicetree flags ? I don't think a driver
> should get involved in deciding the active edge.

No, AFAIK we can only specify the active level types for gpios. This
made sense to me, because I saw no gpio-controller which support
'edge-level' reporting (however it will be called) currently.

Deciding the edge within the driver was/is the only solution is found
currently, but I'm with you, thats a bit stupid. I open minded for other
solutions.

Marco

> Guenter
> 
> 
> > Marco
> > 
> > > Guenter
> > > 
> > > > };
> > > > 
> > > > Supporting interrupts instead of just a gpio would allow for edge
> > > > triggering.
> > > > 
> > > > I can also see that someone might want to create some kind of time
> > > > based hysteresis for circuits that don't have that.  While it would be
> > > > very easy to add a "linux,debounce = <1000>;" property, I imagine that
> > > > would be rejected as configuration in the DT binding.
> > 
> 
> 

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

[Marco Felsch]

On 18-11-01 06:02, Guenter Roeck wrote:
> On 11/1/18 3:40 AM, Marco Felsch wrote:
> > On 18-10-30 13:11, Guenter Roeck wrote:
> > > On Tue, Oct 30, 2018 at 07:34:11PM +0000, Trent Piepho wrote:
> > > > On Tue, 2018-10-30 at 18:00 +0100, Marco Felsch wrote:
> > > > > On 18-10-30 06:13, Guenter Roeck wrote:
> > > > > > On 10/30/18 3:47 AM, Marco Felsch wrote:
> > > > > > > 
> > > > > hwmon-gpio-simple sounds ok for me.
> > > > > 
> > > > > > The most difficult part of such a driver would probably be to define acceptable
> > > > > > devicetree properties.
> > > > > 
> > > > > That's true! One possible solution could be:
> > > > > 
> > > > > hwmon_dev {
> > > > > 	compatible = "hwmon-gpio-simple";
> > > > > 	name = "gpio-generic-hwmon";
> > > > > 	update-interval-ms = 100;
> > > > > 
> > > > > 	hwmon-gpio-simple,dev@0 {
> > > > > 		reg = <0>;
> > > > > 		gpio = <gpio3 15 GPIO_ACTIVE_LOW>;
> > > > > 		hwmon-gpio-simple,type = "in";
> > > > > 		hwmon-gpio-simple,report = "crit_alarm";
> > > > > 	};
> > > > > 
> > > > > 	hwmon-gpio-simple,dev@1 {
> > > > > 		reg = <1>;
> > > > > 		gpio = <gpio3 19 GPIO_ACTIVE_LOW>;
> > > > > 		hwmon-gpio-simple,type = "temp";
> > > > > 		hwmon-gpio-simple,report = "alarm";
> > > > > 	};
> > > > > };
> > > > 
> > > > Here's some options:
> > > > 
> > > > hwmon_dev {
> > > > 	/* Orthogonal to existing "gpio-fan" binding. */
> > > > 	compatible = "gpio-alarm";
> > > > 	/* Standard DT property for GPIO users is [<name>-]gpios */
> > > > 	alarm-gpios = <&gpio3 15 GPIO_ACTIVE_LOW>,
> > > > 	              <&gpio3 19 GPIO_ACTIVE_LOW>;
> > > > 	/* A <prop>-names property is also a DT standard */
> > > >          alarm-gpios-names = "in0", "temp0";
> > > 
> > > temp1, and it would have to specify which alarm, but, yes, that would
> > > be better.
> > > 
> > > > };
> > > > 
> > > > The driver can create hwmon alarm attribute(s) based on the name(s).  I
> > > > used "alarm" as it seemed to fit the pattern established by the "fan"
> > > > driver.  Both the gpio-fan and gpio-alarm driver use gpios, but I think
> > > > considering them one driver for that reason does not make sense.
> > > > 
> > > > The names are very Linuxy, something that is not liked in DT bindings.
> > > > It also doesn't extend well if you need to add more attributes to each
> > > > alarm.  Here's something that's more like what I did for the gpio-leds
> > > > binding.
> > > > 
> > > > hwmon_dev {
> > > > 	compatible = "gpio-alarm";
> > > > 	voltage@0 {
> > > > 		label = "Battery Voltage Low";
> > > > 		type = "voltage";
> > > > 		alarm-gpios = <&gpio3 15 GPIO_ACTIVE_LOW>;
> > > > 	};
> > > > 	cputemp@0 {
> > > > 		label = "CPU Temperature Critical";
> > > > 		type = "temperature";
> > > > 		interrupt-parent = <&gpio3>;
> > > > 		interrupts = <19 IRQ_TYPE_LEVEL_LOW>;
> > > > 	};
> > > 
> > > Even better, though the type of alarm (generic, min, max, lcrit, crit,
> > > cap, emergency, fault) is still needed. That needs to be specified by
> > > some explicit means, not with a label (though having a label is ok).
> > 
> > Thanks for your ideas, looks quite nice.
> > 
> > > There could also be more than one alarm per sensor (eg in0_lcrit_alarm,
> > > in0_min_alarm, in0_max_alarm, in0_crit_alarm), all of which would share
> > > a single label. Something like
> > > 
> > > #define GPIO_ALARM_GENERIC	0
> > > #define GPIO_ALARM_MIN		1
> > > ...
> > > 
> > > 	voltage@0 {
> > 
> > 		reg = <0>;
> > 
> > I remember that we have to add a reg property if we want to use xyz@0.
> > 
> > > 		label = "Battery Voltage";
> > > 		type = "voltage";
> > > 		alarm-type = <GPIO_ALARM_LCRIT, GPIO_ALARM_CRIT>;
> > > 		alarm-gpios = <&gpio3 15 GPIO_ACTIVE_LOW
> > > 				&gpio3 16 GPIO_ACTIVE_LOW>;
> > > 	};
> > > 
> > > with some better (acceptable) values for "alarm-type" and the actual fields.
> > 
> > Should we use the @<reg> suffix to map it to in<reg>_*_alarm or should
> > we do something like that:
> > 
> > hwmon_dev {
> > 	compatible = "gpio-alarm";
> > 
> > 	voltage {
> > 		bat@0 {
> > 			reg = <0>;
> > 
> > 	 		label = "Battery Pack1 Voltage";
> > 			alarm-type = <GPIO_ALARM_LCRIT, GPIO_ALARM_CRIT>;
> > 			interrupt-parent = <&gpio3>;
> > 			interrupts = <15 IRQ_TYPE_LEVEL_LOW>,
> > 				     <16 IRQ_TYPE_EDGE_RISING>;
> > 		
> > 		};
> > 
> > 		bat@1 {
> > 			reg = <1>;
> > 
> > 	 		label = "Battery Pack2 Voltage";
> > 			alarm-type = <GPIO_ALARM_LCRIT, GPIO_ALARM_CRIT>;
> > 			interrupts-extended = <&gpio3 17 IRQ_TYPE_EDGE_FALLING>,
> > 					      <&gpio4 18 IRQ_TYPE_EDGE_FALLING>;
> > 		
> > 		};
> > 	};
> > 
> > 	temperature {
> > 		cputemp {
> > 			label = "CPU Temperature Critical";
> > 			alarm-type = <GPIO_ALARM_CRIT>;
> > 			interrupt-parent = <&gpio3>;
> > 			interrupts = <20 IRQ_TYPE_EDGE_FALLING>;
> > 		};
> > 	};
> > };
> > 
> > Now the subnodes imply the type. Since the hwmon-gpio-simple should
> > work interrupt driven all the time we should replace the alarm-gpios by
> 
> Note that this isn't entirely correct: If the gpio pin doesn't support
> interrupts, the driver would just report the state of the pin.

Okay, but how do we detect and report a alarm if you won't have a
(fallback) polling mechanism nor a gpio which doesn't support
interrupts? Should the user poll the sysfs-entry instead?

Marco
 
> Guenter
> 
> > the interrupt property, so we can use the already existing EDGE
> > flags, as Trent mentoined. Otherwise we have to asume if
> > the gpio is low-active then the interrupt should be triggered on a
> > falling edge.
> > 
> > Marco
> > 
> > > Guenter
> > > 
> > > > };
> > > > 
> > > > Supporting interrupts instead of just a gpio would allow for edge
> > > > triggering.
> > > > 
> > > > I can also see that someone might want to create some kind of time
> > > > based hysteresis for circuits that don't have that.  While it would be
> > > > very easy to add a "linux,debounce = <1000>;" property, I imagine that
> > > > would be rejected as configuration in the DT binding.
> > 
> 
> 

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

[Guenter Roeck]

On Thu, Nov 01, 2018 at 03:58:58PM +0100, Marco Felsch wrote:
> On 18-11-01 06:02, Guenter Roeck wrote:
> > On 11/1/18 3:40 AM, Marco Felsch wrote:
> > > On 18-10-30 13:11, Guenter Roeck wrote:
> > > > On Tue, Oct 30, 2018 at 07:34:11PM +0000, Trent Piepho wrote:
> > > > > On Tue, 2018-10-30 at 18:00 +0100, Marco Felsch wrote:
> > > > > > On 18-10-30 06:13, Guenter Roeck wrote:
> > > > > > > On 10/30/18 3:47 AM, Marco Felsch wrote:
> > > > > > > > 
> > > > > > hwmon-gpio-simple sounds ok for me.
> > > > > > 
> > > > > > > The most difficult part of such a driver would probably be to define acceptable
> > > > > > > devicetree properties.
> > > > > > 
> > > > > > That's true! One possible solution could be:
> > > > > > 
> > > > > > hwmon_dev {
> > > > > > 	compatible = "hwmon-gpio-simple";
> > > > > > 	name = "gpio-generic-hwmon";
> > > > > > 	update-interval-ms = 100;
> > > > > > 
> > > > > > 	hwmon-gpio-simple,dev@0 {
> > > > > > 		reg = <0>;
> > > > > > 		gpio = <gpio3 15 GPIO_ACTIVE_LOW>;
> > > > > > 		hwmon-gpio-simple,type = "in";
> > > > > > 		hwmon-gpio-simple,report = "crit_alarm";
> > > > > > 	};
> > > > > > 
> > > > > > 	hwmon-gpio-simple,dev@1 {
> > > > > > 		reg = <1>;
> > > > > > 		gpio = <gpio3 19 GPIO_ACTIVE_LOW>;
> > > > > > 		hwmon-gpio-simple,type = "temp";
> > > > > > 		hwmon-gpio-simple,report = "alarm";
> > > > > > 	};
> > > > > > };
> > > > > 
> > > > > Here's some options:
> > > > > 
> > > > > hwmon_dev {
> > > > > 	/* Orthogonal to existing "gpio-fan" binding. */
> > > > > 	compatible = "gpio-alarm";
> > > > > 	/* Standard DT property for GPIO users is [<name>-]gpios */
> > > > > 	alarm-gpios = <&gpio3 15 GPIO_ACTIVE_LOW>,
> > > > > 	              <&gpio3 19 GPIO_ACTIVE_LOW>;
> > > > > 	/* A <prop>-names property is also a DT standard */
> > > > >          alarm-gpios-names = "in0", "temp0";
> > > > 
> > > > temp1, and it would have to specify which alarm, but, yes, that would
> > > > be better.
> > > > 
> > > > > };
> > > > > 
> > > > > The driver can create hwmon alarm attribute(s) based on the name(s).  I
> > > > > used "alarm" as it seemed to fit the pattern established by the "fan"
> > > > > driver.  Both the gpio-fan and gpio-alarm driver use gpios, but I think
> > > > > considering them one driver for that reason does not make sense.
> > > > > 
> > > > > The names are very Linuxy, something that is not liked in DT bindings.
> > > > > It also doesn't extend well if you need to add more attributes to each
> > > > > alarm.  Here's something that's more like what I did for the gpio-leds
> > > > > binding.
> > > > > 
> > > > > hwmon_dev {
> > > > > 	compatible = "gpio-alarm";
> > > > > 	voltage@0 {
> > > > > 		label = "Battery Voltage Low";
> > > > > 		type = "voltage";
> > > > > 		alarm-gpios = <&gpio3 15 GPIO_ACTIVE_LOW>;
> > > > > 	};
> > > > > 	cputemp@0 {
> > > > > 		label = "CPU Temperature Critical";
> > > > > 		type = "temperature";
> > > > > 		interrupt-parent = <&gpio3>;
> > > > > 		interrupts = <19 IRQ_TYPE_LEVEL_LOW>;
> > > > > 	};
> > > > 
> > > > Even better, though the type of alarm (generic, min, max, lcrit, crit,
> > > > cap, emergency, fault) is still needed. That needs to be specified by
> > > > some explicit means, not with a label (though having a label is ok).
> > > 
> > > Thanks for your ideas, looks quite nice.
> > > 
> > > > There could also be more than one alarm per sensor (eg in0_lcrit_alarm,
> > > > in0_min_alarm, in0_max_alarm, in0_crit_alarm), all of which would share
> > > > a single label. Something like
> > > > 
> > > > #define GPIO_ALARM_GENERIC	0
> > > > #define GPIO_ALARM_MIN		1
> > > > ...
> > > > 
> > > > 	voltage@0 {
> > > 
> > > 		reg = <0>;
> > > 
> > > I remember that we have to add a reg property if we want to use xyz@0.
> > > 
> > > > 		label = "Battery Voltage";
> > > > 		type = "voltage";
> > > > 		alarm-type = <GPIO_ALARM_LCRIT, GPIO_ALARM_CRIT>;
> > > > 		alarm-gpios = <&gpio3 15 GPIO_ACTIVE_LOW
> > > > 				&gpio3 16 GPIO_ACTIVE_LOW>;
> > > > 	};
> > > > 
> > > > with some better (acceptable) values for "alarm-type" and the actual fields.
> > > 
> > > Should we use the @<reg> suffix to map it to in<reg>_*_alarm or should
> > > we do something like that:
> > > 
> > > hwmon_dev {
> > > 	compatible = "gpio-alarm";
> > > 
> > > 	voltage {
> > > 		bat@0 {
> > > 			reg = <0>;
> > > 
> > > 	 		label = "Battery Pack1 Voltage";
> > > 			alarm-type = <GPIO_ALARM_LCRIT, GPIO_ALARM_CRIT>;
> > > 			interrupt-parent = <&gpio3>;
> > > 			interrupts = <15 IRQ_TYPE_LEVEL_LOW>,
> > > 				     <16 IRQ_TYPE_EDGE_RISING>;
> > > 		
> > > 		};
> > > 
> > > 		bat@1 {
> > > 			reg = <1>;
> > > 
> > > 	 		label = "Battery Pack2 Voltage";
> > > 			alarm-type = <GPIO_ALARM_LCRIT, GPIO_ALARM_CRIT>;
> > > 			interrupts-extended = <&gpio3 17 IRQ_TYPE_EDGE_FALLING>,
> > > 					      <&gpio4 18 IRQ_TYPE_EDGE_FALLING>;
> > > 		
> > > 		};
> > > 	};
> > > 
> > > 	temperature {
> > > 		cputemp {
> > > 			label = "CPU Temperature Critical";
> > > 			alarm-type = <GPIO_ALARM_CRIT>;
> > > 			interrupt-parent = <&gpio3>;
> > > 			interrupts = <20 IRQ_TYPE_EDGE_FALLING>;
> > > 		};
> > > 	};
> > > };
> > > 
> > > Now the subnodes imply the type. Since the hwmon-gpio-simple should
> > > work interrupt driven all the time we should replace the alarm-gpios by
> > 
> > Note that this isn't entirely correct: If the gpio pin doesn't support
> > interrupts, the driver would just report the state of the pin.
> 
> Okay, but how do we detect and report a alarm if you won't have a
> (fallback) polling mechanism nor a gpio which doesn't support
> interrupts? Should the user poll the sysfs-entry instead?
> 
Yes. Not different to existing hwmon drivers which don't implement
interrupt handling.

Guenter

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

[Guenter Roeck]

On Thu, Nov 01, 2018 at 03:53:12PM +0100, Marco Felsch wrote:
> On 18-11-01 06:01, Guenter Roeck wrote:
> > On 11/1/18 3:40 AM, Marco Felsch wrote:
> > > On 18-10-30 13:11, Guenter Roeck wrote:
> > > > On Tue, Oct 30, 2018 at 07:34:11PM +0000, Trent Piepho wrote:
> > > > > On Tue, 2018-10-30 at 18:00 +0100, Marco Felsch wrote:
> > > > > > On 18-10-30 06:13, Guenter Roeck wrote:
> > > > > > > On 10/30/18 3:47 AM, Marco Felsch wrote:
> > > > > > > > 
> > > > > > hwmon-gpio-simple sounds ok for me.
> > > > > > 
> > > > > > > The most difficult part of such a driver would probably be to define acceptable
> > > > > > > devicetree properties.
> > > > > > 
> > > > > > That's true! One possible solution could be:
> > > > > > 
> > > > > > hwmon_dev {
> > > > > > 	compatible = "hwmon-gpio-simple";
> > > > > > 	name = "gpio-generic-hwmon";
> > > > > > 	update-interval-ms = 100;
> > > > > > 
> > > > > > 	hwmon-gpio-simple,dev@0 {
> > > > > > 		reg = <0>;
> > > > > > 		gpio = <gpio3 15 GPIO_ACTIVE_LOW>;
> > > > > > 		hwmon-gpio-simple,type = "in";
> > > > > > 		hwmon-gpio-simple,report = "crit_alarm";
> > > > > > 	};
> > > > > > 
> > > > > > 	hwmon-gpio-simple,dev@1 {
> > > > > > 		reg = <1>;
> > > > > > 		gpio = <gpio3 19 GPIO_ACTIVE_LOW>;
> > > > > > 		hwmon-gpio-simple,type = "temp";
> > > > > > 		hwmon-gpio-simple,report = "alarm";
> > > > > > 	};
> > > > > > };
> > > > > 
> > > > > Here's some options:
> > > > > 
> > > > > hwmon_dev {
> > > > > 	/* Orthogonal to existing "gpio-fan" binding. */
> > > > > 	compatible = "gpio-alarm";
> > > > > 	/* Standard DT property for GPIO users is [<name>-]gpios */
> > > > > 	alarm-gpios = <&gpio3 15 GPIO_ACTIVE_LOW>,
> > > > > 	              <&gpio3 19 GPIO_ACTIVE_LOW>;
> > > > > 	/* A <prop>-names property is also a DT standard */
> > > > >          alarm-gpios-names = "in0", "temp0";
> > > > 
> > > > temp1, and it would have to specify which alarm, but, yes, that would
> > > > be better.
> > > > 
> > > > > };
> > > > > 
> > > > > The driver can create hwmon alarm attribute(s) based on the name(s).  I
> > > > > used "alarm" as it seemed to fit the pattern established by the "fan"
> > > > > driver.  Both the gpio-fan and gpio-alarm driver use gpios, but I think
> > > > > considering them one driver for that reason does not make sense.
> > > > > 
> > > > > The names are very Linuxy, something that is not liked in DT bindings.
> > > > > It also doesn't extend well if you need to add more attributes to each
> > > > > alarm.  Here's something that's more like what I did for the gpio-leds
> > > > > binding.
> > > > > 
> > > > > hwmon_dev {
> > > > > 	compatible = "gpio-alarm";
> > > > > 	voltage@0 {
> > > > > 		label = "Battery Voltage Low";
> > > > > 		type = "voltage";
> > > > > 		alarm-gpios = <&gpio3 15 GPIO_ACTIVE_LOW>;
> > > > > 	};
> > > > > 	cputemp@0 {
> > > > > 		label = "CPU Temperature Critical";
> > > > > 		type = "temperature";
> > > > > 		interrupt-parent = <&gpio3>;
> > > > > 		interrupts = <19 IRQ_TYPE_LEVEL_LOW>;
> > > > > 	};
> > > > 
> > > > Even better, though the type of alarm (generic, min, max, lcrit, crit,
> > > > cap, emergency, fault) is still needed. That needs to be specified by
> > > > some explicit means, not with a label (though having a label is ok).
> > > 
> > > Thanks for your ideas, looks quite nice.
> > > 
> > > > There could also be more than one alarm per sensor (eg in0_lcrit_alarm,
> > > > in0_min_alarm, in0_max_alarm, in0_crit_alarm), all of which would share
> > > > a single label. Something like
> > > > 
> > > > #define GPIO_ALARM_GENERIC	0
> > > > #define GPIO_ALARM_MIN		1
> > > > ...
> > > > 
> > > > 	voltage@0 {
> > > 
> > > 		reg = <0>;
> > > 
> > > I remember that we have to add a reg property if we want to use xyz@0.
> > > 
> > > > 		label = "Battery Voltage";
> > > > 		type = "voltage";
> > > > 		alarm-type = <GPIO_ALARM_LCRIT, GPIO_ALARM_CRIT>;
> > > > 		alarm-gpios = <&gpio3 15 GPIO_ACTIVE_LOW
> > > > 				&gpio3 16 GPIO_ACTIVE_LOW>;
> > > > 	};
> > > > 
> > > > with some better (acceptable) values for "alarm-type" and the actual fields.
> > > 
> > > Should we use the @<reg> suffix to map it to in<reg>_*_alarm or should
> > > we do something like that:
> > > 
> > > hwmon_dev {
> > > 	compatible = "gpio-alarm";
> > > 
> > > 	voltage {
> > > 		bat@0 {
> > > 			reg = <0>;
> > > 
> > > 	 		label = "Battery Pack1 Voltage";
> > > 			alarm-type = <GPIO_ALARM_LCRIT, GPIO_ALARM_CRIT>;
> > > 			interrupt-parent = <&gpio3>;
> > > 			interrupts = <15 IRQ_TYPE_LEVEL_LOW>,
> > > 				     <16 IRQ_TYPE_EDGE_RISING>;
> > > 		
> > > 		};
> > > 
> > > 		bat@1 {
> > > 			reg = <1>;
> > > 
> > > 	 		label = "Battery Pack2 Voltage";
> > > 			alarm-type = <GPIO_ALARM_LCRIT, GPIO_ALARM_CRIT>;
> > > 			interrupts-extended = <&gpio3 17 IRQ_TYPE_EDGE_FALLING>,
> > > 					      <&gpio4 18 IRQ_TYPE_EDGE_FALLING>;
> > > 		
> > > 		};
> > > 	};
> > > 
> > > 	temperature {
> > > 		cputemp {
> > > 			label = "CPU Temperature Critical";
> > > 			alarm-type = <GPIO_ALARM_CRIT>;
> > > 			interrupt-parent = <&gpio3>;
> > > 			interrupts = <20 IRQ_TYPE_EDGE_FALLING>;
> > > 		};
> > > 	};
> > > };
> > > 
> > Works for me.
> > 
> > > Now the subnodes imply the type. Since the hwmon-gpio-simple should
> > > work interrupt driven all the time we should replace the alarm-gpios by
> > > the interrupt property, so we can use the already existing EDGE
> > > flags, as Trent mentoined. Otherwise we have to asume if
> > > the gpio is low-active then the interrupt should be triggered on a
> > > falling edge.
> > > 
> > 
> > Isn't that configurable with devicetree flags ? I don't think a driver
> > should get involved in deciding the active edge.
> 
> No, AFAIK we can only specify the active level types for gpios. This
> made sense to me, because I saw no gpio-controller which support
> 'edge-level' reporting (however it will be called) currently.
> 
> Deciding the edge within the driver was/is the only solution is found
> currently, but I'm with you, thats a bit stupid. I open minded for other
> solutions.
> 
Thinking about it, interrupts should be triggered on both edges for this
to work. Reason is that we want to report all pin status changes to
userspace. Otherwise userspace would have to poll anyway.

Guenter

> Marco
> 
> > Guenter
> > 
> > 
> > > Marco
> > > 
> > > > Guenter
> > > > 
> > > > > };
> > > > > 
> > > > > Supporting interrupts instead of just a gpio would allow for edge
> > > > > triggering.
> > > > > 
> > > > > I can also see that someone might want to create some kind of time
> > > > > based hysteresis for circuits that don't have that.  While it would be
> > > > > very easy to add a "linux,debounce = <1000>;" property, I imagine that
> > > > > would be rejected as configuration in the DT binding.
> > > 
> > 
> > 

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

[Trent Piepho]

On Thu, 2018-11-01 at 11:40 +0100, Marco Felsch wrote:
> On 18-10-30 13:11, Guenter Roeck wrote:
> > On Tue, Oct 30, 2018 at 07:34:11PM +0000, Trent Piepho wrote:
> > > 
> > 	voltage@0 {
> 
> 		reg = <0>;
> 
> I remember that we have to add a reg property if we want to use xyz@0.

Kernel disables that dtc warning, but still seems good to follow that
rule.

> > with some better (acceptable) values for "alarm-type" and the actual fields. 
> 
> Should we use the @<reg> suffix to map it to in<reg>_*_alarm or should
> we do something like that:

Usually the node name, e.g. "node@0", isn't used at all by the driver. 
It's arbitrary, so the dt author can name it what they want.

I've used to identify a device in a udev rule.  So I can have a rule
that acts on a device based on what I want the device to do, rather
than what bus and address the device happens to be on.  The latter are
subject to change based on other devices, SoC pinmuxing, board routing,
etc.

Using the "reg" property to identify the input/temp/fan number seems
totally appropriate to me.

> 
> hwmon_dev {
> 	compatible = "gpio-alarm";
> 
> 	voltage {

I think you'd need something like type = "voltage" here instead of
using the node name.

> 		bat@0 {
> 			reg = <0>;
> 	 		label = "Battery Pack1 Voltage";
> 		};
> 
> 		bat@1 {
> 			reg = <1>;
> 	 		label = "Battery Pack2 Voltage";
> 		};
> 	};

Seems better to me to have the flatter tree with each alarm having a
"type" attribute rather than grouping them by type.

Usually the tree structure of a DT is meant to show a "contains"
relationship, one present in the actual hardware.  A bus with devices
behind it.  Lines attached to controller.  If I've got three op-amps on
GPIOs, two measuring voltage rails and the third on a thermistor, then
IMHO all three are peers.  The hardware design doesn't group the two
voltage rail op-amps in some way that excludes the thermistor op-amp.


> Now the subnodes imply the type. Since the hwmon-gpio-simple should
> work interrupt driven all the time we should replace the alarm-gpios by

Not all GPIOs can generate interrupts.  It would be an unfortunate
hardware design to use such a GPIO for this.  Seems ok to me to defer
that problem to the poor sod who needs to support such hardware if it
ever exists.

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

[Trent Piepho]

On Thu, 2018-11-01 at 08:14 -0700, Guenter Roeck wrote:
> On Thu, Nov 01, 2018 at 03:53:12PM +0100, Marco Felsch wrote:
> > 
> > > 
> > > Isn't that configurable with devicetree flags ? I don't think a driver
> > > should get involved in deciding the active edge.
> > 
> > No, AFAIK we can only specify the active level types for gpios. This
> > made sense to me, because I saw no gpio-controller which support
> > 'edge-level' reporting (however it will be called) currently.

Interrupts types are specific to each interrupt controller, but there
is a standard set of flags that, AFAIK, every Linux controller uses. 
These include IRQ_TYPE_EDGE_BOTH, IRQ_TYPE_EDGE_RISING,
IRQ_TYPE_LEVEL_HIGH, and so on.

So you can support hardware that is inherently edge or level triggered.
 

I have used edge triggered interrupts on GPIO pins on many designs.

From what I remember, most hwmon chips use level triggered signals. 
The general process in the driver:

Level goes to asserted, IRQ handler invoked
Ack interrupt in hwmon chip's alarm register, which usually de-asserts
the alarm line
Set bit in driver state to indicate the alarm attribute should be set
sysfs_notify anything polling the attribute
If alarm line did not de-assert on ack, leave IRQ masked
Sysfs attribute stays set until userspace process acks it (by reading)

The important part here is that the alarm is latched in the driver.  We
don't just report the current alarm status in the attribute.  Otherwise
an alarm could come and go without anyone noticing if they didn't read
the attribute at just the right time.

Put another way, the hwmon alarm attribute means an alarm occurred
since the last time the attribute was reset.  It does not mean the
alarm is currently active.

This also means the driver does not need to continuously track the
alarm status.  Once we detect the first alarm, we don't care what it
does until the alarm status is reset and we need to watch for alarms
again.

If one has something like an op-amp voltage comparator, I think the
driver would register a level interrupt.  It be left masked after the
irq handler notes the alarm, to prevent immediately re-asserting.  This
is normal for level interrupts that can not be de-asserted by an action
of the irq handler.  It would be unmasked on the ack/read of the alarm
attribute.  That would trigger another interrupt if the alarm signal is
still asserted.

If instead, you tried registering for IRQs on both edges, then it's not
reliable.  It's possible for the edges to come in too fast, before the
irq controller or the kernel is ready for them, and then you get out of
sync.

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

[Marco Felsch]

On 18-11-01 18:21, Trent Piepho wrote:
> On Thu, 2018-11-01 at 08:14 -0700, Guenter Roeck wrote:
> > On Thu, Nov 01, 2018 at 03:53:12PM +0100, Marco Felsch wrote:
> > > 
> > > > 
> > > > Isn't that configurable with devicetree flags ? I don't think a driver
> > > > should get involved in deciding the active edge.
> > > 
> > > No, AFAIK we can only specify the active level types for gpios. This
> > > made sense to me, because I saw no gpio-controller which support
> > > 'edge-level' reporting (however it will be called) currently.
> 
> Interrupts types are specific to each interrupt controller, but there
> is a standard set of flags that, AFAIK, every Linux controller uses. 
> These include IRQ_TYPE_EDGE_BOTH, IRQ_TYPE_EDGE_RISING,
> IRQ_TYPE_LEVEL_HIGH, and so on.
> 
> So you can support hardware that is inherently edge or level triggered.

I've been spoken about gpio-controllers and AFAIK there are no edge
types. Interrupt-Controller are a different story, as you pointed out
above.

> I have used edge triggered interrupts on GPIO pins on many designs.
> 
> From what I remember, most hwmon chips use level triggered signals. 
> The general process in the driver:
> 
> Level goes to asserted, IRQ handler invoked
> Ack interrupt in hwmon chip's alarm register, which usually de-asserts
> the alarm line
> Set bit in driver state to indicate the alarm attribute should be set
> sysfs_notify anything polling the attribute
> If alarm line did not de-assert on ack, leave IRQ masked
> Sysfs attribute stays set until userspace process acks it (by reading)

Okay, thanks for that hint. Should we reference this in the
Documentation/hwmon/sysfs-interface?

> The important part here is that the alarm is latched in the driver.  We
> don't just report the current alarm status in the attribute.  Otherwise
> an alarm could come and go without anyone noticing if they didn't read
> the attribute at just the right time.
> 
> Put another way, the hwmon alarm attribute means an alarm occurred
> since the last time the attribute was reset.  It does not mean the
> alarm is currently active.
> 
> This also means the driver does not need to continuously track the
> alarm status.  Once we detect the first alarm, we don't care what it
> does until the alarm status is reset and we need to watch for alarms
> again.
> 
> If one has something like an op-amp voltage comparator, I think the
> driver would register a level interrupt.  It be left masked after the
> irq handler notes the alarm, to prevent immediately re-asserting.  This
> is normal for level interrupts that can not be de-asserted by an action
> of the irq handler.  It would be unmasked on the ack/read of the alarm
> attribute.  That would trigger another interrupt if the alarm signal is
> still asserted.
> 
> If instead, you tried registering for IRQs on both edges, then it's not
> reliable.  It's possible for the edges to come in too fast, before the
> irq controller or the kernel is ready for them, and then you get out of
> sync.

Too fast state changes sounds like a glitch. Anyway, IMHO we should
support support both interrupts and gpios. If the users use gpios they
have to poll the gpio, as Guenter pointed out, else we register a
irq-handler.

Marco

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

[Marco Felsch]

On 18-11-01 17:41, Trent Piepho wrote:
> On Thu, 2018-11-01 at 11:40 +0100, Marco Felsch wrote:
> > On 18-10-30 13:11, Guenter Roeck wrote:
> > > On Tue, Oct 30, 2018 at 07:34:11PM +0000, Trent Piepho wrote:
> > > > 
> > > 	voltage@0 {
> > 
> > 		reg = <0>;
> > 
> > I remember that we have to add a reg property if we want to use xyz@0.
> 
> Kernel disables that dtc warning, but still seems good to follow that
> rule.

Yes, but the maintainers won't confirm it without the reg property.

> > > with some better (acceptable) values for "alarm-type" and the actual fields. 
> > 
> > Should we use the @<reg> suffix to map it to in<reg>_*_alarm or should
> > we do something like that:
> 
> Usually the node name, e.g. "node@0", isn't used at all by the driver. 
> It's arbitrary, so the dt author can name it what they want.

Thats true, but we have to use that convention to conform the DT if we
want to use the reg property.

> I've used to identify a device in a udev rule.  So I can have a rule
> that acts on a device based on what I want the device to do, rather
> than what bus and address the device happens to be on.  The latter are
> subject to change based on other devices, SoC pinmuxing, board routing,
> etc.
> 
> Using the "reg" property to identify the input/temp/fan number seems
> totally appropriate to me.

Okay.

> > 
> > hwmon_dev {
> > 	compatible = "gpio-alarm";
> > 
> > 	voltage {
> 
> I think you'd need something like type = "voltage" here instead of
> using the node name.
> 
> > 		bat@0 {
> > 			reg = <0>;
> > 	 		label = "Battery Pack1 Voltage";
> > 		};
> > 
> > 		bat@1 {
> > 			reg = <1>;
> > 	 		label = "Battery Pack2 Voltage";
> > 		};
> > 	};
> 
> Seems better to me to have the flatter tree with each alarm having a
> "type" attribute rather than grouping them by type.

I'm open minded. Guenter what's your prefered solution?

> Usually the tree structure of a DT is meant to show a "contains"
> relationship, one present in the actual hardware.  A bus with devices
> behind it.  Lines attached to controller.  If I've got three op-amps on
> GPIOs, two measuring voltage rails and the third on a thermistor, then
> IMHO all three are peers.  The hardware design doesn't group the two
> voltage rail op-amps in some way that excludes the thermistor op-amp.
> 
> 
> > Now the subnodes imply the type. Since the hwmon-gpio-simple should
> > work interrupt driven all the time we should replace the alarm-gpios by
> 
> Not all GPIOs can generate interrupts.  It would be an unfortunate
> hardware design to use such a GPIO for this.  Seems ok to me to defer
> that problem to the poor sod who needs to support such hardware if it
> ever exists.

Please look at the other mails. It's important for Guenter to support
the non-interrupt case too. It shouldn't be a big deal to implement both
cases.

Marco

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

[Trent Piepho]

On Fri, 2018-11-02 at 07:38 +0100, Marco Felsch wrote:
> On 18-11-01 18:21, Trent Piepho wrote:
> > On Thu, 2018-11-01 at 08:14 -0700, Guenter Roeck wrote:
> > > On Thu, Nov 01, 2018 at 03:53:12PM +0100, Marco Felsch wrote:
> > > > 
> > > > > 
> > > > > Isn't that configurable with devicetree flags ? I don't think a driver
> > > > > should get involved in deciding the active edge.
> > > > 
> > > > No, AFAIK we can only specify the active level types for gpios. This
> > > > made sense to me, because I saw no gpio-controller which support
> > > > 'edge-level' reporting (however it will be called) currently.
> > 
> > Interrupts types are specific to each interrupt controller, but there
> > is a standard set of flags that, AFAIK, every Linux controller uses. 
> > These include IRQ_TYPE_EDGE_BOTH, IRQ_TYPE_EDGE_RISING,
> > IRQ_TYPE_LEVEL_HIGH, and so on.
> > 
> > So you can support hardware that is inherently edge or level triggered.
> 
> I've been spoken about gpio-controllers and AFAIK there are no edge
> types. Interrupt-Controller are a different story, as you pointed out
> above.

You can use edge triggering with gpios.  Just try writing "rising" or
"falling" into /sys/class/gpio/gpioX/edge

It's level you can't do sysfs.  The irq masking necessary isn't
supported to get it to work in a useful way, i.e. without a live-lock
IRQ loop.

But you can in the kernel.

Normal process is to call gpiod_to_irq() and then use standard IRQF
flags to select level, edge, etc.

> Too fast state changes sounds like a glitch. Anyway, IMHO we should

Linux has no hard real-time guarantee about interrupt latency, so
there's no way you can be sure each interrupt is processed before the
next.

Trying to track level by interrupting on both edges doesn't work well. 
You get out of sync and stuck waiting for something that's already
happened.

> support support both interrupts and gpios. If the users use gpios they
> have to poll the gpio, as Guenter pointed out, else we register a
> irq-handler.

If gpio(d?)_to_irq returns a valid value, why poll?  It usually works
to call this.  Plenty of call sites in the kernel that use it and don't
fallback to polling if it doesn't work.

I think it's fine to call gpiod_to_irq() and fail if that fails, and
let a polling fallback be written if and when the need arises.

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

[Marco Felsch]

On 18-11-02 23:05, Trent Piepho wrote:
> On Fri, 2018-11-02 at 07:38 +0100, Marco Felsch wrote:
> > On 18-11-01 18:21, Trent Piepho wrote:
> > > On Thu, 2018-11-01 at 08:14 -0700, Guenter Roeck wrote:
> > > > On Thu, Nov 01, 2018 at 03:53:12PM +0100, Marco Felsch wrote:
> > > > > 
> > > > > > 
> > > > > > Isn't that configurable with devicetree flags ? I don't think a driver
> > > > > > should get involved in deciding the active edge.
> > > > > 
> > > > > No, AFAIK we can only specify the active level types for gpios. This
> > > > > made sense to me, because I saw no gpio-controller which support
> > > > > 'edge-level' reporting (however it will be called) currently.
> > > 
> > > Interrupts types are specific to each interrupt controller, but there
> > > is a standard set of flags that, AFAIK, every Linux controller uses. 
> > > These include IRQ_TYPE_EDGE_BOTH, IRQ_TYPE_EDGE_RISING,
> > > IRQ_TYPE_LEVEL_HIGH, and so on.
> > > 
> > > So you can support hardware that is inherently edge or level triggered.
> > 
> > I've been spoken about gpio-controllers and AFAIK there are no edge
> > types. Interrupt-Controller are a different story, as you pointed out
> > above.
> 
> You can use edge triggering with gpios.  Just try writing "rising" or
> "falling" into /sys/class/gpio/gpioX/edge

Can we access the gpios trough the sysfs if they are requested by a
driver?

> It's level you can't do sysfs.  The irq masking necessary isn't
> supported to get it to work in a useful way, i.e. without a live-lock
> IRQ loop.
> 
> But you can in the kernel.
> 
> Normal process is to call gpiod_to_irq() and then use standard IRQF
> flags to select level, edge, etc.

Currently I using the gpiod_to_irq() function to convert the sense gpio
into a irq, but I do some magic to determine the edge. I tought there
might be reasons why there are no edge defines in
include/dt-bindings/gpio/gpio.h.

> > Too fast state changes sounds like a glitch. Anyway, IMHO we should
> 
> Linux has no hard real-time guarantee about interrupt latency, so
> there's no way you can be sure each interrupt is processed before the
> next.
> 
> Trying to track level by interrupting on both edges doesn't work well. 
> You get out of sync and stuck waiting for something that's already
> happened.

Yes, I'm with you. 

> > support support both interrupts and gpios. If the users use gpios they
> > have to poll the gpio, as Guenter pointed out, else we register a
> > irq-handler.
> 
> If gpio(d?)_to_irq returns a valid value, why poll?  It usually works
> to call this.  Plenty of call sites in the kernel that use it and don't
> fallback to polling if it doesn't work.
> 
> I think it's fine to call gpiod_to_irq() and fail if that fails, and
> let a polling fallback be written if and when the need arises.

Okay, so no polling for the current solution. Let me summarize our
solution:
 - no polling (currently)
 - dt-node specifies a gpio instead of a interrupt
   -> gpio <-> irq mapping is done by gpiod_to_irq() and fails if gpio
      doesn't support irq's
 - more alarms per sensor

Only one last thing I tought about:

Using a flat design like you mentioned would lead into a "virtual"
address conflict, since both sensors are on the same level. I tought
about i2c/spi/muxes/graph-devices which don't support such "addressing"
scheme.

hwmon_dev {
	compatible = "gpio-alarm";
	bat@0 {
		reg = <0>;
		label = "Battery Pack1 Voltage";
		type = "voltage";
		alarm-type = <GPIO_ALARM_LCRIT, GPIO_ALARM_CRIT>;
		alarm-gpios = <&gpio3 15 GPIO_ACTIVE_LOW
				&gpio3 16 GPIO_ACTIVE_LOW>;
	};
	bat@1 {
		reg = <1>;
		label = "Battery Pack2 Voltage";
		alarm-type = <GPIO_ALARM_LCRIT, GPIO_ALARM_CRIT>;
		alarm-gpios = <&gpio3 9 GPIO_ACTIVE_LOW
				&gpio3 1 GPIO_ACTIVE_LOW>;
	};
	cputemp@0 {
		reg = <0>;
		label = "CPU Temperature Critical";
		type = "temperature";
		alarm-type = <GPIO_ALARM_GENRIC>;
		alarm-gpios = <&gpio4 17 GPIO_ACTIVE_LOW>;
	};
};

Where a more structured layout don't have this "issue".

hwmon_dev {
	compatible = "gpio-alarm";

	voltage {
		bat@0 {
			reg = <0>;
	 		label = "Battery Pack1 Voltage";
			alarm-type = <GPIO_ALARM_LCRIT, GPIO_ALARM_CRIT>;
			alarm-gpios = <&gpio3 15 GPIO_ACTIVE_LOW
					&gpio3 16 GPIO_ACTIVE_LOW>;
		};
		bat@1 {
			reg = <1>;
	 		label = "Battery Pack2 Voltage";
			alarm-type = <GPIO_ALARM_LCRIT, GPIO_ALARM_CRIT>;
			alarm-gpios = <&gpio3 9 GPIO_ACTIVE_LOW
					&gpio3 1 GPIO_ACTIVE_LOW>;
		};
	};
	temperature {
		cputemp {
			label = "CPU Temperature Critical";
			alarm-type = <GPIO_ALARM_GENRIC>;
			alarm-gpios = <&gpio4 17 GPIO_ACTIVE_LOW>;
		};
	};
};

We don't have to take this layout, we can also consider about devices:

hwmon_dev {
	compatible = "gpio-alarm";

	dev@0 {
		reg = <0>;
		voltage {
			label = "Battery Pack1 Voltage";
			alarm-type = <GPIO_ALARM_LCRIT, GPIO_ALARM_CRIT>;
			alarm-gpios = <&gpio3 15 GPIO_ACTIVE_LOW
					&gpio3 16 GPIO_ACTIVE_LOW>;
		};
		temperature {
			label = "Battery Pack1 Temperature Critical";
			alarm-type = <GPIO_ALARM_GENRIC>;
			alarm-gpios = <&gpio4 17 GPIO_ACTIVE_LOW>;
		};
	};
	dev@1 {
		reg = <1>;
		temperature {
			label = "CPU Temperature Critical";
			alarm-type = <GPIO_ALARM_GENRIC>;
			alarm-gpios = <&gpio4 19 GPIO_ACTIVE_LOW>;
		};
	};
};

I don't think that is a issue at all, but I don't know the dt
maintainers opinion of this theme.

Regards,
Marco

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

[Trent Piepho]

On Mon, 2018-11-05 at 09:19 +0100, Marco Felsch wrote:
> On 18-11-02 23:05, Trent Piepho wrote:
> > On Fri, 2018-11-02 at 07:38 +0100, Marco Felsch wrote:
> > > 
> > > > Interrupts types are specific to each interrupt controller, but there
> > > > is a standard set of flags that, AFAIK, every Linux controller uses. 
> > > > These include IRQ_TYPE_EDGE_BOTH, IRQ_TYPE_EDGE_RISING,
> > > > IRQ_TYPE_LEVEL_HIGH, and so on.
> > > > 
> > > > So you can support hardware that is inherently edge or level triggered.
> > > 
> > > I've been spoken about gpio-controllers and AFAIK there are no edge
> > > types. Interrupt-Controller are a different story, as you pointed out
> > > above.
> > 
> > You can use edge triggering with gpios.  Just try writing "rising" or
> > "falling" into /sys/class/gpio/gpioX/edge
> 
> Can we access the gpios trough the sysfs if they are requested by a
> driver?

When I first did the sysfs interface for gpios, you could do that, but
David Brownell wanted it so that you can't access gpios via sysfs if a
driver requested them.  The compromise was that *kernel* code can
explicitly export to sysfs a gpio that is used by a driver (ie. also
requested in kernel code), but you couldn't do it just from userspace.

But that's irrelevant here.  The point is that you can get edge
triggered interrupts on a gpio and if you don't believe me, just try it
for yourself and you'll see it works.  The sysfs interface just
translates into the same calls a kernel driver could make.

> > It's level you can't do sysfs.  The irq masking necessary isn't
> > supported to get it to work in a useful way, i.e. without a live-lock
> > IRQ loop.
> > 
> > But you can in the kernel.
> > 
> > Normal process is to call gpiod_to_irq() and then use standard IRQF
> > flags to select level, edge, etc.
> 
> Currently I using the gpiod_to_irq() function to convert the sense gpio
> into a irq, but I do some magic to determine the edge. I tought there
> might be reasons why there are no edge defines in
> include/dt-bindings/gpio/gpio.h.

Just request the interrupt with IRQF_TRIGGER_RISING and it will work on
almost any SoC.  The reason you see no edge defines with gpio handles
is that edge and level triggering is a interrupt concept, not a gpio
concept.  There are no level triggers defined for gpios either.  The
active low/high flags just define what voltage should be considered
"asserted".  They aren't intended to be related to interrupts.

> Okay, so no polling for the current solution. Let me summarize our
> solution:
>  - no polling (currently)
>  - dt-node specifies a gpio instead of a interrupt
>    -> gpio <-> irq mapping is done by gpiod_to_irq() and fails if gpio
>       doesn't support irq's
>  - more alarms per sensor
> 
> Only one last thing I tought about:
> 
> Using a flat design like you mentioned would lead into a "virtual"
> address conflict, since both sensors are on the same level. I tought
> about i2c/spi/muxes/graph-devices which don't support such "addressing"
> scheme.

You mean a temp alarm and a voltage alarm could both be reg = <1>?  I
don't think anything complains about that.  But it does seem
undesirable.


> hwmon_dev {
> 	compatible = "gpio-alarm";
> 	bat@0 {
> 		reg = <0>;
> 		label = "Battery Pack1 Voltage";
> 		type = "voltage";
> 		alarm-type = <GPIO_ALARM_LCRIT, GPIO_ALARM_CRIT>;

Would have to be <GPIO_ALARM_LCRIT>, <GPIO_ALARM_CRIT>;

I'm not sure if dt bindings prefer symbolic integer constants vs
strings for something which is an enumeration like this.  strings seem
more common to me, e.g. alarm-types = "lcrit", "crit";

> 		alarm-gpios = <&gpio3 15 GPIO_ACTIVE_LOW
> 				&gpio3 16 GPIO_ACTIVE_LOW>;
> 	};
> 	bat@1 {
> 		reg = <1>;
> 		label = "Battery Pack2 Voltage";
> 		alarm-type = <GPIO_ALARM_LCRIT, GPIO_ALARM_CRIT>;
> 		alarm-gpios = <&gpio3 9 GPIO_ACTIVE_LOW
> 				&gpio3 1 GPIO_ACTIVE_LOW>;
> 	};
> 	cputemp@0 {
> 		reg = <0>;
> 		label = "CPU Temperature Critical";
> 		type = "temperature";
> 		alarm-type = <GPIO_ALARM_GENRIC>;
> 		alarm-gpios = <&gpio4 17 GPIO_ACTIVE_LOW>;
> 	};
> };
> 
> Where a more structured layout don't have this "issue".
> 
> hwmon_dev {
> 	compatible = "gpio-alarm";
> 
> 	voltage {
> 		bat@0 {
> 			reg = <0>;
> 	 		label = "Battery Pack1 Voltage";
> 			alarm-type = <GPIO_ALARM_LCRIT, GPIO_ALARM_CRIT>;
> 			alarm-gpios = <&gpio3 15 GPIO_ACTIVE_LOW
> 					&gpio3 16 GPIO_ACTIVE_LOW>;
> 		};
> 		bat@1 {
> 			reg = <1>;
> 	 		label = "Battery Pack2 Voltage";
> 			alarm-type = <GPIO_ALARM_LCRIT, GPIO_ALARM_CRIT>;
> 			alarm-gpios = <&gpio3 9 GPIO_ACTIVE_LOW
> 					&gpio3 1 GPIO_ACTIVE_LOW>;
> 		};
> 	};
> 	temperature {
> 		cputemp {
> 			label = "CPU Temperature Critical";
> 			alarm-type = <GPIO_ALARM_GENRIC>;
> 			alarm-gpios = <&gpio4 17 GPIO_ACTIVE_LOW>;
> 		};
> 	};
> };
> 
> We don't have to take this layout, we can also consider about devices:
> 
> hwmon_dev {
> 	compatible = "gpio-alarm";
> 
> 	dev@0 {
> 		reg = <0>;
> 		voltage {
> 			label = "Battery Pack1 Voltage";
> 			alarm-type = <GPIO_ALARM_LCRIT, GPIO_ALARM_CRIT>;
> 			alarm-gpios = <&gpio3 15 GPIO_ACTIVE_LOW
> 					&gpio3 16 GPIO_ACTIVE_LOW>;
> 		};
> 		temperature {
> 			label = "Battery Pack1 Temperature Critical";
> 			alarm-type = <GPIO_ALARM_GENRIC>;
> 			alarm-gpios = <&gpio4 17 GPIO_ACTIVE_LOW>;
> 		};
> 	};
> 	dev@1 {
> 		reg = <1>;
> 		temperature {
> 			label = "CPU Temperature Critical";
> 			alarm-type = <GPIO_ALARM_GENRIC>;
> 			alarm-gpios = <&gpio4 19 GPIO_ACTIVE_LOW>;
> 		};
> 	};
> };
> 
> I don't think that is a issue at all, but I don't know the dt
> maintainers opinion of this theme.
> 
> Regards,
> Marco

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

[Marco Felsch]

On 18-11-06 20:50, Trent Piepho wrote:
> On Mon, 2018-11-05 at 09:19 +0100, Marco Felsch wrote:
> > On 18-11-02 23:05, Trent Piepho wrote:
> > > On Fri, 2018-11-02 at 07:38 +0100, Marco Felsch wrote:
> > > > 
> > > > > Interrupts types are specific to each interrupt controller, but there
> > > > > is a standard set of flags that, AFAIK, every Linux controller uses. 
> > > > > These include IRQ_TYPE_EDGE_BOTH, IRQ_TYPE_EDGE_RISING,
> > > > > IRQ_TYPE_LEVEL_HIGH, and so on.
> > > > > 
> > > > > So you can support hardware that is inherently edge or level triggered.
> > > > 
> > > > I've been spoken about gpio-controllers and AFAIK there are no edge
> > > > types. Interrupt-Controller are a different story, as you pointed out
> > > > above.
> > > 
> > > You can use edge triggering with gpios.  Just try writing "rising" or
> > > "falling" into /sys/class/gpio/gpioX/edge
> > 
> > Can we access the gpios trough the sysfs if they are requested by a
> > driver?
> 
> When I first did the sysfs interface for gpios, you could do that, but
> David Brownell wanted it so that you can't access gpios via sysfs if a
> driver requested them.  The compromise was that *kernel* code can
> explicitly export to sysfs a gpio that is used by a driver (ie. also
> requested in kernel code), but you couldn't do it just from userspace.
> 
> But that's irrelevant here.  The point is that you can get edge
> triggered interrupts on a gpio and if you don't believe me, just try it
> for yourself and you'll see it works.  The sysfs interface just
> translates into the same calls a kernel driver could make.
> 
> > > It's level you can't do sysfs.  The irq masking necessary isn't
> > > supported to get it to work in a useful way, i.e. without a live-lock
> > > IRQ loop.
> > > 
> > > But you can in the kernel.
> > > 
> > > Normal process is to call gpiod_to_irq() and then use standard IRQF
> > > flags to select level, edge, etc.
> > 
> > Currently I using the gpiod_to_irq() function to convert the sense gpio
> > into a irq, but I do some magic to determine the edge. I tought there
> > might be reasons why there are no edge defines in
> > include/dt-bindings/gpio/gpio.h.
> 
> Just request the interrupt with IRQF_TRIGGER_RISING and it will work on
> almost any SoC.  The reason you see no edge defines with gpio handles
> is that edge and level triggering is a interrupt concept, not a gpio
> concept.  There are no level triggers defined for gpios either.  The
> active low/high flags just define what voltage should be considered
> "asserted".  They aren't intended to be related to interrupts.

Okay, thanks for this hint. So a gpio marked as GPIO_ACTIVE_LOW will
trigger a IRQF_TRIGGER_RISING requested interrupt if the gpio level
(electrical) goes from 1->0? I didn't knew that.

> 
> > Okay, so no polling for the current solution. Let me summarize our
> > solution:
> >  - no polling (currently)
> >  - dt-node specifies a gpio instead of a interrupt
> >    -> gpio <-> irq mapping is done by gpiod_to_irq() and fails if gpio
> >       doesn't support irq's
> >  - more alarms per sensor
> > 
> > Only one last thing I tought about:
> > 
> > Using a flat design like you mentioned would lead into a "virtual"
> > address conflict, since both sensors are on the same level. I tought
> > about i2c/spi/muxes/graph-devices which don't support such "addressing"
> > scheme.
> 
> You mean a temp alarm and a voltage alarm could both be reg = <1>?  I
> don't think anything complains about that.  But it does seem
> undesirable.

Yes, because both types are on the same hierarchy level. As I said it is
more a dt-convention decision.

> > hwmon_dev {
> > 	compatible = "gpio-alarm";
> > 	bat@0 {
> > 		reg = <0>;
> > 		label = "Battery Pack1 Voltage";
> > 		type = "voltage";
> > 		alarm-type = <GPIO_ALARM_LCRIT, GPIO_ALARM_CRIT>;
> 
> Would have to be <GPIO_ALARM_LCRIT>, <GPIO_ALARM_CRIT>;
> 
> I'm not sure if dt bindings prefer symbolic integer constants vs
> strings for something which is an enumeration like this.  strings seem
> more common to me, e.g. alarm-types = "lcrit", "crit";

That's a good question. I term of parsing, the non string variant should
be faster. I don't have any preference, but will try the string variant
first ;)

Regards,
Marco

> > 		alarm-gpios = <&gpio3 15 GPIO_ACTIVE_LOW
> > 				&gpio3 16 GPIO_ACTIVE_LOW>;
> > 	};
> > 	bat@1 {
> > 		reg = <1>;
> > 		label = "Battery Pack2 Voltage";
> > 		alarm-type = <GPIO_ALARM_LCRIT, GPIO_ALARM_CRIT>;
> > 		alarm-gpios = <&gpio3 9 GPIO_ACTIVE_LOW
> > 				&gpio3 1 GPIO_ACTIVE_LOW>;
> > 	};
> > 	cputemp@0 {
> > 		reg = <0>;
> > 		label = "CPU Temperature Critical";
> > 		type = "temperature";
> > 		alarm-type = <GPIO_ALARM_GENRIC>;
> > 		alarm-gpios = <&gpio4 17 GPIO_ACTIVE_LOW>;
> > 	};
> > };
> > 
> > Where a more structured layout don't have this "issue".
> > 
> > hwmon_dev {
> > 	compatible = "gpio-alarm";
> > 
> > 	voltage {
> > 		bat@0 {
> > 			reg = <0>;
> > 	 		label = "Battery Pack1 Voltage";
> > 			alarm-type = <GPIO_ALARM_LCRIT, GPIO_ALARM_CRIT>;
> > 			alarm-gpios = <&gpio3 15 GPIO_ACTIVE_LOW
> > 					&gpio3 16 GPIO_ACTIVE_LOW>;
> > 		};
> > 		bat@1 {
> > 			reg = <1>;
> > 	 		label = "Battery Pack2 Voltage";
> > 			alarm-type = <GPIO_ALARM_LCRIT, GPIO_ALARM_CRIT>;
> > 			alarm-gpios = <&gpio3 9 GPIO_ACTIVE_LOW
> > 					&gpio3 1 GPIO_ACTIVE_LOW>;
> > 		};
> > 	};
> > 	temperature {
> > 		cputemp {
> > 			label = "CPU Temperature Critical";
> > 			alarm-type = <GPIO_ALARM_GENRIC>;
> > 			alarm-gpios = <&gpio4 17 GPIO_ACTIVE_LOW>;
> > 		};
> > 	};
> > };
> > 
> > We don't have to take this layout, we can also consider about devices:
> > 
> > hwmon_dev {
> > 	compatible = "gpio-alarm";
> > 
> > 	dev@0 {
> > 		reg = <0>;
> > 		voltage {
> > 			label = "Battery Pack1 Voltage";
> > 			alarm-type = <GPIO_ALARM_LCRIT, GPIO_ALARM_CRIT>;
> > 			alarm-gpios = <&gpio3 15 GPIO_ACTIVE_LOW
> > 					&gpio3 16 GPIO_ACTIVE_LOW>;
> > 		};
> > 		temperature {
> > 			label = "Battery Pack1 Temperature Critical";
> > 			alarm-type = <GPIO_ALARM_GENRIC>;
> > 			alarm-gpios = <&gpio4 17 GPIO_ACTIVE_LOW>;
> > 		};
> > 	};
> > 	dev@1 {
> > 		reg = <1>;
> > 		temperature {
> > 			label = "CPU Temperature Critical";
> > 			alarm-type = <GPIO_ALARM_GENRIC>;
> > 			alarm-gpios = <&gpio4 19 GPIO_ACTIVE_LOW>;
> > 		};
> > 	};
> > };
> > 
> > I don't think that is a issue at all, but I don't know the dt
> > maintainers opinion of this theme.
> > 
> > Regards,
> > Marco

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

[Trent Piepho]

On Wed, 2018-11-07 at 10:35 +0100, Marco Felsch wrote:
> On 18-11-06 20:50, Trent Piepho wrote:
> > 
> > > Currently I using the gpiod_to_irq() function to convert the sense gpio
> > > into a irq, but I do some magic to determine the edge. I tought there
> > > might be reasons why there are no edge defines in
> > > include/dt-bindings/gpio/gpio.h.
> > 
> > Just request the interrupt with IRQF_TRIGGER_RISING and it will work on
> > almost any SoC.  The reason you see no edge defines with gpio handles
> > is that edge and level triggering is a interrupt concept, not a gpio
> > concept.  There are no level triggers defined for gpios either.  The
> > active low/high flags just define what voltage should be considered
> > "asserted".  They aren't intended to be related to interrupts.
> 
> Okay, thanks for this hint. So a gpio marked as GPIO_ACTIVE_LOW will
> trigger a IRQF_TRIGGER_RISING requested interrupt if the gpio level
> (electrical) goes from 1->0? I didn't knew that.

Yes and no.  Active low is a gpio concept and edge is an interrupt
concept and they don't know about each other.  So you'll find that
requesting a rising edge interrupt on the irq line associated with a
gpio via the kernel irq interface will give you an interrupt on the
GND->Vcc edge.  But then take a look at commit 0769746183c, 

        if (test_bit(FLAG_TRIG_FALL, &gpio_flags))
               irq_flags |= test_bit(FLAG_ACTIVE_LOW, &desc->flags) ?
                       IRQF_TRIGGER_RISING : IRQF_TRIGGER_FALLING;

If you use the sysfs interface to request an edge interrupt, since it
knows about both the gpio active low flag and the edge being requested,
it tries to combine them, so you get an interrupt on the Vcc->GND edge.

> 
> > 
> > > Okay, so no polling for the current solution. Let me summarize our
> > > solution:
> > >  - no polling (currently)
> > >  - dt-node specifies a gpio instead of a interrupt
> > >    -> gpio <-> irq mapping is done by gpiod_to_irq() and fails if gpio
> > >       doesn't support irq's
> > >  - more alarms per sensor
> > > 
> > > Only one last thing I tought about:
> > > 
> > > Using a flat design like you mentioned would lead into a "virtual"
> > > address conflict, since both sensors are on the same level. I tought
> > > about i2c/spi/muxes/graph-devices which don't support such "addressing"
> > > scheme.
> > 
> > You mean a temp alarm and a voltage alarm could both be reg = <1>?  I
> > don't think anything complains about that.  But it does seem
> > undesirable.
> 
> Yes, because both types are on the same hierarchy level. As I said it is
> more a dt-convention decision.
> 
> > > hwmon_dev {
> > > 	compatible = "gpio-alarm";
> > > 	bat@0 {
> > > 		reg = <0>;
> > > 		label = "Battery Pack1 Voltage";
> > > 		type = "voltage";
> > > 		alarm-type = <GPIO_ALARM_LCRIT, GPIO_ALARM_CRIT>;
> > 
> > Would have to be <GPIO_ALARM_LCRIT>, <GPIO_ALARM_CRIT>;
> > 
> > I'm not sure if dt bindings prefer symbolic integer constants vs
> > strings for something which is an enumeration like this.  strings seem
> > more common to me, e.g. alarm-types = "lcrit", "crit";
> 
> That's a good question. I term of parsing, the non string variant should
> be faster. I don't have any preference, but will try the string variant
> first ;)
> 
> Regards,
> Marco
> 
> > > 		alarm-gpios = <&gpio3 15 GPIO_ACTIVE_LOW
> > > 				&gpio3 16 GPIO_ACTIVE_LOW>;
> > > 	};
> > > 	bat@1 {
> > > 		reg = <1>;
> > > 		label = "Battery Pack2 Voltage";
> > > 		alarm-type = <GPIO_ALARM_LCRIT, GPIO_ALARM_CRIT>;
> > > 		alarm-gpios = <&gpio3 9 GPIO_ACTIVE_LOW
> > > 				&gpio3 1 GPIO_ACTIVE_LOW>;
> > > 	};
> > > 	cputemp@0 {
> > > 		reg = <0>;
> > > 		label = "CPU Temperature Critical";
> > > 		type = "temperature";
> > > 		alarm-type = <GPIO_ALARM_GENRIC>;
> > > 		alarm-gpios = <&gpio4 17 GPIO_ACTIVE_LOW>;
> > > 	};
> > > };
> > > 
> > > Where a more structured layout don't have this "issue".
> > > 
> > > hwmon_dev {
> > > 	compatible = "gpio-alarm";
> > > 
> > > 	voltage {
> > > 		bat@0 {
> > > 			reg = <0>;
> > > 	 		label = "Battery Pack1 Voltage";
> > > 			alarm-type = <GPIO_ALARM_LCRIT, GPIO_ALARM_CRIT>;
> > > 			alarm-gpios = <&gpio3 15 GPIO_ACTIVE_LOW
> > > 					&gpio3 16 GPIO_ACTIVE_LOW>;
> > > 		};
> > > 		bat@1 {
> > > 			reg = <1>;
> > > 	 		label = "Battery Pack2 Voltage";
> > > 			alarm-type = <GPIO_ALARM_LCRIT, GPIO_ALARM_CRIT>;
> > > 			alarm-gpios = <&gpio3 9 GPIO_ACTIVE_LOW
> > > 					&gpio3 1 GPIO_ACTIVE_LOW>;
> > > 		};
> > > 	};
> > > 	temperature {
> > > 		cputemp {
> > > 			label = "CPU Temperature Critical";
> > > 			alarm-type = <GPIO_ALARM_GENRIC>;
> > > 			alarm-gpios = <&gpio4 17 GPIO_ACTIVE_LOW>;
> > > 		};
> > > 	};
> > > };
> > > 
> > > We don't have to take this layout, we can also consider about devices:
> > > 
> > > hwmon_dev {
> > > 	compatible = "gpio-alarm";
> > > 
> > > 	dev@0 {
> > > 		reg = <0>;
> > > 		voltage {
> > > 			label = "Battery Pack1 Voltage";
> > > 			alarm-type = <GPIO_ALARM_LCRIT, GPIO_ALARM_CRIT>;
> > > 			alarm-gpios = <&gpio3 15 GPIO_ACTIVE_LOW
> > > 					&gpio3 16 GPIO_ACTIVE_LOW>;
> > > 		};
> > > 		temperature {
> > > 			label = "Battery Pack1 Temperature Critical";
> > > 			alarm-type = <GPIO_ALARM_GENRIC>;
> > > 			alarm-gpios = <&gpio4 17 GPIO_ACTIVE_LOW>;
> > > 		};
> > > 	};
> > > 	dev@1 {
> > > 		reg = <1>;
> > > 		temperature {
> > > 			label = "CPU Temperature Critical";
> > > 			alarm-type = <GPIO_ALARM_GENRIC>;
> > > 			alarm-gpios = <&gpio4 19 GPIO_ACTIVE_LOW>;
> > > 		};
> > > 	};
> > > };
> > > 
> > > I don't think that is a issue at all, but I don't know the dt
> > > maintainers opinion of this theme.
> > > 
> > > Regards,
> > > Marco