[PATCH v4] iio: cros_ec_sensors: add cros_ec_activity driver

[PATCH v4] iio: cros_ec_sensors: add cros_ec_activity driver

[Gwendal Grignou]

ChromeOS EC can report activity information derived from the
accelerometer:
- Reports on-body/off-body as a proximity event.
- Reports significant motion as an activity event.

This new sensor is a virtual sensor, included only when the EC firmware
is compiled with the appropriate module.

Signed-off-by: Gwendal Grignou <gwendal@google.com>
Reviewed-by: Tzung-Bi Shih <tzungbi@kernel.org>
---
Changes in v2:
- fix usage of cros_ec_motion_send_host_cmd
- use dev instead of &pdev->dev
- fix documentation errors.
Changes in v3:
- fix index increment at probe correctly
- prevent sending command on invalid inputs in
  cros_ec_write_event_config
Changes in v4:
- no need to continue setting command structure on error.

 drivers/iio/common/cros_ec_sensors/Kconfig    |   9 +
 drivers/iio/common/cros_ec_sensors/Makefile   |   1 +
 .../common/cros_ec_sensors/cros_ec_activity.c | 322 ++++++++++++++++++
 .../cros_ec_sensors/cros_ec_sensors_core.c    |  10 +
 .../linux/iio/common/cros_ec_sensors_core.h   |   1 +
 .../linux/platform_data/cros_ec_commands.h    |  36 +-
 6 files changed, 373 insertions(+), 6 deletions(-)
 create mode 100644 drivers/iio/common/cros_ec_sensors/cros_ec_activity.c

diff --git a/drivers/iio/common/cros_ec_sensors/Kconfig b/drivers/iio/common/cros_ec_sensors/Kconfig
index fefad9572790..394e319c9c97 100644
--- a/drivers/iio/common/cros_ec_sensors/Kconfig
+++ b/drivers/iio/common/cros_ec_sensors/Kconfig
@@ -30,3 +30,12 @@ config IIO_CROS_EC_SENSORS_LID_ANGLE
 	  convertible devices.
 	  This module is loaded when the EC can calculate the angle between the base
 	  and the lid.
+
+config IIO_CROS_EC_ACTIVITY
+	tristate "ChromeOS EC Activity Sensors"
+	depends on IIO_CROS_EC_SENSORS_CORE
+	help
+	  Module to handle activity events presented by the ChromeOS EC sensor hub.
+	  Activities can be a proximity detector (on body/off body detection)
+	  or a significant motion detector.
+	  Creates an IIO device to manage all activities.
diff --git a/drivers/iio/common/cros_ec_sensors/Makefile b/drivers/iio/common/cros_ec_sensors/Makefile
index e0a33ab66d21..a7849c52f3c8 100644
--- a/drivers/iio/common/cros_ec_sensors/Makefile
+++ b/drivers/iio/common/cros_ec_sensors/Makefile
@@ -3,6 +3,7 @@
 # Makefile for sensors seen through the ChromeOS EC sensor hub.
 #
 
+obj-$(CONFIG_IIO_CROS_EC_ACTIVITY) += cros_ec_activity.o
 obj-$(CONFIG_IIO_CROS_EC_SENSORS_CORE) += cros_ec_sensors_core.o
 obj-$(CONFIG_IIO_CROS_EC_SENSORS) += cros_ec_sensors.o
 obj-$(CONFIG_IIO_CROS_EC_SENSORS_LID_ANGLE) += cros_ec_lid_angle.o
