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

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

[Hans de Goede]

Hi All,

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 (8):
  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: Set label based on accel-location for ACPI
    DUAL250E fwnodes

 drivers/iio/accel/bmc150-accel-core.c |  87 ++----------
 drivers/iio/accel/bmc150-accel-i2c.c  | 192 +++++++++++++++++++++-----
 drivers/iio/accel/bmc150-accel.h      |  66 ++++++++-
 3 files changed, 239 insertions(+), 106 deletions(-)

-- 
2.31.1

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

[Hans de Goede]

Hi All,

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 (8):
  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: Set label based on accel-location for ACPI
    DUAL250E fwnodes

 drivers/iio/accel/bmc150-accel-core.c |  87 ++----------
 drivers/iio/accel/bmc150-accel-i2c.c  | 192 +++++++++++++++++++++-----
 drivers/iio/accel/bmc150-accel.h      |  66 ++++++++-
 3 files changed, 239 insertions(+), 106 deletions(-)

-- 
2.31.1

[PATCH 1/8] 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 1/8] 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 2/8] 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 2/8] 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

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

[Andy Shevchenko]

On Fri, May 21, 2021 at 11:23 PM 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.

I would rather call it aux device (at least in the code). The
terminology maybe needs more clarification (like the main one in the
block with the display panel and aux in the keyboard).

If you disagree, ignore this comment. I have no strong opinion here
since I don't know the difference between them (accelerometers).


-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Fri, May 21, 2021 at 11:23 PM 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.

I would rather call it aux device (at least in the code). The
terminology maybe needs more clarification (like the main one in the
block with the display panel and aux in the keyboard).

If you disagree, ignore this comment. I have no strong opinion here
since I don't know the difference between them (accelerometers).


-- 
With Best Regards,
Andy Shevchenko

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

[Hans de Goede]

Hi,

On 5/22/21 8:06 PM, Andy Shevchenko wrote:
> On Fri, May 21, 2021 at 11:23 PM 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.
> 
> I would rather call it aux device (at least in the code). The
> terminology maybe needs more clarification (like the main one in the
> block with the display panel and aux in the keyboard).
> 
> If you disagree, ignore this comment. I have no strong opinion here
> since I don't know the difference between them (accelerometers).

Thank you for your input, but both sensors are identical, so calling
one aux sounds of to me, so lets keep this as is.

Regards,

Hans

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

[Hans de Goede]

Hi,

On 5/22/21 8:06 PM, Andy Shevchenko wrote:
> On Fri, May 21, 2021 at 11:23 PM 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.
> 
> I would rather call it aux device (at least in the code). The
> terminology maybe needs more clarification (like the main one in the
> block with the display panel and aux in the keyboard).
> 
> If you disagree, ignore this comment. I have no strong opinion here
> since I don't know the difference between them (accelerometers).

Thank you for your input, but both sensors are identical, so calling
one aux sounds of to me, so lets keep this as is.

Regards,

Hans

[PATCH 3/8] 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>
---
 drivers/iio/accel/bmc150-accel-i2c.c | 80 +++++++++++++++++-----------
 1 file changed, 49 insertions(+), 31 deletions(-)

diff --git a/drivers/iio/accel/bmc150-accel-i2c.c b/drivers/iio/accel/bmc150-accel-i2c.c
index 2afaae0294ee..e24ce28a4660 100644
--- a/drivers/iio/accel/bmc150-accel-i2c.c
+++ b/drivers/iio/accel/bmc150-accel-i2c.c
@@ -21,6 +21,51 @@
 
 #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 +75,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);
@@ -46,42 +90,16 @@ static int bmc150_accel_probe(struct i2c_client *client,
 	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.
-	 */
-#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
+	/* The !id check avoids recursion when probe() gets called for the second client. */
+	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 3/8] 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>
---
 drivers/iio/accel/bmc150-accel-i2c.c | 80 +++++++++++++++++-----------
 1 file changed, 49 insertions(+), 31 deletions(-)

diff --git a/drivers/iio/accel/bmc150-accel-i2c.c b/drivers/iio/accel/bmc150-accel-i2c.c
index 2afaae0294ee..e24ce28a4660 100644
--- a/drivers/iio/accel/bmc150-accel-i2c.c
+++ b/drivers/iio/accel/bmc150-accel-i2c.c
@@ -21,6 +21,51 @@
 
 #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 +75,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);
@@ -46,42 +90,16 @@ static int bmc150_accel_probe(struct i2c_client *client,
 	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.
-	 */
-#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
+	/* The !id check avoids recursion when probe() gets called for the second client. */
+	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

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

[Jonathan Cameron]

On Fri, 21 May 2021 19:14:13 +0200
Hans de Goede <hdegoede@redhat.com> wrote:

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

A few places I'd like comments rewrapped on basis of still having
a minor preference for a 80 chars limit unless there is a reason to
do otherwise.

If this is all that turns up in the series, I can do that whilst
applying.

Thanks,

Jonathan


> ---
>  drivers/iio/accel/bmc150-accel-i2c.c | 80 +++++++++++++++++-----------
>  1 file changed, 49 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/iio/accel/bmc150-accel-i2c.c b/drivers/iio/accel/bmc150-accel-i2c.c
> index 2afaae0294ee..e24ce28a4660 100644
> --- a/drivers/iio/accel/bmc150-accel-i2c.c
> +++ b/drivers/iio/accel/bmc150-accel-i2c.c
> @@ -21,6 +21,51 @@
>  
>  #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

80 char wrap still preferred when it doesn't otherwise hurt readability.

> + * 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.

Given the lesser indent after pulling this out into a new function, it would
be good to rewrap this text as nearer to 80 chars.

> +		 */
> +		.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 +75,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);
> @@ -46,42 +90,16 @@ static int bmc150_accel_probe(struct i2c_client *client,
>  	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.
> -	 */
> -#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
> +	/* The !id check avoids recursion when probe() gets called for the second client. */

Won't hurt readability to wrap this to 80 chars as a multiline comment.

> +	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);
>  }

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

[Jonathan Cameron]

On Fri, 21 May 2021 19:14:13 +0200
Hans de Goede <hdegoede@redhat.com> wrote:

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

A few places I'd like comments rewrapped on basis of still having
a minor preference for a 80 chars limit unless there is a reason to
do otherwise.

If this is all that turns up in the series, I can do that whilst
applying.

