* Re: [lm-sensors] [PATCH 1/4] hwmon: (adm1026) Use DIV_ROUND_CLOSEST to simplify implementation for S
2014-07-19 3:37 [lm-sensors] [PATCH 1/4] hwmon: (adm1026) Use DIV_ROUND_CLOSEST to simplify implementation for SCALE Axel Lin
@ 2014-07-19 14:49 ` Guenter Roeck
2014-07-19 14:57 ` Axel Lin
` (3 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Guenter Roeck @ 2014-07-19 14:49 UTC (permalink / raw)
To: lm-sensors
On 07/18/2014 08:37 PM, Axel Lin wrote:
> Signed-off-by: Axel Lin <axel.lin@ingics.com>
> ---
> 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.
Jean, do you have any comments ?
Thanks,
Guenter
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [lm-sensors] [PATCH 1/4] hwmon: (adm1026) Use DIV_ROUND_CLOSEST to simplify implementation for S
2014-07-19 3:37 [lm-sensors] [PATCH 1/4] hwmon: (adm1026) Use DIV_ROUND_CLOSEST to simplify implementation for SCALE Axel Lin
2014-07-19 14:49 ` [lm-sensors] [PATCH 1/4] hwmon: (adm1026) Use DIV_ROUND_CLOSEST to simplify implementation for S Guenter Roeck
@ 2014-07-19 14:57 ` Axel Lin
2014-07-19 15:27 ` Guenter Roeck
` (2 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Axel Lin @ 2014-07-19 14:57 UTC (permalink / raw)
To: lm-sensors
2014-07-19 22:49 GMT+08:00 Guenter Roeck <linux@roeck-us.net>:
> On 07/18/2014 08:37 PM, Axel Lin wrote:
>>
>> Signed-off-by: Axel Lin <axel.lin@ingics.com>
>> ---
>> 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.
Regards,
Axel
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [lm-sensors] [PATCH 1/4] hwmon: (adm1026) Use DIV_ROUND_CLOSEST to simplify implementation for S
2014-07-19 3:37 [lm-sensors] [PATCH 1/4] hwmon: (adm1026) Use DIV_ROUND_CLOSEST to simplify implementation for SCALE Axel Lin
2014-07-19 14:49 ` [lm-sensors] [PATCH 1/4] hwmon: (adm1026) Use DIV_ROUND_CLOSEST to simplify implementation for S Guenter Roeck
2014-07-19 14:57 ` Axel Lin
@ 2014-07-19 15:27 ` Guenter Roeck
2014-07-19 17:12 ` Jean Delvare
2014-07-19 18:12 ` Guenter Roeck
4 siblings, 0 replies; 6+ messages in thread
From: Guenter Roeck @ 2014-07-19 15:27 UTC (permalink / raw)
To: lm-sensors
On 07/19/2014 07:57 AM, Axel Lin wrote:
> 2014-07-19 22:49 GMT+08:00 Guenter Roeck <linux@roeck-us.net>:
>> On 07/18/2014 08:37 PM, Axel Lin wrote:
>>>
>>> Signed-off-by: Axel Lin <axel.lin@ingics.com>
>>> ---
>>> 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.
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
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [lm-sensors] [PATCH 1/4] hwmon: (adm1026) Use DIV_ROUND_CLOSEST to simplify implementation for S
2014-07-19 3:37 [lm-sensors] [PATCH 1/4] hwmon: (adm1026) Use DIV_ROUND_CLOSEST to simplify implementation for SCALE Axel Lin
` (2 preceding siblings ...)
2014-07-19 15:27 ` Guenter Roeck
@ 2014-07-19 17:12 ` Jean Delvare
2014-07-19 18:12 ` Guenter Roeck
4 siblings, 0 replies; 6+ messages in thread
From: Jean Delvare @ 2014-07-19 17:12 UTC (permalink / raw)
To: lm-sensors
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@roeck-us.net>:
> >> On 07/18/2014 08:37 PM, Axel Lin wrote:
> >>>
> >>> Signed-off-by: Axel Lin <axel.lin@ingics.com>
> >>> ---
> >>> 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@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [lm-sensors] [PATCH 1/4] hwmon: (adm1026) Use DIV_ROUND_CLOSEST to simplify implementation for S
2014-07-19 3:37 [lm-sensors] [PATCH 1/4] hwmon: (adm1026) Use DIV_ROUND_CLOSEST to simplify implementation for SCALE Axel Lin
` (3 preceding siblings ...)
2014-07-19 17:12 ` Jean Delvare
@ 2014-07-19 18:12 ` Guenter Roeck
4 siblings, 0 replies; 6+ messages in thread
From: Guenter Roeck @ 2014-07-19 18:12 UTC (permalink / raw)
To: lm-sensors
On 07/19/2014 10:12 AM, Jean Delvare wrote:
> 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@roeck-us.net>:
>>>> On 07/18/2014 08:37 PM, Axel Lin wrote:
>>>>>
>>>>> Signed-off-by: Axel Lin <axel.lin@ingics.com>
>>>>> ---
>>>>> 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.
>
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
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 6+ messages in thread