[v4,2/3] iio: add MEMSensing MSA311 3-axis accelerometer driver

Commit Message

Dmitry Rokosov Aug. 3, 2022, 1:11 p.m. UTC

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 <[e-mail]>
---
 MAINTAINERS                |    6 +
 drivers/iio/accel/Kconfig  |   13 +
 drivers/iio/accel/Makefile |    2 +
 drivers/iio/accel/msa311.c | 1323 ++++++++++++++++++++++++++++++++++++
 4 files changed, 1344 insertions(+)
 create mode 100644 drivers/iio/accel/msa311.c

Comments

Andy Shevchenko Aug. 3, 2022, 5:49 p.m. UTC | #1

On Wed, Aug 3, 2022 at 3:11 PM Dmitry Rokosov <[e-mail]> 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

Thanks for an update, my comments below.

...

> +#include <linux/i2c.h>

> +#include <linux/iio/buffer.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/iio/trigger.h>
> +#include <linux/iio/trigger_consumer.h>
> +#include <linux/iio/triggered_buffer.h>

I would split this group out of generic headers...

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

...and move it here to clearly show that the driver belongs to the IIO
subsystem.

...

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

I'm not sure if you are using programming language here or math language?
In math we refer to powers like 2^11, the 2<<11 puzzles me.

> + */

...

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

Besides that this struct is defined not only once in the file, this is
very well NIH struct s32_fract. Why not use the latter?

...

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

See above.

...

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

Leave a comma here even if we know that in the nearest future it won't
be changed.

> +};

...

> +       .cache_type = REGCACHE_RBTREE,

Tree hash is good for sparse data, do you really have it sparse (a lot
of  big gaps in between)?

> +};

...

> +       .datasheet_name = "ACC_"#axis                                       \

Leave a comma here.

...

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

Ditto.

> +};

...

> +               dev_err(dev,
> +                       "failed to set odr %u.%uHz, not available in %s mode\n",
> +                       msa311_odr_table[odr].val,
> +                       msa311_odr_table[odr].val2 / 1000,

KILO ?

> +                       msa311_pwr_modes[pwr_mode]);

...

> +       static const int unintr_thresh_ms = 20;

You seem not using it as _ms, but always as _us, why not define accordingly?
Also the other one is defined as unsigned long and this is as int. Why
not unsigned?

...

> +       for (fs = 0; fs < ARRAY_SIZE(msa311_fs_table); ++fs)

fs++ will work as well.

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

> +                       if (err)
> +                               dev_err(dev, "cannot update scale (%d)\n", err);

This can be done after putting the device back into (auto)sleep, right?

> +                       break;
> +               }
> +
> +       pm_runtime_mark_last_busy(dev);
> +       pm_runtime_put_autosuspend(dev);

...

> +       for (odr = 0; odr < ARRAY_SIZE(msa311_odr_table); ++odr)

odr++ works well also.

...

> +                               dev_err(dev, "cannot update freq (%d)\n", err);

frequency

...

> +               dev_err(dev, "cannot %s register %u from debugfs (%d)\n",
> +                       readval ? "read" : "write", reg, err);

You may consider taking [1] as a precursor here and use str_read_write().

[1]: https://lore.kernel.org/linux-i2c/[e-mail]/

...

> +       struct device *dev = msa311->dev;

Isn't it the same as indio_dev->dev.parent?
Do you really need that member?

...

> +       int bit = 0, err, i = 0;

How is the bit initial assignment used?

...

> +       /*
> +        * We do not check NEW_DATA int status, because of based on

because based on the

> +        * specification it's cleared automatically after a fixed time.
> +        * So just check that is enabled by driver logic.
> +        */

...

> +       /* TODO: check motion interrupts status */

I don't see the patches for this, so I assume they _will_ come in the
nearest future. Otherwise, drop these TODO lines.

...

> +               dev_dbg(dev, "found MSA311 compatible chip[%#x]\n", partid);

Useless message.

...

> +               return dev_err_probe(dev, err, "cannot disable set0 intrs\n");

interrupts

...

> +               return dev_err_probe(dev, err, "cannot disable set1 intrs\n");

Ditto.

...

> +               return dev_err_probe(dev, err, "failed to unmap map0 intrs\n");

Ditto.

...

> +               return dev_err_probe(dev, err, "failed to unmap map1 intrs\n");

Ditto.

...

> +       /* Disable all axis by default */

axis...

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

...or axes?

...

> +               return dev_err_probe(dev, err, "failed to set accel freq\n");

frequency

...

> +               return dev_err_probe(dev, err, "failed to request irq\n");

IRQ

...

> +               return dev_err_probe(dev, -ENOMEM,
> +                                    "cannot allocate newdata trig\n");

trigger

...

> +               return dev_err_probe(dev, err, "can't register newdata trig\n");

Ditto.

...

> +               return dev_err_probe(dev, err, "cannot enable push-pull int\n");

interrupt

...

> +               return dev_err_probe(dev, err, "cannot set active int level\n");

Ditto.

...

> +               return dev_err_probe(dev, err, "cannot latch int\n");

Ditto.

...

> +               return dev_err_probe(dev, err, "cannot reset int\n");

Ditto.

...

> +               return dev_err_probe(dev, err, "cannot map new data int\n");

interrupt

...

> +               return dev_err_probe(dev, -ENOMEM,
> +                                    "iio device allocation failed\n");

IIO

...

> +       indio_dev->modes = 0; /* setup buffered mode later */

Why explicit assignment to 0? Doesn't kzalloc() do it for you?

...

> +               return dev_err_probe(dev, err, "cannot setup iio trig buf\n");

IIO trigger buffer

...

> +               return dev_err_probe(dev, err, "iio device register failed\n");

IIO

Christophe JAILLET Aug. 3, 2022, 6:11 p.m. UTC | #2

Le 03/08/2022 à 15:11, Dmitry Rokosov a écrit :
> 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 <[e-mail]>
> ---
>   MAINTAINERS                |    6 +
>   drivers/iio/accel/Kconfig  |   13 +
>   drivers/iio/accel/Makefile |    2 +
>   drivers/iio/accel/msa311.c | 1323 ++++++++++++++++++++++++++++++++++++
>   4 files changed, 1344 insertions(+)
>   create mode 100644 drivers/iio/accel/msa311.c
> 

Hi,
a few nits below.

[...]

> +static int msa311_check_partid(struct msa311_priv *msa311)
> +{
> +	struct device *dev = msa311->dev;
> +	unsigned int partid;
> +	int err;
> +
> +	err = regmap_read(msa311->regs, MSA311_PARTID_REG, &partid);
> +	if (err)
> +		return dev_err_probe(dev, err,
> +				     "failed to read partid (%d)\n", err);

No need for "(%d)" and err.

> +
> +	if (partid == MSA311_WHO_AM_I)
> +		dev_dbg(dev, "found MSA311 compatible chip[%#x]\n", partid);
> +	else
> +		dev_warn(dev, "invalid partid (%#x), expected (%#x)\n",
> +			 partid, MSA311_WHO_AM_I);
> +
> +	return 0;
> +}

[...]

> +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) {
> +			regulator_disable(msa311->vdd);

Double regulator_disable(), because of the _or_reset()?

> +			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 (%d)\n", err);

No need for "(%d)" and err

CJ

Dmitry Rokosov Aug. 3, 2022, 6:39 p.m. UTC | #3

Hello Christophe,

Thank you for quick review

On Wed, Aug 03, 2022 at 08:11:05PM +0200, Christophe JAILLET wrote:
> Le 03/08/2022 à 15:11, Dmitry Rokosov a écrit :
> > 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 <[e-mail]>
> > ---
> >   MAINTAINERS                |    6 +
> >   drivers/iio/accel/Kconfig  |   13 +
> >   drivers/iio/accel/Makefile |    2 +
> >   drivers/iio/accel/msa311.c | 1323 ++++++++++++++++++++++++++++++++++++
> >   4 files changed, 1344 insertions(+)
> >   create mode 100644 drivers/iio/accel/msa311.c
> > 
> 
> Hi,
> a few nits below.
> 
> [...]
> 
> > +static int msa311_check_partid(struct msa311_priv *msa311)
> > +{
> > +	struct device *dev = msa311->dev;
> > +	unsigned int partid;
> > +	int err;
> > +
> > +	err = regmap_read(msa311->regs, MSA311_PARTID_REG, &partid);
> > +	if (err)
> > +		return dev_err_probe(dev, err,
> > +				     "failed to read partid (%d)\n", err);
> 
> No need for "(%d)" and err.
> 

Do you mean for all dev_err() calls? I think sometimes it's helpful to
know the actual error value got from external API, isn't? Could you please
explain your point if possible?

> > +
> > +	if (partid == MSA311_WHO_AM_I)
> > +		dev_dbg(dev, "found MSA311 compatible chip[%#x]\n", partid);
> > +	else
> > +		dev_warn(dev, "invalid partid (%#x), expected (%#x)\n",
> > +			 partid, MSA311_WHO_AM_I);
> > +
> > +	return 0;
> > +}
> 
> [...]
> 
> > +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) {
> > +			regulator_disable(msa311->vdd);
> 
> Double regulator_disable(), because of the _or_reset()?
> 

Yep. If devm_add_action_or_reset() returns an error, we will not
call regulator_disable() by devm subsystem. It means, we have to
call it directly.

> > +			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 (%d)\n", err);
> 
> No need for "(%d)" and err

Asked for the clarification above.

> 
> CJ

Dmitry Rokosov Aug. 3, 2022, 7:16 p.m. UTC | #4

Hello Andy,

Thank you for quick review,

On Wed, Aug 03, 2022 at 07:49:33PM +0200, Andy Shevchenko wrote:
> On Wed, Aug 3, 2022 at 3:11 PM Dmitry Rokosov <[e-mail]> 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
> 
> Thanks for an update, my comments below.
> 
> ...
> 
> > +#include <linux/i2c.h>
> 
> > +#include <linux/iio/buffer.h>
> > +#include <linux/iio/iio.h>
> > +#include <linux/iio/sysfs.h>
> > +#include <linux/iio/trigger.h>
> > +#include <linux/iio/trigger_consumer.h>
> > +#include <linux/iio/triggered_buffer.h>
> 
> I would split this group out of generic headers...
> 
> > +#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>
> > +
> 
> ...and move it here to clearly show that the driver belongs to the IIO
> subsystem.

Sure, no problem.

> 
> ...
> 
> > +/*
> > + * 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
> 
> I'm not sure if you are using programming language here or math language?
> In math we refer to powers like 2^11, the 2<<11 puzzles me.
> 

Agree, will change in the v5.

> > + */
> 
> ...
> 
> > +static const struct {
> > +       int val;
> > +       int val2;
> > +} msa311_fs_table[] = {
> > +       {0, 9580}, {0, 19160}, {0, 38320}, {0, 76641}
> > +};
> 
> Besides that this struct is defined not only once in the file, this is
> very well NIH struct s32_fract. Why not use the latter?
> 
> ...
> 

Good point, but looks like this struct is not so popular in the other
kernel drivers:

===
grep "s32_fract" -r -l . | wc -l    
3
===

[...]

> > +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"
> 
> Leave a comma here even if we know that in the nearest future it won't
> be changed.
> 

Good catch, forgot it.

> > +};
> 
> ...
> 
> > +       .cache_type = REGCACHE_RBTREE,
> 
> Tree hash is good for sparse data, do you really have it sparse (a lot
> of  big gaps in between)?

