Changes since V1:
1. Changed the max31785_wait macro to a function, following the conventions
used in other drivers that had the same issue.
2. Changed the function names from max31785_i2c_smbus* to max31785_i2c_* and
from max31785_pmbus_* to _max31785_*, making them more concise.
The MAX31785 has shown erratic behaviour across multiple system
designs, unexpectedly clock stretching and NAKing transactions.
Experimentation shows that this seems to be triggered by a register access
directly back to back with a previous register write. Experimentation also
shows that inserting a small delay after register writes makes the issue go
away.
Use a similar solution to what the max15301 driver does to solve the same
problem. Create a custom set of bus read and write functions that make sure
that the delay is added.
Signed-off-by: Lakshmi Yadlapati <[email protected]>
---
drivers/hwmon/pmbus/max31785.c | 181 +++++++++++++++++++++++++++++----
1 file changed, 160 insertions(+), 21 deletions(-)
diff --git a/drivers/hwmon/pmbus/max31785.c b/drivers/hwmon/pmbus/max31785.c
index f9aa576495a5..0e4f9bec542d 100644
--- a/drivers/hwmon/pmbus/max31785.c
+++ b/drivers/hwmon/pmbus/max31785.c
@@ -3,6 +3,7 @@
* Copyright (C) 2017 IBM Corp.
*/
+#include <linux/delay.h>
#include <linux/kernel.h>
#include <linux/module.h>
#include <linux/init.h>
@@ -23,19 +24,112 @@ enum max31785_regs {
#define MAX31785_NR_PAGES 23
#define MAX31785_NR_FAN_PAGES 6
+#define MAX31785_WAIT_DELAY_US 250
-static int max31785_read_byte_data(struct i2c_client *client, int page,
- int reg)
+struct max31785_data {
+ ktime_t access; /* Chip access time */
+ struct pmbus_driver_info info;
+};
+
+#define to_max31785_data(x) container_of(x, struct max31785_data, info)
+
+/*
+ * MAX31785 Driver Workaround
+ *
+ * The MAX31785 fan controller occasionally exhibits communication issues.
+ * These issues are not indicated by the device itself, except for occasional
+ * NACK responses during master transactions. No error bits are set in STATUS_BYTE.
+ *
+ * To address this, we introduce a delay of 250us between consecutive accesses
+ * to the fan controller. This delay helps mitigate communication problems by
+ * allowing sufficient time between accesses.
+ */
+static inline void max31785_wait(const struct max31785_data *data)
{
- if (page < MAX31785_NR_PAGES)
- return -ENODATA;
+ s64 delta = ktime_us_delta(ktime_get(), data->access);
+
+ if (delta < MAX31785_WAIT_DELAY_US)
+ udelay(MAX31785_WAIT_DELAY_US - delta);
+}
+
+static int max31785_i2c_write_byte_data(struct i2c_client *client,
+ struct max31785_data *driver_data,
+ int command, u16 data)
+{
+ int rc;
+ max31785_wait(driver_data);
+ rc = i2c_smbus_write_byte_data(client, command, data);
+ driver_data->access = ktime_get();
+ return rc;
+}
+
+static int max31785_i2c_read_word_data(struct i2c_client *client,
+ struct max31785_data *driver_data,
+ int command)
+{
+ int rc;
+ max31785_wait(driver_data);
+ rc = i2c_smbus_read_word_data(client, command);
+ driver_data->access = ktime_get();
+ return rc;
+}
+
+static int _max31785_read_byte_data(struct i2c_client *client,
+ struct max31785_data *driver_data,
+ int page, int command)
+{
+ int rc;
+ max31785_wait(driver_data);
+ rc = pmbus_read_byte_data(client, page, command);
+ driver_data->access = ktime_get();
+ return rc;
+}
+
+static int _max31785_write_byte_data(struct i2c_client *client,
+ struct max31785_data *driver_data,
+ int page, int command, u16 data)
+{
+ int rc;
+ max31785_wait(driver_data);
+ rc = pmbus_write_byte_data(client, page, command, data);
+ driver_data->access = ktime_get();
+ return rc;
+}
+
+static int _max31785_read_word_data(struct i2c_client *client,
+ struct max31785_data *driver_data,
+ int page, int phase, int command)
+{
+ int rc;
+ max31785_wait(driver_data);
+ rc = pmbus_read_word_data(client, page, phase, command);
+ driver_data->access = ktime_get();
+ return rc;
+}
+
+static int _max31785_write_word_data(struct i2c_client *client,
+ struct max31785_data *driver_data,
+ int page, int command, u16 data)
+{
+ int rc;
+ max31785_wait(driver_data);
+ rc = pmbus_write_word_data(client, page, command, data);
+ driver_data->access = ktime_get();
+ return rc;
+}
+
+static int max31785_read_byte_data(struct i2c_client *client, int page, int reg)
+{
+ const struct pmbus_driver_info *info = pmbus_get_driver_info(client);
+ struct max31785_data *driver_data = to_max31785_data(info);
switch (reg) {
case PMBUS_VOUT_MODE:
return -ENOTSUPP;
case PMBUS_FAN_CONFIG_12:
- return pmbus_read_byte_data(client, page - MAX31785_NR_PAGES,
- reg);
+ return _max31785_read_byte_data(client, driver_data,
+ page - MAX31785_NR_PAGES,
+ reg);
}
return -ENODATA;
@@ -102,16 +196,19 @@ static int max31785_get_pwm(struct i2c_client *client, int page)
return rv;
}
-static int max31785_get_pwm_mode(struct i2c_client *client, int page)
+static int max31785_get_pwm_mode(struct i2c_client *client,
+ struct max31785_data *driver_data, int page)
{
int config;
int command;
- config = pmbus_read_byte_data(client, page, PMBUS_FAN_CONFIG_12);
+ config = _max31785_read_byte_data(client, driver_data, page,
+ PMBUS_FAN_CONFIG_12);
if (config < 0)
return config;
- command = pmbus_read_word_data(client, page, 0xff, PMBUS_FAN_COMMAND_1);
+ command = _max31785_read_word_data(client, driver_data, page, 0xff,
+ PMBUS_FAN_COMMAND_1);
if (command < 0)
return command;
@@ -129,6 +226,8 @@ static int max31785_get_pwm_mode(struct i2c_client *client, int page)
static int max31785_read_word_data(struct i2c_client *client, int page,
int phase, int reg)
{
+ const struct pmbus_driver_info *info = pmbus_get_driver_info(client);
+ struct max31785_data *driver_data = to_max31785_data(info);
u32 val;
int rv;
@@ -157,7 +256,7 @@ static int max31785_read_word_data(struct i2c_client *client, int page,
rv = max31785_get_pwm(client, page);
break;
case PMBUS_VIRT_PWM_ENABLE_1:
- rv = max31785_get_pwm_mode(client, page);
+ rv = max31785_get_pwm_mode(client, driver_data, page);
break;
default:
rv = -ENODATA;
@@ -188,8 +287,36 @@ static inline u32 max31785_scale_pwm(u32 sensor_val)
return (sensor_val * 100) / 255;
}
-static int max31785_pwm_enable(struct i2c_client *client, int page,
- u16 word)
+static int max31785_update_fan(struct i2c_client *client,
+ struct max31785_data *driver_data, int page,
+ u8 config, u8 mask, u16 command)
+{
+ int from, rv;
+ u8 to;
+
+ from = _max31785_read_byte_data(client, driver_data, page,
+ PMBUS_FAN_CONFIG_12);
+ if (from < 0)
+ return from;
+
+ to = (from & ~mask) | (config & mask);
+
+ if (to != from) {
+ rv = _max31785_write_byte_data(client, driver_data, page,
+ PMBUS_FAN_CONFIG_12, to);
+ if (rv < 0)
+ return rv;
+ }
+
+ rv = _max31785_write_word_data(client, driver_data, page,
+ PMBUS_FAN_COMMAND_1, command);
+
+ return rv;
+}
+
+static int max31785_pwm_enable(struct i2c_client *client,
+ struct max31785_data *driver_data, int page,
+ u16 word)
{
int config = 0;
int rate;
@@ -217,18 +344,23 @@ static int max31785_pwm_enable(struct i2c_client *client, int page,
return -EINVAL;
}
- return pmbus_update_fan(client, page, 0, config, PB_FAN_1_RPM, rate);
+ return max31785_update_fan(client, driver_data, page, config,
+ PB_FAN_1_RPM, rate);
}
static int max31785_write_word_data(struct i2c_client *client, int page,
int reg, u16 word)
{
+ const struct pmbus_driver_info *info = pmbus_get_driver_info(client);
+ struct max31785_data *driver_data = to_max31785_data(info);
+
switch (reg) {
case PMBUS_VIRT_PWM_1:
- return pmbus_update_fan(client, page, 0, 0, PB_FAN_1_RPM,
- max31785_scale_pwm(word));
+ return max31785_update_fan(client, driver_data, page, 0,
+ PB_FAN_1_RPM,
+ max31785_scale_pwm(word));
case PMBUS_VIRT_PWM_ENABLE_1:
- return max31785_pwm_enable(client, page, word);
+ return max31785_pwm_enable(client, driver_data, page, word);
default:
break;
}
@@ -303,13 +435,16 @@ static int max31785_configure_dual_tach(struct i2c_client *client,
{
int ret;
int i;
+ struct max31785_data *driver_data = to_max31785_data(info);
for (i = 0; i < MAX31785_NR_FAN_PAGES; i++) {
- ret = i2c_smbus_write_byte_data(client, PMBUS_PAGE, i);
+ ret = max31785_i2c_write_byte_data(client, driver_data,
+ PMBUS_PAGE, i);
if (ret < 0)
return ret;
- ret = i2c_smbus_read_word_data(client, MFR_FAN_CONFIG);
+ ret = max31785_i2c_read_word_data(client, driver_data,
+ MFR_FAN_CONFIG);
if (ret < 0)
return ret;
@@ -329,6 +464,7 @@ static int max31785_probe(struct i2c_client *client)
{
struct device *dev = &client->dev;
struct pmbus_driver_info *info;
+ struct max31785_data *driver_data;
bool dual_tach = false;
int ret;
@@ -337,13 +473,16 @@ static int max31785_probe(struct i2c_client *client)
I2C_FUNC_SMBUS_WORD_DATA))
return -ENODEV;
- info = devm_kzalloc(dev, sizeof(struct pmbus_driver_info), GFP_KERNEL);
- if (!info)
+ driver_data = devm_kzalloc(dev, sizeof(struct max31785_data), GFP_KERNEL);
+ if (!driver_data)
return -ENOMEM;
+ info = &driver_data->info;
+ driver_data->access = ktime_get();
*info = max31785_info;
- ret = i2c_smbus_write_byte_data(client, PMBUS_PAGE, 255);
+ ret = max31785_i2c_write_byte_data(client,driver_data,
+ PMBUS_PAGE, 255);
if (ret < 0)
return ret;
--
2.39.2
On Mon, Oct 23, 2023 at 01:08:03PM -0500, Lakshmi Yadlapati wrote:
> Changes since V1:
> 1. Changed the max31785_wait macro to a function, following the conventions
> used in other drivers that had the same issue.
> 2. Changed the function names from max31785_i2c_smbus* to max31785_i2c_* and
> from max31785_pmbus_* to _max31785_*, making them more concise.
>
Please check Documentation/process/submitting-patches.rst
for the expected patch format, specifically details on how
description and change log are supposed to look like.
Guenter
Thank you Guenter!
Should I fix and send the patch again?
On 10/25/23, 7:48 PM, "Guenter Roeck" <[email protected] <mailto:[email protected]> on behalf of [email protected] <mailto:[email protected]>> wrote:
On Mon, Oct 23, 2023 at 01:08:03PM -0500, Lakshmi Yadlapati wrote:
> Changes since V1:
> 1. Changed the max31785_wait macro to a function, following the conventions
> used in other drivers that had the same issue.
> 2. Changed the function names from max31785_i2c_smbus* to max31785_i2c_* and
> from max31785_pmbus_* to _max31785_*, making them more concise.
>
Please check Documentation/process/submitting-patches.rst
for the expected patch format, specifically details on how
description and change log are supposed to look like.
Guenter
On 10/26/23 07:48, Lakshmi Yadlapati wrote:
> Thank you Guenter!
> Should I fix and send the patch again?
>
Please do. And don't top-post.
Thanks,
Guenter
> On 10/25/23, 7:48 PM, "Guenter Roeck" <[email protected] <mailto:[email protected]> on behalf of [email protected] <mailto:[email protected]>> wrote:
>
>
> On Mon, Oct 23, 2023 at 01:08:03PM -0500, Lakshmi Yadlapati wrote:
>> Changes since V1:
>> 1. Changed the max31785_wait macro to a function, following the conventions
>> used in other drivers that had the same issue.
>> 2. Changed the function names from max31785_i2c_smbus* to max31785_i2c_* and
>> from max31785_pmbus_* to _max31785_*, making them more concise.
>>
>
>
> Please check Documentation/process/submitting-patches.rst
> for the expected patch format, specifically details on how
> description and change log are supposed to look like.
>
>
> Guenter
>
>
>