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

From: Dmitry Rokosov
Date: Mon Aug 01 2022 - 09:02:49 EST


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@xxxxxxxxx> wrote:
>
> > On Fri, Jul 29, 2022 at 7:23 PM Dmitry Rokosov <DDRokosov@xxxxxxxxxxxxxx> 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