Re: [PATCH v1 1/1] serial: 8250_bcm7271: Replace custom unit definitions

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

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.

-- 
With Best Regards,
Andy Shevchenko