[RFC 0/5] ACPI/power-suppy add fuel-gauge support on cht-wc PMIC without USB-PD support devs

[RFC 0/5] ACPI/power-suppy add fuel-gauge support on cht-wc PMIC without USB-PD support devs

[Hans de Goede]

Hi All,

Together with my earlier series to hookup the charger, Vbus boost converter
and USB role-switching:
https://lore.kernel.org/platform-driver-x86/20211030182813.116672-1-hdegoede@redhat.com/T/#t

This series also adds battery-monitoring support on the Xiaomi Mi Pad 2
and the generic parts of it should also be usable on other devices with
the same PMIC setup.

I've marked this series as a RFC because I'm not happy about the amount of
DMI quirks this series requires. The 3 separate quirks in
drivers/acpi/x86/utils.c are a bit much, but esp. when combined with also
the changes needed in drivers/gpio/gpiolib-acpi.c it all becomes a bit too
much special casing for just a single device.

So I've been thinking about alternatives for this and I've come up with
3 ways to deal with this:

1. This patch set.

2. Instead of the quirks in drivers/acpi/x86/utils.c, write an old-fashioned
"board" .c file/module which autoloads based on a DMI match and manually
instantiates i2c-clients for the BQ27520 fuel-gauge and the KTD20260 LED ctrlr.
Combined with not giving an IRQ to the fuel-gauge i2c-client (i), this allows
completely dropping the gpiolib-acpi.c changes and only requires 1 quirk for
the 2nd PWM controller in drivers/acpi/x86/utils.c. As an added bonus this
approach will also removes the need to add ACPI enumeration support to the
bq27xxx_battery code.

3. While working on this I noticed that the Mi Pad 2 DSDT actually has
full ac and battery ACPI code in its DSDT, which Linux was not trying to
use because of the Whiskey Cove PMIC ACPI HID in acpi_ac_blacklist[] in
drivers/apci/ac.c, resp. a missing _DEP for the ACPI battery.

With the native drivers disabled (the default in 5.15-rc7 without patches),
both those things fixed and a fix to intel_pmic_regs_handler() in
drivers/acpi/pmic/intel_pmic.c, battery monitoring actually starts working
somwhat!

I say somewhat because changes are not detected until userspace polls
the power_supply and switching from charge/device to host mode and
back does not work at all. This is due to the AML code for this relying
on _AEI ACPI events on virtual GPIOs on the PMIC :|  This means that we
would need to reverse engineer which events these virtual GPIO interrupts
represent; and then somehow rework the whole MFD + child driver setup
to deliver, e.g. extcon/pwrsrc events to a to-be-written GPIO driver
which supports these virtual GPIOs, while at the same time also keeping
normal native driver support since boards which USB-PD support need the
native drivers...  So OTOH this option has the promise of solving this
in a generic way which may work on more boards, OTOH it is a big mess
and we lack documentation for it.  Interestingly enough the ACPI
battery/ac code also takes ownership of the notification LED, downgrading
it from a full RGB led to a green charging LED, which is both a pre
and a con at the same time (since we would loose full RGB function).

###

Although I started out with implementing option 1, I now think I
Would personally prefer option 2. This isolates most of the code
needed to support some of these special boards into a single
(per board) file which can be build as a module which can be
autoloaded, rather then growing vmlinuz by adding quirks there.

The downside would be this sorta re-introduces the old ARM model
of one board file per (special-case) board, but there are only
1 or 2 more x86 tablets (ii) that I know about which may also
need such a board file. Which I think is managable and should
not run into the original objections against the original ARM
approach where there were way too many board files in the end.

Option 3 IMHO is a no go unless someone at Intel manages to
come up with documentation on all the virtual GPIOs which the
Windows PMIC drivers implement as method of communicating
between the PMIC driver and the AML code in the DSDT.

I'm a bit in dubio about how to progress with this, so I would
love to hear what others think about this. I would esp. appreciate
Rafael's and Mika's input on this since most of the added ugliness
in this RFC is in the ACPI code.

Regards,

Hans


i) This means that the _AEI ACPI handler for the fuel-gauge will run on
FG interrupts. This is fine it does a single I2C read and a couple of
ACPI notifies which will get ignored. Note the interrupts are "something
changed" pulses which don't need IRQ clearing.

ii) There are not that many CHT boards with a Whiskey Cove PMIC, other
then the GPD win/pocket with full USB-PD support and the Xiaomi Mi Pad 2
I'm only aware of one other, the Lenovo Yoga Book YB1-X91L/F . Since this
whole saga has gotten me quite curious and I already have the other
2 devices I've decided to spend some money on this and bought a 2nd hand
Lenovo Yoga Book YB1-X91L, whose setup is similar to the Mi Pad 2.
I should have this in about a week. I'll post a reply to this thread
with info no how the DSDT looks on the Lenovo Yoga Book and if e.g.
using the standard ACPI battery interface seems to be an option there.

