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

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 4/29/20 4:21 PM, Andy Shevchenko wrote:
> 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

Fixed for v2.

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

Good point, also dropped for v2.

>> +		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)

I think that for backporting it is best to keep this patch as is
(with your other comments addresed). Also this way this can hopefully
be merged for a 5.7-rc# candidate.

I will do a follow up series adding a helper and moving both cases
over to the helper.

Regards,

Hans