Message-ID: <5b095371-f835-4c23-aa43-deefa33123bb@roeck-us.net>
Date: Thu, 12 Dec 2024 17:29:51 -0800
From: Guenter Roeck <linux@...ck-us.net>
To: Ninad Palsule <ninad@...ux.ibm.com>, robh@...nel.org, krzk+dt@...nel.org,
 conor+dt@...nel.org, eajames@...ux.ibm.com, jdelvare@...e.com,
 corbet@....net, joel@....id.au, andrew@...econstruct.com.au,
 Delphine_CC_Chiu@...ynn.com, broonie@...nel.org, peteryin.openbmc@...il.com,
 noahwang.wang@...look.com, naresh.solanki@...ements.com, lukas@...ner.de,
 jbrunet@...libre.com, patrick.rudolph@...ements.com,
 gregkh@...uxfoundation.org, peterz@...radead.org, pbiel7@...il.com,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-hwmon@...r.kernel.org, linux-doc@...r.kernel.org,
 linux-arm-kernel@...ts.infradead.org, linux-aspeed@...ts.ozlabs.org,
 linux-i2c@...r.kernel.org
Subject: Re: [PATCH v1 2/4] hwmon: (pmbus/crps) Add Intel CRPS185 power supply

On 12/12/24 13:49, Ninad Palsule wrote:
> Add the driver to monitor Intel common redundant power supply (crps185)
> with hwmon over pmbus.
> 
> Signed-off-by: Ninad Palsule <ninad@...ux.ibm.com>
> ---
>   Documentation/hwmon/crps.rst |  95 +++++++++++

Documentation/hwmon/index.rst needs to be updated.

>   MAINTAINERS                  |   7 +
>   drivers/hwmon/pmbus/Kconfig  |   9 ++
>   drivers/hwmon/pmbus/Makefile |   1 +
>   drivers/hwmon/pmbus/crps.c   | 299 +++++++++++++++++++++++++++++++++++
>   5 files changed, 411 insertions(+)
>   create mode 100644 Documentation/hwmon/crps.rst
>   create mode 100644 drivers/hwmon/pmbus/crps.c
> 
> diff --git a/Documentation/hwmon/crps.rst b/Documentation/hwmon/crps.rst
> new file mode 100644
> index 000000000000..81d5dfd68a46
> --- /dev/null
> +++ b/Documentation/hwmon/crps.rst
> @@ -0,0 +1,95 @@
> +.. SPDX-License-Identifier: GPL-2.0-or-later
> +
> +Kernel driver crps
> +========================

I am quite sure that this triggers a documentation warning.

> +
> +Supported chips:
> +
> +  * Intel CRPS185
> +
> +    Prefix: 'crps185'
> +
> +    Addresses scanned: -
> +
> +Authors:
> +    Ninad Palsule <ninad@...ux.ibm.com>
> +
> +

Is the documentation available somewhere ?
If yes, please add a reference. If not, add a comment explaining that
it is not available to the public.

> +Description
> +-----------
> +
> +This driver implements support for Common Redundant Power supply with PMBus

For _Intel_ ...

