On Thu, Feb 15, 2024 at 11:27:39AM -0800, Florian Fainelli wrote:
> 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.
Sky is blue and sun is shining and I am happy person :-)
> Reviewed-by: Florian Fainelli <florian.fainelli@xxxxxxxxxxxx>
Thank you!
--
With Best Regards,
Andy Shevchenko
