[RFC PATCH 0/5] iio: Support ROHM/Kionix kx022a

[RFC PATCH 0/5] iio: Support ROHM/Kionix kx022a

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 2857 bytes --]

Add initial support for ROHM/Kionix kx022a accelerometer

This series is not ready for being merged as such. The first two patches
in the series (the new regulator devm interfaces) are already merged to
Mark's tree but have not yet found their way to the mainline. This is
the reason I marked series as RFC. You can also skip the reviewing of
first two patches as they are there just to make the series compile on
top of the v6.0-rc4. I will re-spin the series without the patches 0001
and 0002 and drop the RFC tag when the v6.1-rc1 is out.

About the HW:
KX022A 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)
	- support detecting special events like double tap or motion
	- can be configured to wake-up system when events are detected.

About the series:

This series adds support for only getting the accelerometer data and
configuring the G-range / ODR via IIO. Motion detection or double-tap
detection are not supported by the series. The other quite important but
still missing piece is the runtime PM. Nevertheless, the driver should be
usable and brings the basic support for getting accelerometer data.

---

Matti Vaittinen (5):
  regulator: Add devm helpers for get and enable
  regulator: Add devm helpers for get and enable
  dt-bindings: iio: Add KX022A accelerometer
  iio: accel: Support Kionix/ROHM KX022A accelerometer
  MAINTAINERS: Add KX022A maintainer entry

 .../bindings/iio/accel/kionix,kx022a.yaml     |   58 +
 MAINTAINERS                                   |    5 +
 drivers/iio/accel/Kconfig                     |   23 +
 drivers/iio/accel/Makefile                    |    3 +
 drivers/iio/accel/kionix-kx022a-i2c.c         |   52 +
 drivers/iio/accel/kionix-kx022a-spi.c         |   50 +
 drivers/iio/accel/kionix-kx022a.c             | 1149 +++++++++++++++++
 drivers/iio/accel/kionix-kx022a.h             |   76 ++
 drivers/regulator/devres.c                    |  164 +++
 include/linux/regulator/consumer.h            |   27 +
 10 files changed, 1607 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/accel/kionix,kx022a.yaml
 create mode 100644 drivers/iio/accel/kionix-kx022a-i2c.c
 create mode 100644 drivers/iio/accel/kionix-kx022a-spi.c
 create mode 100644 drivers/iio/accel/kionix-kx022a.c
 create mode 100644 drivers/iio/accel/kionix-kx022a.h

-- 
2.37.1


-- 
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND

~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
Simon says - in Latin please.
~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
Thanks to Simon Glass for the translation =] 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[RFC PATCH 1/5] regulator: Add devm helpers for get and enable

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 4475 bytes --]

A few regulator consumer drivers seem to be just getting a regulator,
enabling it and registering a devm-action to disable the regulator at
the driver detach and then forget about it.

We can simplify this a bit by adding a devm-helper for this pattern.
Add devm_regulator_get_enable() and devm_regulator_get_enable_optional()

Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
(cherry picked from commit b6058e052b842a19c8bb639798d8692cd0e7589f)
---

Please note - this patch is already in Mark's tree. It's here just to
allow compilation of the series on top of v6.0-rc4

 drivers/regulator/devres.c         | 59 ++++++++++++++++++++++++++++++
 include/linux/regulator/consumer.h | 13 +++++++
 2 files changed, 72 insertions(+)

diff --git a/drivers/regulator/devres.c b/drivers/regulator/devres.c
index 32823a87fd40..3cb3eb49ad8a 100644
--- a/drivers/regulator/devres.c
+++ b/drivers/regulator/devres.c
@@ -70,6 +70,65 @@ struct regulator *devm_regulator_get_exclusive(struct device *dev,
 }
 EXPORT_SYMBOL_GPL(devm_regulator_get_exclusive);
 
+static void regulator_action_disable(void *d)
+{
+	struct regulator *r = (struct regulator *)d;
+
+	regulator_disable(r);
+}
+
+static int _devm_regulator_get_enable(struct device *dev, const char *id,
+				      int get_type)
+{
+	struct regulator *r;
+	int ret;
+
+	r = _devm_regulator_get(dev, id, get_type);
+	if (IS_ERR(r))
+		return PTR_ERR(r);
+
+	ret = regulator_enable(r);
+	if (!ret)
+		ret = devm_add_action_or_reset(dev, &regulator_action_disable, r);
+
+	if (ret)
+		devm_regulator_put(r);
+
+	return ret;
+}
+
+/**
+ * devm_regulator_get_enable_optional - Resource managed regulator get and enable
+ * @dev: device to supply
+ * @id:  supply name or regulator ID.
+ *
+ * Get and enable regulator for duration of the device life-time.
+ * regulator_disable() and regulator_put() are automatically called on driver
+ * detach. See regulator_get_optional() and regulator_enable() for more
+ * information.
+ */
+int devm_regulator_get_enable_optional(struct device *dev, const char *id)
+{
+	return _devm_regulator_get_enable(dev, id, OPTIONAL_GET);
+}
+EXPORT_SYMBOL_GPL(devm_regulator_get_enable_optional);
+
+/**
+ * devm_regulator_get_enable - Resource managed regulator get and enable
+ * @dev: device to supply
+ * @id:  supply name or regulator ID.
+ *
+ * Get and enable regulator for duration of the device life-time.
+ * regulator_disable() and regulator_put() are automatically called on driver
+ * detach. See regulator_get() and regulator_enable() for more
+ * information.
+ */
+int devm_regulator_get_enable(struct device *dev, const char *id)
+{
+	return _devm_regulator_get_enable(dev, id, NORMAL_GET);
+}
+EXPORT_SYMBOL_GPL(devm_regulator_get_enable);
+
 /**
  * devm_regulator_get_optional - Resource managed regulator_get_optional()
  * @dev: device to supply
diff --git a/include/linux/regulator/consumer.h b/include/linux/regulator/consumer.h
index bc6cda706d1f..8e6cf65851b0 100644
--- a/include/linux/regulator/consumer.h
+++ b/include/linux/regulator/consumer.h
@@ -207,6 +207,8 @@ struct regulator *__must_check regulator_get_optional(struct device *dev,
 						      const char *id);
 struct regulator *__must_check devm_regulator_get_optional(struct device *dev,
 							   const char *id);
+int devm_regulator_get_enable(struct device *dev, const char *id);
+int devm_regulator_get_enable_optional(struct device *dev, const char *id);
 void regulator_put(struct regulator *regulator);
 void devm_regulator_put(struct regulator *regulator);
 
@@ -354,6 +356,17 @@ devm_regulator_get_exclusive(struct device *dev, const char *id)
 	return ERR_PTR(-ENODEV);
 }
 
+static inline int devm_regulator_get_enable(struct device *dev, const char *id)
+{
+	return -ENODEV;
+}
+
+static inline int devm_regulator_get_enable_optional(struct device *dev,
+						     const char *id)
+{
+	return -ENODEV;
+}
+
 static inline struct regulator *__must_check
 regulator_get_optional(struct device *dev, const char *id)
 {
-- 
2.37.1


-- 
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND

~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
Simon says - in Latin please.
~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
Thanks to Simon Glass for the translation =] 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[RFC PATCH 2/5] regulator: Add devm helpers for get and enable

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 6614 bytes --]

A few regulator consumer drivers seem to be just getting a regulator,
enabling it and registering a devm-action to disable the regulator at
the driver detach and then forget about it.

We can simplify this a bit by adding a devm-helper for this pattern.
Add devm_regulator_get_enable() and devm_regulator_get_enable_optional()

Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
Link: https://lore.kernel.org/r/ed7b8841193bb9749d426f3cb3b199c9460794cd.1660292316.git.mazziesaccount@gmail.com
Signed-off-by: Mark Brown <broonie@kernel.org>
---

Please note - this patch is already in Mark's tree. It's here just to
allow compilation of the series on top of v6.0-rc4

 drivers/regulator/devres.c         | 105 +++++++++++++++++++++++++++++
 include/linux/regulator/consumer.h |  14 ++++
 2 files changed, 119 insertions(+)

diff --git a/drivers/regulator/devres.c b/drivers/regulator/devres.c
index 3cb3eb49ad8a..3265e75e97ab 100644
--- a/drivers/regulator/devres.c
+++ b/drivers/regulator/devres.c
@@ -253,6 +253,111 @@ int devm_regulator_bulk_get_const(struct device *dev, int num_consumers,
 }
 EXPORT_SYMBOL_GPL(devm_regulator_bulk_get_const);
 
+static int devm_regulator_bulk_match(struct device *dev, void *res,
+				     void *data)
+{
+	struct regulator_bulk_devres *match = res;
+	struct regulator_bulk_data *target = data;
+
+	/*
+	 * We check the put uses same consumer list as the get did.
+	 * We _could_ scan all entries in consumer array and check the
+	 * regulators match but ATM I don't see the need. We can change this
+	 * later if needed.
+	 */
+	return match->consumers == target;
+}
+
+/**
+ * devm_regulator_bulk_put - Resource managed regulator_bulk_put()
+ * @consumers: consumers to free
+ *
+ * Deallocate regulators allocated with devm_regulator_bulk_get(). Normally
+ * this function will not need to be called and the resource management
+ * code will ensure that the resource is freed.
+ */
+void devm_regulator_bulk_put(struct regulator_bulk_data *consumers)
+{
+	int rc;
+	struct regulator *regulator = consumers[0].consumer;
+
+	rc = devres_release(regulator->dev, devm_regulator_bulk_release,
+			    devm_regulator_bulk_match, consumers);
+	if (rc != 0)
+		WARN_ON(rc);
+}
+EXPORT_SYMBOL_GPL(devm_regulator_bulk_put);
+
+static void devm_regulator_bulk_disable(void *res)
+{
+	struct regulator_bulk_devres *devres = res;
+	int i;
+
+	for (i = 0; i < devres->num_consumers; i++)
+		regulator_disable(devres->consumers[i].consumer);
+}
+
+/**
+ * devm_regulator_bulk_get_enable - managed get'n enable multiple regulators
+ *
+ * @dev:           device to supply
+ * @num_consumers: number of consumers to register
+ * @id:            list of supply names or regulator IDs
+ *
+ * @return 0 on success, an errno on failure.
+ *
+ * This helper function allows drivers to get several regulator
+ * consumers in one operation with management, the regulators will
+ * automatically be freed when the device is unbound.  If any of the
+ * regulators cannot be acquired then any regulators that were
+ * allocated will be freed before returning to the caller.
+ */
+int devm_regulator_bulk_get_enable(struct device *dev, int num_consumers,
+				   const char * const *id)
+{
+	struct regulator_bulk_devres *devres;
+	struct regulator_bulk_data *consumers;
+	int i, ret;
+
+	devres = devm_kmalloc(dev, sizeof(*devres), GFP_KERNEL);
+	if (!devres)
+		return -ENOMEM;
+
+	devres->consumers = devm_kcalloc(dev, num_consumers, sizeof(*consumers),
+					 GFP_KERNEL);
+	consumers = devres->consumers;
+	if (!consumers)
+		return -ENOMEM;
+
+	devres->num_consumers = num_consumers;
+
+	for (i = 0; i < num_consumers; i++)
+		consumers[i].supply = id[i];
+
+	ret = devm_regulator_bulk_get(dev, num_consumers, consumers);
+	if (ret)
+		return ret;
+
+	for (i = 0; i < num_consumers; i++) {
+		ret = regulator_enable(consumers[i].consumer);
+		if (ret)
+			goto unwind;
+	}
+
+	ret = devm_add_action(dev, devm_regulator_bulk_disable, devres);
+	if (!ret)
+		return 0;
+
+unwind:
+	while (--i >= 0)
+		regulator_disable(consumers[i].consumer);
+
+	devm_regulator_bulk_put(consumers);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(devm_regulator_bulk_get_enable);
+
 static void devm_rdev_release(struct device *dev, void *res)
 {
 	regulator_unregister(*(struct regulator_dev **)res);
diff --git a/include/linux/regulator/consumer.h b/include/linux/regulator/consumer.h
index 8e6cf65851b0..ee3b4a014611 100644
--- a/include/linux/regulator/consumer.h
+++ b/include/linux/regulator/consumer.h
@@ -246,12 +246,15 @@ int __must_check regulator_bulk_get(struct device *dev, int num_consumers,
 				    struct regulator_bulk_data *consumers);
 int __must_check devm_regulator_bulk_get(struct device *dev, int num_consumers,
 					 struct regulator_bulk_data *consumers);
+void devm_regulator_bulk_put(struct regulator_bulk_data *consumers);
 int __must_check devm_regulator_bulk_get_const(
 	struct device *dev, int num_consumers,
 	const struct regulator_bulk_data *in_consumers,
 	struct regulator_bulk_data **out_consumers);
 int __must_check regulator_bulk_enable(int num_consumers,
 				       struct regulator_bulk_data *consumers);
+int devm_regulator_bulk_get_enable(struct device *dev, int num_consumers,
+				   const char * const *id);
 int regulator_bulk_disable(int num_consumers,
 			   struct regulator_bulk_data *consumers);
 int regulator_bulk_force_disable(int num_consumers,
@@ -388,6 +391,10 @@ static inline void devm_regulator_put(struct regulator *regulator)
 {
 }
 
+static inline void devm_regulator_bulk_put(struct regulator_bulk_data *consumers)
+{
+}
+
 static inline int regulator_register_supply_alias(struct device *dev,
 						  const char *id,
 						  struct device *alias_dev,
@@ -478,6 +485,13 @@ static inline int regulator_bulk_enable(int num_consumers,
 	return 0;
 }
 
+static inline int devm_regulator_bulk_get_enable(struct device *dev,
+						 int num_consumers,
+						 const char * const *id)
+{
+	return 0;
+}
+
 static inline int regulator_bulk_disable(int num_consumers,
 					 struct regulator_bulk_data *consumers)
 {
-- 
2.37.1


-- 
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND

~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
Simon says - in Latin please.
~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
Thanks to Simon Glass for the translation =] 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[RFC PATCH 3/5] dt-bindings: iio: Add KX022A accelerometer

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 2835 bytes --]

KX022A is a 3-axis Accelerometer from ROHM/Kionix. The senor features
include variable ODRs, I2C and SPI control, FIFO/LIFO with watermark IRQ,
tap/motion detection, wake-up & back-to-sleep events, four acceleration
ranges (2, 4, 8 and 16g) and probably some other cool fatures.

Add the basic device tree description for the accelerometer. Only basic
accelerometer features are considered as of now - new properties may or
may not be needed in the future if rest of the features are to be supported.

Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
---
 .../bindings/iio/accel/kionix,kx022a.yaml     | 58 +++++++++++++++++++
 1 file changed, 58 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/accel/kionix,kx022a.yaml

diff --git a/Documentation/devicetree/bindings/iio/accel/kionix,kx022a.yaml b/Documentation/devicetree/bindings/iio/accel/kionix,kx022a.yaml
new file mode 100644
index 000000000000..62a0c7991a62
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/accel/kionix,kx022a.yaml
@@ -0,0 +1,58 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/accel/kionix,kx022a.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: ROHM/Kionix KX022A Accelerometer bindings
+
+maintainers:
+  - Matti Vaittinen <matti.vaittinen@fi.rohmeurope.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.
+
+properties:
+  compatible: kionix,kx022a
+
+  reg:
+    description:
+      I2C slave address or SPI chip-select.
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  vdd-supply: true
+  io_vdd-supply: true
+
+  mount-matrix:
+    description: |
+      an optional 3x3 mounting rotation matrix.
+
+required:
+  - compatible
+  - reg
+  - interrupts
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+          accel@1f {
+            compatible = "kionix,kx022a";
+            reg = <0x1f>;
+
+            interrupt-parent = <&gpio1>;
+            interrupts = <29 IRQ_TYPE_LEVEL_LOW>;
+
+            io_vdd-supply = <&iovdd>;
+            vdd-supply = <&vdd>;
+        };
+    };
-- 
2.37.1


-- 
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND

~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
Simon says - in Latin please.
~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
Thanks to Simon Glass for the translation =] 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC PATCH 3/5] dt-bindings: iio: Add KX022A accelerometer

[Krzysztof Kozlowski]

On 21/09/2022 13:45, Matti Vaittinen wrote:
> KX022A is a 3-axis Accelerometer from ROHM/Kionix. The senor features
> include variable ODRs, I2C and SPI control, FIFO/LIFO with watermark IRQ,

Thank you for your patch. There is something to discuss/improve.

> tap/motion detection, wake-up & back-to-sleep events, four acceleration
> ranges (2, 4, 8 and 16g) and probably some other cool fatures.

s/fatures/features/

> +$id: http://devicetree.org/schemas/iio/accel/kionix,kx022a.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: ROHM/Kionix KX022A Accelerometer bindings

Drop "bindings"

> +
> +maintainers:
> +  - Matti Vaittinen <matti.vaittinen@fi.rohmeurope.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.
> +
> +properties:
> +  compatible: kionix,kx022a

Missing const. I wonder how did it pass testing...

> +
> +  reg:
> +    description:
> +      I2C slave address or SPI chip-select.

Skip description, it's obvious.

> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  vdd-supply: true
> +  io_vdd-supply: true

No underscores, so io-vdd-supply