Thanks,

Jonathan


> ---
>  drivers/iio/accel/bmc150-accel-i2c.c | 80 +++++++++++++++++-----------
>  1 file changed, 49 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/iio/accel/bmc150-accel-i2c.c b/drivers/iio/accel/bmc150-accel-i2c.c
> index 2afaae0294ee..e24ce28a4660 100644
> --- a/drivers/iio/accel/bmc150-accel-i2c.c
> +++ b/drivers/iio/accel/bmc150-accel-i2c.c
> @@ -21,6 +21,51 @@
>  
>  #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

80 char wrap still preferred when it doesn't otherwise hurt readability.

> + * 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.

Given the lesser indent after pulling this out into a new function, it would
be good to rewrap this text as nearer to 80 chars.

> +		 */
> +		.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 +75,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);
> @@ -46,42 +90,16 @@ static int bmc150_accel_probe(struct i2c_client *client,
>  	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.
> -	 */
> -#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
> +	/* The !id check avoids recursion when probe() gets called for the second client. */

Won't hurt readability to wrap this to 80 chars as a multiline comment.

> +	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);
>  }

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

[Hans de Goede]

Hi,

On 5/22/21 7:37 PM, Jonathan Cameron wrote:
> On Fri, 21 May 2021 19:14:13 +0200
> Hans de Goede <hdegoede@redhat.com> wrote:
> 
>> 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>
> 
> A few places I'd like comments rewrapped on basis of still having
> a minor preference for a 80 chars limit unless there is a reason to
> do otherwise.
> 
> If this is all that turns up in the series, I can do that whilst
> applying.

Thank for the review.

If you can do the comment rewrapping while applying that would be
great, thanks. If a v2 is necessary I will take care of the 
rewrapping myself.

Regards,

Hans



> 
> Thanks,
> 
> Jonathan
> 
> 
>> ---
>>  drivers/iio/accel/bmc150-accel-i2c.c | 80 +++++++++++++++++-----------
>>  1 file changed, 49 insertions(+), 31 deletions(-)
>>
>> diff --git a/drivers/iio/accel/bmc150-accel-i2c.c b/drivers/iio/accel/bmc150-accel-i2c.c
>> index 2afaae0294ee..e24ce28a4660 100644
>> --- a/drivers/iio/accel/bmc150-accel-i2c.c
>> +++ b/drivers/iio/accel/bmc150-accel-i2c.c
>> @@ -21,6 +21,51 @@
>>  
>>  #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
> 
> 80 char wrap still preferred when it doesn't otherwise hurt readability.
> 
>> + * 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.
> 
> Given the lesser indent after pulling this out into a new function, it would
> be good to rewrap this text as nearer to 80 chars.
> 
>> +		 */
>> +		.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 +75,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);
>> @@ -46,42 +90,16 @@ static int bmc150_accel_probe(struct i2c_client *client,
>>  	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.
>> -	 */
>> -#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
>> +	/* The !id check avoids recursion when probe() gets called for the second client. */
> 
> Won't hurt readability to wrap this to 80 chars as a multiline comment.
> 
>> +	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);
>>  }
> 

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

[Hans de Goede]

Hi,

On 5/22/21 7:37 PM, Jonathan Cameron wrote:
> On Fri, 21 May 2021 19:14:13 +0200
> Hans de Goede <hdegoede@redhat.com> wrote:
> 
>> 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>
> 
> A few places I'd like comments rewrapped on basis of still having
> a minor preference for a 80 chars limit unless there is a reason to
> do otherwise.
> 
> If this is all that turns up in the series, I can do that whilst
> applying.

Thank for the review.

If you can do the comment rewrapping while applying that would be
great, thanks. If a v2 is necessary I will take care of the 
rewrapping myself.

Regards,

Hans



> 
> Thanks,
> 
> Jonathan
> 
> 
>> ---
>>  drivers/iio/accel/bmc150-accel-i2c.c | 80 +++++++++++++++++-----------
>>  1 file changed, 49 insertions(+), 31 deletions(-)
>>
>> diff --git a/drivers/iio/accel/bmc150-accel-i2c.c b/drivers/iio/accel/bmc150-accel-i2c.c
>> index 2afaae0294ee..e24ce28a4660 100644
>> --- a/drivers/iio/accel/bmc150-accel-i2c.c
>> +++ b/drivers/iio/accel/bmc150-accel-i2c.c
>> @@ -21,6 +21,51 @@
>>  
>>  #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
> 
> 80 char wrap still preferred when it doesn't otherwise hurt readability.
> 
>> + * 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.
> 
> Given the lesser indent after pulling this out into a new function, it would
> be good to rewrap this text as nearer to 80 chars.
> 
>> +		 */
>> +		.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 +75,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);
>> @@ -46,42 +90,16 @@ static int bmc150_accel_probe(struct i2c_client *client,
>>  	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.
>> -	 */
>> -#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
>> +	/* The !id check avoids recursion when probe() gets called for the second client. */
> 
> Won't hurt readability to wrap this to 80 chars as a multiline comment.
> 
>> +	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);
>>  }
> 

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

[Andy Shevchenko]

On Fri, May 21, 2021 at 11:23 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> 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.

...

> +#ifdef CONFIG_ACPI
> +static const struct acpi_device_id bmc150_acpi_dual_accel_ids[] = {
> +       {"BOSC0200"},

> +       { },

I guess it is a good chance to drop a comma.

> +};

...

> +       if (!IS_ERR(second_dev))
> +               bmc150_set_second_device(client, second_dev);

I would comment on the pattern here, but I have noticed that this code
is changed in the further patches anyway.

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Fri, May 21, 2021 at 11:23 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> 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.

...

> +#ifdef CONFIG_ACPI
> +static const struct acpi_device_id bmc150_acpi_dual_accel_ids[] = {
> +       {"BOSC0200"},

> +       { },

I guess it is a good chance to drop a comma.

> +};

...

> +       if (!IS_ERR(second_dev))
> +               bmc150_set_second_device(client, second_dev);

I would comment on the pattern here, but I have noticed that this code
is changed in the further patches anyway.

-- 
With Best Regards,
Andy Shevchenko

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

[Hans de Goede]

Hi,