diff --git a/drivers/iio/common/cros_ec_sensors/cros_ec_activity.c b/drivers/iio/common/cros_ec_sensors/cros_ec_activity.c
new file mode 100644
index 000000000000..620203f7aa63
--- /dev/null
+++ b/drivers/iio/common/cros_ec_sensors/cros_ec_activity.c
@@ -0,0 +1,322 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * cros_ec_activity - Driver for activities/gesture recognition.
+ *
+ * Copyright 2025 Google, Inc
+ *
+ * This driver uses the cros-ec interface to communicate with the ChromeOS
+ * EC about activity data. Accelerometer access is presented through iio sysfs.
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/platform_device.h>
+#include <linux/types.h>
+
+#include <linux/platform_data/cros_ec_commands.h>
+#include <linux/platform_data/cros_ec_proto.h>
+
+#include <linux/iio/common/cros_ec_sensors_core.h>
+#include <linux/iio/events.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/trigger_consumer.h>
+
+#define DRV_NAME "cros-ec-activity"
+
+/* state data for ec_sensors iio driver. */
+struct cros_ec_sensors_state {
+	/* Shared by all sensors */
+	struct cros_ec_sensors_core_state core;
+
+	struct iio_chan_spec *channels;
+
+	int body_detection_channel_index;
+	int sig_motion_channel_index;
+};
+
+static const struct iio_event_spec cros_ec_activity_single_shot[] = {
+	{
+		.type = IIO_EV_TYPE_CHANGE,
+		/* significant motion trigger when we get out of still. */
+		.dir = IIO_EV_DIR_FALLING,
+		.mask_separate = BIT(IIO_EV_INFO_ENABLE),
+	},
+};
+
+static const struct iio_event_spec cros_ec_body_detect_events[] = {
+	{
+		.type = IIO_EV_TYPE_CHANGE,
+		.dir = IIO_EV_DIR_EITHER,
+		.mask_separate = BIT(IIO_EV_INFO_ENABLE),
+	},
+};
+
+static int ec_sensors_read(struct iio_dev *indio_dev,
+			   struct iio_chan_spec const *chan,
+			   int *val, int *val2, long mask)
+{
+	struct cros_ec_sensors_state *st = iio_priv(indio_dev);
+	int ret;
+
+	mutex_lock(&st->core.cmd_lock);
+	if (chan->type == IIO_PROXIMITY && mask == IIO_CHAN_INFO_RAW) {
+		st->core.param.cmd = MOTIONSENSE_CMD_GET_ACTIVITY;
+		st->core.param.get_activity.activity =
+			MOTIONSENSE_ACTIVITY_BODY_DETECTION;
+		ret = cros_ec_motion_send_host_cmd(&st->core, 0);
+		if (ret == 0) {
+			*val = st->core.resp->get_activity.state;
+			ret = IIO_VAL_INT;
+		}
+	} else {
+		ret = -EINVAL;
+	}
+	mutex_unlock(&st->core.cmd_lock);
+	return ret;
+}
+
+static int cros_ec_read_event_config(struct iio_dev *indio_dev,
+				     const struct iio_chan_spec *chan,
+				     enum iio_event_type type,
+				     enum iio_event_direction dir)
+{
+	struct cros_ec_sensors_state *st = iio_priv(indio_dev);
+	int ret;
+
+	if (chan->type != IIO_ACTIVITY && chan->type != IIO_PROXIMITY)
+		return -EINVAL;
+
+	mutex_lock(&st->core.cmd_lock);
+	st->core.param.cmd = MOTIONSENSE_CMD_LIST_ACTIVITIES;
+	ret = cros_ec_motion_send_host_cmd(&st->core, 0);
+	if (ret)
+		goto done;
+	switch (chan->type) {
+	case IIO_PROXIMITY:
+		ret = !!(st->core.resp->list_activities.enabled &
+			 (1 << MOTIONSENSE_ACTIVITY_BODY_DETECTION));
+		break;
+	case IIO_ACTIVITY:
+		if (chan->channel2 == IIO_MOD_STILL) {
+			ret = !!(st->core.resp->list_activities.enabled &
+				 (1 << MOTIONSENSE_ACTIVITY_SIG_MOTION));
+		} else {
+			dev_warn(&indio_dev->dev, "Unknown activity: %d\n",
+				 chan->channel2);
+			ret = -EINVAL;
+		}
+		break;
+	default:
+		dev_warn(&indio_dev->dev, "Unknown channel type: %d\n",
+			 chan->type);
+		ret = -EINVAL;
+	}
+done:
+	mutex_unlock(&st->core.cmd_lock);
+	return ret;
+}
+
+static int cros_ec_write_event_config(struct iio_dev *indio_dev,
+				      const struct iio_chan_spec *chan,
+				      enum iio_event_type type,
+				      enum iio_event_direction dir, int state)
+{
+	struct cros_ec_sensors_state *st = iio_priv(indio_dev);
+	int ret = 0;
+
+	mutex_lock(&st->core.cmd_lock);
+	st->core.param.cmd = MOTIONSENSE_CMD_SET_ACTIVITY;
+	switch (chan->type) {
+	case IIO_PROXIMITY:
+		st->core.param.set_activity.activity =
+			MOTIONSENSE_ACTIVITY_BODY_DETECTION;
+		break;
+	case IIO_ACTIVITY:
+		if (chan->channel2 == IIO_MOD_STILL) {
+			st->core.param.set_activity.activity =
+				MOTIONSENSE_ACTIVITY_SIG_MOTION;
+		} else {
+			dev_warn(&indio_dev->dev, "Unknown activity: %d\n",
+				 chan->channel2);
+			ret = -EINVAL;
+		}
+		break;
+	default:
+		dev_warn(&indio_dev->dev, "Unknown channel type: %d\n",
+			 chan->type);
+		ret = -EINVAL;
+	}
+	if (ret == 0) {
+		st->core.param.set_activity.enable = state;
+		ret = cros_ec_motion_send_host_cmd(&st->core, 0);
+	}
+
+	mutex_unlock(&st->core.cmd_lock);
+	return ret;
+}
+
+static int cros_ec_activity_push_data(struct iio_dev *indio_dev,
+				      s16 *data,
+				      s64 timestamp)
+{
+	struct ec_response_activity_data *activity_data =
+			(struct ec_response_activity_data *)data;
+	enum motionsensor_activity activity = activity_data->activity;
+	u8 state = activity_data->state;
+	const struct cros_ec_sensors_state *st = iio_priv(indio_dev);
+	const struct iio_chan_spec *chan;
+	const struct iio_event_spec *event;
+	enum iio_event_direction dir;
+	int index;
+	u64 ev;
+
+	switch (activity) {
+	case MOTIONSENSE_ACTIVITY_BODY_DETECTION:
+		index = st->body_detection_channel_index;
+		dir = state ? IIO_EV_DIR_FALLING : IIO_EV_DIR_RISING;
+		break;
+	case MOTIONSENSE_ACTIVITY_SIG_MOTION:
+		index = st->sig_motion_channel_index;
+		dir = IIO_EV_DIR_FALLING;
+		break;
+	default:
+		dev_warn(&indio_dev->dev, "Unknown activity: %d\n", activity);
+		return 0;
+	}
+	chan = &st->channels[index];
+	event = &chan->event_spec[0];
+
+	ev = IIO_UNMOD_EVENT_CODE(chan->type, index, event->type, dir);
+	iio_push_event(indio_dev, ev, timestamp);
+	return 0;
+}
+
+static irqreturn_t cros_ec_activity_capture(int irq, void *p)
+{
+	struct iio_poll_func *pf = p;
+	struct iio_dev *indio_dev = pf->indio_dev;
+
+	dev_warn(&indio_dev->dev, "%s: Not Expected\n", __func__);
+	return IRQ_NONE;
+}
+
+static const struct iio_info ec_sensors_info = {
+	.read_raw = &ec_sensors_read,
+	.read_event_config = cros_ec_read_event_config,
+	.write_event_config = cros_ec_write_event_config,
+};
+
+static int cros_ec_sensors_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct cros_ec_device *ec_device = dev_get_drvdata(dev->parent);
+	struct iio_dev *indio_dev;
+	struct cros_ec_sensors_state *st;
+	struct iio_chan_spec *channel;
+	unsigned long activities;
+	int i, index, ret, nb_activities;
+
+	if (!ec_device) {
+		dev_warn(dev, "No CROS EC device found.\n");
+		return -EINVAL;
+	}
+
+	indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	ret = cros_ec_sensors_core_init(pdev, indio_dev, true,
+					cros_ec_activity_capture);
+	if (ret)
+		return ret;
+
+	indio_dev->info = &ec_sensors_info;
+	st = iio_priv(indio_dev);
+	st->core.type = st->core.resp->info.type;
+	st->core.read_ec_sensors_data = cros_ec_sensors_read_cmd;
+
+	/*
+	 * List all available activities
+	 */
+	st->core.param.cmd = MOTIONSENSE_CMD_LIST_ACTIVITIES;
+	ret = cros_ec_motion_send_host_cmd(&st->core, 0);
+	if (ret)
+		return ret;
+	activities = st->core.resp->list_activities.enabled |
+		     st->core.resp->list_activities.disabled;
+	if (!activities)
+		return -ENODEV;
+
+	/* Allocate a channel per activity and one for timestamp */
+	nb_activities = hweight_long(activities) + 1;
+	st->channels = devm_kcalloc(dev, nb_activities,
+				    sizeof(*st->channels), GFP_KERNEL);
+	if (!st->channels)
+		return -ENOMEM;
+
+	channel = &st->channels[0];
+	index = 0;
+	for_each_set_bit(i, &activities, BITS_PER_LONG) {
+		/* List all available triggers */
+		if (i == MOTIONSENSE_ACTIVITY_BODY_DETECTION) {
+			channel->type = IIO_PROXIMITY;
+			channel->info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
+			channel->modified = 0;
+			channel->event_spec = cros_ec_body_detect_events;
+			channel->num_event_specs =
+				ARRAY_SIZE(cros_ec_body_detect_events);
+			st->body_detection_channel_index = index;
+		} else {
+			channel->type = IIO_ACTIVITY;
+			channel->modified = 1;
+			channel->event_spec = cros_ec_activity_single_shot;
+			channel->num_event_specs =
+				ARRAY_SIZE(cros_ec_activity_single_shot);
+			if (i == MOTIONSENSE_ACTIVITY_SIG_MOTION) {
+				channel->channel2 = IIO_MOD_STILL;
+				st->sig_motion_channel_index = index;
+			} else {
+				dev_warn(dev, "Unknown activity: %d\n", i);
+				continue;
+			}
+		}
+		channel->ext_info = cros_ec_sensors_limited_info;
+		channel->scan_index = index++;
+		channel++;
+	}
+
+	/* Timestamp */
+	channel->scan_index = index;
+	channel->type = IIO_TIMESTAMP;
+	channel->channel = -1;
+	channel->scan_type.sign = 's';
+	channel->scan_type.realbits = 64;
+	channel->scan_type.storagebits = 64;
+
+	indio_dev->channels = st->channels;
+	indio_dev->num_channels = index + 1;
+
+	return cros_ec_sensors_core_register(dev, indio_dev,
+					     cros_ec_activity_push_data);
+}
+
+static void cros_ec_sensors_remove(struct platform_device *pdev)
+{
+	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
+
+	iio_device_unregister(indio_dev);
+}
+
+static struct platform_driver cros_ec_sensors_platform_driver = {
+	.driver = {
+		.name	= DRV_NAME,
+	},
+	.probe		= cros_ec_sensors_probe,
+	.remove		= cros_ec_sensors_remove,
+};
+module_platform_driver(cros_ec_sensors_platform_driver);
+
+MODULE_DESCRIPTION("ChromeOS EC activity sensors driver");
+MODULE_ALIAS("platform:" DRV_NAME);
+MODULE_LICENSE("GPL v2");
diff --git a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c
index 64bada1e8678..1bd07ca3abfe 100644
--- a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c
+++ b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c
@@ -489,6 +489,16 @@ const struct iio_chan_spec_ext_info cros_ec_sensors_ext_info[] = {
 };
 EXPORT_SYMBOL_GPL(cros_ec_sensors_ext_info);
 
