On Sat, Sep 07, 2024 at 05:08:44PM GMT, Jonathan Cameron wrote:
> On Fri, 6 Sep 2024 12:52:51 -0400
> Alex Lanzano <lanzano.alex@xxxxxxxxx> wrote:
>
> > Add initial i2c support for the Bosch BMI270 6-axis IMU.
> > Provides raw read access to acceleration and angle velocity measurements
> > via iio channels. Device configuration requires firmware provided by
> > Bosch and is requested and load from userspace.
> >
> > Signed-off-by: Alex Lanzano <lanzano.alex@xxxxxxxxx>
>
> Hi Alex,
>
> Various comments inline. Just to check, have you confirmed these have
> a substantially different interface to the other bosch IMU devices?
>
> (I'm too lazy / busy to datasheet dive today!)
>
> Jonathan
>
Yes, the BMI270 has a different register layout and some features not
included in the existing BMI160 and BMI323 such as the wrist worn
gesture detection.
> > diff --git a/drivers/iio/imu/bmi270/Kconfig b/drivers/iio/imu/bmi270/Kconfig
> > new file mode 100644
> > index 000000000000..05e13c67db57
> > --- /dev/null
> > +++ b/drivers/iio/imu/bmi270/Kconfig
> > @@ -0,0 +1,22 @@
> > +# SPDX-License-Identifier: GPL-2.0
> > +#
> > +# BMI270 IMU driver
> > +#
> > +
> > +config BMI270
> > + tristate
> > + select IIO_BUFFER
> > + select IIO_TRIGGERED_BUFFER
> So far the driver isn't using the buffer / triggered buffer
> so drop this until it does.
>
Wiil do in v3!
> > +
> > +config BMI270_I2C
> > + tristate "Bosch BMI270 I2C driver"
> > + depends on I2C
> > + select BMI270
> > + select REGMAP_I2C
> > + help
> > + Enable support for the Bosch BMI270 6-Axis IMU connected to I2C
> > + interface.
> > +
> > + This driver can also be built as a module. If so, the module will be
> > + called bmi270_i2c.
> > +
>
> > diff --git a/drivers/iio/imu/bmi270/bmi270_core.c b/drivers/iio/imu/bmi270/bmi270_core.c
> > new file mode 100644
> > index 000000000000..f8c53e8e35a2
> > --- /dev/null
> > +++ b/drivers/iio/imu/bmi270/bmi270_core.c
> > @@ -0,0 +1,322 @@
> > +// SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +
> > +#include <linux/firmware.h>
> > +#include <linux/i2c.h>
> > +#include <linux/iio/iio.h>
> > +#include <linux/module.h>
> > +#include <linux/regmap.h>
> > +
> > +#include "bmi270.h"
> > +
> > +#define BMI270_CHIP_ID 0x24
> > +#define BMI270_INIT_DATA_FILE "bmi270-init-data.fw"
> > +
> > +enum bmi270_registers {
>
> Unless you use the type somewhere, ti's usually better to just
> use defines for register addresses.
> That lets you group them with the field definitions for the
> elements that make up each register.
>
Will fix in v3!
> > + BMI270_REG_CHIP_ID = 0x00,
> > + BMI270_REG_INTERNAL_STATUS = 0x21,
> > + BMI270_REG_ACC_CONF = 0x40,
> > + BMI270_REG_GYR_CONF = 0x42,
> > + BMI270_REG_INIT_CTRL = 0x59,
> > + BMI270_REG_INIT_DATA = 0x5e,
> > + BMI270_REG_PWR_CONF = 0x7c,
> > + BMI270_REG_PWR_CTRL = 0x7d,
> > +};
>
> > +static int bmi270_get_data(struct bmi270_data *bmi270_device,
> > + int chan_type, int axis, int *val)
> > +{
> > + __le16 sample;
> > + int reg;
> > +
> > + switch (chan_type) {
> > + case IIO_ACCEL:
> > + reg = 0xc + (axis - IIO_MOD_X) * sizeof(sample);
>
> 0xc and 0x12 are magic values, give them names via defines.
> I assume they are the x access data registers.
>
Will fix in v3!
> > + break;
> > + case IIO_ANGL_VEL:
> > + reg = 0x12 + (axis - IIO_MOD_X) * sizeof(sample);
> > + break;
> > + default:
> > + return -EINVAL;
> > + }
> > +
> > + regmap_bulk_read(bmi270_device->regmap, reg, &sample, sizeof(sample));
> check for an error return.
>
Will fix in v3!
> It's fine with i2c to use a buffer on the stack (as it bounce buffers
> everything) but keep in mind that regmap in general doesn't guarantee that
> (even it happens to be the case today) so when you add SPI this will need
> to be a DMA safe buffer. Either allocate one on the heap, or embed one
> marked __aligned(IIO_DMA_MINALIGN) at the end of your iio_priv() structure.
>
> Note you only need to do this once you add spi support.
>
Will make note of this for upcoming SPI support.
> > + *val = sign_extend32(le16_to_cpu(sample), 15);
> > +
> > + return 0;
> > +}
> > +
> > +static int bmi270_read_raw(struct iio_dev *indio_dev,
> > + struct iio_chan_spec const *chan,
> > + int *val, int *val2, long mask)
> > +{
> > + struct bmi270_data *bmi270_device = iio_priv(indio_dev);
> > +
> > + switch (mask) {
> > + case IIO_CHAN_INFO_RAW:
> > + bmi270_get_data(bmi270_device, chan->type, chan->channel2, val);
>
> Check for error return and pass it on if there is one.
>
Will fix in v3.
> > + return IIO_VAL_INT;
> > + default:
> > + return -EINVAL;
> > + }
> > +
> > + return 0;
> unreachable code. Drop this last return.
>
Will fix in v3.
> > +}
> > +
> > +static const struct iio_info bmi270_info = {
> > + .read_raw = bmi270_read_raw,
> > +};
> > +
> > +static const struct iio_chan_spec bmi270_channels[] = {
> > + {
> > + .type = IIO_ACCEL,
> > + .modified = 1,
> > + .channel2 = IIO_MOD_X,
> > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> > + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) |
> > + BIT(IIO_CHAN_INFO_FREQUENCY),
> > + .scan_index = BMI270_SCAN_ACCEL_X,
> > + .scan_type = {
> > + .sign = 's',
> > + .realbits = 16,
> > + .storagebits = 16,
> > + .endianness = IIO_LE,
> > + },
> > + },
> > + {
> > + .type = IIO_ACCEL,
> > + .modified = 1,
> > + .channel2 = IIO_MOD_Y,
> > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> > + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) |
> > + BIT(IIO_CHAN_INFO_FREQUENCY),
> > + .scan_index = BMI270_SCAN_ACCEL_Y,
> > + .scan_type = {
> > + .sign = 's',
> > + .realbits = 16,
> > + .storagebits = 16,
> > + .endianness = IIO_LE,
> > + },
> > + },
> > + {
> > + .type = IIO_ACCEL,
> > + .modified = 1,
> > + .channel2 = IIO_MOD_Z,
> > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> > + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) |
> > + BIT(IIO_CHAN_INFO_FREQUENCY),
> > + .scan_index = BMI270_SCAN_ACCEL_Z,
> > + .scan_type = {
> > + .sign = 's',
> > + .realbits = 16,
> > + .storagebits = 16,
> > + .endianness = IIO_LE,
> > + },
> > + },
> > + {
> > + .type = IIO_ANGL_VEL,
>
> Perhaps a macro given 3 instances that only differ in _X _Y _Z.
> And another one for the acceleration channels.
>
>
Will create macro in v3.
> > + .modified = 1,
> > + .channel2 = IIO_MOD_X,
> > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> > + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) |
> > + BIT(IIO_CHAN_INFO_FREQUENCY),
> > + .scan_index = BMI270_SCAN_GYRO_X,
> > + .scan_type = {
> > + .sign = 's',
> > + .realbits = 16,
> > + .storagebits = 16,
> > + .endianness = IIO_LE,
> > + },
> > + },
> > + {
> > + .type = IIO_ANGL_VEL,
> > + .modified = 1,
> > + .channel2 = IIO_MOD_Y,
> > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> > + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) |
> > + BIT(IIO_CHAN_INFO_FREQUENCY),
> > + .scan_index = BMI270_SCAN_GYRO_Y,
> > + .scan_type = {
> > + .sign = 's',
> > + .realbits = 16,
> > + .storagebits = 16,
> > + .endianness = IIO_LE,
> > + },
> > +
> > + },
> > + {
> > + .type = IIO_ANGL_VEL,
> > + .modified = 1,
> > + .channel2 = IIO_MOD_Z,
> > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> > + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) |
> > + BIT(IIO_CHAN_INFO_FREQUENCY),
> > + .scan_index = BMI270_SCAN_GYRO_Z,
> > + .scan_type = {
> > + .sign = 's',
> > + .realbits = 16,
> > + .storagebits = 16,
> > + .endianness = IIO_LE,
>
> Until you add support for the buffer, scan_index and scan_type should be
> at least mostly irrelevant. If you aren't using them for something else, don't
> set them until the patch where you add buffer support.
>
Will drop in v3.
> > + },
> > + },
> > +};
> > +
> > +static int bmi270_validate_chip_id(struct bmi270_data *bmi270_device)
> > +{
> > + int chip_id;
> > + int ret;
> > + struct device *dev = bmi270_device->dev;
> > + struct regmap *regmap = bmi270_device->regmap;
> > +
> > + ret = regmap_read(regmap, BMI270_REG_CHIP_ID, &chip_id);
> > + if (ret)
> > + return dev_err_probe(dev, ret, "Failed to read chip id");
> > +
> > + if (chip_id != BMI270_CHIP_ID)
> > + return dev_err_probe(dev, -ENODEV, "Invalid chip id");
> > +
> > + return 0;
> > +}
> > +
> > +static int bmi270_write_init_data(struct bmi270_data *bmi270_device)
> Consider renaming. Perhaps bmi270_write_calibration_data()
> or something like that?
>
Will rename in v3.
> > +{
> > + int pwr_conf = 0;
> > + int ret;
> > + int status = 0;
> > + const struct firmware *init_data;
> > + struct device *dev = bmi270_device->dev;
> > + struct regmap *regmap = bmi270_device->regmap;
> > +
> > + ret = regmap_read(regmap, BMI270_REG_PWR_CONF, &pwr_conf);
> > + if (ret)
> > + return dev_err_probe(dev, ret, "Failed to read power configuration");
> > +
> > + pwr_conf &= 0xfffffffe;
>
> Probably define that as ~BIT(0) plus give it a name as that's not obvious.
> regmap_clear_bits() would be cleaner for clearing just one bit.
>
Will use regmap_clear_bits() in v3.
> > + ret = regmap_write(regmap, BMI270_REG_PWR_CONF, pwr_conf);
> > + if (ret)
> > + return dev_err_probe(dev, ret, "Failed to write power configuration");
> > +
> > + usleep_range(450, 1000);
> > +
> > + ret = regmap_write(regmap, BMI270_REG_INIT_CTRL, 0x0);
> > + if (ret)
> > + return dev_err_probe(dev, ret, "Failed to prepare device to load init data");
> > +
> > + ret = request_firmware(&init_data, BMI270_INIT_DATA_FILE, dev);
> > + if (ret)
> > + return dev_err_probe(dev, ret, "Failed to load init data file");
> > +
> > + ret = regmap_bulk_write(regmap, BMI270_REG_INIT_DATA,
> > + init_data->data, init_data->size);
> > + release_firmware(init_data);
> > + if (ret)
> > + return dev_err_probe(dev, ret, "Failed to write init data");
> > +
> > + ret = regmap_write(regmap, BMI270_REG_INIT_CTRL, 0x1);
> Give that bit a define even if it's the only one in the register.
>
Will do in v3.
> > + if (ret)
> > + return dev_err_probe(dev, ret, "Failed to stop device initialization");
> > +
> > + usleep_range(20000, 55000);
> > +
> > + ret = regmap_read(regmap, BMI270_REG_INTERNAL_STATUS, &status);
> > + if (ret)
> > + return dev_err_probe(dev, ret, "Failed to read internal status");
> > +
> > + if (status != 1)
>
> Define even for that 1. It must mean something?
>
Will define in v3.
> > + return dev_err_probe(dev, -ENODEV, "Device failed to initialize");
> > +
> > + return 0;
> > +}
> > +
> > +static int bmi270_configure_imu(struct bmi270_data *bmi270_device)
> > +{
> > + int ret;
> > + struct device *dev = bmi270_device->dev;
> > + struct regmap *regmap = bmi270_device->regmap;
> > +
> > + ret = regmap_write(regmap, BMI270_REG_PWR_CTRL, 0x0e);
>
> Magic register values. Assuming you know what these break down into
> please add the defines for each field so see can see what is
> being controlled by each write.
>
Will define in v3.
> > + if (ret)
> > + return dev_err_probe(dev, ret, "Failed to enable accelerometer and gyroscope");
> > +
> > + ret = regmap_write(regmap, BMI270_REG_ACC_CONF, 0xa8);
> > + if (ret)
> > + return dev_err_probe(dev, ret, "Failed to configure accelerometer");
> > +
> > + ret = regmap_write(regmap, BMI270_REG_GYR_CONF, 0xa9);
> > + if (ret)
> > + return dev_err_probe(dev, ret, "Failed to configure gyroscope");
> > +
> > + ret = regmap_write(regmap, BMI270_REG_PWR_CONF, 0x02);
> > + if (ret)
> > + return dev_err_probe(dev, ret, "Failed to set power configuration");
> > +
> > + return 0;
> > +}
> > +
> > +static int bmi270_chip_init(struct bmi270_data *bmi270_device)
> > +{
> > + int ret;
> > +
> > + ret = bmi270_validate_chip_id(bmi270_device);
> > + if (ret)
> > + return ret;
> > +
> > + ret = bmi270_write_init_data(bmi270_device);
> > + if (ret)
> > + return ret;
> > +
> > + ret = bmi270_configure_imu(bmi270_device);
> > + if (ret)
> > + return ret;
> > +
> > + return 0;
>
> return bmi270_configure_imu()
> saves a few lines for no significant loss of readability.
>
Will do in v3.
> > +}
> > +
> > +int bmi270_core_probe(struct device *dev, struct regmap *regmap,
> > + const char *name, bool use_spi)
>
> Drop the use_spi parameter. That isn't relevant yet (and may never be!)
>
Will do in v3.
> > +{
> > + int ret;
> > + struct bmi270_data *bmi270_device;
> > + struct iio_dev *indio_dev;
> > +
> > + indio_dev = devm_iio_device_alloc(dev, sizeof(struct bmi270_data *));
> > + if (!indio_dev)
> > + return -ENOMEM;
> > +
> > + bmi270_device = iio_priv(indio_dev);
> > + bmi270_device->dev = dev;
> > + bmi270_device->regmap = regmap;
> > +
> > + dev_set_drvdata(dev, indio_dev);
> Is this ever used? If not, don't set it.
>
Will drop in v3.
> > +
> > + ret = bmi270_chip_init(bmi270_device);
> > + if (ret)
> > + return ret;
> > +
> > + indio_dev->channels = bmi270_channels;
> > + indio_dev->num_channels = ARRAY_SIZE(bmi270_channels);
> > + indio_dev->name = name;
> > + indio_dev->modes = INDIO_DIRECT_MODE;
> > + indio_dev->info = &bmi270_info;
> > +
> > + return devm_iio_device_register(dev, indio_dev);
> > +}
> > +EXPORT_SYMBOL_NS_GPL(bmi270_core_probe, IIO_BMI270);
> > +
> > +MODULE_AUTHOR("Alex Lanzano");
> > +MODULE_DESCRIPTION("BMI270 driver");
> > +MODULE_LICENSE("GPL");
> > diff --git a/drivers/iio/imu/bmi270/bmi270_i2c.c b/drivers/iio/imu/bmi270/bmi270_i2c.c
> > new file mode 100644
> > index 000000000000..2a18c3af92d2
> > --- /dev/null
> > +++ b/drivers/iio/imu/bmi270/bmi270_i2c.c
> > @@ -0,0 +1,56 @@
> > +// SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +
> > +#include <linux/module.h>
> > +#include <linux/i2c.h>
> > +#include <linux/iio/iio.h>
> > +#include <linux/module.h>
>
> mod_devicetable.h
>
Will add in v3.
> > +#include <linux/regmap.h>
> > +
> > +#include "bmi270.h"
> > +
> > +static int bmi270_i2c_probe(struct i2c_client *client)
> > +{
> > + const char *name;
> > + struct regmap *regmap;
> > + struct device *dev = &client->dev;
> > + const struct i2c_device_id *id;
> > +
> > + regmap = devm_regmap_init_i2c(client, &bmi270_regmap_config);
> > + if (IS_ERR(regmap)) {
> > + return dev_err_probe(dev, PTR_ERR(regmap),
> > + "Failed to init i2c regmap");
> > + }
>
> No {} needed around a single statement like this.
>
Will fix in v3.
> > +
> > + id = i2c_client_get_device_id(client);
>
> For modern drivers, should only need to get the associated data
> and i2c_get_match_data() deals with various firmware types.
> You only need that once multiple chips are supported.
> For now there should be no reason to query this.
>
Will drop in v3.
> > + if (id)
> > + name = id->name;
> > + else
> > + name = dev_name(dev);
>
> Until the driver supports multiple devices, hardcode this
> in the core code as "bmi270"
>
> When multiple parts are supported, use a chip type specific
> structure with a const char * in it.
> naming via id->name or dev_name and similar tends to give
> unstable results that aren't always the part number of the
> device.
>
Will hardcode this in v3.
> > +
> > + return bmi270_core_probe(dev, regmap, name, false);
> > +}
> > +
> > +static const struct i2c_device_id bmi270_i2c_id[] = {
> > + {"bmi270", 0},
> > + {}
> > +};
> > +
> > +static const struct of_device_id bmi270_of_match[] = {
> > + {.compatible = "bosch,bmi270"},
> > + {},
> Preferred style (I'm slowly tidying this up across IIO) is
> { .compatible = "bosch,bmi270" },
> { }
>
> So spaces and no comma after terminator as we never want to
> add anything after that.
> Applies to other similar cases.
>
Will fix in v3!
> > +};
>
>
Thanks for the review! Will fix this up and resend!
Best regards,
Alex
