iio: Support ROHM/Kionix kx022a

KX022A is a 3-axis accelerometer from ROHM/Kionix. The sensor 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 features.

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.

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

---
v3 => v4 fixes suggested by Andy:
- styling changes
- use str_on_off()
- drop check for !dev
- drop adding return value to print printed using dev_err_probe()
- use dev_err_probe() in SPI/I2C drivers too
- fix IRQ thread return value

v2 => v3 Mostly fixes suggested by Andy
- styling issues
- spell-checks
- use namespaces for exported symbols
- drop module param
- document the lock
- change value written when clearing fifo
- drop WARN_ON()
- correctly check the return value from fwnode_irq_get_byname()
- do not emphasize config Y over M
- reorder struct kx022a_data from potential optimization
- IIO_DEVICE_ATTR_RO instead of IIO_DEVICE_ATTR where applicable
- directly include bits.h for BIT()
- use sysfs_emit() for sysfs
- use unique name for IRQ
- convert read_raw() values to CPU endianess
- fix HW-fifo size to 258 bytes
- kx022a-spi, Fix kconfig dependency
- disable irq (to protect timestamp / sample amount calculation) when the fifo flush is iniriated by user-space

RFCv1 => v2 (mostly based on feedback from Jonathan):
- Fix bunch of typos from the commit message.
- Add missing break; to the kx022a_write_raw()
- Fix SPI driver to use of_match_table
- Fix indentiation in I2C driver
- Drop struct kx022a_trigger
- Drop cross references from Kconfig
- Use /* */ also in file header comments
- Misc minor styling
- Do sensor-reset at probe
- Support both IRQ pins
- Implement read_avail callback
- Use dma aligned buffers for bulk-reads
- Use iio_trigger_poll_chained()
- Use devm consistently
- Drop inclusion of device.h
- Add SPI device ID for module loading
- Add module param for hw fifo / watermark IRQ usage
- Fix io-vdd-supply name to match one in the bindings

---
 drivers/iio/accel/Kconfig             |   21 +
 drivers/iio/accel/Makefile            |    3 +
 drivers/iio/accel/kionix-kx022a-i2c.c |   51 ++
 drivers/iio/accel/kionix-kx022a-spi.c |   58 ++
 drivers/iio/accel/kionix-kx022a.c     | 1145 +++++++++++++++++++++++++
 drivers/iio/accel/kionix-kx022a.h     |   82 ++
 6 files changed, 1360 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 +21 -0

index ffac66db7ac9..b7fd054819d2 100644
@@ -409,6 +409,27 @@ 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
+
+config IIO_KX022A_SPI
+	tristate "Kionix KX022A tri-axis digital accelerometer"
+	depends on SPI
+	select IIO_KX022A
+	select REGMAP_SPI
+	help
+	  Enable support for the Kionix KX022A digital tri-axis
+	  accelerometer connected to I2C interface.
+
+config IIO_KX022A_I2C
+	tristate "Kionix KX022A tri-axis digital accelerometer"
+	depends on I2C
+	select IIO_KX022A
+	select REGMAP_I2C
+	help
+	  Enable support for the Kionix KX022A digital tri-axis
+	  accelerometer connected to I2C interface.
+
 config KXSD9
 	tristate "Kionix KXSD9 Accelerometer Driver"
 	select IIO_BUFFER

drivers/iio/accel/Makefile +3 -0

index 5e45b5fa5ab5..311ead9c3ef1 100644
@@ -40,6 +40,9 @@ obj-$(CONFIG_FXLS8962AF)	+= fxls8962af-core.o
 obj-$(CONFIG_FXLS8962AF_I2C)	+= fxls8962af-i2c.o
 obj-$(CONFIG_FXLS8962AF_SPI)	+= fxls8962af-spi.o
 obj-$(CONFIG_HID_SENSOR_ACCEL_3D) += hid-sensor-accel-3d.o
+obj-$(CONFIG_IIO_KX022A)	+= kionix-kx022a.o
+obj-$(CONFIG_IIO_KX022A_I2C)	+= kionix-kx022a-i2c.o
+obj-$(CONFIG_IIO_KX022A_SPI)	+= kionix-kx022a-spi.o
 obj-$(CONFIG_KXCJK1013) += kxcjk-1013.o
 obj-$(CONFIG_KXSD9)	+= kxsd9.o
 obj-$(CONFIG_KXSD9_SPI)	+= kxsd9-spi.o

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

new file mode 100644
index 000000000000..6510f8d62b85
@@ -0,0 +1,51 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) 2022 ROHM Semiconductors
+ *
+ * ROHM/KIONIX KX022A accelerometer driver
+ */
+
+#include <linux/i2c.h>
+#include <linux/interrupt.h>
+#include <linux/module.h>
+#include <linux/regmap.h>
+
+#include "kionix-kx022a.h"
+
+static int kx022a_i2c_probe(struct i2c_client *i2c)
+{
+	struct device *dev = &i2c->dev;
+	struct regmap *regmap;
+
+	if (!i2c->irq) {
+		dev_err(dev, "No IRQ configured\n");
+		return -EINVAL;
+	}
+
+	regmap = devm_regmap_init_i2c(i2c, &kx022a_regmap);
+	if (IS_ERR(regmap))
+		return dev_err_probe(dev, PTR_ERR(regmap),
+				     "Failed to initialize Regmap\n");
+
+	return kx022a_probe_internal(dev);
+}
+
+static const struct of_device_id kx022a_of_match[] = {
+	{ .compatible = "kionix,kx022a", },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, kx022a_of_match);
+
+static struct i2c_driver kx022a_i2c_driver = {
+	.driver = {
+		.name  = "kx022a-i2c",
+		.of_match_table = kx022a_of_match,
+	  },
+	.probe_new    = kx022a_i2c_probe,
+};
+module_i2c_driver(kx022a_i2c_driver);
+
+MODULE_DESCRIPTION("ROHM/Kionix KX022A accelerometer driver");
+MODULE_AUTHOR("Matti Vaittinen <[email protected]>");
+MODULE_LICENSE("GPL");
+MODULE_IMPORT_NS(KIONIX_ACCEL);

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

new file mode 100644
index 000000000000..7fe3b0aba1fe
@@ -0,0 +1,58 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) 2022 ROHM Semiconductors
+ *
+ * ROHM/KIONIX KX022A accelerometer driver
+ */
+
+#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))
+		return dev_err_probe(dev, PTR_ERR(regmap),
+				     "Failed to initialize Regmap\n");
+
+	return kx022a_probe_internal(dev);
+}
+
+static const struct spi_device_id kx022a_id[] = {
+	{ "kx022a" },
+	{ }
+};
+MODULE_DEVICE_TABLE(spi, kx022a_id);
+
+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",
+		.of_match_table = kx022a_of_match,
+	},
+	.probe = kx022a_spi_probe,
+	.id_table = kx022a_id,
+};
+module_spi_driver(kx022a_spi_driver);
+
+MODULE_DESCRIPTION("ROHM/Kionix kx022A accelerometer driver");
+MODULE_AUTHOR("Matti Vaittinen <[email protected]>");
+MODULE_LICENSE("GPL");
+MODULE_IMPORT_NS(KIONIX_ACCEL);

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