> +
> +  mount-matrix:
> +    description: |
> +      an optional 3x3 mounting rotation matrix.
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +          accel@1f {

Messed up indentation.

> +            compatible = "kionix,kx022a";
> +            reg = <0x1f>;

Best regards,
Krzysztof

Re: [RFC PATCH 3/5] dt-bindings: iio: Add KX022A accelerometer

[Vaittinen, Matti]

Hi dee Ho Krzysztof,

Thanks for looking through this!

On 9/21/22 22:11, Krzysztof Kozlowski wrote:
> On 21/09/2022 13:45, Matti Vaittinen wrote:
>> KX022A is a 3-axis Accelerometer from ROHM/Kionix. The senor features
>> include variable ODRs, I2C and SPI control, FIFO/LIFO with watermark IRQ,
>> +
>> +maintainers:
>> +  - Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>

My own comment - switch the email to the gmail-one. Company mail is 
unreliable at best..

>> +
>> +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.
>> +
>> +properties:
>> +  compatible: kionix,kx022a
> 
> Missing const. I wonder how did it pass testing...

I originally had
oneOf:
  items const ...
construct here as I had separate compatibles for *-spi and *-i2c. I am 
unsure if I remembered to run the tests after dropping the extra 
compatibles :| - Sorry! I'll fix this.

>> +  io_vdd-supply: true
> 
> No underscores, so io-vdd-supply

The rationale behind the underscore is that the data-sheet uses terms 
vdd and vdd_io (with underscore). I wanted to match the supply name to 
what is used in the data-sheet. Not a big thing but I'd rather kept if 
same as the data-sheet if the requirement of "no-underscores" is not 
"hard". (If it is, then I'll drop the underscore).

Other than that I agree with all of your points. Thanks for checking 
this! Appreciated!

Yours,
	--Matti

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

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

Re: [RFC PATCH 3/5] dt-bindings: iio: Add KX022A accelerometer

[Krzysztof Kozlowski]

On 21/09/2022 21:30, Vaittinen, Matti wrote:
> Hi dee Ho Krzysztof,
> 
> Thanks for looking through this!
> 
> On 9/21/22 22:11, Krzysztof Kozlowski wrote:
>> On 21/09/2022 13:45, Matti Vaittinen wrote:
>>> KX022A is a 3-axis Accelerometer from ROHM/Kionix. The senor features
>>> include variable ODRs, I2C and SPI control, FIFO/LIFO with watermark IRQ,
>>> +
>>> +maintainers:
>>> +  - Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
> 
> My own comment - switch the email to the gmail-one. Company mail is 
> unreliable at best..
> 
>>> +
>>> +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.
>>> +
>>> +properties:
>>> +  compatible: kionix,kx022a
>>
>> Missing const. I wonder how did it pass testing...
> 
> I originally had
> oneOf:
>   items const ...
> construct here as I had separate compatibles for *-spi and *-i2c. I am 
> unsure if I remembered to run the tests after dropping the extra 
> compatibles :| - Sorry! I'll fix this.

This should be just:
  compatible:
    const: foo,bar

> 
>>> +  io_vdd-supply: true
>>
>> No underscores, so io-vdd-supply
> 
> The rationale behind the underscore is that the data-sheet uses terms 
> vdd and vdd_io (with underscore). I wanted to match the supply name to 
> what is used in the data-sheet. Not a big thing but I'd rather kept if 
> same as the data-sheet if the requirement of "no-underscores" is not 
> "hard". (If it is, then I'll drop the underscore).

Underscores trigger warnings at some dtc W level (W=1 or W=2) so they
are not allowed.


Best regards,
Krzysztof

Re: [RFC PATCH 3/5] dt-bindings: iio: Add KX022A accelerometer

[Matti Vaittinen]

Good Morning Krzysztof,

On 9/21/22 22:56, Krzysztof Kozlowski wrote:
> On 21/09/2022 21:30, Vaittinen, Matti wrote:
>> Hi dee Ho Krzysztof,
>>
>> Thanks for looking through this!
>>
>> On 9/21/22 22:11, Krzysztof Kozlowski wrote:
>>> On 21/09/2022 13:45, Matti Vaittinen wrote:
>>>> KX022A is a 3-axis Accelerometer from ROHM/Kionix. The senor features
>>>> include variable ODRs, I2C and SPI control, FIFO/LIFO with watermark IRQ,
>>>> +
>>>> +maintainers:
>>>> +  - Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
>>
>> My own comment - switch the email to the gmail-one. Company mail is
>> unreliable at best..
>>
>>>> +
>>>> +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.
>>>> +
>>>> +properties:
>>>> +  compatible: kionix,kx022a
>>>
>>> Missing const. I wonder how did it pass testing...
>>
>> I originally had
>> oneOf:
>>    items const ...
>> construct here as I had separate compatibles for *-spi and *-i2c. I am
>> unsure if I remembered to run the tests after dropping the extra
>> compatibles :| - Sorry! I'll fix this.
> 
> This should be just:
>    compatible:
>      const: foo,bar
> 
>>
>>>> +  io_vdd-supply: true
>>>
>>> No underscores, so io-vdd-supply
>>
>> The rationale behind the underscore is that the data-sheet uses terms
>> vdd and vdd_io (with underscore). I wanted to match the supply name to
>> what is used in the data-sheet. Not a big thing but I'd rather kept if
>> same as the data-sheet if the requirement of "no-underscores" is not
>> "hard". (If it is, then I'll drop the underscore).
> 
> Underscores trigger warnings at some dtc W level (W=1 or W=2) so they
> are not allowed.

Thanks for the explanation. I'll change this too.

Yours,
	-- Matti

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

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

[RFC PATCH 4/5] iio: accel: Support Kionix/ROHM KX022A accelerometer

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 40432 bytes --]

KX022A is a 3-axis Accelerometer from ROHM/Kionix. The senor features
include variable ODRs, I2C and SPI control, FIFO/LIFO with watermark IRQ,
tap/motion detection, wake-up & back-to-sleep events, four acceleration
ranges (2, 4, 8 and 16g) and probably some other cool fatures.

Add support for the basic accelerometer features such as getting the
acceleration data via IIO. (raw reads, triggered buffer [data-ready] or
using the WMI IRQ).

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

NOTE: Filling-up the hardware FIFO should be avoided. During my testing
I noticed that filling up the hardware FIFO might mess-up the sample
count. My sensor ended up in a state where amount of data in FIFO was
reported to be 0xff bytes, which equals to 42,5 samples. Specification
says the FIFO can hold maximum of 41 samples in HiRes mode. Also, at
least once the FIFO was stuck in a state where reading data from
hwardware FIFO did not decrease the amount of data reported to be in the
FIFO - eg. FIFO was "stuck". The code has now an error count and 10
reads with invalid FIFO data count will cause the fifo contents to be
dropped.

Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
---
 drivers/iio/accel/Kconfig             |   23 +
 drivers/iio/accel/Makefile            |    3 +
 drivers/iio/accel/kionix-kx022a-i2c.c |   52 ++
 drivers/iio/accel/kionix-kx022a-spi.c |   50 ++
 drivers/iio/accel/kionix-kx022a.c     | 1149 +++++++++++++++++++++++++
 drivers/iio/accel/kionix-kx022a.h     |   76 ++
 6 files changed, 1353 insertions(+)
 create mode 100644 drivers/iio/accel/kionix-kx022a-i2c.c
 create mode 100644 drivers/iio/accel/kionix-kx022a-spi.c
 create mode 100644 drivers/iio/accel/kionix-kx022a.c
 create mode 100644 drivers/iio/accel/kionix-kx022a.h

diff --git a/drivers/iio/accel/Kconfig b/drivers/iio/accel/Kconfig
index 35798712f811..12ad6bcb2c76 100644
--- a/drivers/iio/accel/Kconfig
+++ b/drivers/iio/accel/Kconfig
@@ -409,6 +409,29 @@ config IIO_ST_ACCEL_SPI_3AXIS
 	  To compile this driver as a module, choose M here. The module
 	  will be called st_accel_spi.
 
+config IIO_KX022A
+	tristate
+
+config IIO_KX022A_SPI
+	tristate "Kionix KX022A tri-axis digital accelerometer"
+	depends on I2C
+	select IIO_KX022A
+	select REGMAP_SPI
+	help
+	  Say Y here to enable support for the Kionix KX022A digital tri-axis
+	  accelerometer connected to I2C interface. See IIO_KX022A_I2C if
+	  you want to enable support for the KX022A connected via I2C.
+
+config IIO_KX022A_I2C
+	tristate "Kionix KX022A tri-axis digital accelerometer"
+	depends on I2C
+	select IIO_KX022A
+	select REGMAP_I2C
+	help
+	  Say Y here to enable support for the Kionix KX022A digital tri-axis
+	  accelerometer connected to I2C interface. See IIO_KX022A_SPI if
+	  you want to enable support for the KX022A connected via SPI.
+
 config KXSD9
 	tristate "Kionix KXSD9 Accelerometer Driver"
 	select IIO_BUFFER
diff --git a/drivers/iio/accel/Makefile b/drivers/iio/accel/Makefile
index 4d8792668838..7bd654b74f42 100644
--- a/drivers/iio/accel/Makefile
+++ b/drivers/iio/accel/Makefile
@@ -40,6 +40,9 @@ obj-$(CONFIG_FXLS8962AF)	+= fxls8962af-core.o
 obj-$(CONFIG_FXLS8962AF_I2C)	+= fxls8962af-i2c.o
 obj-$(CONFIG_FXLS8962AF_SPI)	+= fxls8962af-spi.o
 obj-$(CONFIG_HID_SENSOR_ACCEL_3D) += hid-sensor-accel-3d.o
+obj-$(CONFIG_IIO_KX022A)	+= kionix-kx022a.o
+obj-$(CONFIG_IIO_KX022A_I2C)	+= kionix-kx022a-i2c.o
+obj-$(CONFIG_IIO_KX022A_SPI)	+= kionix-kx022a-spi.o
 obj-$(CONFIG_KXCJK1013) += kxcjk-1013.o
 obj-$(CONFIG_KXSD9)	+= kxsd9.o
 obj-$(CONFIG_KXSD9_SPI)	+= kxsd9-spi.o
diff --git a/drivers/iio/accel/kionix-kx022a-i2c.c b/drivers/iio/accel/kionix-kx022a-i2c.c
new file mode 100644
index 000000000000..2dbf8a33b84b
--- /dev/null
+++ b/drivers/iio/accel/kionix-kx022a-i2c.c
@@ -0,0 +1,52 @@
+// SPDX-License-Identifier: GPL-2.0-only
+//
+// Copyright (C) 2022 ROHM Semiconductors
+//
+// ROHM/KIONIX KX022A accelerometer driver
+
+#include <linux/i2c.h>
+#include <linux/interrupt.h>
+#include <linux/module.h>
+#include <linux/regmap.h>
+
+#include "kionix-kx022a.h"
+
+static int kx022a_i2c_probe(struct i2c_client *i2c)
+{
+	struct regmap *regmap;
+	struct device *dev = &i2c->dev;
+
+	if (!i2c->irq) {
+		dev_err(dev, "No IRQ configured\n");
+		return -EINVAL;
+	}
+
+	regmap = devm_regmap_init_i2c(i2c, &kx022a_regmap);
+	if (IS_ERR(regmap)) {
+		dev_err(dev, "Failed to initialize Regmap\n");
+
+		return PTR_ERR(regmap);
+	}
+
+	return kx022a_probe_internal(dev, i2c->irq);
+}
+
+static const struct of_device_id kx022a_of_match[] = {
+	{ .compatible = "kionix,kx022a", },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, kx022a_of_match);
+
+static struct i2c_driver kx022a_i2c_driver = {
+	.driver = {
+			.name  = "kx022a-i2c",
+			.of_match_table = kx022a_of_match,
+		  },
+	.probe_new    = kx022a_i2c_probe,
+};
+
+module_i2c_driver(kx022a_i2c_driver);
+
+MODULE_DESCRIPTION("ROHM/Kionix KX022A accelerometer driver");
+MODULE_AUTHOR("Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>");
+MODULE_LICENSE("GPL");
diff --git a/drivers/iio/accel/kionix-kx022a-spi.c b/drivers/iio/accel/kionix-kx022a-spi.c
new file mode 100644
index 000000000000..007f4b726d8f
--- /dev/null
+++ b/drivers/iio/accel/kionix-kx022a-spi.c
@@ -0,0 +1,50 @@
+// SPDX-License-Identifier: GPL-2.0-only
+//
+// Copyright (C) 2022 ROHM Semiconductors
+//
+// ROHM/KIONIX KX022A accelerometer driver
+
+#include <linux/interrupt.h>
+#include <linux/module.h>
+#include <linux/regmap.h>
+#include <linux/spi/spi.h>
+
+#include "kionix-kx022a.h"
+
+static int kx022a_spi_probe(struct spi_device *spi)
+{
+	struct device *dev = &spi->dev;
+	struct regmap *regmap;
+
+	if (!spi->irq) {
+		dev_err(dev, "No IRQ configured\n");
+		return -EINVAL;
+	}
+
+	regmap = devm_regmap_init_spi(spi, &kx022a_regmap);
+	if (IS_ERR(regmap)) {
+		dev_err(dev, "Failed to initialize Regmap\n");
+
+		return PTR_ERR(regmap);
+	}
+	return kx022a_probe_internal(dev, spi->irq);
+}
+
+static const struct of_device_id kx022a_of_match[] = {
+	{ .compatible = "kionix,kx022a", },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, kx022a_of_match);
+
+static struct spi_driver kx022a_spi_driver = {
+	.driver = {
+		.name   = "kx022a-spi",
+	},
+	.probe	= kx022a_spi_probe,
+};
+
+module_spi_driver(kx022a_spi_driver);
+
+MODULE_DESCRIPTION("ROHM/Kionix kx022A accelerometer driver");
+MODULE_AUTHOR("Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>");
+MODULE_LICENSE("GPL");
diff --git a/drivers/iio/accel/kionix-kx022a.c b/drivers/iio/accel/kionix-kx022a.c
new file mode 100644
index 000000000000..9f9e0d7efc09
--- /dev/null
+++ b/drivers/iio/accel/kionix-kx022a.c
@@ -0,0 +1,1149 @@
+// SPDX-License-Identifier: GPL-2.0-only
+//
+// Copyright (C) 2022 ROHM Semiconductors
+//
+// ROHM/KIONIX KX022A accelerometer driver
+
+#include <linux/delay.h>
+#include <linux/gpio.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+#include <linux/iio/trigger.h>
+#include <linux/iio/trigger_consumer.h>
+#include <linux/iio/triggered_buffer.h>
+#include <linux/interrupt.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/of.h>
+#include <linux/of_gpio.h>
+#include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
+#include <linux/slab.h>
+
+#include "kionix-kx022a.h"
+
+/*
+ * Threshold for deciding our HW fifo is unrecoverably corrupt and should be
+ * cleared
+ */
+#define KXO22A_FIFO_ERR_THRESHOLD 10
+#define KX022A_FIFO_LENGTH 41
+/* 3 axis, 2 bytes of data for each of the axis */
+#define KX022A_FIFO_SAMPLES_SIZE_BYTES 6
+#define KX022A_FIFO_MAX_BYTES (KX022A_FIFO_LENGTH *			\
+			       KX022A_FIFO_SAMPLES_SIZE_BYTES)
+
+#define MIN_ODR_INTERVAL_MS 5
+#define MAX_ODR_INTERVAL_MS 1280
+#define NUM_SUPPORTED_ODR 9
+
+enum {
+	KX022A_STATE_SAMPLE,
+	KX022A_STATE_FIFO,
+};
+
+/* Regmap configs */
+static const struct regmap_range kx022a_volatile_ranges[] = {
+	{
+		.range_min = KX022A_REG_XHP_L,
+		.range_max = KX022A_REG_COTR,
+	}, {
+		.range_min = KX022A_REG_TSCP,
+		.range_max = KX022A_REG_INT_REL,
+	}, {
+		.range_min = KX022A_REG_BUF_STATUS_1,
+		.range_max = KX022A_REG_BUF_READ,
+	},
+};
+
+static const struct regmap_access_table kx022a_volatile_regs = {
+	.yes_ranges = &kx022a_volatile_ranges[0],
+	.n_yes_ranges = ARRAY_SIZE(kx022a_volatile_ranges),
+};
+
+static const struct regmap_range kx022a_precious_ranges[] = {
+	{
+		.range_min = KX022A_REG_INT_REL,
+		.range_max = KX022A_REG_INT_REL,
+	},
+};
+
+static const struct regmap_access_table kx022a_precious_regs = {
+	.yes_ranges = &kx022a_precious_ranges[0],
+	.n_yes_ranges = ARRAY_SIZE(kx022a_precious_ranges),
+};
+
+/*
+ * The HW does not set WHO_AM_I reg as read-only but we don't want to write it
+ * so we still include it in the read-only ranges.
+ */
+static const struct regmap_range kx022a_read_only_ranges[] = {
+	{
+		.range_min = KX022A_REG_XHP_L,
+		.range_max = KX022A_REG_INT_REL,
+	}, {
+		.range_min = KX022A_REG_BUF_STATUS_1,
+		.range_max = KX022A_REG_BUF_STATUS_2,
+	}, {
+		.range_min = KX022A_REG_BUF_READ,
+		.range_max = KX022A_REG_BUF_READ,
+	},
+};
+
+static const struct regmap_access_table kx022a_ro_regs = {
+	.no_ranges = &kx022a_read_only_ranges[0],
+	.n_no_ranges = ARRAY_SIZE(kx022a_read_only_ranges),
+};
+
+static const struct regmap_range kx022a_write_only_ranges[] = {
+	{
+		.range_min = KX022A_REG_BTS_WUF_TH,
+		.range_max = KX022A_REG_BTS_WUF_TH,
+	}, {
+		.range_min = KX022A_REG_MAN_WAKE,
+		.range_max = KX022A_REG_MAN_WAKE,
+	}, {
+		.range_min = KX022A_REG_SELF_TEST,
+		.range_max = KX022A_REG_SELF_TEST,
+	}, {
+		.range_min = KX022A_REG_BUF_CLEAR,
+		.range_max = KX022A_REG_BUF_CLEAR,
+	},
+};
+
+static const struct regmap_access_table kx022a_wo_regs = {
+	.no_ranges = &kx022a_write_only_ranges[0],
+	.n_no_ranges = ARRAY_SIZE(kx022a_write_only_ranges),
+};
+
+static const struct regmap_range kx022a_noinc_read_ranges[] = {
+	{
+		.range_min = KX022A_REG_BUF_READ,
+		.range_max = KX022A_REG_BUF_READ,
+	},
+};
+
+static const struct regmap_access_table kx022a_nir_regs = {
+	.yes_ranges = &kx022a_noinc_read_ranges[0],
+	.n_yes_ranges = ARRAY_SIZE(kx022a_noinc_read_ranges),
+};
+
+const struct regmap_config kx022a_regmap = {
+	.reg_bits = 8,
+	.val_bits = 8,
+	.volatile_table = &kx022a_volatile_regs,
+	.rd_table = &kx022a_wo_regs,
+	.wr_table = &kx022a_ro_regs,
+	.rd_noinc_table = &kx022a_nir_regs,
+	.precious_table = &kx022a_precious_regs,
+	.max_register = KX022A_MAX_REGISTER,
+	.cache_type = REGCACHE_RBTREE,
+};
+EXPORT_SYMBOL_GPL(kx022a_regmap);
+
+struct kx022a_data;
+
+struct kx022a_trigger {
+	struct kx022a_data *data;
+	struct iio_trigger *indio_trig;
+	bool enabled;
+	const char *name;
+};
+
+struct kx022a_data {
+	int irq;
+	struct regmap *regmap;
+	struct kx022a_trigger trigger;
+	struct device *dev;
+	unsigned int g_range;
+	struct mutex mutex;
+	unsigned int state;
+
+	int64_t timestamp, old_timestamp;
+	unsigned int odr_ns;
+
+	struct iio_mount_matrix orientation;
+	u8 watermark;
+	/* 3 x 16bit accel data + timestamp */
+	s16 buffer[8];
+	struct {
+		__le16 channels[3];
+		s64 ts __aligned(8);
+	} scan;
+};
+
+static const struct iio_mount_matrix *
+kx022a_get_mount_matrix(const struct iio_dev *idev,
+			const struct iio_chan_spec *chan)
+{
+	struct kx022a_data *data = iio_priv(idev);
+
+	return &data->orientation;
+}
+
+enum {
+	AXIS_X,
+	AXIS_Y,
+	AXIS_Z,
+	AXIS_MAX,
+};
+
+static const unsigned long kx022a_scan_masks[] = {
+					BIT(AXIS_X) | BIT(AXIS_Y) | BIT(AXIS_Z),
+					0};
+
+static const struct iio_chan_spec_ext_info kx022a_ext_info[] = {
+	IIO_MOUNT_MATRIX(IIO_SHARED_BY_TYPE, kx022a_get_mount_matrix),
+	{ },
+};
+
+#define KX022A_ACCEL_CHAN(axis, index)						\
+	{								\
+		.type = IIO_ACCEL,					\
+		.modified = 1,						\
+		.channel2 = IIO_MOD_##axis,				\
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),		\
+		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) |	\
+					BIT(IIO_CHAN_INFO_SAMP_FREQ),	\
+		.ext_info = kx022a_ext_info,				\
+		.address = KX022A_REG_##axis##OUT_L,				\
+		.scan_index = index,					\
+		.scan_type = {                                          \
+			.sign = 's',					\
+			.realbits = 16,					\
+			.storagebits = 16,				\
+			.shift = 0,					\
+			.endianness = IIO_LE,				\
+		},							\
+	}
+
+static const struct iio_chan_spec kx022a_channels[] = {
+	KX022A_ACCEL_CHAN(X, 0),
+	KX022A_ACCEL_CHAN(Y, 1),
+	KX022A_ACCEL_CHAN(Z, 2),
+	IIO_CHAN_SOFT_TIMESTAMP(3),
+};
+
+/*
+ * The sensor HW can support ODR up to 1600 Hz - which is beyond what most of
+ * Linux CPUs can handle w/o dropping samples. Also, the low power mode is not
+ * available for higher sample rates. Thus the driver only supports 200 Hz and
+ * slower ODRs. Slowest being 0.78 Hz
+ */
+static IIO_CONST_ATTR_SAMP_FREQ_AVAIL("0.78 1.563 3.125 6.25 12.5 25 50 100 200");
+static IIO_CONST_ATTR(scale_available,
+		      "598.550415 1197.10083 2394.20166 4788.40332");
+
+static struct attribute *kx022a_attributes[] = {
+	&iio_const_attr_sampling_frequency_available.dev_attr.attr,
+	&iio_const_attr_scale_available.dev_attr.attr,
+	NULL,
+};
+
+static const struct attribute_group kx022a_attrs_group = {
+	.attrs = kx022a_attributes,
+};
+
+struct kx022a_tuple {
+	int val;
+	int val2;
+};
+
+/*
+ * range is typically +-2G/4G/8G/16G, distributed over the amount of bits.
+ * The scale table can be calculated using
+ *	(range / 2^bits) * g = (range / 2^bits) * 9.80665 m/s^2
+ *	=> KX022A uses 16 bit (HiRes mode - assume the low 8 bits are zeroed
+ *	in low-power mode(?) )
+ *	=> +/-2G  => 4 / 2^16 * 9,80665 * 10^6 (to scale to micro)
+ *	=> +/-2G  - 598.550415
+ *	   +/-4G  - 1197.10083
+ *	   +/-8G  - 2394.20166
+ *	   +/-16G - 4788.40332
+ */
+static const struct kx022a_tuple kx022a_scale_table[] = {
+	{ 598, 550415 }, { 1197, 100830 }, { 2394, 201660 }, { 4788, 403320 }
+};
+
+/*
+ * ODR (output data rate) table. First value represents the integer portion of
+ * frequency (Hz), and the second value is the decimal part (uHz).
+ * Eg, 0.78 Hz, 1.563 Hz, ...
+ */
+#define KX922A_DEFAULT_PERIOD 20000000
+static const struct kx022a_tuple kx022a_accel_samp_freq_table[] = {
+	{ 0, 780000 }, { 1, 563000 }, { 3, 125000 }, { 6, 250000 },
+	{ 12, 500000 }, { 25, 0 }, { 50, 0 }, { 100, 0 }, { 200, 0 }
+};
+static const unsigned int kx022a_odrs[] = { 1282051282, 639795266, 320000000,
+	 160000000, 80000000, 40000000, 20000000, 10000000, 5000000 };
+
+/* Find index of tuple matching the given values */
+static int kx022a_find_tuple_index(const struct kx022a_tuple *tuples, int n,
+				   int val, int val2)
+{
+	while (n-- > 0)
+		if (val == tuples[n].val && tuples[n].val2 == val2)
+			return n;
+
+	return -EINVAL;
+}
+
+static void kx022a_reg2freq(unsigned int val,  int *val1, int *val2)
+{
+	*val1 = kx022a_accel_samp_freq_table[val & KX022A_MASK_ODR].val;
+	*val2 = kx022a_accel_samp_freq_table[val & KX022A_MASK_ODR].val2;
+}
+
+static void kx022a_reg2scale(unsigned int val, unsigned int *val1,
+			     unsigned int *val2)
+{
+	val &= KX022A_MASK_GSEL;
+	val >>= KX022A_GSEL_SHIFT;
+
+	*val1 = kx022a_scale_table[val].val;
+	*val2 = kx022a_scale_table[val].val2;
+}
+
+static int __kx022a_turn_on_unlocked(struct kx022a_data *data)
+{
+	int ret;
+
+	ret = regmap_set_bits(data->regmap, KX022A_REG_CNTL, KX022A_MASK_PC1);
+	if (ret)
+		dev_err(data->dev, "Turn ON fail %d\n", ret);
+
+	return ret;
+}
+
+static int __kx022a_turn_off_unlocked(struct kx022a_data *data)
+{
+	int ret;
+
+	ret = regmap_clear_bits(data->regmap, KX022A_REG_CNTL, KX022A_MASK_PC1);
+	if (ret)
+		dev_err(data->dev, "Turn OFF fail %d\n", ret);
+
+	return ret;
+}
+
+static int kx022a_turn_off_lock(struct kx022a_data *data)
+{
+	int ret;
+
+	mutex_lock(&data->mutex);
+	ret = __kx022a_turn_off_unlocked(data);
+	if (ret)
+		mutex_unlock(&data->mutex);
+
+	return ret;
+}
+
+static int kx022a_turn_on_unlock(struct kx022a_data *data)
+{
+	int ret;
+
+	ret = __kx022a_turn_on_unlocked(data);
+	mutex_unlock(&data->mutex);
+
+	return ret;
+}
+
+static int kx022a_write_raw(struct iio_dev *idev,
+			    struct iio_chan_spec const *chan,
+			    int val, int val2, long mask)
+{
+	struct kx022a_data *data = iio_priv(idev);
+	int ret;
+
+	/*
+	 * We should not allow changing scale or frequency when FIFO is running
+	 * as it will mess the timestamp/scale for samples existing in the
+	 * buffer. If this turns out to be an issue we can later change logic
+	 * to internally flush the fifo before reconfiguring so the samples in
+	 * fifo keep matching the freq/scale settings. (Such setup could cause
+	 * issues if users trust the watermark to be reached within known
+	 * time-limit).
+	 */
+	mutex_lock(&data->mutex);
+	if (iio_buffer_enabled(idev)) {
+		ret = -EBUSY;
+		goto unlock_out;
+	}
+	switch (mask) {
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		ret = kx022a_find_tuple_index(&kx022a_accel_samp_freq_table[0],
+					      ARRAY_SIZE(kx022a_accel_samp_freq_table),
+					      val, val2);
+		/* Configure if we found valid ODR */
+		if (ret >= 0) {
+			int odr = ret;
+
+			ret = __kx022a_turn_off_unlocked(data);
+			if (ret)
+				goto unlock_out;
+
+			ret = regmap_update_bits(data->regmap, KX022A_REG_ODCNTL,
+						 KX022A_MASK_ODR, odr);
+			data->odr_ns = kx022a_odrs[odr];
+			__kx022a_turn_on_unlocked(data);
+		}
+		break;
+	case IIO_CHAN_INFO_SCALE:
+		ret = kx022a_find_tuple_index(&kx022a_scale_table[0],
+					      ARRAY_SIZE(kx022a_scale_table),
+					      val, val2);
+		/* Configure if we found valid scale */
+		if (ret >= 0) {
+			ret = __kx022a_turn_off_unlocked(data);
+			if (ret)
+				goto unlock_out;
+
+			ret = regmap_update_bits(data->regmap, KX022A_REG_CNTL,
+						 KX022A_MASK_GSEL,
+						 ret << KX022A_GSEL_SHIFT);
+			__kx022a_turn_on_unlocked(data);
+		}
+	default:
+		ret = -EINVAL;
+	}
+
+unlock_out:
+	mutex_unlock(&data->mutex);
+
+	return ret;
+}
+
+static int kx022a_fifo_set_wmi(struct kx022a_data *data)
+{
+	u8 threshold;
+
+	threshold = data->watermark;
+
+	return regmap_update_bits(data->regmap, KX022A_REG_BUF_CNTL1,
+				  KX022A_MASK_WM_TH, threshold);
+}
+
+static int kx022a_fifo_report_data(struct kx022a_data *data, void *buffer,
+				   int samples)
+{
+	int ret, fifo_bytes;
+
+	fifo_bytes = samples * KX022A_FIFO_SAMPLES_SIZE_BYTES;
+	ret = regmap_noinc_read(data->regmap, KX022A_REG_BUF_READ,
+			       buffer, fifo_bytes);
+	if (ret)
+		dev_err(data->dev, "FIFO read failed %d\n", ret);
+
+	return ret;
+}
+
+static int kx022a_get_axis(struct kx022a_data *data,
+			   struct iio_chan_spec const *chan,
+			   int *val)
+{
+	u16 raw_val;
+	int ret;
+
+	ret = regmap_bulk_read(data->regmap, chan->address, &raw_val,
+			       sizeof(raw_val));
+	if (ret)
+		return ret;
+	*val = raw_val;
+
+	return IIO_VAL_INT;
+}
+
+static int kx022a_read_raw(struct iio_dev *idev,
+			   struct iio_chan_spec const *chan,
+			   int *val, int *val2, long mask)
+{
+	struct kx022a_data *data = iio_priv(idev);
+	unsigned int regval;
+	int ret = -EINVAL;
+
+	mutex_lock(&data->mutex);
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		if (iio_buffer_enabled(idev)) {
+			ret = -EBUSY;
+			goto error_ret;
+		}
+
+		ret = kx022a_get_axis(data, chan, val);
+		break;
+	case IIO_CHAN_INFO_SAMP_FREQ:
+	{
+		ret = regmap_read(data->regmap, KX022A_REG_ODCNTL, &regval);
+		if (ret)
+			goto error_ret;
+
+		if ((regval & KX022A_MASK_ODR) >
+		    ARRAY_SIZE(kx022a_accel_samp_freq_table)) {
+			dev_err(data->dev, "Invalid ODR\n");
+			ret = -EINVAL;
+			goto error_ret;
+		}
+
+		kx022a_reg2freq(regval, val, val2);
+		ret = IIO_VAL_INT_PLUS_MICRO;
+
+		break;
+	}
+	case IIO_CHAN_INFO_SCALE:
+		ret = regmap_read(data->regmap, KX022A_REG_CNTL, &regval);
+		if (ret < 0)
+			goto error_ret;
+
+		kx022a_reg2scale(regval, val, val2);
+
+		ret = IIO_VAL_INT_PLUS_MICRO;
+		break;
+	}
+
+error_ret:
+	mutex_unlock(&data->mutex);
+
+	return ret;
+};
+
+static int kx022a_validate_trigger(struct iio_dev *idev,
+				   struct iio_trigger *trig)
+{
+	struct kx022a_data *data = iio_priv(idev);
+
+	if (data->trigger.indio_trig == trig)
+		return 0;
+
+	return -EINVAL;
+}
+
+static int kx022a_set_watermark(struct iio_dev *idev, unsigned int val)
+{
+	struct kx022a_data *data = iio_priv(idev);
+
+	if (val > KX022A_FIFO_LENGTH)
+		val = KX022A_FIFO_LENGTH;
+
+	mutex_lock(&data->mutex);
+	data->watermark = val;
+	mutex_unlock(&data->mutex);
+
+	return 0;
+}
+
+static ssize_t kx022a_get_fifo_state(struct device *dev,
+				     struct device_attribute *attr,
+				     char *buf)
+{
+	struct iio_dev *idev = dev_to_iio_dev(dev);
+	struct kx022a_data *data = iio_priv(idev);
+	bool state;
+
+	mutex_lock(&data->mutex);
+	state = data->state;
+	mutex_unlock(&data->mutex);
+
+	return sprintf(buf, "%d\n", state);
+}
+
+static ssize_t kx022a_get_fifo_watermark(struct device *dev,
+					 struct device_attribute *attr,
+					 char *buf)
+{
+	struct iio_dev *idev = dev_to_iio_dev(dev);
+	struct kx022a_data *data = iio_priv(idev);
+	int wm;
+
+	mutex_lock(&data->mutex);
+	wm = data->watermark;
+	mutex_unlock(&data->mutex);
+
+	return sprintf(buf, "%d\n", wm);
+}
+
+static IIO_DEVICE_ATTR(hwfifo_enabled, 0444,
+		       kx022a_get_fifo_state, NULL, 0);
+static IIO_DEVICE_ATTR(hwfifo_watermark, 0444,
+		       kx022a_get_fifo_watermark, NULL, 0);
+
+static const struct attribute *kx022a_fifo_attributes[] = {
+	&iio_dev_attr_hwfifo_watermark.dev_attr.attr,
+	&iio_dev_attr_hwfifo_enabled.dev_attr.attr,
+	NULL,
+};
+
+static int kx022a_drop_fifo_contents(struct kx022a_data *data)
+{
+	/*
+	 * We must clear the old time-stamp to avoid computing the timestamps
+	 * based on samples acquired when buffer was last enabled
+	 */
+	data->timestamp = 0;
+
+	return regmap_write(data->regmap, KX022A_REG_BUF_CLEAR, 0xff);
+}
+
+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);
+	int ret, i;
+	int count, fifo_bytes;
+	u16 buffer[KX022A_FIFO_LENGTH * 3];
+	int64_t tstamp;
+	uint64_t sample_period;
+	static int err_ctr;
+
+	ret = regmap_read(data->regmap, KX022A_REG_BUF_STATUS_1, &fifo_bytes);
+	if (ret < 0) {
+		dev_err(dev, "Error reading buffer status\n");
+		return ret;
+	}
+
+	/* Let's not owerflow if we for some reason get bogus value from i2c */
+	if (fifo_bytes > KX022A_FIFO_MAX_BYTES) {
+		/*
+		 * I've observed a strange behaviour where FIFO may get stuck if
+		 * samples are not read out fast enough. By 'stuck' I mean
+		 * situation where amount of data adverticed by the STATUS_1
+		 * reg is 255 - which equals to 42,5 (sic!) samples and by
+		 * my experimenting there are situations where reading the
+		 * FIFO buffer does not decrease the data count but the same
+		 * fifo sample level (255 bytes of data) is reported
+		 */
+		err_ctr++;
+		dev_warn(data->dev, "Bad amount of data %u\n", fifo_bytes);
+		fifo_bytes = KX022A_FIFO_MAX_BYTES;
+	} else if (fifo_bytes % KX022A_FIFO_SAMPLES_SIZE_BYTES) {
+		err_ctr++;
+		dev_err(data->dev, "Bad FIFO alignment. Data may be corrupt\n");
+	} else {
+		err_ctr = 0;
+	}
+
+	if (err_ctr > KXO22A_FIFO_ERR_THRESHOLD) {
+		__kx022a_turn_off_unlocked(data);
+		kx022a_drop_fifo_contents(data);
+		__kx022a_turn_on_unlocked(data);
+
+		err_ctr = 0;
+
+		return -EINVAL;
+	}
+
+	count = fifo_bytes / KX022A_FIFO_SAMPLES_SIZE_BYTES;
+	if (!count)
+		return 0;
+
+	/*
+	 * If we are being called from IRQ handler we know the stored timestamp
+	 * is fairly accurate for the last stored sample. Otherwise, if we are
+	 * called as a result of a read operation from userspace and hence
+	 * before the watermark interrupt was triggered, take a timestamp
+	 * now. We can fall anywhere in between two samples so the error in this
+	 * case is at most one sample period.
+	 */
+	if (!irq) {
+		data->old_timestamp = data->timestamp;
+		data->timestamp = iio_get_time_ns(idev);
+	}
+
+	/*
+	 * Approximate timestamps for each of the sample based on the sampling
+	 * frequency, timestamp for last sample and number of samples.
+	 *
+	 * We'd better not use the current bandwidth settings to compute the
+	 * sample period. The real sample rate varies with the device and
+	 * small variation adds when we store a large number of samples.
+	 *
+	 * To avoid this issue we compute the actual sample period ourselves
+	 * based on the timestamp delta between the last two flush operations.
+	 */
+	if (data->old_timestamp) {
+		sample_period = (data->timestamp - data->old_timestamp);
+		do_div(sample_period, count);
+	} else {
+		sample_period = data->odr_ns;
+	}
+	tstamp = data->timestamp - (count - 1) * sample_period;
+
+	if (samples && count > samples) {
+		/*
+		 * Here we leave some old samples to the buffer. We need to
+		 * adjust the timestamp to match the first sample in the buffer
+		 * or we will miscalculate the sample_period at next round.
+		 */
+		data->timestamp -= (count - samples) * sample_period;
+		count = samples;
+	}
+
+	ret = kx022a_fifo_report_data(data, buffer, count);
+	if (ret)
+		return ret;
+
+	for (i = 0; i < count; i++) {
+		int bit;
+		u16 *samples = &buffer[i * 3];
+
+		for_each_set_bit(bit, idev->active_scan_mask, AXIS_MAX)
+			memcpy(&data->scan.channels[bit], &samples[bit],
+			       sizeof(data->scan.channels[0]));
+
+		iio_push_to_buffers_with_timestamp(idev, &data->scan, tstamp);
+
+		tstamp += sample_period;
+	}
+
+	return count;
+}
+
+static int kx022a_fifo_flush(struct iio_dev *idev, unsigned int samples)
+{
+	struct kx022a_data *data = iio_priv(idev);
+	int ret;
+
+	mutex_lock(&data->mutex);
+	ret = __kx022a_fifo_flush(idev, samples, false);
+	mutex_unlock(&data->mutex);
+
+	return ret;
+}
+
+static const struct iio_info kx022a_info = {
+	.read_raw = &kx022a_read_raw,
+	.write_raw = &kx022a_write_raw,
+	.attrs = &kx022a_attrs_group,
+
+	.validate_trigger	= kx022a_validate_trigger,
+	.hwfifo_set_watermark	= kx022a_set_watermark,
+	.hwfifo_flush_to_buffer	= kx022a_fifo_flush,
+};
+
+static int kx022a_set_drdy_irq(struct kx022a_data *data, bool en)
+{
+	if (en)
+		return regmap_set_bits(data->regmap, KX022A_REG_CNTL,
+				       KX022A_MASK_DRDY);
+
+	return regmap_clear_bits(data->regmap, KX022A_REG_CNTL,
+				 KX022A_MASK_DRDY);
+}
+
+static int kx022a_prepare_irq_pin(struct kx022a_data *data)
+{
+	/* Enable IRQ1 pin. Set polarity to active low */
+	int mask = KX022A_MASK_IEN1 | KX022A_MASK_IPOL1 |
+		   KX022A_MASK_ITYP;
+	int val = KX022A_MASK_IEN1 | KX022A_IPOL_LOW |
+		  KX022A_ITYP_LEVEL;
+	int ret;
+
+	ret = regmap_update_bits(data->regmap, KX022A_REG_INC1, mask, val);
+	if (ret)
+		return ret;
+
+	mask = KX022A_MASK_INS2_DRDY | KX122_MASK_INS2_WMI;
+
+	return regmap_set_bits(data->regmap, KX022A_REG_INC4, mask);
+}
+
+static int kx022a_fifo_disable(struct kx022a_data *data)
+{
+	int ret = 0;
+
+	/* PC1 to 0 */
+	ret = kx022a_turn_off_lock(data);
+	if (ret)
+		return ret;
+
+	ret = regmap_clear_bits(data->regmap, KX022A_REG_INC4,
+				KX022A_MASK_WMI1);
+	if (ret)
+		goto unlock_out;
+
+	/* disable buffer */
+	ret = regmap_clear_bits(data->regmap, KX022A_REG_BUF_CNTL2,
+				KX022A_MASK_BUF_EN);
+	if (ret)
+		goto unlock_out;
+
+	data->state &= (~KX022A_STATE_FIFO);
+
+	kx022a_drop_fifo_contents(data);
+
+	return kx022a_turn_on_unlock(data);
+
+unlock_out:
+	mutex_unlock(&data->mutex);
+
+	return ret;
+}
+
+static int kx022a_buffer_predisable(struct iio_dev *idev)
+{
+	struct kx022a_data *data = iio_priv(idev);
+
+	if (iio_device_get_current_mode(idev) == INDIO_BUFFER_TRIGGERED)
+		return 0;
+
+	return kx022a_fifo_disable(data);
+}
+
+static int kx022a_fifo_enable(struct kx022a_data *data)
+{
+	int ret = 0;
+
+	ret = kx022a_turn_off_lock(data);
+	if (ret)
+		return ret;
+
+	/* Write WMI to HW */
+	ret = kx022a_fifo_set_wmi(data);
+	if (ret)
+		goto unlock_out;
+
+	/* Enable buffer */
+	ret = regmap_set_bits(data->regmap, KX022A_REG_BUF_CNTL2,
+			      KX022A_MASK_BUF_EN);
+	if (ret)
+		goto unlock_out;
+
+	data->state |= KX022A_STATE_FIFO;
+	ret = regmap_set_bits(data->regmap, KX022A_REG_INC4,
+			      KX022A_MASK_WMI1);
+	if (ret)
+		goto unlock_out;
+
+	return kx022a_turn_on_unlock(data);
+
+unlock_out:
+	mutex_unlock(&data->mutex);
+
+	return ret;
+}
+
+static int kx022a_buffer_postenable(struct iio_dev *idev)
+{
+	struct kx022a_data *data = iio_priv(idev);
+
+	/*
+	 * If we use triggers, then the IRQs should be handled by trigger
+	 * enable and buffer is not used but we just add results to buffer
+	 * when data-ready triggers.
+	 */
+	if (iio_device_get_current_mode(idev) == INDIO_BUFFER_TRIGGERED)
+		return 0;
+
+	return kx022a_fifo_enable(data);
+}
+
+static const struct iio_buffer_setup_ops kx022a_buffer_ops = {
+	.postenable = kx022a_buffer_postenable,
+	.predisable = kx022a_buffer_predisable,
+};
+
+static irqreturn_t kx022a_trigger_handler(int irq, void *p)
+{
+	struct iio_poll_func *pf = p;
+	struct iio_dev *idev = pf->indio_dev;
+	struct kx022a_data *data = iio_priv(idev);
+	int ret;
+
+	mutex_lock(&data->mutex);
+	ret = regmap_bulk_read(data->regmap, KX022A_REG_XOUT_L, data->buffer,
+			       KX022A_FIFO_SAMPLES_SIZE_BYTES);
+	if (ret < 0)
+		goto err_read;
+
+	iio_push_to_buffers_with_timestamp(idev, data->buffer, pf->timestamp);
+err_read:
+	mutex_unlock(&data->mutex);
+	iio_trigger_notify_done(idev->trig);
+
+	return IRQ_HANDLED;
+}
+
+/* Get timestamps and wake the thread if we need to read data */
+static irqreturn_t kx022a_irq_handler(int irq, void *private)
+{
+	struct iio_dev *idev = private;
+	struct kx022a_data *data = iio_priv(idev);
+	bool ack = false;
+
+	data->old_timestamp = data->timestamp;
+	data->timestamp = iio_get_time_ns(idev);
+
+	if (data->trigger.enabled) {
+		iio_trigger_poll(data->trigger.indio_trig);
+		ack = true;
+	}
+
+	if (data->state & KX022A_STATE_FIFO)
+		return IRQ_WAKE_THREAD;
+
+	if (ack) {
+		/*
+		 * The IRQ is not acked until data is read. We need to disable
+		 * the IRQ in order to schedule the trigger thread. Enabling
+		 * is done in reenable.
+		 *
+		 * It would be possible to set the IRQ to 50uS pulse. If we are
+		 * losing data due to the disabled IRQ we can evaluate the
+		 * option of using edge triggered IRQs with the pulse mode.
+		 */
+		disable_irq_nosync(irq);
+		return IRQ_HANDLED;
+	}
+
+	return IRQ_NONE;
+}
+
+/* Read the data from the fifo and fill IIO buffers */
+static irqreturn_t kx022a_irq_thread_handler(int irq, void *private)
+{
+	struct iio_dev *idev = private;
+	struct kx022a_data *data = iio_priv(idev);
+	bool ack = false;
+	int ret;
+
+	mutex_lock(&data->mutex);
+
+	if (data->state & KX022A_STATE_FIFO) {
+		ret = __kx022a_fifo_flush(idev, KX022A_FIFO_LENGTH, true);
+		if (ret > 0)
+			ack = true;
+	}
+	/*
+	 * WMI and data-ready IRQs are acked when results are read. If we add
+	 * TILT/WAKE or other IRQs - then we may need to implement the acking
+	 * (which is racy).
+	 */
+	if (ack)
+		ret = IRQ_HANDLED;
+	else
+		ret = IRQ_NONE;
+
+	mutex_unlock(&data->mutex);
+
+	return ret;
+}
+
+static int kx022a_trigger_set_state(struct iio_trigger *trig,
+				    bool state)
+{
+	struct kx022a_trigger *t = iio_trigger_get_drvdata(trig);
+	struct kx022a_data *data = t->data;
+	int ret = 0;
+
+	mutex_lock(&data->mutex);
+
+	if (t->enabled == state)
+		goto unlock_out;
+
+	ret = __kx022a_turn_off_unlocked(data);
+	if (ret)
+		goto unlock_out;
+
+	t->enabled = state;
+	ret = kx022a_set_drdy_irq(data, state);
+	if (ret)
+		goto unlock_out;
+
+	ret = __kx022a_turn_on_unlocked(data);
+
+unlock_out:
+	mutex_unlock(&data->mutex);
+
+	return ret;
+}
+static void kx022a_trig_reen(struct iio_trigger *trig)
+{
+	struct kx022a_trigger *t = iio_trigger_get_drvdata(trig);
+	struct kx022a_data *data = t->data;
+
+	enable_irq(data->irq);
+}
+
+static const struct iio_trigger_ops kx022a_trigger_ops = {
+	.set_trigger_state = kx022a_trigger_set_state,
+	.reenable = kx022a_trig_reen,
+};
+
+static int kx022a_chip_init(struct kx022a_data *data)
+{
+	int ret, dummy;
+
+	/*
+	 * Disable IRQs because if the IRQs are left on (for example by
+	 * a shutdown which did not deactivate the accelerometer) we do
+	 * most probably end up flooding the system with unhandled IRQs
+	 * and get the line disabled from SOC side.
+	 */
+	ret = regmap_write(data->regmap, KX022A_REG_INC4, 0);
+	if (ret) {
+		dev_err(data->dev, "Failed to init IRQ states\n");
+		return ret;
+	}
+
+	ret = kx022a_set_drdy_irq(data, false);
+	if (ret) {
+		dev_err(data->dev, "Failed to init DRDY\n");
+		return ret;
+	}
+
+	/* Clear any pending IRQs */
+	ret = regmap_read(data->regmap, KX022A_REG_INT_REL, &dummy);
+	if (ret) {
+		dev_err(data->dev, "Failed to ACK IRQs\n");
+		return ret;
+	}
+	/* set data res 16bit */
+	ret = regmap_set_bits(data->regmap, KX022A_REG_BUF_CNTL2,
+			      KX022A_MASK_BRES);
+	if (ret) {
+		dev_err(data->dev, "Failed to set data resolution\n");
+		return ret;
+	}
+
+	ret = kx022a_prepare_irq_pin(data);
+	if (ret) {
+		dev_err(data->dev, "Failed to configure IRQ pin\n");
+		return ret;
+	}
+
+	/* disable buffer */
+	ret = regmap_clear_bits(data->regmap, KX022A_REG_BUF_CNTL2,
+			      KX022A_MASK_BUF_EN);
+	if (ret)
+		return ret;
+
+	return kx022a_drop_fifo_contents(data);
+}
+
+int kx022a_probe_internal(struct device *dev, int irq)
+{
+	static const char * const regulator_names[] = {"io_vdd", "vdd"};
+	struct iio_trigger *indio_trig;
+	struct kx022a_data *data;
+	struct regmap *regmap;
+	unsigned int chip_id;
+	struct iio_dev *idev;
+	int ret;
+
+	if (WARN_ON(!dev))
+		return -ENODEV;
+
+	regmap = dev_get_regmap(dev, NULL);
+	if (!regmap) {
+		dev_err(dev, "no regmap\n");
+
+		return -EINVAL;
+	}
+
+	idev = devm_iio_device_alloc(dev, sizeof(*data));
+	if (!idev)
+		return -ENOMEM;
+
+	data = iio_priv(idev);
+
+	/*
+	 * VDD is the analog and digital domain voltage supply
+	 * IO_VDD is the digital I/O voltage supply
+	 */
+	ret = devm_regulator_bulk_get_enable(dev, ARRAY_SIZE(regulator_names),
+					     regulator_names);
+	if (ret && ret != -ENODEV)
+		return dev_err_probe(dev, ret, "failed to enable regulator\n");
+
+	ret = regmap_read(regmap, KX022A_REG_WHO, &chip_id);
+	if (ret) {
+		dev_err(dev, "Failed to access sensor\n");
+		return ret;
+	}
+
+	if (chip_id != KX022A_ID) {
+		dev_err(dev, "unsupported device 0x%x\n", chip_id);
+		return -EINVAL;
+	}
+
+	data->regmap = regmap;
+	data->dev = dev;
+	data->irq = irq;
+	data->odr_ns = KX922A_DEFAULT_PERIOD;
+	mutex_init(&data->mutex);
+
+	idev->channels = kx022a_channels;
+	idev->num_channels = ARRAY_SIZE(kx022a_channels);
+	idev->name = "kx022-accel";
+	idev->info = &kx022a_info;
+	idev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_SOFTWARE;
+	idev->available_scan_masks = kx022a_scan_masks;
+
+	/* Read the mounting matrix, if present */
+	ret = iio_read_mount_matrix(dev, &data->orientation);
+	if (ret)
+		return ret;
+
+	/* The sensor must be turned off for configuration */
+	ret = kx022a_turn_off_lock(data);
+	if (ret)
+		return ret;
+
+	ret = kx022a_chip_init(data);
+	if (ret)
+		return ret;
+
+	ret = kx022a_turn_on_unlock(data);
+	if (ret)
+		return ret;
+
+	udelay(100);
+
+	ret = iio_triggered_buffer_setup_ext(idev,
+					     &iio_pollfunc_store_time,
+					     kx022a_trigger_handler,
+					     IIO_BUFFER_DIRECTION_IN,
+					     &kx022a_buffer_ops,
+					     kx022a_fifo_attributes);
+
+	if (ret)
+		return dev_err_probe(data->dev, ret,
+				     "iio_triggered_buffer_setup_ext FAIL %d\n",
+				     ret);
+
+	indio_trig = devm_iio_trigger_alloc(dev, "%sdata-rdy-dev%d", idev->name,
+					    iio_device_id(idev));
+	if (!indio_trig)
+		return -ENOMEM;
+
+	data->trigger.indio_trig = indio_trig;
+
+	indio_trig->ops = &kx022a_trigger_ops;
+	data->trigger.data = data;
+	iio_trigger_set_drvdata(indio_trig, &data->trigger);
+
+	ret = iio_trigger_register(indio_trig);
+	if (ret)
+		return dev_err_probe(data->dev, ret,
+				     "Trigger registration failed\n");
+
+	ret = devm_iio_device_register(data->dev, idev);
+	if (ret < 0)
+		return dev_err_probe(dev, ret,
+				     "Unable to register iio device\n");
+
+	ret = devm_request_threaded_irq(data->dev, irq, kx022a_irq_handler,
+					&kx022a_irq_thread_handler,
+					IRQF_ONESHOT, "kx022", idev);
+	if (ret)
+		return dev_err_probe(data->dev, ret, "Could not request IRQ\n");
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(kx022a_probe_internal);
+
+MODULE_DESCRIPTION("ROHM/Kionix KX022A accelerometer driver");
+MODULE_AUTHOR("Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>");
+MODULE_LICENSE("GPL");
diff --git a/drivers/iio/accel/kionix-kx022a.h b/drivers/iio/accel/kionix-kx022a.h
new file mode 100644
index 000000000000..ec89e46c6ca8
--- /dev/null
+++ b/drivers/iio/accel/kionix-kx022a.h
@@ -0,0 +1,76 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * Copyright (C) 2022 ROHM Semiconductors
+ *
+ * ROHM/KIONIX KX022A accelerometer driver
+ */
+
+#ifndef _KX022A_H_
+#define _KX022A_H_
+
+#include <linux/device.h>
+#include <linux/regmap.h>
+
+#define KX022A_REG_WHO		0x0f
+#define KX022A_ID		0xc8
+
+#define KX022A_REG_CNTL		0x18
+#define KX022A_MASK_PC1		BIT(7)
+#define KX022A_MASK_RES		BIT(6)
+#define KX022A_MASK_DRDY	BIT(5)
+#define KX022A_MASK_GSEL	GENMASK(4, 3)
+#define KX022A_GSEL_SHIFT	3
+#define KX022A_GSEL_2		0x0
+#define KX022A_GSEL_4		BIT(3)
+#define KX022A_GSEL_8		BIT(4)
+#define KX022A_GSEL_16		GENMASK(4, 3)
+
+#define KX022A_REG_INS2		0x13
+#define KX022A_MASK_INS2_DRDY	BIT(4)
+#define KX122_MASK_INS2_WMI	BIT(5)
+
+#define KX022A_REG_XHP_L	0x0
+#define KX022A_REG_XOUT_L	0x06
+#define KX022A_REG_YOUT_L	0x08
+#define KX022A_REG_ZOUT_L	0x0a
+#define KX022A_REG_COTR		0x0c
+#define KX022A_REG_TSCP		0x10
+#define KX022A_REG_INT_REL	0x17
+
+#define KX022A_REG_ODCNTL	0x1b
+
+#define KX022A_REG_BTS_WUF_TH	0x31
+#define KX022A_REG_MAN_WAKE	0x2c
+
+#define KX022A_REG_BUF_CNTL1	0x3a
+#define KX022A_MASK_WM_TH	GENMASK(6, 0)
+#define KX022A_REG_BUF_CNTL2	0x3b
+#define KX022A_MASK_BUF_EN	BIT(7)
+#define KX022A_MASK_BRES	BIT(6)
+#define KX022A_REG_BUF_STATUS_1	0x3c
+#define KX022A_REG_BUF_STATUS_2	0x3d
+#define KX022A_REG_BUF_CLEAR	0x3e
+#define KX022A_REG_BUF_READ	0x3f
+#define KX022A_MASK_ODR		GENMASK(3, 0)
+#define KX022A_ODR_SHIFT	3
+#define KX022A_FIFO_MAX_WMI_TH	41
+
+#define KX022A_REG_INC1		0x1c
+#define KX022A_MASK_IEN1	BIT(5)
+#define KX022A_MASK_IPOL1	BIT(4)
+#define KX022A_IPOL_LOW		0
+#define KX022A_IPOL_HIGH	KX022A_MASK_IPOL1
+#define KX022A_MASK_ITYP	BIT(3)
+#define KX022A_ITYP_PULSE	KX022A_MASK_ITYP
+#define KX022A_ITYP_LEVEL	0
+
+#define KX022A_REG_INC4		0x1f
+#define KX022A_MASK_WMI1	BIT(5)
+
+#define KX022A_REG_SELF_TEST	0x60
+#define KX022A_MAX_REGISTER	0x60
+
+int kx022a_probe_internal(struct device *dev, int irq);
+extern const struct regmap_config kx022a_regmap;
+
+#endif
-- 
2.37.1


-- 
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND

~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
Simon says - in Latin please.
~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
Thanks to Simon Glass for the translation =] 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC PATCH 4/5] iio: accel: Support Kionix/ROHM KX022A accelerometer

