Re: [PATCH v4 4/6] platform/x86/intel/pmc: Create architecture specific callback

[Xi Pardee]

On 1/10/2025 3:50 AM, Ilpo Järvinen wrote:
> On Thu, 9 Jan 2025, Xi Pardee wrote:
>
> > Add architecture specific callback field in pmc_dev_info structure.
> > Architecture specific action could be handled in this callback instead
> > of per architecture init functions. Convert Arrow Lake, Lunar Lake,
> > Meteor Lake and Tiger Lake platforms to use this field.
> >
> > Signed-off-by: Xi Pardee <xi.pardee@xxxxxxxxxxxxxxx>
> > ---
> >   drivers/platform/x86/intel/pmc/arl.c  | 15 +++++++--------
> >   drivers/platform/x86/intel/pmc/core.c |  3 +++
> >   drivers/platform/x86/intel/pmc/core.h |  7 +++++++
> >   drivers/platform/x86/intel/pmc/lnl.c  | 15 +++++++--------
> >   drivers/platform/x86/intel/pmc/mtl.c  | 15 +++++++--------
> >   drivers/platform/x86/intel/pmc/tgl.c  | 15 +++++----------
> >   6 files changed, 36 insertions(+), 34 deletions(-)
> >
> > diff --git a/drivers/platform/x86/intel/pmc/arl.c b/drivers/platform/x86/intel/pmc/arl.c
> > index dedf752237ca0..9ff90d32a635f 100644
> > --- a/drivers/platform/x86/intel/pmc/arl.c
> > +++ b/drivers/platform/x86/intel/pmc/arl.c
> > @@ -698,16 +698,15 @@ static struct pmc_dev_info arl_pmc_dev = {
> >   	.map = &arl_socs_reg_map,
> >   	.suspend = cnl_suspend,
> >   	.resume = arl_resume,
> > +	.arch_specific = arl_specific_init,
> >   };
> >   
> > -int arl_core_init(struct pmc_dev *pmcdev)
> > +void arl_specific_init(struct pmc_dev *pmcdev)
> >   {
> > -	int ret;
> > -
> > -	ret = generic_core_init(pmcdev, &arl_pmc_dev);
> > -	if (ret)
> > -		return ret;
> > -
> >   	arl_d3_fixup();
> > -	return 0;
> > +}
> As I tried to explain already earlier I think the older form is better
> here because it would allow arch specific things in any order:
>
> void xx_init(struct pmc_dev *pmcdev, struct pmc_dev_info *pmc_dev_info)
> {
> 	int ret;
>
> 	xx_pre_fixup();
>
> 	ret = generic_core_init(pmcdev, pmc_dev_info);
> 	if (ret)
> 		return ret;
>
> 	xx_post_fixup();
>
> 	return 0;
> }
>
> If you make it a callback, you have pick either pre or post but cannot do
> both with a single callback. My suggestion would also allow replacing
> generic_core_init() completely if needed in future.
>
> Note how I pass the info parameter above to xx_init() so it can call into
> generic_core_init(). In the core, you'd do this to pick which init
> function to use:
>
> 	if (pmc_dev_info->init)
> 		ret = pmc_dev_info->init(pmcdev, pmc_dev_info);
> 	else
> 		ret = generic_core_init(pmcdev, pmc_dev_info);


Thanks for the feedback. I will change back to init function per 
architect in next version.

Xi