| 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 | expand |
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?
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.
> > 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!
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
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 ? [...]
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
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;