On 11/1/18 3:40 AM, Marco Felsch wrote:
On 18-10-30 13:11, Guenter Roeck wrote:
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).
Thanks for your ideas, looks quite nice.
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 {
reg = <0>;
I remember that we have to add a reg property if we want to use xyz@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.
Should we use the @<reg> suffix to map it to in<reg>_*_alarm or should
we do something like that:
hwmon_dev {
compatible = "gpio-alarm";
voltage {
bat@0 {
reg = <0>;
label = "Battery Pack1 Voltage";
alarm-type = <GPIO_ALARM_LCRIT, GPIO_ALARM_CRIT>;
interrupt-parent = <&gpio3>;
interrupts = <15 IRQ_TYPE_LEVEL_LOW>,
<16 IRQ_TYPE_EDGE_RISING>;
};
bat@1 {
reg = <1>;
label = "Battery Pack2 Voltage";
alarm-type = <GPIO_ALARM_LCRIT, GPIO_ALARM_CRIT>;
interrupts-extended = <&gpio3 17 IRQ_TYPE_EDGE_FALLING>,
<&gpio4 18 IRQ_TYPE_EDGE_FALLING>;
};
};
temperature {
cputemp {
label = "CPU Temperature Critical";
alarm-type = <GPIO_ALARM_CRIT>;
interrupt-parent = <&gpio3>;
interrupts = <20 IRQ_TYPE_EDGE_FALLING>;
};
};
};
Works for me.
Now the subnodes imply the type. Since the hwmon-gpio-simple should
work interrupt driven all the time we should replace the alarm-gpios by
the interrupt property, so we can use the already existing EDGE
flags, as Trent mentoined. Otherwise we have to asume if
the gpio is low-active then the interrupt should be triggered on a
falling edge.
Isn't that configurable with devicetree flags ? I don't think a driver
should get involved in deciding the active edge.
Guenter
Marco
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.
