On 10/29/18 2:16 PM, Trent Piepho wrote:
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?
Possibly, but the ability to stop other devices is at the core of the driver
as submitted.
It seems to map the hwmon interface quite directly.
Agreed.
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.
"If we had" is theory. Do we ? We don't usually add code to the kernel
just in case the hardware it supports might be out there.
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.
The difference here is that those are distinct drivers, with no other hardware
behind it to control the LEDs or to report fan speeds.
For voltage monitoring, that is not normally the case. It is much more likely
that there is in fact a hardware monitoring or power control chip, the
(or an) alarm output of that chip is connected to a gpio line, and its control
interface is connected. If so, the driver for that chip should be enhanced
to support interrupts, and to report the status with its own sysfs attributes.
Agreed, that might be more work for a given hardware, and it would have to
be repeated for each chip with that configuration which doesn't already
have interrupt support. Meaning, unfortunately, almost all hwmon drivers.
It might even be necessary to implement an i2c or spi controller driver
if that does not exist. But it would be the appropriate solution.
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.
We can only consider a driver reporting a specific attribute if there
is a board which doesn't support anything else.
The ability of the hwmon driver to cause certain other devices to be
removed is a different question.
I disagree. That functionality is essential to the driver as submitted.
Guenter
