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

Marco Felsch
2018-11-02T06:38:03Z

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