Message ID | 20250113122104.3870673-4-zhenglifeng1@huawei.com (mailing list archive) |
---|---|
State | Handled Elsewhere, archived |
Headers | show |
Series | Support for autonomous selection in cppc_cpufreq | expand |
On Mon, Jan 13, 2025 at 1:21 PM Lifeng Zheng <zhenglifeng1@huawei.com> wrote: > > Add CPPC_REG_VAL_READ() to implement registers getting functions. > > Add CPPC_REG_VAL_WRITE() to implement registers setting functions. > > Signed-off-by: Lifeng Zheng <zhenglifeng1@huawei.com> I don't particularly like these macros as they will generally make it harder to follow the code. > --- > drivers/acpi/cppc_acpi.c | 14 ++++++++++++++ > 1 file changed, 14 insertions(+) > > diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c > index 571f94855dce..6326a1536cda 100644 > --- a/drivers/acpi/cppc_acpi.c > +++ b/drivers/acpi/cppc_acpi.c > @@ -1279,6 +1279,20 @@ static int cppc_set_reg_val(int cpu, enum cppc_regs reg_idx, u64 val) > return cpc_write(cpu, reg, val); > } > > +#define CPPC_REG_VAL_READ(reg_name, reg_idx) \ > +int cppc_get_##reg_name(int cpu, u64 *val) \ > +{ \ > + return cppc_get_reg_val(cpu, reg_idx, val); \ > +} \ > +EXPORT_SYMBOL_GPL(cppc_get_##reg_name) What about if defining something like #define CPPC_READ_REG_VAL(cpu, reg_name, val) cppc_get_reg_val((cpu), CPPC_REG_IDX(reg_name), (val)) (and analogously for the WRITE_ part), where CPPC_REG_IDX(reg_name) is #define CPPC_REG_IDX(reg_name) CPPC_REG_##reg_name_IDX and there are CPPC_REG_##reg_name_IDX macros defined for all register names in use? For example #define CPPC_REG_desired_perf_IDX DESIRED_PERF
On 2025/1/15 1:58, Rafael J. Wysocki wrote: > On Mon, Jan 13, 2025 at 1:21 PM Lifeng Zheng <zhenglifeng1@huawei.com> wrote: >> >> Add CPPC_REG_VAL_READ() to implement registers getting functions. >> >> Add CPPC_REG_VAL_WRITE() to implement registers setting functions. >> >> Signed-off-by: Lifeng Zheng <zhenglifeng1@huawei.com> > > I don't particularly like these macros as they will generally make it > harder to follow the code. > >> --- >> drivers/acpi/cppc_acpi.c | 14 ++++++++++++++ >> 1 file changed, 14 insertions(+) >> >> diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c >> index 571f94855dce..6326a1536cda 100644 >> --- a/drivers/acpi/cppc_acpi.c >> +++ b/drivers/acpi/cppc_acpi.c >> @@ -1279,6 +1279,20 @@ static int cppc_set_reg_val(int cpu, enum cppc_regs reg_idx, u64 val) >> return cpc_write(cpu, reg, val); >> } >> >> +#define CPPC_REG_VAL_READ(reg_name, reg_idx) \ >> +int cppc_get_##reg_name(int cpu, u64 *val) \ >> +{ \ >> + return cppc_get_reg_val(cpu, reg_idx, val); \ >> +} \ >> +EXPORT_SYMBOL_GPL(cppc_get_##reg_name) > > What about if defining something like > > #define CPPC_READ_REG_VAL(cpu, reg_name, val) > cppc_get_reg_val((cpu), CPPC_REG_IDX(reg_name), (val)) > > (and analogously for the WRITE_ part), where CPPC_REG_IDX(reg_name) is > > #define CPPC_REG_IDX(reg_name) CPPC_REG_##reg_name_IDX > > and there are CPPC_REG_##reg_name_IDX macros defined for all register > names in use? > > For example > > #define CPPC_REG_desired_perf_IDX DESIRED_PERF What about keeping these two macros but replace reg_idx with CPPC_REG_IDX(reg_name)? With this, the only needed parameter for these two macros is reg_name.
On Wed, Jan 15, 2025 at 9:59 AM zhenglifeng (A) <zhenglifeng1@huawei.com> wrote: > > On 2025/1/15 1:58, Rafael J. Wysocki wrote: > > > On Mon, Jan 13, 2025 at 1:21 PM Lifeng Zheng <zhenglifeng1@huawei.com> wrote: > >> > >> Add CPPC_REG_VAL_READ() to implement registers getting functions. > >> > >> Add CPPC_REG_VAL_WRITE() to implement registers setting functions. > >> > >> Signed-off-by: Lifeng Zheng <zhenglifeng1@huawei.com> > > > > I don't particularly like these macros as they will generally make it > > harder to follow the code. > > > >> --- > >> drivers/acpi/cppc_acpi.c | 14 ++++++++++++++ > >> 1 file changed, 14 insertions(+) > >> > >> diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c > >> index 571f94855dce..6326a1536cda 100644 > >> --- a/drivers/acpi/cppc_acpi.c > >> +++ b/drivers/acpi/cppc_acpi.c > >> @@ -1279,6 +1279,20 @@ static int cppc_set_reg_val(int cpu, enum cppc_regs reg_idx, u64 val) > >> return cpc_write(cpu, reg, val); > >> } > >> > >> +#define CPPC_REG_VAL_READ(reg_name, reg_idx) \ > >> +int cppc_get_##reg_name(int cpu, u64 *val) \ > >> +{ \ > >> + return cppc_get_reg_val(cpu, reg_idx, val); \ > >> +} \ > >> +EXPORT_SYMBOL_GPL(cppc_get_##reg_name) > > > > What about if defining something like > > > > #define CPPC_READ_REG_VAL(cpu, reg_name, val) > > cppc_get_reg_val((cpu), CPPC_REG_IDX(reg_name), (val)) > > > > (and analogously for the WRITE_ part), where CPPC_REG_IDX(reg_name) is > > > > #define CPPC_REG_IDX(reg_name) CPPC_REG_##reg_name_IDX > > > > and there are CPPC_REG_##reg_name_IDX macros defined for all register > > names in use? > > > > For example > > > > #define CPPC_REG_desired_perf_IDX DESIRED_PERF > > What about keeping these two macros but replace reg_idx with > CPPC_REG_IDX(reg_name)? With this, the only needed parameter for these two > macros is reg_name. The problem is that looking up functions defined through macros is hard when somebody wants to know what they do, so I'd prefer to avoid doing that.
On 2025/1/15 19:12, Rafael J. Wysocki wrote: > On Wed, Jan 15, 2025 at 9:59 AM zhenglifeng (A) <zhenglifeng1@huawei.com> wrote: >> >> On 2025/1/15 1:58, Rafael J. Wysocki wrote: >> >>> On Mon, Jan 13, 2025 at 1:21 PM Lifeng Zheng <zhenglifeng1@huawei.com> wrote: >>>> >>>> Add CPPC_REG_VAL_READ() to implement registers getting functions. >>>> >>>> Add CPPC_REG_VAL_WRITE() to implement registers setting functions. >>>> >>>> Signed-off-by: Lifeng Zheng <zhenglifeng1@huawei.com> >>> >>> I don't particularly like these macros as they will generally make it >>> harder to follow the code. >>> >>>> --- >>>> drivers/acpi/cppc_acpi.c | 14 ++++++++++++++ >>>> 1 file changed, 14 insertions(+) >>>> >>>> diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c >>>> index 571f94855dce..6326a1536cda 100644 >>>> --- a/drivers/acpi/cppc_acpi.c >>>> +++ b/drivers/acpi/cppc_acpi.c >>>> @@ -1279,6 +1279,20 @@ static int cppc_set_reg_val(int cpu, enum cppc_regs reg_idx, u64 val) >>>> return cpc_write(cpu, reg, val); >>>> } >>>> >>>> +#define CPPC_REG_VAL_READ(reg_name, reg_idx) \ >>>> +int cppc_get_##reg_name(int cpu, u64 *val) \ >>>> +{ \ >>>> + return cppc_get_reg_val(cpu, reg_idx, val); \ >>>> +} \ >>>> +EXPORT_SYMBOL_GPL(cppc_get_##reg_name) >>> >>> What about if defining something like >>> >>> #define CPPC_READ_REG_VAL(cpu, reg_name, val) >>> cppc_get_reg_val((cpu), CPPC_REG_IDX(reg_name), (val)) >>> >>> (and analogously for the WRITE_ part), where CPPC_REG_IDX(reg_name) is >>> >>> #define CPPC_REG_IDX(reg_name) CPPC_REG_##reg_name_IDX >>> >>> and there are CPPC_REG_##reg_name_IDX macros defined for all register >>> names in use? >>> >>> For example >>> >>> #define CPPC_REG_desired_perf_IDX DESIRED_PERF >> >> What about keeping these two macros but replace reg_idx with >> CPPC_REG_IDX(reg_name)? With this, the only needed parameter for these two >> macros is reg_name. > > The problem is that looking up functions defined through macros is > hard when somebody wants to know what they do, so I'd prefer to avoid > doing that. I see your point. Let's just remove these. > >
diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c index 571f94855dce..6326a1536cda 100644 --- a/drivers/acpi/cppc_acpi.c +++ b/drivers/acpi/cppc_acpi.c @@ -1279,6 +1279,20 @@ static int cppc_set_reg_val(int cpu, enum cppc_regs reg_idx, u64 val) return cpc_write(cpu, reg, val); } +#define CPPC_REG_VAL_READ(reg_name, reg_idx) \ +int cppc_get_##reg_name(int cpu, u64 *val) \ +{ \ + return cppc_get_reg_val(cpu, reg_idx, val); \ +} \ +EXPORT_SYMBOL_GPL(cppc_get_##reg_name) + +#define CPPC_REG_VAL_WRITE(reg_name, reg_idx) \ +int cppc_set_##reg_name(int cpu, u64 val) \ +{ \ + return cppc_set_reg_val(cpu, reg_idx, val); \ +} \ +EXPORT_SYMBOL_GPL(cppc_set_##reg_name) + /** * cppc_get_desired_perf - Get the desired performance register value. * @cpunum: CPU from which to get desired performance.
Add CPPC_REG_VAL_READ() to implement registers getting functions. Add CPPC_REG_VAL_WRITE() to implement registers setting functions. Signed-off-by: Lifeng Zheng <zhenglifeng1@huawei.com> --- drivers/acpi/cppc_acpi.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+)