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