All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v5 0/1] Adding support for IIO SCMI based sensors
@ 2021-02-08 21:19 Jyoti Bhayana
  2021-02-08 21:19 ` [PATCH v5 1/1] iio/scmi: Adding support for IIO SCMI Based Sensors Jyoti Bhayana
  0 siblings, 1 reply; 13+ messages in thread
From: Jyoti Bhayana @ 2021-02-08 21:19 UTC (permalink / raw)
  To: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
	Peter Meerwald-Stadler, Jyoti Bhayana, Mauro Carvalho Chehab,
	David S. Miller, Rob Herring, Lukas Bulwahn
  Cc: linux-kernel, linux-iio, cristian.marussi, sudeep.holla,
	egranata, mikhail.golubev, Igor.Skalkin, Peter.hilber,
	ankitarora

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

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 | 673 +++++++++++++++++++++
 7 files changed, 705 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


^ permalink raw reply	[flat|nested] 13+ messages in thread
* [PATCH v5 1/1] iio/scmi: Adding support for IIO SCMI Based Sensors
  2021-02-08 21:19 [PATCH v5 0/1] Adding support for IIO SCMI based sensors Jyoti Bhayana
@ 2021-02-08 21:19 ` Jyoti Bhayana
  2021-02-09  3:27     ` kernel test robot
  2021-02-09 11:56   ` Cristian Marussi
  0 siblings, 2 replies; 13+ messages in thread
From: Jyoti Bhayana @ 2021-02-08 21:19 UTC (permalink / raw)
  To: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
	Peter Meerwald-Stadler, Jyoti Bhayana, Mauro Carvalho Chehab,
	David S. Miller, Rob Herring, Lukas Bulwahn
  Cc: linux-kernel, linux-iio, cristian.marussi, sudeep.holla,
	egranata, mikhail.golubev, Igor.Skalkin, Peter.hilber,
	ankitarora

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

Signed-off-by: Jyoti Bhayana <jbhayana@google.com>
---
 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 | 673 +++++++++++++++++++++
 7 files changed, 705 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 <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/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..093b1fc24e27
--- /dev/null
+++ b/drivers/iio/common/scmi_sensors/scmi_iio.c
@@ -0,0 +1,673 @@
+// 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;
+
+	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
+	 */
+	mult = ilog2(((u64)uHz * 0xFFFF) / UHZ_PER_HZ) / const_ilog2(10);
+
+	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;
+}
+
+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);
+
+	scmi_iio_set_sampling_freq_avail(iiodev);
+
+	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 <jbhayana@google.com>");
+MODULE_DESCRIPTION("SCMI IIO Driver");
+MODULE_LICENSE("GPL v2");
-- 
2.30.0.478.g8a0d178c01-goog


^ permalink raw reply related	[flat|nested] 13+ messages in thread
* Re: [PATCH v5 1/1] iio/scmi: Adding support for IIO SCMI Based Sensors
  2021-02-08 21:19 ` [PATCH v5 1/1] iio/scmi: Adding support for IIO SCMI Based Sensors Jyoti Bhayana
@ 2021-02-09  3:27     ` kernel test robot
  2021-02-09 11:56   ` Cristian Marussi
  1 sibling, 0 replies; 13+ messages in thread
From: kernel test robot @ 2021-02-09  3:27 UTC (permalink / raw)
  To: Jyoti Bhayana, Jonathan Cameron, Hartmut Knaack,
	Lars-Peter Clausen, Peter Meerwald-Stadler,
	Mauro Carvalho Chehab, David S. Miller, Rob Herring
  Cc: kbuild-all, linux-media, netdev

[-- Attachment #1: Type: text/plain, Size: 3795 bytes --]

Hi Jyoti,

I love your patch! Perhaps something to improve:

[auto build test WARNING on iio/togreg]
[also build test WARNING on linus/master sparc-next/master v5.11-rc6 next-20210125]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Jyoti-Bhayana/Adding-support-for-IIO-SCMI-based-sensors/20210209-072954
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
config: arm64-allyesconfig (attached as .config)
compiler: aarch64-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/90744ef17921050ddb26b5c750ba690a2e3e222e
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Jyoti-Bhayana/Adding-support-for-IIO-SCMI-based-sensors/20210209-072954
        git checkout 90744ef17921050ddb26b5c750ba690a2e3e222e
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=arm64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> 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,
         |                 ^~~~~~~~~~~~~~~~~


vim +/scmi_alloc_iiodev +537 drivers/iio/common/scmi_sensors/scmi_iio.c

   536	
 > 537	struct iio_dev *scmi_alloc_iiodev(struct device *dev,
   538					  struct scmi_handle *handle,
   539					  const struct scmi_sensor_info *sensor_info)
   540	{
   541		struct iio_chan_spec *iio_channels;
   542		struct scmi_iio_priv *sensor;
   543		enum iio_modifier modifier;
   544		enum iio_chan_type type;
   545		struct iio_dev *iiodev;
   546		int i, ret;
   547	
   548		iiodev = devm_iio_device_alloc(dev, sizeof(*sensor));
   549		if (!iiodev)
   550			return ERR_PTR(-ENOMEM);
   551	
   552		iiodev->modes = INDIO_DIRECT_MODE;
   553		iiodev->dev.parent = dev;
   554		sensor = iio_priv(iiodev);
   555		sensor->handle = handle;
   556		sensor->sensor_info = sensor_info;
   557		sensor->sensor_update_nb.notifier_call = scmi_iio_sensor_update_cb;
   558		sensor->indio_dev = iiodev;
   559	
   560		/* adding one additional channel for timestamp */
   561		iiodev->num_channels = sensor_info->num_axis + 1;
   562		iiodev->name = sensor_info->name;
   563		iiodev->info = &scmi_iio_info;
   564	
   565		iio_channels =
   566			devm_kzalloc(dev,
   567				     sizeof(*iio_channels) * (iiodev->num_channels),
   568				     GFP_KERNEL);
   569		if (!iio_channels)
   570			return ERR_PTR(-ENOMEM);
   571	
   572		scmi_iio_set_sampling_freq_avail(iiodev);
   573	
   574		for (i = 0; i < sensor_info->num_axis; i++) {
   575			ret = scmi_iio_get_chan_type(sensor_info->axis[i].type, &type);
   576			if (ret < 0)
   577				return ERR_PTR(ret);
   578	
   579			ret = scmi_iio_get_chan_modifier(sensor_info->axis[i].name,
   580							 &modifier);
   581			if (ret < 0)
   582				return ERR_PTR(ret);
   583	
   584			scmi_iio_set_data_channel(&iio_channels[i], type, modifier,
   585						  sensor_info->axis[i].id);
   586		}
   587	
   588		scmi_iio_set_timestamp_channel(&iio_channels[i], i);
   589		iiodev->channels = iio_channels;
   590		return iiodev;
   591	}
   592	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 76567 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread
* Re: [PATCH v5 1/1] iio/scmi: Adding support for IIO SCMI Based Sensors
@ 2021-02-09  3:27     ` kernel test robot
  0 siblings, 0 replies; 13+ messages in thread
From: kernel test robot @ 2021-02-09  3:27 UTC (permalink / raw)
  To: kbuild-all

[-- Attachment #1: Type: text/plain, Size: 3894 bytes --]

Hi Jyoti,

I love your patch! Perhaps something to improve:

[auto build test WARNING on iio/togreg]
[also build test WARNING on linus/master sparc-next/master v5.11-rc6 next-20210125]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Jyoti-Bhayana/Adding-support-for-IIO-SCMI-based-sensors/20210209-072954
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
config: arm64-allyesconfig (attached as .config)
compiler: aarch64-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/90744ef17921050ddb26b5c750ba690a2e3e222e
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Jyoti-Bhayana/Adding-support-for-IIO-SCMI-based-sensors/20210209-072954
        git checkout 90744ef17921050ddb26b5c750ba690a2e3e222e
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=arm64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> 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,
         |                 ^~~~~~~~~~~~~~~~~


vim +/scmi_alloc_iiodev +537 drivers/iio/common/scmi_sensors/scmi_iio.c

   536	
 > 537	struct iio_dev *scmi_alloc_iiodev(struct device *dev,
   538					  struct scmi_handle *handle,
   539					  const struct scmi_sensor_info *sensor_info)
   540	{
   541		struct iio_chan_spec *iio_channels;
   542		struct scmi_iio_priv *sensor;
   543		enum iio_modifier modifier;
   544		enum iio_chan_type type;
   545		struct iio_dev *iiodev;
   546		int i, ret;
   547	
   548		iiodev = devm_iio_device_alloc(dev, sizeof(*sensor));
   549		if (!iiodev)
   550			return ERR_PTR(-ENOMEM);
   551	
   552		iiodev->modes = INDIO_DIRECT_MODE;
   553		iiodev->dev.parent = dev;
   554		sensor = iio_priv(iiodev);
   555		sensor->handle = handle;
   556		sensor->sensor_info = sensor_info;
   557		sensor->sensor_update_nb.notifier_call = scmi_iio_sensor_update_cb;
   558		sensor->indio_dev = iiodev;
   559	
   560		/* adding one additional channel for timestamp */
   561		iiodev->num_channels = sensor_info->num_axis + 1;
   562		iiodev->name = sensor_info->name;
   563		iiodev->info = &scmi_iio_info;
   564	
   565		iio_channels =
   566			devm_kzalloc(dev,
   567				     sizeof(*iio_channels) * (iiodev->num_channels),
   568				     GFP_KERNEL);
   569		if (!iio_channels)
   570			return ERR_PTR(-ENOMEM);
   571	
   572		scmi_iio_set_sampling_freq_avail(iiodev);
   573	
   574		for (i = 0; i < sensor_info->num_axis; i++) {
   575			ret = scmi_iio_get_chan_type(sensor_info->axis[i].type, &type);
   576			if (ret < 0)
   577				return ERR_PTR(ret);
   578	
   579			ret = scmi_iio_get_chan_modifier(sensor_info->axis[i].name,
   580							 &modifier);
   581			if (ret < 0)
   582				return ERR_PTR(ret);
   583	
   584			scmi_iio_set_data_channel(&iio_channels[i], type, modifier,
   585						  sensor_info->axis[i].id);
   586		}
   587	
   588		scmi_iio_set_timestamp_channel(&iio_channels[i], i);
   589		iiodev->channels = iio_channels;
   590		return iiodev;
   591	}
   592	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 76567 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread
* Re: [PATCH v5 1/1] iio/scmi: Adding support for IIO SCMI Based Sensors
  2021-02-08 21:19 ` [PATCH v5 1/1] iio/scmi: Adding support for IIO SCMI Based Sensors Jyoti Bhayana
  2021-02-09  3:27     ` kernel test robot
@ 2021-02-09 11:56   ` Cristian Marussi
       [not found]     ` <CA+=V6c12nRxLCxM2DPst8RV=i+1WatPyHcQQZp4xAzuoN0vKaw@mail.gmail.com>
  1 sibling, 1 reply; 13+ messages in thread
From: Cristian Marussi @ 2021-02-09 11:56 UTC (permalink / raw)
  To: Jyoti Bhayana
  Cc: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
	Peter Meerwald-Stadler, Mauro Carvalho Chehab, David S. Miller,
	Rob Herring, Lukas Bulwahn, linux-kernel, linux-iio,
	sudeep.holla, egranata, mikhail.golubev, Igor.Skalkin,
	Peter.hilber, ankitarora, cristian.marussi

Hi Jyoti

some minor things down below.

Other than that, FWIW about the SCMI side of this:

Reviewed-by: Cristian Marussi <cristian.marussi@arm.com>

Thanks

Cristian

On Mon, Feb 08, 2021 at 09:19:18PM +0000, Jyoti Bhayana 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
> 
> Signed-off-by: Jyoti Bhayana <jbhayana@google.com>
> ---
>  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 | 673 +++++++++++++++++++++
>  7 files changed, 705 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 <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/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..093b1fc24e27
> --- /dev/null
> +++ b/drivers/iio/common/scmi_sensors/scmi_iio.c
> @@ -0,0 +1,673 @@
> +// 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,
> +};

