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

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

[Ninad Palsule]

Hello,

Please review the patchset for Intel CRPS185 driver.
V2:
---
  - Incorporated review comments by Guenter Roeck
  - Incorporated review comments by Krzysztof Kozlowski

Ninad Palsule (4):
  hwmon: (pmbus/core) Add PMBUS_REVISION in debugfs
  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                  | 97 +++++++++++++++++++
 Documentation/hwmon/index.rst                 |  1 +
 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                    | 79 +++++++++++++++
 drivers/hwmon/pmbus/pmbus_core.c              | 13 ++-
 9 files changed, 211 insertions(+), 6 deletions(-)
 create mode 100644 Documentation/hwmon/crps.rst
 create mode 100644 drivers/hwmon/pmbus/crps.c

-- 
2.43.0

[PATCH v2 1/4] hwmon: (pmbus/core) Add PMBUS_REVISION in debugfs

[Ninad Palsule]

Add debugfs file for the PMBUS_REVISION command. This command provides
information about PMBus protocol revision number.

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

diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
index a7000314e5ad..787683e83db6 100644
--- a/drivers/hwmon/pmbus/pmbus_core.c
+++ b/drivers/hwmon/pmbus/pmbus_core.c
@@ -3534,11 +3534,11 @@ static int pmbus_init_debugfs(struct i2c_client *client,
 
 	/*
 	 * Allocate the max possible entries we need.
-	 * 6 entries device-specific
+	 * 7 entries device-specific
 	 * 10 entries page-specific
 	 */
 	entries = devm_kcalloc(data->dev,
-			       6 + data->info->pages * 10, sizeof(*entries),
+			       7 + data->info->pages * 10, sizeof(*entries),
 			       GFP_KERNEL);
 	if (!entries)
 		return -ENOMEM;
@@ -3551,6 +3551,15 @@ static int pmbus_init_debugfs(struct i2c_client *client,
 	 * assume that values of the following registers are the same for all
 	 * pages and report values only for page 0.
 	 */
+	if (pmbus_check_byte_register(client, 0, PMBUS_REVISION)) {
+		entries[idx].client = client;
+		entries[idx].page = 0;
+		entries[idx].reg = PMBUS_REVISION;
+		debugfs_create_file("revision", 0444, data->debugfs,
+				    &entries[idx++],
+				    &pmbus_debugfs_ops);
+	}
+
 	if (pmbus_check_block_register(client, 0, PMBUS_MFR_ID)) {
 		entries[idx].client = client;
 		entries[idx].page = 0;
-- 
2.43.0

[PATCH v2 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  | 97 +++++++++++++++++++++++++++++++++++
 Documentation/hwmon/index.rst |  1 +
 MAINTAINERS                   |  7 +++
 drivers/hwmon/pmbus/Kconfig   |  9 ++++
 drivers/hwmon/pmbus/Makefile  |  1 +
 drivers/hwmon/pmbus/crps.c    | 79 ++++++++++++++++++++++++++++
 6 files changed, 194 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..74570ed1e978
--- /dev/null
+++ b/Documentation/hwmon/crps.rst
@@ -0,0 +1,97 @@
+.. SPDX-License-Identifier: GPL-2.0-or-later
+
+Kernel driver crps
+==================
+
+Supported chips:
+
+  * Intel CRPS185
+
+    Prefix: 'crps185'
+
+    Addresses scanned: -
+
+    Datasheet: Publicly not available.
+
+Authors:
+    Ninad Palsule <ninad@linux.ibm.com>
+
+
+Description
+-----------
+
+This driver implements support for Intel 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/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
index 1a3cb0a59f72..b1ea445479b0 100644
--- a/Documentation/hwmon/index.rst
+++ b/Documentation/hwmon/index.rst
@@ -58,6 +58,7 @@ Hardware Monitoring Kernel Drivers
    corsair-cpro
    corsair-psu
    cros_ec_hwmon
+   crps
    da9052
    da9055
    dell-smm-hwmon
diff --git a/MAINTAINERS b/MAINTAINERS
index 637ddd44245f..e99f26f75733 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -6100,6 +6100,13 @@ L:	linux-input@vger.kernel.org
 S:	Maintained
 F:	drivers/hid/hid-creative-sb0540.c
 
+INTEL 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..43b6df04e0f9 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 "Intel Common Redundant Power Supply"
+	help
+	  If you say yes here you get hardware monitoring support for the Intel
+	  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..09425c404fc8
--- /dev/null
+++ b/drivers/hwmon/pmbus/crps.c
@@ -0,0 +1,79 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright 2024 IBM Corp.
+ */
+
+#include <linux/i2c.h>
+#include <linux/of.h>
+#include <linux/pmbus.h>
+
+#include "pmbus.h"
+
+static const struct i2c_device_id crps_id[] = {
+	{ "intel_crps185" },
+	{}
+};
+MODULE_DEVICE_TABLE(i2c, crps_id);
+
+static struct pmbus_driver_info crps_info = {
+	.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,
+};
+
+static int crps_probe(struct i2c_client *client)
+{
+	int rc;
+	struct device *dev = &client->dev;
+	char buf[I2C_SMBUS_BLOCK_MAX + 2] = { 0 };
+
+	rc = i2c_smbus_read_block_data(client, PMBUS_MFR_MODEL, buf);
+	if (rc < 0) {
+		dev_err_probe(dev, rc, "Failed to read PMBUS_MFR_MODEL\n");
+		return rc;
+	}
+
+	if (strncmp(buf, "03NK260", 7) == 7) {
+		buf[rc] = '\0';
+		dev_err_probe(dev, -ENODEV, "Model '%s' not supported\n", buf);
+		return -ENODEV;
+	}
+
+	rc = pmbus_do_probe(client, &crps_info);
+	if (rc) {
+		dev_err_probe(dev, rc, "Failed to probe %d\n", rc);
+		return rc;
+	}
+
+	return 0;
+}
+
+static const struct of_device_id crps_of_match[] = {
+	{
+		.compatible = "intel,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 Intel Common Redundant power supplies");
+MODULE_LICENSE("GPL");
+MODULE_IMPORT_NS("PMBUS");
-- 
2.43.0

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

[Ninad Palsule]

Add INTEL Common Redundant Power Supply Versions crps185 bindings as
trivial. It is trivial because only compatibility string is required in
the device tree to load this driver.

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 v2 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 v2 2/4] hwmon: (pmbus/crps) Add Intel CRPS185 power supply

[Guenter Roeck]

On 12/16/24 09:50, 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  | 97 +++++++++++++++++++++++++++++++++++
>   Documentation/hwmon/index.rst |  1 +
>   MAINTAINERS                   |  7 +++
>   drivers/hwmon/pmbus/Kconfig   |  9 ++++
>   drivers/hwmon/pmbus/Makefile  |  1 +
>   drivers/hwmon/pmbus/crps.c    | 79 ++++++++++++++++++++++++++++
>   6 files changed, 194 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..74570ed1e978
> --- /dev/null
> +++ b/Documentation/hwmon/crps.rst
> @@ -0,0 +1,97 @@
> +.. SPDX-License-Identifier: GPL-2.0-or-later
> +
> +Kernel driver crps
> +==================
> +
> +Supported chips:
> +
> +  * Intel CRPS185
> +
> +    Prefix: 'crps185'
> +
> +    Addresses scanned: -
> +
> +    Datasheet: Publicly not available.

Maybe "Only available under NDA". Or at least that is what I found.

> +
> +Authors:
> +    Ninad Palsule <ninad@linux.ibm.com>
> +
> +
> +Description
> +-----------
> +
> +This driver implements support for Intel 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.
> +======================= ======================================================

Does this pass "make htmldocs" ?


> diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
> index 1a3cb0a59f72..b1ea445479b0 100644
> --- a/Documentation/hwmon/index.rst
> +++ b/Documentation/hwmon/index.rst
> @@ -58,6 +58,7 @@ Hardware Monitoring Kernel Drivers
>      corsair-cpro
>      corsair-psu
>      cros_ec_hwmon
> +   crps
>      da9052
>      da9055
>      dell-smm-hwmon
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 637ddd44245f..e99f26f75733 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -6100,6 +6100,13 @@ L:	linux-input@vger.kernel.org
>   S:	Maintained
>   F:	drivers/hid/hid-creative-sb0540.c
>   
> +INTEL 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..43b6df04e0f9 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 "Intel Common Redundant Power Supply"
> +	help
> +	  If you say yes here you get hardware monitoring support for the Intel
> +	  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..09425c404fc8
> --- /dev/null
> +++ b/drivers/hwmon/pmbus/crps.c
> @@ -0,0 +1,79 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright 2024 IBM Corp.
> + */
> +
> +#include <linux/i2c.h>
> +#include <linux/of.h>
> +#include <linux/pmbus.h>
> +
> +#include "pmbus.h"
> +
> +static const struct i2c_device_id crps_id[] = {
> +	{ "intel_crps185" },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(i2c, crps_id);
> +
> +static struct pmbus_driver_info crps_info = {
> +	.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,
> +};
> +
> +static int crps_probe(struct i2c_client *client)
> +{
> +	int rc;
> +	struct device *dev = &client->dev;
> +	char buf[I2C_SMBUS_BLOCK_MAX + 2] = { 0 };
> +
> +	rc = i2c_smbus_read_block_data(client, PMBUS_MFR_MODEL, buf);
> +	if (rc < 0) {
> +		dev_err_probe(dev, rc, "Failed to read PMBUS_MFR_MODEL\n");
> +		return rc;

		return dev_err_probe(...);

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

strncmp() never returns 7. You probably want something like

	if (rc != 7 || strncmp(buf, "03NK260", 7)) {

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

		return dev_err_probe(...);

> +		return -ENODEV;
> +	}
> +
> +	rc = pmbus_do_probe(client, &crps_info);
> +	if (rc) {
> +		dev_err_probe(dev, rc, "Failed to probe %d\n", rc);

dev_err_probe() already handles the error, and a message such as
"failed to probe -22" isn't very useful anyway. Also,
		return dev_err_probe(...);

> +		return rc;
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id crps_of_match[] = {
> +	{
> +		.compatible = "intel,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 Intel Common Redundant power supplies");
> +MODULE_LICENSE("GPL");
> +MODULE_IMPORT_NS("PMBUS");

Re: [PATCH v2 1/4] hwmon: (pmbus/core) Add PMBUS_REVISION in debugfs

[Guenter Roeck]

On Mon, Dec 16, 2024 at 11:50:39AM -0600, Ninad Palsule wrote:
> Add debugfs file for the PMBUS_REVISION command. This command provides
> information about PMBus protocol revision number.
> 
> Signed-off-by: Ninad Palsule <ninad@linux.ibm.com>

Applied.

Thanks,
Guenter

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

[Guenter Roeck]

On 12/16/24 09:50, Ninad Palsule wrote:
> Add INTEL Common Redundant Power Supply Versions crps185 bindings as
> trivial. It is trivial because only compatibility string is required in
> the device tree to load this driver.
> 
> Signed-off-by: Ninad Palsule <ninad@linux.ibm.com>

Krzysztof had Acked this patch. I don't immediately see why you dropped it.
Am I missing something ?

Guenter

> ---
>   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

Re: [PATCH v2 0/4] Add support for Intel CRPS PSU

[Guenter Roeck]

On 12/16/24 09:50, Ninad Palsule wrote:
> Hello,
> 
> Please review the patchset for Intel CRPS185 driver.
> V2:
> ---
>    - Incorporated review comments by Guenter Roeck
>    - Incorporated review comments by Krzysztof Kozlowski
> 

That is not a useful change log. Please describe what you changed, not who
asked for it.

Guenter

> Ninad Palsule (4):
>    hwmon: (pmbus/core) Add PMBUS_REVISION in debugfs
>    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                  | 97 +++++++++++++++++++
>   Documentation/hwmon/index.rst                 |  1 +
>   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                    | 79 +++++++++++++++
>   drivers/hwmon/pmbus/pmbus_core.c              | 13 ++-
>   9 files changed, 211 insertions(+), 6 deletions(-)
>   create mode 100644 Documentation/hwmon/crps.rst
>   create mode 100644 drivers/hwmon/pmbus/crps.c
> 

Re: [PATCH v2 0/4] Add support for Intel CRPS PSU

[Ninad Palsule]

Hi Guenter,

Thanks for the review.

On 12/16/24 12:48, Guenter Roeck wrote:
> On 12/16/24 09:50, Ninad Palsule wrote:
>> Hello,
>>
>> Please review the patchset for Intel CRPS185 driver.
>> V2:
>> ---
>>    - Incorporated review comments by Guenter Roeck
>>    - Incorporated review comments by Krzysztof Kozlowski
>>
>
> That is not a useful change log. Please describe what you changed, not 
> who
> asked for it.
>
> Guenter
>
ok, I will improve it in version 3.

Regards,

Ninad

>> Ninad Palsule (4):
>>    hwmon: (pmbus/core) Add PMBUS_REVISION in debugfs
>>    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                  | 97 +++++++++++++++++++
>>   Documentation/hwmon/index.rst                 |  1 +
>>   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                    | 79 +++++++++++++++
>>   drivers/hwmon/pmbus/pmbus_core.c              | 13 ++-
>>   9 files changed, 211 insertions(+), 6 deletions(-)
>>   create mode 100644 Documentation/hwmon/crps.rst
>>   create mode 100644 drivers/hwmon/pmbus/crps.c
>>
>
>

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

[Ninad Palsule]

Hi Guenter,

Thanks for the review.

On 12/16/24 12:47, Guenter Roeck wrote:
> On 12/16/24 09:50, Ninad Palsule wrote:
>> Add INTEL Common Redundant Power Supply Versions crps185 bindings as
>> trivial. It is trivial because only compatibility string is required in
>> the device tree to load this driver.
>>
>> Signed-off-by: Ninad Palsule <ninad@linux.ibm.com>
>
> Krzysztof had Acked this patch. I don't immediately see why you 
> dropped it.
> Am I missing something ?
>
> Guenter
>
I think that was my mistake. Adding it in version 3.


>> ---
>>   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
>
>

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

[Ninad Palsule]

Hi Guenter,

Thanks for the review.

On 12/16/24 12:26, Guenter Roeck wrote:
> On 12/16/24 09:50, 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  | 97 +++++++++++++++++++++++++++++++++++
>>   Documentation/hwmon/index.rst |  1 +
>>   MAINTAINERS                   |  7 +++
>>   drivers/hwmon/pmbus/Kconfig   |  9 ++++
>>   drivers/hwmon/pmbus/Makefile  |  1 +
>>   drivers/hwmon/pmbus/crps.c    | 79 ++++++++++++++++++++++++++++
>>   6 files changed, 194 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..74570ed1e978
>> --- /dev/null
>> +++ b/Documentation/hwmon/crps.rst
>> @@ -0,0 +1,97 @@
>> +.. SPDX-License-Identifier: GPL-2.0-or-later
>> +
>> +Kernel driver crps
>> +==================
>> +
>> +Supported chips:
>> +
>> +  * Intel CRPS185
>> +
>> +    Prefix: 'crps185'
>> +
>> +    Addresses scanned: -
>> +
>> +    Datasheet: Publicly not available.
>
> Maybe "Only available under NDA". Or at least that is what I found.
Updated as per your suggestion.
>
>> +
>> +Authors:
>> +    Ninad Palsule <ninad@linux.ibm.com>
>> +
>> +
>> +Description
>> +-----------
>> +
>> +This driver implements support for Intel 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.
>> +======================= 
>> ======================================================
>
> Does this pass "make htmldocs" ?

Yes, I don't get any error. I run following command on the top level 
directory.

export ARCH=arm;export CROSS_COMPILE="arm-linux-gnueabi-";make htmldocs

>
>
>> diff --git a/Documentation/hwmon/index.rst 
>> b/Documentation/hwmon/index.rst
>> index 1a3cb0a59f72..b1ea445479b0 100644
>> --- a/Documentation/hwmon/index.rst
>> +++ b/Documentation/hwmon/index.rst
>> @@ -58,6 +58,7 @@ Hardware Monitoring Kernel Drivers
>>      corsair-cpro
>>      corsair-psu
>>      cros_ec_hwmon
>> +   crps
>>      da9052
>>      da9055
>>      dell-smm-hwmon
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 637ddd44245f..e99f26f75733 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -6100,6 +6100,13 @@ L:    linux-input@vger.kernel.org
>>   S:    Maintained
>>   F:    drivers/hid/hid-creative-sb0540.c
>>   +INTEL 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..43b6df04e0f9 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 "Intel Common Redundant Power Supply"
>> +    help
>> +      If you say yes here you get hardware monitoring support for 
>> the Intel
>> +      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..09425c404fc8
>> --- /dev/null
>> +++ b/drivers/hwmon/pmbus/crps.c
>> @@ -0,0 +1,79 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +/*
>> + * Copyright 2024 IBM Corp.
>> + */
>> +
>> +#include <linux/i2c.h>
>> +#include <linux/of.h>
>> +#include <linux/pmbus.h>
>> +
>> +#include "pmbus.h"
>> +
>> +static const struct i2c_device_id crps_id[] = {
>> +    { "intel_crps185" },
>> +    {}
>> +};
>> +MODULE_DEVICE_TABLE(i2c, crps_id);
>> +
>> +static struct pmbus_driver_info crps_info = {
>> +    .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,
>> +};
>> +
>> +static int crps_probe(struct i2c_client *client)
>> +{
>> +    int rc;
>> +    struct device *dev = &client->dev;
>> +    char buf[I2C_SMBUS_BLOCK_MAX + 2] = { 0 };
>> +
>> +    rc = i2c_smbus_read_block_data(client, PMBUS_MFR_MODEL, buf);
>> +    if (rc < 0) {
>> +        dev_err_probe(dev, rc, "Failed to read PMBUS_MFR_MODEL\n");
>> +        return rc;
>
>         return dev_err_probe(...);
>
>> +    }
>> +
>> +    if (strncmp(buf, "03NK260", 7) == 7) {
>
> strncmp() never returns 7. You probably want something like
>
>     if (rc != 7 || strncmp(buf, "03NK260", 7)) {
My bad. Fixed it.
>
>> +        buf[rc] = '\0';
>> +        dev_err_probe(dev, -ENODEV, "Model '%s' not supported\n", buf);
>
>         return dev_err_probe(...);
Fixed.
>
>> +        return -ENODEV;
>> +    }
>> +
>> +    rc = pmbus_do_probe(client, &crps_info);
>> +    if (rc) {
>> +        dev_err_probe(dev, rc, "Failed to probe %d\n", rc);
>
> dev_err_probe() already handles the error, and a message such as
> "failed to probe -22" isn't very useful anyway. Also,
>         return dev_err_probe(...);
Fixed.


Thanks and regards,

Ninad Palsule