Re: [PATCH V2] PCI: rcar: Clean up the macros
On 04/08/2018 08:03 PM, Randy Dunlap wrote: > On 04/08/2018 09:53 AM, Marek Vasut wrote: >> On 04/08/2018 06:51 PM, Randy Dunlap wrote: >>> On 04/08/2018 09:34 AM, Marek Vasut wrote: On 04/08/2018 05:27 PM, Randy Dunlap wrote: > On 04/08/2018 06:09 AM, Marek Vasut wrote: >> This patch replaces the (1 << n) with BIT(n) and cleans up whitespace, >> no functional change. >> >> Signed-off-by: Marek Vasut >> Cc: Geert Uytterhoeven >> Cc: Phil Edworthy >> Cc: Simon Horman >> Cc: Wolfram Sang >> Cc: linux-renesas-soc@vger.kernel.org >> --- >> V2: Reword the commit message >> --- >> drivers/pci/host/pcie-rcar.c | 52 >> ++-- >> 1 file changed, 26 insertions(+), 26 deletions(-) >> >> diff --git a/drivers/pci/host/pcie-rcar.c b/drivers/pci/host/pcie-rcar.c >> index 25f68305322c..5ab7bf6a8de0 100644 >> --- a/drivers/pci/host/pcie-rcar.c >> +++ b/drivers/pci/host/pcie-rcar.c >> @@ -30,9 +30,9 @@ > > missing this: > > #include This compiles just fine without it though, which means it's probably pulled in through some of the other includes already present. >>> >>> I suspected that. However, please see rule #1 in >>> Documentation/process/submit-checklist.rst: >>> >>> 1) If you use a facility then #include the file that defines/declares >>>that facility. Don't depend on other header files pulling in ones >>>that you use. >> >> If I was to include every single header from which I use something, >> wouldn't that make the list real long then ? Is that really worth it? > > The length of the list of headers is not an issue. > And if you are "cleaning up," you might as well do a full job of it. I sent a V3 of this "clean up". > Just because it builds on one $ARCH (with various pulled-in headers) does > not mean that it will build on another $ARCH (with different pulled-in > headers). > > Or did you build it on all linux/arch/* ? No, since it is limited in Kconfig: depends on ARCH_RENESAS || (ARM && COMPILE_TEST) -- Best regards, Marek Vasut
Re: [PATCH V2] PCI: rcar: Clean up the macros
On 04/08/2018 09:53 AM, Marek Vasut wrote: > On 04/08/2018 06:51 PM, Randy Dunlap wrote: >> On 04/08/2018 09:34 AM, Marek Vasut wrote: >>> On 04/08/2018 05:27 PM, Randy Dunlap wrote: On 04/08/2018 06:09 AM, Marek Vasut wrote: > This patch replaces the (1 << n) with BIT(n) and cleans up whitespace, > no functional change. > > Signed-off-by: Marek Vasut > Cc: Geert Uytterhoeven > Cc: Phil Edworthy > Cc: Simon Horman > Cc: Wolfram Sang > Cc: linux-renesas-soc@vger.kernel.org > --- > V2: Reword the commit message > --- > drivers/pci/host/pcie-rcar.c | 52 > ++-- > 1 file changed, 26 insertions(+), 26 deletions(-) > > diff --git a/drivers/pci/host/pcie-rcar.c b/drivers/pci/host/pcie-rcar.c > index 25f68305322c..5ab7bf6a8de0 100644 > --- a/drivers/pci/host/pcie-rcar.c > +++ b/drivers/pci/host/pcie-rcar.c > @@ -30,9 +30,9 @@ missing this: #include >>> >>> This compiles just fine without it though, which means it's probably >>> pulled in through some of the other includes already present. >>> >> >> I suspected that. However, please see rule #1 in >> Documentation/process/submit-checklist.rst: >> >> 1) If you use a facility then #include the file that defines/declares >>that facility. Don't depend on other header files pulling in ones >>that you use. > > If I was to include every single header from which I use something, > wouldn't that make the list real long then ? Is that really worth it? The length of the list of headers is not an issue. And if you are "cleaning up," you might as well do a full job of it. Just because it builds on one $ARCH (with various pulled-in headers) does not mean that it will build on another $ARCH (with different pulled-in headers). Or did you build it on all linux/arch/* ? thanks, -- ~Randy
Re: [PATCH V2] PCI: rcar: Clean up the macros
Hi Marek,
Thanks for your work.
On 2018-04-08 15:09:42 +0200, Marek Vasut wrote:
> This patch replaces the (1 << n) with BIT(n) and cleans up whitespace,
> no functional change.
>
> Signed-off-by: Marek Vasut
> Cc: Geert Uytterhoeven
> Cc: Phil Edworthy
> Cc: Simon Horman
> Cc: Wolfram Sang
> Cc: linux-renesas-soc@vger.kernel.org
I like this cleanup! I have no input on Randy's comment about if you
should include bitops.h or not. But for the macro changes themselves.
Reviewed-by: Niklas Söderlund
> ---
> V2: Reword the commit message
> ---
> drivers/pci/host/pcie-rcar.c | 52
> ++--
> 1 file changed, 26 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/pci/host/pcie-rcar.c b/drivers/pci/host/pcie-rcar.c
> index 25f68305322c..5ab7bf6a8de0 100644
> --- a/drivers/pci/host/pcie-rcar.c
> +++ b/drivers/pci/host/pcie-rcar.c
> @@ -30,9 +30,9 @@
>
> #define PCIECAR 0x10
> #define PCIECCTLR0x18
> -#define CONFIG_SEND_ENABLE (1 << 31)
> +#define CONFIG_SEND_ENABLE BIT(31)
> #define TYPE0 (0 << 8)
> -#define TYPE1 (1 << 8)
> +#define TYPE1 BIT(8)
> #define PCIECDR 0x20
> #define PCIEMSR 0x28
> #define PCIEINTXR0x000400
> @@ -44,7 +44,7 @@
> #define PCIETSTR 0x02004
> #define DATA_LINK_ACTIVE1
> #define PCIEERRFR0x02020
> -#define UNSUPPORTED_REQUEST (1 << 4)
> +#define UNSUPPORTED_REQUEST BIT(4)
> #define PCIEMSIFR0x02044
> #define PCIEMSIALR 0x02048
> #define MSIFE 1
> @@ -57,17 +57,17 @@
> /* local address reg & mask */
> #define PCIELAR(x) (0x02200 + ((x) * 0x20))
> #define PCIELAMR(x) (0x02208 + ((x) * 0x20))
> -#define LAM_PREFETCH(1 << 3)
> -#define LAM_64BIT (1 << 2)
> -#define LAR_ENABLE (1 << 1)
> +#define LAM_PREFETCHBIT(3)
> +#define LAM_64BIT BIT(2)
> +#define LAR_ENABLE BIT(1)
>
> /* PCIe address reg & mask */
> #define PCIEPALR(x) (0x03400 + ((x) * 0x20))
> #define PCIEPAUR(x) (0x03404 + ((x) * 0x20))
> #define PCIEPAMR(x) (0x03408 + ((x) * 0x20))
> #define PCIEPTCTLR(x)(0x0340c + ((x) * 0x20))
> -#define PAR_ENABLE (1 << 31)
> -#define IO_SPACE(1 << 8)
> +#define PAR_ENABLE BIT(31)
> +#define IO_SPACEBIT(8)
>
> /* Configuration */
> #define PCICONF(x) (0x01 + ((x) * 0x4))
> @@ -79,23 +79,23 @@
> #define IDSETR1 0x011004
> #define TLCTLR 0x011048
> #define MACSR0x011054
> -#define SPCHGFIN(1 << 4)
> -#define SPCHGFAIL (1 << 6)
> -#define SPCHGSUC(1 << 7)
> +#define SPCHGFINBIT(4)
> +#define SPCHGFAIL BIT(6)
> +#define SPCHGSUCBIT(7)
> #define LINK_SPEED (0xf << 16)
> #define LINK_SPEED_2_5GTS (1 << 16)
> #define LINK_SPEED_5_0GTS (2 << 16)
> #define MACCTLR 0x011058
> -#define SPEED_CHANGE(1 << 24)
> -#define SCRAMBLE_DISABLE(1 << 27)
> +#define SPEED_CHANGEBIT(24)
> +#define SCRAMBLE_DISABLEBIT(27)
> #define MACS2R 0x011078
> #define MACCGSPSETR 0x011084
> -#define SPCNGRSN(1 << 31)
> +#define SPCNGRSNBIT(31)
>
> /* R-Car H1 PHY */
> #define H1_PCIEPHYADRR 0x04000c
> -#define WRITE_CMD (1 << 16)
> -#define PHY_ACK (1 << 24)
> +#define WRITE_CMD BIT(16)
> +#define PHY_ACK BIT(24)
> #define RATE_POS12
> #define LANE_POS8
> #define ADR_POS 0
> @@ -107,19 +107,19 @@
> #define GEN2_PCIEPHYDATA 0x784
> #define GEN2_PCIEPHYCTRL 0x78c
>
> -#define INT_PCI_MSI_NR 32
> +#define INT_PCI_MSI_NR 32
>
> -#define RCONF(x) (PCICONF(0)+(x))
> -#define RPMCAP(x)(PMCAP(0)+(x))
> -#define REXPCAP(x) (EXPCAP(0)+(x))
> -#define RVCCAP(x)(VCCAP(0)+(x))
> +#define RCONF(x) (PCICONF(0) + (x))
> +#define RPMCAP(x)(PMCAP(0) + (x))
> +#define REXPCAP(x) (EXPCAP(0) + (x))
> +#define RVCCAP(x)(VCCAP(0) + (x))
>
> -#define PCIE_CONF_BUS(b)(((b) & 0xff) << 24)
> -#define PCIE_CONF_DEV(d)(((d) & 0x1f) << 19)
> -#define PCIE_CONF_FUNC(f) (((f) & 0x7) << 16)
> +#define PCIE_CONF_BUS(b) (((b) & 0xff) << 24)
> +#define PCIE_CONF_DEV(d) (((d) & 0x1f) << 19)
> +#define PCIE_CONF_FUNC(f)(((f) & 0x7) << 16)
>
> -#define RCAR_PCI_MAX_RESOURCES 4
> -#define MAX_NR_INBOUND_MAPS 6
> +#define RCAR_PCI_MAX_RESOURCES 4
> +#define MAX_NR_INBOUND_MAPS 6
>
> struct rcar_msi {
> DECLARE_BITMAP(used, I
Re: [PATCH V2] PCI: rcar: Clean up the macros
On 04/08/2018 06:51 PM, Randy Dunlap wrote: > On 04/08/2018 09:34 AM, Marek Vasut wrote: >> On 04/08/2018 05:27 PM, Randy Dunlap wrote: >>> On 04/08/2018 06:09 AM, Marek Vasut wrote: This patch replaces the (1 << n) with BIT(n) and cleans up whitespace, no functional change. Signed-off-by: Marek Vasut Cc: Geert Uytterhoeven Cc: Phil Edworthy Cc: Simon Horman Cc: Wolfram Sang Cc: linux-renesas-soc@vger.kernel.org --- V2: Reword the commit message --- drivers/pci/host/pcie-rcar.c | 52 ++-- 1 file changed, 26 insertions(+), 26 deletions(-) diff --git a/drivers/pci/host/pcie-rcar.c b/drivers/pci/host/pcie-rcar.c index 25f68305322c..5ab7bf6a8de0 100644 --- a/drivers/pci/host/pcie-rcar.c +++ b/drivers/pci/host/pcie-rcar.c @@ -30,9 +30,9 @@ >>> >>> missing this: >>> >>> #include >> >> This compiles just fine without it though, which means it's probably >> pulled in through some of the other includes already present. >> > > I suspected that. However, please see rule #1 in > Documentation/process/submit-checklist.rst: > > 1) If you use a facility then #include the file that defines/declares >that facility. Don't depend on other header files pulling in ones >that you use. If I was to include every single header from which I use something, wouldn't that make the list real long then ? Is that really worth it? -- Best regards, Marek Vasut
Re: [PATCH V2] PCI: rcar: Clean up the macros
On 04/08/2018 09:34 AM, Marek Vasut wrote: > On 04/08/2018 05:27 PM, Randy Dunlap wrote: >> On 04/08/2018 06:09 AM, Marek Vasut wrote: >>> This patch replaces the (1 << n) with BIT(n) and cleans up whitespace, >>> no functional change. >>> >>> Signed-off-by: Marek Vasut >>> Cc: Geert Uytterhoeven >>> Cc: Phil Edworthy >>> Cc: Simon Horman >>> Cc: Wolfram Sang >>> Cc: linux-renesas-soc@vger.kernel.org >>> --- >>> V2: Reword the commit message >>> --- >>> drivers/pci/host/pcie-rcar.c | 52 >>> ++-- >>> 1 file changed, 26 insertions(+), 26 deletions(-) >>> >>> diff --git a/drivers/pci/host/pcie-rcar.c b/drivers/pci/host/pcie-rcar.c >>> index 25f68305322c..5ab7bf6a8de0 100644 >>> --- a/drivers/pci/host/pcie-rcar.c >>> +++ b/drivers/pci/host/pcie-rcar.c >>> @@ -30,9 +30,9 @@ >> >> missing this: >> >> #include > > This compiles just fine without it though, which means it's probably > pulled in through some of the other includes already present. > I suspected that. However, please see rule #1 in Documentation/process/submit-checklist.rst: 1) If you use a facility then #include the file that defines/declares that facility. Don't depend on other header files pulling in ones that you use. -- ~Randy
Re: [PATCH V2] PCI: rcar: Clean up the macros
On 04/08/2018 05:27 PM, Randy Dunlap wrote: > On 04/08/2018 06:09 AM, Marek Vasut wrote: >> This patch replaces the (1 << n) with BIT(n) and cleans up whitespace, >> no functional change. >> >> Signed-off-by: Marek Vasut >> Cc: Geert Uytterhoeven >> Cc: Phil Edworthy >> Cc: Simon Horman >> Cc: Wolfram Sang >> Cc: linux-renesas-soc@vger.kernel.org >> --- >> V2: Reword the commit message >> --- >> drivers/pci/host/pcie-rcar.c | 52 >> ++-- >> 1 file changed, 26 insertions(+), 26 deletions(-) >> >> diff --git a/drivers/pci/host/pcie-rcar.c b/drivers/pci/host/pcie-rcar.c >> index 25f68305322c..5ab7bf6a8de0 100644 >> --- a/drivers/pci/host/pcie-rcar.c >> +++ b/drivers/pci/host/pcie-rcar.c >> @@ -30,9 +30,9 @@ > > missing this: > > #include This compiles just fine without it though, which means it's probably pulled in through some of the other includes already present. -- Best regards, Marek Vasut
Re: [PATCH V2] PCI: rcar: Clean up the macros
On 04/08/2018 06:09 AM, Marek Vasut wrote: > This patch replaces the (1 << n) with BIT(n) and cleans up whitespace, > no functional change. > > Signed-off-by: Marek Vasut > Cc: Geert Uytterhoeven > Cc: Phil Edworthy > Cc: Simon Horman > Cc: Wolfram Sang > Cc: linux-renesas-soc@vger.kernel.org > --- > V2: Reword the commit message > --- > drivers/pci/host/pcie-rcar.c | 52 > ++-- > 1 file changed, 26 insertions(+), 26 deletions(-) > > diff --git a/drivers/pci/host/pcie-rcar.c b/drivers/pci/host/pcie-rcar.c > index 25f68305322c..5ab7bf6a8de0 100644 > --- a/drivers/pci/host/pcie-rcar.c > +++ b/drivers/pci/host/pcie-rcar.c > @@ -30,9 +30,9 @@ missing this: #include > #define PCIECAR 0x10 > #define PCIECCTLR0x18 > -#define CONFIG_SEND_ENABLE (1 << 31) > +#define CONFIG_SEND_ENABLE BIT(31) > #define TYPE0 (0 << 8) > -#define TYPE1 (1 << 8) > +#define TYPE1 BIT(8) > #define PCIECDR 0x20 > #define PCIEMSR 0x28 > #define PCIEINTXR0x000400 > @@ -44,7 +44,7 @@ > #define PCIETSTR 0x02004 > #define DATA_LINK_ACTIVE1 > #define PCIEERRFR0x02020 > -#define UNSUPPORTED_REQUEST (1 << 4) > +#define UNSUPPORTED_REQUEST BIT(4) > #define PCIEMSIFR0x02044 > #define PCIEMSIALR 0x02048 > #define MSIFE 1 > @@ -57,17 +57,17 @@ > /* local address reg & mask */ > #define PCIELAR(x) (0x02200 + ((x) * 0x20)) > #define PCIELAMR(x) (0x02208 + ((x) * 0x20)) > -#define LAM_PREFETCH(1 << 3) > -#define LAM_64BIT (1 << 2) > -#define LAR_ENABLE (1 << 1) > +#define LAM_PREFETCHBIT(3) > +#define LAM_64BIT BIT(2) > +#define LAR_ENABLE BIT(1) > > /* PCIe address reg & mask */ > #define PCIEPALR(x) (0x03400 + ((x) * 0x20)) > #define PCIEPAUR(x) (0x03404 + ((x) * 0x20)) > #define PCIEPAMR(x) (0x03408 + ((x) * 0x20)) > #define PCIEPTCTLR(x)(0x0340c + ((x) * 0x20)) > -#define PAR_ENABLE (1 << 31) > -#define IO_SPACE(1 << 8) > +#define PAR_ENABLE BIT(31) > +#define IO_SPACEBIT(8) -- ~Randy
