* [PATCH v3 0/3] iio: accel: add MSA311 accelerometer driver
@ 2022-06-16 10:42 Dmitry Rokosov
2022-06-16 10:42 ` [PATCH v3 1/3] dt-bindings: vendor-prefixes: add MEMSensing Microsystems Co., Ltd Dmitry Rokosov
` (2 more replies)
0 siblings, 3 replies; 16+ messages in thread
From: Dmitry Rokosov @ 2022-06-16 10:42 UTC (permalink / raw)
To: robh+dt, stano.jakubek, shawnguo, jic23, lars, andy.shevchenko, stephan
Cc: linux-iio, devicetree, kernel, linux-kernel, Dmitry Rokosov
MSA311 is a tri-axial, low-g accelerometer with I2C digital output for
sensitivity consumer applications. It has dynamical user selectable full
scales range of +-2g/+-4g/+-8g/+-16g and allows acceleration measurements
with output data rates from 1Hz to 1000Hz.
Datasheet can be found at following URL:
https://cdn-shop.adafruit.com/product-files/5309/MSA311-V1.1-ENG.pdf
This driver supports following MSA311 features:
- IIO interface
- Different power modes: NORMAL and SUSPEND (using pm_runtime)
- ODR (Output Data Rate) selection
- Scale and samp_freq selection
- IIO triggered buffer, IIO reg access
- NEW_DATA interrupt + trigger
Below features to be done:
- Motion Events: ACTIVE, TAP, ORIENT, FREEFALL
- Low Power mode
Also this patchset has new vendor prefix for MEMSensing Microsystems and
MSA311 dt-binding schema.
You can test msa311 driver using libiio and gnuplot following below
instructions:
$ # Create hrtimer trigger object
$ mkdir /sys/kernel/config/iio/triggers/hrtimer/iio_hrtimer_trigger
$ # Read 4K samples using msa311-new-data trigger (irq) and
$ # buffer with depth equals to 64 samples and rotate device a little bit
$ iio_readdev -u "local:" -b 64 -s 4096 -t msa311-new-data -T 0 \
$ msa311 > /tmp/msa311.dat
$ # Or using hrtimer trigger instead of msa311-new-data trigger
$ iio_readdev -u "local:" -b 64 -s 4096 -t iio_hrtimer_trigger -T 0 \
$ msa311 > /data/local/tmp/msa311.dat
$ cat <<EOF >> msa311_data.gnu
set title "MSA311 Accel Data"
set key below
set xdata time
set format x "%H:%M\n%.4S"
set xlabel "timestamp"
set autoscale y
plot 'msa311.dat' binary endian=little \
format='%int16%int16%int16%uint16%uint64' using \
(\$5/1000000000):(int(\$1)/16) title "acc_x" \
with lines,\\
'msa311.dat' binary endian=little \
format='%int16%int16%int16%uint16%uint64' using \
(\$5/1000000000):(int(\$2)/16) title "acc_y" \
with lines,\\
'msa311.dat' binary endian=little \
format='%int16%int16%int16%uint16%uint64' using \
(\$5/1000000000):(int(\$3)/16) title "acc_z" with lines
EOF
$ gnuplot --persist msa311_data.gnu
Changes:
* v2->v3:
- removed MSA311_TIMESTAMP_CHANNEL() macro, used IIO_CHAN_SOFT_TIMESTAMP
directly
- do not call dev_err_probe() inside functions, which is used not only
from probe() path
- simplified error handling a little bit
- used iio_device_claim_direct_mode() and
iio_device_release_direct_mode() to lock attributes when buffer mode
is enabled
- prohibited sampling frequency changing during buffer usage because
otherwise sometimes MSA311 returns outliers when frequency values
grow up in the read operation moment
- allowed scale value changing when buffer mode is enabled
- removed IRQF_TRIGGER_RISING irq flag from irg registration because
it's provided from device tree directly
- do not switch off autosuspend from powerdown() devm callback,
because it's already done from pm_runtime_disable() during
devm pm_runtime actions
- provided more information why we need force suspend state for MSA311
in the powerdown flow
- reworked comments stuff: removed obvious extra comments, provided
more details in the complex driver code places
* v1->v2:
- memsensing vendor prefix was moved to right place by
alphabetical order
- LOW mode mention was deleted, because LOW mode isn't supported
in the current driver version
- reworked some enums with gaps to defines
- reworked register names as Jonathan mentioned in the v1
- do not use regmap_field API for entire registers
- deleted all extra comments
- supported info_mask_*_avail bitmaps instead of explicit IIO attrs
definitions, implemented read_avail() callback for samp_freq and
scale values
- msa311 mutex is still used to protect msa311 power transitions,
samp_freq/scale tune and axes data handling; described this lock
more informative
- ask new_data interruption status from appropriate register,
do not hold atomic variable for that
- optimized reads of axes data by I2C using regmap_bulk API
- use dev_err_probe() instead of dev_err() for all probe() code paths
- from now all I2C bus communication failures are interpreted as errors
- described wait_from_next() semantic better
- deleted all unneeded pm wrappers
- interpreter all axes data as __le16 type and adjust them to right
format (endianness + sign) for raw() flow only
- redesigned msa311_fs_table[] to 2D matrix (it's more comfortable
format for read_avail() callback)
- align and initialize msa311 buffer before pushing properly
- use pm_runtime resume and suspend from buffer preenable/postdisable,
deleted them from trigger set_state
- supported multiple trigger usage (tested with external hrtimer
trigger and internal new_data trigger)
- moved all irq related stuff to msa311_setup_interrupts() routine
- implemented msa311_powerdown() devm release action
- reworked initialization of pm_runtime msa311 flow, use
autosuspend logic
- purged driver remove() callback, because of devm release logic runs
all deinit stuff fully
- fixed dts bindings problems
- changed irq type in the dt-binding description, because interrupt
type for msa311 should have the same type as i2c irq, for example
using the gpio_intc it's IRQ_TYPE_EDGE_RISING usually. Otherwise
we may lose irq map on the second and further insmod attempts
Dmitry Rokosov (3):
dt-bindings: vendor-prefixes: add MEMSensing Microsystems Co., Ltd.
iio: add MEMSensing MSA311 3-axis accelerometer driver
dt-bindings: iio: accel: add dt-binding schema for msa311 accel driver
.../bindings/iio/accel/memsensing,msa311.yaml | 52 +
.../devicetree/bindings/vendor-prefixes.yaml | 2 +
MAINTAINERS | 7 +
drivers/iio/accel/Kconfig | 13 +
drivers/iio/accel/Makefile | 2 +
drivers/iio/accel/msa311.c | 1312 +++++++++++++++++
6 files changed, 1388 insertions(+)
create mode 100644 Documentation/devicetree/bindings/iio/accel/memsensing,msa311.yaml
create mode 100644 drivers/iio/accel/msa311.c
--
2.36.0
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v3 1/3] dt-bindings: vendor-prefixes: add MEMSensing Microsystems Co., Ltd.
2022-06-16 10:42 [PATCH v3 0/3] iio: accel: add MSA311 accelerometer driver Dmitry Rokosov
@ 2022-06-16 10:42 ` Dmitry Rokosov
2022-06-16 10:42 ` [PATCH v3 2/3] iio: add MEMSensing MSA311 3-axis accelerometer driver Dmitry Rokosov
2022-06-16 10:42 ` [PATCH v3 3/3] dt-bindings: iio: accel: add dt-binding schema for msa311 accel driver Dmitry Rokosov
2 siblings, 0 replies; 16+ messages in thread
From: Dmitry Rokosov @ 2022-06-16 10:42 UTC (permalink / raw)
To: robh+dt, stano.jakubek, shawnguo, jic23, lars, andy.shevchenko, stephan
Cc: linux-iio, devicetree, kernel, linux-kernel, Dmitry Rokosov, Rob Herring
MEMSensing Microsystems (Suzhou, China) Co., Ltd. operates as a micro
electromechanical system technology company which produces micro
electromechanical system microphones and sensors.
MEMSensing Microsystems (Suzhou, China) Co., Ltd. applies its products
in consumer electronics, industrial control, medical electronics
and automotive, and other fields.
Signed-off-by: Dmitry Rokosov <ddrokosov@sberdevices.ru>
Acked-by: Rob Herring <robh@kernel.org>
---
Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++
1 file changed, 2 insertions(+)
diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml
index 294093d45a23..632afb05fcf7 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
+++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
@@ -735,6 +735,8 @@ patternProperties:
description: MELFAS Inc.
"^mellanox,.*":
description: Mellanox Technologies
+ "^memsensing,.*":
+ description: MEMSensing Microsystems Co., Ltd.
"^memsic,.*":
description: MEMSIC Inc.
"^menlo,.*":
--
2.36.0
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v3 2/3] iio: add MEMSensing MSA311 3-axis accelerometer driver
2022-06-16 10:42 [PATCH v3 0/3] iio: accel: add MSA311 accelerometer driver Dmitry Rokosov
2022-06-16 10:42 ` [PATCH v3 1/3] dt-bindings: vendor-prefixes: add MEMSensing Microsystems Co., Ltd Dmitry Rokosov
@ 2022-06-16 10:42 ` Dmitry Rokosov
2022-06-16 12:18 ` Andy Shevchenko
2022-06-19 12:27 ` Jonathan Cameron
2022-06-16 10:42 ` [PATCH v3 3/3] dt-bindings: iio: accel: add dt-binding schema for msa311 accel driver Dmitry Rokosov
2 siblings, 2 replies; 16+ messages in thread
From: Dmitry Rokosov @ 2022-06-16 10:42 UTC (permalink / raw)
To: robh+dt, stano.jakubek, shawnguo, jic23, lars, andy.shevchenko, stephan
Cc: linux-iio, devicetree, kernel, linux-kernel, Dmitry Rokosov
MSA311 is a tri-axial, low-g accelerometer with I2C digital output for
sensitivity consumer applications. It has dynamical user selectable full
scales range of +-2g/+-4g/+-8g/+-16g and allows acceleration measurements
with output data rates from 1Hz to 1000Hz.
Datasheet can be found at following URL:
https://cdn-shop.adafruit.com/product-files/5309/MSA311-V1.1-ENG.pdf
This driver supports following MSA311 features:
- IIO interface
- Different power modes: NORMAL and SUSPEND (using pm_runtime)
- ODR (Output Data Rate) selection
- Scale and samp_freq selection
- IIO triggered buffer, IIO reg access
- NEW_DATA interrupt + trigger
Below features to be done:
- Motion Events: ACTIVE, TAP, ORIENT, FREEFALL
- Low Power mode
Signed-off-by: Dmitry Rokosov <ddrokosov@sberdevices.ru>
---
MAINTAINERS | 6 +
drivers/iio/accel/Kconfig | 13 +
drivers/iio/accel/Makefile | 2 +
drivers/iio/accel/msa311.c | 1312 ++++++++++++++++++++++++++++++++++++
4 files changed, 1333 insertions(+)
create mode 100644 drivers/iio/accel/msa311.c
diff --git a/MAINTAINERS b/MAINTAINERS
index d9b2f1731ee0..55aeb25c004c 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12478,6 +12478,12 @@ F: drivers/mtd/
F: include/linux/mtd/
F: include/uapi/mtd/
+MEMSENSING MICROSYSTEMS MSA311 ACCELEROMETER DRIVER
+M: Dmitry Rokosov <ddrokosov@sberdevices.ru>
+L: linux-iio@vger.kernel.org
+S: Maintained
+F: drivers/iio/accel/msa311.c
+
MEN A21 WATCHDOG DRIVER
M: Johannes Thumshirn <morbidrsa@gmail.com>
L: linux-watchdog@vger.kernel.org
diff --git a/drivers/iio/accel/Kconfig b/drivers/iio/accel/Kconfig
index 49587c992a6d..88a265b75eed 100644
--- a/drivers/iio/accel/Kconfig
+++ b/drivers/iio/accel/Kconfig
@@ -508,6 +508,19 @@ config MMA9553
To compile this driver as a module, choose M here: the module
will be called mma9553.
+config MSA311
+ tristate "MEMSensing Digital 3-Axis Accelerometer Driver"
+ depends on I2C
+ select IIO_BUFFER
+ select IIO_TRIGGERED_BUFFER
+ select REGMAP_I2C
+ help
+ Say yes here to build support for the MEMSensing MSA311
+ accelerometer driver.
+
+ To compile this driver as a module, choose M here: the module will be
+ called msa311.
+
config MXC4005
tristate "Memsic MXC4005XC 3-Axis Accelerometer Driver"
depends on I2C
diff --git a/drivers/iio/accel/Makefile b/drivers/iio/accel/Makefile
index d03e2f6bba08..b1ddcaa811b6 100644
--- a/drivers/iio/accel/Makefile
+++ b/drivers/iio/accel/Makefile
@@ -55,6 +55,8 @@ obj-$(CONFIG_MMA9551_CORE) += mma9551_core.o
obj-$(CONFIG_MMA9551) += mma9551.o
obj-$(CONFIG_MMA9553) += mma9553.o
+obj-$(CONFIG_MSA311) += msa311.o
+
obj-$(CONFIG_MXC4005) += mxc4005.o
obj-$(CONFIG_MXC6255) += mxc6255.o
diff --git a/drivers/iio/accel/msa311.c b/drivers/iio/accel/msa311.c
new file mode 100644
index 000000000000..f8a8ed064f21
--- /dev/null
+++ b/drivers/iio/accel/msa311.c
@@ -0,0 +1,1312 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * msa311.c - MEMSensing digital 3-Axis accelerometer
+ *
+ * MSA311 is a tri-axial, low-g accelerometer with I2C digital output for
+ * sensitivity consumer applications. It has dynamical user selectable full
+ * scales range of +-2g/+-4g/+-8g/+-16g and allows acceleration measurements
+ * with output data rates from 1Hz to 1000Hz.
+ *
+ * MSA311 is available in an ultra small (2mm x 2mm, height 0.95mm) LGA package
+ * and is guaranteed to operate over -40C to +85C.
+ *
+ * This driver supports following MSA311 features:
+ * - IIO interface
+ * - Different power modes: NORMAL, SUSPEND
+ * - ODR (Output Data Rate) selection
+ * - Scale selection
+ * - IIO triggered buffer
+ * - NEW_DATA interrupt + trigger
+ *
+ * Below features to be done:
+ * - Motion Events: ACTIVE, TAP, ORIENT, FREEFALL
+ * - Low Power mode
+ *
+ * Copyright (c) 2022, SberDevices. All Rights Reserved.
+ *
+ * Author: Dmitry Rokosov <ddrokosov@sberdevices.ru>
+ */
+
+#include <linux/i2c.h>
+#include <linux/iio/buffer.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/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/pm.h>
+#include <linux/pm_runtime.h>
+#include <linux/regmap.h>
+
+/* Register map */
+
+#define MSA311_SOFT_RESET_REG 0x00
+#define MSA311_PARTID_REG 0x01
+#define MSA311_ACC_X_REG 0x02
+#define MSA311_ACC_Y_REG 0x04
+#define MSA311_ACC_Z_REG 0x06
+#define MSA311_MOTION_INT_REG 0x09
+#define MSA311_DATA_INT_REG 0x0A
+#define MSA311_TAP_ACTIVE_STS_REG 0x0B
+#define MSA311_ORIENT_STS_REG 0x0C
+#define MSA311_RANGE_REG 0x0F
+#define MSA311_ODR_REG 0x10
+#define MSA311_PWR_MODE_REG 0x11
+#define MSA311_SWAP_POLARITY_REG 0x12
+#define MSA311_INT_SET_0_REG 0x16
+#define MSA311_INT_SET_1_REG 0x17
+#define MSA311_INT_MAP_0_REG 0x19
+#define MSA311_INT_MAP_1_REG 0x1A
+#define MSA311_INT_CONFIG_REG 0x20
+#define MSA311_INT_LATCH_REG 0x21
+#define MSA311_FREEFALL_DUR_REG 0x22
+#define MSA311_FREEFALL_TH_REG 0x23
+#define MSA311_FREEFALL_HY_REG 0x24
+#define MSA311_ACTIVE_DUR_REG 0x27
+#define MSA311_ACTIVE_TH_REG 0x28
+#define MSA311_TAP_DUR_REG 0x2A
+#define MSA311_TAP_TH_REG 0x2B
+#define MSA311_ORIENT_HY_REG 0x2C
+#define MSA311_Z_BLOCK_REG 0x2D
+#define MSA311_OFFSET_X_REG 0x38
+#define MSA311_OFFSET_Y_REG 0x39
+#define MSA311_OFFSET_Z_REG 0x3A
+
+enum msa311_fields {
+ F_SOFT_RESET_I2C, F_SOFT_RESET_SPI,
+
+ F_ORIENT_INT, F_S_TAP_INT, F_D_TAP_INT, F_ACTIVE_INT, F_FREEFALL_INT,
+
+ F_NEW_DATA_INT,
+
+ F_TAP_SIGN, F_TAP_FIRST_X, F_TAP_FIRST_Y, F_TAP_FIRST_Z, F_ACTV_SIGN,
+ F_ACTV_FIRST_X, F_ACTV_FIRST_Y, F_ACTV_FIRST_Z,
+
+ F_ORIENT_Z, F_ORIENT_X_Y,
+
+ F_FS,
+
+ F_X_AXIS_DIS, F_Y_AXIS_DIS, F_Z_AXIS_DIS, F_ODR,
+
+ F_PWR_MODE, F_LOW_POWER_BW,
+
+ F_X_POLARITY, F_Y_POLARITY, F_Z_POLARITY, F_X_Y_SWAP,
+
+ F_ORIENT_INT_EN, F_S_TAP_INT_EN, F_D_TAP_INT_EN, F_ACTIVE_INT_EN_Z,
+ F_ACTIVE_INT_EN_Y, F_ACTIVE_INT_EN_X,
+
+ F_NEW_DATA_INT_EN, F_FREEFALL_INT_EN,
+
+ F_INT1_ORIENT, F_INT1_S_TAP, F_INT1_D_TAP, F_INT1_ACTIVE,
+ F_INT1_FREEFALL,
+
+ F_INT1_NEW_DATA,
+
+ F_INT1_OD, F_INT1_LVL,
+
+ F_RESET_INT, F_LATCH_INT,
+
+ F_FREEFALL_MODE, F_FREEFALL_HY,
+
+ F_ACTIVE_DUR,
+
+ F_TAP_QUIET, F_TAP_SHOCK, F_TAP_DUR,
+
+ F_TAP_TH,
+
+ F_ORIENT_HYST, F_ORIENT_BLOCKING, F_ORIENT_MODE,
+
+ F_Z_BLOCKING,
+
+ F_MAX_FIELDS,
+};
+
+static const struct reg_field msa311_reg_fields[] = {
+ [F_SOFT_RESET_I2C] = REG_FIELD(MSA311_SOFT_RESET_REG, 2, 2),
+ [F_SOFT_RESET_SPI] = REG_FIELD(MSA311_SOFT_RESET_REG, 5, 5),
+
+ [F_ORIENT_INT] = REG_FIELD(MSA311_MOTION_INT_REG, 6, 6),
+ [F_S_TAP_INT] = REG_FIELD(MSA311_MOTION_INT_REG, 5, 5),
+ [F_D_TAP_INT] = REG_FIELD(MSA311_MOTION_INT_REG, 4, 4),
+ [F_ACTIVE_INT] = REG_FIELD(MSA311_MOTION_INT_REG, 2, 2),
+ [F_FREEFALL_INT] = REG_FIELD(MSA311_MOTION_INT_REG, 0, 0),
+
+ [F_NEW_DATA_INT] = REG_FIELD(MSA311_DATA_INT_REG, 0, 0),
+
+ [F_TAP_SIGN] = REG_FIELD(MSA311_TAP_ACTIVE_STS_REG, 7, 7),
+ [F_TAP_FIRST_X] = REG_FIELD(MSA311_TAP_ACTIVE_STS_REG, 6, 6),
+ [F_TAP_FIRST_Y] = REG_FIELD(MSA311_TAP_ACTIVE_STS_REG, 5, 5),
+ [F_TAP_FIRST_Z] = REG_FIELD(MSA311_TAP_ACTIVE_STS_REG, 4, 4),
+ [F_ACTV_SIGN] = REG_FIELD(MSA311_TAP_ACTIVE_STS_REG, 3, 3),
+ [F_ACTV_FIRST_X] = REG_FIELD(MSA311_TAP_ACTIVE_STS_REG, 2, 2),
+ [F_ACTV_FIRST_Y] = REG_FIELD(MSA311_TAP_ACTIVE_STS_REG, 1, 1),
+ [F_ACTV_FIRST_Z] = REG_FIELD(MSA311_TAP_ACTIVE_STS_REG, 0, 0),
+
+ [F_ORIENT_Z] = REG_FIELD(MSA311_ORIENT_STS_REG, 6, 6),
+ [F_ORIENT_X_Y] = REG_FIELD(MSA311_ORIENT_STS_REG, 4, 5),
+
+ [F_FS] = REG_FIELD(MSA311_RANGE_REG, 0, 1),
+
+ [F_X_AXIS_DIS] = REG_FIELD(MSA311_ODR_REG, 7, 7),
+ [F_Y_AXIS_DIS] = REG_FIELD(MSA311_ODR_REG, 6, 6),
+ [F_Z_AXIS_DIS] = REG_FIELD(MSA311_ODR_REG, 5, 5),
+ [F_ODR] = REG_FIELD(MSA311_ODR_REG, 0, 3),
+
+ [F_PWR_MODE] = REG_FIELD(MSA311_PWR_MODE_REG, 6, 7),
+ [F_LOW_POWER_BW] = REG_FIELD(MSA311_PWR_MODE_REG, 1, 4),
+
+ [F_X_POLARITY] = REG_FIELD(MSA311_SWAP_POLARITY_REG, 3, 3),
+ [F_Y_POLARITY] = REG_FIELD(MSA311_SWAP_POLARITY_REG, 2, 2),
+ [F_Z_POLARITY] = REG_FIELD(MSA311_SWAP_POLARITY_REG, 1, 1),
+ [F_X_Y_SWAP] = REG_FIELD(MSA311_SWAP_POLARITY_REG, 0, 0),
+
+ [F_ORIENT_INT_EN] = REG_FIELD(MSA311_INT_SET_0_REG, 6, 6),
+ [F_S_TAP_INT_EN] = REG_FIELD(MSA311_INT_SET_0_REG, 5, 5),
+ [F_D_TAP_INT_EN] = REG_FIELD(MSA311_INT_SET_0_REG, 4, 4),
+ [F_ACTIVE_INT_EN_Z] = REG_FIELD(MSA311_INT_SET_0_REG, 2, 2),
+ [F_ACTIVE_INT_EN_Y] = REG_FIELD(MSA311_INT_SET_0_REG, 1, 1),
+ [F_ACTIVE_INT_EN_X] = REG_FIELD(MSA311_INT_SET_0_REG, 0, 0),
+
+ [F_NEW_DATA_INT_EN] = REG_FIELD(MSA311_INT_SET_1_REG, 4, 4),
+ [F_FREEFALL_INT_EN] = REG_FIELD(MSA311_INT_SET_1_REG, 3, 3),
+
+ [F_INT1_ORIENT] = REG_FIELD(MSA311_INT_MAP_0_REG, 6, 6),
+ [F_INT1_S_TAP] = REG_FIELD(MSA311_INT_MAP_0_REG, 5, 5),
+ [F_INT1_D_TAP] = REG_FIELD(MSA311_INT_MAP_0_REG, 4, 4),
+ [F_INT1_ACTIVE] = REG_FIELD(MSA311_INT_MAP_0_REG, 2, 2),
+ [F_INT1_FREEFALL] = REG_FIELD(MSA311_INT_MAP_0_REG, 0, 0),
+
+ [F_INT1_NEW_DATA] = REG_FIELD(MSA311_INT_MAP_1_REG, 0, 0),
+
+ [F_INT1_OD] = REG_FIELD(MSA311_INT_CONFIG_REG, 1, 1),
+ [F_INT1_LVL] = REG_FIELD(MSA311_INT_CONFIG_REG, 0, 0),
+
+ [F_RESET_INT] = REG_FIELD(MSA311_INT_LATCH_REG, 7, 7),
+ [F_LATCH_INT] = REG_FIELD(MSA311_INT_LATCH_REG, 0, 3),
+
+ [F_FREEFALL_MODE] = REG_FIELD(MSA311_FREEFALL_HY_REG, 2, 2),
+ [F_FREEFALL_HY] = REG_FIELD(MSA311_FREEFALL_HY_REG, 0, 1),
+
+ [F_ACTIVE_DUR] = REG_FIELD(MSA311_ACTIVE_DUR_REG, 0, 1),
+
+ [F_TAP_QUIET] = REG_FIELD(MSA311_TAP_DUR_REG, 7, 7),
+ [F_TAP_SHOCK] = REG_FIELD(MSA311_TAP_DUR_REG, 6, 6),
+ [F_TAP_DUR] = REG_FIELD(MSA311_TAP_DUR_REG, 0, 2),
+
+ [F_TAP_TH] = REG_FIELD(MSA311_TAP_TH_REG, 0, 4),
+
+ [F_ORIENT_HYST] = REG_FIELD(MSA311_ORIENT_HY_REG, 4, 6),
+ [F_ORIENT_BLOCKING] = REG_FIELD(MSA311_ORIENT_HY_REG, 2, 3),
+ [F_ORIENT_MODE] = REG_FIELD(MSA311_ORIENT_HY_REG, 0, 1),
+
+ [F_Z_BLOCKING] = REG_FIELD(MSA311_Z_BLOCK_REG, 0, 3),
+};
+
+#define MSA311_WHO_AM_I 0x13
+
+/*
+ * Possible Full Scale ranges
+ *
+ * Axis data is 12-bit signed value, so
+ *
+ * fs0 = (2 + 2) * 9.81 / (2<<11) = 0.009580
+ * fs1 = (4 + 4) * 9.81 / (2<<11) = 0.019160
+ * fs2 = (8 + 8) * 9.81 / (2<<11) = 0.038320
+ * fs3 = (16 + 16) * 9.81 / (2<<11) = 0.076641
+ */
+enum {
+ MSA311_FS_2G,
+ MSA311_FS_4G,
+ MSA311_FS_8G,
+ MSA311_FS_16G,
+};
+
+static const struct {
+ int val;
+ int val2;
+} msa311_fs_table[] = {
+ {0, 9580}, {0, 19160}, {0, 38320}, {0, 76641}
+};
+
+/* Possible Output Data Rate values */
+enum {
+ MSA311_ODR_1_HZ,
+ MSA311_ODR_1_95_HZ,
+ MSA311_ODR_3_9_HZ,
+ MSA311_ODR_7_81_HZ,
+ MSA311_ODR_15_63_HZ,
+ MSA311_ODR_31_25_HZ,
+ MSA311_ODR_62_5_HZ,
+ MSA311_ODR_125_HZ,
+ MSA311_ODR_250_HZ,
+ MSA311_ODR_500_HZ,
+ MSA311_ODR_1000_HZ,
+};
+
+static const struct {
+ int val;
+ int val2;
+} msa311_odr_table[] = {
+ {1, 0}, {1, 950000}, {3, 900000}, {7, 810000}, {15, 630000},
+ {31, 250000}, {62, 500000}, {125, 0}, {250, 0}, {500, 0}, {1000, 0}
+};
+
+/* All supported power modes */
+#define MSA311_PWR_MODE_NORMAL 0b00
+#define MSA311_PWR_MODE_SUSPEND 0b11
+
+/* Autosuspend delay */
+#define MSA311_PWR_SLEEP_DELAY_MS 2000
+
+/* Possible INT1 types and levels */
+enum {
+ MSA311_INT1_OD_PUSH_PULL,
+ MSA311_INT1_OD_OPEN_DRAIN,
+};
+
+enum {
+ MSA311_INT1_LVL_LOW,
+ MSA311_INT1_LVL_HIGH,
+};
+
+/* Latch INT modes */
+#define MSA311_LATCH_INT_NOT_LATCHED 0b0000
+#define MSA311_LATCH_INT_250MS 0b0001
+#define MSA311_LATCH_INT_500MS 0b0010
+#define MSA311_LATCH_INT_1S 0b0011
+#define MSA311_LATCH_INT_2S 0b0100
+#define MSA311_LATCH_INT_4S 0b0101
+#define MSA311_LATCH_INT_8S 0b0110
+#define MSA311_LATCH_INT_1MS 0b1010
+#define MSA311_LATCH_INT_2MS 0b1011
+#define MSA311_LATCH_INT_25MS 0b1100
+#define MSA311_LATCH_INT_50MS 0b1101
+#define MSA311_LATCH_INT_100MS 0b1110
+#define MSA311_LATCH_INT_LATCHED 0b0111
+
+static const struct regmap_range msa311_readonly_registers[] = {
+ regmap_reg_range(MSA311_PARTID_REG, MSA311_ORIENT_STS_REG),
+};
+
+static const struct regmap_access_table msa311_writeable_table = {
+ .no_ranges = msa311_readonly_registers,
+ .n_no_ranges = ARRAY_SIZE(msa311_readonly_registers),
+};
+
+static const struct regmap_range msa311_writeonly_registers[] = {
+ regmap_reg_range(MSA311_SOFT_RESET_REG, MSA311_SOFT_RESET_REG),
+};
+
+static const struct regmap_access_table msa311_readable_table = {
+ .no_ranges = msa311_writeonly_registers,
+ .n_no_ranges = ARRAY_SIZE(msa311_writeonly_registers),
+};
+
+static const struct regmap_range msa311_volatile_registers[] = {
+ regmap_reg_range(MSA311_ACC_X_REG, MSA311_ORIENT_STS_REG),
+};
+
+static const struct regmap_access_table msa311_volatile_table = {
+ .yes_ranges = msa311_volatile_registers,
+ .n_yes_ranges = ARRAY_SIZE(msa311_volatile_registers),
+};
+
+static const struct regmap_config msa311_regmap_config = {
+ .name = "msa311",
+ .reg_bits = 8,
+ .val_bits = 8,
+ .max_register = MSA311_OFFSET_Z_REG,
+ .wr_table = &msa311_writeable_table,
+ .rd_table = &msa311_readable_table,
+ .volatile_table = &msa311_volatile_table,
+ .cache_type = REGCACHE_RBTREE,
+};
+
+#define MSA311_GENMASK(field) ({ \
+ typeof(field) _field = (field); \
+ GENMASK(msa311_reg_fields[_field].msb, \
+ msa311_reg_fields[_field].lsb); \
+})
+
+/**
+ * struct msa311_priv - MSA311 internal private state
+ * @i2c: I2C client object
+ * @lock: Protects msa311 device state between setup and data access routines
+ * (power transitions, samp_freq/scale tune, retrieving axes data, etc)
+ * @new_data_trig: Optional NEW_DATA interrupt driven trigger used
+ * to notify external consumers a new sample is ready
+ * @regs: Underlying I2C bus adapter used to abstract slave
+ * register accesses
+ * @fields: Abstract objects for each registers fields access
+ */
+struct msa311_priv {
+ struct i2c_client *i2c;
+ struct mutex lock; /* state guard */
+
+ struct iio_trigger *new_data_trig;
+
+ struct regmap *regs;
+ struct regmap_field *fields[F_MAX_FIELDS];
+};
+
+/* Channels */
+
+enum msa311_si {
+ MSA311_SI_X,
+ MSA311_SI_Y,
+ MSA311_SI_Z,
+ MSA311_SI_TIMESTAMP,
+};
+
+#define MSA311_ACCEL_CHANNEL(axis) { \
+ .type = IIO_ACCEL, \
+ .modified = 1, \
+ .channel2 = IIO_MOD_##axis, \
+ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
+ .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SCALE) | \
+ BIT(IIO_CHAN_INFO_SAMP_FREQ), \
+ .info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_SCALE) | \
+ BIT(IIO_CHAN_INFO_SAMP_FREQ), \
+ .scan_index = MSA311_SI_##axis, \
+ .scan_type = { \
+ .sign = 's', \
+ .realbits = 12, \
+ .storagebits = 16, \
+ .shift = 4, \
+ .endianness = IIO_LE, \
+ }, \
+ .datasheet_name = "ACC_"#axis \
+}
+
+static const struct iio_chan_spec msa311_channels[] = {
+ MSA311_ACCEL_CHANNEL(X),
+ MSA311_ACCEL_CHANNEL(Y),
+ MSA311_ACCEL_CHANNEL(Z),
+ IIO_CHAN_SOFT_TIMESTAMP(MSA311_SI_TIMESTAMP)
+};
+
+/**
+ * msa311_get_odr() - Read Output Data Rate (ODR) value from MSA311 accel
+ * @msa311: MSA311 internal private state
+ * @odr: output ODR value
+ *
+ * This function should be called under msa311->lock.
+ *
+ * Return: 0 on success, -ERRNO in other failures
+ */
+static inline int msa311_get_odr(struct msa311_priv *msa311, unsigned int *odr)
+{
+ int err;
+
+ err = regmap_field_read(msa311->fields[F_ODR], odr);
+ if (err)
+ return err;
+
+ /*
+ * Filter the same 1000Hz ODR register values based on datasheet info.
+ * ODR can be equal to 1010-1111 for 1000Hz, but function returns 1010
+ * all the time.
+ */
+ if (*odr > MSA311_ODR_1000_HZ)
+ *odr = MSA311_ODR_1000_HZ;
+
+ return 0;
+}
+
+/**
+ * msa311_set_odr() - Setup Output Data Rate (ODR) value for MSA311 accel
+ * @msa311: MSA311 internal private state
+ * @odr: requested ODR value
+ *
+ * This function should be called under msa311->lock. Possible ODR values:
+ * - 1Hz (not available in normal mode)
+ * - 1.95Hz (not available in normal mode)
+ * - 3.9Hz
+ * - 7.81Hz
+ * - 15.63Hz
+ * - 31.25Hz
+ * - 62.5Hz
+ * - 125Hz
+ * - 250Hz
+ * - 500Hz
+ * - 1000Hz
+ *
+ * Return: 0 on success, -EINVAL for bad ODR value in the certain power mode,
+ * -ERRNO in other failures
+ */
+static inline int msa311_set_odr(struct msa311_priv *msa311, unsigned int odr)
+{
+ struct device *dev = &msa311->i2c->dev;
+ const char *mode = NULL;
+ unsigned int pwr_mode;
+ bool good_odr = false;
+ int err;
+
+ err = regmap_field_read(msa311->fields[F_PWR_MODE], &pwr_mode);
+ if (err)
+ return err;
+
+ /* Filter bad ODR values */
+ switch (pwr_mode) {
+ case MSA311_PWR_MODE_NORMAL:
+ mode = "normal";
+ good_odr = (odr > MSA311_ODR_1_95_HZ);
+ break;
+ case MSA311_PWR_MODE_SUSPEND:
+ mode = "suspend";
+ break;
+ default:
+ mode = "unknown";
+ break;
+ }
+
+ if (!good_odr) {
+ dev_err(dev,
+ "failed to set odr %u.%uHz, not available in %s mode\n",
+ msa311_odr_table[odr].val,
+ msa311_odr_table[odr].val2 / 1000, mode);
+ return -EINVAL;
+ }
+
+ return regmap_field_write(msa311->fields[F_ODR], odr);
+}
+
+/**
+ * msa311_wait_for_next_data() - Wait next accel data available after resume
+ * @msa311: MSA311 internal private state
+ *
+ * Return: 0 on success, -EINTR if msleep() was interrupted,
+ * -ERRNO in other failures
+ */
+static int msa311_wait_for_next_data(struct msa311_priv *msa311)
+{
+ static const int unintr_thresh_ms = 20;
+ struct device *dev = &msa311->i2c->dev;
+ unsigned int odr;
+ unsigned long wait_ms;
+ unsigned long freq_uhz;
+ int err;
+
+ err = msa311_get_odr(msa311, &odr);
+ if (err) {
+ dev_err(dev, "cannot get actual freq (%d)\n", err);
+ return err;
+ }
+
+ /*
+ * After msa311 resuming is done, we need to wait for data
+ * to be refreshed by accel logic.
+ * A certain timeout is calculated based on the current ODR value.
+ * If requested timeout isn't so long (let's assume 20ms),
+ * we can wait for next data in uninterruptible sleep.
+ */
+ freq_uhz = msa311_odr_table[odr].val * USEC_PER_SEC +
+ msa311_odr_table[odr].val2;
+ wait_ms = (USEC_PER_SEC * MSEC_PER_SEC) / freq_uhz;
+
+ if (wait_ms < unintr_thresh_ms)
+ usleep_range(wait_ms * USEC_PER_MSEC,
+ unintr_thresh_ms * USEC_PER_MSEC);
+ else
+ return msleep_interruptible(wait_ms) ? -EINTR : 0;
+
+ return 0;
+}
+
+/**
+ * msa311_set_pwr_mode() - Install certain MSA311 power mode
+ * @msa311: MSA311 internal private state
+ * @mode: Power mode can be equal to NORMAL or SUSPEND
+ *
+ * This function should be called under msa311->lock.
+ *
+ * Return: 0 on success, -ERRNO on failure
+ */
+static int msa311_set_pwr_mode(struct msa311_priv *msa311, unsigned int mode)
+{
+ struct device *dev = &msa311->i2c->dev;
+ unsigned int prev_mode;
+ int err;
+
+ dev_dbg(dev, "transition to %s mode\n",
+ (mode == MSA311_PWR_MODE_NORMAL) ? "normal" :
+ (mode == MSA311_PWR_MODE_SUSPEND) ? "suspend" :
+ "unknown");
+
+ err = regmap_field_read(msa311->fields[F_PWR_MODE], &prev_mode);
+ if (err)
+ return err;
+
+ err = regmap_field_write(msa311->fields[F_PWR_MODE], mode);
+ if (err)
+ return err;
+
+ /* Wait actual data if we wakeup */
+ if (prev_mode == MSA311_PWR_MODE_SUSPEND &&
+ mode == MSA311_PWR_MODE_NORMAL)
+ return msa311_wait_for_next_data(msa311);
+
+ return 0;
+}
+
+/**
+ * msa311_get_axis() - Read MSA311 accel data for certain IIO channel axis spec
+ * @msa311: MSA311 internal private state
+ * @chan: IIO channel specification
+ * @axis: Output accel axis data for requested IIO channel spec
+ *
+ * This function should be called under msa311->lock.
+ *
+ * Return: 0 on success, -EINVAL for unknown IIO channel specification,
+ * -ERRNO in other failures
+ */
+static int msa311_get_axis(struct msa311_priv *msa311,
+ const struct iio_chan_spec * const chan,
+ __le16 *axis)
+{
+ struct device *dev = &msa311->i2c->dev;
+ unsigned int axis_reg;
+
+ if (chan->scan_index < MSA311_SI_X || chan->scan_index > MSA311_SI_Z) {
+ dev_err(dev, "invalid scan_index value [%d]\n",
+ chan->scan_index);
+ return -EINVAL;
+ }
+
+ /* Axes data layout has 2 byte gap for each axis starting from X axis */
+ axis_reg = MSA311_ACC_X_REG + (chan->scan_index << 1);
+
+ return regmap_bulk_read(msa311->regs, axis_reg, axis, sizeof(*axis));
+}
+
+static int msa311_read_raw_data(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ int *val, int *val2)
+{
+ struct msa311_priv *msa311 = iio_priv(indio_dev);
+ struct device *dev = &msa311->i2c->dev;
+ __le16 axis;
+ int err;
+
+ err = pm_runtime_resume_and_get(dev);
+ if (err)
+ return err;
+
+ err = iio_device_claim_direct_mode(indio_dev);
+ if (err)
+ return err;
+
+ mutex_lock(&msa311->lock);
+ err = msa311_get_axis(msa311, chan, &axis);
+ mutex_unlock(&msa311->lock);
+
+ iio_device_release_direct_mode(indio_dev);
+
+ if (err) {
+ dev_err(dev, "cannot get axis %s (%d)\n",
+ chan->datasheet_name, err);
+ return err;
+ }
+
+ pm_runtime_mark_last_busy(dev);
+ pm_runtime_put_autosuspend(dev);
+
+ /*
+ * Axis data format is:
+ * ACC_X = (ACC_X_MSB[7:0] << 4) | ACC_X_LSB[7:4]
+ */
+ *val = sign_extend32(le16_to_cpu(axis) >> chan->scan_type.shift,
+ chan->scan_type.realbits - 1);
+
+ return IIO_VAL_INT;
+}
+
+static int msa311_read_scale(struct iio_dev *indio_dev, int *val, int *val2)
+{
+ struct msa311_priv *msa311 = iio_priv(indio_dev);
+ struct device *dev = &msa311->i2c->dev;
+ unsigned int fs;
+ int err;
+
+ mutex_lock(&msa311->lock);
+ err = regmap_field_read(msa311->fields[F_FS], &fs);
+ mutex_unlock(&msa311->lock);
+
+ if (err) {
+ dev_err(dev, "cannot get actual scale (%d)\n", err);
+ return err;
+ }
+
+ *val = msa311_fs_table[fs].val;
+ *val2 = msa311_fs_table[fs].val2;
+
+ return IIO_VAL_INT_PLUS_MICRO;
+}
+
+static int msa311_read_samp_freq(struct iio_dev *indio_dev,
+ int *val, int *val2)
+{
+ struct msa311_priv *msa311 = iio_priv(indio_dev);
+ struct device *dev = &msa311->i2c->dev;
+ unsigned int odr;
+ int err;
+
+ mutex_lock(&msa311->lock);
+ err = msa311_get_odr(msa311, &odr);
+ mutex_unlock(&msa311->lock);
+
+ if (err) {
+ dev_err(dev, "cannot get actual freq (%d)\n", err);
+ return err;
+ }
+
+ *val = msa311_odr_table[odr].val;
+ *val2 = msa311_odr_table[odr].val2;
+
+ return IIO_VAL_INT_PLUS_MICRO;
+}
+
+static int msa311_read_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ int *val, int *val2, long mask)
+{
+ switch (mask) {
+ case IIO_CHAN_INFO_RAW:
+ return msa311_read_raw_data(indio_dev, chan, val, val2);
+
+ case IIO_CHAN_INFO_SCALE:
+ return msa311_read_scale(indio_dev, val, val2);
+
+ case IIO_CHAN_INFO_SAMP_FREQ:
+ return msa311_read_samp_freq(indio_dev, val, val2);
+
+ default:
+ return -EINVAL;
+ }
+}
+
+static int msa311_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 = (int *)msa311_odr_table;
+ *type = IIO_VAL_INT_PLUS_MICRO;
+ /* ODR value has 2 ints (integer and fractional parts) */
+ *length = ARRAY_SIZE(msa311_odr_table) * 2;
+ return IIO_AVAIL_LIST;
+
+ case IIO_CHAN_INFO_SCALE:
+ *vals = (int *)msa311_fs_table;
+ *type = IIO_VAL_INT_PLUS_MICRO;
+ /* FS value has 2 ints (integer and fractional parts) */
+ *length = ARRAY_SIZE(msa311_fs_table) * 2;
+ return IIO_AVAIL_LIST;
+
+ default:
+ return -EINVAL;
+ }
+}
+
+static int msa311_write_scale(struct iio_dev *indio_dev, int val, int val2)
+{
+ struct msa311_priv *msa311 = iio_priv(indio_dev);
+ struct device *dev = &msa311->i2c->dev;
+ unsigned int fs;
+ int err;
+
+ /* We do not have fs >= 1, so skip such values */
+ if (val != 0)
+ return 0;
+
+ err = pm_runtime_resume_and_get(dev);
+ if (err)
+ return err;
+
+ err = -EINVAL;
+ mutex_lock(&msa311->lock);
+ for (fs = 0; fs < ARRAY_SIZE(msa311_fs_table); ++fs)
+ /* Do not check msa311_fs_table[fs].val, it's always 0 */
+ if (val2 == msa311_fs_table[fs].val2) {
+ err = regmap_field_write(msa311->fields[F_FS], fs);
+ if (err) {
+ dev_err(dev, "cannot update scale (%d)\n", err);
+ goto failed;
+ }
+
+ break;
+ }
+
+failed:
+ mutex_unlock(&msa311->lock);
+
+ pm_runtime_mark_last_busy(dev);
+ pm_runtime_put_autosuspend(dev);
+
+ return err;
+}
+
+static int msa311_write_samp_freq(struct iio_dev *indio_dev, int val, int val2)
+{
+ struct msa311_priv *msa311 = iio_priv(indio_dev);
+ struct device *dev = &msa311->i2c->dev;
+ unsigned int odr;
+ int err;
+
+ err = pm_runtime_resume_and_get(dev);
+ if (err)
+ return err;
+
+ /*
+ * Sampling frequency changing is prohibited when buffer mode is
+ * enabled, because sometimes MSA311 chip returns outliers during
+ * frequency values growing up in the read operation moment.
+ */
+ err = iio_device_claim_direct_mode(indio_dev);
+ if (err)
+ return err;
+
+ err = -EINVAL;
+ mutex_lock(&msa311->lock);
+ for (odr = 0; odr < ARRAY_SIZE(msa311_odr_table); ++odr)
+ if (val == msa311_odr_table[odr].val &&
+ val2 == msa311_odr_table[odr].val2) {
+ err = msa311_set_odr(msa311, odr);
+ if (err) {
+ dev_err(dev, "cannot update freq (%d)\n", err);
+ goto failed;
+ }
+
+ break;
+ }
+
+failed:
+ mutex_unlock(&msa311->lock);
+
+ iio_device_release_direct_mode(indio_dev);
+
+ pm_runtime_mark_last_busy(dev);
+ pm_runtime_put_autosuspend(dev);
+
+ return err;
+}
+
+static int msa311_write_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ int val, int val2, long mask)
+{
+ switch (mask) {
+ case IIO_CHAN_INFO_SCALE:
+ return msa311_write_scale(indio_dev, val, val2);
+
+ case IIO_CHAN_INFO_SAMP_FREQ:
+ return msa311_write_samp_freq(indio_dev, val, val2);
+
+ default:
+ return -EINVAL;
+ }
+}
+
+static int msa311_debugfs_reg_access(struct iio_dev *indio_dev,
+ unsigned int reg, unsigned int writeval,
+ unsigned int *readval)
+{
+ struct msa311_priv *msa311 = iio_priv(indio_dev);
+ struct device *dev = &msa311->i2c->dev;
+ int err;
+
+ if (reg > regmap_get_max_register(msa311->regs))
+ return -EINVAL;
+
+ err = pm_runtime_resume_and_get(dev);
+ if (err)
+ return err;
+
+ mutex_lock(&msa311->lock);
+
+ if (!readval)
+ err = regmap_write(msa311->regs, reg, writeval);
+ else
+ err = regmap_read(msa311->regs, reg, readval);
+
+ mutex_unlock(&msa311->lock);
+
+ pm_runtime_mark_last_busy(dev);
+ pm_runtime_put_autosuspend(dev);
+
+ if (err)
+ dev_err(dev, "cannot %s register %u from debugfs (%d)\n",
+ (!readval) ? "write" : "read", reg, err);
+
+ return err;
+}
+
+static int msa311_buffer_preenable(struct iio_dev *indio_dev)
+{
+ struct msa311_priv *msa311 = iio_priv(indio_dev);
+ struct device *dev = &msa311->i2c->dev;
+
+ return pm_runtime_resume_and_get(dev);
+}
+
+static int msa311_buffer_postdisable(struct iio_dev *indio_dev)
+{
+ struct msa311_priv *msa311 = iio_priv(indio_dev);
+ struct device *dev = &msa311->i2c->dev;
+
+ pm_runtime_mark_last_busy(dev);
+ pm_runtime_put_autosuspend(dev);
+
+ return 0;
+}
+
+static int msa311_set_new_data_trig_state(struct iio_trigger *trig, bool state)
+{
+ struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
+ struct msa311_priv *msa311 = iio_priv(indio_dev);
+ struct device *dev = &msa311->i2c->dev;
+ int err;
+
+ mutex_lock(&msa311->lock);
+ err = regmap_field_write(msa311->fields[F_NEW_DATA_INT_EN], state);
+ mutex_unlock(&msa311->lock);
+
+ if (err)
+ dev_err(dev,
+ "cannot %s buffer due to new_data_int failure (%d)\n",
+ state ? "enable" : "disable", err);
+
+ return err;
+}
+
+static int msa311_validate_device(struct iio_trigger *trig,
+ struct iio_dev *indio_dev)
+{
+ return (iio_trigger_get_drvdata(trig) != indio_dev) ? -EINVAL : 0;
+}
+
+static irqreturn_t msa311_buffer_thread(int irq, void *p)
+{
+ struct iio_poll_func *pf = p;
+ struct iio_dev *indio_dev = pf->indio_dev;
+ struct msa311_priv *msa311 = iio_priv(indio_dev);
+ struct device *dev = &msa311->i2c->dev;
+ const struct iio_chan_spec *chan;
+ __le16 axis;
+ int bit = 0, err, i = 0;
+
+ /* Ensure correct alignment of time stamp when present */
+ struct {
+ __le16 channels[MSA311_SI_Z + 1];
+ s64 ts __aligned(8);
+ } buf;
+
+ memset(&buf, 0, sizeof(buf));
+
+ mutex_lock(&msa311->lock);
+
+ for_each_set_bit(bit, indio_dev->active_scan_mask,
+ indio_dev->masklength) {
+ chan = &msa311_channels[bit];
+
+ err = msa311_get_axis(msa311, chan, &axis);
+ if (err) {
+ mutex_unlock(&msa311->lock);
+ dev_err(dev, "cannot get axis %s (%d)\n",
+ chan->datasheet_name, err);
+ goto err;
+ }
+
+ buf.channels[i++] = axis;
+ }
+
+ mutex_unlock(&msa311->lock);
+
+ iio_push_to_buffers_with_timestamp(indio_dev, &buf,
+ iio_get_time_ns(indio_dev));
+
+err:
+ iio_trigger_notify_done(indio_dev->trig);
+
+ return IRQ_HANDLED;
+}
+
+static irqreturn_t msa311_irq_thread(int irq, void *p)
+{
+ struct msa311_priv *msa311 = iio_priv(p);
+ struct device *dev = &msa311->i2c->dev;
+ unsigned int new_data_int_enabled;
+ int err;
+
+ mutex_lock(&msa311->lock);
+
+ /*
+ * We do not check NEW_DATA int status, because of based on
+ * specification it's cleared automatically after a fixed time.
+ * So just check that is enabled by driver logic.
+ */
+ err = regmap_field_read(msa311->fields[F_NEW_DATA_INT_EN],
+ &new_data_int_enabled);
+
+ /* TODO: check motion interrupts status */
+
+ mutex_unlock(&msa311->lock);
+
+ if (err) {
+ dev_err(dev, "cannot read new_data int state (%d)\n", err);
+ return IRQ_NONE;
+ }
+
+ if (new_data_int_enabled)
+ iio_trigger_poll_chained(msa311->new_data_trig);
+
+ /* TODO: send motion events if needed */
+
+ return IRQ_HANDLED;
+}
+
+static const struct iio_info msa311_info = {
+ .read_raw = msa311_read_raw,
+ .read_avail = msa311_read_avail,
+ .write_raw = msa311_write_raw,
+ .debugfs_reg_access = msa311_debugfs_reg_access,
+};
+
+static const struct iio_buffer_setup_ops msa311_buffer_setup_ops = {
+ .preenable = msa311_buffer_preenable,
+ .postdisable = msa311_buffer_postdisable,
+};
+
+static const struct iio_trigger_ops msa311_new_data_trig_ops = {
+ .set_trigger_state = msa311_set_new_data_trig_state,
+ .validate_device = msa311_validate_device,
+};
+
+static int msa311_check_partid(struct msa311_priv *msa311)
+{
+ struct device *dev = &msa311->i2c->dev;
+ unsigned int partid;
+ int err;
+
+ err = regmap_read(msa311->regs, MSA311_PARTID_REG, &partid);
+ if (err) {
+ dev_err(dev, "failed to read partid (%d)\n", err);
+ return err;
+ }
+
+ dev_dbg(dev, "Found MSA311 compatible chip[%#x]\n", partid);
+
+ if (partid != MSA311_WHO_AM_I)
+ dev_warn(dev, "invalid partid (%#x), expected (%#x)\n",
+ partid, MSA311_WHO_AM_I);
+
+ return 0;
+}
+
+static int msa311_chip_init(struct msa311_priv *msa311)
+{
+ struct device *dev = &msa311->i2c->dev;
+ int err;
+
+ err = msa311_check_partid(msa311);
+ if (err)
+ return err;
+
+ /* Soft reset */
+ err = regmap_write(msa311->regs, MSA311_SOFT_RESET_REG,
+ MSA311_GENMASK(F_SOFT_RESET_I2C) |
+ MSA311_GENMASK(F_SOFT_RESET_SPI));
+ if (err)
+ return dev_err_probe(dev, err, "cannot soft reset all logic\n");
+
+ err = msa311_set_pwr_mode(msa311, MSA311_PWR_MODE_NORMAL);
+ if (err)
+ return dev_err_probe(dev, err, "bad normal mode transition\n");
+
+ err = regmap_write(msa311->regs, MSA311_RANGE_REG, MSA311_FS_16G);
+ if (err)
+ return dev_err_probe(dev, err, "failed to setup accel range\n");
+
+ /* Disable all interrupts by default */
+ err = regmap_write(msa311->regs, MSA311_INT_SET_0_REG, 0);
+ if (err)
+ return dev_err_probe(dev, err, "cannot disable set0 intrs\n");
+
+ err = regmap_write(msa311->regs, MSA311_INT_SET_1_REG, 0);
+ if (err)
+ return dev_err_probe(dev, err, "cannot disable set1 intrs\n");
+
+ /* Unmap all INT1 interrupts by default */
+ err = regmap_write(msa311->regs, MSA311_INT_MAP_0_REG, 0);
+ if (err)
+ return dev_err_probe(dev, err, "failed to unmap map0 intrs\n");
+
+ err = regmap_write(msa311->regs, MSA311_INT_MAP_1_REG, 0);
+ if (err)
+ return dev_err_probe(dev, err, "failed to unmap map1 intrs\n");
+
+ /* Disable all axis by default */
+ err = regmap_update_bits(msa311->regs, MSA311_ODR_REG,
+ MSA311_GENMASK(F_X_AXIS_DIS) |
+ MSA311_GENMASK(F_Y_AXIS_DIS) |
+ MSA311_GENMASK(F_Z_AXIS_DIS), 0);
+ if (err)
+ return dev_err_probe(dev, err, "cannot enable all axes\n");
+
+ err = msa311_set_odr(msa311, MSA311_ODR_125_HZ);
+ if (err)
+ return dev_err_probe(dev, err, "failed to set accel freq\n");
+
+ return 0;
+}
+
+static int msa311_setup_interrupts(struct msa311_priv *msa311)
+{
+ struct iio_trigger *trig;
+ struct i2c_client *i2c = msa311->i2c;
+ struct iio_dev *indio_dev = i2c_get_clientdata(i2c);
+ struct device *dev = &i2c->dev;
+ int err;
+
+ err = devm_request_threaded_irq(dev, i2c->irq,
+ NULL, msa311_irq_thread,
+ IRQF_ONESHOT,
+ i2c->name,
+ indio_dev);
+ if (err)
+ return dev_err_probe(dev, err, "failed to request irq\n");
+
+ trig = devm_iio_trigger_alloc(dev, "%s-new-data", i2c->name);
+ if (!trig)
+ return dev_err_probe(dev, -ENOMEM,
+ "cannot allocate newdata trig\n");
+
+ msa311->new_data_trig = trig;
+ msa311->new_data_trig->dev.parent = dev;
+ msa311->new_data_trig->ops = &msa311_new_data_trig_ops;
+ iio_trigger_set_drvdata(msa311->new_data_trig, indio_dev);
+
+ err = devm_iio_trigger_register(dev, msa311->new_data_trig);
+ if (err)
+ return dev_err_probe(dev, err, "can't register newdata trig\n");
+
+ err = regmap_field_write(msa311->fields[F_INT1_OD],
+ MSA311_INT1_OD_PUSH_PULL);
+ if (err)
+ return dev_err_probe(dev, err, "cannot enable push-pull int\n");
+
+ err = regmap_field_write(msa311->fields[F_INT1_LVL],
+ MSA311_INT1_LVL_HIGH);
+ if (err)
+ return dev_err_probe(dev, err, "cannot set active int level\n");
+
+ err = regmap_field_write(msa311->fields[F_LATCH_INT],
+ MSA311_LATCH_INT_LATCHED);
+ if (err)
+ return dev_err_probe(dev, err, "cannot latch int\n");
+
+ err = regmap_field_write(msa311->fields[F_RESET_INT], 1);
+ if (err)
+ return dev_err_probe(dev, err, "cannot reset int\n");
+
+ err = regmap_field_write(msa311->fields[F_INT1_NEW_DATA], 1);
+ if (err)
+ return dev_err_probe(dev, err, "cannot map new data int\n");
+
+ return 0;
+}
+
+static int msa311_regmap_init(struct msa311_priv *msa311)
+{
+ struct regmap_field **fields = msa311->fields;
+ struct device *dev = &msa311->i2c->dev;
+ struct regmap *regmap;
+ int i;
+
+ regmap = devm_regmap_init_i2c(msa311->i2c, &msa311_regmap_config);
+ if (IS_ERR(regmap))
+ return dev_err_probe(dev, PTR_ERR(regmap),
+ "failed to register i2c regmap\n");
+
+ msa311->regs = regmap;
+
+ for (i = 0; i < F_MAX_FIELDS; ++i) {
+ fields[i] = devm_regmap_field_alloc(dev,
+ msa311->regs,
+ msa311_reg_fields[i]);
+ if (IS_ERR(msa311->fields[i])) {
+ return dev_err_probe(dev, PTR_ERR(msa311->fields[i]),
+ "cannot alloc reg field[%d]\n", i);
+ }
+ }
+
+ return 0;
+}
+
+static void msa311_powerdown(void *dev)
+{
+ /* Resume device if any */
+ pm_runtime_get_sync(dev);
+
+ /* Suspend device right now */
+ pm_runtime_put_sync_suspend(dev);
+}
+
+static int msa311_probe(struct i2c_client *i2c)
+{
+ struct msa311_priv *msa311;
+ struct iio_dev *indio_dev;
+ struct device *dev = &i2c->dev;
+ int err;
+
+ indio_dev = devm_iio_device_alloc(dev, sizeof(*msa311));
+ if (!indio_dev)
+ return dev_err_probe(dev, -ENOMEM,
+ "iio device allocation failed\n");
+
+ msa311 = iio_priv(indio_dev);
+ msa311->i2c = i2c;
+ i2c_set_clientdata(i2c, indio_dev);
+
+ err = msa311_regmap_init(msa311);
+ if (err)
+ return err;
+
+ mutex_init(&msa311->lock);
+
+ err = devm_pm_runtime_enable(dev);
+ if (err)
+ return err;
+
+ /* Resume msa311 logic before any interactions with registers */
+ err = pm_runtime_resume_and_get(dev);
+ if (err)
+ return err;
+
+ pm_runtime_set_autosuspend_delay(dev, MSA311_PWR_SLEEP_DELAY_MS);
+ pm_runtime_use_autosuspend(dev);
+
+ err = msa311_chip_init(msa311);
+ if (err)
+ return err;
+
+ indio_dev->modes = INDIO_DIRECT_MODE; /* setup buffered mode later */
+ indio_dev->channels = msa311_channels;
+ indio_dev->num_channels = ARRAY_SIZE(msa311_channels);
+ indio_dev->name = i2c->name;
+ indio_dev->info = &msa311_info;
+
+ err = devm_iio_triggered_buffer_setup(dev,
+ indio_dev,
+ iio_pollfunc_store_time,
+ msa311_buffer_thread,
+ &msa311_buffer_setup_ops);
+ if (err)
+ return dev_err_probe(dev, err, "cannot setup iio trig buf\n");
+
+ if (i2c->irq > 0) {
+ err = msa311_setup_interrupts(msa311);
+ if (err)
+ return err;
+ }
+
+ pm_runtime_mark_last_busy(dev);
+ pm_runtime_put_autosuspend(dev);
+
+ /*
+ * Register powerdown deferred callback which suspends the chip
+ * after module unloaded.
+ *
+ * MSA311 should be in SUSPEND mode in the two cases:
+ * 1) When driver is loaded, but we do not have any data or
+ * configuration requests to it (we are solving it using
+ * autosuspend feature).
+ * 2) When driver is unloaded and device is not used (devm action is
+ * used in this case).
+ */
+ err = devm_add_action_or_reset(dev, msa311_powerdown, dev);
+ if (err)
+ return dev_err_probe(dev, err, "cannot add powerdown action\n");
+
+ err = devm_iio_device_register(dev, indio_dev);
+ if (err)
+ return dev_err_probe(dev, err, "iio device register failed\n");
+
+ return 0;
+}
+
+static int msa311_runtime_suspend(struct device *dev)
+{
+ struct iio_dev *indio_dev = dev_get_drvdata(dev);
+ struct msa311_priv *msa311 = iio_priv(indio_dev);
+ int err;
+
+ dev_dbg(dev, "suspending %s\n", dev->driver->name);
+
+ mutex_lock(&msa311->lock);
+ err = msa311_set_pwr_mode(msa311, MSA311_PWR_MODE_SUSPEND);
+ mutex_unlock(&msa311->lock);
+
+ if (err) {
+ dev_err(dev, "failed to power off device (%d)\n", err);
+ return err;
+ }
+
+ return 0;
+}
+
+static int msa311_runtime_resume(struct device *dev)
+{
+ struct iio_dev *indio_dev = dev_get_drvdata(dev);
+ struct msa311_priv *msa311 = iio_priv(indio_dev);
+ int err;
+
+ dev_dbg(dev, "resuming %s\n", dev->driver->name);
+
+ mutex_lock(&msa311->lock);
+ err = msa311_set_pwr_mode(msa311, MSA311_PWR_MODE_NORMAL);
+ mutex_unlock(&msa311->lock);
+
+ if (err) {
+ dev_err(dev, "failed to power on device (%d)\n", err);
+ return err;
+ }
+
+ return 0;
+}
+
+static DEFINE_RUNTIME_DEV_PM_OPS(msa311_pm_ops, msa311_runtime_suspend,
+ msa311_runtime_resume, NULL);
+
+static const struct i2c_device_id msa311_i2c_id[] = {
+ { .name = "msa311" },
+ { }
+};
+MODULE_DEVICE_TABLE(i2c, msa311_i2c_id);
+
+static const struct of_device_id msa311_of_match[] = {
+ { .compatible = "memsensing,msa311" },
+ { }
+};
+MODULE_DEVICE_TABLE(of, msa311_of_match);
+
+static struct i2c_driver msa311_driver = {
+ .driver = {
+ .name = "msa311",
+ .owner = THIS_MODULE,
+ .of_match_table = msa311_of_match,
+ .pm = pm_ptr(&msa311_pm_ops),
+ },
+ .probe_new = msa311_probe,
+ .id_table = msa311_i2c_id,
+};
+
+module_i2c_driver(msa311_driver);
+
+MODULE_AUTHOR("Dmitry Rokosov <ddrokosov@sberdevices.ru>");
+MODULE_DESCRIPTION("MEMSensing MSA311 3-axis accelerometer driver");
+MODULE_LICENSE("GPL v2");
--
2.36.0
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v3 3/3] dt-bindings: iio: accel: add dt-binding schema for msa311 accel driver
2022-06-16 10:42 [PATCH v3 0/3] iio: accel: add MSA311 accelerometer driver Dmitry Rokosov
2022-06-16 10:42 ` [PATCH v3 1/3] dt-bindings: vendor-prefixes: add MEMSensing Microsystems Co., Ltd Dmitry Rokosov
2022-06-16 10:42 ` [PATCH v3 2/3] iio: add MEMSensing MSA311 3-axis accelerometer driver Dmitry Rokosov
@ 2022-06-16 10:42 ` Dmitry Rokosov
2022-06-19 11:40 ` Jonathan Cameron
2 siblings, 1 reply; 16+ messages in thread
From: Dmitry Rokosov @ 2022-06-16 10:42 UTC (permalink / raw)
To: robh+dt, stano.jakubek, shawnguo, jic23, lars, andy.shevchenko, stephan
Cc: linux-iio, devicetree, kernel, linux-kernel, Dmitry Rokosov
Introduce devicetree binding json-schema for MSA311 tri-axial,
low-g accelerometer driver.
Signed-off-by: Dmitry Rokosov <ddrokosov@sberdevices.ru>
---
.../bindings/iio/accel/memsensing,msa311.yaml | 52 +++++++++++++++++++
MAINTAINERS | 1 +
2 files changed, 53 insertions(+)
create mode 100644 Documentation/devicetree/bindings/iio/accel/memsensing,msa311.yaml
diff --git a/Documentation/devicetree/bindings/iio/accel/memsensing,msa311.yaml b/Documentation/devicetree/bindings/iio/accel/memsensing,msa311.yaml
new file mode 100644
index 000000000000..072632708d42
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/accel/memsensing,msa311.yaml
@@ -0,0 +1,52 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+
+%YAML 1.2
+---
+$id: "http://devicetree.org/schemas/iio/accel/memsensing,msa311.yaml#"
+$schema: "http://devicetree.org/meta-schemas/core.yaml#"
+
+title: MEMSensing digital 3-Axis accelerometer
+
+maintainers:
+ - Dmitry Rokosov <ddrokosov@sberdevices.ru>
+
+description: |
+ MSA311 is a tri-axial, low-g accelerometer with I2C digital output for
+ sensitivity consumer applications. It has dynamical user selectable full
+ scales range of +-2g/+-4g/+-8g/+-16g and allows acceleration measurements
+ with output data rates from 1Hz to 1000Hz.
+ Datasheet can be found at following URL
+ https://cdn-shop.adafruit.com/product-files/5309/MSA311-V1.1-ENG.pdf
+
+properties:
+ compatible:
+ const: memsensing,msa311
+
+ reg:
+ maxItems: 1
+ description: I2C registers address
+
+ interrupts:
+ maxItems: 1
+ description: optional I2C int pin can be freely mapped to specific func
+
+required:
+ - compatible
+ - reg
+
+additionalProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/interrupt-controller/irq.h>
+ i2c {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ accelerometer@62 {
+ compatible = "memsensing,msa311";
+ reg = <0x62>;
+ interrupt-parent = <&gpio_intc>;
+ interrupts = <29 IRQ_TYPE_EDGE_RISING>;
+ };
+ };
diff --git a/MAINTAINERS b/MAINTAINERS
index 55aeb25c004c..be39e5c214fe 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12482,6 +12482,7 @@ MEMSENSING MICROSYSTEMS MSA311 ACCELEROMETER DRIVER
M: Dmitry Rokosov <ddrokosov@sberdevices.ru>
L: linux-iio@vger.kernel.org
S: Maintained
+F: Documentation/devicetree/bindings/iio/accel/memsensing,msa311.yaml
F: drivers/iio/accel/msa311.c
MEN A21 WATCHDOG DRIVER
--
2.36.0
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v3 2/3] iio: add MEMSensing MSA311 3-axis accelerometer driver
2022-06-16 10:42 ` [PATCH v3 2/3] iio: add MEMSensing MSA311 3-axis accelerometer driver Dmitry Rokosov
@ 2022-06-16 12:18 ` Andy Shevchenko
2022-06-16 17:02 ` Dmitry Rokosov
2022-06-19 12:27 ` Jonathan Cameron
1 sibling, 1 reply; 16+ messages in thread
From: Andy Shevchenko @ 2022-06-16 12:18 UTC (permalink / raw)
To: Dmitry Rokosov
Cc: robh+dt, stano.jakubek, shawnguo, jic23, lars, stephan,
linux-iio, devicetree, kernel, linux-kernel
On Thu, Jun 16, 2022 at 12:42 PM Dmitry Rokosov
<DDRokosov@sberdevices.ru> wrote:
>
> MSA311 is a tri-axial, low-g accelerometer with I2C digital output for
> sensitivity consumer applications. It has dynamical user selectable full
dynamic
user-selectable
> scales range of +-2g/+-4g/+-8g/+-16g and allows acceleration measurements
> with output data rates from 1Hz to 1000Hz.
>
> Datasheet can be found at following URL:
> https://cdn-shop.adafruit.com/product-files/5309/MSA311-V1.1-ENG.pdf
Make it a tag. (Move closer to SoB without any blank lines and
keeping as one tag == one line, even if it's longer than any limit)
> This driver supports following MSA311 features:
> - IIO interface
> - Different power modes: NORMAL and SUSPEND (using pm_runtime)
> - ODR (Output Data Rate) selection
> - Scale and samp_freq selection
> - IIO triggered buffer, IIO reg access
> - NEW_DATA interrupt + trigger
>
> Below features to be done:
> - Motion Events: ACTIVE, TAP, ORIENT, FREEFALL
> - Low Power mode
...
> +MEMSENSING MICROSYSTEMS MSA311 ACCELEROMETER DRIVER
If MSA311 is accelerometer _only_, then the word "ACCELEROMETER" can be dropped.
...
> +/*
> + * msa311.c - MEMSensing digital 3-Axis accelerometer
Don't put the filename inside the file. Rationale -- less churn and
error prone in case it would be renamed.
> + *
> + * MSA311 is a tri-axial, low-g accelerometer with I2C digital output for
> + * sensitivity consumer applications. It has dynamical user selectable full
dynamic
user-selectable
> + * scales range of +-2g/+-4g/+-8g/+-16g and allows acceleration measurements
> + * with output data rates from 1Hz to 1000Hz.
> + *
> + * MSA311 is available in an ultra small (2mm x 2mm, height 0.95mm) LGA package
> + * and is guaranteed to operate over -40C to +85C.
> + *
> + * This driver supports following MSA311 features:
> + * - IIO interface
> + * - Different power modes: NORMAL, SUSPEND
> + * - ODR (Output Data Rate) selection
> + * - Scale selection
> + * - IIO triggered buffer
> + * - NEW_DATA interrupt + trigger
> + *
> + * Below features to be done:
> + * - Motion Events: ACTIVE, TAP, ORIENT, FREEFALL
> + * - Low Power mode
> + *
> + * Copyright (c) 2022, SberDevices. All Rights Reserved.
> + *
> + * Author: Dmitry Rokosov <ddrokosov@sberdevices.ru>
> + */
...
> +enum msa311_fields {
> + F_SOFT_RESET_I2C, F_SOFT_RESET_SPI,
> +
> + F_ORIENT_INT, F_S_TAP_INT, F_D_TAP_INT, F_ACTIVE_INT, F_FREEFALL_INT,
> +
> + F_NEW_DATA_INT,
> +
> + F_TAP_SIGN, F_TAP_FIRST_X, F_TAP_FIRST_Y, F_TAP_FIRST_Z, F_ACTV_SIGN,
> + F_ACTV_FIRST_X, F_ACTV_FIRST_Y, F_ACTV_FIRST_Z,
> +
> + F_ORIENT_Z, F_ORIENT_X_Y,
> +
> + F_FS,
> +
> + F_X_AXIS_DIS, F_Y_AXIS_DIS, F_Z_AXIS_DIS, F_ODR,
> +
> + F_PWR_MODE, F_LOW_POWER_BW,
> +
> + F_X_POLARITY, F_Y_POLARITY, F_Z_POLARITY, F_X_Y_SWAP,
> +
> + F_ORIENT_INT_EN, F_S_TAP_INT_EN, F_D_TAP_INT_EN, F_ACTIVE_INT_EN_Z,
> + F_ACTIVE_INT_EN_Y, F_ACTIVE_INT_EN_X,
> +
> + F_NEW_DATA_INT_EN, F_FREEFALL_INT_EN,
> +
> + F_INT1_ORIENT, F_INT1_S_TAP, F_INT1_D_TAP, F_INT1_ACTIVE,
> + F_INT1_FREEFALL,
> +
> + F_INT1_NEW_DATA,
> +
> + F_INT1_OD, F_INT1_LVL,
> +
> + F_RESET_INT, F_LATCH_INT,
> +
> + F_FREEFALL_MODE, F_FREEFALL_HY,
> +
> + F_ACTIVE_DUR,
> +
> + F_TAP_QUIET, F_TAP_SHOCK, F_TAP_DUR,
> +
> + F_TAP_TH,
> +
> + F_ORIENT_HYST, F_ORIENT_BLOCKING, F_ORIENT_MODE,
> +
> + F_Z_BLOCKING,
> +
> + F_MAX_FIELDS,
Not sure why you put those blank lines herey, it makes code not compact.
> +};
...
> +/* All supported power modes */
> +#define MSA311_PWR_MODE_NORMAL 0b00
#define MSA311_PWR_MODE_UNKNOWN_01 0b01
#define MSA311_PWR_MODE_UNKNOWN_10 0b01
> +#define MSA311_PWR_MODE_SUSPEND 0b11
const char * const msa311_pwr_modes[] = { "normal", "unknown_01",
"unknown_10", "suspend" };
See below why this is here.
...
> +#define MSA311_GENMASK(field) ({ \
> + typeof(field) _field = (field); \
Why not define a pointer to the field here?
_field = &msa311_reg_fields[field];
GENMASK(_field->msb, _field->lsb);
> + GENMASK(msa311_reg_fields[_field].msb, \
> + msa311_reg_fields[_field].lsb); \
> +})
...
> +struct msa311_priv {
> + struct i2c_client *i2c;
Not sure you need this. Dropping i2c dependency from this structure
allows much easier to add, e.g. SPI support of the same hardware.
> + struct mutex lock; /* state guard */
> +
> + struct iio_trigger *new_data_trig;
> +
> + struct regmap *regs;
I believe this is used most, so making this a first member in the
structure saves some instructions (check with bloat-o-meter).
> + struct regmap_field *fields[F_MAX_FIELDS];
> +};
...
> + /* Filter bad ODR values */
> + switch (pwr_mode) {
> + case MSA311_PWR_MODE_NORMAL:
> + mode = "normal";
> + good_odr = (odr > MSA311_ODR_1_95_HZ);
> + break;
> + case MSA311_PWR_MODE_SUSPEND:
> + mode = "suspend";
> + break;
> + default:
> + mode = "unknown";
> + break;
> + }
As mentioned below you may think about an array of const char * const [].
...
> + wait_ms = (USEC_PER_SEC * MSEC_PER_SEC) / freq_uhz;
This looks very odd from a physics perspective: sec * sec * sec == sec ?!
Perhaps you meant some HZ* macros from units.h?
...
> + dev_dbg(dev, "transition to %s mode\n",
> + (mode == MSA311_PWR_MODE_NORMAL) ? "normal" :
> + (mode == MSA311_PWR_MODE_SUSPEND) ? "suspend" :
> + "unknown");
I would rather put it (near to the mode enum / definitions) like
const char * const msa311_pwr_modes[] = { "normal", "suspend" };
(Also it seems you may reuse this above.)
And use here something like
mode >= ARRAY_SIZE() ? "unknown" : _pwr_modes[mode];
...
> + /* Wait actual data if we wakeup */
wake up
...
> + /* We do not have fs >= 1, so skip such values */
> + if (val != 0)
if (val)
> + return 0;
...
> + err = -EINVAL;
> + mutex_lock(&msa311->lock);
> + for (odr = 0; odr < ARRAY_SIZE(msa311_odr_table); ++odr)
> + if (val == msa311_odr_table[odr].val &&
> + val2 == msa311_odr_table[odr].val2) {
> + err = msa311_set_odr(msa311, odr);
> + if (err) {
> + dev_err(dev, "cannot update freq (%d)\n", err);
> + goto failed;
> + }
Why is this inside the loop and more important under lock? Also you
may cover the initial error code by this message when moving it out of
the loop and lock.
Ditto for other code snippets in other function(s) where applicable.
> + break;
> + }
> +
> +failed:
> + mutex_unlock(&msa311->lock);
...
> + if (!readval)
Why not positive conditional?
Same Q whenever this is the case.
> + err = regmap_write(msa311->regs, reg, writeval);
> + else
> + err = regmap_read(msa311->regs, reg, readval);
...
> + mutex_lock(&msa311->lock);
> + err = regmap_field_write(msa311->fields[F_NEW_DATA_INT_EN], state);
> + mutex_unlock(&msa311->lock);
> +
No need.
> + if (err)
> + dev_err(dev,
> + "cannot %s buffer due to new_data_int failure (%d)\n",
> + state ? "enable" : "disable", err);
str_enable_disable()
...
> + return (iio_trigger_get_drvdata(trig) != indio_dev) ? -EINVAL : 0;
Unnecessary parentheses and why not the positive conditional?
return ... == ... ? 0 : ...;
...
> + __le16 axis;
> + int bit = 0, err, i = 0;
> +
Unnecessary blank line.
...
> + /* TODO: send motion events if needed */
Are you going to address all TODOs? If no, drop ones that you are not
going to address in the future, better to put into the top of the file
comment what is supported and what's not.
...
> + dev_dbg(dev, "Found MSA311 compatible chip[%#x]\n", partid);
> +
> + if (partid != MSA311_WHO_AM_I)
> + dev_warn(dev, "invalid partid (%#x), expected (%#x)\n",
> + partid, MSA311_WHO_AM_I);
I would put it rather like:
if (partid == ...)
dev_dbg("Found compatible chip'n");
else
dev_warn(...);
...
> + if (IS_ERR(msa311->fields[i])) {
> + return dev_err_probe(dev, PTR_ERR(msa311->fields[i]),
> + "cannot alloc reg field[%d]\n", i);
> + }
{} are not needed.
...
> + struct msa311_priv *msa311;
> + struct iio_dev *indio_dev;
> + struct device *dev = &i2c->dev;
Making it first line (the rule of thumb "longer lines first") here and
in any other similar cases?
...
> + indio_dev->modes = INDIO_DIRECT_MODE; /* setup buffered mode later */
Leaving it 0 is probably better, it's a pity that we don't have
INDIO_NO_MODE_SET 0 there. However, I haven't checked if it's possible
to leave it unset like this.
...
> + if (i2c->irq > 0) {
> + err = msa311_setup_interrupts(msa311);
> + if (err)
> + return err;
> + }
Slightly cleaner is to save what's needed in the your private
structure, drop i2c_client pointer and move the above check inside the
function:
/* IRQ is optional */
if (msa311->irq <= 0)
return 0;
But see if it's indeed better...
...
> +static int msa311_runtime_suspend(struct device *dev)
See comments for ->resume() below.
...
> + dev_dbg(dev, "resuming %s\n", dev->driver->name);
Useless. Remove in other places. In case you need it, use ftracer and
PM suspend/resume debugging features.
> + mutex_lock(&msa311->lock);
> + err = msa311_set_pwr_mode(msa311, MSA311_PWR_MODE_NORMAL);
> + mutex_unlock(&msa311->lock);
> +
No need.
> + if (err) {
> + dev_err(dev, "failed to power on device (%d)\n", err);
> + return err;
> + }
> +
> + return 0;
return err;
4 LoCs --> 1 LoC
...
> +static struct i2c_driver msa311_driver = {
> + .driver = {
> + .name = "msa311",
> + .owner = THIS_MODULE,
> + .of_match_table = msa311_of_match,
> + .pm = pm_ptr(&msa311_pm_ops),
> + },
> + .probe_new = msa311_probe,
> + .id_table = msa311_i2c_id,
> +};
> +
No need for a blank line here.
> +module_i2c_driver(msa311_driver);
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3 2/3] iio: add MEMSensing MSA311 3-axis accelerometer driver
2022-06-16 12:18 ` Andy Shevchenko
@ 2022-06-16 17:02 ` Dmitry Rokosov
2022-06-16 18:38 ` Andy Shevchenko
2022-06-19 12:31 ` Jonathan Cameron
0 siblings, 2 replies; 16+ messages in thread
From: Dmitry Rokosov @ 2022-06-16 17:02 UTC (permalink / raw)
To: Andy Shevchenko
Cc: robh+dt, stano.jakubek, shawnguo, jic23, lars, stephan,
linux-iio, devicetree, kernel, linux-kernel
Hello Andy,
Thank you for the quick review. Please find my comments below.
On Thu, Jun 16, 2022 at 02:18:52PM +0200, Andy Shevchenko wrote:
> On Thu, Jun 16, 2022 at 12:42 PM Dmitry Rokosov
> <DDRokosov@sberdevices.ru> wrote:
...
> > +enum msa311_fields {
> > + F_SOFT_RESET_I2C, F_SOFT_RESET_SPI,
> > +
> > + F_ORIENT_INT, F_S_TAP_INT, F_D_TAP_INT, F_ACTIVE_INT, F_FREEFALL_INT,
> > +
> > + F_NEW_DATA_INT,
> > +
> > + F_TAP_SIGN, F_TAP_FIRST_X, F_TAP_FIRST_Y, F_TAP_FIRST_Z, F_ACTV_SIGN,
> > + F_ACTV_FIRST_X, F_ACTV_FIRST_Y, F_ACTV_FIRST_Z,
> > +
> > + F_ORIENT_Z, F_ORIENT_X_Y,
> > +
> > + F_FS,
> > +
> > + F_X_AXIS_DIS, F_Y_AXIS_DIS, F_Z_AXIS_DIS, F_ODR,
> > +
> > + F_PWR_MODE, F_LOW_POWER_BW,
> > +
> > + F_X_POLARITY, F_Y_POLARITY, F_Z_POLARITY, F_X_Y_SWAP,
> > +
> > + F_ORIENT_INT_EN, F_S_TAP_INT_EN, F_D_TAP_INT_EN, F_ACTIVE_INT_EN_Z,
> > + F_ACTIVE_INT_EN_Y, F_ACTIVE_INT_EN_X,
> > +
> > + F_NEW_DATA_INT_EN, F_FREEFALL_INT_EN,
> > +
> > + F_INT1_ORIENT, F_INT1_S_TAP, F_INT1_D_TAP, F_INT1_ACTIVE,
> > + F_INT1_FREEFALL,
> > +
> > + F_INT1_NEW_DATA,
> > +
> > + F_INT1_OD, F_INT1_LVL,
> > +
> > + F_RESET_INT, F_LATCH_INT,
> > +
> > + F_FREEFALL_MODE, F_FREEFALL_HY,
> > +
> > + F_ACTIVE_DUR,
> > +
> > + F_TAP_QUIET, F_TAP_SHOCK, F_TAP_DUR,
> > +
> > + F_TAP_TH,
> > +
> > + F_ORIENT_HYST, F_ORIENT_BLOCKING, F_ORIENT_MODE,
> > +
> > + F_Z_BLOCKING,
> > +
> > + F_MAX_FIELDS,
>
> Not sure why you put those blank lines herey, it makes code not compact.
>
Here I use blank lines to split fields from different registers.
In other words, in the msa311_fields enum one line contains fields from one
register. But for some heavy registers (like TAP_ACTIVE_STS) we have so many
fields and their declaration doesn't fit to 80 symbols.
So I've made a decision to split registers using blank lines.
...
> > +struct msa311_priv {
>
> > + struct i2c_client *i2c;
>
> Not sure you need this. Dropping i2c dependency from this structure
> allows much easier to add, e.g. SPI support of the same hardware.
>
Mainly I use i2c pointer in the probe() path, and you are right, we can
change i2c pointer to dev and generalize msa311_priv struct from bus
perspective.
> > + struct mutex lock; /* state guard */
> > +
> > + struct iio_trigger *new_data_trig;
> > +
> > + struct regmap *regs;
>
> I believe this is used most, so making this a first member in the
> structure saves some instructions (check with bloat-o-meter).
>
Are you talking about archs where offset calculation adds more bytes to
instruction? And when offset equals to 0, we can save some space.
...
> > + struct regmap_field *fields[F_MAX_FIELDS];
> > +};
>
> ...
>
> > + wait_ms = (USEC_PER_SEC * MSEC_PER_SEC) / freq_uhz;
>
> This looks very odd from a physics perspective: sec * sec * sec == sec ?!
>
> Perhaps you meant some HZ* macros from units.h?
>
I suppose because of UHZ calculation I have to use NANO instead of
USEC_PER_SEC in the following line:
freq_uhz = msa311_odr_table[odr].val * USEC_PER_SEC +
msa311_odr_table[odr].val2;
But below line is right from physics perspective. 1sec = 1/Hz, so
msec = (USEC_PER_SEC / freq_uhz) * MSEC_PER_SEC:
wait_ms = (USEC_PER_SEC * MSEC_PER_SEC) / freq_uhz;
Or do you mean that I should change MSEC_PER_SEC to just MILLI?
> > + err = -EINVAL;
> > + mutex_lock(&msa311->lock);
> > + for (odr = 0; odr < ARRAY_SIZE(msa311_odr_table); ++odr)
> > + if (val == msa311_odr_table[odr].val &&
> > + val2 == msa311_odr_table[odr].val2) {
> > + err = msa311_set_odr(msa311, odr);
>
> > + if (err) {
> > + dev_err(dev, "cannot update freq (%d)\n", err);
> > + goto failed;
> > + }
>
> Why is this inside the loop and more important under lock? Also you
> may cover the initial error code by this message when moving it out of
> the loop and lock.
>
> Ditto for other code snippets in other function(s) where applicable.
>
Yes, I can move dev_err() outside of loop. But all ODR search loop
should be under lock fully, because other msa311 operations should not
be executed when we search proper ODR place.
...
> > + mutex_lock(&msa311->lock);
> > + err = regmap_field_write(msa311->fields[F_NEW_DATA_INT_EN], state);
> > + mutex_unlock(&msa311->lock);
>
> > +
>
> No need.
>
Sorry, I don't understand. We do not need to call it under lock, right?
I think we have to wrap it by msa311 lock, because other msa311
operations should not be executed when we enable or disable new data
interrupt (for example ODR value changing or something else).
> > + if (err)
> > + dev_err(dev,
> > + "cannot %s buffer due to new_data_int failure (%d)\n",
> > + state ? "enable" : "disable", err);
>
> str_enable_disable()
>
> ...
>
Kernel has solutions on all occasions :-)
...
> > + /* TODO: send motion events if needed */
>
> Are you going to address all TODOs? If no, drop ones that you are not
> going to address in the future, better to put into the top of the file
> comment what is supported and what's not.
>
> ...
>
Yes, I plan to maintain this driver and implement all motion event,
that's why I've placed TODO throughout the code.
> > + indio_dev->modes = INDIO_DIRECT_MODE; /* setup buffered mode later */
>
> Leaving it 0 is probably better, it's a pity that we don't have
> INDIO_NO_MODE_SET 0 there. However, I haven't checked if it's possible
> to leave it unset like this.
>
> ...
>
I set INDIO_DIRECT_MODE by default, because this driver support noirq
mode, when device tree doesn't have irq setup.
...
> > + mutex_lock(&msa311->lock);
> > + err = msa311_set_pwr_mode(msa311, MSA311_PWR_MODE_NORMAL);
> > + mutex_unlock(&msa311->lock);
>
> > +
>
> No need.
>
Again I don't understand why, sorry. We do not want to get sporadic
MSA311 attributes changing during power mode transition from another
userspace process.
--
Thank you,
Dmitry
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3 2/3] iio: add MEMSensing MSA311 3-axis accelerometer driver
2022-06-16 17:02 ` Dmitry Rokosov
@ 2022-06-16 18:38 ` Andy Shevchenko
2022-06-17 14:22 ` Dmitry Rokosov
2022-06-19 12:31 ` Jonathan Cameron
1 sibling, 1 reply; 16+ messages in thread
From: Andy Shevchenko @ 2022-06-16 18:38 UTC (permalink / raw)
To: Dmitry Rokosov
Cc: robh+dt, stano.jakubek, shawnguo, jic23, lars, stephan,
linux-iio, devicetree, kernel, linux-kernel
On Thu, Jun 16, 2022 at 7:02 PM Dmitry Rokosov <DDRokosov@sberdevices.ru> wrote:
> On Thu, Jun 16, 2022 at 02:18:52PM +0200, Andy Shevchenko wrote:
> > On Thu, Jun 16, 2022 at 12:42 PM Dmitry Rokosov
> > <DDRokosov@sberdevices.ru> wrote:
...
> > Not sure why you put those blank lines herey, it makes code not compact.
>
> Here I use blank lines to split fields from different registers.
> In other words, in the msa311_fields enum one line contains fields from one
> register. But for some heavy registers (like TAP_ACTIVE_STS) we have so many
> fields and their declaration doesn't fit to 80 symbols.
> So I've made a decision to split registers using blank lines.
Better is to add a comment explaining what register is described
below, and not just a blank line.
...
> > Not sure you need this. Dropping i2c dependency from this structure
> > allows much easier to add, e.g. SPI support of the same hardware.
>
> Mainly I use i2c pointer in the probe() path, and you are right, we can
> change i2c pointer to dev and generalize msa311_priv struct from bus
> perspective.
Yep, note that you may easily retrieve i2c_client from struct device
pointer if you need to do that in some (I believe rare to none) cases.
...
> > > + struct regmap *regs;
> >
> > I believe this is used most, so making this a first member in the
> > structure saves some instructions (check with bloat-o-meter).
> >
>
> Are you talking about archs where offset calculation adds more bytes to
> instruction? And when offset equals to 0, we can save some space.
It doesn't have anything to do with arches, simply compiler
optimization, otherwise yes, that's what I meant.
...
> > > + wait_ms = (USEC_PER_SEC * MSEC_PER_SEC) / freq_uhz;
> >
> > This looks very odd from a physics perspective: sec * sec * sec == sec ?!
> >
> > Perhaps you meant some HZ* macros from units.h?
> >
>
> I suppose because of UHZ calculation I have to use NANO instead of
> USEC_PER_SEC in the following line:
>
> freq_uhz = msa311_odr_table[odr].val * USEC_PER_SEC +
> msa311_odr_table[odr].val2;
>
> But below line is right from physics perspective. 1sec = 1/Hz, so
> msec = (USEC_PER_SEC / freq_uhz) * MSEC_PER_SEC:
>
> wait_ms = (USEC_PER_SEC * MSEC_PER_SEC) / freq_uhz;
>
> Or do you mean that I should change MSEC_PER_SEC to just MILLI?
1 / Hz = 1 sec. That's how physics defines it. Try to figure out what
you meant by above multiplications / divisions and come up with the
best that fits your purposes.
...
> > > + if (err) {
> > > + dev_err(dev, "cannot update freq (%d)\n", err);
> > > + goto failed;
> > > + }
> >
> > Why is this inside the loop and more important under lock? Also you
> > may cover the initial error code by this message when moving it out of
> > the loop and lock.
> >
> > Ditto for other code snippets in other function(s) where applicable.
>
> Yes, I can move dev_err() outside of loop. But all ODR search loop
> should be under lock fully, because other msa311 operations should not
> be executed when we search proper ODR place.
I didn't suggest getting rid of the lock.
...
> > > + mutex_lock(&msa311->lock);
> > > + err = regmap_field_write(msa311->fields[F_NEW_DATA_INT_EN], state);
> > > + mutex_unlock(&msa311->lock);
> >
> > > +
> >
> > No need.
>
> Sorry, I don't understand. We do not need to call it under lock, right?
> I think we have to wrap it by msa311 lock, because other msa311
> operations should not be executed when we enable or disable new data
> interrupt (for example ODR value changing or something else).
The blank line is not needed, I specifically commented on the
emphasized paragraph (by delimiting it with blank lines and leaving
the rest for the better context for you to understand, it seems it did
the opposite...).
...
> > > + mutex_lock(&msa311->lock);
> > > + err = msa311_set_pwr_mode(msa311, MSA311_PWR_MODE_NORMAL);
> > > + mutex_unlock(&msa311->lock);
> >
> > > +
> >
> > No need.
>
> Again I don't understand why, sorry. We do not want to get sporadic
> MSA311 attributes changing during power mode transition from another
> userspace process.
As per above.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3 2/3] iio: add MEMSensing MSA311 3-axis accelerometer driver
2022-06-16 18:38 ` Andy Shevchenko
@ 2022-06-17 14:22 ` Dmitry Rokosov
2022-06-17 16:54 ` Andy Shevchenko
0 siblings, 1 reply; 16+ messages in thread
From: Dmitry Rokosov @ 2022-06-17 14:22 UTC (permalink / raw)
To: Andy Shevchenko
Cc: robh+dt, stano.jakubek, shawnguo, jic23, lars, stephan,
linux-iio, devicetree, kernel, linux-kernel
On Thu, Jun 16, 2022 at 08:38:46PM +0200, Andy Shevchenko wrote:
> On Thu, Jun 16, 2022 at 7:02 PM Dmitry Rokosov <DDRokosov@sberdevices.ru> wrote:
> > On Thu, Jun 16, 2022 at 02:18:52PM +0200, Andy Shevchenko wrote:
> > > On Thu, Jun 16, 2022 at 12:42 PM Dmitry Rokosov
> > > <DDRokosov@sberdevices.ru> wrote:
>
> ...
>
> > > Not sure why you put those blank lines herey, it makes code not compact.
> >
> > Here I use blank lines to split fields from different registers.
> > In other words, in the msa311_fields enum one line contains fields from one
> > register. But for some heavy registers (like TAP_ACTIVE_STS) we have so many
> > fields and their declaration doesn't fit to 80 symbols.
> > So I've made a decision to split registers using blank lines.
>
> Better is to add a comment explaining what register is described
> below, and not just a blank line.
>
> ...
>
Agreed, I'll do that in the v4.
...
> > > > + wait_ms = (USEC_PER_SEC * MSEC_PER_SEC) / freq_uhz;
> > >
> > > This looks very odd from a physics perspective: sec * sec * sec == sec ?!
> > >
> > > Perhaps you meant some HZ* macros from units.h?
> > >
> >
> > I suppose because of UHZ calculation I have to use NANO instead of
> > USEC_PER_SEC in the following line:
> >
> > freq_uhz = msa311_odr_table[odr].val * USEC_PER_SEC +
> > msa311_odr_table[odr].val2;
> >
> > But below line is right from physics perspective. 1sec = 1/Hz, so
> > msec = (USEC_PER_SEC / freq_uhz) * MSEC_PER_SEC:
> >
> > wait_ms = (USEC_PER_SEC * MSEC_PER_SEC) / freq_uhz;
> >
> > Or do you mean that I should change MSEC_PER_SEC to just MILLI?
>
> 1 / Hz = 1 sec. That's how physics defines it. Try to figure out what
> you meant by above multiplications / divisions and come up with the
> best that fits your purposes.
>
> ...
>
From my point of view, I've already implemented the best way to calculate
how much time I need to wait for the next data chunk based on ODR Hz
value :-)
ODR value from the table has val integer part and val2 in microHz.
By this line we calculate microHz conversion to take into account val2
part:
freq_uhz = msa311_odr_table[odr].val * USEC_PER_SEC +
msa311_odr_table[odr].val2;
By the next line we try to calculate miliseconds for msleep() from ODR
microHz value:
wait_ms = (USEC_PER_SEC * MSEC_PER_SEC) / freq_uhz;
(USEC_PER_SEC / freq_uhz) => seconds
seconds * MSEC_PER_SEC => milliseconds
USEC_PER_SEC and MSEC_PER_SEC are just coefficients, they are not
measured in "seconds" units.
> > > > + if (err) {
> > > > + dev_err(dev, "cannot update freq (%d)\n", err);
> > > > + goto failed;
> > > > + }
> > >
> > > Why is this inside the loop and more important under lock? Also you
> > > may cover the initial error code by this message when moving it out of
> > > the loop and lock.
> > >
> > > Ditto for other code snippets in other function(s) where applicable.
> >
> > Yes, I can move dev_err() outside of loop. But all ODR search loop
> > should be under lock fully, because other msa311 operations should not
> > be executed when we search proper ODR place.
>
> I didn't suggest getting rid of the lock.
>
> ...
>
Sorry, I didn't get you... But I fully agree with you about dev_err()
movement.
> > > > + mutex_lock(&msa311->lock);
> > > > + err = regmap_field_write(msa311->fields[F_NEW_DATA_INT_EN], state);
> > > > + mutex_unlock(&msa311->lock);
> > >
> > > > +
> > >
> > > No need.
> >
> > Sorry, I don't understand. We do not need to call it under lock, right?
> > I think we have to wrap it by msa311 lock, because other msa311
> > operations should not be executed when we enable or disable new data
> > interrupt (for example ODR value changing or something else).
>
> The blank line is not needed, I specifically commented on the
> emphasized paragraph (by delimiting it with blank lines and leaving
> the rest for the better context for you to understand, it seems it did
> the opposite...).
>
> ...
>
Yep, didn't get you properly... Sorry for that...
--
Thank you,
Dmitry
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3 2/3] iio: add MEMSensing MSA311 3-axis accelerometer driver
2022-06-17 14:22 ` Dmitry Rokosov
@ 2022-06-17 16:54 ` Andy Shevchenko
0 siblings, 0 replies; 16+ messages in thread
From: Andy Shevchenko @ 2022-06-17 16:54 UTC (permalink / raw)
To: Dmitry Rokosov
Cc: robh+dt, stano.jakubek, shawnguo, jic23, lars, stephan,
linux-iio, devicetree, kernel, linux-kernel
On Fri, Jun 17, 2022 at 4:22 PM Dmitry Rokosov <DDRokosov@sberdevices.ru> wrote:
>
> On Thu, Jun 16, 2022 at 08:38:46PM +0200, Andy Shevchenko wrote:
> > On Thu, Jun 16, 2022 at 7:02 PM Dmitry Rokosov <DDRokosov@sberdevices.ru> wrote:
> > > On Thu, Jun 16, 2022 at 02:18:52PM +0200, Andy Shevchenko wrote:
> > > > On Thu, Jun 16, 2022 at 12:42 PM Dmitry Rokosov
> > > > <DDRokosov@sberdevices.ru> wrote:
...
> > > > > + wait_ms = (USEC_PER_SEC * MSEC_PER_SEC) / freq_uhz;
> > > >
> > > > This looks very odd from a physics perspective: sec * sec * sec == sec ?!
> > > >
> > > > Perhaps you meant some HZ* macros from units.h?
> > >
> > > I suppose because of UHZ calculation I have to use NANO instead of
> > > USEC_PER_SEC in the following line:
> > >
> > > freq_uhz = msa311_odr_table[odr].val * USEC_PER_SEC +
> > > msa311_odr_table[odr].val2;
> > >
> > > But below line is right from physics perspective. 1sec = 1/Hz, so
> > > msec = (USEC_PER_SEC / freq_uhz) * MSEC_PER_SEC:
I believe the first one should be HZ_PER_MHZ, then it will be fine.
> > > wait_ms = (USEC_PER_SEC * MSEC_PER_SEC) / freq_uhz;
> > >
> > > Or do you mean that I should change MSEC_PER_SEC to just MILLI?
> >
> > 1 / Hz = 1 sec. That's how physics defines it. Try to figure out what
> > you meant by above multiplications / divisions and come up with the
> > best that fits your purposes.
>
> From my point of view, I've already implemented the best way to calculate
> how much time I need to wait for the next data chunk based on ODR Hz
> value :-)
>
> ODR value from the table has val integer part and val2 in microHz.
> By this line we calculate microHz conversion to take into account val2
> part:
>
> freq_uhz = msa311_odr_table[odr].val * USEC_PER_SEC +
> msa311_odr_table[odr].val2;
>
> By the next line we try to calculate miliseconds for msleep() from ODR
> microHz value:
>
> wait_ms = (USEC_PER_SEC * MSEC_PER_SEC) / freq_uhz;
>
> (USEC_PER_SEC / freq_uhz) => seconds
> seconds * MSEC_PER_SEC => milliseconds>
> USEC_PER_SEC and MSEC_PER_SEC are just coefficients, they are not
> measured in "seconds" units.
Nope, it's a mistake. Those multipliers imply the unit. The rest are
the numbers. See above how to fix this (as far as I can tell).
...
> > > > > + if (err) {
> > > > > + dev_err(dev, "cannot update freq (%d)\n", err);
> > > > > + goto failed;
> > > > > + }
> > > >
> > > > Why is this inside the loop and more important under lock? Also you
> > > > may cover the initial error code by this message when moving it out of
> > > > the loop and lock.
> > > >
> > > > Ditto for other code snippets in other function(s) where applicable.
> > >
> > > Yes, I can move dev_err() outside of loop. But all ODR search loop
> > > should be under lock fully, because other msa311 operations should not
> > > be executed when we search proper ODR place.
> >
> > I didn't suggest getting rid of the lock.
> Sorry, I didn't get you... But I fully agree with you about dev_err()
> movement.
Yes, that's what I'm talking about. The dev_err() should be outside of
critical section, for example:
mutex_unlock();
if (ret) {
dev_err(...);
return ret;
}
...
return 0;
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3 3/3] dt-bindings: iio: accel: add dt-binding schema for msa311 accel driver
2022-06-16 10:42 ` [PATCH v3 3/3] dt-bindings: iio: accel: add dt-binding schema for msa311 accel driver Dmitry Rokosov
@ 2022-06-19 11:40 ` Jonathan Cameron
0 siblings, 0 replies; 16+ messages in thread
From: Jonathan Cameron @ 2022-06-19 11:40 UTC (permalink / raw)
To: Dmitry Rokosov
Cc: robh+dt, stano.jakubek, shawnguo, lars, andy.shevchenko, stephan,
linux-iio, devicetree, kernel, linux-kernel
On Thu, 16 Jun 2022 10:42:17 +0000
Dmitry Rokosov <DDRokosov@sberdevices.ru> wrote:
> Introduce devicetree binding json-schema for MSA311 tri-axial,
> low-g accelerometer driver.
>
> Signed-off-by: Dmitry Rokosov <ddrokosov@sberdevices.ru>
Hi Dmitry,
A few trivial suggestions to drop description entries that don't
useful information.
One thing we often end up adding very soon after new bindings are
introduced is power supplies. If sensible to do so, it's better
to introduce them at the start and save on the noise.
Looks like this one just needs a property entry of
vdd-supply: true
Obviously you then need to get it in the driver and turn it on
(+ off via a devm_add_action_or_reset() call) as minimal support.
Thanks,
Jonathan
> ---
> .../bindings/iio/accel/memsensing,msa311.yaml | 52 +++++++++++++++++++
> MAINTAINERS | 1 +
> 2 files changed, 53 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/iio/accel/memsensing,msa311.yaml
>
> diff --git a/Documentation/devicetree/bindings/iio/accel/memsensing,msa311.yaml b/Documentation/devicetree/bindings/iio/accel/memsensing,msa311.yaml
> new file mode 100644
> index 000000000000..072632708d42
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/accel/memsensing,msa311.yaml
> @@ -0,0 +1,52 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +
> +%YAML 1.2
> +---
> +$id: "http://devicetree.org/schemas/iio/accel/memsensing,msa311.yaml#"
> +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
> +
> +title: MEMSensing digital 3-Axis accelerometer
> +
> +maintainers:
> + - Dmitry Rokosov <ddrokosov@sberdevices.ru>
> +
> +description: |
> + MSA311 is a tri-axial, low-g accelerometer with I2C digital output for
> + sensitivity consumer applications. It has dynamical user selectable full
> + scales range of +-2g/+-4g/+-8g/+-16g and allows acceleration measurements
> + with output data rates from 1Hz to 1000Hz.
> + Datasheet can be found at following URL
> + https://cdn-shop.adafruit.com/product-files/5309/MSA311-V1.1-ENG.pdf
> +
> +properties:
> + compatible:
> + const: memsensing,msa311
> +
> + reg:
> + maxItems: 1
> + description: I2C registers address
No need for description. It always means that for i2c devices.
> +
> + interrupts:
> + maxItems: 1
> + description: optional I2C int pin can be freely mapped to specific func
Why I2C int? Is there anything associating it with i2c specifically?
I'm not sure the description adds anything useful for this so I'd
drop it.
> +
> +required:
> + - compatible
> + - reg
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + #include <dt-bindings/interrupt-controller/irq.h>
> + i2c {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + accelerometer@62 {
> + compatible = "memsensing,msa311";
> + reg = <0x62>;
> + interrupt-parent = <&gpio_intc>;
> + interrupts = <29 IRQ_TYPE_EDGE_RISING>;
> + };
> + };
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 55aeb25c004c..be39e5c214fe 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -12482,6 +12482,7 @@ MEMSENSING MICROSYSTEMS MSA311 ACCELEROMETER DRIVER
> M: Dmitry Rokosov <ddrokosov@sberdevices.ru>
> L: linux-iio@vger.kernel.org
> S: Maintained
> +F: Documentation/devicetree/bindings/iio/accel/memsensing,msa311.yaml
> F: drivers/iio/accel/msa311.c
>
> MEN A21 WATCHDOG DRIVER
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3 2/3] iio: add MEMSensing MSA311 3-axis accelerometer driver
2022-06-16 10:42 ` [PATCH v3 2/3] iio: add MEMSensing MSA311 3-axis accelerometer driver Dmitry Rokosov
2022-06-16 12:18 ` Andy Shevchenko
@ 2022-06-19 12:27 ` Jonathan Cameron
2022-07-01 13:49 ` Dmitry Rokosov
1 sibling, 1 reply; 16+ messages in thread
From: Jonathan Cameron @ 2022-06-19 12:27 UTC (permalink / raw)
To: Dmitry Rokosov
Cc: robh+dt, stano.jakubek, shawnguo, lars, andy.shevchenko, stephan,
linux-iio, devicetree, kernel, linux-kernel
On Thu, 16 Jun 2022 10:42:14 +0000
Dmitry Rokosov <DDRokosov@sberdevices.ru> wrote:
> MSA311 is a tri-axial, low-g accelerometer with I2C digital output for
> sensitivity consumer applications. It has dynamical user selectable full
> scales range of +-2g/+-4g/+-8g/+-16g and allows acceleration measurements
> with output data rates from 1Hz to 1000Hz.
>
> Datasheet can be found at following URL:
> https://cdn-shop.adafruit.com/product-files/5309/MSA311-V1.1-ENG.pdf
>
> This driver supports following MSA311 features:
> - IIO interface
> - Different power modes: NORMAL and SUSPEND (using pm_runtime)
> - ODR (Output Data Rate) selection
> - Scale and samp_freq selection
> - IIO triggered buffer, IIO reg access
> - NEW_DATA interrupt + trigger
>
> Below features to be done:
> - Motion Events: ACTIVE, TAP, ORIENT, FREEFALL
> - Low Power mode
>
> Signed-off-by: Dmitry Rokosov <ddrokosov@sberdevices.ru>
Hi Dmitry,
A few things I missed before + I'm still not happy with the runtime
pm handling. One case that isn't covered well is !CONFIG_RUNTIME_PM
Thanks,
Jonathan
> ---
> MAINTAINERS | 6 +
> drivers/iio/accel/Kconfig | 13 +
> drivers/iio/accel/Makefile | 2 +
> drivers/iio/accel/msa311.c | 1312 ++++++++++++++++++++++++++++++++++++
> 4 files changed, 1333 insertions(+)
> create mode 100644 drivers/iio/accel/msa311.c
>
>
> diff --git a/drivers/iio/accel/msa311.c b/drivers/iio/accel/msa311.c
> new file mode 100644
> index 000000000000..f8a8ed064f21
> --- /dev/null
> +++ b/drivers/iio/accel/msa311.c
> @@ -0,0 +1,1312 @@
> +
> +#define MSA311_ACCEL_CHANNEL(axis) { \
> + .type = IIO_ACCEL, \
> + .modified = 1, \
> + .channel2 = IIO_MOD_##axis, \
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
> + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SCALE) | \
Scale is very rarely shared by all, because technically that means it also applies
to the timestamp channel. Should be shared_by_type
> + BIT(IIO_CHAN_INFO_SAMP_FREQ), \
> + .info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_SCALE) | \
> + BIT(IIO_CHAN_INFO_SAMP_FREQ), \
> + .scan_index = MSA311_SI_##axis, \
> + .scan_type = { \
> + .sign = 's', \
> + .realbits = 12, \
> + .storagebits = 16, \
> + .shift = 4, \
> + .endianness = IIO_LE, \
> + }, \
> + .datasheet_name = "ACC_"#axis \
> +}
> +
> +static irqreturn_t msa311_buffer_thread(int irq, void *p)
> +{
> + struct iio_poll_func *pf = p;
> + struct iio_dev *indio_dev = pf->indio_dev;
> + struct msa311_priv *msa311 = iio_priv(indio_dev);
> + struct device *dev = &msa311->i2c->dev;
> + const struct iio_chan_spec *chan;
> + __le16 axis;
> + int bit = 0, err, i = 0;
> +
> + /* Ensure correct alignment of time stamp when present */
> + struct {
> + __le16 channels[MSA311_SI_Z + 1];
> + s64 ts __aligned(8);
> + } buf;
> +
> + memset(&buf, 0, sizeof(buf));
> +
> + mutex_lock(&msa311->lock);
> +
> + for_each_set_bit(bit, indio_dev->active_scan_mask,
> + indio_dev->masklength) {
> + chan = &msa311_channels[bit];
Nothing to do with your driver, but feels like it's worth
exploring a
for_each_chan_in_iio_scan(struct iio_chan_spec, struct iio_dev) macro.
I'll add that to my todo list.
> +
> + err = msa311_get_axis(msa311, chan, &axis);
> + if (err) {
> + mutex_unlock(&msa311->lock);
> + dev_err(dev, "cannot get axis %s (%d)\n",
> + chan->datasheet_name, err);
> + goto err;
> + }
> +
> + buf.channels[i++] = axis;
> + }
> +
> + mutex_unlock(&msa311->lock);
> +
> + iio_push_to_buffers_with_timestamp(indio_dev, &buf,
> + iio_get_time_ns(indio_dev));
> +
> +err:
> + iio_trigger_notify_done(indio_dev->trig);
> +
> + return IRQ_HANDLED;
> +}
> +static void msa311_powerdown(void *dev)
> +{
> + /* Resume device if any */
> + pm_runtime_get_sync(dev);
Hmm. I'm never particularly keen on unusual ways of using runtime pm,
so I wonder if we can avoid this turn it on to turn it off now cycle.
See below.
> +
> + /* Suspend device right now */
> + pm_runtime_put_sync_suspend(dev);
I'm still unconvinced this dance is necessary - if we assume runtime_pm
is enabled.
__pm_runtime_use_autosuspend(dev, false); which is called as part of
devm_pm_runtime_enable() being unwound calls update_autosuspend
https://elixir.bootlin.com/linux/latest/source/drivers/base/power/runtime.c#L1595
after setting power.use_autosuspend to false.
Thus it takes the else branch and call rpm_idle() which I think should be
able to suspend the device just as the above does.
Note the more complex sequence below still applies, because runtime
pm is in general optional.
> +}
> +
> +static int msa311_probe(struct i2c_client *i2c)
> +{
> + struct msa311_priv *msa311;
> + struct iio_dev *indio_dev;
> + struct device *dev = &i2c->dev;
> + int err;
> +
> + indio_dev = devm_iio_device_alloc(dev, sizeof(*msa311));
> + if (!indio_dev)
> + return dev_err_probe(dev, -ENOMEM,
> + "iio device allocation failed\n");
> +
> + msa311 = iio_priv(indio_dev);
> + msa311->i2c = i2c;
> + i2c_set_clientdata(i2c, indio_dev);
> +
> + err = msa311_regmap_init(msa311);
> + if (err)
> + return err;
> +
> + mutex_init(&msa311->lock);
> +
When this unwind we will disable autosuspend etc, but leave the device
in whatever state it happens to be in at that stage (if I understand
this handling correctly). That might seem like a bad thing, but if
we register a devm_add_action_or_reset() callback before this which
disables the device independently of anything to do with runtime PM,
then the device will
a) Be turned off as desired.
b) It'll still be turned off even if runtime pm is disabled for the system
which is nice.
Given the particular state register must be writeable and is presumably
idempotent, can we just call
err = msa311_set_pwr_mode(msa311, MSA311_PWR_MODE_SUSPEND);
Unconditionally in such a callback?
> + err = devm_pm_runtime_enable(dev);
> + if (err)
> + return err;
> +
> + /* Resume msa311 logic before any interactions with registers */
> + err = pm_runtime_resume_and_get(dev);
I missed this before, but if runtime pm is disabled, this won't do anything
so device won't be powered on.
One common(ish) way to handle this is the following sequence.
1) Power up supply regs etc and a register a devm_ callback to turn them off again.
2) Put the device into a non suspend state (not using runtime pm calls).
3) Register a callback to turn it off again (that is safe against it being
turned off via another path such as runtime pm).
4) pm_runtime_set_active() to let the runtime pm code know it is turned on.
5) devm_pm_runtime_enable()
6) autosuspend setup and enablement.
If runtime pm isn't enabled then only 1-3 happen. We waste power but the
device works.
> + if (err)
> + return err;
> +
> + pm_runtime_set_autosuspend_delay(dev, MSA311_PWR_SLEEP_DELAY_MS);
> + pm_runtime_use_autosuspend(dev);
> +
> + err = msa311_chip_init(msa311);
> + if (err)
> + return err;
> +
> + indio_dev->modes = INDIO_DIRECT_MODE; /* setup buffered mode later */
> + indio_dev->channels = msa311_channels;
> + indio_dev->num_channels = ARRAY_SIZE(msa311_channels);
> + indio_dev->name = i2c->name;
> + indio_dev->info = &msa311_info;
> +
> + err = devm_iio_triggered_buffer_setup(dev,
> + indio_dev,
> + iio_pollfunc_store_time,
> + msa311_buffer_thread,
> + &msa311_buffer_setup_ops);
> + if (err)
> + return dev_err_probe(dev, err, "cannot setup iio trig buf\n");
> +
> + if (i2c->irq > 0) {
> + err = msa311_setup_interrupts(msa311);
> + if (err)
> + return err;
> + }
> +
> + pm_runtime_mark_last_busy(dev);
> + pm_runtime_put_autosuspend(dev);
> +
> + /*
> + * Register powerdown deferred callback which suspends the chip
> + * after module unloaded.
> + *
> + * MSA311 should be in SUSPEND mode in the two cases:
> + * 1) When driver is loaded, but we do not have any data or
> + * configuration requests to it (we are solving it using
> + * autosuspend feature).
> + * 2) When driver is unloaded and device is not used (devm action is
> + * used in this case).
> + */
> + err = devm_add_action_or_reset(dev, msa311_powerdown, dev);
> + if (err)
> + return dev_err_probe(dev, err, "cannot add powerdown action\n");
> +
> + err = devm_iio_device_register(dev, indio_dev);
> + if (err)
> + return dev_err_probe(dev, err, "iio device register failed\n");
> +
> + return 0;
> +}
> +
> +static int msa311_runtime_suspend(struct device *dev)
> +{
> + struct iio_dev *indio_dev = dev_get_drvdata(dev);
> + struct msa311_priv *msa311 = iio_priv(indio_dev);
> + int err;
> +
> + dev_dbg(dev, "suspending %s\n", dev->driver->name);
> +
> + mutex_lock(&msa311->lock);
> + err = msa311_set_pwr_mode(msa311, MSA311_PWR_MODE_SUSPEND);
> + mutex_unlock(&msa311->lock);
> +
> + if (err) {
> + dev_err(dev, "failed to power off device (%d)\n", err);
> + return err;
> + }
> +
> + return 0;
> +}
> +
> +static int msa311_runtime_resume(struct device *dev)
> +{
> + struct iio_dev *indio_dev = dev_get_drvdata(dev);
> + struct msa311_priv *msa311 = iio_priv(indio_dev);
> + int err;
> +
> + dev_dbg(dev, "resuming %s\n", dev->driver->name);
> +
> + mutex_lock(&msa311->lock);
> + err = msa311_set_pwr_mode(msa311, MSA311_PWR_MODE_NORMAL);
> + mutex_unlock(&msa311->lock);
> +
> + if (err) {
> + dev_err(dev, "failed to power on device (%d)\n", err);
> + return err;
> + }
> +
> + return 0;
> +}
> +
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3 2/3] iio: add MEMSensing MSA311 3-axis accelerometer driver
2022-06-16 17:02 ` Dmitry Rokosov
2022-06-16 18:38 ` Andy Shevchenko
@ 2022-06-19 12:31 ` Jonathan Cameron
2022-07-01 13:06 ` Dmitry Rokosov
1 sibling, 1 reply; 16+ messages in thread
From: Jonathan Cameron @ 2022-06-19 12:31 UTC (permalink / raw)
To: Dmitry Rokosov
Cc: Andy Shevchenko, robh+dt, stano.jakubek, shawnguo, lars, stephan,
linux-iio, devicetree, kernel, linux-kernel
On Thu, 16 Jun 2022 17:02:08 +0000
Dmitry Rokosov <DDRokosov@sberdevices.ru> wrote:
> > > + err = -EINVAL;
> > > + mutex_lock(&msa311->lock);
> > > + for (odr = 0; odr < ARRAY_SIZE(msa311_odr_table); ++odr)
> > > + if (val == msa311_odr_table[odr].val &&
> > > + val2 == msa311_odr_table[odr].val2) {
> > > + err = msa311_set_odr(msa311, odr);
> >
> > > + if (err) {
> > > + dev_err(dev, "cannot update freq (%d)\n", err);
> > > + goto failed;
> > > + }
> >
> > Why is this inside the loop and more important under lock? Also you
> > may cover the initial error code by this message when moving it out of
> > the loop and lock.
> >
> > Ditto for other code snippets in other function(s) where applicable.
> >
>
> Yes, I can move dev_err() outside of loop. But all ODR search loop
> should be under lock fully, because other msa311 operations should not
> be executed when we search proper ODR place.
I don't see why? The search itself is for a match of the input to const data.
That can occur before taking the lock to do the actual write.
I don't see any additional race beyond the one that is always there of
a thread updating ODR whilst another is accessing the device. Which order
those events happen in is not controlled by the driver, but the output
will be consistent with one or other order of those two accesses.
Jonathan
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3 2/3] iio: add MEMSensing MSA311 3-axis accelerometer driver
2022-06-19 12:31 ` Jonathan Cameron
@ 2022-07-01 13:06 ` Dmitry Rokosov
0 siblings, 0 replies; 16+ messages in thread
From: Dmitry Rokosov @ 2022-07-01 13:06 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Andy Shevchenko, robh+dt, stano.jakubek, shawnguo, lars, stephan,
linux-iio, devicetree, kernel, linux-kernel
On Sun, Jun 19, 2022 at 01:31:42PM +0100, Jonathan Cameron wrote:
> On Thu, 16 Jun 2022 17:02:08 +0000
> Dmitry Rokosov <DDRokosov@sberdevices.ru> wrote:
>
>
>
> > > > + err = -EINVAL;
> > > > + mutex_lock(&msa311->lock);
> > > > + for (odr = 0; odr < ARRAY_SIZE(msa311_odr_table); ++odr)
> > > > + if (val == msa311_odr_table[odr].val &&
> > > > + val2 == msa311_odr_table[odr].val2) {
> > > > + err = msa311_set_odr(msa311, odr);
> > >
> > > > + if (err) {
> > > > + dev_err(dev, "cannot update freq (%d)\n", err);
> > > > + goto failed;
> > > > + }
> > >
> > > Why is this inside the loop and more important under lock? Also you
> > > may cover the initial error code by this message when moving it out of
> > > the loop and lock.
> > >
> > > Ditto for other code snippets in other function(s) where applicable.
> > >
> >
> > Yes, I can move dev_err() outside of loop. But all ODR search loop
> > should be under lock fully, because other msa311 operations should not
> > be executed when we search proper ODR place.
>
> I don't see why? The search itself is for a match of the input to const data.
> That can occur before taking the lock to do the actual write.
>
> I don't see any additional race beyond the one that is always there of
> a thread updating ODR whilst another is accessing the device. Which order
> those events happen in is not controlled by the driver, but the output
> will be consistent with one or other order of those two accesses.
>
> Jonathan
Agreed, will be changed in the v4.
--
Thank you,
Dmitry
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3 2/3] iio: add MEMSensing MSA311 3-axis accelerometer driver
2022-06-19 12:27 ` Jonathan Cameron
@ 2022-07-01 13:49 ` Dmitry Rokosov
2022-07-16 15:51 ` Jonathan Cameron
0 siblings, 1 reply; 16+ messages in thread
From: Dmitry Rokosov @ 2022-07-01 13:49 UTC (permalink / raw)
To: Jonathan Cameron
Cc: robh+dt, stano.jakubek, shawnguo, lars, andy.shevchenko, stephan,
linux-iio, devicetree, kernel, linux-kernel
Hello Jonathan,
Sorry for the delayed response.
On Sun, Jun 19, 2022 at 01:27:03PM +0100, Jonathan Cameron wrote:
> On Thu, 16 Jun 2022 10:42:14 +0000
> Dmitry Rokosov <DDRokosov@sberdevices.ru> wrote:
>
> > MSA311 is a tri-axial, low-g accelerometer with I2C digital output for
> > sensitivity consumer applications. It has dynamical user selectable full
> > scales range of +-2g/+-4g/+-8g/+-16g and allows acceleration measurements
> > with output data rates from 1Hz to 1000Hz.
> >
> > Datasheet can be found at following URL:
> > https://cdn-shop.adafruit.com/product-files/5309/MSA311-V1.1-ENG.pdf
> >
> > This driver supports following MSA311 features:
> > - IIO interface
> > - Different power modes: NORMAL and SUSPEND (using pm_runtime)
> > - ODR (Output Data Rate) selection
> > - Scale and samp_freq selection
> > - IIO triggered buffer, IIO reg access
> > - NEW_DATA interrupt + trigger
> >
> > Below features to be done:
> > - Motion Events: ACTIVE, TAP, ORIENT, FREEFALL
> > - Low Power mode
> >
> > Signed-off-by: Dmitry Rokosov <ddrokosov@sberdevices.ru>
> Hi Dmitry,
>
> A few things I missed before + I'm still not happy with the runtime
> pm handling. One case that isn't covered well is !CONFIG_RUNTIME_PM
>
> Thanks,
>
> Jonathan
>
...
> > +static irqreturn_t msa311_buffer_thread(int irq, void *p)
> > +{
> > + struct iio_poll_func *pf = p;
> > + struct iio_dev *indio_dev = pf->indio_dev;
> > + struct msa311_priv *msa311 = iio_priv(indio_dev);
> > + struct device *dev = &msa311->i2c->dev;
> > + const struct iio_chan_spec *chan;
> > + __le16 axis;
> > + int bit = 0, err, i = 0;
> > +
> > + /* Ensure correct alignment of time stamp when present */
> > + struct {
> > + __le16 channels[MSA311_SI_Z + 1];
> > + s64 ts __aligned(8);
> > + } buf;
> > +
> > + memset(&buf, 0, sizeof(buf));
> > +
> > + mutex_lock(&msa311->lock);
> > +
> > + for_each_set_bit(bit, indio_dev->active_scan_mask,
> > + indio_dev->masklength) {
> > + chan = &msa311_channels[bit];
>
> Nothing to do with your driver, but feels like it's worth
> exploring a
> for_each_chan_in_iio_scan(struct iio_chan_spec, struct iio_dev) macro.
>
> I'll add that to my todo list.
>
If you don't mind, I can prepare such a patch.
...
> When this unwind we will disable autosuspend etc, but leave the device
> in whatever state it happens to be in at that stage (if I understand
> this handling correctly). That might seem like a bad thing, but if
> we register a devm_add_action_or_reset() callback before this which
> disables the device independently of anything to do with runtime PM,
> then the device will
> a) Be turned off as desired.
> b) It'll still be turned off even if runtime pm is disabled for the system
> which is nice.
>
> Given the particular state register must be writeable and is presumably
> idempotent, can we just call
> err = msa311_set_pwr_mode(msa311, MSA311_PWR_MODE_SUSPEND);
> Unconditionally in such a callback?
I think it's a good idea. I didn't think about the configs when runtime pm
is disabled. So looks like we need to make sure that device is workable
from a pm perspective, and it is achievable only using a direct
msa311_set_pwr_mode() call as you suggested below.
> > + err = devm_pm_runtime_enable(dev);
> > + if (err)
> > + return err;
> > +
> > + /* Resume msa311 logic before any interactions with registers */
> > + err = pm_runtime_resume_and_get(dev);
> I missed this before, but if runtime pm is disabled, this won't do anything
> so device won't be powered on.
>
> One common(ish) way to handle this is the following sequence.
>
> 1) Power up supply regs etc and a register a devm_ callback to turn them off again.
> 2) Put the device into a non suspend state (not using runtime pm calls).
> 3) Register a callback to turn it off again (that is safe against it being
> turned off via another path such as runtime pm).
> 4) pm_runtime_set_active() to let the runtime pm code know it is turned on.
> 5) devm_pm_runtime_enable()
> 6) autosuspend setup and enablement.
>
> If runtime pm isn't enabled then only 1-3 happen. We waste power but the
> device works.
--
Thank you,
Dmitry
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3 2/3] iio: add MEMSensing MSA311 3-axis accelerometer driver
2022-07-01 13:49 ` Dmitry Rokosov
@ 2022-07-16 15:51 ` Jonathan Cameron
2022-07-18 10:01 ` Dmitry Rokosov
0 siblings, 1 reply; 16+ messages in thread
From: Jonathan Cameron @ 2022-07-16 15:51 UTC (permalink / raw)
To: Dmitry Rokosov
Cc: robh+dt, stano.jakubek, shawnguo, lars, andy.shevchenko, stephan,
linux-iio, devicetree, kernel, linux-kernel
On Fri, 1 Jul 2022 13:49:10 +0000
Dmitry Rokosov <DDRokosov@sberdevices.ru> wrote:
> Hello Jonathan,
>
> Sorry for the delayed response.
>
> On Sun, Jun 19, 2022 at 01:27:03PM +0100, Jonathan Cameron wrote:
> > On Thu, 16 Jun 2022 10:42:14 +0000
> > Dmitry Rokosov <DDRokosov@sberdevices.ru> wrote:
> >
> > > MSA311 is a tri-axial, low-g accelerometer with I2C digital output for
> > > sensitivity consumer applications. It has dynamical user selectable full
> > > scales range of +-2g/+-4g/+-8g/+-16g and allows acceleration measurements
> > > with output data rates from 1Hz to 1000Hz.
> > >
> > > Datasheet can be found at following URL:
> > > https://cdn-shop.adafruit.com/product-files/5309/MSA311-V1.1-ENG.pdf
> > >
> > > This driver supports following MSA311 features:
> > > - IIO interface
> > > - Different power modes: NORMAL and SUSPEND (using pm_runtime)
> > > - ODR (Output Data Rate) selection
> > > - Scale and samp_freq selection
> > > - IIO triggered buffer, IIO reg access
> > > - NEW_DATA interrupt + trigger
> > >
> > > Below features to be done:
> > > - Motion Events: ACTIVE, TAP, ORIENT, FREEFALL
> > > - Low Power mode
> > >
> > > Signed-off-by: Dmitry Rokosov <ddrokosov@sberdevices.ru>
> > Hi Dmitry,
> >
> > A few things I missed before + I'm still not happy with the runtime
> > pm handling. One case that isn't covered well is !CONFIG_RUNTIME_PM
> >
> > Thanks,
> >
> > Jonathan
> >
>
> ...
>
> > > +static irqreturn_t msa311_buffer_thread(int irq, void *p)
> > > +{
> > > + struct iio_poll_func *pf = p;
> > > + struct iio_dev *indio_dev = pf->indio_dev;
> > > + struct msa311_priv *msa311 = iio_priv(indio_dev);
> > > + struct device *dev = &msa311->i2c->dev;
> > > + const struct iio_chan_spec *chan;
> > > + __le16 axis;
> > > + int bit = 0, err, i = 0;
> > > +
> > > + /* Ensure correct alignment of time stamp when present */
> > > + struct {
> > > + __le16 channels[MSA311_SI_Z + 1];
> > > + s64 ts __aligned(8);
> > > + } buf;
> > > +
> > > + memset(&buf, 0, sizeof(buf));
> > > +
> > > + mutex_lock(&msa311->lock);
> > > +
> > > + for_each_set_bit(bit, indio_dev->active_scan_mask,
> > > + indio_dev->masklength) {
> > > + chan = &msa311_channels[bit];
> >
> > Nothing to do with your driver, but feels like it's worth
> > exploring a
> > for_each_chan_in_iio_scan(struct iio_chan_spec, struct iio_dev) macro.
> >
> > I'll add that to my todo list.
> >
>
> If you don't mind, I can prepare such a patch.
I had a look at this whilst travelling and it's a lot more complex than I
thought it would be because of gaps in the scan_index in some drivers (not
all channels have scan indexes and not all scan indexes are used)
If we write such a thing we need to resolve that in the core and I suspect
it will require creation of an indirection structure that lets us
do scan_index based look up of channels. Whilst that works in many drivers
because there is a nice 1 to 1 mapping, there are exceptions.
Hence I think we would be looking at:
1) Check at registration time on whether scan_index == location in
iio_dev->channels, if so set another pointer say iio_dev->channels_linear =
iio_dev->channels.
2) If not, create a lookup table and make iio_dev->channels_linear
point to that.
3) Finally introduce a macro that uses channels_linear.
What fun ;)
Jonathan
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3 2/3] iio: add MEMSensing MSA311 3-axis accelerometer driver
2022-07-16 15:51 ` Jonathan Cameron
@ 2022-07-18 10:01 ` Dmitry Rokosov
0 siblings, 0 replies; 16+ messages in thread
From: Dmitry Rokosov @ 2022-07-18 10:01 UTC (permalink / raw)
To: Jonathan Cameron
Cc: robh+dt, stano.jakubek, shawnguo, lars, andy.shevchenko, stephan,
linux-iio, devicetree, kernel, linux-kernel
Jonathan,
On Sat, Jul 16, 2022 at 04:51:21PM +0100, Jonathan Cameron wrote:
> On Fri, 1 Jul 2022 13:49:10 +0000
> Dmitry Rokosov <DDRokosov@sberdevices.ru> wrote:
>
> > Hello Jonathan,
> >
> > Sorry for the delayed response.
> >
> > On Sun, Jun 19, 2022 at 01:27:03PM +0100, Jonathan Cameron wrote:
> > > On Thu, 16 Jun 2022 10:42:14 +0000
> > > Dmitry Rokosov <DDRokosov@sberdevices.ru> wrote:
> > >
> > > > MSA311 is a tri-axial, low-g accelerometer with I2C digital output for
> > > > sensitivity consumer applications. It has dynamical user selectable full
> > > > scales range of +-2g/+-4g/+-8g/+-16g and allows acceleration measurements
> > > > with output data rates from 1Hz to 1000Hz.
> > > >
> > > > Datasheet can be found at following URL:
> > > > https://cdn-shop.adafruit.com/product-files/5309/MSA311-V1.1-ENG.pdf
> > > >
> > > > This driver supports following MSA311 features:
> > > > - IIO interface
> > > > - Different power modes: NORMAL and SUSPEND (using pm_runtime)
> > > > - ODR (Output Data Rate) selection
> > > > - Scale and samp_freq selection
> > > > - IIO triggered buffer, IIO reg access
> > > > - NEW_DATA interrupt + trigger
> > > >
> > > > Below features to be done:
> > > > - Motion Events: ACTIVE, TAP, ORIENT, FREEFALL
> > > > - Low Power mode
> > > >
> > > > Signed-off-by: Dmitry Rokosov <ddrokosov@sberdevices.ru>
> > > Hi Dmitry,
> > >
> > > A few things I missed before + I'm still not happy with the runtime
> > > pm handling. One case that isn't covered well is !CONFIG_RUNTIME_PM
> > >
> > > Thanks,
> > >
> > > Jonathan
> > >
> >
> > ...
> >
> > > > +static irqreturn_t msa311_buffer_thread(int irq, void *p)
> > > > +{
> > > > + struct iio_poll_func *pf = p;
> > > > + struct iio_dev *indio_dev = pf->indio_dev;
> > > > + struct msa311_priv *msa311 = iio_priv(indio_dev);
> > > > + struct device *dev = &msa311->i2c->dev;
> > > > + const struct iio_chan_spec *chan;
> > > > + __le16 axis;
> > > > + int bit = 0, err, i = 0;
> > > > +
> > > > + /* Ensure correct alignment of time stamp when present */
> > > > + struct {
> > > > + __le16 channels[MSA311_SI_Z + 1];
> > > > + s64 ts __aligned(8);
> > > > + } buf;
> > > > +
> > > > + memset(&buf, 0, sizeof(buf));
> > > > +
> > > > + mutex_lock(&msa311->lock);
> > > > +
> > > > + for_each_set_bit(bit, indio_dev->active_scan_mask,
> > > > + indio_dev->masklength) {
> > > > + chan = &msa311_channels[bit];
> > >
> > > Nothing to do with your driver, but feels like it's worth
> > > exploring a
> > > for_each_chan_in_iio_scan(struct iio_chan_spec, struct iio_dev) macro.
> > >
> > > I'll add that to my todo list.
> > >
> >
> > If you don't mind, I can prepare such a patch.
>
> I had a look at this whilst travelling and it's a lot more complex than I
> thought it would be because of gaps in the scan_index in some drivers (not
> all channels have scan indexes and not all scan indexes are used)
>
> If we write such a thing we need to resolve that in the core and I suspect
> it will require creation of an indirection structure that lets us
> do scan_index based look up of channels. Whilst that works in many drivers
> because there is a nice 1 to 1 mapping, there are exceptions.
> Hence I think we would be looking at:
>
> 1) Check at registration time on whether scan_index == location in
> iio_dev->channels, if so set another pointer say iio_dev->channels_linear =
> iio_dev->channels.
> 2) If not, create a lookup table and make iio_dev->channels_linear
> point to that.
> 3) Finally introduce a macro that uses channels_linear.
>
> What fun ;)
>
> Jonathan
>
Okay, I'll try looking for proper solution and prepare RFC patch.
--
Thank you,
Dmitry
^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2022-07-18 10:02 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-06-16 10:42 [PATCH v3 0/3] iio: accel: add MSA311 accelerometer driver Dmitry Rokosov
2022-06-16 10:42 ` [PATCH v3 1/3] dt-bindings: vendor-prefixes: add MEMSensing Microsystems Co., Ltd Dmitry Rokosov
2022-06-16 10:42 ` [PATCH v3 2/3] iio: add MEMSensing MSA311 3-axis accelerometer driver Dmitry Rokosov
2022-06-16 12:18 ` Andy Shevchenko
2022-06-16 17:02 ` Dmitry Rokosov
2022-06-16 18:38 ` Andy Shevchenko
2022-06-17 14:22 ` Dmitry Rokosov
2022-06-17 16:54 ` Andy Shevchenko
2022-06-19 12:31 ` Jonathan Cameron
2022-07-01 13:06 ` Dmitry Rokosov
2022-06-19 12:27 ` Jonathan Cameron
2022-07-01 13:49 ` Dmitry Rokosov
2022-07-16 15:51 ` Jonathan Cameron
2022-07-18 10:01 ` Dmitry Rokosov
2022-06-16 10:42 ` [PATCH v3 3/3] dt-bindings: iio: accel: add dt-binding schema for msa311 accel driver Dmitry Rokosov
2022-06-19 11:40 ` Jonathan Cameron
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).