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

Remove blank lines pointed out by the checkpatch script

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

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

  drivers/iio/accel/kionix-kx022a.c | 2 --
  1 file changed, 2 deletions(-)

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

index f98393d74666..ff8aa7b9568e 100644
@@ -341,7 +341,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);
 
  	return ret;
-
  }
 
  static int kx022a_turn_off_lock(struct kx022a_data *data)

@@ -1121,7 +1120,6 @@ int kx022a_probe_internal(struct device *dev)
  	if (ret)
  		return dev_err_probe(data->dev, ret, "Could not request IRQ\n");
 
-
  	ret = devm_iio_trigger_register(dev, indio_trig);
  	if (ret)
  		return dev_err_probe(data->dev, ret,

Add the missing i2c device id

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

Quoted 3 lines · Mehdi Djait

Add the missing i2c device id

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

Reviewed-by Matti Vaittinen <[email protected]>

Regarding the question (Jonathan asked in previous version) if this really is a fix - I am unsure if this can be of help when dealing with non OF-systems? (I don't have much of recent experience on those).

Yours,
-- Matti

Add the missing i2c device id

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

+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, kx022a_i2c_id);

Quoted 1 line · Mehdi Djait

Add the missing i2c device id

Seems like unfinished commit message (also note missing period, that's why I paid attention to this).

...

drivers/iio/accel/kionix-kx022a-i2c.c static int kx022a_i2c_probe(struct i2c_client *i2c) · line 34

Quoted 2 lines

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

+static const struct i2c_device_id kx022a_i2c_id[] = {
+	{ .name = "kx022a", 0 },

', 0' is redundant.

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

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

---
v3:
- added the change of the buffer's allocation in the __kx022a_fifo_flush to this patch
- added the chip_info to the struct kx022a_data

v2:
- mentioned the introduction of the i2c_device_id table in the commit
- get i2c_/spi_get_device_id only when 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 |  15 +++-
  drivers/iio/accel/kionix-kx022a-spi.c |  15 +++-
  drivers/iio/accel/kionix-kx022a.c     | 114 +++++++++++++++++---------
  drivers/iio/accel/kionix-kx022a.h     |  54 +++++++++++-
  4 files changed, 147 insertions(+), 51 deletions(-)

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

index 8f23631a1fd3..ce299d0446f7 100644
@@ -15,6 +15,7 @@

static int __kx022a_fifo_flush(struct iio_dev *idev, unsigned int samples,

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

@@ -600,13 +600,17 @@ static int __kx022a_fifo_flush(struct iio_dev *idev, unsigned int samples,
  {
  	struct kx022a_data *data = iio_priv(idev);
  	struct device *dev = regmap_get_device(data->regmap);
-	__le16 buffer[KX022A_FIFO_LENGTH * 3];

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

+
  	ret = regmap_read(data->regmap, KX022A_REG_BUF_STATUS_1, &fifo_bytes);
  	if (ret) {
  		dev_err(dev, "Error reading buffer status\n");

@@ -621,8 +625,10 @@ static int __kx022a_fifo_flush(struct iio_dev *idev, unsigned int samples,
  		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

@@ -679,7 +685,7 @@ static int __kx022a_fifo_flush(struct iio_dev *idev, unsigned int samples,
  	}
 
  	fifo_bytes = count * KX022A_FIFO_SAMPLES_SIZE_BYTES;
-	ret = regmap_noinc_read(data->regmap, KX022A_REG_BUF_READ,
+	ret = regmap_noinc_read(data->regmap, data->chip_info->buf_read,
  				&buffer[0], fifo_bytes);
  	if (ret)
  		goto renable_out;

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

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

-int kx022a_probe_internal(struct device *dev)
+const struct kx022a_chip_info kx022a_chip_info = {
+	.name		  = "kx022-accel",
+	.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,

Quoted 31 lines · Mehdi Djait

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

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

---
v3:
- added the change of the buffer's allocation in the __kx022a_fifo_flush to this patch
- added the chip_info to the struct kx022a_data

v2:
- mentioned the introduction of the i2c_device_id table in the commit
- get i2c_/spi_get_device_id only when 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 |  15 +++-
  drivers/iio/accel/kionix-kx022a-spi.c |  15 +++-
  drivers/iio/accel/kionix-kx022a.c     | 114 +++++++++++++++++---------
  drivers/iio/accel/kionix-kx022a.h     |  54 +++++++++++-
  4 files changed, 147 insertions(+), 51 deletions(-)

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

index 8f23631a1fd3..ce299d0446f7 100644
@@ -15,6 +15,7 @@

...

Quoted 14 lines

static int __kx022a_fifo_flush(struct iio_dev *idev, unsigned int samples,

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

@@ -600,13 +600,17 @@ static int __kx022a_fifo_flush(struct iio_dev *idev, unsigned int samples,
  {
  	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;
  	int64_t tstamp;
  	int ret, i;
+	buffer = kmalloc(data->chip_info->fifo_length * KX022A_FIFO_SAMPLES_SIZE_BYTES, GFP_KERNEL);
+	if (!buffer)

drivers/iio/accel/kionix-kx022a-i2c.c static int __kx022a_fifo_flush(struct iio_dev *idev, unsigned int samples, · line 611

Quoted 1 line

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

+		return -ENOMEM;

Do you think we could get rid of allocating and freeing the buffer for each flush? I feel it is a bit wasteful, and with high sampling frequencies this function can be called quite often. Do you think there would be a way to either use stack (always reserve big enough buffer no matter which chip we have - or is the buffer too big to be safely taken from the stack?), or a buffer stored in private data and allocated at probe or buffer enable?

Also, please avoid such long lines. I know many people don't care about the line length - but for example I tend to have 3 terminal windows open side-by-side on my laptop screen. Hence long lines tend to be harder to read for me.

Quoted 30 lines

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

+
  	ret = regmap_read(data->regmap, KX022A_REG_BUF_STATUS_1, &fifo_bytes);
  	if (ret) {
  		dev_err(dev, "Error reading buffer status\n");

@@ -621,8 +625,10 @@ static int __kx022a_fifo_flush(struct iio_dev *idev, unsigned int samples,
  		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

@@ -679,7 +685,7 @@ static int __kx022a_fifo_flush(struct iio_dev *idev, unsigned int samples,
  	}
  	fifo_bytes = count * KX022A_FIFO_SAMPLES_SIZE_BYTES;
-	ret = regmap_noinc_read(data->regmap, KX022A_REG_BUF_READ,
+	ret = regmap_noinc_read(data->regmap, data->chip_info->buf_read,
  				&buffer[0], fifo_bytes);
  	if (ret)
  		goto renable_out;

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

...

Quoted 14 lines

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

-int kx022a_probe_internal(struct device *dev)
+const struct kx022a_chip_info kx022a_chip_info = {
+	.name		  = "kx022-accel",
+	.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,

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

Quoted 10 lines

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

+	.buf_clear	  = KX022A_REG_BUF_CLEAR,
+	.buf_status1	  = KX022A_REG_BUF_STATUS_1,
+	.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(kx022a_chip_info, IIO_KX022A);

Do you think the fields (or at least some of them) in this struct could be named based on the (main) functionality being used, not based on the register name? Something like "watermark_reg", "buf_en_reg", "reset_reg", "output_rate_reg", "int1_pinconf_reg", "int1_src_reg", "int2_pinconf_reg", "int1_src_reg" ...

I would not be at all surprized to see for example some IRQ control to be shifted from INC<X> to INC<Y> or cntl<X> / buf_cntl<X> stuff to be moved to cntl<Y> or to buf_cntl<Y> for next sensor we want to support. Especially when new cool feature is added to next sensor, resulting also adding a new cntl<Z> or buf_cntl<Z> or INC<Z>.

I, however, believe the _functionality_ will be there (in some register) - at least for the ICs for which we can re-use this driver. Hence, it might be nice - and if you can think of better names for these fields - to rename them based on the _functionality_ we use.

Another benefit would be added clarity to the code. Writing a value to "buf_en_reg", "watermark_reg" or to "int1_src_reg" is much clearer (to me) than writing a value to "buf_cntl1", "buf_cntl2" or "INC4". Especially if you don't have a datasheet at your hands.

I am not "demanding" this (at least not for now :]) because it seems these two Kionix sensors have been pretty consistent what comes to maintaining the same functionality in the registers with same naming - but I believe this is something that may in any case be lurking around the corner.

All in all, looks nice and clean to me! Good job.

Hi Matti,

Quoted 50 lines · Matti Vaittinen

Quoted 31 lines · Mehdi Djait

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

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

---
v3:
- added the change of the buffer's allocation in the __kx022a_fifo_flush to this patch
- added the chip_info to the struct kx022a_data

v2:
- mentioned the introduction of the i2c_device_id table in the commit
- get i2c_/spi_get_device_id only when 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 |  15 +++-
   drivers/iio/accel/kionix-kx022a-spi.c |  15 +++-
   drivers/iio/accel/kionix-kx022a.c     | 114 +++++++++++++++++---------
   drivers/iio/accel/kionix-kx022a.h     |  54 +++++++++++-
   4 files changed, 147 insertions(+), 51 deletions(-)

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

index 8f23631a1fd3..ce299d0446f7 100644
@@ -15,6 +15,7 @@

...

Quoted 14 lines

static int __kx022a_fifo_flush(struct iio_dev *idev, unsigned int samples,

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

@@ -600,13 +600,17 @@ static int __kx022a_fifo_flush(struct iio_dev *idev, unsigned int samples,
   {
   	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;
   	int64_t tstamp;
   	int ret, i;
+	buffer = kmalloc(data->chip_info->fifo_length * KX022A_FIFO_SAMPLES_SIZE_BYTES, GFP_KERNEL);
+	if (!buffer)

Quoted 8 lines

drivers/iio/accel/kionix-kx022a-i2c.c static int __kx022a_fifo_flush(struct iio_dev *idev, unsigned int samples, · line 611

Quoted 1 line

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

+		return -ENOMEM;

Do you think we could get rid of allocating and freeing the buffer for each flush? I feel it is a bit wasteful, and with high sampling frequencies this function can be called quite often. Do you think there would be a way to either use stack (always reserve big enough buffer no matter which chip we have - or is the buffer too big to be safely taken from the stack?), or a buffer stored in private data and allocated at probe or buffer enable?

I tried using the same allocation as before but a device like the KX127 has a fifo_length of 342 (compared to 86 for kx132, and 43 for kx022a). Allocating this much using the stack will result in a Warning.

Quoted 5 lines

Also, please avoid such long lines. I know many people don't care about the line length - but for example I tend to have 3 terminal windows open side-by-side on my laptop screen. Hence long lines tend to be harder to read for me.

That is the case for me also, but Jonathan asked me to change "fifo_length * 6" and the KX022A_FIFO_SAMPLES_SIZE_BYTES is already defined.

Quoted 80 lines

Quoted 31 lines

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

+
   	ret = regmap_read(data->regmap, KX022A_REG_BUF_STATUS_1, &fifo_bytes);
   	if (ret) {
   		dev_err(dev, "Error reading buffer status\n");

@@ -621,8 +625,10 @@ static int __kx022a_fifo_flush(struct iio_dev *idev, unsigned int samples,
   		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

@@ -679,7 +685,7 @@ static int __kx022a_fifo_flush(struct iio_dev *idev, unsigned int samples,
   	}
   	fifo_bytes = count * KX022A_FIFO_SAMPLES_SIZE_BYTES;
-	ret = regmap_noinc_read(data->regmap, KX022A_REG_BUF_READ,
+	ret = regmap_noinc_read(data->regmap, data->chip_info->buf_read,
   				&buffer[0], fifo_bytes);
   	if (ret)
   		goto renable_out;

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

...

Quoted 14 lines

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

-int kx022a_probe_internal(struct device *dev)
+const struct kx022a_chip_info kx022a_chip_info = {
+	.name		  = "kx022-accel",
+	.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,

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

Quoted 10 lines

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

+	.buf_clear	  = KX022A_REG_BUF_CLEAR,
+	.buf_status1	  = KX022A_REG_BUF_STATUS_1,
+	.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(kx022a_chip_info, IIO_KX022A);

Do you think the fields (or at least some of them) in this struct could be named based on the (main) functionality being used, not based on the register name? Something like "watermark_reg", "buf_en_reg", "reset_reg", "output_rate_reg", "int1_pinconf_reg", "int1_src_reg", "int2_pinconf_reg", "int1_src_reg" ...

I would not be at all surprized to see for example some IRQ control to be shifted from INC<X> to INC<Y> or cntl<X> / buf_cntl<X> stuff to be moved to cntl<Y> or to buf_cntl<Y> for next sensor we want to support. Especially when new cool feature is added to next sensor, resulting also adding a new cntl<Z> or buf_cntl<Z> or INC<Z>.

I, however, believe the _functionality_ will be there (in some register) - at least for the ICs for which we can re-use this driver. Hence, it might be nice - and if you can think of better names for these fields - to rename them based on the _functionality_ we use.

Another benefit would be added clarity to the code. Writing a value to "buf_en_reg", "watermark_reg" or to "int1_src_reg" is much clearer (to me) than writing a value to "buf_cntl1", "buf_cntl2" or "INC4". Especially if you don't have a datasheet at your hands.

Quoted 4 lines

I am not "demanding" this (at least not for now :]) because it seems these two Kionix sensors have been pretty consistent what comes to maintaining the same functionality in the registers with same naming - but I believe this is something that may in any case be lurking around the corner.

I agree, this seems to be the better solution. I will look into this.

Quoted 62 lines · Mehdi Djait

Hi Matti,

Quoted 58 lines · Matti Vaittinen

Quoted 31 lines · Mehdi Djait

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

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

---
v3:
- added the change of the buffer's allocation in the __kx022a_fifo_flush to this patch
- added the chip_info to the struct kx022a_data

v2:
- mentioned the introduction of the i2c_device_id table in the commit
- get i2c_/spi_get_device_id only when 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 |  15 +++-
   drivers/iio/accel/kionix-kx022a-spi.c |  15 +++-
   drivers/iio/accel/kionix-kx022a.c     | 114 +++++++++++++++++---------
   drivers/iio/accel/kionix-kx022a.h     |  54 +++++++++++-
   4 files changed, 147 insertions(+), 51 deletions(-)

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

index 8f23631a1fd3..ce299d0446f7 100644
@@ -15,6 +15,7 @@

...

Quoted 5 lines

static int __kx022a_fifo_flush(struct iio_dev *idev, unsigned int samples,

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

@@ -600,13 +600,17 @@ static int __kx022a_fifo_flush(struct iio_dev *idev, unsigned int samples,
   {
   	struct kx022a_data *data = iio_priv(idev);
   	struct device *dev = regmap_get_device(data->regmap);

drivers/iio/accel/kionix-kx022a-i2c.c static int __kx022a_fifo_flush(struct iio_dev *idev, unsigned int samples, · line 611

Quoted 10 lines

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

-	__le16 buffer[KX022A_FIFO_LENGTH * 3];
+	__le16 *buffer;
   	uint64_t sample_period;
   	int count, fifo_bytes;
   	bool renable = false;
   	int64_t tstamp;
   	int ret, i;
+	buffer = kmalloc(data->chip_info->fifo_length * KX022A_FIFO_SAMPLES_SIZE_BYTES, GFP_KERNEL);
+	if (!buffer)
+		return -ENOMEM;

Do you think we could get rid of allocating and freeing the buffer for each flush? I feel it is a bit wasteful, and with high sampling frequencies this function can be called quite often. Do you think there would be a way to either use stack (always reserve big enough buffer no matter which chip we have - or is the buffer too big to be safely taken from the stack?), or a buffer stored in private data and allocated at probe or buffer enable?

Quoted 4 lines

I tried using the same allocation as before but a device like the KX127 has a fifo_length of 342 (compared to 86 for kx132, and 43 for kx022a). Allocating this much using the stack will result in a Warning.

Right. Maybe you could then have the buffer in private-data and allocate it in buffer pre-enable? Do you think that would work?

Quoted 6 lines

Quoted 5 lines

Also, please avoid such long lines. I know many people don't care about the line length - but for example I tend to have 3 terminal windows open side-by-side on my laptop screen. Hence long lines tend to be harder to read for me.

Quoted 3 lines

That is the case for me also, but Jonathan asked me to change "fifo_length * 6" and the KX022A_FIFO_SAMPLES_SIZE_BYTES is already defined.

then please maybe split the line from appropriate point like:
buffer = kmalloc(data->chip_info->fifo_length *

		 KX022A_FIFO_SAMPLES_SIZE_BYTES, GFP_KERNEL);

Quoted 86 lines

Quoted 85 lines

Quoted 32 lines

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

+
   	ret = regmap_read(data->regmap, KX022A_REG_BUF_STATUS_1, &fifo_bytes);
   	if (ret) {
   		dev_err(dev, "Error reading buffer status\n");

@@ -621,8 +625,10 @@ static int __kx022a_fifo_flush(struct iio_dev *idev, unsigned int samples,
   		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

@@ -679,7 +685,7 @@ static int __kx022a_fifo_flush(struct iio_dev *idev, unsigned int samples,
   	}
   	fifo_bytes = count * KX022A_FIFO_SAMPLES_SIZE_BYTES;
-	ret = regmap_noinc_read(data->regmap, KX022A_REG_BUF_READ,
+	ret = regmap_noinc_read(data->regmap, data->chip_info->buf_read,
   				&buffer[0], fifo_bytes);
   	if (ret)
   		goto renable_out;

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

...

Quoted 14 lines

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

-int kx022a_probe_internal(struct device *dev)
+const struct kx022a_chip_info kx022a_chip_info = {
+	.name		  = "kx022-accel",
+	.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,

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

Quoted 10 lines

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

+	.buf_clear	  = KX022A_REG_BUF_CLEAR,
+	.buf_status1	  = KX022A_REG_BUF_STATUS_1,
+	.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(kx022a_chip_info, IIO_KX022A);

Do you think the fields (or at least some of them) in this struct could be named based on the (main) functionality being used, not based on the register name? Something like "watermark_reg", "buf_en_reg", "reset_reg", "output_rate_reg", "int1_pinconf_reg", "int1_src_reg", "int2_pinconf_reg", "int1_src_reg" ...

I would not be at all surprized to see for example some IRQ control to be shifted from INC<X> to INC<Y> or cntl<X> / buf_cntl<X> stuff to be moved to cntl<Y> or to buf_cntl<Y> for next sensor we want to support. Especially when new cool feature is added to next sensor, resulting also adding a new cntl<Z> or buf_cntl<Z> or INC<Z>.

I, however, believe the _functionality_ will be there (in some register) - at least for the ICs for which we can re-use this driver. Hence, it might be nice - and if you can think of better names for these fields - to rename them based on the _functionality_ we use.

Another benefit would be added clarity to the code. Writing a value to "buf_en_reg", "watermark_reg" or to "int1_src_reg" is much clearer (to me) than writing a value to "buf_cntl1", "buf_cntl2" or "INC4". Especially if you don't have a datasheet at your hands.

I am not "demanding" this (at least not for now :]) because it seems these two Kionix sensors have been pretty consistent what comes to maintaining the same functionality in the registers with same naming - but I believe this is something that may in any case be lurking around the corner.

Quoted 2 lines

I agree, this seems to be the better solution. I will look into this.

Thanks for going the extra mile :)

Hi Matti,

Quoted 68 lines · Matti Vaittinen

Quoted 66 lines · Mehdi Djait

Hi Matti,

Quoted 50 lines · Matti Vaittinen

Quoted 31 lines · Mehdi Djait

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

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

---
v3:
- added the change of the buffer's allocation in the __kx022a_fifo_flush to this patch
- added the chip_info to the struct kx022a_data

v2:
- mentioned the introduction of the i2c_device_id table in the commit
- get i2c_/spi_get_device_id only when 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 |  15 +++-
    drivers/iio/accel/kionix-kx022a-spi.c |  15 +++-
    drivers/iio/accel/kionix-kx022a.c     | 114 +++++++++++++++++---------
    drivers/iio/accel/kionix-kx022a.h     |  54 +++++++++++-
    4 files changed, 147 insertions(+), 51 deletions(-)

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

index 8f23631a1fd3..ce299d0446f7 100644
@@ -15,6 +15,7 @@

...

Quoted 14 lines

static int __kx022a_fifo_flush(struct iio_dev *idev, unsigned int samples,

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

@@ -600,13 +600,17 @@ static int __kx022a_fifo_flush(struct iio_dev *idev, unsigned int samples,
    {
    	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;
    	int64_t tstamp;
    	int ret, i;
+	buffer = kmalloc(data->chip_info->fifo_length * KX022A_FIFO_SAMPLES_SIZE_BYTES, GFP_KERNEL);
+	if (!buffer)

Quoted 8 lines

drivers/iio/accel/kionix-kx022a-i2c.c static int __kx022a_fifo_flush(struct iio_dev *idev, unsigned int samples, · line 611

Quoted 1 line

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

+		return -ENOMEM;

Do you think we could get rid of allocating and freeing the buffer for each flush? I feel it is a bit wasteful, and with high sampling frequencies this function can be called quite often. Do you think there would be a way to either use stack (always reserve big enough buffer no matter which chip we have - or is the buffer too big to be safely taken from the stack?), or a buffer stored in private data and allocated at probe or buffer enable?

I tried using the same allocation as before but a device like the KX127 has a fifo_length of 342 (compared to 86 for kx132, and 43 for kx022a). Allocating this much using the stack will result in a Warning.

Quoted 2 lines

Right. Maybe you could then have the buffer in private-data and allocate it in buffer pre-enable? Do you think that would work?

Do you mean add a new function kx022a_buffer_preenable to iio_buffer_setup_ops ?

Would adding the allocation to kx022a_fifo_enable and the free to kx022a_fifo_disable be a good option also ?

Quoted 71 lines · Mehdi Djait

Hi Matti,

Quoted 68 lines · Matti Vaittinen

Quoted 66 lines · Mehdi Djait

Hi Matti,

Quoted 50 lines · Matti Vaittinen

Quoted 31 lines · Mehdi Djait

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

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

---
v3:
- added the change of the buffer's allocation in the __kx022a_fifo_flush to this patch
- added the chip_info to the struct kx022a_data

v2:
- mentioned the introduction of the i2c_device_id table in the commit
- get i2c_/spi_get_device_id only when 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 |  15 +++-
    drivers/iio/accel/kionix-kx022a-spi.c |  15 +++-
    drivers/iio/accel/kionix-kx022a.c     | 114 +++++++++++++++++---------
    drivers/iio/accel/kionix-kx022a.h     |  54 +++++++++++-
    4 files changed, 147 insertions(+), 51 deletions(-)

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

index 8f23631a1fd3..ce299d0446f7 100644
@@ -15,6 +15,7 @@  

...

Quoted 14 lines

static int __kx022a_fifo_flush(struct iio_dev *idev, unsigned int samples,

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

@@ -600,13 +600,17 @@ static int __kx022a_fifo_flush(struct iio_dev *idev, unsigned int samples,
    {
    	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;
    	int64_t tstamp;
    	int ret, i;
+	buffer = kmalloc(data->chip_info->fifo_length * KX022A_FIFO_SAMPLES_SIZE_BYTES, GFP_KERNEL);
+	if (!buffer)

Quoted 8 lines

drivers/iio/accel/kionix-kx022a-i2c.c static int __kx022a_fifo_flush(struct iio_dev *idev, unsigned int samples, · line 611

Quoted 1 line

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

+		return -ENOMEM;  

Do you think we could get rid of allocating and freeing the buffer for each flush? I feel it is a bit wasteful, and with high sampling frequencies this function can be called quite often. Do you think there would be a way to either use stack (always reserve big enough buffer no matter which chip we have - or is the buffer too big to be safely taken from the stack?), or a buffer stored in private data and allocated at probe or buffer enable?

I tried using the same allocation as before but a device like the KX127 has a fifo_length of 342 (compared to 86 for kx132, and 43 for kx022a). Allocating this much using the stack will result in a Warning.

Quoted 4 lines

Quoted 2 lines

Right. Maybe you could then have the buffer in private-data and allocate it in buffer pre-enable? Do you think that would work?

Do you mean add a new function kx022a_buffer_preenable to iio_buffer_setup_ops ?

Sorry. I thought the kx022a already implemented the pre-enable callback but it was the postenable. I was mistaken.

Quoted 3 lines

Would adding the allocation to kx022a_fifo_enable and the free to kx022a_fifo_disable be a good option also ?

Yes. I think that should work!

Yours,
-- Matti

Quoted 51 lines

Quoted 50 lines

Quoted 13 lines

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

+const struct kx022a_chip_info kx022a_chip_info = {
+	.name		  = "kx022-accel",
+	.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,

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

Quoted 10 lines

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

+	.buf_clear	  = KX022A_REG_BUF_CLEAR,
+	.buf_status1	  = KX022A_REG_BUF_STATUS_1,
+	.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(kx022a_chip_info, IIO_KX022A);

Do you think the fields (or at least some of them) in this struct could be named based on the (main) functionality being used, not based on the register name? Something like "watermark_reg", "buf_en_reg", "reset_reg", "output_rate_reg", "int1_pinconf_reg", "int1_src_reg", "int2_pinconf_reg", "int1_src_reg" ...

I would not be at all surprized to see for example some IRQ control to be shifted from INC<X> to INC<Y> or cntl<X> / buf_cntl<X> stuff to be moved to cntl<Y> or to buf_cntl<Y> for next sensor we want to support. Especially when new cool feature is added to next sensor, resulting also adding a new cntl<Z> or buf_cntl<Z> or INC<Z>.

I, however, believe the _functionality_ will be there (in some register) - at least for the ICs for which we can re-use this driver. Hence, it might be nice - and if you can think of better names for these fields - to rename them based on the _functionality_ we use.

Another benefit would be added clarity to the code. Writing a value to "buf_en_reg", "watermark_reg" or to "int1_src_reg" is much clearer (to me) than writing a value to "buf_cntl1", "buf_cntl2" or "INC4". Especially if you don't have a datasheet at your hands.

I am not "demanding" this (at least not for now :]) because it seems these two Kionix sensors have been pretty consistent what comes to maintaining the same functionality in the registers with same naming - but I believe this is something that may in any case be lurking around the corner.

Quoted 2 lines

I agree, this seems to be the better solution. I will look into this.

Thanks for going the extra mile :)

Hello Matti,

Quoted 51 lines · Matti Vaittinen

Quoted 51 lines

Quoted 50 lines

Quoted 13 lines

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

+const struct kx022a_chip_info kx022a_chip_info = {
+	.name		  = "kx022-accel",
+	.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,

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

Quoted 10 lines

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

+	.buf_clear	  = KX022A_REG_BUF_CLEAR,
+	.buf_status1	  = KX022A_REG_BUF_STATUS_1,
+	.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(kx022a_chip_info, IIO_KX022A);

Do you think the fields (or at least some of them) in this struct could be named based on the (main) functionality being used, not based on the register name? Something like "watermark_reg", "buf_en_reg", "reset_reg", "output_rate_reg", "int1_pinconf_reg", "int1_src_reg", "int2_pinconf_reg", "int1_src_reg" ...

I would not be at all surprized to see for example some IRQ control to be shifted from INC<X> to INC<Y> or cntl<X> / buf_cntl<X> stuff to be moved to cntl<Y> or to buf_cntl<Y> for next sensor we want to support. Especially when new cool feature is added to next sensor, resulting also adding a new cntl<Z> or buf_cntl<Z> or INC<Z>.

I, however, believe the _functionality_ will be there (in some register) - at least for the ICs for which we can re-use this driver. Hence, it might be nice - and if you can think of better names for these fields - to rename them based on the _functionality_ we use.

Another benefit would be added clarity to the code. Writing a value to "buf_en_reg", "watermark_reg" or to "int1_src_reg" is much clearer (to me) than writing a value to "buf_cntl1", "buf_cntl2" or "INC4". Especially if you don't have a datasheet at your hands.

I am not "demanding" this (at least not for now :]) because it seems these two Kionix sensors have been pretty consistent what comes to maintaining the same functionality in the registers with same naming - but I believe this is something that may in any case be lurking around the corner.

Quoted 4 lines

Quoted 2 lines

I agree, this seems to be the better solution. I will look into this.

Thanks for going the extra mile :)

I am reconsidering the renaming of the fields

1. inc{1,4,5,6} get assigned once to data->{ien_reg,inc_reg} in the probe function and then never used again

2. buf_cntl2 is used for enabling the buffer and changing the resolution to 16bits, so which name is better than buf_cntl ?

3. cntl seems the most appropriate name, again different functions and the same reg

4. who, id, xout_l, odcntl, buf_{clear, status, read} are quite straightforward

Anyway this is my opinion, what do you think ?

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

Hi Mehdi,

Quoted 4 lines · Mehdi Djait

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

Could you please split this in different patches? Add id in one patch and refactor in a different patch. Please, also the refactorings need to be split.

I see here that this is a general code cleanup, plus some other stuff.

[...]

drivers/iio/accel/kionix-kx022a-spi.c static int kx022a_spi_probe(struct spi_device *spi) · line 29

Quoted 9 lines

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

@@ -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;

you don't need the cast here... if you don't find it messy, I wouldn't mind this form... some hate it, I find it easier to read:

	chip_info = spi_get_device_id(spi)->driver_data;

your choice.

Hi Andi,

Thank you for the review.

Quoted 8 lines · Andi Shyti

Hi Mehdi,

Quoted 4 lines · Mehdi Djait

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

Quoted 6 lines

Could you please split this in different patches? Add id in one patch and refactor in a different patch. Please, also the refactorings need to be split.

I see here that this is a general code cleanup, plus some other stuff.

Looking at the diff and considering the comments from Jonathan in the previous versions, the only thing that can separated from this patch would be the changes related to:

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

-#define KX022A_ACCEL_CHAN(axis, index)				\
+#define KX022A_ACCEL_CHAN(axis, reg, index)			\

Quoted 18 lines

[...]

drivers/iio/accel/kionix-kx022a-spi.c static int kx022a_spi_probe(struct spi_device *spi) · line 29

Quoted 9 lines

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

@@ -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;  

you don't need the cast here... if you don't find it messy, I wouldn't mind this form... some hate it, I find it easier to read:

Quoted 3 lines

	chip_info = spi_get_device_id(spi)->driver_data;

your choice.

I don't really have any strong opinion about this other than keeping the same style used in iio drivers

Again thank you for the review

--
Kind Regards
Mehdi Djait

Quoted 2 lines · On Sat, 29 Apr 2023 15:07:46 +0200 Mehdi Djait <[email protected]> wrote:

Quoted 1 line · Andi Shyti

On Tue, Apr 25, 2023 at 12:22:25AM +0200, Mehdi Djait wrote:

Quoted 13 lines

Quoted 12 lines

drivers/iio/accel/kionix-kx022a-spi.c static int kx022a_spi_probe(struct spi_device *spi) · line 29

Quoted 4 lines

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

+	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;  

you don't need the cast here... if you don't find it messy, I wouldn't mind this form... some hate it, I find it easier to read:

	chip_info = spi_get_device_id(spi)->driver_data;

your choice.

Quoted 4 lines

I don't really have any strong opinion about this other than keeping the same style used in iio drivers

Again thank you for the review

I'm fairly sure the cast is needed because driver_data is (via defines) an unsigned long, which you cannot implicitly cast to a pointer without various warnings being generated.

Quoted 4 lines · Jonathan Cameron

Quoted 2 lines · On Sat, 29 Apr 2023 15:07:46 +0200 Mehdi Djait <[email protected]> wrote:

Quoted 1 line · Andi Shyti

On Tue, Apr 25, 2023 at 12:22:25AM +0200, Mehdi Djait wrote:

...

Quoted 16 lines

Quoted 16 lines

Quoted 9 lines

drivers/iio/accel/kionix-kx022a-spi.c static int kx022a_spi_probe(struct spi_device *spi) · line 29

Quoted 4 lines

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

+	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;  

you don't need the cast here... if you don't find it messy, I wouldn't mind this form... some hate it, I find it easier to read:

Quoted 3 lines

	chip_info = spi_get_device_id(spi)->driver_data;

your choice.

I don't really have any strong opinion about this other than keeping the same style used in iio drivers

Quoted 5 lines

Quoted 1 line

Again thank you for the review

I'm fairly sure the cast is needed because driver_data is (via defines) an unsigned long, which you cannot implicitly cast to a pointer without various warnings being generated.

Instead we have a specific SPI provided helper for the above, please use it instead of open coded stuff.

Since Kionix accelerometers use various numbers of bits to report data, a device-specific function is required.
Implement the function as a callback in the device-specific chip_info structure

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

---
v3:
- fixed the warning of the kernel test robot in kx132_get_fifo_bytes

(invalid assignment: &=, left side has type restricted __le16 right side has type unsigned short)

v2:
- mentioned the kx132-1211 in the Kconfig
- added a kx132-specific get_fifo_bytes function
- changed the device name from "kx132" to "kx132-1211

  drivers/iio/accel/kionix-kx022a.c | 30 ++++++++++++++++++++----------
  drivers/iio/accel/kionix-kx022a.h |  4 ++++
  2 files changed, 24 insertions(+), 10 deletions(-)

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

index 66da3c8c3fd0..4a31d17c1f22 100644
@@ -595,11 +595,28 @@ static int kx022a_drop_fifo_contents(struct kx022a_data *data)
  	return regmap_write(data->regmap, data->chip_info->buf_clear, 0x0);
  }
 
+static int kx022a_get_fifo_bytes(struct kx022a_data *data)
+{

Quoted 8 lines · Mehdi Djait

Since Kionix accelerometers use various numbers of bits to report data, a device-specific function is required.
Implement the function as a callback in the device-specific chip_info structure

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

---
v3:
- fixed the warning of the kernel test robot in kx132_get_fifo_bytes

Do you mean kx022a_get_fifo_bytes? (I guess this won't go to commit logs so no need to respin for this).

Quoted 21 lines

(invalid assignment: &=, left side has type restricted __le16 right side has type unsigned short)

v2:
- mentioned the kx132-1211 in the Kconfig
- added a kx132-specific get_fifo_bytes function
- changed the device name from "kx132" to "kx132-1211

  drivers/iio/accel/kionix-kx022a.c | 30 ++++++++++++++++++++----------
  drivers/iio/accel/kionix-kx022a.h |  4 ++++
  2 files changed, 24 insertions(+), 10 deletions(-)

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

index 66da3c8c3fd0..4a31d17c1f22 100644
@@ -595,11 +595,28 @@ static int kx022a_drop_fifo_contents(struct kx022a_data *data)
  	return regmap_write(data->regmap, data->chip_info->buf_clear, 0x0);
  }
+static int kx022a_get_fifo_bytes(struct kx022a_data *data)
+{

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

Quoted 10 lines

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

+	struct device *dev = regmap_get_device(data->regmap);
+	int ret, fifo_bytes;
+
+	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 */

This comment would need fixing. I guess this is my bad - this comment goes back to first internal draft datasheets where the fifo lenght was said to be shorter than 255 bytes...

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

Quoted 2 lines

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

+	if (fifo_bytes == KX022A_FIFO_FULL_VALUE)
+		fifo_bytes = KX022A_FIFO_MAX_BYTES;

...Currently the fifo size (KX022A_FIFO_MAX_BYTES) is 258 bytes and KX022A_FIFO_FULL_VALUE is 255 (8-bit register) - meaning that no matter what noise there is in the I2C line, we won't overflow the buffer. This check is here to handle the 'quirk' where full 258 bytes of FIFO data is advertised by register max value (255).

It wouldn't be fair to require fixing this in context of this change - but I guess I can ask you to please update the comment if you happen to re-work this file ;)

Kionix KX132-1211 is a tri-axis 16-bit accelerometer that can support ranges from ±2G to ±16G, digital output through I²C/SPI.
Add support for basic accelerometer features such as reading acceleration via IIO using raw reads, triggered buffer (data-ready), or 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/Kconfig             |   8 +-
  drivers/iio/accel/kionix-kx022a-i2c.c |   2 +
  drivers/iio/accel/kionix-kx022a-spi.c |   2 +
  drivers/iio/accel/kionix-kx022a.c     | 147 ++++++++++++++++++++++++++
  drivers/iio/accel/kionix-kx022a.h     |  52 +++++++++
  5 files changed, 207 insertions(+), 4 deletions(-)

drivers/iio/accel/Kconfig +4 -4

index b6b45d359f28..d8cc6e6f2bb9 100644
@@ -418,8 +418,8 @@ config IIO_KX022A_SPI
  	select IIO_KX022A
  	select REGMAP_SPI
  	help
-	  Enable support for the Kionix KX022A digital tri-axis
-	  accelerometer connected to I2C interface.
+	  Enable support for the Kionix KX022A, KX132-1211 digital tri-axis
+	  accelerometers connected to SPI interface.
 
  config IIO_KX022A_I2C
  	tristate "Kionix KX022A tri-axis digital accelerometer I2C interface"

@@ -427,8 +427,8 @@ config IIO_KX022A_I2C
  	select IIO_KX022A
  	select REGMAP_I2C
  	help
-	  Enable support for the Kionix KX022A digital tri-axis
-	  accelerometer connected to I2C interface.
+	  Enable support for the Kionix KX022A, KX132-1211 digital tri-axis
+	  accelerometers connected to I2C interface.
 
  config KXSD9
  	tristate "Kionix KXSD9 Accelerometer Driver"

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

index ce299d0446f7..4ea28d2482ec 100644
@@ -39,12 +39,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)&kx022a_chip_info },
+	{ .name = "kx132-1211", .driver_data = (kernel_ulong_t)&kx132_chip_info },
  	{ }
  };
  MODULE_DEVICE_TABLE(i2c, kx022a_i2c_id);
 
  static const struct of_device_id kx022a_of_match[] = {
  	{ .compatible = "kionix,kx022a", .data = &kx022a_chip_info },
+	{ .compatible = "kionix,kx132-1211", .data = &kx132_chip_info },
  	{ }
  };
  MODULE_DEVICE_TABLE(of, kx022a_of_match);

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

index b84503e24510..b755b2b395ed 100644
@@ -39,12 +39,14 @@ static int kx022a_spi_probe(struct spi_device *spi)
 
  static const struct spi_device_id kx022a_id[] = {
  	{ .name = "kx022a", .driver_data = (kernel_ulong_t)&kx022a_chip_info },
+	{ .name = "kx132-1211", .driver_data = (kernel_ulong_t)&kx132_chip_info },
  	{ }
  };
  MODULE_DEVICE_TABLE(spi, kx022a_id);
 
  static const struct of_device_id kx022a_of_match[] = {
  	{ .compatible = "kionix,kx022a", .data = &kx022a_chip_info },
+	{ .compatible = "kionix,kx132-1211", .data = &kx132_chip_info },
  	{ }
  };
  MODULE_DEVICE_TABLE(of, kx022a_of_match);

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

index 4a31d17c1f22..a6808ab12162 100644
@@ -150,6 +150,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,

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

+
+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[] = {
+	{

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

+	}, {
+		.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,
+};
+
  struct kx022a_data {
  	const struct kx022a_chip_info *chip_info;
  	struct regmap *regmap;

@@ -237,6 +332,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, KX132_REG_XOUT_L, 0),
+	KX022A_ACCEL_CHAN(Y, KX132_REG_YOUT_L, 1),
+	KX022A_ACCEL_CHAN(Z, KX132_REG_ZOUT_L, 2),
+	IIO_CHAN_SOFT_TIMESTAMP(3),
+};
+
  /*
   * The sensor HW can support ODR up to 1600 Hz, which is beyond what most of the
   * Linux CPUs can handle without dropping samples. Also, the low power mode is

@@ -613,6 +715,25 @@ static int kx022a_get_fifo_bytes(struct kx022a_data *data)
  	return fifo_bytes;
  }
 
+static int kx132_get_fifo_bytes(struct kx022a_data *data)
+{
+	struct device *dev = regmap_get_device(data->regmap);
+	__le16 buf_status;
+	int ret, fifo_bytes;

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

+
+	return fifo_bytes;
+}
+
  static int __kx022a_fifo_flush(struct iio_dev *idev, unsigned int samples,
  			       bool irq)
  {

@@ -1036,6 +1157,32 @@ const struct kx022a_chip_info kx022a_chip_info = {
  };
  EXPORT_SYMBOL_NS_GPL(kx022a_chip_info, IIO_KX022A);
 
+const struct kx022a_chip_info kx132_chip_info = {
+	.name		  = "kx132-1211",
+	.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,
+	.get_fifo_bytes	  = kx132_get_fifo_bytes,
+};
+EXPORT_SYMBOL_NS_GPL(kx132_chip_info, IIO_KX022A);
+
  int kx022a_probe_internal(struct device *dev, const struct kx022a_chip_info *chip_info)
  {
  	static const char * const regulator_names[] = {"io-vdd", "vdd"};

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

index f043767067b7..1f4135cf20eb 100644
@@ -74,6 +74,57 @@
  #define KX022A_REG_SELF_TEST	0x60
  #define KX022A_MAX_REGISTER	0x60
 
+#define KX132_REG_WHO		0x13
+#define KX132_ID		0x3d
+
+#define KX132_FIFO_LENGTH	86
+
+#define KX132_REG_CNTL		0x1b
+#define KX132_REG_CNTL2		0x1c
+#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

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

+
+#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		KX022A_MASK_IPOL
+#define KX132_ITYP_PULSE	KX022A_MASK_ITYP
+
+#define KX132_REG_INC4		0x25
+
+#define KX132_REG_SELF_TEST	0x5d
+#define KX132_MAX_REGISTER	0x76
+
  struct device;
 
  struct kx022a_data;

Quoted 6 lines

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

+static int kx132_get_fifo_bytes(struct kx022a_data *data)
+{
+	struct device *dev = regmap_get_device(data->regmap);
+	__le16 buf_status;
+	int ret, fifo_bytes;

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

Quoted 10 lines

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

+
+	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;
+	}
+
+	fifo_bytes = le16_to_cpu(buf_status);
+	fifo_bytes &= data->chip_info->buf_smp_lvl_mask;  

This is probably just my limitation but I've hard time thinking how this works out on BE machines. It'd be much easier for me to understand this if the data was handled as two u8 values and mask was applied before endianes conversion. (Eg - untested pseudo code follows;

__le16 buf_status;
u8 *reg_data;

...

ret = regmap_bulk_read(data->regmap, data->chip_info->buf_status1,

			&buf_status, sizeof(buf_status));

...

reg_data = (u8 *)&buf_status;

/* Clear the unused bits form 2.nd reg */ reg_data[1] = reg_data[i] & MASK_SMP_LVL_REG_HIGH_BITS;

/* Convert to CPU endianess */
fifo_bytes = le16_to_cpu(buf_status);

Well, others may have different view on this :)

Kionix KX132-1211 is a tri-axis 16-bit accelerometer that can support ranges from ±2G to ±16G, digital output through I²C/SPI.
Add support for basic accelerometer features such as reading acceleration via IIO using raw reads, triggered buffer (data-ready), or 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.c +5 -0

+static int kx132_get_fifo_bytes(struct kx022a_data *data)
+{
+	struct device *dev = regmap_get_device(data->regmap);
+	__le16 buf_status;
+	int ret, fifo_bytes;

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

+
+	return fifo_bytes;
+}
+
 static int __kx022a_fifo_flush(struct iio_dev *idev, unsigned int samples,
 			       bool irq)
 {

@@ -1036,6 +1157,32 @@ const struct kx022a_chip_info kx022a_chip_info = {
 };
 EXPORT_SYMBOL_NS_GPL(kx022a_chip_info, IIO_KX022A);
 
+const struct kx022a_chip_info kx132_chip_info = {
+	.name		  = "kx132-1211",
+	.regmap_config	  = &kx132_regmap_config,
+	.channels	  = kx132_channels,
+	.num_channels	  = ARRAY_SIZE(kx132_channels),
+	.fifo_length	  = KX132_FIFO_LENGTH,

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

+	.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,
+	.get_fifo_bytes	  = kx132_get_fifo_bytes,
+};
+EXPORT_SYMBOL_NS_GPL(kx132_chip_info, IIO_KX022A);
+
 int kx022a_probe_internal(struct device *dev, const struct kx022a_chip_info *chip_info)
 {
 	static const char * const regulator_names[] = {"io-vdd", "vdd"};

Quoted 7 lines · On Tue, 25 Apr 2023 00:22:27 +0200 Mehdi Djait <[email protected]> wrote:

Kionix KX132-1211 is a tri-axis 16-bit accelerometer that can support ranges from ±2G to ±16G, digital output through I²C/SPI.
Add support for basic accelerometer features such as reading acceleration via IIO using raw reads, triggered buffer (data-ready), or 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]>

Two tiny things inline.

Quoted 10 lines

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

+static int kx132_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");

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

Quoted 5 lines

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

+		return ret;
+	}
+
+	fifo_bytes = le16_to_cpu(buf_status);
+	fifo_bytes &= data->chip_info->buf_smp_lvl_mask;

Slight preference for FIELD_GET() as it saves me checking the mask includes lowest bits.

Quoted 25 lines

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

+
+	return fifo_bytes;
+}
+
 static int __kx022a_fifo_flush(struct iio_dev *idev, unsigned int samples,
 			       bool irq)
 {

@@ -1036,6 +1157,32 @@ const struct kx022a_chip_info kx022a_chip_info = {
 };
 EXPORT_SYMBOL_NS_GPL(kx022a_chip_info, IIO_KX022A);
 
+const struct kx022a_chip_info kx132_chip_info = {
+	.name		  = "kx132-1211",
+	.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,

drivers/iio/accel/kionix-kx022a.c const struct kx022a_chip_info kx022a_chip_info = { · line 1175

Quoted 4 lines

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

+	.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,

There are some things in here (typically where the define isn't used anywhere else) where I think it would be easier to follow if the value was listed here. Masks and IDs for example.

Hello Jonathan,

Quoted 23 lines · Jonathan Cameron

Quoted 7 lines · On Tue, 25 Apr 2023 00:22:27 +0200 Mehdi Djait <[email protected]> wrote:

Kionix KX132-1211 is a tri-axis 16-bit accelerometer that can support ranges from ±2G to ±16G, digital output through I²C/SPI.
Add support for basic accelerometer features such as reading acceleration via IIO using raw reads, triggered buffer (data-ready), or 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]>

Two tiny things inline.

Quoted 10 lines

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

+static int kx132_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");

Quoted 8 lines

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

Quoted 5 lines

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

+		return ret;
+	}
+
+	fifo_bytes = le16_to_cpu(buf_status);
+	fifo_bytes &= data->chip_info->buf_smp_lvl_mask;  

Slight preference for FIELD_GET() as it saves me checking the mask includes lowest bits.

This will mean I have the remove the chip_info member buf_smp_lvl_mask and use KX132_MASK_BUF_SMP_LVL directly because otherwise the __builtin_constant_p function will cause an error when building.
Check: https://elixir.bootlin.com/linux/latest/source/include/linux/bitfield.h#L65

I can change it to FIELD_GET() if you want to.

Quoted 26 lines

Quoted 26 lines

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

+
+	return fifo_bytes;
+}
+
 static int __kx022a_fifo_flush(struct iio_dev *idev, unsigned int samples,
 			       bool irq)
 {

@@ -1036,6 +1157,32 @@ const struct kx022a_chip_info kx022a_chip_info = {
 };
 EXPORT_SYMBOL_NS_GPL(kx022a_chip_info, IIO_KX022A);
 
+const struct kx022a_chip_info kx132_chip_info = {
+	.name		  = "kx132-1211",
+	.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,

Quoted 9 lines

drivers/iio/accel/kionix-kx022a.c const struct kx022a_chip_info kx022a_chip_info = { · line 1175

Quoted 4 lines

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

+	.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,  

There are some things in here (typically where the define isn't used anywhere else) where I think it would be easier to follow if the value was listed here. Masks and IDs for example.

After removing buf_smp_lvl_mask, which members will be easier to understand (besides id) ?

--
Kind Regards
Mehdi Djait

Quoted 40 lines · On Fri, 5 May 2023 20:11:33 +0200 Mehdi Djait <[email protected]> wrote:

Hello Jonathan,

Quoted 23 lines · Jonathan Cameron

Quoted 7 lines · On Tue, 25 Apr 2023 00:22:27 +0200 Mehdi Djait <[email protected]> wrote:

Kionix KX132-1211 is a tri-axis 16-bit accelerometer that can support ranges from ±2G to ±16G, digital output through I²C/SPI.
Add support for basic accelerometer features such as reading acceleration via IIO using raw reads, triggered buffer (data-ready), or 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]>

Two tiny things inline.

Quoted 10 lines

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

+static int kx132_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");

Quoted 8 lines

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

Quoted 5 lines

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

+		return ret;
+	}
+
+	fifo_bytes = le16_to_cpu(buf_status);
+	fifo_bytes &= data->chip_info->buf_smp_lvl_mask;  

Slight preference for FIELD_GET() as it saves me checking the mask includes lowest bits.

This will mean I have the remove the chip_info member buf_smp_lvl_mask and use KX132_MASK_BUF_SMP_LVL directly because otherwise the __builtin_constant_p function will cause an error when building.
Check: https://elixir.bootlin.com/linux/latest/source/include/linux/bitfield.h#L65

Quoted 1 line

I can change it to FIELD_GET() if you want to.

Good point. You could use le16_get_bits() though which I'm fairly sure will take a variable just fine.

Quoted 36 lines

Quoted 35 lines

Quoted 20 lines

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

+
+	return fifo_bytes;
+}
+
 static int __kx022a_fifo_flush(struct iio_dev *idev, unsigned int samples,
 			       bool irq)
 {

@@ -1036,6 +1157,32 @@ const struct kx022a_chip_info kx022a_chip_info = {
 };
 EXPORT_SYMBOL_NS_GPL(kx022a_chip_info, IIO_KX022A);
 
+const struct kx022a_chip_info kx132_chip_info = {
+	.name		  = "kx132-1211",
+	.regmap_config	  = &kx132_regmap_config,
+	.channels	  = kx132_channels,
+	.num_channels	  = ARRAY_SIZE(kx132_channels),
+	.fifo_length	  = KX132_FIFO_LENGTH,

drivers/iio/accel/kionix-kx022a.c const struct kx022a_chip_info kx022a_chip_info = { · line 1175

Quoted 10 lines

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

+	.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,  

There are some things in here (typically where the define isn't used anywhere else) where I think it would be easier to follow if the value was listed here. Masks and IDs for example.

Quoted 1 line

After removing buf_smp_lvl_mask, which members will be easier to understand (besides id) ?

I haven't gone through them. Length seems an obvious one. It's a number not a magic value. Register addresses might also be simpler if they aren't used elsewhere.

Not really about understanding just about a define that adds nothing if all we do is assign it to a variable of the same name.

Hello Jonathan,

Quoted 43 lines · Jonathan Cameron

Quoted 40 lines · On Fri, 5 May 2023 20:11:33 +0200 Mehdi Djait <[email protected]> wrote:

Hello Jonathan,

Quoted 23 lines · Jonathan Cameron

Quoted 7 lines · On Tue, 25 Apr 2023 00:22:27 +0200 Mehdi Djait <[email protected]> wrote:

Kionix KX132-1211 is a tri-axis 16-bit accelerometer that can support ranges from ±2G to ±16G, digital output through I²C/SPI.
Add support for basic accelerometer features such as reading acceleration via IIO using raw reads, triggered buffer (data-ready), or 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]>

Two tiny things inline.

Quoted 10 lines

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

+static int kx132_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");

Quoted 8 lines

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

Quoted 5 lines

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

+		return ret;
+	}
+
+	fifo_bytes = le16_to_cpu(buf_status);
+	fifo_bytes &= data->chip_info->buf_smp_lvl_mask;    

Slight preference for FIELD_GET() as it saves me checking the mask includes lowest bits.

This will mean I have the remove the chip_info member buf_smp_lvl_mask and use KX132_MASK_BUF_SMP_LVL directly because otherwise the __builtin_constant_p function will cause an error when building.
Check: https://elixir.bootlin.com/linux/latest/source/include/linux/bitfield.h#L65

Quoted 5 lines

Quoted 1 line

I can change it to FIELD_GET() if you want to.

Good point. You could use le16_get_bits() though which I'm fairly sure will take a variable just fine.

I don't think it will work.

From the commit log introducing the {type}_get_bits to <linux/bitfield.h>
" Field specification must be a constant; __builtin_constant_p() doesn't
have to be true for it, but compiler must be able to evaluate it at build time. If it cannot or if the value does not encode any bitfield, the build will fail. "

Actually Geert Uytterhoeven tried to solve excatly this issue, but it
seems that the patch was not accepted.
Check: https://lore.kernel.org/linux-iio/3a54a6703879d10f08cf0275a2a69297ebd2b1d4.1637592133.git.geert+renesas@glider.be/

So which solution would be the best:

1. Use directly KX132_MASK_BUF_SMP_LVL, the only reason I was trying to avoid this was to make this function generic enough for other chip variants

2. Copy the field_get() definition from drivers/clk/at91 or from the commit of Geert and use it in this driver

3. leave it as it is ?

4. another solution ?

Quoted 36 lines

Quoted 36 lines

Quoted 35 lines

Quoted 20 lines

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

+
+	return fifo_bytes;
+}
+
 static int __kx022a_fifo_flush(struct iio_dev *idev, unsigned int samples,
 			       bool irq)
 {

@@ -1036,6 +1157,32 @@ const struct kx022a_chip_info kx022a_chip_info = {
 };
 EXPORT_SYMBOL_NS_GPL(kx022a_chip_info, IIO_KX022A);
 
+const struct kx022a_chip_info kx132_chip_info = {
+	.name		  = "kx132-1211",
+	.regmap_config	  = &kx132_regmap_config,
+	.channels	  = kx132_channels,
+	.num_channels	  = ARRAY_SIZE(kx132_channels),
+	.fifo_length	  = KX132_FIFO_LENGTH,

drivers/iio/accel/kionix-kx022a.c const struct kx022a_chip_info kx022a_chip_info = { · line 1175

Quoted 10 lines

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

+	.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,    

There are some things in here (typically where the define isn't used anywhere else) where I think it would be easier to follow if the value was listed here. Masks and IDs for example.

Quoted 7 lines

Quoted 1 line

After removing buf_smp_lvl_mask, which members will be easier to understand (besides id) ?

I haven't gone through them. Length seems an obvious one. It's a number not a magic value. Register addresses might also be simpler if they aren't used elsewhere.

Not really about understanding just about a define that adds nothing if all we do is assign it to a variable of the same name.

Do you have a strong opinion about this ?

I would really prefer to leave it like this, for the following reasons:

1. If I directly use values here, I have to do it also in the previous patch where I introduce the chip_info for the kx022a -> this means I will have defines in the h file which are not used at all -> the defines should be deleted -> the patch will get unnecessarily bigger. I received multiple comments about removing unnecessary changes and reducing of the patch size when possible.

2. Consistency: having all the defines in one place really seems to be better for understanding IMO. I find the mix of values and defines in the chip-info a bit confusing, e.g., I will use the direct value for KX132_REG_CNTL but not for KX132_REG_CNTL2 because KX132_REG_CNTL2 is referenced in a regmap_range.

--
Kind Regards
Mehdi Djait