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

[Justin Weiss <justin@justinweiss.com>]

Jonathan Cameron <jic23@kernel.org> writes:

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

No, this was intentional -- I added the BMI160 chip ID here so it could
be checked later to avoid conflicting with the existing bmi160 driver. I
could add newlines before and after this group of _ID_VAL #defines if it
makes it clearer.

>> +#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;

This is where that BMI160 chip ID is checked.

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

I like the idea of separate structures, so I'll keep the if / else
here. I think it would be straightforward to change later without
conflicts if there are more supported chips.

I will change this to check against bmi260_chip_info.chip_id,
etc. instead of the constants, to make sure they stay consistent.

Justin

>> +		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;
>>  }