I suppose so. MSA311 regmap has ~6 gaps.

[...]

> 
> > +               dev_err(dev,
> > +                       "failed to set odr %u.%uHz, not available in %s mode\n",
> > +                       msa311_odr_table[odr].val,
> > +                       msa311_odr_table[odr].val2 / 1000,
> 
> KILO ?

MILLIHZ_PER_HZ :)

> 
> > +                       msa311_pwr_modes[pwr_mode]);
> 
> ...
> 
> > +       static const int unintr_thresh_ms = 20;
> 
> You seem not using it as _ms, but always as _us, why not define accordingly?
> Also the other one is defined as unsigned long and this is as int. Why
> not unsigned?
> 
> ...
> 

I compare unintr_thresh_ms with wait_ms and use wait_ms with
msleep_interruptible(), and I think 20ms is more readable than 20000us
or 20 * USEC_PER_MSEC us.

But I agree about sign. It should be unsigned anyway.

> > +       for (fs = 0; fs < ARRAY_SIZE(msa311_fs_table); ++fs)
> 
> fs++ will work as well.
> 

I would prefer ++fs if you don't mind :)

> > +               /* 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);
> 
> > +                       if (err)
> > +                               dev_err(dev, "cannot update scale (%d)\n", err);
> 
> This can be done after putting the device back into (auto)sleep, right?
> 

Are you talking about dev_err logging? Sure, it can be moved after
pm_runtime* calls.

> > +                       break;
> > +               }
> > +
> > +       pm_runtime_mark_last_busy(dev);
> > +       pm_runtime_put_autosuspend(dev);
> 
> ...
> 
> > +       for (odr = 0; odr < ARRAY_SIZE(msa311_odr_table); ++odr)
> 
> odr++ works well also.

I would prefer ++odr if you don't mind :)

> 
> ...
> 
> > +                               dev_err(dev, "cannot update freq (%d)\n", err);
> 
> frequency

It will be more ugly due 80 symbols restriction.

> 
> ...
> 
> > +               dev_err(dev, "cannot %s register %u from debugfs (%d)\n",
> > +                       readval ? "read" : "write", reg, err);
> 
> You may consider taking [1] as a precursor here and use str_read_write().
> 
> [1]: https://lore.kernel.org/linux-i2c/[e-mail]/

Oh, really... Thank you for suggestion!
> 
> ...
> 
> > +       struct device *dev = msa311->dev;
> 
> Isn't it the same as indio_dev->dev.parent?
> Do you really need that member?

Should be the same... I will check.

> 
> ...
> 
> > +       int bit = 0, err, i = 0;
> 
> How is the bit initial assignment used?

You are right, for for_each_set_bit() initializes bit inside.

[...]

> > +               dev_dbg(dev, "found MSA311 compatible chip[%#x]\n", partid);
> 
> Useless message.
> 

Why? It's under dynamic debug, so I will see it if I really want to.

> ...
> 
> > +               return dev_err_probe(dev, err, "cannot disable set0 intrs\n");
> 
> interrupts

It will be more ugly due 80 symbols restriction.

[...]

> > +       /* Disable all axis by default */
> 
> axis...
> 
> > +       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");
> 
> ...or axes?

Axes of course.

> 
> ...
> 
> > +               return dev_err_probe(dev, err, "failed to set accel freq\n");
> 
> frequency
> 

It will be more ugly due 80 symbols restriction.

[...]

> > +               return dev_err_probe(dev, -ENOMEM,
> > +                                    "cannot allocate newdata trig\n");
> 
> trigger

It will be more ugly due 80 symbols restriction.

[...]

> > +               return dev_err_probe(dev, err, "cannot enable push-pull int\n");
> 
> interrupt

It will be more ugly due 80 symbols restriction.

> > +       indio_dev->modes = 0; /* setup buffered mode later */
> 
> Why explicit assignment to 0? Doesn't kzalloc() do it for you?

kzalloc() will do it for me, of course. Previously, I initialized modes to
INDIO_DIRECT_MODE to just provide default value for that. Jonathan
suggested to replace it with 0. I can remove this line at all, no problem.
I just thought, it's more readable.

> 
> ...
> 
> > +               return dev_err_probe(dev, err, "cannot setup iio trig buf\n");
> 
> IIO trigger buffer

It will be more ugly due 80 symbols restriction.

> 
> ...
> 
> > +               return dev_err_probe(dev, err, "iio device register failed\n");
> 
> IIO
> 
> -- 
> With Best Regards,
> Andy Shevchenko

Christophe JAILLET Aug. 3, 2022, 7:18 p.m. UTC | #5