new file mode 100644
index 000000000000..5a8622c8127b
@@ -0,0 +1,1145 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) 2022 ROHM Semiconductors
+ *
+ * ROHM/KIONIX KX022A accelerometer driver
+ */
+
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/interrupt.h>
+#include <linux/module.h>
+#include <linux/moduleparam.h>
+#include <linux/mutex.h>
+#include <linux/property.h>
+#include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
+#include <linux/slab.h>
+#include <linux/string_helpers.h>
+#include <linux/units.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 "kionix-kx022a.h"
+
+/*
+ * The KX022A has FIFO which can store 43 samples of HiRes data from 2
+ * channels. This equals to 43 (samples) * 3 (channels) * 2 (bytes/sample) to
+ * 258 bytes of sample data. The quirk to know is that the amount of bytes in
+ * the FIFO is advertised via 8 bit register (max value 255). The thing to note
+ * is that full 258 bytes of data is indicated using the max value 255.
+ */
+#define KX022A_FIFO_LENGTH			43
+#define KX022A_FIFO_FULL_VALUE			255
+#define KX022A_SOFT_RESET_WAIT_TIME_US		(5 * USEC_PER_MSEC)
+#define KX022A_SOFT_RESET_TOTAL_WAIT_TIME_US	(500 * USEC_PER_MSEC)
+
+/* 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)
+
+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,
+	}, {
+		/* The reset bit will be cleared by sensor */
+		.range_min = KX022A_REG_CNTL2,
+		.range_max = KX022A_REG_CNTL2,
+	}, {
+		.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,
+	.rd_noinc_table = &kx022a_nir_regs,
+	.precious_table = &kx022a_precious_regs,
+	.max_register = KX022A_MAX_REGISTER,
+	.cache_type = REGCACHE_RBTREE,
+};
+EXPORT_SYMBOL_NS_GPL(kx022a_regmap, KIONIX_ACCEL);
+
+struct kx022a_data {
+	struct regmap *regmap;
+	struct iio_trigger *trig;
+	struct device *dev;
+	struct iio_mount_matrix orientation;
+	int64_t timestamp, old_timestamp;
+
+	int irq;
+	int inc_reg;
+	int ien_reg;
+
+	unsigned int g_range;
+	unsigned int state;
+	unsigned int odr_ns;
+
+	bool trigger_enabled;
+	/*
+	 * Prevent toggling the sensor stby/active state (PC1 bit) in the
+	 * middle of a configuration, or when the fifo is enabled. Also,
+	 * protect the data stored/retrieved from this structure from
+	 * concurrent accesses.
+	 */
+	struct mutex mutex;
+	u8 watermark;
+
+	/* 3 x 16bit accel data + timestamp */
+	__le16 buffer[8] __aligned(IIO_DMA_MINALIGN);
+	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),		\
+	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) |	\
+				BIT(IIO_CHAN_INFO_SAMP_FREQ),	\
+	.info_mask_shared_by_type_available =			\
+				BIT(IIO_CHAN_INFO_SCALE) |	\
+				BIT(IIO_CHAN_INFO_SAMP_FREQ),	\
+	.ext_info = kx022a_ext_info,				\
+	.address = KX022A_REG_##axis##OUT_L,			\
+	.scan_index = index,					\
+	.scan_type = {                                          \
+		.sign = 's',					\
+		.realbits = 16,					\
+		.storagebits = 16,				\
+		.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 the
+ * Linux CPUs can handle without dropping samples. Also, the low power mode is
+ * not available for higher sample rates. Thus, the driver only supports 200 Hz
+ * and slower ODRs. The slowest is 0.78 Hz.
+ */
+static const int kx022a_accel_samp_freq_table[][2] = {
+	{ 0, 780000 },
+	{ 1, 563000 },
+	{ 3, 125000 },
+	{ 6, 250000 },
+	{ 12, 500000 },
+	{ 25, 0 },
+	{ 50, 0 },
+	{ 100, 0 },
+	{ 200, 0 },
+};
+
+static const unsigned int kx022a_odrs[] = {
+	1282051282,
+	639795266,
+	320 * MEGA,
+	160 * MEGA,
+	80 * MEGA,
+	40 * MEGA,
+	20 * MEGA,
+	10 * MEGA,
+	5 * MEGA,
+};
+
+/*
+ * 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
+ *	=> KX022A uses 16 bit (HiRes mode - assume the low 8 bits are zeroed
+ *	in low-power mode(?) )
+ *	=> +/-2G  => 4 / 2^16 * 9,80665 * 10^6 (to scale to micro)
+ *	=> +/-2G  - 598.550415
+ *	   +/-4G  - 1197.10083
+ *	   +/-8G  - 2394.20166
+ *	   +/-16G - 4788.40332
+ */
+static const int kx022a_scale_table[][2] = {
+	{ 598, 550415 },
+	{ 1197, 100830 },
+	{ 2394, 201660 },
+	{ 4788, 403320 },
+};
+
+static int kx022a_read_avail(struct iio_dev *indio_dev,
+			     struct iio_chan_spec const *chan,
+			     const int **vals, int *type, int *length,
+			     long mask)
+{
+	switch (mask) {
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		*vals = (const int *)kx022a_accel_samp_freq_table;
+		*length = ARRAY_SIZE(kx022a_accel_samp_freq_table) *
+			  ARRAY_SIZE(kx022a_accel_samp_freq_table[0]);
+		*type = IIO_VAL_INT_PLUS_MICRO;
+		return IIO_AVAIL_LIST;
+	case IIO_CHAN_INFO_SCALE:
+		*vals = (const int *)kx022a_scale_table;
+		*length = ARRAY_SIZE(kx022a_scale_table) *
+			  ARRAY_SIZE(kx022a_scale_table[0]);
+		*type = IIO_VAL_INT_PLUS_MICRO;
+		return IIO_AVAIL_LIST;
+	default:
+		return -EINVAL;
+	}
+}
+
+#define KX022A_DEFAULT_PERIOD_NS (20 * NSEC_PER_MSEC)
+
+static void kx022a_reg2freq(unsigned int val,  int *val1, int *val2)
+{
+	*val1 = kx022a_accel_samp_freq_table[val & KX022A_MASK_ODR][0];
+	*val2 = kx022a_accel_samp_freq_table[val & KX022A_MASK_ODR][1];
+}
+
+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][0];
+	*val2 = kx022a_scale_table[val][1];
+}
+
+static int kx022a_turn_on_off_unlocked(struct kx022a_data *data, bool on)
+{
+	int ret;
+
+	if (on)
+		ret = regmap_set_bits(data->regmap, KX022A_REG_CNTL,
+				      KX022A_MASK_PC1);
+	else
+		ret = regmap_clear_bits(data->regmap, KX022A_REG_CNTL,
+					KX022A_MASK_PC1);
+	if (ret)
+		dev_err(data->dev, "Turn %s fail %d\n", str_on_off(on), ret);
+
+	return ret;
+
+}
+
+static int kx022a_turn_off_lock(struct kx022a_data *data)
+{
+	int ret;
+
+	mutex_lock(&data->mutex);
+	ret = kx022a_turn_on_off_unlocked(data, false);
+	if (ret)
+		mutex_unlock(&data->mutex);
+
+	return ret;
+}
+
+static int kx022a_turn_on_unlock(struct kx022a_data *data)
+{
+	int ret;
+
+	ret = kx022a_turn_on_off_unlocked(data, true);
+	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, n;
+
+	/*
+	 * 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).
+	 */
+	ret = iio_device_claim_direct_mode(idev);
+	if (ret)
+		return ret;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		n = ARRAY_SIZE(kx022a_accel_samp_freq_table);
+
+		while (n--)
+			if (val == kx022a_accel_samp_freq_table[n][0] &&
+			    val2 == kx022a_accel_samp_freq_table[n][1])
+				break;
+		if (n < 0) {
+			ret = -EINVAL;
+			goto unlock_out;
+		}
+		ret = kx022a_turn_off_lock(data);
+		if (ret)
+			break;
+
+		ret = regmap_update_bits(data->regmap,
+					 KX022A_REG_ODCNTL,
+					 KX022A_MASK_ODR, n);
+		data->odr_ns = kx022a_odrs[n];
+		kx022a_turn_on_unlock(data);
+		break;
+	case IIO_CHAN_INFO_SCALE:
+		n = ARRAY_SIZE(kx022a_scale_table);
+
+		while (n-- > 0)
+			if (val == kx022a_scale_table[n][0] &&
+			    val2 == kx022a_scale_table[n][1])
+				break;
+		if (n < 0) {
+			ret = -EINVAL;
+			goto unlock_out;
+		}
+
+		ret = kx022a_turn_off_lock(data);
+		if (ret)
+			break;
+
+		ret = regmap_update_bits(data->regmap, KX022A_REG_CNTL,
+					 KX022A_MASK_GSEL,
+					 n << KX022A_GSEL_SHIFT);
+		kx022a_turn_on_unlock(data);
+		break;
+	default:
+		ret = -EINVAL;
+		break;
+	}
+
+unlock_out:
+	iio_device_release_direct_mode(idev);
+
+	return ret;
+}
+
+static int kx022a_fifo_set_wmi(struct kx022a_data *data)
+{
+	u8 threshold;
+
+	threshold = data->watermark;
+
+	return regmap_update_bits(data->regmap, KX022A_REG_BUF_CNTL1,
+				  KX022A_MASK_WM_TH, threshold);
+}
+
+static int kx022a_get_axis(struct kx022a_data *data,
+			   struct iio_chan_spec const *chan,
+			   int *val)
+{
+	int ret;
+
+	ret = regmap_bulk_read(data->regmap, chan->address, &data->buffer[0],
+			       sizeof(__le16));
+	if (ret)
+		return ret;
+
+	*val = le16_to_cpu(data->buffer[0]);
+
+	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;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		ret = iio_device_claim_direct_mode(idev);
+		if (ret)
+			return ret;
+
+		mutex_lock(&data->mutex);
+		ret = kx022a_get_axis(data, chan, val);
+		mutex_unlock(&data->mutex);
+
+		iio_device_release_direct_mode(idev);
+
+		return ret;
+
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		ret = regmap_read(data->regmap, KX022A_REG_ODCNTL, &regval);
+		if (ret)
+			return ret;
+
+		if ((regval & KX022A_MASK_ODR) >
+		    ARRAY_SIZE(kx022a_accel_samp_freq_table)) {
+			dev_err(data->dev, "Invalid ODR\n");
+			return -EINVAL;
+		}
+
+		kx022a_reg2freq(regval, val, val2);
+
+		return IIO_VAL_INT_PLUS_MICRO;
+
+	case IIO_CHAN_INFO_SCALE:
+		ret = regmap_read(data->regmap, KX022A_REG_CNTL, &regval);
+		if (ret < 0)
+			return ret;
+
+		kx022a_reg2scale(regval, val, val2);
+
+		return IIO_VAL_INT_PLUS_MICRO;
+	}
+
+	return -EINVAL;
+};
+
+static int kx022a_validate_trigger(struct iio_dev *idev,
+				   struct iio_trigger *trig)
+{
+	struct kx022a_data *data = iio_priv(idev);
+
+	if (data->trig != trig)
+		return -EINVAL;
+
+	return 0;
+}
+
+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 hwfifo_enabled_show(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 sysfs_emit(buf, "%d\n", state);
+}
+
+static ssize_t hwfifo_watermark_show(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 sysfs_emit(buf, "%d\n", wm);
+}
+
+static IIO_DEVICE_ATTR_RO(hwfifo_enabled, 0);
+static IIO_DEVICE_ATTR_RO(hwfifo_watermark, 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.
+	 *
+	 * We don't need to protect the timestamp as long as we are only
+	 * called from fifo-disable where we can guarantee the sensor is not
+	 * triggering interrupts and where the mutex is locked to prevent the
+	 * user-space access.
+	 */
+	data->timestamp = 0;
+
+	return regmap_write(data->regmap, KX022A_REG_BUF_CLEAR, 0x0);
+}
+
+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);
+	u16 buffer[KX022A_FIFO_LENGTH * 3];
+	uint64_t sample_period;
+	int count, fifo_bytes;
+	bool renable = false;
+	int64_t tstamp;
+	int ret, i;
+
+	ret = regmap_read(data->regmap, KX022A_REG_BUF_STATUS_1, &fifo_bytes);
+	if (ret) {
+		dev_err(dev, "Error reading buffer status\n");
+		return ret;
+	}
+
+	/* Let's not overflow if we for some reason get bogus value from i2c */
+	if (fifo_bytes == KX022A_FIFO_FULL_VALUE)
+		fifo_bytes = KX022A_FIFO_MAX_BYTES;
+
+	if (fifo_bytes % KX022A_FIFO_SAMPLES_SIZE_BYTES)
+		dev_warn(data->dev, "Bad FIFO alignment. Data may be corrupt\n");
+
+	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) {
+		/*
+		 * We need to have the IRQ disabled or we risk of messing-up
+		 * the timestamps. If we are ran from IRQ, then the
+		 * IRQF_ONESHOT has us covered - but if we are ran by the
+		 * user-space read we need to disable the IRQ to be on a safe
+		 * side. We do this usng synchronous disable so that if the
+		 * IRQ thread is being ran on other CPU we wait for it to be
+		 * finished.
+		 */
+		disable_irq(data->irq);
+		renable = true;
+
+		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;
+	}
+
+	fifo_bytes = count * KX022A_FIFO_SAMPLES_SIZE_BYTES;
+	ret = regmap_noinc_read(data->regmap, KX022A_REG_BUF_READ,
+				buffer, fifo_bytes);
+	if (ret)
+		goto renable_out;
+
+	for (i = 0; i < count; i++) {
+		u16 *sam = &buffer[i * 3];
+		__le16 *chs;
+		int bit;
+
+		chs = &data->scan.channels[0];
+		for_each_set_bit(bit, idev->active_scan_mask, AXIS_MAX)
+			memcpy(&chs[bit], &sam[bit], sizeof(*chs));
+
+		iio_push_to_buffers_with_timestamp(idev, &data->scan, tstamp);
+
+		tstamp += sample_period;
+	}
+
+	ret = count;
+
+renable_out:
+	if (renable)
+		enable_irq(data->irq);
+
+	return ret;
+}
+
+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,
+	.read_avail = &kx022a_read_avail,
+
+	.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)
+		return regmap_set_bits(data->regmap, KX022A_REG_CNTL,
+				       KX022A_MASK_DRDY);
+
+	return regmap_clear_bits(data->regmap, KX022A_REG_CNTL,
+				 KX022A_MASK_DRDY);
+}
+
+static int kx022a_prepare_irq_pin(struct kx022a_data *data)
+{
+	/* Enable IRQ1 pin. Set polarity to active low */
+	int mask = KX022A_MASK_IEN | KX022A_MASK_IPOL |
+		   KX022A_MASK_ITYP;
+	int val = KX022A_MASK_IEN | KX022A_IPOL_LOW |
+		  KX022A_ITYP_LEVEL;
+	int ret;
+
+	ret = regmap_update_bits(data->regmap, data->inc_reg, mask, val);
+	if (ret)
+		return ret;
+
+	/* We enable WMI to IRQ pin only at buffer_enable */
+	mask = KX022A_MASK_INS2_DRDY;
+
+	return regmap_set_bits(data->regmap, data->ien_reg, mask);
+}
+
+static int kx022a_fifo_disable(struct kx022a_data *data)
+{
+	int ret = 0;
+
+	ret = kx022a_turn_off_lock(data);
+	if (ret)
+		return ret;
+

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

+	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);
+}
+
+static int kx022a_fifo_enable(struct kx022a_data *data)
+{
+	int ret;
+
+	ret = kx022a_turn_off_lock(data);
+	if (ret)
+		return ret;
+
+	/* Update watermark to HW */
+	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, data->ien_reg,
+			      KX022A_MASK_WMI);
+	if (ret)
+		goto unlock_out;
+
+	return kx022a_turn_on_unlock(data);
+
+unlock_out:
+	mutex_unlock(&data->mutex);
+
+	return ret;
+}
+
+static int kx022a_buffer_postenable(struct iio_dev *idev)
+{
+	struct kx022a_data *data = iio_priv(idev);
+
+	/*
+	 * If we use data-ready trigger, then the IRQ masks should be handled by
+	 * trigger enable and the hardware buffer is not used but we just update
+	 * results to the IIO fifo when data-ready triggers.
+	 */
+	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;
+
+	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:
+	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);
+
+	data->old_timestamp = data->timestamp;
+	data->timestamp = iio_get_time_ns(idev);
+
+	if (data->state & KX022A_STATE_FIFO || data->trigger_enabled)
+		return IRQ_WAKE_THREAD;
+
+	return IRQ_NONE;
+}
+
+/*

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

+	mutex_lock(&data->mutex);
+
+	if (data->trigger_enabled) {
+		iio_trigger_poll_chained(data->trig);
+		ret = IRQ_HANDLED;
+	}
+
+	if (data->state & KX022A_STATE_FIFO) {
+		int ok;
+
+		ok = __kx022a_fifo_flush(idev, KX022A_FIFO_LENGTH, true);
+		if (ok > 0)
+			ret = IRQ_HANDLED;
+	}
+
+	mutex_unlock(&data->mutex);
+
+	return ret;
+}
+
+static int kx022a_trigger_set_state(struct iio_trigger *trig,
+				    bool state)
+{
+	struct kx022a_data *data = iio_trigger_get_drvdata(trig);
+	int ret = 0;
+
+	mutex_lock(&data->mutex);
+
+	if (data->trigger_enabled == state)
+		goto unlock_out;
+
+	if (data->state & KX022A_STATE_FIFO) {
+		dev_warn(data->dev, "Can't set trigger when FIFO enabled\n");
+		ret = -EBUSY;
+		goto unlock_out;
+	}
+
+	ret = kx022a_turn_on_off_unlocked(data, false);
+	if (ret)
+		goto unlock_out;
+
+	data->trigger_enabled = state;
+	ret = kx022a_set_drdy_irq(data, state);
+	if (ret)
+		goto unlock_out;
+
+	ret = kx022a_turn_on_off_unlocked(data, true);
+
+unlock_out:
+	mutex_unlock(&data->mutex);
+
+	return ret;
+}
+
+static const struct iio_trigger_ops kx022a_trigger_ops = {
+	.set_trigger_state = kx022a_trigger_set_state,
+};
+
+static int kx022a_chip_init(struct kx022a_data *data)
+{
+	int ret, val;
+
+	/* Reset the senor */
+	ret = regmap_write(data->regmap, KX022A_REG_CNTL2, KX022A_MASK_SRST);
+	if (ret)
+		return ret;
+
+	/*
+	 * I've seen I2C read failures if we poll too fast after the sensor
+	 * reset. Slight delay gives I2C block the time to recover.
+	 */
+	msleep(1);
+
+	ret = regmap_read_poll_timeout(data->regmap, KX022A_REG_CNTL2, val,
+				       !(val & KX022A_MASK_SRST),
+				       KX022A_SOFT_RESET_WAIT_TIME_US,
+				       KX022A_SOFT_RESET_TOTAL_WAIT_TIME_US);
+	if (ret) {
+		dev_err(data->dev, "Sensor reset %s\n",
+			val & KX022A_MASK_SRST ? "timeout" : "fail#");
+		return ret;
+	}
+
+	ret = regmap_reinit_cache(data->regmap, &kx022a_regmap);
+	if (ret) {
+		dev_err(data->dev, "Failed to reinit reg cache\n");
+		return ret;
+	}
+
+	/* set data res 16bit */
+	ret = regmap_set_bits(data->regmap, KX022A_REG_BUF_CNTL2,
+			      KX022A_MASK_BRES16);
+	if (ret) {
+		dev_err(data->dev, "Failed to set data resolution\n");
+		return ret;
+	}
+
+	return kx022a_prepare_irq_pin(data);
+}
+
+int kx022a_probe_internal(struct device *dev)
+{
+	static const char * const regulator_names[] = {"io-vdd", "vdd"};
+	struct iio_trigger *indio_trig;
+	struct fwnode_handle *fwnode;
+	struct kx022a_data *data;
+	struct regmap *regmap;
+	unsigned int chip_id;
+	struct iio_dev *idev;
+	int ret, irq;
+	char *name;
+
+	regmap = dev_get_regmap(dev, NULL);
+	if (!regmap) {
+		dev_err(dev, "no regmap\n");
+		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 and
+	 * 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)

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

+	irq = fwnode_irq_get_byname(fwnode, "INT1");
+	if (irq > 0) {
+		data->inc_reg = KX022A_REG_INC1;
+		data->ien_reg = KX022A_REG_INC4;
+
+		if (fwnode_irq_get_byname(fwnode, "INT2") > 0)
+			dev_warn(dev, "Only one IRQ supported\n");
+	} else {
+		irq = fwnode_irq_get_byname(fwnode, "INT2");
+		if (irq <= 0)
+			return dev_err_probe(dev, irq, "No suitable IRQ\n");
+
+		data->inc_reg = KX022A_REG_INC5;
+		data->ien_reg = KX022A_REG_INC6;
+	}
+
+	data->regmap = regmap;
+	data->dev = dev;
+	data->irq = irq;
+	data->odr_ns = KX022A_DEFAULT_PERIOD_NS;
+	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;

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

+	udelay(100);
+	ret = devm_iio_triggered_buffer_setup_ext(dev, idev,
+						  &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\n");
+	indio_trig = devm_iio_trigger_alloc(dev, "%sdata-rdy-dev%d", idev->name,
+					    iio_device_id(idev));
+	if (!indio_trig)
+		return -ENOMEM;
+
+	data->trig = indio_trig;
+
+	indio_trig->ops = &kx022a_trigger_ops;
+	iio_trigger_set_drvdata(indio_trig, data);
+
+	ret = devm_iio_trigger_register(dev, indio_trig);
+	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");
+
+	/*
+	 * No need to check for NULL. request_threadedI_irq() defaults to
+	 * dev_name() should the alloc fail.
+	 */
+	name = devm_kasprintf(data->dev, GFP_KERNEL, "%s-kx022a",
+			      dev_name(data->dev));
+
+	ret = devm_request_threaded_irq(data->dev, irq, kx022a_irq_handler,
+					&kx022a_irq_thread_handler,
+					IRQF_ONESHOT, name, idev);
+	if (ret)
+		return dev_err_probe(data->dev, ret, "Could not request IRQ\n");
+
+	return ret;
+}
+EXPORT_SYMBOL_NS_GPL(kx022a_probe_internal, KIONIX_ACCEL);
+
+MODULE_DESCRIPTION("ROHM/Kionix KX022A accelerometer driver");
+MODULE_AUTHOR("Matti Vaittinen <[email protected]>");
+MODULE_LICENSE("GPL");

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

new file mode 100644
index 000000000000..12424649d438
@@ -0,0 +1,82 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * Copyright (C) 2022 ROHM Semiconductors
+ *
+ * ROHM/KIONIX KX022A accelerometer driver
+ */
+
+#ifndef _KX022A_H_
+#define _KX022A_H_
+
+#include <linux/bits.h>
+#include <linux/regmap.h>
+
+#define KX022A_REG_WHO		0x0f
+#define KX022A_ID		0xc8
+
+#define KX022A_REG_CNTL2	0x19
+#define KX022A_MASK_SRST	BIT(7)
+#define KX022A_REG_CNTL		0x18
+#define KX022A_MASK_PC1		BIT(7)
+#define KX022A_MASK_RES		BIT(6)
+#define KX022A_MASK_DRDY	BIT(5)
+#define KX022A_MASK_GSEL	GENMASK(4, 3)
+#define KX022A_GSEL_SHIFT	3
+#define KX022A_GSEL_2		0x0
+#define KX022A_GSEL_4		BIT(3)
+#define KX022A_GSEL_8		BIT(4)
+#define KX022A_GSEL_16		GENMASK(4, 3)
+
+#define KX022A_REG_INS2		0x13
+#define KX022A_MASK_INS2_DRDY	BIT(4)
+#define KX122_MASK_INS2_WMI	BIT(5)
+
+#define KX022A_REG_XHP_L	0x0
+#define KX022A_REG_XOUT_L	0x06
+#define KX022A_REG_YOUT_L	0x08
+#define KX022A_REG_ZOUT_L	0x0a
+#define KX022A_REG_COTR		0x0c
+#define KX022A_REG_TSCP		0x10
+#define KX022A_REG_INT_REL	0x17
+
+#define KX022A_REG_ODCNTL	0x1b
+
+#define KX022A_REG_BTS_WUF_TH	0x31
+#define KX022A_REG_MAN_WAKE	0x2c
+
+#define KX022A_REG_BUF_CNTL1	0x3a
+#define KX022A_MASK_WM_TH	GENMASK(6, 0)
+#define KX022A_REG_BUF_CNTL2	0x3b
+#define KX022A_MASK_BUF_EN	BIT(7)
+#define KX022A_MASK_BRES16	BIT(6)
+#define KX022A_REG_BUF_STATUS_1	0x3c
+#define KX022A_REG_BUF_STATUS_2	0x3d
+#define KX022A_REG_BUF_CLEAR	0x3e
+#define KX022A_REG_BUF_READ	0x3f
+#define KX022A_MASK_ODR		GENMASK(3, 0)
+#define KX022A_ODR_SHIFT	3
+#define KX022A_FIFO_MAX_WMI_TH	41
+
+#define KX022A_REG_INC1		0x1c
+#define KX022A_REG_INC5		0x20
+#define KX022A_REG_INC6		0x21
+#define KX022A_MASK_IEN		BIT(5)
+#define KX022A_MASK_IPOL	BIT(4)
+#define KX022A_IPOL_LOW		0
+#define KX022A_IPOL_HIGH	KX022A_MASK_IPOL1
+#define KX022A_MASK_ITYP	BIT(3)
+#define KX022A_ITYP_PULSE	KX022A_MASK_ITYP
+#define KX022A_ITYP_LEVEL	0
+
+#define KX022A_REG_INC4		0x1f
+#define KX022A_MASK_WMI		BIT(5)
+
+#define KX022A_REG_SELF_TEST	0x60
+#define KX022A_MAX_REGISTER	0x60
+
+struct device;
+
+int kx022a_probe_internal(struct device *dev);
+extern const struct regmap_config kx022a_regmap;
+
+#endif

Signature · 1 line

2.37.3

Quoted 9 lines · Matti Vaittinen

KX022A is a 3-axis accelerometer from ROHM/Kionix. The sensor 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 features.

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).

