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 <[email protected]>
---
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 <[email protected]>");
+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 <[email protected]>");
+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, ®val);
+ 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, ®val);
+ 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 <[email protected]>");
+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 =]
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
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.
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
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
On Thu, 6 Oct 2022 21:32:11 +0300
Andy Shevchenko <[email protected]> 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
On Sun, Oct 09, 2022 at 01:33:51PM +0100, Jonathan Cameron wrote:
> On Thu, 6 Oct 2022 21:32:11 +0300
> Andy Shevchenko <[email protected]> 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
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! ~~
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
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! ~~
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
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! ~~
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
>> <[email protected]> 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! ~~
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! ~~
On 10/14/22 16:42, Jonathan Cameron wrote:
> On Wed, 12 Oct 2022 10:40:38 +0300
> Matti Vaittinen <[email protected]> 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! ~~
On 10/14/22 16:22, Jonathan Cameron wrote:
> On Tue, 11 Oct 2022 09:10:21 +0000
> "Vaittinen, Matti" <[email protected]> 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
>>>> <[email protected]> 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! ~~
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" <[email protected]> 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
> > > > > <[email protected]> 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