lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Sat, 11 May 2024 12:54:36 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: David Lechner <dlechner@...libre.com>
Cc: Barnabás Czémán <trabarni@...il.com>, Lars-Peter
 Clausen <lars@...afoo.de>, Rob Herring <robh@...nel.org>, Krzysztof
 Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>,
 linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org,
 devicetree@...r.kernel.org, Danila Tikhonov <danila@...xyga.com>
Subject: Re: [PATCH v2 1/2] iio: imu: bmi160: add support for bmi120

On Tue, 7 May 2024 17:05:18 -0500
David Lechner <dlechner@...libre.com> wrote:

> On Sat, May 4, 2024 at 8:01 AM Barnabás Czémán <trabarni@...il.com> wrote:
> >
> > From: Danila Tikhonov <danila@...xyga.com>
> >
> > Add support for bmi120 low power variant of bmi160.
> >
> > Signed-off-by: Danila Tikhonov <danila@...xyga.com>
> > Co-developed-by: Barnabás Czémán <trabarni@...il.com>
> > Signed-off-by: Barnabás Czémán <trabarni@...il.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)  
> 
> ...


Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