diff mbox series

[v2,2/2] iio: imu: Add i2c driver for bmi270 imu

Message ID 20240906165322.1745328-3-lanzano.alex@gmail.com (mailing list archive)
State Changes Requested
Headers show
Series Add I2C driver for Bosch BMI270 IMU | expand

Commit Message

Alex Lanzano Sept. 6, 2024, 4:52 p.m. UTC
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@gmail.com>
---
 MAINTAINERS                          |   7 +
 drivers/iio/imu/Kconfig              |   1 +
 drivers/iio/imu/Makefile             |   1 +
 drivers/iio/imu/bmi270/Kconfig       |  22 ++
 drivers/iio/imu/bmi270/Makefile      |   6 +
 drivers/iio/imu/bmi270/bmi270.h      |  18 ++
 drivers/iio/imu/bmi270/bmi270_core.c | 322 +++++++++++++++++++++++++++
 drivers/iio/imu/bmi270/bmi270_i2c.c  |  56 +++++
 8 files changed, 433 insertions(+)
 create mode 100644 drivers/iio/imu/bmi270/Kconfig
 create mode 100644 drivers/iio/imu/bmi270/Makefile
 create mode 100644 drivers/iio/imu/bmi270/bmi270.h
 create mode 100644 drivers/iio/imu/bmi270/bmi270_core.c
 create mode 100644 drivers/iio/imu/bmi270/bmi270_i2c.c

Comments

Jonathan Cameron Sept. 7, 2024, 4:08 p.m. UTC | #1
On Fri,  6 Sep 2024 12:52:51 -0400
Alex Lanzano <lanzano.alex@gmail.com> 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@gmail.com>

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

> 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.

> +
> +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.

> +	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.

> +		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.

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.

> +	*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.

> +		return IIO_VAL_INT;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
unreachable code. Drop this last return.

> +}
> +
> +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.


> +		.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.

> +		},
> +	},
> +};
> +
> +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?

> +{
> +	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.

> +	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.

> +	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?

> +		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.

> +	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.

> +}
> +
> +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!)

> +{
> +	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.

> +
> +	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

> +#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.

> +
> +	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.

> +	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.

> +
> +	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.

> +};
Alex Lanzano Sept. 8, 2024, 7:04 p.m. UTC | #2
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@gmail.com> 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@gmail.com>
> 
> 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
diff mbox series

Patch

diff --git a/MAINTAINERS b/MAINTAINERS
index a2184637a5d9..6612d27525b5 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3928,6 +3928,13 @@  S:	Maintained
 F:	Documentation/devicetree/bindings/iio/accel/bosch,bma400.yaml
 F:	drivers/iio/accel/bma400*
 
+BOSCH SENSORTEC BMI270 IMU IIO DRIVER
+M:	Alex Lanzano <lanzano.alex@gmail.com>
+L:	linux-iio@vger.kernel.org
+S:	Maintained
+F:	Documentation/devicetree/bindings/iio/imu/bosch,bmi270.yaml
+F:	drivers/iio/imu/bmi270/
+
 BOSCH SENSORTEC BMI323 IMU IIO DRIVER
 M:	Jagath Jog J <jagathjog1996@gmail.com>
 L:	linux-iio@vger.kernel.org
diff --git a/drivers/iio/imu/Kconfig b/drivers/iio/imu/Kconfig
index 782fb80e44c2..489dd898830b 100644
--- a/drivers/iio/imu/Kconfig
+++ b/drivers/iio/imu/Kconfig
@@ -53,6 +53,7 @@  config ADIS16480
 	  ADIS16485, ADIS16488 inertial sensors.
 
 source "drivers/iio/imu/bmi160/Kconfig"
+source "drivers/iio/imu/bmi270/Kconfig"
 source "drivers/iio/imu/bmi323/Kconfig"
 source "drivers/iio/imu/bno055/Kconfig"
 
diff --git a/drivers/iio/imu/Makefile b/drivers/iio/imu/Makefile
index 7e2d7d5c3b7b..79f83ea6f644 100644
--- a/drivers/iio/imu/Makefile
+++ b/drivers/iio/imu/Makefile
@@ -15,6 +15,7 @@  adis_lib-$(CONFIG_IIO_ADIS_LIB_BUFFER) += adis_buffer.o
 obj-$(CONFIG_IIO_ADIS_LIB) += adis_lib.o
 
 obj-y += bmi160/
+obj-y += bmi270/
 obj-y += bmi323/
 obj-y += bno055/
 
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
+
+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/Makefile b/drivers/iio/imu/bmi270/Makefile
new file mode 100644
index 000000000000..ab4acaaee6d2
--- /dev/null
+++ b/drivers/iio/imu/bmi270/Makefile
@@ -0,0 +1,6 @@ 
+# SPDX-License-Identifier: GPL-2.0
+#
+# Makefile for Bosch BMI270 IMU
+#
+obj-$(CONFIG_BMI270) += bmi270_core.o
+obj-$(CONFIG_BMI270_I2C) += bmi270_i2c.o
diff --git a/drivers/iio/imu/bmi270/bmi270.h b/drivers/iio/imu/bmi270/bmi270.h
new file mode 100644
index 000000000000..52e806529748
--- /dev/null
+++ b/drivers/iio/imu/bmi270/bmi270.h
@@ -0,0 +1,18 @@ 
+/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
+
+#ifndef BMI270_H_
+#define BMI270_H_
+
+#include <linux/iio/iio.h>
+
+struct bmi270_data {
+	struct device *dev;
+	struct regmap *regmap;
+};
+
+extern const struct regmap_config bmi270_regmap_config;
+
+int bmi270_core_probe(struct device *dev, struct regmap *regmap,
+		      const char *name, bool use_spi);
+
+#endif  /* BMI270_H_ */
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 {
+	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,
+};
+
+enum bmi270_scan {
+	BMI270_SCAN_ACCEL_X,
+	BMI270_SCAN_ACCEL_Y,
+	BMI270_SCAN_ACCEL_Z,
+	BMI270_SCAN_GYRO_X,
+	BMI270_SCAN_GYRO_Y,
+	BMI270_SCAN_GYRO_Z,
+};
+
+const struct regmap_config bmi270_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+};
+EXPORT_SYMBOL_NS_GPL(bmi270_regmap_config, IIO_BMI270);
+
+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);
+		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));
+	*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);
+		return IIO_VAL_INT;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+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,
+		.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,
+		},
+	},
+};
+
+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)
+{
+	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;
+	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);
+	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)
+		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);
+	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;
+}
+
+int bmi270_core_probe(struct device *dev, struct regmap *regmap,
+		      const char *name, bool use_spi)
+{
+	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);
+
+	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>
+#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");
+	}
+
+	id = i2c_client_get_device_id(client);
+	if (id)
+		name = id->name;
+	else
+		name = dev_name(dev);
+
+	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"},
+	{},
+};
+
+static struct i2c_driver bmi270_i2c_driver = {
+	.driver = {
+		.name = "bmi270_i2c",
+		.of_match_table = bmi270_of_match,
+	},
+	.probe = bmi270_i2c_probe,
+	.id_table = bmi270_i2c_id,
+};
+module_i2c_driver(bmi270_i2c_driver);
+
+MODULE_AUTHOR("Alex Lanzano");
+MODULE_DESCRIPTION("BMI270 driver");
+MODULE_LICENSE("GPL");
+MODULE_IMPORT_NS(IIO_BMI270);