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
Signed-off-by: Dmitry Rokosov <[email protected]>
---
MAINTAINERS | 6 +
drivers/iio/accel/Kconfig | 13 +
drivers/iio/accel/Makefile | 2 +
drivers/iio/accel/msa311.c | 1525 ++++++++++++++++++++++++++++++++++++
4 files changed, 1546 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 <[email protected]>
+L: [email protected]
+S: Maintained
+F: drivers/iio/accel/msa311.c
+
MEN A21 WATCHDOG DRIVER
M: Johannes Thumshirn <[email protected]>
L: [email protected]
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..fe2cff1ed5ef
--- /dev/null
+++ b/drivers/iio/accel/msa311.c
@@ -0,0 +1,1525 @@
+// 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
+ *
+ * Copyright (c) 2022, SberDevices. All Rights Reserved.
+ *
+ * Author: Dmitry Rokosov <[email protected]>
+ */
+
+#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 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,
+};
+
+/**
+ * MSA311_GENMASK() - MSA311 reg_field mask generator
+ * @field: requested regfield from msa311_reg_fields table
+ *
+ * Return: This helper returns reg_field mask to be applied.
+ */
+#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,
+};
+
+/**
+ * MSA311_ACCEL_CHANNEL() - Construct MSA311 accelerometer channel descriptor
+ * @axis: axis name in uppercase
+ */
+#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 \
+}
+
+/**
+ * MSA311_TIMESTAMP_CHANNEL() - Construct MSA311 timestamp channel descriptor
+ */
+#define MSA311_TIMESTAMP_CHANNEL() IIO_CHAN_SOFT_TIMESTAMP(MSA311_SI_TIMESTAMP)
+
+static const struct iio_chan_spec msa311_channels[] = {
+ MSA311_ACCEL_CHANNEL(X),
+ MSA311_ACCEL_CHANNEL(Y),
+ MSA311_ACCEL_CHANNEL(Z),
+ MSA311_TIMESTAMP_CHANNEL(),
+};
+
+/**
+ * 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) {
+ dev_err(&msa311->i2c->dev, "failed to read odr value (%d)\n",
+ 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 err;
+}
+
+/**
+ * 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)
+{
+ 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) {
+ dev_err(&msa311->i2c->dev, "failed to read pwr_mode (%d)\n",
+ 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) {
+ return dev_err_probe(&msa311->i2c->dev, -EINVAL,
+ "failed to set odr %u.%uHz, not available in %s mode\n",
+ msa311_odr_table[odr].val,
+ msa311_odr_table[odr].val2 / 1000, mode);
+ }
+
+ err = regmap_field_write(msa311->fields[F_ODR], odr);
+ if (err)
+ return dev_err_probe(&msa311->i2c->dev, err,
+ "failed to set odr value (%d)\n", err);
+
+ return 0;
+}
+
+/**
+ * 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;
+ unsigned int odr;
+ unsigned long wait_ms;
+ unsigned long freq_uhz;
+ int err;
+
+ err = msa311_get_odr(msa311, &odr);
+ if (err)
+ return dev_err_probe(&msa311->i2c->dev, err,
+ "cannot get actual freq (%d)\n", 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)
+{
+ unsigned int prev_mode;
+ int err;
+
+ dev_dbg(&msa311->i2c->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 dev_err_probe(&msa311->i2c->dev, err,
+ "cannot read prev pwr mode (%d)\n", err);
+
+ err = regmap_field_write(msa311->fields[F_PWR_MODE], mode);
+ if (err)
+ return dev_err_probe(&msa311->i2c->dev, err,
+ "bad pwr mode transition (%d)\n", 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 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)
+{
+ unsigned int axis_reg;
+ int err;
+
+ if (chan->scan_index < MSA311_SI_X || chan->scan_index > MSA311_SI_Z) {
+ dev_err(&msa311->i2c->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);
+ err = regmap_bulk_read(msa311->regs, axis_reg, axis, sizeof(*axis));
+ if (err) {
+ dev_err(&msa311->i2c->dev,
+ "failed to read value of axis %s (%d)\n",
+ chan->datasheet_name, err);
+ return err;
+ }
+
+ return 0;
+}
+
+static int msa311_read_raw_data(struct msa311_priv *msa311,
+ struct iio_chan_spec const *chan,
+ int *val, int *val2)
+{
+ __le16 axis;
+ int err;
+
+ err = pm_runtime_resume_and_get(&msa311->i2c->dev);
+ if (err) {
+ dev_err(&msa311->i2c->dev, "failed powerup (%d)\n", err);
+ return err;
+ }
+
+ mutex_lock(&msa311->lock);
+ err = msa311_get_axis(msa311, chan, &axis);
+ if (err) {
+ dev_err(&msa311->i2c->dev,
+ "cannot get axis %s (%d)\n",
+ chan->datasheet_name, err);
+ mutex_unlock(&msa311->lock);
+ return err;
+ }
+ mutex_unlock(&msa311->lock);
+
+ pm_runtime_mark_last_busy(&msa311->i2c->dev);
+ pm_runtime_put_autosuspend(&msa311->i2c->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);
+ *val2 = 0;
+
+ return IIO_VAL_INT;
+}
+
+static int msa311_read_scale(struct msa311_priv *msa311, int *val, int *val2)
+{
+ unsigned int fs;
+ int err;
+
+ mutex_lock(&msa311->lock);
+ err = regmap_field_read(msa311->fields[F_FS], &fs);
+ if (err) {
+ dev_err(&msa311->i2c->dev,
+ "cannot get actual scale (%d)\n", err);
+ mutex_unlock(&msa311->lock);
+ return err;
+ }
+ mutex_unlock(&msa311->lock);
+
+ *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 msa311_priv *msa311,
+ int *val, int *val2)
+{
+ unsigned int odr;
+ int err;
+
+ mutex_lock(&msa311->lock);
+ err = msa311_get_odr(msa311, &odr);
+ if (err) {
+ dev_err(&msa311->i2c->dev,
+ "cannot get actual freq (%d)\n", err);
+ mutex_unlock(&msa311->lock);
+ return err;
+ }
+ mutex_unlock(&msa311->lock);
+
+ *val = msa311_odr_table[odr].val;
+ *val2 = msa311_odr_table[odr].val2;
+
+ return IIO_VAL_INT_PLUS_MICRO;
+}
+
+/**
+ * msa311_read_raw() - IIO interface function to request attr/accel data
+ * @indio_dev: The IIO device associated with MSA311
+ * @chan: IIO channel specification
+ * @val: Output data value, first part
+ * @val2: Output data value, second part
+ * @mask: Value type selector
+ *
+ * Return: IIO_VAL_* type on success,
+ * -EINVAL for unknown value type (bad mask),
+ * -EBUSY if IIO buffer enabled,
+ * -ERRNO in other failures
+ */
+static int msa311_read_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ int *val, int *val2, long mask)
+{
+ struct msa311_priv *msa311 = iio_priv(indio_dev);
+
+ switch (mask) {
+ case IIO_CHAN_INFO_RAW:
+ if (iio_buffer_enabled(indio_dev))
+ return -EBUSY;
+
+ return msa311_read_raw_data(msa311, chan, val, val2);
+
+ case IIO_CHAN_INFO_SCALE:
+ return msa311_read_scale(msa311, val, val2);
+
+ case IIO_CHAN_INFO_SAMP_FREQ:
+ return msa311_read_samp_freq(msa311, val, val2);
+
+ default:
+ return -EINVAL;
+ }
+}
+
+/**
+ * msa311_read_avail() - IIO interface to request available device attr values
+ * @indio_dev: The IIO device associated with MSA311
+ * @chan: IIO channel specification
+ * @vals: Available device attr values
+ * @type: The type of the vals
+ * @length: Attr values array size
+ * @mask: Attr value type selector
+ *
+ * Return: IIO_AVAIL_LIST specificator on success,
+ * -EINVAL for unknown attr value type (bad mask),
+ */
+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 msa311_priv *msa311, int val, int val2)
+{
+ unsigned int fs;
+ int err;
+
+ /* Skip such values */
+ if (val != 0)
+ return 0;
+
+ err = pm_runtime_resume_and_get(&msa311->i2c->dev);
+ if (err) {
+ dev_err(&msa311->i2c->dev, "failed powerup (%d)\n", 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(&msa311->i2c->dev,
+ "cannot update scale (%d)\n", err);
+ goto failed;
+ }
+
+ break;
+ }
+
+failed:
+ mutex_unlock(&msa311->lock);
+
+ pm_runtime_mark_last_busy(&msa311->i2c->dev);
+ pm_runtime_put_autosuspend(&msa311->i2c->dev);
+
+ return err;
+}
+
+static int msa311_write_samp_freq(struct msa311_priv *msa311, int val, int val2)
+{
+ unsigned int odr;
+ int err;
+
+ err = pm_runtime_resume_and_get(&msa311->i2c->dev);
+ if (err) {
+ dev_err(&msa311->i2c->dev, "failed powerup (%d)\n", 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(&msa311->i2c->dev,
+ "cannot update freq (%d)\n", err);
+ goto failed;
+ }
+
+ break;
+ }
+
+failed:
+ mutex_unlock(&msa311->lock);
+
+ pm_runtime_mark_last_busy(&msa311->i2c->dev);
+ pm_runtime_put_autosuspend(&msa311->i2c->dev);
+
+ return err;
+}
+
+/**
+ * msa311_write_raw() - IIO interface function to write attr/accel data
+ * @indio_dev: The IIO device associated with MSA311
+ * @chan: IIO channel specification
+ * @val: Input data value, first part
+ * @val2: Input data value, second part
+ * @mask: Value type selector
+ *
+ * Return: 0 on success,
+ * -EINVAL for unknown value type (bad mask),
+ * -ERRNO in other failures
+ */
+static int msa311_write_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ int val, int val2, long mask)
+{
+ struct msa311_priv *msa311 = iio_priv(indio_dev);
+
+ switch (mask) {
+ case IIO_CHAN_INFO_SCALE:
+ if (iio_buffer_enabled(indio_dev))
+ return -EBUSY;
+
+ return msa311_write_scale(msa311, val, val2);
+
+ case IIO_CHAN_INFO_SAMP_FREQ:
+ if (iio_buffer_enabled(indio_dev))
+ return -EBUSY;
+
+ return msa311_write_samp_freq(msa311, val, val2);
+
+ default:
+ return -EINVAL;
+ }
+}
+
+/**
+ * msa311_debugfs_reg_access() - IIO interface function to read/write registers
+ * @indio_dev: The IIO device associated with MSA311
+ * @reg: Register offset
+ * @writeval: Input value to be written (applicable if readval == NULL)
+ * @readval: Output value to be read, should be not NULL, if it's read operation
+ *
+ * Return: 0 on success,
+ * -EINVAL for wrong register offset
+ * -ERRNO in other failures
+ */
+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);
+ int err;
+
+ if (reg > regmap_get_max_register(msa311->regs))
+ return -EINVAL;
+
+ err = pm_runtime_resume_and_get(&msa311->i2c->dev);
+ if (err) {
+ dev_err(&msa311->i2c->dev, "failed powerup (%d)\n", 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(&msa311->i2c->dev);
+ pm_runtime_put_autosuspend(&msa311->i2c->dev);
+
+ if (err)
+ dev_err(&msa311->i2c->dev,
+ "cannot %s register %u from debugfs (%d)\n",
+ (!readval) ? "write" : "read", reg, err);
+
+ return err;
+}
+
+/**
+ * msa311_buffer_preenable() - IIO buffer interface preenable actions
+ * @indio_dev: The IIO device associated with MSA311
+ *
+ * Return: 0 on success,
+ * -ERRNO in other failures
+ */
+static int msa311_buffer_preenable(struct iio_dev *indio_dev)
+{
+ struct msa311_priv *msa311 = iio_priv(indio_dev);
+ int err;
+
+ err = pm_runtime_resume_and_get(&msa311->i2c->dev);
+ if (err)
+ dev_err(&msa311->i2c->dev, "failed powerup (%d)\n", err);
+
+ return err;
+}
+
+/**
+ * msa311_buffer_postdisable() - IIO buffer interface postdisable actions
+ * @indio_dev: The IIO device associated with MSA311
+ *
+ * Return: 0 on success,
+ * -ERRNO in other failures
+ */
+static int msa311_buffer_postdisable(struct iio_dev *indio_dev)
+{
+ struct msa311_priv *msa311 = iio_priv(indio_dev);
+
+ pm_runtime_mark_last_busy(&msa311->i2c->dev);
+ pm_runtime_put_autosuspend(&msa311->i2c->dev);
+
+ return 0;
+}
+
+/**
+ * msa311_set_new_data_trig_state() - IIO trigger interface to change trig state
+ * @trig: The IIO device trigger for new data event
+ * @state: New state (enabled or disabled)
+ *
+ * This function changes NEW_DATA interrupt driver trigger state to enabled or
+ * disabled.
+ *
+ * Return: 0 on success,
+ * -ERRNO in other failures
+ */
+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);
+ 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(&msa311->i2c->dev,
+ "cannot %s buffer due to new_data_int failure (%d)\n",
+ state ? "enable" : "disable", err);
+
+ return err;
+}
+
+/**
+ * msa311_validate_device() - IIO trigger interface to validate requested device
+ * @trig: Appropriate IIO trigger
+ * @indio_dev: Requested IIO device
+ *
+ * Return: 0 on success,
+ * -EINVAL when indio_dev isn't linked with appropriate trigger
+ */
+static int msa311_validate_device(struct iio_trigger *trig,
+ struct iio_dev *indio_dev)
+{
+ struct iio_dev *indio = iio_trigger_get_drvdata(trig);
+
+ if (indio != indio_dev)
+ return -EINVAL;
+
+ return 0;
+}
+
+/**
+ * msa311_buffer_thread() - IIO buffer thread to push channels actual data
+ * @irq: The software interrupt assigned to @p
+ * @p: The IIO poll function dispatched by external trigger our device is
+ * attached to.
+ *
+ * Return: IRQ_HANDLED all the time
+ */
+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);
+ 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(&msa311->i2c->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;
+}
+
+/**
+ * msa311_irq_thread() - Interrupt bottom-half handler.
+ * @irq: Interrupt line the hardware uses to notify new data has arrived.
+ * @p: The IIO device associated with the sampling hardware.
+ *
+ * Ack all internal interrupts, check their statuses and wake up appropriate
+ * triggers or send events if needed
+ *
+ * Return: IRQ_HANDLED on success (all interrupts are handled without problems)
+ * IRQ_NONE in other failures
+ */
+static irqreturn_t msa311_irq_thread(int irq, void *p)
+{
+ struct msa311_priv *msa311 = iio_priv(p);
+ 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(&msa311->i2c->dev,
+ "cannot retrieve new_data int enabled (%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,
+};
+
+/**
+ * msa311_check_partid() - Check MSA311 WHO_AM_I identifier.
+ * @msa311: MSA311 internal private state
+ *
+ * Return: 0 on success (MSA311 device was found on the I2C bus)
+ * 0 with warning notice when MSA311 device was found on the I2C bus,
+ * but chipid has not expected value
+ * -ERRNO in other failures
+ */
+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)
+ return dev_err_probe(dev, err,
+ "failed to read partid (%d)\n", err);
+
+ dev_info(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;
+}
+
+/**
+ * msa311_chip_init() - MSA311 chip initialization sequence
+ * @msa311: MSA311 internal private state
+ *
+ * We do not need to hold msa311->lock here, because this function is used
+ * during I2C driver probing process only.
+ *
+ * Return: 0 on success, -ERRNO in other failures
+ */
+static int msa311_chip_init(struct msa311_priv *msa311)
+{
+ 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(&msa311->i2c->dev, err,
+ "cannot soft reset all logic (%d)\n", err);
+
+ err = msa311_set_pwr_mode(msa311, MSA311_PWR_MODE_NORMAL);
+ if (err)
+ return dev_err_probe(&msa311->i2c->dev, err,
+ "bad normal mode transition (%d)\n", err);
+
+ err = regmap_write(msa311->regs, MSA311_RANGE_REG, MSA311_FS_16G);
+ if (err)
+ return dev_err_probe(&msa311->i2c->dev, err,
+ "failed to setup accel range (%d)\n", err);
+
+ /* Disable all interrupts by default */
+ err = regmap_write(msa311->regs, MSA311_INT_SET_0_REG, 0);
+ if (err)
+ return dev_err_probe(&msa311->i2c->dev, err,
+ "cannot disable set0 intrs (%d)\n", err);
+
+ err = regmap_write(msa311->regs, MSA311_INT_SET_1_REG, 0);
+ if (err)
+ return dev_err_probe(&msa311->i2c->dev, err,
+ "cannot disable set1 intrs (%d)\n", err);
+
+ /* Unmap all INT1 interrupts by default */
+ err = regmap_write(msa311->regs, MSA311_INT_MAP_0_REG, 0);
+ if (err)
+ return dev_err_probe(&msa311->i2c->dev, err,
+ "failed to unmap map0 intrs (%d)\n", err);
+
+ err = regmap_write(msa311->regs, MSA311_INT_MAP_1_REG, 0);
+ if (err)
+ return dev_err_probe(&msa311->i2c->dev, err,
+ "failed to unmap map1 intrs (%d)\n", err);
+
+ /* 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(&msa311->i2c->dev, err,
+ "cannot enable all axes (%d)\n", err);
+
+ err = msa311_set_odr(msa311, MSA311_ODR_125_HZ);
+ if (err)
+ return dev_err_probe(&msa311->i2c->dev, err,
+ "failed to set accel freq (%d)\n", err);
+
+ return 0;
+}
+
+/**
+ * msa311_setup_interrupts() - MSA311 interrupts initialization sequence
+ * @msa311: MSA311 internal private state
+ *
+ * We register new data trigger firstly and then setup MSA311 int registers.
+ * This function does not need to hold msa311->lock, because it is used
+ * during I2C driver probing process only.
+ *
+ * Return: 0 on success, -ERRNO in other failures
+ */
+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_TRIGGER_RISING | IRQF_ONESHOT,
+ i2c->name,
+ indio_dev);
+ if (err)
+ return dev_err_probe(dev, err,
+ "failed to request irq (%d)\n", err);
+
+ 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 (%d)\n", err);
+
+ indio_dev->trig = iio_trigger_get(msa311->new_data_trig);
+
+ 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 (%d)\n", err);
+
+ 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 (%d)\n", err);
+
+ err = regmap_field_write(msa311->fields[F_LATCH_INT],
+ MSA311_LATCH_INT_LATCHED);
+ if (err)
+ return dev_err_probe(dev, err, "cannot latch int (%d)\n", err);
+
+ err = regmap_field_write(msa311->fields[F_RESET_INT], 1);
+ if (err)
+ return dev_err_probe(dev, err, "cannot reset int (%d)\n", err);
+
+ err = regmap_field_write(msa311->fields[F_INT1_NEW_DATA], 1);
+ if (err)
+ return dev_err_probe(dev, err,
+ "cannot map new data int (%d)\n", err);
+
+ return 0;
+}
+
+/**
+ * msa311_regmap_init() - MSA311 registers mapping initialization
+ * @msa311: MSA311 internal private state
+ *
+ * We do not need to hold msa311->lock here, because this function is used
+ * during I2C driver probing process only.
+ *
+ * Return: 0 on success, -ERRNO in other failures
+ */
+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;
+}
+
+/**
+ * msa311_powerdown() - MSA311 powerdown callback used by devres
+ *
+ * @msa311: MSA311 internal private state
+ */
+static void msa311_powerdown(void *msa311)
+{
+ struct device *dev = &((struct msa311_priv *)msa311)->i2c->dev;
+
+ /* Resume device if any */
+ pm_runtime_get_sync(dev);
+
+ /* Don't use autosuspend PM runtime feature anymore */
+ pm_runtime_dont_use_autosuspend(dev);
+
+ /* Suspend device right now */
+ pm_runtime_put_sync_suspend(dev);
+}
+
+/**
+ * msa311_probe() - MSA311 main I2C driver probe function
+ * @i2c: I2C client associated with MSA311 device
+ *
+ * Return: 0 on success
+ * -ENOMEM due to memory allocation failures
+ * -ERRNO in other failures
+ */
+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 dev_err_probe(dev, err,
+ "cannot enable runtime PM (%d)\n", err);
+
+ /* Resume msa311 logic before any interactions with registers */
+ err = pm_runtime_resume_and_get(dev);
+ if (err)
+ return dev_err_probe(dev, err,
+ "failed to resume runtime pm (%d)\n", 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 (%d)\n", err);
+
+ if (i2c->irq > 0) {
+ err = msa311_setup_interrupts(msa311);
+ if (err)
+ return err;
+ }
+
+ pm_runtime_mark_last_busy(dev);
+ pm_runtime_put_autosuspend(dev);
+
+ err = devm_add_action_or_reset(dev, msa311_powerdown, msa311);
+ if (err)
+ return dev_err_probe(dev, err,
+ "cannot add powerdown action (%d)\n", err);
+
+ err = devm_iio_device_register(dev, indio_dev);
+ if (err)
+ return dev_err_probe(dev, err,
+ "iio device register failed (%d)\n", err);
+
+ return 0;
+}
+
+/**
+ * msa311_runtime_suspend() - MSA311 pm runtime suspend callback
+ * @dev: Device object associated with MSA311
+ *
+ * Return: 0 on success, -ERRNO in msa311 comms failures
+ */
+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(&msa311->i2c->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(&msa311->i2c->dev,
+ "failed to power off device (%d)\n", err);
+ return err;
+ }
+
+ return 0;
+}
+
+/**
+ * msa311_runtime_resume() - MSA311 pm runtime resume callback
+ * @dev: Device object associated with MSA311
+ *
+ * Return: 0 on success, -ERRNO in msa311 comms failures
+ */
+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(&msa311->i2c->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(&msa311->i2c->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 <[email protected]>");
+MODULE_DESCRIPTION("MEMSensing MSA311 3-axis accelerometer driver");
+MODULE_LICENSE("GPL v2");
--
2.36.0
On Wed, 25 May 2022 18:15:32 +0000
Dmitry Rokosov <[email protected]> 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
>
> Signed-off-by: Dmitry Rokosov <[email protected]>
Hi Dmitry,
Various comments inline. One thing to think about is which of the comments
/ function documentation is useful and which is just stating the obvious.
If things are obvious it is usually better to not add documentation that
doesn't provide additional insight because it provides a maintenance
burden going forwards.
In a similar fashion, consider if a failure path that isn't already resulting
in a print is remotely likely. Error messages are something else that cause
maintenance burden, so there has to be some advantage in having them to
pay that cost.
Thanks,
Jonathan
> ---
> MAINTAINERS | 6 +
> drivers/iio/accel/Kconfig | 13 +
> drivers/iio/accel/Makefile | 2 +
> drivers/iio/accel/msa311.c | 1525 ++++++++++++++++++++++++++++++++++++
> 4 files changed, 1546 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 <[email protected]>
> +L: [email protected]
> +S: Maintained
> +F: drivers/iio/accel/msa311.c
> +
> MEN A21 WATCHDOG DRIVER
> M: Johannes Thumshirn <[email protected]>
> L: [email protected]
> 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..fe2cff1ed5ef
> --- /dev/null
> +++ b/drivers/iio/accel/msa311.c
> @@ -0,0 +1,1525 @@
> +// 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
> + *
> + * Copyright (c) 2022, SberDevices. All Rights Reserved.
> + *
> + * Author: Dmitry Rokosov <[email protected]>
> + */
> +
> +#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 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,
> +};
> +
> +/**
> + * MSA311_GENMASK() - MSA311 reg_field mask generator
> + * @field: requested regfield from msa311_reg_fields table
> + *
> + * Return: This helper returns reg_field mask to be applied.
> + */
> +#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,
> +};
> +
> +/**
> + * MSA311_ACCEL_CHANNEL() - Construct MSA311 accelerometer channel descriptor
> + * @axis: axis name in uppercase
> + */
> +#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 \
> +}
> +
> +/**
> + * MSA311_TIMESTAMP_CHANNEL() - Construct MSA311 timestamp channel descriptor
> + */
Documentation doesn't add anything that isn't clear from the code.
Whilst this may seem harmless, having unnecessary documentation adds
to the potential for it to become actively misleading if it is not
updated in future. As such, we tend to use comments only where there
is something non obvious to convey.
> +#define MSA311_TIMESTAMP_CHANNEL() IIO_CHAN_SOFT_TIMESTAMP(MSA311_SI_TIMESTAMP)
Just put this inline where used, there is no benefit in having a macro
for it.
> +
> +static const struct iio_chan_spec msa311_channels[] = {
> + MSA311_ACCEL_CHANNEL(X),
> + MSA311_ACCEL_CHANNEL(Y),
> + MSA311_ACCEL_CHANNEL(Z),
> + MSA311_TIMESTAMP_CHANNEL(),
> +};
> +
> +/**
> + * 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) {
> + dev_err(&msa311->i2c->dev, "failed to read odr value (%d)\n",
> + 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 err;
> +}
> +
> +/**
> + * 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)
> +{
> + 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) {
> + dev_err(&msa311->i2c->dev, "failed to read pwr_mode (%d)\n",
> + 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) {
> + return dev_err_probe(&msa311->i2c->dev, -EINVAL,
You should only use dev_err_probe() in functions exclusively called
from the probe path. It's there to provide special handling for deferred
probing so doesn't make a lot of sense eslewhere.
> + "failed to set odr %u.%uHz, not available in %s mode\n",
> + msa311_odr_table[odr].val,
> + msa311_odr_table[odr].val2 / 1000, mode);
> + }
> +
> + err = regmap_field_write(msa311->fields[F_ODR], odr);
> + if (err)
> + return dev_err_probe(&msa311->i2c->dev, err,
> + "failed to set odr value (%d)\n", err);
> +
> + return 0;
> +}
> +
> +static int msa311_read_raw_data(struct msa311_priv *msa311,
> + struct iio_chan_spec const *chan,
> + int *val, int *val2)
> +{
> + __le16 axis;
> + int err;
> +
> + err = pm_runtime_resume_and_get(&msa311->i2c->dev);
> + if (err) {
> + dev_err(&msa311->i2c->dev, "failed powerup (%d)\n", err);
> + return err;
> + }
> +
> + mutex_lock(&msa311->lock);
> + err = msa311_get_axis(msa311, chan, &axis);
> + if (err) {
> + dev_err(&msa311->i2c->dev,
> + "cannot get axis %s (%d)\n",
> + chan->datasheet_name, err);
> + mutex_unlock(&msa311->lock);
> + return err;
> + }
> + mutex_unlock(&msa311->lock);
move lock above if (err) to simplify error handling.
> +
> + pm_runtime_mark_last_busy(&msa311->i2c->dev);
> + pm_runtime_put_autosuspend(&msa311->i2c->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);
> + *val2 = 0;
val2 is never used by the IIO core, so shouldn't be necessary to clear it.
> +
> + return IIO_VAL_INT;
> +}
> +
> +static int msa311_read_scale(struct msa311_priv *msa311, int *val, int *val2)
> +{
> + unsigned int fs;
> + int err;
> +
> + mutex_lock(&msa311->lock);
> + err = regmap_field_read(msa311->fields[F_FS], &fs);
> + if (err) {
> + dev_err(&msa311->i2c->dev,
> + "cannot get actual scale (%d)\n", err);
> + mutex_unlock(&msa311->lock);
> + return err;
> + }
> + mutex_unlock(&msa311->lock);
move lock above the if (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 msa311_priv *msa311,
> + int *val, int *val2)
> +{
> + unsigned int odr;
> + int err;
> +
> + mutex_lock(&msa311->lock);
> + err = msa311_get_odr(msa311, &odr);
> + if (err) {
> + dev_err(&msa311->i2c->dev,
> + "cannot get actual freq (%d)\n", err);
> + mutex_unlock(&msa311->lock);
> + return err;
> + }
> + mutex_unlock(&msa311->lock);
Move lock above the if (err)
> +
> + *val = msa311_odr_table[odr].val;
> + *val2 = msa311_odr_table[odr].val2;
> +
> + return IIO_VAL_INT_PLUS_MICRO;
> +}
> +
> +/**
> + * msa311_read_avail() - IIO interface to request available device attr values
> + * @indio_dev: The IIO device associated with MSA311
> + * @chan: IIO channel specification
> + * @vals: Available device attr values
> + * @type: The type of the vals
> + * @length: Attr values array size
> + * @mask: Attr value type selector
> + *
> + * Return: IIO_AVAIL_LIST specificator on success,
> + * -EINVAL for unknown attr value type (bad mask),
I'd not bother with this level of documentation. It's not adding anything
that can't be seen by glancing at the code below.
Worth thinking about many of the other cases as well. Any documentation
is something that has to be maintained and reviewed. If it's not there
then that becomes a lot easier :)
Obviously there is a trade off though as good documentation will
call out the non obvious for the reader.
> + */
> +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;
> + }
> +}
> +
> +/**
> + * msa311_write_raw() - IIO interface function to write attr/accel data
> + * @indio_dev: The IIO device associated with MSA311
> + * @chan: IIO channel specification
> + * @val: Input data value, first part
> + * @val2: Input data value, second part
> + * @mask: Value type selector
> + *
> + * Return: 0 on success,
> + * -EINVAL for unknown value type (bad mask),
> + * -ERRNO in other failures
> + */
> +static int msa311_write_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int val, int val2, long mask)
> +{
> + struct msa311_priv *msa311 = iio_priv(indio_dev);
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_SCALE:
> + if (iio_buffer_enabled(indio_dev))
This races. We have
iio_device_claim_direct_mode() and the matching
release to avoid such races. They will ensure we are in
a mode where the buffer isn't enabled for the duration of
any action like this.
Mind you, why can't we write the scale while the buffer is turned on?
It might be unwise given no way of knowing when it applies, but
that is a problem for userspace dumb enough to do it ;)
If there is a reason to not do so, good to add a comment here
to say why not. Same obviously applies to sampling frequency below.
> + return -EBUSY;
> +
> + return msa311_write_scale(msa311, val, val2);
> +
> + case IIO_CHAN_INFO_SAMP_FREQ:
> + if (iio_buffer_enabled(indio_dev))
> + return -EBUSY;
> +
> + return msa311_write_samp_freq(msa311, val, val2);
> +
> + default:
> + return -EINVAL;
> + }
> +}
> +/**
> + * msa311_buffer_postdisable() - IIO buffer interface postdisable actions
> + * @indio_dev: The IIO device associated with MSA311
> + *
> + * Return: 0 on success,
> + * -ERRNO in other failures
I wouldn't document return values for these - particularly as it's
wrong in this case.
In general, I would advise not provide any documentation for short and obvious
functions. The code and naming should provide enough in many cases.
> + */
> +static int msa311_buffer_postdisable(struct iio_dev *indio_dev)
> +{
> + struct msa311_priv *msa311 = iio_priv(indio_dev);
> +
> + pm_runtime_mark_last_busy(&msa311->i2c->dev);
> + pm_runtime_put_autosuspend(&msa311->i2c->dev);
> +
> + return 0;
> +}
> +
> +/**
> + * msa311_validate_device() - IIO trigger interface to validate requested device
> + * @trig: Appropriate IIO trigger
> + * @indio_dev: Requested IIO device
> + *
> + * Return: 0 on success,
> + * -EINVAL when indio_dev isn't linked with appropriate trigger
> + */
> +static int msa311_validate_device(struct iio_trigger *trig,
> + struct iio_dev *indio_dev)
> +{
> + struct iio_dev *indio = iio_trigger_get_drvdata(trig);
> +
> + if (indio != indio_dev)
Give these clearer names or avoid the local variable so it
is obvious.
if (indio != iio_trigger_get_drvdata())
for example.
> + return -EINVAL;
> +
> + return 0;
> +}
> +
...
> +/**
> + * msa311_check_partid() - Check MSA311 WHO_AM_I identifier.
> + * @msa311: MSA311 internal private state
> + *
> + * Return: 0 on success (MSA311 device was found on the I2C bus)
> + * 0 with warning notice when MSA311 device was found on the I2C bus,
> + * but chipid has not expected value
> + * -ERRNO in other failures
> + */
> +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)
> + return dev_err_probe(dev, err,
> + "failed to read partid (%d)\n", err);
> +
> + dev_info(dev, "Found MSA311 compatible chip[%#x]\n", partid);
This is noise that we generally try to avoid in drivers.
The log on systems is long enough as it is without needing to log
success conditions. dev_dbg is fine if you want to provide a way
of getting to this.
> +
> + if (partid != MSA311_WHO_AM_I)
> + dev_warn(dev, "invalid partid (%#x), expected (%#x)\n",
> + partid, MSA311_WHO_AM_I);
> +
> + return 0;
> +}
> +
> +/**
> + * msa311_chip_init() - MSA311 chip initialization sequence
> + * @msa311: MSA311 internal private state
> + *
> + * We do not need to hold msa311->lock here, because this function is used
> + * during I2C driver probing process only.
> + *
> + * Return: 0 on success, -ERRNO in other failures
> + */
> +static int msa311_chip_init(struct msa311_priv *msa311)
> +{
> + 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(&msa311->i2c->dev, err,
A local
struct device *dev; variable will make these all a little more readable.
> + "cannot soft reset all logic (%d)\n", err);
> +
> + err = msa311_set_pwr_mode(msa311, MSA311_PWR_MODE_NORMAL);
> + if (err)
> + return dev_err_probe(&msa311->i2c->dev, err,
> + "bad normal mode transition (%d)\n", err);
> +
> + err = regmap_write(msa311->regs, MSA311_RANGE_REG, MSA311_FS_16G);
> + if (err)
> + return dev_err_probe(&msa311->i2c->dev, err,
> + "failed to setup accel range (%d)\n", err);
> +
> + /* Disable all interrupts by default */
> + err = regmap_write(msa311->regs, MSA311_INT_SET_0_REG, 0);
> + if (err)
> + return dev_err_probe(&msa311->i2c->dev, err,
> + "cannot disable set0 intrs (%d)\n", err);
> +
> + err = regmap_write(msa311->regs, MSA311_INT_SET_1_REG, 0);
> + if (err)
> + return dev_err_probe(&msa311->i2c->dev, err,
> + "cannot disable set1 intrs (%d)\n", err);
> +
> + /* Unmap all INT1 interrupts by default */
> + err = regmap_write(msa311->regs, MSA311_INT_MAP_0_REG, 0);
> + if (err)
> + return dev_err_probe(&msa311->i2c->dev, err,
> + "failed to unmap map0 intrs (%d)\n", err);
> +
> + err = regmap_write(msa311->regs, MSA311_INT_MAP_1_REG, 0);
> + if (err)
> + return dev_err_probe(&msa311->i2c->dev, err,
> + "failed to unmap map1 intrs (%d)\n", err);
> +
> + /* 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(&msa311->i2c->dev, err,
> + "cannot enable all axes (%d)\n", err);
> +
> + err = msa311_set_odr(msa311, MSA311_ODR_125_HZ);
> + if (err)
> + return dev_err_probe(&msa311->i2c->dev, err,
> + "failed to set accel freq (%d)\n", err);
> +
> + return 0;
> +}
> +
> +/**
> + * msa311_setup_interrupts() - MSA311 interrupts initialization sequence
> + * @msa311: MSA311 internal private state
> + *
> + * We register new data trigger firstly and then setup MSA311 int registers.
> + * This function does not need to hold msa311->lock, because it is used
> + * during I2C driver probing process only.
> + *
> + * Return: 0 on success, -ERRNO in other failures
> + */
> +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_TRIGGER_RISING | IRQF_ONESHOT,
Should leave the 'rising' part to be specified by the firmware (e.g. DT)
If someone happens to have an inverter in the way (common for cheap
level shifting) then the direction of this will be the opposite.
If you don't specify it here, it should get picked up from DT IIRC.
We have this wrong in a lot of drivers, but can't easily fix it
after the event without possibly breaking boards relying on it.
> + i2c->name,
> + indio_dev);
> + if (err)
> + return dev_err_probe(dev, err,
> + "failed to request irq (%d)\n", err);
> +
> + 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 (%d)\n", err);
> +
> + indio_dev->trig = iio_trigger_get(msa311->new_data_trig);
Drop this. Your driver now supports other triggers so leave
this decision to userspace (thus avoiding the issue with remove discussed in
v1).
> +
> + 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 (%d)\n", err);
> +
> + 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 (%d)\n", err);
> +
> + err = regmap_field_write(msa311->fields[F_LATCH_INT],
> + MSA311_LATCH_INT_LATCHED);
> + if (err)
> + return dev_err_probe(dev, err, "cannot latch int (%d)\n", err);
> +
> + err = regmap_field_write(msa311->fields[F_RESET_INT], 1);
> + if (err)
> + return dev_err_probe(dev, err, "cannot reset int (%d)\n", err);
> +
> + err = regmap_field_write(msa311->fields[F_INT1_NEW_DATA], 1);
> + if (err)
> + return dev_err_probe(dev, err,
> + "cannot map new data int (%d)\n", err);
> +
> + return 0;
> +}
> +
> +/**
> + * msa311_powerdown() - MSA311 powerdown callback used by devres
> + *
> + * @msa311: MSA311 internal private state
> + */
> +static void msa311_powerdown(void *msa311)
> +{
> + struct device *dev = &((struct msa311_priv *)msa311)->i2c->dev;
As you want the dev, pass the devm_add_action_or_reset the dev
in the first place so you can avoid this rather ugly line!
> +
> + /* Resume device if any */
> + pm_runtime_get_sync(dev);
> +
> + /* Don't use autosuspend PM runtime feature anymore */
> + pm_runtime_dont_use_autosuspend(dev);
this is done for you in the unwind of
devm_pm_runtime_enable() so If you need to repeat it here, explain why.
> +
> + /* Suspend device right now */
> + pm_runtime_put_sync_suspend(dev);
At this point is this any different from pm_runtime_put_sync?
> +}
> +
> +/**
> + * msa311_probe() - MSA311 main I2C driver probe function
> + * @i2c: I2C client associated with MSA311 device
> + *
> + * Return: 0 on success
> + * -ENOMEM due to memory allocation failures
> + * -ERRNO in other failures
> + */
> +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 dev_err_probe(dev, err,
> + "cannot enable runtime PM (%d)\n", err);
> +
> + /* Resume msa311 logic before any interactions with registers */
> + err = pm_runtime_resume_and_get(dev);
> + if (err)
> + return dev_err_probe(dev, err,
> + "failed to resume runtime pm (%d)\n", err);
Given you already report an error message on the failure path in resume,
having one here as well is probably excessive as any other failure case
is very unlikely.
> +
> + 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 (%d)\n", err);
> +
> + if (i2c->irq > 0) {
> + err = msa311_setup_interrupts(msa311);
> + if (err)
> + return err;
> + }
> +
> + pm_runtime_mark_last_busy(dev);
> + pm_runtime_put_autosuspend(dev);
> +
> + err = devm_add_action_or_reset(dev, msa311_powerdown, msa311);
It's not immediately clear what this devm action corresponds to and hence
why it is at this point in the probe. Needs a comment. I think it's
a way of forcing suspend to have definitely occurred?
> + if (err)
> + return dev_err_probe(dev, err,
> + "cannot add powerdown action (%d)\n", err);
> +
> + err = devm_iio_device_register(dev, indio_dev);
> + if (err)
> + return dev_err_probe(dev, err,
> + "iio device register failed (%d)\n", err);
> +
> + return 0;
> +}
Hello Jonathan,
Thank you for comments. Please find my replies below.
On Sat, May 28, 2022 at 07:33:34PM +0100, Jonathan Cameron wrote:
> On Wed, 25 May 2022 18:15:32 +0000
> Dmitry Rokosov <[email protected]> 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
> >
> > Signed-off-by: Dmitry Rokosov <[email protected]>
>
> Hi Dmitry,
>
> Various comments inline. One thing to think about is which of the comments
> / function documentation is useful and which is just stating the obvious.
> If things are obvious it is usually better to not add documentation that
> doesn't provide additional insight because it provides a maintenance
> burden going forwards.
>
> In a similar fashion, consider if a failure path that isn't already resulting
> in a print is remotely likely. Error messages are something else that cause
> maintenance burden, so there has to be some advantage in having them to
> pay that cost.
>
> Thanks,
>
> Jonathan
>
Sure, no problem. I'll send updated driver without obvious comments and
error messages in v3 patch series.
> > +/**
> > + * msa311_write_raw() - IIO interface function to write attr/accel data
> > + * @indio_dev: The IIO device associated with MSA311
> > + * @chan: IIO channel specification
> > + * @val: Input data value, first part
> > + * @val2: Input data value, second part
> > + * @mask: Value type selector
> > + *
> > + * Return: 0 on success,
> > + * -EINVAL for unknown value type (bad mask),
> > + * -ERRNO in other failures
> > + */
> > +static int msa311_write_raw(struct iio_dev *indio_dev,
> > + struct iio_chan_spec const *chan,
> > + int val, int val2, long mask)
> > +{
> > + struct msa311_priv *msa311 = iio_priv(indio_dev);
> > +
> > + switch (mask) {
> > + case IIO_CHAN_INFO_SCALE:
> > + if (iio_buffer_enabled(indio_dev))
>
> This races. We have
> iio_device_claim_direct_mode() and the matching
> release to avoid such races. They will ensure we are in
> a mode where the buffer isn't enabled for the duration of
> any action like this.
>
>
> Mind you, why can't we write the scale while the buffer is turned on?
> It might be unwise given no way of knowing when it applies, but
> that is a problem for userspace dumb enough to do it ;)
>
> If there is a reason to not do so, good to add a comment here
> to say why not. Same obviously applies to sampling frequency below.
I've inserted such condition, because I used pm_runtime() calls inside
trig_set_state() callback, which was wrong behavior (as you correctly
mentioned before). In those situations, if userspace changed scale or freq
during a buffer reading, it was a time slot where device could go to sleep,
and this is a bad thing. Anyway, for now I'm using pm_runtime() callbacks
during buffer_enable and buffer_disable executions, so I can remove this
condition from scale/freq write operations and race will gone.
> > + i2c->name,
> > + indio_dev);
> > + if (err)
> > + return dev_err_probe(dev, err,
> > + "failed to request irq (%d)\n", err);
> > +
> > + 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 (%d)\n", err);
> > +
> > + indio_dev->trig = iio_trigger_get(msa311->new_data_trig);
>
> Drop this. Your driver now supports other triggers so leave
> this decision to userspace (thus avoiding the issue with remove discussed in
> v1).
>
Okay, but many other drivers have such the same problem. May be it's
better to stay in the consistent state with them? What do you think?
> > +
> > + /* Resume device if any */
> > + pm_runtime_get_sync(dev);
> > +
> > + /* Don't use autosuspend PM runtime feature anymore */
> > + pm_runtime_dont_use_autosuspend(dev);
>
> this is done for you in the unwind of
> devm_pm_runtime_enable() so If you need to repeat it here, explain why.
>
As I understood, devm_pm_runtime_enable() executes pm_runtime_disable()
during resource utilization. I'm not sure that pm_runtime_disable()
switches off autosuspend feature. I'll take a look and remove this line
if needed.
> > +
> > + /* Suspend device right now */
> > + pm_runtime_put_sync_suspend(dev);
>
> At this point is this any different from pm_runtime_put_sync?
>
Yes. Function pm_runtime_put_sync() transfers device to IDLE state if
needed, we do not want it here. Using pm_runtime_put_sync_suspend() our
goal is for msa311 transition to SUSPEND state when driver unloading.
> > +}
> > +
> > +/**
> > + * msa311_probe() - MSA311 main I2C driver probe function
> > + * @i2c: I2C client associated with MSA311 device
> > + *
> > + * Return: 0 on success
> > + * -ENOMEM due to memory allocation failures
> > + * -ERRNO in other failures
> > + */
> > +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 dev_err_probe(dev, err,
> > + "cannot enable runtime PM (%d)\n", err);
> > +
> > + /* Resume msa311 logic before any interactions with registers */
> > + err = pm_runtime_resume_and_get(dev);
> > + if (err)
> > + return dev_err_probe(dev, err,
> > + "failed to resume runtime pm (%d)\n", err);
>
> Given you already report an error message on the failure path in resume,
> having one here as well is probably excessive as any other failure case
> is very unlikely.
>
This dev_err() message is located here, because
pm_runtime_resume_and_get() can contain internal errors which are not
dependent on driver logic. So I try to catch such errors in this place.
> > +
> > + 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 (%d)\n", err);
> > +
> > + if (i2c->irq > 0) {
> > + err = msa311_setup_interrupts(msa311);
> > + if (err)
> > + return err;
> > + }
> > +
> > + pm_runtime_mark_last_busy(dev);
> > + pm_runtime_put_autosuspend(dev);
> > +
> > + err = devm_add_action_or_reset(dev, msa311_powerdown, msa311);
>
> It's not immediately clear what this devm action corresponds to and hence
> why it is at this point in the probe. Needs a comment. I think it's
> a way of forcing suspend to have definitely occurred?
>
Above devm action is needed to force driver to go to SUSPEND mode during
unloading. In other words, the device 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)
--
Thank you,
Dmitry
On Thu, 9 Jun 2022 18:09:05 +0000
Dmitry Rokosov <[email protected]> wrote:
> Hello Jonathan,
>
> Thank you for comments. Please find my replies below.
>
...
> > > + i2c->name,
> > > + indio_dev);
> > > + if (err)
> > > + return dev_err_probe(dev, err,
> > > + "failed to request irq (%d)\n", err);
> > > +
> > > + 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 (%d)\n", err);
> > > +
> > > + indio_dev->trig = iio_trigger_get(msa311->new_data_trig);
> >
> > Drop this. Your driver now supports other triggers so leave
> > this decision to userspace (thus avoiding the issue with remove discussed in
> > v1).
> >
>
> Okay, but many other drivers have such the same problem. May be it's
> better to stay in the consistent state with them? What do you think?
There are special cases:
- only one trigger supported.
- originally only one trigger was supported, but that got relaxed later
and we need to maintain the default to avoid ABI changes.
- maybe one or two that slipped through.
We didn't start setting default triggers until fairly recently so lots
of drivers don't set one. That doesn't mean we shoudn't fix the
issue you identified but in this case it's a policy decision so probably
belongs in userspace anyway.
...
> > > +
> > > + /* Resume msa311 logic before any interactions with registers */
> > > + err = pm_runtime_resume_and_get(dev);
> > > + if (err)
> > > + return dev_err_probe(dev, err,
> > > + "failed to resume runtime pm (%d)\n", err);
> >
> > Given you already report an error message on the failure path in resume,
> > having one here as well is probably excessive as any other failure case
> > is very unlikely.
> >
>
> This dev_err() message is located here, because
> pm_runtime_resume_and_get() can contain internal errors which are not
> dependent on driver logic. So I try to catch such errors in this place.
It's a trade off, but generally we don't spend too much effort printing
errors for things that aren't reasonably likely to happen. Obviously
we do return the error though so that we know some debugging is needed
if it happens!
>
> > > +
> > > + 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 (%d)\n", err);
> > > +
> > > + if (i2c->irq > 0) {
> > > + err = msa311_setup_interrupts(msa311);
> > > + if (err)
> > > + return err;
> > > + }
> > > +
> > > + pm_runtime_mark_last_busy(dev);
> > > + pm_runtime_put_autosuspend(dev);
> > > +
> > > + err = devm_add_action_or_reset(dev, msa311_powerdown, msa311);
> >
> > It's not immediately clear what this devm action corresponds to and hence
> > why it is at this point in the probe. Needs a comment. I think it's
> > a way of forcing suspend to have definitely occurred?
> >
>
> Above devm action is needed to force driver to go to SUSPEND mode during
> unloading. In other words, the device 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)
>
That's fine, but add a comment to explain 2.
As a general rule, devm_ actions clearly match against setup path actions
so when they don't it's useful to provide a little more info.
Jonathan
Le 25/05/2022 à 20:15, Dmitry Rokosov a écrit :
> 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
>
> Signed-off-by: Dmitry Rokosov <[email protected]>
> ---
> MAINTAINERS | 6 +
> drivers/iio/accel/Kconfig | 13 +
> drivers/iio/accel/Makefile | 2 +
> drivers/iio/accel/msa311.c | 1525 ++++++++++++++++++++++++++++++++++++
> 4 files changed, 1546 insertions(+)
> create mode 100644 drivers/iio/accel/msa311.c
[...]
> +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 dev_err_probe(dev, err,
> + "cannot enable runtime PM (%d)\n", err);
> +
Nit: dev_err_probe() already print the 'err' (in a human readable
maner), so unless the code itself is of any interest, it can be removed:
i.e.:
+ return dev_err_probe(dev, err,
+ "cannot enable runtime PM");
This pattern is used in many places.
just my 2c.
CJ
Hello Christophe,
On Sun, Jun 12, 2022 at 11:46:19AM +0200, Christophe JAILLET wrote:
> Le 25/05/2022 à 20:15, Dmitry Rokosov a écrit :
> > +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 dev_err_probe(dev, err,
> > + "cannot enable runtime PM (%d)\n", err);
> > +
>
> Nit: dev_err_probe() already print the 'err' (in a human readable maner), so
> unless the code itself is of any interest, it can be removed:
>
> i.e.:
> + return dev_err_probe(dev, err,
> + "cannot enable runtime PM");
>
> This pattern is used in many places.
Thank you for pointing this! It's a really helpful dev_err_probe() trait.
I'll use it in the v3 patchset.
--
Thank you,
Dmitry