| Message ID | 20240909-pmbus-status-reg-clearing-v1-1-f1c0d68c6408@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | hwmon: Conditionally clear individual status bits for pmbus rev >= 1.2 | expand |
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@gmail.com> > --- > 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. > Most of the above should have been in the description, and "This change adds" should be "Add ...". See the "submitting Patches" document for rationale. > + ret = i2c_smbus_read_byte_data(client, PMBUS_REVISION); > + if (ret > 0) >= 0 for consistency > + data->revision = ret; > + The code needs to be be further up, ahead of clearing faults, to have the faults cleared if the command failed. Never mind, though, I made those changes and applied the patch (I want to make sure this patch is upstream in v6.11 final). Thanks, Guenter
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@gmail.com> > --- > 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
Hi Patryk, kernel test robot noticed the following build warnings: [auto build test WARNING on c763c43396883456ef57e5e78b64d3c259c4babc] url: https://github.com/intel-lab-lkp/linux/commits/Patryk-Biel/hwmon-Conditionally-clear-individual-status-bits-for-pmbus-rev-1-2/20240909-173838 base: c763c43396883456ef57e5e78b64d3c259c4babc patch link: https://lore.kernel.org/r/20240909-pmbus-status-reg-clearing-v1-1-f1c0d68c6408%40gmail.com patch subject: [PATCH] hwmon: Conditionally clear individual status bits for pmbus rev >= 1.2 config: x86_64-rhel-8.3-rust (https://download.01.org/0day-ci/archive/20240912/202409122210.audc5WGS-lkp@intel.com/config) compiler: clang version 18.1.8 (https://github.com/llvm/llvm-project 3b5b5c1ec4a3095ab096dd780e84d7ab81f3d7ff) reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240912/202409122210.audc5WGS-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202409122210.audc5WGS-lkp@intel.com/ All warnings (new ones prefixed by >>): >> drivers/hwmon/pmbus/pmbus_core.c:1100:7: warning: variable 'ret' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized] 1100 | if (data->revision >= PMBUS_REV_12) | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ drivers/hwmon/pmbus/pmbus_core.c:1105:7: note: uninitialized use occurs here 1105 | if (ret) | ^~~ drivers/hwmon/pmbus/pmbus_core.c:1100:3: note: remove the 'if' if its condition is always true 1100 | if (data->revision >= PMBUS_REV_12) | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 1101 | ret = _pmbus_write_byte_data(client, page, reg, regval); | ~ 1102 | else | ~~~~ 1103 | pmbus_clear_fault_page(client, page); | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ drivers/hwmon/pmbus/pmbus_core.c:1083:9: note: initialize the variable 'ret' to silence this warning 1083 | int ret, status; | ^ | = 0 1 warning generated. vim +1100 drivers/hwmon/pmbus/pmbus_core.c 1050 1051 /* 1052 * Return boolean calculated from converted data. 1053 * <index> defines a status register index and mask. 1054 * The mask is in the lower 8 bits, the register index is in bits 8..23. 1055 * 1056 * The associated pmbus_boolean structure contains optional pointers to two 1057 * sensor attributes. If specified, those attributes are compared against each 1058 * other to determine if a limit has been exceeded. 1059 * 1060 * If the sensor attribute pointers are NULL, the function returns true if 1061 * (status[reg] & mask) is true. 1062 * 1063 * If sensor attribute pointers are provided, a comparison against a specified 1064 * limit has to be performed to determine the boolean result. 1065 * In this case, the function returns true if v1 >= v2 (where v1 and v2 are 1066 * sensor values referenced by sensor attribute pointers s1 and s2). 1067 * 1068 * To determine if an object exceeds upper limits, specify <s1,s2> = <v,limit>. 1069 * To determine if an object exceeds lower limits, specify <s1,s2> = <limit,v>. 1070 * 1071 * If a negative value is stored in any of the referenced registers, this value 1072 * reflects an error code which will be returned. 1073 */ 1074 static int pmbus_get_boolean(struct i2c_client *client, struct pmbus_boolean *b, 1075 int index) 1076 { 1077 struct pmbus_data *data = i2c_get_clientdata(client); 1078 struct pmbus_sensor *s1 = b->s1; 1079 struct pmbus_sensor *s2 = b->s2; 1080 u16 mask = pb_index_to_mask(index); 1081 u8 page = pb_index_to_page(index); 1082 u16 reg = pb_index_to_reg(index); 1083 int ret, status; 1084 u16 regval; 1085 1086 mutex_lock(&data->update_lock); 1087 status = pmbus_get_status(client, page, reg); 1088 if (status < 0) { 1089 ret = status; 1090 goto unlock; 1091 } 1092 1093 if (s1) 1094 pmbus_update_sensor_data(client, s1); 1095 if (s2) 1096 pmbus_update_sensor_data(client, s2); 1097 1098 regval = status & mask; 1099 if (regval) { > 1100 if (data->revision >= PMBUS_REV_12) 1101 ret = _pmbus_write_byte_data(client, page, reg, regval); 1102 else 1103 pmbus_clear_fault_page(client, page); 1104 1105 if (ret) 1106 goto unlock; 1107 } 1108 if (s1 && s2) { 1109 s64 v1, v2; 1110 1111 if (s1->data < 0) { 1112 ret = s1->data; 1113 goto unlock; 1114 } 1115 if (s2->data < 0) { 1116 ret = s2->data; 1117 goto unlock; 1118 } 1119 1120 v1 = pmbus_reg2data(data, s1); 1121 v2 = pmbus_reg2data(data, s2); 1122 ret = !!(regval && v1 >= v2); 1123 } else { 1124 ret = !!regval; 1125 } 1126 unlock: 1127 mutex_unlock(&data->update_lock); 1128 return ret; 1129 } 1130
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; } @@ -2653,6 +2659,10 @@ static int pmbus_init_common(struct i2c_client *client, struct pmbus_data *data, } } + ret = i2c_smbus_read_byte_data(client, PMBUS_REVISION); + if (ret > 0) + data->revision = ret; + if (info->pages <= 0 || info->pages > PMBUS_PAGES) { dev_err(dev, "Bad number of PMBus pages: %d\n", info->pages); return -ENODEV;
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@gmail.com> --- 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,