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

[Guenter Roeck <linux@xxxxxxxxxxxx>]

On 10/23/23 06:56, 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>
> ---
>   drivers/hwmon/pmbus/max31785.c | 167 ++++++++++++++++++++++++++++-----
>   1 file changed, 146 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/hwmon/pmbus/max31785.c b/drivers/hwmon/pmbus/max31785.c
> index f9aa576495a5..40fafb3b1867 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,98 @@ 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.
> + */
> +
> +#define max31785_wait(_func, _driver_data, ...)	({			\
> +	int _ret;							\
> +	s64 delta = ktime_us_delta(ktime_get(), driver_data->access);	\
> +	if (delta < MAX31785_WAIT_DELAY_US)				\
> +		udelay(MAX31785_WAIT_DELAY_US - delta);			\
> +	/* All relevant functions return int */				\
> +	_ret = _func(__VA_ARGS__);					\
> +	_driver_data->access = ktime_get();				\
> +	_ret;								\
> +})

Please no function macros. This can easily be implemented as function,
like with all other drivers having the same problem.

> +
> +static int max31785_i2c_smbus_write_byte_data(struct i2c_client *client,
> +					      struct max31785_data *driver_data,
> +					      int command, u16 data)

Please reduce the size of local function names. i2c_smbus -> i2c, and
the _pmbus in the functions below adds no value.

Guenter

