iio: Support ROHM/Kionix kx022a

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)

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

(quoted diff) +2 -0

+	regulator_disable(r);
+}

Quoted 6 lines · Matti Vaittinen

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()

...

Quoted 1 line

(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

Quoted 4 lines

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?

...

drivers/regulator/devres.c struct regulator *devm_regulator_get_exclusive(struct device *dev, · line 75

Quoted 3 lines

drivers/regulator/devres.c +3 -0

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

Cast is not needed.

Quoted 2 lines

(quoted diff) +2 -0

+	regulator_disable(r);
+}  

Quoted 1 line · On Thu, 6 Oct 2022 19:17:39 +0300 Andy Shevchenko…

On Thu, Oct 06, 2022 at 05:36:52PM +0300, Matti Vaittinen wrote:

Quoted 7 lines

Quoted 1 line

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

Quoted 5 lines

Quoted 3 lines

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?

Quoted 10 lines · On Thu, 6 Oct 2022 19:17:39 +0300 Andy Shevchenko…

Quoted 6 lines · Matti Vaittinen

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()

...

Quoted 7 lines

Quoted 1 line

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

Hi Jonathan,

Quoted 21 lines · Jonathan Cameron

Quoted 10 lines · On Thu, 6 Oct 2022 19:17:39 +0300 Andy Shevchenko…

Quoted 6 lines · Matti Vaittinen

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()

...

Quoted 7 lines

Quoted 1 line

(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  

Quoted 3 lines

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

Quoted 6 lines · Matti Vaittinen

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()

...

Quoted 1 line

(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

Quoted 3 lines

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?

Quoted 7 lines · Andy Shevchenko

Quoted 6 lines · Matti Vaittinen

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()

Quoted 12 lines

Quoted 1 line

(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

Quoted 3 lines

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

Quoted 1 line

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.

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.

Documentation/devicetree/bindings/iio/accel/kionix,kx022a.yaml new file +9 -0

+
+properties:
+  compatible:
+    const: kionix,kx022a
+
+  reg:
+    maxItems: 1
+
+  interrupts:

Quoted 5 lines · Matti Vaittinen

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.

Quoted 13 lines

Documentation/devicetree/bindings/iio/accel/kionix,kx022a.yaml new file +13 -0

+
+properties:
+  compatible:
+    const: kionix,kx022a
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    minItems: 1
+    maxItems: 2
+
+  interrupt-names:

Documentation/devicetree/bindings/iio/accel/kionix,kx022a.yaml line 34

Quoted 6 lines

Documentation/devicetree/bindings/iio/accel/kionix,kx022a.yaml new file +6 -0

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

Hi dee Ho Krzysztof,

Quoted 22 lines · Krzysztof Kozlowski

Quoted 5 lines · Matti Vaittinen

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.

Quoted 13 lines

Documentation/devicetree/bindings/iio/accel/kionix,kx022a.yaml new file +13 -0

+
+properties:
+  compatible:
+    const: kionix,kx022a
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    minItems: 1
+    maxItems: 2
+
+  interrupt-names:

Quoted 8 lines

Documentation/devicetree/bindings/iio/accel/kionix,kx022a.yaml line 34

Quoted 6 lines

Documentation/devicetree/bindings/iio/accel/kionix,kx022a.yaml new file +6 -0

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

Quoted 6 lines

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 :)

Quoted 47 lines · On Thu, 6 Oct 2022 18:32:22 +0300 Matti Vaittinen <[email protected]>…

Hi dee Ho Krzysztof,

Quoted 22 lines · Krzysztof Kozlowski

Quoted 5 lines · Matti Vaittinen

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.

Quoted 13 lines

Documentation/devicetree/bindings/iio/accel/kionix,kx022a.yaml new file +13 -0

+
+properties:
+  compatible:
+    const: kionix,kx022a
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    minItems: 1
+    maxItems: 2
+
+  interrupt-names:

Quoted 8 lines

Documentation/devicetree/bindings/iio/accel/kionix,kx022a.yaml line 34

Quoted 6 lines

Documentation/devicetree/bindings/iio/accel/kionix,kx022a.yaml new file +6 -0

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

Quoted 6 lines

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

Quoted 2 lines

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.

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_i2c_driver(kx022a_i2c_driver);

(quoted diff) +2 -0

+	return kx022a_probe_internal(dev);
+}

+module_spi_driver(kx022a_spi_driver);

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

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

+struct kx022a_data;

drivers/iio/accel/kionix-kx022a.c new file +14 -0

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

+};

+};

