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.