Re: [PATCH V2] PCI: rcar: Clean up the macros

2018-04-08 Thread Marek Vasut
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

2018-04-08 Thread Randy Dunlap
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

2018-04-08 Thread Niklas Söderlund
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

2018-04-08 Thread Marek Vasut
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

2018-04-08 Thread Randy Dunlap
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

2018-04-08 Thread Marek Vasut
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

2018-04-08 Thread Randy Dunlap
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