* [PATCH 0/2] Add protocol v3 support @ 2019-06-27 14:04 Fabien Lahoudere 2019-06-27 14:04 ` [PATCH 1/2] iio: common: cros_ec_sensors: determine protocol version Fabien Lahoudere 2019-06-27 14:04 ` [PATCH 2/2] iio: common: cros_ec_sensors: set default frequencies Fabien Lahoudere 0 siblings, 2 replies; 12+ messages in thread From: Fabien Lahoudere @ 2019-06-27 14:04 UTC (permalink / raw) Cc: kernel, Fabien Lahoudere, Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler, linux-iio, linux-kernel This series is a split of the following patch: https://lkml.org/lkml/2019/6/18/268 To fix Enric comments from https://lkml.org/lkml/2019/6/25/949 I extract it from the other serie to speed up acceptance because other patches need it to be upstreamed. Fabien Lahoudere (2): iio: common: cros_ec_sensors: determine protocol version iio: common: cros_ec_sensors: set default frequencies .../cros_ec_sensors/cros_ec_sensors_core.c | 80 ++++++++++++++++++- .../linux/iio/common/cros_ec_sensors_core.h | 3 + 2 files changed, 82 insertions(+), 1 deletion(-) -- 2.20.1 ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 1/2] iio: common: cros_ec_sensors: determine protocol version 2019-06-27 14:04 [PATCH 0/2] Add protocol v3 support Fabien Lahoudere @ 2019-06-27 14:04 ` Fabien Lahoudere 2019-06-27 15:59 ` Enric Balletbo i Serra 2019-06-27 14:04 ` [PATCH 2/2] iio: common: cros_ec_sensors: set default frequencies Fabien Lahoudere 1 sibling, 1 reply; 12+ messages in thread From: Fabien Lahoudere @ 2019-06-27 14:04 UTC (permalink / raw) Cc: kernel, Fabien Lahoudere, Nick Vaccaro, Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler, linux-iio, linux-kernel 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> --- .../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 = { + .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) + *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) + 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); -- 2.20.1 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] iio: common: cros_ec_sensors: determine protocol version 2019-06-27 14:04 ` [PATCH 1/2] iio: common: cros_ec_sensors: determine protocol version Fabien Lahoudere @ 2019-06-27 15:59 ` Enric Balletbo i Serra 2019-06-27 21:59 ` Gwendal Grignou 0 siblings, 1 reply; 12+ messages in thread From: Enric Balletbo i Serra @ 2019-06-27 15:59 UTC (permalink / raw) To: Fabien Lahoudere Cc: kernel, Nick Vaccaro, Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler, linux-iio, linux-kernel, Doug Anderson, Gwendal Grignou, Enrico Granata 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@collabora.com> > Signed-off-by: Nick Vaccaro <nvaccaro@chromium.org> Tested-by: Enric Balletbo i Serra <enric.balletbo@collabora.com> > --- > .../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 = { > + .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) > + *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) > + 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); > > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] iio: common: cros_ec_sensors: determine protocol version 2019-06-27 15:59 ` Enric Balletbo i Serra @ 2019-06-27 21:59 ` Gwendal Grignou 2019-06-28 9:36 ` Fabien Lahoudere 2019-06-28 11:37 ` Fabien Lahoudere 0 siblings, 2 replies; 12+ messages in thread From: Gwendal Grignou @ 2019-06-27 21:59 UTC (permalink / raw) To: Enric Balletbo i Serra Cc: Fabien Lahoudere, kernel, Nick Vaccaro, Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler, linux-iio, linux-kernel, Doug Anderson, Enrico Granata On Thu, Jun 27, 2019 at 8:59 AM Enric Balletbo i Serra <enric.balletbo@collabora.com> 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@collabora.com> > > Signed-off-by: Nick Vaccaro <nvaccaro@chromium.org> > > Tested-by: Enric Balletbo i Serra <enric.balletbo@collabora.com> > > > --- > > .../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); > > > > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] iio: common: cros_ec_sensors: determine protocol version 2019-06-27 21:59 ` Gwendal Grignou @ 2019-06-28 9:36 ` Fabien Lahoudere 2019-06-28 10:32 ` Enric Balletbo i Serra 2019-06-28 11:37 ` Fabien Lahoudere 1 sibling, 1 reply; 12+ messages in thread From: Fabien Lahoudere @ 2019-06-28 9:36 UTC (permalink / raw) To: Gwendal Grignou, Enric Balletbo i Serra Cc: kernel, Nick Vaccaro, Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler, linux-iio, linux-kernel, Doug Anderson, Enrico Granata Thanks Gwendal for reviewing. 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@collabora.com> 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@collabora.com> > > > Signed-off-by: Nick Vaccaro <nvaccaro@chromium.org> > > > > Tested-by: Enric Balletbo i Serra <enric.balletbo@collabora.com> > > > > > --- > > > .../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. Ack > > > + .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. Ack, so we assume that on success, 0 is not possible. > > > + *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. Ack, indeed the communication can work but with invalid data. > > > + 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); > > > > > > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] iio: common: cros_ec_sensors: determine protocol version 2019-06-28 9:36 ` Fabien Lahoudere @ 2019-06-28 10:32 ` Enric Balletbo i Serra 0 siblings, 0 replies; 12+ messages in thread From: Enric Balletbo i Serra @ 2019-06-28 10:32 UTC (permalink / raw) To: Fabien Lahoudere, Gwendal Grignou Cc: kernel, Nick Vaccaro, Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler, linux-iio, linux-kernel, Doug Anderson, Enrico Granata Hi Fabien, Gwendal On 28/6/19 11:36, Fabien Lahoudere wrote: > Thanks Gwendal for reviewing. > > 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@collabora.com> 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@collabora.com> >>>> Signed-off-by: Nick Vaccaro <nvaccaro@chromium.org> >>> >>> Tested-by: Enric Balletbo i Serra <enric.balletbo@collabora.com> >>> >>>> --- >>>> .../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. > I think that from the C standard when using struct partial initialization in c it follows the same rules as static so shouldn't be really needed. Anyway this is always confusing me is for that I tend to use buf = { }; or memset the struct and then assign the required values so it's clear that all the memebers of the struct are initialized. > Ack > >>>> + .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. > Gwendal, from the downstream commit I see: ret = cros_ec_cmd_xfer_status(ec_dev, msg); if (ret >= 0) { if (msg->result == EC_RES_SUCCESS) *mask = resp->version_mask; else *mask = 0; } return ret; I think we're confusing cros_ec_cmd_xfer_status() vs cros_ec_cmd_xfer()? cros_ec_cmd_xfer_status() will return _only_ a value >= 0 value _if result is EC_RES_SUCCESS_ otherwise will return a negative value (see cros_ec_cmd_xfer_status() implementation). So the second check is not really needed and the same can be implemented as: ret = cros_ec_cmd_xfer_status(ec_dev, msg); if (ret < 0) *mask = 0; else *mask = resp->version_mask; return ret; But then I don't see the point to set the mask to 0 and even can be simplified as: ret = cros_ec_cmd_xfer_status(ec_dev, msg); if (ret < 0) return ret; *mask = msg.resp.version_mask; return 0; So cros_ec_get_host_cmd_version_mask() returns 0 on success or negative value on error (protocol error || result != EC_RES_SUCCESS) > Ack, so we assume that on success, 0 is not possible. > >>>> + *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. > Ack, indeed the communication can work but with invalid data. From the downstream commit: if (ret < 0 || ver_mask == 0) { dev_warn(dev, "Motionsense cmd version too old, aborting...\n"); return -ENODEV; } But with the above implementation is the same as if (ret < 0) { dev_warn(dev, "Motionsense cmd version too old, aborting...\n"); return -ENODEV; } Or I'm missing something and what we really want is to cover a corner case? I such case I think we should use cros_ec_cmd_xfer() instead of cros_ec_cmd_xfer_status() 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); >>>> >>>> > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] iio: common: cros_ec_sensors: determine protocol version 2019-06-27 21:59 ` Gwendal Grignou 2019-06-28 9:36 ` Fabien Lahoudere @ 2019-06-28 11:37 ` Fabien Lahoudere 2019-06-28 13:46 ` Enric Balletbo i Serra 1 sibling, 1 reply; 12+ messages in thread From: Fabien Lahoudere @ 2019-06-28 11:37 UTC (permalink / raw) To: Gwendal Grignou, Enric Balletbo i Serra Cc: kernel, Nick Vaccaro, Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler, linux-iio, linux-kernel, Doug Anderson, Enrico Granata 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@collabora.com> 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@collabora.com> > > > Signed-off-by: Nick Vaccaro <nvaccaro@chromium.org> > > > > Tested-by: Enric Balletbo i Serra <enric.balletbo@collabora.com> > > > > > --- > > > .../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 > > > + 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); > > > > > > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] iio: common: cros_ec_sensors: determine protocol version 2019-06-28 11:37 ` Fabien Lahoudere @ 2019-06-28 13:46 ` Enric Balletbo i Serra 2019-06-28 17:01 ` Gwendal Grignou 0 siblings, 1 reply; 12+ messages in thread From: Enric Balletbo i Serra @ 2019-06-28 13:46 UTC (permalink / raw) To: Fabien Lahoudere, Gwendal Grignou Cc: kernel, Nick Vaccaro, Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler, linux-iio, linux-kernel, Doug Anderson, Enrico Granata 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@collabora.com> 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@collabora.com> >>>> Signed-off-by: Nick Vaccaro <nvaccaro@chromium.org> >>> >>> Tested-by: Enric Balletbo i Serra <enric.balletbo@collabora.com> >>> >>>> --- >>>> .../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); >>>> >>>> > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] iio: common: cros_ec_sensors: determine protocol version 2019-06-28 13:46 ` Enric Balletbo i Serra @ 2019-06-28 17:01 ` Gwendal Grignou 0 siblings, 0 replies; 12+ messages in thread From: Gwendal Grignou @ 2019-06-28 17:01 UTC (permalink / raw) To: Enric Balletbo i Serra Cc: Fabien Lahoudere, kernel, Nick Vaccaro, Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler, linux-iio, linux-kernel, Doug Anderson, Enrico Granata On Fri, Jun 28, 2019 at 6:46 AM Enric Balletbo i Serra <enric.balletbo@collabora.com> 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@collabora.com> 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@collabora.com> > >>>> Signed-off-by: Nick Vaccaro <nvaccaro@chromium.org> > >>> > >>> Tested-by: Enric Balletbo i Serra <enric.balletbo@collabora.com> > >>> > >>>> --- > >>>> .../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@chromium.org> Tested-by: Gwendal Grignou <gwendal@chromium.org> > > 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); > >>>> > >>>> > > ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 2/2] iio: common: cros_ec_sensors: set default frequencies 2019-06-27 14:04 [PATCH 0/2] Add protocol v3 support Fabien Lahoudere 2019-06-27 14:04 ` [PATCH 1/2] iio: common: cros_ec_sensors: determine protocol version Fabien Lahoudere @ 2019-06-27 14:04 ` Fabien Lahoudere 2019-06-27 15:48 ` Enric Balletbo i Serra 1 sibling, 1 reply; 12+ messages in thread From: Fabien Lahoudere @ 2019-06-27 14:04 UTC (permalink / raw) Cc: kernel, Fabien Lahoudere, Nick Vaccaro, Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler, linux-iio, linux-kernel Version 3 of the EC protocol provides min and max frequencies for EC sensors. Default frequencies are provided for earlier protocol. Signed-off-by: Fabien Lahoudere <fabien.lahoudere@collabora.com> Signed-off-by: Nick Vaccaro <nvaccaro@chromium.org> --- .../cros_ec_sensors/cros_ec_sensors_core.c | 44 +++++++++++++++++++ .../linux/iio/common/cros_ec_sensors_core.h | 3 ++ 2 files changed, 47 insertions(+) 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 2e0f97448e64..72f56d54cccd 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 @@ -50,6 +50,37 @@ static int cros_ec_get_host_cmd_version_mask(struct cros_ec_device *ec_dev, return ret; } +static void get_default_min_max_freq(enum motionsensor_type type, + u32 *min_freq, + u32 *max_freq) +{ + switch (type) { + case MOTIONSENSE_TYPE_ACCEL: + case MOTIONSENSE_TYPE_GYRO: + *min_freq = 12500; + *max_freq = 100000; + break; + case MOTIONSENSE_TYPE_MAG: + *min_freq = 5000; + *max_freq = 25000; + break; + case MOTIONSENSE_TYPE_PROX: + case MOTIONSENSE_TYPE_LIGHT: + *min_freq = 100; + *max_freq = 50000; + break; + case MOTIONSENSE_TYPE_BARO: + *min_freq = 250; + *max_freq = 20000; + break; + case MOTIONSENSE_TYPE_ACTIVITY: + default: + *min_freq = 0; + *max_freq = 0; + break; + } +} + int cros_ec_sensors_core_init(struct platform_device *pdev, struct iio_dev *indio_dev, bool physical_device) @@ -100,6 +131,19 @@ int cros_ec_sensors_core_init(struct platform_device *pdev, } state->type = state->resp->info.type; state->loc = state->resp->info.location; + + /* Value to stop the device */ + state->frequencies[0] = 0; + if (state->msg->version < 3) { + get_default_min_max_freq(state->resp->info.type, + &state->frequencies[1], + &state->frequencies[2]); + } else { + state->frequencies[1] = + state->resp->info_3.min_frequency; + state->frequencies[2] = + state->resp->info_3.max_frequency; + } } return 0; diff --git a/include/linux/iio/common/cros_ec_sensors_core.h b/include/linux/iio/common/cros_ec_sensors_core.h index 0c636b9fe8d7..94c87da22c04 100644 --- a/include/linux/iio/common/cros_ec_sensors_core.h +++ b/include/linux/iio/common/cros_ec_sensors_core.h @@ -70,6 +70,9 @@ struct cros_ec_sensors_core_state { unsigned long scan_mask, s16 *data); int curr_sampl_freq; + + /* Disable, Min and Max Sampling Frequency in mHz */ + int frequencies[3]; }; /** -- 2.20.1 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] iio: common: cros_ec_sensors: set default frequencies 2019-06-27 14:04 ` [PATCH 2/2] iio: common: cros_ec_sensors: set default frequencies Fabien Lahoudere @ 2019-06-27 15:48 ` Enric Balletbo i Serra 2019-06-28 9:43 ` Fabien Lahoudere 0 siblings, 1 reply; 12+ messages in thread From: Enric Balletbo i Serra @ 2019-06-27 15:48 UTC (permalink / raw) To: Fabien Lahoudere Cc: kernel, Nick Vaccaro, Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler, linux-iio, linux-kernel Hi Fabien, On 27/6/19 16:04, Fabien Lahoudere wrote: > Version 3 of the EC protocol provides min and max frequencies for EC sensors. > Default frequencies are provided for earlier protocol. > This patch should really go together with a respin of your previous patchset to 'Expose cros_ec_sensors frequency range via iio sysfs' [1] [1] https://www.spinics.net/lists/linux-iio/msg44963.html > Signed-off-by: Fabien Lahoudere <fabien.lahoudere@collabora.com> > Signed-off-by: Nick Vaccaro <nvaccaro@chromium.org> > --- > .../cros_ec_sensors/cros_ec_sensors_core.c | 44 +++++++++++++++++++ > .../linux/iio/common/cros_ec_sensors_core.h | 3 ++ > 2 files changed, 47 insertions(+) > > 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 2e0f97448e64..72f56d54cccd 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 > @@ -50,6 +50,37 @@ static int cros_ec_get_host_cmd_version_mask(struct cros_ec_device *ec_dev, > return ret; > } > > +static void get_default_min_max_freq(enum motionsensor_type type, > + u32 *min_freq, > + u32 *max_freq) > +{ > + switch (type) { > + case MOTIONSENSE_TYPE_ACCEL: > + case MOTIONSENSE_TYPE_GYRO: > + *min_freq = 12500; > + *max_freq = 100000; > + break; > + case MOTIONSENSE_TYPE_MAG: > + *min_freq = 5000; > + *max_freq = 25000; > + break; > + case MOTIONSENSE_TYPE_PROX: > + case MOTIONSENSE_TYPE_LIGHT: > + *min_freq = 100; > + *max_freq = 50000; > + break; > + case MOTIONSENSE_TYPE_BARO: > + *min_freq = 250; > + *max_freq = 20000; > + break; > + case MOTIONSENSE_TYPE_ACTIVITY: > + default: > + *min_freq = 0; > + *max_freq = 0; > + break; > + } > +} > + > int cros_ec_sensors_core_init(struct platform_device *pdev, > struct iio_dev *indio_dev, > bool physical_device) > @@ -100,6 +131,19 @@ int cros_ec_sensors_core_init(struct platform_device *pdev, > } > state->type = state->resp->info.type; > state->loc = state->resp->info.location; > + > + /* Value to stop the device */ > + state->frequencies[0] = 0; > + if (state->msg->version < 3) { > + get_default_min_max_freq(state->resp->info.type, > + &state->frequencies[1], > + &state->frequencies[2]); > + } else { > + state->frequencies[1] = > + state->resp->info_3.min_frequency; > + state->frequencies[2] = > + state->resp->info_3.max_frequency; > + } > } > > return 0; > diff --git a/include/linux/iio/common/cros_ec_sensors_core.h b/include/linux/iio/common/cros_ec_sensors_core.h > index 0c636b9fe8d7..94c87da22c04 100644 > --- a/include/linux/iio/common/cros_ec_sensors_core.h > +++ b/include/linux/iio/common/cros_ec_sensors_core.h > @@ -70,6 +70,9 @@ struct cros_ec_sensors_core_state { > unsigned long scan_mask, s16 *data); > > int curr_sampl_freq; > + > + /* Disable, Min and Max Sampling Frequency in mHz */ > + int frequencies[3]; > }; > > /** > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] iio: common: cros_ec_sensors: set default frequencies 2019-06-27 15:48 ` Enric Balletbo i Serra @ 2019-06-28 9:43 ` Fabien Lahoudere 0 siblings, 0 replies; 12+ messages in thread From: Fabien Lahoudere @ 2019-06-28 9:43 UTC (permalink / raw) To: Enric Balletbo i Serra Cc: kernel, Nick Vaccaro, Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler, linux-iio, linux-kernel Hi Enric Le jeudi 27 juin 2019 à 17:48 +0200, Enric Balletbo i Serra a écrit : > Hi Fabien, > > On 27/6/19 16:04, Fabien Lahoudere wrote: > > Version 3 of the EC protocol provides min and max frequencies for > > EC sensors. > > Default frequencies are provided for earlier protocol. > > > > This patch should really go together with a respin of your previous > patchset to > 'Expose cros_ec_sensors frequency range via iio sysfs' [1] > > [1] https://www.spinics.net/lists/linux-iio/msg44963.html > I agree, you're right. I will send only the first patch with Gwendal comments for v2. Thanks > > Signed-off-by: Fabien Lahoudere <fabien.lahoudere@collabora.com> > > Signed-off-by: Nick Vaccaro <nvaccaro@chromium.org> > > --- > > .../cros_ec_sensors/cros_ec_sensors_core.c | 44 > > +++++++++++++++++++ > > .../linux/iio/common/cros_ec_sensors_core.h | 3 ++ > > 2 files changed, 47 insertions(+) > > > > 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 2e0f97448e64..72f56d54cccd 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 > > @@ -50,6 +50,37 @@ static int > > cros_ec_get_host_cmd_version_mask(struct cros_ec_device *ec_dev, > > return ret; > > } > > > > +static void get_default_min_max_freq(enum motionsensor_type type, > > + u32 *min_freq, > > + u32 *max_freq) > > +{ > > + switch (type) { > > + case MOTIONSENSE_TYPE_ACCEL: > > + case MOTIONSENSE_TYPE_GYRO: > > + *min_freq = 12500; > > + *max_freq = 100000; > > + break; > > + case MOTIONSENSE_TYPE_MAG: > > + *min_freq = 5000; > > + *max_freq = 25000; > > + break; > > + case MOTIONSENSE_TYPE_PROX: > > + case MOTIONSENSE_TYPE_LIGHT: > > + *min_freq = 100; > > + *max_freq = 50000; > > + break; > > + case MOTIONSENSE_TYPE_BARO: > > + *min_freq = 250; > > + *max_freq = 20000; > > + break; > > + case MOTIONSENSE_TYPE_ACTIVITY: > > + default: > > + *min_freq = 0; > > + *max_freq = 0; > > + break; > > + } > > +} > > + > > int cros_ec_sensors_core_init(struct platform_device *pdev, > > struct iio_dev *indio_dev, > > bool physical_device) > > @@ -100,6 +131,19 @@ int cros_ec_sensors_core_init(struct > > platform_device *pdev, > > } > > state->type = state->resp->info.type; > > state->loc = state->resp->info.location; > > + > > + /* Value to stop the device */ > > + state->frequencies[0] = 0; > > + if (state->msg->version < 3) { > > + get_default_min_max_freq(state->resp- > > >info.type, > > + &state- > > >frequencies[1], > > + &state- > > >frequencies[2]); > > + } else { > > + state->frequencies[1] = > > + state->resp->info_3.min_frequency; > > + state->frequencies[2] = > > + state->resp->info_3.max_frequency; > > + } > > } > > > > return 0; > > diff --git a/include/linux/iio/common/cros_ec_sensors_core.h > > b/include/linux/iio/common/cros_ec_sensors_core.h > > index 0c636b9fe8d7..94c87da22c04 100644 > > --- a/include/linux/iio/common/cros_ec_sensors_core.h > > +++ b/include/linux/iio/common/cros_ec_sensors_core.h > > @@ -70,6 +70,9 @@ struct cros_ec_sensors_core_state { > > unsigned long scan_mask, s16 > > *data); > > > > int curr_sampl_freq; > > + > > + /* Disable, Min and Max Sampling Frequency in mHz */ > > + int frequencies[3]; > > }; > > > > /** > > ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2019-06-28 17:01 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-06-27 14:04 [PATCH 0/2] Add protocol v3 support Fabien Lahoudere 2019-06-27 14:04 ` [PATCH 1/2] iio: common: cros_ec_sensors: determine protocol version Fabien Lahoudere 2019-06-27 15:59 ` Enric Balletbo i Serra 2019-06-27 21:59 ` Gwendal Grignou 2019-06-28 9:36 ` Fabien Lahoudere 2019-06-28 10:32 ` Enric Balletbo i Serra 2019-06-28 11:37 ` Fabien Lahoudere 2019-06-28 13:46 ` Enric Balletbo i Serra 2019-06-28 17:01 ` Gwendal Grignou 2019-06-27 14:04 ` [PATCH 2/2] iio: common: cros_ec_sensors: set default frequencies Fabien Lahoudere 2019-06-27 15:48 ` Enric Balletbo i Serra 2019-06-28 9:43 ` Fabien Lahoudere
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).