[PATCH v2 0/3] units: complement the set of Hz units

[PATCH v2 0/3] units: complement the set of Hz units

[Dmitry Rokosov]

During msa311 accel IIO driver development

https://lore.kernel.org/linux-iio/20220616104211.9257-1-ddrokosov@sberdevices.ru/

Andy requested to use proper units in the hz->ms calculation. Current
units.h header doesn't have milli, micro and nano HZ coefficients, so
some drivers (in the IIO subsystem) implement their own copies for that.

The current patchset resolves such a problem and intoduces general
MILLIHZ_PER_HZ, UHZ_PER_HZ and NHZ_PER_HZ definitions in the units.h, and
fixes all drivers which duplicate these units.

Changes v1->v2:
    - changed MHZ_PER_HZ to a different name as Andy suggested
      (suppose MILLIHZ_PER_HZ is good enough)

Dmitry Rokosov (3):
  units: complement the set of Hz units
  iio: accel: adxl345: use HZ macro from units.h
  iio: common: scmi_sensors: use HZ macro from units.h

 drivers/iio/accel/adxl345_core.c           | 2 +-
 drivers/iio/common/scmi_sensors/scmi_iio.c | 2 +-
 include/linux/units.h                      | 3 +++
 3 files changed, 5 insertions(+), 2 deletions(-)

-- 
2.36.0

[PATCH v2 1/3] units: complement the set of Hz units

[Dmitry Rokosov]

Currently, Hz units do not have milli, micro and nano Hz coefficients.
Some drivers (IIO especially) use their analogues to calculate
appropriate Hz values. This patch includes them to units.h definitions,
so they can be used from different kernel places.

Signed-off-by: Dmitry Rokosov <ddrokosov@sberdevices.ru>
---
 include/linux/units.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/linux/units.h b/include/linux/units.h
index 681fc652e3d7..8bb83c6ea97d 100644
--- a/include/linux/units.h
+++ b/include/linux/units.h
@@ -20,6 +20,9 @@
 #define PICO	1000000000000ULL
 #define FEMTO	1000000000000000ULL
 
+#define NHZ_PER_HZ		1000000000UL
+#define UHZ_PER_HZ		1000000UL
+#define MILLIHZ_PER_HZ		1000UL
 #define HZ_PER_KHZ		1000UL
 #define KHZ_PER_MHZ		1000UL
 #define HZ_PER_MHZ		1000000UL
-- 
2.36.0

Re: [PATCH v2 1/3] units: complement the set of Hz units

[Andy Shevchenko]

On Fri, Jul 29, 2022 at 7:23 PM Dmitry Rokosov <DDRokosov@sberdevices.ru> wrote:
>
> Currently, Hz units do not have milli, micro and nano Hz coefficients.
> Some drivers (IIO especially) use their analogues to calculate
> appropriate Hz values. This patch includes them to units.h definitions,
> so they can be used from different kernel places.

...

> +#define NHZ_PER_HZ             1000000000UL
> +#define UHZ_PER_HZ             1000000UL
> +#define MILLIHZ_PER_HZ         1000UL

Oh, but then a bit of consistency?

MICRO
NANO

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/3] units: complement the set of Hz units

[Jonathan Cameron]

On Fri, 29 Jul 2022 20:02:42 +0200
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Fri, Jul 29, 2022 at 7:23 PM Dmitry Rokosov <DDRokosov@sberdevices.ru> wrote:
> >
> > Currently, Hz units do not have milli, micro and nano Hz coefficients.
> > Some drivers (IIO especially) use their analogues to calculate
> > appropriate Hz values. This patch includes them to units.h definitions,
> > so they can be used from different kernel places.  
> 
> ...
> 
> > +#define NHZ_PER_HZ             1000000000UL
> > +#define UHZ_PER_HZ             1000000UL
> > +#define MILLIHZ_PER_HZ         1000UL  
> 
> Oh, but then a bit of consistency?
> 
> MICRO
> NANO
Tricky given existing items, but I agree we shouldn't make
it worse.

However, I'm not 100% sold on why we need these conversions relative to HZ.
What's wrong with using MILLI / NANO etc as already defined?  I guess
there is a 'documentation' like effect of making it clear these are frequency
unit conversions, but I don't think it makes sense to add it for all the other
types of unit, so why is Hz special?

