On Thu, Nov 01, 2018 at 03:58:58PM +0100, Marco Felsch wrote:
> On 18-11-01 06:02, Guenter Roeck wrote:
> > 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>;
> > > };
> > > };
> > > };
> > >
> > > 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
> >
> > Note that this isn't entirely correct: If the gpio pin doesn't support
> > interrupts, the driver would just report the state of the pin.
>
> Okay, but how do we detect and report a alarm if you won't have a
> (fallback) polling mechanism nor a gpio which doesn't support
> interrupts? Should the user poll the sysfs-entry instead?
>
Yes. Not different to existing hwmon drivers which don't implement
interrupt handling.
Guenter
