[PATCH 1/2] dt-bindings: iio: accel: add binding documentation for ADIS16240

[PATCH 1/2] dt-bindings: iio: accel: add binding documentation for ADIS16240

[Rodrigo Carvalho]

This patch add device tree binding documentation for ADIS16240.

Signed-off-by: Rodrigo Ribeiro Carvalho <rodrigorsdc@gmail.com>
---
I have doubt about what maintainer I may to put in that documentation. I
put Alexandru as maintainer because he reviewed my last patch on this
driver, so I think that he is a good candidate.
 .../bindings/iio/accel/adi,adis16240.yaml     | 55 +++++++++++++++++++
 1 file changed, 55 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/accel/adi,adis16240.yaml

diff --git a/Documentation/devicetree/bindings/iio/accel/adi,adis16240.yaml b/Documentation/devicetree/bindings/iio/accel/adi,adis16240.yaml
new file mode 100644
index 000000000000..08019b51611c
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/accel/adi,adis16240.yaml
@@ -0,0 +1,55 @@
+# SPDX-License-Identifier: GPL-2.0
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/accel/adi,adis16240.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: ADIS16240 Programmable Impact Sensor and Recorder driver
+
+maintainers:
+  - Alexandru Ardelean <alexandru.ardelean@analog.com>
+
+description: |
+  ADIS16240 Programmable Impact Sensor and Recorder driver that supports
+  SPI interface.
+    https://www.analog.com/en/products/adis16240.html
+
+properties:
+  compatible:
+    enum:
+      - adi,adis16240
+
+  reg:
+    maxItems: 1
+
+  spi-cpha: true
+
+  spi-cpol: true
+
+  interrupts:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+  - interrupts
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+    #include <dt-bindings/interrupt-controller/irq.h>
+    spi0 {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        /* Example for a SPI device node */
+        accelerometer@0 {
+            compatible = "adi,adis16240";
+            reg = <0>;
+            spi-max-frequency = <2500000>;
+            spi-cpol;
+            spi-cpha;
+            interrupt-parent = <&gpio0>;
+            interrupts = <0 IRQ_TYPE_LEVEL_HIGH>;
+        };
+    };
-- 
2.23.0.rc1

[PATCH 2/2] staging: iio: accel: adis16240: move out of staging

[Rodrigo Carvalho]

Move ADIS16240 driver from staging to mainline.

The ADIS16240 is a fully integrated digital shock detection
and recorder system.

Signed-off-by: Rodrigo Ribeiro Carvalho <rodrigorsdc@gmail.com>
---
 drivers/iio/accel/Kconfig             |  12 +
 drivers/iio/accel/Makefile            |   1 +
 drivers/iio/accel/adis16240.c         | 454 ++++++++++++++++++++++++++
 drivers/staging/iio/accel/Kconfig     |  12 -
 drivers/staging/iio/accel/Makefile    |   1 -
 drivers/staging/iio/accel/adis16240.c | 454 --------------------------
 6 files changed, 467 insertions(+), 467 deletions(-)
 create mode 100644 drivers/iio/accel/adis16240.c
 delete mode 100644 drivers/staging/iio/accel/adis16240.c

diff --git a/drivers/iio/accel/Kconfig b/drivers/iio/accel/Kconfig
index d4ef35aeb579..91fd8741c95f 100644
--- a/drivers/iio/accel/Kconfig
+++ b/drivers/iio/accel/Kconfig
@@ -30,6 +30,18 @@ config ADIS16209
 	  To compile this driver as a module, say M here: the module will be
 	  called adis16209.
 
+config ADIS16240
+	tristate "Analog Devices ADIS16240 Programmable Impact Sensor and Recorder"
+	depends on SPI
+	select IIO_ADIS_LIB
+	select IIO_ADIS_LIB_BUFFER if IIO_BUFFER
+	help
+	  Say Y here to build support for Analog Devices adis16240 programmable
+	  impact Sensor and recorder.
+
+	  To compile this driver as a module, say M here: the module will be
+	  called adis16240.
+	  
 config ADXL345
 	tristate
 
diff --git a/drivers/iio/accel/Makefile b/drivers/iio/accel/Makefile
index 56bd0215e0d4..f7e025a86dd9 100644
--- a/drivers/iio/accel/Makefile
+++ b/drivers/iio/accel/Makefile
@@ -6,6 +6,7 @@
 # When adding new entries keep the list in alphabetical order
 obj-$(CONFIG_ADIS16201) += adis16201.o
 obj-$(CONFIG_ADIS16209) += adis16209.o
+obj-$(CONFIG_ADIS16240) += adis16240.o
 obj-$(CONFIG_ADXL345) += adxl345_core.o
 obj-$(CONFIG_ADXL345_I2C) += adxl345_i2c.o
 obj-$(CONFIG_ADXL345_SPI) += adxl345_spi.o
diff --git a/drivers/iio/accel/adis16240.c b/drivers/iio/accel/adis16240.c
new file mode 100644
index 000000000000..82099db4bf0c
--- /dev/null
+++ b/drivers/iio/accel/adis16240.c
@@ -0,0 +1,454 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * ADIS16240 Programmable Impact Sensor and Recorder driver
+ *
+ * Copyright 2010 Analog Devices Inc.
+ */
+
+#include <linux/interrupt.h>
+#include <linux/irq.h>
+#include <linux/gpio.h>
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/kernel.h>
+#include <linux/spi/spi.h>
+#include <linux/slab.h>
+#include <linux/sysfs.h>
+#include <linux/list.h>
+#include <linux/module.h>
+
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+#include <linux/iio/buffer.h>
+#include <linux/iio/imu/adis.h>
+
+#define ADIS16240_STARTUP_DELAY	220 /* ms */
+
+/* Flash memory write count */
+#define ADIS16240_FLASH_CNT      0x00
+
+/* Output, power supply */
+#define ADIS16240_SUPPLY_OUT     0x02
+
+/* Output, x-axis accelerometer */
+#define ADIS16240_XACCL_OUT      0x04
+
+/* Output, y-axis accelerometer */
+#define ADIS16240_YACCL_OUT      0x06
+
+/* Output, z-axis accelerometer */
+#define ADIS16240_ZACCL_OUT      0x08
+
+/* Output, auxiliary ADC input */
+#define ADIS16240_AUX_ADC        0x0A
+
+/* Output, temperature */
+#define ADIS16240_TEMP_OUT       0x0C
+
+/* Output, x-axis acceleration peak */
+#define ADIS16240_XPEAK_OUT      0x0E
+
+/* Output, y-axis acceleration peak */
+#define ADIS16240_YPEAK_OUT      0x10
+
+/* Output, z-axis acceleration peak */
+#define ADIS16240_ZPEAK_OUT      0x12
+
+/* Output, sum-of-squares acceleration peak */
+#define ADIS16240_XYZPEAK_OUT    0x14
+
+/* Output, Capture Buffer 1, X and Y acceleration */
+#define ADIS16240_CAPT_BUF1      0x16
+
+/* Output, Capture Buffer 2, Z acceleration */
+#define ADIS16240_CAPT_BUF2      0x18
+
+/* Diagnostic, error flags */
+#define ADIS16240_DIAG_STAT      0x1A
+
+/* Diagnostic, event counter */
+#define ADIS16240_EVNT_CNTR      0x1C
+
+/* Diagnostic, check sum value from firmware test */
+#define ADIS16240_CHK_SUM        0x1E
+
+/* Calibration, x-axis acceleration offset adjustment */
+#define ADIS16240_XACCL_OFF      0x20
+
+/* Calibration, y-axis acceleration offset adjustment */
+#define ADIS16240_YACCL_OFF      0x22
+
+/* Calibration, z-axis acceleration offset adjustment */
+#define ADIS16240_ZACCL_OFF      0x24
+
+/* Clock, hour and minute */
+#define ADIS16240_CLK_TIME       0x2E
+
+/* Clock, month and day */
+#define ADIS16240_CLK_DATE       0x30
+
+/* Clock, year */
+#define ADIS16240_CLK_YEAR       0x32
+
+/* Wake-up setting, hour and minute */
+#define ADIS16240_WAKE_TIME      0x34
+
+/* Wake-up setting, month and day */
+#define ADIS16240_WAKE_DATE      0x36
+
+/* Alarm 1 amplitude threshold */
+#define ADIS16240_ALM_MAG1       0x38
+
+/* Alarm 2 amplitude threshold */
+#define ADIS16240_ALM_MAG2       0x3A
+
+/* Alarm control */
+#define ADIS16240_ALM_CTRL       0x3C
+
+/* Capture, external trigger control */
+#define ADIS16240_XTRIG_CTRL     0x3E
+
+/* Capture, address pointer */
+#define ADIS16240_CAPT_PNTR      0x40
+
+/* Capture, configuration and control */
+#define ADIS16240_CAPT_CTRL      0x42
+
+/* General-purpose digital input/output control */
+#define ADIS16240_GPIO_CTRL      0x44
+
+/* Miscellaneous control */
+#define ADIS16240_MSC_CTRL       0x46
+
+/* Internal sample period (rate) control */
+#define ADIS16240_SMPL_PRD       0x48
+
+/* System command */
+#define ADIS16240_GLOB_CMD       0x4A
+
+/* MSC_CTRL */
+
+/* Enables sum-of-squares output (XYZPEAK_OUT) */
+#define ADIS16240_MSC_CTRL_XYZPEAK_OUT_EN	BIT(15)
+
+/* Enables peak tracking output (XPEAK_OUT, YPEAK_OUT, and ZPEAK_OUT) */
+#define ADIS16240_MSC_CTRL_X_Y_ZPEAK_OUT_EN	BIT(14)
+
+/* Self-test enable: 1 = apply electrostatic force, 0 = disabled */
+#define ADIS16240_MSC_CTRL_SELF_TEST_EN	        BIT(8)
+
+/* Data-ready enable: 1 = enabled, 0 = disabled */
+#define ADIS16240_MSC_CTRL_DATA_RDY_EN	        BIT(2)
+
+/* Data-ready polarity: 1 = active high, 0 = active low */
+#define ADIS16240_MSC_CTRL_ACTIVE_HIGH	        BIT(1)
+
+/* Data-ready line selection: 1 = DIO2, 0 = DIO1 */
+#define ADIS16240_MSC_CTRL_DATA_RDY_DIO2	BIT(0)
+
+/* DIAG_STAT */
+
+/* Alarm 2 status: 1 = alarm active, 0 = alarm inactive */
+#define ADIS16240_DIAG_STAT_ALARM2      BIT(9)
+
+/* Alarm 1 status: 1 = alarm active, 0 = alarm inactive */
+#define ADIS16240_DIAG_STAT_ALARM1      BIT(8)
+
+/* Capture buffer full: 1 = capture buffer is full */
+#define ADIS16240_DIAG_STAT_CPT_BUF_FUL BIT(7)
+
+/* Flash test, checksum flag: 1 = mismatch, 0 = match */
+#define ADIS16240_DIAG_STAT_CHKSUM      BIT(6)
+
+/* Power-on, self-test flag: 1 = failure, 0 = pass */
+#define ADIS16240_DIAG_STAT_PWRON_FAIL_BIT  5
+
+/* Power-on self-test: 1 = in-progress, 0 = complete */
+#define ADIS16240_DIAG_STAT_PWRON_BUSY  BIT(4)
+
+/* SPI communications failure */
+#define ADIS16240_DIAG_STAT_SPI_FAIL_BIT	3
+
+/* Flash update failure */
+#define ADIS16240_DIAG_STAT_FLASH_UPT_BIT	2
+
+/* Power supply above 3.625 V */
+#define ADIS16240_DIAG_STAT_POWER_HIGH_BIT	1
+
+ /* Power supply below 2.225 V */
+#define ADIS16240_DIAG_STAT_POWER_LOW_BIT	0
+
+/* GLOB_CMD */
+
+#define ADIS16240_GLOB_CMD_RESUME	BIT(8)
+#define ADIS16240_GLOB_CMD_SW_RESET	BIT(7)
+#define ADIS16240_GLOB_CMD_STANDBY	BIT(2)
+
+#define ADIS16240_ERROR_ACTIVE          BIT(14)
+
+/* At the moment triggers are only used for ring buffer
+ * filling. This may change!
+ */
+
+enum adis16240_scan {
+	ADIS16240_SCAN_ACC_X,
+	ADIS16240_SCAN_ACC_Y,
+	ADIS16240_SCAN_ACC_Z,
+	ADIS16240_SCAN_SUPPLY,
+	ADIS16240_SCAN_AUX_ADC,
+	ADIS16240_SCAN_TEMP,
+};
+
+static ssize_t adis16240_spi_read_signed(struct device *dev,
+					 struct device_attribute *attr,
+					 char *buf,
+					 unsigned int bits)
+{
+	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct adis *st = iio_priv(indio_dev);
+	int ret;
+	s16 val = 0;
+	unsigned int shift = 16 - bits;
+	struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
+
+	ret = adis_read_reg_16(st,
+			       this_attr->address, (u16 *)&val);
+	if (ret)
+		return ret;
+
+	if (val & ADIS16240_ERROR_ACTIVE)
+		adis_check_status(st);
+
+	val = (s16)(val << shift) >> shift;
+	return sprintf(buf, "%d\n", val);
+}
+
+static ssize_t adis16240_read_12bit_signed(struct device *dev,
+					   struct device_attribute *attr,
+					   char *buf)
+{
+	return adis16240_spi_read_signed(dev, attr, buf, 12);
+}
+
+static IIO_DEVICE_ATTR(in_accel_xyz_squared_peak_raw, 0444,
+		       adis16240_read_12bit_signed, NULL,
+		       ADIS16240_XYZPEAK_OUT);
+
+static IIO_CONST_ATTR_SAMP_FREQ_AVAIL("4096");
+
+static const u8 adis16240_addresses[][2] = {
+	[ADIS16240_SCAN_ACC_X] = { ADIS16240_XACCL_OFF, ADIS16240_XPEAK_OUT },
+	[ADIS16240_SCAN_ACC_Y] = { ADIS16240_YACCL_OFF, ADIS16240_YPEAK_OUT },
+	[ADIS16240_SCAN_ACC_Z] = { ADIS16240_ZACCL_OFF, ADIS16240_ZPEAK_OUT },
+};
+
+static int adis16240_read_raw(struct iio_dev *indio_dev,
+			      struct iio_chan_spec const *chan,
+			      int *val, int *val2,
+			      long mask)
+{
+	struct adis *st = iio_priv(indio_dev);
+	int ret;
+	u8 addr;
+	s16 val16;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		return adis_single_conversion(indio_dev, chan,
+				ADIS16240_ERROR_ACTIVE, val);
+	case IIO_CHAN_INFO_SCALE:
+		switch (chan->type) {
+		case IIO_VOLTAGE:
+			if (chan->channel == 0) {
+				*val = 4;
+				*val2 = 880000; /* 4.88 mV */
+				return IIO_VAL_INT_PLUS_MICRO;
+			}
+			return -EINVAL;
+		case IIO_TEMP:
+			*val = 244; /* 0.244 C */
+			*val2 = 0;
+			return IIO_VAL_INT_PLUS_MICRO;
+		case IIO_ACCEL:
+			*val = 0;
+			*val2 = IIO_G_TO_M_S_2(51400); /* 51.4 mg */
+			return IIO_VAL_INT_PLUS_MICRO;
+		default:
+			return -EINVAL;
+		}
+		break;
+	case IIO_CHAN_INFO_PEAK_SCALE:
+		*val = 0;
+		*val2 = IIO_G_TO_M_S_2(51400); /* 51.4 mg */
+		return IIO_VAL_INT_PLUS_MICRO;
+	case IIO_CHAN_INFO_OFFSET:
+		*val = 25000 / 244 - 0x133; /* 25 C = 0x133 */
+		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_CALIBBIAS:
+		addr = adis16240_addresses[chan->scan_index][0];
+		ret = adis_read_reg_16(st, addr, &val16);
+		if (ret)
+			return ret;
+		*val = sign_extend32(val16, 9);
+		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_PEAK:
+		addr = adis16240_addresses[chan->scan_index][1];
+		ret = adis_read_reg_16(st, addr, &val16);
+		if (ret)
+			return ret;
+		*val = sign_extend32(val16, 9);
+		return IIO_VAL_INT;
+	}
+	return -EINVAL;
+}
+
+static int adis16240_write_raw(struct iio_dev *indio_dev,
+			       struct iio_chan_spec const *chan,
+			       int val,
+			       int val2,
+			       long mask)
+{
+	struct adis *st = iio_priv(indio_dev);
+	u8 addr;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_CALIBBIAS:
+		addr = adis16240_addresses[chan->scan_index][0];
+		return adis_write_reg_16(st, addr, val & GENMASK(9, 0));
+	}
+	return -EINVAL;
+}
+
+static const struct iio_chan_spec adis16240_channels[] = {
+	ADIS_SUPPLY_CHAN(ADIS16240_SUPPLY_OUT, ADIS16240_SCAN_SUPPLY, 0, 10),
+	ADIS_AUX_ADC_CHAN(ADIS16240_AUX_ADC, ADIS16240_SCAN_AUX_ADC, 0, 10),
+	ADIS_ACCEL_CHAN(X, ADIS16240_XACCL_OUT, ADIS16240_SCAN_ACC_X,
+			BIT(IIO_CHAN_INFO_CALIBBIAS) | BIT(IIO_CHAN_INFO_PEAK),
+			0, 10),
+	ADIS_ACCEL_CHAN(Y, ADIS16240_YACCL_OUT, ADIS16240_SCAN_ACC_Y,
+			BIT(IIO_CHAN_INFO_CALIBBIAS) | BIT(IIO_CHAN_INFO_PEAK),
+			0, 10),
+	ADIS_ACCEL_CHAN(Z, ADIS16240_ZACCL_OUT, ADIS16240_SCAN_ACC_Z,
+			BIT(IIO_CHAN_INFO_CALIBBIAS) | BIT(IIO_CHAN_INFO_PEAK),
+			0, 10),
+	ADIS_TEMP_CHAN(ADIS16240_TEMP_OUT, ADIS16240_SCAN_TEMP, 0, 10),
+	IIO_CHAN_SOFT_TIMESTAMP(6)
+};
+
+static struct attribute *adis16240_attributes[] = {
+	&iio_dev_attr_in_accel_xyz_squared_peak_raw.dev_attr.attr,
+	&iio_const_attr_sampling_frequency_available.dev_attr.attr,
+	NULL
+};
+
+static const struct attribute_group adis16240_attribute_group = {
+	.attrs = adis16240_attributes,
+};
+
+static const struct iio_info adis16240_info = {
+	.attrs = &adis16240_attribute_group,
+	.read_raw = adis16240_read_raw,
+	.write_raw = adis16240_write_raw,
+	.update_scan_mode = adis_update_scan_mode,
+};
+
+static const char * const adis16240_status_error_msgs[] = {
+	[ADIS16240_DIAG_STAT_PWRON_FAIL_BIT] = "Power on, self-test failed",
+	[ADIS16240_DIAG_STAT_SPI_FAIL_BIT] = "SPI failure",
+	[ADIS16240_DIAG_STAT_FLASH_UPT_BIT] = "Flash update failed",
+	[ADIS16240_DIAG_STAT_POWER_HIGH_BIT] = "Power supply above 3.625V",
+	[ADIS16240_DIAG_STAT_POWER_LOW_BIT] = "Power supply below 2.225V",
+};
+
+static const struct adis_data adis16240_data = {
+	.write_delay = 35,
+	.read_delay = 35,
+	.msc_ctrl_reg = ADIS16240_MSC_CTRL,
+	.glob_cmd_reg = ADIS16240_GLOB_CMD,
+	.diag_stat_reg = ADIS16240_DIAG_STAT,
+
+	.self_test_mask = ADIS16240_MSC_CTRL_SELF_TEST_EN,
+	.self_test_no_autoclear = true,
+	.startup_delay = ADIS16240_STARTUP_DELAY,
+
+	.status_error_msgs = adis16240_status_error_msgs,
+	.status_error_mask = BIT(ADIS16240_DIAG_STAT_PWRON_FAIL_BIT) |
+		BIT(ADIS16240_DIAG_STAT_SPI_FAIL_BIT) |
+		BIT(ADIS16240_DIAG_STAT_FLASH_UPT_BIT) |
+		BIT(ADIS16240_DIAG_STAT_POWER_HIGH_BIT) |
+		BIT(ADIS16240_DIAG_STAT_POWER_LOW_BIT),
+};
+
+static int adis16240_probe(struct spi_device *spi)
+{
+	int ret;
+	struct adis *st;
+	struct iio_dev *indio_dev;
+
+	/* setup the industrialio driver allocated elements */
+	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
+	if (!indio_dev)
+		return -ENOMEM;
+	st = iio_priv(indio_dev);
+	/* this is only used for removal purposes */
+	spi_set_drvdata(spi, indio_dev);
+
+	indio_dev->name = spi->dev.driver->name;
+	indio_dev->dev.parent = &spi->dev;
+	indio_dev->info = &adis16240_info;
+	indio_dev->channels = adis16240_channels;
+	indio_dev->num_channels = ARRAY_SIZE(adis16240_channels);
+	indio_dev->modes = INDIO_DIRECT_MODE;
+
+	ret = adis_init(st, indio_dev, spi, &adis16240_data);
+	if (ret)
+		return ret;
+	ret = adis_setup_buffer_and_trigger(st, indio_dev, NULL);
+	if (ret)
+		return ret;
+
+	/* Get the device into a sane initial state */
+	ret = adis_initial_startup(st);
+	if (ret)
+		goto error_cleanup_buffer_trigger;
+	ret = iio_device_register(indio_dev);
+	if (ret)
+		goto error_cleanup_buffer_trigger;
+	return 0;
+
+error_cleanup_buffer_trigger:
+	adis_cleanup_buffer_and_trigger(st, indio_dev);
+	return ret;
+}
+
+static int adis16240_remove(struct spi_device *spi)
+{
+	struct iio_dev *indio_dev = spi_get_drvdata(spi);
+	struct adis *st = iio_priv(indio_dev);
+
+	iio_device_unregister(indio_dev);
+	adis_cleanup_buffer_and_trigger(st, indio_dev);
+
+	return 0;
+}
+
+static const struct of_device_id adis16240_of_match[] = {
+	{ .compatible = "adi,adis16240" },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, adis16240_of_match);
+
+static struct spi_driver adis16240_driver = {
+	.driver = {
+		.name = "adis16240",
+		.of_match_table = adis16240_of_match,
+	},
+	.probe = adis16240_probe,
+	.remove = adis16240_remove,
+};
+module_spi_driver(adis16240_driver);
+
+MODULE_AUTHOR("Barry Song <21cnbao@gmail.com>");
+MODULE_DESCRIPTION("Analog Devices Programmable Impact Sensor and Recorder");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("spi:adis16240");
diff --git a/drivers/staging/iio/accel/Kconfig b/drivers/staging/iio/accel/Kconfig
index 3318997a7009..cee51f64bc4b 100644
--- a/drivers/staging/iio/accel/Kconfig
+++ b/drivers/staging/iio/accel/Kconfig
@@ -16,16 +16,4 @@ config ADIS16203
 	  To compile this driver as a module, say M here: the module will be
 	  called adis16203.
 
-config ADIS16240
-	tristate "Analog Devices ADIS16240 Programmable Impact Sensor and Recorder"
-	depends on SPI
-	select IIO_ADIS_LIB
-	select IIO_ADIS_LIB_BUFFER if IIO_BUFFER
-	help
-	  Say Y here to build support for Analog Devices adis16240 programmable
-	  impact Sensor and recorder.
-
-	  To compile this driver as a module, say M here: the module will be
-	  called adis16240.
-
 endmenu
diff --git a/drivers/staging/iio/accel/Makefile b/drivers/staging/iio/accel/Makefile
index 094cc9be35bd..acac7bc9b9c0 100644
--- a/drivers/staging/iio/accel/Makefile
+++ b/drivers/staging/iio/accel/Makefile
@@ -4,4 +4,3 @@
 #
 
 obj-$(CONFIG_ADIS16203) += adis16203.o
