* [PATCH v1 1/1] serial: 8250_bcm7271: Replace custom unit definitions
@ 2024-02-15 16:02 Andy Shevchenko
2024-02-15 16:32 ` Ilpo Järvinen
2024-02-15 18:35 ` Florian Fainelli
0 siblings, 2 replies; 6+ messages in thread
From: Andy Shevchenko @ 2024-02-15 16:02 UTC (permalink / raw)
To: Greg Kroah-Hartman, linux-serial, linux-kernel
Cc: Al Cooper, Broadcom internal kernel review list, Jiri Slaby,
Andy Shevchenko
Replace custom unit definitions that are available via units.h.
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
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
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH v1 1/1] serial: 8250_bcm7271: Replace custom unit definitions
2024-02-15 16:02 [PATCH v1 1/1] serial: 8250_bcm7271: Replace custom unit definitions Andy Shevchenko
@ 2024-02-15 16:32 ` Ilpo Järvinen
2024-02-15 18:35 ` Florian Fainelli
1 sibling, 0 replies; 6+ messages in thread
From: Ilpo Järvinen @ 2024-02-15 16:32 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Greg Kroah-Hartman, linux-serial, LKML, Al Cooper,
Broadcom internal kernel review list, Jiri Slaby
[-- Attachment #1: Type: text/plain, Size: 262 bytes --]
On Thu, 15 Feb 2024, Andy Shevchenko wrote:
> Replace custom unit definitions that are available via units.h.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
--
i.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v1 1/1] serial: 8250_bcm7271: Replace custom unit definitions
2024-02-15 16:02 [PATCH v1 1/1] serial: 8250_bcm7271: Replace custom unit definitions Andy Shevchenko
2024-02-15 16:32 ` Ilpo Järvinen
@ 2024-02-15 18:35 ` Florian Fainelli
2024-02-15 19:21 ` Andy Shevchenko
1 sibling, 1 reply; 6+ messages in thread
From: Florian Fainelli @ 2024-02-15 18:35 UTC (permalink / raw)
To: Andy Shevchenko, Greg Kroah-Hartman, linux-serial, linux-kernel
Cc: Al Cooper, Broadcom internal kernel review list, Jiri Slaby
[-- Attachment #1: Type: text/plain, Size: 1312 bytes --]
On 2/15/24 08:02, Andy Shevchenko wrote:
> Replace custom unit definitions that are available via units.h.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
> 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
[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4221 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v1 1/1] serial: 8250_bcm7271: Replace custom unit definitions
2024-02-15 18:35 ` Florian Fainelli
@ 2024-02-15 19:21 ` Andy Shevchenko
2024-02-15 19:27 ` Florian Fainelli
0 siblings, 1 reply; 6+ messages in thread
From: Andy Shevchenko @ 2024-02-15 19:21 UTC (permalink / raw)
To: Florian Fainelli
Cc: Greg Kroah-Hartman, linux-serial, linux-kernel, Al Cooper,
Broadcom internal kernel review list, Jiri Slaby
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v1 1/1] serial: 8250_bcm7271: Replace custom unit definitions
2024-02-15 19:21 ` Andy Shevchenko
@ 2024-02-15 19:27 ` Florian Fainelli
2024-02-15 19:40 ` Andy Shevchenko
0 siblings, 1 reply; 6+ messages in thread
From: Florian Fainelli @ 2024-02-15 19:27 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Greg Kroah-Hartman, linux-serial, linux-kernel, Al Cooper,
Broadcom internal kernel review list, Jiri Slaby
[-- Attachment #1: Type: text/plain, Size: 1153 bytes --]
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
[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4221 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v1 1/1] serial: 8250_bcm7271: Replace custom unit definitions
2024-02-15 19:27 ` Florian Fainelli
@ 2024-02-15 19:40 ` Andy Shevchenko
0 siblings, 0 replies; 6+ messages in thread
From: Andy Shevchenko @ 2024-02-15 19:40 UTC (permalink / raw)
To: Florian Fainelli
Cc: Greg Kroah-Hartman, linux-serial, linux-kernel, Al Cooper,
Broadcom internal kernel review list, Jiri Slaby
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@broadcom.com>
Thank you!
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-02-15 19:40 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-02-15 16:02 [PATCH v1 1/1] serial: 8250_bcm7271: Replace custom unit definitions Andy Shevchenko
2024-02-15 16:32 ` Ilpo Järvinen
2024-02-15 18:35 ` Florian Fainelli
2024-02-15 19:21 ` Andy Shevchenko
2024-02-15 19:27 ` Florian Fainelli
2024-02-15 19:40 ` Andy Shevchenko
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox