| Message ID | 20220729172332.19118-2-ddrokosov@sberdevices.ru (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | units: complement the set of Hz units | expand |
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
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 >
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 > > > >
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
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(+)