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

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

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 3158 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-rc7. 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.

Changelog v2: (More detailed log in individual patches)
	- dt-bindings: fixed as suggested by Krzysztof
	- dt-bindings: support both INT1 and INT2
	- KX022A: multiple fixes as suggested by Jonathan
	- fixed SPI driver after some testing

---

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     |   67 +
 MAINTAINERS                                   |    5 +
 drivers/iio/accel/Kconfig                     |   21 +
 drivers/iio/accel/Makefile                    |    3 +
 drivers/iio/accel/kionix-kx022a-i2c.c         |   53 +
 drivers/iio/accel/kionix-kx022a-spi.c         |   59 +
 drivers/iio/accel/kionix-kx022a.c             | 1138 +++++++++++++++++
 drivers/iio/accel/kionix-kx022a.h             |   81 ++
 drivers/regulator/devres.c                    |  164 +++
 include/linux/regulator/consumer.h            |   27 +
 10 files changed, 1618 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


base-commit: f76349cf41451c5c42a99f18a9163377e4b364ff
-- 
2.37.3


-- 
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 v2 1/5] regulator: Add devm helpers for get and enable

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 4521 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)

---

Already in Mark's regulator tree. Not to be merged. Included just for
the sake of the completeness. Will be dropped when series is rebased on
top of the 6.1-rc1
---
 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.3


-- 
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 v2 2/5] regulator: Add devm helpers for get and enable

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 6658 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>

---
Already in Mark's regulator tree. Not to be merged. Included just for
the sake of the completeness. Will be dropped when series is rebased on
top of the 6.1-rc1
---
 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.3


-- 
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 v2 3/5] dt-bindings: iio: Add KX022A accelerometer

[Matti Vaittinen]

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

KX022A is a 3-axis Accelerometer from ROHM/Kionix. The sensor 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 features.

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 when rest of the features are supported.

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

---
RFCv1 => v2:
Based on a review by Krzysztof:
- fix a typo from commit message
- const compatible
- drop unnecessary descriptions/words
- io_vdd-supply => io-vdd-supply
- fix the binding example indentiation
Also,
- change my email address
- support both INT pins
---
 .../bindings/iio/accel/kionix,kx022a.yaml     | 67 +++++++++++++++++++
 1 file changed, 67 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..2919c436e46f
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/accel/kionix,kx022a.yaml
@@ -0,0 +1,67 @@
+# 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
+
+maintainers:
+  - Matti Vaittinen <mazziesaccount@gmail.com>
+
+description: |
+  KX022A is a 3-axis accelerometer supporting +/- 2G, 4G, 8G and 16G ranges,
+  output data-rates from 0.78Hz to 1600Hz and a hardware-fifo buffering.
+  KX022A can be accessed either via I2C or SPI.
+
+properties:
+  compatible:
+    const: kionix,kx022a
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    minItems: 1
+    maxItems: 2
+
+  interrupt-names:
+    minItems: 1
+    maxItems: 2
+    items:
+      enum:
+        - INT1
+        - INT2
+
+  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>;
+            interrupt-names = "INT1";
+
+            io-vdd-supply = <&iovdd>;
+            vdd-supply = <&vdd>;
+        };
+    };
-- 
2.37.3


-- 
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 v2 4/5] iio: accel: Support Kionix/ROHM KX022A accelerometer

[Matti Vaittinen]

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

KX022A is a 3-axis accelerometer from ROHM/Kionix. The senosr 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 features.

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 the amount of data in FIFO was
reported to be 0xff bytes, which equals to 42,5 samples. Specification
says the FIFO can hold a maximum of 41 samples in HiRes mode. Also, at
least once the FIFO was stuck in a state where reading data from
hardware 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>

---
RFCv1 => v2 (mostly based on feedback from Jonathan):
- Fix bunch of typos from the commit message.
- Add missing break; to the kx022a_write_raw()
- Fix SPI driver to use of_match_table
- Fix indentiation in I2C driver
- Drop struct kx022a_trigger
- Drop cross references from Kconfig
- Use /* */ also in file header comments
- Misc minor styling
- Do sensor-reset at probe
- Support both IRQ pins
- Implement read_avail callback
- Use dma aligned buffers for bulk-reads
- Use iio_trigger_poll_chained()
- Use devm consistently
- Drop inclusion of device.h
- Add SPI device ID for module loading
- Add module param for hw fifo / watermark IRQ usage
- Fix io-vdd-supply name to match one in the bindings
---
 drivers/iio/accel/Kconfig             |   21 +
 drivers/iio/accel/Makefile            |    3 +
 drivers/iio/accel/kionix-kx022a-i2c.c |   53 ++
 drivers/iio/accel/kionix-kx022a-spi.c |   59 ++
 drivers/iio/accel/kionix-kx022a.c     | 1138 +++++++++++++++++++++++++
 drivers/iio/accel/kionix-kx022a.h     |   81 ++
 6 files changed, 1355 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..43f1090e8c59 100644
--- a/drivers/iio/accel/Kconfig
+++ b/drivers/iio/accel/Kconfig
@@ -409,6 +409,27 @@ 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.
+
+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.
+
 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..6dd5b4a56401
--- /dev/null
+++ b/drivers/iio/accel/kionix-kx022a-i2c.c
@@ -0,0 +1,53 @@
+// 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);
+}
+
+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..59944b7b49b3
--- /dev/null
+++ b/drivers/iio/accel/kionix-kx022a-spi.c
@@ -0,0 +1,59 @@
+// 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);
+}
+
+static const struct spi_device_id kx022a_id[] = {
+	{ "kx022a" },
+	{ }
+};
+MODULE_DEVICE_TABLE(spi, kx022a_id);
+
+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",
+		.of_match_table = kx022a_of_match,
+	},
+	.probe = kx022a_spi_probe,
+	.id_table = kx022a_id,
+};
+
+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..37d47def5927
--- /dev/null
+++ b/drivers/iio/accel/kionix-kx022a.c
@@ -0,0 +1,1138 @@
+// 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/moduleparam.h>
+#include <linux/mutex.h>
+#include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
+#include <linux/slab.h>
+#include <linux/units.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 KX022A_SOFT_RESET_WAIT_TIME_US (5 * KILO)
+#define KX022A_SOFT_RESET_TOTAL_WAIT_TIME_US (500 * KILO)
+
+static bool g_kx022a_use_buffer;
+
+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,
+	}, {
+		/* The reset bit will be cleared by sensor */
+		.range_min = KX022A_REG_CNTL2,
+		.range_max = KX022A_REG_CNTL2,
+	}, {
+		.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_data {
+	int irq;
+	int inc_reg;
+	int ien_reg;
+	struct regmap *regmap;
+	struct iio_trigger *trig;
+	bool trigger_enabled;
+
+	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] __aligned(IIO_DMA_MINALIGN);
+	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),	\
+		.info_mask_shared_by_type_available =			\
+					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,				\
+			.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 const int kx022a_accel_samp_freq_table[][2] = {
+	[0] = { 0, 780000 },
+	[1] = { 1, 563000 },
+	[2] = { 3, 125000 },
+	[3] = { 6, 250000 },
+	[4] = { 12, 500000 },
+	[5] = { 25, 0 },
+	[6] = { 50, 0 },
+	[7] = { 100, 0 },
+	[8] = { 200, 0 }
+};
+
+static const unsigned int kx022a_odrs[] = { 1282051282, 639795266, 320 * MEGA,
+	 160 * MEGA, 80 * MEGA, 40 * MEGA, 20 * MEGA, 10 * MEGA, 5 * MEGA };
+
+/*
+ * 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 int kx022a_scale_table[][2] = {
+	[0] = { 598, 550415 },
+	[1] = { 1197, 100830 },
+	[2] = { 2394, 201660 },
+	[3] = { 4788, 403320 }
+};
+
+static int kx022a_read_avail(struct iio_dev *indio_dev,
+			     struct iio_chan_spec const *chan,
+			     const int **vals, int *type, int *length,
+			     long mask)
+{
+	switch (mask) {
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		*vals = (const int *)kx022a_accel_samp_freq_table;
+		*length = ARRAY_SIZE(kx022a_accel_samp_freq_table) * 2;
+		*type = IIO_VAL_INT_PLUS_MICRO;
+		return IIO_AVAIL_LIST;
+	case IIO_CHAN_INFO_SCALE:
+		*vals = (const int *)kx022a_scale_table;
+		*length = ARRAY_SIZE(kx022a_scale_table) * 2;
+		*type = IIO_VAL_INT_PLUS_MICRO;
+		return IIO_AVAIL_LIST;
+	default:
+		return -EINVAL;
+	}
+}
+
+#define KX022A_DEFAULT_PERIOD_NS (20 * MEGA)
+
+static void kx022a_reg2freq(unsigned int val,  int *val1, int *val2)
+{
+	*val1 = kx022a_accel_samp_freq_table[val & KX022A_MASK_ODR][0];
+	*val2 = kx022a_accel_samp_freq_table[val & KX022A_MASK_ODR][1];
+}
+
+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][0];
+	*val2 = kx022a_scale_table[val][1];
+}
+
+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).
+	 */
+	ret = iio_device_claim_direct_mode(idev);
+	if (ret)
+		return ret;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_SAMP_FREQ:
+	{
+		int n = ARRAY_SIZE(kx022a_accel_samp_freq_table);
+
+		while (n-- > 0)
+			if (val == kx022a_accel_samp_freq_table[n][0] &&
+			    kx022a_accel_samp_freq_table[n][1] == val2)
+				break;
+		if (n < 0) {
+			ret = -EINVAL;
+			goto unlock_out;
+		}
+		ret = kx022a_turn_off_lock(data);
+		if (ret)
+			break;
+
+		ret = regmap_update_bits(data->regmap,
+					 KX022A_REG_ODCNTL,
+					 KX022A_MASK_ODR, n);
+		data->odr_ns = kx022a_odrs[n];
+		kx022a_turn_on_unlock(data);
+		break;
+	}
+	case IIO_CHAN_INFO_SCALE:
+	{
+		int n = ARRAY_SIZE(kx022a_scale_table);
+
+		while (n-- > 0)
+			if (val == kx022a_scale_table[n][0] &&
+			    kx022a_scale_table[n][1] == val2)
+				break;
+		if (n < 0) {
+			ret = -EINVAL;
+			goto unlock_out;
+		}
+
+		ret = kx022a_turn_off_lock(data);
+		if (ret)
+			break;
+
+		ret = regmap_update_bits(data->regmap, KX022A_REG_CNTL,
+					 KX022A_MASK_GSEL,
+					 n << KX022A_GSEL_SHIFT);
+		kx022a_turn_on_unlock(data);
+		break;
+	}
+	default:
+		ret = -EINVAL;
+	}
+
+unlock_out:
+	iio_device_release_direct_mode(idev);
+
+	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_get_axis(struct kx022a_data *data,
+			   struct iio_chan_spec const *chan,
+			   int *val)
+{
+	int ret;
+
+	ret = regmap_bulk_read(data->regmap, chan->address, &data->buffer,
+			       sizeof(s16));
+	if (ret)
+		return ret;
+
+	*val = data->buffer[0];
+
+	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;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		ret = iio_device_claim_direct_mode(idev);
+		if (ret)
+			return ret;
+
+		mutex_lock(&data->mutex);
+		ret = kx022a_get_axis(data, chan, val);
+		mutex_unlock(&data->mutex);
+
+		iio_device_release_direct_mode(idev);
+		break;
+
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		ret = regmap_read(data->regmap, KX022A_REG_ODCNTL, &regval);
+		if (ret)
+			return ret;
+
+		if ((regval & KX022A_MASK_ODR) >
+		    ARRAY_SIZE(kx022a_accel_samp_freq_table)) {
+			dev_err(data->dev, "Invalid ODR\n");
+			return -EINVAL;
+		}
+
+		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)
+			return ret;
+
+		kx022a_reg2scale(regval, val, val2);
+
+		ret = IIO_VAL_INT_PLUS_MICRO;
+		break;
+	}
+
+	return ret;
+};
+
+static int kx022a_validate_trigger(struct iio_dev *idev,
+				   struct iio_trigger *trig)
+{
+	struct kx022a_data *data = iio_priv(idev);
+
+	if (data->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 overflow 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;
+	}
+
+	fifo_bytes = count * KX022A_FIFO_SAMPLES_SIZE_BYTES;
+	ret = regmap_noinc_read(data->regmap, KX022A_REG_BUF_READ,
+				buffer, fifo_bytes);
+	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,
+	.read_avail = &kx022a_read_avail,
+
+	.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_IEN | KX022A_MASK_IPOL |
+		   KX022A_MASK_ITYP;
+	int val = KX022A_MASK_IEN | KX022A_IPOL_LOW |
+		  KX022A_ITYP_LEVEL;
+	int ret;
+
+	ret = regmap_update_bits(data->regmap, data->inc_reg, mask, val);
+	if (ret)
+		return ret;
+
+	/* We enable WMI to IRQ pin only at buffer_enable */
+	mask = KX022A_MASK_INS2_DRDY /*| KX122_MASK_INS2_WMI */;
+
+	return regmap_set_bits(data->regmap, data->ien_reg, mask);
+}
+
+static int kx022a_fifo_disable(struct kx022a_data *data)
+{
+	int ret = 0;
+
+	ret = kx022a_turn_off_lock(data);
+	if (ret)
+		return ret;
+
+	ret = regmap_clear_bits(data->regmap, data->ien_reg,
+				KX022A_MASK_WMI);
+	if (ret)
+		goto unlock_out;
+
+	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;
+
+	/* Update watermark 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, data->ien_reg,
+			      KX022A_MASK_WMI);
+	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 data-ready trigger, then the IRQ masks should be handled by
+	 * trigger enable and the hardware buffer is not used but we just update
+	 * results to the IIO fifo when data-ready triggers.
+	 */
+	if (iio_device_get_current_mode(idev) == INDIO_BUFFER_TRIGGERED)
+		return 0;
+
+	/*
+	 * Filling up the HW-FIFO can cause nasty problems. Thus we do not
+	 * enable the fifo unless it is explicitly requested by a module param.
+	 * If you are _sure_ your system can serve the interrupts in time you
+	 * can enable the HW fifo. I do not recommend it for sample frequencies
+	 * higher than 2 Hz - and even in that case I would set the watermark
+	 * somewhere near 20 samples (HI-RES) to have magnitude of 10 sec
+	 * safety-margin.
+	 */
+	if (!g_kx022a_use_buffer) {
+		dev_err(data->dev, "Neither trigger set nor hw-fifo enabled\n");
+
+		return -EINVAL;
+	}
+	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;
+
+	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:
+	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);
+
+	data->old_timestamp = data->timestamp;
+	data->timestamp = iio_get_time_ns(idev);
+
+	if (data->state & KX022A_STATE_FIFO || data->trigger_enabled)
+		return IRQ_WAKE_THREAD;
+
+	return IRQ_NONE;
+}
+
+/*
+ * 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).
+ */
+static irqreturn_t kx022a_irq_thread_handler(int irq, void *private)
+{
+	struct iio_dev *idev = private;
+	struct kx022a_data *data = iio_priv(idev);
+	int ret = IRQ_NONE;
+
+	mutex_lock(&data->mutex);
+
+	if (data->trigger_enabled) {
+		iio_trigger_poll_chained(data->trig);
+		ret = IRQ_HANDLED;
+	}
+
+	if (data->state & KX022A_STATE_FIFO) {
+		ret = __kx022a_fifo_flush(idev, KX022A_FIFO_LENGTH, true);
+		if (ret > 0)
+			ret = IRQ_HANDLED;
+	}
+
+	mutex_unlock(&data->mutex);
+
+	return ret;
+}
+
+static int kx022a_trigger_set_state(struct iio_trigger *trig,
+				    bool state)
+{
+	struct kx022a_data *data = iio_trigger_get_drvdata(trig);
+	int ret = 0;
+
+	mutex_lock(&data->mutex);
+
+	if (data->trigger_enabled == state)
+		goto unlock_out;
+
+	ret = __kx022a_turn_off_unlocked(data);
+	if (ret)
+		goto unlock_out;
+
+	data->trigger_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 const struct iio_trigger_ops kx022a_trigger_ops = {
+	.set_trigger_state = kx022a_trigger_set_state,
+};
+
+static int kx022a_chip_init(struct kx022a_data *data)
+{
+	int ret, val;
+
+	/* Reset the senor */
+	ret = regmap_write(data->regmap, KX022A_REG_CNTL2, KX022A_MASK_SRST);
+	if (ret)
+		return ret;
+
+	/*
+	 * I've seen I2C read failures if we poll too fast after the sensor
+	 * reset. Slight delay gives I2C block the time to recover.
+	 */
+	msleep(1);
+
+	ret = regmap_read_poll_timeout(data->regmap, KX022A_REG_CNTL2, val,
+				       !(val & KX022A_MASK_SRST),
+				       KX022A_SOFT_RESET_WAIT_TIME_US,
+				       KX022A_SOFT_RESET_TOTAL_WAIT_TIME_US);
+	if (ret) {
+		dev_err(data->dev, "Sensor reset %s\n",
+			val & KX022A_MASK_SRST ? "timeout" : "fail#");
+		return ret;
+	}
+
+	ret = regmap_reinit_cache(data->regmap, &kx022a_regmap);
+	if (ret) {
+		dev_err(data->dev, "Failed to reinit reg cache\n");
+		return ret;
+	}
+
+	/* set data res 16bit */
+	ret = regmap_set_bits(data->regmap, KX022A_REG_BUF_CNTL2,
+			      KX022A_MASK_BRES16);
+	if (ret) {
+		dev_err(data->dev, "Failed to set data resolution\n");
+		return ret;
+	}
+
+	return kx022a_prepare_irq_pin(data);
+}
+
+int kx022a_probe_internal(struct device *dev)
+{
+	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, irq;
+
+	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_probe(dev, ret, "Failed to access sensor\n");
+		return ret;
+	}
+
+	if (chip_id != KX022A_ID) {
+		dev_err(dev, "unsupported device 0x%x\n", chip_id);
+		return -EINVAL;
+	}
+
+	irq = fwnode_irq_get_byname(dev_fwnode(dev), "INT1");
+	if (irq > 0) {
+		data->inc_reg = KX022A_REG_INC1;
+		data->ien_reg = KX022A_REG_INC4;
+	} else {
+		irq = fwnode_irq_get_byname(dev_fwnode(dev), "INT2");
+		if (irq < 0)
+			return dev_err_probe(dev, irq, "No suitable IRQ\n");
+
+		data->inc_reg = KX022A_REG_INC5;
+		data->ien_reg = KX022A_REG_INC6;
+	}
+
+	data->regmap = regmap;
+	data->dev = dev;
+	data->irq = irq;
+	data->odr_ns = KX022A_DEFAULT_PERIOD_NS;
+	mutex_init(&data->mutex);
+
+	idev->channels = kx022a_channels;
+	idev->num_channels = ARRAY_SIZE(kx022a_channels);
+	idev->name = "kx022-accel";
+	idev->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) {
+		mutex_unlock(&data->mutex);
+		return ret;
+	}
+
+	ret = kx022a_turn_on_unlock(data);
+	if (ret)
+		return ret;
+
+	udelay(100);
+	ret = devm_iio_triggered_buffer_setup_ext(dev, 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->trig = indio_trig;
+
+	indio_trig->ops = &kx022a_trigger_ops;
+	iio_trigger_set_drvdata(indio_trig, data);
+
+	ret = devm_iio_trigger_register(dev, 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_param(g_kx022a_use_buffer, bool, 0);
+MODULE_PARM_DESC(g_kx022a_use_buffer,
+		 "Buffer samples. Use at own risk. Fifo must not overflow");
+
+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..0899fd52fa55
--- /dev/null
+++ b/drivers/iio/accel/kionix-kx022a.h
@@ -0,0 +1,81 @@
+/* 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/regmap.h>
+
+#define KX022A_REG_WHO		0x0f
+#define KX022A_ID		0xc8
+
+#define KX022A_REG_CNTL2	0x19
+#define KX022A_MASK_SRST	BIT(7)
+#define KX022A_REG_CNTL		0x18
+#define KX022A_MASK_PC1		BIT(7)
+#define KX022A_MASK_RES		BIT(6)
+#define KX022A_MASK_DRDY	BIT(5)
+#define KX022A_MASK_GSEL	GENMASK(4, 3)
+#define KX022A_GSEL_SHIFT	3
+#define KX022A_GSEL_2		0x0
+#define KX022A_GSEL_4		BIT(3)
+#define KX022A_GSEL_8		BIT(4)
+#define KX022A_GSEL_16		GENMASK(4, 3)
+
+#define KX022A_REG_INS2		0x13
+#define KX022A_MASK_INS2_DRDY	BIT(4)
+#define KX122_MASK_INS2_WMI	BIT(5)
+
+#define KX022A_REG_XHP_L	0x0
+#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_BRES16	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_REG_INC5		0x20
+#define KX022A_REG_INC6		0x21
+#define KX022A_MASK_IEN		BIT(5)
+#define KX022A_MASK_IPOL	BIT(4)
+#define KX022A_IPOL_LOW		0
+#define KX022A_IPOL_HIGH	KX022A_MASK_IPOL1
+#define KX022A_MASK_ITYP	BIT(3)
+#define KX022A_ITYP_PULSE	KX022A_MASK_ITYP
+#define KX022A_ITYP_LEVEL	0
+
+#define KX022A_REG_INC4		0x1f
+#define KX022A_MASK_WMI		BIT(5)
+
+#define KX022A_REG_SELF_TEST	0x60
+#define KX022A_MAX_REGISTER	0x60
+
+struct device;
+
+int kx022a_probe_internal(struct device *dev);
+extern const struct regmap_config kx022a_regmap;
+
+#endif
-- 
2.37.3


-- 
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: 484 bytes --]

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

[Matti Vaittinen]

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

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

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

---
I can also add myself as a maintainer instead of a reviewer if it better
suits iio maintainer. I however don't plan setting up my own public
repository and hope the further patches will be merged via IIO tree.

So, as Geert once explained to me - In that case the difference between
me as a maintainer vs. a reviewer would be only really relevant to the
subsystem (in this case IIO) maintainer. The subsystem maintainer who
merges patches is allowed to take in changes acked by downstream
maintainer w/o obligation to do thorough review. (Downstream maintainer is
to be blamed if things explode :]). If ack is given by a reviewer, then
the subsystem maintainer has the full responsibility and should always
do the review. Or - this is how I remember our discussion went - feel
free to correct me if I am wrong :] In any case - please let me know if
you'd rather see M: not R: in front of my name for the kx022a.
---
 MAINTAINERS | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index f5ca4aefd184..641b4fc2e5e2 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -11356,6 +11356,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.3


-- 
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 v2 3/5] dt-bindings: iio: Add KX022A accelerometer

[Krzysztof Kozlowski]

On 06/10/2022 16:37, Matti Vaittinen wrote:
> KX022A is a 3-axis Accelerometer from ROHM/Kionix. The sensor 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 features.
> 

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

> +
> +properties:
> +  compatible:
> +    const: kionix,kx022a
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    minItems: 1
> +    maxItems: 2
> +
> +  interrupt-names:
> +    minItems: 1
> +    maxItems: 2
> +    items:
> +      enum:
> +        - INT1
> +        - INT2

This allows any order, which I assume was your intention. However maybe
at least fix it a bit like:
minItems: 1
items:
  - enum: [ int1, int2]
  - const: int2

OTOH, the names are not really descriptive. Do they match pin names?


Best regards,
Krzysztof

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

[Matti Vaittinen]

Hi dee Ho Krzysztof,

On 10/6/22 18:23, Krzysztof Kozlowski wrote:
> On 06/10/2022 16:37, Matti Vaittinen wrote:
>> KX022A is a 3-axis Accelerometer from ROHM/Kionix. The sensor 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 features.
>>
> 
> Thank you for your patch. There is something to discuss/improve.
> 
>> +
>> +properties:
>> +  compatible:
>> +    const: kionix,kx022a
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  interrupts:
>> +    minItems: 1
>> +    maxItems: 2
>> +
>> +  interrupt-names:
>> +    minItems: 1
>> +    maxItems: 2
>> +    items:
>> +      enum:
>> +        - INT1
>> +        - INT2
> 
> This allows any order, which I assume was your intention.

Yes. I don't see real need to restrict ordering - besides, with my 
yaml/schema skills it'd took eternity to find corrct example(s) ;)

My intention is that the user can give either one of these - or both. 
Order needs naturally to match the order of IRQs - but this we can't know.

> However maybe
> at least fix it a bit like:
> minItems: 1
> items:
>    - enum: [ int1, int2]
>    - const: int2

If you say so XD
I can fix this for v3 :)

> 
> OTOH, the names are not really descriptive. Do they match pin names?
> 

Yes. They match to pin names used in the data-sheet.


Yours
	-- Matti

Ps. I've heard this year has not been that good with the mushrooms here 
in Finland ;)

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

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

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

[Andy Shevchenko]

On Thu, Oct 06, 2022 at 05:36:52PM +0300, Matti Vaittinen wrote:
> 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()

...

> (cherry picked from commit b6058e052b842a19c8bb639798d8692cd0e7589f)

Not sure:
 - why this is in the commit message
 - what it points to, since
$ git show b6058e052b842a19c8bb639798d8692cd0e7589f
 fatal: bad object b6058e052b842a19c8bb639798d8692cd0e7589f

> Already in Mark's regulator tree. Not to be merged. Included just for
> the sake of the completeness. Will be dropped when series is rebased on
> top of the 6.1-rc1

Ah, I see, but does it mean the commit has been rebased or you used wrong SHA?

...

> +static void regulator_action_disable(void *d)
> +{
> +	struct regulator *r = (struct regulator *)d;

Cast is not needed.

> +	regulator_disable(r);
> +}

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Thu, Oct 06, 2022 at 05:38:14PM +0300, Matti Vaittinen wrote:
> KX022A is a 3-axis accelerometer from ROHM/Kionix. The senosr 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 features.
> 
> 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 the amount of data in FIFO was
> reported to be 0xff bytes, which equals to 42,5 samples. Specification
> says the FIFO can hold a maximum of 41 samples in HiRes mode. Also, at
> least once the FIFO was stuck in a state where reading data from
> hardware 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.

...

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

Why it tending to get user to choose Y? It might increase the footprint of
bootable image and on the embedded platforms it may increase the chances
to not fit into (flash) ROM.

Also what will be the module name (see other Kconfig for the pattern)?

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

Ditto.

...

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

> +

Redundant blank line.

> +module_i2c_driver(kx022a_i2c_driver);

...

> +static int kx022a_spi_probe(struct spi_device *spi)
> +{
> +	struct device *dev = &spi->dev;
> +	struct regmap *regmap;

Interestingly in the I²C driver you have other style (and I prefer SPI one over
that). Can you fix I²C driver to follow this style?

> +	}

Ditto (blank line).

> +	return kx022a_probe_internal(dev);
> +}

...

> +static struct spi_driver kx022a_spi_driver = {
> +	.driver = {
> +		.name   = "kx022a-spi",
> +		.of_match_table = kx022a_of_match,
> +	},
> +	.probe = kx022a_spi_probe,
> +	.id_table = kx022a_id,
> +};

> +

Redundant blank line.

> +module_spi_driver(kx022a_spi_driver);

...

> +#include <linux/delay.h>

> +#include <linux/gpio.h>

No way. The linux/gpio.h  mustn't appear in the new code.

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

Can this group be placed after units.h (with blank line in between)?

> +#include <linux/interrupt.h>
> +#include <linux/module.h>
> +#include <linux/moduleparam.h>
> +#include <linux/mutex.h>
> +#include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/slab.h>
> +#include <linux/units.h>
> +
> +#include "kionix-kx022a.h"

...

> +/*
> + * Threshold for deciding our HW fifo is unrecoverably corrupt and should be
> + * cleared

Multi-line comments have to follow English grammar and punctuation,
i.e. trailing period.

> + */
> +#define KXO22A_FIFO_ERR_THRESHOLD 10
> +#define KX022A_FIFO_LENGTH 41

Why not to use TAB between definitions and values (also for the rest similar
cases)?

...

> +#define KX022A_FIFO_MAX_BYTES (KX022A_FIFO_LENGTH *			\
> +			       KX022A_FIFO_SAMPLES_SIZE_BYTES)

Slightly better to read:

#define KX022A_FIFO_MAX_BYTES					\
	(KX022A_FIFO_LENGTH * KX022A_FIFO_SAMPLES_SIZE_BYTES)

...

> +#define KX022A_SOFT_RESET_WAIT_TIME_US (5 * KILO)
> +#define KX022A_SOFT_RESET_TOTAL_WAIT_TIME_US (500 * KILO)

For time we have different macros like USEC_PER_MSEC (look into vdso/time64.h).

...

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

No max register? Have you tried debugfs output?

> +};
> +EXPORT_SYMBOL_GPL(kx022a_regmap);

