Re: [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@xxxxxxxxxxxx> wrote:
> > On 4/1/25 15:55, William Kennington wrote:
> > > On Tue, Apr 1, 2025 at 3:52 PM Guenter Roeck <linux@xxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx>
> > > > > ---
> > > > >     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