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

Marco Felsch
2018-11-02T15:54:42Z

On 18-11-01 17:41, Trent Piepho wrote:
> 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.

Yes, but the maintainers won't confirm it without the reg property.

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

Thats true, but we have to use that convention to conform the DT if we
want to use the reg property.

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

Okay.

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

I'm open minded. Guenter what's your prefered solution?

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

Please look at the other mails. It's important for Guenter to support
the non-interrupt case too. It shouldn't be a big deal to implement both
cases.

Marco