Can we use namespace from day 1?

...

> +struct kx022a_data;

Why?

> +struct kx022a_data {
> +	int irq;
> +	int inc_reg;
> +	int ien_reg;

> +	struct regmap *regmap;

Putting this as a first member might improve code by making pointer arithmetics
better. Have you checked possibilities with bloat-o-meter?

> +	struct iio_trigger *trig;
> +	bool trigger_enabled;
> +
> +	struct device *dev;
> +	unsigned int g_range;

> +	struct mutex mutex;

No warning from checkpatch? Every lock should be commented what it is for.

> +	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] __aligned(IIO_DMA_MINALIGN);
> +	struct {
> +		__le16 channels[3];
> +		s64 ts __aligned(8);
> +	} scan;
> +};

...

> +enum {
> +	AXIS_X,
> +	AXIS_Y,
> +	AXIS_Z,
> +	AXIS_MAX,

No comma for terminator entry.

> +};

...

> +static const unsigned long kx022a_scan_masks[] = {
> +					BIT(AXIS_X) | BIT(AXIS_Y) | BIT(AXIS_Z),
> +					0};

Seems broken indentation. The }; is better on a new line.

...

> +static const struct iio_chan_spec_ext_info kx022a_ext_info[] = {
> +	IIO_MOUNT_MATRIX(IIO_SHARED_BY_TYPE, kx022a_get_mount_matrix),

> +	{ },

No comma for the terminator entry.

> +};

...

> +#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),	\
> +		.info_mask_shared_by_type_available =			\
> +					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,				\
> +			.endianness = IIO_LE,				\
> +		},							\
> +	}

It's fine to indent left by one TAB.

...

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

Please, check the punctuation.

> + */

...

> +static const int kx022a_accel_samp_freq_table[][2] = {
> +	[0] = { 0, 780000 },
> +	[1] = { 1, 563000 },
> +	[2] = { 3, 125000 },
> +	[3] = { 6, 250000 },
> +	[4] = { 12, 500000 },
> +	[5] = { 25, 0 },
> +	[6] = { 50, 0 },
> +	[7] = { 100, 0 },
> +	[8] = { 200, 0 }

What do you need the indices for? They are not defines, so are you expecting to
shuffle the entries?

> +};

...

> +static const unsigned int kx022a_odrs[] = { 1282051282, 639795266, 320 * MEGA,
> +	 160 * MEGA, 80 * MEGA, 40 * MEGA, 20 * MEGA, 10 * MEGA, 5 * MEGA };

{ and }; on the new line. And I would suggest to group by 4 or 8, that makes it
easier to read / count.

...

> +#define KX022A_DEFAULT_PERIOD_NS (20 * MEGA)

NSEC_PER_MSEC ?

...

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

With making this a single function with a boolean parameter you may utilise

	if (on)
		ret = set
	else
		ret = clear
	if (ret)
		... str_on_off(on)

...

> +	ret = __kx022a_turn_off_unlocked(data);
> +	ret = __kx022a_turn_on_unlocked(data);

Actually having __ and '_unlocked' seems overkill, I would leave one of them.

...

> +		int n = ARRAY_SIZE(kx022a_accel_samp_freq_table);

You may drop {} in each case and have n defined once for all cases.

> +
> +		while (n-- > 0)

while (n--) ? Or why the 0 is ignored?

> +			if (val == kx022a_accel_samp_freq_table[n][0] &&
> +			    kx022a_accel_samp_freq_table[n][1] == val2)
> +				break;

> +		if (n < 0) {

How is it even possible with your conditional?

> +			ret = -EINVAL;
> +			goto unlock_out;
> +		}

...

> +	default:
> +		ret = -EINVAL;

break; ?

...

> +static int kx022a_get_axis(struct kx022a_data *data,
> +			   struct iio_chan_spec const *chan,
> +			   int *val)
> +{
> +	int ret;
> +
> +	ret = regmap_bulk_read(data->regmap, chan->address, &data->buffer,
> +			       sizeof(s16));

No endianess awareness (sizeof __le16 / __be16) ?

> +	if (ret)
> +		return ret;
> +
> +	*val = data->buffer[0];

Ditto (get_unaligned_be16/le16 / le16/be16_to_cpup()).

> +	return IIO_VAL_INT;
> +}

...

> +static int kx022a_validate_trigger(struct iio_dev *idev,
> +				   struct iio_trigger *trig)
> +{
> +	struct kx022a_data *data = iio_priv(idev);
> +
> +	if (data->trig == trig)
> +		return 0;
> +
> +	return -EINVAL;

Usual pattern is to check for error condition first:

	if (... != trig)
		return -E...

	return 0;

> +}

...

> +	if (val > KX022A_FIFO_LENGTH)
> +		val = KX022A_FIFO_LENGTH;

max() from minmax.h?

...

> +	return sprintf(buf, "%d\n", state);

Documentation states it must be sysfs_emit().
Ditto for the rest of the similar cases.

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

_ATTR_RO ?

...

> +static const struct attribute *kx022a_fifo_attributes[] = {
> +	&iio_dev_attr_hwfifo_watermark.dev_attr.attr,
> +	&iio_dev_attr_hwfifo_enabled.dev_attr.attr,
> +	NULL,

No comma for terminator entry.

> +};

...

> +	/*
> +	 * We must clear the old time-stamp to avoid computing the timestamps
> +	 * based on samples acquired when buffer was last enabled

Period!

> +	 */

...

> +	return regmap_write(data->regmap, KX022A_REG_BUF_CLEAR, 0xff);

GENMASK() ?

...

> +	ret = regmap_read(data->regmap, KX022A_REG_BUF_STATUS_1, &fifo_bytes);
> +	if (ret < 0) {

Is ' < 0' part consistent with the rest of similar checks?

> +		dev_err(dev, "Error reading buffer status\n");
> +		return ret;
> +	}

...

> +	if (data->old_timestamp) {
> +		sample_period = (data->timestamp - data->old_timestamp);

Too many parentheses.

> +		do_div(sample_period, count);
> +	} else {
> +		sample_period = data->odr_ns;
> +	}

...

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

I would put redundant 'else' here to have them on the same column, but
it's pity we don't have regmap_assign_bits() API (or whatever name you
can come up with) to hide this kind of code.

> +	return regmap_clear_bits(data->regmap, KX022A_REG_CNTL,
> +				 KX022A_MASK_DRDY);
> +}

...

> +static int kx022a_fifo_enable(struct kx022a_data *data)
> +{
> +	int ret = 0;

Redundant assignment.

> +	ret = kx022a_turn_off_lock(data);
> +	if (ret)
> +		return ret;

> +}

...

> +	if (!g_kx022a_use_buffer) {
> +		dev_err(data->dev, "Neither trigger set nor hw-fifo enabled\n");

> +

No need in blank line in such cases.

> +		return -EINVAL;
> +	}

Maybe you want to place it here instead?

> +	return kx022a_fifo_enable(data);

...

> +	/*
> +	 * I've seen I2C read failures if we poll too fast after the sensor
> +	 * reset. Slight delay gives I2C block the time to recover.
> +	 */
> +	msleep(1);

msleep(1) is not doing what you think it is. I recommend to use usleep_range()
here.

...

