On Wed, Jan 15, 2025 at 9:59 AM zhenglifeng (A) <zhenglifeng1@xxxxxxxxxx> wrote:
>
> On 2025/1/15 1:58, Rafael J. Wysocki wrote:
>
> > On Mon, Jan 13, 2025 at 1:21 PM Lifeng Zheng <zhenglifeng1@xxxxxxxxxx> 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@xxxxxxxxxx>
> >
> > 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.
