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

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

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