hwmon: (pmbus/max31785) Add delay between bus accesses

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 <lakshmiy@us.ibm.com>
Link: https://lore.kernel.org/r/20231027044346.2167548-1-lakshmiy@us.ibm.com
Signed-off-by: Guenter Roeck <linux@roeck-us.net>

diff --git a/drivers/hwmon/pmbus/max31785.c b/drivers/hwmon/pmbus/max31785.c
index f9aa576495a50fdc57ef0b1ea143594b269d9a7d..5d13bbfc8f476d055e13a549f0809a69a604e657 100644 (file)
--- 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,119 @@ 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)
+               usleep_range(MAX31785_WAIT_DELAY_US - delta,
+                            MAX31785_WAIT_DELAY_US);
+}
+
+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 +203,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 +233,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 +263,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 +294,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 +351,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 +442,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 +471,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 +480,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;