[PATCH v2 0/9] iio: accel: bmc150: Add support for yoga's with dual accelerometers with an ACPI HID of DUAL250E

[PATCH v2 0/9] iio: accel: bmc150: Add support for yoga's with dual accelerometers with an ACPI HID of DUAL250E

[Hans de Goede]

Hi all,

Here is v2 of this series, addressing Andy's review remarks and
rewrap some comments at 80 chars limit.

For more info here is the v1 cover-letter:

Some 360 degree hinges (yoga) style 2-in-1 devices use 2 bmc150 accels
to allow the OS to determine the angle between the display and the base
of the device, so that the OS can determine if the 2-in-1 is in laptop
or in tablet-mode.

We already support this setup on devices using a single ACPI node
with a HID of "BOSC0200" to describe both accelerometers. This patch
set extends this support to also support the same setup but then
using a HID of "DUAL250E".

While testing this I found some crashes on rmmod, patches 1-2
fix those patches, patch 3 does some refactoring and patch 4
adds support for the "DUAL250E" HID.

Unfortunately we need some more special handling though, which the
rest of the patches are for.

On Windows both accelerometers are read (polled) by a special service
and this service calls a DSM (Device Specific Method), which in turn
translates the angles to one of laptop/tablet/tent/stand mode and then
notifies the EC about the new mode and the EC then enables or disables
the builtin keyboard and touchpad based in the mode.

When the 2-in-1 is powered-on or resumed folded in tablet mode the
EC senses this independent of the DSM by using a HALL effect sensor
which senses that the keyboard has been folded away behind the display.

At power-on or resume the EC disables the keyboard based on this and
the only way to get the keyboard to work after this is to call the
DSM to re-enable it (similar to how we also need to call a special
DSM in the kxcjk-1013.c accel driver to re-enable the keyboard).

Patches 5-7 deal with the DSM mess and patch 8 adds labels to the
2 accelerometers specifying which one is which.

Regards,

Hans


Hans de Goede (9):
  iio: accel: bmc150: Fix dereferencing the wrong pointer in
    bmc150_get/set_second_device
  iio: accel: bmc150: Don't make the remove function of the second
    accelerometer unregister itself
  iio: accel: bmc150: Move check for second ACPI device into a separate
    function
  iio: accel: bmc150: Add support for dual-accelerometers with a
    DUAL250E HID
  iio: accel: bmc150: Move struct bmc150_accel_data definition to
    bmc150-accel.h
  iio: accel: bmc150: Remove bmc150_set/get_second_device() accessor
    functions
  iio: accel: bmc150: Add support for DUAL250E ACPI DSM for setting the
    hinge angle
  iio: accel: bmc150: Refactor bmc150_apply_acpi_orientation()
  iio: accel: bmc150: Set label based on accel-location for ACPI
    DUAL250E fwnodes

 drivers/iio/accel/bmc150-accel-core.c | 110 +++++----------
 drivers/iio/accel/bmc150-accel-i2c.c  | 193 ++++++++++++++++++++++----
 drivers/iio/accel/bmc150-accel.h      |  66 ++++++++-
 3 files changed, 260 insertions(+), 109 deletions(-)

-- 
2.31.1

[PATCH v2 1/9] iio: accel: bmc150: Fix dereferencing the wrong pointer in bmc150_get/set_second_device

[Hans de Goede]

The drvdata for iio-parent devices points to the struct iio_dev for
the iio-device. So by directly casting the return from i2c_get_clientdata()
to struct bmc150_accel_data * the code was ending up storing the second_dev
pointer in (and retrieving it from) some semi-random offset inside
struct iio_dev, rather then storing it in the second_dev member of the
bmc150_accel_data struct.

Fix the code to get the struct bmc150_accel_data * pointer to call
iio_priv() on the struct iio_dev * returned by i2c_get_clientdata(),
so that the correct pointer gets dereferenced.

This fixes the following oops on rmmod, caused by trying to
dereference the wrong return of bmc150_get_second_device():

[  238.980737] BUG: unable to handle page fault for address: 0000000000004710
[  238.980755] #PF: supervisor read access in kernel mode
[  238.980760] #PF: error_code(0x0000) - not-present page
...
[  238.980841]  i2c_unregister_device.part.0+0x19/0x60
[  238.980856]  0xffffffffc0815016
[  238.980863]  i2c_device_remove+0x25/0xb0
[  238.980869]  __device_release_driver+0x180/0x240
[  238.980876]  driver_detach+0xd4/0x120
[  238.980882]  bus_remove_driver+0x5b/0xd0
[  238.980888]  i2c_del_driver+0x44/0x70

While at it also remove the now no longer sensible checks for data
being NULL, iio_priv never returns NULL for an iio_dev with non 0
sized private-data.

Fixes: 5bfb3a4bd8f6 ("iio: accel: bmc150: Check for a second ACPI device for BOSC0200")
Cc: Jeremy Cline <jeremy@jcline.org>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/iio/accel/bmc150-accel-core.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/drivers/iio/accel/bmc150-accel-core.c b/drivers/iio/accel/bmc150-accel-core.c
index 04d85ce34e9f..3a3f67930165 100644
--- a/drivers/iio/accel/bmc150-accel-core.c
+++ b/drivers/iio/accel/bmc150-accel-core.c
@@ -1809,10 +1809,7 @@ 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;
+	struct bmc150_accel_data *data = iio_priv(i2c_get_clientdata(client));
 
 	return data->second_device;
 }
@@ -1820,10 +1817,9 @@ 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);
+	struct bmc150_accel_data *data = iio_priv(i2c_get_clientdata(client));
 
-	if (data)
-		data->second_device = client;
+	data->second_device = client;
 }
 EXPORT_SYMBOL_GPL(bmc150_set_second_device);
 
-- 
2.31.1

[PATCH v2 2/9] iio: accel: bmc150: Don't make the remove function of the second accelerometer unregister itself

[Hans de Goede]

On machines with dual accelerometers described in a single ACPI fwnode,
the bmc150_accel_probe() instantiates a second i2c-client for the second
accelerometer.

A pointer to this manually instantiated second i2c-client is stored
inside the iio_dev's private-data through bmc150_set_second_device(),
so that the i2c-client can be unregistered from bmc150_accel_remove().

Before this commit bmc150_set_second_device() took only 1 argument so it
would store the pointer in private-data of the iio_dev belonging to the
manually instantiated i2c-client, leading to the bmc150_accel_remove()
call for the second_dev trying to unregister *itself* while it was
being removed, leading to a deadlock and rmmod hanging.

Change bmc150_set_second_device() to take 2 arguments: 1. The i2c-client
which is instantiating the second i2c-client for the 2nd accelerometer and
2. The second-device pointer itself (which also is an i2c-client).

This will store the second_device pointer in the private data of the
iio_dev belonging to the (ACPI instantiated) i2c-client for the first
accelerometer and will make bmc150_accel_remove() unregister the
second_device i2c-client when called for the first client,
avoiding the deadlock.

Fixes: 5bfb3a4bd8f6 ("iio: accel: bmc150: Check for a second ACPI device for BOSC0200")
Cc: Jeremy Cline <jeremy@jcline.org>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/iio/accel/bmc150-accel-core.c | 4 ++--
 drivers/iio/accel/bmc150-accel-i2c.c  | 2 +-
 drivers/iio/accel/bmc150-accel.h      | 2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/accel/bmc150-accel-core.c b/drivers/iio/accel/bmc150-accel-core.c
