On 3/16/20 8:32 AM, Mike Looijmans wrote:
The BMI088 is a combined module with both accelerometer and gyroscope.
This adds the accelerometer driver support for the SPI interface.
The gyroscope part is already supported by the BMG160 driver.
Looks very good, a few comments inline.
[...]
+static int bmi088_accel_get_temp(struct bmi088_accel_data *data, int *val)
+{
+ struct device *dev = regmap_get_device(data->regmap);
+ int ret;
+ u8 value[2];
+ unsigned int temp;
+
+ mutex_lock(&data->mutex);
+
+ /* Read temp reg MSB */
+ ret = regmap_bulk_read(data->regmap, BMI088_ACCEL_REG_TEMP,
+ &value, sizeof(value));
+ if (ret < 0) {
+ dev_err(dev, "Error reading BMI088_ACCEL_REG_TEMP\n");
+ mutex_unlock(&data->mutex);
+ return ret;
+ }
+ temp = (unsigned int)value[0] << 3;
+ temp |= (value[1] >> 5);
+
+ if (temp > 1023)
+ *val = temp - 2028;
I would be highly surprised if this is not supposed to be 2048.
If it is you can simplify the expression to be able to work without the
conditional by using
*val = sign_extend32(temp, 11);
I believe it is 11, better double check.
+ else
+ *val = temp;
+
+ mutex_unlock(&data->mutex);
+
+ return IIO_VAL_INT;
+}
[...]
+static int bmi088_accel_read_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ int *val, int *val2, long mask)
+{
+ struct bmi088_accel_data *data = iio_priv(indio_dev);
+ int ret;
+ unsigned int range;
+
+ switch (mask) {
+ case IIO_CHAN_INFO_RAW:
+ switch (chan->type) {
+ case IIO_TEMP:
+ return bmi088_accel_get_temp(data, val);
+ case IIO_ACCEL:
+ if (iio_buffer_enabled(indio_dev))
+ return -EBUSY;
I think there is a race condition here. If the buffer is enabled after
the check undefined behavior might occur. Jonathan already mentioned it
in his review. Best is to use iio_device_{claim,release}_direct_mode().
+
+ ret = regmap_read(data->regmap,
+ BMI088_ACCEL_REG_ACC_RANGE, &range);
+ if (ret < 0)
+ return ret;
+
+ ret = bmi088_accel_get_axis(data, chan, val);
+ if (ret < 0)
+ return ret;
+
+ *val = (*val * 3 * 980 * (0x01 << range)) >> 15;
+
+ return IIO_VAL_INT;
+ default:
+ return -EINVAL;
+ }
[...]
+}
+
+static int bmi088_accel_write_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ int val, int val2, long mask)
+{
+ struct bmi088_accel_data *data = iio_priv(indio_dev);
+ int ret;
+
+ switch (mask) {
+ case IIO_CHAN_INFO_SAMP_FREQ:
+ mutex_lock(&data->mutex);
+ ret = bmi088_accel_set_bw(data, val, val2);
+ mutex_unlock(&data->mutex);
+ break;
+ default:
+ ret = -EINVAL;
+ }
+
+ return ret;
+}
+
+static IIO_CONST_ATTR_SAMP_FREQ_AVAIL("12.5 25 50 100 200 400 800 1600");
+
+static struct attribute *bmi088_accel_attributes[] = {
+ &iio_const_attr_sampling_frequency_available.dev_attr.attr,
+ NULL,
+};
+
+static const struct attribute_group bmi088_accel_attrs_group = {
+ .attrs = bmi088_accel_attributes,
+};
+
+#define BMI088_ACCEL_CHANNEL(_axis, bits) { \
+ .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), \
+ .scan_index = AXIS_##_axis, \
+ .scan_type = { \
+ .sign = 's', \
+ .realbits = (bits), \
+ .storagebits = 16, \
+ .shift = 16 - (bits), \
+ .endianness = IIO_LE, \
+ }, \
+}
+
+#define BMI088_ACCEL_CHANNELS(bits) { \
+ { \
+ .type = IIO_TEMP, \
+ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
+ BIT(IIO_CHAN_INFO_SCALE) | \
+ BIT(IIO_CHAN_INFO_OFFSET), \
+ .scan_index = -1, \
+ }, \
+ BMI088_ACCEL_CHANNEL(X, bits), \
+ BMI088_ACCEL_CHANNEL(Y, bits), \
+ BMI088_ACCEL_CHANNEL(Z, bits), \
+ IIO_CHAN_SOFT_TIMESTAMP(3), \
+}
+
+static const struct iio_chan_spec bmi088_accel_channels[] =
+ BMI088_ACCEL_CHANNELS(16);
+
+static const struct bmi088_accel_chip_info bmi088_accel_chip_info_tbl[] = {
+ [0] = {
+ .name = "BMI088A",
+ .chip_id = 0x1E,
+ .channels = bmi088_accel_channels,
+ .num_channels = ARRAY_SIZE(bmi088_accel_channels),
+ .scale_table = { {9610, BMI088_ACCEL_RANGE_3G},
+ {19122, BMI088_ACCEL_RANGE_6G},
+ {38344, BMI088_ACCEL_RANGE_12G},
+ {76590, BMI088_ACCEL_RANGE_24G} },
+ },
+};
+
+static const struct iio_info bmi088_accel_info = {
+ .attrs = &bmi088_accel_attrs_group,
+ .read_raw = bmi088_accel_read_raw,
+ .write_raw = bmi088_accel_write_raw,
+};
+
+static const unsigned long bmi088_accel_scan_masks[] = {
+ BIT(AXIS_X) | BIT(AXIS_Y) | BIT(AXIS_Z),
+ 0};
+
+
+
+#ifdef CONFIG_PM
+static int bmi088_accel_set_power_state(struct bmi088_accel_data *data,
+ bool on)
+{
+ struct device *dev = regmap_get_device(data->regmap);
+ int ret;
+
+ if (on) {
+ ret = pm_runtime_get_sync(dev);
+ } else {
+ pm_runtime_mark_last_busy(dev);
+ ret = pm_runtime_put_autosuspend(dev);
+ }
+
+ if (ret < 0) {
+ dev_err(dev, "Failed: %s(%d)\n", __func__, on);
+ if (on)
+ pm_runtime_put_noidle(dev);
+
+ return ret;
+ }
+
+ return 0;
+}
+#else
+static int bmi088_accel_set_power_state(struct bmi088_accel_data *data,
+ bool on)
+{
+ return 0;
+}
+#endif
+
+static int bmi088_accel_chip_init(struct bmi088_accel_data *data)
+{
+ struct device *dev = regmap_get_device(data->regmap);
+ int ret, i;
+ unsigned int val;
+
+ /* Do a dummy read (of chip ID), to enable SPI interface */
+ regmap_read(data->regmap, BMI088_ACCEL_REG_CHIP_ID, &val);
+
+ /*
+ * Reset chip to get it in a known good state. A delay of 1ms after
+ * reset is required according to the data sheet
+ */
+ regmap_write(data->regmap, BMI088_ACCEL_REG_RESET,
+ BMI088_ACCEL_RESET_VAL);
+ usleep_range(1000, 2000);
+
+ /* Do a dummy read (of chip ID), to enable SPI interface after reset */
+ regmap_read(data->regmap, BMI088_ACCEL_REG_CHIP_ID, &val);
+
+ /* Read chip ID */
+ ret = regmap_read(data->regmap, BMI088_ACCEL_REG_CHIP_ID, &val);
+ if (ret < 0) {
+ dev_err(dev, "Error: Reading chip id\n");
+ return ret;
+ }
+
+ /* Validate chip ID */
+ dev_dbg(dev, "Chip Id %x\n", val);
+ for (i = 0; i < ARRAY_SIZE(bmi088_accel_chip_info_tbl); i++) {
+ if (bmi088_accel_chip_info_tbl[i].chip_id == val) {
+ data->chip_info = &bmi088_accel_chip_info_tbl[i];
+ break;
+ }
+ }
+
+ if (!data->chip_info) {
+ dev_err(dev, "Invalid chip %x\n", val);
+ return -ENODEV;
+ }
+
+ /* Set Active mode (and wait for 5ms) */
+ ret = bmi088_accel_set_mode(data, BMI088_ACCEL_MODE_ACTIVE);
+ if (ret < 0)
+ return ret;
+
+ usleep_range(5000, 10000);
+
+ /* Enable accelerometer */
+ ret = bmi088_accel_enable(data, true);
+ if (ret < 0)
+ return ret;
+
+ /* Set Bandwidth */
+ ret = bmi088_accel_set_bw(data, BMI088_ACCEL_MODE_ODR_100,
+ BMI088_ACCEL_MODE_OSR_NORMAL);
+ if (ret < 0)
+ return ret;
+
+ /* Set Default Range */
+ ret = regmap_write(data->regmap, BMI088_ACCEL_REG_ACC_RANGE,
+ BMI088_ACCEL_RANGE_6G);
+ if (ret < 0) {
+ dev_err(dev, "Error writing ACC_RANGE\n");
+ return ret;
+ }
+
+ return 0;
+}
+
+int bmi088_accel_core_probe(struct device *dev, struct regmap *regmap,
+ int irq, const char *name, bool block_supported)
+{
+ struct bmi088_accel_data *data;
+ struct iio_dev *indio_dev;
+ int ret;
+
+ indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
+ if (!indio_dev)
+ return -ENOMEM;
+
+ data = iio_priv(indio_dev);
+ dev_set_drvdata(dev, indio_dev);
+
+ data->regmap = regmap;
+
+ ret = bmi088_accel_chip_init(data);
+ if (ret < 0)
+ return ret;
+
+ mutex_init(&data->mutex);
+
+ indio_dev->dev.parent = dev;
+ indio_dev->channels = data->chip_info->channels;
+ indio_dev->num_channels = data->chip_info->num_channels;
+ indio_dev->name = name ? name : data->chip_info->name;
Considering that chip_info is chosen by the product ID register,
regardless of what the compatible string was, maybe it is best to always
use chip_info->name here.
+ indio_dev->available_scan_masks = bmi088_accel_scan_masks;
+ indio_dev->modes = INDIO_DIRECT_MODE;
+ indio_dev->info = &bmi088_accel_info;
diff --git a/drivers/iio/accel/bmi088-accel-spi.c b/drivers/iio/accel/bmi088-accel-spi.c
new file mode 100644
index 000000000000..920e146f07d3
--- /dev/null
+++ b/drivers/iio/accel/bmi088-accel-spi.c
@@ -0,0 +1,100 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * 3-axis accelerometer driver supporting following Bosch-Sensortec chips:
+ * - BMI088
+ *
+ * Copyright (c) 2018, Topic Embedded Products
+ */
+
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/acpi.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+#include <linux/spi/spi.h>
+#include <linux/regmap.h>
+
+#include "bmi088-accel.h"
+
+int bmi088_regmap_spi_write(void *context, const void *data, size_t count)
+{
+ struct spi_device *spi = context;
+ u8 buf[count];
+
+ memcpy(buf, data, count);
+
+ dev_dbg(&spi->dev, "Write val: %x to reg: %x\n", buf[1], buf[0]);
+
+ /*
+ * The SPI register address (= full register address without bit 7)
+ * and the write command (bit7 = RW = '0')
+ */
+ buf[0] &= ~0x80;
+
+ return spi_write(spi, buf, count);
+}
+
+int bmi088_regmap_spi_read(void *context, const void *reg,
+ size_t reg_size, void *val, size_t val_size)
+{
+ struct spi_device *spi = context;
+ u8 addr[reg_size + 1];
I believe there is an effort to eliminate variable length arrays that
are placed on the stack from the kernel. reg_size should have a very
small upper limit you should be able to get away with a statically sized
array.
+
+ addr[0] = *(u8 *)reg;
+
+ dev_dbg(&spi->dev, "Read reg: %x\n", addr[0]);
+
+ addr[0] |= 0x80; /* bit7 = RW = '1' */
+
+ /* Do a write of 2 to mimic the dummy byte (see datasheet) */
+ return spi_write_then_read(spi, addr, sizeof(addr), val, val_size);
+}
[...]
