[PATCH v2 1/2] linux/kernel.h: Introduce IDIV_ROUND_CLOSEST

[PATCH v2 1/2] linux/kernel.h: Introduce IDIV_ROUND_CLOSEST

[Guenter Roeck]

DIV_ROUND_CLOSEST returns a bad result for negative dividends:
	DIV_ROUND_CLOSEST(-2, 2) = 0

Most of the time this does not matter. However, in the hardware monitoring
subsystem, it is sometimes used on integers which can be negative (such as
temperatures). Introduce new macro IDIV_ROUND_CLOSEST which also supports
negative dividends.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
v2: v1 did not work if typeof(divisor) was an unsigned variable type
    (which can obviously not be negative).
    Rework to revert to DIV_ROUND_CLOSEST if the dividend is unsigned,
    or if it is signed but non-negative.

Plan is to merge this patch through my hwmon tree unless there is an objection.

 include/linux/kernel.h |   10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 6043821..42f1fd6 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -89,6 +89,16 @@
 }							\
 )
 
+#define IDIV_ROUND_CLOSEST(x, divisor)(			\
+{							\
+	typeof(x) __x = x;				\
+	typeof(divisor) __d = divisor;			\
+	(((typeof(x))-1) >= 0 || (__x) >= 0) ?		\
+		DIV_ROUND_CLOSEST((__x), (__d)) :	\
+		(((__x) - ((__d) / 2)) / (__d));	\
+}							\
+)
+
 /*
  * Multiplies an integer by a fraction, while avoiding unnecessary
  * overflow or loss of precision.
-- 
1.7.9.7

[PATCH 2/2] hwmon: Use IDIV_ROUND_CLOSEST if the dividend can be negative

[Guenter Roeck]

DIV_ROUND_CLOSEST returns bad values for negative dividends.
Replace with IDIV_ROUND_CLOSEST where this can happen.

Cc: Jean Delvare <khali@linux-fr.org>
Cc: Dirk Eibach <eibach@gdsys.de>
Cc: Steve Glendinning <steve.glendinning@shawell.net>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/hwmon/ad7314.c           |    2 +-
 drivers/hwmon/ads1015.c          |    2 +-
 drivers/hwmon/adt7410.c          |    6 +++---
 drivers/hwmon/emc2103.c          |    4 ++--
 drivers/hwmon/pmbus/pmbus_core.c |    4 ++--
 drivers/hwmon/w83627ehf.c        |    2 +-
 6 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/hwmon/ad7314.c b/drivers/hwmon/ad7314.c
index 455294f..6fb3013 100644
--- a/drivers/hwmon/ad7314.c
+++ b/drivers/hwmon/ad7314.c
@@ -81,7 +81,7 @@ static ssize_t ad7314_show_temperature(struct device *dev,
 		data = (data << 2) >> 2;
 
 		return sprintf(buf, "%d\n",
-			       DIV_ROUND_CLOSEST(data * 3125, 100));
+			       IDIV_ROUND_CLOSEST(data * 3125, 100));
 	default:
 		return -EINVAL;
 	}
diff --git a/drivers/hwmon/ads1015.c b/drivers/hwmon/ads1015.c
index 2798246..ab12c7f 100644
--- a/drivers/hwmon/ads1015.c
+++ b/drivers/hwmon/ads1015.c
@@ -114,7 +114,7 @@ static int ads1015_reg_to_mv(struct i2c_client *client, unsigned int channel,
 	unsigned int pga = data->channel_data[channel].pga;
 	int fullscale = fullscale_table[pga];
 
-	return DIV_ROUND_CLOSEST(reg * fullscale, 0x7ff0);
+	return IDIV_ROUND_CLOSEST(reg * fullscale, 0x7ff0);
 }
 
 /* sysfs callback function */
diff --git a/drivers/hwmon/adt7410.c b/drivers/hwmon/adt7410.c
index 030c8d7..0acc887 100644
--- a/drivers/hwmon/adt7410.c
+++ b/drivers/hwmon/adt7410.c
@@ -173,8 +173,8 @@ abort:
 
 static s16 ADT7410_TEMP_TO_REG(long temp)
 {
-	return DIV_ROUND_CLOSEST(SENSORS_LIMIT(temp, ADT7410_TEMP_MIN,
-					       ADT7410_TEMP_MAX) * 128, 1000);
+	return IDIV_ROUND_CLOSEST(SENSORS_LIMIT(temp, ADT7410_TEMP_MIN,
+						ADT7410_TEMP_MAX) * 128, 1000);
 }
 
 static int ADT7410_REG_TO_TEMP(struct adt7410_data *data, s16 reg)
@@ -186,7 +186,7 @@ static int ADT7410_REG_TO_TEMP(struct adt7410_data *data, s16 reg)
 	 * temperature is stored in twos complement format, in steps of
 	 * 1/128°C
 	 */
-	return DIV_ROUND_CLOSEST(reg * 1000, 128);
+	return IDIV_ROUND_CLOSEST(reg * 1000, 128);
 }
 
 /*-----------------------------------------------------------------------*/
