| Message ID | 20240504-bmi120-v1-1-478470a85058@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Add support for bosch bmi120 | expand |
On Sat, 04 May 2024 01:45:24 +0200 Barnabás Czémán <trabarni@gmail.com> wrote: > From: Danila Tikhonov <danila@jiaxyga.com> > > Add support for bmi120 low power variant of bmi160. > > Signed-off-by: Danila Tikhonov <danila@jiaxyga.com> > Co-developed-by: Barnabás Czémán <trabarni@gmail.com> > Signed-off-by: Barnabás Czémán <trabarni@gmail.com> Hi. A comment inline that is more about us doing things wrong in the past than the changes you are making. However, I would like to fix that up whilst we are here. Thanks, Jonathan > --- > drivers/iio/imu/bmi160/bmi160_core.c | 24 ++++++++++++++++++++---- > drivers/iio/imu/bmi160/bmi160_i2c.c | 3 +++ > drivers/iio/imu/bmi160/bmi160_spi.c | 3 +++ > 3 files changed, 26 insertions(+), 4 deletions(-) > > diff --git a/drivers/iio/imu/bmi160/bmi160_core.c b/drivers/iio/imu/bmi160/bmi160_core.c > index a77f1a8348ff..015801ad4d9a 100644 > --- a/drivers/iio/imu/bmi160/bmi160_core.c > +++ b/drivers/iio/imu/bmi160/bmi160_core.c > @@ -26,6 +26,7 @@ > #include "bmi160.h" > > #define BMI160_REG_CHIP_ID 0x00 > +#define BMI120_CHIP_ID_VAL 0xD3 > #define BMI160_CHIP_ID_VAL 0xD1 > > #define BMI160_REG_PMU_STATUS 0x03 > @@ -112,6 +113,11 @@ > .ext_info = bmi160_ext_info, \ > } > > +const u8 bmi_chip_ids[] = { > + BMI120_CHIP_ID_VAL, > + BMI160_CHIP_ID_VAL, > +}; > + > /* scan indexes follow DATA register order */ > enum bmi160_scan_axis { > BMI160_SCAN_EXT_MAGN_X = 0, > @@ -704,6 +710,16 @@ static int bmi160_setup_irq(struct iio_dev *indio_dev, int irq, > return bmi160_probe_trigger(indio_dev, irq, irq_type); > } > > +static int bmi160_check_chip_id(const u8 chip_id) > +{ > + for (int i = 0; i < ARRAY_SIZE(bmi_chip_ids); i++) { > + if (chip_id == bmi_chip_ids[i]) > + return 0; This falls into a miss design issue on the original driver based on my understanding of what we should do with chip IDs back then. My current thinking after discussing with DT maintainers is we should at most print a warning if we fail to match an ID. The intent being to allow for cases exactly like this one to work with an older kernel if a fallback compatible is in use. So whilst you are here, please could you relax the below error path to just do a dev_warn() or similar and return 0 anyway. Or better yet, push that warning print to the caller... > + } > + > + return -ENODEV; > +} > + > static int bmi160_chip_init(struct bmi160_data *data, bool use_spi) > { > int ret; > @@ -737,10 +753,10 @@ static int bmi160_chip_init(struct bmi160_data *data, bool use_spi) > dev_err(dev, "Error reading chip id\n"); > goto disable_regulator; > } > - if (val != BMI160_CHIP_ID_VAL) { > - dev_err(dev, "Wrong chip id, got %x expected %x\n", > - val, BMI160_CHIP_ID_VAL); > - ret = -ENODEV; > + > + ret = bmi160_check_chip_id(val); > + if (ret) { > + dev_err(dev, "Wrong chip id %x\n", val); > goto disable_regulator; For reasons given above, whilst we are hear I'd like this to change to dev_warn() and carry on as if we did have a match. > } > > diff --git a/drivers/iio/imu/bmi160/bmi160_i2c.c b/drivers/iio/imu/bmi160/bmi160_i2c.c > index a081305254db..d0ec5301ad9a 100644 > --- a/drivers/iio/imu/bmi160/bmi160_i2c.c > +++ b/drivers/iio/imu/bmi160/bmi160_i2c.c > @@ -37,6 +37,7 @@ static int bmi160_i2c_probe(struct i2c_client *client) > } > > static const struct i2c_device_id bmi160_i2c_id[] = { > + {"bmi120", 0}, > {"bmi160", 0}, > {} > }; > @@ -52,12 +53,14 @@ static const struct acpi_device_id bmi160_acpi_match[] = { > * the affected devices are from 2021/2022. > */ > {"10EC5280", 0}, > + {"BMI0120", 0}, > {"BMI0160", 0}, > { }, > }; > MODULE_DEVICE_TABLE(acpi, bmi160_acpi_match); > > static const struct of_device_id bmi160_of_match[] = { > + { .compatible = "bosch,bmi120" }, > { .compatible = "bosch,bmi160" }, > { }, > }; > diff --git a/drivers/iio/imu/bmi160/bmi160_spi.c b/drivers/iio/imu/bmi160/bmi160_spi.c > index 8b573ea99af2..9f40500132f7 100644 > --- a/drivers/iio/imu/bmi160/bmi160_spi.c > +++ b/drivers/iio/imu/bmi160/bmi160_spi.c > @@ -34,18 +34,21 @@ static int bmi160_spi_probe(struct spi_device *spi) > } > > static const struct spi_device_id bmi160_spi_id[] = { > + {"bmi120", 0}, > {"bmi160", 0}, > {} > }; > MODULE_DEVICE_TABLE(spi, bmi160_spi_id); > > static const struct acpi_device_id bmi160_acpi_match[] = { > + {"BMI0120", 0}, > {"BMI0160", 0}, > { }, > }; > MODULE_DEVICE_TABLE(acpi, bmi160_acpi_match); > > static const struct of_device_id bmi160_of_match[] = { > + { .compatible = "bosch,bmi120" }, > { .compatible = "bosch,bmi160" }, > { }, > }; >
Hi Barnabás, kernel test robot noticed the following build warnings: [auto build test WARNING on 9221b2819b8a4196eecf5476d66201be60fbcf29] url: https://github.com/intel-lab-lkp/linux/commits/Barnab-s-Cz-m-n/iio-imu-bmi160-add-support-for-bmi120/20240504-074802 base: 9221b2819b8a4196eecf5476d66201be60fbcf29 patch link: https://lore.kernel.org/r/20240504-bmi120-v1-1-478470a85058%40gmail.com patch subject: [PATCH 1/2] iio: imu: bmi160: add support for bmi120 config: x86_64-randconfig-121-20240505 (https://download.01.org/0day-ci/archive/20240505/202405051259.R7WyR8LV-lkp@intel.com/config) compiler: clang version 18.1.4 (https://github.com/llvm/llvm-project e6c3289804a67ea0bb6a86fadbe454dd93b8d855) reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240505/202405051259.R7WyR8LV-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202405051259.R7WyR8LV-lkp@intel.com/ sparse warnings: (new ones prefixed by >>) >> drivers/iio/imu/bmi160/bmi160_core.c:116:10: sparse: sparse: symbol 'bmi_chip_ids' was not declared. Should it be static? vim +/bmi_chip_ids +116 drivers/iio/imu/bmi160/bmi160_core.c 115 > 116 const u8 bmi_chip_ids[] = { 117 BMI120_CHIP_ID_VAL, 118 BMI160_CHIP_ID_VAL, 119 }; 120
diff --git a/drivers/iio/imu/bmi160/bmi160_core.c b/drivers/iio/imu/bmi160/bmi160_core.c index a77f1a8348ff..015801ad4d9a 100644 --- a/drivers/iio/imu/bmi160/bmi160_core.c +++ b/drivers/iio/imu/bmi160/bmi160_core.c @@ -26,6 +26,7 @@ #include "bmi160.h" #define BMI160_REG_CHIP_ID 0x00 +#define BMI120_CHIP_ID_VAL 0xD3 #define BMI160_CHIP_ID_VAL 0xD1 #define BMI160_REG_PMU_STATUS 0x03 @@ -112,6 +113,11 @@ .ext_info = bmi160_ext_info, \ } +const u8 bmi_chip_ids[] = { + BMI120_CHIP_ID_VAL, + BMI160_CHIP_ID_VAL, +}; + /* scan indexes follow DATA register order */ enum bmi160_scan_axis { BMI160_SCAN_EXT_MAGN_X = 0, @@ -704,6 +710,16 @@ static int bmi160_setup_irq(struct iio_dev *indio_dev, int irq, return bmi160_probe_trigger(indio_dev, irq, irq_type); } +static int bmi160_check_chip_id(const u8 chip_id) +{ + for (int i = 0; i < ARRAY_SIZE(bmi_chip_ids); i++) { + if (chip_id == bmi_chip_ids[i]) + return 0; + } + + return -ENODEV; +} + static int bmi160_chip_init(struct bmi160_data *data, bool use_spi) { int ret; @@ -737,10 +753,10 @@ static int bmi160_chip_init(struct bmi160_data *data, bool use_spi) dev_err(dev, "Error reading chip id\n"); goto disable_regulator; } - if (val != BMI160_CHIP_ID_VAL) { - dev_err(dev, "Wrong chip id, got %x expected %x\n", - val, BMI160_CHIP_ID_VAL); - ret = -ENODEV; + + ret = bmi160_check_chip_id(val); + if (ret) { + dev_err(dev, "Wrong chip id %x\n", val); goto disable_regulator; } diff --git a/drivers/iio/imu/bmi160/bmi160_i2c.c b/drivers/iio/imu/bmi160/bmi160_i2c.c index a081305254db..d0ec5301ad9a 100644 --- a/drivers/iio/imu/bmi160/bmi160_i2c.c +++ b/drivers/iio/imu/bmi160/bmi160_i2c.c @@ -37,6 +37,7 @@ static int bmi160_i2c_probe(struct i2c_client *client) } static const struct i2c_device_id bmi160_i2c_id[] = { + {"bmi120", 0}, {"bmi160", 0}, {} }; @@ -52,12 +53,14 @@ static const struct acpi_device_id bmi160_acpi_match[] = { * the affected devices are from 2021/2022. */ {"10EC5280", 0}, + {"BMI0120", 0}, {"BMI0160", 0}, { }, }; MODULE_DEVICE_TABLE(acpi, bmi160_acpi_match); static const struct of_device_id bmi160_of_match[] = { + { .compatible = "bosch,bmi120" }, { .compatible = "bosch,bmi160" }, { }, }; diff --git a/drivers/iio/imu/bmi160/bmi160_spi.c b/drivers/iio/imu/bmi160/bmi160_spi.c index 8b573ea99af2..9f40500132f7 100644 --- a/drivers/iio/imu/bmi160/bmi160_spi.c +++ b/drivers/iio/imu/bmi160/bmi160_spi.c @@ -34,18 +34,21 @@ static int bmi160_spi_probe(struct spi_device *spi) } static const struct spi_device_id bmi160_spi_id[] = { + {"bmi120", 0}, {"bmi160", 0}, {} }; MODULE_DEVICE_TABLE(spi, bmi160_spi_id); static const struct acpi_device_id bmi160_acpi_match[] = { + {"BMI0120", 0}, {"BMI0160", 0}, { }, }; MODULE_DEVICE_TABLE(acpi, bmi160_acpi_match); static const struct of_device_id bmi160_of_match[] = { + { .compatible = "bosch,bmi120" }, { .compatible = "bosch,bmi160" }, { }, };