On Fri, Mar 14, 2025 at 10:25 AM zhenglifeng (A) <zhenglifeng1@xxxxxxxxxx> wrote: > > On 2025/3/13 3:54, Rafael J. Wysocki wrote: > > > On Thu, Feb 6, 2025 at 2:14 PM Lifeng Zheng <zhenglifeng1@xxxxxxxxxx> wrote: > >> > >> Rename cppc_get_perf() to cppc_get_reg_val() as a generic function to read > >> cppc registers. And extract the operations if register is in pcc out as > >> cppc_get_reg_val_in_pcc(). Without functional change. > > > > This should be split into two patches IMV. > > Yes. That makes sense. Thanks. > > > > >> Signed-off-by: Lifeng Zheng <zhenglifeng1@xxxxxxxxxx> > >> --- > >> drivers/acpi/cppc_acpi.c | 66 +++++++++++++++++++++------------------- > >> 1 file changed, 35 insertions(+), 31 deletions(-) > >> > >> diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c > >> index db22f8f107db..3c9c4ce2a0b0 100644 > >> --- a/drivers/acpi/cppc_acpi.c > >> +++ b/drivers/acpi/cppc_acpi.c > >> @@ -1189,48 +1189,52 @@ static int cpc_write(int cpu, struct cpc_register_resource *reg_res, u64 val) > >> return ret_val; > >> } > >> > >> -static int cppc_get_perf(int cpunum, enum cppc_regs reg_idx, u64 *perf) > >> +static int cppc_get_reg_val_in_pcc(int cpu, struct cpc_register_resource *reg, u64 *val) > >> { > >> - struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, cpunum); > >> - struct cpc_register_resource *reg; > >> + int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpu); > >> + struct cppc_pcc_data *pcc_ss_data = NULL; > >> + int ret; > >> > >> - if (!cpc_desc) { > >> - pr_debug("No CPC descriptor for CPU:%d\n", cpunum); > >> + if (pcc_ss_id < 0) { > >> + pr_debug("Invalid pcc_ss_id\n"); > >> return -ENODEV; > >> } > >> > >> - reg = &cpc_desc->cpc_regs[reg_idx]; > >> + pcc_ss_data = pcc_data[pcc_ss_id]; > >> > >> - if (IS_OPTIONAL_CPC_REG(reg_idx) && !CPC_SUPPORTED(reg)) { > >> - pr_debug("CPC register (reg_idx=%d) is not supported\n", reg_idx); > >> - return -EOPNOTSUPP; > >> - } > > > > I'm not a big fan of the IS_OPTIONAL_CPC_REG() macro. I'm not > > convinced at all that it adds any value above (and in the next patch > > for that matter) and the message printing the register index is just > > plain unuseful to anyone who doesn't know how to decode it. > > With this index, it is easier to locate problems. This is what a "pr_debug" > for, isn't it? For those who know how to decode it, yes. For others, not really. > > > > If CPC_SUPPORTED(reg) is not true, the register cannot be used AFAICS > > regardless of what IS_OPTIONAL_CPC_REG() has to say about it. > > The name "CPC_SUPPORTED" may be a little confused. Actually, in ACPI 6.5, > only optional _CPC package fields that are not supported by the platform > should be encoded as 0 intergers or NULL registers. A mandatory field as a > 0 interger is valid. So If I wanted to make this function as a generic one > to read cppc registers, it would have been more reasonable to do this > IS_OPTIONAL_CPC_REG() check before CPC_SUPPORTED(). I see, so you need to explain this in the changelog. And IMV the code logic should be: (1) If this is a NULL register, don't use it. (2) If it is integer 0, check if it is optional. (a) If it is optional, don't use it. (b) Otherwise, use 0 as the value. Of course, there is a problem for platforms that may want to pass 0 as an optional field value, but this is a spec issue.