[PATCH 1/2] units: Add the HZ_PER_KHZ macro

[PATCH 1/2] units: Add the HZ_PER_KHZ macro

[Daniel Lezcano]

The macro for the unit conversion for frequency is duplicated in
different places.

Provide this macro in the 'units' header, so it can be reused.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 include/linux/units.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/include/linux/units.h b/include/linux/units.h
index dcc30a53fa93..218ec0d314b6 100644
--- a/include/linux/units.h
+++ b/include/linux/units.h
@@ -4,6 +4,10 @@
 
 #include <linux/math.h>
 
+#define HZ_PER_KHZ		1000L
+#define KHZ_PER_MHZ		1000L
+#define HZ_PER_MHZ		1000000L
+
 #define MILLIWATT_PER_WATT	1000L
 #define MICROWATT_PER_MILLIWATT	1000L
 #define MICROWATT_PER_WATT	1000000L
-- 
2.17.1

[PATCH 2/2] units: Use the HZ_PER_KHZ macro

[Daniel Lezcano]

The HZ_PER_KHZ macro definition is duplicated in different subsystems.

The macro now exists in include/linux/units.h, make use of it and
remove all the duplicated ones.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/devfreq/devfreq.c         | 2 +-
 drivers/iio/light/as73211.c       | 3 +--
 drivers/thermal/devfreq_cooling.c | 2 +-
 3 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index 6aa10de792b3..4c636c336ace 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -26,6 +26,7 @@
 #include <linux/hrtimer.h>
 #include <linux/of.h>
 #include <linux/pm_qos.h>
+#include <linux/units.h>
 #include "governor.h"
 
 #define CREATE_TRACE_POINTS
@@ -33,7 +34,6 @@
 
 #define IS_SUPPORTED_FLAG(f, name) ((f & DEVFREQ_GOV_FLAG_##name) ? true : false)
 #define IS_SUPPORTED_ATTR(f, name) ((f & DEVFREQ_GOV_ATTR_##name) ? true : false)
-#define HZ_PER_KHZ	1000
 
 static struct class *devfreq_class;
 static struct dentry *devfreq_debugfs;
diff --git a/drivers/iio/light/as73211.c b/drivers/iio/light/as73211.c
index 7b32dfaee9b3..3ba2378df3dd 100644
--- a/drivers/iio/light/as73211.c
+++ b/drivers/iio/light/as73211.c
@@ -24,8 +24,7 @@
 #include <linux/module.h>
 #include <linux/mutex.h>
 #include <linux/pm.h>
-
-#define HZ_PER_KHZ 1000
+#include <linux/units.h>
 
 #define AS73211_DRV_NAME "as73211"
 
diff --git a/drivers/thermal/devfreq_cooling.c b/drivers/thermal/devfreq_cooling.c
index fed3121ff2a1..fa5b8b0c7604 100644
--- a/drivers/thermal/devfreq_cooling.c
+++ b/drivers/thermal/devfreq_cooling.c
@@ -19,10 +19,10 @@
 #include <linux/pm_opp.h>
 #include <linux/pm_qos.h>
 #include <linux/thermal.h>
+#include <linux/units.h>
 
 #include <trace/events/thermal.h>
 
-#define HZ_PER_KHZ		1000
 #define SCALE_ERROR_MITIGATION	100
 
 static DEFINE_IDA(devfreq_ida);
-- 
2.17.1

Re: [PATCH 2/2] units: Use the HZ_PER_KHZ macro

[Christian Eggers]