iii) Note the "power: supply: bq27xxx: Add dev helper variable to
bq27xxx_battery_i2c_probe()" patch applies on top of the
"power: supply: bq27xxx: Fix kernel crash on IRQ handler register error"
bug-fix which I send out earlier.


Hans de Goede (5):
  ACPI / x86: Add 3 devices on the Xiaomi Mi Pad 2 to the always_present
    list
  gpiolib: acpi: Make acpi_gpio_in_ignore_list() more generic
  gpiolib: acpi: Add a new "ignore" module option
  power: supply: bq27xxx: Add dev helper variable to
    bq27xxx_battery_i2c_probe()
  power: supply: bq27xxx: Add support for ACPI enumeration

 drivers/acpi/x86/utils.c                   | 27 +++++++++++-
 drivers/gpio/gpiolib-acpi.c                | 40 +++++++++++++++---
 drivers/power/supply/bq27xxx_battery_i2c.c | 48 +++++++++++++++++-----
 3 files changed, 97 insertions(+), 18 deletions(-)

-- 
2.31.1

[RFC 1/5] ACPI / x86: Add 3 devices on the Xiaomi Mi Pad 2 to the always_present list

[Hans de Goede]

The Xiaomi Mi Pad 2 ships in both Windows and Android variants and the BIOS
detects the OS by looking at the EFI executable the efibootmgr entry points
to. If this executable is not the original Android bootloader (signed with
Xiaomi's own keys) it detects the OS as Windows and sets the OSID variable
used in the DSDT to "1".

This causes the following problems when running Linux:

1. Like many other CHT based tablets the Xiaomi Mi Pad 2 does not have
   a properly working ACPI battery interface instead we need to load
   a native driver for the fuel-gauge, but the TXN27520 ACPI device for
   the fuel-gauge gets hidden when OSID == "1".

2. There are backlit LEDs behind the capacitive menu, home, back buttons
   below the screen which are controlled by the second PWM controller of
   the CHT SoC, this gets hidden when OSID == "1".

3. There is an I2C attached KTD2026 RGB LED controller for the notification
   LED, this gets hidden when OSID != "4".

Add always_present_ids table entries for these devices so that they are
always seen as present independent of the OSID value.

Note the TXN27520 ACPI device does not have a UID set, so add support for
matching devices with no UID.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/acpi/x86/utils.c | 27 +++++++++++++++++++++++++--
 1 file changed, 25 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/x86/utils.c b/drivers/acpi/x86/utils.c
index f22f23933063..831949fda492 100644
--- a/drivers/acpi/x86/utils.c
+++ b/drivers/acpi/x86/utils.c
@@ -58,6 +58,11 @@ static const struct always_present_id always_present_ids[] = {
 	ENTRY("80862289", "2", X86_MATCH(ATOM_AIRMONT), {
 			DMI_MATCH(DMI_PRODUCT_NAME, "Lenovo YB1-X9"),
 		}),
+	/* The Xiaomi Mi Pad 2 uses PWM2 for touchkeys backlight control */
+	ENTRY("80862289", "2", X86_MATCH(ATOM_AIRMONT), {
+		DMI_MATCH(DMI_SYS_VENDOR, "Xiaomi Inc"),
+		DMI_MATCH(DMI_PRODUCT_NAME, "Mipad2"),
+	      }),
 	/*
 	 * The INT0002 device is necessary to clear wakeup interrupt sources
 	 * on Cherry Trail devices, without it we get nobody cared IRQ msgs.
@@ -107,6 +112,24 @@ static const struct always_present_id always_present_ids[] = {
 		DMI_MATCH(DMI_PRODUCT_NAME, "Default string"),
 		DMI_MATCH(DMI_BIOS_DATE, "05/25/2017")
 	      }),
+	/*
+	 * The Xiaomi Mi Pad 2 does not use the ACPI battery interface
+	 * instead we need to load a native driver for the fuel-gauge,
+	 * but if the BIOS thinks we are Windows rather then Android,
+	 * the fuel-gauge ACPI device is hidden.
+	 */
+	ENTRY("TXN27520", NULL, X86_MATCH(ATOM_AIRMONT), {
+		DMI_MATCH(DMI_SYS_VENDOR, "Xiaomi Inc"),
+		DMI_MATCH(DMI_PRODUCT_NAME, "Mipad2"),
+	      }),
+	/*
+	 * The Xiaomi Mi Pad 2 has a RGB LED controller for its notifcation
+	 * LED which is hidden when the BIOS thinks we are Windows, unhide it.
+	 */
+	ENTRY("KTD20260", "1", X86_MATCH(ATOM_AIRMONT), {
+		DMI_MATCH(DMI_SYS_VENDOR, "Xiaomi Inc"),
+		DMI_MATCH(DMI_PRODUCT_NAME, "Mipad2"),
+	      }),
 };
 
 bool acpi_device_always_present(struct acpi_device *adev)
@@ -118,8 +141,8 @@ bool acpi_device_always_present(struct acpi_device *adev)
 		if (acpi_match_device_ids(adev, always_present_ids[i].hid))
 			continue;
 
-		if (!adev->pnp.unique_id ||
-		    strcmp(adev->pnp.unique_id, always_present_ids[i].uid))
+		if (always_present_ids[i].uid && (!adev->pnp.unique_id ||
+		    strcmp(adev->pnp.unique_id, always_present_ids[i].uid)))
 			continue;
 
 		if (!x86_match_cpu(always_present_ids[i].cpu_ids))
-- 
2.31.1

[RFC 2/5] gpiolib: acpi: Make acpi_gpio_in_ignore_list() more generic

[Hans de Goede]

Pass the ignore list to parse into acpi_gpio_in_ignore_list() instead
of hardcoding it to the ignore_wake module-param.

This is a preparation patch for adding support to fully ignore some
_AEI entries.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/gpio/gpiolib-acpi.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index 47712b6903b5..342219a58a32 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c
@@ -320,13 +320,15 @@ static struct gpio_desc *acpi_request_own_gpiod(struct gpio_chip *chip,
 	return desc;
 }
 
-static bool acpi_gpio_in_ignore_list(const char *controller_in, int pin_in)
+static bool acpi_gpio_in_ignore_list(const char *ignore_list,
+				     const char *controller_in,
+				     int pin_in)
 {
 	const char *controller, *pin_str;
 	int len, pin;
 	char *endp;
 
-	controller = ignore_wake;
+	controller = ignore_list;
 	while (controller) {
 		pin_str = strchr(controller, '@');
 		if (!pin_str)
@@ -350,8 +352,8 @@ static bool acpi_gpio_in_ignore_list(const char *controller_in, int pin_in)
 
 	return false;
 err:
-	pr_err_once("Error invalid value for gpiolib_acpi.ignore_wake: %s\n",
-		    ignore_wake);
+	pr_err_once("Error invalid value for gpiolib_acpi.ignore_xxx: %s\n",
+		    ignore_list);
 	return false;
 }
 
@@ -363,7 +365,7 @@ static bool acpi_gpio_irq_is_wake(struct device *parent,
 	if (agpio->wake_capable != ACPI_WAKE_CAPABLE)
 		return false;
 
-	if (acpi_gpio_in_ignore_list(dev_name(parent), pin)) {
+	if (acpi_gpio_in_ignore_list(ignore_wake, dev_name(parent), pin)) {
 		dev_info(parent, "Ignoring wakeup on pin %d\n", pin);
 		return false;
 	}
-- 
2.31.1

[RFC 3/5] gpiolib: acpi: Add a new "ignore" module option

[Hans de Goede]

Add a new "ignore" module option to completely ignore controller@pin combos
from _AEI lists.

And add a DMI quirk to ignore the interrupt of the BQ27520 fuel-gauge IC
on the Xiaomi Mi Pad 2. On this device we use native charger + fuel-gauge
drivers because of issues with the ACPI battery implementation. The _AEI
listing of the fuel-gauge IRQ is intended for use with the unused ACPI
battery implementation and is blocking the bq27xxx fuel-gauge driver
from binding.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/gpio/gpiolib-acpi.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index 342219a58a32..2d08f33a22a6 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c
@@ -32,9 +32,16 @@ MODULE_PARM_DESC(ignore_wake,
 		 "controller@pin combos on which to ignore the ACPI wake flag "
 		 "ignore_wake=controller@pin[,controller@pin[,...]]");
 
+static char *ignore;
+module_param(ignore, charp, 0444);
+MODULE_PARM_DESC(ignore,
+		 "controller@pin combos which are to be ignored from _AEI lists "
+		 "ignore=controller@pin[,controller@pin[,...]]");
+
 struct acpi_gpiolib_dmi_quirk {
 	bool no_edge_events_on_boot;
 	char *ignore_wake;
+	char *ignore;
 };
 
 /**
@@ -407,6 +414,11 @@ static acpi_status acpi_gpiochip_alloc_event(struct acpi_resource *ares,
 	if (!handler)
 		return AE_OK;
 
+	if (acpi_gpio_in_ignore_list(ignore, dev_name(chip->parent), pin)) {
+		dev_info(chip->parent, "Ignoring _AEI entry for pin %d\n", pin);
+		return AE_OK;
+	}
+
 	desc = acpi_request_own_gpiod(chip, agpio, 0, "ACPI:Event");
 	if (IS_ERR(desc)) {
 		dev_err(chip->parent,
@@ -1565,6 +1577,19 @@ static const struct dmi_system_id gpiolib_acpi_quirks[] __initconst = {
 			.ignore_wake = "INT33FF:01@0",
 		},
 	},
+	{
+		/*
+		 * On the Xiaomi Mi Pad 2 we use native battery drivers, disable
+		 * the _AEI entry for the fuel-gauge IRQ.
+		 */
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "Xiaomi Inc"),
+			DMI_MATCH(DMI_PRODUCT_NAME, "Mipad2"),
+		},
+		.driver_data = &(struct acpi_gpiolib_dmi_quirk) {
+			.ignore = "INT33FF:03@52",
+		},
+	},
 	{} /* Terminating entry */
 };
 
