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

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

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

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

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

---
v2:
- made the device name more specific from "kx132" to "kx132-1211"
- removed the output data-rates mentioned and replaced them with "variable output data-rates"

Avoid error returns on a failure to match and instead just warn with assumption that we have a correct dt-binding telling us that some new device with a different ID is backwards compatible.

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

---
v2:
- no changes, this patch is introduced in the v2

  drivers/iio/accel/kionix-kx022a.c | 4 +---
  1 file changed, 1 insertion(+), 3 deletions(-)

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

index f98393d74666..70530005cad3 100644

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

  	irq = fwnode_irq_get_byname(fwnode, "INT1");
  	if (irq > 0) {

Quoted 17 lines · Mehdi Djait

Avoid error returns on a failure to match and instead just warn with assumption that we have a correct dt-binding telling us that some new device with a different ID is backwards compatible.

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

---
v2:
- no changes, this patch is introduced in the v2

  drivers/iio/accel/kionix-kx022a.c | 4 +---
  1 file changed, 1 insertion(+), 3 deletions(-)

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

index f98393d74666..70530005cad3 100644

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

Quoted 7 lines

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

@@ -1038,9 +1038,7 @@ int kx022a_probe_internal(struct device *dev)
  		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;
-	}
+		dev_warn(dev, "unsupported device 0x%x\n", chip_id);  

Just a 'nit' - no need to re-spin the series for this if there is no other changes requested.

Maybe a slightly better wording here would be "unknown device"? If I am not mistaken the code proceeds because device is assumed to be supported.

Jonathan, do you think this change is worth backporting? If yes, then we might need a Fixes tag.

Acked-by Matti Vaittinen <[email protected]>

