[PATCH v3 0/8] Expose cros_ec_sensors frequency range via iio sysfs

[PATCH v3 0/8] 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 v2:

- use read_avail callback
- rework core functions to avoid code duplication

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

Fabien Lahoudere (8):
  iio: common: cros_ec_sensors: move iio_info management to core
  iio: common: cros_ec_sensors: move channels to core structure
  iio: common: cros_ec_sensors: move registration to core
  iio: common: cros_ec_sensors: clean code
  iio: common: cros_ec_sensors: use core structure
  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  | 148 +++++------
 .../cros_ec_sensors/cros_ec_sensors_core.c    | 230 +++++++++++++++---
 drivers/iio/light/cros_ec_light_prox.c        | 124 ++++------
 drivers/iio/pressure/cros_ec_baro.c           | 101 +++-----
 .../linux/iio/common/cros_ec_sensors_core.h   |  43 +++-
 6 files changed, 357 insertions(+), 296 deletions(-)

-- 
2.20.1

[PATCH v3 1/8] iio: common: cros_ec_sensors: move iio_info management to core

[Fabien Lahoudere]

In order to avoid code duplication and make future works easier,
we add an iio_info structure to cros_ec_sensors_core_state structure.

Signed-off-by: Fabien Lahoudere <fabien.lahoudere@collabora.com>
---
 drivers/iio/common/cros_ec_sensors/cros_ec_sensors.c      | 8 ++------
 drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c | 2 ++
 drivers/iio/light/cros_ec_light_prox.c                    | 8 ++------
 drivers/iio/pressure/cros_ec_baro.c                       | 8 ++------
 include/linux/iio/common/cros_ec_sensors_core.h           | 1 +
 5 files changed, 9 insertions(+), 18 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..c59b0ab8fe7d 100644
--- a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors.c
+++ b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors.c
@@ -172,11 +172,6 @@ static int cros_ec_sensors_write(struct iio_dev *indio_dev,
 	return ret;
 }
 
-static const struct iio_info ec_sensors_info = {
-	.read_raw = &cros_ec_sensors_read,
-	.write_raw = &cros_ec_sensors_write,
-};
-
 static int cros_ec_sensors_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -199,7 +194,6 @@ static int cros_ec_sensors_probe(struct platform_device *pdev)
 	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++) {
@@ -235,6 +229,8 @@ static int cros_ec_sensors_probe(struct platform_device *pdev)
 			return -EINVAL;
 		}
 	}
+	state->core.info.read_raw = &cros_ec_sensors_read;
+	state->core.info.write_raw = &cros_ec_sensors_write;
 
 	/* Timestamp */
 	channel->type = IIO_TIMESTAMP;
diff --git a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c
index 130362ca421b..a1b324e1a5d8 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
@@ -68,6 +68,8 @@ int cros_ec_sensors_core_init(struct platform_device *pdev,
 		state->loc = state->resp->info.location;
 	}
 
+	indio_dev->info = &state->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..b319d95fb70f 100644
--- a/drivers/iio/light/cros_ec_light_prox.c
+++ b/drivers/iio/light/cros_ec_light_prox.c
@@ -161,11 +161,6 @@ static int cros_ec_light_prox_write(struct iio_dev *indio_dev,
 	return ret;
 }
 
-static const struct iio_info cros_ec_light_prox_info = {
-	.read_raw = &cros_ec_light_prox_read,
-	.write_raw = &cros_ec_light_prox_write,
-};
-
 static int cros_ec_light_prox_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -188,10 +183,11 @@ static int cros_ec_light_prox_probe(struct platform_device *pdev)
 	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;
+	state->core.info.read_raw = &cros_ec_light_prox_read;
+	state->core.info.write_raw = &cros_ec_light_prox_write;
 	channel = state->channels;
 
 	/* Common part */
diff --git a/drivers/iio/pressure/cros_ec_baro.c b/drivers/iio/pressure/cros_ec_baro.c
index 034ce98d6e97..85a4864e3a4e 100644
--- a/drivers/iio/pressure/cros_ec_baro.c
+++ b/drivers/iio/pressure/cros_ec_baro.c
@@ -107,11 +107,6 @@ static int cros_ec_baro_write(struct iio_dev *indio_dev,
 	return ret;
 }
 
-static const struct iio_info cros_ec_baro_info = {
-	.read_raw = &cros_ec_baro_read,
-	.write_raw = &cros_ec_baro_write,
-};
-
 static int cros_ec_baro_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -134,10 +129,11 @@ static int cros_ec_baro_probe(struct platform_device *pdev)
 	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;
+	state->core.info.read_raw = &cros_ec_baro_read;
+	state->core.info.write_raw = &cros_ec_baro_write;
 	channel = state->channels;
 	/* Common part */
 	channel->info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
diff --git a/include/linux/iio/common/cros_ec_sensors_core.h b/include/linux/iio/common/cros_ec_sensors_core.h
index 0c636b9fe8d7..a729e667f760 100644
--- a/include/linux/iio/common/cros_ec_sensors_core.h
+++ b/include/linux/iio/common/cros_ec_sensors_core.h
@@ -70,6 +70,7 @@ struct cros_ec_sensors_core_state {
 				    unsigned long scan_mask, s16 *data);
 
 	int curr_sampl_freq;
+	struct iio_info info;
 };
 
 /**
-- 
2.20.1

[PATCH v3 2/8] iio: common: cros_ec_sensors: move channels to core structure

[Fabien Lahoudere]

To avoid code duplication, we move channels initialization in
cros_ec_sensors_core.

Signed-off-by: Fabien Lahoudere <fabien.lahoudere@collabora.com>
---
 .../common/cros_ec_sensors/cros_ec_sensors.c  | 49 ++++++-------------
 .../cros_ec_sensors/cros_ec_sensors_core.c    | 25 +++++++++-
 drivers/iio/light/cros_ec_light_prox.c        | 42 ++++------------
 drivers/iio/pressure/cros_ec_baro.c           | 38 +++-----------
 .../linux/iio/common/cros_ec_sensors_core.h   | 24 ++++++++-
 5 files changed, 78 insertions(+), 100 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 c59b0ab8fe7d..897dc83a3355 100644
--- a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors.c
+++ b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors.c
@@ -28,8 +28,6 @@
 struct cros_ec_sensors_state {
 	/* Shared by all sensors */
 	struct cros_ec_sensors_core_state core;
-
-	struct iio_chan_spec channels[CROS_EC_SENSORS_MAX_CHANNELS];
 };
 
 static int cros_ec_sensors_read(struct iio_dev *indio_dev,
@@ -178,7 +176,6 @@ static int cros_ec_sensors_probe(struct platform_device *pdev)
 	struct cros_ec_dev *ec_dev = dev_get_drvdata(dev->parent);
 	struct iio_dev *indio_dev;
 	struct cros_ec_sensors_state *state;
-	struct iio_chan_spec *channel;
 	int ret, i;
 
 	if (!ec_dev || !ec_dev->ec_dev) {
@@ -186,63 +183,49 @@ static int cros_ec_sensors_probe(struct platform_device *pdev)
 		return -EINVAL;
 	}
 
-	indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*state));
-	if (!indio_dev)
-		return -ENOMEM;
-
-	ret = cros_ec_sensors_core_init(pdev, indio_dev, true);
+	ret = cros_ec_sensors_core_init(pdev, sizeof(*state),
+					CROS_EC_SENSORS_MAX_CHANNELS, true);
 	if (ret)
 		return ret;
 
+	indio_dev = platform_get_drvdata(pdev);
 	state = iio_priv(indio_dev);
