* [PATCH] hwmon: (pmbus/mp2975) Fix IRQ masking
@ 2024-01-30 17:51 Naresh Solanki
2024-01-30 21:59 ` Guenter Roeck
0 siblings, 1 reply; 3+ messages in thread
From: Naresh Solanki @ 2024-01-30 17:51 UTC (permalink / raw)
To: Guenter Roeck, Jean Delvare
Cc: mazziesaccount, Patrick Rudolph, Naresh Solanki, linux-hwmon,
linux-kernel
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(+)
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,
base-commit: 909d8d33f8b4664c9b6c7fd585114921af77fc2b
--
2.42.0
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH] hwmon: (pmbus/mp2975) Fix IRQ masking
2024-01-30 17:51 [PATCH] hwmon: (pmbus/mp2975) Fix IRQ masking Naresh Solanki
@ 2024-01-30 21:59 ` Guenter Roeck
2024-02-13 13:42 ` Naresh Solanki
0 siblings, 1 reply; 3+ messages in thread
From: Guenter Roeck @ 2024-01-30 21:59 UTC (permalink / raw)
To: Naresh Solanki
Cc: Jean Delvare, mazziesaccount, Patrick Rudolph, linux-hwmon, linux-kernel
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
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH] hwmon: (pmbus/mp2975) Fix IRQ masking
2024-01-30 21:59 ` Guenter Roeck
@ 2024-02-13 13:42 ` Naresh Solanki
0 siblings, 0 replies; 3+ messages in thread
From: Naresh Solanki @ 2024-02-13 13:42 UTC (permalink / raw)
To: Guenter Roeck
Cc: Jean Delvare, mazziesaccount, Patrick Rudolph, linux-hwmon, linux-kernel
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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2024-02-13 13:42 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-01-30 17:51 [PATCH] hwmon: (pmbus/mp2975) Fix IRQ masking Naresh Solanki
2024-01-30 21:59 ` Guenter Roeck
2024-02-13 13:42 ` Naresh Solanki
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®