index 3a3f67930165..8ff358c37a81 100644
--- a/drivers/iio/accel/bmc150-accel-core.c
+++ b/drivers/iio/accel/bmc150-accel-core.c
@@ -1815,11 +1815,11 @@ struct i2c_client *bmc150_get_second_device(struct i2c_client *client)
 }
 EXPORT_SYMBOL_GPL(bmc150_get_second_device);
 
-void bmc150_set_second_device(struct i2c_client *client)
+void bmc150_set_second_device(struct i2c_client *client, struct i2c_client *second_dev)
 {
 	struct bmc150_accel_data *data = iio_priv(i2c_get_clientdata(client));
 
-	data->second_device = client;
+	data->second_device = second_dev;
 }
 EXPORT_SYMBOL_GPL(bmc150_set_second_device);
 
diff --git a/drivers/iio/accel/bmc150-accel-i2c.c b/drivers/iio/accel/bmc150-accel-i2c.c
index 69f709319484..2afaae0294ee 100644
--- a/drivers/iio/accel/bmc150-accel-i2c.c
+++ b/drivers/iio/accel/bmc150-accel-i2c.c
@@ -70,7 +70,7 @@ static int bmc150_accel_probe(struct i2c_client *client,
 
 		second_dev = i2c_acpi_new_device(&client->dev, 1, &board_info);
 		if (!IS_ERR(second_dev))
-			bmc150_set_second_device(second_dev);
+			bmc150_set_second_device(client, second_dev);
 	}
 #endif
 
diff --git a/drivers/iio/accel/bmc150-accel.h b/drivers/iio/accel/bmc150-accel.h
index 6024f15b9700..e30c1698f6fb 100644
--- a/drivers/iio/accel/bmc150-accel.h
+++ b/drivers/iio/accel/bmc150-accel.h
@@ -18,7 +18,7 @@ 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);
+void bmc150_set_second_device(struct i2c_client *client, struct i2c_client *second_dev);
 extern const struct dev_pm_ops bmc150_accel_pm_ops;
 extern const struct regmap_config bmc150_regmap_conf;
 
-- 
2.31.1

[PATCH v2 3/9] iio: accel: bmc150: Move check for second ACPI device into a separate function

[Hans de Goede]

Move the check for a second ACPI device for BOSC0200 ACPI fwnodes into
a new bmc150_acpi_dual_accel_probe() helper function.

This is a preparation patch for adding support for a new "DUAL250E" ACPI
Hardware-ID (HID) used on some devices.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
- Drop comma after terminating entry of bmc150_acpi_dual_accel_ids array
- Rewrap some comments at 80 chars limit
---
 drivers/iio/accel/bmc150-accel-i2c.c | 80 ++++++++++++++++++----------
 1 file changed, 51 insertions(+), 29 deletions(-)

diff --git a/drivers/iio/accel/bmc150-accel-i2c.c b/drivers/iio/accel/bmc150-accel-i2c.c
index 2afaae0294ee..f7cb40f481ef 100644
--- a/drivers/iio/accel/bmc150-accel-i2c.c
+++ b/drivers/iio/accel/bmc150-accel-i2c.c
@@ -21,6 +21,52 @@
 
 #include "bmc150-accel.h"
 
+#ifdef CONFIG_ACPI
+static const struct acpi_device_id bmc150_acpi_dual_accel_ids[] = {
+	{"BOSC0200"},
+	{ }
+};
+
+/*
+ * Some acpi_devices describe 2 accelerometers in a single ACPI device,
+ * try instantiating a second i2c_client for an I2cSerialBusV2 ACPI resource
+ * with index 1.
+ */
+static void bmc150_acpi_dual_accel_probe(struct i2c_client *client)
+{
+	struct acpi_device *adev = ACPI_COMPANION(&client->dev);
+	struct i2c_client *second_dev;
+	struct i2c_board_info board_info = {
+		.type = "bmc150_accel",
+		/*
+		 * The 2nd accel sits in the base of 2-in-1s. Note this name is
+		 * static, as there should never be more then 1 BOSC0200 ACPI
+		 * node with 2 accelerometers in it.
+		 */
+		.dev_name = "BOSC0200:base",
+		.fwnode = client->dev.fwnode,
+		.irq = -ENOENT,
+	};
+
+	if (acpi_match_device_ids(adev, bmc150_acpi_dual_accel_ids))
+		return;
+
+	second_dev = i2c_acpi_new_device(&client->dev, 1, &board_info);
+	if (!IS_ERR(second_dev))
+		bmc150_set_second_device(client, second_dev);
+}
+
+static void bmc150_acpi_dual_accel_remove(struct i2c_client *client)
+{
+	struct i2c_client *second_dev = bmc150_get_second_device(client);
+
+	i2c_unregister_device(second_dev);
+}
+#else
+static void bmc150_acpi_dual_accel_probe(struct i2c_client *client) {}
+static void bmc150_acpi_dual_accel_remove(struct i2c_client *client) {}
+#endif
+
 static int bmc150_accel_probe(struct i2c_client *client,
 			      const struct i2c_device_id *id)
 {
@@ -30,7 +76,6 @@ 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 __maybe_unused *adev;
 	int ret;
 
 	regmap = devm_regmap_init_i2c(client, &bmc150_regmap_conf);
@@ -47,41 +92,18 @@ static int bmc150_accel_probe(struct i2c_client *client,
 		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.
+	 * The !id check avoids recursion when probe() gets called
+	 * for the second client.
 	 */
-#ifdef CONFIG_ACPI
-	adev = ACPI_COMPANION(&client->dev);
-	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. Note this
-			 * name is static, as there should never be more then 1
-			 * BOSC0200 ACPI node with 2 accelerometers in it.
-			 */
-			.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(client, second_dev);
-	}
-#endif
+	if (!id && has_acpi_companion(&client->dev))
+		bmc150_acpi_dual_accel_probe(client);
 
 	return 0;
 }
 
 static int bmc150_accel_remove(struct i2c_client *client)
 {
-	struct i2c_client *second_dev = bmc150_get_second_device(client);
-
-	i2c_unregister_device(second_dev);
+	bmc150_acpi_dual_accel_remove(client);
 
 	return bmc150_accel_core_remove(&client->dev);
 }
-- 
2.31.1

[PATCH v2 4/9] iio: accel: bmc150: Add support for dual-accelerometers with a DUAL250E HID

[Hans de Goede]

The Lenovo Yoga 300-11IBR has a ACPI fwnode with a HID of DUAL250E
which contains I2C and IRQ resources for 2 accelerometers, 1 in the
display and one in the base of the device. Add support for this.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
- Rewrap some comments at 80 chars limit
- Use acpi_dev_gpio_irq_get() instead of acpi_dev_gpio_irq_get_by()
---
 drivers/iio/accel/bmc150-accel-i2c.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/drivers/iio/accel/bmc150-accel-i2c.c b/drivers/iio/accel/bmc150-accel-i2c.c
index f7cb40f481ef..41b4c9e22d60 100644
--- a/drivers/iio/accel/bmc150-accel-i2c.c
+++ b/drivers/iio/accel/bmc150-accel-i2c.c
@@ -24,6 +24,7 @@
 #ifdef CONFIG_ACPI
 static const struct acpi_device_id bmc150_acpi_dual_accel_ids[] = {
 	{"BOSC0200"},
+	{"DUAL250E"},
 	{ }
 };
 