> +int kx022a_probe_internal(struct device *dev)
> +{
> +	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, irq;

> +	if (WARN_ON(!dev))

Huh?! This can be easily transformed to panic followed by oops. Why is it
necessary to do so?

> +		return -ENODEV;
> +
> +	regmap = dev_get_regmap(dev, NULL);
> +	if (!regmap) {
> +		dev_err(dev, "no regmap\n");

> +

No blank line here as it's usual pattern in ->probe().

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

You know what...

> +	 */
> +	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_probe(dev, ret, "Failed to access sensor\n");
> +		return ret;

If you are going to use that, use in a form of

	return dev_err_probe(...);

> +	}
> +
> +	if (chip_id != KX022A_ID) {
> +		dev_err(dev, "unsupported device 0x%x\n", chip_id);
> +		return -EINVAL;
> +	}
> +
> +	irq = fwnode_irq_get_byname(dev_fwnode(dev), "INT1");
> +	if (irq > 0) {
> +		data->inc_reg = KX022A_REG_INC1;
> +		data->ien_reg = KX022A_REG_INC4;
> +	} else {
> +		irq = fwnode_irq_get_byname(dev_fwnode(dev), "INT2");
> +		if (irq < 0)
> +			return dev_err_probe(dev, irq, "No suitable IRQ\n");

Missed check for 0.

> +		data->inc_reg = KX022A_REG_INC5;
> +		data->ien_reg = KX022A_REG_INC6;
> +	}
> +
> +	data->regmap = regmap;
> +	data->dev = dev;
> +	data->irq = irq;
> +	data->odr_ns = KX022A_DEFAULT_PERIOD_NS;
> +	mutex_init(&data->mutex);
> +
> +	idev->channels = kx022a_channels;
> +	idev->num_channels = ARRAY_SIZE(kx022a_channels);
> +	idev->name = "kx022-accel";
> +	idev->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) {
> +		mutex_unlock(&data->mutex);
> +		return ret;
> +	}
> +
> +	ret = kx022a_turn_on_unlock(data);
> +	if (ret)
> +		return ret;
> +
> +	udelay(100);
> +	ret = devm_iio_triggered_buffer_setup_ext(dev, 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->trig = indio_trig;
> +
> +	indio_trig->ops = &kx022a_trigger_ops;
> +	iio_trigger_set_drvdata(indio_trig, data);
> +
> +	ret = devm_iio_trigger_register(dev, 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 you have more than one device, all handlers will be marked with the same
name, perhaps dev_name() is better to have here?

> +	if (ret)
> +		return dev_err_probe(data->dev, ret, "Could not request IRQ\n");
> +
> +	return ret;
> +}

To me it feels inconsistency to use dev_err_probe() in the cases when
it _might_ be a deferred probe and not use it in the cases we it won't
be a deferred probe.

...

> +module_param(g_kx022a_use_buffer, bool, 0);
> +MODULE_PARM_DESC(g_kx022a_use_buffer,
> +		 "Buffer samples. Use at own risk. Fifo must not overflow");

Why?! We usually do not allow module parameters in the new code.

...

> +#include <linux/regmap.h>

Missed bits.h.

You probably wanted to have

struct device;

as well.

-- 
With Best Regards,
Andy Shevchenko

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

[Joe Perches]

On Thu, 2022-10-06 at 21:32 +0300, Andy Shevchenko wrote:
> On Thu, Oct 06, 2022 at 05:38:14PM +0300, Matti Vaittinen wrote:
> > KX022A is a 3-axis accelerometer from ROHM/Kionix. The senosr 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 features.
[]
> > +/*
> > + * Threshold for deciding our HW fifo is unrecoverably corrupt and should be
> > + * cleared
> 
> Multi-line comments have to follow English grammar and punctuation,
> i.e. trailing period.

I disagree.  I think that would be a silly rule to enforce.

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

[Andy Shevchenko]

On Fri, Oct 07, 2022 at 12:04:20AM -0700, Joe Perches wrote:
> On Thu, 2022-10-06 at 21:32 +0300, Andy Shevchenko wrote:
> > On Thu, Oct 06, 2022 at 05:38:14PM +0300, Matti Vaittinen wrote:
> > > KX022A is a 3-axis accelerometer from ROHM/Kionix. The senosr 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 features.
> []
> > > +/*
> > > + * Threshold for deciding our HW fifo is unrecoverably corrupt and should be
> > > + * cleared
> > 
> > Multi-line comments have to follow English grammar and punctuation,
> > i.e. trailing period.
> 
> I disagree.  I think that would be a silly rule to enforce.

https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#coding-style-notes

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Fri, Oct 07, 2022 at 12:04:20AM -0700, Joe Perches wrote:
> On Thu, 2022-10-06 at 21:32 +0300, Andy Shevchenko wrote:
> > On Thu, Oct 06, 2022 at 05:38:14PM +0300, Matti Vaittinen wrote:
> > > KX022A is a 3-axis accelerometer from ROHM/Kionix. The senosr 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 features.
> []
> > > +/*
> > > + * Threshold for deciding our HW fifo is unrecoverably corrupt and should be
> > > + * cleared
> > 
> > Multi-line comments have to follow English grammar and punctuation,
> > i.e. trailing period.
> 
> I disagree.  I think that would be a silly rule to enforce.

First, you have to read that submission and find at least inconsistency between
styles used for different multi-line comment blocks.

-- 
With Best Regards,
Andy Shevchenko

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

[Jonathan Cameron]

On Thu, 6 Oct 2022 19:17:39 +0300
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> On Thu, Oct 06, 2022 at 05:36:52PM +0300, Matti Vaittinen wrote:
> > 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()  
> 
> ...
> 
> > (cherry picked from commit b6058e052b842a19c8bb639798d8692cd0e7589f)  
> 
> Not sure:
>  - why this is in the commit message
>  - what it points to, since
> $ git show b6058e052b842a19c8bb639798d8692cd0e7589f
>  fatal: bad object b6058e052b842a19c8bb639798d8692cd0e7589f

These are now upstream in Linus' tree and in my testing branch.
I'd not normally advocate working on top of that (because I rebase it), but
if it is useful for this series go ahead.

Jonathan
 
> 
> > Already in Mark's regulator tree. Not to be merged. Included just for
> > the sake of the completeness. Will be dropped when series is rebased on
> > top of the 6.1-rc1  
> 
> Ah, I see, but does it mean the commit has been rebased or you used wrong SHA?
> 
> ...
> 
> > +static void regulator_action_disable(void *d)
> > +{
> > +	struct regulator *r = (struct regulator *)d;  
> 
> Cast is not needed.
> 
> > +	regulator_disable(r);
> > +}  
> 

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

[Jonathan Cameron]

On Thu, 6 Oct 2022 18:32:22 +0300
Matti Vaittinen <mazziesaccount@gmail.com> wrote:

> Hi dee Ho Krzysztof,
> 
> On 10/6/22 18:23, Krzysztof Kozlowski wrote:
> > On 06/10/2022 16:37, Matti Vaittinen wrote:  
> >> KX022A is a 3-axis Accelerometer from ROHM/Kionix. The sensor 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 features.
> >>  
> > 
> > Thank you for your patch. There is something to discuss/improve.
> >   
> >> +
> >> +properties:
> >> +  compatible:
> >> +    const: kionix,kx022a
> >> +
> >> +  reg:
> >> +    maxItems: 1
> >> +
> >> +  interrupts:
> >> +    minItems: 1
> >> +    maxItems: 2
> >> +
> >> +  interrupt-names:
> >> +    minItems: 1
> >> +    maxItems: 2
> >> +    items:
> >> +      enum:
> >> +        - INT1
> >> +        - INT2  
> > 
> > This allows any order, which I assume was your intention.  
> 
> Yes. I don't see real need to restrict ordering - besides, with my 
> yaml/schema skills it'd took eternity to find corrct example(s) ;)
> 
> My intention is that the user can give either one of these - or both. 
> Order needs naturally to match the order of IRQs - but this we can't know.
> 
> > However maybe
> > at least fix it a bit like:
> > minItems: 1
> > items:
> >    - enum: [ int1, int2]
> >    - const: int2  
> 
> If you say so XD
> I can fix this for v3 :)
If my limited understanding is correct, one advantage of this restriction
is that we can't have

"INT1", "INT1"
though that may be prevented elsewhere...

There is no loss of useful flexibility in how Krzysztof suggested doing it
so looks like a good suggestion to me.

Jonathan

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

[Jonathan Cameron]

On Thu, 6 Oct 2022 21:32:11 +0300
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> On Thu, Oct 06, 2022 at 05:38:14PM +0300, Matti Vaittinen wrote:
> > KX022A is a 3-axis accelerometer from ROHM/Kionix. The senosr 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 features.
> > 
> > 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 the amount of data in FIFO was
> > reported to be 0xff bytes, which equals to 42,5 samples. Specification
> > says the FIFO can hold a maximum of 41 samples in HiRes mode. Also, at
> > least once the FIFO was stuck in a state where reading data from
> > hardware 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.  
> 

> ...
...



> ...
> 
> > +module_param(g_kx022a_use_buffer, bool, 0);
> > +MODULE_PARM_DESC(g_kx022a_use_buffer,
> > +		 "Buffer samples. Use at own risk. Fifo must not overflow");  
> 
> Why?! We usually do not allow module parameters in the new code.
> 

Badly broken hardware - was my suggestion.  Alternatives if there are usecases
that need to use the fifo, but it can wedge hard in a fashion that is impossible
to prevent from the driver?  My gut feeling is still drop the support entirely
with a strong comment in the code that the hardware is broken in a fashion we don't
know how to work around.

Jonathan

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

[Jonathan Cameron]

On Thu, 6 Oct 2022 17:38:34 +0300
Matti Vaittinen <mazziesaccount@gmail.com> wrote:

> Add maintainer entry for ROHM/Kionix KX022A accelerometer senor driver.
> 
> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
> 
> ---
> I can also add myself as a maintainer instead of a reviewer if it better
> suits iio maintainer. I however don't plan setting up my own public
> repository and hope the further patches will be merged via IIO tree.
> 
> So, as Geert once explained to me - In that case the difference between
> me as a maintainer vs. a reviewer would be only really relevant to the
> subsystem (in this case IIO) maintainer. The subsystem maintainer who
> merges patches is allowed to take in changes acked by downstream
> maintainer w/o obligation to do thorough review. (Downstream maintainer is
> to be blamed if things explode :]). If ack is given by a reviewer, then
> the subsystem maintainer has the full responsibility and should always
> do the review. Or - this is how I remember our discussion went - feel
> free to correct me if I am wrong :] In any case - please let me know if
> you'd rather see M: not R: in front of my name for the kx022a.


Entirely up to you.  I tend to take a look at all IIO patches but will
take a less detailed look if there is a tag from someone I've built
up trust with - doesn't matter if they are a maintainer of a particular
driver or not. I only ever look at MAINTAINERS when sending out patches,
never when reviewing except to perhaps moan that someone wasn't cc'd
who should have been!

Jonathan


> ---
>  MAINTAINERS | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index f5ca4aefd184..641b4fc2e5e2 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -11356,6 +11356,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

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

[Matti Vaittinen]

Hi Andy,

On 10/6/22 19:17, Andy Shevchenko wrote:
> On Thu, Oct 06, 2022 at 05:36:52PM +0300, Matti Vaittinen wrote:
>> 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()
> 
> ...
> 
>> (cherry picked from commit b6058e052b842a19c8bb639798d8692cd0e7589f)
> 
> Not sure:
>   - why this is in the commit message
>   - what it points to, since
> $ git show b6058e052b842a19c8bb639798d8692cd0e7589f
>   fatal: bad object b6058e052b842a19c8bb639798d8692cd0e7589f
> 
>> Already in Mark's regulator tree. Not to be merged. Included just for
>> the sake of the completeness. Will be dropped when series is rebased on
>> top of the 6.1-rc1
> 
> Ah, I see, but does it mean the commit has been rebased or you used wrong SHA?

I did probably cherry-pick this from my local development branch and not 
from Mark's tree. Sorry for the confusion. I thought people would ignore 
these first two patches when reviewing as was requested in cover-letter.

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

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

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

[Andy Shevchenko]

On Sun, Oct 09, 2022 at 01:24:21PM +0100, Jonathan Cameron wrote:
> On Thu, 6 Oct 2022 19:17:39 +0300
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> > On Thu, Oct 06, 2022 at 05:36:52PM +0300, Matti Vaittinen wrote:

...

> > > (cherry picked from commit b6058e052b842a19c8bb639798d8692cd0e7589f)  
> > 
> > Not sure:
> >  - why this is in the commit message
> >  - what it points to, since
> > $ git show b6058e052b842a19c8bb639798d8692cd0e7589f
> >  fatal: bad object b6058e052b842a19c8bb639798d8692cd0e7589f
> 
> These are now upstream in Linus' tree and in my testing branch.

I don't see them. As I pointed out the commit IDs are not in the any of the
official trees (subsystem maintainer's or Linus'). Again, read my doubts about
the above commit message.

> I'd not normally advocate working on top of that (because I rebase it), but
> if it is useful for this series go ahead.

> > > Already in Mark's regulator tree. Not to be merged. Included just for
> > > the sake of the completeness. Will be dropped when series is rebased on
> > > top of the 6.1-rc1  
> > 
> > Ah, I see, but does it mean the commit has been rebased or you used wrong SHA?

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Mon, Oct 10, 2022 at 07:13:23AM +0300, Matti Vaittinen wrote:
> On 10/6/22 19:17, Andy Shevchenko wrote:
> > On Thu, Oct 06, 2022 at 05:36:52PM +0300, Matti Vaittinen wrote:
> > > 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()

...

> > > (cherry picked from commit b6058e052b842a19c8bb639798d8692cd0e7589f)
> > 
> > Not sure:
> >   - why this is in the commit message
> >   - what it points to, since
> > $ git show b6058e052b842a19c8bb639798d8692cd0e7589f
> >   fatal: bad object b6058e052b842a19c8bb639798d8692cd0e7589f
> > 
> > > Already in Mark's regulator tree. Not to be merged. Included just for
> > > the sake of the completeness. Will be dropped when series is rebased on
> > > top of the 6.1-rc1
> > 
> > Ah, I see, but does it mean the commit has been rebased or you used wrong SHA?
> 
> I did probably cherry-pick this from my local development branch and not
> from Mark's tree. Sorry for the confusion. I thought people would ignore
> these first two patches when reviewing as was requested in cover-letter.

The solution as pointed out by LKP and which will removes the need of the noise
in email and a lot of confusions is to use --base parameter to the patch(es).

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Sun, Oct 09, 2022 at 01:33:51PM +0100, Jonathan Cameron wrote:
> On Thu, 6 Oct 2022 21:32:11 +0300
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> > On Thu, Oct 06, 2022 at 05:38:14PM +0300, Matti Vaittinen wrote:

...

> > > +module_param(g_kx022a_use_buffer, bool, 0);
> > > +MODULE_PARM_DESC(g_kx022a_use_buffer,
> > > +		 "Buffer samples. Use at own risk. Fifo must not overflow");  
> > 
> > Why?! We usually do not allow module parameters in the new code.
> 
> Badly broken hardware - was my suggestion.  Alternatives if there are usecases
> that need to use the fifo, but it can wedge hard in a fashion that is impossible
> to prevent from the driver?  My gut feeling is still drop the support entirely
> with a strong comment in the code that the hardware is broken in a fashion we don't
> know how to work around.

I also would drop this from upstream and if anybody curious, provide some kind
of GitHub gist for that. Also it needs some communication with a vendor to
clarify the things.

-- 
With Best Regards,
Andy Shevchenko

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

[Matti Vaittinen]

Hello Andy,

Thanks for taking the time and doing the review. Quite a few comments I 
do agree with. I didn't respond to those unless you asked a question :) 
Thanks for the help! OTOH, as usual, I do not fully agree with 
everything - and at some places I would like to get some further 
information :)

On 10/6/22 21:32, Andy Shevchenko wrote:
> On Thu, Oct 06, 2022 at 05:38:14PM +0300, Matti Vaittinen wrote:
>> KX022A is a 3-axis accelerometer from ROHM/Kionix. The senosr 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 features.
>>
>> 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 the amount of data in FIFO was
>> reported to be 0xff bytes, which equals to 42,5 samples. Specification
>> says the FIFO can hold a maximum of 41 samples in HiRes mode. Also, at
>> least once the FIFO was stuck in a state where reading data from
>> hardware 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.
> 
> ...
> 
>> +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.
> 
> Why it tending to get user to choose Y? It might increase the footprint of
> bootable image and on the embedded platforms it may increase the chances
> to not fit into (flash) ROM.

It may be just my English skills but to me this is really telling that 
"say Y to enable support for this IC". For me it does not mean that "Say 
Y even if you don't have this IC". I am really not seeing the problem 
here, but as I said - maybe it's just my lack of English skills.

Nevertheless, looking at the top of the IIO/accelerometer/KConfg - 
plenty of the accelerometer driver KConfig entries seem to use similar 
pattern. I am nevertheless usually willing to break the pattern if it 
seems like the right thing ;) Can anyone help me to format better 
description?

> Also what will be the module name (see other Kconfig for the pattern)?

Here the pattern is IMHO improved by cutting the useless text. Even the 
cross-reference from SPI to I2C and I2C to SPI driver which was in v1 is 
more useful. Module autoloading for SPI should just work. Adding the 
module name here seems just a silly habit - which I see many people are 
breaking. If module name is to be needed, then it should be checked from 
the Makefile and not trusted that Kconfig text stays up-to-date...

If the module name in config text is generally seen useful (which I 
doubt), then maybe there should be a script that fetches it from the 
Makefile?

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

Ditto.

> 
>> +static int kx022a_spi_probe(struct spi_device *spi)
>> +{
>> +	struct device *dev = &spi->dev;
>> +	struct regmap *regmap;
> 
> Interestingly in the I²C driver you have other style (and I prefer SPI one over
> that). Can you fix I²C driver to follow this style?
> 

I need to respin this anyways, so yes.

>> +	}
> 
> Ditto (blank line).

The blank prior function return? I think I have been told to have blanks 
preceding returns to make returns more obvious - which I kind of understand.

> ...
> 
> 
>> +#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>
> 
> Can this group be placed after units.h (with blank line in between)?

Why? What's wrong with alphabetical order?

>> + */
>> +#define KXO22A_FIFO_ERR_THRESHOLD 10
>> +#define KX022A_FIFO_LENGTH 41
> 
> Why not to use TAB between definitions and values (also for the rest similar
> cases)?

Because I don't personally see the benefit. I can still do that as I've 
heard many people prefer the aligned values.

> 
>> +#define KX022A_FIFO_MAX_BYTES (KX022A_FIFO_LENGTH *			\
>> +			       KX022A_FIFO_SAMPLES_SIZE_BYTES)
> 
> Slightly better to read:
> 
> #define KX022A_FIFO_MAX_BYTES					\
> 	(KX022A_FIFO_LENGTH * KX022A_FIFO_SAMPLES_SIZE_BYTES)
> 

Really? Well, I can change this but don't personally see any improvement.

> 
>> +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,
> 
> No max register? Have you tried debugfs output?

Can you please explain what you mean by no max register?

> 
>> +struct kx022a_data;
> 
> Why?

Leftover from a removed trigger struct. Thanks.

> 
>> +struct kx022a_data {
>> +	int irq;
>> +	int inc_reg;
>> +	int ien_reg;
> 
>> +	struct regmap *regmap;
> 
> Putting this as a first member might improve code by making pointer arithmetics
> better. Have you checked possibilities with bloat-o-meter?

No. Amount of combinations is just too huge for me to try randomly 
reordering structs and running bloat-o-meter. Can you please explain me 
why you think reordering this would have visible impact and if this is 
something that is universally true or if such an optimization is 
architecture dependent?

> 
>> +	struct iio_trigger *trig;
>> +	bool trigger_enabled;
>> +
>> +	struct device *dev;
>> +	unsigned int g_range;
> 
>> +	struct mutex mutex;
> 
> No warning from checkpatch? Every lock should be commented what it is for.

No. I didn't see a warn from checkpatch with v6.0-rc7. And I am glad I 
didn't. I am not a fan of statements like "always document a lock" or 
"never use a goto" - because blindly following such "rules" tend to lead 
silly corner cases. In lock commenting case it is likely to lead 
copy-pasting comments for locks which are used for trivial protections. 
And I am definitely not a fan of boilerplate.

>> +	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] __aligned(IIO_DMA_MINALIGN);
>> +	struct {
>> +		__le16 channels[3];
>> +		s64 ts __aligned(8);
>> +	} scan;
>> +};
> 
> ...
> 
>> +/*
>> + * 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
> 
> Please, check the punctuation.

I admit it. Punctuation rules for English grammar are something I never 
learned. Actually, in the school I was told to just use short sentences 
because correct punctuation in English is not trivial. I understand that 
grammatically correct comments are pleasing to the eye for people who 
really _know_ English. Yet, even the grammatically incorrect comments 
can provide value for someone wondering why some code is as it is. Thus, 
I write the comments the best way I can - even though I am sure there 
are errors at times :)

I actually just tried running the comment through an online English 
checker. (Just picked first search result, I think it was called 
"Reverso" or something like that) Result the checker gave me was "No 
mistakes detected!"

Can you please suggest me a good tool for checking the grammar (or show 
me the errors - but that won't scale).

> 
>> + */
> 
> ...
> 
>> +static const int kx022a_accel_samp_freq_table[][2] = {
>> +	[0] = { 0, 780000 },
>> +	[1] = { 1, 563000 },
>> +	[2] = { 3, 125000 },
>> +	[3] = { 6, 250000 },
>> +	[4] = { 12, 500000 },
>> +	[5] = { 25, 0 },
>> +	[6] = { 50, 0 },
>> +	[7] = { 100, 0 },
>> +	[8] = { 200, 0 }
> 
> What do you need the indices for? They are not defines, so are you expecting to
> shuffle the entries?

No. Not expecting to suffle. I think I borrowed the construct from 
another driver. I am not sure if it had defines for indices though. 
Sounds like you have a problem with this, right? What would you suggest 
here?

>> +};
> 
> ...
> 
>> +static const unsigned int kx022a_odrs[] = { 1282051282, 639795266, 320 * MEGA,
>> +	 160 * MEGA, 80 * MEGA, 40 * MEGA, 20 * MEGA, 10 * MEGA, 5 * MEGA };
> 
> { and }; on the new line. And I would suggest to group by 4 or 8, that makes it
> easier to read / count.
> 
Right. I actually think Jonathan asked me to burn the lines and do 1 
item / line already during the v1 review. Thanks for pointing this out.


