[PATCH v3 0/7] iio: accel: Add support for Kionix/ROHM KX132-1211 accelerometer

[PATCH v3 0/7] iio: accel: Add support for Kionix/ROHM KX132-1211 accelerometer

[Mehdi Djait]

Hello everyone,

Version 3 for adding support for the kx132-1211 accelerometer

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

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

Changes in v3:
- added two new patches by separating the addition of the 
  i2c_device_id table and the removal of blank lines from other
  unrelated changes
- fixes a warning detected by the kernel test robot
- made all the changes related the chip_info in one patch

Changes in v2:
- added a new patch for warning when the device_id match fails in the
  probe function
- added a new patch for the function that retrieves the number of bytes
  in the buffer
- added a change to the Kconfig file in the patch adding the support
  for the kx132-1211
- various fixes and modifications listed under each patch


Mehdi Djait (7):
  dt-bindings: iio: Add KX132-1211 accelerometer
  iio: accel: kionix-kx022a: Remove blank lines
  iio: accel: kionix-kx022a: Warn on failed matches and assume
    compatibility
  iio: accel: kionix-kx022a: Add an i2c_device_id table
  iio: accel: kionix-kx022a: Refactor driver and add chip_info structure
  iio: accel: kionix-kx022a: Add a function to retrieve number of bytes
    in buffer
  iio: accel: Add support for Kionix/ROHM KX132-1211 accelerometer

 .../bindings/iio/accel/kionix,kx022a.yaml     |  12 +-
 drivers/iio/accel/Kconfig                     |   8 +-
 drivers/iio/accel/kionix-kx022a-i2c.c         |  22 +-
 drivers/iio/accel/kionix-kx022a-spi.c         |  17 +-
 drivers/iio/accel/kionix-kx022a.c             | 295 ++++++++++++++----
 drivers/iio/accel/kionix-kx022a.h             | 110 ++++++-
 6 files changed, 391 insertions(+), 73 deletions(-)

-- 
2.30.2

[PATCH v3 1/7] dt-bindings: iio: Add KX132-1211 accelerometer

[Mehdi Djait]

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

Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Acked-by: Matti Vaittinen <mazziesaccount@gmail.com>
Signed-off-by: Mehdi Djait <mehdi.djait.k@gmail.com>
---
v3:
- no changes 

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"

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

diff --git a/Documentation/devicetree/bindings/iio/accel/kionix,kx022a.yaml b/Documentation/devicetree/bindings/iio/accel/kionix,kx022a.yaml
index 986df1a6ff0a..034b69614416 100644
--- a/Documentation/devicetree/bindings/iio/accel/kionix,kx022a.yaml
+++ b/Documentation/devicetree/bindings/iio/accel/kionix,kx022a.yaml
@@ -4,19 +4,21 @@
 $id: http://devicetree.org/schemas/iio/accel/kionix,kx022a.yaml#
 $schema: http://devicetree.org/meta-schemas/core.yaml#
 
-title: ROHM/Kionix KX022A Accelerometer
+title: ROHM/Kionix KX022A and KX132-1211 Accelerometers
 
 maintainers:
   - Matti Vaittinen <mazziesaccount@gmail.com>
 
 description: |
-  KX022A is a 3-axis accelerometer supporting +/- 2G, 4G, 8G and 16G ranges,
-  output data-rates from 0.78Hz to 1600Hz and a hardware-fifo buffering.
-  KX022A can be accessed either via I2C or SPI.
+  KX022A and KX132-1211 are 3-axis accelerometers supporting +/- 2G, 4G, 8G and
+  16G ranges, variable output data-rates and a hardware-fifo buffering.
+  KX022A and KX132-1211 can be accessed either via I2C or SPI.
 
 properties:
   compatible:
-    const: kionix,kx022a
+    enum:
+      - kionix,kx022a
+      - kionix,kx132-1211
 
   reg:
     maxItems: 1
-- 
2.30.2

[PATCH v3 2/7] iio: accel: kionix-kx022a: Remove blank lines

[Mehdi Djait]

Remove blank lines pointed out by the checkpatch script

Signed-off-by: Mehdi Djait <mehdi.djait.k@gmail.com>
---
v3:
- no changes, this patch is introduced in the v2

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

diff --git a/drivers/iio/accel/kionix-kx022a.c b/drivers/iio/accel/kionix-kx022a.c
index f98393d74666..ff8aa7b9568e 100644
--- a/drivers/iio/accel/kionix-kx022a.c
+++ b/drivers/iio/accel/kionix-kx022a.c
@@ -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,
-- 
2.30.2

Re: [PATCH v3 2/7] iio: accel: kionix-kx022a: Remove blank lines

[Matti Vaittinen]

On 4/25/23 01:22, Mehdi Djait wrote:
> Remove blank lines pointed out by the checkpatch script
> 
> Signed-off-by: Mehdi Djait <mehdi.djait.k@gmail.com>

Reviewed-by: Matti Vaittinen <mazziesaccount@gmail.com>

> ---
> v3:
> - no changes, this patch is introduced in the v2
> 
>   drivers/iio/accel/kionix-kx022a.c | 2 --
>   1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/iio/accel/kionix-kx022a.c b/drivers/iio/accel/kionix-kx022a.c
> index f98393d74666..ff8aa7b9568e 100644
> --- a/drivers/iio/accel/kionix-kx022a.c
> +++ b/drivers/iio/accel/kionix-kx022a.c
> @@ -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,

-- 
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~

[PATCH v3 3/7] iio: accel: kionix-kx022a: Warn on failed matches and assume compatibility

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

Acked-by: Matti Vaittinen <mazziesaccount@gmail.com>
Signed-off-by: Mehdi Djait <mehdi.djait.k@gmail.com>
---
v3:
- changed from 'unsupported' to 'unknown'
- removed the opening bracket

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

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

diff --git a/drivers/iio/accel/kionix-kx022a.c b/drivers/iio/accel/kionix-kx022a.c
index ff8aa7b9568e..494e81ba1da9 100644
--- a/drivers/iio/accel/kionix-kx022a.c
+++ b/drivers/iio/accel/kionix-kx022a.c
@@ -1036,10 +1036,8 @@ int kx022a_probe_internal(struct device *dev)
 	if (ret)
 		return dev_err_probe(dev, ret, "Failed to access sensor\n");
 
-	if (chip_id != KX022A_ID) {
-		dev_err(dev, "unsupported device 0x%x\n", chip_id);
-		return -EINVAL;
-	}
+	if (chip_id != KX022A_ID)
+		dev_warn(dev, "unknown device 0x%x\n", chip_id);
 
 	irq = fwnode_irq_get_byname(fwnode, "INT1");
 	if (irq > 0) {
-- 
2.30.2

[PATCH v3 4/7] iio: accel: kionix-kx022a: Add an i2c_device_id table

[Mehdi Djait]

Add the missing i2c device id 

Signed-off-by: Mehdi Djait <mehdi.djait.k@gmail.com>
---
v3:                                                                             
- no changes, this patch is introduced in the v2 

 drivers/iio/accel/kionix-kx022a-i2c.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/iio/accel/kionix-kx022a-i2c.c b/drivers/iio/accel/kionix-kx022a-i2c.c
index e6fd02d931b6..8f23631a1fd3 100644
--- a/drivers/iio/accel/kionix-kx022a-i2c.c
+++ b/drivers/iio/accel/kionix-kx022a-i2c.c
@@ -30,6 +30,12 @@ static int kx022a_i2c_probe(struct i2c_client *i2c)
 	return kx022a_probe_internal(dev);
 }
 