+const struct iio_chan_spec_ext_info cros_ec_sensors_limited_info[] = {
+	{
+		.name = "id",
+		.shared = IIO_SHARED_BY_ALL,
+		.read = cros_ec_sensors_id
+	},
+	{ },
+};
+EXPORT_SYMBOL_GPL(cros_ec_sensors_limited_info);
+
 /**
  * cros_ec_sensors_idx_to_reg - convert index into offset in shared memory
  * @st:		pointer to state information for device
diff --git a/include/linux/iio/common/cros_ec_sensors_core.h b/include/linux/iio/common/cros_ec_sensors_core.h
index e72167b96d27..bb966abcde53 100644
--- a/include/linux/iio/common/cros_ec_sensors_core.h
+++ b/include/linux/iio/common/cros_ec_sensors_core.h
@@ -126,5 +126,6 @@ extern const struct dev_pm_ops cros_ec_sensors_pm_ops;
 
 /* List of extended channel specification for all sensors. */
 extern const struct iio_chan_spec_ext_info cros_ec_sensors_ext_info[];
+extern const struct iio_chan_spec_ext_info cros_ec_sensors_limited_info[];
 
 #endif  /* __CROS_EC_SENSORS_CORE_H */
diff --git a/include/linux/platform_data/cros_ec_commands.h b/include/linux/platform_data/cros_ec_commands.h
index ecf290a0c98f..0f6d53e7cb97 100644
--- a/include/linux/platform_data/cros_ec_commands.h
+++ b/include/linux/platform_data/cros_ec_commands.h
@@ -2388,6 +2388,12 @@ enum motionsense_command {
 	 */
 	MOTIONSENSE_CMD_SENSOR_SCALE = 18,
 
+	/*
+	 * Activity management
+	 * Retrieve current status of given activity.
+	 */
+	MOTIONSENSE_CMD_GET_ACTIVITY = 20,
+
 	/* Number of motionsense sub-commands. */
 	MOTIONSENSE_NUM_CMDS
 };
@@ -2447,6 +2453,11 @@ enum motionsensor_orientation {
 	MOTIONSENSE_ORIENTATION_UNKNOWN = 4,
 };
 
+struct ec_response_activity_data {
+	uint8_t activity; /* motionsensor_activity */
+	uint8_t state;
+} __ec_todo_packed;
+
 struct ec_response_motion_sensor_data {
 	/* Flags for each sensor. */
 	uint8_t flags;
@@ -2454,15 +2465,14 @@ struct ec_response_motion_sensor_data {
 	uint8_t sensor_num;
 	/* Each sensor is up to 3-axis. */
 	union {
-		int16_t             data[3];
+		int16_t                                  data[3];
 		struct __ec_todo_packed {
-			uint16_t    reserved;
-			uint32_t    timestamp;
+			uint16_t                         reserved;
+			uint32_t                         timestamp;
 		};
 		struct __ec_todo_unpacked {
-			uint8_t     activity; /* motionsensor_activity */
-			uint8_t     state;
-			int16_t     add_info[2];
+			struct ec_response_activity_data activity_data;
+			int16_t                          add_info[2];
 		};
 	};
 } __ec_todo_packed;
@@ -2494,6 +2504,7 @@ enum motionsensor_activity {
 	MOTIONSENSE_ACTIVITY_SIG_MOTION = 1,
 	MOTIONSENSE_ACTIVITY_DOUBLE_TAP = 2,
 	MOTIONSENSE_ACTIVITY_ORIENTATION = 3,
+	MOTIONSENSE_ACTIVITY_BODY_DETECTION = 4,
 };
 
 struct ec_motion_sense_activity {
@@ -2671,6 +2682,7 @@ struct ec_params_motion_sense {
 			uint32_t max_data_vector;
 		} fifo_read;
 
+		/* Used for MOTIONSENSE_CMD_SET_ACTIVITY */
 		struct ec_motion_sense_activity set_activity;
 
 		/* Used for MOTIONSENSE_CMD_LID_ANGLE */
@@ -2716,6 +2728,14 @@ struct ec_params_motion_sense {
 			 */
 			int16_t hys_degree;
 		} tablet_mode_threshold;
+
+		/*
+		 * Used for MOTIONSENSE_CMD_GET_ACTIVITY.
+		 */
+		struct __ec_todo_unpacked {
+			uint8_t sensor_num;
+			uint8_t activity;  /* enum motionsensor_activity */
+		} get_activity;
 	};
 } __ec_todo_packed;
 
@@ -2833,6 +2853,10 @@ struct ec_response_motion_sense {
 			uint16_t hys_degree;
 		} tablet_mode_threshold;
 
+		/* USED for MOTIONSENSE_CMD_GET_ACTIVITY. */
+		struct __ec_todo_unpacked {
+			uint8_t     state;
+		} get_activity;
 	};
 } __ec_todo_packed;
 
-- 
2.49.0.1151.ga128411c76-goog

Re: [PATCH v4] iio: cros_ec_sensors: add cros_ec_activity driver

[Jonathan Cameron]

On Fri, 23 May 2025 10:27:27 -0700
Gwendal Grignou <gwendal@chromium.org> wrote:

Hi Gwendal,

> ChromeOS EC can report activity information derived from the
> accelerometer:
> - Reports on-body/off-body as a proximity event.

How do you do proximity from an accelerometer?

> - Reports significant motion as an activity event.

This one might be from an accelerometer. In which case is there
any information on the algorithm?

> 
> This new sensor is a virtual sensor, included only when the EC firmware
> is compiled with the appropriate module.
> 
> Signed-off-by: Gwendal Grignou <gwendal@google.com>
> Reviewed-by: Tzung-Bi Shih <tzungbi@kernel.org>


Various comments inline.

Biggest question I have is what the significant motion detector
is based on. Do we know more about that as I'd prefer it associated
with an actual channel if possible?

Also does it only surface as an event?  Is there an underlying
value to read.


> diff --git a/drivers/iio/common/cros_ec_sensors/cros_ec_activity.c b/drivers/iio/common/cros_ec_sensors/cros_ec_activity.c
> new file mode 100644
> index 000000000000..620203f7aa63
> --- /dev/null
> +++ b/drivers/iio/common/cros_ec_sensors/cros_ec_activity.c
> @@ -0,0 +1,322 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * cros_ec_activity - Driver for activities/gesture recognition.
> + *
> + * Copyright 2025 Google, Inc
> + *
> + * This driver uses the cros-ec interface to communicate with the ChromeOS
> + * EC about activity data. Accelerometer access is presented through iio sysfs.

Maybe update as this isn't returning accelerometer data .

> + */

