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

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

[Jonathan LoBue]

[-- Attachment #1: Type: text/plain, Size: 372 bytes --]

From: Jonathan LoBue <jlobue10@gmail.com>

Changes since v3:
- Fixed missing summaries.

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 | 12 ++++++++++++
 drivers/iio/imu/bmi323/bmi323_i2c.c  | 20 ++++++++++++++++++++
 2 files changed, 32 insertions(+)
--
2.43.0

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

With the remarks below,
Reviewed-by: Andy Shevchenko <andy.shevchenko@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>
---
 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..df7d48a35e41 100644
--- a/drivers/iio/accel/bmc150-accel-i2c.c
+++ b/drivers/iio/accel/bmc150-accel-i2c.c
@@ -15,6 +15,19 @@
 
 #ifdef CONFIG_ACPI
 static const struct acpi_device_id bmc150_acpi_dual_accel_ids[] = {
+	/*
+ 	 * 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"},
 	{"DUAL250E"},
 	{ }
-- 
2.43.0

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

[Andy Shevchenko]

+Cc: documentation related people

On Thu, Feb 15, 2024 at 12:59 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,

This line is not for a commit message :-)

> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

> Signed-off-by: Jonathan LoBue <jlobue10@gmail.com>

Your SoB (as submitter) should go last, but heh, we have ambiguity in
the Submitting Patches.

https://www.kernel.org/doc/html/latest/process/submitting-patches.html#developer-s-certificate-of-origin-1-1
"...the first SoB entry signalling primary authorship of a single author."

https://www.kernel.org/doc/html/latest/process/submitting-patches.html#when-to-use-acked-by-cc-and-co-developed-by
"Notably, the last Signed-off-by: must always be that of the developer
submitting the patch."

These excerpts contradict each other when the author and submitter is
the same person and the amount of SoBs > 1.

> 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>


-- 
With Best Regards,
Andy Shevchenko

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

[Hans de Goede]

Hi,

On 2/14/24 23:59, Jonathan LoBue 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>
> 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>
> ---
>  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..df7d48a35e41 100644
> --- a/drivers/iio/accel/bmc150-accel-i2c.c
> +++ b/drivers/iio/accel/bmc150-accel-i2c.c
> @@ -15,6 +15,19 @@
>  
>  #ifdef CONFIG_ACPI
>  static const struct acpi_device_id bmc150_acpi_dual_accel_ids[] = {
> +	/*
> + 	 * 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"},
>  	{"DUAL250E"},
>  	{ }

Sorry for not noticing this sooner:

The BOSC0200 ACPI id is listed twice in this file.

The listing you are adding the comment to is for ACPI ids which are known
to be used in yoga-style (360° hinges) 2-in-1s which may have 2 accelerometers
1 in the bottom/keyboard half and 1 in the display half.

The bmc150_acpi_dual_accel_ids list indicates that this might be the case
and enables some extra code for this.

The actual listing of ACPI ids used to determine if the driver should
bind is the bmc150_accel_acpi_match[] list lower in the file and IMHO
that is the list where this comment should go.

Regards,

Hans

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

With the remarks below,
Reviewed-by: Andy Shevchenko <andy.shevchenko@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>
---
 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