[PATCH 0/4] find_closest() macro

[PATCH 0/4] find_closest() macro

[Bartosz Golaszewski]

This series proposes to unduplicate the code used to find the
member in an array closest to 'x'.

The first patch adds a macro implementing the algorithm in two
flavors - for arrays sorted in ascending and descending order.
Other three patches replace duplicated code with calls to one
of these macros in some hwmon drivers.

Bartosz Golaszewski (4):
  kernel.h: add find_closest() macro
  hwmon: (ina2xx) replace ina226_avg_bits() with find_closest()
  hwmon: (lm85) replace XXX_TO_REG() functions with find_closest()
  hwmon: (w83795) use find_closest_desc() in pwm_freq_to_reg()

 drivers/hwmon/ina2xx.c | 16 ++--------------
 drivers/hwmon/lm85.c   | 43 ++++++++++++-------------------------------
 drivers/hwmon/w83795.c |  7 ++-----
 include/linux/kernel.h | 23 +++++++++++++++++++++++
 4 files changed, 39 insertions(+), 50 deletions(-)

-- 
2.1.4

[PATCH 1/4] kernel.h: add find_closest() macro

[Bartosz Golaszewski]

Searching for the member of an array closest to 'x' is
duplicated in several places.

Add two macros that implement this algorithm for arrays
sorted both in ascending and descending order.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 include/linux/kernel.h | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index d6d630d..f4294da 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -116,6 +116,29 @@
 }							\
 )
 
+#define __find_closest(x, a, as, op)(					\
+{									\
+	typeof(as) _i, _as = (as) - 1;					\
+	typeof(x) _x = (x);						\
+	typeof(*a) *_a = (a);						\
+	for (_i = 0; _i < _as; _i++) {					\
+		if (_x op DIV_ROUND_CLOSEST(_a[_i] + _a[_i + 1], 2))	\
+			break;						\
+	}								\
+	(_i);								\
+}									\
+)
+
+/*
+ * Given an array 'a' (sorted in ascending order) of size 'as' return
+ * the index of the element in that array closest to 'x'.
+ */
+#define find_closest(x, a, as) __find_closest(x, a, as, <=)
+/*
+ * Similar to find_closest(), but 'a' is expected to be sorted
+ * in descending order.
+ */
+#define find_closest_desc(x, a, as) __find_closest(x, a, as, >)
 
 #define _RET_IP_		(unsigned long)__builtin_return_address(0)
 #define _THIS_IP_  ({ __label__ __here; __here: (unsigned long)&&__here; })
-- 
2.1.4

Re: [lm-sensors] [PATCH 1/4] kernel.h: add find_closest() macro

[Phil Pokorny]

On Tue, Feb 24, 2015 at 9:42 AM, Bartosz Golaszewski
<bgolaszewski@baylibre.com> wrote:
>
> Searching for the member of an array closest to 'x' is
> duplicated in several places.
>
> Add two macros that implement this algorithm for arrays
> sorted both in ascending and descending order.

I don't see the point here.  You're not saving any code because your
macros create functions at each invocation site.  And your macro is
more complicated than the code it replaces because it has all the
syntactic cruft to make it adaptable to the different datatypes and
sort orders.

Certainly it is easy to make an off by one mistake in a loop like this
so there might be some small value there, but I'm not sure the
complication is worth that savings for the small number of use points.
Particularly because you're not saving any code.


-- 
Philip Pokorny, RHCE
Chief Technology Officer
PENGUIN COMPUTING, Inc
www.penguincomputing.com

Changing the world through technical innovation

Re: [lm-sensors] [PATCH 1/4] kernel.h: add find_closest() macro

[Guenter Roeck]

On Tue, Feb 24, 2015 at 12:33:06PM -0800, Phil Pokorny wrote:
> On Tue, Feb 24, 2015 at 9:42 AM, Bartosz Golaszewski
> <bgolaszewski@baylibre.com> wrote:
> >
> > Searching for the member of an array closest to 'x' is
> > duplicated in several places.
> >
> > Add two macros that implement this algorithm for arrays
> > sorted both in ascending and descending order.
> 
> I don't see the point here.  You're not saving any code because your
> macros create functions at each invocation site.  And your macro is
> more complicated than the code it replaces because it has all the
> syntactic cruft to make it adaptable to the different datatypes and
> sort orders.
> 
> Certainly it is easy to make an off by one mistake in a loop like this
> so there might be some small value there, but I'm not sure the
> complication is worth that savings for the small number of use points.
> Particularly because you're not saving any code.
> 
I think the lm85 conversion actually introduces a bug with such an
off-by-one mistake. And if it doesn't, there is still a unexplained
and not easy to understand '-1' in one of the calls to find_closest().

