Re: [PATCH v2 1/2] iio: imu: bmi160: add support for bmi120

[Jonathan Cameron <jic23@kernel.org>]

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)  
> 
> ...