On Fri, Jun 24, 2022 at 11:27:24PM +1000, Michael Ellerman wrote: > Scott Cheloha <cheloha@xxxxxxxxxxxxx> writes: > > + * - For the "Query Watchdog Capabilities" operation, a 64-bit > > + * value structured as follows: > > + * > > + * Bits 0-15: The minimum supported timeout in milliseconds. > > + * Bits 16-31: The number of watchdogs supported. > > + * Bits 32-63: Reserved. > > + */ > > +#define PSERIES_WDTQ_MIN_TIMEOUT(cap) GETFIELD((cap), 0, 15) > > This one is less obviously better, but I still think it's clearer as all > the logic is there in front of you, rather than hidden in the macro. It > is clearer that we're only returning a 16-bit value. > > #define PSERIES_WDTQ_MIN_TIMEOUT(cap) (((cap) >> 48) & 0xffff) Or even ((cap) >> 48) since it is a 64-bit value. If you want better defences you should not use macros here at all, anyway (but inline functions, instead). I could rant about the 1000UL being meaningless and/or misleading, or that 0x1 is just silly, but it is a sunny day :-) Segher