So the question is if the new code really improves the situation in that
respect.

Guenter

Re: [lm-sensors] [PATCH 1/4] kernel.h: add find_closest() macro

[Bartosz Golaszewski]

2015-02-24 21:51 GMT+01:00 Guenter Roeck <linux@roeck-us.net>:
> I think the lm85 conversion actually introduces a bug with such an
> off-by-one mistake. And if it doesn't, there is still a unexplained
> and not easy to understand '-1' in one of the calls to find_closest().
>
> So the question is if the new code really improves the situation in that
> respect.

Yes, it's a mistake. I couldn't test this one and missed this '-1'.
Sorry for that.

As for the size comparisons - at first glance it seems as if it adds bloat:

ina2xx:
add/remove: 0/0 grow/shrink: 1/0 up/down: 24/0 (24)
function                                     old     new   delta
ina226_set_interval                          296     320     +24

lm85:
add/remove: 0/0 grow/shrink: 3/0 up/down: 72/0 (72)
function                                     old     new   delta
set_temp_auto_temp_min                       364     388     +24
set_temp_auto_temp_max                       336     360     +24
set_pwm_freq                                 284     308     +24

w83795:
add/remove: 0/0 grow/shrink: 1/0 up/down: 16/0 (16)
function                                     old     new   delta
store_pwm                                    496     512     +16

But this actually comes from DIV_ROUND_CLOSEST() since replacing it
with a simple '/ 2' gives different results:

ina2xx.ko:
add/remove: 0/0 grow/shrink: 1/0 up/down: 6/0 (6)
function                                     old     new   delta
__UNIQUE_ID_vermagic0                         73      79      +6

lm85.ko:
add/remove: 0/0 grow/shrink: 1/0 up/down: 6/0 (6)
function                                     old     new   delta
__UNIQUE_ID_vermagic0                         73      79      +6

w83795.ko:
add/remove: 0/0 grow/shrink: 2/0 up/down: 14/0 (14)
function                                     old     new   delta
store_pwm                                    496     504      +8
__UNIQUE_ID_vermagic0                         73      79      +6

The idea however was to remove duplicated operations from source files
and prevent off-by-one bugs (how ironic the lm85 patch...), not really
to reduce size.

Bart

Re: [lm-sensors] [PATCH 1/4] kernel.h: add find_closest() macro

[Guenter Roeck]

On Wed, Feb 25, 2015 at 11:59:51AM +0100, Bartosz Golaszewski wrote:
> 2015-02-24 21:51 GMT+01:00 Guenter Roeck <linux@roeck-us.net>:
> > I think the lm85 conversion actually introduces a bug with such an
> > off-by-one mistake. And if it doesn't, there is still a unexplained
> > and not easy to understand '-1' in one of the calls to find_closest().
> >
> > So the question is if the new code really improves the situation in that
> > respect.
> 
> Yes, it's a mistake. I couldn't test this one and missed this '-1'.
> Sorry for that.
> 
> As for the size comparisons - at first glance it seems as if it adds bloat:
> 
> ina2xx:
> add/remove: 0/0 grow/shrink: 1/0 up/down: 24/0 (24)
> function                                     old     new   delta
> ina226_set_interval                          296     320     +24
> 
> lm85:
> add/remove: 0/0 grow/shrink: 3/0 up/down: 72/0 (72)
> function                                     old     new   delta
> set_temp_auto_temp_min                       364     388     +24
> set_temp_auto_temp_max                       336     360     +24
> set_pwm_freq                                 284     308     +24
> 
> w83795:
> add/remove: 0/0 grow/shrink: 1/0 up/down: 16/0 (16)
> function                                     old     new   delta
> store_pwm                                    496     512     +16
> 
> But this actually comes from DIV_ROUND_CLOSEST() since replacing it
> with a simple '/ 2' gives different results:
> 
> ina2xx.ko:
> add/remove: 0/0 grow/shrink: 1/0 up/down: 6/0 (6)
> function                                     old     new   delta
> __UNIQUE_ID_vermagic0                         73      79      +6
> 
> lm85.ko:
> add/remove: 0/0 grow/shrink: 1/0 up/down: 6/0 (6)
> function                                     old     new   delta
> __UNIQUE_ID_vermagic0                         73      79      +6
> 
> w83795.ko:
> add/remove: 0/0 grow/shrink: 2/0 up/down: 14/0 (14)
> function                                     old     new   delta
> store_pwm                                    496     504      +8
> __UNIQUE_ID_vermagic0                         73      79      +6
> 
> The idea however was to remove duplicated operations from source files
> and prevent off-by-one bugs (how ironic the lm85 patch...), not really
> to reduce size.
> 
I don't know, seems to me it can cause more trouble and potential for getting
something wrong than it is worth. I'll definitely want to see feedback (and
acceptance) from more senior maintainers than myself.