On Tuesday, 23 February 2021, 21:30:02 CET, Daniel Lezcano wrote:
> The HZ_PER_KHZ macro definition is duplicated in different subsystems.
> 
> The macro now exists in include/linux/units.h, make use of it and
> remove all the duplicated ones.
> 
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
>  drivers/devfreq/devfreq.c         | 2 +-
>  drivers/iio/light/as73211.c       | 3 +--
>  drivers/thermal/devfreq_cooling.c | 2 +-
>  3 files changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index 6aa10de792b3..4c636c336ace 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -26,6 +26,7 @@
>  #include <linux/hrtimer.h>
>  #include <linux/of.h>
>  #include <linux/pm_qos.h>
> +#include <linux/units.h>
>  #include "governor.h"
>  
>  #define CREATE_TRACE_POINTS
> @@ -33,7 +34,6 @@
>  
>  #define IS_SUPPORTED_FLAG(f, name) ((f & DEVFREQ_GOV_FLAG_##name) ? true : false)
>  #define IS_SUPPORTED_ATTR(f, name) ((f & DEVFREQ_GOV_ATTR_##name) ? true : false)
> -#define HZ_PER_KHZ	1000
>  
>  static struct class *devfreq_class;
>  static struct dentry *devfreq_debugfs;
> diff --git a/drivers/iio/light/as73211.c b/drivers/iio/light/as73211.c
> index 7b32dfaee9b3..3ba2378df3dd 100644
> --- a/drivers/iio/light/as73211.c
> +++ b/drivers/iio/light/as73211.c
> @@ -24,8 +24,7 @@
>  #include <linux/module.h>
>  #include <linux/mutex.h>
>  #include <linux/pm.h>
> -
> -#define HZ_PER_KHZ 1000
> +#include <linux/units.h>
>  
>  #define AS73211_DRV_NAME "as73211"
>  
> diff --git a/drivers/thermal/devfreq_cooling.c b/drivers/thermal/devfreq_cooling.c
> index fed3121ff2a1..fa5b8b0c7604 100644
> --- a/drivers/thermal/devfreq_cooling.c
> +++ b/drivers/thermal/devfreq_cooling.c
> @@ -19,10 +19,10 @@
>  #include <linux/pm_opp.h>
>  #include <linux/pm_qos.h>
>  #include <linux/thermal.h>
> +#include <linux/units.h>
>  
>  #include <trace/events/thermal.h>
>  
> -#define HZ_PER_KHZ		1000
>  #define SCALE_ERROR_MITIGATION	100
>  
>  static DEFINE_IDA(devfreq_ida);
> 

Reviewed-by: Christian Eggers <ceggers@arri.de>

Re: [PATCH 2/2] units: Use the HZ_PER_KHZ macro

[Chanwoo Choi]

On 2/24/21 5:30 AM, Daniel Lezcano wrote:
> The HZ_PER_KHZ macro definition is duplicated in different subsystems.
> 
> The macro now exists in include/linux/units.h, make use of it and
> remove all the duplicated ones.
> 
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
>  drivers/devfreq/devfreq.c         | 2 +-
>  drivers/iio/light/as73211.c       | 3 +--
>  drivers/thermal/devfreq_cooling.c | 2 +-
>  3 files changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index 6aa10de792b3..4c636c336ace 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -26,6 +26,7 @@
>  #include <linux/hrtimer.h>
>  #include <linux/of.h>
>  #include <linux/pm_qos.h>
> +#include <linux/units.h>
>  #include "governor.h"
>  
>  #define CREATE_TRACE_POINTS
> @@ -33,7 +34,6 @@
>  
>  #define IS_SUPPORTED_FLAG(f, name) ((f & DEVFREQ_GOV_FLAG_##name) ? true : false)
>  #define IS_SUPPORTED_ATTR(f, name) ((f & DEVFREQ_GOV_ATTR_##name) ? true : false)
> -#define HZ_PER_KHZ	1000
>  
>  static struct class *devfreq_class;
>  static struct dentry *devfreq_debugfs;
> diff --git a/drivers/iio/light/as73211.c b/drivers/iio/light/as73211.c
> index 7b32dfaee9b3..3ba2378df3dd 100644
> --- a/drivers/iio/light/as73211.c
> +++ b/drivers/iio/light/as73211.c
> @@ -24,8 +24,7 @@
>  #include <linux/module.h>
>  #include <linux/mutex.h>
>  #include <linux/pm.h>
> -
> -#define HZ_PER_KHZ 1000
> +#include <linux/units.h>
>  
>  #define AS73211_DRV_NAME "as73211"
>  
> diff --git a/drivers/thermal/devfreq_cooling.c b/drivers/thermal/devfreq_cooling.c
> index fed3121ff2a1..fa5b8b0c7604 100644
> --- a/drivers/thermal/devfreq_cooling.c
> +++ b/drivers/thermal/devfreq_cooling.c
> @@ -19,10 +19,10 @@
>  #include <linux/pm_opp.h>
>  #include <linux/pm_qos.h>
>  #include <linux/thermal.h>
> +#include <linux/units.h>
>  
>  #include <trace/events/thermal.h>
>  
> -#define HZ_PER_KHZ		1000
>  #define SCALE_ERROR_MITIGATION	100
>  
>  static DEFINE_IDA(devfreq_ida);
> 

For devfreq part,
Acked-by: Chanwoo Choi <cw00.choi@samsung.com>

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH 1/2] units: Add the HZ_PER_KHZ macro

