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@xxxxxxxxxxxxxxx>
> ---
> 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.
