[PATCH] hwmon: (asus-ec-sensors) read sensors as signed ints

[PATCH] hwmon: (asus-ec-sensors) read sensors as signed ints

[Eugene Shalygin]

Temperature sensor readings are signed, which is hinted by their blank
value (oxd8, 216 as unsigned and -40 as signed). T_Sensor, Crosshair
VIII Hero, and a freezer were used to confirm that.

Here we read fan sensors as signed too, because with their typical
values and 2-byte width, I can't tell a difference between signed and
unsigned, as I don't have a high speed chipset fan.

Signed-off-by: Eugene Shalygin <eugene.shalygin@gmail.com>
---
 drivers/hwmon/asus-ec-sensors.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/hwmon/asus-ec-sensors.c b/drivers/hwmon/asus-ec-sensors.c
index 05244209c0c6..8a04c76527a4 100644
--- a/drivers/hwmon/asus-ec-sensors.c
+++ b/drivers/hwmon/asus-ec-sensors.c
@@ -221,7 +221,7 @@ static const struct dmi_system_id asus_ec_dmi_table[] __initconst = {
 
 struct ec_sensor {
 	unsigned int info_index;
-	u32 cached_value;
+	s32 cached_value;
 };
 
 struct ec_sensors_data {
@@ -408,15 +408,15 @@ static int asus_ec_block_read(const struct device *dev,
 	return status;
 }
 
-static inline u32 get_sensor_value(const struct ec_sensor_info *si, u8 *data)
+static inline s32 get_sensor_value(const struct ec_sensor_info *si, u8 *data)
 {
 	switch (si->addr.components.size) {
 	case 1:
-		return *data;
+		return (s8)*data;
 	case 2:
-		return get_unaligned_be16(data);
+		return (s16)get_unaligned_be16(data);
 	case 4:
-		return get_unaligned_be32(data);
+		return (s32)get_unaligned_be32(data);
 	default:
 		return 0;
 	}
@@ -462,7 +462,7 @@ static int update_ec_sensors(const struct device *dev,
 	return status;
 }
 
-static int scale_sensor_value(u32 value, int data_type)
+static long scale_sensor_value(s32 value, int data_type)
 {
 	switch (data_type) {
 	case hwmon_curr:
@@ -476,7 +476,7 @@ static int scale_sensor_value(u32 value, int data_type)
 
 static int get_cached_value_or_update(const struct device *dev,
 				      int sensor_index,
-				      struct ec_sensors_data *state, u32 *value)
+				      struct ec_sensors_data *state, s32 *value)
 {
 	if (time_after(jiffies, state->last_updated + HZ)) {
 		if (update_ec_sensors(dev, state)) {
@@ -499,7 +499,7 @@ static int asus_ec_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
 			      u32 attr, int channel, long *val)
 {
 	int ret;
-	u32 value = 0;
+	s32 value = 0;
 
 	struct ec_sensors_data *state = dev_get_drvdata(dev);
 	int sidx = find_ec_sensor_index(state, type, channel);
-- 
2.35.1

Re: [PATCH] hwmon: (asus-ec-sensors) read sensors as signed ints

[Guenter Roeck]

On Fri, Feb 04, 2022 at 05:30:45PM +0100, Eugene Shalygin wrote:
> Temperature sensor readings are signed, which is hinted by their blank
> value (oxd8, 216 as unsigned and -40 as signed). T_Sensor, Crosshair
> VIII Hero, and a freezer were used to confirm that.
> 
> Here we read fan sensors as signed too, because with their typical
> values and 2-byte width, I can't tell a difference between signed and
> unsigned, as I don't have a high speed chipset fan.
> 
Hmm, I hope we won't get a situation where an unsigned is reported
and needed, but I guess we can deal with that if/when it happens.

> Signed-off-by: Eugene Shalygin <eugene.shalygin@gmail.com>

Applied.

Thanks,
Guenter

> ---
>  drivers/hwmon/asus-ec-sensors.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/hwmon/asus-ec-sensors.c b/drivers/hwmon/asus-ec-sensors.c
> index 05244209c0c6..8a04c76527a4 100644
> --- a/drivers/hwmon/asus-ec-sensors.c
> +++ b/drivers/hwmon/asus-ec-sensors.c
> @@ -221,7 +221,7 @@ static const struct dmi_system_id asus_ec_dmi_table[] __initconst = {
>  
>  struct ec_sensor {
>  	unsigned int info_index;
> -	u32 cached_value;
> +	s32 cached_value;
>  };
>  
>  struct ec_sensors_data {
> @@ -408,15 +408,15 @@ static int asus_ec_block_read(const struct device *dev,
>  	return status;
>  }
>  
> -static inline u32 get_sensor_value(const struct ec_sensor_info *si, u8 *data)
> +static inline s32 get_sensor_value(const struct ec_sensor_info *si, u8 *data)
>  {
>  	switch (si->addr.components.size) {
>  	case 1:
> -		return *data;
> +		return (s8)*data;
>  	case 2:
> -		return get_unaligned_be16(data);
> +		return (s16)get_unaligned_be16(data);
>  	case 4:
> -		return get_unaligned_be32(data);
> +		return (s32)get_unaligned_be32(data);
>  	default:
>  		return 0;
>  	}
> @@ -462,7 +462,7 @@ static int update_ec_sensors(const struct device *dev,
>  	return status;
>  }
>  
> -static int scale_sensor_value(u32 value, int data_type)
> +static long scale_sensor_value(s32 value, int data_type)
>  {
>  	switch (data_type) {
>  	case hwmon_curr:
> @@ -476,7 +476,7 @@ static int scale_sensor_value(u32 value, int data_type)
>  
>  static int get_cached_value_or_update(const struct device *dev,
>  				      int sensor_index,
> -				      struct ec_sensors_data *state, u32 *value)
> +				      struct ec_sensors_data *state, s32 *value)
>  {
>  	if (time_after(jiffies, state->last_updated + HZ)) {
>  		if (update_ec_sensors(dev, state)) {
> @@ -499,7 +499,7 @@ static int asus_ec_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
>  			      u32 attr, int channel, long *val)
>  {
>  	int ret;
> -	u32 value = 0;
> +	s32 value = 0;
>  
>  	struct ec_sensors_data *state = dev_get_drvdata(dev);
>  	int sidx = find_ec_sensor_index(state, type, channel);

Re: [PATCH] hwmon: (asus-ec-sensors) read sensors as signed ints

[Eugene Shalygin]

> Hmm, I hope we won't get a situation where an unsigned is reported
> and needed, but I guess we can deal with that if/when it happens.

There is still one byte left in the sensor address structure, which I
planned to put data format flags into, and the signees can be one of
those. I know that interpreting the reading for the VRM temperature
misses something, because while the reported value matches the one
reported by the ASUS software when the system thermal state is close
to a thermal equilibrium, right after booting its value (at least with
my board) is lower than the ambient temperature.

Eugene