diff mbox series

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

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

Commit Message

Jonathan LoBue Feb. 13, 2024, 10:38 p.m. UTC 
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(+)

Comments

Andy Shevchenko Feb. 14, 2024, 9:35 a.m. UTC | #1 
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"},
Jonathan LoBue Feb. 14, 2024, 3:07 p.m. UTC | #2 
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
Andy Shevchenko Feb. 14, 2024, 3:39 p.m. UTC | #3 
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.
Jonathan Cameron Feb. 14, 2024, 4:16 p.m. UTC | #4 
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 mbox series

Patch

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"},