Le 03/08/2022 à 20:39, Dmitry Rokosov a écrit :
> Hello Christophe,
> 
> Thank you for quick review
> 
> On Wed, Aug 03, 2022 at 08:11:05PM +0200, Christophe JAILLET wrote:
>> Le 03/08/2022 à 15:11, Dmitry Rokosov a écrit :
>>> 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 <[e-mail]>
>>> ---
>>>    MAINTAINERS                |    6 +
>>>    drivers/iio/accel/Kconfig  |   13 +
>>>    drivers/iio/accel/Makefile |    2 +
>>>    drivers/iio/accel/msa311.c | 1323 ++++++++++++++++++++++++++++++++++++
>>>    4 files changed, 1344 insertions(+)
>>>    create mode 100644 drivers/iio/accel/msa311.c
>>>
>>
>> Hi,
>> a few nits below.
>>
>> [...]
>>
>>> +static int msa311_check_partid(struct msa311_priv *msa311)
>>> +{
>>> +	struct device *dev = msa311->dev;
>>> +	unsigned int partid;
>>> +	int err;
>>> +
>>> +	err = regmap_read(msa311->regs, MSA311_PARTID_REG, &partid);
>>> +	if (err)
>>> +		return dev_err_probe(dev, err,
>>> +				     "failed to read partid (%d)\n", err);
>>
>> No need for "(%d)" and err.
>>
> 
> Do you mean for all dev_err() calls? I think sometimes it's helpful to
> know the actual error value got from external API, isn't? Could you please
> explain your point if possible?
> 

No, my comment is only for dev_err_probe() function.
Having ret for dev_err() calls is fine.

See: https://elixir.bootlin.com/linux/v5.19/source/drivers/base/core.c#L4732

dev_err_probe() already has a "error %pe:..., ERR_PTR(err)"
This means that if ret = -ENOMEM:
    "(%d)"       --> "(-12)"
    "error %pe:" --> "error -ENOMEM:"

So there is no real need to duplicate the error code in the message 
itself, it is already displayed in a human readable manner.

What your code does would result in:
    "error -ENOMEM: failed to read partid (-12)\n"

>>> +
>>> +	if (partid == MSA311_WHO_AM_I)
>>> +		dev_dbg(dev, "found MSA311 compatible chip[%#x]\n", partid);
>>> +	else
>>> +		dev_warn(dev, "invalid partid (%#x), expected (%#x)\n",
>>> +			 partid, MSA311_WHO_AM_I);
>>> +
>>> +	return 0;
>>> +}
>>
>> [...]
>>
>>> +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) {
>>> +			regulator_disable(msa311->vdd);
>>
>> Double regulator_disable(), because of the _or_reset()?
>>
> 
> Yep. If devm_add_action_or_reset() returns an error, we will not
> call regulator_disable() by devm subsystem. It means, we have to
> call it directly.

No.

See 
https://elixir.bootlin.com/linux/v5.19/source/include/linux/device.h#L249

If devm_add_action_or_reset() fails, "action" is called. This is the 
meaning of the _or_reset suffix.

So here, msa311_vdd_disable() would be called and this function is:

+static void msa311_vdd_disable(void *vdd)
+{
+	regulator_disable(vdd);
+}

and "vdd" will be the value of "msa311->vdd"

So, unless I missed something obvious, your code will call twice 
regulator_disable(msa311->vdd).

One in devm_add_action_or_reset() and one explicitly after the "if (err)"


Hoping I'm clear and that I didn't miss something obvious.

CJ

> 
>>> +			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 (%d)\n", err);
>>
>> No need for "(%d)" and err
> 
> Asked for the clarification above.
> 
>>
>> CJ
>

Dmitry Rokosov Aug. 3, 2022, 7:26 p.m. UTC | #6

On Wed, Aug 03, 2022 at 09:18:26PM +0200, Christophe JAILLET wrote:
> Le 03/08/2022 à 20:39, Dmitry Rokosov a écrit :
> > Hello Christophe,
> > 
> > Thank you for quick review
> > 
> > On Wed, Aug 03, 2022 at 08:11:05PM +0200, Christophe JAILLET wrote:
> > > Le 03/08/2022 à 15:11, Dmitry Rokosov a écrit :
> > > > 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 <[e-mail]>
> > > > ---
> > > >    MAINTAINERS                |    6 +
> > > >    drivers/iio/accel/Kconfig  |   13 +
> > > >    drivers/iio/accel/Makefile |    2 +
> > > >    drivers/iio/accel/msa311.c | 1323 ++++++++++++++++++++++++++++++++++++
> > > >    4 files changed, 1344 insertions(+)
> > > >    create mode 100644 drivers/iio/accel/msa311.c
> > > > 
> > > 
> > > Hi,
> > > a few nits below.
> > > 
> > > [...]
> > > 
> > > > +static int msa311_check_partid(struct msa311_priv *msa311)
> > > > +{
> > > > +	struct device *dev = msa311->dev;
> > > > +	unsigned int partid;
> > > > +	int err;
> > > > +
> > > > +	err = regmap_read(msa311->regs, MSA311_PARTID_REG, &partid);
> > > > +	if (err)
> > > > +		return dev_err_probe(dev, err,
> > > > +				     "failed to read partid (%d)\n", err);
> > > 
> > > No need for "(%d)" and err.
> > > 
> > 
> > Do you mean for all dev_err() calls? I think sometimes it's helpful to
> > know the actual error value got from external API, isn't? Could you please
> > explain your point if possible?
> > 
> 
> No, my comment is only for dev_err_probe() function.
> Having ret for dev_err() calls is fine.
> 
> See: https://elixir.bootlin.com/linux/v5.19/source/drivers/base/core.c#L4732
> 
> dev_err_probe() already has a "error %pe:..., ERR_PTR(err)"
> This means that if ret = -ENOMEM:
>    "(%d)"       --> "(-12)"
>    "error %pe:" --> "error -ENOMEM:"
> 
> So there is no real need to duplicate the error code in the message itself,
> it is already displayed in a human readable manner.
> 
> What your code does would result in:
>    "error -ENOMEM: failed to read partid (-12)\n"
https://elixir.bootlin.com/linux/v5.19/source/drivers/base/core.c#L4732> 

Hmm, very interesting. Thank you for good point. I will fix it in the
v5, and will use %pe for all dev_err() calls, I suppose it's helpful.

[...]

> > > > +	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) {
> > > > +			regulator_disable(msa311->vdd);
> > > 
> > > Double regulator_disable(), because of the _or_reset()?
> > > 
> > 
> > Yep. If devm_add_action_or_reset() returns an error, we will not
> > call regulator_disable() by devm subsystem. It means, we have to
> > call it directly.
> 
> No.
> 
> See
https://elixir.bootlin.com/linux/v5.19/source/include/linux/device.h#L249
> 
> If devm_add_action_or_reset() fails, "action" is called. This is the meaning
> of the _or_reset suffix.
> 
> So here, msa311_vdd_disable() would be called and this function is:
> 
> +static void msa311_vdd_disable(void *vdd)
> +{
> +	regulator_disable(vdd);
> +}
> 
> and "vdd" will be the value of "msa311->vdd"
> 
> So, unless I missed something obvious, your code will call twice
> regulator_disable(msa311->vdd).
> 
> One in devm_add_action_or_reset() and one explicitly after the "if (err)"
> 

You are totally right, thanks a lot for the explanation. I will fix that
in the v5.

> 
> Hoping I'm clear and that I didn't miss something obvious.
> 
> CJ
> 
> > 
> > > > +			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 (%d)\n", err);
> > > 
> > > No need for "(%d)" and err
> > 
> > Asked for the clarification above.
> > 
> > > 
> > > CJ
> > 
>

Dmitry Rokosov Aug. 4, 2022, 6:17 p.m. UTC | #7

Andy,

I have one question about str_read_write() helper, please find it below.

On Wed, Aug 03, 2022 at 07:16:13PM +0000, Dmitry Rokosov wrote:
> Hello Andy,
> 
> Thank you for quick review,
> 
> On Wed, Aug 03, 2022 at 07:49:33PM +0200, Andy Shevchenko wrote:
> > On Wed, Aug 3, 2022 at 3:11 PM Dmitry Rokosov <[e-mail]> 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
> > 
> > Thanks for an update, my comments below.

[...]

> > 
> > ...
> > 
> > > +               dev_err(dev, "cannot %s register %u from debugfs (%d)\n",
> > > +                       readval ? "read" : "write", reg, err);
> > 
> > You may consider taking [1] as a precursor here and use str_read_write().
> > 
> > [1]: https://lore.kernel.org/linux-i2c/[e-mail]/
> 
> Oh, really... Thank you for suggestion!

I have taken it closer, and it's really helpful and nice, but looks like
it's not merged to linux-next.
Please advise how I can use it in the driver. Should I provide
"Depends-On:" tag as I did for my HZ units patchset?

Andy Shevchenko Aug. 4, 2022, 6:28 p.m. UTC | #8

On Wed, Aug 3, 2022 at 9:16 PM Dmitry Rokosov <[e-mail]> wrote:
> On Wed, Aug 03, 2022 at 07:49:33PM +0200, Andy Shevchenko wrote:
> > On Wed, Aug 3, 2022 at 3:11 PM Dmitry Rokosov <[e-mail]> wrote:

...

> > > +static const struct {
> > > +       int val;
> > > +       int val2;
> > > +} msa311_fs_table[] = {
> > > +       {0, 9580}, {0, 19160}, {0, 38320}, {0, 76641}
> > > +};
> >
> > Besides that this struct is defined not only once in the file, this is
> > very well NIH struct s32_fract. Why not use the latter?

> Good point, but looks like this struct is not so popular in the other
> kernel drivers:

It's simply new, it is not about popularity. I would put it as it's
not _yet_ so popular.

...

> grep "s32_fract" -r -l . | wc -l
> 3

Hint: `git grep` much much faster on Git repositories (it goes via
index and not working copy) and see
`git grep -lw s32_fract`

...

> > > +       .cache_type = REGCACHE_RBTREE,
> >
> > Tree hash is good for sparse data, do you really have it sparse (a lot
> > of  big gaps in between)?
>
> I suppose so. MSA311 regmap has ~6 gaps.

Yes and how long is each gap in comparison to the overall space?

...

> > > +       for (fs = 0; fs < ARRAY_SIZE(msa311_fs_table); ++fs)
> >
> > fs++ will work as well.
>
> I would prefer ++fs if you don't mind :)

Why? It's a non-standard pattern, and needs an explanation.

> > > +               /* 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);
> >
> > > +                       if (err)
> > > +                               dev_err(dev, "cannot update scale (%d)\n", err);
> >
> > This can be done after putting the device back into (auto)sleep, right?
> >
>
> Are you talking about dev_err logging? Sure, it can be moved after
> pm_runtime* calls.

Yes.

> > > +                       break;
> > > +               }
> > > +
> > > +       pm_runtime_mark_last_busy(dev);
> > > +       pm_runtime_put_autosuspend(dev);

...

> > > +       for (odr = 0; odr < ARRAY_SIZE(msa311_odr_table); ++odr)
> >
> > odr++ works well also.
>
> I would prefer ++odr if you don't mind :)

See above.

...

> > > +                               dev_err(dev, "cannot update freq (%d)\n", err);
> >
> > frequency
>
> It will be more ugly due 80 symbols restriction.

Nope, it will be understandable by the user. You do code for the user
and then for the developer, right?

...

> > > +               dev_dbg(dev, "found MSA311 compatible chip[%#x]\n", partid);
> >
> > Useless message.
>
> Why? It's under dynamic debug, so I will see it if I really want to.

So what the point of the _successful_ detection? You already know from
the code, that partid, you know by other means that probe was
successful, why this message is needed? Especially for debugging when
we have initcall_debug option.

...

> > > +               return dev_err_probe(dev, err, "cannot disable set0 intrs\n");
> >
> > interrupts
>
> It will be more ugly due 80 symbols restriction.

As above to here and other cryptic messages. Please think about the users first.

...

> > > +       indio_dev->modes = 0; /* setup buffered mode later */
> >
> > Why explicit assignment to 0? Doesn't kzalloc() do it for you?
>
> kzalloc() will do it for me, of course. Previously, I initialized modes to
> INDIO_DIRECT_MODE to just provide default value for that. Jonathan
> suggested to replace it with 0. I can remove this line at all, no problem.
> I just thought, it's more readable.

You may leave comment without assignment explaining that IIO core will
set the buffered mode.

Andy Shevchenko Aug. 4, 2022, 6:30 p.m. UTC | #9

On Thu, Aug 4, 2022 at 8:17 PM Dmitry Rokosov <[e-mail]> wrote:
> On Wed, Aug 03, 2022 at 07:16:13PM +0000, Dmitry Rokosov wrote:
> > On Wed, Aug 03, 2022 at 07:49:33PM +0200, Andy Shevchenko wrote:
> > > On Wed, Aug 3, 2022 at 3:11 PM Dmitry Rokosov <[e-mail]> wrote:

Please, remove unneeded context when replying!

...

> > > > +               dev_err(dev, "cannot %s register %u from debugfs (%d)\n",
> > > > +                       readval ? "read" : "write", reg, err);
> > >
> > > You may consider taking [1] as a precursor here and use str_read_write().
> > >
> > > [1]: https://lore.kernel.org/linux-i2c/[e-mail]/
> >
> > Oh, really... Thank you for suggestion!
>
> I have taken it closer, and it's really helpful and nice, but looks like
> it's not merged to linux-next.
> Please advise how I can use it in the driver. Should I provide
> "Depends-On:" tag as I did for my HZ units patchset?

No, just take that patch into your series.

Dmitry Rokosov Aug. 5, 2022, 2:04 p.m. UTC | #10

On Thu, Aug 04, 2022 at 08:30:12PM +0200, Andy Shevchenko wrote:
> > > > > +               dev_err(dev, "cannot %s register %u from debugfs (%d)\n",
> > > > > +                       readval ? "read" : "write", reg, err);
> > > >
> > > > You may consider taking [1] as a precursor here and use str_read_write().
> > > >
> > > > [1]: https://lore.kernel.org/linux-i2c/[e-mail]/
> > >
> > > Oh, really... Thank you for suggestion!
> >
> > I have taken it closer, and it's really helpful and nice, but looks like
> > it's not merged to linux-next.
> > Please advise how I can use it in the driver. Should I provide
> > "Depends-On:" tag as I did for my HZ units patchset?
> 
> No, just take that patch into your series.

Do you mean include your patch to this reply-thread through the
message-id linking? If my understanding is correct, maybe also it's better to
include HZ units series to the current series? I mean patch series on
below link:

https://lore.kernel.org/linux-iio/[e-mail]/

Dmitry Rokosov Aug. 5, 2022, 2:19 p.m. UTC | #11

On Thu, Aug 04, 2022 at 08:28:28PM +0200, Andy Shevchenko wrote:
> > > > +static const struct {
> > > > +       int val;
> > > > +       int val2;
> > > > +} msa311_fs_table[] = {
> > > > +       {0, 9580}, {0, 19160}, {0, 38320}, {0, 76641}
> > > > +};
> > >
> > > Besides that this struct is defined not only once in the file, this is
> > > very well NIH struct s32_fract. Why not use the latter?
> 
> > Good point, but looks like this struct is not so popular in the other
> > kernel drivers:
> 
> It's simply new, it is not about popularity. I would put it as it's
> not _yet_ so popular.

Okay, no problem. I've already reworked it to s32_fract locally.

> 
> ...
> 
> > grep "s32_fract" -r -l . | wc -l
> > 3
> 
> Hint: `git grep` much much faster on Git repositories (it goes via
> index and not working copy) and see
> `git grep -lw s32_fract`
> 

Thank you for the hint. Actually I'm using ripgrep for the kernel
fuzzysearching, it's faster than git grep. Here I gave an example based
on grep command, because it's general shell command for the searching
substrings I suppose.

> ...
> 
> > > > +       .cache_type = REGCACHE_RBTREE,
> > >
> > > Tree hash is good for sparse data, do you really have it sparse (a lot
> > > of  big gaps in between)?
> >
> > I suppose so. MSA311 regmap has ~6 gaps.
> 
> Yes and how long is each gap in comparison to the overall space?

The longest gap is 0xb bytes.
> 
> ...
> 
> > > > +       for (fs = 0; fs < ARRAY_SIZE(msa311_fs_table); ++fs)
> > >
> > > fs++ will work as well.
> >
> > I would prefer ++fs if you don't mind :)
> 
> Why? It's a non-standard pattern, and needs an explanation.
> 

From statistics point of view, you are right :)

$ rg "for" | rg "\+\+[a-z]" | wc -l
7271
$ rg "for" | rg "[a-z]\+\+" | wc -l
81247

[...]

> > > > +                               dev_err(dev, "cannot update freq (%d)\n", err);
> > >
> > > frequency
> >
> > It will be more ugly due 80 symbols restriction.
> 
> Nope, it will be understandable by the user. You do code for the user
> and then for the developer, right?

Sure, that's good point.

[...]

> 
> ...
> 
> > > > +               dev_dbg(dev, "found MSA311 compatible chip[%#x]\n", partid);
> > >
> > > Useless message.
> >
> > Why? It's under dynamic debug, so I will see it if I really want to.
> 
> So what the point of the _successful_ detection? You already know from
> the code, that partid, you know by other means that probe was
> successful, why this message is needed? Especially for debugging when
> we have initcall_debug option.
> 

Agreed

[...]

Andy Shevchenko Aug. 5, 2022, 4:03 p.m. UTC | #12

On Fri, Aug 5, 2022 at 4:04 PM Dmitry Rokosov <[e-mail]> wrote:
>
> On Thu, Aug 04, 2022 at 08:30:12PM +0200, Andy Shevchenko wrote:
> > > > > > +               dev_err(dev, "cannot %s register %u from debugfs (%d)\n",
> > > > > > +                       readval ? "read" : "write", reg, err);
> > > > >
> > > > > You may consider taking [1] as a precursor here and use str_read_write().
> > > > >
> > > > > [1]: https://lore.kernel.org/linux-i2c/[e-mail]/
> > > >
> > > > Oh, really... Thank you for suggestion!
> > >
> > > I have taken it closer, and it's really helpful and nice, but looks like
> > > it's not merged to linux-next.
> > > Please advise how I can use it in the driver. Should I provide
> > > "Depends-On:" tag as I did for my HZ units patchset?
> >
> > No, just take that patch into your series.
>
> Do you mean include your patch to this reply-thread through the
> message-id linking?

No, just take it as a part of your series. Ah, I wrote almost the same
thing above...

The idea is you rebase your tree interactively and put a breakpoint to
the beginning of your series, then you take a link and run `b4 am -s
...` (-s is important) followed by `git am ...` (b4 will show you the
command you need to run). Then you continue your rebasing. Now, when
you send a new version of the series, take one more patch into it by
changing depth from 3 (the number of patches in your series) to 4 (+
my patch).

Generally speaking the HZ series is something different. It's a series
which can't be simply taken because it might touch the different
subsystem(s). Luckily for you the "different subsystem(s)" is the same
subsystem you are taking these patches with. Hence it might not be a
problem to attach it as well into a chain.

Speaking of lib/ code almost any maintainer can take it via their
trees. So taking a _single_ patch against lib/ is fine in most cases.

Dmitry Rokosov Aug. 5, 2022, 4:20 p.m. UTC | #13

On Fri, Aug 05, 2022 at 06:03:12PM +0200, Andy Shevchenko wrote:
> On Fri, Aug 5, 2022 at 4:04 PM Dmitry Rokosov <[e-mail]> wrote:
> >
> > On Thu, Aug 04, 2022 at 08:30:12PM +0200, Andy Shevchenko wrote:
> > > > > > > +               dev_err(dev, "cannot %s register %u from debugfs (%d)\n",
> > > > > > > +                       readval ? "read" : "write", reg, err);
> > > > > >
> > > > > > You may consider taking [1] as a precursor here and use str_read_write().
> > > > > >
> > > > > > [1]: https://lore.kernel.org/linux-i2c/[e-mail]/
> > > > >
> > > > > Oh, really... Thank you for suggestion!
> > > >
> > > > I have taken it closer, and it's really helpful and nice, but looks like
> > > > it's not merged to linux-next.
> > > > Please advise how I can use it in the driver. Should I provide
> > > > "Depends-On:" tag as I did for my HZ units patchset?
> > >
> > > No, just take that patch into your series.
> >
> > Do you mean include your patch to this reply-thread through the
> > message-id linking?
> 
> No, just take it as a part of your series. Ah, I wrote almost the same
> thing above...
> 
> The idea is you rebase your tree interactively and put a breakpoint to
> the beginning of your series, then you take a link and run `b4 am -s
> ...` (-s is important) followed by `git am ...` (b4 will show you the
> command you need to run). Then you continue your rebasing. Now, when
> you send a new version of the series, take one more patch into it by
> changing depth from 3 (the number of patches in your series) to 4 (+
> my patch).
> 
> Generally speaking the HZ series is something different. It's a series
> which can't be simply taken because it might touch the different
> subsystem(s). Luckily for you the "different subsystem(s)" is the same
> subsystem you are taking these patches with. Hence it might not be a
> problem to attach it as well into a chain.
> 
> Speaking of lib/ code almost any maintainer can take it via their
> trees. So taking a _single_ patch against lib/ is fine in most cases.

Oh, got it. Thanks a lot for detailed explanation. I'll attach both of
them: my HZ units series and your str_read_write() patchset.

Andy Shevchenko Aug. 5, 2022, 5:59 p.m. UTC | #14

On Fri, Aug 5, 2022 at 6:21 PM Dmitry Rokosov <[e-mail]> wrote:
> On Fri, Aug 05, 2022 at 06:03:12PM +0200, Andy Shevchenko wrote:

...

> Oh, got it. Thanks a lot for detailed explanation. I'll attach both of
> them: my HZ units series and your str_read_write() patchset.

s/patchset/single patch/
The I2C patches mustn't be included (they are actually rejected,
that's why you haven't seen the first patch in the Linux Next).

