diff mbox series

[1/3] hwmon: (coretemp) Introduce enum for attr index

Message ID	20231127131651.476795-2-rui.zhang@intel.com
State	New
Headers	Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.32 as permitted sender) client-ip=23.128.96.32; From: Zhang Rui <rui.zhang@intel.com> To: linux@roeck-us.net, jdelvare@suse.com Cc: fenghua.yu@intel.com, linux-hwmon@vger.kernel.org, linux-kernel@vger.kernel.org Subject: [PATCH 1/3] hwmon: (coretemp) Introduce enum for attr index Date: Mon, 27 Nov 2023 21:16:49 +0800 Message-Id: <20231127131651.476795-2-rui.zhang@intel.com> In-Reply-To: <20231127131651.476795-1-rui.zhang@intel.com> References: <20231127131651.476795-1-rui.zhang@intel.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Precedence: bulk
Series	hwmon: (coretemp) Fix core count limitation \| [0/3] hwmon: (coretemp) Fix core count limitation [1/3] hwmon: (coretemp) Introduce enum for attr index [2/3] hwmon: (coretemp) Remove unnecessary dependency of array index [3/3] hwmon: (coretemp) Fix core count limitation

Commit Message

Zhang, Rui Nov. 27, 2023, 1:16 p.m. UTC

  Introduce enum coretemp_attr_index to better describe the index of each
sensor attribute and the maximum number of basic/possible attributes.

No functional change.

Signed-off-by: Zhang Rui <rui.zhang@intel.com>
---
 drivers/hwmon/coretemp.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

Comments

Ashok Raj Nov. 30, 2023, 9:51 p.m. UTC | #1

On Mon, Nov 27, 2023 at 09:16:49PM +0800, Zhang Rui wrote:
> Introduce enum coretemp_attr_index to better describe the index of each
> sensor attribute and the maximum number of basic/possible attributes.
> 
> No functional change.
> 
> Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> ---
>  drivers/hwmon/coretemp.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/hwmon/coretemp.c b/drivers/hwmon/coretemp.c
> index ba82d1e79c13..6053ed3761c2 100644
> --- a/drivers/hwmon/coretemp.c
> +++ b/drivers/hwmon/coretemp.c
> @@ -43,10 +43,18 @@ MODULE_PARM_DESC(tjmax, "TjMax value in degrees Celsius");
>  #define BASE_SYSFS_ATTR_NO	2	/* Sysfs Base attr no for coretemp */
>  #define NUM_REAL_CORES		128	/* Number of Real cores per cpu */
>  #define CORETEMP_NAME_LENGTH	28	/* String Length of attrs */
> -#define MAX_CORE_ATTRS		4	/* Maximum no of basic attrs */
> -#define TOTAL_ATTRS		(MAX_CORE_ATTRS + 1)
>  #define MAX_CORE_DATA		(NUM_REAL_CORES + BASE_SYSFS_ATTR_NO)
>  
> +enum coretemp_attr_index {
> +	ATTR_LABEL,
> +	ATTR_CRIT_ALARM,
> +	ATTR_TEMP,
> +	ATTR_TJMAX,
> +	ATTR_TTARGET,
> +	TOTAL_ATTRS,			/* Maximum no of possible attrs */
> +	MAX_CORE_ATTRS = ATTR_TJMAX + 1	/* Maximum no of basic attrs */

This seems odd. TOTAL_ATTRS being the last entry seems fine, but defining a
MAX_CORE_ATTR the way above sounds a bit hacky.

> +};
> +
>  #ifdef CONFIG_SMP
>  #define for_each_sibling(i, cpu) \
>  	for_each_cpu(i, topology_sibling_cpumask(cpu))
> -- 
> 2.34.1
>

Guenter Roeck Dec. 1, 2023, 4:14 a.m. UTC | #2

