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