Re: [PATCH 1/5] hwmon: pmbus: Implement generic bus access delay

[Guenter Roeck]

On 7/15/24 00:30, Patrick Rudolph wrote:
> Some drivers, like the max15301 or zl6100, are intentionally delaying
> SMBus communications, to prevent transmission errors. As this is necessary
> on additional PMBus compatible devices, implement a generic delay mechanism
> in the pmbus core.
>
> Introduces two delay settings in the pmbus_driver_info struct, one applies
> to every SMBus transaction and the other is for write transaction only.
> Once set by the driver the SMBus traffic, using the generic pmbus access
> helpers, is automatically delayed when necessary.
>
> The two settings are:
> access_delay:
>    - Unit in microseconds
>    - Stores the accessed timestamp after every SMBus access
>    - Delays when necessary before the next SMBus access
>
> write_delay:
>    - Unit in microseconds
>    - Stores the written timestamp after a write SMBus access
>    - Delays when necessary before the next SMBus access
>
> This allows to drop the custom delay code from the drivers and easily
> introduce this feature in additional pmbus drivers.
>
> Signed-off-by: Patrick Rudolph <patrick.rudolph@xxxxxxxxxxxxx>
> ---
>   drivers/hwmon/pmbus/pmbus.h      | 10 ++++
>   drivers/hwmon/pmbus/pmbus_core.c | 92 +++++++++++++++++++++++++++++---
>   2 files changed, 96 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/hwmon/pmbus/pmbus.h b/drivers/hwmon/pmbus/pmbus.h
> index fb442fae7b3e..5d5dc774187b 100644
> --- a/drivers/hwmon/pmbus/pmbus.h
> +++ b/drivers/hwmon/pmbus/pmbus.h
> @@ -466,6 +466,16 @@ struct pmbus_driver_info {
>   
>   	/* custom attributes */
>   	const struct attribute_group **groups;
> +
> +	/*
> +	 * Some chips need a little delay between SMBus communication. When
> +	 * set, the generic PMBus helper functions will wait if necessary
> +	 * to meet this requirement. The access delay is honored after
> +	 * every SMBus operation. The write delay is only honored after
> +	 * SMBus write operations.
> +	 */
> +	int access_delay;		/* in microseconds */
> +	int write_delay;		/* in microseconds */
>   };
>   
>   /* Regulator ops */
> diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
> index cb4c65a7f288..5cb093c898a1 100644
> --- a/drivers/hwmon/pmbus/pmbus_core.c
> +++ b/drivers/hwmon/pmbus/pmbus_core.c
> @@ -7,6 +7,7 @@
>    */
>   
>   #include <linux/debugfs.h>
> +#include <linux/delay.h>
>   #include <linux/kernel.h>
>   #include <linux/math64.h>
>   #include <linux/module.h>
> @@ -108,6 +109,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 */
>   };
>   
>   struct pmbus_debugfs_entry {
> @@ -158,6 +161,39 @@ void pmbus_set_update(struct i2c_client *client, u8 reg, bool update)
>   }
>   EXPORT_SYMBOL_NS_GPL(pmbus_set_update, PMBUS);
>   
> +/* Some chips need a delay between accesses. */
> +static inline void pmbus_optional_wait(struct i2c_client *client)

Also, please drop the inline here and below to let the compiler decide
if it wants to inline or not.

Thanks,
Guenter

