[PATCH 0/2] power: supply: Fix AXP288 fallback when not needed

[PATCH 0/2] power: supply: Fix AXP288 fallback when not needed

[Carlo Caione]

From: Carlo Caione <carlo@endlessm.com>

With commits af3ec837 and dccfae6d a blacklist was introduced to avoid
using the ACPI drivers for AC and battery when a native PMIC driver was
already present. While this is in general a good idea (because of broken
DSDT or proprietary and undocumented ACPI opregions for the ACPI
AC/battery devices) we have come across at least one CherryTrail laptop
(ECS EF20EA) shipping the AXP288 together with a separate FG controller
(a MAX17047) instead of the one embedded in the AXP288.

This is the interesting analisys done by Hans de Goede (thank you):

Looking at the _BIX method of the BATC/PNP0C0A device, we see it referencing
FG10:

Method (_BIX, 0, NotSerialized)  // _BIX: Battery Information Extend
{
    If (AVBL == One)
    {
        BUF2 = FG10 /* \_SB_.PCI0.I2C1.FG10 */

And FG10 is defined as:

Field (DVFG, BufferAcc, NoLock, Preserve)
{
    Connection (SMFG),
    Offset (0x10),
    AccessAs (BufferAcc, AttribBytes (0x02)),
    FG10,   8
}

With SMFG being defined as:

Name (SMFG, ResourceTemplate ()
{
    I2cSerialBusV2 (0x0036, ControllerInitiated, 0x000186A0,
        AddressingMode7Bit, "\\_SB.PCI0.I2C1",
        0x00, ResourceConsumer, , Exclusive,
        )
})

Looking for I2C1 address 0x0036 we find:

Device (ANFG)
{
    Name (_HID, "MAX17047" /* Fuel Gauge Controller */)  // _HID: Hardwa
    Name (_CID, "MAX17047" /* Fuel Gauge Controller */)  // _CID: Compat
    Name (_DDN, "Fuel Gauge Controller")  // _DDN: DOS Device Name
    Name (RBUF, ResourceTemplate ()
    {
        I2cSerialBusV2 (0x0036, ControllerInitiated, 0x00061A80,
            AddressingMode7Bit, "\\_SB.PCI0.I2C1",
            0x00, ResourceConsumer, , Exclusive,
            )
        GpioInt (Level, ActiveLow, ExclusiveAndWake, PullNone, 0x0000,
            "\\_SB.GPO3", 0x00, ResourceConsumer, ,
            )
            {   // Pin list
                0x0001
            }
    })

Where as the AXP288 PMIC is I2C7 address 0x034:

Device (PMI1)
{
    Name (_ADR, Zero)  // _ADR: Address
    Name (_HID, "INT33F4" /* XPOWER PMIC Controller */)  // _HID: Ha
    Name (_CID, "INT33F4" /* XPOWER PMIC Controller */)  // _CID: Co
    Name (_DDN, "XPOWER PMIC Controller")  // _DDN: DOS Device Name
    Name (_HRV, 0x03)  // _HRV: Hardware Revision
    Name (_UID, One)  // _UID: Unique ID

    Method (_CRS, 0, NotSerialized)  // _CRS: Current Resource Setti
    {
        Name (SBUF, ResourceTemplate ()
        {
            I2cSerialBusV2 (0x0034, ControllerInitiated, 0x000F4240,
                AddressingMode7Bit, "\\_SB.PCI0.I2C7",
                0x00, ResourceConsumer, , Exclusive,
                )

So basically this laptopt is using a separate FG chip instead of the one
embedded in the AXP288.

To have this correctly working we need basically to avoid the fallback on the
AXP288 driver enabling again the ACPI AC/battery drivers and at the same time
avoiding that the AXP288 FG driver is probed at all.

I'm still not fully convinced that having two different quirks (one to disable
the blacklist and another to disable the AXP288 FG probing) is the right way to
fix this. So any comment is welcome.

Carlo Caione (2):
  power: supply: ACPI/AXP288: Add quirk to avoid using PMIC
  power: supply: ACPI/AXP288: Add quirks for ECS EF20EA

 drivers/acpi/ac.c                        | 33 ++++++++++++++++++++++++--------
 drivers/acpi/battery.c                   | 33 ++++++++++++++++++++++++--------
 drivers/power/supply/axp288_fuel_gauge.c |  6 ++++++
 3 files changed, 56 insertions(+), 16 deletions(-)

-- 
2.14.1

[PATCH 1/2] power: supply: ACPI/AXP288: Add quirk to avoid using PMIC

[Carlo Caione]

From: Carlo Caione <carlo@endlessm.com>

With commits af3ec837 and dccfae6d a blacklist was introduced to avoid
using the ACPI drivers for AC and battery when a native PMIC driver was
already present. While this is in general a good idea (because of broken
DSDT or proprietary and undocumented ACPI opregions for the ACPI
AC/battery devices) we have come across at least one CherryTrail laptop
(ECS EF20EA) shipping the AXP288 together with a separate FG controller
(a MAX17047) instead of the one embedded in the AXP288.

The net effect of blacklisting the ACPI drivers is that on this platform
the AC/battery reporting is broken since the information is coming from
the AXP288 FG driver, not actually used in hardware.

In this case we want to keep using the ACPI AC/battery driver that is
able to interface correctly with the real FG controller.

We introduce therefore a new quirk to avoid the blacklist.

Signed-off-by: Carlo Caione <carlo@endlessm.com>
---
 drivers/acpi/ac.c      | 26 ++++++++++++++++++--------
 drivers/acpi/battery.c | 26 ++++++++++++++++++--------
 2 files changed, 36 insertions(+), 16 deletions(-)

diff --git a/drivers/acpi/ac.c b/drivers/acpi/ac.c
index 47a7ed557bd6..b9a4ca720309 100644
--- a/drivers/acpi/ac.c
+++ b/drivers/acpi/ac.c
@@ -87,6 +87,7 @@ static int acpi_ac_open_fs(struct inode *inode, struct file *file);
 
 
 static int ac_sleep_before_get_state_ms;
+static bool ac_not_use_pmic;
 
 static struct acpi_driver acpi_ac_driver = {
 	.name = "ac",
@@ -316,6 +317,12 @@ static int thinkpad_e530_quirk(const struct dmi_system_id *d)
 	return 0;
 }
 
+static int ac_not_use_pmic_quirk(const struct dmi_system_id *d)
+{
+	ac_not_use_pmic = true;
+	return 0;
+}
+
 static const struct dmi_system_id ac_dmi_table[] = {
 	{
 	.callback = thinkpad_e530_quirk,
@@ -384,7 +391,6 @@ static int acpi_ac_add(struct acpi_device *device)
 		kfree(ac);
 	}
 
-	dmi_check_system(ac_dmi_table);
 	return result;
 }
 
@@ -442,13 +448,17 @@ static int __init acpi_ac_init(void)
 	if (acpi_disabled)
 		return -ENODEV;
 
-	for (i = 0; i < ARRAY_SIZE(acpi_ac_blacklist); i++)
-		if (acpi_dev_present(acpi_ac_blacklist[i].hid, "1",
-				     acpi_ac_blacklist[i].hrv)) {
-			pr_info(PREFIX "AC: found native %s PMIC, not loading\n",
-				acpi_ac_blacklist[i].hid);
-			return -ENODEV;
-		}
+	dmi_check_system(ac_dmi_table);
+
+	if (!ac_not_use_pmic) {
+		for (i = 0; i < ARRAY_SIZE(acpi_ac_blacklist); i++)
+			if (acpi_dev_present(acpi_ac_blacklist[i].hid, "1",
+					     acpi_ac_blacklist[i].hrv)) {
+				pr_info(PREFIX "AC: found native %s PMIC, not loading\n",
+					acpi_ac_blacklist[i].hid);
+				return -ENODEV;
+			}
+	}
 
 #ifdef CONFIG_ACPI_PROCFS_POWER
 	acpi_ac_dir = acpi_lock_ac_dir();
diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
index 7128488a3a72..258d2e66f230 100644
--- a/drivers/acpi/battery.c
+++ b/drivers/acpi/battery.c
@@ -71,6 +71,7 @@ static bool battery_driver_registered;
 static int battery_bix_broken_package;
 static int battery_notification_delay_ms;
 static int battery_full_discharging;
+static bool battery_not_use_pmic;
 static unsigned int cache_time = 1000;
 module_param(cache_time, uint, 0644);
 MODULE_PARM_DESC(cache_time, "cache time in milliseconds");
@@ -1176,6 +1177,13 @@ static int __init battery_full_discharging_quirk(const struct dmi_system_id *d)
 	return 0;
 }
 
+static int __init
+battery_not_use_pmic_quirk(const struct dmi_system_id *d)
+{
+	battery_not_use_pmic = true;
+	return 0;
+}
+
 static const struct dmi_system_id bat_dmi_table[] __initconst = {
 	{
 		.callback = battery_bix_broken_package_quirk,
@@ -1364,16 +1372,18 @@ static void __init acpi_battery_init_async(void *unused, async_cookie_t cookie)
 	unsigned int i;
 	int result;
 
-	for (i = 0; i < ARRAY_SIZE(acpi_battery_blacklist); i++)
-		if (acpi_dev_present(acpi_battery_blacklist[i], "1", -1)) {
-			pr_info(PREFIX ACPI_BATTERY_DEVICE_NAME
-				": found native %s PMIC, not loading\n",
-				acpi_battery_blacklist[i]);
-			return;
-		}
-
 	dmi_check_system(bat_dmi_table);
 
+	if (!battery_not_use_pmic) {
+		for (i = 0; i < ARRAY_SIZE(acpi_battery_blacklist); i++)
+			if (acpi_dev_present(acpi_battery_blacklist[i], "1", -1)) {
+				pr_info(PREFIX ACPI_BATTERY_DEVICE_NAME
+					": found native %s PMIC, not loading\n",
+					acpi_battery_blacklist[i]);
+				return;
+			}
+	}
+
 #ifdef CONFIG_ACPI_PROCFS_POWER
 	acpi_battery_dir = acpi_lock_battery_dir();
 	if (!acpi_battery_dir)
-- 
2.14.1

[PATCH 2/2] power: supply: ACPI/AXP288: Add quirks for ECS EF20EA

[Carlo Caione]

From: Carlo Caione <carlo@endlessm.com>

On the ECS EF20EA laptop we need to move away from the AXP288 FG driver
and enable again the ACPI AC/battery drivers. Add the required quirks to
do that. We rely only on the product name because all the other DMI
entries are dummy or not filled in on this platform.

Signed-off-by: Carlo Caione <carlo@endlessm.com>
---
 drivers/acpi/ac.c                        | 7 +++++++
 drivers/acpi/battery.c                   | 7 +++++++
 drivers/power/supply/axp288_fuel_gauge.c | 6 ++++++
 3 files changed, 20 insertions(+)

diff --git a/drivers/acpi/ac.c b/drivers/acpi/ac.c
index b9a4ca720309..39f778f954f5 100644
--- a/drivers/acpi/ac.c
+++ b/drivers/acpi/ac.c
@@ -332,6 +332,13 @@ static const struct dmi_system_id ac_dmi_table[] = {
 		DMI_MATCH(DMI_PRODUCT_NAME, "32597CG"),
 		},
 	},
+	{
+	.callback = ac_not_use_pmic_quirk,
+	.ident = "ECS EF20EA",
+	.matches = {
+		DMI_MATCH(DMI_PRODUCT_NAME, "EF20EA"),
+		},
+	},
 	{},
 };
 
diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
index 258d2e66f230..fe024f1a8ad5 100644
--- a/drivers/acpi/battery.c
+++ b/drivers/acpi/battery.c
@@ -1233,6 +1233,13 @@ static const struct dmi_system_id bat_dmi_table[] __initconst = {
 			DMI_MATCH(DMI_PRODUCT_NAME, "UX410UAK"),
 		},
 	},
+	{
+		.callback = battery_not_use_pmic_quirk,
+		.ident = "ECS EF20EA",
+		.matches = {
+			DMI_MATCH(DMI_PRODUCT_NAME, "EF20EA"),
+		},
+	},
 	{},
 };
 
diff --git a/drivers/power/supply/axp288_fuel_gauge.c b/drivers/power/supply/axp288_fuel_gauge.c
index 4cc6e038dfdd..903891a9bcf0 100644
--- a/drivers/power/supply/axp288_fuel_gauge.c
+++ b/drivers/power/supply/axp288_fuel_gauge.c
@@ -708,6 +708,12 @@ static const struct dmi_system_id axp288_fuel_gauge_blacklist[] = {
 			DMI_MATCH(DMI_BOARD_VERSION, "V1.1"),
 		},
 	},
+	{
+		/* ECS EF20EA */
+		.matches = {
+			DMI_MATCH(DMI_PRODUCT_NAME, "EF20EA"),
+		},
+	},
 	{}
 };
 
-- 
2.14.1

Re: [PATCH 0/2] power: supply: Fix AXP288 fallback when not needed

[Hans de Goede]

Hi Carlo,

On 14-02-18 20:41, Carlo Caione wrote:
> From: Carlo Caione <carlo@endlessm.com>
> 
> With commits af3ec837 and dccfae6d a blacklist was introduced to avoid
> using the ACPI drivers for AC and battery when a native PMIC driver was
> already present. While this is in general a good idea (because of broken
> DSDT or proprietary and undocumented ACPI opregions for the ACPI
> AC/battery devices) we have come across at least one CherryTrail laptop
> (ECS EF20EA) shipping the AXP288 together with a separate FG controller
> (a MAX17047) instead of the one embedded in the AXP288.

Thank you for the patches, the code looks good, but I think you need
to split them up a bit differently, the patches under drivers/acpi
need to be merged into a different subsys tree as those under
drivers/power/supply, so the "power: supply: ACPI/AXP288: Add quirks
for ECS EF20EA" needs to be split I believe.

Also the patches to files under drivers/acpi should have an
"ACPI: " prefix for their subject AFAIK.

Regards,

Hans




> This is the interesting analisys done by Hans de Goede (thank you):
> 
> Looking at the _BIX method of the BATC/PNP0C0A device, we see it referencing
> FG10:
> 
> Method (_BIX, 0, NotSerialized)  // _BIX: Battery Information Extend
> {
>      If (AVBL == One)
>      {
>          BUF2 = FG10 /* \_SB_.PCI0.I2C1.FG10 */
> 
> And FG10 is defined as:
> 
> Field (DVFG, BufferAcc, NoLock, Preserve)
> {
>      Connection (SMFG),
>      Offset (0x10),
>      AccessAs (BufferAcc, AttribBytes (0x02)),
>      FG10,   8
> }
> 
> With SMFG being defined as:
> 
> Name (SMFG, ResourceTemplate ()
> {
>      I2cSerialBusV2 (0x0036, ControllerInitiated, 0x000186A0,
>          AddressingMode7Bit, "\\_SB.PCI0.I2C1",
>          0x00, ResourceConsumer, , Exclusive,
>          )
> })
> 
> Looking for I2C1 address 0x0036 we find:
> 
> Device (ANFG)
> {
>      Name (_HID, "MAX17047" /* Fuel Gauge Controller */)  // _HID: Hardwa
>      Name (_CID, "MAX17047" /* Fuel Gauge Controller */)  // _CID: Compat
>      Name (_DDN, "Fuel Gauge Controller")  // _DDN: DOS Device Name
>      Name (RBUF, ResourceTemplate ()
>      {
>          I2cSerialBusV2 (0x0036, ControllerInitiated, 0x00061A80,
>              AddressingMode7Bit, "\\_SB.PCI0.I2C1",
>              0x00, ResourceConsumer, , Exclusive,
>              )
>          GpioInt (Level, ActiveLow, ExclusiveAndWake, PullNone, 0x0000,
>              "\\_SB.GPO3", 0x00, ResourceConsumer, ,
>              )
>              {   // Pin list
>                  0x0001
>              }
>      })
> 
> Where as the AXP288 PMIC is I2C7 address 0x034:
> 
> Device (PMI1)
> {
>      Name (_ADR, Zero)  // _ADR: Address
>      Name (_HID, "INT33F4" /* XPOWER PMIC Controller */)  // _HID: Ha
>      Name (_CID, "INT33F4" /* XPOWER PMIC Controller */)  // _CID: Co
>      Name (_DDN, "XPOWER PMIC Controller")  // _DDN: DOS Device Name
>      Name (_HRV, 0x03)  // _HRV: Hardware Revision
>      Name (_UID, One)  // _UID: Unique ID
> 
>      Method (_CRS, 0, NotSerialized)  // _CRS: Current Resource Setti
>      {
>          Name (SBUF, ResourceTemplate ()
>          {
>              I2cSerialBusV2 (0x0034, ControllerInitiated, 0x000F4240,
>                  AddressingMode7Bit, "\\_SB.PCI0.I2C7",
>                  0x00, ResourceConsumer, , Exclusive,
>                  )
> 
> So basically this laptopt is using a separate FG chip instead of the one
> embedded in the AXP288.
> 
> To have this correctly working we need basically to avoid the fallback on the
> AXP288 driver enabling again the ACPI AC/battery drivers and at the same time
> avoiding that the AXP288 FG driver is probed at all.
> 
> I'm still not fully convinced that having two different quirks (one to disable
> the blacklist and another to disable the AXP288 FG probing) is the right way to
> fix this. So any comment is welcome.
> 
> Carlo Caione (2):
>    power: supply: ACPI/AXP288: Add quirk to avoid using PMIC
>    power: supply: ACPI/AXP288: Add quirks for ECS EF20EA
> 
>   drivers/acpi/ac.c                        | 33 ++++++++++++++++++++++++--------
>   drivers/acpi/battery.c                   | 33 ++++++++++++++++++++++++--------
>   drivers/power/supply/axp288_fuel_gauge.c |  6 ++++++
>   3 files changed, 56 insertions(+), 16 deletions(-)
>