+static const struct i2c_device_id kx022a_i2c_id[] = {
+	{ .name = "kx022a", 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, kx022a_i2c_id);
+
 static const struct of_device_id kx022a_of_match[] = {
 	{ .compatible = "kionix,kx022a", },
 	{ }
@@ -42,6 +48,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);
 
-- 
2.30.2

Re: [PATCH v3 4/7] iio: accel: kionix-kx022a: Add an i2c_device_id table

[Matti Vaittinen]

On 4/25/23 01:22, Mehdi Djait wrote:
> Add the missing i2c device id
> 
> Signed-off-by: Mehdi Djait <mehdi.djait.k@gmail.com>

Reviewed-by: Matti Vaittinen <mazziesaccount@gmail.com>

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

-- 
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~

Re: [PATCH v3 4/7] iio: accel: kionix-kx022a: Add an i2c_device_id table

[Jonathan Cameron]

On Tue, 25 Apr 2023 08:31:20 +0300
Matti Vaittinen <mazziesaccount@gmail.com> wrote:

> On 4/25/23 01:22, Mehdi Djait wrote:
> > Add the missing i2c device id
> > 
> > Signed-off-by: Mehdi Djait <mehdi.djait.k@gmail.com>  
> 
> Reviewed-by: Matti Vaittinen <mazziesaccount@gmail.com>
> 
> 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).

I think it's fine to treat this as a 'feature' be it one that
we'd happily backport to stable if someone asked because the driver
didn't work as expected on their systems.

Jonathan

> 
> Yours,
> 	-- Matti
> 

Re: [PATCH v3 4/7] iio: accel: kionix-kx022a: Add an i2c_device_id table

[Andy Shevchenko]

On Tue, Apr 25, 2023 at 12:22:24AM +0200, Mehdi Djait wrote:
> Add the missing i2c device id 

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

...

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

', 0' is redundant.

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

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 4/7] iio: accel: kionix-kx022a: Add an i2c_device_id table

[Mehdi Djait]

Hi Andi,

On Tue, Apr 25, 2023 at 04:40:08PM +0300, Andy Shevchenko wrote:
> On Tue, Apr 25, 2023 at 12:22:24AM +0200, Mehdi Djait wrote:
> > Add the missing i2c device id 
> 
> Seems like unfinished commit message (also note missing period, that's why
> I paid attention to this).
> 

While the period is indeed missing. I did not find anything else to
write in the commit message other than 'Add the missing i2c device id'

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

I will remove it

--
Kind Regards
Mehdi Djait

[PATCH v3 5/7] iio: accel: kionix-kx022a: Refactor driver and add chip_info structure

[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 <mehdi.djait.k@gmail.com>
---
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(-)

diff --git a/drivers/iio/accel/kionix-kx022a-i2c.c b/drivers/iio/accel/kionix-kx022a-i2c.c
index 8f23631a1fd3..ce299d0446f7 100644
--- a/drivers/iio/accel/kionix-kx022a-i2c.c
+++ b/drivers/iio/accel/kionix-kx022a-i2c.c
@@ -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,22 +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", 0 },
+	{ .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);
diff --git a/drivers/iio/accel/kionix-kx022a-spi.c b/drivers/iio/accel/kionix-kx022a-spi.c
index 9cd047f7b346..b84503e24510 100644
--- a/drivers/iio/accel/kionix-kx022a-spi.c
+++ b/drivers/iio/accel/kionix-kx022a-spi.c
@@ -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))
 		return dev_err_probe(dev, PTR_ERR(regmap),
 				     "Failed to initialize Regmap\n");
 
-	return kx022a_probe_internal(dev);
+	return kx022a_probe_internal(dev, chip_info);
 }
 
 static const struct spi_device_id kx022a_id[] = {
-	{ "kx022a" },
+	{ .name = "kx022a", .driver_data = (kernel_ulong_t)&kx022a_chip_info },
 	{ }
 };
 MODULE_DEVICE_TABLE(spi, kx022a_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);
diff --git a/drivers/iio/accel/kionix-kx022a.c b/drivers/iio/accel/kionix-kx022a.c
index 494e81ba1da9..66da3c8c3fd0 100644
--- a/drivers/iio/accel/kionix-kx022a.c
+++ b/drivers/iio/accel/kionix-kx022a.c
@@ -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,9 +149,9 @@ 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 {
+	const struct kx022a_chip_info *chip_info;
 	struct regmap *regmap;
 	struct iio_trigger *trig;
 	struct device *dev;
@@ -208,7 +208,7 @@ static const struct iio_chan_spec_ext_info kx022a_ext_info[] = {
 	{ }
 };
 
-#define KX022A_ACCEL_CHAN(axis, index)				\
+#define KX022A_ACCEL_CHAN(axis, reg, index)			\
 {								\
 	.type = IIO_ACCEL,					\
 	.modified = 1,						\
@@ -220,7 +220,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 +231,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),
 };
 
@@ -332,10 +332,10 @@ static int kx022a_turn_on_off_unlocked(struct kx022a_data *data, bool on)
 	int ret;
 
 	if (on)
