hwmon: (adm1026) Use DIV_ROUND_CLOSEST to simplify implementation for SCALE
PATCH 1/4 hwmon: (adm1026) Use DIV_ROUND_CLOSEST to simplify implementation for SCALE
--- drivers/hwmon/adm1026.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/hwmon/adm1026.c +1 -1
@@ -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]))
Signature · 9 lines
1.9.1 _______________________________________________ lm-sensors mailing list [email protected] http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
Guenter Roeck
maintainerRe: [lm-sensors] [PATCH 1/4] hwmon: (adm1026) Use DIV_ROUND_CLOSEST to simplify implementation for S
Quoted 11 lines · Axel Lin
Signed-off-by Axel Lin <[email protected]>--- drivers/hwmon/adm1026.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/hwmon/adm1026.c +0
-0
drivers/hwmon/adm1026.c static int adm1026_scaling[] = { /* .001 Volts */ · line 202
Quoted 8 lines
drivers/hwmon/adm1026.c +1
-1@@ -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.
Jean, do you have any comments ?
Thanks,
Guenter
_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
Re: [lm-sensors] [PATCH 1/4] hwmon: (adm1026) Use DIV_ROUND_CLOSEST to simplify implementation for S
In reply to Axel Lin2014-07-19 22:49 GMT+08:00 Guenter Roeck <[email protected]>:
Quoted 24 lines · Axel Lin
Quoted 19 lines
Signed-off-by Axel Lin <[email protected]>--- drivers/hwmon/adm1026.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/hwmon/adm1026.c +1
-1@@ -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))Quoted 2 lines
#define INS_FROM_REG(n, val) (SCALE(val, 192, adm1026_scaling[n]))Hi Axel,
Quoted 3 lines
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.
Regards,
Axel
_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
Guenter Roeck
maintainerRe: [lm-sensors] [PATCH 1/4] hwmon: (adm1026) Use DIV_ROUND_CLOSEST to simplify implementation for S
In reply to Axel LinQuoted 25 lines · Axel Lin
2014-07-19 22:49 GMT+08:00 Guenter Roeck <[email protected]>:
Quoted 24 lines · Axel Lin
Quoted 19 lines
Signed-off-by Axel Lin <[email protected]>--- drivers/hwmon/adm1026.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/hwmon/adm1026.c +1
-1@@ -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))Quoted 2 lines
#define INS_FROM_REG(n, val) (SCALE(val, 192, adm1026_scaling[n]))Hi Axel,
Quoted 8 lines
Quoted 3 lines
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 useDIV_ROUND_CLOSESTis 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.
In other words, I'd be more inclined to accept patches which get rid of the SCALE macro or function entirely.
Thanks,
Guenter
_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
Re: [lm-sensors] [PATCH 1/4] hwmon: (adm1026) Use DIV_ROUND_CLOSEST to simplify implementation for S
In reply to Axel LinQuoted 43 lines · Guenter Roeck
Quoted 25 lines · Axel Lin
2014-07-19 22:49 GMT+08:00 Guenter Roeck <[email protected]>:
Quoted 24 lines · Axel Lin
Quoted 19 lines
Signed-off-by Axel Lin <[email protected]>--- drivers/hwmon/adm1026.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/hwmon/adm1026.c +1
-1@@ -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))Quoted 2 lines
#define INS_FROM_REG(n, val) (SCALE(val, 192, adm1026_scaling[n]))Hi Axel,
Quoted 7 lines
Quoted 3 lines
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 useDIV_ROUND_CLOSESTis 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 callingclamp_val, which again invites overflow errors. Sure, those are corner cases, and especially theclamp_valproblem would be difficult to address, but getting rid of SCALE entirely would at least address thelong->intoverflow problem. Your patch doesn't do that.
Quoted 2 lines
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.
Quoted 2 lines
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.
Signature · 7 lines
Jean Delvare SUSE L3 Support _______________________________________________ lm-sensors mailing list [email protected] http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
Guenter Roeck
maintainerRe: [lm-sensors] [PATCH 1/4] hwmon: (adm1026) Use DIV_ROUND_CLOSEST to simplify implementation for S
In reply to Axel LinQuoted 47 lines · Jean Delvare
Quoted 43 lines · Guenter Roeck
Quoted 25 lines · Axel Lin
2014-07-19 22:49 GMT+08:00 Guenter Roeck <[email protected]>:
Quoted 24 lines · Axel Lin
Quoted 19 lines
Signed-off-by Axel Lin <[email protected]>--- drivers/hwmon/adm1026.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/hwmon/adm1026.c +1
-1@@ -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))Quoted 2 lines
#define INS_FROM_REG(n, val) (SCALE(val, 192, adm1026_scaling[n]))Hi Axel,
Quoted 7 lines
Quoted 3 lines
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 useDIV_ROUND_CLOSESTis 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 callingclamp_val, which again invites overflow errors. Sure, those are corner cases, and especially theclamp_valproblem would be difficult to address, but getting rid of SCALE entirely would at least address thelong->intoverflow problem. Your patch doesn't do that.Quoted 2 lines
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.
Quoted 7 lines
This was exactly my thought when looking at this series.
Quoted 2 lines
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.
Lets focus on real problems and simplifications, then. Easy to fix overflow problems like the long->int problem mentioned above, or multiplications by one.
Guenter
_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors