Re: [PATCH 0/3] Add i2c driver for Bosch BMI260 IMU

[Justin Weiss <justin@justinweiss.com>]

Thanks for the review! I really appreciate it.

Jonathan Cameron <jic23@kernel.org> writes:

> On Fri, 11 Oct 2024 08:37:46 -0700
> Justin Weiss <justin@justinweiss.com> wrote:
>
>> The BMI260 is the IMU on a number of handheld PCs. Unfortunately,
>> these devices often misidentify it in ACPI as a BMI160 ("BMI0160," for
>> example), and it can only be correctly identified using the chip
>> ID. I've changed the driver to fail if the chip ID isn't recognized so
>> the firmware initialization data isn't sent to incompatible devices.
>
> So just to check, is the firmware always specific to an individual chip?

For these devices, yes. The BMI160 does not have firmware initialization
data. The 260 and 270 have different firmware data from each other.

> Normally we strongly resist hard checks on mismatched IDs because they break
> the option for using fallback compatibles to get some support on older
> kernels for newer devices, but if the firmware is locked to a
> device then that is a good justification.  Fallback compatibles in DT
> will never work here.

The specific problem I'm trying to avoid with this hard check is the
situation when a device actually has a BMI160, this driver matches
"BMI0160", and sends the BMI260 firmware data to a BMI160 chip.

I suppose this driver could target this situation by only failing to
probe if it detects the BMI160 chip ID. That would imply that the device
is a BMI160 and should not be handled by this driver.

Then, as you suggested in another response, the driver could check the
other chip IDs individually. If the driver detects the 260 chip ID, it
would send the BMI260 firmware and so on with the 270. Otherwise, it
would use the chip_info found by match_data. This would at least handle
the problem we see in shipped devices while keeping it flexible for the
future. I'm happy to make those changes if they make more sense to you.

There's an older thread here that provides more background about the
DSDT confusion:
https://lore.kernel.org/all/CAFqHKTm2WRNkcSoBEE=oNbfu_9d9RagQHLydmv6q1=snO_MXyA@mail.gmail.com/

Justin