[PATCH] hwmon: max34451: Workaround for lost page

[Guenter Roeck]

On 4/2/25 01:33, William Kennington wrote:
> On Tue, Apr 1, 2025 at 5:19 PM Guenter Roeck <linux at roeck-us.net> wrote:
>>
>> On 4/1/25 15:55, William Kennington wrote:
>>> On Tue, Apr 1, 2025 at 3:52 PM Guenter Roeck <linux at roeck-us.net> wrote:
>>>>
>>>> On 4/1/25 15:08, William A. Kennington III wrote:
>>>>> When requesting new pages from the max34451 we sometimes see that the
>>>>> firmware doesn't update the page on the max34451 side fast enough. This
>>>>> results in the kernel receiving data for a different page than what it
>>>>> expects.
>>>>>
>>>>> To remedy this, the manufacturer recommends we wait 50-100us until
>>>>> the firmware should be ready with the new page.
>>>>>
>>>>> Signed-off-by: William A. Kennington III <william at wkennington.com>
>>>>> ---
>>>>>     drivers/hwmon/pmbus/max34440.c | 7 +++++++
>>>>>     1 file changed, 7 insertions(+)
>>>>>
>>>>> diff --git a/drivers/hwmon/pmbus/max34440.c b/drivers/hwmon/pmbus/max34440.c
>>>>> index c9dda33831ff..ac3a26f7cff3 100644
>>>>> --- a/drivers/hwmon/pmbus/max34440.c
>>>>> +++ b/drivers/hwmon/pmbus/max34440.c
>>>>> @@ -12,6 +12,7 @@
>>>>>     #include <linux/init.h>
>>>>>     #include <linux/err.h>
>>>>>     #include <linux/i2c.h>
>>>>> +#include <linux/delay.h>
>>>>>     #include "pmbus.h"
>>>>>
>>>>>     enum chips { max34440, max34441, max34446, max34451, max34460, max34461 };
>>>>> @@ -241,6 +242,12 @@ static int max34451_set_supported_funcs(struct i2c_client *client,
>>>>>                 if (rv < 0)
>>>>>                         return rv;
>>>>>
>>>>> +             /* Firmware is sometimes not ready if we try and read the
>>>>
>>>> This is not the networking subsystem. Standard multi-line comments, please.
>>>
>>> Okay, let me fix that.
>>>
>>>>
>>>>> +              * data from the page immediately after setting. Maxim
>>>>> +              * recommends 50-100us delay.
>>>>> +              */
>>>>> +             fsleep(50);
>>>>
>>>> I would suggest to wait 100uS to be safe. The function is only called during probe,
>>>> so that should be ok.
>>>
>>> Yeah, I don't think they did strenuous measurement of these values on
>>> their end. We have been using this patch for 4-5 years now with
>>> seemingly good robustness on the 50us value. I just pulled up an old
>>> email from the vendor that gives this context.
>>>
>>>>
>>>> Is this a generic problem with this chip when changing pages ?
>>>
>>> I believe that is the case, but this patch is pretty old at this
>>> point. Is there somewhere to add in quirks for such chips that would
>>> allow us to build in such a delay?
>>>
>>
>> So far we only have delays for all accesses and for write operations.
>> See access_delay and write_delay in struct pmbus_data. If the problem
>> only affects page changes, we might have to add page_change_delay or
>> something similar. Alternatively, maybe we could just set write_delay.
>> If the chip has trouble with page changes, it might well be that it has
>> trouble with write operations in general.
>>
> 
> So I did some digging and asked the original contributors to the
> patch. It would appear that it's specifically an issue with this IC
> around page switches and not any arbitrary write command. There is an
> issue where it does not correctly respond to the second command issued
> after a PAGE switch occurs, if the commands come in too quickly. They
> believe it's an issue with max34451 (and other derivative ICs) not
> correctly clock stretching while the PAGE command is processed.
> 
> Let me know what approach you would prefer to take here. It seems like
> it would be most optimal to have a quirk specifically to delay
> commands after a PAGE.
> 

I think we should add page_change_delay to struct pmbus_data, plus its handling
in the pmbus core.

Thanks,
Guenter