[RFC PATCH v2 0/1] Adding support for IIO SCMI based sensors

[RFC PATCH v2 0/1] Adding support for IIO SCMI based sensors

[Jyoti Bhayana]

Hi,

This series adds support for ARM SCMI Protocol based IIO Device.
This driver provides support for Accelerometer and Gyroscope sensor
using new SCMI Sensor Protocol defined by the upcoming SCMIv3.0 ARM
specification, which is available at 

https://developer.arm.com/documentation/den0056/c/

The series is currently based on top of:

commit f83eb664cdb4 ("Merge tag 'scmi-voltage-5.11' of git://git.kernel.org/pub/scm/linux/kernel/git/sudeep.holla/linux into for-next/scmi")

in Sudeep for-next/scmi branch:

https://git.kernel.org/pub/scm/linux/kernel/git/sudeep.holla/\
linux.git/log/?h=for-next/scmi

This version of the patch series has been tested using 
version 5.4.21 branch of Android common kernel.

Any feedback welcome,

Thanks,

Jyoti Bhayana

v1 --> v2
- Incorporated the feedback comments from v1 review of the patch
- Regarding the new ABI for sensor_power,sensor_max_range,
and sensor_resolution, these are some of the sensor attributes
which Android passes to the apps. If there is any other way of getting
those values, please let us know

Jyoti Bhayana (1):
  iio/scmi: Adding support for IIO SCMI Based Sensors

 MAINTAINERS                                |   6 +
 drivers/iio/common/Kconfig                 |   1 +
 drivers/iio/common/Makefile                |   1 +
 drivers/iio/common/scmi_sensors/Kconfig    |  18 +
 drivers/iio/common/scmi_sensors/Makefile   |   5 +
 drivers/iio/common/scmi_sensors/scmi_iio.c | 693 +++++++++++++++++++++
 6 files changed, 724 insertions(+)
 create mode 100644 drivers/iio/common/scmi_sensors/Kconfig
 create mode 100644 drivers/iio/common/scmi_sensors/Makefile
 create mode 100644 drivers/iio/common/scmi_sensors/scmi_iio.c

-- 
2.29.2.729.g45daf8777d-goog

[RFC PATCH v2 1/1] iio/scmi: Adding support for IIO SCMI Based Sensors

[Jyoti Bhayana]

This change provides ARM SCMI Protocol based IIO device.
This driver provides support for Accelerometer and Gyroscope using
new SCMI Sensor Protocol defined by the upcoming SCMIv3.0
ARM specification

Signed-off-by: Jyoti Bhayana <jbhayana@google.com>
---
 MAINTAINERS                                |   6 +
 drivers/iio/common/Kconfig                 |   1 +
 drivers/iio/common/Makefile                |   1 +
 drivers/iio/common/scmi_sensors/Kconfig    |  18 +
 drivers/iio/common/scmi_sensors/Makefile   |   5 +
 drivers/iio/common/scmi_sensors/scmi_iio.c | 696 +++++++++++++++++++++
 6 files changed, 727 insertions(+)
 create mode 100644 drivers/iio/common/scmi_sensors/Kconfig
 create mode 100644 drivers/iio/common/scmi_sensors/Makefile
 create mode 100644 drivers/iio/common/scmi_sensors/scmi_iio.c

diff --git a/MAINTAINERS b/MAINTAINERS
index b516bb34a8d5..ccf37d43ab41 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -8567,6 +8567,12 @@ S:	Maintained
 F:	Documentation/devicetree/bindings/iio/multiplexer/io-channel-mux.txt
 F:	drivers/iio/multiplexer/iio-mux.c
 
+IIO SCMI BASED DRIVER
+M:	Jyoti Bhayana <jbhayana@google.com>
+L:	linux-iio@vger.kernel.org
+S:	Maintained
+F:	drivers/iio/common/scmi_sensors/scmi_iio.c
+
 IIO SUBSYSTEM AND DRIVERS
 M:	Jonathan Cameron <jic23@kernel.org>
 R:	Lars-Peter Clausen <lars@metafoo.de>
diff --git a/drivers/iio/common/Kconfig b/drivers/iio/common/Kconfig
index 2b9ee9161abd..0334b4954773 100644
--- a/drivers/iio/common/Kconfig
+++ b/drivers/iio/common/Kconfig
@@ -6,5 +6,6 @@
 source "drivers/iio/common/cros_ec_sensors/Kconfig"
 source "drivers/iio/common/hid-sensors/Kconfig"
 source "drivers/iio/common/ms_sensors/Kconfig"
+source "drivers/iio/common/scmi_sensors/Kconfig"
 source "drivers/iio/common/ssp_sensors/Kconfig"
 source "drivers/iio/common/st_sensors/Kconfig"
diff --git a/drivers/iio/common/Makefile b/drivers/iio/common/Makefile
index 4bc30bb548e2..fad40e1e1718 100644
--- a/drivers/iio/common/Makefile
+++ b/drivers/iio/common/Makefile
@@ -11,5 +11,6 @@
 obj-y += cros_ec_sensors/
 obj-y += hid-sensors/
 obj-y += ms_sensors/
+obj-y += scmi_sensors/
 obj-y += ssp_sensors/
 obj-y += st_sensors/