Quoted 2 lines

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

While I have some disagreements on some code
pieces, this version is okay to go I think.

Reviewed-by Andy Shevchenko <[email protected]>

Below a few nit-picks in case it needs to be a v5.

KX022A is a 3-axis accelerometer from ROHM/Kionix. The sensor 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 features.

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.

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

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

+{								\
+	.type = IIO_ACCEL,					\
+	.modified = 1,						\
+	.channel2 = IIO_MOD_##axis,				\
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),		\
+	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) |	\
+				BIT(IIO_CHAN_INFO_SAMP_FREQ),	\
+	.info_mask_shared_by_type_available =			\
+				BIT(IIO_CHAN_INFO_SCALE) |	\
+				BIT(IIO_CHAN_INFO_SAMP_FREQ),	\
+	.ext_info = kx022a_ext_info,				\
+	.address = KX022A_REG_##axis##OUT_L,			\
+	.scan_index = index,					\
+	.scan_type = {                                          \
+		.sign = 's',					\
+		.realbits = 16,					\
+		.storagebits = 16,				\
+		.endianness = IIO_LE,				\
+	},							\
+}

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

+
+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);
+	u16 buffer[KX022A_FIFO_LENGTH * 3];
+	uint64_t sample_period;
+	int count, fifo_bytes;
+	bool renable = false;
+	int64_t tstamp;
+	int ret, i;
+
+	ret = regmap_read(data->regmap, KX022A_REG_BUF_STATUS_1, &fifo_bytes);
+	if (ret) {
+		dev_err(dev, "Error reading buffer status\n");
+		return ret;
+	}
+
+	/* Let's not overflow if we for some reason get bogus value from i2c */
+	if (fifo_bytes == KX022A_FIFO_FULL_VALUE)
+		fifo_bytes = KX022A_FIFO_MAX_BYTES;
+
+	if (fifo_bytes % KX022A_FIFO_SAMPLES_SIZE_BYTES)
+		dev_warn(data->dev, "Bad FIFO alignment. Data may be corrupt\n");
+
+	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) {
+		/*
+		 * We need to have the IRQ disabled or we risk of messing-up
+		 * the timestamps. If we are ran from IRQ, then the
+		 * IRQF_ONESHOT has us covered - but if we are ran by the
+		 * user-space read we need to disable the IRQ to be on a safe
+		 * side. We do this usng synchronous disable so that if the
+		 * IRQ thread is being ran on other CPU we wait for it to be
+		 * finished.
+		 */
+		disable_irq(data->irq);
+		renable = true;
+
+		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;
+	}
+
+	fifo_bytes = count * KX022A_FIFO_SAMPLES_SIZE_BYTES;
+	ret = regmap_noinc_read(data->regmap, KX022A_REG_BUF_READ,
+				buffer, fifo_bytes);
+	if (ret)

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