I'm not sure why we have the existing ones for HZ with the
exception of KHZ_PER_MEGAHZ.

Jonathan

> 

Re: [PATCH v2 1/3] units: complement the set of Hz units

[Dmitry Rokosov]

Hello Jonathan,

Thank you for the review. Please find my comments below.

On Sun, Jul 31, 2022 at 12:41:55PM +0100, Jonathan Cameron wrote:
> On Fri, 29 Jul 2022 20:02:42 +0200
> Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> 
> > On Fri, Jul 29, 2022 at 7:23 PM Dmitry Rokosov <DDRokosov@sberdevices.ru> wrote:
> > >
> > > Currently, Hz units do not have milli, micro and nano Hz coefficients.
> > > Some drivers (IIO especially) use their analogues to calculate
> > > appropriate Hz values. This patch includes them to units.h definitions,
> > > so they can be used from different kernel places.  
> > 
> > ...
> > 
> > > +#define NHZ_PER_HZ             1000000000UL
> > > +#define UHZ_PER_HZ             1000000UL
> > > +#define MILLIHZ_PER_HZ         1000UL  
> > 
> > Oh, but then a bit of consistency?
> > 
> > MICRO
> > NANO
> Tricky given existing items, but I agree we shouldn't make
> it worse.

Okay, agree. I'll change them to MILLI/MICRO/NANO in the next version.

> 
> However, I'm not 100% sold on why we need these conversions relative to HZ.
> What's wrong with using MILLI / NANO etc as already defined?  I guess
> there is a 'documentation' like effect of making it clear these are frequency
> unit conversions, but I don't think it makes sense to add it for all the other
> types of unit, so why is Hz special?

Yes, you are totally right, it has some 'documenation' effect from
a physics theory perspective. Kernel already has some units for HZ, so I
think it's a bad idea when sometimes we have to use *HZ for KILO and
MEGA units, but sometimes we have to use abstract MILLI/MICRO/NANO
coefficients. In other words, I suppose the right way is to choose one
option from the following list:
   - remove all *HZ units and use abstract units instead
   - complement *HZ units and use them

In my opinion, the second one is better, because *HZ units are good
opposite to *SEC units (from time64.h).

> 
> I'm not sure why we have the existing ones for HZ with the
> exception of KHZ_PER_MEGAHZ.
> 
> Jonathan
> 
> > 
> 

-- 
Thank you,
Dmitry

[PATCH v2 2/3] iio: accel: adxl345: use HZ macro from units.h

[Dmitry Rokosov]

Remove duplicated definition of NHZ_PER_HZ, because it's available in
the units.h.

Signed-off-by: Dmitry Rokosov <ddrokosov@sberdevices.ru>
---
 drivers/iio/accel/adxl345_core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
index 370bfec1275a..94189133fe8f 100644
--- a/drivers/iio/accel/adxl345_core.c
+++ b/drivers/iio/accel/adxl345_core.c
@@ -10,6 +10,7 @@
 #include <linux/module.h>
 #include <linux/property.h>
 #include <linux/regmap.h>
+#include <linux/units.h>
 
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
@@ -32,7 +33,6 @@
 
 #define ADXL345_BW_RATE			GENMASK(3, 0)
 #define ADXL345_BASE_RATE_NANO_HZ	97656250LL
-#define NHZ_PER_HZ			1000000000LL
 
 #define ADXL345_POWER_CTL_MEASURE	BIT(3)
 #define ADXL345_POWER_CTL_STANDBY	0x00
-- 
2.36.0

Re: [PATCH v2 2/3] iio: accel: adxl345: use HZ macro from units.h

[Andy Shevchenko]

On Fri, Jul 29, 2022 at 7:23 PM Dmitry Rokosov <DDRokosov@sberdevices.ru> wrote:
>
> Remove duplicated definition of NHZ_PER_HZ, because it's available in
> the units.h.

Fine to me in principle (whatever name it gets after all)
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

