[PATCH v1 0/4] Add support for Intel CRPS PSU

[PATCH v1 0/4] Add support for Intel CRPS PSU

[Ninad Palsule]

Hello,

Please review the changes for Intel CRPS power supply driver.
I need to add label in the pmbus core as new application like PSUSensor
requires it.

Ninad Palsule (4):
  hwmon: pmbus-core: Add label for fan and temp
  hwmon: (pmbus/crps) Add Intel CRPS185 power supply
  dt-bindings: hwmon: intel,crps185: Add to trivial
  ARM: dts: aspeed: system1: Use crps PSU driver

 .../devicetree/bindings/trivial-devices.yaml  |   2 +
 Documentation/hwmon/crps.rst                  |  95 ++++++
 MAINTAINERS                                   |   7 +
 .../dts/aspeed/aspeed-bmc-ibm-system1.dts     |   8 +-
 drivers/hwmon/pmbus/Kconfig                   |   9 +
 drivers/hwmon/pmbus/Makefile                  |   1 +
 drivers/hwmon/pmbus/crps.c                    | 299 ++++++++++++++++++
 drivers/hwmon/pmbus/pmbus_core.c              |  14 +
 8 files changed, 431 insertions(+), 4 deletions(-)
 create mode 100644 Documentation/hwmon/crps.rst
 create mode 100644 drivers/hwmon/pmbus/crps.c

-- 
2.43.0

[PATCH v1 1/4] hwmon: pmbus-core: Add label for fan and temp

[Ninad Palsule]

Adding label files for fan and temperature sensors in the power supply.
The openbmc application dbus-sensor(psusensor) requires those files to
consfigure those sensors.
Note that prefix for temp label is temp[A..C] used instead of temp[1..3]
as dbus-sensor(psusensor) application calculate index based on last
digit in the name so we do not want to make index double digit after
appending page index.

Signed-off-by: Ninad Palsule <ninad@linux.ibm.com>
---
 drivers/hwmon/pmbus/pmbus_core.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
