Received-SPF: pass (google.com: domain of linux-kernel+bounces-63606-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Precedence: bulk
MIME-Version: 1.0
References: <20240130175140.3834889-1-naresh.solanki@9elements.com> <cd84b051-1608-48c0-8335-b038cd236f61@roeck-us.net>
In-Reply-To: <cd84b051-1608-48c0-8335-b038cd236f61@roeck-us.net>
From: Naresh Solanki <naresh.solanki@9elements.com>
Date: Tue, 13 Feb 2024 19:12:19 +0530
Message-ID: <CABqG17jbOgAHWUemV5=VFKwufuGX_xyheTDETfJtvoOO9UWjvg@mail.gmail.com>
Subject: Re: [PATCH] hwmon: (pmbus/mp2975) Fix IRQ masking
To: Guenter Roeck <linux@roeck-us.net>
Cc: Jean Delvare <jdelvare@suse.com>, mazziesaccount@gmail.com, 
	Patrick Rudolph <patrick.rudolph@9elements.com>, linux-hwmon@vger.kernel.org, 
	linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"

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