Re: [PATCH v2 4/6] iio: imu: bmi270: Add support for BMI260

[Jonathan Cameron <jic23@kernel.org>]

On Fri, 18 Oct 2024 16:36:10 -0700
Justin Weiss <justin@justinweiss.com> wrote:

> Adds support for the Bosch BMI260 6-axis IMU to the Bosch BMI270
> driver. Setup and operation is nearly identical to the Bosch BMI270,
> but has a different chip ID and requires different firmware.
> 
> Firmware is requested and loaded from userspace.
> 
> Signed-off-by: Justin Weiss <justin@justinweiss.com>
Trivial comments inline and a discussion on whether my earlier
don't use an array comment makes sense in this particular case.

Jonathan

> ---
>  drivers/iio/imu/bmi270/bmi270.h      |  1 +
>  drivers/iio/imu/bmi270/bmi270_core.c | 25 +++++++++++++++++++++++--
>  drivers/iio/imu/bmi270/bmi270_i2c.c  | 13 +++++++++++++
>  drivers/iio/imu/bmi270/bmi270_spi.c  |  8 ++++++++
>  4 files changed, 45 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/imu/bmi270/bmi270.h b/drivers/iio/imu/bmi270/bmi270.h
> index 2e8d85a4e419..51e374fd4290 100644
> --- a/drivers/iio/imu/bmi270/bmi270.h
> +++ b/drivers/iio/imu/bmi270/bmi270.h
> @@ -14,6 +14,7 @@ struct bmi270_data {
>  };
>  
>  enum bmi270_device_type {
> +	BMI260,
>  	BMI270,
>  };
>  
> diff --git a/drivers/iio/imu/bmi270/bmi270_core.c b/drivers/iio/imu/bmi270/bmi270_core.c
> index 799df78ec862..b30201dc4e22 100644
> --- a/drivers/iio/imu/bmi270/bmi270_core.c
> +++ b/drivers/iio/imu/bmi270/bmi270_core.c
> @@ -11,6 +11,8 @@
>  #include "bmi270.h"
>  
>  #define BMI270_CHIP_ID_REG				0x00
> +#define BMI160_CHIP_ID_VAL				0xD1

This one looks like a cut and paste error.

> +#define BMI260_CHIP_ID_VAL				0x27
>  #define BMI270_CHIP_ID_VAL				0x24
>  #define BMI270_CHIP_ID_MSK				GENMASK(7, 0)
>  
> @@ -55,6 +57,7 @@
>  #define BMI270_PWR_CTRL_ACCEL_EN_MSK			BIT(2)
>  #define BMI270_PWR_CTRL_TEMP_EN_MSK			BIT(3)
>  
> +#define BMI260_INIT_DATA_FILE "bmi260-init-data.fw"
>  #define BMI270_INIT_DATA_FILE "bmi270-init-data.fw"
>  
>  enum bmi270_scan {
> @@ -67,6 +70,11 @@ enum bmi270_scan {
>  };
>  
>  const struct bmi270_chip_info bmi270_chip_info[] = {
> +	[BMI260] = {
> +		.name = "bmi260",
> +		.chip_id = BMI260_CHIP_ID_VAL,
> +		.fw_name = BMI260_INIT_DATA_FILE,
> +	},
>  	[BMI270] = {
>  		.name = "bmi270",
>  		.chip_id = BMI270_CHIP_ID_VAL,
> @@ -163,8 +171,21 @@ static int bmi270_validate_chip_id(struct bmi270_data *bmi270_device)
>  	if (ret)
>  		return dev_err_probe(dev, ret, "Failed to read chip id");
>  
> -	if (chip_id != BMI270_CHIP_ID_VAL)
> -		dev_info(dev, "Unknown chip id 0x%x", chip_id);
> +	/*
> +	 * Some manufacturers use "BMI0160" for both the BMI160 and
> +	 * BMI260. If the device is actually a BMI160, the bmi160
> +	 * driver should handle it and this driver should not.
> +	 */
> +	if (chip_id == BMI160_CHIP_ID_VAL)
> +		return -ENODEV;
> +
> +	if (chip_id != bmi270_device->chip_info->chip_id)
> +		dev_info(dev, "Unexpected chip id 0x%x", chip_id);
> +
> +	if (chip_id == BMI260_CHIP_ID_VAL)

Ah. My argument on separate IDs means you'd have to do it this way whereas
I was thinking maybe a loop would be a better idea.  Ah well if we
get a lot of supported chips, then we can rethink how to handle this.
For now what you have here is fine and should deal with lack of appropriate
ACPI ID mess.

> +		bmi270_device->chip_info = &bmi270_chip_info[BMI260];
> +	else if (chip_id == BMI270_CHIP_ID_VAL)
> +		bmi270_device->chip_info = &bmi270_chip_info[BMI270];
>  
>  	return 0;
>  }