From: Sanman Pradhan <psanman@juniper.net>

The MAX31785 fan controller occasionally NACKs master transactions if
accesses are too tightly spaced. To avoid this, the driver currently
enforces a 250us inter-access delay with a private timestamp and wrapper
functions around both raw SMBus accesses and PMBus helper paths.

Use pmbus_driver_info.access_delay for normal PMBus-mediated accesses
instead, and remove the driver-local PMBus read/write wrappers.

Keep local delay handling for raw SMBus accesses used before
pmbus_do_probe(). Also add explicit delays around the raw i2c_transfer()
long-read path, since it bypasses PMBus core timing and still needs to
be spaced from surrounding transactions.

Update max31785_read_byte_data() so PMBUS_FAN_CONFIG_12 accesses are
only remapped for virtual pages, allowing physical-page accesses to fall
back to the PMBus core. With that change, use pmbus_update_fan() for fan
configuration updates.

Also use the delayed raw read helper for MFR_REVISION during probe, drop
the unused to_max31785_data() macro, and rename the local variable
"virtual" to "vpage".

Signed-off-by: Sanman Pradhan <psanman@juniper.net>
---
 drivers/hwmon/pmbus/max31785.c | 186 +++++++++++----------------------
 1 file changed, 59 insertions(+), 127 deletions(-)

diff --git a/drivers/hwmon/pmbus/max31785.c b/drivers/hwmon/pmbus/max31785.c
index 50073fe0c5e8..e9c3c36c8663 100644
--- a/drivers/hwmon/pmbus/max31785.c
+++ b/drivers/hwmon/pmbus/max31785.c
@@ -31,7 +31,6 @@ struct max31785_data {
 	struct pmbus_driver_info info;
 };
 
