[PATCH v2 0/3] Expose cros_ec_sensors frequency range via iio sysfs

[PATCH v2 0/3] Expose cros_ec_sensors frequency range via iio sysfs

[Fabien Lahoudere]

Chromebooks EC sensors must expose a range of frequencies for each sensors using
the standard ABI sampling_frquency_available.

Changes since v1:
- Add a cover letter
- Add Nick Vaccaro SoB to patch 1
- Drop fifo size related code

Fabien Lahoudere (3):
  iio: common: cros_ec_sensors: support protocol v3 message
  iio: common: cros_ec_sensors: add sysfs attribute for frequencies
  docs: iio: add precision about sampling_frequency_available

 Documentation/ABI/testing/sysfs-bus-iio       |   7 +-
 .../common/cros_ec_sensors/cros_ec_sensors.c  |   6 +-
 .../cros_ec_sensors/cros_ec_sensors_core.c    | 121 +++++++++++++++++-
 drivers/iio/light/cros_ec_light_prox.c        |   6 +-
 drivers/iio/pressure/cros_ec_baro.c           |   6 +-
 .../linux/iio/common/cros_ec_sensors_core.h   |   8 +-
 include/linux/mfd/cros_ec_commands.h          |  21 +++
 7 files changed, 162 insertions(+), 13 deletions(-)

-- 
2.20.1

[PATCH v2 1/3] iio: common: cros_ec_sensors: support protocol v3 message

[Fabien Lahoudere]

Version 3 of the EC protocol provides min and max frequencies and fifo
size for EC sensors.

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    | 83 ++++++++++++++++++-
 .../linux/iio/common/cros_ec_sensors_core.h   |  4 +
 include/linux/mfd/cros_ec_commands.h          | 21 +++++
 3 files changed, 107 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 719a0df5aeeb..ac53ea32c1b1 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,67 @@ static char *cros_ec_loc[] = {
 	[MOTIONSENSE_LOC_MAX] = "unknown",
 };
 
+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;
+	}
+}
+
+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 +94,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 +110,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 +137,16 @@ 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(state->resp->info.type,
+						 &state->min_freq,
+						 &state->max_freq);
+		} else {
+			state->min_freq =
+				state->resp->info_3.min_frequency;
+			state->max_freq =
+				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 ce16445411ac..32fd08bbcf52 100644
--- a/include/linux/iio/common/cros_ec_sensors_core.h
+++ b/include/linux/iio/common/cros_ec_sensors_core.h
@@ -78,6 +78,10 @@ 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;
 };
 
 /**
diff --git a/include/linux/mfd/cros_ec_commands.h b/include/linux/mfd/cros_ec_commands.h
index dcec96f01879..db2e3a9a656e 100644
--- a/include/linux/mfd/cros_ec_commands.h
+++ b/include/linux/mfd/cros_ec_commands.h
@@ -1744,6 +1744,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. */
+			uint8_t type;
+
+			/* Should be element of enum motionsensor_location. */
+			uint8_t location;
+
+			/* Should be element of enum motionsensor_chip. */
+			uint8_t chip;
+
+			/* Minimum sensor sampling frequency */
+			uint32_t min_frequency;
+
+			/* Maximum sensor sampling frequency */
+			uint32_t max_frequency;
+
+			/* Reserved field */
+			uint32_t reserved;
+		} info_3;
+
 		/* Used for MOTIONSENSE_CMD_DATA */
 		struct ec_response_motion_sensor_data data;
 
-- 
2.20.1

Re: [PATCH v2 1/3] iio: common: cros_ec_sensors: support protocol v3 message

[Lee Jones]

On Thu, 23 May 2019, Fabien Lahoudere wrote:

> Version 3 of the EC protocol provides min and max frequencies and fifo
> size for EC sensors.
> 
> 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    | 83 ++++++++++++++++++-
>  .../linux/iio/common/cros_ec_sensors_core.h   |  4 +
>  include/linux/mfd/cros_ec_commands.h          | 21 +++++

There have been many changes to this file recently.  We will have to
co-ordinate the merge.

But for now:

For my own reference:
  Acked-for-MFD-by: Lee Jones <lee.jones@linaro.org>

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v2 1/3] iio: common: cros_ec_sensors: support protocol v3 message

[Fabien Lahoudere]

Le mercredi 12 juin 2019 à 09:42 +0100, Lee Jones a écrit :
> On Thu, 23 May 2019, Fabien Lahoudere wrote:
> 
> > Version 3 of the EC protocol provides min and max frequencies and
> > fifo
> > size for EC sensors.
> > 
> > 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    | 83
> > ++++++++++++++++++-
> >  .../linux/iio/common/cros_ec_sensors_core.h   |  4 +
> >  include/linux/mfd/cros_ec_commands.h          | 21 +++++
> 
> There have been many changes to this file recently.  We will have to
> co-ordinate the merge.
> 
> But for now:
> 
> For my own reference:
>   Acked-for-MFD-by: Lee Jones <lee.jones@linaro.org>
> 

Yes I see the changes and my next submission will use recent patch and
drop my modification in cros_ec_commands.h.

Thanks for reviewing

[PATCH v2 2/3] iio: common: cros_ec_sensors: add sysfs attribute for frequencies

[Fabien Lahoudere]

In order to provide minimum and maximum frequencies for each sensors,
we use a standard API (sampling_frequency_available) to provide them
to userland.
As cros_ec_sensors_core_init do not manage default attrs, we change
the signature to let all kind of sensors to provide "struct iio_info"
with their callback. This change impact drivers using that function.

Then cros_ec_* sensors provides frequencies range in sysfs like this:
[min step max]

Signed-off-by: Fabien Lahoudere <fabien.lahoudere@collabora.com>
---
 .../common/cros_ec_sensors/cros_ec_sensors.c  |  6 +--
 .../cros_ec_sensors/cros_ec_sensors_core.c    | 38 +++++++++++++++++++
 drivers/iio/light/cros_ec_light_prox.c        |  6 +--
 drivers/iio/pressure/cros_ec_baro.c           |  6 +--
 .../linux/iio/common/cros_ec_sensors_core.h   |  4 +-
 5 files changed, 50 insertions(+), 10 deletions(-)

diff --git a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors.c b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors.c
index 17af4e0fd5f8..a0ecee15a6c8 100644
--- a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors.c
+++ b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors.c
@@ -172,7 +172,7 @@ static int cros_ec_sensors_write(struct iio_dev *indio_dev,
 	return ret;
 }
 
