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

Guenter Roeck
2024-07-16T02:23:04Z

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@9elements.com>
> ---
> 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)

I'd suggest to name the function either pmbus_access_wait() or even simply pmbus_wait().

> +{
> + 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);

I am not too happy with this one; the delay can get large.
I'd suggest to use fsleep().

> + } else if (info->write_delay) {
> + delta = ktime_us_delta(ktime_get(), data->write_time);
> +
> + if (delta < info->write_delay)
> + udelay(info->write_delay - delta);

Same here.

Thanks,
Guenter