-		ret = regmap_set_bits(data->regmap, KX022A_REG_CNTL,
+		ret = regmap_set_bits(data->regmap, data->chip_info->cntl,
 				      KX022A_MASK_PC1);
 	else
-		ret = regmap_clear_bits(data->regmap, KX022A_REG_CNTL,
+		ret = regmap_clear_bits(data->regmap, data->chip_info->cntl,
 					KX022A_MASK_PC1);
 	if (ret)
 		dev_err(data->dev, "Turn %s fail %d\n", str_on_off(on), ret);
@@ -402,7 +402,7 @@ static int kx022a_write_raw(struct iio_dev *idev,
 			break;
 
 		ret = regmap_update_bits(data->regmap,
-					 KX022A_REG_ODCNTL,
+					 data->chip_info->odcntl,
 					 KX022A_MASK_ODR, n);
 		data->odr_ns = kx022a_odrs[n];
 		kx022a_turn_on_unlock(data);
@@ -423,7 +423,7 @@ static int kx022a_write_raw(struct iio_dev *idev,
 		if (ret)
 			break;
 
-		ret = regmap_update_bits(data->regmap, KX022A_REG_CNTL,
+		ret = regmap_update_bits(data->regmap, data->chip_info->cntl,
 					 KX022A_MASK_GSEL,
 					 n << KX022A_GSEL_SHIFT);
 		kx022a_turn_on_unlock(data);
@@ -445,7 +445,7 @@ static int kx022a_fifo_set_wmi(struct kx022a_data *data)
 
 	threshold = data->watermark;
 
-	return regmap_update_bits(data->regmap, KX022A_REG_BUF_CNTL1,
+	return regmap_update_bits(data->regmap, data->chip_info->buf_cntl1,
 				  KX022A_MASK_WM_TH, threshold);
 }
 
@@ -488,7 +488,7 @@ static int kx022a_read_raw(struct iio_dev *idev,
 		return ret;
 
 	case IIO_CHAN_INFO_SAMP_FREQ:
-		ret = regmap_read(data->regmap, KX022A_REG_ODCNTL, &regval);
+		ret = regmap_read(data->regmap, data->chip_info->odcntl, &regval);
 		if (ret)
 			return ret;
 
@@ -503,7 +503,7 @@ static int kx022a_read_raw(struct iio_dev *idev,
 		return IIO_VAL_INT_PLUS_MICRO;
 
 	case IIO_CHAN_INFO_SCALE:
-		ret = regmap_read(data->regmap, KX022A_REG_CNTL, &regval);
+		ret = regmap_read(data->regmap, data->chip_info->cntl, &regval);
 		if (ret < 0)
 			return ret;
 
@@ -530,8 +530,8 @@ static int kx022a_set_watermark(struct iio_dev *idev, unsigned int val)
 {
 	struct kx022a_data *data = iio_priv(idev);
 
-	if (val > KX022A_FIFO_LENGTH)
-		val = KX022A_FIFO_LENGTH;
+	if (val > data->chip_info->fifo_length)
+		val = data->chip_info->fifo_length;
 
 	mutex_lock(&data->mutex);
 	data->watermark = val;
@@ -592,7 +592,7 @@ static int kx022a_drop_fifo_contents(struct kx022a_data *data)
 	 */
 	data->timestamp = 0;
 
-	return regmap_write(data->regmap, KX022A_REG_BUF_CLEAR, 0x0);
+	return regmap_write(data->regmap, data->chip_info->buf_clear, 0x0);
 }
 
 static int __kx022a_fifo_flush(struct iio_dev *idev, unsigned int samples,
@@ -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)
+		return -ENOMEM;
+
 	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;
 }
 
@@ -732,10 +739,10 @@ static const struct iio_info kx022a_info = {
 static int kx022a_set_drdy_irq(struct kx022a_data *data, bool en)
 {
 	if (en)
-		return regmap_set_bits(data->regmap, KX022A_REG_CNTL,
+		return regmap_set_bits(data->regmap, data->chip_info->cntl,
 				       KX022A_MASK_DRDY);
 
-	return regmap_clear_bits(data->regmap, KX022A_REG_CNTL,
+	return regmap_clear_bits(data->regmap, data->chip_info->cntl,
 				 KX022A_MASK_DRDY);
 }
 
@@ -770,7 +777,7 @@ static int kx022a_fifo_disable(struct kx022a_data *data)
 	if (ret)
 		goto unlock_out;
 
-	ret = regmap_clear_bits(data->regmap, KX022A_REG_BUF_CNTL2,
+	ret = regmap_clear_bits(data->regmap, data->chip_info->buf_cntl2,
 				KX022A_MASK_BUF_EN);
 	if (ret)
 		goto unlock_out;
@@ -811,7 +818,7 @@ static int kx022a_fifo_enable(struct kx022a_data *data)
 		goto unlock_out;
 
 	/* Enable buffer */
-	ret = regmap_set_bits(data->regmap, KX022A_REG_BUF_CNTL2,
+	ret = regmap_set_bits(data->regmap, data->chip_info->buf_cntl2,
 			      KX022A_MASK_BUF_EN);
 	if (ret)
 		goto unlock_out;
@@ -857,7 +864,7 @@ static irqreturn_t kx022a_trigger_handler(int irq, void *p)
 	struct kx022a_data *data = iio_priv(idev);
 	int ret;
 
-	ret = regmap_bulk_read(data->regmap, KX022A_REG_XOUT_L, data->buffer,
+	ret = regmap_bulk_read(data->regmap, data->chip_info->xout_l, data->buffer,
 			       KX022A_FIFO_SAMPLES_SIZE_BYTES);
 	if (ret < 0)
 		goto err_read;
@@ -905,7 +912,7 @@ static irqreturn_t kx022a_irq_thread_handler(int irq, void *private)
 	if (data->state & KX022A_STATE_FIFO) {
 		int ok;
 
-		ok = __kx022a_fifo_flush(idev, KX022A_FIFO_LENGTH, true);
+		ok = __kx022a_fifo_flush(idev, data->chip_info->fifo_length, true);
 		if (ok > 0)
 			ret = IRQ_HANDLED;
 	}
@@ -958,7 +965,7 @@ static int kx022a_chip_init(struct kx022a_data *data)
 	int ret, val;
 
 	/* Reset the senor */
-	ret = regmap_write(data->regmap, KX022A_REG_CNTL2, KX022A_MASK_SRST);
+	ret = regmap_write(data->regmap, data->chip_info->cntl2, KX022A_MASK_SRST);
 	if (ret)
 		return ret;
 
@@ -968,7 +975,7 @@ static int kx022a_chip_init(struct kx022a_data *data)
 	 */
 	msleep(1);
 
-	ret = regmap_read_poll_timeout(data->regmap, KX022A_REG_CNTL2, val,
+	ret = regmap_read_poll_timeout(data->regmap, data->chip_info->cntl2, val,
 				       !(val & KX022A_MASK_SRST),
 				       KX022A_SOFT_RESET_WAIT_TIME_US,
 				       KX022A_SOFT_RESET_TOTAL_WAIT_TIME_US);
@@ -978,14 +985,14 @@ static int kx022a_chip_init(struct kx022a_data *data)
 		return ret;
 	}
 
-	ret = regmap_reinit_cache(data->regmap, &kx022a_regmap);
+	ret = regmap_reinit_cache(data->regmap, data->chip_info->regmap_config);
 	if (ret) {
 		dev_err(data->dev, "Failed to reinit reg cache\n");
 		return ret;
 	}
 
 	/* set data res 16bit */
-	ret = regmap_set_bits(data->regmap, KX022A_REG_BUF_CNTL2,
+	ret = regmap_set_bits(data->regmap, data->chip_info->buf_cntl2,
 			      KX022A_MASK_BRES16);
 	if (ret) {
 		dev_err(data->dev, "Failed to set data resolution\n");
@@ -995,7 +1002,31 @@ static int kx022a_chip_init(struct kx022a_data *data)
 	return kx022a_prepare_irq_pin(data);
 }
 
-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,
+	.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);
+
+int kx022a_probe_internal(struct device *dev, const struct kx022a_chip_info *chip_info)
 {
 	static const char * const regulator_names[] = {"io-vdd", "vdd"};
 	struct iio_trigger *indio_trig;
@@ -1022,6 +1053,7 @@ int kx022a_probe_internal(struct device *dev)
 		return -ENOMEM;
 
 	data = iio_priv(idev);
+	data->chip_info = chip_info;
 
 	/*
 	 * VDD is the analog and digital domain voltage supply and
@@ -1032,24 +1064,24 @@ int kx022a_probe_internal(struct device *dev)
 	if (ret && ret != -ENODEV)
 		return dev_err_probe(dev, ret, "failed to enable regulator\n");
 
-	ret = regmap_read(regmap, KX022A_REG_WHO, &chip_id);
+	ret = regmap_read(regmap, chip_info->who, &chip_id);
 	if (ret)
 		return dev_err_probe(dev, ret, "Failed to access sensor\n");
 
-	if (chip_id != KX022A_ID)
+	if (chip_id != chip_info->id)
 		dev_warn(dev, "unknown device 0x%x\n", chip_id);
 
 	irq = fwnode_irq_get_byname(fwnode, "INT1");
 	if (irq > 0) {
-		data->inc_reg = KX022A_REG_INC1;
-		data->ien_reg = KX022A_REG_INC4;
+		data->inc_reg = chip_info->inc1;
+		data->ien_reg = chip_info->inc4;
 	} else {
 		irq = fwnode_irq_get_byname(fwnode, "INT2");
 		if (irq <= 0)
 			return dev_err_probe(dev, irq, "No suitable IRQ\n");
 
-		data->inc_reg = KX022A_REG_INC5;
-		data->ien_reg = KX022A_REG_INC6;
+		data->inc_reg = chip_info->inc5;
+		data->ien_reg = chip_info->inc6;
 	}
 
 	data->regmap = regmap;
@@ -1058,9 +1090,9 @@ int kx022a_probe_internal(struct device *dev)
 	data->odr_ns = KX022A_DEFAULT_PERIOD_NS;
 	mutex_init(&data->mutex);
 
-	idev->channels = kx022a_channels;
-	idev->num_channels = ARRAY_SIZE(kx022a_channels);
-	idev->name = "kx022-accel";
+	idev->channels = chip_info->channels;
+	idev->num_channels = chip_info->num_channels;
+	idev->name = chip_info->name;
 	idev->info = &kx022a_info;
 	idev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_SOFTWARE;
 	idev->available_scan_masks = kx022a_scan_masks;
diff --git a/drivers/iio/accel/kionix-kx022a.h b/drivers/iio/accel/kionix-kx022a.h
index 12424649d438..3c31e7d88f78 100644
--- a/drivers/iio/accel/kionix-kx022a.h
+++ b/drivers/iio/accel/kionix-kx022a.h
@@ -76,7 +76,57 @@
 
 struct device;
 
-int kx022a_probe_internal(struct device *dev);
-extern const struct regmap_config kx022a_regmap;
+/**
+ * struct kx022a_chip_info - Kionix accelerometer chip specific information
+ *
+ * @name:		name of the device
+ * @regmap_config:	pointer to register map configuration
+ * @channels:		pointer to iio_chan_spec array
+ * @num_channels:	number of iio_chan_spec channels
+ * @fifo_length:	number of 16-bit samples in a full buffer
+ * @who:		WHO_AM_I register
+ * @id:			WHO_AM_I register value
+ * @cntl:		control register 1
+ * @cntl2:		control register 2
+ * @odcntl:		output data control register
+ * @buf_cntl1:		buffer control register 1
+ * @buf_cntl2:		buffer control register 2
+ * @buf_clear:		buffer clear register
+ * @buf_status1:	buffer status register 1
+ * @buf_smp_lvl_mask:	buffer sample level mask
+ * @buf_read:		buffer read register
+ * @inc1:		interrupt control register 1
+ * @inc4:		interrupt control register 4
+ * @inc5:		interrupt control register 5
+ * @inc6:		interrupt control register 6
+ * @xout_l:		x-axis output least significant byte
+ */
+struct kx022a_chip_info {
+	const char *name;
+	const struct regmap_config *regmap_config;
+	const struct iio_chan_spec *channels;
+	unsigned int num_channels;
+	unsigned int fifo_length;
+	u8 who;
+	u8 id;
+	u8 cntl;
+	u8 cntl2;
+	u8 odcntl;
+	u8 buf_cntl1;
+	u8 buf_cntl2;
+	u8 buf_clear;
+	u8 buf_status1;
+	u16 buf_smp_lvl_mask;
+	u8 buf_read;
+	u8 inc1;
+	u8 inc4;
+	u8 inc5;
+	u8 inc6;
+	u8 xout_l;
+};
+
+int kx022a_probe_internal(struct device *dev, const struct kx022a_chip_info *chip_info);
+
+extern const struct kx022a_chip_info kx022a_chip_info;
 
 #endif
-- 
2.30.2

Re: [PATCH v3 5/7] iio: accel: kionix-kx022a: Refactor driver and add chip_info structure

[Matti Vaittinen]

On 4/25/23 01:22, Mehdi Djait wrote:
> 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 <mehdi.djait.k@gmail.com>
> ---
> 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(-)
> 
> diff --git a/drivers/iio/accel/kionix-kx022a-i2c.c b/drivers/iio/accel/kionix-kx022a-i2c.c
> index 8f23631a1fd3..ce299d0446f7 100644
> --- a/drivers/iio/accel/kionix-kx022a-i2c.c
> +++ b/drivers/iio/accel/kionix-kx022a-i2c.c
> @@ -15,6 +15,7 @@

...


>   static int __kx022a_fifo_flush(struct iio_dev *idev, unsigned int samples,
> @@ -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)
> +		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.

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

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

Yours,
	-- Matti

-- 
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~

Re: [PATCH v3 5/7] iio: accel: kionix-kx022a: Refactor driver and add chip_info structure

[Mehdi Djait]

Hi Matti,

On Tue, Apr 25, 2023 at 09:50:11AM +0300, Matti Vaittinen wrote:
> On 4/25/23 01:22, Mehdi Djait wrote:
> > 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 <mehdi.djait.k@gmail.com>
> > ---
> > 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(-)
> > 
> > diff --git a/drivers/iio/accel/kionix-kx022a-i2c.c b/drivers/iio/accel/kionix-kx022a-i2c.c
> > index 8f23631a1fd3..ce299d0446f7 100644
> > --- a/drivers/iio/accel/kionix-kx022a-i2c.c
> > +++ b/drivers/iio/accel/kionix-kx022a-i2c.c
> > @@ -15,6 +15,7 @@
> 
> ...
> 
> 
> >   static int __kx022a_fifo_flush(struct iio_dev *idev, unsigned int samples,
> > @@ -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)
> > +		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.

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

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

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

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

Thank you :)

--
Kind Regards
Mehdi Djait

Re: [PATCH v3 5/7] iio: accel: kionix-kx022a: Refactor driver and add chip_info structure

[Matti Vaittinen]

On 4/25/23 10:24, Mehdi Djait wrote:
> Hi Matti,
> 
> On Tue, Apr 25, 2023 at 09:50:11AM +0300, Matti Vaittinen wrote:
>> On 4/25/23 01:22, Mehdi Djait wrote:
>>> 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 <mehdi.djait.k@gmail.com>
>>> ---
>>> 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(-)
>>>
>>> diff --git a/drivers/iio/accel/kionix-kx022a-i2c.c b/drivers/iio/accel/kionix-kx022a-i2c.c
>>> index 8f23631a1fd3..ce299d0446f7 100644
>>> --- a/drivers/iio/accel/kionix-kx022a-i2c.c
>>> +++ b/drivers/iio/accel/kionix-kx022a-i2c.c
>>> @@ -15,6 +15,7 @@
>>
>> ...
>>
>>
>>>    static int __kx022a_fifo_flush(struct iio_dev *idev, unsigned int samples,
>>> @@ -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)
>>> +		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.
> 

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

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

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

> 
>>
>>> +
>>>    	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;
>>>    }
>>>
>> ...
>>
>>> -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,
>>> +	.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.
> 
> I agree, this seems to be the better solution. I will look into this.
> 

Thanks for going the extra mile :)

Yours,
	-- Matti

-- 
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~

Re: [PATCH v3 5/7] iio: accel: kionix-kx022a: Refactor driver and add chip_info structure

[Mehdi Djait]

Hi Matti,

On Tue, Apr 25, 2023 at 11:12:11AM +0300, Matti Vaittinen wrote:
> On 4/25/23 10:24, Mehdi Djait wrote:
> > Hi Matti,
> > 
> > On Tue, Apr 25, 2023 at 09:50:11AM +0300, Matti Vaittinen wrote:
> > > On 4/25/23 01:22, Mehdi Djait wrote:
> > > > 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 <mehdi.djait.k@gmail.com>
> > > > ---
> > > > 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(-)
> > > > 
> > > > diff --git a/drivers/iio/accel/kionix-kx022a-i2c.c b/drivers/iio/accel/kionix-kx022a-i2c.c
> > > > index 8f23631a1fd3..ce299d0446f7 100644
> > > > --- a/drivers/iio/accel/kionix-kx022a-i2c.c
> > > > +++ b/drivers/iio/accel/kionix-kx022a-i2c.c
> > > > @@ -15,6 +15,7 @@
> > > 
> > > ...
> > > 
> > > 
> > > >    static int __kx022a_fifo_flush(struct iio_dev *idev, unsigned int samples,
> > > > @@ -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)
> > > > +		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.
> > 
> 
> 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 ?

> > > 
> > > 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.
> 
> then please maybe split the line from appropriate point like:
> buffer = kmalloc(data->chip_info->fifo_length *
> 		 KX022A_FIFO_SAMPLES_SIZE_BYTES, GFP_KERNEL);

I will split it

> 
> > 
> > > 
> > > > +
> > > >    	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;
> > > >    }
> > > > 
> > > ...
> > > 
> > > > -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,
> > > > +	.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.
> > 
> > I agree, this seems to be the better solution. I will look into this.
> > 
> 
> Thanks for going the extra mile :)

Thank you for the review

--
Kind Regards
Mehdi Djait

Re: [PATCH v3 5/7] iio: accel: kionix-kx022a: Refactor driver and add chip_info structure

[Matti Vaittinen]

On 4/29/23 15:59, Mehdi Djait wrote:
> Hi Matti,
> 
> On Tue, Apr 25, 2023 at 11:12:11AM +0300, Matti Vaittinen wrote:
>> On 4/25/23 10:24, Mehdi Djait wrote:
>>> Hi Matti,
>>>
>>> On Tue, Apr 25, 2023 at 09:50:11AM +0300, Matti Vaittinen wrote:
>>>> On 4/25/23 01:22, Mehdi Djait wrote:
>>>>> 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 <mehdi.djait.k@gmail.com>
>>>>> ---
>>>>> 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(-)
>>>>>
>>>>> diff --git a/drivers/iio/accel/kionix-kx022a-i2c.c b/drivers/iio/accel/kionix-kx022a-i2c.c
>>>>> index 8f23631a1fd3..ce299d0446f7 100644
>>>>> --- a/drivers/iio/accel/kionix-kx022a-i2c.c
>>>>> +++ b/drivers/iio/accel/kionix-kx022a-i2c.c
>>>>> @@ -15,6 +15,7 @@
>>>>
>>>> ...
>>>>
>>>>
>>>>>     static int __kx022a_fifo_flush(struct iio_dev *idev, unsigned int samples,
>>>>> @@ -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)
>>>>> +		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.
>>>
>>
>> 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.

> 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


-- 
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~

Re: [PATCH v3 5/7] iio: accel: kionix-kx022a: Refactor driver and add chip_info structure

[Jonathan Cameron]

On Sat, 29 Apr 2023 16:56:38 +0300
Matti Vaittinen <mazziesaccount@gmail.com> wrote:

> On 4/29/23 15:59, Mehdi Djait wrote:
> > Hi Matti,
> > 
> > On Tue, Apr 25, 2023 at 11:12:11AM +0300, Matti Vaittinen wrote:  
> >> On 4/25/23 10:24, Mehdi Djait wrote:  
> >>> Hi Matti,
> >>>
> >>> On Tue, Apr 25, 2023 at 09:50:11AM +0300, Matti Vaittinen wrote:  
> >>>> On 4/25/23 01:22, Mehdi Djait wrote:  
> >>>>> 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 <mehdi.djait.k@gmail.com>
> >>>>> ---
> >>>>> 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(-)
> >>>>>
> >>>>> diff --git a/drivers/iio/accel/kionix-kx022a-i2c.c b/drivers/iio/accel/kionix-kx022a-i2c.c
> >>>>> index 8f23631a1fd3..ce299d0446f7 100644
> >>>>> --- a/drivers/iio/accel/kionix-kx022a-i2c.c
> >>>>> +++ b/drivers/iio/accel/kionix-kx022a-i2c.c
> >>>>> @@ -15,6 +15,7 @@  
> >>>>
> >>>> ...
> >>>>
> >>>>  
> >>>>>     static int __kx022a_fifo_flush(struct iio_dev *idev, unsigned int samples,
> >>>>> @@ -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)
> >>>>> +		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.
> >>>  
> >>
> >> 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.

Separation between what should be done in preenable and postenable has been
vague for a long time. These days only really matters if you need to
order them wrt updating the scan mode I think.

> 
> > 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!

Agreed that these allocations should be taken out of this hot path.
Either of these options should work so up to you.

> 
> Yours,
> 	-- Matti
> 
> 

Re: [PATCH v3 5/7] iio: accel: kionix-kx022a: Refactor driver and add chip_info structure

[Mehdi Djait]

Hello Matti,

On Tue, Apr 25, 2023 at 11:12:11AM +0300, Matti Vaittinen wrote:
> > > > +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,
> > > > +	.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.
> > 
> > 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 ? 

--
Kind Regards,
Mehdi Djait

Re: [PATCH v3 5/7] iio: accel: kionix-kx022a: Refactor driver and add chip_info structure

[Matti Vaittinen]

On 5/7/23 23:45, Mehdi Djait wrote:
> Hello Matti,
> 
> On Tue, Apr 25, 2023 at 11:12:11AM +0300, Matti Vaittinen wrote:
>>>>> +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,
>>>>> +	.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.
>>>
>>> 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

I think that for the clarity and re-usability we would have fields for 
functions. We could for example have separate fields for buffer enable 
and resolution even though it means assigning the same register to both. 
This, however, is somewhat wasteful (memory vise).

Problem with buf_cntl1 and buf_cntl2 is that the 'cntl' is too broad to 
really tell what the control is. Also, what is the difference of 
buf_cntl1 and buf_cntl2?

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

I agree. These look pretty clear to me, although 'status' is also 
somewhat ambiguous. (Is it sample level? Is it some corruption 
detection? Can the buffer be 'busy'?).

> Anyway this is my opinion, what do you think ?

I can currently live with both of these approaches. We may need to 
review this if/when adding support to other sensor(s) though. I will 
leave the decision to you - just make the code best you can ;)

Yours,
	-- Matti

-- 
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~

Re: [PATCH v3 5/7] iio: accel: kionix-kx022a: Refactor driver and add chip_info structure

[Andi Shyti]

Hi Mehdi,

On Tue, Apr 25, 2023 at 12:22:25AM +0200, Mehdi Djait wrote:
> 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.

[...]

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

Andi

Re: [PATCH v3 5/7] iio: accel: kionix-kx022a: Refactor driver and add chip_info structure

[Mehdi Djait]

Hi Andi,

Thank you for the review.

On Tue, Apr 25, 2023 at 05:57:34PM +0200, Andi Shyti wrote:
> Hi Mehdi,
> 
> On Tue, Apr 25, 2023 at 12:22:25AM +0200, Mehdi Djait wrote:
> > 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.

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:
-#define KX022A_ACCEL_CHAN(axis, index)				\
+#define KX022A_ACCEL_CHAN(axis, reg, index)			\

> 
> [...]
> 
> > @@ -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.

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

Re: [PATCH v3 5/7] iio: accel: kionix-kx022a: Refactor driver and add chip_info structure

[Jonathan Cameron]

On Sat, 29 Apr 2023 15:07:46 +0200
Mehdi Djait <mehdi.djait.k@gmail.com> wrote:

> Hi Andi,
> 
> Thank you for the review.
> 
> On Tue, Apr 25, 2023 at 05:57:34PM +0200, Andi Shyti wrote:
> > Hi Mehdi,
> > 
> > On Tue, Apr 25, 2023 at 12:22:25AM +0200, Mehdi Djait wrote:  
> > > 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.  
> 
> 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:
> -#define KX022A_ACCEL_CHAN(axis, index)				\
> +#define KX022A_ACCEL_CHAN(axis, reg, index)			\
> 
> > 
> > [...]
> >   
> > > @@ -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.  
> 
> 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.

Jonathan

> 
> --
> Kind Regards
> Mehdi Djait
> 

Re: [PATCH v3 5/7] iio: accel: kionix-kx022a: Refactor driver and add chip_info structure

[Andy Shevchenko]

On Sun, Apr 30, 2023 at 06:49:10PM +0100, Jonathan Cameron wrote:
> On Sat, 29 Apr 2023 15:07:46 +0200
> Mehdi Djait <mehdi.djait.k@gmail.com> wrote:
> > On Tue, Apr 25, 2023 at 05:57:34PM +0200, Andi Shyti wrote:
> > > On Tue, Apr 25, 2023 at 12:22:25AM +0200, Mehdi Djait wrote:  

...

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

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

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 5/7] iio: accel: kionix-kx022a: Refactor driver and add chip_info structure

[Mehdi Djait]

Hello Andy,

On Tue, May 02, 2023 at 10:41:31PM +0300, Andy Shevchenko wrote:
> On Sun, Apr 30, 2023 at 06:49:10PM +0100, Jonathan Cameron wrote:
> > On Sat, 29 Apr 2023 15:07:46 +0200
> > Mehdi Djait <mehdi.djait.k@gmail.com> wrote:
> > > On Tue, Apr 25, 2023 at 05:57:34PM +0200, Andi Shyti wrote:
> > > > On Tue, Apr 25, 2023 at 12:22:25AM +0200, Mehdi Djait wrote:  
> 
> ...
> 
> > > > > +	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.  
> > > 
> > > 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.
> 
> Instead we have a specific SPI provided helper for the above, please use it
> instead of open coded stuff.

I will change it in the next version

--
Kind Regards
Mehdi Djait

[PATCH v3 6/7] iio: accel: kionix-kx022a: Add a function to retrieve number of bytes in buffer

[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 <mehdi.djait.k@gmail.com>
---
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(-)

diff --git a/drivers/iio/accel/kionix-kx022a.c b/drivers/iio/accel/kionix-kx022a.c
index 66da3c8c3fd0..4a31d17c1f22 100644
--- a/drivers/iio/accel/kionix-kx022a.c
+++ b/drivers/iio/accel/kionix-kx022a.c
@@ -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)
+{
+	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 */
+	if (fifo_bytes == KX022A_FIFO_FULL_VALUE)
+		fifo_bytes = KX022A_FIFO_MAX_BYTES;
+
+	return fifo_bytes;
+}
+
 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;
 	uint64_t sample_period;
 	int count, fifo_bytes;
@@ -611,15 +628,7 @@ static int __kx022a_fifo_flush(struct iio_dev *idev, unsigned int samples,
 	if (!buffer)
 		return -ENOMEM;
 
-	ret = regmap_read(data->regmap, KX022A_REG_BUF_STATUS_1, &fifo_bytes);
-	if (ret) {
-		dev_err(dev, "Error reading buffer status\n");
-		return ret;
-	}
-
-	/* Let's not overflow if we for some reason get bogus value from i2c */
-	if (fifo_bytes == KX022A_FIFO_FULL_VALUE)
-		fifo_bytes = KX022A_FIFO_MAX_BYTES;
+	fifo_bytes = data->chip_info->get_fifo_bytes(data);
 
 	if (fifo_bytes % KX022A_FIFO_SAMPLES_SIZE_BYTES)
 		dev_warn(data->dev, "Bad FIFO alignment. Data may be corrupt\n");
@@ -1023,6 +1032,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);
 
diff --git a/drivers/iio/accel/kionix-kx022a.h b/drivers/iio/accel/kionix-kx022a.h
index 3c31e7d88f78..f043767067b7 100644
--- a/drivers/iio/accel/kionix-kx022a.h
+++ b/drivers/iio/accel/kionix-kx022a.h
@@ -76,6 +76,8 @@
 
 struct device;
 
+struct kx022a_data;
+
 /**
  * struct kx022a_chip_info - Kionix accelerometer chip specific information
  *
@@ -100,6 +102,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 +126,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);
-- 
2.30.2

Re: [PATCH v3 6/7] iio: accel: kionix-kx022a: Add a function to retrieve number of bytes in buffer

[Matti Vaittinen]

On 4/25/23 01:22, Mehdi Djait wrote:
> 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 <mehdi.djait.k@gmail.com>
> ---
> 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).

> 	(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(-)
> 
> diff --git a/drivers/iio/accel/kionix-kx022a.c b/drivers/iio/accel/kionix-kx022a.c
> index 66da3c8c3fd0..4a31d17c1f22 100644
> --- a/drivers/iio/accel/kionix-kx022a.c
> +++ b/drivers/iio/accel/kionix-kx022a.c
> @@ -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)
> +{
> +	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...

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

> +
> +	return fifo_bytes;
> +}
> +

Reviewed-by: Matti Vaittinen <mazziesaccount@gmail.com>

-- 
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~

Re: [PATCH v3 6/7] iio: accel: kionix-kx022a: Add a function to retrieve number of bytes in buffer

[Mehdi Djait]

Hi Matti,

On Tue, Apr 25, 2023 at 10:07:54AM +0300, Matti Vaittinen wrote:
> On 4/25/23 01:22, Mehdi Djait wrote:
> > 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 <mehdi.djait.k@gmail.com>
> > ---
> > 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).
> 
> > 	(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(-)
> > 
> > diff --git a/drivers/iio/accel/kionix-kx022a.c b/drivers/iio/accel/kionix-kx022a.c
> > index 66da3c8c3fd0..4a31d17c1f22 100644
> > --- a/drivers/iio/accel/kionix-kx022a.c
> > +++ b/drivers/iio/accel/kionix-kx022a.c
> > @@ -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)
> > +{
> > +	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...
> 
> > +	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 ;)

I will take care of the comment if there is a v4

--
Kind Regards
Mehdi Djait

[PATCH v3 7/7] iio: accel: Add support for Kionix/ROHM KX132-1211 accelerometer

[Mehdi Djait]

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 <mehdi.djait.k@gmail.com>
---
 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(-)

diff --git a/drivers/iio/accel/Kconfig b/drivers/iio/accel/Kconfig
index b6b45d359f28..d8cc6e6f2bb9 100644
--- a/drivers/iio/accel/Kconfig
+++ b/drivers/iio/accel/Kconfig
@@ -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"
diff --git a/drivers/iio/accel/kionix-kx022a-i2c.c b/drivers/iio/accel/kionix-kx022a-i2c.c
index ce299d0446f7..4ea28d2482ec 100644
--- a/drivers/iio/accel/kionix-kx022a-i2c.c
+++ b/drivers/iio/accel/kionix-kx022a-i2c.c
@@ -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);
diff --git a/drivers/iio/accel/kionix-kx022a-spi.c b/drivers/iio/accel/kionix-kx022a-spi.c
index b84503e24510..b755b2b395ed 100644
--- a/drivers/iio/accel/kionix-kx022a-spi.c
+++ b/drivers/iio/accel/kionix-kx022a-spi.c
@@ -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);
diff --git a/drivers/iio/accel/kionix-kx022a.c b/drivers/iio/accel/kionix-kx022a.c
index 4a31d17c1f22..a6808ab12162 100644
--- a/drivers/iio/accel/kionix-kx022a.c
+++ b/drivers/iio/accel/kionix-kx022a.c
@@ -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,
+		.range_max = KX132_REG_INT_REL,
+	}, {
+		/* The reset bit will be cleared by sensor */
+		.range_min = KX132_REG_CNTL2,
+		.range_max = KX132_REG_CNTL2,
+	}, {
+		.range_min = KX132_REG_BUF_STATUS_1,
+		.range_max = KX132_REG_BUF_READ,
+	},
+};
+
+static const struct regmap_access_table kx132_volatile_regs = {
+	.yes_ranges = &kx132_volatile_ranges[0],
+	.n_yes_ranges = ARRAY_SIZE(kx132_volatile_ranges),
+};
+
+static const struct regmap_range kx132_precious_ranges[] = {
+	{
+		.range_min = KX132_REG_INT_REL,
+		.range_max = KX132_REG_INT_REL,
+	},
+};
+
+static const struct regmap_access_table kx132_precious_regs = {
+	.yes_ranges = &kx132_precious_ranges[0],
+	.n_yes_ranges = ARRAY_SIZE(kx132_precious_ranges),
+};
+
+static const struct regmap_range kx132_read_only_ranges[] = {
+	{
+		.range_min = KX132_REG_XADP_L,
+		.range_max = KX132_REG_INT_REL,
+	}, {
+		.range_min = KX132_REG_BUF_STATUS_1,
+		.range_max = KX132_REG_BUF_STATUS_2,
+	}, {
+		.range_min = KX132_REG_BUF_READ,
+		.range_max = KX132_REG_BUF_READ,
+	},
+};
+
+static const struct regmap_access_table kx132_ro_regs = {
+	.no_ranges = &kx132_read_only_ranges[0],
+	.n_no_ranges = ARRAY_SIZE(kx132_read_only_ranges),
+};
+
+static const struct regmap_range kx132_write_only_ranges[] = {
+	{
+		.range_min = KX132_REG_MAN_WAKE,
+		.range_max = KX132_REG_MAN_WAKE,
+	}, {
+		.range_min = KX132_REG_SELF_TEST,
+		.range_max = KX132_REG_SELF_TEST,
+	}, {
+		.range_min = KX132_REG_BUF_CLEAR,
+		.range_max = KX132_REG_BUF_CLEAR,
+	},
+};
+
+static const struct regmap_access_table kx132_wo_regs = {
+	.no_ranges = &kx132_write_only_ranges[0],
+	.n_no_ranges = ARRAY_SIZE(kx132_write_only_ranges),
+};
+
+static const struct regmap_range kx132_noinc_read_ranges[] = {
+	{
+		.range_min = KX132_REG_BUF_READ,
+		.range_max = KX132_REG_BUF_READ,
+	},
+};
+
+static const struct regmap_access_table kx132_nir_regs = {
+	.yes_ranges = &kx132_noinc_read_ranges[0],
+	.n_yes_ranges = ARRAY_SIZE(kx132_noinc_read_ranges),
+};
+
+static const struct regmap_config kx132_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+	.volatile_table = &kx132_volatile_regs,
+	.rd_table = &kx132_wo_regs,
+	.wr_table = &kx132_ro_regs,
+	.rd_noinc_table = &kx132_nir_regs,
+	.precious_table = &kx132_precious_regs,
+	.max_register = KX132_MAX_REGISTER,
+	.cache_type = REGCACHE_RBTREE,
+};
+
 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;
+
+	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;
+
+	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"};
diff --git a/drivers/iio/accel/kionix-kx022a.h b/drivers/iio/accel/kionix-kx022a.h
index f043767067b7..1f4135cf20eb 100644
--- a/drivers/iio/accel/kionix-kx022a.h
+++ b/drivers/iio/accel/kionix-kx022a.h
@@ -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
+#define KX132_REG_YOUT_L	0x0a
+#define KX132_REG_ZOUT_L	0x0c
+#define KX132_REG_COTR		0x12
+#define KX132_REG_TSCP		0x14
+#define KX132_REG_INT_REL	0x1a
+
+#define KX132_REG_ODCNTL	0x21
+
+#define KX132_REG_BTS_WUF_TH	0x4a
+#define KX132_REG_MAN_WAKE	0x4d
+
+#define KX132_REG_BUF_CNTL1	0x5e
+#define KX132_REG_BUF_CNTL2	0x5f
+#define KX132_REG_BUF_STATUS_1	0x60
+#define KX132_REG_BUF_STATUS_2	0x61
+#define KX132_MASK_BUF_SMP_LVL	GENMASK(9, 0)
+#define KX132_REG_BUF_CLEAR	0x62
+#define KX132_REG_BUF_READ	0x63
+#define KX132_ODR_SHIFT		3
+#define KX132_FIFO_MAX_WMI_TH	86
+
+#define KX132_REG_INC1		0x22
+#define KX132_REG_INC5		0x26
+#define KX132_REG_INC6		0x27
+#define KX132_IPOL_LOW		0
+#define KX132_IPOL_HIGH		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;
@@ -132,5 +183,6 @@ struct kx022a_chip_info {
 int kx022a_probe_internal(struct device *dev, const struct kx022a_chip_info *chip_info);
 
 extern const struct kx022a_chip_info kx022a_chip_info;
+extern const struct kx022a_chip_info kx132_chip_info;
 
 #endif
-- 
2.30.2

Re: [PATCH v3 7/7] iio: accel: Add support for Kionix/ROHM KX132-1211 accelerometer

[Matti Vaittinen]

On 4/25/23 01:22, Mehdi Djait 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 <mehdi.djait.k@gmail.com>
> ---
>   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(-)
> 
> diff --git a/drivers/iio/accel/Kconfig b/drivers/iio/accel/Kconfig
> index b6b45d359f28..d8cc6e6f2bb9 100644
> --- a/drivers/iio/accel/Kconfig
> +++ b/drivers/iio/accel/Kconfig
> @@ -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"
> diff --git a/drivers/iio/accel/kionix-kx022a-i2c.c b/drivers/iio/accel/kionix-kx022a-i2c.c
> index ce299d0446f7..4ea28d2482ec 100644
> --- a/drivers/iio/accel/kionix-kx022a-i2c.c
> +++ b/drivers/iio/accel/kionix-kx022a-i2c.c
> @@ -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);
> diff --git a/drivers/iio/accel/kionix-kx022a-spi.c b/drivers/iio/accel/kionix-kx022a-spi.c
> index b84503e24510..b755b2b395ed 100644
> --- a/drivers/iio/accel/kionix-kx022a-spi.c
> +++ b/drivers/iio/accel/kionix-kx022a-spi.c
> @@ -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);
> diff --git a/drivers/iio/accel/kionix-kx022a.c b/drivers/iio/accel/kionix-kx022a.c
> index 4a31d17c1f22..a6808ab12162 100644
> --- a/drivers/iio/accel/kionix-kx022a.c
> +++ b/drivers/iio/accel/kionix-kx022a.c
> @@ -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,
> +		.range_max = KX132_REG_INT_REL,
> +	}, {
> +		/* The reset bit will be cleared by sensor */
> +		.range_min = KX132_REG_CNTL2,
> +		.range_max = KX132_REG_CNTL2,
> +	}, {
> +		.range_min = KX132_REG_BUF_STATUS_1,
> +		.range_max = KX132_REG_BUF_READ,
> +	},
> +};

