[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <72dd19cc-5f24-4b76-ad31-86f7432c6d34@linux.intel.com>
Date: Tue, 13 Aug 2024 16:01:45 -0700
From: Xi Pardee <xi.pardee@...ux.intel.com>
To: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
Cc: irenic.rajneesh@...il.com, david.e.box@...ux.intel.com,
Hans de Goede <hdegoede@...hat.com>, platform-driver-x86@...r.kernel.org,
LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 0/7] Create Intel PMC SSRAM Telemetry driver
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
>>
>>
Powered by blists - more mailing lists