[PATCH v1 0/3] Introduce cros_ec power monitoring

[PATCH v1 0/3] Introduce cros_ec power monitoring

[Daniel Balint]

This series adds the ChromeOS EC On-Device Power Monitoring 
(cros_ec_pwrmon) driver to read onboard power-monitoring ADCs,
such as PAC194x, wired through the embedded controller (EC).

Several ChromeOS platforms are equipped with onboard ADCs that are
connected to the EC via I2C bus. The driver interfaces with the EC
via the existing EC_CMD_PWRMON host command API (and EC_MKBP_EVENT_PWRMON
events) to manage ADC accumulation and expose accumulated per-rail energy
through an IIO sysfs attribute.

The driver is only loaded when the underlying hardware feature is present.
The series also fixes alignment padding in EC_CMD_PWRMON definitions.

Daniel Balint (3):
  platform/chrome: cros_ec: fix alignment of ec_params_pwrmon
  iio: adc: add cros_ec_pwrmon driver
  mfd: cros_ec: load cros_ec_pwrmon on supported ECs

 .../ABI/testing/sysfs-bus-iio-cros-ec         |  13 +
 MAINTAINERS                                   |   7 +
 drivers/iio/adc/Kconfig                       |  11 +
 drivers/iio/adc/Makefile                      |   1 +
 drivers/iio/adc/cros_ec_pwrmon.c              | 333 ++++++++++++++++++
 drivers/mfd/cros_ec_dev.c                     |   9 +
 .../linux/platform_data/cros_ec_commands.h    |   2 +
 7 files changed, 376 insertions(+)
 create mode 100644 drivers/iio/adc/cros_ec_pwrmon.c


base-commit: 6628e7d4c6b25c8d18f1345e3a20d21829aae7e4
-- 
2.56.0.455.g6c5a503b38-goog

[PATCH v1 1/3] platform/chrome: cros_ec: fix alignment of ec_params_pwrmon

[Daniel Balint]

Sync EC_CMD_PWRMON structure padding with ChromeOS EC firmware by adding
explicit reserved padding to struct ec_params_pwrmon (2-byte union
alignment) and struct pwrmon_dump_info (4-byte struct size alignment),
as __ec_align2 and __ec_align4 expand to __packed in Linux.

Fixes: b57af7f8d194 ("platform/chrome: cros_ec: Sync host command header with EC")
Signed-off-by: Daniel Balint <dbalint@google.com>
---
 include/linux/platform_data/cros_ec_commands.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/linux/platform_data/cros_ec_commands.h b/include/linux/platform_data/cros_ec_commands.h
