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

[Gwendal Grignou]

On Fri, Jun 28, 2019 at 6:46 AM Enric Balletbo i Serra
<enric.balletbo@xxxxxxxxxxxxx> wrote:
>
> Hi Fabien, Gwendal,
>
> On 28/6/19 13:37, Fabien Lahoudere wrote:
> > Le jeudi 27 juin 2019 Ã 14:59 -0700, Gwendal Grignou a Ãcrit :
> >> 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.
> >
> > To not return a positive value on error if ret >= 0 and ver_mask = 0
> > I would prefer this:
> >
> >       if (ret <= 0)
> >               return ret;
> >
> >       if (ver_mask == 0)
> >               return -EIO;
> >
> > Let me know if I am wrong
> >
>
> Ok, after discussing with Fabien I think I understood all this and I was
> confused. So the thing is that some very old EC sets the version_mask to 0 and
> the communication succeeds. I think all this deserves a comment in the code for
> dummies like me :-)
Looking into the code, when the command is successful, the EC would
return 4 and a valid ver_mask, otherwise ret would -EPROTO (EC answers
INVALID_PARAM).

So the original code will work as is. Thanks for your help about C
[partial] Initialization.

Reviewed-by: Gwendal Grignou <gwendal@xxxxxxxxxxxx>
Tested-by: Gwendal Grignou <gwendal@xxxxxxxxxxxx>

>
> Thanks,
> ~ Enric
>
> >>>> +
>  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);
> >>>>
> >>>>
> >