Re: [PATCH v3 3/4] platform/x86/amd: pmc: Add helper function to check the cpu id

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

On Tue, 16 May 2023, Shyam Sundar S K wrote:

> Add a helper routine to check the underlying cpu id, that can be used
> across the PMC driver to remove the duplicate code.
> 
> Co-developed-by: Sanket Goswami <Sanket.Goswami@amd.com>
> Signed-off-by: Sanket Goswami <Sanket.Goswami@amd.com>
> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> ---
>  drivers/platform/x86/amd/pmc.c | 17 ++++++++++++++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/platform/x86/amd/pmc.c b/drivers/platform/x86/amd/pmc.c
> index e2439fda5c02..7e5e6afb3410 100644
> --- a/drivers/platform/x86/amd/pmc.c
> +++ b/drivers/platform/x86/amd/pmc.c
> @@ -564,6 +564,18 @@ static void amd_pmc_dbgfs_unregister(struct amd_pmc_dev *dev)
>  	debugfs_remove_recursive(dev->dbgfs_dir);
>  }
>  
> +static bool amd_pmc_check_sup_cpuid(struct amd_pmc_dev *dev)

Does sup refer to "supported" or some other acronym? If the latter, 
you should mention/open it in the changelog and/or in a comment. If the 
former, the function naming seems too generic (an observation entirely 
based on how/where the function is used, you're not exactly verbose on 
what this actually checks for other than what looks like a set of CPU 
IDs but clearly there's more behind it).

-- 
 i.


> +{
> +	switch (dev->cpu_id) {
> +	case AMD_CPU_ID_YC:
> +	case AMD_CPU_ID_CB:
> +	case AMD_CPU_ID_PS:
> +		return true;
> +	default:
> +		return false;
> +	}
> +}
> +
>  static void amd_pmc_dbgfs_register(struct amd_pmc_dev *dev)
>  {
>  	dev->dbgfs_dir = debugfs_create_dir("amd_pmc", NULL);
> @@ -575,8 +587,7 @@ static void amd_pmc_dbgfs_register(struct amd_pmc_dev *dev)
>  			    &amd_pmc_idlemask_fops);
>  	/* Enable STB only when the module_param is set */
>  	if (enable_stb) {
> -		if (dev->cpu_id == AMD_CPU_ID_YC || dev->cpu_id == AMD_CPU_ID_CB ||
> -		    dev->cpu_id == AMD_CPU_ID_PS)
> +		if (amd_pmc_check_sup_cpuid(dev))
>  			debugfs_create_file("stb_read", 0644, dev->dbgfs_dir, dev,
>  					    &amd_pmc_stb_debugfs_fops_v2);
>  		else
> @@ -1036,7 +1047,7 @@ static int amd_pmc_probe(struct platform_device *pdev)
>  
>  	mutex_init(&dev->lock);
>  
> -	if (enable_stb && (dev->cpu_id == AMD_CPU_ID_YC || dev->cpu_id == AMD_CPU_ID_CB)) {
> +	if (enable_stb && amd_pmc_check_sup_cpuid(dev)) {
>  		err = amd_pmc_s2d_init(dev);
>  		if (err)
>  			goto err_pci_dev_put;
>