drivers/iio/accel/kionix-kx022a.c new file +11 -0

+#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),	\

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

+ */

+};

+#define KX022A_DEFAULT_PERIOD_NS (20 * MEGA)

drivers/iio/accel/kionix-kx022a.c new file +11 -0

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

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

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

(quoted diff) +2 -0

+	return IIO_VAL_INT;
+}

+}

+};

(quoted diff) +1 -0

+	 */

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

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

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

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

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

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

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

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

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

+	return kx022a_fifo_enable(data);

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

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

drivers/iio/accel/kionix-kx022a.c new file +62 -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,

+#include <linux/regmap.h>

Quoted 4 lines · Matti Vaittinen

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.

Quoted 3 lines

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

Quoted 5 lines · Andy Shevchenko

Quoted 4 lines · Matti Vaittinen

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.

[]

Quoted 6 lines

Quoted 3 lines

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

Quoted 5 lines · Andy Shevchenko

Quoted 4 lines · Matti Vaittinen

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.

[]

Quoted 6 lines

Quoted 3 lines

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

Quoted 22 lines · Matti Vaittinen

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.

...

...

drivers/iio/accel/kionix-kx022a.c line 1134

Quoted 3 lines

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

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

Quoted 1 line · On Thu, 6 Oct 2022 21:32:11 +0300 Andy Shevchenko…

On Thu, Oct 06, 2022 at 05:38:14PM +0300, Matti Vaittinen wrote:

Quoted 5 lines

drivers/iio/accel/kionix-kx022a.c line 1134

Quoted 3 lines

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

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

Quoted 3 lines · Jonathan Cameron

Quoted 1 line · On Thu, 6 Oct 2022 21:32:11 +0300 Andy Shevchenko…

On Thu, Oct 06, 2022 at 05:38:14PM +0300, Matti Vaittinen wrote:

...

Quoted 6 lines

Quoted 5 lines

Quoted 3 lines

(quoted diff) +2 -0

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

Quoted 6 lines

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.

Quoted 13 lines · Andy Shevchenko

Quoted 3 lines · Jonathan Cameron

Quoted 1 line · On Thu, 6 Oct 2022 21:32:11 +0300 Andy Shevchenko…

On Thu, Oct 06, 2022 at 05:38:14PM +0300, Matti Vaittinen wrote:

...

Quoted 6 lines

Quoted 5 lines

Quoted 3 lines

(quoted diff) +2 -0

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

Quoted 6 lines

Quoted 6 lines

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

Quoted 4 lines

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.

Quoted 3 lines

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

Quoted 4 lines · Matti

Quoted 3 lines · Andy Shevchenko

Quoted 2 lines · Jonathan Cameron

On Thu, 6 Oct 2022 21:32:11 +0300 Andy Shevchenko
<[email protected]> wrote:

Quoted 7 lines

Quoted 7 lines

Quoted 1 line

Quoted 1 line · Matti Vaittinen

...

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

(quoted diff) +2 -0

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

Quoted 11 lines

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.

Quoted 8 lines

Quoted 2 lines

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

Quoted 2 lines

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

:)

Quoted 7 lines · Jonathan Cameron

Quoted 5 lines · Matti

Quoted 4 lines · Andy Shevchenko

Quoted 3 lines · Jonathan Cameron

Quoted 1 line · On Thu, 6 Oct 2022 21:32:11 +0300 Andy Shevchenko…

On Thu, Oct 06, 2022 at 05:38:14PM +0300, Matti Vaittinen wrote:

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

(quoted diff) +2 -0

+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

Quoted 12 lines

Quoted 11 lines

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

Quoted 4 lines

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

Quoted 8 lines

Quoted 8 lines

Quoted 2 lines

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

Quoted 4 lines

Quoted 2 lines

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!

Quoted 22 lines · Matti Vaittinen

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.

...

drivers/iio/accel/Kconfig config IIO_ST_ACCEL_SPI_3AXIS · line 422

Quoted 8 lines

drivers/iio/accel/Kconfig +8 -0

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

drivers/iio/accel/Kconfig config IIO_ST_ACCEL_SPI_3AXIS · line 431

Quoted 8 lines

drivers/iio/accel/Kconfig +8 -0

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

