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 :-) 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); >>>> >>>> >