> +
> +static int ec_sensors_read(struct iio_dev *indio_dev,

Given this only applies to this sensor, maybe needs a more specific name?

> +			   struct iio_chan_spec const *chan,
> +			   int *val, int *val2, long mask)
> +{
> +	struct cros_ec_sensors_state *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	mutex_lock(&st->core.cmd_lock);
Include cleanup.h and use

	guard(mutex)(&st->core.cmd_lock);
to simpilfy this.

> +	if (chan->type == IIO_PROXIMITY && mask == IIO_CHAN_INFO_RAW) {
> +		st->core.param.cmd = MOTIONSENSE_CMD_GET_ACTIVITY;
> +		st->core.param.get_activity.activity =
> +			MOTIONSENSE_ACTIVITY_BODY_DETECTION;
> +		ret = cros_ec_motion_send_host_cmd(&st->core, 0);
> +		if (ret == 0) {
> +			*val = st->core.resp->get_activity.state;
> +			ret = IIO_VAL_INT;
With guard magic, can handle error out path and return directly in good path.
		if (ret)
			return ret;

		*val = ..

		return IIO_VAL_INT;

> +		}
> +	} else {
> +		ret = -EINVAL;

With guard() magic can just return here.

> +	}
> +	mutex_unlock(&st->core.cmd_lock);
> +	return ret;
> +}
> +
> +static int cros_ec_read_event_config(struct iio_dev *indio_dev,
> +				     const struct iio_chan_spec *chan,
> +				     enum iio_event_type type,
> +				     enum iio_event_direction dir)
> +{
> +	struct cros_ec_sensors_state *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	if (chan->type != IIO_ACTIVITY && chan->type != IIO_PROXIMITY)
> +		return -EINVAL;
> +
> +	mutex_lock(&st->core.cmd_lock);

Similar to below. guard() will simplify this quite a bit.

> +	st->core.param.cmd = MOTIONSENSE_CMD_LIST_ACTIVITIES;
> +	ret = cros_ec_motion_send_host_cmd(&st->core, 0);
> +	if (ret)
> +		goto done;

Blank line here.

> +	switch (chan->type) {
> +	case IIO_PROXIMITY:
> +		ret = !!(st->core.resp->list_activities.enabled &
> +			 (1 << MOTIONSENSE_ACTIVITY_BODY_DETECTION));
> +		break;
> +	case IIO_ACTIVITY:
> +		if (chan->channel2 == IIO_MOD_STILL) {
> +			ret = !!(st->core.resp->list_activities.enabled &
> +				 (1 << MOTIONSENSE_ACTIVITY_SIG_MOTION));
> +		} else {
> +			dev_warn(&indio_dev->dev, "Unknown activity: %d\n",
> +				 chan->channel2);
> +			ret = -EINVAL;

Flip logic and direct returns etc.

> +		}
> +		break;
> +	default:
> +		dev_warn(&indio_dev->dev, "Unknown channel type: %d\n",
> +			 chan->type);
> +		ret = -EINVAL;
> +	}
> +done:
> +	mutex_unlock(&st->core.cmd_lock);
> +	return ret;
> +}
> +
> +static int cros_ec_write_event_config(struct iio_dev *indio_dev,
> +				      const struct iio_chan_spec *chan,
> +				      enum iio_event_type type,
> +				      enum iio_event_direction dir, int state)
> +{
> +	struct cros_ec_sensors_state *st = iio_priv(indio_dev);
> +	int ret = 0;
> +
> +	mutex_lock(&st->core.cmd_lock);

Guard and direct returns. (See below for more on this)

> +	st->core.param.cmd = MOTIONSENSE_CMD_SET_ACTIVITY;
> +	switch (chan->type) {
> +	case IIO_PROXIMITY:
> +		st->core.param.set_activity.activity =
> +			MOTIONSENSE_ACTIVITY_BODY_DETECTION;
> +		break;
> +	case IIO_ACTIVITY:
> +		if (chan->channel2 == IIO_MOD_STILL) {
> +			st->core.param.set_activity.activity =
> +				MOTIONSENSE_ACTIVITY_SIG_MOTION;
> +		} else {
flip logic so error is out of line and good path inline.  Then with
guard you an directly return for error path.

> +			dev_warn(&indio_dev->dev, "Unknown activity: %d\n",
> +				 chan->channel2);
> +			ret = -EINVAL;
> +		}
> +		break;
> +	default:
> +		dev_warn(&indio_dev->dev, "Unknown channel type: %d\n",
> +			 chan->type);
> +		ret = -EINVAL;
		return -EINVAL; when using guard()
> +	}
> +	if (ret == 0) {

Then we only get here if ret == 0 so condition can go away.

> +		st->core.param.set_activity.enable = state;
> +		ret = cros_ec_motion_send_host_cmd(&st->core, 0);
> +	}
> +
> +	mutex_unlock(&st->core.cmd_lock);
> +	return ret;
> +}
> +
> +static int cros_ec_activity_push_data(struct iio_dev *indio_dev,
> +				      s16 *data,
> +				      s64 timestamp)

Can reduce wrap and have a parameters share a line.  Nice to stay under 80
but this is well below that.

> +{
> +	struct ec_response_activity_data *activity_data =
> +			(struct ec_response_activity_data *)data;
> +	enum motionsensor_activity activity = activity_data->activity;
> +	u8 state = activity_data->state;
> +	const struct cros_ec_sensors_state *st = iio_priv(indio_dev);
> +	const struct iio_chan_spec *chan;
> +	const struct iio_event_spec *event;
> +	enum iio_event_direction dir;
> +	int index;
> +	u64 ev;
> +
> +	switch (activity) {
> +	case MOTIONSENSE_ACTIVITY_BODY_DETECTION:
> +		index = st->body_detection_channel_index;
> +		dir = state ? IIO_EV_DIR_FALLING : IIO_EV_DIR_RISING;
> +		break;
> +	case MOTIONSENSE_ACTIVITY_SIG_MOTION:
> +		index = st->sig_motion_channel_index;
> +		dir = IIO_EV_DIR_FALLING;

Falling stillness is I guess a detection of activity... As below though I'd
like to know if we have any more info on what this is actually measuring?

> +		break;
> +	default:
> +		dev_warn(&indio_dev->dev, "Unknown activity: %d\n", activity);
> +		return 0;
> +	}
> +	chan = &st->channels[index];
> +	event = &chan->event_spec[0];

I'd not have so many local variables. Pick some to put inline. Either
these two or ev.  Just in the interests of shorter code and little
cost to readability.

> +
> +	ev = IIO_UNMOD_EVENT_CODE(chan->type, index, event->type, dir);
> +	iio_push_event(indio_dev, ev, timestamp);

Blank line here.
> +	return 0;
> +}
> +
> +static irqreturn_t cros_ec_activity_capture(int irq, void *p)
> +{
> +	struct iio_poll_func *pf = p;
> +	struct iio_dev *indio_dev = pf->indio_dev;
> +
> +	dev_warn(&indio_dev->dev, "%s: Not Expected\n", __func__);

Add a comment on why.

> +	return IRQ_NONE;
> +}

> +static int cros_ec_sensors_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct cros_ec_device *ec_device = dev_get_drvdata(dev->parent);
> +	struct iio_dev *indio_dev;
> +	struct cros_ec_sensors_state *st;
> +	struct iio_chan_spec *channel;
> +	unsigned long activities;
> +	int i, index, ret, nb_activities;
> +
> +	if (!ec_device) {
> +		dev_warn(dev, "No CROS EC device found.\n");
> +		return -EINVAL;
> +	}
> +
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	ret = cros_ec_sensors_core_init(pdev, indio_dev, true,
> +					cros_ec_activity_capture);
> +	if (ret)
> +		return ret;
> +
> +	indio_dev->info = &ec_sensors_info;
> +	st = iio_priv(indio_dev);
> +	st->core.type = st->core.resp->info.type;
> +	st->core.read_ec_sensors_data = cros_ec_sensors_read_cmd;
> +
> +	/*
> +	 * List all available activities

Single line comment.  also, it's kind of obvious given the name
of the command so maybe don't bother with the comment at all?


> +	 */
> +	st->core.param.cmd = MOTIONSENSE_CMD_LIST_ACTIVITIES;
> +	ret = cros_ec_motion_send_host_cmd(&st->core, 0);
> +	if (ret)
> +		return ret;

I'd put a blank line here to separate the error path out
from where we carry on to in the good case.