diff --git a/drivers/iio/common/scmi_sensors/Kconfig b/drivers/iio/common/scmi_sensors/Kconfig
new file mode 100644
index 000000000000..67e084cbb1ab
--- /dev/null
+++ b/drivers/iio/common/scmi_sensors/Kconfig
@@ -0,0 +1,18 @@
+#
+# IIO over SCMI
+#
+# When adding new entries keep the list in alphabetical order
+
+menu "IIO SCMI Sensors"
+
+config IIO_SCMI
+	tristate "IIO SCMI"
+        depends on ARM_SCMI_PROTOCOL
+        select IIO_BUFFER
+        select IIO_KFIFO_BUF
+	help
+          Say yes here to build support for IIO SCMI Driver.
+          This provides ARM SCMI Protocol based IIO device.
+          This driver provides support for accelerometer and gyroscope
+          sensors available on SCMI based platforms.
+endmenu
diff --git a/drivers/iio/common/scmi_sensors/Makefile b/drivers/iio/common/scmi_sensors/Makefile
new file mode 100644
index 000000000000..f13140a2575a
--- /dev/null
+++ b/drivers/iio/common/scmi_sensors/Makefile
@@ -0,0 +1,5 @@
+# SPDX - License - Identifier : GPL - 2.0 - only
+#
+# Makefile for the IIO over SCMI
+#
+obj-$(CONFIG_IIO_SCMI) += scmi_iio.o
diff --git a/drivers/iio/common/scmi_sensors/scmi_iio.c b/drivers/iio/common/scmi_sensors/scmi_iio.c
new file mode 100644
index 000000000000..2f5c884e22ab
--- /dev/null
+++ b/drivers/iio/common/scmi_sensors/scmi_iio.c
@@ -0,0 +1,696 @@
+// SPDX-License-Identifier: GPL-2.0
+
+/*
+ * System Control and Management Interface(SCMI) based IIO sensor driver
+ *
+ * Copyright (C) 2020 Google LLC
+ */
+
+#include <linux/delay.h>
+#include <linux/err.h>
+#include <linux/iio/buffer.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/kfifo_buf.h>
+#include <linux/iio/sysfs.h>
+#include <linux/kernel.h>
+#include <linux/kthread.h>
+#include <linux/module.h>
+#include <linux/scmi_protocol.h>
+#include <linux/time.h>
+#include <linux/types.h>
+
+#define NSEC_MULT_POW_10 (const_ilog2(NSEC_PER_SEC) / const_ilog2(10))
+#define UHZ_PER_HZ 1000000UL
+#define ODR_EXPAND(odr, uodr) (((odr)*1000000ULL) + (uodr))
+#define MAX_NUM_OF_CHANNELS 4
+
+struct scmi_iio_priv {
+	struct scmi_handle *handle;
+	const struct scmi_sensor_info *sensor_info;
+	struct iio_dev *indio_dev;
+	long long iio_buf[MAX_NUM_OF_CHANNELS];
+	struct notifier_block sensor_update_nb;
+	u32 *freq_avail;
+};
+
+struct sensor_freq {
+	u64 hz;
+	u64 uhz;
+};
+
+static int sensor_update_cb(struct notifier_block *nb, unsigned long event,
+			    void *data)
+{
+	struct scmi_sensor_update_report *sensor_update = data;
+	u64 time, time_ns;
+	s64 sensor_value;
+	struct iio_dev *scmi_iio_dev;
+	s8 tstamp_scale_ns;
+	int i;
+	struct scmi_iio_priv *sensor =
+		container_of(nb, struct scmi_iio_priv, sensor_update_nb);
+
+	scmi_iio_dev = sensor->indio_dev;
+
+	for (i = 0; i < sensor_update->readings_count; i++) {
+		sensor_value = sensor_update->readings[i].value;
+		time = sensor_update->readings[i].timestamp;
+		sensor->iio_buf[i] = sensor_value;
+	}
+
+	if (!sensor->sensor_info->timestamped) {
+		time_ns = iio_get_time_ns(scmi_iio_dev);
+	} else {
+		tstamp_scale_ns =
+			sensor->sensor_info->tstamp_scale + NSEC_MULT_POW_10;
+		if (tstamp_scale_ns < 0) {
+			tstamp_scale_ns = -1 * tstamp_scale_ns;
+			time_ns = div64_u64(time, int_pow(10, tstamp_scale_ns));
+		} else {
+			time_ns = time * int_pow(10, tstamp_scale_ns);
+		}
+	}
+
+	iio_push_to_buffers_with_timestamp(scmi_iio_dev, sensor->iio_buf,
+					   time_ns);
+
+	return NOTIFY_OK;
+}
+
+static int scmi_iio_buffer_preenable(struct iio_dev *dev)
+{
+	struct scmi_iio_priv *sensor = iio_priv(dev);
+	u32 sensor_id = sensor->sensor_info->id;
+	u32 sensor_config;
+	int err;
+
+	if (sensor->sensor_info->timestamped)
+		sensor_config |= FIELD_PREP(SCMI_SENS_CFG_TSTAMP_ENABLED_MASK,
+					    SCMI_SENS_CFG_TSTAMP_ENABLE);
+
+	sensor_config |= FIELD_PREP(SCMI_SENS_CFG_SENSOR_ENABLED_MASK,
+				    SCMI_SENS_CFG_SENSOR_ENABLE);
+
+	err = sensor->handle->notify_ops->register_event_notifier(
+		sensor->handle, SCMI_PROTOCOL_SENSOR, SCMI_EVENT_SENSOR_UPDATE,
+		&sensor_id, &sensor->sensor_update_nb);
+	if (err) {
+		pr_err("Error in registering sensor update notifier for sensor %s err %d",
+		       sensor->sensor_info->name, err);
+		return err;
+	}
+
+	err = sensor->handle->sensor_ops->config_set(
+		sensor->handle, sensor->sensor_info->id, sensor_config);
+	if (err)
+		pr_err("Error in enabling sensor %s err %d",
+		       sensor->sensor_info->name, err);
+
+	return err;
+}
+
+static int scmi_iio_buffer_postdisable(struct iio_dev *iio_dev)
+{
+	struct scmi_iio_priv *sensor = iio_priv(iio_dev);
+	u32 sensor_id = sensor->sensor_info->id;
+	u32 sensor_config = 0;
+	int err;
+
+	sensor_config |= FIELD_PREP(SCMI_SENS_CFG_SENSOR_ENABLED_MASK,
+				    SCMI_SENS_CFG_SENSOR_DISABLE);
+
+	err = sensor->handle->notify_ops->unregister_event_notifier(
+		sensor->handle, SCMI_PROTOCOL_SENSOR, SCMI_EVENT_SENSOR_UPDATE,
+		&sensor_id, &sensor->sensor_update_nb);
+	if (err) {
+		pr_err("Error in unregistering sensor update notifier for sensor %s err %d",
+		       sensor->sensor_info->name, err);
+		return err;
+	}
+
+	err = sensor->handle->sensor_ops->config_set(sensor->handle, sensor_id,
+						     sensor_config);
+	if (err)
+		pr_err("Error in disabling sensor %s with err %d",
+		       sensor->sensor_info->name, err);
+
+	return err;
+}
+
+static const struct iio_buffer_setup_ops scmi_iio_buffer_ops = {
+	.preenable = scmi_iio_buffer_preenable,
+	.postdisable = scmi_iio_buffer_postdisable,
+};
+
+static int scmi_iio_read_avail(struct iio_dev *iio_dev,
+					 struct iio_chan_spec const *chan,
+					 const int **vals,
+					 int *type, int *length, long mask)
+{
+	struct scmi_iio_priv *sensor = iio_priv(iio_dev);
+
+	switch (mask) {
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		*vals = sensor->freq_avail;
+		*type = IIO_VAL_INT_PLUS_MICRO;
+		*length = sensor->sensor_info->intervals.count * 2;
+		if (sensor->sensor_info->intervals.segmented)
+			return IIO_AVAIL_RANGE;
+		else
+			return IIO_AVAIL_LIST;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int scmi_iio_set_odr_val(struct iio_dev *iio_dev, int val, int val2)
+{
+	struct scmi_iio_priv *sensor = iio_priv(iio_dev);
+	u32 sensor_config;
+	u64 sec, mult, uHz;
+	char buf[32];
+
+	int err = sensor->handle->sensor_ops->config_get(
+		sensor->handle, sensor->sensor_info->id, &sensor_config);
+	if (err) {
+		pr_err("%s: Error in getting sensor config for sensor %s err %d",
+		       __func__, sensor->sensor_info->name, err);
+		return err;
+	}
+
+	uHz = ODR_EXPAND(val, val2);
+
+	/*
+	 * The seconds field in the sensor interval in SCMI is 16 bits long
+	 * Therefore seconds  = 1/Hz <= 0xFFFF. As floating point calculations are
+	 * discouraged in the kernel driver code, to calculate the scale factor (sf)
+	 * (1* 1000000 * sf)/uHz <= 0xFFFF. Therefore, sf <= (uHz * 0xFFFF)/1000000
+	 *  To calculate the multiplier,we convert the sf into char string  and
+	 *  count the number of characters
+	 */
+
+	mult = scnprintf(buf, 32, "%llu", ((u64)uHz * 0xFFFF) / UHZ_PER_HZ) - 1;
+
+	sec = div64_u64(int_pow(10, mult) * UHZ_PER_HZ, uHz);
+	if (sec == 0) {
+		pr_err("Trying to set invalid sensor update value for sensor %s",
+		       sensor->sensor_info->name);
+		return -EINVAL;
+	}
+
+	sensor_config &= ~SCMI_SENS_CFG_UPDATE_SECS_MASK;
+	sensor_config |= FIELD_PREP(SCMI_SENS_CFG_UPDATE_SECS_MASK, sec);
+	sensor_config &= ~SCMI_SENS_CFG_UPDATE_EXP_MASK;
+	sensor_config |= FIELD_PREP(SCMI_SENS_CFG_UPDATE_EXP_MASK, -mult);
+
+	if (sensor->sensor_info->timestamped) {
+		sensor_config &= ~SCMI_SENS_CFG_TSTAMP_ENABLED_MASK;
+		sensor_config |= FIELD_PREP(SCMI_SENS_CFG_TSTAMP_ENABLED_MASK,
+					    SCMI_SENS_CFG_TSTAMP_ENABLE);
+	}
+
+	sensor_config &= ~SCMI_SENS_CFG_ROUND_MASK;
+	sensor_config |= FIELD_PREP(SCMI_SENS_CFG_ROUND_MASK, SCMI_SENS_CFG_ROUND_AUTO);
+
+	err = sensor->handle->sensor_ops->config_set(
+		sensor->handle, sensor->sensor_info->id, sensor_config);
+	if (err)
+		pr_err("Error in setting sensor update interval for sensor %s value %u err %d",
+		       sensor->sensor_info->name, sensor_config, err);
+
+	return err;
+}
+
+static int scmi_iio_write_raw(struct iio_dev *iio_dev,
+			      struct iio_chan_spec const *chan, int val,
+			      int val2, long mask)
+{
+	int err;
+
+	mutex_lock(&iio_dev->mlock);
+	switch (mask) {
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		err = scmi_iio_set_odr_val(iio_dev, val, val2);
+		break;
+	default:
+		err = -EINVAL;
+		break;
+	}
+	mutex_unlock(&iio_dev->mlock);
+
+	return err;
+}
+
+static u64 convert_interval_to_ns(u32 val)
+{
+	u64 sensor_interval_mult;
+	u64 sensor_update_interval = SCMI_SENS_INTVL_GET_SECS(val) * NSEC_PER_SEC;
+
+	int mult = SCMI_SENS_INTVL_GET_EXP(val);
+
+	if (mult < 0) {
+		sensor_interval_mult = int_pow(10, abs(mult));
+		sensor_update_interval =
+			sensor_update_interval / sensor_interval_mult;
+	} else {
+		sensor_interval_mult = int_pow(10, mult);
+		sensor_update_interval =
+			sensor_update_interval * sensor_interval_mult;
+	}
+
+	return sensor_update_interval;
+}
+
+static void convert_ns_to_freq(u64 interval_ns, struct sensor_freq *freq)
+{
+	u64 rem;
+
+	freq->hz = div64_u64_rem(NSEC_PER_SEC, interval_ns, &rem);
+	freq->uhz = (rem * 1000000UL) / interval_ns;
+}
+
+static int scmi_iio_get_odr_val(struct iio_dev *iio_dev, int *val, int *val2)
+{
+	struct scmi_iio_priv *sensor = iio_priv(iio_dev);
+	u32 sensor_config;
+	struct sensor_freq freq;
+	u64 sensor_update_interval, sensor_interval_mult;
+	int mult;
+
+	int err = sensor->handle->sensor_ops->config_get(
+		sensor->handle, sensor->sensor_info->id, &sensor_config);
+	if (err) {
+		pr_err("%s: Error in getting sensor config for sensor %s err %d",
+		       __func__, sensor->sensor_info->name, err);
+		return err;
+	}
+
+	sensor_update_interval = SCMI_SENS_CFG_GET_UPDATE_SECS(sensor_config) * NSEC_PER_SEC;
+
+	mult = SCMI_SENS_CFG_GET_UPDATE_EXP(sensor_config);
+	if (mult < 0) {
+		sensor_interval_mult = int_pow(10, abs(mult));
+		sensor_update_interval =
+			sensor_update_interval / sensor_interval_mult;
+	} else {
+		sensor_interval_mult = int_pow(10, mult);
+		sensor_update_interval =
+			sensor_update_interval * sensor_interval_mult;
+	}
+
+	convert_ns_to_freq(sensor_update_interval, &freq);
+	*val = freq.hz;
+	*val2 = freq.uhz;
+
+	return 0;
+}
+
+static int scmi_iio_read_raw(struct iio_dev *iio_dev,
+			     struct iio_chan_spec const *ch, int *val,
+			     int *val2, long mask)
+{
+	struct scmi_iio_priv *sensor = iio_priv(iio_dev);
+	s8 scale;
+	int ret;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_SCALE:
+		scale = sensor->sensor_info->axis[ch->scan_index].scale;
+		if (scale < 0) {
+			scale = -scale;
+			*val = 1;
+			*val2 = int_pow(10, scale);
+			return IIO_VAL_FRACTIONAL;
+		}
+		*val = int_pow(10, scale);
+		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		ret = scmi_iio_get_odr_val(iio_dev, val, val2);
+		return ret ? ret : IIO_VAL_INT_PLUS_MICRO;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static ssize_t scmi_iio_get_sensor_power(struct device *dev,
+					 struct device_attribute *attr,
+					 char *buf)
+{
+	struct scmi_iio_priv *sensor = iio_priv(dev_get_drvdata(dev));
+	int len;
+
+	if (sensor->sensor_info->extended_scalar_attrs)
+		len = scnprintf(buf, PAGE_SIZE, "%u\n",
+				sensor->sensor_info->sensor_power);
+
+	return len;
+}
+
+static ssize_t scmi_iio_get_sensor_max_range(struct device *dev,
+					     struct device_attribute *attr,
+					     char *buf)
+{
+	struct scmi_iio_priv *sensor = iio_priv(dev_get_drvdata(dev));
+	int i;
+	s64 max_range = S64_MIN, max_range_axis;
+
+	for (i = 0; i < sensor->sensor_info->num_axis; i++) {
+		if (sensor->sensor_info->axis[i].extended_attrs) {
+			max_range_axis =
+				sensor->sensor_info->axis[i].attrs.max_range;
+			max_range = max(max_range, max_range_axis);
+		}
+	}
+
+	return scnprintf(buf, PAGE_SIZE, "%lld\n", max_range);
+}
+
+static ssize_t scmi_iio_get_sensor_resolution(struct device *dev,
+					      struct device_attribute *attr,
+					      char *buf)
+{
+	struct scmi_iio_priv *sensor = iio_priv(dev_get_drvdata(dev));
+	int len;
+
+	/*
+	 * All the axes are supposed to have the same value for resolution
+	 * and exponent. We are just using the values from the Axis 0 here.
+	 */
+
+	if (sensor->sensor_info->axis[0].extended_attrs) {
+		u32 resolution = sensor->sensor_info->axis[0].resolution;
+		s8 exponent = sensor->sensor_info->axis[0].exponent;
+		u32 multiplier = int_pow(10, abs(exponent));
+
+		if (exponent < 0) {
+			int vals[] = { resolution, multiplier };
+
+			len = iio_format_value(buf, IIO_VAL_FRACTIONAL,
+					       ARRAY_SIZE(vals), vals);
+		} else {
+			int vals[] = { resolution * multiplier };
+
+			len = iio_format_value(buf, IIO_VAL_INT,
+					       ARRAY_SIZE(vals), vals);
+		}
+	}
+
+	return len;
+}
+
+static IIO_DEVICE_ATTR(sensor_power, 0444, scmi_iio_get_sensor_power, NULL, 0);
+static IIO_DEVICE_ATTR(sensor_max_range, 0444, scmi_iio_get_sensor_max_range,
+		       NULL, 0);
+static IIO_DEVICE_ATTR(sensor_resolution, 0444, scmi_iio_get_sensor_resolution,
+		       NULL, 0);
+
+static struct attribute *scmi_iio_attributes[] = {
+	&iio_dev_attr_sensor_power.dev_attr.attr,
+	&iio_dev_attr_sensor_max_range.dev_attr.attr,
+	&iio_dev_attr_sensor_resolution.dev_attr.attr,
+	NULL,
+};
+
+static const struct attribute_group scmi_iio_attribute_group = {
+	.attrs = scmi_iio_attributes,
+};
+
+static const struct iio_info scmi_iio_info = {
+	.read_raw = scmi_iio_read_raw,
+	.read_avail = scmi_iio_read_avail,
+	.write_raw = scmi_iio_write_raw,
+	.attrs = &scmi_iio_attribute_group,
+};
+
+static int scmi_iio_get_chan_type(u8 scmi_type, enum iio_chan_type *iio_type)
+{
+	switch (scmi_type) {
+	case METERS_SEC_SQUARED:
+		*iio_type = IIO_ACCEL;
+		return 0;
+	case RADIANS_SEC:
+		*iio_type = IIO_ANGL_VEL;
+		return 0;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int scmi_iio_get_chan_modifier(const char *name,
+				      enum iio_modifier *modifier)
+{
+	int ret;
+	char *pch = strrchr(name, '_');
+
+	if (!pch)
+		return -EINVAL;
+
+	if (strcmp(pch + 1, "X") == 0)
+		*modifier = IIO_MOD_X;
+	else if (strcmp(pch + 1, "Y") == 0)
+		*modifier = IIO_MOD_Y;
+	else if (strcmp(pch + 1, "Z") == 0)
+		*modifier = IIO_MOD_Z;
+	else
+		return -EINVAL;
+
+	return ret;
+}
+
+static void scmi_iio_set_data_channel(struct iio_chan_spec *iio_chan,
+				      enum iio_chan_type type,
+				      enum iio_modifier mod, int scan_index)
+{
+	iio_chan->type = type;
+	iio_chan->modified = 1;
+	iio_chan->channel2 = mod;
+	iio_chan->info_mask_separate = BIT(IIO_CHAN_INFO_SCALE);
+	iio_chan->info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SAMP_FREQ);
+	iio_chan->info_mask_shared_by_type_available = BIT(IIO_CHAN_INFO_SAMP_FREQ);
+	iio_chan->scan_index = scan_index;
+	iio_chan->scan_type.sign = 's';
+	iio_chan->scan_type.realbits = 64;
+	iio_chan->scan_type.storagebits = 64;
+	iio_chan->scan_type.endianness = IIO_LE;
+}
+
+static void scmi_iio_set_timestamp_channel(struct iio_chan_spec *iio_chan,
+					   int scan_index)
+{
+	iio_chan->type = IIO_TIMESTAMP;
+	iio_chan->channel = -1;
+	iio_chan->scan_index = scan_index;
+	iio_chan->scan_type.sign = 'u';
+	iio_chan->scan_type.realbits = 64;
+	iio_chan->scan_type.storagebits = 64;
+}
+
+static int set_sampling_freq_avail(struct iio_dev *iio_dev)
+{
+	struct scmi_iio_priv *sensor = iio_priv(iio_dev);
+	int i;
+	struct sensor_freq freq;
+	u64 cur_interval_ns, low_interval_ns, high_interval_ns, step_size_ns;
+	unsigned int low_interval, high_interval, step_size;
+
+	sensor->freq_avail =  devm_kzalloc(&iio_dev->dev,
+					sizeof(u32) * (sensor->sensor_info->intervals.count * 2),
+					GFP_KERNEL);
+	if (!sensor->freq_avail)
+		return -ENOMEM;
+
+	if (sensor->sensor_info->intervals.segmented) {
+		low_interval = sensor->sensor_info->intervals.desc[SCMI_SENS_INTVL_SEGMENT_LOW];
+		low_interval_ns = convert_interval_to_ns(low_interval);
+		convert_ns_to_freq(low_interval_ns, &freq);
+		sensor->freq_avail[0] = freq.hz;
+		sensor->freq_avail[1] = freq.uhz;
+
+		step_size = sensor->sensor_info->intervals.desc[SCMI_SENS_INTVL_SEGMENT_STEP];
+		step_size_ns = convert_interval_to_ns(step_size);
+		convert_ns_to_freq(step_size_ns, &freq);
+		sensor->freq_avail[2] = freq.hz;
+		sensor->freq_avail[3] = freq.uhz;
+
+		high_interval = sensor->sensor_info->intervals.desc[SCMI_SENS_INTVL_SEGMENT_HIGH];
+		high_interval_ns = convert_interval_to_ns(high_interval);
+		convert_ns_to_freq(high_interval_ns, &freq);
+		sensor->freq_avail[4] = freq.hz;
+		sensor->freq_avail[5] = freq.uhz;
+	} else {
+		for (i = 0; i < sensor->sensor_info->intervals.count; i++) {
+			cur_interval_ns = convert_interval_to_ns
+						(sensor->sensor_info->intervals.desc[i]);
+			convert_ns_to_freq(cur_interval_ns, &freq);
+			sensor->freq_avail[i*2] = freq.hz;
+			sensor->freq_avail[i*2+1] = freq.uhz;
+		}
+	}
+
+	return 0;
+}
+
+static int scmi_iio_buffers_setup(struct iio_dev *scmi_iiodev)
+{
+	struct iio_buffer *buffer = devm_iio_kfifo_allocate(&scmi_iiodev->dev);
+
+	if (!buffer)
+		return -ENOMEM;
+
+	iio_device_attach_buffer(scmi_iiodev, buffer);
+	scmi_iiodev->modes |= INDIO_BUFFER_SOFTWARE;
+	scmi_iiodev->setup_ops = &scmi_iio_buffer_ops;
+
+	return 0;
+}
+
+static int scmi_alloc_iiodev(struct device *dev, struct scmi_handle *handle,
+			     const struct scmi_sensor_info *sensor_info,
+			     struct iio_dev **scmi_iio_dev)
+{
+	struct scmi_iio_priv *sensor;
+	struct iio_chan_spec *iio_channels;
+	enum iio_chan_type type;
+	enum iio_modifier modifier;
+	struct iio_dev *iiodev;
+	int i, ret;
+
+	iiodev = devm_iio_device_alloc(dev, sizeof(*sensor));
+	if (!iiodev)
+		return -ENOMEM;
+
+	iiodev->modes = INDIO_DIRECT_MODE;
+	iiodev->dev.parent = dev;
+	sensor = iio_priv(iiodev);
+	sensor->handle = handle;
+	sensor->sensor_info = sensor_info;
+	sensor->sensor_update_nb.notifier_call = sensor_update_cb;
+	sensor->indio_dev = iiodev;
+	iiodev->num_channels =
+		sensor_info->num_axis +
+		1; /* adding one additional channel for timestamp */
+	iiodev->name = sensor_info->name;
+	iiodev->info = &scmi_iio_info;
+
+	iio_channels =
+		devm_kzalloc(dev,
+			     sizeof(*iio_channels) * (iiodev->num_channels),
+			     GFP_KERNEL);
+	if (!iio_channels)
+		return -ENOMEM;
+
+	for (i = 0; i < sensor_info->num_axis; i++) {
+		ret = scmi_iio_get_chan_type(sensor_info->axis[i].type, &type);
+		if (ret < 0)
+			return ret;
+
+		ret = scmi_iio_get_chan_modifier(sensor_info->axis[i].name,
+						 &modifier);
+		if (ret < 0)
+			return ret;
+
+		scmi_iio_set_data_channel(&iio_channels[i], type, modifier,
+					  sensor_info->axis[i].id);
+	}
+
+	scmi_iio_set_timestamp_channel(&iio_channels[i], i);
+	iiodev->channels = iio_channels;
+
+	ret = set_sampling_freq_avail(iiodev);
+	if (ret < 0)
+		return ret;
+
+	*scmi_iio_dev = iiodev;
+
+	return ret;
+}
+
+static int scmi_iio_dev_probe(struct scmi_device *sdev)
+{
+	struct iio_dev *scmi_iio_dev;
+	const struct scmi_sensor_info *sensor_info;
+	int err, i;
+	u16 nr_sensors;
+	struct device *dev = &sdev->dev;
+	struct scmi_handle *handle = sdev->handle;
+
+	if (!handle || !handle->sensor_ops || !handle->sensor_ops->count_get ||
+	    !handle->sensor_ops->info_get || !handle->sensor_ops->config_get ||
+	    !handle->sensor_ops->config_set) {
+		dev_err(dev, "SCMI device has no sensor interface\n");
+		return -EINVAL;
+	}
+
+	nr_sensors = handle->sensor_ops->count_get(handle);
+	if (!nr_sensors) {
+		dev_dbg(dev, "0 sensors found via SCMI bus\n");
+		return -EINVAL;
+	}
+
+	dev_dbg(dev, "%d sensors found via SCMI bus\n", nr_sensors);
+
+	for (i = 0; i < nr_sensors; i++) {
+		sensor_info = handle->sensor_ops->info_get(handle, i);
+		if (!sensor_info) {
+			dev_err(dev, "SCMI sensor %d has missing info\n", i);
+			return -EINVAL;
+		}
+
+		/* Skipping scalar sensor,as this driver only supports accel and gyro */
+		if (sensor_info->num_axis == 0)
+			continue;
+
+		err = scmi_alloc_iiodev(dev, handle, sensor_info,
+					&scmi_iio_dev);
+		if (err < 0) {
+			dev_err(dev,
+				"failed to allocate IIO device for sensor %s: %d\n",
+				sensor_info->name, err);
+			return err;
+		}
+		if (!scmi_iio_dev) {
+			dev_err(dev, "memory allocation failed at sensor %s\n",
+				sensor_info->name);
+			return -ENOMEM;
+		}
+
+		err = scmi_iio_buffers_setup(scmi_iio_dev);
+		if (err < 0) {
+			dev_err(dev,
+				"IIO buffer setup error at sensor %s: %d\n",
+				sensor_info->name, err);
+			return err;
+		}
+
+		err = devm_iio_device_register(dev, scmi_iio_dev);
+		if (err) {
+			dev_err(dev,
+				"IIO device registration failed at sensor %s: %d\n",
+				sensor_info->name, err);
+			return err;
+		}
+	}
+
+	return err;
+}
+
+static const struct scmi_device_id scmi_id_table[] = {
+	{ SCMI_PROTOCOL_SENSOR, "iiodev" },
+	{},
+};
+
+MODULE_DEVICE_TABLE(scmi, scmi_id_table);
+
+static struct scmi_driver scmi_iiodev_driver = {
+	.name = "scmi-sensor-iiodev",
+	.probe = scmi_iio_dev_probe,
+	.id_table = scmi_id_table,
+};
+
+module_scmi_driver(scmi_iiodev_driver);
+
+MODULE_AUTHOR("Jyoti Bhayana <jbhayana@google.com>");
+MODULE_DESCRIPTION("SCMI IIO Driver");
+MODULE_LICENSE("GPL v2");
-- 
2.29.2.729.g45daf8777d-goog

Re: [RFC PATCH v2 1/1] iio/scmi: Adding support for IIO SCMI Based Sensors

[Jonathan Cameron]

On Thu, 24 Dec 2020 03:19:21 +0000
Jyoti Bhayana <jbhayana@google.com> wrote:

> This change provides ARM SCMI Protocol based IIO device.
> This driver provides support for Accelerometer and Gyroscope using
> new SCMI Sensor Protocol defined by the upcoming SCMIv3.0
> ARM specification
> 
> Signed-off-by: Jyoti Bhayana <jbhayana@google.com>

Hi Jyoti,

There are a few things that still need thought it in here.  I've raised
various bits inline.  Note that some of them apply to whole driver
rather than just the specific instance I've mentioned.

Thanks,

Jonathan

> ---
>  MAINTAINERS                                |   6 +
>  drivers/iio/common/Kconfig                 |   1 +
>  drivers/iio/common/Makefile                |   1 +
>  drivers/iio/common/scmi_sensors/Kconfig    |  18 +
>  drivers/iio/common/scmi_sensors/Makefile   |   5 +
>  drivers/iio/common/scmi_sensors/scmi_iio.c | 696 +++++++++++++++++++++
>  6 files changed, 727 insertions(+)
>  create mode 100644 drivers/iio/common/scmi_sensors/Kconfig
>  create mode 100644 drivers/iio/common/scmi_sensors/Makefile
>  create mode 100644 drivers/iio/common/scmi_sensors/scmi_iio.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index b516bb34a8d5..ccf37d43ab41 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -8567,6 +8567,12 @@ S:	Maintained
>  F:	Documentation/devicetree/bindings/iio/multiplexer/io-channel-mux.txt
>  F:	drivers/iio/multiplexer/iio-mux.c
>  
> +IIO SCMI BASED DRIVER
> +M:	Jyoti Bhayana <jbhayana@google.com>
> +L:	linux-iio@vger.kernel.org
> +S:	Maintained
> +F:	drivers/iio/common/scmi_sensors/scmi_iio.c
> +
>  IIO SUBSYSTEM AND DRIVERS
>  M:	Jonathan Cameron <jic23@kernel.org>
>  R:	Lars-Peter Clausen <lars@metafoo.de>
> diff --git a/drivers/iio/common/Kconfig b/drivers/iio/common/Kconfig
> index 2b9ee9161abd..0334b4954773 100644
> --- a/drivers/iio/common/Kconfig
> +++ b/drivers/iio/common/Kconfig
> @@ -6,5 +6,6 @@
>  source "drivers/iio/common/cros_ec_sensors/Kconfig"
>  source "drivers/iio/common/hid-sensors/Kconfig"
>  source "drivers/iio/common/ms_sensors/Kconfig"
> +source "drivers/iio/common/scmi_sensors/Kconfig"
>  source "drivers/iio/common/ssp_sensors/Kconfig"
>  source "drivers/iio/common/st_sensors/Kconfig"
> diff --git a/drivers/iio/common/Makefile b/drivers/iio/common/Makefile
> index 4bc30bb548e2..fad40e1e1718 100644
> --- a/drivers/iio/common/Makefile
> +++ b/drivers/iio/common/Makefile
> @@ -11,5 +11,6 @@
>  obj-y += cros_ec_sensors/
>  obj-y += hid-sensors/
>  obj-y += ms_sensors/
> +obj-y += scmi_sensors/
>  obj-y += ssp_sensors/
>  obj-y += st_sensors/
> diff --git a/drivers/iio/common/scmi_sensors/Kconfig b/drivers/iio/common/scmi_sensors/Kconfig
> new file mode 100644
> index 000000000000..67e084cbb1ab
> --- /dev/null
> +++ b/drivers/iio/common/scmi_sensors/Kconfig
> @@ -0,0 +1,18 @@
> +#
> +# IIO over SCMI
> +#
> +# When adding new entries keep the list in alphabetical order
> +
> +menu "IIO SCMI Sensors"
> +
> +config IIO_SCMI
> +	tristate "IIO SCMI"
> +        depends on ARM_SCMI_PROTOCOL
> +        select IIO_BUFFER
> +        select IIO_KFIFO_BUF
> +	help
> +          Say yes here to build support for IIO SCMI Driver.
> +          This provides ARM SCMI Protocol based IIO device.
> +          This driver provides support for accelerometer and gyroscope
> +          sensors available on SCMI based platforms.
> +endmenu
> diff --git a/drivers/iio/common/scmi_sensors/Makefile b/drivers/iio/common/scmi_sensors/Makefile
> new file mode 100644
> index 000000000000..f13140a2575a
> --- /dev/null
> +++ b/drivers/iio/common/scmi_sensors/Makefile
> @@ -0,0 +1,5 @@
> +# SPDX - License - Identifier : GPL - 2.0 - only
> +#
> +# Makefile for the IIO over SCMI
> +#
> +obj-$(CONFIG_IIO_SCMI) += scmi_iio.o
> diff --git a/drivers/iio/common/scmi_sensors/scmi_iio.c b/drivers/iio/common/scmi_sensors/scmi_iio.c
> new file mode 100644
> index 000000000000..2f5c884e22ab
> --- /dev/null
> +++ b/drivers/iio/common/scmi_sensors/scmi_iio.c
> @@ -0,0 +1,696 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +/*
> + * System Control and Management Interface(SCMI) based IIO sensor driver
> + *
> + * Copyright (C) 2020 Google LLC
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/err.h>
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/kfifo_buf.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/kernel.h>
> +#include <linux/kthread.h>
> +#include <linux/module.h>
> +#include <linux/scmi_protocol.h>
> +#include <linux/time.h>
> +#include <linux/types.h>
> +
> +#define NSEC_MULT_POW_10 (const_ilog2(NSEC_PER_SEC) / const_ilog2(10))
> +#define UHZ_PER_HZ 1000000UL
> +#define ODR_EXPAND(odr, uodr) (((odr)*1000000ULL) + (uodr))
> +#define MAX_NUM_OF_CHANNELS 4
> +
> +struct scmi_iio_priv {
> +	struct scmi_handle *handle;
> +	const struct scmi_sensor_info *sensor_info;
> +	struct iio_dev *indio_dev;
> +	long long iio_buf[MAX_NUM_OF_CHANNELS];
> +	struct notifier_block sensor_update_nb;
> +	u32 *freq_avail;
> +};
> +
> +struct sensor_freq {
> +	u64 hz;
> +	u64 uhz;
> +};
> +
> +static int sensor_update_cb(struct notifier_block *nb, unsigned long event,
> +			    void *data)
> +{
> +	struct scmi_sensor_update_report *sensor_update = data;
> +	u64 time, time_ns;
> +	s64 sensor_value;
> +	struct iio_dev *scmi_iio_dev;
> +	s8 tstamp_scale_ns;
> +	int i;
> +	struct scmi_iio_priv *sensor =
> +		container_of(nb, struct scmi_iio_priv, sensor_update_nb);
> +
> +	scmi_iio_dev = sensor->indio_dev;
> +
> +	for (i = 0; i < sensor_update->readings_count; i++) {
> +		sensor_value = sensor_update->readings[i].value;
> +		time = sensor_update->readings[i].timestamp;

I'd like a comment for time.  I assume the assumption is that it's
the same for all the readings we get in a single call of this function?
Also better to only store it here if (sensor_info->timestamped) and
for that matter drop it out of the loop if they are all the same.

> +		sensor->iio_buf[i] = sensor_value;

Why the local variable for sensor_value?

		sensor->iio_buf[i] = sensor_update->readings[i].value;

> +	}
> +
> +	if (!sensor->sensor_info->timestamped) {
> +		time_ns = iio_get_time_ns(scmi_iio_dev);
> +	} else {
> +		tstamp_scale_ns =
> +			sensor->sensor_info->tstamp_scale + NSEC_MULT_POW_10;
> +		if (tstamp_scale_ns < 0) {
> +			tstamp_scale_ns = -1 * tstamp_scale_ns;

use absolute rather than -1 * as makes it explicit that result is unsigned.
Can do it inline in the line below.

> +			time_ns = div64_u64(time, int_pow(10, tstamp_scale_ns));
> +		} else {
> +			time_ns = time * int_pow(10, tstamp_scale_ns);
> +		}
> +	}
> +
> +	iio_push_to_buffers_with_timestamp(scmi_iio_dev, sensor->iio_buf,
> +					   time_ns);
> +
> +	return NOTIFY_OK;
> +}
> +
> +static int scmi_iio_buffer_preenable(struct iio_dev *dev)
> +{
> +	struct scmi_iio_priv *sensor = iio_priv(dev);
> +	u32 sensor_id = sensor->sensor_info->id;
> +	u32 sensor_config;
> +	int err;
> +
> +	if (sensor->sensor_info->timestamped)
> +		sensor_config |= FIELD_PREP(SCMI_SENS_CFG_TSTAMP_ENABLED_MASK,
> +					    SCMI_SENS_CFG_TSTAMP_ENABLE);
> +
> +	sensor_config |= FIELD_PREP(SCMI_SENS_CFG_SENSOR_ENABLED_MASK,
> +				    SCMI_SENS_CFG_SENSOR_ENABLE);
> +
> +	err = sensor->handle->notify_ops->register_event_notifier(
> +		sensor->handle, SCMI_PROTOCOL_SENSOR, SCMI_EVENT_SENSOR_UPDATE,
> +		&sensor_id, &sensor->sensor_update_nb);
> +	if (err) {
> +		pr_err("Error in registering sensor update notifier for sensor %s err %d",
> +		       sensor->sensor_info->name, err);
> +		return err;
> +	}
> +
> +	err = sensor->handle->sensor_ops->config_set(
> +		sensor->handle, sensor->sensor_info->id, sensor_config);
> +	if (err)
> +		pr_err("Error in enabling sensor %s err %d",
> +		       sensor->sensor_info->name, err);
> +
> +	return err;
> +}
> +
> +static int scmi_iio_buffer_postdisable(struct iio_dev *iio_dev)
> +{
> +	struct scmi_iio_priv *sensor = iio_priv(iio_dev);
> +	u32 sensor_id = sensor->sensor_info->id;
> +	u32 sensor_config = 0;
> +	int err;
> +
> +	sensor_config |= FIELD_PREP(SCMI_SENS_CFG_SENSOR_ENABLED_MASK,
> +				    SCMI_SENS_CFG_SENSOR_DISABLE);
> +
> +	err = sensor->handle->notify_ops->unregister_event_notifier(
> +		sensor->handle, SCMI_PROTOCOL_SENSOR, SCMI_EVENT_SENSOR_UPDATE,
> +		&sensor_id, &sensor->sensor_update_nb);
> +	if (err) {
> +		pr_err("Error in unregistering sensor update notifier for sensor %s err %d",
> +		       sensor->sensor_info->name, err);
> +		return err;
> +	}
> +
> +	err = sensor->handle->sensor_ops->config_set(sensor->handle, sensor_id,
> +						     sensor_config);
> +	if (err)
> +		pr_err("Error in disabling sensor %s with err %d",
> +		       sensor->sensor_info->name, err);
> +
> +	return err;
> +}
> +
> +static const struct iio_buffer_setup_ops scmi_iio_buffer_ops = {
> +	.preenable = scmi_iio_buffer_preenable,
> +	.postdisable = scmi_iio_buffer_postdisable,
> +};
> +
> +static int scmi_iio_read_avail(struct iio_dev *iio_dev,
> +					 struct iio_chan_spec const *chan,
> +					 const int **vals,
> +					 int *type, int *length, long mask)
> +{
> +	struct scmi_iio_priv *sensor = iio_priv(iio_dev);
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		*vals = sensor->freq_avail;
> +		*type = IIO_VAL_INT_PLUS_MICRO;
> +		*length = sensor->sensor_info->intervals.count * 2;
> +		if (sensor->sensor_info->intervals.segmented)
> +			return IIO_AVAIL_RANGE;
> +		else
> +			return IIO_AVAIL_LIST;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int scmi_iio_set_odr_val(struct iio_dev *iio_dev, int val, int val2)
> +{
> +	struct scmi_iio_priv *sensor = iio_priv(iio_dev);
> +	u32 sensor_config;
> +	u64 sec, mult, uHz;
> +	char buf[32];
> +
> +	int err = sensor->handle->sensor_ops->config_get(
> +		sensor->handle, sensor->sensor_info->id, &sensor_config);
> +	if (err) {
> +		pr_err("%s: Error in getting sensor config for sensor %s err %d",
> +		       __func__, sensor->sensor_info->name, err);
> +		return err;
> +	}
> +
> +	uHz = ODR_EXPAND(val, val2);
> +
> +	/*
> +	 * The seconds field in the sensor interval in SCMI is 16 bits long
> +	 * Therefore seconds  = 1/Hz <= 0xFFFF. As floating point calculations are
> +	 * discouraged in the kernel driver code, to calculate the scale factor (sf)
> +	 * (1* 1000000 * sf)/uHz <= 0xFFFF. Therefore, sf <= (uHz * 0xFFFF)/1000000
> +	 *  To calculate the multiplier,we convert the sf into char string  and
> +	 *  count the number of characters
> +	 */
> +
> +	mult = scnprintf(buf, 32, "%llu", ((u64)uHz * 0xFFFF) / UHZ_PER_HZ) - 1;

use sizeof(buf) instead of having 32 again.

> +
> +	sec = div64_u64(int_pow(10, mult) * UHZ_PER_HZ, uHz);
> +	if (sec == 0) {
> +		pr_err("Trying to set invalid sensor update value for sensor %s",
> +		       sensor->sensor_info->name);
> +		return -EINVAL;
> +	}
> +
> +	sensor_config &= ~SCMI_SENS_CFG_UPDATE_SECS_MASK;
> +	sensor_config |= FIELD_PREP(SCMI_SENS_CFG_UPDATE_SECS_MASK, sec);
> +	sensor_config &= ~SCMI_SENS_CFG_UPDATE_EXP_MASK;
> +	sensor_config |= FIELD_PREP(SCMI_SENS_CFG_UPDATE_EXP_MASK, -mult);
> +
> +	if (sensor->sensor_info->timestamped) {
> +		sensor_config &= ~SCMI_SENS_CFG_TSTAMP_ENABLED_MASK;
> +		sensor_config |= FIELD_PREP(SCMI_SENS_CFG_TSTAMP_ENABLED_MASK,
> +					    SCMI_SENS_CFG_TSTAMP_ENABLE);
> +	}
> +
> +	sensor_config &= ~SCMI_SENS_CFG_ROUND_MASK;
> +	sensor_config |= FIELD_PREP(SCMI_SENS_CFG_ROUND_MASK, SCMI_SENS_CFG_ROUND_AUTO);
> +
> +	err = sensor->handle->sensor_ops->config_set(
> +		sensor->handle, sensor->sensor_info->id, sensor_config);
> +	if (err)
> +		pr_err("Error in setting sensor update interval for sensor %s value %u err %d",
> +		       sensor->sensor_info->name, sensor_config, err);
> +
> +	return err;
> +}
> +
> +static int scmi_iio_write_raw(struct iio_dev *iio_dev,
> +			      struct iio_chan_spec const *chan, int val,
> +			      int val2, long mask)
> +{
> +	int err;
> +
> +	mutex_lock(&iio_dev->mlock);

Move the lock in to around the call that actually need protecting.
Will simplify the rest of the code somewhat.

> +	switch (mask) {
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		err = scmi_iio_set_odr_val(iio_dev, val, val2);
> +		break;
> +	default:
> +		err = -EINVAL;
> +		break;
> +	}
> +	mutex_unlock(&iio_dev->mlock);
> +
> +	return err;
> +}
> +
> +static u64 convert_interval_to_ns(u32 val)
> +{
> +	u64 sensor_interval_mult;
> +	u64 sensor_update_interval = SCMI_SENS_INTVL_GET_SECS(val) * NSEC_PER_SEC;
> +
> +	int mult = SCMI_SENS_INTVL_GET_EXP(val);
> +
> +	if (mult < 0) {
> +		sensor_interval_mult = int_pow(10, abs(mult));
> +		sensor_update_interval =
> +			sensor_update_interval / sensor_interval_mult;
> +	} else {
> +		sensor_interval_mult = int_pow(10, mult);
> +		sensor_update_interval =
> +			sensor_update_interval * sensor_interval_mult;
> +	}
> +
> +	return sensor_update_interval;
> +}
> +
> +static void convert_ns_to_freq(u64 interval_ns, struct sensor_freq *freq)

Prefix with scmi_iio as that name is generic enough we might get a clash in the future.
Also seems like you might as well just pass in two integers instead of freq as you
always end up copying the values out to such integers anyway.

> +{
> +	u64 rem;
> +
> +	freq->hz = div64_u64_rem(NSEC_PER_SEC, interval_ns, &rem);
> +	freq->uhz = (rem * 1000000UL) / interval_ns;
> +}
> +
> +static int scmi_iio_get_odr_val(struct iio_dev *iio_dev, int *val, int *val2)
> +{
> +	struct scmi_iio_priv *sensor = iio_priv(iio_dev);
> +	u32 sensor_config;
> +	struct sensor_freq freq;
> +	u64 sensor_update_interval, sensor_interval_mult;

Probably worth doing reverse xmas tree ordering throughout the driver
where it doesn't hurt readability.

> +	int mult;
> +
> +	int err = sensor->handle->sensor_ops->config_get(
> +		sensor->handle, sensor->sensor_info->id, &sensor_config);
> +	if (err) {
> +		pr_err("%s: Error in getting sensor config for sensor %s err %d",
> +		       __func__, sensor->sensor_info->name, err);

Probably better to use dev_err where you can get an appropriate device,
That should result in a suitable name being printed.

Also no need to add __func__ etc to messages as I'm fairly sure that can be done
via dynamic debug if desired.


> +		return err;
> +	}
> +
> +	sensor_update_interval = SCMI_SENS_CFG_GET_UPDATE_SECS(sensor_config) * NSEC_PER_SEC;
> +
> +	mult = SCMI_SENS_CFG_GET_UPDATE_EXP(sensor_config);
> +	if (mult < 0) {
> +		sensor_interval_mult = int_pow(10, abs(mult));
> +		sensor_update_interval =
> +			sensor_update_interval / sensor_interval_mult;
> +	} else {
> +		sensor_interval_mult = int_pow(10, mult);
> +		sensor_update_interval =
> +			sensor_update_interval * sensor_interval_mult;
> +	}
> +
> +	convert_ns_to_freq(sensor_update_interval, &freq);
> +	*val = freq.hz;
> +	*val2 = freq.uhz;
> +
> +	return 0;
> +}
> +
> +static int scmi_iio_read_raw(struct iio_dev *iio_dev,
> +			     struct iio_chan_spec const *ch, int *val,
> +			     int *val2, long mask)
> +{
> +	struct scmi_iio_priv *sensor = iio_priv(iio_dev);
> +	s8 scale;
> +	int ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_SCALE:
> +		scale = sensor->sensor_info->axis[ch->scan_index].scale;
> +		if (scale < 0) {
> +			scale = -scale;
> +			*val = 1;
> +			*val2 = int_pow(10, scale);
> +			return IIO_VAL_FRACTIONAL;
> +		}
> +		*val = int_pow(10, scale);
> +		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		ret = scmi_iio_get_odr_val(iio_dev, val, val2);
> +		return ret ? ret : IIO_VAL_INT_PLUS_MICRO;
> +	default:
> +		return -EINVAL;
> +	}
> +

Can't get here.  Check all your code for similar and make sure to clean them out.
One of the static analyzers will moan about this if not (I can't recall which right now).


> +	return 0;
> +}
> +
> +static ssize_t scmi_iio_get_sensor_power(struct device *dev,
> +					 struct device_attribute *attr,
> +					 char *buf)
> +{
> +	struct scmi_iio_priv *sensor = iio_priv(dev_get_drvdata(dev));
> +	int len;
> +
> +	if (sensor->sensor_info->extended_scalar_attrs)
> +		len = scnprintf(buf, PAGE_SIZE, "%u\n",
> +				sensor->sensor_info->sensor_power);
> +
> +	return len;
> +}
> +
> +static ssize_t scmi_iio_get_sensor_max_range(struct device *dev,
> +					     struct device_attribute *attr,
> +					     char *buf)
> +{
> +	struct scmi_iio_priv *sensor = iio_priv(dev_get_drvdata(dev));
> +	int i;
> +	s64 max_range = S64_MIN, max_range_axis;
> +
> +	for (i = 0; i < sensor->sensor_info->num_axis; i++) {
> +		if (sensor->sensor_info->axis[i].extended_attrs) {
> +			max_range_axis =
> +				sensor->sensor_info->axis[i].attrs.max_range;
> +			max_range = max(max_range, max_range_axis);
> +		}
> +	}
> +
> +	return scnprintf(buf, PAGE_SIZE, "%lld\n", max_range);

What are units of this?  From Android docs this would be expected to be a float.

> +}
> +
> +static ssize_t scmi_iio_get_sensor_resolution(struct device *dev,
> +					      struct device_attribute *attr,
> +					      char *buf)
> +{
> +	struct scmi_iio_priv *sensor = iio_priv(dev_get_drvdata(dev));
> +	int len;
> +
> +	/*
> +	 * All the axes are supposed to have the same value for resolution
> +	 * and exponent. We are just using the values from the Axis 0 here.
> +	 */
> +
> +	if (sensor->sensor_info->axis[0].extended_attrs) {
> +		u32 resolution = sensor->sensor_info->axis[0].resolution;
> +		s8 exponent = sensor->sensor_info->axis[0].exponent;
> +		u32 multiplier = int_pow(10, abs(exponent));
> +
> +		if (exponent < 0) {
> +			int vals[] = { resolution, multiplier };
> +
> +			len = iio_format_value(buf, IIO_VAL_FRACTIONAL,
> +					       ARRAY_SIZE(vals), vals);
> +		} else {
> +			int vals[] = { resolution * multiplier };
> +
> +			len = iio_format_value(buf, IIO_VAL_INT,
> +					       ARRAY_SIZE(vals), vals);
> +		}
> +	}
> +
> +	return len;
> +}
> +
> +static IIO_DEVICE_ATTR(sensor_power, 0444, scmi_iio_get_sensor_power, NULL, 0);
> +static IIO_DEVICE_ATTR(sensor_max_range, 0444, scmi_iio_get_sensor_max_range,
> +		       NULL, 0);
> +static IIO_DEVICE_ATTR(sensor_resolution, 0444, scmi_iio_get_sensor_resolution,
> +		       NULL, 0);
> +
> +static struct attribute *scmi_iio_attributes[] = {
> +	&iio_dev_attr_sensor_power.dev_attr.attr,
> +	&iio_dev_attr_sensor_max_range.dev_attr.attr,
> +	&iio_dev_attr_sensor_resolution.dev_attr.attr,

Replied to these in the cover letter.  I think resolution is _scale in IIO terms.
(multiplier of 1LSB needed to get to real world units)
max_range might be doable using the *_raw_avail or by setting realbits more precisely
for the buffered channels.

Power is an odd one.  As I mentioned I'm not keen to do something IIO specific
for that as it's nothing to do with it being an IIO driven device as such.
I'm not sure how much luck we'll have with defining a more general scheme
though.


> +	NULL,
> +};
> +
> +static const struct attribute_group scmi_iio_attribute_group = {
> +	.attrs = scmi_iio_attributes,
> +};
> +
> +static const struct iio_info scmi_iio_info = {
> +	.read_raw = scmi_iio_read_raw,
> +	.read_avail = scmi_iio_read_avail,
> +	.write_raw = scmi_iio_write_raw,
> +	.attrs = &scmi_iio_attribute_group,
> +};
> +
> +static int scmi_iio_get_chan_type(u8 scmi_type, enum iio_chan_type *iio_type)
> +{
> +	switch (scmi_type) {
> +	case METERS_SEC_SQUARED:
> +		*iio_type = IIO_ACCEL;
> +		return 0;
> +	case RADIANS_SEC:
> +		*iio_type = IIO_ANGL_VEL;
> +		return 0;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int scmi_iio_get_chan_modifier(const char *name,
> +				      enum iio_modifier *modifier)
> +{
> +	int ret;
> +	char *pch = strrchr(name, '_');
> +
> +	if (!pch)
> +		return -EINVAL;
> +
> +	if (strcmp(pch + 1, "X") == 0)
> +		*modifier = IIO_MOD_X;
> +	else if (strcmp(pch + 1, "Y") == 0)
> +		*modifier = IIO_MOD_Y;
> +	else if (strcmp(pch + 1, "Z") == 0)
> +		*modifier = IIO_MOD_Z;
> +	else
> +		return -EINVAL;
> +
> +	return ret;
In the good path, ret has never been set.
return 0;


> +}
> +
> +static void scmi_iio_set_data_channel(struct iio_chan_spec *iio_chan,
> +				      enum iio_chan_type type,
> +				      enum iio_modifier mod, int scan_index)
> +{
> +	iio_chan->type = type;
> +	iio_chan->modified = 1;
> +	iio_chan->channel2 = mod;
> +	iio_chan->info_mask_separate = BIT(IIO_CHAN_INFO_SCALE);
> +	iio_chan->info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SAMP_FREQ);
> +	iio_chan->info_mask_shared_by_type_available = BIT(IIO_CHAN_INFO_SAMP_FREQ);
> +	iio_chan->scan_index = scan_index;
> +	iio_chan->scan_type.sign = 's';
> +	iio_chan->scan_type.realbits = 64;
> +	iio_chan->scan_type.storagebits = 64;
> +	iio_chan->scan_type.endianness = IIO_LE;
> +}
> +
> +static void scmi_iio_set_timestamp_channel(struct iio_chan_spec *iio_chan,
> +					   int scan_index)
> +{
> +	iio_chan->type = IIO_TIMESTAMP;
> +	iio_chan->channel = -1;
> +	iio_chan->scan_index = scan_index;
> +	iio_chan->scan_type.sign = 'u';
> +	iio_chan->scan_type.realbits = 64;
> +	iio_chan->scan_type.storagebits = 64;
> +}
> +
> +static int set_sampling_freq_avail(struct iio_dev *iio_dev)
> +{
> +	struct scmi_iio_priv *sensor = iio_priv(iio_dev);
> +	int i;
> +	struct sensor_freq freq;
> +	u64 cur_interval_ns, low_interval_ns, high_interval_ns, step_size_ns;
> +	unsigned int low_interval, high_interval, step_size;
> +
> +	sensor->freq_avail =  devm_kzalloc(&iio_dev->dev,
> +					sizeof(u32) * (sensor->sensor_info->intervals.count * 2),
> +					GFP_KERNEL);
> +	if (!sensor->freq_avail)
> +		return -ENOMEM;
> +
> +	if (sensor->sensor_info->intervals.segmented) {
> +		low_interval = sensor->sensor_info->intervals.desc[SCMI_SENS_INTVL_SEGMENT_LOW];
> +		low_interval_ns = convert_interval_to_ns(low_interval);
> +		convert_ns_to_freq(low_interval_ns, &freq);
> +		sensor->freq_avail[0] = freq.hz;
> +		sensor->freq_avail[1] = freq.uhz;
> +
> +		step_size = sensor->sensor_info->intervals.desc[SCMI_SENS_INTVL_SEGMENT_STEP];
> +		step_size_ns = convert_interval_to_ns(step_size);
> +		convert_ns_to_freq(step_size_ns, &freq);
> +		sensor->freq_avail[2] = freq.hz;
> +		sensor->freq_avail[3] = freq.uhz;
> +
> +		high_interval = sensor->sensor_info->intervals.desc[SCMI_SENS_INTVL_SEGMENT_HIGH];
> +		high_interval_ns = convert_interval_to_ns(high_interval);
> +		convert_ns_to_freq(high_interval_ns, &freq);
> +		sensor->freq_avail[4] = freq.hz;
> +		sensor->freq_avail[5] = freq.uhz;
> +	} else {
> +		for (i = 0; i < sensor->sensor_info->intervals.count; i++) {
> +			cur_interval_ns = convert_interval_to_ns
> +						(sensor->sensor_info->intervals.desc[i]);
> +			convert_ns_to_freq(cur_interval_ns, &freq);
> +			sensor->freq_avail[i*2] = freq.hz;
> +			sensor->freq_avail[i*2+1] = freq.uhz;

Spacing as per kernel style guide https://www.kernel.org/doc/html/v4.10/process/coding-style.html

[i * 2]
[i * 2 + 1] 

> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static int scmi_iio_buffers_setup(struct iio_dev *scmi_iiodev)
> +{
> +	struct iio_buffer *buffer = devm_iio_kfifo_allocate(&scmi_iiodev->dev);
> +
> +	if (!buffer)
> +		return -ENOMEM;
> +
> +	iio_device_attach_buffer(scmi_iiodev, buffer);
> +	scmi_iiodev->modes |= INDIO_BUFFER_SOFTWARE;
> +	scmi_iiodev->setup_ops = &scmi_iio_buffer_ops;
> +
> +	return 0;
> +}
> +
> +static int scmi_alloc_iiodev(struct device *dev, struct scmi_handle *handle,
> +			     const struct scmi_sensor_info *sensor_info,
> +			     struct iio_dev **scmi_iio_dev)
> +{
> +	struct scmi_iio_priv *sensor;
> +	struct iio_chan_spec *iio_channels;
> +	enum iio_chan_type type;
> +	enum iio_modifier modifier;
> +	struct iio_dev *iiodev;
> +	int i, ret;
> +
> +	iiodev = devm_iio_device_alloc(dev, sizeof(*sensor));
> +	if (!iiodev)
> +		return -ENOMEM;
> +
> +	iiodev->modes = INDIO_DIRECT_MODE;
> +	iiodev->dev.parent = dev;
> +	sensor = iio_priv(iiodev);
> +	sensor->handle = handle;
> +	sensor->sensor_info = sensor_info;
> +	sensor->sensor_update_nb.notifier_call = sensor_update_cb;
> +	sensor->indio_dev = iiodev;
> +	iiodev->num_channels =
> +		sensor_info->num_axis +
> +		1; /* adding one additional channel for timestamp */

Perhaps move the comment to above the assignment then we can have
nicer looking indentation.
/* adding...
iiodev->num_channels = sensor_info->num_axis + 1;

> +	iiodev->name = sensor_info->name;
> +	iiodev->info = &scmi_iio_info;
> +
> +	iio_channels =
> +		devm_kzalloc(dev,
> +			     sizeof(*iio_channels) * (iiodev->num_channels),
> +			     GFP_KERNEL);
> +	if (!iio_channels)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < sensor_info->num_axis; i++) {
> +		ret = scmi_iio_get_chan_type(sensor_info->axis[i].type, &type);
> +		if (ret < 0)
> +			return ret;
> +
> +		ret = scmi_iio_get_chan_modifier(sensor_info->axis[i].name,
> +						 &modifier);
> +		if (ret < 0)
> +			return ret;
> +
> +		scmi_iio_set_data_channel(&iio_channels[i], type, modifier,
> +					  sensor_info->axis[i].id);
> +	}
> +
> +	scmi_iio_set_timestamp_channel(&iio_channels[i], i);
> +	iiodev->channels = iio_channels;
> +
> +	ret = set_sampling_freq_avail(iiodev);
> +	if (ret < 0)
> +		return ret;
> +
> +	*scmi_iio_dev = iiodev;
> +
> +	return ret;
> +}
> +
> +static int scmi_iio_dev_probe(struct scmi_device *sdev)
> +{
> +	struct iio_dev *scmi_iio_dev;
> +	const struct scmi_sensor_info *sensor_info;
> +	int err, i;
> +	u16 nr_sensors;
> +	struct device *dev = &sdev->dev;
> +	struct scmi_handle *handle = sdev->handle;
> +
> +	if (!handle || !handle->sensor_ops || !handle->sensor_ops->count_get ||
> +	    !handle->sensor_ops->info_get || !handle->sensor_ops->config_get ||
> +	    !handle->sensor_ops->config_set) {
> +		dev_err(dev, "SCMI device has no sensor interface\n");
> +		return -EINVAL;
> +	}
> +
> +	nr_sensors = handle->sensor_ops->count_get(handle);
> +	if (!nr_sensors) {
> +		dev_dbg(dev, "0 sensors found via SCMI bus\n");
> +		return -EINVAL;
> +	}
> +
> +	dev_dbg(dev, "%d sensors found via SCMI bus\n", nr_sensors);
> +
> +	for (i = 0; i < nr_sensors; i++) {
> +		sensor_info = handle->sensor_ops->info_get(handle, i);
> +		if (!sensor_info) {
> +			dev_err(dev, "SCMI sensor %d has missing info\n", i);
> +			return -EINVAL;
> +		}
> +
> +		/* Skipping scalar sensor,as this driver only supports accel and gyro */
> +		if (sensor_info->num_axis == 0)
> +			continue;
> +
> +		err = scmi_alloc_iiodev(dev, handle, sensor_info,
> +					&scmi_iio_dev);

I think you are under 80 chars with the above on one line.

> +		if (err < 0) {
> +			dev_err(dev,
> +				"failed to allocate IIO device for sensor %s: %d\n",
> +				sensor_info->name, err);
> +			return err;
> +		}
> +		if (!scmi_iio_dev) {

Looks to me like you can't get here.  If there was an issue with lack of memory it
would also have reported an err < 0 and been caught above.

> +			dev_err(dev, "memory allocation failed at sensor %s\n",
> +				sensor_info->name);
> +			return -ENOMEM;
> +		}
> +
> +		err = scmi_iio_buffers_setup(scmi_iio_dev);
> +		if (err < 0) {
> +			dev_err(dev,
> +				"IIO buffer setup error at sensor %s: %d\n",
> +				sensor_info->name, err);
> +			return err;
> +		}
> +
> +		err = devm_iio_device_register(dev, scmi_iio_dev);
> +		if (err) {
> +			dev_err(dev,
> +				"IIO device registration failed at sensor %s: %d\n",
> +				sensor_info->name, err);
> +			return err;
> +		}
> +	}
> +
> +	return err;
> +}
> +
> +static const struct scmi_device_id scmi_id_table[] = {
> +	{ SCMI_PROTOCOL_SENSOR, "iiodev" },
> +	{},
> +};
> +
> +MODULE_DEVICE_TABLE(scmi, scmi_id_table);
> +
> +static struct scmi_driver scmi_iiodev_driver = {
> +	.name = "scmi-sensor-iiodev",
> +	.probe = scmi_iio_dev_probe,
> +	.id_table = scmi_id_table,
> +};
> +
> +module_scmi_driver(scmi_iiodev_driver);
> +
> +MODULE_AUTHOR("Jyoti Bhayana <jbhayana@google.com>");
> +MODULE_DESCRIPTION("SCMI IIO Driver");
> +MODULE_LICENSE("GPL v2");

Re: [RFC PATCH v2 1/1] iio/scmi: Adding support for IIO SCMI Based Sensors

[Lars-Peter Clausen]

On 12/30/20 2:41 PM, Jonathan Cameron wrote:
> On Thu, 24 Dec 2020 03:19:21 +0000
> Jyoti Bhayana <jbhayana@google.com> wrote:
>
>> +	/*
>> +	 * The seconds field in the sensor interval in SCMI is 16 bits long
>> +	 * Therefore seconds  = 1/Hz <= 0xFFFF. As floating point calculations are
>> +	 * discouraged in the kernel driver code, to calculate the scale factor (sf)
>> +	 * (1* 1000000 * sf)/uHz <= 0xFFFF. Therefore, sf <= (uHz * 0xFFFF)/1000000
>> +	 *  To calculate the multiplier,we convert the sf into char string  and
>> +	 *  count the number of characters
>> +	 */
>> +
>> +	mult = scnprintf(buf, 32, "%llu", ((u64)uHz * 0xFFFF) / UHZ_PER_HZ) - 1;
> use sizeof(buf) instead of having 32 again.
>
Since this is just interested in the number of characters and not the 
string itself I believe it is possible to just call sprintf with NULL 
instead of a buffer. It will then still return the number of characters, 
but not print anything.

But maybe providing a ilog10() helper is the better approach.

Re: [RFC PATCH v2 0/1] Adding support for IIO SCMI based sensors

[Jonathan Cameron]

On Thu, 24 Dec 2020 03:19:20 +0000
Jyoti Bhayana <jbhayana@google.com> wrote:

> Hi,
> 
> This series adds support for ARM SCMI Protocol based IIO Device.
> This driver provides support for Accelerometer and Gyroscope sensor
> using new SCMI Sensor Protocol defined by the upcoming SCMIv3.0 ARM
> specification, which is available at 
> 
> https://developer.arm.com/documentation/den0056/c/
> 
> The series is currently based on top of:
> 
> commit f83eb664cdb4 ("Merge tag 'scmi-voltage-5.11' of git://git.kernel.org/pub/scm/linux/kernel/git/sudeep.holla/linux into for-next/scmi")
> 
> in Sudeep for-next/scmi branch:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/sudeep.holla/\
> linux.git/log/?h=for-next/scmi
> 
> This version of the patch series has been tested using 
> version 5.4.21 branch of Android common kernel.
> 
> Any feedback welcome,
> 
> Thanks,
> 
> Jyoti Bhayana
> 
> v1 --> v2
> - Incorporated the feedback comments from v1 review of the patch
> - Regarding the new ABI for sensor_power,sensor_max_range,
> and sensor_resolution, these are some of the sensor attributes
> which Android passes to the apps. If there is any other way of getting
> those values, please let us know

So, sensor_max_range can effectively be exposed as a combination of
scale and the *_raw_avail for a raw read (via the read_avail callback in IIO).
We'll ignore the fact the interface assumes a single value (so I assume symmetric?)

I think resolution in android is equivalent to _scale in IIO terms?
Docs seem to say it's "resolution of the sensor in sensor's units".
There are a few corners where that might not be true (sensors that do some
odd padding for example) but we can probably rely on it normally being fine.

Power.  Hmm. Currently we have no provision for this as users
of the sensor don't care.  The OS might of course, but individual applications
tend not to.

The question is much more general than IIO if we go down the route of
exposing this from the kernel as we should define some sort of power query
interface for any device in the system in a generic way. I'm not sure if
any such thing already exists.

Jonathan

> 
> Jyoti Bhayana (1):
>   iio/scmi: Adding support for IIO SCMI Based Sensors
> 
>  MAINTAINERS                                |   6 +
>  drivers/iio/common/Kconfig                 |   1 +
>  drivers/iio/common/Makefile                |   1 +
>  drivers/iio/common/scmi_sensors/Kconfig    |  18 +
>  drivers/iio/common/scmi_sensors/Makefile   |   5 +
>  drivers/iio/common/scmi_sensors/scmi_iio.c | 693 +++++++++++++++++++++
>  6 files changed, 724 insertions(+)
>  create mode 100644 drivers/iio/common/scmi_sensors/Kconfig
>  create mode 100644 drivers/iio/common/scmi_sensors/Makefile
>  create mode 100644 drivers/iio/common/scmi_sensors/scmi_iio.c
> 

Reply to [RFC PATCH v2 0/1] Adding support for IIO SCMI based sensors

[Jyoti Bhayana]

Hi Jonathan,

> So, sensor_max_range can effectively be exposed as a combination of
> scale and the *_raw_avail for a raw read (via the read_avail callback in IIO).
> We'll ignore the fact the interface assumes a single value (so I assume symmetric?)

Based on the SCMI specification the sensor min range and max range are 64 bits signed number.

looks like IIO_AVAIL_RANGE can only take the following
types of data which all looks like 32 bit. IIO_VAL_FRACTIONAL
also takes two int type numbers.
How can I send 64 bit sensor range using this and read_avail callback?

#define IIO_VAL_INT 1
#define IIO_VAL_INT_PLUS_MICRO 2
#define IIO_VAL_INT_PLUS_NANO 3
#define IIO_VAL_INT_PLUS_MICRO_DB 4
#define IIO_VAL_INT_MULTIPLE 5
#define IIO_VAL_FRACTIONAL 10
#define IIO_VAL_FRACTIONAL_LOG2 11
#define IIO_VAL_CHAR 12



Thanks,
Jyoti

Re: Reply to [RFC PATCH v2 0/1] Adding support for IIO SCMI based sensors

[Jonathan Cameron]

On Tue, 5 Jan 2021 23:09:20 +0000
Jyoti Bhayana <jbhayana@google.com> wrote:

> Hi Jonathan,
> 
> > So, sensor_max_range can effectively be exposed as a combination of
> > scale and the *_raw_avail for a raw read (via the read_avail callback in IIO).
> > We'll ignore the fact the interface assumes a single value (so I assume symmetric?)  
> 
> Based on the SCMI specification the sensor min range and max range are 64 bits signed number.
> 
> looks like IIO_AVAIL_RANGE can only take the following
> types of data which all looks like 32 bit. IIO_VAL_FRACTIONAL
> also takes two int type numbers.
> How can I send 64 bit sensor range using this and read_avail callback?
> 
> #define IIO_VAL_INT 1
> #define IIO_VAL_INT_PLUS_MICRO 2
> #define IIO_VAL_INT_PLUS_NANO 3
> #define IIO_VAL_INT_PLUS_MICRO_DB 4
> #define IIO_VAL_INT_MULTIPLE 5
> #define IIO_VAL_FRACTIONAL 10
> #define IIO_VAL_FRACTIONAL_LOG2 11
> #define IIO_VAL_CHAR 12

Hmm It is a bit unfortunate that SCMI decided to pretend that real sensor resolutions were
greater than 32 bits. I doubt they will ever actually be any (as such accurate measurements
are completely pointless) and they aren't anywhere near that today.  Only way it might end
up looking a bit like that would be result of a highly non linear sensor being shoved through
an interface that pretends it isn't (goody).

Having said that, specifications are what they are and we have to cope with that.

There is no real problem with defining a new value type except for the fact that any
legacy userspace won't necessarily expect to see values that large. Given we need the full
64 bits it would have to be something like
IIO_VAL_INT_H32_L32 with the 64 bit values split up appropriately and put back together
at time of formatting.   Not particularly pretty but I'm not keep to put that much effort
in to support something like this for one driver (so not interesting in changing that
the read_raw_* interfaces)

Jonathan
 

> 
> 
> 
> Thanks,
> Jyoti
> 

Re: Reply to [RFC PATCH v2 0/1] Adding support for IIO SCMI based sensors

[Cristian Marussi]

Hi

On Wed, Jan 06, 2021 at 10:29:17AM +0000, Jonathan Cameron wrote:
> On Tue, 5 Jan 2021 23:09:20 +0000
> Jyoti Bhayana <jbhayana@google.com> wrote:
> 
> > Hi Jonathan,
> > 
> > > So, sensor_max_range can effectively be exposed as a combination of
> > > scale and the *_raw_avail for a raw read (via the read_avail callback in IIO).
> > > We'll ignore the fact the interface assumes a single value (so I assume symmetric?)  
> > 
> > Based on the SCMI specification the sensor min range and max range are 64 bits signed number.
> > 
> > looks like IIO_AVAIL_RANGE can only take the following
> > types of data which all looks like 32 bit. IIO_VAL_FRACTIONAL
> > also takes two int type numbers.
> > How can I send 64 bit sensor range using this and read_avail callback?
> > 
> > #define IIO_VAL_INT 1
> > #define IIO_VAL_INT_PLUS_MICRO 2
> > #define IIO_VAL_INT_PLUS_NANO 3
> > #define IIO_VAL_INT_PLUS_MICRO_DB 4
> > #define IIO_VAL_INT_MULTIPLE 5
> > #define IIO_VAL_FRACTIONAL 10
> > #define IIO_VAL_FRACTIONAL_LOG2 11
> > #define IIO_VAL_CHAR 12
> 
> Hmm It is a bit unfortunate that SCMI decided to pretend that real sensor resolutions were
> greater than 32 bits. I doubt they will ever actually be any (as such accurate measurements
> are completely pointless) and they aren't anywhere near that today.  Only way it might end
> up looking a bit like that would be result of a highly non linear sensor being shoved through
> an interface that pretends it isn't (goody).
> 

We shared this info internally to involve our architects about this.

> Having said that, specifications are what they are and we have to cope with that.
> 
> There is no real problem with defining a new value type except for the fact that any
> legacy userspace won't necessarily expect to see values that large. Given we need the full
> 64 bits it would have to be something like
> IIO_VAL_INT_H32_L32 with the 64 bit values split up appropriately and put back together
> at time of formatting.   Not particularly pretty but I'm not keep to put that much effort
> in to support something like this for one driver (so not interesting in changing that
> the read_raw_* interfaces)
> 

Regarding the current spec and this IIODEV limit to 32bit, I was thinking about
the following horrid thing (:D) as a solution: given that as of now no sensor
exist in real life reporting bigger than 32bits values, instead of adding new
defines in IIODEV framework to support 64bit that no userspace is expecting and
no sensor will really emit ever in the foreseable future, couldn't this SCMI
IIODEV driver simply:

- truncate silently straight away 64bit vals into 32bit when detects
  that he upper MSB 32bit are unused (zeros or sign-extension) and in
  fact the values fits into 32bits

- WARN and do not truncate if the upper MSB 32bit are in fact valid because
  such a 64bit capable sensor was indeed used finally (at that point in time
  IIODEV driver and framework will need a 64bit update)

Or I am missing something ?

Feel free to insult me about this hack ... :D 

Thanks

Cristian

> Jonathan
>  
> 
> > 
> > 
> > 
> > Thanks,
> > Jyoti
> > 
> 

Re: Reply to [RFC PATCH v2 0/1] Adding support for IIO SCMI based sensors

[Jonathan Cameron]

On Wed, 6 Jan 2021 11:26:59 +0000
Cristian Marussi <cristian.marussi@arm.com> wrote:

> Hi
> 
> On Wed, Jan 06, 2021 at 10:29:17AM +0000, Jonathan Cameron wrote:
> > On Tue, 5 Jan 2021 23:09:20 +0000
> > Jyoti Bhayana <jbhayana@google.com> wrote:
> >   
> > > Hi Jonathan,
> > >   
> > > > So, sensor_max_range can effectively be exposed as a combination of
> > > > scale and the *_raw_avail for a raw read (via the read_avail callback in IIO).
> > > > We'll ignore the fact the interface assumes a single value (so I assume symmetric?)    
> > > 
> > > Based on the SCMI specification the sensor min range and max range are 64 bits signed number.
> > > 
> > > looks like IIO_AVAIL_RANGE can only take the following
> > > types of data which all looks like 32 bit. IIO_VAL_FRACTIONAL
> > > also takes two int type numbers.
> > > How can I send 64 bit sensor range using this and read_avail callback?
> > > 
> > > #define IIO_VAL_INT 1
> > > #define IIO_VAL_INT_PLUS_MICRO 2
> > > #define IIO_VAL_INT_PLUS_NANO 3
> > > #define IIO_VAL_INT_PLUS_MICRO_DB 4
> > > #define IIO_VAL_INT_MULTIPLE 5
> > > #define IIO_VAL_FRACTIONAL 10
> > > #define IIO_VAL_FRACTIONAL_LOG2 11
> > > #define IIO_VAL_CHAR 12  
> > 
> > Hmm It is a bit unfortunate that SCMI decided to pretend that real sensor resolutions were
> > greater than 32 bits. I doubt they will ever actually be any (as such accurate measurements
> > are completely pointless) and they aren't anywhere near that today.  Only way it might end
> > up looking a bit like that would be result of a highly non linear sensor being shoved through
> > an interface that pretends it isn't (goody).
> >   
> 
> We shared this info internally to involve our architects about this.
> 
> > Having said that, specifications are what they are and we have to cope with that.
> > 
> > There is no real problem with defining a new value type except for the fact that any
> > legacy userspace won't necessarily expect to see values that large. Given we need the full
> > 64 bits it would have to be something like
> > IIO_VAL_INT_H32_L32 with the 64 bit values split up appropriately and put back together
> > at time of formatting.   Not particularly pretty but I'm not keep to put that much effort
> > in to support something like this for one driver (so not interesting in changing that
> > the read_raw_* interfaces)
> >   
> 
> Regarding the current spec and this IIODEV limit to 32bit, I was thinking about
> the following horrid thing (:D) as a solution: given that as of now no sensor
> exist in real life reporting bigger than 32bits values, instead of adding new
> defines in IIODEV framework to support 64bit that no userspace is expecting and
> no sensor will really emit ever in the foreseable future, couldn't this SCMI
> IIODEV driver simply:
> 
> - truncate silently straight away 64bit vals into 32bit when detects
>   that he upper MSB 32bit are unused (zeros or sign-extension) and in
>   fact the values fits into 32bits
> 
> - WARN and do not truncate if the upper MSB 32bit are in fact valid because
>   such a 64bit capable sensor was indeed used finally (at that point in time
>   IIODEV driver and framework will need a 64bit update)
> 
> Or I am missing something ?
> 
> Feel free to insult me about this hack ... :D 

I had a similar thought :)  But every time we do something like this someone
does something crazy like right shifting the value by 24 bits or similar.
Warning should make it obvious if we need to paper over this though.

But I'm not sure it matters in reality as this is mostly hidden away inside
the kernel.   The exposure of these values is limited to:
1) In kernel users - probably aren't any current ones for these sensors.
2) Passing to the pretty print functions that produce sysfs values.
   The risk here is existing userspace may fail to parse a number that needs
   more than 32 bits as it's never seen one before.
   However, if the numbers happen to be smaller it will be fine.  So we are storing
   up potential esoteric bugs for a long time in the future.  Hopefully any such
   userspace will report an error though!

If we do support this, I would ask that we also add a channel to the
dummy driver to allow us to easily exercise relevant code paths without having
to emulate everything needed to fake devices behind SCMI (docs on how to do that
would be good as well if there are any (assuming there is public code that does
this by now!) :)

Jonathan

> 
> Thanks
> 
> Cristian
> 
> > Jonathan
> >  
> >   
> > > 
> > > 
> > > 
> > > Thanks,
> > > Jyoti
> > >   
> >   

Re: Reply to [RFC PATCH v2 0/1] Adding support for IIO SCMI based sensors

[Cristian Marussi]

On Wed, Jan 06, 2021 at 02:36:45PM +0000, Jonathan Cameron wrote:
> On Wed, 6 Jan 2021 11:26:59 +0000
> Cristian Marussi <cristian.marussi@arm.com> wrote:
> 
> > Hi
> > 
> > On Wed, Jan 06, 2021 at 10:29:17AM +0000, Jonathan Cameron wrote:
> > > On Tue, 5 Jan 2021 23:09:20 +0000
> > > Jyoti Bhayana <jbhayana@google.com> wrote:
> > >   
> > > > Hi Jonathan,
> > > >   
> > > > > So, sensor_max_range can effectively be exposed as a combination of
> > > > > scale and the *_raw_avail for a raw read (via the read_avail callback in IIO).
> > > > > We'll ignore the fact the interface assumes a single value (so I assume symmetric?)    
> > > > 
> > > > Based on the SCMI specification the sensor min range and max range are 64 bits signed number.
> > > > 
> > > > looks like IIO_AVAIL_RANGE can only take the following
> > > > types of data which all looks like 32 bit. IIO_VAL_FRACTIONAL
> > > > also takes two int type numbers.
> > > > How can I send 64 bit sensor range using this and read_avail callback?
> > > > 
> > > > #define IIO_VAL_INT 1
> > > > #define IIO_VAL_INT_PLUS_MICRO 2
> > > > #define IIO_VAL_INT_PLUS_NANO 3
> > > > #define IIO_VAL_INT_PLUS_MICRO_DB 4
> > > > #define IIO_VAL_INT_MULTIPLE 5
> > > > #define IIO_VAL_FRACTIONAL 10
> > > > #define IIO_VAL_FRACTIONAL_LOG2 11
> > > > #define IIO_VAL_CHAR 12  
> > > 
> > > Hmm It is a bit unfortunate that SCMI decided to pretend that real sensor resolutions were
> > > greater than 32 bits. I doubt they will ever actually be any (as such accurate measurements
> > > are completely pointless) and they aren't anywhere near that today.  Only way it might end
> > > up looking a bit like that would be result of a highly non linear sensor being shoved through
> > > an interface that pretends it isn't (goody).
> > >   
> > 
> > We shared this info internally to involve our architects about this.
> > 
> > > Having said that, specifications are what they are and we have to cope with that.
> > > 
> > > There is no real problem with defining a new value type except for the fact that any
> > > legacy userspace won't necessarily expect to see values that large. Given we need the full
> > > 64 bits it would have to be something like
> > > IIO_VAL_INT_H32_L32 with the 64 bit values split up appropriately and put back together
> > > at time of formatting.   Not particularly pretty but I'm not keep to put that much effort
> > > in to support something like this for one driver (so not interesting in changing that
> > > the read_raw_* interfaces)
> > >   
> > 
> > Regarding the current spec and this IIODEV limit to 32bit, I was thinking about
> > the following horrid thing (:D) as a solution: given that as of now no sensor
> > exist in real life reporting bigger than 32bits values, instead of adding new
> > defines in IIODEV framework to support 64bit that no userspace is expecting and
> > no sensor will really emit ever in the foreseable future, couldn't this SCMI
> > IIODEV driver simply:
> > 
> > - truncate silently straight away 64bit vals into 32bit when detects
> >   that he upper MSB 32bit are unused (zeros or sign-extension) and in
> >   fact the values fits into 32bits
> > 
> > - WARN and do not truncate if the upper MSB 32bit are in fact valid because
> >   such a 64bit capable sensor was indeed used finally (at that point in time
> >   IIODEV driver and framework will need a 64bit update)
> > 
> > Or I am missing something ?
> > 
> > Feel free to insult me about this hack ... :D 
> 
> I had a similar thought :)  But every time we do something like this someone
> does something crazy like right shifting the value by 24 bits or similar.
> Warning should make it obvious if we need to paper over this though.

Not sure to have understood where this right shift could be done...on
sysfs output values you mean ?

It is pretty similar (only conceptually) to what happened with SCMIv3.0
Voltage and SCMI regulator: Voltage protocol supports negative voltages
representation to be generic enough to handle any kind of regulators
while regulator framework does NOT support negatives as of now (being mostly
used in a digital context AFAICU), so the SCMI regulator driver refuses to
handle any SCMI exposed voltage domain which declares itself as supporting
negative voltages. (and emit a pr_warn)

I was thinking around the same lines here, given that each SCMI sensor/axis
is exposed and described also by a descriptor containing scmi_range_attrs
(a min/max_range 64bit values), the SCMI IIODEV driver could simply skip them
at initialization time when it see an unsupported range and just do not expose
such an iiodev, but warn: this way nothing potentially representing something
greater than 32bit would even appear in sysfs.
(But I'm far from having a decent knowledge of IIO so I could be missing
something ovious here).

Thanks

Cristian

> 
> But I'm not sure it matters in reality as this is mostly hidden away inside
> the kernel.   The exposure of these values is limited to:
> 1) In kernel users - probably aren't any current ones for these sensors.
> 2) Passing to the pretty print functions that produce sysfs values.
>    The risk here is existing userspace may fail to parse a number that needs
>    more than 32 bits as it's never seen one before.
>    However, if the numbers happen to be smaller it will be fine.  So we are storing
>    up potential esoteric bugs for a long time in the future.  Hopefully any such
>    userspace will report an error though!
> 
> If we do support this, I would ask that we also add a channel to the
> dummy driver to allow us to easily exercise relevant code paths without having
> to emulate everything needed to fake devices behind SCMI (docs on how to do that
> would be good as well if there are any (assuming there is public code that does
> this by now!) :)
> 
> Jonathan
> 
> > 
> > Thanks
> > 
> > Cristian
> > 
> > > Jonathan
> > >  
> > >   
> > > > 
> > > > 
> > > > 
> > > > Thanks,
> > > > Jyoti
> > > >   
> > >   
> 

