Re: [PATCH v3] hwmon: (k10temp) Enable AMD3255 Proc to show negative temperature

["Limonciello, Mario" <mario.limonciello@amd.com>]

On 6/19/2023 12:07 PM, Guenter Roeck wrote:
> On 6/19/23 09:54, Baskaran Kannan wrote:
>> Industrial processor i3255 supports temperatures -40 deg celcius
>> to 105 deg Celcius. The current implementation of k10temp_read_temp
>> rounds off any negative
>> temperatures to '0'. To fix this, the following changes have been made.
>> Added a flag 'disp_negative' to struct k10temp_data to support
>> AMD i3255 processors. Flag 'disp_negative' is set if 3255 processor
>> is found during k10temp_probe.  Flag 'disp_negative' is used to 
>> determine
>> whether to round off negative temperatures to '0' in k10temp_read_temp.
>>
>> Signed-off-by: Baskaran Kannan <Baski.Kannan@amd.com>
>
> Now you have made changes you were not asked to make, extended the flag
> to cover a range of processors instead of just i3255, and did not provide
> a change log nor a comment in the code describing why processors with
> certain model numbers should display negative temperatures.
>
i3255 happens to be one of the industrial processors in family 17h models
01h through 08h.  These are potentially used at subzero temperatures and
so displaying negative numbers makes a lot sense.

So I think the commit message needs to be be amended to better explain that.

I guided Kannan against leaving a comment in the code with specific models
because it either won't age well as other industrial processors are
introduced or may need to be ping-ponged each time.

But perhaps it should be more generic like:

/* Industrial processors may be used at sub zero temperatures */

> Guenter
>
>> ---
>>   drivers/hwmon/k10temp.c | 8 ++++++--
>>   1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/hwmon/k10temp.c b/drivers/hwmon/k10temp.c
>> index 7b177b9fbb09..2613420d43ff 100644
>> --- a/drivers/hwmon/k10temp.c
>> +++ b/drivers/hwmon/k10temp.c
>> @@ -86,6 +86,7 @@ struct k10temp_data {
>>       u32 show_temp;
>>       bool is_zen;
>>       u32 ccd_offset;
>> +    bool disp_negative;
>>   };
>>     #define TCTL_BIT    0
>> @@ -204,12 +205,12 @@ static int k10temp_read_temp(struct device 
>> *dev, u32 attr, int channel,
>>           switch (channel) {
>>           case 0:        /* Tctl */
>>               *val = get_raw_temp(data);
>> -            if (*val < 0)
>> +            if (*val < 0 && !data->disp_negative)
>>                   *val = 0;
>>               break;
>>           case 1:        /* Tdie */
>>               *val = get_raw_temp(data) - data->temp_offset;
>> -            if (*val < 0)
>> +            if (*val < 0 && !data->disp_negative)
>>                   *val = 0;
>>               break;
>>           case 2 ... 13:        /* Tccd{1-12} */
>> @@ -405,6 +406,9 @@ static int k10temp_probe(struct pci_dev *pdev, 
>> const struct pci_device_id *id)
>>       data->pdev = pdev;
>>       data->show_temp |= BIT(TCTL_BIT);    /* Always show Tctl */
>>   +    if (boot_cpu_data.x86 == 0x17 && boot_cpu_data.x86_model < 0x8)
>> +        data->disp_negative = true;
>> +
>>       if (boot_cpu_data.x86 == 0x15 &&
>>           ((boot_cpu_data.x86_model & 0xf0) == 0x60 ||
>>            (boot_cpu_data.x86_model & 0xf0) == 0x70)) {
>