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

[Trent Piepho <tpiepho@impinj.com>]

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.