Replace custom unit definitions that are available via units.h.
Signed-off-by: Andy Shevchenko <[email protected]>
---
drivers/tty/serial/8250/8250_bcm7271.c | 17 ++++++++---------
1 file changed, 8 insertions(+), 9 deletions(-)
diff --git a/drivers/tty/serial/8250/8250_bcm7271.c b/drivers/tty/serial/8250/8250_bcm7271.c
index 504c4c020857..1532fa2e8ec4 100644
--- a/drivers/tty/serial/8250/8250_bcm7271.c
+++ b/drivers/tty/serial/8250/8250_bcm7271.c
@@ -22,6 +22,7 @@
#include <linux/delay.h>
#include <linux/clk.h>
#include <linux/debugfs.h>
+#include <linux/units.h>
#include "8250.h"
@@ -187,21 +188,19 @@
#define TX_BUF_SIZE 4096
#define RX_BUF_SIZE 4096
#define RX_BUFS_COUNT 2
-#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,
};
static const u32 brcmstb_rate_table_7278[] = {
- MHZ(81),
- MHZ(108),
+ 81 * HZ_PER_MHZ,
+ 108 * HZ_PER_MHZ,
0,
- MHZ(48),
+ 48 * HZ_PER_MHZ,
};
struct brcmuart_priv {
--
2.43.0.rc1.1.gbec44491f096
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
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 <[email protected]>
--
Florian
On 2/15/24 08:02, Andy Shevchenko wrote:
> Replace custom unit definitions that are available via units.h.
>
> Signed-off-by: Andy Shevchenko <[email protected]>
> ---
> drivers/tty/serial/8250/8250_bcm7271.c | 17 ++++++++---------
> 1 file changed, 8 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/tty/serial/8250/8250_bcm7271.c b/drivers/tty/serial/8250/8250_bcm7271.c
> index 504c4c020857..1532fa2e8ec4 100644
> --- a/drivers/tty/serial/8250/8250_bcm7271.c
> +++ b/drivers/tty/serial/8250/8250_bcm7271.c
> @@ -22,6 +22,7 @@
> #include <linux/delay.h>
> #include <linux/clk.h>
> #include <linux/debugfs.h>
> +#include <linux/units.h>
>
> #include "8250.h"
>
> @@ -187,21 +188,19 @@
> #define TX_BUF_SIZE 4096
> #define RX_BUF_SIZE 4096
> #define RX_BUFS_COUNT 2
> -#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, can we meet in the middle
and do:
#define MHZ(x) ((x) * HZ_PER_MHZ
and avoid touching the tables entirely?
--
Florian
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 <[email protected]>
Thank you!
--
With Best Regards,
Andy Shevchenko