| Message ID | 20241207053607.14806-3-xi.pardee@linux.intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Add Arrow Lake U/H support | expand |
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?
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); }
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(-)