> ...
> 
>> +		int n = ARRAY_SIZE(kx022a_accel_samp_freq_table);
> 
> You may drop {} in each case and have n defined once for all cases.

Could you please explain how? We use different table depending on the case.

> 
>> +
>> +		while (n-- > 0)
> 
> while (n--) ? Or why the 0 is ignored?

What do you mean by 0 being ignored? I am sorry but I do have 
difficulties following some of your comments.

> 
>> +			if (val == kx022a_accel_samp_freq_table[n][0] &&
>> +			    kx022a_accel_samp_freq_table[n][1] == val2)
>> +				break;
> 
>> +		if (n < 0) {
> 
> How is it even possible with your conditional?

Eh? Why it would not be possible? I don't see any problem with the code. 
Could you explain me what you mean?

> 
>> +			ret = -EINVAL;
>> +			goto unlock_out;
>> +		}
> 
> ...
> 
>> +static int kx022a_get_axis(struct kx022a_data *data,
>> +			   struct iio_chan_spec const *chan,
>> +			   int *val)
>> +{
>> +	int ret;
>> +
>> +	ret = regmap_bulk_read(data->regmap, chan->address, &data->buffer,
>> +			       sizeof(s16));
> 
> No endianess awareness (sizeof __le16 / __be16)  >
>> +	if (ret)
>> +		return ret;
>> +
>> +	*val = data->buffer[0];
> 
> Ditto (get_unaligned_be16/le16 / le16/be16_to_cpup()).


I have probably misunderstood something but I don't see why we should 
use 'endianess awareness' in drivers? I thought the IIO framework code 
takes care of the endianes conversions based on scan_type so each 
individual driver does not need to do that. That however has been just 
my assumption. I will need to check this. Thanks for pointing it out.

> 
>> +	return IIO_VAL_INT;
>> +}
> 
>  > ...
> 
>> +	if (val > KX022A_FIFO_LENGTH)
>> +		val = KX022A_FIFO_LENGTH;
> 
> max() from minmax.h?

Definitely not. We could use min() to squeeze one line but for my 
personal preference the open-coded limit check is clearer. Can switch to 
min() if this is a subsystem standard though.

> ...
> 
>> +	/*
>> +	 * We must clear the old time-stamp to avoid computing the timestamps
>> +	 * based on samples acquired when buffer was last enabled
> 
> Period!

Is the use of exclamation mark is really justified? It makes your 
suggestion feel slightly rude.

> 
>> +	 */
> 
> ...
> 
>> +	return regmap_write(data->regmap, KX022A_REG_BUF_CLEAR, 0xff);
> 
> GENMASK() ?

I don't really see a need for GENMASK here. It does not matter what is 
written to this register.

> ...
> 
>> +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);
> 
> I would put redundant 'else' here to have them on the same column, but
> it's pity we don't have regmap_assign_bits() API (or whatever name you
> can come up with) to hide this kind of code.

Eh, you mean you would put else here even though we return from the if? 
And then put another return in else, and no return outside the if/else?

I definitely don't like the idea.

We could probably use regmap_update_bits and ternary but in my opinion 
that would be just much less obvious. I do like the use of set/clear 
bits which also makes the meaning / "polarity" of bits really clear.

> 
>> +	return regmap_clear_bits(data->regmap, KX022A_REG_CNTL,
>> +				 KX022A_MASK_DRDY);
>> +}
> 
> ...
> 
>> +	if (!g_kx022a_use_buffer) {
>> +		dev_err(data->dev, "Neither trigger set nor hw-fifo enabled\n");
> 
>> +
> 
> No need in blank line in such cases.
> 
>> +		return -EINVAL;
>> +	}
> 
> Maybe you want to place it here instead?
> 
Yes.

>> +	return kx022a_fifo_enable(data);
> 
> ...
> 
>> +	/*
>> +	 * I've seen I2C read failures if we poll too fast after the sensor
>> +	 * reset. Slight delay gives I2C block the time to recover.
>> +	 */
>> +	msleep(1);
> 
> msleep(1) is not doing what you think it is. I recommend to use usleep_range()
> here.

I think msleep(1) is sleeping 1ms or (likely) quite a bit more. It also 
allows other things to be scheduled meanwhile and does not do any (in 
this case) unnecessary real-time (like) constrains. Could you please 
educate me as to which part I got wrong? And no, I don't really care if 
the probe is resumed only after 1, 10, 20 or 40ms assuming the CPU has 
other things to do. (I don't think forcing this to be continued within 
hard time limit would be beneficial)

> 
> ...
> 
>> +int kx022a_probe_internal(struct device *dev)
>> +{
>> +	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, irq;
> 
>> +	if (WARN_ON(!dev))
> 
> Huh?! This can be easily transformed to panic followed by oops. Why is it
> necessary to do so?

Because the call to this probe is only issued from I2C / SPI wrappers 
which are part of the same driver. I think missing a dev here means 
error in this driver - and it should be caught immediately. I don't 
think this should ever happen unless one developing this driver makes an 
error.

Well, if there are doubts I can remove this though.

> 
>> +		return -ENODEV;

>> +	if (chip_id != KX022A_ID) {
>> +		dev_err(dev, "unsupported device 0x%x\n", chip_id);
>> +		return -EINVAL;
>> +	}
>> +
>> +	irq = fwnode_irq_get_byname(dev_fwnode(dev), "INT1");
>> +	if (irq > 0) {
>> +		data->inc_reg = KX022A_REG_INC1;
>> +		data->ien_reg = KX022A_REG_INC4;
>> +	} else {
>> +		irq = fwnode_irq_get_byname(dev_fwnode(dev), "INT2");
>> +		if (irq < 0)
>> +			return dev_err_probe(dev, irq, "No suitable IRQ\n");
> 
> Missed check for 0.

Why check for 0? fwnode_irq_get_byname() doc states:
  * Return:
  * Linux IRQ number on success, or negative errno otherwise.

I don't see 0 indicating a failure?

> 
> To me it feels inconsistency to use dev_err_probe() in the cases when
> it _might_ be a deferred probe and not use it in the cases we it won't
> be a deferred probe.
> 

I think we discussed this already. To me it feels better to have 
inconsistency for hard-coded return values than use dev_err_probe. At 
times using the "consistency" as a rationale for things feels like 
excusing the infamous "when you're used to use a hammer as solution, all 
problems start looking like nails" approach...

> 
>> +module_param(g_kx022a_use_buffer, bool, 0);
>> +MODULE_PARM_DESC(g_kx022a_use_buffer,
>> +		 "Buffer samples. Use at own risk. Fifo must not overflow");
> 
> Why?! We usually do not allow module parameters in the new code.

There are still quite a few parameters that have been added during 
previous cycle. And many of those are to allow behaviour which is 
undesirable in many cases for those who can benefit from it.

3dbec44d9c94 ("KVM: VMX: Reject kvm_intel if an inconsistent VMCS config 
is detected")

can serve as a random example.

As explained by the comment:
/*
  * Filling up the HW-FIFO can cause nasty problems. Thus we do not
  * enable the fifo unless it is explicitly requested by a module param.
  * If you are _sure_ your system can serve the interrupts in time you
  * can enable the HW fifo. I do not recommend it for sample frequencies
  * higher than 2 Hz - and even in that case I would set the watermark
  * somewhere near 20 samples (HI-RES) to have magnitude of 10 sec
  * safety-margin.
  */

this is also the use-case here.

And yes, using the FIFO regardless of the problems can be beneficial so 
just dropping the support completely would be counter-productive. I 
agree with Jonathan that disabling the FIFO by default does probably 
help avoiding bug reports - but allowing enabling the FIFO with own risk 
is just giving a hand to those who need it. Thus I'd really like to see 
the support upstream. We all know that code which is sitting in some 
downstream git is not likely to help people - and it is the reason why I 
do upstream code in first place.

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 v2 1/5] regulator: Add devm helpers for get and enable

[Matti Vaittinen]

Hi Jonathan,

On 10/9/22 15:24, Jonathan Cameron wrote:
> On Thu, 6 Oct 2022 19:17:39 +0300
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> 
>> On Thu, Oct 06, 2022 at 05:36:52PM +0300, Matti Vaittinen wrote:
>>> 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()
>>
>> ...
>>
>>> (cherry picked from commit b6058e052b842a19c8bb639798d8692cd0e7589f)
>>
>> Not sure:
>>   - why this is in the commit message
>>   - what it points to, since
>> $ git show b6058e052b842a19c8bb639798d8692cd0e7589f
>>   fatal: bad object b6058e052b842a19c8bb639798d8692cd0e7589f
> 
> These are now upstream in Linus' tree and in my testing branch.
> I'd not normally advocate working on top of that (because I rebase it), but
> if it is useful for this series go ahead.

Thanks for the explanation :)

This series will conflict with my fixup series for triggered-buffer 
attributes. Hence I though I might combine these two series into one if 
I need to respin the fixup series. I thought of using the v6.1-rc1 when 
it is out. (I think the 6.1-rc1 should not be that far away)

OTOH, I just read your another mail which told that there will be one 
more driver which will conflict with the fixup coming in during this 
cycle. If that driver lands in your tree before the fix - then I guess I 
need to rebase the fixup series (and maybe this too) on top of your tree 
+ add conversion of this new driver. I don't think that would be the 
testing branch though(?)

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 v2 3/5] dt-bindings: iio: Add KX022A accelerometer

[Matti Vaittinen]

On 10/9/22 15:27, Jonathan Cameron wrote:
> On Thu, 6 Oct 2022 18:32:22 +0300
> Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> 
>> Hi dee Ho Krzysztof,
>>
>> On 10/6/22 18:23, Krzysztof Kozlowski wrote:
>>> On 06/10/2022 16:37, Matti Vaittinen wrote:
>>>> KX022A is a 3-axis Accelerometer from ROHM/Kionix. The sensor 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 features.
>>>>   
>>>
>>> Thank you for your patch. There is something to discuss/improve.
>>>    
>>>> +
>>>> +properties:
>>>> +  compatible:
>>>> +    const: kionix,kx022a
>>>> +
>>>> +  reg:
>>>> +    maxItems: 1
>>>> +
>>>> +  interrupts:
>>>> +    minItems: 1
>>>> +    maxItems: 2
>>>> +
>>>> +  interrupt-names:
>>>> +    minItems: 1
>>>> +    maxItems: 2
>>>> +    items:
>>>> +      enum:
>>>> +        - INT1
>>>> +        - INT2
>>>
>>> This allows any order, which I assume was your intention.
>>
>> Yes. I don't see real need to restrict ordering - besides, with my
>> yaml/schema skills it'd took eternity to find corrct example(s) ;)
>>
>> My intention is that the user can give either one of these - or both.
>> Order needs naturally to match the order of IRQs - but this we can't know.
>>
>>> However maybe
>>> at least fix it a bit like:
>>> minItems: 1
>>> items:
>>>     - enum: [ int1, int2]
>>>     - const: int2
>>
>> If you say so XD
>> I can fix this for v3 :)
> If my limited understanding is correct, one advantage of this restriction
> is that we can't have
> 
> "INT1", "INT1"
> though that may be prevented elsewhere...
> 
> There is no loss of useful flexibility in how Krzysztof suggested doing it
> so looks like a good suggestion to me.
Thanks Krzysztof and Jonathan :) I'll use Krzysztof's suggestion for the 
v3.

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

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

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

[Matti Vaittinen]

On 10/9/22 15:38, Jonathan Cameron wrote:
> On Thu, 6 Oct 2022 17:38:34 +0300
> Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> 
>> Add maintainer entry for ROHM/Kionix KX022A accelerometer senor driver.
>>
>> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
>>
>> ---
>> I can also add myself as a maintainer instead of a reviewer if it better
>> suits iio maintainer. I however don't plan setting up my own public
>> repository and hope the further patches will be merged via IIO tree.
>>
>> So, as Geert once explained to me - In that case the difference between
>> me as a maintainer vs. a reviewer would be only really relevant to the
>> subsystem (in this case IIO) maintainer. The subsystem maintainer who
>> merges patches is allowed to take in changes acked by downstream
>> maintainer w/o obligation to do thorough review. (Downstream maintainer is
>> to be blamed if things explode :]). If ack is given by a reviewer, then
>> the subsystem maintainer has the full responsibility and should always
>> do the review. Or - this is how I remember our discussion went - feel
>> free to correct me if I am wrong :] In any case - please let me know if
>> you'd rather see M: not R: in front of my name for the kx022a.
> 
> 
> Entirely up to you.  I tend to take a look at all IIO patches but will
> take a less detailed look if there is a tag from someone I've built
> up trust with - doesn't matter if they are a maintainer of a particular
> driver or not. I only ever look at MAINTAINERS when sending out patches,
> never when reviewing except to perhaps moan that someone wasn't cc'd
> who should have been!

In that case I'll keep the R as I have done with the other corners I've 
touched.

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 v2 4/5] iio: accel: Support Kionix/ROHM KX022A accelerometer

[Andy Shevchenko]

On Mon, Oct 10, 2022 at 12:12:34PM +0300, Matti Vaittinen wrote:
> Hello Andy,
> 
> Thanks for taking the time and doing the review. Quite a few comments I do
> agree with. I didn't respond to those unless you asked a question :) Thanks
> for the help! OTOH, as usual, I do not fully agree with everything - and at
> some places I would like to get some further information :)
> 
> On 10/6/22 21:32, Andy Shevchenko wrote:
> > On Thu, Oct 06, 2022 at 05:38:14PM +0300, Matti Vaittinen wrote:

...

> > > +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.
> > 
> > Why it tending to get user to choose Y? It might increase the footprint of
> > bootable image and on the embedded platforms it may increase the chances
> > to not fit into (flash) ROM.
> 
> It may be just my English skills but to me this is really telling that "say
> Y to enable support for this IC". For me it does not mean that "Say Y even
> if you don't have this IC". I am really not seeing the problem here, but as
> I said - maybe it's just my lack of English skills.
> 
> Nevertheless, looking at the top of the IIO/accelerometer/KConfg - plenty of
> the accelerometer driver KConfig entries seem to use similar pattern. I am
> nevertheless usually willing to break the pattern if it seems like the right
> thing ;) Can anyone help me to format better description?

Oh, I didn't know that the rest is also using this pattern. My main objection
here is Y vs. M and maybe less insisting tone of the language (it doesn't tell
anything like "if you have the sensor".

I leave this to native speaker(s) to propose something here, because for me
English is not the one.

> > Also what will be the module name (see other Kconfig for the pattern)?
> 
> Here the pattern is IMHO improved by cutting the useless text. Even the
> cross-reference from SPI to I2C and I2C to SPI driver which was in v1 is
> more useful. Module autoloading for SPI should just work. Adding the module
> name here seems just a silly habit - which I see many people are breaking.
> If module name is to be needed, then it should be checked from the Makefile
> and not trusted that Kconfig text stays up-to-date...

I think the module name is a good thing to have for a user who might be
not knowing about Makefile:s and how to derive the module name from that
(sometimes it's not so obvious).

> If the module name in config text is generally seen useful (which I doubt),
> then maybe there should be a script that fetches it from the Makefile?

No idea about this, if it's even possible.

...

> > Can you fix I²C driver to follow this style?
> 
> I need to respin this anyways, so yes.
> 
> > > +	}
> > 
> > Ditto (blank line).
> 
> The blank prior function return? I think I have been told to have blanks
> preceding returns to make returns more obvious - which I kind of understand.

Yes, _adding_ blank line (see what I referred to? To SPI driver as a good
example).

...

> > > +#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>
> > 
> > Can this group be placed after units.h (with blank line in between)?
> 
> Why? What's wrong with alphabetical order?

Nothing wrong with the order and order is orthogonal to the grouping.
My comment is for the latter. The rationale is to easily see generic header
group and subsystem related group.

...

> > > +#define KXO22A_FIFO_ERR_THRESHOLD 10
> > > +#define KX022A_FIFO_LENGTH 41
> > 
> > Why not to use TAB between definitions and values (also for the rest similar
> > cases)?
> 
> Because I don't personally see the benefit. I can still do that as I've
> heard many people prefer the aligned values.

For me it's hard to read when they are not on the same column, that's the
benefit I see.

...

> > > +#define KX022A_FIFO_MAX_BYTES (KX022A_FIFO_LENGTH *			\
> > > +			       KX022A_FIFO_SAMPLES_SIZE_BYTES)
> > 
> > Slightly better to read:
> > 
> > #define KX022A_FIFO_MAX_BYTES					\
> > 	(KX022A_FIFO_LENGTH * KX022A_FIFO_SAMPLES_SIZE_BYTES)
> > 
> 
> Really?

Really.

> Well, I can change this but don't personally see any improvement.

...

> > > +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,
> > 
> > No max register? Have you tried debugfs output?
> 
> Can you please explain what you mean by no max register?

There is a field which name I don't remember by heart limits the register
range. This, in particular is visible when one tries to dump the registers
via debugfs.

But I see that it is there, I simply missed it.

...

> > > +struct kx022a_data {
> > > +	int irq;
> > > +	int inc_reg;
> > > +	int ien_reg;
> > 
> > > +	struct regmap *regmap;
> > 
> > Putting this as a first member might improve code by making pointer arithmetics
> > better. Have you checked possibilities with bloat-o-meter?
> 
> No. Amount of combinations is just too huge for me to try randomly
> reordering structs and running bloat-o-meter. Can you please explain me why
> you think reordering this would have visible impact and if this is something
> that is universally true or if such an optimization is architecture
> dependent?

Usually regmap pointer is used more often than, for instance, the 'irq' member,
and I believe that moving 'irq' member deeper will be harmless, while moving
regmap might be winning.

> > > +	struct iio_trigger *trig;
> > > +	bool trigger_enabled;
> > > +
> > > +	struct device *dev;
> > > +	unsigned int g_range;
> > 
> > > +	struct mutex mutex;
> > 
> > No warning from checkpatch? Every lock should be commented what it is for.
> 
> No. I didn't see a warn from checkpatch with v6.0-rc7. And I am glad I
> didn't. I am not a fan of statements like "always document a lock" or "never
> use a goto" - because blindly following such "rules" tend to lead silly
> corner cases. In lock commenting case it is likely to lead copy-pasting
> comments for locks which are used for trivial protections. And I am
> definitely not a fan of boilerplate.

So, tell then, what is this lock for? How as a code reader I have to know that?

> > > +	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] __aligned(IIO_DMA_MINALIGN);
> > > +	struct {
> > > +		__le16 channels[3];
> > > +		s64 ts __aligned(8);
> > > +	} scan;
> > > +};

...

