| Message ID | 20241011153751.65152-2-justin@justinweiss.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Add i2c driver for Bosch BMI260 IMU | expand |
On Fri, 11 Oct 2024 08:37:47 -0700 Justin Weiss <justin@justinweiss.com> wrote: Title needs an edit to reflect the description that follows. iio: imu: bmi270: Add i2c support for bmi260 > Adds i2c 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 | 16 ++++++++++++++- > drivers/iio/imu/bmi270/bmi270_core.c | 29 +++++++++++++++++++++++----- > drivers/iio/imu/bmi270/bmi270_i2c.c | 22 ++++++++++++++++++--- > drivers/iio/imu/bmi270/bmi270_spi.c | 11 ++++++++--- > 4 files changed, 66 insertions(+), 12 deletions(-) > > diff --git a/drivers/iio/imu/bmi270/bmi270.h b/drivers/iio/imu/bmi270/bmi270.h > index 8ac20ad7ee94..51e374fd4290 100644 > --- a/drivers/iio/imu/bmi270/bmi270.h > +++ b/drivers/iio/imu/bmi270/bmi270.h > @@ -10,10 +10,24 @@ struct device; > struct bmi270_data { > struct device *dev; > struct regmap *regmap; > + const struct bmi270_chip_info *chip_info; > +}; > + > +enum bmi270_device_type { > + BMI260, > + BMI270, > +}; It is obviously fairly trivial in this case, but the 'ideal' form for a patch series adding this flexibility is: Patch 1) Add a noop refactor to include the configuration structures etc. Patch 2) Add the support for the new device. First patch can then be reviewed on basis it's not destructive and second one just for the chip specific data added > diff --git a/drivers/iio/imu/bmi270/bmi270_core.c b/drivers/iio/imu/bmi270/bmi270_core.c > index aeda7c4228df..e5ee80c12166 100644 > --- a/drivers/iio/imu/bmi270/bmi270_core.c > +++ b/drivers/iio/imu/bmi270/bmi270_core.c > @@ -11,6 +11,7 @@ > static int bmi270_get_data(struct bmi270_data *bmi270_device, > int chan_type, int axis, int *val) > { > @@ -154,8 +170,8 @@ 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); > + if (chip_id != bmi270_device->chip_info->chip_id) If we have multiple known IDs it can be slightly more friendly to check them all and if another one matches, just moan about broken firmware before carrying on using the correct data. > + return dev_err_probe(dev, -ENODEV, "Unknown chip id 0x%x", chip_id); > > return 0; > } > @@ -187,7 +203,8 @@ static int bmi270_write_calibration_data(struct bmi270_data *bmi270_device) > return dev_err_probe(dev, ret, > "Failed to prepare device to load init data"); > > - ret = request_firmware(&init_data, BMI270_INIT_DATA_FILE, dev); > + ret = request_firmware(&init_data, > + bmi270_device->chip_info->fw_name, dev); > if (ret) > return dev_err_probe(dev, ret, "Failed to load init data file"); > > @@ -274,7 +291,8 @@ static int bmi270_chip_init(struct bmi270_data *bmi270_device) > return bmi270_configure_imu(bmi270_device); > } > diff --git a/drivers/iio/imu/bmi270/bmi270_i2c.c b/drivers/iio/imu/bmi270/bmi270_i2c.c > index e9025d22d5cc..c8c90666c76b 100644 > --- a/drivers/iio/imu/bmi270/bmi270_i2c.c > +++ b/drivers/iio/imu/bmi270/bmi270_i2c.c > @@ -18,28 +18,44 @@ static int bmi270_i2c_probe(struct i2c_client *client) > { > struct regmap *regmap; > struct device *dev = &client->dev; > + const struct bmi270_chip_info *chip_info; > + > + chip_info = i2c_get_match_data(client); > + if (!chip_info) > + return -ENODEV; > > 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"); > > - return bmi270_core_probe(dev, regmap); > + return bmi270_core_probe(dev, regmap, chip_info); > } > > static const struct i2c_device_id bmi270_i2c_id[] = { > - { "bmi270", 0 }, > + { "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] }, > + { "BMI0260", (kernel_ulong_t)&bmi270_chip_info[BMI260] }, > + { "BOSC0160", (kernel_ulong_t)&bmi270_chip_info[BMI260] }, > + { "BMI0160", (kernel_ulong_t)&bmi270_chip_info[BMI260] }, Sigh. That's not a valid ACPI ID or PNP ID. (Well technically it is, but it belongs to the Benson Instrument Company not Bosch) Which of these have been seen in the wild? For any that are not of the BOSC0160 type form add a comment giving a device on which they are in use. > + { "10EC5280", (kernel_ulong_t)&bmi270_chip_info[BMI260] }, What's this one? There is no such vendor ID. > + { }, No trailing comma on null terminators like this. > +}; > + > static const struct of_device_id bmi270_of_match[] = { > - { .compatible = "bosch,bmi270" }, > + { .compatible = "bosch,bmi270", .data = &bmi270_chip_info[BMI270] }, Add the 260 here as well + add to the dt docs. > { } > }; > > 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 34d5ba6273bb..3d240f9651bc 100644 > --- a/drivers/iio/imu/bmi270/bmi270_spi.c > +++ b/drivers/iio/imu/bmi270/bmi270_spi.c > @@ -50,6 +50,11 @@ static int bmi270_spi_probe(struct spi_device *spi) > { > struct regmap *regmap; > struct device *dev = &spi->dev; > + const struct bmi270_chip_info *chip_info; > + > + chip_info = spi_get_device_match_data(spi); > + if (!chip_info) > + return -ENODEV; > > regmap = devm_regmap_init(dev, &bmi270_regmap_bus, dev, > &bmi270_spi_regmap_config); > @@ -57,16 +62,16 @@ static int bmi270_spi_probe(struct spi_device *spi) > return dev_err_probe(dev, PTR_ERR(regmap), > "Failed to init i2c regmap"); > > - return bmi270_core_probe(dev, regmap); > + return bmi270_core_probe(dev, regmap, chip_info); > } > > static const struct spi_device_id bmi270_spi_id[] = { > - { "bmi270" }, > + { "bmi270", (kernel_ulong_t)&bmi270_chip_info[BMI270] }, > { } > }; > > static const struct of_device_id bmi270_of_match[] = { > - { .compatible = "bosch,bmi270" }, > + { .compatible = "bosch,bmi270", .data = &bmi270_chip_info[BMI270] }, If the bmi260 supports SPI, should be added here as well. (I've no idea if it does!) Or is this because you can't test it? > { } > }; >
Jonathan Cameron <jic23@kernel.org> writes: > On Fri, 11 Oct 2024 08:37:47 -0700 > Justin Weiss <justin@justinweiss.com> wrote: > > Title needs an edit to reflect the description that follows. > > iio: imu: bmi270: Add i2c support for bmi260 Thanks, I'll fix this in v2. >> Adds i2c 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 | 16 ++++++++++++++- >> drivers/iio/imu/bmi270/bmi270_core.c | 29 +++++++++++++++++++++++----- >> drivers/iio/imu/bmi270/bmi270_i2c.c | 22 ++++++++++++++++++--- >> drivers/iio/imu/bmi270/bmi270_spi.c | 11 ++++++++--- >> 4 files changed, 66 insertions(+), 12 deletions(-) >> >> diff --git a/drivers/iio/imu/bmi270/bmi270.h b/drivers/iio/imu/bmi270/bmi270.h >> index 8ac20ad7ee94..51e374fd4290 100644 >> --- a/drivers/iio/imu/bmi270/bmi270.h >> +++ b/drivers/iio/imu/bmi270/bmi270.h >> @@ -10,10 +10,24 @@ struct device; >> struct bmi270_data { >> struct device *dev; >> struct regmap *regmap; >> + const struct bmi270_chip_info *chip_info; >> +}; >> + >> +enum bmi270_device_type { >> + BMI260, >> + BMI270, >> +}; > It is obviously fairly trivial in this case, but the 'ideal' form for > a patch series adding this flexibility is: > Patch 1) Add a noop refactor to include the configuration structures etc. > Patch 2) Add the support for the new device. > > First patch can then be reviewed on basis it's not destructive and second one just for > the chip specific data added Makes sense, I'll split these up for v2. >> diff --git a/drivers/iio/imu/bmi270/bmi270_core.c b/drivers/iio/imu/bmi270/bmi270_core.c >> index aeda7c4228df..e5ee80c12166 100644 >> --- a/drivers/iio/imu/bmi270/bmi270_core.c >> +++ b/drivers/iio/imu/bmi270/bmi270_core.c >> @@ -11,6 +11,7 @@ > >> static int bmi270_get_data(struct bmi270_data *bmi270_device, >> int chan_type, int axis, int *val) >> { >> @@ -154,8 +170,8 @@ 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); >> + if (chip_id != bmi270_device->chip_info->chip_id) > If we have multiple known IDs it can be slightly more friendly to check them all > and if another one matches, just moan about broken firmware before carrying on > using the correct data. Sounds good. I'll dev_info a message if it doesn't match what the chip_info expects, and then switch to the chip_info corresponding to the chip ID returned by the device. > >> + return dev_err_probe(dev, -ENODEV, "Unknown chip id 0x%x", chip_id); >> >> return 0; >> } >> @@ -187,7 +203,8 @@ static int bmi270_write_calibration_data(struct bmi270_data *bmi270_device) >> return dev_err_probe(dev, ret, >> "Failed to prepare device to load init data"); >> >> - ret = request_firmware(&init_data, BMI270_INIT_DATA_FILE, dev); >> + ret = request_firmware(&init_data, >> + bmi270_device->chip_info->fw_name, dev); >> if (ret) >> return dev_err_probe(dev, ret, "Failed to load init data file"); >> >> @@ -274,7 +291,8 @@ static int bmi270_chip_init(struct bmi270_data *bmi270_device) >> return bmi270_configure_imu(bmi270_device); >> } > >> diff --git a/drivers/iio/imu/bmi270/bmi270_i2c.c b/drivers/iio/imu/bmi270/bmi270_i2c.c >> index e9025d22d5cc..c8c90666c76b 100644 >> --- a/drivers/iio/imu/bmi270/bmi270_i2c.c >> +++ b/drivers/iio/imu/bmi270/bmi270_i2c.c >> @@ -18,28 +18,44 @@ static int bmi270_i2c_probe(struct i2c_client *client) >> { >> struct regmap *regmap; >> struct device *dev = &client->dev; >> + const struct bmi270_chip_info *chip_info; >> + >> + chip_info = i2c_get_match_data(client); >> + if (!chip_info) >> + return -ENODEV; >> >> 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"); >> >> - return bmi270_core_probe(dev, regmap); >> + return bmi270_core_probe(dev, regmap, chip_info); >> } >> >> static const struct i2c_device_id bmi270_i2c_id[] = { >> - { "bmi270", 0 }, >> + { "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] }, >> + { "BMI0260", (kernel_ulong_t)&bmi270_chip_info[BMI260] }, >> + { "BOSC0160", (kernel_ulong_t)&bmi270_chip_info[BMI260] }, >> + { "BMI0160", (kernel_ulong_t)&bmi270_chip_info[BMI260] }, > > Sigh. That's not a valid ACPI ID or PNP ID. > (Well technically it is, but it belongs to the Benson Instrument Company > not Bosch) > > Which of these have been seen in the wild? > For any that are not of the BOSC0160 type form add a comment giving > a device on which they are in use. I know of the BMI0160 (this seems to be the most common way the BMI260 is identified on handheld PCs), and the 10EC5280 has been seen in the wild, as described here: https://lore.kernel.org/all/CAFqHKTm2WRNkcSoBEE=oNbfu_9d9RagQHLydmv6q1=snO_MXyA@mail.gmail.com/ I have not personally seen any devices using BMI0260, but I'll add comments to the BMI0160 and 10EC5280 entries with some examples of devices that use those IDs. >> + { "10EC5280", (kernel_ulong_t)&bmi270_chip_info[BMI260] }, > > What's this one? There is no such vendor ID. > >> + { }, > No trailing comma on null terminators like this. Thanks, will fix in v2. >> +}; >> + >> static const struct of_device_id bmi270_of_match[] = { >> - { .compatible = "bosch,bmi270" }, >> + { .compatible = "bosch,bmi270", .data = &bmi270_chip_info[BMI270] }, > > Add the 260 here as well + add to the dt docs. Will fix in v2. >> { } >> }; >> >> 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 34d5ba6273bb..3d240f9651bc 100644 >> --- a/drivers/iio/imu/bmi270/bmi270_spi.c >> +++ b/drivers/iio/imu/bmi270/bmi270_spi.c >> @@ -50,6 +50,11 @@ static int bmi270_spi_probe(struct spi_device *spi) >> { >> struct regmap *regmap; >> struct device *dev = &spi->dev; >> + const struct bmi270_chip_info *chip_info; >> + >> + chip_info = spi_get_device_match_data(spi); >> + if (!chip_info) >> + return -ENODEV; >> >> regmap = devm_regmap_init(dev, &bmi270_regmap_bus, dev, >> &bmi270_spi_regmap_config); >> @@ -57,16 +62,16 @@ static int bmi270_spi_probe(struct spi_device *spi) >> return dev_err_probe(dev, PTR_ERR(regmap), >> "Failed to init i2c regmap"); >> >> - return bmi270_core_probe(dev, regmap); >> + return bmi270_core_probe(dev, regmap, chip_info); >> } >> >> static const struct spi_device_id bmi270_spi_id[] = { >> - { "bmi270" }, >> + { "bmi270", (kernel_ulong_t)&bmi270_chip_info[BMI270] }, >> { } >> }; >> >> static const struct of_device_id bmi270_of_match[] = { >> - { .compatible = "bosch,bmi270" }, >> + { .compatible = "bosch,bmi270", .data = &bmi270_chip_info[BMI270] }, > > If the bmi260 supports SPI, should be added here as well. (I've no idea if it does!) > > Or is this because you can't test it? Yeah, it was because I can't test it, the BMI260 does support SPI. I can add entries here, though. Should the ACPI match entries from I2C also go here? All of the devices with mismatched IDs seem to use I2C so there might not be as much of a problem here. >> { } >> }; >> Thanks again, Justin
> >> +static const struct acpi_device_id bmi270_acpi_match[] = { > >> + { "BOSC0260", (kernel_ulong_t)&bmi270_chip_info[BMI260] }, > >> + { "BMI0260", (kernel_ulong_t)&bmi270_chip_info[BMI260] }, > >> + { "BOSC0160", (kernel_ulong_t)&bmi270_chip_info[BMI260] }, > >> + { "BMI0160", (kernel_ulong_t)&bmi270_chip_info[BMI260] }, > > > > Sigh. That's not a valid ACPI ID or PNP ID. > > (Well technically it is, but it belongs to the Benson Instrument Company > > not Bosch) > > > > Which of these have been seen in the wild? > > For any that are not of the BOSC0160 type form add a comment giving > > a device on which they are in use. > > I know of the BMI0160 (this seems to be the most common way the BMI260 > is identified on handheld PCs), and the 10EC5280 has been seen in the > wild, as described here: > https://lore.kernel.org/all/CAFqHKTm2WRNkcSoBEE=oNbfu_9d9RagQHLydmv6q1=snO_MXyA@mail.gmail.com/ > > I have not personally seen any devices using BMI0260, but I'll add > comments to the BMI0160 and 10EC5280 entries with some examples of > devices that use those IDs. Drop any we don't have evidence are out there. Do we have any confirmation from Bosch (or products in the wild) for the structurally correct BOSC0160 etc? Those would normally have to be tracked by Bosch as allocated for this purpose. > > >> + { "10EC5280", (kernel_ulong_t)&bmi270_chip_info[BMI260] }, > > > > What's this one? There is no such vendor ID. > > > ... > >> > >> static const struct of_device_id bmi270_of_match[] = { > >> - { .compatible = "bosch,bmi270" }, > >> + { .compatible = "bosch,bmi270", .data = &bmi270_chip_info[BMI270] }, > > > > If the bmi260 supports SPI, should be added here as well. (I've no idea if it does!) > > > > Or is this because you can't test it? > > Yeah, it was because I can't test it, the BMI260 does support SPI. I can > add entries here, though. > > Should the ACPI match entries from I2C also go here? All of the devices > with mismatched IDs seem to use I2C so there might not be as much of a > problem here. We want the incorrect formatted ones to be as hard to use as possible to discourage them going into new products. Can't do anything to solve the i2c cases but definitely don't want to allow them for SPI as well if no evidence of products where it yet matters. If we have confirmation from Bosch of the BOSC forms, then those I would like in the SPI drivers as well (to point to the correct option for anyone using this in future!) Jonathan > > >> { } > >> }; > >> > > Thanks again, > Justin
Jonathan Cameron <jic23@kernel.org> writes: >> >> +static const struct acpi_device_id bmi270_acpi_match[] = { >> >> + { "BOSC0260", (kernel_ulong_t)&bmi270_chip_info[BMI260] }, >> >> + { "BMI0260", (kernel_ulong_t)&bmi270_chip_info[BMI260] }, >> >> + { "BOSC0160", (kernel_ulong_t)&bmi270_chip_info[BMI260] }, >> >> + { "BMI0160", (kernel_ulong_t)&bmi270_chip_info[BMI260] }, >> > >> > Sigh. That's not a valid ACPI ID or PNP ID. >> > (Well technically it is, but it belongs to the Benson Instrument Company >> > not Bosch) >> > >> > Which of these have been seen in the wild? >> > For any that are not of the BOSC0160 type form add a comment giving >> > a device on which they are in use. >> >> I know of the BMI0160 (this seems to be the most common way the BMI260 >> is identified on handheld PCs), and the 10EC5280 has been seen in the >> wild, as described here: >> https://lore.kernel.org/all/CAFqHKTm2WRNkcSoBEE=oNbfu_9d9RagQHLydmv6q1=snO_MXyA@mail.gmail.com/ >> >> I have not personally seen any devices using BMI0260, but I'll add >> comments to the BMI0160 and 10EC5280 entries with some examples of >> devices that use those IDs. > > Drop any we don't have evidence are out there. > > Do we have any confirmation from Bosch (or products in the wild) for > the structurally correct BOSC0160 etc? Those would normally have > to be tracked by Bosch as allocated for this purpose. > I haven't seen any reported, but the Windows driver INF has all five of these entries listed. I don't see any evidence of the BOSC0160 or BOSC0260 being used other than that Windows driver file. BMI0160 seems by far the most common, with some appearances of 10EC5280 (some AYANEO devices, possibly some GPD Win Max 2 devices) and BMI0260 (OrangePi NEO). >> >> >> + { "10EC5280", (kernel_ulong_t)&bmi270_chip_info[BMI260] }, >> > >> > What's this one? There is no such vendor ID. >> > >> > ... > >> >> >> >> static const struct of_device_id bmi270_of_match[] = { >> >> - { .compatible = "bosch,bmi270" }, >> >> + { .compatible = "bosch,bmi270", .data = &bmi270_chip_info[BMI270] }, >> > >> > If the bmi260 supports SPI, should be added here as well. (I've no idea if it does!) >> > >> > Or is this because you can't test it? >> >> Yeah, it was because I can't test it, the BMI260 does support SPI. I can >> add entries here, though. >> >> Should the ACPI match entries from I2C also go here? All of the devices >> with mismatched IDs seem to use I2C so there might not be as much of a >> problem here. > We want the incorrect formatted ones to be as hard to use as possible to discourage > them going into new products. Can't do anything to solve the i2c cases > but definitely don't want to allow them for SPI as well if no evidence > of products where it yet matters. > > If we have confirmation from Bosch of the BOSC forms, then those I would like > in the SPI drivers as well (to point to the correct option for anyone using > this in future!) > > Jonathan > Agreed. Since we don't have confirmation of the correct values here or any that are in use, I would be OK either adding the single BOSC0260 entry (as a guess, which may or may not be used) or leaving it out entirely until an entry is needed. Justin >> >> >> { } >> >> }; >> >> >> >> Thanks again, >> Justin
On Sun, 13 Oct 2024 13:36:51 -0700 Justin Weiss <justin@justinweiss.com> wrote: > Jonathan Cameron <jic23@kernel.org> writes: > > >> >> +static const struct acpi_device_id bmi270_acpi_match[] = { > >> >> + { "BOSC0260", (kernel_ulong_t)&bmi270_chip_info[BMI260] }, > >> >> + { "BMI0260", (kernel_ulong_t)&bmi270_chip_info[BMI260] }, > >> >> + { "BOSC0160", (kernel_ulong_t)&bmi270_chip_info[BMI260] }, > >> >> + { "BMI0160", (kernel_ulong_t)&bmi270_chip_info[BMI260] }, > >> > > >> > Sigh. That's not a valid ACPI ID or PNP ID. > >> > (Well technically it is, but it belongs to the Benson Instrument Company > >> > not Bosch) > >> > > >> > Which of these have been seen in the wild? > >> > For any that are not of the BOSC0160 type form add a comment giving > >> > a device on which they are in use. > >> > >> I know of the BMI0160 (this seems to be the most common way the BMI260 > >> is identified on handheld PCs), and the 10EC5280 has been seen in the > >> wild, as described here: > >> https://lore.kernel.org/all/CAFqHKTm2WRNkcSoBEE=oNbfu_9d9RagQHLydmv6q1=snO_MXyA@mail.gmail.com/ > >> > >> I have not personally seen any devices using BMI0260, but I'll add > >> comments to the BMI0160 and 10EC5280 entries with some examples of > >> devices that use those IDs. > > > > Drop any we don't have evidence are out there. > > > > Do we have any confirmation from Bosch (or products in the wild) for > > the structurally correct BOSC0160 etc? Those would normally have > > to be tracked by Bosch as allocated for this purpose. > > > > I haven't seen any reported, but the Windows driver INF has all five of > these entries listed. I don't see any evidence of the BOSC0160 or > BOSC0260 being used other than that Windows driver file. Ok. Lets leave the extras out for now and see if anyone screams. > > BMI0160 seems by far the most common, with some appearances of 10EC5280 > (some AYANEO devices, possibly some GPD Win Max 2 devices) and BMI0260 > (OrangePi NEO). > > >> > >> >> + { "10EC5280", (kernel_ulong_t)&bmi270_chip_info[BMI260] }, > >> > > >> > What's this one? There is no such vendor ID. > >> > > >> > > ... > > > >> >> > >> >> static const struct of_device_id bmi270_of_match[] = { > >> >> - { .compatible = "bosch,bmi270" }, > >> >> + { .compatible = "bosch,bmi270", .data = &bmi270_chip_info[BMI270] }, > >> > > >> > If the bmi260 supports SPI, should be added here as well. (I've no idea if it does!) > >> > > >> > Or is this because you can't test it? > >> > >> Yeah, it was because I can't test it, the BMI260 does support SPI. I can > >> add entries here, though. > >> > >> Should the ACPI match entries from I2C also go here? All of the devices > >> with mismatched IDs seem to use I2C so there might not be as much of a > >> problem here. > > We want the incorrect formatted ones to be as hard to use as possible to discourage > > them going into new products. Can't do anything to solve the i2c cases > > but definitely don't want to allow them for SPI as well if no evidence > > of products where it yet matters. > > > > If we have confirmation from Bosch of the BOSC forms, then those I would like > > in the SPI drivers as well (to point to the correct option for anyone using > > this in future!) > > > > Jonathan > > > > Agreed. Since we don't have confirmation of the correct values here or > any that are in use, I would be OK either adding the single BOSC0260 > entry (as a guess, which may or may not be used) or leaving it out > entirely until an entry is needed. > Add that one as it might encourage anyone who happens to consider Linux support to do this right... If anyone has contacts at Bosch to moan at about their garbage use of ACPI IDs in windows drivers, please do. Jonathan > Justin > > >> > >> >> { } > >> >> }; > >> >> > >> > >> Thanks again, > >> Justin
diff --git a/drivers/iio/imu/bmi270/bmi270.h b/drivers/iio/imu/bmi270/bmi270.h index 8ac20ad7ee94..51e374fd4290 100644 --- a/drivers/iio/imu/bmi270/bmi270.h +++ b/drivers/iio/imu/bmi270/bmi270.h @@ -10,10 +10,24 @@ struct device; struct bmi270_data { struct device *dev; struct regmap *regmap; + const struct bmi270_chip_info *chip_info; +}; + +enum bmi270_device_type { + BMI260, + BMI270, +}; + +struct bmi270_chip_info { + const char *name; + int chip_id; + const char *fw_name; }; extern const struct regmap_config bmi270_regmap_config; +extern const struct bmi270_chip_info bmi270_chip_info[]; -int bmi270_core_probe(struct device *dev, struct regmap *regmap); +int bmi270_core_probe(struct device *dev, struct regmap *regmap, + const struct bmi270_chip_info *chip_info); #endif /* BMI270_H_ */ diff --git a/drivers/iio/imu/bmi270/bmi270_core.c b/drivers/iio/imu/bmi270/bmi270_core.c index aeda7c4228df..e5ee80c12166 100644 --- a/drivers/iio/imu/bmi270/bmi270_core.c +++ b/drivers/iio/imu/bmi270/bmi270_core.c @@ -11,6 +11,7 @@ #include "bmi270.h" #define BMI270_CHIP_ID_REG 0x00 +#define BMI260_CHIP_ID_VAL 0x27 #define BMI270_CHIP_ID_VAL 0x24 #define BMI270_CHIP_ID_MSK GENMASK(7, 0) @@ -55,6 +56,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 { @@ -66,6 +68,20 @@ enum bmi270_scan { BMI270_SCAN_GYRO_Z, }; +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, + .fw_name = BMI270_INIT_DATA_FILE, + } +}; +EXPORT_SYMBOL_NS_GPL(bmi270_chip_info, IIO_BMI270); + static int bmi270_get_data(struct bmi270_data *bmi270_device, int chan_type, int axis, int *val) { @@ -154,8 +170,8 @@ 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); + if (chip_id != bmi270_device->chip_info->chip_id) + return dev_err_probe(dev, -ENODEV, "Unknown chip id 0x%x", chip_id); return 0; } @@ -187,7 +203,8 @@ static int bmi270_write_calibration_data(struct bmi270_data *bmi270_device) return dev_err_probe(dev, ret, "Failed to prepare device to load init data"); - ret = request_firmware(&init_data, BMI270_INIT_DATA_FILE, dev); + ret = request_firmware(&init_data, + bmi270_device->chip_info->fw_name, dev); if (ret) return dev_err_probe(dev, ret, "Failed to load init data file"); @@ -274,7 +291,8 @@ static int bmi270_chip_init(struct bmi270_data *bmi270_device) return bmi270_configure_imu(bmi270_device); } -int bmi270_core_probe(struct device *dev, struct regmap *regmap) +int bmi270_core_probe(struct device *dev, struct regmap *regmap, + const struct bmi270_chip_info *chip_info) { int ret; struct bmi270_data *bmi270_device; @@ -287,6 +305,7 @@ int bmi270_core_probe(struct device *dev, struct regmap *regmap) bmi270_device = iio_priv(indio_dev); bmi270_device->dev = dev; bmi270_device->regmap = regmap; + bmi270_device->chip_info = chip_info; ret = bmi270_chip_init(bmi270_device); if (ret) @@ -294,7 +313,7 @@ int bmi270_core_probe(struct device *dev, struct regmap *regmap) indio_dev->channels = bmi270_channels; indio_dev->num_channels = ARRAY_SIZE(bmi270_channels); - indio_dev->name = "bmi270"; + indio_dev->name = chip_info->name; indio_dev->modes = INDIO_DIRECT_MODE; indio_dev->info = &bmi270_info; diff --git a/drivers/iio/imu/bmi270/bmi270_i2c.c b/drivers/iio/imu/bmi270/bmi270_i2c.c index e9025d22d5cc..c8c90666c76b 100644 --- a/drivers/iio/imu/bmi270/bmi270_i2c.c +++ b/drivers/iio/imu/bmi270/bmi270_i2c.c @@ -18,28 +18,44 @@ static int bmi270_i2c_probe(struct i2c_client *client) { struct regmap *regmap; struct device *dev = &client->dev; + const struct bmi270_chip_info *chip_info; + + chip_info = i2c_get_match_data(client); + if (!chip_info) + return -ENODEV; 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"); - return bmi270_core_probe(dev, regmap); + return bmi270_core_probe(dev, regmap, chip_info); } static const struct i2c_device_id bmi270_i2c_id[] = { - { "bmi270", 0 }, + { "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] }, + { "BMI0260", (kernel_ulong_t)&bmi270_chip_info[BMI260] }, + { "BOSC0160", (kernel_ulong_t)&bmi270_chip_info[BMI260] }, + { "BMI0160", (kernel_ulong_t)&bmi270_chip_info[BMI260] }, + { "10EC5280", (kernel_ulong_t)&bmi270_chip_info[BMI260] }, + { }, +}; + static const struct of_device_id bmi270_of_match[] = { - { .compatible = "bosch,bmi270" }, + { .compatible = "bosch,bmi270", .data = &bmi270_chip_info[BMI270] }, { } }; 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 34d5ba6273bb..3d240f9651bc 100644 --- a/drivers/iio/imu/bmi270/bmi270_spi.c +++ b/drivers/iio/imu/bmi270/bmi270_spi.c @@ -50,6 +50,11 @@ static int bmi270_spi_probe(struct spi_device *spi) { struct regmap *regmap; struct device *dev = &spi->dev; + const struct bmi270_chip_info *chip_info; + + chip_info = spi_get_device_match_data(spi); + if (!chip_info) + return -ENODEV; regmap = devm_regmap_init(dev, &bmi270_regmap_bus, dev, &bmi270_spi_regmap_config); @@ -57,16 +62,16 @@ static int bmi270_spi_probe(struct spi_device *spi) return dev_err_probe(dev, PTR_ERR(regmap), "Failed to init i2c regmap"); - return bmi270_core_probe(dev, regmap); + return bmi270_core_probe(dev, regmap, chip_info); } static const struct spi_device_id bmi270_spi_id[] = { - { "bmi270" }, + { "bmi270", (kernel_ulong_t)&bmi270_chip_info[BMI270] }, { } }; static const struct of_device_id bmi270_of_match[] = { - { .compatible = "bosch,bmi270" }, + { .compatible = "bosch,bmi270", .data = &bmi270_chip_info[BMI270] }, { } };
Adds i2c 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 | 16 ++++++++++++++- drivers/iio/imu/bmi270/bmi270_core.c | 29 +++++++++++++++++++++++----- drivers/iio/imu/bmi270/bmi270_i2c.c | 22 ++++++++++++++++++--- drivers/iio/imu/bmi270/bmi270_spi.c | 11 ++++++++--- 4 files changed, 66 insertions(+), 12 deletions(-)