[RFC 0/1] Add new sysfs ABI for chromebook devices

[RFC 0/1] Add new sysfs ABI for chromebook devices

[Fabien Lahoudere]

Chromebooks embedded controller (EC) provides new field in the 3rd version of 
its protocol. Those values need to be exposed as sysfs entries.

Minimum and maximum frequencies may probably be a standard iio ABI.
There is an existing ABI (sampling_frequency_available) but only for discrete 
set of values.
We have different possible solution and I want to ask interested people for the 
best one:
1. we keep the ABI proposed in this RFC (in the chromebook specific code)
2. we move min and max freq as 2 different standard ABI
3. we add a new standard iio ABI to set a range (sampling_frequency_range)
4. we change the 'sampling_frequency_available' ABI to return discrete values 
like '2 4 6 8' or a range like '[2 - 8]'
5. the solution I didn't think about

Thanks

Gwendal Grignou (1):
  iio: common: cros_ec_sensors: add extra sensor API

 .../ABI/testing/sysfs-bus-iio-cros-ec         |  24 ++++
 .../cros_ec_sensors/cros_ec_sensors_core.c    | 126 +++++++++++++++++-
 .../linux/iio/common/cros_ec_sensors_core.h   |   7 +
 include/linux/mfd/cros_ec_commands.h          |  21 +++
 4 files changed, 177 insertions(+), 1 deletion(-)

-- 
2.19.2

[RFC 1/1] iio: common: cros_ec_sensors: add extra sensor API

[Fabien Lahoudere]

From: Gwendal Grignou <gwendal@chromium.org>

Version 3 of the EC protocol provides min and max frequencies and fifo
size for EC sensors.
The driver allows EC to set parameters for version 3 and set default values
for earlier versions.
The sysfs entries are read only and cannot be used to impact on values.

Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
Signed-off-by: Fabien Lahoudere <fabien.lahoudere@collabora.com>
---
 .../ABI/testing/sysfs-bus-iio-cros-ec         |  24 ++++
 .../cros_ec_sensors/cros_ec_sensors_core.c    | 126 +++++++++++++++++-
 .../linux/iio/common/cros_ec_sensors_core.h   |   7 +
 include/linux/mfd/cros_ec_commands.h          |  21 +++
 4 files changed, 177 insertions(+), 1 deletion(-)

diff --git a/Documentation/ABI/testing/sysfs-bus-iio-cros-ec b/Documentation/ABI/testing/sysfs-bus-iio-cros-ec
index 0e95c2ca105c..85d266f4a57e 100644
--- a/Documentation/ABI/testing/sysfs-bus-iio-cros-ec
+++ b/Documentation/ABI/testing/sysfs-bus-iio-cros-ec
@@ -26,3 +26,27 @@ Description:
 		driver and represents the sensor ID as exposed by the EC. This
 		ID is used by the Android sensor service hardware abstraction
 		layer (sensor HAL) through the Android container on ChromeOS.
+
+What:		/sys/bus/iio/devices/iio:deviceX/min_frequency
+Date:		April 2019
+KernelVersion:	5.0
+Contact:	linux-iio@vger.kernel.org
+Description:
+		This attribute is exposed by CrOS EC sensors. It defines the
+		minimum frequency used by the sensor.
+
+What:		/sys/bus/iio/devices/iio:deviceX/max_frequency
+Date:		April 2019
+KernelVersion:	5.0
+Contact:	linux-iio@vger.kernel.org
+Description:
+		This attribute is exposed by CrOS EC sensors. It defines the
+		maximum frequency used by the sensor.
+
+What:		/sys/bus/iio/devices/iio:deviceX/max_events
+Date:		April 2019
+KernelVersion:	5.0
+Contact:	linux-iio@vger.kernel.org
+Description:
+		This attribute is exposed by CrOS EC sensors. It defines the
+		maximum number of events in the	fifo.
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 719a0df5aeeb..84908a4085ef 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
@@ -19,12 +19,84 @@
 #include <linux/slab.h>
 #include <linux/platform_device.h>
 
+/*
+ * Hard coded to the first device to support sensor fifo.  The EC has a 2048
+ * byte fifo and will trigger an interrupt when fifo is 2/3 full.
+ */
+#define CROS_EC_FIFO_SIZE (2048 * 2 / 3)
+
 static char *cros_ec_loc[] = {
 	[MOTIONSENSE_LOC_BASE] = "base",
 	[MOTIONSENSE_LOC_LID] = "lid",
 	[MOTIONSENSE_LOC_MAX] = "unknown",
 };
 
