Message-ID: <87d2068b-2644-0307-d722-5539b1f9fb36@redhat.com>
Date:   Thu, 16 Feb 2023 14:07:53 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.7.1
Subject: Re: [PATCH v2 3/5] apple-gmux: Use GMSP acpi method for interrupt
 clear
To:     Orlando Chamberlain <orlandoch.dev@gmail.com>
Cc:     Mark Gross <markgross@kernel.org>,
        platform-driver-x86@vger.kernel.org, linux-kernel@vger.kernel.org,
        Lukas Wunner <lukas@wunner.de>,
        Seth Forshee <sforshee@kernel.org>,
        Aditya Garg <gargaditya08@live.com>,
        Aun-Ali Zaidi <admin@kodeit.net>,
        Kerem Karabay <kekrby@gmail.com>
References: <20230216122342.5918-1-orlandoch.dev@gmail.com>
 <20230216122342.5918-4-orlandoch.dev@gmail.com>
Content-Language: en-US, nl
From:   Hans de Goede <hdegoede@redhat.com>
In-Reply-To: <20230216122342.5918-4-orlandoch.dev@gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi Orlando,

Thank you for the new version patches 1 + 2 look good,
one small remark on this one.

On 2/16/23 13:23, Orlando Chamberlain wrote:
> This is needed for interrupts to be cleared correctly on MMIO based
> gmux's. It is untested if this helps/hinders other gmux types, so
> currently this is only enabled for the MMIO gmux's.
> 
> There is also a "GMLV" acpi method, and the "GMSP" method can be called
> with 1 as its argument, but the purposes of these aren't known and they
> don't seem to be needed.
> 
> Signed-off-by: Orlando Chamberlain <orlandoch.dev@gmail.com>
> ---
> v1->v2: Only enable this on MMIO gmux's
>  drivers/platform/x86/apple-gmux.c | 30 +++++++++++++++++++++++++++++-
>  1 file changed, 29 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/x86/apple-gmux.c b/drivers/platform/x86/apple-gmux.c
> index 36208e93d745..12a93fc49c36 100644
> --- a/drivers/platform/x86/apple-gmux.c
> +++ b/drivers/platform/x86/apple-gmux.c
> @@ -76,6 +76,7 @@ struct apple_gmux_config {
>  	enum vga_switcheroo_handler_flags_t handler_flags;
>  	unsigned long resource_type;
>  	bool read_version_as_u32;
> +	bool use_acpi_gmsp;
>  	char *name;
>  };
>  
> @@ -488,6 +489,7 @@ static const struct apple_gmux_config apple_gmux_pio = {
>  	.handler_flags = VGA_SWITCHEROO_CAN_SWITCH_DDC,
>  	.resource_type = IORESOURCE_IO,
>  	.read_version_as_u32 = false,
> +	.use_acpi_gmsp = false,
>  	.name = "classic"
>  };
>  
> @@ -500,6 +502,7 @@ static const struct apple_gmux_config apple_gmux_index = {
>  	.handler_flags = VGA_SWITCHEROO_NEEDS_EDP_CONFIG,
>  	.resource_type = IORESOURCE_IO,
>  	.read_version_as_u32 = true,
> +	.use_acpi_gmsp = false,
>  	.name = "indexed"
>  };
>  
> @@ -511,8 +514,29 @@ static const struct apple_gmux_config apple_gmux_index = {
>   * MCP79, on all following generations it's GPIO pin 6 of the Intel PCH.
>   * The GPE merely signals that an interrupt occurred, the actual type of event
>   * is identified by reading a gmux register.
> + *
> + * On MMIO gmux's, we also need to call the acpi method GMSP to properly clear
> + * interrupts.
>   */
>  
> +static int gmux_call_acpi_gmsp(struct apple_gmux_data *gmux_data, int arg)
> +{
> +	acpi_status status = AE_OK;
> +	union acpi_object arg0 = { ACPI_TYPE_INTEGER };
> +	struct acpi_object_list arg_list = { 1, &arg0 };
> +
> +	arg0.integer.value = arg;
> +
> +	status = acpi_evaluate_object(gmux_data->dhandle, "GMSP", &arg_list, NULL);
> +	if (ACPI_FAILURE(status)) {
> +		pr_err("GMSP call failed: %s\n",
> +		       acpi_format_exception(status));
> +		return -ENODEV;
> +	}
> +
> +	return 0;
> +}
> +
>  static inline void gmux_disable_interrupts(struct apple_gmux_data *gmux_data)
>  {
>  	gmux_write8(gmux_data, GMUX_PORT_INTERRUPT_ENABLE,
> @@ -536,7 +560,11 @@ static void gmux_clear_interrupts(struct apple_gmux_data *gmux_data)
>  
>  	/* to clear interrupts write back current status */
>  	status = gmux_interrupt_get_status(gmux_data);
> -	gmux_write8(gmux_data, GMUX_PORT_INTERRUPT_STATUS, status);
> +	if (status) {
> +		gmux_write8(gmux_data, GMUX_PORT_INTERRUPT_STATUS, status);
> +		if (gmux_data->config->use_acpi_gmsp)
> +			gmux_call_acpi_gmsp(gmux_data, 0);
> +	}

This changes the behavior on the existing supported models to
only write back status when it is non 0. This is likely fine
but given that we seem to lack testers for the old models
I would prefer to not change the behavior there.

So how about:

	gmux_write8(gmux_data, GMUX_PORT_INTERRUPT_STATUS, status);
	if (status && gmux_data->config->use_acpi_gmsp)
		gmux_call_acpi_gmsp(gmux_data, 0);

?

The 0 write to what presumably is a register with
write 1 to clear bits should be harmless.

You can test that it is harmless on the new MMIO models
and this way we don't change the behavior on the older models.

Regards,

Hans