On 11/30/23 13:51, Ashok Raj wrote:
> On Mon, Nov 27, 2023 at 09:16:49PM +0800, Zhang Rui wrote:
>> Introduce enum coretemp_attr_index to better describe the index of each
>> sensor attribute and the maximum number of basic/possible attributes.
>>
>> No functional change.
>>
>> Signed-off-by: Zhang Rui <rui.zhang@intel.com>
>> ---
>>   drivers/hwmon/coretemp.c | 12 ++++++++++--
>>   1 file changed, 10 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/hwmon/coretemp.c b/drivers/hwmon/coretemp.c
>> index ba82d1e79c13..6053ed3761c2 100644
>> --- a/drivers/hwmon/coretemp.c
>> +++ b/drivers/hwmon/coretemp.c
>> @@ -43,10 +43,18 @@ MODULE_PARM_DESC(tjmax, "TjMax value in degrees Celsius");
>>   #define BASE_SYSFS_ATTR_NO	2	/* Sysfs Base attr no for coretemp */
>>   #define NUM_REAL_CORES		128	/* Number of Real cores per cpu */
>>   #define CORETEMP_NAME_LENGTH	28	/* String Length of attrs */
>> -#define MAX_CORE_ATTRS		4	/* Maximum no of basic attrs */
>> -#define TOTAL_ATTRS		(MAX_CORE_ATTRS + 1)
>>   #define MAX_CORE_DATA		(NUM_REAL_CORES + BASE_SYSFS_ATTR_NO)
>>   
>> +enum coretemp_attr_index {
>> +	ATTR_LABEL,
>> +	ATTR_CRIT_ALARM,
>> +	ATTR_TEMP,
>> +	ATTR_TJMAX,
>> +	ATTR_TTARGET,
>> +	TOTAL_ATTRS,			/* Maximum no of possible attrs */
>> +	MAX_CORE_ATTRS = ATTR_TJMAX + 1	/* Maximum no of basic attrs */
> 
> This seems odd. TOTAL_ATTRS being the last entry seems fine, but defining a
> MAX_CORE_ATTR the way above sounds a bit hacky.
> 

Complaining is easy. What do you suggest that would be better ?

Guenter

>> +};
>> +
>>   #ifdef CONFIG_SMP
>>   #define for_each_sibling(i, cpu) \
>>   	for_each_cpu(i, topology_sibling_cpumask(cpu))
>> -- 
>> 2.34.1
>>
>

Ashok Raj Dec. 1, 2023, 4:47 a.m. UTC | #3