+
+		iio_push_to_buffers_with_timestamp(idev, &data->scan, tstamp);
+
+		tstamp += sample_period;
+	}
+
+	ret = count;
+
+renable_out:
+	if (renable)
+		enable_irq(data->irq);
+
+	return ret;
+}
 
 
+int kx022a_probe_internal(struct device *dev)
+{
+	static const char * const regulator_names[] = {"io-vdd", "vdd"};
+	struct iio_trigger *indio_trig;
+	struct fwnode_handle *fwnode;
+	struct kx022a_data *data;
+	struct regmap *regmap;
+	unsigned int chip_id;
+	struct iio_dev *idev;
+	int ret, irq;
+	char *name;
+
+	regmap = dev_get_regmap(dev, NULL);
+	if (!regmap) {
+		dev_err(dev, "no regmap\n");
+		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 and
+	 * 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)
+		return dev_err_probe(dev, ret, "Failed to access sensor\n");
+
+	if (chip_id != KX022A_ID) {
+		dev_err(dev, "unsupported device 0x%x\n", chip_id);
+		return -EINVAL;
+	}
+
+	fwnode = dev_fwnode(dev);

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

+	} else {
+		irq = fwnode_irq_get_byname(fwnode, "INT2");
+		if (irq <= 0)
+			return dev_err_probe(dev, irq, "No suitable IRQ\n");
+
+		data->inc_reg = KX022A_REG_INC5;
+		data->ien_reg = KX022A_REG_INC6;
+	}
+
+	data->regmap = regmap;
+	data->dev = dev;
+	data->irq = irq;
+	data->odr_ns = KX022A_DEFAULT_PERIOD_NS;
+	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);
+	if (ret) {
+		mutex_unlock(&data->mutex);
+		return ret;
+	}
+
+	ret = kx022a_turn_on_unlock(data);
+	if (ret)
+		return ret;
+
+	udelay(100);
+	ret = devm_iio_triggered_buffer_setup_ext(dev, idev,
+						  &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\n");
+	indio_trig = devm_iio_trigger_alloc(dev, "%sdata-rdy-dev%d", idev->name,
+					    iio_device_id(idev));
+	if (!indio_trig)
+		return -ENOMEM;
+
+	data->trig = indio_trig;
+
+	indio_trig->ops = &kx022a_trigger_ops;
+	iio_trigger_set_drvdata(indio_trig, data);
+
+	ret = devm_iio_trigger_register(dev, indio_trig);
+	if (ret)

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

+	if (ret)
+		return dev_err_probe(data->dev, ret, "Could not request IRQ\n");
+
+	return ret;
+}
+EXPORT_SYMBOL_NS_GPL(kx022a_probe_internal, KIONIX_ACCEL);
+
+MODULE_DESCRIPTION("ROHM/Kionix KX022A accelerometer driver");
+MODULE_AUTHOR("Matti Vaittinen <[email protected]>");
+MODULE_LICENSE("GPL");

