Re: [PATCH 5/7] platform/x86/amd/pmf: Add support to get APTS index numbers for static slider

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

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

> APMF spec has a newer section called the APTS (AMD Performance and
> Thermal State) information, where each slider/power mode is associated
> with an index number.
> 
> Add support to get these indices for the Static Slider.
> 
> 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 | 10 +++++++
>  drivers/platform/x86/amd/pmf/pmf.h  | 24 +++++++++++++++++
>  drivers/platform/x86/amd/pmf/sps.c  | 42 ++++++++++++++++++++++++++++-
>  3 files changed, 75 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/x86/amd/pmf/acpi.c b/drivers/platform/x86/amd/pmf/acpi.c
> index 0fc8ad0ac3e9..28df45c058db 100644
> --- a/drivers/platform/x86/amd/pmf/acpi.c
> +++ b/drivers/platform/x86/amd/pmf/acpi.c
> @@ -96,6 +96,16 @@ int is_apmf_func_supported(struct amd_pmf_dev *pdev, unsigned long index)
>  	return !!(pdev->supported_func & BIT(index - 1));
>  }
>  
> +int apmf_get_static_slider_granular_v2(struct amd_pmf_dev *pdev,
> +				       struct apmf_static_slider_granular_output_v2 *data)
> +{
> +	if (!is_apmf_func_supported(pdev, APMF_FUNC_STATIC_SLIDER_GRANULAR))
> +		return -EINVAL;
> +
> +	return apmf_if_call_store_buffer(pdev, APMF_FUNC_STATIC_SLIDER_GRANULAR,
> +								data, sizeof(*data));

Wrong aligment. Please go through all the patches to check these.

> diff --git a/drivers/platform/x86/amd/pmf/sps.c b/drivers/platform/x86/amd/pmf/sps.c
> index 33e23e25c8b1..dc4c7ccd4c43 100644
> --- a/drivers/platform/x86/amd/pmf/sps.c
> +++ b/drivers/platform/x86/amd/pmf/sps.c
> @@ -10,6 +10,7 @@
>  
>  #include "pmf.h"
>  
> +static struct amd_pmf_static_slider_granular_v2 config_store_v2;
>  static struct amd_pmf_static_slider_granular config_store;
>  
>  #ifdef CONFIG_AMD_PMF_DEBUG
> @@ -63,10 +64,46 @@ static void amd_pmf_dump_sps_defaults(struct amd_pmf_static_slider_granular *dat
>  
>  	pr_debug("Static Slider Data - END\n");
>  }
> +
> +static void amd_pmf_dump_sps_defaults_v2(struct amd_pmf_static_slider_granular_v2 *data)
> +{
> +	pr_debug("Static Slider APTS state index data - BEGIN");
> +	pr_debug("size: %u\n", data->size);
> +	pr_debug("ac_best_perf: %u\n", data->sps_idx.ac_best_perf);
> +	pr_debug("ac_balanced: %u\n", data->sps_idx.ac_balanced);
> +	pr_debug("ac_best_pwr_efficiency: %u\n", data->sps_idx.ac_best_pwr_efficiency);
> +	pr_debug("ac_energy_saver: %u\n", data->sps_idx.ac_energy_saver);
> +	pr_debug("dc_best_perf: %u\n", data->sps_idx.dc_best_perf);
> +	pr_debug("dc_balanced: %u\n", data->sps_idx.dc_balanced);
> +	pr_debug("dc_best_pwr_efficiency: %u\n", data->sps_idx.dc_best_pwr_efficiency);
> +	pr_debug("dc_battery_saver: %u\n", data->sps_idx.dc_battery_saver);

I know these are debug only but what is the advantage of having the 
underscores in them? I think they'd read & match just fine without them 
(perhaps pwr->power would be better but it's up to you) and ac/dc can then 
be capitalized.


-- 
 i.