Hi Guenter,
On Wed, 31 Jan 2024 at 03:30, Guenter Roeck <linux@xxxxxxxxxxxx> wrote:
>
> On Tue, Jan 30, 2024 at 11:21:39PM +0530, Naresh Solanki wrote:
> > From: Patrick Rudolph <patrick.rudolph@xxxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxxx>
> > Signed-off-by: Naresh Solanki <naresh.solanki@xxxxxxxxxxxxx>
> > ---
> > 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