> +	activities = st->core.resp->list_activities.enabled |
> +		     st->core.resp->list_activities.disabled;
> +	if (!activities)
> +		return -ENODEV;
> +
> +	/* Allocate a channel per activity and one for timestamp */
> +	nb_activities = hweight_long(activities) + 1;
> +	st->channels = devm_kcalloc(dev, nb_activities,
> +				    sizeof(*st->channels), GFP_KERNEL);
> +	if (!st->channels)
> +		return -ENOMEM;
> +
> +	channel = &st->channels[0];
> +	index = 0;
> +	for_each_set_bit(i, &activities, BITS_PER_LONG) {
> +		/* List all available triggers */
> +		if (i == MOTIONSENSE_ACTIVITY_BODY_DETECTION) {
> +			channel->type = IIO_PROXIMITY;
> +			channel->info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
> +			channel->modified = 0;

Not modified is the 'obvious' default so can probably drop that one.

> +			channel->event_spec = cros_ec_body_detect_events;
> +			channel->num_event_specs =
> +				ARRAY_SIZE(cros_ec_body_detect_events);
> +			st->body_detection_channel_index = index;
> +		} else {
> +			channel->type = IIO_ACTIVITY;

This is getting a little creative.  Do we know anything about what the
underlying detection actually is?  Activity sensor are often some form
of magnitude sensor on a specific type of data.  I guess if we have no
idea what this then activity detection makes some sense.

Is it only ever an event?

> +			channel->modified = 1;
> +			channel->event_spec = cros_ec_activity_single_shot;
> +			channel->num_event_specs =
> +				ARRAY_SIZE(cros_ec_activity_single_shot);
> +			if (i == MOTIONSENSE_ACTIVITY_SIG_MOTION) {
> +				channel->channel2 = IIO_MOD_STILL;
> +				st->sig_motion_channel_index = index;
> +			} else {
> +				dev_warn(dev, "Unknown activity: %d\n", i);
> +				continue;
> +			}
> +		}
> +		channel->ext_info = cros_ec_sensors_limited_info;
> +		channel->scan_index = index++;
> +		channel++;
> +	}
> +
> +	/* Timestamp */
> +	channel->scan_index = index;
> +	channel->type = IIO_TIMESTAMP;
> +	channel->channel = -1;
> +	channel->scan_type.sign = 's';
> +	channel->scan_type.realbits = 64;
> +	channel->scan_type.storagebits = 64;
> +
> +	indio_dev->channels = st->channels;
> +	indio_dev->num_channels = index + 1;
> +
> +	return cros_ec_sensors_core_register(dev, indio_dev,
> +					     cros_ec_activity_push_data);
> +}
> +
> +static void cros_ec_sensors_remove(struct platform_device *pdev)
> +{
> +	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> +
> +	iio_device_unregister(indio_dev);

Why?  The cros_ec_sensors_core_register() uses devm internally
to clean this up so I'd not expect to see a remove function at all
for one of these drivers

> +}
> +
> +static struct platform_driver cros_ec_sensors_platform_driver = {
> +	.driver = {
> +		.name	= DRV_NAME,
> +	},
> +	.probe		= cros_ec_sensors_probe,
> +	.remove		= cros_ec_sensors_remove,
> +};
> +module_platform_driver(cros_ec_sensors_platform_driver);
> +
> +MODULE_DESCRIPTION("ChromeOS EC activity sensors driver");
> +MODULE_ALIAS("platform:" DRV_NAME);
> +MODULE_LICENSE("GPL v2");
> diff --git a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c
> index 64bada1e8678..1bd07ca3abfe 100644
> --- a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c
> +++ b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c
> @@ -489,6 +489,16 @@ const struct iio_chan_spec_ext_info cros_ec_sensors_ext_info[] = {
>  };
>  EXPORT_SYMBOL_GPL(cros_ec_sensors_ext_info);
>  
> +const struct iio_chan_spec_ext_info cros_ec_sensors_limited_info[] = {
> +	{
> +		.name = "id",
> +		.shared = IIO_SHARED_BY_ALL,
> +		.read = cros_ec_sensors_id
> +	},
> +	{ },

No comma on terminating entries / sentinels like this.  We don't want
it to be easy to add things after them as that should never happen.

> +};
> +EXPORT_SYMBOL_GPL(cros_ec_sensors_limited_info);
> +
>  /**
>   * cros_ec_sensors_idx_to_reg - convert index into offset in shared memory
>   * @st:		pointer to state information for device

> diff --git a/include/linux/platform_data/cros_ec_commands.h b/include/linux/platform_data/cros_ec_commands.h
> index ecf290a0c98f..0f6d53e7cb97 100644
> --- a/include/linux/platform_data/cros_ec_commands.h
> +++ b/include/linux/platform_data/cros_ec_commands.h


>  struct ec_response_motion_sensor_data {
>  	/* Flags for each sensor. */
>  	uint8_t flags;
> @@ -2454,15 +2465,14 @@ struct ec_response_motion_sensor_data {
>  	uint8_t sensor_num;
>  	/* Each sensor is up to 3-axis. */
>  	union {
> -		int16_t             data[3];
> +		int16_t                                  data[3];

I dislike careful alignment like this because of the noise
it tends to add - this being a classic example. 
I'd just not update the existing field and let one through that isn't
aligned.

>  		struct __ec_todo_packed {
> -			uint16_t    reserved;
> -			uint32_t    timestamp;
> +			uint16_t                         reserved;
> +			uint32_t                         timestamp;
>  		};
>  		struct __ec_todo_unpacked {
> -			uint8_t     activity; /* motionsensor_activity */
> -			uint8_t     state;
> -			int16_t     add_info[2];
> +			struct ec_response_activity_data activity_data;
> +			int16_t                          add_info[2];
>  		};
>  	};
>  } __ec_todo_packed;

>  
>  struct ec_motion_sense_activity {
> @@ -2671,6 +2682,7 @@ struct ec_params_motion_sense {
>  			uint32_t max_data_vector;
>  		} fifo_read;
>  
> +		/* Used for MOTIONSENSE_CMD_SET_ACTIVITY */
>  		struct ec_motion_sense_activity set_activity;
>  
>  		/* Used for MOTIONSENSE_CMD_LID_ANGLE */
> @@ -2716,6 +2728,14 @@ struct ec_params_motion_sense {
>  			 */
>  			int16_t hys_degree;
>  		} tablet_mode_threshold;
> +
> +		/*
> +		 * Used for MOTIONSENSE_CMD_GET_ACTIVITY.

Single line comment works fine here and fits with local style.
		/* Used for MOTIONSENSE_CMD_GET_ACTIVITY */

> +		 */
> +		struct __ec_todo_unpacked {
> +			uint8_t sensor_num;
> +			uint8_t activity;  /* enum motionsensor_activity */
> +		} get_activity;
>  	};
>  } __ec_todo_packed;
>  
> @@ -2833,6 +2853,10 @@ struct ec_response_motion_sense {
>  			uint16_t hys_degree;
>  		} tablet_mode_threshold;
>  
> +		/* USED for MOTIONSENSE_CMD_GET_ACTIVITY. */

Maybe no fullstop is more consistent with local style? Only a bit visible
in this patch and I'm lazy  :)

> +		struct __ec_todo_unpacked {
> +			uint8_t     state;
> +		} get_activity;
>  	};
>  } __ec_todo_packed;
>  

Re: [PATCH v4] iio: cros_ec_sensors: add cros_ec_activity driver

[Jonathan Cameron]

On Fri, 23 May 2025 10:27:27 -0700
Gwendal Grignou <gwendal@chromium.org> wrote:

> ChromeOS EC can report activity information derived from the
> accelerometer:
> - Reports on-body/off-body as a proximity event.
> - Reports significant motion as an activity event.
> 
> This new sensor is a virtual sensor, included only when the EC firmware
> is compiled with the appropriate module.
> 
> Signed-off-by: Gwendal Grignou <gwendal@google.com>
> Reviewed-by: Tzung-Bi Shih <tzungbi@kernel.org>
4 versions in as many days isn't a good way to post to the list.

Please slow it down to give time for more reviews to come in.

Thanks,

Jonathan

Re: [PATCH v4] iio: cros_ec_sensors: add cros_ec_activity driver

[Gwendal Grignou]

Jonathan,

Thanks for the thorough review. I just send a v5, much cleaner driver.

Gwendal.