+static void get_default_min_max_freq_and_fifo_size(enum motionsensor_type type,
+						   u32 *min_freq,
+						   u32 *max_freq,
+						   u32 *max_fifo_events)
+{
+	/* we don't know fifo size, set to size previously used by sensor HAL */
+	*max_fifo_events = CROS_EC_FIFO_SIZE;
+
+	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:
+		*max_fifo_events = 0;
+		*min_freq = 0;
+		*max_freq = 0;
+		break;
+	}
+}
+
+static int cros_ec_get_host_cmd_version_mask(struct cros_ec_device *ec_dev,
+					     u16 cmd_offset, u16 cmd, u32 *mask)
+{
+	struct {
+		struct cros_ec_command msg;
+		union {
+			struct ec_params_get_cmd_versions params;
+			struct ec_response_get_cmd_versions resp;
+		};
+	} __packed buf;
+	struct ec_params_get_cmd_versions *params = &buf.params;
+	struct ec_response_get_cmd_versions *resp = &buf.resp;
+	struct cros_ec_command *msg = &buf.msg;
+	int ret;
+
+	memset(&buf, 0, sizeof(buf));
+	msg->command = EC_CMD_GET_CMD_VERSIONS + cmd_offset;
+	msg->insize = sizeof(*resp);
+	msg->outsize = sizeof(*params);
+	params->cmd = cmd;
+	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;
+}
+
 int cros_ec_sensors_core_init(struct platform_device *pdev,
 			      struct iio_dev *indio_dev,
 			      bool physical_device)
@@ -33,6 +105,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 +121,16 @@ int cros_ec_sensors_core_init(struct platform_device *pdev,
 
 	mutex_init(&state->cmd_lock);
 
+	/* determine what version of MOTIONSENSE CMD EC has */
+	ret = cros_ec_get_host_cmd_version_mask(state->ec,
+						ec->cmd_offset,
+						EC_CMD_MOTION_SENSE_CMD,
+						&ver_mask);
+	if (ret < 0 || ver_mask == 0)
+		return -ENODEV;
+
 	/* Set up the host command structure. */
-	state->msg->version = 2;
+	state->msg->version = fls(ver_mask) - 1;
 	state->msg->command = EC_CMD_MOTION_SENSE_CMD + ec->cmd_offset;
 	state->msg->outsize = sizeof(struct ec_params_motion_sense);
 
@@ -66,6 +148,20 @@ int cros_ec_sensors_core_init(struct platform_device *pdev,
 		}
 		state->type = state->resp->info.type;
 		state->loc = state->resp->info.location;
+		if (state->msg->version < 3) {
+			get_default_min_max_freq_and_fifo_size(
+					state->resp->info.type,
+					&state->min_freq,
+					&state->max_freq,
+					&state->fifo_max_event_count);
+		} else {
+			state->min_freq =
+				state->resp->info_3.min_frequency;
+			state->max_freq =
+				state->resp->info_3.max_frequency;
+			state->fifo_max_event_count =
+				state->resp->info_3.fifo_max_event_count;
+		}
 	}
 
 	return 0;
@@ -134,6 +230,19 @@ static ssize_t cros_ec_sensors_loc(struct iio_dev *indio_dev,
 	return snprintf(buf, PAGE_SIZE, "%s\n", cros_ec_loc[st->loc]);
 }
 
