linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v5 0/7] iio: accel: add MSA311 accelerometer driver
@ 2022-08-12 16:52 Dmitry Rokosov
  2022-08-12 16:52 ` [PATCH v5 1/7] lib/string_helpers: Add str_read_write() helper Dmitry Rokosov
                   ` (6 more replies)
  0 siblings, 7 replies; 17+ messages in thread
From: Dmitry Rokosov @ 2022-08-12 16:52 UTC (permalink / raw)
  To: akpm, jic23, robh+dt, andriy.shevchenko, andy.shevchenko,
	christophe.jaillet, stano.jakubek, shawnguo, stephan,
	daniel.lezcano, wsa, lars, Michael.Hennerich, jbhayana,
	lucas.demarchi, jani.nikula, linus.walleij
  Cc: linux-iio, kernel, devicetree, linux-kernel, Dmitry Rokosov

MSA311 is a tri-axial, low-g accelerometer with I2C digital output for
sensitivity consumer applications. It has dynamic user-selectable full
scales range of +-2g/+-4g/+-8g/+-16g and allows acceleration measurements
with output data rates from 1Hz to 1000Hz.

Spec: https://cdn-shop.adafruit.com/product-files/5309/MSA311-V1.1-ENG.pdf

This driver supports following MSA311 features:
    - IIO interface
    - Different power modes: NORMAL and SUSPEND (using pm_runtime)
    - ODR (Output Data Rate) selection
    - Scale and samp_freq selection
    - IIO triggered buffer, IIO reg access
    - NEW_DATA interrupt + trigger

Below features to be done:
    - Motion Events: ACTIVE, TAP, ORIENT, FREEFALL
    - Low Power mode

Also this patchset has new vendor prefix for MEMSensing Microsystems and
MSA311 dt-binding schema.

You can test msa311 driver using libiio and gnuplot following below
instructions:
  $ # Create hrtimer trigger object
  $ mkdir /sys/kernel/config/iio/triggers/hrtimer/iio_hrtimer_trigger
  $ # Read 4K samples using msa311-13-new-data trigger (irq) and
  $ # buffer with depth equals to 64 samples and rotate device a little bit
  $ iio_readdev -u "local:" -b 64 -s 4096 -t msa311-13-new-data -T 0 \
  $             msa311-13 > /tmp/msa311.dat
  $ # Or using hrtimer trigger instead of msa311-13-new-data trigger
  $ iio_readdev -u "local:" -b 64 -s 4096 -t iio_hrtimer_trigger -T 0 \
  $                msa311 > /data/local/tmp/msa311.dat
  $ cat <<EOF >> msa311_data.gnu
  set title "MSA311 Accel Data"

  set key below

  set xdata time
  set format x "%H:%M\n%.4S"
  set xlabel "timestamp"

  set autoscale y

  plot 'msa311.dat' binary endian=little \
                    format='%int16%int16%int16%uint16%uint64' using \
                    (\$5/1000000000):(int(\$1)/16) title "acc_x" \
                    with lines,\\
       'msa311.dat' binary endian=little \
                    format='%int16%int16%int16%uint16%uint64' using \
                    (\$5/1000000000):(int(\$2)/16) title "acc_y" \
                    with lines,\\
       'msa311.dat' binary endian=little \
                    format='%int16%int16%int16%uint16%uint64' using \
                    (\$5/1000000000):(int(\$3)/16) title "acc_z" with lines
  EOF
  $ gnuplot --persist msa311_data.gnu

Changes:
* v4->v5:
    - used chip_name for IRQ and trigger name generation in the form
      msa311-%partid%-*
    - split generic with IIO headers
    - fixup some mathematical forms inside comments
    - provided small code refactoring for commas, comments, and logs
    - removed errno value logging from dev_err_probe() calls per
      Christophe suggestion to avoid extra errno string output
    - returned INDIO_DIRECT_MODE default initializer for indio_dev->modes
    - provided pm_runtime_set_active() call during msa311 probing to let
      runtime PM know that we are starting up with msa311 chip turned on
      as Jonathan suggested
    - used HZ units for hz calculations
    - removed logging contractions
    - removed double calling of regulator_disable() in the probe error
      path
    - used postfix increment operations instead of prefix form :)
    - used %pe specifier to print errno as a string in the dev_err()
    - merged with HZ units patchset from
      https://lore.kernel.org/linux-iio/20220801143811.14817-1-ddrokosov@sberdevices.ru/
    - merged with Andy's str_read_write() patch from
      https://lore.kernel.org/linux-i2c/20220703154232.55549-1-andriy.shevchenko@linux.intel.com/
    - used str_read_write() string helper inside driver implementation

* v3->v4:
    - totally reworked pm_runtime flow based on Jonathan suggestions
    - replaced temporary field variable with tmp pointer to field in the
      MSA311_GENMASK macro helper
    - removed i2c pointer from MSA311 private context, retrieved it from
      msa311 object, if anything
    - added struct device *dev reference to MSA311 private context for
      easier msa311->dev translation
    - moved regmap pointer to the beginning of MSA311 private context to
      save some instructions
    - refactored 'if' conditions to be positive and shorter
    - moved msa311_check_partid() and msa311_soft_reset() to separate
      routines and call them before powerup IP logic during probe()
      execution
    - used str_enable_disable() string helper as Andy suggested
    - used msa311_pwr_modes const char * array to translate power modes
      to strings
    - reworked hz->ms translation, used MICROHZ_PER_HZ from the
      following review:
      https://lore.kernel.org/linux-iio/20220801143811.14817-1-ddrokosov@sberdevices.ru/
    - moved dev_dbg() log about MSA311 compatible chip finding under
      partid check
    - refactored stack variables definitions based on "longer lines
      first" thumb
    - used 0 instead of INDIO_DIRECT_MODE before iio buffer setup
    - moved i2c->irq check to msa311_setup_interrupts()
    - removed dev_dbg() prints from ->resume() and ->suspend() callbacks
    - removed "description" fields from "interrupts" and i2c "reg" YAML
      schema nodes
    - implemented simple power supply for MSA311 (vdd-supply)
    - reworked shared_by_all info mask to shared_by_type for MSA311
      accel channels
    - tagged datasheet URL link in the commit message
    - made mutex-based critical section shorter inside odr and fs loop as
      Jonathan suggested
    - fixed wording in the commit messages and comments a little bit,
      refactored some indentations
    - replaced blank lines between register offset definitions with
      short comments

* v2->v3:
    - removed MSA311_TIMESTAMP_CHANNEL() macro, used IIO_CHAN_SOFT_TIMESTAMP
      directly
    - do not call dev_err_probe() inside functions, which is used not only
      from probe() path
    - simplified error handling a little bit
    - used iio_device_claim_direct_mode() and
      iio_device_release_direct_mode() to lock attributes when buffer mode
      is enabled
    - prohibited sampling frequency changing during buffer usage because
      otherwise sometimes MSA311 returns outliers when frequency values
      grow up in the read operation moment
    - allowed scale value changing when buffer mode is enabled
    - removed IRQF_TRIGGER_RISING irq flag from irg registration because
      it's provided from device tree directly
    - do not switch off autosuspend from powerdown() devm callback,
      because it's already done from pm_runtime_disable() during
      devm pm_runtime actions
    - provided more information why we need force suspend state for MSA311
      in the powerdown flow
    - reworked comments stuff: removed obvious extra comments, provided
      more details in the complex driver code places

* v1->v2:
    - memsensing vendor prefix was moved to right place by
      alphabetical order
    - LOW mode mention was deleted, because LOW mode isn't supported
      in the current driver version
    - reworked some enums with gaps to defines
    - reworked register names as Jonathan mentioned in the v1
    - do not use regmap_field API for entire registers
    - deleted all extra comments
    - supported info_mask_*_avail bitmaps instead of explicit IIO attrs
      definitions, implemented read_avail() callback for samp_freq and
      scale values
    - msa311 mutex is still used to protect msa311 power transitions,
      samp_freq/scale tune and axes data handling; described this lock
      more informative
    - ask new_data interruption status from appropriate register,
      do not hold atomic variable for that
    - optimized reads of axes data by I2C using regmap_bulk API
    - use dev_err_probe() instead of dev_err() for all probe() code paths
    - from now all I2C bus communication failures are interpreted as errors
    - described wait_from_next() semantic better
    - deleted all unneeded pm wrappers
    - interpreter all axes data as __le16 type and adjust them to right
      format (endianness + sign) for raw() flow only
    - redesigned msa311_fs_table[] to 2D matrix (it's more comfortable
      format for read_avail() callback)
    - align and initialize msa311 buffer before pushing properly
    - use pm_runtime resume and suspend from buffer preenable/postdisable,
      deleted them from trigger set_state
    - supported multiple trigger usage (tested with external hrtimer
      trigger and internal new_data trigger)
    - moved all irq related stuff to msa311_setup_interrupts() routine
    - implemented msa311_powerdown() devm release action
    - reworked initialization of pm_runtime msa311 flow, use
      autosuspend logic
    - purged driver remove() callback, because of devm release logic runs
      all deinit stuff fully
    - fixed dts bindings problems
    - changed irq type in the dt-binding description, because interrupt
      type for msa311 should have the same type as i2c irq, for example
      using the gpio_intc it's IRQ_TYPE_EDGE_RISING usually. Otherwise
      we may lose irq map on the second and further insmod attempts

Andy Shevchenko (1):
  lib/string_helpers: Add str_read_write() helper

Dmitry Rokosov (6):
  units: complement the set of Hz units
  iio: accel: adxl345: use HZ macro from units.h
  iio: common: scmi_sensors: use HZ macro from units.h
  dt-bindings: vendor-prefixes: add MEMSensing Microsystems Co., Ltd.
  iio: add MEMSensing MSA311 3-axis accelerometer driver
  dt-bindings: iio: accel: add dt-binding schema for msa311 accel driver

 .../bindings/iio/accel/memsensing,msa311.yaml |   53 +
 .../devicetree/bindings/vendor-prefixes.yaml  |    2 +
 MAINTAINERS                                   |    7 +
 drivers/iio/accel/Kconfig                     |   13 +
 drivers/iio/accel/Makefile                    |    2 +
 drivers/iio/accel/adxl345_core.c              |    7 +-
 drivers/iio/accel/msa311.c                    | 1339 +++++++++++++++++
 drivers/iio/common/scmi_sensors/scmi_iio.c    |    8 +-
 include/linux/string_helpers.h                |    5 +
 include/linux/units.h                         |    3 +
 10 files changed, 1432 insertions(+), 7 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/iio/accel/memsensing,msa311.yaml
 create mode 100644 drivers/iio/accel/msa311.c

-- 
2.36.0

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH v5 1/7] lib/string_helpers: Add str_read_write() helper
  2022-08-12 16:52 [PATCH v5 0/7] iio: accel: add MSA311 accelerometer driver Dmitry Rokosov
@ 2022-08-12 16:52 ` Dmitry Rokosov
  2022-08-12 17:15   ` Dmitry Rokosov
  2022-08-12 16:52 ` [PATCH v5 2/7] units: complement the set of Hz units Dmitry Rokosov
                   ` (5 subsequent siblings)
  6 siblings, 1 reply; 17+ messages in thread
From: Dmitry Rokosov @ 2022-08-12 16:52 UTC (permalink / raw)
  To: akpm, jic23, robh+dt, andriy.shevchenko, andy.shevchenko,
	christophe.jaillet, stano.jakubek, shawnguo, stephan,
	daniel.lezcano, wsa, lars, Michael.Hennerich, jbhayana,
	lucas.demarchi, jani.nikula, linus.walleij
  Cc: linux-iio, kernel, devicetree, linux-kernel, Dmitry Rokosov

From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Add str_read_write() helper to return 'read' or 'write' string literal.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Dmitry Rokosov <ddrokosov@sberdevices.ru>
---
 include/linux/string_helpers.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/include/linux/string_helpers.h b/include/linux/string_helpers.h
index 4d72258d42fd..9e22cd78f3b8 100644
--- a/include/linux/string_helpers.h
+++ b/include/linux/string_helpers.h
@@ -126,4 +126,9 @@ static inline const char *str_enabled_disabled(bool v)
 	return v ? "enabled" : "disabled";
 }
 
+static inline const char *str_read_write(bool v)
+{
+	return v ? "read" : "write";
+}
+
 #endif
-- 
2.36.0

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH v5 2/7] units: complement the set of Hz units
  2022-08-12 16:52 [PATCH v5 0/7] iio: accel: add MSA311 accelerometer driver Dmitry Rokosov
  2022-08-12 16:52 ` [PATCH v5 1/7] lib/string_helpers: Add str_read_write() helper Dmitry Rokosov
@ 2022-08-12 16:52 ` Dmitry Rokosov
  2022-08-28 15:55   ` Jonathan Cameron
  2022-08-12 16:52 ` [PATCH v5 4/7] iio: common: scmi_sensors: use HZ macro from units.h Dmitry Rokosov
                   ` (4 subsequent siblings)
  6 siblings, 1 reply; 17+ messages in thread
From: Dmitry Rokosov @ 2022-08-12 16:52 UTC (permalink / raw)
  To: akpm, jic23, robh+dt, andriy.shevchenko, andy.shevchenko,
	christophe.jaillet, stano.jakubek, shawnguo, stephan,
	daniel.lezcano, wsa, lars, Michael.Hennerich, jbhayana,
	lucas.demarchi, jani.nikula, linus.walleij
  Cc: linux-iio, kernel, devicetree, linux-kernel, Dmitry Rokosov

Currently, Hz units do not have milli, micro and nano Hz coefficients.
Some drivers (IIO especially) use their analogues to calculate
appropriate Hz values. This patch includes them to units.h definitions,
so they can be used from different kernel places.

Signed-off-by: Dmitry Rokosov <ddrokosov@sberdevices.ru>
---
 include/linux/units.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/linux/units.h b/include/linux/units.h
index 681fc652e3d7..2793a41e73a2 100644
--- a/include/linux/units.h
+++ b/include/linux/units.h
@@ -20,6 +20,9 @@
 #define PICO	1000000000000ULL
 #define FEMTO	1000000000000000ULL
 
+#define NANOHZ_PER_HZ		1000000000UL
+#define MICROHZ_PER_HZ		1000000UL
+#define MILLIHZ_PER_HZ		1000UL
 #define HZ_PER_KHZ		1000UL
 #define KHZ_PER_MHZ		1000UL
 #define HZ_PER_MHZ		1000000UL
-- 
2.36.0

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH v5 4/7] iio: common: scmi_sensors: use HZ macro from units.h
  2022-08-12 16:52 [PATCH v5 0/7] iio: accel: add MSA311 accelerometer driver Dmitry Rokosov
  2022-08-12 16:52 ` [PATCH v5 1/7] lib/string_helpers: Add str_read_write() helper Dmitry Rokosov
  2022-08-12 16:52 ` [PATCH v5 2/7] units: complement the set of Hz units Dmitry Rokosov
@ 2022-08-12 16:52 ` Dmitry Rokosov
  2022-08-12 16:52 ` [PATCH v5 3/7] iio: accel: adxl345: " Dmitry Rokosov
                   ` (3 subsequent siblings)
  6 siblings, 0 replies; 17+ messages in thread
