Re: [PATCH] iio: imu: bmi323: Support loading of bmi323 driver for ASUS ROG ALLY

[Jonathan LoBue <jlobue10@gmail.com>]

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

On Saturday, February 10, 2024 7:25:50 AM PST Jonathan Cameron wrote:
> On Fri, 09 Feb 2024 08:05:14 -0800
> Jonathan LoBue <jlobue10@gmail.com> wrote:
> 
> > Due to an ACPI match of "BOSC0200" and existing gyro drivers, the ASUS ROG ALLY attempts to incorrectly load the bmc150 driver.
> > This leaves the gyro inoperable for ASUS ROG ALLY. The correct gyro driver, bmi323, has already been upstreamed as part of the 6.8 kernel changes.
> > In order to load the correct bmi323 driver for ASUS ROG ALLY's gyro, this patch uses a DMI match to unhook the ASUS ROG ALLY from loading the bmc150 driver.
> > This unhooking is also added for the Ayaneo AIR Plus device, as requested by ChimeraOS devs.
> > 
> 
> Please reformat as a patch as per the documentation for submitting patches.
> Wrap the lines to 75 chars in the description.
> 
> > ---
> The cut lines affect what git will pick up and I don't think that's your
> intent.
> 
> More generally I'd just like to confirm I have understood the correctly.
> We have two incompatible devices advertised with the same ACPI ID?
> If anyone has contacts with Bosch or Asus can we chase down how this happened
> and preferably point out to them that it causes problems if they
> through device that don't have actually compatible register sets into
> the same ID.
> 
> I assume this occurred because there is some hyrda of a driver on
> windows that copes with all sorts of different Bosch devices.
> 
> It's a valid bosch ID - I've no idea how bosch issues these but
> normal practice is one per device interface (so if a device register
> compatible you can share an ID, not otherwise).  They have lots of
> ID space (and can trivially get more if they need it)...
> 
> Solution wise, I'm not keen on having having a DMI check against
> particular boards.  Possibly we can add another driver that
> binds just to the BOSCH ID and does just enough querying of the part
> ID (not the identify of the board) to figure out what it is and
> kick of probing the right driver.
> 
> Andy, you see more of this mess I think than anyway, any thoughts on
> how to handle this elegantly?
> 
> Jonathan
> 
> 
> > 
> > --- a/drivers/iio/accel/bmc150-accel-core.c
> > +++ b/drivers/iio/accel/bmc150-accel-core.c
> > @@ -10,6 +10,7 @@
> >  #include <linux/delay.h>
> >  #include <linux/slab.h>
> >  #include <linux/acpi.h>
> > +#include <linux/dmi.h>
> >  #include <linux/of_irq.h>
> >  #include <linux/pm.h>
> >  #include <linux/pm_runtime.h>
> > @@ -1670,6 +1671,9 @@ int bmc150_accel_core_probe(struct device *dev, struct regmap *regmap, int irq,
> >  	struct iio_dev *indio_dev;
> >  	int ret;
> >  
> > +	if (dmi_match(DMI_BOARD_NAME, "RC71L") || (dmi_match(DMI_BOARD_NAME, "AB05-AMD") && dmi_match(DMI_PRODUCT_NAME, "AIR Plus")))
> > +		return -ENODEV; // Abort loading bmc150 for ASUS ROG ALLY, Ayaneo Air Plus
> > +
> >  	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> >  	if (!indio_dev)
> >  		return -ENOMEM;
> > 
> > ---
> > 
> > Now, after this unhooking from bmc150, loading the correct bmi323 driver needs to occur. In order to accomplish this, an ACPI match table is added to bmi323.
> > 
> > ---
> > 
> > --- a/drivers/iio/imu/bmi323/bmi323_i2c.c
> > +++ b/drivers/iio/imu/bmi323/bmi323_i2c.c
> > @@ -5,6 +5,7 @@
> >   * Copyright (C) 2023, Jagath Jog J <jagathjog1996@gmail.com>
> >   */
> >  
> > +#include <linux/acpi.h>
> >  #include <linux/i2c.h>
> >  #include <linux/mod_devicetable.h>
> >  #include <linux/module.h>
> > @@ -93,6 +94,12 @@ static int bmi323_i2c_probe(struct i2c_c
> >  	return bmi323_core_probe(dev);
> >  }
> >  
> > +static const struct acpi_device_id bmi323_acpi_match[] = {
> > +	{"BOSC0200"},
> > +	{ },
> > +};
> > +MODULE_DEVICE_TABLE(acpi, bmi323_acpi_match);
> > +
> >  static const struct i2c_device_id bmi323_i2c_ids[] = {
> >  	{ "bmi323" },
> >  	{ }
> > @@ -109,6 +116,7 @@ static struct i2c_driver bmi323_i2c_driv
> >  	.driver = {
> >  		.name = "bmi323",
> >  		.of_match_table = bmi323_of_i2c_match,
> > +		.acpi_match_table = ACPI_PTR(bmi323_acpi_match),
> >  	},
> >  	.probe = bmi323_i2c_probe,
> >  	.id_table = bmi323_i2c_ids,
> > 
> > ---
> > 
> > Patching these two files in this manner successfully accomplishes unhooking the ASUS ROG ALLY from the bmc150 driver and loading of the bmi323 driver.
> > 
> > Best Regards,
> > Jon LoBue
> > 
> > Co-developed-by: Jonathan LoBue <jlobue10@gmail.com>
> > Signed-off-by: Jonathan LoBue <jlobue10@gmail.com>
> > Co-developed-by: Luke D. Jones <luke@ljones.dev>
> > Signed-off-by: Luke D. Jones <luke@ljones.dev>
> > Co-developed-by: Denis Benato <benato.denis96@gmail.com>
> > Signed-off-by: Denis Benato <benato.denis96@gmail.com>
> > Co-developed-by: Antheas Kapenekakis <lkml@antheas.dev>
> > Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
> As above, look up how to submit a kernel patch.
> 
> https://www.kernel.org/doc/html/v4.17/process/submitting-patches.html
> 
> Thanks,
> 
> Jonathan

Thank you for the feedback. I will work to fix the formatting. I must have made a mistake somewhere. I was referring to that exact website while doing this.
The original source for the patch is here: https://github.com/jlobue10/ALLY_Nobara_fixes/raw/main/0003-iio-imu_Add_ROG_ALLY_bmi323-support.patch .
Several Linux distros have been unofficially using it for a while now that I know of (Nobara, Bazzite, Manjaro, ChimeraOS soon).
I will fix that in a future reply. I normally use git diff and perhaps made a mistake trying to do the diff -up method instead. I will fix it.
I understand that other canonical parts may be missing too. This will also be fixed in a future reply.

I completely agree with your assessment. This proposed patch is really meant as a temporary "band-aid" solution.
There should be consensus reached about a better long term solution going forward.
Needing to do the DMI match and abort the loading of the bmc150 driver for ASUS ROG ALLY is a symptom of a larger problem.
The generic identifier "BOSC0200" is not adequate when referring to multiple different devices that the kernel wants to load, which then happens on a first come basis.
I think a better identifier in this case would be something like "BMI323" but this decision should be left up to BOSCH (and ASUS in this case).
The problem is that these identifiers don't really matter for the Windows' side drivers so manufacturers give them little to no extra thought or consideration.
There needs to be some agreement or consensus reached by the manufacturers in this regard. This is a much larger problem with several gyro drivers.
I've had this conversation before and confirmed the larger issue with some ChimeraOS devs. Thanks.

Best Regards,
Jon LoBue

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