Jonathan Cameron Aug. 6, 2022, 2:55 p.m. UTC | #15

> 
> [...]
> 
> > > +               return dev_err_probe(dev, err, "cannot enable push-pull int\n");  
> > 
> > interrupt  
> 
> It will be more ugly due 80 symbols restriction.

These days we let that stretch a little for cases like these.

> 
> > > +       indio_dev->modes = 0; /* setup buffered mode later */  
> > 
> > Why explicit assignment to 0? Doesn't kzalloc() do it for you?  
> 
> kzalloc() will do it for me, of course. Previously, I initialized modes to
> INDIO_DIRECT_MODE to just provide default value for that. Jonathan
> suggested to replace it with 0. 

I did?  I wonder what I was smoking that day. 
Should be set to INDIO_DIRECT_MODE as you had it previously.

(From what I recall it will work either way but we have in the past had
core code that checked this and may do again in the future so drivers should
still be setting it to specify they provide sysfs interfaces to directly read
the channels).

> I can remove this line at all, no problem.
> I just thought, it's more readable.

Jonathan Cameron Aug. 6, 2022, 3:32 p.m. UTC | #16

On Wed, 3 Aug 2022 13:11:25 +0000
Dmitry Rokosov <[e-mail]> 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 <[e-mail]>
Hi Dmitry, 