From: Dmitry Rokosov @ 2022-08-12 16:52 UTC (permalink / raw)
  To: akpm, jic23, robh+dt, andriy.shevchenko, andy.shevchenko,
	christophe.jaillet, stano.jakubek, shawnguo, stephan,
	daniel.lezcano, wsa, lars, Michael.Hennerich, jbhayana,
	lucas.demarchi, jani.nikula, linus.walleij
  Cc: linux-iio, kernel, devicetree, linux-kernel, Dmitry Rokosov

Remove duplicated definition of UHZ_PER_HZ, because it's available in
the units.h as MICROHZ_PER_HZ.

Signed-off-by: Dmitry Rokosov <ddrokosov@sberdevices.ru>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
---
 drivers/iio/common/scmi_sensors/scmi_iio.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/common/scmi_sensors/scmi_iio.c b/drivers/iio/common/scmi_sensors/scmi_iio.c
index 793d628db55f..54ccf19ab2bb 100644
--- a/drivers/iio/common/scmi_sensors/scmi_iio.c
+++ b/drivers/iio/common/scmi_sensors/scmi_iio.c
@@ -18,6 +18,7 @@
 #include <linux/scmi_protocol.h>
 #include <linux/time.h>
 #include <linux/types.h>
+#include <linux/units.h>
 
 #define SCMI_IIO_NUM_OF_AXIS 3
 
