[PATCH 0/2] Add support for bosch bmi120

[PATCH 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>
---
Danila Tikhonov (2):
      iio: imu: bmi160: add support for bmi120
      dt-bindings: iio: imu: bmi160: add bmi120

 .../devicetree/bindings/iio/imu/bosch,bmi160.yaml  |  4 +++-
 drivers/iio/imu/bmi160/bmi160_core.c               | 24 ++++++++++++++++++----
 drivers/iio/imu/bmi160/bmi160_i2c.c                |  3 +++
 drivers/iio/imu/bmi160/bmi160_spi.c                |  3 +++
 4 files changed, 29 insertions(+), 5 deletions(-)
---
base-commit: 9221b2819b8a4196eecf5476d66201be60fbcf29
change-id: 20240504-bmi120-d3c88dcb3073

Best regards,
-- 
Barnabás Czémán <trabarni@gmail.com>

[PATCH 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 | 24 ++++++++++++++++++++----
 drivers/iio/imu/bmi160/bmi160_i2c.c  |  3 +++
 drivers/iio/imu/bmi160/bmi160_spi.c  |  3 +++
 3 files changed, 26 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/imu/bmi160/bmi160_core.c b/drivers/iio/imu/bmi160/bmi160_core.c
index a77f1a8348ff..015801ad4d9a 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,10 +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;
+
+	ret = bmi160_check_chip_id(val);
+	if (ret) {
+		dev_err(dev, "Wrong chip id %x\n", val);
 		goto disable_regulator;
 	}
 
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 1/2] iio: imu: bmi160: add support for bmi120

[Jonathan Cameron]

On Sat, 04 May 2024 01:45:24 +0200
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>

Hi. A comment inline that is more about us doing things wrong
in the past than the changes you are making.  However, I would
like to fix that up whilst we are here.

Thanks,

Jonathan

> ---
>  drivers/iio/imu/bmi160/bmi160_core.c | 24 ++++++++++++++++++++----
>  drivers/iio/imu/bmi160/bmi160_i2c.c  |  3 +++
>  drivers/iio/imu/bmi160/bmi160_spi.c  |  3 +++
>  3 files changed, 26 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iio/imu/bmi160/bmi160_core.c b/drivers/iio/imu/bmi160/bmi160_core.c
> index a77f1a8348ff..015801ad4d9a 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;

This falls into a miss design issue on the original driver based on
my understanding of what we should do with chip IDs back then.

My current thinking after discussing with DT maintainers is we should
at most print a warning if we fail to match an ID.

The intent being to allow for cases exactly like this one to work with
an older kernel if a fallback compatible is in use.

So whilst you are here, please could you relax the below error path
to just do a dev_warn() or similar and return 0 anyway.

Or better yet, push that warning print to the caller...

> +	}
> +
> +	return -ENODEV;
> +}
> +
>  static int bmi160_chip_init(struct bmi160_data *data, bool use_spi)
>  {
>  	int ret;
> @@ -737,10 +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;
> +
> +	ret = bmi160_check_chip_id(val);
> +	if (ret) {
> +		dev_err(dev, "Wrong chip id %x\n", val);
>  		goto disable_regulator;

For reasons given above, whilst we are hear I'd like this to change to dev_warn() and
carry on as if we did have a match.

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

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

[kernel test robot]

Hi Barnabás,

kernel test robot noticed the following build warnings:

[auto build test WARNING on 9221b2819b8a4196eecf5476d66201be60fbcf29]

url:    https://github.com/intel-lab-lkp/linux/commits/Barnab-s-Cz-m-n/iio-imu-bmi160-add-support-for-bmi120/20240504-074802
base:   9221b2819b8a4196eecf5476d66201be60fbcf29
patch link:    https://lore.kernel.org/r/20240504-bmi120-v1-1-478470a85058%40gmail.com
patch subject: [PATCH 1/2] iio: imu: bmi160: add support for bmi120
config: x86_64-randconfig-121-20240505 (https://download.01.org/0day-ci/archive/20240505/202405051259.R7WyR8LV-lkp@intel.com/config)
compiler: clang version 18.1.4 (https://github.com/llvm/llvm-project e6c3289804a67ea0bb6a86fadbe454dd93b8d855)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240505/202405051259.R7WyR8LV-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202405051259.R7WyR8LV-lkp@intel.com/

sparse warnings: (new ones prefixed by >>)
>> drivers/iio/imu/bmi160/bmi160_core.c:116:10: sparse: sparse: symbol 'bmi_chip_ids' was not declared. Should it be static?

vim +/bmi_chip_ids +116 drivers/iio/imu/bmi160/bmi160_core.c

   115	
 > 116	const u8 bmi_chip_ids[] = {
   117		BMI120_CHIP_ID_VAL,
   118		BMI160_CHIP_ID_VAL,
   119	};
   120	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

[PATCH 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 | 4 +++-
 1 file changed, 3 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..9ca874aea837 100644
--- a/Documentation/devicetree/bindings/iio/imu/bosch,bmi160.yaml
+++ b/Documentation/devicetree/bindings/iio/imu/bosch,bmi160.yaml
@@ -16,7 +16,9 @@ description: |
 
 properties:
   compatible:
-    const: bosch,bmi160
+    enum:
+      - bosch,bmi120
+      - bosch,bmi160
 
   reg:
     maxItems: 1

-- 
2.45.0

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

[Jonathan Cameron]

On Sat, 04 May 2024 01:45:25 +0200
Barnabás Czémán <trabarni@gmail.com> 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>
> ---
>  Documentation/devicetree/bindings/iio/imu/bosch,bmi160.yaml | 4 +++-
>  1 file changed, 3 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..9ca874aea837 100644
> --- a/Documentation/devicetree/bindings/iio/imu/bosch,bmi160.yaml
> +++ b/Documentation/devicetree/bindings/iio/imu/bosch,bmi160.yaml
> @@ -16,7 +16,9 @@ description: |
>  
>  properties:
>    compatible:
> -    const: bosch,bmi160
> +    enum:
> +      - bosch,bmi120
> +      - bosch,bmi160

Please use a fallback compatible for the bmi120 similar to handling
done for st,asm330lhhx in st,lsm6dsx.yaml

Whilst the driver would currently reject the chip ID, the binding should
assume the driver does fallback handling correctly.

Jonathan

>  
>    reg:
>      maxItems: 1
>