[PATCH v5 0/2] Add bmi323 support for ASUS ROG ALLY

[PATCH v5 0/2] Add bmi323 support for ASUS ROG ALLY

[J Lo]

From: Jonathan LoBue <jlobue10@gmail.com>

Changes since v4:
- Fixed comment location in bmc150.
- Fixed signed off by portion.

Jonathan LoBue (2):
  iio: accel: bmc150: Duplicate ACPI entries
  iio: imu: bmi323: Add and enable ACPI Match Table

 drivers/iio/accel/bmc150-accel-i2c.c | 13 +++++++++++++
 drivers/iio/imu/bmi323/bmi323_i2c.c  | 20 ++++++++++++++++++++
 2 files changed, 33 insertions(+)
--
2.43.0

[PATCH v5 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.

Reviewed-by: Andy Shevchenko <andy.shevchenko@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>
Signed-off-by: Jonathan LoBue <jlobue10@gmail.com>
---
 drivers/iio/accel/bmc150-accel-i2c.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/iio/accel/bmc150-accel-i2c.c b/drivers/iio/accel/bmc150-accel-i2c.c
index ee1ba134ad42..353271e23235 100644
--- a/drivers/iio/accel/bmc150-accel-i2c.c
+++ b/drivers/iio/accel/bmc150-accel-i2c.c
@@ -224,6 +224,19 @@ static const struct acpi_device_id bmc150_accel_acpi_match[] = {
 	{"BMA250E"},
 	{"BMC150A"},
 	{"BMI055A"},
+	/*
+ 	 * 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) because the chip IDs received (via i2c) are unique between
+ 	 * bmc150 and bmi323 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, and
+ 	 * other devices using the bmi323 chip.
+ 	 */
 	{"BOSC0200"},
 	{"BSBA0150"},
 	{"DUAL250E"},
-- 
2.43.0

[PATCH v5 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", and a comment about duplicate
ACPI identifiers between devices using the bmc150 and bmi323 chips.

Reviewed-by: Andy Shevchenko <andy.shevchenko@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>
Signed-off-by: Jonathan LoBue <jlobue10@gmail.com>
---
 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..f7550503e47e 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);
 }
 
+static const struct acpi_device_id bmi323_acpi_match[] = {
+	/*
+ 	 * The "BOSC0200" identifier used here is not unique to bmi323 devices.
+ 	 * The same "BOSC0200" identifier is found in the ACPI tables of 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 because the chip IDs received (via i2c) are
+ 	 * unique between bmc150 and bmi323 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.
+ 	 */
+	{ "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 v5 0/2] Add bmi323 support for ASUS ROG ALLY

[Jonathan Cameron]

On Thu, 15 Feb 2024 10:19:52 -0800
J Lo <jlobue10@gmail.com> wrote:

> From: Jonathan LoBue <jlobue10@gmail.com>

Hi Jonathan

Cover letter should always include at least a short overview of what
the patch is doing.

Long term this solution may be a pain to maintain.
The reasoning is the DT path where we have moved over time to allow
for fallback compatibles (same concept exists in ACPI even if it is
little used) to be used even if we don't recognise a ID read from
the chip.  The intent being to allow old kernels to work with new
devices where they really are backwards compatible.

If that gets fixed in these drivers, we will have to explicitly
exclude ACPI IDs.

Hopefully we'll pick up such issues in review though so this should be fine.

I'd like input from Hans though on whether this solution of duplicating
the IDs generally works out longer term and is appropriate here.

Jonathan


> 
> Changes since v4:
> - Fixed comment location in bmc150.
> - Fixed signed off by portion.
> 
> Jonathan LoBue (2):
>   iio: accel: bmc150: Duplicate ACPI entries
>   iio: imu: bmi323: Add and enable ACPI Match Table
> 
>  drivers/iio/accel/bmc150-accel-i2c.c | 13 +++++++++++++
>  drivers/iio/imu/bmi323/bmi323_i2c.c  | 20 ++++++++++++++++++++
>  2 files changed, 33 insertions(+)
> --
> 2.43.0

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

[Jonathan Cameron]

On Thu, 15 Feb 2024 10:24:25 -0800
Jonathan LoBue <jlobue10@gmail.com> wrote:

> This patch adds a description of the duplicate ACPI identifier issue
> between devices using bmc150 and bmi323.
Title of patch doesn't reflect what is going on.

If there are no other changes needed I can tweak that whilst applying
to

iio: accel: bmc150: Document duplicate ACPI entries with bmi323 driver

> 
> Reviewed-by: Andy Shevchenko <andy.shevchenko@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>
> Signed-off-by: Jonathan LoBue <jlobue10@gmail.com>
> ---
>  drivers/iio/accel/bmc150-accel-i2c.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/drivers/iio/accel/bmc150-accel-i2c.c b/drivers/iio/accel/bmc150-accel-i2c.c
> index ee1ba134ad42..353271e23235 100644
> --- a/drivers/iio/accel/bmc150-accel-i2c.c
> +++ b/drivers/iio/accel/bmc150-accel-i2c.c
> @@ -224,6 +224,19 @@ static const struct acpi_device_id bmc150_accel_acpi_match[] = {
>  	{"BMA250E"},
>  	{"BMC150A"},
>  	{"BMI055A"},
> +	/*
> + 	 * 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) because the chip IDs received (via i2c) are unique between
> + 	 * bmc150 and bmi323 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, and
> + 	 * other devices using the bmi323 chip.
> + 	 */
>  	{"BOSC0200"},
>  	{"BSBA0150"},
>  	{"DUAL250E"},

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

[Jonathan Cameron]

On Thu, 15 Feb 2024 10:24:41 -0800
Jonathan LoBue <jlobue10@gmail.com> wrote:

Add ACPI Match Table

is enough - would be odd if we didn't use it ;)

This looks fine to me, but I'm not the expert in work arounds for this
sort of ACPI issue. 

I'm too used to the nice world of servers where we fix wrong firmware
(and this sort of change is always rejected by upstream). 

> This patch adds the ACPI match table for ASUS ROG ALLY to load the bmi323
> driver with an ACPI match of "BOSC0200", and a comment about duplicate
> ACPI identifiers between devices using the bmc150 and bmi323 chips.
> 
> Reviewed-by: Andy Shevchenko <andy.shevchenko@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>
> Signed-off-by: Jonathan LoBue <jlobue10@gmail.com>
> ---
>  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..f7550503e47e 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);
>  }
>  
> +static const struct acpi_device_id bmi323_acpi_match[] = {
> +	/*
> + 	 * The "BOSC0200" identifier used here is not unique to bmi323 devices.
> + 	 * The same "BOSC0200" identifier is found in the ACPI tables of 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 because the chip IDs received (via i2c) are
> + 	 * unique between bmc150 and bmi323 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.
> + 	 */
> +	{ "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 v5 0/2] Add bmi323 support for ASUS ROG ALLY

[Hans de Goede]

Hi Jonathan,

On 2/16/24 12:35, Jonathan Cameron wrote:
> On Thu, 15 Feb 2024 10:19:52 -0800
> J Lo <jlobue10@gmail.com> wrote:
> 
>> From: Jonathan LoBue <jlobue10@gmail.com>
> 
> Hi Jonathan
> 
> Cover letter should always include at least a short overview of what
> the patch is doing.
> 
> Long term this solution may be a pain to maintain.
> The reasoning is the DT path where we have moved over time to allow
> for fallback compatibles (same concept exists in ACPI even if it is
> little used) to be used even if we don't recognise a ID read from
> the chip.  The intent being to allow old kernels to work with new
> devices where they really are backwards compatible.
> 
> If that gets fixed in these drivers, we will have to explicitly
> exclude ACPI IDs.
> 
> Hopefully we'll pick up such issues in review though so this should be fine.
> 
> I'd like input from Hans though on whether this solution of duplicating
> the IDs generally works out longer term and is appropriate here.

The BOSC0200 ACPI ID is used in a lot of devices and the ACPI
tables of these devices are not under our control. So we really
have no other option.

Having 2 different drivers match/bind to the same (ACPI or other)
id/device is not unheard of. As long as the probe() method then figures
out it is not the right device and cleanly exits then this is fine.

I'll run a test with patch 2/2 + the bmi323 driver enabled
on a device with a BOSC0200 ACPI id which does actually need
the bmc150 driver to make sure that the bmi323 driver properly
refuses to bind there.

I do see that both drivers write to a reset-register before reading
the id register and those registers are different ...

Looking at the registers used for reset then on the bmc150 it
seems that the bmi323 code is writing the last bytes of the fifo
which should be fine.

And when the bmc150 code is trying to reset a bmi323 it is
writing to the BMI323_FEAT_IO_STATUS_REG. Since the bmi323
driver does a reset itself and programs that register
during init I guess that should be fine to since the value
written by bmc150's probe() will be overwritten.

I'll get back to with test results of letting the bmi323 driver
probe a BOSC0200 (*) device, before the bmc150 driver probes
it and see if things still work then.

Regards,

Hans


*) which will typically be a BMA250E




>> Changes since v4:
>> - Fixed comment location in bmc150.
>> - Fixed signed off by portion.
>>
>> Jonathan LoBue (2):
>>   iio: accel: bmc150: Duplicate ACPI entries
>>   iio: imu: bmi323: Add and enable ACPI Match Table
>>
>>  drivers/iio/accel/bmc150-accel-i2c.c | 13 +++++++++++++
>>  drivers/iio/imu/bmi323/bmi323_i2c.c  | 20 ++++++++++++++++++++
>>  2 files changed, 33 insertions(+)
>> --
>> 2.43.0
> 

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

[Jonathan LoBue]

Would you prefer if I make these small changes in a version 6?

Best Regards,
Jon LoBue

On Fri, Feb 16, 2024 at 3:37 AM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Thu, 15 Feb 2024 10:24:25 -0800
> Jonathan LoBue <jlobue10@gmail.com> wrote:
>
> > This patch adds a description of the duplicate ACPI identifier issue
> > between devices using bmc150 and bmi323.
> Title of patch doesn't reflect what is going on.
>
> If there are no other changes needed I can tweak that whilst applying
> to
>
> iio: accel: bmc150: Document duplicate ACPI entries with bmi323 driver
>
> >
> > Reviewed-by: Andy Shevchenko <andy.shevchenko@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>
> > Signed-off-by: Jonathan LoBue <jlobue10@gmail.com>
> > ---
> >  drivers/iio/accel/bmc150-accel-i2c.c | 13 +++++++++++++
> >  1 file changed, 13 insertions(+)
> >
> > diff --git a/drivers/iio/accel/bmc150-accel-i2c.c b/drivers/iio/accel/bmc150-accel-i2c.c
> > index ee1ba134ad42..353271e23235 100644
> > --- a/drivers/iio/accel/bmc150-accel-i2c.c
> > +++ b/drivers/iio/accel/bmc150-accel-i2c.c
> > @@ -224,6 +224,19 @@ static const struct acpi_device_id bmc150_accel_acpi_match[] = {
> >       {"BMA250E"},
> >       {"BMC150A"},
> >       {"BMI055A"},
> > +     /*
> > +      * 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) because the chip IDs received (via i2c) are unique between
> > +      * bmc150 and bmi323 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, and
> > +      * other devices using the bmi323 chip.
> > +      */
> >       {"BOSC0200"},
> >       {"BSBA0150"},
> >       {"DUAL250E"},
>

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

[Jonathan Cameron]

On Fri, 16 Feb 2024 06:30:10 -0800
Jonathan LoBue <jlobue10@gmail.com> wrote:

> Would you prefer if I make these small changes in a version 6?

Sure, saves me a few seconds ;)  We will be waiting for Hans to have
a chance to test anyway.

Jonathan

> 
> Best Regards,
> Jon LoBue
> 
> On Fri, Feb 16, 2024 at 3:37 AM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Thu, 15 Feb 2024 10:24:25 -0800
> > Jonathan LoBue <jlobue10@gmail.com> wrote:
> >  
> > > This patch adds a description of the duplicate ACPI identifier issue
> > > between devices using bmc150 and bmi323.  
> > Title of patch doesn't reflect what is going on.
> >
> > If there are no other changes needed I can tweak that whilst applying
> > to
> >
> > iio: accel: bmc150: Document duplicate ACPI entries with bmi323 driver
> >  
> > >
> > > Reviewed-by: Andy Shevchenko <andy.shevchenko@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>
> > > Signed-off-by: Jonathan LoBue <jlobue10@gmail.com>
> > > ---
> > >  drivers/iio/accel/bmc150-accel-i2c.c | 13 +++++++++++++
> > >  1 file changed, 13 insertions(+)
> > >
> > > diff --git a/drivers/iio/accel/bmc150-accel-i2c.c b/drivers/iio/accel/bmc150-accel-i2c.c
> > > index ee1ba134ad42..353271e23235 100644
> > > --- a/drivers/iio/accel/bmc150-accel-i2c.c
> > > +++ b/drivers/iio/accel/bmc150-accel-i2c.c
> > > @@ -224,6 +224,19 @@ static const struct acpi_device_id bmc150_accel_acpi_match[] = {
> > >       {"BMA250E"},
> > >       {"BMC150A"},
> > >       {"BMI055A"},
> > > +     /*
> > > +      * 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) because the chip IDs received (via i2c) are unique between
> > > +      * bmc150 and bmi323 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, and
> > > +      * other devices using the bmi323 chip.
> > > +      */
> > >       {"BOSC0200"},
> > >       {"BSBA0150"},
> > >       {"DUAL250E"},  
> >