[Vaittinen, Matti]

On 9/21/22 14:45, Matti Vaittinen wrote:
> KX022A is a 3-axis Accelerometer from ROHM/Kionix. The senor features
> include variable ODRs, I2C and SPI control, FIFO/LIFO with watermark IRQ,
> tap/motion detection, wake-up & back-to-sleep events, four acceleration
> ranges (2, 4, 8 and 16g) and probably some other cool fatures.
> 
> Add support for the basic accelerometer features such as getting the
> acceleration data via IIO. (raw reads, triggered buffer [data-ready] or
> using the WMI IRQ).
> 
> Important things to be added include the double-tap, motion
> detection and wake-up as well as the runtime power management.
> 
> NOTE: Filling-up the hardware FIFO should be avoided. During my testing
> I noticed that filling up the hardware FIFO might mess-up the sample
> count. My sensor ended up in a state where amount of data in FIFO was
> reported to be 0xff bytes, which equals to 42,5 samples. Specification
> says the FIFO can hold maximum of 41 samples in HiRes mode. Also, at
> least once the FIFO was stuck in a state where reading data from
> hwardware FIFO did not decrease the amount of data reported to be in the
> FIFO - eg. FIFO was "stuck". The code has now an error count and 10
> reads with invalid FIFO data count will cause the fifo contents to be
> dropped.
> 
> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
> ---

//snip

> diff --git a/drivers/iio/accel/kionix-kx022a.c b/drivers/iio/accel/kionix-kx022a.c

//snip