On 5/22/21 8:09 PM, Andy Shevchenko wrote:
> On Fri, May 21, 2021 at 11:23 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> 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.
> 
> ...
> 
>> +#ifdef CONFIG_ACPI
>> +static const struct acpi_device_id bmc150_acpi_dual_accel_ids[] = {
>> +       {"BOSC0200"},
> 
>> +       { },
> 
> I guess it is a good chance to drop a comma.

Ack, will drop for v2.

Regards,

Hans


> 
>> +};
> 
> ...
> 
>> +       if (!IS_ERR(second_dev))
>> +               bmc150_set_second_device(client, second_dev);
> 
> I would comment on the pattern here, but I have noticed that this code
> is changed in the further patches anyway.
> 

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

[Hans de Goede]

Hi,

On 5/22/21 8:09 PM, Andy Shevchenko wrote:
> On Fri, May 21, 2021 at 11:23 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> 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.
> 
> ...
> 
>> +#ifdef CONFIG_ACPI
>> +static const struct acpi_device_id bmc150_acpi_dual_accel_ids[] = {
>> +       {"BOSC0200"},
> 
>> +       { },
> 
> I guess it is a good chance to drop a comma.

Ack, will drop for v2.

Regards,

Hans


> 
>> +};
> 
> ...
> 
>> +       if (!IS_ERR(second_dev))
>> +               bmc150_set_second_device(client, second_dev);
> 
> I would comment on the pattern here, but I have noticed that this code
> is changed in the further patches anyway.
> 

[PATCH 4/8] 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>
---
 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 e24ce28a4660..b81e4005788e 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"},
 	{ },
 };
 
@@ -35,21 +36,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 in it.
+	 */
+	snprintf(dev_name, sizeof(dev_name), "%s:base", acpi_device_hid(adev));
+
+	board_info.irq = acpi_dev_gpio_irq_get_by(adev, NULL, 1);
+
 	second_dev = i2c_acpi_new_device(&client->dev, 1, &board_info);
 	if (!IS_ERR(second_dev))
 		bmc150_set_second_device(client, second_dev);
@@ -114,6 +118,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 4/8] 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>
---
 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 e24ce28a4660..b81e4005788e 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"},
 	{ },
 };
 
@@ -35,21 +36,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 in it.
+	 */
+	snprintf(dev_name, sizeof(dev_name), "%s:base", acpi_device_hid(adev));
+
+	board_info.irq = acpi_dev_gpio_irq_get_by(adev, NULL, 1);
+
 	second_dev = i2c_acpi_new_device(&client->dev, 1, &board_info);
 	if (!IS_ERR(second_dev))
 		bmc150_set_second_device(client, second_dev);
@@ -114,6 +118,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

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

[Jonathan Cameron]

On Fri, 21 May 2021 19:14:14 +0200
Hans de Goede <hdegoede@redhat.com> wrote:

> 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>
> ---
>  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 e24ce28a4660..b81e4005788e 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"},
>  	{ },
>  };
>  
> @@ -35,21 +36,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];

I'm a bit in two minds about having a fixed length array for this.
Obviously this is always big enough (I think a bit too big), but it
might be a place where a future bug is introduced.  Perhaps it's worth the dance
of a kasprintf and kfree, to avoid that possibility?

>  	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 in it.
> +	 */
> +	snprintf(dev_name, sizeof(dev_name), "%s:base", acpi_device_hid(adev));
> +
> +	board_info.irq = acpi_dev_gpio_irq_get_by(adev, NULL, 1);
> +
>  	second_dev = i2c_acpi_new_device(&client->dev, 1, &board_info);
>  	if (!IS_ERR(second_dev))
>  		bmc150_set_second_device(client, second_dev);
> @@ -114,6 +118,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);

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

[Jonathan Cameron]

On Fri, 21 May 2021 19:14:14 +0200
Hans de Goede <hdegoede@redhat.com> wrote:

> 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>
> ---
>  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 e24ce28a4660..b81e4005788e 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"},
>  	{ },
>  };
>  
> @@ -35,21 +36,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];

I'm a bit in two minds about having a fixed length array for this.
Obviously this is always big enough (I think a bit too big), but it
might be a place where a future bug is introduced.  Perhaps it's worth the dance
of a kasprintf and kfree, to avoid that possibility?

>  	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 in it.
> +	 */
> +	snprintf(dev_name, sizeof(dev_name), "%s:base", acpi_device_hid(adev));
> +
> +	board_info.irq = acpi_dev_gpio_irq_get_by(adev, NULL, 1);
> +
>  	second_dev = i2c_acpi_new_device(&client->dev, 1, &board_info);
>  	if (!IS_ERR(second_dev))
>  		bmc150_set_second_device(client, second_dev);
> @@ -114,6 +118,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);

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

[Hans de Goede]

Hi,

On 5/22/21 7:43 PM, Jonathan Cameron wrote:
> On Fri, 21 May 2021 19:14:14 +0200
> Hans de Goede <hdegoede@redhat.com> wrote:
> 
>> 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>
>> ---
>>  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 e24ce28a4660..b81e4005788e 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"},
>>  	{ },
>>  };
>>  
>> @@ -35,21 +36,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];
> 
> I'm a bit in two minds about having a fixed length array for this.
> Obviously this is always big enough (I think a bit too big), but it
> might be a place where a future bug is introduced.  Perhaps it's worth the dance
> of a kasprintf and kfree, to avoid that possibility?

I would prefer to keep this as is, using malloc + free always leads
to problems if an error-exit path shows up between the 2.

But if you've a strong preference for switching to
kasprintf + kfree I can do that for v2.

Regards,

Hans



> 
>>  	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 in it.
>> +	 */
>> +	snprintf(dev_name, sizeof(dev_name), "%s:base", acpi_device_hid(adev));
>> +
>> +	board_info.irq = acpi_dev_gpio_irq_get_by(adev, NULL, 1);
>> +
>>  	second_dev = i2c_acpi_new_device(&client->dev, 1, &board_info);
>>  	if (!IS_ERR(second_dev))
>>  		bmc150_set_second_device(client, second_dev);
>> @@ -114,6 +118,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);
> 

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

[Hans de Goede]

Hi,

