Re: [PATCH v2 2/2] iio: imu: bmi323: Add and enable ACPI Match Table

[Jonathan LoBue <jlobue10@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2236 bytes --]

On Wednesday, February 14, 2024 1:39:19 AM PST Andy Shevchenko wrote:
> On Wed, Feb 14, 2024 at 12:39 AM Jonathan LoBue <jlobue10@gmail.com> wrote:
> >
> > This patch adds the ACPI match table for ASUS ROG ALLY to load the bmi323
> > driver with an ACPI match of "BOSC0200".
> 
> With the remarks below,
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> (carry the tag if you send a new version)
> 
> ...
> 
> The below comment...
> 
> > +/*
> > + * The "BOSC0200" ACPI identifier used here in the bmi323 driver is not
> 
> s/ACPI//
> s/in the bmi323 driver//

I will fix the first sentence in the comment.

> This seems different wording to the other one. Have you looked at the
> code if it's indeed the case? Because we may not rely on the module
> load order.
> 

Yes it's slightly different wording intentionally. I am able to test that
when the bmc150 driver starts loading on the ASUS ROG ALLY with a bmi323
chip that the ACPI match happens, the driver attempts to initialize but
does correctly fail at the chip id check portion.

I do not own a device with a bmc150 chip in it, but the same should be
happening in the reverse situation where a device with a bmc150 chip
starts to load the bmi323 driver. There is a chip id check in the
bmi323_init function where a bmc150 device should fail at, and the driver
should release the device. Without a device, I am unable to test that
this works correctly or not. Logically the code looks similar between the
two drivers.

> > If and when a different driver (such as bmc150) starts to load
> > + * with the "BOSC0200" ACPI match, a short reset should ensure that the
> > + * device is not in a bad state during that driver initialization. This
> > + * device reset does occur in both the bmi323 and bmc150 init sequences.
> > + */
> > +
> > +static const struct acpi_device_id bmi323_acpi_match[] = {
> 
> ...should be here (and indented accordingly).
> 
> > +       { "BOSC0200" },
> > +       { }
> > +};
> 

Depending on Jonathan Cameron's preference about where to put the comment,
and if he wants a v3 or not... If we want to make a v3, should I create a new thread for that?

Best Regards,
Jon LoBue



[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 488 bytes --]