Quoted 5 lines · On Fri, 21 Oct 2022 14:22:49 +0300 Matti Vaittinen <[email protected]>…

KX022A is a 3-axis accelerometer from ROHM/Kionix. The sensor 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 features.

Quoted 8 lines

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.

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

Hi Matti,

A few things from me on this read through to add to those last few comments from Andy.

Thanks,

Jonathan

Quoted 6 lines

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

new file mode 100644
index 000000000000..6510f8d62b85
@@ -0,0 +1,51 @@

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

Quoted 5 lines

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

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

Not sure why I didn't noticed this before, but after various debates we ended up prefixing namespaces to limit them to IIO. Also, there are other kionix drivers already and may be more in future. Better to limit this to scope of this driver,

IIO_KX022A >
seems like a reaonsable choice.

Quoted 48 lines

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

+int kx022a_probe_internal(struct device *dev)
+{
+	static const char * const regulator_names[] = {"io-vdd", "vdd"};
+	struct iio_trigger *indio_trig;
+	struct fwnode_handle *fwnode;
+	struct kx022a_data *data;
+	struct regmap *regmap;
+	unsigned int chip_id;
+	struct iio_dev *idev;
+	int ret, irq;
+	char *name;
+
+	regmap = dev_get_regmap(dev, NULL);
+	if (!regmap) {
+		dev_err(dev, "no regmap\n");
+		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 and
+	 * 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)
+		return dev_err_probe(dev, ret, "Failed to access sensor\n");
+
+	if (chip_id != KX022A_ID) {
+		dev_err(dev, "unsupported device 0x%x\n", chip_id);
+		return -EINVAL;
+	}
+
+	fwnode = dev_fwnode(dev);
+	if (!fwnode)
+		return -ENODEV;
+
+	irq = fwnode_irq_get_byname(fwnode, "INT1");
+	if (irq > 0) {

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

Quoted 5 lines

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

+		data->inc_reg = KX022A_REG_INC1;
+		data->ien_reg = KX022A_REG_INC4;
+
+		if (fwnode_irq_get_byname(fwnode, "INT2") > 0)
+			dev_warn(dev, "Only one IRQ supported\n");

Why? If you get the both, only the first one will be used by the driver. Not really worth warning about the lack of features...

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

Quoted 7 lines

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

+	ret = devm_iio_device_register(data->dev, idev);
+	if (ret < 0)
+		return dev_err_probe(dev, ret,
+				     "Unable to register iio device\n");
+
+	/*
+	 * No need to check for NULL. request_threadedI_irq() defaults to

Why I?

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

Quoted 8 lines

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

+	 * dev_name() should the alloc fail.
+	 */
+	name = devm_kasprintf(data->dev, GFP_KERNEL, "%s-kx022a",
+			      dev_name(data->dev));
+
+	ret = devm_request_threaded_irq(data->dev, irq, kx022a_irq_handler,
+					&kx022a_irq_thread_handler,
+					IRQF_ONESHOT, name, idev);

You are requesting the irq 'after' exposing the userspace interfaces. Technically that potentially introduces a race as we might in theory successfully trigger an interrupt before requesting it.

Obviously that would be challenging to pull off given likely short window, but good to close it anyway! Rule of thumb is that devm_iio_device_register() is always the last thing to call in problem. The very rare exceptions need a comment to explain why they are there...

Note that the devm_iio_trigger_register() also exposes the userspace part of the trigger so if you allow that to be used by other drivers you'd need the irq registration before that as well.

Good Morning Jonathan,

Quoted 34 lines · Jonathan Cameron

Quoted 5 lines · On Fri, 21 Oct 2022 14:22:49 +0300 Matti Vaittinen <[email protected]>…

KX022A is a 3-axis accelerometer from ROHM/Kionix. The sensor 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 features.

Quoted 8 lines

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.

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

Hi Matti,

A few things from me on this read through to add to those last few comments from Andy.

Thanks,

Jonathan

Quoted 6 lines

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

new file mode 100644
index 000000000000..6510f8d62b85
@@ -0,0 +1,51 @@  

Quoted 9 lines

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

Quoted 5 lines

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

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

Not sure why I didn't noticed this before, but after various debates we ended up prefixing namespaces to limit them to IIO. Also, there are other kionix drivers already and may be more in future. Better to limit this to scope of this driver,

Prefixing with IIO_ is all fine. And I was thinking of using the KX022A but ended up with KIONIX as I am pretty sure this same driver will be extended to cover the old 022 (without A) and probably also 122 (at least)...

...After that being said - I have been doing this same naming by part-name with ROHM PMIC drivers. As an example, first IC I was working with after jumping from Nokia to ROHM was BD71837. After the driver was written I encountered BD71847 - which was almost identical but just omitted few BUCKs. So, I renamed driver to BD718x7 and added the support.

As you can probably guess, this was not the end of the story. Next came BD71850 - which is pretty much the BD71847 with different OTP. I think I used some defines with BD718XX_...

...and later I did drivers for BD71815, BD71827, BD71828 ... - which all were completely different from the ones supported by bd718x7 driver.

What I learned was - either use the exact part name in file/defines (even when supporting many ICs) or use completely abstract name with no part name. It seems to be impossible to do correct wildcards unless you can dictate the IC naming :)

Quoted 5 lines

IIO_KX022A >
seems like a reaonsable choice.

Long babbling just to say: "Yes".
We can use IIO_KX022A even if we support many ICs. I don't plan to change the file/function/define naming when adding support to new ICs :)

Quoted 49 lines

Quoted 49 lines

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

+int kx022a_probe_internal(struct device *dev)
+{
+	static const char * const regulator_names[] = {"io-vdd", "vdd"};
+	struct iio_trigger *indio_trig;
+	struct fwnode_handle *fwnode;
+	struct kx022a_data *data;
+	struct regmap *regmap;
+	unsigned int chip_id;
+	struct iio_dev *idev;
+	int ret, irq;
+	char *name;
+
+	regmap = dev_get_regmap(dev, NULL);
+	if (!regmap) {
+		dev_err(dev, "no regmap\n");
+		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 and
+	 * 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)
+		return dev_err_probe(dev, ret, "Failed to access sensor\n");
+
+	if (chip_id != KX022A_ID) {
+		dev_err(dev, "unsupported device 0x%x\n", chip_id);
+		return -EINVAL;
+	}
+
+	fwnode = dev_fwnode(dev);
+	if (!fwnode)
+		return -ENODEV;
+
+	irq = fwnode_irq_get_byname(fwnode, "INT1");
+	if (irq > 0) {

Quoted 8 lines

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

Quoted 5 lines

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

+		data->inc_reg = KX022A_REG_INC1;
+		data->ien_reg = KX022A_REG_INC4;
+
+		if (fwnode_irq_get_byname(fwnode, "INT2") > 0)
+			dev_warn(dev, "Only one IRQ supported\n");  

Why? If you get the both, only the first one will be used by the driver. Not really worth warning about the lack of features...

My thinking regarding developing new device went along the lines:

Precondition: The HW (and data-sheet) explain how there is two INT pins.
1. Board designer reads the data-sheet and uses both INT pins.
2. SW engineer finds the driver and reads the DT-binding description.
3. SW engineer writes the DT-description and hopes everything "just works". (Amount of hope is probably inversely proportional to the amount of experience XD).
4) SW engineer gives a first go to the sensor SW and notices everything does not just magically work.

I think there is important difference between following options:
4a) Log shows print "Only one IRQ supported" from the kx022a driver 4b) Log shows no errors.

