RE: [PATCH v2] hwmon: max34451: Work around lost page

[Torreno, Alexis Czezar]

> -----Original Message-----
> From: Guenter Roeck <groeck7@xxxxxxxxx> On Behalf Of Guenter Roeck
> Sent: Tuesday, April 8, 2025 10:06 AM
> To: William A. Kennington III <william@xxxxxxxxxxxxxxx>
> Cc: Jean Delvare <jdelvare@xxxxxxxx>; linux-hwmon@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; Torreno, Alexis Czezar
> <AlexisCzezar.Torreno@xxxxxxxxxx>
> Subject: Re: [PATCH v2] hwmon: max34451: Work around lost page
> 
> [External]
> 
> On Mon, Apr 07, 2025 at 06:10:06PM -0700, William A. Kennington III wrote:
> > When requesting new pages from the max34451 we sometimes see that the
> > firmware responds with stale or bad data to reads that happen
> > immediately after a page change. This is due to a lack of clock
> > stretching after page changing on the device side when it needs more
> > time to complete the operation.
> >
> > To remedy this, the manufacturer recommends we wait 50us until the
> > firmware should be ready with the new page.
> >
> > Signed-off-by: William A. Kennington III <william@xxxxxxxxxxxxxxx>
> 
> Applied to hwmon-next.
> 
> Alexis: Do you happen to know if the new revision of MAX34451 still has this
> problem ? Also, it would be nice to get some feedback if the patch covers all
> affected devices.
> 

I unfortunately am not aware of this. Let me see if can get an answer from the
engr of the part.

> 
> > ---
> > V1 -> V2: Make all page changes delay the required 50us
> >
> >  drivers/hwmon/pmbus/max34440.c | 16 ++++++++++++++++
> >  1 file changed, 16 insertions(+)
> >
> > diff --git a/drivers/hwmon/pmbus/max34440.c
> > b/drivers/hwmon/pmbus/max34440.c index c9dda33831ff..0d9cb39a9cc6
> > 100644
> > --- a/drivers/hwmon/pmbus/max34440.c
> > +++ b/drivers/hwmon/pmbus/max34440.c
> > @@ -12,10 +12,19 @@
> >  #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 };
> >
> > +/*
> > + * Firmware is sometimes not ready if we try and read the
> > + * data from the page immediately after setting. Maxim
> > + * recommends 50us delay due to the chip failing to clock
> > + * stretch long enough here.
> > + */
> > +#define MAX34440_PAGE_CHANGE_DELAY 50
> > +
> >  #define MAX34440_MFR_VOUT_PEAK		0xd4
> >  #define MAX34440_MFR_IOUT_PEAK		0xd5
> >  #define MAX34440_MFR_TEMPERATURE_PEAK	0xd6
> > @@ -238,6 +247,7 @@ static int max34451_set_supported_funcs(struct
> > i2c_client *client,
> >
> >  	for (page = 0; page < 16; page++) {
> >  		rv = i2c_smbus_write_byte_data(client, PMBUS_PAGE, page);
> > +		fsleep(MAX34440_PAGE_CHANGE_DELAY);
> >  		if (rv < 0)
> >  			return rv;
> >
> > @@ -312,6 +322,7 @@ static struct pmbus_driver_info max34440_info[] = {
> >  		.read_byte_data = max34440_read_byte_data,
> >  		.read_word_data = max34440_read_word_data,
> >  		.write_word_data = max34440_write_word_data,
> > +		.page_change_delay = MAX34440_PAGE_CHANGE_DELAY,
> >  	},
> >  	[max34441] = {
> >  		.pages = 12,
> > @@ -355,6 +366,7 @@ static struct pmbus_driver_info max34440_info[] = {
> >  		.read_byte_data = max34440_read_byte_data,
> >  		.read_word_data = max34440_read_word_data,
> >  		.write_word_data = max34440_write_word_data,
> > +		.page_change_delay = MAX34440_PAGE_CHANGE_DELAY,
> >  	},
> >  	[max34446] = {
> >  		.pages = 7,
> > @@ -392,6 +404,7 @@ static struct pmbus_driver_info max34440_info[] = {
> >  		.read_byte_data = max34440_read_byte_data,
> >  		.read_word_data = max34440_read_word_data,
> >  		.write_word_data = max34440_write_word_data,
> > +		.page_change_delay = MAX34440_PAGE_CHANGE_DELAY,
> >  	},
> >  	[max34451] = {
> >  		.pages = 21,
> > @@ -415,6 +428,7 @@ static struct pmbus_driver_info max34440_info[] = {
> >  		.func[20] = PMBUS_HAVE_TEMP |
> PMBUS_HAVE_STATUS_TEMP,
> >  		.read_word_data = max34440_read_word_data,
> >  		.write_word_data = max34440_write_word_data,
> > +		.page_change_delay = MAX34440_PAGE_CHANGE_DELAY,
> >  	},
> >  	[max34460] = {
> >  		.pages = 18,
> > @@ -445,6 +459,7 @@ static struct pmbus_driver_info max34440_info[] = {
> >  		.func[17] = PMBUS_HAVE_TEMP |
> PMBUS_HAVE_STATUS_TEMP,
> >  		.read_word_data = max34440_read_word_data,
> >  		.write_word_data = max34440_write_word_data,
> > +		.page_change_delay = MAX34440_PAGE_CHANGE_DELAY,
> >  	},
> >  	[max34461] = {
> >  		.pages = 23,
> > @@ -480,6 +495,7 @@ static struct pmbus_driver_info max34440_info[] = {
> >  		.func[21] = PMBUS_HAVE_TEMP |
> PMBUS_HAVE_STATUS_TEMP,
> >  		.read_word_data = max34440_read_word_data,
> >  		.write_word_data = max34440_write_word_data,
> > +		.page_change_delay = MAX34440_PAGE_CHANGE_DELAY,
> >  	},
> >  };
> >