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
