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

[Jonathan Cameron <jic23@kernel.org>]

On Thu, 5 Jun 2025 06:28:59 +0000
Tzung-Bi Shih <tzungbi@kernel.org> wrote:

> 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 "}".

Agreed. I tweaked this.

> 
> > +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)
In this case we aren't really doing anything before the switch statement, unlike
read where we read the data from the device.

So I left this one alone.

Needed another couple of tweaks.  Signature of this function now takes
a bool state and the { }, fix was already present upstream.
Also some mess in the Makefile due to tracing patch that it seems
you don't have on your tree.

Please check the result. Pushed out for now as testing but I'll rebase
on rc1 sometime soon.

Jonathan