-	for (channel = state->channels, i = CROS_EC_SENSOR_X;
-	     i < CROS_EC_SENSOR_MAX_AXIS; i++, channel++) {
+	for (i = CROS_EC_SENSOR_X; i < CROS_EC_SENSOR_MAX_AXIS; i++) {
 		/* Common part */
-		channel->info_mask_separate =
+		cros_ec_core_channel_init(state->core.channels, i + 1);
+		state->core.channels[i + 1].info_mask_separate =
 			BIT(IIO_CHAN_INFO_RAW) |
 			BIT(IIO_CHAN_INFO_CALIBBIAS);
-		channel->info_mask_shared_by_all =
+		state->core.channels[i + 1].info_mask_shared_by_all =
 			BIT(IIO_CHAN_INFO_SCALE) |
 			BIT(IIO_CHAN_INFO_FREQUENCY) |
 			BIT(IIO_CHAN_INFO_SAMP_FREQ);
-		channel->scan_type.realbits = CROS_EC_SENSOR_BITS;
-		channel->scan_type.storagebits = CROS_EC_SENSOR_BITS;
-		channel->scan_index = i;
-		channel->ext_info = cros_ec_sensors_ext_info;
-		channel->modified = 1;
-		channel->channel2 = IIO_MOD_X + i;
-		channel->scan_type.sign = 's';
+		state->core.channels[i + 1].scan_index = i;
+		state->core.channels[i + 1].modified = 1;
+		state->core.channels[i + 1].channel2 = IIO_MOD_X + i;
+		state->core.channels[i + 1].scan_type.sign = 's';
 
 		/* Sensor specific */
 		switch (state->core.type) {
 		case MOTIONSENSE_TYPE_ACCEL:
-			channel->type = IIO_ACCEL;
+			state->core.channels[i + 1].type = IIO_ACCEL;
 			break;
 		case MOTIONSENSE_TYPE_GYRO:
-			channel->type = IIO_ANGL_VEL;
+			state->core.channels[i + 1].type = IIO_ANGL_VEL;
 			break;
 		case MOTIONSENSE_TYPE_MAG:
-			channel->type = IIO_MAGN;
+			state->core.channels[i + 1].type = IIO_MAGN;
 			break;
 		default:
 			dev_err(&pdev->dev, "Unknown motion sensor\n");
 			return -EINVAL;
 		}
 	}
+	state->core.channels[0].scan_index = CROS_EC_SENSOR_MAX_AXIS;
+
 	state->core.info.read_raw = &cros_ec_sensors_read;
 	state->core.info.write_raw = &cros_ec_sensors_write;
 
-	/* Timestamp */
-	channel->type = IIO_TIMESTAMP;
-	channel->channel = -1;
-	channel->scan_index = CROS_EC_SENSOR_MAX_AXIS;
-	channel->scan_type.sign = 's';
-	channel->scan_type.realbits = 64;
-	channel->scan_type.storagebits = 64;
-
-	indio_dev->channels = state->channels;
-	indio_dev->num_channels = CROS_EC_SENSORS_MAX_CHANNELS;
-
 	/* There is only enough room for accel and gyro in the io space */
 	if ((state->core.ec->cmd_readmem != NULL) &&
 	    (state->core.type != MOTIONSENSE_TYPE_MAG))
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 a1b324e1a5d8..e5181e007dd7 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
@@ -26,15 +26,25 @@ static char *cros_ec_loc[] = {
 };
 
 int cros_ec_sensors_core_init(struct platform_device *pdev,
-			      struct iio_dev *indio_dev,
+			      int sizeof_priv,
+			      int num_channels,
 			      bool physical_device)
 {
 	struct device *dev = &pdev->dev;
-	struct cros_ec_sensors_core_state *state = iio_priv(indio_dev);
+	struct cros_ec_sensors_core_state *state;
 	struct cros_ec_dev *ec = dev_get_drvdata(pdev->dev.parent);
 	struct cros_ec_sensor_platform *sensor_platform = dev_get_platdata(dev);
+	struct iio_dev *indio_dev;
+
+	if (num_channels > CROS_EC_SENSORS_CORE_MAX_CHANNELS)
+		return -EINVAL;
+
+	indio_dev = devm_iio_device_alloc(dev, sizeof_priv);
+	if (!indio_dev)
+		return -ENOMEM;
 
 	platform_set_drvdata(pdev, indio_dev);
+	state = iio_priv(indio_dev);
 
 	state->ec = ec->ec_dev;
 	state->msg = devm_kzalloc(&pdev->dev,
@@ -70,6 +80,17 @@ int cros_ec_sensors_core_init(struct platform_device *pdev,
 
 	indio_dev->info = &state->info;
 
+	/* Timestamp channel */
+	state->channels[0].type = IIO_TIMESTAMP;
+	state->channels[0].channel = -1;
+	state->channels[0].scan_index = 0;
+	state->channels[0].scan_type.sign = 's';
+	state->channels[0].scan_type.realbits = 64;
+	state->channels[0].scan_type.storagebits = 64;
+
+	indio_dev->channels = state->channels;
+	indio_dev->num_channels = num_channels;
+
 	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 b319d95fb70f..32ea5afd495f 100644
--- a/drivers/iio/light/cros_ec_light_prox.c
+++ b/drivers/iio/light/cros_ec_light_prox.c
@@ -31,8 +31,6 @@
 struct cros_ec_light_prox_state {
 	/* Shared by all sensors */
 	struct cros_ec_sensors_core_state core;
-
-	struct iio_chan_spec channels[CROS_EC_LIGHT_PROX_MAX_CHANNELS];
 };
 
 static int cros_ec_light_prox_read(struct iio_dev *indio_dev,
@@ -167,7 +165,6 @@ static int cros_ec_light_prox_probe(struct platform_device *pdev)
 	struct cros_ec_dev *ec_dev = dev_get_drvdata(dev->parent);
 	struct iio_dev *indio_dev;
 	struct cros_ec_light_prox_state *state;
-	struct iio_chan_spec *channel;
 	int ret;
 
 	if (!ec_dev || !ec_dev->ec_dev) {
@@ -175,46 +172,38 @@ static int cros_ec_light_prox_probe(struct platform_device *pdev)
 		return -EINVAL;
 	}
 
-	indio_dev = devm_iio_device_alloc(dev, sizeof(*state));
-	if (!indio_dev)
-		return -ENOMEM;
-
-	ret = cros_ec_sensors_core_init(pdev, indio_dev, true);
+	ret = cros_ec_sensors_core_init(pdev, sizeof(*state),
+					CROS_EC_LIGHT_PROX_MAX_CHANNELS, true);
 	if (ret)
 		return ret;
 
+	indio_dev = platform_get_drvdata(pdev);
 	state = iio_priv(indio_dev);
 	state->core.type = state->core.resp->info.type;
 	state->core.loc = state->core.resp->info.location;
 	state->core.info.read_raw = &cros_ec_light_prox_read;
 	state->core.info.write_raw = &cros_ec_light_prox_write;
-	channel = state->channels;
 
 	/* Common part */
-	channel->info_mask_shared_by_all =
+	cros_ec_core_channel_init(state->core.channels, 1);
+	state->core.channels[1].info_mask_shared_by_all =
 		BIT(IIO_CHAN_INFO_SAMP_FREQ) |
 		BIT(IIO_CHAN_INFO_FREQUENCY);
-	channel->scan_type.realbits = CROS_EC_SENSOR_BITS;
-	channel->scan_type.storagebits = CROS_EC_SENSOR_BITS;
-	channel->scan_type.shift = 0;
-	channel->scan_index = 0;
-	channel->ext_info = cros_ec_sensors_ext_info;
-	channel->scan_type.sign = 'u';
 
 	state->core.calib[0] = 0;
 
 	/* Sensor specific */
 	switch (state->core.type) {
 	case MOTIONSENSE_TYPE_LIGHT:
-		channel->type = IIO_LIGHT;
-		channel->info_mask_separate =
+		state->core.channels[1].type = IIO_LIGHT;
+		state->core.channels[1].info_mask_separate =
 			BIT(IIO_CHAN_INFO_PROCESSED) |
 			BIT(IIO_CHAN_INFO_CALIBBIAS) |
 			BIT(IIO_CHAN_INFO_CALIBSCALE);
 		break;
 	case MOTIONSENSE_TYPE_PROX:
-		channel->type = IIO_PROXIMITY;
-		channel->info_mask_separate =
+		state->core.channels[1].type = IIO_PROXIMITY;
+		state->core.channels[1].info_mask_separate =
 			BIT(IIO_CHAN_INFO_RAW) |
 			BIT(IIO_CHAN_INFO_CALIBBIAS) |
 			BIT(IIO_CHAN_INFO_CALIBSCALE);
@@ -224,19 +213,6 @@ static int cros_ec_light_prox_probe(struct platform_device *pdev)
 		return -EINVAL;
 	}
 
-	/* Timestamp */
-	channel++;
-	channel->type = IIO_TIMESTAMP;
-	channel->channel = -1;
-	channel->scan_index = 1;
-	channel->scan_type.sign = 's';
-	channel->scan_type.realbits = 64;
-	channel->scan_type.storagebits = 64;
-
-	indio_dev->channels = state->channels;
-
-	indio_dev->num_channels = CROS_EC_LIGHT_PROX_MAX_CHANNELS;
-
 	state->core.read_ec_sensors_data = cros_ec_sensors_read_cmd;
 
 	ret = devm_iio_triggered_buffer_setup(dev, indio_dev, NULL,
diff --git a/drivers/iio/pressure/cros_ec_baro.c b/drivers/iio/pressure/cros_ec_baro.c
index 85a4864e3a4e..8718036d74d2 100644
--- a/drivers/iio/pressure/cros_ec_baro.c
+++ b/drivers/iio/pressure/cros_ec_baro.c
@@ -29,8 +29,6 @@
 struct cros_ec_baro_state {
 	/* Shared by all sensors */
 	struct cros_ec_sensors_core_state core;
-
-	struct iio_chan_spec channels[CROS_EC_BARO_MAX_CHANNELS];
 };
 
 static int cros_ec_baro_read(struct iio_dev *indio_dev,
@@ -113,7 +111,6 @@ static int cros_ec_baro_probe(struct platform_device *pdev)
 	struct cros_ec_dev *ec_dev = dev_get_drvdata(dev->parent);
 	struct iio_dev *indio_dev;
 	struct cros_ec_baro_state *state;
-	struct iio_chan_spec *channel;
 	int ret;
 
 	if (!ec_dev || !ec_dev->ec_dev) {
@@ -121,57 +118,38 @@ static int cros_ec_baro_probe(struct platform_device *pdev)
 		return -EINVAL;
 	}
 
-	indio_dev = devm_iio_device_alloc(dev, sizeof(*state));
-	if (!indio_dev)
-		return -ENOMEM;
-
-	ret = cros_ec_sensors_core_init(pdev, indio_dev, true);
+	ret = cros_ec_sensors_core_init(pdev, sizeof(*state),
+					CROS_EC_BARO_MAX_CHANNELS, true);
 	if (ret)
 		return ret;
 
+	indio_dev = platform_get_drvdata(pdev);
 	state = iio_priv(indio_dev);
 	state->core.type = state->core.resp->info.type;
 	state->core.loc = state->core.resp->info.location;
 	state->core.info.read_raw = &cros_ec_baro_read;
 	state->core.info.write_raw = &cros_ec_baro_write;
-	channel = state->channels;
+
 	/* Common part */
-	channel->info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
-	channel->info_mask_shared_by_all =
+	cros_ec_core_channel_init(state->core.channels, 1);
+	state->core.channels[1].info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
+	state->core.channels[1].info_mask_shared_by_all =
 		BIT(IIO_CHAN_INFO_SCALE) |
 		BIT(IIO_CHAN_INFO_SAMP_FREQ) |
 		BIT(IIO_CHAN_INFO_FREQUENCY);
-	channel->scan_type.realbits = CROS_EC_SENSOR_BITS;
-	channel->scan_type.storagebits = CROS_EC_SENSOR_BITS;
-	channel->scan_type.shift = 0;
-	channel->scan_index = 0;
-	channel->ext_info = cros_ec_sensors_ext_info;
-	channel->scan_type.sign = 'u';
 
 	state->core.calib[0] = 0;
 
 	/* Sensor specific */
 	switch (state->core.type) {
 	case MOTIONSENSE_TYPE_BARO:
-		channel->type = IIO_PRESSURE;
+		state->core.channels[1].type = IIO_PRESSURE;
 		break;
 	default:
 		dev_warn(dev, "Unknown motion sensor\n");
 		return -EINVAL;
 	}
 
-	/* Timestamp */
-	channel++;
-	channel->type = IIO_TIMESTAMP;
-	channel->channel = -1;
-	channel->scan_index = 1;
-	channel->scan_type.sign = 's';
-	channel->scan_type.realbits = 64;
-	channel->scan_type.storagebits = 64;
-
-	indio_dev->channels = state->channels;
-	indio_dev->num_channels = CROS_EC_BARO_MAX_CHANNELS;
-
 	state->core.read_ec_sensors_data = cros_ec_sensors_read_cmd;
 
 	ret = devm_iio_triggered_buffer_setup(dev, indio_dev, NULL,
diff --git a/include/linux/iio/common/cros_ec_sensors_core.h b/include/linux/iio/common/cros_ec_sensors_core.h
index a729e667f760..485c649b421f 100644
--- a/include/linux/iio/common/cros_ec_sensors_core.h
+++ b/include/linux/iio/common/cros_ec_sensors_core.h
@@ -31,6 +31,8 @@ enum {
 /* Minimum sampling period to use when device is suspending */
 #define CROS_EC_MIN_SUSPEND_SAMPLING_FREQUENCY 1000  /* 1 second */
 
+#define CROS_EC_SENSORS_CORE_MAX_CHANNELS 8
+
 /**
  * struct cros_ec_sensors_core_state - state data for EC sensors IIO driver
  * @ec:				cros EC device structure
@@ -71,6 +73,7 @@ struct cros_ec_sensors_core_state {
 
 	int curr_sampl_freq;
 	struct iio_info info;
+	struct iio_chan_spec channels[CROS_EC_SENSORS_CORE_MAX_CHANNELS];
 };
 
 /**
@@ -102,13 +105,15 @@ 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
+ * @sizeof_priv:	size of the private structure
+ * @num_channels:	Number of channel
  * @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);
+			      int sizeof_priv, int num_channels,
+			      bool physical_device);
 
 /**
  * cros_ec_sensors_capture() - the trigger handler function
@@ -124,6 +129,21 @@ int cros_ec_sensors_core_init(struct platform_device *pdev,
  */
 irqreturn_t cros_ec_sensors_capture(int irq, void *p);
 
+/**
+ * cros_ec_core_channel_init() - initialize channel
+ * @channel:		channels table
+ * @idx:		channel index to initialize
+ *
+ * Return: 0 on success, -errno on failure.
+ */
+#define cros_ec_core_channel_init(channel, idx) \
+	channel[idx].scan_type.realbits = CROS_EC_SENSOR_BITS;\
+	channel[idx].scan_type.storagebits = CROS_EC_SENSOR_BITS;\
+	channel[idx].scan_type.shift = 0;\
+	channel[idx].scan_index = idx;\
+	channel[idx].ext_info = cros_ec_sensors_ext_info;\
+	channel[idx].scan_type.sign = 'u';
+
 /**
  * cros_ec_motion_send_host_cmd() - send motion sense host command
  * @st:		pointer to state information for device
-- 
2.20.1

[PATCH v3 3/8] iio: common: cros_ec_sensors: move registration to core

[Fabien Lahoudere]

In order to simplify derivated drivers from cros_ec_sensors_core,
a new core function is created to registered IIO stricture.

Signed-off-by: Fabien Lahoudere <fabien.lahoudere@collabora.com>
---
 .../common/cros_ec_sensors/cros_ec_sensors.c  |  9 +-
 .../cros_ec_sensors/cros_ec_sensors_core.c    | 97 ++++++++++++-------
 drivers/iio/light/cros_ec_light_prox.c        |  7 +-
 drivers/iio/pressure/cros_ec_baro.c           |  7 +-
 .../linux/iio/common/cros_ec_sensors_core.h   | 16 ++-
 5 files changed, 72 insertions(+), 64 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 897dc83a3355..c4bee9265246 100644
--- a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors.c
+++ b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors.c
@@ -14,7 +14,7 @@
 #include <linux/iio/iio.h>
 #include <linux/iio/kfifo_buf.h>
 #include <linux/iio/trigger_consumer.h>
-#include <linux/iio/triggered_buffer.h>
+
 #include <linux/kernel.h>
 #include <linux/mfd/cros_ec.h>
 #include <linux/mfd/cros_ec_commands.h>
@@ -233,12 +233,7 @@ static int cros_ec_sensors_probe(struct platform_device *pdev)
 	else
 		state->core.read_ec_sensors_data = cros_ec_sensors_read_cmd;
 
-	ret = devm_iio_triggered_buffer_setup(dev, indio_dev, NULL,
-			cros_ec_sensors_capture, NULL);
-	if (ret)
-		return ret;
-
-	return devm_iio_device_register(dev, indio_dev);
+	return cros_ec_sensors_core_register(pdev, indio_dev);
 }
 
 static const struct platform_device_id cros_ec_sensors_ids[] = {
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 e5181e007dd7..3880849c5cca 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
@@ -12,6 +12,7 @@
 #include <linux/iio/iio.h>
 #include <linux/iio/kfifo_buf.h>
 #include <linux/iio/trigger_consumer.h>
+#include <linux/iio/triggered_buffer.h>
 #include <linux/kernel.h>
 #include <linux/mfd/cros_ec.h>
 #include <linux/mfd/cros_ec_commands.h>
@@ -95,6 +96,67 @@ int cros_ec_sensors_core_init(struct platform_device *pdev,
 }
 EXPORT_SYMBOL_GPL(cros_ec_sensors_core_init);
 
+/**
+ * cros_ec_sensors_capture() - the trigger handler function
+ * @irq:	the interrupt number.
+ * @p:		a pointer to the poll function.
+ *
+ * On a trigger event occurring, if the pollfunc is attached then this
+ * handler is called as a threaded interrupt (and hence may sleep). It
+ * is responsible for grabbing data from the device and pushing it into
+ * the associated buffer.
+ *
+ * Return: IRQ_HANDLED
+ */
+static irqreturn_t cros_ec_sensors_capture(int irq, void *p)
+{
+	struct iio_poll_func *pf = p;
+	struct iio_dev *indio_dev = pf->indio_dev;
+	struct cros_ec_sensors_core_state *st = iio_priv(indio_dev);
+	int ret;
+
+	mutex_lock(&st->cmd_lock);
+
+	/* Clear capture data. */
+	memset(st->samples, 0, indio_dev->scan_bytes);
+
+	/* Read data based on which channels are enabled in scan mask. */
+	ret = st->read_ec_sensors_data(indio_dev,
+				       *indio_dev->active_scan_mask,
+				       (s16 *)st->samples);
+	if (ret < 0)
+		goto done;
+
+	iio_push_to_buffers_with_timestamp(indio_dev, st->samples,
+					   iio_get_time_ns(indio_dev));
+
+done:
+	/*
+	 * Tell the core we are done with this trigger and ready for the
+	 * next one.
+	 */
+	iio_trigger_notify_done(indio_dev->trig);
+
+	mutex_unlock(&st->cmd_lock);
+
+	return IRQ_HANDLED;
+}
+
+int cros_ec_sensors_core_register(struct platform_device *pdev,
+				  struct iio_dev *indio_dev)
+{
+	int ret;
+	struct device *dev = &pdev->dev;
+
+	ret = devm_iio_triggered_buffer_setup(dev, indio_dev, NULL,
+					      cros_ec_sensors_capture, NULL);
+	if (ret)
+		return ret;
+
+	return devm_iio_device_register(dev, indio_dev);
+}
+EXPORT_SYMBOL_GPL(cros_ec_sensors_core_register);
+
 int cros_ec_motion_send_host_cmd(struct cros_ec_sensors_core_state *state,
 				 u16 opt_length)
 {
@@ -380,41 +442,6 @@ int cros_ec_sensors_read_cmd(struct iio_dev *indio_dev,
 }
 EXPORT_SYMBOL_GPL(cros_ec_sensors_read_cmd);
 
-irqreturn_t cros_ec_sensors_capture(int irq, void *p)
-{
-	struct iio_poll_func *pf = p;
-	struct iio_dev *indio_dev = pf->indio_dev;
-	struct cros_ec_sensors_core_state *st = iio_priv(indio_dev);
-	int ret;
-
-	mutex_lock(&st->cmd_lock);
-
-	/* Clear capture data. */
-	memset(st->samples, 0, indio_dev->scan_bytes);
-
-	/* Read data based on which channels are enabled in scan mask. */
-	ret = st->read_ec_sensors_data(indio_dev,
-				       *(indio_dev->active_scan_mask),
-				       (s16 *)st->samples);
-	if (ret < 0)
-		goto done;
-
-	iio_push_to_buffers_with_timestamp(indio_dev, st->samples,
-					   iio_get_time_ns(indio_dev));
-
-done:
-	/*
-	 * Tell the core we are done with this trigger and ready for the
-	 * next one.
-	 */
-	iio_trigger_notify_done(indio_dev->trig);
-
-	mutex_unlock(&st->cmd_lock);
-
-	return IRQ_HANDLED;
-}
-EXPORT_SYMBOL_GPL(cros_ec_sensors_capture);
-
 int cros_ec_sensors_core_read(struct cros_ec_sensors_core_state *st,
 			  struct iio_chan_spec const *chan,
 			  int *val, int *val2, long mask)
diff --git a/drivers/iio/light/cros_ec_light_prox.c b/drivers/iio/light/cros_ec_light_prox.c
index 32ea5afd495f..682dc19c2bf3 100644
--- a/drivers/iio/light/cros_ec_light_prox.c
+++ b/drivers/iio/light/cros_ec_light_prox.c
@@ -215,12 +215,7 @@ static int cros_ec_light_prox_probe(struct platform_device *pdev)
 
 	state->core.read_ec_sensors_data = cros_ec_sensors_read_cmd;
 
-	ret = devm_iio_triggered_buffer_setup(dev, indio_dev, NULL,
-					      cros_ec_sensors_capture, NULL);
-	if (ret)
-		return ret;
-
-	return devm_iio_device_register(dev, indio_dev);
+	return cros_ec_sensors_core_register(pdev, indio_dev);
 }
 
 static const struct platform_device_id cros_ec_light_prox_ids[] = {
diff --git a/drivers/iio/pressure/cros_ec_baro.c b/drivers/iio/pressure/cros_ec_baro.c
index 8718036d74d2..9d3745bc2fba 100644
--- a/drivers/iio/pressure/cros_ec_baro.c
+++ b/drivers/iio/pressure/cros_ec_baro.c
@@ -152,12 +152,7 @@ static int cros_ec_baro_probe(struct platform_device *pdev)
 
 	state->core.read_ec_sensors_data = cros_ec_sensors_read_cmd;
 
-	ret = devm_iio_triggered_buffer_setup(dev, indio_dev, NULL,
-					      cros_ec_sensors_capture, NULL);
-	if (ret)
-		return ret;
-
-	return devm_iio_device_register(dev, indio_dev);
+	return cros_ec_sensors_core_register(pdev, indio_dev);
 }
 
 static const struct platform_device_id cros_ec_baro_ids[] = {
diff --git a/include/linux/iio/common/cros_ec_sensors_core.h b/include/linux/iio/common/cros_ec_sensors_core.h
index 485c649b421f..60f40d253f4a 100644
--- a/include/linux/iio/common/cros_ec_sensors_core.h
+++ b/include/linux/iio/common/cros_ec_sensors_core.h
@@ -116,18 +116,14 @@ int cros_ec_sensors_core_init(struct platform_device *pdev,
 			      bool physical_device);
 
 /**
- * cros_ec_sensors_capture() - the trigger handler function
- * @irq:	the interrupt number.
- * @p:		a pointer to the poll function.
- *
- * On a trigger event occurring, if the pollfunc is attached then this
- * handler is called as a threaded interrupt (and hence may sleep). It
- * is responsible for grabbing data from the device and pushing it into
- * the associated buffer.
+ * cros_ec_sensors_core_register() - registration of the core structure
+ * @pdev:		platform device created for the sensors
+ * @indio_dev:		iio device structure of the device
  *
- * Return: IRQ_HANDLED
+ * Return: 0 on success, -errno on failure.
  */
-irqreturn_t cros_ec_sensors_capture(int irq, void *p);
+int cros_ec_sensors_core_register(struct platform_device *pdev,
+				  struct iio_dev *indio_dev);
 
 /**
  * cros_ec_core_channel_init() - initialize channel
-- 
2.20.1

[PATCH v3 4/8] iio: common: cros_ec_sensors: clean code

[Fabien Lahoudere]

Drop redundant initialization previously set in cros_ec_sensors_init

Signed-off-by: Fabien Lahoudere <fabien.lahoudere@collabora.com>
---
 drivers/iio/light/cros_ec_light_prox.c | 2 --
 drivers/iio/pressure/cros_ec_baro.c    | 2 --
 2 files changed, 4 deletions(-)

diff --git a/drivers/iio/light/cros_ec_light_prox.c b/drivers/iio/light/cros_ec_light_prox.c
index 682dc19c2bf3..73f5dcbda0d5 100644
--- a/drivers/iio/light/cros_ec_light_prox.c
+++ b/drivers/iio/light/cros_ec_light_prox.c
@@ -179,8 +179,6 @@ static int cros_ec_light_prox_probe(struct platform_device *pdev)
 
 	indio_dev = platform_get_drvdata(pdev);
 	state = iio_priv(indio_dev);
-	state->core.type = state->core.resp->info.type;
-	state->core.loc = state->core.resp->info.location;
 	state->core.info.read_raw = &cros_ec_light_prox_read;
 	state->core.info.write_raw = &cros_ec_light_prox_write;
 
diff --git a/drivers/iio/pressure/cros_ec_baro.c b/drivers/iio/pressure/cros_ec_baro.c
index 9d3745bc2fba..f8107a8b8e72 100644
--- a/drivers/iio/pressure/cros_ec_baro.c
+++ b/drivers/iio/pressure/cros_ec_baro.c
@@ -125,8 +125,6 @@ static int cros_ec_baro_probe(struct platform_device *pdev)
 
 	indio_dev = platform_get_drvdata(pdev);
 	state = iio_priv(indio_dev);
-	state->core.type = state->core.resp->info.type;
-	state->core.loc = state->core.resp->info.location;
 	state->core.info.read_raw = &cros_ec_baro_read;
 	state->core.info.write_raw = &cros_ec_baro_write;
 
-- 
2.20.1

[PATCH v3 5/8] iio: common: cros_ec_sensors: use core structure

[Fabien Lahoudere]

Drivers based on cros_ec_sensors_core have structure containing the
core structure. In order to simplify, we drop all the specific and
useless structure to use the same one in all drivers.
If a future driver need specific field, we can add a private pointer
to that data.

Signed-off-by: Fabien Lahoudere <fabien.lahoudere@collabora.com>
---
 .../common/cros_ec_sensors/cros_ec_sensors.c  | 114 ++++++++----------
 .../cros_ec_sensors/cros_ec_sensors_core.c    |   3 +-
 drivers/iio/light/cros_ec_light_prox.c        |  85 ++++++-------
 drivers/iio/pressure/cros_ec_baro.c           |  62 +++++-----
 .../linux/iio/common/cros_ec_sensors_core.h   |   3 +-
 5 files changed, 121 insertions(+), 146 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 c4bee9265246..1f0d1c614ffc 100644
--- a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors.c
+++ b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors.c
@@ -24,58 +24,52 @@
 
 #define CROS_EC_SENSORS_MAX_CHANNELS 4
 
-/* State data for ec_sensors iio driver. */
-struct cros_ec_sensors_state {
-	/* Shared by all sensors */
-	struct cros_ec_sensors_core_state core;
-};
-
 static int cros_ec_sensors_read(struct iio_dev *indio_dev,
 			  struct iio_chan_spec const *chan,
 			  int *val, int *val2, long mask)
 {
-	struct cros_ec_sensors_state *st = iio_priv(indio_dev);
+	struct cros_ec_sensors_core_state *st = iio_priv(indio_dev);
 	s16 data = 0;
 	s64 val64;
 	int i;
 	int ret;
 	int idx = chan->scan_index;
 
-	mutex_lock(&st->core.cmd_lock);
+	mutex_lock(&st->cmd_lock);
 
 	switch (mask) {
 	case IIO_CHAN_INFO_RAW:
-		ret = st->core.read_ec_sensors_data(indio_dev, 1 << idx, &data);
+		ret = st->read_ec_sensors_data(indio_dev, 1 << idx, &data);
 		if (ret < 0)
 			break;
 		ret = IIO_VAL_INT;
 		*val = data;
 		break;
 	case IIO_CHAN_INFO_CALIBBIAS:
-		st->core.param.cmd = MOTIONSENSE_CMD_SENSOR_OFFSET;
-		st->core.param.sensor_offset.flags = 0;
+		st->param.cmd = MOTIONSENSE_CMD_SENSOR_OFFSET;
+		st->param.sensor_offset.flags = 0;
 
-		ret = cros_ec_motion_send_host_cmd(&st->core, 0);
+		ret = cros_ec_motion_send_host_cmd(st, 0);
 		if (ret < 0)
 			break;
 
 		/* Save values */
 		for (i = CROS_EC_SENSOR_X; i < CROS_EC_SENSOR_MAX_AXIS; i++)
-			st->core.calib[i] =
-				st->core.resp->sensor_offset.offset[i];
+			st->calib[i] =
+				st->resp->sensor_offset.offset[i];
 		ret = IIO_VAL_INT;
-		*val = st->core.calib[idx];
+		*val = st->calib[idx];
 		break;
 	case IIO_CHAN_INFO_SCALE:
-		st->core.param.cmd = MOTIONSENSE_CMD_SENSOR_RANGE;
-		st->core.param.sensor_range.data = EC_MOTION_SENSE_NO_VALUE;
+		st->param.cmd = MOTIONSENSE_CMD_SENSOR_RANGE;
+		st->param.sensor_range.data = EC_MOTION_SENSE_NO_VALUE;
 
-		ret = cros_ec_motion_send_host_cmd(&st->core, 0);
+		ret = cros_ec_motion_send_host_cmd(st, 0);
 		if (ret < 0)
 			break;
 
-		val64 = st->core.resp->sensor_range.ret;
-		switch (st->core.type) {
+		val64 = st->resp->sensor_range.ret;
+		switch (st->type) {
 		case MOTIONSENSE_TYPE_ACCEL:
 			/*
 			 * EC returns data in g, iio exepects m/s^2.
@@ -110,11 +104,10 @@ static int cros_ec_sensors_read(struct iio_dev *indio_dev,
 		}
 		break;
 	default:
-		ret = cros_ec_sensors_core_read(&st->core, chan, val, val2,
-						mask);
+		ret = cros_ec_sensors_core_read(st, chan, val, val2, mask);
 		break;
 	}
-	mutex_unlock(&st->core.cmd_lock);
+	mutex_unlock(&st->cmd_lock);
 
 	return ret;
 }
@@ -123,49 +116,48 @@ static int cros_ec_sensors_write(struct iio_dev *indio_dev,
 			       struct iio_chan_spec const *chan,
 			       int val, int val2, long mask)
 {
-	struct cros_ec_sensors_state *st = iio_priv(indio_dev);
+	struct cros_ec_sensors_core_state *st = iio_priv(indio_dev);
 	int i;
 	int ret;
 	int idx = chan->scan_index;
 
-	mutex_lock(&st->core.cmd_lock);
+	mutex_lock(&st->cmd_lock);
 
 	switch (mask) {
 	case IIO_CHAN_INFO_CALIBBIAS:
-		st->core.calib[idx] = val;
+		st->calib[idx] = val;
 
 		/* Send to EC for each axis, even if not complete */
-		st->core.param.cmd = MOTIONSENSE_CMD_SENSOR_OFFSET;
-		st->core.param.sensor_offset.flags =
+		st->param.cmd = MOTIONSENSE_CMD_SENSOR_OFFSET;
+		st->param.sensor_offset.flags =
 			MOTION_SENSE_SET_OFFSET;
 		for (i = CROS_EC_SENSOR_X; i < CROS_EC_SENSOR_MAX_AXIS; i++)
-			st->core.param.sensor_offset.offset[i] =
-				st->core.calib[i];
-		st->core.param.sensor_offset.temp =
+			st->param.sensor_offset.offset[i] =
+				st->calib[i];
+		st->param.sensor_offset.temp =
 			EC_MOTION_SENSE_INVALID_CALIB_TEMP;
 
-		ret = cros_ec_motion_send_host_cmd(&st->core, 0);
+		ret = cros_ec_motion_send_host_cmd(st, 0);
 		break;
 	case IIO_CHAN_INFO_SCALE:
-		if (st->core.type == MOTIONSENSE_TYPE_MAG) {
+		if (st->type == MOTIONSENSE_TYPE_MAG) {
 			ret = -EINVAL;
 			break;
 		}
-		st->core.param.cmd = MOTIONSENSE_CMD_SENSOR_RANGE;
-		st->core.param.sensor_range.data = val;
+		st->param.cmd = MOTIONSENSE_CMD_SENSOR_RANGE;
+		st->param.sensor_range.data = val;
 
 		/* Always roundup, so caller gets at least what it asks for. */
-		st->core.param.sensor_range.roundup = 1;
+		st->param.sensor_range.roundup = 1;
 
-		ret = cros_ec_motion_send_host_cmd(&st->core, 0);
+		ret = cros_ec_motion_send_host_cmd(st, 0);
 		break;
 	default:
-		ret = cros_ec_sensors_core_write(
-				&st->core, chan, val, val2, mask);
+		ret = cros_ec_sensors_core_write(st, chan, val, val2, mask);
 		break;
 	}
 
-	mutex_unlock(&st->core.cmd_lock);
+	mutex_unlock(&st->cmd_lock);
 
 	return ret;
 }
@@ -175,7 +167,7 @@ static int cros_ec_sensors_probe(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	struct cros_ec_dev *ec_dev = dev_get_drvdata(dev->parent);
 	struct iio_dev *indio_dev;
-	struct cros_ec_sensors_state *state;
+	struct cros_ec_sensors_core_state *state;
 	int ret, i;
 
 	if (!ec_dev || !ec_dev->ec_dev) {
@@ -183,8 +175,8 @@ static int cros_ec_sensors_probe(struct platform_device *pdev)
 		return -EINVAL;
 	}
 
-	ret = cros_ec_sensors_core_init(pdev, sizeof(*state),
-					CROS_EC_SENSORS_MAX_CHANNELS, true);
+	ret = cros_ec_sensors_core_init(pdev, CROS_EC_SENSORS_MAX_CHANNELS,
+					true);
 	if (ret)
 		return ret;
 
@@ -192,46 +184,46 @@ static int cros_ec_sensors_probe(struct platform_device *pdev)
 	state = iio_priv(indio_dev);
 	for (i = CROS_EC_SENSOR_X; i < CROS_EC_SENSOR_MAX_AXIS; i++) {
 		/* Common part */
-		cros_ec_core_channel_init(state->core.channels, i + 1);
-		state->core.channels[i + 1].info_mask_separate =
+		cros_ec_core_channel_init(state->channels, i + 1);
+		state->channels[i + 1].info_mask_separate =
 			BIT(IIO_CHAN_INFO_RAW) |
 			BIT(IIO_CHAN_INFO_CALIBBIAS);
-		state->core.channels[i + 1].info_mask_shared_by_all =
+		state->channels[i + 1].info_mask_shared_by_all =
 			BIT(IIO_CHAN_INFO_SCALE) |
 			BIT(IIO_CHAN_INFO_FREQUENCY) |
 			BIT(IIO_CHAN_INFO_SAMP_FREQ);
-		state->core.channels[i + 1].scan_index = i;
-		state->core.channels[i + 1].modified = 1;
-		state->core.channels[i + 1].channel2 = IIO_MOD_X + i;
-		state->core.channels[i + 1].scan_type.sign = 's';
+		state->channels[i + 1].scan_index = i;
+		state->channels[i + 1].modified = 1;
+		state->channels[i + 1].channel2 = IIO_MOD_X + i;
+		state->channels[i + 1].scan_type.sign = 's';
 
 		/* Sensor specific */
-		switch (state->core.type) {
+		switch (state->type) {
 		case MOTIONSENSE_TYPE_ACCEL:
-			state->core.channels[i + 1].type = IIO_ACCEL;
+			state->channels[i + 1].type = IIO_ACCEL;
 			break;
 		case MOTIONSENSE_TYPE_GYRO:
-			state->core.channels[i + 1].type = IIO_ANGL_VEL;
+			state->channels[i + 1].type = IIO_ANGL_VEL;
 			break;
 		case MOTIONSENSE_TYPE_MAG:
-			state->core.channels[i + 1].type = IIO_MAGN;
+			state->channels[i + 1].type = IIO_MAGN;
 			break;
 		default:
 			dev_err(&pdev->dev, "Unknown motion sensor\n");
 			return -EINVAL;
 		}
 	}
-	state->core.channels[0].scan_index = CROS_EC_SENSOR_MAX_AXIS;
+	state->channels[0].scan_index = CROS_EC_SENSOR_MAX_AXIS;
 
-	state->core.info.read_raw = &cros_ec_sensors_read;
-	state->core.info.write_raw = &cros_ec_sensors_write;
+	state->info.read_raw = &cros_ec_sensors_read;
+	state->info.write_raw = &cros_ec_sensors_write;
 
 	/* There is only enough room for accel and gyro in the io space */
-	if ((state->core.ec->cmd_readmem != NULL) &&
-	    (state->core.type != MOTIONSENSE_TYPE_MAG))
-		state->core.read_ec_sensors_data = cros_ec_sensors_read_lpc;
+	if ((state->ec->cmd_readmem != NULL) &&
+	    (state->type != MOTIONSENSE_TYPE_MAG))
+		state->read_ec_sensors_data = cros_ec_sensors_read_lpc;
 	else
-		state->core.read_ec_sensors_data = cros_ec_sensors_read_cmd;
+		state->read_ec_sensors_data = cros_ec_sensors_read_cmd;
 
 	return cros_ec_sensors_core_register(pdev, indio_dev);
 }
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 3880849c5cca..57034e212fe1 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
@@ -27,7 +27,6 @@ static char *cros_ec_loc[] = {
 };
 
 int cros_ec_sensors_core_init(struct platform_device *pdev,
-			      int sizeof_priv,
 			      int num_channels,
 			      bool physical_device)
 {
@@ -40,7 +39,7 @@ int cros_ec_sensors_core_init(struct platform_device *pdev,
 	if (num_channels > CROS_EC_SENSORS_CORE_MAX_CHANNELS)
 		return -EINVAL;
 
-	indio_dev = devm_iio_device_alloc(dev, sizeof_priv);
+	indio_dev = devm_iio_device_alloc(dev, sizeof(*state));
 	if (!indio_dev)
 		return -ENOMEM;
 
diff --git a/drivers/iio/light/cros_ec_light_prox.c b/drivers/iio/light/cros_ec_light_prox.c
index 73f5dcbda0d5..6e1075dc6458 100644
--- a/drivers/iio/light/cros_ec_light_prox.c
+++ b/drivers/iio/light/cros_ec_light_prox.c
@@ -27,23 +27,17 @@
  */
 #define CROS_EC_LIGHT_PROX_MAX_CHANNELS (1 + 1)
 
-/* State data for ec_sensors iio driver. */
-struct cros_ec_light_prox_state {
-	/* Shared by all sensors */
-	struct cros_ec_sensors_core_state core;
-};
-
 static int cros_ec_light_prox_read(struct iio_dev *indio_dev,
 				   struct iio_chan_spec const *chan,
 				   int *val, int *val2, long mask)
 {
-	struct cros_ec_light_prox_state *st = iio_priv(indio_dev);
+	struct cros_ec_sensors_core_state *st = iio_priv(indio_dev);
 	u16 data = 0;
 	s64 val64;
 	int ret = IIO_VAL_INT;
 	int idx = chan->scan_index;
 
-	mutex_lock(&st->core.cmd_lock);
+	mutex_lock(&st->cmd_lock);
 
 	switch (mask) {
 	case IIO_CHAN_INFO_RAW:
@@ -77,18 +71,18 @@ static int cros_ec_light_prox_read(struct iio_dev *indio_dev,
 		}
 		break;
 	case IIO_CHAN_INFO_CALIBBIAS:
-		st->core.param.cmd = MOTIONSENSE_CMD_SENSOR_OFFSET;
-		st->core.param.sensor_offset.flags = 0;
+		st->param.cmd = MOTIONSENSE_CMD_SENSOR_OFFSET;
+		st->param.sensor_offset.flags = 0;
 
-		if (cros_ec_motion_send_host_cmd(&st->core, 0)) {
+		if (cros_ec_motion_send_host_cmd(st, 0)) {
 			ret = -EIO;
 			break;
 		}
 
 		/* Save values */
-		st->core.calib[0] = st->core.resp->sensor_offset.offset[0];
+		st->calib[0] = st->resp->sensor_offset.offset[0];
 
-		*val = st->core.calib[idx];
+		*val = st->calib[idx];
 		break;
 	case IIO_CHAN_INFO_CALIBSCALE:
 		/*
@@ -96,26 +90,26 @@ static int cros_ec_light_prox_read(struct iio_dev *indio_dev,
 		 * scale is a number x.y, where x is coded on 16 bits,
 		 * y coded on 16 bits, between 0 and 9999.
 		 */
-		st->core.param.cmd = MOTIONSENSE_CMD_SENSOR_RANGE;
-		st->core.param.sensor_range.data = EC_MOTION_SENSE_NO_VALUE;
+		st->param.cmd = MOTIONSENSE_CMD_SENSOR_RANGE;
+		st->param.sensor_range.data = EC_MOTION_SENSE_NO_VALUE;
 
-		if (cros_ec_motion_send_host_cmd(&st->core, 0)) {
+		if (cros_ec_motion_send_host_cmd(st, 0)) {
 			ret = -EIO;
 			break;
 		}
 
-		val64 = st->core.resp->sensor_range.ret;
+		val64 = st->resp->sensor_range.ret;
 		*val = val64 >> 16;
 		*val2 = (val64 & 0xffff) * 100;
 		ret = IIO_VAL_INT_PLUS_MICRO;
 		break;
 	default:
-		ret = cros_ec_sensors_core_read(&st->core, chan, val, val2,
+		ret = cros_ec_sensors_core_read(st, chan, val, val2,
 						mask);
 		break;
 	}
 
-	mutex_unlock(&st->core.cmd_lock);
+	mutex_unlock(&st->cmd_lock);
 
 	return ret;
 }
@@ -124,37 +118,37 @@ static int cros_ec_light_prox_write(struct iio_dev *indio_dev,
 			       struct iio_chan_spec const *chan,
 			       int val, int val2, long mask)
 {
-	struct cros_ec_light_prox_state *st = iio_priv(indio_dev);
+	struct cros_ec_sensors_core_state *st = iio_priv(indio_dev);
 	int ret = 0;
 	int idx = chan->scan_index;
 
-	mutex_lock(&st->core.cmd_lock);
+	mutex_lock(&st->cmd_lock);
 
 	switch (mask) {
 	case IIO_CHAN_INFO_CALIBBIAS:
-		st->core.calib[idx] = val;
+		st->calib[idx] = val;
 		/* Send to EC for each axis, even if not complete */
-		st->core.param.cmd = MOTIONSENSE_CMD_SENSOR_OFFSET;
-		st->core.param.sensor_offset.flags = MOTION_SENSE_SET_OFFSET;
-		st->core.param.sensor_offset.offset[0] = st->core.calib[0];
-		st->core.param.sensor_offset.temp =
+		st->param.cmd = MOTIONSENSE_CMD_SENSOR_OFFSET;
+		st->param.sensor_offset.flags = MOTION_SENSE_SET_OFFSET;
+		st->param.sensor_offset.offset[0] = st->calib[0];
+		st->param.sensor_offset.temp =
 					EC_MOTION_SENSE_INVALID_CALIB_TEMP;
-		if (cros_ec_motion_send_host_cmd(&st->core, 0))
+		if (cros_ec_motion_send_host_cmd(st, 0))
 			ret = -EIO;
 		break;
 	case IIO_CHAN_INFO_CALIBSCALE:
-		st->core.param.cmd = MOTIONSENSE_CMD_SENSOR_RANGE;
-		st->core.param.sensor_range.data = (val << 16) | (val2 / 100);
-		if (cros_ec_motion_send_host_cmd(&st->core, 0))
+		st->param.cmd = MOTIONSENSE_CMD_SENSOR_RANGE;
+		st->param.sensor_range.data = (val << 16) | (val2 / 100);
+		if (cros_ec_motion_send_host_cmd(st, 0))
 			ret = -EIO;
 		break;
 	default:
-		ret = cros_ec_sensors_core_write(&st->core, chan, val, val2,
+		ret = cros_ec_sensors_core_write(st, chan, val, val2,
 						 mask);
 		break;
 	}
 
-	mutex_unlock(&st->core.cmd_lock);
+	mutex_unlock(&st->cmd_lock);
 
 	return ret;
 }
@@ -164,7 +158,7 @@ static int cros_ec_light_prox_probe(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	struct cros_ec_dev *ec_dev = dev_get_drvdata(dev->parent);
 	struct iio_dev *indio_dev;
-	struct cros_ec_light_prox_state *state;
+	struct cros_ec_sensors_core_state *state;
 	int ret;
 
 	if (!ec_dev || !ec_dev->ec_dev) {
@@ -172,36 +166,35 @@ static int cros_ec_light_prox_probe(struct platform_device *pdev)
 		return -EINVAL;
 	}
 
-	ret = cros_ec_sensors_core_init(pdev, sizeof(*state),
-					CROS_EC_LIGHT_PROX_MAX_CHANNELS, true);
+	ret = cros_ec_sensors_core_init(pdev, CROS_EC_LIGHT_PROX_MAX_CHANNELS, true);
 	if (ret)
 		return ret;
 
 	indio_dev = platform_get_drvdata(pdev);
 	state = iio_priv(indio_dev);
-	state->core.info.read_raw = &cros_ec_light_prox_read;
-	state->core.info.write_raw = &cros_ec_light_prox_write;
+	state->info.read_raw = &cros_ec_light_prox_read;
+	state->info.write_raw = &cros_ec_light_prox_write;
 
 	/* Common part */
-	cros_ec_core_channel_init(state->core.channels, 1);
-	state->core.channels[1].info_mask_shared_by_all =
+	cros_ec_core_channel_init(state->channels, 1);
+	state->channels[1].info_mask_shared_by_all =
 		BIT(IIO_CHAN_INFO_SAMP_FREQ) |
 		BIT(IIO_CHAN_INFO_FREQUENCY);
 
-	state->core.calib[0] = 0;
+	state->calib[0] = 0;
 
 	/* Sensor specific */
-	switch (state->core.type) {
+	switch (state->type) {
 	case MOTIONSENSE_TYPE_LIGHT:
-		state->core.channels[1].type = IIO_LIGHT;
-		state->core.channels[1].info_mask_separate =
+		state->channels[1].type = IIO_LIGHT;
+		state->channels[1].info_mask_separate =
 			BIT(IIO_CHAN_INFO_PROCESSED) |
 			BIT(IIO_CHAN_INFO_CALIBBIAS) |
 			BIT(IIO_CHAN_INFO_CALIBSCALE);
 		break;
 	case MOTIONSENSE_TYPE_PROX:
-		state->core.channels[1].type = IIO_PROXIMITY;
-		state->core.channels[1].info_mask_separate =
+		state->channels[1].type = IIO_PROXIMITY;
+		state->channels[1].info_mask_separate =
 			BIT(IIO_CHAN_INFO_RAW) |
 			BIT(IIO_CHAN_INFO_CALIBBIAS) |
 			BIT(IIO_CHAN_INFO_CALIBSCALE);
@@ -211,7 +204,7 @@ static int cros_ec_light_prox_probe(struct platform_device *pdev)
 		return -EINVAL;
 	}
 
-	state->core.read_ec_sensors_data = cros_ec_sensors_read_cmd;
+	state->read_ec_sensors_data = cros_ec_sensors_read_cmd;
 
 	return cros_ec_sensors_core_register(pdev, indio_dev);
 }
diff --git a/drivers/iio/pressure/cros_ec_baro.c b/drivers/iio/pressure/cros_ec_baro.c
index f8107a8b8e72..a2703771a6d1 100644
--- a/drivers/iio/pressure/cros_ec_baro.c
+++ b/drivers/iio/pressure/cros_ec_baro.c
@@ -25,22 +25,16 @@
  */
 #define CROS_EC_BARO_MAX_CHANNELS (1 + 1)
 
-/* State data for ec_sensors iio driver. */
-struct cros_ec_baro_state {
-	/* Shared by all sensors */
-	struct cros_ec_sensors_core_state core;
-};
-
 static int cros_ec_baro_read(struct iio_dev *indio_dev,
 			     struct iio_chan_spec const *chan,
 			     int *val, int *val2, long mask)
 {
-	struct cros_ec_baro_state *st = iio_priv(indio_dev);
+	struct cros_ec_sensors_core_state *st = iio_priv(indio_dev);
 	u16 data = 0;
 	int ret = IIO_VAL_INT;
 	int idx = chan->scan_index;
 
-	mutex_lock(&st->core.cmd_lock);
+	mutex_lock(&st->cmd_lock);
 
 	switch (mask) {
 	case IIO_CHAN_INFO_RAW:
@@ -50,26 +44,26 @@ static int cros_ec_baro_read(struct iio_dev *indio_dev,
 		*val = data;
 		break;
 	case IIO_CHAN_INFO_SCALE:
-		st->core.param.cmd = MOTIONSENSE_CMD_SENSOR_RANGE;
-		st->core.param.sensor_range.data = EC_MOTION_SENSE_NO_VALUE;
+		st->param.cmd = MOTIONSENSE_CMD_SENSOR_RANGE;
+		st->param.sensor_range.data = EC_MOTION_SENSE_NO_VALUE;
 
-		if (cros_ec_motion_send_host_cmd(&st->core, 0)) {
+		if (cros_ec_motion_send_host_cmd(st, 0)) {
 			ret = -EIO;
 			break;
 		}
-		*val = st->core.resp->sensor_range.ret;
+		*val = st->resp->sensor_range.ret;
 
 		/* scale * in_pressure_raw --> kPa */
 		*val2 = 10 << CROS_EC_SENSOR_BITS;
 		ret = IIO_VAL_FRACTIONAL;
 		break;
 	default:
-		ret = cros_ec_sensors_core_read(&st->core, chan, val, val2,
+		ret = cros_ec_sensors_core_read(st, chan, val, val2,
 						mask);
 		break;
 	}
 
-	mutex_unlock(&st->core.cmd_lock);
+	mutex_unlock(&st->cmd_lock);
 
 	return ret;
 }
@@ -78,29 +72,28 @@ static int cros_ec_baro_write(struct iio_dev *indio_dev,
 			      struct iio_chan_spec const *chan,
 			      int val, int val2, long mask)
 {
-	struct cros_ec_baro_state *st = iio_priv(indio_dev);
+	struct cros_ec_sensors_core_state *st = iio_priv(indio_dev);
 	int ret = 0;
 
-	mutex_lock(&st->core.cmd_lock);
+	mutex_lock(&st->cmd_lock);
 
 	switch (mask) {
 	case IIO_CHAN_INFO_SCALE:
-		st->core.param.cmd = MOTIONSENSE_CMD_SENSOR_RANGE;
-		st->core.param.sensor_range.data = val;
+		st->param.cmd = MOTIONSENSE_CMD_SENSOR_RANGE;
+		st->param.sensor_range.data = val;
 
 		/* Always roundup, so caller gets at least what it asks for. */
-		st->core.param.sensor_range.roundup = 1;
+		st->param.sensor_range.roundup = 1;
 
-		if (cros_ec_motion_send_host_cmd(&st->core, 0))
+		if (cros_ec_motion_send_host_cmd(st, 0))
 			ret = -EIO;
 		break;
 	default:
-		ret = cros_ec_sensors_core_write(&st->core, chan, val, val2,
-						 mask);
+		ret = cros_ec_sensors_core_write(st, chan, val, val2, mask);
 		break;
 	}
 
-	mutex_unlock(&st->core.cmd_lock);
+	mutex_unlock(&st->cmd_lock);
 
 	return ret;
 }
@@ -110,7 +103,7 @@ static int cros_ec_baro_probe(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	struct cros_ec_dev *ec_dev = dev_get_drvdata(dev->parent);
 	struct iio_dev *indio_dev;
-	struct cros_ec_baro_state *state;
+	struct cros_ec_sensors_core_state *state;
 	int ret;
 
 	if (!ec_dev || !ec_dev->ec_dev) {
@@ -118,37 +111,36 @@ static int cros_ec_baro_probe(struct platform_device *pdev)
 		return -EINVAL;
 	}
 
-	ret = cros_ec_sensors_core_init(pdev, sizeof(*state),
-					CROS_EC_BARO_MAX_CHANNELS, true);
+	ret = cros_ec_sensors_core_init(pdev, CROS_EC_BARO_MAX_CHANNELS, true);
 	if (ret)
 		return ret;
 
 	indio_dev = platform_get_drvdata(pdev);
 	state = iio_priv(indio_dev);
-	state->core.info.read_raw = &cros_ec_baro_read;
-	state->core.info.write_raw = &cros_ec_baro_write;
+	state->info.read_raw = &cros_ec_baro_read;
+	state->info.write_raw = &cros_ec_baro_write;
 
 	/* Common part */
-	cros_ec_core_channel_init(state->core.channels, 1);
-	state->core.channels[1].info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
-	state->core.channels[1].info_mask_shared_by_all =
+	cros_ec_core_channel_init(state->channels, 1);
+	state->channels[1].info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
+	state->channels[1].info_mask_shared_by_all =
 		BIT(IIO_CHAN_INFO_SCALE) |
 		BIT(IIO_CHAN_INFO_SAMP_FREQ) |
 		BIT(IIO_CHAN_INFO_FREQUENCY);
 
-	state->core.calib[0] = 0;
+	state->calib[0] = 0;
 
 	/* Sensor specific */
-	switch (state->core.type) {
+	switch (state->type) {
 	case MOTIONSENSE_TYPE_BARO:
-		state->core.channels[1].type = IIO_PRESSURE;
+		state->channels[1].type = IIO_PRESSURE;
 		break;
 	default:
 		dev_warn(dev, "Unknown motion sensor\n");
 		return -EINVAL;
 	}
 
-	state->core.read_ec_sensors_data = cros_ec_sensors_read_cmd;
+	state->read_ec_sensors_data = cros_ec_sensors_read_cmd;
 
 	return cros_ec_sensors_core_register(pdev, indio_dev);
 }
diff --git a/include/linux/iio/common/cros_ec_sensors_core.h b/include/linux/iio/common/cros_ec_sensors_core.h
index 60f40d253f4a..3e6de427076e 100644
--- a/include/linux/iio/common/cros_ec_sensors_core.h
+++ b/include/linux/iio/common/cros_ec_sensors_core.h
@@ -105,14 +105,13 @@ struct platform_device;
 /**
  * cros_ec_sensors_core_init() - basic initialization of the core structure
  * @pdev:		platform device created for the sensors
- * @sizeof_priv:	size of the private structure
  * @num_channels:	Number of channel
  * @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,
-			      int sizeof_priv, int num_channels,
+			      int num_channels,
 			      bool physical_device);
 
 /**
-- 
2.20.1

[PATCH v3 6/8] iio: common: cros_ec_sensors: support protocol v3 message

[Fabien Lahoudere]

Version 3 of the EC protocol provides min and max frequencies 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    | 85 ++++++++++++++++++-
 .../linux/iio/common/cros_ec_sensors_core.h   |  3 +
 2 files changed, 87 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 57034e212fe1..2ce077b576a4 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
@@ -26,6 +26,66 @@ 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)
+{
+	int ret;
+	struct {
+		struct cros_ec_command msg;
+		union {
+			struct ec_params_get_cmd_versions params;
+			struct ec_response_get_cmd_versions resp;
+		};
+	} __packed buf = {
+		.msg = {
+			.command = EC_CMD_GET_CMD_VERSIONS + cmd_offset,
+			.insize = sizeof(struct ec_response_get_cmd_versions),
+			.outsize = sizeof(struct ec_params_get_cmd_versions)
+			},
+		.params = {.cmd = cmd}
+	};
+
+	ret = cros_ec_cmd_xfer_status(ec_dev, &buf.msg);
+	if (ret >= 0) {
+		if (buf.msg.result == EC_RES_SUCCESS)
+			*mask = buf.resp.version_mask;
+		else
+			*mask = 0;
+	}
+	return ret;
+}
+
 int cros_ec_sensors_core_init(struct platform_device *pdev,
 			      int num_channels,
 			      bool physical_device)
@@ -35,6 +95,8 @@ int cros_ec_sensors_core_init(struct platform_device *pdev,
 	struct cros_ec_dev *ec = dev_get_drvdata(pdev->dev.parent);
 	struct cros_ec_sensor_platform *sensor_platform = dev_get_platdata(dev);
 	struct iio_dev *indio_dev;
+	u32 ver_mask;
+	int ret;
 
 	if (num_channels > CROS_EC_SENSORS_CORE_MAX_CHANNELS)
 		return -EINVAL;
@@ -57,8 +119,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);
 
@@ -76,6 +146,19 @@ int cros_ec_sensors_core_init(struct platform_device *pdev,
 		}
 		state->type = state->resp->info.type;
 		state->loc = state->resp->info.location;
+
+		/* Value to stop the device */
+		state->frequency_range[0] = 0;
+		if (state->msg->version < 3) {
+			get_default_min_max_freq(state->resp->info.type,
+						 &state->frequency_range[1],
+						 &state->frequency_range[2]);
+		} else {
+			state->frequency_range[1] =
+			    state->resp->info_3.min_frequency;
+			state->frequency_range[2] =
+			    state->resp->info_3.max_frequency;
+		}
 	}
 
 	indio_dev->info = &state->info;
diff --git a/include/linux/iio/common/cros_ec_sensors_core.h b/include/linux/iio/common/cros_ec_sensors_core.h
index 3e6de427076e..89937ad242ef 100644
--- a/include/linux/iio/common/cros_ec_sensors_core.h
+++ b/include/linux/iio/common/cros_ec_sensors_core.h
@@ -74,6 +74,9 @@ struct cros_ec_sensors_core_state {
 	int curr_sampl_freq;
 	struct iio_info info;
 	struct iio_chan_spec channels[CROS_EC_SENSORS_CORE_MAX_CHANNELS];
+
+	/* Disable, Min and Max Sampling Frequency in mHz */
+	int frequency_range[3];
 };
 
 /**
-- 
2.20.1

[PATCH v3 7/8] iio: common: cros_ec_sensors: add sysfs attribute for frequencies

[Fabien Lahoudere]

Embedded controller return minimum and maximum frequencies, unfortunately
we have no way to know the step for all available frequencies.
Even if not complete, we can return a list of known values using the
standard read_avail callback (IIO_CHAN_INFO_SAMP_FREQ) to provide them to
userland.

Now cros_ec_* sensors provides frequencies values in sysfs like this:
"0 min max".

0 is always true to disable the sensor.

Signed-off-by: Fabien Lahoudere <fabien.lahoudere@collabora.com>
---
 .../cros_ec_sensors/cros_ec_sensors_core.c    | 22 +++++++++++++++++++
 .../linux/iio/common/cros_ec_sensors_core.h   |  4 +++-
 2 files changed, 25 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 2ce077b576a4..8df82b675752 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/iio/triggered_buffer.h>
@@ -86,6 +87,26 @@ static int cros_ec_get_host_cmd_version_mask(struct cros_ec_device *ec_dev,
 	return ret;
 }
 
+static int cros_ec_read_avail(struct iio_dev *indio_dev,
+			      struct iio_chan_spec const *chan,
+			      const int **vals,
+			      int *type,
+			      int *length,
+			      long mask)
+{
+	struct cros_ec_sensors_core_state *state = iio_priv(indio_dev);
+
+	switch (mask) {
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		*length = 3;
+		*vals = (const int *)&state->frequency_range;
+		*type = IIO_VAL_INT;
+		return IIO_AVAIL_LIST;
+	}
+
+	return -EINVAL;
+}
+
 int cros_ec_sensors_core_init(struct platform_device *pdev,
 			      int num_channels,
 			      bool physical_device)
@@ -161,6 +182,7 @@ int cros_ec_sensors_core_init(struct platform_device *pdev,
 		}
 	}
 
+	state->info.read_avail = cros_ec_read_avail;
 	indio_dev->info = &state->info;
 
 	/* Timestamp channel */
diff --git a/include/linux/iio/common/cros_ec_sensors_core.h b/include/linux/iio/common/cros_ec_sensors_core.h
index 89937ad242ef..5fa9ba5332e0 100644
--- a/include/linux/iio/common/cros_ec_sensors_core.h
+++ b/include/linux/iio/common/cros_ec_sensors_core.h
@@ -140,7 +140,9 @@ int cros_ec_sensors_core_register(struct platform_device *pdev,
 	channel[idx].scan_type.shift = 0;\
 	channel[idx].scan_index = idx;\
 	channel[idx].ext_info = cros_ec_sensors_ext_info;\
-	channel[idx].scan_type.sign = 'u';
+	channel[idx].scan_type.sign = 'u';\
+	channel[idx].info_mask_shared_by_all_available = \
+		BIT(IIO_CHAN_INFO_SAMP_FREQ);
 
 /**
  * cros_ec_motion_send_host_cmd() - send motion sense host command
-- 
2.20.1

[PATCH v3 8/8] 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 v3 0/8] Expose cros_ec_sensors frequency range via iio sysfs

[Fabien Lahoudere]

Le mardi 18 juin 2019 à 11:06 +0200, Fabien Lahoudere a écrit :
> Chromebooks EC sensors must expose a range of frequencies for each
> sensors using
> the standard ABI sampling_frquency_available.
> 
> Changes since v2:
> 
> - use read_avail callback
> - rework core functions to avoid code duplication

- now sample-frequency-available sysfs return a list of known
frequencies instead of a range, because the step depend on sensors
(fast vs slow) and is hidden by EC.
See https://patchwork.kernel.org/patch/10957141/#22662201

> 
> Changes since v1:
> - Add a cover letter
> - Add Nick Vaccaro SoB to patch 1
> - Drop fifo size related code
> 
> Fabien Lahoudere (8):
>   iio: common: cros_ec_sensors: move iio_info management to core
>   iio: common: cros_ec_sensors: move channels to core structure
>   iio: common: cros_ec_sensors: move registration to core
>   iio: common: cros_ec_sensors: clean code
>   iio: common: cros_ec_sensors: use core structure
>   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  | 148 +++++------
>  .../cros_ec_sensors/cros_ec_sensors_core.c    | 230 +++++++++++++++-
> --
>  drivers/iio/light/cros_ec_light_prox.c        | 124 ++++------
>  drivers/iio/pressure/cros_ec_baro.c           | 101 +++-----
>  .../linux/iio/common/cros_ec_sensors_core.h   |  43 +++-
>  6 files changed, 357 insertions(+), 296 deletions(-)
> 

Re: [PATCH v3 2/8] iio: common: cros_ec_sensors: move channels to core structure

[Jonathan Cameron]

On Tue, 18 Jun 2019 11:06:33 +0200
Fabien Lahoudere <fabien.lahoudere@collabora.com> wrote:

> To avoid code duplication, we move channels initialization in
> cros_ec_sensors_core.
> 
> Signed-off-by: Fabien Lahoudere <fabien.lahoudere@collabora.com>
Hi Fabien,

In of itself a fairly minor gain in code sharing and at some slight
cost in readability. Still on balance worthwhile I think.
Minor stuff inline.

Thanks,

Jonathan

> ---
>  .../common/cros_ec_sensors/cros_ec_sensors.c  | 49 ++++++-------------
>  .../cros_ec_sensors/cros_ec_sensors_core.c    | 25 +++++++++-
>  drivers/iio/light/cros_ec_light_prox.c        | 42 ++++------------
>  drivers/iio/pressure/cros_ec_baro.c           | 38 +++-----------
>  .../linux/iio/common/cros_ec_sensors_core.h   | 24 ++++++++-
>  5 files changed, 78 insertions(+), 100 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 c59b0ab8fe7d..897dc83a3355 100644
> --- a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors.c
> +++ b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors.c
> @@ -28,8 +28,6 @@
>  struct cros_ec_sensors_state {
>  	/* Shared by all sensors */
>  	struct cros_ec_sensors_core_state core;
> -
> -	struct iio_chan_spec channels[CROS_EC_SENSORS_MAX_CHANNELS];
>  };
>  
>  static int cros_ec_sensors_read(struct iio_dev *indio_dev,
> @@ -178,7 +176,6 @@ static int cros_ec_sensors_probe(struct platform_device *pdev)
>  	struct cros_ec_dev *ec_dev = dev_get_drvdata(dev->parent);
>  	struct iio_dev *indio_dev;
>  	struct cros_ec_sensors_state *state;
> -	struct iio_chan_spec *channel;
>  	int ret, i;
>  
>  	if (!ec_dev || !ec_dev->ec_dev) {
> @@ -186,63 +183,49 @@ static int cros_ec_sensors_probe(struct platform_device *pdev)
>  		return -EINVAL;
>  	}
>  
> -	indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*state));
> -	if (!indio_dev)
> -		return -ENOMEM;
> -
> -	ret = cros_ec_sensors_core_init(pdev, indio_dev, true);
> +	ret = cros_ec_sensors_core_init(pdev, sizeof(*state),
> +					CROS_EC_SENSORS_MAX_CHANNELS, true);
>  	if (ret)
>  		return ret;
>  
> +	indio_dev = platform_get_drvdata(pdev);
>  	state = iio_priv(indio_dev);
> -	for (channel = state->channels, i = CROS_EC_SENSOR_X;
> -	     i < CROS_EC_SENSOR_MAX_AXIS; i++, channel++) {
> +	for (i = CROS_EC_SENSOR_X; i < CROS_EC_SENSOR_MAX_AXIS; i++) {
>  		/* Common part */
> -		channel->info_mask_separate =
> +		cros_ec_core_channel_init(state->core.channels, i + 1);
Given this is kind of 'partially' initializing the channel, perhaps
we can name it as something that hints at that.

cros_ec_core_channel_common_init() or something like that?

> +		state->core.channels[i + 1].info_mask_separate =
>  			BIT(IIO_CHAN_INFO_RAW) |
>  			BIT(IIO_CHAN_INFO_CALIBBIAS);
> -		channel->info_mask_shared_by_all =
> +		state->core.channels[i + 1].info_mask_shared_by_all =
>  			BIT(IIO_CHAN_INFO_SCALE) |
>  			BIT(IIO_CHAN_INFO_FREQUENCY) |
>  			BIT(IIO_CHAN_INFO_SAMP_FREQ);
> -		channel->scan_type.realbits = CROS_EC_SENSOR_BITS;
> -		channel->scan_type.storagebits = CROS_EC_SENSOR_BITS;
> -		channel->scan_index = i;
> -		channel->ext_info = cros_ec_sensors_ext_info;
> -		channel->modified = 1;
> -		channel->channel2 = IIO_MOD_X + i;
> -		channel->scan_type.sign = 's';
> +		state->core.channels[i + 1].scan_index = i;
> +		state->core.channels[i + 1].modified = 1;
> +		state->core.channels[i + 1].channel2 = IIO_MOD_X + i;
> +		state->core.channels[i + 1].scan_type.sign = 's';
>  
>  		/* Sensor specific */
>  		switch (state->core.type) {
>  		case MOTIONSENSE_TYPE_ACCEL:
> -			channel->type = IIO_ACCEL;
> +			state->core.channels[i + 1].type = IIO_ACCEL;
>  			break;
>  		case MOTIONSENSE_TYPE_GYRO:
> -			channel->type = IIO_ANGL_VEL;
> +			state->core.channels[i + 1].type = IIO_ANGL_VEL;
>  			break;
>  		case MOTIONSENSE_TYPE_MAG:
> -			channel->type = IIO_MAGN;
> +			state->core.channels[i + 1].type = IIO_MAGN;
>  			break;
>  		default:
>  			dev_err(&pdev->dev, "Unknown motion sensor\n");
>  			return -EINVAL;
>  		}
>  	}
> +	state->core.channels[0].scan_index = CROS_EC_SENSOR_MAX_AXIS;
> +
>  	state->core.info.read_raw = &cros_ec_sensors_read;
>  	state->core.info.write_raw = &cros_ec_sensors_write;
>  
> -	/* Timestamp */
> -	channel->type = IIO_TIMESTAMP;
> -	channel->channel = -1;
> -	channel->scan_index = CROS_EC_SENSOR_MAX_AXIS;
> -	channel->scan_type.sign = 's';
> -	channel->scan_type.realbits = 64;
> -	channel->scan_type.storagebits = 64;
> -
> -	indio_dev->channels = state->channels;
> -	indio_dev->num_channels = CROS_EC_SENSORS_MAX_CHANNELS;
> -
>  	/* There is only enough room for accel and gyro in the io space */
>  	if ((state->core.ec->cmd_readmem != NULL) &&
>  	    (state->core.type != MOTIONSENSE_TYPE_MAG))
> 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 a1b324e1a5d8..e5181e007dd7 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
> @@ -26,15 +26,25 @@ static char *cros_ec_loc[] = {
>  };
>  
>  int cros_ec_sensors_core_init(struct platform_device *pdev,
> -			      struct iio_dev *indio_dev,
> +			      int sizeof_priv,
> +			      int num_channels,
>  			      bool physical_device)
>  {
>  	struct device *dev = &pdev->dev;
> -	struct cros_ec_sensors_core_state *state = iio_priv(indio_dev);
> +	struct cros_ec_sensors_core_state *state;
>  	struct cros_ec_dev *ec = dev_get_drvdata(pdev->dev.parent);
>  	struct cros_ec_sensor_platform *sensor_platform = dev_get_platdata(dev);
> +	struct iio_dev *indio_dev;
> +
> +	if (num_channels > CROS_EC_SENSORS_CORE_MAX_CHANNELS)
> +		return -EINVAL;
> +
> +	indio_dev = devm_iio_device_alloc(dev, sizeof_priv);
> +	if (!indio_dev)
> +		return -ENOMEM;
>  
>  	platform_set_drvdata(pdev, indio_dev);
> +	state = iio_priv(indio_dev);
>  
>  	state->ec = ec->ec_dev;
>  	state->msg = devm_kzalloc(&pdev->dev,
> @@ -70,6 +80,17 @@ int cros_ec_sensors_core_init(struct platform_device *pdev,
>  
>  	indio_dev->info = &state->info;
>  
> +	/* Timestamp channel */
> +	state->channels[0].type = IIO_TIMESTAMP;
> +	state->channels[0].channel = -1;
> +	state->channels[0].scan_index = 0;
> +	state->channels[0].scan_type.sign = 's';
> +	state->channels[0].scan_type.realbits = 64;
> +	state->channels[0].scan_type.storagebits = 64;
> +
> +	indio_dev->channels = state->channels;
> +	indio_dev->num_channels = num_channels;
> +
>  	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 b319d95fb70f..32ea5afd495f 100644
> --- a/drivers/iio/light/cros_ec_light_prox.c
> +++ b/drivers/iio/light/cros_ec_light_prox.c
> @@ -31,8 +31,6 @@
>  struct cros_ec_light_prox_state {
>  	/* Shared by all sensors */
>  	struct cros_ec_sensors_core_state core;
> -
> -	struct iio_chan_spec channels[CROS_EC_LIGHT_PROX_MAX_CHANNELS];
>  };
>  
>  static int cros_ec_light_prox_read(struct iio_dev *indio_dev,
> @@ -167,7 +165,6 @@ static int cros_ec_light_prox_probe(struct platform_device *pdev)
>  	struct cros_ec_dev *ec_dev = dev_get_drvdata(dev->parent);
>  	struct iio_dev *indio_dev;
>  	struct cros_ec_light_prox_state *state;
> -	struct iio_chan_spec *channel;
>  	int ret;
>  
>  	if (!ec_dev || !ec_dev->ec_dev) {
> @@ -175,46 +172,38 @@ static int cros_ec_light_prox_probe(struct platform_device *pdev)
>  		return -EINVAL;
>  	}
>  
> -	indio_dev = devm_iio_device_alloc(dev, sizeof(*state));
> -	if (!indio_dev)
> -		return -ENOMEM;
> -
> -	ret = cros_ec_sensors_core_init(pdev, indio_dev, true);
> +	ret = cros_ec_sensors_core_init(pdev, sizeof(*state),
> +					CROS_EC_LIGHT_PROX_MAX_CHANNELS, true);
>  	if (ret)
>  		return ret;
>  
> +	indio_dev = platform_get_drvdata(pdev);
>  	state = iio_priv(indio_dev);
>  	state->core.type = state->core.resp->info.type;
>  	state->core.loc = state->core.resp->info.location;
>  	state->core.info.read_raw = &cros_ec_light_prox_read;
>  	state->core.info.write_raw = &cros_ec_light_prox_write;
> -	channel = state->channels;
>  
>  	/* Common part */
> -	channel->info_mask_shared_by_all =
> +	cros_ec_core_channel_init(state->core.channels, 1);
> +	state->core.channels[1].info_mask_shared_by_all =
>  		BIT(IIO_CHAN_INFO_SAMP_FREQ) |
>  		BIT(IIO_CHAN_INFO_FREQUENCY);
> -	channel->scan_type.realbits = CROS_EC_SENSOR_BITS;
> -	channel->scan_type.storagebits = CROS_EC_SENSOR_BITS;
> -	channel->scan_type.shift = 0;
> -	channel->scan_index = 0;
> -	channel->ext_info = cros_ec_sensors_ext_info;
> -	channel->scan_type.sign = 'u';
>  
>  	state->core.calib[0] = 0;
>  
>  	/* Sensor specific */
>  	switch (state->core.type) {
>  	case MOTIONSENSE_TYPE_LIGHT:
> -		channel->type = IIO_LIGHT;
> -		channel->info_mask_separate =
> +		state->core.channels[1].type = IIO_LIGHT;
> +		state->core.channels[1].info_mask_separate =
>  			BIT(IIO_CHAN_INFO_PROCESSED) |
>  			BIT(IIO_CHAN_INFO_CALIBBIAS) |
>  			BIT(IIO_CHAN_INFO_CALIBSCALE);
>  		break;
>  	case MOTIONSENSE_TYPE_PROX:
> -		channel->type = IIO_PROXIMITY;
> -		channel->info_mask_separate =
> +		state->core.channels[1].type = IIO_PROXIMITY;
> +		state->core.channels[1].info_mask_separate =
>  			BIT(IIO_CHAN_INFO_RAW) |
>  			BIT(IIO_CHAN_INFO_CALIBBIAS) |
>  			BIT(IIO_CHAN_INFO_CALIBSCALE);
> @@ -224,19 +213,6 @@ static int cros_ec_light_prox_probe(struct platform_device *pdev)
>  		return -EINVAL;
>  	}
>  
> -	/* Timestamp */
> -	channel++;
> -	channel->type = IIO_TIMESTAMP;
> -	channel->channel = -1;
> -	channel->scan_index = 1;
> -	channel->scan_type.sign = 's';
> -	channel->scan_type.realbits = 64;
> -	channel->scan_type.storagebits = 64;
> -
> -	indio_dev->channels = state->channels;
> -
> -	indio_dev->num_channels = CROS_EC_LIGHT_PROX_MAX_CHANNELS;
> -
>  	state->core.read_ec_sensors_data = cros_ec_sensors_read_cmd;
>  
>  	ret = devm_iio_triggered_buffer_setup(dev, indio_dev, NULL,
> diff --git a/drivers/iio/pressure/cros_ec_baro.c b/drivers/iio/pressure/cros_ec_baro.c
> index 85a4864e3a4e..8718036d74d2 100644
> --- a/drivers/iio/pressure/cros_ec_baro.c
> +++ b/drivers/iio/pressure/cros_ec_baro.c
> @@ -29,8 +29,6 @@
>  struct cros_ec_baro_state {
>  	/* Shared by all sensors */
>  	struct cros_ec_sensors_core_state core;
> -
> -	struct iio_chan_spec channels[CROS_EC_BARO_MAX_CHANNELS];
>  };
>  
>  static int cros_ec_baro_read(struct iio_dev *indio_dev,
> @@ -113,7 +111,6 @@ static int cros_ec_baro_probe(struct platform_device *pdev)
>  	struct cros_ec_dev *ec_dev = dev_get_drvdata(dev->parent);
>  	struct iio_dev *indio_dev;
>  	struct cros_ec_baro_state *state;
> -	struct iio_chan_spec *channel;
>  	int ret;
>  
>  	if (!ec_dev || !ec_dev->ec_dev) {
> @@ -121,57 +118,38 @@ static int cros_ec_baro_probe(struct platform_device *pdev)
>  		return -EINVAL;
>  	}
>  
> -	indio_dev = devm_iio_device_alloc(dev, sizeof(*state));
> -	if (!indio_dev)
> -		return -ENOMEM;
> -
> -	ret = cros_ec_sensors_core_init(pdev, indio_dev, true);
> +	ret = cros_ec_sensors_core_init(pdev, sizeof(*state),
> +					CROS_EC_BARO_MAX_CHANNELS, true);
>  	if (ret)
>  		return ret;
>  
> +	indio_dev = platform_get_drvdata(pdev);
>  	state = iio_priv(indio_dev);
>  	state->core.type = state->core.resp->info.type;
>  	state->core.loc = state->core.resp->info.location;
>  	state->core.info.read_raw = &cros_ec_baro_read;
>  	state->core.info.write_raw = &cros_ec_baro_write;
> -	channel = state->channels;
> +
>  	/* Common part */
> -	channel->info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
> -	channel->info_mask_shared_by_all =
> +	cros_ec_core_channel_init(state->core.channels, 1);
> +	state->core.channels[1].info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
> +	state->core.channels[1].info_mask_shared_by_all =
>  		BIT(IIO_CHAN_INFO_SCALE) |
>  		BIT(IIO_CHAN_INFO_SAMP_FREQ) |
>  		BIT(IIO_CHAN_INFO_FREQUENCY);
> -	channel->scan_type.realbits = CROS_EC_SENSOR_BITS;
> -	channel->scan_type.storagebits = CROS_EC_SENSOR_BITS;
> -	channel->scan_type.shift = 0;
> -	channel->scan_index = 0;
> -	channel->ext_info = cros_ec_sensors_ext_info;
> -	channel->scan_type.sign = 'u';
>  
>  	state->core.calib[0] = 0;
>  
>  	/* Sensor specific */
>  	switch (state->core.type) {
>  	case MOTIONSENSE_TYPE_BARO:
> -		channel->type = IIO_PRESSURE;
> +		state->core.channels[1].type = IIO_PRESSURE;
>  		break;
>  	default:
>  		dev_warn(dev, "Unknown motion sensor\n");
>  		return -EINVAL;
>  	}
>  
> -	/* Timestamp */
> -	channel++;
> -	channel->type = IIO_TIMESTAMP;
> -	channel->channel = -1;
> -	channel->scan_index = 1;
> -	channel->scan_type.sign = 's';
> -	channel->scan_type.realbits = 64;
> -	channel->scan_type.storagebits = 64;
> -
> -	indio_dev->channels = state->channels;
> -	indio_dev->num_channels = CROS_EC_BARO_MAX_CHANNELS;
> -
>  	state->core.read_ec_sensors_data = cros_ec_sensors_read_cmd;
>  
>  	ret = devm_iio_triggered_buffer_setup(dev, indio_dev, NULL,
> diff --git a/include/linux/iio/common/cros_ec_sensors_core.h b/include/linux/iio/common/cros_ec_sensors_core.h
> index a729e667f760..485c649b421f 100644
> --- a/include/linux/iio/common/cros_ec_sensors_core.h
> +++ b/include/linux/iio/common/cros_ec_sensors_core.h
> @@ -31,6 +31,8 @@ enum {
>  /* Minimum sampling period to use when device is suspending */
>  #define CROS_EC_MIN_SUSPEND_SAMPLING_FREQUENCY 1000  /* 1 second */
>  
> +#define CROS_EC_SENSORS_CORE_MAX_CHANNELS 8
> +
>  /**
>   * struct cros_ec_sensors_core_state - state data for EC sensors IIO driver
>   * @ec:				cros EC device structure
> @@ -71,6 +73,7 @@ struct cros_ec_sensors_core_state {
>  
>  	int curr_sampl_freq;
>  	struct iio_info info;
> +	struct iio_chan_spec channels[CROS_EC_SENSORS_CORE_MAX_CHANNELS];
>  };
>  
>  /**
> @@ -102,13 +105,15 @@ 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
> + * @sizeof_priv:	size of the private structure
> + * @num_channels:	Number of channel
>   * @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);
> +			      int sizeof_priv, int num_channels,
> +			      bool physical_device);
>  
>  /**
>   * cros_ec_sensors_capture() - the trigger handler function
> @@ -124,6 +129,21 @@ int cros_ec_sensors_core_init(struct platform_device *pdev,
>   */
>  irqreturn_t cros_ec_sensors_capture(int irq, void *p);
>  
> +/**
> + * cros_ec_core_channel_init() - initialize channel
> + * @channel:		channels table
> + * @idx:		channel index to initialize
> + *
> + * Return: 0 on success, -errno on failure.
> + */
> +#define cros_ec_core_channel_init(channel, idx) \
> +	channel[idx].scan_type.realbits = CROS_EC_SENSOR_BITS;\
> +	channel[idx].scan_type.storagebits = CROS_EC_SENSOR_BITS;\
> +	channel[idx].scan_type.shift = 0;\

No need to define the 'obvious' 0 shift case. 
These must already be initialized to zero or all sorts of things are
going to potentially break.

> +	channel[idx].scan_index = idx;\
> +	channel[idx].ext_info = cros_ec_sensors_ext_info;\
> +	channel[idx].scan_type.sign = 'u';
> +
>  /**
>   * cros_ec_motion_send_host_cmd() - send motion sense host command
>   * @st:		pointer to state information for device

Re: [PATCH v3 3/8] iio: common: cros_ec_sensors: move registration to core

[Jonathan Cameron]

On Tue, 18 Jun 2019 11:06:34 +0200
Fabien Lahoudere <fabien.lahoudere@collabora.com> wrote:

> In order to simplify derivated drivers from cros_ec_sensors_core,
> a new core function is created to registered IIO stricture.
> 
> Signed-off-by: Fabien Lahoudere <fabien.lahoudere@collabora.com>
This one looks good to me.
I'll pick it up once the minor stuff in other patches is sorted.

Thanks,

Jonathan

> ---
>  .../common/cros_ec_sensors/cros_ec_sensors.c  |  9 +-
>  .../cros_ec_sensors/cros_ec_sensors_core.c    | 97 ++++++++++++-------
>  drivers/iio/light/cros_ec_light_prox.c        |  7 +-
>  drivers/iio/pressure/cros_ec_baro.c           |  7 +-
>  .../linux/iio/common/cros_ec_sensors_core.h   | 16 ++-
>  5 files changed, 72 insertions(+), 64 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 897dc83a3355..c4bee9265246 100644
> --- a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors.c
> +++ b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors.c
> @@ -14,7 +14,7 @@
>  #include <linux/iio/iio.h>
>  #include <linux/iio/kfifo_buf.h>
>  #include <linux/iio/trigger_consumer.h>
> -#include <linux/iio/triggered_buffer.h>
> +
>  #include <linux/kernel.h>
>  #include <linux/mfd/cros_ec.h>
>  #include <linux/mfd/cros_ec_commands.h>
> @@ -233,12 +233,7 @@ static int cros_ec_sensors_probe(struct platform_device *pdev)
>  	else
>  		state->core.read_ec_sensors_data = cros_ec_sensors_read_cmd;
>  
> -	ret = devm_iio_triggered_buffer_setup(dev, indio_dev, NULL,
> -			cros_ec_sensors_capture, NULL);
> -	if (ret)
> -		return ret;
> -
> -	return devm_iio_device_register(dev, indio_dev);
> +	return cros_ec_sensors_core_register(pdev, indio_dev);
>  }
>  
>  static const struct platform_device_id cros_ec_sensors_ids[] = {
> 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 e5181e007dd7..3880849c5cca 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
> @@ -12,6 +12,7 @@
>  #include <linux/iio/iio.h>
>  #include <linux/iio/kfifo_buf.h>
>  #include <linux/iio/trigger_consumer.h>
> +#include <linux/iio/triggered_buffer.h>
>  #include <linux/kernel.h>
>  #include <linux/mfd/cros_ec.h>
>  #include <linux/mfd/cros_ec_commands.h>
> @@ -95,6 +96,67 @@ int cros_ec_sensors_core_init(struct platform_device *pdev,
>  }
>  EXPORT_SYMBOL_GPL(cros_ec_sensors_core_init);
>  
> +/**
> + * cros_ec_sensors_capture() - the trigger handler function
> + * @irq:	the interrupt number.
> + * @p:		a pointer to the poll function.
> + *
> + * On a trigger event occurring, if the pollfunc is attached then this
> + * handler is called as a threaded interrupt (and hence may sleep). It
> + * is responsible for grabbing data from the device and pushing it into
> + * the associated buffer.
> + *
> + * Return: IRQ_HANDLED
> + */
> +static irqreturn_t cros_ec_sensors_capture(int irq, void *p)
> +{
> +	struct iio_poll_func *pf = p;
> +	struct iio_dev *indio_dev = pf->indio_dev;
> +	struct cros_ec_sensors_core_state *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	mutex_lock(&st->cmd_lock);
> +
> +	/* Clear capture data. */
> +	memset(st->samples, 0, indio_dev->scan_bytes);
> +
> +	/* Read data based on which channels are enabled in scan mask. */
> +	ret = st->read_ec_sensors_data(indio_dev,
> +				       *indio_dev->active_scan_mask,
> +				       (s16 *)st->samples);
> +	if (ret < 0)
> +		goto done;
> +
> +	iio_push_to_buffers_with_timestamp(indio_dev, st->samples,
> +					   iio_get_time_ns(indio_dev));
> +
> +done:
> +	/*
> +	 * Tell the core we are done with this trigger and ready for the
> +	 * next one.
> +	 */
> +	iio_trigger_notify_done(indio_dev->trig);
> +
> +	mutex_unlock(&st->cmd_lock);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +int cros_ec_sensors_core_register(struct platform_device *pdev,
> +				  struct iio_dev *indio_dev)
> +{
> +	int ret;
> +	struct device *dev = &pdev->dev;
> +
> +	ret = devm_iio_triggered_buffer_setup(dev, indio_dev, NULL,
> +					      cros_ec_sensors_capture, NULL);
> +	if (ret)
> +		return ret;
> +
> +	return devm_iio_device_register(dev, indio_dev);
> +}
> +EXPORT_SYMBOL_GPL(cros_ec_sensors_core_register);
> +
>  int cros_ec_motion_send_host_cmd(struct cros_ec_sensors_core_state *state,
>  				 u16 opt_length)
>  {
> @@ -380,41 +442,6 @@ int cros_ec_sensors_read_cmd(struct iio_dev *indio_dev,
>  }
>  EXPORT_SYMBOL_GPL(cros_ec_sensors_read_cmd);
>  
> -irqreturn_t cros_ec_sensors_capture(int irq, void *p)
> -{
> -	struct iio_poll_func *pf = p;
> -	struct iio_dev *indio_dev = pf->indio_dev;
> -	struct cros_ec_sensors_core_state *st = iio_priv(indio_dev);
> -	int ret;
> -
> -	mutex_lock(&st->cmd_lock);
> -
> -	/* Clear capture data. */
> -	memset(st->samples, 0, indio_dev->scan_bytes);
> -
> -	/* Read data based on which channels are enabled in scan mask. */
> -	ret = st->read_ec_sensors_data(indio_dev,
> -				       *(indio_dev->active_scan_mask),
> -				       (s16 *)st->samples);
> -	if (ret < 0)
> -		goto done;
> -
> -	iio_push_to_buffers_with_timestamp(indio_dev, st->samples,
> -					   iio_get_time_ns(indio_dev));
> -
> -done:
> -	/*
> -	 * Tell the core we are done with this trigger and ready for the
> -	 * next one.
> -	 */
> -	iio_trigger_notify_done(indio_dev->trig);
> -
> -	mutex_unlock(&st->cmd_lock);
> -
> -	return IRQ_HANDLED;
> -}
> -EXPORT_SYMBOL_GPL(cros_ec_sensors_capture);
> -
>  int cros_ec_sensors_core_read(struct cros_ec_sensors_core_state *st,
>  			  struct iio_chan_spec const *chan,
>  			  int *val, int *val2, long mask)
> diff --git a/drivers/iio/light/cros_ec_light_prox.c b/drivers/iio/light/cros_ec_light_prox.c
> index 32ea5afd495f..682dc19c2bf3 100644> --- a/drivers/iio/light/cros_ec_light_prox.c
> +++ b/drivers/iio/light/cros_ec_light_prox.c
> @@ -215,12 +215,7 @@ static int cros_ec_light_prox_probe(struct platform_device *pdev)
>  
>  	state->core.read_ec_sensors_data = cros_ec_sensors_read_cmd;
>  
> -	ret = devm_iio_triggered_buffer_setup(dev, indio_dev, NULL,
> -					      cros_ec_sensors_capture, NULL);
> -	if (ret)
> -		return ret;
> -
> -	return devm_iio_device_register(dev, indio_dev);
> +	return cros_ec_sensors_core_register(pdev, indio_dev);
>  }
>  
>  static const struct platform_device_id cros_ec_light_prox_ids[] = {
> diff --git a/drivers/iio/pressure/cros_ec_baro.c b/drivers/iio/pressure/cros_ec_baro.c
> index 8718036d74d2..9d3745bc2fba 100644
> --- a/drivers/iio/pressure/cros_ec_baro.c
> +++ b/drivers/iio/pressure/cros_ec_baro.c
> @@ -152,12 +152,7 @@ static int cros_ec_baro_probe(struct platform_device *pdev)
>  
>  	state->core.read_ec_sensors_data = cros_ec_sensors_read_cmd;
>  
> -	ret = devm_iio_triggered_buffer_setup(dev, indio_dev, NULL,
> -					      cros_ec_sensors_capture, NULL);
> -	if (ret)
> -		return ret;
> -
> -	return devm_iio_device_register(dev, indio_dev);
> +	return cros_ec_sensors_core_register(pdev, indio_dev);
>  }
>  
>  static const struct platform_device_id cros_ec_baro_ids[] = {
> diff --git a/include/linux/iio/common/cros_ec_sensors_core.h b/include/linux/iio/common/cros_ec_sensors_core.h
> index 485c649b421f..60f40d253f4a 100644
> --- a/include/linux/iio/common/cros_ec_sensors_core.h
> +++ b/include/linux/iio/common/cros_ec_sensors_core.h
> @@ -116,18 +116,14 @@ int cros_ec_sensors_core_init(struct platform_device *pdev,
>  			      bool physical_device);
>  
>  /**
> - * cros_ec_sensors_capture() - the trigger handler function
> - * @irq:	the interrupt number.
> - * @p:		a pointer to the poll function.
> - *
> - * On a trigger event occurring, if the pollfunc is attached then this
> - * handler is called as a threaded interrupt (and hence may sleep). It
> - * is responsible for grabbing data from the device and pushing it into
> - * the associated buffer.
> + * cros_ec_sensors_core_register() - registration of the core structure
> + * @pdev:		platform device created for the sensors
> + * @indio_dev:		iio device structure of the device
>   *
> - * Return: IRQ_HANDLED
> + * Return: 0 on success, -errno on failure.
>   */
> -irqreturn_t cros_ec_sensors_capture(int irq, void *p);
> +int cros_ec_sensors_core_register(struct platform_device *pdev,
> +				  struct iio_dev *indio_dev);
>  
>  /**
>   * cros_ec_core_channel_init() - initialize channel

Re: [PATCH v3 6/8] iio: common: cros_ec_sensors: support protocol v3 message

[Jonathan Cameron]

On Tue, 18 Jun 2019 11:06:37 +0200
Fabien Lahoudere <fabien.lahoudere@collabora.com> wrote:

> Version 3 of the EC protocol provides min and max frequencies for EC sensors.
> 
> Signed-off-by: Fabien Lahoudere <fabien.lahoudere@collabora.com>
> Signed-off-by: Nick Vaccaro <nvaccaro@chromium.org>
Looks good to me. I'll pick up next time if no one else raises any
issues on this one.

Thanks,

Jonathan

> ---
>  .../cros_ec_sensors/cros_ec_sensors_core.c    | 85 ++++++++++++++++++-
>  .../linux/iio/common/cros_ec_sensors_core.h   |  3 +
>  2 files changed, 87 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 57034e212fe1..2ce077b576a4 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
> @@ -26,6 +26,66 @@ 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)
> +{
> +	int ret;
> +	struct {
> +		struct cros_ec_command msg;
> +		union {
> +			struct ec_params_get_cmd_versions params;
> +			struct ec_response_get_cmd_versions resp;
> +		};
> +	} __packed buf = {
> +		.msg = {
> +			.command = EC_CMD_GET_CMD_VERSIONS + cmd_offset,
> +			.insize = sizeof(struct ec_response_get_cmd_versions),
> +			.outsize = sizeof(struct ec_params_get_cmd_versions)
> +			},
> +		.params = {.cmd = cmd}
> +	};
> +
> +	ret = cros_ec_cmd_xfer_status(ec_dev, &buf.msg);
> +	if (ret >= 0) {
> +		if (buf.msg.result == EC_RES_SUCCESS)
> +			*mask = buf.resp.version_mask;
> +		else
> +			*mask = 0;
> +	}
> +	return ret;
> +}
> +
>  int cros_ec_sensors_core_init(struct platform_device *pdev,
>  			      int num_channels,
>  			      bool physical_device)
> @@ -35,6 +95,8 @@ int cros_ec_sensors_core_init(struct platform_device *pdev,
>  	struct cros_ec_dev *ec = dev_get_drvdata(pdev->dev.parent);
>  	struct cros_ec_sensor_platform *sensor_platform = dev_get_platdata(dev);
>  	struct iio_dev *indio_dev;
> +	u32 ver_mask;
> +	int ret;
>  
>  	if (num_channels > CROS_EC_SENSORS_CORE_MAX_CHANNELS)
>  		return -EINVAL;
> @@ -57,8 +119,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);
>  
> @@ -76,6 +146,19 @@ int cros_ec_sensors_core_init(struct platform_device *pdev,
>  		}
>  		state->type = state->resp->info.type;
>  		state->loc = state->resp->info.location;
> +
> +		/* Value to stop the device */
> +		state->frequency_range[0] = 0;
> +		if (state->msg->version < 3) {
> +			get_default_min_max_freq(state->resp->info.type,
> +						 &state->frequency_range[1],
> +						 &state->frequency_range[2]);
> +		} else {
> +			state->frequency_range[1] =
> +			    state->resp->info_3.min_frequency;
> +			state->frequency_range[2] =
> +			    state->resp->info_3.max_frequency;
> +		}
>  	}
>  
>  	indio_dev->info = &state->info;
> diff --git a/include/linux/iio/common/cros_ec_sensors_core.h b/include/linux/iio/common/cros_ec_sensors_core.h
> index 3e6de427076e..89937ad242ef 100644
> --- a/include/linux/iio/common/cros_ec_sensors_core.h
> +++ b/include/linux/iio/common/cros_ec_sensors_core.h
> @@ -74,6 +74,9 @@ struct cros_ec_sensors_core_state {
>  	int curr_sampl_freq;
>  	struct iio_info info;
>  	struct iio_chan_spec channels[CROS_EC_SENSORS_CORE_MAX_CHANNELS];
> +
> +	/* Disable, Min and Max Sampling Frequency in mHz */
> +	int frequency_range[3];
>  };
>  
>  /**

Re: [PATCH v3 7/8] iio: common: cros_ec_sensors: add sysfs attribute for frequencies

[Jonathan Cameron]

On Tue, 18 Jun 2019 11:06:38 +0200
Fabien Lahoudere <fabien.lahoudere@collabora.com> wrote:

> Embedded controller return minimum and maximum frequencies, unfortunately
> we have no way to know the step for all available frequencies.
> Even if not complete, we can return a list of known values using the
> standard read_avail callback (IIO_CHAN_INFO_SAMP_FREQ) to provide them to
> userland.
> 
> Now cros_ec_* sensors provides frequencies values in sysfs like this:
> "0 min max".
> 
> 0 is always true to disable the sensor.

Hmm. 0 frequency feels like a kind of non standard use of the ABI.
I suppose it's fairly logical and it does no harm if userspace never uses
it so I guess we can let this use in without updating the docs.

I'd like some feedback from the chromeos side on whether this is a sensible
way of representing things?  Note that generic userspace will only pick
one of those values, it won't magically decide to go for somewhere inbetween.

Jonathan

> 
> Signed-off-by: Fabien Lahoudere <fabien.lahoudere@collabora.com>
> ---
>  .../cros_ec_sensors/cros_ec_sensors_core.c    | 22 +++++++++++++++++++
>  .../linux/iio/common/cros_ec_sensors_core.h   |  4 +++-
>  2 files changed, 25 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 2ce077b576a4..8df82b675752 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/iio/triggered_buffer.h>
> @@ -86,6 +87,26 @@ static int cros_ec_get_host_cmd_version_mask(struct cros_ec_device *ec_dev,
>  	return ret;
>  }
>  
> +static int cros_ec_read_avail(struct iio_dev *indio_dev,
> +			      struct iio_chan_spec const *chan,
> +			      const int **vals,
> +			      int *type,
> +			      int *length,
> +			      long mask)
> +{
> +	struct cros_ec_sensors_core_state *state = iio_priv(indio_dev);
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		*length = 3;
> +		*vals = (const int *)&state->frequency_range;
> +		*type = IIO_VAL_INT;
> +		return IIO_AVAIL_LIST;
> +	}
> +
> +	return -EINVAL;
> +}
> +
>  int cros_ec_sensors_core_init(struct platform_device *pdev,
>  			      int num_channels,
>  			      bool physical_device)
> @@ -161,6 +182,7 @@ int cros_ec_sensors_core_init(struct platform_device *pdev,
>  		}
>  	}
>  
> +	state->info.read_avail = cros_ec_read_avail;
>  	indio_dev->info = &state->info;
>  
>  	/* Timestamp channel */
> diff --git a/include/linux/iio/common/cros_ec_sensors_core.h b/include/linux/iio/common/cros_ec_sensors_core.h
> index 89937ad242ef..5fa9ba5332e0 100644
> --- a/include/linux/iio/common/cros_ec_sensors_core.h
> +++ b/include/linux/iio/common/cros_ec_sensors_core.h
> @@ -140,7 +140,9 @@ int cros_ec_sensors_core_register(struct platform_device *pdev,
>  	channel[idx].scan_type.shift = 0;\
>  	channel[idx].scan_index = idx;\
>  	channel[idx].ext_info = cros_ec_sensors_ext_info;\
> -	channel[idx].scan_type.sign = 'u';
> +	channel[idx].scan_type.sign = 'u';\
> +	channel[idx].info_mask_shared_by_all_available = \
> +		BIT(IIO_CHAN_INFO_SAMP_FREQ);
>  
>  /**
>   * cros_ec_motion_send_host_cmd() - send motion sense host command

Re: [PATCH v3 8/8] docs: iio: add precision about sampling_frequency_available

[Jonathan Cameron]

On Tue, 18 Jun 2019 11:06:39 +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>
It seems I already applied this one, though probably haven't sent
a pull request for it to Greg yet.

Please drop it from your v4 posting as otherwise I'll get confused
(again).

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

Re: [PATCH v3 3/8] iio: common: cros_ec_sensors: move registration to core

[Jonathan Cameron]

On Sat, 22 Jun 2019 11:13:08 +0100
Jonathan Cameron <jic23@kernel.org> wrote:

> On Tue, 18 Jun 2019 11:06:34 +0200
> Fabien Lahoudere <fabien.lahoudere@collabora.com> wrote:
> 
> > In order to simplify derivated drivers from cros_ec_sensors_core,
> > a new core function is created to registered IIO stricture.
> > 
> > Signed-off-by: Fabien Lahoudere <fabien.lahoudere@collabora.com>  
> This one looks good to me.
> I'll pick it up once the minor stuff in other patches is sorted.

Sorry, I confused a couple of patches as held some of these back
in my queue to see how you used them later.

This one I'm actually unconvinced by.  It's a 'cleanup'
that adds code and hides what is going on. I was expecting to find
more stuff being added to the resulting function later in the series.
I didn't find any, but may have missed something.

So not keen on this one patch in the series!

thanks,

Jonathan

> 
> Thanks,
> 
> Jonathan
> 
> > ---
> >  .../common/cros_ec_sensors/cros_ec_sensors.c  |  9 +-
> >  .../cros_ec_sensors/cros_ec_sensors_core.c    | 97 ++++++++++++-------
> >  drivers/iio/light/cros_ec_light_prox.c        |  7 +-
> >  drivers/iio/pressure/cros_ec_baro.c           |  7 +-
> >  .../linux/iio/common/cros_ec_sensors_core.h   | 16 ++-
> >  5 files changed, 72 insertions(+), 64 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 897dc83a3355..c4bee9265246 100644
> > --- a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors.c
> > +++ b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors.c
> > @@ -14,7 +14,7 @@
> >  #include <linux/iio/iio.h>
> >  #include <linux/iio/kfifo_buf.h>
> >  #include <linux/iio/trigger_consumer.h>
> > -#include <linux/iio/triggered_buffer.h>
> > +
> >  #include <linux/kernel.h>
> >  #include <linux/mfd/cros_ec.h>
> >  #include <linux/mfd/cros_ec_commands.h>
> > @@ -233,12 +233,7 @@ static int cros_ec_sensors_probe(struct platform_device *pdev)
> >  	else
> >  		state->core.read_ec_sensors_data = cros_ec_sensors_read_cmd;
> >  
> > -	ret = devm_iio_triggered_buffer_setup(dev, indio_dev, NULL,
> > -			cros_ec_sensors_capture, NULL);
> > -	if (ret)
> > -		return ret;
> > -
> > -	return devm_iio_device_register(dev, indio_dev);
> > +	return cros_ec_sensors_core_register(pdev, indio_dev);
> >  }
> >  
> >  static const struct platform_device_id cros_ec_sensors_ids[] = {
> > 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 e5181e007dd7..3880849c5cca 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
> > @@ -12,6 +12,7 @@
> >  #include <linux/iio/iio.h>
> >  #include <linux/iio/kfifo_buf.h>
> >  #include <linux/iio/trigger_consumer.h>
> > +#include <linux/iio/triggered_buffer.h>
> >  #include <linux/kernel.h>
> >  #include <linux/mfd/cros_ec.h>
> >  #include <linux/mfd/cros_ec_commands.h>
> > @@ -95,6 +96,67 @@ int cros_ec_sensors_core_init(struct platform_device *pdev,
> >  }
> >  EXPORT_SYMBOL_GPL(cros_ec_sensors_core_init);
> >  
> > +/**
> > + * cros_ec_sensors_capture() - the trigger handler function
> > + * @irq:	the interrupt number.
> > + * @p:		a pointer to the poll function.
> > + *
> > + * On a trigger event occurring, if the pollfunc is attached then this
> > + * handler is called as a threaded interrupt (and hence may sleep). It
> > + * is responsible for grabbing data from the device and pushing it into
> > + * the associated buffer.
> > + *
> > + * Return: IRQ_HANDLED
> > + */
> > +static irqreturn_t cros_ec_sensors_capture(int irq, void *p)
> > +{
> > +	struct iio_poll_func *pf = p;
> > +	struct iio_dev *indio_dev = pf->indio_dev;
> > +	struct cros_ec_sensors_core_state *st = iio_priv(indio_dev);
> > +	int ret;
> > +
> > +	mutex_lock(&st->cmd_lock);
> > +
> > +	/* Clear capture data. */
> > +	memset(st->samples, 0, indio_dev->scan_bytes);
> > +
> > +	/* Read data based on which channels are enabled in scan mask. */
> > +	ret = st->read_ec_sensors_data(indio_dev,
> > +				       *indio_dev->active_scan_mask,
> > +				       (s16 *)st->samples);
> > +	if (ret < 0)
> > +		goto done;
> > +
> > +	iio_push_to_buffers_with_timestamp(indio_dev, st->samples,
> > +					   iio_get_time_ns(indio_dev));
> > +
> > +done:
> > +	/*
> > +	 * Tell the core we are done with this trigger and ready for the
> > +	 * next one.
> > +	 */
> > +	iio_trigger_notify_done(indio_dev->trig);
> > +
> > +	mutex_unlock(&st->cmd_lock);
> > +
> > +	return IRQ_HANDLED;
> > +}
> > +
> > +int cros_ec_sensors_core_register(struct platform_device *pdev,
> > +				  struct iio_dev *indio_dev)
> > +{
> > +	int ret;
> > +	struct device *dev = &pdev->dev;
> > +
> > +	ret = devm_iio_triggered_buffer_setup(dev, indio_dev, NULL,
> > +					      cros_ec_sensors_capture, NULL);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return devm_iio_device_register(dev, indio_dev);
> > +}
> > +EXPORT_SYMBOL_GPL(cros_ec_sensors_core_register);
> > +
> >  int cros_ec_motion_send_host_cmd(struct cros_ec_sensors_core_state *state,
> >  				 u16 opt_length)
> >  {
> > @@ -380,41 +442,6 @@ int cros_ec_sensors_read_cmd(struct iio_dev *indio_dev,
> >  }
> >  EXPORT_SYMBOL_GPL(cros_ec_sensors_read_cmd);
> >  
> > -irqreturn_t cros_ec_sensors_capture(int irq, void *p)
> > -{
> > -	struct iio_poll_func *pf = p;
> > -	struct iio_dev *indio_dev = pf->indio_dev;
> > -	struct cros_ec_sensors_core_state *st = iio_priv(indio_dev);
> > -	int ret;
> > -
> > -	mutex_lock(&st->cmd_lock);
> > -
> > -	/* Clear capture data. */
> > -	memset(st->samples, 0, indio_dev->scan_bytes);
> > -
> > -	/* Read data based on which channels are enabled in scan mask. */
> > -	ret = st->read_ec_sensors_data(indio_dev,
> > -				       *(indio_dev->active_scan_mask),
> > -				       (s16 *)st->samples);
> > -	if (ret < 0)
> > -		goto done;
> > -
> > -	iio_push_to_buffers_with_timestamp(indio_dev, st->samples,
> > -					   iio_get_time_ns(indio_dev));
> > -
> > -done:
> > -	/*
> > -	 * Tell the core we are done with this trigger and ready for the
> > -	 * next one.
> > -	 */
> > -	iio_trigger_notify_done(indio_dev->trig);
> > -
> > -	mutex_unlock(&st->cmd_lock);
> > -
> > -	return IRQ_HANDLED;
> > -}
> > -EXPORT_SYMBOL_GPL(cros_ec_sensors_capture);
> > -
> >  int cros_ec_sensors_core_read(struct cros_ec_sensors_core_state *st,
> >  			  struct iio_chan_spec const *chan,
> >  			  int *val, int *val2, long mask)
> > diff --git a/drivers/iio/light/cros_ec_light_prox.c b/drivers/iio/light/cros_ec_light_prox.c
> > index 32ea5afd495f..682dc19c2bf3 100644> --- a/drivers/iio/light/cros_ec_light_prox.c
> > +++ b/drivers/iio/light/cros_ec_light_prox.c
> > @@ -215,12 +215,7 @@ static int cros_ec_light_prox_probe(struct platform_device *pdev)
> >  
> >  	state->core.read_ec_sensors_data = cros_ec_sensors_read_cmd;
> >  
> > -	ret = devm_iio_triggered_buffer_setup(dev, indio_dev, NULL,
> > -					      cros_ec_sensors_capture, NULL);
> > -	if (ret)
> > -		return ret;
> > -
> > -	return devm_iio_device_register(dev, indio_dev);
> > +	return cros_ec_sensors_core_register(pdev, indio_dev);
> >  }
> >  
> >  static const struct platform_device_id cros_ec_light_prox_ids[] = {
> > diff --git a/drivers/iio/pressure/cros_ec_baro.c b/drivers/iio/pressure/cros_ec_baro.c
> > index 8718036d74d2..9d3745bc2fba 100644
> > --- a/drivers/iio/pressure/cros_ec_baro.c
> > +++ b/drivers/iio/pressure/cros_ec_baro.c
> > @@ -152,12 +152,7 @@ static int cros_ec_baro_probe(struct platform_device *pdev)
> >  
> >  	state->core.read_ec_sensors_data = cros_ec_sensors_read_cmd;
> >  
> > -	ret = devm_iio_triggered_buffer_setup(dev, indio_dev, NULL,
> > -					      cros_ec_sensors_capture, NULL);
> > -	if (ret)
> > -		return ret;
> > -
> > -	return devm_iio_device_register(dev, indio_dev);
> > +	return cros_ec_sensors_core_register(pdev, indio_dev);
> >  }
> >  
> >  static const struct platform_device_id cros_ec_baro_ids[] = {
> > diff --git a/include/linux/iio/common/cros_ec_sensors_core.h b/include/linux/iio/common/cros_ec_sensors_core.h
> > index 485c649b421f..60f40d253f4a 100644
> > --- a/include/linux/iio/common/cros_ec_sensors_core.h
> > +++ b/include/linux/iio/common/cros_ec_sensors_core.h
> > @@ -116,18 +116,14 @@ int cros_ec_sensors_core_init(struct platform_device *pdev,
> >  			      bool physical_device);
> >  
> >  /**
> > - * cros_ec_sensors_capture() - the trigger handler function
> > - * @irq:	the interrupt number.
> > - * @p:		a pointer to the poll function.
> > - *
> > - * On a trigger event occurring, if the pollfunc is attached then this
> > - * handler is called as a threaded interrupt (and hence may sleep). It
> > - * is responsible for grabbing data from the device and pushing it into
> > - * the associated buffer.
> > + * cros_ec_sensors_core_register() - registration of the core structure
> > + * @pdev:		platform device created for the sensors
> > + * @indio_dev:		iio device structure of the device
> >   *
> > - * Return: IRQ_HANDLED
> > + * Return: 0 on success, -errno on failure.
> >   */
> > -irqreturn_t cros_ec_sensors_capture(int irq, void *p);
> > +int cros_ec_sensors_core_register(struct platform_device *pdev,
> > +				  struct iio_dev *indio_dev);
> >  
> >  /**
> >   * cros_ec_core_channel_init() - initialize channel  
> 

Re: [PATCH v3 5/8] iio: common: cros_ec_sensors: use core structure

[Jonathan Cameron]

On Tue, 18 Jun 2019 11:06:36 +0200
Fabien Lahoudere <fabien.lahoudere@collabora.com> wrote:

> Drivers based on cros_ec_sensors_core have structure containing the
> core structure. In order to simplify, we drop all the specific and
> useless structure to use the same one in all drivers.
> If a future driver need specific field, we can add a private pointer
> to that data.
> 
> Signed-off-by: Fabien Lahoudere <fabien.lahoudere@collabora.com>
This is the one I wanted to say seemed entirely sensible and
I'll pick up once the other minor bits and bobs are resolved!

Sorry for the confusion.

Jonathan

> ---
>  .../common/cros_ec_sensors/cros_ec_sensors.c  | 114 ++++++++----------
>  .../cros_ec_sensors/cros_ec_sensors_core.c    |   3 +-
>  drivers/iio/light/cros_ec_light_prox.c        |  85 ++++++-------
>  drivers/iio/pressure/cros_ec_baro.c           |  62 +++++-----
>  .../linux/iio/common/cros_ec_sensors_core.h   |   3 +-
>  5 files changed, 121 insertions(+), 146 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 c4bee9265246..1f0d1c614ffc 100644
> --- a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors.c
> +++ b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors.c
> @@ -24,58 +24,52 @@
>  
>  #define CROS_EC_SENSORS_MAX_CHANNELS 4
>  
> -/* State data for ec_sensors iio driver. */
> -struct cros_ec_sensors_state {
> -	/* Shared by all sensors */
> -	struct cros_ec_sensors_core_state core;
> -};
> -
>  static int cros_ec_sensors_read(struct iio_dev *indio_dev,
>  			  struct iio_chan_spec const *chan,
>  			  int *val, int *val2, long mask)
>  {
> -	struct cros_ec_sensors_state *st = iio_priv(indio_dev);
> +	struct cros_ec_sensors_core_state *st = iio_priv(indio_dev);
>  	s16 data = 0;
>  	s64 val64;
>  	int i;
>  	int ret;
>  	int idx = chan->scan_index;
>  
> -	mutex_lock(&st->core.cmd_lock);
> +	mutex_lock(&st->cmd_lock);
>  
>  	switch (mask) {
>  	case IIO_CHAN_INFO_RAW:
> -		ret = st->core.read_ec_sensors_data(indio_dev, 1 << idx, &data);
> +		ret = st->read_ec_sensors_data(indio_dev, 1 << idx, &data);
>  		if (ret < 0)
>  			break;
>  		ret = IIO_VAL_INT;
>  		*val = data;
>  		break;
>  	case IIO_CHAN_INFO_CALIBBIAS:
> -		st->core.param.cmd = MOTIONSENSE_CMD_SENSOR_OFFSET;
> -		st->core.param.sensor_offset.flags = 0;
> +		st->param.cmd = MOTIONSENSE_CMD_SENSOR_OFFSET;
> +		st->param.sensor_offset.flags = 0;
>  
> -		ret = cros_ec_motion_send_host_cmd(&st->core, 0);
> +		ret = cros_ec_motion_send_host_cmd(st, 0);
>  		if (ret < 0)
>  			break;
>  
>  		/* Save values */
>  		for (i = CROS_EC_SENSOR_X; i < CROS_EC_SENSOR_MAX_AXIS; i++)
> -			st->core.calib[i] =
> -				st->core.resp->sensor_offset.offset[i];
> +			st->calib[i] =
> +				st->resp->sensor_offset.offset[i];
>  		ret = IIO_VAL_INT;
> -		*val = st->core.calib[idx];
> +		*val = st->calib[idx];
>  		break;
>  	case IIO_CHAN_INFO_SCALE:
> -		st->core.param.cmd = MOTIONSENSE_CMD_SENSOR_RANGE;
> -		st->core.param.sensor_range.data = EC_MOTION_SENSE_NO_VALUE;
> +		st->param.cmd = MOTIONSENSE_CMD_SENSOR_RANGE;
> +		st->param.sensor_range.data = EC_MOTION_SENSE_NO_VALUE;
>  
> -		ret = cros_ec_motion_send_host_cmd(&st->core, 0);
> +		ret = cros_ec_motion_send_host_cmd(st, 0);
>  		if (ret < 0)
>  			break;
>  
> -		val64 = st->core.resp->sensor_range.ret;
> -		switch (st->core.type) {
> +		val64 = st->resp->sensor_range.ret;
> +		switch (st->type) {
>  		case MOTIONSENSE_TYPE_ACCEL:
>  			/*
>  			 * EC returns data in g, iio exepects m/s^2.
> @@ -110,11 +104,10 @@ static int cros_ec_sensors_read(struct iio_dev *indio_dev,
>  		}
>  		break;
>  	default:
> -		ret = cros_ec_sensors_core_read(&st->core, chan, val, val2,
> -						mask);
> +		ret = cros_ec_sensors_core_read(st, chan, val, val2, mask);
>  		break;
>  	}
> -	mutex_unlock(&st->core.cmd_lock);
> +	mutex_unlock(&st->cmd_lock);
>  
>  	return ret;
>  }
> @@ -123,49 +116,48 @@ static int cros_ec_sensors_write(struct iio_dev *indio_dev,
>  			       struct iio_chan_spec const *chan,
>  			       int val, int val2, long mask)
>  {
> -	struct cros_ec_sensors_state *st = iio_priv(indio_dev);
> +	struct cros_ec_sensors_core_state *st = iio_priv(indio_dev);
>  	int i;
>  	int ret;
>  	int idx = chan->scan_index;
>  
> -	mutex_lock(&st->core.cmd_lock);
> +	mutex_lock(&st->cmd_lock);
>  
>  	switch (mask) {
>  	case IIO_CHAN_INFO_CALIBBIAS:
> -		st->core.calib[idx] = val;
> +		st->calib[idx] = val;
>  
>  		/* Send to EC for each axis, even if not complete */
> -		st->core.param.cmd = MOTIONSENSE_CMD_SENSOR_OFFSET;
> -		st->core.param.sensor_offset.flags =
> +		st->param.cmd = MOTIONSENSE_CMD_SENSOR_OFFSET;
> +		st->param.sensor_offset.flags =
>  			MOTION_SENSE_SET_OFFSET;
>  		for (i = CROS_EC_SENSOR_X; i < CROS_EC_SENSOR_MAX_AXIS; i++)
> -			st->core.param.sensor_offset.offset[i] =
> -				st->core.calib[i];
> -		st->core.param.sensor_offset.temp =
> +			st->param.sensor_offset.offset[i] =
> +				st->calib[i];
> +		st->param.sensor_offset.temp =
>  			EC_MOTION_SENSE_INVALID_CALIB_TEMP;
>  
> -		ret = cros_ec_motion_send_host_cmd(&st->core, 0);
> +		ret = cros_ec_motion_send_host_cmd(st, 0);
>  		break;
>  	case IIO_CHAN_INFO_SCALE:
> -		if (st->core.type == MOTIONSENSE_TYPE_MAG) {
> +		if (st->type == MOTIONSENSE_TYPE_MAG) {
>  			ret = -EINVAL;
>  			break;
>  		}
> -		st->core.param.cmd = MOTIONSENSE_CMD_SENSOR_RANGE;
> -		st->core.param.sensor_range.data = val;
> +		st->param.cmd = MOTIONSENSE_CMD_SENSOR_RANGE;
> +		st->param.sensor_range.data = val;
>  
>  		/* Always roundup, so caller gets at least what it asks for. */
> -		st->core.param.sensor_range.roundup = 1;
> +		st->param.sensor_range.roundup = 1;
>  
> -		ret = cros_ec_motion_send_host_cmd(&st->core, 0);
> +		ret = cros_ec_motion_send_host_cmd(st, 0);
>  		break;
>  	default:
> -		ret = cros_ec_sensors_core_write(
> -				&st->core, chan, val, val2, mask);
> +		ret = cros_ec_sensors_core_write(st, chan, val, val2, mask);
>  		break;
>  	}
>  
> -	mutex_unlock(&st->core.cmd_lock);
> +	mutex_unlock(&st->cmd_lock);
>  
>  	return ret;
>  }
> @@ -175,7 +167,7 @@ static int cros_ec_sensors_probe(struct platform_device *pdev)
>  	struct device *dev = &pdev->dev;
>  	struct cros_ec_dev *ec_dev = dev_get_drvdata(dev->parent);
>  	struct iio_dev *indio_dev;
> -	struct cros_ec_sensors_state *state;
> +	struct cros_ec_sensors_core_state *state;
>  	int ret, i;
>  
>  	if (!ec_dev || !ec_dev->ec_dev) {
> @@ -183,8 +175,8 @@ static int cros_ec_sensors_probe(struct platform_device *pdev)
>  		return -EINVAL;
>  	}
>  
> -	ret = cros_ec_sensors_core_init(pdev, sizeof(*state),
> -					CROS_EC_SENSORS_MAX_CHANNELS, true);
> +	ret = cros_ec_sensors_core_init(pdev, CROS_EC_SENSORS_MAX_CHANNELS,
> +					true);
>  	if (ret)
>  		return ret;
>  
> @@ -192,46 +184,46 @@ static int cros_ec_sensors_probe(struct platform_device *pdev)
>  	state = iio_priv(indio_dev);
>  	for (i = CROS_EC_SENSOR_X; i < CROS_EC_SENSOR_MAX_AXIS; i++) {
>  		/* Common part */
> -		cros_ec_core_channel_init(state->core.channels, i + 1);
> -		state->core.channels[i + 1].info_mask_separate =
> +		cros_ec_core_channel_init(state->channels, i + 1);
> +		state->channels[i + 1].info_mask_separate =
>  			BIT(IIO_CHAN_INFO_RAW) |
>  			BIT(IIO_CHAN_INFO_CALIBBIAS);
> -		state->core.channels[i + 1].info_mask_shared_by_all =
> +		state->channels[i + 1].info_mask_shared_by_all =
>  			BIT(IIO_CHAN_INFO_SCALE) |
>  			BIT(IIO_CHAN_INFO_FREQUENCY) |
>  			BIT(IIO_CHAN_INFO_SAMP_FREQ);
> -		state->core.channels[i + 1].scan_index = i;
> -		state->core.channels[i + 1].modified = 1;
> -		state->core.channels[i + 1].channel2 = IIO_MOD_X + i;
> -		state->core.channels[i + 1].scan_type.sign = 's';
> +		state->channels[i + 1].scan_index = i;
> +		state->channels[i + 1].modified = 1;
> +		state->channels[i + 1].channel2 = IIO_MOD_X + i;
> +		state->channels[i + 1].scan_type.sign = 's';
>  
>  		/* Sensor specific */
> -		switch (state->core.type) {
> +		switch (state->type) {
>  		case MOTIONSENSE_TYPE_ACCEL:
> -			state->core.channels[i + 1].type = IIO_ACCEL;
> +			state->channels[i + 1].type = IIO_ACCEL;
>  			break;
>  		case MOTIONSENSE_TYPE_GYRO:
> -			state->core.channels[i + 1].type = IIO_ANGL_VEL;
> +			state->channels[i + 1].type = IIO_ANGL_VEL;
>  			break;
>  		case MOTIONSENSE_TYPE_MAG:
> -			state->core.channels[i + 1].type = IIO_MAGN;
> +			state->channels[i + 1].type = IIO_MAGN;
>  			break;
>  		default:
>  			dev_err(&pdev->dev, "Unknown motion sensor\n");
>  			return -EINVAL;
>  		}
>  	}
> -	state->core.channels[0].scan_index = CROS_EC_SENSOR_MAX_AXIS;
> +	state->channels[0].scan_index = CROS_EC_SENSOR_MAX_AXIS;
>  
> -	state->core.info.read_raw = &cros_ec_sensors_read;
> -	state->core.info.write_raw = &cros_ec_sensors_write;
> +	state->info.read_raw = &cros_ec_sensors_read;
> +	state->info.write_raw = &cros_ec_sensors_write;
>  
>  	/* There is only enough room for accel and gyro in the io space */
> -	if ((state->core.ec->cmd_readmem != NULL) &&
> -	    (state->core.type != MOTIONSENSE_TYPE_MAG))
> -		state->core.read_ec_sensors_data = cros_ec_sensors_read_lpc;
> +	if ((state->ec->cmd_readmem != NULL) &&
> +	    (state->type != MOTIONSENSE_TYPE_MAG))
> +		state->read_ec_sensors_data = cros_ec_sensors_read_lpc;
>  	else
> -		state->core.read_ec_sensors_data = cros_ec_sensors_read_cmd;
> +		state->read_ec_sensors_data = cros_ec_sensors_read_cmd;
>  
>  	return cros_ec_sensors_core_register(pdev, indio_dev);
>  }
> 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 3880849c5cca..57034e212fe1 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
> @@ -27,7 +27,6 @@ static char *cros_ec_loc[] = {
>  };
>  
>  int cros_ec_sensors_core_init(struct platform_device *pdev,
> -			      int sizeof_priv,
>  			      int num_channels,
>  			      bool physical_device)
>  {
> @@ -40,7 +39,7 @@ int cros_ec_sensors_core_init(struct platform_device *pdev,
>  	if (num_channels > CROS_EC_SENSORS_CORE_MAX_CHANNELS)
>  		return -EINVAL;
>  
> -	indio_dev = devm_iio_device_alloc(dev, sizeof_priv);
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*state));
>  	if (!indio_dev)
>  		return -ENOMEM;
>  
> diff --git a/drivers/iio/light/cros_ec_light_prox.c b/drivers/iio/light/cros_ec_light_prox.c
> index 73f5dcbda0d5..6e1075dc6458 100644
> --- a/drivers/iio/light/cros_ec_light_prox.c
> +++ b/drivers/iio/light/cros_ec_light_prox.c
> @@ -27,23 +27,17 @@
>   */
>  #define CROS_EC_LIGHT_PROX_MAX_CHANNELS (1 + 1)
>  
> -/* State data for ec_sensors iio driver. */
> -struct cros_ec_light_prox_state {
> -	/* Shared by all sensors */
> -	struct cros_ec_sensors_core_state core;
> -};
> -
>  static int cros_ec_light_prox_read(struct iio_dev *indio_dev,
>  				   struct iio_chan_spec const *chan,
>  				   int *val, int *val2, long mask)
>  {
> -	struct cros_ec_light_prox_state *st = iio_priv(indio_dev);
> +	struct cros_ec_sensors_core_state *st = iio_priv(indio_dev);
>  	u16 data = 0;
>  	s64 val64;
>  	int ret = IIO_VAL_INT;
>  	int idx = chan->scan_index;
>  
> -	mutex_lock(&st->core.cmd_lock);
> +	mutex_lock(&st->cmd_lock);
>  
>  	switch (mask) {
>  	case IIO_CHAN_INFO_RAW:
> @@ -77,18 +71,18 @@ static int cros_ec_light_prox_read(struct iio_dev *indio_dev,
>  		}
>  		break;
>  	case IIO_CHAN_INFO_CALIBBIAS:
> -		st->core.param.cmd = MOTIONSENSE_CMD_SENSOR_OFFSET;
> -		st->core.param.sensor_offset.flags = 0;
> +		st->param.cmd = MOTIONSENSE_CMD_SENSOR_OFFSET;
> +		st->param.sensor_offset.flags = 0;
>  
> -		if (cros_ec_motion_send_host_cmd(&st->core, 0)) {
> +		if (cros_ec_motion_send_host_cmd(st, 0)) {
>  			ret = -EIO;
>  			break;
>  		}
>  
>  		/* Save values */
> -		st->core.calib[0] = st->core.resp->sensor_offset.offset[0];
> +		st->calib[0] = st->resp->sensor_offset.offset[0];
>  
> -		*val = st->core.calib[idx];
> +		*val = st->calib[idx];
>  		break;
>  	case IIO_CHAN_INFO_CALIBSCALE:
>  		/*
> @@ -96,26 +90,26 @@ static int cros_ec_light_prox_read(struct iio_dev *indio_dev,
>  		 * scale is a number x.y, where x is coded on 16 bits,
>  		 * y coded on 16 bits, between 0 and 9999.
>  		 */
> -		st->core.param.cmd = MOTIONSENSE_CMD_SENSOR_RANGE;
> -		st->core.param.sensor_range.data = EC_MOTION_SENSE_NO_VALUE;
> +		st->param.cmd = MOTIONSENSE_CMD_SENSOR_RANGE;
> +		st->param.sensor_range.data = EC_MOTION_SENSE_NO_VALUE;
>  
> -		if (cros_ec_motion_send_host_cmd(&st->core, 0)) {
> +		if (cros_ec_motion_send_host_cmd(st, 0)) {
>  			ret = -EIO;
>  			break;
>  		}
>  
> -		val64 = st->core.resp->sensor_range.ret;
> +		val64 = st->resp->sensor_range.ret;
>  		*val = val64 >> 16;
>  		*val2 = (val64 & 0xffff) * 100;
>  		ret = IIO_VAL_INT_PLUS_MICRO;
>  		break;
>  	default:
> -		ret = cros_ec_sensors_core_read(&st->core, chan, val, val2,
> +		ret = cros_ec_sensors_core_read(st, chan, val, val2,
>  						mask);
>  		break;
>  	}
>  
> -	mutex_unlock(&st->core.cmd_lock);
> +	mutex_unlock(&st->cmd_lock);
>  
>  	return ret;
>  }
> @@ -124,37 +118,37 @@ static int cros_ec_light_prox_write(struct iio_dev *indio_dev,
>  			       struct iio_chan_spec const *chan,
>  			       int val, int val2, long mask)
>  {
> -	struct cros_ec_light_prox_state *st = iio_priv(indio_dev);
> +	struct cros_ec_sensors_core_state *st = iio_priv(indio_dev);
>  	int ret = 0;
>  	int idx = chan->scan_index;
>  
> -	mutex_lock(&st->core.cmd_lock);
> +	mutex_lock(&st->cmd_lock);
>  
>  	switch (mask) {
>  	case IIO_CHAN_INFO_CALIBBIAS:
> -		st->core.calib[idx] = val;
> +		st->calib[idx] = val;
>  		/* Send to EC for each axis, even if not complete */
> -		st->core.param.cmd = MOTIONSENSE_CMD_SENSOR_OFFSET;
> -		st->core.param.sensor_offset.flags = MOTION_SENSE_SET_OFFSET;
> -		st->core.param.sensor_offset.offset[0] = st->core.calib[0];
> -		st->core.param.sensor_offset.temp =
> +		st->param.cmd = MOTIONSENSE_CMD_SENSOR_OFFSET;
> +		st->param.sensor_offset.flags = MOTION_SENSE_SET_OFFSET;
> +		st->param.sensor_offset.offset[0] = st->calib[0];
> +		st->param.sensor_offset.temp =
>  					EC_MOTION_SENSE_INVALID_CALIB_TEMP;
> -		if (cros_ec_motion_send_host_cmd(&st->core, 0))
> +		if (cros_ec_motion_send_host_cmd(st, 0))
>  			ret = -EIO;
>  		break;
>  	case IIO_CHAN_INFO_CALIBSCALE:
> -		st->core.param.cmd = MOTIONSENSE_CMD_SENSOR_RANGE;
> -		st->core.param.sensor_range.data = (val << 16) | (val2 / 100);
> -		if (cros_ec_motion_send_host_cmd(&st->core, 0))
> +		st->param.cmd = MOTIONSENSE_CMD_SENSOR_RANGE;
> +		st->param.sensor_range.data = (val << 16) | (val2 / 100);
> +		if (cros_ec_motion_send_host_cmd(st, 0))
>  			ret = -EIO;
>  		break;
>  	default:
> -		ret = cros_ec_sensors_core_write(&st->core, chan, val, val2,
> +		ret = cros_ec_sensors_core_write(st, chan, val, val2,
>  						 mask);
>  		break;
>  	}
>  
> -	mutex_unlock(&st->core.cmd_lock);
> +	mutex_unlock(&st->cmd_lock);
>  
>  	return ret;
>  }
> @@ -164,7 +158,7 @@ static int cros_ec_light_prox_probe(struct platform_device *pdev)
>  	struct device *dev = &pdev->dev;
>  	struct cros_ec_dev *ec_dev = dev_get_drvdata(dev->parent);
>  	struct iio_dev *indio_dev;
> -	struct cros_ec_light_prox_state *state;
> +	struct cros_ec_sensors_core_state *state;
>  	int ret;
>  
>  	if (!ec_dev || !ec_dev->ec_dev) {
> @@ -172,36 +166,35 @@ static int cros_ec_light_prox_probe(struct platform_device *pdev)
>  		return -EINVAL;
>  	}
>  
> -	ret = cros_ec_sensors_core_init(pdev, sizeof(*state),
> -					CROS_EC_LIGHT_PROX_MAX_CHANNELS, true);
> +	ret = cros_ec_sensors_core_init(pdev, CROS_EC_LIGHT_PROX_MAX_CHANNELS, true);
>  	if (ret)
>  		return ret;
>  
>  	indio_dev = platform_get_drvdata(pdev);
>  	state = iio_priv(indio_dev);
> -	state->core.info.read_raw = &cros_ec_light_prox_read;
> -	state->core.info.write_raw = &cros_ec_light_prox_write;
> +	state->info.read_raw = &cros_ec_light_prox_read;
> +	state->info.write_raw = &cros_ec_light_prox_write;
>  
>  	/* Common part */
> -	cros_ec_core_channel_init(state->core.channels, 1);
> -	state->core.channels[1].info_mask_shared_by_all =
> +	cros_ec_core_channel_init(state->channels, 1);
> +	state->channels[1].info_mask_shared_by_all =
>  		BIT(IIO_CHAN_INFO_SAMP_FREQ) |
>  		BIT(IIO_CHAN_INFO_FREQUENCY);
>  
> -	state->core.calib[0] = 0;
> +	state->calib[0] = 0;
>  
>  	/* Sensor specific */
> -	switch (state->core.type) {
> +	switch (state->type) {
>  	case MOTIONSENSE_TYPE_LIGHT:
> -		state->core.channels[1].type = IIO_LIGHT;
> -		state->core.channels[1].info_mask_separate =
> +		state->channels[1].type = IIO_LIGHT;
> +		state->channels[1].info_mask_separate =
>  			BIT(IIO_CHAN_INFO_PROCESSED) |
>  			BIT(IIO_CHAN_INFO_CALIBBIAS) |
>  			BIT(IIO_CHAN_INFO_CALIBSCALE);
>  		break;
>  	case MOTIONSENSE_TYPE_PROX:
> -		state->core.channels[1].type = IIO_PROXIMITY;
> -		state->core.channels[1].info_mask_separate =
> +		state->channels[1].type = IIO_PROXIMITY;
> +		state->channels[1].info_mask_separate =
>  			BIT(IIO_CHAN_INFO_RAW) |
>  			BIT(IIO_CHAN_INFO_CALIBBIAS) |
>  			BIT(IIO_CHAN_INFO_CALIBSCALE);
> @@ -211,7 +204,7 @@ static int cros_ec_light_prox_probe(struct platform_device *pdev)
>  		return -EINVAL;
>  	}
>  
> -	state->core.read_ec_sensors_data = cros_ec_sensors_read_cmd;
> +	state->read_ec_sensors_data = cros_ec_sensors_read_cmd;
>  
>  	return cros_ec_sensors_core_register(pdev, indio_dev);
>  }
> diff --git a/drivers/iio/pressure/cros_ec_baro.c b/drivers/iio/pressure/cros_ec_baro.c
> index f8107a8b8e72..a2703771a6d1 100644
> --- a/drivers/iio/pressure/cros_ec_baro.c
> +++ b/drivers/iio/pressure/cros_ec_baro.c
> @@ -25,22 +25,16 @@
>   */
>  #define CROS_EC_BARO_MAX_CHANNELS (1 + 1)
>  
> -/* State data for ec_sensors iio driver. */
> -struct cros_ec_baro_state {
> -	/* Shared by all sensors */
> -	struct cros_ec_sensors_core_state core;
> -};
> -
>  static int cros_ec_baro_read(struct iio_dev *indio_dev,
>  			     struct iio_chan_spec const *chan,
>  			     int *val, int *val2, long mask)
>  {
> -	struct cros_ec_baro_state *st = iio_priv(indio_dev);
> +	struct cros_ec_sensors_core_state *st = iio_priv(indio_dev);
>  	u16 data = 0;
>  	int ret = IIO_VAL_INT;
>  	int idx = chan->scan_index;
>  
> -	mutex_lock(&st->core.cmd_lock);
> +	mutex_lock(&st->cmd_lock);
>  
>  	switch (mask) {
>  	case IIO_CHAN_INFO_RAW:
> @@ -50,26 +44,26 @@ static int cros_ec_baro_read(struct iio_dev *indio_dev,
>  		*val = data;
>  		break;
>  	case IIO_CHAN_INFO_SCALE:
> -		st->core.param.cmd = MOTIONSENSE_CMD_SENSOR_RANGE;
> -		st->core.param.sensor_range.data = EC_MOTION_SENSE_NO_VALUE;
> +		st->param.cmd = MOTIONSENSE_CMD_SENSOR_RANGE;
> +		st->param.sensor_range.data = EC_MOTION_SENSE_NO_VALUE;
>  
> -		if (cros_ec_motion_send_host_cmd(&st->core, 0)) {
> +		if (cros_ec_motion_send_host_cmd(st, 0)) {
>  			ret = -EIO;
>  			break;
>  		}
> -		*val = st->core.resp->sensor_range.ret;
> +		*val = st->resp->sensor_range.ret;
>  
>  		/* scale * in_pressure_raw --> kPa */
>  		*val2 = 10 << CROS_EC_SENSOR_BITS;
>  		ret = IIO_VAL_FRACTIONAL;
>  		break;
>  	default:
> -		ret = cros_ec_sensors_core_read(&st->core, chan, val, val2,
> +		ret = cros_ec_sensors_core_read(st, chan, val, val2,
>  						mask);
>  		break;
>  	}
>  
> -	mutex_unlock(&st->core.cmd_lock);
> +	mutex_unlock(&st->cmd_lock);
>  
>  	return ret;
>  }
> @@ -78,29 +72,28 @@ static int cros_ec_baro_write(struct iio_dev *indio_dev,
>  			      struct iio_chan_spec const *chan,
>  			      int val, int val2, long mask)
>  {
> -	struct cros_ec_baro_state *st = iio_priv(indio_dev);
> +	struct cros_ec_sensors_core_state *st = iio_priv(indio_dev);
>  	int ret = 0;
>  
> -	mutex_lock(&st->core.cmd_lock);
> +	mutex_lock(&st->cmd_lock);
>  
>  	switch (mask) {
>  	case IIO_CHAN_INFO_SCALE:
> -		st->core.param.cmd = MOTIONSENSE_CMD_SENSOR_RANGE;
> -		st->core.param.sensor_range.data = val;
> +		st->param.cmd = MOTIONSENSE_CMD_SENSOR_RANGE;
> +		st->param.sensor_range.data = val;
>  
>  		/* Always roundup, so caller gets at least what it asks for. */
> -		st->core.param.sensor_range.roundup = 1;
> +		st->param.sensor_range.roundup = 1;
>  
> -		if (cros_ec_motion_send_host_cmd(&st->core, 0))
> +		if (cros_ec_motion_send_host_cmd(st, 0))
>  			ret = -EIO;
>  		break;
>  	default:
> -		ret = cros_ec_sensors_core_write(&st->core, chan, val, val2,
> -						 mask);
> +		ret = cros_ec_sensors_core_write(st, chan, val, val2, mask);
>  		break;
>  	}
>  
> -	mutex_unlock(&st->core.cmd_lock);
> +	mutex_unlock(&st->cmd_lock);
>  
>  	return ret;
>  }
> @@ -110,7 +103,7 @@ static int cros_ec_baro_probe(struct platform_device *pdev)
>  	struct device *dev = &pdev->dev;
>  	struct cros_ec_dev *ec_dev = dev_get_drvdata(dev->parent);
>  	struct iio_dev *indio_dev;
> -	struct cros_ec_baro_state *state;
> +	struct cros_ec_sensors_core_state *state;
>  	int ret;
>  
>  	if (!ec_dev || !ec_dev->ec_dev) {
> @@ -118,37 +111,36 @@ static int cros_ec_baro_probe(struct platform_device *pdev)
>  		return -EINVAL;
>  	}
>  
> -	ret = cros_ec_sensors_core_init(pdev, sizeof(*state),
> -					CROS_EC_BARO_MAX_CHANNELS, true);
> +	ret = cros_ec_sensors_core_init(pdev, CROS_EC_BARO_MAX_CHANNELS, true);
>  	if (ret)
>  		return ret;
>  
>  	indio_dev = platform_get_drvdata(pdev);
>  	state = iio_priv(indio_dev);
> -	state->core.info.read_raw = &cros_ec_baro_read;
> -	state->core.info.write_raw = &cros_ec_baro_write;
> +	state->info.read_raw = &cros_ec_baro_read;
> +	state->info.write_raw = &cros_ec_baro_write;
>  
>  	/* Common part */
> -	cros_ec_core_channel_init(state->core.channels, 1);
> -	state->core.channels[1].info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
> -	state->core.channels[1].info_mask_shared_by_all =
> +	cros_ec_core_channel_init(state->channels, 1);
> +	state->channels[1].info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
> +	state->channels[1].info_mask_shared_by_all =
>  		BIT(IIO_CHAN_INFO_SCALE) |
>  		BIT(IIO_CHAN_INFO_SAMP_FREQ) |
>  		BIT(IIO_CHAN_INFO_FREQUENCY);
>  
> -	state->core.calib[0] = 0;
> +	state->calib[0] = 0;
>  
>  	/* Sensor specific */
> -	switch (state->core.type) {
> +	switch (state->type) {
>  	case MOTIONSENSE_TYPE_BARO:
> -		state->core.channels[1].type = IIO_PRESSURE;
> +		state->channels[1].type = IIO_PRESSURE;
>  		break;
>  	default:
>  		dev_warn(dev, "Unknown motion sensor\n");
>  		return -EINVAL;
>  	}
>  
> -	state->core.read_ec_sensors_data = cros_ec_sensors_read_cmd;
> +	state->read_ec_sensors_data = cros_ec_sensors_read_cmd;
>  
>  	return cros_ec_sensors_core_register(pdev, indio_dev);
>  }
> diff --git a/include/linux/iio/common/cros_ec_sensors_core.h b/include/linux/iio/common/cros_ec_sensors_core.h
> index 60f40d253f4a..3e6de427076e 100644
> --- a/include/linux/iio/common/cros_ec_sensors_core.h
> +++ b/include/linux/iio/common/cros_ec_sensors_core.h
> @@ -105,14 +105,13 @@ struct platform_device;
>  /**
>   * cros_ec_sensors_core_init() - basic initialization of the core structure
>   * @pdev:		platform device created for the sensors
> - * @sizeof_priv:	size of the private structure
>   * @num_channels:	Number of channel
>   * @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,
> -			      int sizeof_priv, int num_channels,
> +			      int num_channels,
>  			      bool physical_device);
>  
>  /**

Re: [PATCH v3 6/8] iio: common: cros_ec_sensors: support protocol v3 message

[Enric Balletbo i Serra]

Hi Fabien, Jonathan,

cc'ing Gwendal and Enrico who might be interested on this patch.

It'd be nice if we can land this patch before [1], otherwise the legacy support
for cros-ec sensors on veyron minnie won't work and we will mess the kernel log
with a couple of errors.

I just have a few comments that I think should be quick to respin.

[1] https://lkml.org/lkml/2019/6/24/1464

On 22/6/19 12:15, Jonathan Cameron wrote:
> On Tue, 18 Jun 2019 11:06:37 +0200
> Fabien Lahoudere <fabien.lahoudere@collabora.com> wrote:
> 
>> Version 3 of the EC protocol provides min and max frequencies for EC sensors.
>>

I think we are mixing two things. One is determine what version of the
MOTIONSENSE command the EC has, and another one is add some default values
supported by the third version. I'd split this in two separate patches, and fix
the subject and the commit description.


>> Signed-off-by: Fabien Lahoudere <fabien.lahoudere@collabora.com>
>> Signed-off-by: Nick Vaccaro <nvaccaro@chromium.org>
> Looks good to me. I'll pick up next time if no one else raises any
> issues on this one.
> 
> Thanks,
> 
> Jonathan
> 
>> ---
>>  .../cros_ec_sensors/cros_ec_sensors_core.c    | 85 ++++++++++++++++++-
>>  .../linux/iio/common/cros_ec_sensors_core.h   |  3 +
>>  2 files changed, 87 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 57034e212fe1..2ce077b576a4 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
>> @@ -26,6 +26,66 @@ 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;
>> +	}
>> +}

This is the second part. It adds default values for version 3. I'd send this
part on the patch that adds support min/max freq.

>> +
>> +static int cros_ec_get_host_cmd_version_mask(struct cros_ec_device *ec_dev,
>> +					     u16 cmd_offset, u16 cmd, u32 *mask)
>> +{
>> +	int ret;
>> +	struct {
>> +		struct cros_ec_command msg;
>> +		union {
>> +			struct ec_params_get_cmd_versions params;
>> +			struct ec_response_get_cmd_versions resp;
>> +		};
>> +	} __packed buf = {
>> +		.msg = {
>> +			.command = EC_CMD_GET_CMD_VERSIONS + cmd_offset,
>> +			.insize = sizeof(struct ec_response_get_cmd_versions),
>> +			.outsize = sizeof(struct ec_params_get_cmd_versions)
>> +			},
>> +		.params = {.cmd = cmd}
>> +	};
>> +

nit: Actually when someone is sending a command to the EC there is a bit of mess
how to do it, some use dynamic allocations, other static. IMO is more readable
have something that explicitly initializes the struct and then assigns the
different fields. Something like this:


        struct {
                struct cros_ec_command cmd;
                union {
                        struct ec_params_get_cmd_versions params;
                        struct ec_response_get_cmd_versions resp;
                };
        } __packed msg = {};
        int ret;

        msg.cmd.command = EC_CMD_GET_CMD_VERSIONS + cmd_offset;
        msg.cmd.insize = sizeof(msg.resp);
        msg.cmd.outsize = sizeof(msg.params);
        msg.params.cmd = cmd;


>> +	ret = cros_ec_cmd_xfer_status(ec_dev, &buf.msg);
>> +	if (ret >= 0) {
>> +		if (buf.msg.result == EC_RES_SUCCESS)

Note that cros_ec_cmd_xfer_status returns a <0 on error and 0 or positive number
when EC_RES_SUCCESS. So no need to double check the result.

>> +			*mask = buf.resp.version_mask;
>> +		else
>> +			*mask = 0;
>> +	}
>> +	return ret;

So, I think that all this can be reworked as

        ret = cros_ec_cmd_xfer_status(ec_dev, &buf.msg);
        if (ret < 0)
                return ret;

        *mask = msg.resp.version_mask;

        return 0;


>> +}
>> +
>>  int cros_ec_sensors_core_init(struct platform_device *pdev,
>>  			      int num_channels,
>>  			      bool physical_device)
>> @@ -35,6 +95,8 @@ int cros_ec_sensors_core_init(struct platform_device *pdev,
>>  	struct cros_ec_dev *ec = dev_get_drvdata(pdev->dev.parent);
>>  	struct cros_ec_sensor_platform *sensor_platform = dev_get_platdata(dev);
>>  	struct iio_dev *indio_dev;
>> +	u32 ver_mask;
>> +	int ret;
>>  
>>  	if (num_channels > CROS_EC_SENSORS_CORE_MAX_CHANNELS)
>>  		return -EINVAL;
>> @@ -57,8 +119,16 @@ int cros_ec_sensors_core_init(struct platform_device *pdev,
>>  
>>  	mutex_init(&state->cmd_lock);
>>  
>> +	/* determine what version of MOTIONSENSE CMD EC has */

nit: Capitalize

>> +	ret = cros_ec_get_host_cmd_version_mask(state->ec,
>> +						ec->cmd_offset,
>> +						EC_CMD_MOTION_SENSE_CMD,
>> +						&ver_mask);

It will return <0 on error

>> +	if (ret < 0 || ver_mask == 0)
>> +		return -ENODEV;
>> +

so no need to check ver_mask

        if (ret < 0)
		return ret;


>>  	/* Set up the host command structure. */
>> -	state->msg->version = 2;
>> +	state->msg->version = fls(ver_mask) - 1;
>>  	state->msg->command = EC_CMD_MOTION_SENSE_CMD + ec->cmd_offset;
>>  	state->msg->outsize = sizeof(struct ec_params_motion_sense);
>>  
>> @@ -76,6 +146,19 @@ int cros_ec_sensors_core_init(struct platform_device *pdev,
>>  		}
>>  		state->type = state->resp->info.type;
>>  		state->loc = state->resp->info.location;
>> +
>> +		/* Value to stop the device */
>> +		state->frequency_range[0] = 0;
>> +		if (state->msg->version < 3) {
>> +			get_default_min_max_freq(state->resp->info.type,
>> +						 &state->frequency_range[1],
>> +						 &state->frequency_range[2]);
>> +		} else {
>> +			state->frequency_range[1] =
>> +			    state->resp->info_3.min_frequency;
>> +			state->frequency_range[2] =
>> +			    state->resp->info_3.max_frequency;
>> +		}

This is part of the second patch.

>>  	}
>>  
>>  	indio_dev->info = &state->info;
>> diff --git a/include/linux/iio/common/cros_ec_sensors_core.h b/include/linux/iio/common/cros_ec_sensors_core.h
>> index 3e6de427076e..89937ad242ef 100644
>> --- a/include/linux/iio/common/cros_ec_sensors_core.h
>> +++ b/include/linux/iio/common/cros_ec_sensors_core.h
>> @@ -74,6 +74,9 @@ struct cros_ec_sensors_core_state {
>>  	int curr_sampl_freq;
>>  	struct iio_info info;
>>  	struct iio_chan_spec channels[CROS_EC_SENSORS_CORE_MAX_CHANNELS];
>> +
>> +	/* Disable, Min and Max Sampling Frequency in mHz */
>> +	int frequency_range[3];
>>  };
>>  
>>  /**
> 
> 

As I said I'd send a first patch with the EC protocol bits separated of this
patchset and create a second patch into this patchset with the min/max frequency
bits.

Thanks,
~ Enric

Re: [PATCH v3 6/8] iio: common: cros_ec_sensors: support protocol v3 message

[Fabien Lahoudere]

Hi Enric

I will split in two patches and send it soon.
Nevertheless, I won't change the structure initialisation because it
was requested on a previous comment.
I will fix other comments.

Thanks for reviewing

Le mardi 25 juin 2019 à 19:04 +0200, Enric Balletbo i Serra a écrit :
> Hi Fabien, Jonathan,
> 
> cc'ing Gwendal and Enrico who might be interested on this patch.
> 
> It'd be nice if we can land this patch before [1], otherwise the
> legacy support
> for cros-ec sensors on veyron minnie won't work and we will mess the
> kernel log
> with a couple of errors.
> 
> I just have a few comments that I think should be quick to respin.
> 
> [1] https://lkml.org/lkml/2019/6/24/1464
> 
> On 22/6/19 12:15, Jonathan Cameron wrote:
> > On Tue, 18 Jun 2019 11:06:37 +0200
> > Fabien Lahoudere <fabien.lahoudere@collabora.com> wrote:
> > 
> > > Version 3 of the EC protocol provides min and max frequencies for
> > > EC sensors.
> > > 
> 
> I think we are mixing two things. One is determine what version of
> the
> MOTIONSENSE command the EC has, and another one is add some default
> values
> supported by the third version. I'd split this in two separate
> patches, and fix
> the subject and the commit description.
> 
> 
> > > Signed-off-by: Fabien Lahoudere <fabien.lahoudere@collabora.com>
> > > Signed-off-by: Nick Vaccaro <nvaccaro@chromium.org>
> > Looks good to me. I'll pick up next time if no one else raises any
> > issues on this one.
> > 
> > Thanks,
> > 
> > Jonathan
> > 
> > > ---
> > >  .../cros_ec_sensors/cros_ec_sensors_core.c    | 85
> > > ++++++++++++++++++-
> > >  .../linux/iio/common/cros_ec_sensors_core.h   |  3 +
> > >  2 files changed, 87 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 57034e212fe1..2ce077b576a4 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
> > > @@ -26,6 +26,66 @@ 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;
> > > +	}
> > > +}
> 
> This is the second part. It adds default values for version 3. I'd
> send this
> part on the patch that adds support min/max freq.
> 
> > > +
> > > +static int cros_ec_get_host_cmd_version_mask(struct
> > > cros_ec_device *ec_dev,
> > > +					     u16 cmd_offset, u16 cmd,
> > > u32 *mask)
> > > +{
> > > +	int ret;
> > > +	struct {
> > > +		struct cros_ec_command msg;
> > > +		union {
> > > +			struct ec_params_get_cmd_versions params;
> > > +			struct ec_response_get_cmd_versions resp;
> > > +		};
> > > +	} __packed buf = {
> > > +		.msg = {
> > > +			.command = EC_CMD_GET_CMD_VERSIONS +
> > > cmd_offset,
> > > +			.insize = sizeof(struct
> > > ec_response_get_cmd_versions),
> > > +			.outsize = sizeof(struct
> > > ec_params_get_cmd_versions)
> > > +			},
> > > +		.params = {.cmd = cmd}
> > > +	};
> > > +
> 
> nit: Actually when someone is sending a command to the EC there is a
> bit of mess
> how to do it, some use dynamic allocations, other static. IMO is more
> readable
> have something that explicitly initializes the struct and then
> assigns the
> different fields. Something like this:
> 
> 
>         struct {
>                 struct cros_ec_command cmd;
>                 union {
>                         struct ec_params_get_cmd_versions params;
>                         struct ec_response_get_cmd_versions resp;
>                 };
>         } __packed msg = {};
>         int ret;
> 
>         msg.cmd.command = EC_CMD_GET_CMD_VERSIONS + cmd_offset;
>         msg.cmd.insize = sizeof(msg.resp);
>         msg.cmd.outsize = sizeof(msg.params);
>         msg.params.cmd = cmd;
> 
> 
> > > +	ret = cros_ec_cmd_xfer_status(ec_dev, &buf.msg);
> > > +	if (ret >= 0) {
> > > +		if (buf.msg.result == EC_RES_SUCCESS)
> 
> Note that cros_ec_cmd_xfer_status returns a <0 on error and 0 or
> positive number
> when EC_RES_SUCCESS. So no need to double check the result.
> 
> > > +			*mask = buf.resp.version_mask;
> > > +		else
> > > +			*mask = 0;
> > > +	}
> > > +	return ret;
> 
> So, I think that all this can be reworked as
> 
>         ret = cros_ec_cmd_xfer_status(ec_dev, &buf.msg);
>         if (ret < 0)
>                 return ret;
> 
>         *mask = msg.resp.version_mask;
> 
>         return 0;
> 
> 
> > > +}
> > > +
> > >  int cros_ec_sensors_core_init(struct platform_device *pdev,
> > >  			      int num_channels,
> > >  			      bool physical_device)
> > > @@ -35,6 +95,8 @@ int cros_ec_sensors_core_init(struct
> > > platform_device *pdev,
> > >  	struct cros_ec_dev *ec = dev_get_drvdata(pdev->dev.parent);
> > >  	struct cros_ec_sensor_platform *sensor_platform =
> > > dev_get_platdata(dev);
> > >  	struct iio_dev *indio_dev;
> > > +	u32 ver_mask;
> > > +	int ret;
> > >  
> > >  	if (num_channels > CROS_EC_SENSORS_CORE_MAX_CHANNELS)
> > >  		return -EINVAL;
> > > @@ -57,8 +119,16 @@ int cros_ec_sensors_core_init(struct
> > > platform_device *pdev,
> > >  
> > >  	mutex_init(&state->cmd_lock);
> > >  
> > > +	/* determine what version of MOTIONSENSE CMD EC has */
> 
> nit: Capitalize
> 
> > > +	ret = cros_ec_get_host_cmd_version_mask(state->ec,
> > > +						ec->cmd_offset,
> > > +						EC_CMD_MOTION_SENSE_CMD
> > > ,
> > > +						&ver_mask);
> 
> It will return <0 on error
> 
> > > +	if (ret < 0 || ver_mask == 0)
> > > +		return -ENODEV;
> > > +
> 
> so no need to check ver_mask
> 
>         if (ret < 0)
> 		return ret;
> 
> 
> > >  	/* Set up the host command structure. */
> > > -	state->msg->version = 2;
> > > +	state->msg->version = fls(ver_mask) - 1;
> > >  	state->msg->command = EC_CMD_MOTION_SENSE_CMD + ec->cmd_offset;
> > >  	state->msg->outsize = sizeof(struct ec_params_motion_sense);
> > >  
> > > @@ -76,6 +146,19 @@ int cros_ec_sensors_core_init(struct
> > > platform_device *pdev,
> > >  		}
> > >  		state->type = state->resp->info.type;
> > >  		state->loc = state->resp->info.location;
> > > +
> > > +		/* Value to stop the device */
> > > +		state->frequency_range[0] = 0;
> > > +		if (state->msg->version < 3) {
> > > +			get_default_min_max_freq(state->resp-
> > > >info.type,
> > > +						 &state-
> > > >frequency_range[1],
> > > +						 &state-
> > > >frequency_range[2]);
> > > +		} else {
> > > +			state->frequency_range[1] =
> > > +			    state->resp->info_3.min_frequency;
> > > +			state->frequency_range[2] =
> > > +			    state->resp->info_3.max_frequency;
> > > +		}
> 
> This is part of the second patch.
> 
> > >  	}
> > >  
> > >  	indio_dev->info = &state->info;
> > > diff --git a/include/linux/iio/common/cros_ec_sensors_core.h
> > > b/include/linux/iio/common/cros_ec_sensors_core.h
> > > index 3e6de427076e..89937ad242ef 100644
> > > --- a/include/linux/iio/common/cros_ec_sensors_core.h
> > > +++ b/include/linux/iio/common/cros_ec_sensors_core.h
> > > @@ -74,6 +74,9 @@ struct cros_ec_sensors_core_state {
> > >  	int curr_sampl_freq;
> > >  	struct iio_info info;
> > >  	struct iio_chan_spec
> > > channels[CROS_EC_SENSORS_CORE_MAX_CHANNELS];
> > > +
> > > +	/* Disable, Min and Max Sampling Frequency in mHz */
> > > +	int frequency_range[3];
> > >  };
> > >  
> > >  /**
> 
> As I said I'd send a first patch with the EC protocol bits separated
> of this
> patchset and create a second patch into this patchset with the
> min/max frequency
> bits.
> 
> Thanks,
> ~ Enric

Re: [PATCH v3 8/8] docs: iio: add precision about sampling_frequency_available

[Fabien Lahoudere]

Le samedi 22 juin 2019 à 11:21 +0100, Jonathan Cameron a écrit :
> On Tue, 18 Jun 2019 11:06:39 +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>
> It seems I already applied this one, though probably haven't sent
> a pull request for it to Greg yet.
> 
> Please drop it from your v4 posting as otherwise I'll get confused
> (again).
> 

Sorry, it will be dropped next time.

> 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/s
> > ampling_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_r
> > atio
> >  KernelVersion:	2.6.38