drivers/iio/accel/kionix-kx022a-spi.c line 18

Quoted 5 lines

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

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

Review

Quoted 1 line

(quoted diff) +1 -0

+	}

Ditto (blank line).

...

drivers/iio/accel/kionix-kx022a.c line 14

Quoted 5 lines

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

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

drivers/iio/accel/kionix-kx022a.c line 31

Quoted 3 lines

+ */

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

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

drivers/iio/accel/kionix-kx022a.c line 35

Quoted 3 lines

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

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

drivers/iio/accel/kionix-kx022a.c line 146

Quoted 11 lines

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

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

Quoted 2 lines

+struct kx022a_data;

Why?

drivers/iio/accel/kionix-kx022a.c line 156

Quoted 7 lines

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

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

drivers/iio/accel/kionix-kx022a.c line 162

Quoted 8 lines

drivers/iio/accel/kionix-kx022a.c new file +6 -0

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

Quoted 14 lines

drivers/iio/accel/kionix-kx022a.c new file +14 -0

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

...

Quoted 5 lines

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

Quoted 2 lines

+ */

...

drivers/iio/accel/kionix-kx022a.c line 247

Quoted 10 lines

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

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

Quoted 1 line

+};

...

drivers/iio/accel/kionix-kx022a.c line 251

Quoted 2 lines

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

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

...

drivers/iio/accel/kionix-kx022a.c line 378

Quoted 1 line

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

+		int n = ARRAY_SIZE(kx022a_accel_samp_freq_table);

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

Review

Quoted 3 lines

(quoted diff) +2 -0

+
+		while (n-- > 0)

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

drivers/iio/accel/kionix-kx022a.c line 384

Quoted 6 lines

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

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

Quoted 4 lines

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

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

...

drivers/iio/accel/kionix-kx022a.c line 449

Quoted 8 lines

drivers/iio/accel/kionix-kx022a.c new file +8 -0

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

drivers/iio/accel/kionix-kx022a.c line 453

Quoted 4 lines

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

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

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

drivers/iio/accel/kionix-kx022a.c line 525

Quoted 8 lines

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

+	return IIO_VAL_INT;
+}
 
...
 
+	if (val > KX022A_FIFO_LENGTH)
+		val = KX022A_FIFO_LENGTH;

max() from minmax.h?

...

drivers/iio/accel/kionix-kx022a.c line 579

Quoted 3 lines

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

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

Period!

Quoted 2 lines

(quoted diff) +1 -0

+	 */

...

drivers/iio/accel/kionix-kx022a.c line 583

Quoted 1 line

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

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

GENMASK() ?

...

drivers/iio/accel/kionix-kx022a.c line 729

Quoted 5 lines

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

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

Quoted 4 lines

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

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

...

Quoted 2 lines

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

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

Quoted 1 line

+

No need in blank line in such cases.

Review

Quoted 2 lines

(quoted diff) +2 -0

+		return -EINVAL;
+	}

Maybe you want to place it here instead?

Quoted 1 line

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

+	return kx022a_fifo_enable(data);

...

drivers/iio/accel/kionix-kx022a.c line 969

Quoted 5 lines

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

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

...

Quoted 9 lines

drivers/iio/accel/kionix-kx022a.c new file +9 -0

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

drivers/iio/accel/kionix-kx022a.c line 1008

Quoted 1 line

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

+	if (WARN_ON(!dev))

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

Quoted 6 lines

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

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

drivers/iio/accel/kionix-kx022a.c line 1051

Quoted 10 lines

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

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

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.

drivers/iio/accel/kionix-kx022a.c line 1134

Quoted 4 lines

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

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

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 :)

Quoted 1 line · Andy Shevchenko

On Thu, Oct 06, 2022 at 05:38:14PM +0300, Matti Vaittinen wrote:

Quoted 4 lines

Quoted 4 lines

drivers/iio/accel/Kconfig +4 -0

+config IIO_KX022A_SPI
+	tristate "Kionix KX022A tri-axis digital accelerometer"
+	depends on I2C
+	select IIO_KX022A

Quoted 8 lines

drivers/iio/accel/Kconfig config IIO_ST_ACCEL_SPI_3AXIS · line 422

Quoted 4 lines

drivers/iio/accel/Kconfig +4 -0

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

Quoted 1 line

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?

Quoted 1 line

Can you fix I²C driver to follow this style?

I need to respin this anyways, so yes.

