drivers/hwmon/pmbus/pmbus.h | 6 ++++++ drivers/hwmon/pmbus/pmbus_core.c | 12 +++++++++++- 2 files changed, 17 insertions(+), 1 deletion(-)
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(-)
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;
---
base-commit: c763c43396883456ef57e5e78b64d3c259c4babc
change-id: 20240905-pmbus-status-reg-clearing-abc9c0184c3b
Best regards,
--
Patryk Biel <pbiel7@gmail.com>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
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
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
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
© 2016 - 2024 Red Hat, Inc.