This is just a question, I'm not suggesting to change anything here at
this point to be clear, since it works just fine as it is.

Following up a previous email, given these are called on enable/disable
by sysfs, is there a specific reason why you configure here, inside
these ops, also timestamping and callbacks  i.e. each time the sensor is
turned on/off by sysfs ? ... instead of just, as an example, enabling
in _preenable the sensor while registering callbacks and enabling
timestamping once for all earlier during probe phase ?
(likewise for _postdisable -> remove)

AFAIU the spec says notifications are emitted for sensors which has
requested them (via SENSOR_CONTINUOUS_UPDATE_NOTIFY) BUT only if the
sensor is enabled as a whole (via proper CONFIG_SET as you do), so
that enabling/disabling the sensor as a whole should result in starting/
stopping the notification flow without the need of unregistering the
callbacks everytime. (same goes with the timestamping)

In other words, I would expect the sensor to maintain its state (on the
platform side) even when going through enable/disable cycles, so that
it 'remembers' that timestamping/notifications were enabled across an
on/off.

This would reduce the number of SCMI messages exchanges between the
kernel and the platform and should be supported by both, but as said,
it's more of a question for the future, not necessarily for this series.

> +

[snip]

> +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;
> +}
> +
> +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);
> +
> +	scmi_iio_set_sampling_freq_avail(iiodev);

You don't check this for retval, and it could fail with -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 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;
> +}
> +

^ permalink raw reply	[flat|nested] 13+ messages in thread
* Re: [PATCH v5 1/1] iio/scmi: Adding support for IIO SCMI Based Sensors
       [not found]     ` <CA+=V6c12nRxLCxM2DPst8RV=i+1WatPyHcQQZp4xAzuoN0vKaw@mail.gmail.com>
@ 2021-02-10 21:46       ` Cristian Marussi
  2021-02-12 19:18         ` Jonathan Cameron
  0 siblings, 1 reply; 13+ messages in thread
From: Cristian Marussi @ 2021-02-10 21:46 UTC (permalink / raw)
  To: Jyoti Bhayana
  Cc: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
	Peter Meerwald-Stadler, Mauro Carvalho Chehab, David S. Miller,
	Rob Herring, Lukas Bulwahn, linux-kernel, linux-iio,
	Sudeep Holla, Enrico Granata, Mikhail Golubev, Igor Skalkin,
	Peter Hilber, Ankit Arora, cristian.marussi

Hi Jyoti,

On Wed, Feb 10, 2021 at 11:19:35AM -0800, Jyoti Bhayana wrote:
> Hi Cristian,
> 
> Thanks for the feedback. Regarding registering callbacks at the probe time
> instead of .preenable, I have tried it before but I think due to some
> issues(don't remember it now maybe on the platform side)  I kept it at the
> .preenable level.
> 
> But you are right, that it will be nice to move it at the probe level
> instead. I will try again and test if it works and would move it at the
> probe level. Regarding the unregistering of the notifier, is it required at
> the remove of iio driver or scmi driver will take care of it?
> Because if I add unregister at the iio driver remove level, I would have to
> iterate all the sensors again and unregister them.

Yes you are right if you move callbacks registration once for all to the
.probe step you'll have to unregister them all in a .remove.

BUT I think instead you should stick with your current solution given
it's working fine anyway and it's supported by the notification
framework and also for another reason I'm going to explain down below
(which is also the reason why I asked you this at first :D)

As you may remember I'm refactoring all the SCMI internals in a separate
series to ease modularization and vendor protocols support, and that will
lead also to some changes in the SCMI driver interface that you use:
amongst other things one interesting addition will be a new devres managed
notification registration method, something like:

handle->notify_ops->devm_register_notifier(sdev, ...);

With such method you could just move your registration to the .probe
step and just forget about it, without the need to add any unregistration
in the .remove step, since the core will take care to remove all the
callbacks at driver unloading time.

Now, this series, which is here if you want to have a look:

https://lore.kernel.org/linux-arm-kernel/20210202221555.41167-1-cristian.marussi@arm.com/

is already taking care to port any existent SCMI driver to the new interface,
so when your IIODEV SCMI driver will be finally queued somewhere for merge, I
can in turn rebase my series on yours and take care to port your driver too to
the new interface applying the changes above in the context of my series.
(and ask you to review of course :D)

I'm saying that is better if you keep your series as it is for now
(old interface + .preenable/.postdisable regs/unregs) because, as said,
with the new interface the devm_ methods will ease the registration
@probe time, and also especially because the new interface is not (and
most probably won't) be part of the v5.4 backport that you are testing
against: so if you stick with your current solution you'll have a
working patch easily backportable now, and once queued I'll port it to
the interface using devm_ (so simplifying it)

In this context, it would be indeed important to know if in general moving
registration to the probe phase (which should be fine by the spec) poses
any kind of problem. (and that's reason why asked it)

Hope to have been clear despite the flood of words :D

Thanks

Cristian

> 
> Thanks,
> Jyoti
> 
> On Tue, Feb 9, 2021 at 3:56 AM Cristian Marussi <cristian.marussi@arm.com>
> wrote:
> 
> > Hi Jyoti
> >
> > some minor things down below.
> >
> > Other than that, FWIW about the SCMI side of this:
> >
> > Reviewed-by: Cristian Marussi <cristian.marussi@arm.com>
> >
> > Thanks
> >
> > Cristian
> >
> > On Mon, Feb 08, 2021 at 09:19:18PM +0000, Jyoti Bhayana 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
> > >
> > > Signed-off-by: Jyoti Bhayana <jbhayana@google.com>
> > > ---
> > >  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 | 673 +++++++++++++++++++++
> > >  7 files changed, 705 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 <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/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..093b1fc24e27
> > > --- /dev/null
> > > +++ b/drivers/iio/common/scmi_sensors/scmi_iio.c
> > > @@ -0,0 +1,673 @@
> > > +// 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,
> > > +};
> >
> > This is just a question, I'm not suggesting to change anything here at
> > this point to be clear, since it works just fine as it is.
> >
> > Following up a previous email, given these are called on enable/disable
> > by sysfs, is there a specific reason why you configure here, inside
> > these ops, also timestamping and callbacks  i.e. each time the sensor is
> > turned on/off by sysfs ? ... instead of just, as an example, enabling
> > in _preenable the sensor while registering callbacks and enabling
> > timestamping once for all earlier during probe phase ?
> > (likewise for _postdisable -> remove)
> >
> > AFAIU the spec says notifications are emitted for sensors which has
> > requested them (via SENSOR_CONTINUOUS_UPDATE_NOTIFY) BUT only if the
> > sensor is enabled as a whole (via proper CONFIG_SET as you do), so
> > that enabling/disabling the sensor as a whole should result in starting/
> > stopping the notification flow without the need of unregistering the
> > callbacks everytime. (same goes with the timestamping)
> >
> > In other words, I would expect the sensor to maintain its state (on the
> > platform side) even when going through enable/disable cycles, so that
> > it 'remembers' that timestamping/notifications were enabled across an
> > on/off.
> >
> > This would reduce the number of SCMI messages exchanges between the
> > kernel and the platform and should be supported by both, but as said,
> > it's more of a question for the future, not necessarily for this series.
> >
> > > +
> >
> > [snip]
> >
> > > +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;
> > > +}
> > > +
> > > +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);
> > > +
> > > +     scmi_iio_set_sampling_freq_avail(iiodev);
> >
> > You don't check this for retval, and it could fail with -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 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;
> > > +}
> > > +
> >

^ permalink raw reply	[flat|nested] 13+ messages in thread
* Re: [PATCH v5 1/1] iio/scmi: Adding support for IIO SCMI Based Sensors
  2021-02-10 21:46       ` Cristian Marussi
@ 2021-02-12 19:18         ` Jonathan Cameron
  2021-02-15  9:25           ` Cristian Marussi
  0 siblings, 1 reply; 13+ messages in thread
From: Jonathan Cameron @ 2021-02-12 19:18 UTC (permalink / raw)
  To: Cristian Marussi
  Cc: Jyoti Bhayana, Hartmut Knaack, Lars-Peter Clausen,
	Peter Meerwald-Stadler, Mauro Carvalho Chehab, David S. Miller,
	Rob Herring, Lukas Bulwahn, linux-kernel, linux-iio,
	Sudeep Holla, Enrico Granata, Mikhail Golubev, Igor Skalkin,
	Peter Hilber, Ankit Arora

On Wed, 10 Feb 2021 21:46:19 +0000
Cristian Marussi <cristian.marussi@arm.com> wrote:

