[PATCH 0/2] Add protocol v3 support

[PATCH 0/2] Add protocol v3 support

[Fabien Lahoudere]

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

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

[Fabien Lahoudere]

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

[PATCH 2/2] iio: common: cros_ec_sensors: set default frequencies

[Fabien Lahoudere]

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

Re: [PATCH 2/2] iio: common: cros_ec_sensors: set default frequencies

[Enric Balletbo i Serra]

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];
>  };
>  
>  /**
> 

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

[Enric Balletbo i Serra]

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

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

[Gwendal Grignou]

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

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

[Fabien Lahoudere]

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

Re: [PATCH 2/2] iio: common: cros_ec_sensors: set default frequencies

[Fabien Lahoudere]

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];
> >  };
> >  
> >  /**
> > 

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

[Enric Balletbo i Serra]

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

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

[Fabien Lahoudere]

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

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

[Enric Balletbo i Serra]

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

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