| Message ID | 20241018233723.28757-3-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:08 -0700 Justin Weiss <justin@justinweiss.com> wrote: > Prepare the bmi270 driver to support similar devices like the bmi260. > > Signed-off-by: Justin Weiss <justin@justinweiss.com> One thing in here. The enum ID thing tends to end up costing more than the benefit it brings, so for newer drivers preferred option is separate structure instances rather than an array. > --- > drivers/iio/imu/bmi270/bmi270.h | 15 ++++++++++++++- > drivers/iio/imu/bmi270/bmi270_core.c | 18 +++++++++++++++--- > drivers/iio/imu/bmi270/bmi270_i2c.c | 11 ++++++++--- > drivers/iio/imu/bmi270/bmi270_spi.c | 11 ++++++++--- > 4 files changed, 45 insertions(+), 10 deletions(-) > > diff --git a/drivers/iio/imu/bmi270/bmi270.h b/drivers/iio/imu/bmi270/bmi270.h > index 8ac20ad7ee94..2e8d85a4e419 100644 > --- a/drivers/iio/imu/bmi270/bmi270.h > +++ b/drivers/iio/imu/bmi270/bmi270.h > @@ -10,10 +10,23 @@ struct device; > struct bmi270_data { > struct device *dev; > struct regmap *regmap; > + const struct bmi270_chip_info *chip_info; > +}; > + > +enum bmi270_device_type { > + BMI270, Whilst quite a few drivers do it this way, over time we've found that it's much easier to just skip the array of structures and have independent ones. Increase the extern lines to one per supported device, but removes need for an enum here and generally gives slightly more readable code. > +}; > }; > > static const struct of_device_id bmi270_of_match[] = { > - { .compatible = "bosch,bmi270" }, > + { .compatible = "bosch,bmi270", .data = &bmi270_chip_info[BMI270] }, After dropping the enum this just becomes &bmi270_chip_info and later you'll add bmi260_chip_info etc. > { } > }; >
Jonathan Cameron <jic23@kernel.org> writes: > On Fri, 18 Oct 2024 16:36:08 -0700 > Justin Weiss <justin@justinweiss.com> wrote: > >> Prepare the bmi270 driver to support similar devices like the bmi260. >> >> Signed-off-by: Justin Weiss <justin@justinweiss.com> > One thing in here. The enum ID thing tends to end up costing more than > the benefit it brings, so for newer drivers preferred option is separate > structure instances rather than an array. That makes sense to me, even considering your comments on patch #4. I'll switch to separate structures here and keep the if / else in that later patch. Justin >> --- >> drivers/iio/imu/bmi270/bmi270.h | 15 ++++++++++++++- >> drivers/iio/imu/bmi270/bmi270_core.c | 18 +++++++++++++++--- >> drivers/iio/imu/bmi270/bmi270_i2c.c | 11 ++++++++--- >> drivers/iio/imu/bmi270/bmi270_spi.c | 11 ++++++++--- >> 4 files changed, 45 insertions(+), 10 deletions(-) >> >> diff --git a/drivers/iio/imu/bmi270/bmi270.h b/drivers/iio/imu/bmi270/bmi270.h >> index 8ac20ad7ee94..2e8d85a4e419 100644 >> --- a/drivers/iio/imu/bmi270/bmi270.h >> +++ b/drivers/iio/imu/bmi270/bmi270.h >> @@ -10,10 +10,23 @@ struct device; >> struct bmi270_data { >> struct device *dev; >> struct regmap *regmap; >> + const struct bmi270_chip_info *chip_info; >> +}; >> + >> +enum bmi270_device_type { >> + BMI270, > > Whilst quite a few drivers do it this way, over time we've found that it's > much easier to just skip the array of structures and have independent ones. > Increase the extern lines to one per supported device, but removes > need for an enum here and generally gives slightly more readable code. > > >> +}; > >> }; >> >> static const struct of_device_id bmi270_of_match[] = { >> - { .compatible = "bosch,bmi270" }, >> + { .compatible = "bosch,bmi270", .data = &bmi270_chip_info[BMI270] }, > > After dropping the enum this just becomes &bmi270_chip_info > and later you'll add bmi260_chip_info etc. > >> { } >> }; >>
diff --git a/drivers/iio/imu/bmi270/bmi270.h b/drivers/iio/imu/bmi270/bmi270.h index 8ac20ad7ee94..2e8d85a4e419 100644 --- a/drivers/iio/imu/bmi270/bmi270.h +++ b/drivers/iio/imu/bmi270/bmi270.h @@ -10,10 +10,23 @@ struct device; struct bmi270_data { struct device *dev; struct regmap *regmap; + const struct bmi270_chip_info *chip_info; +}; + +enum bmi270_device_type { + 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 87036f352698..799df78ec862 100644 --- a/drivers/iio/imu/bmi270/bmi270_core.c +++ b/drivers/iio/imu/bmi270/bmi270_core.c @@ -66,6 +66,15 @@ enum bmi270_scan { BMI270_SCAN_GYRO_Z, }; +const struct bmi270_chip_info bmi270_chip_info[] = { + [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) { @@ -187,7 +196,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 +284,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 +298,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 +306,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..742149701849 100644 --- a/drivers/iio/imu/bmi270/bmi270_i2c.c +++ b/drivers/iio/imu/bmi270/bmi270_i2c.c @@ -18,22 +18,27 @@ 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 }, + { "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] }, { } }; 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] }, { } };
Prepare the bmi270 driver to support similar devices like the bmi260. Signed-off-by: Justin Weiss <justin@justinweiss.com> --- drivers/iio/imu/bmi270/bmi270.h | 15 ++++++++++++++- drivers/iio/imu/bmi270/bmi270_core.c | 18 +++++++++++++++--- drivers/iio/imu/bmi270/bmi270_i2c.c | 11 ++++++++--- drivers/iio/imu/bmi270/bmi270_spi.c | 11 ++++++++--- 4 files changed, 45 insertions(+), 10 deletions(-)