> Hi Jyoti,
> 
> On Wed, Feb 10, 2021 at 11:19:35AM -0800, Jyoti Bhayana wrote:
> > Hi Cristian,
> > 
> > Thanks for the feedback. Regarding registering callbacks at the probe time
> > instead of .preenable, I have tried it before but I think due to some
> > issues(don't remember it now maybe on the platform side)  I kept it at the
> > .preenable level.
> > 
> > But you are right, that it will be nice to move it at the probe level
> > instead. I will try again and test if it works and would move it at the
> > probe level. Regarding the unregistering of the notifier, is it required at
> > the remove of iio driver or scmi driver will take care of it?
> > Because if I add unregister at the iio driver remove level, I would have to
> > iterate all the sensors again and unregister them.  
> 
> Yes you are right if you move callbacks registration once for all to the
> .probe step you'll have to unregister them all in a .remove.
> 
> BUT I think instead you should stick with your current solution given
> it's working fine anyway and it's supported by the notification
> framework and also for another reason I'm going to explain down below
> (which is also the reason why I asked you this at first :D)
> 
> As you may remember I'm refactoring all the SCMI internals in a separate
> series to ease modularization and vendor protocols support, and that will
> lead also to some changes in the SCMI driver interface that you use:
> amongst other things one interesting addition will be a new devres managed
> notification registration method, something like:
> 
> handle->notify_ops->devm_register_notifier(sdev, ...);
> 
> With such method you could just move your registration to the .probe
> step and just forget about it, without the need to add any unregistration
> in the .remove step, since the core will take care to remove all the
> callbacks at driver unloading time.
> 
> Now, this series, which is here if you want to have a look:
> 
> https://lore.kernel.org/linux-arm-kernel/20210202221555.41167-1-cristian.marussi@arm.com/
> 
> is already taking care to port any existent SCMI driver to the new interface,
> so when your IIODEV SCMI driver will be finally queued somewhere for merge, I
> can in turn rebase my series on yours and take care to port your driver too to
> the new interface applying the changes above in the context of my series.
> (and ask you to review of course :D)

I'm guessing you probably want this driver in an immutable branch then
rather than having to wait another cycle for it to tick through to a
a sensible upstream?

Jonathan

> 
> I'm saying that is better if you keep your series as it is for now
> (old interface + .preenable/.postdisable regs/unregs) because, as said,
> with the new interface the devm_ methods will ease the registration
> @probe time, and also especially because the new interface is not (and
> most probably won't) be part of the v5.4 backport that you are testing
> against: so if you stick with your current solution you'll have a
> working patch easily backportable now, and once queued I'll port it to
> the interface using devm_ (so simplifying it)
> 
> In this context, it would be indeed important to know if in general moving
> registration to the probe phase (which should be fine by the spec) poses
> any kind of problem. (and that's reason why asked it)
> 
> Hope to have been clear despite the flood of words :D
> 
> Thanks
> 
> Cristian
> 
> > 
> > Thanks,
> > Jyoti
> > 
> > On Tue, Feb 9, 2021 at 3:56 AM Cristian Marussi <cristian.marussi@arm.com>
> > wrote:
> >   
> > > Hi Jyoti
> > >
> > > some minor things down below.
> > >
> > > Other than that, FWIW about the SCMI side of this:
> > >
> > > Reviewed-by: Cristian Marussi <cristian.marussi@arm.com>
> > >
> > > Thanks
> > >
> > > Cristian
> > >
> > > On Mon, Feb 08, 2021 at 09:19:18PM +0000, Jyoti Bhayana 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
> > > >
> > > > Signed-off-by: Jyoti Bhayana <jbhayana@google.com>
> > > > ---
> > > >  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 | 673 +++++++++++++++++++++
> > > >  7 files changed, 705 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 <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/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..093b1fc24e27
> > > > --- /dev/null
> > > > +++ b/drivers/iio/common/scmi_sensors/scmi_iio.c
> > > > @@ -0,0 +1,673 @@
> > > > +// 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,
> > > > +};  
> > >
> > > This is just a question, I'm not suggesting to change anything here at
> > > this point to be clear, since it works just fine as it is.
> > >
> > > Following up a previous email, given these are called on enable/disable
> > > by sysfs, is there a specific reason why you configure here, inside
> > > these ops, also timestamping and callbacks  i.e. each time the sensor is
> > > turned on/off by sysfs ? ... instead of just, as an example, enabling
> > > in _preenable the sensor while registering callbacks and enabling
> > > timestamping once for all earlier during probe phase ?
> > > (likewise for _postdisable -> remove)
> > >
> > > AFAIU the spec says notifications are emitted for sensors which has
> > > requested them (via SENSOR_CONTINUOUS_UPDATE_NOTIFY) BUT only if the
> > > sensor is enabled as a whole (via proper CONFIG_SET as you do), so
> > > that enabling/disabling the sensor as a whole should result in starting/
> > > stopping the notification flow without the need of unregistering the
> > > callbacks everytime. (same goes with the timestamping)
> > >
> > > In other words, I would expect the sensor to maintain its state (on the
> > > platform side) even when going through enable/disable cycles, so that
> > > it 'remembers' that timestamping/notifications were enabled across an
> > > on/off.
> > >
> > > This would reduce the number of SCMI messages exchanges between the
> > > kernel and the platform and should be supported by both, but as said,
> > > it's more of a question for the future, not necessarily for this series.
> > >  
> > > > +  
> > >
> > > [snip]
> > >  
> > > > +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;
> > > > +}
> > > > +
> > > > +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);
> > > > +
> > > > +     scmi_iio_set_sampling_freq_avail(iiodev);  
> > >
> > > You don't check this for retval, and it could fail with -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 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;
> > > > +}
> > > > +  
> > >  


^ permalink raw reply	[flat|nested] 13+ messages in thread
* Re: [PATCH v5 1/1] iio/scmi: Adding support for IIO SCMI Based Sensors
  2021-02-12 19:18         ` Jonathan Cameron
@ 2021-02-15  9:25           ` Cristian Marussi
  2021-02-15 11:07             ` Jonathan Cameron
  0 siblings, 1 reply; 13+ messages in thread
From: Cristian Marussi @ 2021-02-15  9:25 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: Jyoti Bhayana, Hartmut Knaack, Lars-Peter Clausen,
	Peter Meerwald-Stadler, Mauro Carvalho Chehab, David S. Miller,
	Rob Herring, Lukas Bulwahn, linux-kernel, linux-iio,
	Sudeep Holla, Enrico Granata, Mikhail Golubev, Igor Skalkin,
	Peter Hilber, Ankit Arora

On Fri, Feb 12, 2021 at 07:18:58PM +0000, Jonathan Cameron wrote:
> On Wed, 10 Feb 2021 21:46:19 +0000
> Cristian Marussi <cristian.marussi@arm.com> wrote:
> 
> > Hi Jyoti,
> > 
> > On Wed, Feb 10, 2021 at 11:19:35AM -0800, Jyoti Bhayana wrote:
> > > Hi Cristian,
> > > 
> > > Thanks for the feedback. Regarding registering callbacks at the probe time
> > > instead of .preenable, I have tried it before but I think due to some
> > > issues(don't remember it now maybe on the platform side)  I kept it at the
> > > .preenable level.
> > > 
> > > But you are right, that it will be nice to move it at the probe level
> > > instead. I will try again and test if it works and would move it at the
> > > probe level. Regarding the unregistering of the notifier, is it required at
> > > the remove of iio driver or scmi driver will take care of it?
> > > Because if I add unregister at the iio driver remove level, I would have to
> > > iterate all the sensors again and unregister them.  
> > 
> > Yes you are right if you move callbacks registration once for all to the
> > .probe step you'll have to unregister them all in a .remove.
> > 
> > BUT I think instead you should stick with your current solution given
> > it's working fine anyway and it's supported by the notification
> > framework and also for another reason I'm going to explain down below
> > (which is also the reason why I asked you this at first :D)
> > 
> > As you may remember I'm refactoring all the SCMI internals in a separate
> > series to ease modularization and vendor protocols support, and that will
> > lead also to some changes in the SCMI driver interface that you use:
> > amongst other things one interesting addition will be a new devres managed
> > notification registration method, something like:
> > 
> > handle->notify_ops->devm_register_notifier(sdev, ...);
> > 
> > With such method you could just move your registration to the .probe
> > step and just forget about it, without the need to add any unregistration
> > in the .remove step, since the core will take care to remove all the
> > callbacks at driver unloading time.
> > 
> > Now, this series, which is here if you want to have a look:
> > 
> > https://lore.kernel.org/linux-arm-kernel/20210202221555.41167-1-cristian.marussi@arm.com/
> > 
> > is already taking care to port any existent SCMI driver to the new interface,
> > so when your IIODEV SCMI driver will be finally queued somewhere for merge, I
> > can in turn rebase my series on yours and take care to port your driver too to
> > the new interface applying the changes above in the context of my series.
> > (and ask you to review of course :D)
> 
> I'm guessing you probably want this driver in an immutable branch then
> rather than having to wait another cycle for it to tick through to a
> a sensible upstream?
> 

Hi Jonathan

the above series (still pending a final review from Sudeep) is targeted
at 5.13 at this point and usually it'd be queued via Sudeep for-next/scmi
which in turn goes via the ARM soc branch.

Having said that, I'm not really familiar enough with this sort of clashes
to know how they should be properly handled, so I'll stick to what you
and Sudeep would think it's better :D (..and I'm pinging him to have a say)

Thanks

Cristian

> Jonathan
> 
> > 
> > I'm saying that is better if you keep your series as it is for now
> > (old interface + .preenable/.postdisable regs/unregs) because, as said,
> > with the new interface the devm_ methods will ease the registration
> > @probe time, and also especially because the new interface is not (and
> > most probably won't) be part of the v5.4 backport that you are testing
> > against: so if you stick with your current solution you'll have a
> > working patch easily backportable now, and once queued I'll port it to
> > the interface using devm_ (so simplifying it)
> > 
> > In this context, it would be indeed important to know if in general moving
> > registration to the probe phase (which should be fine by the spec) poses
> > any kind of problem. (and that's reason why asked it)
> > 
> > Hope to have been clear despite the flood of words :D
> > 
> > Thanks
> > 
> > Cristian
> > 
> > > 
> > > Thanks,
> > > Jyoti
> > > 
> > > On Tue, Feb 9, 2021 at 3:56 AM Cristian Marussi <cristian.marussi@arm.com>
> > > wrote:
> > >   
> > > > Hi Jyoti
> > > >
> > > > some minor things down below.
> > > >
> > > > Other than that, FWIW about the SCMI side of this:
> > > >
> > > > Reviewed-by: Cristian Marussi <cristian.marussi@arm.com>
> > > >
> > > > Thanks
> > > >
> > > > Cristian
> > > >
> > > > On Mon, Feb 08, 2021 at 09:19:18PM +0000, Jyoti Bhayana 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
> > > > >
> > > > > Signed-off-by: Jyoti Bhayana <jbhayana@google.com>
> > > > > ---
> > > > >  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 | 673 +++++++++++++++++++++
> > > > >  7 files changed, 705 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 <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/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..093b1fc24e27
> > > > > --- /dev/null
> > > > > +++ b/drivers/iio/common/scmi_sensors/scmi_iio.c
> > > > > @@ -0,0 +1,673 @@
> > > > > +// 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,
> > > > > +};  
> > > >
> > > > This is just a question, I'm not suggesting to change anything here at
> > > > this point to be clear, since it works just fine as it is.
> > > >
> > > > Following up a previous email, given these are called on enable/disable
> > > > by sysfs, is there a specific reason why you configure here, inside
> > > > these ops, also timestamping and callbacks  i.e. each time the sensor is
> > > > turned on/off by sysfs ? ... instead of just, as an example, enabling
> > > > in _preenable the sensor while registering callbacks and enabling
> > > > timestamping once for all earlier during probe phase ?
> > > > (likewise for _postdisable -> remove)
> > > >
> > > > AFAIU the spec says notifications are emitted for sensors which has
> > > > requested them (via SENSOR_CONTINUOUS_UPDATE_NOTIFY) BUT only if the
> > > > sensor is enabled as a whole (via proper CONFIG_SET as you do), so
> > > > that enabling/disabling the sensor as a whole should result in starting/
> > > > stopping the notification flow without the need of unregistering the
> > > > callbacks everytime. (same goes with the timestamping)
> > > >
> > > > In other words, I would expect the sensor to maintain its state (on the
> > > > platform side) even when going through enable/disable cycles, so that
> > > > it 'remembers' that timestamping/notifications were enabled across an
> > > > on/off.
> > > >
> > > > This would reduce the number of SCMI messages exchanges between the
> > > > kernel and the platform and should be supported by both, but as said,
> > > > it's more of a question for the future, not necessarily for this series.
> > > >  
> > > > > +  
> > > >
> > > > [snip]
> > > >  
> > > > > +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;
> > > > > +}
> > > > > +
> > > > > +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);
> > > > > +
> > > > > +     scmi_iio_set_sampling_freq_avail(iiodev);  
> > > >
> > > > You don't check this for retval, and it could fail with -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 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;
> > > > > +}
> > > > > +  
> > > >  
> 

^ permalink raw reply	[flat|nested] 13+ messages in thread
* Re: [PATCH v5 1/1] iio/scmi: Adding support for IIO SCMI Based Sensors
  2021-02-15  9:25           ` Cristian Marussi