@@ -130,7 +131,6 @@ static const struct iio_buffer_setup_ops scmi_iio_buffer_ops = {
 static int scmi_iio_set_odr_val(struct iio_dev *iio_dev, int val, int val2)
 {
 	struct scmi_iio_priv *sensor = iio_priv(iio_dev);
-	const unsigned long UHZ_PER_HZ = 1000000UL;
 	u64 sec, mult, uHz, sf;
 	u32 sensor_config;
 	char buf[32];
@@ -145,7 +145,7 @@ static int scmi_iio_set_odr_val(struct iio_dev *iio_dev, int val, int val2)
 		return err;
 	}
 
-	uHz = val * UHZ_PER_HZ + val2;
+	uHz = val * MICROHZ_PER_HZ + val2;
 
 	/*
 	 * The seconds field in the sensor interval in SCMI is 16 bits long
@@ -156,10 +156,10 @@ static int scmi_iio_set_odr_val(struct iio_dev *iio_dev, int val, int val2)
 	 * count the number of characters
 	 */
 	sf = (u64)uHz * 0xFFFF;
-	do_div(sf,  UHZ_PER_HZ);
+	do_div(sf,  MICROHZ_PER_HZ);
 	mult = scnprintf(buf, sizeof(buf), "%llu", sf) - 1;
 
-	sec = int_pow(10, mult) * UHZ_PER_HZ;
+	sec = int_pow(10, mult) * MICROHZ_PER_HZ;
 	do_div(sec, uHz);
 	if (sec == 0) {
 		dev_err(&iio_dev->dev,
-- 
2.36.0

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH v5 3/7] iio: accel: adxl345: use HZ macro from units.h
  2022-08-12 16:52 [PATCH v5 0/7] iio: accel: add MSA311 accelerometer driver Dmitry Rokosov
                   ` (2 preceding siblings ...)
  2022-08-12 16:52 ` [PATCH v5 4/7] iio: common: scmi_sensors: use HZ macro from units.h Dmitry Rokosov
@ 2022-08-12 16:52 ` Dmitry Rokosov
  2022-08-12 16:52 ` [PATCH v5 5/7] dt-bindings: vendor-prefixes: add MEMSensing Microsystems Co., Ltd Dmitry Rokosov
                   ` (2 subsequent siblings)
  6 siblings, 0 replies; 17+ messages in thread
From: Dmitry Rokosov @ 2022-08-12 16:52 UTC (permalink / raw)
  To: akpm, jic23, robh+dt, andriy.shevchenko, andy.shevchenko,
	christophe.jaillet, stano.jakubek, shawnguo, stephan,
	daniel.lezcano, wsa, lars, Michael.Hennerich, jbhayana,
	lucas.demarchi, jani.nikula, linus.walleij
  Cc: linux-iio, kernel, devicetree, linux-kernel, Dmitry Rokosov

Remove duplicated definition of NHZ_PER_HZ, because it's available in
the units.h as NANOHZ_PER_HZ.

Signed-off-by: Dmitry Rokosov <ddrokosov@sberdevices.ru>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
---
 drivers/iio/accel/adxl345_core.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
index 370bfec1275a..1919e0089c11 100644
--- a/drivers/iio/accel/adxl345_core.c
+++ b/drivers/iio/accel/adxl345_core.c
@@ -10,6 +10,7 @@
 #include <linux/module.h>
 #include <linux/property.h>
 #include <linux/regmap.h>
+#include <linux/units.h>
 
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
@@ -32,7 +33,6 @@
 
 #define ADXL345_BW_RATE			GENMASK(3, 0)
 #define ADXL345_BASE_RATE_NANO_HZ	97656250LL
-#define NHZ_PER_HZ			1000000000LL
 
 #define ADXL345_POWER_CTL_MEASURE	BIT(3)
 #define ADXL345_POWER_CTL_STANDBY	0x00
@@ -139,7 +139,7 @@ static int adxl345_read_raw(struct iio_dev *indio_dev,
 
 		samp_freq_nhz = ADXL345_BASE_RATE_NANO_HZ <<
 				(regval & ADXL345_BW_RATE);
-		*val = div_s64_rem(samp_freq_nhz, NHZ_PER_HZ, val2);
+		*val = div_s64_rem(samp_freq_nhz, NANOHZ_PER_HZ, val2);
 
 		return IIO_VAL_INT_PLUS_NANO;
 	}
@@ -164,7 +164,8 @@ static int adxl345_write_raw(struct iio_dev *indio_dev,
 				    ADXL345_REG_OFS_AXIS(chan->address),
 				    val / 4);
 	case IIO_CHAN_INFO_SAMP_FREQ:
-		n = div_s64(val * NHZ_PER_HZ + val2, ADXL345_BASE_RATE_NANO_HZ);
+		n = div_s64(val * NANOHZ_PER_HZ + val2,
+			    ADXL345_BASE_RATE_NANO_HZ);
 
 		return regmap_update_bits(data->regmap, ADXL345_REG_BW_RATE,
 					  ADXL345_BW_RATE,
-- 
2.36.0

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH v5 5/7] dt-bindings: vendor-prefixes: add MEMSensing Microsystems Co., Ltd.
  2022-08-12 16:52 [PATCH v5 0/7] iio: accel: add MSA311 accelerometer driver Dmitry Rokosov
                   ` (3 preceding siblings ...)
  2022-08-12 16:52 ` [PATCH v5 3/7] iio: accel: adxl345: " Dmitry Rokosov
@ 2022-08-12 16:52 ` Dmitry Rokosov
  2022-08-12 16:52 ` [PATCH v5 6/7] iio: add MEMSensing MSA311 3-axis accelerometer driver Dmitry Rokosov
  2022-08-12 16:52 ` [PATCH v5 7/7] dt-bindings: iio: accel: add dt-binding schema for msa311 accel driver Dmitry Rokosov
  6 siblings, 0 replies; 17+ messages in thread
From: Dmitry Rokosov @ 2022-08-12 16:52 UTC (permalink / raw)
  To: akpm, jic23, robh+dt, andriy.shevchenko, andy.shevchenko,
	christophe.jaillet, stano.jakubek, shawnguo, stephan,
	daniel.lezcano, wsa, lars, Michael.Hennerich, jbhayana,
	lucas.demarchi, jani.nikula, linus.walleij
  Cc: linux-iio, kernel, devicetree, linux-kernel, Dmitry Rokosov, Rob Herring

MEMSensing Microsystems (Suzhou, China) Co., Ltd. operates as a micro
electromechanical system technology company which produces micro
electromechanical system microphones and sensors.
MEMSensing Microsystems (Suzhou, China) Co., Ltd. applies its products
in consumer electronics, industrial control, medical electronics
and automotive, and other fields.

Signed-off-by: Dmitry Rokosov <ddrokosov@sberdevices.ru>
Acked-by: Rob Herring <robh@kernel.org>
---
 Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml
index 0496773a3c4d..404b40eac011 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
+++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
@@ -761,6 +761,8 @@ patternProperties:
     description: MELFAS Inc.
   "^mellanox,.*":
     description: Mellanox Technologies
+  "^memsensing,.*":
+    description: MEMSensing Microsystems Co., Ltd.
   "^memsic,.*":
     description: MEMSIC Inc.
   "^menlo,.*":
-- 
2.36.0

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH v5 6/7] iio: add MEMSensing MSA311 3-axis accelerometer driver
  2022-08-12 16:52 [PATCH v5 0/7] iio: accel: add MSA311 accelerometer driver Dmitry Rokosov
                   ` (4 preceding siblings ...)
  2022-08-12 16:52 ` [PATCH v5 5/7] dt-bindings: vendor-prefixes: add MEMSensing Microsystems Co., Ltd Dmitry Rokosov
@ 2022-08-12 16:52 ` Dmitry Rokosov
  2022-08-12 17:10   ` Dmitry Rokosov
                     ` (2 more replies)
  2022-08-12 16:52 ` [PATCH v5 7/7] dt-bindings: iio: accel: add dt-binding schema for msa311 accel driver Dmitry Rokosov
  6 siblings, 3 replies; 17+ messages in thread
From: Dmitry Rokosov @ 2022-08-12 16:52 UTC (permalink / raw)
  To: akpm, jic23, robh+dt, andriy.shevchenko, andy.shevchenko,
	christophe.jaillet, stano.jakubek, shawnguo, stephan,
	daniel.lezcano, wsa, lars, Michael.Hennerich, jbhayana,
	lucas.demarchi, jani.nikula, linus.walleij
  Cc: linux-iio, kernel, devicetree, linux-kernel, Dmitry Rokosov

MSA311 is a tri-axial, low-g accelerometer with I2C digital output for
sensitivity consumer applications. It has dynamic user-selectable full
scales range of +-2g/+-4g/+-8g/+-16g and allows acceleration measurements
with output data rates from 1Hz to 1000Hz.

Spec: https://cdn-shop.adafruit.com/product-files/5309/MSA311-V1.1-ENG.pdf

This driver supports following MSA311 features:
    - IIO interface
    - Different power modes: NORMAL and SUSPEND (using pm_runtime)
    - ODR (Output Data Rate) selection
    - Scale and samp_freq selection
    - IIO triggered buffer, IIO reg access
    - NEW_DATA interrupt + trigger

Below features to be done:
    - Motion Events: ACTIVE, TAP, ORIENT, FREEFALL
    - Low Power mode

Signed-off-by: Dmitry Rokosov <ddrokosov@sberdevices.ru>
---
 MAINTAINERS                |    6 +
 drivers/iio/accel/Kconfig  |   13 +
 drivers/iio/accel/Makefile |    2 +
 drivers/iio/accel/msa311.c | 1339 ++++++++++++++++++++++++++++++++++++
 4 files changed, 1360 insertions(+)
 create mode 100644 drivers/iio/accel/msa311.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 64379c699903..010e7d854bf7 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12992,6 +12992,12 @@ F:	drivers/mtd/
 F:	include/linux/mtd/
 F:	include/uapi/mtd/
 
+MEMSENSING MICROSYSTEMS MSA311 DRIVER
+M:	Dmitry Rokosov <ddrokosov@sberdevices.ru>
+L:	linux-iio@vger.kernel.org
+S:	Maintained
+F:	drivers/iio/accel/msa311.c
+
 MEN A21 WATCHDOG DRIVER
 M:	Johannes Thumshirn <morbidrsa@gmail.com>
 L:	linux-watchdog@vger.kernel.org
diff --git a/drivers/iio/accel/Kconfig b/drivers/iio/accel/Kconfig
index b53f010f3e40..36a5ddf631ef 100644
--- a/drivers/iio/accel/Kconfig
+++ b/drivers/iio/accel/Kconfig
@@ -539,6 +539,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 4d8792668838..5e45b5fa5ab5 100644
--- a/drivers/iio/accel/Makefile
+++ b/drivers/iio/accel/Makefile
@@ -58,6 +58,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..6b992f7c0745
--- /dev/null
+++ b/drivers/iio/accel/msa311.c
@@ -0,0 +1,1339 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * MEMSensing digital 3-Axis accelerometer
+ *
+ * MSA311 is a tri-axial, low-g accelerometer with I2C digital output for
+ * sensitivity consumer applications. It has dynamic user-selectable full
+ * scales range of +-2g/+-4g/+-8g/+-16g and allows acceleration measurements
+ * with output data rates from 1Hz to 1000Hz.
+ *
+ * MSA311 is available in an ultra small (2mm x 2mm, height 0.95mm) LGA package
+ * and is guaranteed to operate over -40C to +85C.
+ *
+ * This driver supports following MSA311 features:
+ *     - IIO interface
+ *     - Different power modes: NORMAL, SUSPEND
+ *     - ODR (Output Data Rate) selection
+ *     - Scale selection
+ *     - IIO triggered buffer
+ *     - NEW_DATA interrupt + trigger
+ *
+ * Below features to be done:
+ *     - Motion Events: ACTIVE, TAP, ORIENT, FREEFALL
+ *     - Low Power mode
+ *
+ * Copyright (c) 2022, SberDevices. All Rights Reserved.
+ *
+ * Author: Dmitry Rokosov <ddrokosov@sberdevices.ru>
+ */
+
+#include <linux/i2c.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/pm.h>
+#include <linux/pm_runtime.h>
+#include <linux/regmap.h>
+#include <linux/string_helpers.h>
+#include <linux/units.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>
+
+#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 {
+	/* Soft_Reset */
+	F_SOFT_RESET_I2C, F_SOFT_RESET_SPI,
+	/* Motion_Interrupt */
+	F_ORIENT_INT, F_S_TAP_INT, F_D_TAP_INT, F_ACTIVE_INT, F_FREEFALL_INT,
+	/* Data_Interrupt */
+	F_NEW_DATA_INT,
+	/* Tap_Active_Status */
+	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,
+	/* Orientation_Status */
+	F_ORIENT_Z, F_ORIENT_X_Y,
+	/* Range */
+	F_FS,
+	/* ODR */
+	F_X_AXIS_DIS, F_Y_AXIS_DIS, F_Z_AXIS_DIS, F_ODR,
+	/* Power Mode/Bandwidth */
+	F_PWR_MODE, F_LOW_POWER_BW,
+	/* Swap_Polarity */
+	F_X_POLARITY, F_Y_POLARITY, F_Z_POLARITY, F_X_Y_SWAP,
+	/* Int_Set_0 */
+	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,
+	/* Int_Set_1 */
+	F_NEW_DATA_INT_EN, F_FREEFALL_INT_EN,
+	/* Int_Map_0 */
+	F_INT1_ORIENT, F_INT1_S_TAP, F_INT1_D_TAP, F_INT1_ACTIVE,
+	F_INT1_FREEFALL,
+	/* Int_Map_1 */
+	F_INT1_NEW_DATA,
+	/* Int_Config */
+	F_INT1_OD, F_INT1_LVL,
+	/* Int_Latch */
+	F_RESET_INT, F_LATCH_INT,
+	/* Freefall_Hy */
+	F_FREEFALL_MODE, F_FREEFALL_HY,
+	/* Active_Dur */
+	F_ACTIVE_DUR,
+	/* Tap_Dur */
+	F_TAP_QUIET, F_TAP_SHOCK, F_TAP_DUR,
+	/* Tap_Th */
+	F_TAP_TH,
+	/* Orient_Hy */
+	F_ORIENT_HYST, F_ORIENT_BLOCKING, F_ORIENT_MODE,
+	/* Z_Block */
+	F_Z_BLOCKING,
+	/* Offset_compensation */
+	F_MAX_FIELDS,
+};
+
+static const struct reg_field msa311_reg_fields[] = {
+	[F_SOFT_RESET_I2C] = REG_FIELD(MSA311_SOFT_RESET_REG, 2, 2),
+	[F_SOFT_RESET_SPI] = REG_FIELD(MSA311_SOFT_RESET_REG, 5, 5),
+
+	[F_ORIENT_INT] = REG_FIELD(MSA311_MOTION_INT_REG, 6, 6),
+	[F_S_TAP_INT] = REG_FIELD(MSA311_MOTION_INT_REG, 5, 5),
+	[F_D_TAP_INT] = REG_FIELD(MSA311_MOTION_INT_REG, 4, 4),
+	[F_ACTIVE_INT] = REG_FIELD(MSA311_MOTION_INT_REG, 2, 2),
+	[F_FREEFALL_INT] = REG_FIELD(MSA311_MOTION_INT_REG, 0, 0),
+
+	[F_NEW_DATA_INT] = REG_FIELD(MSA311_DATA_INT_REG, 0, 0),
+
+	[F_TAP_SIGN] = REG_FIELD(MSA311_TAP_ACTIVE_STS_REG, 7, 7),
+	[F_TAP_FIRST_X] = REG_FIELD(MSA311_TAP_ACTIVE_STS_REG, 6, 6),
+	[F_TAP_FIRST_Y] = REG_FIELD(MSA311_TAP_ACTIVE_STS_REG, 5, 5),
+	[F_TAP_FIRST_Z] = REG_FIELD(MSA311_TAP_ACTIVE_STS_REG, 4, 4),
+	[F_ACTV_SIGN] = REG_FIELD(MSA311_TAP_ACTIVE_STS_REG, 3, 3),
+	[F_ACTV_FIRST_X] = REG_FIELD(MSA311_TAP_ACTIVE_STS_REG, 2, 2),
+	[F_ACTV_FIRST_Y] = REG_FIELD(MSA311_TAP_ACTIVE_STS_REG, 1, 1),
+	[F_ACTV_FIRST_Z] = REG_FIELD(MSA311_TAP_ACTIVE_STS_REG, 0, 0),
+
+	[F_ORIENT_Z] = REG_FIELD(MSA311_ORIENT_STS_REG, 6, 6),
+	[F_ORIENT_X_Y] = REG_FIELD(MSA311_ORIENT_STS_REG, 4, 5),
+
+	[F_FS] = REG_FIELD(MSA311_RANGE_REG, 0, 1),
+
+	[F_X_AXIS_DIS] = REG_FIELD(MSA311_ODR_REG, 7, 7),
+	[F_Y_AXIS_DIS] = REG_FIELD(MSA311_ODR_REG, 6, 6),
+	[F_Z_AXIS_DIS] = REG_FIELD(MSA311_ODR_REG, 5, 5),
+	[F_ODR] = REG_FIELD(MSA311_ODR_REG, 0, 3),
+
+	[F_PWR_MODE] = REG_FIELD(MSA311_PWR_MODE_REG, 6, 7),
+	[F_LOW_POWER_BW] = REG_FIELD(MSA311_PWR_MODE_REG, 1, 4),
+
+	[F_X_POLARITY] = REG_FIELD(MSA311_SWAP_POLARITY_REG, 3, 3),
+	[F_Y_POLARITY] = REG_FIELD(MSA311_SWAP_POLARITY_REG, 2, 2),
+	[F_Z_POLARITY] = REG_FIELD(MSA311_SWAP_POLARITY_REG, 1, 1),
+	[F_X_Y_SWAP] = REG_FIELD(MSA311_SWAP_POLARITY_REG, 0, 0),
+
+	[F_ORIENT_INT_EN] = REG_FIELD(MSA311_INT_SET_0_REG, 6, 6),
+	[F_S_TAP_INT_EN] = REG_FIELD(MSA311_INT_SET_0_REG, 5, 5),
+	[F_D_TAP_INT_EN] = REG_FIELD(MSA311_INT_SET_0_REG, 4, 4),
+	[F_ACTIVE_INT_EN_Z] = REG_FIELD(MSA311_INT_SET_0_REG, 2, 2),
+	[F_ACTIVE_INT_EN_Y] = REG_FIELD(MSA311_INT_SET_0_REG, 1, 1),
+	[F_ACTIVE_INT_EN_X] = REG_FIELD(MSA311_INT_SET_0_REG, 0, 0),
+
+	[F_NEW_DATA_INT_EN] = REG_FIELD(MSA311_INT_SET_1_REG, 4, 4),
+	[F_FREEFALL_INT_EN] = REG_FIELD(MSA311_INT_SET_1_REG, 3, 3),
+
+	[F_INT1_ORIENT] = REG_FIELD(MSA311_INT_MAP_0_REG, 6, 6),
+	[F_INT1_S_TAP] = REG_FIELD(MSA311_INT_MAP_0_REG, 5, 5),
+	[F_INT1_D_TAP] = REG_FIELD(MSA311_INT_MAP_0_REG, 4, 4),
+	[F_INT1_ACTIVE] = REG_FIELD(MSA311_INT_MAP_0_REG, 2, 2),
+	[F_INT1_FREEFALL] = REG_FIELD(MSA311_INT_MAP_0_REG, 0, 0),
+
+	[F_INT1_NEW_DATA] = REG_FIELD(MSA311_INT_MAP_1_REG, 0, 0),
+
+	[F_INT1_OD] = REG_FIELD(MSA311_INT_CONFIG_REG, 1, 1),
+	[F_INT1_LVL] = REG_FIELD(MSA311_INT_CONFIG_REG, 0, 0),
+
+	[F_RESET_INT] = REG_FIELD(MSA311_INT_LATCH_REG, 7, 7),
+	[F_LATCH_INT] = REG_FIELD(MSA311_INT_LATCH_REG, 0, 3),
+
+	[F_FREEFALL_MODE] = REG_FIELD(MSA311_FREEFALL_HY_REG, 2, 2),
+	[F_FREEFALL_HY] = REG_FIELD(MSA311_FREEFALL_HY_REG, 0, 1),
+
+	[F_ACTIVE_DUR] = REG_FIELD(MSA311_ACTIVE_DUR_REG, 0, 1),
+
+	[F_TAP_QUIET] = REG_FIELD(MSA311_TAP_DUR_REG, 7, 7),
+	[F_TAP_SHOCK] = REG_FIELD(MSA311_TAP_DUR_REG, 6, 6),
+	[F_TAP_DUR] = REG_FIELD(MSA311_TAP_DUR_REG, 0, 2),
+
+	[F_TAP_TH] = REG_FIELD(MSA311_TAP_TH_REG, 0, 4),
+
+	[F_ORIENT_HYST] = REG_FIELD(MSA311_ORIENT_HY_REG, 4, 6),
+	[F_ORIENT_BLOCKING] = REG_FIELD(MSA311_ORIENT_HY_REG, 2, 3),
+	[F_ORIENT_MODE] = REG_FIELD(MSA311_ORIENT_HY_REG, 0, 1),
+
+	[F_Z_BLOCKING] = REG_FIELD(MSA311_Z_BLOCK_REG, 0, 3),
+};
+
+#define MSA311_WHO_AM_I 0x13
+
+/*
+ * Possible Full Scale ranges
+ *
+ * Axis data is 12-bit signed value, so
+ *
+ * fs0 = (2 + 2) * 9.81 / (2^11) = 0.009580
+ * fs1 = (4 + 4) * 9.81 / (2^11) = 0.019160
+ * fs2 = (8 + 8) * 9.81 / (2^11) = 0.038320
+ * fs3 = (16 + 16) * 9.81 / (2^11) = 0.076641
+ */
+enum {
+	MSA311_FS_2G,
+	MSA311_FS_4G,
+	MSA311_FS_8G,
+	MSA311_FS_16G,
+};
+
+static const struct {
+	int val;
+	int val2;
+} msa311_fs_table[] = {
+	{0, 9580}, {0, 19160}, {0, 38320}, {0, 76641}
+};
+
+/* Possible Output Data Rate values */
+enum {
+	MSA311_ODR_1_HZ,
+	MSA311_ODR_1_95_HZ,
+	MSA311_ODR_3_9_HZ,
+	MSA311_ODR_7_81_HZ,
+	MSA311_ODR_15_63_HZ,
+	MSA311_ODR_31_25_HZ,
+	MSA311_ODR_62_5_HZ,
+	MSA311_ODR_125_HZ,
+	MSA311_ODR_250_HZ,
+	MSA311_ODR_500_HZ,
+	MSA311_ODR_1000_HZ,
+};
+
+static const struct {
+	int val;
+	int val2;
+} msa311_odr_table[] = {
+	{1, 0}, {1, 950000}, {3, 900000}, {7, 810000}, {15, 630000},
+	{31, 250000}, {62, 500000}, {125, 0}, {250, 0}, {500, 0}, {1000, 0}
+};
+
+/* All supported power modes */
+#define MSA311_PWR_MODE_NORMAL  0b00
+#define MSA311_PWR_MODE_LOW     0b01
+#define MSA311_PWR_MODE_UNKNOWN 0b10
+#define MSA311_PWR_MODE_SUSPEND 0b11
+static const char * const msa311_pwr_modes[] = {
+	[MSA311_PWR_MODE_NORMAL] = "normal",
+	[MSA311_PWR_MODE_LOW] = "low",
+	[MSA311_PWR_MODE_UNKNOWN] = "unknown",
+	[MSA311_PWR_MODE_SUSPEND] = "suspend",
+};
+
+/* Autosuspend delay */
+#define MSA311_PWR_SLEEP_DELAY_MS 2000
+
+/* Possible INT1 types and levels */
+enum {
+	MSA311_INT1_OD_PUSH_PULL,
+	MSA311_INT1_OD_OPEN_DRAIN,
+};
+
+enum {
+	MSA311_INT1_LVL_LOW,
+	MSA311_INT1_LVL_HIGH,
+};
+
+/* Latch INT modes */
+#define MSA311_LATCH_INT_NOT_LATCHED 0b0000
+#define MSA311_LATCH_INT_250MS       0b0001
+#define MSA311_LATCH_INT_500MS       0b0010
+#define MSA311_LATCH_INT_1S          0b0011
+#define MSA311_LATCH_INT_2S          0b0100
+#define MSA311_LATCH_INT_4S          0b0101
+#define MSA311_LATCH_INT_8S          0b0110
+#define MSA311_LATCH_INT_1MS         0b1010
+#define MSA311_LATCH_INT_2MS         0b1011
+#define MSA311_LATCH_INT_25MS        0b1100
+#define MSA311_LATCH_INT_50MS        0b1101
+#define MSA311_LATCH_INT_100MS       0b1110
+#define MSA311_LATCH_INT_LATCHED     0b0111
+
+static const struct regmap_range msa311_readonly_registers[] = {
+	regmap_reg_range(MSA311_PARTID_REG, MSA311_ORIENT_STS_REG),
+};
+
+static const struct regmap_access_table msa311_writeable_table = {
+	.no_ranges = msa311_readonly_registers,
+	.n_no_ranges = ARRAY_SIZE(msa311_readonly_registers),
+};
+
+static const struct regmap_range msa311_writeonly_registers[] = {
+	regmap_reg_range(MSA311_SOFT_RESET_REG, MSA311_SOFT_RESET_REG),
+};
+
+static const struct regmap_access_table msa311_readable_table = {
+	.no_ranges = msa311_writeonly_registers,
+	.n_no_ranges = ARRAY_SIZE(msa311_writeonly_registers),
+};
+
+static const struct regmap_range msa311_volatile_registers[] = {
+	regmap_reg_range(MSA311_ACC_X_REG, MSA311_ORIENT_STS_REG),
+};
+
+static const struct regmap_access_table msa311_volatile_table = {
+	.yes_ranges = msa311_volatile_registers,
+	.n_yes_ranges = ARRAY_SIZE(msa311_volatile_registers),
+};
+
+static const struct regmap_config msa311_regmap_config = {
+	.name = "msa311",
+	.reg_bits = 8,
+	.val_bits = 8,
+	.max_register = MSA311_OFFSET_Z_REG,
+	.wr_table = &msa311_writeable_table,
+	.rd_table = &msa311_readable_table,
+	.volatile_table = &msa311_volatile_table,
+	.cache_type = REGCACHE_RBTREE,
+};
+
+#define MSA311_GENMASK(field) ({                \
+	typeof(&(msa311_reg_fields)[0]) _field; \
+	_field = &msa311_reg_fields[(field)];   \
+	GENMASK(_field->msb, _field->lsb);      \
+})
+
+/**
+ * struct msa311_priv - MSA311 internal private state
+ * @regs: Underlying I2C bus adapter used to abstract slave
+ *        register accesses
+ * @fields: Abstract objects for each registers fields access
+ * @dev: Device handler associated with appropriate bus client
+ * @lock: Protects msa311 device state between setup and data access routines
+ *        (power transitions, samp_freq/scale tune, retrieving axes data, etc)
+ * @chip_name: Chip name in the format "msa311-%hhx" % partid
+ * @new_data_trig: Optional NEW_DATA interrupt driven trigger used
+ *                 to notify external consumers a new sample is ready
+ * @vdd: Optional external voltage regulator for the device power supply
+ */
+struct msa311_priv {
+	struct regmap *regs;
+	struct regmap_field *fields[F_MAX_FIELDS];
+
+	struct device *dev;
+	struct mutex lock;
+	char chip_name[10];
+
+	struct iio_trigger *new_data_trig;
+	struct regulator *vdd;
+};
+
+enum msa311_si {
+	MSA311_SI_X,
+	MSA311_SI_Y,
+	MSA311_SI_Z,
+	MSA311_SI_TIMESTAMP,
+};
+
+#define MSA311_ACCEL_CHANNEL(axis) {                                        \
+	.type = IIO_ACCEL,                                                  \
+	.modified = 1,                                                      \
+	.channel2 = IIO_MOD_##axis,                                         \
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),                       \
+	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) |              \
+				    BIT(IIO_CHAN_INFO_SAMP_FREQ),           \
+	.info_mask_shared_by_type_available = BIT(IIO_CHAN_INFO_SCALE) |    \
+					      BIT(IIO_CHAN_INFO_SAMP_FREQ), \
+	.scan_index = MSA311_SI_##axis,                                     \
+	.scan_type = {                                                      \
+		.sign = 's',                                                \
+		.realbits = 12,                                             \
+		.storagebits = 16,                                          \
+		.shift = 4,                                                 \
+		.endianness = IIO_LE,                                       \
+	},                                                                  \
+	.datasheet_name = "ACC_"#axis,                                      \
+}
+
+static const struct iio_chan_spec msa311_channels[] = {
+	MSA311_ACCEL_CHANNEL(X),
+	MSA311_ACCEL_CHANNEL(Y),
+	MSA311_ACCEL_CHANNEL(Z),
+	IIO_CHAN_SOFT_TIMESTAMP(MSA311_SI_TIMESTAMP),
+};
+
+/**
+ * msa311_get_odr() - Read Output Data Rate (ODR) value from MSA311 accel
+ * @msa311: MSA311 internal private state
+ * @odr: output ODR value
+ *
+ * This function should be called under msa311->lock.
+ *
+ * Return: 0 on success, -ERRNO in other failures
+ */
+static inline int msa311_get_odr(struct msa311_priv *msa311, unsigned int *odr)
+{
+	int err;
+
+	err = regmap_field_read(msa311->fields[F_ODR], odr);
+	if (err)
+		return err;
+
+	/*
+	 * Filter the same 1000Hz ODR register values based on datasheet info.
+	 * ODR can be equal to 1010-1111 for 1000Hz, but function returns 1010
+	 * all the time.
+	 */
+	if (*odr > MSA311_ODR_1000_HZ)
+		*odr = MSA311_ODR_1000_HZ;
+
+	return 0;
+}
+
+/**
+ * msa311_set_odr() - Setup Output Data Rate (ODR) value for MSA311 accel
+ * @msa311: MSA311 internal private state
+ * @odr: requested ODR value
+ *
+ * This function should be called under msa311->lock. Possible ODR values:
+ *     - 1Hz (not available in normal mode)
+ *     - 1.95Hz (not available in normal mode)
+ *     - 3.9Hz
+ *     - 7.81Hz
+ *     - 15.63Hz
+ *     - 31.25Hz
+ *     - 62.5Hz
+ *     - 125Hz
+ *     - 250Hz
+ *     - 500Hz
+ *     - 1000Hz
+ *
+ * Return: 0 on success, -EINVAL for bad ODR value in the certain power mode,
+ *         -ERRNO in other failures
+ */
+static inline int msa311_set_odr(struct msa311_priv *msa311, unsigned int odr)
+{
+	struct device *dev = msa311->dev;
+	unsigned int pwr_mode;
+	bool good_odr = false;
+	int err;
+
+	err = regmap_field_read(msa311->fields[F_PWR_MODE], &pwr_mode);
+	if (err)
+		return err;
+
+	/* Filter bad ODR values */
+	if (pwr_mode == MSA311_PWR_MODE_NORMAL)
+		good_odr = (odr > MSA311_ODR_1_95_HZ);
+
+	if (!good_odr) {
+		dev_err(dev,
+			"cannot set odr %u.%luHz, not available in %s mode\n",
+			msa311_odr_table[odr].val,
+			msa311_odr_table[odr].val2 / MILLIHZ_PER_HZ,
+			msa311_pwr_modes[pwr_mode]);
+		return -EINVAL;
+	}
+
+	return regmap_field_write(msa311->fields[F_ODR], odr);
+}
+
+/**
+ * msa311_wait_for_next_data() - Wait next accel data available after resume
+ * @msa311: MSA311 internal private state
+ *
+ * Return: 0 on success, -EINTR if msleep() was interrupted,
+ *         -ERRNO in other failures
+ */
+static int msa311_wait_for_next_data(struct msa311_priv *msa311)
+{
+	static const unsigned int unintr_thresh_ms = 20;
+	struct device *dev = msa311->dev;
+	unsigned long freq_uhz;
+	unsigned long wait_ms;
+	unsigned int odr;
+	int err;
+
+	err = msa311_get_odr(msa311, &odr);
+	if (err) {
+		dev_err(dev, "cannot get actual frequency (%pe)\n",
+			ERR_PTR(err));
+		return err;
+	}
+
+	/*
+	 * After msa311 resuming is done, we need to wait for data
+	 * to be refreshed by accel logic.
+	 * A certain timeout is calculated based on the current ODR value.
+	 * If requested timeout isn't so long (let's assume 20ms),
+	 * we can wait for next data in uninterruptible sleep.
+	 */
+	freq_uhz = msa311_odr_table[odr].val * MICROHZ_PER_HZ +
+		   msa311_odr_table[odr].val2;
+	wait_ms = (MICROHZ_PER_HZ / freq_uhz) * MSEC_PER_SEC;
+
+	if (wait_ms < unintr_thresh_ms)
+		usleep_range(wait_ms * USEC_PER_MSEC,
+			     unintr_thresh_ms * USEC_PER_MSEC);
+	else
+		return msleep_interruptible(wait_ms) ? -EINTR : 0;
+
+	return 0;
+}
+
+/**
+ * msa311_set_pwr_mode() - Install certain MSA311 power mode
+ * @msa311: MSA311 internal private state
+ * @mode: Power mode can be equal to NORMAL or SUSPEND
+ *
+ * This function should be called under msa311->lock.
+ *
+ * Return: 0 on success, -ERRNO on failure
+ */
+static int msa311_set_pwr_mode(struct msa311_priv *msa311, unsigned int mode)
+{
+	struct device *dev = msa311->dev;
+	unsigned int prev_mode;
+	int err;
+
+	if (mode >= ARRAY_SIZE(msa311_pwr_modes))
+		return -EINVAL;
+
+	dev_dbg(dev, "transition to %s mode\n", msa311_pwr_modes[mode]);
+
+	err = regmap_field_read(msa311->fields[F_PWR_MODE], &prev_mode);
+	if (err)
+		return err;
+
+	err = regmap_field_write(msa311->fields[F_PWR_MODE], mode);
+	if (err)
+		return err;
+
+	/* Wait actual data if we wake up */
+	if (prev_mode == MSA311_PWR_MODE_SUSPEND &&
+	    mode == MSA311_PWR_MODE_NORMAL)
+		return msa311_wait_for_next_data(msa311);
+
+	return 0;
+}
+
+/**
+ * msa311_get_axis() - Read MSA311 accel data for certain IIO channel axis spec
+ * @msa311: MSA311 internal private state
+ * @chan: IIO channel specification
+ * @axis: Output accel axis data for requested IIO channel spec
+ *
+ * This function should be called under msa311->lock.
+ *
+ * Return: 0 on success, -EINVAL for unknown IIO channel specification,
+ *         -ERRNO in other failures
+ */
+static int msa311_get_axis(struct msa311_priv *msa311,
+			   const struct iio_chan_spec * const chan,
+			   __le16 *axis)
+{
+	struct device *dev = msa311->dev;
+	unsigned int axis_reg;
+
+	if (chan->scan_index < MSA311_SI_X || chan->scan_index > MSA311_SI_Z) {
+		dev_err(dev, "invalid scan_index value [%d]\n",
+			chan->scan_index);
+		return -EINVAL;
+	}
+
+	/* Axes data layout has 2 byte gap for each axis starting from X axis */
+	axis_reg = MSA311_ACC_X_REG + (chan->scan_index << 1);
+
+	return regmap_bulk_read(msa311->regs, axis_reg, axis, sizeof(*axis));
+}
+
+static int msa311_read_raw_data(struct iio_dev *indio_dev,
+				struct iio_chan_spec const *chan,
+				int *val, int *val2)
+{
+	struct msa311_priv *msa311 = iio_priv(indio_dev);
+	struct device *dev = msa311->dev;
+	__le16 axis;
+	int err;
+
+	err = pm_runtime_resume_and_get(dev);
+	if (err)
+		return err;
+
+	err = iio_device_claim_direct_mode(indio_dev);
+	if (err)
+		return err;
+
+	mutex_lock(&msa311->lock);
+	err = msa311_get_axis(msa311, chan, &axis);
+	mutex_unlock(&msa311->lock);
+
+	iio_device_release_direct_mode(indio_dev);
+
+	if (err) {
+		dev_err(dev, "cannot get axis %s (%pe)\n",
+			chan->datasheet_name, ERR_PTR(err));
+		return err;
+	}
+
+	pm_runtime_mark_last_busy(dev);
+	pm_runtime_put_autosuspend(dev);
+
+	/*
+	 * Axis data format is:
+	 * ACC_X = (ACC_X_MSB[7:0] << 4) | ACC_X_LSB[7:4]
+	 */
+	*val = sign_extend32(le16_to_cpu(axis) >> chan->scan_type.shift,
+			     chan->scan_type.realbits - 1);
+
+	return IIO_VAL_INT;
+}
+
+static int msa311_read_scale(struct iio_dev *indio_dev, int *val, int *val2)
+{
+	struct msa311_priv *msa311 = iio_priv(indio_dev);
+	struct device *dev = msa311->dev;
+	unsigned int fs;
+	int err;
+
+	mutex_lock(&msa311->lock);
+	err = regmap_field_read(msa311->fields[F_FS], &fs);
+	mutex_unlock(&msa311->lock);
+	if (err) {
+		dev_err(dev, "cannot get actual scale (%pe)\n", ERR_PTR(err));
+		return err;
+	}
+
+	*val = msa311_fs_table[fs].val;
+	*val2 = msa311_fs_table[fs].val2;
+
+	return IIO_VAL_INT_PLUS_MICRO;
+}
+
+static int msa311_read_samp_freq(struct iio_dev *indio_dev,
+				 int *val, int *val2)
+{
+	struct msa311_priv *msa311 = iio_priv(indio_dev);
+	struct device *dev = msa311->dev;
+	unsigned int odr;
+	int err;
+
+	mutex_lock(&msa311->lock);
+	err = msa311_get_odr(msa311, &odr);
+	mutex_unlock(&msa311->lock);
+	if (err) {
+		dev_err(dev, "cannot get actual frequency (%pe)\n",
+			ERR_PTR(err));
+		return err;
+	}
+
+	*val = msa311_odr_table[odr].val;
+	*val2 = msa311_odr_table[odr].val2;
+
+	return IIO_VAL_INT_PLUS_MICRO;
+}
+
+static int msa311_read_raw(struct iio_dev *indio_dev,
+			   struct iio_chan_spec const *chan,
+			   int *val, int *val2, long mask)
+{
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		return msa311_read_raw_data(indio_dev, chan, val, val2);
+
+	case IIO_CHAN_INFO_SCALE:
+		return msa311_read_scale(indio_dev, val, val2);
+
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		return msa311_read_samp_freq(indio_dev, val, val2);
+
+	default:
+		return -EINVAL;
+	}
+}
+
+static int msa311_read_avail(struct iio_dev *indio_dev,
+			     struct iio_chan_spec const *chan,
+			     const int **vals, int *type,
+			     int *length, long mask)
+{
+	switch (mask) {
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		*vals = (int *)msa311_odr_table;
+		*type = IIO_VAL_INT_PLUS_MICRO;
+		/* ODR value has 2 ints (integer and fractional parts) */
+		*length = ARRAY_SIZE(msa311_odr_table) * 2;
+		return IIO_AVAIL_LIST;
+
+	case IIO_CHAN_INFO_SCALE:
+		*vals = (int *)msa311_fs_table;
+		*type = IIO_VAL_INT_PLUS_MICRO;
+		/* FS value has 2 ints (integer and fractional parts) */
+		*length = ARRAY_SIZE(msa311_fs_table) * 2;
+		return IIO_AVAIL_LIST;
+
+	default:
+		return -EINVAL;
+	}
+}
+
+static int msa311_write_scale(struct iio_dev *indio_dev, int val, int val2)
+{
+	struct msa311_priv *msa311 = iio_priv(indio_dev);
+	struct device *dev = msa311->dev;
+	unsigned int fs;
+	int err;
+
+	/* We do not have fs >= 1, so skip such values */
+	if (val)
+		return 0;
+
+	err = pm_runtime_resume_and_get(dev);
+	if (err)
+		return err;
+
+	err = -EINVAL;
+	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) {
+			mutex_lock(&msa311->lock);
+			err = regmap_field_write(msa311->fields[F_FS], fs);
+			mutex_unlock(&msa311->lock);
+			break;
+		}
+
+	pm_runtime_mark_last_busy(dev);
+	pm_runtime_put_autosuspend(dev);
+
+	if (err)
+		dev_err(dev, "cannot update scale (%pe)\n", ERR_PTR(err));
+
+	return err;
+}
+
+static int msa311_write_samp_freq(struct iio_dev *indio_dev, int val, int val2)
+{
+	struct msa311_priv *msa311 = iio_priv(indio_dev);
+	struct device *dev = msa311->dev;
+	unsigned int odr;
+	int err;
+
+	err = pm_runtime_resume_and_get(dev);
+	if (err)
+		return err;
+
+	/*
+	 * Sampling frequency changing is prohibited when buffer mode is
+	 * enabled, because sometimes MSA311 chip returns outliers during
+	 * frequency values growing up in the read operation moment.
+	 */
+	err = iio_device_claim_direct_mode(indio_dev);
+	if (err)
+		return err;
+
+	err = -EINVAL;
+	for (odr = 0; odr < ARRAY_SIZE(msa311_odr_table); odr++)
+		if (val == msa311_odr_table[odr].val &&
+		    val2 == msa311_odr_table[odr].val2) {
+			mutex_lock(&msa311->lock);
+			err = msa311_set_odr(msa311, odr);
+			mutex_unlock(&msa311->lock);
+			break;
+		}
+
+	iio_device_release_direct_mode(indio_dev);
+
+	pm_runtime_mark_last_busy(dev);
+	pm_runtime_put_autosuspend(dev);
+
+	if (err)
+		dev_err(dev, "cannot update frequency (%pe)\n", ERR_PTR(err));
+
+	return err;
+}
+
+static int msa311_write_raw(struct iio_dev *indio_dev,
+			    struct iio_chan_spec const *chan,
+			    int val, int val2, long mask)
+{
+	switch (mask) {
+	case IIO_CHAN_INFO_SCALE:
+		return msa311_write_scale(indio_dev, val, val2);
+
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		return msa311_write_samp_freq(indio_dev, val, val2);
+
+	default:
+		return -EINVAL;
+	}
+}
+
+static int msa311_debugfs_reg_access(struct iio_dev *indio_dev,
+				     unsigned int reg, unsigned int writeval,
+				     unsigned int *readval)
+{
+	struct msa311_priv *msa311 = iio_priv(indio_dev);
+	struct device *dev = msa311->dev;
+	int err;
+
+	if (reg > regmap_get_max_register(msa311->regs))
+		return -EINVAL;
+
+	err = pm_runtime_resume_and_get(dev);
+	if (err)
+		return err;
+
+	mutex_lock(&msa311->lock);
+
+	if (readval)
+		err = regmap_read(msa311->regs, reg, readval);
+	else
+		err = regmap_write(msa311->regs, reg, writeval);
+
+	mutex_unlock(&msa311->lock);
+
+	pm_runtime_mark_last_busy(dev);
+	pm_runtime_put_autosuspend(dev);
+
+	if (err)
+		dev_err(dev, "cannot %s register %u from debugfs (%pe)\n",
+			str_read_write(readval), reg, ERR_PTR(err));
+
+	return err;
+}
+
+static int msa311_buffer_preenable(struct iio_dev *indio_dev)
+{
+	struct msa311_priv *msa311 = iio_priv(indio_dev);
+	struct device *dev = msa311->dev;
+
+	return pm_runtime_resume_and_get(dev);
+}
+
+static int msa311_buffer_postdisable(struct iio_dev *indio_dev)
+{
+	struct msa311_priv *msa311 = iio_priv(indio_dev);
+	struct device *dev = msa311->dev;
+
+	pm_runtime_mark_last_busy(dev);
+	pm_runtime_put_autosuspend(dev);
+
+	return 0;
+}
+
+static int msa311_set_new_data_trig_state(struct iio_trigger *trig, bool state)
+{
+	struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
+	struct msa311_priv *msa311 = iio_priv(indio_dev);
+	struct device *dev = msa311->dev;
+	int err;
+
+	mutex_lock(&msa311->lock);
+	err = regmap_field_write(msa311->fields[F_NEW_DATA_INT_EN], state);
+	mutex_unlock(&msa311->lock);
+	if (err)
+		dev_err(dev,
+			"cannot %s buffer due to new_data_int failure (%pe)\n",
+			str_enable_disable(state), ERR_PTR(err));
+
+	return err;
+}
+
+static int msa311_validate_device(struct iio_trigger *trig,
+				  struct iio_dev *indio_dev)
+{
+	return iio_trigger_get_drvdata(trig) == indio_dev ? 0 : -EINVAL;
+}
+
+static irqreturn_t msa311_buffer_thread(int irq, void *p)
+{
+	struct iio_poll_func *pf = p;
+	struct msa311_priv *msa311 = iio_priv(pf->indio_dev);
+	struct iio_dev *indio_dev = pf->indio_dev;
+	const struct iio_chan_spec *chan;
+	struct device *dev = msa311->dev;
+	int bit, err, i = 0;
+	__le16 axis;
+	struct {
+		__le16 channels[MSA311_SI_Z + 1];
+		s64 ts __aligned(8);
+	} buf;
+
+	memset(&buf, 0, sizeof(buf));
+
+	mutex_lock(&msa311->lock);
+
+	for_each_set_bit(bit, indio_dev->active_scan_mask,
+			 indio_dev->masklength) {
+		chan = &msa311_channels[bit];
+
+		err = msa311_get_axis(msa311, chan, &axis);
+		if (err) {
+			mutex_unlock(&msa311->lock);
+			dev_err(dev, "cannot get axis %s (%pe)\n",
+				chan->datasheet_name, ERR_PTR(err));
+			goto err;
+		}
+
+		buf.channels[i++] = axis;
+	}
+
+	mutex_unlock(&msa311->lock);
+
+	iio_push_to_buffers_with_timestamp(indio_dev, &buf,
+					   iio_get_time_ns(indio_dev));
+
+err:
+	iio_trigger_notify_done(indio_dev->trig);
+
+	return IRQ_HANDLED;
+}
+
+static irqreturn_t msa311_irq_thread(int irq, void *p)
+{
+	struct msa311_priv *msa311 = iio_priv(p);
+	unsigned int new_data_int_enabled;
+	struct device *dev = msa311->dev;
+	int err;
+
+	mutex_lock(&msa311->lock);
+
+	/*
+	 * We do not check NEW_DATA int status, because based on the
+	 * 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);
+
+	mutex_unlock(&msa311->lock);
+	if (err) {
+		dev_err(dev, "cannot read new_data interrupt state (%pe)\n",
+			ERR_PTR(err));
+		return IRQ_NONE;
+	}
+
+	if (new_data_int_enabled)
+		iio_trigger_poll_chained(msa311->new_data_trig);
+
+	return IRQ_HANDLED;
+}
+
+static const struct iio_info msa311_info = {
+	.read_raw = msa311_read_raw,
+	.read_avail = msa311_read_avail,
+	.write_raw = msa311_write_raw,
+	.debugfs_reg_access = msa311_debugfs_reg_access,
+};
+
+static const struct iio_buffer_setup_ops msa311_buffer_setup_ops = {
+	.preenable = msa311_buffer_preenable,
+	.postdisable = msa311_buffer_postdisable,
+};
+
+static const struct iio_trigger_ops msa311_new_data_trig_ops = {
+	.set_trigger_state = msa311_set_new_data_trig_state,
+	.validate_device = msa311_validate_device,
+};
+
+static int msa311_check_partid(struct msa311_priv *msa311)
+{
+	struct device *dev = msa311->dev;
+	unsigned int partid;
+	int used, err;
+
+	err = regmap_read(msa311->regs, MSA311_PARTID_REG, &partid);
+	if (err)
+		return dev_err_probe(dev, err, "failed to read partid\n");
+
+	if (partid != MSA311_WHO_AM_I)
+		dev_warn(dev, "invalid partid (%#x), expected (%#x)\n",
+			 partid, MSA311_WHO_AM_I);
+
+	used = scnprintf(msa311->chip_name, sizeof(msa311->chip_name),
+			 "msa311-%hhx", partid);
+	msa311->chip_name[used] = '\0';
+
+	return 0;
+}
+
+static int msa311_soft_reset(struct msa311_priv *msa311)
+{
+	struct device *dev = msa311->dev;
+	int err;
+
+	err = regmap_write(msa311->regs, MSA311_SOFT_RESET_REG,
+			   MSA311_GENMASK(F_SOFT_RESET_I2C) |
+			   MSA311_GENMASK(F_SOFT_RESET_SPI));
+	if (err)
+		return dev_err_probe(dev, err, "cannot soft reset all logic\n");
+
+	return 0;
+}
+
+static int msa311_chip_init(struct msa311_priv *msa311)
+{
+	struct device *dev = msa311->dev;
+	int err;
+
+	err = regmap_write(msa311->regs, MSA311_RANGE_REG, MSA311_FS_16G);
+	if (err)
+		return dev_err_probe(dev, err, "failed to setup accel range\n");
+
+	/* Disable all interrupts by default */
+	err = regmap_write(msa311->regs, MSA311_INT_SET_0_REG, 0);
+	if (err)
+		return dev_err_probe(dev, err,
+				     "cannot disable set0 interrupts\n");
+
+	err = regmap_write(msa311->regs, MSA311_INT_SET_1_REG, 0);
+	if (err)
+		return dev_err_probe(dev, err,
+				     "cannot disable set1 interrupts\n");
+
+	/* Unmap all INT1 interrupts by default */
+	err = regmap_write(msa311->regs, MSA311_INT_MAP_0_REG, 0);
+	if (err)
+		return dev_err_probe(dev, err,
+				     "failed to unmap map0 interrupts\n");
+
+	err = regmap_write(msa311->regs, MSA311_INT_MAP_1_REG, 0);
+	if (err)
+		return dev_err_probe(dev, err,
+				     "failed to unmap map1 interrupts\n");
+
+	/* Disable all axes by default */
+	err = regmap_update_bits(msa311->regs, MSA311_ODR_REG,
+				 MSA311_GENMASK(F_X_AXIS_DIS) |
+				 MSA311_GENMASK(F_Y_AXIS_DIS) |
+				 MSA311_GENMASK(F_Z_AXIS_DIS), 0);
+	if (err)
+		return dev_err_probe(dev, err, "cannot enable all axes\n");
+
+	err = msa311_set_odr(msa311, MSA311_ODR_125_HZ);
+	if (err)
+		return dev_err_probe(dev, err,
+				     "failed to set accel frequency\n");
+
+	return 0;
+}
+
+static int msa311_setup_interrupts(struct msa311_priv *msa311)
+{
+	struct device *dev = msa311->dev;
+	struct i2c_client *i2c = to_i2c_client(dev);
+	struct iio_dev *indio_dev = i2c_get_clientdata(i2c);
+	struct iio_trigger *trig;
+	int err;
+
+	/* Keep going without interrupts if no initialized I2C irq */
+	if (i2c->irq <= 0)
+		return 0;
+
+	err = devm_request_threaded_irq(&i2c->dev, i2c->irq,
+					NULL, msa311_irq_thread,
+					IRQF_ONESHOT,
+					msa311->chip_name,
+					indio_dev);
+	if (err)
+		return dev_err_probe(dev, err, "failed to request IRQ\n");
+
+	trig = devm_iio_trigger_alloc(dev, "%s-new-data", msa311->chip_name);
+	if (!trig)
+		return dev_err_probe(dev, -ENOMEM,
+				     "cannot allocate newdata trigger\n");
+
+	msa311->new_data_trig = trig;
+	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 trigger\n");
+
+	err = regmap_field_write(msa311->fields[F_INT1_OD],
+				 MSA311_INT1_OD_PUSH_PULL);
+	if (err)
+		return dev_err_probe(dev, err,
+				     "cannot enable push-pull interrupt\n");
+
+	err = regmap_field_write(msa311->fields[F_INT1_LVL],
+				 MSA311_INT1_LVL_HIGH);
+	if (err)
+		return dev_err_probe(dev, err,
+				     "cannot set active interrupt level\n");
+
+	err = regmap_field_write(msa311->fields[F_LATCH_INT],
+				 MSA311_LATCH_INT_LATCHED);
+	if (err)
+		return dev_err_probe(dev, err,
+				     "cannot latch interrupt\n");
+
+	err = regmap_field_write(msa311->fields[F_RESET_INT], 1);
+	if (err)
+		return dev_err_probe(dev, err,
+				     "cannot reset interrupt\n");
+
+	err = regmap_field_write(msa311->fields[F_INT1_NEW_DATA], 1);
+	if (err)
+		return dev_err_probe(dev, err,
+				     "cannot map new data interrupt\n");
+
+	return 0;
+}
+
+static int msa311_regmap_init(struct msa311_priv *msa311)
+{
+	struct regmap_field **fields = msa311->fields;
+	struct device *dev = msa311->dev;
+	struct i2c_client *i2c = to_i2c_client(dev);
+	struct regmap *regmap;
+	int i;
+
+	regmap = devm_regmap_init_i2c(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 field[%d]\n", i);
+	}
+
+	return 0;
+}
+
+static void msa311_powerdown(void *msa311)
+{
+	msa311_set_pwr_mode(msa311, MSA311_PWR_MODE_SUSPEND);
+}
+
+static void msa311_vdd_disable(void *vdd)
+{
+	regulator_disable(vdd);
+}
+
+static int msa311_probe(struct i2c_client *i2c)
+{
+	struct device *dev = &i2c->dev;
+	struct msa311_priv *msa311;
+	struct iio_dev *indio_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->dev = dev;
+	i2c_set_clientdata(i2c, indio_dev);
+
+	err = msa311_regmap_init(msa311);
+	if (err)
+		return err;
+
+	mutex_init(&msa311->lock);
+
+	msa311->vdd = devm_regulator_get_optional(dev, "vdd");
+	if (IS_ERR(msa311->vdd)) {
+		err = PTR_ERR(msa311->vdd);
+		if (err == -ENODEV)
+			msa311->vdd = NULL;
+		else
+			return dev_err_probe(dev, PTR_ERR(msa311->vdd),
+					     "cannot get vdd supply\n");
+	}
+
+	if (msa311->vdd) {
+		err = regulator_enable(msa311->vdd);
+		if (err)
+			return dev_err_probe(dev, err,
+					     "cannot enable vdd supply\n");
+
+		err = devm_add_action_or_reset(dev, msa311_vdd_disable,
+					       msa311->vdd);
+		if (err) {
+			return dev_err_probe(dev, err,
+					     "cannot add vdd disable action\n");
+		}
+	}
+
+	err = msa311_check_partid(msa311);
+	if (err)
+		return err;
+
+	err = msa311_soft_reset(msa311);
+	if (err)
+		return err;
+
+	err = msa311_set_pwr_mode(msa311, MSA311_PWR_MODE_NORMAL);
+	if (err)
+		return dev_err_probe(dev, err, "failed to power on device\n");
+
+	/*
+	 * Register powerdown deferred callback which suspends the chip
+	 * after module unloaded.
+	 *
+	 * MSA311 should be in SUSPEND mode in the two cases:
+	 * 1) When driver is loaded, but we do not have any data or
+	 *    configuration requests to it (we are solving it using
+	 *    autosuspend feature).
+	 * 2) When driver is unloaded and device is not used (devm action is
+	 *    used in this case).
+	 */
+	err = devm_add_action_or_reset(dev, msa311_powerdown, msa311);
+	if (err)
+		return dev_err_probe(dev, err, "cannot add powerdown action\n");
+
+	err = pm_runtime_set_active(dev);
+	if (err)
+		return err;
+
+	err = devm_pm_runtime_enable(dev);
+	if (err)
+		return err;
+
+	pm_runtime_get_noresume(dev);
+	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;
+	indio_dev->channels = msa311_channels;
+	indio_dev->num_channels = ARRAY_SIZE(msa311_channels);
+	indio_dev->name = msa311->chip_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 trigger buffer\n");
+
+	err = msa311_setup_interrupts(msa311);
+	if (err)
+		return err;
+
+	pm_runtime_mark_last_busy(dev);
+	pm_runtime_put_autosuspend(dev);
+
+	err = devm_iio_device_register(dev, indio_dev);
+	if (err)
+		return dev_err_probe(dev, err, "IIO device register failed\n");
+
+	return 0;
+}
+
+static int msa311_runtime_suspend(struct device *dev)
+{
+	struct iio_dev *indio_dev = dev_get_drvdata(dev);
+	struct msa311_priv *msa311 = iio_priv(indio_dev);
+	int err;
+
+	mutex_lock(&msa311->lock);
+	err = msa311_set_pwr_mode(msa311, MSA311_PWR_MODE_SUSPEND);
+	mutex_unlock(&msa311->lock);
+	if (err)
+		dev_err(dev, "failed to power off device (%pe)\n",
+			ERR_PTR(err));
+
+	return err;
+}
+
+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;
+
+	mutex_lock(&msa311->lock);
+	err = msa311_set_pwr_mode(msa311, MSA311_PWR_MODE_NORMAL);
+	mutex_unlock(&msa311->lock);
+	if (err)
+		dev_err(dev, "failed to power on device (%pe)\n",
+			ERR_PTR(err));
+
+	return err;
+}
+
+static DEFINE_RUNTIME_DEV_PM_OPS(msa311_pm_ops, msa311_runtime_suspend,
+				 msa311_runtime_resume, NULL);
+
+static const struct i2c_device_id msa311_i2c_id[] = {
+	{ .name = "msa311" },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, msa311_i2c_id);
+
+static const struct of_device_id msa311_of_match[] = {
+	{ .compatible = "memsensing,msa311" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, msa311_of_match);
+
+static struct i2c_driver msa311_driver = {
+	.driver = {
+		.name = "msa311",
+		.owner = THIS_MODULE,
+		.of_match_table = msa311_of_match,
+		.pm = pm_ptr(&msa311_pm_ops),
+	},
+	.probe_new	= msa311_probe,
+	.id_table	= msa311_i2c_id,
+};
+module_i2c_driver(msa311_driver);
+
+MODULE_AUTHOR("Dmitry Rokosov <ddrokosov@sberdevices.ru>");
+MODULE_DESCRIPTION("MEMSensing MSA311 3-axis accelerometer driver");
+MODULE_LICENSE("GPL");
-- 
2.36.0

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH v5 7/7] dt-bindings: iio: accel: add dt-binding schema for msa311 accel driver
  2022-08-12 16:52 [PATCH v5 0/7] iio: accel: add MSA311 accelerometer driver Dmitry Rokosov
                   ` (5 preceding siblings ...)
  2022-08-12 16:52 ` [PATCH v5 6/7] iio: add MEMSensing MSA311 3-axis accelerometer driver Dmitry Rokosov