On Sun, May 25, 2025 at 6:42 AM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Fri, 23 May 2025 10:27:27 -0700
> Gwendal Grignou <gwendal@chromium.org> wrote:
>
> Hi Gwendal,
>
> > ChromeOS EC can report activity information derived from the
> > accelerometer:
> > - Reports on-body/off-body as a proximity event.
>
> How do you do proximity from an accelerometer?
It is a crude proximity detection: The accelerometer is set to ~50Hz
and uses a low pass filter to check for vibration.
It is not vibrating (sitting on a table), then it is in off-body mode.
It is very conservative, erring on the side of on-body: if the table
is in a train, the device may stay in on-body mode. See its
implmentation here
(https://chromium.googlesource.com/chromiumos/platform/ec/+/refs/heads/main/common/body_detection.c).
The sensor can be used to leave the device unlocked (see
https://support.google.com/android/answer/9075927#zippy=%2Ckeep-your-phone-unlocked-when-its-on-you)
or kick on the CPU fans earlier to keep the device cooler than when it
is on a table.

>
> > - Reports significant motion as an activity event.
>
> This one might be from an accelerometer. In which case is there
> any information on the algorithm?
This one looks at a significant motion a few seconds apart. It can
come directly from the accelerometer, see BMI160 datasheet
(https://www.bosch-sensortec.com/media/boschsensortec/downloads/datasheets/bst-bmi160-ds000.pdf)
secion 2.6.2. Or it can be implemented in the firmware of the embedded
controller/sensor hub.

>
> >
> > This new sensor is a virtual sensor, included only when the EC firmware
> > is compiled with the appropriate module.
> >
> > Signed-off-by: Gwendal Grignou <gwendal@google.com>
> > Reviewed-by: Tzung-Bi Shih <tzungbi@kernel.org>
>
>
> Various comments inline.
>
> Biggest question I have is what the significant motion detector
> is based on. Do we know more about that as I'd prefer it associated
> with an actual channel if possible?
These 2 events are indeed currently generated today from the raw
output of a single accelerometer. It does not have to be: we could
further improve the algorithms by fusionning data from the
accelerometer and the gyroscope. As such a virtual sensor is presented
to the kernel by the sensor hub just for the activities.

>
> Also does it only surface as an event?  Is there an underlying
> value to read.
The on-body/off-body algorithm calculates a moving average of the
samples variance and uses an internal threshold to decide the device
state.
The thresholds are model specific (depends on the sensors/device
weight) and a set in the factory.
Same with significant motion; threshold set in the factory based on
experiments decides to trigger an event or not.

>
>
> > diff --git a/drivers/iio/common/cros_ec_sensors/cros_ec_activity.c b/drivers/iio/common/cros_ec_sensors/cros_ec_activity.c
> > new file mode 100644
> > index 000000000000..620203f7aa63
> > --- /dev/null
> > +++ b/drivers/iio/common/cros_ec_sensors/cros_ec_activity.c
> > @@ -0,0 +1,322 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * cros_ec_activity - Driver for activities/gesture recognition.
> > + *
> > + * Copyright 2025 Google, Inc
> > + *
> > + * This driver uses the cros-ec interface to communicate with the ChromeOS
> > + * EC about activity data. Accelerometer access is presented through iio sysfs.
>
> Maybe update as this isn't returning accelerometer data .
Removed.
>
> > + */
>
> > +
> > +static int ec_sensors_read(struct iio_dev *indio_dev,
>
> Given this only applies to this sensor, maybe needs a more specific name?
>
> > +                        struct iio_chan_spec const *chan,
> > +                        int *val, int *val2, long mask)
> > +{
> > +     struct cros_ec_sensors_state *st = iio_priv(indio_dev);
> > +     int ret;
> > +
> > +     mutex_lock(&st->core.cmd_lock);
> Include cleanup.h and use
>
>         guard(mutex)(&st->core.cmd_lock);
> to simpilfy this.
Done. It is indeed much cleaner. I will work on another patch to clean
up the rest of the cros_ec drivers.
>
> > +     if (chan->type == IIO_PROXIMITY && mask == IIO_CHAN_INFO_RAW) {
> > +             st->core.param.cmd = MOTIONSENSE_CMD_GET_ACTIVITY;
> > +             st->core.param.get_activity.activity =
> > +                     MOTIONSENSE_ACTIVITY_BODY_DETECTION;
> > +             ret = cros_ec_motion_send_host_cmd(&st->core, 0);
> > +             if (ret == 0) {
> > +                     *val = st->core.resp->get_activity.state;
> > +                     ret = IIO_VAL_INT;
> With guard magic, can handle error out path and return directly in good path.
>                 if (ret)
>                         return ret;
>
>                 *val = ..
>
>                 return IIO_VAL_INT;
>
> > +             }
> > +     } else {
> > +             ret = -EINVAL;
>
> With guard() magic can just return here.
>
> > +     }
> > +     mutex_unlock(&st->core.cmd_lock);
> > +     return ret;
> > +}
> > +
> > +static int cros_ec_read_event_config(struct iio_dev *indio_dev,
> > +                                  const struct iio_chan_spec *chan,
> > +                                  enum iio_event_type type,
> > +                                  enum iio_event_direction dir)
> > +{
> > +     struct cros_ec_sensors_state *st = iio_priv(indio_dev);
> > +     int ret;
> > +
> > +     if (chan->type != IIO_ACTIVITY && chan->type != IIO_PROXIMITY)
> > +             return -EINVAL;
> > +
> > +     mutex_lock(&st->core.cmd_lock);
>
> Similar to below. guard() will simplify this quite a bit.
>
> > +     st->core.param.cmd = MOTIONSENSE_CMD_LIST_ACTIVITIES;
> > +     ret = cros_ec_motion_send_host_cmd(&st->core, 0);
> > +     if (ret)
> > +             goto done;
>
> Blank line here.
>
> > +     switch (chan->type) {
> > +     case IIO_PROXIMITY:
> > +             ret = !!(st->core.resp->list_activities.enabled &
> > +                      (1 << MOTIONSENSE_ACTIVITY_BODY_DETECTION));
> > +             break;
> > +     case IIO_ACTIVITY:
> > +             if (chan->channel2 == IIO_MOD_STILL) {
> > +                     ret = !!(st->core.resp->list_activities.enabled &
> > +                              (1 << MOTIONSENSE_ACTIVITY_SIG_MOTION));
> > +             } else {
> > +                     dev_warn(&indio_dev->dev, "Unknown activity: %d\n",
> > +                              chan->channel2);
> > +                     ret = -EINVAL;
>
> Flip logic and direct returns etc.
>
> > +             }
> > +             break;
> > +     default:
> > +             dev_warn(&indio_dev->dev, "Unknown channel type: %d\n",
> > +                      chan->type);
> > +             ret = -EINVAL;
> > +     }
> > +done:
> > +     mutex_unlock(&st->core.cmd_lock);
> > +     return ret;
> > +}
> > +
> > +static int cros_ec_write_event_config(struct iio_dev *indio_dev,
> > +                                   const struct iio_chan_spec *chan,
> > +                                   enum iio_event_type type,
> > +                                   enum iio_event_direction dir, int state)
> > +{
> > +     struct cros_ec_sensors_state *st = iio_priv(indio_dev);
> > +     int ret = 0;
> > +
> > +     mutex_lock(&st->core.cmd_lock);
>
> Guard and direct returns. (See below for more on this)
>
> > +     st->core.param.cmd = MOTIONSENSE_CMD_SET_ACTIVITY;
> > +     switch (chan->type) {
> > +     case IIO_PROXIMITY:
> > +             st->core.param.set_activity.activity =
> > +                     MOTIONSENSE_ACTIVITY_BODY_DETECTION;
> > +             break;
> > +     case IIO_ACTIVITY:
> > +             if (chan->channel2 == IIO_MOD_STILL) {
> > +                     st->core.param.set_activity.activity =
> > +                             MOTIONSENSE_ACTIVITY_SIG_MOTION;
> > +             } else {
> flip logic so error is out of line and good path inline.  Then with
> guard you an directly return for error path.
>
> > +                     dev_warn(&indio_dev->dev, "Unknown activity: %d\n",
> > +                              chan->channel2);
> > +                     ret = -EINVAL;
> > +             }
> > +             break;
> > +     default:
> > +             dev_warn(&indio_dev->dev, "Unknown channel type: %d\n",
> > +                      chan->type);
> > +             ret = -EINVAL;
>                 return -EINVAL; when using guard()
> > +     }
> > +     if (ret == 0) {
>
> Then we only get here if ret == 0 so condition can go away.
>
> > +             st->core.param.set_activity.enable = state;
> > +             ret = cros_ec_motion_send_host_cmd(&st->core, 0);
> > +     }
> > +
> > +     mutex_unlock(&st->core.cmd_lock);
> > +     return ret;
> > +}
> > +
> > +static int cros_ec_activity_push_data(struct iio_dev *indio_dev,
> > +                                   s16 *data,
> > +                                   s64 timestamp)
>
> Can reduce wrap and have a parameters share a line.  Nice to stay under 80
> but this is well below that.
Done
>
> > +{
> > +     struct ec_response_activity_data *activity_data =
> > +                     (struct ec_response_activity_data *)data;
> > +     enum motionsensor_activity activity = activity_data->activity;
> > +     u8 state = activity_data->state;
> > +     const struct cros_ec_sensors_state *st = iio_priv(indio_dev);
> > +     const struct iio_chan_spec *chan;
> > +     const struct iio_event_spec *event;
> > +     enum iio_event_direction dir;
> > +     int index;
> > +     u64 ev;
> > +
> > +     switch (activity) {
> > +     case MOTIONSENSE_ACTIVITY_BODY_DETECTION:
> > +             index = st->body_detection_channel_index;
> > +             dir = state ? IIO_EV_DIR_FALLING : IIO_EV_DIR_RISING;
> > +             break;
> > +     case MOTIONSENSE_ACTIVITY_SIG_MOTION:
> > +             index = st->sig_motion_channel_index;
> > +             dir = IIO_EV_DIR_FALLING;
>
> Falling stillness is I guess a detection of activity... As below though I'd
> like to know if we have any more info on what this is actually measuring?
Since the activity name is "IIO_MOD_STILL", when the sensor detects a
significant motion, we are out of still, so the condition is false,
transitioning from true (1) to false (0), thus falling. Note the event
is a one-off, it has to be retriggered, to prevent an avalanche of
significant motion events.
>
> > +             break;
> > +     default:
> > +             dev_warn(&indio_dev->dev, "Unknown activity: %d\n", activity);
> > +             return 0;
> > +     }
> > +     chan = &st->channels[index];
> > +     event = &chan->event_spec[0];
>
> I'd not have so many local variables. Pick some to put inline. Either
> these two or ev.  Just in the interests of shorter code and little
> cost to readability.
Done, removed event.
>
> > +
> > +     ev = IIO_UNMOD_EVENT_CODE(chan->type, index, event->type, dir);
> > +     iio_push_event(indio_dev, ev, timestamp);
>
> Blank line here.
> > +     return 0;
> > +}
> > +
> > +static irqreturn_t cros_ec_activity_capture(int irq, void *p)
> > +{
> > +     struct iio_poll_func *pf = p;
> > +     struct iio_dev *indio_dev = pf->indio_dev;
> > +
> > +     dev_warn(&indio_dev->dev, "%s: Not Expected\n", __func__);
>
> Add a comment on why.
Done.
>
> > +     return IRQ_NONE;
> > +}
>
> > +static int cros_ec_sensors_probe(struct platform_device *pdev)
> > +{
> > +     struct device *dev = &pdev->dev;
> > +     struct cros_ec_device *ec_device = dev_get_drvdata(dev->parent);
> > +     struct iio_dev *indio_dev;
> > +     struct cros_ec_sensors_state *st;
> > +     struct iio_chan_spec *channel;
> > +     unsigned long activities;
> > +     int i, index, ret, nb_activities;
> > +
> > +     if (!ec_device) {
> > +             dev_warn(dev, "No CROS EC device found.\n");
> > +             return -EINVAL;
> > +     }
> > +
> > +     indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
> > +     if (!indio_dev)
> > +             return -ENOMEM;
> > +
> > +     ret = cros_ec_sensors_core_init(pdev, indio_dev, true,
> > +                                     cros_ec_activity_capture);
> > +     if (ret)
> > +             return ret;
> > +
> > +     indio_dev->info = &ec_sensors_info;
> > +     st = iio_priv(indio_dev);
> > +     st->core.type = st->core.resp->info.type;
> > +     st->core.read_ec_sensors_data = cros_ec_sensors_read_cmd;
> > +
> > +     /*
> > +      * List all available activities
>
> Single line comment.  also, it's kind of obvious given the name
> of the command so maybe don't bother with the comment at all?
Removed.
>
>
> > +      */
> > +     st->core.param.cmd = MOTIONSENSE_CMD_LIST_ACTIVITIES;
> > +     ret = cros_ec_motion_send_host_cmd(&st->core, 0);
> > +     if (ret)
> > +             return ret;
>
> I'd put a blank line here to separate the error path out
> from where we carry on to in the good case.
>
> > +     activities = st->core.resp->list_activities.enabled |
> > +                  st->core.resp->list_activities.disabled;
> > +     if (!activities)
> > +             return -ENODEV;
> > +
> > +     /* Allocate a channel per activity and one for timestamp */
> > +     nb_activities = hweight_long(activities) + 1;
> > +     st->channels = devm_kcalloc(dev, nb_activities,
> > +                                 sizeof(*st->channels), GFP_KERNEL);
> > +     if (!st->channels)
> > +             return -ENOMEM;
> > +
> > +     channel = &st->channels[0];
> > +     index = 0;
> > +     for_each_set_bit(i, &activities, BITS_PER_LONG) {
> > +             /* List all available triggers */
> > +             if (i == MOTIONSENSE_ACTIVITY_BODY_DETECTION) {
> > +                     channel->type = IIO_PROXIMITY;
> > +                     channel->info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
> > +                     channel->modified = 0;
>
> Not modified is the 'obvious' default so can probably drop that one.
>
> > +                     channel->event_spec = cros_ec_body_detect_events;
> > +                     channel->num_event_specs =
> > +                             ARRAY_SIZE(cros_ec_body_detect_events);
> > +                     st->body_detection_channel_index = index;
> > +             } else {
> > +                     channel->type = IIO_ACTIVITY;
>
> This is getting a little creative.  Do we know anything about what the
> underlying detection actually is?  Activity sensor are often some form
> of magnitude sensor on a specific type of data.  I guess if we have no
> idea what this then activity detection makes some sense.
As written earlier, the event is triggered when a threshold is
crossed. So contrary to other activity sensors like walking or
swimming, there is no confidence level, it just happened that we are
not still anymore.

For proximity, since the proximity API is used, the output is either
far or near, there are no distances.
>
> Is it only ever an event?
>
> > +                     channel->modified = 1;
> > +                     channel->event_spec = cros_ec_activity_single_shot;
> > +                     channel->num_event_specs =
> > +                             ARRAY_SIZE(cros_ec_activity_single_shot);
> > +                     if (i == MOTIONSENSE_ACTIVITY_SIG_MOTION) {
> > +                             channel->channel2 = IIO_MOD_STILL;
> > +                             st->sig_motion_channel_index = index;
> > +                     } else {
> > +                             dev_warn(dev, "Unknown activity: %d\n", i);
> > +                             continue;
> > +                     }
> > +             }
> > +             channel->ext_info = cros_ec_sensors_limited_info;
> > +             channel->scan_index = index++;
> > +             channel++;
> > +     }
> > +
> > +     /* Timestamp */
> > +     channel->scan_index = index;
> > +     channel->type = IIO_TIMESTAMP;
> > +     channel->channel = -1;
> > +     channel->scan_type.sign = 's';
> > +     channel->scan_type.realbits = 64;
> > +     channel->scan_type.storagebits = 64;
> > +
> > +     indio_dev->channels = st->channels;
> > +     indio_dev->num_channels = index + 1;
> > +
> > +     return cros_ec_sensors_core_register(dev, indio_dev,
> > +                                          cros_ec_activity_push_data);
> > +}
> > +
> > +static void cros_ec_sensors_remove(struct platform_device *pdev)
> > +{
> > +     struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> > +
> > +     iio_device_unregister(indio_dev);
>
> Why?  The cros_ec_sensors_core_register() uses devm internally
> to clean this up so I'd not expect to see a remove function at all
> for one of these drivers
Removed.
>
> > +}
> > +
> > +static struct platform_driver cros_ec_sensors_platform_driver = {
> > +     .driver = {
> > +             .name   = DRV_NAME,
> > +     },
> > +     .probe          = cros_ec_sensors_probe,
> > +     .remove         = cros_ec_sensors_remove,
> > +};
> > +module_platform_driver(cros_ec_sensors_platform_driver);
> > +
> > +MODULE_DESCRIPTION("ChromeOS EC activity sensors driver");
> > +MODULE_ALIAS("platform:" DRV_NAME);
> > +MODULE_LICENSE("GPL v2");
> > diff --git a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c
> > index 64bada1e8678..1bd07ca3abfe 100644
> > --- a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c
> > +++ b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c
> > @@ -489,6 +489,16 @@ const struct iio_chan_spec_ext_info cros_ec_sensors_ext_info[] = {
> >  };
> >  EXPORT_SYMBOL_GPL(cros_ec_sensors_ext_info);
> >
> > +const struct iio_chan_spec_ext_info cros_ec_sensors_limcros_ec_sensors_limited_info
ited_info[] = {
> > +     {
> > +             .name = "id",
> > +             .shared = IIO_SHARED_BY_ALL,
> > +             .read = cros_ec_sensors_id
> > +     },
> > +     { },
>
> No comma on terminating entries / sentinels like this.  We don't want
> it to be easy to add things after them as that should never happen.
Done for `cros_ec_sensors_ext_info` as well.
>
> > +};
> > +EXPORT_SYMBOL_GPL(cros_ec_sensors_limited_info);
> > +
> >  /**
> >   * cros_ec_sensors_idx_to_reg - convert index into offset in shared memory
> >   * @st:              pointer to state information for device
>
> > diff --git a/include/linux/platform_data/cros_ec_commands.h b/include/linux/platform_data/cros_ec_commands.h
> > index ecf290a0c98f..0f6d53e7cb97 100644
> > --- a/include/linux/platform_data/cros_ec_commands.h
> > +++ b/include/linux/platform_data/cros_ec_commands.h
>
>
> >  struct ec_response_motion_sensor_data {
> >       /* Flags for each sensor. */
> >       uint8_t flags;Each sensor is up to 3-axis.
> > @@ -2454,15 +2465,14 @@ struct ec_response_motion_sensor_data {
> >       uint8_t sensor_num;
> >       /* Each sensor is up to 3-axis. */
> >       union {
> > -             int16_t             data[3];
> > +             int16_t                                  data[3];
>
> I dislike careful alignment like this because of the noise
> it tends to add - this being a classic example.
> I'd just not update the existing field and let one through that isn't
> aligned.
Done.
>
> >               struct __ec_todo_packed {
> > -                     uint16_t    reserved;
> > -                     uint32_t    timestamp;
> > +                     uint16_t                         reserved;
> > +                     uint32_t                         timestamp;
> >               };
> >               struct __ec_todo_unpacked {
> > -                     uint8_t     activity; /* motionsensor_activity */
> > -                     uint8_t     state;
> > -                     int16_t     add_info[2];
> > +                     struct ec_response_activity_data activity_data;
> > +                     int16_t                          add_info[2];
> >               };
> >       };
> >  } __ec_todo_packed;
>
> >
> >  struct ec_motion_sense_activity {
> > @@ -2671,6 +2682,7 @@ struct ec_params_motion_sense {
> >                       uint32_t max_data_vector;
> >               } fifo_read;
> >
> > +             /* Used for MOTIONSENSE_CMD_SET_ACTIVITY */
> >               struct ec_motion_sense_activity set_activity;
> >
> >               /* Used for MOTIONSENSE_CMD_LID_ANGLE */
> > @@ -2716,6 +2728,14 @@ struct ec_params_motion_sense {
> >                        */
> >                       int16_t hys_degree;
> >               } tablet_mode_threshold;
> > +
> > +             /*
> > +              * Used for MOTIONSENSE_CMD_GET_ACTIVITY.
>
> Single line comment works fine here and fits with local style.
>                 /* Used for MOTIONSENSE_CMD_GET_ACTIVITY */
>
> > +              */
> > +             struct __ec_todo_unpacked {
> > +                     uint8_t sensor_num;
> > +                     uint8_t activity;  /* enum motionsensor_activity */
> > +             } get_activity;
> >       };
> >  } __ec_todo_packed;
> >
> > @@ -2833,6 +2853,10 @@ struct ec_response_motion_sense {
> >                       uint16_t hys_degree;
> >               } tablet_mode_threshold;
> >
> > +             /* USED for MOTIONSENSE_CMD_GET_ACTIVITY. */
>
> Maybe no fullstop is more consistent with local style? Only a bit visible
> in this patch and I'm lazy  :)
It is a mix and match in this file. Full stop is supposed to be the
norm, but it was not always been the case. Remove full stop to blend
with other commands.

>
> > +             struct __ec_todo_unpacked {
> > +                     uint8_t     state;
> > +             } get_activity;
> >       };
> >  } __ec_todo_packed;
> >
>

