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

[Tzung-Bi Shih <tzungbi@kernel.org>]

On Mon, May 19, 2025 at 09:01:00PM -0700, Gwendal Grignou 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.

s/^r/R/.

> diff --git a/drivers/iio/common/cros_ec_sensors/Kconfig b/drivers/iio/common/cros_ec_sensors/Kconfig
> [...]
> +config IIO_CROS_EC_ACTIVITY
> +	tristate "ChromeOS EC Activity Sensors"
> +	depends on IIO_CROS_EC_SENSORS_CORE
> +	help
> +	  Module to handle activity events detections presented by the ChromeOS
                           ^^^^^^^^^^^^^^^^^^^^^^^^^^
I'm confused and wondering if there is a typo.  Which one:
- activity event detections
- activity events
- activity detections

> +	  EC Sensor hub.

s/S/s/?

> +	  Activities can be a proximity detector (on body/off body detection)
> +	  or an activity to trigger an event on significant motion.

s/an activity to trigger //?

> diff --git a/drivers/iio/common/cros_ec_sensors/cros_ec_activity.c b/drivers/iio/common/cros_ec_sensors/cros_ec_activity.c
> [...]
> +// SPDX-License-Identifier: GPL-2.0
> +//

Drop the empty comment line.

> +/*
> + * cros_ec_sensors_activity - Driver for activities/gesture recognition.
> + *
> + * Copyright 2025 Google, Inc
> + *
> + * This driver uses the cros-ec interface to communicate with the Chrome OS

s/Chrome OS/ChromeOS/.

> +#include <linux/delay.h>

Drop it.  The driver doesn't use anything from the header.

> +#include <linux/device.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>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/platform_data/cros_ec_commands.h>
> +#include <linux/platform_data/cros_ec_proto.h>
> +#include <linux/platform_device.h>

I guess the list may be copied from somewhere drivers.  Could you review
again to make sure the driver needs to include them?

> +#include <linux/slab.h>

Drop it.  The driver doesn't use anything from the header.

> +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),
> +	 },

Remove the extra space before "}".

> +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),
> +	 },

Remove the extra space before "}".

> +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;
> +		if (cros_ec_motion_send_host_cmd(&st->core, 0) !=
> +				EC_RES_SUCCESS) {

cros_ec_motion_send_host_cmd() returns 0 or negative errno like most other
kernel APIs.  Even though EC_RES_SUCCESS is also 0, however, they aren't
the same domain space (see also cros_ec_map_error()).  Don't use it.

		if (cros_ec_motion_send_host_cmd(&st->core, 0))

> +static int cros_ec_read_event_config(struct iio_dev *indio_dev,
> [...]
> +	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;

Curious about: wouldn't compiler or some checkers emit warnings about it
lacks a `break`?

> +static int cros_ec_write_event_config(struct iio_dev *indio_dev,
> [...]
> +	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);
> +		}
> +		break;
> +	default:
> +		dev_warn(&indio_dev->dev, "Unknown channel type: %d\n",
> +			 chan->type);

Same here.

> +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(&pdev->dev, "No CROS EC device found.\n");

The function already has a local variable `dev` for `&pdev->dev`.  Use it.

> +		return -EINVAL;
> +	}
> +
> +	indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*st));

`dev`.

> +	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;
> +
> +	/*
> +	 * 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;
> +	nb_activities = hweight_long(activities) + 1;

Moving the line just right before the following devm_kcalloc() for
`st->channels` makes more sense.

> +
> +	if (!activities)
> +		return -ENODEV;
> +
> +	/* Allocate a channel per activity and one for timestamp */
> +	st->channels = devm_kcalloc(&pdev->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) {
> +		channel->scan_index = index;

It seems the loop body doesn't use the `channel->scan_index`.  Move the
assignment to the end of loop body when it's sure to occupy the index?

		channel->scan_index = index++;

> +
> +		/* List all available activities */

Drop the comment as it doesn't reflect what the following code does?

> +		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(&pdev->dev,

`dev`.

> +					 "Unknown activity: %d\n", i);
> +				continue;
> +			}
> +		}
> +		channel->ext_info = cros_ec_sensors_limited_info;
> +		channel++;
> +		index++;
> +	}
> +
> +	/* 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;
> +
> +	st->core.read_ec_sensors_data = cros_ec_sensors_read_cmd;

The statement here isn't intuitive.  Can it move somewhere above when setting
the `st->core`?  E.g. right after the assignment for `st->core.type`.