On 5/22/21 7:43 PM, Jonathan Cameron wrote:
> On Fri, 21 May 2021 19:14:14 +0200
> Hans de Goede <hdegoede@redhat.com> wrote:
> 
>> 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>
>> ---
>>  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 e24ce28a4660..b81e4005788e 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"},
>>  	{ },
>>  };
>>  
>> @@ -35,21 +36,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];
> 
> I'm a bit in two minds about having a fixed length array for this.
> Obviously this is always big enough (I think a bit too big), but it
> might be a place where a future bug is introduced.  Perhaps it's worth the dance
> of a kasprintf and kfree, to avoid that possibility?

I would prefer to keep this as is, using malloc + free always leads
to problems if an error-exit path shows up between the 2.

But if you've a strong preference for switching to
kasprintf + kfree I can do that for v2.

Regards,

Hans



> 
>>  	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 in it.
>> +	 */
>> +	snprintf(dev_name, sizeof(dev_name), "%s:base", acpi_device_hid(adev));
>> +
>> +	board_info.irq = acpi_dev_gpio_irq_get_by(adev, NULL, 1);
>> +
>>  	second_dev = i2c_acpi_new_device(&client->dev, 1, &board_info);
>>  	if (!IS_ERR(second_dev))
>>  		bmc150_set_second_device(client, second_dev);
>> @@ -114,6 +118,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);
> 

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

[Jonathan Cameron]

On Sat, 22 May 2021 19:44:55 +0200
Hans de Goede <hdegoede@redhat.com> wrote:

> Hi,
> 
> On 5/22/21 7:43 PM, Jonathan Cameron wrote:
> > On Fri, 21 May 2021 19:14:14 +0200
> > Hans de Goede <hdegoede@redhat.com> wrote:
> >   
> >> 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>
> >> ---
> >>  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 e24ce28a4660..b81e4005788e 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"},
> >>  	{ },
> >>  };
> >>  
> >> @@ -35,21 +36,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];  
> > 
> > I'm a bit in two minds about having a fixed length array for this.
> > Obviously this is always big enough (I think a bit too big), but it
> > might be a place where a future bug is introduced.  Perhaps it's worth the dance
> > of a kasprintf and kfree, to avoid that possibility?  
> 
> I would prefer to keep this as is, using malloc + free always leads
> to problems if an error-exit path shows up between the 2.
> 
> But if you've a strong preference for switching to
> kasprintf + kfree I can do that for v2.

Lets leave it as is and I get to be smug if we ever get a bug as a result
(given that way the one you suggest can't happen, so I can't be proved wrong :)

J
> 
> Regards,
> 
> Hans
> 
> 
> 
> >   
> >>  	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 in it.
> >> +	 */
> >> +	snprintf(dev_name, sizeof(dev_name), "%s:base", acpi_device_hid(adev));
> >> +
> >> +	board_info.irq = acpi_dev_gpio_irq_get_by(adev, NULL, 1);
> >> +
> >>  	second_dev = i2c_acpi_new_device(&client->dev, 1, &board_info);
> >>  	if (!IS_ERR(second_dev))
> >>  		bmc150_set_second_device(client, second_dev);
> >> @@ -114,6 +118,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);  
> >   
> 

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

[Jonathan Cameron]

On Sat, 22 May 2021 19:44:55 +0200
Hans de Goede <hdegoede@redhat.com> wrote:

> Hi,
> 
> On 5/22/21 7:43 PM, Jonathan Cameron wrote:
> > On Fri, 21 May 2021 19:14:14 +0200
> > Hans de Goede <hdegoede@redhat.com> wrote:
> >   
> >> 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>
> >> ---
> >>  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 e24ce28a4660..b81e4005788e 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"},
> >>  	{ },
> >>  };
> >>  
> >> @@ -35,21 +36,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];  
> > 
> > I'm a bit in two minds about having a fixed length array for this.
> > Obviously this is always big enough (I think a bit too big), but it
> > might be a place where a future bug is introduced.  Perhaps it's worth the dance
> > of a kasprintf and kfree, to avoid that possibility?  
> 
> I would prefer to keep this as is, using malloc + free always leads
> to problems if an error-exit path shows up between the 2.
> 
> But if you've a strong preference for switching to
> kasprintf + kfree I can do that for v2.

Lets leave it as is and I get to be smug if we ever get a bug as a result
(given that way the one you suggest can't happen, so I can't be proved wrong :)

J
> 
> Regards,
> 
> Hans
> 
> 
> 
> >   
> >>  	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 in it.
> >> +	 */
> >> +	snprintf(dev_name, sizeof(dev_name), "%s:base", acpi_device_hid(adev));
> >> +
> >> +	board_info.irq = acpi_dev_gpio_irq_get_by(adev, NULL, 1);
> >> +
> >>  	second_dev = i2c_acpi_new_device(&client->dev, 1, &board_info);
> >>  	if (!IS_ERR(second_dev))
> >>  		bmc150_set_second_device(client, second_dev);
> >> @@ -114,6 +118,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);  
> >   
> 

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

[Andy Shevchenko]

On Fri, May 21, 2021 at 11:23 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> 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.

...

> +       board_info.irq = acpi_dev_gpio_irq_get_by(adev, NULL, 1);

If NULL will be there after the series, why not to use
acpi_dev_gpio_irq_get() directly?

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Fri, May 21, 2021 at 11:23 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> 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.

...

> +       board_info.irq = acpi_dev_gpio_irq_get_by(adev, NULL, 1);

If NULL will be there after the series, why not to use
acpi_dev_gpio_irq_get() directly?

-- 
With Best Regards,
Andy Shevchenko

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

[Hans de Goede]

Hi,

On 5/22/21 8:11 PM, Andy Shevchenko wrote:
> On Fri, May 21, 2021 at 11:23 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> 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.
> 
> ...
> 
>> +       board_info.irq = acpi_dev_gpio_irq_get_by(adev, NULL, 1);
> 
> If NULL will be there after the series, why not to use
> acpi_dev_gpio_irq_get() directly?

I looked in drivers/gpio/gpiolib-acpi.c to see what is available
and that is an inline helper in include/linux/acpi.h, so I missed
it. I'll switch over in v2.

Regards,

Hans

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

[Hans de Goede]

Hi,

On 5/22/21 8:11 PM, Andy Shevchenko wrote:
> On Fri, May 21, 2021 at 11:23 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> 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.
> 
> ...
> 
>> +       board_info.irq = acpi_dev_gpio_irq_get_by(adev, NULL, 1);
> 
> If NULL will be there after the series, why not to use
> acpi_dev_gpio_irq_get() directly?