Quoted 3 lines

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

  	irq = fwnode_irq_get_byname(fwnode, "INT1");
  	if (irq > 0) {  

Avoid error returns on a failure to match and instead just warn with assumption that we have a correct dt-binding telling us that some new device with a different ID is backwards compatible.

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

---
v2:
- no changes, this patch is introduced in the v2

 drivers/iio/accel/kionix-kx022a.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

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

index f98393d74666..70530005cad3 100644

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

 	irq = fwnode_irq_get_byname(fwnode, "INT1");
 	if (irq > 0) {

Quoted 19 lines · On Thu, 20 Apr 2023 22:22:02 +0200 Mehdi Djait <[email protected]> wrote:

Avoid error returns on a failure to match and instead just warn with assumption that we have a correct dt-binding telling us that some new device with a different ID is backwards compatible.

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

---
v2:
- no changes, this patch is introduced in the v2

 drivers/iio/accel/kionix-kx022a.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

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

index f98393d74666..70530005cad3 100644
@@ -1038,9 +1038,7 @@ int kx022a_probe_internal(struct device *dev)
 		return dev_err_probe(dev, ret, "Failed to access sensor\n");
 

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

Quoted 5 lines

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

 	if (chip_id != KX022A_ID) {
-		dev_err(dev, "unsupported device 0x%x\n", chip_id);
-		return -EINVAL;
-	}
+		dev_warn(dev, "unsupported device 0x%x\n", chip_id);

Try building this ;) You have remove the closing bracket but kept the opening one.

Refactor the kx022a driver implementation to make it more generic and extensible.
Introduce an i2c_device_id table.
Add the chip_info structure to the driver's private data to hold all the device specific infos.

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

---
v2:
- mentioned the introduction of the i2c_device_id table in the commit

- get i2c_/spi_get_device_id only with device get match fails
- removed the generic KX_define
- removed the kx022a_device_type enum
- added comments for the chip_info struct elements
- fixed errors pointed out by the kernel test robot

  drivers/iio/accel/kionix-kx022a-i2c.c |  20 ++++-
  drivers/iio/accel/kionix-kx022a-spi.c |  21 ++++--
  drivers/iio/accel/kionix-kx022a.c     | 102 ++++++++++++++++----------
  drivers/iio/accel/kionix-kx022a.h     |  54 +++++++++++++-
  4 files changed, 146 insertions(+), 51 deletions(-)

drivers/iio/accel/kionix-kx022a-i2c.c +17 -3

index e6fd02d931b6..ce299d0446f7 100644
@@ -15,6 +15,7 @@
  static int kx022a_i2c_probe(struct i2c_client *i2c)
  {
  	struct device *dev = &i2c->dev;
+	const struct kx022a_chip_info *chip_info;
  	struct regmap *regmap;
 
  	if (!i2c->irq) {

@@ -22,16 +23,28 @@ static int kx022a_i2c_probe(struct i2c_client *i2c)
  		return -EINVAL;
  	}
 
-	regmap = devm_regmap_init_i2c(i2c, &kx022a_regmap);
+	chip_info = device_get_match_data(&i2c->dev);
+	if (!chip_info) {
+		const struct i2c_device_id *id = i2c_client_get_device_id(i2c);
+		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");
 
-	return kx022a_probe_internal(dev);
+	return kx022a_probe_internal(dev, chip_info);
  }
 
+static const struct i2c_device_id kx022a_i2c_id[] = {
+	{ .name = "kx022a", .driver_data = (kernel_ulong_t)&kx022a_chip_info },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, kx022a_i2c_id);
+
  static const struct of_device_id kx022a_of_match[] = {
-	{ .compatible = "kionix,kx022a", },
+	{ .compatible = "kionix,kx022a", .data = &kx022a_chip_info },
  	{ }
  };
  MODULE_DEVICE_TABLE(of, kx022a_of_match);

@@ -42,6 +55,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 +8 -1

index 9cd047f7b346..7bc81588e40e 100644
@@ -15,6 +15,7 @@
  static int kx022a_spi_probe(struct spi_device *spi)
  {
  	struct device *dev = &spi->dev;
+	const struct kx022a_chip_info *chip_info;
  	struct regmap *regmap;
 
  	if (!spi->irq) {

@@ -22,22 +23,28 @@ static int kx022a_spi_probe(struct spi_device *spi)
  		return -EINVAL;
  	}
 
-	regmap = devm_regmap_init_spi(spi, &kx022a_regmap);
+	chip_info = device_get_match_data(&spi->dev);
+	if (!chip_info) {
+		const struct spi_device_id *id = spi_get_device_id(spi);
+		chip_info = (const struct kx022a_chip_info *)id->driver_data;
+	}
+
+	regmap = devm_regmap_init_spi(spi, chip_info->regmap_config);
  	if (IS_ERR(regmap))

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

  };
  module_spi_driver(kx022a_spi_driver);
 

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

index 70530005cad3..7f9a2c29790b 100644
@@ -48,7 +48,7 @@ enum {
  	KX022A_STATE_FIFO,
  };
 
-/* Regmap configs */
+/* kx022a Regmap configs */
  static const struct regmap_range kx022a_volatile_ranges[] = {
  	{
  		.range_min = KX022A_REG_XHP_L,

@@ -138,7 +138,7 @@ static const struct regmap_access_table kx022a_nir_regs = {
  	.n_yes_ranges = ARRAY_SIZE(kx022a_noinc_read_ranges),
  };
 
-const struct regmap_config kx022a_regmap = {
+static const struct regmap_config kx022a_regmap_config = {
  	.reg_bits = 8,
  	.val_bits = 8,
  	.volatile_table = &kx022a_volatile_regs,

@@ -149,7 +149,6 @@ const struct regmap_config kx022a_regmap = {
  	.max_register = KX022A_MAX_REGISTER,
  	.cache_type = REGCACHE_RBTREE,
  };
-EXPORT_SYMBOL_NS_GPL(kx022a_regmap, IIO_KX022A);
 
  struct kx022a_data {
  	struct regmap *regmap;

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

-#define KX022A_ACCEL_CHAN(axis, index)				\
+#define KX022A_ACCEL_CHAN(axis, reg, index)			\
  {								\
  	.type = IIO_ACCEL,					\
  	.modified = 1,						\

@@ -220,7 +219,7 @@ static const struct iio_chan_spec_ext_info kx022a_ext_info[] = {
  				BIT(IIO_CHAN_INFO_SCALE) |	\
  				BIT(IIO_CHAN_INFO_SAMP_FREQ),	\
  	.ext_info = kx022a_ext_info,				\
-	.address = KX022A_REG_##axis##OUT_L,			\
+	.address = reg,						\
  	.scan_index = index,					\
  	.scan_type = {                                          \
  		.sign = 's',					\

@@ -231,9 +230,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, KX022A_REG_XOUT_L, 0),
+	KX022A_ACCEL_CHAN(Y, KX022A_REG_YOUT_L, 1),
+	KX022A_ACCEL_CHAN(Z, KX022A_REG_ZOUT_L, 2),
  	IIO_CHAN_SOFT_TIMESTAMP(3),
  };
 

Hi Mehdi,

Thanks for working on this driver :) Much appreciated!

Quoted 6 lines · Mehdi Djait

Refactor the kx022a driver implementation to make it more generic and extensible.
Introduce an i2c_device_id table.
Add the chip_info structure to the driver's private data to hold all the device specific infos.

Quoted 4 lines

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

---
v2:
- mentioned the introduction of the i2c_device_id table in the commit

Maybe adding the i2c_device_id table could be done in a separate patch with a Fixes tag? That would help backporting (and I think changes like this are worth it).

Quoted 16 lines · On Fri, 21 Apr 2023 09:19:32 +0300 Matti Vaittinen <[email protected]>…

Hi Mehdi,

Thanks for working on this driver :) Much appreciated!

Quoted 10 lines · Mehdi Djait

Refactor the kx022a driver implementation to make it more generic and extensible.
Introduce an i2c_device_id table.
Add the chip_info structure to the driver's private data to hold all the device specific infos.

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

---
v2:
- mentioned the introduction of the i2c_device_id table in the commit

Quoted 3 lines

Maybe adding the i2c_device_id table could be done in a separate patch with a Fixes tag? That would help backporting (and I think changes like this are worth it).

Is it technically a fix? I thought it was but turned out my reasoning only
applied to spi. Agreed it would be nice as a separate patch though.
It's not directly related to the rest of what is happening here.

Since Kionix accelerometers use various numbers of bits to report data, a device-specific function is required.

Move the driver's private data to the header file to support the new function.

Make the allocation of the "buffer" array in the fifo_flush function dynamic and more generic.

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

---
v2:
- separated this change from the chip_info introduction and made it a patch in v2

- changed the function from generic implementation for to device-specific one
- removed blank lines pointed out by checkpatch
- changed the allocation of the "buffer" array in __kx022a_fifo_flush

  drivers/iio/accel/kionix-kx022a.c | 72 +++++++++++++------------------
  drivers/iio/accel/kionix-kx022a.h | 37 ++++++++++++++++
  2 files changed, 66 insertions(+), 43 deletions(-)

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

index 7f9a2c29790b..1c81ea1657b9 100644

Hi Mehdi,

Thanks for the v2!

Quoted 2 lines · Mehdi Djait

Since Kionix accelerometers use various numbers of bits to report data, a device-specific function is required.

I think this is the right approach. Thanks for adding this device-specific function.

Quoted 1 line

Move the driver's private data to the header file to support the new function.

Hmm. Why this move is necessary? I didn't immediately spot this struct being used outside this C-file. I'd rather saw the struct in C-file if possible.

Quoted 7 lines

Make the allocation of the "buffer" array in the fifo_flush function dynamic and more generic.

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

---
v2:
- separated this change from the chip_info introduction and made it a patch in v2

I am unsure if this separation was needed. I'd only separate the "naming changes" which bring no changes to code flow, and then patches which are fixes and need to be backported (to minimize backporting effort and impact to stable branches). Well, I am fine with this separation though, seems like I am just making a noise here :).

Quoted 14 lines

- changed the function from generic implementation for to device-specific one
- removed blank lines pointed out by checkpatch
- changed the allocation of the "buffer" array in __kx022a_fifo_flush

  drivers/iio/accel/kionix-kx022a.c | 72 +++++++++++++------------------
  drivers/iio/accel/kionix-kx022a.h | 37 ++++++++++++++++
  2 files changed, 66 insertions(+), 43 deletions(-)

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

index 7f9a2c29790b..1c81ea1657b9 100644

drivers/iio/accel/kionix-kx022a.c static const struct regmap_config kx022a_regmap_config = { · line 151

Quoted 2 lines

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

@@ -150,36 +150,6 @@ static const struct regmap_config kx022a_regmap_config = {
  	.cache_type = REGCACHE_RBTREE,
  };  

Yours,
-- Matti

Quoted 25 lines · On Fri, 21 Apr 2023 09:14:10 +0300 Matti Vaittinen <[email protected]>…

Hi Mehdi,

Thanks for the v2!

Quoted 2 lines · Mehdi Djait

Since Kionix accelerometers use various numbers of bits to report data, a device-specific function is required.

I think this is the right approach. Thanks for adding this device-specific function.

Quoted 1 line

Move the driver's private data to the header file to support the new function.

Hmm. Why this move is necessary? I didn't immediately spot this struct being used outside this C-file. I'd rather saw the struct in C-file if possible.

Quoted 7 lines

Make the allocation of the "buffer" array in the fifo_flush function dynamic and more generic.

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

---
v2:
- separated this change from the chip_info introduction and made it a patch in v2

Quoted 5 lines

I am unsure if this separation was needed. I'd only separate the "naming changes" which bring no changes to code flow, and then patches which are fixes and need to be backported (to minimize backporting effort and impact to stable branches). Well, I am fine with this separation though, seems like I am just making a noise here :).

I prefer them split. I want to see a noop patch for refactors. It's easier to quickly check it really makes no functional change. Adding new structures for a new device just adds noise to checking that.

Quoted 22 lines

Quoted 16 lines

- changed the function from generic implementation for to device-specific one
- removed blank lines pointed out by checkpatch
- changed the allocation of the "buffer" array in __kx022a_fifo_flush

  drivers/iio/accel/kionix-kx022a.c | 72 +++++++++++++------------------
  drivers/iio/accel/kionix-kx022a.h | 37 ++++++++++++++++
  2 files changed, 66 insertions(+), 43 deletions(-)

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

index 7f9a2c29790b..1c81ea1657b9 100644

drivers/iio/accel/kionix-kx022a.c static const struct regmap_config kx022a_regmap_config = { · line 151

Quoted 2 lines

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

@@ -150,36 +150,6 @@ static const struct regmap_config kx022a_regmap_config = {
  	.cache_type = REGCACHE_RBTREE,
  };    

Yours,
-- Matti

Hi Mehdi,

Thanks for the v2!

Quoted 2 lines · Mehdi Djait

Since Kionix accelerometers use various numbers of bits to report data, a device-specific function is required.

I think this is the right approach. Thanks for adding this device-specific function.

Quoted 1 line

Move the driver's private data to the header file to support the new function.

Hmm. Why this move is necessary? I didn't immediately spot this struct being used outside this C-file. I'd rather saw the struct in C-file if possible.

Since Kionix accelerometers use various numbers of bits to report data, a device-specific function is required.
Move the driver's private data to the header file to support the new function. Make the allocation of the "buffer" array in the fifo_flush function dynamic and more generic.

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

---
v2:
- separated this change from the chip_info introduction and made it a patch in v2
- changed the function from generic implementation for to device-specific one
- removed blank lines pointed out by checkpatch
- changed the allocation of the "buffer" array in __kx022a_fifo_flush

 drivers/iio/accel/kionix-kx022a.c | 72 +++++++++++++------------------
 drivers/iio/accel/kionix-kx022a.h | 37 ++++++++++++++++
 2 files changed, 66 insertions(+), 43 deletions(-)

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

index 7f9a2c29790b..1c81ea1657b9 100644
@@ -150,36 +150,6 @@ static const struct regmap_config kx022a_regmap_config = {
 	.cache_type = REGCACHE_RBTREE,
 };
 
-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.
-	 */
-	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 *

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

 }
 
 static int kx022a_turn_off_lock(struct kx022a_data *data)

@@ -595,34 +564,50 @@ static int kx022a_drop_fifo_contents(struct kx022a_data *data)
 	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;
 	uint64_t sample_period;
 	int count, fifo_bytes;
 	bool renable = false;

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

 	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)
+	if (!count) {
+		kfree(buffer);
 		return 0;
+	}
 
 	/*
 	 * If we are being called from IRQ handler we know the stored timestamp

@@ -704,6 +689,7 @@ static int __kx022a_fifo_flush(struct iio_dev *idev, unsigned int samples,
 	if (renable)
 		enable_irq(data->irq);
 
+	kfree(buffer);
 	return ret;
 }
 

@@ -1016,6 +1002,7 @@ const struct kx022a_chip_info kx022a_chip_info = {
 	.inc5		  = KX022A_REG_INC5,
 	.inc6		  = KX022A_REG_INC6,
 	.xout_l		  = KX022A_REG_XOUT_L,
+	.get_fifo_bytes	  = kx022a_get_fifo_bytes,
 };
 EXPORT_SYMBOL_NS_GPL(kx022a_chip_info, IIO_KX022A);
 

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

 	ret = devm_iio_trigger_register(dev, indio_trig);
 	if (ret)
 		return dev_err_probe(data->dev, ret,

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

index 3c31e7d88f78..43e32e688258 100644
@@ -11,6 +11,8 @@
 #include <linux/bits.h>
 #include <linux/regmap.h>
 
+#include <linux/iio/iio.h>
+
 #define KX022A_REG_WHO		0x0f
 #define KX022A_ID		0xc8
 

@@ -76,6 +78,39 @@
 
 struct device;
 
+struct kx022a_data {
+	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;
+
+	/* 3 x 16bit accel data + timestamp */
+	__le16 buffer[8] __aligned(IIO_DMA_MINALIGN);
+	struct {
+		__le16 channels[3];
+		s64 ts __aligned(8);
+	} scan;
+};
+
 /**
  * struct kx022a_chip_info - Kionix accelerometer chip specific information
  *

@@ -100,6 +135,7 @@ struct device;
  * @inc5:		interrupt control register 5
  * @inc6:		interrupt control register 6
  * @xout_l:		x-axis output least significant byte
+ * @get_fifo_bytes:	function pointer to get number of bytes in the FIFO buffer
  */
 struct kx022a_chip_info {
 	const char *name;

@@ -123,6 +159,7 @@ struct kx022a_chip_info {
 	u8 inc5;
 	u8 inc6;
 	u8 xout_l;
+	int (*get_fifo_bytes)(struct kx022a_data *);
 };
 
 int kx022a_probe_internal(struct device *dev, const struct kx022a_chip_info *chip_info);

Quoted 7 lines · On Thu, 20 Apr 2023 22:22:04 +0200 Mehdi Djait <[email protected]> wrote:

Since Kionix accelerometers use various numbers of bits to report data, a device-specific function is required.
Move the driver's private data to the header file to support the new function. Make the allocation of the "buffer" array in the fifo_flush function dynamic and more generic.

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

This results in some fifo_length changes in here and some in the previous patch. Either keep it fixed for first patch, then make those changes and the ones you have here in a single patch, or if that's hard maybe a single patch is cleaner.

I'd only expect the stuff about bytes in buffer to be in this patch.

Jonathan

Quoted 58 lines

---
v2:
- separated this change from the chip_info introduction and made it a patch in v2
- changed the function from generic implementation for to device-specific one
- removed blank lines pointed out by checkpatch
- changed the allocation of the "buffer" array in __kx022a_fifo_flush

 drivers/iio/accel/kionix-kx022a.c | 72 +++++++++++++------------------
 drivers/iio/accel/kionix-kx022a.h | 37 ++++++++++++++++
 2 files changed, 66 insertions(+), 43 deletions(-)

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

index 7f9a2c29790b..1c81ea1657b9 100644
@@ -150,36 +150,6 @@ static const struct regmap_config kx022a_regmap_config = {
 	.cache_type = REGCACHE_RBTREE,
 };
 
-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.
-	 */
-	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 *

@@ -340,7 +310,6 @@ static int kx022a_turn_on_off_unlocked(struct kx022a_data *data, bool on)
 		dev_err(data->dev, "Turn %s fail %d\n", str_on_off(on), ret);
 

drivers/iio/accel/kionix-kx022a.c static int kx022a_turn_on_off_unlocked(struct kx022a_data *data, bool on) · line 343

Quoted 2 lines

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

 	return ret;
-

Grumpy maintainer hat on: I don't want to see white space changes in unrelated code in a patch doing anything other than white space cleanup.

Quoted 20 lines

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

 }
 
 static int kx022a_turn_off_lock(struct kx022a_data *data)

@@ -595,34 +564,50 @@ static int kx022a_drop_fifo_contents(struct kx022a_data *data)
 	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;
 	uint64_t sample_period;
 	int count, fifo_bytes;
 	bool renable = false;

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

Quoted 10 lines

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

 	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;
-	}
+	/* 3 axis, 2 bytes of data for each of the axis */
+	buffer = kmalloc(data->chip_info->fifo_length * 6, GFP_KERNEL);

Split that 6 up into sizeof(*buffer) * 3
Then just comment on 3 axis. Or use a define for number of axis and drop the comment entirely.

Also, this feels like it fitst better in previous patch.

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

Quoted 9 lines

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

+	if (!buffer)
+		return -ENOMEM;
 
-	/* 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;
+	fifo_bytes = data->chip_info->get_fifo_bytes(data);

Only this small part is what this patch claims to do.. (+ the callback of course).

Quoted 29 lines

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

 	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)
+	if (!count) {
+		kfree(buffer);
 		return 0;
+	}
 
 	/*
 	 * If we are being called from IRQ handler we know the stored timestamp

@@ -704,6 +689,7 @@ static int __kx022a_fifo_flush(struct iio_dev *idev, unsigned int samples,
 	if (renable)
 		enable_irq(data->irq);
 
+	kfree(buffer);
 	return ret;
 }
 

@@ -1016,6 +1002,7 @@ const struct kx022a_chip_info kx022a_chip_info = {
 	.inc5		  = KX022A_REG_INC5,
 	.inc6		  = KX022A_REG_INC6,
 	.xout_l		  = KX022A_REG_XOUT_L,
+	.get_fifo_bytes	  = kx022a_get_fifo_bytes,
 };
 EXPORT_SYMBOL_NS_GPL(kx022a_chip_info, IIO_KX022A);
 

drivers/iio/accel/kionix-kx022a.c int kx022a_probe_internal(struct device *dev, const struct kx022a_chip_info *chi · line 1146

Quoted 5 lines

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

@@ -1143,7 +1130,6 @@ int kx022a_probe_internal(struct device *dev, const struct kx022a_chip_info *chi
 	if (ret)
 		return dev_err_probe(data->dev, ret, "Could not request IRQ\n");
 
-

Another one. If those predate your series, feel free to clean them up either at the start or end of this series.

Hi Mehdi,

kernel test robot noticed the following build warnings:

   686	{
   687		struct device *dev = regmap_get_device(data->regmap);
   688		__le16 buf_status;
   689		int ret, fifo_bytes;
   690	
   691		ret = regmap_bulk_read(data->regmap, data->chip_info->buf_status1,
   692				       &buf_status, sizeof(buf_status));
   693		if (ret) {
   694			dev_err(dev, "Error reading buffer status\n");
   695			return ret;
   696		}
   697	

Quoted 1 line

698 buf_status &= data->chip_info->buf_smp_lvl_mask;

699 fifo_bytes = le16_to_cpu(buf_status);

   701		return fifo_bytes;
   702	}

Quoted 3 lines · kernel test robot

Hi Mehdi,

kernel test robot noticed the following build warnings:

I believe it's not just a warning, it's a full functional error in the code.

Quoted 12 lines

   686	{
   687		struct device *dev = regmap_get_device(data->regmap);
   688		__le16 buf_status;
   689		int ret, fifo_bytes;
   690	
   691		ret = regmap_bulk_read(data->regmap, data->chip_info->buf_status1,
   692				       &buf_status, sizeof(buf_status));
   693		if (ret) {
   694			dev_err(dev, "Error reading buffer status\n");
   695			return ret;
   696		}
   697	

Quoted 2 lines

Quoted 1 line

698 buf_status &= data->chip_info->buf_smp_lvl_mask;

699 fifo_bytes = le16_to_cpu(buf_status);

You need to mask in the same endianess space, i.o.w. either on CPU or device side.

I believe you wanted to have fifo_bytes to be masked, but I'm not sure.

Hello Andy,

thank you for the review.

Quoted 22 lines · Andy Shevchenko

Quoted 3 lines · kernel test robot

Hi Mehdi,

kernel test robot noticed the following build warnings:

I believe it's not just a warning, it's a full functional error in the code.

Quoted 12 lines

   686	{
   687		struct device *dev = regmap_get_device(data->regmap);
   688		__le16 buf_status;
   689		int ret, fifo_bytes;
   690	
   691		ret = regmap_bulk_read(data->regmap, data->chip_info->buf_status1,
   692				       &buf_status, sizeof(buf_status));
   693		if (ret) {
   694			dev_err(dev, "Error reading buffer status\n");
   695			return ret;
   696		}
   697	

Quoted 2 lines

Quoted 1 line

698 buf_status &= data->chip_info->buf_smp_lvl_mask;

699 fifo_bytes = le16_to_cpu(buf_status);

Quoted 3 lines

You need to mask in the same endianess space, i.o.w. either on CPU or device side.

I believe you wanted to have fifo_bytes to be masked, but I'm not sure.

I wanted to read the registers buf_status_1 and buf_status_2 --> 16 bits and mask the result of the read to get the bits 0..9 which is the
buf_status: the number of bytes in the buffer

This is due to my lack of experience, but I have a question:
If I don't get any warnings when testing, how should I go about this ? I will obviously fix this, but this is for the future.