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

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

[Jonathan LoBue]

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

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.

---

--- 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 <dbenato.denis96@gmail.com>
Signed-off-by: Denis Benato <dbenato.denis96@gmail.com>
Co-developed-by: Antheas Kapenekakis <lkml@antheas.dev>
Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>

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

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

[Jonathan Cameron]

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 <dbenato.denis96@gmail.com>
> Signed-off-by: Denis Benato <dbenato.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

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

[Jonathan LoBue]

[-- 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 --]

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

[Jonathan Cameron]

On Sat, 10 Feb 2024 08:23:00 -0800
Jonathan LoBue <jlobue10@gmail.com> wrote:

> 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).
It needs to be a compliant ACPI ID.  BOSC is Bosch's valid manufacturer ID.
The code after that tends to just be allocated by someone inside the company
who keeps a bit list of IDs  (I have access to the list of HiSilicon HISIXXXX ones for
example but no idea how Bosch manages this.)

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

Sure. Please also wrap your emails to the mailing list to under 80 chars (roughly).


> 
> Best Regards,
> Jon LoBue

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

[Jonathan LoBue]

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

> It needs to be a compliant ACPI ID.  BOSC is Bosch's valid manufacturer ID.
> The code after that tends to just be allocated by someone inside the company
> who keeps a bit list of IDs  (I have access to the list of HiSilicon HISIXXXX ones for
> example but no idea how Bosch manages this.)

Understood. So if there isn't already a valid and unique identifier for
the BMI323 chip, then BOSCH must come up with one in that BOSCXXXX format.
Then ASUS should update the ACPI table with a BIOS update to use the
proper code for the BOSCH BMI323 chip. This is certainly the better long-term
solution. As it stands now, this proposed patch will get around the current
problem. If and when BOSCH and ASUS update the identifier, then the first
portion of this patch (unhooking from bmc150) could be removed. Then the
ACPI match table added in the BMI323 driver would just need to be updated
to reflect the proper code for the BOSCH BMI323 chip.

> Sure. Please also wrap your emails to the mailing list to under 80 chars (roughly).

Yes, sorry. Thank you for the guidance.

Best Regards,
Jon LoBue



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

[PATCH 1/2] iio: accel: bmc150: ASUS ROG ALLY Abort Loading

[Jonathan LoBue]

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

From 0aed4d398be185d43b92db63693bb1c5c6a8a78b Mon Sep 17 00:00:00 2001
From: Jonathan LoBue <jlobue10@gmail.com>
Date: Sat, 10 Feb 2024 12:28:35 -0800
Subject: [PATCH 1/2] iio: accel: bmc150: ASUS ROG ALLY Abort Loading

This portion of the patch series aborts the loading of the bmc150 driver
upon DMI board match for ASUS ROG ALLY and Ayaneo Air Plus (requested by
ChimeraOS dev).

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>
---
 drivers/iio/accel/bmc150-accel-core.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/iio/accel/bmc150-accel-core.c b/drivers/iio/accel/bmc150-accel-core.c
index 110591804b4c..576da9def8eb 100644
--- 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;
-- 
2.43.0

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

Re: [PATCH 1/2] iio: accel: bmc150: ASUS ROG ALLY Abort Loading

[Andy Shevchenko]

On Sun, Feb 11, 2024 at 12:32 AM Jonathan LoBue <jlobue10@gmail.com> wrote:
>
> From 0aed4d398be185d43b92db63693bb1c5c6a8a78b Mon Sep 17 00:00:00 2001
> From: Jonathan LoBue <jlobue10@gmail.com>
> Date: Sat, 10 Feb 2024 12:28:35 -0800
> Subject: [PATCH 1/2] iio: accel: bmc150: ASUS ROG ALLY Abort Loading

Something went wrong, please use `git send-email ...` to send patches.

> This portion of the patch series aborts the loading of the bmc150 driver
> upon DMI board match for ASUS ROG ALLY and Ayaneo Air Plus (requested by
> ChimeraOS dev).

...

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

Please, make this as the proper table (see many examples in
drivers/platform/x86/ folder on how to do that).

...

Speaking of the PDx86 subsystem, OTOH maybe we want the quirk patch to
be outside of IIO and enumerate the IIO drivers based on i2c/spi ID
tables (there are also examples unde PDx86 folder for such tricks).

Hans, Ilpo, what do you think? (Jonathan, please also include Hans in
the similar cases in the future with ACPI IDs and related ambiguity.)


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/2] iio: accel: bmc150: ASUS ROG ALLY Abort Loading

[Jonathan LoBue]

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

On Sunday, February 11, 2024 9:04:56 AM PST Andy Shevchenko wrote:
> Something went wrong, please use `git send-email ...` to send patches.

