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
