linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 1/2] linux/kernel.h: Introduce IDIV_ROUND_CLOSEST
@ 2012-08-28 16:30 Guenter Roeck
  2012-08-28 16:30 ` [PATCH 2/2] hwmon: Use IDIV_ROUND_CLOSEST if the dividend can be negative Guenter Roeck
  2012-08-30 23:15 ` [PATCH v2 1/2] linux/kernel.h: Introduce IDIV_ROUND_CLOSEST Andrew Morton
  0 siblings, 2 replies; 4+ messages in thread
From: Guenter Roeck @ 2012-08-28 16:30 UTC (permalink / raw)
  To: linux-kernel
  Cc: Andrew Morton, Pekka Enberg, Ingo Molnar, H. Peter Anvin,
	Jean Delvare, lm-sensors, 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


^ permalink raw reply	[flat|nested] 4+ messages in thread

* [PATCH 2/2] hwmon: Use IDIV_ROUND_CLOSEST if the dividend can be negative
  2012-08-28 16:30 [PATCH v2 1/2] linux/kernel.h: Introduce IDIV_ROUND_CLOSEST Guenter Roeck
@ 2012-08-28 16:30 ` Guenter Roeck
  2012-08-30 23:15 ` [PATCH v2 1/2] linux/kernel.h: Introduce IDIV_ROUND_CLOSEST Andrew Morton
  1 sibling, 0 replies; 4+ messages in thread
From: Guenter Roeck @ 2012-08-28 16:30 UTC (permalink / raw)
  To: linux-kernel
  Cc: Andrew Morton, Pekka Enberg, Ingo Molnar, H. Peter Anvin,
	Jean Delvare, lm-sensors, Guenter Roeck, Dirk Eibach,
	Steve Glendinning

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


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v2 1/2] linux/kernel.h: Introduce IDIV_ROUND_CLOSEST
  2012-08-28 16:30 [PATCH v2 1/2] linux/kernel.h: Introduce IDIV_ROUND_CLOSEST Guenter Roeck
  2012-08-28 16:30 ` [PATCH 2/2] hwmon: Use IDIV_ROUND_CLOSEST if the dividend can be negative Guenter Roeck
@ 2012-08-30 23:15 ` Andrew Morton
  2012-08-30 23:50   ` Guenter Roeck
  1 sibling, 1 reply; 4+ messages in thread
From: Andrew Morton @ 2012-08-30 23:15 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: linux-kernel, Pekka Enberg, Ingo Molnar, H. Peter Anvin,
	Jean Delvare, lm-sensors

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!

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v2 1/2] linux/kernel.h: Introduce IDIV_ROUND_CLOSEST
  2012-08-30 23:15 ` [PATCH v2 1/2] linux/kernel.h: Introduce IDIV_ROUND_CLOSEST Andrew Morton
@ 2012-08-30 23:50   ` Guenter Roeck
  0 siblings, 0 replies; 4+ messages in thread
From: Guenter Roeck @ 2012-08-30 23:50 UTC (permalink / raw)
  To: Andrew Morton
  Cc: linux-kernel, Pekka Enberg, Ingo Molnar, H. Peter Anvin,
	Jean Delvare, lm-sensors

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2012-08-30 23:50 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-08-28 16:30 [PATCH v2 1/2] linux/kernel.h: Introduce IDIV_ROUND_CLOSEST Guenter Roeck
2012-08-28 16:30 ` [PATCH 2/2] hwmon: Use IDIV_ROUND_CLOSEST if the dividend can be negative Guenter Roeck
2012-08-30 23:15 ` [PATCH v2 1/2] linux/kernel.h: Introduce IDIV_ROUND_CLOSEST Andrew Morton
2012-08-30 23:50   ` Guenter Roeck

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).