Re: [PATCH] hwmon: (pmbus) Clear pmbus fault/warning bits before read

[Vikash Chandola <vikash.chandola@linux.intel.com>]

On 2/10/2022 10:55 PM, Guenter Roeck wrote:
> On 2/10/22 07:57, Vikash Chandola wrote:
>>
>>
>> On 2/10/2022 8:14 PM, Guenter Roeck wrote:
>>> On 2/10/22 04:41, Vikash Chandola wrote:
>>>> pmbus fault and warning bits are not cleared by itself once 
>>>> fault/warning
>>>> condition is not valid anymore. As per pmbus datasheet faults must be
>>>> cleared by user.
>>>> Modify hwmon behavior to clear latched status bytes if any bit in 
>>>> status
>>>> register is high prior to returning fresh data to userspace. If
>>>> fault/warning conditions are still applicable fault/warning bits 
>>>> will be
>>>> set and we will get updated data in second read.
>>>>
>>>> Hwmon behavior is changed here. Now sysfs reads will reflect latest
>>>> values from pmbus slave, not latched values.
>>>> In case a transient warning/fault has happened in the past, it will no
>>>> longer be reported to userspace.
>>>>
>>>
>>>
>>> NACK.
>>>
>>> Reporting that information is exactly the point of the current code.
>>> We _do_ want to report at least once that a problem occurred in the 
>>> past,
>>> and only clear the warning flag(s) afterwards.
>>>
>>> Guenter
>>>
>> But as per current implementation we will continue to report fault 
>> even after fault condition is cleared. I could not find sysfs entry or 
>> any other means by which user/kernel can clear the faults/warnings 
>> after it is reported. Please point if I am missing anything.
>>
> 
> Normally a chip should clear the status after the fault condition cleared
> and the register was read. At least that was my experience so far.
> What chip (or chips) don't do that ?
> 
> Guenter

I see that pmbus spec says that once fault/warning bits are set
they won't be cleared by itself. Section 10.2.3 of
"PMBus Specification Rev. 1.3.1 Part II 2015-03-13" from
https://pmbus.org/specification-archives/   says

"
Almost all of the warning or fault bits set in the status registers 
remain set, even if the fault or warning condition is removed
or corrected, until one of the following occur:
* The bit is individually cleared (Section 10.2.4),
* The device receives a CLEAR_FAULTS command (Section 15.1),
* A RESET signal (if one exists) is asserted,
* The output is commanded through the CONTROL pin, the OPERATION 
command, or the combined action of the CONTROL pin and OPERATION
command, to turn off and then to turn back on, or
* Bias power is removed from the PMBus device...
"

 From this I concluded that slave(PSU) following pmbus spec will not
clear the fault/warning in status registers.
I don't have exact chip details handy but I can get it by tomorrow.
However this looks to be issue not related to specific chip ?

If this is a problem, how should I approach this ? Shall I create a new
sysfs entry that user space application can invoke and clear all faults
or provide sysfs entry to clear individual fault/warning bits or
something else ?

> 
>>>> Signed-off-by: Vikash Chandola <vikash.chandola@linux.intel.com>
>>>> ---
>>>>   drivers/hwmon/pmbus/pmbus_core.c | 9 +++++++++
>>>>   1 file changed, 9 insertions(+)
>>>>
>>>> diff --git a/drivers/hwmon/pmbus/pmbus_core.c 
>>>> b/drivers/hwmon/pmbus/pmbus_core.c
>>>> index 776ee2237be2..1cc82d644079 100644
>>>> --- a/drivers/hwmon/pmbus/pmbus_core.c
>>>> +++ b/drivers/hwmon/pmbus/pmbus_core.c
>>>> @@ -577,6 +577,15 @@ static int pmbus_get_status(struct i2c_client 
>>>> *client, int page, int reg)
>>>>           break;
>>>>       default:
>>>>           status = _pmbus_read_byte_data(client, page, reg);
>>>> +        if (status > 0) {
>>>> +            /*
>>>> +             * Status greater than 0 could mean that there was a 
>>>> fault/warning.
>>>> +             * Clear faults and do a second read to make sure we 
>>>> are not getting
>>>> +             * stale values.
>>>> +             */
>>>> +            pmbus_clear_fault_page(client, page);
>>>> +            status = _pmbus_read_byte_data(client, page, reg);
>>>> +        }
>>>>           break;
>>>>       }
>>>>       if (status < 0)
>>>
> 

-- 
Vikash