[PATCH RFC] hwmon: Add Support for PMIC5000 Power Management ICs

All of lore.kernel.org
 help / color / mirror / Atom feed

* [PATCH RFC] hwmon: Add Support for PMIC5000 Power Management ICs
@ 2026-08-19 13:37 Stephen Horvath
  2026-08-19 13:51 ` sashiko-bot
                   ` (2 more replies)
  0 siblings, 3 replies; 7+ messages in thread
From: Stephen Horvath @ 2026-08-19 13:37 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: Andi Shyti, linux-hwmon, linux-i2c, linux-kernel, Stephen Horvath,
	Stephen Horvath

Add support for sensors present on PMIC5000 (JEDEC JESD301) compliant
power management ICs. These chips are commonly found on DDR5 memory
modules.

Tested against PMIC5100 chips, but only features present in the
PMIC5000 (JESD301-1) specification have been implemented.

Signed-off-by: Stephen Horvath <linux@stevetech.au>
---
Hi maintainers, I wrote this over the past couple days and I'm looking
for feedback.

Here's a few concerns I have:
- I'm not too experienced with kernel development.
- I don't think there's a good way to auto-detect PMIC5000 chips 
	standalone, would it be reasonable to instantiate it from
	spd5118?
- I'm unsure what to do with temperatures below 85°C, I'd rather not
	return 85 as that would make it seem hotter than it is, but
	returning -EOPNOTSUPP seems wrong.
- Toggling between current and power seems like it's a bit of a hack,
	is that okay, or should I just pick one and drop the other?
- Does this belong in HWMON, or should it be part of the regulator
	subsystem?
---
 .../devicetree/bindings/trivial-devices.yaml       |   2 +
 Documentation/hwmon/index.rst                      |   1 +
 Documentation/hwmon/pmic5000.rst                   | 103 +++
 drivers/hwmon/Kconfig                              |  12 +
 drivers/hwmon/Makefile                             |   1 +
 drivers/hwmon/pmic5000.c                           | 896 +++++++++++++++++++++
 6 files changed, 1015 insertions(+)

diff --git a/Documentation/devicetree/bindings/trivial-devices.yaml b/Documentation/devicetree/bindings/trivial-devices.yaml
index 2de8eb09cb7d..86e76be7d41b 100644
--- a/Documentation/devicetree/bindings/trivial-devices.yaml
+++ b/Documentation/devicetree/bindings/trivial-devices.yaml
@@ -194,6 +194,8 @@ properties:
           - isil,isl29030
             # Intersil ISL76682 Ambient Light Sensor
           - isil,isl76682
+            # JEDEC JESD301 (PMIC5000) Power Management IC
+          - jedec,pmic5000
             # JEDEC JESD300 (SPD5118) Hub and Serial Presence Detect
           - jedec,spd5118
             # Kandou KB9002 PCIe 5.0 retimer
diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
index d979e6d6e9f2..4bb518d1927c 100644
--- a/Documentation/hwmon/index.rst
+++ b/Documentation/hwmon/index.rst
@@ -226,6 +226,7 @@ Hardware Monitoring Kernel Drivers
    peci-cputemp
    peci-dimmtemp
    pmbus
+   pmic5000
    powerz
    powr1220
    prom21-xhci
diff --git a/Documentation/hwmon/pmic5000.rst b/Documentation/hwmon/pmic5000.rst
new file mode 100644
index 000000000000..e7d864088ba5
--- /dev/null
+++ b/Documentation/hwmon/pmic5000.rst
@@ -0,0 +1,103 @@
+.. SPDX-License-Identifier: GPL-2.0-or-later
+
+Kernel driver pmic5000
+=====================
+
+Supported chips:
+
+  * PMIC5000 (JEDEC JESD301) compliant power management chips
+
+    JEDEC standard download:
+	https://www.jedec.org/standards-documents/docs/jesd301-1a03
+	(account required)
+
+
+    Prefix: 'pmic5000'
+
+Author:
+	Stephen Horvath <linux@stevetech.au>
+
+
+Description
+-----------
+
+This driver implements support for PMIC5000 (JEDEC JESD301) compliant power
+management chips, which are used on many DDR5 memory modules.
+
+The driver does not auto-detect PMIC5000 compliant chips, and must be
+instantiated manually or via device tree. The address of these chips are
+usually between (inclusive) 0x48 and 0x4F.
+
+
+Hardware monitoring sysfs entries
+---------------------------------
+
+======================= ==================================
+in0_enable		Whether SWA is enabled (RO)
+in0_input		SWA Voltage (RO)
+in0_min			Minimum SWA Voltage (RO)
+in0_max			Maximum SWA Voltage (RO)
+in0_min_alarm		SWA Voltage low alarm
+in0_max_alarm		SWA Voltage high alarm
+
+in1_enable		Whether SWB is enabled (RO)
+in1_input		SWB Voltage (RO)
+in1_min			Minimum SWB Voltage (RO)
+in1_max			Maximum SWB Voltage (RO)
+in1_min_alarm		SWB Voltage low alarm
+in1_max_alarm		SWB Voltage high alarm
+
+in2_enable		Whether SWC is enabled (RO)
+in2_input		SWC Voltage (RO)
+in2_min			Minimum SWC Voltage (RO)
+in2_max			Maximum SWC Voltage (RO)
+in2_min_alarm		SWC Voltage low alarm
+in2_max_alarm		SWC Voltage high alarm
+
+in3_enable		Whether SWD is enabled (RO)
+in3_input		SWD Voltage (RO)
+in3_min			Minimum SWD Voltage (RO)
+in3_max			Maximum SWD Voltage (RO)
+in3_min_alarm		SWD Voltage low alarm
+in3_max_alarm		SWD Voltage high alarm
+
+in5_input		VIN_Bulk Voltage (RO)
+in5_max			Maximum VIN_Bulk Voltage (RO)
+in5_max_alarm		VIN_Bulk Voltage high alarm
+
+in6_input		VIN_Mgmt Voltage (RO)
+in6_max			Maximum VIN_Mgmt Voltage (RO)
+in6_max_alarm		VIN_Mgmt Voltage high alarm
+
+in7_input		VBias Voltage (RO)
+in8_input		VOUT_1.8V Voltage (RO)
+in9_input		VOUT_1.0V Voltage (RO)
+
+temp1_input		Temperature (RO)
+temp1_max		Maximum temperature (RO)
+temp1_max_alarm		Temperature high alarm
+
+power1_input 		SWA Wattage (RO)
+power2_input 		SWB Wattage (RO)
+power3_input 		SWC Wattage (RO)
+power4_input 		SWD Wattage (RO)
+
+curr1_input		SWA Current (RO)
+curr1_max		Maximum SWA Current (RO)
+curr1_max_alarm		SWA Current high alarm
+
+curr2_input		SWB Current (RO)
+curr2_max		Maximum SWB Current (RO)
+curr2_max_alarm		SWB Current high alarm
+
+curr3_input		SWC Current (RO)
+curr3_max		Maximum SWC Current (RO)
+curr3_max_alarm		SWC Current high alarm
+
+curr4_input		SWD Current (RO)
+curr4_max		Maximum SWD Current (RO)
+curr4_max_alarm		SWD Current high alarm
+
+update_interval		Update interval in milliseconds (RW), Allowed values: 1, 2, 4, 8
+
+======================= ==================================
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 81a9a1d40eec..5ed5f9499c43 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -950,6 +950,18 @@ config SENSORS_JC42
 	  This driver can also be built as a module. If so, the module
 	  will be called jc42.
 
+config SENSORS_PMIC5000
+	tristate "PMIC5000 Compliant Sensors"
+	depends on I2C
+	select REGMAP_I2C
+	help
+	  If you say yes here you get support for PMIC5000 (JEDEC JESD301)
+	  compliant sensors. Such sensors are found on DDR5 memory
+	  modules.
+
+	  This driver can also be built as a module. If so, the module
+	  will be called pmic5000.
+
 config SENSORS_POLARFIRE_SOC_TVS
 	tristate "PolarFire SoC (MPFS) temperature and voltage sensor"
 	depends on POLARFIRE_SOC_SYSCONS || COMPILE_TEST
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index 0cad7e21634c..ac519123796f 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -199,6 +199,7 @@ obj-$(CONFIG_SENSORS_NZXT_SMART2) += nzxt-smart2.o
 obj-$(CONFIG_SENSORS_PC87360)	+= pc87360.o
 obj-$(CONFIG_SENSORS_PC87427)	+= pc87427.o
 obj-$(CONFIG_SENSORS_PCF8591)	+= pcf8591.o
+obj-$(CONFIG_SENSORS_PMIC5000)	+= pmic5000.o
 obj-$(CONFIG_SENSORS_POLARFIRE_SOC_TVS)  += tvs-mpfs.o
 obj-$(CONFIG_SENSORS_POWERZ)	+= powerz.o
 obj-$(CONFIG_SENSORS_POWR1220)  += powr1220.o