> Signed-off-by: Dmitry Rokosov <ddrokosov@sberdevices.ru>
> ---
>  drivers/iio/accel/adxl345_core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
> index 370bfec1275a..94189133fe8f 100644
> --- a/drivers/iio/accel/adxl345_core.c
> +++ b/drivers/iio/accel/adxl345_core.c
> @@ -10,6 +10,7 @@
>  #include <linux/module.h>
>  #include <linux/property.h>
>  #include <linux/regmap.h>
> +#include <linux/units.h>
>
>  #include <linux/iio/iio.h>
>  #include <linux/iio/sysfs.h>
> @@ -32,7 +33,6 @@
>
>  #define ADXL345_BW_RATE                        GENMASK(3, 0)
>  #define ADXL345_BASE_RATE_NANO_HZ      97656250LL
> -#define NHZ_PER_HZ                     1000000000LL
>
>  #define ADXL345_POWER_CTL_MEASURE      BIT(3)
>  #define ADXL345_POWER_CTL_STANDBY      0x00
> --
> 2.36.0



-- 
With Best Regards,
Andy Shevchenko

[PATCH v2 3/3] iio: common: scmi_sensors: use HZ macro from units.h

[Dmitry Rokosov]

Remove duplicated definition of UHZ_PER_HZ, because it's available in
the units.h.

Signed-off-by: Dmitry Rokosov <ddrokosov@sberdevices.ru>
---
 drivers/iio/common/scmi_sensors/scmi_iio.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/common/scmi_sensors/scmi_iio.c b/drivers/iio/common/scmi_sensors/scmi_iio.c
index 793d628db55f..c6d2cf5504cb 100644
--- a/drivers/iio/common/scmi_sensors/scmi_iio.c
+++ b/drivers/iio/common/scmi_sensors/scmi_iio.c
@@ -18,6 +18,7 @@
 #include <linux/scmi_protocol.h>
 #include <linux/time.h>
 #include <linux/types.h>
+#include <linux/units.h>
 
 #define SCMI_IIO_NUM_OF_AXIS 3
 
@@ -130,7 +131,6 @@ static const struct iio_buffer_setup_ops scmi_iio_buffer_ops = {
 static int scmi_iio_set_odr_val(struct iio_dev *iio_dev, int val, int val2)
 {
 	struct scmi_iio_priv *sensor = iio_priv(iio_dev);
-	const unsigned long UHZ_PER_HZ = 1000000UL;
 	u64 sec, mult, uHz, sf;
 	u32 sensor_config;
 	char buf[32];
-- 
2.36.0

Re: [PATCH v2 3/3] iio: common: scmi_sensors: use HZ macro from units.h

[Andy Shevchenko]

On Fri, Jul 29, 2022 at 7:23 PM Dmitry Rokosov <DDRokosov@sberdevices.ru> wrote:
>
> Remove duplicated definition of UHZ_PER_HZ, because it's available in
> the units.h.

Fine to me in principle (whatever name it gets after all)
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

> Signed-off-by: Dmitry Rokosov <ddrokosov@sberdevices.ru>
> ---
>  drivers/iio/common/scmi_sensors/scmi_iio.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/iio/common/scmi_sensors/scmi_iio.c b/drivers/iio/common/scmi_sensors/scmi_iio.c
> index 793d628db55f..c6d2cf5504cb 100644
> --- a/drivers/iio/common/scmi_sensors/scmi_iio.c
> +++ b/drivers/iio/common/scmi_sensors/scmi_iio.c
> @@ -18,6 +18,7 @@
>  #include <linux/scmi_protocol.h>
>  #include <linux/time.h>
>  #include <linux/types.h>
> +#include <linux/units.h>
>
>  #define SCMI_IIO_NUM_OF_AXIS 3
>
> @@ -130,7 +131,6 @@ static const struct iio_buffer_setup_ops scmi_iio_buffer_ops = {
>  static int scmi_iio_set_odr_val(struct iio_dev *iio_dev, int val, int val2)
>  {
>         struct scmi_iio_priv *sensor = iio_priv(iio_dev);
> -       const unsigned long UHZ_PER_HZ = 1000000UL;
>         u64 sec, mult, uHz, sf;
>         u32 sensor_config;
>         char buf[32];
> --
> 2.36.0



-- 
With Best Regards,
Andy Shevchenko