Quoted 3 lines

Review

Quoted 1 line

(quoted diff) +1 -0

+	}

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.

Quoted 7 lines

drivers/iio/accel/kionix-kx022a.c line 14

Quoted 5 lines

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

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

Quoted 5 lines

drivers/iio/accel/kionix-kx022a.c line 31

Quoted 2 lines

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

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

Quoted 8 lines

drivers/iio/accel/kionix-kx022a.c line 35

Quoted 2 lines

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

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

Quoted 4 lines

Quoted 4 lines

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

+const struct regmap_config kx022a_regmap = {
+	.reg_bits = 8,
+	.val_bits = 8,
+	.volatile_table = &kx022a_volatile_regs,

Quoted 8 lines

drivers/iio/accel/kionix-kx022a.c line 146

Quoted 6 lines

drivers/iio/accel/kionix-kx022a.c new file +6 -0

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

Quoted 9 lines

drivers/iio/accel/kionix-kx022a.c line 156

Quoted 6 lines

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

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

Quoted 9 lines

drivers/iio/accel/kionix-kx022a.c line 162

Quoted 7 lines

drivers/iio/accel/kionix-kx022a.c new file +6 -0

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

Quoted 14 lines

Quoted 14 lines

drivers/iio/accel/kionix-kx022a.c new file +14 -0

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

Quoted 6 lines

Quoted 4 lines

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

Quoted 5 lines

Quoted 5 lines

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

+static const int kx022a_accel_samp_freq_table[][2] = {
+	[0] = { 0, 780000 },
+	[1] = { 1, 563000 },
+	[2] = { 3, 125000 },
+	[3] = { 6, 250000 },

Quoted 8 lines

drivers/iio/accel/kionix-kx022a.c line 247

Quoted 5 lines

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

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

Quoted 1 line

Quoted 1 line

+};

Quoted 3 lines

drivers/iio/accel/kionix-kx022a.c line 378

Quoted 1 line

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

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

Quoted 4 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

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

Quoted 7 lines

drivers/iio/accel/kionix-kx022a.c line 384

Quoted 5 lines

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

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

Quoted 4 lines

Quoted 4 lines

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

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

Quoted 11 lines

drivers/iio/accel/kionix-kx022a.c line 449

Quoted 2 lines

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

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

No endianess awareness (sizeof __le16 / __be16)

drivers/iio/accel/kionix-kx022a.c line 453

Quoted 4 lines

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

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

Quoted 4 lines

drivers/iio/accel/kionix-kx022a.c line 525

Quoted 2 lines

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

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

Quoted 3 lines

drivers/iio/accel/kionix-kx022a.c line 583

Quoted 1 line

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

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

Quoted 7 lines

drivers/iio/accel/kionix-kx022a.c line 729

Quoted 3 lines

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

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

Quoted 2 lines

Quoted 2 lines

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

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

Quoted 4 lines

drivers/iio/accel/kionix-kx022a.c line 1008

Quoted 1 line

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

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

Quoted 1 line

Quoted 1 line

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

+		return -ENODEV;

Quoted 10 lines

drivers/iio/accel/kionix-kx022a.c line 1051

Quoted 8 lines

drivers/iio/accel/kionix-kx022a.c new file +8 -0

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

Quoted 3 lines

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

Quoted 5 lines

drivers/iio/accel/kionix-kx022a.c line 1134

Quoted 3 lines

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

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

Quoted 9 lines · 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 :)

Quoted 1 line · Andy Shevchenko

On Thu, Oct 06, 2022 at 05:38:14PM +0300, Matti Vaittinen wrote:

...

Quoted 9 lines

Quoted 9 lines

drivers/iio/accel/Kconfig config IIO_ST_ACCEL_SPI_3AXIS · line 422

Quoted 8 lines

drivers/iio/accel/Kconfig +8 -0

+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

Quoted 8 lines

Quoted 1 line

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

Quoted 3 lines

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

...

Quoted 8 lines

Quoted 7 lines

drivers/iio/accel/kionix-kx022a.c line 14

Quoted 5 lines

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

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

Quoted 1 line

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.

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

...

Quoted 10 lines

Quoted 9 lines

drivers/iio/accel/kionix-kx022a.c line 156

Quoted 6 lines

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

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

Quoted 5 lines

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.

Quoted 11 lines

Quoted 10 lines

drivers/iio/accel/kionix-kx022a.c line 162

