Re: [PATCH v5 0/2] Add bmi323 support for ASUS ROG ALLY

[Hans de Goede <hdegoede@redhat.com>]

Hi Jonathan,

On 2/16/24 12:35, Jonathan Cameron wrote:
> On Thu, 15 Feb 2024 10:19:52 -0800
> J Lo <jlobue10@gmail.com> wrote:
> 
>> From: Jonathan LoBue <jlobue10@gmail.com>
> 
> Hi Jonathan
> 
> Cover letter should always include at least a short overview of what
> the patch is doing.
> 
> Long term this solution may be a pain to maintain.
> The reasoning is the DT path where we have moved over time to allow
> for fallback compatibles (same concept exists in ACPI even if it is
> little used) to be used even if we don't recognise a ID read from
> the chip.  The intent being to allow old kernels to work with new
> devices where they really are backwards compatible.
> 
> If that gets fixed in these drivers, we will have to explicitly
> exclude ACPI IDs.
> 
> Hopefully we'll pick up such issues in review though so this should be fine.
> 
> I'd like input from Hans though on whether this solution of duplicating
> the IDs generally works out longer term and is appropriate here.

The BOSC0200 ACPI ID is used in a lot of devices and the ACPI
tables of these devices are not under our control. So we really
have no other option.

Having 2 different drivers match/bind to the same (ACPI or other)
id/device is not unheard of. As long as the probe() method then figures
out it is not the right device and cleanly exits then this is fine.

I'll run a test with patch 2/2 + the bmi323 driver enabled
on a device with a BOSC0200 ACPI id which does actually need
the bmc150 driver to make sure that the bmi323 driver properly
refuses to bind there.

I do see that both drivers write to a reset-register before reading
the id register and those registers are different ...

Looking at the registers used for reset then on the bmc150 it
seems that the bmi323 code is writing the last bytes of the fifo
which should be fine.

And when the bmc150 code is trying to reset a bmi323 it is
writing to the BMI323_FEAT_IO_STATUS_REG. Since the bmi323
driver does a reset itself and programs that register
during init I guess that should be fine to since the value
written by bmc150's probe() will be overwritten.

I'll get back to with test results of letting the bmi323 driver
probe a BOSC0200 (*) device, before the bmc150 driver probes
it and see if things still work then.

Regards,

Hans


*) which will typically be a BMA250E




>> Changes since v4:
>> - Fixed comment location in bmc150.
>> - Fixed signed off by portion.
>>
>> Jonathan LoBue (2):
>>   iio: accel: bmc150: Duplicate ACPI entries
>>   iio: imu: bmi323: Add and enable ACPI Match Table
>>
>>  drivers/iio/accel/bmc150-accel-i2c.c | 13 +++++++++++++
>>  drivers/iio/imu/bmi323/bmi323_i2c.c  | 20 ++++++++++++++++++++
>>  2 files changed, 33 insertions(+)
>> --
>> 2.43.0
>