Reply to [RFC PATCH v2 0/1] Adding support for IIO SCMI based sensors

[Jyoti Bhayana]

Hi Jonathan,

Instead of adding IIO_VAL_INT_H32_L32, I am thinking of adding IIO_VAL_FRACTIONAL_LONG
or IIO_VAL_FRACTIONAL_64 as the scale/exponent used for min/max range can be different
than the one used in resolution according to specification. 

I am planning to use read_avail for IIO_CHAN_INFO_PROCESSED using IIO_AVAIL_RANGE 
and this new IIO_VAL_FRACTIONAL_64 for min range,max range and resolution.
Instead of two values used in IIO_VAL_FRACTIONAL, IIO_VAL_FRACTIONAL_64 will use 4 values
val_high,val_low,and val2_high and val2_low.

Let me know if that is an acceptable solution.


Thanks,
Jyoti

Re: Reply to [RFC PATCH v2 0/1] Adding support for IIO SCMI based sensors

[Jonathan Cameron]

On Wed,  6 Jan 2021 21:23:53 +0000
Jyoti Bhayana <jbhayana@google.com> wrote:

> Hi Jonathan,
> 
> Instead of adding IIO_VAL_INT_H32_L32, I am thinking of adding IIO_VAL_FRACTIONAL_LONG
> or IIO_VAL_FRACTIONAL_64 as the scale/exponent used for min/max range can be different
> than the one used in resolution according to specification. 

