[PATCH 0/8] iio: accel: bmc150: Add support for yoga's with dual accelerometers with an ACPI HID of DUAL250E
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(-)
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);
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;
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).
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
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); }
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);
}
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);
}
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.
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.
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);
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,
.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);
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,
.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);
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,
.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);
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?
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
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,
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;
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; };
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;
};
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 ?
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
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;
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";elselabel = "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
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";elselabel = "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
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(-)
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.
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.
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(-)
participants (3)
-
Andy Shevchenko -
Hans de Goede -
Jonathan Cameron