I looked in drivers/gpio/gpiolib-acpi.c to see what is available
and that is an inline helper in include/linux/acpi.h, so I missed
it. I'll switch over in v2.

Regards,

Hans

[PATCH 5/8] 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 5/8] 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 6/8] 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 b81e4005788e..1dd7b8a9a382 100644
--- a/drivers/iio/accel/bmc150-accel-i2c.c
+++ b/drivers/iio/accel/bmc150-accel-i2c.c
@@ -34,8 +34,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",
@@ -54,16 +54,14 @@ static void bmc150_acpi_dual_accel_probe(struct i2c_client *client)
 
 	board_info.irq = acpi_dev_gpio_irq_get_by(adev, NULL, 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 6/8] 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 b81e4005788e..1dd7b8a9a382 100644
--- a/drivers/iio/accel/bmc150-accel-i2c.c
+++ b/drivers/iio/accel/bmc150-accel-i2c.c
@@ -34,8 +34,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",
@@ -54,16 +54,14 @@ static void bmc150_acpi_dual_accel_probe(struct i2c_client *client)
 
 	board_info.irq = acpi_dev_gpio_irq_get_by(adev, NULL, 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 7/8] 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>
---
 drivers/iio/accel/bmc150-accel-core.c |   3 +
 drivers/iio/accel/bmc150-accel-i2c.c  | 109 ++++++++++++++++++++++++++
 drivers/iio/accel/bmc150-accel.h      |   3 +
 3 files changed, 115 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 1dd7b8a9a382..31256c32a33c 100644
--- a/drivers/iio/accel/bmc150-accel-i2c.c
+++ b/drivers/iio/accel/bmc150-accel-i2c.c
@@ -28,6 +28,107 @@ 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 (strcmp(acpi_device_hid(adev), "DUAL250E"))
+		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 with index 1.
@@ -55,12 +156,20 @@ static void bmc150_acpi_dual_accel_probe(struct i2c_client *client)
 	board_info.irq = acpi_dev_gpio_irq_get_by(adev, NULL, 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 7/8] 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>
---
 drivers/iio/accel/bmc150-accel-core.c |   3 +
 drivers/iio/accel/bmc150-accel-i2c.c  | 109 ++++++++++++++++++++++++++
 drivers/iio/accel/bmc150-accel.h      |   3 +
 3 files changed, 115 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 1dd7b8a9a382..31256c32a33c 100644
