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

[Mika Westerberg <mika.westerberg@xxxxxxxxxxxxxxx>]

+Rafael and ACPICA folks.

On Mon, May 04, 2020 at 04:59:57PM +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.

Do we know why this works under Windows? I mean if possible we should do
the same and I kind of suspect that they forcibly call _REG in their
GPIO driver.

Are the ACPI tables from this system available somewhere?

> 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.

I would like to understand the issue bit better before we do this.

> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
> ---
> Changes in v2:
> - Drop unnecessary if (acpi_has_method(adev->handle, "_REG")) check
> - Fix Cherryview spelling in the commit message
> ---
>  drivers/pinctrl/intel/pinctrl-cherryview.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/drivers/pinctrl/intel/pinctrl-cherryview.c b/drivers/pinctrl/intel/pinctrl-cherryview.c
> index 4c74fdde576d..4817aec114d6 100644
> --- a/drivers/pinctrl/intel/pinctrl-cherryview.c
> +++ b/drivers/pinctrl/intel/pinctrl-cherryview.c
> @@ -1693,6 +1693,8 @@ static acpi_status chv_pinctrl_mmio_access_handler(u32 function,
>  
>  static int chv_pinctrl_probe(struct platform_device *pdev)
>  {
> +	struct acpi_object_list input;
> +	union acpi_object params[2];
>  	struct chv_pinctrl *pctrl;
>  	struct acpi_device *adev;
>  	acpi_status status;
> @@ -1755,6 +1757,22 @@ static int chv_pinctrl_probe(struct platform_device *pdev)
>  	if (ACPI_FAILURE(status))
>  		dev_err(&pdev->dev, "failed to install ACPI addr space handler\n");
>  
> +	/*
> +	 * Some DSDT-s use the chv_pinctrl_mmio_access_handler while checking
> +	 * for the regular GeneralPurposeIo OpRegion availability, mixed with
> +	 * the DSDT not defining a GeneralPurposeIo OpRegion at all. In this
> +	 * case the ACPICA code will not call _REG to signal availability of
> +	 * the GeneralPurposeIo OpRegion. Manually call _REG here so that
> +	 * the DSDT-s GeneralPurposeIo availability checks will succeed.
> +	 */
> +	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;
> +	input.count = 2;
> +	input.pointer = params;
> +	acpi_evaluate_object(adev->handle, "_REG", &input, NULL);
> +
>  	platform_set_drvdata(pdev, pctrl);
>  
>  	return 0;
> -- 
> 2.26.0