+#define DEVICE_STATE_INT_ATTR(_name, _var) \
+static ssize_t cros_ec_sensors_##_name(struct iio_dev *indio_dev, \
+		uintptr_t private, const struct iio_chan_spec *chan, \
+		char *buf) \
+{ \
+	struct cros_ec_sensors_core_state *st = iio_priv(indio_dev); \
+	return snprintf(buf, PAGE_SIZE, "%d\n", st->_var); \
+}
+
+DEVICE_STATE_INT_ATTR(min_freq, min_freq);
+DEVICE_STATE_INT_ATTR(max_freq, max_freq);
+DEVICE_STATE_INT_ATTR(max_events, fifo_max_event_count);
+
 const struct iio_chan_spec_ext_info cros_ec_sensors_ext_info[] = {
 	{
 		.name = "calibrate",
@@ -145,6 +254,21 @@ const struct iio_chan_spec_ext_info cros_ec_sensors_ext_info[] = {
 		.shared = IIO_SHARED_BY_ALL,
 		.read = cros_ec_sensors_loc
 	},
+	{
+		.name = "min_frequency",
+		.shared = IIO_SHARED_BY_ALL,
+		.read = cros_ec_sensors_min_freq
+	},
+	{
+		.name = "max_frequency",
+		.shared = IIO_SHARED_BY_ALL,
+		.read = cros_ec_sensors_max_freq
+	},
+	{
+		.name = "max_events",
+		.shared = IIO_SHARED_BY_ALL,
+		.read = cros_ec_sensors_max_events
+	},
 	{ },
 };
 EXPORT_SYMBOL_GPL(cros_ec_sensors_ext_info);
diff --git a/include/linux/iio/common/cros_ec_sensors_core.h b/include/linux/iio/common/cros_ec_sensors_core.h
index ce16445411ac..4742a9637a85 100644
--- a/include/linux/iio/common/cros_ec_sensors_core.h
+++ b/include/linux/iio/common/cros_ec_sensors_core.h
@@ -78,6 +78,13 @@ struct cros_ec_sensors_core_state {
 				    unsigned long scan_mask, s16 *data);
 
 	int curr_sampl_freq;
+
+	/* Min and Max Sampling Frequency in mHz */
+	u32 min_freq;
+	u32 max_freq;
+
+	/* event fifo size represented in number of events */
+	u32 fifo_max_event_count;
 };
 
 /**
diff --git a/include/linux/mfd/cros_ec_commands.h b/include/linux/mfd/cros_ec_commands.h
index 43bee4f7d137..7fbf29416a0b 100644
--- a/include/linux/mfd/cros_ec_commands.h
+++ b/include/linux/mfd/cros_ec_commands.h
@@ -1714,6 +1714,27 @@ struct ec_response_motion_sense {
 			uint8_t chip;
 		} info;
 
+		/* Used for MOTIONSENSE_CMD_INFO version 3 */
+		struct __ec_todo_unpacked {
+			/* Should be element of enum motionsensor_type. */
+			u8 type;
+
+			/* Should be element of enum motionsensor_location. */
+			u8 location;
+
+			/* Should be element of enum motionsensor_chip. */
+			u8 chip;
+
+			/* Minimum sensor sampling frequency */
+			u32 min_frequency;
+
+			/* Maximum sensor sampling frequency */
+			u32 max_frequency;
+
+			/* Max number of sensor events that could be in fifo */
+			u32 fifo_max_event_count;
+		} info_3;
+
 		/* Used for MOTIONSENSE_CMD_DATA */
 		struct ec_response_motion_sensor_data data;
 
-- 
2.19.2

Re: [RFC 0/1] Add new sysfs ABI for chromebook devices

[Jonathan Cameron]

On Fri,  3 May 2019 12:54:45 +0200
Fabien Lahoudere <fabien.lahoudere@collabora.com> wrote:

> Chromebooks embedded controller (EC) provides new field in the 3rd version of 
> its protocol. Those values need to be exposed as sysfs entries.
> 
> Minimum and maximum frequencies may probably be a standard iio ABI.
> There is an existing ABI (sampling_frequency_available) but only for discrete 
> set of values.
> We have different possible solution and I want to ask interested people for the 
> best one:
> 1. we keep the ABI proposed in this RFC (in the chromebook specific code)
> 2. we move min and max freq as 2 different standard ABI
> 3. we add a new standard iio ABI to set a range (sampling_frequency_range)
> 4. we change the 'sampling_frequency_available' ABI to return discrete values 
> like '2 4 6 8' or a range like '[2 - 8]'
> 5. the solution I didn't think about
Gah, it's been on my todo list for far too long to put out proper docs for
it but we do have a way of doing this in IIO.

Best bet is too look at the code in
industrialio-core.c for iio_read_channel_info_avail, and specifically
iio_format_avail_range

That produces strings of the format [MIN STEP MAX] so more or less your
option 4, with the [] syntax indicating to userspace that it's a range
version.

We did this a while back to provide a single interface for consumer drivers
and for userspace.  If you poke around in the period around when that
patch was merged you'll find a discussion which was pretty much the
options you have above.

Note if anyone else wants to have a go at docs for this feature I would
definitely welcome it!

Jonathan

> 
> Thanks
> 
> Gwendal Grignou (1):
>   iio: common: cros_ec_sensors: add extra sensor API
> 
>  .../ABI/testing/sysfs-bus-iio-cros-ec         |  24 ++++
>  .../cros_ec_sensors/cros_ec_sensors_core.c    | 126 +++++++++++++++++-
>  .../linux/iio/common/cros_ec_sensors_core.h   |   7 +
>  include/linux/mfd/cros_ec_commands.h          |  21 +++
>  4 files changed, 177 insertions(+), 1 deletion(-)
> 

Re: [RFC 1/1] iio: common: cros_ec_sensors: add extra sensor API

[Jonathan Cameron]

On Fri,  3 May 2019 12:54:46 +0200
Fabien Lahoudere <fabien.lahoudere@collabora.com> wrote:

