[1/3] PCI: rcar: Replace unsigned long with u32 for register values
diff mbox series

Message ID 20190317000608.24881-1-marek.vasut@gmail.com
State Superseded
Delegated to: Lorenzo Pieralisi
Headers show
Series
  • [1/3] PCI: rcar: Replace unsigned long with u32 for register values
Related show

Commit Message

Marek Vasut March 17, 2019, 12:06 a.m. UTC
From: Marek Vasut <marek.vasut+renesas@gmail.com>

Replace unsigned long with u32 type for variables holding
register values, since the registers are 32bit. Note that
rcar_pcie_msi_irq() still uses unsigned long because both
find_first_bit() and __fls() require unsigned long as an
argument.

Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
Cc: Geert Uytterhoeven <geert+renesas@glider.be>
Cc: Phil Edworthy <phil.edworthy@renesas.com>
Cc: Simon Horman <horms+renesas@verge.net.au>
Cc: Wolfram Sang <wsa@the-dreams.de>
Cc: linux-renesas-soc@vger.kernel.org
To: linux-pci@vger.kernel.org
---
 drivers/pci/controller/pcie-rcar.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

Comments

Wolfram Sang March 17, 2019, 9:09 a.m. UTC | #1
On Sun, Mar 17, 2019 at 01:06:06AM +0100, marek.vasut@gmail.com wrote:
> From: Marek Vasut <marek.vasut+renesas@gmail.com>
> 
> Replace unsigned long with u32 type for variables holding

s/unsigned long/various variable types/

> register values, since the registers are 32bit. Note that
> rcar_pcie_msi_irq() still uses unsigned long because both
> find_first_bit() and __fls() require unsigned long as an
> argument.
> 
> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
...

> -	int shift = 8 * (where & 3);
> +	u32 shift = 8 * (where & 3);

Minor nit: Since this is about shifting, maybe replace 8 with << 3 while
we are here?

There is also a 'shift' var in rcar_pcie_write_conf(). I think we should
convert this for consistency, too?
Marek Vasut March 17, 2019, 10:58 p.m. UTC | #2
On 3/17/19 10:09 AM, Wolfram Sang wrote:
> On Sun, Mar 17, 2019 at 01:06:06AM +0100, marek.vasut@gmail.com wrote:
>> From: Marek Vasut <marek.vasut+renesas@gmail.com>
>>
>> Replace unsigned long with u32 type for variables holding
> 
> s/unsigned long/various variable types/

Fixed

>> register values, since the registers are 32bit. Note that
>> rcar_pcie_msi_irq() still uses unsigned long because both
>> find_first_bit() and __fls() require unsigned long as an
>> argument.
>>
>> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
> ...
> 
>> -	int shift = 8 * (where & 3);
>> +	u32 shift = 8 * (where & 3);
> 
> Minor nit: Since this is about shifting, maybe replace 8 with << 3 while
> we are here?
> 
> There is also a 'shift' var in rcar_pcie_write_conf(). I think we should
> convert this for consistency, too?

OK, I might as well collect this and the other cleanup series and repost
it together as a V2 to make it easier to pick.
Wolfram Sang March 18, 2019, 7:33 a.m. UTC | #3
> > There is also a 'shift' var in rcar_pcie_write_conf(). I think we should
> > convert this for consistency, too?
> 
> OK, I might as well collect this and the other cleanup series and repost
> it together as a V2 to make it easier to pick.

Sounds good!
Geert Uytterhoeven March 18, 2019, 8:47 a.m. UTC | #4
Hi Marek,