> +{
> +	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)
> +			udelay(info->access_delay - delta);
> +	} else if (info->write_delay) {
> +		delta = ktime_us_delta(ktime_get(), data->write_time);
> +
> +		if (delta < info->write_delay)
> +			udelay(info->write_delay - delta);
> +	}
> +}
> +
> +/* Sets the last accessed timestamp for pmbus_optional_wait */
> +static inline void pmbus_update_ts(struct i2c_client *client, bool write_op)
> +{
> +	struct pmbus_data *data = i2c_get_clientdata(client);
> +	const struct pmbus_driver_info *info = data->info;
> +
> +	if (info->access_delay) {
> +		data->access_time = ktime_get();
> +	} else if (info->write_delay && write_op) {
> +		data->write_time = ktime_get();
> +	}
> +}
> +
>   int pmbus_set_page(struct i2c_client *client, int page, int phase)
>   {
>   	struct pmbus_data *data = i2c_get_clientdata(client);
> @@ -168,11 +204,15 @@ int pmbus_set_page(struct i2c_client *client, int page, int phase)
>   
>   	if (!(data->info->func[page] & PMBUS_PAGE_VIRTUAL) &&
>   	    data->info->pages > 1 && page != data->currpage) {
> +		pmbus_optional_wait(client);
>   		rv = i2c_smbus_write_byte_data(client, PMBUS_PAGE, page);
> +		pmbus_update_ts(client, true);
>   		if (rv < 0)
>   			return rv;
>   
> +		pmbus_optional_wait(client);
>   		rv = i2c_smbus_read_byte_data(client, PMBUS_PAGE);
> +		pmbus_update_ts(client, false);
>   		if (rv < 0)
>   			return rv;
>   
> @@ -183,8 +223,10 @@ int pmbus_set_page(struct i2c_client *client, int page, int phase)
>   
>   	if (data->info->phases[page] && data->currphase != phase &&
>   	    !(data->info->func[page] & PMBUS_PHASE_VIRTUAL)) {
> +		pmbus_optional_wait(client);
>   		rv = i2c_smbus_write_byte_data(client, PMBUS_PHASE,
>   					       phase);
> +		pmbus_update_ts(client, true);
>   		if (rv)
>   			return rv;
>   	}
> @@ -202,7 +244,11 @@ int pmbus_write_byte(struct i2c_client *client, int page, u8 value)
>   	if (rv < 0)
>   		return rv;
>   
> -	return i2c_smbus_write_byte(client, value);
> +	pmbus_optional_wait(client);
> +	rv = i2c_smbus_write_byte(client, value);
> +	pmbus_update_ts(client, true);
> +
> +	return rv;
>   }
>   EXPORT_SYMBOL_NS_GPL(pmbus_write_byte, PMBUS);
>   
> @@ -233,7 +279,11 @@ int pmbus_write_word_data(struct i2c_client *client, int page, u8 reg,
>   	if (rv < 0)
>   		return rv;
>   
> -	return i2c_smbus_write_word_data(client, reg, word);
> +	pmbus_optional_wait(client);
> +	rv = i2c_smbus_write_word_data(client, reg, word);
> +	pmbus_update_ts(client, true);
> +
> +	return rv;
>   }
>   EXPORT_SYMBOL_NS_GPL(pmbus_write_word_data, PMBUS);
>   
> @@ -351,7 +401,11 @@ int pmbus_read_word_data(struct i2c_client *client, int page, int phase, u8 reg)
>   	if (rv < 0)
>   		return rv;
>   
> -	return i2c_smbus_read_word_data(client, reg);
> +	pmbus_optional_wait(client);
> +	rv = i2c_smbus_read_word_data(client, reg);
> +	pmbus_update_ts(client, false);
> +
> +	return rv;
>   }
>   EXPORT_SYMBOL_NS_GPL(pmbus_read_word_data, PMBUS);
>   
> @@ -410,7 +464,11 @@ int pmbus_read_byte_data(struct i2c_client *client, int page, u8 reg)
>   	if (rv < 0)
>   		return rv;
>   
> -	return i2c_smbus_read_byte_data(client, reg);
> +	pmbus_optional_wait(client);
> +	rv = i2c_smbus_read_byte_data(client, reg);
> +	pmbus_update_ts(client, false);
> +
> +	return rv;
>   }
>   EXPORT_SYMBOL_NS_GPL(pmbus_read_byte_data, PMBUS);
>   
> @@ -422,7 +480,11 @@ int pmbus_write_byte_data(struct i2c_client *client, int page, u8 reg, u8 value)
>   	if (rv < 0)
>   		return rv;
>   
> -	return i2c_smbus_write_byte_data(client, reg, value);
> +	pmbus_optional_wait(client);
> +	rv = i2c_smbus_write_byte_data(client, reg, value);
> +	pmbus_update_ts(client, true);
> +
> +	return rv;
>   }
>   EXPORT_SYMBOL_NS_GPL(pmbus_write_byte_data, PMBUS);
>   
> @@ -454,7 +516,11 @@ static int pmbus_read_block_data(struct i2c_client *client, int page, u8 reg,
>   	if (rv < 0)
>   		return rv;
>   
> -	return i2c_smbus_read_block_data(client, reg, data_buf);
> +	pmbus_optional_wait(client);
> +	rv = i2c_smbus_read_block_data(client, reg, data_buf);
> +	pmbus_update_ts(client, false);
> +
> +	return rv;
>   }
>   
>   static struct pmbus_sensor *pmbus_find_sensor(struct pmbus_data *data, int page,
> @@ -2450,9 +2516,11 @@ static int pmbus_read_coefficients(struct i2c_client *client,
>   	data.block[1] = attr->reg;
>   	data.block[2] = 0x01;
>   
> +	pmbus_optional_wait(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, false);
>   
>   	if (rv < 0)
>   		return rv;
> @@ -2604,7 +2672,10 @@ static int pmbus_init_common(struct i2c_client *client, struct pmbus_data *data,
>   
>   	/* Enable PEC if the controller and bus supports it */
>   	if (!(data->flags & PMBUS_NO_CAPABILITY)) {
> +		pmbus_optional_wait(client);
>   		ret = i2c_smbus_read_byte_data(client, PMBUS_CAPABILITY);
> +		pmbus_update_ts(client, false);
> +
>   		if (ret >= 0 && (ret & PB_CAPABILITY_ERROR_CHECK)) {
>   			if (i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_PEC))
>   				client->flags |= I2C_CLIENT_PEC;
> @@ -2617,10 +2688,16 @@ static int pmbus_init_common(struct i2c_client *client, struct pmbus_data *data,
>   	 * Bail out if both registers are not supported.
>   	 */
>   	data->read_status = pmbus_read_status_word;
> +	pmbus_optional_wait(client);
>   	ret = i2c_smbus_read_word_data(client, PMBUS_STATUS_WORD);
> +	pmbus_update_ts(client, false);
> +
>   	if (ret < 0 || ret == 0xffff) {
>   		data->read_status = pmbus_read_status_byte;
> +		pmbus_optional_wait(client);
>   		ret = i2c_smbus_read_byte_data(client, PMBUS_STATUS_BYTE);
> +		pmbus_update_ts(client, false);
> +
>   		if (ret < 0 || ret == 0xff) {
>   			dev_err(dev, "PMBus status register not found\n");
>   			return -ENODEV;
> @@ -2635,7 +2712,10 @@ static int pmbus_init_common(struct i2c_client *client, struct pmbus_data *data,
>   	 * limit registers need to be disabled.
>   	 */
>   	if (!(data->flags & PMBUS_NO_WRITE_PROTECT)) {
> +		pmbus_optional_wait(client);
>   		ret = i2c_smbus_read_byte_data(client, PMBUS_WRITE_PROTECT);
> +		pmbus_update_ts(client, false);
> +
>   		if (ret > 0 && (ret & PB_WP_ANY))
>   			data->flags |= PMBUS_WRITE_PROTECTED | PMBUS_SKIP_STATUS_CHECK;
>   	}