Re: [PATCH v4 5/5] iio: accel: add ADXL367 driver

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

On Sun,  6 Feb 2022 23:13:07 +0200
Cosmin Tanislav <demonsingur@gmail.com> wrote:

> The ADXL367 is an ultralow power, 3-axis MEMS accelerometer.
> 
> The ADXL367 does not alias input signals to achieve ultralow power
> consumption, it samples the full bandwidth of the sensor at all
> data rates. Measurement ranges of +-2g, +-4g, and +-8g are available,
> with a resolution of 0.25mg/LSB on the +-2 g range.
> 
> In addition to its ultralow power consumption, the ADXL367
> has many features to enable true system level power reduction.
> It includes a deep multimode output FIFO, a built-in micropower
> temperature sensor, and an internal ADC for synchronous conversion
> of an additional analog input.
> 
> Signed-off-by: Cosmin Tanislav <cosmin.tanislav@analog.com>

I missed the issue below with the use of available_scan_masks
in v3. Other than that and a few trivial things below, the series
looks good to me.

Thanks,

Jonathan


> ---
>  MAINTAINERS                     |    8 +
>  drivers/iio/accel/Kconfig       |   27 +
>  drivers/iio/accel/Makefile      |    3 +
>  drivers/iio/accel/adxl367.c     | 1585 +++++++++++++++++++++++++++++++
>  drivers/iio/accel/adxl367.h     |   23 +
>  drivers/iio/accel/adxl367_i2c.c |   90 ++
>  drivers/iio/accel/adxl367_spi.c |  164 ++++
>  7 files changed, 1900 insertions(+)
>  create mode 100644 drivers/iio/accel/adxl367.c
>  create mode 100644 drivers/iio/accel/adxl367.h
>  create mode 100644 drivers/iio/accel/adxl367_i2c.c
>  create mode 100644 drivers/iio/accel/adxl367_spi.c
> 


> new file mode 100644
> index 000000000000..cac47db7d89c
> --- /dev/null
> +++ b/drivers/iio/accel/adxl367.c
> @@ -0,0 +1,1585 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (C) 2021 Analog Devices, Inc.
> + * Author: Cosmin Tanislav <cosmin.tanislav@analog.com>
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/bitops.h>
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/events.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/kfifo_buf.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
> +#include <linux/module.h>

#include <linux/mod_devicetable.h>
for the id tables.

> +#include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>
> +#include <asm/unaligned.h>
> +
> +#include "adxl367.h"
> +




> +static const unsigned long adxl367_channel_masks[] = {
> +	[ADXL367_FIFO_FORMAT_XYZ]  = ADXL367_X_CHANNEL_MASK
> +				     | ADXL367_Y_CHANNEL_MASK
> +				     | ADXL367_Z_CHANNEL_MASK,
> +	[ADXL367_FIFO_FORMAT_X]    = ADXL367_X_CHANNEL_MASK,
> +	[ADXL367_FIFO_FORMAT_Y]    = ADXL367_Y_CHANNEL_MASK,
> +	[ADXL367_FIFO_FORMAT_Z]    = ADXL367_Z_CHANNEL_MASK,
> +	[ADXL367_FIFO_FORMAT_XYZT] = ADXL367_X_CHANNEL_MASK
> +				     | ADXL367_Y_CHANNEL_MASK
> +				     | ADXL367_Z_CHANNEL_MASK
> +				     | ADXL367_TEMP_CHANNEL_MASK,
> +	[ADXL367_FIFO_FORMAT_XT]   = ADXL367_X_CHANNEL_MASK
> +				     | ADXL367_TEMP_CHANNEL_MASK,
> +	[ADXL367_FIFO_FORMAT_YT]   = ADXL367_Y_CHANNEL_MASK
> +				     | ADXL367_TEMP_CHANNEL_MASK,
> +	[ADXL367_FIFO_FORMAT_ZT]   = ADXL367_Z_CHANNEL_MASK
> +				     | ADXL367_TEMP_CHANNEL_MASK,
> +	[ADXL367_FIFO_FORMAT_XYZA] = ADXL367_X_CHANNEL_MASK
> +				     | ADXL367_Y_CHANNEL_MASK
> +				     | ADXL367_Z_CHANNEL_MASK
> +				     | ADXL367_EX_ADC_CHANNEL_MASK,
> +	[ADXL367_FIFO_FORMAT_XA]   = ADXL367_X_CHANNEL_MASK
> +				     | ADXL367_EX_ADC_CHANNEL_MASK,
> +	[ADXL367_FIFO_FORMAT_YA]   = ADXL367_Y_CHANNEL_MASK
> +				     | ADXL367_EX_ADC_CHANNEL_MASK,
> +	[ADXL367_FIFO_FORMAT_ZA]   = ADXL367_Z_CHANNEL_MASK
> +				     | ADXL367_EX_ADC_CHANNEL_MASK,
> +	0,
> +};

The way available scan_masks works is to search for an entry
for which the desired mask is a subset.

That means to get the minimal mode IIRC you need to order them from
smallest to largest. e.g.
https://elixir.bootlin.com/linux/latest/source/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c#L1122

> +

> +EXPORT_SYMBOL_NS_GPL(adxl367_probe, ADXL367);

Trivial but we decided to prefix IIO namespaces with IIO_ so this should
be IIO_ADXL367.

> +
> +MODULE_AUTHOR("Cosmin Tanislav <cosmin.tanislav@analog.com>");
> +MODULE_DESCRIPTION("Analog Devices ADXL367 3-axis accelerometer driver");
> +MODULE_LICENSE("GPL");