RE: [PATCH v3] hwmon: (pmbus/max31785) Add delay between bus accesses

[Lakshmi Yadlapati]

On 10/26/23, 4:41 PM, "Guenter Roeck" <groeck7@xxxxxxxxx <mailto:groeck7@xxxxxxxxx> on behalf of linux@xxxxxxxxxxxx <mailto:linux@xxxxxxxxxxxx>> wrote:


On Thu, Oct 26, 2023 at 04:20:09PM -0500, Lakshmi Yadlapati wrote:
> The MAX31785 has shown erratic behaviour across multiple system
> designs, unexpectedly clock stretching and NAKing transactions.
> 
> Experimentation shows that this seems to be triggered by a register access
> directly back to back with a previous register write. Experimentation also
> shows that inserting a small delay after register writes makes the issue go
> away.
> 
> Use a similar solution to what the max15301 driver does to solve the same
> problem. Create a custom set of bus read and write functions that make sure
> that the delay is added.
> 
> Signed-off-by: Lakshmi Yadlapati <lakshmiy@xxxxxxxxxx <mailto:lakshmiy@xxxxxxxxxx>>


>I didn't really expect this at this point, but checkpatch says:


>total: 3 errors, 8 warnings, 0 checks, 272 lines checked


>_Please run your patch through checkoatch --strict and fix
>what it reports.

Fixed warnings related to this commit. There is one old warning that I will address in another commit.

WARNING: DT compatible string "maxim,max31785b" appears un-documented -- check ./Documentation/devicetree/bindings/
#531: FILE: drivers/hwmon/pmbus/max31785.c:531:
+	{ .compatible = "maxim,max31785b" },

> ---
> V2 -> V3: Fixed the commit message
> V1 -> V2: 
> - Changed the max31785_wait macro to a function, following the conventions
> used in other drivers that had the same issue.
> - Changed the function names from max31785_i2c_smbus* to max31785_i2c_* and
> from max31785_pmbus_* to _max31785_*, making them more concise.
> 
> drivers/hwmon/pmbus/max31785.c | 181 +++++++++++++++++++++++++++++----
> 1 file changed, 160 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/hwmon/pmbus/max31785.c b/drivers/hwmon/pmbus/max31785.c
> index f9aa576495a5..0e4f9bec542d 100644
> --- a/drivers/hwmon/pmbus/max31785.c
> +++ b/drivers/hwmon/pmbus/max31785.c
> @@ -3,6 +3,7 @@
> * Copyright (C) 2017 IBM Corp.
> */
> 
> +#include <linux/delay.h>
> #include <linux/kernel.h>
> #include <linux/module.h>
> #include <linux/init.h>
> @@ -23,19 +24,112 @@ enum max31785_regs {
> 
> #define MAX31785_NR_PAGES 23
> #define MAX31785_NR_FAN_PAGES 6
> +#define MAX31785_WAIT_DELAY_US 250
> 
> -static int max31785_read_byte_data(struct i2c_client *client, int page,
> - int reg)
> +struct max31785_data {
> + ktime_t access; /* Chip access time */
> + struct pmbus_driver_info info;
> +};
> +
> +#define to_max31785_data(x) container_of(x, struct max31785_data, info)
> +
> +/*
> + * MAX31785 Driver Workaround
> + *
> + * The MAX31785 fan controller occasionally exhibits communication issues.
> + * 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.
> + */
> +static inline void max31785_wait(const struct max31785_data *data)
> {
> - if (page < MAX31785_NR_PAGES)
> - return -ENODATA;
> + s64 delta = ktime_us_delta(ktime_get(), data->access);
> +
> + if (delta < MAX31785_WAIT_DELAY_US)
> + udelay(MAX31785_WAIT_DELAY_US - delta);


Sure you want to use udelay() here and not usleep_range()
or maybe fsleep() ?


Thanks,
Guenter