> > > + * 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
> > 
> > Please, check the punctuation.
> 
> I admit it. Punctuation rules for English grammar are something I never
> learned. Actually, in the school I was told to just use short sentences
> because correct punctuation in English is not trivial. I understand that
> grammatically correct comments are pleasing to the eye for people who really
> _know_ English. Yet, even the grammatically incorrect comments can provide
> value for someone wondering why some code is as it is. Thus, I write the
> comments the best way I can - even though I am sure there are errors at
> times :)
> 
> I actually just tried running the comment through an online English checker.
> (Just picked first search result, I think it was called "Reverso" or
> something like that) Result the checker gave me was "No mistakes detected!"
> 
> Can you please suggest me a good tool for checking the grammar (or show me
> the errors - but that won't scale).

My eyes caught the missed period at the end of the comment (easy to catch),
other than that I use these rules:
 - Oxford comma;
 - commas for the subsentences that do not affect the meaning of the main
   one if they were dropped.

Rarely I use tools for punctuation as I agree it's not obvious in English.

...

> > > +static const int kx022a_accel_samp_freq_table[][2] = {
> > > +	[0] = { 0, 780000 },
> > > +	[1] = { 1, 563000 },
> > > +	[2] = { 3, 125000 },
> > > +	[3] = { 6, 250000 },
> > > +	[4] = { 12, 500000 },
> > > +	[5] = { 25, 0 },
> > > +	[6] = { 50, 0 },
> > > +	[7] = { 100, 0 },
> > > +	[8] = { 200, 0 }
> > 
> > What do you need the indices for? They are not defines, so are you expecting to
> > shuffle the entries?
> 
> No. Not expecting to suffle. I think I borrowed the construct from another
> driver. I am not sure if it had defines for indices though. Sounds like you
> have a problem with this, right? What would you suggest here?

Drop the indices and add a comma to the last entry (#8 according to your
indices). Similar to the rest of the similar cases.

> > > +};

...

> > > +		int n = ARRAY_SIZE(kx022a_accel_samp_freq_table);
> > 
> > You may drop {} in each case and have n defined once for all cases.
> 
> Could you please explain how? We use different table depending on the case.

	{
		int n;

		switch (x) {
		case Y:
			n = ARRAY_SIZE(foo);
			...
			break
		case Z:
			n = ARRAY_SIZE(bar);
			...
		}
	}

> > > +
> > > +		while (n-- > 0)
> > 
> > while (n--) ? Or why the 0 is ignored?
> 
> What do you mean by 0 being ignored? I am sorry but I do have difficulties
> following some of your comments.

Your while loop stops when n == 0. The 0 is not going inside while-loop body...

> > > +			if (val == kx022a_accel_samp_freq_table[n][0] &&
> > > +			    kx022a_accel_samp_freq_table[n][1] == val2)
> > > +				break;
> > 
> > > +		if (n < 0) {
> > 
> > How is it even possible with your conditional?
> 
> Eh? Why it would not be possible? I don't see any problem with the code.
> Could you explain me what you mean?

...and as a side effect, this condition won't be ever true. Did I miss
something?

> > 
> > > +			ret = -EINVAL;
> > > +			goto unlock_out;
> > > +		}

...

> > > +	ret = regmap_bulk_read(data->regmap, chan->address, &data->buffer,
> > > +			       sizeof(s16));

> > No endianess awareness (sizeof __le16 / __be16)

> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	*val = data->buffer[0];
> > 
> > Ditto (get_unaligned_be16/le16 / le16/be16_to_cpup()).
> 
> I have probably misunderstood something but I don't see why we should use
> 'endianess awareness' in drivers? I thought the IIO framework code takes
> care of the endianes conversions based on scan_type so each individual
> driver does not need to do that. That however has been just my assumption. I
> will need to check this. Thanks for pointing it out.

The IIO core uses endianness field only once in iio_show_fixed_type() AFAICS.
And it does nothing with it. Maybe Jonathan can shed a light what is it for
(I mean the field)?

...

> > > +	if (val > KX022A_FIFO_LENGTH)
> > > +		val = KX022A_FIFO_LENGTH;
> > 
> > max() from minmax.h?
> 
> Definitely not. We could use min() to squeeze one line but for my personal
> preference the open-coded limit check is clearer. Can switch to min() if
> this is a subsystem standard though.

Not sure how min() here would check for >. Are you sure you haven't forgotten
to tell me something more?

...

> > > +	return regmap_write(data->regmap, KX022A_REG_BUF_CLEAR, 0xff);
> > 
> > GENMASK() ?
> 
> I don't really see a need for GENMASK here. It does not matter what is
> written to this register.

Why not 0? At least it will be aligned to the _CLEAR part without need of
any comment. Otherwise it sounds like 0xff is special here.

...

> > > +	if (en)
> > > +		return regmap_set_bits(data->regmap, KX022A_REG_CNTL,
> > > +				       KX022A_MASK_DRDY);
> > 
> > I would put redundant 'else' here to have them on the same column, but
> > it's pity we don't have regmap_assign_bits() API (or whatever name you
> > can come up with) to hide this kind of code.
> 
> Eh, you mean you would put else here even though we return from the if? And
> then put another return in else, and no return outside the if/else?
> 
> I definitely don't like the idea.
> 
> We could probably use regmap_update_bits and ternary but in my opinion that
> would be just much less obvious. I do like the use of set/clear bits which
> also makes the meaning / "polarity" of bits really clear.

The idea behind is simple (and note that I'm usually on the side of _removing_
redundancy)

	if (en)
		return regmap_set_bits(data->regmap, KX022A_REG_CNTL,
				       KX022A_MASK_DRDY);
	else
		return regmap_clear_bits(data->regmap, KX022A_REG_CNTL,
					 ...

Because the branches are semantically tighten to each other. But it's not
a big deal to me.

> > > +	return regmap_clear_bits(data->regmap, KX022A_REG_CNTL,
> > > +				 KX022A_MASK_DRDY);

...

> > > +	if (WARN_ON(!dev))
> > 
> > Huh?! This can be easily transformed to panic followed by oops. Why is it
> > necessary to do so?
> 
> Because the call to this probe is only issued from I2C / SPI wrappers which
> are part of the same driver. I think missing a dev here means error in this
> driver - and it should be caught immediately. I don't think this should ever
> happen unless one developing this driver makes an error.
> 
> Well, if there are doubts I can remove this though.

I guess simple removal is right thing to do in this case.

> > > +		return -ENODEV;

...

> > > +	irq = fwnode_irq_get_byname(dev_fwnode(dev), "INT1");
> > > +	if (irq > 0) {
> > > +		data->inc_reg = KX022A_REG_INC1;
> > > +		data->ien_reg = KX022A_REG_INC4;
> > > +	} else {
> > > +		irq = fwnode_irq_get_byname(dev_fwnode(dev), "INT2");
> > > +		if (irq < 0)
> > > +			return dev_err_probe(dev, irq, "No suitable IRQ\n");
> > 
> > Missed check for 0.
> 
> Why check for 0? fwnode_irq_get_byname() doc states:
>  * Return:
>  * Linux IRQ number on success, or negative errno otherwise.
> 
> I don't see 0 indicating a failure?

It's a mislead by documentation. You need to read the fwnode_irq_get()
to see what's going on. Feel free to submit a fix for the
fwnode_irq_get_byname() kernel doc.

...

> > To me it feels inconsistency to use dev_err_probe() in the cases when
> > it _might_ be a deferred probe and not use it in the cases we it won't
> > be a deferred probe.
> 
> I think we discussed this already. To me it feels better to have
> inconsistency for hard-coded return values than use dev_err_probe. At times
> using the "consistency" as a rationale for things feels like excusing the
> infamous "when you're used to use a hammer as solution, all problems start
> looking like nails" approach...

Yes, just shading my opinion once again.

...

> > > +module_param(g_kx022a_use_buffer, bool, 0);
> > > +MODULE_PARM_DESC(g_kx022a_use_buffer,
> > > +		 "Buffer samples. Use at own risk. Fifo must not overflow");
> > 
> > Why?! We usually do not allow module parameters in the new code.
> 
> There are still quite a few parameters that have been added during previous
> cycle. And many of those are to allow behaviour which is undesirable in many
> cases for those who can benefit from it.
> 
> 3dbec44d9c94 ("KVM: VMX: Reject kvm_intel if an inconsistent VMCS config is
> detected")
> 
> can serve as a random example.

KVM is quite different by nature to an arbitrary driver of an arbitrary sensor
that might appear in the real hardware at hand on 1:100 ratio basis.

> As explained by the comment:
> /*
>  * Filling up the HW-FIFO can cause nasty problems. Thus we do not
>  * enable the fifo unless it is explicitly requested by a module param.
>  * If you are _sure_ your system can serve the interrupts in time you
>  * can enable the HW fifo. I do not recommend it for sample frequencies
>  * higher than 2 Hz - and even in that case I would set the watermark
>  * somewhere near 20 samples (HI-RES) to have magnitude of 10 sec
>  * safety-margin.
>  */
> 
> this is also the use-case here.
> 
> And yes, using the FIFO regardless of the problems can be beneficial so just
> dropping the support completely would be counter-productive. I agree with
> Jonathan that disabling the FIFO by default does probably help avoiding bug
> reports - but allowing enabling the FIFO with own risk is just giving a hand
> to those who need it. Thus I'd really like to see the support upstream. We
> all know that code which is sitting in some downstream git is not likely to
> help people - and it is the reason why I do upstream code in first place.

I would suggest to split the part that adds parameter (with whatever it does
elsewhere) from the basic configuration. In this case the commit message should
at least clarify the needs of the parameter.

JFYI: It's Greg KH strong opinion about parameters and I agree with him on
that. Note that this subsystem goes via on of the Greg's trees.

-- 
With Best Regards,
Andy Shevchenko

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

[Vaittinen, Matti]

On 10/10/22 14:58, Andy Shevchenko wrote:
> On Mon, Oct 10, 2022 at 12:12:34PM +0300, Matti Vaittinen wrote:
>> Hello Andy,
>>
>> Thanks for taking the time and doing the review. Quite a few comments I do
>> agree with. I didn't respond to those unless you asked a question :) Thanks
>> for the help! OTOH, as usual, I do not fully agree with everything - and at
>> some places I would like to get some further information :)
>>
>> On 10/6/22 21:32, Andy Shevchenko wrote:
>>> On Thu, Oct 06, 2022 at 05:38:14PM +0300, Matti Vaittinen wrote:
> 
> ...
> 
>>>> +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.
>>>

> Oh, I didn't know that the rest is also using this pattern. My main objection
> here is Y vs. M and maybe less insisting tone of the language

Ah. I didn't notice how this emphasizes Y over M. Now I think I 
understand your note. Thanks for the explanation.

>>> Also what will be the module name (see other Kconfig for the pattern)?
>>
>> Here the pattern is IMHO improved by cutting the useless text. Even the
>> cross-reference from SPI to I2C and I2C to SPI driver which was in v1 is
>> more useful. Module autoloading for SPI should just work. Adding the module
>> name here seems just a silly habit - which I see many people are breaking.
>> If module name is to be needed, then it should be checked from the Makefile
>> and not trusted that Kconfig text stays up-to-date...
> 
> I think the module name is a good thing to have for a user who might be
> not knowing about Makefile:s and how to derive the module name from that
> (sometimes it's not so obvious).

My point is that average users do not really even need to know the 
module names. For most simple drivers the module name is just the base 
of the filename. I really doubt there are that many people who

a) do need to know module name
b) read config menu entries but don't know how to find a module name 
from Makefile
c) want to know module name and can't pair the name from lsmod to module
d) rather look up the config entry than browse the contents of 
lib/modules/...

It'd be nice to know if anyone reading this mail has actually ever used 
the module name information placed in Kconfig entry for _anything else_ 
except for making the Kconfig entry to look a bit more fatty...


>>
>> The blank prior function return? I think I have been told to have blanks
>> preceding returns to make returns more obvious - which I kind of understand.
> 
> Yes, _adding_ blank line (see what I referred to? To SPI driver as a good
> example).

Ah. Thanks. I misunderstood your comment.

> ...
> 
>>>> +#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>
>>>
>>> Can this group be placed after units.h (with blank line in between)?
>>
>> Why? What's wrong with alphabetical order?
> 
> Nothing wrong with the order and order is orthogonal to the grouping.
> My comment is for the latter. The rationale is to easily see generic header
> group and subsystem related group.

I can for sure change this but It'd be nice to know why should the 
subsystem headers be treated differently from the rest of the headers?


> But I see that it is there, I simply missed it.
> 
Ok.

> ...
> 
>>>> +struct kx022a_data {
>>>> +	int irq;
>>>> +	int inc_reg;
>>>> +	int ien_reg;
>>>
>>>> +	struct regmap *regmap;
>>>
>>> Putting this as a first member might improve code by making pointer arithmetics
>>> better. Have you checked possibilities with bloat-o-meter?
>>
>> No. Amount of combinations is just too huge for me to try randomly
>> reordering structs and running bloat-o-meter. Can you please explain me why
>> you think reordering this would have visible impact and if this is something
>> that is universally true or if such an optimization is architecture
>> dependent?
> 
> Usually regmap pointer is used more often than, for instance, the 'irq' member,
> and I believe that moving 'irq' member deeper will be harmless, while moving
> regmap might be winning.

So you suggest us to try optimizing the code by placing a pointer to 
more frequently used member(s) at the top of the struct? I didn't know 
it would have any impact. I understand arranging the members of 
frequently copied structs based on sizes so that we can avoid eating 
space by padding - but I had never heard of things being improved by 
ordering based on frequency of use. It'd be great to understand the 
mechanism this optimization is based on?

> 
>>>> +	struct iio_trigger *trig;
>>>> +	bool trigger_enabled;
>>>> +
>>>> +	struct device *dev;
>>>> +	unsigned int g_range;
>>>
>>>> +	struct mutex mutex;
>>>
>>> No warning from checkpatch? Every lock should be commented what it is for.
>>
>> No. I didn't see a warn from checkpatch with v6.0-rc7. And I am glad I
>> didn't. I am not a fan of statements like "always document a lock" or "never
>> use a goto" - because blindly following such "rules" tend to lead silly
>> corner cases. In lock commenting case it is likely to lead copy-pasting
>> comments for locks which are used for trivial protections. And I am
>> definitely not a fan of boilerplate.
> 
> So, tell then, what is this lock for? How as a code reader I have to know that?

I can add comment to this lock as it does protect more than one thing 
(and may not be trivial). But I am not at all sure requiring all locks 
to be documented will have optimal end-result... And no, I don't think 
checkpatch in v6.0-rc7 did warn about this lock. Do you think checkpatch 
has been modified to do that?

> ...
> 
>>>> +		int n = ARRAY_SIZE(kx022a_accel_samp_freq_table);
>>>
>>> You may drop {} in each case and have n defined once for all cases.
>>
>> Could you please explain how? We use different table depending on the case.
> 
> 	{
> 		int n;
> 
> 		switch (x) {
> 		case Y:
> 			n = ARRAY_SIZE(foo);
> 			...
> 			break
> 		case Z:
> 			n = ARRAY_SIZE(bar);
> 			...
> 		}
> 	}

Ah, sure. I feel a bit stupid :) But thanks for the example!

> 
>>>> +
>>>> +		while (n-- > 0)
>>>
>>> while (n--) ? Or why the 0 is ignored?
>>
>> What do you mean by 0 being ignored? I am sorry but I do have difficulties
>> following some of your comments.
> 
> Your while loop stops when n == 0. The 0 is not going inside while-loop body...

I think you are making a mistake here.

>>>> +			if (val == kx022a_accel_samp_freq_table[n][0] &&
>>>> +			    kx022a_accel_samp_freq_table[n][1] == val2)
>>>> +				break;
>>>
>>>> +		if (n < 0) {
>>>
>>> How is it even possible with your conditional?
>>
>> Eh? Why it would not be possible? I don't see any problem with the code.
>> Could you explain me what you mean?
> 
> ...and as a side effect, this condition won't be ever true. Did I miss
> something?
> 

Please try running:
#include <stdio.h>

int main()
{
	int n = 2;

	while (n-- > 0)
		printf("in loop n = %d\n", n);

	printf("exit loop n = %d\n", n);

	return 0;
}

>>>
>>>> +			ret = -EINVAL;
>>>> +			goto unlock_out;
>>>> +		}
> 
> ...
> 
>>>> +	ret = regmap_bulk_read(data->regmap, chan->address, &data->buffer,
>>>> +			       sizeof(s16));
> 
>>> No endianess awareness (sizeof __le16 / __be16)
> 
>>>> +	if (ret)
>>>> +		return ret;
>>>> +
>>>> +	*val = data->buffer[0];
>>>
>>> Ditto (get_unaligned_be16/le16 / le16/be16_to_cpup()).
>>
>> I have probably misunderstood something but I don't see why we should use
>> 'endianess awareness' in drivers? I thought the IIO framework code takes
>> care of the endianes conversions based on scan_type so each individual
>> driver does not need to do that. That however has been just my assumption. I
>> will need to check this. Thanks for pointing it out.
> 
> The IIO core uses endianness field only once in iio_show_fixed_type() AFAICS.
> And it does nothing with it. Maybe Jonathan can shed a light what is it for
> (I mean the field)?
> 
> ...
> 
>>>> +	if (val > KX022A_FIFO_LENGTH)
>>>> +		val = KX022A_FIFO_LENGTH;
>>>
>>> max() from minmax.h?
>>
>> Definitely not. We could use min() to squeeze one line but for my personal
>> preference the open-coded limit check is clearer. Can switch to min() if
>> this is a subsystem standard though.
> 
> Not sure how min() here would check for >. Are you sure you haven't forgotten
> to tell me something more?
>

Please bear with me if I am wrong here. I have spent the weekend 
wandering in the forest and I am tired as ....

Anyways, my pretty dizzy brains are telling me that we want to use the 
val if it is smaller than the KX022A_FIFO_LENGTH. If val is greater, 
then we use the KX022A_FIFO_LENGTH. This should match with the

 >>>> +	if (val > KX022A_FIFO_LENGTH)
 >>>> +		val = KX022A_FIFO_LENGTH;

Eg, we always want to use the smaller of the two, right? Hence:

val = min(val, KX022A_FIFO_LENGTH);

would do the same job, right? But for me the open-coded check if val 
exceeds the FIFO_LENGTH looks more obvious.

> ...
> 
>>>> +	return regmap_write(data->regmap, KX022A_REG_BUF_CLEAR, 0xff);
>>>
>>> GENMASK() ?
>>
>> I don't really see a need for GENMASK here. It does not matter what is
>> written to this register.
> 
> Why not 0? At least it will be aligned to the _CLEAR part without need of
> any comment. Otherwise it sounds like 0xff is special here.

I can change it to 0 if it pleases the audience :) 0xff was just first 
thing that came to my mind when I read the spec that something should be 
written to this register.

