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