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

[Trent Piepho <tpiepho@impinj.com>]

On Thu, 2018-11-01 at 11:40 +0100, 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:
> > > 
> > 	voltage@0 {
> 
> 		reg = <0>;
> 
> I remember that we have to add a reg property if we want to use xyz@0.

Kernel disables that dtc warning, but still seems good to follow that
rule.

> > 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:

Usually the node name, e.g. "node@0", isn't used at all by the driver. 
It's arbitrary, so the dt author can name it what they want.

I've used to identify a device in a udev rule.  So I can have a rule
that acts on a device based on what I want the device to do, rather
than what bus and address the device happens to be on.  The latter are
subject to change based on other devices, SoC pinmuxing, board routing,
etc.

Using the "reg" property to identify the input/temp/fan number seems
totally appropriate to me.

> 
> hwmon_dev {
> 	compatible = "gpio-alarm";
> 
> 	voltage {

I think you'd need something like type = "voltage" here instead of
using the node name.

> 		bat@0 {
> 			reg = <0>;
> 	 		label = "Battery Pack1 Voltage";
> 		};
> 
> 		bat@1 {
> 			reg = <1>;
> 	 		label = "Battery Pack2 Voltage";
> 		};
> 	};

Seems better to me to have the flatter tree with each alarm having a
"type" attribute rather than grouping them by type.

Usually the tree structure of a DT is meant to show a "contains"
relationship, one present in the actual hardware.  A bus with devices
behind it.  Lines attached to controller.  If I've got three op-amps on
GPIOs, two measuring voltage rails and the third on a thermistor, then
IMHO all three are peers.  The hardware design doesn't group the two
voltage rail op-amps in some way that excludes the thermistor op-amp.


> 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

Not all GPIOs can generate interrupts.  It would be an unfortunate
hardware design to use such a GPIO for this.  Seems ok to me to defer
that problem to the poor sod who needs to support such hardware if it
ever exists.