diff --git a/drivers/hwmon/pmic5000.c b/drivers/hwmon/pmic5000.c
new file mode 100644
index 000000000000..d3f3126dd9c8
--- /dev/null
+++ b/drivers/hwmon/pmic5000.c
@@ -0,0 +1,896 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Driver for Jedec PMIC5000 compliant sensors
+ *
+ * Copyright (c) 2026 Stephen Horvath
+ *
+ * Inspired by spd5118.c.
+ *
+ * PMIC5000 compliant sensors are typically used on DDR5 memory modules.
+ */
+
+#include <linux/bitops.h>
+#include <linux/bits.h>
+#include <linux/err.h>
+#include <linux/hwmon.h>
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/pm.h>
+#include <linux/regmap.h>
+#include <linux/units.h>
+
+/* PMIC5000 registers. */
+// clang-format off
+#define PMIC5000_REG_OVER_VOLT_IN	0x08
+#define PMIC5000_REG_OVER_CURRENT	0x09
+	#define PMIC5000_HIGH_TEMP		BIT(7)
+#define PMIC5000_REG_OVER_VOLTAGE	0x0A
+#define PMIC5000_REG_UNDER_VOLTAGE	0x0B
+#define PMIC5000_REG_SWA_POWER		0x0C
+#define PMIC5000_REG_SWB_POWER		0x0D
+#define PMIC5000_REG_SWC_POWER		0x0E
+#define PMIC5000_REG_SWD_POWER		0x0F
+#define PMIC5000_REG_OUTPUT_SELECT	0x1A
+	#define PMIC5000_OUTPUT_SELECT		BIT(1)
+#define PMIC5000_REG_THRES_AND_SEL	0x1B
+	#define PMIC5000_VIN_MGMT_THRESH	BIT(5)
+	#define PMIC5000_CURR_OR_PWR		BIT(6)
+	#define PMIC5000_VIN_BULK_THRESH	BIT(7)
+#define PMIC5000_REG_SWA_CURR_WARN	0x1C
+#define PMIC5000_REG_SWB_CURR_WARN	0x1D
+#define PMIC5000_REG_SWC_CURR_WARN	0x1E
+#define PMIC5000_REG_SWD_CURR_WARN	0x1F
+#define PMIC5000_REG_SWA_VOLT_SET	0x21
+#define PMIC5000_REG_SWA_THRESH		0x22
+#define PMIC5000_REG_SWB_VOLT_SET	0x23
+#define PMIC5000_REG_SWB_THRESH		0x24
+#define PMIC5000_REG_SWC_VOLT_SET	0x25
+#define PMIC5000_REG_SWC_THRESH		0x26
+#define PMIC5000_REG_SWD_VOLT_SET	0x27
+#define PMIC5000_REG_SWD_THRESH		0x28
+#define PMIC5000_REG_SW_VOLT_RANGE	0x2B
+	#define PMIC5000_SWD_RANGE		BIT(0)
+	#define PMIC5000_SWC_RANGE		BIT(3)
+	#define PMIC5000_SWB_RANGE		BIT(4)
+	#define PMIC5000_SWA_RANGE		BIT(5)
+#define PMIC5000_REG_REGULATOR_CONTROL	0x2F
+	#define PMIC5000_SWD_CONTROL		BIT(3)
+	#define PMIC5000_SWC_CONTROL		BIT(4)
+	#define PMIC5000_SWB_CONTROL		BIT(5)
+	#define PMIC5000_SWA_CONTROL		BIT(6)
+#define PMIC5000_REG_ADC_CONFIG		0x30
+	#define PMIC5000_ADC_SELECT_MASK	GENMASK(6, 3)
+	#define PMIC5000_ADC_ENABLE		BIT(7)
+#define PMIC5000_REG_ADC_VOLTAGE	0x31
+#define PMIC5000_REG_TEMPERATURE	0x33
+#define PMIC5000_REG_REVISION		0x3B
+#define PMIC5000_REG_VENDOR		0x3C
+
+
+/* 125 mA multiplier */
+#define PMIC5000_CURR_UNIT		125
+/* 125 mW multiplier */
+#define PMIC5000_POWER_UNIT		(125 * MILLIWATT_PER_WATT)
+/* mV multipliers */
+#define PMIC5000_VOLT_UNIT		15
+#define PMIC5000_VINBULK_UNIT		70
+#define PMIC5000_VBIAS_UNIT		25
+// clang-format on
+
+struct pmic5000_data {
+	struct regmap *regmap;
+	struct mutex mode_lock;
+	struct mutex adc_lock;
+};
+
+static const char *const pmic5000_power_labels[] = { "SWA", "SWB", "SWC",
+						     "SWD" };
+
+static const char *const pmic5000_voltage_labels[] = {
+	"SWA",	    "SWB",	"SWC",	 "SWD",	      NULL,
+	"VIN_Bulk", "VIN_Mgmt", "VBias", "VOUT_1.8V", "VOUT_1.0V"
+};
+
+/* hwmon */
+
+static int pmic5000_check_regulator_enabled(struct regmap *regmap, int channel)
+{
+	u32 regval;
+	int err;
+
+	err = regmap_read(regmap, PMIC5000_REG_REGULATOR_CONTROL, &regval);
+	if (err)
+		return err;
+	switch (channel) {
+	case 0:
+		return !!(regval & PMIC5000_SWA_CONTROL);
+	case 1:
+		return !!(regval & PMIC5000_SWB_CONTROL);
+	case 2:
+		return !!(regval & PMIC5000_SWC_CONTROL);
+	case 3:
+		return !!(regval & PMIC5000_SWD_CONTROL);
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static int pmic5000_read_temp(struct regmap *regmap, u32 attr, int channel,
+			      long *val)
+{
+	int err;
+	u32 regval;
+
+	if (channel != 0)
+		return -EOPNOTSUPP;
+
+	switch (attr) {
+	case hwmon_temp_input: {
+		err = regmap_read(regmap, PMIC5000_REG_TEMPERATURE, &regval);
+		if (err)
+			return err;
+		regval >>= 5;
+		/* Below 85°C */
+		if (regval == 0)
+			return -EOPNOTSUPP;
+		/* 0b001 = 85°C, 0b010 = 95°C, etc. */
+		*val = (75 + regval * 10) * MILLIDEGREE_PER_DEGREE;
+		return 0;
+	}
+	case hwmon_temp_max: {
+		err = regmap_read(regmap, PMIC5000_REG_THRES_AND_SEL, &regval);
+		if (err)
+			return err;
+		regval &= 0x07;
+		/* Reserved */
+		if (regval == 0 || regval == 0x7)
+			return -EOPNOTSUPP;
+		*val = (75 + regval * 10) * MILLIDEGREE_PER_DEGREE;
+		return 0;
+	}
+	case hwmon_temp_max_alarm: {
+		err = regmap_read(regmap, PMIC5000_REG_OVER_CURRENT, &regval);
+		if (err)
+			return err;
+		*val = !!(regval & PMIC5000_HIGH_TEMP);
+		return 0;
+	}
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static int pmic5000_read_curr(struct pmic5000_data *data, u32 attr, int channel,
+			      long *val)
+{
+	struct regmap *regmap = data->regmap;
+	int reg, err;
+	int shift = 0;
+	u32 regval;
+
+	if (attr == hwmon_curr_input) {
+		mutex_lock(&data->mode_lock);
+		/* Select power measurements */
+		err = regmap_update_bits(regmap, PMIC5000_REG_THRES_AND_SEL,
+					 PMIC5000_CURR_OR_PWR, 0);
+		if (err)
+			goto error;
+
+		switch (channel) {
+		case 0:
+			reg = PMIC5000_REG_SWA_POWER;
+			break;
+		case 1:
+			reg = PMIC5000_REG_SWB_POWER;
+			break;
+		case 2:
+			reg = PMIC5000_REG_SWC_POWER;
+			break;
+		case 3:
+			reg = PMIC5000_REG_SWD_POWER;
+			break;
+		default:
+			err = -EOPNOTSUPP;
+			goto error;
+		}
+	} else if (attr == hwmon_curr_max) {
+		shift = 2;
+		switch (channel) {
+		case 0:
+			reg = PMIC5000_REG_SWA_CURR_WARN;
+			break;
+		case 1:
+			reg = PMIC5000_REG_SWB_CURR_WARN;
+			break;
+		case 2:
+			reg = PMIC5000_REG_SWC_CURR_WARN;
+			break;
+		case 3:
+			reg = PMIC5000_REG_SWD_CURR_WARN;
+			break;
+		default:
+			return -EOPNOTSUPP;
+		}
+	} else if (attr == hwmon_curr_max_alarm && channel >= 0 &&
+		   channel <= 3) {
+		err = regmap_read(regmap, PMIC5000_REG_OVER_CURRENT, &regval);
+		if (err)
+			return err;
+		*val = regval >> (3 - channel) & 0x01;
+		return 0;
+	} else {
+		return -EOPNOTSUPP;
+	}
+
+	err = regmap_read(regmap, reg, &regval);
+	if (err)
+		goto error;
+
+	if (attr == hwmon_curr_input)
+		mutex_unlock(&data->mode_lock);
+
+	*val = regval * PMIC5000_CURR_UNIT >> shift;
+	return 0;
+
+error:
+	if (attr == hwmon_curr_input)
+		mutex_unlock(&data->mode_lock);
+	return err;
+}
+
+static int pmic5000_read_power(struct pmic5000_data *data, u32 attr,
+			       int channel, long *val)
+{
+	struct regmap *regmap = data->regmap;
+	int reg, err;
+	u32 regval;
+
+	if (attr != hwmon_power_input)
+		return -EOPNOTSUPP;
+
+	mutex_lock(&data->mode_lock);
+	/* Select power measurements */
+	err = regmap_update_bits(regmap, PMIC5000_REG_THRES_AND_SEL,
+				 PMIC5000_CURR_OR_PWR, PMIC5000_CURR_OR_PWR);
+	if (err)
+		goto error;
+
+	switch (channel) {
+	case 0:
+		reg = PMIC5000_REG_SWA_POWER;
+		break;
+	case 1:
+		reg = PMIC5000_REG_SWB_POWER;
+		break;
+	case 2:
+		reg = PMIC5000_REG_SWC_POWER;
+		break;
+	case 3:
+		reg = PMIC5000_REG_SWD_POWER;
+		break;
+	default:
+		err = -EOPNOTSUPP;
+		goto error;
+	}
+
+	err = regmap_read(regmap, reg, &regval);
+	if (err)
+		goto error;
+
+	mutex_unlock(&data->mode_lock);
+
+	*val = regval * PMIC5000_POWER_UNIT;
+	return 0;
+
+error:
+	mutex_unlock(&data->mode_lock);
+	return err;
+}
+
+static int pmic5000_read_volt_thresholds(struct regmap *regmap, u32 attr,
+					 int channel, long *val)
+{
+	int err;
+	u32 set_reg, thresh_reg, range_bit;
+	u32 set_regval, thresh_regval, range_regval;
+	u32 volt_set;
+	int base_volts[2];
+
+	switch (channel) {
+	case 0:
+		set_reg = PMIC5000_REG_SWA_VOLT_SET;
+		thresh_reg = PMIC5000_REG_SWA_THRESH;
+		range_bit = PMIC5000_SWA_RANGE;
+		base_volts[0] = 800;
+		base_volts[1] = 600;
+		break;
+	case 1:
+		set_reg = PMIC5000_REG_SWB_VOLT_SET;
+		thresh_reg = PMIC5000_REG_SWB_THRESH;
+		range_bit = PMIC5000_SWB_RANGE;
+		base_volts[0] = 800;
+		base_volts[1] = 600;
+		break;
+	case 2:
+		set_reg = PMIC5000_REG_SWC_VOLT_SET;
+		thresh_reg = PMIC5000_REG_SWC_THRESH;
+		range_bit = PMIC5000_SWC_RANGE;
+		base_volts[0] = 800;
+		base_volts[1] = 600;
+		break;
+	case 3:
+		set_reg = PMIC5000_REG_SWD_VOLT_SET;
+		thresh_reg = PMIC5000_REG_SWD_THRESH;
+		range_bit = PMIC5000_SWD_RANGE;
+		base_volts[0] = 1500;
+		base_volts[1] = 2200;
+		break;
+	case 5:
+	case 6: {
+		err = regmap_read(regmap, PMIC5000_REG_THRES_AND_SEL,
+				  &thresh_regval);
+		if (err)
+			return err;
+		if (channel == 5) {
+			if (thresh_regval & PMIC5000_VIN_BULK_THRESH)
+				*val = 14500;
+			else
+				*val = 16000;
+			return 0;
+		} else {
+			if (thresh_regval & PMIC5000_VIN_MGMT_THRESH)
+				*val = 3800;
+			else
+				*val = 3700;
+			return 0;
+		}
+	}
+
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	err = regmap_read(regmap, set_reg, &set_regval);
+	if (err)
+		return err;
+	err = regmap_read(regmap, thresh_reg, &thresh_regval);
+	if (err)
+		return err;
+	err = regmap_read(regmap, PMIC5000_REG_SW_VOLT_RANGE, &range_regval);
+	if (err)
+		return err;
+
+	volt_set = range_regval & range_bit ? base_volts[1] : base_volts[0];
+	volt_set += (set_regval >> 1) * 5;
+
+	switch (attr) {
+	case hwmon_in_min:
+		/* 10%, 12.5%, Reserved, Reserved */
+		const int min_permilles[4] = { 100, 125, PERMILLE, PERMILLE };
+		*val = volt_set - (min_permilles[(thresh_regval >> 2) & 0x03] *
+				   volt_set / PERMILLE);
+		return 0;
+	case hwmon_in_max:
+		/* 7.5%, 10%, 12.5%, Reserved */
+		const int max_permilles[4] = { 75, 100, 125, PERMILLE };
+		*val = volt_set + (max_permilles[(thresh_regval >> 4) & 0x03] *
+				   volt_set / PERMILLE);
+		return 0;
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static int pmic5000_read_adc_alarms(struct regmap *regmap, u32 attr,
+				    int channel, long *val)
+{
+	int err;
+	u32 regval;
+
+	if (channel >= 0 && channel <= 3) {
+		switch (attr) {
+		case hwmon_in_min_alarm: {
+			err = regmap_read(regmap, PMIC5000_REG_UNDER_VOLTAGE,
+					  &regval);
+			if (err)
+				return err;
+			*val = regval >> (3 - channel) & 0x01;
+			return 0;
+		}
+		case hwmon_in_max_alarm: {
+			err = regmap_read(regmap, PMIC5000_REG_OVER_VOLTAGE,
+					  &regval);
+			if (err)
+				return err;
+			*val = regval >> (7 - channel) & 0x01;
+			return 0;
+		}
+		}
+	} else if (channel == 5 || channel == 6) {
+		switch (attr) {
+		case hwmon_in_max_alarm: {
+			err = regmap_read(regmap, PMIC5000_REG_OVER_VOLT_IN,
+					  &regval);
+			if (err)
+				return err;
+			*val = regval >> (channel - 4) & 0x01;
+			return 0;
+		}
+		}
+	}
+
+	return -EOPNOTSUPP;
+}
+
+static int pmic5000_read_adc(struct pmic5000_data *data, u32 attr, int channel,
+			     long *val)
+{
+	struct regmap *regmap = data->regmap;
+	int err, mult;
+	u32 regval;
+
+	switch (attr) {
+	case hwmon_in_enable:
+		err = pmic5000_check_regulator_enabled(regmap, channel);
+		if (err < 0)
+			return err;
+		*val = err;
+		return 0;
+	case hwmon_in_input:
+		break;
+	case hwmon_in_min:
+	case hwmon_in_max:
+		return pmic5000_read_volt_thresholds(regmap, attr, channel,
+						     val);
+	case hwmon_in_min_alarm:
+	case hwmon_in_max_alarm:
+		return pmic5000_read_adc_alarms(regmap, attr, channel, val);
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	/* Channel 4 is reserved */
+	if (channel < 0 || channel > 9 || channel == 4)
+		return -EOPNOTSUPP;
+
+	switch (channel) {
+	case 5:
+		mult = PMIC5000_VINBULK_UNIT;
+		break;
+	case 7:
+		mult = PMIC5000_VBIAS_UNIT;
+		break;
+	default:
+		mult = PMIC5000_VOLT_UNIT;
+		break;
+	}
+
+	mutex_lock(&data->adc_lock);
+
+	err = regmap_update_bits(regmap, PMIC5000_REG_ADC_CONFIG,
+				 PMIC5000_ADC_SELECT_MASK, channel << 3);
+	if (err)
+		goto error;
+
+	/*
+	 * The host shall wait minimum of 9 ms delay after the input selection
+	 * for ADC readout and the actual readout
+	 *
+	 * msleep may sleep for up to 20ms, which is fine.
+	 */
+	msleep(9);
+
+	err = regmap_read(regmap, PMIC5000_REG_ADC_VOLTAGE, &regval);
+	if (err)
+		goto error;
+
+	mutex_unlock(&data->adc_lock);
+
+	*val = regval * mult;
+	return 0;
+
+error:
+	mutex_unlock(&data->adc_lock);
+	return err;
+}
+
+static int pmic5000_read_interval(struct regmap *regmap, u32 attr, long *val)
+{
+	unsigned int regval;
+	int err;
+
+	if (attr != hwmon_chip_update_interval)
+		return -EOPNOTSUPP;
+
+	err = regmap_read(regmap, PMIC5000_REG_ADC_CONFIG, &regval);
+	if (err < 0)
+		return err;
+	*val = 1 << (regval & 0x03);
+	return 0;
+}
+
+static int pmic5000_read(struct device *dev, enum hwmon_sensor_types type,
+			 u32 attr, int channel, long *val)
+{
+	struct pmic5000_data *data = dev_get_drvdata(dev);
+	struct regmap *regmap = data->regmap;
+
+	switch (type) {
+	case hwmon_chip:
+		return pmic5000_read_interval(regmap, attr, val);
+	case hwmon_temp:
+		return pmic5000_read_temp(regmap, attr, channel, val);
+	case hwmon_in:
+		return pmic5000_read_adc(data, attr, channel, val);
+	case hwmon_curr:
+		return pmic5000_read_curr(data, attr, channel, val);
+	case hwmon_power:
+		return pmic5000_read_power(data, attr, channel, val);
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static int pmic5000_read_string(struct device *dev,
+				enum hwmon_sensor_types type, u32 attr,
+				int channel, const char **str)
+{
+	if (type == hwmon_curr && attr == hwmon_curr_label) {
+		if (channel < 0 || channel > 3)
+			return -EOPNOTSUPP;
+		*str = pmic5000_power_labels[channel];
+	} else if (type == hwmon_power && attr == hwmon_power_label) {
+		if (channel < 0 || channel > 3)
+			return -EOPNOTSUPP;
+		*str = pmic5000_power_labels[channel];
+	} else if (type == hwmon_in && attr == hwmon_in_label) {
+		if (channel < 0 || channel > 9 || channel == 4)
+			return -EOPNOTSUPP;
+		*str = pmic5000_voltage_labels[channel];
+	} else {
+		return -EOPNOTSUPP;
+	}
+
+	return 0;
+}
+
+static int pmic5000_write_interval(struct pmic5000_data *data, long val)
+{
+	struct regmap *regmap = data->regmap;
+	u32 regval;
+	int err;
+
+	switch (val) {
+	case 1:
+		regval = 0;
+		break;
+	case 2:
+		regval = 1;
+		break;
+	case 4:
+		regval = 2;
+		break;
+	case 8:
+		regval = 3;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	mutex_lock(&data->adc_lock);
+	err = regmap_update_bits(regmap, PMIC5000_REG_ADC_CONFIG, 0x03, regval);
+	mutex_unlock(&data->adc_lock);
+	return err;
+}
+
+static int pmic5000_write(struct device *dev, enum hwmon_sensor_types type,
+			  u32 attr, int channel, long val)
+{
+	struct pmic5000_data *data = dev_get_drvdata(dev);
+
+	switch (type) {
+	case hwmon_chip:
+		return pmic5000_write_interval(data, val);
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static umode_t pmic5000_is_visible(const void *data,
+				   enum hwmon_sensor_types type, u32 attr,
+				   int channel)
+{
+	int ret;
+	struct pmic5000_data *pmic_data = (struct pmic5000_data *)data;
+	struct regmap *regmap = pmic_data->regmap;
+
+	switch (type) {
+	case hwmon_chip:
+		if (attr == hwmon_chip_update_interval)
+			return 0644;
+		break;
+	case hwmon_temp:
+		return 0444;
+	case hwmon_in:
+		if (channel == 4)
+			return 0;
+		if (channel >= 0 && channel <= 3 && (attr != hwmon_in_enable)) {
+			ret = pmic5000_check_regulator_enabled(regmap, channel);
+			if (!ret || ret < 0)
+				return 0;
+		}
+		return 0444;
+	case hwmon_power:
+		if (channel >= 0 && channel <= 3) {
+			ret = pmic5000_check_regulator_enabled(regmap, channel);
+			if (!ret || ret < 0)
+				return 0;
+		}
+		return 0444;
+	case hwmon_curr:
+		if (channel >= 0 && channel <= 3) {
+			ret = pmic5000_check_regulator_enabled(regmap, channel);
+			if (!ret || ret < 0)
+				return 0;
+		}
+		return 0444;
+	default:
+		break;
+	}
+	return 0444;
+}
+
+/*
+ * Bank and vendor id are 8-bit fields with seven data bits and odd parity.
+ * Vendor IDs 0 and 0x7f are invalid.
+ * See Jedec standard JEP106BJ for details and a list of assigned vendor IDs.
+ */
+static bool pmic5000_vendor_valid(u8 bank, u8 id)
+{
+	if (parity8(bank) == 0 || parity8(id) == 0)
+		return false;
+
+	id &= 0x7f;
+	return id && id != 0x7f;
+}
+
+static const struct hwmon_channel_info *pmic5000_info[] = {
+	HWMON_CHANNEL_INFO(chip, HWMON_C_UPDATE_INTERVAL),
+	HWMON_CHANNEL_INFO(temp,
+			   HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_MAX_ALARM),
+	HWMON_CHANNEL_INFO(
+		in,
+		HWMON_I_ENABLE | HWMON_I_INPUT | HWMON_I_MIN | HWMON_I_MAX |
+			HWMON_I_MIN_ALARM | HWMON_I_MAX_ALARM | HWMON_I_LABEL,
+		HWMON_I_ENABLE | HWMON_I_INPUT | HWMON_I_MIN | HWMON_I_MAX |
+			HWMON_I_MIN_ALARM | HWMON_I_MAX_ALARM | HWMON_I_LABEL,
+		HWMON_I_ENABLE | HWMON_I_INPUT | HWMON_I_MIN | HWMON_I_MAX |
+			HWMON_I_MIN_ALARM | HWMON_I_MAX_ALARM | HWMON_I_LABEL,
+		HWMON_I_ENABLE | HWMON_I_INPUT | HWMON_I_MIN | HWMON_I_MAX |
+			HWMON_I_MIN_ALARM | HWMON_I_MAX_ALARM | HWMON_I_LABEL,
+		HWMON_I_INPUT,
+		HWMON_I_INPUT | HWMON_I_MAX | HWMON_I_MAX_ALARM | HWMON_I_LABEL,
+		HWMON_I_INPUT | HWMON_I_MAX | HWMON_I_MAX_ALARM | HWMON_I_LABEL,
+		HWMON_I_INPUT | HWMON_I_LABEL, HWMON_I_INPUT | HWMON_I_LABEL,
+		HWMON_I_INPUT | HWMON_I_LABEL),
+	HWMON_CHANNEL_INFO(
+		curr,
+		HWMON_C_INPUT | HWMON_C_MAX | HWMON_C_MAX_ALARM | HWMON_C_LABEL,
+		HWMON_C_INPUT | HWMON_C_MAX | HWMON_C_MAX_ALARM | HWMON_C_LABEL,
+		HWMON_C_INPUT | HWMON_C_MAX | HWMON_C_MAX_ALARM | HWMON_C_LABEL,
+		HWMON_C_INPUT | HWMON_C_MAX | HWMON_C_MAX_ALARM |
+			HWMON_C_LABEL),
+	HWMON_CHANNEL_INFO(power, HWMON_P_INPUT | HWMON_P_LABEL,
+			   HWMON_P_INPUT | HWMON_P_LABEL,
+			   HWMON_P_INPUT | HWMON_P_LABEL,
+			   HWMON_P_INPUT | HWMON_P_LABEL),
+	NULL
+};
+
+static const struct hwmon_ops pmic5000_hwmon_ops = {
+	.is_visible = pmic5000_is_visible,
+	.read = pmic5000_read,
+	.read_string = pmic5000_read_string,
+	.write = pmic5000_write,
+};
+
+static const struct hwmon_chip_info pmic5000_chip_info = {
+	.ops = &pmic5000_hwmon_ops,
+	.info = pmic5000_info,
+};
+
+/* regmap */
+
+static bool pmic5000_writeable_reg(struct device *dev, unsigned int reg)
+{
+	switch (reg) {
+	case PMIC5000_REG_OUTPUT_SELECT:
+	case PMIC5000_REG_THRES_AND_SEL:
+	case PMIC5000_REG_ADC_CONFIG:
+		return true;
+	default:
+		return false;
+	}
+}
+
+static bool pmic5000_volatile_reg(struct device *dev, unsigned int reg)
+{
+	switch (reg) {
+	case PMIC5000_REG_OVER_VOLT_IN:
+	case PMIC5000_REG_OVER_CURRENT:
+	case PMIC5000_REG_OVER_VOLTAGE:
+	case PMIC5000_REG_UNDER_VOLTAGE:
+	case PMIC5000_REG_SWA_POWER:
+	case PMIC5000_REG_SWB_POWER:
+	case PMIC5000_REG_SWC_POWER:
+	case PMIC5000_REG_SWD_POWER:
+	case PMIC5000_REG_ADC_VOLTAGE:
+	case PMIC5000_REG_TEMPERATURE:
+		return true;
+	default:
+		return false;
+	}
+}
+
+static const struct regmap_config pmic5000_regmap8_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+	.max_register = 0x3f,
+	.writeable_reg = pmic5000_writeable_reg,
+	.volatile_reg = pmic5000_volatile_reg,
+	.cache_type = REGCACHE_MAPLE,
+};
+
+static int pmic5000_suspend(struct device *dev)
+{
+	struct pmic5000_data *data = dev_get_drvdata(dev);
+	struct regmap *regmap = data->regmap;
+	u32 regval;
+	int err;
+
+	/*
+	 * Make sure the configuration register in the regmap cache is current
+	 * before bypassing it.
+	 */
+	err = regmap_read(regmap, PMIC5000_REG_ADC_CONFIG, &regval);
+	if (err < 0)
+		return err;
+
+	regcache_cache_bypass(regmap, true);
+	regmap_update_bits(regmap, PMIC5000_REG_ADC_CONFIG, PMIC5000_ADC_ENABLE,
+			   0);
+	regcache_cache_bypass(regmap, false);
+
+	regcache_cache_only(regmap, true);
+	regcache_mark_dirty(regmap);
+
+	return 0;
+}
+
+static int pmic5000_resume(struct device *dev)
+{
+	struct pmic5000_data *data = dev_get_drvdata(dev);
+	struct regmap *regmap = data->regmap;
+
+	regcache_cache_only(regmap, false);
+	return regcache_sync(regmap);
+}
+
+static DEFINE_SIMPLE_DEV_PM_OPS(pmic5000_pm_ops, pmic5000_suspend,
+				pmic5000_resume);
+
+static int pmic5000_common_probe(struct device *dev, struct regmap *regmap)
+{
+	unsigned int revision, vendor, bank;
+	struct pmic5000_data *data;
+	struct device *hwmon_dev;
+	int err;
+
+	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	err = regmap_read(regmap, PMIC5000_REG_REVISION, &revision);
+	if (err)
+		return err;
+
+	err = regmap_read(regmap, PMIC5000_REG_VENDOR, &bank);
+	if (err)
+		return err;
+	err = regmap_read(regmap, PMIC5000_REG_VENDOR + 1, &vendor);
+	if (err)
+		return err;
+	if (!pmic5000_vendor_valid(bank, vendor))
+		return -ENODEV;
+
+	data->regmap = regmap;
+	mutex_init(&data->mode_lock);
+	mutex_init(&data->adc_lock);
+	dev_set_drvdata(dev, data);
+
+	hwmon_dev = devm_hwmon_device_register_with_info(
+		dev, "pmic5000", data, &pmic5000_chip_info, NULL);
+	if (IS_ERR(hwmon_dev))
+		return PTR_ERR(hwmon_dev);
+
+	dev_info(dev, "DDR5 PMIC sensor: vendor 0x%02x:0x%02x revision %d.%d\n",
+		 bank & 0x7f, vendor, ((revision >> 4) & 0x03) + 1,
+		 ((revision >> 1) & 0x07) + 1);
+
+	/* Enable individual measurements and enable ADC */
+	err = regmap_update_bits(regmap, PMIC5000_REG_OUTPUT_SELECT,
+				 PMIC5000_OUTPUT_SELECT,
+				 PMIC5000_OUTPUT_SELECT);
+	if (err)
+		return err;
+	err = regmap_update_bits(regmap, PMIC5000_REG_ADC_CONFIG,
+				 PMIC5000_ADC_ENABLE, PMIC5000_ADC_ENABLE);
+	if (err)
+		return err;
+
+	return 0;
+}
+
+/* I2C */
+
+static int pmic5000_i2c_init(struct i2c_client *client)
+{
+	struct i2c_adapter *adapter = client->adapter;
+
+	/*
+	 * Register accesses are 8-bit, so require byte-data transactions only.
+	 * Requiring WORD_DATA here rejects otherwise valid adapters.
+	 */
+	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA))
+		return -ENODEV;
+
+	return 0;
+}
+
+static int pmic5000_i2c_probe(struct i2c_client *client)
+{
+	struct device *dev = &client->dev;
+	struct regmap *regmap;
+	int err;
+
+	err = pmic5000_i2c_init(client);
+	if (err)
+		return dev_err_probe(dev, err, "I2C capability check failed\n");
+
+	regmap = devm_regmap_init_i2c(client, &pmic5000_regmap8_config);
+	if (IS_ERR(regmap))
+		return dev_err_probe(dev, PTR_ERR(regmap),
+				     "regmap init failed\n");
+
+	return pmic5000_common_probe(dev, regmap);
+}
+
+static const struct i2c_device_id pmic5000_i2c_id[] = { { .name = "pmic5000" },
+							{} };
+MODULE_DEVICE_TABLE(i2c, pmic5000_i2c_id);
+
+static const struct of_device_id pmic5000_of_ids[] = {
+	{
+		.compatible = "jedec,pmic5000",
+	},
+	{}
+};
+MODULE_DEVICE_TABLE(of, pmic5000_of_ids);
+
+static struct i2c_driver pmic5000_i2c_driver = {
+	.class		= I2C_CLASS_HWMON,
+	.driver = {
+		.name	= "pmic5000",
+		.of_match_table = pmic5000_of_ids,
+		.pm = pm_sleep_ptr(&pmic5000_pm_ops),
+	},
+	.probe		= pmic5000_i2c_probe,
+	.id_table	= pmic5000_i2c_id,
+};
+
+module_i2c_driver(pmic5000_i2c_driver);
+
+MODULE_AUTHOR("Stephen Horvath <linux@stevetech.au>");
+MODULE_DESCRIPTION("PMIC5000 driver");
+MODULE_LICENSE("GPL");

---
base-commit: 75f2c0b3690702c90863c2e138cb5520670845ea
change-id: 20260819-pmic5000-3840f8a9ade1

Best regards,
--  
Stephen Horvath <linux@stevetech.au>


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH RFC] hwmon: Add Support for PMIC5000 Power Management ICs
  2026-08-19 13:37 [PATCH RFC] hwmon: Add Support for PMIC5000 Power Management ICs Stephen Horvath
@ 2026-08-19 13:51 ` sashiko-bot
  2026-08-20 15:34 ` Guenter Roeck
  2026-08-23 20:18 ` kernel test robot
  2 siblings, 0 replies; 7+ messages in thread
From: sashiko-bot @ 2026-08-19 13:51 UTC (permalink / raw)
  To: Stephen Horvath; +Cc: linux-hwmon

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Medium] Race condition: sysfs registration before hardware initialization
- [Medium] Incorrect error code for out-of-range values
- [Medium] hwmon update_interval attribute violates ABI units (returns raw values instead of milliseconds)
- [Medium] Architectural layering violation: PMIC directly registered as hwmon I2C driver
- [Low] C11 syntax error due to declaration directly after a label
--

--- Patch [1]: [PATCH RFC] hwmon: Add Support for PMIC5000 Power Management ICs ---
commit 4dd6cacf5a9ff70a4083c91afc8539b2aae42832
Author: Stephen Horvath <linux@stevetech.au>

hwmon: Add Support for PMIC5000 Power Management ICs

Add support for sensors present on PMIC5000 (JEDEC JESD301) compliant
power management ICs. These chips are commonly found on DDR5 memory
modules.

> diff --git a/drivers/hwmon/pmic5000.c b/drivers/hwmon/pmic5000.c
> index 0000000000000..d3f3126dd9c80
> --- /dev/null
> +++ b/drivers/hwmon/pmic5000.c

[ ... ]

> +static int pmic5000_read_temp(struct regmap *regmap, u32 attr, int channel,
> +			      long *val)
> +{
> +	int err;
> +	u32 regval;
> +
> +	if (channel != 0)
> +		return -EOPNOTSUPP;
> +
> +	switch (attr) {
> +	case hwmon_temp_input: {
> +		err = regmap_read(regmap, PMIC5000_REG_TEMPERATURE, &regval);
> +		if (err)
> +			return err;
> +		regval >>= 5;
> +		/* Below 85°C */
> +		if (regval == 0)
> +			return -EOPNOTSUPP;

[Severity: Medium]
Since returning -EOPNOTSUPP causes userspace tools to treat the attribute 
as permanently unsupported by the chip, should this return -ENODATA instead 
when the temperature is temporarily out of range?

> +		/* 0b001 = 85°C, 0b010 = 95°C, etc. */
> +		*val = (75 + regval * 10) * MILLIDEGREE_PER_DEGREE;
> +		return 0;
> +	}

[ ... ]

> +static int pmic5000_read_volt_thresholds(struct regmap *regmap, u32 attr,
> +					 int channel, long *val)
> +{

[ ... ]

> +	volt_set = range_regval & range_bit ? base_volts[1] : base_volts[0];
> +	volt_set += (set_regval >> 1) * 5;
> +
> +	switch (attr) {
> +	case hwmon_in_min:
> +		/* 10%, 12.5%, Reserved, Reserved */
> +		const int min_permilles[4] = { 100, 125, PERMILLE, PERMILLE };

[Severity: Low]
Can declaring a variable immediately following a case label cause a build 
failure? The C11 standard expects a statement after a label, rather than 
a declaration. Would wrapping the contents of this case block in braces 
resolve this syntax error?

> +		*val = volt_set - (min_permilles[(thresh_regval >> 2) & 0x03] *
> +				   volt_set / PERMILLE);
> +		return 0;
> +	case hwmon_in_max:
> +		/* 7.5%, 10%, 12.5%, Reserved */
> +		const int max_permilles[4] = { 75, 100, 125, PERMILLE };
> +		*val = volt_set + (max_permilles[(thresh_regval >> 4) & 0x03] *
> +				   volt_set / PERMILLE);
> +		return 0;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +}

[ ... ]

> +static int pmic5000_read_interval(struct regmap *regmap, u32 attr, long *val)
> +{
> +	unsigned int regval;
> +	int err;
> +
> +	if (attr != hwmon_chip_update_interval)
> +		return -EOPNOTSUPP;
> +
> +	err = regmap_read(regmap, PMIC5000_REG_ADC_CONFIG, &regval);
> +	if (err < 0)
> +		return err;
> +	*val = 1 << (regval & 0x03);
> +	return 0;
> +}

[Severity: Medium]
Is this exposing raw values like 1, 2, 4, or 8 instead of milliseconds? 
The hwmon ABI specifies that update_interval should be reported and set 
in milliseconds. Do these values need to be translated to match the ABI 
requirements?

[ ... ]

> +static int pmic5000_common_probe(struct device *dev, struct regmap *regmap)
> +{
> +	unsigned int revision, vendor, bank;
> +	struct pmic5000_data *data;
> +	struct device *hwmon_dev;
> +	int err;

[ ... ]

> +	hwmon_dev = devm_hwmon_device_register_with_info(
> +		dev, "pmic5000", data, &pmic5000_chip_info, NULL);
> +	if (IS_ERR(hwmon_dev))
> +		return PTR_ERR(hwmon_dev);
> +
> +	dev_info(dev, "DDR5 PMIC sensor: vendor 0x%02x:0x%02x revision %d.%d\n",
> +		 bank & 0x7f, vendor, ((revision >> 4) & 0x03) + 1,
> +		 ((revision >> 1) & 0x07) + 1);
> +
> +	/* Enable individual measurements and enable ADC */
> +	err = regmap_update_bits(regmap, PMIC5000_REG_OUTPUT_SELECT,
> +				 PMIC5000_OUTPUT_SELECT,
> +				 PMIC5000_OUTPUT_SELECT);
> +	if (err)
> +		return err;
> +	err = regmap_update_bits(regmap, PMIC5000_REG_ADC_CONFIG,
> +				 PMIC5000_ADC_ENABLE, PMIC5000_ADC_ENABLE);

[Severity: Medium]
Does this expose the sysfs files before the ADC is actually enabled? If 
userspace polling tools access the sysfs attributes right after 
devm_hwmon_device_register_with_info() completes, could it trigger reads on 
uninitialized hardware? Should the hardware initialization happen before 
registration?

[ ... ]

> +static struct i2c_driver pmic5000_i2c_driver = {
> +	.class		= I2C_CLASS_HWMON,
> +	.driver = {
> +		.name	= "pmic5000",
> +		.of_match_table = pmic5000_of_ids,
> +		.pm = pm_sleep_ptr(&pmic5000_pm_ops),
> +	},
> +	.probe		= pmic5000_i2c_probe,
> +	.id_table	= pmic5000_i2c_id,
> +};
> +
> +module_i2c_driver(pmic5000_i2c_driver);

[Severity: Medium]
Is this driver placed in the correct subsystem? For PMIC chips where hardware 
monitoring isn't the primary function, the hwmon subsystem guidelines state 
the core driver should reside in drivers/mfd or drivers/regulator, and 
instantiate the hwmon functionality as an auxiliary device. Does binding 
directly to the I2C device here prevent regulator drivers from attaching to 
this PMIC later?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260819-pmic5000-v1-1-e5e0ddcf0863@stevetech.au?part=1

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH RFC] hwmon: Add Support for PMIC5000 Power Management ICs
  2026-08-19 13:37 [PATCH RFC] hwmon: Add Support for PMIC5000 Power Management ICs Stephen Horvath
  2026-08-19 13:51 ` sashiko-bot
@ 2026-08-20 15:34 ` Guenter Roeck
  2026-08-21  3:27   ` Stephen Horvath
  2026-08-23 20:18 ` kernel test robot
  2 siblings, 1 reply; 7+ messages in thread
From: Guenter Roeck @ 2026-08-20 15:34 UTC (permalink / raw)
  To: Stephen Horvath
  Cc: Andi Shyti, linux-hwmon, linux-i2c, linux-kernel, Stephen Horvath

On Wed, Aug 19, 2026 at 11:37:45PM +1000, Stephen Horvath wrote:
> Add support for sensors present on PMIC5000 (JEDEC JESD301) compliant
> power management ICs. These chips are commonly found on DDR5 memory
> modules.
> 
> Tested against PMIC5100 chips, but only features present in the
> PMIC5000 (JESD301-1) specification have been implemented.
> 
> Signed-off-by: Stephen Horvath <linux@stevetech.au>
> ---
> Hi maintainers, I wrote this over the past couple days and I'm looking
> for feedback.
> 
> Here's a few concerns I have:
> - I'm not too experienced with kernel development.
> - I don't think there's a good way to auto-detect PMIC5000 chips 
> 	standalone, would it be reasonable to instantiate it from
> 	spd5118?
> - I'm unsure what to do with temperatures below 85°C, I'd rather not
> 	return 85 as that would make it seem hotter than it is, but
> 	returning -EOPNOTSUPP seems wrong.
> - Toggling between current and power seems like it's a bit of a hack,
> 	is that okay, or should I just pick one and drop the other?
> - Does this belong in HWMON, or should it be part of the regulator
> 	subsystem?

It depends on its functionality. Is this just for monitoring, or are the voltages
controllable ?


> ---
>  .../devicetree/bindings/trivial-devices.yaml       |   2 +
>  Documentation/hwmon/index.rst                      |   1 +
>  Documentation/hwmon/pmic5000.rst                   | 103 +++
>  drivers/hwmon/Kconfig                              |  12 +
>  drivers/hwmon/Makefile                             |   1 +
>  drivers/hwmon/pmic5000.c                           | 896 +++++++++++++++++++++
>  6 files changed, 1015 insertions(+)
> 
> 
> ---
> base-commit: 75f2c0b3690702c90863c2e138cb5520670845ea
> change-id: 20260819-pmic5000-3840f8a9ade1
> 
> Best regards,
> --  
> Stephen Horvath <linux@stevetech.au>
> 
> diff --git a/Documentation/devicetree/bindings/trivial-devices.yaml b/Documentation/devicetree/bindings/trivial-devices.yaml
> index 2de8eb09cb7d..86e76be7d41b 100644
> --- a/Documentation/devicetree/bindings/trivial-devices.yaml
> +++ b/Documentation/devicetree/bindings/trivial-devices.yaml
> @@ -194,6 +194,8 @@ properties:
>            - isil,isl29030
>              # Intersil ISL76682 Ambient Light Sensor
>            - isil,isl76682
> +            # JEDEC JESD301 (PMIC5000) Power Management IC
> +          - jedec,pmic5000
>              # JEDEC JESD300 (SPD5118) Hub and Serial Presence Detect
>            - jedec,spd5118
>              # Kandou KB9002 PCIe 5.0 retimer
> diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
> index d979e6d6e9f2..4bb518d1927c 100644
> --- a/Documentation/hwmon/index.rst
> +++ b/Documentation/hwmon/index.rst
> @@ -226,6 +226,7 @@ Hardware Monitoring Kernel Drivers
>     peci-cputemp
>     peci-dimmtemp
>     pmbus
> +   pmic5000
>     powerz
>     powr1220
>     prom21-xhci
> diff --git a/Documentation/hwmon/pmic5000.rst b/Documentation/hwmon/pmic5000.rst
> new file mode 100644
> index 000000000000..e7d864088ba5
> --- /dev/null
> +++ b/Documentation/hwmon/pmic5000.rst
> @@ -0,0 +1,103 @@
> +.. SPDX-License-Identifier: GPL-2.0-or-later
> +
> +Kernel driver pmic5000
> +=====================
> +
> +Supported chips:
> +
> +  * PMIC5000 (JEDEC JESD301) compliant power management chips
> +
> +    JEDEC standard download:
> +	https://www.jedec.org/standards-documents/docs/jesd301-1a03
> +	(account required)
> +
> +
> +    Prefix: 'pmic5000'
> +
> +Author:
> +	Stephen Horvath <linux@stevetech.au>
> +
> +
> +Description
> +-----------
> +
> +This driver implements support for PMIC5000 (JEDEC JESD301) compliant power
> +management chips, which are used on many DDR5 memory modules.
> +
> +The driver does not auto-detect PMIC5000 compliant chips, and must be
> +instantiated manually or via device tree. The address of these chips are
> +usually between (inclusive) 0x48 and 0x4F.
> +
> +
> +Hardware monitoring sysfs entries
> +---------------------------------
> +
> +======================= ==================================
> +in0_enable		Whether SWA is enabled (RO)

Does that enable the voltage or its monitoriong ? The enable attribute is
only to enable monitoring, not to enable the voltage. If this is a
controllable voltage, the driver should be a regulator driver, as Sashiko
suggested.

> +in0_input		SWA Voltage (RO)
> +in0_min			Minimum SWA Voltage (RO)
> +in0_max			Maximum SWA Voltage (RO)
> +in0_min_alarm		SWA Voltage low alarm
> +in0_max_alarm		SWA Voltage high alarm
> +
> +in1_enable		Whether SWB is enabled (RO)
> +in1_input		SWB Voltage (RO)
> +in1_min			Minimum SWB Voltage (RO)
> +in1_max			Maximum SWB Voltage (RO)
> +in1_min_alarm		SWB Voltage low alarm
> +in1_max_alarm		SWB Voltage high alarm
> +
> +in2_enable		Whether SWC is enabled (RO)
> +in2_input		SWC Voltage (RO)
> +in2_min			Minimum SWC Voltage (RO)
> +in2_max			Maximum SWC Voltage (RO)
> +in2_min_alarm		SWC Voltage low alarm
> +in2_max_alarm		SWC Voltage high alarm
> +
> +in3_enable		Whether SWD is enabled (RO)
> +in3_input		SWD Voltage (RO)
> +in3_min			Minimum SWD Voltage (RO)
> +in3_max			Maximum SWD Voltage (RO)
> +in3_min_alarm		SWD Voltage low alarm
> +in3_max_alarm		SWD Voltage high alarm
> +
> +in5_input		VIN_Bulk Voltage (RO)
> +in5_max			Maximum VIN_Bulk Voltage (RO)
> +in5_max_alarm		VIN_Bulk Voltage high alarm
> +
> +in6_input		VIN_Mgmt Voltage (RO)
> +in6_max			Maximum VIN_Mgmt Voltage (RO)
> +in6_max_alarm		VIN_Mgmt Voltage high alarm
> +
> +in7_input		VBias Voltage (RO)
> +in8_input		VOUT_1.8V Voltage (RO)
> +in9_input		VOUT_1.0V Voltage (RO)
> +
> +temp1_input		Temperature (RO)
> +temp1_max		Maximum temperature (RO)
> +temp1_max_alarm		Temperature high alarm
> +
> +power1_input 		SWA Wattage (RO)
> +power2_input 		SWB Wattage (RO)
> +power3_input 		SWC Wattage (RO)
> +power4_input 		SWD Wattage (RO)
> +
> +curr1_input		SWA Current (RO)
> +curr1_max		Maximum SWA Current (RO)
> +curr1_max_alarm		SWA Current high alarm
> +
> +curr2_input		SWB Current (RO)
> +curr2_max		Maximum SWB Current (RO)
> +curr2_max_alarm		SWB Current high alarm
> +
> +curr3_input		SWC Current (RO)
> +curr3_max		Maximum SWC Current (RO)
> +curr3_max_alarm		SWC Current high alarm
> +
> +curr4_input		SWD Current (RO)
> +curr4_max		Maximum SWD Current (RO)
> +curr4_max_alarm		SWD Current high alarm
> +
> +update_interval		Update interval in milliseconds (RW), Allowed values: 1, 2, 4, 8
> +
> +======================= ==================================
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 81a9a1d40eec..5ed5f9499c43 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -950,6 +950,18 @@ config SENSORS_JC42
>  	  This driver can also be built as a module. If so, the module
>  	  will be called jc42.
>  
> +config SENSORS_PMIC5000
> +	tristate "PMIC5000 Compliant Sensors"
> +	depends on I2C
> +	select REGMAP_I2C
> +	help
> +	  If you say yes here you get support for PMIC5000 (JEDEC JESD301)
> +	  compliant sensors. Such sensors are found on DDR5 memory
> +	  modules.
> +
> +	  This driver can also be built as a module. If so, the module
> +	  will be called pmic5000.
> +
>  config SENSORS_POLARFIRE_SOC_TVS
>  	tristate "PolarFire SoC (MPFS) temperature and voltage sensor"
>  	depends on POLARFIRE_SOC_SYSCONS || COMPILE_TEST
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index 0cad7e21634c..ac519123796f 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -199,6 +199,7 @@ obj-$(CONFIG_SENSORS_NZXT_SMART2) += nzxt-smart2.o
>  obj-$(CONFIG_SENSORS_PC87360)	+= pc87360.o
>  obj-$(CONFIG_SENSORS_PC87427)	+= pc87427.o
>  obj-$(CONFIG_SENSORS_PCF8591)	+= pcf8591.o
> +obj-$(CONFIG_SENSORS_PMIC5000)	+= pmic5000.o
>  obj-$(CONFIG_SENSORS_POLARFIRE_SOC_TVS)  += tvs-mpfs.o
>  obj-$(CONFIG_SENSORS_POWERZ)	+= powerz.o
>  obj-$(CONFIG_SENSORS_POWR1220)  += powr1220.o
> diff --git a/drivers/hwmon/pmic5000.c b/drivers/hwmon/pmic5000.c
> new file mode 100644
> index 000000000000..d3f3126dd9c8
> --- /dev/null
> +++ b/drivers/hwmon/pmic5000.c
> @@ -0,0 +1,896 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Driver for Jedec PMIC5000 compliant sensors
> + *
> + * Copyright (c) 2026 Stephen Horvath
> + *
> + * Inspired by spd5118.c.
> + *
> + * PMIC5000 compliant sensors are typically used on DDR5 memory modules.
> + */
> +
> +#include <linux/bitops.h>
> +#include <linux/bits.h>
> +#include <linux/err.h>
> +#include <linux/hwmon.h>
> +#include <linux/i2c.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/pm.h>
> +#include <linux/regmap.h>
> +#include <linux/units.h>
> +
> +/* PMIC5000 registers. */
> +// clang-format off

No way. Are you serious ?

> +#define PMIC5000_REG_OVER_VOLT_IN	0x08
> +#define PMIC5000_REG_OVER_CURRENT	0x09
> +	#define PMIC5000_HIGH_TEMP		BIT(7)
> +#define PMIC5000_REG_OVER_VOLTAGE	0x0A
> +#define PMIC5000_REG_UNDER_VOLTAGE	0x0B
> +#define PMIC5000_REG_SWA_POWER		0x0C
> +#define PMIC5000_REG_SWB_POWER		0x0D
> +#define PMIC5000_REG_SWC_POWER		0x0E
> +#define PMIC5000_REG_SWD_POWER		0x0F
> +#define PMIC5000_REG_OUTPUT_SELECT	0x1A
> +	#define PMIC5000_OUTPUT_SELECT		BIT(1)
> +#define PMIC5000_REG_THRES_AND_SEL	0x1B
> +	#define PMIC5000_VIN_MGMT_THRESH	BIT(5)
> +	#define PMIC5000_CURR_OR_PWR		BIT(6)
> +	#define PMIC5000_VIN_BULK_THRESH	BIT(7)
> +#define PMIC5000_REG_SWA_CURR_WARN	0x1C
> +#define PMIC5000_REG_SWB_CURR_WARN	0x1D
> +#define PMIC5000_REG_SWC_CURR_WARN	0x1E
> +#define PMIC5000_REG_SWD_CURR_WARN	0x1F
> +#define PMIC5000_REG_SWA_VOLT_SET	0x21
> +#define PMIC5000_REG_SWA_THRESH		0x22
> +#define PMIC5000_REG_SWB_VOLT_SET	0x23
> +#define PMIC5000_REG_SWB_THRESH		0x24
> +#define PMIC5000_REG_SWC_VOLT_SET	0x25
> +#define PMIC5000_REG_SWC_THRESH		0x26
> +#define PMIC5000_REG_SWD_VOLT_SET	0x27
> +#define PMIC5000_REG_SWD_THRESH		0x28
> +#define PMIC5000_REG_SW_VOLT_RANGE	0x2B
> +	#define PMIC5000_SWD_RANGE		BIT(0)
> +	#define PMIC5000_SWC_RANGE		BIT(3)
> +	#define PMIC5000_SWB_RANGE		BIT(4)
> +	#define PMIC5000_SWA_RANGE		BIT(5)
> +#define PMIC5000_REG_REGULATOR_CONTROL	0x2F
> +	#define PMIC5000_SWD_CONTROL		BIT(3)
> +	#define PMIC5000_SWC_CONTROL		BIT(4)
> +	#define PMIC5000_SWB_CONTROL		BIT(5)
> +	#define PMIC5000_SWA_CONTROL		BIT(6)
> +#define PMIC5000_REG_ADC_CONFIG		0x30
> +	#define PMIC5000_ADC_SELECT_MASK	GENMASK(6, 3)
> +	#define PMIC5000_ADC_ENABLE		BIT(7)
> +#define PMIC5000_REG_ADC_VOLTAGE	0x31
> +#define PMIC5000_REG_TEMPERATURE	0x33
> +#define PMIC5000_REG_REVISION		0x3B
> +#define PMIC5000_REG_VENDOR		0x3C
> +
> +

Please run checkpatch --strict on your patches and fix what it reports.

> +/* 125 mA multiplier */
> +#define PMIC5000_CURR_UNIT		125
> +/* 125 mW multiplier */
> +#define PMIC5000_POWER_UNIT		(125 * MILLIWATT_PER_WATT)
> +/* mV multipliers */
> +#define PMIC5000_VOLT_UNIT		15
> +#define PMIC5000_VINBULK_UNIT		70
> +#define PMIC5000_VBIAS_UNIT		25
> +// clang-format on
> +
> +struct pmic5000_data {
> +	struct regmap *regmap;
> +	struct mutex mode_lock;
> +	struct mutex adc_lock;

Explain why those are needed on top of the hwmon subsystem lock.

> +};
> +
> +static const char *const pmic5000_power_labels[] = { "SWA", "SWB", "SWC",
> +						     "SWD" };
> +
> +static const char *const pmic5000_voltage_labels[] = {
> +	"SWA",	    "SWB",	"SWC",	 "SWD",	      NULL,
> +	"VIN_Bulk", "VIN_Mgmt", "VBias", "VOUT_1.8V", "VOUT_1.0V"
> +};
> +
> +/* hwmon */
> +
> +static int pmic5000_check_regulator_enabled(struct regmap *regmap, int channel)

Pretty clear indication that this should be a regulator driver.

> +{
> +	u32 regval;
> +	int err;
> +
> +	err = regmap_read(regmap, PMIC5000_REG_REGULATOR_CONTROL, &regval);
> +	if (err)
> +		return err;
> +	switch (channel) {
> +	case 0:
> +		return !!(regval & PMIC5000_SWA_CONTROL);
> +	case 1:
> +		return !!(regval & PMIC5000_SWB_CONTROL);
> +	case 2:
> +		return !!(regval & PMIC5000_SWC_CONTROL);
> +	case 3:
> +		return !!(regval & PMIC5000_SWD_CONTROL);
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +}
> +
> +static int pmic5000_read_temp(struct regmap *regmap, u32 attr, int channel,
> +			      long *val)
> +{
> +	int err;
> +	u32 regval;
> +
> +	if (channel != 0)
> +		return -EOPNOTSUPP;

Unnecessary check.

> +
> +	switch (attr) {
> +	case hwmon_temp_input: {
> +		err = regmap_read(regmap, PMIC5000_REG_TEMPERATURE, &regval);
> +		if (err)
> +			return err;
> +		regval >>= 5;
> +		/* Below 85°C */
> +		if (regval == 0)
> +			return -EOPNOTSUPP;
> +		/* 0b001 = 85°C, 0b010 = 95°C, etc. */
> +		*val = (75 + regval * 10) * MILLIDEGREE_PER_DEGREE;
> +		return 0;
> +	}
> +	case hwmon_temp_max: {
> +		err = regmap_read(regmap, PMIC5000_REG_THRES_AND_SEL, &regval);
> +		if (err)
> +			return err;
> +		regval &= 0x07;
> +		/* Reserved */
> +		if (regval == 0 || regval == 0x7)
> +			return -EOPNOTSUPP;
> +		*val = (75 + regval * 10) * MILLIDEGREE_PER_DEGREE;
> +		return 0;
> +	}
> +	case hwmon_temp_max_alarm: {
> +		err = regmap_read(regmap, PMIC5000_REG_OVER_CURRENT, &regval);
> +		if (err)
> +			return err;
> +		*val = !!(regval & PMIC5000_HIGH_TEMP);
> +		return 0;
> +	}
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +}
> +
> +static int pmic5000_read_curr(struct pmic5000_data *data, u32 attr, int channel,
> +			      long *val)
> +{
> +	struct regmap *regmap = data->regmap;
> +	int reg, err;
> +	int shift = 0;
> +	u32 regval;
> +
> +	if (attr == hwmon_curr_input) {
> +		mutex_lock(&data->mode_lock);

I don't see why this is needed on top of the hwmon subsystem lock.
Same for other usage of this lock.

> +		/* Select power measurements */
> +		err = regmap_update_bits(regmap, PMIC5000_REG_THRES_AND_SEL,
> +					 PMIC5000_CURR_OR_PWR, 0);
> +		if (err)
> +			goto error;
> +
> +		switch (channel) {
> +		case 0:
> +			reg = PMIC5000_REG_SWA_POWER;
> +			break;
> +		case 1:
> +			reg = PMIC5000_REG_SWB_POWER;
> +			break;
> +		case 2:
> +			reg = PMIC5000_REG_SWC_POWER;
> +			break;
> +		case 3:
> +			reg = PMIC5000_REG_SWD_POWER;
> +			break;
> +		default:
> +			err = -EOPNOTSUPP;
> +			goto error;
> +		}
> +	} else if (attr == hwmon_curr_max) {
> +		shift = 2;
> +		switch (channel) {
> +		case 0:
> +			reg = PMIC5000_REG_SWA_CURR_WARN;
> +			break;
> +		case 1:
> +			reg = PMIC5000_REG_SWB_CURR_WARN;
> +			break;
> +		case 2:
> +			reg = PMIC5000_REG_SWC_CURR_WARN;
> +			break;
> +		case 3:
> +			reg = PMIC5000_REG_SWD_CURR_WARN;
> +			break;
> +		default:
> +			return -EOPNOTSUPP;
> +		}
> +	} else if (attr == hwmon_curr_max_alarm && channel >= 0 &&
> +		   channel <= 3) {

Unnecessary channel checks (at least if the is_visible function does its job
and the info data is correct).

> +		err = regmap_read(regmap, PMIC5000_REG_OVER_CURRENT, &regval);
> +		if (err)
> +			return err;
> +		*val = regval >> (3 - channel) & 0x01;
> +		return 0;
> +	} else {
> +		return -EOPNOTSUPP;
> +	}
> +
> +	err = regmap_read(regmap, reg, &regval);
> +	if (err)
> +		goto error;
> +
> +	if (attr == hwmon_curr_input)
> +		mutex_unlock(&data->mode_lock);
> +
> +	*val = regval * PMIC5000_CURR_UNIT >> shift;
> +	return 0;
> +
> +error:
> +	if (attr == hwmon_curr_input)
> +		mutex_unlock(&data->mode_lock);
> +	return err;
> +}
> +
> +static int pmic5000_read_power(struct pmic5000_data *data, u32 attr,
> +			       int channel, long *val)
> +{
> +	struct regmap *regmap = data->regmap;
> +	int reg, err;
> +	u32 regval;
> +
> +	if (attr != hwmon_power_input)
> +		return -EOPNOTSUPP;
> +
> +	mutex_lock(&data->mode_lock);
> +	/* Select power measurements */
> +	err = regmap_update_bits(regmap, PMIC5000_REG_THRES_AND_SEL,
> +				 PMIC5000_CURR_OR_PWR, PMIC5000_CURR_OR_PWR);
> +	if (err)
> +		goto error;
> +
> +	switch (channel) {
> +	case 0:
> +		reg = PMIC5000_REG_SWA_POWER;
> +		break;
> +	case 1:
> +		reg = PMIC5000_REG_SWB_POWER;
> +		break;
> +	case 2:
> +		reg = PMIC5000_REG_SWC_POWER;
> +		break;
> +	case 3:
> +		reg = PMIC5000_REG_SWD_POWER;
> +		break;
> +	default:
> +		err = -EOPNOTSUPP;
> +		goto error;
> +	}
> +
> +	err = regmap_read(regmap, reg, &regval);
> +	if (err)
> +		goto error;
> +
> +	mutex_unlock(&data->mode_lock);
> +
> +	*val = regval * PMIC5000_POWER_UNIT;
> +	return 0;
> +
> +error:
> +	mutex_unlock(&data->mode_lock);
> +	return err;
> +}
> +
> +static int pmic5000_read_volt_thresholds(struct regmap *regmap, u32 attr,
> +					 int channel, long *val)
> +{
> +	int err;
> +	u32 set_reg, thresh_reg, range_bit;
> +	u32 set_regval, thresh_regval, range_regval;
> +	u32 volt_set;
> +	int base_volts[2];
> +
> +	switch (channel) {
> +	case 0:
> +		set_reg = PMIC5000_REG_SWA_VOLT_SET;
> +		thresh_reg = PMIC5000_REG_SWA_THRESH;
> +		range_bit = PMIC5000_SWA_RANGE;
> +		base_volts[0] = 800;
> +		base_volts[1] = 600;
> +		break;
> +	case 1:
> +		set_reg = PMIC5000_REG_SWB_VOLT_SET;
> +		thresh_reg = PMIC5000_REG_SWB_THRESH;
> +		range_bit = PMIC5000_SWB_RANGE;
> +		base_volts[0] = 800;
> +		base_volts[1] = 600;
> +		break;
> +	case 2:
> +		set_reg = PMIC5000_REG_SWC_VOLT_SET;
> +		thresh_reg = PMIC5000_REG_SWC_THRESH;
> +		range_bit = PMIC5000_SWC_RANGE;
> +		base_volts[0] = 800;
> +		base_volts[1] = 600;
> +		break;
> +	case 3:
> +		set_reg = PMIC5000_REG_SWD_VOLT_SET;
> +		thresh_reg = PMIC5000_REG_SWD_THRESH;
> +		range_bit = PMIC5000_SWD_RANGE;
> +		base_volts[0] = 1500;
> +		base_volts[1] = 2200;
> +		break;
> +	case 5:
> +	case 6: {

Why "{" ?

> +		err = regmap_read(regmap, PMIC5000_REG_THRES_AND_SEL,
> +				  &thresh_regval);
> +		if (err)
> +			return err;
> +		if (channel == 5) {
> +			if (thresh_regval & PMIC5000_VIN_BULK_THRESH)
> +				*val = 14500;
> +			else
> +				*val = 16000;
> +			return 0;
> +		} else {
> +			if (thresh_regval & PMIC5000_VIN_MGMT_THRESH)
> +				*val = 3800;
> +			else
> +				*val = 3700;
> +			return 0;
> +		}
> +	}
> +
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +
> +	err = regmap_read(regmap, set_reg, &set_regval);
> +	if (err)
> +		return err;
> +	err = regmap_read(regmap, thresh_reg, &thresh_regval);
> +	if (err)
> +		return err;
> +	err = regmap_read(regmap, PMIC5000_REG_SW_VOLT_RANGE, &range_regval);
> +	if (err)
> +		return err;
> +
> +	volt_set = range_regval & range_bit ? base_volts[1] : base_volts[0];
> +	volt_set += (set_regval >> 1) * 5;
> +
> +	switch (attr) {
> +	case hwmon_in_min:
> +		/* 10%, 12.5%, Reserved, Reserved */
> +		const int min_permilles[4] = { 100, 125, PERMILLE, PERMILLE };
> +		*val = volt_set - (min_permilles[(thresh_regval >> 2) & 0x03] *
> +				   volt_set / PERMILLE);
> +		return 0;
> +	case hwmon_in_max:
> +		/* 7.5%, 10%, 12.5%, Reserved */
> +		const int max_permilles[4] = { 75, 100, 125, PERMILLE };
> +		*val = volt_set + (max_permilles[(thresh_regval >> 4) & 0x03] *
> +				   volt_set / PERMILLE);
> +		return 0;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +}
> +
> +static int pmic5000_read_adc_alarms(struct regmap *regmap, u32 attr,
> +				    int channel, long *val)
> +{
> +	int err;
> +	u32 regval;
> +
> +	if (channel >= 0 && channel <= 3) {

channel is always >= 0.

> +		switch (attr) {
> +		case hwmon_in_min_alarm: {
> +			err = regmap_read(regmap, PMIC5000_REG_UNDER_VOLTAGE,
> +					  &regval);
> +			if (err)
> +				return err;
> +			*val = regval >> (3 - channel) & 0x01;
> +			return 0;
> +		}
> +		case hwmon_in_max_alarm: {
> +			err = regmap_read(regmap, PMIC5000_REG_OVER_VOLTAGE,
> +					  &regval);
> +			if (err)
> +				return err;
> +			*val = regval >> (7 - channel) & 0x01;
> +			return 0;
> +		}
> +		}

Alignment.

> +	} else if (channel == 5 || channel == 6) {
> +		switch (attr) {
> +		case hwmon_in_max_alarm: {
> +			err = regmap_read(regmap, PMIC5000_REG_OVER_VOLT_IN,
> +					  &regval);
> +			if (err)
> +				return err;
> +			*val = regval >> (channel - 4) & 0x01;
> +			return 0;
> +		}
> +		}
> +	}
> +
> +	return -EOPNOTSUPP;
> +}
> +
> +static int pmic5000_read_adc(struct pmic5000_data *data, u32 attr, int channel,
> +			     long *val)
> +{
> +	struct regmap *regmap = data->regmap;
> +	int err, mult;
> +	u32 regval;
> +
> +	switch (attr) {
> +	case hwmon_in_enable:
> +		err = pmic5000_check_regulator_enabled(regmap, channel);
> +		if (err < 0)
> +			return err;
> +		*val = err;
> +		return 0;

The enable attribute is supposed to enable or disable monitoring.
It is not supposed to report regulator status.

> +	case hwmon_in_input:
> +		break;
> +	case hwmon_in_min:
> +	case hwmon_in_max:
> +		return pmic5000_read_volt_thresholds(regmap, attr, channel,
> +						     val);
> +	case hwmon_in_min_alarm:
> +	case hwmon_in_max_alarm:
> +		return pmic5000_read_adc_alarms(regmap, attr, channel, val);
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +
> +	/* Channel 4 is reserved */
> +	if (channel < 0 || channel > 9 || channel == 4)
> +		return -EOPNOTSUPP;

Then it should not be enabled by the is_visible function.
Also, channel is never < 0 or > 9.

> +
> +	switch (channel) {
 +	case 5:
> +		mult = PMIC5000_VINBULK_UNIT;
> +		break;
> +	case 7:
> +		mult = PMIC5000_VBIAS_UNIT;
> +		break;
> +	default:
> +		mult = PMIC5000_VOLT_UNIT;
> +		break;
> +	}
> +
> +	mutex_lock(&data->adc_lock);
> +
> +	err = regmap_update_bits(regmap, PMIC5000_REG_ADC_CONFIG,
> +				 PMIC5000_ADC_SELECT_MASK, channel << 3);
> +	if (err)
> +		goto error;
> +
> +	/*
> +	 * The host shall wait minimum of 9 ms delay after the input selection
> +	 * for ADC readout and the actual readout
> +	 *
> +	 * msleep may sleep for up to 20ms, which is fine.

No, it isn't fine.

Also, why wait if the channel was not changed ?

> +	 */
> +	msleep(9);
> +
> +	err = regmap_read(regmap, PMIC5000_REG_ADC_VOLTAGE, &regval);
> +	if (err)
> +		goto error;
> +
> +	mutex_unlock(&data->adc_lock);
> +
> +	*val = regval * mult;
> +	return 0;
> +
> +error:
> +	mutex_unlock(&data->adc_lock);
> +	return err;
> +}
> +
> +static int pmic5000_read_interval(struct regmap *regmap, u32 attr, long *val)
> +{
> +	unsigned int regval;
> +	int err;
> +
> +	if (attr != hwmon_chip_update_interval)
> +		return -EOPNOTSUPP;
> +
> +	err = regmap_read(regmap, PMIC5000_REG_ADC_CONFIG, &regval);
> +	if (err < 0)
> +		return err;
> +	*val = 1 << (regval & 0x03);

		BIT()

> +	return 0;
> +}
> +
> +static int pmic5000_read(struct device *dev, enum hwmon_sensor_types type,
> +			 u32 attr, int channel, long *val)
> +{
> +	struct pmic5000_data *data = dev_get_drvdata(dev);
> +	struct regmap *regmap = data->regmap;
> +
> +	switch (type) {
> +	case hwmon_chip:
> +		return pmic5000_read_interval(regmap, attr, val);
> +	case hwmon_temp:
> +		return pmic5000_read_temp(regmap, attr, channel, val);
> +	case hwmon_in:
> +		return pmic5000_read_adc(data, attr, channel, val);
> +	case hwmon_curr:
> +		return pmic5000_read_curr(data, attr, channel, val);
> +	case hwmon_power:
> +		return pmic5000_read_power(data, attr, channel, val);
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +}
> +
> +static int pmic5000_read_string(struct device *dev,
> +				enum hwmon_sensor_types type, u32 attr,
> +				int channel, const char **str)
> +{
> +	if (type == hwmon_curr && attr == hwmon_curr_label) {
> +		if (channel < 0 || channel > 3)
> +			return -EOPNOTSUPP;

Again, channel is never < 0, and unless is_visible and/or the configuration data is wrong
it should never be > 3.

> +		*str = pmic5000_power_labels[channel];
> +	} else if (type == hwmon_power && attr == hwmon_power_label) {
> +		if (channel < 0 || channel > 3)
> +			return -EOPNOTSUPP;
> +		*str = pmic5000_power_labels[channel];
> +	} else if (type == hwmon_in && attr == hwmon_in_label) {
> +		if (channel < 0 || channel > 9 || channel == 4)

Many more unencessary channel checks.

> +			return -EOPNOTSUPP;
> +		*str = pmic5000_voltage_labels[channel];
> +	} else {
> +		return -EOPNOTSUPP;
> +	}
> +
> +	return 0;
> +}
> +
> +static int pmic5000_write_interval(struct pmic5000_data *data, long val)
> +{
> +	struct regmap *regmap = data->regmap;
> +	u32 regval;
> +	int err;
> +
> +	switch (val) {
> +	case 1:
> +		regval = 0;
> +		break;
> +	case 2:
> +		regval = 1;
> +		break;
> +	case 4:
> +		regval = 2;
> +		break;
> +	case 8:
> +		regval = 3;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}

find_closest() would be more appropriate here. We don't usually expect users to know valid
update intervals.

> +
> +	mutex_lock(&data->adc_lock);
> +	err = regmap_update_bits(regmap, PMIC5000_REG_ADC_CONFIG, 0x03, regval);
> +	mutex_unlock(&data->adc_lock);

Another unnecessary lock.

> +	return err;
> +}
> +
> +static int pmic5000_write(struct device *dev, enum hwmon_sensor_types type,
> +			  u32 attr, int channel, long val)
> +{
> +	struct pmic5000_data *data = dev_get_drvdata(dev);
> +
> +	switch (type) {
> +	case hwmon_chip:
> +		return pmic5000_write_interval(data, val);
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +}
> +
> +static umode_t pmic5000_is_visible(const void *data,
> +				   enum hwmon_sensor_types type, u32 attr,
> +				   int channel)
> +{
> +	int ret;
> +	struct pmic5000_data *pmic_data = (struct pmic5000_data *)data;
> +	struct regmap *regmap = pmic_data->regmap;
> +
> +	switch (type) {
> +	case hwmon_chip:
> +		if (attr == hwmon_chip_update_interval)
> +			return 0644;
> +		break;
> +	case hwmon_temp:
> +		return 0444;
> +	case hwmon_in:
> +		if (channel == 4)
> +			return 0;
> +		if (channel >= 0 && channel <= 3 && (attr != hwmon_in_enable)) {
> +			ret = pmic5000_check_regulator_enabled(regmap, channel);
> +			if (!ret || ret < 0)
> +				return 0;
> +		}
> +		return 0444;
> +	case hwmon_power:
> +		if (channel >= 0 && channel <= 3) {
> +			ret = pmic5000_check_regulator_enabled(regmap, channel);
> +			if (!ret || ret < 0)
> +				return 0;
> +		}
> +		return 0444;
> +	case hwmon_curr:
> +		if (channel >= 0 && channel <= 3) {
> +			ret = pmic5000_check_regulator_enabled(regmap, channel);
> +			if (!ret || ret < 0)
> +				return 0;
> +		}
> +		return 0444;
> +	default:
> +		break;
> +	}
> +	return 0444;
> +}
> +
> +/*
> + * Bank and vendor id are 8-bit fields with seven data bits and odd parity.
> + * Vendor IDs 0 and 0x7f are invalid.
> + * See Jedec standard JEP106BJ for details and a list of assigned vendor IDs.
> + */
> +static bool pmic5000_vendor_valid(u8 bank, u8 id)
> +{
> +	if (parity8(bank) == 0 || parity8(id) == 0)
> +		return false;
> +
> +	id &= 0x7f;
> +	return id && id != 0x7f;
> +}
> +
> +static const struct hwmon_channel_info *pmic5000_info[] = {
> +	HWMON_CHANNEL_INFO(chip, HWMON_C_UPDATE_INTERVAL),
> +	HWMON_CHANNEL_INFO(temp,
> +			   HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_MAX_ALARM),
> +	HWMON_CHANNEL_INFO(
> +		in,
> +		HWMON_I_ENABLE | HWMON_I_INPUT | HWMON_I_MIN | HWMON_I_MAX |
> +			HWMON_I_MIN_ALARM | HWMON_I_MAX_ALARM | HWMON_I_LABEL,
> +		HWMON_I_ENABLE | HWMON_I_INPUT | HWMON_I_MIN | HWMON_I_MAX |
> +			HWMON_I_MIN_ALARM | HWMON_I_MAX_ALARM | HWMON_I_LABEL,
> +		HWMON_I_ENABLE | HWMON_I_INPUT | HWMON_I_MIN | HWMON_I_MAX |
> +			HWMON_I_MIN_ALARM | HWMON_I_MAX_ALARM | HWMON_I_LABEL,
> +		HWMON_I_ENABLE | HWMON_I_INPUT | HWMON_I_MIN | HWMON_I_MAX |
> +			HWMON_I_MIN_ALARM | HWMON_I_MAX_ALARM | HWMON_I_LABEL,
> +		HWMON_I_INPUT,
> +		HWMON_I_INPUT | HWMON_I_MAX | HWMON_I_MAX_ALARM | HWMON_I_LABEL,
> +		HWMON_I_INPUT | HWMON_I_MAX | HWMON_I_MAX_ALARM | HWMON_I_LABEL,
> +		HWMON_I_INPUT | HWMON_I_LABEL, HWMON_I_INPUT | HWMON_I_LABEL,
> +		HWMON_I_INPUT | HWMON_I_LABEL),
> +	HWMON_CHANNEL_INFO(
> +		curr,
> +		HWMON_C_INPUT | HWMON_C_MAX | HWMON_C_MAX_ALARM | HWMON_C_LABEL,
> +		HWMON_C_INPUT | HWMON_C_MAX | HWMON_C_MAX_ALARM | HWMON_C_LABEL,
> +		HWMON_C_INPUT | HWMON_C_MAX | HWMON_C_MAX_ALARM | HWMON_C_LABEL,
> +		HWMON_C_INPUT | HWMON_C_MAX | HWMON_C_MAX_ALARM |
> +			HWMON_C_LABEL),
> +	HWMON_CHANNEL_INFO(power, HWMON_P_INPUT | HWMON_P_LABEL,
> +			   HWMON_P_INPUT | HWMON_P_LABEL,
> +			   HWMON_P_INPUT | HWMON_P_LABEL,
> +			   HWMON_P_INPUT | HWMON_P_LABEL),
> +	NULL
> +};
> +
> +static const struct hwmon_ops pmic5000_hwmon_ops = {
> +	.is_visible = pmic5000_is_visible,
> +	.read = pmic5000_read,
> +	.read_string = pmic5000_read_string,
> +	.write = pmic5000_write,
> +};
> +
> +static const struct hwmon_chip_info pmic5000_chip_info = {
> +	.ops = &pmic5000_hwmon_ops,
> +	.info = pmic5000_info,
> +};
> +
> +/* regmap */
> +
> +static bool pmic5000_writeable_reg(struct device *dev, unsigned int reg)
> +{
> +	switch (reg) {
> +	case PMIC5000_REG_OUTPUT_SELECT:
> +	case PMIC5000_REG_THRES_AND_SEL:
> +	case PMIC5000_REG_ADC_CONFIG:
> +		return true;
> +	default:
> +		return false;
> +	}
> +}
> +
> +static bool pmic5000_volatile_reg(struct device *dev, unsigned int reg)
> +{
> +	switch (reg) {
> +	case PMIC5000_REG_OVER_VOLT_IN:
> +	case PMIC5000_REG_OVER_CURRENT:
> +	case PMIC5000_REG_OVER_VOLTAGE:
> +	case PMIC5000_REG_UNDER_VOLTAGE:
> +	case PMIC5000_REG_SWA_POWER:
> +	case PMIC5000_REG_SWB_POWER:
> +	case PMIC5000_REG_SWC_POWER:
> +	case PMIC5000_REG_SWD_POWER:
> +	case PMIC5000_REG_ADC_VOLTAGE:
> +	case PMIC5000_REG_TEMPERATURE:
> +		return true;
> +	default:
> +		return false;
> +	}
> +}
> +
> +static const struct regmap_config pmic5000_regmap8_config = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +	.max_register = 0x3f,
> +	.writeable_reg = pmic5000_writeable_reg,
> +	.volatile_reg = pmic5000_volatile_reg,
> +	.cache_type = REGCACHE_MAPLE,
> +};
> +
> +static int pmic5000_suspend(struct device *dev)
> +{
> +	struct pmic5000_data *data = dev_get_drvdata(dev);
> +	struct regmap *regmap = data->regmap;
> +	u32 regval;
> +	int err;
> +
> +	/*
> +	 * Make sure the configuration register in the regmap cache is current
> +	 * before bypassing it.
> +	 */
> +	err = regmap_read(regmap, PMIC5000_REG_ADC_CONFIG, &regval);
> +	if (err < 0)
> +		return err;
> +
> +	regcache_cache_bypass(regmap, true);
> +	regmap_update_bits(regmap, PMIC5000_REG_ADC_CONFIG, PMIC5000_ADC_ENABLE,
> +			   0);
> +	regcache_cache_bypass(regmap, false);
> +
> +	regcache_cache_only(regmap, true);
> +	regcache_mark_dirty(regmap);
> +
> +	return 0;
> +}
> +
> +static int pmic5000_resume(struct device *dev)
> +{
> +	struct pmic5000_data *data = dev_get_drvdata(dev);
> +	struct regmap *regmap = data->regmap;
> +
> +	regcache_cache_only(regmap, false);
> +	return regcache_sync(regmap);
> +}
> +
> +static DEFINE_SIMPLE_DEV_PM_OPS(pmic5000_pm_ops, pmic5000_suspend,
> +				pmic5000_resume);
> +
> +static int pmic5000_common_probe(struct device *dev, struct regmap *regmap)

Why pmic5000_common_probe() ? There is only one caller.

> +{
> +	unsigned int revision, vendor, bank;
> +	struct pmic5000_data *data;
> +	struct device *hwmon_dev;
> +	int err;
> +
> +	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	err = regmap_read(regmap, PMIC5000_REG_REVISION, &revision);
> +	if (err)
> +		return err;
> +
> +	err = regmap_read(regmap, PMIC5000_REG_VENDOR, &bank);
> +	if (err)
> +		return err;
> +	err = regmap_read(regmap, PMIC5000_REG_VENDOR + 1, &vendor);
> +	if (err)
> +		return err;
> +	if (!pmic5000_vendor_valid(bank, vendor))
> +		return -ENODEV;
> +
> +	data->regmap = regmap;
> +	mutex_init(&data->mode_lock);
> +	mutex_init(&data->adc_lock);
> +	dev_set_drvdata(dev, data);
> +
> +	hwmon_dev = devm_hwmon_device_register_with_info(
> +		dev, "pmic5000", data, &pmic5000_chip_info, NULL);
> +	if (IS_ERR(hwmon_dev))
> +		return PTR_ERR(hwmon_dev);
> +
> +	dev_info(dev, "DDR5 PMIC sensor: vendor 0x%02x:0x%02x revision %d.%d\n",
> +		 bank & 0x7f, vendor, ((revision >> 4) & 0x03) + 1,
> +		 ((revision >> 1) & 0x07) + 1);
> +
> +	/* Enable individual measurements and enable ADC */
> +	err = regmap_update_bits(regmap, PMIC5000_REG_OUTPUT_SELECT,
> +				 PMIC5000_OUTPUT_SELECT,
> +				 PMIC5000_OUTPUT_SELECT);
> +	if (err)
> +		return err;
> +	err = regmap_update_bits(regmap, PMIC5000_REG_ADC_CONFIG,
> +				 PMIC5000_ADC_ENABLE, PMIC5000_ADC_ENABLE);
> +	if (err)
> +		return err;
> +
> +	return 0;
> +}
> +
> +/* I2C */
> +
> +static int pmic5000_i2c_init(struct i2c_client *client)
> +{
> +	struct i2c_adapter *adapter = client->adapter;
> +
> +	/*
> +	 * Register accesses are 8-bit, so require byte-data transactions only.
> +	 * Requiring WORD_DATA here rejects otherwise valid adapters.
> +	 */
> +	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA))
> +		return -ENODEV;
> +
> +	return 0;
> +}
> +
> +static int pmic5000_i2c_probe(struct i2c_client *client)
> +{
> +	struct device *dev = &client->dev;
> +	struct regmap *regmap;
> +	int err;
> +
> +	err = pmic5000_i2c_init(client);
> +	if (err)
> +		return dev_err_probe(dev, err, "I2C capability check failed\n");
> +
> +	regmap = devm_regmap_init_i2c(client, &pmic5000_regmap8_config);
> +	if (IS_ERR(regmap))
> +		return dev_err_probe(dev, PTR_ERR(regmap),
> +				     "regmap init failed\n");
> +
> +	return pmic5000_common_probe(dev, regmap);
> +}
> +
> +static const struct i2c_device_id pmic5000_i2c_id[] = { { .name = "pmic5000" },
> +							{} };
> +MODULE_DEVICE_TABLE(i2c, pmic5000_i2c_id);
> +
> +static const struct of_device_id pmic5000_of_ids[] = {
> +	{
> +		.compatible = "jedec,pmic5000",

jedec,pmic5000 is not documented.

> +	},
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, pmic5000_of_ids);
> +
> +static struct i2c_driver pmic5000_i2c_driver = {
> +	.class		= I2C_CLASS_HWMON,
> +	.driver = {
> +		.name	= "pmic5000",
> +		.of_match_table = pmic5000_of_ids,
> +		.pm = pm_sleep_ptr(&pmic5000_pm_ops),
> +	},
> +	.probe		= pmic5000_i2c_probe,
> +	.id_table	= pmic5000_i2c_id,
> +};
> +
> +module_i2c_driver(pmic5000_i2c_driver);
> +
> +MODULE_AUTHOR("Stephen Horvath <linux@stevetech.au>");
> +MODULE_DESCRIPTION("PMIC5000 driver");
> +MODULE_LICENSE("GPL");

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH RFC] hwmon: Add Support for PMIC5000 Power Management ICs
  2026-08-20 15:34 ` Guenter Roeck
@ 2026-08-21  3:27   ` Stephen Horvath
  2026-08-21  3:52     ` Guenter Roeck
  0 siblings, 1 reply; 7+ messages in thread
From: Stephen Horvath @ 2026-08-21  3:27 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: Andi Shyti, linux-hwmon, linux-i2c, linux-kernel, Stephen Horvath

Hi Guenter,

Thanks for your review!

On 21/8/26 01:34, Guenter Roeck wrote:
>> - Does this belong in HWMON, or should it be part of the regulator
>> 	subsystem?
> 
> It depends on its functionality. Is this just for monitoring, or are the voltages
> controllable ?

This driver is just for monitoring. They could be controllable, but I
haven't implemented that as changing the voltage of active RAM modules
doesn't seem safe (without retraining and so on).

>> +in0_enable		Whether SWA is enabled (RO)
> 
> Does that enable the voltage or its monitoriong ? The enable attribute is
> only to enable monitoring, not to enable the voltage. If this is a
> controllable voltage, the driver should be a regulator driver, as Sashiko
> suggested.

It just queries whether that output is enabled. I'll remove it though as
the disabled channels are hidden by is_visible anyway.

>> +/* PMIC5000 registers. */
>> +// clang-format off
> 
> No way. Are you serious ?

clang-format would break the alignment of the #defines; and there's a
few other drivers with "// clang-format" in them, so I assumed it would
be okay (though none are hwmon). I'll remove it next revision.

>> +
>> +
> 
> Please run checkpatch --strict on your patches and fix what it reports.

Will do!

>> +struct pmic5000_data {
>> +	struct regmap *regmap;
>> +	struct mutex mode_lock;
>> +	struct mutex adc_lock;
> 
> Explain why those are needed on top of the hwmon subsystem lock.

I didn't know the subsystem lock existed, I'll remove them.

>> +	if (channel != 0)
>> +		return -EOPNOTSUPP;
> 
> Unnecessary check.

Sure.

> 
> Unnecessary channel checks (at least if the is_visible function does its job
> and the info data is correct).
> 

Okay.

>> +	case 6: {
> 
> Why "{" ?

No reason, I'll remove it. I had previously declared something there.

>> +	if (channel >= 0 && channel <= 3) {
> 
> channel is always >= 0.
I wasn't too sure, thanks for that!

>> +	/*
>> +	 * The host shall wait minimum of 9 ms delay after the input selection
>> +	 * for ADC readout and the actual readout
>> +	 *
>> +	 * msleep may sleep for up to 20ms, which is fine.
> 
> No, it isn't fine.

Fair enough, I'll change it to fsleep.

> Also, why wait if the channel was not changed ?

Good point.

>> +	*val = 1 << (regval & 0x03);
> 
> 		BIT()

Thanks!

>> +static int pmic5000_write_interval(struct pmic5000_data *data, long val)
>> +{
>> +	struct regmap *regmap = data->regmap;
>> +	u32 regval;
>> +	int err;
>> +
>> +	switch (val) {
>> +	case 1:
>> +		regval = 0;
>> +		break;
>> +	case 2:
>> +		regval = 1;
>> +		break;
>> +	case 4:
>> +		regval = 2;
>> +		break;
>> +	case 8:
>> +		regval = 3;
>> +		break;
>> +	default:
>> +		return -EINVAL;
>> +	}
> 
> find_closest() would be more appropriate here. We don't usually expect users to know valid
> update intervals.

Thanks!

>> +static int pmic5000_common_probe(struct device *dev, struct regmap *regmap)
> 
> Why pmic5000_common_probe() ? There is only one caller.

Good point, I'll move it.

>> +static const struct of_device_id pmic5000_of_ids[] = {
>> +	{
>> +		.compatible = "jedec,pmic5000",
> 
> jedec,pmic5000 is not documented.

Is there some centralised database for device tree identifiers, or does
it just need to be added to trivial-devices.yaml or something?

Thanks again for your review!
Steve

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH RFC] hwmon: Add Support for PMIC5000 Power Management ICs
  2026-08-21  3:27   ` Stephen Horvath
@ 2026-08-21  3:52     ` Guenter Roeck
  0 siblings, 0 replies; 7+ messages in thread
From: Guenter Roeck @ 2026-08-21  3:52 UTC (permalink / raw)
  To: Stephen Horvath
  Cc: Andi Shyti, linux-hwmon, linux-i2c, linux-kernel, Stephen Horvath

On 8/20/26 20:27, Stephen Horvath wrote:
...

>>> +static const struct of_device_id pmic5000_of_ids[] = {
>>> +	{
>>> +		.compatible = "jedec,pmic5000",
>>
>> jedec,pmic5000 is not documented.
> 
> Is there some centralised database for device tree identifiers, or does
> it just need to be added to trivial-devices.yaml or something?

You'll need a separate patch adding the property to trivial-devices.yaml.

Guenter


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH RFC] hwmon: Add Support for PMIC5000 Power Management ICs
  2026-08-19 13:37 [PATCH RFC] hwmon: Add Support for PMIC5000 Power Management ICs Stephen Horvath
  2026-08-19 13:51 ` sashiko-bot
  2026-08-20 15:34 ` Guenter Roeck
@ 2026-08-23 20:18 ` kernel test robot
  2 siblings, 0 replies; 7+ messages in thread
From: kernel test robot @ 2026-08-23 20:18 UTC (permalink / raw)
  To: Stephen Horvath; +Cc: oe-kbuild-all

Hi Stephen,

[This is a private test report for your RFC patch.]
kernel test robot noticed the following build warnings:

[auto build test WARNING on 75f2c0b3690702c90863c2e138cb5520670845ea]

url:    https://github.com/intel-lab-lkp/linux/commits/Stephen-Horvath/hwmon-Add-Support-for-PMIC5000-Power-Management-ICs/20260819-233745
base:   75f2c0b3690702c90863c2e138cb5520670845ea
patch link:    https://lore.kernel.org/r/20260819-pmic5000-v1-1-e5e0ddcf0863%40stevetech.au
patch subject: [PATCH RFC] hwmon: Add Support for PMIC5000 Power Management ICs
compiler: clang version 22.1.8 (https://github.com/llvm/llvm-project ca7933e47d3a3451d81e72ac174dcb5aa28b59d1)
docutils: docutils (Docutils 0.21.2, Python 3.13.5, on linux)
reproduce: (https://download.01.org/0day-ci/archive/20260823/202608232257.ltZnDbDc-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202608232257.ltZnDbDc-lkp@intel.com/

All warnings (new ones prefixed by >>):

   Runtime Survivability
   ===================== [docutils]
>> Documentation/hwmon/pmic5000.rst:4: WARNING: Title underline too short.


vim +4 Documentation/hwmon/pmic5000.rst

     2	
     3	Kernel driver pmic5000
   > 4	=====================
     5	

--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH RFC] hwmon: Add Support for PMIC5000 Power Management ICs
@ 2026-08-23 11:45 kernel test robot
  0 siblings, 0 replies; 7+ messages in thread
From: kernel test robot @ 2026-08-23 11:45 UTC (permalink / raw)
  To: oe-kbuild; +Cc: lkp, Dan Carpenter

BCC: lkp@intel.com
CC: oe-kbuild-all@lists.linux.dev
In-Reply-To: <20260819-pmic5000-v1-1-e5e0ddcf0863@stevetech.au>
References: <20260819-pmic5000-v1-1-e5e0ddcf0863@stevetech.au>
TO: Stephen Horvath <linux@stevetech.au>

Hi Stephen,

[This is a private test report for your RFC patch.]
kernel test robot noticed the following build warnings:

[auto build test WARNING on 75f2c0b3690702c90863c2e138cb5520670845ea]

url:    https://github.com/intel-lab-lkp/linux/commits/Stephen-Horvath/hwmon-Add-Support-for-PMIC5000-Power-Management-ICs/20260819-233745
base:   75f2c0b3690702c90863c2e138cb5520670845ea
patch link:    https://lore.kernel.org/r/20260819-pmic5000-v1-1-e5e0ddcf0863%40stevetech.au
patch subject: [PATCH RFC] hwmon: Add Support for PMIC5000 Power Management ICs
:::::: branch date: 2 days ago
:::::: commit date: 2 days ago
config: riscv-randconfig-r071-20260823 (https://download.01.org/0day-ci/archive/20260823/202608231513.PfEj0vse-lkp@intel.com/config)
compiler: riscv64-linux-gcc (GCC) 12.5.0
smatch: v0.5.0-9187-g5189e3fb

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <error27@gmail.com>
| Closes: https://lore.kernel.org/r/202608231513.PfEj0vse-lkp@intel.com/

New smatch warnings:
drivers/hwmon/pmic5000.c:370 pmic5000_read_volt_thresholds() warn: statement has no effect 'const'

Old smatch warnings:
drivers/hwmon/pmic5000.c:376 pmic5000_read_volt_thresholds() warn: statement has no effect 'const'

vim +/const +370 drivers/hwmon/pmic5000.c

a0d30d2a53a818 Stephen Horvath 2026-08-19  290  
a0d30d2a53a818 Stephen Horvath 2026-08-19  291  static int pmic5000_read_volt_thresholds(struct regmap *regmap, u32 attr,
a0d30d2a53a818 Stephen Horvath 2026-08-19  292  					 int channel, long *val)
a0d30d2a53a818 Stephen Horvath 2026-08-19  293  {
a0d30d2a53a818 Stephen Horvath 2026-08-19  294  	int err;
a0d30d2a53a818 Stephen Horvath 2026-08-19  295  	u32 set_reg, thresh_reg, range_bit;
a0d30d2a53a818 Stephen Horvath 2026-08-19  296  	u32 set_regval, thresh_regval, range_regval;
a0d30d2a53a818 Stephen Horvath 2026-08-19  297  	u32 volt_set;
a0d30d2a53a818 Stephen Horvath 2026-08-19  298  	int base_volts[2];
a0d30d2a53a818 Stephen Horvath 2026-08-19  299  
a0d30d2a53a818 Stephen Horvath 2026-08-19  300  	switch (channel) {
a0d30d2a53a818 Stephen Horvath 2026-08-19  301  	case 0:
a0d30d2a53a818 Stephen Horvath 2026-08-19  302  		set_reg = PMIC5000_REG_SWA_VOLT_SET;
a0d30d2a53a818 Stephen Horvath 2026-08-19  303  		thresh_reg = PMIC5000_REG_SWA_THRESH;
a0d30d2a53a818 Stephen Horvath 2026-08-19  304  		range_bit = PMIC5000_SWA_RANGE;
a0d30d2a53a818 Stephen Horvath 2026-08-19  305  		base_volts[0] = 800;
a0d30d2a53a818 Stephen Horvath 2026-08-19  306  		base_volts[1] = 600;
a0d30d2a53a818 Stephen Horvath 2026-08-19  307  		break;
a0d30d2a53a818 Stephen Horvath 2026-08-19  308  	case 1:
a0d30d2a53a818 Stephen Horvath 2026-08-19  309  		set_reg = PMIC5000_REG_SWB_VOLT_SET;
a0d30d2a53a818 Stephen Horvath 2026-08-19  310  		thresh_reg = PMIC5000_REG_SWB_THRESH;
a0d30d2a53a818 Stephen Horvath 2026-08-19  311  		range_bit = PMIC5000_SWB_RANGE;
a0d30d2a53a818 Stephen Horvath 2026-08-19  312  		base_volts[0] = 800;
a0d30d2a53a818 Stephen Horvath 2026-08-19  313  		base_volts[1] = 600;
a0d30d2a53a818 Stephen Horvath 2026-08-19  314  		break;
a0d30d2a53a818 Stephen Horvath 2026-08-19  315  	case 2:
a0d30d2a53a818 Stephen Horvath 2026-08-19  316  		set_reg = PMIC5000_REG_SWC_VOLT_SET;
a0d30d2a53a818 Stephen Horvath 2026-08-19  317  		thresh_reg = PMIC5000_REG_SWC_THRESH;
a0d30d2a53a818 Stephen Horvath 2026-08-19  318  		range_bit = PMIC5000_SWC_RANGE;
a0d30d2a53a818 Stephen Horvath 2026-08-19  319  		base_volts[0] = 800;
a0d30d2a53a818 Stephen Horvath 2026-08-19  320  		base_volts[1] = 600;
a0d30d2a53a818 Stephen Horvath 2026-08-19  321  		break;
a0d30d2a53a818 Stephen Horvath 2026-08-19  322  	case 3:
a0d30d2a53a818 Stephen Horvath 2026-08-19  323  		set_reg = PMIC5000_REG_SWD_VOLT_SET;
a0d30d2a53a818 Stephen Horvath 2026-08-19  324  		thresh_reg = PMIC5000_REG_SWD_THRESH;
a0d30d2a53a818 Stephen Horvath 2026-08-19  325  		range_bit = PMIC5000_SWD_RANGE;
a0d30d2a53a818 Stephen Horvath 2026-08-19  326  		base_volts[0] = 1500;
a0d30d2a53a818 Stephen Horvath 2026-08-19  327  		base_volts[1] = 2200;
a0d30d2a53a818 Stephen Horvath 2026-08-19  328  		break;
a0d30d2a53a818 Stephen Horvath 2026-08-19  329  	case 5:
a0d30d2a53a818 Stephen Horvath 2026-08-19  330  	case 6: {
a0d30d2a53a818 Stephen Horvath 2026-08-19  331  		err = regmap_read(regmap, PMIC5000_REG_THRES_AND_SEL,
a0d30d2a53a818 Stephen Horvath 2026-08-19  332  				  &thresh_regval);
a0d30d2a53a818 Stephen Horvath 2026-08-19  333  		if (err)
a0d30d2a53a818 Stephen Horvath 2026-08-19  334  			return err;
a0d30d2a53a818 Stephen Horvath 2026-08-19  335  		if (channel == 5) {
a0d30d2a53a818 Stephen Horvath 2026-08-19  336  			if (thresh_regval & PMIC5000_VIN_BULK_THRESH)
a0d30d2a53a818 Stephen Horvath 2026-08-19  337  				*val = 14500;
a0d30d2a53a818 Stephen Horvath 2026-08-19  338  			else
a0d30d2a53a818 Stephen Horvath 2026-08-19  339  				*val = 16000;
a0d30d2a53a818 Stephen Horvath 2026-08-19  340  			return 0;
a0d30d2a53a818 Stephen Horvath 2026-08-19  341  		} else {
a0d30d2a53a818 Stephen Horvath 2026-08-19  342  			if (thresh_regval & PMIC5000_VIN_MGMT_THRESH)
a0d30d2a53a818 Stephen Horvath 2026-08-19  343  				*val = 3800;
a0d30d2a53a818 Stephen Horvath 2026-08-19  344  			else
a0d30d2a53a818 Stephen Horvath 2026-08-19  345  				*val = 3700;
a0d30d2a53a818 Stephen Horvath 2026-08-19  346  			return 0;
a0d30d2a53a818 Stephen Horvath 2026-08-19  347  		}
a0d30d2a53a818 Stephen Horvath 2026-08-19  348  	}
a0d30d2a53a818 Stephen Horvath 2026-08-19  349  
a0d30d2a53a818 Stephen Horvath 2026-08-19  350  	default:
a0d30d2a53a818 Stephen Horvath 2026-08-19  351  		return -EOPNOTSUPP;
a0d30d2a53a818 Stephen Horvath 2026-08-19  352  	}
a0d30d2a53a818 Stephen Horvath 2026-08-19  353  
a0d30d2a53a818 Stephen Horvath 2026-08-19  354  	err = regmap_read(regmap, set_reg, &set_regval);
a0d30d2a53a818 Stephen Horvath 2026-08-19  355  	if (err)
a0d30d2a53a818 Stephen Horvath 2026-08-19  356  		return err;
a0d30d2a53a818 Stephen Horvath 2026-08-19  357  	err = regmap_read(regmap, thresh_reg, &thresh_regval);
a0d30d2a53a818 Stephen Horvath 2026-08-19  358  	if (err)
a0d30d2a53a818 Stephen Horvath 2026-08-19  359  		return err;
a0d30d2a53a818 Stephen Horvath 2026-08-19  360  	err = regmap_read(regmap, PMIC5000_REG_SW_VOLT_RANGE, &range_regval);
a0d30d2a53a818 Stephen Horvath 2026-08-19  361  	if (err)
a0d30d2a53a818 Stephen Horvath 2026-08-19  362  		return err;
a0d30d2a53a818 Stephen Horvath 2026-08-19  363  
a0d30d2a53a818 Stephen Horvath 2026-08-19  364  	volt_set = range_regval & range_bit ? base_volts[1] : base_volts[0];
a0d30d2a53a818 Stephen Horvath 2026-08-19  365  	volt_set += (set_regval >> 1) * 5;
a0d30d2a53a818 Stephen Horvath 2026-08-19  366  
a0d30d2a53a818 Stephen Horvath 2026-08-19  367  	switch (attr) {
a0d30d2a53a818 Stephen Horvath 2026-08-19  368  	case hwmon_in_min:
a0d30d2a53a818 Stephen Horvath 2026-08-19  369  		/* 10%, 12.5%, Reserved, Reserved */
a0d30d2a53a818 Stephen Horvath 2026-08-19 @370  		const int min_permilles[4] = { 100, 125, PERMILLE, PERMILLE };
a0d30d2a53a818 Stephen Horvath 2026-08-19  371  		*val = volt_set - (min_permilles[(thresh_regval >> 2) & 0x03] *
a0d30d2a53a818 Stephen Horvath 2026-08-19  372  				   volt_set / PERMILLE);
a0d30d2a53a818 Stephen Horvath 2026-08-19  373  		return 0;
a0d30d2a53a818 Stephen Horvath 2026-08-19  374  	case hwmon_in_max:
a0d30d2a53a818 Stephen Horvath 2026-08-19  375  		/* 7.5%, 10%, 12.5%, Reserved */
a0d30d2a53a818 Stephen Horvath 2026-08-19  376  		const int max_permilles[4] = { 75, 100, 125, PERMILLE };
a0d30d2a53a818 Stephen Horvath 2026-08-19  377  		*val = volt_set + (max_permilles[(thresh_regval >> 4) & 0x03] *
a0d30d2a53a818 Stephen Horvath 2026-08-19  378  				   volt_set / PERMILLE);
a0d30d2a53a818 Stephen Horvath 2026-08-19  379  		return 0;
a0d30d2a53a818 Stephen Horvath 2026-08-19  380  	default:
a0d30d2a53a818 Stephen Horvath 2026-08-19  381  		return -EOPNOTSUPP;
a0d30d2a53a818 Stephen Horvath 2026-08-19  382  	}
a0d30d2a53a818 Stephen Horvath 2026-08-19  383  }
a0d30d2a53a818 Stephen Horvath 2026-08-19  384  

--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2026-08-23 20:19 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-08-19 13:37 [PATCH RFC] hwmon: Add Support for PMIC5000 Power Management ICs Stephen Horvath
2026-08-19 13:51 ` sashiko-bot
2026-08-20 15:34 ` Guenter Roeck
2026-08-21  3:27   ` Stephen Horvath
2026-08-21  3:52     ` Guenter Roeck
2026-08-23 20:18 ` kernel test robot
  -- strict thread matches above, loose matches on Subject: below --
2026-08-23 11:45 kernel test robot

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.