[Andrew Morton]

On Wed, 24 Feb 2021 10:39:36 +0200 Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Wednesday, February 24, 2021, Andy Shevchenko <andy.shevchenko@gmail.com>
> wrote:
> 
> >
> >
> > On Tuesday, February 23, 2021, Daniel Lezcano <daniel.lezcano@linaro.org>
> > wrote:
> >
> >> The macro for the unit conversion for frequency is duplicated in
> >> different places.
> >>
> >> Provide this macro in the 'units' header, so it can be reused.
> >>
> >>
> >
> > Thanks! That was the idea behind my reviews to add those definitions
> > explicitly in the users. I just want to be sure you covered them all. Also
> > there are few non-standard names for above in some drivers (they can be
> > fixed on per driver basis in separate patches though).
> >
> >
> 
> Seems you introduced a common macro and forget about dropping it elsewhere.
> 
> https://elixir.bootlin.com/linux/latest/A/ident/HZ_PER_MHZ

Yes.  And HZ_PER_KHZ.

Also, why make them signed types?  Negative Hz is physically
nonsensical.  If that upsets some code somewhere because it was dealing
with signed types then, well, that code needed fixing anyway.

Ditto MILLIWATT_PER_WATT and friends, sigh.

Re: [PATCH 1/2] units: Add the HZ_PER_KHZ macro

[Andy Shevchenko]

On Thu, Mar 4, 2021 at 2:31 AM Andrew Morton <akpm@linux-foundation.org> wrote:
> On Wed, 24 Feb 2021 10:39:36 +0200 Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

...

> Also, why make them signed types?  Negative Hz is physically
> nonsensical.  If that upsets some code somewhere because it was dealing
> with signed types then, well, that code needed fixing anyway.
>
> Ditto MILLIWATT_PER_WATT and friends, sigh.

And all seconds also. I guess it's historically like this and in any
case I don't expect we will have multipliers that don't fit long long.

There might be some subtle integral promotion rules that screw the
flow up, but I have not much experience with it to be honest.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/2] units: Add the HZ_PER_KHZ macro

[Daniel Lezcano]

Hi Andrew,

On 04/03/2021 01:31, Andrew Morton wrote:
> On Wed, 24 Feb 2021 10:39:36 +0200 Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> 
>> On Wednesday, February 24, 2021, Andy Shevchenko <andy.shevchenko@gmail.com>
>> wrote:
>>
>>>
>>>
>>> On Tuesday, February 23, 2021, Daniel Lezcano <daniel.lezcano@linaro.org>
>>> wrote:
>>>
>>>> The macro for the unit conversion for frequency is duplicated in
>>>> different places.
>>>>
>>>> Provide this macro in the 'units' header, so it can be reused.
>>>>
>>>>
>>>
>>> Thanks! That was the idea behind my reviews to add those definitions
>>> explicitly in the users. I just want to be sure you covered them all. Also
>>> there are few non-standard names for above in some drivers (they can be
>>> fixed on per driver basis in separate patches though).
>>>
>>>
>>
>> Seems you introduced a common macro and forget about dropping it elsewhere.
>>
>> https://elixir.bootlin.com/linux/latest/A/ident/HZ_PER_MHZ
> 
> Yes.  And HZ_PER_KHZ.

Thanks for the review, it is fixed it in the v2.

> Also, why make them signed types?  Negative Hz is physically
> nonsensical.  If that upsets some code somewhere because it was dealing
> with signed types then, well, that code needed fixing anyway.
> 
> Ditto MILLIWATT_PER_WATT and friends, sigh.

At the first glance converting to unsigned long should not hurt the
users of this macro.

The current series introduces the macro and its usage but by converting
the existing type.

Is it ok if I send a separate series to change the units from L to UL?

-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH 1/2] units: Add the HZ_PER_KHZ macro

[Andrew Morton]

On Thu, 4 Mar 2021 13:41:27 +0100 Daniel Lezcano <daniel.lezcano@linaro.org> wrote:

> > Also, why make them signed types?  Negative Hz is physically
> > nonsensical.  If that upsets some code somewhere because it was dealing
> > with signed types then, well, that code needed fixing anyway.
> > 
> > Ditto MILLIWATT_PER_WATT and friends, sigh.
> 
> At the first glance converting to unsigned long should not hurt the
> users of this macro.
> 
> The current series introduces the macro and its usage but by converting
> the existing type.
> 
> Is it ok if I send a separate series to change the units from L to UL?

That's the way to do it...