Re: [PATCH 2/3] platform/x86:intel/pmc: Create info structure for pmc device

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

On Fri, 6 Dec 2024, Xi Pardee wrote:

> Create an info structure to store platform specific information.
> For Tiger Lake and Arrow Lake platforms, multiple platform variations
> could share one generic init function. Using info structure could
> avoid if () forest. Modify tgl.c to use the info structure.
> 
> Signed-off-by: Xi Pardee <xi.pardee@linux.intel.com>
> ---
>  drivers/platform/x86/intel/pmc/core.h | 15 ++++++++++++
>  drivers/platform/x86/intel/pmc/tgl.c  | 33 +++++++++++++++------------
>  2 files changed, 33 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel/pmc/core.h b/drivers/platform/x86/intel/pmc/core.h
> index a1886d8e1ef3e..3124315d2b925 100644
> --- a/drivers/platform/x86/intel/pmc/core.h
> +++ b/drivers/platform/x86/intel/pmc/core.h
> @@ -428,6 +428,21 @@ struct pmc_dev {
>  	struct pmc_info *regmap_list;
>  };
>  
> +/**
> + * struct pmc_dev_info - Structure to keep pmc device info
> + * @func:		function number of the primary pmc
> + * @map:		pointer to a pmc_reg_map struct that contains platform
> + *			specific attributes of the primary pmc
> + * @suspend:		Function to perform platform specific suspend
> + * @resume:		Function to perform platform specific resume
> + */
> +struct pmc_dev_info {
> +	u8 func;
> +	const struct pmc_reg_map *map;
> +	void (*suspend)(struct pmc_dev *pmcdev);
> +	int (*resume)(struct pmc_dev *pmcdev);
> +};
> +
>  enum pmc_index {
>  	PMC_IDX_MAIN,
>  	PMC_IDX_SOC = PMC_IDX_MAIN,
> diff --git a/drivers/platform/x86/intel/pmc/tgl.c b/drivers/platform/x86/intel/pmc/tgl.c
> index 4fec43d212d01..c6fc3a0225a55 100644
> --- a/drivers/platform/x86/intel/pmc/tgl.c
> +++ b/drivers/platform/x86/intel/pmc/tgl.c
> @@ -13,11 +13,6 @@
>  #define ACPI_S0IX_DSM_UUID		"57a6512e-3979-4e9d-9708-ff13b2508972"
>  #define ACPI_GET_LOW_MODE_REGISTERS	1
>  
> -enum pch_type {
> -	PCH_H,
> -	PCH_LP
> -};
> -
>  const struct pmc_bit_map tgl_pfear_map[] = {
>  	{"PSF9",		BIT(0)},
>  	{"RES_66",		BIT(1)},
> @@ -285,18 +280,26 @@ void pmc_core_get_tgl_lpm_reqs(struct platform_device *pdev)
>  	ACPI_FREE(out_obj);
>  }
>  
> -static int tgl_core_generic_init(struct pmc_dev *pmcdev, int pch_tp)
> +static struct pmc_dev_info tgl_l_pmc_dev = {
> +	.map = &tgl_reg_map,
> +	.suspend = cnl_suspend,
> +	.resume = cnl_resume,
> +};
> +
> +static struct pmc_dev_info tgl_pmc_dev = {
> +	.map = &tgl_h_reg_map,
> +	.suspend = cnl_suspend,
> +	.resume = cnl_resume,
> +};
> +
> +static int tgl_core_generic_init(struct pmc_dev *pmcdev, struct pmc_dev_info *pmc_dev_info)
>  {
>  	struct pmc *pmc = pmcdev->pmcs[PMC_IDX_MAIN];
>  	int ret;
>  
> -	if (pch_tp == PCH_H)
> -		pmc->map = &tgl_h_reg_map;
> -	else
> -		pmc->map = &tgl_reg_map;
> -
> -	pmcdev->suspend = cnl_suspend;
> -	pmcdev->resume = cnl_resume;
> +	pmc->map = pmc_dev_info->map;
> +	pmcdev->suspend = pmc_dev_info->suspend;
> +	pmcdev->resume = pmc_dev_info->resume;
>  
>  	ret = get_primary_reg_base(pmc);
>  	if (ret)
> @@ -310,10 +313,10 @@ static int tgl_core_generic_init(struct pmc_dev *pmcdev, int pch_tp)
>  
>  int tgl_l_core_init(struct pmc_dev *pmcdev)
>  {
> -	return tgl_core_generic_init(pmcdev, PCH_LP);
> +	return tgl_core_generic_init(pmcdev, &tgl_l_pmc_dev);
>  }
>  
>  int tgl_core_init(struct pmc_dev *pmcdev)
>  {
> -	return tgl_core_generic_init(pmcdev, PCH_H);
> +	return tgl_core_generic_init(pmcdev, &tgl_pmc_dev);
>  }
> 

While adding the struct seems right direction, I don't feel good about the 
scoping in this patch. It seems that we'll still end up duplicating lots
of init code.

If I do (in drivers/platform/x86/intel/pmc):

git grep -A7 -B7 -e 'suspend =' -e 'resume =' -e '->map ='

...I see basically:

- assign those variables (minor variations: PMC_IDX_MAIN/PMC_IDX_SOC 
  index, NULL resume/suspend that is already handled)
- if regmap_list is set, call to pmc_core_ssram_init() (the pointer put 
  into ->regmap_list seems another candidate to be included into the info 
  struct.)
- else (or if ssram init failed), call get_primary_reg_base()
- call pmc_core_get_low_power_modes()

Why cannot we like have a common init function which handles all those and 
remove all that duplication from per arch files? I don't see anything that 
looked meaningful variations so the current archs should be all 
generalizable I think. Why leave all those into per arch init as done in 
this series?

-- 
 i.