The amount of work potentially avoided by the check here is probably again inversely proportional to the SW engineer's experience - but I think it may be significant. OTOH, the amount of problems caused by this check is (in my opinion) negligible. Well, the additional print string does eat up few bytes of space - and I know people/systems that do not like this. I am still tempted to say that those systems can opt to disable the config for this driver though.

Well, this is not a point I would like to start fighting over though. If this email does not convince the audience about usefulness of the print
- then I'll just drop it from the v5 :)

Quoted 10 lines

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

Quoted 8 lines

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

+	ret = devm_iio_device_register(data->dev, idev);
+	if (ret < 0)
+		return dev_err_probe(dev, ret,
+				     "Unable to register iio device\n");
+
+	/*
+	 * No need to check for NULL. request_threadedI_irq() defaults to  

Why I?

NoI_idea(). I'll fix this, thanks! :)

Quoted 13 lines

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

Quoted 8 lines

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

+	 * dev_name() should the alloc fail.
+	 */
+	name = devm_kasprintf(data->dev, GFP_KERNEL, "%s-kx022a",
+			      dev_name(data->dev));
+
+	ret = devm_request_threaded_irq(data->dev, irq, kx022a_irq_handler,
+					&kx022a_irq_thread_handler,
+					IRQF_ONESHOT, name, idev);  

