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

[Limonciello, Mario]

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@xxxxxxx>
>
> 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)) {