That's somewhat 'odd'.  Given min/max are inherently values the sensor is supposed to
be able to return why give them different resolutions?  Can you point me at a specific
section of the spec?  The axis_min_range_low etc fields don't seem to have units specified
but I assumed they were in sensor units and so same scale factors?

> 
> I am planning to use read_avail for IIO_CHAN_INFO_PROCESSED using IIO_AVAIL_RANGE 
> and this new IIO_VAL_FRACTIONAL_64 for min range,max range and resolution.
> Instead of two values used in IIO_VAL_FRACTIONAL, IIO_VAL_FRACTIONAL_64 will use 4 values
> val_high,val_low,and val2_high and val2_low.

I'm not keen on the changing that internal kernel interface unless we absolutely
have to.  read_avail() is called from consumer drivers and they won't know anything
about this new variant.

> 
> Let me know if that is an acceptable solution.

Hmm. It isn't a standard use of the ABI given the value in the buffer is (I assume)
raw (needs scale applied).  However, it isn't excluded by the ABI docs.  Whether
a standard userspace is going to expect it is not clear to me.

I don't want to end up in a position where we end up with available being generally
added for processed when what most people care about is what the value range they
might get from a polled read is (rather than via a buffer).

So I'm not that keen on this solution but if we can find a way to avoid it.

