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

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

On Tue, Jun 03, 2025 at 10:39:03PM -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.
> 
> 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 v5:
> - Use guard(mutex), simplify error path.
> - Use driver name as function prefix.
> - Remove cros_ec_sensors_remove function.
> - Remove identation of structure field.
> - Invert raw value to return 1m when far, 0m when close.

As v5 changed a bit from v4, it'd be better to drop my R-b tag in v5.

Anyway, it overall looks good to me.  Only a few minor comments:
Reviewed-by: Tzung-Bi Shih <tzungbi@kernel.org>

> +static int cros_ec_activity_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;
> +
> +	guard(mutex)(&st->core.cmd_lock);
> +	st->core.param.cmd = MOTIONSENSE_CMD_LIST_ACTIVITIES;
> +	ret = cros_ec_motion_send_host_cmd(&st->core, 0);
> +	if (ret)
> +		return ret;
> +
> +	switch (chan->type) {
> +	case IIO_PROXIMITY:
> +		return !!(st->core.resp->list_activities.enabled &
> +			 (1 << MOTIONSENSE_ACTIVITY_BODY_DETECTION));
> +	case IIO_ACTIVITY:
> +		if (chan->channel2 == IIO_MOD_STILL) {
> +			return !!(st->core.resp->list_activities.enabled &
> +				 (1 << MOTIONSENSE_ACTIVITY_SIG_MOTION));
> +		} else {
> +			dev_warn(&indio_dev->dev, "Unknown activity: %d\n",
> +				 chan->channel2);
> +			return -EINVAL;
> +		}

By referencing to cros_ec_activity_write_event_config(), maybe consider to
drop the " else {" and "}".

> +static int cros_ec_activity_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);
> +

By referencing to cros_ec_activity_read_event_config(), maybe consider to
exit the function earlier:
	if (chan->type != IIO_ACTIVITY && chan->type != IIO_PROXIMITY)