A few small things I noticed whilst taking a quick look.

> ---

...

> +
> +/* Register map */
I'm not a big fan of 'structure of code' type comments like this one.
They tend to become wrong remarkably quickly. So unless they
are adding significant value, I would drop them.

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

> +/**
> + * 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)
> + * @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; /* state guard */

Shouldn't need this comment given documentation above that provides
more information.

> +
> +	struct iio_trigger *new_data_trig;
> +	struct regulator *vdd;
> +};
>


> +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 of based on
> +	 * specification it's cleared automatically after a fixed time.
> +	 * So just check that is enabled by driver logic.

That is going to be very problematic if we can have this and events coming
through the same interrupt pin.  Not harmful for now though given you are
only supporting NEW_DATA for now.  Just something to watch out for.

> +	 */
> +	err = regmap_field_read(msa311->fields[F_NEW_DATA_INT_EN],
> +				&new_data_int_enabled);
> +
> +	/* TODO: check motion interrupts status */
> +
> +	mutex_unlock(&msa311->lock);
> +	if (err) {
> +		dev_err(dev, "cannot read new_data int state (%d)\n", err);
> +		return IRQ_NONE;
> +	}
> +
> +	if (new_data_int_enabled)
> +		iio_trigger_poll_chained(msa311->new_data_trig);
> +
> +	/* TODO: send motion events if needed */
> +
> +	return IRQ_HANDLED;
> +}
> +

> +
> +static int msa311_check_partid(struct msa311_priv *msa311)
> +{
> +	struct device *dev = msa311->dev;
> +	unsigned int partid;
> +	int err;
> +
> +	err = regmap_read(msa311->regs, MSA311_PARTID_REG, &partid);
> +	if (err)
> +		return dev_err_probe(dev, err,
> +				     "failed to read partid (%d)\n", err);
> +
> +	if (partid == MSA311_WHO_AM_I)
> +		dev_dbg(dev, "found MSA311 compatible chip[%#x]\n", partid);

I think I'd drop this dev_dbg as in most cases other tracing options + absence
of the warning for when it doesn't match will be enough information.

> +	else
> +		dev_warn(dev, "invalid partid (%#x), expected (%#x)\n",
> +			 partid, MSA311_WHO_AM_I);
> +
> +	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,
> +					i2c->name,

As below. I'd prefer the name hardcoded over relying on particular
firmwares having written the i2c name.

> +					indio_dev);
> +	if (err)
> +		return dev_err_probe(dev, err, "failed to request irq\n");
> +
> +	trig = devm_iio_trigger_alloc(dev, "%s-new-data", i2c->name);
here too.
> +	if (!trig)
> +		return dev_err_probe(dev, -ENOMEM,
> +				     "cannot allocate newdata trig\n");
> +
> +	msa311->new_data_trig = trig;
> +	msa311->new_data_trig->dev.parent = dev;

This is set by the IIO core as part of devm_iio_trigger_alloc()
We moved it to allocation (from registration) very recently
to simplify the few cases where it mattered. I don't think it did matter
in this particular case though - shouldn't have been in this driver either
way!

> +	msa311->new_data_trig->ops = &msa311_new_data_trig_ops;
> +	iio_trigger_set_drvdata(msa311->new_data_trig, indio_dev);
> +
> +	err = devm_iio_trigger_register(dev, msa311->new_data_trig);
> +	if (err)
> +		return dev_err_probe(dev, err, "can't register newdata trig\n");
> +
> +	err = regmap_field_write(msa311->fields[F_INT1_OD],
> +				 MSA311_INT1_OD_PUSH_PULL);
> +	if (err)
> +		return dev_err_probe(dev, err, "cannot enable push-pull int\n");
> +
> +	err = regmap_field_write(msa311->fields[F_INT1_LVL],
> +				 MSA311_INT1_LVL_HIGH);
> +	if (err)
> +		return dev_err_probe(dev, err, "cannot set active int level\n");
> +
> +	err = regmap_field_write(msa311->fields[F_LATCH_INT],
> +				 MSA311_LATCH_INT_LATCHED);
> +	if (err)
> +		return dev_err_probe(dev, err, "cannot latch int\n");
> +
> +	err = regmap_field_write(msa311->fields[F_RESET_INT], 1);
> +	if (err)
> +		return dev_err_probe(dev, err, "cannot reset int\n");
> +
> +	err = regmap_field_write(msa311->fields[F_INT1_NEW_DATA], 1);
> +	if (err)
> +		return dev_err_probe(dev, err, "cannot map new data int\n");
> +
> +	return 0;
> +}
> +