--- a/drivers/iio/accel/bmc150-accel-i2c.c
+++ b/drivers/iio/accel/bmc150-accel-i2c.c
@@ -28,6 +28,107 @@ 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 (strcmp(acpi_device_hid(adev), "DUAL250E"))
+		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 with index 1.
@@ -55,12 +156,20 @@ static void bmc150_acpi_dual_accel_probe(struct i2c_client *client)
 	board_info.irq = acpi_dev_gpio_irq_get_by(adev, NULL, 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

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

[Jonathan Cameron]

On Fri, 21 May 2021 19:14:17 +0200
Hans de Goede <hdegoede@redhat.com> wrote:

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

Putting aside my general grumpiness at this stuff having to pollute
accelerometer drivers (and the broken DSM implementation on the hardware!)
this is a fairly clean implementation so I guess we can survive it.

Jonathan

> ---
>  drivers/iio/accel/bmc150-accel-core.c |   3 +
>  drivers/iio/accel/bmc150-accel-i2c.c  | 109 ++++++++++++++++++++++++++
>  drivers/iio/accel/bmc150-accel.h      |   3 +
>  3 files changed, 115 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 1dd7b8a9a382..31256c32a33c 100644
> --- a/drivers/iio/accel/bmc150-accel-i2c.c
> +++ b/drivers/iio/accel/bmc150-accel-i2c.c
> @@ -28,6 +28,107 @@ 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 (strcmp(acpi_device_hid(adev), "DUAL250E"))
> +		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.

ouch.

> +	 */
> +	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 with index 1.
> @@ -55,12 +156,20 @@ static void bmc150_acpi_dual_accel_probe(struct i2c_client *client)
>  	board_info.irq = acpi_dev_gpio_irq_get_by(adev, NULL, 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;
>  };
>  

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

[Jonathan Cameron]

On Fri, 21 May 2021 19:14:17 +0200
Hans de Goede <hdegoede@redhat.com> wrote:

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

Putting aside my general grumpiness at this stuff having to pollute
accelerometer drivers (and the broken DSM implementation on the hardware!)
this is a fairly clean implementation so I guess we can survive it.

Jonathan

> ---
>  drivers/iio/accel/bmc150-accel-core.c |   3 +
>  drivers/iio/accel/bmc150-accel-i2c.c  | 109 ++++++++++++++++++++++++++
>  drivers/iio/accel/bmc150-accel.h      |   3 +
>  3 files changed, 115 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 1dd7b8a9a382..31256c32a33c 100644
> --- a/drivers/iio/accel/bmc150-accel-i2c.c
> +++ b/drivers/iio/accel/bmc150-accel-i2c.c
> @@ -28,6 +28,107 @@ 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 (strcmp(acpi_device_hid(adev), "DUAL250E"))
> +		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.

ouch.

> +	 */
> +	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 with index 1.
> @@ -55,12 +156,20 @@ static void bmc150_acpi_dual_accel_probe(struct i2c_client *client)
>  	board_info.irq = acpi_dev_gpio_irq_get_by(adev, NULL, 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;
>  };
>  

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

[Andy Shevchenko]

On Fri, May 21, 2021 at 11:22 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> 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.

...

> +       if (strcmp(acpi_device_hid(adev), "DUAL250E"))

Can we use like in the other case in this series the acpi_dev_hid_uid_match() ?

Because it's actually what you are doing here and it may be better to
see the same approach for this HID done in different places in the
code to recognize what it is about.

> +               return false;

...

> +       /*
> +        * 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);

I feel that you conducted a research and answer to the following will
be no, but...

Do we have any means of polling something (embedded controller / ASL /
etc) to actually see the ACK for the action?

> +       return bmc150_acpi_set_angle_dsm(client, 0, 90);

...

> +       schedule_delayed_work(&data->resume_work, msecs_to_jiffies(1000));

Isn't it the same as 1 * HZ ?

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Fri, May 21, 2021 at 11:22 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> 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.

...

> +       if (strcmp(acpi_device_hid(adev), "DUAL250E"))

Can we use like in the other case in this series the acpi_dev_hid_uid_match() ?

Because it's actually what you are doing here and it may be better to
see the same approach for this HID done in different places in the
code to recognize what it is about.

> +               return false;

...

> +       /*
> +        * 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);

I feel that you conducted a research and answer to the following will
be no, but...

Do we have any means of polling something (embedded controller / ASL /
etc) to actually see the ACK for the action?

> +       return bmc150_acpi_set_angle_dsm(client, 0, 90);

...

> +       schedule_delayed_work(&data->resume_work, msecs_to_jiffies(1000));

Isn't it the same as 1 * HZ ?

-- 
With Best Regards,
Andy Shevchenko

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

[Hans de Goede]

Hi,

On 5/22/21 8:21 PM, Andy Shevchenko wrote:
> On Fri, May 21, 2021 at 11:22 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> 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.
> 
> ...
> 
>> +       if (strcmp(acpi_device_hid(adev), "DUAL250E"))
> 
> Can we use like in the other case in this series the acpi_dev_hid_uid_match() ?

I agree that that would be more consistent, I'll fix this for 2.


> Because it's actually what you are doing here and it may be better to
> see the same approach for this HID done in different places in the
> code to recognize what it is about.
> 
>> +               return false;
> 
> ...
> 
>> +       /*
>> +        * 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);
> 
> I feel that you conducted a research and answer to the following will
> be no, but...
> 
> Do we have any means of polling something (embedded controller / ASL /
> etc) to actually see the ACK for the action?

Nope, there is nothing to poll and I tried shorter time-outs and that
lead to the EC sometimes not seeing the change.

> 
>> +       return bmc150_acpi_set_angle_dsm(client, 0, 90);
> 
> ...
> 
>> +       schedule_delayed_work(&data->resume_work, msecs_to_jiffies(1000));
> 
> Isn't it the same as 1 * HZ ?

Yes, but this makes it clearer that the delay is 1 second, IMHO using n * HZ
is harder to read / reason about then having the delay right there in msecs.
This also means less churn if it needs to change to a different amount of msecs
later.

Regards,

Hans

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

[Hans de Goede]

Hi,

On 5/22/21 8:21 PM, Andy Shevchenko wrote:
> On Fri, May 21, 2021 at 11:22 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> 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.
> 
> ...
> 
>> +       if (strcmp(acpi_device_hid(adev), "DUAL250E"))
> 
> Can we use like in the other case in this series the acpi_dev_hid_uid_match() ?

I agree that that would be more consistent, I'll fix this for 2.


> Because it's actually what you are doing here and it may be better to
> see the same approach for this HID done in different places in the
> code to recognize what it is about.
> 
>> +               return false;
> 
> ...
> 
>> +       /*
>> +        * 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);
> 
> I feel that you conducted a research and answer to the following will
> be no, but...
> 
> Do we have any means of polling something (embedded controller / ASL /
> etc) to actually see the ACK for the action?

Nope, there is nothing to poll and I tried shorter time-outs and that
lead to the EC sometimes not seeing the change.

> 
>> +       return bmc150_acpi_set_angle_dsm(client, 0, 90);
> 
> ...
> 
>> +       schedule_delayed_work(&data->resume_work, msecs_to_jiffies(1000));
> 
> Isn't it the same as 1 * HZ ?

Yes, but this makes it clearer that the delay is 1 second, IMHO using n * HZ
is harder to read / reason about then having the delay right there in msecs.
This also means less churn if it needs to change to a different amount of msecs
later.

Regards,

Hans

[PATCH 8/8] 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>
---
 drivers/iio/accel/bmc150-accel-core.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/iio/accel/bmc150-accel-core.c b/drivers/iio/accel/bmc150-accel-core.c
index 932007895f18..08966ee82e43 100644
--- a/drivers/iio/accel/bmc150-accel-core.c
+++ b/drivers/iio/accel/bmc150-accel-core.c
@@ -397,6 +397,17 @@ static bool bmc150_apply_acpi_orientation(struct device *dev,
 	acpi_status status;
 	int i, j, val[3];
 
+	/* Special case for devices with a "DUAL250E" HID */
+	if (adev && acpi_dev_hid_uid_match(adev, "DUAL250E", NULL)) {
+		if (strcmp(dev_name(dev), "i2c-DUAL250E:base") == 0)
+			label = "accel-base";
+		else
+			label = "accel-display";
+
+		indio_dev->label = label;
+		return false; /* DUAL250E fwnodes have no mount matrix info */
+	}
+
 	if (!adev || !acpi_dev_hid_uid_match(adev, "BOSC0200", NULL))
 		return false;
 
-- 
2.31.1

[PATCH 8/8] 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>
---
 drivers/iio/accel/bmc150-accel-core.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/iio/accel/bmc150-accel-core.c b/drivers/iio/accel/bmc150-accel-core.c
index 932007895f18..08966ee82e43 100644
--- a/drivers/iio/accel/bmc150-accel-core.c
+++ b/drivers/iio/accel/bmc150-accel-core.c
@@ -397,6 +397,17 @@ static bool bmc150_apply_acpi_orientation(struct device *dev,
 	acpi_status status;
 	int i, j, val[3];
 
+	/* Special case for devices with a "DUAL250E" HID */
+	if (adev && acpi_dev_hid_uid_match(adev, "DUAL250E", NULL)) {
+		if (strcmp(dev_name(dev), "i2c-DUAL250E:base") == 0)
+			label = "accel-base";
+		else
+			label = "accel-display";
+
+		indio_dev->label = label;
+		return false; /* DUAL250E fwnodes have no mount matrix info */
+	}
+
 	if (!adev || !acpi_dev_hid_uid_match(adev, "BOSC0200", NULL))
 		return false;
 
-- 
2.31.1

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

[Andy Shevchenko]

On Fri, May 21, 2021 at 11:23 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> 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.

> +       /* Special case for devices with a "DUAL250E" HID */
> +       if (adev && acpi_dev_hid_uid_match(adev, "DUAL250E", NULL)) {
> +               if (strcmp(dev_name(dev), "i2c-DUAL250E:base") == 0)
> +                       label = "accel-base";
> +               else
> +                       label = "accel-display";
> +
> +               indio_dev->label = label;
> +               return false; /* DUAL250E fwnodes have no mount matrix info */
> +       }
> +
>         if (!adev || !acpi_dev_hid_uid_match(adev, "BOSC0200", NULL))
>                 return false;


This sounds to me like

_apply_orientation_generic()
...

_apply_orientation_dual250e()


_apply_orientation()

if ()
  return _apply_orientation_generic()

if ()
 return _apply_orientation_dual250e

return false;



--
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Fri, May 21, 2021 at 11:23 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> 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.

> +       /* Special case for devices with a "DUAL250E" HID */
> +       if (adev && acpi_dev_hid_uid_match(adev, "DUAL250E", NULL)) {
> +               if (strcmp(dev_name(dev), "i2c-DUAL250E:base") == 0)
> +                       label = "accel-base";
> +               else
> +                       label = "accel-display";
> +
> +               indio_dev->label = label;
> +               return false; /* DUAL250E fwnodes have no mount matrix info */
> +       }
> +
>         if (!adev || !acpi_dev_hid_uid_match(adev, "BOSC0200", NULL))
>                 return false;


This sounds to me like

_apply_orientation_generic()
...

_apply_orientation_dual250e()


_apply_orientation()

if ()
  return _apply_orientation_generic()

if ()
 return _apply_orientation_dual250e

return false;



--
With Best Regards,
Andy Shevchenko

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

[Hans de Goede]

Hi,

On 5/22/21 8:34 PM, Andy Shevchenko wrote:
> On Fri, May 21, 2021 at 11:23 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> 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.
> 
>> +       /* Special case for devices with a "DUAL250E" HID */
>> +       if (adev && acpi_dev_hid_uid_match(adev, "DUAL250E", NULL)) {
>> +               if (strcmp(dev_name(dev), "i2c-DUAL250E:base") == 0)
>> +                       label = "accel-base";
>> +               else
>> +                       label = "accel-display";
>> +
>> +               indio_dev->label = label;
>> +               return false; /* DUAL250E fwnodes have no mount matrix info */
>> +       }
>> +
>>         if (!adev || !acpi_dev_hid_uid_match(adev, "BOSC0200", NULL))
>>                 return false;
> 
> 
> This sounds to me like
> 
> _apply_orientation_generic()
> ...
> 
> _apply_orientation_dual250e()
> 
> 
> _apply_orientation()
> 
> if ()
>   return _apply_orientation_generic()
> 
> if ()
>  return _apply_orientation_dual250e
> 
> return false;

Good point, I'll give that a try for v2 and see if I like the end result
of that. If it turns out to be a bit ugly I'll just stick with
what is in v1.

Regards,

Hans

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

[Hans de Goede]

Hi,

On 5/22/21 8:34 PM, Andy Shevchenko wrote:
> On Fri, May 21, 2021 at 11:23 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> 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.
> 
>> +       /* Special case for devices with a "DUAL250E" HID */
>> +       if (adev && acpi_dev_hid_uid_match(adev, "DUAL250E", NULL)) {
>> +               if (strcmp(dev_name(dev), "i2c-DUAL250E:base") == 0)
>> +                       label = "accel-base";
>> +               else
>> +                       label = "accel-display";
>> +
>> +               indio_dev->label = label;
>> +               return false; /* DUAL250E fwnodes have no mount matrix info */
>> +       }
>> +
>>         if (!adev || !acpi_dev_hid_uid_match(adev, "BOSC0200", NULL))
>>                 return false;
> 
> 
> This sounds to me like
> 
> _apply_orientation_generic()
> ...
> 
> _apply_orientation_dual250e()
> 
> 
> _apply_orientation()
> 
> if ()
>   return _apply_orientation_generic()
> 
> if ()
>  return _apply_orientation_dual250e
> 
> return false;

Good point, I'll give that a try for v2 and see if I like the end result
of that. If it turns out to be a bit ugly I'll just stick with
what is in v1.

Regards,

Hans

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

[Jonathan Cameron]

On Fri, 21 May 2021 19:14:10 +0200
Hans de Goede <hdegoede@redhat.com> wrote:

> Hi All,
> 
> 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.

Given only thing I'm planning to do is tweak the line wrapping, I'm
happy to pick this series up.

The two fixes will slow things down a bit though as we should probably
get those upstream this cycle.

I'm going to leave this on list for a few days before I take anything
though, to give others time to take a look.

One side note, cc list includes a few random choices... Seems you've
accidentally included alsa people as well as IIO ones. 

Thanks

Jonathan

> 
> Regards,
> 
> Hans
> 
> 
> Hans de Goede (8):
>   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: Set label based on accel-location for ACPI
>     DUAL250E fwnodes
> 
>  drivers/iio/accel/bmc150-accel-core.c |  87 ++----------
>  drivers/iio/accel/bmc150-accel-i2c.c  | 192 +++++++++++++++++++++-----
>  drivers/iio/accel/bmc150-accel.h      |  66 ++++++++-
>  3 files changed, 239 insertions(+), 106 deletions(-)
> 

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

[Jonathan Cameron]

On Fri, 21 May 2021 19:14:10 +0200
Hans de Goede <hdegoede@redhat.com> wrote:

> Hi All,
> 
> 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.

Given only thing I'm planning to do is tweak the line wrapping, I'm
happy to pick this series up.

The two fixes will slow things down a bit though as we should probably
get those upstream this cycle.

I'm going to leave this on list for a few days before I take anything
though, to give others time to take a look.

One side note, cc list includes a few random choices... Seems you've
accidentally included alsa people as well as IIO ones. 

Thanks

Jonathan

> 
> Regards,
> 
> Hans
> 
> 
> Hans de Goede (8):
>   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: Set label based on accel-location for ACPI
>     DUAL250E fwnodes
> 
>  drivers/iio/accel/bmc150-accel-core.c |  87 ++----------
>  drivers/iio/accel/bmc150-accel-i2c.c  | 192 +++++++++++++++++++++-----
>  drivers/iio/accel/bmc150-accel.h      |  66 ++++++++-
>  3 files changed, 239 insertions(+), 106 deletions(-)
> 

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

[Andy Shevchenko]

On Sat, May 22, 2021 at 9:00 PM Jonathan Cameron <jic23@kernel.org> wrote:

> On Fri, 21 May 2021 19:14:10 +0200
> Hans de Goede <hdegoede@redhat.com> wrote:

> > 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.
>
> Given only thing I'm planning to do is tweak the line wrapping, I'm
> happy to pick this series up.
>
> The two fixes will slow things down a bit though as we should probably
> get those upstream this cycle.
>
> I'm going to leave this on list for a few days before I take anything
> though, to give others time to take a look.

You are, guys, too fast :-)

I have few (minor) comments on a few patches, in general they are okay!
So, after settling on the comments,
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
for the entire series.

Thanks, Hans, for doing this!

> One side note, cc list includes a few random choices... Seems you've
> accidentally included alsa people as well as IIO ones.


-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Sat, May 22, 2021 at 9:00 PM Jonathan Cameron <jic23@kernel.org> wrote:

> On Fri, 21 May 2021 19:14:10 +0200
> Hans de Goede <hdegoede@redhat.com> wrote:

> > 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.
>
> Given only thing I'm planning to do is tweak the line wrapping, I'm
> happy to pick this series up.
>
> The two fixes will slow things down a bit though as we should probably
> get those upstream this cycle.
>
> I'm going to leave this on list for a few days before I take anything
> though, to give others time to take a look.

You are, guys, too fast :-)

