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

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

On Wed, May 21, 2025 at 04:51:08PM -0700, Gwendal Grignou wrote:
> diff --git a/drivers/iio/common/cros_ec_sensors/cros_ec_activity.c b/drivers/iio/common/cros_ec_sensors/cros_ec_activity.c
> [...]
> +/*
> + * cros_ec_sensors_activity - Driver for activities/gesture recognition.

s/sensors//.

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

If either activity or channel type is unknown, does it still need to send
the host command for writing the config?

> +static int cros_ec_sensors_probe(struct platform_device *pdev)
> [...]
> +	/*
> +	 * 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;

See review comments for v1 too.  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(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++;

This doesn't look correct.  It generates some unused `index` if the activity
is unknown.

Moving the statement to the end of loop?  See review comments for v1 too.