index a7000314e5ad..149b3c5f3a4c 100644
--- a/drivers/hwmon/pmbus/pmbus_core.c
+++ b/drivers/hwmon/pmbus/pmbus_core.c
@@ -2144,6 +2144,7 @@ static const struct pmbus_sensor_attr temp_attributes[] = {
 	{
 		.reg = PMBUS_READ_TEMPERATURE_1,
 		.class = PSC_TEMPERATURE,
+		.label = "tempA",
 		.paged = true,
 		.update = true,
 		.compare = true,
@@ -2156,6 +2157,7 @@ static const struct pmbus_sensor_attr temp_attributes[] = {
 	}, {
 		.reg = PMBUS_READ_TEMPERATURE_2,
 		.class = PSC_TEMPERATURE,
+		.label = "tempB",
 		.paged = true,
 		.update = true,
 		.compare = true,
@@ -2168,6 +2170,7 @@ static const struct pmbus_sensor_attr temp_attributes[] = {
 	}, {
 		.reg = PMBUS_READ_TEMPERATURE_3,
 		.class = PSC_TEMPERATURE,
+		.label = "tempC",
 		.paged = true,
 		.update = true,
 		.compare = true,
@@ -2282,6 +2285,17 @@ static int pmbus_add_fan_attributes(struct i2c_client *client,
 					     PSC_FAN, true, true, true) == NULL)
 				return -ENOMEM;
 
+			/*
+			 * Add fan label.
+			 * Assuming paged attributes while passing page index
+			 */
+			ret = pmbus_add_label(data, "fan", index, "fan",
+					      page + 1, 0xff);
+			if (ret) {
+				dev_err(data->dev, "Fan label add failed ret=%d\n", ret);
+				return ret;
+			}
+
 			/* Fan control */
 			if (pmbus_check_word_register(client, page,
 					pmbus_fan_command_registers[f])) {
-- 
2.43.0

[PATCH v1 2/4] hwmon: (pmbus/crps) Add Intel CRPS185 power supply

[Ninad Palsule]

Add the driver to monitor Intel common redundant power supply (crps185)
with hwmon over pmbus.

Signed-off-by: Ninad Palsule <ninad@linux.ibm.com>
---
 Documentation/hwmon/crps.rst |  95 +++++++++++
 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
+========================
+
+Supported chips:
+
+  * Intel CRPS185
+
+    Prefix: 'crps185'
+
+    Addresses scanned: -
+
+Authors:
+    Ninad Palsule <ninad@linux.ibm.com>
+
+
+Description
+-----------
+
+This driver implements support for Common Redundant Power supply with PMBus
+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@vger.kernel.org
 S:	Maintained
 F:	drivers/hid/hid-creative-sb0540.c
 
+CRPS COMMON REDUNDANT PSU DRIVER
+M:	Ninad Palsule <ninad@linux.ibm.com>
+L:	linux-hwmon@vger.kernel.org
+S:	Maintained
+F:	Documentation/hwmon/crps.rst
+F:	drivers/hwmon/pmbus/crps.c
+
 CRYPTO API
 M:	Herbert Xu <herbert@gondor.apana.org.au>
 M:	"David S. Miller" <davem@davemloft.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"
+	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
+
+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 };
+
+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,
+};
+
+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);
+			}
+		}
+		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;
+	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;
+	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;
+	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;
+	}
+
+	if (!vs || vs >= crps_unknown) {
+		dev_err(dev, "Version %d not supported\n", vs);
+		return -EINVAL;
+	}
+
+	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;
+	}
+	if (strncmp(buf, "03NK260", 7)) {
+		buf[rc] = '\0';
+		dev_err(dev, "Model '%s' not supported\n", buf);
+		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;
+	}
+
+	/*
+	 * 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;
+
+	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");
+MODULE_LICENSE("GPL");
+MODULE_IMPORT_NS("PMBUS");
-- 
2.43.0

[PATCH v1 3/4] dt-bindings: hwmon: intel,crps185: Add to trivial

[Ninad Palsule]

Add INTEL Common Redundant Power Supply Versions crps185 bindings as
trivial.

Signed-off-by: Ninad Palsule <ninad@linux.ibm.com>
---
 Documentation/devicetree/bindings/trivial-devices.yaml | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/trivial-devices.yaml b/Documentation/devicetree/bindings/trivial-devices.yaml
index 73a49d50c4ef..7d07b08b1459 100644
--- a/Documentation/devicetree/bindings/trivial-devices.yaml
+++ b/Documentation/devicetree/bindings/trivial-devices.yaml
@@ -151,6 +151,8 @@ properties:
           - injoinic,ip5306
             # Inspur Power System power supply unit version 1
           - inspur,ipsps1
+            # Intel common redudant power supply crps185
+          - intel,crps185
             # Intersil ISL29028 Ambient Light and Proximity Sensor
           - isil,isl29028
             # Intersil ISL29030 Ambient Light and Proximity Sensor
-- 
2.43.0

[PATCH v1 4/4] ARM: dts: aspeed: system1: Use crps PSU driver

[Ninad Palsule]

The system1 uses Intel common redundant (crps185) power supplies so move
to correct new crps driver.

Signed-off-by: Ninad Palsule <ninad@linux.ibm.com>
---
 arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-system1.dts | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-system1.dts b/arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-system1.dts
index 8f77bc9e860c..360b9ce3c850 100644
--- a/arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-system1.dts
+++ b/arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-system1.dts
@@ -681,22 +681,22 @@ &i2c2 {
 	status = "okay";
 
 	power-supply@58 {
-		compatible = "ibm,cffps";
+		compatible = "intel,crps185";
 		reg = <0x58>;
 	};
 
 	power-supply@59 {
-		compatible = "ibm,cffps";
+		compatible = "intel,crps185";
 		reg = <0x59>;
 	};
 
 	power-supply@5a {
-		compatible = "ibm,cffps";
+		compatible = "intel,crps185";
 		reg = <0x5a>;
 	};
 
 	power-supply@5b {
-		compatible = "ibm,cffps";
+		compatible = "intel,crps185";
 		reg = <0x5b>;
 	};
 };
-- 
2.43.0

Re: [PATCH v1 1/4] hwmon: pmbus-core: Add label for fan and temp

[Guenter Roeck]

On 12/12/24 13:49, Ninad Palsule wrote:
> Adding label files for fan and temperature sensors in the power supply.
> The openbmc application dbus-sensor(psusensor) requires those files to
> consfigure those sensors.
> Note that prefix for temp label is temp[A..C] used instead of temp[1..3]
> as dbus-sensor(psusensor) application calculate index based on last
> digit in the name so we do not want to make index double digit after
> appending page index.
> 
> Signed-off-by: Ninad Palsule <ninad@linux.ibm.com>

We are not going to fix userspace problems in the kernel.

Guenter

> ---
>   drivers/hwmon/pmbus/pmbus_core.c | 14 ++++++++++++++
>   1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
> index a7000314e5ad..149b3c5f3a4c 100644
> --- a/drivers/hwmon/pmbus/pmbus_core.c
> +++ b/drivers/hwmon/pmbus/pmbus_core.c
> @@ -2144,6 +2144,7 @@ static const struct pmbus_sensor_attr temp_attributes[] = {
>   	{
>   		.reg = PMBUS_READ_TEMPERATURE_1,
>   		.class = PSC_TEMPERATURE,
> +		.label = "tempA",
>   		.paged = true,
>   		.update = true,
>   		.compare = true,
> @@ -2156,6 +2157,7 @@ static const struct pmbus_sensor_attr temp_attributes[] = {
>   	}, {
>   		.reg = PMBUS_READ_TEMPERATURE_2,
>   		.class = PSC_TEMPERATURE,
> +		.label = "tempB",
>   		.paged = true,
>   		.update = true,
>   		.compare = true,
> @@ -2168,6 +2170,7 @@ static const struct pmbus_sensor_attr temp_attributes[] = {
>   	}, {
>   		.reg = PMBUS_READ_TEMPERATURE_3,
>   		.class = PSC_TEMPERATURE,
> +		.label = "tempC",
>   		.paged = true,
>   		.update = true,
>   		.compare = true,
> @@ -2282,6 +2285,17 @@ static int pmbus_add_fan_attributes(struct i2c_client *client,
>   					     PSC_FAN, true, true, true) == NULL)
>   				return -ENOMEM;
>   
> +			/*
> +			 * Add fan label.
> +			 * Assuming paged attributes while passing page index
> +			 */
> +			ret = pmbus_add_label(data, "fan", index, "fan",
> +					      page + 1, 0xff);
> +			if (ret) {
> +				dev_err(data->dev, "Fan label add failed ret=%d\n", ret);
> +				return ret;
> +			}
> +
>   			/* Fan control */
>   			if (pmbus_check_word_register(client, page,
>   					pmbus_fan_command_registers[f])) {

Re: [PATCH v1 2/4] hwmon: (pmbus/crps) Add Intel CRPS185 power supply

[Guenter Roeck]

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@linux.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@linux.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@vger.kernel.org
>   S:	Maintained
>   F:	drivers/hid/hid-creative-sb0540.c
>   
> +CRPS COMMON REDUNDANT PSU DRIVER

This is _INTEL_ CRPS.

> +M:	Ninad Palsule <ninad@linux.ibm.com>
> +L:	linux-hwmon@vger.kernel.org
> +S:	Maintained
> +F:	Documentation/hwmon/crps.rst
> +F:	drivers/hwmon/pmbus/crps.c
> +
>   CRYPTO API
>   M:	Herbert Xu <herbert@gondor.apana.org.au>
>   M:	"David S. Miller" <davem@davemloft.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");

Re: [PATCH v1 3/4] dt-bindings: hwmon: intel,crps185: Add to trivial

[Krzysztof Kozlowski]

On Thu, Dec 12, 2024 at 03:49:02PM -0600, Ninad Palsule wrote:
> Add INTEL Common Redundant Power Supply Versions crps185 bindings as
> trivial.

becuse they are trivial or you don't care? Some broader context would be
useful here.

Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

Re: [PATCH v1 1/4] hwmon: pmbus-core: Add label for fan and temp

[Ninad Palsule]

Hello Guenter,

On 12/12/24 16:06, Guenter Roeck wrote:
> On 12/12/24 13:49, Ninad Palsule wrote:
>> Adding label files for fan and temperature sensors in the power supply.
>> The openbmc application dbus-sensor(psusensor) requires those files to
>> consfigure those sensors.
>> Note that prefix for temp label is temp[A..C] used instead of temp[1..3]
>> as dbus-sensor(psusensor) application calculate index based on last
>> digit in the name so we do not want to make index double digit after
>> appending page index.
>>
>> Signed-off-by: Ninad Palsule <ninad@linux.ibm.com>
>
> We are not going to fix userspace problems in the kernel.
>
> Guenter
>

Thanks for the quick review.

Sorry I am not clear on this. I feel that it is better to support labels 
for temperature

sensors and fans like other. Are you saying we should not support these 
labels or

I need update in the patch to support them better?

Please let me know.

Thanks & Regards,

Ninad Palsule

Re: [PATCH v1 1/4] hwmon: pmbus-core: Add label for fan and temp

[Guenter Roeck]

On 12/13/24 08:12, Ninad Palsule wrote:
> Hello Guenter,
> 
> On 12/12/24 16:06, Guenter Roeck wrote:
>> On 12/12/24 13:49, Ninad Palsule wrote:
>>> Adding label files for fan and temperature sensors in the power supply.
>>> The openbmc application dbus-sensor(psusensor) requires those files to
>>> consfigure those sensors.
>>> Note that prefix for temp label is temp[A..C] used instead of temp[1..3]
>>> as dbus-sensor(psusensor) application calculate index based on last
>>> digit in the name so we do not want to make index double digit after
>>> appending page index.
>>>
>>> Signed-off-by: Ninad Palsule <ninad@linux.ibm.com>
>>
>> We are not going to fix userspace problems in the kernel.
>>
>> Guenter
>>
> 
> Thanks for the quick review.
> 
> Sorry I am not clear on this. I feel that it is better to support labels for temperature
> 
> sensors and fans like other. Are you saying we should not support these labels or
> 
> I need update in the patch to support them better?
> 

There should be no such labels. Labels are supposed to have specific meanings,
such as "this is the CPU temperature sensor", not vague meanings such as "tempA".

Guenter

Re: [PATCH v1 1/4] hwmon: pmbus-core: Add label for fan and temp

[Ninad Palsule]

Hi Guenter,


On 12/13/24 11:08, Guenter Roeck wrote:
> On 12/13/24 08:12, Ninad Palsule wrote:
>> Hello Guenter,
>>
>> On 12/12/24 16:06, Guenter Roeck wrote:
>>> On 12/12/24 13:49, Ninad Palsule wrote:
>>>> Adding label files for fan and temperature sensors in the power 
>>>> supply.
>>>> The openbmc application dbus-sensor(psusensor) requires those files to
>>>> consfigure those sensors.
>>>> Note that prefix for temp label is temp[A..C] used instead of 
>>>> temp[1..3]
>>>> as dbus-sensor(psusensor) application calculate index based on last
>>>> digit in the name so we do not want to make index double digit after
>>>> appending page index.
>>>>
>>>> Signed-off-by: Ninad Palsule <ninad@linux.ibm.com>
>>>
>>> We are not going to fix userspace problems in the kernel.
>>>
>>> Guenter
>>>
>>
>> Thanks for the quick review.
>>
>> Sorry I am not clear on this. I feel that it is better to support 
>> labels for temperature
>>
>> sensors and fans like other. Are you saying we should not support 
>> these labels or
>>
>> I need update in the patch to support them better?
>>
>
> There should be no such labels. Labels are supposed to have specific 
> meanings,
> such as "this is the CPU temperature sensor", not vague meanings such 
> as "tempA".
>
> Guenter

Thanks for the quick response. I will remove these changes for now and 
will talk to you later about

better option.

Thanks & Regards,

Ninad Palsule

Re: [PATCH v1 2/4] hwmon: (pmbus/crps) Add Intel CRPS185 power supply

[Ninad Palsule]

Hi Guenter,

Thanks for the review!

On 12/12/24 19:29, Guenter Roeck wrote:
> 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@linux.ibm.com>
>> ---
>>   Documentation/hwmon/crps.rst |  95 +++++++++++
>
> Documentation/hwmon/index.rst needs to be updated.
Added the entry. Sorry for that.
>
>>   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.
Fixed it.
>
>> +
>> +Supported chips:
>> +
>> +  * Intel CRPS185
>> +
>> +    Prefix: 'crps185'
>> +
>> +    Addresses scanned: -
>> +
>> +Authors:
>> +    Ninad Palsule <ninad@linux.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.
There is no public spec, added the comment
>
>> +Description
>> +-----------
>> +
>> +This driver implements support for Common Redundant Power supply 
>> with PMBus
>
> For _Intel_ ...
Updated.
>
>> +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@vger.kernel.org
>>   S:    Maintained
>>   F:    drivers/hid/hid-creative-sb0540.c
>>   +CRPS COMMON REDUNDANT PSU DRIVER
>
> This is _INTEL_ CRPS.
Updated.
>
>> +M:    Ninad Palsule <ninad@linux.ibm.com>
>> +L:    linux-hwmon@vger.kernel.org
>> +S:    Maintained
>> +F:    Documentation/hwmon/crps.rst
>> +F:    drivers/hwmon/pmbus/crps.c
>> +
>>   CRYPTO API
>>   M:    Herbert Xu <herbert@gondor.apana.org.au>
>>   M:    "David S. Miller" <davem@davemloft.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.
Fixed.
>
>> +    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.
Removed these defines.
>
>> +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.
Removed the enum.
>
>> +
>> +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.
I removed it and still driver is working. I added it because spec did 
not mention about PMBUS_STATUS_CML
>
>> +
>> +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.

Moved the attribute in the 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.
Removed debugfs
>
>> +    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 ?
You are right, removed it.
>
>> +    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 ?
I realized that some of these registers exist. So removed it.
>
> On top of that, on writes, the actual warning limits are overwritten.
> That makes even less sense.
I have removed read function so now we don't need write too.
>
>> +    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().
Removed this code.
>
>> +    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.
Yes, removed it.
>
>> +
>> +    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().
Changed to use this function.
>
>> +    }
>> +    if (strncmp(buf, "03NK260", 7)) {
>
> This should also check and ensure that rc == 7.
Good idea, Added this check.
>
>> +        buf[rc] = '\0';
>> +        dev_err(dev, "Model '%s' not supported\n", buf);
>
> dev_err_probe()
Changed the function
>
>> +        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().
Changed this function.
>
>> +    }
>> +
>> +    /*
>> +     * 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.
Yes, Removed debugfs.
>
>> +    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.

Yes, Added Intel


Thanks and Regards,

Ninad Palsule

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

Re: [PATCH v1 3/4] dt-bindings: hwmon: intel,crps185: Add to trivial

[Ninad Palsule]

Hi Krzysztof,

Thanks for the review.

On 12/13/24 05:08, Krzysztof Kozlowski wrote:
> On Thu, Dec 12, 2024 at 03:49:02PM -0600, Ninad Palsule wrote:
>> Add INTEL Common Redundant Power Supply Versions crps185 bindings as
>> trivial.
> becuse they are trivial or you don't care? Some broader context would be
> useful here.
>
> Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>
> Best regards,
> Krzysztof

Yes, I care for it. I have updated the comment. It is trivial because 
device tree only need compatibility string.

Thanks & Regards,

Ninad Palsule

>