[PATCH] hwmon: max34451: Workaround for lost page

[PATCH] hwmon: max34451: Workaround for lost page

[William A. Kennington III]

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@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
+		 * data from the page immediately after setting. Maxim
+		 * recommends 50-100us delay.
+		 */
+		fsleep(50);
+
 		rv = i2c_smbus_read_word_data(client,
 					      MAX34451_MFR_CHANNEL_CONFIG);
 		if (rv < 0)
-- 
2.49.0.472.ge94155a9ec-goog

Re: [PATCH] hwmon: max34451: Workaround for lost page

[Guenter Roeck]

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@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.

> +		 * 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.

Is this a generic problem with this chip when changing pages ?

Thanks,
Guenter

Re: [PATCH] hwmon: max34451: Workaround for lost page

[William Kennington]

On Tue, Apr 1, 2025 at 3:52 PM Guenter Roeck <linux@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@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?

>
> Thanks,
> Guenter
>

Re: [PATCH] hwmon: max34451: Workaround for lost page

[Guenter Roeck]

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

Thanks,
Guenter

Re: [PATCH] hwmon: max34451: Workaround for lost page

[William Kennington]

On Tue, Apr 1, 2025 at 5:19 PM Guenter Roeck <linux@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@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@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.

> Thanks,
> Guenter
>

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

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

[William A. Kennington III]

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@wkennington.com>
---
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,
 	},
 };
 
-- 
2.49.0.504.g3bcea36a83-goog

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@wkennington.com>

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

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

[Torreno, Alexis Czezar]

> -----Original Message-----
> From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter Roeck
> Sent: Tuesday, April 8, 2025 10:06 AM
> To: William A. Kennington III <william@wkennington.com>
> Cc: Jean Delvare <jdelvare@suse.com>; linux-hwmon@vger.kernel.org; linux-
> kernel@vger.kernel.org; Torreno, Alexis Czezar
> <AlexisCzezar.Torreno@analog.com>
> 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@wkennington.com>
> 
> 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,
> >  	},
> >  };
> >

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

[Torreno, Alexis Czezar]

> > [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@wkennington.com>
> >
> > 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.

To follow up, here's the exact response I got.

- This is not an issue with the part. As the fix mentions, "the manufacturer
recommends we wait 50us until the firmware should be ready with the new page".
Compliant with I2C/SMBus spec, some parts require a longer idle time for the
bus to be free before receiving the next command, in this situation it is normal to
recommend a longer idle time in between commands. It's more related to the
part's inner timing and architecture.

So I guess no specific changes related to this was made for later versions so the 
patch here should cover all.

Thanks,

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

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

[Guenter Roeck]

On 4/10/25 16:59, Torreno, Alexis Czezar wrote:
>>> [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@wkennington.com>
>>>
>>> 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.
> 
> To follow up, here's the exact response I got.
> 
> - This is not an issue with the part. As the fix mentions, "the manufacturer
> recommends we wait 50us until the firmware should be ready with the new page".
> Compliant with I2C/SMBus spec, some parts require a longer idle time for the
> bus to be free before receiving the next command, in this situation it is normal to
> recommend a longer idle time in between commands. It's more related to the
> part's inner timing and architecture.
> 
> So I guess no specific changes related to this was made for later versions so the
> patch here should cover all.
> 

Ok. Thanks a lot for looking into this!

Guenter