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

[Guenter Roeck <linux@roeck-us.net>]

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