Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Sender: Guenter Roeck <groeck7@gmail.com>
Date:   Thu, 29 Dec 2022 06:26:05 -0800
From:   Guenter Roeck <linux@roeck-us.net>
To:     Armin Wolf <W_Armin@gmx.de>
Cc:     jdelvare@suse.com, linux-hwmon@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/3] hwmon: (ftsteutates) Fix scaling of measurements
Message-ID: <20221229142605.GA20427@roeck-us.net>
References: <20221224041855.83981-1-W_Armin@gmx.de>
 <20221224041855.83981-2-W_Armin@gmx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20221224041855.83981-2-W_Armin@gmx.de>
Precedence: bulk

On Sat, Dec 24, 2022 at 05:18:53AM +0100, Armin Wolf wrote:
> A user complained that the ftsteutates driver was displaying
> bogus values since its introduction. This happens because the
> sensor measurements need to be scaled in order to produce
> meaningful results:
> - the fan speed needs to be multiplied by 60 since its in RPS
> - the temperature is in degrees celsius and needs an offset of 64
> - the voltage is in 1/256 of 3.3V
> 
> The offical datasheet says the voltage needs to be divided by 256,
> but this is likely an off-by-one-error, since even the BIOS
> devides by 255 (otherwise 3.3V could not be measured).
> 
> The voltage channels additionally need a board-specific multiplier,
> however this can be done by the driver since its board-specific.
> 
> The reason the missing scaling of measurements is the way Fujitsu
> used this driver when it was still out-of-tree. Back then, all
> scaling was done in userspace by libsensors, even the generic one.
> 
> Tested on a Fujitsu DS3401-B1.
> 
> Fixes: 08426eda58e0 ("hwmon: Add driver for FTS BMC chip "Teutates"")
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>

Applied.

Thanks,
Guenter

> ---
>  Documentation/hwmon/ftsteutates.rst |  4 ++++
>  drivers/hwmon/ftsteutates.c         | 19 +++++++++++++------
>  2 files changed, 17 insertions(+), 6 deletions(-)
> 
> --
> 2.30.2
> 
> diff --git a/Documentation/hwmon/ftsteutates.rst b/Documentation/hwmon/ftsteutates.rst
> index 58a2483d8d0d..198fa8e2819d 100644
> --- a/Documentation/hwmon/ftsteutates.rst
> +++ b/Documentation/hwmon/ftsteutates.rst
> @@ -22,6 +22,10 @@ enhancements. It can monitor up to 4 voltages, 16 temperatures and
>  8 fans. It also contains an integrated watchdog which is currently
>  implemented in this driver.
> 
> +The 4 voltages require a board-specific multiplier, since the BMC can
> +only measure voltages up to 3.3V and thus relies on voltage dividers.
> +Consult your motherboard manual for details.
> +
>  To clear a temperature or fan alarm, execute the following command with the
>  correct path to the alarm file::
> 
> diff --git a/drivers/hwmon/ftsteutates.c b/drivers/hwmon/ftsteutates.c
> index f5b8e724a8ca..ffa0bb364877 100644
> --- a/drivers/hwmon/ftsteutates.c
> +++ b/drivers/hwmon/ftsteutates.c
> @@ -12,6 +12,7 @@
>  #include <linux/i2c.h>
>  #include <linux/init.h>
>  #include <linux/jiffies.h>
> +#include <linux/math.h>
>  #include <linux/module.h>
>  #include <linux/mutex.h>
>  #include <linux/slab.h>
> @@ -347,13 +348,15 @@ static ssize_t in_value_show(struct device *dev,
>  {
>  	struct fts_data *data = dev_get_drvdata(dev);
>  	int index = to_sensor_dev_attr(devattr)->index;
> -	int err;
> +	int value, err;
> 
>  	err = fts_update_device(data);
>  	if (err < 0)
>  		return err;
> 
> -	return sprintf(buf, "%u\n", data->volt[index]);
> +	value = DIV_ROUND_CLOSEST(data->volt[index] * 3300, 255);
> +
> +	return sprintf(buf, "%d\n", value);
>  }
> 
>  static ssize_t temp_value_show(struct device *dev,
> @@ -361,13 +364,15 @@ static ssize_t temp_value_show(struct device *dev,
>  {
>  	struct fts_data *data = dev_get_drvdata(dev);
>  	int index = to_sensor_dev_attr(devattr)->index;
> -	int err;
> +	int value, err;
> 
>  	err = fts_update_device(data);
>  	if (err < 0)
>  		return err;
> 
> -	return sprintf(buf, "%u\n", data->temp_input[index]);
> +	value = (data->temp_input[index] - 64) * 1000;
> +
> +	return sprintf(buf, "%d\n", value);
>  }
> 
>  static ssize_t temp_fault_show(struct device *dev,
> @@ -436,13 +441,15 @@ static ssize_t fan_value_show(struct device *dev,
>  {
>  	struct fts_data *data = dev_get_drvdata(dev);
>  	int index = to_sensor_dev_attr(devattr)->index;
> -	int err;
> +	int value, err;
> 
>  	err = fts_update_device(data);
>  	if (err < 0)
>  		return err;
> 
> -	return sprintf(buf, "%u\n", data->fan_input[index]);
> +	value = data->fan_input[index] * 60;
> +
> +	return sprintf(buf, "%d\n", value);
>  }
> 
>  static ssize_t fan_source_show(struct device *dev,