Re: [PATCH] hwmon: Conditionally clear individual status bits for pmbus rev >= 1.2

[Guenter Roeck]

On Mon, Sep 09, 2024 at 11:30:28AM +0200, Patryk Biel wrote:
> This change adds fetching PMBus revision and using it to conditionally
> clear individual status bits while calling pmbus_show_boolean, only if
> the device is compliant with PMBus specs >= 1.2.
> 
> Signed-off-by: Patryk Biel <pbiel7@xxxxxxxxx>
> ---
> Current implementation of pmbus_show_boolean assumes that all devices
> support write-back operation of status register so as to clear pending
> warning or faults. Since clearing individual bits in the status registers
> was introduced in PMBus specification 1.2, this operation may not be 
> supported by some older devices, thus resulting in error while reading 
> boolean attributes like e.g. temp1_max_alarm. 
> 
> This change adds fetching PMBus revision supported by device and
> modifies pmbus_show_boolean so that it only tries to clear individual
> status bits if the device is compilant with PMBus specs >= 1.2.
> 
> Tested on: LTC2971, LTC2971-1, LTC2974, LTC2977.
> ---
>  drivers/hwmon/pmbus/pmbus.h      |  6 ++++++
>  drivers/hwmon/pmbus/pmbus_core.c | 12 +++++++++++-
>  2 files changed, 17 insertions(+), 1 deletion(-)
> 
> 
> ---
> base-commit: c763c43396883456ef57e5e78b64d3c259c4babc
> change-id: 20240905-pmbus-status-reg-clearing-abc9c0184c3b
> 
> Best regards,
> 
> diff --git a/drivers/hwmon/pmbus/pmbus.h b/drivers/hwmon/pmbus/pmbus.h
> index fb442fae7b3e..0bea603994e7 100644
> --- a/drivers/hwmon/pmbus/pmbus.h
> +++ b/drivers/hwmon/pmbus/pmbus.h
> @@ -418,6 +418,12 @@ enum pmbus_sensor_classes {
>  enum pmbus_data_format { linear = 0, ieee754, direct, vid };
>  enum vrm_version { vr11 = 0, vr12, vr13, imvp9, amd625mv };
>  
> +/* PMBus revision identifiers */
> +#define PMBUS_REV_10 0x00	/* PMBus revision 1.0 */
> +#define PMBUS_REV_11 0x11	/* PMBus revision 1.1 */
> +#define PMBUS_REV_12 0x22	/* PMBus revision 1.2 */
> +#define PMBUS_REV_13 0x33	/* PMBus revision 1.3 */
> +
>  struct pmbus_driver_info {
>  	int pages;		/* Total number of pages */
>  	u8 phases[PMBUS_PAGES];	/* Number of phases per page */
> diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
> index cb4c65a7f288..50ba093a38e8 100644
> --- a/drivers/hwmon/pmbus/pmbus_core.c
> +++ b/drivers/hwmon/pmbus/pmbus_core.c
> @@ -108,6 +108,8 @@ struct pmbus_data {
>  
>  	int vout_low[PMBUS_PAGES];	/* voltage low margin */
>  	int vout_high[PMBUS_PAGES];	/* voltage high margin */
> +
> +	u8 revision;	/* The PMBus revision the device is compliant with */
>  };
>  
>  struct pmbus_debugfs_entry {
> @@ -1095,7 +1097,11 @@ static int pmbus_get_boolean(struct i2c_client *client, struct pmbus_boolean *b,
>  
>  	regval = status & mask;
>  	if (regval) {
> -		ret = _pmbus_write_byte_data(client, page, reg, regval);
> +		if (data->revision >= PMBUS_REV_12)
> +			ret = _pmbus_write_byte_data(client, page, reg, regval);
> +		else
> +			pmbus_clear_fault_page(client, page);
> +
>  		if (ret)
>  			goto unlock;

That check needs to be part of the if() statement above. Never mind, though,
I fixed that up.

Guenter