Quoted 8 lines

drivers/iio/accel/kionix-kx022a.c new file +6 -0

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

Quoted 6 lines

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?

...

Quoted 5 lines

Quoted 3 lines

drivers/iio/accel/kionix-kx022a.c line 378

Quoted 1 line

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

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

Quoted 8 lines

Quoted 5 lines

Review

Quoted 3 lines

(quoted diff) +2 -0

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

Quoted 8 lines

Quoted 7 lines

drivers/iio/accel/kionix-kx022a.c line 384

Quoted 5 lines

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

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

Quoted 2 lines

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?

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

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

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

...

Quoted 12 lines

Quoted 11 lines

drivers/iio/accel/kionix-kx022a.c line 449

Quoted 2 lines

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

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

No endianess awareness (sizeof __le16 / __be16)

drivers/iio/accel/kionix-kx022a.c line 453

Quoted 4 lines

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

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

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

Quoted 5 lines

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

...

Quoted 5 lines

Quoted 4 lines

drivers/iio/accel/kionix-kx022a.c line 525

Quoted 2 lines

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

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

max() from minmax.h?

Quoted 3 lines

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?

Quoted 2 lines

Quoted 2 lines

drivers/iio/accel/kionix-kx022a.c line 525

Quoted 2 lines

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

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

...

Quoted 6 lines

Quoted 3 lines

drivers/iio/accel/kionix-kx022a.c line 583

Quoted 1 line

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

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

Quoted 8 lines

Quoted 7 lines

drivers/iio/accel/kionix-kx022a.c line 729

Quoted 3 lines

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

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

Quoted 8 lines

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.

Quoted 20 lines

Quoted 5 lines

Quoted 3 lines

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

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

Quoted 10 lines

drivers/iio/accel/kionix-kx022a.c line 1051

Quoted 8 lines

drivers/iio/accel/kionix-kx022a.c new file +8 -0

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

Quoted 1 line

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.

...

Quoted 6 lines

Quoted 5 lines

drivers/iio/accel/kionix-kx022a.c line 1134

Quoted 3 lines

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

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

Quoted 8 lines

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.

Quoted 13 lines

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.

Quoted 7 lines

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.

Quoted 3 lines · Andy Shevchenko

Quoted 2 lines · Matti Vaittinen

Quoted 1 line · Andy Shevchenko

On Thu, Oct 06, 2022 at 05:38:14PM +0300, Matti Vaittinen wrote:

Quoted 9 lines

Quoted 8 lines

Quoted 1 line

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

Quoted 3 lines

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

Quoted 8 lines

Quoted 8 lines

Quoted 7 lines

drivers/iio/accel/kionix-kx022a.c line 14

Quoted 5 lines

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

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

Quoted 5 lines

Quoted 1 line

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?

Quoted 16 lines

Quoted 10 lines

Quoted 9 lines

drivers/iio/accel/kionix-kx022a.c line 156

Quoted 6 lines

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

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

Quoted 5 lines

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?

Quoted 3 lines

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?

Quoted 17 lines

Quoted 16 lines

Quoted 9 lines

drivers/iio/accel/kionix-kx022a.c line 162

Quoted 7 lines

drivers/iio/accel/kionix-kx022a.c new file +6 -0

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

Quoted 1 line

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?

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+
+		while (n-- > 0)

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

Quoted 7 lines

Quoted 6 lines

Quoted 3 lines

drivers/iio/accel/kionix-kx022a.c line 583

Quoted 1 line

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

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

Quoted 2 lines

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.

Quoted 27 lines

Quoted 8 lines

Quoted 7 lines

drivers/iio/accel/kionix-kx022a.c line 729

Quoted 3 lines

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

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

Quoted 8 lines

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

Quoted 2 lines

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.

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

drivers/iio/accel/kionix-kx022a.c line 732

Quoted 2 lines

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

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

Quoted 4 lines · Matti

Quoted 3 lines · Andy Shevchenko

Quoted 2 lines · Matti Vaittinen

Quoted 1 line · Andy Shevchenko

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

...

Quoted 13 lines

Quoted 9 lines

Quoted 8 lines

Quoted 1 line

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

Quoted 3 lines

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

Quoted 5 lines

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

Quoted 2 lines

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.

Quoted 1 line

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

It's reversed mapping we need.

Quoted 2 lines

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?

Quoted 3 lines

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.