Re: [PATCH v4 3/6] ACPI: CPPC: Add macros to generally implement registers getting and setting functions

[Rafael J. Wysocki]

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