> +
> +static int msa311_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) {
> +			regulator_disable(msa311->vdd);

Don't need the regulator_disable() call here. If devm_add_action_or_reset() fails
it will always call the callback (that's how it differs from devm_add_action())


> +			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 (%d)\n", err);
> +
> +	/*
> +	 * 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");
> +

Probably want a pm_runtime_set_active() call here to let runtime PM know we
are starting it up with the chip turned on.  runtime pm gives me a headache
though so possible you don't strictly need this.

> +	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 = 0; /* setup buffered mode later */

As per other branch, I led you astray here it seems.

> +	indio_dev->channels = msa311_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(msa311_channels);
> +	indio_dev->name = i2c->name;

I'd rather see the part number encoded directly here as it should only ever
take one value (and that value should be based on the chipID once we have
multiple chipIDs supported - not what the firmware claims it is...)

> +	indio_dev->info = &msa311_info;
> +
> +	err = devm_iio_triggered_buffer_setup(dev,
> +					      indio_dev,
> +					      iio_pollfunc_store_time,
> +					      msa311_buffer_thread,
> +					      &msa311_buffer_setup_ops);
> +	if (err)
> +		return dev_err_probe(dev, err, "cannot setup iio trig buf\n");
> +
> +	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;
> +}

...

Dmitry Rokosov Aug. 9, 2022, 9:47 a.m. UTC | #17

Hello Jonathan,

On Sat, Aug 06, 2022 at 04:32:04PM +0100, Jonathan Cameron wrote:

[...]

> > +/**
> > + * 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)
> > + * @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; /* state guard */
> 
> Shouldn't need this comment given documentation above that provides
> more information.

Without this comment checkpatch.pl raises a warning about uncommented
lock definition.
I agree with you, above comment is redundant, but is it okay to ignore
such warnings before sending the patch?

I'm talking about below checkpatch condition:
=====
# check for spinlock_t definitions without a comment.
		if ($line =~ /^.\s*(struct\s+mutex|spinlock_t)\s+\S+;/ ||
		    $line =~ /^.\s*(DEFINE_MUTEX)\s*\(/) {
			my $which = $1;
			if (!ctx_has_comment($first_line, $linenr)) {
				CHK("UNCOMMENTED_DEFINITION",
				    "$1 definition without comment\n" . $herecurr);
			}
		}
=====

> 
> > +
> > +	struct iio_trigger *new_data_trig;
> > +	struct regulator *vdd;
> > +};
> >
> 
> 
> > +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 of based on
> > +	 * specification it's cleared automatically after a fixed time.
> > +	 * So just check that is enabled by driver logic.
> 
> That is going to be very problematic if we can have this and events coming
> through the same interrupt pin.  Not harmful for now though given you are
> only supporting NEW_DATA for now.  Just something to watch out for.
> 

Actually, I have run some experiments with NEW_DATA status bits. And
looks like we can't determince actual status of NEW_DATA virtual
interrupt when physical IRQ is raised. I will back to this problem when
begin Motion Events feature implementation.

[...]

> > +	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 = 0; /* setup buffered mode later */
> 
> As per other branch, I led you astray here it seems.
> 

Sorry, I've made a mistake. Comment about INDIO_DIRECT_MODE was left
by Andy here:

https://lore.kernel.org/linux-iio/[e-mail]/

[...]

Dmitry Rokosov Aug. 9, 2022, 9:52 a.m. UTC | #18

On Sat, Aug 06, 2022 at 03:55:23PM +0100, Jonathan Cameron wrote:

[...]

> > 
> > > > +       indio_dev->modes = 0; /* setup buffered mode later */  
> > > 
> > > Why explicit assignment to 0? Doesn't kzalloc() do it for you?  
> > 
> > kzalloc() will do it for me, of course. Previously, I initialized modes to
> > INDIO_DIRECT_MODE to just provide default value for that. Jonathan
> > suggested to replace it with 0. 
> 
> I did?  I wonder what I was smoking that day. 
> Should be set to INDIO_DIRECT_MODE as you had it previously.
> 
> (From what I recall it will work either way but we have in the past had
> core code that checked this and may do again in the future so drivers should
> still be setting it to specify they provide sysfs interfaces to directly read
> the channels).

Jonathan, really sorry I referred to you. I'm confused. This comment was
from Andy in the v3 discussion:

https://lore.kernel.org/linux-iio/[e-mail]/

I will revert this change. Thank you for feedback.

Andy Shevchenko Aug. 9, 2022, 10:05 a.m. UTC | #19

On Tue, Aug 9, 2022 at 11:52 AM Dmitry Rokosov <[e-mail]> wrote:
> On Sat, Aug 06, 2022 at 03:55:23PM +0100, Jonathan Cameron wrote:
> > > > > +       indio_dev->modes = 0; /* setup buffered mode later */
> > > >
> > > > Why explicit assignment to 0? Doesn't kzalloc() do it for you?
> > >
> > > kzalloc() will do it for me, of course. Previously, I initialized modes to
> > > INDIO_DIRECT_MODE to just provide default value for that. Jonathan
> > > suggested to replace it with 0.
> >
> > I did?  I wonder what I was smoking that day.
> > Should be set to INDIO_DIRECT_MODE as you had it previously.
> >
> > (From what I recall it will work either way but we have in the past had
> > core code that checked this and may do again in the future so drivers should
> > still be setting it to specify they provide sysfs interfaces to directly read
> > the channels).
>
> Jonathan, really sorry I referred to you. I'm confused. This comment was
> from Andy in the v3 discussion:
>
> https://lore.kernel.org/linux-iio/[e-mail]/

Indeed. I was confused by the comment. My understanding at that time
was that the triggered mode is inevitable and hence assigning to
something which _will_ be reassigned later makes a little sense. So,
does it mean that triggered mode is optional and might not be set? In
such a case the comment is misleading.

Dmitry Rokosov Aug. 9, 2022, 10:35 a.m. UTC | #20

On Tue, Aug 09, 2022 at 12:05:12PM +0200, Andy Shevchenko wrote:
> > > > > > +       indio_dev->modes = 0; /* setup buffered mode later */
> > > > >
> > > > > Why explicit assignment to 0? Doesn't kzalloc() do it for you?
> > > >
> > > > kzalloc() will do it for me, of course. Previously, I initialized modes to
> > > > INDIO_DIRECT_MODE to just provide default value for that. Jonathan
> > > > suggested to replace it with 0.
> > >
> > > I did?  I wonder what I was smoking that day.
> > > Should be set to INDIO_DIRECT_MODE as you had it previously.
> > >
> > > (From what I recall it will work either way but we have in the past had
> > > core code that checked this and may do again in the future so drivers should
> > > still be setting it to specify they provide sysfs interfaces to directly read
> > > the channels).
> >
> > Jonathan, really sorry I referred to you. I'm confused. This comment was
> > from Andy in the v3 discussion:
> >
> > https://lore.kernel.org/linux-iio/[e-mail]/
> 
> Indeed. I was confused by the comment. My understanding at that time
> was that the triggered mode is inevitable and hence assigning to
> something which _will_ be reassigned later makes a little sense. So,
> does it mean that triggered mode is optional and might not be set? In
> such a case the comment is misleading.

Actually, this comment was introduced in the early MSA311 driver
versions, when I have made buffer setup only if HW irq is enabled. In
the newest versions buffer is setup unconditionally, because buffer mode
can be used based on hrtimer software trigger.

Jonathan, why we shouldn't delete INDIO_DIRECT_MODE initialization if
after couple of lines we always setup buffer mode?

Jonathan Cameron Aug. 13, 2022, 3:27 p.m. UTC | #21

On Tue, 9 Aug 2022 10:35:19 +0000
Dmitry Rokosov <[e-mail]> wrote:

> On Tue, Aug 09, 2022 at 12:05:12PM +0200, Andy Shevchenko wrote:
> > > > > > > +       indio_dev->modes = 0; /* setup buffered mode later */  
> > > > > >
> > > > > > Why explicit assignment to 0? Doesn't kzalloc() do it for you?  
> > > > >
> > > > > kzalloc() will do it for me, of course. Previously, I initialized modes to
> > > > > INDIO_DIRECT_MODE to just provide default value for that. Jonathan
> > > > > suggested to replace it with 0.  
> > > >
> > > > I did?  I wonder what I was smoking that day.
> > > > Should be set to INDIO_DIRECT_MODE as you had it previously.
> > > >
> > > > (From what I recall it will work either way but we have in the past had
> > > > core code that checked this and may do again in the future so drivers should
> > > > still be setting it to specify they provide sysfs interfaces to directly read
> > > > the channels).  
> > >
> > > Jonathan, really sorry I referred to you. I'm confused. This comment was
> > > from Andy in the v3 discussion:
> > >
> > > https://lore.kernel.org/linux-iio/[e-mail]/  
> > 
> > Indeed. I was confused by the comment. My understanding at that time
> > was that the triggered mode is inevitable and hence assigning to
> > something which _will_ be reassigned later makes a little sense. So,
> > does it mean that triggered mode is optional and might not be set? In
> > such a case the comment is misleading.  
> 
> Actually, this comment was introduced in the early MSA311 driver
> versions, when I have made buffer setup only if HW irq is enabled. In
> the newest versions buffer is setup unconditionally, because buffer mode
> can be used based on hrtimer software trigger.
> 
> Jonathan, why we shouldn't delete INDIO_DIRECT_MODE initialization if
> after couple of lines we always setup buffer mode?
> 

The buffered mode setup does
modes |= INDIO_BUFFER_TRIGGERED;
https://elixir.bootlin.com/linux/latest/source/drivers/iio/buffer/industrialio-triggered-buffer.c#L71

Direct mode indicates that it is possible to read the channels without
using any of the triggered modes (there are devices - though rare - where
it is not set as they are only accessible through FIFOs for example).

We don't make much use of IIO_DIRECT_MODE today (though we did until fairly
recently).  It could be replaced with a specific check on provision of
raw / processed channels I guess - but I'm not that keen to see it go without
thinking hard about whether we should be using that flag to catch misconfiguration
in some cases.  So I'd rather postpone any changes in that for now.

Jonathan

Jonathan Cameron Aug. 13, 2022, 3:34 p.m. UTC | #22

On Tue, 9 Aug 2022 09:47:54 +0000
Dmitry Rokosov <[e-mail]> wrote:

> Hello Jonathan,
> 
> On Sat, Aug 06, 2022 at 04:32:04PM +0100, Jonathan Cameron wrote:
> 
> [...]
> 
> > > +/**
> > > + * 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)
> > > + * @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; /* state guard */  
> > 
> > Shouldn't need this comment given documentation above that provides
> > more information.  
> 
> Without this comment checkpatch.pl raises a warning about uncommented
> lock definition.
> I agree with you, above comment is redundant, but is it okay to ignore
> such warnings before sending the patch?
> 
> I'm talking about below checkpatch condition:
> =====
> # check for spinlock_t definitions without a comment.
> 		if ($line =~ /^.\s*(struct\s+mutex|spinlock_t)\s+\S+;/ ||
> 		    $line =~ /^.\s*(DEFINE_MUTEX)\s*\(/) {
> 			my $which = $1;
> 			if (!ctx_has_comment($first_line, $linenr)) {
> 				CHK("UNCOMMENTED_DEFINITION",
> 				    "$1 definition without comment\n" . $herecurr);
> 			}
> 		}
> =====

Hmm. I guess checkpatch is more stupid than I thought on this. Definitely
fine to ignore that shortcoming of checkpatch.

> 
> >   
> > > +
> > > +	struct iio_trigger *new_data_trig;
> > > +	struct regulator *vdd;
> > > +};
> > >  
> > 
> >   
> > > +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 of based on
> > > +	 * specification it's cleared automatically after a fixed time.
> > > +	 * So just check that is enabled by driver logic.  
> > 
> > That is going to be very problematic if we can have this and events coming
> > through the same interrupt pin.  Not harmful for now though given you are
> > only supporting NEW_DATA for now.  Just something to watch out for.
> >   
> 
> Actually, I have run some experiments with NEW_DATA status bits. And
> looks like we can't determince actual status of NEW_DATA virtual
> interrupt when physical IRQ is raised. I will back to this problem when
> begin Motion Events feature implementation.
> 
> [...]
> 
> > > +	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 = 0; /* setup buffered mode later */  
> > 
> > As per other branch, I led you astray here it seems.
> >   
> 
> Sorry, I've made a mistake. Comment about INDIO_DIRECT_MODE was left
> by Andy here:
> 
> https://lore.kernel.org/linux-iio/[e-mail]/
> 
> [...]

No problem. That's an odd historical corner case.   I liked having
clear values for 'currentmode' (now in iio_opaque) and those matching
the bits set in available modes.  So even if we didn't set it we'd end up
with a reserved bit which would add extra confusion.

The direct mode is currently just used as a placeholder for 'not a buffered mode',
rather than the state variable has never been set.

Jonathan

>

Jonathan Cameron Aug. 13, 2022, 3:39 p.m. UTC | #23

On Sat, 13 Aug 2022 16:27:15 +0100
Jonathan Cameron <[e-mail]> wrote:

> On Tue, 9 Aug 2022 10:35:19 +0000
> Dmitry Rokosov <[e-mail]> wrote:
> 
> > On Tue, Aug 09, 2022 at 12:05:12PM +0200, Andy Shevchenko wrote:  
> > > > > > > > +       indio_dev->modes = 0; /* setup buffered mode later */    
> > > > > > >
> > > > > > > Why explicit assignment to 0? Doesn't kzalloc() do it for you?    
> > > > > >
> > > > > > kzalloc() will do it for me, of course. Previously, I initialized modes to
> > > > > > INDIO_DIRECT_MODE to just provide default value for that. Jonathan
> > > > > > suggested to replace it with 0.    
> > > > >
> > > > > I did?  I wonder what I was smoking that day.
> > > > > Should be set to INDIO_DIRECT_MODE as you had it previously.
> > > > >
> > > > > (From what I recall it will work either way but we have in the past had
> > > > > core code that checked this and may do again in the future so drivers should
> > > > > still be setting it to specify they provide sysfs interfaces to directly read
> > > > > the channels).    
> > > >
> > > > Jonathan, really sorry I referred to you. I'm confused. This comment was
> > > > from Andy in the v3 discussion:
> > > >
> > > > https://lore.kernel.org/linux-iio/[e-mail]/    
> > > 
> > > Indeed. I was confused by the comment. My understanding at that time
> > > was that the triggered mode is inevitable and hence assigning to
> > > something which _will_ be reassigned later makes a little sense. So,
> > > does it mean that triggered mode is optional and might not be set? In
> > > such a case the comment is misleading.    
> > 
> > Actually, this comment was introduced in the early MSA311 driver
> > versions, when I have made buffer setup only if HW irq is enabled. In
> > the newest versions buffer is setup unconditionally, because buffer mode
> > can be used based on hrtimer software trigger.
> > 
> > Jonathan, why we shouldn't delete INDIO_DIRECT_MODE initialization if
> > after couple of lines we always setup buffer mode?
> >   
> 
> The buffered mode setup does
> modes |= INDIO_BUFFER_TRIGGERED;
> https://elixir.bootlin.com/linux/latest/source/drivers/iio/buffer/industrialio-triggered-buffer.c#L71
> 
> Direct mode indicates that it is possible to read the channels without
> using any of the triggered modes (there are devices - though rare - where
> it is not set as they are only accessible through FIFOs for example).
> 
> We don't make much use of IIO_DIRECT_MODE today (though we did until fairly
> recently).  It could be replaced with a specific check on provision of
> raw / processed channels I guess - but I'm not that keen to see it go without
> thinking hard about whether we should be using that flag to catch misconfiguration
> in some cases.  So I'd rather postpone any changes in that for now.

I remembered a bit more about this.  INDIO_DIRECT_MODE is used internally
in the state machine for currentmode (now moved to the opaque structure but
accessible via appropriate function). It's there to provide an explicit
state rather than 0 which would be not in a particular state (that I don't
like).  We'd have a somewhat odd (though possible) disconnect if we didn't
have the state in the ->modes bitmask.  (We could reserve the first bit
for example). However I'm still not keen in cleaning it up further.

Either way, we'd still need the various mode setting functions to carry only
doing |= as the driver is permitted to support multiple buffered modes
(unusual, but there are a few drivers doing so IIRC).

J
> 
> Jonathan

