Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 13 Sep 2022 18:44:03 +0200
From:   Philipp Zabel <philipp.zabel@gmail.com>
To:     Mario Limonciello <mario.limonciello@amd.com>
Cc:     rafael@kernel.org, linux-kernel@vger.kernel.org,
        catalin@antebit.com, travisghansen@yahoo.com,
        Shyam-sundar.S-k@amd.com, Len Brown <lenb@kernel.org>,
        linux-acpi@vger.kernel.org
Subject: Re: [PATCH v2 1/6] acpi/x86: s2idle: Move _HID handling for AMD
 systems into structures
Message-ID: <YyCzU12b+bX3yvvu@rog>
References: <20220912172401.22301-1-mario.limonciello@amd.com>
 <20220912172401.22301-2-mario.limonciello@amd.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220912172401.22301-2-mario.limonciello@amd.com>
Precedence: bulk

Am Mon, Sep 12, 2022 at 12:23:55PM -0500 schrieb Mario Limonciello:
> Right now the information about which cases to use for what are in a
> comment, but this is error prone.  Instead move all information into
> a dedicated structure.
> 
> Tested-by: catalin@antebit.com
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
>  drivers/acpi/x86/s2idle.c | 63 ++++++++++++++++++++++++++++-----------
>  1 file changed, 46 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/acpi/x86/s2idle.c b/drivers/acpi/x86/s2idle.c
> index f9ac12b778e6..a7757551f750 100644
> --- a/drivers/acpi/x86/s2idle.c
> +++ b/drivers/acpi/x86/s2idle.c
> @@ -363,6 +363,39 @@ static int validate_dsm(acpi_handle handle, const char *uuid, int rev, guid_t *d
>  	return ret;
>  }
>  
> +struct amd_lps0_hid_device_data {
> +	const unsigned int rev_id;
> +	const bool check_off_by_one;
> +	const bool prefer_amd_guid;
> +};
> +
> +static const struct amd_lps0_hid_device_data amd_picasso = {
> +	.rev_id = 0,
> +	.check_off_by_one = true,
> +	.prefer_amd_guid = false,
> +};
> +
> +static const struct amd_lps0_hid_device_data amd_cezanne = {
> +	.rev_id = 0,
> +	.check_off_by_one = false,
> +	.prefer_amd_guid = false,
> +};
> +
> +static const struct amd_lps0_hid_device_data amd_rembrandt = {
> +	.rev_id = 2,
> +	.check_off_by_one = false,
> +	.prefer_amd_guid = true,
> +};
> +
> +static const struct acpi_device_id amd_hid_ids[] = {
> +	{"AMD0004",	(kernel_ulong_t)&amd_picasso,	},
> +	{"AMD0005",	(kernel_ulong_t)&amd_picasso,	},
> +	{"AMDI0005",	(kernel_ulong_t)&amd_picasso,	},
> +	{"AMDI0006",	(kernel_ulong_t)&amd_cezanne,	},
> +	{"AMDI0007",	(kernel_ulong_t)&amd_rembrandt,	},
> +	{}
> +};
> +
>  static int lps0_device_attach(struct acpi_device *adev,
>  			      const struct acpi_device_id *not_used)
>  {
> @@ -370,31 +403,27 @@ static int lps0_device_attach(struct acpi_device *adev,
>  		return 0;
>  
>  	if (acpi_s2idle_vendor_amd()) {
> -		/* AMD0004, AMD0005, AMDI0005:
> -		 * - Should use rev_id 0x0
> -		 * - function mask > 0x3: Should use AMD method, but has off by one bug
> -		 * - function mask = 0x3: Should use Microsoft method
> -		 * AMDI0006:
> -		 * - should use rev_id 0x0
> -		 * - function mask = 0x3: Should use Microsoft method
> -		 * AMDI0007:
> -		 * - Should use rev_id 0x2
> -		 * - Should only use AMD method
> -		 */
> -		const char *hid = acpi_device_hid(adev);
> -		rev_id = strcmp(hid, "AMDI0007") ? 0 : 2;
> +		static const struct acpi_device_id *dev_id;
> +		const struct amd_lps0_hid_device_data *data;
> +
> +		for (dev_id = &amd_hid_ids[0]; dev_id->id[0]; dev_id++)
> +			if (acpi_dev_hid_uid_match(adev, dev_id->id, NULL))
> +				break;
> +		if (dev_id != NULL)
> +			data = (const struct amd_lps0_hid_device_data *) dev_id->driver_data;
> +		else
> +			return 0;

The "!= NULL" seems unnecessary, I would change this to:

+		if (!dev_id)
+			return 0;
+		data = (const struct amd_lps0_hid_device_data *) dev_id->driver_data;

But either way,

Reviewed-by: Philipp Zabel <philipp.zabel@gmail.com>
Tested-by: Philipp Zabel <philipp.zabel@gmail.com> # GA402RJ

regards
Philipp