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

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

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

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