Message-ID: <ltddorwstm4hbsczsf4klqaoboagvdj3bbdhwyvbeslfdnnoc6@6fow6su5fju4>
Date: Sun, 8 Sep 2024 15:04:03 -0400
From: Alex Lanzano <lanzano.alex@...il.com>
To: Jonathan Cameron <jic23@...nel.org>
Cc: Lars-Peter Clausen <lars@...afoo.de>, Rob Herring <robh@...nel.org>, 
	Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, 
	Jagath Jog J <jagathjog1996@...il.com>, Ramona Gradinariu <ramona.bolboaca13@...il.com>, 
	Nuno Sa <nuno.sa@...log.com>, Jonathan Cameron <Jonathan.Cameron@...wei.com>, 
	linux-iio@...r.kernel.org, devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/2] iio: imu: Add i2c driver for bmi270 imu

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

Powered by blists - more mailing lists