| Message ID | 20241018233723.28757-5-justin@justinweiss.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Add i2c driver for Bosch BMI260 IMU | expand |
On Fri, 18 Oct 2024 16:36:10 -0700 Justin Weiss <justin@justinweiss.com> wrote: > Adds support for the Bosch BMI260 6-axis IMU to the Bosch BMI270 > driver. Setup and operation is nearly identical to the Bosch BMI270, > but has a different chip ID and requires different firmware. > > Firmware is requested and loaded from userspace. > > Signed-off-by: Justin Weiss <justin@justinweiss.com> Trivial comments inline and a discussion on whether my earlier don't use an array comment makes sense in this particular case. Jonathan > --- > drivers/iio/imu/bmi270/bmi270.h | 1 + > drivers/iio/imu/bmi270/bmi270_core.c | 25 +++++++++++++++++++++++-- > drivers/iio/imu/bmi270/bmi270_i2c.c | 13 +++++++++++++ > drivers/iio/imu/bmi270/bmi270_spi.c | 8 ++++++++ > 4 files changed, 45 insertions(+), 2 deletions(-) > > diff --git a/drivers/iio/imu/bmi270/bmi270.h b/drivers/iio/imu/bmi270/bmi270.h > index 2e8d85a4e419..51e374fd4290 100644 > --- a/drivers/iio/imu/bmi270/bmi270.h > +++ b/drivers/iio/imu/bmi270/bmi270.h > @@ -14,6 +14,7 @@ struct bmi270_data { > }; > > enum bmi270_device_type { > + BMI260, > BMI270, > }; > > diff --git a/drivers/iio/imu/bmi270/bmi270_core.c b/drivers/iio/imu/bmi270/bmi270_core.c > index 799df78ec862..b30201dc4e22 100644 > --- a/drivers/iio/imu/bmi270/bmi270_core.c > +++ b/drivers/iio/imu/bmi270/bmi270_core.c > @@ -11,6 +11,8 @@ > #include "bmi270.h" > > #define BMI270_CHIP_ID_REG 0x00 > +#define BMI160_CHIP_ID_VAL 0xD1 This one looks like a cut and paste error. > +#define BMI260_CHIP_ID_VAL 0x27 > #define BMI270_CHIP_ID_VAL 0x24 > #define BMI270_CHIP_ID_MSK GENMASK(7, 0) > > @@ -55,6 +57,7 @@ > #define BMI270_PWR_CTRL_ACCEL_EN_MSK BIT(2) > #define BMI270_PWR_CTRL_TEMP_EN_MSK BIT(3) > > +#define BMI260_INIT_DATA_FILE "bmi260-init-data.fw" > #define BMI270_INIT_DATA_FILE "bmi270-init-data.fw" > > enum bmi270_scan { > @@ -67,6 +70,11 @@ enum bmi270_scan { > }; > > const struct bmi270_chip_info bmi270_chip_info[] = { > + [BMI260] = { > + .name = "bmi260", > + .chip_id = BMI260_CHIP_ID_VAL, > + .fw_name = BMI260_INIT_DATA_FILE, > + }, > [BMI270] = { > .name = "bmi270", > .chip_id = BMI270_CHIP_ID_VAL, > @@ -163,8 +171,21 @@ static int bmi270_validate_chip_id(struct bmi270_data *bmi270_device) > if (ret) > return dev_err_probe(dev, ret, "Failed to read chip id"); > > - if (chip_id != BMI270_CHIP_ID_VAL) > - dev_info(dev, "Unknown chip id 0x%x", chip_id); > + /* > + * Some manufacturers use "BMI0160" for both the BMI160 and > + * BMI260. If the device is actually a BMI160, the bmi160 > + * driver should handle it and this driver should not. > + */ > + if (chip_id == BMI160_CHIP_ID_VAL) > + return -ENODEV; > + > + if (chip_id != bmi270_device->chip_info->chip_id) > + dev_info(dev, "Unexpected chip id 0x%x", chip_id); > + > + if (chip_id == BMI260_CHIP_ID_VAL) Ah. My argument on separate IDs means you'd have to do it this way whereas I was thinking maybe a loop would be a better idea. Ah well if we get a lot of supported chips, then we can rethink how to handle this. For now what you have here is fine and should deal with lack of appropriate ACPI ID mess. > + bmi270_device->chip_info = &bmi270_chip_info[BMI260]; > + else if (chip_id == BMI270_CHIP_ID_VAL) > + bmi270_device->chip_info = &bmi270_chip_info[BMI270]; > > return 0; > }
Jonathan Cameron <jic23@kernel.org> writes: > On Fri, 18 Oct 2024 16:36:10 -0700 > Justin Weiss <justin@justinweiss.com> wrote: > >> Adds support for the Bosch BMI260 6-axis IMU to the Bosch BMI270 >> driver. Setup and operation is nearly identical to the Bosch BMI270, >> but has a different chip ID and requires different firmware. >> >> Firmware is requested and loaded from userspace. >> >> Signed-off-by: Justin Weiss <justin@justinweiss.com> > Trivial comments inline and a discussion on whether my earlier > don't use an array comment makes sense in this particular case. > > Jonathan > >> --- >> drivers/iio/imu/bmi270/bmi270.h | 1 + >> drivers/iio/imu/bmi270/bmi270_core.c | 25 +++++++++++++++++++++++-- >> drivers/iio/imu/bmi270/bmi270_i2c.c | 13 +++++++++++++ >> drivers/iio/imu/bmi270/bmi270_spi.c | 8 ++++++++ >> 4 files changed, 45 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/iio/imu/bmi270/bmi270.h b/drivers/iio/imu/bmi270/bmi270.h >> index 2e8d85a4e419..51e374fd4290 100644 >> --- a/drivers/iio/imu/bmi270/bmi270.h >> +++ b/drivers/iio/imu/bmi270/bmi270.h >> @@ -14,6 +14,7 @@ struct bmi270_data { >> }; >> >> enum bmi270_device_type { >> + BMI260, >> BMI270, >> }; >> >> diff --git a/drivers/iio/imu/bmi270/bmi270_core.c b/drivers/iio/imu/bmi270/bmi270_core.c >> index 799df78ec862..b30201dc4e22 100644 >> --- a/drivers/iio/imu/bmi270/bmi270_core.c >> +++ b/drivers/iio/imu/bmi270/bmi270_core.c >> @@ -11,6 +11,8 @@ >> #include "bmi270.h" >> >> #define BMI270_CHIP_ID_REG 0x00 >> +#define BMI160_CHIP_ID_VAL 0xD1 > > This one looks like a cut and paste error. No, this was intentional -- I added the BMI160 chip ID here so it could be checked later to avoid conflicting with the existing bmi160 driver. I could add newlines before and after this group of _ID_VAL #defines if it makes it clearer. >> +#define BMI260_CHIP_ID_VAL 0x27 >> #define BMI270_CHIP_ID_VAL 0x24 >> #define BMI270_CHIP_ID_MSK GENMASK(7, 0) >> >> @@ -55,6 +57,7 @@ >> #define BMI270_PWR_CTRL_ACCEL_EN_MSK BIT(2) >> #define BMI270_PWR_CTRL_TEMP_EN_MSK BIT(3) >> >> +#define BMI260_INIT_DATA_FILE "bmi260-init-data.fw" >> #define BMI270_INIT_DATA_FILE "bmi270-init-data.fw" >> >> enum bmi270_scan { >> @@ -67,6 +70,11 @@ enum bmi270_scan { >> }; >> >> const struct bmi270_chip_info bmi270_chip_info[] = { >> + [BMI260] = { >> + .name = "bmi260", >> + .chip_id = BMI260_CHIP_ID_VAL, >> + .fw_name = BMI260_INIT_DATA_FILE, >> + }, >> [BMI270] = { >> .name = "bmi270", >> .chip_id = BMI270_CHIP_ID_VAL, >> @@ -163,8 +171,21 @@ static int bmi270_validate_chip_id(struct bmi270_data *bmi270_device) >> if (ret) >> return dev_err_probe(dev, ret, "Failed to read chip id"); >> >> - if (chip_id != BMI270_CHIP_ID_VAL) >> - dev_info(dev, "Unknown chip id 0x%x", chip_id); >> + /* >> + * Some manufacturers use "BMI0160" for both the BMI160 and >> + * BMI260. If the device is actually a BMI160, the bmi160 >> + * driver should handle it and this driver should not. >> + */ >> + if (chip_id == BMI160_CHIP_ID_VAL) >> + return -ENODEV; This is where that BMI160 chip ID is checked. >> + >> + if (chip_id != bmi270_device->chip_info->chip_id) >> + dev_info(dev, "Unexpected chip id 0x%x", chip_id); >> + >> + if (chip_id == BMI260_CHIP_ID_VAL) > > Ah. My argument on separate IDs means you'd have to do it this way whereas > I was thinking maybe a loop would be a better idea. Ah well if we > get a lot of supported chips, then we can rethink how to handle this. > For now what you have here is fine and should deal with lack of appropriate > ACPI ID mess. I like the idea of separate structures, so I'll keep the if / else here. I think it would be straightforward to change later without conflicts if there are more supported chips. I will change this to check against bmi260_chip_info.chip_id, etc. instead of the constants, to make sure they stay consistent. Justin >> + bmi270_device->chip_info = &bmi270_chip_info[BMI260]; >> + else if (chip_id == BMI270_CHIP_ID_VAL) >> + bmi270_device->chip_info = &bmi270_chip_info[BMI270]; >> >> return 0; >> }
> >> #define BMI270_CHIP_ID_REG 0x00 > >> +#define BMI160_CHIP_ID_VAL 0xD1 > > > > This one looks like a cut and paste error. > > No, this was intentional -- I added the BMI160 chip ID here so it could > be checked later to avoid conflicting with the existing bmi160 driver. I > could add newlines before and after this group of _ID_VAL #defines if it > makes it clearer. Got it. Just add a comment that it's to exclude known bad firmwares and that is fine to keep.
diff --git a/drivers/iio/imu/bmi270/bmi270.h b/drivers/iio/imu/bmi270/bmi270.h index 2e8d85a4e419..51e374fd4290 100644 --- a/drivers/iio/imu/bmi270/bmi270.h +++ b/drivers/iio/imu/bmi270/bmi270.h @@ -14,6 +14,7 @@ struct bmi270_data { }; enum bmi270_device_type { + BMI260, BMI270, }; diff --git a/drivers/iio/imu/bmi270/bmi270_core.c b/drivers/iio/imu/bmi270/bmi270_core.c index 799df78ec862..b30201dc4e22 100644 --- a/drivers/iio/imu/bmi270/bmi270_core.c +++ b/drivers/iio/imu/bmi270/bmi270_core.c @@ -11,6 +11,8 @@ #include "bmi270.h" #define BMI270_CHIP_ID_REG 0x00 +#define BMI160_CHIP_ID_VAL 0xD1 +#define BMI260_CHIP_ID_VAL 0x27 #define BMI270_CHIP_ID_VAL 0x24 #define BMI270_CHIP_ID_MSK GENMASK(7, 0) @@ -55,6 +57,7 @@ #define BMI270_PWR_CTRL_ACCEL_EN_MSK BIT(2) #define BMI270_PWR_CTRL_TEMP_EN_MSK BIT(3) +#define BMI260_INIT_DATA_FILE "bmi260-init-data.fw" #define BMI270_INIT_DATA_FILE "bmi270-init-data.fw" enum bmi270_scan { @@ -67,6 +70,11 @@ enum bmi270_scan { }; const struct bmi270_chip_info bmi270_chip_info[] = { + [BMI260] = { + .name = "bmi260", + .chip_id = BMI260_CHIP_ID_VAL, + .fw_name = BMI260_INIT_DATA_FILE, + }, [BMI270] = { .name = "bmi270", .chip_id = BMI270_CHIP_ID_VAL, @@ -163,8 +171,21 @@ static int bmi270_validate_chip_id(struct bmi270_data *bmi270_device) if (ret) return dev_err_probe(dev, ret, "Failed to read chip id"); - if (chip_id != BMI270_CHIP_ID_VAL) - dev_info(dev, "Unknown chip id 0x%x", chip_id); + /* + * Some manufacturers use "BMI0160" for both the BMI160 and + * BMI260. If the device is actually a BMI160, the bmi160 + * driver should handle it and this driver should not. + */ + if (chip_id == BMI160_CHIP_ID_VAL) + return -ENODEV; + + if (chip_id != bmi270_device->chip_info->chip_id) + dev_info(dev, "Unexpected chip id 0x%x", chip_id); + + if (chip_id == BMI260_CHIP_ID_VAL) + bmi270_device->chip_info = &bmi270_chip_info[BMI260]; + else if (chip_id == BMI270_CHIP_ID_VAL) + bmi270_device->chip_info = &bmi270_chip_info[BMI270]; return 0; } diff --git a/drivers/iio/imu/bmi270/bmi270_i2c.c b/drivers/iio/imu/bmi270/bmi270_i2c.c index 742149701849..d6417b7c799c 100644 --- a/drivers/iio/imu/bmi270/bmi270_i2c.c +++ b/drivers/iio/imu/bmi270/bmi270_i2c.c @@ -33,11 +33,23 @@ static int bmi270_i2c_probe(struct i2c_client *client) } static const struct i2c_device_id bmi270_i2c_id[] = { + { "bmi260", (kernel_ulong_t)&bmi270_chip_info[BMI260] }, { "bmi270", (kernel_ulong_t)&bmi270_chip_info[BMI270] }, { } }; +static const struct acpi_device_id bmi270_acpi_match[] = { + /* OrangePi NEO */ + { "BMI0260", (kernel_ulong_t)&bmi270_chip_info[BMI260] }, + /* GPD Win Mini, Aya Neo AIR Pro, OXP Mini Pro, etc. */ + { "BMI0160", (kernel_ulong_t)&bmi270_chip_info[BMI260] }, + /* GPD Win Max 2 */ + { "10EC5280", (kernel_ulong_t)&bmi270_chip_info[BMI260] }, + { } +}; + static const struct of_device_id bmi270_of_match[] = { + { .compatible = "bosch,bmi260", .data = &bmi270_chip_info[BMI260] }, { .compatible = "bosch,bmi270", .data = &bmi270_chip_info[BMI270] }, { } }; @@ -45,6 +57,7 @@ static const struct of_device_id bmi270_of_match[] = { static struct i2c_driver bmi270_i2c_driver = { .driver = { .name = "bmi270_i2c", + .acpi_match_table = bmi270_acpi_match, .of_match_table = bmi270_of_match, }, .probe = bmi270_i2c_probe, diff --git a/drivers/iio/imu/bmi270/bmi270_spi.c b/drivers/iio/imu/bmi270/bmi270_spi.c index 3d240f9651bc..88173e9a3529 100644 --- a/drivers/iio/imu/bmi270/bmi270_spi.c +++ b/drivers/iio/imu/bmi270/bmi270_spi.c @@ -66,11 +66,18 @@ static int bmi270_spi_probe(struct spi_device *spi) } static const struct spi_device_id bmi270_spi_id[] = { + { "bmi260", (kernel_ulong_t)&bmi270_chip_info[BMI260] }, { "bmi270", (kernel_ulong_t)&bmi270_chip_info[BMI270] }, { } }; +static const struct acpi_device_id bmi270_acpi_match[] = { + { "BOSC0260", (kernel_ulong_t)&bmi270_chip_info[BMI260] }, + { } +}; + static const struct of_device_id bmi270_of_match[] = { + { .compatible = "bosch,bmi260", .data = &bmi270_chip_info[BMI260] }, { .compatible = "bosch,bmi270", .data = &bmi270_chip_info[BMI270] }, { } }; @@ -78,6 +85,7 @@ static const struct of_device_id bmi270_of_match[] = { static struct spi_driver bmi270_spi_driver = { .driver = { .name = "bmi270", + .acpi_match_table = bmi270_acpi_match, .of_match_table = bmi270_of_match, }, .probe = bmi270_spi_probe,
Adds support for the Bosch BMI260 6-axis IMU to the Bosch BMI270 driver. Setup and operation is nearly identical to the Bosch BMI270, but has a different chip ID and requires different firmware. Firmware is requested and loaded from userspace. Signed-off-by: Justin Weiss <justin@justinweiss.com> --- drivers/iio/imu/bmi270/bmi270.h | 1 + drivers/iio/imu/bmi270/bmi270_core.c | 25 +++++++++++++++++++++++-- drivers/iio/imu/bmi270/bmi270_i2c.c | 13 +++++++++++++ drivers/iio/imu/bmi270/bmi270_spi.c | 8 ++++++++ 4 files changed, 45 insertions(+), 2 deletions(-)