-obj-$(CONFIG_ADIS16240) += adis16240.o
diff --git a/drivers/staging/iio/accel/adis16240.c b/drivers/staging/iio/accel/adis16240.c
deleted file mode 100644
index 82099db4bf0c..000000000000
--- a/drivers/staging/iio/accel/adis16240.c
+++ /dev/null
@@ -1,454 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0+
-/*
- * ADIS16240 Programmable Impact Sensor and Recorder driver
- *
- * Copyright 2010 Analog Devices Inc.
- */
-
-#include <linux/interrupt.h>
-#include <linux/irq.h>
-#include <linux/gpio.h>
-#include <linux/delay.h>
-#include <linux/device.h>
-#include <linux/kernel.h>
-#include <linux/spi/spi.h>
-#include <linux/slab.h>
-#include <linux/sysfs.h>
-#include <linux/list.h>
-#include <linux/module.h>
-
-#include <linux/iio/iio.h>
-#include <linux/iio/sysfs.h>
-#include <linux/iio/buffer.h>
-#include <linux/iio/imu/adis.h>
-
-#define ADIS16240_STARTUP_DELAY	220 /* ms */
-
-/* Flash memory write count */
-#define ADIS16240_FLASH_CNT      0x00
-
-/* Output, power supply */
-#define ADIS16240_SUPPLY_OUT     0x02
-
-/* Output, x-axis accelerometer */
-#define ADIS16240_XACCL_OUT      0x04
-
-/* Output, y-axis accelerometer */
-#define ADIS16240_YACCL_OUT      0x06
-
-/* Output, z-axis accelerometer */
-#define ADIS16240_ZACCL_OUT      0x08
-
-/* Output, auxiliary ADC input */
-#define ADIS16240_AUX_ADC        0x0A
-
-/* Output, temperature */
-#define ADIS16240_TEMP_OUT       0x0C
-
-/* Output, x-axis acceleration peak */
-#define ADIS16240_XPEAK_OUT      0x0E
-
-/* Output, y-axis acceleration peak */
-#define ADIS16240_YPEAK_OUT      0x10
-
-/* Output, z-axis acceleration peak */
-#define ADIS16240_ZPEAK_OUT      0x12
-
-/* Output, sum-of-squares acceleration peak */
-#define ADIS16240_XYZPEAK_OUT    0x14
-
-/* Output, Capture Buffer 1, X and Y acceleration */
-#define ADIS16240_CAPT_BUF1      0x16
-
-/* Output, Capture Buffer 2, Z acceleration */
-#define ADIS16240_CAPT_BUF2      0x18
-
-/* Diagnostic, error flags */
-#define ADIS16240_DIAG_STAT      0x1A
-
-/* Diagnostic, event counter */
-#define ADIS16240_EVNT_CNTR      0x1C
-
-/* Diagnostic, check sum value from firmware test */
-#define ADIS16240_CHK_SUM        0x1E
-
-/* Calibration, x-axis acceleration offset adjustment */
-#define ADIS16240_XACCL_OFF      0x20
-
-/* Calibration, y-axis acceleration offset adjustment */
-#define ADIS16240_YACCL_OFF      0x22
-
-/* Calibration, z-axis acceleration offset adjustment */
-#define ADIS16240_ZACCL_OFF      0x24
-
-/* Clock, hour and minute */
-#define ADIS16240_CLK_TIME       0x2E
-
-/* Clock, month and day */
-#define ADIS16240_CLK_DATE       0x30
-
-/* Clock, year */
-#define ADIS16240_CLK_YEAR       0x32
-
-/* Wake-up setting, hour and minute */
-#define ADIS16240_WAKE_TIME      0x34
-
-/* Wake-up setting, month and day */
-#define ADIS16240_WAKE_DATE      0x36
-
-/* Alarm 1 amplitude threshold */
-#define ADIS16240_ALM_MAG1       0x38
-
-/* Alarm 2 amplitude threshold */
-#define ADIS16240_ALM_MAG2       0x3A
-
-/* Alarm control */
-#define ADIS16240_ALM_CTRL       0x3C
-
-/* Capture, external trigger control */
-#define ADIS16240_XTRIG_CTRL     0x3E
-
-/* Capture, address pointer */
-#define ADIS16240_CAPT_PNTR      0x40
-
-/* Capture, configuration and control */
-#define ADIS16240_CAPT_CTRL      0x42
-
-/* General-purpose digital input/output control */
-#define ADIS16240_GPIO_CTRL      0x44
-
-/* Miscellaneous control */
-#define ADIS16240_MSC_CTRL       0x46
-
-/* Internal sample period (rate) control */
-#define ADIS16240_SMPL_PRD       0x48
-
-/* System command */
-#define ADIS16240_GLOB_CMD       0x4A
-
-/* MSC_CTRL */
-
-/* Enables sum-of-squares output (XYZPEAK_OUT) */
-#define ADIS16240_MSC_CTRL_XYZPEAK_OUT_EN	BIT(15)
-
-/* Enables peak tracking output (XPEAK_OUT, YPEAK_OUT, and ZPEAK_OUT) */
-#define ADIS16240_MSC_CTRL_X_Y_ZPEAK_OUT_EN	BIT(14)
-
-/* Self-test enable: 1 = apply electrostatic force, 0 = disabled */
-#define ADIS16240_MSC_CTRL_SELF_TEST_EN	        BIT(8)
-
-/* Data-ready enable: 1 = enabled, 0 = disabled */
-#define ADIS16240_MSC_CTRL_DATA_RDY_EN	        BIT(2)
-
-/* Data-ready polarity: 1 = active high, 0 = active low */
-#define ADIS16240_MSC_CTRL_ACTIVE_HIGH	        BIT(1)
-
-/* Data-ready line selection: 1 = DIO2, 0 = DIO1 */
-#define ADIS16240_MSC_CTRL_DATA_RDY_DIO2	BIT(0)
-
-/* DIAG_STAT */
-
-/* Alarm 2 status: 1 = alarm active, 0 = alarm inactive */
-#define ADIS16240_DIAG_STAT_ALARM2      BIT(9)
-
-/* Alarm 1 status: 1 = alarm active, 0 = alarm inactive */
-#define ADIS16240_DIAG_STAT_ALARM1      BIT(8)
-
-/* Capture buffer full: 1 = capture buffer is full */
-#define ADIS16240_DIAG_STAT_CPT_BUF_FUL BIT(7)
-
-/* Flash test, checksum flag: 1 = mismatch, 0 = match */
-#define ADIS16240_DIAG_STAT_CHKSUM      BIT(6)
-
-/* Power-on, self-test flag: 1 = failure, 0 = pass */
-#define ADIS16240_DIAG_STAT_PWRON_FAIL_BIT  5
-
-/* Power-on self-test: 1 = in-progress, 0 = complete */
-#define ADIS16240_DIAG_STAT_PWRON_BUSY  BIT(4)
-
-/* SPI communications failure */
-#define ADIS16240_DIAG_STAT_SPI_FAIL_BIT	3
-
-/* Flash update failure */
-#define ADIS16240_DIAG_STAT_FLASH_UPT_BIT	2
-
-/* Power supply above 3.625 V */
-#define ADIS16240_DIAG_STAT_POWER_HIGH_BIT	1
-
- /* Power supply below 2.225 V */
-#define ADIS16240_DIAG_STAT_POWER_LOW_BIT	0
-
-/* GLOB_CMD */
-
-#define ADIS16240_GLOB_CMD_RESUME	BIT(8)
-#define ADIS16240_GLOB_CMD_SW_RESET	BIT(7)
-#define ADIS16240_GLOB_CMD_STANDBY	BIT(2)
-
-#define ADIS16240_ERROR_ACTIVE          BIT(14)
-
-/* At the moment triggers are only used for ring buffer
- * filling. This may change!
- */
-
-enum adis16240_scan {
-	ADIS16240_SCAN_ACC_X,
-	ADIS16240_SCAN_ACC_Y,
-	ADIS16240_SCAN_ACC_Z,
-	ADIS16240_SCAN_SUPPLY,
-	ADIS16240_SCAN_AUX_ADC,
-	ADIS16240_SCAN_TEMP,
-};
-
-static ssize_t adis16240_spi_read_signed(struct device *dev,
-					 struct device_attribute *attr,
-					 char *buf,
-					 unsigned int bits)
-{
-	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
-	struct adis *st = iio_priv(indio_dev);
-	int ret;
-	s16 val = 0;
-	unsigned int shift = 16 - bits;
-	struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
-
-	ret = adis_read_reg_16(st,
-			       this_attr->address, (u16 *)&val);
-	if (ret)
-		return ret;
-
-	if (val & ADIS16240_ERROR_ACTIVE)
-		adis_check_status(st);
-
-	val = (s16)(val << shift) >> shift;
-	return sprintf(buf, "%d\n", val);
-}
-
-static ssize_t adis16240_read_12bit_signed(struct device *dev,
-					   struct device_attribute *attr,
-					   char *buf)
-{
-	return adis16240_spi_read_signed(dev, attr, buf, 12);
-}
-
-static IIO_DEVICE_ATTR(in_accel_xyz_squared_peak_raw, 0444,
-		       adis16240_read_12bit_signed, NULL,
-		       ADIS16240_XYZPEAK_OUT);
-
-static IIO_CONST_ATTR_SAMP_FREQ_AVAIL("4096");
-
-static const u8 adis16240_addresses[][2] = {
-	[ADIS16240_SCAN_ACC_X] = { ADIS16240_XACCL_OFF, ADIS16240_XPEAK_OUT },
-	[ADIS16240_SCAN_ACC_Y] = { ADIS16240_YACCL_OFF, ADIS16240_YPEAK_OUT },
-	[ADIS16240_SCAN_ACC_Z] = { ADIS16240_ZACCL_OFF, ADIS16240_ZPEAK_OUT },
-};
-
-static int adis16240_read_raw(struct iio_dev *indio_dev,
-			      struct iio_chan_spec const *chan,
-			      int *val, int *val2,
-			      long mask)
-{
-	struct adis *st = iio_priv(indio_dev);
-	int ret;
-	u8 addr;
-	s16 val16;
-
-	switch (mask) {
-	case IIO_CHAN_INFO_RAW:
-		return adis_single_conversion(indio_dev, chan,
-				ADIS16240_ERROR_ACTIVE, val);
-	case IIO_CHAN_INFO_SCALE:
-		switch (chan->type) {
-		case IIO_VOLTAGE:
-			if (chan->channel == 0) {
-				*val = 4;
-				*val2 = 880000; /* 4.88 mV */
-				return IIO_VAL_INT_PLUS_MICRO;
-			}
-			return -EINVAL;
-		case IIO_TEMP:
-			*val = 244; /* 0.244 C */
-			*val2 = 0;
-			return IIO_VAL_INT_PLUS_MICRO;
-		case IIO_ACCEL:
-			*val = 0;
-			*val2 = IIO_G_TO_M_S_2(51400); /* 51.4 mg */
-			return IIO_VAL_INT_PLUS_MICRO;
-		default:
-			return -EINVAL;
-		}
-		break;
-	case IIO_CHAN_INFO_PEAK_SCALE:
-		*val = 0;
-		*val2 = IIO_G_TO_M_S_2(51400); /* 51.4 mg */
-		return IIO_VAL_INT_PLUS_MICRO;
-	case IIO_CHAN_INFO_OFFSET:
-		*val = 25000 / 244 - 0x133; /* 25 C = 0x133 */
-		return IIO_VAL_INT;
-	case IIO_CHAN_INFO_CALIBBIAS:
-		addr = adis16240_addresses[chan->scan_index][0];
-		ret = adis_read_reg_16(st, addr, &val16);
-		if (ret)
-			return ret;
-		*val = sign_extend32(val16, 9);
-		return IIO_VAL_INT;
-	case IIO_CHAN_INFO_PEAK:
-		addr = adis16240_addresses[chan->scan_index][1];
-		ret = adis_read_reg_16(st, addr, &val16);
-		if (ret)
-			return ret;
-		*val = sign_extend32(val16, 9);
-		return IIO_VAL_INT;
-	}
-	return -EINVAL;
-}
-
-static int adis16240_write_raw(struct iio_dev *indio_dev,
-			       struct iio_chan_spec const *chan,
-			       int val,
-			       int val2,
-			       long mask)
-{
-	struct adis *st = iio_priv(indio_dev);
-	u8 addr;
-
-	switch (mask) {
-	case IIO_CHAN_INFO_CALIBBIAS:
-		addr = adis16240_addresses[chan->scan_index][0];
-		return adis_write_reg_16(st, addr, val & GENMASK(9, 0));
-	}
-	return -EINVAL;
-}
-
-static const struct iio_chan_spec adis16240_channels[] = {
-	ADIS_SUPPLY_CHAN(ADIS16240_SUPPLY_OUT, ADIS16240_SCAN_SUPPLY, 0, 10),
-	ADIS_AUX_ADC_CHAN(ADIS16240_AUX_ADC, ADIS16240_SCAN_AUX_ADC, 0, 10),
-	ADIS_ACCEL_CHAN(X, ADIS16240_XACCL_OUT, ADIS16240_SCAN_ACC_X,
-			BIT(IIO_CHAN_INFO_CALIBBIAS) | BIT(IIO_CHAN_INFO_PEAK),
-			0, 10),
-	ADIS_ACCEL_CHAN(Y, ADIS16240_YACCL_OUT, ADIS16240_SCAN_ACC_Y,
-			BIT(IIO_CHAN_INFO_CALIBBIAS) | BIT(IIO_CHAN_INFO_PEAK),
-			0, 10),
-	ADIS_ACCEL_CHAN(Z, ADIS16240_ZACCL_OUT, ADIS16240_SCAN_ACC_Z,
-			BIT(IIO_CHAN_INFO_CALIBBIAS) | BIT(IIO_CHAN_INFO_PEAK),
-			0, 10),
-	ADIS_TEMP_CHAN(ADIS16240_TEMP_OUT, ADIS16240_SCAN_TEMP, 0, 10),
-	IIO_CHAN_SOFT_TIMESTAMP(6)
-};
-
-static struct attribute *adis16240_attributes[] = {
-	&iio_dev_attr_in_accel_xyz_squared_peak_raw.dev_attr.attr,
-	&iio_const_attr_sampling_frequency_available.dev_attr.attr,
-	NULL
-};
-
-static const struct attribute_group adis16240_attribute_group = {
-	.attrs = adis16240_attributes,
-};
-
-static const struct iio_info adis16240_info = {
-	.attrs = &adis16240_attribute_group,
-	.read_raw = adis16240_read_raw,
-	.write_raw = adis16240_write_raw,
-	.update_scan_mode = adis_update_scan_mode,
-};
-
-static const char * const adis16240_status_error_msgs[] = {
-	[ADIS16240_DIAG_STAT_PWRON_FAIL_BIT] = "Power on, self-test failed",
-	[ADIS16240_DIAG_STAT_SPI_FAIL_BIT] = "SPI failure",
-	[ADIS16240_DIAG_STAT_FLASH_UPT_BIT] = "Flash update failed",
-	[ADIS16240_DIAG_STAT_POWER_HIGH_BIT] = "Power supply above 3.625V",
-	[ADIS16240_DIAG_STAT_POWER_LOW_BIT] = "Power supply below 2.225V",
-};
-
-static const struct adis_data adis16240_data = {
-	.write_delay = 35,
-	.read_delay = 35,
-	.msc_ctrl_reg = ADIS16240_MSC_CTRL,
-	.glob_cmd_reg = ADIS16240_GLOB_CMD,
-	.diag_stat_reg = ADIS16240_DIAG_STAT,
-
-	.self_test_mask = ADIS16240_MSC_CTRL_SELF_TEST_EN,
-	.self_test_no_autoclear = true,
-	.startup_delay = ADIS16240_STARTUP_DELAY,
-
-	.status_error_msgs = adis16240_status_error_msgs,
-	.status_error_mask = BIT(ADIS16240_DIAG_STAT_PWRON_FAIL_BIT) |
-		BIT(ADIS16240_DIAG_STAT_SPI_FAIL_BIT) |
-		BIT(ADIS16240_DIAG_STAT_FLASH_UPT_BIT) |
-		BIT(ADIS16240_DIAG_STAT_POWER_HIGH_BIT) |
-		BIT(ADIS16240_DIAG_STAT_POWER_LOW_BIT),
-};
-
-static int adis16240_probe(struct spi_device *spi)
-{
-	int ret;
-	struct adis *st;
-	struct iio_dev *indio_dev;
-
-	/* setup the industrialio driver allocated elements */
-	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
-	if (!indio_dev)
-		return -ENOMEM;
-	st = iio_priv(indio_dev);
-	/* this is only used for removal purposes */
-	spi_set_drvdata(spi, indio_dev);
-
-	indio_dev->name = spi->dev.driver->name;
-	indio_dev->dev.parent = &spi->dev;
-	indio_dev->info = &adis16240_info;
-	indio_dev->channels = adis16240_channels;
-	indio_dev->num_channels = ARRAY_SIZE(adis16240_channels);
-	indio_dev->modes = INDIO_DIRECT_MODE;
-
-	ret = adis_init(st, indio_dev, spi, &adis16240_data);
-	if (ret)
-		return ret;
-	ret = adis_setup_buffer_and_trigger(st, indio_dev, NULL);
-	if (ret)
-		return ret;
-
-	/* Get the device into a sane initial state */
-	ret = adis_initial_startup(st);
-	if (ret)
-		goto error_cleanup_buffer_trigger;
-	ret = iio_device_register(indio_dev);
-	if (ret)
-		goto error_cleanup_buffer_trigger;
-	return 0;
-
-error_cleanup_buffer_trigger:
-	adis_cleanup_buffer_and_trigger(st, indio_dev);
-	return ret;
-}
-
-static int adis16240_remove(struct spi_device *spi)
-{
-	struct iio_dev *indio_dev = spi_get_drvdata(spi);
-	struct adis *st = iio_priv(indio_dev);
-
-	iio_device_unregister(indio_dev);
-	adis_cleanup_buffer_and_trigger(st, indio_dev);
-
-	return 0;
-}
-
-static const struct of_device_id adis16240_of_match[] = {
-	{ .compatible = "adi,adis16240" },
-	{ },
-};
-MODULE_DEVICE_TABLE(of, adis16240_of_match);
-
-static struct spi_driver adis16240_driver = {
-	.driver = {
-		.name = "adis16240",
-		.of_match_table = adis16240_of_match,
-	},
-	.probe = adis16240_probe,
-	.remove = adis16240_remove,
-};
-module_spi_driver(adis16240_driver);
-
-MODULE_AUTHOR("Barry Song <21cnbao@gmail.com>");
-MODULE_DESCRIPTION("Analog Devices Programmable Impact Sensor and Recorder");
-MODULE_LICENSE("GPL v2");
-MODULE_ALIAS("spi:adis16240");
-- 
2.23.0.rc1

Re: [PATCH 2/2] staging: iio: accel: adis16240: move out of staging

[Ardelean, Alexandru]

On Sun, 2019-09-01 at 21:59 -0300, Rodrigo Carvalho wrote:
> Move ADIS16240 driver from staging to mainline.
> 
> The ADIS16240 is a fully integrated digital shock detection
> and recorder system.


Hey,

Comments inline.

I'll probably take a look in the next days again.
There seem to be some ABI/sysfs attributes that need to be resolved before moving this out of staging.

> 
> Signed-off-by: Rodrigo Ribeiro Carvalho <rodrigorsdc@gmail.com>
> ---
>  drivers/iio/accel/Kconfig             |  12 +
>  drivers/iio/accel/Makefile            |   1 +
>  drivers/iio/accel/adis16240.c         | 454 ++++++++++++++++++++++++++
>  drivers/staging/iio/accel/Kconfig     |  12 -
>  drivers/staging/iio/accel/Makefile    |   1 -
>  drivers/staging/iio/accel/adis16240.c | 454 --------------------------
>  6 files changed, 467 insertions(+), 467 deletions(-)
>  create mode 100644 drivers/iio/accel/adis16240.c
>  delete mode 100644 drivers/staging/iio/accel/adis16240.c

Looks like MAINTAINERS file also needs to be updated, also with the DT bindings file.
I think checkpatch usually complains about these.

