Hi, On 11/3/21 10:18, Andy Shevchenko wrote: > On Wed, Nov 3, 2021 at 12:40 AM Hans de Goede <hdegoede@xxxxxxxxxx> wrote: > > ... > >> While working on this I realized that there also is a 4th option, >> which is basically option 1 from the v1 RFC minus the 2 gpiolib-acpi >> patches. >> >> With the 2nd option (as implemented by this RFC) we leave the >> _AEI handler in place and run the fuel-gauge without interrupt, >> we can do the same when marking the fuel-gauge as always present >> by treating IRQs on ACPI devices the same way as in the >> max17042_battery code, which has already solved the IRQ problem >> without disabling the _AEI handler: >> >> /* >> * On ACPI systems the IRQ may be handled by ACPI-event code, >> * so we need to share (if the ACPI code is willing to share). >> */ > >> if (acpi_id) >> flags |= IRQF_SHARED | IRQF_PROBE_SHARED; >> >> This is a pretty decent option too, it requires: >> >> 1. 2 more always_present quirks in the ACPI scan code which is part of >> the main kernel image. >> >> 2. Patches to the bq27xxx_battery code to support ACPI enumeration. > > If it works, why not try it? I'm 99.9% sure it will work, so I see no reason to actually try it unless we decide that this is a better option. This option would basically be patches 1 + 4-5 from option 1 / the v1 RFC: https://lore.kernel.org/platform-driver-x86/20211031162428.22368-1-hdegoede@xxxxxxxxxx/ plus extra changes to the bq27xxx_battery code IRQ handling like above. So this would mean 2 extra quirks in drivers/apci/x86/utils.c vs this RFC v2 patches + various changes to the bq27xxx_battery code, where as this (option 2/RFC v2) approach requires no changes to the bq27xxx_battery code at all (1). So at the cost of a board_file (patch 2/2 of this RFC v2 series) we save 2 quirks which would be in the main vmlinuz image for everyone, as well as not having to add any special code to the bq27xxx_battery code just for this one board; and as mentioned the board-file .ko will only ever auto-load on the actual board, so it just costs diskspace when enabled in generic distro kernels, where as e.g. the 2 quirks take op memory for everyone. So all in all I still believe that the approach in this v2 RFC is the best. Also note that we already have an i2c-client being instantiated for a bq27xxx_battery fuel-gauge (rather then forcing an ACPI-device as always- present) here: drivers/platform/x86/intel/int33fe/intel_cht_int33fe_microb.c and for the max17042 fg here: drivers/platform/x86/intel/int33fe/intel_cht_int33fe_typec.c So doing the manual instantiation already is somewhat of a pattern and IMHO is the best (least bad) option to deal with this. > I like the common base for the FG drivers that can be used as a pattern then. I don't believe this is a pattern which we want to introduce, with the exception of one buggy BIOS version (2) fuel-gauges on ACPI devices are never directly instantiated through an ACPI-device / fwnode. The whole business of using native fuel-gauge drivers on X86/ACPI is a special case just for BYT/CHT devices to begin with (because of broken or outright missing ACPI battery class support) and on all but the broken BIOS version the platform_dev or i2c-client for the fuel-gauge is instantiated by specific kernel-code, rather then directly from ACPI tables. So having ACPI enumeration support in fuel-gauge drivers is something which we do not need and should not want. (Arguably we also need an always_hidden option for the ACPI quirks and then use that to hide the anomaly of the MAX17042 ACPI device not being hidden on the one troublesome BIOS version, this would allow removing a nice amount of code in a bunch of places.) Regards, Hans 1) Well I notices some issues there which need fixes but those issues impact all users of the bq27xxx_battery code and are not related to the discussed enumeration issues 2) BIOS version, not board, other BIOS versions for the same board are fine
