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

[Xi Pardee <xi.pardee@linux.intel.com>]

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@linux.intel.com>
>> ---
>>   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