> +static int kx022a_write_raw(struct iio_dev *idev,
> +			    struct iio_chan_spec const *chan,
> +			    int val, int val2, long mask)
> +{
> +	struct kx022a_data *data = iio_priv(idev);
> +	int ret;
> +
> +	/*
> +	 * We should not allow changing scale or frequency when FIFO is running
> +	 * as it will mess the timestamp/scale for samples existing in the
> +	 * buffer. If this turns out to be an issue we can later change logic
> +	 * to internally flush the fifo before reconfiguring so the samples in
> +	 * fifo keep matching the freq/scale settings. (Such setup could cause
> +	 * issues if users trust the watermark to be reached within known
> +	 * time-limit).
> +	 */
> +	mutex_lock(&data->mutex);
> +	if (iio_buffer_enabled(idev)) {
> +		ret = -EBUSY;
> +		goto unlock_out;
> +	}
> +	switch (mask) {
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		ret = kx022a_find_tuple_index(&kx022a_accel_samp_freq_table[0],
> +					      ARRAY_SIZE(kx022a_accel_samp_freq_table),
> +					      val, val2);
> +		/* Configure if we found valid ODR */
> +		if (ret >= 0) {
> +			int odr = ret;
> +
> +			ret = __kx022a_turn_off_unlocked(data);
> +			if (ret)
> +				goto unlock_out;
> +
> +			ret = regmap_update_bits(data->regmap, KX022A_REG_ODCNTL,
> +						 KX022A_MASK_ODR, odr);
> +			data->odr_ns = kx022a_odrs[odr];
> +			__kx022a_turn_on_unlocked(data);
> +		}
> +		break;
> +	case IIO_CHAN_INFO_SCALE:
> +		ret = kx022a_find_tuple_index(&kx022a_scale_table[0],
> +					      ARRAY_SIZE(kx022a_scale_table),
> +					      val, val2);
> +		/* Configure if we found valid scale */
> +		if (ret >= 0) {
> +			ret = __kx022a_turn_off_unlocked(data);
> +			if (ret)
> +				goto unlock_out;
> +
> +			ret = regmap_update_bits(data->regmap, KX022A_REG_CNTL,
> +						 KX022A_MASK_GSEL,
> +						 ret << KX022A_GSEL_SHIFT);
> +			__kx022a_turn_on_unlocked(data);
> +		}

I did a last minute change to the locking here. As a result I missed the 
break causing -EINVAL to be returned for scale setting. The build bot 
pinged me about it :) Will be fixed when I respin this!

> +	default:
> +		ret = -EINVAL;
> +	}
> +
> +unlock_out:
> +	mutex_unlock(&data->mutex);
> +
> +	return ret;
> +}
> +
Yours,
	--Matti

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

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

Re: [RFC PATCH 4/5] iio: accel: Support Kionix/ROHM KX022A accelerometer

[Jonathan Cameron]

On Wed, 21 Sep 2022 14:45:35 +0300
Matti Vaittinen <mazziesaccount@gmail.com> wrote:

> KX022A is a 3-axis Accelerometer from ROHM/Kionix. The senor features
> include variable ODRs, I2C and SPI control, FIFO/LIFO with watermark IRQ,
> tap/motion detection, wake-up & back-to-sleep events, four acceleration
> ranges (2, 4, 8 and 16g) and probably some other cool fatures.
> 
> Add support for the basic accelerometer features such as getting the
> acceleration data via IIO. (raw reads, triggered buffer [data-ready] or
> using the WMI IRQ).
> 
> Important things to be added include the double-tap, motion
> detection and wake-up as well as the runtime power management.
> 
> NOTE: Filling-up the hardware FIFO should be avoided. During my testing
> I noticed that filling up the hardware FIFO might mess-up the sample
> count. My sensor ended up in a state where amount of data in FIFO was
> reported to be 0xff bytes, which equals to 42,5 samples. Specification
> says the FIFO can hold maximum of 41 samples in HiRes mode. Also, at
> least once the FIFO was stuck in a state where reading data from
> hwardware FIFO did not decrease the amount of data reported to be in the
spell check this.

> FIFO - eg. FIFO was "stuck". The code has now an error count and 10
> reads with invalid FIFO data count will cause the fifo contents to be
> dropped.
Ouch - that's nasty.

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

Hi Matti,

A somewhat superficial review as I'm short on time, but wanted to get some
initial comments out to you as I'd started reviewing this yesterday and not
sure when I'd get back to it.

Jonathan

> ---
>  drivers/iio/accel/Kconfig             |   23 +
>  drivers/iio/accel/Makefile            |    3 +
>  drivers/iio/accel/kionix-kx022a-i2c.c |   52 ++
>  drivers/iio/accel/kionix-kx022a-spi.c |   50 ++
>  drivers/iio/accel/kionix-kx022a.c     | 1149 +++++++++++++++++++++++++
>  drivers/iio/accel/kionix-kx022a.h     |   76 ++
>  6 files changed, 1353 insertions(+)
>  create mode 100644 drivers/iio/accel/kionix-kx022a-i2c.c
>  create mode 100644 drivers/iio/accel/kionix-kx022a-spi.c
>  create mode 100644 drivers/iio/accel/kionix-kx022a.c
>  create mode 100644 drivers/iio/accel/kionix-kx022a.h
> 
> diff --git a/drivers/iio/accel/Kconfig b/drivers/iio/accel/Kconfig
> index 35798712f811..12ad6bcb2c76 100644
> --- a/drivers/iio/accel/Kconfig
> +++ b/drivers/iio/accel/Kconfig
> @@ -409,6 +409,29 @@ config IIO_ST_ACCEL_SPI_3AXIS
>  	  To compile this driver as a module, choose M here. The module
>  	  will be called st_accel_spi.
>  
> +config IIO_KX022A
> +	tristate
> +
> +config IIO_KX022A_SPI
> +	tristate "Kionix KX022A tri-axis digital accelerometer"
> +	depends on I2C
> +	select IIO_KX022A
> +	select REGMAP_SPI
> +	help
> +	  Say Y here to enable support for the Kionix KX022A digital tri-axis
> +	  accelerometer connected to I2C interface. See IIO_KX022A_I2C if
> +	  you want to enable support for the KX022A connected via I2C.

I would not bother with the cross reference to the I2C config element.

> +
> +config IIO_KX022A_I2C
> +	tristate "Kionix KX022A tri-axis digital accelerometer"
> +	depends on I2C
> +	select IIO_KX022A
> +	select REGMAP_I2C
> +	help
> +	  Say Y here to enable support for the Kionix KX022A digital tri-axis
> +	  accelerometer connected to I2C interface. See IIO_KX022A_SPI if
> +	  you want to enable support for the KX022A connected via SPI.
> +

> diff --git a/drivers/iio/accel/kionix-kx022a-i2c.c b/drivers/iio/accel/kionix-kx022a-i2c.c
> new file mode 100644
> index 000000000000..2dbf8a33b84b
> --- /dev/null
> +++ b/drivers/iio/accel/kionix-kx022a-i2c.c
> @@ -0,0 +1,52 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +//

Other than where required for SPDX use the
/*
 * Copyright...
 */
syntax.

> +// Copyright (C) 2022 ROHM Semiconductors
> +//
> +// ROHM/KIONIX KX022A accelerometer driver
> +
> +#include <linux/i2c.h>
> +#include <linux/interrupt.h>
> +#include <linux/module.h>
> +#include <linux/regmap.h>
> +
> +#include "kionix-kx022a.h"
> +
> +static int kx022a_i2c_probe(struct i2c_client *i2c)
> +{
> +	struct regmap *regmap;
> +	struct device *dev = &i2c->dev;
> +
> +	if (!i2c->irq) {

There look to be 2 interrupt lines, so you need to know which
one this is.  fwnode_irq_get_byname() trying first int1 then
int2 (as only a single one might be mapped).

> +		dev_err(dev, "No IRQ configured\n");
> +		return -EINVAL;
> +	}
> +
> +	regmap = devm_regmap_init_i2c(i2c, &kx022a_regmap);
> +	if (IS_ERR(regmap)) {
> +		dev_err(dev, "Failed to initialize Regmap\n");
> +
> +		return PTR_ERR(regmap);
> +	}
> +
> +	return kx022a_probe_internal(dev, i2c->irq);
> +}
> +
> +static const struct of_device_id kx022a_of_match[] = {
> +	{ .compatible = "kionix,kx022a", },
> +	{ },

No comma on null terminators like this.

> +};
> +MODULE_DEVICE_TABLE(of, kx022a_of_match);
> +
> +static struct i2c_driver kx022a_i2c_driver = {
> +	.driver = {
> +			.name  = "kx022a-i2c",
> +			.of_match_table = kx022a_of_match,
> +		  },
> +	.probe_new    = kx022a_i2c_probe,

I'd avoid tab alignment like this. It breaks far to often
and doesn't really help readability.

> +};
> +
> +module_i2c_driver(kx022a_i2c_driver);
> +
> +MODULE_DESCRIPTION("ROHM/Kionix KX022A accelerometer driver");
> +MODULE_AUTHOR("Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/iio/accel/kionix-kx022a-spi.c b/drivers/iio/accel/kionix-kx022a-spi.c
> new file mode 100644
> index 000000000000..007f4b726d8f
> --- /dev/null
> +++ b/drivers/iio/accel/kionix-kx022a-spi.c
> @@ -0,0 +1,50 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +//
> +// Copyright (C) 2022 ROHM Semiconductors
> +//
> +// ROHM/KIONIX KX022A accelerometer driver

Pretty much same comments as for the i2c part.

> +
> +#include <linux/interrupt.h>
> +#include <linux/module.h>
> +#include <linux/regmap.h>
> +#include <linux/spi/spi.h>
> +
> +#include "kionix-kx022a.h"
> +
> +static int kx022a_spi_probe(struct spi_device *spi)
> +{
> +	struct device *dev = &spi->dev;
> +	struct regmap *regmap;
> +
> +	if (!spi->irq) {
> +		dev_err(dev, "No IRQ configured\n");
> +		return -EINVAL;
> +	}
> +
> +	regmap = devm_regmap_init_spi(spi, &kx022a_regmap);
> +	if (IS_ERR(regmap)) {
> +		dev_err(dev, "Failed to initialize Regmap\n");
> +
> +		return PTR_ERR(regmap);
> +	}
> +	return kx022a_probe_internal(dev, spi->irq);
> +}
> +
> +static const struct of_device_id kx022a_of_match[] = {
> +	{ .compatible = "kionix,kx022a", },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, kx022a_of_match);
> +
> +static struct spi_driver kx022a_spi_driver = {
> +	.driver = {
> +		.name   = "kx022a-spi",
> +	},
> +	.probe	= kx022a_spi_probe,
> +};
> +
> +module_spi_driver(kx022a_spi_driver);
> +
> +MODULE_DESCRIPTION("ROHM/Kionix kx022A accelerometer driver");
> +MODULE_AUTHOR("Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/iio/accel/kionix-kx022a.c b/drivers/iio/accel/kionix-kx022a.c
> new file mode 100644
> index 000000000000..9f9e0d7efc09
> --- /dev/null
> +++ b/drivers/iio/accel/kionix-kx022a.c
> @@ -0,0 +1,1149 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +//
> +// Copyright (C) 2022 ROHM Semiconductors
> +//
> +// ROHM/KIONIX KX022A accelerometer driver
> +
> +#include <linux/delay.h>
> +#include <linux/gpio.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/iio/trigger.h>
> +#include <linux/iio/trigger_consumer.h>
> +#include <linux/iio/triggered_buffer.h>
> +#include <linux/interrupt.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/of.h>
> +#include <linux/of_gpio.h>
> +#include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/slab.h>
> +
> +#include "kionix-kx022a.h"
> +
> +/*
> + * Threshold for deciding our HW fifo is unrecoverably corrupt and should be
> + * cleared
> + */
> +#define KXO22A_FIFO_ERR_THRESHOLD 10
> +#define KX022A_FIFO_LENGTH 41
> +/* 3 axis, 2 bytes of data for each of the axis */
> +#define KX022A_FIFO_SAMPLES_SIZE_BYTES 6
> +#define KX022A_FIFO_MAX_BYTES (KX022A_FIFO_LENGTH *			\
> +			       KX022A_FIFO_SAMPLES_SIZE_BYTES)
> +
> +#define MIN_ODR_INTERVAL_MS 5
> +#define MAX_ODR_INTERVAL_MS 1280
> +#define NUM_SUPPORTED_ODR 9
> +
> +enum {
> +	KX022A_STATE_SAMPLE,
> +	KX022A_STATE_FIFO,
> +};
> +
> +/* Regmap configs */
> +static const struct regmap_range kx022a_volatile_ranges[] = {
> +	{
> +		.range_min = KX022A_REG_XHP_L,
> +		.range_max = KX022A_REG_COTR,
> +	}, {
> +		.range_min = KX022A_REG_TSCP,
> +		.range_max = KX022A_REG_INT_REL,
> +	}, {
> +		.range_min = KX022A_REG_BUF_STATUS_1,
> +		.range_max = KX022A_REG_BUF_READ,
> +	},
> +};
> +
> +static const struct regmap_access_table kx022a_volatile_regs = {
> +	.yes_ranges = &kx022a_volatile_ranges[0],
> +	.n_yes_ranges = ARRAY_SIZE(kx022a_volatile_ranges),
> +};
> +
> +static const struct regmap_range kx022a_precious_ranges[] = {
> +	{
> +		.range_min = KX022A_REG_INT_REL,
> +		.range_max = KX022A_REG_INT_REL,
> +	},
> +};
> +
> +static const struct regmap_access_table kx022a_precious_regs = {
> +	.yes_ranges = &kx022a_precious_ranges[0],
> +	.n_yes_ranges = ARRAY_SIZE(kx022a_precious_ranges),
> +};
> +
> +/*
> + * The HW does not set WHO_AM_I reg as read-only but we don't want to write it
> + * so we still include it in the read-only ranges.
> + */
> +static const struct regmap_range kx022a_read_only_ranges[] = {
> +	{
> +		.range_min = KX022A_REG_XHP_L,
> +		.range_max = KX022A_REG_INT_REL,
> +	}, {
> +		.range_min = KX022A_REG_BUF_STATUS_1,
> +		.range_max = KX022A_REG_BUF_STATUS_2,
> +	}, {
> +		.range_min = KX022A_REG_BUF_READ,
> +		.range_max = KX022A_REG_BUF_READ,
> +	},
> +};
> +
> +static const struct regmap_access_table kx022a_ro_regs = {
> +	.no_ranges = &kx022a_read_only_ranges[0],
> +	.n_no_ranges = ARRAY_SIZE(kx022a_read_only_ranges),
> +};
> +
> +static const struct regmap_range kx022a_write_only_ranges[] = {
> +	{
> +		.range_min = KX022A_REG_BTS_WUF_TH,
> +		.range_max = KX022A_REG_BTS_WUF_TH,
> +	}, {
> +		.range_min = KX022A_REG_MAN_WAKE,
> +		.range_max = KX022A_REG_MAN_WAKE,
> +	}, {
> +		.range_min = KX022A_REG_SELF_TEST,
> +		.range_max = KX022A_REG_SELF_TEST,
> +	}, {
> +		.range_min = KX022A_REG_BUF_CLEAR,
> +		.range_max = KX022A_REG_BUF_CLEAR,
> +	},
> +};
> +
> +static const struct regmap_access_table kx022a_wo_regs = {
> +	.no_ranges = &kx022a_write_only_ranges[0],
> +	.n_no_ranges = ARRAY_SIZE(kx022a_write_only_ranges),
> +};
> +
> +static const struct regmap_range kx022a_noinc_read_ranges[] = {
> +	{
> +		.range_min = KX022A_REG_BUF_READ,
> +		.range_max = KX022A_REG_BUF_READ,
> +	},
> +};
> +
> +static const struct regmap_access_table kx022a_nir_regs = {
> +	.yes_ranges = &kx022a_noinc_read_ranges[0],
> +	.n_yes_ranges = ARRAY_SIZE(kx022a_noinc_read_ranges),
> +};
> +
> +const struct regmap_config kx022a_regmap = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +	.volatile_table = &kx022a_volatile_regs,
> +	.rd_table = &kx022a_wo_regs,
> +	.wr_table = &kx022a_ro_regs,
> +	.rd_noinc_table = &kx022a_nir_regs,
> +	.precious_table = &kx022a_precious_regs,
> +	.max_register = KX022A_MAX_REGISTER,
> +	.cache_type = REGCACHE_RBTREE,
> +};
> +EXPORT_SYMBOL_GPL(kx022a_regmap);
> +
> +struct kx022a_data;
> +
> +struct kx022a_trigger {

Unusual structure. IIRC we normally just squash this stuff into
the iio_priv structure and set the trigger devdata to the iio_dev.
I haven't really thought about the advantages of this way around
but can see a disadvantage is you get circular pointer loops
this way.

> +	struct kx022a_data *data;

Looks like you can get to the data pointer via a container_of()
so do that instead of carrying a pointer to the containing structure.

> +	struct iio_trigger *indio_trig;
> +	bool enabled;
> +	const char *name;
> +};
> +
> +struct kx022a_data {
> +	int irq;
> +	struct regmap *regmap;
> +	struct kx022a_trigger trigger;
> +	struct device *dev;
> +	unsigned int g_range;
> +	struct mutex mutex;
> +	unsigned int state;
> +
> +	int64_t timestamp, old_timestamp;
> +	unsigned int odr_ns;
> +
> +	struct iio_mount_matrix orientation;
> +	u8 watermark;
> +	/* 3 x 16bit accel data + timestamp */
> +	s16 buffer[8];
> +	struct {
> +		__le16 channels[3];
> +		s64 ts __aligned(8);
> +	} scan;
> +};
> +
> +static const struct iio_mount_matrix *
> +kx022a_get_mount_matrix(const struct iio_dev *idev,
> +			const struct iio_chan_spec *chan)
> +{
> +	struct kx022a_data *data = iio_priv(idev);
> +
> +	return &data->orientation;
> +}
> +
> +enum {
> +	AXIS_X,
> +	AXIS_Y,
> +	AXIS_Z,
> +	AXIS_MAX,
> +};
> +
> +static const unsigned long kx022a_scan_masks[] = {
> +					BIT(AXIS_X) | BIT(AXIS_Y) | BIT(AXIS_Z),
> +					0};
> +
> +static const struct iio_chan_spec_ext_info kx022a_ext_info[] = {
> +	IIO_MOUNT_MATRIX(IIO_SHARED_BY_TYPE, kx022a_get_mount_matrix),
> +	{ },
> +};
> +
> +#define KX022A_ACCEL_CHAN(axis, index)						\
> +	{								\
> +		.type = IIO_ACCEL,					\
> +		.modified = 1,						\
> +		.channel2 = IIO_MOD_##axis,				\
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),		\
> +		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) |	\
> +					BIT(IIO_CHAN_INFO_SAMP_FREQ),	\
> +		.ext_info = kx022a_ext_info,				\
> +		.address = KX022A_REG_##axis##OUT_L,				\
> +		.scan_index = index,					\
> +		.scan_type = {                                          \
> +			.sign = 's',					\
> +			.realbits = 16,					\
> +			.storagebits = 16,				\
> +			.shift = 0,					\

shift = 0 is 'obvious' default, so don't bother providing it explicitly.


> +			.endianness = IIO_LE,				\
> +		},							\
> +	}
> +
> +static const struct iio_chan_spec kx022a_channels[] = {
> +	KX022A_ACCEL_CHAN(X, 0),
> +	KX022A_ACCEL_CHAN(Y, 1),
> +	KX022A_ACCEL_CHAN(Z, 2),
> +	IIO_CHAN_SOFT_TIMESTAMP(3),
> +};
> +
> +/*
> + * The sensor HW can support ODR up to 1600 Hz - which is beyond what most of
> + * Linux CPUs can handle w/o dropping samples. Also, the low power mode is not
> + * available for higher sample rates. Thus the driver only supports 200 Hz and
> + * slower ODRs. Slowest being 0.78 Hz
> + */
> +static IIO_CONST_ATTR_SAMP_FREQ_AVAIL("0.78 1.563 3.125 6.25 12.5 25 50 100 200");
> +static IIO_CONST_ATTR(scale_available,
> +		      "598.550415 1197.10083 2394.20166 4788.40332");
> +
> +static struct attribute *kx022a_attributes[] = {
> +	&iio_const_attr_sampling_frequency_available.dev_attr.attr,
> +	&iio_const_attr_scale_available.dev_attr.attr,

Use the read_avail() callback instead of doing these as attributes.
That makes the values available to consumer drivers...

> +	NULL,
> +};
> +
> +static const struct attribute_group kx022a_attrs_group = {
> +	.attrs = kx022a_attributes,
> +};
> +
> +struct kx022a_tuple {
> +	int val;
> +	int val2;
> +};
> +
> +/*
> + * range is typically +-2G/4G/8G/16G, distributed over the amount of bits.
> + * The scale table can be calculated using
> + *	(range / 2^bits) * g = (range / 2^bits) * 9.80665 m/s^2
> + *	=> KX022A uses 16 bit (HiRes mode - assume the low 8 bits are zeroed
> + *	in low-power mode(?) )
> + *	=> +/-2G  => 4 / 2^16 * 9,80665 * 10^6 (to scale to micro)
> + *	=> +/-2G  - 598.550415
> + *	   +/-4G  - 1197.10083
> + *	   +/-8G  - 2394.20166
> + *	   +/-16G - 4788.40332
> + */
> +static const struct kx022a_tuple kx022a_scale_table[] = {
> +	{ 598, 550415 }, { 1197, 100830 }, { 2394, 201660 }, { 4788, 403320 }

Burn some lines with one pair per line as it is more readable.

> +};
> +
> +/*
> + * ODR (output data rate) table. First value represents the integer portion of
> + * frequency (Hz), and the second value is the decimal part (uHz).
> + * Eg, 0.78 Hz, 1.563 Hz, ...
> + */
> +#define KX922A_DEFAULT_PERIOD 20000000
> +static const struct kx022a_tuple kx022a_accel_samp_freq_table[] = {
> +	{ 0, 780000 }, { 1, 563000 }, { 3, 125000 }, { 6, 250000 },
> +	{ 12, 500000 }, { 25, 0 }, { 50, 0 }, { 100, 0 }, { 200, 0 }
> +};

Spend the lines and have one entry pair per line as more readable.

> +static const unsigned int kx022a_odrs[] = { 1282051282, 639795266, 320000000,
> +	 160000000, 80000000, 40000000, 20000000, 10000000, 5000000 };

where we have lots of zeros useful to use the MEGA etc defines in units.h