> From: Gwendal Grignou <gwendal@chromium.org>
> 
> Version 3 of the EC protocol provides min and max frequencies and fifo
> size for EC sensors.
> The driver allows EC to set parameters for version 3 and set default values
> for earlier versions.
> The sysfs entries are read only and cannot be used to impact on values.
> 
> Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
> Signed-off-by: Fabien Lahoudere <fabien.lahoudere@collabora.com>
Mostly changes in here will follow from comments on the cover letter
but a few other things inline.

Thanks,

Jonathan
> ---
>  .../ABI/testing/sysfs-bus-iio-cros-ec         |  24 ++++
>  .../cros_ec_sensors/cros_ec_sensors_core.c    | 126 +++++++++++++++++-
>  .../linux/iio/common/cros_ec_sensors_core.h   |   7 +
>  include/linux/mfd/cros_ec_commands.h          |  21 +++
>  4 files changed, 177 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-iio-cros-ec b/Documentation/ABI/testing/sysfs-bus-iio-cros-ec
> index 0e95c2ca105c..85d266f4a57e 100644
> --- a/Documentation/ABI/testing/sysfs-bus-iio-cros-ec
> +++ b/Documentation/ABI/testing/sysfs-bus-iio-cros-ec
> @@ -26,3 +26,27 @@ Description:
>  		driver and represents the sensor ID as exposed by the EC. This
>  		ID is used by the Android sensor service hardware abstraction
>  		layer (sensor HAL) through the Android container on ChromeOS.
> +
> +What:		/sys/bus/iio/devices/iio:deviceX/min_frequency
> +Date:		April 2019
> +KernelVersion:	5.0
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		This attribute is exposed by CrOS EC sensors. It defines the
> +		minimum frequency used by the sensor.
> +
> +What:		/sys/bus/iio/devices/iio:deviceX/max_frequency
> +Date:		April 2019
> +KernelVersion:	5.0
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		This attribute is exposed by CrOS EC sensors. It defines the
> +		maximum frequency used by the sensor.
> +
See reply to the cover letter on these.  Bonus points if you implement them
with the read_avail callback.

> +What:		/sys/bus/iio/devices/iio:deviceX/max_events
> +Date:		April 2019
> +KernelVersion:	5.0
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		This attribute is exposed by CrOS EC sensors. It defines the
> +		maximum number of events in the	fifo.

Why does userspace care?

