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

[Guenter Roeck <linux@roeck-us.net>]

On 10/30/18 3:47 AM, Marco Felsch wrote:
> Hi Guenter,
> 
> thanks for the quick response, please see my comments below.
> 
> On 18-10-29 18:12, Guenter Roeck wrote:
>> 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.
> 
> Yes, there are "good old" hwmon chips without any management interface,
> e.g. LTC694-3.3, LTC7862, ... also there might be a discrete hw circuit.
> I think it's better to handle those "simple" chips by a generic hwmon
> driver. By simple I mean chips which informs the host only by toggling a
> gpio to report a state. For this purpose my driver (including name
> convention) isn't generic enough, I think about a hwmon-simple device.
> Such a device can support reporting states, no voltage/power/temp values.
> 

Yes, I agree. It doesn't really make sense to keep adding single-attribute drivers.
hwmon-gpio-simple, maybe, to indicate that it gets its data from gpio ?
The most difficult part of such a driver would probably be to define acceptable
devicetree properties.

>>> 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.
> 
> Please see my above comments.
> 
>>> 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.
> 
> You're right thats important for my use-case, but I got you, thats not a
> hwmon related problem. If I understood the device-model correctly there
> are absolut no problems to hot-unplug devices, this is always the case
> if we unbind a driver from user-space. So why we can't handle it
> directly in the kernel. Are there any concerns? If not can you give me
> some hints to get the logic at thr right place.
> 
Well, the easiest solution would have been to do nothing code-wise and
register a gpio-keys instance on the gpio pin to create a KEY_POWER event.
Of course that might be considered an abuse of the input subsystem,
as Dmitry has pointed out, and is probably not acceptable. Now, if you
want that functionality, you could probably write some udev rules and
accomplish the same by listening for a change event from a sysfs attribute
supporting it (I think KEY_POWER is not really handled in the kernel,
but I am not entirely sure).

Handling the event in the kernel is more tricky. First, I don't think the
selective device removal as you have suggested would be acceptable anywhere;
it may cause all kinds of trouble. The thermal subsystem supports the
functionality to shut down the kernel in emergencies, but is limited
in its scope (or at least I think so) to thermal events. Anything else
would have to be discussed. I for my part prefer handling it from userspace.

Guenter