> 
> diff --git a/drivers/iio/accel/Kconfig b/drivers/iio/accel/Kconfig
> index d4ef35aeb579..91fd8741c95f 100644
> --- a/drivers/iio/accel/Kconfig
> +++ b/drivers/iio/accel/Kconfig
> @@ -30,6 +30,18 @@ config ADIS16209
>  	  To compile this driver as a module, say M here: the module will be
>  	  called adis16209.
>  
> +config ADIS16240
> +	tristate "Analog Devices ADIS16240 Programmable Impact Sensor and Recorder"
> +	depends on SPI
> +	select IIO_ADIS_LIB
> +	select IIO_ADIS_LIB_BUFFER if IIO_BUFFER
> +	help
> +	  Say Y here to build support for Analog Devices adis16240 programmable
> +	  impact Sensor and recorder.
> +
> +	  To compile this driver as a module, say M here: the module will be
> +	  called adis16240.
> +	  
>  config ADXL345
>  	tristate
>  
> diff --git a/drivers/iio/accel/Makefile b/drivers/iio/accel/Makefile
> index 56bd0215e0d4..f7e025a86dd9 100644
> --- a/drivers/iio/accel/Makefile
> +++ b/drivers/iio/accel/Makefile
> @@ -6,6 +6,7 @@
>  # When adding new entries keep the list in alphabetical order
>  obj-$(CONFIG_ADIS16201) += adis16201.o
>  obj-$(CONFIG_ADIS16209) += adis16209.o
> +obj-$(CONFIG_ADIS16240) += adis16240.o
>  obj-$(CONFIG_ADXL345) += adxl345_core.o
>  obj-$(CONFIG_ADXL345_I2C) += adxl345_i2c.o
>  obj-$(CONFIG_ADXL345_SPI) += adxl345_spi.o
> diff --git a/drivers/iio/accel/adis16240.c b/drivers/iio/accel/adis16240.c
> new file mode 100644
> index 000000000000..82099db4bf0c
> --- /dev/null
> +++ b/drivers/iio/accel/adis16240.c
> @@ -0,0 +1,454 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * ADIS16240 Programmable Impact Sensor and Recorder driver
> + *
> + * Copyright 2010 Analog Devices Inc.
> + */
> +
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
> +#include <linux/gpio.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/kernel.h>
> +#include <linux/spi/spi.h>
> +#include <linux/slab.h>
> +#include <linux/sysfs.h>
> +#include <linux/list.h>
> +#include <linux/module.h>
> +
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/imu/adis.h>
> +
> +#define ADIS16240_STARTUP_DELAY	220 /* ms */
> +
> +/* Flash memory write count */
> +#define ADIS16240_FLASH_CNT      0x00
> +
> +/* Output, power supply */
> +#define ADIS16240_SUPPLY_OUT     0x02
> +
> +/* Output, x-axis accelerometer */
> +#define ADIS16240_XACCL_OUT      0x04
> +
> +/* Output, y-axis accelerometer */
> +#define ADIS16240_YACCL_OUT      0x06
> +
> +/* Output, z-axis accelerometer */
> +#define ADIS16240_ZACCL_OUT      0x08
> +
> +/* Output, auxiliary ADC input */
> +#define ADIS16240_AUX_ADC        0x0A
> +
> +/* Output, temperature */
> +#define ADIS16240_TEMP_OUT       0x0C
> +
> +/* Output, x-axis acceleration peak */
> +#define ADIS16240_XPEAK_OUT      0x0E
> +
> +/* Output, y-axis acceleration peak */
> +#define ADIS16240_YPEAK_OUT      0x10
> +
> +/* Output, z-axis acceleration peak */
> +#define ADIS16240_ZPEAK_OUT      0x12
> +
> +/* Output, sum-of-squares acceleration peak */
> +#define ADIS16240_XYZPEAK_OUT    0x14
> +
> +/* Output, Capture Buffer 1, X and Y acceleration */
> +#define ADIS16240_CAPT_BUF1      0x16
> +
> +/* Output, Capture Buffer 2, Z acceleration */
> +#define ADIS16240_CAPT_BUF2      0x18
> +
> +/* Diagnostic, error flags */
> +#define ADIS16240_DIAG_STAT      0x1A
> +This looks like it could be converted to IIO_CHAN_INFO_SAMP_FREQ attribute.
> +/* Diagnostic, event counter */
> +#define ADIS16240_EVNT_CNTR      0x1C
> +
> +/* Diagnostic, check sum value from firmware test */
> +#define ADIS16240_CHK_SUM        0x1E
> +
> +/* Calibration, x-axis acceleration offset adjustment */
> +#define ADIS16240_XACCL_OFF      0x20
> +
> +/* Calibration, y-axis acceleration offset adjustment */
> +#define ADIS16240_YACCL_OFF      0x22
> +
> +/* Calibration, z-axis acceleration offset adjustment */
> +#define ADIS16240_ZACCL_OFF      0x24
> +This looks like it could be converted to IIO_CHAN_INFO_SAMP_FREQ attribute.
> +/* Clock, hour and minute */
> +#define ADIS16240_CLK_TIME       0x2E
> +
> +/* Clock, month and day */
> +#define ADIS16240_CLK_DATE       0x30
> +
> +/* Clock, year */
> +#define ADIS16240_CLK_YEAR       0x32
> +
> +/* Wake-up setting, hour and minute */
> +#define ADIS16240_WAKE_TIME      0x34
> +
> +/* Wake-up setting, month and day */
> +#define ADIS16240_WAKE_DATE      0x36
> +
> +/* Alarm 1 amplitude threshold */
> +#define ADIS16240_ALM_MAG1       0x38
> +
> +/* Alarm 2 amplitude threshold */
> +#define ADIS16240_ALM_MAG2       0x3A
> +
> +/* Alarm control */
> +#define ADIS16240_ALM_CTRL       0x3C
> +
> +/* Capture, external trigger control */
> +#define ADIS16240_XTRIG_CTRL     0x3E
> +
> +/* Capture, address pointer */
> +#define ADIS16240_CAPT_PNTR      0x40
> +
> +/* Capture, configuration and control */
> +#define ADIS16240_CAPT_CTRL      0x42
> +
> +/* General-purpose digital input/output control */
> +#define ADIS16240_GPIO_CTRL      0x44
> +
> +/* Miscellaneous control */
> +#define ADIS16240_MSC_CTRL       0x46
> +
> +/* Internal sample period (rate) control */
> +#define ADIS16240_SMPL_PRD       0x48
> +
> +/* System command */
> +#define ADIS16240_GLOB_CMD       0x4A
> +
> +/* MSC_CTRL */
> +
> +/* Enables sum-of-squares output (XYZPEAK_OUT) */
> +#define ADIS16240_MSC_CTRL_XYZPEAK_OUT_EN	BIT(15)
> +
> +/* Enables peak tracking output (XPEAK_OUT, YPEAK_OUT, and ZPEAK_OUT) */
> +#define ADIS16240_MSC_CTRL_X_Y_ZPEAK_OUT_EN	BIT(14)
> +
> +/* Self-test enable: 1 = apply electrostatic force, 0 = disabled */
> +#define ADIS16240_MSC_CTRL_SELF_TEST_EN	        BIT(8)
> +
> +/* Data-ready enable: 1 = enabled, 0 = disabled */
> +#define ADIS16240_MSC_CTRL_DATA_RDY_EN	        BIT(2)
> +
> +/* Data-ready polarity: 1 = active high, 0 = active low */
> +#define ADIS16240_MSC_CTRL_ACTIVE_HIGH	        BIT(1)
> +
> +/* Data-ready line selection: 1 = DIO2, 0 = DIO1 */
> +#define ADIS16240_MSC_CTRL_DATA_RDY_DIO2	BIT(0)
> +
> +/* DIAG_STAT */
> +
> +/* Alarm 2 status: 1 = alarm active, 0 = alarm inactive */
> +#define ADIS16240_DIAG_STAT_ALARM2      BIT(9)
> +
> +/* Alarm 1 status: 1 = alarm active, 0 = alarm inactive */
> +#define ADIS16240_DIAG_STAT_ALARM1      BIT(8)
> +
> +/* Capture buffer full: 1 = capture buffer is full */
> +#define ADIS16240_DIAG_STAT_CPT_BUF_FUL BIT(7)
> +
> +/* Flash test, checksum flag: 1 = mismatch, 0 = match */
> +#define ADIS16240_DIAG_STAT_CHKSUM      BIT(6)
> +
> +/* Power-on, self-test flag: 1 = failure, 0 = pass */
> +#define ADIS16240_DIAG_STAT_PWRON_FAIL_BIT  5
> +
> +/* Power-on self-test: 1 = in-progress, 0 = complete */
> +#define ADIS16240_DIAG_STAT_PWRON_BUSY  BIT(4)
> +
> +/* SPI communications failure */
> +#define ADIS16240_DIAG_STAT_SPI_FAIL_BIT	3
> +
> +/* Flash update failure */
> +#define ADIS16240_DIAG_STAT_FLASH_UPT_BIT	2
> +
> +/* Power supply above 3.625 V */
> +#define ADIS16240_DIAG_STAT_POWER_HIGH_BIT	1
> +
> + /* Power supply below 2.225 V */
> +#define ADIS16240_DIAG_STAT_POWER_LOW_BIT	0
> +
> +/* GLOB_CMD */
> +
> +#define ADIS16240_GLOB_CMD_RESUME	BIT(8)
> +#define ADIS16240_GLOB_CMD_SW_RESET	BIT(7)
> +#define ADIS16240_GLOB_CMD_STANDBY	BIT(2)
> +
> +#define ADIS16240_ERROR_ACTIVE          BIT(14)
> +
> +/* At the moment triggers are only used for ring buffer
> + * filling. This may change!
> + */
> +
> +enum adis16240_scan {
> +	ADIS16240_SCAN_ACC_X,
> +	ADIS16240_SCAN_ACC_Y,
> +	ADIS16240_SCAN_ACC_Z,
> +	ADIS16240_SCAN_SUPPLY,
> +	ADIS16240_SCAN_AUX_ADC,
> +	ADIS16240_SCAN_TEMP,
> +};
> +
> +static ssize_t adis16240_spi_read_signed(struct device *dev,
> +					 struct device_attribute *attr,
> +					 char *buf,
> +					 unsigned int bits)
> +{
> +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> +	struct adis *st = iio_priv(indio_dev);
> +	int ret;
> +	s16 val = 0;
> +	unsigned int shift = 16 - bits;
> +	struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
> +
> +	ret = adis_read_reg_16(st,
> +			       this_attr->address, (u16 *)&val);
> +	if (ret)
> +		return ret;
> +
> +	if (val & ADIS16240_ERROR_ACTIVE)
> +		adis_check_status(st);
> +
> +	val = (s16)(val << shift) >> shift;
> +	return sprintf(buf, "%d\n", val);
> +}
> +
> +static ssize_t adis16240_read_12bit_signed(struct device *dev,
> +					   struct device_attribute *attr,
> +					   char *buf)
> +{
> +	return adis16240_spi_read_signed(dev, attr, buf, 12);
> +}
> +
> +static IIO_DEVICE_ATTR(in_accel_xyz_squared_peak_raw, 0444,
> +		       adis16240_read_12bit_signed, NULL,
> +		       ADIS16240_XYZPEAK_OUT);
> +
> +static IIO_CONST_ATTR_SAMP_FREQ_AVAIL("4096");
> +
> +static const u8 adis16240_addresses[][2] = {
> +	[ADIS16240_SCAN_ACC_X] = { ADIS16240_XACCL_OFF, ADIS16240_XPEAK_OUT },
> +	[ADIS16240_SCAN_ACC_Y] = { ADIS16240_YACCL_OFF, ADIS16240_YPEAK_OUT },
> +	[ADIS16240_SCAN_ACC_Z] = { ADIS16240_ZACCL_OFF, ADIS16240_ZPEAK_OUT },
> +};
> +
> +static int adis16240_read_raw(struct iio_dev *indio_dev,
> +			      struct iio_chan_spec const *chan,
> +			      int *val, int *val2,
> +			      long mask)
> +{
> +	struct adis *st = iio_priv(indio_dev);
> +	int ret;
> +	u8 addr;
> +	s16 val16;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		return adis_single_conversion(indio_dev, chan,
> +				ADIS16240_ERROR_ACTIVE, val);
> +	case IIO_CHAN_INFO_SCALE:
> +		switch (chan->type) {
> +		case IIO_VOLTAGE:
> +			if (chan->channel == 0) {
> +				*val = 4;
> +				*val2 = 880000; /* 4.88 mV */
> +				return IIO_VAL_INT_PLUS_MICRO;
> +			}
> +			return -EINVAL;
> +		case IIO_TEMP:
> +			*val = 244; /* 0.244 C */
> +			*val2 = 0;

This looks like a bug in the original driver.
I think *val = 0, and *val2 = 244 ?

> +			return IIO_VAL_INT_PLUS_MICRO;
> +		case IIO_ACCEL:
> +			*val = 0;
> +			*val2 = IIO_G_TO_M_S_2(51400); /* 51.4 mg */
> +			return IIO_VAL_INT_PLUS_MICRO;
> +		default:
> +			return -EINVAL;
> +		}
> +		break;
> +	case IIO_CHAN_INFO_PEAK_SCALE:
> +		*val = 0;
> +		*val2 = IIO_G_TO_M_S_2(51400); /* 51.4 mg */
> +		return IIO_VAL_INT_PLUS_MICRO;
> +	case IIO_CHAN_INFO_OFFSET:
> +		*val = 25000 / 244 - 0x133; /* 25 C = 0x133 */
> +		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_CALIBBIAS:
> +		addr = adis16240_addresses[chan->scan_index][0];
> +		ret = adis_read_reg_16(st, addr, &val16);
> +		if (ret)
> +			return ret;
> +		*val = sign_extend32(val16, 9);
> +		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_PEAK:
> +		addr = adis16240_addresses[chan->scan_index][1];
> +		ret = adis_read_reg_16(st, addr, &val16);
> +		if (ret)
> +			return ret;
> +		*val = sign_extend32(val16, 9);
> +		return IIO_VAL_INT;
> +	}
> +	return -EINVAL;
> +}
> +
> +static int adis16240_write_raw(struct iio_dev *indio_dev,
> +			       struct iio_chan_spec const *chan,
> +			       int val,
> +			       int val2,
> +			       long mask)
> +{
> +	struct adis *st = iio_priv(indio_dev);
> +	u8 addr;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_CALIBBIAS:
> +		addr = adis16240_addresses[chan->scan_index][0];
> +		return adis_write_reg_16(st, addr, val & GENMASK(9, 0));
> +	}
> +	return -EINVAL;
> +}
> +
> +static const struct iio_chan_spec adis16240_channels[] = {
> +	ADIS_SUPPLY_CHAN(ADIS16240_SUPPLY_OUT, ADIS16240_SCAN_SUPPLY, 0, 10),
> +	ADIS_AUX_ADC_CHAN(ADIS16240_AUX_ADC, ADIS16240_SCAN_AUX_ADC, 0, 10),
> +	ADIS_ACCEL_CHAN(X, ADIS16240_XACCL_OUT, ADIS16240_SCAN_ACC_X,
> +			BIT(IIO_CHAN_INFO_CALIBBIAS) | BIT(IIO_CHAN_INFO_PEAK),
> +			0, 10),
> +	ADIS_ACCEL_CHAN(Y, ADIS16240_YACCL_OUT, ADIS16240_SCAN_ACC_Y,
> +			BIT(IIO_CHAN_INFO_CALIBBIAS) | BIT(IIO_CHAN_INFO_PEAK),
> +			0, 10),
> +	ADIS_ACCEL_CHAN(Z, ADIS16240_ZACCL_OUT, ADIS16240_SCAN_ACC_Z,
> +			BIT(IIO_CHAN_INFO_CALIBBIAS) | BIT(IIO_CHAN_INFO_PEAK),
> +			0, 10),
> +	ADIS_TEMP_CHAN(ADIS16240_TEMP_OUT, ADIS16240_SCAN_TEMP, 0, 10),
> +	IIO_CHAN_SOFT_TIMESTAMP(6)
> +};
> +
> +static struct attribute *adis16240_attributes[] = {
> +	&iio_dev_attr_in_accel_xyz_squared_peak_raw.dev_attr.attr,


This one looks a bit weird/rare.
I am not sure about it's use-case/need right now (at this point in time).

Maybe we could just drop it?
Since I did not write the initial driver, I don't know yet what it was supposed to be/do.

In any case, if we want to add it, it looks like it needs an ABI documentation.


> +	&iio_const_attr_sampling_frequency_available.dev_attr.attr,

This looks like it could be converted to IIO_CHAN_INFO_SAMP_FREQ attribute.


> +	NULL
> +};
> +
> +static const struct attribute_group adis16240_attribute_group = {
> +	.attrs = adis16240_attributes,
> +};
> +
> +static const struct iio_info adis16240_info = {
> +	.attrs = &adis16240_attribute_group,
> +	.read_raw = adis16240_read_raw,
> +	.write_raw = adis16240_write_raw,
> +	.update_scan_mode = adis_update_scan_mode,
> +};
> +
> +static const char * const adis16240_status_error_msgs[] = {
> +	[ADIS16240_DIAG_STAT_PWRON_FAIL_BIT] = "Power on, self-test failed",
> +	[ADIS16240_DIAG_STAT_SPI_FAIL_BIT] = "SPI failure",
> +	[ADIS16240_DIAG_STAT_FLASH_UPT_BIT] = "Flash update failed",
> +	[ADIS16240_DIAG_STAT_POWER_HIGH_BIT] = "Power supply above 3.625V",
> +	[ADIS16240_DIAG_STAT_POWER_LOW_BIT] = "Power supply below 2.225V",
> +};
> +
> +static const struct adis_data adis16240_data = {
> +	.write_delay = 35,
> +	.read_delay = 35,
> +	.msc_ctrl_reg = ADIS16240_MSC_CTRL,
> +	.glob_cmd_reg = ADIS16240_GLOB_CMD,
> +	.diag_stat_reg = ADIS16240_DIAG_STAT,
> +
> +	.self_test_mask = ADIS16240_MSC_CTRL_SELF_TEST_EN,
> +	.self_test_no_autoclear = true,
> +	.startup_delay = ADIS16240_STARTUP_DELAY,
> +
> +	.status_error_msgs = adis16240_status_error_msgs,
> +	.status_error_mask = BIT(ADIS16240_DIAG_STAT_PWRON_FAIL_BIT) |
> +		BIT(ADIS16240_DIAG_STAT_SPI_FAIL_BIT) |
> +		BIT(ADIS16240_DIAG_STAT_FLASH_UPT_BIT) |
> +		BIT(ADIS16240_DIAG_STAT_POWER_HIGH_BIT) |
> +		BIT(ADIS16240_DIAG_STAT_POWER_LOW_BIT),
> +};
> +
> +static int adis16240_probe(struct spi_device *spi)
> +{
> +	int ret;
> +	struct adis *st;
> +	struct iio_dev *indio_dev;
> +
> +	/* setup the industrialio driver allocated elements */
> +	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +	st = iio_priv(indio_dev);
> +	/* this is only used for removal purposes */
> +	spi_set_drvdata(spi, indio_dev);
> +
> +	indio_dev->name = spi->dev.driver->name;
> +	indio_dev->dev.parent = &spi->dev;
> +	indio_dev->info = &adis16240_info;
> +	indio_dev->channels = adis16240_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(adis16240_channels);
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +
> +	ret = adis_init(st, indio_dev, spi, &adis16240_data);
> +	if (ret)
> +		return ret;
> +	ret = adis_setup_buffer_and_trigger(st, indio_dev, NULL);
> +	if (ret)
> +		return ret;
> +
> +	/* Get the device into a sane initial state */
> +	ret = adis_initial_startup(st);
> +	if (ret)
> +		goto error_cleanup_buffer_trigger;
> +	ret = iio_device_register(indio_dev);
> +	if (ret)
> +		goto error_cleanup_buffer_trigger;
> +	return 0;
> +
> +error_cleanup_buffer_trigger:
> +	adis_cleanup_buffer_and_trigger(st, indio_dev);
> +	return ret;
> +}
> +
> +static int adis16240_remove(struct spi_device *spi)
> +{
> +	struct iio_dev *indio_dev = spi_get_drvdata(spi);
> +	struct adis *st = iio_priv(indio_dev);
> +
> +	iio_device_unregister(indio_dev);
> +	adis_cleanup_buffer_and_trigger(st, indio_dev);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id adis16240_of_match[] = {
> +	{ .compatible = "adi,adis16240" },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, adis16240_of_match);
> +
> +static struct spi_driver adis16240_driver = {
> +	.driver = {
> +		.name = "adis16240",
> +		.of_match_table = adis16240_of_match,
> +	},
> +	.probe = adis16240_probe,
> +	.remove = adis16240_remove,
> +};
> +module_spi_driver(adis16240_driver);
> +
> +MODULE_AUTHOR("Barry Song <21cnbao@gmail.com>");
> +MODULE_DESCRIPTION("Analog Devices Programmable Impact Sensor and Recorder");
> +MODULE_LICENSE("GPL v2");
> +MODULE_ALIAS("spi:adis16240");
> diff --git a/drivers/staging/iio/accel/Kconfig b/drivers/staging/iio/accel/Kconfig
> index 3318997a7009..cee51f64bc4b 100644
> --- a/drivers/staging/iio/accel/Kconfig
> +++ b/drivers/staging/iio/accel/Kconfig
> @@ -16,16 +16,4 @@ config ADIS16203
>  	  To compile this driver as a module, say M here: the module will be
>  	  called adis16203.
>  
> -config ADIS16240
> -	tristate "Analog Devices ADIS16240 Programmable Impact Sensor and Recorder"
> -	depends on SPI
> -	select IIO_ADIS_LIB
> -	select IIO_ADIS_LIB_BUFFER if IIO_BUFFER
> -	help
> -	  Say Y here to build support for Analog Devices adis16240 programmable
> -	  impact Sensor and recorder.
> -
> -	  To compile this driver as a module, say M here: the module will be
> -	  called adis16240.
> -
>  endmenu
> diff --git a/drivers/staging/iio/accel/Makefile b/drivers/staging/iio/accel/Makefile
> index 094cc9be35bd..acac7bc9b9c0 100644
> --- a/drivers/staging/iio/accel/Makefile
> +++ b/drivers/staging/iio/accel/Makefile
> @@ -4,4 +4,3 @@
>  #
>  
>  obj-$(CONFIG_ADIS16203) += adis16203.o
> -obj-$(CONFIG_ADIS16240) += adis16240.o
> diff --git a/drivers/staging/iio/accel/adis16240.c b/drivers/staging/iio/accel/adis16240.c
> deleted file mode 100644
> index 82099db4bf0c..000000000000
> --- a/drivers/staging/iio/accel/adis16240.c
> +++ /dev/null
> @@ -1,454 +0,0 @@
> -// SPDX-License-Identifier: GPL-2.0+
> -/*
> - * ADIS16240 Programmable Impact Sensor and Recorder driver
> - *
> - * Copyright 2010 Analog Devices Inc.
> - */
> -
> -#include <linux/interrupt.h>
> -#include <linux/irq.h>
> -#include <linux/gpio.h>
> -#include <linux/delay.h>
> -#include <linux/device.h>
> -#include <linux/kernel.h>
> -#include <linux/spi/spi.h>
> -#include <linux/slab.h>
> -#include <linux/sysfs.h>
> -#include <linux/list.h>
> -#include <linux/module.h>
> -
> -#include <linux/iio/iio.h>
> -#include <linux/iio/sysfs.h>
> -#include <linux/iio/buffer.h>
> -#include <linux/iio/imu/adis.h>
> -
> -#define ADIS16240_STARTUP_DELAY	220 /* ms */
> -
> -/* Flash memory write count */
> -#define ADIS16240_FLASH_CNT      0x00
> -
> -/* Output, power supply */
> -#define ADIS16240_SUPPLY_OUT     0x02
> -
> -/* Output, x-axis accelerometer */
> -#define ADIS16240_XACCL_OUT      0x04
> -
> -/* Output, y-axis accelerometer */
> -#define ADIS16240_YACCL_OUT      0x06
> -
> -/* Output, z-axis accelerometer */
> -#define ADIS16240_ZACCL_OUT      0x08
> -
> -/* Output, auxiliary ADC input */
> -#define ADIS16240_AUX_ADC        0x0A
> -
> -/* Output, temperature */
> -#define ADIS16240_TEMP_OUT       0x0C
> -
> -/* Output, x-axis acceleration peak */
> -#define ADIS16240_XPEAK_OUT      0x0E
> -
> -/* Output, y-axis acceleration peak */
> -#define ADIS16240_YPEAK_OUT      0x10
> -
> -/* Output, z-axis acceleration peak */
> -#define ADIS16240_ZPEAK_OUT      0x12
> -
> -/* Output, sum-of-squares acceleration peak */
> -#define ADIS16240_XYZPEAK_OUT    0x14
> -
> -/* Output, Capture Buffer 1, X and Y acceleration */
> -#define ADIS16240_CAPT_BUF1      0x16
> -
> -/* Output, Capture Buffer 2, Z acceleration */
> -#define ADIS16240_CAPT_BUF2      0x18
> -
> -/* Diagnostic, error flags */
> -#define ADIS16240_DIAG_STAT      0x1A
> -
> -/* Diagnostic, event counter */
> -#define ADIS16240_EVNT_CNTR      0x1C
> -
> -/* Diagnostic, check sum value from firmware test */
> -#define ADIS16240_CHK_SUM        0x1E
> -
> -/* Calibration, x-axis acceleration offset adjustment */
> -#define ADIS16240_XACCL_OFF      0x20
> -
> -/* Calibration, y-axis acceleration offset adjustment */
> -#define ADIS16240_YACCL_OFF      0x22
> -
> -/* Calibration, z-axis acceleration offset adjustment */
> -#define ADIS16240_ZACCL_OFF      0x24
> -
> -/* Clock, hour and minute */
> -#define ADIS16240_CLK_TIME       0x2E
> -
> -/* Clock, month and day */
> -#define ADIS16240_CLK_DATE       0x30
> -
> -/* Clock, year */
> -#define ADIS16240_CLK_YEAR       0x32
> -
> -/* Wake-up setting, hour and minute */
> -#define ADIS16240_WAKE_TIME      0x34
> -
> -/* Wake-up setting, month and day */
> -#define ADIS16240_WAKE_DATE      0x36
> -
> -/* Alarm 1 amplitude threshold */
> -#define ADIS16240_ALM_MAG1       0x38
> -
> -/* Alarm 2 amplitude threshold */
> -#define ADIS16240_ALM_MAG2       0x3A
> -
> -/* Alarm control */
> -#define ADIS16240_ALM_CTRL       0x3C
> -
> -/* Capture, external trigger control */
> -#define ADIS16240_XTRIG_CTRL     0x3E
> -
> -/* Capture, address pointer */
> -#define ADIS16240_CAPT_PNTR      0x40
> -
> -/* Capture, configuration and control */
> -#define ADIS16240_CAPT_CTRL      0x42
> -
> -/* General-purpose digital input/output control */
> -#define ADIS16240_GPIO_CTRL      0x44
> -
> -/* Miscellaneous control */
> -#define ADIS16240_MSC_CTRL       0x46
> -
> -/* Internal sample period (rate) control */
> -#define ADIS16240_SMPL_PRD       0x48
> -
> -/* System command */
> -#define ADIS16240_GLOB_CMD       0x4A
> -
> -/* MSC_CTRL */
> -
> -/* Enables sum-of-squares output (XYZPEAK_OUT) */
> -#define ADIS16240_MSC_CTRL_XYZPEAK_OUT_EN	BIT(15)
> -
> -/* Enables peak tracking output (XPEAK_OUT, YPEAK_OUT, and ZPEAK_OUT) */
> -#define ADIS16240_MSC_CTRL_X_Y_ZPEAK_OUT_EN	BIT(14)
> -
> -/* Self-test enable: 1 = apply electrostatic force, 0 = disabled */
> -#define ADIS16240_MSC_CTRL_SELF_TEST_EN	        BIT(8)
> -
> -/* Data-ready enable: 1 = enabled, 0 = disabled */
> -#define ADIS16240_MSC_CTRL_DATA_RDY_EN	        BIT(2)
> -
> -/* Data-ready polarity: 1 = active high, 0 = active low */
> -#define ADIS16240_MSC_CTRL_ACTIVE_HIGH	        BIT(1)
> -
> -/* Data-ready line selection: 1 = DIO2, 0 = DIO1 */
> -#define ADIS16240_MSC_CTRL_DATA_RDY_DIO2	BIT(0)
> -
> -/* DIAG_STAT */
> -
> -/* Alarm 2 status: 1 = alarm active, 0 = alarm inactive */
> -#define ADIS16240_DIAG_STAT_ALARM2      BIT(9)
> -
> -/* Alarm 1 status: 1 = alarm active, 0 = alarm inactive */
> -#define ADIS16240_DIAG_STAT_ALARM1      BIT(8)
> -
> -/* Capture buffer full: 1 = capture buffer is full */
> -#define ADIS16240_DIAG_STAT_CPT_BUF_FUL BIT(7)
> -
> -/* Flash test, checksum flag: 1 = mismatch, 0 = match */
> -#define ADIS16240_DIAG_STAT_CHKSUM      BIT(6)
> -
> -/* Power-on, self-test flag: 1 = failure, 0 = pass */
> -#define ADIS16240_DIAG_STAT_PWRON_FAIL_BIT  5
> -
> -/* Power-on self-test: 1 = in-progress, 0 = complete */
> -#define ADIS16240_DIAG_STAT_PWRON_BUSY  BIT(4)
> -
> -/* SPI communications failure */
> -#define ADIS16240_DIAG_STAT_SPI_FAIL_BIT	3
> -
> -/* Flash update failure */
> -#define ADIS16240_DIAG_STAT_FLASH_UPT_BIT	2
> -
> -/* Power supply above 3.625 V */
> -#define ADIS16240_DIAG_STAT_POWER_HIGH_BIT	1
> -
> - /* Power supply below 2.225 V */
> -#define ADIS16240_DIAG_STAT_POWER_LOW_BIT	0
> -
> -/* GLOB_CMD */
> -
> -#define ADIS16240_GLOB_CMD_RESUME	BIT(8)
> -#define ADIS16240_GLOB_CMD_SW_RESET	BIT(7)
> -#define ADIS16240_GLOB_CMD_STANDBY	BIT(2)
> -
> -#define ADIS16240_ERROR_ACTIVE          BIT(14)
> -
> -/* At the moment triggers are only used for ring buffer
> - * filling. This may change!
> - */
> -
> -enum adis16240_scan {
> -	ADIS16240_SCAN_ACC_X,
> -	ADIS16240_SCAN_ACC_Y,
> -	ADIS16240_SCAN_ACC_Z,
> -	ADIS16240_SCAN_SUPPLY,
> -	ADIS16240_SCAN_AUX_ADC,
> -	ADIS16240_SCAN_TEMP,
> -};
> -
> -static ssize_t adis16240_spi_read_signed(struct device *dev,
> -					 struct device_attribute *attr,
> -					 char *buf,
> -					 unsigned int bits)
> -{
> -	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> -	struct adis *st = iio_priv(indio_dev);
> -	int ret;
> -	s16 val = 0;
> -	unsigned int shift = 16 - bits;
> -	struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
> -
> -	ret = adis_read_reg_16(st,
> -			       this_attr->address, (u16 *)&val);
> -	if (ret)
> -		return ret;
> -
> -	if (val & ADIS16240_ERROR_ACTIVE)
> -		adis_check_status(st);
> -
> -	val = (s16)(val << shift) >> shift;
> -	return sprintf(buf, "%d\n", val);
> -}
> -
> -static ssize_t adis16240_read_12bit_signed(struct device *dev,
> -					   struct device_attribute *attr,
> -					   char *buf)
> -{
> -	return adis16240_spi_read_signed(dev, attr, buf, 12);
> -}
> -
> -static IIO_DEVICE_ATTR(in_accel_xyz_squared_peak_raw, 0444,
> -		       adis16240_read_12bit_signed, NULL,
> -		       ADIS16240_XYZPEAK_OUT);
> -
> -static IIO_CONST_ATTR_SAMP_FREQ_AVAIL("4096");
> -
> -static const u8 adis16240_addresses[][2] = {
> -	[ADIS16240_SCAN_ACC_X] = { ADIS16240_XACCL_OFF, ADIS16240_XPEAK_OUT },
> -	[ADIS16240_SCAN_ACC_Y] = { ADIS16240_YACCL_OFF, ADIS16240_YPEAK_OUT },
> -	[ADIS16240_SCAN_ACC_Z] = { ADIS16240_ZACCL_OFF, ADIS16240_ZPEAK_OUT },
> -};
> -
> -static int adis16240_read_raw(struct iio_dev *indio_dev,
> -			      struct iio_chan_spec const *chan,
> -			      int *val, int *val2,
> -			      long mask)
> -{
> -	struct adis *st = iio_priv(indio_dev);
> -	int ret;
> -	u8 addr;
> -	s16 val16;
> -
> -	switch (mask) {
> -	case IIO_CHAN_INFO_RAW:
> -		return adis_single_conversion(indio_dev, chan,
> -				ADIS16240_ERROR_ACTIVE, val);
> -	case IIO_CHAN_INFO_SCALE:
> -		switch (chan->type) {
> -		case IIO_VOLTAGE:
> -			if (chan->channel == 0) {
> -				*val = 4;
> -				*val2 = 880000; /* 4.88 mV */
> -				return IIO_VAL_INT_PLUS_MICRO;
> -			}
> -			return -EINVAL;
> -		case IIO_TEMP:
> -			*val = 244; /* 0.244 C */
> -			*val2 = 0;
> -			return IIO_VAL_INT_PLUS_MICRO;
> -		case IIO_ACCEL:
> -			*val = 0;
> -			*val2 = IIO_G_TO_M_S_2(51400); /* 51.4 mg */
> -			return IIO_VAL_INT_PLUS_MICRO;
> -		default:
> -			return -EINVAL;
> -		}
> -		break;
> -	case IIO_CHAN_INFO_PEAK_SCALE:
> -		*val = 0;
> -		*val2 = IIO_G_TO_M_S_2(51400); /* 51.4 mg */
> -		return IIO_VAL_INT_PLUS_MICRO;
> -	case IIO_CHAN_INFO_OFFSET:
> -		*val = 25000 / 244 - 0x133; /* 25 C = 0x133 */
> -		return IIO_VAL_INT;
> -	case IIO_CHAN_INFO_CALIBBIAS:
> -		addr = adis16240_addresses[chan->scan_index][0];
> -		ret = adis_read_reg_16(st, addr, &val16);
> -		if (ret)
> -			return ret;
> -		*val = sign_extend32(val16, 9);
> -		return IIO_VAL_INT;
> -	case IIO_CHAN_INFO_PEAK:
> -		addr = adis16240_addresses[chan->scan_index][1];
> -		ret = adis_read_reg_16(st, addr, &val16);
> -		if (ret)
> -			return ret;
> -		*val = sign_extend32(val16, 9);
> -		return IIO_VAL_INT;
> -	}
> -	return -EINVAL;
> -}
> -
> -static int adis16240_write_raw(struct iio_dev *indio_dev,
> -			       struct iio_chan_spec const *chan,
> -			       int val,
> -			       int val2,
> -			       long mask)
> -{
> -	struct adis *st = iio_priv(indio_dev);
> -	u8 addr;
> -
> -	switch (mask) {
> -	case IIO_CHAN_INFO_CALIBBIAS:
> -		addr = adis16240_addresses[chan->scan_index][0];
> -		return adis_write_reg_16(st, addr, val & GENMASK(9, 0));
> -	}
> -	return -EINVAL;
> -}
> -
> -static const struct iio_chan_spec adis16240_channels[] = {
> -	ADIS_SUPPLY_CHAN(ADIS16240_SUPPLY_OUT, ADIS16240_SCAN_SUPPLY, 0, 10),
> -	ADIS_AUX_ADC_CHAN(ADIS16240_AUX_ADC, ADIS16240_SCAN_AUX_ADC, 0, 10),
> -	ADIS_ACCEL_CHAN(X, ADIS16240_XACCL_OUT, ADIS16240_SCAN_ACC_X,
> -			BIT(IIO_CHAN_INFO_CALIBBIAS) | BIT(IIO_CHAN_INFO_PEAK),
> -			0, 10),
> -	ADIS_ACCEL_CHAN(Y, ADIS16240_YACCL_OUT, ADIS16240_SCAN_ACC_Y,
> -			BIT(IIO_CHAN_INFO_CALIBBIAS) | BIT(IIO_CHAN_INFO_PEAK),
> -			0, 10),
> -	ADIS_ACCEL_CHAN(Z, ADIS16240_ZACCL_OUT, ADIS16240_SCAN_ACC_Z,
> -			BIT(IIO_CHAN_INFO_CALIBBIAS) | BIT(IIO_CHAN_INFO_PEAK),
> -			0, 10),
> -	ADIS_TEMP_CHAN(ADIS16240_TEMP_OUT, ADIS16240_SCAN_TEMP, 0, 10),
> -	IIO_CHAN_SOFT_TIMESTAMP(6)
> -};
> -
> -static struct attribute *adis16240_attributes[] = {
> -	&iio_dev_attr_in_accel_xyz_squared_peak_raw.dev_attr.attr,
> -	&iio_const_attr_sampling_frequency_available.dev_attr.attr,
> -	NULL
> -};
> -
> -static const struct attribute_group adis16240_attribute_group = {
> -	.attrs = adis16240_attributes,
> -};
> -
> -static const struct iio_info adis16240_info = {
> -	.attrs = &adis16240_attribute_group,
> -	.read_raw = adis16240_read_raw,
> -	.write_raw = adis16240_write_raw,
> -	.update_scan_mode = adis_update_scan_mode,
> -};
> -
> -static const char * const adis16240_status_error_msgs[] = {
> -	[ADIS16240_DIAG_STAT_PWRON_FAIL_BIT] = "Power on, self-test failed",
> -	[ADIS16240_DIAG_STAT_SPI_FAIL_BIT] = "SPI failure",
> -	[ADIS16240_DIAG_STAT_FLASH_UPT_BIT] = "Flash update failed",
> -	[ADIS16240_DIAG_STAT_POWER_HIGH_BIT] = "Power supply above 3.625V",
> -	[ADIS16240_DIAG_STAT_POWER_LOW_BIT] = "Power supply below 2.225V",
> -};
> -
> -static const struct adis_data adis16240_data = {
> -	.write_delay = 35,
> -	.read_delay = 35,
> -	.msc_ctrl_reg = ADIS16240_MSC_CTRL,
> -	.glob_cmd_reg = ADIS16240_GLOB_CMD,
> -	.diag_stat_reg = ADIS16240_DIAG_STAT,
> -
> -	.self_test_mask = ADIS16240_MSC_CTRL_SELF_TEST_EN,
> -	.self_test_no_autoclear = true,
> -	.startup_delay = ADIS16240_STARTUP_DELAY,
> -
> -	.status_error_msgs = adis16240_status_error_msgs,
> -	.status_error_mask = BIT(ADIS16240_DIAG_STAT_PWRON_FAIL_BIT) |
> -		BIT(ADIS16240_DIAG_STAT_SPI_FAIL_BIT) |
> -		BIT(ADIS16240_DIAG_STAT_FLASH_UPT_BIT) |
> -		BIT(ADIS16240_DIAG_STAT_POWER_HIGH_BIT) |
> -		BIT(ADIS16240_DIAG_STAT_POWER_LOW_BIT),
> -};
> -
> -static int adis16240_probe(struct spi_device *spi)
> -{
> -	int ret;
> -	struct adis *st;
> -	struct iio_dev *indio_dev;
> -
> -	/* setup the industrialio driver allocated elements */
> -	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
> -	if (!indio_dev)
> -		return -ENOMEM;
> -	st = iio_priv(indio_dev);
> -	/* this is only used for removal purposes */
> -	spi_set_drvdata(spi, indio_dev);
> -
> -	indio_dev->name = spi->dev.driver->name;
> -	indio_dev->dev.parent = &spi->dev;
> -	indio_dev->info = &adis16240_info;
> -	indio_dev->channels = adis16240_channels;
> -	indio_dev->num_channels = ARRAY_SIZE(adis16240_channels);
> -	indio_dev->modes = INDIO_DIRECT_MODE;
> -
> -	ret = adis_init(st, indio_dev, spi, &adis16240_data);
> -	if (ret)
> -		return ret;
> -	ret = adis_setup_buffer_and_trigger(st, indio_dev, NULL);
> -	if (ret)
> -		return ret;
> -
> -	/* Get the device into a sane initial state */
> -	ret = adis_initial_startup(st);
> -	if (ret)
> -		goto error_cleanup_buffer_trigger;
> -	ret = iio_device_register(indio_dev);
> -	if (ret)
> -		goto error_cleanup_buffer_trigger;
> -	return 0;
> -
> -error_cleanup_buffer_trigger:
> -	adis_cleanup_buffer_and_trigger(st, indio_dev);
> -	return ret;
> -}
> -
> -static int adis16240_remove(struct spi_device *spi)
> -{
> -	struct iio_dev *indio_dev = spi_get_drvdata(spi);
> -	struct adis *st = iio_priv(indio_dev);
> -
> -	iio_device_unregister(indio_dev);
> -	adis_cleanup_buffer_and_trigger(st, indio_dev);
> -
> -	return 0;
> -}
> -
> -static const struct of_device_id adis16240_of_match[] = {
> -	{ .compatible = "adi,adis16240" },
> -	{ },
> -};
> -MODULE_DEVICE_TABLE(of, adis16240_of_match);
> -
> -static struct spi_driver adis16240_driver = {
> -	.driver = {
> -		.name = "adis16240",
> -		.of_match_table = adis16240_of_match,
> -	},
> -	.probe = adis16240_probe,
> -	.remove = adis16240_remove,
> -};
> -module_spi_driver(adis16240_driver);
> -
> -MODULE_AUTHOR("Barry Song <21cnbao@gmail.com>");
> -MODULE_DESCRIPTION("Analog Devices Programmable Impact Sensor and Recorder");
> -MODULE_LICENSE("GPL v2");
> -MODULE_ALIAS("spi:adis16240");

Re: [PATCH 2/2] staging: iio: accel: adis16240: move out of staging

[Jonathan Cameron]

On Mon, 2 Sep 2019 13:26:02 +0000
"Ardelean, Alexandru" <alexandru.Ardelean@analog.com> wrote:

> On Sun, 2019-09-01 at 21:59 -0300, Rodrigo Carvalho wrote:
> > Move ADIS16240 driver from staging to mainline.
> > 
> > The ADIS16240 is a fully integrated digital shock detection
> > and recorder system.  
> 
> 
> Hey,
> 
> Comments inline.
> 
> I'll probably take a look in the next days again.
> There seem to be some ABI/sysfs attributes that need to be resolved before moving this out of staging.

Absolutely. It is a 'new' type of device so there are definitely some
corners that need discussing before we move out of staging and commit
to maintaining the ABI moving forwards.

That is the real reason this driver was still in staging!  No one
had been through the process of proposing the ABI and responding to
questions etc.

The issue with impact sensors has always been that they don't really fit
our normal model for buffers or triggers.

So normally a trigger (if exposed in IIO) is used as one trigger
causes 1 set of samples (so like a frame trigger for a camera).

These devices tend to work in a mode where one trigger causes data
to be captured for a period of time.  In this part that's the event
recorder function

No one is realistically going to buy an impact sensor to just use it
as an accelerometer which is what this driver is currently doing.
I suppose we could just leave support in that form for now, but
I'm no sure how much use it is to anyone.

Analog Devices people, worth working out how to support the event
recorder?  For that someone needs to have hardware as it is complex
to say the least!

We could move it out but might be worth adding a comment somewhere
saying this only really supports direct access to channels, and
not the event recorder functionality.


Jonathan


> 
> > 
> > Signed-off-by: Rodrigo Ribeiro Carvalho <rodrigorsdc@gmail.com>
> > ---
> >  drivers/iio/accel/Kconfig             |  12 +
> >  drivers/iio/accel/Makefile            |   1 +
> >  drivers/iio/accel/adis16240.c         | 454 ++++++++++++++++++++++++++
> >  drivers/staging/iio/accel/Kconfig     |  12 -
> >  drivers/staging/iio/accel/Makefile    |   1 -
> >  drivers/staging/iio/accel/adis16240.c | 454 --------------------------
> >  6 files changed, 467 insertions(+), 467 deletions(-)
> >  create mode 100644 drivers/iio/accel/adis16240.c
> >  delete mode 100644 drivers/staging/iio/accel/adis16240.c  
> 
> Looks like MAINTAINERS file also needs to be updated, also with the DT bindings file.
> I think checkpatch usually complains about these.
> 
> > 
> > diff --git a/drivers/iio/accel/Kconfig b/drivers/iio/accel/Kconfig
> > index d4ef35aeb579..91fd8741c95f 100644
> > --- a/drivers/iio/accel/Kconfig
> > +++ b/drivers/iio/accel/Kconfig
> > @@ -30,6 +30,18 @@ config ADIS16209
> >  	  To compile this driver as a module, say M here: the module will be
> >  	  called adis16209.
> >  
> > +config ADIS16240
> > +	tristate "Analog Devices ADIS16240 Programmable Impact Sensor and Recorder"
> > +	depends on SPI
> > +	select IIO_ADIS_LIB
> > +	select IIO_ADIS_LIB_BUFFER if IIO_BUFFER
> > +	help
> > +	  Say Y here to build support for Analog Devices adis16240 programmable
> > +	  impact Sensor and recorder.
> > +
> > +	  To compile this driver as a module, say M here: the module will be
> > +	  called adis16240.
> > +	  
> >  config ADXL345
> >  	tristate
> >  
> > diff --git a/drivers/iio/accel/Makefile b/drivers/iio/accel/Makefile
> > index 56bd0215e0d4..f7e025a86dd9 100644
> > --- a/drivers/iio/accel/Makefile
> > +++ b/drivers/iio/accel/Makefile
> > @@ -6,6 +6,7 @@
> >  # When adding new entries keep the list in alphabetical order
> >  obj-$(CONFIG_ADIS16201) += adis16201.o
> >  obj-$(CONFIG_ADIS16209) += adis16209.o
> > +obj-$(CONFIG_ADIS16240) += adis16240.o
> >  obj-$(CONFIG_ADXL345) += adxl345_core.o
> >  obj-$(CONFIG_ADXL345_I2C) += adxl345_i2c.o
> >  obj-$(CONFIG_ADXL345_SPI) += adxl345_spi.o
> > diff --git a/drivers/iio/accel/adis16240.c b/drivers/iio/accel/adis16240.c
> > new file mode 100644
> > index 000000000000..82099db4bf0c
> > --- /dev/null
> > +++ b/drivers/iio/accel/adis16240.c
> > @@ -0,0 +1,454 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * ADIS16240 Programmable Impact Sensor and Recorder driver
> > + *
> > + * Copyright 2010 Analog Devices Inc.
> > + */
> > +
> > +#include <linux/interrupt.h>
> > +#include <linux/irq.h>
> > +#include <linux/gpio.h>
> > +#include <linux/delay.h>
> > +#include <linux/device.h>
> > +#include <linux/kernel.h>
> > +#include <linux/spi/spi.h>
> > +#include <linux/slab.h>
> > +#include <linux/sysfs.h>
> > +#include <linux/list.h>
> > +#include <linux/module.h>
> > +
> > +#include <linux/iio/iio.h>
> > +#include <linux/iio/sysfs.h>
> > +#include <linux/iio/buffer.h>
> > +#include <linux/iio/imu/adis.h>
> > +
> > +#define ADIS16240_STARTUP_DELAY	220 /* ms */
> > +
> > +/* Flash memory write count */
> > +#define ADIS16240_FLASH_CNT      0x00
> > +
> > +/* Output, power supply */
> > +#define ADIS16240_SUPPLY_OUT     0x02
> > +
> > +/* Output, x-axis accelerometer */
> > +#define ADIS16240_XACCL_OUT      0x04
> > +
> > +/* Output, y-axis accelerometer */
> > +#define ADIS16240_YACCL_OUT      0x06
> > +
> > +/* Output, z-axis accelerometer */
> > +#define ADIS16240_ZACCL_OUT      0x08
> > +
> > +/* Output, auxiliary ADC input */
> > +#define ADIS16240_AUX_ADC        0x0A
> > +
> > +/* Output, temperature */
> > +#define ADIS16240_TEMP_OUT       0x0C
> > +
> > +/* Output, x-axis acceleration peak */
> > +#define ADIS16240_XPEAK_OUT      0x0E
> > +
> > +/* Output, y-axis acceleration peak */
> > +#define ADIS16240_YPEAK_OUT      0x10
> > +
> > +/* Output, z-axis acceleration peak */
> > +#define ADIS16240_ZPEAK_OUT      0x12
> > +
> > +/* Output, sum-of-squares acceleration peak */
> > +#define ADIS16240_XYZPEAK_OUT    0x14
> > +
> > +/* Output, Capture Buffer 1, X and Y acceleration */
> > +#define ADIS16240_CAPT_BUF1      0x16
> > +
> > +/* Output, Capture Buffer 2, Z acceleration */
> > +#define ADIS16240_CAPT_BUF2      0x18
> > +
> > +/* Diagnostic, error flags */
> > +#define ADIS16240_DIAG_STAT      0x1A
> > +This looks like it could be converted to IIO_CHAN_INFO_SAMP_FREQ attribute.
> > +/* Diagnostic, event counter */
> > +#define ADIS16240_EVNT_CNTR      0x1C
> > +
> > +/* Diagnostic, check sum value from firmware test */
> > +#define ADIS16240_CHK_SUM        0x1E
> > +
> > +/* Calibration, x-axis acceleration offset adjustment */
> > +#define ADIS16240_XACCL_OFF      0x20
> > +
> > +/* Calibration, y-axis acceleration offset adjustment */
> > +#define ADIS16240_YACCL_OFF      0x22
> > +
> > +/* Calibration, z-axis acceleration offset adjustment */
> > +#define ADIS16240_ZACCL_OFF      0x24
> > +This looks like it could be converted to IIO_CHAN_INFO_SAMP_FREQ attribute.
> > +/* Clock, hour and minute */
> > +#define ADIS16240_CLK_TIME       0x2E
> > +
> > +/* Clock, month and day */
> > +#define ADIS16240_CLK_DATE       0x30
> > +
> > +/* Clock, year */
> > +#define ADIS16240_CLK_YEAR       0x32
> > +
> > +/* Wake-up setting, hour and minute */
> > +#define ADIS16240_WAKE_TIME      0x34
> > +
> > +/* Wake-up setting, month and day */
> > +#define ADIS16240_WAKE_DATE      0x36
> > +
> > +/* Alarm 1 amplitude threshold */
> > +#define ADIS16240_ALM_MAG1       0x38
> > +
> > +/* Alarm 2 amplitude threshold */
> > +#define ADIS16240_ALM_MAG2       0x3A
> > +
> > +/* Alarm control */
> > +#define ADIS16240_ALM_CTRL       0x3C
> > +
> > +/* Capture, external trigger control */
> > +#define ADIS16240_XTRIG_CTRL     0x3E
> > +
> > +/* Capture, address pointer */
> > +#define ADIS16240_CAPT_PNTR      0x40
> > +
> > +/* Capture, configuration and control */
> > +#define ADIS16240_CAPT_CTRL      0x42
> > +
> > +/* General-purpose digital input/output control */
> > +#define ADIS16240_GPIO_CTRL      0x44
> > +
> > +/* Miscellaneous control */
> > +#define ADIS16240_MSC_CTRL       0x46
> > +
> > +/* Internal sample period (rate) control */
> > +#define ADIS16240_SMPL_PRD       0x48
> > +
> > +/* System command */
> > +#define ADIS16240_GLOB_CMD       0x4A
> > +
> > +/* MSC_CTRL */
> > +
> > +/* Enables sum-of-squares output (XYZPEAK_OUT) */
> > +#define ADIS16240_MSC_CTRL_XYZPEAK_OUT_EN	BIT(15)
> > +
> > +/* Enables peak tracking output (XPEAK_OUT, YPEAK_OUT, and ZPEAK_OUT) */
> > +#define ADIS16240_MSC_CTRL_X_Y_ZPEAK_OUT_EN	BIT(14)
> > +
> > +/* Self-test enable: 1 = apply electrostatic force, 0 = disabled */
> > +#define ADIS16240_MSC_CTRL_SELF_TEST_EN	        BIT(8)
> > +
> > +/* Data-ready enable: 1 = enabled, 0 = disabled */
> > +#define ADIS16240_MSC_CTRL_DATA_RDY_EN	        BIT(2)
> > +
> > +/* Data-ready polarity: 1 = active high, 0 = active low */
> > +#define ADIS16240_MSC_CTRL_ACTIVE_HIGH	        BIT(1)
> > +
> > +/* Data-ready line selection: 1 = DIO2, 0 = DIO1 */
> > +#define ADIS16240_MSC_CTRL_DATA_RDY_DIO2	BIT(0)
> > +
> > +/* DIAG_STAT */
> > +
> > +/* Alarm 2 status: 1 = alarm active, 0 = alarm inactive */
> > +#define ADIS16240_DIAG_STAT_ALARM2      BIT(9)
> > +
> > +/* Alarm 1 status: 1 = alarm active, 0 = alarm inactive */
> > +#define ADIS16240_DIAG_STAT_ALARM1      BIT(8)
> > +
> > +/* Capture buffer full: 1 = capture buffer is full */
> > +#define ADIS16240_DIAG_STAT_CPT_BUF_FUL BIT(7)
> > +
> > +/* Flash test, checksum flag: 1 = mismatch, 0 = match */
> > +#define ADIS16240_DIAG_STAT_CHKSUM      BIT(6)
> > +
> > +/* Power-on, self-test flag: 1 = failure, 0 = pass */
> > +#define ADIS16240_DIAG_STAT_PWRON_FAIL_BIT  5
> > +
> > +/* Power-on self-test: 1 = in-progress, 0 = complete */
> > +#define ADIS16240_DIAG_STAT_PWRON_BUSY  BIT(4)
> > +
> > +/* SPI communications failure */
> > +#define ADIS16240_DIAG_STAT_SPI_FAIL_BIT	3
> > +
> > +/* Flash update failure */
> > +#define ADIS16240_DIAG_STAT_FLASH_UPT_BIT	2
> > +
> > +/* Power supply above 3.625 V */
> > +#define ADIS16240_DIAG_STAT_POWER_HIGH_BIT	1
> > +
> > + /* Power supply below 2.225 V */
> > +#define ADIS16240_DIAG_STAT_POWER_LOW_BIT	0
> > +
> > +/* GLOB_CMD */
> > +
> > +#define ADIS16240_GLOB_CMD_RESUME	BIT(8)
> > +#define ADIS16240_GLOB_CMD_SW_RESET	BIT(7)
> > +#define ADIS16240_GLOB_CMD_STANDBY	BIT(2)
> > +
> > +#define ADIS16240_ERROR_ACTIVE          BIT(14)
> > +
> > +/* At the moment triggers are only used for ring buffer
> > + * filling. This may change!
> > + */
> > +
> > +enum adis16240_scan {
> > +	ADIS16240_SCAN_ACC_X,
> > +	ADIS16240_SCAN_ACC_Y,
> > +	ADIS16240_SCAN_ACC_Z,
> > +	ADIS16240_SCAN_SUPPLY,
> > +	ADIS16240_SCAN_AUX_ADC,
> > +	ADIS16240_SCAN_TEMP,
> > +};
> > +
> > +static ssize_t adis16240_spi_read_signed(struct device *dev,
> > +					 struct device_attribute *attr,
> > +					 char *buf,
> > +					 unsigned int bits)
> > +{
> > +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> > +	struct adis *st = iio_priv(indio_dev);
> > +	int ret;
> > +	s16 val = 0;
> > +	unsigned int shift = 16 - bits;
> > +	struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
> > +
> > +	ret = adis_read_reg_16(st,
> > +			       this_attr->address, (u16 *)&val);

Check for odd line breaks.  That looks like it'll fit under 80 chars
just fine.

> > +	if (ret)
> > +		return ret;
> > +
> > +	if (val & ADIS16240_ERROR_ACTIVE)
> > +		adis_check_status(st);
> > +
> > +	val = (s16)(val << shift) >> shift;

Use sign_extend32 (added after this driver I think!)

> > +	return sprintf(buf, "%d\n", val);
> > +}
> > +
> > +static ssize_t adis16240_read_12bit_signed(struct device *dev,
> > +					   struct device_attribute *attr,
> > +					   char *buf)
> > +{
> > +	return adis16240_spi_read_signed(dev, attr, buf, 12);
> > +}
> > +
> > +static IIO_DEVICE_ATTR(in_accel_xyz_squared_peak_raw, 0444,
> > +		       adis16240_read_12bit_signed, NULL,
> > +		       ADIS16240_XYZPEAK_OUT);
> > +
> > +static IIO_CONST_ATTR_SAMP_FREQ_AVAIL("4096");
> > +
> > +static const u8 adis16240_addresses[][2] = {
> > +	[ADIS16240_SCAN_ACC_X] = { ADIS16240_XACCL_OFF, ADIS16240_XPEAK_OUT },
> > +	[ADIS16240_SCAN_ACC_Y] = { ADIS16240_YACCL_OFF, ADIS16240_YPEAK_OUT },
> > +	[ADIS16240_SCAN_ACC_Z] = { ADIS16240_ZACCL_OFF, ADIS16240_ZPEAK_OUT },
> > +};
> > +
> > +static int adis16240_read_raw(struct iio_dev *indio_dev,
> > +			      struct iio_chan_spec const *chan,
> > +			      int *val, int *val2,
> > +			      long mask)
> > +{
> > +	struct adis *st = iio_priv(indio_dev);
> > +	int ret;
> > +	u8 addr;
> > +	s16 val16;
> > +
> > +	switch (mask) {
> > +	case IIO_CHAN_INFO_RAW:
> > +		return adis_single_conversion(indio_dev, chan,
> > +				ADIS16240_ERROR_ACTIVE, val);
> > +	case IIO_CHAN_INFO_SCALE:
> > +		switch (chan->type) {
> > +		case IIO_VOLTAGE:
> > +			if (chan->channel == 0) {
> > +				*val = 4;
> > +				*val2 = 880000; /* 4.88 mV */
> > +				return IIO_VAL_INT_PLUS_MICRO;
> > +			}
> > +			return -EINVAL;
> > +		case IIO_TEMP:
> > +			*val = 244; /* 0.244 C */
> > +			*val2 = 0;  
> 
> This looks like a bug in the original driver.
> I think *val = 0, and *val2 = 244 ?
> 
> > +			return IIO_VAL_INT_PLUS_MICRO;
> > +		case IIO_ACCEL:
> > +			*val = 0;
> > +			*val2 = IIO_G_TO_M_S_2(51400); /* 51.4 mg */
> > +			return IIO_VAL_INT_PLUS_MICRO;
> > +		default:
> > +			return -EINVAL;
> > +		}
> > +		break;
> > +	case IIO_CHAN_INFO_PEAK_SCALE:
> > +		*val = 0;
> > +		*val2 = IIO_G_TO_M_S_2(51400); /* 51.4 mg */
> > +		return IIO_VAL_INT_PLUS_MICRO;
> > +	case IIO_CHAN_INFO_OFFSET:
> > +		*val = 25000 / 244 - 0x133; /* 25 C = 0x133 */
> > +		return IIO_VAL_INT;
> > +	case IIO_CHAN_INFO_CALIBBIAS:
> > +		addr = adis16240_addresses[chan->scan_index][0];
> > +		ret = adis_read_reg_16(st, addr, &val16);
> > +		if (ret)
> > +			return ret;
> > +		*val = sign_extend32(val16, 9);
> > +		return IIO_VAL_INT;
> > +	case IIO_CHAN_INFO_PEAK:
> > +		addr = adis16240_addresses[chan->scan_index][1];
> > +		ret = adis_read_reg_16(st, addr, &val16);
> > +		if (ret)
> > +			return ret;
> > +		*val = sign_extend32(val16, 9);
> > +		return IIO_VAL_INT;
> > +	}
> > +	return -EINVAL;
> > +}
> > +
> > +static int adis16240_write_raw(struct iio_dev *indio_dev,
> > +			       struct iio_chan_spec const *chan,
> > +			       int val,
> > +			       int val2,
> > +			       long mask)
> > +{
> > +	struct adis *st = iio_priv(indio_dev);
> > +	u8 addr;
> > +
> > +	switch (mask) {
> > +	case IIO_CHAN_INFO_CALIBBIAS:
> > +		addr = adis16240_addresses[chan->scan_index][0];
> > +		return adis_write_reg_16(st, addr, val & GENMASK(9, 0));
> > +	}
> > +	return -EINVAL;
> > +}
> > +
> > +static const struct iio_chan_spec adis16240_channels[] = {
> > +	ADIS_SUPPLY_CHAN(ADIS16240_SUPPLY_OUT, ADIS16240_SCAN_SUPPLY, 0, 10),
> > +	ADIS_AUX_ADC_CHAN(ADIS16240_AUX_ADC, ADIS16240_SCAN_AUX_ADC, 0, 10),
> > +	ADIS_ACCEL_CHAN(X, ADIS16240_XACCL_OUT, ADIS16240_SCAN_ACC_X,
> > +			BIT(IIO_CHAN_INFO_CALIBBIAS) | BIT(IIO_CHAN_INFO_PEAK),
> > +			0, 10),
> > +	ADIS_ACCEL_CHAN(Y, ADIS16240_YACCL_OUT, ADIS16240_SCAN_ACC_Y,
> > +			BIT(IIO_CHAN_INFO_CALIBBIAS) | BIT(IIO_CHAN_INFO_PEAK),
> > +			0, 10),
> > +	ADIS_ACCEL_CHAN(Z, ADIS16240_ZACCL_OUT, ADIS16240_SCAN_ACC_Z,
> > +			BIT(IIO_CHAN_INFO_CALIBBIAS) | BIT(IIO_CHAN_INFO_PEAK),
> > +			0, 10),
> > +	ADIS_TEMP_CHAN(ADIS16240_TEMP_OUT, ADIS16240_SCAN_TEMP, 0, 10),
> > +	IIO_CHAN_SOFT_TIMESTAMP(6)
> > +};
> > +
> > +static struct attribute *adis16240_attributes[] = {
> > +	&iio_dev_attr_in_accel_xyz_squared_peak_raw.dev_attr.attr,  
> 
> 
> This one looks a bit weird/rare.
> I am not sure about it's use-case/need right now (at this point in time).
> 
> Maybe we could just drop it?
> Since I did not write the initial driver, I don't know yet what it was supposed to be/do.

For an impact sensor it's a fairly useful thing I suspect as it is giving the maximum
acceleration in any direction that occured (once you square root it in userspace.)

> 
> In any case, if we want to add it, it looks like it needs an ABI documentation.

Agreed. Needs docs.  We actually have a modifier for it so can be handled
as a channel. IIO_MOD_SUM_SQUARED_X_Y_Z :)


> 
> 
> > +	&iio_const_attr_sampling_frequency_available.dev_attr.attr,  
> 
> This looks like it could be converted to IIO_CHAN_INFO_SAMP_FREQ attribute.
> 
> 
> > +	NULL
> > +};
> > +
> > +static const struct attribute_group adis16240_attribute_group = {
> > +	.attrs = adis16240_attributes,
> > +};
> > +
> > +static const struct iio_info adis16240_info = {
> > +	.attrs = &adis16240_attribute_group,
> > +	.read_raw = adis16240_read_raw,
> > +	.write_raw = adis16240_write_raw,
> > +	.update_scan_mode = adis_update_scan_mode,
> > +};
> > +
> > +static const char * const adis16240_status_error_msgs[] = {
> > +	[ADIS16240_DIAG_STAT_PWRON_FAIL_BIT] = "Power on, self-test failed",
> > +	[ADIS16240_DIAG_STAT_SPI_FAIL_BIT] = "SPI failure",
> > +	[ADIS16240_DIAG_STAT_FLASH_UPT_BIT] = "Flash update failed",
> > +	[ADIS16240_DIAG_STAT_POWER_HIGH_BIT] = "Power supply above 3.625V",
> > +	[ADIS16240_DIAG_STAT_POWER_LOW_BIT] = "Power supply below 2.225V",
> > +};
> > +
> > +static const struct adis_data adis16240_data = {
> > +	.write_delay = 35,
> > +	.read_delay = 35,
> > +	.msc_ctrl_reg = ADIS16240_MSC_CTRL,
> > +	.glob_cmd_reg = ADIS16240_GLOB_CMD,
> > +	.diag_stat_reg = ADIS16240_DIAG_STAT,
> > +
> > +	.self_test_mask = ADIS16240_MSC_CTRL_SELF_TEST_EN,
> > +	.self_test_no_autoclear = true,
> > +	.startup_delay = ADIS16240_STARTUP_DELAY,
> > +
> > +	.status_error_msgs = adis16240_status_error_msgs,
> > +	.status_error_mask = BIT(ADIS16240_DIAG_STAT_PWRON_FAIL_BIT) |
> > +		BIT(ADIS16240_DIAG_STAT_SPI_FAIL_BIT) |
> > +		BIT(ADIS16240_DIAG_STAT_FLASH_UPT_BIT) |
> > +		BIT(ADIS16240_DIAG_STAT_POWER_HIGH_BIT) |
> > +		BIT(ADIS16240_DIAG_STAT_POWER_LOW_BIT),
> > +};
> > +
> > +static int adis16240_probe(struct spi_device *spi)
> > +{
> > +	int ret;
> > +	struct adis *st;
> > +	struct iio_dev *indio_dev;
> > +
> > +	/* setup the industrialio driver allocated elements */

I would review comments and decide whether they actually add anything.
This one for example - I'm not convinced!

> > +	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
> > +	if (!indio_dev)
> > +		return -ENOMEM;
> > +	st = iio_priv(indio_dev);
> > +	/* this is only used for removal purposes */
> > +	spi_set_drvdata(spi, indio_dev);
> > +
> > +	indio_dev->name = spi->dev.driver->name;
> > +	indio_dev->dev.parent = &spi->dev;
> > +	indio_dev->info = &adis16240_info;
> > +	indio_dev->channels = adis16240_channels;
> > +	indio_dev->num_channels = ARRAY_SIZE(adis16240_channels);
> > +	indio_dev->modes = INDIO_DIRECT_MODE;
> > +
> > +	ret = adis_init(st, indio_dev, spi, &adis16240_data);
> > +	if (ret)
> > +		return ret;

Might be nice to look at adding some white space after error handling
blocks like this.  A blank line in locations like this make it
easier for the eye to follow the code.

> > +	ret = adis_setup_buffer_and_trigger(st, indio_dev, NULL);
> > +	if (ret)
> > +		return ret;
> > +
> > +	/* Get the device into a sane initial state */
> > +	ret = adis_initial_startup(st);
> > +	if (ret)
> > +		goto error_cleanup_buffer_trigger;
> > +	ret = iio_device_register(indio_dev);
> > +	if (ret)
> > +		goto error_cleanup_buffer_trigger;
> > +	return 0;
> > +
> > +error_cleanup_buffer_trigger:
> > +	adis_cleanup_buffer_and_trigger(st, indio_dev);
> > +	return ret;
> > +}
> > +
> > +static int adis16240_remove(struct spi_device *spi)
> > +{
> > +	struct iio_dev *indio_dev = spi_get_drvdata(spi);
> > +	struct adis *st = iio_priv(indio_dev);
> > +
> > +	iio_device_unregister(indio_dev);
> > +	adis_cleanup_buffer_and_trigger(st, indio_dev);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct of_device_id adis16240_of_match[] = {
> > +	{ .compatible = "adi,adis16240" },
> > +	{ },
> > +};
> > +MODULE_DEVICE_TABLE(of, adis16240_of_match);
> > +
> > +static struct spi_driver adis16240_driver = {
> > +	.driver = {
> > +		.name = "adis16240",
> > +		.of_match_table = adis16240_of_match,
> > +	},
> > +	.probe = adis16240_probe,
> > +	.remove = adis16240_remove,
> > +};
> > +module_spi_driver(adis16240_driver);
> > +
> > +MODULE_AUTHOR("Barry Song <21cnbao@gmail.com>");
> > +MODULE_DESCRIPTION("Analog Devices Programmable Impact Sensor and Recorder");
> > +MODULE_LICENSE("GPL v2");
> > +MODULE_ALIAS("spi:adis16240");
...

Re: [PATCH 2/2] staging: iio: accel: adis16240: move out of staging

[Ardelean, Alexandru]

On Sun, 2019-09-08 at 12:09 +0100, Jonathan Cameron wrote:
> On Mon, 2 Sep 2019 13:26:02 +0000
> "Ardelean, Alexandru" <alexandru.Ardelean@analog.com> wrote:
> 
> > On Sun, 2019-09-01 at 21:59 -0300, Rodrigo Carvalho wrote:
> > > Move ADIS16240 driver from staging to mainline.
> > > 
> > > The ADIS16240 is a fully integrated digital shock detection
> > > and recorder system.  
> > 
> > Hey,
> > 
> > Comments inline.
> > 
> > I'll probably take a look in the next days again.
> > There seem to be some ABI/sysfs attributes that need to be resolved before moving this out of staging.
> 
> Absolutely. It is a 'new' type of device so there are definitely some
> corners that need discussing before we move out of staging and commit
> to maintaining the ABI moving forwards.
> 
> That is the real reason this driver was still in staging!  No one
> had been through the process of proposing the ABI and responding to
> questions etc.
> 
> The issue with impact sensors has always been that they don't really fit
> our normal model for buffers or triggers.
> 
> So normally a trigger (if exposed in IIO) is used as one trigger
> causes 1 set of samples (so like a frame trigger for a camera).
> 
> These devices tend to work in a mode where one trigger causes data
> to be captured for a period of time.  In this part that's the event
> recorder function
> 
> No one is realistically going to buy an impact sensor to just use it
> as an accelerometer which is what this driver is currently doing.
> I suppose we could just leave support in that form for now, but
> I'm no sure how much use it is to anyone.
> 
> Analog Devices people, worth working out how to support the event
> recorder?  For that someone needs to have hardware as it is complex
> to say the least!

Worth it: yes.
But we don't have any resources to allocate for this [at this point in time].

> 
> We could move it out but might be worth adding a comment somewhere
> saying this only really supports direct access to channels, and
> not the event recorder functionality.
> 

I guess, I would vote for leaving it in staging.
It's also a way to mark it as a work-in-progress/not-done/still-needs-something kind of thing.
If we move it now, it gets the status of "everything-resolved" which is not yet the case.

Thanks for the insight/background info.
Much of it was discussed before my time.

> 
> Jonathan

Alex

> 
> 
> > > Signed-off-by: Rodrigo Ribeiro Carvalho <rodrigorsdc@gmail.com>
> > > ---
> > >  drivers/iio/accel/Kconfig             |  12 +
> > >  drivers/iio/accel/Makefile            |   1 +
> > >  drivers/iio/accel/adis16240.c         | 454 ++++++++++++++++++++++++++
> > >  drivers/staging/iio/accel/Kconfig     |  12 -
> > >  drivers/staging/iio/accel/Makefile    |   1 -
> > >  drivers/staging/iio/accel/adis16240.c | 454 --------------------------
> > >  6 files changed, 467 insertions(+), 467 deletions(-)
> > >  create mode 100644 drivers/iio/accel/adis16240.c
> > >  delete mode 100644 drivers/staging/iio/accel/adis16240.c  
> > 
> > Looks like MAINTAINERS file also needs to be updated, also with the DT bindings file.
> > I think checkpatch usually complains about these.
> > 
> > > diff --git a/drivers/iio/accel/Kconfig b/drivers/iio/accel/Kconfig
> > > index d4ef35aeb579..91fd8741c95f 100644
> > > --- a/drivers/iio/accel/Kconfig
> > > +++ b/drivers/iio/accel/Kconfig
> > > @@ -30,6 +30,18 @@ config ADIS16209
> > >  	  To compile this driver as a module, say M here: the module will be
> > >  	  called adis16209.
> > >  
> > > +config ADIS16240
> > > +	tristate "Analog Devices ADIS16240 Programmable Impact Sensor and Recorder"
> > > +	depends on SPI
> > > +	select IIO_ADIS_LIB
> > > +	select IIO_ADIS_LIB_BUFFER if IIO_BUFFER
> > > +	help
> > > +	  Say Y here to build support for Analog Devices adis16240 programmable
> > > +	  impact Sensor and recorder.
> > > +
> > > +	  To compile this driver as a module, say M here: the module will be
> > > +	  called adis16240.
> > > +	  
> > >  config ADXL345
> > >  	tristate
> > >  
> > > diff --git a/drivers/iio/accel/Makefile b/drivers/iio/accel/Makefile
> > > index 56bd0215e0d4..f7e025a86dd9 100644
> > > --- a/drivers/iio/accel/Makefile
> > > +++ b/drivers/iio/accel/Makefile
> > > @@ -6,6 +6,7 @@
> > >  # When adding new entries keep the list in alphabetical order
> > >  obj-$(CONFIG_ADIS16201) += adis16201.o
> > >  obj-$(CONFIG_ADIS16209) += adis16209.o
> > > +obj-$(CONFIG_ADIS16240) += adis16240.o
> > >  obj-$(CONFIG_ADXL345) += adxl345_core.o
> > >  obj-$(CONFIG_ADXL345_I2C) += adxl345_i2c.o
> > >  obj-$(CONFIG_ADXL345_SPI) += adxl345_spi.o
> > > diff --git a/drivers/iio/accel/adis16240.c b/drivers/iio/accel/adis16240.c
> > > new file mode 100644
> > > index 000000000000..82099db4bf0c
> > > --- /dev/null
> > > +++ b/drivers/iio/accel/adis16240.c
> > > @@ -0,0 +1,454 @@
> > > +// SPDX-License-Identifier: GPL-2.0+
> > > +/*
> > > + * ADIS16240 Programmable Impact Sensor and Recorder driver
> > > + *
> > > + * Copyright 2010 Analog Devices Inc.
> > > + */
> > > +
> > > +#include <linux/interrupt.h>
> > > +#include <linux/irq.h>
> > > +#include <linux/gpio.h>
> > > +#include <linux/delay.h>
> > > +#include <linux/device.h>
> > > +#include <linux/kernel.h>
> > > +#include <linux/spi/spi.h>
> > > +#include <linux/slab.h>
> > > +#include <linux/sysfs.h>
> > > +#include <linux/list.h>
> > > +#include <linux/module.h>
> > > +
> > > +#include <linux/iio/iio.h>
> > > +#include <linux/iio/sysfs.h>
> > > +#include <linux/iio/buffer.h>
> > > +#include <linux/iio/imu/adis.h>
> > > +
> > > +#define ADIS16240_STARTUP_DELAY	220 /* ms */
> > > +
> > > +/* Flash memory write count */
> > > +#define ADIS16240_FLASH_CNT      0x00
> > > +
> > > +/* Output, power supply */
> > > +#define ADIS16240_SUPPLY_OUT     0x02
> > > +
> > > +/* Output, x-axis accelerometer */
> > > +#define ADIS16240_XACCL_OUT      0x04
> > > +
> > > +/* Output, y-axis accelerometer */
> > > +#define ADIS16240_YACCL_OUT      0x06
> > > +
> > > +/* Output, z-axis accelerometer */
> > > +#define ADIS16240_ZACCL_OUT      0x08
> > > +
> > > +/* Output, auxiliary ADC input */
> > > +#define ADIS16240_AUX_ADC        0x0A
> > > +
> > > +/* Output, temperature */
> > > +#define ADIS16240_TEMP_OUT       0x0C
> > > +
> > > +/* Output, x-axis acceleration peak */
> > > +#define ADIS16240_XPEAK_OUT      0x0E
> > > +
> > > +/* Output, y-axis acceleration peak */
> > > +#define ADIS16240_YPEAK_OUT      0x10
> > > +
> > > +/* Output, z-axis acceleration peak */
> > > +#define ADIS16240_ZPEAK_OUT      0x12
> > > +
> > > +/* Output, sum-of-squares acceleration peak */
> > > +#define ADIS16240_XYZPEAK_OUT    0x14
> > > +
> > > +/* Output, Capture Buffer 1, X and Y acceleration */
> > > +#define ADIS16240_CAPT_BUF1      0x16
> > > +
> > > +/* Output, Capture Buffer 2, Z acceleration */
> > > +#define ADIS16240_CAPT_BUF2      0x18
> > > +
> > > +/* Diagnostic, error flags */
> > > +#define ADIS16240_DIAG_STAT      0x1A
> > > +This looks like it could be converted to IIO_CHAN_INFO_SAMP_FREQ attribute.
> > > +/* Diagnostic, event counter */
> > > +#define ADIS16240_EVNT_CNTR      0x1C
> > > +
> > > +/* Diagnostic, check sum value from firmware test */
> > > +#define ADIS16240_CHK_SUM        0x1E
> > > +
> > > +/* Calibration, x-axis acceleration offset adjustment */
> > > +#define ADIS16240_XACCL_OFF      0x20
> > > +
> > > +/* Calibration, y-axis acceleration offset adjustment */
> > > +#define ADIS16240_YACCL_OFF      0x22
> > > +
> > > +/* Calibration, z-axis acceleration offset adjustment */
> > > +#define ADIS16240_ZACCL_OFF      0x24
> > > +This looks like it could be converted to IIO_CHAN_INFO_SAMP_FREQ attribute.
> > > +/* Clock, hour and minute */
> > > +#define ADIS16240_CLK_TIME       0x2E
> > > +
> > > +/* Clock, month and day */
> > > +#define ADIS16240_CLK_DATE       0x30
> > > +
> > > +/* Clock, year */
> > > +#define ADIS16240_CLK_YEAR       0x32
> > > +
> > > +/* Wake-up setting, hour and minute */
> > > +#define ADIS16240_WAKE_TIME      0x34
> > > +
> > > +/* Wake-up setting, month and day */
> > > +#define ADIS16240_WAKE_DATE      0x36
> > > +
> > > +/* Alarm 1 amplitude threshold */
> > > +#define ADIS16240_ALM_MAG1       0x38
> > > +
> > > +/* Alarm 2 amplitude threshold */
> > > +#define ADIS16240_ALM_MAG2       0x3A
> > > +
> > > +/* Alarm control */
> > > +#define ADIS16240_ALM_CTRL       0x3C
> > > +
> > > +/* Capture, external trigger control */
> > > +#define ADIS16240_XTRIG_CTRL     0x3E
> > > +
> > > +/* Capture, address pointer */
> > > +#define ADIS16240_CAPT_PNTR      0x40
> > > +
> > > +/* Capture, configuration and control */
> > > +#define ADIS16240_CAPT_CTRL      0x42
> > > +
> > > +/* General-purpose digital input/output control */
> > > +#define ADIS16240_GPIO_CTRL      0x44
> > > +
> > > +/* Miscellaneous control */
> > > +#define ADIS16240_MSC_CTRL       0x46
> > > +
> > > +/* Internal sample period (rate) control */
> > > +#define ADIS16240_SMPL_PRD       0x48
> > > +
> > > +/* System command */
> > > +#define ADIS16240_GLOB_CMD       0x4A
> > > +
> > > +/* MSC_CTRL */
> > > +
> > > +/* Enables sum-of-squares output (XYZPEAK_OUT) */
> > > +#define ADIS16240_MSC_CTRL_XYZPEAK_OUT_EN	BIT(15)
> > > +
> > > +/* Enables peak tracking output (XPEAK_OUT, YPEAK_OUT, and ZPEAK_OUT) */
> > > +#define ADIS16240_MSC_CTRL_X_Y_ZPEAK_OUT_EN	BIT(14)
> > > +
> > > +/* Self-test enable: 1 = apply electrostatic force, 0 = disabled */
> > > +#define ADIS16240_MSC_CTRL_SELF_TEST_EN	        BIT(8)
> > > +
> > > +/* Data-ready enable: 1 = enabled, 0 = disabled */
> > > +#define ADIS16240_MSC_CTRL_DATA_RDY_EN	        BIT(2)
> > > +
> > > +/* Data-ready polarity: 1 = active high, 0 = active low */
> > > +#define ADIS16240_MSC_CTRL_ACTIVE_HIGH	        BIT(1)
> > > +
> > > +/* Data-ready line selection: 1 = DIO2, 0 = DIO1 */
> > > +#define ADIS16240_MSC_CTRL_DATA_RDY_DIO2	BIT(0)
> > > +
> > > +/* DIAG_STAT */
> > > +
> > > +/* Alarm 2 status: 1 = alarm active, 0 = alarm inactive */
> > > +#define ADIS16240_DIAG_STAT_ALARM2      BIT(9)
> > > +
> > > +/* Alarm 1 status: 1 = alarm active, 0 = alarm inactive */
> > > +#define ADIS16240_DIAG_STAT_ALARM1      BIT(8)
> > > +
> > > +/* Capture buffer full: 1 = capture buffer is full */
> > > +#define ADIS16240_DIAG_STAT_CPT_BUF_FUL BIT(7)
> > > +
> > > +/* Flash test, checksum flag: 1 = mismatch, 0 = match */
> > > +#define ADIS16240_DIAG_STAT_CHKSUM      BIT(6)
> > > +
> > > +/* Power-on, self-test flag: 1 = failure, 0 = pass */
> > > +#define ADIS16240_DIAG_STAT_PWRON_FAIL_BIT  5
> > > +
> > > +/* Power-on self-test: 1 = in-progress, 0 = complete */
> > > +#define ADIS16240_DIAG_STAT_PWRON_BUSY  BIT(4)
> > > +
> > > +/* SPI communications failure */
> > > +#define ADIS16240_DIAG_STAT_SPI_FAIL_BIT	3
> > > +
> > > +/* Flash update failure */
> > > +#define ADIS16240_DIAG_STAT_FLASH_UPT_BIT	2
> > > +
> > > +/* Power supply above 3.625 V */
> > > +#define ADIS16240_DIAG_STAT_POWER_HIGH_BIT	1
> > > +
> > > + /* Power supply below 2.225 V */
> > > +#define ADIS16240_DIAG_STAT_POWER_LOW_BIT	0
> > > +
> > > +/* GLOB_CMD */
> > > +
> > > +#define ADIS16240_GLOB_CMD_RESUME	BIT(8)
> > > +#define ADIS16240_GLOB_CMD_SW_RESET	BIT(7)
> > > +#define ADIS16240_GLOB_CMD_STANDBY	BIT(2)
> > > +
> > > +#define ADIS16240_ERROR_ACTIVE          BIT(14)
> > > +
> > > +/* At the moment triggers are only used for ring buffer
> > > + * filling. This may change!
> > > + */
> > > +
> > > +enum adis16240_scan {
> > > +	ADIS16240_SCAN_ACC_X,
> > > +	ADIS16240_SCAN_ACC_Y,
> > > +	ADIS16240_SCAN_ACC_Z,
> > > +	ADIS16240_SCAN_SUPPLY,
> > > +	ADIS16240_SCAN_AUX_ADC,
> > > +	ADIS16240_SCAN_TEMP,
> > > +};
> > > +
> > > +static ssize_t adis16240_spi_read_signed(struct device *dev,
> > > +					 struct device_attribute *attr,
> > > +					 char *buf,
> > > +					 unsigned int bits)
> > > +{
> > > +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> > > +	struct adis *st = iio_priv(indio_dev);
> > > +	int ret;
> > > +	s16 val = 0;
> > > +	unsigned int shift = 16 - bits;
> > > +	struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
> > > +
> > > +	ret = adis_read_reg_16(st,
> > > +			       this_attr->address, (u16 *)&val);
> 
> Check for odd line breaks.  That looks like it'll fit under 80 chars
> just fine.
> 
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	if (val & ADIS16240_ERROR_ACTIVE)
> > > +		adis_check_status(st);
> > > +
> > > +	val = (s16)(val << shift) >> shift;
> 
> Use sign_extend32 (added after this driver I think!)
> 
> > > +	return sprintf(buf, "%d\n", val);
> > > +}
> > > +
> > > +static ssize_t adis16240_read_12bit_signed(struct device *dev,
> > > +					   struct device_attribute *attr,
> > > +					   char *buf)
> > > +{
> > > +	return adis16240_spi_read_signed(dev, attr, buf, 12);
> > > +}
> > > +
> > > +static IIO_DEVICE_ATTR(in_accel_xyz_squared_peak_raw, 0444,
> > > +		       adis16240_read_12bit_signed, NULL,
> > > +		       ADIS16240_XYZPEAK_OUT);
> > > +
> > > +static IIO_CONST_ATTR_SAMP_FREQ_AVAIL("4096");
> > > +
> > > +static const u8 adis16240_addresses[][2] = {
> > > +	[ADIS16240_SCAN_ACC_X] = { ADIS16240_XACCL_OFF, ADIS16240_XPEAK_OUT },
> > > +	[ADIS16240_SCAN_ACC_Y] = { ADIS16240_YACCL_OFF, ADIS16240_YPEAK_OUT },
> > > +	[ADIS16240_SCAN_ACC_Z] = { ADIS16240_ZACCL_OFF, ADIS16240_ZPEAK_OUT },
> > > +};
> > > +
> > > +static int adis16240_read_raw(struct iio_dev *indio_dev,
> > > +			      struct iio_chan_spec const *chan,
> > > +			      int *val, int *val2,
> > > +			      long mask)
> > > +{
> > > +	struct adis *st = iio_priv(indio_dev);
> > > +	int ret;
> > > +	u8 addr;
> > > +	s16 val16;
> > > +
> > > +	switch (mask) {
> > > +	case IIO_CHAN_INFO_RAW:
> > > +		return adis_single_conversion(indio_dev, chan,
> > > +				ADIS16240_ERROR_ACTIVE, val);
> > > +	case IIO_CHAN_INFO_SCALE:
> > > +		switch (chan->type) {
> > > +		case IIO_VOLTAGE:
> > > +			if (chan->channel == 0) {
> > > +				*val = 4;
> > > +				*val2 = 880000; /* 4.88 mV */
> > > +				return IIO_VAL_INT_PLUS_MICRO;
> > > +			}
> > > +			return -EINVAL;
> > > +		case IIO_TEMP:
> > > +			*val = 244; /* 0.244 C */
> > > +			*val2 = 0;  
> > 
> > This looks like a bug in the original driver.
> > I think *val = 0, and *val2 = 244 ?
> > 
> > > +			return IIO_VAL_INT_PLUS_MICRO;
> > > +		case IIO_ACCEL:
> > > +			*val = 0;
> > > +			*val2 = IIO_G_TO_M_S_2(51400); /* 51.4 mg */
> > > +			return IIO_VAL_INT_PLUS_MICRO;
> > > +		default:
> > > +			return -EINVAL;
> > > +		}
> > > +		break;
> > > +	case IIO_CHAN_INFO_PEAK_SCALE:
> > > +		*val = 0;
> > > +		*val2 = IIO_G_TO_M_S_2(51400); /* 51.4 mg */
> > > +		return IIO_VAL_INT_PLUS_MICRO;
> > > +	case IIO_CHAN_INFO_OFFSET:
> > > +		*val = 25000 / 244 - 0x133; /* 25 C = 0x133 */
> > > +		return IIO_VAL_INT;
> > > +	case IIO_CHAN_INFO_CALIBBIAS:
> > > +		addr = adis16240_addresses[chan->scan_index][0];
> > > +		ret = adis_read_reg_16(st, addr, &val16);
> > > +		if (ret)
> > > +			return ret;
> > > +		*val = sign_extend32(val16, 9);
> > > +		return IIO_VAL_INT;
> > > +	case IIO_CHAN_INFO_PEAK:
> > > +		addr = adis16240_addresses[chan->scan_index][1];
> > > +		ret = adis_read_reg_16(st, addr, &val16);
> > > +		if (ret)
> > > +			return ret;
> > > +		*val = sign_extend32(val16, 9);
> > > +		return IIO_VAL_INT;
> > > +	}
> > > +	return -EINVAL;
> > > +}
> > > +
> > > +static int adis16240_write_raw(struct iio_dev *indio_dev,
> > > +			       struct iio_chan_spec const *chan,
> > > +			       int val,
> > > +			       int val2,
> > > +			       long mask)
> > > +{
> > > +	struct adis *st = iio_priv(indio_dev);
> > > +	u8 addr;
> > > +
> > > +	switch (mask) {
> > > +	case IIO_CHAN_INFO_CALIBBIAS:
> > > +		addr = adis16240_addresses[chan->scan_index][0];
> > > +		return adis_write_reg_16(st, addr, val & GENMASK(9, 0));
> > > +	}
> > > +	return -EINVAL;
> > > +}
> > > +
> > > +static const struct iio_chan_spec adis16240_channels[] = {
> > > +	ADIS_SUPPLY_CHAN(ADIS16240_SUPPLY_OUT, ADIS16240_SCAN_SUPPLY, 0, 10),
> > > +	ADIS_AUX_ADC_CHAN(ADIS16240_AUX_ADC, ADIS16240_SCAN_AUX_ADC, 0, 10),
> > > +	ADIS_ACCEL_CHAN(X, ADIS16240_XACCL_OUT, ADIS16240_SCAN_ACC_X,
> > > +			BIT(IIO_CHAN_INFO_CALIBBIAS) | BIT(IIO_CHAN_INFO_PEAK),
> > > +			0, 10),
> > > +	ADIS_ACCEL_CHAN(Y, ADIS16240_YACCL_OUT, ADIS16240_SCAN_ACC_Y,
> > > +			BIT(IIO_CHAN_INFO_CALIBBIAS) | BIT(IIO_CHAN_INFO_PEAK),
> > > +			0, 10),
> > > +	ADIS_ACCEL_CHAN(Z, ADIS16240_ZACCL_OUT, ADIS16240_SCAN_ACC_Z,
> > > +			BIT(IIO_CHAN_INFO_CALIBBIAS) | BIT(IIO_CHAN_INFO_PEAK),
> > > +			0, 10),
> > > +	ADIS_TEMP_CHAN(ADIS16240_TEMP_OUT, ADIS16240_SCAN_TEMP, 0, 10),
> > > +	IIO_CHAN_SOFT_TIMESTAMP(6)
> > > +};
> > > +
> > > +static struct attribute *adis16240_attributes[] = {
> > > +	&iio_dev_attr_in_accel_xyz_squared_peak_raw.dev_attr.attr,  
> > 
> > This one looks a bit weird/rare.
> > I am not sure about it's use-case/need right now (at this point in time).
> > 
> > Maybe we could just drop it?
> > Since I did not write the initial driver, I don't know yet what it was supposed to be/do.
> 
> For an impact sensor it's a fairly useful thing I suspect as it is giving the maximum
> acceleration in any direction that occured (once you square root it in userspace.)
> 
> > In any case, if we want to add it, it looks like it needs an ABI documentation.
> 
> Agreed. Needs docs.  We actually have a modifier for it so can be handled
> as a channel. IIO_MOD_SUM_SQUARED_X_Y_Z :)
> 
> 
> > 
> > > +	&iio_const_attr_sampling_frequency_available.dev_attr.attr,  
> > 
> > This looks like it could be converted to IIO_CHAN_INFO_SAMP_FREQ attribute.
> > 
> > 
> > > +	NULL
> > > +};
> > > +
> > > +static const struct attribute_group adis16240_attribute_group = {
> > > +	.attrs = adis16240_attributes,
> > > +};
> > > +
> > > +static const struct iio_info adis16240_info = {
> > > +	.attrs = &adis16240_attribute_group,
> > > +	.read_raw = adis16240_read_raw,
> > > +	.write_raw = adis16240_write_raw,
> > > +	.update_scan_mode = adis_update_scan_mode,
> > > +};
> > > +
> > > +static const char * const adis16240_status_error_msgs[] = {
> > > +	[ADIS16240_DIAG_STAT_PWRON_FAIL_BIT] = "Power on, self-test failed",
> > > +	[ADIS16240_DIAG_STAT_SPI_FAIL_BIT] = "SPI failure",
> > > +	[ADIS16240_DIAG_STAT_FLASH_UPT_BIT] = "Flash update failed",
> > > +	[ADIS16240_DIAG_STAT_POWER_HIGH_BIT] = "Power supply above 3.625V",
> > > +	[ADIS16240_DIAG_STAT_POWER_LOW_BIT] = "Power supply below 2.225V",
> > > +};
> > > +
> > > +static const struct adis_data adis16240_data = {
> > > +	.write_delay = 35,
> > > +	.read_delay = 35,
> > > +	.msc_ctrl_reg = ADIS16240_MSC_CTRL,
> > > +	.glob_cmd_reg = ADIS16240_GLOB_CMD,
> > > +	.diag_stat_reg = ADIS16240_DIAG_STAT,
> > > +
> > > +	.self_test_mask = ADIS16240_MSC_CTRL_SELF_TEST_EN,
> > > +	.self_test_no_autoclear = true,
> > > +	.startup_delay = ADIS16240_STARTUP_DELAY,
> > > +
> > > +	.status_error_msgs = adis16240_status_error_msgs,
> > > +	.status_error_mask = BIT(ADIS16240_DIAG_STAT_PWRON_FAIL_BIT) |
> > > +		BIT(ADIS16240_DIAG_STAT_SPI_FAIL_BIT) |
> > > +		BIT(ADIS16240_DIAG_STAT_FLASH_UPT_BIT) |
> > > +		BIT(ADIS16240_DIAG_STAT_POWER_HIGH_BIT) |
> > > +		BIT(ADIS16240_DIAG_STAT_POWER_LOW_BIT),
> > > +};
> > > +
> > > +static int adis16240_probe(struct spi_device *spi)
> > > +{
> > > +	int ret;
> > > +	struct adis *st;
> > > +	struct iio_dev *indio_dev;
> > > +
> > > +	/* setup the industrialio driver allocated elements */
> 
> I would review comments and decide whether they actually add anything.
> This one for example - I'm not convinced!
> 
> > > +	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
> > > +	if (!indio_dev)
> > > +		return -ENOMEM;
> > > +	st = iio_priv(indio_dev);
> > > +	/* this is only used for removal purposes */
> > > +	spi_set_drvdata(spi, indio_dev);
> > > +
> > > +	indio_dev->name = spi->dev.driver->name;
> > > +	indio_dev->dev.parent = &spi->dev;
> > > +	indio_dev->info = &adis16240_info;
> > > +	indio_dev->channels = adis16240_channels;
> > > +	indio_dev->num_channels = ARRAY_SIZE(adis16240_channels);
> > > +	indio_dev->modes = INDIO_DIRECT_MODE;
> > > +
> > > +	ret = adis_init(st, indio_dev, spi, &adis16240_data);
> > > +	if (ret)
> > > +		return ret;
> 
> Might be nice to look at adding some white space after error handling
> blocks like this.  A blank line in locations like this make it
> easier for the eye to follow the code.
> 
> > > +	ret = adis_setup_buffer_and_trigger(st, indio_dev, NULL);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	/* Get the device into a sane initial state */
> > > +	ret = adis_initial_startup(st);
> > > +	if (ret)
> > > +		goto error_cleanup_buffer_trigger;
> > > +	ret = iio_device_register(indio_dev);
> > > +	if (ret)
> > > +		goto error_cleanup_buffer_trigger;
> > > +	return 0;
> > > +
> > > +error_cleanup_buffer_trigger:
> > > +	adis_cleanup_buffer_and_trigger(st, indio_dev);
> > > +	return ret;
> > > +}
> > > +
> > > +static int adis16240_remove(struct spi_device *spi)
> > > +{
> > > +	struct iio_dev *indio_dev = spi_get_drvdata(spi);
> > > +	struct adis *st = iio_priv(indio_dev);
> > > +
> > > +	iio_device_unregister(indio_dev);
> > > +	adis_cleanup_buffer_and_trigger(st, indio_dev);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static const struct of_device_id adis16240_of_match[] = {
> > > +	{ .compatible = "adi,adis16240" },
> > > +	{ },
> > > +};
> > > +MODULE_DEVICE_TABLE(of, adis16240_of_match);
> > > +
> > > +static struct spi_driver adis16240_driver = {
> > > +	.driver = {
> > > +		.name = "adis16240",
> > > +		.of_match_table = adis16240_of_match,
> > > +	},
> > > +	.probe = adis16240_probe,
> > > +	.remove = adis16240_remove,
> > > +};
> > > +module_spi_driver(adis16240_driver);
> > > +
> > > +MODULE_AUTHOR("Barry Song <21cnbao@gmail.com>");
> > > +MODULE_DESCRIPTION("Analog Devices Programmable Impact Sensor and Recorder");
> > > +MODULE_LICENSE("GPL v2");
> > > +MODULE_ALIAS("spi:adis16240");
> ...

Re: [PATCH 2/2] staging: iio: accel: adis16240: move out of staging

[Rodrigo Carvalho]

Hi,

Em seg, 9 de set de 2019 às 02:53, Ardelean, Alexandru
<alexandru.Ardelean@analog.com> escreveu:
>
> On Sun, 2019-09-08 at 12:09 +0100, Jonathan Cameron wrote:
> > On Mon, 2 Sep 2019 13:26:02 +0000
> > "Ardelean, Alexandru" <alexandru.Ardelean@analog.com> wrote:
> >
> > > On Sun, 2019-09-01 at 21:59 -0300, Rodrigo Carvalho wrote:
> > > > Move ADIS16240 driver from staging to mainline.
> > > >
> > > > The ADIS16240 is a fully integrated digital shock detection
> > > > and recorder system.
> > >
> > > Hey,
> > >
> > > Comments inline.
> > >
> > > I'll probably take a look in the next days again.
> > > There seem to be some ABI/sysfs attributes that need to be resolved before moving this out of staging.
> >
> > Absolutely. It is a 'new' type of device so there are definitely some
> > corners that need discussing before we move out of staging and commit
> > to maintaining the ABI moving forwards.
> >
> > That is the real reason this driver was still in staging!  No one
> > had been through the process of proposing the ABI and responding to
> > questions etc.
> >
> > The issue with impact sensors has always been that they don't really fit
> > our normal model for buffers or triggers.
> >
> > So normally a trigger (if exposed in IIO) is used as one trigger
> > causes 1 set of samples (so like a frame trigger for a camera).
> >
> > These devices tend to work in a mode where one trigger causes data
> > to be captured for a period of time.  In this part that's the event
> > recorder function
> >
> > No one is realistically going to buy an impact sensor to just use it
> > as an accelerometer which is what this driver is currently doing.
> > I suppose we could just leave support in that form for now, but
> > I'm no sure how much use it is to anyone.
> >
> > Analog Devices people, worth working out how to support the event
> > recorder?  For that someone needs to have hardware as it is complex
> > to say the least!
>
> Worth it: yes.
> But we don't have any resources to allocate for this [at this point in time].
>
> >
> > We could move it out but might be worth adding a comment somewhere
> > saying this only really supports direct access to channels, and
> > not the event recorder functionality.
> >
>
> I guess, I would vote for leaving it in staging.
> It's also a way to mark it as a work-in-progress/not-done/still-needs-something kind of thing.
> If we move it now, it gets the status of "everything-resolved" which is not yet the case.
>
> Thanks for the insight/background info.
> Much of it was discussed before my time.

How about adis16203? Is it more simple to move this driver out of staging?

> >
> > Jonathan
>
> Alex
>
> >
> >
> > > > Signed-off-by: Rodrigo Ribeiro Carvalho <rodrigorsdc@gmail.com>
> > > > ---
> > > >  drivers/iio/accel/Kconfig             |  12 +
> > > >  drivers/iio/accel/Makefile            |   1 +
> > > >  drivers/iio/accel/adis16240.c         | 454 ++++++++++++++++++++++++++
> > > >  drivers/staging/iio/accel/Kconfig     |  12 -
> > > >  drivers/staging/iio/accel/Makefile    |   1 -
> > > >  drivers/staging/iio/accel/adis16240.c | 454 --------------------------
> > > >  6 files changed, 467 insertions(+), 467 deletions(-)
> > > >  create mode 100644 drivers/iio/accel/adis16240.c
> > > >  delete mode 100644 drivers/staging/iio/accel/adis16240.c
> > >
> > > Looks like MAINTAINERS file also needs to be updated, also with the DT bindings file.
> > > I think checkpatch usually complains about these.
> > >
> > > > diff --git a/drivers/iio/accel/Kconfig b/drivers/iio/accel/Kconfig
> > > > index d4ef35aeb579..91fd8741c95f 100644
> > > > --- a/drivers/iio/accel/Kconfig
> > > > +++ b/drivers/iio/accel/Kconfig
> > > > @@ -30,6 +30,18 @@ config ADIS16209
> > > >     To compile this driver as a module, say M here: the module will be
> > > >     called adis16209.
> > > >
> > > > +config ADIS16240
> > > > + tristate "Analog Devices ADIS16240 Programmable Impact Sensor and Recorder"
> > > > + depends on SPI
> > > > + select IIO_ADIS_LIB
> > > > + select IIO_ADIS_LIB_BUFFER if IIO_BUFFER
> > > > + help
> > > > +   Say Y here to build support for Analog Devices adis16240 programmable
> > > > +   impact Sensor and recorder.
> > > > +
> > > > +   To compile this driver as a module, say M here: the module will be
> > > > +   called adis16240.
> > > > +
> > > >  config ADXL345
> > > >   tristate
> > > >
> > > > diff --git a/drivers/iio/accel/Makefile b/drivers/iio/accel/Makefile
> > > > index 56bd0215e0d4..f7e025a86dd9 100644
> > > > --- a/drivers/iio/accel/Makefile
> > > > +++ b/drivers/iio/accel/Makefile
> > > > @@ -6,6 +6,7 @@
> > > >  # When adding new entries keep the list in alphabetical order
> > > >  obj-$(CONFIG_ADIS16201) += adis16201.o
> > > >  obj-$(CONFIG_ADIS16209) += adis16209.o
> > > > +obj-$(CONFIG_ADIS16240) += adis16240.o
> > > >  obj-$(CONFIG_ADXL345) += adxl345_core.o
> > > >  obj-$(CONFIG_ADXL345_I2C) += adxl345_i2c.o
> > > >  obj-$(CONFIG_ADXL345_SPI) += adxl345_spi.o
> > > > diff --git a/drivers/iio/accel/adis16240.c b/drivers/iio/accel/adis16240.c
> > > > new file mode 100644
> > > > index 000000000000..82099db4bf0c
> > > > --- /dev/null
> > > > +++ b/drivers/iio/accel/adis16240.c
> > > > @@ -0,0 +1,454 @@
> > > > +// SPDX-License-Identifier: GPL-2.0+
> > > > +/*
> > > > + * ADIS16240 Programmable Impact Sensor and Recorder driver
> > > > + *
> > > > + * Copyright 2010 Analog Devices Inc.
> > > > + */
> > > > +
> > > > +#include <linux/interrupt.h>
> > > > +#include <linux/irq.h>
> > > > +#include <linux/gpio.h>
> > > > +#include <linux/delay.h>
> > > > +#include <linux/device.h>
> > > > +#include <linux/kernel.h>
> > > > +#include <linux/spi/spi.h>
> > > > +#include <linux/slab.h>
> > > > +#include <linux/sysfs.h>
> > > > +#include <linux/list.h>
> > > > +#include <linux/module.h>
> > > > +
> > > > +#include <linux/iio/iio.h>
> > > > +#include <linux/iio/sysfs.h>
> > > > +#include <linux/iio/buffer.h>
> > > > +#include <linux/iio/imu/adis.h>
> > > > +
> > > > +#define ADIS16240_STARTUP_DELAY  220 /* ms */
> > > > +
> > > > +/* Flash memory write count */
> > > > +#define ADIS16240_FLASH_CNT      0x00
> > > > +
> > > > +/* Output, power supply */
> > > > +#define ADIS16240_SUPPLY_OUT     0x02
> > > > +
> > > > +/* Output, x-axis accelerometer */
> > > > +#define ADIS16240_XACCL_OUT      0x04
> > > > +
> > > > +/* Output, y-axis accelerometer */
> > > > +#define ADIS16240_YACCL_OUT      0x06
> > > > +
> > > > +/* Output, z-axis accelerometer */
> > > > +#define ADIS16240_ZACCL_OUT      0x08
> > > > +
> > > > +/* Output, auxiliary ADC input */
> > > > +#define ADIS16240_AUX_ADC        0x0A
> > > > +
> > > > +/* Output, temperature */
> > > > +#define ADIS16240_TEMP_OUT       0x0C
> > > > +
> > > > +/* Output, x-axis acceleration peak */
> > > > +#define ADIS16240_XPEAK_OUT      0x0E
> > > > +
> > > > +/* Output, y-axis acceleration peak */
> > > > +#define ADIS16240_YPEAK_OUT      0x10
> > > > +
> > > > +/* Output, z-axis acceleration peak */
> > > > +#define ADIS16240_ZPEAK_OUT      0x12
> > > > +
> > > > +/* Output, sum-of-squares acceleration peak */
> > > > +#define ADIS16240_XYZPEAK_OUT    0x14
> > > > +
> > > > +/* Output, Capture Buffer 1, X and Y acceleration */
> > > > +#define ADIS16240_CAPT_BUF1      0x16
> > > > +
> > > > +/* Output, Capture Buffer 2, Z acceleration */
> > > > +#define ADIS16240_CAPT_BUF2      0x18
> > > > +
> > > > +/* Diagnostic, error flags */
> > > > +#define ADIS16240_DIAG_STAT      0x1A
> > > > +This looks like it could be converted to IIO_CHAN_INFO_SAMP_FREQ attribute.
> > > > +/* Diagnostic, event counter */
> > > > +#define ADIS16240_EVNT_CNTR      0x1C
> > > > +
> > > > +/* Diagnostic, check sum value from firmware test */
> > > > +#define ADIS16240_CHK_SUM        0x1E
> > > > +
> > > > +/* Calibration, x-axis acceleration offset adjustment */
> > > > +#define ADIS16240_XACCL_OFF      0x20
> > > > +
> > > > +/* Calibration, y-axis acceleration offset adjustment */
> > > > +#define ADIS16240_YACCL_OFF      0x22
> > > > +
> > > > +/* Calibration, z-axis acceleration offset adjustment */
> > > > +#define ADIS16240_ZACCL_OFF      0x24
> > > > +This looks like it could be converted to IIO_CHAN_INFO_SAMP_FREQ attribute.
> > > > +/* Clock, hour and minute */
> > > > +#define ADIS16240_CLK_TIME       0x2E
> > > > +
> > > > +/* Clock, month and day */
> > > > +#define ADIS16240_CLK_DATE       0x30
> > > > +
> > > > +/* Clock, year */
> > > > +#define ADIS16240_CLK_YEAR       0x32
> > > > +
> > > > +/* Wake-up setting, hour and minute */
> > > > +#define ADIS16240_WAKE_TIME      0x34
> > > > +
> > > > +/* Wake-up setting, month and day */
> > > > +#define ADIS16240_WAKE_DATE      0x36
> > > > +
> > > > +/* Alarm 1 amplitude threshold */
> > > > +#define ADIS16240_ALM_MAG1       0x38
> > > > +
> > > > +/* Alarm 2 amplitude threshold */
> > > > +#define ADIS16240_ALM_MAG2       0x3A
> > > > +
> > > > +/* Alarm control */
> > > > +#define ADIS16240_ALM_CTRL       0x3C
> > > > +
> > > > +/* Capture, external trigger control */
> > > > +#define ADIS16240_XTRIG_CTRL     0x3E
> > > > +
> > > > +/* Capture, address pointer */
> > > > +#define ADIS16240_CAPT_PNTR      0x40
> > > > +
> > > > +/* Capture, configuration and control */
> > > > +#define ADIS16240_CAPT_CTRL      0x42
> > > > +
> > > > +/* General-purpose digital input/output control */
> > > > +#define ADIS16240_GPIO_CTRL      0x44
> > > > +
> > > > +/* Miscellaneous control */
> > > > +#define ADIS16240_MSC_CTRL       0x46
> > > > +
> > > > +/* Internal sample period (rate) control */
> > > > +#define ADIS16240_SMPL_PRD       0x48
> > > > +
> > > > +/* System command */
> > > > +#define ADIS16240_GLOB_CMD       0x4A
> > > > +
> > > > +/* MSC_CTRL */
> > > > +
> > > > +/* Enables sum-of-squares output (XYZPEAK_OUT) */
> > > > +#define ADIS16240_MSC_CTRL_XYZPEAK_OUT_EN        BIT(15)
> > > > +
> > > > +/* Enables peak tracking output (XPEAK_OUT, YPEAK_OUT, and ZPEAK_OUT) */
> > > > +#define ADIS16240_MSC_CTRL_X_Y_ZPEAK_OUT_EN      BIT(14)
> > > > +
> > > > +/* Self-test enable: 1 = apply electrostatic force, 0 = disabled */
> > > > +#define ADIS16240_MSC_CTRL_SELF_TEST_EN          BIT(8)
> > > > +
> > > > +/* Data-ready enable: 1 = enabled, 0 = disabled */
> > > > +#define ADIS16240_MSC_CTRL_DATA_RDY_EN           BIT(2)
> > > > +
> > > > +/* Data-ready polarity: 1 = active high, 0 = active low */
> > > > +#define ADIS16240_MSC_CTRL_ACTIVE_HIGH           BIT(1)
> > > > +
> > > > +/* Data-ready line selection: 1 = DIO2, 0 = DIO1 */
> > > > +#define ADIS16240_MSC_CTRL_DATA_RDY_DIO2 BIT(0)
> > > > +
> > > > +/* DIAG_STAT */
> > > > +
> > > > +/* Alarm 2 status: 1 = alarm active, 0 = alarm inactive */
> > > > +#define ADIS16240_DIAG_STAT_ALARM2      BIT(9)
> > > > +
> > > > +/* Alarm 1 status: 1 = alarm active, 0 = alarm inactive */
> > > > +#define ADIS16240_DIAG_STAT_ALARM1      BIT(8)
> > > > +
> > > > +/* Capture buffer full: 1 = capture buffer is full */
> > > > +#define ADIS16240_DIAG_STAT_CPT_BUF_FUL BIT(7)
> > > > +
> > > > +/* Flash test, checksum flag: 1 = mismatch, 0 = match */
> > > > +#define ADIS16240_DIAG_STAT_CHKSUM      BIT(6)
> > > > +
> > > > +/* Power-on, self-test flag: 1 = failure, 0 = pass */
> > > > +#define ADIS16240_DIAG_STAT_PWRON_FAIL_BIT  5
> > > > +
> > > > +/* Power-on self-test: 1 = in-progress, 0 = complete */
> > > > +#define ADIS16240_DIAG_STAT_PWRON_BUSY  BIT(4)
> > > > +
> > > > +/* SPI communications failure */
> > > > +#define ADIS16240_DIAG_STAT_SPI_FAIL_BIT 3
> > > > +
> > > > +/* Flash update failure */
> > > > +#define ADIS16240_DIAG_STAT_FLASH_UPT_BIT        2
> > > > +
> > > > +/* Power supply above 3.625 V */
> > > > +#define ADIS16240_DIAG_STAT_POWER_HIGH_BIT       1
> > > > +
> > > > + /* Power supply below 2.225 V */
> > > > +#define ADIS16240_DIAG_STAT_POWER_LOW_BIT        0
> > > > +
> > > > +/* GLOB_CMD */
> > > > +
> > > > +#define ADIS16240_GLOB_CMD_RESUME        BIT(8)
> > > > +#define ADIS16240_GLOB_CMD_SW_RESET      BIT(7)
> > > > +#define ADIS16240_GLOB_CMD_STANDBY       BIT(2)
> > > > +
> > > > +#define ADIS16240_ERROR_ACTIVE          BIT(14)
> > > > +
> > > > +/* At the moment triggers are only used for ring buffer
> > > > + * filling. This may change!
> > > > + */
> > > > +
> > > > +enum adis16240_scan {
> > > > + ADIS16240_SCAN_ACC_X,
> > > > + ADIS16240_SCAN_ACC_Y,
> > > > + ADIS16240_SCAN_ACC_Z,
> > > > + ADIS16240_SCAN_SUPPLY,
> > > > + ADIS16240_SCAN_AUX_ADC,
> > > > + ADIS16240_SCAN_TEMP,
> > > > +};
> > > > +
> > > > +static ssize_t adis16240_spi_read_signed(struct device *dev,
> > > > +                                  struct device_attribute *attr,
> > > > +                                  char *buf,
> > > > +                                  unsigned int bits)
> > > > +{
> > > > + struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> > > > + struct adis *st = iio_priv(indio_dev);
> > > > + int ret;
> > > > + s16 val = 0;
> > > > + unsigned int shift = 16 - bits;
> > > > + struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
> > > > +
> > > > + ret = adis_read_reg_16(st,
> > > > +                        this_attr->address, (u16 *)&val);
> >
> > Check for odd line breaks.  That looks like it'll fit under 80 chars
> > just fine.
> >
> > > > + if (ret)
> > > > +         return ret;
> > > > +
> > > > + if (val & ADIS16240_ERROR_ACTIVE)
> > > > +         adis_check_status(st);
> > > > +
> > > > + val = (s16)(val << shift) >> shift;
> >
> > Use sign_extend32 (added after this driver I think!)
> >
> > > > + return sprintf(buf, "%d\n", val);
> > > > +}
> > > > +
> > > > +static ssize_t adis16240_read_12bit_signed(struct device *dev,
> > > > +                                    struct device_attribute *attr,
> > > > +                                    char *buf)
> > > > +{
> > > > + return adis16240_spi_read_signed(dev, attr, buf, 12);
> > > > +}
> > > > +
> > > > +static IIO_DEVICE_ATTR(in_accel_xyz_squared_peak_raw, 0444,
> > > > +                adis16240_read_12bit_signed, NULL,
> > > > +                ADIS16240_XYZPEAK_OUT);
> > > > +
> > > > +static IIO_CONST_ATTR_SAMP_FREQ_AVAIL("4096");
> > > > +
> > > > +static const u8 adis16240_addresses[][2] = {
> > > > + [ADIS16240_SCAN_ACC_X] = { ADIS16240_XACCL_OFF, ADIS16240_XPEAK_OUT },
> > > > + [ADIS16240_SCAN_ACC_Y] = { ADIS16240_YACCL_OFF, ADIS16240_YPEAK_OUT },
> > > > + [ADIS16240_SCAN_ACC_Z] = { ADIS16240_ZACCL_OFF, ADIS16240_ZPEAK_OUT },
> > > > +};
> > > > +
> > > > +static int adis16240_read_raw(struct iio_dev *indio_dev,
> > > > +                       struct iio_chan_spec const *chan,
> > > > +                       int *val, int *val2,
> > > > +                       long mask)
> > > > +{
> > > > + struct adis *st = iio_priv(indio_dev);
> > > > + int ret;
> > > > + u8 addr;
> > > > + s16 val16;
> > > > +
> > > > + switch (mask) {
> > > > + case IIO_CHAN_INFO_RAW:
> > > > +         return adis_single_conversion(indio_dev, chan,
> > > > +                         ADIS16240_ERROR_ACTIVE, val);
> > > > + case IIO_CHAN_INFO_SCALE:
> > > > +         switch (chan->type) {
> > > > +         case IIO_VOLTAGE:
> > > > +                 if (chan->channel == 0) {
> > > > +                         *val = 4;
> > > > +                         *val2 = 880000; /* 4.88 mV */
> > > > +                         return IIO_VAL_INT_PLUS_MICRO;
> > > > +                 }
> > > > +                 return -EINVAL;
> > > > +         case IIO_TEMP:
> > > > +                 *val = 244; /* 0.244 C */
> > > > +                 *val2 = 0;
> > >
> > > This looks like a bug in the original driver.
> > > I think *val = 0, and *val2 = 244 ?

I have doubt about this. Also in adis16203, adis16209 and
adis16201 decimal parts are in *val and not in *val2 on
IIO_TEMP. What do you think? If it is really a bug, I can send
a patch.

> > > > +                 return IIO_VAL_INT_PLUS_MICRO;
> > > > +         case IIO_ACCEL:
> > > > +                 *val = 0;
> > > > +                 *val2 = IIO_G_TO_M_S_2(51400); /* 51.4 mg */
> > > > +                 return IIO_VAL_INT_PLUS_MICRO;
> > > > +         default:
> > > > +                 return -EINVAL;
> > > > +         }
> > > > +         break;
> > > > + case IIO_CHAN_INFO_PEAK_SCALE:
> > > > +         *val = 0;
> > > > +         *val2 = IIO_G_TO_M_S_2(51400); /* 51.4 mg */
> > > > +         return IIO_VAL_INT_PLUS_MICRO;
> > > > + case IIO_CHAN_INFO_OFFSET:
> > > > +         *val = 25000 / 244 - 0x133; /* 25 C = 0x133 */
> > > > +         return IIO_VAL_INT;
> > > > + case IIO_CHAN_INFO_CALIBBIAS:
> > > > +         addr = adis16240_addresses[chan->scan_index][0];
> > > > +         ret = adis_read_reg_16(st, addr, &val16);
> > > > +         if (ret)
> > > > +                 return ret;
> > > > +         *val = sign_extend32(val16, 9);
> > > > +         return IIO_VAL_INT;
> > > > + case IIO_CHAN_INFO_PEAK:
> > > > +         addr = adis16240_addresses[chan->scan_index][1];
> > > > +         ret = adis_read_reg_16(st, addr, &val16);
> > > > +         if (ret)
> > > > +                 return ret;
> > > > +         *val = sign_extend32(val16, 9);
> > > > +         return IIO_VAL_INT;
> > > > + }
> > > > + return -EINVAL;
> > > > +}
> > > > +
> > > > +static int adis16240_write_raw(struct iio_dev *indio_dev,
> > > > +                        struct iio_chan_spec const *chan,
> > > > +                        int val,
> > > > +                        int val2,
> > > > +                        long mask)
> > > > +{
> > > > + struct adis *st = iio_priv(indio_dev);
> > > > + u8 addr;
> > > > +
> > > > + switch (mask) {
> > > > + case IIO_CHAN_INFO_CALIBBIAS:
> > > > +         addr = adis16240_addresses[chan->scan_index][0];
> > > > +         return adis_write_reg_16(st, addr, val & GENMASK(9, 0));
> > > > + }
> > > > + return -EINVAL;
> > > > +}
> > > > +
> > > > +static const struct iio_chan_spec adis16240_channels[] = {
> > > > + ADIS_SUPPLY_CHAN(ADIS16240_SUPPLY_OUT, ADIS16240_SCAN_SUPPLY, 0, 10),
> > > > + ADIS_AUX_ADC_CHAN(ADIS16240_AUX_ADC, ADIS16240_SCAN_AUX_ADC, 0, 10),
> > > > + ADIS_ACCEL_CHAN(X, ADIS16240_XACCL_OUT, ADIS16240_SCAN_ACC_X,
> > > > +                 BIT(IIO_CHAN_INFO_CALIBBIAS) | BIT(IIO_CHAN_INFO_PEAK),
> > > > +                 0, 10),
> > > > + ADIS_ACCEL_CHAN(Y, ADIS16240_YACCL_OUT, ADIS16240_SCAN_ACC_Y,
> > > > +                 BIT(IIO_CHAN_INFO_CALIBBIAS) | BIT(IIO_CHAN_INFO_PEAK),
> > > > +                 0, 10),
> > > > + ADIS_ACCEL_CHAN(Z, ADIS16240_ZACCL_OUT, ADIS16240_SCAN_ACC_Z,
> > > > +                 BIT(IIO_CHAN_INFO_CALIBBIAS) | BIT(IIO_CHAN_INFO_PEAK),
> > > > +                 0, 10),
> > > > + ADIS_TEMP_CHAN(ADIS16240_TEMP_OUT, ADIS16240_SCAN_TEMP, 0, 10),
> > > > + IIO_CHAN_SOFT_TIMESTAMP(6)
> > > > +};
> > > > +
> > > > +static struct attribute *adis16240_attributes[] = {
> > > > + &iio_dev_attr_in_accel_xyz_squared_peak_raw.dev_attr.attr,
> > >
> > > This one looks a bit weird/rare.
> > > I am not sure about it's use-case/need right now (at this point in time).
> > >
> > > Maybe we could just drop it?
> > > Since I did not write the initial driver, I don't know yet what it was supposed to be/do.
> >
> > For an impact sensor it's a fairly useful thing I suspect as it is giving the maximum
> > acceleration in any direction that occured (once you square root it in userspace.)
> >
> > > In any case, if we want to add it, it looks like it needs an ABI documentation.
> >
> > Agreed. Needs docs.  We actually have a modifier for it so can be handled
> > as a channel. IIO_MOD_SUM_SQUARED_X_Y_Z :)
> >
> >
> > >
> > > > + &iio_const_attr_sampling_frequency_available.dev_attr.attr,
> > >
> > > This looks like it could be converted to IIO_CHAN_INFO_SAMP_FREQ attribute.
> > >
> > >
> > > > + NULL
> > > > +};
> > > > +
> > > > +static const struct attribute_group adis16240_attribute_group = {
> > > > + .attrs = adis16240_attributes,
> > > > +};
> > > > +
> > > > +static const struct iio_info adis16240_info = {
> > > > + .attrs = &adis16240_attribute_group,
> > > > + .read_raw = adis16240_read_raw,
> > > > + .write_raw = adis16240_write_raw,
> > > > + .update_scan_mode = adis_update_scan_mode,
> > > > +};
> > > > +
> > > > +static const char * const adis16240_status_error_msgs[] = {
> > > > + [ADIS16240_DIAG_STAT_PWRON_FAIL_BIT] = "Power on, self-test failed",
> > > > + [ADIS16240_DIAG_STAT_SPI_FAIL_BIT] = "SPI failure",
> > > > + [ADIS16240_DIAG_STAT_FLASH_UPT_BIT] = "Flash update failed",
> > > > + [ADIS16240_DIAG_STAT_POWER_HIGH_BIT] = "Power supply above 3.625V",
> > > > + [ADIS16240_DIAG_STAT_POWER_LOW_BIT] = "Power supply below 2.225V",
> > > > +};
> > > > +
> > > > +static const struct adis_data adis16240_data = {
> > > > + .write_delay = 35,
> > > > + .read_delay = 35,
> > > > + .msc_ctrl_reg = ADIS16240_MSC_CTRL,
> > > > + .glob_cmd_reg = ADIS16240_GLOB_CMD,
> > > > + .diag_stat_reg = ADIS16240_DIAG_STAT,
> > > > +
> > > > + .self_test_mask = ADIS16240_MSC_CTRL_SELF_TEST_EN,
> > > > + .self_test_no_autoclear = true,
> > > > + .startup_delay = ADIS16240_STARTUP_DELAY,
> > > > +
> > > > + .status_error_msgs = adis16240_status_error_msgs,
> > > > + .status_error_mask = BIT(ADIS16240_DIAG_STAT_PWRON_FAIL_BIT) |
> > > > +         BIT(ADIS16240_DIAG_STAT_SPI_FAIL_BIT) |
> > > > +         BIT(ADIS16240_DIAG_STAT_FLASH_UPT_BIT) |
> > > > +         BIT(ADIS16240_DIAG_STAT_POWER_HIGH_BIT) |
> > > > +         BIT(ADIS16240_DIAG_STAT_POWER_LOW_BIT),
> > > > +};
> > > > +
> > > > +static int adis16240_probe(struct spi_device *spi)
> > > > +{
> > > > + int ret;
> > > > + struct adis *st;
> > > > + struct iio_dev *indio_dev;
> > > > +
> > > > + /* setup the industrialio driver allocated elements */
> >
> > I would review comments and decide whether they actually add anything.
> > This one for example - I'm not convinced!
> >
> > > > + indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
> > > > + if (!indio_dev)
> > > > +         return -ENOMEM;
> > > > + st = iio_priv(indio_dev);
> > > > + /* this is only used for removal purposes */
> > > > + spi_set_drvdata(spi, indio_dev);
> > > > +
> > > > + indio_dev->name = spi->dev.driver->name;
> > > > + indio_dev->dev.parent = &spi->dev;
> > > > + indio_dev->info = &adis16240_info;
> > > > + indio_dev->channels = adis16240_channels;
> > > > + indio_dev->num_channels = ARRAY_SIZE(adis16240_channels);
> > > > + indio_dev->modes = INDIO_DIRECT_MODE;
> > > > +
> > > > + ret = adis_init(st, indio_dev, spi, &adis16240_data);
> > > > + if (ret)
> > > > +         return ret;
> >
> > Might be nice to look at adding some white space after error handling
> > blocks like this.  A blank line in locations like this make it
> > easier for the eye to follow the code.
> >
> > > > + ret = adis_setup_buffer_and_trigger(st, indio_dev, NULL);
> > > > + if (ret)
> > > > +         return ret;
> > > > +
> > > > + /* Get the device into a sane initial state */
> > > > + ret = adis_initial_startup(st);
> > > > + if (ret)
> > > > +         goto error_cleanup_buffer_trigger;
> > > > + ret = iio_device_register(indio_dev);
> > > > + if (ret)
> > > > +         goto error_cleanup_buffer_trigger;
> > > > + return 0;
> > > > +
> > > > +error_cleanup_buffer_trigger:
> > > > + adis_cleanup_buffer_and_trigger(st, indio_dev);
> > > > + return ret;
> > > > +}
> > > > +
> > > > +static int adis16240_remove(struct spi_device *spi)
> > > > +{
> > > > + struct iio_dev *indio_dev = spi_get_drvdata(spi);
> > > > + struct adis *st = iio_priv(indio_dev);
> > > > +
> > > > + iio_device_unregister(indio_dev);
> > > > + adis_cleanup_buffer_and_trigger(st, indio_dev);
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static const struct of_device_id adis16240_of_match[] = {
> > > > + { .compatible = "adi,adis16240" },
> > > > + { },
> > > > +};
> > > > +MODULE_DEVICE_TABLE(of, adis16240_of_match);
> > > > +
> > > > +static struct spi_driver adis16240_driver = {
> > > > + .driver = {
> > > > +         .name = "adis16240",
> > > > +         .of_match_table = adis16240_of_match,
> > > > + },
> > > > + .probe = adis16240_probe,
> > > > + .remove = adis16240_remove,
> > > > +};
> > > > +module_spi_driver(adis16240_driver);
> > > > +
> > > > +MODULE_AUTHOR("Barry Song <21cnbao@gmail.com>");
> > > > +MODULE_DESCRIPTION("Analog Devices Programmable Impact Sensor and Recorder");
> > > > +MODULE_LICENSE("GPL v2");
> > > > +MODULE_ALIAS("spi:adis16240");
> > ...

Thanks,
Rodrigo

Re: [PATCH 2/2] staging: iio: accel: adis16240: move out of staging

[Alexandru Ardelean]

On Wed, Sep 11, 2019 at 7:21 PM Rodrigo Carvalho <rodrigorsdc@gmail.com> wrote:
>
> Hi,
>
> Em seg, 9 de set de 2019 às 02:53, Ardelean, Alexandru
> <alexandru.Ardelean@analog.com> escreveu:
> >
> > On Sun, 2019-09-08 at 12:09 +0100, Jonathan Cameron wrote:
> > > On Mon, 2 Sep 2019 13:26:02 +0000
> > > "Ardelean, Alexandru" <alexandru.Ardelean@analog.com> wrote:
> > >
> > > > On Sun, 2019-09-01 at 21:59 -0300, Rodrigo Carvalho wrote:
> > > > > Move ADIS16240 driver from staging to mainline.
> > > > >
> > > > > The ADIS16240 is a fully integrated digital shock detection
> > > > > and recorder system.
> > > >
> > > > Hey,
> > > >
> > > > Comments inline.
> > > >
> > > > I'll probably take a look in the next days again.
> > > > There seem to be some ABI/sysfs attributes that need to be resolved before moving this out of staging.
> > >
> > > Absolutely. It is a 'new' type of device so there are definitely some
> > > corners that need discussing before we move out of staging and commit
> > > to maintaining the ABI moving forwards.
> > >
> > > That is the real reason this driver was still in staging!  No one
> > > had been through the process of proposing the ABI and responding to
> > > questions etc.
> > >
> > > The issue with impact sensors has always been that they don't really fit
> > > our normal model for buffers or triggers.
> > >
> > > So normally a trigger (if exposed in IIO) is used as one trigger
> > > causes 1 set of samples (so like a frame trigger for a camera).
> > >
> > > These devices tend to work in a mode where one trigger causes data
> > > to be captured for a period of time.  In this part that's the event
> > > recorder function
> > >
> > > No one is realistically going to buy an impact sensor to just use it
> > > as an accelerometer which is what this driver is currently doing.
> > > I suppose we could just leave support in that form for now, but
> > > I'm no sure how much use it is to anyone.
> > >
> > > Analog Devices people, worth working out how to support the event
> > > recorder?  For that someone needs to have hardware as it is complex
> > > to say the least!
> >
> > Worth it: yes.
> > But we don't have any resources to allocate for this [at this point in time].
> >
> > >
> > > We could move it out but might be worth adding a comment somewhere
> > > saying this only really supports direct access to channels, and
> > > not the event recorder functionality.
> > >
> >
> > I guess, I would vote for leaving it in staging.
> > It's also a way to mark it as a work-in-progress/not-done/still-needs-something kind of thing.
> > If we move it now, it gets the status of "everything-resolved" which is not yet the case.
> >
> > Thanks for the insight/background info.
> > Much of it was discussed before my time.
>
> How about adis16203? Is it more simple to move this driver out of staging?

Replying from my personal email, as I seem to have my periodical
issues with the work-email.

At first glance I can't see any obvious blockers for this.

I think Jonathan may need to provide some feedback here.

Thanks
Alex

>
> > >
> > > Jonathan
> >
> > Alex
> >
> > >
> > >
> > > > > Signed-off-by: Rodrigo Ribeiro Carvalho <rodrigorsdc@gmail.com>
> > > > > ---
> > > > >  drivers/iio/accel/Kconfig             |  12 +
> > > > >  drivers/iio/accel/Makefile            |   1 +
> > > > >  drivers/iio/accel/adis16240.c         | 454 ++++++++++++++++++++++++++
> > > > >  drivers/staging/iio/accel/Kconfig     |  12 -
> > > > >  drivers/staging/iio/accel/Makefile    |   1 -
> > > > >  drivers/staging/iio/accel/adis16240.c | 454 --------------------------
> > > > >  6 files changed, 467 insertions(+), 467 deletions(-)
> > > > >  create mode 100644 drivers/iio/accel/adis16240.c
> > > > >  delete mode 100644 drivers/staging/iio/accel/adis16240.c
> > > >
> > > > Looks like MAINTAINERS file also needs to be updated, also with the DT bindings file.
> > > > I think checkpatch usually complains about these.
> > > >
> > > > > diff --git a/drivers/iio/accel/Kconfig b/drivers/iio/accel/Kconfig
> > > > > index d4ef35aeb579..91fd8741c95f 100644
> > > > > --- a/drivers/iio/accel/Kconfig
> > > > > +++ b/drivers/iio/accel/Kconfig
> > > > > @@ -30,6 +30,18 @@ config ADIS16209
> > > > >     To compile this driver as a module, say M here: the module will be
> > > > >     called adis16209.
> > > > >
> > > > > +config ADIS16240
> > > > > + tristate "Analog Devices ADIS16240 Programmable Impact Sensor and Recorder"
> > > > > + depends on SPI
> > > > > + select IIO_ADIS_LIB
> > > > > + select IIO_ADIS_LIB_BUFFER if IIO_BUFFER
> > > > > + help
> > > > > +   Say Y here to build support for Analog Devices adis16240 programmable
> > > > > +   impact Sensor and recorder.
> > > > > +
> > > > > +   To compile this driver as a module, say M here: the module will be
> > > > > +   called adis16240.
> > > > > +
> > > > >  config ADXL345
> > > > >   tristate
> > > > >
> > > > > diff --git a/drivers/iio/accel/Makefile b/drivers/iio/accel/Makefile
> > > > > index 56bd0215e0d4..f7e025a86dd9 100644
> > > > > --- a/drivers/iio/accel/Makefile
> > > > > +++ b/drivers/iio/accel/Makefile
> > > > > @@ -6,6 +6,7 @@
> > > > >  # When adding new entries keep the list in alphabetical order
> > > > >  obj-$(CONFIG_ADIS16201) += adis16201.o
> > > > >  obj-$(CONFIG_ADIS16209) += adis16209.o
> > > > > +obj-$(CONFIG_ADIS16240) += adis16240.o
> > > > >  obj-$(CONFIG_ADXL345) += adxl345_core.o
> > > > >  obj-$(CONFIG_ADXL345_I2C) += adxl345_i2c.o
> > > > >  obj-$(CONFIG_ADXL345_SPI) += adxl345_spi.o
> > > > > diff --git a/drivers/iio/accel/adis16240.c b/drivers/iio/accel/adis16240.c
> > > > > new file mode 100644
> > > > > index 000000000000..82099db4bf0c
> > > > > --- /dev/null
> > > > > +++ b/drivers/iio/accel/adis16240.c
> > > > > @@ -0,0 +1,454 @@
> > > > > +// SPDX-License-Identifier: GPL-2.0+
> > > > > +/*
> > > > > + * ADIS16240 Programmable Impact Sensor and Recorder driver
> > > > > + *
> > > > > + * Copyright 2010 Analog Devices Inc.
> > > > > + */
> > > > > +
> > > > > +#include <linux/interrupt.h>
> > > > > +#include <linux/irq.h>
> > > > > +#include <linux/gpio.h>
> > > > > +#include <linux/delay.h>
> > > > > +#include <linux/device.h>
> > > > > +#include <linux/kernel.h>
> > > > > +#include <linux/spi/spi.h>
> > > > > +#include <linux/slab.h>
> > > > > +#include <linux/sysfs.h>
> > > > > +#include <linux/list.h>
> > > > > +#include <linux/module.h>
> > > > > +
> > > > > +#include <linux/iio/iio.h>
> > > > > +#include <linux/iio/sysfs.h>
> > > > > +#include <linux/iio/buffer.h>
> > > > > +#include <linux/iio/imu/adis.h>
> > > > > +
> > > > > +#define ADIS16240_STARTUP_DELAY  220 /* ms */
> > > > > +
> > > > > +/* Flash memory write count */
> > > > > +#define ADIS16240_FLASH_CNT      0x00
> > > > > +
> > > > > +/* Output, power supply */
> > > > > +#define ADIS16240_SUPPLY_OUT     0x02
> > > > > +
> > > > > +/* Output, x-axis accelerometer */
> > > > > +#define ADIS16240_XACCL_OUT      0x04
> > > > > +
> > > > > +/* Output, y-axis accelerometer */
> > > > > +#define ADIS16240_YACCL_OUT      0x06
> > > > > +
> > > > > +/* Output, z-axis accelerometer */
> > > > > +#define ADIS16240_ZACCL_OUT      0x08
> > > > > +
> > > > > +/* Output, auxiliary ADC input */
> > > > > +#define ADIS16240_AUX_ADC        0x0A
> > > > > +
> > > > > +/* Output, temperature */
> > > > > +#define ADIS16240_TEMP_OUT       0x0C
> > > > > +
> > > > > +/* Output, x-axis acceleration peak */
> > > > > +#define ADIS16240_XPEAK_OUT      0x0E
> > > > > +
> > > > > +/* Output, y-axis acceleration peak */
> > > > > +#define ADIS16240_YPEAK_OUT      0x10
> > > > > +
> > > > > +/* Output, z-axis acceleration peak */
> > > > > +#define ADIS16240_ZPEAK_OUT      0x12
> > > > > +
> > > > > +/* Output, sum-of-squares acceleration peak */
> > > > > +#define ADIS16240_XYZPEAK_OUT    0x14
> > > > > +
> > > > > +/* Output, Capture Buffer 1, X and Y acceleration */
> > > > > +#define ADIS16240_CAPT_BUF1      0x16
> > > > > +
> > > > > +/* Output, Capture Buffer 2, Z acceleration */
> > > > > +#define ADIS16240_CAPT_BUF2      0x18
> > > > > +
> > > > > +/* Diagnostic, error flags */
> > > > > +#define ADIS16240_DIAG_STAT      0x1A
> > > > > +This looks like it could be converted to IIO_CHAN_INFO_SAMP_FREQ attribute.
> > > > > +/* Diagnostic, event counter */
> > > > > +#define ADIS16240_EVNT_CNTR      0x1C
> > > > > +
> > > > > +/* Diagnostic, check sum value from firmware test */
> > > > > +#define ADIS16240_CHK_SUM        0x1E
> > > > > +
> > > > > +/* Calibration, x-axis acceleration offset adjustment */
> > > > > +#define ADIS16240_XACCL_OFF      0x20
> > > > > +
> > > > > +/* Calibration, y-axis acceleration offset adjustment */
> > > > > +#define ADIS16240_YACCL_OFF      0x22
> > > > > +
> > > > > +/* Calibration, z-axis acceleration offset adjustment */
> > > > > +#define ADIS16240_ZACCL_OFF      0x24
> > > > > +This looks like it could be converted to IIO_CHAN_INFO_SAMP_FREQ attribute.
> > > > > +/* Clock, hour and minute */
> > > > > +#define ADIS16240_CLK_TIME       0x2E
> > > > > +
> > > > > +/* Clock, month and day */
> > > > > +#define ADIS16240_CLK_DATE       0x30
> > > > > +
> > > > > +/* Clock, year */
> > > > > +#define ADIS16240_CLK_YEAR       0x32
> > > > > +
> > > > > +/* Wake-up setting, hour and minute */
> > > > > +#define ADIS16240_WAKE_TIME      0x34
> > > > > +
> > > > > +/* Wake-up setting, month and day */
> > > > > +#define ADIS16240_WAKE_DATE      0x36
> > > > > +
> > > > > +/* Alarm 1 amplitude threshold */
> > > > > +#define ADIS16240_ALM_MAG1       0x38
> > > > > +
> > > > > +/* Alarm 2 amplitude threshold */
> > > > > +#define ADIS16240_ALM_MAG2       0x3A
> > > > > +
> > > > > +/* Alarm control */
> > > > > +#define ADIS16240_ALM_CTRL       0x3C
> > > > > +
> > > > > +/* Capture, external trigger control */
> > > > > +#define ADIS16240_XTRIG_CTRL     0x3E
> > > > > +
> > > > > +/* Capture, address pointer */
> > > > > +#define ADIS16240_CAPT_PNTR      0x40
> > > > > +
> > > > > +/* Capture, configuration and control */
> > > > > +#define ADIS16240_CAPT_CTRL      0x42
> > > > > +
> > > > > +/* General-purpose digital input/output control */
> > > > > +#define ADIS16240_GPIO_CTRL      0x44
> > > > > +
> > > > > +/* Miscellaneous control */
> > > > > +#define ADIS16240_MSC_CTRL       0x46
> > > > > +
> > > > > +/* Internal sample period (rate) control */
> > > > > +#define ADIS16240_SMPL_PRD       0x48
> > > > > +
> > > > > +/* System command */
> > > > > +#define ADIS16240_GLOB_CMD       0x4A
> > > > > +
> > > > > +/* MSC_CTRL */
> > > > > +
> > > > > +/* Enables sum-of-squares output (XYZPEAK_OUT) */
> > > > > +#define ADIS16240_MSC_CTRL_XYZPEAK_OUT_EN        BIT(15)
> > > > > +
> > > > > +/* Enables peak tracking output (XPEAK_OUT, YPEAK_OUT, and ZPEAK_OUT) */
> > > > > +#define ADIS16240_MSC_CTRL_X_Y_ZPEAK_OUT_EN      BIT(14)
> > > > > +
> > > > > +/* Self-test enable: 1 = apply electrostatic force, 0 = disabled */
> > > > > +#define ADIS16240_MSC_CTRL_SELF_TEST_EN          BIT(8)
> > > > > +
> > > > > +/* Data-ready enable: 1 = enabled, 0 = disabled */
> > > > > +#define ADIS16240_MSC_CTRL_DATA_RDY_EN           BIT(2)
> > > > > +
> > > > > +/* Data-ready polarity: 1 = active high, 0 = active low */
> > > > > +#define ADIS16240_MSC_CTRL_ACTIVE_HIGH           BIT(1)
> > > > > +
> > > > > +/* Data-ready line selection: 1 = DIO2, 0 = DIO1 */
> > > > > +#define ADIS16240_MSC_CTRL_DATA_RDY_DIO2 BIT(0)
> > > > > +
> > > > > +/* DIAG_STAT */
> > > > > +
> > > > > +/* Alarm 2 status: 1 = alarm active, 0 = alarm inactive */
> > > > > +#define ADIS16240_DIAG_STAT_ALARM2      BIT(9)
> > > > > +
> > > > > +/* Alarm 1 status: 1 = alarm active, 0 = alarm inactive */
> > > > > +#define ADIS16240_DIAG_STAT_ALARM1      BIT(8)
> > > > > +
> > > > > +/* Capture buffer full: 1 = capture buffer is full */
> > > > > +#define ADIS16240_DIAG_STAT_CPT_BUF_FUL BIT(7)
> > > > > +
> > > > > +/* Flash test, checksum flag: 1 = mismatch, 0 = match */
> > > > > +#define ADIS16240_DIAG_STAT_CHKSUM      BIT(6)
> > > > > +
> > > > > +/* Power-on, self-test flag: 1 = failure, 0 = pass */
> > > > > +#define ADIS16240_DIAG_STAT_PWRON_FAIL_BIT  5
> > > > > +
> > > > > +/* Power-on self-test: 1 = in-progress, 0 = complete */
> > > > > +#define ADIS16240_DIAG_STAT_PWRON_BUSY  BIT(4)
> > > > > +
> > > > > +/* SPI communications failure */
> > > > > +#define ADIS16240_DIAG_STAT_SPI_FAIL_BIT 3
> > > > > +
> > > > > +/* Flash update failure */
> > > > > +#define ADIS16240_DIAG_STAT_FLASH_UPT_BIT        2
> > > > > +
> > > > > +/* Power supply above 3.625 V */
> > > > > +#define ADIS16240_DIAG_STAT_POWER_HIGH_BIT       1
> > > > > +
> > > > > + /* Power supply below 2.225 V */
> > > > > +#define ADIS16240_DIAG_STAT_POWER_LOW_BIT        0
> > > > > +
> > > > > +/* GLOB_CMD */
> > > > > +
> > > > > +#define ADIS16240_GLOB_CMD_RESUME        BIT(8)
> > > > > +#define ADIS16240_GLOB_CMD_SW_RESET      BIT(7)
> > > > > +#define ADIS16240_GLOB_CMD_STANDBY       BIT(2)
> > > > > +
> > > > > +#define ADIS16240_ERROR_ACTIVE          BIT(14)
> > > > > +
> > > > > +/* At the moment triggers are only used for ring buffer
> > > > > + * filling. This may change!
> > > > > + */
> > > > > +
> > > > > +enum adis16240_scan {
> > > > > + ADIS16240_SCAN_ACC_X,
> > > > > + ADIS16240_SCAN_ACC_Y,
> > > > > + ADIS16240_SCAN_ACC_Z,
> > > > > + ADIS16240_SCAN_SUPPLY,
> > > > > + ADIS16240_SCAN_AUX_ADC,
> > > > > + ADIS16240_SCAN_TEMP,
> > > > > +};
> > > > > +
> > > > > +static ssize_t adis16240_spi_read_signed(struct device *dev,
> > > > > +                                  struct device_attribute *attr,
> > > > > +                                  char *buf,
> > > > > +                                  unsigned int bits)
> > > > > +{
> > > > > + struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> > > > > + struct adis *st = iio_priv(indio_dev);
> > > > > + int ret;
> > > > > + s16 val = 0;
> > > > > + unsigned int shift = 16 - bits;
> > > > > + struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
> > > > > +
> > > > > + ret = adis_read_reg_16(st,
> > > > > +                        this_attr->address, (u16 *)&val);
> > >
> > > Check for odd line breaks.  That looks like it'll fit under 80 chars
> > > just fine.
> > >
> > > > > + if (ret)
> > > > > +         return ret;
> > > > > +
> > > > > + if (val & ADIS16240_ERROR_ACTIVE)
> > > > > +         adis_check_status(st);
> > > > > +
> > > > > + val = (s16)(val << shift) >> shift;
> > >
> > > Use sign_extend32 (added after this driver I think!)
> > >
> > > > > + return sprintf(buf, "%d\n", val);
> > > > > +}
> > > > > +
> > > > > +static ssize_t adis16240_read_12bit_signed(struct device *dev,
> > > > > +                                    struct device_attribute *attr,
> > > > > +                                    char *buf)
> > > > > +{
> > > > > + return adis16240_spi_read_signed(dev, attr, buf, 12);
> > > > > +}
> > > > > +
> > > > > +static IIO_DEVICE_ATTR(in_accel_xyz_squared_peak_raw, 0444,
> > > > > +                adis16240_read_12bit_signed, NULL,
> > > > > +                ADIS16240_XYZPEAK_OUT);
> > > > > +
> > > > > +static IIO_CONST_ATTR_SAMP_FREQ_AVAIL("4096");
> > > > > +
> > > > > +static const u8 adis16240_addresses[][2] = {
> > > > > + [ADIS16240_SCAN_ACC_X] = { ADIS16240_XACCL_OFF, ADIS16240_XPEAK_OUT },
> > > > > + [ADIS16240_SCAN_ACC_Y] = { ADIS16240_YACCL_OFF, ADIS16240_YPEAK_OUT },
> > > > > + [ADIS16240_SCAN_ACC_Z] = { ADIS16240_ZACCL_OFF, ADIS16240_ZPEAK_OUT },
> > > > > +};
> > > > > +
> > > > > +static int adis16240_read_raw(struct iio_dev *indio_dev,
> > > > > +                       struct iio_chan_spec const *chan,
> > > > > +                       int *val, int *val2,
> > > > > +                       long mask)
> > > > > +{
> > > > > + struct adis *st = iio_priv(indio_dev);
> > > > > + int ret;
> > > > > + u8 addr;
> > > > > + s16 val16;
> > > > > +
> > > > > + switch (mask) {
> > > > > + case IIO_CHAN_INFO_RAW:
> > > > > +         return adis_single_conversion(indio_dev, chan,
> > > > > +                         ADIS16240_ERROR_ACTIVE, val);
> > > > > + case IIO_CHAN_INFO_SCALE:
> > > > > +         switch (chan->type) {
> > > > > +         case IIO_VOLTAGE:
> > > > > +                 if (chan->channel == 0) {
> > > > > +                         *val = 4;
> > > > > +                         *val2 = 880000; /* 4.88 mV */
> > > > > +                         return IIO_VAL_INT_PLUS_MICRO;
> > > > > +                 }
> > > > > +                 return -EINVAL;
> > > > > +         case IIO_TEMP:
> > > > > +                 *val = 244; /* 0.244 C */
> > > > > +                 *val2 = 0;
> > > >
> > > > This looks like a bug in the original driver.
> > > > I think *val = 0, and *val2 = 244 ?
>
> I have doubt about this. Also in adis16203, adis16209 and
> adis16201 decimal parts are in *val and not in *val2 on
> IIO_TEMP. What do you think? If it is really a bug, I can send
> a patch.
>
> > > > > +                 return IIO_VAL_INT_PLUS_MICRO;
> > > > > +         case IIO_ACCEL:
> > > > > +                 *val = 0;
> > > > > +                 *val2 = IIO_G_TO_M_S_2(51400); /* 51.4 mg */
> > > > > +                 return IIO_VAL_INT_PLUS_MICRO;
> > > > > +         default:
> > > > > +                 return -EINVAL;
> > > > > +         }
> > > > > +         break;
> > > > > + case IIO_CHAN_INFO_PEAK_SCALE:
> > > > > +         *val = 0;
> > > > > +         *val2 = IIO_G_TO_M_S_2(51400); /* 51.4 mg */
> > > > > +         return IIO_VAL_INT_PLUS_MICRO;
> > > > > + case IIO_CHAN_INFO_OFFSET:
> > > > > +         *val = 25000 / 244 - 0x133; /* 25 C = 0x133 */
> > > > > +         return IIO_VAL_INT;
> > > > > + case IIO_CHAN_INFO_CALIBBIAS:
> > > > > +         addr = adis16240_addresses[chan->scan_index][0];
> > > > > +         ret = adis_read_reg_16(st, addr, &val16);
> > > > > +         if (ret)
> > > > > +                 return ret;
> > > > > +         *val = sign_extend32(val16, 9);
> > > > > +         return IIO_VAL_INT;
> > > > > + case IIO_CHAN_INFO_PEAK:
> > > > > +         addr = adis16240_addresses[chan->scan_index][1];
> > > > > +         ret = adis_read_reg_16(st, addr, &val16);
> > > > > +         if (ret)
> > > > > +                 return ret;
> > > > > +         *val = sign_extend32(val16, 9);
> > > > > +         return IIO_VAL_INT;
> > > > > + }
> > > > > + return -EINVAL;
> > > > > +}
> > > > > +
> > > > > +static int adis16240_write_raw(struct iio_dev *indio_dev,
> > > > > +                        struct iio_chan_spec const *chan,
> > > > > +                        int val,
> > > > > +                        int val2,
> > > > > +                        long mask)
> > > > > +{
> > > > > + struct adis *st = iio_priv(indio_dev);
> > > > > + u8 addr;
> > > > > +
> > > > > + switch (mask) {
> > > > > + case IIO_CHAN_INFO_CALIBBIAS:
> > > > > +         addr = adis16240_addresses[chan->scan_index][0];
> > > > > +         return adis_write_reg_16(st, addr, val & GENMASK(9, 0));
> > > > > + }
> > > > > + return -EINVAL;
> > > > > +}
> > > > > +
> > > > > +static const struct iio_chan_spec adis16240_channels[] = {
> > > > > + ADIS_SUPPLY_CHAN(ADIS16240_SUPPLY_OUT, ADIS16240_SCAN_SUPPLY, 0, 10),
> > > > > + ADIS_AUX_ADC_CHAN(ADIS16240_AUX_ADC, ADIS16240_SCAN_AUX_ADC, 0, 10),
> > > > > + ADIS_ACCEL_CHAN(X, ADIS16240_XACCL_OUT, ADIS16240_SCAN_ACC_X,
> > > > > +                 BIT(IIO_CHAN_INFO_CALIBBIAS) | BIT(IIO_CHAN_INFO_PEAK),
> > > > > +                 0, 10),
> > > > > + ADIS_ACCEL_CHAN(Y, ADIS16240_YACCL_OUT, ADIS16240_SCAN_ACC_Y,
> > > > > +                 BIT(IIO_CHAN_INFO_CALIBBIAS) | BIT(IIO_CHAN_INFO_PEAK),
> > > > > +                 0, 10),
> > > > > + ADIS_ACCEL_CHAN(Z, ADIS16240_ZACCL_OUT, ADIS16240_SCAN_ACC_Z,
> > > > > +                 BIT(IIO_CHAN_INFO_CALIBBIAS) | BIT(IIO_CHAN_INFO_PEAK),
> > > > > +                 0, 10),
> > > > > + ADIS_TEMP_CHAN(ADIS16240_TEMP_OUT, ADIS16240_SCAN_TEMP, 0, 10),
> > > > > + IIO_CHAN_SOFT_TIMESTAMP(6)
> > > > > +};
> > > > > +
> > > > > +static struct attribute *adis16240_attributes[] = {
> > > > > + &iio_dev_attr_in_accel_xyz_squared_peak_raw.dev_attr.attr,
> > > >
> > > > This one looks a bit weird/rare.
> > > > I am not sure about it's use-case/need right now (at this point in time).
> > > >
> > > > Maybe we could just drop it?
> > > > Since I did not write the initial driver, I don't know yet what it was supposed to be/do.
> > >
> > > For an impact sensor it's a fairly useful thing I suspect as it is giving the maximum
> > > acceleration in any direction that occured (once you square root it in userspace.)
> > >
> > > > In any case, if we want to add it, it looks like it needs an ABI documentation.
> > >
> > > Agreed. Needs docs.  We actually have a modifier for it so can be handled
> > > as a channel. IIO_MOD_SUM_SQUARED_X_Y_Z :)
> > >
> > >
> > > >
> > > > > + &iio_const_attr_sampling_frequency_available.dev_attr.attr,
> > > >
> > > > This looks like it could be converted to IIO_CHAN_INFO_SAMP_FREQ attribute.
> > > >
> > > >
> > > > > + NULL
> > > > > +};
> > > > > +
> > > > > +static const struct attribute_group adis16240_attribute_group = {
> > > > > + .attrs = adis16240_attributes,
> > > > > +};
> > > > > +
> > > > > +static const struct iio_info adis16240_info = {
> > > > > + .attrs = &adis16240_attribute_group,
> > > > > + .read_raw = adis16240_read_raw,
> > > > > + .write_raw = adis16240_write_raw,
> > > > > + .update_scan_mode = adis_update_scan_mode,
> > > > > +};
> > > > > +
> > > > > +static const char * const adis16240_status_error_msgs[] = {
> > > > > + [ADIS16240_DIAG_STAT_PWRON_FAIL_BIT] = "Power on, self-test failed",
> > > > > + [ADIS16240_DIAG_STAT_SPI_FAIL_BIT] = "SPI failure",
> > > > > + [ADIS16240_DIAG_STAT_FLASH_UPT_BIT] = "Flash update failed",
> > > > > + [ADIS16240_DIAG_STAT_POWER_HIGH_BIT] = "Power supply above 3.625V",
> > > > > + [ADIS16240_DIAG_STAT_POWER_LOW_BIT] = "Power supply below 2.225V",
> > > > > +};
> > > > > +
> > > > > +static const struct adis_data adis16240_data = {
> > > > > + .write_delay = 35,
> > > > > + .read_delay = 35,
> > > > > + .msc_ctrl_reg = ADIS16240_MSC_CTRL,
> > > > > + .glob_cmd_reg = ADIS16240_GLOB_CMD,
> > > > > + .diag_stat_reg = ADIS16240_DIAG_STAT,
> > > > > +
> > > > > + .self_test_mask = ADIS16240_MSC_CTRL_SELF_TEST_EN,
> > > > > + .self_test_no_autoclear = true,
> > > > > + .startup_delay = ADIS16240_STARTUP_DELAY,
> > > > > +
> > > > > + .status_error_msgs = adis16240_status_error_msgs,
> > > > > + .status_error_mask = BIT(ADIS16240_DIAG_STAT_PWRON_FAIL_BIT) |
> > > > > +         BIT(ADIS16240_DIAG_STAT_SPI_FAIL_BIT) |
> > > > > +         BIT(ADIS16240_DIAG_STAT_FLASH_UPT_BIT) |
> > > > > +         BIT(ADIS16240_DIAG_STAT_POWER_HIGH_BIT) |
> > > > > +         BIT(ADIS16240_DIAG_STAT_POWER_LOW_BIT),
> > > > > +};
> > > > > +
> > > > > +static int adis16240_probe(struct spi_device *spi)
> > > > > +{
> > > > > + int ret;
> > > > > + struct adis *st;
> > > > > + struct iio_dev *indio_dev;
> > > > > +
> > > > > + /* setup the industrialio driver allocated elements */
> > >
> > > I would review comments and decide whether they actually add anything.
> > > This one for example - I'm not convinced!
> > >
> > > > > + indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
> > > > > + if (!indio_dev)
> > > > > +         return -ENOMEM;
> > > > > + st = iio_priv(indio_dev);
> > > > > + /* this is only used for removal purposes */
> > > > > + spi_set_drvdata(spi, indio_dev);
> > > > > +
> > > > > + indio_dev->name = spi->dev.driver->name;
> > > > > + indio_dev->dev.parent = &spi->dev;
> > > > > + indio_dev->info = &adis16240_info;
> > > > > + indio_dev->channels = adis16240_channels;
> > > > > + indio_dev->num_channels = ARRAY_SIZE(adis16240_channels);
> > > > > + indio_dev->modes = INDIO_DIRECT_MODE;
> > > > > +
> > > > > + ret = adis_init(st, indio_dev, spi, &adis16240_data);
> > > > > + if (ret)
> > > > > +         return ret;
> > >
> > > Might be nice to look at adding some white space after error handling
> > > blocks like this.  A blank line in locations like this make it
> > > easier for the eye to follow the code.
> > >
> > > > > + ret = adis_setup_buffer_and_trigger(st, indio_dev, NULL);
> > > > > + if (ret)
> > > > > +         return ret;
> > > > > +
> > > > > + /* Get the device into a sane initial state */
> > > > > + ret = adis_initial_startup(st);
> > > > > + if (ret)
> > > > > +         goto error_cleanup_buffer_trigger;
> > > > > + ret = iio_device_register(indio_dev);
> > > > > + if (ret)
> > > > > +         goto error_cleanup_buffer_trigger;
> > > > > + return 0;
> > > > > +
> > > > > +error_cleanup_buffer_trigger:
> > > > > + adis_cleanup_buffer_and_trigger(st, indio_dev);
> > > > > + return ret;
> > > > > +}
> > > > > +
> > > > > +static int adis16240_remove(struct spi_device *spi)
> > > > > +{
> > > > > + struct iio_dev *indio_dev = spi_get_drvdata(spi);
> > > > > + struct adis *st = iio_priv(indio_dev);
> > > > > +
> > > > > + iio_device_unregister(indio_dev);
> > > > > + adis_cleanup_buffer_and_trigger(st, indio_dev);
> > > > > +
> > > > > + return 0;
> > > > > +}
> > > > > +
> > > > > +static const struct of_device_id adis16240_of_match[] = {
> > > > > + { .compatible = "adi,adis16240" },
> > > > > + { },
> > > > > +};
> > > > > +MODULE_DEVICE_TABLE(of, adis16240_of_match);
> > > > > +
> > > > > +static struct spi_driver adis16240_driver = {
> > > > > + .driver = {
> > > > > +         .name = "adis16240",
> > > > > +         .of_match_table = adis16240_of_match,
> > > > > + },
> > > > > + .probe = adis16240_probe,
> > > > > + .remove = adis16240_remove,
> > > > > +};
> > > > > +module_spi_driver(adis16240_driver);
> > > > > +
> > > > > +MODULE_AUTHOR("Barry Song <21cnbao@gmail.com>");
> > > > > +MODULE_DESCRIPTION("Analog Devices Programmable Impact Sensor and Recorder");
> > > > > +MODULE_LICENSE("GPL v2");
> > > > > +MODULE_ALIAS("spi:adis16240");
> > > ...
>
> Thanks,
> Rodrigo

