| Message ID | 20240130175140.3834889-1-naresh.solanki@9elements.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | hwmon: (pmbus/mp2975) Fix IRQ masking | expand |
On Tue, Jan 30, 2024 at 11:21:39PM +0530, Naresh Solanki wrote: > From: Patrick Rudolph <patrick.rudolph@9elements.com> > > The MP2971/MP2973 use a custom 16bit register format for > SMBALERT_MASK which doesn't follow the PMBUS specification. > > Map the PMBUS defined bits used by the common code onto the custom > format used by MPS and since the SMBALERT_MASK is currently never read > by common code only implement the mapping for write transactions. > > Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com> > Signed-off-by: Naresh Solanki <naresh.solanki@9elements.com> > --- > drivers/hwmon/pmbus/mp2975.c | 57 ++++++++++++++++++++++++++++++++++++ > 1 file changed, 57 insertions(+) > > > base-commit: 909d8d33f8b4664c9b6c7fd585114921af77fc2b > > diff --git a/drivers/hwmon/pmbus/mp2975.c b/drivers/hwmon/pmbus/mp2975.c > index b9bb469e2d8f..788ec2c5a45f 100644 > --- a/drivers/hwmon/pmbus/mp2975.c > +++ b/drivers/hwmon/pmbus/mp2975.c > @@ -377,6 +377,62 @@ static int mp2973_read_word_data(struct i2c_client *client, int page, > return ret; > } > > +static int mp2973_write_word_data(struct i2c_client *client, int page, > + int reg, u16 word) > +{ > + u8 target, mask; > + int ret; > + > + if (reg != PMBUS_SMBALERT_MASK) > + return -ENODATA; > + > + /* > + * Vendor-specific SMBALERT_MASK register with 16 maskable bits. > + */ > + ret = pmbus_read_word_data(client, 0, 0, PMBUS_SMBALERT_MASK); > + if (ret < 0) > + return ret; > + > + target = word & 0xff; > + mask = word >> 8; > + > +#define SWAP(cond, bit) (ret = (cond) ? (ret & ~BIT(bit)) : (ret | BIT(bit))) This isn't really a "SWAP", but setting or clearing of bits in "ret" depending on a bit set in "cond". I don't have a good idea for a better name, but either case I think a comment describing what it does would be useful. "ret" use is implied, but "mask" is always provided as parameter. Please either provide both as arguments, or make both implied. Also, the first parameter is a bit mask, while the second parameter is a bit position. Please used defines for the second parameter and make it a mask as well. > + switch (target) { > + case PMBUS_STATUS_CML: > + SWAP(mask & PB_CML_FAULT_INVALID_DATA, 8); > + SWAP(mask & PB_CML_FAULT_INVALID_COMMAND, 9); > + SWAP(mask & PB_CML_FAULT_OTHER_COMM, 5); > + SWAP(mask & PB_CML_FAULT_PACKET_ERROR, 7); > + break; > + case PMBUS_STATUS_VOUT: > + SWAP(mask & PB_VOLTAGE_UV_FAULT, 13); > + SWAP(mask & PB_VOLTAGE_OV_FAULT, 14); > + break; > + case PMBUS_STATUS_IOUT: > + SWAP(mask & PB_IOUT_OC_FAULT, 11); > + SWAP(mask & PB_IOUT_OC_LV_FAULT, 10); > + break; > + case PMBUS_STATUS_TEMPERATURE: > + SWAP(mask & PB_TEMP_OT_FAULT, 0); > + break; > + /* > + * Map remaining bits to MFR specific to let the PMBUS core mask > + * those bits by default. > + */ > + case PMBUS_STATUS_MFR_SPECIFIC: > + SWAP(mask & BIT(1), 1); > + SWAP(mask & BIT(3), 3); > + SWAP(mask & BIT(4), 4); > + SWAP(mask & BIT(6), 6); > + break; Coming back to using defines for the second parameter: The above bit positions appear to be purely random. Having defines for those bits will at least explain what is being masked (and hopefully explain why bit 2, 12, and 15 are not covered at all). For example, at least one other chip from the same vendor defines bit 6 as CRC_ERROR, and the matching status register bit is bit 4 (memory fault detected) in STATUS_CML. Also, it is unclear why the chip would not issue any alerts when warning limits are exceeded. Without knowing what the bits in SMBALERT_MASK mean it is impossible to validate if the above is correct and/or complete. Thanks, Guenter
Hi Guenter, On Wed, 31 Jan 2024 at 03:30, Guenter Roeck <linux@roeck-us.net> wrote: > > On Tue, Jan 30, 2024 at 11:21:39PM +0530, Naresh Solanki wrote: > > From: Patrick Rudolph <patrick.rudolph@9elements.com> > > > > The MP2971/MP2973 use a custom 16bit register format for > > SMBALERT_MASK which doesn't follow the PMBUS specification. > > > > Map the PMBUS defined bits used by the common code onto the custom > > format used by MPS and since the SMBALERT_MASK is currently never read > > by common code only implement the mapping for write transactions. > > > > Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com> > > Signed-off-by: Naresh Solanki <naresh.solanki@9elements.com> > > --- > > drivers/hwmon/pmbus/mp2975.c | 57 ++++++++++++++++++++++++++++++++++++ > > 1 file changed, 57 insertions(+) > > > > > > base-commit: 909d8d33f8b4664c9b6c7fd585114921af77fc2b > > > > diff --git a/drivers/hwmon/pmbus/mp2975.c b/drivers/hwmon/pmbus/mp2975.c > > index b9bb469e2d8f..788ec2c5a45f 100644 > > --- a/drivers/hwmon/pmbus/mp2975.c > > +++ b/drivers/hwmon/pmbus/mp2975.c > > @@ -377,6 +377,62 @@ static int mp2973_read_word_data(struct i2c_client *client, int page, > > return ret; > > } > > > > +static int mp2973_write_word_data(struct i2c_client *client, int page, > > + int reg, u16 word) > > +{ > > + u8 target, mask; > > + int ret; > > + > > + if (reg != PMBUS_SMBALERT_MASK) > > + return -ENODATA; > > + > > + /* > > + * Vendor-specific SMBALERT_MASK register with 16 maskable bits. > > + */ > > + ret = pmbus_read_word_data(client, 0, 0, PMBUS_SMBALERT_MASK); > > + if (ret < 0) > > + return ret; > > + > > + target = word & 0xff; > > + mask = word >> 8; > > + > > +#define SWAP(cond, bit) (ret = (cond) ? (ret & ~BIT(bit)) : (ret | BIT(bit))) > > This isn't really a "SWAP", but setting or clearing of bits in "ret" > depending on a bit set in "cond". I don't have a good idea for a > better name, but either case I think a comment describing what it > does would be useful. Yes. will add below comment /* * Set/Clear 'bit' in 'ret' based on condition */ > > "ret" use is implied, but "mask" is always provided as parameter. > Please either provide both as arguments, or make both implied. Sure. Will update as: #define SWAP(cond, bit) ret = (mask & cond) ? (ret & ~BIT(bit)) : (ret | BIT(bit)) > > Also, the first parameter is a bit mask, while the second parameter > is a bit position. Please used defines for the second parameter > and make it a mask as well. Sure will be adding defines for second parameter as well. #define MP2973_INVALID_DATA 8 #define MP2973_INVALID_COMMAND 9 #define MP2973_OTHER_COMM 5 #define MP2973_PACKET_ERROR 7 #define MP2973_VOLTAGE_UV 13 #define MP2973_VOLTAGE_OV 14 #define MP2973_IOUT_OC 11 #define MP2973_IOUT_OC_LV 10 #define MP2973_TEMP_OT 0 > > > + switch (target) { > > + case PMBUS_STATUS_CML: > > + SWAP(mask & PB_CML_FAULT_INVALID_DATA, 8); > > + SWAP(mask & PB_CML_FAULT_INVALID_COMMAND, 9); > > + SWAP(mask & PB_CML_FAULT_OTHER_COMM, 5); > > + SWAP(mask & PB_CML_FAULT_PACKET_ERROR, 7); > > + break; > > + case PMBUS_STATUS_VOUT: > > + SWAP(mask & PB_VOLTAGE_UV_FAULT, 13); > > + SWAP(mask & PB_VOLTAGE_OV_FAULT, 14); > > + break; > > + case PMBUS_STATUS_IOUT: > > + SWAP(mask & PB_IOUT_OC_FAULT, 11); > > + SWAP(mask & PB_IOUT_OC_LV_FAULT, 10); > > + break; > > + case PMBUS_STATUS_TEMPERATURE: > > + SWAP(mask & PB_TEMP_OT_FAULT, 0); > > + break; > > + /* > > + * Map remaining bits to MFR specific to let the PMBUS core mask > > + * those bits by default. > > + */ > > + case PMBUS_STATUS_MFR_SPECIFIC: > > + SWAP(mask & BIT(1), 1); > > + SWAP(mask & BIT(3), 3); > > + SWAP(mask & BIT(4), 4); > > + SWAP(mask & BIT(6), 6); > > + break; > > Coming back to using defines for the second parameter: The > above bit positions appear to be purely random. Having defines for > those bits will at least explain what is being masked (and hopefully > explain why bit 2, 12, and 15 are not covered at all). > For example, at least one other chip from the same vendor defines > bit 6 as CRC_ERROR, and the matching status register bit is bit > 4 (memory fault detected) in STATUS_CML. Also, it is unclear why > the chip would not issue any alerts when warning limits are exceeded. > Without knowing what the bits in SMBALERT_MASK mean it is impossible > to validate if the above is correct and/or complete. Agree. Bit 2 & 15 are reserved & will add a comment to mention that. For others, I will add #define as below.. #define MP2973_VIN_UVLO 1 #define MP2973_VIN_OVP 3 #define MP2973_MTP_FAULT 4 #define MP2973_MTP_BLK_TRIG 6 #define MP2973_VOUT_MAX_MIN_WARNING 12 Regards, Naresh > > Thanks, > Guenter
diff --git a/drivers/hwmon/pmbus/mp2975.c b/drivers/hwmon/pmbus/mp2975.c index b9bb469e2d8f..788ec2c5a45f 100644 --- a/drivers/hwmon/pmbus/mp2975.c +++ b/drivers/hwmon/pmbus/mp2975.c @@ -377,6 +377,62 @@ static int mp2973_read_word_data(struct i2c_client *client, int page, return ret; } +static int mp2973_write_word_data(struct i2c_client *client, int page, + int reg, u16 word) +{ + u8 target, mask; + int ret; + + if (reg != PMBUS_SMBALERT_MASK) + return -ENODATA; + + /* + * Vendor-specific SMBALERT_MASK register with 16 maskable bits. + */ + ret = pmbus_read_word_data(client, 0, 0, PMBUS_SMBALERT_MASK); + if (ret < 0) + return ret; + + target = word & 0xff; + mask = word >> 8; + +#define SWAP(cond, bit) (ret = (cond) ? (ret & ~BIT(bit)) : (ret | BIT(bit))) + switch (target) { + case PMBUS_STATUS_CML: + SWAP(mask & PB_CML_FAULT_INVALID_DATA, 8); + SWAP(mask & PB_CML_FAULT_INVALID_COMMAND, 9); + SWAP(mask & PB_CML_FAULT_OTHER_COMM, 5); + SWAP(mask & PB_CML_FAULT_PACKET_ERROR, 7); + break; + case PMBUS_STATUS_VOUT: + SWAP(mask & PB_VOLTAGE_UV_FAULT, 13); + SWAP(mask & PB_VOLTAGE_OV_FAULT, 14); + break; + case PMBUS_STATUS_IOUT: + SWAP(mask & PB_IOUT_OC_FAULT, 11); + SWAP(mask & PB_IOUT_OC_LV_FAULT, 10); + break; + case PMBUS_STATUS_TEMPERATURE: + SWAP(mask & PB_TEMP_OT_FAULT, 0); + break; + /* + * Map remaining bits to MFR specific to let the PMBUS core mask + * those bits by default. + */ + case PMBUS_STATUS_MFR_SPECIFIC: + SWAP(mask & BIT(1), 1); + SWAP(mask & BIT(3), 3); + SWAP(mask & BIT(4), 4); + SWAP(mask & BIT(6), 6); + break; + default: + return 0; + } +#undef SWAP + + return pmbus_write_word_data(client, 0, PMBUS_SMBALERT_MASK, ret); +} + static int mp2975_read_word_data(struct i2c_client *client, int page, int phase, int reg) { @@ -891,6 +947,7 @@ static struct pmbus_driver_info mp2973_info = { PMBUS_HAVE_TEMP | PMBUS_HAVE_STATUS_TEMP | PMBUS_HAVE_POUT | PMBUS_HAVE_PIN | PMBUS_HAVE_STATUS_INPUT, .read_word_data = mp2973_read_word_data, + .write_word_data = mp2973_write_word_data, #if IS_ENABLED(CONFIG_SENSORS_MP2975_REGULATOR) .num_regulators = 1, .reg_desc = mp2975_reg_desc,