Maybe the CNTL5 should also be added to volatile table as it has "self 
clearing" bits? I didn't go through all the registers to see if there 
are more.

> +
> +static const struct regmap_access_table kx132_volatile_regs = {
> +	.yes_ranges = &kx132_volatile_ranges[0],
> +	.n_yes_ranges = ARRAY_SIZE(kx132_volatile_ranges),
> +};
> +
> +static const struct regmap_range kx132_precious_ranges[] = {
> +	{
> +		.range_min = KX132_REG_INT_REL,
> +		.range_max = KX132_REG_INT_REL,
> +	},
> +};
> +
> +static const struct regmap_access_table kx132_precious_regs = {
> +	.yes_ranges = &kx132_precious_ranges[0],
> +	.n_yes_ranges = ARRAY_SIZE(kx132_precious_ranges),
> +};
> +
> +static const struct regmap_range kx132_read_only_ranges[] = {
> +	{
> +		.range_min = KX132_REG_XADP_L,
> +		.range_max = KX132_REG_INT_REL,
> +	}, {
> +		.range_min = KX132_REG_BUF_STATUS_1,
> +		.range_max = KX132_REG_BUF_STATUS_2,
> +	}, {
> +		.range_min = KX132_REG_BUF_READ,
> +		.range_max = KX132_REG_BUF_READ,
> +	},
> +};