>>>> +	if (en)
>>>> +		return regmap_set_bits(data->regmap, KX022A_REG_CNTL,
>>>> +				       KX022A_MASK_DRDY);
>>>
>>> I would put redundant 'else' here to have them on the same column, but
>>> it's pity we don't have regmap_assign_bits() API (or whatever name you
>>> can come up with) to hide this kind of code.
>>
>> Eh, you mean you would put else here even though we return from the if? And
>> then put another return in else, and no return outside the if/else?
>>
>> I definitely don't like the idea.
>>
>> We could probably use regmap_update_bits and ternary but in my opinion that
>> would be just much less obvious. I do like the use of set/clear bits which
>> also makes the meaning / "polarity" of bits really clear.
> 
> The idea behind is simple (and note that I'm usually on the side of _removing_
> redundancy)
> 
> 	if (en)
> 		return regmap_set_bits(data->regmap, KX022A_REG_CNTL,
> 				       KX022A_MASK_DRDY);
> 	else
> 		return regmap_clear_bits(data->regmap, KX022A_REG_CNTL,
> 					 ...
> 
> Because the branches are semantically tighten to each other. But it's not
> a big deal to me.

What I do not really like in above example is that we never reach the 
end of function body. It is against the expectation - and adding the 
else has no real meaning when if returns. I actually think that 
checkpatch could even warn about that construct.

> 
>>>> +	return regmap_clear_bits(data->regmap, KX022A_REG_CNTL,
>>>> +				 KX022A_MASK_DRDY);
> 
> 
>>>> +	irq = fwnode_irq_get_byname(dev_fwnode(dev), "INT1");
>>>> +	if (irq > 0) {
>>>> +		data->inc_reg = KX022A_REG_INC1;
>>>> +		data->ien_reg = KX022A_REG_INC4;
>>>> +	} else {
>>>> +		irq = fwnode_irq_get_byname(dev_fwnode(dev), "INT2");
>>>> +		if (irq < 0)
>>>> +			return dev_err_probe(dev, irq, "No suitable IRQ\n");
>>>
>>> Missed check for 0.
>>
>> Why check for 0? fwnode_irq_get_byname() doc states:
>>   * Return:
>>   * Linux IRQ number on success, or negative errno otherwise.
>>
>> I don't see 0 indicating a failure?
> 
> It's a mislead by documentation. You need to read the fwnode_irq_get()
> to see what's going on. Feel free to submit a fix for the
> fwnode_irq_get_byname() kernel doc.

Thanks. I had missed the mapping error.

> ...
> 
>>>> +module_param(g_kx022a_use_buffer, bool, 0);
>>>> +MODULE_PARM_DESC(g_kx022a_use_buffer,
>>>> +		 "Buffer samples. Use at own risk. Fifo must not overflow");
>>>
>>> Why?! We usually do not allow module parameters in the new code.
>>
>> There are still quite a few parameters that have been added during previous
>> cycle. And many of those are to allow behaviour which is undesirable in many
>> cases for those who can benefit from it.
>>
>> 3dbec44d9c94 ("KVM: VMX: Reject kvm_intel if an inconsistent VMCS config is
>> detected")
>>
>> can serve as a random example.
> 
> KVM is quite different by nature to an arbitrary driver of an arbitrary sensor
> that might appear in the real hardware at hand on 1:100 ratio basis.
> 
>> As explained by the comment:
>> /*
>>   * Filling up the HW-FIFO can cause nasty problems. Thus we do not
>>   * enable the fifo unless it is explicitly requested by a module param.
>>   * If you are _sure_ your system can serve the interrupts in time you
>>   * can enable the HW fifo. I do not recommend it for sample frequencies
>>   * higher than 2 Hz - and even in that case I would set the watermark
>>   * somewhere near 20 samples (HI-RES) to have magnitude of 10 sec
>>   * safety-margin.
>>   */
>>
>> this is also the use-case here.
>>
>> And yes, using the FIFO regardless of the problems can be beneficial so just
>> dropping the support completely would be counter-productive. I agree with
>> Jonathan that disabling the FIFO by default does probably help avoiding bug
>> reports - but allowing enabling the FIFO with own risk is just giving a hand
>> to those who need it. Thus I'd really like to see the support upstream. We
>> all know that code which is sitting in some downstream git is not likely to
>> help people - and it is the reason why I do upstream code in first place.
> 
> I would suggest to split the part that adds parameter (with whatever it does
> elsewhere) from the basic configuration. In this case the commit message should
> at least clarify the needs of the parameter.

I think adding the parameter in a separate patch is a good idea - even 
if patching something that was added by earlier patch in the series is 
often frowned upon.

> JFYI: It's Greg KH strong opinion about parameters and I agree with him on
> that. Note that this subsystem goes via on of the Greg's trees.

Sure. And if this is not accepted by a maintainer in the chain - then it 
needs to be re-evaluated, changed or dropped. But at least I can try 
explaining why I think this is needed / useful.

Thanks for the help once again.

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 v2 4/5] iio: accel: Support Kionix/ROHM KX022A accelerometer

[Andy Shevchenko]

On Mon, Oct 10, 2022 at 01:20:23PM +0000, Vaittinen, Matti wrote:
> On 10/10/22 14:58, Andy Shevchenko wrote:
> > On Mon, Oct 10, 2022 at 12:12:34PM +0300, Matti Vaittinen wrote:
> >> On 10/6/22 21:32, Andy Shevchenko wrote:
> >>> On Thu, Oct 06, 2022 at 05:38:14PM +0300, Matti Vaittinen wrote:

(Note, everything I have dropped without comment can be treated
 as "yes, I agree on your points". Same applies to the early or
 the future replies.)

...

> >>> Also what will be the module name (see other Kconfig for the pattern)?
> >>
> >> Here the pattern is IMHO improved by cutting the useless text. Even the
> >> cross-reference from SPI to I2C and I2C to SPI driver which was in v1 is
> >> more useful. Module autoloading for SPI should just work. Adding the module
> >> name here seems just a silly habit - which I see many people are breaking.
> >> If module name is to be needed, then it should be checked from the Makefile
> >> and not trusted that Kconfig text stays up-to-date...
> > 
> > I think the module name is a good thing to have for a user who might be
> > not knowing about Makefile:s and how to derive the module name from that
> > (sometimes it's not so obvious).
> 
> My point is that average users do not really even need to know the 
> module names. For most simple drivers the module name is just the base 
> of the filename. I really doubt there are that many people who
> 
> a) do need to know module name

Everyone? When I need to debug the issue (assisted debug or doing myself) it is
often good to know:

 1) is the driver built as a module;
 2) is that module ever loaded;
 3) how to find initial module parameters if any
    (see /sys/module/$NAME/parameters).

> b) read config menu entries but don't know how to find a module name
> from Makefile

Isn't it orthogonal? Average user may easily choose everything in the kernel
configuration via `make menuconfig` because it _is_ user interface / tool.
Makefile simply is not for the users.

> c) want to know module name and can't pair the name from lsmod to module

It's reversed mapping we need.

> d) rather look up the config entry than browse the contents of
> lib/modules/...

So, I have a module called 'foo-bar.ko' and lsmod shows me 'baz-hell'.
How should I know that mapping to begin with?

> It'd be nice to know if anyone reading this mail has actually ever used
> the module name information placed in Kconfig entry for _anything else_
> except for making the Kconfig entry to look a bit more fatty...

That said, I haven't found any good justification from your side to avoid
including the module name, while I see the advantages of otherwise.

...

> >>>> +#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>
> >>>
> >>> Can this group be placed after units.h (with blank line in between)?
> >>
> >> Why? What's wrong with alphabetical order?
> > 
> > Nothing wrong with the order and order is orthogonal to the grouping.
> > My comment is for the latter. The rationale is to easily see generic header
> > group and subsystem related group.
> 
> I can for sure change this but It'd be nice to know why should the 
> subsystem headers be treated differently from the rest of the headers?

Rationale is based on the rule 'generic headers are included first'.
Subsystem's headers can be considered less generic than the rest.

...

> >>>> +struct kx022a_data {
> >>>> +	int irq;
> >>>> +	int inc_reg;
> >>>> +	int ien_reg;
> >>>
> >>>> +	struct regmap *regmap;
> >>>
> >>> Putting this as a first member might improve code by making pointer arithmetics
> >>> better. Have you checked possibilities with bloat-o-meter?
> >>
> >> No. Amount of combinations is just too huge for me to try randomly
> >> reordering structs and running bloat-o-meter. Can you please explain me why
> >> you think reordering this would have visible impact and if this is something
> >> that is universally true or if such an optimization is architecture
> >> dependent?
> > 
> > Usually regmap pointer is used more often than, for instance, the 'irq' member,
> > and I believe that moving 'irq' member deeper will be harmless, while moving
> > regmap might be winning.
> 
> So you suggest us to try optimizing the code by placing a pointer to 
> more frequently used member(s) at the top of the struct? I didn't know 
> it would have any impact. I understand arranging the members of 
> frequently copied structs based on sizes so that we can avoid eating 
> space by padding - but I had never heard of things being improved by 
> ordering based on frequency of use. It'd be great to understand the 
> mechanism this optimization is based on?

It's based on the pointer arithmetics. In some cases it might be a win, in some
actually no advantage, that's why it's hard to say just by looking at the code.

Most affected cases are for the embedded data structures, where container_of()
is used on top, esp. if most used member of those structures are also appeared
first. Often example is:

struct foo {
	struct list_head node;
	...
};

vs.

struct foo {
	...
	struct list_head node;
};

where you may even see by reading the code that list_for_each_entry()
will be shorter.

With pointers it's less visible, but still might have a difference.

> >>>> +	struct iio_trigger *trig;
> >>>> +	bool trigger_enabled;
> >>>> +
> >>>> +	struct device *dev;
> >>>> +	unsigned int g_range;
> >>>
> >>>> +	struct mutex mutex;
> >>>
> >>> No warning from checkpatch? Every lock should be commented what it is for.
> >>
> >> No. I didn't see a warn from checkpatch with v6.0-rc7. And I am glad I
> >> didn't. I am not a fan of statements like "always document a lock" or "never
> >> use a goto" - because blindly following such "rules" tend to lead silly
> >> corner cases. In lock commenting case it is likely to lead copy-pasting
> >> comments for locks which are used for trivial protections. And I am
> >> definitely not a fan of boilerplate.
> > 
> > So, tell then, what is this lock for? How as a code reader I have to know that?
> 
> I can add comment to this lock as it does protect more than one thing 
> (and may not be trivial). But I am not at all sure requiring all locks 
> to be documented will have optimal end-result... And no, I don't think 
> checkpatch in v6.0-rc7 did warn about this lock. Do you think checkpatch 
> has been modified to do that?

In the commit 4a0df2ef4569 ("update checkpatch.pl to version 0.03"):
...
          check that spinlock_t and struct mutex use is commented
...

dated back 2007.

...

> >>>> +
> >>>> +		while (n-- > 0)
> >>>
> >>> while (n--) ? Or why the 0 is ignored?

So, it appears that yours and mine are equivalents, but mine is shorter :-)

...

> >>>> +	return regmap_write(data->regmap, KX022A_REG_BUF_CLEAR, 0xff);
> >>>
> >>> GENMASK() ?
> >>
> >> I don't really see a need for GENMASK here. It does not matter what is
> >> written to this register.
> > 
> > Why not 0? At least it will be aligned to the _CLEAR part without need of
> > any comment. Otherwise it sounds like 0xff is special here.
> 
> I can change it to 0 if it pleases the audience :) 0xff was just first 
> thing that came to my mind when I read the spec that something should be 
> written to this register.

At least it will be less confusing in my opinion.

...

> >>>> +	if (en)
> >>>> +		return regmap_set_bits(data->regmap, KX022A_REG_CNTL,
> >>>> +				       KX022A_MASK_DRDY);
> >>>
> >>> I would put redundant 'else' here to have them on the same column, but
> >>> it's pity we don't have regmap_assign_bits() API (or whatever name you
> >>> can come up with) to hide this kind of code.
> >>
> >> Eh, you mean you would put else here even though we return from the if? And
> >> then put another return in else, and no return outside the if/else?
> >>
> >> I definitely don't like the idea.
> >>
> >> We could probably use regmap_update_bits and ternary but in my opinion that
> >> would be just much less obvious. I do like the use of set/clear bits which
> >> also makes the meaning / "polarity" of bits really clear.
> > 
> > The idea behind is simple (and note that I'm usually on the side of _removing_
> > redundancy)
> > 
> > 	if (en)
> > 		return regmap_set_bits(data->regmap, KX022A_REG_CNTL,
> > 				       KX022A_MASK_DRDY);
> > 	else
> > 		return regmap_clear_bits(data->regmap, KX022A_REG_CNTL,
> > 					 ...
> > 
> > Because the branches are semantically tighten to each other. But it's not
> > a big deal to me.
> 
> What I do not really like in above example is that we never reach the 
> end of function body.

What do you mean by that? Compiler does warn or what?

> It is against the expectation - and adding the 
> else has no real meaning when if returns. I actually think that 
> checkpatch could even warn about that construct.

No way we ever accept such a thing for checkpatch because it's subjective
(very dependant on the code piece). OTOH the proposed pattern is used in
many places and left like that in places where I cleaned up the 'else',
to leave the semantic tights with the above lines).

> >>>> +	return regmap_clear_bits(data->regmap, KX022A_REG_CNTL,
> >>>> +				 KX022A_MASK_DRDY);

I see that we have a strong disagreement here. I leave it to maintainers.

-- 
With Best Regards,
Andy Shevchenko

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

[Vaittinen, Matti]

On 10/10/22 17:09, Andy Shevchenko wrote:
> On Mon, Oct 10, 2022 at 01:20:23PM +0000, Vaittinen, Matti wrote:
>> On 10/10/22 14:58, Andy Shevchenko wrote:
>>> On Mon, Oct 10, 2022 at 12:12:34PM +0300, Matti Vaittinen wrote:
>>>> On 10/6/22 21:32, Andy Shevchenko wrote:
>>>>> On Thu, Oct 06, 2022 at 05:38:14PM +0300, Matti Vaittinen wrote:
> 
> (Note, everything I have dropped without comment can be treated
>   as "yes, I agree on your points". Same applies to the early or
>   the future replies.)
> 
> ...
> 
>>>>> Also what will be the module name (see other Kconfig for the pattern)?
>>>>
>>>> Here the pattern is IMHO improved by cutting the useless text. Even the
>>>> cross-reference from SPI to I2C and I2C to SPI driver which was in v1 is
>>>> more useful. Module autoloading for SPI should just work. Adding the module
>>>> name here seems just a silly habit - which I see many people are breaking.
>>>> If module name is to be needed, then it should be checked from the Makefile
>>>> and not trusted that Kconfig text stays up-to-date...
>>>
>>> I think the module name is a good thing to have for a user who might be
>>> not knowing about Makefile:s and how to derive the module name from that
>>> (sometimes it's not so obvious).
>>
>> My point is that average users do not really even need to know the
>> module names. For most simple drivers the module name is just the base
>> of the filename. I really doubt there are that many people who
>>
>> a) do need to know module name
> 
> Everyone? When I need to debug the issue (assisted debug or doing myself) it is
> often good to know:
> 
>   1) is the driver built as a module;
>   2) is that module ever loaded;
>   3) how to find initial module parameters if any
>      (see /sys/module/$NAME/parameters).
> 

Fair enough. Next question is that how often do you launch kernel 
configuration tool and try finding the correct config option when you 
need to know the module name? My gut feeling is that average users do 
not seek this information using config tools. (In my opinion) config is 
just a wrong place for this information. When you need to debug a module 
you should not be using the kernel config tool to find debugging 
guidance. It is just a wrong place. The config tool is for kernel 
configuration - and at that phase the module name does not matter. At 
that phase you want to know what support the option gives to you and if 
you should use it on your system. Name of a module is completely irrelevant.

>> b) read config menu entries but don't know how to find a module name
>> from Makefile
> 
> Isn't it orthogonal? Average user may easily choose everything in the kernel
> configuration via `make menuconfig` because it _is_ user interface / tool.
> Makefile simply is not for the users.

Average users use distribution kernels. Some users may compile own 
kernel - but use config from previous kernel. Some users use "more 
exotic" boards - and use defconfigs for those. It is really just a 
handful of people who do read/change a config option for an 
accelerometer, PMIC, RTC, GPIO-controller or any other such single HW 
component. And I am pretty sure that those who do, do also understand 
the syntax of a Makefile.

And still, the moment of doing kernel configuration is not the moment 
when you need the module name.

>> c) want to know module name and can't pair the name from lsmod to module
> 
> It's reversed mapping we need.
> 
>> d) rather look up the config entry than browse the contents of
>> lib/modules/...
> 
> So, I have a module called 'foo-bar.ko' and lsmod shows me 'baz-hell'.
> How should I know that mapping to begin with?
> 
>> It'd be nice to know if anyone reading this mail has actually ever used
>> the module name information placed in Kconfig entry for _anything else_
>> except for making the Kconfig entry to look a bit more fatty...
> 
> That said, I haven't found any good justification from your side to avoid
> including the module name, while I see the advantages of otherwise.

The disadvantage is systematically polluting the config entries with 
irrelevant (to configuration) information. We should focus more on the 
question if the config entry explains user whether he should or should 
not enable the piece of config on his system. IMNAHEO (In My Not Always 
Humble Enough Opinion) all the rest belongs to some other documentation.

> ...
> 
>>>>>> +struct kx022a_data {
>>>>>> +	int irq;
>>>>>> +	int inc_reg;
>>>>>> +	int ien_reg;
>>>>>
>>>>>> +	struct regmap *regmap;
>>>>>
>>>>> Putting this as a first member might improve code by making pointer arithmetics
>>>>> better. Have you checked possibilities with bloat-o-meter?
>>>>
>>>> No. Amount of combinations is just too huge for me to try randomly
>>>> reordering structs and running bloat-o-meter. Can you please explain me why
>>>> you think reordering this would have visible impact and if this is something
>>>> that is universally true or if such an optimization is architecture
>>>> dependent?
>>>
>>> Usually regmap pointer is used more often than, for instance, the 'irq' member,
>>> and I believe that moving 'irq' member deeper will be harmless, while moving
>>> regmap might be winning.
>>
>> So you suggest us to try optimizing the code by placing a pointer to
>> more frequently used member(s) at the top of the struct? I didn't know
>> it would have any impact. I understand arranging the members of
>> frequently copied structs based on sizes so that we can avoid eating
>> space by padding - but I had never heard of things being improved by
>> ordering based on frequency of use. It'd be great to understand the
>> mechanism this optimization is based on?
> 
> It's based on the pointer arithmetics. In some cases it might be a win, in some
> actually no advantage, that's why it's hard to say just by looking at the code.
> 
> Most affected cases are for the embedded data structures, where container_of()
> is used on top,

Oh. Do you mean that in some cases when CPU needs some information 
frequently, it should be at the top of a struct so that no offset needs 
to be added when information is obtained using a pointer to the struct? 
Eg. Adding offset to the pointer (pointer arithmetics) is not done when 
first member of a struct is obtained - resulting maybe one instruction 
less. So having the most frequently used data as first member of a 
struct can be beneficial? I assume that if offset needs to be added, 
then it does not really matter how big the offset is - although 
naturally the size of the (offset) struct matters when we consider how 
data fits in cache.

Okay, cool thing to keep in mind when optimizing some really performance 
critical code in a fixed environment - or when dealing with a set of 
instructions copied to many places. I am afraid that for a driver 
intended to be used in different systems the manual optimization results 
are not really that good. Also, bloat-o-meter results on kernel compiled 
with my specific optimization settings and compiler would probably not 
be universally true. (At the moment I do develop the driver using pretty 
ancient version of a 32-bit arm compiler). (I don't really know the 
bloat-o-meter but my understanding is that it compares the compiled 
binaries to find out the differencies)

> esp. if most used member of those structures are also appeared
> first. Often example is:
> 
> struct foo {
> 	struct list_head node;
> 	...
> };
> 
> vs.
> 
> struct foo {
> 	...
> 	struct list_head node;
> };
> 
> where you may even see by reading the code that list_for_each_entry()
> will be shorter.
> 
> With pointers it's less visible, but still might have a difference.
> 
> ...
> 
>>>>>> +
>>>>>> +		while (n-- > 0)
>>>>>
>>>>> while (n--) ? Or why the 0 is ignored?
> 
> So, it appears that yours and mine are equivalents, but mine is shorter :-)

Yes. Besides, I actually like looks of while (n--) more than while (n-- 
 > 0). I think while (n-- > 0) has originated from the code where I 
stole the logic :)