@@ -36,21 +37,24 @@ static void bmc150_acpi_dual_accel_probe(struct i2c_client *client)
 {
 	struct acpi_device *adev = ACPI_COMPANION(&client->dev);
 	struct i2c_client *second_dev;
+	char dev_name[16];
 	struct i2c_board_info board_info = {
 		.type = "bmc150_accel",
-		/*
-		 * The 2nd accel sits in the base of 2-in-1s. Note this name is
-		 * static, as there should never be more then 1 BOSC0200 ACPI
-		 * node with 2 accelerometers in it.
-		 */
-		.dev_name = "BOSC0200:base",
+		.dev_name = dev_name,
 		.fwnode = client->dev.fwnode,
-		.irq = -ENOENT,
 	};
 
 	if (acpi_match_device_ids(adev, bmc150_acpi_dual_accel_ids))
 		return;
 
+	/*
+	 * The 2nd accel sits in the base of 2-in-1s. The suffix is static, as
+	 * there should never be more then 1 ACPI node with 2 accelerometers.
+	 */
+	snprintf(dev_name, sizeof(dev_name), "%s:base", acpi_device_hid(adev));
+
+	board_info.irq = acpi_dev_gpio_irq_get(adev, 1);
+
 	second_dev = i2c_acpi_new_device(&client->dev, 1, &board_info);
 	if (!IS_ERR(second_dev))
 		bmc150_set_second_device(client, second_dev);
@@ -118,6 +122,7 @@ static const struct acpi_device_id bmc150_accel_acpi_match[] = {
 	{"BMA222E",	bma222e},
 	{"BMA0280",	bma280},
 	{"BOSC0200"},
+	{"DUAL250E"},
 	{ },
 };
 MODULE_DEVICE_TABLE(acpi, bmc150_accel_acpi_match);
-- 
2.31.1

[PATCH v2 5/9] iio: accel: bmc150: Move struct bmc150_accel_data definition to bmc150-accel.h

[Hans de Goede]

Further patches to bmc150-accel-i2c.c need to store some extra info
(on top of the second_dev pointer) in the bmc150_accel_data struct,
rather then adding yet more accessor functions for this lets just
move the struct bmc150_accel_data definition to bmc150-accel.h.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/iio/accel/bmc150-accel-core.c | 53 -----------------------
 drivers/iio/accel/bmc150-accel.h      | 61 +++++++++++++++++++++++++++
 2 files changed, 61 insertions(+), 53 deletions(-)

diff --git a/drivers/iio/accel/bmc150-accel-core.c b/drivers/iio/accel/bmc150-accel-core.c
index 8ff358c37a81..0d76df9e08eb 100644
--- a/drivers/iio/accel/bmc150-accel-core.c
+++ b/drivers/iio/accel/bmc150-accel-core.c
@@ -157,59 +157,6 @@ struct bmc150_accel_chip_info {
 	const struct bmc150_scale_info scale_table[4];
 };
 
-struct bmc150_accel_interrupt {
-	const struct bmc150_accel_interrupt_info *info;
-	atomic_t users;
-};
-
-struct bmc150_accel_trigger {
-	struct bmc150_accel_data *data;
-	struct iio_trigger *indio_trig;
-	int (*setup)(struct bmc150_accel_trigger *t, bool state);
-	int intr;
-	bool enabled;
-};
-
-enum bmc150_accel_interrupt_id {
-	BMC150_ACCEL_INT_DATA_READY,
-	BMC150_ACCEL_INT_ANY_MOTION,
-	BMC150_ACCEL_INT_WATERMARK,
-	BMC150_ACCEL_INTERRUPTS,
-};
-
-enum bmc150_accel_trigger_id {
-	BMC150_ACCEL_TRIGGER_DATA_READY,
-	BMC150_ACCEL_TRIGGER_ANY_MOTION,
-	BMC150_ACCEL_TRIGGERS,
-};
-
-struct bmc150_accel_data {
-	struct regmap *regmap;
-	struct regulator_bulk_data regulators[2];
-	struct bmc150_accel_interrupt interrupts[BMC150_ACCEL_INTERRUPTS];
-	struct bmc150_accel_trigger triggers[BMC150_ACCEL_TRIGGERS];
-	struct mutex mutex;
-	u8 fifo_mode, watermark;
-	s16 buffer[8];
-	/*
-	 * Ensure there is sufficient space and correct alignment for
-	 * the timestamp if enabled
-	 */
-	struct {
-		__le16 channels[3];
-		s64 ts __aligned(8);
-	} scan;
-	u8 bw_bits;
-	u32 slope_dur;
-	u32 slope_thres;
-	u32 range;
-	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;
-};
-
 static const struct {
 	int val;
 	int val2;
diff --git a/drivers/iio/accel/bmc150-accel.h b/drivers/iio/accel/bmc150-accel.h
index e30c1698f6fb..f503c5b5801e 100644
--- a/drivers/iio/accel/bmc150-accel.h
+++ b/drivers/iio/accel/bmc150-accel.h
@@ -2,7 +2,68 @@
 #ifndef _BMC150_ACCEL_H_
 #define _BMC150_ACCEL_H_
 
+#include <linux/atomic.h>
+#include <linux/iio/iio.h>
+#include <linux/mutex.h>
+#include <linux/regulator/consumer.h>
+
 struct regmap;
+struct i2c_client;
+struct bmc150_accel_chip_info;
+struct bmc150_accel_interrupt_info;
+
+struct bmc150_accel_interrupt {
+	const struct bmc150_accel_interrupt_info *info;
+	atomic_t users;
+};
+
+struct bmc150_accel_trigger {
+	struct bmc150_accel_data *data;
+	struct iio_trigger *indio_trig;
+	int (*setup)(struct bmc150_accel_trigger *t, bool state);
+	int intr;
+	bool enabled;
+};
+
+enum bmc150_accel_interrupt_id {
+	BMC150_ACCEL_INT_DATA_READY,
+	BMC150_ACCEL_INT_ANY_MOTION,
+	BMC150_ACCEL_INT_WATERMARK,
+	BMC150_ACCEL_INTERRUPTS,
+};
+
+enum bmc150_accel_trigger_id {
+	BMC150_ACCEL_TRIGGER_DATA_READY,
+	BMC150_ACCEL_TRIGGER_ANY_MOTION,
+	BMC150_ACCEL_TRIGGERS,
+};
+
+struct bmc150_accel_data {
+	struct regmap *regmap;
+	struct regulator_bulk_data regulators[2];
+	struct bmc150_accel_interrupt interrupts[BMC150_ACCEL_INTERRUPTS];
+	struct bmc150_accel_trigger triggers[BMC150_ACCEL_TRIGGERS];
+	struct mutex mutex;
+	u8 fifo_mode, watermark;
+	s16 buffer[8];
+	/*
+	 * Ensure there is sufficient space and correct alignment for
+	 * the timestamp if enabled
+	 */
+	struct {
+		__le16 channels[3];
+		s64 ts __aligned(8);
+	} scan;
+	u8 bw_bits;
+	u32 slope_dur;
+	u32 slope_thres;
+	u32 range;
+	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;
+};
 
 enum {
 	bmc150,
-- 
2.31.1

[PATCH v2 6/9] iio: accel: bmc150: Remove bmc150_set/get_second_device() accessor functions

[Hans de Goede]

Now that the definition of the bmc150_accel_data struct is no longer
private to bmc150-accel-core.c, bmc150-accel-i2c.c can simply directly
access the second_dev member and the accessor functions are no longer
necessary.

Note if the i2c_acpi_new_device() for the second-client now fails,
an ERR_PTR gets stored in data->second_dev this is fine since it is only
ever passed to i2c_unregister_device() which has an IS_ERR_OR_NULL() check.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/iio/accel/bmc150-accel-core.c | 16 ----------------
 drivers/iio/accel/bmc150-accel-i2c.c  | 10 ++++------
 drivers/iio/accel/bmc150-accel.h      |  2 --
 3 files changed, 4 insertions(+), 24 deletions(-)

diff --git a/drivers/iio/accel/bmc150-accel-core.c b/drivers/iio/accel/bmc150-accel-core.c
index 0d76df9e08eb..0291512648b2 100644
--- a/drivers/iio/accel/bmc150-accel-core.c
+++ b/drivers/iio/accel/bmc150-accel-core.c
@@ -1754,22 +1754,6 @@ 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 = iio_priv(i2c_get_clientdata(client));
-
-	return data->second_device;
-}
-EXPORT_SYMBOL_GPL(bmc150_get_second_device);
-
-void bmc150_set_second_device(struct i2c_client *client, struct i2c_client *second_dev)
-{
-	struct bmc150_accel_data *data = iio_priv(i2c_get_clientdata(client));
-
-	data->second_device = second_dev;
-}
-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 41b4c9e22d60..ab0cda8ff8fa 100644
--- a/drivers/iio/accel/bmc150-accel-i2c.c
+++ b/drivers/iio/accel/bmc150-accel-i2c.c
@@ -35,8 +35,8 @@ static const struct acpi_device_id bmc150_acpi_dual_accel_ids[] = {
  */
 static void bmc150_acpi_dual_accel_probe(struct i2c_client *client)
 {
+	struct bmc150_accel_data *data = iio_priv(i2c_get_clientdata(client));
 	struct acpi_device *adev = ACPI_COMPANION(&client->dev);
-	struct i2c_client *second_dev;
 	char dev_name[16];
 	struct i2c_board_info board_info = {
 		.type = "bmc150_accel",
@@ -55,16 +55,14 @@ static void bmc150_acpi_dual_accel_probe(struct i2c_client *client)
 
 	board_info.irq = acpi_dev_gpio_irq_get(adev, 1);
 
-	second_dev = i2c_acpi_new_device(&client->dev, 1, &board_info);
-	if (!IS_ERR(second_dev))
-		bmc150_set_second_device(client, second_dev);
+	data->second_device = i2c_acpi_new_device(&client->dev, 1, &board_info);
 }
 
 static void bmc150_acpi_dual_accel_remove(struct i2c_client *client)
 {
-	struct i2c_client *second_dev = bmc150_get_second_device(client);
+	struct bmc150_accel_data *data = iio_priv(i2c_get_clientdata(client));
 
-	i2c_unregister_device(second_dev);
+	i2c_unregister_device(data->second_device);
 }
 #else
 static void bmc150_acpi_dual_accel_probe(struct i2c_client *client) {}
diff --git a/drivers/iio/accel/bmc150-accel.h b/drivers/iio/accel/bmc150-accel.h
index f503c5b5801e..5da6fd32bac5 100644
--- a/drivers/iio/accel/bmc150-accel.h
+++ b/drivers/iio/accel/bmc150-accel.h
@@ -78,8 +78,6 @@ 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 *client, struct i2c_client *second_dev);
 extern const struct dev_pm_ops bmc150_accel_pm_ops;
 extern const struct regmap_config bmc150_regmap_conf;
 
-- 
2.31.1

[PATCH v2 7/9] iio: accel: bmc150: Add support for DUAL250E ACPI DSM for setting the hinge angle

[Hans de Goede]

Some 360 degree hinges (yoga) style 2-in-1 devices use 2 bmc150 accels
to allow the OS to determine the angle between the display and the base
of the device, so that the OS can determine if the 2-in-1 is in laptop
or in tablet-mode.

On Windows both accelerometers are read (polled) by a special service
and this service calls the DSM (Device Specific Method), which in turn
translates the angles to one of laptop/tablet/tent/stand mode and then
notifies the EC about the new mode and the EC then enables or disables
the builtin keyboard and touchpad based in the mode.

When the 2-in-1 is powered-on or resumed folded in tablet mode the
EC senses this independent of the DSM by using a HALL effect sensor
which senses that the keyboard has been folded away behind the display.

At power-on or resume the EC disables the keyboard based on this and
the only way to get the keyboard to work after this is to call the
DSM to re-enable it.

Call the DSM on probe() and resume() to fix the keyboard not working
when powered-on / resumed in tablet-mode.

This patch was developed and tested on a Lenovo Yoga 300-IBR.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
- Use acpi_dev_hid_uid_match() to check that we are dealing with a
  "DUAL250E" device in bmc150_acpi_set_angle_dsm()
- Rewrap some comments at 80 chars limit
---
 drivers/iio/accel/bmc150-accel-core.c |   3 +
 drivers/iio/accel/bmc150-accel-i2c.c  | 110 ++++++++++++++++++++++++++
 drivers/iio/accel/bmc150-accel.h      |   3 +
 3 files changed, 116 insertions(+)

diff --git a/drivers/iio/accel/bmc150-accel-core.c b/drivers/iio/accel/bmc150-accel-core.c
index 0291512648b2..932007895f18 100644
--- a/drivers/iio/accel/bmc150-accel-core.c
+++ b/drivers/iio/accel/bmc150-accel-core.c
@@ -1803,6 +1803,9 @@ static int bmc150_accel_resume(struct device *dev)
 	bmc150_accel_fifo_set_mode(data);
 	mutex_unlock(&data->mutex);
 
+	if (data->resume_callback)
+		data->resume_callback(dev);
+
 	return 0;
 }
 #endif
diff --git a/drivers/iio/accel/bmc150-accel-i2c.c b/drivers/iio/accel/bmc150-accel-i2c.c
index ab0cda8ff8fa..d34dddb850d9 100644
--- a/drivers/iio/accel/bmc150-accel-i2c.c
+++ b/drivers/iio/accel/bmc150-accel-i2c.c
@@ -28,6 +28,108 @@ static const struct acpi_device_id bmc150_acpi_dual_accel_ids[] = {
 	{ }
 };
 
+/*
+ * The DUAL250E ACPI device for 360° hinges type 2-in-1s with 1 accelerometer
+ * in the display and 1 in the hinge has an ACPI-method (DSM) to tell the
+ * ACPI code about the angle between the 2 halves. This will make the ACPI
+ * code enable/disable the keyboard and touchpad. We need to call this to avoid
+ * the keyboard being disabled when the 2-in-1 is turned-on or resumed while
+ * fully folded into tablet mode (which gets detected with a HALL-sensor).
+ * If we don't call this then the keyboard won't work even when the 2-in-1 is
+ * changed to be used in laptop mode after the power-on / resume.
+ *
+ * This DSM takes 2 angles, selected by setting aux0 to 0 or 1, these presumably
+ * define the angle between the gravity vector measured by the accelerometer in
+ * the display (aux0=0) resp. the base (aux0=1) and some reference vector.
+ * The 2 angles get subtracted from each other so the reference vector does
+ * not matter and we can simply leave the second angle at 0.
+ */
+
+#define BMC150_DSM_GUID				"7681541e-8827-4239-8d9d-36be7fe12542"
+#define DUAL250E_SET_ANGLE_FN_INDEX		3
+
+struct dual250e_set_angle_args {
+	u32 aux0;
+	u32 ang0;
+	u32 rawx;
+	u32 rawy;
+	u32 rawz;
+} __packed;
+
+static bool bmc150_acpi_set_angle_dsm(struct i2c_client *client, u32 aux0, u32 ang0)
+{
+	struct acpi_device *adev = ACPI_COMPANION(&client->dev);
+	struct dual250e_set_angle_args args = {
+		.aux0 = aux0,
+		.ang0 = ang0,
+	};
+	union acpi_object args_obj, *obj;
+	guid_t guid;
+
+	if (!acpi_dev_hid_uid_match(adev, "DUAL250E", NULL))
+		return false;
+
+	guid_parse(BMC150_DSM_GUID, &guid);
+
+	if (!acpi_check_dsm(adev->handle, &guid, 0, BIT(DUAL250E_SET_ANGLE_FN_INDEX)))
+		return false;
+
+	/*
+	 * Note this triggers the following warning:
+	 * "ACPI Warning: \_SB.PCI0.I2C2.ACC1._DSM: Argument #4 type mismatch -
+	 *                Found [Buffer], ACPI requires [Package]"
+	 * This is unavoidable since the _DSM implementation expects a "naked"
+	 * buffer, so wrapping it in a package will _not_ work.
+	 */
+	args_obj.type = ACPI_TYPE_BUFFER;
+	args_obj.buffer.length = sizeof(args);
+	args_obj.buffer.pointer = (u8 *)&args;
+
+	obj = acpi_evaluate_dsm(adev->handle, &guid, 0, DUAL250E_SET_ANGLE_FN_INDEX, &args_obj);
+	if (!obj) {
+		dev_err(&client->dev, "Failed to call DSM to enable keyboard and touchpad\n");
+		return false;
+	}
+
+	ACPI_FREE(obj);
+	return true;
+}
+
+static bool bmc150_acpi_enable_keyboard(struct i2c_client *client)
+{
+	/*
+	 * The EC must see a change for it to re-enable the kbd, so first
+	 * set the angle to 270° (tent/stand mode) and then change it to
+	 * 90° (laptop mode).
+	 */
+	if (!bmc150_acpi_set_angle_dsm(client, 0, 270))
+		return false;
+
+	/* The EC needs some time to notice the angle being changed */
+	msleep(100);
+
+	return bmc150_acpi_set_angle_dsm(client, 0, 90);
+}
+
+static void bmc150_acpi_resume_work(struct work_struct *work)
+{
+	struct bmc150_accel_data *data =
+		container_of(work, struct bmc150_accel_data, resume_work.work);
+
+	bmc150_acpi_enable_keyboard(data->second_device);
+}
+
+static void bmc150_acpi_resume_handler(struct device *dev)
+{
+	struct bmc150_accel_data *data = iio_priv(dev_get_drvdata(dev));
+
+	/*
+	 * Delay the bmc150_acpi_enable_keyboard() call till after the system
+	 * resume has completed, otherwise it will not work.
+	 */
+	schedule_delayed_work(&data->resume_work, msecs_to_jiffies(1000));
+}
+
 /*
  * Some acpi_devices describe 2 accelerometers in a single ACPI device,
  * try instantiating a second i2c_client for an I2cSerialBusV2 ACPI resource
@@ -56,12 +158,20 @@ static void bmc150_acpi_dual_accel_probe(struct i2c_client *client)
 	board_info.irq = acpi_dev_gpio_irq_get(adev, 1);
 
 	data->second_device = i2c_acpi_new_device(&client->dev, 1, &board_info);
+
+	if (!IS_ERR(data->second_device) && bmc150_acpi_enable_keyboard(data->second_device)) {
+		INIT_DELAYED_WORK(&data->resume_work, bmc150_acpi_resume_work);
+		data->resume_callback = bmc150_acpi_resume_handler;
+	}
 }
 
 static void bmc150_acpi_dual_accel_remove(struct i2c_client *client)
 {
 	struct bmc150_accel_data *data = iio_priv(i2c_get_clientdata(client));
 
+	if (data->resume_callback)
+		cancel_delayed_work_sync(&data->resume_work);
+
 	i2c_unregister_device(data->second_device);
 }
 #else
diff --git a/drivers/iio/accel/bmc150-accel.h b/drivers/iio/accel/bmc150-accel.h
index 5da6fd32bac5..d67d6ed6ae77 100644
--- a/drivers/iio/accel/bmc150-accel.h
+++ b/drivers/iio/accel/bmc150-accel.h
@@ -6,6 +6,7 @@
 #include <linux/iio/iio.h>
 #include <linux/mutex.h>
 #include <linux/regulator/consumer.h>
+#include <linux/workqueue.h>
 
 struct regmap;
 struct i2c_client;
@@ -62,6 +63,8 @@ struct bmc150_accel_data {
 	int64_t timestamp, old_timestamp; /* Only used in hw fifo mode. */
 	const struct bmc150_accel_chip_info *chip_info;
 	struct i2c_client *second_device;
+	void (*resume_callback)(struct device *dev);
+	struct delayed_work resume_work;
 	struct iio_mount_matrix orientation;
 };
 
-- 
2.31.1

[PATCH v2 8/9] iio: accel: bmc150: Refactor bmc150_apply_acpi_orientation()

[Hans de Goede]

Factor the BOSC0200 ACPI HID handling out into a new
bmc150_apply_bosc0200_acpi_orientation() function and make
bmc150_apply_acpi_orientation() call that when dealing with
a BOSC0200 ACPI device (and make it return false otherwise).

This is a preparation patch for adding special handling for other
ACPI HIDs (the "DUAL250E" HID).

Suggested-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
- This is a new patch in v2 of this patch-set
---
 drivers/iio/accel/bmc150-accel-core.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/drivers/iio/accel/bmc150-accel-core.c b/drivers/iio/accel/bmc150-accel-core.c
index 932007895f18..f685ed617424 100644
--- a/drivers/iio/accel/bmc150-accel-core.c
+++ b/drivers/iio/accel/bmc150-accel-core.c
@@ -386,8 +386,8 @@ static int bmc150_accel_set_power_state(struct bmc150_accel_data *data, bool on)
  * Onda V80 plus
  * Predia Basic Tablet
  */
-static bool bmc150_apply_acpi_orientation(struct device *dev,
-					  struct iio_mount_matrix *orientation)
+static bool bmc150_apply_bosc0200_acpi_orientation(struct device *dev,
+						   struct iio_mount_matrix *orientation)
 {
 	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
 	struct iio_dev *indio_dev = dev_get_drvdata(dev);
@@ -397,9 +397,6 @@ static bool bmc150_apply_acpi_orientation(struct device *dev,
 	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";
 		label = "accel-base";
@@ -455,6 +452,17 @@ static bool bmc150_apply_acpi_orientation(struct device *dev,
 	kfree(buffer.pointer);
 	return false;
 }
+
+static bool bmc150_apply_acpi_orientation(struct device *dev,
+					  struct iio_mount_matrix *orientation)
+{
+	struct acpi_device *adev = ACPI_COMPANION(dev);
+
+	if (adev && acpi_dev_hid_uid_match(adev, "BOSC0200", NULL))
+		return bmc150_apply_bosc0200_acpi_orientation(dev, orientation);
+
+	return false;
+}
 #else
 static bool bmc150_apply_acpi_orientation(struct device *dev,
 					  struct iio_mount_matrix *orientation)
-- 
2.31.1

[PATCH v2 9/9] iio: accel: bmc150: Set label based on accel-location for ACPI DUAL250E fwnodes

[Hans de Goede]

Some Yoga laptops with 1 accelerometer in the display and 1 in the base,
use an ACPI HID of DUAL250E instead of BOSC0200.

Set the iio-device's label for DUAL250E devices to a value indicating which
sensor is which, mirroring how we do this for BOSC0200 dual sensor devices.

Note the DUAL250E fwnode unfortunately does not include a mount-matrix.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
- Rebase on top of the new "iio: accel: bmc150: Refactor
  bmc150_apply_acpi_orientation()" patch
---
 drivers/iio/accel/bmc150-accel-core.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/iio/accel/bmc150-accel-core.c b/drivers/iio/accel/bmc150-accel-core.c
index f685ed617424..95a9ef2d333e 100644
--- a/drivers/iio/accel/bmc150-accel-core.c
+++ b/drivers/iio/accel/bmc150-accel-core.c
@@ -453,6 +453,19 @@ static bool bmc150_apply_bosc0200_acpi_orientation(struct device *dev,
 	return false;
 }
 
+static bool bmc150_apply_dual250e_acpi_orientation(struct device *dev,
+						   struct iio_mount_matrix *orientation)
+{
+	struct iio_dev *indio_dev = dev_get_drvdata(dev);
+
+	if (strcmp(dev_name(dev), "i2c-DUAL250E:base") == 0)
+		indio_dev->label = "accel-base";
+	else
+		indio_dev->label = "accel-display";
+
+	return false; /* DUAL250E fwnodes have no mount matrix info */
+}
+
 static bool bmc150_apply_acpi_orientation(struct device *dev,
 					  struct iio_mount_matrix *orientation)
 {
@@ -461,6 +474,9 @@ static bool bmc150_apply_acpi_orientation(struct device *dev,
 	if (adev && acpi_dev_hid_uid_match(adev, "BOSC0200", NULL))
 		return bmc150_apply_bosc0200_acpi_orientation(dev, orientation);
 
+	if (adev && acpi_dev_hid_uid_match(adev, "DUAL250E", NULL))
+		return bmc150_apply_dual250e_acpi_orientation(dev, orientation);
+
 	return false;
 }
 #else
-- 
2.31.1

Re: [PATCH v2 0/9] iio: accel: bmc150: Add support for yoga's with dual accelerometers with an ACPI HID of DUAL250E

[Andy Shevchenko]

On Sun, May 23, 2021 at 8:01 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi all,
>
> Here is v2 of this series, addressing Andy's review remarks and
> rewrap some comments at 80 chars limit.
>
> For more info here is the v1 cover-letter:
>
> Some 360 degree hinges (yoga) style 2-in-1 devices use 2 bmc150 accels
> to allow the OS to determine the angle between the display and the base
> of the device, so that the OS can determine if the 2-in-1 is in laptop
> or in tablet-mode.
>
> We already support this setup on devices using a single ACPI node
> with a HID of "BOSC0200" to describe both accelerometers. This patch
> set extends this support to also support the same setup but then
> using a HID of "DUAL250E".
>
> While testing this I found some crashes on rmmod, patches 1-2
> fix those patches, patch 3 does some refactoring and patch 4
> adds support for the "DUAL250E" HID.
>
> Unfortunately we need some more special handling though, which the
> rest of the patches are for.
>
> On Windows both accelerometers are read (polled) by a special service
> and this service calls a DSM (Device Specific Method), which in turn
> translates the angles to one of laptop/tablet/tent/stand mode and then
> notifies the EC about the new mode and the EC then enables or disables
> the builtin keyboard and touchpad based in the mode.
>
> When the 2-in-1 is powered-on or resumed folded in tablet mode the
> EC senses this independent of the DSM by using a HALL effect sensor
> which senses that the keyboard has been folded away behind the display.
>
> At power-on or resume the EC disables the keyboard based on this and
> the only way to get the keyboard to work after this is to call the
> DSM to re-enable it (similar to how we also need to call a special
> DSM in the kxcjk-1013.c accel driver to re-enable the keyboard).
>
> Patches 5-7 deal with the DSM mess and patch 8 adds labels to the
> 2 accelerometers specifying which one is which.

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

> Regards,
>
> Hans
>
>
> Hans de Goede (9):
>   iio: accel: bmc150: Fix dereferencing the wrong pointer in
>     bmc150_get/set_second_device
>   iio: accel: bmc150: Don't make the remove function of the second
>     accelerometer unregister itself
>   iio: accel: bmc150: Move check for second ACPI device into a separate
>     function
>   iio: accel: bmc150: Add support for dual-accelerometers with a
>     DUAL250E HID
>   iio: accel: bmc150: Move struct bmc150_accel_data definition to
>     bmc150-accel.h
>   iio: accel: bmc150: Remove bmc150_set/get_second_device() accessor
>     functions
>   iio: accel: bmc150: Add support for DUAL250E ACPI DSM for setting the
>     hinge angle
>   iio: accel: bmc150: Refactor bmc150_apply_acpi_orientation()
>   iio: accel: bmc150: Set label based on accel-location for ACPI
>     DUAL250E fwnodes
>
>  drivers/iio/accel/bmc150-accel-core.c | 110 +++++----------
>  drivers/iio/accel/bmc150-accel-i2c.c  | 193 ++++++++++++++++++++++----
>  drivers/iio/accel/bmc150-accel.h      |  66 ++++++++-
>  3 files changed, 260 insertions(+), 109 deletions(-)
>
> --
> 2.31.1
>


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/9] iio: accel: bmc150: Don't make the remove function of the second accelerometer unregister itself

[Jonathan Cameron]

On Sun, 23 May 2021 19:00:56 +0200
Hans de Goede <hdegoede@redhat.com> wrote:

> On machines with dual accelerometers described in a single ACPI fwnode,
> the bmc150_accel_probe() instantiates a second i2c-client for the second
> accelerometer.
> 
> A pointer to this manually instantiated second i2c-client is stored
> inside the iio_dev's private-data through bmc150_set_second_device(),
> so that the i2c-client can be unregistered from bmc150_accel_remove().
> 
> Before this commit bmc150_set_second_device() took only 1 argument so it
> would store the pointer in private-data of the iio_dev belonging to the
> manually instantiated i2c-client, leading to the bmc150_accel_remove()
> call for the second_dev trying to unregister *itself* while it was
> being removed, leading to a deadlock and rmmod hanging.
> 
> Change bmc150_set_second_device() to take 2 arguments: 1. The i2c-client
> which is instantiating the second i2c-client for the 2nd accelerometer and
> 2. The second-device pointer itself (which also is an i2c-client).
> 
> This will store the second_device pointer in the private data of the
> iio_dev belonging to the (ACPI instantiated) i2c-client for the first
> accelerometer and will make bmc150_accel_remove() unregister the
> second_device i2c-client when called for the first client,
> avoiding the deadlock.
> 
> Fixes: 5bfb3a4bd8f6 ("iio: accel: bmc150: Check for a second ACPI device for BOSC0200")
> Cc: Jeremy Cline <jeremy@jcline.org>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Patches 1 and 2 applied to the fixes-togreg branch of iio.git and marked for stable.
The rest will have to wait for now.

Thanks,

Jonathan

> ---
>  drivers/iio/accel/bmc150-accel-core.c | 4 ++--
>  drivers/iio/accel/bmc150-accel-i2c.c  | 2 +-
>  drivers/iio/accel/bmc150-accel.h      | 2 +-
>  3 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iio/accel/bmc150-accel-core.c b/drivers/iio/accel/bmc150-accel-core.c
> index 3a3f67930165..8ff358c37a81 100644
> --- a/drivers/iio/accel/bmc150-accel-core.c
> +++ b/drivers/iio/accel/bmc150-accel-core.c
> @@ -1815,11 +1815,11 @@ struct i2c_client *bmc150_get_second_device(struct i2c_client *client)
>  }
>  EXPORT_SYMBOL_GPL(bmc150_get_second_device);
>  
> -void bmc150_set_second_device(struct i2c_client *client)
> +void bmc150_set_second_device(struct i2c_client *client, struct i2c_client *second_dev)
>  {
>  	struct bmc150_accel_data *data = iio_priv(i2c_get_clientdata(client));
>  
> -	data->second_device = client;
> +	data->second_device = second_dev;
>  }
>  EXPORT_SYMBOL_GPL(bmc150_set_second_device);
>  
> diff --git a/drivers/iio/accel/bmc150-accel-i2c.c b/drivers/iio/accel/bmc150-accel-i2c.c
> index 69f709319484..2afaae0294ee 100644
> --- a/drivers/iio/accel/bmc150-accel-i2c.c
> +++ b/drivers/iio/accel/bmc150-accel-i2c.c
> @@ -70,7 +70,7 @@ static int bmc150_accel_probe(struct i2c_client *client,
>  
>  		second_dev = i2c_acpi_new_device(&client->dev, 1, &board_info);
>  		if (!IS_ERR(second_dev))
> -			bmc150_set_second_device(second_dev);
> +			bmc150_set_second_device(client, second_dev);
>  	}
>  #endif
>  
> diff --git a/drivers/iio/accel/bmc150-accel.h b/drivers/iio/accel/bmc150-accel.h
> index 6024f15b9700..e30c1698f6fb 100644
> --- a/drivers/iio/accel/bmc150-accel.h
> +++ b/drivers/iio/accel/bmc150-accel.h
> @@ -18,7 +18,7 @@ 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);
> +void bmc150_set_second_device(struct i2c_client *client, struct i2c_client *second_dev);
>  extern const struct dev_pm_ops bmc150_accel_pm_ops;
>  extern const struct regmap_config bmc150_regmap_conf;
>  

Re: [PATCH v2 2/9] iio: accel: bmc150: Don't make the remove function of the second accelerometer unregister itself

[Jonathan Cameron]

On Wed, 26 May 2021 17:55:41 +0100
Jonathan Cameron <jic23@kernel.org> wrote:

> On Sun, 23 May 2021 19:00:56 +0200
> Hans de Goede <hdegoede@redhat.com> wrote:
> 
> > On machines with dual accelerometers described in a single ACPI fwnode,
> > the bmc150_accel_probe() instantiates a second i2c-client for the second
> > accelerometer.
> > 
> > A pointer to this manually instantiated second i2c-client is stored
> > inside the iio_dev's private-data through bmc150_set_second_device(),
> > so that the i2c-client can be unregistered from bmc150_accel_remove().
> > 
> > Before this commit bmc150_set_second_device() took only 1 argument so it
> > would store the pointer in private-data of the iio_dev belonging to the
> > manually instantiated i2c-client, leading to the bmc150_accel_remove()
> > call for the second_dev trying to unregister *itself* while it was
> > being removed, leading to a deadlock and rmmod hanging.
> > 
> > Change bmc150_set_second_device() to take 2 arguments: 1. The i2c-client
> > which is instantiating the second i2c-client for the 2nd accelerometer and
> > 2. The second-device pointer itself (which also is an i2c-client).
> > 
> > This will store the second_device pointer in the private data of the
> > iio_dev belonging to the (ACPI instantiated) i2c-client for the first
> > accelerometer and will make bmc150_accel_remove() unregister the
> > second_device i2c-client when called for the first client,
> > avoiding the deadlock.
> > 
> > Fixes: 5bfb3a4bd8f6 ("iio: accel: bmc150: Check for a second ACPI device for BOSC0200")
> > Cc: Jeremy Cline <jeremy@jcline.org>
> > Signed-off-by: Hans de Goede <hdegoede@redhat.com>  
> Patches 1 and 2 applied to the fixes-togreg branch of iio.git and marked for stable.
> The rest will have to wait for now.
Cycle has gone past rather quicker than expected, so I've changed
my mind on these two and pulled them across to the togreg branch
targeting 5.14.  That will mean the hit stable a little later (after 5.14-rc1)
but pretty much ensures the rest of the series makes it into 5.14

Thanks,

Jonathan

> 
> Thanks,
> 
> Jonathan
> 
> > ---
> >  drivers/iio/accel/bmc150-accel-core.c | 4 ++--
> >  drivers/iio/accel/bmc150-accel-i2c.c  | 2 +-
> >  drivers/iio/accel/bmc150-accel.h      | 2 +-
> >  3 files changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/iio/accel/bmc150-accel-core.c b/drivers/iio/accel/bmc150-accel-core.c
> > index 3a3f67930165..8ff358c37a81 100644
> > --- a/drivers/iio/accel/bmc150-accel-core.c
> > +++ b/drivers/iio/accel/bmc150-accel-core.c
> > @@ -1815,11 +1815,11 @@ struct i2c_client *bmc150_get_second_device(struct i2c_client *client)
> >  }
> >  EXPORT_SYMBOL_GPL(bmc150_get_second_device);
> >  
> > -void bmc150_set_second_device(struct i2c_client *client)
> > +void bmc150_set_second_device(struct i2c_client *client, struct i2c_client *second_dev)
> >  {
> >  	struct bmc150_accel_data *data = iio_priv(i2c_get_clientdata(client));
> >  
> > -	data->second_device = client;
> > +	data->second_device = second_dev;
> >  }
> >  EXPORT_SYMBOL_GPL(bmc150_set_second_device);
> >  
> > diff --git a/drivers/iio/accel/bmc150-accel-i2c.c b/drivers/iio/accel/bmc150-accel-i2c.c
> > index 69f709319484..2afaae0294ee 100644
> > --- a/drivers/iio/accel/bmc150-accel-i2c.c
> > +++ b/drivers/iio/accel/bmc150-accel-i2c.c
> > @@ -70,7 +70,7 @@ static int bmc150_accel_probe(struct i2c_client *client,
> >  
> >  		second_dev = i2c_acpi_new_device(&client->dev, 1, &board_info);
> >  		if (!IS_ERR(second_dev))
> > -			bmc150_set_second_device(second_dev);
> > +			bmc150_set_second_device(client, second_dev);
> >  	}
> >  #endif
> >  
> > diff --git a/drivers/iio/accel/bmc150-accel.h b/drivers/iio/accel/bmc150-accel.h
> > index 6024f15b9700..e30c1698f6fb 100644
> > --- a/drivers/iio/accel/bmc150-accel.h
> > +++ b/drivers/iio/accel/bmc150-accel.h
> > @@ -18,7 +18,7 @@ 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);
> > +void bmc150_set_second_device(struct i2c_client *client, struct i2c_client *second_dev);
> >  extern const struct dev_pm_ops bmc150_accel_pm_ops;
> >  extern const struct regmap_config bmc150_regmap_conf;
> >    
> 