Re: [PATCH v4] iio: cros_ec_sensors: add cros_ec_activity driver

[Tzung-Bi Shih]

On Tue, Jun 03, 2025 at 10:41:25PM -0700, Gwendal Grignou wrote:
> > > diff --git a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c
> [...]
> > > @@ -2716,6 +2728,14 @@ struct ec_params_motion_sense {
> > >                        */
> > >                       int16_t hys_degree;
> > >               } tablet_mode_threshold;
> > > +
> > > +             /*
> > > +              * Used for MOTIONSENSE_CMD_GET_ACTIVITY.
> >
> > Single line comment works fine here and fits with local style.
> >                 /* Used for MOTIONSENSE_CMD_GET_ACTIVITY */
> >
> > > +              */
> > > +             struct __ec_todo_unpacked {
> > > +                     uint8_t sensor_num;
> > > +                     uint8_t activity;  /* enum motionsensor_activity */
> > > +             } get_activity;
> > >       };
> > >  } __ec_todo_packed;
> > >
> > > @@ -2833,6 +2853,10 @@ struct ec_response_motion_sense {
> > >                       uint16_t hys_degree;
> > >               } tablet_mode_threshold;
> > >
> > > +             /* USED for MOTIONSENSE_CMD_GET_ACTIVITY. */
> >
> > Maybe no fullstop is more consistent with local style? Only a bit visible
> > in this patch and I'm lazy  :)
> It is a mix and match in this file. Full stop is supposed to be the
> norm, but it was not always been the case. Remove full stop to blend
> with other commands.

