Re: [PATCH 1/2] iio: common: cros_ec_sensors: determine protocol version

[Gwendal Grignou]

On Thu, Jun 27, 2019 at 8:59 AM Enric Balletbo i Serra
<enric.balletbo@xxxxxxxxxxxxx> wrote:
>
> Hi,
>
> cc'ing Doug, Gwendal and Enrico that might be interested to give a review.
>
> This patch can be picked alone without 2/2, an is needed to have cros-ec-sensors
> legacy support on ARM (see [1] and [2])
>
> Jonathan, as [1] and [2] will go through the chrome-platform tree if you don't
> mind I'd also like to carry with this patch once you're fine with it.
>
> Thanks,
> ~ Enric
>
> [1] https://patchwork.kernel.org/patch/11014329/
> [2] https://patchwork.kernel.org/patch/11014327/
>
> On 27/6/19 16:04, Fabien Lahoudere wrote:
> > This patch adds a function to determine which version of the
> > protocol is used to communicate with EC.
> >
> > Signed-off-by: Fabien Lahoudere <fabien.lahoudere@xxxxxxxxxxxxx>
> > Signed-off-by: Nick Vaccaro <nvaccaro@xxxxxxxxxxxx>
>
> Tested-by: Enric Balletbo i Serra <enric.balletbo@xxxxxxxxxxxxx>
>
> > ---
> >  .../cros_ec_sensors/cros_ec_sensors_core.c    | 36 ++++++++++++++++++-
> >  1 file changed, 35 insertions(+), 1 deletion(-)
> >
> > 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
> > index 130362ca421b..2e0f97448e64 100644
> > --- a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c
> > +++ b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c
> > @@ -25,6 +25,31 @@ static char *cros_ec_loc[] = {
> >       [MOTIONSENSE_LOC_MAX] = "unknown",
> >  };
> >
> > +static int cros_ec_get_host_cmd_version_mask(struct cros_ec_device *ec_dev,
> > +                                          u16 cmd_offset, u16 cmd, u32 *mask)
> > +{
> > +     int ret;
> > +     struct {
> > +             struct cros_ec_command msg;
> > +             union {
> > +                     struct ec_params_get_cmd_versions params;
> > +                     struct ec_response_get_cmd_versions resp;
> > +             };
> > +     } __packed buf = {
> > +             .msg = {
add
.version = 0,
As the variable is coming from the stack, the version should be set.
> > +                     .command = EC_CMD_GET_CMD_VERSIONS + cmd_offset,
> > +                     .insize = sizeof(struct ec_response_get_cmd_versions),
> > +                     .outsize = sizeof(struct ec_params_get_cmd_versions)
> > +                     },
> > +             .params = {.cmd = cmd}
> > +     };
> > +
> > +     ret = cros_ec_cmd_xfer_status(ec_dev, &buf.msg);
> > +     if (ret >= 0)
It should be > 0: when the command is a success, it returns the number
of byte in the response buffer. When don't expect == 0  here, because
when successful, EC_CMD_GET_CMD_VERSIONS will return a mask.
> > +             *mask = buf.resp.version_mask;
> > +     return ret;
> > +}
> > +
> >  int cros_ec_sensors_core_init(struct platform_device *pdev,
> >                             struct iio_dev *indio_dev,
> >                             bool physical_device)
> > @@ -33,6 +58,8 @@ int cros_ec_sensors_core_init(struct platform_device *pdev,
> >       struct cros_ec_sensors_core_state *state = iio_priv(indio_dev);
> >       struct cros_ec_dev *ec = dev_get_drvdata(pdev->dev.parent);
> >       struct cros_ec_sensor_platform *sensor_platform = dev_get_platdata(dev);
> > +     u32 ver_mask;
> > +     int ret;
> >
> >       platform_set_drvdata(pdev, indio_dev);
> >
> > @@ -47,8 +74,15 @@ int cros_ec_sensors_core_init(struct platform_device *pdev,
> >
> >       mutex_init(&state->cmd_lock);
> >
> > +     ret = cros_ec_get_host_cmd_version_mask(state->ec,
> > +                                             ec->cmd_offset,
> > +                                             EC_CMD_MOTION_SENSE_CMD,
> > +                                             &ver_mask);
> > +     if (ret < 0)
Use:
if (ret <= 0 || ver_mask == 0) {
In case the EC is really old or misbehaving, we don't want to set an
invalid version later.
> > +             return ret;
> > +
> >       /* Set up the host command structure. */
> > -     state->msg->version = 2;
> > +     state->msg->version = fls(ver_mask) - 1;;
> >       state->msg->command = EC_CMD_MOTION_SENSE_CMD + ec->cmd_offset;
> >       state->msg->outsize = sizeof(struct ec_params_motion_sense);
> >
> >