diff --git a/drivers/hwmon/emc2103.c b/drivers/hwmon/emc2103.c
index 77f434c..161343d 100644
--- a/drivers/hwmon/emc2103.c
+++ b/drivers/hwmon/emc2103.c
@@ -250,7 +250,7 @@ static ssize_t set_temp_min(struct device *dev, struct device_attribute *da,
 	if (result < 0)
 		return -EINVAL;
 
-	val = DIV_ROUND_CLOSEST(val, 1000);
+	val = IDIV_ROUND_CLOSEST(val, 1000);
 	if ((val < -63) || (val > 127))
 		return -EINVAL;
 
@@ -274,7 +274,7 @@ static ssize_t set_temp_max(struct device *dev, struct device_attribute *da,
 	if (result < 0)
 		return -EINVAL;
 
-	val = DIV_ROUND_CLOSEST(val, 1000);
+	val = IDIV_ROUND_CLOSEST(val, 1000);
 	if ((val < -63) || (val > 127))
 		return -EINVAL;
 
diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
index 29b319d..58ddf57 100644
--- a/drivers/hwmon/pmbus/pmbus_core.c
+++ b/drivers/hwmon/pmbus/pmbus_core.c
@@ -507,7 +507,7 @@ static long pmbus_reg2data_direct(struct pmbus_data *data,
 		R--;
 	}
 	while (R < 0) {
-		val = DIV_ROUND_CLOSEST(val, 10);
+		val = IDIV_ROUND_CLOSEST(val, 10);
 		R++;
 	}
 
@@ -647,7 +647,7 @@ static u16 pmbus_data2reg_direct(struct pmbus_data *data,
 		R--;
 	}
 	while (R < 0) {
-		val = DIV_ROUND_CLOSEST(val, 10);
+		val = IDIV_ROUND_CLOSEST(val, 10);
 		R++;
 	}
 
diff --git a/drivers/hwmon/w83627ehf.c b/drivers/hwmon/w83627ehf.c
index 1821b74..f500fa9 100644
--- a/drivers/hwmon/w83627ehf.c
+++ b/drivers/hwmon/w83627ehf.c
@@ -1258,7 +1258,7 @@ store_temp_offset(struct device *dev, struct device_attribute *attr,
 	if (err < 0)
 		return err;
 
-	val = SENSORS_LIMIT(DIV_ROUND_CLOSEST(val, 1000), -128, 127);
+	val = SENSORS_LIMIT(IDIV_ROUND_CLOSEST(val, 1000), -128, 127);
 
 	mutex_lock(&data->update_lock);
 	data->temp_offset[nr] = val;
-- 
1.7.9.7

Re: [PATCH v2 1/2] linux/kernel.h: Introduce IDIV_ROUND_CLOSEST

[Andrew Morton]

On Tue, 28 Aug 2012 09:30:55 -0700
Guenter Roeck <linux@roeck-us.net> wrote:

> DIV_ROUND_CLOSEST returns a bad result for negative dividends:
> 	DIV_ROUND_CLOSEST(-2, 2) = 0
> 
> Most of the time this does not matter. However, in the hardware monitoring
> subsystem, it is sometimes used on integers which can be negative (such as
> temperatures). Introduce new macro IDIV_ROUND_CLOSEST which also supports
> negative dividends.
> 

Can't we just fix DIV_ROUND_CLOSEST?  That will make it a bit slower
but it's not exactly a speed demon right now.  And fixing
DIV_ROUND_CLOSEST() might just fix other bugs that we don't know about
yet.

Also, the name IDIV_ROUND_CLOSEST doesn't communicate much at all.


> +#define IDIV_ROUND_CLOSEST(x, divisor)(			\
> +{							\
> +	typeof(x) __x = x;				\
> +	typeof(divisor) __d = divisor;			\
> +	(((typeof(x))-1) >= 0 || (__x) >= 0) ?		\
> +		DIV_ROUND_CLOSEST((__x), (__d)) :	\
> +		(((__x) - ((__d) / 2)) / (__d));	\
> +}							\
> +)

And it doesn't help that the new "function" is undocumented.  Yes, we
screwed up with DIV_ROUND_CLOSEST(), but that doesn't mean we need to
keep screwing up!

Re: [PATCH v2 1/2] linux/kernel.h: Introduce IDIV_ROUND_CLOSEST

[Guenter Roeck]

On Thu, Aug 30, 2012 at 04:15:32PM -0700, Andrew Morton wrote:
> On Tue, 28 Aug 2012 09:30:55 -0700
> Guenter Roeck <linux@roeck-us.net> wrote:
> 
> > DIV_ROUND_CLOSEST returns a bad result for negative dividends:
> > 	DIV_ROUND_CLOSEST(-2, 2) = 0
> > 
> > Most of the time this does not matter. However, in the hardware monitoring
> > subsystem, it is sometimes used on integers which can be negative (such as
> > temperatures). Introduce new macro IDIV_ROUND_CLOSEST which also supports
> > negative dividends.
> > 
> 
> Can't we just fix DIV_ROUND_CLOSEST?  That will make it a bit slower
> but it's not exactly a speed demon right now.  And fixing
> DIV_ROUND_CLOSEST() might just fix other bugs that we don't know about
> yet.
> 
Sure, fine with me. I'll submit a patch. Let's see who starts screaming.

Guenter