On Fri, 18 Oct 2024 16:36:10 -0700 Justin Weiss <justin@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx> 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; > }