Do you think adding the "Kionix reserved" registers to read-only ranges 
would make things safer or is there a reason to keep them writable? I 
think the data-sheet states these should not be written to.

> +
> +static const struct regmap_access_table kx132_ro_regs = {
> +	.no_ranges = &kx132_read_only_ranges[0],
> +	.n_no_ranges = ARRAY_SIZE(kx132_read_only_ranges),
> +};
> +
> +static const struct regmap_range kx132_write_only_ranges[] = {
> +	{
> +		.range_min = KX132_REG_MAN_WAKE,
> +		.range_max = KX132_REG_MAN_WAKE,

Why the WUFC is write-only? Also, I don't think the KX022A_REG_MAN_WAKE 
and KX132_REG_MAN_WAKE reflect same functionality. The naming of the 
define may be slightly misleading.

> +	}, {
> +		.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;
> +
> +	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 :)


> +
> +	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"};
> diff --git a/drivers/iio/accel/kionix-kx022a.h b/drivers/iio/accel/kionix-kx022a.h
> index f043767067b7..1f4135cf20eb 100644
> --- a/drivers/iio/accel/kionix-kx022a.h
> +++ b/drivers/iio/accel/kionix-kx022a.h
> @@ -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
> +#define KX132_REG_YOUT_L	0x0a
> +#define KX132_REG_ZOUT_L	0x0c
> +#define KX132_REG_COTR		0x12
> +#define KX132_REG_TSCP		0x14
> +#define KX132_REG_INT_REL	0x1a
> +
> +#define KX132_REG_ODCNTL	0x21
> +
> +#define KX132_REG_BTS_WUF_TH	0x4a
> +#define KX132_REG_MAN_WAKE	0x4d

As mentioned elsewhere, I don't think this is functionally same as 
KX022A_REG_MAN_WAKE.

> +
> +#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;
> @@ -132,5 +183,6 @@ struct kx022a_chip_info {
>   int kx022a_probe_internal(struct device *dev, const struct kx022a_chip_info *chip_info);
>   
>   extern const struct kx022a_chip_info kx022a_chip_info;
> +extern const struct kx022a_chip_info kx132_chip_info;
>   
>   #endif


Thanks for adding this support!

Yours,
	-- Matti


-- 
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~

Re: [PATCH v3 7/7] iio: accel: Add support for Kionix/ROHM KX132-1211 accelerometer

[Jonathan Cameron]

> > +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");
> > +		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 :)

:) 