Will do once I can test the suggested updated table method.

> Please, make this as the proper table (see many examples in
> drivers/platform/x86/ folder on how to do that).

This DMI board match and aborting of loading the driver is hopefully
a temporary portion of this patch. The ideal fix is that BOSCH informs
ASUS and other system builders of the proper and unique BOSCXXXX
identifier so that BIOSes can be updated with those and this portion
of the patch can be removed. As it stands now, this is the "band-aid"
workaround of having conflicting (same) IDs for different chips.

Best Regards,
Jon LoBue



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

Re: [PATCH 1/2] iio: accel: bmc150: ASUS ROG ALLY Abort Loading

[Andy Shevchenko]

On Mon, Feb 12, 2024 at 9:21 AM Jonathan LoBue <jlobue10@gmail.com> wrote:
>
> On Sunday, February 11, 2024 9:04:56 AM PST Andy Shevchenko wrote:
> > Something went wrong, please use `git send-email ...` to send patches.
>
> Will do once I can test the suggested updated table method.
>
> > Please, make this as the proper table (see many examples in
> > drivers/platform/x86/ folder on how to do that).
>
> This DMI board match and aborting of loading the driver is hopefully
> a temporary portion of this patch. The ideal fix is that BOSCH informs
> ASUS and other system builders of the proper and unique BOSCXXXX
> identifier so that BIOSes can be updated with those and this portion
> of the patch can be removed. As it stands now, this is the "band-aid"
> workaround of having conflicting (same) IDs for different chips.

Even if fixed (which has to be done anyway) it can be undone in old
firmwares — there is no solution to make all affected users update
firmware. Do we have real products on the market with the wrong ID (I
assume we do)?

-- 
With Best Regards,
Andy Shevchenko

[PATCH v1] iio: imu: bmi323: Add and enable ACPI Match Table

[Jonathan LoBue]

This patch adds the ACPI match table for ASUS ROG ALLY to load the bmi323
driver with an ACPI match of "BOSC0200".

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>
---
 drivers/iio/imu/bmi323/bmi323_i2c.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/iio/imu/bmi323/bmi323_i2c.c b/drivers/iio/imu/bmi323/bmi323_i2c.c