> +support.
> +
> +The driver is a client driver to the core PMBus driver.
> +Please see Documentation/hwmon/pmbus.rst for details on PMBus client drivers.
> +
> +
> +Usage Notes
> +-----------
> +
> +This driver does not auto-detect devices. You will have to instantiate the
> +devices explicitly. Please see Documentation/i2c/instantiating-devices.rst for
> +details.
> +
> +
> +Sysfs entries
> +-------------
> +
> +======================= ======================================================
> +curr1_label		"iin"
> +curr1_input		Measured input current
> +curr1_max		Maximum input current
> +curr1_max_alarm         Input maximum current high alarm
> +curr1_crit              Critial high input current
> +curr1_crit_alarm        Input critical current high alarm
> +curr1_rated_max		Maximum rated input current
> +
> +curr2_label		"iout1"
> +curr2_input		Measured output current
> +curr2_max		Maximum output current
> +curr2_max_alarm         Output maximum current high alarm
> +curr2_crit	        Critial high output current
> +curr2_crit_alarm        Output critical current high alarm
> +curr2_rated_max		Maximum rated output current
> +
> +in1_label		"vin"
> +in1_input		Measured input voltage
> +in1_crit                Critical input over voltage
> +in1_crit_alarm          Critical input over voltage alarm
> +in1_max                 Maximum input over voltage
> +in1_max_alarm           Maximum input over voltage alarm
> +in1_rated_min		Minimum rated input voltage
> +in1_rated_max		Maximum rated input voltage
> +
> +in2_label		"vout1"
> +in2_input		Measured input voltage
> +in2_crit                Critical input over voltage
> +in2_crit_alarm          Critical input over voltage alarm
> +in2_lcrit               Critical input under voltage fault
> +in2_lcrit_alarm         Critical input under voltage fault alarm
> +in2_max                 Maximum input over voltage
> +in2_max_alarm           Maximum input over voltage alarm
> +in2_min                 Minimum input under voltage warning
> +in2_min_alarm           Minimum input under voltage warning alarm
> +in2_rated_min		Minimum rated input voltage
> +in2_rated_max		Maximum rated input voltage
> +
> +power1_label		"pin"
> +power1_input		Measured input power
> +power1_alarm		Input power high alarm
> +power1_max  		Maximum input power
> +power1_rated_max        Maximum rated input power
> +
> +temp[1-2]_input		Measured temperature
> +temp[1-2]_crit 		Critical temperature
> +temp[1-2]_crit_alarm    Critical temperature alarm
> +temp[1-2]_max  		Maximum temperature
> +temp[1-2]_max_alarm     Maximum temperature alarm
> +temp[1-2]_rated_max     Maximum rated temperature
> +
> +fan1_alarm		Fan 1 warning.
> +fan1_fault		Fan 1 fault.
> +fan1_input		Fan 1 speed in RPM.
> +fan1_target             Fan 1 target.
> +======================= ======================================================
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 637ddd44245f..6b31d545f0f1 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -6100,6 +6100,13 @@ L:	linux-input@...r.kernel.org
>   S:	Maintained
>   F:	drivers/hid/hid-creative-sb0540.c
>   
> +CRPS COMMON REDUNDANT PSU DRIVER

This is _INTEL_ CRPS.

> +M:	Ninad Palsule <ninad@...ux.ibm.com>
> +L:	linux-hwmon@...r.kernel.org
> +S:	Maintained
> +F:	Documentation/hwmon/crps.rst
> +F:	drivers/hwmon/pmbus/crps.c
> +
>   CRYPTO API
>   M:	Herbert Xu <herbert@...dor.apana.org.au>
>   M:	"David S. Miller" <davem@...emloft.net>
> diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig
> index 22418a05ced0..56c4eb4b846e 100644
> --- a/drivers/hwmon/pmbus/Kconfig
> +++ b/drivers/hwmon/pmbus/Kconfig
> @@ -85,6 +85,15 @@ config SENSORS_BPA_RS600
>   	  This driver can also be built as a module. If so, the module will
>   	  be called bpa-rs600.
>   
> +config SENSORS_CRPS
> +	tristate "Common Redundant Power Supply"

Again, this is an Intel product.

> +	help
> +	  If you say yes here you get hardware monitoring support for the Common
> +	  Redundant Power Supply.
> +
> +	  This driver can also be built as a module. If so, the module will
> +	  be called crps.
> +
>   config SENSORS_DELTA_AHE50DC_FAN
>   	tristate "Delta AHE-50DC fan control module"
>   	help
> diff --git a/drivers/hwmon/pmbus/Makefile b/drivers/hwmon/pmbus/Makefile
> index 3d3183f8d2a7..c7eb7739b7f8 100644
> --- a/drivers/hwmon/pmbus/Makefile
> +++ b/drivers/hwmon/pmbus/Makefile
> @@ -62,3 +62,4 @@ obj-$(CONFIG_SENSORS_XDPE122)	+= xdpe12284.o
>   obj-$(CONFIG_SENSORS_XDPE152)	+= xdpe152c4.o
>   obj-$(CONFIG_SENSORS_ZL6100)	+= zl6100.o
>   obj-$(CONFIG_SENSORS_PIM4328)	+= pim4328.o
> +obj-$(CONFIG_SENSORS_CRPS)	+= crps.o
> diff --git a/drivers/hwmon/pmbus/crps.c b/drivers/hwmon/pmbus/crps.c
> new file mode 100644
> index 000000000000..44d309f81803
> --- /dev/null
> +++ b/drivers/hwmon/pmbus/crps.c
> @@ -0,0 +1,299 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright 2024 IBM Corp.
> + */
> +
> +#include <linux/debugfs.h>
> +#include <linux/i2c.h>
> +#include <linux/of.h>
> +#include <linux/pmbus.h>
> +
> +#include "pmbus.h"
> +
> +/* Intel crps185 specific commands. */
> +#define CRPS185_MFR_IOUT_MAX		0xA6
> +#define CRPS185_MFR_POUT_MAX		0xA7
> +

