Re: [PATCH v2 3/5] platform/x86:intel/pmc: Improve function to show substate header

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

Hi Ilpo,

Thanks for reviewing the patch. My response is inline.

On 6/30/2025 4:34 AM, Ilpo Järvinen wrote:
> On Tue, 24 Jun 2025, Xi Pardee wrote:
>
>> Refactor pmc_core_substate_req_header_show() to accept a new argument.
>> This is a preparation patch to introduce a new way to show Low Power
>> Mode substate requirement data for platforms starting from Panther
>> Lake. Increased the size for the name column as the Low Power Mode
>> requirement register name is longer in newer platforms.
>>
>> Signed-off-by: Xi Pardee <xi.pardee@linux.intel.com>
>> ---
>>   drivers/platform/x86/intel/pmc/core.c | 15 +++++++++------
>>   1 file changed, 9 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/platform/x86/intel/pmc/core.c b/drivers/platform/x86/intel/pmc/core.c
>> index a1dd80bdbd413..47cc5120e7dd6 100644
>> --- a/drivers/platform/x86/intel/pmc/core.c
>> +++ b/drivers/platform/x86/intel/pmc/core.c
>> @@ -828,17 +828,20 @@ static int pmc_core_substate_l_sts_regs_show(struct seq_file *s, void *unused)
>>   }
>>   DEFINE_SHOW_ATTRIBUTE(pmc_core_substate_l_sts_regs);
>>   
>> -static void pmc_core_substate_req_header_show(struct seq_file *s, int pmc_index)
>> +static void pmc_core_substate_req_header_show(struct seq_file *s, int pmc_index, char *name)
>>   {
>>   	struct pmc_dev *pmcdev = s->private;
>>   	int mode;
>>   
>> -	seq_printf(s, "%30s |", "Element");
>> +	seq_printf(s, "%40s |", "Element");
>>   	pmc_for_each_mode(mode, pmcdev)
>>   		seq_printf(s, " %9s |", pmc_lpm_modes[mode]);
>>   
>> -	seq_printf(s, " %9s |", "Status");
>> -	seq_printf(s, " %11s |\n", "Live Status");
>> +	if (!strcmp(name, "Status")) {
> I'm not very happy passing this as string. Could we use an
> internal defines/enum instead for this differentiation?
Will use enum in next version.
>
>> +		seq_printf(s, " %9s |", name);
>> +		seq_printf(s, " %11s |\n", "Live Status");
>> +	} else
>> +		seq_printf(s, " %9s |\n", name);
> Please always use braces symmetrically in both blocks.

Will use braces in next version.

Thanks!

Xi

>
>>   }
>>   
>>   static int pmc_core_substate_req_regs_show(struct seq_file *s, void *unused)
>> @@ -872,7 +875,7 @@ static int pmc_core_substate_req_regs_show(struct seq_file *s, void *unused)
>>   			continue;
>>   
>>   		/* Display the header */
>> -		pmc_core_substate_req_header_show(s, pmc_index);
>> +		pmc_core_substate_req_header_show(s, pmc_index, "Status");
>>   
>>   		/* Loop over maps */
>>   		for (mp = 0; mp < num_maps; mp++) {
>> @@ -910,7 +913,7 @@ static int pmc_core_substate_req_regs_show(struct seq_file *s, void *unused)
>>   				}
>>   
>>   				/* Display the element name in the first column */
>> -				seq_printf(s, "pmc%d: %26s |", pmc_index, map[i].name);
>> +				seq_printf(s, "pmc%d: %34s |", pmc_index, map[i].name);
>>   
>>   				/* Loop over the enabled states and display if required */
>>   				pmc_for_each_mode(mode, pmcdev) {
>>