Message ID | 20240213223806.27056-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: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"},
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
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.
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. >
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"},