iio: accel: Add support for Kionix/ROHM KX132 accelerometer

Extend the kionix,kx022a.yaml file to support the
kx132 device

Signed-off-by Mehdi Djait <[email protected]>

---
 .../bindings/iio/accel/kionix,kx022a.yaml           | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

Documentation/devicetree/bindings/iio/accel/kionix,kx022a.yaml +1 -1

index 986df1a6ff0a..ac1e27402d5e 100644
@@ -4,19 +4,22 @@
 $id: http://devicetree.org/schemas/iio/accel/kionix,kx022a.yaml#
 $schema: http://devicetree.org/meta-schemas/core.yaml#
 
-title: ROHM/Kionix KX022A Accelerometer
+title: ROHM/Kionix KX022A and KX132 Accelerometers

Documentation/devicetree/bindings/iio/accel/kionix,kx022a.yaml +5 -1

+  buffering.
+  KX022A and KX132 can be accessed either via I2C or SPI.
 
 properties:
   compatible:
-    const: kionix,kx022a
+    enum:
+      - kionix,kx022a
+      - kionix,kx132
 
   reg:
     maxItems: 1

Quoted 4 lines · On Fri, 17 Mar 2023 00:48:35 +0100 Mehdi Djait <[email protected]> wrote:

Extend the kionix,kx022a.yaml file to support the
kx132 device

Signed-off-by Mehdi Djait <[email protected]>

Pins and power supplies etc all look the same to me so indeed seems that you have covered all that is needed. One small comment inline and I think Matti's point about more specific compatibles probably needs to be taken into account if there are known variants.

Kionix has done this for a long time. I remember that fun with the kxsd9 lots of years back - that had lots of subtle variants.

Quoted 14 lines

---
 .../bindings/iio/accel/kionix,kx022a.yaml           | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

Documentation/devicetree/bindings/iio/accel/kionix,kx022a.yaml +1 -1

index 986df1a6ff0a..ac1e27402d5e 100644
@@ -4,19 +4,22 @@
 $id: http://devicetree.org/schemas/iio/accel/kionix,kx022a.yaml#
 $schema: http://devicetree.org/meta-schemas/core.yaml#
 
-title: ROHM/Kionix KX022A Accelerometer
+title: ROHM/Kionix KX022A and KX132 Accelerometers

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

Quoted 10 lines

Documentation/devicetree/bindings/iio/accel/kionix,kx022a.yaml +2 -3

 
 maintainers:
   - Matti Vaittinen <[email protected]>
 
 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.
+  KX022A and KX132 are 3-axis accelerometers supporting +/- 2G, 4G, 8G and
+  16G ranges, output data-rates from 0.78Hz to 1600Hz and a hardware-fifo