index 20a8001b9956..fd1c109bf75c 100644
--- a/drivers/iio/imu/bmi323/bmi323_i2c.c
+++ b/drivers/iio/imu/bmi323/bmi323_i2c.c
@@ -93,6 +93,12 @@ static int bmi323_i2c_probe(struct i2c_client *i2c)
 	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 +115,7 @@ static struct i2c_driver bmi323_i2c_driver = {
 	.driver = {
 		.name = "bmi323",
 		.of_match_table = bmi323_of_i2c_match,
+		.acpi_match_table = bmi323_acpi_match,
 	},
 	.probe = bmi323_i2c_probe,
 	.id_table = bmi323_i2c_ids,
-- 
2.43.0

Re: [PATCH v1] iio: imu: bmi323: Add and enable ACPI Match Table

[Andy Shevchenko]

On Tue, Feb 13, 2024 at 4: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".

> ---

I'm lost. You sent a lot of patches / patch series all of which are v1. Can you:
- use versioning (`git format-patch -v<X>...`, where <X> is a plain
version number)
- add a changelog here (after the cutter '---' line) to explain the
history of the changes
?

...

> +static const struct acpi_device_id bmi323_acpi_match[] = {
> +       { "BOSC0200" },
> +       { }
> +};

Since there is a collision please add a big comment in _both_ drivers
before such ID to explain what's going on.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1] iio: imu: bmi323: Add and enable ACPI Match Table

[Jonathan LoBue]

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

On Tuesday, February 13, 2024 2:49:24 AM PST Andy Shevchenko wrote:
> I'm lost. You sent a lot of patches / patch series all of which are v1. Can you:
> - use versioning (`git format-patch -v<X>...`, where <X> is a plain
> version number)
> - add a changelog here (after the cutter '---' line) to explain the
> history of the changes
> ?

Yes, I will do this. The changes so far included dropping the no longer
necessary DMI quirks portion in the bmc150 driver. I understand from
your comment that we want to add a comment in the bmc150 driver though
to explain what is going on with duplicate ACPI identifiers in different
drivers. I will add a similar comment in the bmi323 driver. The changes
so far also included the fixes that you requested earlier in bmi323:
dropping the duplicate header include entry (included already in other
header file), removing the unnecessary comma in the ACPI match table
portion, and removing the ACPI_PTR when invoking the ACPI match table.

> Since there is a collision please add a big comment in _both_ drivers
> before such ID to explain what's going on.

I will do this and add a changelog after the cutter as requested. Since there
are some changes from my initial submission attempt and with the additional
requested comments, is v2 going to be okay to use so there's no ambiguity
about which patch version to use? I will attach the version label with
git format-patch as requested. Thanks.

Best Regards,
Jon LoBue


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

Re: [PATCH v1] iio: imu: bmi323: Add and enable ACPI Match Table

[Andy Shevchenko]

On Tue, Feb 13, 2024 at 7:14 PM Jonathan LoBue <jlobue10@gmail.com> wrote:
> On Tuesday, February 13, 2024 2:49:24 AM PST Andy Shevchenko wrote:
> > I'm lost. You sent a lot of patches / patch series all of which are v1. Can you:
> > - use versioning (`git format-patch -v<X>...`, where <X> is a plain
> > version number)
> > - add a changelog here (after the cutter '---' line) to explain the
> > history of the changes
> > ?
>
> Yes, I will do this. The changes so far included dropping the no longer
> necessary DMI quirks portion in the bmc150 driver. I understand from
> your comment that we want to add a comment in the bmc150 driver though
> to explain what is going on with duplicate ACPI identifiers in different
> drivers. I will add a similar comment in the bmi323 driver. The changes
> so far also included the fixes that you requested earlier in bmi323:
> dropping the duplicate header include entry (included already in other
> header file), removing the unnecessary comma in the ACPI match table
> portion, and removing the ACPI_PTR when invoking the ACPI match table.
>
> > Since there is a collision please add a big comment in _both_ drivers
> > before such ID to explain what's going on.
>
> I will do this and add a changelog after the cutter as requested. Since there
> are some changes from my initial submission attempt and with the additional
> requested comments, is v2 going to be okay to use so there's no ambiguity
> about which patch version to use?

Sounds good as we seem never to have seen v2 on the topic.

> I will attach the version label with
> git format-patch as requested. Thanks.


-- 
With Best Regards,
Andy Shevchenko

[PATCH v2 1/2] iio: accel: bmc150: Duplicate ACPI entries

[Jonathan LoBue]

This patch adds a description of the duplicate ACPI identifier issue
between devices using bmc150 and bmi323.

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

Comment describing the duplicate ACPI identifier issue has been added
before the "BOSC0200" entry here.

 drivers/iio/accel/bmc150-accel-i2c.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/iio/accel/bmc150-accel-i2c.c b/drivers/iio/accel/bmc150-accel-i2c.c
index ee1ba134ad42..c831642a0e3d 100644
--- a/drivers/iio/accel/bmc150-accel-i2c.c
+++ b/drivers/iio/accel/bmc150-accel-i2c.c
@@ -13,6 +13,18 @@
 
 #include "bmc150-accel.h"
 
+/*
+ * The "BOSC0200" ACPI identifier used here in the bmc150 driver is not
+ * unique to devices using bmc150. The same "BOSC0200" identifier is found
+ * in the ACPI tables of the ASUS ROG ALLY and Ayaneo AIR Plus which both
+ * use a Bosch BMI323 chip. This creates a conflict with duplicate ACPI
+ * identifiers which multiple drivers want to use. Fortunately, when the
+ * bmc150 driver starts to load on the ASUS ROG ALLY, the chip id check
+ * portion fails (correctly) and a dmesg output similar to this:
+ * "bmc150_accel_i2c i2c-BOSC0200:00: Invalid chip 0" can be seen.
+ * This allows the bmi323 driver to take over for ASUS ROG ALLY.
+ */
+
 #ifdef CONFIG_ACPI
 static const struct acpi_device_id bmc150_acpi_dual_accel_ids[] = {
 	{"BOSC0200"},
-- 
2.43.0

Re: [PATCH v2 1/2] iio: accel: bmc150: Duplicate ACPI entries

[Andy Shevchenko]

On Wed, Feb 14, 2024 at 12:38 AM Jonathan LoBue <jlobue10@gmail.com> wrote:
>
> This patch adds a description of the duplicate ACPI identifier issue
> between devices using bmc150 and bmi323.

With the remarks below,
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
(carry the tag if you send a new version)

...

> Comment describing the duplicate ACPI identifier issue has been added
> before the "BOSC0200" entry here.

Hmm...

...

> +/*
> + * The "BOSC0200" ACPI identifier used here in the bmc150 driver is not

s/ACPI//
s/in the bmc150 driver//

> + * unique to devices using bmc150. The same "BOSC0200" identifier is found
> + * in the ACPI tables of the ASUS ROG ALLY and Ayaneo AIR Plus which both
> + * use a Bosch BMI323 chip. This creates a conflict with duplicate ACPI
> + * identifiers which multiple drivers want to use. Fortunately, when the
> + * bmc150 driver starts to load on the ASUS ROG ALLY, the chip id check
> + * portion fails (correctly) and a dmesg output similar to this:
> + * "bmc150_accel_i2c i2c-BOSC0200:00: Invalid chip 0" can be seen.
> + * This allows the bmi323 driver to take over for ASUS ROG ALLY.
> + */
> +
>  #ifdef CONFIG_ACPI
>  static const struct acpi_device_id bmc150_acpi_dual_accel_ids[] = {

...it should be here. But don't resend, let's Jonathan to decide in
case he won't amend this when applying.

>         {"BOSC0200"},


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/2] iio: accel: bmc150: Duplicate ACPI entries

[Jonathan LoBue]

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

On Wednesday, February 14, 2024 1:35:56 AM PST Andy Shevchenko wrote:
> On Wed, Feb 14, 2024 at 12:38 AM Jonathan LoBue <jlobue10@gmail.com> wrote:
> >
> > This patch adds a description of the duplicate ACPI identifier issue
> > between devices using bmc150 and bmi323.
> 
> With the remarks below,
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> (carry the tag if you send a new version)
> 
> ...

Will do.

> > Comment describing the duplicate ACPI identifier issue has been added
> > before the "BOSC0200" entry here.
> 
> Hmm...

You asked for a changelog after the cutter, although it really seems
unnecessary to me here as it's repetitive in nature with comment above.

> > +/*
> > + * The "BOSC0200" ACPI identifier used here in the bmc150 driver is not
> 
> s/ACPI//
> s/in the bmc150 driver//
> 

So update the first sentence in the comment to be:

The "BOSC0200" identifier used here is not...
?

> > + * unique to devices using bmc150. The same "BOSC0200" identifier is found
> > + * in the ACPI tables of the ASUS ROG ALLY and Ayaneo AIR Plus which both
> > + * use a Bosch BMI323 chip. This creates a conflict with duplicate ACPI
> > + * identifiers which multiple drivers want to use. Fortunately, when the
> > + * bmc150 driver starts to load on the ASUS ROG ALLY, the chip id check
> > + * portion fails (correctly) and a dmesg output similar to this:
> > + * "bmc150_accel_i2c i2c-BOSC0200:00: Invalid chip 0" can be seen.
> > + * This allows the bmi323 driver to take over for ASUS ROG ALLY.
> > + */
> > +
> >  #ifdef CONFIG_ACPI
> >  static const struct acpi_device_id bmc150_acpi_dual_accel_ids[] = {
> 
> ...it should be here. But don't resend, let's Jonathan to decide in
> case he won't amend this when applying.
> 
> >         {"BOSC0200"},

This seems to be a stylistic preference on whether or not to include this
long comment inside of the ACPI match table or not. Stylistically, my
preference would be to include it directly above the match table and not
inside of it. I will wait for Jonathan Cameron's comments about what to do
here.

Best Regards,
Jon LoBue

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

Re: [PATCH v2 1/2] iio: accel: bmc150: Duplicate ACPI entries

[Andy Shevchenko]

On Wed, Feb 14, 2024 at 5:07 PM Jonathan LoBue <jlobue10@gmail.com> wrote:
> On Wednesday, February 14, 2024 1:35:56 AM PST Andy Shevchenko wrote:
> > On Wed, Feb 14, 2024 at 12:38 AM Jonathan LoBue <jlobue10@gmail.com> wrote:

...

> > > Comment describing the duplicate ACPI identifier issue has been added
> > > before the "BOSC0200" entry here.
> >
> > Hmm...
>
> You asked for a changelog after the cutter, although it really seems
> unnecessary to me here as it's repetitive in nature with comment above.

This is fine and needed. My comment was about the actual placement of
the comment (should be immediately before the ID entry and not
detached from it.

...

> > > + * The "BOSC0200" ACPI identifier used here in the bmc150 driver is not
> >
> > s/ACPI//
> > s/in the bmc150 driver//
> >
>
> So update the first sentence in the comment to be:
>
> The "BOSC0200" identifier used here is not...
> ?

Yes.

> > > + * unique to devices using bmc150. The same "BOSC0200" identifier is found
> > > + * in the ACPI tables of the ASUS ROG ALLY and Ayaneo AIR Plus which both
> > > + * use a Bosch BMI323 chip. This creates a conflict with duplicate ACPI
> > > + * identifiers which multiple drivers want to use. Fortunately, when the
> > > + * bmc150 driver starts to load on the ASUS ROG ALLY, the chip id check
> > > + * portion fails (correctly) and a dmesg output similar to this:
> > > + * "bmc150_accel_i2c i2c-BOSC0200:00: Invalid chip 0" can be seen.
> > > + * This allows the bmi323 driver to take over for ASUS ROG ALLY.

...

> > >  static const struct acpi_device_id bmc150_acpi_dual_accel_ids[] = {
> >
> > ...it should be here. But don't resend, let's Jonathan to decide in
> > case he won't amend this when applying.
> >
> > >         {"BOSC0200"},
>
> This seems to be a stylistic preference on whether or not to include this
> long comment inside of the ACPI match table or not. Stylistically, my
> preference would be to include it directly above the match table and not
> inside of it. I will wait for Jonathan Cameron's comments about what to do
> here.

In my p.o.v. it's not stylic as we refer to the exact ID and having
comment detached is, besides being unusual, may go outdated too
quickly as code is being grown and developed. So, I really want it to
be closer to the ID entry.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/2] iio: accel: bmc150: Duplicate ACPI entries

[Jonathan Cameron]

On Wed, 14 Feb 2024 17:39:53 +0200
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Wed, Feb 14, 2024 at 5:07 PM Jonathan LoBue <jlobue10@gmail.com> wrote:
> > On Wednesday, February 14, 2024 1:35:56 AM PST Andy Shevchenko wrote:  
> > > On Wed, Feb 14, 2024 at 12:38 AM Jonathan LoBue <jlobue10@gmail.com> wrote:  
> 
> ...
> 
> > > > Comment describing the duplicate ACPI identifier issue has been added
> > > > before the "BOSC0200" entry here.  
> > >
> > > Hmm...  
> >
> > You asked for a changelog after the cutter, although it really seems
> > unnecessary to me here as it's repetitive in nature with comment above.  
> 
> This is fine and needed. My comment was about the actual placement of
> the comment (should be immediately before the ID entry and not
> detached from it.
> 
> ...
> 
> > > > + * The "BOSC0200" ACPI identifier used here in the bmc150 driver is not  
> > >
> > > s/ACPI//
> > > s/in the bmc150 driver//
> > >  
> >
> > So update the first sentence in the comment to be:
> >
> > The "BOSC0200" identifier used here is not...
> > ?  
> 
> Yes.
> 
> > > > + * unique to devices using bmc150. The same "BOSC0200" identifier is found
> > > > + * in the ACPI tables of the ASUS ROG ALLY and Ayaneo AIR Plus which both
> > > > + * use a Bosch BMI323 chip. This creates a conflict with duplicate ACPI
> > > > + * identifiers which multiple drivers want to use. Fortunately, when the
> > > > + * bmc150 driver starts to load on the ASUS ROG ALLY, the chip id check
> > > > + * portion fails (correctly) and a dmesg output similar to this:
> > > > + * "bmc150_accel_i2c i2c-BOSC0200:00: Invalid chip 0" can be seen.
> > > > + * This allows the bmi323 driver to take over for ASUS ROG ALLY.  
> 
> ...
> 
> > > >  static const struct acpi_device_id bmc150_acpi_dual_accel_ids[] = {  
> > >
> > > ...it should be here. But don't resend, let's Jonathan to decide in
> > > case he won't amend this when applying.
> > >  
> > > >         {"BOSC0200"},  
> >
> > This seems to be a stylistic preference on whether or not to include this
> > long comment inside of the ACPI match table or not. Stylistically, my
> > preference would be to include it directly above the match table and not
> > inside of it. I will wait for Jonathan Cameron's comments about what to do
> > here.  
> 
> In my p.o.v. it's not stylic as we refer to the exact ID and having
> comment detached is, besides being unusual, may go outdated too
> quickly as code is being grown and developed. So, I really want it to
> be closer to the ID entry.

Yes, please send a v3 with it next to the relevant ID.
Also dont send new versions in reply to old ones.
For IIO patches at least, a new thread every time please.

> 

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

[Jonathan LoBue]

This patch adds the ACPI match table for ASUS ROG ALLY to load the bmi323
driver with an ACPI match of "BOSC0200".

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

Formatting fixes, removed duplicate header, and removed ACPI_PTR
from previous submission.

Added an explanation of the duplicate ACPI identifier issue between
devices using bmc150 and bmi323.

 drivers/iio/imu/bmi323/bmi323_i2c.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/iio/imu/bmi323/bmi323_i2c.c b/drivers/iio/imu/bmi323/bmi323_i2c.c
index 20a8001b9956..22826a2efc6f 100644
--- a/drivers/iio/imu/bmi323/bmi323_i2c.c
+++ b/drivers/iio/imu/bmi323/bmi323_i2c.c
@@ -93,6 +93,25 @@ static int bmi323_i2c_probe(struct i2c_client *i2c)
 	return bmi323_core_probe(dev);
 }
 
+/*
+ * The "BOSC0200" ACPI identifier used here in the bmi323 driver is not
+ * unique to bmi323 devices. The same "BOSC0200" identifier is found in
+ * devices using the bmc150 chip. This creates a conflict with duplicate
+ * ACPI identifiers which multiple drivers want to use. If a non-bmi323
+ * device starts to load with this "BOSC0200" ACPI match here, then the
+ * chip id check portion should fail and the driver should relinquish the
+ * device. 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[] = {
+	{ "BOSC0200" },
+	{ }
+};
+MODULE_DEVICE_TABLE(acpi, bmi323_acpi_match);
+
 static const struct i2c_device_id bmi323_i2c_ids[] = {
 	{ "bmi323" },
 	{ }
@@ -109,6 +128,7 @@ static struct i2c_driver bmi323_i2c_driver = {
 	.driver = {
 		.name = "bmi323",
 		.of_match_table = bmi323_of_i2c_match,
+		.acpi_match_table = bmi323_acpi_match,
 	},
 	.probe = bmi323_i2c_probe,
 	.id_table = bmi323_i2c_ids,
-- 
2.43.0

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

[Andy Shevchenko]

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

> + * unique to bmi323 devices. The same "BOSC0200" identifier is found in
> + * devices using the bmc150 chip. This creates a conflict with duplicate
> + * ACPI identifiers which multiple drivers want to use. If a non-bmi323
> + * device starts to load with this "BOSC0200" ACPI match here, then the
> + * chip id check portion should fail and the driver should relinquish the
> + * device.

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.

> 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" },
> +       { }
> +};


-- 
With Best Regards,
Andy Shevchenko

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

[Jonathan LoBue]

[-- 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 --]

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

[Andy Shevchenko]

On Wed, Feb 14, 2024 at 5:15 PM Jonathan LoBue <jlobue10@gmail.com> wrote:
> 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:

...

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

But are those IDs different?

...

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

Sure.

I also recommend looking at my "smart" script [1] that helps sending
kernel related patches. Improvements are welcome as GH pull-requests!

[1]: https://github.com/andy-shev/home-bin-tools/blob/master/ge2maintainer.sh

-- 
With Best Regards,
Andy Shevchenko

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

[Jonathan LoBue]

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

On Wednesday, February 14, 2024 7:31:14 AM PST Andy Shevchenko wrote:
> But are those IDs different?

During the chip id check during init (after ACPI match), yes those IDs are
different between devices with bmi323 and bmc150.

> I also recommend looking at my "smart" script [1] that helps sending
> kernel related patches. Improvements are welcome as GH pull-requests!
> 
> [1]: https://github.com/andy-shev/home-bin-tools/blob/master/ge2maintainer.sh

Thanks for this useful script, and thanks for your patience and guidance.
If I make a future submission after this patch series, the whole process
should go much smoother as I have learned a lot in this thread.

Best Regards,
Jon LoBue


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

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

[Andy Shevchenko]

On Wed, Feb 14, 2024 at 7:35 PM Jonathan LoBue <jlobue10@gmail.com> wrote:
>
> On Wednesday, February 14, 2024 7:31:14 AM PST Andy Shevchenko wrote:
> > But are those IDs different?
>
> During the chip id check during init (after ACPI match), yes those IDs are
> different between devices with bmi323 and bmc150.

Thanks for confirming!
Perhaps you may improve the wording based on this information.

> > I also recommend looking at my "smart" script [1] that helps sending
> > kernel related patches. Improvements are welcome as GH pull-requests!
> >
> > [1]: https://github.com/andy-shev/home-bin-tools/blob/master/ge2maintainer.sh
>
> Thanks for this useful script, and thanks for your patience and guidance.
> If I make a future submission after this patch series, the whole process
> should go much smoother as I have learned a lot in this thread.

You're welcome!

-- 
With Best Regards,
Andy Shevchenko

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

[Jonathan Cameron]

On Tue, 13 Feb 2024 14:39:10 -0800
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".
> 
> Co-developed-by: Jonathan LoBue <jlobue10@gmail.com>

Take another look at how to use Co-developed in submitting-patches.rst
there are examples - key is that the author (From: in the email)
does not have a Co-developed-by line.

> 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>
> ---
> 
> Formatting fixes, removed duplicate header, and removed ACPI_PTR
> from previous submission.
> 
> Added an explanation of the duplicate ACPI identifier issue between
> devices using bmc150 and bmi323.
> 
>  drivers/iio/imu/bmi323/bmi323_i2c.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/drivers/iio/imu/bmi323/bmi323_i2c.c b/drivers/iio/imu/bmi323/bmi323_i2c.c
> index 20a8001b9956..22826a2efc6f 100644
> --- a/drivers/iio/imu/bmi323/bmi323_i2c.c
> +++ b/drivers/iio/imu/bmi323/bmi323_i2c.c
> @@ -93,6 +93,25 @@ static int bmi323_i2c_probe(struct i2c_client *i2c)
>  	return bmi323_core_probe(dev);
>  }
>  
> +/*
> + * The "BOSC0200" ACPI identifier used here in the bmi323 driver is not
> + * unique to bmi323 devices. The same "BOSC0200" identifier is found in
> + * devices using the bmc150 chip. This creates a conflict with duplicate
> + * ACPI identifiers which multiple drivers want to use. If a non-bmi323
> + * device starts to load with this "BOSC0200" ACPI match here, then the
> + * chip id check portion should fail and the driver should relinquish the
> + * device. 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[] = {
> +	{ "BOSC0200" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(acpi, bmi323_acpi_match);
> +
>  static const struct i2c_device_id bmi323_i2c_ids[] = {
>  	{ "bmi323" },
>  	{ }
> @@ -109,6 +128,7 @@ static struct i2c_driver bmi323_i2c_driver = {
>  	.driver = {
>  		.name = "bmi323",
>  		.of_match_table = bmi323_of_i2c_match,
> +		.acpi_match_table = bmi323_acpi_match,
>  	},
>  	.probe = bmi323_i2c_probe,
>  	.id_table = bmi323_i2c_ids,

Re: [PATCH 1/2] iio: accel: bmc150: ASUS ROG ALLY Abort Loading

[Jonathan LoBue]

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

On Monday, February 12, 2024 1:46:21 AM PST Andy Shevchenko wrote:
> Even if fixed (which has to be done anyway) it can be undone in old
> firmwares — there is no solution to make all affected users update
> firmware. Do we have real products on the market with the wrong ID (I
> assume we do)?

After some conversations with other devs today, we retested and confirmed
that the DMI quirks to abort loading of bmc150 is actually unnecessary.
There was some confusion among us about why they had tried that approach
in the past. The bmc150 driver does in fact start to load on ASUS ROG ALLY
with a "BOSC0200" ACPI match, but when it gets to the chip id check portion
it correctly aborts loading the driver for mismatched chip ID. This allows
the bmi323 driver to pick it up properly with the "BOSC0200" ACPI match
table match. Tested and confirmed working on my end with the submitted
v1 patch. The DMI quirks and any modifications to bmc150 driver has been
dropped. Thanks all for the feedback and help.

Best Regards,
Jon LoBue



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

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

[Jonathan LoBue]

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

From c65d1ef44d749958f02d2b9a50a0e788b4497854 Mon Sep 17 00:00:00 2001
From: Jonathan LoBue <jlobue10@gmail.com>
Date: Sat, 10 Feb 2024 12:31:54 -0800
Subject: [PATCH 2/2] iio: imu: bmi323: Add and enable ACPI Match Table

This patch adds the ACPI match table for ASUS ROG ALLY to load the bmi323
driver with an ACPI match of "BOSC0200".

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>
---
 drivers/iio/imu/bmi323/bmi323_i2c.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/iio/imu/bmi323/bmi323_i2c.c b/drivers/iio/imu/bmi323/bmi323_i2c.c
index 20a8001b9956..346ba2d1a169 100644
--- 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_client *i2c)
 	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_driver = {
 	.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,
-- 
2.43.0

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

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

[Jonathan Cameron]

On Sat, 10 Feb 2024 14:34:11 -0800
Jonathan LoBue <jlobue10@gmail.com> wrote:

> From c65d1ef44d749958f02d2b9a50a0e788b4497854 Mon Sep 17 00:00:00 2001
> From: Jonathan LoBue <jlobue10@gmail.com>
> Date: Sat, 10 Feb 2024 12:31:54 -0800
> Subject: [PATCH 2/2] iio: imu: bmi323: Add and enable ACPI Match Table
> 
> This patch adds the ACPI match table for ASUS ROG ALLY to load the bmi323
> driver with an ACPI match of "BOSC0200".
> 
> 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>

This approach is sustainable or maintainable. Let's wait to see
what people think of the suggestion I made of a wrapper driver that
is capable of identifying the device and causing the correct driver
to be loaded.

If nothing else this has no DMI type protections so if this one loads on
a board where it is a BMC150 compatible part we'll end up in the same
mess you were seeing just the other way around.

Jonathan



> ---
>  drivers/iio/imu/bmi323/bmi323_i2c.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/iio/imu/bmi323/bmi323_i2c.c b/drivers/iio/imu/bmi323/bmi323_i2c.c
> index 20a8001b9956..346ba2d1a169 100644
> --- 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_client *i2c)
>  	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_driver = {
>  	.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,

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

[Andy Shevchenko]

On Sun, Feb 11, 2024 at 12:34 AM Jonathan LoBue <jlobue10@gmail.com> wrote:
>
> From c65d1ef44d749958f02d2b9a50a0e788b4497854 Mon Sep 17 00:00:00 2001
> From: Jonathan LoBue <jlobue10@gmail.com>
> Date: Sat, 10 Feb 2024 12:31:54 -0800
> Subject: [PATCH 2/2] iio: imu: bmi323: Add and enable ACPI Match Table

Something went wrong with the email body.

> This patch adds the ACPI match table for ASUS ROG ALLY to load the bmi323
> driver with an ACPI match of "BOSC0200".

...

> +#include <linux/acpi.h>

It's not used. You don't need it as the proper one is already included...

>  #include <linux/i2c.h>
>  #include <linux/mod_devicetable.h>

...here ^^^.

>  #include <linux/module.h>

...

> +static const struct acpi_device_id bmi323_acpi_match[] = {
> +       {"BOSC0200"},
> +       { },

No comma for the terminator line.

> +};

...

> +               .acpi_match_table = ACPI_PTR(bmi323_acpi_match),

No ACPI_PTR() in new code. It's more problematic than helpful.

...


Above for your information for the future contributions, as I said in
the other patch comment, I think the better approach is to enumerate
from an external driver under the PDx86 realm.


-- 
With Best Regards,
Andy Shevchenko

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

[Jonathan LoBue]

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

On Sunday, February 11, 2024 9:08:59 AM PST Andy Shevchenko wrote:
> 
> No ACPI_PTR() in new code. It's more problematic than helpful.
> 
> Above for your information for the future contributions, as I said in
> the other patch comment, I think the better approach is to enumerate
> from an external driver under the PDx86 realm.

Thanks for the constructive feedback. I'm working to fix and re-send via
git send-email. I think the ACPI match table method should be okay and
seems pretty standard for a lot of devices. The problem in this case is
that the identifiers are not currently unique to each chip. This is something
that should be rectified with BOSCH and system builders and then in the
future, the ACPI match table(s) can be updated, and the aborting portion
of loading the bmc150 driver for ASUS ROG ALLY can be removed.

Best Regards,
Jon LoBue



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

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

[Andy Shevchenko]

On Mon, Feb 12, 2024 at 9:30 AM Jonathan LoBue <jlobue10@gmail.com> wrote:
> On Sunday, February 11, 2024 9:08:59 AM PST Andy Shevchenko wrote:

...

> > No ACPI_PTR() in new code. It's more problematic than helpful.
> >
> > Above for your information for the future contributions, as I said in
> > the other patch comment, I think the better approach is to enumerate
> > from an external driver under the PDx86 realm.
>
> Thanks for the constructive feedback.

You're welcome!

> I'm working to fix and re-send via
> git send-email. I think the ACPI match table method should be okay and
> seems pretty standard for a lot of devices. The problem in this case is
> that the identifiers are not currently unique to each chip.

Yes, that's why this portion is called "DMI quirk". And it does not
belong to the driver as such. In some cases we may have it inside the
driver, but here, I believe, and Hans can correct me, we may avoid
polluting the driver with a quirk.

> This is something
> that should be rectified with BOSCH and system builders and then in the
> future, the ACPI match table(s) can be updated, and the aborting portion
> of loading the bmc150 driver for ASUS ROG ALLY can be removed.

As I said in a reply to the other patch, it probably will stay forever.

-- 
With Best Regards,
Andy Shevchenko

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

[Jonathan LoBue]

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

On Monday, February 12, 2024 1:50:14 AM PST Andy Shevchenko wrote:
> 
> Yes, that's why this portion is called "DMI quirk". And it does not
> belong to the driver as such. In some cases we may have it inside the
> driver, but here, I believe, and Hans can correct me, we may avoid
> polluting the driver with a quirk.

After discussing with some devs who have worked on gyro software,
I have decided to abandon this "DMI quirk" approach in favor of fixing the
bmc150 chip id check to properly abandon the driver loading when
appropriate. This should be a far better approach and work for all
affected devices. I will need to re-write (and test) the first portion
of this 2 part patch in this manner before I re-submit. Thanks.

Best Regards,
Jon LoBue




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