> 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 719a0df5aeeb..84908a4085ef 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
> @@ -19,12 +19,84 @@
>  #include <linux/slab.h>
>  #include <linux/platform_device.h>
>  
> +/*
> + * Hard coded to the first device to support sensor fifo.  The EC has a 2048
> + * byte fifo and will trigger an interrupt when fifo is 2/3 full.
> + */
> +#define CROS_EC_FIFO_SIZE (2048 * 2 / 3)
> +
>  static char *cros_ec_loc[] = {
>  	[MOTIONSENSE_LOC_BASE] = "base",
>  	[MOTIONSENSE_LOC_LID] = "lid",
>  	[MOTIONSENSE_LOC_MAX] = "unknown",
>  };
>  
> +static void get_default_min_max_freq_and_fifo_size(enum motionsensor_type type,
> +						   u32 *min_freq,
> +						   u32 *max_freq,
> +						   u32 *max_fifo_events)
> +{
> +	/* we don't know fifo size, set to size previously used by sensor HAL */
> +	*max_fifo_events = CROS_EC_FIFO_SIZE;
> +
> +	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:
> +		*max_fifo_events = 0;
> +		*min_freq = 0;
> +		*max_freq = 0;
> +		break;
> +	}
> +}
> +
> +static int cros_ec_get_host_cmd_version_mask(struct cros_ec_device *ec_dev,
> +					     u16 cmd_offset, u16 cmd, u32 *mask)
> +{
> +	struct {
> +		struct cros_ec_command msg;
> +		union {
> +			struct ec_params_get_cmd_versions params;
> +			struct ec_response_get_cmd_versions resp;
> +		};
> +	} __packed buf;
> +	struct ec_params_get_cmd_versions *params = &buf.params;
> +	struct ec_response_get_cmd_versions *resp = &buf.resp;
> +	struct cros_ec_command *msg = &buf.msg;
> +	int ret;
> +
> +	memset(&buf, 0, sizeof(buf));
> +	msg->command = EC_CMD_GET_CMD_VERSIONS + cmd_offset;
> +	msg->insize = sizeof(*resp);
> +	msg->outsize = sizeof(*params);
You could, I think, use c99 structure initialization to set these neatly
and avoid the need for the memset being explicit.


> +	params->cmd = cmd;
> +	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;
> +}
> +
>  int cros_ec_sensors_core_init(struct platform_device *pdev,
>  			      struct iio_dev *indio_dev,
>  			      bool physical_device)
> @@ -33,6 +105,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 +121,16 @@ int cros_ec_sensors_core_init(struct platform_device *pdev,
>  
>  	mutex_init(&state->cmd_lock);
>  
> +	/* determine what version of MOTIONSENSE CMD EC has */
> +	ret = cros_ec_get_host_cmd_version_mask(state->ec,
> +						ec->cmd_offset,
> +						EC_CMD_MOTION_SENSE_CMD,
> +						&ver_mask);
> +	if (ret < 0 || ver_mask == 0)
> +		return -ENODEV;
> +
>  	/* Set up the host command structure. */
> -	state->msg->version = 2;
> +	state->msg->version = fls(ver_mask) - 1;
>  	state->msg->command = EC_CMD_MOTION_SENSE_CMD + ec->cmd_offset;
>  	state->msg->outsize = sizeof(struct ec_params_motion_sense);
>  
> @@ -66,6 +148,20 @@ int cros_ec_sensors_core_init(struct platform_device *pdev,
>  		}
>  		state->type = state->resp->info.type;
>  		state->loc = state->resp->info.location;
> +		if (state->msg->version < 3) {
> +			get_default_min_max_freq_and_fifo_size(
> +					state->resp->info.type,
> +					&state->min_freq,
> +					&state->max_freq,
> +					&state->fifo_max_event_count);
> +		} else {
> +			state->min_freq =
> +				state->resp->info_3.min_frequency;
> +			state->max_freq =
> +				state->resp->info_3.max_frequency;
> +			state->fifo_max_event_count =
> +				state->resp->info_3.fifo_max_event_count;
> +		}
>  	}
>  
>  	return 0;
> @@ -134,6 +230,19 @@ static ssize_t cros_ec_sensors_loc(struct iio_dev *indio_dev,
>  	return snprintf(buf, PAGE_SIZE, "%s\n", cros_ec_loc[st->loc]);
>  }
>  
> +#define DEVICE_STATE_INT_ATTR(_name, _var) \
> +static ssize_t cros_ec_sensors_##_name(struct iio_dev *indio_dev, \
> +		uintptr_t private, const struct iio_chan_spec *chan, \
> +		char *buf) \
> +{ \
> +	struct cros_ec_sensors_core_state *st = iio_priv(indio_dev); \
> +	return snprintf(buf, PAGE_SIZE, "%d\n", st->_var); \
> +}
> +
> +DEVICE_STATE_INT_ATTR(min_freq, min_freq);
> +DEVICE_STATE_INT_ATTR(max_freq, max_freq);
> +DEVICE_STATE_INT_ATTR(max_events, fifo_max_event_count);
> +
>  const struct iio_chan_spec_ext_info cros_ec_sensors_ext_info[] = {
>  	{
>  		.name = "calibrate",
> @@ -145,6 +254,21 @@ const struct iio_chan_spec_ext_info cros_ec_sensors_ext_info[] = {
>  		.shared = IIO_SHARED_BY_ALL,
>  		.read = cros_ec_sensors_loc
>  	},
> +	{
> +		.name = "min_frequency",
> +		.shared = IIO_SHARED_BY_ALL,
> +		.read = cros_ec_sensors_min_freq
> +	},
> +	{
> +		.name = "max_frequency",
> +		.shared = IIO_SHARED_BY_ALL,
> +		.read = cros_ec_sensors_max_freq
> +	},
> +	{
> +		.name = "max_events",
> +		.shared = IIO_SHARED_BY_ALL,
> +		.read = cros_ec_sensors_max_events
> +	},
>  	{ },
>  };
>  EXPORT_SYMBOL_GPL(cros_ec_sensors_ext_info);
> diff --git a/include/linux/iio/common/cros_ec_sensors_core.h b/include/linux/iio/common/cros_ec_sensors_core.h
> index ce16445411ac..4742a9637a85 100644
> --- a/include/linux/iio/common/cros_ec_sensors_core.h
> +++ b/include/linux/iio/common/cros_ec_sensors_core.h
> @@ -78,6 +78,13 @@ struct cros_ec_sensors_core_state {
>  				    unsigned long scan_mask, s16 *data);
>  
>  	int curr_sampl_freq;
> +
> +	/* Min and Max Sampling Frequency in mHz */
> +	u32 min_freq;
> +	u32 max_freq;
> +
> +	/* event fifo size represented in number of events */
> +	u32 fifo_max_event_count;
>  };
>  
>  /**
> diff --git a/include/linux/mfd/cros_ec_commands.h b/include/linux/mfd/cros_ec_commands.h
> index 43bee4f7d137..7fbf29416a0b 100644
> --- a/include/linux/mfd/cros_ec_commands.h
> +++ b/include/linux/mfd/cros_ec_commands.h
> @@ -1714,6 +1714,27 @@ struct ec_response_motion_sense {
>  			uint8_t chip;
>  		} info;
>  
> +		/* Used for MOTIONSENSE_CMD_INFO version 3 */
> +		struct __ec_todo_unpacked {
> +			/* Should be element of enum motionsensor_type. */
> +			u8 type;
> +
> +			/* Should be element of enum motionsensor_location. */
> +			u8 location;
> +
> +			/* Should be element of enum motionsensor_chip. */
> +			u8 chip;
> +
> +			/* Minimum sensor sampling frequency */
> +			u32 min_frequency;
> +
> +			/* Maximum sensor sampling frequency */
> +			u32 max_frequency;
> +
> +			/* Max number of sensor events that could be in fifo */
> +			u32 fifo_max_event_count;
> +		} info_3;
> +
>  		/* Used for MOTIONSENSE_CMD_DATA */
>  		struct ec_response_motion_sensor_data data;
>  