Re: [PATCH 2/2] staging: iio: accel: adis16240: move out of staging

[Jonathan Cameron]

On Thu, 12 Sep 2019 16:59:10 +0300
Alexandru Ardelean <ardeleanalex@gmail.com> wrote:

> On Wed, Sep 11, 2019 at 7:21 PM Rodrigo Carvalho <rodrigorsdc@gmail.com> wrote:
> >
> > Hi,
> >
> > Em seg, 9 de set de 2019 às 02:53, Ardelean, Alexandru
> > <alexandru.Ardelean@analog.com> escreveu:  
> > >
> > > On Sun, 2019-09-08 at 12:09 +0100, Jonathan Cameron wrote:  
> > > > On Mon, 2 Sep 2019 13:26:02 +0000
> > > > "Ardelean, Alexandru" <alexandru.Ardelean@analog.com> wrote:
> > > >  
> > > > > On Sun, 2019-09-01 at 21:59 -0300, Rodrigo Carvalho wrote:  
> > > > > > Move ADIS16240 driver from staging to mainline.
> > > > > >
> > > > > > The ADIS16240 is a fully integrated digital shock detection
> > > > > > and recorder system.  
> > > > >
> > > > > Hey,
> > > > >
> > > > > Comments inline.
> > > > >
> > > > > I'll probably take a look in the next days again.
> > > > > There seem to be some ABI/sysfs attributes that need to be resolved before moving this out of staging.  
> > > >
> > > > Absolutely. It is a 'new' type of device so there are definitely some
> > > > corners that need discussing before we move out of staging and commit
> > > > to maintaining the ABI moving forwards.
> > > >
> > > > That is the real reason this driver was still in staging!  No one
> > > > had been through the process of proposing the ABI and responding to
> > > > questions etc.
> > > >
> > > > The issue with impact sensors has always been that they don't really fit
> > > > our normal model for buffers or triggers.
> > > >
> > > > So normally a trigger (if exposed in IIO) is used as one trigger
> > > > causes 1 set of samples (so like a frame trigger for a camera).
> > > >
> > > > These devices tend to work in a mode where one trigger causes data
> > > > to be captured for a period of time.  In this part that's the event
> > > > recorder function
> > > >
> > > > No one is realistically going to buy an impact sensor to just use it
> > > > as an accelerometer which is what this driver is currently doing.
> > > > I suppose we could just leave support in that form for now, but
> > > > I'm no sure how much use it is to anyone.
> > > >
> > > > Analog Devices people, worth working out how to support the event
> > > > recorder?  For that someone needs to have hardware as it is complex
> > > > to say the least!  
> > >
> > > Worth it: yes.
> > > But we don't have any resources to allocate for this [at this point in time].
> > >  
> > > >
> > > > We could move it out but might be worth adding a comment somewhere
> > > > saying this only really supports direct access to channels, and
> > > > not the event recorder functionality.
> > > >  
> > >
> > > I guess, I would vote for leaving it in staging.
> > > It's also a way to mark it as a work-in-progress/not-done/still-needs-something kind of thing.
> > > If we move it now, it gets the status of "everything-resolved" which is not yet the case.
> > >
> > > Thanks for the insight/background info.
> > > Much of it was discussed before my time.  
> >
> > How about adis16203? Is it more simple to move this driver out of staging?  
> 
> Replying from my personal email, as I seem to have my periodical
> issues with the work-email.
> 
> At first glance I can't see any obvious blockers for this.
> 
> I think Jonathan may need to provide some feedback here.

