Re: [PATCH 4/7] platform/x86/amd/pmf: Add support to notify sbios heart beat event

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

On Tue, 27 Feb 2024, Shyam Sundar S K wrote:

> Add support for newer revision of the heart beat notify events.
> This event is used to notify to the OEM BIOS on driver
> load/unload/suspend/resume scenarios.
> 
> If OEM BIOS does not receive the heart beat event from PMF driver, OEM
> BIOS shall conclude that PMF driver is no more active and BIOS will
> update to the legacy system power thermals.
> 
> Co-developed-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
> Signed-off-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> ---
>  drivers/platform/x86/amd/pmf/acpi.c | 42 +++++++++++++++++++++++++++--
>  drivers/platform/x86/amd/pmf/core.c | 10 +++++++
>  drivers/platform/x86/amd/pmf/pmf.h  | 17 ++++++++++++
>  3 files changed, 67 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/platform/x86/amd/pmf/acpi.c b/drivers/platform/x86/amd/pmf/acpi.c
> index 1b2a099c0cef..0fc8ad0ac3e9 100644
> --- a/drivers/platform/x86/amd/pmf/acpi.c
> +++ b/drivers/platform/x86/amd/pmf/acpi.c
> @@ -140,6 +140,44 @@ static void apmf_sbios_heartbeat_notify(struct work_struct *work)
>  	kfree(info);
>  }
>  
> +int amd_pmf_notify_sbios_heartbeat_event_v2(struct amd_pmf_dev *dev, u8 flag)
> +{
> +	struct sbios_hb_event_v2 args = { };
> +	struct acpi_buffer params;
> +	union acpi_object *info;
> +
> +	args.size = sizeof(args);
> +
> +	switch (flag) {
> +	case ON_LOAD:
> +		args.load = 1;
> +		break;
> +	case ON_UNLOAD:
> +		args.unload = 1;
> +		break;
> +	case ON_SUSPEND:
> +		args.suspend = 1;
> +		break;
> +	case ON_RESUME:
> +		args.resume = 1;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	params.length = sizeof(args);
> +	params.pointer = (void *)&args;

Casting type pointer to void pointer (and vice-versa) is not necessary as 
it's implicitly done for you by the compiler.

> +	info = apmf_if_call(dev, APMF_FUNC_SBIOS_HEARTBEAT_V2, &params);
> +	if (!info)
> +		return -EIO;
> +
> +	dev_dbg(dev->dev, "Sending v2 heartbeat event to SBIOS\n");

Perhaps including the flag would make this more useful. It's a bit odd 
though you dev_dbg the success but not the failure case. I'd tend to think 
the failure is more useful to know than things working normally.

-- 
 i.