I go the other way. It's less obvious to me that it is appropriate
to apply le16_to_cpu(buf_status) after applying a mask to some
bits. The moment that is appropriate, then we certainly hope a single
mask application is as well.

I think treating it as a 16 bit register is appropriate, in particular
as the field is described as SMP_LEV[9:0] on the datasheet
(of course there are datasheets that do that for unconnected sets of
bits so this doesn't always work ;)

Jonathan

Re: [PATCH v3 7/7] iio: accel: Add support for Kionix/ROHM KX132-1211 accelerometer

[Jonathan Cameron]

On Tue, 25 Apr 2023 00:22:27 +0200
Mehdi Djait <mehdi.djait.k@gmail.com> 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 <mehdi.djait.k@gmail.com>

Two tiny things inline.  

> +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");
> +		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.


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

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. 

> +	.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"};

Re: [PATCH v3 7/7] iio: accel: Add support for Kionix/ROHM KX132-1211 accelerometer

[Mehdi Djait]

Hello Jonathan,

On Mon, May 01, 2023 at 03:56:45PM +0100, Jonathan Cameron wrote:
> On Tue, 25 Apr 2023 00:22:27 +0200
> Mehdi Djait <mehdi.djait.k@gmail.com> 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 <mehdi.djait.k@gmail.com>
> 
> Two tiny things inline.  
> 
> > +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");
> > +		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.

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

Re: [PATCH v3 7/7] iio: accel: Add support for Kionix/ROHM KX132-1211 accelerometer

[Jonathan Cameron]

On Fri, 5 May 2023 20:11:33 +0200
Mehdi Djait <mehdi.djait.k@gmail.com> wrote:

> Hello Jonathan,
> 
> On Mon, May 01, 2023 at 03:56:45PM +0100, Jonathan Cameron wrote:
> > On Tue, 25 Apr 2023 00:22:27 +0200
> > Mehdi Djait <mehdi.djait.k@gmail.com> 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 <mehdi.djait.k@gmail.com>  
> > 
> > Two tiny things inline.  
> >   
> > > +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");
> > > +		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.

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

> 
> > 
> >   
> > > +
> > > +	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,  
> > 
> > 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) ? 

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.

