Re: [PATCH linux dev-5.10 06/14] pmbus: (max31785) Add a local pmbus_set_page() implementation

[Matthew Barth <msbarth@linux.ibm.com>]

On 8/11/21 10:42 AM, Eddie James wrote:
> From: Andrew Jeffery <andrew@aj.id.au>
>
> Extensive testing and tracing has shown that the MAX31785 is unreliable
> in the face of PAGE write commands, ACK'ing the PAGE request but
> reporting a value of 0 on some subsequent PAGE reads. The trace data
> suggests that a one-shot retry of the PAGE write is enough to get the
> requested value to stick.
>
> As we configure the device before registering with the PMBus core,
> centralise PAGE handling inside the driver and implement the one-shot
> retry semantics there.
>
> OpenBMC-Staging-Count: 1
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> Signed-off-by: Joel Stanley <joel@jms.id.au>
Reviewed-by: Matthew Barth <msbarth@linux.ibm.com>
> ---
>  drivers/hwmon/pmbus/max31785.c | 32 ++++++++++++++++++++++++++------
>  1 file changed, 26 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/hwmon/pmbus/max31785.c b/drivers/hwmon/pmbus/max31785.c
> index d097f72d4d47..7518fff356f9 100644
> --- a/drivers/hwmon/pmbus/max31785.c
> +++ b/drivers/hwmon/pmbus/max31785.c
> @@ -362,6 +362,27 @@ static int max31785_write_word_data(struct i2c_client *client, int page,
>  	return -ENXIO;
>  }
>  
> +static int max31785_pmbus_set_page(struct i2c_client *client, int page)
> +{
> +	int ret;
> +	int i;
> +
> +	for (i = 0; i < 2; i++) {
> +		ret = max31785_i2c_smbus_write_byte_data(client, PMBUS_PAGE, page);
> +		if (ret < 0)
> +			return ret;
> +
> +		ret = max31785_i2c_smbus_read_byte_data(client, PMBUS_PAGE);
> +		if (ret < 0)
> +			return ret;
> +
> +		if (ret == page)
> +			return 0;
> +	}
> +
> +	return -EIO;
> +}
> +
>  /*
>   * Returns negative error codes if an unrecoverable problem is detected, 0 if a
>   * recoverable problem is detected, or a positive value on success.
> @@ -392,7 +413,7 @@ static int max31785_of_fan_config(struct i2c_client *client,
>  		return -ENXIO;
>  	}
>  
> -	ret = max31785_i2c_smbus_write_byte_data(client, PMBUS_PAGE, page);
> +	ret = max31785_pmbus_set_page(client, page);
>  	if (ret < 0)
>  		return ret;
>  
> @@ -599,7 +620,7 @@ static int max31785_of_tmp_config(struct i2c_client *client,
>  		return -ENXIO;
>  	}
>  
> -	ret = max31785_i2c_smbus_write_byte_data(client, PMBUS_PAGE, page);
> +	ret = max31785_pmbus_set_page(client, page);
>  	if (ret < 0)
>  		return ret;
>  
> @@ -700,7 +721,7 @@ static int max31785_configure_dual_tach(struct i2c_client *client,
>  	int i;
>  
>  	for (i = 0; i < MAX31785_NR_FAN_PAGES; i++) {
> -		ret = max31785_i2c_smbus_write_byte_data(client, PMBUS_PAGE, i);
> +		ret = max31785_pmbus_set_page(client, i);
>  		if (ret < 0)
>  			return ret;
>  
> @@ -741,7 +762,7 @@ static int max31785_probe(struct i2c_client *client)
>  
>  	*info = max31785_info;
>  
> -	ret = max31785_i2c_smbus_write_byte_data(client, PMBUS_PAGE, 255);
> +	ret = max31785_pmbus_set_page(client, 255);
>  	if (ret < 0)
>  		return ret;
>  
> @@ -785,8 +806,7 @@ static int max31785_probe(struct i2c_client *client)
>  		if (!have_fan || fan_configured)
>  			continue;
>  
> -		ret = max31785_i2c_smbus_write_byte_data(client, PMBUS_PAGE,
> -							 i);
> +		ret = max31785_pmbus_set_page(client, i);
>  		if (ret < 0)
>  			return ret;
>