iio: Support ROHM/Kionix kx022a

KX022A is a 3-axis Accelerometer from ROHM/Kionix. The senor features include variable ODRs, I2C and SPI control, FIFO/LIFO with watermark IRQ,

tap/motion detection, wake-up & back-to-sleep events, four acceleration ranges (2, 4, 8 and 16g) and probably some other cool fatures.

Documentation/devicetree/bindings/iio/accel/kionix,kx022a.yaml new file +9 -0

+
+  mount-matrix:
+    description: |
+      an optional 3x3 mounting rotation matrix.
+
+required:
+  - compatible
+  - reg
+  - interrupts

Documentation/devicetree/bindings/iio/accel/kionix,kx022a.yaml line 10

Quoted 5 lines · Matti Vaittinen

KX022A is a 3-axis Accelerometer from ROHM/Kionix. The senor features include variable ODRs, I2C and SPI control, FIFO/LIFO with watermark IRQ,

Documentation/devicetree/bindings/iio/accel/kionix,kx022a.yaml new file +3 -0

+
+maintainers:
+  - Matti Vaittinen <[email protected]>

Documentation/devicetree/bindings/iio/accel/kionix,kx022a.yaml line 18

Quoted 8 lines

Documentation/devicetree/bindings/iio/accel/kionix,kx022a.yaml new file +8 -0

+
+description: |
+  KX022A is a 3-axis accelerometer supporting +/- 2G, 4G, 8G and 16G ranges,
+  output data-rates from 0.78Hz to 1600Hz and a hardware-fifo buffering.
+  KX022A can be accessed either via I2C or SPI.
+
+properties:
+  compatible: kionix,kx022a

Missing const. I wonder how did it pass testing...

Quoted 1 line

+ io_vdd-supply: true

No underscores, so io-vdd-supply

Hi dee Ho Krzysztof,

Thanks for looking through this!

Quoted 6 lines · Krzysztof Kozlowski

Documentation/devicetree/bindings/iio/accel/kionix,kx022a.yaml line 10

Quoted 5 lines · Matti Vaittinen

KX022A is a 3-axis Accelerometer from ROHM/Kionix. The senor features include variable ODRs, I2C and SPI control, FIFO/LIFO with watermark IRQ,

Documentation/devicetree/bindings/iio/accel/kionix,kx022a.yaml new file +3 -0

+
+maintainers:
+  - Matti Vaittinen <[email protected]>

My own comment - switch the email to the gmail-one. Company mail is unreliable at best..

Quoted 10 lines

Documentation/devicetree/bindings/iio/accel/kionix,kx022a.yaml line 18

Quoted 8 lines

Documentation/devicetree/bindings/iio/accel/kionix,kx022a.yaml new file +8 -0

+
+description: |
+  KX022A is a 3-axis accelerometer supporting +/- 2G, 4G, 8G and 16G ranges,
+  output data-rates from 0.78Hz to 1600Hz and a hardware-fifo buffering.
+  KX022A can be accessed either via I2C or SPI.
+
+properties:
+  compatible: kionix,kx022a

Missing const. I wonder how did it pass testing...

I originally had
oneOf:
items const ...
construct here as I had separate compatibles for *-spi and *-i2c. I am unsure if I remembered to run the tests after dropping the extra compatibles :| - Sorry! I'll fix this.

Quoted 4 lines

Quoted 2 lines

+ io_vdd-supply: true

No underscores, so io-vdd-supply

The rationale behind the underscore is that the data-sheet uses terms vdd and vdd_io (with underscore). I wanted to match the supply name to what is used in the data-sheet. Not a big thing but I'd rather kept if same as the data-sheet if the requirement of "no-underscores" is not "hard". (If it is, then I'll drop the underscore).

Quoted 26 lines · Matti

Hi dee Ho Krzysztof,

Thanks for looking through this!

Quoted 6 lines · Krzysztof Kozlowski

Documentation/devicetree/bindings/iio/accel/kionix,kx022a.yaml line 10

Quoted 5 lines · Matti Vaittinen

KX022A is a 3-axis Accelerometer from ROHM/Kionix. The senor features include variable ODRs, I2C and SPI control, FIFO/LIFO with watermark IRQ,

Documentation/devicetree/bindings/iio/accel/kionix,kx022a.yaml new file +3 -0

+
+maintainers:
+  - Matti Vaittinen <[email protected]>

My own comment - switch the email to the gmail-one. Company mail is unreliable at best..

Quoted 10 lines

Documentation/devicetree/bindings/iio/accel/kionix,kx022a.yaml line 18

Quoted 8 lines

Documentation/devicetree/bindings/iio/accel/kionix,kx022a.yaml new file +8 -0

+
+description: |
+  KX022A is a 3-axis accelerometer supporting +/- 2G, 4G, 8G and 16G ranges,
+  output data-rates from 0.78Hz to 1600Hz and a hardware-fifo buffering.
+  KX022A can be accessed either via I2C or SPI.
+
+properties:
+  compatible: kionix,kx022a

Missing const. I wonder how did it pass testing...

Quoted 6 lines

I originally had
oneOf:
items const ...
construct here as I had separate compatibles for *-spi and *-i2c. I am unsure if I remembered to run the tests after dropping the extra compatibles :| - Sorry! I'll fix this.

This should be just:
compatible:
const: foo,bar

Quoted 5 lines

Quoted 4 lines

Quoted 2 lines

+ io_vdd-supply: true

No underscores, so io-vdd-supply

Quoted 5 lines

The rationale behind the underscore is that the data-sheet uses terms vdd and vdd_io (with underscore). I wanted to match the supply name to what is used in the data-sheet. Not a big thing but I'd rather kept if same as the data-sheet if the requirement of "no-underscores" is not "hard". (If it is, then I'll drop the underscore).

Underscores trigger warnings at some dtc W level (W=1 or W=2) so they are not allowed.

KX022A is a 3-axis Accelerometer from ROHM/Kionix. The senor features include variable ODRs, I2C and SPI control, FIFO/LIFO with watermark IRQ, tap/motion detection, wake-up & back-to-sleep events, four acceleration ranges (2, 4, 8 and 16g) and probably some other cool fatures.

Add support for the basic accelerometer features such as getting the acceleration data via IIO. (raw reads, triggered buffer [data-ready] or using the WMI IRQ).

Important things to be added include the double-tap, motion detection and wake-up as well as the runtime power management.

NOTE: Filling-up the hardware FIFO should be avoided. During my testing I noticed that filling up the hardware FIFO might mess-up the sample count. My sensor ended up in a state where amount of data in FIFO was reported to be 0xff bytes, which equals to 42,5 samples. Specification says the FIFO can hold maximum of 41 samples in HiRes mode. Also, at least once the FIFO was stuck in a state where reading data from hwardware FIFO did not decrease the amount of data reported to be in the FIFO - eg. FIFO was "stuck". The code has now an error count and 10 reads with invalid FIFO data count will cause the fifo contents to be dropped.

Signed-off-by Matti Vaittinen <[email protected]>

---

drivers/iio/accel/kionix-kx022a.c +45 -0