On Sun, Mar 17, 2019 at 1:06 AM <marek.vasut@gmail.com> wrote:
> From: Marek Vasut <marek.vasut+renesas@gmail.com>
>
> Replace unsigned long with u32 type for variables holding
> register values, since the registers are 32bit. Note that
> rcar_pcie_msi_irq() still uses unsigned long because both
> find_first_bit() and __fls() require unsigned long as an
> argument.
>
> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
> Cc: Geert Uytterhoeven <geert+renesas@glider.be>
> Cc: Phil Edworthy <phil.edworthy@renesas.com>
> Cc: Simon Horman <horms+renesas@verge.net.au>
> Cc: Wolfram Sang <wsa@the-dreams.de>
> Cc: linux-renesas-soc@vger.kernel.org
> To: linux-pci@vger.kernel.org
> ---
>  drivers/pci/controller/pcie-rcar.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/pci/controller/pcie-rcar.c b/drivers/pci/controller/pcie-rcar.c
> index 1408c8aa758b..857d88fd03d5 100644
> --- a/drivers/pci/controller/pcie-rcar.c
> +++ b/drivers/pci/controller/pcie-rcar.c
> @@ -169,7 +169,7 @@ enum {
>
>  static void rcar_rmw32(struct rcar_pcie *pcie, int where, u32 mask, u32 data)
>  {
> -       int shift = 8 * (where & 3);
> +       u32 shift = 8 * (where & 3);

shift is not a register value, so IMHO the original type is fine (the "int"
comes from the pci_ops API, BTW).

>         u32 val = rcar_pci_read_reg(pcie, where & ~3);
>
>         val &= ~(mask << shift);
> @@ -179,7 +179,7 @@ static void rcar_rmw32(struct rcar_pcie *pcie, int where, u32 mask, u32 data)
>
>  static u32 rcar_read_conf(struct rcar_pcie *pcie, int where)
>  {
> -       int shift = 8 * (where & 3);
> +       u32 shift = 8 * (where & 3);

Likewise

>         u32 val = rcar_pci_read_reg(pcie, where & ~3);
>
>         return val >> shift;
> @@ -190,7 +190,7 @@ static int rcar_pcie_config_access(struct rcar_pcie *pcie,
>                 unsigned char access_type, struct pci_bus *bus,
>                 unsigned int devfn, int where, u32 *data)
>  {
> -       int dev, func, reg, index;
> +       u32 dev, func, reg, index;

reg is the only register value, isn't it?

>
>         dev = PCI_SLOT(devfn);
>         func = PCI_FUNC(devfn);
> @@ -508,7 +508,7 @@ static void phy_write_reg(struct rcar_pcie *pcie,
>                                  unsigned int rate, unsigned int addr,
>                                  unsigned int lane, unsigned int data)
>  {
> -       unsigned long phyaddr;
> +       u32 phyaddr;
>
>         phyaddr = WRITE_CMD |
>                 ((rate & 1) << RATE_POS) |
> @@ -1116,7 +1116,7 @@ static int rcar_pcie_probe(struct platform_device *pdev)
>  {
>         struct device *dev = &pdev->dev;
>         struct rcar_pcie *pcie;
> -       unsigned int data;
> +       u32 data;
>         int err;
>         int (*phy_init_fn)(struct rcar_pcie *);
>         struct pci_host_bridge *bridge;

For the last two changes:
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert
Marek Vasut March 21, 2019, 3:25 a.m. UTC | #5
On 3/18/19 9:47 AM, Geert Uytterhoeven wrote:
> Hi Marek,
> 
> On Sun, Mar 17, 2019 at 1:06 AM <marek.vasut@gmail.com> wrote:
>> From: Marek Vasut <marek.vasut+renesas@gmail.com>
>>
>> Replace unsigned long with u32 type for variables holding
>> register values, since the registers are 32bit. Note that
>> rcar_pcie_msi_irq() still uses unsigned long because both
>> find_first_bit() and __fls() require unsigned long as an
>> argument.
>>
>> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
>> Cc: Geert Uytterhoeven <geert+renesas@glider.be>
>> Cc: Phil Edworthy <phil.edworthy@renesas.com>
>> Cc: Simon Horman <horms+renesas@verge.net.au>
>> Cc: Wolfram Sang <wsa@the-dreams.de>
>> Cc: linux-renesas-soc@vger.kernel.org
>> To: linux-pci@vger.kernel.org
>> ---
>>  drivers/pci/controller/pcie-rcar.c | 10 +++++-----
>>  1 file changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/pci/controller/pcie-rcar.c b/drivers/pci/controller/pcie-rcar.c
>> index 1408c8aa758b..857d88fd03d5 100644
>> --- a/drivers/pci/controller/pcie-rcar.c
>> +++ b/drivers/pci/controller/pcie-rcar.c
>> @@ -169,7 +169,7 @@ enum {
>>
>>  static void rcar_rmw32(struct rcar_pcie *pcie, int where, u32 mask, u32 data)
>>  {
>> -       int shift = 8 * (where & 3);
>> +       u32 shift = 8 * (where & 3);
> 
> shift is not a register value, so IMHO the original type is fine (the "int"
> comes from the pci_ops API, BTW).

I presume it should be at least unsigned ?

[...]
Geert Uytterhoeven March 21, 2019, 7:31 a.m. UTC | #6
Hi Marek,

On Thu, Mar 21, 2019 at 5:14 AM Marek Vasut <marek.vasut@gmail.com> wrote:
> On 3/18/19 9:47 AM, Geert Uytterhoeven wrote:
> > On Sun, Mar 17, 2019 at 1:06 AM <marek.vasut@gmail.com> wrote:
> >> From: Marek Vasut <marek.vasut+renesas@gmail.com>
> >>
> >> Replace unsigned long with u32 type for variables holding
> >> register values, since the registers are 32bit. Note that
> >> rcar_pcie_msi_irq() still uses unsigned long because both
> >> find_first_bit() and __fls() require unsigned long as an
> >> argument.
> >>
> >> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
> >> Cc: Geert Uytterhoeven <geert+renesas@glider.be>
> >> Cc: Phil Edworthy <phil.edworthy@renesas.com>
> >> Cc: Simon Horman <horms+renesas@verge.net.au>
> >> Cc: Wolfram Sang <wsa@the-dreams.de>
> >> Cc: linux-renesas-soc@vger.kernel.org
> >> To: linux-pci@vger.kernel.org
> >> ---
> >>  drivers/pci/controller/pcie-rcar.c | 10 +++++-----
> >>  1 file changed, 5 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/drivers/pci/controller/pcie-rcar.c b/drivers/pci/controller/pcie-rcar.c
> >> index 1408c8aa758b..857d88fd03d5 100644
> >> --- a/drivers/pci/controller/pcie-rcar.c
> >> +++ b/drivers/pci/controller/pcie-rcar.c
> >> @@ -169,7 +169,7 @@ enum {
> >>
> >>  static void rcar_rmw32(struct rcar_pcie *pcie, int where, u32 mask, u32 data)
> >>  {
> >> -       int shift = 8 * (where & 3);
> >> +       u32 shift = 8 * (where & 3);
> >
> > shift is not a register value, so IMHO the original type is fine (the "int"
> > comes from the pci_ops API, BTW).
>
> I presume it should be at least unsigned ?

Yes, and "where" too.

Note that the other uses of "where" are also int, and IMHO should be
unsigned, too. But changing that means changing the PCI API and all
drivers, sigh...

Gr{oetje,eeting}s,

                        Geert

Patch
diff mbox series

diff --git a/drivers/pci/controller/pcie-rcar.c b/drivers/pci/controller/pcie-rcar.c
index 1408c8aa758b..857d88fd03d5 100644
--- a/drivers/pci/controller/pcie-rcar.c
+++ b/drivers/pci/controller/pcie-rcar.c
@@ -169,7 +169,7 @@  enum {
 
 static void rcar_rmw32(struct rcar_pcie *pcie, int where, u32 mask, u32 data)
 {
-	int shift = 8 * (where & 3);
+	u32 shift = 8 * (where & 3);
 	u32 val = rcar_pci_read_reg(pcie, where & ~3);
 
 	val &= ~(mask << shift);
@@ -179,7 +179,7 @@  static void rcar_rmw32(struct rcar_pcie *pcie, int where, u32 mask, u32 data)
 
 static u32 rcar_read_conf(struct rcar_pcie *pcie, int where)
 {
-	int shift = 8 * (where & 3);
+	u32 shift = 8 * (where & 3);
 	u32 val = rcar_pci_read_reg(pcie, where & ~3);
 
 	return val >> shift;
@@ -190,7 +190,7 @@  static int rcar_pcie_config_access(struct rcar_pcie *pcie,
 		unsigned char access_type, struct pci_bus *bus,
 		unsigned int devfn, int where, u32 *data)
 {
-	int dev, func, reg, index;
+	u32 dev, func, reg, index;
 
 	dev = PCI_SLOT(devfn);
 	func = PCI_FUNC(devfn);
@@ -508,7 +508,7 @@  static void phy_write_reg(struct rcar_pcie *pcie,
 				 unsigned int rate, unsigned int addr,
 				 unsigned int lane, unsigned int data)
 {
-	unsigned long phyaddr;
+	u32 phyaddr;
 
 	phyaddr = WRITE_CMD |
 		((rate & 1) << RATE_POS) |
@@ -1116,7 +1116,7 @@  static int rcar_pcie_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct rcar_pcie *pcie;
-	unsigned int data;
+	u32 data;
 	int err;
 	int (*phy_init_fn)(struct rcar_pcie *);
 	struct pci_host_bridge *bridge;