Re: [PATCH v2 4/5] platform/x86/amd/pmf: Switch to platform_get_resource() and devm_ioremap_resource()

[Shyam Sundar S K <Shyam-sundar.S-k@amd.com>]

On 10/21/2024 16:18, Ilpo Järvinen wrote:
> On Tue, 15 Oct 2024, Shyam Sundar S K wrote:
> 
>> Use platform_get_resource() to fetch the memory resource instead of
>> acpi_walk_resources() and devm_ioremap_resource() for mapping the
>> resources.
>>
>> 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/Kconfig  |  1 +
>>  drivers/platform/x86/amd/pmf/acpi.c   | 37 ++++++++-------------------
>>  drivers/platform/x86/amd/pmf/pmf.h    |  6 +++--
>>  drivers/platform/x86/amd/pmf/tee-if.c |  8 +++---
>>  4 files changed, 20 insertions(+), 32 deletions(-)
>>
>> diff --git a/drivers/platform/x86/amd/pmf/Kconfig b/drivers/platform/x86/amd/pmf/Kconfig
>> index f4fa8bd8bda8..99d67cdbd91e 100644
>> --- a/drivers/platform/x86/amd/pmf/Kconfig
>> +++ b/drivers/platform/x86/amd/pmf/Kconfig
>> @@ -11,6 +11,7 @@ config AMD_PMF
>>  	select ACPI_PLATFORM_PROFILE
>>  	depends on TEE && AMDTEE
>>  	depends on AMD_SFH_HID
>> +	depends on HAS_IOMEM
>>  	help
>>  	  This driver provides support for the AMD Platform Management Framework.
>>  	  The goal is to enhance end user experience by making AMD PCs smarter,
>> diff --git a/drivers/platform/x86/amd/pmf/acpi.c b/drivers/platform/x86/amd/pmf/acpi.c
>> index d5b496433d69..2d871ff74fa7 100644
>> --- a/drivers/platform/x86/amd/pmf/acpi.c
>> +++ b/drivers/platform/x86/amd/pmf/acpi.c
>> @@ -433,37 +433,22 @@ int apmf_install_handler(struct amd_pmf_dev *pmf_dev)
>>  	return 0;
>>  }
>>  
>> -static acpi_status apmf_walk_resources(struct acpi_resource *res, void *data)
>> +int apmf_check_smart_pc(struct amd_pmf_dev *pmf_dev)
>>  {
>> -	struct amd_pmf_dev *dev = data;
>> +	struct platform_device *pdev = to_platform_device(pmf_dev->dev);
>>  
>> -	switch (res->type) {
>> -	case ACPI_RESOURCE_TYPE_ADDRESS64:
>> -		dev->policy_addr = res->data.address64.address.minimum;
>> -		dev->policy_sz = res->data.address64.address.address_length;
>> -		break;
>> -	case ACPI_RESOURCE_TYPE_FIXED_MEMORY32:
>> -		dev->policy_addr = res->data.fixed_memory32.address;
>> -		dev->policy_sz = res->data.fixed_memory32.address_length;
>> -		break;
>> -	}
>> -
>> -	if (!dev->policy_addr || dev->policy_sz > POLICY_BUF_MAX_SZ || dev->policy_sz == 0) {
>> -		pr_err("Incorrect Policy params, possibly a SBIOS bug\n");
>> -		return AE_ERROR;
>> +	pmf_dev->res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +	if (!pmf_dev->res) {
>> +		dev_err(pmf_dev->dev, "Failed to get I/O memory resource\n");
>> +		return -EINVAL;
>>  	}
>>  
>> -	return AE_OK;
>> -}
>> +	pmf_dev->policy_addr = pmf_dev->res->start;
>> +	pmf_dev->policy_sz = pmf_dev->res->end - pmf_dev->res->start;
> 
> A small thing still, I realized this should really have a comment because 
> it has a big risk of getting "fixed".
> 
> Also please describe what's going on here in the changelog (answer "why?") 
> since this is such a thing that people who look this code from history 
> have zero chance on figuring out the reasoning on their own.
> 

OK Sure. I will add a comment and leave a note in the changelog.

Thanks,
Shyam