2019-10-18 22:26:41

by Randy Dunlap

[permalink] [raw]
Subject: Re: [PATCH v4 2/2] iio: (bma400) add driver for the BMA400

On 10/17/19 8:18 PM, Dan Robertson wrote:
> Add a IIO driver for the Bosch BMA400 3-axes ultra-low power accelerometer.
> The driver supports reading from the acceleration and temperature
> registers. The driver also supports reading and configuring the output data
> rate, oversampling ratio, and scale.
>
> Signed-off-by: Dan Robertson <[email protected]>

Hi,
I'm repeating some comments from my V2 review.

> ---
> drivers/iio/accel/Kconfig | 18 +
> drivers/iio/accel/Makefile | 2 +
> drivers/iio/accel/bma400.h | 85 ++++
> drivers/iio/accel/bma400_core.c | 796 ++++++++++++++++++++++++++++++++
> drivers/iio/accel/bma400_i2c.c | 60 +++
> 5 files changed, 961 insertions(+)
> create mode 100644 drivers/iio/accel/bma400.h
> create mode 100644 drivers/iio/accel/bma400_core.c
> create mode 100644 drivers/iio/accel/bma400_i2c.c
>
> diff --git a/drivers/iio/accel/Kconfig b/drivers/iio/accel/Kconfig
> index 9b9656ce37e6..a1081b902d16 100644
> --- a/drivers/iio/accel/Kconfig
> +++ b/drivers/iio/accel/Kconfig
> @@ -112,6 +112,24 @@ config BMA220
> To compile this driver as a module, choose M here: the
> module will be called bma220_spi.
>
> +config BMA400
> + tristate "Bosch BMA400 3-Axis Accelerometer Driver"
> + depends on I2C
> + select REGMAP
> + select BMA400_I2C if (I2C)

Since BMA400 already depends on I2C, the "if (I2C)" above is
redundant so it's not needed.

> + help
> + Say Y here if you want to build a driver for the Bosch BMA400
> + triaxial acceleration sensor.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called bma400_core and you will also get
> + bma400_i2c for I2C.
> +
> +config BMA400_I2C
> + tristate
> + depends on BMA400
> + select REGMAP_I2C
> +
> config BMC150_ACCEL
> tristate "Bosch BMC150 Accelerometer Driver"
> select IIO_BUFFER


> diff --git a/drivers/iio/accel/bma400_core.c b/drivers/iio/accel/bma400_core.c
> new file mode 100644
> index 000000000000..80f1ee6713fa
> --- /dev/null
> +++ b/drivers/iio/accel/bma400_core.c
> @@ -0,0 +1,796 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * bma400_core.c - Core IIO driver for Bosch BMA400 triaxial acceleration
> + * sensor. Used by bma400-i2c.
> + *
> + * Copyright 2019 Dan Robertson <[email protected]>
> + *
> + * TODO:
> + * - Support for power management
> + * - Support events and interrupts
> + * - Create channel the step count
> + * - Create channel for sensor time
> + */
> +
> +#include <linux/device.h>
> +#include <linux/module.h>
> +#include <linux/regmap.h>
> +#include <linux/bitops.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +
> +#include "bma400.h"
> +
> +/*
> + * The G-range selection may be one of 2g, 4g, 8, or 16g. The scale may
> + * be selected with the acc_range bits of the ACC_CONFIG1 register.
> + */
> +static const int bma400_scale_table[] = {
> + 0, 38344,
> + 0, 76590,
> + 0, 153277,
> + 0, 306457
> +};
> +
> +static const int bma400_osr_table[] = { 0, 1, 3 };
> +
> +/* See the ACC_CONFIG1 section of the datasheet */
> +static const int bma400_sample_freqs[] = {
> + 12, 500000,
> + 25, 0,
> + 50, 0,
> + 100, 0,
> + 200, 0,
> + 400, 0,
> + 800, 0,
> +};
> +
> +/* See the ACC_CONFIG0 section of the datasheet */
> +enum bma400_power_mode {
> + POWER_MODE_SLEEP = 0x00,
> + POWER_MODE_LOW = 0x01,
> + POWER_MODE_NORMAL = 0x02,
> + POWER_MODE_INVALID = 0x03,
> +};
> +
> +struct bma400_sample_freq {
> + int hz;
> + int uhz;
> +};
> +
> +struct bma400_data {
> + struct device *dev;
> + struct mutex mutex; /* data register lock */

needs #include <linux/mutex.h>

> + struct iio_mount_matrix orientation;
> + struct regmap *regmap;
> + enum bma400_power_mode power_mode;
> + struct bma400_sample_freq sample_freq;
> + int oversampling_ratio;
> + int scale;
> +};
> +

[snip]

> +static int bma400_get_accel_scale_idx(struct bma400_data *data, int val)
> +{
> + int i;
> +
> + for (i = 1; i < ARRAY_SIZE(bma400_scale_table); i += 2) {

needs #include <linux/kernel.h>
for ARRAY_SIZE()

> + if (bma400_scale_table[i] == val)
> + return i - 1;
> + }
> + return -EINVAL;
> +}

[snip]

Thanks.

--
~Randy
See Documentation/process/submit-checklist.rst, especially Rule #1.


2019-10-19 09:51:42

by Dan Robertson

[permalink] [raw]
Subject: Re: [PATCH v4 2/2] iio: (bma400) add driver for the BMA400

On Thu, Oct 17, 2019 at 09:25:37PM -0700, Randy Dunlap wrote:
> On 10/17/19 8:18 PM, Dan Robertson wrote:
> > +config BMA400
> > + tristate "Bosch BMA400 3-Axis Accelerometer Driver"
> > + depends on I2C
> > + select REGMAP
> > + select BMA400_I2C if (I2C)
>
> Since BMA400 already depends on I2C, the "if (I2C)" above is
> redundant so it's not needed.

You are correct. I'll remove the depends on I2C.

> > +struct bma400_data {
> > + struct device *dev;
> > + struct mutex mutex; /* data register lock */
>
> needs #include <linux/mutex.h>

Good point.

> > +static int bma400_get_accel_scale_idx(struct bma400_data *data, int val)
> > +{
> > + int i;
> > +
> > + for (i = 1; i < ARRAY_SIZE(bma400_scale_table); i += 2) {
>
> needs #include <linux/kernel.h>
> for ARRAY_SIZE()

Thanks

Cheers,

- Dan


Attachments:
(No filename) (843.00 B)
signature.asc (849.00 B)
Digital signature
Download all attachments