Jonathan


> 
> 
> Thanks,
> Jyoti
> 

Re: Reply to [RFC PATCH v2 0/1] Adding support for IIO SCMI based sensors

[Jyoti Bhayana]

Hi Jonathan,

In section 4.7.2.5.1 of the specification, the following exponent is
the scale value

uint32 axis_attributes_high
Bits[15:11] Exponent: The power-of-10 multiplier in two’s-complement
format that is applied to the sensor unit
specified by the SensorType field.

and the resolution is

uint32 axis_resolution
Bits[31:27] Exponent: The power-of-10 multiplier in two’s-complement format
that is applied to the Res field. Bits[26:0] Res: The resolution of
the sensor axis.

From code in scmi_protocol.h
/**
 * scmi_sensor_axis_info  - describes one sensor axes
 * @id: The axes ID.
 * @type: Axes type. Chosen amongst one of @enum scmi_sensor_class.
 * @scale: Power-of-10 multiplier applied to the axis unit.
 * @name: NULL-terminated string representing axes name as advertised by
 *  SCMI platform.
 * @extended_attrs: Flag to indicate the presence of additional extended
 *    attributes for this axes.
 * @resolution: Extended attribute representing the resolution of the axes.
 * Set to 0 if not reported by this axes.
 * @exponent: Extended attribute representing the power-of-10 multiplier that
 *      is applied to the resolution field. Set to 0 if not reported by
 *      this axes.
 * @attrs: Extended attributes representing minimum and maximum values
 *   measurable by this axes. Set to 0 if not reported by this sensor.
 */

struct scmi_sensor_axis_info {
unsigned int id;
unsigned int type;
int scale; //This is the scale used for min/max range
char name[SCMI_MAX_STR_SIZE];
bool extended_attrs;
unsigned int resolution;
int exponent; // This is the scale used in resolution
struct scmi_range_attrs attrs;
};

The scale above  is the Power-of-10 multiplier which is applied to the min range
and the max range value
but the resolution is equal to resolution and multiplied by
Power-of-10 multiplier
of exponent in the above struct.
So as can be seen above the value of the power of 10 multiplier used
for min/max range
can be different than the value of the power of 10 multiplier used for
the resolution.
Hence, if I have to use IIO_AVAIL_RANGE to specify min/max range and as well
as resolution, then I have to use the float format with the scale applied.

If there is another way which you know of and prefer, please let me know.

Thanks,
Jyoti




Thanks,
Jyoti

On Sat, Jan 9, 2021 at 11:01 AM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Wed,  6 Jan 2021 21:23:53 +0000
> Jyoti Bhayana <jbhayana@google.com> wrote:
>
> > Hi Jonathan,
> >
> > Instead of adding IIO_VAL_INT_H32_L32, I am thinking of adding IIO_VAL_FRACTIONAL_LONG
> > or IIO_VAL_FRACTIONAL_64 as the scale/exponent used for min/max range can be different
> > than the one used in resolution according to specification.
>
> That's somewhat 'odd'.  Given min/max are inherently values the sensor is supposed to
> be able to return why give them different resolutions?  Can you point me at a specific
> section of the spec?  The axis_min_range_low etc fields don't seem to have units specified
> but I assumed they were in sensor units and so same scale factors?
>
> >
> > I am planning to use read_avail for IIO_CHAN_INFO_PROCESSED using IIO_AVAIL_RANGE
> > and this new IIO_VAL_FRACTIONAL_64 for min range,max range and resolution.
> > Instead of two values used in IIO_VAL_FRACTIONAL, IIO_VAL_FRACTIONAL_64 will use 4 values
> > val_high,val_low,and val2_high and val2_low.
>
> I'm not keen on the changing that internal kernel interface unless we absolutely
> have to.  read_avail() is called from consumer drivers and they won't know anything
> about this new variant.
>
> >
> > Let me know if that is an acceptable solution.
>
> Hmm. It isn't a standard use of the ABI given the value in the buffer is (I assume)
> raw (needs scale applied).  However, it isn't excluded by the ABI docs.  Whether
> a standard userspace is going to expect it is not clear to me.
>
> I don't want to end up in a position where we end up with available being generally
> added for processed when what most people care about is what the value range they
> might get from a polled read is (rather than via a buffer).
>
> So I'm not that keen on this solution but if we can find a way to avoid it.
>
> Jonathan
>
>
> >
> >
> > Thanks,
> > Jyoti
> >
>

Re: Reply to [RFC PATCH v2 0/1] Adding support for IIO SCMI based sensors

[Jonathan Cameron]

On Sun, 10 Jan 2021 22:44:44 -0800
Jyoti Bhayana <jbhayana@google.com> wrote:

> Hi Jonathan,
> 
> In section 4.7.2.5.1 of the specification, the following exponent is
> the scale value
> 
> uint32 axis_attributes_high
> Bits[15:11] Exponent: The power-of-10 multiplier in two’s-complement
> format that is applied to the sensor unit
> specified by the SensorType field.
> 
> and the resolution is
> 
> uint32 axis_resolution
> Bits[31:27] Exponent: The power-of-10 multiplier in two’s-complement format
> that is applied to the Res field. Bits[26:0] Res: The resolution of
> the sensor axis.
> 
> From code in scmi_protocol.h
> /**
>  * scmi_sensor_axis_info  - describes one sensor axes
>  * @id: The axes ID.
>  * @type: Axes type. Chosen amongst one of @enum scmi_sensor_class.
>  * @scale: Power-of-10 multiplier applied to the axis unit.
>  * @name: NULL-terminated string representing axes name as advertised by
>  *  SCMI platform.
>  * @extended_attrs: Flag to indicate the presence of additional extended
>  *    attributes for this axes.
>  * @resolution: Extended attribute representing the resolution of the axes.
>  * Set to 0 if not reported by this axes.
>  * @exponent: Extended attribute representing the power-of-10 multiplier that
>  *      is applied to the resolution field. Set to 0 if not reported by
>  *      this axes.
>  * @attrs: Extended attributes representing minimum and maximum values
>  *   measurable by this axes. Set to 0 if not reported by this sensor.
>  */
> 
> struct scmi_sensor_axis_info {
> unsigned int id;
> unsigned int type;
> int scale; //This is the scale used for min/max range
> char name[SCMI_MAX_STR_SIZE];
> bool extended_attrs;
> unsigned int resolution;
> int exponent; // This is the scale used in resolution
> struct scmi_range_attrs attrs;
> };
> 
> The scale above  is the Power-of-10 multiplier which is applied to the min range
> and the max range value
> but the resolution is equal to resolution and multiplied by
> Power-of-10 multiplier
> of exponent in the above struct.
> So as can be seen above the value of the power of 10 multiplier used
> for min/max range
> can be different than the value of the power of 10 multiplier used for
> the resolution.
> Hence, if I have to use IIO_AVAIL_RANGE to specify min/max range and as well
> as resolution, then I have to use the float format with the scale applied.
> 
> If there is another way which you know of and prefer, please let me know.
I'll confess I've gotten a bit lost here.

So I think where we are is how to describe the range of the sensor and why we can't
use in_accel_x_raw_available to provide the 

Understood that the resolution can have different scaling.  That is presumably
to allow for the case where a device is reporting values at a finer scale than
it's real resolution.  Resolution might take into account expected noise for
example.  So it should be decoupled from the scaling of both the actual measurements
and the axis high / low limits.

However, I'd read that as saying the axis high / low limits and the actual sensor
readings should be scaled by the exponent in axis_attributes_high.
So I think we are fine for the range, but my earlier assumption that resolution
was equivalent to scale in IIO (real world value for 1LSB) may be completely wrong
as resolution may be unconnected to how you convert to a real world value?

If nothing else I'd like to suggest the spec needs to be tightened a bit here
to say exactly how we convert a value coming in to real world units (maybe
I'm just missing it).

Anyhow, I suspect we've been looking at this the wrong way and what we actually
need is non standard ABI for resolution.

Jonathan




> 
> Thanks,
> Jyoti
> 
> 
> 
> 
> Thanks,
> Jyoti
> 
> On Sat, Jan 9, 2021 at 11:01 AM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Wed,  6 Jan 2021 21:23:53 +0000
> > Jyoti Bhayana <jbhayana@google.com> wrote:
> >  
> > > Hi Jonathan,
> > >
> > > Instead of adding IIO_VAL_INT_H32_L32, I am thinking of adding IIO_VAL_FRACTIONAL_LONG
> > > or IIO_VAL_FRACTIONAL_64 as the scale/exponent used for min/max range can be different
> > > than the one used in resolution according to specification.  
> >
> > That's somewhat 'odd'.  Given min/max are inherently values the sensor is supposed to
> > be able to return why give them different resolutions?  Can you point me at a specific
> > section of the spec?  The axis_min_range_low etc fields don't seem to have units specified
> > but I assumed they were in sensor units and so same scale factors?
> >  
> > >
> > > I am planning to use read_avail for IIO_CHAN_INFO_PROCESSED using IIO_AVAIL_RANGE
> > > and this new IIO_VAL_FRACTIONAL_64 for min range,max range and resolution.
> > > Instead of two values used in IIO_VAL_FRACTIONAL, IIO_VAL_FRACTIONAL_64 will use 4 values
> > > val_high,val_low,and val2_high and val2_low.  
> >
> > I'm not keen on the changing that internal kernel interface unless we absolutely
> > have to.  read_avail() is called from consumer drivers and they won't know anything
> > about this new variant.
> >  
> > >
> > > Let me know if that is an acceptable solution.  
> >
> > Hmm. It isn't a standard use of the ABI given the value in the buffer is (I assume)
> > raw (needs scale applied).  However, it isn't excluded by the ABI docs.  Whether
> > a standard userspace is going to expect it is not clear to me.
> >
> > I don't want to end up in a position where we end up with available being generally
> > added for processed when what most people care about is what the value range they
> > might get from a polled read is (rather than via a buffer).
> >
> > So I'm not that keen on this solution but if we can find a way to avoid it.
> >
> > Jonathan
> >
> >  
> > >
> > >
> > > Thanks,
> > > Jyoti
> > >  
> >  

Re: Reply to [RFC PATCH v2 0/1] Adding support for IIO SCMI based sensors

[Jyoti Bhayana]

Hi Jonathan,

I know it is a bit confusing. Let me try to explain it with some
examples to hopefully clarify some things here.
SCMI Platform talks to the native/actual sensor, gets the raw values
from the native sensor and applies the scale and then sends those
values to the SCMI agent and the SCMI IIO driver.
Since the sensor readings which SCMI IIO driver gets are integer, to
convert them to float , we need to apply scale to these sensor values
which is the unit_exponent(power-of-10 multiplier in two’s-complement
format) specified in the SCMI specification

Native Sensor -> SCMI platform->SCMI Agent->SCMI IIO Driver

So if Native Sensor gets the sensor value
32767 and the scale the SCMI Platform is using is 0.002392.
SCMI platform does the calculation of 32767 * 0.002392 = 78.378664
and send the sensor value as 78378664 and the scale as .000001 to the
SCMI agent and SCMI IIO driver

so for SCMI IIO driver the sensor reading = 78378664 and scale = .000001
and  the sensor value is sensor_reading * scale = 78378664 *  .000001
=  78.378664
and the resolution which the SCMI Platform sends to the SCMI agent is 0.002392.
In the SCMI IIO driver, scale which is .000001 is applied to the min
range/max range and the actual sensor values.
sensor resolution which is  0.002392 is just passed to the userspace
layer so that they know the Native sensor resolution/scale
being applied by the SCMI platform.

Regarding your comments in the previous email, when you mentioned
"what we actually
need is non standard ABI for resolution"? Does that mean that it is ok
to have sensor resolution
as the IIO attribute shown below?

static IIO_DEVICE_ATTR(sensor_resolution, 0444, scmi_iio_get_sensor_resolution,
                     NULL, 0);

static struct attribute *scmi_iio_attributes[] = {
       &iio_dev_attr_sensor_resolution.dev_attr.attr,
       NULL,
};

and for the min/max range, I can use the read_avail callback?

Also, for the min/max range, there were two options discussed in the
email thread:
option 1)  Add new IIO val Type IIO_VAL_INT_H32_L32, and modify the
iio_format_value to format the 64 bit int properly for the userspace
option 2) Ignore the H32 bits and use the existing IIO_VAL_INT as just
L32 bits should be sufficient for current sensor values.

Let me know which option you prefer for min/max range. and also please
confirm if it is ok to have an IIO attribute for resolution like
mentioned above.


Thanks,
Jyoti

Thank you so much

Jyoti



On Mon, Jan 11, 2021 at 4:34 AM Jonathan Cameron
<Jonathan.Cameron@huawei.com> wrote:
>
> On Sun, 10 Jan 2021 22:44:44 -0800
> Jyoti Bhayana <jbhayana@google.com> wrote:
>
> > Hi Jonathan,
> >
> > In section 4.7.2.5.1 of the specification, the following exponent is
> > the scale value
> >
> > uint32 axis_attributes_high
> > Bits[15:11] Exponent: The power-of-10 multiplier in two’s-complement
> > format that is applied to the sensor unit
> > specified by the SensorType field.
> >
> > and the resolution is
> >
> > uint32 axis_resolution
> > Bits[31:27] Exponent: The power-of-10 multiplier in two’s-complement format
> > that is applied to the Res field. Bits[26:0] Res: The resolution of
> > the sensor axis.
> >
> > From code in scmi_protocol.h
> > /**
> >  * scmi_sensor_axis_info  - describes one sensor axes
> >  * @id: The axes ID.
> >  * @type: Axes type. Chosen amongst one of @enum scmi_sensor_class.
> >  * @scale: Power-of-10 multiplier applied to the axis unit.
> >  * @name: NULL-terminated string representing axes name as advertised by
> >  *  SCMI platform.
> >  * @extended_attrs: Flag to indicate the presence of additional extended
> >  *    attributes for this axes.
> >  * @resolution: Extended attribute representing the resolution of the axes.
> >  * Set to 0 if not reported by this axes.
> >  * @exponent: Extended attribute representing the power-of-10 multiplier that
> >  *      is applied to the resolution field. Set to 0 if not reported by
> >  *      this axes.
> >  * @attrs: Extended attributes representing minimum and maximum values
> >  *   measurable by this axes. Set to 0 if not reported by this sensor.
> >  */
> >
> > struct scmi_sensor_axis_info {
> > unsigned int id;
> > unsigned int type;
> > int scale; //This is the scale used for min/max range
> > char name[SCMI_MAX_STR_SIZE];
> > bool extended_attrs;
> > unsigned int resolution;
> > int exponent; // This is the scale used in resolution
> > struct scmi_range_attrs attrs;
> > };
> >
> > The scale above  is the Power-of-10 multiplier which is applied to the min range
> > and the max range value
> > but the resolution is equal to resolution and multiplied by
> > Power-of-10 multiplier
> > of exponent in the above struct.
> > So as can be seen above the value of the power of 10 multiplier used
> > for min/max range
> > can be different than the value of the power of 10 multiplier used for
> > the resolution.
> > Hence, if I have to use IIO_AVAIL_RANGE to specify min/max range and as well
> > as resolution, then I have to use the float format with the scale applied.
> >
> > If there is another way which you know of and prefer, please let me know.
> I'll confess I've gotten a bit lost here.
>
> So I think where we are is how to describe the range of the sensor and why we can't
> use in_accel_x_raw_available to provide the
>
> Understood that the resolution can have different scaling.  That is presumably
> to allow for the case where a device is reporting values at a finer scale than
> it's real resolution.  Resolution might take into account expected noise for
> example.  So it should be decoupled from the scaling of both the actual measurements
> and the axis high / low limits.
>
> However, I'd read that as saying the axis high / low limits and the actual sensor
> readings should be scaled by the exponent in axis_attributes_high.
> So I think we are fine for the range, but my earlier assumption that resolution
> was equivalent to scale in IIO (real world value for 1LSB) may be completely wrong
> as resolution may be unconnected to how you convert to a real world value?
>
> If nothing else I'd like to suggest the spec needs to be tightened a bit here
> to say exactly how we convert a value coming in to real world units (maybe
> I'm just missing it).
>
> Anyhow, I suspect we've been looking at this the wrong way and what we actually
> need is non standard ABI for resolution.
>
> Jonathan
>
>
>
>
> >
> > Thanks,
> > Jyoti
> >
> >
> >
> >
> > Thanks,
> > Jyoti
> >
> > On Sat, Jan 9, 2021 at 11:01 AM Jonathan Cameron <jic23@kernel.org> wrote:
> > >
> > > On Wed,  6 Jan 2021 21:23:53 +0000
> > > Jyoti Bhayana <jbhayana@google.com> wrote:
> > >
> > > > Hi Jonathan,
> > > >
> > > > Instead of adding IIO_VAL_INT_H32_L32, I am thinking of adding IIO_VAL_FRACTIONAL_LONG
> > > > or IIO_VAL_FRACTIONAL_64 as the scale/exponent used for min/max range can be different
> > > > than the one used in resolution according to specification.
> > >
> > > That's somewhat 'odd'.  Given min/max are inherently values the sensor is supposed to
> > > be able to return why give them different resolutions?  Can you point me at a specific
> > > section of the spec?  The axis_min_range_low etc fields don't seem to have units specified
> > > but I assumed they were in sensor units and so same scale factors?
> > >
> > > >
> > > > I am planning to use read_avail for IIO_CHAN_INFO_PROCESSED using IIO_AVAIL_RANGE
> > > > and this new IIO_VAL_FRACTIONAL_64 for min range,max range and resolution.
> > > > Instead of two values used in IIO_VAL_FRACTIONAL, IIO_VAL_FRACTIONAL_64 will use 4 values
> > > > val_high,val_low,and val2_high and val2_low.
> > >
> > > I'm not keen on the changing that internal kernel interface unless we absolutely
> > > have to.  read_avail() is called from consumer drivers and they won't know anything
> > > about this new variant.
> > >
> > > >
> > > > Let me know if that is an acceptable solution.
> > >
> > > Hmm. It isn't a standard use of the ABI given the value in the buffer is (I assume)
> > > raw (needs scale applied).  However, it isn't excluded by the ABI docs.  Whether
> > > a standard userspace is going to expect it is not clear to me.
> > >
> > > I don't want to end up in a position where we end up with available being generally
> > > added for processed when what most people care about is what the value range they
> > > might get from a polled read is (rather than via a buffer).
> > >
> > > So I'm not that keen on this solution but if we can find a way to avoid it.
> > >
> > > Jonathan
> > >
> > >
> > > >
> > > >
> > > > Thanks,
> > > > Jyoti
> > > >
> > >
>

Re: Reply to [RFC PATCH v2 0/1] Adding support for IIO SCMI based sensors

[Jonathan Cameron]

On Mon, 11 Jan 2021 13:17:51 -0800
Jyoti Bhayana <jbhayana@google.com> wrote:

> Hi Jonathan,
> 
> I know it is a bit confusing. Let me try to explain it with some
> examples to hopefully clarify some things here.
> SCMI Platform talks to the native/actual sensor, gets the raw values
> from the native sensor and applies the scale and then sends those
> values to the SCMI agent and the SCMI IIO driver.
> Since the sensor readings which SCMI IIO driver gets are integer, to
> convert them to float , we need to apply scale to these sensor values
> which is the unit_exponent(power-of-10 multiplier in two’s-complement
> format) specified in the SCMI specification
> 
> Native Sensor -> SCMI platform->SCMI Agent->SCMI IIO Driver
> 
> So if Native Sensor gets the sensor value
> 32767 and the scale the SCMI Platform is using is 0.002392.
> SCMI platform does the calculation of 32767 * 0.002392 = 78.378664
> and send the sensor value as 78378664 and the scale as .000001 to the
> SCMI agent and SCMI IIO driver
> 
> so for SCMI IIO driver the sensor reading = 78378664 and scale = .000001
> and  the sensor value is sensor_reading * scale = 78378664 *  .000001
> =  78.378664
> and the resolution which the SCMI Platform sends to the SCMI agent is 0.002392.
> In the SCMI IIO driver, scale which is .000001 is applied to the min
> range/max range and the actual sensor values.
> sensor resolution which is  0.002392 is just passed to the userspace
> layer so that they know the Native sensor resolution/scale
> being applied by the SCMI platform.

That was pretty much where I'd gotten to.  
Whilst the reasoning might be different it is equivalent to a sensor
providing info on expected noise by giving a 'valid resolution'.
In that case as well you have a sensor providing a number that looks to have
more precision than it actually does.

Anyhow, that similarity doesn't really matter here.

I'll also add that a design that applies scale in two places is inherently
less than ideal.   A cleaner design would have maintained the separation
between scale and raw value all the way up the stack.  That would result
in 0 loss of information and also be a cleaner interface.
Ah well, we live with what we have :)

> 
> Regarding your comments in the previous email, when you mentioned
> "what we actually
> need is non standard ABI for resolution"? Does that mean that it is ok
> to have sensor resolution
> as the IIO attribute shown below?
> 
> static IIO_DEVICE_ATTR(sensor_resolution, 0444, scmi_iio_get_sensor_resolution,
>                      NULL, 0);