> 
> --
> Kind Regards
> Mehdi Djait

Re: [PATCH v3 7/7] iio: accel: Add support for Kionix/ROHM KX132-1211 accelerometer

[Mehdi Djait]

Hello Jonathan,

On Sat, May 06, 2023 at 05:46:51PM +0100, Jonathan Cameron wrote:
> On Fri, 5 May 2023 20:11:33 +0200
> Mehdi Djait <mehdi.djait.k@gmail.com> wrote:
> 
> > Hello Jonathan,
> > 
> > On Mon, May 01, 2023 at 03:56:45PM +0100, Jonathan Cameron wrote:
> > > On Tue, 25 Apr 2023 00:22:27 +0200
> > > Mehdi Djait <mehdi.djait.k@gmail.com> 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 <mehdi.djait.k@gmail.com>  
> > > 
> > > Two tiny things inline.  
> > >   
> > > > +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");
> > > > +		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.
> 
> 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 ?

> >
> > > 
> > >   
> > > > +
> > > > +	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,  
> > > 
> > > 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) ? 
> 
> 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

Re: [PATCH v3 7/7] iio: accel: Add support for Kionix/ROHM KX132-1211 accelerometer

[Jonathan Cameron]

On Sun, 7 May 2023 16:56:55 +0200
Mehdi Djait <mehdi.djait.k@gmail.com> wrote:

> Hello Jonathan,
> 
> On Sat, May 06, 2023 at 05:46:51PM +0100, Jonathan Cameron wrote:
> > On Fri, 5 May 2023 20:11:33 +0200
> > Mehdi Djait <mehdi.djait.k@gmail.com> wrote:
> >   
> > > Hello Jonathan,
> > > 
> > > On Mon, May 01, 2023 at 03:56:45PM +0100, Jonathan Cameron wrote:  
> > > > On Tue, 25 Apr 2023 00:22:27 +0200
> > > > Mehdi Djait <mehdi.djait.k@gmail.com> 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 <mehdi.djait.k@gmail.com>    
> > > > 
> > > > Two tiny things inline.  
> > > >     
> > > > > +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");
> > > > > +		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.  
> > 
> > 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 ? 
This fine.  Sorry for the diversion to nowhere!

Jonathan

> 
> 4. another solution ?
> 
> > >  
> > > > 
> > > >     
> > > > > +
> > > > > +	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,    
> > > > 
> > > > 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) ?   
> > 
> > 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