Re: [PATCH 2/2] hwmon: (pmbus/max31785) use access_delay for PMBus-mediated accesses

[Guenter Roeck]

On Sat, Mar 07, 2026 at 02:45:20PM -0800, Sanman Pradhan wrote:
> From: Sanman Pradhan <psanman@xxxxxxxxxxx>
> 
> 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@xxxxxxxxxxx>
> ---
>  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