It seems the full stop is still in v5.

Just a side note: people manually synchronizes the header with [1] when
needed.  I wouldn't bother with the details in the header as long as the
struct content is the same with [1].  Fix it or leave it as is are both
fine for me.

[1]: https://chromium.googlesource.com/chromiumos/platform/ec/+/refs/heads/main/include/ec_commands.h

Re: [PATCH v4] iio: cros_ec_sensors: add cros_ec_activity driver

[Jonathan Cameron]

> > > +     for_each_set_bit(i, &activities, BITS_PER_LONG) {
> > > +             /* List all available triggers */
> > > +             if (i == MOTIONSENSE_ACTIVITY_BODY_DETECTION) {
> > > +                     channel->type = IIO_PROXIMITY;
> > > +                     channel->info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
> > > +                     channel->modified = 0;  
> >
> > Not modified is the 'obvious' default so can probably drop that one.
> >  
> > > +                     channel->event_spec = cros_ec_body_detect_events;
> > > +                     channel->num_event_specs =
> > > +                             ARRAY_SIZE(cros_ec_body_detect_events);
> > > +                     st->body_detection_channel_index = index;
> > > +             } else {
> > > +                     channel->type = IIO_ACTIVITY;  
> >
> > This is getting a little creative.  Do we know anything about what the
> > underlying detection actually is?  Activity sensor are often some form
> > of magnitude sensor on a specific type of data.  I guess if we have no
> > idea what this then activity detection makes some sense.  
> As written earlier, the event is triggered when a threshold is
> crossed. So contrary to other activity sensors like walking or
> swimming, there is no confidence level, it just happened that we are
> not still anymore.

The confidence level is a bit of a fiction for all sensors. I just wanted
the ABI to allow for algorithms getting more clever over time.  Right now
they all claim to be 0 or 100.

Anyhow, I don't think that matters anyway.