Patch

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 <[e-mail]>
+L:	[e-mail]
+S:	Maintained
+F:	drivers/iio/accel/msa311.c
+
 MEN A21 WATCHDOG DRIVER
 M:	Johannes Thumshirn <[e-mail]>
 L:	[e-mail]
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..763e16a5d7ae
--- /dev/null
+++ b/drivers/iio/accel/msa311.c
@@ -0,0 +1,1323 @@ 
+// 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 <[e-mail]>
+ */
+
+#include <linux/i2c.h>
+#include <linux/iio/buffer.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+#include <linux/iio/trigger.h>
+#include <linux/iio/trigger_consumer.h>
+#include <linux/iio/triggered_buffer.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/pm.h>
+#include <linux/pm_runtime.h>
+#include <linux/regmap.h>
+#include <linux/string_helpers.h>
+#include <linux/units.h>
+
+/* Register map */
+
+#define MSA311_SOFT_RESET_REG     0x00
+#define MSA311_PARTID_REG         0x01
+#define MSA311_ACC_X_REG          0x02
+#define MSA311_ACC_Y_REG          0x04
+#define MSA311_ACC_Z_REG          0x06
+#define MSA311_MOTION_INT_REG     0x09
+#define MSA311_DATA_INT_REG       0x0A
+#define MSA311_TAP_ACTIVE_STS_REG 0x0B
+#define MSA311_ORIENT_STS_REG     0x0C
+#define MSA311_RANGE_REG          0x0F
+#define MSA311_ODR_REG            0x10
+#define MSA311_PWR_MODE_REG       0x11
+#define MSA311_SWAP_POLARITY_REG  0x12
+#define MSA311_INT_SET_0_REG      0x16
+#define MSA311_INT_SET_1_REG      0x17
+#define MSA311_INT_MAP_0_REG      0x19
+#define MSA311_INT_MAP_1_REG      0x1A
+#define MSA311_INT_CONFIG_REG     0x20
+#define MSA311_INT_LATCH_REG      0x21
+#define MSA311_FREEFALL_DUR_REG   0x22
+#define MSA311_FREEFALL_TH_REG    0x23
+#define MSA311_FREEFALL_HY_REG    0x24
+#define MSA311_ACTIVE_DUR_REG     0x27
+#define MSA311_ACTIVE_TH_REG      0x28
+#define MSA311_TAP_DUR_REG        0x2A
+#define MSA311_TAP_TH_REG         0x2B
+#define MSA311_ORIENT_HY_REG      0x2C
+#define MSA311_Z_BLOCK_REG        0x2D
+#define MSA311_OFFSET_X_REG       0x38
+#define MSA311_OFFSET_Y_REG       0x39
+#define MSA311_OFFSET_Z_REG       0x3A
+
+enum msa311_fields {
+	/* 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)
+ * @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; /* state guard */
+
+	struct iio_trigger *new_data_trig;
+	struct regulator *vdd;
+};
+
+/* Channels */
+
+enum msa311_si {
+	MSA311_SI_X,
+	MSA311_SI_Y,
+	MSA311_SI_Z,
+	MSA311_SI_TIMESTAMP,
+};
+
+#define MSA311_ACCEL_CHANNEL(axis) {                                        \
+	.type = IIO_ACCEL,                                                  \
+	.modified = 1,                                                      \
+	.channel2 = IIO_MOD_##axis,                                         \
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),                       \
+	.info_mask_shared_by_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,
+			"failed to set odr %u.%uHz, not available in %s mode\n",
+			msa311_odr_table[odr].val,
+			msa311_odr_table[odr].val2 / 1000,
+			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 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 freq (%d)\n", err);
+		return err;
+	}
+
+	/*
+	 * After msa311 resuming is done, we need to wait for data
+	 * to be refreshed by accel logic.
+	 * A certain timeout is calculated based on the current ODR value.
+	 * If requested timeout isn't so long (let's assume 20ms),
+	 * we can wait for next data in uninterruptible sleep.
+	 */
+	freq_uhz = msa311_odr_table[odr].val * 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 (%d)\n",
+			chan->datasheet_name, err);
+		return err;
+	}
+
+	pm_runtime_mark_last_busy(dev);
+	pm_runtime_put_autosuspend(dev);
+
+	/*
+	 * Axis data format is:
+	 * ACC_X = (ACC_X_MSB[7:0] << 4) | ACC_X_LSB[7:4]
+	 */
+	*val = sign_extend32(le16_to_cpu(axis) >> chan->scan_type.shift,
+			     chan->scan_type.realbits - 1);
+
+	return IIO_VAL_INT;
+}
+
+static int msa311_read_scale(struct iio_dev *indio_dev, int *val, int *val2)
+{
+	struct msa311_priv *msa311 = iio_priv(indio_dev);
+	struct device *dev = msa311->dev;
+	unsigned int fs;
+	int err;
+
+	mutex_lock(&msa311->lock);
+	err = regmap_field_read(msa311->fields[F_FS], &fs);
+	mutex_unlock(&msa311->lock);
+	if (err) {
+		dev_err(dev, "cannot get actual scale (%d)\n", err);
+		return err;
+	}
+
+	*val = msa311_fs_table[fs].val;
+	*val2 = msa311_fs_table[fs].val2;
+
+	return IIO_VAL_INT_PLUS_MICRO;
+}
+
+static int msa311_read_samp_freq(struct iio_dev *indio_dev,
+				 int *val, int *val2)
+{
+	struct msa311_priv *msa311 = iio_priv(indio_dev);
+	struct device *dev = msa311->dev;
+	unsigned int odr;
+	int err;
+
+	mutex_lock(&msa311->lock);
+	err = msa311_get_odr(msa311, &odr);
+	mutex_unlock(&msa311->lock);
+	if (err) {
+		dev_err(dev, "cannot get actual freq (%d)\n", err);
+		return err;
+	}
+
+	*val = msa311_odr_table[odr].val;
+	*val2 = msa311_odr_table[odr].val2;
+
+	return IIO_VAL_INT_PLUS_MICRO;
+}
+
+static int msa311_read_raw(struct iio_dev *indio_dev,
+			   struct iio_chan_spec const *chan,
+			   int *val, int *val2, long mask)
+{
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		return msa311_read_raw_data(indio_dev, chan, val, val2);
+
+	case IIO_CHAN_INFO_SCALE:
+		return msa311_read_scale(indio_dev, val, val2);
+
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		return msa311_read_samp_freq(indio_dev, val, val2);
+
+	default:
+		return -EINVAL;
+	}
+}
+
+static int msa311_read_avail(struct iio_dev *indio_dev,
+			     struct iio_chan_spec const *chan,
+			     const int **vals, int *type,
+			     int *length, long mask)
+{
+	switch (mask) {
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		*vals = (int *)msa311_odr_table;
+		*type = IIO_VAL_INT_PLUS_MICRO;
+		/* ODR value has 2 ints (integer and fractional parts) */
+		*length = ARRAY_SIZE(msa311_odr_table) * 2;
+		return IIO_AVAIL_LIST;
+
+	case IIO_CHAN_INFO_SCALE:
+		*vals = (int *)msa311_fs_table;
+		*type = IIO_VAL_INT_PLUS_MICRO;
+		/* FS value has 2 ints (integer and fractional parts) */
+		*length = ARRAY_SIZE(msa311_fs_table) * 2;
+		return IIO_AVAIL_LIST;
+
+	default:
+		return -EINVAL;
+	}
+}
+
+static int msa311_write_scale(struct iio_dev *indio_dev, int val, int val2)
+{
+	struct msa311_priv *msa311 = iio_priv(indio_dev);
+	struct device *dev = msa311->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);
+			if (err)
+				dev_err(dev, "cannot update scale (%d)\n", err);
+			break;
+		}
+
+	pm_runtime_mark_last_busy(dev);
+	pm_runtime_put_autosuspend(dev);
+
+	return err;
+}
+
+static int msa311_write_samp_freq(struct iio_dev *indio_dev, int val, int val2)
+{
+	struct msa311_priv *msa311 = iio_priv(indio_dev);
+	struct device *dev = msa311->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);
+			if (err)
+				dev_err(dev, "cannot update freq (%d)\n", err);
+			break;
+		}
+
+	iio_device_release_direct_mode(indio_dev);
+
+	pm_runtime_mark_last_busy(dev);
+	pm_runtime_put_autosuspend(dev);
+
+	return err;
+}
+
+static int msa311_write_raw(struct iio_dev *indio_dev,
+			    struct iio_chan_spec const *chan,
+			    int val, int val2, long mask)
+{
+	switch (mask) {
+	case IIO_CHAN_INFO_SCALE:
+		return msa311_write_scale(indio_dev, val, val2);
+
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		return msa311_write_samp_freq(indio_dev, val, val2);
+
+	default:
+		return -EINVAL;
+	}
+}
+
+static int msa311_debugfs_reg_access(struct iio_dev *indio_dev,
+				     unsigned int reg, unsigned int writeval,
+				     unsigned int *readval)
+{
+	struct msa311_priv *msa311 = iio_priv(indio_dev);
+	struct device *dev = msa311->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 (%d)\n",
+			readval ? "read" : "write", reg, err);
+
+	return err;
+}
+
+static int msa311_buffer_preenable(struct iio_dev *indio_dev)
+{
+	struct msa311_priv *msa311 = iio_priv(indio_dev);
+	struct device *dev = msa311->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 (%d)\n",
+			str_enable_disable(state), 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 = 0, 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 (%d)\n",
+				chan->datasheet_name, err);
+			goto err;
+		}
+
+		buf.channels[i++] = axis;
+	}
+
+	mutex_unlock(&msa311->lock);
+
+	iio_push_to_buffers_with_timestamp(indio_dev, &buf,
+					   iio_get_time_ns(indio_dev));
+
+err:
+	iio_trigger_notify_done(indio_dev->trig);
+
+	return IRQ_HANDLED;
+}
+
+static irqreturn_t msa311_irq_thread(int irq, void *p)
+{
+	struct msa311_priv *msa311 = iio_priv(p);
+	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 of based on
+	 * specification it's cleared automatically after a fixed time.
+	 * So just check that is enabled by driver logic.
+	 */
+	err = regmap_field_read(msa311->fields[F_NEW_DATA_INT_EN],
+				&new_data_int_enabled);
+
+	/* TODO: check motion interrupts status */
+
+	mutex_unlock(&msa311->lock);
+	if (err) {
+		dev_err(dev, "cannot read new_data int state (%d)\n", err);
+		return IRQ_NONE;
+	}
+
+	if (new_data_int_enabled)
+		iio_trigger_poll_chained(msa311->new_data_trig);
+
+	/* TODO: send motion events if needed */
+
+	return IRQ_HANDLED;
+}
+
+static const struct iio_info msa311_info = {
+	.read_raw = msa311_read_raw,
+	.read_avail = msa311_read_avail,
+	.write_raw = msa311_write_raw,
+	.debugfs_reg_access = msa311_debugfs_reg_access,
+};
+
+static const struct iio_buffer_setup_ops msa311_buffer_setup_ops = {
+	.preenable = msa311_buffer_preenable,
+	.postdisable = msa311_buffer_postdisable,
+};
+
+static const struct iio_trigger_ops msa311_new_data_trig_ops = {
+	.set_trigger_state = msa311_set_new_data_trig_state,
+	.validate_device = msa311_validate_device,
+};
+
+static int msa311_check_partid(struct msa311_priv *msa311)
+{
+	struct device *dev = msa311->dev;
+	unsigned int partid;
+	int err;
+
+	err = regmap_read(msa311->regs, MSA311_PARTID_REG, &partid);
+	if (err)
+		return dev_err_probe(dev, err,
+				     "failed to read partid (%d)\n", err);
+
+	if (partid == MSA311_WHO_AM_I)
+		dev_dbg(dev, "found MSA311 compatible chip[%#x]\n", partid);
+	else
+		dev_warn(dev, "invalid partid (%#x), expected (%#x)\n",
+			 partid, MSA311_WHO_AM_I);
+
+	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 intrs\n");
+
+	err = regmap_write(msa311->regs, MSA311_INT_SET_1_REG, 0);
+	if (err)
+		return dev_err_probe(dev, err, "cannot disable set1 intrs\n");
+
+	/* Unmap all INT1 interrupts by default */
+	err = regmap_write(msa311->regs, MSA311_INT_MAP_0_REG, 0);
+	if (err)
+		return dev_err_probe(dev, err, "failed to unmap map0 intrs\n");
+
+	err = regmap_write(msa311->regs, MSA311_INT_MAP_1_REG, 0);
+	if (err)
+		return dev_err_probe(dev, err, "failed to unmap map1 intrs\n");
+
+	/* Disable all axis by default */
+	err = regmap_update_bits(msa311->regs, MSA311_ODR_REG,
+				 MSA311_GENMASK(F_X_AXIS_DIS) |
+				 MSA311_GENMASK(F_Y_AXIS_DIS) |
+				 MSA311_GENMASK(F_Z_AXIS_DIS), 0);
+	if (err)
+		return dev_err_probe(dev, err, "cannot enable all axes\n");
+
+	err = msa311_set_odr(msa311, MSA311_ODR_125_HZ);
+	if (err)
+		return dev_err_probe(dev, err, "failed to set accel freq\n");
+
+	return 0;
+}
+
+static int msa311_setup_interrupts(struct msa311_priv *msa311)
+{
+	struct 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,
+					i2c->name,
+					indio_dev);
+	if (err)
+		return dev_err_probe(dev, err, "failed to request irq\n");
+
+	trig = devm_iio_trigger_alloc(dev, "%s-new-data", i2c->name);
+	if (!trig)
+		return dev_err_probe(dev, -ENOMEM,
+				     "cannot allocate newdata trig\n");
+
+	msa311->new_data_trig = trig;
+	msa311->new_data_trig->dev.parent = dev;
+	msa311->new_data_trig->ops = &msa311_new_data_trig_ops;
+	iio_trigger_set_drvdata(msa311->new_data_trig, indio_dev);
+
+	err = devm_iio_trigger_register(dev, msa311->new_data_trig);
+	if (err)
+		return dev_err_probe(dev, err, "can't register newdata trig\n");
+
+	err = regmap_field_write(msa311->fields[F_INT1_OD],
+				 MSA311_INT1_OD_PUSH_PULL);
+	if (err)
+		return dev_err_probe(dev, err, "cannot enable push-pull int\n");
+
+	err = regmap_field_write(msa311->fields[F_INT1_LVL],
+				 MSA311_INT1_LVL_HIGH);
+	if (err)
+		return dev_err_probe(dev, err, "cannot set active int level\n");
+
+	err = regmap_field_write(msa311->fields[F_LATCH_INT],
+				 MSA311_LATCH_INT_LATCHED);
+	if (err)
+		return dev_err_probe(dev, err, "cannot latch int\n");
+
+	err = regmap_field_write(msa311->fields[F_RESET_INT], 1);
+	if (err)
+		return dev_err_probe(dev, err, "cannot reset int\n");
+
+	err = regmap_field_write(msa311->fields[F_INT1_NEW_DATA], 1);
+	if (err)
+		return dev_err_probe(dev, err, "cannot map new data int\n");
+
+	return 0;
+}
+
+static int msa311_regmap_init(struct msa311_priv *msa311)
+{
+	struct regmap_field **fields = msa311->fields;
+	struct device *dev = msa311->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 reg 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) {
+			regulator_disable(msa311->vdd);
+			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 (%d)\n", err);
+
+	/*
+	 * 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 = 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 = 0; /* setup buffered mode later */
+	indio_dev->channels = msa311_channels;
+	indio_dev->num_channels = ARRAY_SIZE(msa311_channels);
+	indio_dev->name = i2c->name;
+	indio_dev->info = &msa311_info;
+
+	err = devm_iio_triggered_buffer_setup(dev,
+					      indio_dev,
+					      iio_pollfunc_store_time,
+					      msa311_buffer_thread,
+					      &msa311_buffer_setup_ops);
+	if (err)
+		return dev_err_probe(dev, err, "cannot setup iio trig buf\n");
+
+	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 (%d)\n", 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 (%d)\n", 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 <[e-mail]>");
+MODULE_DESCRIPTION("MEMSensing MSA311 3-axis accelerometer driver");
+MODULE_LICENSE("GPL");