-static const struct iio_info ec_sensors_info = {
+static struct iio_info ec_sensors_info = {
 	.read_raw = &cros_ec_sensors_read,
 	.write_raw = &cros_ec_sensors_write,
 };
@@ -195,11 +195,11 @@ static int cros_ec_sensors_probe(struct platform_device *pdev)
 	if (!indio_dev)
 		return -ENOMEM;
 
-	ret = cros_ec_sensors_core_init(pdev, indio_dev, true);
+	ret = cros_ec_sensors_core_init(pdev, indio_dev, &ec_sensors_info,
+					true);
 	if (ret)
 		return ret;
 
-	indio_dev->info = &ec_sensors_info;
 	state = iio_priv(indio_dev);
 	for (channel = state->channels, i = CROS_EC_SENSOR_X;
 	     i < CROS_EC_SENSOR_MAX_AXIS; i++, channel++) {
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 ac53ea32c1b1..08fb5d3dc7b5 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
@@ -10,6 +10,7 @@
 #include <linux/iio/buffer.h>
 #include <linux/iio/common/cros_ec_sensors_core.h>
 #include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
 #include <linux/iio/kfifo_buf.h>
 #include <linux/iio/trigger_consumer.h>
 #include <linux/kernel.h>
@@ -86,8 +87,42 @@ static int cros_ec_get_host_cmd_version_mask(struct cros_ec_device *ec_dev,
 	return ret;
 }
 
+/**
+ * cros_ec_sensors_read_freq() - sysfs function to get available frequencies
+ * @dev: Device structure for this device.
+ * @attr: Description of the attribute.
+ * @buf: Incoming string
+ *
+ * The later modes are only relevant to the ring buffer - and depend on current
+ * mode. Note that data sheet gives rather wide tolerances for these so integer
+ * division will give good enough answer and not all chips have them specified
+ * at all.
+ **/
+static ssize_t cros_ec_sensors_read_freq(struct device *dev,
+					 struct device_attribute *attr,
+					 char *buf)
+{
+	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct cros_ec_sensors_core_state *state = iio_priv(indio_dev);
+
+	return snprintf(buf, PAGE_SIZE, "[%d 1 %d]\n", state->min_freq,
+			state->max_freq);
+}
+
+static IIO_DEV_ATTR_SAMP_FREQ_AVAIL(cros_ec_sensors_read_freq);
+
+static struct attribute *cros_ec_sensors_attributes[] = {
+	&iio_dev_attr_sampling_frequency_available.dev_attr.attr,
+	NULL,
+};
+
+static const struct attribute_group cros_ec_sensors_attribute_group = {
+	.attrs = cros_ec_sensors_attributes,
+};
+
 int cros_ec_sensors_core_init(struct platform_device *pdev,
 			      struct iio_dev *indio_dev,
+			      struct iio_info *info,
 			      bool physical_device)
 {
 	struct device *dev = &pdev->dev;
@@ -149,6 +184,9 @@ int cros_ec_sensors_core_init(struct platform_device *pdev,
 		}
 	}
 
+	info->attrs = &cros_ec_sensors_attribute_group;
+	indio_dev->info = info;
+
 	return 0;
 }
 EXPORT_SYMBOL_GPL(cros_ec_sensors_core_init);
diff --git a/drivers/iio/light/cros_ec_light_prox.c b/drivers/iio/light/cros_ec_light_prox.c
index 308ee6ff2e22..1772e339cf14 100644
--- a/drivers/iio/light/cros_ec_light_prox.c
+++ b/drivers/iio/light/cros_ec_light_prox.c
@@ -161,7 +161,7 @@ static int cros_ec_light_prox_write(struct iio_dev *indio_dev,
 	return ret;
 }
 
-static const struct iio_info cros_ec_light_prox_info = {
+static struct iio_info cros_ec_light_prox_info = {
 	.read_raw = &cros_ec_light_prox_read,
 	.write_raw = &cros_ec_light_prox_write,
 };
@@ -184,11 +184,11 @@ static int cros_ec_light_prox_probe(struct platform_device *pdev)
 	if (!indio_dev)
 		return -ENOMEM;
 
-	ret = cros_ec_sensors_core_init(pdev, indio_dev, true);
+	ret = cros_ec_sensors_core_init(pdev, indio_dev,
+					&cros_ec_light_prox_info, true);
 	if (ret)
 		return ret;
 
-	indio_dev->info = &cros_ec_light_prox_info;
 	state = iio_priv(indio_dev);
 	state->core.type = state->core.resp->info.type;
 	state->core.loc = state->core.resp->info.location;
diff --git a/drivers/iio/pressure/cros_ec_baro.c b/drivers/iio/pressure/cros_ec_baro.c
index 034ce98d6e97..cd3be0f16226 100644
--- a/drivers/iio/pressure/cros_ec_baro.c
+++ b/drivers/iio/pressure/cros_ec_baro.c
@@ -107,7 +107,7 @@ static int cros_ec_baro_write(struct iio_dev *indio_dev,
 	return ret;
 }
 
-static const struct iio_info cros_ec_baro_info = {
+static struct iio_info cros_ec_baro_info = {
 	.read_raw = &cros_ec_baro_read,
 	.write_raw = &cros_ec_baro_write,
 };
@@ -130,11 +130,11 @@ static int cros_ec_baro_probe(struct platform_device *pdev)
 	if (!indio_dev)
 		return -ENOMEM;
 
-	ret = cros_ec_sensors_core_init(pdev, indio_dev, true);
+	ret = cros_ec_sensors_core_init(pdev, indio_dev, &cros_ec_baro_info,
+					true);
 	if (ret)
 		return ret;
 
-	indio_dev->info = &cros_ec_baro_info;
 	state = iio_priv(indio_dev);
 	state->core.type = state->core.resp->info.type;
 	state->core.loc = state->core.resp->info.location;
diff --git a/include/linux/iio/common/cros_ec_sensors_core.h b/include/linux/iio/common/cros_ec_sensors_core.h
index 32fd08bbcf52..f170a72ac08d 100644
--- a/include/linux/iio/common/cros_ec_sensors_core.h
+++ b/include/linux/iio/common/cros_ec_sensors_core.h
@@ -114,12 +114,14 @@ struct platform_device;
  * cros_ec_sensors_core_init() - basic initialization of the core structure
  * @pdev:		platform device created for the sensors
  * @indio_dev:		iio device structure of the device
+ * @info:		iio info structure with read and write callback
  * @physical_device:	true if the device refers to a physical device
  *
  * Return: 0 on success, -errno on failure.
  */
 int cros_ec_sensors_core_init(struct platform_device *pdev,
-			      struct iio_dev *indio_dev, bool physical_device);
+			      struct iio_dev *indio_dev, struct iio_info *info,
+			      bool physical_device);
 
 /**
  * cros_ec_sensors_capture() - the trigger handler function
-- 
2.20.1

Re: [PATCH v2 2/3] iio: common: cros_ec_sensors: add sysfs attribute for frequencies

[Jonathan Cameron]

On Thu, 23 May 2019 11:07:36 +0200
Fabien Lahoudere <fabien.lahoudere@collabora.com> wrote:

> In order to provide minimum and maximum frequencies for each sensors,
> we use a standard API (sampling_frequency_available) to provide them
> to userland.
> As cros_ec_sensors_core_init do not manage default attrs, we change
> the signature to let all kind of sensors to provide "struct iio_info"
> with their callback. This change impact drivers using that function.
> 
> Then cros_ec_* sensors provides frequencies range in sysfs like this:
> [min step max]
> 
> Signed-off-by: Fabien Lahoudere <fabien.lahoudere@collabora.com>
When I was pointing at the _available syntax I was meaning that
the ideal is to implement this using the associated callbacks rather
than as a custom sysfs attribute.

> ---
>  .../common/cros_ec_sensors/cros_ec_sensors.c  |  6 +--
>  .../cros_ec_sensors/cros_ec_sensors_core.c    | 38 +++++++++++++++++++
>  drivers/iio/light/cros_ec_light_prox.c        |  6 +--
>  drivers/iio/pressure/cros_ec_baro.c           |  6 +--
>  .../linux/iio/common/cros_ec_sensors_core.h   |  4 +-
>  5 files changed, 50 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors.c b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors.c
> index 17af4e0fd5f8..a0ecee15a6c8 100644
> --- a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors.c
> +++ b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors.c
> @@ -172,7 +172,7 @@ static int cros_ec_sensors_write(struct iio_dev *indio_dev,
>  	return ret;
>  }
>  
> -static const struct iio_info ec_sensors_info = {
> +static struct iio_info ec_sensors_info = {
>  	.read_raw = &cros_ec_sensors_read,
>  	.write_raw = &cros_ec_sensors_write,
>  };
> @@ -195,11 +195,11 @@ static int cros_ec_sensors_probe(struct platform_device *pdev)
>  	if (!indio_dev)
>  		return -ENOMEM;
>  
> -	ret = cros_ec_sensors_core_init(pdev, indio_dev, true);
> +	ret = cros_ec_sensors_core_init(pdev, indio_dev, &ec_sensors_info,
> +					true);
>  	if (ret)
>  		return ret;
>  
> -	indio_dev->info = &ec_sensors_info;
>  	state = iio_priv(indio_dev);
>  	for (channel = state->channels, i = CROS_EC_SENSOR_X;
>  	     i < CROS_EC_SENSOR_MAX_AXIS; i++, channel++) {
> 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 ac53ea32c1b1..08fb5d3dc7b5 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
> @@ -10,6 +10,7 @@
>  #include <linux/iio/buffer.h>
>  #include <linux/iio/common/cros_ec_sensors_core.h>
>  #include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
>  #include <linux/iio/kfifo_buf.h>
>  #include <linux/iio/trigger_consumer.h>
>  #include <linux/kernel.h>
> @@ -86,8 +87,42 @@ static int cros_ec_get_host_cmd_version_mask(struct cros_ec_device *ec_dev,
>  	return ret;
>  }
>  
> +/**
> + * cros_ec_sensors_read_freq() - sysfs function to get available frequencies
> + * @dev: Device structure for this device.
> + * @attr: Description of the attribute.
> + * @buf: Incoming string
> + *
> + * The later modes are only relevant to the ring buffer - and depend on current
> + * mode. Note that data sheet gives rather wide tolerances for these so integer
> + * division will give good enough answer and not all chips have them specified
> + * at all.
> + **/
> +static ssize_t cros_ec_sensors_read_freq(struct device *dev,
> +					 struct device_attribute *attr,
> +					 char *buf)
> +{
> +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> +	struct cros_ec_sensors_core_state *state = iio_priv(indio_dev);
> +
> +	return snprintf(buf, PAGE_SIZE, "[%d 1 %d]\n", state->min_freq,
> +			state->max_freq);
Whilst it is a bit more fiddly I would much prefer if this was done with
the info_mask_shared_by_all_available bit mask in the iio_dev and providing
the read_avail callback.

The original reason to introduce this form was as part of trying to
(far too slowly) kill off as much hand defined ABI as possible. 
Ultimate aim is to make the IIO interface optional for cases where
the channels are mostly being used by other consumer drivers rather than
being directly consumed by userspace.  To do that we need all of
these elements to be easily accessible from the consumer hooks.



> +}
> +
> +static IIO_DEV_ATTR_SAMP_FREQ_AVAIL(cros_ec_sensors_read_freq);
> +
> +static struct attribute *cros_ec_sensors_attributes[] = {
> +	&iio_dev_attr_sampling_frequency_available.dev_attr.attr,
> +	NULL,
> +};
> +
> +static const struct attribute_group cros_ec_sensors_attribute_group = {
> +	.attrs = cros_ec_sensors_attributes,
> +};
> +
>  int cros_ec_sensors_core_init(struct platform_device *pdev,
>  			      struct iio_dev *indio_dev,
> +			      struct iio_info *info,
>  			      bool physical_device)
>  {
>  	struct device *dev = &pdev->dev;
> @@ -149,6 +184,9 @@ int cros_ec_sensors_core_init(struct platform_device *pdev,
>  		}
>  	}
>  
> +	info->attrs = &cros_ec_sensors_attribute_group;
> +	indio_dev->info = info;
> +
>  	return 0;
>  }
>  EXPORT_SYMBOL_GPL(cros_ec_sensors_core_init);
> diff --git a/drivers/iio/light/cros_ec_light_prox.c b/drivers/iio/light/cros_ec_light_prox.c
> index 308ee6ff2e22..1772e339cf14 100644
> --- a/drivers/iio/light/cros_ec_light_prox.c
> +++ b/drivers/iio/light/cros_ec_light_prox.c
> @@ -161,7 +161,7 @@ static int cros_ec_light_prox_write(struct iio_dev *indio_dev,
>  	return ret;
>  }
>  
> -static const struct iio_info cros_ec_light_prox_info = {
> +static struct iio_info cros_ec_light_prox_info = {
>  	.read_raw = &cros_ec_light_prox_read,
>  	.write_raw = &cros_ec_light_prox_write,
>  };
> @@ -184,11 +184,11 @@ static int cros_ec_light_prox_probe(struct platform_device *pdev)
>  	if (!indio_dev)
>  		return -ENOMEM;
>  
> -	ret = cros_ec_sensors_core_init(pdev, indio_dev, true);
> +	ret = cros_ec_sensors_core_init(pdev, indio_dev,
> +					&cros_ec_light_prox_info, true);
>  	if (ret)
>  		return ret;
>  
> -	indio_dev->info = &cros_ec_light_prox_info;
>  	state = iio_priv(indio_dev);
>  	state->core.type = state->core.resp->info.type;
>  	state->core.loc = state->core.resp->info.location;
> diff --git a/drivers/iio/pressure/cros_ec_baro.c b/drivers/iio/pressure/cros_ec_baro.c
> index 034ce98d6e97..cd3be0f16226 100644
> --- a/drivers/iio/pressure/cros_ec_baro.c
> +++ b/drivers/iio/pressure/cros_ec_baro.c
> @@ -107,7 +107,7 @@ static int cros_ec_baro_write(struct iio_dev *indio_dev,
>  	return ret;
>  }
>  
> -static const struct iio_info cros_ec_baro_info = {
> +static struct iio_info cros_ec_baro_info = {
>  	.read_raw = &cros_ec_baro_read,
>  	.write_raw = &cros_ec_baro_write,
>  };
> @@ -130,11 +130,11 @@ static int cros_ec_baro_probe(struct platform_device *pdev)
>  	if (!indio_dev)
>  		return -ENOMEM;
>  
> -	ret = cros_ec_sensors_core_init(pdev, indio_dev, true);
> +	ret = cros_ec_sensors_core_init(pdev, indio_dev, &cros_ec_baro_info,
> +					true);
>  	if (ret)
>  		return ret;
>  
> -	indio_dev->info = &cros_ec_baro_info;
>  	state = iio_priv(indio_dev);
>  	state->core.type = state->core.resp->info.type;
>  	state->core.loc = state->core.resp->info.location;
> diff --git a/include/linux/iio/common/cros_ec_sensors_core.h b/include/linux/iio/common/cros_ec_sensors_core.h
> index 32fd08bbcf52..f170a72ac08d 100644
> --- a/include/linux/iio/common/cros_ec_sensors_core.h
> +++ b/include/linux/iio/common/cros_ec_sensors_core.h
> @@ -114,12 +114,14 @@ struct platform_device;
>   * cros_ec_sensors_core_init() - basic initialization of the core structure
>   * @pdev:		platform device created for the sensors
>   * @indio_dev:		iio device structure of the device
> + * @info:		iio info structure with read and write callback
>   * @physical_device:	true if the device refers to a physical device
>   *
>   * Return: 0 on success, -errno on failure.
>   */
>  int cros_ec_sensors_core_init(struct platform_device *pdev,
> -			      struct iio_dev *indio_dev, bool physical_device);
> +			      struct iio_dev *indio_dev, struct iio_info *info,
> +			      bool physical_device);
>  
>  /**
>   * cros_ec_sensors_capture() - the trigger handler function

Re: [PATCH v2 2/3] iio: common: cros_ec_sensors: add sysfs attribute for frequencies

[Fabien Lahoudere]

Le dimanche 26 mai 2019 à 18:45 +0100, Jonathan Cameron a écrit :
> On Thu, 23 May 2019 11:07:36 +0200
> Fabien Lahoudere <fabien.lahoudere@collabora.com> wrote:
> 
> > In order to provide minimum and maximum frequencies for each
> > sensors,
> > we use a standard API (sampling_frequency_available) to provide
> > them
> > to userland.
> > As cros_ec_sensors_core_init do not manage default attrs, we change
> > the signature to let all kind of sensors to provide "struct
> > iio_info"
> > with their callback. This change impact drivers using that
> > function.
> > 
> > Then cros_ec_* sensors provides frequencies range in sysfs like
> > this:
> > [min step max]
> > 
> > Signed-off-by: Fabien Lahoudere <fabien.lahoudere@collabora.com>
> When I was pointing at the _available syntax I was meaning that
> the ideal is to implement this using the associated callbacks rather
> than as a custom sysfs attribute.
> 

Sorry, I misunderstood. Let me retry with that callback implemented.

> > ---
> >  .../common/cros_ec_sensors/cros_ec_sensors.c  |  6 +--
> >  .../cros_ec_sensors/cros_ec_sensors_core.c    | 38
> > +++++++++++++++++++
> >  drivers/iio/light/cros_ec_light_prox.c        |  6 +--
> >  drivers/iio/pressure/cros_ec_baro.c           |  6 +--
> >  .../linux/iio/common/cros_ec_sensors_core.h   |  4 +-
> >  5 files changed, 50 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors.c
> > b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors.c
> > index 17af4e0fd5f8..a0ecee15a6c8 100644
> > --- a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors.c
> > +++ b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors.c
> > @@ -172,7 +172,7 @@ static int cros_ec_sensors_write(struct iio_dev
> > *indio_dev,
> >  	return ret;
> >  }
> >  
> > -static const struct iio_info ec_sensors_info = {
> > +static struct iio_info ec_sensors_info = {
> >  	.read_raw = &cros_ec_sensors_read,
> >  	.write_raw = &cros_ec_sensors_write,
> >  };
> > @@ -195,11 +195,11 @@ static int cros_ec_sensors_probe(struct
> > platform_device *pdev)
> >  	if (!indio_dev)
> >  		return -ENOMEM;
> >  
> > -	ret = cros_ec_sensors_core_init(pdev, indio_dev, true);
> > +	ret = cros_ec_sensors_core_init(pdev, indio_dev,
> > &ec_sensors_info,
> > +					true);
> >  	if (ret)
> >  		return ret;
> >  
> > -	indio_dev->info = &ec_sensors_info;
> >  	state = iio_priv(indio_dev);
> >  	for (channel = state->channels, i = CROS_EC_SENSOR_X;
> >  	     i < CROS_EC_SENSOR_MAX_AXIS; i++, channel++) {
> > 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 ac53ea32c1b1..08fb5d3dc7b5 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
> > @@ -10,6 +10,7 @@
> >  #include <linux/iio/buffer.h>
> >  #include <linux/iio/common/cros_ec_sensors_core.h>
> >  #include <linux/iio/iio.h>
> > +#include <linux/iio/sysfs.h>
> >  #include <linux/iio/kfifo_buf.h>
> >  #include <linux/iio/trigger_consumer.h>
> >  #include <linux/kernel.h>
> > @@ -86,8 +87,42 @@ static int
> > cros_ec_get_host_cmd_version_mask(struct cros_ec_device *ec_dev,
> >  	return ret;
> >  }
> >  
> > +/**
> > + * cros_ec_sensors_read_freq() - sysfs function to get available
> > frequencies
> > + * @dev: Device structure for this device.
> > + * @attr: Description of the attribute.
> > + * @buf: Incoming string
> > + *
> > + * The later modes are only relevant to the ring buffer - and
> > depend on current
> > + * mode. Note that data sheet gives rather wide tolerances for
> > these so integer
> > + * division will give good enough answer and not all chips have
> > them specified
> > + * at all.
> > + **/
> > +static ssize_t cros_ec_sensors_read_freq(struct device *dev,
> > +					 struct device_attribute *attr,
> > +					 char *buf)
> > +{
> > +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> > +	struct cros_ec_sensors_core_state *state = iio_priv(indio_dev);
> > +
> > +	return snprintf(buf, PAGE_SIZE, "[%d 1 %d]\n", state->min_freq,
> > +			state->max_freq);
> Whilst it is a bit more fiddly I would much prefer if this was done
> with
> the info_mask_shared_by_all_available bit mask in the iio_dev and
> providing
> the read_avail callback.
> 
> The original reason to introduce this form was as part of trying to
> (far too slowly) kill off as much hand defined ABI as possible. 
> Ultimate aim is to make the IIO interface optional for cases where
> the channels are mostly being used by other consumer drivers rather
> than
> being directly consumed by userspace.  To do that we need all of
> these elements to be easily accessible from the consumer hooks.
> 
> 
> 
> > +}
> > +
> > +static IIO_DEV_ATTR_SAMP_FREQ_AVAIL(cros_ec_sensors_read_freq);
> > +
> > +static struct attribute *cros_ec_sensors_attributes[] = {
> > +	&iio_dev_attr_sampling_frequency_available.dev_attr.attr,
> > +	NULL,
> > +};
> > +
> > +static const struct attribute_group
> > cros_ec_sensors_attribute_group = {
> > +	.attrs = cros_ec_sensors_attributes,
> > +};
> > +
> >  int cros_ec_sensors_core_init(struct platform_device *pdev,
> >  			      struct iio_dev *indio_dev,
> > +			      struct iio_info *info,
> >  			      bool physical_device)
> >  {
> >  	struct device *dev = &pdev->dev;
> > @@ -149,6 +184,9 @@ int cros_ec_sensors_core_init(struct
> > platform_device *pdev,
> >  		}
> >  	}
> >  
> > +	info->attrs = &cros_ec_sensors_attribute_group;
> > +	indio_dev->info = info;
> > +
> >  	return 0;
> >  }
> >  EXPORT_SYMBOL_GPL(cros_ec_sensors_core_init);
> > diff --git a/drivers/iio/light/cros_ec_light_prox.c
> > b/drivers/iio/light/cros_ec_light_prox.c
> > index 308ee6ff2e22..1772e339cf14 100644
> > --- a/drivers/iio/light/cros_ec_light_prox.c
> > +++ b/drivers/iio/light/cros_ec_light_prox.c
> > @@ -161,7 +161,7 @@ static int cros_ec_light_prox_write(struct
> > iio_dev *indio_dev,
> >  	return ret;
> >  }
> >  
> > -static const struct iio_info cros_ec_light_prox_info = {
> > +static struct iio_info cros_ec_light_prox_info = {
> >  	.read_raw = &cros_ec_light_prox_read,
> >  	.write_raw = &cros_ec_light_prox_write,
> >  };
> > @@ -184,11 +184,11 @@ static int cros_ec_light_prox_probe(struct
> > platform_device *pdev)
> >  	if (!indio_dev)
> >  		return -ENOMEM;
> >  
> > -	ret = cros_ec_sensors_core_init(pdev, indio_dev, true);
> > +	ret = cros_ec_sensors_core_init(pdev, indio_dev,
> > +					&cros_ec_light_prox_info,
> > true);
> >  	if (ret)
> >  		return ret;
> >  
> > -	indio_dev->info = &cros_ec_light_prox_info;
> >  	state = iio_priv(indio_dev);
> >  	state->core.type = state->core.resp->info.type;
> >  	state->core.loc = state->core.resp->info.location;
> > diff --git a/drivers/iio/pressure/cros_ec_baro.c
> > b/drivers/iio/pressure/cros_ec_baro.c
> > index 034ce98d6e97..cd3be0f16226 100644
> > --- a/drivers/iio/pressure/cros_ec_baro.c
> > +++ b/drivers/iio/pressure/cros_ec_baro.c
> > @@ -107,7 +107,7 @@ static int cros_ec_baro_write(struct iio_dev
> > *indio_dev,
> >  	return ret;
> >  }
> >  
> > -static const struct iio_info cros_ec_baro_info = {
> > +static struct iio_info cros_ec_baro_info = {
> >  	.read_raw = &cros_ec_baro_read,
> >  	.write_raw = &cros_ec_baro_write,
> >  };
> > @@ -130,11 +130,11 @@ static int cros_ec_baro_probe(struct
> > platform_device *pdev)
> >  	if (!indio_dev)
> >  		return -ENOMEM;
> >  
> > -	ret = cros_ec_sensors_core_init(pdev, indio_dev, true);
> > +	ret = cros_ec_sensors_core_init(pdev, indio_dev,
> > &cros_ec_baro_info,
> > +					true);
> >  	if (ret)
> >  		return ret;
> >  
> > -	indio_dev->info = &cros_ec_baro_info;
> >  	state = iio_priv(indio_dev);
> >  	state->core.type = state->core.resp->info.type;
> >  	state->core.loc = state->core.resp->info.location;
> > diff --git a/include/linux/iio/common/cros_ec_sensors_core.h
> > b/include/linux/iio/common/cros_ec_sensors_core.h
> > index 32fd08bbcf52..f170a72ac08d 100644
> > --- a/include/linux/iio/common/cros_ec_sensors_core.h
> > +++ b/include/linux/iio/common/cros_ec_sensors_core.h
> > @@ -114,12 +114,14 @@ struct platform_device;
> >   * cros_ec_sensors_core_init() - basic initialization of the core
> > structure
> >   * @pdev:		platform device created for the sensors
> >   * @indio_dev:		iio device structure of the device
> > + * @info:		iio info structure with read and write callback
> >   * @physical_device:	true if the device refers to a physical
> > device
> >   *
> >   * Return: 0 on success, -errno on failure.
> >   */
> >  int cros_ec_sensors_core_init(struct platform_device *pdev,
> > -			      struct iio_dev *indio_dev, bool
> > physical_device);
> > +			      struct iio_dev *indio_dev, struct
> > iio_info *info,
> > +			      bool physical_device);
> >  
> >  /**
> >   * cros_ec_sensors_capture() - the trigger handler function

Re: [PATCH v2 2/3] iio: common: cros_ec_sensors: add sysfs attribute for frequencies

[Gwendal Grignou]

On Mon, May 27, 2019 at 2:55 AM Fabien Lahoudere
<fabien.lahoudere@collabora.com> wrote:
>
> Le dimanche 26 mai 2019 à 18:45 +0100, Jonathan Cameron a écrit :
> > On Thu, 23 May 2019 11:07:36 +0200
> > Fabien Lahoudere <fabien.lahoudere@collabora.com> wrote:
> >
> > > In order to provide minimum and maximum frequencies for each
> > > sensors,
> > > we use a standard API (sampling_frequency_available) to provide
> > > them
> > > to userland.
> > > As cros_ec_sensors_core_init do not manage default attrs, we change
> > > the signature to let all kind of sensors to provide "struct
> > > iio_info"
> > > with their callback. This change impact drivers using that
> > > function.
> > >
> > > Then cros_ec_* sensors provides frequencies range in sysfs like
> > > this:
> > > [min step max]
> > >
> > > Signed-off-by: Fabien Lahoudere <fabien.lahoudere@collabora.com>
> > When I was pointing at the _available syntax I was meaning that
> > the ideal is to implement this using the associated callbacks rather
> > than as a custom sysfs attribute.
> >
>
> Sorry, I misunderstood. Let me retry with that callback implemented.
>
> > > ---
> > >  .../common/cros_ec_sensors/cros_ec_sensors.c  |  6 +--
> > >  .../cros_ec_sensors/cros_ec_sensors_core.c    | 38
> > > +++++++++++++++++++
> > >  drivers/iio/light/cros_ec_light_prox.c        |  6 +--
> > >  drivers/iio/pressure/cros_ec_baro.c           |  6 +--
> > >  .../linux/iio/common/cros_ec_sensors_core.h   |  4 +-
> > >  5 files changed, 50 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors.c
> > > b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors.c
> > > index 17af4e0fd5f8..a0ecee15a6c8 100644
> > > --- a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors.c
> > > +++ b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors.c
> > > @@ -172,7 +172,7 @@ static int cros_ec_sensors_write(struct iio_dev
> > > *indio_dev,
> > >     return ret;
> > >  }
> > >
> > > -static const struct iio_info ec_sensors_info = {
> > > +static struct iio_info ec_sensors_info = {
> > >     .read_raw = &cros_ec_sensors_read,
> > >     .write_raw = &cros_ec_sensors_write,
> > >  };
> > > @@ -195,11 +195,11 @@ static int cros_ec_sensors_probe(struct
> > > platform_device *pdev)
> > >     if (!indio_dev)
> > >             return -ENOMEM;
> > >
> > > -   ret = cros_ec_sensors_core_init(pdev, indio_dev, true);
> > > +   ret = cros_ec_sensors_core_init(pdev, indio_dev,
> > > &ec_sensors_info,
> > > +                                   true);
> > >     if (ret)
> > >             return ret;
> > >
> > > -   indio_dev->info = &ec_sensors_info;
> > >     state = iio_priv(indio_dev);
> > >     for (channel = state->channels, i = CROS_EC_SENSOR_X;
> > >          i < CROS_EC_SENSOR_MAX_AXIS; i++, channel++) {
> > > 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 ac53ea32c1b1..08fb5d3dc7b5 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
> > > @@ -10,6 +10,7 @@
> > >  #include <linux/iio/buffer.h>
> > >  #include <linux/iio/common/cros_ec_sensors_core.h>
> > >  #include <linux/iio/iio.h>
> > > +#include <linux/iio/sysfs.h>
> > >  #include <linux/iio/kfifo_buf.h>
> > >  #include <linux/iio/trigger_consumer.h>
> > >  #include <linux/kernel.h>
> > > @@ -86,8 +87,42 @@ static int
> > > cros_ec_get_host_cmd_version_mask(struct cros_ec_device *ec_dev,
> > >     return ret;
> > >  }
> > >
> > > +/**
> > > + * cros_ec_sensors_read_freq() - sysfs function to get available
> > > frequencies
> > > + * @dev: Device structure for this device.
> > > + * @attr: Description of the attribute.
> > > + * @buf: Incoming string
> > > + *
> > > + * The later modes are only relevant to the ring buffer - and
> > > depend on current
> > > + * mode. Note that data sheet gives rather wide tolerances for
> > > these so integer
> > > + * division will give good enough answer and not all chips have
> > > them specified
> > > + * at all.
> > > + **/
> > > +static ssize_t cros_ec_sensors_read_freq(struct device *dev,
> > > +                                    struct device_attribute *attr,
> > > +                                    char *buf)
> > > +{
> > > +   struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> > > +   struct cros_ec_sensors_core_state *state = iio_priv(indio_dev);
> > > +
> > > +   return snprintf(buf, PAGE_SIZE, "[%d 1 %d]\n", state->min_freq,
> > > +                   state->max_freq);
Step to 1 [Hz?] is not right; EC uses the frequency the sensors
supports, rounded up by default.
Given EC is hiding the sensors it controls, it is not straight-forward
to find the supported frequencies between min_freq and max_freq.
However, sensors mostly follow the following rules:
- "fast" sensors: accelerometer, gyroscope, magnetometer,  barometer:
Supported frequencies are: [ min_freq, max_freq >> n, max_freq >> (n -
1), ... max_freq >> 1, max_freq], where (min_freq >> n) > min_freq.
frequency are expressed in mHz resolution.
- "Slow" sensors: light, proximity:
Any frequencies between min_freq and max_freq are supported.

Note that frequency 0 is accepted, it puts the given sensor in suspend mode.

Given that information, it would make sense to follow the existing
macro IIO_DEV_ATTR_SAMP_FREQ_AVAIL and report an array of frequencies,
like bmc150_magn_show_samp_freq_avail does.

Gwendal.

> > Whilst it is a bit more fiddly I would much prefer if this was done
> > with
> > the info_mask_shared_by_all_available bit mask in the iio_dev and
> > providing
> > the read_avail callback.
> >
> > The original reason to introduce this form was as part of trying to
> > (far too slowly) kill off as much hand defined ABI as possible.
> > Ultimate aim is to make the IIO interface optional for cases where
> > the channels are mostly being used by other consumer drivers rather
> > than
> > being directly consumed by userspace.  To do that we need all of
> > these elements to be easily accessible from the consumer hooks.
> >
> >
> >
> > > +}
> > > +
> > > +static IIO_DEV_ATTR_SAMP_FREQ_AVAIL(cros_ec_sensors_read_freq);
> > > +
> > > +static struct attribute *cros_ec_sensors_attributes[] = {
> > > +   &iio_dev_attr_sampling_frequency_available.dev_attr.attr,
> > > +   NULL,
> > > +};
> > > +
> > > +static const struct attribute_group
> > > cros_ec_sensors_attribute_group = {
> > > +   .attrs = cros_ec_sensors_attributes,
> > > +};
> > > +
> > >  int cros_ec_sensors_core_init(struct platform_device *pdev,
> > >                           struct iio_dev *indio_dev,
> > > +                         struct iio_info *info,
> > >                           bool physical_device)
> > >  {
> > >     struct device *dev = &pdev->dev;
> > > @@ -149,6 +184,9 @@ int cros_ec_sensors_core_init(struct
> > > platform_device *pdev,
> > >             }
> > >     }
> > >
> > > +   info->attrs = &cros_ec_sensors_attribute_group;
> > > +   indio_dev->info = info;
> > > +
> > >     return 0;
> > >  }
> > >  EXPORT_SYMBOL_GPL(cros_ec_sensors_core_init);
> > > diff --git a/drivers/iio/light/cros_ec_light_prox.c
> > > b/drivers/iio/light/cros_ec_light_prox.c
> > > index 308ee6ff2e22..1772e339cf14 100644
> > > --- a/drivers/iio/light/cros_ec_light_prox.c
> > > +++ b/drivers/iio/light/cros_ec_light_prox.c
> > > @@ -161,7 +161,7 @@ static int cros_ec_light_prox_write(struct
> > > iio_dev *indio_dev,
> > >     return ret;
> > >  }
> > >
> > > -static const struct iio_info cros_ec_light_prox_info = {
> > > +static struct iio_info cros_ec_light_prox_info = {
> > >     .read_raw = &cros_ec_light_prox_read,
> > >     .write_raw = &cros_ec_light_prox_write,
> > >  };
> > > @@ -184,11 +184,11 @@ static int cros_ec_light_prox_probe(struct
> > > platform_device *pdev)
> > >     if (!indio_dev)
> > >             return -ENOMEM;
> > >
> > > -   ret = cros_ec_sensors_core_init(pdev, indio_dev, true);
> > > +   ret = cros_ec_sensors_core_init(pdev, indio_dev,
> > > +                                   &cros_ec_light_prox_info,
> > > true);
> > >     if (ret)
> > >             return ret;
> > >
> > > -   indio_dev->info = &cros_ec_light_prox_info;
> > >     state = iio_priv(indio_dev);
> > >     state->core.type = state->core.resp->info.type;
> > >     state->core.loc = state->core.resp->info.location;
> > > diff --git a/drivers/iio/pressure/cros_ec_baro.c
> > > b/drivers/iio/pressure/cros_ec_baro.c
> > > index 034ce98d6e97..cd3be0f16226 100644
> > > --- a/drivers/iio/pressure/cros_ec_baro.c
> > > +++ b/drivers/iio/pressure/cros_ec_baro.c
> > > @@ -107,7 +107,7 @@ static int cros_ec_baro_write(struct iio_dev
> > > *indio_dev,
> > >     return ret;
> > >  }
> > >
> > > -static const struct iio_info cros_ec_baro_info = {
> > > +static struct iio_info cros_ec_baro_info = {
> > >     .read_raw = &cros_ec_baro_read,
> > >     .write_raw = &cros_ec_baro_write,
> > >  };
> > > @@ -130,11 +130,11 @@ static int cros_ec_baro_probe(struct
> > > platform_device *pdev)
> > >     if (!indio_dev)
> > >             return -ENOMEM;
> > >
> > > -   ret = cros_ec_sensors_core_init(pdev, indio_dev, true);
> > > +   ret = cros_ec_sensors_core_init(pdev, indio_dev,
> > > &cros_ec_baro_info,
> > > +                                   true);
> > >     if (ret)
> > >             return ret;
> > >
> > > -   indio_dev->info = &cros_ec_baro_info;
> > >     state = iio_priv(indio_dev);
> > >     state->core.type = state->core.resp->info.type;
> > >     state->core.loc = state->core.resp->info.location;
> > > diff --git a/include/linux/iio/common/cros_ec_sensors_core.h
> > > b/include/linux/iio/common/cros_ec_sensors_core.h
> > > index 32fd08bbcf52..f170a72ac08d 100644
> > > --- a/include/linux/iio/common/cros_ec_sensors_core.h
> > > +++ b/include/linux/iio/common/cros_ec_sensors_core.h
> > > @@ -114,12 +114,14 @@ struct platform_device;
> > >   * cros_ec_sensors_core_init() - basic initialization of the core
> > > structure
> > >   * @pdev:          platform device created for the sensors
> > >   * @indio_dev:             iio device structure of the device
> > > + * @info:          iio info structure with read and write callback
> > >   * @physical_device:       true if the device refers to a physical
> > > device
> > >   *
> > >   * Return: 0 on success, -errno on failure.
> > >   */
> > >  int cros_ec_sensors_core_init(struct platform_device *pdev,
> > > -                         struct iio_dev *indio_dev, bool
> > > physical_device);
> > > +                         struct iio_dev *indio_dev, struct
> > > iio_info *info,
> > > +                         bool physical_device);
> > >
> > >  /**
> > >   * cros_ec_sensors_capture() - the trigger handler function
>

Re: [PATCH v2 2/3] iio: common: cros_ec_sensors: add sysfs attribute for frequencies

[Fabien Lahoudere]

Le mardi 28 mai 2019 à 04:04 -0700, Gwendal Grignou a écrit :
> On Mon, May 27, 2019 at 2:55 AM Fabien Lahoudere
> <fabien.lahoudere@collabora.com> wrote:
> > Le dimanche 26 mai 2019 à 18:45 +0100, Jonathan Cameron a écrit :
> > > On Thu, 23 May 2019 11:07:36 +0200
> > > Fabien Lahoudere <fabien.lahoudere@collabora.com> wrote:
> > > 
> > > > In order to provide minimum and maximum frequencies for each
> > > > sensors,
> > > > we use a standard API (sampling_frequency_available) to provide
> > > > them
> > > > to userland.
> > > > As cros_ec_sensors_core_init do not manage default attrs, we
> > > > change
> > > > the signature to let all kind of sensors to provide "struct
> > > > iio_info"
> > > > with their callback. This change impact drivers using that
> > > > function.
> > > > 
> > > > Then cros_ec_* sensors provides frequencies range in sysfs like
> > > > this:
> > > > [min step max]
> > > > 
> > > > Signed-off-by: Fabien Lahoudere <fabien.lahoudere@collabora.com
> > > > >
> > > When I was pointing at the _available syntax I was meaning that
> > > the ideal is to implement this using the associated callbacks
> > > rather
> > > than as a custom sysfs attribute.
> > > 
> > 
> > Sorry, I misunderstood. Let me retry with that callback
> > implemented.
> > 
> > > > ---
> > > >  .../common/cros_ec_sensors/cros_ec_sensors.c  |  6 +--
> > > >  .../cros_ec_sensors/cros_ec_sensors_core.c    | 38
> > > > +++++++++++++++++++
> > > >  drivers/iio/light/cros_ec_light_prox.c        |  6 +--
> > > >  drivers/iio/pressure/cros_ec_baro.c           |  6 +--
> > > >  .../linux/iio/common/cros_ec_sensors_core.h   |  4 +-
> > > >  5 files changed, 50 insertions(+), 10 deletions(-)
> > > > 
> > > > diff --git
> > > > a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors.c
> > > > b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors.c
> > > > index 17af4e0fd5f8..a0ecee15a6c8 100644
> > > > --- a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors.c
> > > > +++ b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors.c
> > > > @@ -172,7 +172,7 @@ static int cros_ec_sensors_write(struct
> > > > iio_dev
> > > > *indio_dev,
> > > >     return ret;
> > > >  }
> > > > 
> > > > -static const struct iio_info ec_sensors_info = {
> > > > +static struct iio_info ec_sensors_info = {
> > > >     .read_raw = &cros_ec_sensors_read,
> > > >     .write_raw = &cros_ec_sensors_write,
> > > >  };
> > > > @@ -195,11 +195,11 @@ static int cros_ec_sensors_probe(struct
> > > > platform_device *pdev)
> > > >     if (!indio_dev)
> > > >             return -ENOMEM;
> > > > 
> > > > -   ret = cros_ec_sensors_core_init(pdev, indio_dev, true);
> > > > +   ret = cros_ec_sensors_core_init(pdev, indio_dev,
> > > > &ec_sensors_info,
> > > > +                                   true);
> > > >     if (ret)
> > > >             return ret;
> > > > 
> > > > -   indio_dev->info = &ec_sensors_info;
> > > >     state = iio_priv(indio_dev);
> > > >     for (channel = state->channels, i = CROS_EC_SENSOR_X;
> > > >          i < CROS_EC_SENSOR_MAX_AXIS; i++, channel++) {
> > > > 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 ac53ea32c1b1..08fb5d3dc7b5 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
> > > > @@ -10,6 +10,7 @@
> > > >  #include <linux/iio/buffer.h>
> > > >  #include <linux/iio/common/cros_ec_sensors_core.h>
> > > >  #include <linux/iio/iio.h>
> > > > +#include <linux/iio/sysfs.h>
> > > >  #include <linux/iio/kfifo_buf.h>
> > > >  #include <linux/iio/trigger_consumer.h>
> > > >  #include <linux/kernel.h>
> > > > @@ -86,8 +87,42 @@ static int
> > > > cros_ec_get_host_cmd_version_mask(struct cros_ec_device
> > > > *ec_dev,
> > > >     return ret;
> > > >  }
> > > > 
> > > > +/**
> > > > + * cros_ec_sensors_read_freq() - sysfs function to get
> > > > available
> > > > frequencies
> > > > + * @dev: Device structure for this device.
> > > > + * @attr: Description of the attribute.
> > > > + * @buf: Incoming string
> > > > + *
> > > > + * The later modes are only relevant to the ring buffer - and
> > > > depend on current
> > > > + * mode. Note that data sheet gives rather wide tolerances for
> > > > these so integer
> > > > + * division will give good enough answer and not all chips
> > > > have
> > > > them specified
> > > > + * at all.
> > > > + **/
> > > > +static ssize_t cros_ec_sensors_read_freq(struct device *dev,
> > > > +                                    struct device_attribute
> > > > *attr,
> > > > +                                    char *buf)
> > > > +{
> > > > +   struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> > > > +   struct cros_ec_sensors_core_state *state =
> > > > iio_priv(indio_dev);
> > > > +
> > > > +   return snprintf(buf, PAGE_SIZE, "[%d 1 %d]\n", state-
> > > > >min_freq,
> > > > +                   state->max_freq);
> Step to 1 [Hz?] is not right; EC uses the frequency the sensors
> supports, rounded up by default.
> Given EC is hiding the sensors it controls, it is not straight-
> forward
> to find the supported frequencies between min_freq and max_freq.
> However, sensors mostly follow the following rules:
> - "fast" sensors: accelerometer, gyroscope, magnetometer,  barometer:
> Supported frequencies are: [ min_freq, max_freq >> n, max_freq >> (n
> -
> 1), ... max_freq >> 1, max_freq], where (min_freq >> n) > min_freq.
> frequency are expressed in mHz resolution.
> - "Slow" sensors: light, proximity:
> Any frequencies between min_freq and max_freq are supported.
> 
> Note that frequency 0 is accepted, it puts the given sensor in
> suspend mode.
> 
> Given that information, it would make sense to follow the existing
> macro IIO_DEV_ATTR_SAMP_FREQ_AVAIL and report an array of
> frequencies,
> like bmc150_magn_show_samp_freq_avail does.

Thanks Gwendal.
IIUC, it is wroing to give a range and better to give a set of value.
Where can i find all available values for each sensors?


> 
> Gwendal.
> 
> > > Whilst it is a bit more fiddly I would much prefer if this was
> > > done
> > > with
> > > the info_mask_shared_by_all_available bit mask in the iio_dev and
> > > providing
> > > the read_avail callback.
> > > 
> > > The original reason to introduce this form was as part of trying
> > > to
> > > (far too slowly) kill off as much hand defined ABI as possible.
> > > Ultimate aim is to make the IIO interface optional for cases
> > > where
> > > the channels are mostly being used by other consumer drivers
> > > rather
> > > than
> > > being directly consumed by userspace.  To do that we need all of
> > > these elements to be easily accessible from the consumer hooks.
> > > 
> > > 
> > > 
> > > > +}
> > > > +
> > > > +static
> > > > IIO_DEV_ATTR_SAMP_FREQ_AVAIL(cros_ec_sensors_read_freq);
> > > > +
> > > > +static struct attribute *cros_ec_sensors_attributes[] = {
> > > > +   &iio_dev_attr_sampling_frequency_available.dev_attr.attr,
> > > > +   NULL,
> > > > +};
> > > > +
> > > > +static const struct attribute_group
> > > > cros_ec_sensors_attribute_group = {
> > > > +   .attrs = cros_ec_sensors_attributes,
> > > > +};
> > > > +
> > > >  int cros_ec_sensors_core_init(struct platform_device *pdev,
> > > >                           struct iio_dev *indio_dev,
> > > > +                         struct iio_info *info,
> > > >                           bool physical_device)
> > > >  {
> > > >     struct device *dev = &pdev->dev;
> > > > @@ -149,6 +184,9 @@ int cros_ec_sensors_core_init(struct
> > > > platform_device *pdev,
> > > >             }
> > > >     }
> > > > 
> > > > +   info->attrs = &cros_ec_sensors_attribute_group;
> > > > +   indio_dev->info = info;
> > > > +
> > > >     return 0;
> > > >  }
> > > >  EXPORT_SYMBOL_GPL(cros_ec_sensors_core_init);
> > > > diff --git a/drivers/iio/light/cros_ec_light_prox.c
> > > > b/drivers/iio/light/cros_ec_light_prox.c
> > > > index 308ee6ff2e22..1772e339cf14 100644
> > > > --- a/drivers/iio/light/cros_ec_light_prox.c
> > > > +++ b/drivers/iio/light/cros_ec_light_prox.c
> > > > @@ -161,7 +161,7 @@ static int cros_ec_light_prox_write(struct
> > > > iio_dev *indio_dev,
> > > >     return ret;
> > > >  }
> > > > 
> > > > -static const struct iio_info cros_ec_light_prox_info = {
> > > > +static struct iio_info cros_ec_light_prox_info = {
> > > >     .read_raw = &cros_ec_light_prox_read,
> > > >     .write_raw = &cros_ec_light_prox_write,
> > > >  };
> > > > @@ -184,11 +184,11 @@ static int
> > > > cros_ec_light_prox_probe(struct
> > > > platform_device *pdev)
> > > >     if (!indio_dev)
> > > >             return -ENOMEM;
> > > > 
> > > > -   ret = cros_ec_sensors_core_init(pdev, indio_dev, true);
> > > > +   ret = cros_ec_sensors_core_init(pdev, indio_dev,
> > > > +                                   &cros_ec_light_prox_info,
> > > > true);
> > > >     if (ret)
> > > >             return ret;
> > > > 
> > > > -   indio_dev->info = &cros_ec_light_prox_info;
> > > >     state = iio_priv(indio_dev);
> > > >     state->core.type = state->core.resp->info.type;
> > > >     state->core.loc = state->core.resp->info.location;
> > > > diff --git a/drivers/iio/pressure/cros_ec_baro.c
> > > > b/drivers/iio/pressure/cros_ec_baro.c
> > > > index 034ce98d6e97..cd3be0f16226 100644
> > > > --- a/drivers/iio/pressure/cros_ec_baro.c
> > > > +++ b/drivers/iio/pressure/cros_ec_baro.c
> > > > @@ -107,7 +107,7 @@ static int cros_ec_baro_write(struct
> > > > iio_dev
> > > > *indio_dev,
> > > >     return ret;
> > > >  }
> > > > 
> > > > -static const struct iio_info cros_ec_baro_info = {
> > > > +static struct iio_info cros_ec_baro_info = {
> > > >     .read_raw = &cros_ec_baro_read,
> > > >     .write_raw = &cros_ec_baro_write,
> > > >  };
> > > > @@ -130,11 +130,11 @@ static int cros_ec_baro_probe(struct
> > > > platform_device *pdev)
> > > >     if (!indio_dev)
> > > >             return -ENOMEM;
> > > > 
> > > > -   ret = cros_ec_sensors_core_init(pdev, indio_dev, true);
> > > > +   ret = cros_ec_sensors_core_init(pdev, indio_dev,
> > > > &cros_ec_baro_info,
> > > > +                                   true);
> > > >     if (ret)
> > > >             return ret;
> > > > 
> > > > -   indio_dev->info = &cros_ec_baro_info;
> > > >     state = iio_priv(indio_dev);
> > > >     state->core.type = state->core.resp->info.type;
> > > >     state->core.loc = state->core.resp->info.location;
> > > > diff --git a/include/linux/iio/common/cros_ec_sensors_core.h
> > > > b/include/linux/iio/common/cros_ec_sensors_core.h
> > > > index 32fd08bbcf52..f170a72ac08d 100644
> > > > --- a/include/linux/iio/common/cros_ec_sensors_core.h
> > > > +++ b/include/linux/iio/common/cros_ec_sensors_core.h
> > > > @@ -114,12 +114,14 @@ struct platform_device;
> > > >   * cros_ec_sensors_core_init() - basic initialization of the
> > > > core
> > > > structure
> > > >   * @pdev:          platform device created for the sensors
> > > >   * @indio_dev:             iio device structure of the device
> > > > + * @info:          iio info structure with read and write
> > > > callback
> > > >   * @physical_device:       true if the device refers to a
> > > > physical
> > > > device
> > > >   *
> > > >   * Return: 0 on success, -errno on failure.
> > > >   */
> > > >  int cros_ec_sensors_core_init(struct platform_device *pdev,
> > > > -                         struct iio_dev *indio_dev, bool
> > > > physical_device);
> > > > +                         struct iio_dev *indio_dev, struct
> > > > iio_info *info,
> > > > +                         bool physical_device);
> > > > 
> > > >  /**
> > > >   * cros_ec_sensors_capture() - the trigger handler function

[PATCH v2 3/3] docs: iio: add precision about sampling_frequency_available

[Fabien Lahoudere]

The documentation give some exemple on what format can be expected
from sampling_frequency_available sysfs attribute

Signed-off-by: Fabien Lahoudere <fabien.lahoudere@collabora.com>
---
 Documentation/ABI/testing/sysfs-bus-iio | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
index 6aef7dbbde44..680451695422 100644
--- a/Documentation/ABI/testing/sysfs-bus-iio
+++ b/Documentation/ABI/testing/sysfs-bus-iio
@@ -61,8 +61,11 @@ What:		/sys/bus/iio/devices/triggerX/sampling_frequency_available
 KernelVersion:	2.6.35
 Contact:	linux-iio@vger.kernel.org
 Description:
-		When the internal sampling clock can only take a small
-		discrete set of values, this file lists those available.
+		When the internal sampling clock can only take a specific set of
+		frequencies, we can specify the available values with:
+		- a small discrete set of values like "0 2 4 6 8"
+		- a range with minimum, step and maximum frequencies like
+		  "[min step max]"
 
 What:		/sys/bus/iio/devices/iio:deviceX/oversampling_ratio
 KernelVersion:	2.6.38
-- 
2.20.1

Re: [PATCH v2 3/3] docs: iio: add precision about sampling_frequency_available

[Jonathan Cameron]

On Thu, 23 May 2019 11:07:37 +0200
Fabien Lahoudere <fabien.lahoudere@collabora.com> wrote:

> The documentation give some exemple on what format can be expected
> from sampling_frequency_available sysfs attribute
> 
> Signed-off-by: Fabien Lahoudere <fabien.lahoudere@collabora.com>
Great.

Applied to the togreg branch of iio.git and pushed out as testing for
the autobuilders to completely ignore ;)

Thanks,

Jonathan

> ---
>  Documentation/ABI/testing/sysfs-bus-iio | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
> index 6aef7dbbde44..680451695422 100644
> --- a/Documentation/ABI/testing/sysfs-bus-iio
> +++ b/Documentation/ABI/testing/sysfs-bus-iio
> @@ -61,8 +61,11 @@ What:		/sys/bus/iio/devices/triggerX/sampling_frequency_available
>  KernelVersion:	2.6.35
>  Contact:	linux-iio@vger.kernel.org
>  Description:
> -		When the internal sampling clock can only take a small
> -		discrete set of values, this file lists those available.
> +		When the internal sampling clock can only take a specific set of
> +		frequencies, we can specify the available values with:
> +		- a small discrete set of values like "0 2 4 6 8"
> +		- a range with minimum, step and maximum frequencies like
> +		  "[min step max]"
>  
>  What:		/sys/bus/iio/devices/iio:deviceX/oversampling_ratio
>  KernelVersion:	2.6.38