@ 2021-02-15 11:07             ` Jonathan Cameron
  2021-02-15 11:17               ` Sudeep Holla
  2021-02-15 14:47               ` Cristian Marussi
  0 siblings, 2 replies; 13+ messages in thread
From: Jonathan Cameron @ 2021-02-15 11:07 UTC (permalink / raw)
  To: Cristian Marussi
  Cc: Jyoti Bhayana, Hartmut Knaack, Lars-Peter Clausen,
	Peter Meerwald-Stadler, Mauro Carvalho Chehab, David S. Miller,
	Rob Herring, Lukas Bulwahn, linux-kernel, linux-iio,
	Sudeep Holla, Enrico Granata, Mikhail Golubev, Igor Skalkin,
	Peter Hilber, Ankit Arora

On Mon, 15 Feb 2021 09:25:26 +0000
Cristian Marussi <cristian.marussi@arm.com> wrote:

> On Fri, Feb 12, 2021 at 07:18:58PM +0000, Jonathan Cameron wrote:
> > On Wed, 10 Feb 2021 21:46:19 +0000
> > Cristian Marussi <cristian.marussi@arm.com> wrote:
> >   
> > > Hi Jyoti,
> > > 
> > > On Wed, Feb 10, 2021 at 11:19:35AM -0800, Jyoti Bhayana wrote:  
> > > > Hi Cristian,
> > > > 
> > > > Thanks for the feedback. Regarding registering callbacks at the probe time
> > > > instead of .preenable, I have tried it before but I think due to some
> > > > issues(don't remember it now maybe on the platform side)  I kept it at the
> > > > .preenable level.
> > > > 
> > > > But you are right, that it will be nice to move it at the probe level
> > > > instead. I will try again and test if it works and would move it at the
> > > > probe level. Regarding the unregistering of the notifier, is it required at
> > > > the remove of iio driver or scmi driver will take care of it?
> > > > Because if I add unregister at the iio driver remove level, I would have to
> > > > iterate all the sensors again and unregister them.    
> > > 
> > > Yes you are right if you move callbacks registration once for all to the
> > > .probe step you'll have to unregister them all in a .remove.
> > > 
> > > BUT I think instead you should stick with your current solution given
> > > it's working fine anyway and it's supported by the notification
> > > framework and also for another reason I'm going to explain down below
> > > (which is also the reason why I asked you this at first :D)
> > > 
> > > As you may remember I'm refactoring all the SCMI internals in a separate
> > > series to ease modularization and vendor protocols support, and that will
> > > lead also to some changes in the SCMI driver interface that you use:
> > > amongst other things one interesting addition will be a new devres managed
> > > notification registration method, something like:
> > > 
> > > handle->notify_ops->devm_register_notifier(sdev, ...);
> > > 
> > > With such method you could just move your registration to the .probe
> > > step and just forget about it, without the need to add any unregistration
> > > in the .remove step, since the core will take care to remove all the
> > > callbacks at driver unloading time.
> > > 
> > > Now, this series, which is here if you want to have a look:
> > > 
> > > https://lore.kernel.org/linux-arm-kernel/20210202221555.41167-1-cristian.marussi@arm.com/
> > > 
> > > is already taking care to port any existent SCMI driver to the new interface,
> > > so when your IIODEV SCMI driver will be finally queued somewhere for merge, I
> > > can in turn rebase my series on yours and take care to port your driver too to
> > > the new interface applying the changes above in the context of my series.
> > > (and ask you to review of course :D)  
> > 
> > I'm guessing you probably want this driver in an immutable branch then
> > rather than having to wait another cycle for it to tick through to a
> > a sensible upstream?
> >   
> 
> Hi Jonathan
> 
> the above series (still pending a final review from Sudeep) is targeted
> at 5.13 at this point and usually it'd be queued via Sudeep for-next/scmi
> which in turn goes via the ARM soc branch.
> 
> Having said that, I'm not really familiar enough with this sort of clashes
> to know how they should be properly handled, so I'll stick to what you
> and Sudeep would think it's better :D (..and I'm pinging him to have a say)
> 
> Thanks
> 
> Cristian
Hi Cristian,

So this driver will also be 5.13 material now (merge window for IIO effectively
closes 1-2 weeks before Linus opens the main one).

The way we normally handle cases like this where we likely to have dependencies
on a patch set from two separate directions is to do what is known as an
immutable branch.  This is a branch that would probably be based on 5.12-rc1
containing just this driver.

Then both trees, in this case IIO and scmi merge that branch.  The magic
of git then means that when Linus gets the eventual pull requests for
the two trees, the git IDs and content will be the same and the history
of that particular set of files will be cleanly maintained.

This happens quite a lot for certain parts of the kernel because there are
a lot of cross dependencies.

@Sudeep, that work for you?  Have to wait for 5.12-rc1 though to give
us a sensible base.

Jonathan

> 
> > Jonathan
> >   
> > > 
> > > I'm saying that is better if you keep your series as it is for now
> > > (old interface + .preenable/.postdisable regs/unregs) because, as said,
> > > with the new interface the devm_ methods will ease the registration
> > > @probe time, and also especially because the new interface is not (and
> > > most probably won't) be part of the v5.4 backport that you are testing
> > > against: so if you stick with your current solution you'll have a
> > > working patch easily backportable now, and once queued I'll port it to
> > > the interface using devm_ (so simplifying it)
> > > 
> > > In this context, it would be indeed important to know if in general moving
> > > registration to the probe phase (which should be fine by the spec) poses
> > > any kind of problem. (and that's reason why asked it)
> > > 
> > > Hope to have been clear despite the flood of words :D
> > > 
> > > Thanks
> > > 
> > > Cristian
> > >   
> > > > 
> > > > Thanks,
> > > > Jyoti
> > > > 
> > > > On Tue, Feb 9, 2021 at 3:56 AM Cristian Marussi <cristian.marussi@arm.com>
> > > > wrote:
> > > >     
> > > > > Hi Jyoti
> > > > >
> > > > > some minor things down below.
> > > > >
> > > > > Other than that, FWIW about the SCMI side of this:
> > > > >
> > > > > Reviewed-by: Cristian Marussi <cristian.marussi@arm.com>
> > > > >
> > > > > Thanks
> > > > >
> > > > > Cristian
> > > > >
> > > > > On Mon, Feb 08, 2021 at 09:19:18PM +0000, Jyoti Bhayana 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
> > > > > >
> > > > > > Signed-off-by: Jyoti Bhayana <jbhayana@google.com>
> > > > > > ---
> > > > > >  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 | 673 +++++++++++++++++++++
> > > > > >  7 files changed, 705 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 <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/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..093b1fc24e27
> > > > > > --- /dev/null
> > > > > > +++ b/drivers/iio/common/scmi_sensors/scmi_iio.c
> > > > > > @@ -0,0 +1,673 @@
> > > > > > +// 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,
> > > > > > +};    
> > > > >
> > > > > This is just a question, I'm not suggesting to change anything here at
> > > > > this point to be clear, since it works just fine as it is.
> > > > >
> > > > > Following up a previous email, given these are called on enable/disable
> > > > > by sysfs, is there a specific reason why you configure here, inside
> > > > > these ops, also timestamping and callbacks  i.e. each time the sensor is
> > > > > turned on/off by sysfs ? ... instead of just, as an example, enabling
> > > > > in _preenable the sensor while registering callbacks and enabling
> > > > > timestamping once for all earlier during probe phase ?
> > > > > (likewise for _postdisable -> remove)
> > > > >
> > > > > AFAIU the spec says notifications are emitted for sensors which has
> > > > > requested them (via SENSOR_CONTINUOUS_UPDATE_NOTIFY) BUT only if the
> > > > > sensor is enabled as a whole (via proper CONFIG_SET as you do), so
> > > > > that enabling/disabling the sensor as a whole should result in starting/
> > > > > stopping the notification flow without the need of unregistering the
> > > > > callbacks everytime. (same goes with the timestamping)
> > > > >
> > > > > In other words, I would expect the sensor to maintain its state (on the
> > > > > platform side) even when going through enable/disable cycles, so that
> > > > > it 'remembers' that timestamping/notifications were enabled across an
> > > > > on/off.
> > > > >
> > > > > This would reduce the number of SCMI messages exchanges between the
> > > > > kernel and the platform and should be supported by both, but as said,
> > > > > it's more of a question for the future, not necessarily for this series.
> > > > >    
> > > > > > +    
> > > > >
> > > > > [snip]
> > > > >    
> > > > > > +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;
> > > > > > +}
> > > > > > +
> > > > > > +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);
> > > > > > +
> > > > > > +     scmi_iio_set_sampling_freq_avail(iiodev);    
> > > > >
> > > > > You don't check this for retval, and it could fail with -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 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;
> > > > > > +}
> > > > > > +    
> > > > >    
> >   


^ permalink raw reply	[flat|nested] 13+ messages in thread
* Re: [PATCH v5 1/1] iio/scmi: Adding support for IIO SCMI Based Sensors
  2021-02-15 11:07             ` Jonathan Cameron
