Received-SPF: pass (google.com: domain of linux-kernel+bounces-45359-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Sender: Guenter Roeck <groeck7@gmail.com>
Date: Tue, 30 Jan 2024 13:59:57 -0800
From: Guenter Roeck <linux@roeck-us.net>
To: Naresh Solanki <naresh.solanki@9elements.com>
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
Subject: Re: [PATCH] hwmon: (pmbus/mp2975) Fix IRQ masking
Message-ID: <cd84b051-1608-48c0-8335-b038cd236f61@roeck-us.net>
References: <20240130175140.3834889-1-naresh.solanki@9elements.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240130175140.3834889-1-naresh.solanki@9elements.com>

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