+static int kx022a_write_raw(struct iio_dev *idev,
+			    struct iio_chan_spec const *chan,
+			    int val, int val2, long mask)
+{
+	struct kx022a_data *data = iio_priv(idev);
+	int ret;
+
+	/*
+	 * We should not allow changing scale or frequency when FIFO is running
+	 * as it will mess the timestamp/scale for samples existing in the
+	 * buffer. If this turns out to be an issue we can later change logic
+	 * to internally flush the fifo before reconfiguring so the samples in
+	 * fifo keep matching the freq/scale settings. (Such setup could cause
+	 * issues if users trust the watermark to be reached within known
+	 * time-limit).
+	 */
+	mutex_lock(&data->mutex);
+	if (iio_buffer_enabled(idev)) {
+		ret = -EBUSY;
+		goto unlock_out;
+	}
+	switch (mask) {
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		ret = kx022a_find_tuple_index(&kx022a_accel_samp_freq_table[0],
+					      ARRAY_SIZE(kx022a_accel_samp_freq_table),
+					      val, val2);
+		/* Configure if we found valid ODR */
+		if (ret >= 0) {
+			int odr = ret;
+
+			ret = __kx022a_turn_off_unlocked(data);
+			if (ret)
+				goto unlock_out;
+
+			ret = regmap_update_bits(data->regmap, KX022A_REG_ODCNTL,
+						 KX022A_MASK_ODR, odr);
+			data->odr_ns = kx022a_odrs[odr];
+			__kx022a_turn_on_unlocked(data);
+		}
+		break;
+	case IIO_CHAN_INFO_SCALE:
+		ret = kx022a_find_tuple_index(&kx022a_scale_table[0],
+					      ARRAY_SIZE(kx022a_scale_table),
+					      val, val2);
+		/* Configure if we found valid scale */

KX022A is a 3-axis Accelerometer from ROHM/Kionix. The senor features include variable ODRs, I2C and SPI control, FIFO/LIFO with watermark IRQ, tap/motion detection, wake-up & back-to-sleep events, four acceleration ranges (2, 4, 8 and 16g) and probably some other cool fatures.

Add support for the basic accelerometer features such as getting the acceleration data via IIO. (raw reads, triggered buffer [data-ready] or using the WMI IRQ).

Important things to be added include the double-tap, motion detection and wake-up as well as the runtime power management.

NOTE: Filling-up the hardware FIFO should be avoided. During my testing I noticed that filling up the hardware FIFO might mess-up the sample count. My sensor ended up in a state where amount of data in FIFO was reported to be 0xff bytes, which equals to 42,5 samples. Specification says the FIFO can hold maximum of 41 samples in HiRes mode. Also, at least once the FIFO was stuck in a state where reading data from hwardware FIFO did not decrease the amount of data reported to be in the

FIFO - eg. FIFO was "stuck". The code has now an error count and 10 reads with invalid FIFO data count will cause the fifo contents to be dropped.

Signed-off-by Matti Vaittinen <[email protected]>

---
 drivers/iio/accel/Kconfig             |   23 +
 drivers/iio/accel/Makefile            |    3 +
 drivers/iio/accel/kionix-kx022a-i2c.c |   52 ++
 drivers/iio/accel/kionix-kx022a-spi.c |   50 ++
 drivers/iio/accel/kionix-kx022a.c     | 1149 +++++++++++++++++++++++++
 drivers/iio/accel/kionix-kx022a.h     |   76 ++
 6 files changed, 1353 insertions(+)
 create mode 100644 drivers/iio/accel/kionix-kx022a-i2c.c
 create mode 100644 drivers/iio/accel/kionix-kx022a-spi.c
 create mode 100644 drivers/iio/accel/kionix-kx022a.c
 create mode 100644 drivers/iio/accel/kionix-kx022a.h

drivers/iio/accel/Kconfig +2 -0

index 35798712f811..12ad6bcb2c76 100644
@@ -409,6 +409,29 @@ config IIO_ST_ACCEL_SPI_3AXIS
 	  To compile this driver as a module, choose M here. The module
 	  will be called st_accel_spi.
 
+config IIO_KX022A
+	tristate

drivers/iio/accel/Kconfig +11 -0

+
+config IIO_KX022A_I2C
+	tristate "Kionix KX022A tri-axis digital accelerometer"
+	depends on I2C
+	select IIO_KX022A
+	select REGMAP_I2C
+	help
+	  Say Y here to enable support for the Kionix KX022A digital tri-axis
+	  accelerometer connected to I2C interface. See IIO_KX022A_SPI if
+	  you want to enable support for the KX022A connected via SPI.
+
 

drivers/iio/accel/kionix-kx022a-i2c.c new file +0 -0

new file mode 100644
index 000000000000..2dbf8a33b84b

drivers/iio/accel/kionix-kx022a-i2c.c new file +7 -0

+// Copyright (C) 2022 ROHM Semiconductors
+//
+// ROHM/KIONIX KX022A accelerometer driver
+
+#include <linux/i2c.h>
+#include <linux/interrupt.h>
+#include <linux/module.h>

drivers/iio/accel/kionix-kx022a-i2c.c new file +7 -0

+		dev_err(dev, "No IRQ configured\n");
+		return -EINVAL;
+	}
+
+	regmap = devm_regmap_init_i2c(i2c, &kx022a_regmap);
+	if (IS_ERR(regmap)) {
+		dev_err(dev, "Failed to initialize Regmap\n");

drivers/iio/accel/kionix-kx022a-i2c.c new file +7 -0

+};
+
+module_i2c_driver(kx022a_i2c_driver);
+
+MODULE_DESCRIPTION("ROHM/Kionix KX022A accelerometer driver");
+MODULE_AUTHOR("Matti Vaittinen <[email protected]>");
+MODULE_LICENSE("GPL");

drivers/iio/accel/kionix-kx022a-spi.c new file +0 -0

new file mode 100644
index 000000000000..007f4b726d8f

drivers/iio/accel/kionix-kx022a-spi.c new file +45 -0

+
+#include <linux/interrupt.h>
+#include <linux/module.h>
+#include <linux/regmap.h>
+#include <linux/spi/spi.h>
+
+#include "kionix-kx022a.h"
+
+static int kx022a_spi_probe(struct spi_device *spi)
+{
+	struct device *dev = &spi->dev;
+	struct regmap *regmap;
+
+	if (!spi->irq) {
+		dev_err(dev, "No IRQ configured\n");
+		return -EINVAL;
+	}
+
+	regmap = devm_regmap_init_spi(spi, &kx022a_regmap);
+	if (IS_ERR(regmap)) {
+		dev_err(dev, "Failed to initialize Regmap\n");
+
+		return PTR_ERR(regmap);
+	}
+	return kx022a_probe_internal(dev, spi->irq);
+}
+
+static const struct of_device_id kx022a_of_match[] = {
+	{ .compatible = "kionix,kx022a", },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, kx022a_of_match);
+
+static struct spi_driver kx022a_spi_driver = {
+	.driver = {
+		.name   = "kx022a-spi",
+	},
+	.probe	= kx022a_spi_probe,
+};
+
+module_spi_driver(kx022a_spi_driver);
+
+MODULE_DESCRIPTION("ROHM/Kionix kx022A accelerometer driver");
+MODULE_AUTHOR("Matti Vaittinen <[email protected]>");
+MODULE_LICENSE("GPL");

drivers/iio/accel/kionix-kx022a.c new file +136 -0

new file mode 100644
index 000000000000..9f9e0d7efc09
@@ -0,0 +1,1149 @@
+// SPDX-License-Identifier: GPL-2.0-only
+//
+// Copyright (C) 2022 ROHM Semiconductors
+//
+// ROHM/KIONIX KX022A accelerometer driver
+
+#include <linux/delay.h>
+#include <linux/gpio.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+#include <linux/iio/trigger.h>
+#include <linux/iio/trigger_consumer.h>
+#include <linux/iio/triggered_buffer.h>
+#include <linux/interrupt.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/of.h>
+#include <linux/of_gpio.h>
+#include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
+#include <linux/slab.h>
+
+#include "kionix-kx022a.h"
+
+/*
+ * Threshold for deciding our HW fifo is unrecoverably corrupt and should be
+ * cleared
+ */
+#define KXO22A_FIFO_ERR_THRESHOLD 10
+#define KX022A_FIFO_LENGTH 41
+/* 3 axis, 2 bytes of data for each of the axis */
+#define KX022A_FIFO_SAMPLES_SIZE_BYTES 6
+#define KX022A_FIFO_MAX_BYTES (KX022A_FIFO_LENGTH *			\
+			       KX022A_FIFO_SAMPLES_SIZE_BYTES)
+
+#define MIN_ODR_INTERVAL_MS 5
+#define MAX_ODR_INTERVAL_MS 1280
+#define NUM_SUPPORTED_ODR 9
+
+enum {
+	KX022A_STATE_SAMPLE,
+	KX022A_STATE_FIFO,
+};
+
+/* Regmap configs */
+static const struct regmap_range kx022a_volatile_ranges[] = {
+	{
+		.range_min = KX022A_REG_XHP_L,
+		.range_max = KX022A_REG_COTR,
+	}, {
+		.range_min = KX022A_REG_TSCP,
+		.range_max = KX022A_REG_INT_REL,
+	}, {
+		.range_min = KX022A_REG_BUF_STATUS_1,
+		.range_max = KX022A_REG_BUF_READ,
+	},
+};
+
+static const struct regmap_access_table kx022a_volatile_regs = {
+	.yes_ranges = &kx022a_volatile_ranges[0],
+	.n_yes_ranges = ARRAY_SIZE(kx022a_volatile_ranges),
+};
+
+static const struct regmap_range kx022a_precious_ranges[] = {
+	{
+		.range_min = KX022A_REG_INT_REL,
+		.range_max = KX022A_REG_INT_REL,
+	},
+};
+
+static const struct regmap_access_table kx022a_precious_regs = {
+	.yes_ranges = &kx022a_precious_ranges[0],
+	.n_yes_ranges = ARRAY_SIZE(kx022a_precious_ranges),
+};
+
+/*
+ * The HW does not set WHO_AM_I reg as read-only but we don't want to write it
+ * so we still include it in the read-only ranges.
+ */
+static const struct regmap_range kx022a_read_only_ranges[] = {
+	{
+		.range_min = KX022A_REG_XHP_L,
+		.range_max = KX022A_REG_INT_REL,
+	}, {
+		.range_min = KX022A_REG_BUF_STATUS_1,
+		.range_max = KX022A_REG_BUF_STATUS_2,
+	}, {
+		.range_min = KX022A_REG_BUF_READ,
+		.range_max = KX022A_REG_BUF_READ,
+	},
+};
+
+static const struct regmap_access_table kx022a_ro_regs = {
+	.no_ranges = &kx022a_read_only_ranges[0],
+	.n_no_ranges = ARRAY_SIZE(kx022a_read_only_ranges),
+};
+
+static const struct regmap_range kx022a_write_only_ranges[] = {
+	{
+		.range_min = KX022A_REG_BTS_WUF_TH,
+		.range_max = KX022A_REG_BTS_WUF_TH,
+	}, {
+		.range_min = KX022A_REG_MAN_WAKE,
+		.range_max = KX022A_REG_MAN_WAKE,
+	}, {
+		.range_min = KX022A_REG_SELF_TEST,
+		.range_max = KX022A_REG_SELF_TEST,
+	}, {
+		.range_min = KX022A_REG_BUF_CLEAR,
+		.range_max = KX022A_REG_BUF_CLEAR,
+	},
+};
+
+static const struct regmap_access_table kx022a_wo_regs = {
+	.no_ranges = &kx022a_write_only_ranges[0],
+	.n_no_ranges = ARRAY_SIZE(kx022a_write_only_ranges),
+};
+
+static const struct regmap_range kx022a_noinc_read_ranges[] = {
+	{
+		.range_min = KX022A_REG_BUF_READ,
+		.range_max = KX022A_REG_BUF_READ,
+	},
+};
+
+static const struct regmap_access_table kx022a_nir_regs = {
+	.yes_ranges = &kx022a_noinc_read_ranges[0],
+	.n_yes_ranges = ARRAY_SIZE(kx022a_noinc_read_ranges),
+};
+
+const struct regmap_config kx022a_regmap = {
+	.reg_bits = 8,
+	.val_bits = 8,
+	.volatile_table = &kx022a_volatile_regs,
+	.rd_table = &kx022a_wo_regs,
+	.wr_table = &kx022a_ro_regs,

drivers/iio/accel/kionix-kx022a.c new file +58 -0

+	struct iio_trigger *indio_trig;
+	bool enabled;
+	const char *name;
+};
+
+struct kx022a_data {
+	int irq;
+	struct regmap *regmap;
+	struct kx022a_trigger trigger;
+	struct device *dev;
+	unsigned int g_range;
+	struct mutex mutex;
+	unsigned int state;
+
+	int64_t timestamp, old_timestamp;
+	unsigned int odr_ns;
+
+	struct iio_mount_matrix orientation;
+	u8 watermark;
+	/* 3 x 16bit accel data + timestamp */
+	s16 buffer[8];
+	struct {
+		__le16 channels[3];
+		s64 ts __aligned(8);
+	} scan;
+};
+
+static const struct iio_mount_matrix *
+kx022a_get_mount_matrix(const struct iio_dev *idev,
+			const struct iio_chan_spec *chan)
+{
+	struct kx022a_data *data = iio_priv(idev);
+
+	return &data->orientation;
+}
+
+enum {
+	AXIS_X,
+	AXIS_Y,
+	AXIS_Z,
+	AXIS_MAX,
+};
+
+static const unsigned long kx022a_scan_masks[] = {
+					BIT(AXIS_X) | BIT(AXIS_Y) | BIT(AXIS_Z),
+					0};
+
+static const struct iio_chan_spec_ext_info kx022a_ext_info[] = {
+	IIO_MOUNT_MATRIX(IIO_SHARED_BY_TYPE, kx022a_get_mount_matrix),
+	{ },
+};
+
+#define KX022A_ACCEL_CHAN(axis, index)						\
+	{								\
+		.type = IIO_ACCEL,					\
+		.modified = 1,						\
+		.channel2 = IIO_MOD_##axis,				\
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),		\

drivers/iio/accel/kionix-kx022a.c new file +14 -0