I have few (minor) comments on a few patches, in general they are okay!
So, after settling on the comments,
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
for the entire series.

Thanks, Hans, for doing this!

> One side note, cc list includes a few random choices... Seems you've
> accidentally included alsa people as well as IIO ones.


-- 
With Best Regards,
Andy Shevchenko

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

[Jonathan Cameron]

On Sat, 22 May 2021 21:03:02 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Sat, May 22, 2021 at 9:00 PM Jonathan Cameron <jic23@kernel.org> wrote:
> 
> > On Fri, 21 May 2021 19:14:10 +0200
> > Hans de Goede <hdegoede@redhat.com> wrote:  
> 
> > > 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.  
> >
> > Given only thing I'm planning to do is tweak the line wrapping, I'm
> > happy to pick this series up.
> >
> > The two fixes will slow things down a bit though as we should probably
> > get those upstream this cycle.
> >
> > I'm going to leave this on list for a few days before I take anything
> > though, to give others time to take a look.  
> 
> You are, guys, too fast :-)

One day did seem a bit short for a series doing as much as this one :)


> 
> I have few (minor) comments on a few patches, in general they are okay!
> So, after settling on the comments,
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> for the entire series.
> 
> Thanks, Hans, for doing this!
> 
> > One side note, cc list includes a few random choices... Seems you've
> > accidentally included alsa people as well as IIO ones.  
> 
> 

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