Re: [PATCH v2 0/9] iio: accel: bmc150: Add support for yoga's with dual accelerometers with an ACPI HID of DUAL250E

[Jonathan Cameron]

On Sun, 23 May 2021 22:08:57 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Sun, May 23, 2021 at 8:01 PM Hans de Goede <hdegoede@redhat.com> wrote:
> >
> > Hi all,
> >
> > Here is v2 of this series, addressing Andy's review remarks and
> > rewrap some comments at 80 chars limit.
> >
> > For more info here is the v1 cover-letter:
> >
> > Some 360 degree hinges (yoga) style 2-in-1 devices use 2 bmc150 accels
> > to allow the OS to determine the angle between the display and the base
> > of the device, so that the OS can determine if the 2-in-1 is in laptop
> > or in tablet-mode.
> >
> > We already support this setup on devices using a single ACPI node
> > with a HID of "BOSC0200" to describe both accelerometers. This patch
> > set extends this support to also support the same setup but then
> > using a HID of "DUAL250E".
> >
> > While testing this I found some crashes on rmmod, patches 1-2
> > fix those patches, patch 3 does some refactoring and patch 4
> > adds support for the "DUAL250E" HID.
> >
> > Unfortunately we need some more special handling though, which the
> > rest of the patches are for.
> >
> > On Windows both accelerometers are read (polled) by a special service
> > and this service calls a DSM (Device Specific Method), which in turn
> > translates the angles to one of laptop/tablet/tent/stand mode and then
> > notifies the EC about the new mode and the EC then enables or disables
> > the builtin keyboard and touchpad based in the mode.
> >
> > When the 2-in-1 is powered-on or resumed folded in tablet mode the
> > EC senses this independent of the DSM by using a HALL effect sensor
> > which senses that the keyboard has been folded away behind the display.
> >
> > At power-on or resume the EC disables the keyboard based on this and
> > the only way to get the keyboard to work after this is to call the
> > DSM to re-enable it (similar to how we also need to call a special
> > DSM in the kxcjk-1013.c accel driver to re-enable the keyboard).
> >
> > Patches 5-7 deal with the DSM mess and patch 8 adds labels to the
> > 2 accelerometers specifying which one is which.  
> 
> Thanks!
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Whole series now applied to the togreg branch of iio.git and pushed out
as testing to give 0-day a chance to pick on it.