+			.endianness = IIO_LE,				\
+		},							\
+	}
+
+static const struct iio_chan_spec kx022a_channels[] = {
+	KX022A_ACCEL_CHAN(X, 0),
+	KX022A_ACCEL_CHAN(Y, 1),
+	KX022A_ACCEL_CHAN(Z, 2),
+	IIO_CHAN_SOFT_TIMESTAMP(3),
+};
+
+/*
+ * The sensor HW can support ODR up to 1600 Hz - which is beyond what most of
+ * Linux CPUs can handle w/o dropping samples. Also, the low power mode is not

drivers/iio/accel/kionix-kx022a.c new file +16 -0

+	NULL,
+};
+
+static const struct attribute_group kx022a_attrs_group = {
+	.attrs = kx022a_attributes,
+};
+
+struct kx022a_tuple {
+	int val;
+	int val2;
+};
+
+/*
+ * range is typically +-2G/4G/8G/16G, distributed over the amount of bits.
+ * The scale table can be calculated using
+ *	(range / 2^bits) * g = (range / 2^bits) * 9.80665 m/s^2

drivers/iio/accel/kionix-kx022a.c new file +43 -0

+
+/* Find index of tuple matching the given values */
+static int kx022a_find_tuple_index(const struct kx022a_tuple *tuples, int n,
+				   int val, int val2)
+{
+	while (n-- > 0)
+		if (val == tuples[n].val && tuples[n].val2 == val2)
+			return n;
+
+	return -EINVAL;
+}
+
+static void kx022a_reg2freq(unsigned int val,  int *val1, int *val2)
+{
+	*val1 = kx022a_accel_samp_freq_table[val & KX022A_MASK_ODR].val;
+	*val2 = kx022a_accel_samp_freq_table[val & KX022A_MASK_ODR].val2;
+}
+
+static void kx022a_reg2scale(unsigned int val, unsigned int *val1,
+			     unsigned int *val2)
+{
+	val &= KX022A_MASK_GSEL;
+	val >>= KX022A_GSEL_SHIFT;
+
+	*val1 = kx022a_scale_table[val].val;
+	*val2 = kx022a_scale_table[val].val2;
+}
+
+static int __kx022a_turn_on_unlocked(struct kx022a_data *data)
+{
+	int ret;
+
+	ret = regmap_set_bits(data->regmap, KX022A_REG_CNTL, KX022A_MASK_PC1);
+	if (ret)
+		dev_err(data->dev, "Turn ON fail %d\n", ret);
+
+	return ret;
+}
+
+static int __kx022a_turn_off_unlocked(struct kx022a_data *data)
+{
+	int ret;
+

drivers/iio/accel/kionix-kx022a.c new file +14 -0

+	ret = __kx022a_turn_on_unlocked(data);
+	mutex_unlock(&data->mutex);
+
+	return ret;
+}
+
+static int kx022a_write_raw(struct iio_dev *idev,
+			    struct iio_chan_spec const *chan,
+			    int val, int val2, long mask)
+{
+	struct kx022a_data *data = iio_priv(idev);
+	int ret;
+
+	/*

drivers/iio/accel/kionix-kx022a.c new file +50 -0

+		ret = -EBUSY;
+		goto unlock_out;
+	}
+	switch (mask) {
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		ret = kx022a_find_tuple_index(&kx022a_accel_samp_freq_table[0],
+					      ARRAY_SIZE(kx022a_accel_samp_freq_table),
+					      val, val2);
+		/* Configure if we found valid ODR */
+		if (ret >= 0) {
+			int odr = ret;
+
+			ret = __kx022a_turn_off_unlocked(data);
+			if (ret)
+				goto unlock_out;
+
+			ret = regmap_update_bits(data->regmap, KX022A_REG_ODCNTL,
+						 KX022A_MASK_ODR, odr);
+			data->odr_ns = kx022a_odrs[odr];
+			__kx022a_turn_on_unlocked(data);
+		}
+		break;
+	case IIO_CHAN_INFO_SCALE:
+		ret = kx022a_find_tuple_index(&kx022a_scale_table[0],
+					      ARRAY_SIZE(kx022a_scale_table),
+					      val, val2);
+		/* Configure if we found valid scale */
+		if (ret >= 0) {
+			ret = __kx022a_turn_off_unlocked(data);
+			if (ret)
+				goto unlock_out;
+
+			ret = regmap_update_bits(data->regmap, KX022A_REG_CNTL,
+						 KX022A_MASK_GSEL,
+						 ret << KX022A_GSEL_SHIFT);
+			__kx022a_turn_on_unlocked(data);
+		}
+	default:
+		ret = -EINVAL;
+	}
+
+unlock_out:
+	mutex_unlock(&data->mutex);
+
+	return ret;
+}
+
+static int kx022a_fifo_set_wmi(struct kx022a_data *data)
+{
+	u8 threshold;

drivers/iio/accel/kionix-kx022a.c new file +10 -0

+	int ret, fifo_bytes;
+
+	fifo_bytes = samples * KX022A_FIFO_SAMPLES_SIZE_BYTES;
+	ret = regmap_noinc_read(data->regmap, KX022A_REG_BUF_READ,
+			       buffer, fifo_bytes);
+	if (ret)
+		dev_err(data->dev, "FIFO read failed %d\n", ret);
+
+	return ret;
+}

drivers/iio/accel/kionix-kx022a.c new file +9 -0

+	if (ret)
+		return ret;
+	*val = raw_val;
+
+	return IIO_VAL_INT;
+}
+
+static int kx022a_read_raw(struct iio_dev *idev,
+			   struct iio_chan_spec const *chan,

drivers/iio/accel/kionix-kx022a.c new file +119 -0

+		ret = regmap_read(data->regmap, KX022A_REG_ODCNTL, &regval);
+		if (ret)
+			goto error_ret;
+
+		if ((regval & KX022A_MASK_ODR) >
+		    ARRAY_SIZE(kx022a_accel_samp_freq_table)) {
+			dev_err(data->dev, "Invalid ODR\n");
+			ret = -EINVAL;
+			goto error_ret;
+		}
+
+		kx022a_reg2freq(regval, val, val2);
+		ret = IIO_VAL_INT_PLUS_MICRO;
+
+		break;
+	}
+	case IIO_CHAN_INFO_SCALE:
+		ret = regmap_read(data->regmap, KX022A_REG_CNTL, &regval);
+		if (ret < 0)
+			goto error_ret;
+
+		kx022a_reg2scale(regval, val, val2);
+
+		ret = IIO_VAL_INT_PLUS_MICRO;
+		break;
+	}
+
+error_ret:
+	mutex_unlock(&data->mutex);
+
+	return ret;
+};
+
+static int kx022a_validate_trigger(struct iio_dev *idev,
+				   struct iio_trigger *trig)
+{
+	struct kx022a_data *data = iio_priv(idev);
+
+	if (data->trigger.indio_trig == trig)
+		return 0;
+
+	return -EINVAL;
+}
+
+static int kx022a_set_watermark(struct iio_dev *idev, unsigned int val)
+{
+	struct kx022a_data *data = iio_priv(idev);
+
+	if (val > KX022A_FIFO_LENGTH)
+		val = KX022A_FIFO_LENGTH;
+
+	mutex_lock(&data->mutex);
+	data->watermark = val;
+	mutex_unlock(&data->mutex);
+
+	return 0;
+}
+
+static ssize_t kx022a_get_fifo_state(struct device *dev,
+				     struct device_attribute *attr,
+				     char *buf)
+{
+	struct iio_dev *idev = dev_to_iio_dev(dev);
+	struct kx022a_data *data = iio_priv(idev);
+	bool state;
+
+	mutex_lock(&data->mutex);
+	state = data->state;
+	mutex_unlock(&data->mutex);
+
+	return sprintf(buf, "%d\n", state);
+}
+
+static ssize_t kx022a_get_fifo_watermark(struct device *dev,
+					 struct device_attribute *attr,
+					 char *buf)
+{
+	struct iio_dev *idev = dev_to_iio_dev(dev);
+	struct kx022a_data *data = iio_priv(idev);
+	int wm;
+
+	mutex_lock(&data->mutex);
+	wm = data->watermark;
+	mutex_unlock(&data->mutex);
+
+	return sprintf(buf, "%d\n", wm);
+}
+
+static IIO_DEVICE_ATTR(hwfifo_enabled, 0444,
+		       kx022a_get_fifo_state, NULL, 0);
+static IIO_DEVICE_ATTR(hwfifo_watermark, 0444,
+		       kx022a_get_fifo_watermark, NULL, 0);
+
+static const struct attribute *kx022a_fifo_attributes[] = {
+	&iio_dev_attr_hwfifo_watermark.dev_attr.attr,
+	&iio_dev_attr_hwfifo_enabled.dev_attr.attr,
+	NULL,
+};
+
+static int kx022a_drop_fifo_contents(struct kx022a_data *data)
+{
+	/*
+	 * We must clear the old time-stamp to avoid computing the timestamps
+	 * based on samples acquired when buffer was last enabled
+	 */
+	data->timestamp = 0;
+
+	return regmap_write(data->regmap, KX022A_REG_BUF_CLEAR, 0xff);
+}
+
+static int __kx022a_fifo_flush(struct iio_dev *idev, unsigned int samples,
+			       bool irq)
+{
+	struct kx022a_data *data = iio_priv(idev);
+	struct device *dev = regmap_get_device(data->regmap);
+	int ret, i;
+	int count, fifo_bytes;
+	u16 buffer[KX022A_FIFO_LENGTH * 3];
+	int64_t tstamp;

drivers/iio/accel/kionix-kx022a.c new file +121 -0

+	if (fifo_bytes > KX022A_FIFO_MAX_BYTES) {
+		/*
+		 * I've observed a strange behaviour where FIFO may get stuck if
+		 * samples are not read out fast enough. By 'stuck' I mean
+		 * situation where amount of data adverticed by the STATUS_1
+		 * reg is 255 - which equals to 42,5 (sic!) samples and by
+		 * my experimenting there are situations where reading the
+		 * FIFO buffer does not decrease the data count but the same
+		 * fifo sample level (255 bytes of data) is reported
+		 */
+		err_ctr++;
+		dev_warn(data->dev, "Bad amount of data %u\n", fifo_bytes);
+		fifo_bytes = KX022A_FIFO_MAX_BYTES;
+	} else if (fifo_bytes % KX022A_FIFO_SAMPLES_SIZE_BYTES) {
+		err_ctr++;
+		dev_err(data->dev, "Bad FIFO alignment. Data may be corrupt\n");
+	} else {
+		err_ctr = 0;
+	}
+
+	if (err_ctr > KXO22A_FIFO_ERR_THRESHOLD) {
+		__kx022a_turn_off_unlocked(data);
+		kx022a_drop_fifo_contents(data);
+		__kx022a_turn_on_unlocked(data);
+
+		err_ctr = 0;
+
+		return -EINVAL;
+	}
+
+	count = fifo_bytes / KX022A_FIFO_SAMPLES_SIZE_BYTES;
+	if (!count)
+		return 0;
+
+	/*
+	 * If we are being called from IRQ handler we know the stored timestamp
+	 * is fairly accurate for the last stored sample. Otherwise, if we are
+	 * called as a result of a read operation from userspace and hence
+	 * before the watermark interrupt was triggered, take a timestamp
+	 * now. We can fall anywhere in between two samples so the error in this
+	 * case is at most one sample period.
+	 */
+	if (!irq) {
+		data->old_timestamp = data->timestamp;
+		data->timestamp = iio_get_time_ns(idev);
+	}
+
+	/*
+	 * Approximate timestamps for each of the sample based on the sampling
+	 * frequency, timestamp for last sample and number of samples.
+	 *
+	 * We'd better not use the current bandwidth settings to compute the
+	 * sample period. The real sample rate varies with the device and
+	 * small variation adds when we store a large number of samples.
+	 *
+	 * To avoid this issue we compute the actual sample period ourselves
+	 * based on the timestamp delta between the last two flush operations.
+	 */
+	if (data->old_timestamp) {
+		sample_period = (data->timestamp - data->old_timestamp);
+		do_div(sample_period, count);
+	} else {
+		sample_period = data->odr_ns;
+	}
+	tstamp = data->timestamp - (count - 1) * sample_period;
+
+	if (samples && count > samples) {
+		/*
+		 * Here we leave some old samples to the buffer. We need to
+		 * adjust the timestamp to match the first sample in the buffer
+		 * or we will miscalculate the sample_period at next round.
+		 */
+		data->timestamp -= (count - samples) * sample_period;
+		count = samples;
+	}
+
+	ret = kx022a_fifo_report_data(data, buffer, count);
+	if (ret)
+		return ret;
+
+	for (i = 0; i < count; i++) {
+		int bit;
+		u16 *samples = &buffer[i * 3];
+
+		for_each_set_bit(bit, idev->active_scan_mask, AXIS_MAX)
+			memcpy(&data->scan.channels[bit], &samples[bit],
+			       sizeof(data->scan.channels[0]));
+
+		iio_push_to_buffers_with_timestamp(idev, &data->scan, tstamp);
+
+		tstamp += sample_period;
+	}
+
+	return count;
+}
+
+static int kx022a_fifo_flush(struct iio_dev *idev, unsigned int samples)
+{
+	struct kx022a_data *data = iio_priv(idev);
+	int ret;
+
+	mutex_lock(&data->mutex);
+	ret = __kx022a_fifo_flush(idev, samples, false);
+	mutex_unlock(&data->mutex);
+
+	return ret;
+}
+
+static const struct iio_info kx022a_info = {
+	.read_raw = &kx022a_read_raw,
+	.write_raw = &kx022a_write_raw,
+	.attrs = &kx022a_attrs_group,
+
+	.validate_trigger	= kx022a_validate_trigger,
+	.hwfifo_set_watermark	= kx022a_set_watermark,
+	.hwfifo_flush_to_buffer	= kx022a_fifo_flush,
+};
+
+static int kx022a_set_drdy_irq(struct kx022a_data *data, bool en)
+{
+	if (en)

drivers/iio/accel/kionix-kx022a.c new file +10 -0

+	int mask = KX022A_MASK_IEN1 | KX022A_MASK_IPOL1 |
+		   KX022A_MASK_ITYP;
+	int val = KX022A_MASK_IEN1 | KX022A_IPOL_LOW |
+		  KX022A_ITYP_LEVEL;
+	int ret;
+
+	ret = regmap_update_bits(data->regmap, KX022A_REG_INC1, mask, val);
+	if (ret)
+		return ret;
+

drivers/iio/accel/kionix-kx022a.c new file +36 -0

+	ret = kx022a_turn_off_lock(data);
+	if (ret)
+		return ret;
+
+	ret = regmap_clear_bits(data->regmap, KX022A_REG_INC4,
+				KX022A_MASK_WMI1);
+	if (ret)
+		goto unlock_out;
+
+	/* disable buffer */
+	ret = regmap_clear_bits(data->regmap, KX022A_REG_BUF_CNTL2,
+				KX022A_MASK_BUF_EN);
+	if (ret)
+		goto unlock_out;
+
+	data->state &= (~KX022A_STATE_FIFO);
+
+	kx022a_drop_fifo_contents(data);
+
+	return kx022a_turn_on_unlock(data);
+
+unlock_out:
+	mutex_unlock(&data->mutex);
+
+	return ret;
+}
+
+static int kx022a_buffer_predisable(struct iio_dev *idev)
+{
+	struct kx022a_data *data = iio_priv(idev);
+
+	if (iio_device_get_current_mode(idev) == INDIO_BUFFER_TRIGGERED)
+		return 0;
+
+	return kx022a_fifo_disable(data);
+}