[Jonathan Cameron]

On Sat, 22 May 2021 21:03:02 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Sat, May 22, 2021 at 9:00 PM Jonathan Cameron <jic23@kernel.org> wrote:
> 
> > On Fri, 21 May 2021 19:14:10 +0200
> > Hans de Goede <hdegoede@redhat.com> wrote:  
> 
> > > 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.  
> >
> > Given only thing I'm planning to do is tweak the line wrapping, I'm
> > happy to pick this series up.
> >
> > The two fixes will slow things down a bit though as we should probably
> > get those upstream this cycle.
> >
> > I'm going to leave this on list for a few days before I take anything
> > though, to give others time to take a look.  
> 
> You are, guys, too fast :-)

One day did seem a bit short for a series doing as much as this one :)


> 
> I have few (minor) comments on a few patches, in general they are okay!
> So, after settling on the comments,
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> for the entire series.
> 
> Thanks, Hans, for doing this!
> 
> > One side note, cc list includes a few random choices... Seems you've
> > accidentally included alsa people as well as IIO ones.  
> 
> 

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

[Hans de Goede]

Hi,

On 5/22/21 8:01 PM, Jonathan Cameron wrote:
> On Fri, 21 May 2021 19:14:10 +0200
> Hans de Goede <hdegoede@redhat.com> wrote:
> 
>> Hi All,
>>
>> 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.
> 
> Given only thing I'm planning to do is tweak the line wrapping, I'm
> happy to pick this series up.
> 
> The two fixes will slow things down a bit though as we should probably
> get those upstream this cycle.
> 
> I'm going to leave this on list for a few days before I take anything
> though, to give others time to take a look.

I'll do a v2 addressing a few minor things which Andy pointed out,
I'll also take care of the comment re-wrap in the v2.

> One side note, cc list includes a few random choices... Seems you've
> accidentally included alsa people as well as IIO ones. 

You're right, I accidentally included the address-list which I use
for ASoC patches. ASoc folks, sorry for the noise.

Regards,

Hans



>> Hans de Goede (8):
>>   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: Set label based on accel-location for ACPI
>>     DUAL250E fwnodes
>>
>>  drivers/iio/accel/bmc150-accel-core.c |  87 ++----------
>>  drivers/iio/accel/bmc150-accel-i2c.c  | 192 +++++++++++++++++++++-----
>>  drivers/iio/accel/bmc150-accel.h      |  66 ++++++++-
>>  3 files changed, 239 insertions(+), 106 deletions(-)
>>
> 

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

[Hans de Goede]

Hi,

On 5/22/21 8:01 PM, Jonathan Cameron wrote:
> On Fri, 21 May 2021 19:14:10 +0200
> Hans de Goede <hdegoede@redhat.com> wrote:
> 
>> Hi All,
>>
>> 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.
> 
> Given only thing I'm planning to do is tweak the line wrapping, I'm
> happy to pick this series up.
> 
> The two fixes will slow things down a bit though as we should probably
> get those upstream this cycle.
> 
> I'm going to leave this on list for a few days before I take anything
> though, to give others time to take a look.

I'll do a v2 addressing a few minor things which Andy pointed out,
I'll also take care of the comment re-wrap in the v2.

> One side note, cc list includes a few random choices... Seems you've
> accidentally included alsa people as well as IIO ones. 

You're right, I accidentally included the address-list which I use
for ASoC patches. ASoc folks, sorry for the noise.

Regards,

Hans



>> Hans de Goede (8):
>>   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: Set label based on accel-location for ACPI
>>     DUAL250E fwnodes
>>
>>  drivers/iio/accel/bmc150-accel-core.c |  87 ++----------
>>  drivers/iio/accel/bmc150-accel-i2c.c  | 192 +++++++++++++++++++++-----
>>  drivers/iio/accel/bmc150-accel.h      |  66 ++++++++-
>>  3 files changed, 239 insertions(+), 106 deletions(-)
>>
>