On Thu, Nov 30, 2023 at 08:14:48PM -0800, Guenter Roeck wrote:
> On 11/30/23 13:51, Ashok Raj wrote:
> > On Mon, Nov 27, 2023 at 09:16:49PM +0800, Zhang Rui wrote:
> > > Introduce enum coretemp_attr_index to better describe the index of each
> > > sensor attribute and the maximum number of basic/possible attributes.
> > > 
> > > No functional change.
> > > 
> > > Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> > > ---
> > >   drivers/hwmon/coretemp.c | 12 ++++++++++--
> > >   1 file changed, 10 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/hwmon/coretemp.c b/drivers/hwmon/coretemp.c
> > > index ba82d1e79c13..6053ed3761c2 100644
> > > --- a/drivers/hwmon/coretemp.c
> > > +++ b/drivers/hwmon/coretemp.c
> > > @@ -43,10 +43,18 @@ MODULE_PARM_DESC(tjmax, "TjMax value in degrees Celsius");
> > >   #define BASE_SYSFS_ATTR_NO	2	/* Sysfs Base attr no for coretemp */
> > >   #define NUM_REAL_CORES		128	/* Number of Real cores per cpu */
> > >   #define CORETEMP_NAME_LENGTH	28	/* String Length of attrs */
> > > -#define MAX_CORE_ATTRS		4	/* Maximum no of basic attrs */
> > > -#define TOTAL_ATTRS		(MAX_CORE_ATTRS + 1)
> > >   #define MAX_CORE_DATA		(NUM_REAL_CORES + BASE_SYSFS_ATTR_NO)
> > > +enum coretemp_attr_index {
> > > +	ATTR_LABEL,
> > > +	ATTR_CRIT_ALARM,
> > > +	ATTR_TEMP,
> > > +	ATTR_TJMAX,
> > > +	ATTR_TTARGET,
> > > +	TOTAL_ATTRS,			/* Maximum no of possible attrs */
> > > +	MAX_CORE_ATTRS = ATTR_TJMAX + 1	/* Maximum no of basic attrs */
> > 
> > This seems odd. TOTAL_ATTRS being the last entry seems fine, but defining a
> > MAX_CORE_ATTR the way above sounds a bit hacky.
> > 
> 
> Complaining is easy. What do you suggest that would be better ?
> 
Fair enough.

How about 

ATTR_LABEL,
ATTR_CRIT_ALARM,
ATTR_TEMP,
ATTR_TJMAX,
MAX_CORE_ATTRS,		/* One more than TJMAX */
ATTR_TTARGET = MAX_CORE_ATTRS,
TOTAL_ATTRS

Each enum can be assigned any value, but this way they are just increasing
order.

Zhang, Rui Dec. 1, 2023, 5:29 p.m. UTC | #4

On Thu, 2023-11-30 at 20:47 -0800, Ashok Raj wrote:
> On Thu, Nov 30, 2023 at 08:14:48PM -0800, Guenter Roeck wrote:
> > On 11/30/23 13:51, Ashok Raj wrote:
> > > On Mon, Nov 27, 2023 at 09:16:49PM +0800, Zhang Rui wrote:
> > > > Introduce enum coretemp_attr_index to better describe the index
> > > > of each
> > > > sensor attribute and the maximum number of basic/possible
> > > > attributes.
> > > > 
> > > > No functional change.
> > > > 
> > > > Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> > > > ---
> > > >   drivers/hwmon/coretemp.c | 12 ++++++++++--
> > > >   1 file changed, 10 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/hwmon/coretemp.c
> > > > b/drivers/hwmon/coretemp.c
> > > > index ba82d1e79c13..6053ed3761c2 100644
> > > > --- a/drivers/hwmon/coretemp.c
> > > > +++ b/drivers/hwmon/coretemp.c
> > > > @@ -43,10 +43,18 @@ MODULE_PARM_DESC(tjmax, "TjMax value in
> > > > degrees Celsius");
> > > >   #define BASE_SYSFS_ATTR_NO    2       /* Sysfs Base attr no
> > > > for coretemp */
> > > >   #define NUM_REAL_CORES                128     /* Number of
> > > > Real cores per cpu */
> > > >   #define CORETEMP_NAME_LENGTH  28      /* String Length of
> > > > attrs */
> > > > -#define MAX_CORE_ATTRS         4       /* Maximum no of basic
> > > > attrs */
> > > > -#define TOTAL_ATTRS            (MAX_CORE_ATTRS + 1)
> > > >   #define MAX_CORE_DATA         (NUM_REAL_CORES +
> > > > BASE_SYSFS_ATTR_NO)
> > > > +enum coretemp_attr_index {
> > > > +       ATTR_LABEL,
> > > > +       ATTR_CRIT_ALARM,
> > > > +       ATTR_TEMP,
> > > > +       ATTR_TJMAX,
> > > > +       ATTR_TTARGET,
> > > > +       TOTAL_ATTRS,                    /* Maximum no of
> > > > possible attrs */
> > > > +       MAX_CORE_ATTRS = ATTR_TJMAX + 1 /* Maximum no of basic
> > > > attrs */
> > > 
> > > This seems odd. TOTAL_ATTRS being the last entry seems fine, but
> > > defining a
> > > MAX_CORE_ATTR the way above sounds a bit hacky.
> > > 
> > 
> > Complaining is easy. What do you suggest that would be better ?
> > 
> Fair enough.
> 
> How about 
> 
> ATTR_LABEL,
> ATTR_CRIT_ALARM,
> ATTR_TEMP,
> ATTR_TJMAX,
> MAX_CORE_ATTRS,         /* One more than TJMAX */
> ATTR_TTARGET = MAX_CORE_ATTRS,
> TOTAL_ATTRS
> 
> Each enum can be assigned any value, but this way they are just
> increasing
> order. 

ATTR_TTARGET is the next attribute after ATTR_TJMAX so it should be
right after ATTR_TJMAX.
MAX_CORE_ATTRS is the number of basic attributes so it should be
ATTR_TJMAX + 1.
TOTAL_ATTRS is the number of possible attributes so it should be
ATTR_TTARGET + 1

ATTR_LABEL,				// 0
ATTR_CRIT_ALARM,			// 1
ATTR_TEMP,				// 2
ATTR_TJMAX,				// 3
ATTR_TTARGET,				// 4
MAX_CORE_ATTRS = ATTR_TJMAX + 1,	// 4
TOTAL_ATTRS = ATTR_TTARGET + 1,		// 5

How about this one?

thanks,
rui

Ashok Raj Dec. 5, 2023, 7:20 p.m. UTC | #5

On Fri, Dec 01, 2023 at 09:29:24AM -0800, Zhang, Rui wrote:

[snip]

> > 
> > How about 
> > 
> > ATTR_LABEL,
> > ATTR_CRIT_ALARM,
> > ATTR_TEMP,
> > ATTR_TJMAX,
> > MAX_CORE_ATTRS,         /* One more than TJMAX */
> > ATTR_TTARGET = MAX_CORE_ATTRS,
> > TOTAL_ATTRS
> > 
> > Each enum can be assigned any value, but this way they are just
> > increasing
> > order. 
> 
> ATTR_TTARGET is the next attribute after ATTR_TJMAX so it should be
> right after ATTR_TJMAX.
> MAX_CORE_ATTRS is the number of basic attributes so it should be
> ATTR_TJMAX + 1.
> TOTAL_ATTRS is the number of possible attributes so it should be
> ATTR_TTARGET + 1
> 
> ATTR_LABEL,				// 0
> ATTR_CRIT_ALARM,			// 1
> ATTR_TEMP,				// 2
> ATTR_TJMAX,				// 3
> ATTR_TTARGET,				// 4
> MAX_CORE_ATTRS = ATTR_TJMAX + 1,	// 4
> TOTAL_ATTRS = ATTR_TTARGET + 1,		// 5
> 
> How about this one?

Sorry for the delay... yes, this sounds fine.

diff mbox series

Patch

diff --git a/drivers/hwmon/coretemp.c b/drivers/hwmon/coretemp.c
index ba82d1e79c13..6053ed3761c2 100644
--- a/drivers/hwmon/coretemp.c
+++ b/drivers/hwmon/coretemp.c
@@ -43,10 +43,18 @@  MODULE_PARM_DESC(tjmax, "TjMax value in degrees Celsius");
 #define BASE_SYSFS_ATTR_NO	2	/* Sysfs Base attr no for coretemp */
 #define NUM_REAL_CORES		128	/* Number of Real cores per cpu */
 #define CORETEMP_NAME_LENGTH	28	/* String Length of attrs */
-#define MAX_CORE_ATTRS		4	/* Maximum no of basic attrs */
-#define TOTAL_ATTRS		(MAX_CORE_ATTRS + 1)
 #define MAX_CORE_DATA		(NUM_REAL_CORES + BASE_SYSFS_ATTR_NO)
 
+enum coretemp_attr_index {
+	ATTR_LABEL,
+	ATTR_CRIT_ALARM,
+	ATTR_TEMP,
+	ATTR_TJMAX,
+	ATTR_TTARGET,
+	TOTAL_ATTRS,			/* Maximum no of possible attrs */
+	MAX_CORE_ATTRS = ATTR_TJMAX + 1	/* Maximum no of basic attrs */
+};
+
 #ifdef CONFIG_SMP
 #define for_each_sibling(i, cpu) \
 	for_each_cpu(i, topology_sibling_cpumask(cpu))