There is a 'fixme' in the channels which I suspect is the remaining blocker
on this one.  The current y axis is actually just a different representation
of the value on the x axis.  I 'think' the only reason to support both 
of these is the interaction with the threshold detectors.   Those
are rather simple, so if you have -10 on the second channel which is 350 on the
first, it would detected as < 10 on the second but not the first.
It is ugly, but we may be better just treating these as two separate channels
on the same axis - similar to the way we would deal with the case of two
accelerometers in a package, with different ranges.

Also a rather odd bit of abstraction around _addresses given there
is only one specified.

Might be nice to clean out some of the less useful comments for registers
where the register name makes it clear what they are.  It's a mixture of
some useful ones and some pointless ones at the moment.

So a few bits to tidy up and may more come up in a thorough review.

Thanks,

Jonathan

> 
> Thanks
> Alex
> 
> >  
> > > >
> > > > Jonathan  
> > >
> > > Alex
> > >  
> > > >
> > > >  
> > > > > > Signed-off-by: Rodrigo Ribeiro Carvalho <rodrigorsdc@gmail.com>
> > > > > > ---
> > > > > >  drivers/iio/accel/Kconfig             |  12 +
> > > > > >  drivers/iio/accel/Makefile            |   1 +
> > > > > >  drivers/iio/accel/adis16240.c         | 454 ++++++++++++++++++++++++++
> > > > > >  drivers/staging/iio/accel/Kconfig     |  12 -
> > > > > >  drivers/staging/iio/accel/Makefile    |   1 -
> > > > > >  drivers/staging/iio/accel/adis16240.c | 454 --------------------------
> > > > > >  6 files changed, 467 insertions(+), 467 deletions(-)
> > > > > >  create mode 100644 drivers/iio/accel/adis16240.c
> > > > > >  delete mode 100644 drivers/staging/iio/accel/adis16240.c  
> > > > >
> > > > > Looks like MAINTAINERS file also needs to be updated, also with the DT bindings file.
> > > > > I think checkpatch usually complains about these.
> > > > >  
> > > > > > diff --git a/drivers/iio/accel/Kconfig b/drivers/iio/accel/Kconfig
> > > > > > index d4ef35aeb579..91fd8741c95f 100644
> > > > > > --- a/drivers/iio/accel/Kconfig
> > > > > > +++ b/drivers/iio/accel/Kconfig
> > > > > > @@ -30,6 +30,18 @@ config ADIS16209
> > > > > >     To compile this driver as a module, say M here: the module will be
> > > > > >     called adis16209.
> > > > > >
> > > > > > +config ADIS16240
> > > > > > + tristate "Analog Devices ADIS16240 Programmable Impact Sensor and Recorder"
> > > > > > + depends on SPI
> > > > > > + select IIO_ADIS_LIB
> > > > > > + select IIO_ADIS_LIB_BUFFER if IIO_BUFFER
> > > > > > + help
> > > > > > +   Say Y here to build support for Analog Devices adis16240 programmable
> > > > > > +   impact Sensor and recorder.
> > > > > > +
> > > > > > +   To compile this driver as a module, say M here: the module will be
> > > > > > +   called adis16240.
> > > > > > +
> > > > > >  config ADXL345
> > > > > >   tristate
> > > > > >
> > > > > > diff --git a/drivers/iio/accel/Makefile b/drivers/iio/accel/Makefile
> > > > > > index 56bd0215e0d4..f7e025a86dd9 100644
> > > > > > --- a/drivers/iio/accel/Makefile
> > > > > > +++ b/drivers/iio/accel/Makefile
> > > > > > @@ -6,6 +6,7 @@
> > > > > >  # When adding new entries keep the list in alphabetical order
> > > > > >  obj-$(CONFIG_ADIS16201) += adis16201.o
> > > > > >  obj-$(CONFIG_ADIS16209) += adis16209.o
> > > > > > +obj-$(CONFIG_ADIS16240) += adis16240.o
> > > > > >  obj-$(CONFIG_ADXL345) += adxl345_core.o
> > > > > >  obj-$(CONFIG_ADXL345_I2C) += adxl345_i2c.o
> > > > > >  obj-$(CONFIG_ADXL345_SPI) += adxl345_spi.o
> > > > > > diff --git a/drivers/iio/accel/adis16240.c b/drivers/iio/accel/adis16240.c
> > > > > > new file mode 100644
> > > > > > index 000000000000..82099db4bf0c
> > > > > > --- /dev/null
> > > > > > +++ b/drivers/iio/accel/adis16240.c
> > > > > > @@ -0,0 +1,454 @@
> > > > > > +// SPDX-License-Identifier: GPL-2.0+
> > > > > > +/*
> > > > > > + * ADIS16240 Programmable Impact Sensor and Recorder driver
> > > > > > + *
> > > > > > + * Copyright 2010 Analog Devices Inc.
> > > > > > + */
> > > > > > +
> > > > > > +#include <linux/interrupt.h>
> > > > > > +#include <linux/irq.h>
> > > > > > +#include <linux/gpio.h>
> > > > > > +#include <linux/delay.h>
> > > > > > +#include <linux/device.h>
> > > > > > +#include <linux/kernel.h>
> > > > > > +#include <linux/spi/spi.h>
> > > > > > +#include <linux/slab.h>
> > > > > > +#include <linux/sysfs.h>
> > > > > > +#include <linux/list.h>
> > > > > > +#include <linux/module.h>
> > > > > > +
> > > > > > +#include <linux/iio/iio.h>
> > > > > > +#include <linux/iio/sysfs.h>
> > > > > > +#include <linux/iio/buffer.h>
> > > > > > +#include <linux/iio/imu/adis.h>
> > > > > > +
> > > > > > +#define ADIS16240_STARTUP_DELAY  220 /* ms */
> > > > > > +
> > > > > > +/* Flash memory write count */
> > > > > > +#define ADIS16240_FLASH_CNT      0x00
> > > > > > +
> > > > > > +/* Output, power supply */
> > > > > > +#define ADIS16240_SUPPLY_OUT     0x02
> > > > > > +
> > > > > > +/* Output, x-axis accelerometer */
> > > > > > +#define ADIS16240_XACCL_OUT      0x04
> > > > > > +
> > > > > > +/* Output, y-axis accelerometer */
> > > > > > +#define ADIS16240_YACCL_OUT      0x06
> > > > > > +
> > > > > > +/* Output, z-axis accelerometer */
> > > > > > +#define ADIS16240_ZACCL_OUT      0x08
> > > > > > +
> > > > > > +/* Output, auxiliary ADC input */
> > > > > > +#define ADIS16240_AUX_ADC        0x0A
> > > > > > +
> > > > > > +/* Output, temperature */
> > > > > > +#define ADIS16240_TEMP_OUT       0x0C
> > > > > > +
> > > > > > +/* Output, x-axis acceleration peak */
> > > > > > +#define ADIS16240_XPEAK_OUT      0x0E
> > > > > > +
> > > > > > +/* Output, y-axis acceleration peak */
> > > > > > +#define ADIS16240_YPEAK_OUT      0x10
> > > > > > +
> > > > > > +/* Output, z-axis acceleration peak */
> > > > > > +#define ADIS16240_ZPEAK_OUT      0x12
> > > > > > +
> > > > > > +/* Output, sum-of-squares acceleration peak */
> > > > > > +#define ADIS16240_XYZPEAK_OUT    0x14
> > > > > > +
> > > > > > +/* Output, Capture Buffer 1, X and Y acceleration */
> > > > > > +#define ADIS16240_CAPT_BUF1      0x16
> > > > > > +
> > > > > > +/* Output, Capture Buffer 2, Z acceleration */
> > > > > > +#define ADIS16240_CAPT_BUF2      0x18
> > > > > > +
> > > > > > +/* Diagnostic, error flags */
> > > > > > +#define ADIS16240_DIAG_STAT      0x1A
> > > > > > +This looks like it could be converted to IIO_CHAN_INFO_SAMP_FREQ attribute.
> > > > > > +/* Diagnostic, event counter */
> > > > > > +#define ADIS16240_EVNT_CNTR      0x1C
> > > > > > +
> > > > > > +/* Diagnostic, check sum value from firmware test */
> > > > > > +#define ADIS16240_CHK_SUM        0x1E
> > > > > > +
> > > > > > +/* Calibration, x-axis acceleration offset adjustment */
> > > > > > +#define ADIS16240_XACCL_OFF      0x20
> > > > > > +
> > > > > > +/* Calibration, y-axis acceleration offset adjustment */
> > > > > > +#define ADIS16240_YACCL_OFF      0x22
> > > > > > +
> > > > > > +/* Calibration, z-axis acceleration offset adjustment */
> > > > > > +#define ADIS16240_ZACCL_OFF      0x24
> > > > > > +This looks like it could be converted to IIO_CHAN_INFO_SAMP_FREQ attribute.
> > > > > > +/* Clock, hour and minute */
> > > > > > +#define ADIS16240_CLK_TIME       0x2E
> > > > > > +
> > > > > > +/* Clock, month and day */
> > > > > > +#define ADIS16240_CLK_DATE       0x30
> > > > > > +
> > > > > > +/* Clock, year */
> > > > > > +#define ADIS16240_CLK_YEAR       0x32
> > > > > > +
> > > > > > +/* Wake-up setting, hour and minute */
> > > > > > +#define ADIS16240_WAKE_TIME      0x34
> > > > > > +
> > > > > > +/* Wake-up setting, month and day */
> > > > > > +#define ADIS16240_WAKE_DATE      0x36
> > > > > > +
> > > > > > +/* Alarm 1 amplitude threshold */
> > > > > > +#define ADIS16240_ALM_MAG1       0x38
> > > > > > +
> > > > > > +/* Alarm 2 amplitude threshold */
> > > > > > +#define ADIS16240_ALM_MAG2       0x3A
> > > > > > +
> > > > > > +/* Alarm control */
> > > > > > +#define ADIS16240_ALM_CTRL       0x3C
> > > > > > +
> > > > > > +/* Capture, external trigger control */
> > > > > > +#define ADIS16240_XTRIG_CTRL     0x3E
> > > > > > +
> > > > > > +/* Capture, address pointer */
> > > > > > +#define ADIS16240_CAPT_PNTR      0x40
> > > > > > +
> > > > > > +/* Capture, configuration and control */
> > > > > > +#define ADIS16240_CAPT_CTRL      0x42
> > > > > > +
> > > > > > +/* General-purpose digital input/output control */
> > > > > > +#define ADIS16240_GPIO_CTRL      0x44
> > > > > > +
> > > > > > +/* Miscellaneous control */
> > > > > > +#define ADIS16240_MSC_CTRL       0x46
> > > > > > +
> > > > > > +/* Internal sample period (rate) control */
> > > > > > +#define ADIS16240_SMPL_PRD       0x48
> > > > > > +
> > > > > > +/* System command */
> > > > > > +#define ADIS16240_GLOB_CMD       0x4A
> > > > > > +
> > > > > > +/* MSC_CTRL */
> > > > > > +
> > > > > > +/* Enables sum-of-squares output (XYZPEAK_OUT) */
> > > > > > +#define ADIS16240_MSC_CTRL_XYZPEAK_OUT_EN        BIT(15)
> > > > > > +
> > > > > > +/* Enables peak tracking output (XPEAK_OUT, YPEAK_OUT, and ZPEAK_OUT) */
> > > > > > +#define ADIS16240_MSC_CTRL_X_Y_ZPEAK_OUT_EN      BIT(14)
> > > > > > +
> > > > > > +/* Self-test enable: 1 = apply electrostatic force, 0 = disabled */
> > > > > > +#define ADIS16240_MSC_CTRL_SELF_TEST_EN          BIT(8)
> > > > > > +
> > > > > > +/* Data-ready enable: 1 = enabled, 0 = disabled */
> > > > > > +#define ADIS16240_MSC_CTRL_DATA_RDY_EN           BIT(2)
> > > > > > +
> > > > > > +/* Data-ready polarity: 1 = active high, 0 = active low */
> > > > > > +#define ADIS16240_MSC_CTRL_ACTIVE_HIGH           BIT(1)
> > > > > > +
> > > > > > +/* Data-ready line selection: 1 = DIO2, 0 = DIO1 */
> > > > > > +#define ADIS16240_MSC_CTRL_DATA_RDY_DIO2 BIT(0)
> > > > > > +
> > > > > > +/* DIAG_STAT */
> > > > > > +
> > > > > > +/* Alarm 2 status: 1 = alarm active, 0 = alarm inactive */
> > > > > > +#define ADIS16240_DIAG_STAT_ALARM2      BIT(9)
> > > > > > +
> > > > > > +/* Alarm 1 status: 1 = alarm active, 0 = alarm inactive */
> > > > > > +#define ADIS16240_DIAG_STAT_ALARM1      BIT(8)
> > > > > > +
> > > > > > +/* Capture buffer full: 1 = capture buffer is full */
> > > > > > +#define ADIS16240_DIAG_STAT_CPT_BUF_FUL BIT(7)
> > > > > > +
> > > > > > +/* Flash test, checksum flag: 1 = mismatch, 0 = match */
> > > > > > +#define ADIS16240_DIAG_STAT_CHKSUM      BIT(6)
> > > > > > +
> > > > > > +/* Power-on, self-test flag: 1 = failure, 0 = pass */
> > > > > > +#define ADIS16240_DIAG_STAT_PWRON_FAIL_BIT  5
> > > > > > +
> > > > > > +/* Power-on self-test: 1 = in-progress, 0 = complete */
> > > > > > +#define ADIS16240_DIAG_STAT_PWRON_BUSY  BIT(4)
> > > > > > +
> > > > > > +/* SPI communications failure */
> > > > > > +#define ADIS16240_DIAG_STAT_SPI_FAIL_BIT 3
> > > > > > +
> > > > > > +/* Flash update failure */
> > > > > > +#define ADIS16240_DIAG_STAT_FLASH_UPT_BIT        2
> > > > > > +
> > > > > > +/* Power supply above 3.625 V */
> > > > > > +#define ADIS16240_DIAG_STAT_POWER_HIGH_BIT       1
> > > > > > +
> > > > > > + /* Power supply below 2.225 V */
> > > > > > +#define ADIS16240_DIAG_STAT_POWER_LOW_BIT        0
> > > > > > +
> > > > > > +/* GLOB_CMD */
> > > > > > +
> > > > > > +#define ADIS16240_GLOB_CMD_RESUME        BIT(8)
> > > > > > +#define ADIS16240_GLOB_CMD_SW_RESET      BIT(7)
> > > > > > +#define ADIS16240_GLOB_CMD_STANDBY       BIT(2)
> > > > > > +
> > > > > > +#define ADIS16240_ERROR_ACTIVE          BIT(14)
> > > > > > +
> > > > > > +/* At the moment triggers are only used for ring buffer
> > > > > > + * filling. This may change!
> > > > > > + */
> > > > > > +
> > > > > > +enum adis16240_scan {
> > > > > > + ADIS16240_SCAN_ACC_X,
> > > > > > + ADIS16240_SCAN_ACC_Y,
> > > > > > + ADIS16240_SCAN_ACC_Z,
> > > > > > + ADIS16240_SCAN_SUPPLY,
> > > > > > + ADIS16240_SCAN_AUX_ADC,
> > > > > > + ADIS16240_SCAN_TEMP,
> > > > > > +};
> > > > > > +
> > > > > > +static ssize_t adis16240_spi_read_signed(struct device *dev,
> > > > > > +                                  struct device_attribute *attr,
> > > > > > +                                  char *buf,
> > > > > > +                                  unsigned int bits)
> > > > > > +{
> > > > > > + struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> > > > > > + struct adis *st = iio_priv(indio_dev);
> > > > > > + int ret;
> > > > > > + s16 val = 0;
> > > > > > + unsigned int shift = 16 - bits;
> > > > > > + struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
> > > > > > +
> > > > > > + ret = adis_read_reg_16(st,
> > > > > > +                        this_attr->address, (u16 *)&val);  
> > > >
> > > > Check for odd line breaks.  That looks like it'll fit under 80 chars
> > > > just fine.
> > > >  
> > > > > > + if (ret)
> > > > > > +         return ret;
> > > > > > +
> > > > > > + if (val & ADIS16240_ERROR_ACTIVE)
> > > > > > +         adis_check_status(st);
> > > > > > +
> > > > > > + val = (s16)(val << shift) >> shift;  
> > > >
> > > > Use sign_extend32 (added after this driver I think!)
> > > >  
> > > > > > + return sprintf(buf, "%d\n", val);
> > > > > > +}
> > > > > > +
> > > > > > +static ssize_t adis16240_read_12bit_signed(struct device *dev,
> > > > > > +                                    struct device_attribute *attr,
> > > > > > +                                    char *buf)
> > > > > > +{
> > > > > > + return adis16240_spi_read_signed(dev, attr, buf, 12);
> > > > > > +}
> > > > > > +
> > > > > > +static IIO_DEVICE_ATTR(in_accel_xyz_squared_peak_raw, 0444,
> > > > > > +                adis16240_read_12bit_signed, NULL,
> > > > > > +                ADIS16240_XYZPEAK_OUT);
> > > > > > +
> > > > > > +static IIO_CONST_ATTR_SAMP_FREQ_AVAIL("4096");
> > > > > > +
> > > > > > +static const u8 adis16240_addresses[][2] = {
> > > > > > + [ADIS16240_SCAN_ACC_X] = { ADIS16240_XACCL_OFF, ADIS16240_XPEAK_OUT },
> > > > > > + [ADIS16240_SCAN_ACC_Y] = { ADIS16240_YACCL_OFF, ADIS16240_YPEAK_OUT },
> > > > > > + [ADIS16240_SCAN_ACC_Z] = { ADIS16240_ZACCL_OFF, ADIS16240_ZPEAK_OUT },
> > > > > > +};
> > > > > > +
> > > > > > +static int adis16240_read_raw(struct iio_dev *indio_dev,
> > > > > > +                       struct iio_chan_spec const *chan,
> > > > > > +                       int *val, int *val2,
> > > > > > +                       long mask)
> > > > > > +{
> > > > > > + struct adis *st = iio_priv(indio_dev);
> > > > > > + int ret;
> > > > > > + u8 addr;
> > > > > > + s16 val16;
> > > > > > +
> > > > > > + switch (mask) {
> > > > > > + case IIO_CHAN_INFO_RAW:
> > > > > > +         return adis_single_conversion(indio_dev, chan,
> > > > > > +                         ADIS16240_ERROR_ACTIVE, val);
> > > > > > + case IIO_CHAN_INFO_SCALE:
> > > > > > +         switch (chan->type) {
> > > > > > +         case IIO_VOLTAGE:
> > > > > > +                 if (chan->channel == 0) {
> > > > > > +                         *val = 4;
> > > > > > +                         *val2 = 880000; /* 4.88 mV */
> > > > > > +                         return IIO_VAL_INT_PLUS_MICRO;
> > > > > > +                 }
> > > > > > +                 return -EINVAL;
> > > > > > +         case IIO_TEMP:
> > > > > > +                 *val = 244; /* 0.244 C */
> > > > > > +                 *val2 = 0;  
> > > > >
> > > > > This looks like a bug in the original driver.
> > > > > I think *val = 0, and *val2 = 244 ?  
> >
> > I have doubt about this. Also in adis16203, adis16209 and
> > adis16201 decimal parts are in *val and not in *val2 on
> > IIO_TEMP. What do you think? If it is really a bug, I can send
> > a patch.
> >  
> > > > > > +                 return IIO_VAL_INT_PLUS_MICRO;
> > > > > > +         case IIO_ACCEL:
> > > > > > +                 *val = 0;
> > > > > > +                 *val2 = IIO_G_TO_M_S_2(51400); /* 51.4 mg */
> > > > > > +                 return IIO_VAL_INT_PLUS_MICRO;
> > > > > > +         default:
> > > > > > +                 return -EINVAL;
> > > > > > +         }
> > > > > > +         break;
> > > > > > + case IIO_CHAN_INFO_PEAK_SCALE:
> > > > > > +         *val = 0;
> > > > > > +         *val2 = IIO_G_TO_M_S_2(51400); /* 51.4 mg */
> > > > > > +         return IIO_VAL_INT_PLUS_MICRO;
> > > > > > + case IIO_CHAN_INFO_OFFSET:
> > > > > > +         *val = 25000 / 244 - 0x133; /* 25 C = 0x133 */
> > > > > > +         return IIO_VAL_INT;
> > > > > > + case IIO_CHAN_INFO_CALIBBIAS:
> > > > > > +         addr = adis16240_addresses[chan->scan_index][0];
> > > > > > +         ret = adis_read_reg_16(st, addr, &val16);
> > > > > > +         if (ret)
> > > > > > +                 return ret;
> > > > > > +         *val = sign_extend32(val16, 9);
> > > > > > +         return IIO_VAL_INT;
> > > > > > + case IIO_CHAN_INFO_PEAK:
> > > > > > +         addr = adis16240_addresses[chan->scan_index][1];
> > > > > > +         ret = adis_read_reg_16(st, addr, &val16);
> > > > > > +         if (ret)
> > > > > > +                 return ret;
> > > > > > +         *val = sign_extend32(val16, 9);
> > > > > > +         return IIO_VAL_INT;
> > > > > > + }
> > > > > > + return -EINVAL;
> > > > > > +}
> > > > > > +
> > > > > > +static int adis16240_write_raw(struct iio_dev *indio_dev,
> > > > > > +                        struct iio_chan_spec const *chan,
> > > > > > +                        int val,
> > > > > > +                        int val2,
> > > > > > +                        long mask)
> > > > > > +{
> > > > > > + struct adis *st = iio_priv(indio_dev);
> > > > > > + u8 addr;
> > > > > > +
> > > > > > + switch (mask) {
> > > > > > + case IIO_CHAN_INFO_CALIBBIAS:
> > > > > > +         addr = adis16240_addresses[chan->scan_index][0];
> > > > > > +         return adis_write_reg_16(st, addr, val & GENMASK(9, 0));
> > > > > > + }
> > > > > > + return -EINVAL;
> > > > > > +}
> > > > > > +
> > > > > > +static const struct iio_chan_spec adis16240_channels[] = {
> > > > > > + ADIS_SUPPLY_CHAN(ADIS16240_SUPPLY_OUT, ADIS16240_SCAN_SUPPLY, 0, 10),
> > > > > > + ADIS_AUX_ADC_CHAN(ADIS16240_AUX_ADC, ADIS16240_SCAN_AUX_ADC, 0, 10),
> > > > > > + ADIS_ACCEL_CHAN(X, ADIS16240_XACCL_OUT, ADIS16240_SCAN_ACC_X,
> > > > > > +                 BIT(IIO_CHAN_INFO_CALIBBIAS) | BIT(IIO_CHAN_INFO_PEAK),
> > > > > > +                 0, 10),
> > > > > > + ADIS_ACCEL_CHAN(Y, ADIS16240_YACCL_OUT, ADIS16240_SCAN_ACC_Y,
> > > > > > +                 BIT(IIO_CHAN_INFO_CALIBBIAS) | BIT(IIO_CHAN_INFO_PEAK),
> > > > > > +                 0, 10),
> > > > > > + ADIS_ACCEL_CHAN(Z, ADIS16240_ZACCL_OUT, ADIS16240_SCAN_ACC_Z,
> > > > > > +                 BIT(IIO_CHAN_INFO_CALIBBIAS) | BIT(IIO_CHAN_INFO_PEAK),
> > > > > > +                 0, 10),
> > > > > > + ADIS_TEMP_CHAN(ADIS16240_TEMP_OUT, ADIS16240_SCAN_TEMP, 0, 10),
> > > > > > + IIO_CHAN_SOFT_TIMESTAMP(6)
> > > > > > +};
> > > > > > +
> > > > > > +static struct attribute *adis16240_attributes[] = {
> > > > > > + &iio_dev_attr_in_accel_xyz_squared_peak_raw.dev_attr.attr,  
> > > > >
> > > > > This one looks a bit weird/rare.
> > > > > I am not sure about it's use-case/need right now (at this point in time).
> > > > >
> > > > > Maybe we could just drop it?
> > > > > Since I did not write the initial driver, I don't know yet what it was supposed to be/do.  
> > > >
> > > > For an impact sensor it's a fairly useful thing I suspect as it is giving the maximum
> > > > acceleration in any direction that occured (once you square root it in userspace.)
> > > >  
> > > > > In any case, if we want to add it, it looks like it needs an ABI documentation.  
> > > >
> > > > Agreed. Needs docs.  We actually have a modifier for it so can be handled
> > > > as a channel. IIO_MOD_SUM_SQUARED_X_Y_Z :)
> > > >
> > > >  
> > > > >  
> > > > > > + &iio_const_attr_sampling_frequency_available.dev_attr.attr,  
> > > > >
> > > > > This looks like it could be converted to IIO_CHAN_INFO_SAMP_FREQ attribute.
> > > > >
> > > > >  
> > > > > > + NULL
> > > > > > +};
> > > > > > +
> > > > > > +static const struct attribute_group adis16240_attribute_group = {
> > > > > > + .attrs = adis16240_attributes,
> > > > > > +};
> > > > > > +
> > > > > > +static const struct iio_info adis16240_info = {
> > > > > > + .attrs = &adis16240_attribute_group,
> > > > > > + .read_raw = adis16240_read_raw,
> > > > > > + .write_raw = adis16240_write_raw,
> > > > > > + .update_scan_mode = adis_update_scan_mode,
> > > > > > +};
> > > > > > +
> > > > > > +static const char * const adis16240_status_error_msgs[] = {
> > > > > > + [ADIS16240_DIAG_STAT_PWRON_FAIL_BIT] = "Power on, self-test failed",
> > > > > > + [ADIS16240_DIAG_STAT_SPI_FAIL_BIT] = "SPI failure",
> > > > > > + [ADIS16240_DIAG_STAT_FLASH_UPT_BIT] = "Flash update failed",
> > > > > > + [ADIS16240_DIAG_STAT_POWER_HIGH_BIT] = "Power supply above 3.625V",
> > > > > > + [ADIS16240_DIAG_STAT_POWER_LOW_BIT] = "Power supply below 2.225V",
> > > > > > +};
> > > > > > +
> > > > > > +static const struct adis_data adis16240_data = {
> > > > > > + .write_delay = 35,
> > > > > > + .read_delay = 35,
> > > > > > + .msc_ctrl_reg = ADIS16240_MSC_CTRL,
> > > > > > + .glob_cmd_reg = ADIS16240_GLOB_CMD,
> > > > > > + .diag_stat_reg = ADIS16240_DIAG_STAT,
> > > > > > +
> > > > > > + .self_test_mask = ADIS16240_MSC_CTRL_SELF_TEST_EN,
> > > > > > + .self_test_no_autoclear = true,
> > > > > > + .startup_delay = ADIS16240_STARTUP_DELAY,
> > > > > > +
> > > > > > + .status_error_msgs = adis16240_status_error_msgs,
> > > > > > + .status_error_mask = BIT(ADIS16240_DIAG_STAT_PWRON_FAIL_BIT) |
> > > > > > +         BIT(ADIS16240_DIAG_STAT_SPI_FAIL_BIT) |
> > > > > > +         BIT(ADIS16240_DIAG_STAT_FLASH_UPT_BIT) |
> > > > > > +         BIT(ADIS16240_DIAG_STAT_POWER_HIGH_BIT) |
> > > > > > +         BIT(ADIS16240_DIAG_STAT_POWER_LOW_BIT),
> > > > > > +};
> > > > > > +
> > > > > > +static int adis16240_probe(struct spi_device *spi)
> > > > > > +{
> > > > > > + int ret;
> > > > > > + struct adis *st;
> > > > > > + struct iio_dev *indio_dev;
> > > > > > +
> > > > > > + /* setup the industrialio driver allocated elements */  
> > > >
> > > > I would review comments and decide whether they actually add anything.
> > > > This one for example - I'm not convinced!
> > > >  
> > > > > > + indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
> > > > > > + if (!indio_dev)
> > > > > > +         return -ENOMEM;
> > > > > > + st = iio_priv(indio_dev);
> > > > > > + /* this is only used for removal purposes */
> > > > > > + spi_set_drvdata(spi, indio_dev);
> > > > > > +
> > > > > > + indio_dev->name = spi->dev.driver->name;
> > > > > > + indio_dev->dev.parent = &spi->dev;
> > > > > > + indio_dev->info = &adis16240_info;
> > > > > > + indio_dev->channels = adis16240_channels;
> > > > > > + indio_dev->num_channels = ARRAY_SIZE(adis16240_channels);
> > > > > > + indio_dev->modes = INDIO_DIRECT_MODE;
> > > > > > +
> > > > > > + ret = adis_init(st, indio_dev, spi, &adis16240_data);
> > > > > > + if (ret)
> > > > > > +         return ret;  
> > > >
> > > > Might be nice to look at adding some white space after error handling
> > > > blocks like this.  A blank line in locations like this make it
> > > > easier for the eye to follow the code.
> > > >  
> > > > > > + ret = adis_setup_buffer_and_trigger(st, indio_dev, NULL);
> > > > > > + if (ret)
> > > > > > +         return ret;
> > > > > > +
> > > > > > + /* Get the device into a sane initial state */
> > > > > > + ret = adis_initial_startup(st);
> > > > > > + if (ret)
> > > > > > +         goto error_cleanup_buffer_trigger;
> > > > > > + ret = iio_device_register(indio_dev);
> > > > > > + if (ret)
> > > > > > +         goto error_cleanup_buffer_trigger;
> > > > > > + return 0;
> > > > > > +
> > > > > > +error_cleanup_buffer_trigger:
> > > > > > + adis_cleanup_buffer_and_trigger(st, indio_dev);
> > > > > > + return ret;
> > > > > > +}
> > > > > > +
> > > > > > +static int adis16240_remove(struct spi_device *spi)
> > > > > > +{
> > > > > > + struct iio_dev *indio_dev = spi_get_drvdata(spi);
> > > > > > + struct adis *st = iio_priv(indio_dev);
> > > > > > +
> > > > > > + iio_device_unregister(indio_dev);
> > > > > > + adis_cleanup_buffer_and_trigger(st, indio_dev);
> > > > > > +
> > > > > > + return 0;
> > > > > > +}
> > > > > > +
> > > > > > +static const struct of_device_id adis16240_of_match[] = {
> > > > > > + { .compatible = "adi,adis16240" },
> > > > > > + { },
> > > > > > +};
> > > > > > +MODULE_DEVICE_TABLE(of, adis16240_of_match);
> > > > > > +
> > > > > > +static struct spi_driver adis16240_driver = {
> > > > > > + .driver = {
> > > > > > +         .name = "adis16240",
> > > > > > +         .of_match_table = adis16240_of_match,
> > > > > > + },
> > > > > > + .probe = adis16240_probe,
> > > > > > + .remove = adis16240_remove,
> > > > > > +};
> > > > > > +module_spi_driver(adis16240_driver);
> > > > > > +
> > > > > > +MODULE_AUTHOR("Barry Song <21cnbao@gmail.com>");
> > > > > > +MODULE_DESCRIPTION("Analog Devices Programmable Impact Sensor and Recorder");
> > > > > > +MODULE_LICENSE("GPL v2");
> > > > > > +MODULE_ALIAS("spi:adis16240");  
> > > > ...  
> >
> > Thanks,
> > Rodrigo  

