Re: [PATCH] pinctrl: cherryview: Ensure _REG(ACPI_ADR_SPACE_GPIO, 1) gets called

[Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>]

On Wed, Apr 29, 2020 at 12:46:51PM +0200, Hans de Goede wrote:
> On Cherry Trail devices there are 2 possible ACPI OpRegions for
> accessing GPIOs. The standard GeneralPurposeIo OpRegion and the Cherry
> Trail specific UserDefined 0x9X OpRegions.
> 
> Having 2 different types of OpRegions leads to potential issues with
> checks for OpRegion availability, or in other words checks if _REG has
> been called for the OpRegion which the ACPI code wants to use.
> 
> The ACPICA core does not call _REG on an ACPI node which does not
> define an OpRegion matching the type being registered; and the reference
> design DSDT, from which most Cherry Trail DSDTs are derived, does not
> define GeneralPurposeIo, nor UserDefined(0x93) OpRegions for the GPO2
> (UID 3) device, because no pins were assigned ACPI controlled functions
> in the reference design.
> 
> Together this leads to the perfect storm, at least on the Cherry Trail
> based Medion Akayo E1239T. This design does use a GPO2 pin from its ACPI
> code and has added the Cherry Trail specific UserDefined(0x93) opregion
> to its GPO2 ACPI node to access this pin.
> 
> But it uses a has _REG been called availability check for the standard
> GeneralPurposeIo OpRegion. This clearly is a bug in the DSDT, but this
> does work under Windows. This issue leads to the intel_vbtn driver
> reporting the device always being in tablet-mode at boot, even if it
> is in laptop mode. Which in turn causes userspace to ignore touchpad
> events. So iow this issues causes the touchpad to not work at boot.
> 
> Since the bug in the DSDT stems from the confusion of having 2 different
> OpRegion types for accessing GPIOs on Cherry Trail devices, I believe
> that this is best fixed inside the cherryview pinctrl driver.
> 
> This commit adds a workaround to the cherryview pinctrl driver so
> that the DSDT's expectations of _REG always getting called for the
> GeneralPurposeIo OpRegion are met.

s/cherryview/Cherryview/g

...

> +	if (acpi_has_method(adev->handle, "_REG")) {

And this check si redundant, you may call it as is (you didn't check for error
anyway), see also below.

> +		struct acpi_object_list input;
> +		union acpi_object params[2];
> +
> +		input.count = 2;
> +		input.pointer = params;
> +		params[0].type = ACPI_TYPE_INTEGER;
> +		params[0].integer.value = ACPI_ADR_SPACE_GPIO;
> +		params[1].type = ACPI_TYPE_INTEGER;
> +		params[1].integer.value = 1;
> +		acpi_evaluate_object(adev->handle, "_REG", &input, NULL);
> +	}

Can you consider to unify this with one in drivers/pci/hotplug/acpiphp_glue.c,
so we will have some helper function at the end? (perhaps as separate changes
to make less burden on backporting this one)

-- 
With Best Regards,
Andy Shevchenko