> +
> +/* Find index of tuple matching the given values */
> +static int kx022a_find_tuple_index(const struct kx022a_tuple *tuples, int n,
> +				   int val, int val2)
> +{
> +	while (n-- > 0)
> +		if (val == tuples[n].val && tuples[n].val2 == val2)
> +			return n;
> +
> +	return -EINVAL;
> +}
> +
> +static void kx022a_reg2freq(unsigned int val,  int *val1, int *val2)
> +{
> +	*val1 = kx022a_accel_samp_freq_table[val & KX022A_MASK_ODR].val;
> +	*val2 = kx022a_accel_samp_freq_table[val & KX022A_MASK_ODR].val2;
> +}
> +
> +static void kx022a_reg2scale(unsigned int val, unsigned int *val1,
> +			     unsigned int *val2)
> +{
> +	val &= KX022A_MASK_GSEL;
> +	val >>= KX022A_GSEL_SHIFT;
> +
> +	*val1 = kx022a_scale_table[val].val;
> +	*val2 = kx022a_scale_table[val].val2;
> +}
> +
> +static int __kx022a_turn_on_unlocked(struct kx022a_data *data)
> +{
> +	int ret;
> +
> +	ret = regmap_set_bits(data->regmap, KX022A_REG_CNTL, KX022A_MASK_PC1);
> +	if (ret)
> +		dev_err(data->dev, "Turn ON fail %d\n", ret);
> +
> +	return ret;
> +}
> +
> +static int __kx022a_turn_off_unlocked(struct kx022a_data *data)
> +{
> +	int ret;
> +
> +	ret = regmap_clear_bits(data->regmap, KX022A_REG_CNTL, KX022A_MASK_PC1);
> +	if (ret)
> +		dev_err(data->dev, "Turn OFF fail %d\n", ret);
> +
> +	return ret;
> +}
> +
> +static int kx022a_turn_off_lock(struct kx022a_data *data)
> +{
> +	int ret;

As below, I'm not convinced this wrapper is worthwhile

> +
> +	mutex_lock(&data->mutex);
> +	ret = __kx022a_turn_off_unlocked(data);
> +	if (ret)
> +		mutex_unlock(&data->mutex);
> +
> +	return ret;
> +}
> +
> +static int kx022a_turn_on_unlock(struct kx022a_data *data)
> +{
> +	int ret;
> +
This is not used enough that I can see a strong reason for the
wrapper.  Just put the two calls inline and rename the unlocked case.

> +	ret = __kx022a_turn_on_unlocked(data);
> +	mutex_unlock(&data->mutex);
> +
> +	return ret;
> +}
> +
> +static int kx022a_write_raw(struct iio_dev *idev,
> +			    struct iio_chan_spec const *chan,
> +			    int val, int val2, long mask)
> +{
> +	struct kx022a_data *data = iio_priv(idev);
> +	int ret;
> +
> +	/*
> +	 * We should not allow changing scale or frequency when FIFO is running
> +	 * as it will mess the timestamp/scale for samples existing in the
> +	 * buffer. If this turns out to be an issue we can later change logic
> +	 * to internally flush the fifo before reconfiguring so the samples in
> +	 * fifo keep matching the freq/scale settings. (Such setup could cause
> +	 * issues if users trust the watermark to be reached within known
> +	 * time-limit).
> +	 */
> +	mutex_lock(&data->mutex);
> +	if (iio_buffer_enabled(idev)) {

As below - do this with iio_device_claim_direct_mode() before taking the mutex.
It's the standard way to avoid the races and prevent access when buffers are enabled
(if they are - you can't claim direct mode).

> +		ret = -EBUSY;
> +		goto unlock_out;
> +	}
> +	switch (mask) {
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		ret = kx022a_find_tuple_index(&kx022a_accel_samp_freq_table[0],
> +					      ARRAY_SIZE(kx022a_accel_samp_freq_table),
> +					      val, val2);
> +		/* Configure if we found valid ODR */
> +		if (ret >= 0) {
> +			int odr = ret;
> +
> +			ret = __kx022a_turn_off_unlocked(data);
> +			if (ret)
> +				goto unlock_out;
> +
> +			ret = regmap_update_bits(data->regmap, KX022A_REG_ODCNTL,
> +						 KX022A_MASK_ODR, odr);
> +			data->odr_ns = kx022a_odrs[odr];
> +			__kx022a_turn_on_unlocked(data);
> +		}
> +		break;
> +	case IIO_CHAN_INFO_SCALE:
> +		ret = kx022a_find_tuple_index(&kx022a_scale_table[0],
> +					      ARRAY_SIZE(kx022a_scale_table),
> +					      val, val2);
> +		/* Configure if we found valid scale */
> +		if (ret >= 0) {
> +			ret = __kx022a_turn_off_unlocked(data);
> +			if (ret)
> +				goto unlock_out;
> +
> +			ret = regmap_update_bits(data->regmap, KX022A_REG_CNTL,
> +						 KX022A_MASK_GSEL,
> +						 ret << KX022A_GSEL_SHIFT);
> +			__kx022a_turn_on_unlocked(data);
> +		}
> +	default:
> +		ret = -EINVAL;
> +	}
> +
> +unlock_out:
> +	mutex_unlock(&data->mutex);
> +
> +	return ret;
> +}
> +
> +static int kx022a_fifo_set_wmi(struct kx022a_data *data)
> +{
> +	u8 threshold;
> +
> +	threshold = data->watermark;
> +
> +	return regmap_update_bits(data->regmap, KX022A_REG_BUF_CNTL1,
> +				  KX022A_MASK_WM_TH, threshold);
> +}
> +
> +static int kx022a_fifo_report_data(struct kx022a_data *data, void *buffer,
> +				   int samples)
> +{

I would just put this code inline at the single call site.

> +	int ret, fifo_bytes;
> +
> +	fifo_bytes = samples * KX022A_FIFO_SAMPLES_SIZE_BYTES;
> +	ret = regmap_noinc_read(data->regmap, KX022A_REG_BUF_READ,
> +			       buffer, fifo_bytes);
> +	if (ret)
> +		dev_err(data->dev, "FIFO read failed %d\n", ret);
> +
> +	return ret;
> +}
> +
> +static int kx022a_get_axis(struct kx022a_data *data,
> +			   struct iio_chan_spec const *chan,
> +			   int *val)
> +{
> +	u16 raw_val;
> +	int ret;
> +
> +	ret = regmap_bulk_read(data->regmap, chan->address, &raw_val,
> +			       sizeof(raw_val));

Bulk reads for SPI still require dma safe buffers. Long story but
The short version is today regmap happens to always use bounce buffers
for SPI (or it did last time I looked at it). However there is no
guarantee on that in future.  As such you need to use a buffer
that is  __aligned(IIO_DMA_MINALIGN);
Easiest option is to put one at the end of the iio_priv() structure.

> +	if (ret)
> +		return ret;
> +	*val = raw_val;
> +
> +	return IIO_VAL_INT;
> +}
> +
> +static int kx022a_read_raw(struct iio_dev *idev,
> +			   struct iio_chan_spec const *chan,
> +			   int *val, int *val2, long mask)
> +{
> +	struct kx022a_data *data = iio_priv(idev);
> +	unsigned int regval;
> +	int ret = -EINVAL;
> +
> +	mutex_lock(&data->mutex);
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		if (iio_buffer_enabled(idev)) {

Claim direct mode as standard way to avoid racing in read_raw.
Note you should be very careful on locking order as well.
Claim direct mode first then take any other locks.


> +			ret = -EBUSY;
> +			goto error_ret;
> +		}
> +
> +		ret = kx022a_get_axis(data, chan, val);
> +		break;
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +	{

Brackets don't add anything here as not defining scope of anything.

> +		ret = regmap_read(data->regmap, KX022A_REG_ODCNTL, &regval);
> +		if (ret)
> +			goto error_ret;
> +
> +		if ((regval & KX022A_MASK_ODR) >
> +		    ARRAY_SIZE(kx022a_accel_samp_freq_table)) {
> +			dev_err(data->dev, "Invalid ODR\n");
> +			ret = -EINVAL;
> +			goto error_ret;
> +		}
> +
> +		kx022a_reg2freq(regval, val, val2);
> +		ret = IIO_VAL_INT_PLUS_MICRO;
> +
> +		break;
> +	}
> +	case IIO_CHAN_INFO_SCALE:
> +		ret = regmap_read(data->regmap, KX022A_REG_CNTL, &regval);
> +		if (ret < 0)
> +			goto error_ret;
> +
> +		kx022a_reg2scale(regval, val, val2);
> +
> +		ret = IIO_VAL_INT_PLUS_MICRO;
> +		break;
> +	}
> +
> +error_ret:
> +	mutex_unlock(&data->mutex);
> +
> +	return ret;
> +};
> +
> +static int kx022a_validate_trigger(struct iio_dev *idev,
> +				   struct iio_trigger *trig)
> +{
> +	struct kx022a_data *data = iio_priv(idev);
> +
> +	if (data->trigger.indio_trig == trig)
> +		return 0;
> +
> +	return -EINVAL;
> +}
> +
> +static int kx022a_set_watermark(struct iio_dev *idev, unsigned int val)
> +{
> +	struct kx022a_data *data = iio_priv(idev);
> +
> +	if (val > KX022A_FIFO_LENGTH)
> +		val = KX022A_FIFO_LENGTH;
> +
> +	mutex_lock(&data->mutex);
> +	data->watermark = val;
> +	mutex_unlock(&data->mutex);
> +
> +	return 0;
> +}
> +
> +static ssize_t kx022a_get_fifo_state(struct device *dev,
> +				     struct device_attribute *attr,
> +				     char *buf)
> +{
> +	struct iio_dev *idev = dev_to_iio_dev(dev);
> +	struct kx022a_data *data = iio_priv(idev);
> +	bool state;
> +
> +	mutex_lock(&data->mutex);
> +	state = data->state;
> +	mutex_unlock(&data->mutex);
> +
> +	return sprintf(buf, "%d\n", state);
> +}
> +
> +static ssize_t kx022a_get_fifo_watermark(struct device *dev,
> +					 struct device_attribute *attr,
> +					 char *buf)
> +{
> +	struct iio_dev *idev = dev_to_iio_dev(dev);
> +	struct kx022a_data *data = iio_priv(idev);
> +	int wm;
> +
> +	mutex_lock(&data->mutex);
> +	wm = data->watermark;
> +	mutex_unlock(&data->mutex);
> +
> +	return sprintf(buf, "%d\n", wm);
> +}
> +
> +static IIO_DEVICE_ATTR(hwfifo_enabled, 0444,
> +		       kx022a_get_fifo_state, NULL, 0);
> +static IIO_DEVICE_ATTR(hwfifo_watermark, 0444,
> +		       kx022a_get_fifo_watermark, NULL, 0);
> +
> +static const struct attribute *kx022a_fifo_attributes[] = {
> +	&iio_dev_attr_hwfifo_watermark.dev_attr.attr,
> +	&iio_dev_attr_hwfifo_enabled.dev_attr.attr,
> +	NULL,
> +};
> +
> +static int kx022a_drop_fifo_contents(struct kx022a_data *data)
> +{
> +	/*
> +	 * We must clear the old time-stamp to avoid computing the timestamps
> +	 * based on samples acquired when buffer was last enabled
> +	 */
> +	data->timestamp = 0;
> +
> +	return regmap_write(data->regmap, KX022A_REG_BUF_CLEAR, 0xff);
> +}
> +
> +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);
> +	int ret, i;
> +	int count, fifo_bytes;
> +	u16 buffer[KX022A_FIFO_LENGTH * 3];
> +	int64_t tstamp;
> +	uint64_t sample_period;
> +	static int err_ctr;
> +
> +	ret = regmap_read(data->regmap, KX022A_REG_BUF_STATUS_1, &fifo_bytes);
> +	if (ret < 0) {
> +		dev_err(dev, "Error reading buffer status\n");
> +		return ret;
> +	}
> +
> +	/* Let's not owerflow if we for some reason get bogus value from i2c */

overflow. Make sure to spell check.