Re: [PATCH 1/2] dt-bindings: iio: accel: add binding documentation for ADIS16240

[Ardelean, Alexandru]

On Sun, 2019-09-01 at 21:59 -0300, Rodrigo Carvalho wrote:
> This patch add device tree binding documentation for ADIS16240.
> 
> Signed-off-by: Rodrigo Ribeiro Carvalho <rodrigorsdc@gmail.com>
> ---
> I have doubt about what maintainer I may to put in that documentation. I
> put Alexandru as maintainer because he reviewed my last patch on this
> driver, so I think that he is a good candidate.

Fine to list me as maintainer for this.

Reviewed-by: Alexandru Ardelean <alexandru.ardelean@analog.com>

>  .../bindings/iio/accel/adi,adis16240.yaml     | 55 +++++++++++++++++++
>  1 file changed, 55 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/accel/adi,adis16240.yaml
> 
> diff --git a/Documentation/devicetree/bindings/iio/accel/adi,adis16240.yaml
> b/Documentation/devicetree/bindings/iio/accel/adi,adis16240.yaml
> new file mode 100644
> index 000000000000..08019b51611c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/accel/adi,adis16240.yaml
> @@ -0,0 +1,55 @@
> +# SPDX-License-Identifier: GPL-2.0
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/accel/adi,adis16240.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: ADIS16240 Programmable Impact Sensor and Recorder driver
> +
> +maintainers:
> +  - Alexandru Ardelean <alexandru.ardelean@analog.com>
> +
> +description: |
> +  ADIS16240 Programmable Impact Sensor and Recorder driver that supports
> +  SPI interface.
> +    https://www.analog.com/en/products/adis16240.html
> +
> +properties:
> +  compatible:
> +    enum:
> +      - adi,adis16240
> +
> +  reg:
> +    maxItems: 1
> +
> +  spi-cpha: true
> +
> +  spi-cpol: true
> +
> +  interrupts:
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +
> +examples:
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +    spi0 {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        /* Example for a SPI device node */
> +        accelerometer@0 {
> +            compatible = "adi,adis16240";
> +            reg = <0>;
> +            spi-max-frequency = <2500000>;
> +            spi-cpol;
> +            spi-cpha;
> +            interrupt-parent = <&gpio0>;
> +            interrupts = <0 IRQ_TYPE_LEVEL_HIGH>;
> +        };
> +    };

