Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Sender: Guenter Roeck <groeck7@gmail.com>
Date:   Thu, 3 Jun 2021 03:51:27 -0700
From:   Guenter Roeck <linux@roeck-us.net>
To:     Riwen Lu <luriwen@kylinos.cn>
Cc:     jdelvare@suse.com, linux-hwmon@vger.kernel.org,
        linux-kernel@vger.kernel.org, Xin Chen <chenxin@kylinos.cn>
Subject: Re: [PATCH v2] hwmon: (scpi-hwmon) shows the negative temperature
 properly
Message-ID: <20210603105127.GB644104@roeck-us.net>
References: <20210603083845.569751-1-luriwen@kylinos.cn>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20210603083845.569751-1-luriwen@kylinos.cn>
Precedence: bulk

On Thu, Jun 03, 2021 at 04:38:45PM +0800, Riwen Lu wrote:
> The scpi hwmon shows the sub-zero temperature in an unsigned integer,
> which would confuse the users when the machine works in low temperature
> environment. This shows the sub-zero temperature in an signed value and
> users can get it properly from sensors.
> 
> Signed-off-by: Riwen Lu <luriwen@kylinos.cn>
> Tested-by: Xin Chen <chenxin@kylinos.cn>

Please ask Xin Chen to send a separate Tested-by: tag.

> 
> ---
> Changes since v1:
> - Add judgment for sensor->info.class. If it is TEMPERATURE situation,
>   return the sensor value as a signed value, otherwise return it as a
>   unsigned value.
> ---
>  drivers/hwmon/scpi-hwmon.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/hwmon/scpi-hwmon.c b/drivers/hwmon/scpi-hwmon.c
> index 25aac40f2764..45a7e319143d 100644
> --- a/drivers/hwmon/scpi-hwmon.c
> +++ b/drivers/hwmon/scpi-hwmon.c
> @@ -99,7 +99,10 @@ scpi_show_sensor(struct device *dev, struct device_attribute *attr, char *buf)
>  
>  	scpi_scale_reading(&value, sensor);
>  
> -	return sprintf(buf, "%llu\n", value);
> +	if (sensor->info.class == TEMPERATURE)
> +		return sprintf(buf, "%lld\n", value);

'value' is u64, so it needs a typecast to s64.

Also, please add a comment ahead of the if statement explaining that temperature
sensor values are treated as signed values based on observation even though that
is not explicitly specified, and because an unsigned u64 temperature does not
really make practical sense.

> +	else

else after return is not needed

Thanks,
Guenter

> +		return sprintf(buf, "%llu\n", value);
>  }
>  
>  static ssize_t
> -- 
> 2.25.1
> 
> 
> No virus found
> 		Checked by Hillstone Network AntiVirus