diff mbox series

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

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

Commit Message

zhenglifeng (A) Jan. 13, 2025, 12:21 p.m. UTC
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(+)

Comments

Rafael J. Wysocki Jan. 14, 2025, 5:58 p.m. UTC | #1
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
zhenglifeng (A) Jan. 15, 2025, 8:58 a.m. UTC | #2
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.
Rafael J. Wysocki Jan. 15, 2025, 11:12 a.m. UTC | #3
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.
zhenglifeng (A) Jan. 16, 2025, 1:12 a.m. UTC | #4
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 mbox series

Patch

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.