On Sat, 19 Jul 2014 08:27:26 -0700, Guenter Roeck wrote:
> On 07/19/2014 07:57 AM, Axel Lin wrote:
> > 2014-07-19 22:49 GMT+08:00 Guenter Roeck <linux@xxxxxxxxxxxx>:
> >> On 07/18/2014 08:37 PM, Axel Lin wrote:
> >>>
> >>> Signed-off-by: Axel Lin <axel.lin@xxxxxxxxxx>
> >>> ---
> >>> drivers/hwmon/adm1026.c | 2 +-
> >>> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/hwmon/adm1026.c b/drivers/hwmon/adm1026.c
> >>> index ca8430f..c632e46 100644
> >>> --- a/drivers/hwmon/adm1026.c
> >>> +++ b/drivers/hwmon/adm1026.c
> >>> @@ -196,7 +196,7 @@ static int adm1026_scaling[] = { /* .001 Volts */
> >>> 3330, 4995, 2250, 12000, 13875
> >>> };
> >>> #define NEG12_OFFSET 16000
> >>> -#define SCALE(val, from, to) (((val)*(to) + ((from)/2))/(from))
> >>> +#define SCALE(val, from, to) DIV_ROUND_CLOSEST((val) * (to), (from))
> >>> #define INS_TO_REG(n, val) (clamp_val(SCALE(val, adm1026_scaling[n],
> >>> 192),\
> >>> 0, 255))
> >>> #define INS_FROM_REG(n, val) (SCALE(val, 192, adm1026_scaling[n]))
> >>>
> >> Hi Axel,
> >>
> >> I don't really see the value in this series. It does not really improve
> >> anything,
> >> or make the code smaller.
> >
> > Though there is nothing wrong in original code.
> > IMHO, I think use DIV_ROUND_CLOSEST is less error prone than having
> > the similar calculation in various drivers.
>
> That is more applicable to the SCALE macro or function itself, though.
>
> For example, if you look into adm9240.c, you'll notice that SCALE returns
> an int but gets a long as argument. This is asking for overflows. Also,
> it is called prior to calling clamp_val, which again invites overflow errors.
> Sure, those are corner cases, and especially the clamp_val problem would
> be difficult to address, but getting rid of SCALE entirely would at least
> address the long->int overflow problem. Your patch doesn't do that.
>
> In other cases, SCALE uses a multiplication factor of 1, which means it is
> really unnecessary and could be replaced directly with DIV_ROUND_CLOSEST.
This was exactly my thought when looking at this series.
> In other words, I'd be more inclined to accept patches which get rid of
> the SCALE macro or function entirely.
I really have no opinion about this, sorry.
--
Jean Delvare
SUSE L3 Support
_______________________________________________
lm-sensors mailing list
lm-sensors@xxxxxxxxxxxxxx
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