> 
>>>>>> +	if (en)
>>>>>> +		return regmap_set_bits(data->regmap, KX022A_REG_CNTL,
>>>>>> +				       KX022A_MASK_DRDY);
>>>>>
>>>>> I would put redundant 'else' here to have them on the same column, but
>>>>> it's pity we don't have regmap_assign_bits() API (or whatever name you
>>>>> can come up with) to hide this kind of code.
>>>>
>>>> Eh, you mean you would put else here even though we return from the if? And
>>>> then put another return in else, and no return outside the if/else?
>>>>
>>>> I definitely don't like the idea.
>>>>
>>>> We could probably use regmap_update_bits and ternary but in my opinion that
>>>> would be just much less obvious. I do like the use of set/clear bits which
>>>> also makes the meaning / "polarity" of bits really clear.
>>>
>>> The idea behind is simple (and note that I'm usually on the side of _removing_
>>> redundancy)
>>>
>>> 	if (en)
>>> 		return regmap_set_bits(data->regmap, KX022A_REG_CNTL,
>>> 				       KX022A_MASK_DRDY);
>>> 	else
>>> 		return regmap_clear_bits(data->regmap, KX022A_REG_CNTL,
>>> 					 ...
>>>
>>> Because the branches are semantically tighten to each other. But it's not
>>> a big deal to me.
>>
>> What I do not really like in above example is that we never reach the
>> end of function body.
> 
> What do you mean by that? Compiler does warn or what?

I don't know if compiler warns about it as I didn't test it. Now that 
you mentioned it, I think I have seen such warnings from a compiler or 
some static analyzer (klocwork?) in the past though. What I mean is that:

int foo()
{
	if () {
		...
		return 1;
	} else {
		...
		return 2;
	}
}

construct makes mistakes like:

int foo()
{
	if () {
		...
		return 1;
	} else {
		...
		return 2;
	}

	...

	return 0;
}

easy to make. When one uses:

int foo()
{
	if () {
		...
		return 1;
	}

	...
	return 2;
}

to begin with there is zero room for such confusion.

> 
>> It is against the expectation - and adding the
>> else has no real meaning when if returns. I actually think that
>> checkpatch could even warn about that construct.
> 
> No way we ever accept such a thing for checkpatch because it's subjective

Eh. Are you telling me that there is no subjective stuff in checkpatch? ;)

> (very dependant on the code piece). OTOH the proposed pattern is used in
> many places and left like that in places where I cleaned up the 'else',
> to leave the semantic tights with the above lines).
> 
>>>>>> +	return regmap_clear_bits(data->regmap, KX022A_REG_CNTL,
>>>>>> +				 KX022A_MASK_DRDY);
> 
> I see that we have a strong disagreement here. I leave it to maintainers.


Okay, let's hear what others have to say here.

Thanks for all the input this far.

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 v2 4/5] iio: accel: Support Kionix/ROHM KX022A accelerometer

[Vaittinen, Matti]

On 10/10/22 09:15, Andy Shevchenko wrote:
> On Sun, Oct 09, 2022 at 01:33:51PM +0100, Jonathan Cameron wrote:
>> On Thu, 6 Oct 2022 21:32:11 +0300 Andy Shevchenko 
>> <andriy.shevchenko@linux.intel.com> wrote:
>>> On Thu, Oct 06, 2022 at 05:38:14PM +0300, Matti Vaittinen wrote:
> 
> ...
> 
>>>> +module_param(g_kx022a_use_buffer, bool, 0); 
>>>> +MODULE_PARM_DESC(g_kx022a_use_buffer, +		 "Buffer samples. Use
>>>> at own risk. Fifo must not overflow");
>>> 
>>> Why?! We usually do not allow module parameters in the new code.
>> 
>> Badly broken hardware - was my suggestion.  Alternatives if there 
>> are usecases that need to use the fifo, but it can wedge hard in a
>>  fashion that is impossible to prevent from the driver?  My gut 
>> feeling is still drop the support entirely with a strong comment in
>> the code that the hardware is broken in a fashion we don't know how
>> to work around.

I did some quick study regarding couple of other Kionix sensors. (like 
KX122 and old KX022 - without the 'A'). It seems to me that the register 
interfaces between many of the sensors are largely identical. Extending 
the driver to support those seems pretty straightforward (scales and 
resolution may need tweaking, as does the FIFO size) but register 
contents and even offsets are largely identical.

As said, it seems the Kionix sensors may have different size of internal 
FIFOs, or even no FIFO at all. So, maybe we could provide a 
"kionix,fifo-enable" flag (or even "kionix,fifo-size") from device-tree? 
This would be a way to have the FIFO disabled by default and warn users 
via dt-binding docs if they decide to explicitly enable the FIFO. 
(Besides, I believe the FIFO is usable on at least some of the Kionix 
sensors - because I've heard it is used on a few platforms).

This could give us safe defaults while not shutting the doors from those 
who wish to use the FIFO. Sure we need a buy-in from Krzysztof / Rob, 
but that may be less of an obstacle compared to the module param if Greg 
is so strongly oppsoing those. (And the dt-property could also provide 
some technical merites as these sensors seem to really have differencies 
in FIFOs).

> 
> I also would drop this from upstream and if anybody curious, provide
>  some kind of GitHub gist for that.

Well, I think we all agree that downstream code hosted in some 
unofficial github repositories are rarely that valuable. They're less 
reliable, less tested, less reviewed, less secure and pretty much 
impossible to maintain in a way that interested user could get a version 
matching his preferred kernel.

There are reasons why I (people) keep sending the drivers to upstream - 
and why some companies even spend $$ for that. Having this feature in 
downstream repo is not nearly on same page for user's point of view as 
is having the support upstream.

> Also it needs some communication
>  with a vendor to clarify the things.

I do communication with the vendor on a daily basis :] Nowadays Kionix 
is part of ROHM, and Finland SWDC has been collaboration with Kionix 
even before they "merged" (but I have not been part of the "sensor team" 
back then).

Unfortunately, reaching the correct people inside the company is hard 
and occasionally impossible - long story...


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

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

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

[Matti Vaittinen]

On 10/10/22 16:20, Vaittinen, Matti wrote:
> On 10/10/22 14:58, Andy Shevchenko wrote:
>> On Mon, Oct 10, 2022 at 12:12:34PM +0300, Matti Vaittinen wrote:
>> ...
>>
>>>>> +	ret = regmap_bulk_read(data->regmap, chan->address, &data->buffer,
>>>>> +			       sizeof(s16));
>>
>>>> No endianess awareness (sizeof __le16 / __be16)
>>
>>>>> +	if (ret)
>>>>> +		return ret;
>>>>> +
>>>>> +	*val = data->buffer[0];
>>>>
>>>> Ditto (get_unaligned_be16/le16 / le16/be16_to_cpup()).
>>>
>>> I have probably misunderstood something but I don't see why we should use
>>> 'endianess awareness' in drivers? I thought the IIO framework code takes
>>> care of the endianes conversions based on scan_type so each individual
>>> driver does not need to do that. That however has been just my assumption. I
>>> will need to check this. Thanks for pointing it out.
>>
>> The IIO core uses endianness field only once in iio_show_fixed_type() AFAICS.

Following is some hand waving and speculation after my quick code read. 
So, I may be utterly wrong in which case please do correct me...

Anyways, it seems to me that you're correct. The endianness field is 
only used by the IIO to build the channel information for user-space so 
that applications reading data can parse it. As far as I understand, the 
driver does not need to do the conversions for user-space, but the 
user-space tools should inspect the type information and do the 
conversion. I think it makes sense as user-space applications may be 
better equipped to do some maths. It also may be some applications do 
not want to spend cycles doing the conversion but the conversions can be 
done later "offline" for the captured raw data. So omitting conversion 
in the IIO driver kind of makes sense to me.

I haven't thoroughly looked (and I have never used) the in-kernel IIO 
APIs for getting the data. A quick look at the 
include/linux/iio/consumer.h allows me to assume the iio_chan_spec can 
be obtained by the consumer drivers. This should make the endianess 
information available for the consumer drivers as well. So, again, 
consumer drivers can parse the raw-format data themself.

I have this far only used the sysfs and iio_generic_buffer on a 
little-endian machine so I have had no issues with the little-endian 
data and I have only observed the code. Hence I can not really say if my 
reasoning is correct - or if it is how IIO has been designed to operate. 
But based on my quick study I don't see a need for the IIO driver to do 
endianess conversion to any other format but what is indicated by 
scan_type. Specifically for KX022A, the data is already 16B LE when read 
from the sensor. This is also advertised by scan_type so no conversion 
should be needed (unless, of course, I am mistaken :]).

>> And it does nothing with it. Maybe Jonathan can shed a light what is it for
>> (I mean the field)?
>>

I agree. It'd be great to listen to someone who actually knows what he 
is talking about and is not just guessing as I am ^_^;

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 v2 1/5] regulator: Add devm helpers for get and enable

[Jonathan Cameron]

On Mon, 10 Oct 2022 12:24:51 +0300
Matti Vaittinen <mazziesaccount@gmail.com> wrote:

> Hi Jonathan,
> 
> On 10/9/22 15:24, Jonathan Cameron wrote:
> > On Thu, 6 Oct 2022 19:17:39 +0300
> > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> >   
> >> On Thu, Oct 06, 2022 at 05:36:52PM +0300, Matti Vaittinen wrote:  
> >>> 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()  
> >>
> >> ...
> >>  
> >>> (cherry picked from commit b6058e052b842a19c8bb639798d8692cd0e7589f)  
> >>
> >> Not sure:
> >>   - why this is in the commit message
> >>   - what it points to, since
> >> $ git show b6058e052b842a19c8bb639798d8692cd0e7589f
> >>   fatal: bad object b6058e052b842a19c8bb639798d8692cd0e7589f  
> > 
> > These are now upstream in Linus' tree and in my testing branch.
> > I'd not normally advocate working on top of that (because I rebase it), but
> > if it is useful for this series go ahead.  
> 
> Thanks for the explanation :)
> 
> This series will conflict with my fixup series for triggered-buffer 
> attributes. Hence I though I might combine these two series into one if 
> I need to respin the fixup series. I thought of using the v6.1-rc1 when 
> it is out. (I think the 6.1-rc1 should not be that far away)
> 
> OTOH, I just read your another mail which told that there will be one 
> more driver which will conflict with the fixup coming in during this 
> cycle. If that driver lands in your tree before the fix - then I guess I 
> need to rebase the fixup series (and maybe this too) on top of your tree 
> + add conversion of this new driver. I don't think that would be the 
> testing branch though(?)
I'll be messy, but the majority of that fix series will need to go
in during rcX..

The last patch (refactoring one) will need to wait until those are
upstream and have worked there way around to be in my togreg branch
(I usually rebase that tree only after pull requests, but given this
 is going on I may do an earlier pull request than normal.)

That refactoring patch will probably need to cover the new driver.
I can do that as a fixup whilst applying it though once we get that far.

Jonathan

> 
> Yours
> 	-- Matti	
> 

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

[Jonathan Cameron]

On Tue, 11 Oct 2022 09:10:21 +0000
"Vaittinen, Matti" <Matti.Vaittinen@fi.rohmeurope.com> wrote:

> On 10/10/22 09:15, Andy Shevchenko wrote:
> > On Sun, Oct 09, 2022 at 01:33:51PM +0100, Jonathan Cameron wrote:  
> >> On Thu, 6 Oct 2022 21:32:11 +0300 Andy Shevchenko 
> >> <andriy.shevchenko@linux.intel.com> wrote:  
> >>> On Thu, Oct 06, 2022 at 05:38:14PM +0300, Matti Vaittinen wrote:  
> > 
> > ...
> >   
> >>>> +module_param(g_kx022a_use_buffer, bool, 0); 
> >>>> +MODULE_PARM_DESC(g_kx022a_use_buffer, +		 "Buffer samples. Use
> >>>> at own risk. Fifo must not overflow");  
> >>> 
> >>> Why?! We usually do not allow module parameters in the new code.  
> >> 
> >> Badly broken hardware - was my suggestion.  Alternatives if there 
> >> are usecases that need to use the fifo, but it can wedge hard in a
> >>  fashion that is impossible to prevent from the driver?  My gut 
> >> feeling is still drop the support entirely with a strong comment in
> >> the code that the hardware is broken in a fashion we don't know how
> >> to work around.  
> 
> I did some quick study regarding couple of other Kionix sensors. (like 
> KX122 and old KX022 - without the 'A'). It seems to me that the register 
> interfaces between many of the sensors are largely identical. Extending 
> the driver to support those seems pretty straightforward (scales and 
> resolution may need tweaking, as does the FIFO size) but register 
> contents and even offsets are largely identical.

Last kionix part I had was a kxsd9 and I don't recall that having a fifo
so obviously didn't hit this issue.

> 
> As said, it seems the Kionix sensors may have different size of internal 
> FIFOs, or even no FIFO at all. So, maybe we could provide a 
> "kionix,fifo-enable" flag (or even "kionix,fifo-size") from device-tree? 

For device where we don't have reports of this issue, that should be derived
from the compatible (or even better a whoami register if there is one).
The driver should know the fifo-size if it isn't discoverable.

> This would be a way to have the FIFO disabled by default and warn users 
> via dt-binding docs if they decide to explicitly enable the FIFO. 
> (Besides, I believe the FIFO is usable on at least some of the Kionix 
> sensors - because I've heard it is used on a few platforms).
> 
> This could give us safe defaults while not shutting the doors from those 
> who wish to use the FIFO. Sure we need a buy-in from Krzysztof / Rob, 
> but that may be less of an obstacle compared to the module param if Greg 
> is so strongly oppsoing those. (And the dt-property could also provide 
> some technical merites as these sensors seem to really have differencies 
> in FIFOs).

I'm dubious about having this for known broken parts - but I guess you
can propose it and see what the dt-maintainers say.

I don't want to see fifo size in the dt binding though.
> 
> > 
> > I also would drop this from upstream and if anybody curious, provide
> >  some kind of GitHub gist for that.  
> 
> Well, I think we all agree that downstream code hosted in some 
> unofficial github repositories are rarely that valuable. They're less 
> reliable, less tested, less reviewed, less secure and pretty much 
> impossible to maintain in a way that interested user could get a version 
> matching his preferred kernel.
> 
> There are reasons why I (people) keep sending the drivers to upstream - 
> and why some companies even spend $$ for that. Having this feature in 
> downstream repo is not nearly on same page for user's point of view as 
> is having the support upstream.

It's not really support if it comes with big warnings and potentially we
even taint the kernel of someone turns it on...

> 
> > Also it needs some communication
> >  with a vendor to clarify the things.  
> 
> I do communication with the vendor on a daily basis :] Nowadays Kionix 
> is part of ROHM, and Finland SWDC has been collaboration with Kionix 
> even before they "merged" (but I have not been part of the "sensor team" 
> back then).
> 
> Unfortunately, reaching the correct people inside the company is hard 
> and occasionally impossible - long story...

:)

Jonathan

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

[Jonathan Cameron]

...

> >   
> >>>>>> +	if (en)
> >>>>>> +		return regmap_set_bits(data->regmap, KX022A_REG_CNTL,
> >>>>>> +				       KX022A_MASK_DRDY);  
> >>>>>
> >>>>> I would put redundant 'else' here to have them on the same column, but
> >>>>> it's pity we don't have regmap_assign_bits() API (or whatever name you
> >>>>> can come up with) to hide this kind of code.  
> >>>>
> >>>> Eh, you mean you would put else here even though we return from the if? And
> >>>> then put another return in else, and no return outside the if/else?
> >>>>
> >>>> I definitely don't like the idea.
> >>>>
> >>>> We could probably use regmap_update_bits and ternary but in my opinion that
> >>>> would be just much less obvious. I do like the use of set/clear bits which
> >>>> also makes the meaning / "polarity" of bits really clear.  
> >>>
> >>> The idea behind is simple (and note that I'm usually on the side of _removing_
> >>> redundancy)
> >>>
> >>> 	if (en)
> >>> 		return regmap_set_bits(data->regmap, KX022A_REG_CNTL,
> >>> 				       KX022A_MASK_DRDY);
> >>> 	else
> >>> 		return regmap_clear_bits(data->regmap, KX022A_REG_CNTL,
> >>> 					 ...
> >>>
> >>> Because the branches are semantically tighten to each other. But it's not
> >>> a big deal to me.  
> >>
> >> What I do not really like in above example is that we never reach the
> >> end of function body.  
> > 
> > What do you mean by that? Compiler does warn or what?  
> 
> I don't know if compiler warns about it as I didn't test it. Now that 
> you mentioned it, I think I have seen such warnings from a compiler or 
> some static analyzer (klocwork?) in the past though. What I mean is that:
> 
> int foo()
> {
> 	if () {
> 		...
> 		return 1;
> 	} else {
> 		...
> 		return 2;
> 	}
> }
> 

For reference, this is the one I'd write if both options are good
(or both are bad) and we don't need to worry about reducing indent
for readability.

However, I long since decided this was trivial enough not to
comment on it in the code of others!

> construct makes mistakes like:
> 
> int foo()
> {
> 	if () {
> 		...
> 		return 1;
> 	} else {
> 		...
> 		return 2;
> 	}
> 
> 	...
> 
> 	return 0;
> }

That should given unreachable code warning unless you've really managed
to confuse the compiler  / static analysis tooling.

> 
> easy to make. When one uses:
> 
> int foo()
> {
> 	if () {
> 		...
> 		return 1;
> 	}
> 
> 	...
> 	return 2;
> }
> 
> to begin with there is zero room for such confusion.
> 
> >   
> >> It is against the expectation - and adding the
> >> else has no real meaning when if returns. I actually think that
> >> checkpatch could even warn about that construct.  
> > 
> > No way we ever accept such a thing for checkpatch because it's subjective  
> 
> Eh. Are you telling me that there is no subjective stuff in checkpatch? ;)
> 
> > (very dependant on the code piece). OTOH the proposed pattern is used in
> > many places and left like that in places where I cleaned up the 'else',
> > to leave the semantic tights with the above lines).
> >   
> >>>>>> +	return regmap_clear_bits(data->regmap, KX022A_REG_CNTL,
> >>>>>> +				 KX022A_MASK_DRDY);  
> > 
> > I see that we have a strong disagreement here. I leave it to maintainers.  
> 
> 
> Okay, let's hear what others have to say here.

Non answer above ;)

Time for the old "Don't let perfect be the enemy of good!"


> 
> Thanks for all the input this far.
> 
> Best Regards
> 	-- Matti
> 

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

[Jonathan Cameron]

On Wed, 12 Oct 2022 10:40:38 +0300
Matti Vaittinen <mazziesaccount@gmail.com> wrote:

> On 10/10/22 16:20, Vaittinen, Matti wrote:
> > On 10/10/22 14:58, Andy Shevchenko wrote:  
> >> On Mon, Oct 10, 2022 at 12:12:34PM +0300, Matti Vaittinen wrote:
> >> ...
> >>  
> >>>>> +	ret = regmap_bulk_read(data->regmap, chan->address, &data->buffer,
> >>>>> +			       sizeof(s16));  
> >>  
> >>>> No endianess awareness (sizeof __le16 / __be16)  
> >>  
> >>>>> +	if (ret)
> >>>>> +		return ret;
> >>>>> +
> >>>>> +	*val = data->buffer[0];  
> >>>>
> >>>> Ditto (get_unaligned_be16/le16 / le16/be16_to_cpup()).  
> >>>
> >>> I have probably misunderstood something but I don't see why we should use
> >>> 'endianess awareness' in drivers? I thought the IIO framework code takes
> >>> care of the endianes conversions based on scan_type so each individual
> >>> driver does not need to do that. That however has been just my assumption. I
> >>> will need to check this. Thanks for pointing it out.  
> >>
> >> The IIO core uses endianness field only once in iio_show_fixed_type() AFAICS.  
> 
> Following is some hand waving and speculation after my quick code read. 
> So, I may be utterly wrong in which case please do correct me...
> 
> Anyways, it seems to me that you're correct. The endianness field is 
> only used by the IIO to build the channel information for user-space so 
> that applications reading data can parse it. As far as I understand, the 
> driver does not need to do the conversions for user-space, but the 
> user-space tools should inspect the type information and do the 
> conversion. I think it makes sense as user-space applications may be 
> better equipped to do some maths. It also may be some applications do 
> not want to spend cycles doing the conversion but the conversions can be 
> done later "offline" for the captured raw data. So omitting conversion 
> in the IIO driver kind of makes sense to me.