index a7bd69d83522..6d74371c7074 100644
--- a/include/linux/platform_data/cros_ec_commands.h
+++ b/include/linux/platform_data/cros_ec_commands.h
@@ -6635,6 +6635,7 @@ enum ec_pwrmon_cmd {
 
 struct ec_params_pwrmon {
 	uint8_t cmd;
+	uint8_t reserved;
 	union {
 		uint16_t set_rate;
 		uint8_t channel_id;
@@ -6651,6 +6652,7 @@ struct ec_params_pwrmon {
 struct pwrmon_dump_info {
 	uint8_t channel_id;
 	char channel_name[32];
+	uint8_t reserved[3];
 } __ec_align4;
 
 struct ec_response_pwrmon {
-- 
2.56.0.455.g6c5a503b38-goog

[PATCH v1 2/3] iio: adc: add cros_ec_pwrmon driver

[Daniel Balint]

Add the ChromeOS EC On-Device Power Monitoring (ODPM) driver to read
onboard power-monitoring ADCs (such as PAC194x) wired through the EC.

The driver interfaces with the EC via the existing EC_CMD_PWRMON host
command API and EC_MKBP_EVENT_PWRMON events to start/stop accumulation,
latch ADC accumulators, and expose cumulative per-rail energy in
microjoules (uWs) through the energy_value sysfs attribute.

Signed-off-by: Daniel Balint <dbalint@google.com>
---
 .../ABI/testing/sysfs-bus-iio-cros-ec         |  13 +
 MAINTAINERS                                   |   7 +
 drivers/iio/adc/Kconfig                       |  11 +
 drivers/iio/adc/Makefile                      |   1 +
 drivers/iio/adc/cros_ec_pwrmon.c              | 333 ++++++++++++++++++
 5 files changed, 365 insertions(+)
 create mode 100644 drivers/iio/adc/cros_ec_pwrmon.c

diff --git a/Documentation/ABI/testing/sysfs-bus-iio-cros-ec b/Documentation/ABI/testing/sysfs-bus-iio-cros-ec
index 3de1dfc98389..9043824f2831 100644
--- a/Documentation/ABI/testing/sysfs-bus-iio-cros-ec
+++ b/Documentation/ABI/testing/sysfs-bus-iio-cros-ec
@@ -19,3 +19,16 @@ Description:
 		represents the sensor ID as exposed by the EC. This ID is used
 		by the Android sensor service hardware abstraction layer (sensor
 		HAL) through the Android container on ChromeOS.
+
+What:		/sys/bus/iio/devices/iio:deviceX/energy_value
+Date:		November 2026
+KernelVersion:	7.5
+Contact:	chrome-platform@lists.linux.dev, linux-iio@vger.kernel.org
+Description:
+		This read-only attribute is exposed by the ChromeOS EC power
+		monitor (cros_ec_pwrmon) driver for On-Device Power Monitoring
+		(ODPM). Reading it latches all EC power accumulators and returns
+		a snapshot timestamp followed by per-channel accumulated energy:
+
+		t=<Measurement timestamp in ms since boot>
+		CH<N>(T=<Duration in ms>)[<Rail schematic name>], <Accumulated energy in uWs>
diff --git a/MAINTAINERS b/MAINTAINERS
index 93abbb315065..c58e3943cfdc 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -6335,6 +6335,13 @@ M:	Thomas Weißschuh <linux@weissschuh.net>
 S:	Maintained
 F:	drivers/leds/leds-cros_ec.c
 
+CHROMEOS EC POWER MONITORING
+M:	Daniel Balint <dbalint@google.com>
+L:	chrome-platform@lists.linux.dev
+L:	linux-iio@vger.kernel.org
+S:	Maintained
+F:	drivers/iio/adc/cros_ec_pwrmon.c
+
 CHROMEOS EC SUBDRIVERS
 M:	Benson Leung <bleung@chromium.org>
 R:	Guenter Roeck <groeck@chromium.org>
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index a802d8ee4dd9..fbe250019342 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -743,6 +743,17 @@ config CPCAP_ADC
 	  This driver can also be built as a module. If so, the module will be
 	  called cpcap-adc.
 
+config CROS_EC_PWRMON
+	tristate "ChromeOS EC Power Monitor Driver"
+	depends on MFD_CROS_EC_DEV
+	default MFD_CROS_EC_DEV
+	help
+	  Say yes here to build support for the ChromeOS EC On-Device Power
+	  Monitoring (ODPM) driver.
+
+	  To compile this driver as a module, choose M here: the module will be
+	  called cros_ec_pwrmon.
+
 config DA9150_GPADC
 	tristate "Dialog DA9150 GPADC driver support"
 	depends on MFD_DA9150
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index 9e82c21dbd37..2f36a3c4a68f 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -63,6 +63,7 @@ obj-$(CONFIG_BCM_IPROC_ADC) += bcm_iproc_adc.o
 obj-$(CONFIG_BERLIN2_ADC) += berlin2-adc.o
 obj-$(CONFIG_CC10001_ADC) += cc10001_adc.o
 obj-$(CONFIG_CPCAP_ADC) += cpcap-adc.o
+obj-$(CONFIG_CROS_EC_PWRMON) += cros_ec_pwrmon.o
 obj-$(CONFIG_DA9150_GPADC) += da9150-gpadc.o
 obj-$(CONFIG_DLN2_ADC) += dln2-adc.o
 obj-$(CONFIG_ENVELOPE_DETECTOR) += envelope-detector.o
diff --git a/drivers/iio/adc/cros_ec_pwrmon.c b/drivers/iio/adc/cros_ec_pwrmon.c
new file mode 100644
index 000000000000..795b5f2b2a2c
--- /dev/null
+++ b/drivers/iio/adc/cros_ec_pwrmon.c
@@ -0,0 +1,333 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * ChromeOS EC Power Monitor Driver
+ *
+ * Copyright (c) 2026 Google Inc.
+ */
+
+#include <linux/cleanup.h>
+#include <linux/device.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+#include <linux/ktime.h>
+#include <linux/math64.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/notifier.h>
+#include <linux/platform_data/cros_ec_commands.h>
+#include <linux/platform_data/cros_ec_proto.h>
+#include <linux/platform_device.h>
+#include <linux/spinlock.h>
+#include <linux/string.h>
+#include <linux/sysfs.h>
+#include <linux/types.h>
+#include <linux/unaligned.h>
+#include <linux/wait.h>
+
+#define PWRMON_MAX_CHANNELS 64
+#define PWRMON_MKBP_TIMEOUT_MS 1000
+
+struct cros_ec_pwrmon_state {
+	struct cros_ec_command *msg;
+	struct cros_ec_dev *ec_dev;
+	struct cros_ec_device *ec;
+	/* Serializes EC host commands and energy accumulation state */
+	struct mutex lock;
+	/* Protects MKBP event results and collection state */
+	spinlock_t event_lock;
+	struct notifier_block notifier;
+	wait_queue_head_t wq;
+	struct {
+		s64 value;
+		bool valid;
+	} results[PWRMON_MAX_CHANNELS];
+	int results_received;
+	bool collecting;
+	u64 *accumulated_energy;
+	char (*channel_names)[32];
+	int num_channels;
+	u64 last_time_ms;
+};
+
+/*
+ * MKBP event handler — called once per channel event from the EC.
+ */
+static int cros_ec_pwrmon_notify(struct notifier_block *nb,
+				 unsigned long queued_during_suspend,
+				 void *data)
+{
+	struct cros_ec_pwrmon_state *st =
+		container_of(nb, struct cros_ec_pwrmon_state, notifier);
+	struct cros_ec_device *ec_dev = st->ec;
+	s64 val;
+	u8 ch;
+
+	if (ec_dev->event_data.event_type != EC_MKBP_EVENT_PWRMON)
+		return NOTIFY_DONE;
+
+	ch = ec_dev->event_data.data.pwrmon_data.channel_id;
+	val = get_unaligned_le64(&ec_dev->event_data.data.pwrmon_data.value);
+
+	scoped_guard(spinlock, &st->event_lock) {
+		if (!st->collecting)
+			return NOTIFY_OK;
+
+		if (ch < st->num_channels) {
+			st->results[ch].value = val;
+			st->results[ch].valid = true;
+		}
+
+		WRITE_ONCE(st->results_received, st->results_received + 1);
+	}
+
+	wake_up(&st->wq);
+
+	return NOTIFY_OK;
+}
+
+/*
+ * Expose ODPM results in the form of accumulated energy
+ * through the energy_value sysfs node.
+ * Format requirement:
+ * t=<Measurement timestamp, ms>
+ * CH<N>(T=<Duration, ms>)[<Schematic name>], <Accumulated Energy, uWs>
+ */
+static ssize_t energy_value_show(struct device *dev,
+				 struct device_attribute *attr,
+				 char *buf)
+{
+	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct cros_ec_pwrmon_state *st = iio_priv(indio_dev);
+	struct ec_params_pwrmon *params;
+	u64 current_time_ms;
+	s64 delta_uws;
+	u32 duration_ms;
+	int received;
+	ssize_t len = 0;
+	int ret, i;
+
+	guard(mutex)(&st->lock);
+
+	/* Reset results and prepare for MKBP events */
+	scoped_guard(spinlock, &st->event_lock) {
+		memset(st->results, 0, sizeof(st->results));
+		WRITE_ONCE(st->results_received, 0);
+		st->collecting = true;
+	}
+
+	/* Latch all PACs — this also triggers async MKBP delivery. */
+	params = (struct ec_params_pwrmon *)st->msg->data;
+	params->cmd = EC_PWRMON_LATCH;
+	ret = cros_ec_cmd_xfer_status(st->ec, st->msg);
+	if (ret < 0) {
+		scoped_guard(spinlock, &st->event_lock)
+			st->collecting = false;
+		dev_warn(dev, "Failed to latch PAC accumulators: %d\n", ret);
+		return ret;
+	}
+
+	/* Wait for all MKBP events to arrive from the EC */
+	ret = wait_event_timeout(st->wq,
+				 READ_ONCE(st->results_received) >= st->num_channels,
+				 msecs_to_jiffies(PWRMON_MKBP_TIMEOUT_MS));
+
+	scoped_guard(spinlock, &st->event_lock) {
+		st->collecting = false;
+		received = st->results_received;
+	}
+
+	if (!ret)
+		dev_warn(dev, "Timed out waiting for PWRMON MKBP data (received %d/%d)\n",
+			 received, st->num_channels);
+
+	/* Timestamp of snapshot taken. */
+	current_time_ms = ktime_to_ms(ktime_get_boottime());
+	len += sysfs_emit_at(buf, len, "t=%llu\n", current_time_ms);
+
+	duration_ms = current_time_ms - st->last_time_ms;
+
+	for (i = 0; i < st->num_channels; i++) {
+		if (!st->results[i].valid) {
+			dev_warn(dev, "Missing data for channel %d\n", i);
+			continue;
+		}
+
+		delta_uws = div_s64(st->results[i].value * duration_ms,
+				    MSEC_PER_SEC);
+		if (delta_uws > 0)
+			st->accumulated_energy[i] += delta_uws;
+
+		len += sysfs_emit_at(buf, len, "CH%d(T=%u)[%s], %llu\n",
+				     i, duration_ms, st->channel_names[i],
+				     st->accumulated_energy[i]);
+	}
+
+	st->last_time_ms = current_time_ms;
+
+	return len;
+}
+
+static IIO_DEVICE_ATTR(energy_value, 0444, energy_value_show, NULL, 0);
+
+static struct attribute *cros_ec_pwrmon_attrs[] = {
+	&iio_dev_attr_energy_value.dev_attr.attr,
+	NULL
+};
+
+static const struct attribute_group cros_ec_pwrmon_attr_group = {
+	.attrs = cros_ec_pwrmon_attrs,
+};
+
+static const struct iio_info cros_ec_pwrmon_info = {
+	.attrs = &cros_ec_pwrmon_attr_group,
+};
+
+static void cros_ec_pwrmon_stop(void *data)
+{
+	struct cros_ec_pwrmon_state *st = data;
+	struct ec_params_pwrmon *params;
+	int ret;
+
+	guard(mutex)(&st->lock);
+
+	params = (struct ec_params_pwrmon *)st->msg->data;
+	params->cmd = EC_PWRMON_STOP;
+	ret = cros_ec_cmd_xfer_status(st->ec, st->msg);
+	if (ret < 0)
+		dev_warn(st->ec_dev->dev, "Failed to stop PACs: %d\n", ret);
+}
+
+static void cros_ec_pwrmon_unregister_notifier(void *data)
+{
+	struct cros_ec_pwrmon_state *st = data;
+
+	blocking_notifier_chain_unregister(&st->ec->event_notifier,
+					   &st->notifier);
+}
+
+static int cros_ec_pwrmon_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct cros_ec_dev *ec_dev = dev_get_drvdata(dev->parent);
+	struct cros_ec_pwrmon_state *st;
+	struct ec_params_pwrmon *params;
+	struct pwrmon_dump_info *dump;
+	struct iio_dev *indio_dev;
+	size_t msg_size;
+	int ret, i;
+
+	indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	st = iio_priv(indio_dev);
+	st->ec_dev = ec_dev;
+	st->ec = ec_dev->ec_dev;
+
+	spin_lock_init(&st->event_lock);
+	init_waitqueue_head(&st->wq);
+
+	ret = devm_mutex_init(dev, &st->lock);
+	if (ret)
+		return ret;
+
+	msg_size = sizeof(*st->msg) + max(sizeof(struct ec_params_pwrmon),
+					  sizeof(struct ec_response_pwrmon));
+	st->msg = devm_kzalloc(dev, msg_size, GFP_KERNEL);
+	if (!st->msg)
+		return -ENOMEM;
+
+	st->msg->version = 0;
+	st->msg->command = EC_CMD_PWRMON + st->ec_dev->cmd_offset;
+	params = (struct ec_params_pwrmon *)st->msg->data;
+	st->msg->outsize = sizeof(*params);
+	st->msg->insize = sizeof(struct ec_response_pwrmon);
+
+	/* Start PACs */
+	params->cmd = EC_PWRMON_START;
+	ret = cros_ec_cmd_xfer_status(st->ec, st->msg);
+	if (ret < 0) {
+		dev_warn(dev, "Failed to start PACs: %d\n", ret);
+		return ret;
+	}
+	st->last_time_ms = ktime_to_ms(ktime_get_boottime());
+
+	ret = devm_add_action_or_reset(dev, cros_ec_pwrmon_stop, st);
+	if (ret)
+		return ret;
+
+	/* Get Channel Count */
+	params->cmd = EC_PWRMON_GET_CHANNEL_COUNT;
+	ret = cros_ec_cmd_xfer_status(st->ec, st->msg);
+	if (ret < 0) {
+		dev_warn(dev, "Failed to get channel count: %d\n", ret);
+		return ret;
+	}
+	st->num_channels = ((struct ec_response_pwrmon *)st->msg->data)->channel_count;
+	if (st->num_channels > PWRMON_MAX_CHANNELS)
+		st->num_channels = PWRMON_MAX_CHANNELS;
+
+	st->accumulated_energy = devm_kcalloc(dev, st->num_channels,
+					      sizeof(*st->accumulated_energy),
+					      GFP_KERNEL);
+	st->channel_names = devm_kcalloc(dev, st->num_channels,
+					 sizeof(*st->channel_names),
+					 GFP_KERNEL);
+	if (!st->accumulated_energy || !st->channel_names)
+		return -ENOMEM;
+
+	/* Get Dump Info (Names) */
+	for (i = 0; i < st->num_channels; i++) {
+		params->cmd = EC_PWRMON_DUMP_INFO;
+		params->channel_id = i;
+		ret = cros_ec_cmd_xfer_status(st->ec, st->msg);
+		if (ret < 0) {
+			dev_warn(dev, "Failed to get dump info for channel %d: %d\n",
+				 i, ret);
+			continue;
+		}
+		dump = &((struct ec_response_pwrmon *)st->msg->data)->dump_info;
+		strscpy(st->channel_names[i], dump->channel_name,
+			sizeof(st->channel_names[i]));
+	}
+
+	indio_dev->name = dev_name(dev);
+	indio_dev->info = &cros_ec_pwrmon_info;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+
+	/* Register MKBP event notifier */
+	st->notifier.notifier_call = cros_ec_pwrmon_notify;
+	ret = blocking_notifier_chain_register(&st->ec->event_notifier,
+					       &st->notifier);
+	if (ret < 0) {
+		dev_err(dev, "Failed to register notifier: %d\n", ret);
+		return ret;
+	}
+
+	ret = devm_add_action_or_reset(dev, cros_ec_pwrmon_unregister_notifier,
+				       st);
+	if (ret)
+		return ret;
+
+	return devm_iio_device_register(dev, indio_dev);
+}
+
+static const struct platform_device_id cros_ec_pwrmon_id[] = {
+	{ "cros-ec-pwrmon", 0 },
+	{}
+};
+MODULE_DEVICE_TABLE(platform, cros_ec_pwrmon_id);
+
+static struct platform_driver cros_ec_pwrmon_driver = {
+	.driver = {
+		.name = "cros-ec-pwrmon",
+	},
+	.probe = cros_ec_pwrmon_probe,
+	.id_table = cros_ec_pwrmon_id,
+};
+module_platform_driver(cros_ec_pwrmon_driver);
+
+MODULE_AUTHOR("Daniel Balint <dbalint@google.com>");
+MODULE_DESCRIPTION("ChromeOS EC Power Monitor Driver");
+MODULE_LICENSE("GPL");
-- 
2.56.0.455.g6c5a503b38-goog

Re: [PATCH v1 2/3] iio: adc: add cros_ec_pwrmon driver

[Jonathan Cameron]

On Fri,  6 Nov 2026 19:31:16 +0000
Daniel Balint <dbalint@google.com> wrote:

> Add the ChromeOS EC On-Device Power Monitoring (ODPM) driver to read
> onboard power-monitoring ADCs (such as PAC194x) wired through the EC.
> 
> The driver interfaces with the EC via the existing EC_CMD_PWRMON host
> command API and EC_MKBP_EVENT_PWRMON events to start/stop accumulation,
> latch ADC accumulators, and expose cumulative per-rail energy in
> microjoules (uWs) through the energy_value sysfs attribute.
> 
> Signed-off-by: Daniel Balint <dbalint@google.com>
Hi Daniel

Main comment here is the custom ABI.  It doesn't comply with sysfs
conventions or the IIO ABI.  For this sort of capture lots at the
same time we have the IIO chardev 'buffer' interface.

Jonathan

> ---
>  .../ABI/testing/sysfs-bus-iio-cros-ec         |  13 +
>  MAINTAINERS                                   |   7 +
>  drivers/iio/adc/Kconfig                       |  11 +
>  drivers/iio/adc/Makefile                      |   1 +
>  drivers/iio/adc/cros_ec_pwrmon.c              | 333 ++++++++++++++++++
>  5 files changed, 365 insertions(+)
>  create mode 100644 drivers/iio/adc/cros_ec_pwrmon.c
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-iio-cros-ec b/Documentation/ABI/testing/sysfs-bus-iio-cros-ec
> index 3de1dfc98389..9043824f2831 100644
> --- a/Documentation/ABI/testing/sysfs-bus-iio-cros-ec
> +++ b/Documentation/ABI/testing/sysfs-bus-iio-cros-ec
> @@ -19,3 +19,16 @@ Description:
>  		represents the sensor ID as exposed by the EC. This ID is used
>  		by the Android sensor service hardware abstraction layer (sensor
>  		HAL) through the Android container on ChromeOS.
> +
> +What:		/sys/bus/iio/devices/iio:deviceX/energy_value

Why custom ABI? 

If you need a multi channel snapshot, use buffered IIO interface.
That should give you a nice binary representation of this will
sysfs description of data backing etc.  Hook it up to a sysfs
trigger if you want to have userspace request a capture on demand.

As a general rule, multi value sysfs attributes are frowned upon.
Occasionally we justify them as a group of numbers that are one
value (quaternions for instance) but this one doesn't
fit that pattern.

> +Date:		November 2026
> +KernelVersion:	7.5
> +Contact:	chrome-platform@lists.linux.dev, linux-iio@vger.kernel.org
> +Description:
> +		This read-only attribute is exposed by the ChromeOS EC power
> +		monitor (cros_ec_pwrmon) driver for On-Device Power Monitoring
> +		(ODPM). Reading it latches all EC power accumulators and returns
> +		a snapshot timestamp followed by per-channel accumulated energy:
> +
> +		t=<Measurement timestamp in ms since boot>
> +		CH<N>(T=<Duration in ms>)[<Rail schematic name>], <Accumulated energy in uWs>

> diff --git a/drivers/iio/adc/cros_ec_pwrmon.c b/drivers/iio/adc/cros_ec_pwrmon.c
> new file mode 100644
> index 000000000000..795b5f2b2a2c
> --- /dev/null
> +++ b/drivers/iio/adc/cros_ec_pwrmon.c
> @@ -0,0 +1,333 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * ChromeOS EC Power Monitor Driver
> + *
> + * Copyright (c) 2026 Google Inc.
> + */
> +
> +#include <linux/cleanup.h>
> +#include <linux/device.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/ktime.h>
> +#include <linux/math64.h>
> +#include <linux/mod_devicetable.h>

Shouldn't see that in a new driver.  The finer grained tables
structure defs in linux/device-id are included directly by
the relevant bus.  Here platform_device.h has it.

> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/notifier.h>
> +#include <linux/platform_data/cros_ec_commands.h>
> +#include <linux/platform_data/cros_ec_proto.h>
> +#include <linux/platform_device.h>
> +#include <linux/spinlock.h>
> +#include <linux/string.h>
> +#include <linux/sysfs.h>
> +#include <linux/types.h>
> +#include <linux/unaligned.h>
> +#include <linux/wait.h>

> +
> +static void cros_ec_pwrmon_unregister_notifier(void *data)
> +{
> +	struct cros_ec_pwrmon_state *st = data;
> +
> +	blocking_notifier_chain_unregister(&st->ec->event_notifier,
> +					   &st->notifier);
> +}
> +
> +static int cros_ec_pwrmon_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct cros_ec_dev *ec_dev = dev_get_drvdata(dev->parent);
> +	struct cros_ec_pwrmon_state *st;
> +	struct ec_params_pwrmon *params;
> +	struct pwrmon_dump_info *dump;
> +	struct iio_dev *indio_dev;
> +	size_t msg_size;
> +	int ret, i;
> +
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	st = iio_priv(indio_dev);
> +	st->ec_dev = ec_dev;
> +	st->ec = ec_dev->ec_dev;

That's somewhat confusing naming wise!  Why do we need both of them?

> +
> +	spin_lock_init(&st->event_lock);
> +	init_waitqueue_head(&st->wq);
> +
> +	ret = devm_mutex_init(dev, &st->lock);
> +	if (ret)
> +		return ret;
> +
> +	msg_size = sizeof(*st->msg) + max(sizeof(struct ec_params_pwrmon),

Given you immediately assign params to this, sizeof(*params)
would be nicer.

> +					  sizeof(struct ec_response_pwrmon));

It is a bit of an unusual case but I'd use struct_size() here as it has
a trailing u8 data[] into which you are putting the union of those two structures.
 
> +	st->msg = devm_kzalloc(dev, msg_size, GFP_KERNEL);
> +	if (!st->msg)
> +		return -ENOMEM;
> +
> +	st->msg->version = 0;
> +	st->msg->command = EC_CMD_PWRMON + st->ec_dev->cmd_offset;
> +	params = (struct ec_params_pwrmon *)st->msg->data;
> +	st->msg->outsize = sizeof(*params);
> +	st->msg->insize = sizeof(struct ec_response_pwrmon);
> +
> +	/* Start PACs */
> +	params->cmd = EC_PWRMON_START;
> +	ret = cros_ec_cmd_xfer_status(st->ec, st->msg);
> +	if (ret < 0) {
> +		dev_warn(dev, "Failed to start PACs: %d\n", ret);

Error out of probe?  Use

		return dev_err_probe();

> +		return ret;
> +	}
> +	st->last_time_ms = ktime_to_ms(ktime_get_boottime());
> +
> +	ret = devm_add_action_or_reset(dev, cros_ec_pwrmon_stop, st);
> +	if (ret)
> +		return ret;
> +
> +	/* Get Channel Count */
> +	params->cmd = EC_PWRMON_GET_CHANNEL_COUNT;
> +	ret = cros_ec_cmd_xfer_status(st->ec, st->msg);
> +	if (ret < 0) {
> +		dev_warn(dev, "Failed to get channel count: %d\n", ret);
> +		return ret;

return dev_err_probe()

> +	}
> +	st->num_channels = ((struct ec_response_pwrmon *)st->msg->data)->channel_count;

Use a local variable rather than cast. Added bonus is you can use it for sizing the
storage above rather.

> +	if (st->num_channels > PWRMON_MAX_CHANNELS)
> +		st->num_channels = PWRMON_MAX_CHANNELS;

	st->num_channels = min(st->num_channels, PWRMON_MAX_CHANNELS);

> +
> +	st->accumulated_energy = devm_kcalloc(dev, st->num_channels,
> +					      sizeof(*st->accumulated_energy),
> +					      GFP_KERNEL);
> +	st->channel_names = devm_kcalloc(dev, st->num_channels,
> +					 sizeof(*st->channel_names),
> +					 GFP_KERNEL);
> +	if (!st->accumulated_energy || !st->channel_names)
> +		return -ENOMEM;
> +
> +	/* Get Dump Info (Names) */
> +	for (i = 0; i < st->num_channels; i++) {

	for (unsigned int i = 0; ...
as might as well keep it locally scoped.


> +		params->cmd = EC_PWRMON_DUMP_INFO;
> +		params->channel_id = i;
> +		ret = cros_ec_cmd_xfer_status(st->ec, st->msg);
> +		if (ret < 0) {
> +			dev_warn(dev, "Failed to get dump info for channel %d: %d\n",
> +				 i, ret);
> +			continue;
> +		}
> +		dump = &((struct ec_response_pwrmon *)st->msg->data)->dump_info;

As above, local variable seems more appropriate.

> +		strscpy(st->channel_names[i], dump->channel_name,
> +			sizeof(st->channel_names[i]));
> +	}
> +
> +	indio_dev->name = dev_name(dev);

Generally we want a part number for dev name. What does this end up as here?
Often it is better to hard code them.

> +	indio_dev->info = &cros_ec_pwrmon_info;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +
> +	/* Register MKBP event notifier */
> +	st->notifier.notifier_call = cros_ec_pwrmon_notify;
> +	ret = blocking_notifier_chain_register(&st->ec->event_notifier,
> +					       &st->notifier);
> +	if (ret < 0) {
> +		dev_err(dev, "Failed to register notifier: %d\n", ret);
> +		return ret;

return dev_err_probe()

> +	}
> +
> +	ret = devm_add_action_or_reset(dev, cros_ec_pwrmon_unregister_notifier,
> +				       st);

This one has annoyed me before so maybe not really an ask for you to fix
but it would be nice to just have a devm_blocking _notifier_chain_register()
that would stash both parameters needed for the unregister.

> +	if (ret)
> +		return ret;
> +
> +	return devm_iio_device_register(dev, indio_dev);
> +}
> +
> +static const struct platform_device_id cros_ec_pwrmon_id[] = {
> +	{ "cros-ec-pwrmon", 0 },
Named initializers for these.  This is slowly getting tidied
up across the kernel. Not using the named fields makes certain
types of refactor trickier to do.  Don't set the 0 though
as that's what C will fill in anyway and for now there is no
use of that value.
> +	{}
	{ }

That one is a trivial IIO style preference.  We ended up standardizing
that a few years back in the interests of consistency and the choice
was with the space (there was no strong reason to go one way or the other!)

Jonathan

[PATCH v1 3/3] mfd: cros_ec: load cros_ec_pwrmon on supported ECs

[Daniel Balint]

Load cros_ec_pwrmon driver when the ChromeOS EC advertises
EC_FEATURE_PWRMON.

Signed-off-by: Daniel Balint <dbalint@google.com>
---
 drivers/mfd/cros_ec_dev.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/mfd/cros_ec_dev.c b/drivers/mfd/cros_ec_dev.c
index 529a23b804f7..9cf85636b7f9 100644
--- a/drivers/mfd/cros_ec_dev.c
+++ b/drivers/mfd/cros_ec_dev.c
@@ -115,6 +115,10 @@ static const struct mfd_cell cros_ec_charge_control_cells[] = {
 	{ .name = "cros-charge-control", },
 };
 
+static const struct mfd_cell cros_ec_pwrmon_cells[] = {
+	{ .name = "cros-ec-pwrmon", },
+};
+
 static const struct cros_feature_to_cells cros_subdevices[] = {
 	{
 		.id		= EC_FEATURE_CEC,
@@ -151,6 +155,11 @@ static const struct cros_feature_to_cells cros_subdevices[] = {
 		.mfd_cells	= cros_ec_charge_control_cells,
 		.num_cells	= ARRAY_SIZE(cros_ec_charge_control_cells),
 	},
+	{
+		.id		= EC_FEATURE_PWRMON,
+		.mfd_cells	= cros_ec_pwrmon_cells,
+		.num_cells	= ARRAY_SIZE(cros_ec_pwrmon_cells),
+	},
 };
 
 static const struct mfd_cell cros_ec_platform_cells[] = {
-- 
2.56.0.455.g6c5a503b38-goog

Re: [PATCH v1 0/3] Introduce cros_ec power monitoring

[Jonathan Cameron]

On Fri,  6 Nov 2026 19:31:14 +0000
Daniel Balint <dbalint@google.com> wrote:

> This series adds the ChromeOS EC On-Device Power Monitoring 
> (cros_ec_pwrmon) driver to read onboard power-monitoring ADCs,
> such as PAC194x, wired through the embedded controller (EC).
> 
> Several ChromeOS platforms are equipped with onboard ADCs that are
> connected to the EC via I2C bus. The driver interfaces with the EC
> via the existing EC_CMD_PWRMON host command API (and EC_MKBP_EVENT_PWRMON
> events) to manage ADC accumulation and expose accumulated per-rail energy
> through an IIO sysfs attribute.
> 
> The driver is only loaded when the underlying hardware feature is present.
> The series also fixes alignment padding in EC_CMD_PWRMON definitions.

Hi Daniel

I'm not necessarily against this but when adding monitoring components
to IIO the usual question is why not hwmon?  What features are not
well supported by that framework?

Note historically I suspect we accepted one or two drives into IIO that
would have fitted better there so precedence isn't a strong argument.

Convention for these somewhat borderline case is +CC hmwon maintainer
and list as well as IIO with that explanation in the cover letter.
That avoids any last minute surprises!

Jonathan

> 
> Daniel Balint (3):
>   platform/chrome: cros_ec: fix alignment of ec_params_pwrmon
>   iio: adc: add cros_ec_pwrmon driver
>   mfd: cros_ec: load cros_ec_pwrmon on supported ECs
> 
>  .../ABI/testing/sysfs-bus-iio-cros-ec         |  13 +
>  MAINTAINERS                                   |   7 +
>  drivers/iio/adc/Kconfig                       |  11 +
>  drivers/iio/adc/Makefile                      |   1 +
>  drivers/iio/adc/cros_ec_pwrmon.c              | 333 ++++++++++++++++++
>  drivers/mfd/cros_ec_dev.c                     |   9 +
>  .../linux/platform_data/cros_ec_commands.h    |   2 +
>  7 files changed, 376 insertions(+)
>  create mode 100644 drivers/iio/adc/cros_ec_pwrmon.c
> 
> 
> base-commit: 6628e7d4c6b25c8d18f1345e3a20d21829aae7e4