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

[Guenter Roeck <linux@roeck-us.net>]

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.