@ 2022-08-12 16:52 ` Dmitry Rokosov
  6 siblings, 0 replies; 17+ messages in thread
From: Dmitry Rokosov @ 2022-08-12 16:52 UTC (permalink / raw)
  To: akpm, jic23, robh+dt, andriy.shevchenko, andy.shevchenko,
	christophe.jaillet, stano.jakubek, shawnguo, stephan,
	daniel.lezcano, wsa, lars, Michael.Hennerich, jbhayana,
	lucas.demarchi, jani.nikula, linus.walleij
  Cc: linux-iio, kernel, devicetree, linux-kernel, Dmitry Rokosov, Rob Herring

Introduce devicetree binding json-schema for MSA311 tri-axial,
low-g accelerometer driver.

Signed-off-by: Dmitry Rokosov <ddrokosov@sberdevices.ru>
Reviewed-by: Rob Herring <robh@kernel.org>
---
 .../bindings/iio/accel/memsensing,msa311.yaml | 53 +++++++++++++++++++
 MAINTAINERS                                   |  1 +
 2 files changed, 54 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/accel/memsensing,msa311.yaml

diff --git a/Documentation/devicetree/bindings/iio/accel/memsensing,msa311.yaml b/Documentation/devicetree/bindings/iio/accel/memsensing,msa311.yaml
new file mode 100644
index 000000000000..23528dcaa073
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/accel/memsensing,msa311.yaml
@@ -0,0 +1,53 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+
+%YAML 1.2
+---
+$id: "http://devicetree.org/schemas/iio/accel/memsensing,msa311.yaml#"
+$schema: "http://devicetree.org/meta-schemas/core.yaml#"
+
+title: MEMSensing digital 3-Axis accelerometer
+
+maintainers:
+  - Dmitry Rokosov <ddrokosov@sberdevices.ru>
+
+description: |
+  MSA311 is a tri-axial, low-g accelerometer with I2C digital output for
+  sensitivity consumer applications. It has dynamical user selectable full
+  scales range of +-2g/+-4g/+-8g/+-16g and allows acceleration measurements
+  with output data rates from 1Hz to 1000Hz.
+  Datasheet can be found at following URL
+  https://cdn-shop.adafruit.com/product-files/5309/MSA311-V1.1-ENG.pdf
+
+properties:
+  compatible:
+    const: memsensing,msa311
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  vdd-supply: true
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        accelerometer@62 {
+            compatible = "memsensing,msa311";
+            reg = <0x62>;
+            interrupt-parent = <&gpio_intc>;
+            interrupts = <29 IRQ_TYPE_EDGE_RISING>;
+            vdd-supply = <&vcc_5v>;
+        };
+    };
diff --git a/MAINTAINERS b/MAINTAINERS
index 010e7d854bf7..4b76052e81cf 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12996,6 +12996,7 @@ MEMSENSING MICROSYSTEMS MSA311 DRIVER
 M:	Dmitry Rokosov <ddrokosov@sberdevices.ru>
 L:	linux-iio@vger.kernel.org
 S:	Maintained
+F:	Documentation/devicetree/bindings/iio/accel/memsensing,msa311.yaml
 F:	drivers/iio/accel/msa311.c
 
 MEN A21 WATCHDOG DRIVER
-- 
2.36.0

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [PATCH v5 6/7] iio: add MEMSensing MSA311 3-axis accelerometer driver
  2022-08-12 16:52 ` [PATCH v5 6/7] iio: add MEMSensing MSA311 3-axis accelerometer driver Dmitry Rokosov
