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

[Guenter Roeck]

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.

Thanks,
Guenter

> ---
> 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,
>  	},
>  };
>