| Message ID | 4956451.31r3eYUQgx@nobara-ally-pc (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/2] iio: accel: bmc150: ASUS ROG ALLY Abort Loading | expand |
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,
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.
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
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.
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
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,