Messages in this thread | | | Date | Thu, 15 Feb 2024 11:27:39 -0800 | Subject | Re: [PATCH v1 1/1] serial: 8250_bcm7271: Replace custom unit definitions | From | Florian Fainelli <> |
| |
On 2/15/24 11:21, Andy Shevchenko wrote: > On Thu, Feb 15, 2024 at 10:35:11AM -0800, Florian Fainelli wrote: >> On 2/15/24 08:02, Andy Shevchenko wrote: > > ... > >>> -#define KHZ 1000 >>> -#define MHZ(x) ((x) * KHZ * KHZ) >>> static const u32 brcmstb_rate_table[] = { >>> - MHZ(81), >>> - MHZ(108), >>> - MHZ(64), /* Actually 64285715 for some chips */ >>> - MHZ(48), >>> + 81 * HZ_PER_MHZ, >>> + 108 * HZ_PER_MHZ, >>> + 64 * HZ_PER_MHZ, /* Actually 64285715 for some chips */ >>> + 48 * HZ_PER_MHZ, >> >> The previous notation was IMHO more readable, > > I tend to disagree as we read in plain text "frequency is 64 MHz", > the patch follows natural language. > >> can we meet in the middle and do: >> >> #define MHZ(x) ((x) * HZ_PER_MHZ >> >> and avoid touching the tables entirely? > > I don't like the intermediate layer which hides the implementation of MHZ(). > What does it do exactly? You need to look at the internals, with the patch > applied you immediately see that these are just constants. >
OK, I suppose today's color is blue for the bike shed.
Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com> -- Florian
[unhandled content-type:application/pkcs7-signature] | |