diff mbox series

[1/2] hwmon: (k10temp) Move the CCD limit info inside k10temp_data structure

Message ID 163770216907.777059.6947726637265961161.stgit@bmoger-ubuntu (mailing list archive)
State Changes Requested
Headers show
Series [1/2] hwmon: (k10temp) Move the CCD limit info inside k10temp_data structure | expand

Commit Message

Moger, Babu Nov. 23, 2021, 9:16 p.m. UTC
It seems appropriate to move the CCD specific information inside the
k10temp_data structure.

Signed-off-by: Babu Moger <babu.moger@amd.com>
---
Note: Generated the patch on top of hwmon-next.

 drivers/hwmon/k10temp.c |   17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

Comments

Guenter Roeck Nov. 23, 2021, 9:40 p.m. UTC | #1
On 11/23/21 1:16 PM, Babu Moger wrote:
> It seems appropriate to move the CCD specific information inside the
> k10temp_data structure.
> 

Why ? I don't see it used outside k10temp_get_ccd_support().

Guenter

> Signed-off-by: Babu Moger <babu.moger@amd.com>
> ---
> Note: Generated the patch on top of hwmon-next.
> 
>   drivers/hwmon/k10temp.c |   17 +++++++++++------
>   1 file changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/hwmon/k10temp.c b/drivers/hwmon/k10temp.c
> index 880990fa4795..bd436b380a02 100644
> --- a/drivers/hwmon/k10temp.c
> +++ b/drivers/hwmon/k10temp.c
> @@ -85,6 +85,7 @@ struct k10temp_data {
>   	u32 show_temp;
>   	bool is_zen;
>   	u32 ccd_offset;
> +	u32 ccd_limit;
>   };
>   
>   #define TCTL_BIT	0
> @@ -357,12 +358,12 @@ static const struct hwmon_chip_info k10temp_chip_info = {
>   };
>   
>   static void k10temp_get_ccd_support(struct pci_dev *pdev,
> -				    struct k10temp_data *data, int limit)
> +				    struct k10temp_data *data)
>   {
>   	u32 regval;
>   	int i;
>   
> -	for (i = 0; i < limit; i++) {
> +	for (i = 0; i < data->ccd_limit; i++) {
>   		amd_smn_read(amd_pci_dev_to_node_id(pdev),
>   			     ZEN_CCD_TEMP(data->ccd_offset, i), &regval);
>   		if (regval & ZEN_CCD_TEMP_VALID)
> @@ -411,14 +412,16 @@ static int k10temp_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>   		case 0x11:	/* Zen APU */
>   		case 0x18:	/* Zen+ APU */
>   			data->ccd_offset = 0x154;
> -			k10temp_get_ccd_support(pdev, data, 4);
> +			data->ccd_limit = 4;
> +			k10temp_get_ccd_support(pdev, data);
>   			break;
>   		case 0x31:	/* Zen2 Threadripper */
>   		case 0x60:	/* Renoir */
>   		case 0x68:	/* Lucienne */
>   		case 0x71:	/* Zen2 */
>   			data->ccd_offset = 0x154;
> -			k10temp_get_ccd_support(pdev, data, 8);
> +			data->ccd_limit = 8;
> +			k10temp_get_ccd_support(pdev, data);
>   			break;
>   		}
>   	} else if (boot_cpu_data.x86 == 0x19) {
> @@ -431,13 +434,15 @@ static int k10temp_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>   		case 0x21:		/* Zen3 Ryzen Desktop */
>   		case 0x50 ... 0x5f:	/* Green Sardine */
>   			data->ccd_offset = 0x154;
> -			k10temp_get_ccd_support(pdev, data, 8);
> +			data->ccd_limit = 8;
> +			k10temp_get_ccd_support(pdev, data);
>   			break;
>   		case 0x10 ... 0x1f:
>   		case 0x40 ... 0x4f:	/* Yellow Carp */
>   		case 0xa0 ... 0xaf:
>   			data->ccd_offset = 0x300;
> -			k10temp_get_ccd_support(pdev, data, 8);
> +			data->ccd_limit = 8;
> +			k10temp_get_ccd_support(pdev, data);
>   			break;
>   		}
>   	} else {
> 
>
Moger, Babu Nov. 23, 2021, 9:52 p.m. UTC | #2
On 11/23/2021 3:40 PM, Guenter Roeck wrote:
> On 11/23/21 1:16 PM, Babu Moger wrote:
>> It seems appropriate to move the CCD specific information inside the
>> k10temp_data structure.
>>
> 
> Why ? I don't see it used outside k10temp_get_ccd_support().

Thought it will be cleaner to have it all together at one structure. If
you feel otherwise I can remove it.
Thanks
Babu

> 
> Guenter
> 
>> Signed-off-by: Babu Moger <babu.moger@amd.com>
>> ---
>> Note: Generated the patch on top of hwmon-next.
>>
>>   drivers/hwmon/k10temp.c |   17 +++++++++++------
>>   1 file changed, 11 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/hwmon/k10temp.c b/drivers/hwmon/k10temp.c
>> index 880990fa4795..bd436b380a02 100644
>> --- a/drivers/hwmon/k10temp.c
>> +++ b/drivers/hwmon/k10temp.c
>> @@ -85,6 +85,7 @@ struct k10temp_data {
>>       u32 show_temp;
>>       bool is_zen;
>>       u32 ccd_offset;
>> +    u32 ccd_limit;
>>   };
>>   #define TCTL_BIT    0
>> @@ -357,12 +358,12 @@ static const struct hwmon_chip_info 
>> k10temp_chip_info = {
>>   };
>>   static void k10temp_get_ccd_support(struct pci_dev *pdev,
>> -                    struct k10temp_data *data, int limit)
>> +                    struct k10temp_data *data)
>>   {
>>       u32 regval;
>>       int i;
>> -    for (i = 0; i < limit; i++) {
>> +    for (i = 0; i < data->ccd_limit; i++) {
>>           amd_smn_read(amd_pci_dev_to_node_id(pdev),
>>                    ZEN_CCD_TEMP(data->ccd_offset, i), &regval);
>>           if (regval & ZEN_CCD_TEMP_VALID)
>> @@ -411,14 +412,16 @@ static int k10temp_probe(struct pci_dev *pdev, 
>> const struct pci_device_id *id)
>>           case 0x11:    /* Zen APU */
>>           case 0x18:    /* Zen+ APU */
>>               data->ccd_offset = 0x154;
>> -            k10temp_get_ccd_support(pdev, data, 4);
>> +            data->ccd_limit = 4;
>> +            k10temp_get_ccd_support(pdev, data);
>>               break;
>>           case 0x31:    /* Zen2 Threadripper */
>>           case 0x60:    /* Renoir */
>>           case 0x68:    /* Lucienne */
>>           case 0x71:    /* Zen2 */
>>               data->ccd_offset = 0x154;
>> -            k10temp_get_ccd_support(pdev, data, 8);
>> +            data->ccd_limit = 8;
>> +            k10temp_get_ccd_support(pdev, data);
>>               break;
>>           }
>>       } else if (boot_cpu_data.x86 == 0x19) {
>> @@ -431,13 +434,15 @@ static int k10temp_probe(struct pci_dev *pdev, 
>> const struct pci_device_id *id)
>>           case 0x21:        /* Zen3 Ryzen Desktop */
>>           case 0x50 ... 0x5f:    /* Green Sardine */
>>               data->ccd_offset = 0x154;
>> -            k10temp_get_ccd_support(pdev, data, 8);
>> +            data->ccd_limit = 8;
>> +            k10temp_get_ccd_support(pdev, data);
>>               break;
>>           case 0x10 ... 0x1f:
>>           case 0x40 ... 0x4f:    /* Yellow Carp */
>>           case 0xa0 ... 0xaf:
>>               data->ccd_offset = 0x300;
>> -            k10temp_get_ccd_support(pdev, data, 8);
>> +            data->ccd_limit = 8;
>> +            k10temp_get_ccd_support(pdev, data);
>>               break;
>>           }
>>       } else {
>>
>>
>
Guenter Roeck Nov. 23, 2021, 10:07 p.m. UTC | #3
On 11/23/21 1:52 PM, Moger, Babu wrote:
> 
> 
> On 11/23/2021 3:40 PM, Guenter Roeck wrote:
>> On 11/23/21 1:16 PM, Babu Moger wrote:
>>> It seems appropriate to move the CCD specific information inside the
>>> k10temp_data structure.
>>>
>>
>> Why ? I don't see it used outside k10temp_get_ccd_support().
> 
> Thought it will be cleaner to have it all together at one structure. If
> you feel otherwise I can remove it.

I don't see the point of having a value in a data structure that isn't
used anywhere outside probing but nevertheless lives forever.

I also don't see how it is "cleaner" to assign a value to a variable
in a data structure only to dereference it in a single called function,
when it can be just as easily be passed as argument to that function.
I would call that just the opposite, since it creates the impression
that the variable is needed outside probing when that is not the case.

Guenter
Moger, Babu Nov. 23, 2021, 10:15 p.m. UTC | #4
On 11/23/21 4:07 PM, Guenter Roeck wrote:
> On 11/23/21 1:52 PM, Moger, Babu wrote:
>>
>>
>> On 11/23/2021 3:40 PM, Guenter Roeck wrote:
>>> On 11/23/21 1:16 PM, Babu Moger wrote:
>>>> It seems appropriate to move the CCD specific information inside the
>>>> k10temp_data structure.
>>>>
>>>
>>> Why ? I don't see it used outside k10temp_get_ccd_support().
>>
>> Thought it will be cleaner to have it all together at one structure. If
>> you feel otherwise I can remove it.
>
> I don't see the point of having a value in a data structure that isn't
> used anywhere outside probing but nevertheless lives forever.
>
> I also don't see how it is "cleaner" to assign a value to a variable
> in a data structure only to dereference it in a single called function,
> when it can be just as easily be passed as argument to that function.
> I would call that just the opposite, since it creates the impression
> that the variable is needed outside probing when that is not the case.
>  
Ok. Sure. I will remove this change and re-submitt.
diff mbox series

Patch

diff --git a/drivers/hwmon/k10temp.c b/drivers/hwmon/k10temp.c
index 880990fa4795..bd436b380a02 100644
--- a/drivers/hwmon/k10temp.c
+++ b/drivers/hwmon/k10temp.c
@@ -85,6 +85,7 @@  struct k10temp_data {
 	u32 show_temp;
 	bool is_zen;
 	u32 ccd_offset;
+	u32 ccd_limit;
 };
 
 #define TCTL_BIT	0
@@ -357,12 +358,12 @@  static const struct hwmon_chip_info k10temp_chip_info = {
 };
 
 static void k10temp_get_ccd_support(struct pci_dev *pdev,
-				    struct k10temp_data *data, int limit)
+				    struct k10temp_data *data)
 {
 	u32 regval;
 	int i;
 
-	for (i = 0; i < limit; i++) {
+	for (i = 0; i < data->ccd_limit; i++) {
 		amd_smn_read(amd_pci_dev_to_node_id(pdev),
 			     ZEN_CCD_TEMP(data->ccd_offset, i), &regval);
 		if (regval & ZEN_CCD_TEMP_VALID)
@@ -411,14 +412,16 @@  static int k10temp_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 		case 0x11:	/* Zen APU */
 		case 0x18:	/* Zen+ APU */
 			data->ccd_offset = 0x154;
-			k10temp_get_ccd_support(pdev, data, 4);
+			data->ccd_limit = 4;
+			k10temp_get_ccd_support(pdev, data);
 			break;
 		case 0x31:	/* Zen2 Threadripper */
 		case 0x60:	/* Renoir */
 		case 0x68:	/* Lucienne */
 		case 0x71:	/* Zen2 */
 			data->ccd_offset = 0x154;
-			k10temp_get_ccd_support(pdev, data, 8);
+			data->ccd_limit = 8;
+			k10temp_get_ccd_support(pdev, data);
 			break;
 		}
 	} else if (boot_cpu_data.x86 == 0x19) {
@@ -431,13 +434,15 @@  static int k10temp_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 		case 0x21:		/* Zen3 Ryzen Desktop */
 		case 0x50 ... 0x5f:	/* Green Sardine */
 			data->ccd_offset = 0x154;
-			k10temp_get_ccd_support(pdev, data, 8);
+			data->ccd_limit = 8;
+			k10temp_get_ccd_support(pdev, data);
 			break;
 		case 0x10 ... 0x1f:
 		case 0x40 ... 0x4f:	/* Yellow Carp */
 		case 0xa0 ... 0xaf:
 			data->ccd_offset = 0x300;
-			k10temp_get_ccd_support(pdev, data, 8);
+			data->ccd_limit = 8;
+			k10temp_get_ccd_support(pdev, data);
 			break;
 		}
 	} else {