Re: [RFC 1/1] iio: common: cros_ec_sensors: add extra sensor API

[Gwendal Grignou]

On Sun, May 5, 2019 at 8:36 AM Jonathan Cameron
<jic23@jic23.retrosnub.co.uk> wrote:
>
> On Fri,  3 May 2019 12:54:46 +0200
> Fabien Lahoudere <fabien.lahoudere@collabora.com> wrote:
>
> > From: Gwendal Grignou <gwendal@chromium.org>
> >
> > Version 3 of the EC protocol provides min and max frequencies and fifo
> > size for EC sensors.
> > The driver allows EC to set parameters for version 3 and set default values
> > for earlier versions.
> > The sysfs entries are read only and cannot be used to impact on values.
> >
> > Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
> > Signed-off-by: Fabien Lahoudere <fabien.lahoudere@collabora.com>
> Mostly changes in here will follow from comments on the cover letter
> but a few other things inline.
>
> Thanks,
>
> Jonathan
> > ---
> >  .../ABI/testing/sysfs-bus-iio-cros-ec         |  24 ++++
> >  .../cros_ec_sensors/cros_ec_sensors_core.c    | 126 +++++++++++++++++-
> >  .../linux/iio/common/cros_ec_sensors_core.h   |   7 +
> >  include/linux/mfd/cros_ec_commands.h          |  21 +++
> >  4 files changed, 177 insertions(+), 1 deletion(-)
> >
> > diff --git a/Documentation/ABI/testing/sysfs-bus-iio-cros-ec b/Documentation/ABI/testing/sysfs-bus-iio-cros-ec
> > index 0e95c2ca105c..85d266f4a57e 100644
> > --- a/Documentation/ABI/testing/sysfs-bus-iio-cros-ec
> > +++ b/Documentation/ABI/testing/sysfs-bus-iio-cros-ec
> > @@ -26,3 +26,27 @@ Description:
> >               driver and represents the sensor ID as exposed by the EC. This
> >               ID is used by the Android sensor service hardware abstraction
> >               layer (sensor HAL) through the Android container on ChromeOS.
> > +
> > +What:                /sys/bus/iio/devices/iio:deviceX/min_frequency
> > +Date:                April 2019
> > +KernelVersion:       5.0
> > +Contact:     linux-iio@vger.kernel.org
> > +Description:
> > +             This attribute is exposed by CrOS EC sensors. It defines the
> > +             minimum frequency used by the sensor.
> > +
> > +What:                /sys/bus/iio/devices/iio:deviceX/max_frequency
> > +Date:                April 2019
> > +KernelVersion:       5.0
> > +Contact:     linux-iio@vger.kernel.org
> > +Description:
> > +             This attribute is exposed by CrOS EC sensors. It defines the
> > +             maximum frequency used by the sensor.
> > +
> See reply to the cover letter on these.  Bonus points if you implement them
> with the read_avail callback.
>
> > +What:                /sys/bus/iio/devices/iio:deviceX/max_events
> > +Date:                April 2019
> > +KernelVersion:       5.0
> > +Contact:     linux-iio@vger.kernel.org
> > +Description:
> > +             This attribute is exposed by CrOS EC sensors. It defines the
> > +             maximum number of events in the fifo.
>
> Why does userspace care?
Is use to report the size of the hardware FIFO to Android, to enable
batching mode:
https://source.android.com/devices/sensors/batching.
It is reported in fifoReservedEventCount or fifoMaxEventCount in the
sensor_t structure described at
https://source.android.com/devices/sensors/hal-interface#sensor_t.
Having said that, batching is not terribly useful for tablet or
laptop, but is critical for wearable.

Re: [RFC 1/1] iio: common: cros_ec_sensors: add extra sensor API

[Jonathan Cameron]