We could do something new (see later for why I don't think we need to)
Would need to clearly reflect what it applies to and I'm not sure resolution
is even an unambiguous name given sensor resolution is often described as 8bit
10bit etc.  E.g. this selection table from Maxim for ADCs.
https://www.maximintegrated.com/en/products/parametric/search.html?fam=prec_adc&tree=master&metaTitle=Precision%20ADCs%20(%20%205Msps)&hide=270
Of course sometimes it's also used for what you want here.

Hohum.  So we might be still be able to do this with standard ABI but we
are going to need to do some maths in the driver.
So if we were to express it via 

in_accel_raw_avail for example we could use the [low step high] form.

low and high are straight forward as those are expressed directly from
axis_min_range and axis_max_range which I think are in the same units
as the _raw channel itself.

For resolution, we have it expressed as [res] x 10^res_exponent
and if we just put that in as the 'step' above it would have the wrong
exponent (as we'd expect to still have to apply your 0.00001 from above
example).  

Hence we express it as [res] x 10^(res_exponent - exponent)

I'm going to slightly modify your example above because the two exponents
are the same so it's hard to tell if I have them right way around.
Hence let res = 0.00293 = 293 x 10^(-5)  (I just dropped the trailing 2)

scale = 10^(-6) exponent = -6

So step = 2392 x 10^(-5 + 6) = 2390
giving us

[min 2390 max] for _raw_available

Hence when userspace comes along and wants this in relevant base units (here
m/sec^2) it applies the x10^(-6) mutliplier from _scale we get out expected value
of 0.00239 m/sec^2

That should work for any case we see but the maths done in the driver will have
to cope with potential negative exponents for step.

One catch will be the 64 bit potential values for min and max :(

> 
> static struct attribute *scmi_iio_attributes[] = {
>        &iio_dev_attr_sensor_resolution.dev_attr.attr,
>        NULL,
> };
> 
> and for the min/max range, I can use the read_avail callback?

I would have said yes normally but if we are going to cope with
a potential floating point value for step as touched on above we
may have to do it by hand in the driver.  Not ideal but may
be only option :(

> 
> Also, for the min/max range, there were two options discussed in the
> email thread:
> option 1)  Add new IIO val Type IIO_VAL_INT_H32_L32, and modify the
> iio_format_value to format the 64 bit int properly for the userspace
> option 2) Ignore the H32 bits and use the existing IIO_VAL_INT as just
> L32 bits should be sufficient for current sensor values.

Ignore is a strong way of putting it.  We would definitely want to
shout about it if we do get anything set in H32. 

If we are fairly sure that we aren't going to anything greater than
32 bits than we are fine.

It should be possible to work through the worst cases given
limits of say +-20g for accelerometers for example and the relatively
limited exponents (5 bits).  + sensible resolution.
 
If it's fairly safe I'd like to go for option 2. as it would ensure we
can do floating point for the step (which is then used to compute the
resolution value for android)

Thanks

Jonathan

> 
> Let me know which option you prefer for min/max range. and also please
> confirm if it is ok to have an IIO attribute for resolution like
> mentioned above.
> 
> 
> Thanks,
> Jyoti
> 
> Thank you so much
> 
> Jyoti
> 
> 
> 
> On Mon, Jan 11, 2021 at 4:34 AM Jonathan Cameron
> <Jonathan.Cameron@huawei.com> wrote:
> >
> > On Sun, 10 Jan 2021 22:44:44 -0800
> > Jyoti Bhayana <jbhayana@google.com> wrote:
> >  
> > > Hi Jonathan,
> > >
> > > In section 4.7.2.5.1 of the specification, the following exponent is
> > > the scale value
> > >
> > > uint32 axis_attributes_high
> > > Bits[15:11] Exponent: The power-of-10 multiplier in two’s-complement
> > > format that is applied to the sensor unit
> > > specified by the SensorType field.
> > >
> > > and the resolution is
> > >
> > > uint32 axis_resolution
> > > Bits[31:27] Exponent: The power-of-10 multiplier in two’s-complement format
> > > that is applied to the Res field. Bits[26:0] Res: The resolution of
> > > the sensor axis.
> > >
> > > From code in scmi_protocol.h
> > > /**
> > >  * scmi_sensor_axis_info  - describes one sensor axes
> > >  * @id: The axes ID.
> > >  * @type: Axes type. Chosen amongst one of @enum scmi_sensor_class.
> > >  * @scale: Power-of-10 multiplier applied to the axis unit.
> > >  * @name: NULL-terminated string representing axes name as advertised by
> > >  *  SCMI platform.
> > >  * @extended_attrs: Flag to indicate the presence of additional extended
> > >  *    attributes for this axes.
> > >  * @resolution: Extended attribute representing the resolution of the axes.
> > >  * Set to 0 if not reported by this axes.
> > >  * @exponent: Extended attribute representing the power-of-10 multiplier that
> > >  *      is applied to the resolution field. Set to 0 if not reported by
> > >  *      this axes.
> > >  * @attrs: Extended attributes representing minimum and maximum values
> > >  *   measurable by this axes. Set to 0 if not reported by this sensor.
> > >  */
> > >
> > > struct scmi_sensor_axis_info {
> > > unsigned int id;
> > > unsigned int type;
> > > int scale; //This is the scale used for min/max range
> > > char name[SCMI_MAX_STR_SIZE];
> > > bool extended_attrs;
> > > unsigned int resolution;
> > > int exponent; // This is the scale used in resolution
> > > struct scmi_range_attrs attrs;
> > > };
> > >
> > > The scale above  is the Power-of-10 multiplier which is applied to the min range
> > > and the max range value
> > > but the resolution is equal to resolution and multiplied by
> > > Power-of-10 multiplier
> > > of exponent in the above struct.
> > > So as can be seen above the value of the power of 10 multiplier used
> > > for min/max range
> > > can be different than the value of the power of 10 multiplier used for
> > > the resolution.
> > > Hence, if I have to use IIO_AVAIL_RANGE to specify min/max range and as well
> > > as resolution, then I have to use the float format with the scale applied.
> > >
> > > If there is another way which you know of and prefer, please let me know.  
> > I'll confess I've gotten a bit lost here.
> >
> > So I think where we are is how to describe the range of the sensor and why we can't
> > use in_accel_x_raw_available to provide the
> >
> > Understood that the resolution can have different scaling.  That is presumably
> > to allow for the case where a device is reporting values at a finer scale than
> > it's real resolution.  Resolution might take into account expected noise for
> > example.  So it should be decoupled from the scaling of both the actual measurements
> > and the axis high / low limits.
> >
> > However, I'd read that as saying the axis high / low limits and the actual sensor
> > readings should be scaled by the exponent in axis_attributes_high.
> > So I think we are fine for the range, but my earlier assumption that resolution
> > was equivalent to scale in IIO (real world value for 1LSB) may be completely wrong
> > as resolution may be unconnected to how you convert to a real world value?
> >
> > If nothing else I'd like to suggest the spec needs to be tightened a bit here
> > to say exactly how we convert a value coming in to real world units (maybe
> > I'm just missing it).
> >
> > Anyhow, I suspect we've been looking at this the wrong way and what we actually
> > need is non standard ABI for resolution.
> >
> > Jonathan
> >
> >
> >
> >  
> > >
> > > Thanks,
> > > Jyoti
> > >
> > >
> > >
> > >
> > > Thanks,
> > > Jyoti
> > >
> > > On Sat, Jan 9, 2021 at 11:01 AM Jonathan Cameron <jic23@kernel.org> wrote:  
> > > >
> > > > On Wed,  6 Jan 2021 21:23:53 +0000
> > > > Jyoti Bhayana <jbhayana@google.com> wrote:
> > > >  
> > > > > Hi Jonathan,
> > > > >
> > > > > Instead of adding IIO_VAL_INT_H32_L32, I am thinking of adding IIO_VAL_FRACTIONAL_LONG
> > > > > or IIO_VAL_FRACTIONAL_64 as the scale/exponent used for min/max range can be different
> > > > > than the one used in resolution according to specification.  
> > > >
> > > > That's somewhat 'odd'.  Given min/max are inherently values the sensor is supposed to
> > > > be able to return why give them different resolutions?  Can you point me at a specific
> > > > section of the spec?  The axis_min_range_low etc fields don't seem to have units specified
> > > > but I assumed they were in sensor units and so same scale factors?
> > > >  
> > > > >
> > > > > I am planning to use read_avail for IIO_CHAN_INFO_PROCESSED using IIO_AVAIL_RANGE
> > > > > and this new IIO_VAL_FRACTIONAL_64 for min range,max range and resolution.
> > > > > Instead of two values used in IIO_VAL_FRACTIONAL, IIO_VAL_FRACTIONAL_64 will use 4 values
> > > > > val_high,val_low,and val2_high and val2_low.  
> > > >
> > > > I'm not keen on the changing that internal kernel interface unless we absolutely
> > > > have to.  read_avail() is called from consumer drivers and they won't know anything
> > > > about this new variant.
> > > >  
> > > > >
> > > > > Let me know if that is an acceptable solution.  
> > > >
> > > > Hmm. It isn't a standard use of the ABI given the value in the buffer is (I assume)
> > > > raw (needs scale applied).  However, it isn't excluded by the ABI docs.  Whether
> > > > a standard userspace is going to expect it is not clear to me.
> > > >
> > > > I don't want to end up in a position where we end up with available being generally
> > > > added for processed when what most people care about is what the value range they
> > > > might get from a polled read is (rather than via a buffer).
> > > >
> > > > So I'm not that keen on this solution but if we can find a way to avoid it.
> > > >
> > > > Jonathan
> > > >
> > > >  
> > > > >
> > > > >
> > > > > Thanks,
> > > > > Jyoti
> > > > >  
> > > >  
> >  

Re: Reply to [RFC PATCH v2 0/1] Adding support for IIO SCMI based sensors

[Jyoti Bhayana]

Hi Jonathan,

Can you clarify one thing ? If we go with option 2 which is using
IIO_AVAIL_RANGE for min,step,high using IIO_VAL_INT then how will it
ensure the possible floating value for step as we are using int type?

Thanks,
Jyoti

On Sat, Jan 16, 2021 at 11:33 AM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Mon, 11 Jan 2021 13:17:51 -0800
> Jyoti Bhayana <jbhayana@google.com> wrote:
>
> > Hi Jonathan,
> >
> > I know it is a bit confusing. Let me try to explain it with some
> > examples to hopefully clarify some things here.
> > SCMI Platform talks to the native/actual sensor, gets the raw values
> > from the native sensor and applies the scale and then sends those
> > values to the SCMI agent and the SCMI IIO driver.
> > Since the sensor readings which SCMI IIO driver gets are integer, to
> > convert them to float , we need to apply scale to these sensor values
> > which is the unit_exponent(power-of-10 multiplier in two’s-complement
> > format) specified in the SCMI specification
> >
> > Native Sensor -> SCMI platform->SCMI Agent->SCMI IIO Driver
> >
> > So if Native Sensor gets the sensor value
> > 32767 and the scale the SCMI Platform is using is 0.002392.
> > SCMI platform does the calculation of 32767 * 0.002392 = 78.378664
> > and send the sensor value as 78378664 and the scale as .000001 to the
> > SCMI agent and SCMI IIO driver
> >
> > so for SCMI IIO driver the sensor reading = 78378664 and scale = .000001
> > and  the sensor value is sensor_reading * scale = 78378664 *  .000001
> > =  78.378664
> > and the resolution which the SCMI Platform sends to the SCMI agent is 0.002392.
> > In the SCMI IIO driver, scale which is .000001 is applied to the min
> > range/max range and the actual sensor values.
> > sensor resolution which is  0.002392 is just passed to the userspace
> > layer so that they know the Native sensor resolution/scale
> > being applied by the SCMI platform.
>
> That was pretty much where I'd gotten to.
> Whilst the reasoning might be different it is equivalent to a sensor
> providing info on expected noise by giving a 'valid resolution'.
> In that case as well you have a sensor providing a number that looks to have
> more precision than it actually does.
>
> Anyhow, that similarity doesn't really matter here.
>
> I'll also add that a design that applies scale in two places is inherently
> less than ideal.   A cleaner design would have maintained the separation
> between scale and raw value all the way up the stack.  That would result
> in 0 loss of information and also be a cleaner interface.
> Ah well, we live with what we have :)
>
> >
> > Regarding your comments in the previous email, when you mentioned
> > "what we actually
> > need is non standard ABI for resolution"? Does that mean that it is ok
> > to have sensor resolution
> > as the IIO attribute shown below?
> >
> > static IIO_DEVICE_ATTR(sensor_resolution, 0444, scmi_iio_get_sensor_resolution,
> >                      NULL, 0);
>
> We could do something new (see later for why I don't think we need to)
> Would need to clearly reflect what it applies to and I'm not sure resolution
> is even an unambiguous name given sensor resolution is often described as 8bit
> 10bit etc.  E.g. this selection table from Maxim for ADCs.
> https://www.maximintegrated.com/en/products/parametric/search.html?fam=prec_adc&tree=master&metaTitle=Precision%20ADCs%20(%20%205Msps)&hide=270
> Of course sometimes it's also used for what you want here.
>
> Hohum.  So we might be still be able to do this with standard ABI but we
> are going to need to do some maths in the driver.
> So if we were to express it via
>
> in_accel_raw_avail for example we could use the [low step high] form.
>
> low and high are straight forward as those are expressed directly from
> axis_min_range and axis_max_range which I think are in the same units
> as the _raw channel itself.
>
> For resolution, we have it expressed as [res] x 10^res_exponent
> and if we just put that in as the 'step' above it would have the wrong
> exponent (as we'd expect to still have to apply your 0.00001 from above
> example).
>
> Hence we express it as [res] x 10^(res_exponent - exponent)
>
> I'm going to slightly modify your example above because the two exponents
> are the same so it's hard to tell if I have them right way around.
> Hence let res = 0.00293 = 293 x 10^(-5)  (I just dropped the trailing 2)
>
> scale = 10^(-6) exponent = -6
>
> So step = 2392 x 10^(-5 + 6) = 2390
> giving us
>
> [min 2390 max] for _raw_available
>
> Hence when userspace comes along and wants this in relevant base units (here
> m/sec^2) it applies the x10^(-6) mutliplier from _scale we get out expected value
> of 0.00239 m/sec^2
>
> That should work for any case we see but the maths done in the driver will have
> to cope with potential negative exponents for step.
>
> One catch will be the 64 bit potential values for min and max :(
>
> >
> > static struct attribute *scmi_iio_attributes[] = {
> >        &iio_dev_attr_sensor_resolution.dev_attr.attr,
> >        NULL,
> > };
> >
> > and for the min/max range, I can use the read_avail callback?
>
> I would have said yes normally but if we are going to cope with
> a potential floating point value for step as touched on above we
> may have to do it by hand in the driver.  Not ideal but may
> be only option :(
>
> >
> > Also, for the min/max range, there were two options discussed in the
> > email thread:
> > option 1)  Add new IIO val Type IIO_VAL_INT_H32_L32, and modify the
> > iio_format_value to format the 64 bit int properly for the userspace
> > option 2) Ignore the H32 bits and use the existing IIO_VAL_INT as just
> > L32 bits should be sufficient for current sensor values.
>
> Ignore is a strong way of putting it.  We would definitely want to
> shout about it if we do get anything set in H32.
>
> If we are fairly sure that we aren't going to anything greater than
> 32 bits than we are fine.
>
> It should be possible to work through the worst cases given
> limits of say +-20g for accelerometers for example and the relatively
> limited exponents (5 bits).  + sensible resolution.
>
> If it's fairly safe I'd like to go for option 2. as it would ensure we
> can do floating point for the step (which is then used to compute the
> resolution value for android)
>
> Thanks
>
> Jonathan
>
> >
> > Let me know which option you prefer for min/max range. and also please
> > confirm if it is ok to have an IIO attribute for resolution like
> > mentioned above.
> >
> >
> > Thanks,
> > Jyoti
> >
> > Thank you so much
> >
> > Jyoti
> >
> >
> >
> > On Mon, Jan 11, 2021 at 4:34 AM Jonathan Cameron
> > <Jonathan.Cameron@huawei.com> wrote:
> > >
> > > On Sun, 10 Jan 2021 22:44:44 -0800
> > > Jyoti Bhayana <jbhayana@google.com> wrote:
> > >
> > > > Hi Jonathan,
> > > >
> > > > In section 4.7.2.5.1 of the specification, the following exponent is
> > > > the scale value
> > > >
> > > > uint32 axis_attributes_high
> > > > Bits[15:11] Exponent: The power-of-10 multiplier in two’s-complement
> > > > format that is applied to the sensor unit
> > > > specified by the SensorType field.
> > > >
> > > > and the resolution is
> > > >
> > > > uint32 axis_resolution
> > > > Bits[31:27] Exponent: The power-of-10 multiplier in two’s-complement format
> > > > that is applied to the Res field. Bits[26:0] Res: The resolution of
> > > > the sensor axis.
> > > >
> > > > From code in scmi_protocol.h
> > > > /**
> > > >  * scmi_sensor_axis_info  - describes one sensor axes
> > > >  * @id: The axes ID.
> > > >  * @type: Axes type. Chosen amongst one of @enum scmi_sensor_class.
> > > >  * @scale: Power-of-10 multiplier applied to the axis unit.
> > > >  * @name: NULL-terminated string representing axes name as advertised by
> > > >  *  SCMI platform.
> > > >  * @extended_attrs: Flag to indicate the presence of additional extended
> > > >  *    attributes for this axes.
> > > >  * @resolution: Extended attribute representing the resolution of the axes.
> > > >  * Set to 0 if not reported by this axes.
> > > >  * @exponent: Extended attribute representing the power-of-10 multiplier that
> > > >  *      is applied to the resolution field. Set to 0 if not reported by
> > > >  *      this axes.
> > > >  * @attrs: Extended attributes representing minimum and maximum values
> > > >  *   measurable by this axes. Set to 0 if not reported by this sensor.
> > > >  */
> > > >
> > > > struct scmi_sensor_axis_info {
> > > > unsigned int id;
> > > > unsigned int type;
> > > > int scale; //This is the scale used for min/max range
> > > > char name[SCMI_MAX_STR_SIZE];
> > > > bool extended_attrs;
> > > > unsigned int resolution;
> > > > int exponent; // This is the scale used in resolution
> > > > struct scmi_range_attrs attrs;
> > > > };
> > > >
> > > > The scale above  is the Power-of-10 multiplier which is applied to the min range
> > > > and the max range value
> > > > but the resolution is equal to resolution and multiplied by
> > > > Power-of-10 multiplier
> > > > of exponent in the above struct.
> > > > So as can be seen above the value of the power of 10 multiplier used
> > > > for min/max range
> > > > can be different than the value of the power of 10 multiplier used for
> > > > the resolution.
> > > > Hence, if I have to use IIO_AVAIL_RANGE to specify min/max range and as well
> > > > as resolution, then I have to use the float format with the scale applied.
> > > >
> > > > If there is another way which you know of and prefer, please let me know.
> > > I'll confess I've gotten a bit lost here.
> > >
> > > So I think where we are is how to describe the range of the sensor and why we can't
> > > use in_accel_x_raw_available to provide the
> > >
> > > Understood that the resolution can have different scaling.  That is presumably
> > > to allow for the case where a device is reporting values at a finer scale than
> > > it's real resolution.  Resolution might take into account expected noise for
> > > example.  So it should be decoupled from the scaling of both the actual measurements
> > > and the axis high / low limits.
> > >
> > > However, I'd read that as saying the axis high / low limits and the actual sensor
> > > readings should be scaled by the exponent in axis_attributes_high.
> > > So I think we are fine for the range, but my earlier assumption that resolution
> > > was equivalent to scale in IIO (real world value for 1LSB) may be completely wrong
> > > as resolution may be unconnected to how you convert to a real world value?
> > >
> > > If nothing else I'd like to suggest the spec needs to be tightened a bit here
> > > to say exactly how we convert a value coming in to real world units (maybe
> > > I'm just missing it).
> > >
> > > Anyhow, I suspect we've been looking at this the wrong way and what we actually
> > > need is non standard ABI for resolution.
> > >
> > > Jonathan
> > >
> > >
> > >
> > >
> > > >
> > > > Thanks,
> > > > Jyoti
> > > >
> > > >
> > > >
> > > >
> > > > Thanks,
> > > > Jyoti
> > > >
> > > > On Sat, Jan 9, 2021 at 11:01 AM Jonathan Cameron <jic23@kernel.org> wrote:
> > > > >
> > > > > On Wed,  6 Jan 2021 21:23:53 +0000
> > > > > Jyoti Bhayana <jbhayana@google.com> wrote:
> > > > >
> > > > > > Hi Jonathan,
> > > > > >
> > > > > > Instead of adding IIO_VAL_INT_H32_L32, I am thinking of adding IIO_VAL_FRACTIONAL_LONG
> > > > > > or IIO_VAL_FRACTIONAL_64 as the scale/exponent used for min/max range can be different
> > > > > > than the one used in resolution according to specification.
> > > > >
> > > > > That's somewhat 'odd'.  Given min/max are inherently values the sensor is supposed to
> > > > > be able to return why give them different resolutions?  Can you point me at a specific
> > > > > section of the spec?  The axis_min_range_low etc fields don't seem to have units specified
> > > > > but I assumed they were in sensor units and so same scale factors?
> > > > >
> > > > > >
> > > > > > I am planning to use read_avail for IIO_CHAN_INFO_PROCESSED using IIO_AVAIL_RANGE
> > > > > > and this new IIO_VAL_FRACTIONAL_64 for min range,max range and resolution.
> > > > > > Instead of two values used in IIO_VAL_FRACTIONAL, IIO_VAL_FRACTIONAL_64 will use 4 values
> > > > > > val_high,val_low,and val2_high and val2_low.
> > > > >
> > > > > I'm not keen on the changing that internal kernel interface unless we absolutely
> > > > > have to.  read_avail() is called from consumer drivers and they won't know anything
> > > > > about this new variant.
> > > > >
> > > > > >
> > > > > > Let me know if that is an acceptable solution.
> > > > >
> > > > > Hmm. It isn't a standard use of the ABI given the value in the buffer is (I assume)
> > > > > raw (needs scale applied).  However, it isn't excluded by the ABI docs.  Whether
> > > > > a standard userspace is going to expect it is not clear to me.
> > > > >
> > > > > I don't want to end up in a position where we end up with available being generally
> > > > > added for processed when what most people care about is what the value range they
> > > > > might get from a polled read is (rather than via a buffer).
> > > > >
> > > > > So I'm not that keen on this solution but if we can find a way to avoid it.
> > > > >
> > > > > Jonathan
> > > > >
> > > > >
> > > > > >
> > > > > >
> > > > > > Thanks,
> > > > > > Jyoti
> > > > > >
> > > > >
> > >
>

Re: Reply to [RFC PATCH v2 0/1] Adding support for IIO SCMI based sensors

[Jonathan Cameron]

On Sat, 16 Jan 2021 23:15:56 -0800
Jyoti Bhayana <jbhayana@google.com> wrote:

> Hi Jonathan,
> 
> Can you clarify one thing ? If we go with option 2 which is using
> IIO_AVAIL_RANGE for min,step,high using IIO_VAL_INT then how will it
> ensure the possible floating value for step as we are using int type?
As you've identified we can't go with IIO_VAL_INT if we are doing that
approach, would need to be IIO_VAL_INT_PLUS_MICRO (possibly NANO)

That is why it comes unstuck if we need to use both val and val2 to
do 64 bit integers...

For the  min and max we just set the val2 value to 0.

Jonathan


> 
> Thanks,
> Jyoti
> 
> On Sat, Jan 16, 2021 at 11:33 AM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Mon, 11 Jan 2021 13:17:51 -0800
> > Jyoti Bhayana <jbhayana@google.com> wrote:
> >  
> > > Hi Jonathan,
> > >
> > > I know it is a bit confusing. Let me try to explain it with some
> > > examples to hopefully clarify some things here.
> > > SCMI Platform talks to the native/actual sensor, gets the raw values
> > > from the native sensor and applies the scale and then sends those
> > > values to the SCMI agent and the SCMI IIO driver.
> > > Since the sensor readings which SCMI IIO driver gets are integer, to
> > > convert them to float , we need to apply scale to these sensor values
> > > which is the unit_exponent(power-of-10 multiplier in two’s-complement
> > > format) specified in the SCMI specification
> > >
> > > Native Sensor -> SCMI platform->SCMI Agent->SCMI IIO Driver
> > >
> > > So if Native Sensor gets the sensor value
> > > 32767 and the scale the SCMI Platform is using is 0.002392.
> > > SCMI platform does the calculation of 32767 * 0.002392 = 78.378664
> > > and send the sensor value as 78378664 and the scale as .000001 to the
> > > SCMI agent and SCMI IIO driver
> > >
> > > so for SCMI IIO driver the sensor reading = 78378664 and scale = .000001
> > > and  the sensor value is sensor_reading * scale = 78378664 *  .000001
> > > =  78.378664
> > > and the resolution which the SCMI Platform sends to the SCMI agent is 0.002392.
> > > In the SCMI IIO driver, scale which is .000001 is applied to the min
> > > range/max range and the actual sensor values.
> > > sensor resolution which is  0.002392 is just passed to the userspace
> > > layer so that they know the Native sensor resolution/scale
> > > being applied by the SCMI platform.  
> >
> > That was pretty much where I'd gotten to.
> > Whilst the reasoning might be different it is equivalent to a sensor
> > providing info on expected noise by giving a 'valid resolution'.
> > In that case as well you have a sensor providing a number that looks to have
> > more precision than it actually does.
> >
> > Anyhow, that similarity doesn't really matter here.
> >
> > I'll also add that a design that applies scale in two places is inherently
> > less than ideal.   A cleaner design would have maintained the separation
> > between scale and raw value all the way up the stack.  That would result
> > in 0 loss of information and also be a cleaner interface.
> > Ah well, we live with what we have :)
> >  
> > >
> > > Regarding your comments in the previous email, when you mentioned
> > > "what we actually
> > > need is non standard ABI for resolution"? Does that mean that it is ok
> > > to have sensor resolution
> > > as the IIO attribute shown below?
> > >
> > > static IIO_DEVICE_ATTR(sensor_resolution, 0444, scmi_iio_get_sensor_resolution,
> > >                      NULL, 0);  
> >
> > We could do something new (see later for why I don't think we need to)
> > Would need to clearly reflect what it applies to and I'm not sure resolution
> > is even an unambiguous name given sensor resolution is often described as 8bit
> > 10bit etc.  E.g. this selection table from Maxim for ADCs.
> > https://www.maximintegrated.com/en/products/parametric/search.html?fam=prec_adc&tree=master&metaTitle=Precision%20ADCs%20(%20%205Msps)&hide=270
> > Of course sometimes it's also used for what you want here.
> >
> > Hohum.  So we might be still be able to do this with standard ABI but we
> > are going to need to do some maths in the driver.
> > So if we were to express it via
> >
> > in_accel_raw_avail for example we could use the [low step high] form.
> >
> > low and high are straight forward as those are expressed directly from
> > axis_min_range and axis_max_range which I think are in the same units
> > as the _raw channel itself.
> >
> > For resolution, we have it expressed as [res] x 10^res_exponent
> > and if we just put that in as the 'step' above it would have the wrong
> > exponent (as we'd expect to still have to apply your 0.00001 from above
> > example).
> >
> > Hence we express it as [res] x 10^(res_exponent - exponent)
> >
> > I'm going to slightly modify your example above because the two exponents
> > are the same so it's hard to tell if I have them right way around.
> > Hence let res = 0.00293 = 293 x 10^(-5)  (I just dropped the trailing 2)
> >
> > scale = 10^(-6) exponent = -6
> >
> > So step = 2392 x 10^(-5 + 6) = 2390
> > giving us
> >
> > [min 2390 max] for _raw_available
> >
> > Hence when userspace comes along and wants this in relevant base units (here
> > m/sec^2) it applies the x10^(-6) mutliplier from _scale we get out expected value
> > of 0.00239 m/sec^2
> >
> > That should work for any case we see but the maths done in the driver will have
> > to cope with potential negative exponents for step.
> >
> > One catch will be the 64 bit potential values for min and max :(
> >  
> > >
> > > static struct attribute *scmi_iio_attributes[] = {
> > >        &iio_dev_attr_sensor_resolution.dev_attr.attr,
> > >        NULL,
> > > };
> > >
> > > and for the min/max range, I can use the read_avail callback?  
> >
> > I would have said yes normally but if we are going to cope with
> > a potential floating point value for step as touched on above we
> > may have to do it by hand in the driver.  Not ideal but may
> > be only option :(
> >  
> > >
> > > Also, for the min/max range, there were two options discussed in the
> > > email thread:
> > > option 1)  Add new IIO val Type IIO_VAL_INT_H32_L32, and modify the
> > > iio_format_value to format the 64 bit int properly for the userspace
> > > option 2) Ignore the H32 bits and use the existing IIO_VAL_INT as just
> > > L32 bits should be sufficient for current sensor values.  
> >
> > Ignore is a strong way of putting it.  We would definitely want to
> > shout about it if we do get anything set in H32.
> >
> > If we are fairly sure that we aren't going to anything greater than
> > 32 bits than we are fine.
> >
> > It should be possible to work through the worst cases given
> > limits of say +-20g for accelerometers for example and the relatively
> > limited exponents (5 bits).  + sensible resolution.
> >
> > If it's fairly safe I'd like to go for option 2. as it would ensure we
> > can do floating point for the step (which is then used to compute the
> > resolution value for android)
> >
> > Thanks
> >
> > Jonathan
> >  
> > >
> > > Let me know which option you prefer for min/max range. and also please
> > > confirm if it is ok to have an IIO attribute for resolution like
> > > mentioned above.
> > >
> > >
> > > Thanks,
> > > Jyoti
> > >
> > > Thank you so much
> > >
> > > Jyoti
> > >
> > >
> > >
> > > On Mon, Jan 11, 2021 at 4:34 AM Jonathan Cameron
> > > <Jonathan.Cameron@huawei.com> wrote:  
> > > >
> > > > On Sun, 10 Jan 2021 22:44:44 -0800
> > > > Jyoti Bhayana <jbhayana@google.com> wrote:
> > > >  
> > > > > Hi Jonathan,
> > > > >
> > > > > In section 4.7.2.5.1 of the specification, the following exponent is
> > > > > the scale value
> > > > >
> > > > > uint32 axis_attributes_high
> > > > > Bits[15:11] Exponent: The power-of-10 multiplier in two’s-complement
> > > > > format that is applied to the sensor unit
> > > > > specified by the SensorType field.
> > > > >
> > > > > and the resolution is
> > > > >
> > > > > uint32 axis_resolution
> > > > > Bits[31:27] Exponent: The power-of-10 multiplier in two’s-complement format
> > > > > that is applied to the Res field. Bits[26:0] Res: The resolution of
> > > > > the sensor axis.
> > > > >
> > > > > From code in scmi_protocol.h
> > > > > /**
> > > > >  * scmi_sensor_axis_info  - describes one sensor axes
> > > > >  * @id: The axes ID.
> > > > >  * @type: Axes type. Chosen amongst one of @enum scmi_sensor_class.
> > > > >  * @scale: Power-of-10 multiplier applied to the axis unit.
> > > > >  * @name: NULL-terminated string representing axes name as advertised by
> > > > >  *  SCMI platform.
> > > > >  * @extended_attrs: Flag to indicate the presence of additional extended
> > > > >  *    attributes for this axes.
> > > > >  * @resolution: Extended attribute representing the resolution of the axes.
> > > > >  * Set to 0 if not reported by this axes.
> > > > >  * @exponent: Extended attribute representing the power-of-10 multiplier that
> > > > >  *      is applied to the resolution field. Set to 0 if not reported by
> > > > >  *      this axes.
> > > > >  * @attrs: Extended attributes representing minimum and maximum values
> > > > >  *   measurable by this axes. Set to 0 if not reported by this sensor.
> > > > >  */
> > > > >
> > > > > struct scmi_sensor_axis_info {
> > > > > unsigned int id;
> > > > > unsigned int type;
> > > > > int scale; //This is the scale used for min/max range
> > > > > char name[SCMI_MAX_STR_SIZE];
> > > > > bool extended_attrs;
> > > > > unsigned int resolution;
> > > > > int exponent; // This is the scale used in resolution
> > > > > struct scmi_range_attrs attrs;
> > > > > };
> > > > >
> > > > > The scale above  is the Power-of-10 multiplier which is applied to the min range
> > > > > and the max range value
> > > > > but the resolution is equal to resolution and multiplied by
> > > > > Power-of-10 multiplier
> > > > > of exponent in the above struct.
> > > > > So as can be seen above the value of the power of 10 multiplier used
> > > > > for min/max range
> > > > > can be different than the value of the power of 10 multiplier used for
> > > > > the resolution.
> > > > > Hence, if I have to use IIO_AVAIL_RANGE to specify min/max range and as well
> > > > > as resolution, then I have to use the float format with the scale applied.
> > > > >
> > > > > If there is another way which you know of and prefer, please let me know.  
> > > > I'll confess I've gotten a bit lost here.
> > > >
> > > > So I think where we are is how to describe the range of the sensor and why we can't
> > > > use in_accel_x_raw_available to provide the
> > > >
> > > > Understood that the resolution can have different scaling.  That is presumably
> > > > to allow for the case where a device is reporting values at a finer scale than
> > > > it's real resolution.  Resolution might take into account expected noise for
> > > > example.  So it should be decoupled from the scaling of both the actual measurements
> > > > and the axis high / low limits.
> > > >
> > > > However, I'd read that as saying the axis high / low limits and the actual sensor
> > > > readings should be scaled by the exponent in axis_attributes_high.
> > > > So I think we are fine for the range, but my earlier assumption that resolution
> > > > was equivalent to scale in IIO (real world value for 1LSB) may be completely wrong
> > > > as resolution may be unconnected to how you convert to a real world value?
> > > >
> > > > If nothing else I'd like to suggest the spec needs to be tightened a bit here
> > > > to say exactly how we convert a value coming in to real world units (maybe
> > > > I'm just missing it).
> > > >
> > > > Anyhow, I suspect we've been looking at this the wrong way and what we actually
> > > > need is non standard ABI for resolution.
> > > >
> > > > Jonathan
> > > >
> > > >
> > > >
> > > >  
> > > > >
> > > > > Thanks,
> > > > > Jyoti
> > > > >
> > > > >
> > > > >
> > > > >
> > > > > Thanks,
> > > > > Jyoti
> > > > >
> > > > > On Sat, Jan 9, 2021 at 11:01 AM Jonathan Cameron <jic23@kernel.org> wrote:  
> > > > > >
> > > > > > On Wed,  6 Jan 2021 21:23:53 +0000
> > > > > > Jyoti Bhayana <jbhayana@google.com> wrote:
> > > > > >  
> > > > > > > Hi Jonathan,
> > > > > > >
> > > > > > > Instead of adding IIO_VAL_INT_H32_L32, I am thinking of adding IIO_VAL_FRACTIONAL_LONG
> > > > > > > or IIO_VAL_FRACTIONAL_64 as the scale/exponent used for min/max range can be different
> > > > > > > than the one used in resolution according to specification.  
> > > > > >
> > > > > > That's somewhat 'odd'.  Given min/max are inherently values the sensor is supposed to
> > > > > > be able to return why give them different resolutions?  Can you point me at a specific
> > > > > > section of the spec?  The axis_min_range_low etc fields don't seem to have units specified
> > > > > > but I assumed they were in sensor units and so same scale factors?
> > > > > >  
> > > > > > >
> > > > > > > I am planning to use read_avail for IIO_CHAN_INFO_PROCESSED using IIO_AVAIL_RANGE
> > > > > > > and this new IIO_VAL_FRACTIONAL_64 for min range,max range and resolution.
> > > > > > > Instead of two values used in IIO_VAL_FRACTIONAL, IIO_VAL_FRACTIONAL_64 will use 4 values
> > > > > > > val_high,val_low,and val2_high and val2_low.  
> > > > > >
> > > > > > I'm not keen on the changing that internal kernel interface unless we absolutely
> > > > > > have to.  read_avail() is called from consumer drivers and they won't know anything
> > > > > > about this new variant.
> > > > > >  
> > > > > > >
> > > > > > > Let me know if that is an acceptable solution.  
> > > > > >
> > > > > > Hmm. It isn't a standard use of the ABI given the value in the buffer is (I assume)
> > > > > > raw (needs scale applied).  However, it isn't excluded by the ABI docs.  Whether
> > > > > > a standard userspace is going to expect it is not clear to me.
> > > > > >
> > > > > > I don't want to end up in a position where we end up with available being generally
> > > > > > added for processed when what most people care about is what the value range they
> > > > > > might get from a polled read is (rather than via a buffer).
> > > > > >
> > > > > > So I'm not that keen on this solution but if we can find a way to avoid it.
> > > > > >
> > > > > > Jonathan
> > > > > >
> > > > > >  
> > > > > > >
> > > > > > >
> > > > > > > Thanks,
> > > > > > > Jyoti
> > > > > > >  
> > > > > >  
> > > >  
> >  

Re: Reply to [RFC PATCH v2 0/1] Adding support for IIO SCMI based sensors

[Jyoti Bhayana]

Hi Jonathan,

Instead of IIO_VAL_INT_PLUS_MICRO, we can also use IIO_VAL_FRACTIONAL
right for the raw values for min/max range and resolution? But, to
keep things simple though, it will be nice if the IIO driver uses
IIO_VAL_INT and throws an error if there is a negative exponent in the
following  calculation for the resolution which means the res_exponent
is always the same or greater than the scale_exponent.

 [res] x 10^(res_exponent - exponent)

so the IIO driver will use IIO_AVAIL_RANGE and IIO_VAL_INT and throw
error and not support that sensor if the following conditions happen:
1) min/max range is using H32 bits
2) resolution exponent is greater than the scale exponent.


Thanks,
Jyoti


On Sun, Jan 17, 2021 at 3:56 AM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Sat, 16 Jan 2021 23:15:56 -0800
> Jyoti Bhayana <jbhayana@google.com> wrote:
>
> > Hi Jonathan,
> >
> > Can you clarify one thing ? If we go with option 2 which is using
> > IIO_AVAIL_RANGE for min,step,high using IIO_VAL_INT then how will it
> > ensure the possible floating value for step as we are using int type?
> As you've identified we can't go with IIO_VAL_INT if we are doing that
> approach, would need to be IIO_VAL_INT_PLUS_MICRO (possibly NANO)
>
> That is why it comes unstuck if we need to use both val and val2 to
> do 64 bit integers...
>
> For the  min and max we just set the val2 value to 0.
>
> Jonathan
>
>
> >
> > Thanks,
> > Jyoti
> >
> > On Sat, Jan 16, 2021 at 11:33 AM Jonathan Cameron <jic23@kernel.org> wrote:
> > >
> > > On Mon, 11 Jan 2021 13:17:51 -0800
> > > Jyoti Bhayana <jbhayana@google.com> wrote:
> > >
> > > > Hi Jonathan,
> > > >
> > > > I know it is a bit confusing. Let me try to explain it with some
> > > > examples to hopefully clarify some things here.
> > > > SCMI Platform talks to the native/actual sensor, gets the raw values
> > > > from the native sensor and applies the scale and then sends those
> > > > values to the SCMI agent and the SCMI IIO driver.
> > > > Since the sensor readings which SCMI IIO driver gets are integer, to
> > > > convert them to float , we need to apply scale to these sensor values
> > > > which is the unit_exponent(power-of-10 multiplier in two’s-complement
> > > > format) specified in the SCMI specification
> > > >
> > > > Native Sensor -> SCMI platform->SCMI Agent->SCMI IIO Driver
> > > >
> > > > So if Native Sensor gets the sensor value
> > > > 32767 and the scale the SCMI Platform is using is 0.002392.
> > > > SCMI platform does the calculation of 32767 * 0.002392 = 78.378664
> > > > and send the sensor value as 78378664 and the scale as .000001 to the
> > > > SCMI agent and SCMI IIO driver
> > > >
> > > > so for SCMI IIO driver the sensor reading = 78378664 and scale = .000001
> > > > and  the sensor value is sensor_reading * scale = 78378664 *  .000001
> > > > =  78.378664
> > > > and the resolution which the SCMI Platform sends to the SCMI agent is 0.002392.
> > > > In the SCMI IIO driver, scale which is .000001 is applied to the min
> > > > range/max range and the actual sensor values.
> > > > sensor resolution which is  0.002392 is just passed to the userspace
> > > > layer so that they know the Native sensor resolution/scale
> > > > being applied by the SCMI platform.
> > >
> > > That was pretty much where I'd gotten to.
> > > Whilst the reasoning might be different it is equivalent to a sensor
> > > providing info on expected noise by giving a 'valid resolution'.
> > > In that case as well you have a sensor providing a number that looks to have
> > > more precision than it actually does.
> > >
> > > Anyhow, that similarity doesn't really matter here.
> > >
> > > I'll also add that a design that applies scale in two places is inherently
> > > less than ideal.   A cleaner design would have maintained the separation
> > > between scale and raw value all the way up the stack.  That would result
> > > in 0 loss of information and also be a cleaner interface.
> > > Ah well, we live with what we have :)
> > >
> > > >
> > > > Regarding your comments in the previous email, when you mentioned
> > > > "what we actually
> > > > need is non standard ABI for resolution"? Does that mean that it is ok
> > > > to have sensor resolution
> > > > as the IIO attribute shown below?
> > > >
> > > > static IIO_DEVICE_ATTR(sensor_resolution, 0444, scmi_iio_get_sensor_resolution,
> > > >                      NULL, 0);
> > >
> > > We could do something new (see later for why I don't think we need to)
> > > Would need to clearly reflect what it applies to and I'm not sure resolution
> > > is even an unambiguous name given sensor resolution is often described as 8bit
> > > 10bit etc.  E.g. this selection table from Maxim for ADCs.
> > > https://www.maximintegrated.com/en/products/parametric/search.html?fam=prec_adc&tree=master&metaTitle=Precision%20ADCs%20(%20%205Msps)&hide=270
> > > Of course sometimes it's also used for what you want here.
> > >
> > > Hohum.  So we might be still be able to do this with standard ABI but we
> > > are going to need to do some maths in the driver.
> > > So if we were to express it via
> > >
> > > in_accel_raw_avail for example we could use the [low step high] form.
> > >
> > > low and high are straight forward as those are expressed directly from
> > > axis_min_range and axis_max_range which I think are in the same units
> > > as the _raw channel itself.
> > >
> > > For resolution, we have it expressed as [res] x 10^res_exponent
> > > and if we just put that in as the 'step' above it would have the wrong
> > > exponent (as we'd expect to still have to apply your 0.00001 from above
> > > example).
> > >
> > > Hence we express it as [res] x 10^(res_exponent - exponent)
> > >
> > > I'm going to slightly modify your example above because the two exponents
> > > are the same so it's hard to tell if I have them right way around.
> > > Hence let res = 0.00293 = 293 x 10^(-5)  (I just dropped the trailing 2)
> > >
> > > scale = 10^(-6) exponent = -6
> > >
> > > So step = 2392 x 10^(-5 + 6) = 2390
> > > giving us
> > >
> > > [min 2390 max] for _raw_available
> > >
> > > Hence when userspace comes along and wants this in relevant base units (here
> > > m/sec^2) it applies the x10^(-6) mutliplier from _scale we get out expected value
> > > of 0.00239 m/sec^2
> > >
> > > That should work for any case we see but the maths done in the driver will have
> > > to cope with potential negative exponents for step.
> > >
> > > One catch will be the 64 bit potential values for min and max :(
> > >
> > > >
> > > > static struct attribute *scmi_iio_attributes[] = {
> > > >        &iio_dev_attr_sensor_resolution.dev_attr.attr,
> > > >        NULL,
> > > > };
> > > >
> > > > and for the min/max range, I can use the read_avail callback?
> > >
> > > I would have said yes normally but if we are going to cope with
> > > a potential floating point value for step as touched on above we
> > > may have to do it by hand in the driver.  Not ideal but may
> > > be only option :(
> > >
> > > >
> > > > Also, for the min/max range, there were two options discussed in the
> > > > email thread:
> > > > option 1)  Add new IIO val Type IIO_VAL_INT_H32_L32, and modify the
> > > > iio_format_value to format the 64 bit int properly for the userspace
> > > > option 2) Ignore the H32 bits and use the existing IIO_VAL_INT as just
> > > > L32 bits should be sufficient for current sensor values.
> > >
> > > Ignore is a strong way of putting it.  We would definitely want to
> > > shout about it if we do get anything set in H32.
> > >
> > > If we are fairly sure that we aren't going to anything greater than
> > > 32 bits than we are fine.
> > >
> > > It should be possible to work through the worst cases given
> > > limits of say +-20g for accelerometers for example and the relatively
> > > limited exponents (5 bits).  + sensible resolution.
> > >
> > > If it's fairly safe I'd like to go for option 2. as it would ensure we
> > > can do floating point for the step (which is then used to compute the
> > > resolution value for android)
> > >
> > > Thanks
> > >
> > > Jonathan
> > >
> > > >
> > > > Let me know which option you prefer for min/max range. and also please
> > > > confirm if it is ok to have an IIO attribute for resolution like
> > > > mentioned above.
> > > >
> > > >
> > > > Thanks,
> > > > Jyoti
> > > >
> > > > Thank you so much
> > > >
> > > > Jyoti
> > > >
> > > >
> > > >
> > > > On Mon, Jan 11, 2021 at 4:34 AM Jonathan Cameron
> > > > <Jonathan.Cameron@huawei.com> wrote:
> > > > >
> > > > > On Sun, 10 Jan 2021 22:44:44 -0800
> > > > > Jyoti Bhayana <jbhayana@google.com> wrote:
> > > > >
> > > > > > Hi Jonathan,
> > > > > >
> > > > > > In section 4.7.2.5.1 of the specification, the following exponent is
> > > > > > the scale value
> > > > > >
> > > > > > uint32 axis_attributes_high
> > > > > > Bits[15:11] Exponent: The power-of-10 multiplier in two’s-complement
> > > > > > format that is applied to the sensor unit
> > > > > > specified by the SensorType field.
> > > > > >
> > > > > > and the resolution is
> > > > > >
> > > > > > uint32 axis_resolution
> > > > > > Bits[31:27] Exponent: The power-of-10 multiplier in two’s-complement format
> > > > > > that is applied to the Res field. Bits[26:0] Res: The resolution of
> > > > > > the sensor axis.
> > > > > >
> > > > > > From code in scmi_protocol.h
> > > > > > /**
> > > > > >  * scmi_sensor_axis_info  - describes one sensor axes
> > > > > >  * @id: The axes ID.
> > > > > >  * @type: Axes type. Chosen amongst one of @enum scmi_sensor_class.
> > > > > >  * @scale: Power-of-10 multiplier applied to the axis unit.
> > > > > >  * @name: NULL-terminated string representing axes name as advertised by
> > > > > >  *  SCMI platform.
> > > > > >  * @extended_attrs: Flag to indicate the presence of additional extended
> > > > > >  *    attributes for this axes.
> > > > > >  * @resolution: Extended attribute representing the resolution of the axes.
> > > > > >  * Set to 0 if not reported by this axes.
> > > > > >  * @exponent: Extended attribute representing the power-of-10 multiplier that
> > > > > >  *      is applied to the resolution field. Set to 0 if not reported by
> > > > > >  *      this axes.
> > > > > >  * @attrs: Extended attributes representing minimum and maximum values
> > > > > >  *   measurable by this axes. Set to 0 if not reported by this sensor.
> > > > > >  */
> > > > > >
> > > > > > struct scmi_sensor_axis_info {
> > > > > > unsigned int id;
> > > > > > unsigned int type;
> > > > > > int scale; //This is the scale used for min/max range
> > > > > > char name[SCMI_MAX_STR_SIZE];
> > > > > > bool extended_attrs;
> > > > > > unsigned int resolution;
> > > > > > int exponent; // This is the scale used in resolution
> > > > > > struct scmi_range_attrs attrs;
> > > > > > };
> > > > > >
> > > > > > The scale above  is the Power-of-10 multiplier which is applied to the min range
> > > > > > and the max range value
> > > > > > but the resolution is equal to resolution and multiplied by
> > > > > > Power-of-10 multiplier
> > > > > > of exponent in the above struct.
> > > > > > So as can be seen above the value of the power of 10 multiplier used
> > > > > > for min/max range
> > > > > > can be different than the value of the power of 10 multiplier used for
> > > > > > the resolution.
> > > > > > Hence, if I have to use IIO_AVAIL_RANGE to specify min/max range and as well
> > > > > > as resolution, then I have to use the float format with the scale applied.
> > > > > >
> > > > > > If there is another way which you know of and prefer, please let me know.
> > > > > I'll confess I've gotten a bit lost here.
> > > > >
> > > > > So I think where we are is how to describe the range of the sensor and why we can't
> > > > > use in_accel_x_raw_available to provide the
> > > > >
> > > > > Understood that the resolution can have different scaling.  That is presumably
> > > > > to allow for the case where a device is reporting values at a finer scale than
> > > > > it's real resolution.  Resolution might take into account expected noise for
> > > > > example.  So it should be decoupled from the scaling of both the actual measurements
> > > > > and the axis high / low limits.
> > > > >
> > > > > However, I'd read that as saying the axis high / low limits and the actual sensor
> > > > > readings should be scaled by the exponent in axis_attributes_high.
> > > > > So I think we are fine for the range, but my earlier assumption that resolution
> > > > > was equivalent to scale in IIO (real world value for 1LSB) may be completely wrong
> > > > > as resolution may be unconnected to how you convert to a real world value?
> > > > >
> > > > > If nothing else I'd like to suggest the spec needs to be tightened a bit here
> > > > > to say exactly how we convert a value coming in to real world units (maybe
> > > > > I'm just missing it).
> > > > >
> > > > > Anyhow, I suspect we've been looking at this the wrong way and what we actually
> > > > > need is non standard ABI for resolution.
> > > > >
> > > > > Jonathan
> > > > >
> > > > >
> > > > >
> > > > >
> > > > > >
> > > > > > Thanks,
> > > > > > Jyoti
> > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > > Thanks,
> > > > > > Jyoti
> > > > > >
> > > > > > On Sat, Jan 9, 2021 at 11:01 AM Jonathan Cameron <jic23@kernel.org> wrote:
> > > > > > >
> > > > > > > On Wed,  6 Jan 2021 21:23:53 +0000
> > > > > > > Jyoti Bhayana <jbhayana@google.com> wrote:
> > > > > > >
> > > > > > > > Hi Jonathan,
> > > > > > > >
> > > > > > > > Instead of adding IIO_VAL_INT_H32_L32, I am thinking of adding IIO_VAL_FRACTIONAL_LONG
> > > > > > > > or IIO_VAL_FRACTIONAL_64 as the scale/exponent used for min/max range can be different
> > > > > > > > than the one used in resolution according to specification.
> > > > > > >
> > > > > > > That's somewhat 'odd'.  Given min/max are inherently values the sensor is supposed to
> > > > > > > be able to return why give them different resolutions?  Can you point me at a specific
> > > > > > > section of the spec?  The axis_min_range_low etc fields don't seem to have units specified
> > > > > > > but I assumed they were in sensor units and so same scale factors?
> > > > > > >
> > > > > > > >
> > > > > > > > I am planning to use read_avail for IIO_CHAN_INFO_PROCESSED using IIO_AVAIL_RANGE
> > > > > > > > and this new IIO_VAL_FRACTIONAL_64 for min range,max range and resolution.
> > > > > > > > Instead of two values used in IIO_VAL_FRACTIONAL, IIO_VAL_FRACTIONAL_64 will use 4 values
> > > > > > > > val_high,val_low,and val2_high and val2_low.
> > > > > > >
> > > > > > > I'm not keen on the changing that internal kernel interface unless we absolutely
> > > > > > > have to.  read_avail() is called from consumer drivers and they won't know anything
> > > > > > > about this new variant.
> > > > > > >
> > > > > > > >
> > > > > > > > Let me know if that is an acceptable solution.
> > > > > > >
> > > > > > > Hmm. It isn't a standard use of the ABI given the value in the buffer is (I assume)
> > > > > > > raw (needs scale applied).  However, it isn't excluded by the ABI docs.  Whether
> > > > > > > a standard userspace is going to expect it is not clear to me.
> > > > > > >
> > > > > > > I don't want to end up in a position where we end up with available being generally
> > > > > > > added for processed when what most people care about is what the value range they
> > > > > > > might get from a polled read is (rather than via a buffer).
> > > > > > >
> > > > > > > So I'm not that keen on this solution but if we can find a way to avoid it.
> > > > > > >
> > > > > > > Jonathan
> > > > > > >
> > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > > Thanks,
> > > > > > > > Jyoti
> > > > > > > >
> > > > > > >
> > > > >
> > >
>

Re: Reply to [RFC PATCH v2 0/1] Adding support for IIO SCMI based sensors

[Jonathan Cameron]

On Sun, 17 Jan 2021 13:02:31 -0800
Jyoti Bhayana <jbhayana@google.com> wrote:

> Hi Jonathan,
> 
> Instead of IIO_VAL_INT_PLUS_MICRO, we can also use IIO_VAL_FRACTIONAL
> right for the raw values for min/max range and resolution? But, to

Sure on IIO_VAL_FRACTIONAL, that should be fine.

> keep things simple though, it will be nice if the IIO driver uses
> IIO_VAL_INT and throws an error if there is a negative exponent in the
> following  calculation for the resolution which means the res_exponent
> is always the same or greater than the scale_exponent.

If that tends to be true, then indeed that should work fine.

> 
>  [res] x 10^(res_exponent - exponent)
> 
> so the IIO driver will use IIO_AVAIL_RANGE and IIO_VAL_INT and throw
> error and not support that sensor if the following conditions happen:
> 1) min/max range is using H32 bits
> 2) resolution exponent is greater than the scale exponent.

Sounds good though I've not thought through whether we can in general
expect 2 to be true.  Feels like it should be though!

Jonathan
> 
> 
> Thanks,
> Jyoti
> 
> 
> On Sun, Jan 17, 2021 at 3:56 AM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Sat, 16 Jan 2021 23:15:56 -0800
> > Jyoti Bhayana <jbhayana@google.com> wrote:
> >  
> > > Hi Jonathan,
> > >
> > > Can you clarify one thing ? If we go with option 2 which is using
> > > IIO_AVAIL_RANGE for min,step,high using IIO_VAL_INT then how will it
> > > ensure the possible floating value for step as we are using int type?  
> > As you've identified we can't go with IIO_VAL_INT if we are doing that
> > approach, would need to be IIO_VAL_INT_PLUS_MICRO (possibly NANO)
> >
> > That is why it comes unstuck if we need to use both val and val2 to
> > do 64 bit integers...
> >
> > For the  min and max we just set the val2 value to 0.
> >
> > Jonathan
> >
> >  
> > >
> > > Thanks,
> > > Jyoti
> > >
> > > On Sat, Jan 16, 2021 at 11:33 AM Jonathan Cameron <jic23@kernel.org> wrote:  
> > > >
> > > > On Mon, 11 Jan 2021 13:17:51 -0800
> > > > Jyoti Bhayana <jbhayana@google.com> wrote:
> > > >  
> > > > > Hi Jonathan,
> > > > >
> > > > > I know it is a bit confusing. Let me try to explain it with some
> > > > > examples to hopefully clarify some things here.
> > > > > SCMI Platform talks to the native/actual sensor, gets the raw values
> > > > > from the native sensor and applies the scale and then sends those
> > > > > values to the SCMI agent and the SCMI IIO driver.
> > > > > Since the sensor readings which SCMI IIO driver gets are integer, to
> > > > > convert them to float , we need to apply scale to these sensor values
> > > > > which is the unit_exponent(power-of-10 multiplier in two’s-complement
> > > > > format) specified in the SCMI specification
> > > > >
> > > > > Native Sensor -> SCMI platform->SCMI Agent->SCMI IIO Driver
> > > > >
> > > > > So if Native Sensor gets the sensor value
> > > > > 32767 and the scale the SCMI Platform is using is 0.002392.
> > > > > SCMI platform does the calculation of 32767 * 0.002392 = 78.378664
> > > > > and send the sensor value as 78378664 and the scale as .000001 to the
> > > > > SCMI agent and SCMI IIO driver
> > > > >
> > > > > so for SCMI IIO driver the sensor reading = 78378664 and scale = .000001
> > > > > and  the sensor value is sensor_reading * scale = 78378664 *  .000001
> > > > > =  78.378664
> > > > > and the resolution which the SCMI Platform sends to the SCMI agent is 0.002392.
> > > > > In the SCMI IIO driver, scale which is .000001 is applied to the min
> > > > > range/max range and the actual sensor values.
> > > > > sensor resolution which is  0.002392 is just passed to the userspace
> > > > > layer so that they know the Native sensor resolution/scale
> > > > > being applied by the SCMI platform.  
> > > >
> > > > That was pretty much where I'd gotten to.
> > > > Whilst the reasoning might be different it is equivalent to a sensor
> > > > providing info on expected noise by giving a 'valid resolution'.
> > > > In that case as well you have a sensor providing a number that looks to have
> > > > more precision than it actually does.
> > > >
> > > > Anyhow, that similarity doesn't really matter here.
> > > >
> > > > I'll also add that a design that applies scale in two places is inherently
> > > > less than ideal.   A cleaner design would have maintained the separation
> > > > between scale and raw value all the way up the stack.  That would result
> > > > in 0 loss of information and also be a cleaner interface.
> > > > Ah well, we live with what we have :)
> > > >  
> > > > >
> > > > > Regarding your comments in the previous email, when you mentioned
> > > > > "what we actually
> > > > > need is non standard ABI for resolution"? Does that mean that it is ok
> > > > > to have sensor resolution
> > > > > as the IIO attribute shown below?
> > > > >
> > > > > static IIO_DEVICE_ATTR(sensor_resolution, 0444, scmi_iio_get_sensor_resolution,
> > > > >                      NULL, 0);  
> > > >
> > > > We could do something new (see later for why I don't think we need to)
> > > > Would need to clearly reflect what it applies to and I'm not sure resolution
> > > > is even an unambiguous name given sensor resolution is often described as 8bit
> > > > 10bit etc.  E.g. this selection table from Maxim for ADCs.
> > > > https://www.maximintegrated.com/en/products/parametric/search.html?fam=prec_adc&tree=master&metaTitle=Precision%20ADCs%20(%20%205Msps)&hide=270
> > > > Of course sometimes it's also used for what you want here.
> > > >
> > > > Hohum.  So we might be still be able to do this with standard ABI but we
> > > > are going to need to do some maths in the driver.
> > > > So if we were to express it via
> > > >
> > > > in_accel_raw_avail for example we could use the [low step high] form.
> > > >
> > > > low and high are straight forward as those are expressed directly from
> > > > axis_min_range and axis_max_range which I think are in the same units
> > > > as the _raw channel itself.
> > > >
> > > > For resolution, we have it expressed as [res] x 10^res_exponent
> > > > and if we just put that in as the 'step' above it would have the wrong
> > > > exponent (as we'd expect to still have to apply your 0.00001 from above
> > > > example).
> > > >
> > > > Hence we express it as [res] x 10^(res_exponent - exponent)
> > > >
> > > > I'm going to slightly modify your example above because the two exponents
> > > > are the same so it's hard to tell if I have them right way around.
> > > > Hence let res = 0.00293 = 293 x 10^(-5)  (I just dropped the trailing 2)
> > > >
> > > > scale = 10^(-6) exponent = -6
> > > >
> > > > So step = 2392 x 10^(-5 + 6) = 2390
> > > > giving us
> > > >
> > > > [min 2390 max] for _raw_available
> > > >
> > > > Hence when userspace comes along and wants this in relevant base units (here
> > > > m/sec^2) it applies the x10^(-6) mutliplier from _scale we get out expected value
> > > > of 0.00239 m/sec^2
> > > >
> > > > That should work for any case we see but the maths done in the driver will have
> > > > to cope with potential negative exponents for step.
> > > >
> > > > One catch will be the 64 bit potential values for min and max :(
> > > >  
> > > > >
> > > > > static struct attribute *scmi_iio_attributes[] = {
> > > > >        &iio_dev_attr_sensor_resolution.dev_attr.attr,
> > > > >        NULL,
> > > > > };
> > > > >
> > > > > and for the min/max range, I can use the read_avail callback?  
> > > >
> > > > I would have said yes normally but if we are going to cope with
> > > > a potential floating point value for step as touched on above we
> > > > may have to do it by hand in the driver.  Not ideal but may
> > > > be only option :(
> > > >  
> > > > >
> > > > > Also, for the min/max range, there were two options discussed in the
> > > > > email thread:
> > > > > option 1)  Add new IIO val Type IIO_VAL_INT_H32_L32, and modify the
> > > > > iio_format_value to format the 64 bit int properly for the userspace
> > > > > option 2) Ignore the H32 bits and use the existing IIO_VAL_INT as just
> > > > > L32 bits should be sufficient for current sensor values.  
> > > >
> > > > Ignore is a strong way of putting it.  We would definitely want to
> > > > shout about it if we do get anything set in H32.
> > > >
> > > > If we are fairly sure that we aren't going to anything greater than
> > > > 32 bits than we are fine.
> > > >
> > > > It should be possible to work through the worst cases given
> > > > limits of say +-20g for accelerometers for example and the relatively
> > > > limited exponents (5 bits).  + sensible resolution.
> > > >
> > > > If it's fairly safe I'd like to go for option 2. as it would ensure we
> > > > can do floating point for the step (which is then used to compute the
> > > > resolution value for android)
> > > >
> > > > Thanks
> > > >
> > > > Jonathan
> > > >  
> > > > >
> > > > > Let me know which option you prefer for min/max range. and also please
> > > > > confirm if it is ok to have an IIO attribute for resolution like
> > > > > mentioned above.
> > > > >
> > > > >
> > > > > Thanks,
> > > > > Jyoti
> > > > >
> > > > > Thank you so much
> > > > >
> > > > > Jyoti
> > > > >
> > > > >
> > > > >
> > > > > On Mon, Jan 11, 2021 at 4:34 AM Jonathan Cameron
> > > > > <Jonathan.Cameron@huawei.com> wrote:  
> > > > > >
> > > > > > On Sun, 10 Jan 2021 22:44:44 -0800
> > > > > > Jyoti Bhayana <jbhayana@google.com> wrote:
> > > > > >  
> > > > > > > Hi Jonathan,
> > > > > > >
> > > > > > > In section 4.7.2.5.1 of the specification, the following exponent is
> > > > > > > the scale value
> > > > > > >
> > > > > > > uint32 axis_attributes_high
> > > > > > > Bits[15:11] Exponent: The power-of-10 multiplier in two’s-complement
> > > > > > > format that is applied to the sensor unit
> > > > > > > specified by the SensorType field.
> > > > > > >
> > > > > > > and the resolution is
> > > > > > >
> > > > > > > uint32 axis_resolution
> > > > > > > Bits[31:27] Exponent: The power-of-10 multiplier in two’s-complement format
> > > > > > > that is applied to the Res field. Bits[26:0] Res: The resolution of
> > > > > > > the sensor axis.
> > > > > > >
> > > > > > > From code in scmi_protocol.h
> > > > > > > /**
> > > > > > >  * scmi_sensor_axis_info  - describes one sensor axes
> > > > > > >  * @id: The axes ID.
> > > > > > >  * @type: Axes type. Chosen amongst one of @enum scmi_sensor_class.
> > > > > > >  * @scale: Power-of-10 multiplier applied to the axis unit.
> > > > > > >  * @name: NULL-terminated string representing axes name as advertised by
> > > > > > >  *  SCMI platform.
> > > > > > >  * @extended_attrs: Flag to indicate the presence of additional extended
> > > > > > >  *    attributes for this axes.
> > > > > > >  * @resolution: Extended attribute representing the resolution of the axes.
> > > > > > >  * Set to 0 if not reported by this axes.
> > > > > > >  * @exponent: Extended attribute representing the power-of-10 multiplier that
> > > > > > >  *      is applied to the resolution field. Set to 0 if not reported by
> > > > > > >  *      this axes.
> > > > > > >  * @attrs: Extended attributes representing minimum and maximum values
> > > > > > >  *   measurable by this axes. Set to 0 if not reported by this sensor.
> > > > > > >  */
> > > > > > >
> > > > > > > struct scmi_sensor_axis_info {
> > > > > > > unsigned int id;
> > > > > > > unsigned int type;
> > > > > > > int scale; //This is the scale used for min/max range
> > > > > > > char name[SCMI_MAX_STR_SIZE];
> > > > > > > bool extended_attrs;
> > > > > > > unsigned int resolution;
> > > > > > > int exponent; // This is the scale used in resolution
> > > > > > > struct scmi_range_attrs attrs;
> > > > > > > };
> > > > > > >
> > > > > > > The scale above  is the Power-of-10 multiplier which is applied to the min range
> > > > > > > and the max range value
> > > > > > > but the resolution is equal to resolution and multiplied by
> > > > > > > Power-of-10 multiplier
> > > > > > > of exponent in the above struct.
> > > > > > > So as can be seen above the value of the power of 10 multiplier used
> > > > > > > for min/max range
> > > > > > > can be different than the value of the power of 10 multiplier used for
> > > > > > > the resolution.
> > > > > > > Hence, if I have to use IIO_AVAIL_RANGE to specify min/max range and as well
> > > > > > > as resolution, then I have to use the float format with the scale applied.
> > > > > > >
> > > > > > > If there is another way which you know of and prefer, please let me know.  
> > > > > > I'll confess I've gotten a bit lost here.
> > > > > >
> > > > > > So I think where we are is how to describe the range of the sensor and why we can't
> > > > > > use in_accel_x_raw_available to provide the
> > > > > >
> > > > > > Understood that the resolution can have different scaling.  That is presumably
> > > > > > to allow for the case where a device is reporting values at a finer scale than
> > > > > > it's real resolution.  Resolution might take into account expected noise for
> > > > > > example.  So it should be decoupled from the scaling of both the actual measurements
> > > > > > and the axis high / low limits.
> > > > > >
> > > > > > However, I'd read that as saying the axis high / low limits and the actual sensor
> > > > > > readings should be scaled by the exponent in axis_attributes_high.
> > > > > > So I think we are fine for the range, but my earlier assumption that resolution
> > > > > > was equivalent to scale in IIO (real world value for 1LSB) may be completely wrong
> > > > > > as resolution may be unconnected to how you convert to a real world value?
> > > > > >
> > > > > > If nothing else I'd like to suggest the spec needs to be tightened a bit here
> > > > > > to say exactly how we convert a value coming in to real world units (maybe
> > > > > > I'm just missing it).
> > > > > >
> > > > > > Anyhow, I suspect we've been looking at this the wrong way and what we actually
> > > > > > need is non standard ABI for resolution.
> > > > > >
> > > > > > Jonathan
> > > > > >
> > > > > >
> > > > > >
> > > > > >  
> > > > > > >
> > > > > > > Thanks,
> > > > > > > Jyoti
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > > Thanks,
> > > > > > > Jyoti
> > > > > > >
> > > > > > > On Sat, Jan 9, 2021 at 11:01 AM Jonathan Cameron <jic23@kernel.org> wrote:  
> > > > > > > >
> > > > > > > > On Wed,  6 Jan 2021 21:23:53 +0000
> > > > > > > > Jyoti Bhayana <jbhayana@google.com> wrote:
> > > > > > > >  
> > > > > > > > > Hi Jonathan,
> > > > > > > > >
> > > > > > > > > Instead of adding IIO_VAL_INT_H32_L32, I am thinking of adding IIO_VAL_FRACTIONAL_LONG
> > > > > > > > > or IIO_VAL_FRACTIONAL_64 as the scale/exponent used for min/max range can be different
> > > > > > > > > than the one used in resolution according to specification.  
> > > > > > > >
> > > > > > > > That's somewhat 'odd'.  Given min/max are inherently values the sensor is supposed to
> > > > > > > > be able to return why give them different resolutions?  Can you point me at a specific
> > > > > > > > section of the spec?  The axis_min_range_low etc fields don't seem to have units specified
> > > > > > > > but I assumed they were in sensor units and so same scale factors?
> > > > > > > >  
> > > > > > > > >
> > > > > > > > > I am planning to use read_avail for IIO_CHAN_INFO_PROCESSED using IIO_AVAIL_RANGE
> > > > > > > > > and this new IIO_VAL_FRACTIONAL_64 for min range,max range and resolution.
> > > > > > > > > Instead of two values used in IIO_VAL_FRACTIONAL, IIO_VAL_FRACTIONAL_64 will use 4 values
> > > > > > > > > val_high,val_low,and val2_high and val2_low.  
> > > > > > > >
> > > > > > > > I'm not keen on the changing that internal kernel interface unless we absolutely
> > > > > > > > have to.  read_avail() is called from consumer drivers and they won't know anything
> > > > > > > > about this new variant.
> > > > > > > >  
> > > > > > > > >
> > > > > > > > > Let me know if that is an acceptable solution.  
> > > > > > > >
> > > > > > > > Hmm. It isn't a standard use of the ABI given the value in the buffer is (I assume)
> > > > > > > > raw (needs scale applied).  However, it isn't excluded by the ABI docs.  Whether
> > > > > > > > a standard userspace is going to expect it is not clear to me.
> > > > > > > >
> > > > > > > > I don't want to end up in a position where we end up with available being generally
> > > > > > > > added for processed when what most people care about is what the value range they
> > > > > > > > might get from a polled read is (rather than via a buffer).
> > > > > > > >
> > > > > > > > So I'm not that keen on this solution but if we can find a way to avoid it.
> > > > > > > >
> > > > > > > > Jonathan
> > > > > > > >
> > > > > > > >  
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > Thanks,
> > > > > > > > > Jyoti
> > > > > > > > >  
> > > > > > > >  
> > > > > >  
> > > >  
> >