drivers/iio/accel/kionix-kx022a.c new file +22 -0

+	ret = kx022a_fifo_set_wmi(data);
+	if (ret)
+		goto unlock_out;
+
+	/* Enable buffer */
+	ret = regmap_set_bits(data->regmap, KX022A_REG_BUF_CNTL2,
+			      KX022A_MASK_BUF_EN);
+	if (ret)
+		goto unlock_out;
+
+	data->state |= KX022A_STATE_FIFO;
+	ret = regmap_set_bits(data->regmap, KX022A_REG_INC4,
+			      KX022A_MASK_WMI1);
+	if (ret)
+		goto unlock_out;
+
+	return kx022a_turn_on_unlock(data);
+
+unlock_out:
+	mutex_unlock(&data->mutex);
+
+	return ret;

drivers/iio/accel/kionix-kx022a.c new file +46 -0

+	 */
+	if (iio_device_get_current_mode(idev) == INDIO_BUFFER_TRIGGERED)
+		return 0;
+
+	return kx022a_fifo_enable(data);
+}
+
+static const struct iio_buffer_setup_ops kx022a_buffer_ops = {
+	.postenable = kx022a_buffer_postenable,
+	.predisable = kx022a_buffer_predisable,
+};
+
+static irqreturn_t kx022a_trigger_handler(int irq, void *p)
+{
+	struct iio_poll_func *pf = p;
+	struct iio_dev *idev = pf->indio_dev;
+	struct kx022a_data *data = iio_priv(idev);
+	int ret;
+
+	mutex_lock(&data->mutex);
+	ret = regmap_bulk_read(data->regmap, KX022A_REG_XOUT_L, data->buffer,
+			       KX022A_FIFO_SAMPLES_SIZE_BYTES);
+	if (ret < 0)
+		goto err_read;
+
+	iio_push_to_buffers_with_timestamp(idev, data->buffer, pf->timestamp);
+err_read:
+	mutex_unlock(&data->mutex);
+	iio_trigger_notify_done(idev->trig);
+
+	return IRQ_HANDLED;
+}
+
+/* Get timestamps and wake the thread if we need to read data */
+static irqreturn_t kx022a_irq_handler(int irq, void *private)
+{
+	struct iio_dev *idev = private;
+	struct kx022a_data *data = iio_priv(idev);
+	bool ack = false;
+
+	data->old_timestamp = data->timestamp;
+	data->timestamp = iio_get_time_ns(idev);
+
+	if (data->trigger.enabled) {
+		iio_trigger_poll(data->trigger.indio_trig);
+		ack = true;

drivers/iio/accel/kionix-kx022a.c new file +21 -0

+		 *
+		 * It would be possible to set the IRQ to 50uS pulse. If we are
+		 * losing data due to the disabled IRQ we can evaluate the
+		 * option of using edge triggered IRQs with the pulse mode.
+		 */
+		disable_irq_nosync(irq);
+		return IRQ_HANDLED;
+	}
+
+	return IRQ_NONE;
+}
+
+/* Read the data from the fifo and fill IIO buffers */
+static irqreturn_t kx022a_irq_thread_handler(int irq, void *private)
+{
+	struct iio_dev *idev = private;
+	struct kx022a_data *data = iio_priv(idev);
+	bool ack = false;
+	int ret;
+
+	mutex_lock(&data->mutex);

drivers/iio/accel/kionix-kx022a.c new file +29 -0

+	 */
+	if (ack)
+		ret = IRQ_HANDLED;
+	else
+		ret = IRQ_NONE;
+
+	mutex_unlock(&data->mutex);
+
+	return ret;
+}
+
+static int kx022a_trigger_set_state(struct iio_trigger *trig,
+				    bool state)
+{
+	struct kx022a_trigger *t = iio_trigger_get_drvdata(trig);
+	struct kx022a_data *data = t->data;
+	int ret = 0;
+
+	mutex_lock(&data->mutex);
+
+	if (t->enabled == state)
+		goto unlock_out;
+
+	ret = __kx022a_turn_off_unlocked(data);
+	if (ret)
+		goto unlock_out;
+
+	t->enabled = state;
+	ret = kx022a_set_drdy_irq(data, state);

drivers/iio/accel/kionix-kx022a.c new file +11 -0

+	struct kx022a_data *data = t->data;
+
+	enable_irq(data->irq);
+}
+
+static const struct iio_trigger_ops kx022a_trigger_ops = {
+	.set_trigger_state = kx022a_trigger_set_state,
+	.reenable = kx022a_trig_reen,
+};
+
+static int kx022a_chip_init(struct kx022a_data *data)

drivers/iio/accel/kionix-kx022a.c new file +10 -0

+	if (ret) {
+		dev_err(data->dev, "Failed to init IRQ states\n");
+		return ret;
+	}
+
+	ret = kx022a_set_drdy_irq(data, false);
+	if (ret) {
+		dev_err(data->dev, "Failed to init DRDY\n");
+		return ret;
+	}

drivers/iio/accel/kionix-kx022a.c new file +14 -0

+	ret = regmap_clear_bits(data->regmap, KX022A_REG_BUF_CNTL2,
+			      KX022A_MASK_BUF_EN);
+	if (ret)
+		return ret;
+
+	return kx022a_drop_fifo_contents(data);
+}
+
+int kx022a_probe_internal(struct device *dev, int irq)
+{
+	static const char * const regulator_names[] = {"io_vdd", "vdd"};
+	struct iio_trigger *indio_trig;
+	struct kx022a_data *data;
+	struct regmap *regmap;

drivers/iio/accel/kionix-kx022a.c new file +54 -0

+
+		return -EINVAL;
+	}
+
+	idev = devm_iio_device_alloc(dev, sizeof(*data));
+	if (!idev)
+		return -ENOMEM;
+
+	data = iio_priv(idev);
+
+	/*
+	 * VDD is the analog and digital domain voltage supply
+	 * IO_VDD is the digital I/O voltage supply
+	 */
+	ret = devm_regulator_bulk_get_enable(dev, ARRAY_SIZE(regulator_names),
+					     regulator_names);
+	if (ret && ret != -ENODEV)
+		return dev_err_probe(dev, ret, "failed to enable regulator\n");
+
+	ret = regmap_read(regmap, KX022A_REG_WHO, &chip_id);
+	if (ret) {
+		dev_err(dev, "Failed to access sensor\n");
+		return ret;
+	}
+
+	if (chip_id != KX022A_ID) {
+		dev_err(dev, "unsupported device 0x%x\n", chip_id);
+		return -EINVAL;
+	}
+
+	data->regmap = regmap;
+	data->dev = dev;
+	data->irq = irq;
+	data->odr_ns = KX922A_DEFAULT_PERIOD;
+	mutex_init(&data->mutex);
+
+	idev->channels = kx022a_channels;
+	idev->num_channels = ARRAY_SIZE(kx022a_channels);
+	idev->name = "kx022-accel";
+	idev->info = &kx022a_info;
+	idev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_SOFTWARE;
+	idev->available_scan_masks = kx022a_scan_masks;
+
+	/* Read the mounting matrix, if present */
+	ret = iio_read_mount_matrix(dev, &data->orientation);
+	if (ret)
+		return ret;
+
+	/* The sensor must be turned off for configuration */
+	ret = kx022a_turn_off_lock(data);
+	if (ret)
+		return ret;
+
+	ret = kx022a_chip_init(data);

drivers/iio/accel/kionix-kx022a.c new file +13 -0

+					     &iio_pollfunc_store_time,
+					     kx022a_trigger_handler,
+					     IIO_BUFFER_DIRECTION_IN,
+					     &kx022a_buffer_ops,
+					     kx022a_fifo_attributes);
+
+	if (ret)
+		return dev_err_probe(data->dev, ret,
+				     "iio_triggered_buffer_setup_ext FAIL %d\n",
+				     ret);
+
+	indio_trig = devm_iio_trigger_alloc(dev, "%sdata-rdy-dev%d", idev->name,
+					    iio_device_id(idev));

drivers/iio/accel/kionix-kx022a.c new file +22 -0

+	if (ret)
+		return dev_err_probe(data->dev, ret,
+				     "Trigger registration failed\n");
+
+	ret = devm_iio_device_register(data->dev, idev);
+	if (ret < 0)
+		return dev_err_probe(dev, ret,
+				     "Unable to register iio device\n");
+
+	ret = devm_request_threaded_irq(data->dev, irq, kx022a_irq_handler,
+					&kx022a_irq_thread_handler,
+					IRQF_ONESHOT, "kx022", idev);
+	if (ret)
+		return dev_err_probe(data->dev, ret, "Could not request IRQ\n");
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(kx022a_probe_internal);
+
+MODULE_DESCRIPTION("ROHM/Kionix KX022A accelerometer driver");
+MODULE_AUTHOR("Matti Vaittinen <[email protected]>");
+MODULE_LICENSE("GPL");

drivers/iio/accel/kionix-kx022a.h new file +0 -0

new file mode 100644
index 000000000000..ec89e46c6ca8
@@ -0,0 +1,76 @@

drivers/iio/accel/kionix-kx022a.h new file +4 -0

+int kx022a_probe_internal(struct device *dev, int irq);
+extern const struct regmap_config kx022a_regmap;
+
+#endif

