Re: [PATCH 0/7] Create Intel PMC SSRAM Telemetry driver

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

On 8/13/2024 9:01 AM, Ilpo Järvinen wrote:
> On Fri, 9 Aug 2024, Xi Pardee wrote:
>
>> This patch series removes the SSRAM support from Intel PMC Core driver
>> and creates a separate PCI driver for SSRAM device. The new Intel PMC
>> SSRAM driver provides the following functionalities:
>>   
>> 1. Search and store the PMC information in a structure, including PWRMBASE
>> address and devid for each available PMC. Then Intel PMC Core driver
>> achieves the PMC information using the API provided by the new driver.
>>   
>> 2. Search and register Intel Platform Monitoring Techology telemetry
>> regions so they would by available for read through sysfs and Intel PMT
>> API. Intel PMC Core driver can achieve Low Power Mode requirement
>> information from a telemetry region registered by the new driver.
>> The above functionalities was previously handled by Intel PMC Core
>> driver. Intel PMC Core driver returns -EPROBE_DEFER when trying to read
>> data from a telem region that is not available yet. This setup may
>> result in an infinite loop of .probe() calls as Intel PMC Core driver
>> creates child devices. Creating a separate PCI driver avoids the infinite
>> loop possibility.
>>   
>> Xi Pardee (7):
>>    platform/x86:intel/pmc: Remove SSRAM support from PMC Core
>>    platform/x86:intel/pmc: Create Intel PMC SSRAM Telemetry driver
>>    platform/x86:intel/pmc: Add support to get PMC information from SSRAM
>>    platform/x86:intel/pmt: Get PMC from SSRAM for available platforms
>>    platform/x86:intel/pmt: Create inline version for telemetry functions
>>    platform/x86:intel/pmc: Add support to Retrieve LPM information
>>    platform/x86:intel/pmc: Get LPM information for available platforms
> Hi,
>
> I don't see why the removal first, then re-add approach would be justified
> here. You're basically adding the same code back later in many cases with
> only very minimal changes, and some changes are entirely pointless such as
> pmc_idx -> pmc_index parameter rename. This is just a big pain to review.
>
> I'd suggest you move functions in first patch into core.c. Try to
> avoid logic/code changes other than making making the necessary functions
> non-static and adding the prototypes for them into a header (temporarily).
>
> Then rename the ssram file to its new name in the second change.
>
> Then do the rework on top of that (and make things back static again).
>
> Try to split the rework into sensible chunks, anything that can be taken
> away from the main rework change is less lines to review in that patch.
> If you e.g. want to do pcidev -> pdev renames, put them into own separate
> change (and do it consistently then, not just for some of the cases like
> currently :-/).
>
> The move patches are nearly trivial to review and take large chunk of
> diff away from the actual rework itself which doesn't seem that
> complicated to review once the 1:1 move bits and trivial rename churn is
> eliminated from the diff.


Hi,

Thanks for reviewing the patches. I will rearrange the code in next version.

Xi

>>   drivers/platform/x86/intel/pmc/Kconfig        |  13 +-
>>   drivers/platform/x86/intel/pmc/Makefile       |   8 +-
>>   drivers/platform/x86/intel/pmc/arl.c          |  36 +-
>>   drivers/platform/x86/intel/pmc/core.c         | 216 +++++++++++-
>>   drivers/platform/x86/intel/pmc/core.h         |  25 +-
>>   drivers/platform/x86/intel/pmc/core_ssram.c   | 326 ------------------
>>   drivers/platform/x86/intel/pmc/lnl.c          |  36 +-
>>   drivers/platform/x86/intel/pmc/mtl.c          |  34 +-
>>   .../platform/x86/intel/pmc/ssram_telemetry.c  | 184 ++++++++++
>>   .../platform/x86/intel/pmc/ssram_telemetry.h  |  45 +++
>>   drivers/platform/x86/intel/pmt/telemetry.h    |  19 +-
>>   11 files changed, 550 insertions(+), 392 deletions(-)
>>   delete mode 100644 drivers/platform/x86/intel/pmc/core_ssram.c
>>   create mode 100644 drivers/platform/x86/intel/pmc/ssram_telemetry.c
>>   create mode 100644 drivers/platform/x86/intel/pmc/ssram_telemetry.h
>>
>>