@ 2022-08-12 17:10   ` Dmitry Rokosov
  2022-08-12 22:34   ` Andy Shevchenko
  2022-08-14 16:41   ` Jonathan Cameron
  2 siblings, 0 replies; 17+ messages in thread
From: Dmitry Rokosov @ 2022-08-12 17:10 UTC (permalink / raw)
  To: akpm, jic23, robh+dt, andriy.shevchenko, andy.shevchenko,
	christophe.jaillet, stano.jakubek, shawnguo, stephan,
	daniel.lezcano, wsa, lars, Michael.Hennerich, jbhayana,
	lucas.demarchi, jani.nikula, linus.walleij
  Cc: linux-iio, kernel, devicetree, linux-kernel

Hello Jonathan and Andy,

Please find a few comments below. They explain why I didn't do something
which was mentioned or suggested in the v4.

On Fri, Aug 12, 2022 at 04:52:29PM +0000, Dmitry Rokosov wrote:

[...]

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

I didn't change odr and fs table structures to s32_fract, because they
don't have numerator/denominator format, but it's an integer.fractional
format. I didn't find such a common struct for this format. If we want to
generalize such a fractional number format, I suppose it's better to do it
in the separate patch series over the whole IIO subsystem, due to the many
val.val2-like structures inside it.

[...]

> +/**
> + * struct msa311_priv - MSA311 internal private state
> + * @regs: Underlying I2C bus adapter used to abstract slave
> + *        register accesses
> + * @fields: Abstract objects for each registers fields access
> + * @dev: Device handler associated with appropriate bus client
> + * @lock: Protects msa311 device state between setup and data access routines
> + *        (power transitions, samp_freq/scale tune, retrieving axes data, etc)
> + * @chip_name: Chip name in the format "msa311-%hhx" % partid
> + * @new_data_trig: Optional NEW_DATA interrupt driven trigger used
> + *                 to notify external consumers a new sample is ready
> + * @vdd: Optional external voltage regulator for the device power supply
> + */
> +struct msa311_priv {
> +	struct regmap *regs;
> +	struct regmap_field *fields[F_MAX_FIELDS];
> +
> +	struct device *dev;

I stay struct device *dev pointer in the msa311 private structure,
because without it code looks more grubby and we can't retrieve indio_dev
from msa311_priv object using container_of or something else
(it's just a dynamic pointer).

> +	struct mutex lock;

I've removed the state guard comment, but checkpatch.pl is still raising
a warning.

[...]

> +	err = msa311_chip_init(msa311);
> +	if (err)
> +		return err;
> +
> +	indio_dev->modes = INDIO_DIRECT_MODE;

I returned INDIO_DIRECT_MODE initialization, but actually I'm not sure
if it's needed when we setup buffer mode regardless of any optional
parameters.

[...]

-- 
Thank you,
Dmitry

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v5 1/7] lib/string_helpers: Add str_read_write() helper
  2022-08-12 16:52 ` [PATCH v5 1/7] lib/string_helpers: Add str_read_write() helper Dmitry Rokosov
