[PATCH 1/3] iio: accel: bmc150: Improve ACPI enumeration support

[PATCH 1/3] iio: accel: bmc150: Improve ACPI enumeration support

[Hans de Goede]

Hi All,

This series improves support for ACPI enumeration of bmc150 accels
described by an ACPI node with an ACPI hw-id of BOSC0200:

1. Add support for nodes which describe 2 acceleromers in a single node,
fixing: https://bugzilla.kernel.org/show_bug.cgi?id=198671

2. Add support for reading the mount-matrix from the ACPI node.

This is done in patches 2 - 3, patch 1 is a trivial cleanup which I noticed
could be done while working on this.

Patch 2 is based on an earlier patch for this from Jeremy Cline:
https://lore.kernel.org/r/010001602cf53153-39ad69f1-1b39-4e6d-a748-9455a16c2fbd-000000@email.amazonses.com

That patch was put on hold because normally ACPI nodes which describe
multiple i2c-clients in a single node are handled by:
drivers/platform/x86/i2c-multi-instantiate.c

Which I tried to do at first, but as explained in the commit msg
of the updated patch, that is not possible in this special case
(because it would cause userspace breakage due to the modalias changing).

Regards,

Hans

[PATCH 1/3] iio: accel: bmc150: Removed unused bmc150_accel_dat irq member

[Hans de Goede]

