| Message ID | 20240213223910.27127-1-jlobue10@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2,1/2] iio: accel: bmc150: Duplicate ACPI entries | expand |
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" }, > + { } > +};
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
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
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,
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
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!
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,