@ 2021-02-15 11:17               ` Sudeep Holla
  2021-02-15 14:47               ` Cristian Marussi
  1 sibling, 0 replies; 13+ messages in thread
From: Sudeep Holla @ 2021-02-15 11:17 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: Cristian Marussi, Jyoti Bhayana, Sudeep Holla, Hartmut Knaack,
	Lars-Peter Clausen, Peter Meerwald-Stadler,
	Mauro Carvalho Chehab, David S. Miller, Rob Herring,
	Lukas Bulwahn, linux-kernel, linux-iio, Enrico Granata,
	Mikhail Golubev, Igor Skalkin, Peter Hilber, Ankit Arora

On Mon, Feb 15, 2021 at 11:07:56AM +0000, Jonathan Cameron wrote:
> Hi Cristian,
>
> So this driver will also be 5.13 material now (merge window for IIO effectively
> closes 1-2 weeks before Linus opens the main one).
>

I guessed so.

> The way we normally handle cases like this where we likely to have dependencies
> on a patch set from two separate directions is to do what is known as an
> immutable branch.  This is a branch that would probably be based on 5.12-rc1
> containing just this driver.
>

Make sense.

> Then both trees, in this case IIO and scmi merge that branch.  The magic
> of git then means that when Linus gets the eventual pull requests for
> the two trees, the git IDs and content will be the same and the history
> of that particular set of files will be cleanly maintained.
>
> This happens quite a lot for certain parts of the kernel because there are
> a lot of cross dependencies.
>
> @Sudeep, that work for you?  Have to wait for 5.12-rc1 though to give
> us a sensible base.
>

Since this is just one patch, I can pull in the immutable branch if you
could share and then Cristian can make appropriate changes needed for his
series on top of that unless you want to merge 30+ patches that I might
have with Cristain series 😉.

In short I am happy to pull in your immutable branch and I agree 5.12-rc1
is sensible base.

--
Regards,
Sudeep

^ permalink raw reply	[flat|nested] 13+ messages in thread
* Re: [PATCH v5 1/1] iio/scmi: Adding support for IIO SCMI Based Sensors
  2021-02-15 11:07             ` Jonathan Cameron
  2021-02-15 11:17               ` Sudeep Holla
@ 2021-02-15 14:47               ` Cristian Marussi
       [not found]                 ` <CA+=V6c3xmidg9BCZuEoigwcbDwW-sTCZWnVCkPhj4KMCzYCehg@mail.gmail.com>
  1 sibling, 1 reply; 13+ messages in thread
From: Cristian Marussi @ 2021-02-15 14:47 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: Jyoti Bhayana, Hartmut Knaack, Lars-Peter Clausen,
	Peter Meerwald-Stadler, Mauro Carvalho Chehab, David S. Miller,
	Rob Herring, Lukas Bulwahn, linux-kernel, linux-iio,
	Sudeep Holla, Enrico Granata, Mikhail Golubev, Igor Skalkin,
	Peter Hilber, Ankit Arora

On Mon, Feb 15, 2021 at 11:07:56AM +0000, Jonathan Cameron wrote:
> On Mon, 15 Feb 2021 09:25:26 +0000
> Cristian Marussi <cristian.marussi@arm.com> wrote:
> 
> > On Fri, Feb 12, 2021 at 07:18:58PM +0000, Jonathan Cameron wrote:
> > > On Wed, 10 Feb 2021 21:46:19 +0000
> > > Cristian Marussi <cristian.marussi@arm.com> wrote:
> > >   
> > > > Hi Jyoti,
> > > > 
> > > > On Wed, Feb 10, 2021 at 11:19:35AM -0800, Jyoti Bhayana wrote:  
> > > > > Hi Cristian,
> > > > > 
> > > > > Thanks for the feedback. Regarding registering callbacks at the probe time
> > > > > instead of .preenable, I have tried it before but I think due to some
> > > > > issues(don't remember it now maybe on the platform side)  I kept it at the
> > > > > .preenable level.
> > > > > 
> > > > > But you are right, that it will be nice to move it at the probe level
> > > > > instead. I will try again and test if it works and would move it at the
> > > > > probe level. Regarding the unregistering of the notifier, is it required at
> > > > > the remove of iio driver or scmi driver will take care of it?
> > > > > Because if I add unregister at the iio driver remove level, I would have to
> > > > > iterate all the sensors again and unregister them.    
> > > > 
> > > > Yes you are right if you move callbacks registration once for all to the
> > > > .probe step you'll have to unregister them all in a .remove.
> > > > 
> > > > BUT I think instead you should stick with your current solution given
> > > > it's working fine anyway and it's supported by the notification
> > > > framework and also for another reason I'm going to explain down below
> > > > (which is also the reason why I asked you this at first :D)
> > > > 
> > > > As you may remember I'm refactoring all the SCMI internals in a separate
> > > > series to ease modularization and vendor protocols support, and that will
> > > > lead also to some changes in the SCMI driver interface that you use:
> > > > amongst other things one interesting addition will be a new devres managed
> > > > notification registration method, something like:
> > > > 
> > > > handle->notify_ops->devm_register_notifier(sdev, ...);
> > > > 
> > > > With such method you could just move your registration to the .probe
> > > > step and just forget about it, without the need to add any unregistration
> > > > in the .remove step, since the core will take care to remove all the
> > > > callbacks at driver unloading time.
> > > > 
> > > > Now, this series, which is here if you want to have a look:
> > > > 
> > > > https://lore.kernel.org/linux-arm-kernel/20210202221555.41167-1-cristian.marussi@arm.com/
> > > > 
> > > > is already taking care to port any existent SCMI driver to the new interface,
> > > > so when your IIODEV SCMI driver will be finally queued somewhere for merge, I
> > > > can in turn rebase my series on yours and take care to port your driver too to
> > > > the new interface applying the changes above in the context of my series.
> > > > (and ask you to review of course :D)  
> > > 
> > > I'm guessing you probably want this driver in an immutable branch then
> > > rather than having to wait another cycle for it to tick through to a
> > > a sensible upstream?
> > >   
> > 
> > Hi Jonathan
> > 
> > the above series (still pending a final review from Sudeep) is targeted
> > at 5.13 at this point and usually it'd be queued via Sudeep for-next/scmi
> > which in turn goes via the ARM soc branch.
> > 
> > Having said that, I'm not really familiar enough with this sort of clashes
> > to know how they should be properly handled, so I'll stick to what you
> > and Sudeep would think it's better :D (..and I'm pinging him to have a say)
> > 
> > Thanks
> > 
> > Cristian
> Hi Cristian,
> 
> So this driver will also be 5.13 material now (merge window for IIO effectively
> closes 1-2 weeks before Linus opens the main one).
> 
> The way we normally handle cases like this where we likely to have dependencies
> on a patch set from two separate directions is to do what is known as an
> immutable branch.  This is a branch that would probably be based on 5.12-rc1
> containing just this driver.
> 
> Then both trees, in this case IIO and scmi merge that branch.  The magic
> of git then means that when Linus gets the eventual pull requests for
> the two trees, the git IDs and content will be the same and the history
> of that particular set of files will be cleanly maintained.
> 
> This happens quite a lot for certain parts of the kernel because there are
> a lot of cross dependencies.
> 
Hi Jonathan

thanks for clarifying.

Cristian