Thanks,

Jonathan

> 
> > Regards,
> >
> > Hans
> >
> >
> > Hans de Goede (9):
> >   iio: accel: bmc150: Fix dereferencing the wrong pointer in
> >     bmc150_get/set_second_device
> >   iio: accel: bmc150: Don't make the remove function of the second
> >     accelerometer unregister itself
> >   iio: accel: bmc150: Move check for second ACPI device into a separate
> >     function
> >   iio: accel: bmc150: Add support for dual-accelerometers with a
> >     DUAL250E HID
> >   iio: accel: bmc150: Move struct bmc150_accel_data definition to
> >     bmc150-accel.h
> >   iio: accel: bmc150: Remove bmc150_set/get_second_device() accessor
> >     functions
> >   iio: accel: bmc150: Add support for DUAL250E ACPI DSM for setting the
> >     hinge angle
> >   iio: accel: bmc150: Refactor bmc150_apply_acpi_orientation()
> >   iio: accel: bmc150: Set label based on accel-location for ACPI
> >     DUAL250E fwnodes
> >
> >  drivers/iio/accel/bmc150-accel-core.c | 110 +++++----------
> >  drivers/iio/accel/bmc150-accel-i2c.c  | 193 ++++++++++++++++++++++----
> >  drivers/iio/accel/bmc150-accel.h      |  66 ++++++++-
> >  3 files changed, 260 insertions(+), 109 deletions(-)
> >
> > --
> > 2.31.1
> >  
> 
> 