Quoted 12 lines · On Wed, 21 Sep 2022 14:45:35 +0300 Matti Vaittinen <[email protected]>…

KX022A is a 3-axis Accelerometer from ROHM/Kionix. The senor features include variable ODRs, I2C and SPI control, FIFO/LIFO with watermark IRQ, tap/motion detection, wake-up & back-to-sleep events, four acceleration ranges (2, 4, 8 and 16g) and probably some other cool fatures.

Add support for the basic accelerometer features such as getting the acceleration data via IIO. (raw reads, triggered buffer [data-ready] or using the WMI IRQ).

Important things to be added include the double-tap, motion detection and wake-up as well as the runtime power management.

Quoted 7 lines

NOTE: Filling-up the hardware FIFO should be avoided. During my testing I noticed that filling up the hardware FIFO might mess-up the sample count. My sensor ended up in a state where amount of data in FIFO was reported to be 0xff bytes, which equals to 42,5 samples. Specification says the FIFO can hold maximum of 41 samples in HiRes mode. Also, at least once the FIFO was stuck in a state where reading data from hwardware FIFO did not decrease the amount of data reported to be in the

spell check this.

Quoted 3 lines

FIFO - eg. FIFO was "stuck". The code has now an error count and 10 reads with invalid FIFO data count will cause the fifo contents to be dropped.

Ouch - that's nasty.

Quoted 2 lines

Signed-off-by Matti Vaittinen <[email protected]>

Hi Matti,

A somewhat superficial review as I'm short on time, but wanted to get some initial comments out to you as I'd started reviewing this yesterday and not sure when I'd get back to it.

drivers/iio/accel/Kconfig config IIO_ST_ACCEL_SPI_3AXIS · line 423

Quoted 9 lines

drivers/iio/accel/Kconfig +9 -0

+config IIO_KX022A_SPI
+	tristate "Kionix KX022A tri-axis digital accelerometer"
+	depends on I2C
+	select IIO_KX022A
+	select REGMAP_SPI
+	help
+	  Say Y here to enable support for the Kionix KX022A digital tri-axis
+	  accelerometer connected to I2C interface. See IIO_KX022A_I2C if
+	  you want to enable support for the KX022A connected via I2C.

I would not bother with the cross reference to the I2C config element.

Review

Quoted 2 lines

(quoted diff) +2 -0

+// SPDX-License-Identifier: GPL-2.0-only
+//

Other than where required for SPDX use the
/*
* Copyright...
*/
syntax.

Quoted 5 lines

drivers/iio/accel/kionix-kx022a-i2c.c new file +4 -0

+// Copyright (C) 2022 ROHM Semiconductors
+//
+// ROHM/KIONIX KX022A accelerometer driver
+

drivers/iio/accel/kionix-kx022a-i2c.c line 19

Quoted 6 lines

drivers/iio/accel/kionix-kx022a-i2c.c new file +6 -0