You are requesting the irq 'after' exposing the userspace interfaces. Technically that potentially introduces a race as we might in theory successfully trigger an interrupt before requesting it.

Quoted 9 lines

Obviously that would be challenging to pull off given likely short window, but good to close it anyway! Rule of thumb is that devm_iio_device_register() is always the last thing to call in problem. The very rare exceptions need a comment to explain why they are there...

Note that the devm_iio_trigger_register() also exposes the userspace part of the trigger so if you allow that to be used by other drivers you'd need the irq registration before that as well.

Thanks! I did overlook this. In a few cases handling the IRQs use subsystem facilities established during driver registration. In those cases the dependencies and the race goes the other way around. Thus it is almost a habit for me to register IRQs as last thing and purge them as first thing.

I'll cook-up the v5 soon-ish :)

Yours
-- Matti Vaittinen

Quoted 9 lines · Matti

Quoted 6 lines

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

Quoted 3 lines

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

+
+		if (fwnode_irq_get_byname(fwnode, "INT2") > 0)
+			dev_warn(dev, "Only one IRQ supported\n");

Why? If you get the both, only the first one will be used by the driver. Not really worth warning about the lack of features...

My thinking regarding developing new device went along the lines:

Quoted 8 lines

Precondition: The HW (and data-sheet) explain how there is two INT pins.
1. Board designer reads the data-sheet and uses both INT pins.
2. SW engineer finds the driver and reads the DT-binding description.
3. SW engineer writes the DT-description and hopes everything "just works". (Amount of hope is probably inversely proportional to the amount of experience XD).
4) SW engineer gives a first go to the sensor SW and notices everything does not just magically work.

Ah but it does "work". We simply don't use one of the IRQs. That's not normally a problem as there are lots of other features we don't fully support. Not using something is not normally considered a problem.

So far I'm not seeing anything that doesn't work because we only support one IRQ. There may be combinations of interrupts that are tricky to handle on one IRQ line (or may not be supported at the same time on a single line) but so far you don't support those anyway.. Adding a more informative warning when adding those features would be reasonable

"Feature X not supported as only a single IRQ line available".

KX022A is a 3-axis accelerometer from ROHM/Kionix. The sensor 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 features.

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.

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

---

+/*
+ * The KX022A has FIFO which can store 43 samples of HiRes data from 2

+ * channels. This equals to 43 (samples) * 3 (channels) * 2 (bytes/sample) to
+ * 258 bytes of sample data. The quirk to know is that the amount of bytes in
+ * the FIFO is advertised via 8 bit register (max value 255). The thing to note
+ * is that full 258 bytes of data is indicated using the max value 255.
+ */

Le 21/10/2022 à 13:22, Matti Vaittinen a écrit :

Quoted 9 lines

KX022A is a 3-axis accelerometer from ROHM/Kionix. The sensor 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 features.

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).

Quoted 6 lines

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

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

---

Hi, should there be a v5:

Add maintainer entry for ROHM/Kionix KX022A accelerometer sensor driver.

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

---
 MAINTAINERS | 5 +++++
 1 file changed, 5 insertions(+)

MAINTAINERS +0 -0

index cf0f18502372..3ab9c5f97dfe 100644

MAINTAINERS +1 -0

+
 KMEMLEAK
 M:	Catalin Marinas <[email protected]>
 S:	Maintained

Signature · 1 line

2.37.3

Quoted 15 lines · Matti Vaittinen

Add maintainer entry for ROHM/Kionix KX022A accelerometer sensor driver.

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

---
  MAINTAINERS | 5 +++++
  1 file changed, 5 insertions(+)

MAINTAINERS +0 -0

index cf0f18502372..3ab9c5f97dfe 100644
@@ -11435,6 +11435,11 @@ F:	drivers/mfd/khadas-mcu.c
  F:	include/linux/mfd/khadas-mcu.h
  F:	drivers/thermal/khadas_mcu_fan.c
 

MAINTAINERS F: drivers/mfd/khadas-mcu.c · line 11441

Quoted 4 lines

MAINTAINERS +4 -0

+KIONIX/ROHM KX022A ACCELEROMETER
+R:	Matti Vaittinen <[email protected]>
+S:	Supported
+F:	drivers/iio/accel/kionix-kx022a*

How is this "S: Supported" without an M: maintainer?

Quoted 9 lines

I can also add myself as a maintainer instead of a reviewer if it better suits iio maintainer. I however don't plan setting up my own public repository and hope the further patches will be merged via IIO tree.

