Re: [PATCH v3] hwmon: (pmbus): Introduce page_change_delay

[Guenter Roeck]

On Thu, Apr 03, 2025 at 02:12:46PM -0700, William A. Kennington III wrote:
> We have some buggy pmbus devices that require a delay after performing a
> page change operation before trying to issue more commands to the
> device.
> 
> This allows for a configurable delay after page changes, but not
> affecting other read or write operations.
> 
> Signed-off-by: William A. Kennington III <william@xxxxxxxxxxxxxxx>
> ---
> V1 -> V2: Simplify how the backoff time is stored and computed
> V2 -> V3: Use the BIT macro
> 
>  drivers/hwmon/pmbus/pmbus.h      |  1 +
>  drivers/hwmon/pmbus/pmbus_core.c | 67 +++++++++++++++-----------------
>  2 files changed, 33 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/hwmon/pmbus/pmbus.h b/drivers/hwmon/pmbus/pmbus.h
> index ddb19c9726d6..742dafc44390 100644
> --- a/drivers/hwmon/pmbus/pmbus.h
> +++ b/drivers/hwmon/pmbus/pmbus.h
> @@ -482,6 +482,7 @@ struct pmbus_driver_info {
>  	 */
>  	int access_delay;		/* in microseconds */
>  	int write_delay;		/* in microseconds */
> +	int page_change_delay;		/* in microseconds */
>  };
>  
>  /* Regulator ops */
> diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
> index 787683e83db6..3aa5851610b2 100644
> --- a/drivers/hwmon/pmbus/pmbus_core.c
> +++ b/drivers/hwmon/pmbus/pmbus_core.c
> @@ -114,8 +114,8 @@ struct pmbus_data {
>  
>  	int vout_low[PMBUS_PAGES];	/* voltage low margin */
>  	int vout_high[PMBUS_PAGES];	/* voltage high margin */
> -	ktime_t write_time;		/* Last SMBUS write timestamp */
> -	ktime_t access_time;		/* Last SMBUS access timestamp */
> +
> +	ktime_t next_access_backoff;	/* Wait until at least this time */
>  };
>  
>  struct pmbus_debugfs_entry {
> @@ -170,33 +170,30 @@ EXPORT_SYMBOL_NS_GPL(pmbus_set_update, "PMBUS");
>  static void pmbus_wait(struct i2c_client *client)
>  {
>  	struct pmbus_data *data = i2c_get_clientdata(client);
> -	const struct pmbus_driver_info *info = data->info;
> -	s64 delta;
> -
> -	if (info->access_delay) {
> -		delta = ktime_us_delta(ktime_get(), data->access_time);
> -
> -		if (delta < info->access_delay)
> -			fsleep(info->access_delay - delta);
> -	} else if (info->write_delay) {
> -		delta = ktime_us_delta(ktime_get(), data->write_time);
> +	s64 delay = ktime_us_delta(data->next_access_backoff, ktime_get());
>  
> -		if (delta < info->write_delay)
> -			fsleep(info->write_delay - delta);
> -	}
> +	if (delay > 0)
> +		fsleep(delay);
>  }
>  
> -/* Sets the last accessed timestamp for pmbus_wait */
> -static void pmbus_update_ts(struct i2c_client *client, bool write_op)
> +#define PMBUS_OP_READ BIT(0)
> +#define PMBUS_OP_WRITE BIT(1)
> +#define PMBUS_OP_PAGE_CHANGE BIT(2)

I guess you really don't like tabs. Ok, no problem, I can fix that up when
I apply the patch. Either case, please move the defines ahead of the first
code block.

> +
> +/* Sets the last operation timestamp for pmbus_wait */
> +static void pmbus_update_ts(struct i2c_client *client, int op)
>  {
>  	struct pmbus_data *data = i2c_get_clientdata(client);
>  	const struct pmbus_driver_info *info = data->info;
> +	int delay = info->access_delay;

Hmm, this is a functional change. It always sets the minimum wait
time to access_delay, even if the operation is a write. I guess
it makes sense because otherwise there would be no delay after
a write if only access_delay is set. However, that means that
PMBUS_OP_READ is not really needed anymore and can be dropped.

This should be explained in the patch description.

>  
> -	if (info->access_delay) {
> -		data->access_time = ktime_get();
> -	} else if (info->write_delay && write_op) {
> -		data->write_time = ktime_get();
> -	}
> +	if (op & (PMBUS_OP_WRITE | PMBUS_OP_PAGE_CHANGE))
> +		delay = max(delay, info->write_delay);
> +	if (op & PMBUS_OP_PAGE_CHANGE)
> +		delay = max(delay, info->page_change_delay);
> +

I would have set PMBUS_OP_WRITE | PMBUS_OP_PAGE_CHANGE in the calling code,
but this is ok too. However, please make it

	if (op & (PMBUS_OP_WRITE | PMBUS_OP_PAGE_CHANGE)) {
		delay = max(delay, info->write_delay);
		if (op & PMBUS_OP_PAGE_CHANGE)
			delay = max(delay, info->page_change_delay);
	}

After dropping PMBUS_OP_READ, that can be simplified further to

	if (op) {
		...
	}

> +	if (delay > 0)
> +		data->next_access_backoff = ktime_add_us(ktime_get(), delay);
>  }
>  
>  int pmbus_set_page(struct i2c_client *client, int page, int phase)
> @@ -211,13 +208,13 @@ int pmbus_set_page(struct i2c_client *client, int page, int phase)
>  	    data->info->pages > 1 && page != data->currpage) {
>  		pmbus_wait(client);
>  		rv = i2c_smbus_write_byte_data(client, PMBUS_PAGE, page);
> -		pmbus_update_ts(client, true);
> +		pmbus_update_ts(client, PMBUS_OP_PAGE_CHANGE);
>  		if (rv < 0)
>  			return rv;
>  
>  		pmbus_wait(client);
>  		rv = i2c_smbus_read_byte_data(client, PMBUS_PAGE);
> -		pmbus_update_ts(client, false);
> +		pmbus_update_ts(client, PMBUS_OP_READ);
>  		if (rv < 0)
>  			return rv;
>  
> @@ -231,7 +228,7 @@ int pmbus_set_page(struct i2c_client *client, int page, int phase)
>  		pmbus_wait(client);
>  		rv = i2c_smbus_write_byte_data(client, PMBUS_PHASE,
>  					       phase);
> -		pmbus_update_ts(client, true);
> +		pmbus_update_ts(client, PMBUS_OP_WRITE);
>  		if (rv)
>  			return rv;
>  	}
> @@ -251,7 +248,7 @@ int pmbus_write_byte(struct i2c_client *client, int page, u8 value)
>  
>  	pmbus_wait(client);
>  	rv = i2c_smbus_write_byte(client, value);
> -	pmbus_update_ts(client, true);
> +	pmbus_update_ts(client, PMBUS_OP_WRITE);
>  
>  	return rv;
>  }
> @@ -286,7 +283,7 @@ int pmbus_write_word_data(struct i2c_client *client, int page, u8 reg,
>  
>  	pmbus_wait(client);
>  	rv = i2c_smbus_write_word_data(client, reg, word);
> -	pmbus_update_ts(client, true);
> +	pmbus_update_ts(client, PMBUS_OP_WRITE);
>  
>  	return rv;
>  }
> @@ -408,7 +405,7 @@ int pmbus_read_word_data(struct i2c_client *client, int page, int phase, u8 reg)
>  
>  	pmbus_wait(client);
>  	rv = i2c_smbus_read_word_data(client, reg);
> -	pmbus_update_ts(client, false);
> +	pmbus_update_ts(client, PMBUS_OP_READ);
>  
>  	return rv;
>  }
> @@ -471,7 +468,7 @@ int pmbus_read_byte_data(struct i2c_client *client, int page, u8 reg)
>  
>  	pmbus_wait(client);
>  	rv = i2c_smbus_read_byte_data(client, reg);
> -	pmbus_update_ts(client, false);
> +	pmbus_update_ts(client, PMBUS_OP_READ);
>  
>  	return rv;
>  }
> @@ -487,7 +484,7 @@ int pmbus_write_byte_data(struct i2c_client *client, int page, u8 reg, u8 value)
>  
>  	pmbus_wait(client);
>  	rv = i2c_smbus_write_byte_data(client, reg, value);
> -	pmbus_update_ts(client, true);
> +	pmbus_update_ts(client, PMBUS_OP_WRITE);
>  
>  	return rv;
>  }
> @@ -523,7 +520,7 @@ static int pmbus_read_block_data(struct i2c_client *client, int page, u8 reg,
>  
>  	pmbus_wait(client);
>  	rv = i2c_smbus_read_block_data(client, reg, data_buf);
> -	pmbus_update_ts(client, false);
> +	pmbus_update_ts(client, PMBUS_OP_READ);
>  
>  	return rv;
>  }
> @@ -2530,7 +2527,7 @@ static int pmbus_read_coefficients(struct i2c_client *client,
>  	rv = i2c_smbus_xfer(client->adapter, client->addr, client->flags,
>  			    I2C_SMBUS_WRITE, PMBUS_COEFFICIENTS,
>  			    I2C_SMBUS_BLOCK_PROC_CALL, &data);
> -	pmbus_update_ts(client, true);
> +	pmbus_update_ts(client, PMBUS_OP_READ | PMBUS_OP_WRITE);

I'd argue that this does not warrant a PMBUS_OP_WRITE in the first place.
>From the chip's perspective, the operation is complete after the data
is returned. This is just as much a write as any other SMBus read operation
(a write of an address followed by a read). If you think otherwise, please
explain.

Either case, the change warrants an explanation in the patch description.

Thanks,
Guenter