+static int kx022a_i2c_probe(struct i2c_client *i2c)
+{
+	struct regmap *regmap;
+	struct device *dev = &i2c->dev;
+
+	if (!i2c->irq) {

There look to be 2 interrupt lines, so you need to know which one this is. fwnode_irq_get_byname() trying first int1 then int2 (as only a single one might be mapped).

drivers/iio/accel/kionix-kx022a-i2c.c line 45

Quoted 6 lines

drivers/iio/accel/kionix-kx022a-i2c.c new file +6 -0

+static struct i2c_driver kx022a_i2c_driver = {
+	.driver = {
+			.name  = "kx022a-i2c",
+			.of_match_table = kx022a_of_match,
+		  },
+	.probe_new    = kx022a_i2c_probe,

I'd avoid tab alignment like this. It breaks far to often and doesn't really help readability.

drivers/iio/accel/kionix-kx022a.c line 146

Quoted 4 lines

drivers/iio/accel/kionix-kx022a.c new file +4 -0

+
+struct kx022a_data;
+
+struct kx022a_trigger {

Unusual structure. IIRC we normally just squash this stuff into the iio_priv structure and set the trigger devdata to the iio_dev. I haven't really thought about the advantages of this way around but can see a disadvantage is you get circular pointer loops this way.

Review

Quoted 1 line

(quoted diff) +1 -0

+	struct kx022a_data *data;

Looks like you can get to the data pointer via a container_of() so do that instead of carrying a pointer to the containing structure.

drivers/iio/accel/kionix-kx022a.c line 239

Quoted 7 lines

drivers/iio/accel/kionix-kx022a.c new file +7 -0

+static IIO_CONST_ATTR_SAMP_FREQ_AVAIL("0.78 1.563 3.125 6.25 12.5 25 50 100 200");
+static IIO_CONST_ATTR(scale_available,
+		      "598.550415 1197.10083 2394.20166 4788.40332");
+
+static struct attribute *kx022a_attributes[] = {
+	&iio_const_attr_sampling_frequency_available.dev_attr.attr,
+	&iio_const_attr_scale_available.dev_attr.attr,

Use the read_avail() callback instead of doing these as attributes. That makes the values available to consumer drivers...

drivers/iio/accel/kionix-kx022a.c line 279

Quoted 2 lines

drivers/iio/accel/kionix-kx022a.c new file +2 -0

+static const unsigned int kx022a_odrs[] = { 1282051282, 639795266, 320000000,
+	 160000000, 80000000, 40000000, 20000000, 10000000, 5000000 };

where we have lots of zeros useful to use the MEGA etc defines in units.h

drivers/iio/accel/kionix-kx022a.c line 332

Quoted 4 lines

drivers/iio/accel/kionix-kx022a.c new file +4 -0

+
+static int kx022a_turn_off_lock(struct kx022a_data *data)
+{
+	int ret;

As below, I'm not convinced this wrapper is worthwhile

drivers/iio/accel/kionix-kx022a.c line 345

Quoted 13 lines

drivers/iio/accel/kionix-kx022a.c new file +13 -0

+
+	mutex_lock(&data->mutex);
+	ret = __kx022a_turn_off_unlocked(data);
+	if (ret)
+		mutex_unlock(&data->mutex);
+
+	return ret;
+}
+
+static int kx022a_turn_on_unlock(struct kx022a_data *data)
+{
+	int ret;
+

This is not used enough that I can see a strong reason for the wrapper. Just put the two calls inline and rename the unlocked case.

Quoted 13 lines

drivers/iio/accel/kionix-kx022a.c new file +13 -0

+	ret = __kx022a_turn_on_unlocked(data);
+	mutex_unlock(&data->mutex);
+
+	return ret;
+}
+
+	/*
+	 * We should not allow changing scale or frequency when FIFO is running
+	 * as it will mess the timestamp/scale for samples existing in the
+	 * buffer. If this turns out to be an issue we can later change logic
+	 * to internally flush the fifo before reconfiguring so the samples in
+	 * fifo keep matching the freq/scale settings. (Such setup could cause
+	 * issues if users trust the watermark to be reached within known

drivers/iio/accel/kionix-kx022a.c line 369

Quoted 4 lines

drivers/iio/accel/kionix-kx022a.c new file +4 -0

+	 * time-limit).
+	 */
+	mutex_lock(&data->mutex);
+	if (iio_buffer_enabled(idev)) {

As below - do this with iio_device_claim_direct_mode() before taking the mutex. It's the standard way to avoid the races and prevent access when buffers are enabled (if they are - you can't claim direct mode).

drivers/iio/accel/kionix-kx022a.c line 429

Quoted 4 lines

drivers/iio/accel/kionix-kx022a.c new file +4 -0

+
+static int kx022a_fifo_report_data(struct kx022a_data *data, void *buffer,
+				   int samples)
+{

I would just put this code inline at the single call site.

Quoted 20 lines

drivers/iio/accel/kionix-kx022a.c new file +19 -0

+	int ret, fifo_bytes;
+
+	fifo_bytes = samples * KX022A_FIFO_SAMPLES_SIZE_BYTES;
+	ret = regmap_noinc_read(data->regmap, KX022A_REG_BUF_READ,
+			       buffer, fifo_bytes);
+	if (ret)
+		dev_err(data->dev, "FIFO read failed %d\n", ret);
+
+	return ret;
+}
+
+static int kx022a_get_axis(struct kx022a_data *data,
+			   struct iio_chan_spec const *chan,
+			   int *val)
+{
+	u16 raw_val;
+	int ret;
+
+	ret = regmap_bulk_read(data->regmap, chan->address, &raw_val,

drivers/iio/accel/kionix-kx022a.c line 449

Quoted 1 line

drivers/iio/accel/kionix-kx022a.c new file +1 -0

+			       sizeof(raw_val));

Bulk reads for SPI still require dma safe buffers. Long story but The short version is today regmap happens to always use bounce buffers for SPI (or it did last time I looked at it). However there is no guarantee on that in future. As such you need to use a buffer that is __aligned(IIO_DMA_MINALIGN);
Easiest option is to put one at the end of the iio_priv() structure.

Quoted 15 lines

drivers/iio/accel/kionix-kx022a.c new file +15 -0

+	if (ret)
+		return ret;
+	*val = raw_val;
+
+	return IIO_VAL_INT;
+}
+
+static int kx022a_read_raw(struct iio_dev *idev,
+			   struct iio_chan_spec const *chan,
+			   int *val, int *val2, long mask)
+{
+	struct kx022a_data *data = iio_priv(idev);
+	unsigned int regval;
+	int ret = -EINVAL;
+

drivers/iio/accel/kionix-kx022a.c line 468

Quoted 4 lines

drivers/iio/accel/kionix-kx022a.c new file +4 -0

+	mutex_lock(&data->mutex);
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		if (iio_buffer_enabled(idev)) {

Claim direct mode as standard way to avoid racing in read_raw. Note you should be very careful on locking order as well. Claim direct mode first then take any other locks.

Quoted 65 lines

drivers/iio/accel/kionix-kx022a.c new file +65 -0

+	if (fifo_bytes > KX022A_FIFO_MAX_BYTES) {
+		/*
+		 * I've observed a strange behaviour where FIFO may get stuck if
+		 * samples are not read out fast enough. By 'stuck' I mean
+		 * situation where amount of data adverticed by the STATUS_1
+		 * reg is 255 - which equals to 42,5 (sic!) samples and by
+		 * my experimenting there are situations where reading the
+		 * FIFO buffer does not decrease the data count but the same
+		 * fifo sample level (255 bytes of data) is reported
+		 */
+		err_ctr++;
+		dev_warn(data->dev, "Bad amount of data %u\n", fifo_bytes);
+		fifo_bytes = KX022A_FIFO_MAX_BYTES;
+	} else if (fifo_bytes % KX022A_FIFO_SAMPLES_SIZE_BYTES) {
+		err_ctr++;
+		dev_err(data->dev, "Bad FIFO alignment. Data may be corrupt\n");
+	} else {
+		err_ctr = 0;
+	}
+
+	if (err_ctr > KXO22A_FIFO_ERR_THRESHOLD) {
+		__kx022a_turn_off_unlocked(data);
+		kx022a_drop_fifo_contents(data);
+		__kx022a_turn_on_unlocked(data);
+
+		err_ctr = 0;
+
+		return -EINVAL;
+	}
+
+	count = fifo_bytes / KX022A_FIFO_SAMPLES_SIZE_BYTES;
+	if (!count)
+		return 0;
+
+	/*
+	 * If we are being called from IRQ handler we know the stored timestamp
+	 * is fairly accurate for the last stored sample. Otherwise, if we are
+	 * called as a result of a read operation from userspace and hence
+	 * before the watermark interrupt was triggered, take a timestamp
+	 * now. We can fall anywhere in between two samples so the error in this
+	 * case is at most one sample period.
+	 */
+	if (!irq) {
+		data->old_timestamp = data->timestamp;
+		data->timestamp = iio_get_time_ns(idev);
+	}
+
+	/*
+	 * Approximate timestamps for each of the sample based on the sampling
+	 * frequency, timestamp for last sample and number of samples.
+	 *
+	 * We'd better not use the current bandwidth settings to compute the
+	 * sample period. The real sample rate varies with the device and
+	 * small variation adds when we store a large number of samples.
+	 *
+	 * To avoid this issue we compute the actual sample period ourselves
+	 * based on the timestamp delta between the last two flush operations.
+	 */
+	if (data->old_timestamp) {
+		sample_period = (data->timestamp - data->old_timestamp);
+		do_div(sample_period, count);
+	} else {
+		sample_period = data->odr_ns;
+	}
+	tstamp = data->timestamp - (count - 1) * sample_period;

drivers/iio/accel/kionix-kx022a.c line 679

Quoted 8 lines

drivers/iio/accel/kionix-kx022a.c new file +8 -0

+
+	if (samples && count > samples) {
+		/*
+		 * Here we leave some old samples to the buffer. We need to
+		 * adjust the timestamp to match the first sample in the buffer
+		 * or we will miscalculate the sample_period at next round.
+		 */ >> +		data->timestamp -= (count - samples) * sample_period;
+		count = samples;

Quoted 20 lines

drivers/iio/accel/kionix-kx022a.c new file +20 -0

+	}
+
+	ret = kx022a_fifo_report_data(data, buffer, count);
+	if (ret)
+		return ret;
+
+	for (i = 0; i < count; i++) {
+		int bit;
+		u16 *samples = &buffer[i * 3];
+
+		for_each_set_bit(bit, idev->active_scan_mask, AXIS_MAX)
+			memcpy(&data->scan.channels[bit], &samples[bit],
+			       sizeof(data->scan.channels[0]));
+
+		iio_push_to_buffers_with_timestamp(idev, &data->scan, tstamp);
+
+		tstamp += sample_period;
+	}
+
+	return count;

drivers/iio/accel/kionix-kx022a.c line 736

Quoted 5 lines

drivers/iio/accel/kionix-kx022a.c new file +5 -0

+}
+
+static int kx022a_prepare_irq_pin(struct kx022a_data *data)
+{
+	/* Enable IRQ1 pin. Set polarity to active low */

Must either handle both pins or at least know if it is irq2 and treat that as no irq for now.

Quoted 14 lines

drivers/iio/accel/kionix-kx022a.c new file +13 -0

+	int mask = KX022A_MASK_IEN1 | KX022A_MASK_IPOL1 |
+		   KX022A_MASK_ITYP;
+	int val = KX022A_MASK_IEN1 | KX022A_IPOL_LOW |
+		  KX022A_ITYP_LEVEL;
+	int ret;
+
+	ret = regmap_update_bits(data->regmap, KX022A_REG_INC1, mask, val);
+	if (ret)
+		return ret;
+
+	mask = KX022A_MASK_INS2_DRDY | KX122_MASK_INS2_WMI;
+
+	return regmap_set_bits(data->regmap, KX022A_REG_INC4, mask);

drivers/iio/accel/kionix-kx022a.c line 756

Quoted 7 lines

drivers/iio/accel/kionix-kx022a.c new file +7 -0

+}
+
+static int kx022a_fifo_disable(struct kx022a_data *data)
+{
+	int ret = 0;
+
+	/* PC1 to 0 */

? No idea what that means...

Quoted 28 lines

drivers/iio/accel/kionix-kx022a.c new file +27 -0

+	ret = kx022a_turn_off_lock(data);
+	if (ret)
+		return ret;
+
+	ret = regmap_clear_bits(data->regmap, KX022A_REG_INC4,
+				KX022A_MASK_WMI1);
+	if (ret)
+		goto unlock_out;
+
+	/* disable buffer */
+	ret = regmap_clear_bits(data->regmap, KX022A_REG_BUF_CNTL2,
+				KX022A_MASK_BUF_EN);
+	if (ret)
+		goto unlock_out;
+
+	data->state &= (~KX022A_STATE_FIFO);
+
+	kx022a_drop_fifo_contents(data);
+
+	return kx022a_turn_on_unlock(data);
+
+unlock_out:
+	mutex_unlock(&data->mutex);
+
+	return ret;
+}
+

drivers/iio/accel/kionix-kx022a.c line 802

Quoted 9 lines

drivers/iio/accel/kionix-kx022a.c new file +9 -0

+static int kx022a_fifo_enable(struct kx022a_data *data)
+{
+	int ret = 0;
+
+	ret = kx022a_turn_off_lock(data);
+	if (ret)
+		return ret;
+
+	/* Write WMI to HW */

Where WMI is?

Quoted 4 lines

drivers/iio/accel/kionix-kx022a.c new file +4 -0

+
+static int kx022a_buffer_postenable(struct iio_dev *idev)
+{
+	struct kx022a_data *data = iio_priv(idev);

drivers/iio/accel/kionix-kx022a.c line 834

Quoted 5 lines

drivers/iio/accel/kionix-kx022a.c new file +5 -0

+
+	/*
+	 * If we use triggers, then the IRQs should be handled by trigger
+	 * enable and buffer is not used but we just add results to buffer
+	 * when data-ready triggers.

That comment needs rewriting given you have two buffers and I think they are very different things..

Quoted 21 lines

drivers/iio/accel/kionix-kx022a.c new file +21 -0

+
+/* Get timestamps and wake the thread if we need to read data */
+static irqreturn_t kx022a_irq_handler(int irq, void *private)
+{
+	struct iio_dev *idev = private;
+	struct kx022a_data *data = iio_priv(idev);
+	bool ack = false;
+
+	data->old_timestamp = data->timestamp;
+	data->timestamp = iio_get_time_ns(idev);
+
+	if (data->trigger.enabled) {
+		iio_trigger_poll(data->trigger.indio_trig);
+		ack = true;
+	}
+
+	if (data->state & KX022A_STATE_FIFO)
+		return IRQ_WAKE_THREAD;
+
+	if (ack) {
+		/*

drivers/iio/accel/kionix-kx022a.c line 890

Quoted 3 lines

drivers/iio/accel/kionix-kx022a.c new file +3 -0

+		 * The IRQ is not acked until data is read. We need to disable
+		 * the IRQ in order to schedule the trigger thread. Enabling
+		 * is done in reenable.

I'm a little confused at what is going on here, but a common solution to situations like this is to handling to use iio_trigger_poll_chained() from the thread and a IRQF_ONESHOT irq. Thus the interrupt will remain masked until the trigger consumers are all done.

Quoted 26 lines

drivers/iio/accel/kionix-kx022a.c new file +25 -0

+		 *
+		 * It would be possible to set the IRQ to 50uS pulse. If we are
+		 * losing data due to the disabled IRQ we can evaluate the
+		 * option of using edge triggered IRQs with the pulse mode.
+		 */
+		disable_irq_nosync(irq);
+		return IRQ_HANDLED;
+	}
+
+	return IRQ_NONE;
+}
+
+/* Read the data from the fifo and fill IIO buffers */
+static irqreturn_t kx022a_irq_thread_handler(int irq, void *private)
+{
+	struct iio_dev *idev = private;
+	struct kx022a_data *data = iio_priv(idev);
+	bool ack = false;
+	int ret;
+
+	mutex_lock(&data->mutex);
+
+	if (data->state & KX022A_STATE_FIFO) {
+		ret = __kx022a_fifo_flush(idev, KX022A_FIFO_LENGTH, true);
+		if (ret > 0)

drivers/iio/accel/kionix-kx022a.c line 921

Quoted 6 lines

drivers/iio/accel/kionix-kx022a.c new file +6 -0

+			ack = true;
+	}
+	/*
+	 * WMI and data-ready IRQs are acked when results are read. If we add
+	 * TILT/WAKE or other IRQs - then we may need to implement the acking
+	 * (which is racy).

I'm not even going to ask for more info at this stage but that sound worrying.

Quoted 10 lines

drivers/iio/accel/kionix-kx022a.c new file +9 -0

+	 */
+	if (ack)
+		ret = IRQ_HANDLED;
+	else
+		ret = IRQ_NONE;
+
+	mutex_unlock(&data->mutex);
+
+	return ret;

drivers/iio/accel/kionix-kx022a.c line 963

Quoted 5 lines

drivers/iio/accel/kionix-kx022a.c new file +5 -0

+}
+
+static void kx022a_trig_reen(struct iio_trigger *trig)
+{
+	struct kx022a_trigger *t = iio_trigger_get_drvdata(trig);

Mentioned above, but I really don't like the pointer loops between the trigger and main _data structure.

Quoted 16 lines

drivers/iio/accel/kionix-kx022a.c new file +16 -0

+	struct kx022a_data *data = t->data;
+
+	enable_irq(data->irq);
+}
+
+static const struct iio_trigger_ops kx022a_trigger_ops = {
+	.set_trigger_state = kx022a_trigger_set_state,
+	.reenable = kx022a_trig_reen,
+};
+
+static int kx022a_chip_init(struct kx022a_data *data)
+{
+	int ret, dummy;
+
+	/*
+	 * Disable IRQs because if the IRQs are left on (for example by

drivers/iio/accel/kionix-kx022a.c line 984

Quoted 5 lines

drivers/iio/accel/kionix-kx022a.c new file +5 -0

+	 * a shutdown which did not deactivate the accelerometer) we do
+	 * most probably end up flooding the system with unhandled IRQs
+	 * and get the line disabled from SOC side.
+	 */
+	ret = regmap_write(data->regmap, KX022A_REG_INC4, 0);

Unusual to do this rather than a reset. Quick look suggests there is a suitable software reset (CNTL2)

Quoted 14 lines

drivers/iio/accel/kionix-kx022a.c new file +14 -0

+	if (ret) {
+		dev_err(data->dev, "Failed to init IRQ states\n");
+		return ret;
+	}
+
+	ret = kx022a_set_drdy_irq(data, false);
+	if (ret) {
+		dev_err(data->dev, "Failed to init DRDY\n");
+		return ret;
+	}
+
+	/* Clear any pending IRQs */
+	ret = regmap_read(data->regmap, KX022A_REG_INT_REL, &dummy);
+	if (ret) {

drivers/iio/accel/kionix-kx022a.c line 1004

Quoted 6 lines

drivers/iio/accel/kionix-kx022a.c new file +6 -0

+		dev_err(data->dev, "Failed to ACK IRQs\n");
+		return ret;
+	}
+	/* set data res 16bit */
+	ret = regmap_set_bits(data->regmap, KX022A_REG_BUF_CNTL2,
+			      KX022A_MASK_BRES);

Can we have a _BRES_16 to make this self documenting?

drivers/iio/accel/kionix-kx022a.c line 1104

Quoted 2 lines

drivers/iio/accel/kionix-kx022a.c new file +2 -0

+
+	ret = iio_triggered_buffer_setup_ext(idev,

devm_

Quoted 18 lines

drivers/iio/accel/kionix-kx022a.c new file +17 -0

+					     &iio_pollfunc_store_time,
+					     kx022a_trigger_handler,
+					     IIO_BUFFER_DIRECTION_IN,
+					     &kx022a_buffer_ops,
+					     kx022a_fifo_attributes);
+
+	if (ret)
+		return dev_err_probe(data->dev, ret,
+				     "iio_triggered_buffer_setup_ext FAIL %d\n",
+				     ret);
+
+	indio_trig = devm_iio_trigger_alloc(dev, "%sdata-rdy-dev%d", idev->name,
+					    iio_device_id(idev));
+	if (!indio_trig)
+		return -ENOMEM;
+
+	data->trigger.indio_trig = indio_trig;

drivers/iio/accel/kionix-kx022a.c line 1127

Quoted 6 lines

drivers/iio/accel/kionix-kx022a.c new file +6 -0

+
+	indio_trig->ops = &kx022a_trigger_ops;
+	data->trigger.data = data;
+	iio_trigger_set_drvdata(indio_trig, &data->trigger);
+
+	ret = iio_trigger_register(indio_trig);

Don't mix devm and non devm allocation / registration.

Quoted 11 lines

drivers/iio/accel/kionix-kx022a.c new file +10 -0

+	if (ret)
+		return dev_err_probe(data->dev, ret,
+				     "Trigger registration failed\n");
+
+	ret = devm_iio_device_register(data->dev, idev);
+	if (ret < 0)
+		return dev_err_probe(dev, ret,
+				     "Unable to register iio device\n");
+
+	ret = devm_request_threaded_irq(data->dev, irq, kx022a_irq_handler,

drivers/iio/accel/kionix-kx022a.c line 1145

Quoted 8 lines

drivers/iio/accel/kionix-kx022a.c new file +8 -0

+					&kx022a_irq_thread_handler,
+					IRQF_ONESHOT, "kx022", idev);
+	if (ret)
+		return dev_err_probe(data->dev, ret, "Could not request IRQ\n");
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(kx022a_probe_internal);

Quoted 22 lines · Jonathan Cameron

Quoted 12 lines · On Wed, 21 Sep 2022 14:45:35 +0300 Matti Vaittinen <[email protected]>…

KX022A is a 3-axis Accelerometer from ROHM/Kionix. The senor features include variable ODRs, I2C and SPI control, FIFO/LIFO with watermark IRQ, tap/motion detection, wake-up & back-to-sleep events, four acceleration ranges (2, 4, 8 and 16g) and probably some other cool fatures.

Add support for the basic accelerometer features such as getting the acceleration data via IIO. (raw reads, triggered buffer [data-ready] or using the WMI IRQ).

Important things to be added include the double-tap, motion detection and wake-up as well as the runtime power management.

Quoted 7 lines

NOTE: Filling-up the hardware FIFO should be avoided. During my testing I noticed that filling up the hardware FIFO might mess-up the sample count. My sensor ended up in a state where amount of data in FIFO was reported to be 0xff bytes, which equals to 42,5 samples. Specification says the FIFO can hold maximum of 41 samples in HiRes mode. Also, at least once the FIFO was stuck in a state where reading data from hwardware FIFO did not decrease the amount of data reported to be in the

Quoted 6 lines

spell check this.

Quoted 3 lines

FIFO - eg. FIFO was "stuck". The code has now an error count and 10 reads with invalid FIFO data count will cause the fifo contents to be dropped.

Ouch - that's nasty.

Indeed it is. As this commit states, this is pretty initial support for the accelerometer. I want to enable people to do basic experimenting and also use the component to some slow ODR solutions. Besides, having even a basic support in-tree enable people to add further improvements :) So, I am hoping / expecting to see improvements added also by other people using this. I think that after this initial support many people still _need_ for example the runtime PM. Maybe we will also end up with some nicer solution to the FIFO issues.

Oh, please note that the patch is no longer complete. I've "snipped" unrelated pieces when replying to Jonathan. I did also just silently ignore the comments which I simply agree with.

Quoted 9 lines

Quoted 3 lines

Signed-off-by Matti Vaittinen <[email protected]>

Hi Matti,

A somewhat superficial review as I'm short on time, but wanted to get some initial comments out to you as I'd started reviewing this yesterday and not sure when I'd get back to it.

No problem Jonathan. I appreciate the feedback - and even if the review has not been completed the initial feedback will already allow me to rework :) I won't go anywhere so you can for sure send me more comments during the coming new iterations.

Besides, I appreciate your engagement even more as some maintainers do not have the time to look at the RFC patches at all. Getting even initial feedback this quickly is much more than I expected! So, thanks!

Quoted 8 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+// SPDX-License-Identifier: GPL-2.0-only
+//  

Other than where required for SPDX use the
/*
* Copyright...
*/
syntax.

No problem. Some maintainers prefer having the top of the file comment block looking coherent and it also pleases my eye more. Still, can be changed to suit the IIO-style.

Quoted 7 lines

Quoted 6 lines

drivers/iio/accel/kionix-kx022a-i2c.c new file +4 -0

+// Copyright (C) 2022 ROHM Semiconductors
+//
+// ROHM/KIONIX KX022A accelerometer driver
+  

Quoted 10 lines

drivers/iio/accel/kionix-kx022a-i2c.c line 19

Quoted 6 lines

drivers/iio/accel/kionix-kx022a-i2c.c new file +6 -0

+static int kx022a_i2c_probe(struct i2c_client *i2c)
+{
+	struct regmap *regmap;
+	struct device *dev = &i2c->dev;
+
+	if (!i2c->irq) {  

There look to be 2 interrupt lines, so you need to know which one this is. fwnode_irq_get_byname() trying first int1 then int2 (as only a single one might be mapped).

Seems like you know the Kionix component(s).

Cool! My idea is initially
to only support a fixed configuration with IRQ connected to the IRQ1 pin. The support for the second IRQ can be added when needed. I think it's better to have at first something working and add features iteratively when needed. I do also believe that people who add support to IRQ2-pin should also be testing this. I have currently only IRQ1 connected.

Would a comment explaining this do?

You made me think that this should probably be explained in the dt-bindings already. I need to add the irq-names and mention that if the irq-names is not populated it is assumed (only) the IRQ1 is used.

Quoted 11 lines

drivers/iio/accel/kionix-kx022a.c line 146

Quoted 5 lines

drivers/iio/accel/kionix-kx022a.c new file +4 -0

+
+struct kx022a_data;
+
+struct kx022a_trigger {  

Unusual structure. IIRC we normally just squash this stuff into the iio_priv structure and set the trigger devdata to the iio_dev. I haven't really thought about the advantages of this way around but can see a disadvantage is you get circular pointer loops this way.

I'd better admit I didn't invent this myself. I stole big parts of the logic from the bmc150-accel-core.c which supports multiple triggers. (By the way, I should probably metion that in the commit message). All triggers are in an array - so if you only get the trigger pointer to one of the triggers in the array, then calculating the start of the containing struct becomes non trivial.

This does not apply to this driver now as we only support the data-ready trigger so I can change this. OTOH, reading the list of features KX022A hardware provides - I wouldn't be surprized to see a new trigger(s) in the future. It'd be nice to get support for different modes like for example capturing N events that were stored prior certain movement.

These might be best implemented with triggers. What is your adviece - is it worth trying to keep adding new trigger(s) as a low-hanging fruit - or is it just better to make the current (limited) implementation as simple as possible?

Quoted 11 lines

drivers/iio/accel/kionix-kx022a.c line 345

Quoted 8 lines

drivers/iio/accel/kionix-kx022a.c new file +4 -0

+static int kx022a_turn_on_unlock(struct kx022a_data *data)
+{
+	int ret;
+  

This is not used enough that I can see a strong reason for the wrapper. Just put the two calls inline and rename the unlocked case.

I kind of see the value of the wrappers as it documents the purpose of locking. It's kind of a wake-up that we really want to keep the access locked during sensor 'shut-down'. I don't know how other sensors work but for me the Kionix sensors requiring the sensor to be "turned off" for the duration of pretty much _any_ configurations was a surprize...

Again, I think this will be required for _all_ new configuration code we add in the driver.

Quoted 6 lines

Quoted 6 lines

drivers/iio/accel/kionix-kx022a.c new file +5 -0

+	ret = __kx022a_turn_on_unlocked(data);
+	mutex_unlock(&data->mutex);
+
+	return ret;
+}

Quoted 24 lines

Quoted 24 lines

drivers/iio/accel/kionix-kx022a.c new file +24 -0

+	/*
+	 * Approximate timestamps for each of the sample based on the sampling
+	 * frequency, timestamp for last sample and number of samples.
+	 *
+	 * We'd better not use the current bandwidth settings to compute the
+	 * sample period. The real sample rate varies with the device and
+	 * small variation adds when we store a large number of samples.
+	 *
+	 * To avoid this issue we compute the actual sample period ourselves
+	 * based on the timestamp delta between the last two flush operations.
+	 */
+	if (data->old_timestamp) {
+		sample_period = (data->timestamp - data->old_timestamp);
+		do_div(sample_period, count);
+	} else {
+		sample_period = data->odr_ns;
+	}
+	tstamp = data->timestamp - (count - 1) * sample_period;
+
+	if (samples && count > samples) {
+		/*
+		 * Here we leave some old samples to the buffer. We need to
+		 * adjust the timestamp to match the first sample in the buffer
+		 * or we will miscalculate the sample_period at next round.

Quoted 2 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+		 */ >> +		data->timestamp -= (count - samples) * sample_period;
+		count = samples;  

Just a note not directly related to this driver. I borrowed (stoled) the timestamp logic from the bmc150-accel-core.c. The bmc150-core did not have this timestamp adjustment which I think is necessary in case the fifo is not read empty. Someone familiar with the bmc150-accel-core.c might want to check this.

Quoted 2 lines

Quoted 2 lines

(quoted diff) +1 -0

+	}

Quoted 7 lines

drivers/iio/accel/kionix-kx022a.c line 736

Quoted 4 lines

drivers/iio/accel/kionix-kx022a.c new file +4 -0

+
+static int kx022a_prepare_irq_pin(struct kx022a_data *data)
+{
+	/* Enable IRQ1 pin. Set polarity to active low */  

Must either handle both pins or at least know if it is irq2 and treat that as no irq for now.

I don't want to try supporting both pins for now. It makes this somewhat more complex - especially if we want to support using two IRQs. That will require some thorough thinking which I don't have time to do right now :(

I can add the irq-names to the bindings and add check to the probe so that if the irq2 is used we error out with nice 'not supported' message.

Quoted 11 lines

drivers/iio/accel/kionix-kx022a.c line 802

Quoted 9 lines

drivers/iio/accel/kionix-kx022a.c new file +9 -0

+static int kx022a_fifo_enable(struct kx022a_data *data)
+{
+	int ret = 0;
+
+	ret = kx022a_turn_off_lock(data);
+	if (ret)
+		return ret;
+
+	/* Write WMI to HW */  

Where WMI is?

Stored in the iio_priv. data->watermark. The kx022a_set_watermark() only updates the value in iio_priv() and we take it in use only when the buffer is enabled.

Quoted 16 lines

Quoted 16 lines

drivers/iio/accel/kionix-kx022a.c new file +13 -0

+/* Read the data from the fifo and fill IIO buffers */
+static irqreturn_t kx022a_irq_thread_handler(int irq, void *private)
+{
+	struct iio_dev *idev = private;
+	struct kx022a_data *data = iio_priv(idev);
+	bool ack = false;
+	int ret;
+
+	mutex_lock(&data->mutex);
+
+	if (data->state & KX022A_STATE_FIFO) {
+		ret = __kx022a_fifo_flush(idev, KX022A_FIFO_LENGTH, true);
+		if (ret > 0)

Quoted 8 lines

drivers/iio/accel/kionix-kx022a.c line 921

Quoted 6 lines

drivers/iio/accel/kionix-kx022a.c new file +6 -0

+			ack = true;
+	}
+	/*
+	 * WMI and data-ready IRQs are acked when results are read. If we add
+	 * TILT/WAKE or other IRQs - then we may need to implement the acking
+	 * (which is racy).  

I'm not even going to ask for more info at this stage but that sound worrying.

Yep, you didn't ask but I can still babble a bit :) It's just about the fact that this (and many other) Kionix sensors clear all IRQs when a single bit of a single register is written. This naturally is racy because there will always be a time-window for new IRQs to trigger between reading the active IRQs and writing the ack. So with this acking mechanism we do potentially lose IRQs.

It might be there were separate ack registers for both of the IRQ pins - meaning we might be able to use separate pins for separate (ackable) IRQs - and be able to support two IRQs without an acking race. This however would require some logic for which pin to use for which IRQ - and potentially this selection could be HW dependant. Currently I am not supporting anything but watermark and data-ready, both of which get cleaned when data is read. It's still worth mentioning that care must be taken if ackable IRQs are to be added...

Quoted 17 lines

Quoted 17 lines

drivers/iio/accel/kionix-kx022a.c new file +16 -0

+	struct kx022a_data *data = t->data;
+
+	enable_irq(data->irq);
+}
+
+static const struct iio_trigger_ops kx022a_trigger_ops = {
+	.set_trigger_state = kx022a_trigger_set_state,
+	.reenable = kx022a_trig_reen,
+};
+
+static int kx022a_chip_init(struct kx022a_data *data)
+{
+	int ret, dummy;
+
+	/*
+	 * Disable IRQs because if the IRQs are left on (for example by

Quoted 8 lines

drivers/iio/accel/kionix-kx022a.c line 984

Quoted 5 lines

drivers/iio/accel/kionix-kx022a.c new file +5 -0

+	 * a shutdown which did not deactivate the accelerometer) we do
+	 * most probably end up flooding the system with unhandled IRQs
+	 * and get the line disabled from SOC side.
+	 */
+	ret = regmap_write(data->regmap, KX022A_REG_INC4, 0);  

Unusual to do this rather than a reset. Quick look suggests there is a suitable software reset (CNTL2)

Same thing was just told me by a colleague of mine yesterday. Reset simplifies this quite a bit. (I just wonder if we can trust the reset always initializes the IC to same defaults or if there may be OTP differencies. I am new to these sensors).

Quoted 30 lines · On Fri, 23 Sep 2022 09:31:12 +0300 Matti Vaittinen <[email protected]>…

Quoted 22 lines · Jonathan Cameron

Quoted 12 lines · On Wed, 21 Sep 2022 14:45:35 +0300 Matti Vaittinen <[email protected]>…

KX022A is a 3-axis Accelerometer from ROHM/Kionix. The senor features include variable ODRs, I2C and SPI control, FIFO/LIFO with watermark IRQ, tap/motion detection, wake-up & back-to-sleep events, four acceleration ranges (2, 4, 8 and 16g) and probably some other cool fatures.

Add support for the basic accelerometer features such as getting the acceleration data via IIO. (raw reads, triggered buffer [data-ready] or using the WMI IRQ).

Important things to be added include the double-tap, motion detection and wake-up as well as the runtime power management.

Quoted 7 lines

NOTE: Filling-up the hardware FIFO should be avoided. During my testing I noticed that filling up the hardware FIFO might mess-up the sample count. My sensor ended up in a state where amount of data in FIFO was reported to be 0xff bytes, which equals to 42,5 samples. Specification says the FIFO can hold maximum of 41 samples in HiRes mode. Also, at least once the FIFO was stuck in a state where reading data from hwardware FIFO did not decrease the amount of data reported to be in the

Quoted 6 lines

spell check this.

Quoted 3 lines

FIFO - eg. FIFO was "stuck". The code has now an error count and 10 reads with invalid FIFO data count will cause the fifo contents to be dropped.

Ouch - that's nasty.

Quoted 8 lines

Indeed it is. As this commit states, this is pretty initial support for the accelerometer. I want to enable people to do basic experimenting and also use the component to some slow ODR solutions. Besides, having even a basic support in-tree enable people to add further improvements :) So, I am hoping / expecting to see improvements added also by other people using this. I think that after this initial support many people still _need_ for example the runtime PM. Maybe we will also end up with some nicer solution to the FIFO issues.

My initial gut feeling is that, if that fifo issue doesn't have a clean solution, we don't don't support the fifo (by default anyway - could have a module parameter or something to turn it on). Late handling of interrupts is something that can happen for lots of reasons. It should never be fatal!

Simple first. Different matter if you add the other triggers later in
the same patch series, but history says half the interesting features we plan to add never get added.

Quoted 8 lines

Quoted 2 lines

Must either handle both pins or at least know if it is irq2 and treat that as no irq for now.

I don't want to try supporting both pins for now. It makes this somewhat more complex - especially if we want to support using two IRQs. That will require some thorough thinking which I don't have time to do right now :(

Quoted 3 lines

I can add the irq-names to the bindings and add check to the probe so that if the irq2 is used we error out with nice 'not supported' message.

A slightly nicer thing to do is support one irq, but let it be either irq1 or irq2. If both are supplied just ignore the second one. People have an 'amusing' habit of only wiring up irq2 on boards.

Quoted 16 lines

Quoted 15 lines

drivers/iio/accel/kionix-kx022a.c line 984

Quoted 12 lines

drivers/iio/accel/kionix-kx022a.c new file +12 -0

+
+static int kx022a_chip_init(struct kx022a_data *data)
+{
+	int ret, dummy;
+
+	/*
+	 * Disable IRQs because if the IRQs are left on (for example by
+	 * a shutdown which did not deactivate the accelerometer) we do
+	 * most probably end up flooding the system with unhandled IRQs
+	 * and get the line disabled from SOC side.
+	 */
+	ret = regmap_write(data->regmap, KX022A_REG_INC4, 0);

Unusual to do this rather than a reset. Quick look suggests there is a suitable software reset (CNTL2)

Quoted 5 lines

Same thing was just told me by a colleague of mine yesterday. Reset simplifies this quite a bit. (I just wonder if we can trust the reset always initializes the IC to same defaults or if there may be OTP differencies. I am new to these sensors).

I really hope we can rely on any documented reset values!

Quoted 42 lines · Jonathan Cameron

Quoted 30 lines · On Fri, 23 Sep 2022 09:31:12 +0300 Matti Vaittinen <[email protected]>…

Quoted 22 lines · Jonathan Cameron

Quoted 12 lines · On Wed, 21 Sep 2022 14:45:35 +0300 Matti Vaittinen <[email protected]>…

KX022A is a 3-axis Accelerometer from ROHM/Kionix. The senor features include variable ODRs, I2C and SPI control, FIFO/LIFO with watermark IRQ, tap/motion detection, wake-up & back-to-sleep events, four acceleration ranges (2, 4, 8 and 16g) and probably some other cool fatures.

Add support for the basic accelerometer features such as getting the acceleration data via IIO. (raw reads, triggered buffer [data-ready] or using the WMI IRQ).

Important things to be added include the double-tap, motion detection and wake-up as well as the runtime power management.

Quoted 7 lines

NOTE: Filling-up the hardware FIFO should be avoided. During my testing I noticed that filling up the hardware FIFO might mess-up the sample count. My sensor ended up in a state where amount of data in FIFO was reported to be 0xff bytes, which equals to 42,5 samples. Specification says the FIFO can hold maximum of 41 samples in HiRes mode. Also, at least once the FIFO was stuck in a state where reading data from hwardware FIFO did not decrease the amount of data reported to be in the

Quoted 6 lines

spell check this.

Quoted 3 lines

FIFO - eg. FIFO was "stuck". The code has now an error count and 10 reads with invalid FIFO data count will cause the fifo contents to be dropped.

Ouch - that's nasty.

Quoted 8 lines

Indeed it is. As this commit states, this is pretty initial support for the accelerometer. I want to enable people to do basic experimenting and also use the component to some slow ODR solutions. Besides, having even a basic support in-tree enable people to add further improvements :) So, I am hoping / expecting to see improvements added also by other people using this. I think that after this initial support many people still _need_ for example the runtime PM. Maybe we will also end up with some nicer solution to the FIFO issues.

Quoted 5 lines

My initial gut feeling is that, if that fifo issue doesn't have a clean solution, we don't don't support the fifo (by default anyway - could have a module parameter or something to turn it on). Late handling of interrupts is something that can happen for lots of reasons. It should never be fatal!

Tell me about it... More than 10-years ago I "inherited" maintenance of a timing code which was built on periodic 10msec IRQ which incremented a counter. (And after that there were many new generations of Linux based devices with various "rt"-requirements). Missing an IRQ was fatal as there were no hardware where we could read the correct timestamp and other units in the system were no longer in sync if the counter was wrong. Only recovery was complete system restart for all units - which in that use case was really bad. I learned to absolutely hate the serial prints over slow UART - and I learned to love irqsoff tracer. I don't work for that company anymore and I believe the product has already retired. Yet, I still get *shrugs* when I see hard time limits for serving IRQs on Linux. (Other than that I kind of like pondering the rt-challenges).

Anyways, I don't see a real risk for example when using the ODRs < 2 Hz and setting the watermark to somewhere near 20 samples.

It's really unfortunate the HW has these "hickups" - but I think it is still perfectly usable for many cases - we just really need to document the corner cases somewhere.

Quoted 4 lines

Simple first. Different matter if you add the other triggers later in
the same patch series, but history says half the interesting features we plan to add never get added.

ack.

Quoted 8 lines

Quoted 8 lines

Quoted 2 lines

Must either handle both pins or at least know if it is irq2 and treat that as no irq for now.

I don't want to try supporting both pins for now. It makes this somewhat more complex - especially if we want to support using two IRQs. That will require some thorough thinking which I don't have time to do right now :(

Quoted 8 lines

Quoted 3 lines

I can add the irq-names to the bindings and add check to the probe so that if the irq2 is used we error out with nice 'not supported' message.

A slightly nicer thing to do is support one irq, but let it be either irq1 or irq2. If both are supplied just ignore the second one. People have an 'amusing' habit of only wiring up irq2 on boards.

Ok. It shouldn't be such a big change for the code. I'll see what it will look like.

Quoted 16 lines

Quoted 16 lines

Quoted 15 lines

drivers/iio/accel/kionix-kx022a.c line 984

Quoted 12 lines

drivers/iio/accel/kionix-kx022a.c new file +12 -0

+
+static int kx022a_chip_init(struct kx022a_data *data)
+{
+	int ret, dummy;
+
+	/*
+	 * Disable IRQs because if the IRQs are left on (for example by
+	 * a shutdown which did not deactivate the accelerometer) we do
+	 * most probably end up flooding the system with unhandled IRQs
+	 * and get the line disabled from SOC side.
+	 */
+	ret = regmap_write(data->regmap, KX022A_REG_INC4, 0);  

Unusual to do this rather than a reset. Quick look suggests there is a suitable software reset (CNTL2)

Quoted 7 lines

Quoted 5 lines

Same thing was just told me by a colleague of mine yesterday. Reset simplifies this quite a bit. (I just wonder if we can trust the reset always initializes the IC to same defaults or if there may be OTP differencies. I am new to these sensors).

I really hope we can rely on any documented reset values!

That is a sane assumption to do - but in my experience we don't live in a sane world ;) I've seen way too many cases where the defaults are changed for ICs for example by changing OTP. And sometimes the OTP change has not been visible to the drivers from any revision registers or such.

I'm not talking about Kionix sensors though as I've never worked with Kionix sensors before - so let's just try out the reset and fix things if problems emerge. I am probably just a bit paranoid :)