@ 2022-08-12 17:15   ` Dmitry Rokosov
  0 siblings, 0 replies; 17+ messages in thread
From: Dmitry Rokosov @ 2022-08-12 17:15 UTC (permalink / raw)
  To: andriy.shevchenko, andy.shevchenko
  Cc: linux-iio, akpm, jic23, robh+dt, christophe.jaillet,
	stano.jakubek, shawnguo, stephan, daniel.lezcano, wsa, lars,
	Michael.Hennerich, jbhayana, lucas.demarchi, jani.nikula,
	linus.walleij, kernel, devicetree, linux-kernel

Hello Andy,

Please be informed, I've patched commit msg a little bit, replaced
'retun' misstyping to 'return'.

On Fri, Aug 12, 2022 at 04:52:25PM +0000, Dmitry Rokosov wrote:
> From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> 
> Add str_read_write() helper to return 'read' or 'write' string literal.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Signed-off-by: Dmitry Rokosov <ddrokosov@sberdevices.ru>
> ---
>  include/linux/string_helpers.h | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/include/linux/string_helpers.h b/include/linux/string_helpers.h
> index 4d72258d42fd..9e22cd78f3b8 100644
> --- a/include/linux/string_helpers.h
> +++ b/include/linux/string_helpers.h
> @@ -126,4 +126,9 @@ static inline const char *str_enabled_disabled(bool v)
>  	return v ? "enabled" : "disabled";
>  }
>  
> +static inline const char *str_read_write(bool v)
> +{
> +	return v ? "read" : "write";
> +}
> +
>  #endif
> -- 
> 2.36.0

-- 
Thank you,
Dmitry

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v5 6/7] iio: add MEMSensing MSA311 3-axis accelerometer driver
  2022-08-12 16:52 ` [PATCH v5 6/7] iio: add MEMSensing MSA311 3-axis accelerometer driver Dmitry Rokosov
  2022-08-12 17:10   ` Dmitry Rokosov
@ 2022-08-12 22:34   ` Andy Shevchenko
  2022-08-14 16:26     ` Jonathan Cameron
  2022-08-15 14:01     ` Dmitry Rokosov
  2022-08-14 16:41   ` Jonathan Cameron
  2 siblings, 2 replies; 17+ messages in thread
From: Andy Shevchenko @ 2022-08-12 22:34 UTC (permalink / raw)
  To: Dmitry Rokosov
  Cc: akpm, jic23, robh+dt, andriy.shevchenko, christophe.jaillet,
	stano.jakubek, shawnguo, stephan, daniel.lezcano, wsa, lars,
	Michael.Hennerich, jbhayana, lucas.demarchi, jani.nikula,
	linus.walleij, linux-iio, kernel, devicetree, linux-kernel

On Fri, Aug 12, 2022 at 7:52 PM Dmitry Rokosov <DDRokosov@sberdevices.ru> wrote:
>
> MSA311 is a tri-axial, low-g accelerometer with I2C digital output for
> sensitivity consumer applications. It has dynamic user-selectable full
> scales range of +-2g/+-4g/+-8g/+-16g and allows acceleration measurements
> with output data rates from 1Hz to 1000Hz.

> Spec: https://cdn-shop.adafruit.com/product-files/5309/MSA311-V1.1-ENG.pdf

Can you use Datasheet: tag below (just before your SoB tag)?

> This driver supports following MSA311 features:
>     - IIO interface
>     - Different power modes: NORMAL and SUSPEND (using pm_runtime)
>     - ODR (Output Data Rate) selection
>     - Scale and samp_freq selection
>     - IIO triggered buffer, IIO reg access
>     - NEW_DATA interrupt + trigger
>
> Below features to be done:
>     - Motion Events: ACTIVE, TAP, ORIENT, FREEFALL
>     - Low Power mode

...

> +static const struct {
> +       int val;
> +       int val2;
> +} msa311_fs_table[] = {
> +       {0, 9580}, {0, 19160}, {0, 38320}, {0, 76641}
> +};

At least you may deduplicate the type definition for these data structures, like

struct iio_float {
  int integer;
  int fract;
};

...

> +               dev_err(dev,
> +                       "cannot set odr %u.%luHz, not available in %s mode\n",
> +                       msa311_odr_table[odr].val,
> +                       msa311_odr_table[odr].val2 / MILLIHZ_PER_HZ,

Logically it's MICROHZ_PER_MILLIHZ.

> +                       msa311_pwr_modes[pwr_mode]);

...

> +       freq_uhz = msa311_odr_table[odr].val * MICROHZ_PER_HZ +
> +                  msa311_odr_table[odr].val2;
> +       wait_ms = (MICROHZ_PER_HZ / freq_uhz) * MSEC_PER_SEC;

On the contrary this seems correct.

...

> +       err = iio_device_claim_direct_mode(indio_dev);
> +       if (err)
> +               return err;
> +
> +       mutex_lock(&msa311->lock);
> +       err = msa311_get_axis(msa311, chan, &axis);
> +       mutex_unlock(&msa311->lock);
> +
> +       iio_device_release_direct_mode(indio_dev);
> +
> +       if (err) {
> +               dev_err(dev, "cannot get axis %s (%pe)\n",
> +                       chan->datasheet_name, ERR_PTR(err));
> +               return err;
> +       }
> +
> +       pm_runtime_mark_last_busy(dev);
> +       pm_runtime_put_autosuspend(dev);

All error cases above miss the PM restoration. Is it on purpose?
Otherwise fix it here and check everywhere else.

...

> +       used = scnprintf(msa311->chip_name, sizeof(msa311->chip_name),
> +                        "msa311-%hhx", partid);

> +       msa311->chip_name[used] = '\0';

What is this for exactly?

...

> +       /* Disable all interrupts by default */
> +       err = regmap_write(msa311->regs, MSA311_INT_SET_0_REG, 0);
> +       if (err)
> +               return dev_err_probe(dev, err,
> +                                    "cannot disable set0 interrupts\n");
> +
> +       err = regmap_write(msa311->regs, MSA311_INT_SET_1_REG, 0);
> +       if (err)
> +               return dev_err_probe(dev, err,
> +                                    "cannot disable set1 interrupts\n");

Wondering if the above can be combined to bulk write.

...

> +       /* Unmap all INT1 interrupts by default */
> +       err = regmap_write(msa311->regs, MSA311_INT_MAP_0_REG, 0);
> +       if (err)
> +               return dev_err_probe(dev, err,
> +                                    "failed to unmap map0 interrupts\n");
> +
> +       err = regmap_write(msa311->regs, MSA311_INT_MAP_1_REG, 0);
> +       if (err)
> +               return dev_err_probe(dev, err,
> +                                    "failed to unmap map1 interrupts\n");

Ditto.

...

> +       /* Keep going without interrupts if no initialized I2C irq */

IRQ

...

> +                                    "cannot allocate newdata trigger\n");

In case you wish to make them shorter, you can use "can't".

...

> +                                    "can't register newdata trigger\n");

...esp. if you already do this in the other message, but not in all.
So, some consistency would be good to have.

...

I avoided commenting on the points that were not addressed and you
explained why.

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v5 6/7] iio: add MEMSensing MSA311 3-axis accelerometer driver
  2022-08-12 22:34   ` Andy Shevchenko