On Fri, 24 May 2019 17:28:42 -0700
Gwendal Grignou <gwendal@chromium.org> wrote:

> On Sun, May 5, 2019 at 8:36 AM Jonathan Cameron
> <jic23@jic23.retrosnub.co.uk> wrote:
> >
> > On Fri,  3 May 2019 12:54:46 +0200
> > Fabien Lahoudere <fabien.lahoudere@collabora.com> wrote:
> >  
> > > From: Gwendal Grignou <gwendal@chromium.org>
> > >
> > > Version 3 of the EC protocol provides min and max frequencies and fifo
> > > size for EC sensors.
> > > The driver allows EC to set parameters for version 3 and set default values
> > > for earlier versions.
> > > The sysfs entries are read only and cannot be used to impact on values.
> > >
> > > Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
> > > Signed-off-by: Fabien Lahoudere <fabien.lahoudere@collabora.com>  
> > Mostly changes in here will follow from comments on the cover letter
> > but a few other things inline.
> >
> > Thanks,
> >
> > Jonathan  
> > > ---
> > >  .../ABI/testing/sysfs-bus-iio-cros-ec         |  24 ++++
> > >  .../cros_ec_sensors/cros_ec_sensors_core.c    | 126 +++++++++++++++++-
> > >  .../linux/iio/common/cros_ec_sensors_core.h   |   7 +
> > >  include/linux/mfd/cros_ec_commands.h          |  21 +++
> > >  4 files changed, 177 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/Documentation/ABI/testing/sysfs-bus-iio-cros-ec b/Documentation/ABI/testing/sysfs-bus-iio-cros-ec
> > > index 0e95c2ca105c..85d266f4a57e 100644
> > > --- a/Documentation/ABI/testing/sysfs-bus-iio-cros-ec
> > > +++ b/Documentation/ABI/testing/sysfs-bus-iio-cros-ec
> > > @@ -26,3 +26,27 @@ Description:
> > >               driver and represents the sensor ID as exposed by the EC. This
> > >               ID is used by the Android sensor service hardware abstraction
> > >               layer (sensor HAL) through the Android container on ChromeOS.
> > > +
> > > +What:                /sys/bus/iio/devices/iio:deviceX/min_frequency
> > > +Date:                April 2019
> > > +KernelVersion:       5.0
> > > +Contact:     linux-iio@vger.kernel.org
> > > +Description:
> > > +             This attribute is exposed by CrOS EC sensors. It defines the
> > > +             minimum frequency used by the sensor.
> > > +
> > > +What:                /sys/bus/iio/devices/iio:deviceX/max_frequency
> > > +Date:                April 2019
> > > +KernelVersion:       5.0
> > > +Contact:     linux-iio@vger.kernel.org
> > > +Description:
> > > +             This attribute is exposed by CrOS EC sensors. It defines the
> > > +             maximum frequency used by the sensor.
> > > +  
> > See reply to the cover letter on these.  Bonus points if you implement them
> > with the read_avail callback.
> >  
> > > +What:                /sys/bus/iio/devices/iio:deviceX/max_events
> > > +Date:                April 2019
> > > +KernelVersion:       5.0
> > > +Contact:     linux-iio@vger.kernel.org
> > > +Description:
> > > +             This attribute is exposed by CrOS EC sensors. It defines the
> > > +             maximum number of events in the fifo.  
> >
> > Why does userspace care?  
> Is use to report the size of the hardware FIFO to Android, to enable
> batching mode:
> https://source.android.com/devices/sensors/batching.
> It is reported in fifoReservedEventCount or fifoMaxEventCount in the
> sensor_t structure described at
> https://source.android.com/devices/sensors/hal-interface#sensor_t.
> Having said that, batching is not terribly useful for tablet or
> laptop, but is critical for wearable.

Ok. So let me just check, in android terms, events are just readings
from the sensor?  These aren't events in the IIO sense of 'thresholds'
being passed?

If so can we map this to hwfifo_watermark[_available] using the range
descriptor? We have a bit of legacy around that with  hwfifo_watermark_max
and hwfifo_watermark_min also defined, but should probably move towards
the more standard _available form and deprecate the other two.

Jonathan

Re: [RFC 1/1] iio: common: cros_ec_sensors: add extra sensor API

[Gwendal Grignou]