> @Sudeep, that work for you?  Have to wait for 5.12-rc1 though to give
> us a sensible base.
> 
> Jonathan
> 
> > 
> > > Jonathan
> > >   
> > > > 
> > > > I'm saying that is better if you keep your series as it is for now
> > > > (old interface + .preenable/.postdisable regs/unregs) because, as said,
> > > > with the new interface the devm_ methods will ease the registration
> > > > @probe time, and also especially because the new interface is not (and
> > > > most probably won't) be part of the v5.4 backport that you are testing
> > > > against: so if you stick with your current solution you'll have a
> > > > working patch easily backportable now, and once queued I'll port it to
> > > > the interface using devm_ (so simplifying it)
> > > > 
> > > > In this context, it would be indeed important to know if in general moving
> > > > registration to the probe phase (which should be fine by the spec) poses
> > > > any kind of problem. (and that's reason why asked it)
> > > > 
> > > > Hope to have been clear despite the flood of words :D
> > > > 
> > > > Thanks
> > > > 
> > > > Cristian
> > > >   
> > > > > 
> > > > > Thanks,
> > > > > Jyoti
> > > > > 
> > > > > On Tue, Feb 9, 2021 at 3:56 AM Cristian Marussi <cristian.marussi@arm.com>
> > > > > wrote:
> > > > >     
> > > > > > Hi Jyoti
> > > > > >
> > > > > > some minor things down below.
> > > > > >
> > > > > > Other than that, FWIW about the SCMI side of this:
> > > > > >
> > > > > > Reviewed-by: Cristian Marussi <cristian.marussi@arm.com>
> > > > > >
> > > > > > Thanks
> > > > > >
> > > > > > Cristian
> > > > > >
> > > > > > On Mon, Feb 08, 2021 at 09:19:18PM +0000, Jyoti Bhayana 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
> > > > > > >
> > > > > > > Signed-off-by: Jyoti Bhayana <jbhayana@google.com>
> > > > > > > ---
> > > > > > >  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 | 673 +++++++++++++++++++++
> > > > > > >  7 files changed, 705 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 <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/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..093b1fc24e27
> > > > > > > --- /dev/null
> > > > > > > +++ b/drivers/iio/common/scmi_sensors/scmi_iio.c
> > > > > > > @@ -0,0 +1,673 @@
> > > > > > > +// 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,
> > > > > > > +};    
> > > > > >
> > > > > > This is just a question, I'm not suggesting to change anything here at
> > > > > > this point to be clear, since it works just fine as it is.
> > > > > >
> > > > > > Following up a previous email, given these are called on enable/disable
> > > > > > by sysfs, is there a specific reason why you configure here, inside
> > > > > > these ops, also timestamping and callbacks  i.e. each time the sensor is
> > > > > > turned on/off by sysfs ? ... instead of just, as an example, enabling
> > > > > > in _preenable the sensor while registering callbacks and enabling
> > > > > > timestamping once for all earlier during probe phase ?
> > > > > > (likewise for _postdisable -> remove)
> > > > > >
> > > > > > AFAIU the spec says notifications are emitted for sensors which has
> > > > > > requested them (via SENSOR_CONTINUOUS_UPDATE_NOTIFY) BUT only if the
> > > > > > sensor is enabled as a whole (via proper CONFIG_SET as you do), so
> > > > > > that enabling/disabling the sensor as a whole should result in starting/
> > > > > > stopping the notification flow without the need of unregistering the
> > > > > > callbacks everytime. (same goes with the timestamping)
> > > > > >
> > > > > > In other words, I would expect the sensor to maintain its state (on the
> > > > > > platform side) even when going through enable/disable cycles, so that
> > > > > > it 'remembers' that timestamping/notifications were enabled across an
> > > > > > on/off.
> > > > > >
> > > > > > This would reduce the number of SCMI messages exchanges between the
> > > > > > kernel and the platform and should be supported by both, but as said,
> > > > > > it's more of a question for the future, not necessarily for this series.
> > > > > >    
> > > > > > > +    
> > > > > >
> > > > > > [snip]
> > > > > >    
> > > > > > > +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;
> > > > > > > +}
> > > > > > > +
> > > > > > > +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);
> > > > > > > +
> > > > > > > +     scmi_iio_set_sampling_freq_avail(iiodev);    
> > > > > >
> > > > > > You don't check this for retval, and it could fail with -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 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;
> > > > > > > +}
> > > > > > > +    
> > > > > >    
> > >   
> 

^ permalink raw reply	[flat|nested] 13+ messages in thread
* Re: [PATCH v5 1/1] iio/scmi: Adding support for IIO SCMI Based Sensors
       [not found]                 ` <CA+=V6c3xmidg9BCZuEoigwcbDwW-sTCZWnVCkPhj4KMCzYCehg@mail.gmail.com>
@ 2021-02-16 10:20                   ` Cristian Marussi
  2021-02-16 20:05                     ` Jonathan Cameron
  0 siblings, 1 reply; 13+ messages in thread
From: Cristian Marussi @ 2021-02-16 10:20 UTC (permalink / raw)
  To: Jyoti Bhayana
  Cc: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
	Peter Meerwald-Stadler, Mauro Carvalho Chehab, David S. Miller,
	Rob Herring, Lukas Bulwahn, linux-kernel, linux-iio,
	Sudeep Holla, Enrico Granata, Mikhail Golubev, Igor Skalkin,
	Peter Hilber, Ankit Arora

Hi Jyoti,

On Mon, Feb 15, 2021 at 04:14:57PM -0800, Jyoti Bhayana wrote:
> Hi Jonathan/Cristian,
> 
> I have uploaded v6 of the IIO SCMI patch. Not sure if you got a chance to
> review that version and if any further changes are needed.

Seen that, for the SCMI part seemed fine to me, I'll just have still to
reply.

> My IIO SCMI patch is independent of Cristian series mentioned at
> https://lore.kernel.org/linux-arm-kernel/20210202221555.41167-1-cristian.marussi@arm.com/
> right?

Yes, that's true, your patch is independent and goes in as it is now, and I'll
rebase my series on yours on a dedicated branch and make all the needed
(small) interface changes.

> and it can be merged without the Cristian Series right? Are there plans to
> merge my v6 of the IIO SCMI patch and Cristian latest series in the same
> linux tree version?

Yes absolutely, your driver can be merged as it is without my series, but due
to the timing they ended up targeting the same Linux version 5.13 at the
end, so they will be merged both in v5.13, yours first as it is now, mine
immediately rebased on top of it aftewards: the end result will be both
series in the same v5.13 but as distinct patches. (so you can still backport
yours on v5.4 as you're doing now.)

Thanks

Cristian

> 
> Thanks,
> Jyoti
> 
> On Mon, Feb 15, 2021 at 6:48 AM Cristian Marussi <cristian.marussi@arm.com>
> wrote:
> 
> > On Mon, Feb 15, 2021 at 11:07:56AM +0000, Jonathan Cameron wrote:
> > > On Mon, 15 Feb 2021 09:25:26 +0000
> > > Cristian Marussi <cristian.marussi@arm.com> wrote:
> > >
> > > > On Fri, Feb 12, 2021 at 07:18:58PM +0000, Jonathan Cameron wrote:
> > > > > On Wed, 10 Feb 2021 21:46:19 +0000
> > > > > Cristian Marussi <cristian.marussi@arm.com> wrote:
> > > > >
> > > > > > Hi Jyoti,
> > > > > >
> > > > > > On Wed, Feb 10, 2021 at 11:19:35AM -0800, Jyoti Bhayana wrote:
> > > > > > > Hi Cristian,
> > > > > > >
> > > > > > > Thanks for the feedback. Regarding registering callbacks at the
> > probe time
> > > > > > > instead of .preenable, I have tried it before but I think due to
> > some
> > > > > > > issues(don't remember it now maybe on the platform side)  I kept
> > it at the
> > > > > > > .preenable level.
> > > > > > >
> > > > > > > But you are right, that it will be nice to move it at the probe
> > level
> > > > > > > instead. I will try again and test if it works and would move it
> > at the
> > > > > > > probe level. Regarding the unregistering of the notifier, is it
> > required at
> > > > > > > the remove of iio driver or scmi driver will take care of it?
> > > > > > > Because if I add unregister at the iio driver remove level, I
> > would have to
> > > > > > > iterate all the sensors again and unregister them.
> > > > > >
> > > > > > Yes you are right if you move callbacks registration once for all
> > to the
> > > > > > .probe step you'll have to unregister them all in a .remove.
> > > > > >
> > > > > > BUT I think instead you should stick with your current solution
> > given
> > > > > > it's working fine anyway and it's supported by the notification
> > > > > > framework and also for another reason I'm going to explain down
> > below
> > > > > > (which is also the reason why I asked you this at first :D)
> > > > > >
> > > > > > As you may remember I'm refactoring all the SCMI internals in a
> > separate
> > > > > > series to ease modularization and vendor protocols support, and
> > that will
> > > > > > lead also to some changes in the SCMI driver interface that you
> > use:
> > > > > > amongst other things one interesting addition will be a new devres
> > managed
> > > > > > notification registration method, something like:
> > > > > >
> > > > > > handle->notify_ops->devm_register_notifier(sdev, ...);
> > > > > >
> > > > > > With such method you could just move your registration to the
> > .probe
> > > > > > step and just forget about it, without the need to add any
> > unregistration
> > > > > > in the .remove step, since the core will take care to remove all
> > the
> > > > > > callbacks at driver unloading time.
> > > > > >
> > > > > > Now, this series, which is here if you want to have a look:
> > > > > >
> > > > > >
> > https://lore.kernel.org/linux-arm-kernel/20210202221555.41167-1-cristian.marussi@arm.com/
> > > > > >
> > > > > > is already taking care to port any existent SCMI driver to the new
> > interface,
> > > > > > so when your IIODEV SCMI driver will be finally queued somewhere
> > for merge, I
> > > > > > can in turn rebase my series on yours and take care to port your
> > driver too to
> > > > > > the new interface applying the changes above in the context of my
> > series.
> > > > > > (and ask you to review of course :D)
> > > > >
> > > > > I'm guessing you probably want this driver in an immutable branch
> > then
> > > > > rather than having to wait another cycle for it to tick through to a
> > > > > a sensible upstream?
> > > > >
> > > >
> > > > Hi Jonathan
> > > >
> > > > the above series (still pending a final review from Sudeep) is targeted
> > > > at 5.13 at this point and usually it'd be queued via Sudeep
> > for-next/scmi
> > > > which in turn goes via the ARM soc branch.
> > > >
> > > > Having said that, I'm not really familiar enough with this sort of
> > clashes
> > > > to know how they should be properly handled, so I'll stick to what you
> > > > and Sudeep would think it's better :D (..and I'm pinging him to have a
> > say)
> > > >
> > > > Thanks
> > > >
> > > > Cristian
> > > Hi Cristian,
> > >
> > > So this driver will also be 5.13 material now (merge window for IIO
> > effectively
> > > closes 1-2 weeks before Linus opens the main one).
> > >
> > > The way we normally handle cases like this where we likely to have
> > dependencies
> > > on a patch set from two separate directions is to do what is known as an
> > > immutable branch.  This is a branch that would probably be based on
> > 5.12-rc1
> > > containing just this driver.
> > >
> > > Then both trees, in this case IIO and scmi merge that branch.  The magic
> > > of git then means that when Linus gets the eventual pull requests for
> > > the two trees, the git IDs and content will be the same and the history
> > > of that particular set of files will be cleanly maintained.
> > >
> > > This happens quite a lot for certain parts of the kernel because there
> > are
> > > a lot of cross dependencies.
> > >
> > Hi Jonathan
> >
> > thanks for clarifying.
> >
> > Cristian
> >
> > > @Sudeep, that work for you?  Have to wait for 5.12-rc1 though to give
> > > us a sensible base.
> > >
> > > Jonathan
> > >
> > > >
> > > > > Jonathan
> > > > >
> > > > > >
> > > > > > I'm saying that is better if you keep your series as it is for now
> > > > > > (old interface + .preenable/.postdisable regs/unregs) because, as
> > said,
> > > > > > with the new interface the devm_ methods will ease the registration
> > > > > > @probe time, and also especially because the new interface is not
> > (and
> > > > > > most probably won't) be part of the v5.4 backport that you are
> > testing
> > > > > > against: so if you stick with your current solution you'll have a
> > > > > > working patch easily backportable now, and once queued I'll port
> > it to
> > > > > > the interface using devm_ (so simplifying it)
> > > > > >
> > > > > > In this context, it would be indeed important to know if in
> > general moving
> > > > > > registration to the probe phase (which should be fine by the spec)
> > poses
> > > > > > any kind of problem. (and that's reason why asked it)
> > > > > >
> > > > > > Hope to have been clear despite the flood of words :D
> > > > > >
> > > > > > Thanks
> > > > > >
> > > > > > Cristian
> > > > > >
> > > > > > >
> > > > > > > Thanks,
> > > > > > > Jyoti
> > > > > > >
> > > > > > > On Tue, Feb 9, 2021 at 3:56 AM Cristian Marussi <
> > cristian.marussi@arm.com>
> > > > > > > wrote:
> > > > > > >
> > > > > > > > Hi Jyoti
> > > > > > > >
> > > > > > > > some minor things down below.
> > > > > > > >
> > > > > > > > Other than that, FWIW about the SCMI side of this:
> > > > > > > >
> > > > > > > > Reviewed-by: Cristian Marussi <cristian.marussi@arm.com>
> > > > > > > >
> > > > > > > > Thanks
> > > > > > > >
> > > > > > > > Cristian
> > > > > > > >
> > > > > > > > On Mon, Feb 08, 2021 at 09:19:18PM +0000, Jyoti Bhayana
> > 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
> > > > > > > > >
> > > > > > > > > Signed-off-by: Jyoti Bhayana <jbhayana@google.com>
> > > > > > > > > ---
> > > > > > > > >  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 | 673
> > +++++++++++++++++++++
> > > > > > > > >  7 files changed, 705 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 <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/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..093b1fc24e27
> > > > > > > > > --- /dev/null
> > > > > > > > > +++ b/drivers/iio/common/scmi_sensors/scmi_iio.c
> > > > > > > > > @@ -0,0 +1,673 @@
> > > > > > > > > +// 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,
> > > > > > > > > +};
> > > > > > > >
> > > > > > > > This is just a question, I'm not suggesting to change anything
> > here at
> > > > > > > > this point to be clear, since it works just fine as it is.
> > > > > > > >
> > > > > > > > Following up a previous email, given these are called on
> > enable/disable
> > > > > > > > by sysfs, is there a specific reason why you configure here,
> > inside
> > > > > > > > these ops, also timestamping and callbacks  i.e. each time the
> > sensor is
> > > > > > > > turned on/off by sysfs ? ... instead of just, as an example,
> > enabling
> > > > > > > > in _preenable the sensor while registering callbacks and
> > enabling
> > > > > > > > timestamping once for all earlier during probe phase ?
> > > > > > > > (likewise for _postdisable -> remove)
> > > > > > > >
> > > > > > > > AFAIU the spec says notifications are emitted for sensors
> > which has
> > > > > > > > requested them (via SENSOR_CONTINUOUS_UPDATE_NOTIFY) BUT only
> > if the
> > > > > > > > sensor is enabled as a whole (via proper CONFIG_SET as you
> > do), so
> > > > > > > > that enabling/disabling the sensor as a whole should result in
> > starting/
> > > > > > > > stopping the notification flow without the need of
> > unregistering the
> > > > > > > > callbacks everytime. (same goes with the timestamping)
> > > > > > > >
> > > > > > > > In other words, I would expect the sensor to maintain its
> > state (on the
> > > > > > > > platform side) even when going through enable/disable cycles,
> > so that
> > > > > > > > it 'remembers' that timestamping/notifications were enabled
> > across an
> > > > > > > > on/off.
> > > > > > > >
> > > > > > > > This would reduce the number of SCMI messages exchanges
> > between the
> > > > > > > > kernel and the platform and should be supported by both, but
> > as said,
> > > > > > > > it's more of a question for the future, not necessarily for
> > this series.
> > > > > > > >
> > > > > > > > > +
> > > > > > > >
> > > > > > > > [snip]
> > > > > > > >
> > > > > > > > > +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;
> > > > > > > > > +}
> > > > > > > > > +
> > > > > > > > > +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);
> > > > > > > > > +
> > > > > > > > > +     scmi_iio_set_sampling_freq_avail(iiodev);
> > > > > > > >
> > > > > > > > You don't check this for retval, and it could fail with
> > -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 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;
> > > > > > > > > +}
> > > > > > > > > +
> > > > > > > >
> > > > >
> > >
> >

^ permalink raw reply	[flat|nested] 13+ messages in thread
* Re: [PATCH v5 1/1] iio/scmi: Adding support for IIO SCMI Based Sensors
  2021-02-16 10:20                   ` Cristian Marussi