Re: [PATCH v2 2/9] iio: accel: bmc150: Don't make the remove function of the second accelerometer unregister itself

[Hans de Goede]

Hi,

On 6/9/21 9:49 PM, Jonathan Cameron wrote:
> On Wed, 26 May 2021 17:55:41 +0100
> Jonathan Cameron <jic23@kernel.org> wrote:
> 
>> On Sun, 23 May 2021 19:00:56 +0200
>> Hans de Goede <hdegoede@redhat.com> wrote:
>>
>>> On machines with dual accelerometers described in a single ACPI fwnode,
>>> the bmc150_accel_probe() instantiates a second i2c-client for the second
>>> accelerometer.
>>>
>>> A pointer to this manually instantiated second i2c-client is stored
>>> inside the iio_dev's private-data through bmc150_set_second_device(),
>>> so that the i2c-client can be unregistered from bmc150_accel_remove().
>>>
>>> Before this commit bmc150_set_second_device() took only 1 argument so it
>>> would store the pointer in private-data of the iio_dev belonging to the
>>> manually instantiated i2c-client, leading to the bmc150_accel_remove()
>>> call for the second_dev trying to unregister *itself* while it was
>>> being removed, leading to a deadlock and rmmod hanging.
>>>
>>> Change bmc150_set_second_device() to take 2 arguments: 1. The i2c-client
>>> which is instantiating the second i2c-client for the 2nd accelerometer and
>>> 2. The second-device pointer itself (which also is an i2c-client).
>>>
>>> This will store the second_device pointer in the private data of the
>>> iio_dev belonging to the (ACPI instantiated) i2c-client for the first
>>> accelerometer and will make bmc150_accel_remove() unregister the
>>> second_device i2c-client when called for the first client,
>>> avoiding the deadlock.
>>>
>>> Fixes: 5bfb3a4bd8f6 ("iio: accel: bmc150: Check for a second ACPI device for BOSC0200")
>>> Cc: Jeremy Cline <jeremy@jcline.org>
>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>  
>> Patches 1 and 2 applied to the fixes-togreg branch of iio.git and marked for stable.
>> The rest will have to wait for now.
> Cycle has gone past rather quicker than expected, so I've changed
> my mind on these two and pulled them across to the togreg branch
> targeting 5.14.
> That will mean the hit stable a little later (after 5.14-rc1)

