Re: [PATCH 1/1] iio: (bma400) add driver for the BMA400

From: Peter Meerwald-Stadler
Date: Tue Oct 08 2019 - 11:29:36 EST

Next message: Vlastimil Babka: "[PATCH] mm, compaction: fix wrong pfn handling in __reset_isolation_pfn()"
Previous message: Randy Dunlap: "[PATCH -next] hung_task: fix Documentation warning"
In reply to: Dan Robertson: "Re: [PATCH 1/1] iio: (bma400) add driver for the BMA400"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Sun, 6 Oct 2019, Jonathan Cameron wrote:

> On Wed, 25 Sep 2019 01:39:41 +0000
> Dan Robertson <dan@xxxxxxxxxxxxxxx> 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.

some more comments (on top of Jonathan's)

> > Signed-off-by: Dan Robertson <dan@xxxxxxxxxxxxxxx>
> Hi Dan,
>
> So my first thought was that Bosch have been moderately good at keeping their
> interfaces consistent across generations. Seems not this time from
> a quick look!
>
> Minor stuff inline. Quite a bit of it is stylistic rather than functional.
> I'm a lazy maintainer and tend to end up reading a lot of code so want to
> think as little as possible ;)
>
> I'd also expect a modern driver to have DT bindings. Might let that go if
> it was being used with ACPI but neither is true here.
>
> Thanks,
>
> Jonathan
>
> > ---
> > drivers/iio/accel/Kconfig | 19 +
> > drivers/iio/accel/Makefile | 2 +
> > drivers/iio/accel/bma400.h | 74 +++
> > drivers/iio/accel/bma400_core.c | 862 ++++++++++++++++++++++++++++++++
> > drivers/iio/accel/bma400_i2c.c | 54 ++
> > 5 files changed, 1011 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..cca6727e037e 100644
> > --- a/drivers/iio/accel/Kconfig
> > +++ b/drivers/iio/accel/Kconfig
> > @@ -112,6 +112,25 @@ 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)
> > + 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
> > + depends on I2C
> > + select REGMAP_I2C
> > +
> > config BMC150_ACCEL
> > tristate "Bosch BMC150 Accelerometer Driver"
> > select IIO_BUFFER
> > diff --git a/drivers/iio/accel/Makefile b/drivers/iio/accel/Makefile
> > index 56bd0215e0d4..3a051cf37f40 100644
> > --- a/drivers/iio/accel/Makefile
> > +++ b/drivers/iio/accel/Makefile
> > @@ -14,6 +14,8 @@ obj-$(CONFIG_ADXL372_I2C) += adxl372_i2c.o
> > obj-$(CONFIG_ADXL372_SPI) += adxl372_spi.o
> > obj-$(CONFIG_BMA180) += bma180.o
> > obj-$(CONFIG_BMA220) += bma220_spi.o
> > +obj-$(CONFIG_BMA400) += bma400_core.o
> > +obj-$(CONFIG_BMA400_I2C) += bma400_i2c.o
> > obj-$(CONFIG_BMC150_ACCEL) += bmc150-accel-core.o
> > obj-$(CONFIG_BMC150_ACCEL_I2C) += bmc150-accel-i2c.o
> > obj-$(CONFIG_BMC150_ACCEL_SPI) += bmc150-accel-spi.o
> > diff --git a/drivers/iio/accel/bma400.h b/drivers/iio/accel/bma400.h
> > new file mode 100644
> > index 000000000000..7fa92bc457f6
> > --- /dev/null
> > +++ b/drivers/iio/accel/bma400.h
> > @@ -0,0 +1,74 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +/*
> > + * bma400.h - Register constants and other forward declarations
> > + * needed by the bma400 sources.
> > + *
> > + * Copyright 2019 Dan Robertson <dan@xxxxxxxxxxxxxxx>
> > + *
>
> Blank line not needed.
>
> > + */
> > +
> > +#include <linux/regmap.h>
> > +
> > +/*
> > + * Read-Only Registers
> > + */
> > +
> > +/* Status and ID registers */
> > +#define BMA400_CHIP_ID_REG 0x00
> > +#define BMA400_ERR_REG 0x02
> > +#define BMA400_STATUS_REG 0x03
> > +
> > +/* Acceleration registers */
> > +#define BMA400_X_AXIS_LSB_REG 0x04
> > +#define BMA400_X_AXIS_MSB_REG 0x05
> > +#define BMA400_Y_AXIS_LSB_REG 0x06
> > +#define BMA400_Y_AXIS_MSB_REG 0x07
> > +#define BMA400_Z_AXIS_LSB_REG 0x08
> > +#define BMA400_Z_AXIS_MSB_REG 0x09
> > +
> > +/* Sensort time registers */

Sensor time

> > +#define BMA400_SENSOR_TIME0 0x0a
> > +#define BMA400_SENSOR_TIME1 0x0b
> > +#define BMA400_SENSOR_TIME2 0x0c
> > +
> > +/* Event and interrupt registers */
> > +#define BMA400_EVENT_REG 0x0d
> > +#define BMA400_INT_STAT0_REG 0x0e
> > +#define BMA400_INT_STAT1_REG 0x0f
> > +#define BMA400_INT_STAT2_REG 0x10
> > +
> > +/* Temperature register */
> > +#define BMA400_TEMP_DATA_REG 0x11
> > +
> > +/* FIFO length and data registers */
> > +#define BMA400_FIFO_LENGTH0_REG 0x12
> > +#define BMA400_FIFO_LENGTH1_REG 0x13
> > +#define BMA400_FIFO_DATA_REG 0x14
> > +
> > +/* Step count registers */
> > +#define BMA400_STEP_CNT0_REG 0x15
> > +#define BMA400_STEP_CNT1_REG 0x16
> > +#define BMA400_STEP_CNT3_REG 0x17
> > +#define BMA400_STEP_STAT_REG 0x18
> > +
> > +/*
> > + * Read-write configuration registers
> > + */
> > +#define BMA400_ACC_CONFIG0_REG 0x19
> > +#define BMA400_ACC_CONFIG1_REG 0x1a
> > +#define BMA400_ACC_CONFIG2_REG 0x1b
> > +#define BMA400_CMD_REG 0x7e
> > +
> > +/* Chip ID of BMA 400 devices found in the chip ID register. */
> > +#define BMA400_ID_REG_VAL 0x90
> > +
> > +/* The softreset command */
> > +#define BMA400_SOFTRESET_CMD 0xb6
> > +
> > +extern const struct regmap_config bma400_regmap_config;
> > +
> > +int bma400_probe(struct device *dev,
> > + struct regmap *regmap,
> > + const char *name);
> > +
> > +int bma400_remove(struct device *dev);
> > diff --git a/drivers/iio/accel/bma400_core.c b/drivers/iio/accel/bma400_core.c
> > new file mode 100644
> > index 000000000000..55fe2f220c30
> > --- /dev/null
> > +++ b/drivers/iio/accel/bma400_core.c
> > @@ -0,0 +1,862 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * bma400-core.c - Core IIO driver for Bosch BMA400 triaxial acceleration

bma400_core.c (underscore)

> > + * sensor. Used by bma400-i2c.
> > + *
> > + * Copyright 2019 Dan Robertson <dan@xxxxxxxxxxxxxxx>
> > + *
> > + * 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.
> > + */
> > +const int bma400_scale_table[] = { 38344, 76590, 153277, 306457 };

static?

> > +
> > +struct bma400_sample_freq {
> > + int hz;
> > + int micro_hz;
> > +};
> > +
> > +/* See the ACC_CONFIG1 section of the datasheet */
> > +const struct bma400_sample_freq bma400_sample_freqs[] = {

static?

> > + { .hz = 12, .micro_hz = 500000 },
> > + { .hz = 25, .micro_hz = 0 },
> > + { .hz = 50, .micro_hz = 0 },
> > + { .hz = 100, .micro_hz = 0 },
> > + { .hz = 200, .micro_hz = 0 },
> > + { .hz = 400, .micro_hz = 0 },
> > + { .hz = 800, .micro_hz = 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_data {
> > + struct device *dev;
> > + struct mutex mutex; /* data register lock */
> > + struct iio_mount_matrix orientation;
> > + struct regmap *regmap;
> > + enum bma400_power_mode power_mode;
> > + const struct bma400_sample_freq *sample_freq;
> > + int oversampling_ratio;
> > + int scale;
> > +};
> > +
> > +static bool bma400_is_writable_reg(struct device *dev, unsigned int reg)
> > +{
> > + switch (reg) {
> > + case BMA400_CHIP_ID_REG:
> > + case BMA400_ERR_REG:
> > + case BMA400_STATUS_REG:
> > + case BMA400_X_AXIS_LSB_REG:
> > + case BMA400_X_AXIS_MSB_REG:
> > + case BMA400_Y_AXIS_LSB_REG:
> > + case BMA400_Y_AXIS_MSB_REG:
> > + case BMA400_Z_AXIS_LSB_REG:
> > + case BMA400_Z_AXIS_MSB_REG:
> > + case BMA400_SENSOR_TIME0:
> > + case BMA400_SENSOR_TIME1:
> > + case BMA400_SENSOR_TIME2:
> > + case BMA400_EVENT_REG:
> > + case BMA400_INT_STAT0_REG:
> > + case BMA400_INT_STAT1_REG:
> > + case BMA400_INT_STAT2_REG:
> > + case BMA400_TEMP_DATA_REG:
> > + case BMA400_FIFO_LENGTH0_REG:
> > + case BMA400_FIFO_LENGTH1_REG:
> > + case BMA400_FIFO_DATA_REG:
> > + case BMA400_STEP_CNT0_REG:
> > + case BMA400_STEP_CNT1_REG:
> > + case BMA400_STEP_CNT3_REG:
> > + case BMA400_STEP_STAT_REG:
> > + return false;
> > + default:
> > + return true;
> > + }
> > +}
> > +
> > +static bool bma400_is_volatile_reg(struct device *dev, unsigned int reg)
> > +{
> > + switch (reg) {
> > + case BMA400_ERR_REG:
> > + case BMA400_STATUS_REG:
> > + case BMA400_X_AXIS_LSB_REG:
> > + case BMA400_X_AXIS_MSB_REG:
> > + case BMA400_Y_AXIS_LSB_REG:
> > + case BMA400_Y_AXIS_MSB_REG:
> > + case BMA400_Z_AXIS_LSB_REG:
> > + case BMA400_Z_AXIS_MSB_REG:
> > + case BMA400_SENSOR_TIME0:
> > + case BMA400_SENSOR_TIME1:
> > + case BMA400_SENSOR_TIME2:
> > + case BMA400_EVENT_REG:
> > + case BMA400_INT_STAT0_REG:
> > + case BMA400_INT_STAT1_REG:
> > + case BMA400_INT_STAT2_REG:
> > + case BMA400_TEMP_DATA_REG:
> > + case BMA400_FIFO_LENGTH0_REG:
> > + case BMA400_FIFO_LENGTH1_REG:
> > + case BMA400_FIFO_DATA_REG:
> > + case BMA400_STEP_CNT0_REG:
> > + case BMA400_STEP_CNT1_REG:
> > + case BMA400_STEP_CNT3_REG:
> > + case BMA400_STEP_STAT_REG:
> > + return true;
> > + default:
> > + return false;
> > + }
> > +}
> > +
> > +const struct regmap_config bma400_regmap_config = {
> > + .reg_bits = 8,
> > + .val_bits = 8,
> > + .max_register = BMA400_CMD_REG,
> > + .cache_type = REGCACHE_RBTREE,
> > + .writeable_reg = bma400_is_writable_reg,
> > + .volatile_reg = bma400_is_volatile_reg,
> > +};
> > +EXPORT_SYMBOL(bma400_regmap_config);
> > +
> > +static const struct iio_mount_matrix *
> > +bma400_accel_get_mount_matrix(const struct iio_dev *indio_dev,
> > + const struct iio_chan_spec *chan)
> > +{
> > + struct bma400_data *data = iio_priv(indio_dev);
> > +
> > + return &data->orientation;
> > +}
> > +
> > +static const struct iio_chan_spec_ext_info bma400_ext_info[] = {
> > + IIO_MOUNT_MATRIX(IIO_SHARED_BY_DIR, bma400_accel_get_mount_matrix),
> > + { }
> > +};
> > +
> > +#define BMA400_ACC_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_SAMP_FREQ) | \
> > + BIT(IIO_CHAN_INFO_SCALE) | \
> > + BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), \
> > + .ext_info = bma400_ext_info \

end with ,

> > +}
> > +
> > +static const struct iio_chan_spec bma400_channels[] = {
> > + BMA400_ACC_CHANNEL(X),
> > + BMA400_ACC_CHANNEL(Y),
> > + BMA400_ACC_CHANNEL(Z),
> > + {
> > + .type = IIO_TEMP,
> > + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
> > + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SAMP_FREQ),
> > + },
> > +};
> > +
> > +static int bma400_get_temp_reg(struct bma400_data *data, int *val, int *val2)
> > +{
> > + int ret;
> > + int host_temp;
> > + unsigned int raw_temp;
> > +
> > + if (data->power_mode == POWER_MODE_SLEEP)
> > + return -EBUSY;
> > +
> > + ret = regmap_read(data->regmap, BMA400_TEMP_DATA_REG, &raw_temp);
> > +
> > + if (!ret) {
> Error path out of line, not the good one. See below.
>
> > + host_temp = sign_extend32(raw_temp, 7);
> > + /*
> > + * The formula for the TEMP_DATA register in the datasheet
> > + * is: x * 0.5 + 23
> > + */
> > + *val = (host_temp >> 1) + 23;
> > + *val2 = (host_temp & 0x1) * 500000;
> > + ret = IIO_VAL_INT_PLUS_MICRO;
> > + }
> > + return ret;
> > +}
> > +
> > +static int bma400_get_accel_reg(struct bma400_data *data,
> > + const struct iio_chan_spec *chan,
> > + int *val)
> > +{
> > + int ret;
> > + u16 lsb_reg;

the BMA400 seems to use 8-bit register addresses, u16 doesn't make sense
here (either u8 or just int)

> > + __le16 raw_accel = 0;
>
> I don't think we need the assignment... Is there a compiler that
> can't figure that out?
>
> > +
> > + if (data->power_mode == POWER_MODE_SLEEP)
> > + return -EBUSY;
> > +
> > + switch (chan->channel2) {
> > + case IIO_MOD_X:
> > + lsb_reg = BMA400_X_AXIS_LSB_REG;
> > + break;
> > + case IIO_MOD_Y:
> > + lsb_reg = BMA400_Y_AXIS_LSB_REG;
> > + break;
> > + case IIO_MOD_Z:
> > + lsb_reg = BMA400_Z_AXIS_LSB_REG;
> > + break;
> > + default:
> > + dev_err(data->dev, "invalid axis channel modifier");
> > + return -EINVAL;
> > + }
> > +
> > + /* bulk read two registers, with the base being the LSB register */
> > + ret = regmap_bulk_read(data->regmap, lsb_reg, &raw_accel, 2);

2 == sizeof(rawaccel)

> > + if (!ret) {
>
> Same point as below on error path out of line.
>
> > + *val = sign_extend32(le16_to_cpu(raw_accel), 11);
> > + ret = IIO_VAL_INT;
> > + }
> > +
> > + return ret;
> > +}
> > +
> > +static int bma400_ready_for_cmd(struct bma400_data *data)
> > +{
> > + unsigned int val;
> > + int ret = regmap_read(data->regmap, BMA400_STATUS_REG, &val);
> > +
> > + if (ret < 0 || !(val & 0x10))
> > + return 0;
> > +
> > + return 1;
>
> Maybe... (up to you on this one as I'm not convinced this is better :)
>
> if (ret < 0)
> return 0;
>
> return (val & 0x10) ? 1 : 0;
>
> Separates the error case from checking the parameter.

the magic 0x10 would benefit from a #define

> > +}
> > +
> > +static int bma400_softreset(struct bma400_data *data)
> > +{
> > + int ret;
> > +
> > + if (!bma400_ready_for_cmd(data))
> > + return -EAGAIN;
> > +
> > + ret = regmap_write(data->regmap, BMA400_CMD_REG,
> > + BMA400_SOFTRESET_CMD);
> > + if (!ret) {
> As below, prefer the error path indented even if it costs a line or two of
> code.
> if (ret)
> return ret;
>
> data->...
> > + /* a softreset restores registers to their defaults */
> > + data->power_mode = POWER_MODE_SLEEP;
> > + data->sample_freq = NULL;
> > + data->oversampling_ratio = -1;
> > + data->scale = bma400_scale_table[0];
> > + }
> > + return ret;
> > +}
> > +
> > +static int bma400_get_acc_config0_reg(struct bma400_data *data)
> > +{
>
> I'm unconvinced these wrappers add anything. Just do the regmap_read
> inline.
>
> > + int ret;
> > + unsigned int val;
> > +
> > + ret = regmap_read(data->regmap, BMA400_ACC_CONFIG0_REG,
> > + &val);
> > + if (ret < 0) {
> > + dev_err(data->dev, "Failed read acc_config0");
> > + return ret;
> > + }
> > +
> > + return val;
> > +}
> > +
> > +static int bma400_get_acc_config1_reg(struct bma400_data *data)
> > +{
> > + int ret;
> > + unsigned int val;
> > +
> > + ret = regmap_read(data->regmap, BMA400_ACC_CONFIG1_REG,
> > + &val);
> > + if (ret < 0) {
> > + dev_err(data->dev, "Failed read acc_config1");
> > + return ret;
> > + }
> > +
> > + return val;
> > +}
> > +
> > +static int bma400_get_accel_output_data_rate(struct bma400_data *data)
> > +{
> > + int acc_config1;
> > + unsigned int odr;
> > + int idx;
> > +
> > + switch (data->power_mode) {
> > + case POWER_MODE_LOW:
> > + /*
> > + * Runs at a fixed rate in low-power mode. See section 4.3
> > + * in the datasheet.
> > + */
> > + data->sample_freq = &bma400_sample_freqs[1];
> > + return 0;
> > + case POWER_MODE_NORMAL:
> > + /*
> > + * In normal mode the ODR can be found in the ACC_CONFIG1
> > + * register.
> > + */
> > + acc_config1 = bma400_get_acc_config1_reg(data);
> > + if (acc_config1 < 0) {
> > + data->sample_freq = NULL;
> > + return acc_config1;
> > + }
> > +
> > + odr = (acc_config1 & 0x0f);
> > + if (odr < 0x05 || odr > 0x0b) {
> > + dev_err(data->dev, "invalid ODR=%x", odr);
> > + data->sample_freq = NULL;
> > + return -EINVAL;
> > + }
> > + idx = odr - 0x05;
> > +
> > + data->sample_freq = &bma400_sample_freqs[idx];
> > + return 0;
> > + default:
> > + data->sample_freq = NULL;
> > + return 0;
> > + }
> > +}
> > +
> > +static int bma400_get_accel_output_data_rate_idx(struct bma400_data *data,
> > + int hz, int uhz)
> > +{
> > + int i;
> > + const struct bma400_sample_freq *sample_freq;
> > +
> > + for (i = 0; i < ARRAY_SIZE(bma400_sample_freqs); ++i) {
> > + sample_freq = &bma400_sample_freqs[i];
> > + if (sample_freq->hz == hz &&
> > + sample_freq->micro_hz == uhz)
> > + return i;
> > + }
> > +
> > + return -EINVAL;
> > +}
> > +
> > +static int bma400_set_accel_output_data_rate(struct bma400_data *data,
> > + int hz, int uhz)
> > +{
> > + int acc_config1;
> > + unsigned int odr;
> > + int idx;
> > + int ret;
> > +
> > + idx = bma400_get_accel_output_data_rate_idx(data, hz, uhz);
> > +
> > + if (idx < 0)
> > + return idx;
> > +
> > + acc_config1 = bma400_get_acc_config1_reg(data);
> > +
> > + if (acc_config1 < 0)
> > + return acc_config1;
> > +
> > + /* preserve the range and normal mode osr */
> > + odr = (0xf0 & acc_config1) | (idx + 0x5);
> > +
> > + ret = regmap_write(data->regmap, BMA400_ACC_CONFIG1_REG, odr);
> > + if (!ret)
> > + data->sample_freq = &bma400_sample_freqs[idx];
>
> I'd rather we kept error flow indented even if a bit longer. It means
> these fine error checks are handled the same as earlier ones and makes
> reviewing a little bit easier.
>
> if (ret)
> return ret;
>
> data->...
>
> > +
> > + return ret;
> > +}
> > +
> > +static int bma400_get_accel_oversampling_ratio(struct bma400_data *data)
> > +{
> > + int acc_config;
> > +
> > + /*
> > + * The oversampling ratio is stored in a different register
> > + * based on the power-mode. In normal mode the OSR is stored
> > + * in ACC_CONFIG1. In low-power mode it is stored in
> > + * ACC_CONFIG0.
> > + */
> > + switch (data->power_mode) {
> > + case POWER_MODE_LOW:
> > + acc_config = bma400_get_acc_config0_reg(data);
> > + if (acc_config < 0) {
> > + data->oversampling_ratio = -1;
> > + return acc_config;
> > + }
> > +
> > + data->oversampling_ratio = (acc_config & 0x60) >> 5;
> > + return 0;
> > + case POWER_MODE_NORMAL:
> > + acc_config = bma400_get_acc_config1_reg(data);
> > + if (acc_config < 0) {
> > + data->oversampling_ratio = -1;
> > + return acc_config;
> > + }
> > +
> > + data->oversampling_ratio = (acc_config & 0x30) >> 4;
> > + return 0;
> > + default:
> > + data->oversampling_ratio = -1;
> > + return 0;
> > + }
> > +}
> > +
> > +static int bma400_set_accel_oversampling_ratio(struct bma400_data *data,
> > + int val)
> > +{
> > + int ret;
> > + int acc_config;
> > +
> > + /* The oversampling ratio is a two bit field */
> > + if (val & ~0x3)
>
> Use a define for that mask. Then we don't need the comment.
>
> > + return -EINVAL;
> > +
> > + /* The oversampling ratio is stored in a different register
>
> Most of the kernel, including iio, use
> /*
> * The...
> * ..
> */
>
> > + * based on the power-mode.
> > + */
> > + switch (data->power_mode) {
> > + case POWER_MODE_LOW:
> > + acc_config = bma400_get_acc_config0_reg(data);
> > + if (acc_config < 0)
> > + return acc_config;
> > +
> > + ret = regmap_write(data->regmap, BMA400_ACC_CONFIG0_REG,
> > + (acc_config & 0x9f) | (val << 5));
>
> Use defines for these masks.
>
> > + if (ret < 0)
> > + dev_err(data->dev, "Failed to write out OSR");
> > + else
> > + data->oversampling_ratio = val;
> > + break;
> > + case POWER_MODE_NORMAL:
> > + acc_config = bma400_get_acc_config1_reg(data);
> > + if (acc_config < 0)
> > + return acc_config;
> > +
> > + ret = regmap_write(data->regmap, BMA400_ACC_CONFIG1_REG,
> > + (acc_config & 0xcf) | (val << 4));
> > + if (ret < 0)
> > + dev_err(data->dev, "Failed to write out OSR");
> > + else
> > + data->oversampling_ratio = val;
> Nicer (even if a little longer) to read as:
>
> if (ret < 0) {
> dev_err(data->dev, "Failed to write out OSR");
> return ret;
> }
>
> data->oversampling_ration = val;
>
> return 0;
> > + break;
> > + default:
> > + return -EINVAL;
> > + }
> > + return ret;
>
>
> > +}
> > +
> > +static int bma400_get_accel_scale(struct bma400_data *data)
> > +{
> > + int idx;
> > + int acc_config1 = bma400_get_acc_config1_reg(data);
> > +
> > + if (acc_config1 < 0)
> > + return acc_config1;
> > +
> > + idx = (acc_config1 & 0xc0) >> 6;
> > + if (idx < ARRAY_SIZE(bma400_scale_table)) {
> > + data->scale = bma400_scale_table[idx];
> > + return 0;
>
> Preference for error path indented in cases where it doesn't
> make a substantial difference in code length.
>
> if (idx >= ARRAY_SIZE(bma400_scale_table)
> return -EINVAL;
>
> data->scale = bma400_scale_table[idx];
>
> return 0;
>
> > + }
> > + return -EINVAL;
> > +}
> > +
> > +static int bma400_get_accel_scale_idx(struct bma400_data *data, int val)
> > +{
> > + int i;
> > +
> > + for (i = 0; i < ARRAY_SIZE(bma400_scale_table); ++i) {
> > + if (bma400_scale_table[i] == val)
> > + return i;
> > + }
> > + return -EINVAL;
> > +}
> > +
> > +static int bma400_set_accel_scale(struct bma400_data *data, unsigned int val)
> > +{
> > + int ret;
> > + int idx;
> > + int acc_config1 = bma400_get_acc_config1_reg(data);
> > +
> > + if (acc_config1 < 0)
> > + return acc_config1;
> > +
> > + idx = bma400_get_accel_scale_idx(data, val);
> > +
> > + if (idx < 0)
> > + return idx;
> > +
> > + ret = regmap_write(data->regmap, BMA400_ACC_CONFIG1_REG,
> > + (acc_config1 & 0x3f) | (idx << 6));
> > + if (ret < 0)
> > + return ret;
> > +
> > + data->scale = val;
> > + return 0;
> > +}
> > +
> > +static int bma400_get_power_mode(struct bma400_data *data)
> > +{
> > + int ret;
> > + unsigned int val;
> > +
> > + ret = regmap_read(data->regmap, BMA400_STATUS_REG, &val);
> > + if (ret < 0) {
> > + dev_err(data->dev, "Failed to read status register");
> > + return ret;
> > + }
> > +
> > + data->power_mode = (val >> 1) & 0x03;
>
> Blank line here and in similar cases helps readability (a tiny bit).
>
> > + return 0;
> > +}
> > +
> > +static int bma400_set_power_mode(struct bma400_data *data,
> > + enum bma400_power_mode mode)
> > +{
> > + int ret;
> > +
> > + ret = bma400_get_acc_config0_reg(data);
> > +
> > + if (ret < 0)
> > + return ret;
> > +
> > + if (data->power_mode == mode)
> > + return 0;
> > +
> > + if (mode == POWER_MODE_INVALID)
> > + return -EINVAL;
> > +
> > + /* Preserve the low-power oversample ratio etc */
> > + ret = regmap_write(data->regmap, BMA400_ACC_CONFIG0_REG,
> > + mode | (ret & 0xfc));
> > +
> > + if (ret < 0) {
> > + dev_err(data->dev, "Failed to write to power-mode");
> > + return ret;
> > + }
> > +
> > + /* Update our cached power-mode */
>
> Bit marginal on whether that comment adds anything not obvious from
> the code. From a maintenance point of view, excess comments are
> just opportunities for rot. Hence we are generally only in favour
> of those that add significant value.
>
> > + data->power_mode = mode;
> > +
> > + /*
> > + * Update our cached osr and odr based on the new
> > + * power-mode.
> > + */
> > + bma400_get_accel_output_data_rate(data);
> > + bma400_get_accel_oversampling_ratio(data);
> > +
> > + return 0;
> > +}
> > +
> > +static int bma400_init(struct bma400_data *data)
> > +{
> > + int ret;
> > + unsigned int val;
> > +
> > + /* Try to read chip_id register. It must return 0x90. */
> > + ret = regmap_read(data->regmap, BMA400_CHIP_ID_REG, &val);
> > +
> > + if (ret < 0) {
> > + dev_err(data->dev, "Failed to read chip id register: %x!", ret);
> > + return ret;
> > + } else if (val != BMA400_ID_REG_VAL) {
> > + dev_err(data->dev, "CHIP ID MISMATCH: %x!", ret);
> > + return -ENODEV;
> > + }
> > +
> > + ret = bma400_get_power_mode(data);
> > + if (ret < 0) {
> > + dev_err(data->dev, "Failed to get the initial power-mode!");
> > + return ret;
> > + }
> > +
> > + if (data->power_mode != POWER_MODE_NORMAL) {
> > + ret = bma400_set_power_mode(data, POWER_MODE_NORMAL);
> > + if (ret < 0) {
> > + dev_err(data->dev, "Failed to wake up the device!");
> > + return ret;
> > + }
> > + /*
> > + * TODO: The datasheet waits 1500us here in the example, but
> > + * lists 2/ODR as the wakeup time.
> > + */
> > + usleep_range(1500, 20000);
> > + }
> > +
> > + ret = bma400_get_accel_output_data_rate(data);
> > + if (ret < 0)
> > + return ret;
> > +
> > + ret = bma400_get_accel_oversampling_ratio(data);
> > + if (ret < 0)
> > + return ret;
> > +
> > + ret = bma400_get_accel_scale(data);
> > + if (ret < 0)
> > + return ret;
> > +
> > + /*
> > + * Once the interrupt engine is supported we might use the
> > + * data_src_reg, but for now ensure this is set to the
> > + * variable ODR filter selectable by the sample frequency
> > + * channel.
> > + */
> > + return regmap_write(data->regmap, BMA400_ACC_CONFIG2_REG, 0x00);
> > +}
> > +
> > +static ssize_t bma400_show_samp_freq_avail(struct device *dev,
> > + struct device_attribute *attr,
> > + char *buf)
> > +{
> > + int i;
> > + int len = 0;
> > + const struct bma400_sample_freq *sample_freq;
> > +
> > + for (i = 0; i < ARRAY_SIZE(bma400_sample_freqs); ++i) {
> > + sample_freq = &bma400_sample_freqs[i];
> > + len += scnprintf(buf + len, PAGE_SIZE - len, "%d.%06d ",
> > + sample_freq->hz, sample_freq->micro_hz);
> > + }
> > +
> > + buf[len - 1] = '\n';
> > +
> > + return len;
> > +}
> > +
> > +static ssize_t bma400_show_scale_avail(struct device *dev,
> > + struct device_attribute *attr,
> > + char *buf)
> As you are providing functions for it anyway, perhaps use the core
> functions to do this?
> See the docs for struct iio_info -> read_avail.
>
> Not that many users yet as we added this for a few specific cases
> that needed to query these from in kernel users.
>
>
> > +{
> > + int i;
> > + int len = 0;
> > +
> > + for (i = 0; i < ARRAY_SIZE(bma400_scale_table); ++i)
> > + len += scnprintf(buf + len, PAGE_SIZE - len, "0.%06d ",
> > + bma400_scale_table[i]);
> > +
> > + buf[len - 1] = '\n';
> > +
> > + return len;
> > +}
> > +
> > +static IIO_DEV_ATTR_SAMP_FREQ_AVAIL(bma400_show_samp_freq_avail);
> > +
> > +static IIO_DEVICE_ATTR(in_accel_scale_available, 0444,
> > + bma400_show_scale_avail, NULL, 0);
> > +
> > +static struct attribute *bma400_attributes[] = {
> > + &iio_dev_attr_sampling_frequency_available.dev_attr.attr,
> > + &iio_dev_attr_in_accel_scale_available.dev_attr.attr,
> > + NULL,
> > +};
> > +
> > +static const struct attribute_group bma400_attrs_group = {
> > + .attrs = bma400_attributes,
> > +};
> > +
> > +static int bma400_read_raw(struct iio_dev *indio_dev,
> > + struct iio_chan_spec const *chan, int *val,
> > + int *val2, long mask)
> > +{
> > + struct bma400_data *data = iio_priv(indio_dev);
> > + int ret;
> > +
> > + switch (mask) {
> > + case IIO_CHAN_INFO_PROCESSED:
> > + mutex_lock(&data->mutex);
> > + ret = bma400_get_temp_reg(data, val, val2);
> > + mutex_unlock(&data->mutex);
> > + return ret;
> > + case IIO_CHAN_INFO_RAW:
> > + mutex_lock(&data->mutex);
> > + ret = bma400_get_accel_reg(data, chan, val);
> > + mutex_unlock(&data->mutex);
> > + return ret;
> > + case IIO_CHAN_INFO_SAMP_FREQ:
> > + switch (chan->type) {
> > + case IIO_ACCEL:
> > + if (!data->sample_freq)
> > + return -EINVAL;
> > +
> > + *val = data->sample_freq->hz;
> > + if (!data->sample_freq->micro_hz)
> > + return IIO_VAL_INT;
> > +
> > + *val2 = data->sample_freq->micro_hz;
> > + return IIO_VAL_INT_PLUS_MICRO;
> > + case IIO_TEMP:
> > + /*
> > + * Runs at a fixed sampling frequency. See Section 4.4
> > + * of the datasheet.
> > + */
> > + *val = 6;
> > + *val2 = 250000;
> > + return IIO_VAL_INT_PLUS_MICRO;
> > + default:
> > + return -EINVAL;
> > + }
> > + case IIO_CHAN_INFO_SCALE:
> > + *val = 0;
> > + *val2 = data->scale;
> > + return IIO_VAL_INT_PLUS_MICRO;
> > + case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> > + /*
> > + * TODO: We could avoid this logic and returning -EINVAL here if
> > + * we set both the low-power and normal mode OSR registers when
> > + * we configure the device.
> > + */
> > + if (data->oversampling_ratio < 0)
> > + return -EINVAL;
> > +
> > + *val = data->oversampling_ratio;
> > + return IIO_VAL_INT;
> > + default:
> > + return -EINVAL;
> > + }
> > +}
> > +
> > +static int bma400_write_raw(struct iio_dev *indio_dev,
> > + struct iio_chan_spec const *chan, int val, int val2,
> > + long mask)
> > +{
> > + int ret;
> > + struct bma400_data *data = iio_priv(indio_dev);
> > +
> > + switch (mask) {
> > + case IIO_CHAN_INFO_SAMP_FREQ:
> > + /*
> > + * The sample frequency is readonly for the temperature
> > + * register and a fixed value in low-power mode.
> > + */
> > + if (chan->type != IIO_ACCEL)
> > + return -EINVAL;
> > +
> > + mutex_lock(&data->mutex);
> > + ret = bma400_set_accel_output_data_rate(data, val, val2);
> > + mutex_unlock(&data->mutex);
> > + return ret;
> > + case IIO_CHAN_INFO_SCALE:
> > + if (val != 0)
> > + return -EINVAL;
> > +
> > + mutex_lock(&data->mutex);
> > + ret = bma400_set_accel_scale(data, val2);
> > + mutex_unlock(&data->mutex);
> > + return ret;
> > + case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> > + mutex_lock(&data->mutex);
> > + ret = bma400_set_accel_oversampling_ratio(data, val);
> > + mutex_unlock(&data->mutex);
> > + return ret;
> > + default:
> > + return -EINVAL;
> > + }
> > +}
> > +
> > +static int bma400_write_raw_get_fmt(struct iio_dev *indio_dev,
> > + struct iio_chan_spec const *chan,
> > + long mask)
> > +{
> > + switch (mask) {
> > + case IIO_CHAN_INFO_SAMP_FREQ:
> > + return IIO_VAL_INT_PLUS_MICRO;
> > + case IIO_CHAN_INFO_SCALE:
> > + return IIO_VAL_INT_PLUS_MICRO;
> > + case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> > + return IIO_VAL_INT;
> > + default:
> > + return -EINVAL;
> > + }
> > +}
> > +
> > +static const struct iio_info bma400_info = {
> > + .attrs = &bma400_attrs_group,
> > + .read_raw = bma400_read_raw,
> > + .write_raw = bma400_write_raw,
> > + .write_raw_get_fmt = bma400_write_raw_get_fmt,
> > +};
> > +
> > +int bma400_probe(struct device *dev,
> > + struct regmap *regmap,
> > + const char *name)
> > +{
> > + int ret;
> > + struct bma400_data *data;
> > + struct iio_dev *indio_dev;
> > +
> > + indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> > + if (!indio_dev)
> > + return -ENOMEM;
> > +
> > + data = iio_priv(indio_dev);
> > + data->regmap = regmap;
> > + data->dev = dev;
> > +
> > + ret = bma400_init(data);
> > + if (ret < 0)
> > + return ret;
> > +
> > + ret = iio_read_mount_matrix(dev, "mount-matrix",
> > + &data->orientation);
>
> Looks like the above will fit on one line..
>
> > + if (ret)
> > + return ret;
> > +
> > + mutex_init(&data->mutex);
> > + indio_dev->dev.parent = dev;
> > + indio_dev->name = name;
> > + indio_dev->info = &bma400_info;
> > + indio_dev->channels = bma400_channels;
> > + indio_dev->num_channels = ARRAY_SIZE(bma400_channels);
> > + indio_dev->modes = INDIO_DIRECT_MODE;
> > +
> > + dev_set_drvdata(dev, indio_dev);
> > +
> > + ret = iio_device_register(indio_dev);
> > + if (ret < 0) {
> > + dev_err(dev, "unable to register iio device\n");
> > + return ret;
>
> Drop the return out of the if statement and no need for the return 0 below.

I'd also avoid the error message altogether

>
> > + }
> > +
> > + return 0;
> > +}
> > +EXPORT_SYMBOL(bma400_probe);
> > +
> > +int bma400_remove(struct device *dev)
> > +{
> > + int ret;
> > + struct iio_dev *indio_dev = dev_get_drvdata(dev);
> > + struct bma400_data *data = iio_priv(indio_dev);
> > +
> > + mutex_lock(&data->mutex);
> > + ret = bma400_softreset(data);
> > + if (ret < 0) {
> > + /*
> > + * If the softreset failed, try to put the device in
> > + * sleep mode, but still report the error.
> > + */
> > + dev_err(data->dev, "Failed to reset the device");
> > + bma400_set_power_mode(data, POWER_MODE_SLEEP);
> > + }
> > + mutex_unlock(&data->mutex);
> > +
> > + iio_device_unregister(indio_dev);
> > +
> > + return ret;
> > +}
> > +EXPORT_SYMBOL(bma400_remove);
> > +
> > +MODULE_AUTHOR("Dan Robertson <dan@xxxxxxxxxxxxxxx>");
> > +MODULE_DESCRIPTION("Bosch BMA400 triaxial acceleration sensor");
> > +MODULE_LICENSE("GPL");
> > diff --git a/drivers/iio/accel/bma400_i2c.c b/drivers/iio/accel/bma400_i2c.c
> > new file mode 100644
> > index 000000000000..227012a32e13
> > --- /dev/null
> > +++ b/drivers/iio/accel/bma400_i2c.c
> > @@ -0,0 +1,54 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * bma400-i2c.c - I2C IIO driver for Bosch BMA400 triaxial acceleration sensor.
> > + *
> > + * Copyright 2019 Dan Robertson <dan@xxxxxxxxxxxxxxx>
> > + *
> > + * I2C address is either 0x14 or 0x15 depending on SDO
> > + *
>
> This blank line doesn't add anything (nitpick)
>
> > + */
> > +#include <linux/module.h>
> > +#include <linux/i2c.h>
> > +#include <linux/acpi.h>
>
> Slight preference for alphabetical order. Also why ACPI?
>
> > +#include <linux/of.h>
> > +#include <linux/regmap.h>
> > +
> > +#include "bma400.h"
> > +
> > +static int bma400_i2c_probe(struct i2c_client *client,
> > + const struct i2c_device_id *id)
> > +{
> > + struct regmap *regmap;
> > +
> > + regmap = devm_regmap_init_i2c(client,
> > + &bma400_regmap_config);
> > +
> > + return bma400_probe(&client->dev, regmap, id->name);
> > +}
> > +
> > +static int bma400_i2c_remove(struct i2c_client *client)
> > +{
> > + return bma400_remove(&client->dev);
> > +}
> > +
> > +static const struct i2c_device_id bma400_i2c_ids[] = {
> > + { "bma400", 0 },
> > + { }
> > +};
> > +
> > +MODULE_DEVICE_TABLE(i2c, bma400_i2c_ids);
> > +
>
> Good to have a of_device_id table as well from the start.
> There is a general (but slow) move to stop using the fallback
> to the i2c_device_id table.
>
> > +static struct i2c_driver bma400_i2c_driver = {
> > + .driver = {
> > + .name = "bma400",
> > + },
> > + .probe = bma400_i2c_probe,
> > + .remove = bma400_i2c_remove,
> > + .id_table = bma400_i2c_ids,
> > +};
> > +
> > +module_i2c_driver(bma400_i2c_driver);
> > +
> > +MODULE_AUTHOR("Dan Robertson <dan@xxxxxxxxxxxxxxx>");
> > +MODULE_DESCRIPTION("Bosch BMA400 triaxial acceleration sensor");
> > +MODULE_LICENSE("GPL");
> >
> >
>

--

Peter Meerwald-Stadler
Mobile: +43 664 24 44 418

Next message: Vlastimil Babka: "[PATCH] mm, compaction: fix wrong pfn handling in __reset_isolation_pfn()"
Previous message: Randy Dunlap: "[PATCH -next] hung_task: fix Documentation warning"
In reply to: Dan Robertson: "Re: [PATCH 1/1] iio: (bma400) add driver for the BMA400"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]