> +	if (fifo_bytes > KX022A_FIFO_MAX_BYTES) {
> +		/*
> +		 * I've observed a strange behaviour where FIFO may get stuck if
> +		 * samples are not read out fast enough. By 'stuck' I mean
> +		 * situation where amount of data adverticed by the STATUS_1
> +		 * reg is 255 - which equals to 42,5 (sic!) samples and by
> +		 * my experimenting there are situations where reading the
> +		 * FIFO buffer does not decrease the data count but the same
> +		 * fifo sample level (255 bytes of data) is reported
> +		 */
> +		err_ctr++;
> +		dev_warn(data->dev, "Bad amount of data %u\n", fifo_bytes);
> +		fifo_bytes = KX022A_FIFO_MAX_BYTES;
> +	} else if (fifo_bytes % KX022A_FIFO_SAMPLES_SIZE_BYTES) {
> +		err_ctr++;
> +		dev_err(data->dev, "Bad FIFO alignment. Data may be corrupt\n");
> +	} else {
> +		err_ctr = 0;
> +	}
> +
> +	if (err_ctr > KXO22A_FIFO_ERR_THRESHOLD) {
> +		__kx022a_turn_off_unlocked(data);
> +		kx022a_drop_fifo_contents(data);
> +		__kx022a_turn_on_unlocked(data);
> +
> +		err_ctr = 0;
> +
> +		return -EINVAL;
> +	}
> +
> +	count = fifo_bytes / KX022A_FIFO_SAMPLES_SIZE_BYTES;
> +	if (!count)
> +		return 0;
> +
> +	/*
> +	 * If we are being called from IRQ handler we know the stored timestamp
> +	 * is fairly accurate for the last stored sample. Otherwise, if we are
> +	 * called as a result of a read operation from userspace and hence
> +	 * before the watermark interrupt was triggered, take a timestamp
> +	 * now. We can fall anywhere in between two samples so the error in this
> +	 * case is at most one sample period.
> +	 */
> +	if (!irq) {
> +		data->old_timestamp = data->timestamp;
> +		data->timestamp = iio_get_time_ns(idev);
> +	}
> +
> +	/*
> +	 * Approximate timestamps for each of the sample based on the sampling
> +	 * frequency, timestamp for last sample and number of samples.
> +	 *
> +	 * We'd better not use the current bandwidth settings to compute the
> +	 * sample period. The real sample rate varies with the device and
> +	 * small variation adds when we store a large number of samples.
> +	 *
> +	 * To avoid this issue we compute the actual sample period ourselves
> +	 * based on the timestamp delta between the last two flush operations.
> +	 */
> +	if (data->old_timestamp) {
> +		sample_period = (data->timestamp - data->old_timestamp);
> +		do_div(sample_period, count);
> +	} else {
> +		sample_period = data->odr_ns;
> +	}
> +	tstamp = data->timestamp - (count - 1) * sample_period;
> +
> +	if (samples && count > samples) {
> +		/*
> +		 * Here we leave some old samples to the buffer. We need to
> +		 * adjust the timestamp to match the first sample in the buffer
> +		 * or we will miscalculate the sample_period at next round.
> +		 */
> +		data->timestamp -= (count - samples) * sample_period;
> +		count = samples;
> +	}
> +
> +	ret = kx022a_fifo_report_data(data, buffer, count);
> +	if (ret)
> +		return ret;
> +
> +	for (i = 0; i < count; i++) {
> +		int bit;
> +		u16 *samples = &buffer[i * 3];
> +
> +		for_each_set_bit(bit, idev->active_scan_mask, AXIS_MAX)
> +			memcpy(&data->scan.channels[bit], &samples[bit],
> +			       sizeof(data->scan.channels[0]));
> +
> +		iio_push_to_buffers_with_timestamp(idev, &data->scan, tstamp);
> +
> +		tstamp += sample_period;
> +	}
> +
> +	return count;
> +}
> +
> +static int kx022a_fifo_flush(struct iio_dev *idev, unsigned int samples)
> +{
> +	struct kx022a_data *data = iio_priv(idev);
> +	int ret;
> +
> +	mutex_lock(&data->mutex);
> +	ret = __kx022a_fifo_flush(idev, samples, false);
> +	mutex_unlock(&data->mutex);
> +
> +	return ret;
> +}
> +
> +static const struct iio_info kx022a_info = {
> +	.read_raw = &kx022a_read_raw,
> +	.write_raw = &kx022a_write_raw,
> +	.attrs = &kx022a_attrs_group,
> +
> +	.validate_trigger	= kx022a_validate_trigger,
> +	.hwfifo_set_watermark	= kx022a_set_watermark,
> +	.hwfifo_flush_to_buffer	= kx022a_fifo_flush,
> +};
> +
> +static int kx022a_set_drdy_irq(struct kx022a_data *data, bool en)
> +{
> +	if (en)
> +		return regmap_set_bits(data->regmap, KX022A_REG_CNTL,
> +				       KX022A_MASK_DRDY);
> +
> +	return regmap_clear_bits(data->regmap, KX022A_REG_CNTL,
> +				 KX022A_MASK_DRDY);
> +}
> +
> +static int kx022a_prepare_irq_pin(struct kx022a_data *data)
> +{
> +	/* Enable IRQ1 pin. Set polarity to active low */

Must either handle both pins or at least know if it is irq2 and
treat that as no irq for now.

> +	int mask = KX022A_MASK_IEN1 | KX022A_MASK_IPOL1 |
> +		   KX022A_MASK_ITYP;
> +	int val = KX022A_MASK_IEN1 | KX022A_IPOL_LOW |
> +		  KX022A_ITYP_LEVEL;
> +	int ret;
> +
> +	ret = regmap_update_bits(data->regmap, KX022A_REG_INC1, mask, val);
> +	if (ret)
> +		return ret;
> +
> +	mask = KX022A_MASK_INS2_DRDY | KX122_MASK_INS2_WMI;
> +
> +	return regmap_set_bits(data->regmap, KX022A_REG_INC4, mask);
> +}
> +
> +static int kx022a_fifo_disable(struct kx022a_data *data)
> +{
> +	int ret = 0;
> +
> +	/* PC1 to 0 */
?  No idea what that means...

> +	ret = kx022a_turn_off_lock(data);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_clear_bits(data->regmap, KX022A_REG_INC4,
> +				KX022A_MASK_WMI1);
> +	if (ret)
> +		goto unlock_out;
> +
> +	/* disable buffer */
> +	ret = regmap_clear_bits(data->regmap, KX022A_REG_BUF_CNTL2,
> +				KX022A_MASK_BUF_EN);
> +	if (ret)
> +		goto unlock_out;
> +
> +	data->state &= (~KX022A_STATE_FIFO);
> +
> +	kx022a_drop_fifo_contents(data);
> +
> +	return kx022a_turn_on_unlock(data);
> +
> +unlock_out:
> +	mutex_unlock(&data->mutex);
> +
> +	return ret;
> +}
> +
> +static int kx022a_buffer_predisable(struct iio_dev *idev)
> +{
> +	struct kx022a_data *data = iio_priv(idev);
> +
> +	if (iio_device_get_current_mode(idev) == INDIO_BUFFER_TRIGGERED)
> +		return 0;
> +
> +	return kx022a_fifo_disable(data);
> +}
> +
> +static int kx022a_fifo_enable(struct kx022a_data *data)
> +{
> +	int ret = 0;
> +
> +	ret = kx022a_turn_off_lock(data);
> +	if (ret)
> +		return ret;
> +
> +	/* Write WMI to HW */

Where WMI is?

> +	ret = kx022a_fifo_set_wmi(data);
> +	if (ret)
> +		goto unlock_out;
> +
> +	/* Enable buffer */
> +	ret = regmap_set_bits(data->regmap, KX022A_REG_BUF_CNTL2,
> +			      KX022A_MASK_BUF_EN);
> +	if (ret)
> +		goto unlock_out;
> +
> +	data->state |= KX022A_STATE_FIFO;
> +	ret = regmap_set_bits(data->regmap, KX022A_REG_INC4,
> +			      KX022A_MASK_WMI1);
> +	if (ret)
> +		goto unlock_out;
> +
> +	return kx022a_turn_on_unlock(data);
> +
> +unlock_out:
> +	mutex_unlock(&data->mutex);
> +
> +	return ret;
> +}
> +
> +static int kx022a_buffer_postenable(struct iio_dev *idev)
> +{
> +	struct kx022a_data *data = iio_priv(idev);
> +
> +	/*
> +	 * If we use triggers, then the IRQs should be handled by trigger
> +	 * enable and buffer is not used but we just add results to buffer
> +	 * when data-ready triggers.

That comment needs rewriting given you have two buffers and I think they
are very different things..

> +	 */
> +	if (iio_device_get_current_mode(idev) == INDIO_BUFFER_TRIGGERED)
> +		return 0;
> +
> +	return kx022a_fifo_enable(data);
> +}
> +
> +static const struct iio_buffer_setup_ops kx022a_buffer_ops = {
> +	.postenable = kx022a_buffer_postenable,
> +	.predisable = kx022a_buffer_predisable,
> +};
> +
> +static irqreturn_t kx022a_trigger_handler(int irq, void *p)
> +{
> +	struct iio_poll_func *pf = p;
> +	struct iio_dev *idev = pf->indio_dev;
> +	struct kx022a_data *data = iio_priv(idev);
> +	int ret;
> +
> +	mutex_lock(&data->mutex);
> +	ret = regmap_bulk_read(data->regmap, KX022A_REG_XOUT_L, data->buffer,
> +			       KX022A_FIFO_SAMPLES_SIZE_BYTES);
> +	if (ret < 0)
> +		goto err_read;
> +
> +	iio_push_to_buffers_with_timestamp(idev, data->buffer, pf->timestamp);
> +err_read:
> +	mutex_unlock(&data->mutex);
> +	iio_trigger_notify_done(idev->trig);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +/* Get timestamps and wake the thread if we need to read data */
> +static irqreturn_t kx022a_irq_handler(int irq, void *private)
> +{
> +	struct iio_dev *idev = private;
> +	struct kx022a_data *data = iio_priv(idev);
> +	bool ack = false;
> +
> +	data->old_timestamp = data->timestamp;
> +	data->timestamp = iio_get_time_ns(idev);
> +
> +	if (data->trigger.enabled) {
> +		iio_trigger_poll(data->trigger.indio_trig);
> +		ack = true;
> +	}
> +
> +	if (data->state & KX022A_STATE_FIFO)
> +		return IRQ_WAKE_THREAD;
> +
> +	if (ack) {
> +		/*
> +		 * The IRQ is not acked until data is read. We need to disable
> +		 * the IRQ in order to schedule the trigger thread. Enabling
> +		 * is done in reenable.

I'm a little confused at what is going on here, but a common solution to situations
like this is to handling to use iio_trigger_poll_chained() from the thread and
a IRQF_ONESHOT irq.  Thus the interrupt will remain masked until the trigger
consumers are all done.

> +		 *
> +		 * It would be possible to set the IRQ to 50uS pulse. If we are
> +		 * losing data due to the disabled IRQ we can evaluate the
> +		 * option of using edge triggered IRQs with the pulse mode.
> +		 */
> +		disable_irq_nosync(irq);
> +		return IRQ_HANDLED;
> +	}
> +
> +	return IRQ_NONE;
> +}
> +
> +/* Read the data from the fifo and fill IIO buffers */
> +static irqreturn_t kx022a_irq_thread_handler(int irq, void *private)
> +{
> +	struct iio_dev *idev = private;
> +	struct kx022a_data *data = iio_priv(idev);
> +	bool ack = false;
> +	int ret;
> +
> +	mutex_lock(&data->mutex);
> +
> +	if (data->state & KX022A_STATE_FIFO) {
> +		ret = __kx022a_fifo_flush(idev, KX022A_FIFO_LENGTH, true);
> +		if (ret > 0)
> +			ack = true;
> +	}
> +	/*
> +	 * WMI and data-ready IRQs are acked when results are read. If we add
> +	 * TILT/WAKE or other IRQs - then we may need to implement the acking
> +	 * (which is racy).

I'm not even going to ask for more info at this stage but that sound worrying.

> +	 */
> +	if (ack)
> +		ret = IRQ_HANDLED;
> +	else
> +		ret = IRQ_NONE;
> +
> +	mutex_unlock(&data->mutex);
> +
> +	return ret;
> +}
> +
> +static int kx022a_trigger_set_state(struct iio_trigger *trig,
> +				    bool state)
> +{
> +	struct kx022a_trigger *t = iio_trigger_get_drvdata(trig);
> +	struct kx022a_data *data = t->data;
> +	int ret = 0;
> +
> +	mutex_lock(&data->mutex);
> +
> +	if (t->enabled == state)
> +		goto unlock_out;
> +
> +	ret = __kx022a_turn_off_unlocked(data);
> +	if (ret)
> +		goto unlock_out;
> +
> +	t->enabled = state;
> +	ret = kx022a_set_drdy_irq(data, state);
> +	if (ret)
> +		goto unlock_out;
> +
> +	ret = __kx022a_turn_on_unlocked(data);
> +
> +unlock_out:
> +	mutex_unlock(&data->mutex);
> +
> +	return ret;
> +}

blank line between functions.

> +static void kx022a_trig_reen(struct iio_trigger *trig)
> +{
> +	struct kx022a_trigger *t = iio_trigger_get_drvdata(trig);

Mentioned above, but I really don't like the pointer loops between
the trigger and main _data structure.

> +	struct kx022a_data *data = t->data;
> +
> +	enable_irq(data->irq);
> +}
> +
> +static const struct iio_trigger_ops kx022a_trigger_ops = {
> +	.set_trigger_state = kx022a_trigger_set_state,
> +	.reenable = kx022a_trig_reen,
> +};
> +
> +static int kx022a_chip_init(struct kx022a_data *data)
> +{
> +	int ret, dummy;
> +
> +	/*
> +	 * Disable IRQs because if the IRQs are left on (for example by
> +	 * a shutdown which did not deactivate the accelerometer) we do
> +	 * most probably end up flooding the system with unhandled IRQs
> +	 * and get the line disabled from SOC side.
> +	 */
> +	ret = regmap_write(data->regmap, KX022A_REG_INC4, 0);

Unusual to do this rather than a reset.  Quick look suggests there is
a suitable software reset (CNTL2)

> +	if (ret) {
> +		dev_err(data->dev, "Failed to init IRQ states\n");
> +		return ret;
> +	}
> +
> +	ret = kx022a_set_drdy_irq(data, false);
> +	if (ret) {
> +		dev_err(data->dev, "Failed to init DRDY\n");
> +		return ret;
> +	}
> +
> +	/* Clear any pending IRQs */
> +	ret = regmap_read(data->regmap, KX022A_REG_INT_REL, &dummy);
> +	if (ret) {
> +		dev_err(data->dev, "Failed to ACK IRQs\n");
> +		return ret;
> +	}
> +	/* set data res 16bit */
> +	ret = regmap_set_bits(data->regmap, KX022A_REG_BUF_CNTL2,
> +			      KX022A_MASK_BRES);

Can we have a _BRES_16 to make this self documenting?

> +	if (ret) {
> +		dev_err(data->dev, "Failed to set data resolution\n");
> +		return ret;
> +	}
> +
> +	ret = kx022a_prepare_irq_pin(data);
> +	if (ret) {
> +		dev_err(data->dev, "Failed to configure IRQ pin\n");
> +		return ret;
> +	}
> +
> +	/* disable buffer */

consistent capitalisation of comments...

> +	ret = regmap_clear_bits(data->regmap, KX022A_REG_BUF_CNTL2,
> +			      KX022A_MASK_BUF_EN);
> +	if (ret)
> +		return ret;
> +
> +	return kx022a_drop_fifo_contents(data);
> +}
> +
> +int kx022a_probe_internal(struct device *dev, int irq)
> +{
> +	static const char * const regulator_names[] = {"io_vdd", "vdd"};
> +	struct iio_trigger *indio_trig;
> +	struct kx022a_data *data;
> +	struct regmap *regmap;
> +	unsigned int chip_id;
> +	struct iio_dev *idev;
> +	int ret;
> +
> +	if (WARN_ON(!dev))
> +		return -ENODEV;
> +
> +	regmap = dev_get_regmap(dev, NULL);
> +	if (!regmap) {
> +		dev_err(dev, "no regmap\n");

Use dev_err_probe() for all dev_err() stuff in probe paths.
It ends up cleaner and we don't care about the tiny overhead
of checking for deferred.

> +
> +		return -EINVAL;
> +	}
> +
> +	idev = devm_iio_device_alloc(dev, sizeof(*data));
> +	if (!idev)
> +		return -ENOMEM;
> +
> +	data = iio_priv(idev);
> +
> +	/*
> +	 * VDD is the analog and digital domain voltage supply
> +	 * IO_VDD is the digital I/O voltage supply
> +	 */
> +	ret = devm_regulator_bulk_get_enable(dev, ARRAY_SIZE(regulator_names),
> +					     regulator_names);
> +	if (ret && ret != -ENODEV)
> +		return dev_err_probe(dev, ret, "failed to enable regulator\n");
> +
> +	ret = regmap_read(regmap, KX022A_REG_WHO, &chip_id);
> +	if (ret) {
> +		dev_err(dev, "Failed to access sensor\n");
> +		return ret;
> +	}
> +
> +	if (chip_id != KX022A_ID) {
> +		dev_err(dev, "unsupported device 0x%x\n", chip_id);
> +		return -EINVAL;
> +	}
> +
> +	data->regmap = regmap;
> +	data->dev = dev;
> +	data->irq = irq;
> +	data->odr_ns = KX922A_DEFAULT_PERIOD;
> +	mutex_init(&data->mutex);
> +
> +	idev->channels = kx022a_channels;
> +	idev->num_channels = ARRAY_SIZE(kx022a_channels);
> +	idev->name = "kx022-accel";
> +	idev->info = &kx022a_info;
> +	idev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_SOFTWARE;
> +	idev->available_scan_masks = kx022a_scan_masks;
> +
> +	/* Read the mounting matrix, if present */
> +	ret = iio_read_mount_matrix(dev, &data->orientation);
> +	if (ret)
> +		return ret;
> +
> +	/* The sensor must be turned off for configuration */
> +	ret = kx022a_turn_off_lock(data);
> +	if (ret)
> +		return ret;
> +
> +	ret = kx022a_chip_init(data);
> +	if (ret)
> +		return ret;
> +
> +	ret = kx022a_turn_on_unlock(data);
> +	if (ret)
> +		return ret;
> +
> +	udelay(100);
> +
> +	ret = iio_triggered_buffer_setup_ext(idev,

devm_

> +					     &iio_pollfunc_store_time,
> +					     kx022a_trigger_handler,
> +					     IIO_BUFFER_DIRECTION_IN,
> +					     &kx022a_buffer_ops,
> +					     kx022a_fifo_attributes);
> +
> +	if (ret)
> +		return dev_err_probe(data->dev, ret,
> +				     "iio_triggered_buffer_setup_ext FAIL %d\n",
> +				     ret);
> +
> +	indio_trig = devm_iio_trigger_alloc(dev, "%sdata-rdy-dev%d", idev->name,
> +					    iio_device_id(idev));
> +	if (!indio_trig)
> +		return -ENOMEM;
> +
> +	data->trigger.indio_trig = indio_trig;
> +
> +	indio_trig->ops = &kx022a_trigger_ops;
> +	data->trigger.data = data;
> +	iio_trigger_set_drvdata(indio_trig, &data->trigger);
> +
> +	ret = iio_trigger_register(indio_trig);

Don't mix devm and non devm allocation / registration.

> +	if (ret)
> +		return dev_err_probe(data->dev, ret,
> +				     "Trigger registration failed\n");
> +
> +	ret = devm_iio_device_register(data->dev, idev);
> +	if (ret < 0)
> +		return dev_err_probe(dev, ret,
> +				     "Unable to register iio device\n");
> +
> +	ret = devm_request_threaded_irq(data->dev, irq, kx022a_irq_handler,
> +					&kx022a_irq_thread_handler,
> +					IRQF_ONESHOT, "kx022", idev);
> +	if (ret)
> +		return dev_err_probe(data->dev, ret, "Could not request IRQ\n");
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(kx022a_probe_internal);
> +
> +MODULE_DESCRIPTION("ROHM/Kionix KX022A accelerometer driver");
> +MODULE_AUTHOR("Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/iio/accel/kionix-kx022a.h b/drivers/iio/accel/kionix-kx022a.h
> new file mode 100644
> index 000000000000..ec89e46c6ca8
> --- /dev/null
> +++ b/drivers/iio/accel/kionix-kx022a.h
> @@ -0,0 +1,76 @@

...

> +#define KX022A_REG_INC4		0x1f
> +#define KX022A_MASK_WMI1	BIT(5)
> +
> +#define KX022A_REG_SELF_TEST	0x60
> +#define KX022A_MAX_REGISTER	0x60
> +

Use a forward definition for struct device
rather than including the header.

> +int kx022a_probe_internal(struct device *dev, int irq);
> +extern const struct regmap_config kx022a_regmap;
> +
> +#endif

Re: [RFC PATCH 4/5] iio: accel: Support Kionix/ROHM KX022A accelerometer

[Matti Vaittinen]

On 9/22/22 20:03, Jonathan Cameron wrote:
> On Wed, 21 Sep 2022 14:45:35 +0300
> Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> 
>> KX022A is a 3-axis Accelerometer from ROHM/Kionix. The senor features
>> include variable ODRs, I2C and SPI control, FIFO/LIFO with watermark IRQ,
>> tap/motion detection, wake-up & back-to-sleep events, four acceleration
>> ranges (2, 4, 8 and 16g) and probably some other cool fatures.
>>
>> Add support for the basic accelerometer features such as getting the
>> acceleration data via IIO. (raw reads, triggered buffer [data-ready] or
>> using the WMI IRQ).
>>
>> Important things to be added include the double-tap, motion
>> detection and wake-up as well as the runtime power management.
>>
>> NOTE: Filling-up the hardware FIFO should be avoided. During my testing
>> I noticed that filling up the hardware FIFO might mess-up the sample
>> count. My sensor ended up in a state where amount of data in FIFO was
>> reported to be 0xff bytes, which equals to 42,5 samples. Specification
>> says the FIFO can hold maximum of 41 samples in HiRes mode. Also, at
>> least once the FIFO was stuck in a state where reading data from
>> hwardware FIFO did not decrease the amount of data reported to be in the
> spell check this.
> 
>> FIFO - eg. FIFO was "stuck". The code has now an error count and 10
>> reads with invalid FIFO data count will cause the fifo contents to be
>> dropped.
> Ouch - that's nasty.

Indeed it is. As this commit states, this is pretty initial support for 
the accelerometer. I want to enable people to do basic experimenting and 
also use the component to some slow ODR solutions. Besides, having even 
a basic support in-tree enable people to add further improvements :) So, 
I am hoping / expecting to see improvements added also by other people 
using this. I think that after this initial support many people still 
_need_ for example the runtime PM. Maybe we will also end up with some 
nicer solution to the FIFO issues.

Oh, please note that the patch is no longer complete. I've "snipped" 
unrelated pieces when replying to Jonathan. I did also just silently 
ignore the comments which I simply agree with.

>>
>> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
> 
> Hi Matti,
> 
> A somewhat superficial review as I'm short on time, but wanted to get some
> initial comments out to you as I'd started reviewing this yesterday and not
> sure when I'd get back to it.

No problem Jonathan. I appreciate the feedback - and even if the review 
has not been completed the initial feedback will already allow me to 
rework :) I won't go anywhere so you can for sure send me more comments 
during the coming new iterations.

Besides, I appreciate your engagement even more as some maintainers do 
not have the time to look at the RFC patches at all. Getting even 
initial feedback this quickly is much more than I expected! So, thanks!

>> +config IIO_KX022A_SPI
>> +	tristate "Kionix KX022A tri-axis digital accelerometer"
>> +	depends on I2C
>> +	select IIO_KX022A
>> +	select REGMAP_SPI
>> +	help
>> +	  Say Y here to enable support for the Kionix KX022A digital tri-axis
>> +	  accelerometer connected to I2C interface. See IIO_KX022A_I2C if
>> +	  you want to enable support for the KX022A connected via I2C.
> 
> I would not bother with the cross reference to the I2C config element.

I thought it might help avoid some confusion - but can be dropped.


>> +// SPDX-License-Identifier: GPL-2.0-only
>> +//
> 
> Other than where required for SPDX use the
> /*
>   * Copyright...
>   */
> syntax.

No problem. Some maintainers prefer having the top of the file comment 
block looking coherent and it also pleases my eye more. Still, can be 
changed to suit the IIO-style.

> 
>> +// Copyright (C) 2022 ROHM Semiconductors
>> +//
>> +// ROHM/KIONIX KX022A accelerometer driver
>> +

>> +static int kx022a_i2c_probe(struct i2c_client *i2c)
>> +{
>> +	struct regmap *regmap;
>> +	struct device *dev = &i2c->dev;
>> +
>> +	if (!i2c->irq) {
> 
> There look to be 2 interrupt lines, so you need to know which
> one this is.  fwnode_irq_get_byname() trying first int1 then
> int2 (as only a single one might be mapped).

Seems like you know the Kionix component(s). Cool! My idea is initially 
to only support a fixed configuration with IRQ connected to the IRQ1 
pin. The support for the second IRQ can be added when needed. I think 
it's better to have at first something working and add features 
iteratively when needed. I do also believe that people who add support 
to IRQ2-pin should also be testing this. I have currently only IRQ1 
connected.

Would a comment explaining this do?

You made me think that this should probably be explained in the 
dt-bindings already. I need to add the irq-names and mention that if the 
irq-names is not populated it is assumed (only) the IRQ1 is used.

>> +static struct i2c_driver kx022a_i2c_driver = {
>> +	.driver = {
>> +			.name  = "kx022a-i2c",
>> +			.of_match_table = kx022a_of_match,
>> +		  },
>> +	.probe_new    = kx022a_i2c_probe,
> 
> I'd avoid tab alignment like this. It breaks far to often
> and doesn't really help readability.

Yep. I already fixed this in my local tree yesterday :) Will carry the 
fix to the v2 :)

>> +
>> +struct kx022a_data;
>> +
>> +struct kx022a_trigger {
> 
> Unusual structure. IIRC we normally just squash this stuff into
> the iio_priv structure and set the trigger devdata to the iio_dev.
> I haven't really thought about the advantages of this way around
> but can see a disadvantage is you get circular pointer loops
> this way.

I'd better admit I didn't invent this myself. I stole big parts of the 
logic from the bmc150-accel-core.c which supports multiple triggers. (By 
the way, I should probably metion that in the commit message). All 
triggers are in an array - so if you only get the trigger pointer to one 
of the triggers in the array, then calculating the start of the 
containing struct becomes non trivial.

This does not apply to this driver now as we only support the data-ready 
trigger so I can change this. OTOH, reading the list of features KX022A 
hardware provides - I wouldn't be surprized to see a new trigger(s) in 
the future. It'd be nice to get support for different modes like for 
example capturing N events that were stored prior certain movement. 
These might be best implemented with triggers. What is your adviece - is 
it worth trying to keep adding new trigger(s) as a low-hanging fruit - 
or is it just better to make the current (limited) implementation as 
simple as possible?

>> +	struct kx022a_data *data;
> 
> Looks like you can get to the data pointer via a container_of()
> so do that instead of carrying a pointer to the containing structure.

True. As long as I have just one trigger... (and yes, we have just one 
right now).

>> +static IIO_CONST_ATTR_SAMP_FREQ_AVAIL("0.78 1.563 3.125 6.25 12.5 25 50 100 200");
>> +static IIO_CONST_ATTR(scale_available,
>> +		      "598.550415 1197.10083 2394.20166 4788.40332");
>> +
>> +static struct attribute *kx022a_attributes[] = {
>> +	&iio_const_attr_sampling_frequency_available.dev_attr.attr,
>> +	&iio_const_attr_scale_available.dev_attr.attr,
> 
> Use the read_avail() callback instead of doing these as attributes.
> That makes the values available to consumer drivers...

Thanks for the guidance. My first IIO driver => all help much appreciated :)

>> +static const unsigned int kx022a_odrs[] = { 1282051282, 639795266, 320000000,
>> +	 160000000, 80000000, 40000000, 20000000, 10000000, 5000000 };
> 
> where we have lots of zeros useful to use the MEGA etc defines in units.h

One of the best things in upstream collaboration is always learning new 
(to me) things :) I'll check then out. Thanks!

>> +
>> +static int kx022a_turn_off_lock(struct kx022a_data *data)
>> +{
>> +	int ret;
> 
> As below, I'm not convinced this wrapper is worthwhile
> 
>> +
>> +	mutex_lock(&data->mutex);
>> +	ret = __kx022a_turn_off_unlocked(data);
>> +	if (ret)
>> +		mutex_unlock(&data->mutex);
>> +
>> +	return ret;
>> +}
>> +
>> +static int kx022a_turn_on_unlock(struct kx022a_data *data)
>> +{
>> +	int ret;
>> +
> This is not used enough that I can see a strong reason for the
> wrapper.  Just put the two calls inline and rename the unlocked case.
> 

I kind of see the value of the wrappers as it documents the purpose of 
locking. It's kind of a wake-up that we really want to keep the access 
locked during sensor 'shut-down'. I don't know how other sensors work 
but for me the Kionix sensors requiring the sensor to be "turned off" 
for the duration of pretty much _any_ configurations was a surprize... 
Again, I think this will be required for _all_ new configuration code we 
add in the driver.

>> +	ret = __kx022a_turn_on_unlocked(data);
>> +	mutex_unlock(&data->mutex);
>> +
>> +	return ret;
>> +}
>> +
>> +	/*
>> +	 * We should not allow changing scale or frequency when FIFO is running
>> +	 * as it will mess the timestamp/scale for samples existing in the
>> +	 * buffer. If this turns out to be an issue we can later change logic
>> +	 * to internally flush the fifo before reconfiguring so the samples in
>> +	 * fifo keep matching the freq/scale settings. (Such setup could cause
>> +	 * issues if users trust the watermark to be reached within known
>> +	 * time-limit).
>> +	 */
>> +	mutex_lock(&data->mutex);
>> +	if (iio_buffer_enabled(idev)) {
> 
> As below - do this with iio_device_claim_direct_mode() before taking the mutex.
> It's the standard way to avoid the races and prevent access when buffers are enabled
> (if they are - you can't claim direct mode).

Thanks! I'll see the iio_device_claim_direct_mode() - again, appreciate 
your advices!

>> +
>> +static int kx022a_fifo_report_data(struct kx022a_data *data, void *buffer,
>> +				   int samples)
>> +{
> 
> I would just put this code inline at the single call site.

Eg, meld this in the fifo flushing function? Ok.

> 
>> +	int ret, fifo_bytes;
>> +
>> +	fifo_bytes = samples * KX022A_FIFO_SAMPLES_SIZE_BYTES;
>> +	ret = regmap_noinc_read(data->regmap, KX022A_REG_BUF_READ,
>> +			       buffer, fifo_bytes);
>> +	if (ret)
>> +		dev_err(data->dev, "FIFO read failed %d\n", ret);
>> +
>> +	return ret;
>> +}
>> +
>> +static int kx022a_get_axis(struct kx022a_data *data,
>> +			   struct iio_chan_spec const *chan,
>> +			   int *val)
>> +{
>> +	u16 raw_val;
>> +	int ret;
>> +
>> +	ret = regmap_bulk_read(data->regmap, chan->address, &raw_val,
>> +			       sizeof(raw_val));
> 
> Bulk reads for SPI still require dma safe buffers. Long story but
> The short version is today regmap happens to always use bounce buffers
> for SPI (or it did last time I looked at it). However there is no
> guarantee on that in future.  As such you need to use a buffer
> that is  __aligned(IIO_DMA_MINALIGN);
> Easiest option is to put one at the end of the iio_priv() structure.
> 

Thanks. That's a bummer. I would prefer buffer from the stack which 
would be a simple way of avoiding races. OTOH, the access is serialized 
so there should be no problem with your suggestion of using buffer from 
iio_priv. I'll fix this.

>> +	if (ret)
>> +		return ret;
>> +	*val = raw_val;
>> +
>> +	return IIO_VAL_INT;
>> +}
>> +
>> +static int kx022a_read_raw(struct iio_dev *idev,
>> +			   struct iio_chan_spec const *chan,
>> +			   int *val, int *val2, long mask)
>> +{
>> +	struct kx022a_data *data = iio_priv(idev);
>> +	unsigned int regval;
>> +	int ret = -EINVAL;
>> +
>> +	mutex_lock(&data->mutex);
>> +	switch (mask) {
>> +	case IIO_CHAN_INFO_RAW:
>> +		if (iio_buffer_enabled(idev)) {
> 
> Claim direct mode as standard way to avoid racing in read_raw.
> Note you should be very careful on locking order as well.
> Claim direct mode first then take any other locks.
> 

Thanks again.

>> +	if (fifo_bytes > KX022A_FIFO_MAX_BYTES) {
>> +		/*
>> +		 * I've observed a strange behaviour where FIFO may get stuck if
>> +		 * samples are not read out fast enough. By 'stuck' I mean
>> +		 * situation where amount of data adverticed by the STATUS_1
>> +		 * reg is 255 - which equals to 42,5 (sic!) samples and by
>> +		 * my experimenting there are situations where reading the
>> +		 * FIFO buffer does not decrease the data count but the same
>> +		 * fifo sample level (255 bytes of data) is reported
>> +		 */
>> +		err_ctr++;
>> +		dev_warn(data->dev, "Bad amount of data %u\n", fifo_bytes);
>> +		fifo_bytes = KX022A_FIFO_MAX_BYTES;
>> +	} else if (fifo_bytes % KX022A_FIFO_SAMPLES_SIZE_BYTES) {
>> +		err_ctr++;
>> +		dev_err(data->dev, "Bad FIFO alignment. Data may be corrupt\n");
>> +	} else {
>> +		err_ctr = 0;
>> +	}
>> +
>> +	if (err_ctr > KXO22A_FIFO_ERR_THRESHOLD) {
>> +		__kx022a_turn_off_unlocked(data);
>> +		kx022a_drop_fifo_contents(data);
>> +		__kx022a_turn_on_unlocked(data);
>> +
>> +		err_ctr = 0;
>> +
>> +		return -EINVAL;
>> +	}
>> +
>> +	count = fifo_bytes / KX022A_FIFO_SAMPLES_SIZE_BYTES;
>> +	if (!count)
>> +		return 0;
>> +
>> +	/*
>> +	 * If we are being called from IRQ handler we know the stored timestamp
>> +	 * is fairly accurate for the last stored sample. Otherwise, if we are
>> +	 * called as a result of a read operation from userspace and hence
>> +	 * before the watermark interrupt was triggered, take a timestamp
>> +	 * now. We can fall anywhere in between two samples so the error in this
>> +	 * case is at most one sample period.
>> +	 */
>> +	if (!irq) {
>> +		data->old_timestamp = data->timestamp;
>> +		data->timestamp = iio_get_time_ns(idev);
>> +	}
>> +
>> +	/*
>> +	 * Approximate timestamps for each of the sample based on the sampling
>> +	 * frequency, timestamp for last sample and number of samples.
>> +	 *
>> +	 * We'd better not use the current bandwidth settings to compute the
>> +	 * sample period. The real sample rate varies with the device and
>> +	 * small variation adds when we store a large number of samples.
>> +	 *
>> +	 * To avoid this issue we compute the actual sample period ourselves
>> +	 * based on the timestamp delta between the last two flush operations.
>> +	 */
>> +	if (data->old_timestamp) {
>> +		sample_period = (data->timestamp - data->old_timestamp);
>> +		do_div(sample_period, count);
>> +	} else {
>> +		sample_period = data->odr_ns;
>> +	}
>> +	tstamp = data->timestamp - (count - 1) * sample_period;
>> +
>> +	if (samples && count > samples) {
>> +		/*
>> +		 * Here we leave some old samples to the buffer. We need to
>> +		 * adjust the timestamp to match the first sample in the buffer
>> +		 * or we will miscalculate the sample_period at next round.
>> +		 */ >> +		data->timestamp -= (count - samples) * sample_period;
>> +		count = samples;

Just a note not directly related to this driver. I borrowed (stoled) the 
timestamp logic from the bmc150-accel-core.c. The bmc150-core did not 
have this timestamp adjustment which I think is necessary in case the 
fifo is not read empty. Someone familiar with the bmc150-accel-core.c 
might want to check this.

>> +	}
>> +
>> +	ret = kx022a_fifo_report_data(data, buffer, count);
>> +	if (ret)
>> +		return ret;
>> +
>> +	for (i = 0; i < count; i++) {
>> +		int bit;
>> +		u16 *samples = &buffer[i * 3];
>> +
>> +		for_each_set_bit(bit, idev->active_scan_mask, AXIS_MAX)
>> +			memcpy(&data->scan.channels[bit], &samples[bit],
>> +			       sizeof(data->scan.channels[0]));
>> +
>> +		iio_push_to_buffers_with_timestamp(idev, &data->scan, tstamp);
>> +
>> +		tstamp += sample_period;
>> +	}
>> +
>> +	return count;
>> +}
>> +
>> +static int kx022a_prepare_irq_pin(struct kx022a_data *data)
>> +{
>> +	/* Enable IRQ1 pin. Set polarity to active low */
> 
> Must either handle both pins or at least know if it is irq2 and
> treat that as no irq for now.

I don't want to try supporting both pins for now. It makes this somewhat 
more complex - especially if we want to support using two IRQs. That 
will require some thorough thinking which I don't have time to do right 
now :(

I can add the irq-names to the bindings and add check to the probe so 
that if the irq2 is used we error out with nice 'not supported' message.

> 
>> +	int mask = KX022A_MASK_IEN1 | KX022A_MASK_IPOL1 |
>> +		   KX022A_MASK_ITYP;
>> +	int val = KX022A_MASK_IEN1 | KX022A_IPOL_LOW |
>> +		  KX022A_ITYP_LEVEL;
>> +	int ret;
>> +
>> +	ret = regmap_update_bits(data->regmap, KX022A_REG_INC1, mask, val);
>> +	if (ret)
>> +		return ret;
>> +
>> +	mask = KX022A_MASK_INS2_DRDY | KX122_MASK_INS2_WMI;
>> +
>> +	return regmap_set_bits(data->regmap, KX022A_REG_INC4, mask);
>> +}
>> +
>> +static int kx022a_fifo_disable(struct kx022a_data *data)
>> +{
>> +	int ret = 0;
>> +
>> +	/* PC1 to 0 */
> ?  No idea what that means...

Sorry about confusing print. The sensor state control bit is named as 
PC1 in the data-sheet and almost every single register table refers to 
the "PC1 bit" because pretty much nothing can be configured when sensor 
is running. I still agree this comment is terrible for anyone not 
reading the data-sheet. Thanks for pointing it out - I'll either drop it 
or make it more obvious...

> 
>> +	ret = kx022a_turn_off_lock(data);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = regmap_clear_bits(data->regmap, KX022A_REG_INC4,
>> +				KX022A_MASK_WMI1);
>> +	if (ret)
>> +		goto unlock_out;
>> +
>> +	/* disable buffer */
>> +	ret = regmap_clear_bits(data->regmap, KX022A_REG_BUF_CNTL2,
>> +				KX022A_MASK_BUF_EN);
>> +	if (ret)
>> +		goto unlock_out;
>> +
>> +	data->state &= (~KX022A_STATE_FIFO);
>> +
>> +	kx022a_drop_fifo_contents(data);
>> +
>> +	return kx022a_turn_on_unlock(data);
>> +
>> +unlock_out:
>> +	mutex_unlock(&data->mutex);
>> +
>> +	return ret;
>> +}
>> +

>> +static int kx022a_fifo_enable(struct kx022a_data *data)
>> +{
>> +	int ret = 0;
>> +
>> +	ret = kx022a_turn_off_lock(data);
>> +	if (ret)
>> +		return ret;
>> +
>> +	/* Write WMI to HW */
> 
> Where WMI is?

Stored in the iio_priv. data->watermark. The kx022a_set_watermark() only 
updates the value in iio_priv() and we take it in use only when the 
buffer is enabled.


>> +
>> +static int kx022a_buffer_postenable(struct iio_dev *idev)
>> +{
>> +	struct kx022a_data *data = iio_priv(idev);
>> +
>> +	/*
>> +	 * If we use triggers, then the IRQs should be handled by trigger
>> +	 * enable and buffer is not used but we just add results to buffer
>> +	 * when data-ready triggers.
> 
> That comment needs rewriting given you have two buffers and I think they
> are very different things..

True. Sometimes it just requires someone else to read your comments to 
really see the obfuscations like this one. Thanks for pointing it out.

>> +
>> +/* Get timestamps and wake the thread if we need to read data */
>> +static irqreturn_t kx022a_irq_handler(int irq, void *private)
>> +{
>> +	struct iio_dev *idev = private;
>> +	struct kx022a_data *data = iio_priv(idev);
>> +	bool ack = false;
>> +
>> +	data->old_timestamp = data->timestamp;
>> +	data->timestamp = iio_get_time_ns(idev);
>> +
>> +	if (data->trigger.enabled) {
>> +		iio_trigger_poll(data->trigger.indio_trig);
>> +		ack = true;
>> +	}
>> +
>> +	if (data->state & KX022A_STATE_FIFO)
>> +		return IRQ_WAKE_THREAD;
>> +
>> +	if (ack) {
>> +		/*
>> +		 * The IRQ is not acked until data is read. We need to disable
>> +		 * the IRQ in order to schedule the trigger thread. Enabling
>> +		 * is done in reenable.
> 
> I'm a little confused at what is going on here, but a common solution to situations
> like this is to handling to use iio_trigger_poll_chained() from the thread and
> a IRQF_ONESHOT irq.  Thus the interrupt will remain masked until the trigger
> consumers are all done.

Oh, right. This is again probably because of my lack of experience with 
IIO. I did just use the iio_trigger_poll() from the handler - which in 
turn ends up to separate thread for handling. Eg, handing was not done 
in the context of the irq thread => IRQF_ONESHOT did not keep irq 
disabled for the duration of the thread scheduled by the 
iio_trigger_poll(). Thus I disabled the IRQ so that the thread scheduled 
by iio_trigger_poll() was able to run and read the sample.

I will revise this with the iio_trigger_poll_chained() - hopefully I can 
make this nicer with it! Thanks

> 
>> +		 *
>> +		 * It would be possible to set the IRQ to 50uS pulse. If we are
>> +		 * losing data due to the disabled IRQ we can evaluate the
>> +		 * option of using edge triggered IRQs with the pulse mode.
>> +		 */
>> +		disable_irq_nosync(irq);
>> +		return IRQ_HANDLED;
>> +	}
>> +
>> +	return IRQ_NONE;
>> +}
>> +
>> +/* Read the data from the fifo and fill IIO buffers */
>> +static irqreturn_t kx022a_irq_thread_handler(int irq, void *private)
>> +{
>> +	struct iio_dev *idev = private;
>> +	struct kx022a_data *data = iio_priv(idev);
>> +	bool ack = false;
>> +	int ret;
>> +
>> +	mutex_lock(&data->mutex);
>> +
>> +	if (data->state & KX022A_STATE_FIFO) {
>> +		ret = __kx022a_fifo_flush(idev, KX022A_FIFO_LENGTH, true);
>> +		if (ret > 0)
>> +			ack = true;
>> +	}
>> +	/*
>> +	 * WMI and data-ready IRQs are acked when results are read. If we add
>> +	 * TILT/WAKE or other IRQs - then we may need to implement the acking
>> +	 * (which is racy).
> 
> I'm not even going to ask for more info at this stage but that sound worrying.

Yep, you didn't ask but I can still babble a bit :) It's just about the 
fact that this (and many other) Kionix sensors clear all IRQs when a 
single bit of a single register is written. This naturally is racy 
because there will always be a time-window for new IRQs to trigger 
between reading the active IRQs and writing the ack. So with this acking 
mechanism we do potentially lose IRQs.

It might be there were separate ack registers for both of the IRQ pins - 
meaning we might be able to use separate pins for separate (ackable) 
IRQs - and be able to support two IRQs without an acking race. This 
however would require some logic for which pin to use for which IRQ - 
and potentially this selection could be HW dependant. Currently I am not 
supporting anything but watermark and data-ready, both of which get 
cleaned when data is read. It's still worth mentioning that care must be 
taken if ackable IRQs are to be added...

> 
>> +	 */
>> +	if (ack)
>> +		ret = IRQ_HANDLED;
>> +	else
>> +		ret = IRQ_NONE;
>> +
>> +	mutex_unlock(&data->mutex);
>> +
>> +	return ret;
>> +}
>> +
>> +static void kx022a_trig_reen(struct iio_trigger *trig)
>> +{
>> +	struct kx022a_trigger *t = iio_trigger_get_drvdata(trig);
> 
> Mentioned above, but I really don't like the pointer loops between
> the trigger and main _data structure.
> 

Yep. This is also something that remained from supporting an array of 
triggers.

>> +	struct kx022a_data *data = t->data;
>> +
>> +	enable_irq(data->irq);
>> +}
>> +
>> +static const struct iio_trigger_ops kx022a_trigger_ops = {
>> +	.set_trigger_state = kx022a_trigger_set_state,
>> +	.reenable = kx022a_trig_reen,
>> +};
>> +
>> +static int kx022a_chip_init(struct kx022a_data *data)
>> +{
>> +	int ret, dummy;
>> +
>> +	/*
>> +	 * Disable IRQs because if the IRQs are left on (for example by
>> +	 * a shutdown which did not deactivate the accelerometer) we do
>> +	 * most probably end up flooding the system with unhandled IRQs
>> +	 * and get the line disabled from SOC side.
>> +	 */
>> +	ret = regmap_write(data->regmap, KX022A_REG_INC4, 0);
> 
> Unusual to do this rather than a reset.  Quick look suggests there is
> a suitable software reset (CNTL2)

Same thing was just told me by a colleague of mine yesterday. Reset 
simplifies this quite a bit. (I just wonder if we can trust the reset 
always initializes the IC to same defaults or if there may be OTP 
differencies. I am new to these sensors).

>> +	if (ret) {
>> +		dev_err(data->dev, "Failed to init IRQ states\n");
>> +		return ret;
>> +	}
>> +
>> +	ret = kx022a_set_drdy_irq(data, false);
>> +	if (ret) {
>> +		dev_err(data->dev, "Failed to init DRDY\n");
>> +		return ret;
>> +	}
>> +
>> +	/* Clear any pending IRQs */
>> +	ret = regmap_read(data->regmap, KX022A_REG_INT_REL, &dummy);
>> +	if (ret) {
>> +		dev_err(data->dev, "Failed to ACK IRQs\n");
>> +		return ret;
>> +	}
>> +	/* set data res 16bit */
>> +	ret = regmap_set_bits(data->regmap, KX022A_REG_BUF_CNTL2,
>> +			      KX022A_MASK_BRES);
> 
> Can we have a _BRES_16 to make this self documenting?

Yes we can :)


>> +
>> +	ret = iio_triggered_buffer_setup_ext(idev,
> 
> devm_

Ouch. Yes.

> 
>> +					     &iio_pollfunc_store_time,
>> +					     kx022a_trigger_handler,
>> +					     IIO_BUFFER_DIRECTION_IN,
>> +					     &kx022a_buffer_ops,
>> +					     kx022a_fifo_attributes);
>> +
>> +	if (ret)
>> +		return dev_err_probe(data->dev, ret,
>> +				     "iio_triggered_buffer_setup_ext FAIL %d\n",
>> +				     ret);
>> +
>> +	indio_trig = devm_iio_trigger_alloc(dev, "%sdata-rdy-dev%d", idev->name,
>> +					    iio_device_id(idev));
>> +	if (!indio_trig)
>> +		return -ENOMEM;
>> +
>> +	data->trigger.indio_trig = indio_trig;
>> +
>> +	indio_trig->ops = &kx022a_trigger_ops;
>> +	data->trigger.data = data;
>> +	iio_trigger_set_drvdata(indio_trig, &data->trigger);
>> +
>> +	ret = iio_trigger_register(indio_trig);
> 
> Don't mix devm and non devm allocation / registration.

Ouch2. I know and should have seen this. Thanks for pointing it out.

> 
>> +	if (ret)
>> +		return dev_err_probe(data->dev, ret,
>> +				     "Trigger registration failed\n");
>> +
>> +	ret = devm_iio_device_register(data->dev, idev);
>> +	if (ret < 0)
>> +		return dev_err_probe(dev, ret,
>> +				     "Unable to register iio device\n");
>> +
>> +	ret = devm_request_threaded_irq(data->dev, irq, kx022a_irq_handler,
>> +					&kx022a_irq_thread_handler,
>> +					IRQF_ONESHOT, "kx022", idev);
>> +	if (ret)
>> +		return dev_err_probe(data->dev, ret, "Could not request IRQ\n");
>> +
>> +	return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(kx022a_probe_internal);

Best Regards
	-- Matti

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

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

Re: [RFC PATCH 4/5] iio: accel: Support Kionix/ROHM KX022A accelerometer

[Jonathan Cameron]

On Fri, 23 Sep 2022 09:31:12 +0300
Matti Vaittinen <mazziesaccount@gmail.com> wrote:

> On 9/22/22 20:03, Jonathan Cameron wrote:
> > On Wed, 21 Sep 2022 14:45:35 +0300
> > Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> >   
> >> KX022A is a 3-axis Accelerometer from ROHM/Kionix. The senor features
> >> include variable ODRs, I2C and SPI control, FIFO/LIFO with watermark IRQ,
> >> tap/motion detection, wake-up & back-to-sleep events, four acceleration
> >> ranges (2, 4, 8 and 16g) and probably some other cool fatures.
> >>
> >> Add support for the basic accelerometer features such as getting the
> >> acceleration data via IIO. (raw reads, triggered buffer [data-ready] or
> >> using the WMI IRQ).
> >>
> >> Important things to be added include the double-tap, motion
> >> detection and wake-up as well as the runtime power management.
> >>
> >> NOTE: Filling-up the hardware FIFO should be avoided. During my testing
> >> I noticed that filling up the hardware FIFO might mess-up the sample
> >> count. My sensor ended up in a state where amount of data in FIFO was
> >> reported to be 0xff bytes, which equals to 42,5 samples. Specification
> >> says the FIFO can hold maximum of 41 samples in HiRes mode. Also, at
> >> least once the FIFO was stuck in a state where reading data from
> >> hwardware FIFO did not decrease the amount of data reported to be in the  
> > spell check this.
> >   
> >> FIFO - eg. FIFO was "stuck". The code has now an error count and 10
> >> reads with invalid FIFO data count will cause the fifo contents to be
> >> dropped.  
> > Ouch - that's nasty.  
> 
> Indeed it is. As this commit states, this is pretty initial support for 
> the accelerometer. I want to enable people to do basic experimenting and 
> also use the component to some slow ODR solutions. Besides, having even 
> a basic support in-tree enable people to add further improvements :) So, 
> I am hoping / expecting to see improvements added also by other people 
> using this. I think that after this initial support many people still 
> _need_ for example the runtime PM. Maybe we will also end up with some 
> nicer solution to the FIFO issues.

My initial gut feeling is that, if that fifo issue doesn't have a clean
solution, we don't don't support the fifo (by default anyway -
could have a module parameter or something to turn it on).  Late handling
of interrupts is something that can happen for lots of reasons. It should
never be fatal!

> 
> Oh, please note that the patch is no longer complete. I've "snipped" 
> unrelated pieces when replying to Jonathan. I did also just silently 
> ignore the comments which I simply agree with.

Exactly as it should be!  Thanks for taking the time to do that.

> 
> >>
> >> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>  
> > 
> > Hi Matti,
> > 
> > A somewhat superficial review as I'm short on time, but wanted to get some
> > initial comments out to you as I'd started reviewing this yesterday and not
> > sure when I'd get back to it.  
> 
> No problem Jonathan. I appreciate the feedback - and even if the review 
> has not been completed the initial feedback will already allow me to 
> rework :) I won't go anywhere so you can for sure send me more comments 
> during the coming new iterations.
> 
> Besides, I appreciate your engagement even more as some maintainers do 
> not have the time to look at the RFC patches at all. Getting even 
> initial feedback this quickly is much more than I expected! So, thanks!

I only get to RFCs because of the help others give with review of IIO
patches which means I'm not always completely snowed under!

> >> +// SPDX-License-Identifier: GPL-2.0-only
> >> +//  
> > 
> > Other than where required for SPDX use the
> > /*
> >   * Copyright...
> >   */
> > syntax.  
> 
> No problem. Some maintainers prefer having the top of the file comment 
> block looking coherent and it also pleases my eye more. Still, can be 
> changed to suit the IIO-style.

IIO style well predates SPDX in the kernel (and maybe in general!) :)

> 
> >   
> >> +// Copyright (C) 2022 ROHM Semiconductors
> >> +//
> >> +// ROHM/KIONIX KX022A accelerometer driver
> >> +  
> 
> >> +static int kx022a_i2c_probe(struct i2c_client *i2c)
> >> +{
> >> +	struct regmap *regmap;
> >> +	struct device *dev = &i2c->dev;
> >> +
> >> +	if (!i2c->irq) {  
> > 
> > There look to be 2 interrupt lines, so you need to know which
> > one this is.  fwnode_irq_get_byname() trying first int1 then
> > int2 (as only a single one might be mapped).  
> 
> Seems like you know the Kionix component(s).

Last one I had was a kxsd9 which was over 10 years ago!  Just get used
to looking through datasheets for things that we have gotten wrong before :)

> Cool! My idea is initially 
> to only support a fixed configuration with IRQ connected to the IRQ1 
> pin. The support for the second IRQ can be added when needed. I think 
> it's better to have at first something working and add features 
> iteratively when needed. I do also believe that people who add support 
> to IRQ2-pin should also be testing this. I have currently only IRQ1 
> connected.
> 
> Would a comment explaining this do?
Hmm. Binding still needs to support either interrupt, but if the driver
doesn't that's just a feature to add. Normally it's pretty simple to
do irq1 or irq2 though...
irq1 and irq2 is of course much harder!

> 
> You made me think that this should probably be explained in the 
> dt-bindings already. I need to add the irq-names and mention that if the 
> irq-names is not populated it is assumed (only) the IRQ1 is used.

New binding. Just insist that interrupt-names is provided :)
Makes for simpler code and would be good practice anyway I think.

> 
...
> 
> >> +
> >> +struct kx022a_data;
> >> +
> >> +struct kx022a_trigger {  
> > 
> > Unusual structure. IIRC we normally just squash this stuff into
> > the iio_priv structure and set the trigger devdata to the iio_dev.
> > I haven't really thought about the advantages of this way around
> > but can see a disadvantage is you get circular pointer loops
> > this way.  
> 
> I'd better admit I didn't invent this myself. I stole big parts of the 
> logic from the bmc150-accel-core.c which supports multiple triggers. (By 
> the way, I should probably metion that in the commit message). All 
> triggers are in an array - so if you only get the trigger pointer to one 
> of the triggers in the array, then calculating the start of the 
> containing struct becomes non trivial.
> 
> This does not apply to this driver now as we only support the data-ready 
> trigger so I can change this. OTOH, reading the list of features KX022A 
> hardware provides - I wouldn't be surprized to see a new trigger(s) in 
> the future. It'd be nice to get support for different modes like for 
> example capturing N events that were stored prior certain movement. 

That stuff is hard to do in a generic enough way that standard software
can support it.  Still I'd be interested to see a proposal.  It's a subset
of what impact sensors tend to do.

> These might be best implemented with triggers. What is your adviece - is 
> it worth trying to keep adding new trigger(s) as a low-hanging fruit - 
> or is it just better to make the current (limited) implementation as 
> simple as possible?

Simple first.   Different matter if you add the other triggers later in
the same patch series, but history says half the interesting features
we plan to add never get added.

> 



> >> +static int kx022a_turn_on_unlock(struct kx022a_data *data)
> >> +{
> >> +	int ret;
> >> +  
> > This is not used enough that I can see a strong reason for the
> > wrapper.  Just put the two calls inline and rename the unlocked case.
> >   
> 
> I kind of see the value of the wrappers as it documents the purpose of 
> locking. It's kind of a wake-up that we really want to keep the access 
> locked during sensor 'shut-down'. I don't know how other sensors work 
> but for me the Kionix sensors requiring the sensor to be "turned off" 
> for the duration of pretty much _any_ configurations was a surprize... 

That's unusual in remotely modern devices.  It was more common some years
back to have restrictions like that.  Often I suspect the device was fine
but no one wanted to test all the corner cases of making changes whilst
it was running so they made it impossible to hit them.


> Again, I think this will be required for _all_ new configuration code we 
> add in the driver.

ok. Maybe rename the mutex to make it self describing?  Mind you I can't
think of a good name.  interlock would probably be a bit confusing though
that's what it would be called if it was on a heavy bit of machinery!

> 
> >> +	ret = __kx022a_turn_on_unlocked(data);
> >> +	mutex_unlock(&data->mutex);
> >> +
> >> +	return ret;
> >> +}

...


...

> >> +	/*
> >> +	 * Approximate timestamps for each of the sample based on the sampling
> >> +	 * frequency, timestamp for last sample and number of samples.
> >> +	 *
> >> +	 * We'd better not use the current bandwidth settings to compute the
> >> +	 * sample period. The real sample rate varies with the device and
> >> +	 * small variation adds when we store a large number of samples.
> >> +	 *
> >> +	 * To avoid this issue we compute the actual sample period ourselves
> >> +	 * based on the timestamp delta between the last two flush operations.
> >> +	 */
> >> +	if (data->old_timestamp) {
> >> +		sample_period = (data->timestamp - data->old_timestamp);
> >> +		do_div(sample_period, count);
> >> +	} else {
> >> +		sample_period = data->odr_ns;
> >> +	}
> >> +	tstamp = data->timestamp - (count - 1) * sample_period;
> >> +
> >> +	if (samples && count > samples) {
> >> +		/*
> >> +		 * Here we leave some old samples to the buffer. We need to
> >> +		 * adjust the timestamp to match the first sample in the buffer
> >> +		 * or we will miscalculate the sample_period at next round.
> >> +		 */ >> +		data->timestamp -= (count - samples) * sample_period;
> >> +		count = samples;  
> 
> Just a note not directly related to this driver. I borrowed (stoled) the 
> timestamp logic from the bmc150-accel-core.c. The bmc150-core did not 
> have this timestamp adjustment which I think is necessary in case the 
> fifo is not read empty. Someone familiar with the bmc150-accel-core.c 
> might want to check this.

Interesting point.  Probably worth sending a separate email to query this
as quite likely no one who is that position is reading this thread!

> 
> >> +	}

....


> >> +
> >> +static int kx022a_prepare_irq_pin(struct kx022a_data *data)
> >> +{
> >> +	/* Enable IRQ1 pin. Set polarity to active low */  
> > 
> > Must either handle both pins or at least know if it is irq2 and
> > treat that as no irq for now.  
> 
> I don't want to try supporting both pins for now. It makes this somewhat 
> more complex - especially if we want to support using two IRQs. That 
> will require some thorough thinking which I don't have time to do right 
> now :(
> 
> I can add the irq-names to the bindings and add check to the probe so 
> that if the irq2 is used we error out with nice 'not supported' message.
> 

A slightly nicer thing to do is support one irq, but let it be either irq1 or
irq2. If both are supplied just ignore the second one.  People have
an 'amusing' habit of only wiring up irq2 on boards.

...


> >> +static int kx022a_fifo_enable(struct kx022a_data *data)
> >> +{
> >> +	int ret = 0;
> >> +
> >> +	ret = kx022a_turn_off_lock(data);
> >> +	if (ret)
> >> +		return ret;
> >> +
> >> +	/* Write WMI to HW */  
> > 
> > Where WMI is?  
> 
> Stored in the iio_priv. data->watermark. The kx022a_set_watermark() only 
> updates the value in iio_priv() and we take it in use only when the 
> buffer is enabled.

/* Write watermark to HW */ ?

> 
> 
>
> >> +/* Read the data from the fifo and fill IIO buffers */
> >> +static irqreturn_t kx022a_irq_thread_handler(int irq, void *private)
> >> +{
> >> +	struct iio_dev *idev = private;
> >> +	struct kx022a_data *data = iio_priv(idev);
> >> +	bool ack = false;
> >> +	int ret;
> >> +
> >> +	mutex_lock(&data->mutex);
> >> +
> >> +	if (data->state & KX022A_STATE_FIFO) {
> >> +		ret = __kx022a_fifo_flush(idev, KX022A_FIFO_LENGTH, true);
> >> +		if (ret > 0)
> >> +			ack = true;
> >> +	}
> >> +	/*
> >> +	 * WMI and data-ready IRQs are acked when results are read. If we add
> >> +	 * TILT/WAKE or other IRQs - then we may need to implement the acking
> >> +	 * (which is racy).  
> > 
> > I'm not even going to ask for more info at this stage but that sound worrying.  
> 
> Yep, you didn't ask but I can still babble a bit :) It's just about the 
> fact that this (and many other) Kionix sensors clear all IRQs when a 
> single bit of a single register is written. This naturally is racy 
> because there will always be a time-window for new IRQs to trigger 
> between reading the active IRQs and writing the ack. So with this acking 
> mechanism we do potentially lose IRQs.
> 
> It might be there were separate ack registers for both of the IRQ pins - 
> meaning we might be able to use separate pins for separate (ackable) 
> IRQs - and be able to support two IRQs without an acking race. This 
> however would require some logic for which pin to use for which IRQ - 
> and potentially this selection could be HW dependant. Currently I am not 
> supporting anything but watermark and data-ready, both of which get 
> cleaned when data is read. It's still worth mentioning that care must be 
> taken if ackable IRQs are to be added...

Fair enough.  I 'love' stupid designs like this.  If you want another
silly interrupt example, take the PCI Data Object Exchanges spec that
has caused me much woe over the last year :) 

For this one, probably a case of only one interrupt source at a time
(or one per pin as you suggest).

> 
> >> +	struct kx022a_data *data = t->data;
> >> +
> >> +	enable_irq(data->irq);
> >> +}
> >> +
> >> +static const struct iio_trigger_ops kx022a_trigger_ops = {
> >> +	.set_trigger_state = kx022a_trigger_set_state,
> >> +	.reenable = kx022a_trig_reen,
> >> +};
> >> +
> >> +static int kx022a_chip_init(struct kx022a_data *data)
> >> +{
> >> +	int ret, dummy;
> >> +
> >> +	/*
> >> +	 * Disable IRQs because if the IRQs are left on (for example by
> >> +	 * a shutdown which did not deactivate the accelerometer) we do
> >> +	 * most probably end up flooding the system with unhandled IRQs
> >> +	 * and get the line disabled from SOC side.
> >> +	 */
> >> +	ret = regmap_write(data->regmap, KX022A_REG_INC4, 0);  
> > 
> > Unusual to do this rather than a reset.  Quick look suggests there is
> > a suitable software reset (CNTL2)  
> 
> Same thing was just told me by a colleague of mine yesterday. Reset 
> simplifies this quite a bit. (I just wonder if we can trust the reset 
> always initializes the IC to same defaults or if there may be OTP 
> differencies. I am new to these sensors).
> 

I really hope we can rely on any documented reset values!

Jonathan

Re: [RFC PATCH 4/5] iio: accel: Support Kionix/ROHM KX022A accelerometer

[Vaittinen, Matti]

On 9/24/22 18:17, Jonathan Cameron wrote:
> On Fri, 23 Sep 2022 09:31:12 +0300
> Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> 
>> On 9/22/22 20:03, Jonathan Cameron wrote:
>>> On Wed, 21 Sep 2022 14:45:35 +0300
>>> Matti Vaittinen <mazziesaccount@gmail.com> wrote:
>>>    
>>>> KX022A is a 3-axis Accelerometer from ROHM/Kionix. The senor features
>>>> include variable ODRs, I2C and SPI control, FIFO/LIFO with watermark IRQ,
>>>> tap/motion detection, wake-up & back-to-sleep events, four acceleration
>>>> ranges (2, 4, 8 and 16g) and probably some other cool fatures.
>>>>
>>>> Add support for the basic accelerometer features such as getting the
>>>> acceleration data via IIO. (raw reads, triggered buffer [data-ready] or
>>>> using the WMI IRQ).
>>>>
>>>> Important things to be added include the double-tap, motion
>>>> detection and wake-up as well as the runtime power management.
>>>>
>>>> NOTE: Filling-up the hardware FIFO should be avoided. During my testing
>>>> I noticed that filling up the hardware FIFO might mess-up the sample
>>>> count. My sensor ended up in a state where amount of data in FIFO was
>>>> reported to be 0xff bytes, which equals to 42,5 samples. Specification
>>>> says the FIFO can hold maximum of 41 samples in HiRes mode. Also, at
>>>> least once the FIFO was stuck in a state where reading data from
>>>> hwardware FIFO did not decrease the amount of data reported to be in the
>>> spell check this.
>>>    
>>>> FIFO - eg. FIFO was "stuck". The code has now an error count and 10
>>>> reads with invalid FIFO data count will cause the fifo contents to be
>>>> dropped.
>>> Ouch - that's nasty.
>>
>> Indeed it is. As this commit states, this is pretty initial support for
>> the accelerometer. I want to enable people to do basic experimenting and
>> also use the component to some slow ODR solutions. Besides, having even
>> a basic support in-tree enable people to add further improvements :) So,
>> I am hoping / expecting to see improvements added also by other people
>> using this. I think that after this initial support many people still
>> _need_ for example the runtime PM. Maybe we will also end up with some
>> nicer solution to the FIFO issues.
> 
> My initial gut feeling is that, if that fifo issue doesn't have a clean
> solution, we don't don't support the fifo (by default anyway -
> could have a module parameter or something to turn it on).  Late handling
> of interrupts is something that can happen for lots of reasons. It should
> never be fatal!

Tell me about it... More than 10-years ago I "inherited" maintenance of 
a timing code which was built on periodic 10msec IRQ which incremented a 
counter. (And after that there were many new generations of Linux based 
devices with various "rt"-requirements). Missing an IRQ was fatal as 
there were no hardware where we could read the correct timestamp and 
other units in the system were no longer in sync if the counter was 
wrong. Only recovery was complete system restart for all units - which 
in that use case was really bad. I learned to absolutely hate the serial 
prints over slow UART - and I learned to love irqsoff tracer. I don't 
work for that company anymore and I believe the product has already 
retired. Yet, I still get *shrugs* when I see hard time limits for 
serving IRQs on Linux. (Other than that I kind of like pondering the 
rt-challenges).

Anyways, I don't see a real risk for example when using the ODRs < 2 Hz 
and setting the watermark to somewhere near 20 samples.

It's really unfortunate the HW has these "hickups" - but I think it is 
still perfectly usable for many cases - we just really need to document 
the corner cases somewhere.

> Simple first.   Different matter if you add the other triggers later in
> the same patch series, but history says half the interesting features
> we plan to add never get added.

ack.

>>> Must either handle both pins or at least know if it is irq2 and
>>> treat that as no irq for now.
>>
>> I don't want to try supporting both pins for now. It makes this somewhat
>> more complex - especially if we want to support using two IRQs. That
>> will require some thorough thinking which I don't have time to do right
>> now :(
>>
>> I can add the irq-names to the bindings and add check to the probe so
>> that if the irq2 is used we error out with nice 'not supported' message.
>>
> 
> A slightly nicer thing to do is support one irq, but let it be either irq1 or
> irq2. If both are supplied just ignore the second one.  People have
> an 'amusing' habit of only wiring up irq2 on boards.
> 

Ok. It shouldn't be such a big change for the code. I'll see what it 
will look like.

>>>> +
>>>> +static int kx022a_chip_init(struct kx022a_data *data)
>>>> +{
>>>> +	int ret, dummy;
>>>> +
>>>> +	/*
>>>> +	 * Disable IRQs because if the IRQs are left on (for example by
>>>> +	 * a shutdown which did not deactivate the accelerometer) we do
>>>> +	 * most probably end up flooding the system with unhandled IRQs
>>>> +	 * and get the line disabled from SOC side.
>>>> +	 */
>>>> +	ret = regmap_write(data->regmap, KX022A_REG_INC4, 0);
>>>
>>> Unusual to do this rather than a reset.  Quick look suggests there is
>>> a suitable software reset (CNTL2)
>>
>> Same thing was just told me by a colleague of mine yesterday. Reset
>> simplifies this quite a bit. (I just wonder if we can trust the reset
>> always initializes the IC to same defaults or if there may be OTP
>> differencies. I am new to these sensors).
>>
> 
> I really hope we can rely on any documented reset values!

That is a sane assumption to do - but in my experience we don't live in 
a sane world ;) I've seen way too many cases where the defaults are 
changed for ICs for example by changing OTP. And sometimes the OTP 
change has not been visible to the drivers from any revision registers 
or such.

I'm not talking about Kionix sensors though as I've never worked with 
Kionix sensors before - so let's just try out the reset and fix things 
if problems emerge. I am probably just a bit paranoid :)

Thanks for all the help!

Yours,
	-- Matti

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

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

Re: [RFC PATCH 4/5] iio: accel: Support Kionix/ROHM KX022A accelerometer

[Jonathan Cameron]

On Mon, 26 Sep 2022 05:02:44 +0000
"Vaittinen, Matti" <Matti.Vaittinen@fi.rohmeurope.com> wrote:

> On 9/24/22 18:17, Jonathan Cameron wrote:
> > On Fri, 23 Sep 2022 09:31:12 +0300
> > Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> >   
> >> On 9/22/22 20:03, Jonathan Cameron wrote:  
> >>> On Wed, 21 Sep 2022 14:45:35 +0300
> >>> Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> >>>      
> >>>> KX022A is a 3-axis Accelerometer from ROHM/Kionix. The senor features
> >>>> include variable ODRs, I2C and SPI control, FIFO/LIFO with watermark IRQ,
> >>>> tap/motion detection, wake-up & back-to-sleep events, four acceleration
> >>>> ranges (2, 4, 8 and 16g) and probably some other cool fatures.
> >>>>
> >>>> Add support for the basic accelerometer features such as getting the
> >>>> acceleration data via IIO. (raw reads, triggered buffer [data-ready] or
> >>>> using the WMI IRQ).
> >>>>
> >>>> Important things to be added include the double-tap, motion
> >>>> detection and wake-up as well as the runtime power management.
> >>>>
> >>>> NOTE: Filling-up the hardware FIFO should be avoided. During my testing
> >>>> I noticed that filling up the hardware FIFO might mess-up the sample
> >>>> count. My sensor ended up in a state where amount of data in FIFO was
> >>>> reported to be 0xff bytes, which equals to 42,5 samples. Specification
> >>>> says the FIFO can hold maximum of 41 samples in HiRes mode. Also, at
> >>>> least once the FIFO was stuck in a state where reading data from
> >>>> hwardware FIFO did not decrease the amount of data reported to be in the  
> >>> spell check this.
> >>>      
> >>>> FIFO - eg. FIFO was "stuck". The code has now an error count and 10
> >>>> reads with invalid FIFO data count will cause the fifo contents to be
> >>>> dropped.  
> >>> Ouch - that's nasty.  
> >>
> >> Indeed it is. As this commit states, this is pretty initial support for
> >> the accelerometer. I want to enable people to do basic experimenting and
> >> also use the component to some slow ODR solutions. Besides, having even
> >> a basic support in-tree enable people to add further improvements :) So,
> >> I am hoping / expecting to see improvements added also by other people
> >> using this. I think that after this initial support many people still
> >> _need_ for example the runtime PM. Maybe we will also end up with some
> >> nicer solution to the FIFO issues.  
> > 
> > My initial gut feeling is that, if that fifo issue doesn't have a clean
> > solution, we don't don't support the fifo (by default anyway -
> > could have a module parameter or something to turn it on).  Late handling
> > of interrupts is something that can happen for lots of reasons. It should
> > never be fatal!  
> 
> Tell me about it... More than 10-years ago I "inherited" maintenance of 
> a timing code which was built on periodic 10msec IRQ which incremented a 
> counter. (And after that there were many new generations of Linux based 
> devices with various "rt"-requirements). Missing an IRQ was fatal as 
> there were no hardware where we could read the correct timestamp and 
> other units in the system were no longer in sync if the counter was 
> wrong. Only recovery was complete system restart for all units - which 
> in that use case was really bad. I learned to absolutely hate the serial 
> prints over slow UART - and I learned to love irqsoff tracer. I don't 
> work for that company anymore and I believe the product has already 
> retired. Yet, I still get *shrugs* when I see hard time limits for 
> serving IRQs on Linux. (Other than that I kind of like pondering the 
> rt-challenges).
> 
> Anyways, I don't see a real risk for example when using the ODRs < 2 Hz 
> and setting the watermark to somewhere near 20 samples.
> 
> It's really unfortunate the HW has these "hickups" - but I think it is 
> still perfectly usable for many cases - we just really need to document 
> the corner cases somewhere.

I'd rather we gated it - so by default the fifo mode isn't there and
people who are really confident they want it set a module parameter or
similar.  Cuts down on the bug reports.

> 
> > Simple first.   Different matter if you add the other triggers later in
> > the same patch series, but history says half the interesting features
> > we plan to add never get added.  
> 
> ack.
> 
> >>> Must either handle both pins or at least know if it is irq2 and
> >>> treat that as no irq for now.  
> >>
> >> I don't want to try supporting both pins for now. It makes this somewhat
> >> more complex - especially if we want to support using two IRQs. That
> >> will require some thorough thinking which I don't have time to do right
> >> now :(
> >>
> >> I can add the irq-names to the bindings and add check to the probe so
> >> that if the irq2 is used we error out with nice 'not supported' message.
> >>  
> > 
> > A slightly nicer thing to do is support one irq, but let it be either irq1 or
> > irq2. If both are supplied just ignore the second one.  People have
> > an 'amusing' habit of only wiring up irq2 on boards.
> >   
> 
> Ok. It shouldn't be such a big change for the code. I'll see what it 
> will look like.
> 
> >>>> +
> >>>> +static int kx022a_chip_init(struct kx022a_data *data)
> >>>> +{
> >>>> +	int ret, dummy;
> >>>> +
> >>>> +	/*
> >>>> +	 * Disable IRQs because if the IRQs are left on (for example by
> >>>> +	 * a shutdown which did not deactivate the accelerometer) we do
> >>>> +	 * most probably end up flooding the system with unhandled IRQs
> >>>> +	 * and get the line disabled from SOC side.
> >>>> +	 */
> >>>> +	ret = regmap_write(data->regmap, KX022A_REG_INC4, 0);  
> >>>
> >>> Unusual to do this rather than a reset.  Quick look suggests there is
> >>> a suitable software reset (CNTL2)  
> >>
> >> Same thing was just told me by a colleague of mine yesterday. Reset
> >> simplifies this quite a bit. (I just wonder if we can trust the reset
> >> always initializes the IC to same defaults or if there may be OTP
> >> differencies. I am new to these sensors).
> >>  
> > 
> > I really hope we can rely on any documented reset values!  
> 
> That is a sane assumption to do - but in my experience we don't live in 
> a sane world ;) I've seen way too many cases where the defaults are 
> changed for ICs for example by changing OTP. And sometimes the OTP 
> change has not been visible to the drivers from any revision registers 
> or such.
> 
> I'm not talking about Kionix sensors though as I've never worked with 
> Kionix sensors before - so let's just try out the reset and fix things 
> if problems emerge. I am probably just a bit paranoid :)

Entirely reasonable!

Jonathan

> 
> Thanks for all the help!
> 
> Yours,
> 	-- Matti
> 

Re: [RFC PATCH 4/5] iio: accel: Support Kionix/ROHM KX022A accelerometer

[Matti Vaittinen]

Hi Jonathan,

On 9/22/22 20:03, Jonathan Cameron wrote:
> On Wed, 21 Sep 2022 14:45:35 +0300
> Matti Vaittinen <mazziesaccount@gmail.com> wrote:
>> +
>> +/*
>> + * The sensor HW can support ODR up to 1600 Hz - which is beyond what most of
>> + * Linux CPUs can handle w/o dropping samples. Also, the low power mode is not
>> + * available for higher sample rates. Thus the driver only supports 200 Hz and
>> + * slower ODRs. Slowest being 0.78 Hz
>> + */
>> +static IIO_CONST_ATTR_SAMP_FREQ_AVAIL("0.78 1.563 3.125 6.25 12.5 25 50 100 200");
>> +static IIO_CONST_ATTR(scale_available,
>> +		      "598.550415 1197.10083 2394.20166 4788.40332");
>> +
>> +static struct attribute *kx022a_attributes[] = {
>> +	&iio_const_attr_sampling_frequency_available.dev_attr.attr,
>> +	&iio_const_attr_scale_available.dev_attr.attr,
> 
> Use the read_avail() callback instead of doing these as attributes.
> That makes the values available to consumer drivers...

Am I correct that populating the read_avail() does not add sysfs entries 
for available scale/frequency? Eg, if I wish to expose the supported 
values via sysfs I still need these attributes? Implementing the 
read_avail() as well is not a problem though.

>> +static int kx022a_turn_on_unlock(struct kx022a_data *data)
>> +{
>> +	int ret;
>> +
> This is not used enough that I can see a strong reason for the
> wrapper.  Just put the two calls inline and rename the unlocked case.

In my opinion the kx022a_turn_on_unlock() and  kx022a_turn_off_lock() do 
simplify functions. Especially after I started using the 
iio_device_claim_direct_mode() :) Thus I will leave these for the v2 - 
please ping me again if you still want to see them removed (but I think 
the usage of iio_device_claim_direct_mode() changed this to favour the 
kx022a_turn_on_unlock() and kx022a_turn_off_lock()).

>> +static int kx022a_chip_init(struct kx022a_data *data)
>> +{
>> +	int ret, dummy;
>> +
>> +	/*
>> +	 * Disable IRQs because if the IRQs are left on (for example by
>> +	 * a shutdown which did not deactivate the accelerometer) we do
>> +	 * most probably end up flooding the system with unhandled IRQs
>> +	 * and get the line disabled from SOC side.
>> +	 */
>> +	ret = regmap_write(data->regmap, KX022A_REG_INC4, 0);
> 
> Unusual to do this rather than a reset.  Quick look suggests there is
> a suitable software reset (CNTL2)

I switched to the software reset as you suggested. I am not really 
convinced it is a better way. It seems the software reset requires us to 
re-init the regmap cache. Well, I don't think it is a bid geal though - 
just something worth noticing I guess.

>> +
>> +int kx022a_probe_internal(struct device *dev, int irq)
>> +{
>> +	static const char * const regulator_names[] = {"io_vdd", "vdd"};
>> +	struct iio_trigger *indio_trig;
>> +	struct kx022a_data *data;
>> +	struct regmap *regmap;
>> +	unsigned int chip_id;
>> +	struct iio_dev *idev;
>> +	int ret;
>> +
>> +	if (WARN_ON(!dev))
>> +		return -ENODEV;
>> +
>> +	regmap = dev_get_regmap(dev, NULL);
>> +	if (!regmap) {
>> +		dev_err(dev, "no regmap\n");
> 
> Use dev_err_probe() for all dev_err() stuff in probe paths.
> It ends up cleaner and we don't care about the tiny overhead
> of checking for deferred.

This one bothers me a bit. It just does not feel correct to pass -EINVAL 
for the dev_err_probe() so the dev_err_probe() can check if -EINVAL != 
-EPROBE_DEFER. I do understand perfectly well the consistent use of 
dev_err_probe() for all cases where we get an error-code from a function 
and return it - but using dev_err_probe() when we hard-code the return 
value in code calling the dev_err_probe() does not feel like "the right 
thing to do" (tm).

Eg, I agree that
return dev_err_probe(dev, ret, "bar");
is nice even if we know the function that gave us the "ret" never 
requests defer (as that can change some day).

However, I don't like issuing:
return dev_err_probe(dev, -EINVAL, "bar");

Well, please let me know if you think the dev_err_probe() should be used 
even in cases where we hard code the return to something...

For v2 I do change the other prints (like the one about failed regmap 
read below).

> 
>> +
>> +		return -EINVAL;
>> +	}
>> +
>> +	idev = devm_iio_device_alloc(dev, sizeof(*data));
>> +	if (!idev)
>> +		return -ENOMEM;
>> +
>> +	data = iio_priv(idev);
>> +
>> +	/*
>> +	 * VDD is the analog and digital domain voltage supply
>> +	 * IO_VDD is the digital I/O voltage supply
>> +	 */
>> +	ret = devm_regulator_bulk_get_enable(dev, ARRAY_SIZE(regulator_names),
>> +					     regulator_names);
>> +	if (ret && ret != -ENODEV)
>> +		return dev_err_probe(dev, ret, "failed to enable regulator\n");
>> +
>> +	ret = regmap_read(regmap, KX022A_REG_WHO, &chip_id);
>> +	if (ret) {
>> +		dev_err(dev, "Failed to access sensor\n");
Yours,
	-- Matti Vaittinen

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

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

Re: [RFC PATCH 4/5] iio: accel: Support Kionix/ROHM KX022A accelerometer

[Andy Shevchenko]

On Wed, Sep 28, 2022 at 02:14:14PM +0300, Matti Vaittinen wrote:
> On 9/22/22 20:03, Jonathan Cameron wrote:
> > On Wed, 21 Sep 2022 14:45:35 +0300

...

> > > +		dev_err(dev, "no regmap\n");
> > 
> > Use dev_err_probe() for all dev_err() stuff in probe paths.
> > It ends up cleaner and we don't care about the tiny overhead
> > of checking for deferred.
> 
> This one bothers me a bit. It just does not feel correct to pass -EINVAL for
> the dev_err_probe() so the dev_err_probe() can check if -EINVAL !=
> -EPROBE_DEFER. I do understand perfectly well the consistent use of
> dev_err_probe() for all cases where we get an error-code from a function and
> return it - but using dev_err_probe() when we hard-code the return value in
> code calling the dev_err_probe() does not feel like "the right thing to do"
> (tm).
> 
> Eg, I agree that
> return dev_err_probe(dev, ret, "bar");
> is nice even if we know the function that gave us the "ret" never requests
> defer (as that can change some day).
> 
> However, I don't like issuing:
> return dev_err_probe(dev, -EINVAL, "bar");

This case specifically was added into documentation by 7065f92255bb ("driver
core: Clarify that dev_err_probe() is OK even w/out -EPROBE_DEFER").

> Well, please let me know if you think the dev_err_probe() should be used
> even in cases where we hard code the return to something...

And this should be, of course, maintainer's decision.

-- 
With Best Regards,
Andy Shevchenko

Re: [RFC PATCH 4/5] iio: accel: Support Kionix/ROHM KX022A accelerometer

[Matti Vaittinen]

Hi Andy,

On 9/28/22 17:06, Andy Shevchenko wrote:
> On Wed, Sep 28, 2022 at 02:14:14PM +0300, Matti Vaittinen wrote:
>> On 9/22/22 20:03, Jonathan Cameron wrote:
>>> On Wed, 21 Sep 2022 14:45:35 +0300
> 
> ...
> 
>>>> +		dev_err(dev, "no regmap\n");
>>>
>>> Use dev_err_probe() for all dev_err() stuff in probe paths.
>>> It ends up cleaner and we don't care about the tiny overhead
>>> of checking for deferred.
>>
>> This one bothers me a bit. It just does not feel correct to pass -EINVAL for
>> the dev_err_probe() so the dev_err_probe() can check if -EINVAL !=
>> -EPROBE_DEFER. I do understand perfectly well the consistent use of
>> dev_err_probe() for all cases where we get an error-code from a function and
>> return it - but using dev_err_probe() when we hard-code the return value in
>> code calling the dev_err_probe() does not feel like "the right thing to do"
>> (tm).
>>
>> Eg, I agree that
>> return dev_err_probe(dev, ret, "bar");
>> is nice even if we know the function that gave us the "ret" never requests
>> defer (as that can change some day).
>>
>> However, I don't like issuing:
>> return dev_err_probe(dev, -EINVAL, "bar");
> 
> This case specifically was added into documentation by 7065f92255bb ("driver
> core: Clarify that dev_err_probe() is OK even w/out -EPROBE_DEFER").

Yes. And this is exactly what I meant with:
 >> Eg, I agree that
 >> return dev_err_probe(dev, ret, "bar");
 >> is nice even if we know the function that gave us the "ret" never 
requests
 >> defer

There is still (in my opinion) a significant difference if we call:
 >> return dev_err_probe(dev, -EINVAL, "bar");

- where we really hard-code the -EINVAL as a parameter to the 
dev_err_probe()

>> Well, please let me know if you think the dev_err_probe() should be used
>> even in cases where we hard code the return to something...
> 
> And this should be, of course, maintainer's decision.

Ultimately, yes.

Best Regards
	--Matti

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

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

Re: [RFC PATCH 4/5] iio: accel: Support Kionix/ROHM KX022A accelerometer

[Jonathan Cameron]

On Wed, 28 Sep 2022 19:23:06 +0300
Matti Vaittinen <mazziesaccount@gmail.com> wrote:

> Hi Andy,
> 
> On 9/28/22 17:06, Andy Shevchenko wrote:
> > On Wed, Sep 28, 2022 at 02:14:14PM +0300, Matti Vaittinen wrote:  
> >> On 9/22/22 20:03, Jonathan Cameron wrote:  
> >>> On Wed, 21 Sep 2022 14:45:35 +0300  
> > 
> > ...
> >   
> >>>> +		dev_err(dev, "no regmap\n");  
> >>>
> >>> Use dev_err_probe() for all dev_err() stuff in probe paths.
> >>> It ends up cleaner and we don't care about the tiny overhead
> >>> of checking for deferred.  
> >>
> >> This one bothers me a bit. It just does not feel correct to pass -EINVAL for
> >> the dev_err_probe() so the dev_err_probe() can check if -EINVAL !=
> >> -EPROBE_DEFER. I do understand perfectly well the consistent use of
> >> dev_err_probe() for all cases where we get an error-code from a function and
> >> return it - but using dev_err_probe() when we hard-code the return value in
> >> code calling the dev_err_probe() does not feel like "the right thing to do"
> >> (tm).
> >>
> >> Eg, I agree that
> >> return dev_err_probe(dev, ret, "bar");
> >> is nice even if we know the function that gave us the "ret" never requests
> >> defer (as that can change some day).
> >>
> >> However, I don't like issuing:
> >> return dev_err_probe(dev, -EINVAL, "bar");  
> > 
> > This case specifically was added into documentation by 7065f92255bb ("driver
> > core: Clarify that dev_err_probe() is OK even w/out -EPROBE_DEFER").  
> 
> Yes. And this is exactly what I meant with:
>  >> Eg, I agree that
>  >> return dev_err_probe(dev, ret, "bar");
>  >> is nice even if we know the function that gave us the "ret" never   
> requests
>  >> defer  
> 
> There is still (in my opinion) a significant difference if we call:
>  >> return dev_err_probe(dev, -EINVAL, "bar");  
> 
> - where we really hard-code the -EINVAL as a parameter to the 
> dev_err_probe()
> 
> >> Well, please let me know if you think the dev_err_probe() should be used
> >> even in cases where we hard code the return to something...  
> > 
> > And this should be, of course, maintainer's decision.  
> 
> Ultimately, yes.

I'm not that fussed. So happy to accept code taking either view where it
is hard coded in the call like this.
I'd love a dev_err() that took and returned the error value though just
for all those single lines of code saved.

J

> 
> Best Regards
> 	--Matti
> 

Re: [RFC PATCH 4/5] iio: accel: Support Kionix/ROHM KX022A accelerometer

[Jonathan Cameron]

On Wed, 28 Sep 2022 14:14:14 +0300
Matti Vaittinen <mazziesaccount@gmail.com> wrote:

> Hi Jonathan,
> 
> On 9/22/22 20:03, Jonathan Cameron wrote:
> > On Wed, 21 Sep 2022 14:45:35 +0300
> > Matti Vaittinen <mazziesaccount@gmail.com> wrote:  
> >> +
> >> +/*
> >> + * The sensor HW can support ODR up to 1600 Hz - which is beyond what most of
> >> + * Linux CPUs can handle w/o dropping samples. Also, the low power mode is not
> >> + * available for higher sample rates. Thus the driver only supports 200 Hz and
> >> + * slower ODRs. Slowest being 0.78 Hz
> >> + */
> >> +static IIO_CONST_ATTR_SAMP_FREQ_AVAIL("0.78 1.563 3.125 6.25 12.5 25 50 100 200");
> >> +static IIO_CONST_ATTR(scale_available,
> >> +		      "598.550415 1197.10083 2394.20166 4788.40332");
> >> +
> >> +static struct attribute *kx022a_attributes[] = {
> >> +	&iio_const_attr_sampling_frequency_available.dev_attr.attr,
> >> +	&iio_const_attr_scale_available.dev_attr.attr,  
> > 
> > Use the read_avail() callback instead of doing these as attributes.
> > That makes the values available to consumer drivers...  
> 
> Am I correct that populating the read_avail() does not add sysfs entries 
> for available scale/frequency? Eg, if I wish to expose the supported 
> values via sysfs I still need these attributes? Implementing the 
> read_avail() as well is not a problem though.

Need to also set the relevant bit in 
info_mask_shared_by_xxx_avail in the channels for the sysfs files to be created
by calling the read_avail() callback.

When I introduced those I thought about making it mandatory to introduce them
for all the info_mask_shared_by_xxx entries and not having the extra bitmap
but that meant figuring out the relevant entries for a mass of stuff whenever
a driver was converted from the old approach like you've used here.

> 
> >> +static int kx022a_turn_on_unlock(struct kx022a_data *data)
> >> +{
> >> +	int ret;
> >> +  
> > This is not used enough that I can see a strong reason for the
> > wrapper.  Just put the two calls inline and rename the unlocked case.  
> 
> In my opinion the kx022a_turn_on_unlock() and  kx022a_turn_off_lock() do 
> simplify functions. Especially after I started using the 
> iio_device_claim_direct_mode() :) Thus I will leave these for the v2 - 
> please ping me again if you still want to see them removed (but I think 
> the usage of iio_device_claim_direct_mode() changed this to favour the 
> kx022a_turn_on_unlock() and kx022a_turn_off_lock()).
Let's see how it looks in v2.

> 
> >> +static int kx022a_chip_init(struct kx022a_data *data)
> >> +{
> >> +	int ret, dummy;
> >> +
> >> +	/*
> >> +	 * Disable IRQs because if the IRQs are left on (for example by
> >> +	 * a shutdown which did not deactivate the accelerometer) we do
> >> +	 * most probably end up flooding the system with unhandled IRQs
> >> +	 * and get the line disabled from SOC side.
> >> +	 */
> >> +	ret = regmap_write(data->regmap, KX022A_REG_INC4, 0);  
> > 
> > Unusual to do this rather than a reset.  Quick look suggests there is
> > a suitable software reset (CNTL2)  
> 
> I switched to the software reset as you suggested. I am not really 
> convinced it is a better way. It seems the software reset requires us to 
> re-init the regmap cache. 

Yup, though if you've provided the reset defaults that should be quick.

Jonathan

Re: [RFC PATCH 4/5] iio: accel: Support Kionix/ROHM KX022A accelerometer

[Matti Vaittinen]

On 10/2/22 14:18, Jonathan Cameron wrote:
> On Wed, 28 Sep 2022 14:14:14 +0300
> Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> 
>> Hi Jonathan,
>>
>> On 9/22/22 20:03, Jonathan Cameron wrote:
>>> On Wed, 21 Sep 2022 14:45:35 +0300
>>> Matti Vaittinen <mazziesaccount@gmail.com> wrote:
>>>> +
>>>> +/*
>>>> + * The sensor HW can support ODR up to 1600 Hz - which is beyond what most of
>>>> + * Linux CPUs can handle w/o dropping samples. Also, the low power mode is not
>>>> + * available for higher sample rates. Thus the driver only supports 200 Hz and
>>>> + * slower ODRs. Slowest being 0.78 Hz
>>>> + */
>>>> +static IIO_CONST_ATTR_SAMP_FREQ_AVAIL("0.78 1.563 3.125 6.25 12.5 25 50 100 200");
>>>> +static IIO_CONST_ATTR(scale_available,
>>>> +		      "598.550415 1197.10083 2394.20166 4788.40332");
>>>> +
>>>> +static struct attribute *kx022a_attributes[] = {
>>>> +	&iio_const_attr_sampling_frequency_available.dev_attr.attr,
>>>> +	&iio_const_attr_scale_available.dev_attr.attr,
>>>
>>> Use the read_avail() callback instead of doing these as attributes.
>>> That makes the values available to consumer drivers...
>>
>> Am I correct that populating the read_avail() does not add sysfs entries
>> for available scale/frequency? Eg, if I wish to expose the supported
>> values via sysfs I still need these attributes? Implementing the
>> read_avail() as well is not a problem though.
> 
> Need to also set the relevant bit in
> info_mask_shared_by_xxx_avail in the channels for the sysfs files to be created

Thanks for the help! I missed this. I'll try that :)

Yours
	-- Matti

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

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

[RFC PATCH 5/5] MAINTAINERS: Add KX022A maintainer entry

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 1017 bytes --]

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

Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
---
 MAINTAINERS | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index d30f26e07cd3..74f1c75775b5 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -11355,6 +11355,11 @@ F:	drivers/mfd/khadas-mcu.c
 F:	include/linux/mfd/khadas-mcu.h
 F:	drivers/thermal/khadas_mcu_fan.c
 
+KIONIX/ROHM KX022A ACCELEROMETER
+R:	Matti Vaittinen <mazziesaccount@gmail.com>
+S:	Supported
+F:	drivers/iio/accel/kionix-kx022a*
+
 KMEMLEAK
 M:	Catalin Marinas <catalin.marinas@arm.com>
 S:	Maintained
-- 
2.37.1


-- 
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND

~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
Simon says - in Latin please.
~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
Thanks to Simon Glass for the translation =] 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]