@@ -1587,6 +1612,9 @@ static int __init acpi_gpio_setup_params(void)
 	if (ignore_wake == NULL && quirk && quirk->ignore_wake)
 		ignore_wake = quirk->ignore_wake;
 
+	if (ignore == NULL && quirk && quirk->ignore)
+		ignore = quirk->ignore;
+
 	return 0;
 }
 
-- 
2.31.1

[RFC 4/5] power: supply: bq27xxx: Add dev helper variable to bq27xxx_battery_i2c_probe()

[Hans de Goede]

Add a dev local variable to the bq27xxx_battery_i2c_probe(), to replace
"&client->dev" in various places.

Note the resulting cleanup is somewhat minimal, this is mostly
a preparation for adding ACPI enumeration support which will
use the new "dev" variable more.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/power/supply/bq27xxx_battery_i2c.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/power/supply/bq27xxx_battery_i2c.c b/drivers/power/supply/bq27xxx_battery_i2c.c
index cf38cbfe13e9..0a1b922389e1 100644
--- a/drivers/power/supply/bq27xxx_battery_i2c.c
+++ b/drivers/power/supply/bq27xxx_battery_i2c.c
@@ -139,6 +139,7 @@ static int bq27xxx_battery_i2c_bulk_write(struct bq27xxx_device_info *di,
 static int bq27xxx_battery_i2c_probe(struct i2c_client *client,
 				     const struct i2c_device_id *id)
 {
+	struct device *dev = &client->dev;
 	struct bq27xxx_device_info *di;
 	int ret;
 	char *name;
@@ -151,16 +152,16 @@ static int bq27xxx_battery_i2c_probe(struct i2c_client *client,
 	if (num < 0)
 		return num;
 
-	name = devm_kasprintf(&client->dev, GFP_KERNEL, "%s-%d", id->name, num);
+	name = devm_kasprintf(dev, GFP_KERNEL, "%s-%d", id->name, num);
 	if (!name)
 		goto err_mem;
 
-	di = devm_kzalloc(&client->dev, sizeof(*di), GFP_KERNEL);
+	di = devm_kzalloc(dev, sizeof(*di), GFP_KERNEL);
 	if (!di)
 		goto err_mem;
 
 	di->id = num;
-	di->dev = &client->dev;
+	di->dev = dev;
 	di->chip = id->driver_data;
 	di->name = name;
 
@@ -179,13 +180,12 @@ static int bq27xxx_battery_i2c_probe(struct i2c_client *client,
 	i2c_set_clientdata(client, di);
 
 	if (client->irq) {
-		ret = devm_request_threaded_irq(&client->dev, client->irq,
+		ret = devm_request_threaded_irq(dev, client->irq,
 				NULL, bq27xxx_battery_irq_handler_thread,
 				IRQF_ONESHOT,
 				di->name, di);
 		if (ret) {
-			dev_err(&client->dev,
-				"Unable to register IRQ %d error %d\n",
+			dev_err(dev, "Unable to register IRQ %d error %d\n",
 				client->irq, ret);
 			bq27xxx_battery_teardown(di);
 			goto err_failed;
-- 
2.31.1

[RFC 5/5] power: supply: bq27xxx: Add support for ACPI enumeration

[Hans de Goede]

Some x86/ACPI devices with a bq27xxx fuel-gauge do not use the standard
ACPI battery API for battery monitoring. Instead they have an ACPI
device describing the actual fuel-gauge IC and expect the OS to have
a native driver for the fuel-gauge.

Add support for such ACPI enumerated bq27xxx fuel-gauges.

To get the fuel-gauge to work properly on ACPI devs 3 changes are needed:

1. Add an acpi_match_table and set di->chip based on this, note the
   new "if (id) check also fixes a NULL pointer deref when someone tries to
   manually bind the driver from sysfs.

2. When the charger IC has external power applied or removed, the psy-core
   should call bq27xxx_external_power_changed(). This requires a valid
   consumer<->supplier link between the charger and the fuel-gauge
   power-supplies. For ACPI enumerated fuel-gauges this link is missing.

   The charger-ICs used on these devices already contain a special
   power_supply_config.supplied_to default set to "main-battery".
   This commit makes use of this by setting the power-supply's name to
   "main-battery" when enumerated by ACPI, to establish the missing
   consumer<->supplier link.

3. Sometimes the irqflags in the ACPI tables are no good, override them.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/power/supply/bq27xxx_battery_i2c.c | 38 ++++++++++++++++++----
 1 file changed, 32 insertions(+), 6 deletions(-)

diff --git a/drivers/power/supply/bq27xxx_battery_i2c.c b/drivers/power/supply/bq27xxx_battery_i2c.c
index 0a1b922389e1..c3fe1e4b08bd 100644
--- a/drivers/power/supply/bq27xxx_battery_i2c.c
+++ b/drivers/power/supply/bq27xxx_battery_i2c.c
@@ -6,6 +6,7 @@
  *	Andrew F. Davis <afd@ti.com>
  */
 
+#include <linux/acpi.h>
 #include <linux/i2c.h>
 #include <linux/interrupt.h>
 #include <linux/module.h>
@@ -139,8 +140,10 @@ static int bq27xxx_battery_i2c_bulk_write(struct bq27xxx_device_info *di,
 static int bq27xxx_battery_i2c_probe(struct i2c_client *client,
 				     const struct i2c_device_id *id)
 {
+	const struct acpi_device_id *acpi_id = NULL;
 	struct device *dev = &client->dev;
 	struct bq27xxx_device_info *di;
+	unsigned long irqflags = 0;
 	int ret;
 	char *name;
 	int num;
@@ -152,17 +155,31 @@ static int bq27xxx_battery_i2c_probe(struct i2c_client *client,
 	if (num < 0)
 		return num;
 
-	name = devm_kasprintf(dev, GFP_KERNEL, "%s-%d", id->name, num);
-	if (!name)
-		goto err_mem;
-
 	di = devm_kzalloc(dev, sizeof(*di), GFP_KERNEL);
 	if (!di)
 		goto err_mem;
 
+	if (id) {
+		di->chip = id->driver_data;
+	} else {
+		acpi_id = acpi_match_device(dev->driver->acpi_match_table, dev);
+		if (!acpi_id)
+			return -ENODEV;
+
+		irqflags = IRQF_TRIGGER_RISING;
+		di->chip = acpi_id->driver_data;
+	}
+
+	if (acpi_id && num == 0) {
+		name = "main-battery";
+	} else {
+		name = devm_kasprintf(dev, GFP_KERNEL, "%s-%d", id->name, num);
+		if (!name)
+			goto err_mem;
+	}
+
 	di->id = num;
 	di->dev = dev;
-	di->chip = id->driver_data;
 	di->name = name;
 
 	di->bus.read = bq27xxx_battery_i2c_read;
@@ -182,7 +199,7 @@ static int bq27xxx_battery_i2c_probe(struct i2c_client *client,
 	if (client->irq) {
 		ret = devm_request_threaded_irq(dev, client->irq,
 				NULL, bq27xxx_battery_irq_handler_thread,
-				IRQF_ONESHOT,
+				irqflags | IRQF_ONESHOT,
 				di->name, di);
 		if (ret) {
 			dev_err(dev, "Unable to register IRQ %d error %d\n",
@@ -292,10 +309,19 @@ static const struct of_device_id bq27xxx_battery_i2c_of_match_table[] = {
 MODULE_DEVICE_TABLE(of, bq27xxx_battery_i2c_of_match_table);
 #endif
 
+#ifdef CONFIG_ACPI
+static const struct acpi_device_id bq27xxx_acpi_match[] = {
+	{ "TXN27520", BQ2752X },
+	{ }
+};
+MODULE_DEVICE_TABLE(acpi, bq27xxx_acpi_match);
+#endif
+
 static struct i2c_driver bq27xxx_battery_i2c_driver = {
 	.driver = {
 		.name = "bq27xxx-battery",
 		.of_match_table = of_match_ptr(bq27xxx_battery_i2c_of_match_table),
+		.acpi_match_table = ACPI_PTR(bq27xxx_acpi_match),
 	},
 	.probe = bq27xxx_battery_i2c_probe,
 	.remove = bq27xxx_battery_i2c_remove,
-- 
2.31.1

Re: [RFC 0/5] ACPI/power-suppy add fuel-gauge support on cht-wc PMIC without USB-PD support devs

[Andy Shevchenko]

On Sun, Oct 31, 2021 at 6:25 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi All,
>
> Together with my earlier series to hookup the charger, Vbus boost converter
> and USB role-switching:
> https://lore.kernel.org/platform-driver-x86/20211030182813.116672-1-hdegoede@redhat.com/T/#t
>
> This series also adds battery-monitoring support on the Xiaomi Mi Pad 2
> and the generic parts of it should also be usable on other devices with
> the same PMIC setup.
>
> I've marked this series as a RFC because I'm not happy about the amount of
> DMI quirks this series requires. The 3 separate quirks in
> drivers/acpi/x86/utils.c are a bit much, but esp. when combined with also
> the changes needed in drivers/gpio/gpiolib-acpi.c it all becomes a bit too
> much special casing for just a single device.
>
> So I've been thinking about alternatives for this and I've come up with
> 3 ways to deal with this:
>
> 1. This patch set.
>
> 2. Instead of the quirks in drivers/acpi/x86/utils.c, write an old-fashioned
> "board" .c file/module which autoloads based on a DMI match and manually
> instantiates i2c-clients for the BQ27520 fuel-gauge and the KTD20260 LED ctrlr.
> Combined with not giving an IRQ to the fuel-gauge i2c-client (i), this allows
> completely dropping the gpiolib-acpi.c changes and only requires 1 quirk for
> the 2nd PWM controller in drivers/acpi/x86/utils.c. As an added bonus this
> approach will also removes the need to add ACPI enumeration support to the
> bq27xxx_battery code.
>
> 3. While working on this I noticed that the Mi Pad 2 DSDT actually has
> full ac and battery ACPI code in its DSDT, which Linux was not trying to
> use because of the Whiskey Cove PMIC ACPI HID in acpi_ac_blacklist[] in
> drivers/apci/ac.c, resp. a missing _DEP for the ACPI battery.
>
> With the native drivers disabled (the default in 5.15-rc7 without patches),
> both those things fixed and a fix to intel_pmic_regs_handler() in
> drivers/acpi/pmic/intel_pmic.c, battery monitoring actually starts working
> somwhat!
>
> I say somewhat because changes are not detected until userspace polls
> the power_supply and switching from charge/device to host mode and
> back does not work at all. This is due to the AML code for this relying
> on _AEI ACPI events on virtual GPIOs on the PMIC :|  This means that we
> would need to reverse engineer which events these virtual GPIO interrupts
> represent; and then somehow rework the whole MFD + child driver setup
> to deliver, e.g. extcon/pwrsrc events to a to-be-written GPIO driver
> which supports these virtual GPIOs, while at the same time also keeping
> normal native driver support since boards which USB-PD support need the
> native drivers...  So OTOH this option has the promise of solving this
> in a generic way which may work on more boards, OTOH it is a big mess
> and we lack documentation for it.  Interestingly enough the ACPI
> battery/ac code also takes ownership of the notification LED, downgrading
> it from a full RGB led to a green charging LED, which is both a pre
> and a con at the same time (since we would loose full RGB function).
>
> ###

> Although I started out with implementing option 1, I now think I
> Would personally prefer option 2. This isolates most of the code
> needed to support some of these special boards into a single
> (per board) file which can be build as a module which can be
> autoloaded, rather then growing vmlinuz by adding quirks there.

Even before reading this my attention was on option 2 as well.
However, we might give another round of searching the documentation
for the vGPIO lines.

Meanwhile, have you tried to see if Android tree(s) has(ve) the
patches related to all this? (I'm a bit sceptical they do the right
thing and most probably just fall into board files case)

> The downside would be this sorta re-introduces the old ARM model
> of one board file per (special-case) board, but there are only
> 1 or 2 more x86 tablets (ii) that I know about which may also
> need such a board file. Which I think is managable and should
> not run into the original objections against the original ARM
> approach where there were way too many board files in the end.
>
> Option 3 IMHO is a no go unless someone at Intel manages to
> come up with documentation on all the virtual GPIOs which the
> Windows PMIC drivers implement as method of communicating
> between the PMIC driver and the AML code in the DSDT.
>
> I'm a bit in dubio about how to progress with this, so I would
> love to hear what others think about this. I would esp. appreciate
> Rafael's and Mika's input on this since most of the added ugliness
> in this RFC is in the ACPI code.
>
> Regards,
>
> Hans
>
>
> i) This means that the _AEI ACPI handler for the fuel-gauge will run on
> FG interrupts. This is fine it does a single I2C read and a couple of
> ACPI notifies which will get ignored. Note the interrupts are "something
> changed" pulses which don't need IRQ clearing.
>
> ii) There are not that many CHT boards with a Whiskey Cove PMIC, other
> then the GPD win/pocket with full USB-PD support and the Xiaomi Mi Pad 2
> I'm only aware of one other, the Lenovo Yoga Book YB1-X91L/F . Since this
> whole saga has gotten me quite curious and I already have the other
> 2 devices I've decided to spend some money on this and bought a 2nd hand
> Lenovo Yoga Book YB1-X91L, whose setup is similar to the Mi Pad 2.
> I should have this in about a week. I'll post a reply to this thread
> with info no how the DSDT looks on the Lenovo Yoga Book and if e.g.
> using the standard ACPI battery interface seems to be an option there.
>
> iii) Note the "power: supply: bq27xxx: Add dev helper variable to
> bq27xxx_battery_i2c_probe()" patch applies on top of the
> "power: supply: bq27xxx: Fix kernel crash on IRQ handler register error"
> bug-fix which I send out earlier.

-- 
With Best Regards,
Andy Shevchenko

Re: [RFC 3/5] gpiolib: acpi: Add a new "ignore" module option

[Andy Shevchenko]

On Sun, Oct 31, 2021 at 6:25 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Add a new "ignore" module option to completely ignore controller@pin combos
> from _AEI lists.
>
> And add a DMI quirk to ignore the interrupt of the BQ27520 fuel-gauge IC
> on the Xiaomi Mi Pad 2. On this device we use native charger + fuel-gauge
> drivers because of issues with the ACPI battery implementation. The _AEI
> listing of the fuel-gauge IRQ is intended for use with the unused ACPI
> battery implementation and is blocking the bq27xxx fuel-gauge driver
> from binding.

I'm wondering if the idea behind this is something relative to
https://elixir.bootlin.com/linux/latest/source/drivers/acpi/sysfs.c

-- 
With Best Regards,
Andy Shevchenko

Re: [RFC 3/5] gpiolib: acpi: Add a new "ignore" module option

[Andy Shevchenko]

On Sun, Oct 31, 2021 at 9:59 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Sun, Oct 31, 2021 at 6:25 PM Hans de Goede <hdegoede@redhat.com> wrote:

> I'm wondering if the idea behind this is something relative to
> https://elixir.bootlin.com/linux/latest/source/drivers/acpi/sysfs.c

Missed line:
https://elixir.bootlin.com/linux/latest/source/drivers/acpi/sysfs.c#L792

-- 
With Best Regards,
Andy Shevchenko

Re: [RFC 0/5] ACPI/power-suppy add fuel-gauge support on cht-wc PMIC without USB-PD support devs

[Hans de Goede]

Hi,

On 10/31/21 20:49, Andy Shevchenko wrote:
> On Sun, Oct 31, 2021 at 6:25 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> Hi All,
>>
>> Together with my earlier series to hookup the charger, Vbus boost converter
>> and USB role-switching:
>> https://lore.kernel.org/platform-driver-x86/20211030182813.116672-1-hdegoede@redhat.com/T/#t
>>
>> This series also adds battery-monitoring support on the Xiaomi Mi Pad 2
>> and the generic parts of it should also be usable on other devices with
>> the same PMIC setup.
>>
>> I've marked this series as a RFC because I'm not happy about the amount of
>> DMI quirks this series requires. The 3 separate quirks in
>> drivers/acpi/x86/utils.c are a bit much, but esp. when combined with also
>> the changes needed in drivers/gpio/gpiolib-acpi.c it all becomes a bit too
>> much special casing for just a single device.
>>
>> So I've been thinking about alternatives for this and I've come up with
>> 3 ways to deal with this:
>>
>> 1. This patch set.
>>
>> 2. Instead of the quirks in drivers/acpi/x86/utils.c, write an old-fashioned
>> "board" .c file/module which autoloads based on a DMI match and manually
>> instantiates i2c-clients for the BQ27520 fuel-gauge and the KTD20260 LED ctrlr.
>> Combined with not giving an IRQ to the fuel-gauge i2c-client (i), this allows
>> completely dropping the gpiolib-acpi.c changes and only requires 1 quirk for
>> the 2nd PWM controller in drivers/acpi/x86/utils.c. As an added bonus this
>> approach will also removes the need to add ACPI enumeration support to the
>> bq27xxx_battery code.
>>
>> 3. While working on this I noticed that the Mi Pad 2 DSDT actually has
>> full ac and battery ACPI code in its DSDT, which Linux was not trying to
>> use because of the Whiskey Cove PMIC ACPI HID in acpi_ac_blacklist[] in
>> drivers/apci/ac.c, resp. a missing _DEP for the ACPI battery.
>>
>> With the native drivers disabled (the default in 5.15-rc7 without patches),
>> both those things fixed and a fix to intel_pmic_regs_handler() in
>> drivers/acpi/pmic/intel_pmic.c, battery monitoring actually starts working
>> somwhat!
>>
>> I say somewhat because changes are not detected until userspace polls
>> the power_supply and switching from charge/device to host mode and
>> back does not work at all. This is due to the AML code for this relying
>> on _AEI ACPI events on virtual GPIOs on the PMIC :|  This means that we
>> would need to reverse engineer which events these virtual GPIO interrupts
>> represent; and then somehow rework the whole MFD + child driver setup
>> to deliver, e.g. extcon/pwrsrc events to a to-be-written GPIO driver
>> which supports these virtual GPIOs, while at the same time also keeping
>> normal native driver support since boards which USB-PD support need the
>> native drivers...  So OTOH this option has the promise of solving this
>> in a generic way which may work on more boards, OTOH it is a big mess
>> and we lack documentation for it.  Interestingly enough the ACPI
>> battery/ac code also takes ownership of the notification LED, downgrading
>> it from a full RGB led to a green charging LED, which is both a pre
>> and a con at the same time (since we would loose full RGB function).
>>
>> ###
> 
>> Although I started out with implementing option 1, I now think I
>> Would personally prefer option 2. This isolates most of the code
>> needed to support some of these special boards into a single
>> (per board) file which can be build as a module which can be
>> autoloaded, rather then growing vmlinuz by adding quirks there.
> 
> Even before reading this my attention was on option 2 as well.

Its good to hear that you think this is likely the best option too.
I hope to send out another RFC patch-series taking this approach
instead soon.

> However, we might give another round of searching the documentation
> for the vGPIO lines.

Having those would be good regardless.

> Meanwhile, have you tried to see if Android tree(s) has(ve) the
> patches related to all this? (I'm a bit sceptical they do the right
> thing and most probably just fall into board files case)

The Android code takes the native driver path, when the EFI firmware
sees it is about to exec Xiaomi's Android bootloader (I think it
checks the signature) it sets OSID = 0x04 which makes all the
ACPI devices which patch 1/5 of this RFC makes "always_present"
return 0xf from their _STA method and it disables the troublesome
_AEI handler too when OSID==4.

So basically it does everything which this RFC series does with
quirks automatically correct based on the OSID. But we cannot
influence this ourselves, there is a BIOS option for it, but
that gets overridden by OS autodetect code at boot.

The fact that the Android on the tablet also goes the use
native charger + fuel-gauge drivers route does to me is a further
hint that using native drivers is probably the right thing to do
(OTOH some of the code in the Android port the device ships with
 is not so great).

Regards,

Hans

Re: [RFC 3/5] gpiolib: acpi: Add a new "ignore" module option

[Hans de Goede]

Hi,

On 10/31/21 20:59, Andy Shevchenko wrote:
> On Sun, Oct 31, 2021 at 6:25 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> Add a new "ignore" module option to completely ignore controller@pin combos
>> from _AEI lists.
>>
>> And add a DMI quirk to ignore the interrupt of the BQ27520 fuel-gauge IC
>> on the Xiaomi Mi Pad 2. On this device we use native charger + fuel-gauge
>> drivers because of issues with the ACPI battery implementation. The _AEI
>> listing of the fuel-gauge IRQ is intended for use with the unused ACPI
>> battery implementation and is blocking the bq27xxx fuel-gauge driver
>> from binding.
> 
> I'm wondering if the idea behind this is something relative to
> https://elixir.bootlin.com/linux/latest/source/drivers/acpi/sysfs.c

The idea indeed is similar, but there is only one set of GPEs and
the GPIO pin-namespace is per GPIO controller, hence the
controller-name@pin format used, which is also used for the
already existing ignore_wake gpiolib-acpi.c module option and
the patches re-use the existing parsing code.

But since there seems to be agreement that using a board-file to
work around the DSDT deficiencies is the best option this patch
will no longer be needed. It is probably still good to keep it
archived somewhere in case the functionality turns out to be
useful on some other device(s).

Regards,

Hans