So, as Geert once explained to me - In that case the difference between me as a maintainer vs. a reviewer would be only really relevant to the subsystem (in this case IIO) maintainer. The subsystem maintainer who merges patches is allowed to take in changes acked by downstream maintainer w/o obligation to do thorough review. (Downstream

Quoted 5 lines

to be blamed if things explode :]). If ack is given by a reviewer, then the subsystem maintainer has the full responsibility and should always do the review. Or - this is how I remember our discussion went - feel free to correct me if I am wrong :] In any case - please let me know if you'd rather see M: not R: in front of my name for the kx022a.

Hi Joe,

Quoted 16 lines · Joe Perches

Quoted 15 lines · Matti Vaittinen

Add maintainer entry for ROHM/Kionix KX022A accelerometer sensor driver.

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

---
  MAINTAINERS | 5 +++++
  1 file changed, 5 insertions(+)

MAINTAINERS +0 -0

index cf0f18502372..3ab9c5f97dfe 100644
@@ -11435,6 +11435,11 @@ F:	drivers/mfd/khadas-mcu.c
  F:	include/linux/mfd/khadas-mcu.h
  F:	drivers/thermal/khadas_mcu_fan.c
 

Quoted 6 lines

MAINTAINERS F: drivers/mfd/khadas-mcu.c · line 11441

Quoted 4 lines

MAINTAINERS +4 -0

+KIONIX/ROHM KX022A ACCELEROMETER
+R:	Matti Vaittinen <[email protected]>
+S:	Supported
+F:	drivers/iio/accel/kionix-kx022a*

How is this "S: Supported" without an M: maintainer?

I am currently paid to work with the Kionix/ROHM upstream drivers. Hence I add 'S:' to ones I am looking after.

The ideology why I have 'R' and not 'M' is summarized by my earlier patch:

Quoted 9 lines

Quoted 9 lines

I can also add myself as a maintainer instead of a reviewer if it better suits iio maintainer. I however don't plan setting up my own public repository and hope the further patches will be merged via IIO tree.

So, as Geert once explained to me - In that case the difference between me as a maintainer vs. a reviewer would be only really relevant to the subsystem (in this case IIO) maintainer. The subsystem maintainer who merges patches is allowed to take in changes acked by downstream maintainer w/o obligation to do thorough review. (Downstream

maintainer is

Quoted 5 lines

Quoted 5 lines

to be blamed if things explode :]). If ack is given by a reviewer, then the subsystem maintainer has the full responsibility and should always do the review. Or - this is how I remember our discussion went - feel free to correct me if I am wrong :] In any case - please let me know if you'd rather see M: not R: in front of my name for the kx022a.

This seemed to be fine with Jonathan:

https://lore.kernel.org/all/[email protected]/

I've also written a longer version of this in an LinkedIn article:
https://www.linkedin.com/pulse/should-you-linux-kernel-maintainer-matti-vaittinen/

(I enjoy writing small stories. So doing an occasional small LinkedIn articles on working with the upstream is kind of an hobby for me.)

Anyways, I don't see a contradiction with 'S + R' compared to 'S + M'. Well, please educate me if I am wrong :]

Quoted 54 lines · Matti

Hi Joe,

Quoted 16 lines · Joe Perches

Quoted 15 lines · Matti Vaittinen

Add maintainer entry for ROHM/Kionix KX022A accelerometer sensor driver.

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

---
   MAINTAINERS | 5 +++++
   1 file changed, 5 insertions(+)

MAINTAINERS +0 -0

index cf0f18502372..3ab9c5f97dfe 100644
@@ -11435,6 +11435,11 @@ F:	drivers/mfd/khadas-mcu.c
   F:	include/linux/mfd/khadas-mcu.h
   F:	drivers/thermal/khadas_mcu_fan.c
 

Quoted 6 lines

MAINTAINERS F: drivers/mfd/khadas-mcu.c · line 11441

Quoted 4 lines

MAINTAINERS +4 -0

+KIONIX/ROHM KX022A ACCELEROMETER
+R:	Matti Vaittinen <[email protected]>
+S:	Supported
+F:	drivers/iio/accel/kionix-kx022a*

How is this "S: Supported" without an M: maintainer?

I am currently paid to work with the Kionix/ROHM upstream drivers. Hence I add 'S:' to ones I am looking after.

The ideology why I have 'R' and not 'M' is summarized by my earlier patch:

Quoted 9 lines

Quoted 9 lines

I can also add myself as a maintainer instead of a reviewer if it better suits iio maintainer. I however don't plan setting up my own public repository and hope the further patches will be merged via IIO tree.

So, as Geert once explained to me - In that case the difference between me as a maintainer vs. a reviewer would be only really relevant to the subsystem (in this case IIO) maintainer. The subsystem maintainer who merges patches is allowed to take in changes acked by downstream maintainer w/o obligation to do thorough review. (Downstream

maintainer is

Quoted 5 lines

Quoted 5 lines

to be blamed if things explode :]). If ack is given by a reviewer, then the subsystem maintainer has the full responsibility and should always do the review. Or - this is how I remember our discussion went - feel free to correct me if I am wrong :] In any case - please let me know if you'd rather see M: not R: in front of my name for the kx022a.

This seemed to be fine with Jonathan:

https://lore.kernel.org/all/[email protected]/

I've also written a longer version of this in an LinkedIn article:
https://www.linkedin.com/pulse/should-you-linux-kernel-maintainer-matti-vaittinen/

Quoted 5 lines

(I enjoy writing small stories. So doing an occasional small LinkedIn articles on working with the upstream is kind of an hobby for me.)

Anyways, I don't see a contradiction with 'S + R' compared to 'S + M'. Well, please educate me if I am wrong :]

The subsystem is one thing, someone outside of KIONIX/ROHM may be supporting the subsystem. If this _particular_ driver is "supported"

there should be an individual listed as its actual maintainer, not just a person that might review submitted patches.

S: *Status*, one of the following:
Supported: Someone is actually paid to look after this. > Maintained: Someone actually looks after it.

"this" is this particular driver, not any subsystem "above" it.

On 10/24/22 13:40, Joe Perches wrote:

Quoted 5 lines

S: *Status*, one of the following:
Supported: Someone is actually paid to look after this. > Maintained: Someone actually looks after it.

"this" is this particular driver, not any subsystem "above" it.

Yes. And as I wrote, I am paid to look after this driver as well as other drivers I've submitted upstream for ROHM components (Kionix being part of ROHM these days). I have used this Supported + Reviewer combination for all other IC drivers as well. This is why, by definition, the S eg. supported is correct. Question is whether one supporting a driver must be a maintainer? If this is the case, then I'd better review all of my MAINTAINER entries. However, I (still) don't see the problem of having a reviewer supporting the IC.

Quoted 1 line · Matti

On 10/24/22 13:40, Joe Perches wrote:

[]

Quoted 6 lines

Quoted 5 lines

S: *Status*, one of the following:
Supported: Someone is actually paid to look after this. > Maintained: Someone actually looks after it.

"this" is this particular driver, not any subsystem "above" it.

Quoted 8 lines

Yes. And as I wrote, I am paid to look after this driver as well as other drivers I've submitted upstream for ROHM components (Kionix being part of ROHM these days). I have used this Supported + Reviewer combination for all other IC drivers as well. This is why, by definition, the S eg. supported is correct. Question is whether one supporting a driver must be a maintainer? If this is the case, then I'd better review all of my MAINTAINER entries. However, I (still) don't see the problem of having a reviewer supporting the IC.

Please do not conflate a "reviewer", someone that "might" look at a patch and offer comments, and a "supporter", someone that actively supports the driver/subsystem. I don't have a tree that is pulled yet I am the get_maintainer and checkpatch maintainer.