| Message ID | 20181029143521.22122-3-m.felsch@pengutronix.de (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
| Series | Add GPIO brownout detection support | expand |
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 >
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.
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
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 >
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
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
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?
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.
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.
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
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.
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?
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
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.
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. >
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. >
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. > > > >
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. > > > >
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
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. > > > > > > >
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.
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.
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
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
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.
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
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
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
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
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");
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