Messages in this thread | | | From | Dmitry Rokosov <> | Subject | Re: [PATCH v2 1/3] units: complement the set of Hz units | Date | Mon, 1 Aug 2022 13:01:40 +0000 |
| |
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
| |