-#define to_max31785_data(x)  container_of(x, struct max31785_data, info)
 
 /*
  * MAX31785 Driver Workaround
@@ -40,9 +39,8 @@ struct max31785_data {
  * 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.
+ * Keep minimal local delay handling for raw pre-probe SMBus accesses.
+ * Normal PMBus-mediated accesses use pmbus_driver_info.access_delay instead.
  */
 static inline void max31785_wait(const struct max31785_data *data)
 {
@@ -54,89 +52,42 @@ static inline void max31785_wait(const struct max31785_data *data)
 }
 
 static int max31785_i2c_write_byte_data(struct i2c_client *client,
-					struct max31785_data *driver_data,
-					int command, u8 data)
+					struct max31785_data *data,
+					int command, u8 value)
 {
 	int rc;
 
-	max31785_wait(driver_data);
-	rc = i2c_smbus_write_byte_data(client, command, data);
-	driver_data->access = ktime_get();
+	max31785_wait(data);
+	rc = i2c_smbus_write_byte_data(client, command, value);
+	data->access = ktime_get();
 	return rc;
 }
 
 static int max31785_i2c_read_word_data(struct i2c_client *client,
-				       struct max31785_data *driver_data,
+				       struct max31785_data *data,
 				       int command)
 {
 	int rc;
 
-	max31785_wait(driver_data);
+	max31785_wait(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();
+	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 _max31785_read_byte_data(client, driver_data,
-						page - MAX31785_NR_PAGES,
-						reg);
+		if (page < MAX31785_NR_PAGES)
+			return -ENODATA;
+
+		return pmbus_read_byte_data(client,
+					    page - MAX31785_NR_PAGES,
+					    reg);
 	}
 
 	return -ENODATA;
@@ -178,9 +129,22 @@ static int max31785_read_long_data(struct i2c_client *client, int page,
 	if (rc < 0)
 		return rc;
 
+	/*
+	 * The following raw i2c_transfer() bypasses PMBus core access timing.
+	 * Add an explicit delay before the transfer so it is properly spaced
+	 * from the preceding PMBus transaction.
+	 */
+	usleep_range(MAX31785_WAIT_DELAY_US, MAX31785_WAIT_DELAY_US + 50);
+
 	rc = i2c_transfer(client->adapter, msg, ARRAY_SIZE(msg));
 	if (rc < 0)
 		return rc;
+	/*
+	 * The preceding raw i2c_transfer() bypasses PMBus core access timing.
+	 * Add an explicit delay after the transfer so the next PMBus access
+	 * preserves the required inter-transaction spacing.
+	 */
+	usleep_range(MAX31785_WAIT_DELAY_US, MAX31785_WAIT_DELAY_US + 50);
 
 	*data = (rspbuf[0] << (0 * 8)) | (rspbuf[1] << (1 * 8)) |
 		(rspbuf[2] << (2 * 8)) | (rspbuf[3] << (3 * 8));
@@ -203,19 +167,18 @@ static int max31785_get_pwm(struct i2c_client *client, int page)
 	return rv;
 }
 
-static int max31785_get_pwm_mode(struct i2c_client *client,
-				 struct max31785_data *driver_data, int page)
+static int max31785_get_pwm_mode(struct i2c_client *client, int page)
 {
 	int config;
 	int command;
 
-	config = _max31785_read_byte_data(client, driver_data, page,
-					  PMBUS_FAN_CONFIG_12);
+	config = pmbus_read_byte_data(client, page,
+				      PMBUS_FAN_CONFIG_12);
 	if (config < 0)
 		return config;
 
-	command = _max31785_read_word_data(client, driver_data, page, 0xff,
-					   PMBUS_FAN_COMMAND_1);
+	command = pmbus_read_word_data(client, page, 0xff,
+				       PMBUS_FAN_COMMAND_1);
 	if (command < 0)
 		return command;
 
@@ -233,8 +196,6 @@ static int max31785_get_pwm_mode(struct i2c_client *client,
 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;
 
@@ -263,7 +224,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, driver_data, page);
+		rv = max31785_get_pwm_mode(client, page);
 		break;
 	default:
 		rv = -ENODATA;
@@ -294,35 +255,7 @@ static inline u32 max31785_scale_pwm(u32 sensor_val)
 	return (sensor_val * 100) / 255;
 }
 
-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,
+static int max31785_pwm_enable(struct i2c_client *client, int page,
 			       u16 word)
 {
 	int config = 0;
@@ -351,23 +284,21 @@ static int max31785_pwm_enable(struct i2c_client *client,
 		return -EINVAL;
 	}
 
-	return max31785_update_fan(client, driver_data, page, config,
+	return pmbus_update_fan(client, page, 0, 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 max31785_update_fan(client, driver_data, page, 0,
-					   PB_FAN_1_RPM,
-					   max31785_scale_pwm(word));
+		return pmbus_update_fan(client, page, 0, 0,
+					PB_FAN_1_RPM,
+					max31785_scale_pwm(word));
 	case PMBUS_VIRT_PWM_ENABLE_1:
-		return max31785_pwm_enable(client, driver_data, page, word);
+		return max31785_pwm_enable(client, page, word);
 	default:
 		break;
 	}
@@ -391,6 +322,7 @@ static const struct pmbus_driver_info max31785_info = {
 	.read_byte_data = max31785_read_byte_data,
 	.read_word_data = max31785_read_word_data,
 	.write_byte = max31785_write_byte,
+	.access_delay = MAX31785_WAIT_DELAY_US,
 
 	/* RPM */
 	.format[PSC_FAN] = direct,
@@ -438,29 +370,29 @@ static const struct pmbus_driver_info max31785_info = {
 };
 
 static int max31785_configure_dual_tach(struct i2c_client *client,
-					struct pmbus_driver_info *info)
+					struct max31785_data *data)
 {
+	struct pmbus_driver_info *info = &data->info;
 	int ret;
 	int i;
-	struct max31785_data *driver_data = to_max31785_data(info);
 
 	for (i = 0; i < MAX31785_NR_FAN_PAGES; i++) {
-		ret = max31785_i2c_write_byte_data(client, driver_data,
+		ret = max31785_i2c_write_byte_data(client, data,
 						   PMBUS_PAGE, i);
 		if (ret < 0)
 			return ret;
 
-		ret = max31785_i2c_read_word_data(client, driver_data,
+		ret = max31785_i2c_read_word_data(client, data,
 						  MFR_FAN_CONFIG);
 		if (ret < 0)
 			return ret;
 
 		if (ret & MFR_FAN_CONFIG_DUAL_TACH) {
-			int virtual = MAX31785_NR_PAGES + i;
+			int vpage = MAX31785_NR_PAGES + i;
 
-			info->pages = virtual + 1;
-			info->func[virtual] |= PMBUS_HAVE_FAN12;
-			info->func[virtual] |= PMBUS_PAGE_VIRTUAL;
+			info->pages = vpage + 1;
+			info->func[vpage] |= PMBUS_HAVE_FAN12;
+			info->func[vpage] |= PMBUS_PAGE_VIRTUAL;
 		}
 	}
 
@@ -471,7 +403,7 @@ static int max31785_probe(struct i2c_client *client)
 {
 	struct device *dev = &client->dev;
 	struct pmbus_driver_info *info;
-	struct max31785_data *driver_data;
+	struct max31785_data *data;
 	bool dual_tach = false;
 	int ret;
 
@@ -480,20 +412,20 @@ static int max31785_probe(struct i2c_client *client)
 				     I2C_FUNC_SMBUS_WORD_DATA))
 		return -ENODEV;
 
-	driver_data = devm_kzalloc(dev, sizeof(struct max31785_data), GFP_KERNEL);
-	if (!driver_data)
+	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
 		return -ENOMEM;
 
-	info = &driver_data->info;
-	driver_data->access = ktime_get();
+	data->access = ktime_get();
+	info = &data->info;
 	*info = max31785_info;
 
-	ret = max31785_i2c_write_byte_data(client, driver_data,
-					   PMBUS_PAGE, 255);
+	ret = max31785_i2c_write_byte_data(client, data,
+					   PMBUS_PAGE, 0xff);
 	if (ret < 0)
 		return ret;
 
-	ret = i2c_smbus_read_word_data(client, MFR_REVISION);
+	ret = max31785_i2c_read_word_data(client, data, MFR_REVISION);
 	if (ret < 0)
 		return ret;
 
@@ -509,7 +441,7 @@ static int max31785_probe(struct i2c_client *client)
 	}
 
 	if (dual_tach) {
-		ret = max31785_configure_dual_tach(client, info);
+		ret = max31785_configure_dual_tach(client, data);
 		if (ret < 0)
 			return ret;
 	}
-- 
2.34.1

On Sat, Mar 07, 2026 at 02:45:20PM -0800, Sanman Pradhan wrote:
> From: Sanman Pradhan <psanman@juniper.net>
> 
> The MAX31785 fan controller occasionally NACKs master transactions if
> accesses are too tightly spaced. To avoid this, the driver currently
> enforces a 250us inter-access delay with a private timestamp and wrapper
> functions around both raw SMBus accesses and PMBus helper paths.
> 
> Use pmbus_driver_info.access_delay for normal PMBus-mediated accesses
> instead, and remove the driver-local PMBus read/write wrappers.
> 
> Keep local delay handling for raw SMBus accesses used before
> pmbus_do_probe(). Also add explicit delays around the raw i2c_transfer()
> long-read path, since it bypasses PMBus core timing and still needs to
> be spaced from surrounding transactions.
> 
> Update max31785_read_byte_data() so PMBUS_FAN_CONFIG_12 accesses are
> only remapped for virtual pages, allowing physical-page accesses to fall
> back to the PMBus core. With that change, use pmbus_update_fan() for fan
> configuration updates.
> 
> Also use the delayed raw read helper for MFR_REVISION during probe, drop
> the unused to_max31785_data() macro, and rename the local variable
> "virtual" to "vpage".
> 
> Signed-off-by: Sanman Pradhan <psanman@juniper.net>
> ---
>  drivers/hwmon/pmbus/max31785.c | 186 +++++++++++----------------------
>  1 file changed, 59 insertions(+), 127 deletions(-)
> 
> diff --git a/drivers/hwmon/pmbus/max31785.c b/drivers/hwmon/pmbus/max31785.c
> index 50073fe0c5e8..e9c3c36c8663 100644
> --- a/drivers/hwmon/pmbus/max31785.c
> +++ b/drivers/hwmon/pmbus/max31785.c
...
> @@ -178,9 +129,22 @@ static int max31785_read_long_data(struct i2c_client *client, int page,
>  	if (rc < 0)
>  		return rc;
>  
> +	/*
> +	 * The following raw i2c_transfer() bypasses PMBus core access timing.
> +	 * Add an explicit delay before the transfer so it is properly spaced
> +	 * from the preceding PMBus transaction.
> +	 */
> +	usleep_range(MAX31785_WAIT_DELAY_US, MAX31785_WAIT_DELAY_US + 50);
> +
>  	rc = i2c_transfer(client->adapter, msg, ARRAY_SIZE(msg));
>  	if (rc < 0)
>  		return rc;
> +	/*
> +	 * The preceding raw i2c_transfer() bypasses PMBus core access timing.
> +	 * Add an explicit delay after the transfer so the next PMBus access
> +	 * preserves the required inter-transaction spacing.
> +	 */
> +	usleep_range(MAX31785_WAIT_DELAY_US, MAX31785_WAIT_DELAY_US + 50);

AI feedback:

  If `i2c_transfer()` fails, the function returns early without executing the
  trailing `usleep_range()`. Since this raw transfer bypasses the PMBus core
  timing management (`pmbus_update_ts()`), the PMBus core is unaware of the
  exact time this I2C transfer occurred.
  By returning early on error, the required 250us inter-transaction delay is
  skipped. The next PMBus transaction issued by the core might be executed
  immediately, potentially leading to a cascade of communication failures.
  Would it be better to move the trailing `usleep_range()` before the error
  check so it executes unconditionally?

Seems to me it has a point.

Also, would it make sense to export and use pmbus_wait() and pmbus_update_ts()
instead ?

Thanks,
Guenter