Re: [PATCH 2/4] platform/x86/intel/pmc: Dump raw SLP_Sx_DBG registers and distinguish between them

["Guilherme G. Piccoli" <gpiccoli@igalia.com>]

Hi Ilpo, thanks a lot for your review, very nice points! Comments below:


On 23/09/2025 04:59, Ilpo Järvinen wrote:
> [...]
>> This should bring no functional change, the goal is only to improve
>> reading and allow full comparison between raw register values.
> 
> Hi,
> 
> I don't think that's exactly the definition of "no function change" if you 
> intentionally make a change to the reading. :-)
>

Hehe yeah, you're right - it changes the output, so that's a functional
change indeed (imagine a userspace script parsing it...). I was thinking
in functional change as in "no extra register reads are performed", but
I agree with you and will drop this text from next version, thanks for
pointing!


>[...]
>> +
>> +		if (dev)
>> +			dev_info(dev, "\nSLP_S%u_DBG:\t0x%x\n", cnt, data);
>> +		if (s)
>> +			seq_printf(s, "\nSLP_S%u_DBG:\t0x%x\n", cnt, data);
>>  		while (map->name) {
>>  			if (dev)
>> -				dev_info(dev, "SLP_S0_DBG: %-32s\tState: %s\n",
>> -					map->name,
>> +				dev_info(dev, "SLP_S%u_DBG: %-32s\tState: %s\n",
> 
> I'm not sure about this change. To me it looks the naming is "SLP S0 DEBUG 
> REGx (SLP_S0_DBG_x)" according to this:
> 
> https://edc.intel.com/content/www/tw/zh/design/publications/14th-generation-core-processors-ioe-p-registers/slp-s0-debug-reg2-slp-s0-dbg-2-offset-10bc/
> 
> ...So changing from S0 to S1 or S2 does not seem correct here?
> 
> I wonder if this really a problem to begin with as the names should be 
> unique, no?

Totally right again! Nice catch, it should be as you pointed, the
different ID is at the end of the string.
And no, it's definitely not a problem / big deal - I just took the
opportunity to improve the output, but I messed up heh

Lemme know if you prefer that I keep the old naming or fix it properly,
like SLP_S0_DBG_2, etc.


> [...]
> This assumption seems somewhat fragile but maybe it's not worth 
> engineering it beyond this at this point.

Sorry, I couldn't understand this sentence. Can you clarify it for me?
What assumption and what do you think we should do?


>[...] 
> Also, please remember to add all maintainers as receipients when posting.

My apologies - I checked MAINTAINERS directly and added everyone from
INTEL PMC + Xi Pardee (many patches in the driver); but I should have
used get_maintainers instead, which brings your name. Thanks for
reviewing even when I forgot to add your email!!
Cheers,


Guilherme