| Message ID | 20200827133002.369439-3-andrew@aj.id.au |
|---|---|
| State | New |
| Headers | show |
| Series | MAX31785 Fan Controller Work-arounds | expand |
On Thu, 27 Aug 2020 at 13:30, Andrew Jeffery <andrew@aj.id.au> wrote: > > Extensive testing and tracing has shown that the MAX31785 is unreliable > in the face of PAGE write commands, ACK'ing the PAGE request but > reporting a value of 0 on some subsequent PAGE reads. The trace data > suggests that a one-shot retry of the PAGE write is enough to get the > requested value to stick. > > As we configure the device before registering with the PMBus core, > centralise PAGE handling inside the driver and implement the one-shot > retry semantics there. Are you still doing a retry with every operation due to the macro magic? > > Signed-off-by: Andrew Jeffery <andrew@aj.id.au> > --- > drivers/hwmon/pmbus/max31785.c | 32 ++++++++++++++++++++++++++------ > 1 file changed, 26 insertions(+), 6 deletions(-) > > diff --git a/drivers/hwmon/pmbus/max31785.c b/drivers/hwmon/pmbus/max31785.c > index 88b7156d777e..a392c0efe0a6 100644 > --- a/drivers/hwmon/pmbus/max31785.c > +++ b/drivers/hwmon/pmbus/max31785.c > @@ -361,6 +361,27 @@ static int max31785_write_word_data(struct i2c_client *client, int page, > return -ENXIO; > } > > +static int max31785_pmbus_set_page(struct i2c_client *client, int page) > +{ > + int ret; > + int i; > + > + for (i = 0; i < 2; i++) { > + ret = max31785_i2c_smbus_write_byte_data(client, PMBUS_PAGE, page); > + if (ret < 0) > + return ret; > + > + ret = max31785_i2c_smbus_read_byte_data(client, PMBUS_PAGE); > + if (ret < 0) > + return ret; > + > + if (ret == page) > + return 0; > + } > + > + return -EIO; > +} > + > /* > * Returns negative error codes if an unrecoverable problem is detected, 0 if a > * recoverable problem is detected, or a positive value on success. > @@ -392,7 +413,7 @@ static int max31785_of_fan_config(struct i2c_client *client, > return -ENXIO; > } > > - ret = max31785_i2c_smbus_write_byte_data(client, PMBUS_PAGE, page); > + ret = max31785_pmbus_set_page(client, page); > if (ret < 0) > return ret; > > @@ -613,7 +634,7 @@ static int max31785_of_tmp_config(struct i2c_client *client, > return -ENXIO; > } > > - ret = max31785_i2c_smbus_write_byte_data(client, PMBUS_PAGE, page); > + ret = max31785_pmbus_set_page(client, page); > if (ret < 0) > return ret; > > @@ -714,7 +735,7 @@ static int max31785_configure_dual_tach(struct i2c_client *client, > int i; > > for (i = 0; i < MAX31785_NR_FAN_PAGES; i++) { > - ret = max31785_i2c_smbus_write_byte_data(client, PMBUS_PAGE, i); > + ret = max31785_pmbus_set_page(client, i); > if (ret < 0) > return ret; > > @@ -756,7 +777,7 @@ static int max31785_probe(struct i2c_client *client, > > *info = max31785_info; > > - ret = max31785_i2c_smbus_write_byte_data(client, PMBUS_PAGE, 255); > + ret = max31785_pmbus_set_page(client, 255); > if (ret < 0) > return ret; > > @@ -798,8 +819,7 @@ static int max31785_probe(struct i2c_client *client, > if (!have_fan || fan_configured) > continue; > > - ret = max31785_i2c_smbus_write_byte_data(client, PMBUS_PAGE, > - i); > + ret = max31785_pmbus_set_page(client, i); > if (ret < 0) > return ret; > > -- > 2.25.1 >
diff --git a/drivers/hwmon/pmbus/max31785.c b/drivers/hwmon/pmbus/max31785.c index 88b7156d777e..a392c0efe0a6 100644 --- a/drivers/hwmon/pmbus/max31785.c +++ b/drivers/hwmon/pmbus/max31785.c @@ -361,6 +361,27 @@ static int max31785_write_word_data(struct i2c_client *client, int page, return -ENXIO; } +static int max31785_pmbus_set_page(struct i2c_client *client, int page) +{ + int ret; + int i; + + for (i = 0; i < 2; i++) { + ret = max31785_i2c_smbus_write_byte_data(client, PMBUS_PAGE, page); + if (ret < 0) + return ret; + + ret = max31785_i2c_smbus_read_byte_data(client, PMBUS_PAGE); + if (ret < 0) + return ret; + + if (ret == page) + return 0; + } + + return -EIO; +} + /* * Returns negative error codes if an unrecoverable problem is detected, 0 if a * recoverable problem is detected, or a positive value on success. @@ -392,7 +413,7 @@ static int max31785_of_fan_config(struct i2c_client *client, return -ENXIO; } - ret = max31785_i2c_smbus_write_byte_data(client, PMBUS_PAGE, page); + ret = max31785_pmbus_set_page(client, page); if (ret < 0) return ret; @@ -613,7 +634,7 @@ static int max31785_of_tmp_config(struct i2c_client *client, return -ENXIO; } - ret = max31785_i2c_smbus_write_byte_data(client, PMBUS_PAGE, page); + ret = max31785_pmbus_set_page(client, page); if (ret < 0) return ret; @@ -714,7 +735,7 @@ static int max31785_configure_dual_tach(struct i2c_client *client, int i; for (i = 0; i < MAX31785_NR_FAN_PAGES; i++) { - ret = max31785_i2c_smbus_write_byte_data(client, PMBUS_PAGE, i); + ret = max31785_pmbus_set_page(client, i); if (ret < 0) return ret; @@ -756,7 +777,7 @@ static int max31785_probe(struct i2c_client *client, *info = max31785_info; - ret = max31785_i2c_smbus_write_byte_data(client, PMBUS_PAGE, 255); + ret = max31785_pmbus_set_page(client, 255); if (ret < 0) return ret; @@ -798,8 +819,7 @@ static int max31785_probe(struct i2c_client *client, if (!have_fan || fan_configured) continue; - ret = max31785_i2c_smbus_write_byte_data(client, PMBUS_PAGE, - i); + ret = max31785_pmbus_set_page(client, i); if (ret < 0) return ret;
Extensive testing and tracing has shown that the MAX31785 is unreliable in the face of PAGE write commands, ACK'ing the PAGE request but reporting a value of 0 on some subsequent PAGE reads. The trace data suggests that a one-shot retry of the PAGE write is enough to get the requested value to stick. As we configure the device before registering with the PMBus core, centralise PAGE handling inside the driver and implement the one-shot retry semantics there. Signed-off-by: Andrew Jeffery <andrew@aj.id.au> --- drivers/hwmon/pmbus/max31785.c | 32 ++++++++++++++++++++++++++------ 1 file changed, 26 insertions(+), 6 deletions(-)