That was indeed the original reasonining for buffered data path
(note the endian marker is for scans only which only apply in buffered
 / chardev case).

It's less obvious for the sysfs path as that's inherently slow.
We could have made this a problem for the IIO core, but we didn't :)

> 
> I haven't thoroughly looked (and I have never used) the in-kernel IIO 
> APIs for getting the data. A quick look at the 
> include/linux/iio/consumer.h allows me to assume the iio_chan_spec can 
> be obtained by the consumer drivers. This should make the endianess 
> information available for the consumer drivers as well. So, again, 
> consumer drivers can parse the raw-format data themself.

yes consumers should be be endian aware if they are using the
callback buffer route to get the data.  Now you mention it, we
may well have cases where that isn't handled correctly.
There are few enough users of that interface that it might well work
by coincidence rather than design. oops.

> 
> I have this far only used the sysfs and iio_generic_buffer on a 
> little-endian machine so I have had no issues with the little-endian 
> data and I have only observed the code. Hence I can not really say if my 
> reasoning is correct - or if it is how IIO has been designed to operate. 
> But based on my quick study I don't see a need for the IIO driver to do 
> endianess conversion to any other format but what is indicated by 
> scan_type. Specifically for KX022A, the data is already 16B LE when read 
> from the sensor. This is also advertised by scan_type so no conversion 
> should be needed (unless, of course, I am mistaken :]).

Ah. I'd missed that. Data storage should reflect the read back endianness
and for the read_raw path you need to perform the conversion in driver
(but not the high perf push to buffers path).

Sure we could probably have handled read_raw in tree as well but we didn't
and probably too late to sensibly fix that now.  One of many things we'd
probably do differently if we were starting again.

J

> 
> >> And it does nothing with it. Maybe Jonathan can shed a light what is it for
> >> (I mean the field)?
> >>  
> 
> I agree. It'd be great to listen to someone who actually knows what he 
> is talking about and is not just guessing as I am ^_^;
> 
> Yours,
> 	-- Matti
> 

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

[Matti Vaittinen]

On 10/14/22 16:42, Jonathan Cameron wrote:
> On Wed, 12 Oct 2022 10:40:38 +0300
> Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> 
>> On 10/10/22 16:20, Vaittinen, Matti wrote:
>>> On 10/10/22 14:58, Andy Shevchenko wrote:
>>>> On Mon, Oct 10, 2022 at 12:12:34PM +0300, Matti Vaittinen wrote:
>>>> ...
>>>>   
>>>>>>> +	ret = regmap_bulk_read(data->regmap, chan->address, &data->buffer,
>>>>>>> +			       sizeof(s16));
>>>>   
>>>>>> No endianess awareness (sizeof __le16 / __be16)
>>>>   
>>>>>>> +	if (ret)
>>>>>>> +		return ret;
>>>>>>> +
>>>>>>> +	*val = data->buffer[0];
>>>>>>
>>>>>> Ditto (get_unaligned_be16/le16 / le16/be16_to_cpup()).
>>>>>
>>>>> I have probably misunderstood something but I don't see why we should use
>>>>> 'endianess awareness' in drivers? I thought the IIO framework code takes
>>>>> care of the endianes conversions based on scan_type so each individual
>>>>> driver does not need to do that. That however has been just my assumption. I
>>>>> will need to check this. Thanks for pointing it out.
>>>>
>>>> The IIO core uses endianness field only once in iio_show_fixed_type() AFAICS.
>>
>> Following is some hand waving and speculation after my quick code read.
>> So, I may be utterly wrong in which case please do correct me...
>>
>> Anyways, it seems to me that you're correct. The endianness field is
>> only used by the IIO to build the channel information for user-space so
>> that applications reading data can parse it. As far as I understand, the
>> driver does not need to do the conversions for user-space, but the
>> user-space tools should inspect the type information and do the
>> conversion. I think it makes sense as user-space applications may be
>> better equipped to do some maths. It also may be some applications do
>> not want to spend cycles doing the conversion but the conversions can be
>> done later "offline" for the captured raw data. So omitting conversion
>> in the IIO driver kind of makes sense to me.
> 
> That was indeed the original reasonining for buffered data path
> (note the endian marker is for scans only which only apply in buffered
>   / chardev case).

So, in a case where we "push_to_buffers" the data, we can leave the data 
to use the endianess we advertise via endianess info field?

> It's less obvious for the sysfs path as that's inherently slow.
> We could have made this a problem for the IIO core, but we didn't :)

But again, as far as I understood, the user-space is still expected to 
read the sysfs field for "scan_elements/in_accel_<channel>_type"? I 
guess it would be confusing to say "le:s16/16>>0" there while returning 
CPU native endianess values from sysfs files?

>> I haven't thoroughly looked (and I have never used) the in-kernel IIO
>> APIs for getting the data. A quick look at the
>> include/linux/iio/consumer.h allows me to assume the iio_chan_spec can
>> be obtained by the consumer drivers. This should make the endianess
>> information available for the consumer drivers as well. So, again,
>> consumer drivers can parse the raw-format data themself.
> 
> yes consumers should be be endian aware if they are using the
> callback buffer route to get the data.  Now you mention it, we
> may well have cases where that isn't handled correctly.
> There are few enough users of that interface that it might well work
> by coincidence rather than design. oops.
> 
>>
>> I have this far only used the sysfs and iio_generic_buffer on a
>> little-endian machine so I have had no issues with the little-endian
>> data and I have only observed the code. Hence I can not really say if my
>> reasoning is correct - or if it is how IIO has been designed to operate.
>> But based on my quick study I don't see a need for the IIO driver to do
>> endianess conversion to any other format but what is indicated by
>> scan_type. Specifically for KX022A, the data is already 16B LE when read
>> from the sensor. This is also advertised by scan_type so no conversion
>> should be needed (unless, of course, I am mistaken :]).
> 
> Ah. I'd missed that. Data storage should reflect the read back endianness
> and for the read_raw path you need to perform the conversion in driver
> (but not the high perf push to buffers path).

Oh, really? I think it might be confusing to say "le:s16/16>>0" in 
"scan_elements/in_accel_<channel>_type" but return something else from 
the in_accel_<channel>_raw. Especially the "raw" word at the end of the 
file signals the data is in non converted raw format.

I take your word for that if you say this is what the user-space 
expects, it just is not what I did expect. Well, I do very little work 
on the user-space these days ;) Still just to be on safe side - do you 
mean I should convert the data returned from read_raw to the CPU endianess?

> Sure we could probably have handled read_raw in tree as well but we didn't
> and probably too late to sensibly fix that now.  One of many things we'd
> probably do differently if we were starting again.

Well, this is pretty usual story :) Predicting the future is hard. My 
crystal ball ran out of batteries a long ago ;)

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 v2 4/5] iio: accel: Support Kionix/ROHM KX022A accelerometer

[Matti Vaittinen]

[-- Attachment #1.1.1: Type: text/plain, Size: 3517 bytes --]

On 10/14/22 16:22, Jonathan Cameron wrote:
> On Tue, 11 Oct 2022 09:10:21 +0000
> "Vaittinen, Matti" <Matti.Vaittinen@fi.rohmeurope.com> wrote:
> 
>> On 10/10/22 09:15, Andy Shevchenko wrote:
>>> On Sun, Oct 09, 2022 at 01:33:51PM +0100, Jonathan Cameron wrote:
>>>> On Thu, 6 Oct 2022 21:32:11 +0300 Andy Shevchenko
>>>> <andriy.shevchenko@linux.intel.com> wrote:
>>>>> On Thu, Oct 06, 2022 at 05:38:14PM +0300, Matti Vaittinen wrote:
>>>
>>> ...
>>>    
>>>>>> +module_param(g_kx022a_use_buffer, bool, 0);
>>>>>> +MODULE_PARM_DESC(g_kx022a_use_buffer, +		 "Buffer samples. Use
>>>>>> at own risk. Fifo must not overflow");
// snip

>> This would be a way to have the FIFO disabled by default and warn users
>> via dt-binding docs if they decide to explicitly enable the FIFO.
>> (Besides, I believe the FIFO is usable on at least some of the Kionix
>> sensors - because I've heard it is used on a few platforms).
>>
>> This could give us safe defaults while not shutting the doors from those
>> who wish to use the FIFO. Sure we need a buy-in from Krzysztof / Rob,
>> but that may be less of an obstacle compared to the module param if Greg
>> is so strongly oppsoing those. (And the dt-property could also provide
>> some technical merites as these sensors seem to really have differencies
>> in FIFOs).
> 
> I'm dubious about having this for known broken parts - but I guess you
> can propose it and see what the dt-maintainers say.
> 
> I don't want to see fifo size in the dt binding though.

// snip

>>> Also it needs some communication
>>>   with a vendor to clarify the things.
>>
>> I do communication with the vendor on a daily basis :] Nowadays Kionix
>> is part of ROHM, and Finland SWDC has been collaboration with Kionix
>> even before they "merged" (but I have not been part of the "sensor team"
>> back then).
>>
>> Unfortunately, reaching the correct people inside the company is hard
>> and occasionally impossible - long story...
> 
> :)

Just a note. I have done some further investigation (further testing 
combined with tracing the SPI lines) and also got some answer(s) from 
the ASIC designers. First thing is that the underlying FIFO is 258 bytes 
and can hold up-to 43 HiRes samples. This is also fixed in the recent 
data-sheet. The register informing how many bytes of data is stored in 
FIFO is still only 8bits. When FIFO is full, the magic value 255 is used 
to indicate the 258 bytes of data. This explains the strange 42,5 
samples (255 bytes) of data being reported.

Furthermore, I've noticed that the FIFO appearing to be "stuck", eg. 
amount of data stored in FIFO not decreasing when data is read is 100% 
reproducible. The condition is that the PC1 bit (accelerometer state 
control) is toggled before the FIFO is read. The issue does not seem to 
be reproducible if the PC1 is not touched. I have also asked if this is 
a known limitation.

Anyways, it seems we have a solution to the problem. We simply handle 
the case when 255 bytes is reported correctly (by reading 258 bytes of 
valid data) - and we prevent changing the sensor configuration when FIFO 
is enabled.

I will implement these changes to the v3 and drop both the module-param 
and the dt-property. Sorry for the hassle and thanks for the 
patience/support!

Yours
	-- Matti

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

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


[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 5335 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

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

[Andy Shevchenko]

On Tue, Oct 18, 2022 at 02:27:20PM +0300, Matti Vaittinen wrote:
> On 10/14/22 16:22, Jonathan Cameron wrote:
> > On Tue, 11 Oct 2022 09:10:21 +0000
> > "Vaittinen, Matti" <Matti.Vaittinen@fi.rohmeurope.com> wrote:
> > > On 10/10/22 09:15, Andy Shevchenko wrote:
> > > > On Sun, Oct 09, 2022 at 01:33:51PM +0100, Jonathan Cameron wrote:
> > > > > On Thu, 6 Oct 2022 21:32:11 +0300 Andy Shevchenko
> > > > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > > > On Thu, Oct 06, 2022 at 05:38:14PM +0300, Matti Vaittinen wrote:

...

> > > > > > > +module_param(g_kx022a_use_buffer, bool, 0);
> > > > > > > +MODULE_PARM_DESC(g_kx022a_use_buffer, +		 "Buffer samples. Use
> > > > > > > at own risk. Fifo must not overflow");
> // snip
> 
> > > This would be a way to have the FIFO disabled by default and warn users
> > > via dt-binding docs if they decide to explicitly enable the FIFO.
> > > (Besides, I believe the FIFO is usable on at least some of the Kionix
> > > sensors - because I've heard it is used on a few platforms).
> > > 
> > > This could give us safe defaults while not shutting the doors from those
> > > who wish to use the FIFO. Sure we need a buy-in from Krzysztof / Rob,
> > > but that may be less of an obstacle compared to the module param if Greg
> > > is so strongly oppsoing those. (And the dt-property could also provide
> > > some technical merites as these sensors seem to really have differencies
> > > in FIFOs).
> > 
> > I'm dubious about having this for known broken parts - but I guess you
> > can propose it and see what the dt-maintainers say.
> > 
> > I don't want to see fifo size in the dt binding though.
> 
> // snip
> 
> > > > Also it needs some communication
> > > >   with a vendor to clarify the things.
> > > 
> > > I do communication with the vendor on a daily basis :] Nowadays Kionix
> > > is part of ROHM, and Finland SWDC has been collaboration with Kionix
> > > even before they "merged" (but I have not been part of the "sensor team"
> > > back then).
> > > 
> > > Unfortunately, reaching the correct people inside the company is hard
> > > and occasionally impossible - long story...
> > 
> > :)
> 
> Just a note. I have done some further investigation (further testing
> combined with tracing the SPI lines) and also got some answer(s) from the
> ASIC designers. First thing is that the underlying FIFO is 258 bytes and can
> hold up-to 43 HiRes samples. This is also fixed in the recent data-sheet.
> The register informing how many bytes of data is stored in FIFO is still
> only 8bits. When FIFO is full, the magic value 255 is used to indicate the
> 258 bytes of data. This explains the strange 42,5 samples (255 bytes) of
> data being reported.
> 
> Furthermore, I've noticed that the FIFO appearing to be "stuck", eg. amount
> of data stored in FIFO not decreasing when data is read is 100%
> reproducible. The condition is that the PC1 bit (accelerometer state
> control) is toggled before the FIFO is read. The issue does not seem to be
> reproducible if the PC1 is not touched. I have also asked if this is a known
> limitation.
> 
> Anyways, it seems we have a solution to the problem. We simply handle the
> case when 255 bytes is reported correctly (by reading 258 bytes of valid
> data) - and we prevent changing the sensor configuration when FIFO is
> enabled.
> 
> I will implement these changes to the v3 and drop both the module-param and
> the dt-property. Sorry for the hassle and thanks for the patience/support!

This is very good news, Matti, thanks for pushing this forward and finding
better solution!

-- 
With Best Regards,
Andy Shevchenko

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

[Jonathan Cameron]

On Tue, 18 Oct 2022 14:10:59 +0300
Matti Vaittinen <mazziesaccount@gmail.com> wrote:

> On 10/14/22 16:42, Jonathan Cameron wrote:
> > On Wed, 12 Oct 2022 10:40:38 +0300
> > Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> >   
> >> On 10/10/22 16:20, Vaittinen, Matti wrote:  
> >>> On 10/10/22 14:58, Andy Shevchenko wrote:  
> >>>> On Mon, Oct 10, 2022 at 12:12:34PM +0300, Matti Vaittinen wrote:
> >>>> ...
> >>>>     
> >>>>>>> +	ret = regmap_bulk_read(data->regmap, chan->address, &data->buffer,
> >>>>>>> +			       sizeof(s16));  
> >>>>     
> >>>>>> No endianess awareness (sizeof __le16 / __be16)  
> >>>>     
> >>>>>>> +	if (ret)
> >>>>>>> +		return ret;
> >>>>>>> +
> >>>>>>> +	*val = data->buffer[0];  
> >>>>>>
> >>>>>> Ditto (get_unaligned_be16/le16 / le16/be16_to_cpup()).  
> >>>>>
> >>>>> I have probably misunderstood something but I don't see why we should use
> >>>>> 'endianess awareness' in drivers? I thought the IIO framework code takes
> >>>>> care of the endianes conversions based on scan_type so each individual
> >>>>> driver does not need to do that. That however has been just my assumption. I
> >>>>> will need to check this. Thanks for pointing it out.  
> >>>>
> >>>> The IIO core uses endianness field only once in iio_show_fixed_type() AFAICS.  
> >>
> >> Following is some hand waving and speculation after my quick code read.
> >> So, I may be utterly wrong in which case please do correct me...
> >>
> >> Anyways, it seems to me that you're correct. The endianness field is
> >> only used by the IIO to build the channel information for user-space so
> >> that applications reading data can parse it. As far as I understand, the
> >> driver does not need to do the conversions for user-space, but the
> >> user-space tools should inspect the type information and do the
> >> conversion. I think it makes sense as user-space applications may be
> >> better equipped to do some maths. It also may be some applications do
> >> not want to spend cycles doing the conversion but the conversions can be
> >> done later "offline" for the captured raw data. So omitting conversion
> >> in the IIO driver kind of makes sense to me.  
> > 
> > That was indeed the original reasonining for buffered data path
> > (note the endian marker is for scans only which only apply in buffered
> >   / chardev case).  
> 
> So, in a case where we "push_to_buffers" the data, we can leave the data 
> to use the endianess we advertise via endianess info field?

Exactly.

> 
> > It's less obvious for the sysfs path as that's inherently slow.
> > We could have made this a problem for the IIO core, but we didn't :)  
> 
> But again, as far as I understood, the user-space is still expected to 
> read the sysfs field for "scan_elements/in_accel_<channel>_type"? I 
> guess it would be confusing to say "le:s16/16>>0" there while returning 
> CPU native endianess values from sysfs files?

Agreed that it is probably less than ideal but that's what the interface
is.  scan_elements refers to the "scan elements"  channels read via sysfs
files are not scan elements - scan's are only relevant to buffered readback.

> 
> >> I haven't thoroughly looked (and I have never used) the in-kernel IIO
> >> APIs for getting the data. A quick look at the
> >> include/linux/iio/consumer.h allows me to assume the iio_chan_spec can
> >> be obtained by the consumer drivers. This should make the endianess
> >> information available for the consumer drivers as well. So, again,
> >> consumer drivers can parse the raw-format data themself.  
> > 
> > yes consumers should be be endian aware if they are using the
> > callback buffer route to get the data.  Now you mention it, we
> > may well have cases where that isn't handled correctly.
> > There are few enough users of that interface that it might well work
> > by coincidence rather than design. oops.
> >   
> >>
> >> I have this far only used the sysfs and iio_generic_buffer on a
> >> little-endian machine so I have had no issues with the little-endian
> >> data and I have only observed the code. Hence I can not really say if my
> >> reasoning is correct - or if it is how IIO has been designed to operate.
> >> But based on my quick study I don't see a need for the IIO driver to do
> >> endianess conversion to any other format but what is indicated by
> >> scan_type. Specifically for KX022A, the data is already 16B LE when read
> >> from the sensor. This is also advertised by scan_type so no conversion
> >> should be needed (unless, of course, I am mistaken :]).  
> > 
> > Ah. I'd missed that. Data storage should reflect the read back endianness
> > and for the read_raw path you need to perform the conversion in driver
> > (but not the high perf push to buffers path).  
> 
> Oh, really? I think it might be confusing to say "le:s16/16>>0" in 
> "scan_elements/in_accel_<channel>_type" but return something else from 
> the in_accel_<channel>_raw. Especially the "raw" word at the end of the 
> file signals the data is in non converted raw format.
> 
> I take your word for that if you say this is what the user-space 
> expects, it just is not what I did expect. Well, I do very little work 
> on the user-space these days ;) Still just to be on safe side - do you 
> mean I should convert the data returned from read_raw to the CPU endianess?

yes.

> 
> > Sure we could probably have handled read_raw in tree as well but we didn't
> > and probably too late to sensibly fix that now.  One of many things we'd
> > probably do differently if we were starting again.  
> 
> Well, this is pretty usual story :) Predicting the future is hard. My 
> crystal ball ran out of batteries a long ago ;)

:)

> 
> Best Regards
> 	-- Matti
>