@ 2021-02-16 20:05                     ` Jonathan Cameron
  0 siblings, 0 replies; 13+ messages in thread
From: Jonathan Cameron @ 2021-02-16 20:05 UTC (permalink / raw)
  To: Cristian Marussi
  Cc: Jyoti Bhayana, Hartmut Knaack, Lars-Peter Clausen,
	Peter Meerwald-Stadler, Mauro Carvalho Chehab, David S. Miller,
	Rob Herring, Lukas Bulwahn, linux-kernel, linux-iio,
	Sudeep Holla, Enrico Granata, Mikhail Golubev, Igor Skalkin,
	Peter Hilber, Ankit Arora

On Tue, 16 Feb 2021 10:20:59 +0000
Cristian Marussi <cristian.marussi@arm.com> wrote:

> Hi Jyoti,
> 
> On Mon, Feb 15, 2021 at 04:14:57PM -0800, Jyoti Bhayana wrote:
> > Hi Jonathan/Cristian,
> > 
> > I have uploaded v6 of the IIO SCMI patch. Not sure if you got a chance to
> > review that version and if any further changes are needed.  
> 
> Seen that, for the SCMI part seemed fine to me, I'll just have still to
> reply.

I'll confess I'm not rushing reviewing this, simply because it's 
proved complex / unusual enough that I want to have another hard look at it
+ we have a bit of time now.  I can't fall back on semi conscious reviewing
for this one ;) 

Apologies for the delay in the meantime!

Jonathan

> 
> > My IIO SCMI patch is independent of Cristian series mentioned at
> > https://lore.kernel.org/linux-arm-kernel/20210202221555.41167-1-cristian.marussi@arm.com/
> > right?  
> 
> Yes, that's true, your patch is independent and goes in as it is now, and I'll
> rebase my series on yours on a dedicated branch and make all the needed
> (small) interface changes.
> 
> > and it can be merged without the Cristian Series right? Are there plans to
> > merge my v6 of the IIO SCMI patch and Cristian latest series in the same
> > linux tree version?  
> 
> Yes absolutely, your driver can be merged as it is without my series, but due
> to the timing they ended up targeting the same Linux version 5.13 at the
> end, so they will be merged both in v5.13, yours first as it is now, mine
> immediately rebased on top of it aftewards: the end result will be both
> series in the same v5.13 but as distinct patches. (so you can still backport
> yours on v5.4 as you're doing now.)
> 
> Thanks
> 
> Cristian
> 
> > 
> > Thanks,
> > Jyoti
> > 
> > On Mon, Feb 15, 2021 at 6:48 AM Cristian Marussi <cristian.marussi@arm.com>
> > wrote:
> >   
> > > On Mon, Feb 15, 2021 at 11:07:56AM +0000, Jonathan Cameron wrote:  
> > > > On Mon, 15 Feb 2021 09:25:26 +0000
> > > > Cristian Marussi <cristian.marussi@arm.com> wrote:
> > > >  
> > > > > On Fri, Feb 12, 2021 at 07:18:58PM +0000, Jonathan Cameron wrote:  
> > > > > > On Wed, 10 Feb 2021 21:46:19 +0000
> > > > > > Cristian Marussi <cristian.marussi@arm.com> wrote:
> > > > > >  
> > > > > > > Hi Jyoti,
> > > > > > >
> > > > > > > On Wed, Feb 10, 2021 at 11:19:35AM -0800, Jyoti Bhayana wrote:  
> > > > > > > > Hi Cristian,
> > > > > > > >
> > > > > > > > Thanks for the feedback. Regarding registering callbacks at the  
> > > probe time  
> > > > > > > > instead of .preenable, I have tried it before but I think due to  
> > > some  
> > > > > > > > issues(don't remember it now maybe on the platform side)  I kept  
> > > it at the  
> > > > > > > > .preenable level.
> > > > > > > >
> > > > > > > > But you are right, that it will be nice to move it at the probe  
> > > level  
> > > > > > > > instead. I will try again and test if it works and would move it  
> > > at the  
> > > > > > > > probe level. Regarding the unregistering of the notifier, is it  
> > > required at  
> > > > > > > > the remove of iio driver or scmi driver will take care of it?
> > > > > > > > Because if I add unregister at the iio driver remove level, I  
> > > would have to  
> > > > > > > > iterate all the sensors again and unregister them.  
> > > > > > >
> > > > > > > Yes you are right if you move callbacks registration once for all  
> > > to the  
> > > > > > > .probe step you'll have to unregister them all in a .remove.
> > > > > > >
> > > > > > > BUT I think instead you should stick with your current solution  
> > > given  
> > > > > > > it's working fine anyway and it's supported by the notification
> > > > > > > framework and also for another reason I'm going to explain down  
> > > below  
> > > > > > > (which is also the reason why I asked you this at first :D)
> > > > > > >
> > > > > > > As you may remember I'm refactoring all the SCMI internals in a  
> > > separate  
> > > > > > > series to ease modularization and vendor protocols support, and  
> > > that will  
> > > > > > > lead also to some changes in the SCMI driver interface that you  
> > > use:  
> > > > > > > amongst other things one interesting addition will be a new devres  
> > > managed  
> > > > > > > notification registration method, something like:
> > > > > > >
> > > > > > > handle->notify_ops->devm_register_notifier(sdev, ...);
> > > > > > >
> > > > > > > With such method you could just move your registration to the  
> > > .probe  
> > > > > > > step and just forget about it, without the need to add any  
> > > unregistration  
> > > > > > > in the .remove step, since the core will take care to remove all  
> > > the  
> > > > > > > callbacks at driver unloading time.
> > > > > > >
> > > > > > > Now, this series, which is here if you want to have a look:
> > > > > > >
> > > > > > >  
> > > https://lore.kernel.org/linux-arm-kernel/20210202221555.41167-1-cristian.marussi@arm.com/  
> > > > > > >
> > > > > > > is already taking care to port any existent SCMI driver to the new  
> > > interface,  
> > > > > > > so when your IIODEV SCMI driver will be finally queued somewhere  
> > > for merge, I  
> > > > > > > can in turn rebase my series on yours and take care to port your  
> > > driver too to  
> > > > > > > the new interface applying the changes above in the context of my  
> > > series.  
> > > > > > > (and ask you to review of course :D)  
> > > > > >
> > > > > > I'm guessing you probably want this driver in an immutable branch  
> > > then  
> > > > > > rather than having to wait another cycle for it to tick through to a
> > > > > > a sensible upstream?
> > > > > >  
> > > > >
> > > > > Hi Jonathan
> > > > >
> > > > > the above series (still pending a final review from Sudeep) is targeted
> > > > > at 5.13 at this point and usually it'd be queued via Sudeep  
> > > for-next/scmi  
> > > > > which in turn goes via the ARM soc branch.
> > > > >
> > > > > Having said that, I'm not really familiar enough with this sort of  
> > > clashes  
> > > > > to know how they should be properly handled, so I'll stick to what you
> > > > > and Sudeep would think it's better :D (..and I'm pinging him to have a  
> > > say)  
> > > > >
> > > > > Thanks
> > > > >
> > > > > Cristian  
> > > > Hi Cristian,
> > > >
> > > > So this driver will also be 5.13 material now (merge window for IIO  
> > > effectively  
> > > > closes 1-2 weeks before Linus opens the main one).
> > > >
> > > > The way we normally handle cases like this where we likely to have  
> > > dependencies  
> > > > on a patch set from two separate directions is to do what is known as an
> > > > immutable branch.  This is a branch that would probably be based on  
> > > 5.12-rc1  
> > > > containing just this driver.
> > > >
> > > > Then both trees, in this case IIO and scmi merge that branch.  The magic
> > > > of git then means that when Linus gets the eventual pull requests for
> > > > the two trees, the git IDs and content will be the same and the history
> > > > of that particular set of files will be cleanly maintained.
> > > >
> > > > This happens quite a lot for certain parts of the kernel because there  
> > > are  
> > > > a lot of cross dependencies.
> > > >  
> > > Hi Jonathan
> > >
> > > thanks for clarifying.
> > >
> > > Cristian
> > >  
> > > > @Sudeep, that work for you?  Have to wait for 5.12-rc1 though to give
> > > > us a sensible base.
> > > >
> > > > Jonathan
> > > >  
> > > > >  
> > > > > > Jonathan
> > > > > >  
> > > > > > >
> > > > > > > I'm saying that is better if you keep your series as it is for now
> > > > > > > (old interface + .preenable/.postdisable regs/unregs) because, as  
> > > said,  
> > > > > > > with the new interface the devm_ methods will ease the registration
> > > > > > > @probe time, and also especially because the new interface is not  
> > > (and  
> > > > > > > most probably won't) be part of the v5.4 backport that you are  
> > > testing  
> > > > > > > against: so if you stick with your current solution you'll have a
> > > > > > > working patch easily backportable now, and once queued I'll port  
> > > it to  
> > > > > > > the interface using devm_ (so simplifying it)
> > > > > > >
> > > > > > > In this context, it would be indeed important to know if in  
> > > general moving  
> > > > > > > registration to the probe phase (which should be fine by the spec)  
> > > poses  
> > > > > > > any kind of problem. (and that's reason why asked it)
> > > > > > >
> > > > > > > Hope to have been clear despite the flood of words :D
> > > > > > >
> > > > > > > Thanks
> > > > > > >
> > > > > > > Cristian
> > > > > > >  
> > > > > > > >
> > > > > > > > Thanks,
> > > > > > > > Jyoti
> > > > > > > >
> > > > > > > > On Tue, Feb 9, 2021 at 3:56 AM Cristian Marussi <  
> > > cristian.marussi@arm.com>  
> > > > > > > > wrote:
> > > > > > > >  
> > > > > > > > > Hi Jyoti
> > > > > > > > >
> > > > > > > > > some minor things down below.
> > > > > > > > >
> > > > > > > > > Other than that, FWIW about the SCMI side of this:
> > > > > > > > >
> > > > > > > > > Reviewed-by: Cristian Marussi <cristian.marussi@arm.com>
> > > > > > > > >
> > > > > > > > > Thanks
> > > > > > > > >
> > > > > > > > > Cristian
> > > > > > > > >
> > > > > > > > > On Mon, Feb 08, 2021 at 09:19:18PM +0000, Jyoti Bhayana  
> > > 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  
> > > > > > > > > >
> > > > > > > > > > Signed-off-by: Jyoti Bhayana <jbhayana@google.com>
> > > > > > > > > > ---
> > > > > > > > > >  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 | 673  
> > > +++++++++++++++++++++  
> > > > > > > > > >  7 files changed, 705 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 <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/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..093b1fc24e27
> > > > > > > > > > --- /dev/null
> > > > > > > > > > +++ b/drivers/iio/common/scmi_sensors/scmi_iio.c
> > > > > > > > > > @@ -0,0 +1,673 @@
> > > > > > > > > > +// 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,
> > > > > > > > > > +};  
> > > > > > > > >
> > > > > > > > > This is just a question, I'm not suggesting to change anything  
> > > here at  
> > > > > > > > > this point to be clear, since it works just fine as it is.
> > > > > > > > >
> > > > > > > > > Following up a previous email, given these are called on  
> > > enable/disable  
> > > > > > > > > by sysfs, is there a specific reason why you configure here,  
> > > inside  
> > > > > > > > > these ops, also timestamping and callbacks  i.e. each time the  
> > > sensor is  
> > > > > > > > > turned on/off by sysfs ? ... instead of just, as an example,  
> > > enabling  
> > > > > > > > > in _preenable the sensor while registering callbacks and  
> > > enabling  
> > > > > > > > > timestamping once for all earlier during probe phase ?
> > > > > > > > > (likewise for _postdisable -> remove)
> > > > > > > > >
> > > > > > > > > AFAIU the spec says notifications are emitted for sensors  
> > > which has  
> > > > > > > > > requested them (via SENSOR_CONTINUOUS_UPDATE_NOTIFY) BUT only  
> > > if the  
> > > > > > > > > sensor is enabled as a whole (via proper CONFIG_SET as you  
> > > do), so  
> > > > > > > > > that enabling/disabling the sensor as a whole should result in  
> > > starting/  
> > > > > > > > > stopping the notification flow without the need of  
> > > unregistering the  
> > > > > > > > > callbacks everytime. (same goes with the timestamping)
> > > > > > > > >
> > > > > > > > > In other words, I would expect the sensor to maintain its  
> > > state (on the  
> > > > > > > > > platform side) even when going through enable/disable cycles,  
> > > so that  
> > > > > > > > > it 'remembers' that timestamping/notifications were enabled  
> > > across an  
> > > > > > > > > on/off.
> > > > > > > > >
> > > > > > > > > This would reduce the number of SCMI messages exchanges  
> > > between the  
> > > > > > > > > kernel and the platform and should be supported by both, but  
> > > as said,  
> > > > > > > > > it's more of a question for the future, not necessarily for  
> > > this series.  
> > > > > > > > >  
> > > > > > > > > > +  
> > > > > > > > >
> > > > > > > > > [snip]
> > > > > > > > >  
> > > > > > > > > > +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;
> > > > > > > > > > +}
> > > > > > > > > > +
> > > > > > > > > > +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);
> > > > > > > > > > +
> > > > > > > > > > +     scmi_iio_set_sampling_freq_avail(iiodev);  
> > > > > > > > >
> > > > > > > > > You don't check this for retval, and it could fail with  
> > > -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 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;
> > > > > > > > > > +}
> > > > > > > > > > +  
> > > > > > > > >  
> > > > > >  
> > > >  
> > >  


^ permalink raw reply	[flat|nested] 13+ messages in thread
end of thread, other threads:[~2021-02-16 20:06 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-08 21:19 [PATCH v5 0/1] Adding support for IIO SCMI based sensors Jyoti Bhayana
2021-02-08 21:19 ` [PATCH v5 1/1] iio/scmi: Adding support for IIO SCMI Based Sensors Jyoti Bhayana
2021-02-09  3:27   ` kernel test robot
2021-02-09  3:27     ` kernel test robot
2021-02-09 11:56   ` Cristian Marussi
     [not found]     ` <CA+=V6c12nRxLCxM2DPst8RV=i+1WatPyHcQQZp4xAzuoN0vKaw@mail.gmail.com>
2021-02-10 21:46       ` Cristian Marussi
2021-02-12 19:18         ` Jonathan Cameron
2021-02-15  9:25           ` Cristian Marussi
2021-02-15 11:07             ` Jonathan Cameron
2021-02-15 11:17               ` Sudeep Holla
2021-02-15 14:47               ` Cristian Marussi
     [not found]                 ` <CA+=V6c3xmidg9BCZuEoigwcbDwW-sTCZWnVCkPhj4KMCzYCehg@mail.gmail.com>
2021-02-16 10:20                   ` Cristian Marussi
2021-02-16 20:05                     ` Jonathan Cameron

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.