Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.33 as permitted sender) client-ip=23.128.96.33;
From:   Lakshmi Yadlapati <lakshmiy@us.ibm.com>
To:     joel@jms.id.au, andrew@aj.id.au, eajames@linux.ibm.com,
        ninad@linux.ibm.com, linux@roeck-us.net, jdelvare@suse.com
Cc:     Lakshmi Yadlapati <lakshmiy@us.ibm.com>,
        linux-hwmon@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: [PATCH v4] hwmon: (pmbus/max31785) Add delay between bus accesses
Date:   Thu, 26 Oct 2023 17:17:08 -0500
Message-Id: <20231026221708.1582518-1-lakshmiy@us.ibm.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

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>
---
V3 -> V4: Fixed warnings realted to this commit
V2 -> V3: Fixed the commit message
V1 -> V2:
- Changed the max31785_wait macro to a function, following the conventions
  used in other drivers that had the same issue.
- Changed the function names from max31785_i2c_smbus* to max31785_i2c_* and
  from max31785_pmbus_* to _max31785_*, making them more concise.

 drivers/hwmon/pmbus/max31785.c | 187 +++++++++++++++++++++++++++++----
 1 file changed, 166 insertions(+), 21 deletions(-)

diff --git a/drivers/hwmon/pmbus/max31785.c b/drivers/hwmon/pmbus/max31785.c
index f9aa576495a5..cc9acd7bac37 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,118 @@ 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 +202,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 +232,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 +262,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 +293,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 +350,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 +441,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 +470,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 +479,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