>   {
> -	if (page < MAX31785_NR_PAGES)
> -		return -ENODATA;
> +	return max31785_wait(i2c_smbus_write_byte_data, driver_data, client,
> +			     command, data);
> +}
> +
> +static int max31785_i2c_smbus_read_word_data(struct i2c_client *client,
> +					     struct max31785_data *driver_data,
> +					     int command)
> +{
> +	return max31785_wait(i2c_smbus_read_word_data, driver_data, client,
> +			     command);
> +}
> +
> +static int max31785_pmbus_read_byte_data(struct i2c_client *client,
> +					 struct max31785_data *driver_data,
> +					 int page, int command)
> +{
> +	return max31785_wait(pmbus_read_byte_data, driver_data, client, page,
> +			     command);
> +}
> +
> +static int max31785_pmbus_write_byte_data(struct i2c_client *client,
> +					  struct max31785_data *driver_data,
> +					  int page, int command, u16 data)
> +{
> +	return max31785_wait(pmbus_write_byte_data, driver_data, client, page,
> +			     command, data);
> +}
> +
> +static int max31785_pmbus_read_word_data(struct i2c_client *client,
> +					 struct max31785_data *driver_data,
> +					 int page, int phase, int command)
> +{
> +	return max31785_wait(pmbus_read_word_data, driver_data, client, page,
> +			     phase, command);
> +}
> +
> +static int max31785_pmbus_write_word_data(struct i2c_client *client,
> +					  struct max31785_data *driver_data,
> +					  int page, int command, u16 data)
> +{
> +	return max31785_wait(pmbus_write_word_data, driver_data, client, page,
> +			     command, data);
> +}
> +
> +static int max31785_read_byte_data(struct i2c_client *client, int page, int reg)
> +{
> +	const struct pmbus_driver_info *info = pmbus_get_driver_info(client);
> +	struct max31785_data *driver_data = to_max31785_data(info);
>   
>   	switch (reg) {
>   	case PMBUS_VOUT_MODE:
>   		return -ENOTSUPP;
>   	case PMBUS_FAN_CONFIG_12:
> -		return pmbus_read_byte_data(client, page - MAX31785_NR_PAGES,
> -					    reg);
> +		return max31785_pmbus_read_byte_data(client, driver_data,
> +						     page - MAX31785_NR_PAGES,
> +						     reg);
>   	}
>   
>   	return -ENODATA;
> @@ -102,16 +182,19 @@ static int max31785_get_pwm(struct i2c_client *client, int page)
>   	return rv;
>   }
>   
> -static int max31785_get_pwm_mode(struct i2c_client *client, int page)
> +static int max31785_get_pwm_mode(struct i2c_client *client,
> +                                 struct max31785_data *driver_data, int page)
>   {
>   	int config;
>   	int command;
>   
> -	config = pmbus_read_byte_data(client, page, PMBUS_FAN_CONFIG_12);
> +	config = max31785_pmbus_read_byte_data(client, driver_data, page,
> +                                               PMBUS_FAN_CONFIG_12);
>   	if (config < 0)
>   		return config;
>   
> -	command = pmbus_read_word_data(client, page, 0xff, PMBUS_FAN_COMMAND_1);
> +	command = max31785_pmbus_read_word_data(client, driver_data, page, 0xff,
> +                                                PMBUS_FAN_COMMAND_1);
>   	if (command < 0)
>   		return command;
>   
> @@ -129,6 +212,8 @@ static int max31785_get_pwm_mode(struct i2c_client *client, int page)
>   static int max31785_read_word_data(struct i2c_client *client, int page,
>   				   int phase, int reg)
>   {
> +	const struct pmbus_driver_info *info = pmbus_get_driver_info(client);
> +	struct max31785_data *driver_data = to_max31785_data(info);
>   	u32 val;
>   	int rv;
>   
> @@ -157,7 +242,7 @@ static int max31785_read_word_data(struct i2c_client *client, int page,
>   		rv = max31785_get_pwm(client, page);
>   		break;
>   	case PMBUS_VIRT_PWM_ENABLE_1:
> -		rv = max31785_get_pwm_mode(client, page);
> +		rv = max31785_get_pwm_mode(client, driver_data, page);
>   		break;
>   	default:
>   		rv = -ENODATA;
> @@ -188,8 +273,36 @@ static inline u32 max31785_scale_pwm(u32 sensor_val)
>   	return (sensor_val * 100) / 255;
>   }
>   
> -static int max31785_pwm_enable(struct i2c_client *client, int page,
> -				    u16 word)
> +static int max31785_update_fan(struct i2c_client *client,
> +			       struct max31785_data *driver_data, int page,
> +			       u8 config, u8 mask, u16 command)
> +{
> +	int from, rv;
> +	u8 to;
> +
> +	from = max31785_pmbus_read_byte_data(client, driver_data, page,
> +					     PMBUS_FAN_CONFIG_12);
> +	if (from < 0)
> +		return from;
> +
> +	to = (from & ~mask) | (config & mask);
> +
> +	if (to != from) {
> +		rv = max31785_pmbus_write_byte_data(client, driver_data, page,
> +						    PMBUS_FAN_CONFIG_12, to);
> +		if (rv < 0)
> +			return rv;
> +	}
> +
> +	rv = max31785_pmbus_write_word_data(client, driver_data, page,
> +					    PMBUS_FAN_COMMAND_1, command);
> +
> +	return rv;
> +}
> +
> +static int max31785_pwm_enable(struct i2c_client *client,
> +			       struct max31785_data *driver_data, int page,
> +			       u16 word)
>   {
>   	int config = 0;
>   	int rate;
> @@ -217,18 +330,23 @@ static int max31785_pwm_enable(struct i2c_client *client, int page,
>   		return -EINVAL;
>   	}
>   
> -	return pmbus_update_fan(client, page, 0, config, PB_FAN_1_RPM, rate);
> +	return max31785_update_fan(client, driver_data, page, config,
> +                                   PB_FAN_1_RPM, rate);
>   }
>   
>   static int max31785_write_word_data(struct i2c_client *client, int page,
>   				    int reg, u16 word)
>   {
> +	const struct pmbus_driver_info *info = pmbus_get_driver_info(client);
> +	struct max31785_data *driver_data = to_max31785_data(info);
> +
>   	switch (reg) {
>   	case PMBUS_VIRT_PWM_1:
> -		return pmbus_update_fan(client, page, 0, 0, PB_FAN_1_RPM,
> -					max31785_scale_pwm(word));
> +		return max31785_update_fan(client, driver_data, page, 0,
> +					   PB_FAN_1_RPM,
> +					   max31785_scale_pwm(word));
>   	case PMBUS_VIRT_PWM_ENABLE_1:
> -		return max31785_pwm_enable(client, page, word);
> +		return max31785_pwm_enable(client, driver_data, page, word);
>   	default:
>   		break;
>   	}
> @@ -303,13 +421,16 @@ static int max31785_configure_dual_tach(struct i2c_client *client,
>   {
>   	int ret;
>   	int i;
> +	struct max31785_data *driver_data = to_max31785_data(info);
>   
>   	for (i = 0; i < MAX31785_NR_FAN_PAGES; i++) {
> -		ret = i2c_smbus_write_byte_data(client, PMBUS_PAGE, i);
> +		ret = max31785_i2c_smbus_write_byte_data(client, driver_data,
> +							 PMBUS_PAGE, i);
>   		if (ret < 0)
>   			return ret;
>   
> -		ret = i2c_smbus_read_word_data(client, MFR_FAN_CONFIG);
> +		ret = max31785_i2c_smbus_read_word_data(client, driver_data,
> +                                                        MFR_FAN_CONFIG);
>   		if (ret < 0)
>   			return ret;
>   
> @@ -329,6 +450,7 @@ static int max31785_probe(struct i2c_client *client)
>   {
>   	struct device *dev = &client->dev;
>   	struct pmbus_driver_info *info;
> +	struct max31785_data *driver_data;
>   	bool dual_tach = false;
>   	int ret;
>   
> @@ -337,13 +459,16 @@ static int max31785_probe(struct i2c_client *client)
>   				     I2C_FUNC_SMBUS_WORD_DATA))
>   		return -ENODEV;
>   
> -	info = devm_kzalloc(dev, sizeof(struct pmbus_driver_info), GFP_KERNEL);
> -	if (!info)
> +	driver_data = devm_kzalloc(dev, sizeof(struct max31785_data), GFP_KERNEL);
> +	if (!driver_data)
>   		return -ENOMEM;
>   
> +	info = &driver_data->info;
> +	driver_data->access = ktime_get();
>   	*info = max31785_info;
>   
> -	ret = i2c_smbus_write_byte_data(client, PMBUS_PAGE, 255);
> +	ret = max31785_i2c_smbus_write_byte_data(client,driver_data,
> +						 PMBUS_PAGE, 255);
>   	if (ret < 0)
>   		return ret;
>