| Message ID | 20211031162428.22368-1-hdegoede@redhat.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | ACPI/power-suppy add fuel-gauge support on cht-wc PMIC without USB-PD support devs | expand |
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.
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
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(-)