| 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 |
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), ®val); > 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 { > >
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), ®val); >> 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 { >> >> >
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
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 --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), ®val); 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 {
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(-)