Ok, note the problems fixed in the first 2 patches are only hit on
a rmmod (or an unbind), so there is no big hurry in getting them
in to the stable series.

> but pretty much ensures the rest of the series makes it into 5.14

Getting the rest of the series into 5.14 without issues is much
appreciated, thank you.

Regards,

Hans



>>> ---
>>>  drivers/iio/accel/bmc150-accel-core.c | 4 ++--
>>>  drivers/iio/accel/bmc150-accel-i2c.c  | 2 +-
>>>  drivers/iio/accel/bmc150-accel.h      | 2 +-
>>>  3 files changed, 4 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/iio/accel/bmc150-accel-core.c b/drivers/iio/accel/bmc150-accel-core.c
>>> index 3a3f67930165..8ff358c37a81 100644
>>> --- a/drivers/iio/accel/bmc150-accel-core.c
>>> +++ b/drivers/iio/accel/bmc150-accel-core.c
>>> @@ -1815,11 +1815,11 @@ struct i2c_client *bmc150_get_second_device(struct i2c_client *client)
>>>  }
>>>  EXPORT_SYMBOL_GPL(bmc150_get_second_device);
>>>  
>>> -void bmc150_set_second_device(struct i2c_client *client)
>>> +void bmc150_set_second_device(struct i2c_client *client, struct i2c_client *second_dev)
>>>  {
>>>  	struct bmc150_accel_data *data = iio_priv(i2c_get_clientdata(client));
>>>  
>>> -	data->second_device = client;
>>> +	data->second_device = second_dev;
>>>  }
>>>  EXPORT_SYMBOL_GPL(bmc150_set_second_device);
>>>  
>>> diff --git a/drivers/iio/accel/bmc150-accel-i2c.c b/drivers/iio/accel/bmc150-accel-i2c.c
>>> index 69f709319484..2afaae0294ee 100644
>>> --- a/drivers/iio/accel/bmc150-accel-i2c.c
>>> +++ b/drivers/iio/accel/bmc150-accel-i2c.c
>>> @@ -70,7 +70,7 @@ static int bmc150_accel_probe(struct i2c_client *client,
>>>  
>>>  		second_dev = i2c_acpi_new_device(&client->dev, 1, &board_info);
>>>  		if (!IS_ERR(second_dev))
>>> -			bmc150_set_second_device(second_dev);
>>> +			bmc150_set_second_device(client, second_dev);
>>>  	}
>>>  #endif
>>>  
>>> diff --git a/drivers/iio/accel/bmc150-accel.h b/drivers/iio/accel/bmc150-accel.h
>>> index 6024f15b9700..e30c1698f6fb 100644
>>> --- a/drivers/iio/accel/bmc150-accel.h
>>> +++ b/drivers/iio/accel/bmc150-accel.h
>>> @@ -18,7 +18,7 @@ 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);
>>> +void bmc150_set_second_device(struct i2c_client *client, struct i2c_client *second_dev);
>>>  extern const struct dev_pm_ops bmc150_accel_pm_ops;
>>>  extern const struct regmap_config bmc150_regmap_conf;
>>>    
>>
>