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

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

Hi,

Thanks for the feedback. I will send out a new version on rc1 of 6.14 cycle.

Xi

On 1/14/2025 8:39 AM, Xi Pardee wrote:
>
> 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
>
>