| Message ID | f5e824de2c423b310c4981b723bba4945a6512d4.1561731659.git.fabien.lahoudere@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | iio: common: cros_ec_sensors: Add protocol v3 support | expand |
Hi, Thanks for the quick respin, two few comments below On 28/6/19 16:41, 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@collabora.com> > Signed-off-by: Nick Vaccaro <nvaccaro@chromium.org> The order must be the opposite, first Nick and then you. > --- > .../cros_ec_sensors/cros_ec_sensors_core.c | 40 ++++++++++++++++++- > 1 file changed, 39 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..75d9b617f6c8 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,35 @@ 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 = { > + .version = 0, > + .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) > + return ret; > + > + *mask = buf.resp.version_mask; > + > + return 0; > +} > + > int cros_ec_sensors_core_init(struct platform_device *pdev, > struct iio_dev *indio_dev, > bool physical_device) > @@ -33,6 +62,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; If you follow the error path in the cros_ec_get_host_cmd_version_mask there is a possible use of an uninitialized variable. u32 ver_mask = 0; > + int ret; > > platform_set_drvdata(pdev, indio_dev); > > @@ -47,8 +78,15 @@ int cros_ec_sensors_core_init(struct platform_device *pdev, > > mutex_init(&state->cmd_lock); > What about adding a comment to explain the ver_mask thing for the record? /* * If the EC is very old or misbehaving is it possible that the * communication succeed and have the version mask set to an invalid * value. So check that version mask is valid (!= 0), otherwise return * an error. */ At least this is what I understood from the previous discussion. > + ret = cros_ec_get_host_cmd_version_mask(state->ec, > + ec->cmd_offset, > + EC_CMD_MOTION_SENSE_CMD, > + &ver_mask); > + if (ret < 0 || ver_mask == 0) > + return -ENODEV; > + > /* 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); > > Thanks, ~ Enric
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..75d9b617f6c8 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,35 @@ 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 = { + .version = 0, + .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) + return ret; + + *mask = buf.resp.version_mask; + + return 0; +} + int cros_ec_sensors_core_init(struct platform_device *pdev, struct iio_dev *indio_dev, bool physical_device) @@ -33,6 +62,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 +78,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 || ver_mask == 0) + return -ENODEV; + /* 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);