| Message ID | 20240504-bmi120-v2-0-3b3ce6e1c3c6@gmail.com |
|---|---|
| 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 Sat, May 4, 2024 at 8:00 AM Barnabás Czémán <trabarni@gmail.com> wrote: > > Add support for bosch bmi120. > BMI120 is an energy-efficient version of BMI160. Despite having a different > CHIPID value, this variant seems to be fully compatible with BMI160. > It could be find in many phones like xiaomi-vince or xiaomi-tissot. > > Signed-off-by: Barnabás Czémán <trabarni@gmail.com> > --- > Changes in v2: > - Add bosch,bmi120 as a fallback compatible. > - Remove error path if chipid is not found. > - Link to v1: https://lore.kernel.org/r/20240504-bmi120-v1-0-478470a85058@gmail.com > > --- > Danila Tikhonov (2): > iio: imu: bmi160: add support for bmi120 > dt-bindings: iio: imu: bmi160: add bmi120 > Preferably, the DT bindings patch should go first in the series before the code that use it (makes it easier for reviewers to read it in right order).
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) > > > > ... > >
Add support for bosch bmi120. BMI120 is an energy-efficient version of BMI160. Despite having a different CHIPID value, this variant seems to be fully compatible with BMI160. It could be find in many phones like xiaomi-vince or xiaomi-tissot. Signed-off-by: Barnabás Czémán <trabarni@gmail.com> --- Changes in v2: - Add bosch,bmi120 as a fallback compatible. - Remove error path if chipid is not found. - Link to v1: https://lore.kernel.org/r/20240504-bmi120-v1-0-478470a85058@gmail.com --- Danila Tikhonov (2): iio: imu: bmi160: add support for bmi120 dt-bindings: iio: imu: bmi160: add bmi120 .../devicetree/bindings/iio/imu/bosch,bmi160.yaml | 6 ++++- drivers/iio/imu/bmi160/bmi160_core.c | 26 +++++++++++++++++----- drivers/iio/imu/bmi160/bmi160_i2c.c | 3 +++ drivers/iio/imu/bmi160/bmi160_spi.c | 3 +++ 4 files changed, 31 insertions(+), 7 deletions(-) --- base-commit: 9221b2819b8a4196eecf5476d66201be60fbcf29 change-id: 20240504-bmi120-d3c88dcb3073 Best regards,