I fail to see the point in those defines, even more so since
PMBUS_MFR_POUT_MAX and PMBUS_MFR_IOUT_MAX are used below.

> +enum {
> +	CRPS_DEBUGFS_PMBUS_REVISION = 0,
> +	CRPS_DEBUGFS_MAX_POWER_OUT,
> +	CRPS_DEBUGFS_MAX_CURRENT_OUT,
> +	CRPS_DEBUGFS_NUM_ENTRIES
> +};
> +
> +enum models { crps185 = 1, crps_unknown };

Pointless enum. The driver only supports a single power supply.

> +
> +struct crps {
> +	enum models version;
> +	struct i2c_client *client;
> +
> +	int debugfs_entries[CRPS_DEBUGFS_NUM_ENTRIES];
> +};
> +
> +#define to_psu(x, y) container_of((x), struct crps, debugfs_entries[(y)])
> +
> +static struct pmbus_platform_data crps_pdata = {
> +	.flags = PMBUS_SKIP_STATUS_CHECK,
> +};

Did you confirm that it is needed ? If so, explain why it is needed.

> +
> +static const struct i2c_device_id crps_id[] = {
> +	{ "intel_crps185", crps185 },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(i2c, crps_id);
> +
> +/*
> + * Convert linear format word to machine format. 11 LSB side bits are two's
> + * complement integer mantissa and 5 MSB side bits are two's complement
> + * exponent
> + */
> +static int crps_convert_linear(int rc)
> +{
> +	s16 exponent;
> +	s32 mantissa;
> +	s64 val;
> +
> +	exponent = ((s16)rc) >> 11;
> +	mantissa = ((s16)((rc & 0x7ff) << 5)) >> 5;
> +
> +	val = mantissa;
> +	if (exponent >= 0)
> +		val <<= exponent;
> +	else
> +		val >>= -exponent;
> +
> +	return (int)val;
> +}
> +
> +static ssize_t crps_debugfs_read(struct file *file, char __user *buf,
> +				 size_t count, loff_t *ppos)
> +{
> +	int rc;
> +	int *idxp = file->private_data;
> +	int idx = *idxp;
> +	struct crps *psu = to_psu(idxp, idx);
> +	char data[2 * I2C_SMBUS_BLOCK_MAX] = { 0 };
> +
> +	rc = pmbus_lock_interruptible(psu->client);
> +	if (rc)
> +		return rc;
> +
> +	rc = pmbus_set_page(psu->client, 0, 0xff);
> +	if (rc)
> +		goto unlock;
> +
> +	switch (idx) {
> +	case CRPS_DEBUGFS_PMBUS_REVISION:
> +		rc = i2c_smbus_read_byte_data(psu->client, PMBUS_REVISION);
> +		if (rc >= 0) {
> +			if (psu->version == crps185) {
> +				if (rc == 0)
> +					rc = sprintf(data, "%s", "1.0");
> +				else if (rc == 0x11)
> +					rc = sprintf(data, "%s", "1.1");
> +				else if (rc == 0x22)
> +					rc = sprintf(data, "%s", "1.2");
> +				else
> +					rc = snprintf(data, 3, "0x%02x", rc);
> +			} else {
> +				rc = snprintf(data, 3, "%02x", rc);
> +			}
> +		}

This attribute should be added into the PMBus core.

> +		break;
> +	case CRPS_DEBUGFS_MAX_POWER_OUT:
> +		rc = i2c_smbus_read_word_data(psu->client, PMBUS_MFR_POUT_MAX);
> +		if (rc >= 0) {
> +			rc = crps_convert_linear(rc);
> +			rc = snprintf(data, I2C_SMBUS_BLOCK_MAX, "%d", rc);
> +		}
> +		break;
> +	case CRPS_DEBUGFS_MAX_CURRENT_OUT:
> +		rc = i2c_smbus_read_word_data(psu->client, PMBUS_MFR_IOUT_MAX);
> +		if (rc >= 0) {
> +			rc = crps_convert_linear(rc);
> +			rc = snprintf(data, I2C_SMBUS_BLOCK_MAX, "%d", rc);
> +		}
> +		break;

What is the point of those two attributes ? There are already
standard sysfs attributes reporting those values.

> +	default:
> +		rc = -EINVAL;
> +		break;
> +	}
> +
> +unlock:
> +	pmbus_unlock(psu->client);
> +	if (rc < 0)
> +		return rc;
> +
> +	data[rc] = '\n';
> +	rc += 2;
> +
> +	return simple_read_from_buffer(buf, count, ppos, data, rc);
> +}
> +
> +static const struct file_operations crps_debugfs_fops = {
> +	.llseek = noop_llseek,
> +	.read = crps_debugfs_read,
> +	.open = simple_open,
> +};
> +
> +static int crps_read_word_data(struct i2c_client *client, int page,
> +				int phase, int reg)
> +{
> +	int rc;
> +
> +	switch (reg) {
> +	case PMBUS_STATUS_WORD:
> +		rc = pmbus_read_word_data(client, page, phase, reg);
> +		if (rc < 0)
> +			return rc;
> +		break;

Why is this needed ?

> +	case PMBUS_OT_WARN_LIMIT:
> +		rc = pmbus_read_word_data(client, page, phase,
> +					  PMBUS_MFR_MAX_TEMP_1);
> +		if (rc < 0)
> +			return rc;
> +		break;
> +	case PMBUS_IOUT_OC_WARN_LIMIT:
> +		rc = pmbus_read_word_data(client, page, phase,
> +					  CRPS185_MFR_IOUT_MAX);
> +		if (rc < 0)
> +			return rc;
> +		break;
> +	case PMBUS_POUT_OP_WARN_LIMIT:
> +		rc = pmbus_read_word_data(client, page, phase,
> +					  CRPS185_MFR_POUT_MAX);
> +		if (rc < 0)
> +			return rc;
> +		break;

The above three values are more than odd. They duplicate the respective
standard rated_max attributes as warning limits. Why ?

On top of that, on writes, the actual warning limits are overwritten.
That makes even less sense.

> +	default:
> +		rc = -ENODATA;
> +		break;
> +	}
> +
> +	return rc;
> +}
> +
> +static struct pmbus_driver_info crps_info[] = {
> +	[crps185] = {
> +		.pages = 1,
> +		/* PSU uses default linear data format. */
> +		.func[0] = PMBUS_HAVE_PIN | PMBUS_HAVE_IOUT |
> +			PMBUS_HAVE_STATUS_IOUT | PMBUS_HAVE_IIN |
> +			PMBUS_HAVE_VIN | PMBUS_HAVE_STATUS_INPUT |
> +			PMBUS_HAVE_VOUT | PMBUS_HAVE_STATUS_VOUT |
> +			PMBUS_HAVE_TEMP | PMBUS_HAVE_TEMP2 |
> +			PMBUS_HAVE_STATUS_TEMP |
> +			PMBUS_HAVE_FAN12 | PMBUS_HAVE_STATUS_FAN12,
> +		.read_word_data = crps_read_word_data,
> +	},
> +};
> +
> +#define to_psu(x, y) container_of((x), struct crps, debugfs_entries[(y)])
> +
> +static void crps_init_debugfs(struct crps *psu)
> +{
> +	struct i2c_client *client = psu->client;
> +	struct dentry *debugfs;
> +	int i;
> +
> +	/* Don't fail the probe if we can't create debugfs */
> +	debugfs = pmbus_get_debugfs_dir(client);
> +	if (!debugfs)
> +		return;
> +
> +	for (i = 0; i < CRPS_DEBUGFS_NUM_ENTRIES; ++i)
> +		psu->debugfs_entries[i] = i;
> +
> +	debugfs_create_file("pmbus_revision", 0444, debugfs,
> +			    &psu->debugfs_entries[CRPS_DEBUGFS_PMBUS_REVISION],
> +			    &crps_debugfs_fops);
> +	debugfs_create_file("max_power_out", 0444, debugfs,
> +			    &psu->debugfs_entries[CRPS_DEBUGFS_MAX_POWER_OUT],
> +			    &crps_debugfs_fops);
> +	debugfs_create_file("max_current_out", 0444, debugfs,
> +			    &psu->debugfs_entries[CRPS_DEBUGFS_MAX_CURRENT_OUT],
> +			    &crps_debugfs_fops);
> +}
> +
> +static int crps_probe(struct i2c_client *client)
> +{
> +	int rc;
> +	struct device *dev = &client->dev;
> +	enum models vs = crps_unknown;
> +	struct crps *psu;
> +	const void *md = of_device_get_match_data(&client->dev);
> +	const struct i2c_device_id *id = NULL;
> +	char buf[I2C_SMBUS_BLOCK_MAX + 2] = { 0 };
> +
> +	if (md) {
> +		vs = (uintptr_t)md;
> +	} else {
> +		id = i2c_match_id(crps_id, client);
> +		if (id)
> +			vs = (enum models)id->driver_data;
> +	}
> +
Consider using i2c_get_match_data().

> +	if (!vs || vs >= crps_unknown) {
> +		dev_err(dev, "Version %d not supported\n", vs);
> +		return -EINVAL;
> +	}

This is very much pointless. The driver would not be instantiated withut match.

> +
> +	rc = i2c_smbus_read_block_data(client, PMBUS_MFR_MODEL, buf);
> +	if (rc < 0) {
> +		dev_err(dev, "Failed to read PMBUS_MFR_MODEL\n");
> +		return rc;

dev_err_probe().

> +	}
> +	if (strncmp(buf, "03NK260", 7)) {

This should also check and ensure that rc == 7.

> +		buf[rc] = '\0';
> +		dev_err(dev, "Model '%s' not supported\n", buf);

dev_err_probe()

> +		return -ENODEV;
> +	}
> +
> +	client->dev.platform_data = &crps_pdata;
> +	rc = pmbus_do_probe(client, &crps_info[vs]);
> +	if (rc) {
> +		dev_err(dev, "Failed to probe %d\n", rc);
> +		return rc;

dev_err_probe().

> +	}
> +
> +	/*
> +	 * Don't fail the probe if there isn't enough memory for debugfs.
> +	 */
> +	psu = devm_kzalloc(&client->dev, sizeof(*psu), GFP_KERNEL);
> +	if (!psu) {
> +		dev_warn(dev, "Failed to allocate memory. debugfs are not supported.\n");
> +		return 0;
> +	}
> +
> +	psu->version = vs;
> +	psu->client = client;
> +

Drop all this. Add the PMBus version as standard debugfs attribute to the PMBus core
if needed/wanted. The rest is already provided as standard sysfs attributes, and
reporting the same value in debugfs files adds no value.

> +	crps_init_debugfs(psu);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id crps_of_match[] = {
> +	{
> +		.compatible = "intel,crps185",
> +		.data = (void *)crps185
> +	},
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, crps_of_match);
> +
> +static struct i2c_driver crps_driver = {
> +	.driver = {
> +		.name = "crps",
> +		.of_match_table = crps_of_match,
> +	},
> +	.probe = crps_probe,
> +	.id_table = crps_id,
> +};
> +
> +module_i2c_driver(crps_driver);
> +
> +MODULE_AUTHOR("Ninad Palsule");
> +MODULE_DESCRIPTION("PMBus driver for Common Redundant power supplies");

Again, this is for _Intel_ power supplies.

> +MODULE_LICENSE("GPL");
> +MODULE_IMPORT_NS("PMBUS");

Powered by blists - more mailing lists