Re: [PATCH 1/2] dt-bindings: iio: accel: add binding documentation for ADIS16240

[Marcelo Schmitt]

Hi Rodrigo,

This dt doc looks overal fine IMHO.
I would just add some inline comments about the cpha and cpol
properties.

On 09/01, Rodrigo Carvalho wrote:
> This patch add device tree binding documentation for ADIS16240.
> 
> Signed-off-by: Rodrigo Ribeiro Carvalho <rodrigorsdc@gmail.com>
> ---
> I have doubt about what maintainer I may to put in that documentation. I
> put Alexandru as maintainer because he reviewed my last patch on this
> driver, so I think that he is a good candidate.
>  .../bindings/iio/accel/adi,adis16240.yaml     | 55 +++++++++++++++++++
>  1 file changed, 55 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/accel/adi,adis16240.yaml
> 
> diff --git a/Documentation/devicetree/bindings/iio/accel/adi,adis16240.yaml b/Documentation/devicetree/bindings/iio/accel/adi,adis16240.yaml
> new file mode 100644
> index 000000000000..08019b51611c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/accel/adi,adis16240.yaml
> @@ -0,0 +1,55 @@
> +# SPDX-License-Identifier: GPL-2.0
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/accel/adi,adis16240.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: ADIS16240 Programmable Impact Sensor and Recorder driver
> +
> +maintainers:
> +  - Alexandru Ardelean <alexandru.ardelean@analog.com>
> +
> +description: |
> +  ADIS16240 Programmable Impact Sensor and Recorder driver that supports
> +  SPI interface.
> +    https://www.analog.com/en/products/adis16240.html
> +
> +properties:
> +  compatible:
> +    enum:
> +      - adi,adis16240
> +
> +  reg:
> +    maxItems: 1
> +
> +  spi-cpha: true
> +
> +  spi-cpol: true
Boolean properties don't require to be explicitly set. It would also be
nice to add a description pointing to the spi-bus documentation. Like
this:

  spi-cpha:
    description: |
      See Documentation/devicetree/bindings/spi/spi-bus.txt
    maxItems: 1

  spi-cpol:
    description: |
      See Documentation/devicetree/bindings/spi/spi-bus.txt
    maxItems: 1