@ 2022-08-14 16:26     ` Jonathan Cameron
  2022-08-15 14:01     ` Dmitry Rokosov
  1 sibling, 0 replies; 17+ messages in thread
From: Jonathan Cameron @ 2022-08-14 16:26 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Dmitry Rokosov, akpm, robh+dt, andriy.shevchenko,
	christophe.jaillet, stano.jakubek, shawnguo, stephan,
	daniel.lezcano, wsa, lars, Michael.Hennerich, jbhayana,
	lucas.demarchi, jani.nikula, linus.walleij, linux-iio, kernel,
	devicetree, linux-kernel

On Sat, 13 Aug 2022 01:34:40 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Fri, Aug 12, 2022 at 7:52 PM Dmitry Rokosov <DDRokosov@sberdevices.ru> wrote:
> >
> > MSA311 is a tri-axial, low-g accelerometer with I2C digital output for
> > sensitivity consumer applications. It has dynamic user-selectable full
> > scales range of +-2g/+-4g/+-8g/+-16g and allows acceleration measurements
> > with output data rates from 1Hz to 1000Hz.  
> 
> > Spec: https://cdn-shop.adafruit.com/product-files/5309/MSA311-V1.1-ENG.pdf  
> 
> Can you use Datasheet: tag below (just before your SoB tag)?
> 
> > This driver supports following MSA311 features:
> >     - IIO interface
> >     - Different power modes: NORMAL and SUSPEND (using pm_runtime)
> >     - ODR (Output Data Rate) selection
> >     - Scale and samp_freq selection
> >     - IIO triggered buffer, IIO reg access
> >     - NEW_DATA interrupt + trigger
> >
> > Below features to be done:
> >     - Motion Events: ACTIVE, TAP, ORIENT, FREEFALL
> >     - Low Power mode  
> 
> ...
> 
> > +static const struct {
> > +       int val;
> > +       int val2;
> > +} msa311_fs_table[] = {
> > +       {0, 9580}, {0, 19160}, {0, 38320}, {0, 76641}
> > +};  
> 
> At least you may deduplicate the type definition for these data structures, like
> 
> struct iio_float {

iio_int_plus_micro maybe...
we have lots of fixed point types and they definitely are floats
then integer, micro


>   int integer;
>   int fract;
> };
>

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v5 6/7] iio: add MEMSensing MSA311 3-axis accelerometer driver
  2022-08-12 16:52 ` [PATCH v5 6/7] iio: add MEMSensing MSA311 3-axis accelerometer driver Dmitry Rokosov
  2022-08-12 17:10   ` Dmitry Rokosov
  2022-08-12 22:34   ` Andy Shevchenko
@ 2022-08-14 16:41   ` Jonathan Cameron
  2 siblings, 0 replies; 17+ messages in thread
From: Jonathan Cameron @ 2022-08-14 16:41 UTC (permalink / raw)
  To: Dmitry Rokosov
  Cc: akpm, robh+dt, andriy.shevchenko, andy.shevchenko,
	christophe.jaillet, stano.jakubek, shawnguo, stephan,
	daniel.lezcano, wsa, lars, Michael.Hennerich, jbhayana,
	lucas.demarchi, jani.nikula, linus.walleij, linux-iio, kernel,
	devicetree, linux-kernel

On Fri, 12 Aug 2022 16:52:29 +0000
Dmitry Rokosov <DDRokosov@sberdevices.ru> wrote:

> MSA311 is a tri-axial, low-g accelerometer with I2C digital output for
> sensitivity consumer applications. It has dynamic user-selectable full
> scales range of +-2g/+-4g/+-8g/+-16g and allows acceleration measurements
> with output data rates from 1Hz to 1000Hz.
> 
> Spec: https://cdn-shop.adafruit.com/product-files/5309/MSA311-V1.1-ENG.pdf
> 
> This driver supports following MSA311 features:
>     - IIO interface
>     - Different power modes: NORMAL and SUSPEND (using pm_runtime)
>     - ODR (Output Data Rate) selection
>     - Scale and samp_freq selection
>     - IIO triggered buffer, IIO reg access
>     - NEW_DATA interrupt + trigger
> 
> Below features to be done:
>     - Motion Events: ACTIVE, TAP, ORIENT, FREEFALL
>     - Low Power mode
> 
> Signed-off-by: Dmitry Rokosov <ddrokosov@sberdevices.ru>
Hi Dmitry, a few minor comments from another read through.

> + */
> +static inline int msa311_set_odr(struct msa311_priv *msa311, unsigned int odr)
> +{
> +	struct device *dev = msa311->dev;
> +	unsigned int pwr_mode;
> +	bool good_odr = false;
> +	int err;
> +
> +	err = regmap_field_read(msa311->fields[F_PWR_MODE], &pwr_mode);
> +	if (err)
> +		return err;
> +
> +	/* Filter bad ODR values */
> +	if (pwr_mode == MSA311_PWR_MODE_NORMAL)
> +		good_odr = (odr > MSA311_ODR_1_95_HZ);
> +
> +	if (!good_odr) {
> +		dev_err(dev,
> +			"cannot set odr %u.%luHz, not available in %s mode\n",

Don't you need to zero pad the fractional part?
or use iio_format_values.

> +			msa311_odr_table[odr].val,
> +			msa311_odr_table[odr].val2 / MILLIHZ_PER_HZ,
> +			msa311_pwr_modes[pwr_mode]);
> +		return -EINVAL;
> +	}
> +
> +	return regmap_field_write(msa311->fields[F_ODR], odr);
> +}


> +
> +static int msa311_setup_interrupts(struct msa311_priv *msa311)
> +{
> +	struct device *dev = msa311->dev;
> +	struct i2c_client *i2c = to_i2c_client(dev);
> +	struct iio_dev *indio_dev = i2c_get_clientdata(i2c);
> +	struct iio_trigger *trig;
> +	int err;
> +
> +	/* Keep going without interrupts if no initialized I2C irq */
> +	if (i2c->irq <= 0)
> +		return 0;
> +
> +	err = devm_request_threaded_irq(&i2c->dev, i2c->irq,
> +					NULL, msa311_irq_thread,
> +					IRQF_ONESHOT,
> +					msa311->chip_name,
> +					indio_dev);

Could wrap this a little less given you are respinning anyway. It's way under
80 chars currently.

...
	     "cannot map new data interrupt\n");
> +
> +	return 0;
> +}
> +

> +	msa311->vdd = devm_regulator_get_optional(dev, "vdd");
> +	if (IS_ERR(msa311->vdd)) {
> +		err = PTR_ERR(msa311->vdd);
> +		if (err == -ENODEV)
> +			msa311->vdd = NULL;
> +		else
> +			return dev_err_probe(dev, PTR_ERR(msa311->vdd),

use err instead of PTR_ERR(msa311->vdd) ?

> +					     "cannot get vdd supply\n");
> +	}
> +
> +	if (msa311->vdd) {
> +		err = regulator_enable(msa311->vdd);
> +		if (err)
> +			return dev_err_probe(dev, err,
> +					     "cannot enable vdd supply\n");
> +
> +		err = devm_add_action_or_reset(dev, msa311_vdd_disable,
> +					       msa311->vdd);
> +		if (err) {
> +			return dev_err_probe(dev, err,
> +					     "cannot add vdd disable action\n");
> +		}
> +	}
> +
> +	err = msa311_check_partid(msa311);
> +	if (err)
> +		return err;
> +
> +	err = msa311_soft_reset(msa311);
> +	if (err)
> +		return err;
> +


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v5 6/7] iio: add MEMSensing MSA311 3-axis accelerometer driver
  2022-08-12 22:34   ` Andy Shevchenko
  2022-08-14 16:26     ` Jonathan Cameron
