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

[Trent Piepho <tpiepho@impinj.com>]

On Mon, 2018-10-29 at 12:52 -0700, Guenter Roeck wrote:
> On Mon, Oct 29, 2018 at 03:35:21PM +0100, Marco Felsch wrote:
> > Most the time low voltage detection happens on a external device
> > e.g. a pmic or any other hw-logic. Some of such devices can pass the
> > power state (good/bad) to the host via i2c or by toggling a gpio.
> > 
> > This patch adds the support to evaluate a gpio line to determine
> > the power good/bad state. The state is represented by the
> > in0_lcrit_alarm. Furthermore the driver supports to release device from
> > their driver upon a low voltage detection. This feature is supported by
> > OF-based firmware only.
> > 
> 
> NACK, sorry.
> 
> hwmon is strictly about monitoring, not about taking any action, and much
> less about removing devices from the system after some status change,
> it be a gpio pin value or anything else. Other than that, the driver simply
> maps a gpio pin to a (pretty much arbitrary) sysfs attribute, for which
> a driver isn't really needed.
> 
> I don't know if there is a space in the kernel for handling the problem
> you are trying to solve, but it isn't hwmon.

If we ignore the ability to stop other devices, how is this not a hwmon
device with just alarm features?

It seems to map the hwmon interface quite directly.

Consider, what if we had a "classic" hwmon chip, but on this board the
I2C/LPC/SPI interface was not connected as an appropriate master was
not available, and the default configuration of the chip was
acceptable.  The chip's alarm outputs are connected to GPIOs, as it
normal for a hwmon chip with alarm outputs.

Are the alarms no longer appropriate to appear in hwmon?  But if we
connect the chip's I2C interface, then those same alarms should appear
in hwmon?

That just doesn't make sense to me.

Also consider the gpio-fan driver.  That's pretty much just an
interface to a gpio too.

Or consider the leds-gpio driver.  That allows a gpio controlled LED to
appear in the kernel's led subsystem.  It doesn't do anything besides
turn the gpio on and off.  It's hardly needed if all we cared about was
controlling the LED in some way.  Yet it's used quite often.

So it seems perfectly correct to me that a GPIO based hardware
monitoring alarm should appear in the kernel's hardware monitoring
subsystem with all the other hardware monitoring alarms.

The ability of the hwmon driver to cause certain other devices to be
removed is a different question.