| Message ID | 20240927183717.3613601-1-lanzano.alex@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: imu: bmi270: Add spi driver for bmi270 imu | expand |
On Fri, 27 Sep 2024 14:37:10 -0400 Alex Lanzano <lanzano.alex@gmail.com> wrote: > Implement SPI driver for the Bosch BMI270 6-axis IMU. Provide raw read > write access to acceleration and angle velocity measurements via the SPI > interface on the device. > > Signed-off-by: Alex Lanzano <lanzano.alex@gmail.com> A few minor things inline but looks good in general. Jonathan > diff --git a/drivers/iio/imu/bmi270/bmi270.h b/drivers/iio/imu/bmi270/bmi270.h > index 608b29ea58a3..8950e6234203 100644 > --- a/drivers/iio/imu/bmi270/bmi270.h > +++ b/drivers/iio/imu/bmi270/bmi270.h > @@ -4,11 +4,13 @@ > #define BMI270_H_ > > #include <linux/regmap.h> > +#include <linux/iio/iio.h> > > struct device; > struct bmi270_data { > struct device *dev; > struct regmap *regmap; > + __le16 sample __aligned(IIO_DMA_MINALIGN); For the read path you are bouncing anyway, so the DMA_MINALIGN is only needed for anything the write direction. Make the suggested change below and that will bounce as well so that you don't need this. > }; > > extern const struct regmap_config bmi270_regmap_config; > diff --git a/drivers/iio/imu/bmi270/bmi270_core.c b/drivers/iio/imu/bmi270/bmi270_core.c > index 8e45343d6472..4decdad791d9 100644 > --- a/drivers/iio/imu/bmi270/bmi270_core.c > +++ b/drivers/iio/imu/bmi270/bmi270_core.c > @@ -66,16 +66,9 @@ enum bmi270_scan { > 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; > int ret; > > @@ -90,11 +83,13 @@ static int bmi270_get_data(struct bmi270_data *bmi270_device, > return -EINVAL; > } > > - ret = regmap_bulk_read(bmi270_device->regmap, reg, &sample, sizeof(sample)); > + ret = regmap_bulk_read(bmi270_device->regmap, reg, > + &bmi270_device->sample, > + sizeof(bmi270_device->sample)); > if (ret) > return ret; > > - *val = sign_extend32(le16_to_cpu(sample), 15); > + *val = sign_extend32(le16_to_cpu(bmi270_device->sample), 15); > > return 0; > } > diff --git a/drivers/iio/imu/bmi270/bmi270_i2c.c b/drivers/iio/imu/bmi270/bmi270_i2c.c > index f70dee2d8a64..ce8279ae90cd 100644 > --- a/drivers/iio/imu/bmi270/bmi270_i2c.c > +++ b/drivers/iio/imu/bmi270/bmi270_i2c.c > @@ -9,12 +9,17 @@ > > #include "bmi270.h" > > +const struct regmap_config bmi270_i2c_regmap_config = { static const (same for spi one) > + .reg_bits = 8, > + .val_bits = 8, > +}; > + > static int bmi270_i2c_probe(struct i2c_client *client) > { > struct regmap *regmap; > struct device *dev = &client->dev; > > - regmap = devm_regmap_init_i2c(client, &bmi270_regmap_config); > + regmap = devm_regmap_init_i2c(client, &bmi270_i2c_regmap_config); > if (IS_ERR(regmap)) > return dev_err_probe(dev, PTR_ERR(regmap), > "Failed to init i2c regmap"); > diff --git a/drivers/iio/imu/bmi270/bmi270_spi.c b/drivers/iio/imu/bmi270/bmi270_spi.c > new file mode 100644 > index 000000000000..906b9b852a09 > --- /dev/null > +++ b/drivers/iio/imu/bmi270/bmi270_spi.c > @@ -0,0 +1,89 @@ > +// SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > + > +#include <linux/module.h> > +#include <linux/spi/spi.h> > +#include <linux/iio/iio.h> > +#include <linux/module.h> > +#include <linux/mod_devicetable.h> > +#include <linux/regmap.h> Alphabetical order preferred. > + > +#include "bmi270.h" > + > +/* > + * The following two functions are taken from the BMI323 spi driver code. > + * In section 6.4 of the BMI270 data it specifies that after a read > + * operation the first data byte from the device is a dummy byte > + */ > +static int bmi270_regmap_spi_read(void *context, const void *reg_buf, > + size_t reg_size, void *val_buf, > + size_t val_size) > +{ > + struct spi_device *spi = context; I'd be tempted to rename the input parameter context to spi and then parse it directly to the spi_write_then_read() > + > + return spi_write_then_read(spi, reg_buf, reg_size, val_buf, val_size); > +} > + > +static int bmi270_regmap_spi_write(void *context, const void *data, > + size_t count) > +{ > + struct spi_device *spi = context; > + u8 *data_buff = (u8 *)data; > + > + /* > + * Remove the extra pad byte since its only needed for the read > + * operation > + */ > + data_buff[1] = data_buff[0]; > + return spi_write(spi, data_buff + 1, count - 1); That needs a DMA safe buffer (unlike write_then_read which always bounces). I'd avoid that complexity by using spi_write_then_read here as well but set the read to 0 length and pass NULL for the buffer. That function is intended to be used like this as it special cases 0 length for either write or read buffers. > +}
Thanks for the review! On Sun, Sep 29, 2024 at 03:54:37PM GMT, Jonathan Cameron wrote: > On Fri, 27 Sep 2024 14:37:10 -0400 > Alex Lanzano <lanzano.alex@gmail.com> wrote: > > > Implement SPI driver for the Bosch BMI270 6-axis IMU. Provide raw read > > write access to acceleration and angle velocity measurements via the SPI > > interface on the device. > > > > Signed-off-by: Alex Lanzano <lanzano.alex@gmail.com> > > A few minor things inline but looks good in general. > > Jonathan > > > diff --git a/drivers/iio/imu/bmi270/bmi270.h b/drivers/iio/imu/bmi270/bmi270.h > > index 608b29ea58a3..8950e6234203 100644 > > --- a/drivers/iio/imu/bmi270/bmi270.h > > +++ b/drivers/iio/imu/bmi270/bmi270.h > > @@ -4,11 +4,13 @@ > > #define BMI270_H_ > > > > #include <linux/regmap.h> > > +#include <linux/iio/iio.h> > > > > struct device; > > struct bmi270_data { > > struct device *dev; > > struct regmap *regmap; > > + __le16 sample __aligned(IIO_DMA_MINALIGN); > > For the read path you are bouncing anyway, so the DMA_MINALIGN is only needed > for anything the write direction. Make the suggested change below and that > will bounce as well so that you don't need this. > Understood! Will remove in v2 and use spi_write_then_read > > }; > > > > extern const struct regmap_config bmi270_regmap_config; > > diff --git a/drivers/iio/imu/bmi270/bmi270_core.c b/drivers/iio/imu/bmi270/bmi270_core.c > > index 8e45343d6472..4decdad791d9 100644 > > --- a/drivers/iio/imu/bmi270/bmi270_core.c > > +++ b/drivers/iio/imu/bmi270/bmi270_core.c > > @@ -66,16 +66,9 @@ enum bmi270_scan { > > 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; > > int ret; > > > > @@ -90,11 +83,13 @@ static int bmi270_get_data(struct bmi270_data *bmi270_device, > > return -EINVAL; > > } > > > > - ret = regmap_bulk_read(bmi270_device->regmap, reg, &sample, sizeof(sample)); > > + ret = regmap_bulk_read(bmi270_device->regmap, reg, > > + &bmi270_device->sample, > > + sizeof(bmi270_device->sample)); > > if (ret) > > return ret; > > > > - *val = sign_extend32(le16_to_cpu(sample), 15); > > + *val = sign_extend32(le16_to_cpu(bmi270_device->sample), 15); > > > > return 0; > > } > > diff --git a/drivers/iio/imu/bmi270/bmi270_i2c.c b/drivers/iio/imu/bmi270/bmi270_i2c.c > > index f70dee2d8a64..ce8279ae90cd 100644 > > --- a/drivers/iio/imu/bmi270/bmi270_i2c.c > > +++ b/drivers/iio/imu/bmi270/bmi270_i2c.c > > @@ -9,12 +9,17 @@ > > > > #include "bmi270.h" > > > > +const struct regmap_config bmi270_i2c_regmap_config = { > static const > > (same for spi one) > Will fix in v2! > > + .reg_bits = 8, > > + .val_bits = 8, > > +}; > > + > > static int bmi270_i2c_probe(struct i2c_client *client) > > { > > struct regmap *regmap; > > struct device *dev = &client->dev; > > > > - regmap = devm_regmap_init_i2c(client, &bmi270_regmap_config); > > + regmap = devm_regmap_init_i2c(client, &bmi270_i2c_regmap_config); > > if (IS_ERR(regmap)) > > return dev_err_probe(dev, PTR_ERR(regmap), > > "Failed to init i2c regmap"); > > diff --git a/drivers/iio/imu/bmi270/bmi270_spi.c b/drivers/iio/imu/bmi270/bmi270_spi.c > > new file mode 100644 > > index 000000000000..906b9b852a09 > > --- /dev/null > > +++ b/drivers/iio/imu/bmi270/bmi270_spi.c > > @@ -0,0 +1,89 @@ > > +// SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > + > > +#include <linux/module.h> > > +#include <linux/spi/spi.h> > > +#include <linux/iio/iio.h> > > +#include <linux/module.h> > > +#include <linux/mod_devicetable.h> > > +#include <linux/regmap.h> > Alphabetical order preferred. > Will fix in v2 > > + > > +#include "bmi270.h" > > + > > +/* > > + * The following two functions are taken from the BMI323 spi driver code. > > + * In section 6.4 of the BMI270 data it specifies that after a read > > + * operation the first data byte from the device is a dummy byte > > + */ > > +static int bmi270_regmap_spi_read(void *context, const void *reg_buf, > > + size_t reg_size, void *val_buf, > > + size_t val_size) > > +{ > > + struct spi_device *spi = context; > > I'd be tempted to rename the input parameter context to spi and then > parse it directly to the spi_write_then_read() > Will do! I named it context since that's what the function pointer uses > > + > > + return spi_write_then_read(spi, reg_buf, reg_size, val_buf, val_size); > > +} > > + > > +static int bmi270_regmap_spi_write(void *context, const void *data, > > + size_t count) > > +{ > > + struct spi_device *spi = context; > > + u8 *data_buff = (u8 *)data; > > + > > + /* > > + * Remove the extra pad byte since its only needed for the read > > + * operation > > + */ > > + data_buff[1] = data_buff[0]; > > + return spi_write(spi, data_buff + 1, count - 1); > That needs a DMA safe buffer (unlike write_then_read which always > bounces). I'd avoid that complexity by using spi_write_then_read > here as well but set the read to 0 length and pass NULL for the buffer. > That function is intended to be used like this as it special cases 0 > length for either write or read buffers. > Understood! Will fix in v2. > > +} >
diff --git a/drivers/iio/imu/bmi270/Kconfig b/drivers/iio/imu/bmi270/Kconfig index a8db44187286..0ffd29794fda 100644 --- a/drivers/iio/imu/bmi270/Kconfig +++ b/drivers/iio/imu/bmi270/Kconfig @@ -18,3 +18,15 @@ config BMI270_I2C This driver can also be built as a module. If so, the module will be called bmi270_i2c. + +config BMI270_SPI + tristate "Bosch BMI270 SPI driver" + depends on SPI + select BMI270 + select REGMAP_SPI + help + Enable support for the Bosch BMI270 6-Axis IMU connected to SPI + interface. + + This driver can also be built as a module. If so, the module will be + called bmi270_spi. diff --git a/drivers/iio/imu/bmi270/Makefile b/drivers/iio/imu/bmi270/Makefile index ab4acaaee6d2..d96c96fc3d83 100644 --- a/drivers/iio/imu/bmi270/Makefile +++ b/drivers/iio/imu/bmi270/Makefile @@ -4,3 +4,4 @@ # obj-$(CONFIG_BMI270) += bmi270_core.o obj-$(CONFIG_BMI270_I2C) += bmi270_i2c.o +obj-$(CONFIG_BMI270_SPI) += bmi270_spi.o diff --git a/drivers/iio/imu/bmi270/bmi270.h b/drivers/iio/imu/bmi270/bmi270.h index 608b29ea58a3..8950e6234203 100644 --- a/drivers/iio/imu/bmi270/bmi270.h +++ b/drivers/iio/imu/bmi270/bmi270.h @@ -4,11 +4,13 @@ #define BMI270_H_ #include <linux/regmap.h> +#include <linux/iio/iio.h> struct device; struct bmi270_data { struct device *dev; struct regmap *regmap; + __le16 sample __aligned(IIO_DMA_MINALIGN); }; extern const struct regmap_config bmi270_regmap_config; diff --git a/drivers/iio/imu/bmi270/bmi270_core.c b/drivers/iio/imu/bmi270/bmi270_core.c index 8e45343d6472..4decdad791d9 100644 --- a/drivers/iio/imu/bmi270/bmi270_core.c +++ b/drivers/iio/imu/bmi270/bmi270_core.c @@ -66,16 +66,9 @@ enum bmi270_scan { 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; int ret; @@ -90,11 +83,13 @@ static int bmi270_get_data(struct bmi270_data *bmi270_device, return -EINVAL; } - ret = regmap_bulk_read(bmi270_device->regmap, reg, &sample, sizeof(sample)); + ret = regmap_bulk_read(bmi270_device->regmap, reg, + &bmi270_device->sample, + sizeof(bmi270_device->sample)); if (ret) return ret; - *val = sign_extend32(le16_to_cpu(sample), 15); + *val = sign_extend32(le16_to_cpu(bmi270_device->sample), 15); return 0; } diff --git a/drivers/iio/imu/bmi270/bmi270_i2c.c b/drivers/iio/imu/bmi270/bmi270_i2c.c index f70dee2d8a64..ce8279ae90cd 100644 --- a/drivers/iio/imu/bmi270/bmi270_i2c.c +++ b/drivers/iio/imu/bmi270/bmi270_i2c.c @@ -9,12 +9,17 @@ #include "bmi270.h" +const struct regmap_config bmi270_i2c_regmap_config = { + .reg_bits = 8, + .val_bits = 8, +}; + static int bmi270_i2c_probe(struct i2c_client *client) { struct regmap *regmap; struct device *dev = &client->dev; - regmap = devm_regmap_init_i2c(client, &bmi270_regmap_config); + regmap = devm_regmap_init_i2c(client, &bmi270_i2c_regmap_config); if (IS_ERR(regmap)) return dev_err_probe(dev, PTR_ERR(regmap), "Failed to init i2c regmap"); diff --git a/drivers/iio/imu/bmi270/bmi270_spi.c b/drivers/iio/imu/bmi270/bmi270_spi.c new file mode 100644 index 000000000000..906b9b852a09 --- /dev/null +++ b/drivers/iio/imu/bmi270/bmi270_spi.c @@ -0,0 +1,89 @@ +// SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) + +#include <linux/module.h> +#include <linux/spi/spi.h> +#include <linux/iio/iio.h> +#include <linux/module.h> +#include <linux/mod_devicetable.h> +#include <linux/regmap.h> + +#include "bmi270.h" + +/* + * The following two functions are taken from the BMI323 spi driver code. + * In section 6.4 of the BMI270 data it specifies that after a read + * operation the first data byte from the device is a dummy byte + */ +static int bmi270_regmap_spi_read(void *context, const void *reg_buf, + size_t reg_size, void *val_buf, + size_t val_size) +{ + struct spi_device *spi = context; + + return spi_write_then_read(spi, reg_buf, reg_size, val_buf, val_size); +} + +static int bmi270_regmap_spi_write(void *context, const void *data, + size_t count) +{ + struct spi_device *spi = context; + u8 *data_buff = (u8 *)data; + + /* + * Remove the extra pad byte since its only needed for the read + * operation + */ + data_buff[1] = data_buff[0]; + return spi_write(spi, data_buff + 1, count - 1); +} + +static const struct regmap_bus bmi270_regmap_bus = { + .read = bmi270_regmap_spi_read, + .write = bmi270_regmap_spi_write, +}; + +const struct regmap_config bmi270_spi_regmap_config = { + .reg_bits = 8, + .val_bits = 8, + .pad_bits = 8, + .read_flag_mask = BIT(7), +}; + +static int bmi270_spi_probe(struct spi_device *spi) +{ + struct regmap *regmap; + struct device *dev = &spi->dev; + + regmap = devm_regmap_init(dev, &bmi270_regmap_bus, dev, + &bmi270_spi_regmap_config); + if (IS_ERR(regmap)) + return dev_err_probe(dev, PTR_ERR(regmap), + "Failed to init i2c regmap"); + + return bmi270_core_probe(dev, regmap); +} + +static const struct spi_device_id bmi270_spi_id[] = { + { "bmi270" }, + { } +}; + +static const struct of_device_id bmi270_of_match[] = { + { .compatible = "bosch,bmi270" }, + { } +}; + +static struct spi_driver bmi270_spi_driver = { + .driver = { + .name = "bmi270", + .of_match_table = bmi270_of_match, + }, + .probe = bmi270_spi_probe, + .id_table = bmi270_spi_id, +}; +module_spi_driver(bmi270_spi_driver); + +MODULE_AUTHOR("Alex Lanzano"); +MODULE_DESCRIPTION("BMI270 driver"); +MODULE_LICENSE("GPL"); +MODULE_IMPORT_NS(IIO_BMI270);
Implement SPI driver for the Bosch BMI270 6-axis IMU. Provide raw read write access to acceleration and angle velocity measurements via the SPI interface on the device. Signed-off-by: Alex Lanzano <lanzano.alex@gmail.com> --- drivers/iio/imu/bmi270/Kconfig | 12 ++++ drivers/iio/imu/bmi270/Makefile | 1 + drivers/iio/imu/bmi270/bmi270.h | 2 + drivers/iio/imu/bmi270/bmi270_core.c | 13 ++-- drivers/iio/imu/bmi270/bmi270_i2c.c | 7 ++- drivers/iio/imu/bmi270/bmi270_spi.c | 89 ++++++++++++++++++++++++++++ 6 files changed, 114 insertions(+), 10 deletions(-) create mode 100644 drivers/iio/imu/bmi270/bmi270_spi.c