| Message ID | 20240504-bmi120-v2-1-3b3ce6e1c3c6@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add support for bosch bmi120 | expand |
On Sat, May 4, 2024 at 8:01 AM 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> > --- > drivers/iio/imu/bmi160/bmi160_core.c | 26 ++++++++++++++++++++------ > drivers/iio/imu/bmi160/bmi160_i2c.c | 3 +++ > drivers/iio/imu/bmi160/bmi160_spi.c | 3 +++ > 3 files changed, 26 insertions(+), 6 deletions(-) > > diff --git a/drivers/iio/imu/bmi160/bmi160_core.c b/drivers/iio/imu/bmi160/bmi160_core.c > index a77f1a8348ff..468aa80318fc 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; It looks like this will match either chip to either ID. If we do this, then why check the ID at all? Another approach could be to put the chip ID as the match data in bmi160_*_match, then you would get the right ID based on the compatible string. > + } > + > + return -ENODEV; > +} > + > static int bmi160_chip_init(struct bmi160_data *data, bool use_spi) > { > int ret; > @@ -737,12 +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; > - goto disable_regulator; > - } > + > + ret = bmi160_check_chip_id(val); > + if (ret) > + dev_warn(dev, "Chip id not found: %x\n", val); This changes the error with probe failure to a warning, but the commit message doesn't explain why. We always want to know why changes were made. :-) Should also probably be in a separate patch since changing the behavior here is a separate change from adding support for a new chip. > > ret = bmi160_set_mode(data, BMI160_ACCEL, true); > if (ret) ...
On Tue, 7 May 2024 17:05:18 -0500 David Lechner <dlechner@baylibre.com> wrote: > On Sat, May 4, 2024 at 8:01 AM 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> > > --- > > drivers/iio/imu/bmi160/bmi160_core.c | 26 ++++++++++++++++++++------ > > drivers/iio/imu/bmi160/bmi160_i2c.c | 3 +++ > > drivers/iio/imu/bmi160/bmi160_spi.c | 3 +++ > > 3 files changed, 26 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/iio/imu/bmi160/bmi160_core.c b/drivers/iio/imu/bmi160/bmi160_core.c > > index a77f1a8348ff..468aa80318fc 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; > > It looks like this will match either chip to either ID. If we do this, > then why check the ID at all? > > Another approach could be to put the chip ID as the match data in > bmi160_*_match, then you would get the right ID based on the > compatible string. It is useful as a sanity check to at least hint to the user that we either recognise the device or not. It's annoyingly common for vendors to switch out a chip for one where the vendor driver reads the ID from the device and deals with completely incompatible parts. They do this without updating the firmware. In one or two cases we've had to wrap multiple Linux drivers up to paper over this garbage. (It seems to be more common for ACPI tables, where we can push that as a platform quirk :) If we end up with this driver supporting slightly incompatible variants then adding that info to the ID table is useful because then, if we fail to match ID here (because someone is using a fallback compatible) we can pick the device that their firmware is claiming the replacement is backwards compatible with. For now, just warning here on no match and carrying on is the right approach. > > > + } > > + > > + return -ENODEV; > > +} > > + > > static int bmi160_chip_init(struct bmi160_data *data, bool use_spi) > > { > > int ret; > > @@ -737,12 +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; > > - goto disable_regulator; > > - } > > + > > + ret = bmi160_check_chip_id(val); > > + if (ret) > > + dev_warn(dev, "Chip id not found: %x\n", val); > > This changes the error with probe failure to a warning, but the commit > message doesn't explain why. We always want to know why changes were > made. :-) > > Should also probably be in a separate patch since changing the > behavior here is a separate change from adding support for a new chip. True, separate patch would be ideal as maybe someone will backport this change and not the rest. > > > > > ret = bmi160_set_mode(data, BMI160_ACCEL, true); > > if (ret) > > ...
> > > > > + } > > > + > > > + return -ENODEV; > > > +} > > > + > > > static int bmi160_chip_init(struct bmi160_data *data, bool use_spi) > > > { > > > int ret; > > > @@ -737,12 +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; > > > - goto disable_regulator; > > > - } > > > + > > > + ret = bmi160_check_chip_id(val); > > > + if (ret) > > > + dev_warn(dev, "Chip id not found: %x\n", val); > > > > This changes the error with probe failure to a warning, but the commit > > message doesn't explain why. We always want to know why changes were > > made. :-) > > > > Should also probably be in a separate patch since changing the > > behavior here is a separate change from adding support for a new chip. > True, separate patch would be ideal as maybe someone will backport this change and > not the rest. Given I'd already picked up v3, I added a note on this to the commit rather than splitting it. I doubt anyone will care about dragging in bmi120 IDs along with the relaxation of matching if they just want the relaxation. Jonathan > > > > > > > > ret = bmi160_set_mode(data, BMI160_ACCEL, true); > > > if (ret) > > > > ... > >
diff --git a/drivers/iio/imu/bmi160/bmi160_core.c b/drivers/iio/imu/bmi160/bmi160_core.c index a77f1a8348ff..468aa80318fc 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,12 +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; - goto disable_regulator; - } + + ret = bmi160_check_chip_id(val); + if (ret) + dev_warn(dev, "Chip id not found: %x\n", val); ret = bmi160_set_mode(data, BMI160_ACCEL, true); if (ret) 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" }, { }, };