[PATCH v2 0/2] Add support for bosch bmi120

[PATCH v2 0/2] Add support for bosch bmi120

[Barnabás Czémán]

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,
-- 
Barnabás Czémán <trabarni@gmail.com>

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

[Barnabás Czémán]

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;
+	}
+
+	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" },
 	{ },
 };

-- 
2.45.0

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

[David Lechner]

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)

...

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

[Jonathan Cameron]

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

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

[Jonathan Cameron]

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

[PATCH v2 2/2] dt-bindings: iio: imu: bmi160: add bmi120

[Barnabás Czémán]

From: Danila Tikhonov <danila@jiaxyga.com>

Document bosch,bmi120 compatible.

Signed-off-by: Danila Tikhonov <danila@jiaxyga.com>
Signed-off-by: Barnbás Czémán <trabarni@gmail.com>
---
 Documentation/devicetree/bindings/iio/imu/bosch,bmi160.yaml | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/iio/imu/bosch,bmi160.yaml b/Documentation/devicetree/bindings/iio/imu/bosch,bmi160.yaml
index 47cfba939ca6..3b0a2d8b2e91 100644
--- a/Documentation/devicetree/bindings/iio/imu/bosch,bmi160.yaml
+++ b/Documentation/devicetree/bindings/iio/imu/bosch,bmi160.yaml
@@ -16,7 +16,11 @@ description: |
 
 properties:
   compatible:
-    const: bosch,bmi160
+    oneOf:
+      - const: bosch,bmi160
+      - items:
+          - const: bosch,bmi120
+          - const: bosch,bmi160
 
   reg:
     maxItems: 1

-- 
2.45.0

Re: [PATCH v2 2/2] dt-bindings: iio: imu: bmi160: add bmi120

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 340 bytes --]

On Sat, May 04, 2024 at 02:59:49PM +0200, Barnabás Czémán wrote:
> From: Danila Tikhonov <danila@jiaxyga.com>
> 
> Document bosch,bmi120 compatible.
> 
> Signed-off-by: Danila Tikhonov <danila@jiaxyga.com>
> Signed-off-by: Barnbás Czémán <trabarni@gmail.com>

Acked-by: Conor Dooley <conor.dooley@microchip.com>

Thanks,
Conor.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v2 0/2] Add support for bosch bmi120

[David Lechner]

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