Hi,
This series adds support for ARM SCMI Protocol based IIO Device.
This driver provides support for Accelerometer and Gyroscope sensor using
SCMI Sensor Protocol extensions added in the SCMIv3.0 ARM specification,
which is available at
https://developer.arm.com/documentation/den0056/c/
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
v5 --> v6
- Fixed the warning by kernel test robot
- Incorporated the feedback comments from v5
- Fixed the bug found in scmi_iio_set_odr_val
for calculating the multiplier
v4 --> v5
- Dropped the RFC tag
- Added channel ext_info for raw_available
- Incorporated the feedback comments from v4 review of the patch
v3 --> v4
- Incorporated the feedback comments from v3 review of the patch
v2 --> v3
- Incorporated the feedback comments from v2 review of the patch
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/firmware/arm_scmi/driver.c | 2 +-
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 | 678 +++++++++++++++++++++
7 files changed, 710 insertions(+), 1 deletion(-)
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.30.0.478.g8a0d178c01-goog
This change provides ARM SCMI Protocol based IIO device.
This driver provides support for Accelerometer and Gyroscope using
SCMI Sensor Protocol extensions added in the SCMIv3.0 ARM specification
Reported-by: kernel test robot <[email protected]>
Signed-off-by: Jyoti Bhayana <[email protected]>
---
MAINTAINERS | 6 +
drivers/firmware/arm_scmi/driver.c | 2 +-
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 | 678 +++++++++++++++++++++
7 files changed, 710 insertions(+), 1 deletion(-)
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 <[email protected]>
+L: [email protected]
+S: Maintained
+F: drivers/iio/common/scmi_sensors/scmi_iio.c
+
IIO SUBSYSTEM AND DRIVERS
M: Jonathan Cameron <[email protected]>
R: Lars-Peter Clausen <[email protected]>
diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
index 5392e1fc6b4e..248313bbd473 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -741,7 +741,7 @@ static struct scmi_prot_devnames devnames[] = {
{ SCMI_PROTOCOL_SYSTEM, { "syspower" },},
{ SCMI_PROTOCOL_PERF, { "cpufreq" },},
{ SCMI_PROTOCOL_CLOCK, { "clocks" },},
- { SCMI_PROTOCOL_SENSOR, { "hwmon" },},
+ { SCMI_PROTOCOL_SENSOR, { "hwmon", "iiodev" },},
{ SCMI_PROTOCOL_RESET, { "reset" },},
{ SCMI_PROTOCOL_VOLTAGE, { "regulator" },},
};
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..31977c3bc600
--- /dev/null
+++ b/drivers/iio/common/scmi_sensors/scmi_iio.c
@@ -0,0 +1,678 @@
+// SPDX-License-Identifier: GPL-2.0
+
+/*
+ * System Control and Management Interface(SCMI) based IIO sensor driver
+ *
+ * Copyright (C) 2021 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 SCMI_IIO_NUM_OF_AXIS 3
+
+struct scmi_iio_priv {
+ struct scmi_handle *handle;
+ const struct scmi_sensor_info *sensor_info;
+ struct iio_dev *indio_dev;
+ /* adding one additional channel for timestamp */
+ long long iio_buf[SCMI_IIO_NUM_OF_AXIS + 1];
+ struct notifier_block sensor_update_nb;
+ u32 *freq_avail;
+};
+
+static int scmi_iio_sensor_update_cb(struct notifier_block *nb,
+ unsigned long event, void *data)
+{
+ struct scmi_sensor_update_report *sensor_update = data;
+ struct iio_dev *scmi_iio_dev;
+ struct scmi_iio_priv *sensor;
+ s8 tstamp_scale;
+ u64 time, time_ns;
+ int i;
+
+ if (sensor_update->readings_count == 0)
+ return NOTIFY_DONE;
+
+ sensor = container_of(nb, struct scmi_iio_priv, sensor_update_nb);
+
+ for (i = 0; i < sensor_update->readings_count; i++)
+ sensor->iio_buf[i] = sensor_update->readings[i].value;
+
+ if (!sensor->sensor_info->timestamped) {
+ time_ns = ktime_to_ns(sensor_update->timestamp);
+ } else {
+ /*
+ * All the axes are supposed to have the same value for timestamp.
+ * We are just using the values from the Axis 0 here.
+ */
+ time = sensor_update->readings[0].timestamp;
+
+ /*
+ * Timestamp returned by SCMI is in seconds and is equal to
+ * time * power-of-10 multiplier(tstamp_scale) seconds.
+ * Converting the timestamp to nanoseconds below.
+ */
+ tstamp_scale = sensor->sensor_info->tstamp_scale +
+ const_ilog2(NSEC_PER_SEC) / const_ilog2(10);
+ if (tstamp_scale < 0)
+ time_ns =
+ div64_u64(time, int_pow(10, abs(tstamp_scale)));
+ else
+ time_ns = time * int_pow(10, tstamp_scale);
+ }
+
+ scmi_iio_dev = sensor->indio_dev;
+ 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 *iio_dev)
+{
+ struct scmi_iio_priv *sensor = iio_priv(iio_dev);
+ u32 sensor_id = sensor->sensor_info->id;
+ u32 sensor_config = 0;
+ 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) {
+ dev_err(&iio_dev->dev,
+ "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) {
+ sensor->handle->notify_ops->unregister_event_notifier(sensor->handle,
+ SCMI_PROTOCOL_SENSOR,
+ SCMI_EVENT_SENSOR_UPDATE, &sensor_id,
+ &sensor->sensor_update_nb);
+ dev_err(&iio_dev->dev, "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) {
+ dev_err(&iio_dev->dev,
+ "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) {
+ dev_err(&iio_dev->dev,
+ "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_set_odr_val(struct iio_dev *iio_dev, int val, int val2)
+{
+ struct scmi_iio_priv *sensor = iio_priv(iio_dev);
+ const unsigned long UHZ_PER_HZ = 1000000UL;
+ u64 sec, mult, uHz;
+ u32 sensor_config;
+ char buf[32];
+
+ int err = sensor->handle->sensor_ops->config_get(sensor->handle,
+ sensor->sensor_info->id, &sensor_config);
+ if (err) {
+ dev_err(&iio_dev->dev,
+ "Error in getting sensor config for sensor %s err %d",
+ sensor->sensor_info->name, err);
+ return err;
+ }
+
+ uHz = val * UHZ_PER_HZ + 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, sizeof(buf), "%llu", ((u64)uHz * 0xFFFF) / UHZ_PER_HZ) - 1;
+
+ sec = div64_u64(int_pow(10, mult) * UHZ_PER_HZ, uHz);
+ if (sec == 0) {
+ dev_err(&iio_dev->dev,
+ "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)
+ dev_err(&iio_dev->dev,
+ "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;
+
+ switch (mask) {
+ case IIO_CHAN_INFO_SAMP_FREQ:
+ mutex_lock(&iio_dev->mlock);
+ err = scmi_iio_set_odr_val(iio_dev, val, val2);
+ mutex_unlock(&iio_dev->mlock);
+ return err;
+ default:
+ return -EINVAL;
+ }
+}
+
+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 void convert_ns_to_freq(u64 interval_ns, u64 *hz, u64 *uhz)
+{
+ u64 rem;
+
+ *hz = div64_u64_rem(NSEC_PER_SEC, interval_ns, &rem);
+ *uhz = (rem * 1000000UL) / interval_ns;
+}
+
+static int scmi_iio_get_odr_val(struct iio_dev *iio_dev, int *val, int *val2)
+{
+ u64 sensor_update_interval, sensor_interval_mult, hz, uhz;
+ struct scmi_iio_priv *sensor = iio_priv(iio_dev);
+ u32 sensor_config;
+ int mult;
+
+ int err = sensor->handle->sensor_ops->config_get(sensor->handle,
+ sensor->sensor_info->id, &sensor_config);
+ if (err) {
+ dev_err(&iio_dev->dev,
+ "Error in getting sensor config for sensor %s err %d",
+ 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, &hz, &uhz);
+ *val = hz;
+ *val2 = 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) {
+ *val = 1;
+ *val2 = int_pow(10, abs(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;
+ }
+}
+
+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,
+};
+
+static ssize_t scmi_iio_get_raw_available(struct iio_dev *iio_dev,
+ uintptr_t private,
+ const struct iio_chan_spec *chan,
+ char *buf)
+{
+ struct scmi_iio_priv *sensor = iio_priv(iio_dev);
+ unsigned long long resolution, rem;
+ long long min_range, max_range;
+ s8 exponent, scale;
+ int len = 0;
+
+ /*
+ * All the axes are supposed to have the same value for range and resolution.
+ * We are just using the values from the Axis 0 here.
+ */
+ if (sensor->sensor_info->axis[0].extended_attrs) {
+ min_range = sensor->sensor_info->axis[0].attrs.min_range;
+ max_range = sensor->sensor_info->axis[0].attrs.max_range;
+ resolution = sensor->sensor_info->axis[0].resolution;
+ exponent = sensor->sensor_info->axis[0].exponent;
+ scale = sensor->sensor_info->axis[0].scale;
+
+ /*
+ * To provide the raw value for the resolution to the userspace,
+ * need to divide the resolution exponent by the sensor scale
+ */
+ exponent = exponent - scale;
+ if (exponent < 0) {
+ resolution = div64_u64_rem(resolution,
+ int_pow(10, abs(exponent)),
+ &rem);
+ len = scnprintf(buf, PAGE_SIZE,
+ "[%lld %llu.%llu %lld]\n", min_range,
+ resolution, rem, max_range);
+ } else {
+ resolution = resolution * int_pow(10, exponent);
+ len = scnprintf(buf, PAGE_SIZE, "[%lld %llu %lld]\n",
+ min_range, resolution, max_range);
+ }
+ }
+ return len;
+}
+
+static const struct iio_chan_spec_ext_info scmi_iio_ext_info[] = {
+ {
+ .name = "raw_available",
+ .read = scmi_iio_get_raw_available,
+ .shared = IIO_SHARED_BY_TYPE,
+ },
+ {},
+};
+
+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 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;
+ iio_chan->ext_info = scmi_iio_ext_info;
+}
+
+static int scmi_iio_get_chan_modifier(const char *name,
+ enum iio_modifier *modifier)
+{
+ char *pch, mod;
+
+ if (!name)
+ return -EINVAL;
+
+ pch = strrchr(name, '_');
+ if (!pch)
+ return -EINVAL;
+
+ mod = *(pch + 1);
+ switch (mod) {
+ case 'X':
+ *modifier = IIO_MOD_X;
+ return 0;
+ case 'Y':
+ *modifier = IIO_MOD_Y;
+ return 0;
+ case 'Z':
+ *modifier = IIO_MOD_Z;
+ return 0;
+ default:
+ return -EINVAL;
+ }
+}
+
+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 u64 scmi_iio_convert_interval_to_ns(u32 val)
+{
+ u64 sensor_update_interval =
+ SCMI_SENS_INTVL_GET_SECS(val) * NSEC_PER_SEC;
+ u64 sensor_interval_mult;
+ int mult;
+
+ 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 int scmi_iio_set_sampling_freq_avail(struct iio_dev *iio_dev)
+{
+ u64 cur_interval_ns, low_interval_ns, high_interval_ns, step_size_ns,
+ hz, uhz;
+ unsigned int cur_interval, low_interval, high_interval, step_size;
+ struct scmi_iio_priv *sensor = iio_priv(iio_dev);
+ int i;
+
+ sensor->freq_avail =
+ devm_kzalloc(&iio_dev->dev,
+ sizeof(*sensor->freq_avail) *
+ (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 = scmi_iio_convert_interval_to_ns(low_interval);
+ convert_ns_to_freq(low_interval_ns, &hz, &uhz);
+ sensor->freq_avail[0] = hz;
+ sensor->freq_avail[1] = uhz;
+
+ step_size = sensor->sensor_info->intervals
+ .desc[SCMI_SENS_INTVL_SEGMENT_STEP];
+ step_size_ns = scmi_iio_convert_interval_to_ns(step_size);
+ convert_ns_to_freq(step_size_ns, &hz, &uhz);
+ sensor->freq_avail[2] = hz;
+ sensor->freq_avail[3] = uhz;
+
+ high_interval = sensor->sensor_info->intervals
+ .desc[SCMI_SENS_INTVL_SEGMENT_HIGH];
+ high_interval_ns =
+ scmi_iio_convert_interval_to_ns(high_interval);
+ convert_ns_to_freq(high_interval_ns, &hz, &uhz);
+ sensor->freq_avail[4] = hz;
+ sensor->freq_avail[5] = uhz;
+ } else {
+ for (i = 0; i < sensor->sensor_info->intervals.count; i++) {
+ cur_interval = sensor->sensor_info->intervals.desc[i];
+ cur_interval_ns =
+ scmi_iio_convert_interval_to_ns(cur_interval);
+ convert_ns_to_freq(cur_interval_ns, &hz, &uhz);
+ sensor->freq_avail[i * 2] = hz;
+ sensor->freq_avail[i * 2 + 1] = uhz;
+ }
+ }
+ return 0;
+}
+
+static int scmi_iio_buffers_setup(struct iio_dev *scmi_iiodev)
+{
+ struct iio_buffer *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 struct iio_dev *scmi_alloc_iiodev(struct device *dev,
+ struct scmi_handle *handle,
+ const struct scmi_sensor_info *sensor_info)
+{
+ struct iio_chan_spec *iio_channels;
+ struct scmi_iio_priv *sensor;
+ enum iio_modifier modifier;
+ enum iio_chan_type type;
+ struct iio_dev *iiodev;
+ int i, ret;
+
+ iiodev = devm_iio_device_alloc(dev, sizeof(*sensor));
+ if (!iiodev)
+ return ERR_PTR(-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 = scmi_iio_sensor_update_cb;
+ sensor->indio_dev = iiodev;
+
+ /* adding one additional channel for timestamp */
+ 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 ERR_PTR(-ENOMEM);
+
+ ret = scmi_iio_set_sampling_freq_avail(iiodev);
+ if (ret < 0)
+ return ERR_PTR(ret);
+
+ 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 ERR_PTR(ret);
+
+ ret = scmi_iio_get_chan_modifier(sensor_info->axis[i].name,
+ &modifier);
+ if (ret < 0)
+ return ERR_PTR(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;
+ return iiodev;
+}
+
+static int scmi_iio_dev_probe(struct scmi_device *sdev)
+{
+ const struct scmi_sensor_info *sensor_info;
+ struct scmi_handle *handle = sdev->handle;
+ struct device *dev = &sdev->dev;
+ struct iio_dev *scmi_iio_dev;
+ u16 nr_sensors;
+ int err = -ENODEV, i;
+
+ if (!handle || !handle->sensor_ops) {
+ 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 -ENODEV;
+ }
+
+ 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;
+ }
+
+ /* This driver only supports 3-axis accel and gyro, skipping other sensors */
+ if (sensor_info->num_axis != SCMI_IIO_NUM_OF_AXIS)
+ continue;
+
+ /* This driver only supports 3-axis accel and gyro, skipping other sensors */
+ if (sensor_info->axis[0].type != METERS_SEC_SQUARED &&
+ sensor_info->axis[0].type != RADIANS_SEC)
+ continue;
+
+ scmi_iio_dev = scmi_alloc_iiodev(dev, handle, sensor_info);
+ if (IS_ERR(scmi_iio_dev)) {
+ dev_err(dev,
+ "failed to allocate IIO device for sensor %s: %ld\n",
+ sensor_info->name, PTR_ERR(scmi_iio_dev));
+ return PTR_ERR(scmi_iio_dev);
+ }
+
+ 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 <[email protected]>");
+MODULE_DESCRIPTION("SCMI IIO Driver");
+MODULE_LICENSE("GPL v2");
--
2.30.0.478.g8a0d178c01-goog
On Fri, 12 Feb 2021 17:22:35 +0000
Jyoti Bhayana <[email protected]> wrote:
> This change provides ARM SCMI Protocol based IIO device.
> This driver provides support for Accelerometer and Gyroscope using
> SCMI Sensor Protocol extensions added in the SCMIv3.0 ARM specification
Hi Joyti
A few things inline but nothing to require a v7.
1) Use of long long to get s64 - I can tidy that up whilst applying.
2) Going to have a clash with Alex's multi buffer rework of the core
I'll sort that out when I apply this as well.
>
> Reported-by: kernel test robot <[email protected]>
For fixes within a larger patch as a result of the various build robots,
we should either only mention them in comments, or add a note after the
Reported-by to say what was fixed. E.g something like
Reported-by: kernel test robot <[email protected]> # off by 1 error
I can't remember what was actually reported for this one.
If you can reply with such a comment I'll add it on, if not I'll drop
the Reported-by as uninformative. Right now it looks like the whole
patch is a fix for an issue that 0-day reported :)
> Signed-off-by: Jyoti Bhayana <[email protected]>
> ---
> MAINTAINERS | 6 +
> drivers/firmware/arm_scmi/driver.c | 2 +-
> 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 | 678 +++++++++++++++++++++
> 7 files changed, 710 insertions(+), 1 deletion(-)
> 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 <[email protected]>
> +L: [email protected]
> +S: Maintained
> +F: drivers/iio/common/scmi_sensors/scmi_iio.c
> +
...
> 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..31977c3bc600
> --- /dev/null
> +++ b/drivers/iio/common/scmi_sensors/scmi_iio.c
> @@ -0,0 +1,678 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +/*
> + * System Control and Management Interface(SCMI) based IIO sensor driver
> + *
> + * Copyright (C) 2021 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 SCMI_IIO_NUM_OF_AXIS 3
> +
> +struct scmi_iio_priv {
> + struct scmi_handle *handle;
> + const struct scmi_sensor_info *sensor_info;
> + struct iio_dev *indio_dev;
> + /* adding one additional channel for timestamp */
> + long long iio_buf[SCMI_IIO_NUM_OF_AXIS + 1];
Missed this previously but we should probably be careful to
make this explicitly 64 bit rather than rely on long long being
that length. s64 iio_buf[SCMI_IIO_NUM_OF_AXIS + 1];
I can tidy this up whilst applying if that is fine with you.
> + struct notifier_block sensor_update_nb;
> + u32 *freq_avail;
> +};
> +
...
> +
> +static ssize_t scmi_iio_get_raw_available(struct iio_dev *iio_dev,
> + uintptr_t private,
> + const struct iio_chan_spec *chan,
> + char *buf)
Looks good. Thanks for persevering with this!
> +{
> + struct scmi_iio_priv *sensor = iio_priv(iio_dev);
> + unsigned long long resolution, rem;
> + long long min_range, max_range;
> + s8 exponent, scale;
> + int len = 0;
> +
> + /*
> + * All the axes are supposed to have the same value for range and resolution.
> + * We are just using the values from the Axis 0 here.
> + */
> + if (sensor->sensor_info->axis[0].extended_attrs) {
> + min_range = sensor->sensor_info->axis[0].attrs.min_range;
> + max_range = sensor->sensor_info->axis[0].attrs.max_range;
> + resolution = sensor->sensor_info->axis[0].resolution;
> + exponent = sensor->sensor_info->axis[0].exponent;
> + scale = sensor->sensor_info->axis[0].scale;
> +
> + /*
> + * To provide the raw value for the resolution to the userspace,
> + * need to divide the resolution exponent by the sensor scale
> + */
> + exponent = exponent - scale;
> + if (exponent < 0) {
> + resolution = div64_u64_rem(resolution,
> + int_pow(10, abs(exponent)),
> + &rem);
> + len = scnprintf(buf, PAGE_SIZE,
> + "[%lld %llu.%llu %lld]\n", min_range,
> + resolution, rem, max_range);
> + } else {
> + resolution = resolution * int_pow(10, exponent);
> + len = scnprintf(buf, PAGE_SIZE, "[%lld %llu %lld]\n",
> + min_range, resolution, max_range);
> + }
> + }
> + return len;
> +}
> +
> +static const struct iio_chan_spec_ext_info scmi_iio_ext_info[] = {
> + {
> + .name = "raw_available",
> + .read = scmi_iio_get_raw_available,
> + .shared = IIO_SHARED_BY_TYPE,
> + },
> + {},
> +};
> +
> +static void scmi_iio_set_timestamp_channel(struct iio_chan_spec *iio_chan,
> + int scan_index)
Not relevant to this patch!: I wonder how many times
we now have this replicated in various drivers. Feels like a good thing
to just have as a library function in the IIO core.
(about 8 copies of this from a quick grep)
> +{
> + 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 scmi_iio_set_sampling_freq_avail(struct iio_dev *iio_dev)
Really trivial, but if you happen to be respinning for some reason, the
naming of this function is a little confusing. My initial
thought is it would somehow specify the sampling frequencies that
the driver expects to be available, rather than the opposite where
it is the driver establishing what is available.
If _get_ is also ambiguous, perhaps _query_ or _format_ or _convert_
(to reflect you are converting values to expected form).
> +{
> + u64 cur_interval_ns, low_interval_ns, high_interval_ns, step_size_ns,
> + hz, uhz;
...
> +}
> +
> +static int scmi_iio_buffers_setup(struct iio_dev *scmi_iiodev)
> +{
> + struct iio_buffer *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;
Ah. This has now crossed with Alex's large rework of the buffer
registration in the core (to support multiple buffers).
Specifically it needs to flip over to using the function introduced
in https://lore.kernel.org/linux-iio/[email protected]/T/#u
This is going to make taking this via an immutable branch more fiddly.
Don't worry about it though; I'll figure it out once rc1 is out.
(either the merge of this tree will have to before Alex's series, or
I'll need to do a non trivial merge resolution).
The one thing we can't do is rebase this series as that would then delay
Cristian's work for a whole cycle (or require some usual tree management.)
What fun :)
> + return 0;
> +}
> +
> +static struct iio_dev *scmi_alloc_iiodev(struct device *dev,
> + struct scmi_handle *handle,
> + const struct scmi_sensor_info *sensor_info)
> +{
> + struct iio_chan_spec *iio_channels;
> + struct scmi_iio_priv *sensor;
> + enum iio_modifier modifier;
> + enum iio_chan_type type;
> + struct iio_dev *iiodev;
> + int i, ret;
> +
> + iiodev = devm_iio_device_alloc(dev, sizeof(*sensor));
> + if (!iiodev)
> + return ERR_PTR(-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 = scmi_iio_sensor_update_cb;
> + sensor->indio_dev = iiodev;
> +
> + /* adding one additional channel for timestamp */
> + 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 ERR_PTR(-ENOMEM);
> +
> + ret = scmi_iio_set_sampling_freq_avail(iiodev);
> + if (ret < 0)
> + return ERR_PTR(ret);
> +
> + 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 ERR_PTR(ret);
> +
> + ret = scmi_iio_get_chan_modifier(sensor_info->axis[i].name,
> + &modifier);
> + if (ret < 0)
> + return ERR_PTR(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;
> + return iiodev;
> +}
> +
> +static int scmi_iio_dev_probe(struct scmi_device *sdev)
> +{
> + const struct scmi_sensor_info *sensor_info;
> + struct scmi_handle *handle = sdev->handle;
> + struct device *dev = &sdev->dev;
> + struct iio_dev *scmi_iio_dev;
> + u16 nr_sensors;
> + int err = -ENODEV, i;
> +
> + if (!handle || !handle->sensor_ops) {
> + 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 -ENODEV;
> + }
> +
> + 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;
> + }
> +
> + /* This driver only supports 3-axis accel and gyro, skipping other sensors */
> + if (sensor_info->num_axis != SCMI_IIO_NUM_OF_AXIS)
> + continue;
> +
> + /* This driver only supports 3-axis accel and gyro, skipping other sensors */
> + if (sensor_info->axis[0].type != METERS_SEC_SQUARED &&
> + sensor_info->axis[0].type != RADIANS_SEC)
> + continue;
> +
> + scmi_iio_dev = scmi_alloc_iiodev(dev, handle, sensor_info);
> + if (IS_ERR(scmi_iio_dev)) {
> + dev_err(dev,
> + "failed to allocate IIO device for sensor %s: %ld\n",
> + sensor_info->name, PTR_ERR(scmi_iio_dev));
> + return PTR_ERR(scmi_iio_dev);
> + }
> +
> + 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;
> +}
Hi Jonathan,
Thanks for the detailed and careful review of this patch. Good to hear
that v7 is not required. Please find below answers to your
questions. Looking forward to seeing this patch merged in the next
cycle. Thanks for your help in making this happen.
1)
>1) Use of long long to get s64
Please feel free to change this
2)
> Reported-by: kernel test robot <[email protected]>
The test robot gave the following warning which I fixed in this v6 of
the patch. I added static to the below function to fix the warning
>drivers/iio/common/scmi_sensors/scmi_iio.c:537:17: warning: no previous prototype for 'scmi_alloc_iiodev' [-Wmissing-prototypes]
>537 | struct iio_dev *scmi_alloc_iiodev(struct device *dev,
3) Please feel free to change the name to _query if you like
> +static int scmi_iio_set_sampling_freq_avail(struct iio_dev *iio_dev)
>if _get_ is also ambiguous, perhaps _query_ or _format_ or _convert_
(to reflect you are converting values to expected form).
Thanks,
Jyoti
On Sun, Feb 21, 2021 at 6:46 AM Jonathan Cameron <[email protected]> wrote:
>
> On Fri, 12 Feb 2021 17:22:35 +0000
> Jyoti Bhayana <[email protected]> wrote:
>
> > This change provides ARM SCMI Protocol based IIO device.
> > This driver provides support for Accelerometer and Gyroscope using
> > SCMI Sensor Protocol extensions added in the SCMIv3.0 ARM specification
>
> Hi Joyti
>
> A few things inline but nothing to require a v7.
>
> 1) Use of long long to get s64 - I can tidy that up whilst applying.
> 2) Going to have a clash with Alex's multi buffer rework of the core
> I'll sort that out when I apply this as well.
>
> >
> > Reported-by: kernel test robot <[email protected]>
>
> For fixes within a larger patch as a result of the various build robots,
> we should either only mention them in comments, or add a note after the
> Reported-by to say what was fixed. E.g something like
>
> Reported-by: kernel test robot <[email protected]> # off by 1 error
>
> I can't remember what was actually reported for this one.
>
> If you can reply with such a comment I'll add it on, if not I'll drop
> the Reported-by as uninformative. Right now it looks like the whole
> patch is a fix for an issue that 0-day reported :)
>
>
> > Signed-off-by: Jyoti Bhayana <[email protected]>
> > ---
> > MAINTAINERS | 6 +
> > drivers/firmware/arm_scmi/driver.c | 2 +-
> > 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 | 678 +++++++++++++++++++++
> > 7 files changed, 710 insertions(+), 1 deletion(-)
> > 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 <[email protected]>
> > +L: [email protected]
> > +S: Maintained
> > +F: drivers/iio/common/scmi_sensors/scmi_iio.c
> > +
>
> ...
>
> > 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..31977c3bc600
> > --- /dev/null
> > +++ b/drivers/iio/common/scmi_sensors/scmi_iio.c
> > @@ -0,0 +1,678 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +/*
> > + * System Control and Management Interface(SCMI) based IIO sensor driver
> > + *
> > + * Copyright (C) 2021 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 SCMI_IIO_NUM_OF_AXIS 3
> > +
> > +struct scmi_iio_priv {
> > + struct scmi_handle *handle;
> > + const struct scmi_sensor_info *sensor_info;
> > + struct iio_dev *indio_dev;
> > + /* adding one additional channel for timestamp */
> > + long long iio_buf[SCMI_IIO_NUM_OF_AXIS + 1];
>
> Missed this previously but we should probably be careful to
> make this explicitly 64 bit rather than rely on long long being
> that length. s64 iio_buf[SCMI_IIO_NUM_OF_AXIS + 1];
>
> I can tidy this up whilst applying if that is fine with you.
>
> > + struct notifier_block sensor_update_nb;
> > + u32 *freq_avail;
> > +};
> > +
>
> ...
>
> > +
> > +static ssize_t scmi_iio_get_raw_available(struct iio_dev *iio_dev,
> > + uintptr_t private,
> > + const struct iio_chan_spec *chan,
> > + char *buf)
>
> Looks good. Thanks for persevering with this!
>
> > +{
> > + struct scmi_iio_priv *sensor = iio_priv(iio_dev);
> > + unsigned long long resolution, rem;
> > + long long min_range, max_range;
> > + s8 exponent, scale;
> > + int len = 0;
> > +
> > + /*
> > + * All the axes are supposed to have the same value for range and resolution.
> > + * We are just using the values from the Axis 0 here.
> > + */
> > + if (sensor->sensor_info->axis[0].extended_attrs) {
> > + min_range = sensor->sensor_info->axis[0].attrs.min_range;
> > + max_range = sensor->sensor_info->axis[0].attrs.max_range;
> > + resolution = sensor->sensor_info->axis[0].resolution;
> > + exponent = sensor->sensor_info->axis[0].exponent;
> > + scale = sensor->sensor_info->axis[0].scale;
> > +
> > + /*
> > + * To provide the raw value for the resolution to the userspace,
> > + * need to divide the resolution exponent by the sensor scale
> > + */
> > + exponent = exponent - scale;
> > + if (exponent < 0) {
> > + resolution = div64_u64_rem(resolution,
> > + int_pow(10, abs(exponent)),
> > + &rem);
> > + len = scnprintf(buf, PAGE_SIZE,
> > + "[%lld %llu.%llu %lld]\n", min_range,
> > + resolution, rem, max_range);
> > + } else {
> > + resolution = resolution * int_pow(10, exponent);
> > + len = scnprintf(buf, PAGE_SIZE, "[%lld %llu %lld]\n",
> > + min_range, resolution, max_range);
> > + }
> > + }
> > + return len;
> > +}
> > +
> > +static const struct iio_chan_spec_ext_info scmi_iio_ext_info[] = {
> > + {
> > + .name = "raw_available",
> > + .read = scmi_iio_get_raw_available,
> > + .shared = IIO_SHARED_BY_TYPE,
> > + },
> > + {},
> > +};
> > +
> > +static void scmi_iio_set_timestamp_channel(struct iio_chan_spec *iio_chan,
> > + int scan_index)
>
> Not relevant to this patch!: I wonder how many times
> we now have this replicated in various drivers. Feels like a good thing
> to just have as a library function in the IIO core.
> (about 8 copies of this from a quick grep)
>
> > +{
> > + 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 scmi_iio_set_sampling_freq_avail(struct iio_dev *iio_dev)
>
> Really trivial, but if you happen to be respinning for some reason, the
> naming of this function is a little confusing. My initial
> thought is it would somehow specify the sampling frequencies that
> the driver expects to be available, rather than the opposite where
> it is the driver establishing what is available.
>
> If _get_ is also ambiguous, perhaps _query_ or _format_ or _convert_
> (to reflect you are converting values to expected form).
>
> > +{
> > + u64 cur_interval_ns, low_interval_ns, high_interval_ns, step_size_ns,
> > + hz, uhz;
> ...
>
> > +}
> > +
> > +static int scmi_iio_buffers_setup(struct iio_dev *scmi_iiodev)
> > +{
> > + struct iio_buffer *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;
>
> Ah. This has now crossed with Alex's large rework of the buffer
> registration in the core (to support multiple buffers).
>
> Specifically it needs to flip over to using the function introduced
> in https://lore.kernel.org/linux-iio/[email protected]/T/#u
>
> This is going to make taking this via an immutable branch more fiddly.
> Don't worry about it though; I'll figure it out once rc1 is out.
> (either the merge of this tree will have to before Alex's series, or
> I'll need to do a non trivial merge resolution).
>
> The one thing we can't do is rebase this series as that would then delay
> Cristian's work for a whole cycle (or require some usual tree management.)
> What fun :)
>
> > + return 0;
> > +}
> > +
> > +static struct iio_dev *scmi_alloc_iiodev(struct device *dev,
> > + struct scmi_handle *handle,
> > + const struct scmi_sensor_info *sensor_info)
> > +{
> > + struct iio_chan_spec *iio_channels;
> > + struct scmi_iio_priv *sensor;
> > + enum iio_modifier modifier;
> > + enum iio_chan_type type;
> > + struct iio_dev *iiodev;
> > + int i, ret;
> > +
> > + iiodev = devm_iio_device_alloc(dev, sizeof(*sensor));
> > + if (!iiodev)
> > + return ERR_PTR(-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 = scmi_iio_sensor_update_cb;
> > + sensor->indio_dev = iiodev;
> > +
> > + /* adding one additional channel for timestamp */
> > + 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 ERR_PTR(-ENOMEM);
> > +
> > + ret = scmi_iio_set_sampling_freq_avail(iiodev);
> > + if (ret < 0)
> > + return ERR_PTR(ret);
> > +
> > + 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 ERR_PTR(ret);
> > +
> > + ret = scmi_iio_get_chan_modifier(sensor_info->axis[i].name,
> > + &modifier);
> > + if (ret < 0)
> > + return ERR_PTR(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;
> > + return iiodev;
> > +}
> > +
> > +static int scmi_iio_dev_probe(struct scmi_device *sdev)
> > +{
> > + const struct scmi_sensor_info *sensor_info;
> > + struct scmi_handle *handle = sdev->handle;
> > + struct device *dev = &sdev->dev;
> > + struct iio_dev *scmi_iio_dev;
> > + u16 nr_sensors;
> > + int err = -ENODEV, i;
> > +
> > + if (!handle || !handle->sensor_ops) {
> > + 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 -ENODEV;
> > + }
> > +
> > + 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;
> > + }
> > +
> > + /* This driver only supports 3-axis accel and gyro, skipping other sensors */
> > + if (sensor_info->num_axis != SCMI_IIO_NUM_OF_AXIS)
> > + continue;
> > +
> > + /* This driver only supports 3-axis accel and gyro, skipping other sensors */
> > + if (sensor_info->axis[0].type != METERS_SEC_SQUARED &&
> > + sensor_info->axis[0].type != RADIANS_SEC)
> > + continue;
> > +
> > + scmi_iio_dev = scmi_alloc_iiodev(dev, handle, sensor_info);
> > + if (IS_ERR(scmi_iio_dev)) {
> > + dev_err(dev,
> > + "failed to allocate IIO device for sensor %s: %ld\n",
> > + sensor_info->name, PTR_ERR(scmi_iio_dev));
> > + return PTR_ERR(scmi_iio_dev);
> > + }
> > +
> > + 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;
> > +}
Hi Jonathan,
On Tue, Feb 23, 2021 at 10:30:37AM -0800, Jyoti Bhayana wrote:
> Hi Jonathan,
>
> Thanks for the detailed and careful review of this patch. Good to hear
> that v7 is not required. Please find below answers to your
> questions. Looking forward to seeing this patch merged in the next
> cycle. Thanks for your help in making this happen.
>
Any update on this ? Please share the branch with is patch so that I
can pull and ask Cristian to make his changes on top.
--
Regards,
Sudeep
On Mon, 8 Mar 2021 04:28:42 +0000
Sudeep Holla <[email protected]> wrote:
> Hi Jonathan,
>
> On Tue, Feb 23, 2021 at 10:30:37AM -0800, Jyoti Bhayana wrote:
> > Hi Jonathan,
> >
> > Thanks for the detailed and careful review of this patch. Good to hear
> > that v7 is not required. Please find below answers to your
> > questions. Looking forward to seeing this patch merged in the next
> > cycle. Thanks for your help in making this happen.
> >
>
> Any update on this ? Please share the branch with is patch so that I
> can pull and ask Cristian to make his changes on top.
Running a bit behind at the moment.
Anyhow, there should now be an ib-iio-scmi-5.12-rc1 branch
on https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git
Includes making the various long long local variables explicitly
s64 and u64 as relevant.
Based on the rc1 that eats babies so handle with care :)
I've also merge this into the togreg branch of iio.git.
As explained above that wasn't entirely trivial so Jyoti
please take a quick look and check that changes are fine.
Pushed out as testing to let the autobuilders poke at it.
Assuming they don't find anything, it should be fine
for Sudeep to merge that ib and everything will unwind
nicely in Linus' tree next cycle.
There is a bit of an ongoing discussion of an earlier patch
in the IIO tree, so I might end up redoing this merge
if I need to rebase to sort that out, but I'll make sure
the diff is the same (git ID might change).
Thanks,
Jonathan
>
> --
> Regards,
> Sudeep
Hi Jonathan,
I looked at ib-iio-scmi-5.12-rc1 branch and the changes look good to
me. Once again, thanks for all your help.
Thanks,
Jyoti
On Mon, Mar 8, 2021 at 11:48 AM Jonathan Cameron <[email protected]> wrote:
>
> On Mon, 8 Mar 2021 04:28:42 +0000
> Sudeep Holla <[email protected]> wrote:
>
> > Hi Jonathan,
> >
> > On Tue, Feb 23, 2021 at 10:30:37AM -0800, Jyoti Bhayana wrote:
> > > Hi Jonathan,
> > >
> > > Thanks for the detailed and careful review of this patch. Good to hear
> > > that v7 is not required. Please find below answers to your
> > > questions. Looking forward to seeing this patch merged in the next
> > > cycle. Thanks for your help in making this happen.
> > >
> >
> > Any update on this ? Please share the branch with is patch so that I
> > can pull and ask Cristian to make his changes on top.
> Running a bit behind at the moment.
>
> Anyhow, there should now be an ib-iio-scmi-5.12-rc1 branch
> on https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git
>
> Includes making the various long long local variables explicitly
> s64 and u64 as relevant.
>
> Based on the rc1 that eats babies so handle with care :)
>
> I've also merge this into the togreg branch of iio.git.
> As explained above that wasn't entirely trivial so Jyoti
> please take a quick look and check that changes are fine.
> Pushed out as testing to let the autobuilders poke at it.
> Assuming they don't find anything, it should be fine
> for Sudeep to merge that ib and everything will unwind
> nicely in Linus' tree next cycle.
>
> There is a bit of an ongoing discussion of an earlier patch
> in the IIO tree, so I might end up redoing this merge
> if I need to rebase to sort that out, but I'll make sure
> the diff is the same (git ID might change).
>
> Thanks,
>
> Jonathan
>
> >
> > --
> > Regards,
> > Sudeep
>
On Mon, Mar 08, 2021 at 07:48:41PM +0000, Jonathan Cameron wrote:
> On Mon, 8 Mar 2021 04:28:42 +0000
> Sudeep Holla <[email protected]> wrote:
>
> > Hi Jonathan,
> >
> > On Tue, Feb 23, 2021 at 10:30:37AM -0800, Jyoti Bhayana wrote:
> > > Hi Jonathan,
> > >
> > > Thanks for the detailed and careful review of this patch. Good to hear
> > > that v7 is not required. Please find below answers to your
> > > questions. Looking forward to seeing this patch merged in the next
> > > cycle. Thanks for your help in making this happen.
> > >
> >
> > Any update on this ? Please share the branch with is patch so that I
> > can pull and ask Cristian to make his changes on top.
> Running a bit behind at the moment.
>
No worries.
> Anyhow, there should now be an ib-iio-scmi-5.12-rc1 branch
> on https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git
>
Thanks
> Includes making the various long long local variables explicitly
> s64 and u64 as relevant.
>
> Based on the rc1 that eats babies so handle with care :)
>
????
> I've also merge this into the togreg branch of iio.git.
> As explained above that wasn't entirely trivial so Jyoti
> please take a quick look and check that changes are fine.
> Pushed out as testing to let the autobuilders poke at it.
> Assuming they don't find anything, it should be fine
> for Sudeep to merge that ib and everything will unwind
> nicely in Linus' tree next cycle.
>
Hope so.
> There is a bit of an ongoing discussion of an earlier patch
> in the IIO tree, so I might end up redoing this merge
> if I need to rebase to sort that out, but I'll make sure
> the diff is the same (git ID might change).
>
I can wait for a week or 2 if you think things will settle down by then.
We can avoid 2 different git IDs if possible. The main intention was to
give some reference to Cristian to rebase/post his series. I am all
fine to wait for a week or 2 for final branch.
--
Regards,
Sudeep
Hi
On Tue, Mar 09, 2021 at 06:37:27AM +0000, Sudeep Holla wrote:
> On Mon, Mar 08, 2021 at 07:48:41PM +0000, Jonathan Cameron wrote:
> > On Mon, 8 Mar 2021 04:28:42 +0000
> > Sudeep Holla <[email protected]> wrote:
> >
> > > Hi Jonathan,
> > >
> > > On Tue, Feb 23, 2021 at 10:30:37AM -0800, Jyoti Bhayana wrote:
> > > > Hi Jonathan,
> > > >
> > > > Thanks for the detailed and careful review of this patch. Good to hear
> > > > that v7 is not required. Please find below answers to your
> > > > questions. Looking forward to seeing this patch merged in the next
> > > > cycle. Thanks for your help in making this happen.
> > > >
> > >
> > > Any update on this ? Please share the branch with is patch so that I
> > > can pull and ask Cristian to make his changes on top.
> > Running a bit behind at the moment.
> >
>
> No worries.
>
> > Anyhow, there should now be an ib-iio-scmi-5.12-rc1 branch
> > on https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git
> >
>
> Thanks
>
> > Includes making the various long long local variables explicitly
> > s64 and u64 as relevant.
> >
> > Based on the rc1 that eats babies so handle with care :)
> >
>
> ????
>
> > I've also merge this into the togreg branch of iio.git.
> > As explained above that wasn't entirely trivial so Jyoti
> > please take a quick look and check that changes are fine.
> > Pushed out as testing to let the autobuilders poke at it.
> > Assuming they don't find anything, it should be fine
> > for Sudeep to merge that ib and everything will unwind
> > nicely in Linus' tree next cycle.
> >
>
> Hope so.
>
> > There is a bit of an ongoing discussion of an earlier patch
> > in the IIO tree, so I might end up redoing this merge
> > if I need to rebase to sort that out, but I'll make sure
> > the diff is the same (git ID might change).
> >
>
> I can wait for a week or 2 if you think things will settle down by then.
> We can avoid 2 different git IDs if possible. The main intention was to
> give some reference to Cristian to rebase/post his series. I am all
> fine to wait for a week or 2 for final branch.
>
In the meantime, I've anyway started reworking just based on -rc2 and barely
cherry-picked Jyoti v6 on top of it just to able to start porting early the
iiodev to the new SCMI API and be able to fully test it out (no problems
so far); I will finally rebase on whatever final base branch Sudeep will
pick but as Sudeep said I can wait, since I'm not expecting so much work
still to do in that final rebase to -rc1-smh-final (...last famous words o_O)
Side question for Jyoti/Jonathan, for basic testing of this IIO SCMI
driver (given that I'm not really familiar with IIO and I have not a full
Android CTS/VTS suite to use it for testing), I'm doing something like:
(debian-arm64)root@debarm64:~# cd /sys/bus/iio/devices/iio\:device0
(debian-arm64)root@debarm64:/sys/bus/iio/devices/iio:device0# echo 1 > scan_elements/in_accel_x_en
(debian-arm64)root@debarm64:/sys/bus/iio/devices/iio:device0# echo 1 > scan_elements/in_accel_y_en
(debian-arm64)root@debarm64:/sys/bus/iio/devices/iio:device0# echo 1 > scan_elements/in_accel_z_en
(debian-arm64)root@debarm64:/sys/bus/iio/devices/iio:device0# echo 1 > scan_elements/in_timestamp_en
(debian-arm64)root@debarm64:/sys/bus/iio/devices/iio:device0# echo 1 > buffer/enable && cat /dev/iio\:device0 | xxd
00000000: 08da ffff ffff ffff 10da ffff ffff ffff ................
00000010: 18da ffff ffff ffff 00b8 d3a0 c09b 6a16 ..............j.
00000020: 08da ffff ffff ffff 10da ffff ffff ffff ................
00000030: 18da ffff ffff ffff 0082 6edc c09b 6a16 ..........n...j.
00000040: 08da ffff ffff ffff 10da ffff ffff ffff ................
00000050: 18da ffff ffff ffff 004c 0918 c19b 6a16 .........L....j.
00000060: 08da ffff ffff ffff 10da ffff ffff ffff ................
00000070: 18da ffff ffff ffff 0016 a453 c19b 6a16 ...........S..j.
00000080: 08da ffff ffff ffff 10da ffff ffff ffff ................
00000090: 18da ffff ffff ffff 00aa d9ca c19b 6a16 ..............j.
000000a0: 08da ffff ffff ffff 10da ffff ffff ffff ................
000000b0: 18da ffff ffff ffff 0074 7406 c29b 6a16 .........tt...j.
^C
(debian-arm64)root@debarm64:/sys/bus/iio/devices/iio:device0# echo 0 > buffer/enable
(plus a bunch of DEBUG in my series to track notifs flows...)
Is this any good ?
Thanks
Cristian
> --
> Regards,
> Sudeep
On Tue, 9 Mar 2021 07:38:13 +0000
Cristian Marussi <[email protected]> wrote:
> Hi
>
> On Tue, Mar 09, 2021 at 06:37:27AM +0000, Sudeep Holla wrote:
> > On Mon, Mar 08, 2021 at 07:48:41PM +0000, Jonathan Cameron wrote:
> > > On Mon, 8 Mar 2021 04:28:42 +0000
> > > Sudeep Holla <[email protected]> wrote:
> > >
> > > > Hi Jonathan,
> > > >
> > > > On Tue, Feb 23, 2021 at 10:30:37AM -0800, Jyoti Bhayana wrote:
> > > > > Hi Jonathan,
> > > > >
> > > > > Thanks for the detailed and careful review of this patch. Good to hear
> > > > > that v7 is not required. Please find below answers to your
> > > > > questions. Looking forward to seeing this patch merged in the next
> > > > > cycle. Thanks for your help in making this happen.
> > > > >
> > > >
> > > > Any update on this ? Please share the branch with is patch so that I
> > > > can pull and ask Cristian to make his changes on top.
> > > Running a bit behind at the moment.
> > >
> >
> > No worries.
> >
> > > Anyhow, there should now be an ib-iio-scmi-5.12-rc1 branch
> > > on https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git
> > >
> >
> > Thanks
> >
> > > Includes making the various long long local variables explicitly
> > > s64 and u64 as relevant.
> > >
> > > Based on the rc1 that eats babies so handle with care :)
> > >
> >
> > ????
> >
> > > I've also merge this into the togreg branch of iio.git.
> > > As explained above that wasn't entirely trivial so Jyoti
> > > please take a quick look and check that changes are fine.
> > > Pushed out as testing to let the autobuilders poke at it.
> > > Assuming they don't find anything, it should be fine
> > > for Sudeep to merge that ib and everything will unwind
> > > nicely in Linus' tree next cycle.
> > >
> >
> > Hope so.
> >
> > > There is a bit of an ongoing discussion of an earlier patch
> > > in the IIO tree, so I might end up redoing this merge
> > > if I need to rebase to sort that out, but I'll make sure
> > > the diff is the same (git ID might change).
> > >
> >
> > I can wait for a week or 2 if you think things will settle down by then.
> > We can avoid 2 different git IDs if possible. The main intention was to
> > give some reference to Cristian to rebase/post his series. I am all
> > fine to wait for a week or 2 for final branch.
> >
>
> In the meantime, I've anyway started reworking just based on -rc2 and barely
> cherry-picked Jyoti v6 on top of it just to able to start porting early the
> iiodev to the new SCMI API and be able to fully test it out (no problems
> so far); I will finally rebase on whatever final base branch Sudeep will
> pick but as Sudeep said I can wait, since I'm not expecting so much work
> still to do in that final rebase to -rc1-smh-final (...last famous words o_O)
Side note, there was a build bug on 32 bit that needs fixing so we'll be
tweaking the original patch.
>
> Side question for Jyoti/Jonathan, for basic testing of this IIO SCMI
> driver (given that I'm not really familiar with IIO and I have not a full
> Android CTS/VTS suite to use it for testing), I'm doing something like:
>
> (debian-arm64)root@debarm64:~# cd /sys/bus/iio/devices/iio\:device0
> (debian-arm64)root@debarm64:/sys/bus/iio/devices/iio:device0# echo 1 > scan_elements/in_accel_x_en
> (debian-arm64)root@debarm64:/sys/bus/iio/devices/iio:device0# echo 1 > scan_elements/in_accel_y_en
> (debian-arm64)root@debarm64:/sys/bus/iio/devices/iio:device0# echo 1 > scan_elements/in_accel_z_en
> (debian-arm64)root@debarm64:/sys/bus/iio/devices/iio:device0# echo 1 > scan_elements/in_timestamp_en
> (debian-arm64)root@debarm64:/sys/bus/iio/devices/iio:device0# echo 1 > buffer/enable && cat /dev/iio\:device0 | xxd
> 00000000: 08da ffff ffff ffff 10da ffff ffff ffff ................
> 00000010: 18da ffff ffff ffff 00b8 d3a0 c09b 6a16 ..............j.
> 00000020: 08da ffff ffff ffff 10da ffff ffff ffff ................
> 00000030: 18da ffff ffff ffff 0082 6edc c09b 6a16 ..........n...j.
> 00000040: 08da ffff ffff ffff 10da ffff ffff ffff ................
> 00000050: 18da ffff ffff ffff 004c 0918 c19b 6a16 .........L....j.
> 00000060: 08da ffff ffff ffff 10da ffff ffff ffff ................
> 00000070: 18da ffff ffff ffff 0016 a453 c19b 6a16 ...........S..j.
> 00000080: 08da ffff ffff ffff 10da ffff ffff ffff ................
> 00000090: 18da ffff ffff ffff 00aa d9ca c19b 6a16 ..............j.
> 000000a0: 08da ffff ffff ffff 10da ffff ffff ffff ................
> 000000b0: 18da ffff ffff ffff 0074 7406 c29b 6a16 .........tt...j.
> ^C
> (debian-arm64)root@debarm64:/sys/bus/iio/devices/iio:device0# echo 0 > buffer/enable
That looks superficially plausible.
There is a test tool in
tools/iio/iio_generic_buffer.c which is fairly simple to use and will pretty print values for
you. That'll sanity check all the type descriptions etc are correct as well.
Jonathan
>
> (plus a bunch of DEBUG in my series to track notifs flows...)
>
> Is this any good ?
>
> Thanks
>
> Cristian
>
> > --
> > Regards,
> > Sudeep
Hi Jonathan,
On Tue, Mar 09, 2021 at 10:27:01AM +0000, Jonathan Cameron wrote:
> On Tue, 9 Mar 2021 07:38:13 +0000
> Cristian Marussi <[email protected]> wrote:
>
> > Hi
> >
> > On Tue, Mar 09, 2021 at 06:37:27AM +0000, Sudeep Holla wrote:
> > > On Mon, Mar 08, 2021 at 07:48:41PM +0000, Jonathan Cameron wrote:
> > > > On Mon, 8 Mar 2021 04:28:42 +0000
> > > > Sudeep Holla <[email protected]> wrote:
> > > >
> > > > > Hi Jonathan,
> > > > >
> > > > > On Tue, Feb 23, 2021 at 10:30:37AM -0800, Jyoti Bhayana wrote:
> > > > > > Hi Jonathan,
> > > > > >
> > > > > > Thanks for the detailed and careful review of this patch. Good to hear
> > > > > > that v7 is not required. Please find below answers to your
> > > > > > questions. Looking forward to seeing this patch merged in the next
> > > > > > cycle. Thanks for your help in making this happen.
> > > > > >
> > > > >
> > > > > Any update on this ? Please share the branch with is patch so that I
> > > > > can pull and ask Cristian to make his changes on top.
> > > > Running a bit behind at the moment.
> > > >
> > >
> > > No worries.
> > >
> > > > Anyhow, there should now be an ib-iio-scmi-5.12-rc1 branch
> > > > on https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git
> > > >
> > >
> > > Thanks
> > >
> > > > Includes making the various long long local variables explicitly
> > > > s64 and u64 as relevant.
> > > >
> > > > Based on the rc1 that eats babies so handle with care :)
> > > >
> > >
> > > ????
> > >
> > > > I've also merge this into the togreg branch of iio.git.
> > > > As explained above that wasn't entirely trivial so Jyoti
> > > > please take a quick look and check that changes are fine.
> > > > Pushed out as testing to let the autobuilders poke at it.
> > > > Assuming they don't find anything, it should be fine
> > > > for Sudeep to merge that ib and everything will unwind
> > > > nicely in Linus' tree next cycle.
> > > >
> > >
> > > Hope so.
> > >
> > > > There is a bit of an ongoing discussion of an earlier patch
> > > > in the IIO tree, so I might end up redoing this merge
> > > > if I need to rebase to sort that out, but I'll make sure
> > > > the diff is the same (git ID might change).
> > > >
> > >
> > > I can wait for a week or 2 if you think things will settle down by then.
> > > We can avoid 2 different git IDs if possible. The main intention was to
> > > give some reference to Cristian to rebase/post his series. I am all
> > > fine to wait for a week or 2 for final branch.
> > >
> >
> > In the meantime, I've anyway started reworking just based on -rc2 and barely
> > cherry-picked Jyoti v6 on top of it just to able to start porting early the
> > iiodev to the new SCMI API and be able to fully test it out (no problems
> > so far); I will finally rebase on whatever final base branch Sudeep will
> > pick but as Sudeep said I can wait, since I'm not expecting so much work
> > still to do in that final rebase to -rc1-smh-final (...last famous words o_O)
>
> Side note, there was a build bug on 32 bit that needs fixing so we'll be
> tweaking the original patch.
>
Sure, thanks, I'll rework it anyway, it was just preliminary work.
> >
> > Side question for Jyoti/Jonathan, for basic testing of this IIO SCMI
> > driver (given that I'm not really familiar with IIO and I have not a full
> > Android CTS/VTS suite to use it for testing), I'm doing something like:
> >
> > (debian-arm64)root@debarm64:~# cd /sys/bus/iio/devices/iio\:device0
> > (debian-arm64)root@debarm64:/sys/bus/iio/devices/iio:device0# echo 1 > scan_elements/in_accel_x_en
> > (debian-arm64)root@debarm64:/sys/bus/iio/devices/iio:device0# echo 1 > scan_elements/in_accel_y_en
> > (debian-arm64)root@debarm64:/sys/bus/iio/devices/iio:device0# echo 1 > scan_elements/in_accel_z_en
> > (debian-arm64)root@debarm64:/sys/bus/iio/devices/iio:device0# echo 1 > scan_elements/in_timestamp_en
> > (debian-arm64)root@debarm64:/sys/bus/iio/devices/iio:device0# echo 1 > buffer/enable && cat /dev/iio\:device0 | xxd
> > 00000000: 08da ffff ffff ffff 10da ffff ffff ffff ................
> > 00000010: 18da ffff ffff ffff 00b8 d3a0 c09b 6a16 ..............j.
> > 00000020: 08da ffff ffff ffff 10da ffff ffff ffff ................
> > 00000030: 18da ffff ffff ffff 0082 6edc c09b 6a16 ..........n...j.
> > 00000040: 08da ffff ffff ffff 10da ffff ffff ffff ................
> > 00000050: 18da ffff ffff ffff 004c 0918 c19b 6a16 .........L....j.
> > 00000060: 08da ffff ffff ffff 10da ffff ffff ffff ................
> > 00000070: 18da ffff ffff ffff 0016 a453 c19b 6a16 ...........S..j.
> > 00000080: 08da ffff ffff ffff 10da ffff ffff ffff ................
> > 00000090: 18da ffff ffff ffff 00aa d9ca c19b 6a16 ..............j.
> > 000000a0: 08da ffff ffff ffff 10da ffff ffff ffff ................
> > 000000b0: 18da ffff ffff ffff 0074 7406 c29b 6a16 .........tt...j.
> > ^C
> > (debian-arm64)root@debarm64:/sys/bus/iio/devices/iio:device0# echo 0 > buffer/enable
> That looks superficially plausible.
>
Yes, definitely not an in depth testing, I was just trying to double
check not to break the general mechanism when porting to the new API.
> There is a test tool in
> tools/iio/iio_generic_buffer.c which is fairly simple to use and will pretty print values for
> you. That'll sanity check all the type descriptions etc are correct as well.
>
Cool, thanks a lot I'll give it a go.
Thanks
Cristian
> Jonathan
>
> >
> > (plus a bunch of DEBUG in my series to track notifs flows...)
> >
> > Is this any good ?
> >
> > Thanks
> >
> > Cristian
> >
> > > --
> > > Regards,
> > > Sudeep
>