As far as I know, spi-cpol and spi-cpha stand for SPI chip polarity and
SPI chip phase respectively. By default, it is assumed that SPI
input/output data is available at uprising clock edges, however, some
chips may work with different configuration (taking input data and/or
push it out in falling edges). I'm not 100% sure but, from what I've
seen on IIO, cpol is set to invert the input/output logic (making IO be
taken on falling edges) while cpha is usually set when MISO valid out
data is available on SCLK falling edge. If anyone has more comments
about this please, add them here, I'm curious about it. :)

> +
> +  interrupts:
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +
> +examples:
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +    spi0 {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        /* Example for a SPI device node */
> +        accelerometer@0 {
> +            compatible = "adi,adis16240";
> +            reg = <0>;
> +            spi-max-frequency = <2500000>;
> +            spi-cpol;
> +            spi-cpha;
> +            interrupt-parent = <&gpio0>;
> +            interrupts = <0 IRQ_TYPE_LEVEL_HIGH>;
> +        };
> +    };
> -- 
> 2.23.0.rc1
> 
> -- 
> You received this message because you are subscribed to the Google Groups "Kernel USP" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to kernel-usp+unsubscribe@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/kernel-usp/20190902005938.7734-1-rodrigorsdc%40gmail.com.

Re: [PATCH 1/2] dt-bindings: iio: accel: add binding documentation for ADIS16240

[Jonathan Cameron]

On Mon, 2 Sep 2019 14:14:18 -0300
Marcelo Schmitt <marcelo.schmitt1@gmail.com> wrote:

> Hi Rodrigo,
> 
> This dt doc looks overal fine IMHO.
> I would just add some inline comments about the cpha and cpol
> properties.
> 
> On 09/01, Rodrigo Carvalho wrote:
> > This patch add device tree binding documentation for ADIS16240.
> > 
> > Signed-off-by: Rodrigo Ribeiro Carvalho <rodrigorsdc@gmail.com>
> > ---
> > I have doubt about what maintainer I may to put in that documentation. I
> > put Alexandru as maintainer because he reviewed my last patch on this
> > driver, so I think that he is a good candidate.
> >  .../bindings/iio/accel/adi,adis16240.yaml     | 55 +++++++++++++++++++
> >  1 file changed, 55 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/iio/accel/adi,adis16240.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/iio/accel/adi,adis16240.yaml b/Documentation/devicetree/bindings/iio/accel/adi,adis16240.yaml
> > new file mode 100644
> > index 000000000000..08019b51611c
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/accel/adi,adis16240.yaml
> > @@ -0,0 +1,55 @@
> > +# SPDX-License-Identifier: GPL-2.0
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/iio/accel/adi,adis16240.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: ADIS16240 Programmable Impact Sensor and Recorder driver
> > +
> > +maintainers:
> > +  - Alexandru Ardelean <alexandru.ardelean@analog.com>
> > +
> > +description: |
> > +  ADIS16240 Programmable Impact Sensor and Recorder driver that supports
> > +  SPI interface.
> > +    https://www.analog.com/en/products/adis16240.html
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - adi,adis16240
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  spi-cpha: true
> > +
> > +  spi-cpol: true  
> Boolean properties don't require to be explicitly set. It would also be
> nice to add a description pointing to the spi-bus documentation. Like
> this:
> 
>   spi-cpha:
>     description: |
>       See Documentation/devicetree/bindings/spi/spi-bus.txt
>     maxItems: 1
> 
>   spi-cpol:
>     description: |
>       See Documentation/devicetree/bindings/spi/spi-bus.txt
>     maxItems: 1
> 
> As far as I know, spi-cpol and spi-cpha stand for SPI chip polarity and
> SPI chip phase respectively. By default, it is assumed that SPI
> input/output data is available at uprising clock edges, however, some
> chips may work with different configuration (taking input data and/or
> push it out in falling edges). I'm not 100% sure but, from what I've
> seen on IIO, cpol is set to invert the input/output logic (making IO be
> taken on falling edges) while cpha is usually set when MISO valid out
> data is available on SCLK falling edge. If anyone has more comments
> about this please, add them here, I'm curious about it. :)
> 

They may well be constant for a given device (some will cope with
several combinations).  So the binding should reflect if they 'must'
be set.

Adding the cross reference is indeed nice.

Jonathan

> > +
> > +  interrupts:
> > +    maxItems: 1
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - interrupts
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/gpio/gpio.h>
> > +    #include <dt-bindings/interrupt-controller/irq.h>
> > +    spi0 {
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +
> > +        /* Example for a SPI device node */
> > +        accelerometer@0 {
> > +            compatible = "adi,adis16240";
> > +            reg = <0>;
> > +            spi-max-frequency = <2500000>;
> > +            spi-cpol;
> > +            spi-cpha;
> > +            interrupt-parent = <&gpio0>;
> > +            interrupts = <0 IRQ_TYPE_LEVEL_HIGH>;
> > +        };
> > +    };
> > -- 
> > 2.23.0.rc1
> > 
> > -- 
> > You received this message because you are subscribed to the Google Groups "Kernel USP" group.
> > To unsubscribe from this group and stop receiving emails from it, send an email to kernel-usp+unsubscribe@googlegroups.com.
> > To view this discussion on the web visit https://groups.google.com/d/msgid/kernel-usp/20190902005938.7734-1-rodrigorsdc%40gmail.com.  

Re: [PATCH 1/2] dt-bindings: iio: accel: add binding documentation for ADIS16240

[Rodrigo Carvalho]

Hi,

Thanks for comments. I will send a v2.

Em dom, 8 de set de 2019 às 07:52, Jonathan Cameron <jic23@kernel.org> escreveu:
>
> On Mon, 2 Sep 2019 14:14:18 -0300
> Marcelo Schmitt <marcelo.schmitt1@gmail.com> wrote:
>
> > Hi Rodrigo,
> >
> > This dt doc looks overal fine IMHO.
> > I would just add some inline comments about the cpha and cpol
> > properties.
> >
> > On 09/01, Rodrigo Carvalho wrote:
> > > This patch add device tree binding documentation for ADIS16240.
> > >
> > > Signed-off-by: Rodrigo Ribeiro Carvalho <rodrigorsdc@gmail.com>
> > > ---
> > > I have doubt about what maintainer I may to put in that documentation. I
> > > put Alexandru as maintainer because he reviewed my last patch on this
> > > driver, so I think that he is a good candidate.
> > >  .../bindings/iio/accel/adi,adis16240.yaml     | 55 +++++++++++++++++++
> > >  1 file changed, 55 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/iio/accel/adi,adis16240.yaml
> > >
> > > diff --git a/Documentation/devicetree/bindings/iio/accel/adi,adis16240.yaml b/Documentation/devicetree/bindings/iio/accel/adi,adis16240.yaml
> > > new file mode 100644
> > > index 000000000000..08019b51611c
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/iio/accel/adi,adis16240.yaml
> > > @@ -0,0 +1,55 @@
> > > +# SPDX-License-Identifier: GPL-2.0
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/iio/accel/adi,adis16240.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: ADIS16240 Programmable Impact Sensor and Recorder driver
> > > +
> > > +maintainers:
> > > +  - Alexandru Ardelean <alexandru.ardelean@analog.com>
> > > +
> > > +description: |
> > > +  ADIS16240 Programmable Impact Sensor and Recorder driver that supports
> > > +  SPI interface.
> > > +    https://www.analog.com/en/products/adis16240.html
> > > +
> > > +properties:
> > > +  compatible:
> > > +    enum:
> > > +      - adi,adis16240
> > > +
> > > +  reg:
> > > +    maxItems: 1
> > > +
> > > +  spi-cpha: true
> > > +
> > > +  spi-cpol: true
> > Boolean properties don't require to be explicitly set. It would also be
> > nice to add a description pointing to the spi-bus documentation. Like
> > this:
> >
> >   spi-cpha:
> >     description: |
> >       See Documentation/devicetree/bindings/spi/spi-bus.txt
> >     maxItems: 1
> >
> >   spi-cpol:
> >     description: |
> >       See Documentation/devicetree/bindings/spi/spi-bus.txt
> >     maxItems: 1
> >
> > As far as I know, spi-cpol and spi-cpha stand for SPI chip polarity and
> > SPI chip phase respectively. By default, it is assumed that SPI
> > input/output data is available at uprising clock edges, however, some
> > chips may work with different configuration (taking input data and/or
> > push it out in falling edges). I'm not 100% sure but, from what I've
> > seen on IIO, cpol is set to invert the input/output logic (making IO be
> > taken on falling edges) while cpha is usually set when MISO valid out
> > data is available on SCLK falling edge. If anyone has more comments
> > about this please, add them here, I'm curious about it. :)
> >
>
> They may well be constant for a given device (some will cope with
> several combinations).  So the binding should reflect if they 'must'
> be set.
>
> Adding the cross reference is indeed nice.
>
> Jonathan
>
> > > +
> > > +  interrupts:
> > > +    maxItems: 1
> > > +
> > > +required:
> > > +  - compatible
> > > +  - reg
> > > +  - interrupts
> > > +
> > > +examples:
> > > +  - |
> > > +    #include <dt-bindings/gpio/gpio.h>
> > > +    #include <dt-bindings/interrupt-controller/irq.h>
> > > +    spi0 {
> > > +        #address-cells = <1>;
> > > +        #size-cells = <0>;
> > > +
> > > +        /* Example for a SPI device node */
> > > +        accelerometer@0 {
> > > +            compatible = "adi,adis16240";
> > > +            reg = <0>;
> > > +            spi-max-frequency = <2500000>;
> > > +            spi-cpol;
> > > +            spi-cpha;
> > > +            interrupt-parent = <&gpio0>;
> > > +            interrupts = <0 IRQ_TYPE_LEVEL_HIGH>;
> > > +        };
> > > +    };
> > > --
> > > 2.23.0.rc1
> > >
> > > --
> > > You received this message because you are subscribed to the Google Groups "Kernel USP" group.
> > > To unsubscribe from this group and stop receiving emails from it, send an email to kernel-usp+unsubscribe@googlegroups.com.
> > > To view this discussion on the web visit https://groups.google.com/d/msgid/kernel-usp/20190902005938.7734-1-rodrigorsdc%40gmail.com.
>