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

[Marco Felsch <m.felsch@pengutronix.de>]

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