Guenter

[PATCH 2/4] hwmon: (ina2xx) replace ina226_avg_bits() with find_closest()

[Bartosz Golaszewski]

Use find_closest() to locate the closest average in ina226_avg_tab.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 drivers/hwmon/ina2xx.c | 16 ++--------------
 1 file changed, 2 insertions(+), 14 deletions(-)

diff --git a/drivers/hwmon/ina2xx.c b/drivers/hwmon/ina2xx.c
index d1542b7..fc7f023 100644
--- a/drivers/hwmon/ina2xx.c
+++ b/drivers/hwmon/ina2xx.c
@@ -141,19 +141,6 @@ static const struct ina2xx_config ina2xx_config[] = {
  */
 static const int ina226_avg_tab[] = { 1, 4, 16, 64, 128, 256, 512, 1024 };
 
-static int ina226_avg_bits(int avg)
-{
-	int i;
-
-	/* Get the closest average from the tab. */
-	for (i = 0; i < ARRAY_SIZE(ina226_avg_tab) - 1; i++) {
-		if (avg <= (ina226_avg_tab[i] + ina226_avg_tab[i + 1]) / 2)
-			break;
-	}
-
-	return i; /* Return 0b0111 for values greater than 1024. */
-}
-
 static int ina226_reg_to_interval(u16 config)
 {
 	int avg = ina226_avg_tab[INA226_READ_AVG(config)];
@@ -171,7 +158,8 @@ static u16 ina226_interval_to_reg(int interval, u16 config)
 
 	avg = DIV_ROUND_CLOSEST(interval * 1000,
 				INA226_TOTAL_CONV_TIME_DEFAULT);
-	avg_bits = ina226_avg_bits(avg);
+	avg_bits = find_closest(avg, ina226_avg_tab,
+				ARRAY_SIZE(ina226_avg_tab));
 
 	return (config & ~INA226_AVG_RD_MASK) | INA226_SHIFT_AVG(avg_bits);
 }
-- 
2.1.4

[PATCH 4/4] hwmon: (w83795) use find_closest_desc() in pwm_freq_to_reg()

[Bartosz Golaszewski]

Replace the loop iterating over pwm_freq_cksel0 by a call to
find_closest_desc().

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 drivers/hwmon/w83795.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/hwmon/w83795.c b/drivers/hwmon/w83795.c
index 2189413..1f0b301 100644
--- a/drivers/hwmon/w83795.c
+++ b/drivers/hwmon/w83795.c
@@ -308,11 +308,8 @@ static u8 pwm_freq_to_reg(unsigned long val, u16 clkin)
 	unsigned long best0, best1;
 
 	/* Best fit for cksel = 0 */
-	for (reg0 = 0; reg0 < ARRAY_SIZE(pwm_freq_cksel0) - 1; reg0++) {
-		if (val > (pwm_freq_cksel0[reg0] +
-			   pwm_freq_cksel0[reg0 + 1]) / 2)
-			break;
-	}
+	reg0 = find_closest_desc(val, pwm_freq_cksel0,
+				 ARRAY_SIZE(pwm_freq_cksel0));
 	if (val < 375)	/* cksel = 1 can't beat this */
 		return reg0;
 	best0 = pwm_freq_cksel0[reg0];
-- 
2.1.4