The bmc150_accel_dat struct irq member is only ever used inside
bmc150_accel_core_probe, drop it and just use the function argument
directly.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/iio/accel/bmc150-accel-core.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/iio/accel/bmc150-accel-core.c b/drivers/iio/accel/bmc150-accel-core.c
index 48435865fdaf..088716d55855 100644
--- a/drivers/iio/accel/bmc150-accel-core.c
+++ b/drivers/iio/accel/bmc150-accel-core.c
@@ -183,7 +183,6 @@ enum bmc150_accel_trigger_id {
 
 struct bmc150_accel_data {
 	struct regmap *regmap;
-	int irq;
 	struct bmc150_accel_interrupt interrupts[BMC150_ACCEL_INTERRUPTS];
 	struct bmc150_accel_trigger triggers[BMC150_ACCEL_TRIGGERS];
 	struct mutex mutex;
@@ -1568,7 +1567,6 @@ int bmc150_accel_core_probe(struct device *dev, struct regmap *regmap, int irq,
 
 	data = iio_priv(indio_dev);
 	dev_set_drvdata(dev, indio_dev);
-	data->irq = irq;
 
 	data->regmap = regmap;
 
@@ -1599,9 +1597,8 @@ int bmc150_accel_core_probe(struct device *dev, struct regmap *regmap, int irq,
 		return ret;
 	}
 
-	if (data->irq > 0) {
-		ret = devm_request_threaded_irq(
-						dev, data->irq,
+	if (irq > 0) {
+		ret = devm_request_threaded_irq(dev, irq,
 						bmc150_accel_irq_handler,
 						bmc150_accel_irq_thread_handler,
 						IRQF_TRIGGER_RISING,
-- 
2.28.0

[PATCH 2/3] iio: accel: bmc150: Check for a second ACPI device for BOSC0200

[Hans de Goede]

From: Jeremy Cline <jeremy@jcline.org>

Some BOSC0200 acpi_device-s describe two accelerometers in a single ACPI
device. Normally we would handle this by letting the special
drivers/platform/x86/i2c-multi-instantiate.c driver handle the BOSC0200
ACPI id and let it instantiate 2 bmc150_accel type i2c_client-s for us.

But doing so changes the modalias for the first accelerometer
(which is already supported and used on many devices) from
acpi:BOSC0200 to i2c:bmc150_accel. The modalias is not only used
to load the driver, but is also used by hwdb matches in
/lib/udev/hwdb.d/60-sensor.hwdb which provide a mountmatrix to
userspace by setting the ACCEL_MOUNT_MATRIX udev property.

Switching the handling of the BOSC0200 over to i2c-multi-instantiate.c
will break the hwdb matches causing the ACCEL_MOUNT_MATRIX udev prop
to no longer be set. So switching over to i2c-multi-instantiate.c is
not an option.

Changes by Hans de Goede:
-Add explanation to the commit message why i2c-multi-instantiate.c
 cannot be used
-Also set the dev_name, fwnode and irq i2c_board_info struct members
 for the 2nd client

BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=198671
Signed-off-by: Jeremy Cline <jeremy@jcline.org>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/iio/accel/bmc150-accel-core.c | 21 ++++++++++++++++
 drivers/iio/accel/bmc150-accel-i2c.c  | 35 +++++++++++++++++++++++++--
 drivers/iio/accel/bmc150-accel.h      |  2 ++
 3 files changed, 56 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/accel/bmc150-accel-core.c b/drivers/iio/accel/bmc150-accel-core.c
index 088716d55855..2976aefad89b 100644
--- a/drivers/iio/accel/bmc150-accel-core.c
+++ b/drivers/iio/accel/bmc150-accel-core.c
@@ -203,6 +203,7 @@ struct bmc150_accel_data {
 	int ev_enable_state;
 	int64_t timestamp, old_timestamp; /* Only used in hw fifo mode. */
 	const struct bmc150_accel_chip_info *chip_info;
+	struct i2c_client *second_device;
 	struct iio_mount_matrix orientation;
 };
 
@@ -1659,6 +1660,26 @@ int bmc150_accel_core_probe(struct device *dev, struct regmap *regmap, int irq,
 }
 EXPORT_SYMBOL_GPL(bmc150_accel_core_probe);
 
+struct i2c_client *bmc150_get_second_device(struct i2c_client *client)
+{
+	struct bmc150_accel_data *data = i2c_get_clientdata(client);
+
+	if (!data)
+		return NULL;
+
+	return data->second_device;
+}
+EXPORT_SYMBOL_GPL(bmc150_get_second_device);
+
+void bmc150_set_second_device(struct i2c_client *client)
+{
+	struct bmc150_accel_data *data = i2c_get_clientdata(client);
+
+	if (data)
+		data->second_device = client;
+}
+EXPORT_SYMBOL_GPL(bmc150_set_second_device);
+
 int bmc150_accel_core_remove(struct device *dev)
 {
 	struct iio_dev *indio_dev = dev_get_drvdata(dev);
diff --git a/drivers/iio/accel/bmc150-accel-i2c.c b/drivers/iio/accel/bmc150-accel-i2c.c
index 06021c8685a7..117184159bb6 100644
--- a/drivers/iio/accel/bmc150-accel-i2c.c
+++ b/drivers/iio/accel/bmc150-accel-i2c.c
@@ -29,6 +29,8 @@ static int bmc150_accel_probe(struct i2c_client *client,
 		i2c_check_functionality(client->adapter, I2C_FUNC_I2C) ||
 		i2c_check_functionality(client->adapter,
 					I2C_FUNC_SMBUS_READ_I2C_BLOCK);
+	struct acpi_device *adev = ACPI_COMPANION(&client->dev);
+	int ret;
 
 	regmap = devm_regmap_init_i2c(client, &bmc150_regmap_conf);
 	if (IS_ERR(regmap)) {
@@ -39,12 +41,41 @@ static int bmc150_accel_probe(struct i2c_client *client,
 	if (id)
 		name = id->name;
 
-	return bmc150_accel_core_probe(&client->dev, regmap, client->irq, name,
-				       block_supported);
+	ret = bmc150_accel_core_probe(&client->dev, regmap, client->irq, name, block_supported);
+	if (ret)
+		return ret;
+
+	/*
+	 * Some BOSC0200 acpi_devices describe 2 accelerometers in a single ACPI
+	 * device, try instantiating a second i2c_client for an I2cSerialBusV2
+	 * ACPI resource with index 1. The !id check avoids recursion when
+	 * bmc150_accel_probe() gets called for the second client.
+	 */
+	if (!id && adev && strcmp(acpi_device_hid(adev), "BOSC0200") == 0) {
+		struct i2c_board_info board_info = {
+			.type = "bmc150_accel",
+			/* The 2nd accel sits in the base of 2-in-1s */
+			.dev_name = "BOSC0200:base",
+			.fwnode = client->dev.fwnode,
+			.irq = -ENOENT,
+		};
+		struct i2c_client *second_dev;
+
+		second_dev = i2c_acpi_new_device(&client->dev, 1, &board_info);
+		if (!IS_ERR(second_dev))
+			bmc150_set_second_device(second_dev);
+	}
+
+	return 0;
 }
 
 static int bmc150_accel_remove(struct i2c_client *client)
 {
+	struct i2c_client *second_dev = bmc150_get_second_device(client);
+
+	if (second_dev)
+		i2c_unregister_device(second_dev);
+
 	return bmc150_accel_core_remove(&client->dev);
 }
 
diff --git a/drivers/iio/accel/bmc150-accel.h b/drivers/iio/accel/bmc150-accel.h
index ae6118ae11b1..6e965a3ca322 100644
--- a/drivers/iio/accel/bmc150-accel.h
+++ b/drivers/iio/accel/bmc150-accel.h
@@ -16,6 +16,8 @@ enum {
 int bmc150_accel_core_probe(struct device *dev, struct regmap *regmap, int irq,
 			    const char *name, bool block_supported);
 int bmc150_accel_core_remove(struct device *dev);
+struct i2c_client *bmc150_get_second_device(struct i2c_client *second_device);
+void bmc150_set_second_device(struct i2c_client *second_device);
 extern const struct dev_pm_ops bmc150_accel_pm_ops;
 extern const struct regmap_config bmc150_regmap_conf;
 
-- 
2.28.0

[PATCH 3/3] iio: accel: bmc150: Get mount-matrix from ACPI

[Hans de Goede]

bmc150 accelerometers with an ACPI hardware-id of BOSC0200 have an ACPI
method providing their mount-matrix, add support for retrieving this.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/iio/accel/bmc150-accel-core.c | 82 +++++++++++++++++++++++++--
 1 file changed, 78 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/accel/bmc150-accel-core.c b/drivers/iio/accel/bmc150-accel-core.c
index 2976aefad89b..2ce65d509f93 100644
--- a/drivers/iio/accel/bmc150-accel-core.c
+++ b/drivers/iio/accel/bmc150-accel-core.c
@@ -410,6 +410,78 @@ static int bmc150_accel_set_power_state(struct bmc150_accel_data *data, bool on)
 }
 #endif
 
+#ifdef CONFIG_ACPI
+static bool bmc150_apply_acpi_orientation(struct device *dev,
+					  struct iio_mount_matrix *orientation)
+{
+	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
+	struct acpi_device *adev = ACPI_COMPANION(dev);
+	union acpi_object *obj, *elements;
+	char *name, *alt_name, *str;
+	acpi_status status;
+	int i, j, val[3];
+
+	if (!adev || !acpi_dev_hid_uid_match(adev, "BOSC0200", NULL))
+		return false;
+
+	if (strcmp(dev_name(dev), "i2c-BOSC0200:base") == 0)
+		alt_name = "ROMK";
+	else
+		alt_name = "ROMS";
+
+	if (acpi_has_method(adev->handle, "ROTM"))
+		name = "ROTM";
+	else if (acpi_has_method(adev->handle, alt_name))
+		name = alt_name;
+	else
+		return false;
+
+	status = acpi_evaluate_object(adev->handle, name, NULL, &buffer);
+	if (ACPI_FAILURE(status)) {
+		dev_warn(dev, "Failed to get ACPI mount matrix: %d\n", status);
+		return false;
+	}
+
+	obj = buffer.pointer;
+	if (obj->type != ACPI_TYPE_PACKAGE || obj->package.count != 3)
+		goto unknown_format;
+
+	elements = obj->package.elements;
+	for (i = 0; i < 3; i++) {
+		if (elements[i].type != ACPI_TYPE_STRING)
+			goto unknown_format;
+
+		str = elements[i].string.pointer;
+		if (sscanf(str, "%d %d %d", &val[0], &val[1], &val[2]) != 3)
+			goto unknown_format;
+
+		for (j = 0; j < 3; j++) {
+			switch (val[j]) {
+			case -1: str = "-1"; break;
+			case 0:  str = "0";  break;
+			case 1:  str = "1";  break;
+			default: goto unknown_format;
+			}
+			orientation->rotation[i * 3 + j] = str;
+		}
+	}
+
+	kfree(buffer.pointer);
+	return true;
+
+unknown_format:
+	dev_warn(dev, "Unknown ACPI mount matrix format, ignoring\n");
+	kfree(buffer.pointer);
+	return false;
+}
+#else
+static bool bmc150_apply_acpi_orientation(struct device *dev,
+					  struct iio_mount_matrix *orientation)
+{
+	return false;
+}
+#endif
+
 static const struct bmc150_accel_interrupt_info {
 	u8 map_reg;
 	u8 map_bitmask;
@@ -1571,10 +1643,12 @@ int bmc150_accel_core_probe(struct device *dev, struct regmap *regmap, int irq,
 
 	data->regmap = regmap;
 
-	ret = iio_read_mount_matrix(dev, "mount-matrix",
-				     &data->orientation);
-	if (ret)
-		return ret;
+	if (!bmc150_apply_acpi_orientation(dev, &data->orientation)) {
+		ret = iio_read_mount_matrix(dev, "mount-matrix",
+					     &data->orientation);
+		if (ret)
+			return ret;
+	}
 
 	ret = bmc150_accel_chip_init(data);
 	if (ret < 0)
-- 
2.28.0

Re: [PATCH 1/3] iio: accel: bmc150: Improve ACPI enumeration support

[Andy Shevchenko]

On Wed, Nov 25, 2020 at 10:37 AM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi All,
>
> This series improves support for ACPI enumeration of bmc150 accels
> described by an ACPI node with an ACPI hw-id of BOSC0200:
>
> 1. Add support for nodes which describe 2 acceleromers in a single node,

accelerometers

> fixing: https://bugzilla.kernel.org/show_bug.cgi?id=198671
>
> 2. Add support for reading the mount-matrix from the ACPI node.
>
> This is done in patches 2 - 3, patch 1 is a trivial cleanup which I noticed
> could be done while working on this.
>
> Patch 2 is based on an earlier patch for this from Jeremy Cline:
> https://lore.kernel.org/r/010001602cf53153-39ad69f1-1b39-4e6d-a748-9455a16c2fbd-000000@email.amazonses.com
>
> That patch was put on hold because normally ACPI nodes which describe
> multiple i2c-clients in a single node are handled by:
> drivers/platform/x86/i2c-multi-instantiate.c
>
> Which I tried to do at first, but as explained in the commit msg
> of the updated patch, that is not possible in this special case
> (because it would cause userspace breakage due to the modalias changing).

This is marked as patch 1?!
Usually --cover-letter produces a correct template...

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/3] iio: accel: bmc150: Improve ACPI enumeration support

[Hans de Goede]

Hi,

On 11/25/20 11:41 AM, Andy Shevchenko wrote:
> On Wed, Nov 25, 2020 at 10:37 AM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> Hi All,
>>
>> This series improves support for ACPI enumeration of bmc150 accels
>> described by an ACPI node with an ACPI hw-id of BOSC0200:
>>
>> 1. Add support for nodes which describe 2 acceleromers in a single node,
> 
> accelerometers
> 
>> fixing: https://bugzilla.kernel.org/show_bug.cgi?id=198671
>>
>> 2. Add support for reading the mount-matrix from the ACPI node.
>>
>> This is done in patches 2 - 3, patch 1 is a trivial cleanup which I noticed
>> could be done while working on this.
>>
>> Patch 2 is based on an earlier patch for this from Jeremy Cline:
>> https://lore.kernel.org/r/010001602cf53153-39ad69f1-1b39-4e6d-a748-9455a16c2fbd-000000@email.amazonses.com
>>
>> That patch was put on hold because normally ACPI nodes which describe
>> multiple i2c-clients in a single node are handled by:
>> drivers/platform/x86/i2c-multi-instantiate.c
>>
>> Which I tried to do at first, but as explained in the commit msg
>> of the updated patch, that is not possible in this special case
>> (because it would cause userspace breakage due to the modalias changing).
> 
> This is marked as patch 1?!

Yes my bad, sorry.

> Usually --cover-letter produces a correct template...

I use --compose, time to switch to --cover-letter instead I guess.

Regards,

Hans

Re: [PATCH 2/3] iio: accel: bmc150: Check for a second ACPI device for BOSC0200

[Andy Shevchenko]

On Wed, Nov 25, 2020 at 10:37 AM Hans de Goede <hdegoede@redhat.com> wrote:
>
> From: Jeremy Cline <jeremy@jcline.org>
>
> Some BOSC0200 acpi_device-s describe two accelerometers in a single ACPI
> device. Normally we would handle this by letting the special
> drivers/platform/x86/i2c-multi-instantiate.c driver handle the BOSC0200
> ACPI id and let it instantiate 2 bmc150_accel type i2c_client-s for us.
>
> But doing so changes the modalias for the first accelerometer
> (which is already supported and used on many devices) from
> acpi:BOSC0200 to i2c:bmc150_accel. The modalias is not only used
> to load the driver, but is also used by hwdb matches in
> /lib/udev/hwdb.d/60-sensor.hwdb which provide a mountmatrix to
> userspace by setting the ACCEL_MOUNT_MATRIX udev property.
>
> Switching the handling of the BOSC0200 over to i2c-multi-instantiate.c
> will break the hwdb matches causing the ACCEL_MOUNT_MATRIX udev prop
> to no longer be set. So switching over to i2c-multi-instantiate.c is
> not an option.

I'm wondering if we can meanwhile update hwdb to support
i2c-multi-instantiate cases in the future and in a few years switch to
it.

> Changes by Hans de Goede:
> -Add explanation to the commit message why i2c-multi-instantiate.c
>  cannot be used
> -Also set the dev_name, fwnode and irq i2c_board_info struct members
>  for the 2nd client

...

> +       ret = bmc150_accel_core_probe(&client->dev, regmap, client->irq, name, block_supported);
> +       if (ret)
> +               return ret;
> +
> +       /*
> +        * Some BOSC0200 acpi_devices describe 2 accelerometers in a single ACPI
> +        * device, try instantiating a second i2c_client for an I2cSerialBusV2
> +        * ACPI resource with index 1. The !id check avoids recursion when
> +        * bmc150_accel_probe() gets called for the second client.
> +        */

> +       if (!id && adev && strcmp(acpi_device_hid(adev), "BOSC0200") == 0) {

> +               struct i2c_board_info board_info = {
> +                       .type = "bmc150_accel",
> +                       /* The 2nd accel sits in the base of 2-in-1s */

> +                       .dev_name = "BOSC0200:base",

Hmm... Can we use '.' (dot) rather than ':' (colon) to avoid confusion
with ACPI device naming schema? (Or was it on purpose?)
And this seems to be the only device in the system, second as this is
not allowed as far as I understand. Right? But theoretically I can
create an ACPI SSDT with quite similar excerpt and sensor and
enumerate it via ConfigFS (I understand that is quite unlikely).

> +                       .fwnode = client->dev.fwnode,
> +                       .irq = -ENOENT,
> +               };
> +               struct i2c_client *second_dev;
> +
> +               second_dev = i2c_acpi_new_device(&client->dev, 1, &board_info);
> +               if (!IS_ERR(second_dev))
> +                       bmc150_set_second_device(second_dev);
> +       }

...

>  static int bmc150_accel_remove(struct i2c_client *client)
>  {
> +       struct i2c_client *second_dev = bmc150_get_second_device(client);

> +       if (second_dev)

Redundant.

> +               i2c_unregister_device(second_dev);
> +
>         return bmc150_accel_core_remove(&client->dev);
>  }

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 3/3] iio: accel: bmc150: Get mount-matrix from ACPI

[Andy Shevchenko]

On Wed, Nov 25, 2020 at 10:37 AM Hans de Goede <hdegoede@redhat.com> wrote:
>
> bmc150 accelerometers with an ACPI hardware-id of BOSC0200 have an ACPI
> method providing their mount-matrix, add support for retrieving this.

...

> +       if (strcmp(dev_name(dev), "i2c-BOSC0200:base") == 0)
> +               alt_name = "ROMK";
> +       else
> +               alt_name = "ROMS";
> +
> +       if (acpi_has_method(adev->handle, "ROTM"))
> +               name = "ROTM";

My gosh, it's a third method of this...

...

> +       elements = obj->package.elements;
> +       for (i = 0; i < 3; i++) {
> +               if (elements[i].type != ACPI_TYPE_STRING)
> +                       goto unknown_format;
> +
> +               str = elements[i].string.pointer;
> +               if (sscanf(str, "%d %d %d", &val[0], &val[1], &val[2]) != 3)
> +                       goto unknown_format;
> +
> +               for (j = 0; j < 3; j++) {
> +                       switch (val[j]) {
> +                       case -1: str = "-1"; break;
> +                       case 0:  str = "0";  break;
> +                       case 1:  str = "1";  break;
> +                       default: goto unknown_format;
> +                       }
> +                       orientation->rotation[i * 3 + j] = str;
> +               }
> +       }

I'm wondering if we can come up with some common code out of this and
existing apply_acpi_orientation().

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/3] iio: accel: bmc150: Check for a second ACPI device for BOSC0200

[Hans de Goede]

Hi,

On 11/25/20 11:55 AM, Andy Shevchenko wrote:
> On Wed, Nov 25, 2020 at 10:37 AM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> From: Jeremy Cline <jeremy@jcline.org>
>>
>> Some BOSC0200 acpi_device-s describe two accelerometers in a single ACPI
>> device. Normally we would handle this by letting the special
>> drivers/platform/x86/i2c-multi-instantiate.c driver handle the BOSC0200
>> ACPI id and let it instantiate 2 bmc150_accel type i2c_client-s for us.
>>
>> But doing so changes the modalias for the first accelerometer
>> (which is already supported and used on many devices) from
>> acpi:BOSC0200 to i2c:bmc150_accel. The modalias is not only used
>> to load the driver, but is also used by hwdb matches in
>> /lib/udev/hwdb.d/60-sensor.hwdb which provide a mountmatrix to
>> userspace by setting the ACCEL_MOUNT_MATRIX udev property.
>>
>> Switching the handling of the BOSC0200 over to i2c-multi-instantiate.c
>> will break the hwdb matches causing the ACCEL_MOUNT_MATRIX udev prop
>> to no longer be set. So switching over to i2c-multi-instantiate.c is
>> not an option.
> 
> I'm wondering if we can meanwhile update hwdb to support
> i2c-multi-instantiate cases in the future and in a few years switch to
> it.

Even if we fix current hwdb entries to match on both, then there
is no guarantee newly added entries will also contain the new match.

Now with the code to get the matrix from the ACPI tables new entries
should happen less often, but I saw at least one model where the ACPI
provided matrix appears to be wrong (if the ACPI matrix was always
correct then breaking hwdb would not really be an issue).

So I don't think this is going to work and all in all it feels like
a lot of work for little gain.


>> Changes by Hans de Goede:
>> -Add explanation to the commit message why i2c-multi-instantiate.c
>>  cannot be used
>> -Also set the dev_name, fwnode and irq i2c_board_info struct members
>>  for the 2nd client
> 
> ...
> 
>> +       ret = bmc150_accel_core_probe(&client->dev, regmap, client->irq, name, block_supported);
>> +       if (ret)
>> +               return ret;
>> +
>> +       /*
>> +        * Some BOSC0200 acpi_devices describe 2 accelerometers in a single ACPI
>> +        * device, try instantiating a second i2c_client for an I2cSerialBusV2
>> +        * ACPI resource with index 1. The !id check avoids recursion when
>> +        * bmc150_accel_probe() gets called for the second client.
>> +        */
> 
>> +       if (!id && adev && strcmp(acpi_device_hid(adev), "BOSC0200") == 0) {
> 
>> +               struct i2c_board_info board_info = {
>> +                       .type = "bmc150_accel",
>> +                       /* The 2nd accel sits in the base of 2-in-1s */
> 
>> +                       .dev_name = "BOSC0200:base",
> 
> Hmm... Can we use '.' (dot) rather than ':' (colon) to avoid confusion
> with ACPI device naming schema? (Or was it on purpose?)

So with the ':' the end result is:

[root@localhost ~]# cd /sys/bus/i2c/devices/
[root@localhost devices]# ls | cat
6-0050
i2c-0
i2c-1
i2c-2
i2c-3
i2c-4
i2c-5
i2c-6
i2c-BOSC0200:00
i2c-BOSC0200:base
i2c-WCOM50BD:00

Which looks nice and consistent, which is why I went with the ':'
and since base is not a number, there is no chance on conflicting with
ACPI device names (it does look somewhat like an ACPI device name, but
it is an ACPI enumerated device, so ...).

Anyways if there is a strong preference for changing this to a '.'
I would be happy to make this change.

> And this seems to be the only device in the system, second as this is
> not allowed as far as I understand. Right?

I don't understand what you are trying to say here, sorry.

> But theoretically I can
> create an ACPI SSDT with quite similar excerpt and sensor and
> enumerate it via ConfigFS (I understand that is quite unlikely).


> 
>> +                       .fwnode = client->dev.fwnode,
>> +                       .irq = -ENOENT,
>> +               };
>> +               struct i2c_client *second_dev;
>> +
>> +               second_dev = i2c_acpi_new_device(&client->dev, 1, &board_info);
>> +               if (!IS_ERR(second_dev))
>> +                       bmc150_set_second_device(second_dev);
>> +       }
> 
> ...
> 
>>  static int bmc150_accel_remove(struct i2c_client *client)
>>  {
>> +       struct i2c_client *second_dev = bmc150_get_second_device(client);
> 
>> +       if (second_dev)
> 
> Redundant.

True, I will fix this for v2, once the ':' vs '.' thing is settled.

Regards,

Hans



> 
>> +               i2c_unregister_device(second_dev);
>> +
>>         return bmc150_accel_core_remove(&client->dev);
>>  }
> 

Re: [PATCH 3/3] iio: accel: bmc150: Get mount-matrix from ACPI

[Hans de Goede]

Hi,

On 11/25/20 12:07 PM, Andy Shevchenko wrote:
> On Wed, Nov 25, 2020 at 10:37 AM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> bmc150 accelerometers with an ACPI hardware-id of BOSC0200 have an ACPI
>> method providing their mount-matrix, add support for retrieving this.
> 
> ...
> 
>> +       if (strcmp(dev_name(dev), "i2c-BOSC0200:base") == 0)
>> +               alt_name = "ROMK";
>> +       else
>> +               alt_name = "ROMS";
>> +
>> +       if (acpi_has_method(adev->handle, "ROTM"))
>> +               name = "ROTM";
> 
> My gosh, it's a third method of this...
> 
> ...
> 
>> +       elements = obj->package.elements;
>> +       for (i = 0; i < 3; i++) {
>> +               if (elements[i].type != ACPI_TYPE_STRING)
>> +                       goto unknown_format;
>> +
>> +               str = elements[i].string.pointer;
>> +               if (sscanf(str, "%d %d %d", &val[0], &val[1], &val[2]) != 3)
>> +                       goto unknown_format;
>> +
>> +               for (j = 0; j < 3; j++) {
>> +                       switch (val[j]) {
>> +                       case -1: str = "-1"; break;
>> +                       case 0:  str = "0";  break;
>> +                       case 1:  str = "1";  break;
>> +                       default: goto unknown_format;
>> +                       }
>> +                       orientation->rotation[i * 3 + j] = str;
>> +               }
>> +       }
> 
> I'm wondering if we can come up with some common code out of this and
> existing apply_acpi_orientation().

Honestly they are all different enough that I don't think it is worth
the trouble (I did take a look at this, but it did not seem feasible
without creating horrible code).

Regards,

Hans

Re: [PATCH 2/3] iio: accel: bmc150: Check for a second ACPI device for BOSC0200

[Andy Shevchenko]

On Wed, Nov 25, 2020 at 1:11 PM Hans de Goede <hdegoede@redhat.com> wrote:
> On 11/25/20 11:55 AM, Andy Shevchenko wrote:
> > On Wed, Nov 25, 2020 at 10:37 AM Hans de Goede <hdegoede@redhat.com> wrote:

...

> > I'm wondering if we can meanwhile update hwdb to support
> > i2c-multi-instantiate cases in the future and in a few years switch to
> > it.
>
> Even if we fix current hwdb entries to match on both, then there
> is no guarantee newly added entries will also contain the new match.
>
> Now with the code to get the matrix from the ACPI tables new entries
> should happen less often, but I saw at least one model where the ACPI
> provided matrix appears to be wrong (if the ACPI matrix was always
> correct then breaking hwdb would not really be an issue).
>
> So I don't think this is going to work and all in all it feels like
> a lot of work for little gain.

Okay!

...

> >> +                       .dev_name = "BOSC0200:base",
> >
> > Hmm... Can we use '.' (dot) rather than ':' (colon) to avoid confusion
> > with ACPI device naming schema? (Or was it on purpose?)
>
> So with the ':' the end result is:
>
> [root@localhost ~]# cd /sys/bus/i2c/devices/
> [root@localhost devices]# ls | cat
> 6-0050
> i2c-0
> i2c-1
> i2c-2
> i2c-3
> i2c-4
> i2c-5
> i2c-6
> i2c-BOSC0200:00
> i2c-BOSC0200:base
> i2c-WCOM50BD:00
>
> Which looks nice and consistent, which is why I went with the ':'
> and since base is not a number, there is no chance on conflicting with
> ACPI device names (it does look somewhat like an ACPI device name, but
> it is an ACPI enumerated device, so ...).

I see. So this was made on purpose.

> Anyways if there is a strong preference for changing this to a '.'
> I would be happy to make this change.

No strong preferences from my side.

> > And this seems to be the only device in the system, second as this is
> > not allowed as far as I understand. Right?
>
> I don't understand what you are trying to say here, sorry.
>
> > But theoretically I can
> > create an ACPI SSDT with quite similar excerpt and sensor and
> > enumerate it via ConfigFS (I understand that is quite unlikely).

What if you have two devices with the same ID and both have two
I2cSerialBusV2() resources? Second one can't be instantiated because
'base' is already here.
Making it like i2c-BOSC0200:00.base would be much better in my opinion.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 3/3] iio: accel: bmc150: Get mount-matrix from ACPI

[Andy Shevchenko]

On Wed, Nov 25, 2020 at 1:12 PM Hans de Goede <hdegoede@redhat.com> wrote:
> On 11/25/20 12:07 PM, Andy Shevchenko wrote:
> > On Wed, Nov 25, 2020 at 10:37 AM Hans de Goede <hdegoede@redhat.com> wrote:

...

> > I'm wondering if we can come up with some common code out of this and
> > existing apply_acpi_orientation().
>
> Honestly they are all different enough that I don't think it is worth
> the trouble (I did take a look at this, but it did not seem feasible
> without creating horrible code).

Okay!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/3] iio: accel: bmc150: Check for a second ACPI device for BOSC0200

[Hans de Goede]

Hi,

On 11/25/20 12:20 PM, Andy Shevchenko wrote:
> On Wed, Nov 25, 2020 at 1:11 PM Hans de Goede <hdegoede@redhat.com> wrote:
>> On 11/25/20 11:55 AM, Andy Shevchenko wrote:
>>> On Wed, Nov 25, 2020 at 10:37 AM Hans de Goede <hdegoede@redhat.com> wrote:
> 
> ...
> 
>>> I'm wondering if we can meanwhile update hwdb to support
>>> i2c-multi-instantiate cases in the future and in a few years switch to
>>> it.
>>
>> Even if we fix current hwdb entries to match on both, then there
>> is no guarantee newly added entries will also contain the new match.
>>
>> Now with the code to get the matrix from the ACPI tables new entries
>> should happen less often, but I saw at least one model where the ACPI
>> provided matrix appears to be wrong (if the ACPI matrix was always
>> correct then breaking hwdb would not really be an issue).
>>
>> So I don't think this is going to work and all in all it feels like
>> a lot of work for little gain.
> 
> Okay!
> 
> ...
> 
>>>> +                       .dev_name = "BOSC0200:base",
>>>
>>> Hmm... Can we use '.' (dot) rather than ':' (colon) to avoid confusion
>>> with ACPI device naming schema? (Or was it on purpose?)
>>
>> So with the ':' the end result is:
>>
>> [root@localhost ~]# cd /sys/bus/i2c/devices/
>> [root@localhost devices]# ls | cat
>> 6-0050
>> i2c-0
>> i2c-1
>> i2c-2
>> i2c-3
>> i2c-4
>> i2c-5
>> i2c-6
>> i2c-BOSC0200:00
>> i2c-BOSC0200:base
>> i2c-WCOM50BD:00
>>
>> Which looks nice and consistent, which is why I went with the ':'
>> and since base is not a number, there is no chance on conflicting with
>> ACPI device names (it does look somewhat like an ACPI device name, but
>> it is an ACPI enumerated device, so ...).
> 
> I see. So this was made on purpose.
> 
>> Anyways if there is a strong preference for changing this to a '.'
>> I would be happy to make this change.
> 
> No strong preferences from my side.
> 
>>> And this seems to be the only device in the system, second as this is
>>> not allowed as far as I understand. Right?
>>
>> I don't understand what you are trying to say here, sorry.
>>
>>> But theoretically I can
>>> create an ACPI SSDT with quite similar excerpt and sensor and
>>> enumerate it via ConfigFS (I understand that is quite unlikely).
> 
> What if you have two devices with the same ID and both have two
> I2cSerialBusV2() resources? Second one can't be instantiated because
> 'base' is already here.
> Making it like i2c-BOSC0200:00.base would be much better in my opinion.

Ah I see, that is a somewhat valid point. But I really never expect
there to be 2 ACPI devices with a BOSC0200 hw-id, while also specifying
more then 1 i2c-client per node. That would just be all kinds of messed-up.

Thinking about this I think that getting a WARN_ON (and thus a bug report)
about a duplicate kobject-name when this happens would actually be good,
because then we need to figure out what the beep is going on on that
system. Note that other then triggering a WARN_ON the second
i2c_acpi_new_device will simply fail in this very unlikely scenario
(I know because I triggered this by accident while working on the patch).

Since in a way getting this WARN_ON is actually good (lets us know about
completely unexpected circumstances) and that making the name dynamic
as you suggest requires a bit of extra code I would actually prefer to
keep this as. Please let me know if that is ok with you.

Regards,

Hans

Re: [PATCH 2/3] iio: accel: bmc150: Check for a second ACPI device for BOSC0200

[Andy Shevchenko]

On Wed, Nov 25, 2020 at 6:09 PM Hans de Goede <hdegoede@redhat.com> wrote:
> On 11/25/20 12:20 PM, Andy Shevchenko wrote:
> > On Wed, Nov 25, 2020 at 1:11 PM Hans de Goede <hdegoede@redhat.com> wrote:
> >> On 11/25/20 11:55 AM, Andy Shevchenko wrote:

...

> >> i2c-BOSC0200:base

> > What if you have two devices with the same ID and both have two
> > I2cSerialBusV2() resources? Second one can't be instantiated because
> > 'base' is already here.
> > Making it like i2c-BOSC0200:00.base would be much better in my opinion.
>
> Ah I see, that is a somewhat valid point. But I really never expect
> there to be 2 ACPI devices with a BOSC0200 hw-id, while also specifying
> more then 1 i2c-client per node. That would just be all kinds of messed-up.

I also don't expect such, but probability is still greater than zero
(somebody may copy'n'paste the ASL excerpt from this device and apply
as SSDT in one of DIYs kinda projects).

> Thinking about this I think that getting a WARN_ON (and thus a bug report)
> about a duplicate kobject-name when this happens would actually be good,
> because then we need to figure out what the beep is going on on that
> system. Note that other then triggering a WARN_ON the second
> i2c_acpi_new_device will simply fail in this very unlikely scenario
> (I know because I triggered this by accident while working on the patch).
>
> Since in a way getting this WARN_ON is actually good (lets us know about
> completely unexpected circumstances) and that making the name dynamic
> as you suggest requires a bit of extra code I would actually prefer to
> keep this as. Please let me know if that is ok with you.

Can you put a comment in the code that this name is considered global
for now as we do not expect such circumstances. Then I'll be fine.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/3] iio: accel: bmc150: Improve ACPI enumeration support

[Jonathan Cameron]

On Wed, 25 Nov 2020 11:49:15 +0100
Hans de Goede <hdegoede@redhat.com> wrote:

> Hi,
> 
> On 11/25/20 11:41 AM, Andy Shevchenko wrote:
> > On Wed, Nov 25, 2020 at 10:37 AM Hans de Goede <hdegoede@redhat.com> wrote:  
> >>
> >> Hi All,
> >>
> >> This series improves support for ACPI enumeration of bmc150 accels
> >> described by an ACPI node with an ACPI hw-id of BOSC0200:
> >>
> >> 1. Add support for nodes which describe 2 acceleromers in a single node,  
> > 
> > accelerometers
> >   
> >> fixing: https://bugzilla.kernel.org/show_bug.cgi?id=198671
> >>
> >> 2. Add support for reading the mount-matrix from the ACPI node.
> >>
> >> This is done in patches 2 - 3, patch 1 is a trivial cleanup which I noticed
> >> could be done while working on this.
> >>
> >> Patch 2 is based on an earlier patch for this from Jeremy Cline:
> >> https://lore.kernel.org/r/010001602cf53153-39ad69f1-1b39-4e6d-a748-9455a16c2fbd-000000@email.amazonses.com
> >>
> >> That patch was put on hold because normally ACPI nodes which describe
> >> multiple i2c-clients in a single node are handled by:
> >> drivers/platform/x86/i2c-multi-instantiate.c
> >>
> >> Which I tried to do at first, but as explained in the commit msg
> >> of the updated patch, that is not possible in this special case
> >> (because it would cause userspace breakage due to the modalias changing).  
> > 
> > This is marked as patch 1?!  
> 
> Yes my bad, sorry.
> 
> > Usually --cover-letter produces a correct template...  
> 
> I use --compose, time to switch to --cover-letter instead I guess.
> 
> Regards,
> 
> Hans
> 

Other than the minor bits Andy mentioned I'm fine with this.

It does sort of feel like we should be gathering documentation somewhere of
where this crazy stuff has been seen in the wild.

Oh for a standard... Whilst I've been known to push the odd thing through
standards bodies, I'm not sure I can really claim this one is related to my
day job (Huawei is a UEFI forum member).  If anyone is particularly
enthusiastic and doesn't have direct access to the UEFI forum via their
job, they could try using the code first route into ACPI via tianocore.

Of course, nothing says anyone will actually use the standard anyway but
at least it would give us the moral high ground ;)

Jonathan

Re: [PATCH 1/3] iio: accel: bmc150: Improve ACPI enumeration support

[Hans de Goede]

Hi,

On 11/28/20 2:16 PM, Jonathan Cameron wrote:
> On Wed, 25 Nov 2020 11:49:15 +0100
> Hans de Goede <hdegoede@redhat.com> wrote:
> 
>> Hi,
>>
>> On 11/25/20 11:41 AM, Andy Shevchenko wrote:
>>> On Wed, Nov 25, 2020 at 10:37 AM Hans de Goede <hdegoede@redhat.com> wrote:  
>>>>
>>>> Hi All,
>>>>
>>>> This series improves support for ACPI enumeration of bmc150 accels
>>>> described by an ACPI node with an ACPI hw-id of BOSC0200:
>>>>
>>>> 1. Add support for nodes which describe 2 acceleromers in a single node,  
>>>
>>> accelerometers
>>>   
>>>> fixing: https://bugzilla.kernel.org/show_bug.cgi?id=198671
>>>>
>>>> 2. Add support for reading the mount-matrix from the ACPI node.
>>>>
>>>> This is done in patches 2 - 3, patch 1 is a trivial cleanup which I noticed
>>>> could be done while working on this.
>>>>
>>>> Patch 2 is based on an earlier patch for this from Jeremy Cline:
>>>> https://lore.kernel.org/r/010001602cf53153-39ad69f1-1b39-4e6d-a748-9455a16c2fbd-000000@email.amazonses.com
>>>>
>>>> That patch was put on hold because normally ACPI nodes which describe
>>>> multiple i2c-clients in a single node are handled by:
>>>> drivers/platform/x86/i2c-multi-instantiate.c
>>>>
>>>> Which I tried to do at first, but as explained in the commit msg
>>>> of the updated patch, that is not possible in this special case
>>>> (because it would cause userspace breakage due to the modalias changing).  
>>>
>>> This is marked as patch 1?!  
>>
>> Yes my bad, sorry.
>>
>>> Usually --cover-letter produces a correct template...  
>>
>> I use --compose, time to switch to --cover-letter instead I guess.
>>
>> Regards,
>>
>> Hans
>>
> 
> Other than the minor bits Andy mentioned I'm fine with this.
> 
> It does sort of feel like we should be gathering documentation somewhere of
> where this crazy stuff has been seen in the wild.

I can provide an (incomplete) list of devices known to use the BOSC0200 ACPI HID,
both in single and dual accelerometer configs.

In lieu of a better place, I guess I could best just drop this info in
a big comment block near the ACPI mount-matrix parsing stuff ?

If you agree that a comment is a reasonable place to place this info I
can add this for v2 of the patch-set.

Regards,

Hans

Re: [PATCH 1/3] iio: accel: bmc150: Improve ACPI enumeration support

[Jonathan Cameron]

On Sat, 28 Nov 2020 14:32:49 +0100
Hans de Goede <hdegoede@redhat.com> wrote:

> Hi,
> 
> On 11/28/20 2:16 PM, Jonathan Cameron wrote:
> > On Wed, 25 Nov 2020 11:49:15 +0100
> > Hans de Goede <hdegoede@redhat.com> wrote:
> >   
> >> Hi,
> >>
> >> On 11/25/20 11:41 AM, Andy Shevchenko wrote:  
> >>> On Wed, Nov 25, 2020 at 10:37 AM Hans de Goede <hdegoede@redhat.com> wrote:    
> >>>>
> >>>> Hi All,
> >>>>
> >>>> This series improves support for ACPI enumeration of bmc150 accels
> >>>> described by an ACPI node with an ACPI hw-id of BOSC0200:
> >>>>
> >>>> 1. Add support for nodes which describe 2 acceleromers in a single node,    
> >>>
> >>> accelerometers
> >>>     
> >>>> fixing: https://bugzilla.kernel.org/show_bug.cgi?id=198671
> >>>>
> >>>> 2. Add support for reading the mount-matrix from the ACPI node.
> >>>>
> >>>> This is done in patches 2 - 3, patch 1 is a trivial cleanup which I noticed
> >>>> could be done while working on this.
> >>>>
> >>>> Patch 2 is based on an earlier patch for this from Jeremy Cline:
> >>>> https://lore.kernel.org/r/010001602cf53153-39ad69f1-1b39-4e6d-a748-9455a16c2fbd-000000@email.amazonses.com
> >>>>
> >>>> That patch was put on hold because normally ACPI nodes which describe
> >>>> multiple i2c-clients in a single node are handled by:
> >>>> drivers/platform/x86/i2c-multi-instantiate.c
> >>>>
> >>>> Which I tried to do at first, but as explained in the commit msg
> >>>> of the updated patch, that is not possible in this special case
> >>>> (because it would cause userspace breakage due to the modalias changing).    
> >>>
> >>> This is marked as patch 1?!    
> >>
> >> Yes my bad, sorry.
> >>  
> >>> Usually --cover-letter produces a correct template...    
> >>
> >> I use --compose, time to switch to --cover-letter instead I guess.
> >>
> >> Regards,
> >>
> >> Hans
> >>  
> > 
> > Other than the minor bits Andy mentioned I'm fine with this.
> > 
> > It does sort of feel like we should be gathering documentation somewhere of
> > where this crazy stuff has been seen in the wild.  
> 
> I can provide an (incomplete) list of devices known to use the BOSC0200 ACPI HID,
> both in single and dual accelerometer configs.
> 
> In lieu of a better place, I guess I could best just drop this info in
> a big comment block near the ACPI mount-matrix parsing stuff ?
> 
> If you agree that a comment is a reasonable place to place this info I
> can add this for v2 of the patch-set.
It is as good a place as any so go for it.

thanks,

Jonathan

p.s. Mailing list is messing around today I think as neither I nor lore.kernel.org
are getting mails from it.  It'll probably be resolved shorty but in meantime
discussion may get a bit separated and I can't use b4 for new patches which
will make it trickier to apply things until that is fixed.

> 
> Regards,
> 
> Hans
>