This may be one of those 'there are many versions' of the chip issues, but the random datasheet I got via digikey (kionix website was slow and I'm impatient) has max as 25600Hz for the KX132-1211.

No particular reason the sampling rates need to be in this description so if they are different I'd just remove the mention or just say "variable output data-rates"

Refactor the kx022a driver implementation to make it more generic and extensible.
Add the chip_info structure will to the driver's private data to hold all the device specific infos.
Move the enum, struct and constants definitions to the header file.

#define KX022A_REG_WHO 0x0f
#define KX022A_ID 0xc8

Quoted 6 lines · Mehdi Djait

Refactor the kx022a driver implementation to make it more generic and extensible.
Add the chip_info structure will to the driver's private data to hold all the device specific infos.
Move the enum, struct and constants definitions to the header file.

Please, compile and test before sending.

...

drivers/iio/accel/kionix-kx022a-spi.c line 55

Quoted 5 lines

drivers/iio/accel/kionix-kx022a-spi.c +1 -1

 	.driver = {
-		.name   = "kx022a-spi",
+		.name	= "kx022a-spi",
 		.of_match_table = kx022a_of_match,
 	},

What was changed here?

...

drivers/iio/accel/kionix-kx022a-spi.c line 57

Quoted 2 lines

drivers/iio/accel/kionix-kx022a-spi.c +1 -1

-	.id_table = kx022a_id,
+	.id_table = kx022a_spi_id,

Why do we need this change?

...

drivers/iio/accel/kionix-kx022a.c int kx022a_probe_internal(struct device *dev) · line 1097

Quoted 3 lines

drivers/iio/accel/kionix-kx022a.c +1 -1

-	name = devm_kasprintf(data->dev, GFP_KERNEL, "%s-kx022a",
+	name = devm_kasprintf(data->dev, GFP_KERNEL, "%s-accel",
 			      dev_name(data->dev));

Shouldn't you use the name from chip info?

...

drivers/iio/accel/kionix-kx022a.h line 38

Quoted 3 lines

drivers/iio/accel/kionix-kx022a.h +3 -0

+#define KX_MASK_BRES16			    BIT(6)
+
+

One blank line is enough.

Refactor the kx022a driver implementation to make it more generic and extensible.
Add the chip_info structure will to the driver's private data to hold all the device specific infos.
Move the enum, struct and constants definitions to the header file.

Signed-off-by Mehdi Djait <[email protected]>

---
 drivers/iio/accel/kionix-kx022a-i2c.c |  19 +-
 drivers/iio/accel/kionix-kx022a-spi.c |  22 +-
 drivers/iio/accel/kionix-kx022a.c     | 289 ++++++++++++--------------
 drivers/iio/accel/kionix-kx022a.h     | 128 ++++++++++--
 4 files changed, 274 insertions(+), 184 deletions(-)

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

index e6fd02d931b6..21c4c0ae1a68 100644
@@ -15,23 +15,35 @@
 static int kx022a_i2c_probe(struct i2c_client *i2c)
 {
 	struct device *dev = &i2c->dev;
+	struct kx022a_chip_info *chip_info;
 	struct regmap *regmap;
+	const struct i2c_device_id *id = i2c_client_get_device_id(i2c);

drivers/iio/accel/kionix-kx022a-i2c.c +6 -1

+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, kx022a_i2c_id);
+
 static const struct of_device_id kx022a_of_match[] = {
-	{ .compatible = "kionix,kx022a", },
+	{ .compatible = "kionix,kx022a", .data = &kx_chip_info[KX022A] },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, kx022a_of_match);

@@ -42,6 +54,7 @@ static struct i2c_driver kx022a_i2c_driver = {
 		.of_match_table = kx022a_of_match,
 	  },
 	.probe_new    = kx022a_i2c_probe,
+	.id_table     = kx022a_i2c_id,
 };
 module_i2c_driver(kx022a_i2c_driver);
 

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

index 9cd047f7b346..ec076af0f261 100644
@@ -15,40 +15,46 @@
 static int kx022a_spi_probe(struct spi_device *spi)
 {
 	struct device *dev = &spi->dev;
+	struct kx022a_chip_info *chip_info;
 	struct regmap *regmap;

drivers/iio/accel/kionix-kx022a-spi.c +10 -7

+		chip_info = (const struct kx022a_chip_info *) id->driver_data;
+
+	regmap = devm_regmap_init_spi(spi, chip_info->regmap_config);
 	if (IS_ERR(regmap))
 		return dev_err_probe(dev, PTR_ERR(regmap),
 				     "Failed to initialize Regmap\n");
 
-	return kx022a_probe_internal(dev);
+	return kx022a_probe_internal(dev, chip_info);
 }
 
-static const struct spi_device_id kx022a_id[] = {
-	{ "kx022a" },
+static const struct spi_device_id kx022a_spi_id[] = {
+	{ .name = "kx022a", .driver_data = (kernel_ulong_t)&kx_chip_info[KX022A] },
 	{ }
 };
-MODULE_DEVICE_TABLE(spi, kx022a_id);
+MODULE_DEVICE_TABLE(spi, kx022a_spi_id);
 
 static const struct of_device_id kx022a_of_match[] = {
-	{ .compatible = "kionix,kx022a", },
+	{ .compatible = "kionix,kx022a", .data = &kx_chip_info[KX022A] },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, kx022a_of_match);
 
 static struct spi_driver kx022a_spi_driver = {
 	.driver = {
-		.name   = "kx022a-spi",
+		.name	= "kx022a-spi",
 		.of_match_table = kx022a_of_match,
 	},
 	.probe = kx022a_spi_probe,
-	.id_table = kx022a_id,
+	.id_table = kx022a_spi_id,
 };
 module_spi_driver(kx022a_spi_driver);
 

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

index 8dac0337c249..27e8642aa8f5 100644
@@ -26,29 +26,7 @@

drivers/iio/accel/kionix-kx022a.c +30 -3

 	.scan_index = index,					\
 	.scan_type = {                                          \
 		.sign = 's',					\

@@ -231,9 +169,9 @@ static const struct iio_chan_spec_ext_info kx022a_ext_info[] = {
 }
 
 static const struct iio_chan_spec kx022a_channels[] = {
-	KX022A_ACCEL_CHAN(X, 0),
-	KX022A_ACCEL_CHAN(Y, 1),
-	KX022A_ACCEL_CHAN(Z, 2),
+	KX022A_ACCEL_CHAN(X, 0, KX022A),
+	KX022A_ACCEL_CHAN(Y, 1, KX022A),
+	KX022A_ACCEL_CHAN(Z, 2, KX022A),
 	IIO_CHAN_SOFT_TIMESTAMP(3),
 };
 

@@ -286,6 +224,34 @@ static const int kx022a_scale_table[][2] = {
 	{ 4788, 403320 },
 };
 
+const struct kx022a_chip_info kx_chip_info[] = {
+	[KX022A] = {
+		.name		  = "kx022a",
+		.type		  = KX022A,
+		.regmap_config	  = &kx022a_regmap_config,
+		.channels	  = kx022a_channels,
+		.num_channels	  = ARRAY_SIZE(kx022a_channels),
+		.fifo_length	  = KX022A_FIFO_LENGTH,
+		.who		  = KX022A_REG_WHO,
+		.id		  = KX022A_ID,
+		.cntl		  = KX022A_REG_CNTL,
+		.cntl2		  = KX022A_REG_CNTL2,
+		.odcntl		  = KX022A_REG_ODCNTL,
+		.buf_cntl1	  = KX022A_REG_BUF_CNTL1,
+		.buf_cntl2	  = KX022A_REG_BUF_CNTL2,
+		.buf_clear	  = KX022A_REG_BUF_CLEAR,
+		.buf_status1	  = KX022A_REG_BUF_STATUS_1,
+		.buf_smp_lvl_mask = KX022A_MASK_BUF_SMP_LVL,
+		.buf_read	  = KX022A_REG_BUF_READ,
+		.inc1		  = KX022A_REG_INC1,
+		.inc4		  = KX022A_REG_INC4,
+		.inc5		  = KX022A_REG_INC5,
+		.inc6		  = KX022A_REG_INC6,
+		.xout_l		  = KX022A_REG_XOUT_L,
+	},
+};
+EXPORT_SYMBOL_NS_GPL(kx_chip_info, IIO_KX022A);

+static int kx022a_get_fifo_bytes(struct kx022a_data *data)

drivers/iio/accel/kionix-kx022a.c +34 -17

+{
+	struct device *dev = regmap_get_device(data->regmap);
+	__le16 buf_status;
+	int ret, fifo_bytes;
+
+	ret = regmap_bulk_read(data->regmap, data->chip_info->buf_status1, &buf_status, sizeof(buf_status));
+	if (ret) {
+		dev_err(dev, "Error reading buffer status\n");
+		return ret;
+	}
+
+	buf_status &= data->chip_info->buf_smp_lvl_mask;
+	fifo_bytes = le16_to_cpu(buf_status);
+
+	/*
+	 * 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.
+	 */
+	if (data->chip_info->type == KX022A && fifo_bytes == KX022A_FIFO_FULL_VALUE)
+		fifo_bytes = KX022A_FIFO_MAX_BYTES;
+
+	if (fifo_bytes % KX_FIFO_SAMPLES_SIZE_BYTES)
+		dev_warn(data->dev, "Bad FIFO alignment. Data may be corrupt\n");
+
+	return fifo_bytes;
+}
+
 static int kx022a_drop_fifo_contents(struct kx022a_data *data)
 {
 	/*

@@ -593,35 +588,22 @@ static int kx022a_drop_fifo_contents(struct kx022a_data *data)
 	 */
 	data->timestamp = 0;
 
-	return regmap_write(data->regmap, KX022A_REG_BUF_CLEAR, 0x0);
+	return regmap_write(data->regmap, data->chip_info->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);
-	__le16 buffer[KX022A_FIFO_LENGTH * 3];
+	__le16 buffer[data->chip_info->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;
+	fifo_bytes = kx022a_get_fifo_bytes(data);
+	count = fifo_bytes / KX_FIFO_SAMPLES_SIZE_BYTES;
 	if (!count)
 		return 0;

drivers/iio/accel/kionix-kx022a.c +6 -5

 	if (ret) {
 		dev_err(data->dev, "Sensor reset %s\n",
-			val & KX022A_MASK_SRST ? "timeout" : "fail#");
+			val & KX_MASK_SRST ? "timeout" : "fail#");
 		return ret;
 	}
 
-	ret = regmap_reinit_cache(data->regmap, &kx022a_regmap);
+	ret = regmap_reinit_cache(data->regmap, data->chip_info->regmap_config);
 	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);
+	ret = regmap_set_bits(data->regmap, data->chip_info->buf_cntl2,
+			      KX_MASK_BRES16);
 	if (ret) {
 		dev_err(data->dev, "Failed to set data resolution\n");
 		return ret;

@@ -996,7 +976,7 @@ static int kx022a_chip_init(struct kx022a_data *data)
 	return kx022a_prepare_irq_pin(data);
 }
 
-int kx022a_probe_internal(struct device *dev)
+int kx022a_probe_internal(struct device *dev, const struct kx022a_chip_info *chip_info)
 {
 	static const char * const regulator_names[] = {"io-vdd", "vdd"};
 	struct iio_trigger *indio_trig;

@@ -1023,6 +1003,7 @@ int kx022a_probe_internal(struct device *dev)
 		return -ENOMEM;
 
 	data = iio_priv(idev);
+	data->chip_info = chip_info;
 
 	/*
 	 * VDD is the analog and digital domain voltage supply and

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

 		dev_err(dev, "unsupported device 0x%x\n", chip_id);
 		return -EINVAL;
 	}

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

 	ret = devm_request_threaded_irq(data->dev, irq, kx022a_irq_handler,

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

index 12424649d438..3bb40e9f5613 100644

drivers/iio/accel/kionix-kx022a.h +29 -16

+#define KX_SOFT_RESET_WAIT_TIME_US	    (5 * USEC_PER_MSEC)
+#define KX_SOFT_RESET_TOTAL_WAIT_TIME_US    (500 * USEC_PER_MSEC)
+#define KX_DEFAULT_PERIOD_NS		    (20 * NSEC_PER_MSEC)
+#define KX_MASK_GSEL			    GENMASK(4, 3)
+#define KX_MASK_ODR			    GENMASK(3, 0)
+#define KX_MASK_WM_TH			    GENMASK(6, 0)
+#define KX_GSEL_SHIFT			    3
+#define KX_MASK_IEN			    BIT(5)
+#define KX_MASK_DRDY			    BIT(5)
+#define KX_MASK_PC1			    BIT(7)
+#define KX_MASK_IPOL			    BIT(4)
+#define KX_IPOL_LOW			    0
+#define KX_MASK_ITYP			    BIT(3)
+#define KX_ITYP_LEVEL			    0
+#define KX_MASK_INS2_DRDY		    BIT(4)
+#define KX_MASK_WMI			    BIT(5)
+#define KX_MASK_BUF_EN			    BIT(7)
+#define KX_MASK_SRST			    BIT(7)
+#define KX_MASK_BRES16			    BIT(6)
+
+
 #define KX022A_REG_WHO		0x0f
 #define KX022A_ID		0xc8
 
+#define KX022A_FIFO_LENGTH	43
+#define KX022A_FIFO_FULL_VALUE	255
+#define KX022A_FIFO_MAX_BYTES					\
+	 (KX022A_FIFO_LENGTH * KX_FIFO_SAMPLES_SIZE_BYTES)
+
 #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

@@ -45,38 +69,104 @@
 #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_MASK_BUF_SMP_LVL GENMASK(6, 0)
 #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_IPOL_HIGH	KX_MASK_IPOL
+#define KX022A_ITYP_PULSE	KX_MASK_ITYP

drivers/iio/accel/kionix-kx022a.h +13 -0

+
+enum {
+	KX_STATE_SAMPLE,
+	KX_STATE_FIFO,
+};
+
+enum {
+	AXIS_X,
+	AXIS_Y,
+	AXIS_Z,
+	AXIS_MAX
+};
+
 struct device;
 

drivers/iio/accel/kionix-kx022a.h +8 -0

+	u8 id;
+	u8 cntl;
+	u8 cntl2;
+	u8 odcntl;
+	u8 buf_cntl1;
+	u8 buf_cntl2;
+	u8 buf_clear;
+	u8 buf_status1;

drivers/iio/accel/kionix-kx022a.h +24 -0

+	const struct kx022a_chip_info *chip_info;
+	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 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;
+

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

 #endif

Quoted 6 lines · On Fri, 17 Mar 2023 00:48:36 +0100 Mehdi Djait <[email protected]> wrote:

Refactor the kx022a driver implementation to make it more generic and extensible.
Add the chip_info structure will to the driver's private data to hold all the device specific infos.
Move the enum, struct and constants definitions to the header file.

You also introduce an i2c_device_id table

Without that I think module autoloading will be broken anyway so that's definitely a good thing to add.

A few comments inline. Mostly around reducing the name changes. Wild cards (or simply shorted 'generic' prefixes like KX_) almost always bite back in the long run. Hence we generally just try to name things after the first device that they were relevant to.

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

Quoted 3 lines

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

+static const struct i2c_device_id kx022a_i2c_id[] = {
+	{ .name = "kx022a", .driver_data = (kernel_ulong_t)&kx_chip_info[KX022A] },

If there are a small set and we aren't ever going to index the chip_info structure we might be better off not bothering with the enum and instead using a separate instance of the structure for each chip.

Quoted 7 lines

  	data->regmap = regmap;
  	data->dev = dev;
  	data->irq = irq;

drivers/iio/accel/kionix-kx022a.c +1 -1

-	data->odr_ns = KX022A_DEFAULT_PERIOD_NS;
+	data->odr_ns = KX_DEFAULT_PERIOD_NS;
  	mutex_init(&data->mutex);

drivers/iio/accel/kionix-kx022a.c int kx022a_probe_internal(struct device *dev) · line 1066

Quoted 3 lines

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

-	idev->channels = kx022a_channels;
-	idev->num_channels = ARRAY_SIZE(kx022a_channels);
-	idev->name = "kx022-accel";

Ah. Missed this naming in original driver review. We only normally postfix with accel in devices that have multiple sensors with separate drivers. Don't think that is true of the kx022a.

It's ABI so we are stuck with it, but avoid repeating that issue for new devices. >

drivers/iio/accel/kionix-kx022a.h line 95

Quoted 4 lines

drivers/iio/accel/kionix-kx022a.h +3 -0

+enum kx022a_device_type {
+	KX022A,
+};

As below. I'd avoid using the enum unless needed.
That can make sense where a driver supports lots of devices but I don't think it does here.

Quoted 4 lines

drivers/iio/accel/kionix-kx022a.h +2 -2

-int kx022a_probe_internal(struct device *dev);
-extern const struct regmap_config kx022a_regmap;
+struct kx022a_chip_info {
+	const char *name;

drivers/iio/accel/kionix-kx022a.h line 118

Quoted 6 lines

drivers/iio/accel/kionix-kx022a.h +6 -0

+	enum kx022a_device_type type;
+	const struct regmap_config *regmap_config;
+	const struct iio_chan_spec *channels;
+	unsigned int num_channels;
+	unsigned int fifo_length;
+	u8 who;

Some of these are no immediately obvious so either rename the field so it is more obvious what it is, or add comments.

Quoted 13 lines

drivers/iio/accel/kionix-kx022a.h +13 -0

+	u8 id;
+	u8 cntl;
+	u8 cntl2;
+	u8 odcntl;
+	u8 buf_cntl1;
+	u8 buf_cntl2;
+	u8 buf_clear;
+	u8 buf_status1;
+	u16 buf_smp_lvl_mask;
+	u8 buf_read;
+	u8 inc1;
+	u8 inc4;
+	u8 inc5;

drivers/iio/accel/kionix-kx022a.h line 136

Quoted 5 lines

drivers/iio/accel/kionix-kx022a.h +5 -0

+	u8 inc6;
+	u8 xout_l;
+};
+
+struct kx022a_data {

Why move this to the header? Unless there is a strong reason I'd prefer this to stay down in the .c file.

Hi Mehdi and Jonathan,

Just my take on couple of comments from Jonathan :) I still have my own review to do though...

Quoted 10 lines · Jonathan Cameron

Quoted 6 lines · On Fri, 17 Mar 2023 00:48:36 +0100 Mehdi Djait <[email protected]> wrote:

Refactor the kx022a driver implementation to make it more generic and extensible.
Add the chip_info structure will to the driver's private data to hold all the device specific infos.
Move the enum, struct and constants definitions to the header file.

Quoted 4 lines

You also introduce an i2c_device_id table

Without that I think module autoloading will be broken anyway so that's definitely a good thing to add.

I am pretty sure the autoloading worked for OF-systems. But yes, adding the i2c_device_id is probably a good idea. Thanks.

Quoted 5 lines

A few comments inline. Mostly around reducing the name changes. Wild cards (or simply shorted 'generic' prefixes like KX_) almost always bite back in the long run. Hence we generally just try to name things after the first device that they were relevant to.

I must say I disagree on this with you Jonathan. I know wildcards tend to get confusing - but I still like the idea of showing which of the definitions are IC specific and which ones are generic or at least used by more than one variant - especially as long as we only have two supported ICs. I definitely like the macro naming added by Mehdi. This approach has been very helpful for me for example in the BD718x7 (BD71837/BD71847/BD71850) PMIC driver. My take on this is:

1) I like the generic KX_define.

2) I would not try adding wildcards like KX_X22 - to denote support for 122 and 022 - while not supporting 132 - in my experience - that won't scale.

3) I definitely like the idea of using exact model number prefix for 'stuff' which is intended to work only on one exact .

Regarding the 3) - I am not so strict on how the register/mask defines are handled - I _like_ the 1) 2) 3) approach above - but mask/register defines tend to get set (correctly) once and not required to be looked up after this. But. When the 'stuff' is functions - this gets very useful as one is very often required to see which functions are executed on which IC variant. Same goes to structs.

So, if we manage to convince Jonathan about the naming, then I like what yoo had here! I would hovever do it in two steps. I would at first do renaming patch where the generic defines were renamed - without any functional changes - and only then add the kx132 stuff in a subsequent patch. That would simplify seeing which changes are just renaming and which are functional ones.

But here, I must go with the wind - if subsystem maintainer says the code should not have naming like this - then I have no say over it... :/

Quoted 8 lines

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

Quoted 4 lines

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

+static const struct i2c_device_id kx022a_i2c_id[] = {
+	{ .name = "kx022a", .driver_data = (kernel_ulong_t)&kx_chip_info[KX022A] },  

If there are a small set and we aren't ever going to index the chip_info structure we might be better off not bothering with the enum and instead using a separate instance of the structure for each chip.

I kind of like also the table added by Mehdi. I admit I was at first just thinking that we should have a pointer to the struct here without any tables - but... After I took a peek in the kionix-kx022a.c - I kind of liked the table and not exporting the struct names. So, I don't have a strong opinion on this.

I think it's worth noting that this driver could (maybe easily enough) be extended to support also a few other kionix accelerometers. Maybe, if we don't scare Mehdi away, we will see a few other variants supported as well ;)

Quoted 8 lines

Quoted 8 lines

  	data->regmap = regmap;
  	data->dev = dev;
  	data->irq = irq;

drivers/iio/accel/kionix-kx022a.c +1 -1

-	data->odr_ns = KX022A_DEFAULT_PERIOD_NS;
+	data->odr_ns = KX_DEFAULT_PERIOD_NS;
  	mutex_init(&data->mutex);

Quoted 7 lines

drivers/iio/accel/kionix-kx022a.c int kx022a_probe_internal(struct device *dev) · line 1066

Quoted 3 lines

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

-	idev->channels = kx022a_channels;
-	idev->num_channels = ARRAY_SIZE(kx022a_channels);
-	idev->name = "kx022-accel";  

Ah. Missed this naming in original driver review. We only normally postfix with accel in devices that have multiple sensors with separate drivers. Don't think that is true of the kx022a.

Ouch. I am not 100% sure but may be you didn't miss it. It may be I just missed fixing this because your comment here sounds somewhat familiar to me! (Or then you commented on suffix in driver-name).

Quoted 5 lines

It's ABI so we are stuck with it, but avoid repeating that issue for new devices. >

Quoted 1 line

Quoted 7 lines

drivers/iio/accel/kionix-kx022a.h line 95

Quoted 3 lines

drivers/iio/accel/kionix-kx022a.h +3 -0

+enum kx022a_device_type {
+	KX022A,
+};  

As below. I'd avoid using the enum unless needed.
That can make sense where a driver supports lots of devices but I don't think it does here.

Well, I know it is usually not too clever to be prepared for the future stuff too well. But - I don't think the enum and table are adding much of complexity? I am saying this as I think this driver could be extended to support also kx022 (without the A), kx023, kx122. I've also seen some references to model kx022A-120B (but I have no idea what's the story there or if that IC is publicly available). Maybe Mehdi would like to extend this driver further after the KX132 is done ;)

Quoted 13 lines

drivers/iio/accel/kionix-kx022a.h line 118

Quoted 11 lines

drivers/iio/accel/kionix-kx022a.h +8 -2

-int kx022a_probe_internal(struct device *dev);
-extern const struct regmap_config kx022a_regmap;
+struct kx022a_chip_info {
+	const char *name;
+	enum kx022a_device_type type;
+	const struct regmap_config *regmap_config;
+	const struct iio_chan_spec *channels;
+	unsigned int num_channels;
+	unsigned int fifo_length;
+	u8 who;  

Some of these are no immediately obvious so either rename the field so it is more obvious what it is, or add comments.

I would vote for adding a comment :) I like the who. Both the band and this member here :) Data-sheet has register named as "who_am_i" - so I don't think this name is too obfuscating - and what matters to me - it is short yet meaningful.

Quoted 13 lines

Quoted 13 lines

drivers/iio/accel/kionix-kx022a.h +13 -0

+	u8 id;
+	u8 cntl;
+	u8 cntl2;
+	u8 odcntl;
+	u8 buf_cntl1;
+	u8 buf_cntl2;
+	u8 buf_clear;
+	u8 buf_status1;
+	u16 buf_smp_lvl_mask;
+	u8 buf_read;
+	u8 inc1;
+	u8 inc4;
+	u8 inc5;

Quoted 8 lines

drivers/iio/accel/kionix-kx022a.h line 136

Quoted 5 lines

drivers/iio/accel/kionix-kx022a.h +5 -0

+	u8 inc6;
+	u8 xout_l;
+};
+
+struct kx022a_data {  

Why move this to the header? Unless there is a strong reason I'd prefer this to stay down in the .c file.

So would I. It's definitely nice to be able to see the struct in the same file where the code referencing it is.

Yours,
-- Matti

Quoted 16 lines · On Mon, 20 Mar 2023 09:17:54 +0200 Matti Vaittinen <[email protected]>…

Hi Mehdi and Jonathan,

Just my take on couple of comments from Jonathan :) I still have my own review to do though...

Quoted 10 lines · Jonathan Cameron

Quoted 6 lines · On Fri, 17 Mar 2023 00:48:36 +0100 Mehdi Djait <[email protected]> wrote:

Refactor the kx022a driver implementation to make it more generic and extensible.
Add the chip_info structure will to the driver's private data to hold all the device specific infos.
Move the enum, struct and constants definitions to the header file.

Quoted 7 lines

Quoted 4 lines

You also introduce an i2c_device_id table

Without that I think module autoloading will be broken anyway so that's definitely a good thing to add.

I am pretty sure the autoloading worked for OF-systems. But yes, adding the i2c_device_id is probably a good idea. Thanks.

Ah. Maybe that issue only occurred for SPI - I'd assumed it was more general.

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/?id=5fa6863ba692

Quoted 14 lines

Quoted 5 lines

A few comments inline. Mostly around reducing the name changes. Wild cards (or simply shorted 'generic' prefixes like KX_) almost always bite back in the long run. Hence we generally just try to name things after the first device that they were relevant to.

I must say I disagree on this with you Jonathan. I know wildcards tend to get confusing - but I still like the idea of showing which of the definitions are IC specific and which ones are generic or at least used by more than one variant - especially as long as we only have two supported ICs. I definitely like the macro naming added by Mehdi. This approach has been very helpful for me for example in the BD718x7 (BD71837/BD71847/BD71850) PMIC driver. My take on this is:

Quoted 1 line

1) I like the generic KX_define.

We already have other kionix drivers that don't use these defines. This is less of an issue if they are very local - so pushed down to the C file, but I still don't like the implication that they extend to a broad range of devices.

Quoted 3 lines

2) I would not try adding wildcards like KX_X22 - to denote support for 122 and 022 - while not supporting 132 - in my experience - that won't scale.

I think this already runs into this problem or at least sets the driver up to hit it very soon. The reality is that these definitions are shared by the 2 parts supported so far. 3rd part comes along and I'd be willing to place a bet that at least one of these definitions doesn't apply. So we end up with a mess converting it back to a specific name.

I've gone down this path many times before and it very rarely works out.

Quoted 2 lines

3) I definitely like the idea of using exact model number prefix for 'stuff' which is intended to work only on one exact .

When you have 2 devices it is easy to separate the 'generic' from the 'specific'. That breaks when you have 3. If we are sure there won't be a 3rd device supported by this driver then fair enough...

Quoted 7 lines

Regarding the 3) - I am not so strict on how the register/mask defines are handled - I _like_ the 1) 2) 3) approach above - but mask/register defines tend to get set (correctly) once and not required to be looked up after this. But. When the 'stuff' is functions - this gets very useful as one is very often required to see which functions are executed on which IC variant. Same goes to structs.

Given they tend to be accessed via a function pointer, even functions are only set up the once. For these I'm fine with a nasty listing type approach with multiple part names in the function defintion. That doesn't scale great either as lots of parts get added but it at least calls out which function covers which parts.

Quoted 8 lines

So, if we manage to convince Jonathan about the naming, then I like what yoo had here! I would hovever do it in two steps. I would at first do renaming patch where the generic defines were renamed - without any functional changes - and only then add the kx132 stuff in a subsequent patch. That would simplify seeing which changes are just renaming and which are functional ones.

Quoted 2 lines

But here, I must go with the wind - if subsystem maintainer says the code should not have naming like this - then I have no say over it... :/

If we have truely universal defines - sometimes this happens for WHO AM I registers for example as they are the same over all devices from a manufacturer (more or less anyway) then the broad forms are fine. Otherwise it just tends to end up as a mess if lots of parts added.

Quoted 15 lines

Quoted 8 lines

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

Quoted 4 lines

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

+static const struct i2c_device_id kx022a_i2c_id[] = {
+	{ .name = "kx022a", .driver_data = (kernel_ulong_t)&kx_chip_info[KX022A] },  

If there are a small set and we aren't ever going to index the chip_info structure we might be better off not bothering with the enum and instead using a separate instance of the structure for each chip.

I kind of like also the table added by Mehdi. I admit I was at first just thinking that we should have a pointer to the struct here without any tables - but... After I took a peek in the kionix-kx022a.c - I kind of liked the table and not exporting the struct names. So, I don't have a strong opinion on this.

Quoted 4 lines

I think it's worth noting that this driver could (maybe easily enough) be extended to support also a few other kionix accelerometers. Maybe, if we don't scare Mehdi away, we will see a few other variants supported as well ;)

This one wasn't a particularly important bit of feedback. I'm fine with the table, though seems slightly less readable to my eyes.

Quoted 17 lines

Quoted 9 lines

Quoted 9 lines

  	data->regmap = regmap;
  	data->dev = dev;
  	data->irq = irq;

drivers/iio/accel/kionix-kx022a.c +1 -1

-	data->odr_ns = KX022A_DEFAULT_PERIOD_NS;
+	data->odr_ns = KX_DEFAULT_PERIOD_NS;
  	mutex_init(&data->mutex);

Quoted 7 lines

drivers/iio/accel/kionix-kx022a.c int kx022a_probe_internal(struct device *dev) · line 1066

Quoted 3 lines

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

-	idev->channels = kx022a_channels;
-	idev->num_channels = ARRAY_SIZE(kx022a_channels);
-	idev->name = "kx022-accel";  

Ah. Missed this naming in original driver review. We only normally postfix with accel in devices that have multiple sensors with separate drivers. Don't think that is true of the kx022a.

Quoted 3 lines

Ouch. I am not 100% sure but may be you didn't miss it. It may be I just missed fixing this because your comment here sounds somewhat familiar to me! (Or then you commented on suffix in driver-name).

Meh. This stuff happens and at the end of the day it's a magic string that userspace can match against. No userspace knows all of them anyway so most likely it's just provided in a 'selection' box for a user or encoded in a custom script / config file. So not hugely important for it to have the simplest possible form.

Quoted 3 lines

Quoted 3 lines

It's ABI so we are stuck with it, but avoid repeating that issue for new devices. >

Quoted 10 lines

Quoted 9 lines

drivers/iio/accel/kionix-kx022a.h line 95

Quoted 5 lines

drivers/iio/accel/kionix-kx022a.h +3 -0

+enum kx022a_device_type {
+	KX022A,
+};  

As below. I'd avoid using the enum unless needed.
That can make sense where a driver supports lots of devices but I don't think it does here.

Quoted 7 lines

Well, I know it is usually not too clever to be prepared for the future stuff too well. But - I don't think the enum and table are adding much of complexity? I am saying this as I think this driver could be extended to support also kx022 (without the A), kx023, kx122. I've also seen some references to model kx022A-120B (but I have no idea what's the story there or if that IC is publicly available). Maybe Mehdi would like to extend this driver further after the KX132 is done ;)

Not adding a lot, but you are going to end up with adding one line to an enum in the header for each new device, vs one extern line. So I'm not sure it saves anything either.

Quoted 41 lines

Quoted 13 lines

drivers/iio/accel/kionix-kx022a.h line 118

Quoted 11 lines

drivers/iio/accel/kionix-kx022a.h +8 -2

-int kx022a_probe_internal(struct device *dev);
-extern const struct regmap_config kx022a_regmap;
+struct kx022a_chip_info {
+	const char *name;
+	enum kx022a_device_type type;
+	const struct regmap_config *regmap_config;
+	const struct iio_chan_spec *channels;
+	unsigned int num_channels;
+	unsigned int fifo_length;
+	u8 who;  

Some of these are no immediately obvious so either rename the field so it is more obvious what it is, or add comments.

I would vote for adding a comment :) I like the who. Both the band and this member here :) Data-sheet has register named as "who_am_i" - so I don't think this name is too obfuscating - and what matters to me - it is short yet meaningful.

Quoted 21 lines

Quoted 8 lines

drivers/iio/accel/kionix-kx022a.h +8 -0

+	u8 id;
+	u8 cntl;
+	u8 cntl2;
+	u8 odcntl;
+	u8 buf_cntl1;
+	u8 buf_cntl2;
+	u8 buf_clear;
+	u8 buf_status1;

drivers/iio/accel/kionix-kx022a.h line 136

Quoted 10 lines

drivers/iio/accel/kionix-kx022a.h +10 -0

+	u16 buf_smp_lvl_mask;
+	u8 buf_read;
+	u8 inc1;
+	u8 inc4;
+	u8 inc5;
+	u8 inc6;
+	u8 xout_l;
+};
+
+struct kx022a_data {  

Why move this to the header? Unless there is a strong reason I'd prefer this to stay down in the .c file.

Quoted 2 lines

So would I. It's definitely nice to be able to see the struct in the same file where the code referencing it is.

Refactor the kx022a driver implementation to make it more generic and extensible.
Add the chip_info structure will to the driver's private data to hold all the device specific infos.
Move the enum, struct and constants definitions to the header file.

Signed-off-by Mehdi Djait <[email protected]>

---
  drivers/iio/accel/kionix-kx022a-i2c.c |  19 +-
  drivers/iio/accel/kionix-kx022a-spi.c |  22 +-
  drivers/iio/accel/kionix-kx022a.c     | 289 ++++++++++++--------------
  drivers/iio/accel/kionix-kx022a.h     | 128 ++++++++++--
  4 files changed, 274 insertions(+), 184 deletions(-)

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

index e6fd02d931b6..21c4c0ae1a68 100644
@@ -15,23 +15,35 @@
  static int kx022a_i2c_probe(struct i2c_client *i2c)
  {
  	struct device *dev = &i2c->dev;
+	struct kx022a_chip_info *chip_info;
  	struct regmap *regmap;
+	const struct i2c_device_id *id = i2c_client_get_device_id(i2c);
 
  	if (!i2c->irq) {
  		dev_err(dev, "No IRQ configured\n");
  		return -EINVAL;
  	}

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

-
-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 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.
-	 */

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

+		.channels	  = kx022a_channels,
+		.num_channels	  = ARRAY_SIZE(kx022a_channels),
+		.fifo_length	  = KX022A_FIFO_LENGTH,
+		.who		  = KX022A_REG_WHO,
+		.id		  = KX022A_ID,
+		.cntl		  = KX022A_REG_CNTL,
+		.cntl2		  = KX022A_REG_CNTL2,
+		.odcntl		  = KX022A_REG_ODCNTL,
+		.buf_cntl1	  = KX022A_REG_BUF_CNTL1,
+		.buf_cntl2	  = KX022A_REG_BUF_CNTL2,
+		.buf_clear	  = KX022A_REG_BUF_CLEAR,
+		.buf_status1	  = KX022A_REG_BUF_STATUS_1,
+		.buf_smp_lvl_mask = KX022A_MASK_BUF_SMP_LVL,
+		.buf_read	  = KX022A_REG_BUF_READ,
+		.inc1		  = KX022A_REG_INC1,
+		.inc4		  = KX022A_REG_INC4,
+		.inc5		  = KX022A_REG_INC5,
+		.inc6		  = KX022A_REG_INC6,
+		.xout_l		  = KX022A_REG_XOUT_L,
+	},
+};
+EXPORT_SYMBOL_NS_GPL(kx_chip_info, IIO_KX022A);
+
  static int kx022a_read_avail(struct iio_dev *indio_dev,
  			     struct iio_chan_spec const *chan,
  			     const int **vals, int *type, int *length,

@@ -309,19 +275,17 @@ static int kx022a_read_avail(struct iio_dev *indio_dev,
  	}
  }
 

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

+static int kx022a_get_fifo_bytes(struct kx022a_data *data)
+{
+	struct device *dev = regmap_get_device(data->regmap);
+	__le16 buf_status;
+	int ret, fifo_bytes;
+
+	ret = regmap_bulk_read(data->regmap, data->chip_info->buf_status1, &buf_status, sizeof(buf_status));
+	if (ret) {
+		dev_err(dev, "Error reading buffer status\n");
+		return ret;
+	}
+
+	buf_status &= data->chip_info->buf_smp_lvl_mask;
+	fifo_bytes = le16_to_cpu(buf_status);
+
+	/*
+	 * 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

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

+
  static int kx022a_drop_fifo_contents(struct kx022a_data *data)
  {
  	/*

@@ -593,35 +588,22 @@ static int kx022a_drop_fifo_contents(struct kx022a_data *data)
  	 */
  	data->timestamp = 0;
 
-	return regmap_write(data->regmap, KX022A_REG_BUF_CLEAR, 0x0);

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

@@ -812,14 +792,14 @@ static int kx022a_fifo_enable(struct kx022a_data *data)
  		goto unlock_out;
 
  	/* Enable buffer */
-	ret = regmap_set_bits(data->regmap, KX022A_REG_BUF_CNTL2,
-			      KX022A_MASK_BUF_EN);

Quoted 6 lines · Mehdi Djait

Refactor the kx022a driver implementation to make it more generic and extensible.
Add the chip_info structure will to the driver's private data to hold all the device specific infos.
Move the enum, struct and constants definitions to the header file.

Something like:

enum {
	KIONIX_IC_KX022A,
	KIONIX_IC_KX132_xxx, /* xxx denotes accurate model suffix */
};

static const struct of_device_id kx022a_of_match[] = {
	{ .compatible = "kionix,kx022a", .data = KIONIX_IC_KX022A },
	...

chip_id = device_get_match_data(&i2c->dev);

Quoted 32 lines · Mehdi Djait

Refactor the kx022a driver implementation to make it more generic and extensible.
Add the chip_info structure will to the driver's private data to hold all the device specific infos.
Move the enum, struct and constants definitions to the header file.

Signed-off-by Mehdi Djait <[email protected]>

---
  drivers/iio/accel/kionix-kx022a-i2c.c |  19 +-
  drivers/iio/accel/kionix-kx022a-spi.c |  22 +-
  drivers/iio/accel/kionix-kx022a.c     | 289 ++++++++++++--------------
  drivers/iio/accel/kionix-kx022a.h     | 128 ++++++++++--
  4 files changed, 274 insertions(+), 184 deletions(-)

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

index e6fd02d931b6..21c4c0ae1a68 100644
@@ -15,23 +15,35 @@
  static int kx022a_i2c_probe(struct i2c_client *i2c)
  {
  	struct device *dev = &i2c->dev;
+	struct kx022a_chip_info *chip_info;
  	struct regmap *regmap;
+	const struct i2c_device_id *id = i2c_client_get_device_id(i2c);
 
  	if (!i2c->irq) {
  		dev_err(dev, "No IRQ configured\n");
  		return -EINVAL;
  	}
 

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

Quoted 9 lines

drivers/iio/accel/kionix-kx022a-i2c.c +5 -1

-	regmap = devm_regmap_init_i2c(i2c, &kx022a_regmap);
+	chip_info = device_get_match_data(&i2c->dev);
+	if (!chip_info)
+		chip_info = (const struct kx022a_chip_info *) id->driver_data;
+
+	regmap = devm_regmap_init_i2c(i2c, chip_info->regmap_config);
  	if (IS_ERR(regmap))
  		return dev_err_probe(dev, PTR_ERR(regmap),
  				     "Failed to initialize Regmap\n");  

Hm. I would like to pull the regmap_config out of the chip_info struct. As far as I see, the regmap_config is only needed in these bus specific files. On the other hand, the chip-info is only needed in the kionix-kx022a.c file, right?

So, maybe you could here just get the regmap_config based on the chip-id (enum value you added - the data pointer in match tables could be just the enum value indicating the IC type). Then, you could pass this enum value to kx022a_probe_internal() - and the chip-info struct could be selected in the kionix-kx022a.c based on it. That way you would not need the struct chip-info here or regmap_config in kionix-kx022a.c. Same in the *-spi.c

Something like:

enum {
	KIONIX_IC_KX022A,
	KIONIX_IC_KX132_xxx, /* xxx denotes accurate model suffix */
};

static const struct of_device_id kx022a_of_match[] = {
	{ .compatible = "kionix,kx022a", .data = KIONIX_IC_KX022A },
	...

chip_id = device_get_match_data(&i2c->dev);

regmap_cfg = kx022a_kx_regmap_cfg[chip_id];
regmap = devm_regmap_init_i2c(i2c, regmap_cfg);
...
return kx022a_probe_internal(dev, chip_id);

Do you think that would work?

OTOH, to really benefit from this we should probably pull out the regmap-configs from the kionix-kx022a.c. I am not really sure where we should put it then though. Hence, if there is no good ideas how to split the config and chip-info so they are only available/used where needed - then I am also Ok with the current approach.

Quoted 28 lines

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

+static int kx022a_get_fifo_bytes(struct kx022a_data *data)
+{
+	struct device *dev = regmap_get_device(data->regmap);
+	__le16 buf_status;
+	int ret, fifo_bytes;
+
+	ret = regmap_bulk_read(data->regmap, data->chip_info->buf_status1, &buf_status, sizeof(buf_status));
+	if (ret) {
+		dev_err(dev, "Error reading buffer status\n");
+		return ret;
+	}
+
+	buf_status &= data->chip_info->buf_smp_lvl_mask;
+	fifo_bytes = le16_to_cpu(buf_status);
+
+	/*
+	 * 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.
+	 */
+	if (data->chip_info->type == KX022A && fifo_bytes == KX022A_FIFO_FULL_VALUE)
+		fifo_bytes = KX022A_FIFO_MAX_BYTES;
+
+	if (fifo_bytes % KX_FIFO_SAMPLES_SIZE_BYTES)

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

Quoted 4 lines

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

+		dev_warn(data->dev, "Bad FIFO alignment. Data may be corrupt\n");
+
+	return fifo_bytes;
+}  

I like adding this function. Here I agree with Jonathan - having a device specific functions would clarify this a bit. The KX022A "quirk" is a bit confusing. You could then get rid of the buf_smp_lvl_mask.

Quoted 13 lines

drivers/iio/accel/kionix-kx022a-i2c.c +2 -1

+
  static int kx022a_drop_fifo_contents(struct kx022a_data *data)
  {
  	/*

@@ -593,35 +588,22 @@ static int kx022a_drop_fifo_contents(struct kx022a_data *data)
  	 */
  	data->timestamp = 0;
 
-	return regmap_write(data->regmap, KX022A_REG_BUF_CLEAR, 0x0);
+	return regmap_write(data->regmap, data->chip_info->buf_clear, 0x0);
  }
 

drivers/iio/accel/kionix-kx022a-i2c.c static int kx022a_drop_fifo_contents(struct kx022a_data *data) · line 598

Quoted 7 lines

drivers/iio/accel/kionix-kx022a-i2c.c +1 -2

  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);
-	__le16 buffer[KX022A_FIFO_LENGTH * 3];
+	__le16 buffer[data->chip_info->fifo_length * 3];  

I don't like this. Having the length of an array decided at run-time is not something I appreciate. Maybe you could just always reserve the memory so that the largest FIFO gets supported. I am just wondering how large arrays we can safely allocate from the stack?

Quoted 6 lines

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

  	if (ret)
  		goto unlock_out;
 

drivers/iio/accel/kionix-kx022a-i2c.c static int kx022a_drop_fifo_contents(struct kx022a_data *data) · line 602

Quoted 2 lines

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

-int kx022a_probe_internal(struct device *dev)
+int kx022a_probe_internal(struct device *dev, const struct kx022a_chip_info *chip_info)  

As mentioned elsewhere, this might also work if the chip-type enum was passed here as parameter. That way the bus specific part would not need to know about the struct chip_info...

Quoted 7 lines · On Mon, 20 Mar 2023 11:35:06 +0200 Matti Vaittinen <[email protected]>…

On 3/17/23 01:48, Mehdi Djait wrote:

OTOH, to really benefit from this we should probably pull out the regmap-configs from the kionix-kx022a.c. I am not really sure where we should put it then though. Hence, if there is no good ideas how to split the config and chip-info so they are only available/used where needed - then I am also Ok with the current approach.

Definitely stick to current approach. If I had the time I'd rip out all the code useing enums in match tables. It's bitten us a few times with nasty to track down bugs that only affect more obscure ways of binding the driver.

Quoted 26 lines

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

+static int kx022a_get_fifo_bytes(struct kx022a_data *data)
+{
+	struct device *dev = regmap_get_device(data->regmap);
+	__le16 buf_status;
+	int ret, fifo_bytes;
+
+	ret = regmap_bulk_read(data->regmap, data->chip_info->buf_status1, &buf_status, sizeof(buf_status));
+	if (ret) {
+		dev_err(dev, "Error reading buffer status\n");
+		return ret;
+	}
+
+	buf_status &= data->chip_info->buf_smp_lvl_mask;
+	fifo_bytes = le16_to_cpu(buf_status);
+
+	/*
+	 * 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.
+	 */
+	if (data->chip_info->type == KX022A && fifo_bytes == KX022A_FIFO_FULL_VALUE)
+		fifo_bytes = KX022A_FIFO_MAX_BYTES;
+
+	if (fifo_bytes % KX_FIFO_SAMPLES_SIZE_BYTES)

drivers/iio/accel/kionix-kx022a.c static const struct iio_dev_attr *kx022a_fifo_attributes[] = { · line 576

Quoted 4 lines

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

+		dev_warn(data->dev, "Bad FIFO alignment. Data may be corrupt\n");
+
+	return fifo_bytes;
+}

I like adding this function. Here I agree with Jonathan - having a device specific functions would clarify this a bit. The KX022A "quirk" is a bit confusing. You could then get rid of the buf_smp_lvl_mask.

Quoted 15 lines

drivers/iio/accel/kionix-kx022a.c +2 -1

+
  static int kx022a_drop_fifo_contents(struct kx022a_data *data)
  {
  	/*

@@ -593,35 +588,22 @@ static int kx022a_drop_fifo_contents(struct kx022a_data *data)
  	 */
  	data->timestamp = 0;
-	return regmap_write(data->regmap, KX022A_REG_BUF_CLEAR, 0x0);
+	return regmap_write(data->regmap, data->chip_info->buf_clear, 0x0);
  }
  static int __kx022a_fifo_flush(struct iio_dev *idev, unsigned int samples,
  			       bool irq)
  {
  	struct kx022a_data *data = iio_priv(idev);

drivers/iio/accel/kionix-kx022a.c static int kx022a_drop_fifo_contents(struct kx022a_data *data) · line 598

Quoted 3 lines

drivers/iio/accel/kionix-kx022a.c +1 -2

-	struct device *dev = regmap_get_device(data->regmap);
-	__le16 buffer[KX022A_FIFO_LENGTH * 3];
+	__le16 buffer[data->chip_info->fifo_length * 3];

I don't like this. Having the length of an array decided at run-time is not something I appreciate. Maybe you could just always reserve the memory so that the largest FIFO gets supported. I am just wondering how large arrays we can safely allocate from the stack?

Quoted 6 lines

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

@@ -812,14 +792,14 @@ static int kx022a_fifo_enable(struct kx022a_data *data)
  		goto unlock_out;
  	/* Enable buffer */
-	ret = regmap_set_bits(data->regmap, KX022A_REG_BUF_CNTL2,

drivers/iio/accel/kionix-kx022a.c static int kx022a_fifo_enable(struct kx022a_data *data) · line 800

Quoted 10 lines

drivers/iio/accel/kionix-kx022a.c +4 -3

-			      KX022A_MASK_BUF_EN);
+	ret = regmap_set_bits(data->regmap, data->chip_info->buf_cntl2,
+			      KX_MASK_BUF_EN);
  	if (ret)
  		goto unlock_out;
-	data->state |= KX022A_STATE_FIFO;
+	data->state |= KX_STATE_FIFO;
  	ret = regmap_set_bits(data->regmap, data->ien_reg,
-			      KX022A_MASK_WMI);
+			      KX_MASK_WMI);

I think this fits to one line now. (even on my screen)

drivers/iio/accel/kionix-kx022a.c static int kx022a_chip_init(struct kx022a_data *data) · line 979

Quoted 5 lines

drivers/iio/accel/kionix-kx022a.c +1 -1

  	if (ret)
  		goto unlock_out;
 
-int kx022a_probe_internal(struct device *dev)
+int kx022a_probe_internal(struct device *dev, const struct kx022a_chip_info *chip_info)

As mentioned elsewhere, this might also work if the chip-type enum was passed here as parameter. That way the bus specific part would not need to know about the struct chip_info...

drivers/iio/accel/kionix-kx022a.c int kx022a_probe_internal(struct device *dev) · line 1005

Quoted 7 lines

(quoted diff) +0 -0

  {

  	static const char * const regulator_names[] = {"io-vdd", "vdd"};
  	struct iio_trigger *indio_trig;

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

@@ -1023,6 +1003,7 @@ int kx022a_probe_internal(struct device *dev)
  		return -ENOMEM;
  	data = iio_priv(idev);
+	data->chip_info = chip_info;

...Here you could then pick the correct chip_info based on the chip-type enum. In that case I'd like to get the regmap_config(s) in own file. Not sure how that would look like though.

All in all, I like how this looks like. Nice job!

Hello Matti,

Quoted 26 lines

Quoted 26 lines

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

+static int kx022a_get_fifo_bytes(struct kx022a_data *data)
+{
+	struct device *dev = regmap_get_device(data->regmap);
+	__le16 buf_status;
+	int ret, fifo_bytes;
+
+	ret = regmap_bulk_read(data->regmap, data->chip_info->buf_status1, &buf_status, sizeof(buf_status));
+	if (ret) {
+		dev_err(dev, "Error reading buffer status\n");
+		return ret;
+	}
+
+	buf_status &= data->chip_info->buf_smp_lvl_mask;
+	fifo_bytes = le16_to_cpu(buf_status);
+
+	/*
+	 * 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.
+	 */
+	if (data->chip_info->type == KX022A && fifo_bytes == KX022A_FIFO_FULL_VALUE)
+		fifo_bytes = KX022A_FIFO_MAX_BYTES;
+
+	if (fifo_bytes % KX_FIFO_SAMPLES_SIZE_BYTES)

Quoted 8 lines

drivers/iio/accel/kionix-kx022a.c static const struct iio_dev_attr *kx022a_fifo_attributes[] = { · line 576

Quoted 4 lines

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

+		dev_warn(data->dev, "Bad FIFO alignment. Data may be corrupt\n");
+
+	return fifo_bytes;
+}

I like adding this function. Here I agree with Jonathan - having a device specific functions would clarify this a bit. The KX022A "quirk" is a bit confusing. You could then get rid of the buf_smp_lvl_mask.

my bad here, I should have made a separate patch and explained more ... buf_smp_lvl_mask is essential because kionix products use different number of bits to report "the number of data bytes that have been stored in the sample buffer" using the registers BUF_STATUS_1 and BUF_STATUS_2

kx022a: 8bits
kx132: 10bits
kx12x: 11bits
kx126: 12bits

I think this function is quite generic and can be used for different kionix devices:

- It reads BUF_STATUS_1 and BUF_STATUS_2 and then uses a chip specific mask
- It takes care of the quirk of kx022a which is just a simple if statement

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

Datasheet: https://kionixfs.azureedge.net/en/document/KX132-1211-Technical-Reference-Manual-Rev-5.0.pdf

Signed-off-by Mehdi Djait <[email protected]>

---
 drivers/iio/accel/kionix-kx022a-i2c.c |   2 +
 drivers/iio/accel/kionix-kx022a-spi.c |   2 +
 drivers/iio/accel/kionix-kx022a.c     | 126 ++++++++++++++++++++++++++
 drivers/iio/accel/kionix-kx022a.h     |  53 +++++++++++
 4 files changed, 183 insertions(+)

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

index 21c4c0ae1a68..f9b2383c43f1 100644
@@ -38,12 +38,14 @@ static int kx022a_i2c_probe(struct i2c_client *i2c)
 
 static const struct i2c_device_id kx022a_i2c_id[] = {
 	{ .name = "kx022a", .driver_data = (kernel_ulong_t)&kx_chip_info[KX022A] },
+	{ .name = "kx132",  .driver_data = (kernel_ulong_t)&kx_chip_info[KX132] },
 	{ }
 };
 MODULE_DEVICE_TABLE(i2c, kx022a_i2c_id);
 
 static const struct of_device_id kx022a_of_match[] = {
 	{ .compatible = "kionix,kx022a", .data = &kx_chip_info[KX022A] },
+	{ .compatible = "kionix,kx132",  .data = &kx_chip_info[KX132] },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, kx022a_of_match);

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

index ec076af0f261..86a10d6d33ff 100644
@@ -38,12 +38,14 @@ static int kx022a_spi_probe(struct spi_device *spi)
 
 static const struct spi_device_id kx022a_spi_id[] = {
 	{ .name = "kx022a", .driver_data = (kernel_ulong_t)&kx_chip_info[KX022A] },
+	{ .name = "kx132",  .driver_data = (kernel_ulong_t)&kx_chip_info[KX132] },
 	{ }
 };
 MODULE_DEVICE_TABLE(spi, kx022a_spi_id);
 
 static const struct of_device_id kx022a_of_match[] = {
 	{ .compatible = "kionix,kx022a", .data = &kx_chip_info[KX022A] },
+	{ .compatible = "kionix,kx132",  .data = &kx_chip_info[KX132] },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, kx022a_of_match);

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

index 27e8642aa8f5..3cacec99f792 100644
@@ -128,6 +128,101 @@ static const struct regmap_config kx022a_regmap_config = {
 	.cache_type = REGCACHE_RBTREE,
 };
 
+/* Regmap configs kx132 */
+static const struct regmap_range kx132_volatile_ranges[] = {
+	{
+		.range_min = KX132_REG_XADP_L,
+		.range_max = KX132_REG_COTR,
+	}, {
+		.range_min = KX132_REG_TSCP,
+		.range_max = KX132_REG_INT_REL,
+	}, {
+		/* The reset bit will be cleared by sensor */
+		.range_min = KX132_REG_CNTL2,
+		.range_max = KX132_REG_CNTL2,
+	}, {
+		.range_min = KX132_REG_BUF_STATUS_1,
+		.range_max = KX132_REG_BUF_READ,
+	},
+};
+
+static const struct regmap_access_table kx132_volatile_regs = {
+	.yes_ranges = &kx132_volatile_ranges[0],
+	.n_yes_ranges = ARRAY_SIZE(kx132_volatile_ranges),
+};
+
+static const struct regmap_range kx132_precious_ranges[] = {
+	{
+		.range_min = KX132_REG_INT_REL,
+		.range_max = KX132_REG_INT_REL,
+	},
+};
+
+static const struct regmap_access_table kx132_precious_regs = {
+	.yes_ranges = &kx132_precious_ranges[0],
+	.n_yes_ranges = ARRAY_SIZE(kx132_precious_ranges),
+};
+
+static const struct regmap_range kx132_read_only_ranges[] = {
+	{
+		.range_min = KX132_REG_XADP_L,
+		.range_max = KX132_REG_INT_REL,
+	}, {
+		.range_min = KX132_REG_BUF_STATUS_1,
+		.range_max = KX132_REG_BUF_STATUS_2,
+	}, {
+		.range_min = KX132_REG_BUF_READ,
+		.range_max = KX132_REG_BUF_READ,
+	},
+};
+
+static const struct regmap_access_table kx132_ro_regs = {
+	.no_ranges = &kx132_read_only_ranges[0],
+	.n_no_ranges = ARRAY_SIZE(kx132_read_only_ranges),
+};
+
+static const struct regmap_range kx132_write_only_ranges[] = {
+	{
+		.range_min = KX132_REG_MAN_WAKE,
+		.range_max = KX132_REG_MAN_WAKE,
+	}, {
+		.range_min = KX132_REG_SELF_TEST,
+		.range_max = KX132_REG_SELF_TEST,
+	}, {
+		.range_min = KX132_REG_BUF_CLEAR,
+		.range_max = KX132_REG_BUF_CLEAR,
+	},
+};
+
+static const struct regmap_access_table kx132_wo_regs = {
+	.no_ranges = &kx132_write_only_ranges[0],
+	.n_no_ranges = ARRAY_SIZE(kx132_write_only_ranges),
+};
+
+static const struct regmap_range kx132_noinc_read_ranges[] = {
+	{
+		.range_min = KX132_REG_BUF_READ,
+		.range_max = KX132_REG_BUF_READ,
+	},
+};
+
+static const struct regmap_access_table kx132_nir_regs = {
+	.yes_ranges = &kx132_noinc_read_ranges[0],
+	.n_yes_ranges = ARRAY_SIZE(kx132_noinc_read_ranges),
+};
+
+static const struct regmap_config kx132_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+	.volatile_table = &kx132_volatile_regs,
+	.rd_table = &kx132_wo_regs,
+	.wr_table = &kx132_ro_regs,
+	.rd_noinc_table = &kx132_nir_regs,
+	.precious_table = &kx132_precious_regs,
+	.max_register = KX132_MAX_REGISTER,
+	.cache_type = REGCACHE_RBTREE,
+};
+
 static const struct iio_mount_matrix *
 kx022a_get_mount_matrix(const struct iio_dev *idev,
 			const struct iio_chan_spec *chan)

@@ -175,6 +270,13 @@ static const struct iio_chan_spec kx022a_channels[] = {
 	IIO_CHAN_SOFT_TIMESTAMP(3),
 };
 
+static const struct iio_chan_spec kx132_channels[] = {
+	KX022A_ACCEL_CHAN(X, 0, KX132),
+	KX022A_ACCEL_CHAN(Y, 1, KX132),
+	KX022A_ACCEL_CHAN(Z, 2, KX132),
+	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

@@ -249,6 +351,30 @@ const struct kx022a_chip_info kx_chip_info[] = {
 		.inc6		  = KX022A_REG_INC6,
 		.xout_l		  = KX022A_REG_XOUT_L,
 	},
+	[KX132] = {
+		.name		  = "kx132",
+		.type		  = KX132,
+		.regmap_config	  = &kx132_regmap_config,
+		.channels	  = kx132_channels,
+		.num_channels	  = ARRAY_SIZE(kx132_channels),
+		.fifo_length	  = KX132_FIFO_LENGTH,
+		.who		  = KX132_REG_WHO,
+		.id		  = KX132_ID,
+		.cntl		  = KX132_REG_CNTL,
+		.cntl2		  = KX132_REG_CNTL2,
+		.odcntl		  = KX132_REG_ODCNTL,
+		.buf_cntl1	  = KX132_REG_BUF_CNTL1,
+		.buf_cntl2	  = KX132_REG_BUF_CNTL2,
+		.buf_clear	  = KX132_REG_BUF_CLEAR,
+		.buf_status1	  = KX132_REG_BUF_STATUS_1,
+		.buf_smp_lvl_mask = KX132_MASK_BUF_SMP_LVL,
+		.buf_read	  = KX132_REG_BUF_READ,
+		.inc1		  = KX132_REG_INC1,
+		.inc4		  = KX132_REG_INC4,
+		.inc5		  = KX132_REG_INC5,
+		.inc6		  = KX132_REG_INC6,
+		.xout_l		  = KX132_REG_XOUT_L,
+	},
 };
 EXPORT_SYMBOL_NS_GPL(kx_chip_info, IIO_KX022A);
 

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

index 3bb40e9f5613..7e43bdb37156 100644

drivers/iio/accel/kionix-kx022a.h +44 -0

+#define KX132_REG_WHO		0x13
+#define KX132_ID		0x3d
+
+#define KX132_FIFO_LENGTH	86
+
+#define KX132_REG_CNTL2		0x1c
+#define KX132_REG_CNTL		0x1b
+#define KX132_MASK_RES		BIT(6)
+#define KX132_GSEL_2		0x0
+#define KX132_GSEL_4		BIT(3)
+#define KX132_GSEL_8		BIT(4)
+#define KX132_GSEL_16		GENMASK(4, 3)
+
+#define KX132_REG_INS2		0x17
+#define KX132_MASK_INS2_WMI	BIT(5)
+
+#define KX132_REG_XADP_L	0x02
+#define KX132_REG_XOUT_L	0x08
+#define KX132_REG_YOUT_L	0x0a
+#define KX132_REG_ZOUT_L	0x0c
+#define KX132_REG_COTR		0x12
+#define KX132_REG_TSCP		0x14
+#define KX132_REG_INT_REL	0x1a
+
+#define KX132_REG_ODCNTL	0x21
+
+#define KX132_REG_BTS_WUF_TH	0x4a
+#define KX132_REG_MAN_WAKE	0x4d
+
+#define KX132_REG_BUF_CNTL1	0x5e
+#define KX132_REG_BUF_CNTL2	0x5f
+#define KX132_REG_BUF_STATUS_1	0x60
+#define KX132_REG_BUF_STATUS_2	0x61
+#define KX132_MASK_BUF_SMP_LVL	GENMASK(9, 0)
+#define KX132_REG_BUF_CLEAR	0x62
+#define KX132_REG_BUF_READ	0x63
+#define KX132_ODR_SHIFT		3
+#define KX132_FIFO_MAX_WMI_TH	86
+
+#define KX132_REG_INC1		0x22
+#define KX132_REG_INC5		0x26
+#define KX132_REG_INC6		0x27
+#define KX132_IPOL_LOW		0
+#define KX132_IPOL_HIGH		KX_MASK_IPOL

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

 };
 
 enum {

Quoted 6 lines · On Fri, 17 Mar 2023 00:48:37 +0100 Mehdi Djait <[email protected]> wrote:

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

Datasheet: https://kionixfs.azureedge.net/en/document/KX132-1211-Technical-Reference-Manual-Rev-5.0.pdf

Signed-off-by Mehdi Djait <[email protected]>

Nothing much specific to this patch, most changes will be as a result of bringing this inline with the changes suggested for patch 2.

thanks,

Jonathan

Quoted 9 lines

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

index 3bb40e9f5613..7e43bdb37156 100644
@@ -90,8 +90,61 @@
 #define KX022A_REG_SELF_TEST	0x60
 #define KX022A_MAX_REGISTER	0x60
 

drivers/iio/accel/kionix-kx022a.h line 93

Quoted 1 line

drivers/iio/accel/kionix-kx022a.h +1 -0

+

Push these down into the c file.

Quoted 50 lines

drivers/iio/accel/kionix-kx022a.h +49 -0

+#define KX132_REG_WHO		0x13
+#define KX132_ID		0x3d
+
+#define KX132_FIFO_LENGTH	86
+
+#define KX132_REG_CNTL2		0x1c
+#define KX132_REG_CNTL		0x1b
+#define KX132_MASK_RES		BIT(6)
+#define KX132_GSEL_2		0x0
+#define KX132_GSEL_4		BIT(3)
+#define KX132_GSEL_8		BIT(4)
+#define KX132_GSEL_16		GENMASK(4, 3)
+
+#define KX132_REG_INS2		0x17
+#define KX132_MASK_INS2_WMI	BIT(5)
+
+#define KX132_REG_XADP_L	0x02
+#define KX132_REG_XOUT_L	0x08
+#define KX132_REG_YOUT_L	0x0a
+#define KX132_REG_ZOUT_L	0x0c
+#define KX132_REG_COTR		0x12
+#define KX132_REG_TSCP		0x14
+#define KX132_REG_INT_REL	0x1a
+
+#define KX132_REG_ODCNTL	0x21
+
+#define KX132_REG_BTS_WUF_TH	0x4a
+#define KX132_REG_MAN_WAKE	0x4d
+
+#define KX132_REG_BUF_CNTL1	0x5e
+#define KX132_REG_BUF_CNTL2	0x5f
+#define KX132_REG_BUF_STATUS_1	0x60
+#define KX132_REG_BUF_STATUS_2	0x61
+#define KX132_MASK_BUF_SMP_LVL	GENMASK(9, 0)
+#define KX132_REG_BUF_CLEAR	0x62
+#define KX132_REG_BUF_READ	0x63
+#define KX132_ODR_SHIFT		3
+#define KX132_FIFO_MAX_WMI_TH	86
+
+#define KX132_REG_INC1		0x22
+#define KX132_REG_INC5		0x26
+#define KX132_REG_INC6		0x27
+#define KX132_IPOL_LOW		0
+#define KX132_IPOL_HIGH		KX_MASK_IPOL
+#define KX132_ITYP_PULSE	KX_MASK_ITYP
+
+#define KX132_REG_INC4		0x25
+
+#define KX132_REG_SELF_TEST	0x5d

drivers/iio/accel/kionix-kx022a.h line 147

Quoted 5 lines

drivers/iio/accel/kionix-kx022a.h +3 -0

+#define KX132_MAX_REGISTER	0x76
+
 enum kx022a_device_type {
 	KX022A,
+	KX132,

As mentioned in previous review, I think this would be neater done by just exporting the chip_info structures directly rather than putting them in an array.

Hello Jonathan,

Quoted 25 lines · Jonathan Cameron

Quoted 6 lines · On Fri, 17 Mar 2023 00:48:37 +0100 Mehdi Djait <[email protected]> wrote:

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

Datasheet: https://kionixfs.azureedge.net/en/document/KX132-1211-Technical-Reference-Manual-Rev-5.0.pdf

Signed-off-by Mehdi Djait <[email protected]>

Nothing much specific to this patch, most changes will be as a result of bringing this inline with the changes suggested for patch 2.

thanks,

Jonathan

Quoted 9 lines

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

index 3bb40e9f5613..7e43bdb37156 100644
@@ -90,8 +90,61 @@
 #define KX022A_REG_SELF_TEST	0x60
 #define KX022A_MAX_REGISTER	0x60
 

Quoted 3 lines

drivers/iio/accel/kionix-kx022a.h line 93

Quoted 1 line

drivers/iio/accel/kionix-kx022a.h +1 -0

+  

Push these down into the c file.

Do you mean all REG and MASK defines ?
Even kx022a defines them in the h file, or am I misunderstanding your comment ?

Quoted 51 lines

Quoted 51 lines

drivers/iio/accel/kionix-kx022a.h +49 -0

+#define KX132_REG_WHO		0x13
+#define KX132_ID		0x3d
+
+#define KX132_FIFO_LENGTH	86
+
+#define KX132_REG_CNTL2		0x1c
+#define KX132_REG_CNTL		0x1b
+#define KX132_MASK_RES		BIT(6)
+#define KX132_GSEL_2		0x0
+#define KX132_GSEL_4		BIT(3)
+#define KX132_GSEL_8		BIT(4)
+#define KX132_GSEL_16		GENMASK(4, 3)
+
+#define KX132_REG_INS2		0x17
+#define KX132_MASK_INS2_WMI	BIT(5)
+
+#define KX132_REG_XADP_L	0x02
+#define KX132_REG_XOUT_L	0x08
+#define KX132_REG_YOUT_L	0x0a
+#define KX132_REG_ZOUT_L	0x0c
+#define KX132_REG_COTR		0x12
+#define KX132_REG_TSCP		0x14
+#define KX132_REG_INT_REL	0x1a
+
+#define KX132_REG_ODCNTL	0x21
+
+#define KX132_REG_BTS_WUF_TH	0x4a
+#define KX132_REG_MAN_WAKE	0x4d
+
+#define KX132_REG_BUF_CNTL1	0x5e
+#define KX132_REG_BUF_CNTL2	0x5f
+#define KX132_REG_BUF_STATUS_1	0x60
+#define KX132_REG_BUF_STATUS_2	0x61
+#define KX132_MASK_BUF_SMP_LVL	GENMASK(9, 0)
+#define KX132_REG_BUF_CLEAR	0x62
+#define KX132_REG_BUF_READ	0x63
+#define KX132_ODR_SHIFT		3
+#define KX132_FIFO_MAX_WMI_TH	86
+
+#define KX132_REG_INC1		0x22
+#define KX132_REG_INC5		0x26
+#define KX132_REG_INC6		0x27
+#define KX132_IPOL_LOW		0
+#define KX132_IPOL_HIGH		KX_MASK_IPOL
+#define KX132_ITYP_PULSE	KX_MASK_ITYP
+
+#define KX132_REG_INC4		0x25
+
+#define KX132_REG_SELF_TEST	0x5d

Quoted 8 lines

drivers/iio/accel/kionix-kx022a.h line 147

Quoted 5 lines

drivers/iio/accel/kionix-kx022a.h +3 -0

+#define KX132_MAX_REGISTER	0x76
+
 enum kx022a_device_type {
 	KX022A,
+	KX132,  

As mentioned in previous review, I think this would be neater done by just exporting the chip_info structures directly rather than putting them in an array.

I gave the reason in a response to the previous review.

--
Kind Regards
Mehdi Djait

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

KX132 accelerometer is a sensor which:
- supports G-ranges of (+/-) 2, 4, 8, and 16G
- can be connected to I2C or SPI
- has internal HW FIFO buffer
- supports various ODRs (output data rates)

The KX132 accelerometer is very similair to the KX022A.
One key difference is number of bits to report the number of data bytes that have been stored in the sample buffer: 8 bits for KX022A vs 10 bits for KX132.

A complete list of differences is listed in [1]
[1] https://kionixfs.azureedge.net/en/document/AN112-Transitioning-to-KX132-1211-Accelerometer.pdf1

Quoted 6 lines · Mehdi Djait

KX132 accelerometer is a sensor which:
- supports G-ranges of (+/-) 2, 4, 8, and 16G
- can be connected to I2C or SPI
- has internal HW FIFO buffer
- supports various ODRs (output data rates)

Quoted 6 lines

The KX132 accelerometer is very similair to the KX022A.
One key difference is number of bits to report the number of data bytes that have been stored in the sample buffer: 8 bits for KX022A vs 10 bits for KX132.

A complete list of differences is listed in [1]
[1] https://kionixfs.azureedge.net/en/document/AN112-Transitioning-to-KX132-1211-Accelerometer.pdf1

Is it really the first version of this contribution?

KX132 accelerometer is a sensor which:
- supports G-ranges of (+/-) 2, 4, 8, and 16G
- can be connected to I2C or SPI
- has internal HW FIFO buffer
- supports various ODRs (output data rates)

The KX132 accelerometer is very similair to the KX022A.
One key difference is number of bits to report the number of data bytes that have been stored in the sample buffer: 8 bits for KX022A vs 10 bits for KX132.

A complete list of differences is listed in [1]

[1] https://kionixfs.azureedge.net/en/document/AN112-Transitioning-to-KX132-1211-Accelerometer.pdf1

Mehdi Djait (3):
dt-bindings: iio: Add KX132 accelerometer
iio: accel: kionix-kx022a: Add chip_info structure
iio: accel: Add support for Kionix/ROHM KX132 accelerometer

  .../bindings/iio/accel/kionix,kx022a.yaml     |  13 +-
  drivers/iio/accel/kionix-kx022a-i2c.c         |  21 +-
  drivers/iio/accel/kionix-kx022a-spi.c         |  24 +-
  drivers/iio/accel/kionix-kx022a.c             | 413 +++++++++++-------
  drivers/iio/accel/kionix-kx022a.h             | 181 +++++++-
  5 files changed, 464 insertions(+), 188 deletions(-)

Hi Mehdi,

Things have been piling up for me during last two weeks... I will do proper review during next week.

Quoted 6 lines · Mehdi Djait

KX132 accelerometer is a sensor which:
- supports G-ranges of (+/-) 2, 4, 8, and 16G
- can be connected to I2C or SPI
- has internal HW FIFO buffer
- supports various ODRs (output data rates)

Quoted 3 lines

The KX132 accelerometer is very similair to the KX022A.
One key difference is number of bits to report the number of data bytes that have been stored in the sample buffer: 8 bits for KX022A vs 10 bits for KX132.

The KX022A has 16bits of data in HiRes mode. This is the default for kx022a driver.

Quoted 5 lines

A complete list of differences is listed in [1]

[1] https://kionixfs.azureedge.net/en/document/AN112-Transitioning-to-KX132-1211-Accelerometer.pdf1

This document is somewhat misleading. It does not contain KX022A but the older KX022. Kionix has the somewhat confusing habit of having very similar names for models with - occasionally significant - differences. (My own opinion).

I the "Technical referene manual" is more interesting document than the data-sheet:

https://kionixfs.azureedge.net/en/document/KX132-1211-Technical-Reference-Manual-Rev-5.0.pdf

I have heard that there have been a few very different versions of KX132 as well. Not sure if they have "leaked" out to public though. In any case, for the kx132 it might be safest to use the full model name - especially in the DT compatibles.

Finally, AFAIK the key "thing" in KX132 is the "ADP" (Advanced Data Path) feature which allows filtering the data "in sensor". Unfortunately, I am not really familiar with this feature. Do you think this is something that might get configured only once at start-up depending on the purpose of the board? If yes, this might be something that will end-up having properties in device-tree. If yes, then it might be a good idea to have own binding doc for KX132. Currently it seems Ok to have them in the same binding doc though.

Hi Matti,

Quoted 12 lines · Matti Vaittinen

Hi Mehdi,

Things have been piling up for me during last two weeks... I will do proper review during next week.

Quoted 6 lines · Mehdi Djait

KX132 accelerometer is a sensor which:
- supports G-ranges of (+/-) 2, 4, 8, and 16G
- can be connected to I2C or SPI
- has internal HW FIFO buffer
- supports various ODRs (output data rates)

Quoted 6 lines

Quoted 3 lines

The KX132 accelerometer is very similair to the KX022A.
One key difference is number of bits to report the number of data bytes that have been stored in the sample buffer: 8 bits for KX022A vs 10 bits for KX132.

The KX022A has 16bits of data in HiRes mode. This is the default for kx022a driver.

I am talking here about "Buffer Sample Level number of bits":

8 bits: just BUF_STATUS_1 to report the status of the buffer. kx132 uses BUF_STATUS_1 and the Bit0, Bit1 of BUF_STATUS_2.

That's the reason for adding the kx022a_get_fifo_bytes function.

Quoted 6 lines

Quoted 5 lines

A complete list of differences is listed in [1]

[1] https://kionixfs.azureedge.net/en/document/AN112-Transitioning-to-KX132-1211-Accelerometer.pdf1

Quoted 4 lines

This document is somewhat misleading. It does not contain KX022A but the older KX022. Kionix has the somewhat confusing habit of having very similar names for models with - occasionally significant - differences. (My own opinion).

Yes, I am aware that it does not contain KX022A, but from my understanding of your code, the document can be used to highlight the difference I mentioned

Quoted 9 lines

Finally, AFAIK the key "thing" in KX132 is the "ADP" (Advanced Data Path) feature which allows filtering the data "in sensor". Unfortunately, I am not really familiar with this feature. Do you think this is something that might get configured only once at start-up depending on the purpose of the board? If yes, this might be something that will end-up having properties in device-tree. If yes, then it might be a good idea to have own binding doc for KX132. Currently it seems Ok to have them in the same binding doc though.

Correct me if I am wrong: the Devicetree is a description of the hardware

and the transitioning document states:

"From a hardware perspective, all these sensors
have an identical pad/pin layouts and identical pin functionality"

I was thinking about adding an advanced_data_path boolean to the chip_info struct and providing different driver callbacks depending on the value.

Something like in the bmc150-accel-core: iio_info for bmc150_accel_info and iio_info for bmc150_accel_info_fifo