@ 2022-08-15 14:01     ` Dmitry Rokosov
  1 sibling, 0 replies; 17+ messages in thread
From: Dmitry Rokosov @ 2022-08-15 14:01 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: akpm, jic23, robh+dt, andriy.shevchenko, christophe.jaillet,
	stano.jakubek, shawnguo, stephan, daniel.lezcano, wsa, lars,
	Michael.Hennerich, jbhayana, lucas.demarchi, jani.nikula,
	linus.walleij, linux-iio, kernel, devicetree, linux-kernel

On Sat, Aug 13, 2022 at 01:34:40AM +0300, Andy Shevchenko wrote:

[...]

> > Spec: https://cdn-shop.adafruit.com/product-files/5309/MSA311-V1.1-ENG.pdf
> 
> Can you use Datasheet: tag below (just before your SoB tag)?

Sure, I can move it on the previous line beforce SoB tag.
But with Datasheet: tag this line will be over the commit msg line limit.
If you don't mind I will stay with Spec: tag instead of Datasheet:

[...]

> > +static const struct {
> > +       int val;
> > +       int val2;
> > +} msa311_fs_table[] = {
> > +       {0, 9580}, {0, 19160}, {0, 38320}, {0, 76641}
> > +};
> 
> At least you may deduplicate the type definition for these data structures, like
> 
> struct iio_float {
>   int integer;
>   int fract;
> };

Agreed. I will make deduplication of this type inside msa311 driver
only.

[...]

> > +               dev_err(dev,
> > +                       "cannot set odr %u.%luHz, not available in %s mode\n",
> > +                       msa311_odr_table[odr].val,
> > +                       msa311_odr_table[odr].val2 / MILLIHZ_PER_HZ,
> 
> Logically it's MICROHZ_PER_MILLIHZ.
> 

You are right. But I think it would be better to print the original val2
value with zero-padding.

[...]

> > +       freq_uhz = msa311_odr_table[odr].val * MICROHZ_PER_HZ +
> > +                  msa311_odr_table[odr].val2;
> > +       wait_ms = (MICROHZ_PER_HZ / freq_uhz) * MSEC_PER_SEC;
> 
> On the contrary this seems correct.

Good

> > +       err = iio_device_claim_direct_mode(indio_dev);
> > +       if (err)
> > +               return err;
> > +
> > +       mutex_lock(&msa311->lock);
> > +       err = msa311_get_axis(msa311, chan, &axis);
> > +       mutex_unlock(&msa311->lock);
> > +
> > +       iio_device_release_direct_mode(indio_dev);
> > +
> > +       if (err) {
> > +               dev_err(dev, "cannot get axis %s (%pe)\n",
> > +                       chan->datasheet_name, ERR_PTR(err));
> > +               return err;
> > +       }
> > +
> > +       pm_runtime_mark_last_busy(dev);
> > +       pm_runtime_put_autosuspend(dev);
> 
> All error cases above miss the PM restoration. Is it on purpose?
> Otherwise fix it here and check everywhere else.

Nice catch! This is a bug. I'll fix it in v6

> > +       used = scnprintf(msa311->chip_name, sizeof(msa311->chip_name),
> > +                        "msa311-%hhx", partid);
> 
> > +       msa311->chip_name[used] = '\0';
> 
> What is this for exactly?
> 

Ah, you are right, scnprintf() makes null terminating inside.

> > +       /* Disable all interrupts by default */
> > +       err = regmap_write(msa311->regs, MSA311_INT_SET_0_REG, 0);
> > +       if (err)
> > +               return dev_err_probe(dev, err,
> > +                                    "cannot disable set0 interrupts\n");
> > +
> > +       err = regmap_write(msa311->regs, MSA311_INT_SET_1_REG, 0);
> > +       if (err)
> > +               return dev_err_probe(dev, err,
> > +                                    "cannot disable set1 interrupts\n");
> 
> Wondering if the above can be combined to bulk write.
> 

I will try and rework this place if it's workable.

> > +       /* Unmap all INT1 interrupts by default */
> > +       err = regmap_write(msa311->regs, MSA311_INT_MAP_0_REG, 0);
> > +       if (err)
> > +               return dev_err_probe(dev, err,
> > +                                    "failed to unmap map0 interrupts\n");
> > +
> > +       err = regmap_write(msa311->regs, MSA311_INT_MAP_1_REG, 0);
> > +       if (err)
> > +               return dev_err_probe(dev, err,
> > +                                    "failed to unmap map1 interrupts\n");
> 
> Ditto.

The same as above.

[...]

-- 
Thank you,
Dmitry

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v5 2/7] units: complement the set of Hz units
  2022-08-12 16:52 ` [PATCH v5 2/7] units: complement the set of Hz units Dmitry Rokosov
@ 2022-08-28 15:55   ` Jonathan Cameron
  2022-08-29 21:06     ` Andrew Morton
  2022-08-29 21:07     ` Andrew Morton
  0 siblings, 2 replies; 17+ messages in thread
From: Jonathan Cameron @ 2022-08-28 15:55 UTC (permalink / raw)
  To: Dmitry Rokosov
  Cc: akpm, robh+dt, andriy.shevchenko, andy.shevchenko,
	christophe.jaillet, stano.jakubek, shawnguo, stephan,
	daniel.lezcano, wsa, lars, Michael.Hennerich, jbhayana,
	lucas.demarchi, jani.nikula, linus.walleij, linux-iio, kernel,
	devicetree, linux-kernel, Andrew Morton

On Fri, 12 Aug 2022 16:52:26 +0000
Dmitry Rokosov <DDRokosov@sberdevices.ru> wrote:

> Currently, Hz units do not have milli, micro and nano Hz coefficients.
> Some drivers (IIO especially) use their analogues to calculate
> appropriate Hz values. This patch includes them to units.h definitions,
> so they can be used from different kernel places.
> 
> Signed-off-by: Dmitry Rokosov <ddrokosov@sberdevices.ru>
I'm not really sure why Andrew Morton picked these up as no obviously
dependencies outside of IIO and we have other patches under review that
need these.

Anyhow, I see they are still in Andrew's nonmm-unstable tree, so
assuming he won't mind me picking them up through IIO instead / as well.
If nothing else git will sort this out when the two trees reach
linux-next or upstream anyway.

+Cc Andrew Morton.

this and next two patches applied to the togreg branch of iio.git.
I'll push that out as testing for 0-day to do it's sanity checks then
it'll go out as iio.git / togreg and get picked up by linux-next.

Thanks,

Jonathan

> ---
>  include/linux/units.h | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/include/linux/units.h b/include/linux/units.h
> index 681fc652e3d7..2793a41e73a2 100644
> --- a/include/linux/units.h
> +++ b/include/linux/units.h
> @@ -20,6 +20,9 @@
>  #define PICO	1000000000000ULL
>  #define FEMTO	1000000000000000ULL
>  
> +#define NANOHZ_PER_HZ		1000000000UL
> +#define MICROHZ_PER_HZ		1000000UL
> +#define MILLIHZ_PER_HZ		1000UL
>  #define HZ_PER_KHZ		1000UL
>  #define KHZ_PER_MHZ		1000UL
>  #define HZ_PER_MHZ		1000000UL


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v5 2/7] units: complement the set of Hz units
  2022-08-28 15:55   ` Jonathan Cameron
@ 2022-08-29 21:06     ` Andrew Morton
  2022-08-29 21:07     ` Andrew Morton
  1 sibling, 0 replies; 17+ messages in thread
From: Andrew Morton @ 2022-08-29 21:06 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: Dmitry Rokosov, robh+dt, andriy.shevchenko, andy.shevchenko,
	christophe.jaillet, stano.jakubek, shawnguo, stephan,
	daniel.lezcano, wsa, lars, Michael.Hennerich, jbhayana,
	lucas.demarchi, jani.nikula, linus.walleij, linux-iio, kernel,
	devicetree, linux-kernel

On Sun, 28 Aug 2022 16:55:41 +0100 Jonathan Cameron <jic23@kernel.org> wrote:

> On Fri, 12 Aug 2022 16:52:26 +0000
> Dmitry Rokosov <DDRokosov@sberdevices.ru> wrote:
> 
> > Currently, Hz units do not have milli, micro and nano Hz coefficients.
> > Some drivers (IIO especially) use their analogues to calculate
> > appropriate Hz values. This patch includes them to units.h definitions,
> > so they can be used from different kernel places.
> > 
> > Signed-off-by: Dmitry Rokosov <ddrokosov@sberdevices.ru>
> I'm not really sure why Andrew Morton picked these up as no obviously
> dependencies outside of IIO and we have other patches under review that
> need these.
> 
> Anyhow, I see they are still in Andrew's nonmm-unstable tree, so
> assuming he won't mind me picking them up through IIO instead / as well.
> If nothing else git will sort this out when the two trees reach
> linux-next or upstream anyway.
> 
> +Cc Andrew Morton.
> 
> this and next two patches applied to the togreg branch of iio.git.
> I'll push that out as testing for 0-day to do it's sanity checks then
> it'll go out as iio.git / togreg and get picked up by linux-next.
> 
> Thanks,
> 
> Jonathan
> 
> > ---
> >  include/linux/units.h | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/include/linux/units.h b/include/linux/units.h
> > index 681fc652e3d7..2793a41e73a2 100644
> > --- a/include/linux/units.h
> > +++ b/include/linux/units.h
> > @@ -20,6 +20,9 @@
> >  #define PICO	1000000000000ULL
> >  #define FEMTO	1000000000000000ULL
> >  
> > +#define NANOHZ_PER_HZ		1000000000UL
> > +#define MICROHZ_PER_HZ		1000000UL
> > +#define MILLIHZ_PER_HZ		1000UL
> >  #define HZ_PER_KHZ		1000UL
> >  #define KHZ_PER_MHZ		1000UL
> >  #define HZ_PER_MHZ		1000000UL

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v5 2/7] units: complement the set of Hz units
  2022-08-28 15:55   ` Jonathan Cameron
  2022-08-29 21:06     ` Andrew Morton
@ 2022-08-29 21:07     ` Andrew Morton
  1 sibling, 0 replies; 17+ messages in thread
From: Andrew Morton @ 2022-08-29 21:07 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: Dmitry Rokosov, robh+dt, andriy.shevchenko, andy.shevchenko,
	christophe.jaillet, stano.jakubek, shawnguo, stephan,
	daniel.lezcano, wsa, lars, Michael.Hennerich, jbhayana,
	lucas.demarchi, jani.nikula, linus.walleij, linux-iio, kernel,
	devicetree, linux-kernel

On Sun, 28 Aug 2022 16:55:41 +0100 Jonathan Cameron <jic23@kernel.org> wrote:

> On Fri, 12 Aug 2022 16:52:26 +0000
> Dmitry Rokosov <DDRokosov@sberdevices.ru> wrote:
> 
> > Currently, Hz units do not have milli, micro and nano Hz coefficients.
> > Some drivers (IIO especially) use their analogues to calculate
> > appropriate Hz values. This patch includes them to units.h definitions,
> > so they can be used from different kernel places.
> > 
> > Signed-off-by: Dmitry Rokosov <ddrokosov@sberdevices.ru>
> I'm not really sure why Andrew Morton picked these up as no obviously
> dependencies outside of IIO and we have other patches under review that
> need these.

Mainly because [0/n] was titled "units: ...", not "iio: ..."!

> Anyhow, I see they are still in Andrew's nonmm-unstable tree, so
> assuming he won't mind me picking them up through IIO instead / as well.
> If nothing else git will sort this out when the two trees reach
> linux-next or upstream anyway.

I'll drop 'em.

^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2022-08-29 21:07 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-08-12 16:52 [PATCH v5 0/7] iio: accel: add MSA311 accelerometer driver Dmitry Rokosov
2022-08-12 16:52 ` [PATCH v5 1/7] lib/string_helpers: Add str_read_write() helper Dmitry Rokosov
2022-08-12 17:15   ` Dmitry Rokosov
2022-08-12 16:52 ` [PATCH v5 2/7] units: complement the set of Hz units Dmitry Rokosov
2022-08-28 15:55   ` Jonathan Cameron
2022-08-29 21:06     ` Andrew Morton
2022-08-29 21:07     ` Andrew Morton
2022-08-12 16:52 ` [PATCH v5 4/7] iio: common: scmi_sensors: use HZ macro from units.h Dmitry Rokosov
2022-08-12 16:52 ` [PATCH v5 3/7] iio: accel: adxl345: " Dmitry Rokosov
2022-08-12 16:52 ` [PATCH v5 5/7] dt-bindings: vendor-prefixes: add MEMSensing Microsystems Co., Ltd Dmitry Rokosov
2022-08-12 16:52 ` [PATCH v5 6/7] iio: add MEMSensing MSA311 3-axis accelerometer driver Dmitry Rokosov
2022-08-12 17:10   ` Dmitry Rokosov
2022-08-12 22:34   ` Andy Shevchenko
2022-08-14 16:26     ` Jonathan Cameron
2022-08-15 14:01     ` Dmitry Rokosov
2022-08-14 16:41   ` Jonathan Cameron
2022-08-12 16:52 ` [PATCH v5 7/7] dt-bindings: iio: accel: add dt-binding schema for msa311 accel driver Dmitry Rokosov

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).