On Mon, May 27, 2019 at 2:18 AM Jonathan Cameron
<jic23@jic23.retrosnub.co.uk> wrote:
>
> On Fri, 24 May 2019 17:28:42 -0700
> Gwendal Grignou <gwendal@chromium.org> wrote:
>
> > On Sun, May 5, 2019 at 8:36 AM Jonathan Cameron
> > <jic23@jic23.retrosnub.co.uk> wrote:
> > >
> > > On Fri,  3 May 2019 12:54:46 +0200
> > > Fabien Lahoudere <fabien.lahoudere@collabora.com> wrote:
> > >
> > > > From: Gwendal Grignou <gwendal@chromium.org>
> > > >
> > > > Version 3 of the EC protocol provides min and max frequencies and fifo
> > > > size for EC sensors.
> > > > The driver allows EC to set parameters for version 3 and set default values
> > > > for earlier versions.
> > > > The sysfs entries are read only and cannot be used to impact on values.
> > > >
> > > > Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
> > > > Signed-off-by: Fabien Lahoudere <fabien.lahoudere@collabora.com>
> > > Mostly changes in here will follow from comments on the cover letter
> > > but a few other things inline.
> > >
> > > Thanks,
> > >
> > > Jonathan
> > > > ---
> > > >  .../ABI/testing/sysfs-bus-iio-cros-ec         |  24 ++++
> > > >  .../cros_ec_sensors/cros_ec_sensors_core.c    | 126 +++++++++++++++++-
> > > >  .../linux/iio/common/cros_ec_sensors_core.h   |   7 +
> > > >  include/linux/mfd/cros_ec_commands.h          |  21 +++
> > > >  4 files changed, 177 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/Documentation/ABI/testing/sysfs-bus-iio-cros-ec b/Documentation/ABI/testing/sysfs-bus-iio-cros-ec
> > > > index 0e95c2ca105c..85d266f4a57e 100644
> > > > --- a/Documentation/ABI/testing/sysfs-bus-iio-cros-ec
> > > > +++ b/Documentation/ABI/testing/sysfs-bus-iio-cros-ec
> > > > @@ -26,3 +26,27 @@ Description:
> > > >               driver and represents the sensor ID as exposed by the EC. This
> > > >               ID is used by the Android sensor service hardware abstraction
> > > >               layer (sensor HAL) through the Android container on ChromeOS.
> > > > +
> > > > +What:                /sys/bus/iio/devices/iio:deviceX/min_frequency
> > > > +Date:                April 2019
> > > > +KernelVersion:       5.0
> > > > +Contact:     linux-iio@vger.kernel.org
> > > > +Description:
> > > > +             This attribute is exposed by CrOS EC sensors. It defines the
> > > > +             minimum frequency used by the sensor.
> > > > +
> > > > +What:                /sys/bus/iio/devices/iio:deviceX/max_frequency
> > > > +Date:                April 2019
> > > > +KernelVersion:       5.0
> > > > +Contact:     linux-iio@vger.kernel.org
> > > > +Description:
> > > > +             This attribute is exposed by CrOS EC sensors. It defines the
> > > > +             maximum frequency used by the sensor.
> > > > +
> > > See reply to the cover letter on these.  Bonus points if you implement them
> > > with the read_avail callback.
> > >
> > > > +What:                /sys/bus/iio/devices/iio:deviceX/max_events
> > > > +Date:                April 2019
> > > > +KernelVersion:       5.0
> > > > +Contact:     linux-iio@vger.kernel.org
> > > > +Description:
> > > > +             This attribute is exposed by CrOS EC sensors. It defines the
> > > > +             maximum number of events in the fifo.
> > >
> > > Why does userspace care?
> > Is use to report the size of the hardware FIFO to Android, to enable
> > batching mode:
> > https://source.android.com/devices/sensors/batching.
> > It is reported in fifoReservedEventCount or fifoMaxEventCount in the
> > sensor_t structure described at
> > https://source.android.com/devices/sensors/hal-interface#sensor_t.
> > Having said that, batching is not terribly useful for tablet or
> > laptop, but is critical for wearable.
>
> Ok. So let me just check, in android terms, events are just readings
> from the sensor?
Yes [https://source.android.com/devices/sensors/hal-interface#sensors_event_t]
> These aren't events in the IIO sense of 'thresholds'
> being passed?
Correct.
>
> If so can we map this to hwfifo_watermark[_available] using the range
> descriptor? We have a bit of legacy around that with  hwfifo_watermark_max
> and hwfifo_watermark_min also defined, but should probably move towards
> the more standard _available form and deprecate the other two.
That make sense: hwfifo_watermark_available will be set to a single
value, which match hwfifo_watermak_max.
Cros EC has a limited support:
- the size can not be changed
- the EC will seldom fill the hwfifo, it fires interrupt to a hard
coded threshold to avoid losing samples
- the hwfifo is shared by all sensors
- sampling_frequency attribute is used to force the EC to generate
interrupt before the hwfifo is full.

Replacing max_events with hwfifo_watermark_max works for me.

Thanks,
Gwendal.
>
> Jonathan
>
>