| Message ID | 20131008000435.12954.92136.stgit@bhelgaas-glaptop.roam.corp.google.com |
|---|---|
| State | Accepted |
| Headers | show |
On Mon, Oct 7, 2013 at 6:04 PM, Bjorn Helgaas <bhelgaas@google.com> wrote: > The following variables and functions are used only in pcie-designware.c, > so make them static: ... > diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c > index c10e9ac..900e875 100644 > --- a/drivers/pci/host/pcie-designware.c > +++ b/drivers/pci/host/pcie-designware.c > @@ -64,7 +64,7 @@ > > static struct hw_pci dw_pci; > > -unsigned long global_io_offset; > +static unsigned long global_io_offset; While you're looking at this, I think "cfg_read()" and "cfg_write()" are too generic to be global symbols. I don't know if it would make sense to rename them "dw_cfg_read()", pass around pointers in a structure or what. Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Oct 08, 2013 at 08:07:24AM +0800, Bjorn Helgaas wrote: > On Mon, Oct 7, 2013 at 6:04 PM, Bjorn Helgaas <bhelgaas@google.com> wrote: > > The following variables and functions are used only in pcie-designware.c, > > so make them static: > ... > > diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c > > index c10e9ac..900e875 100644 > > --- a/drivers/pci/host/pcie-designware.c > > +++ b/drivers/pci/host/pcie-designware.c > > @@ -64,7 +64,7 @@ > > > > static struct hw_pci dw_pci; > > > > -unsigned long global_io_offset; > > +static unsigned long global_io_offset; > > While you're looking at this, I think "cfg_read()" and "cfg_write()" > are too generic to be global symbols. I don't know if it would make > sense to rename them "dw_cfg_read()", pass around pointers in a > structure or what. In fact cfg_read, cfg_write & sys_to_pcie should go to common library, as you will find similar function in other files too. Regards Pratyush > > Bjorn > -- > To unsubscribe from this list: send the line "unsubscribe linux-pci" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday, October 08, 2013 3:07 PM, Pratyush Anand wrote: > On Tue, Oct 08, 2013 at 08:07:24AM +0800, Bjorn Helgaas wrote: > > On Mon, Oct 7, 2013 at 6:04 PM, Bjorn Helgaas <bhelgaas@google.com> wrote: > > > The following variables and functions are used only in pcie-designware.c, > > > so make them static: > > ... > > > diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c > > > index c10e9ac..900e875 100644 > > > --- a/drivers/pci/host/pcie-designware.c > > > +++ b/drivers/pci/host/pcie-designware.c > > > @@ -64,7 +64,7 @@ > > > > > > static struct hw_pci dw_pci; > > > > > > -unsigned long global_io_offset; > > > +static unsigned long global_io_offset; > > > > While you're looking at this, I think "cfg_read()" and "cfg_write()" > > are too generic to be global symbols. I don't know if it would make > > sense to rename them "dw_cfg_read()", pass around pointers in a > > structure or what. > > In fact cfg_read, cfg_write & sys_to_pcie should go to common library, > as you will find similar function in other files too. > (+CC each PCIe host controller maintainer) Yes, you're right. I think so, too. :-) cfg_read, cfg_write & sys_to_pcie can go to common library. I looked at pcie-designware.c, pci-tegra.c, & pci-mvebu.c. These drivers can use common cfg_read(), cfg_write(), and sys_to_pcie(). For example, common cfg_read() can be used as below: If there is no objection, I will send the patch. However, I cannot decide which file is a good place for the common cfg_read(), cfg_write(), and sys_to_pcie(). int cfg_read(void __iomem *addr, int where, int size, u32 *val) { *val = readl(addr); if (size == 1) *val = (*val >> (8 * (where & 3))) & 0xff; else if (size == 2) *val = (*val >> (8 * (where & 3))) & 0xffff; else if (size != 4) return PCIBIOS_BAD_REGISTER_NUMBER; return PCIBIOS_SUCCESSFUL; } ./drivers/pci/host/pci-mvebu.c @@ -243,14 +243,7 @@ static int mvebu_pcie_hw_rd_conf(struct mvebu_pcie_port *port, writel(PCIE_CONF_ADDR(bus->number, devfn, where), port->base + PCIE_CONF_ADDR_OFF); - *val = readl(port->base + PCIE_CONF_DATA_OFF); - - if (size == 1) - *val = (*val >> (8 * (where & 3))) & 0xff; - else if (size == 2) - *val = (*val >> (8 * (where & 3))) & 0xffff; - - return PCIBIOS_SUCCESSFUL; + return cfg_read(port->base + PCIE_CONF_DATA_OFF, where, size, val); } ./drivers/pci/host/pci-tegra.c @@ -462,14 +462,7 @@ static int tegra_pcie_read_conf(struct pci_bus *bus, unsigned int devfn, return PCIBIOS_DEVICE_NOT_FOUND; } - *value = readl(addr); - - if (size == 1) - *value = (*value >> (8 * (where & 3))) & 0xff; - else if (size == 2) - *value = (*value >> (8 * (where & 3))) & 0xffff; - - return PCIBIOS_SUCCESSFUL; + return cfg_read(addr, where, size, val); } Best regards, Jingoo Han -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday, October 08, 2013 9:05 AM, Bjorn Helgaas wrote: > > The following variables and functions are used only in pcie-designware.c, > so make them static: > > global_io_offset > dw_pcie_rd_own_conf() > dw_pcie_wr_own_conf() > dw_pcie_setup() > dw_pcie_scan_bus() > dw_pcie_map_irq() > > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> Acked-by: Jingoo Han <jg1.han@samsung.com> It looks good. Also I tested this patch on Exynos5440. It works properly. Thank you for sending the patch. Best regards, Jingoo Han > --- > drivers/pci/host/pcie-designware.c | 16 ++++++++-------- > drivers/pci/host/pcie-designware.h | 7 ------- > 2 files changed, 8 insertions(+), 15 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Dear Jingoo Han, On Wed, 09 Oct 2013 10:33:30 +0900, Jingoo Han wrote: > ./drivers/pci/host/pci-mvebu.c > @@ -243,14 +243,7 @@ static int mvebu_pcie_hw_rd_conf(struct mvebu_pcie_port *port, > writel(PCIE_CONF_ADDR(bus->number, devfn, where), > port->base + PCIE_CONF_ADDR_OFF); > > - *val = readl(port->base + PCIE_CONF_DATA_OFF); > - > - if (size == 1) > - *val = (*val >> (8 * (where & 3))) & 0xff; > - else if (size == 2) > - *val = (*val >> (8 * (where & 3))) & 0xffff; > - > - return PCIBIOS_SUCCESSFUL; > + return cfg_read(port->base + PCIE_CONF_DATA_OFF, where, size, val); > } The cfg_read() name looks too generic to me to be globally exported in the kernel, but provided a more specific name is used, I'm fine with using that in the PCIe mvebu driver. Thanks! Thomas
On Wed, Oct 9, 2013 at 1:50 AM, Jingoo Han <jg1.han@samsung.com> wrote: > On Tuesday, October 08, 2013 9:05 AM, Bjorn Helgaas wrote: >> >> The following variables and functions are used only in pcie-designware.c, >> so make them static: >> >> global_io_offset >> dw_pcie_rd_own_conf() >> dw_pcie_wr_own_conf() >> dw_pcie_setup() >> dw_pcie_scan_bus() >> dw_pcie_map_irq() >> >> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> > > Acked-by: Jingoo Han <jg1.han@samsung.com> > > It looks good. > Also I tested this patch on Exynos5440. It works properly. > Thank you for sending the patch. I applied this to my pci/host-designware branch for v3.13. Thanks! >> --- >> drivers/pci/host/pcie-designware.c | 16 ++++++++-------- >> drivers/pci/host/pcie-designware.h | 7 ------- >> 2 files changed, 8 insertions(+), 15 deletions(-) > -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Oct 9, 2013 at 9:00 AM, Bjorn Helgaas <bhelgaas@google.com> wrote: > On Wed, Oct 9, 2013 at 1:50 AM, Jingoo Han <jg1.han@samsung.com> wrote: >> On Tuesday, October 08, 2013 9:05 AM, Bjorn Helgaas wrote: >>> >>> The following variables and functions are used only in pcie-designware.c, >>> so make them static: >>> >>> global_io_offset >>> dw_pcie_rd_own_conf() >>> dw_pcie_wr_own_conf() >>> dw_pcie_setup() >>> dw_pcie_scan_bus() >>> dw_pcie_map_irq() >>> >>> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> >> >> Acked-by: Jingoo Han <jg1.han@samsung.com> >> >> It looks good. >> Also I tested this patch on Exynos5440. It works properly. >> Thank you for sending the patch. > > I applied this to my pci/host-designware branch for v3.13. Thanks! Correction: I moved this to pci/host-exynos because it's tangled up with the MSI support there. -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c index c10e9ac..900e875 100644 --- a/drivers/pci/host/pcie-designware.c +++ b/drivers/pci/host/pcie-designware.c @@ -64,7 +64,7 @@ static struct hw_pci dw_pci; -unsigned long global_io_offset; +static unsigned long global_io_offset; static inline struct pcie_port *sys_to_pcie(struct pci_sys_data *sys) { @@ -115,8 +115,8 @@ static inline void dw_pcie_writel_rc(struct pcie_port *pp, u32 val, u32 reg) writel(val, pp->dbi_base + reg); } -int dw_pcie_rd_own_conf(struct pcie_port *pp, int where, int size, - u32 *val) +static int dw_pcie_rd_own_conf(struct pcie_port *pp, int where, int size, + u32 *val) { int ret; @@ -128,8 +128,8 @@ int dw_pcie_rd_own_conf(struct pcie_port *pp, int where, int size, return ret; } -int dw_pcie_wr_own_conf(struct pcie_port *pp, int where, int size, - u32 val) +static int dw_pcie_wr_own_conf(struct pcie_port *pp, int where, int size, + u32 val) { int ret; @@ -438,7 +438,7 @@ static struct pci_ops dw_pcie_ops = { .write = dw_pcie_wr_conf, }; -int dw_pcie_setup(int nr, struct pci_sys_data *sys) +static int dw_pcie_setup(int nr, struct pci_sys_data *sys) { struct pcie_port *pp; @@ -461,7 +461,7 @@ int dw_pcie_setup(int nr, struct pci_sys_data *sys) return 1; } -struct pci_bus *dw_pcie_scan_bus(int nr, struct pci_sys_data *sys) +static struct pci_bus *dw_pcie_scan_bus(int nr, struct pci_sys_data *sys) { struct pci_bus *bus; struct pcie_port *pp = sys_to_pcie(sys); @@ -478,7 +478,7 @@ struct pci_bus *dw_pcie_scan_bus(int nr, struct pci_sys_data *sys) return bus; } -int dw_pcie_map_irq(const struct pci_dev *dev, u8 slot, u8 pin) +static int dw_pcie_map_irq(const struct pci_dev *dev, u8 slot, u8 pin) { struct pcie_port *pp = sys_to_pcie(dev->bus->sysdata); diff --git a/drivers/pci/host/pcie-designware.h b/drivers/pci/host/pcie-designware.h index 133820f..401b154 100644 --- a/drivers/pci/host/pcie-designware.h +++ b/drivers/pci/host/pcie-designware.h @@ -51,15 +51,8 @@ struct pcie_host_ops { void (*host_init)(struct pcie_port *pp); }; -extern unsigned long global_io_offset; - int cfg_read(void __iomem *addr, int where, int size, u32 *val); int cfg_write(void __iomem *addr, int where, int size, u32 val); -int dw_pcie_wr_own_conf(struct pcie_port *pp, int where, int size, u32 val); -int dw_pcie_rd_own_conf(struct pcie_port *pp, int where, int size, u32 *val); int dw_pcie_link_up(struct pcie_port *pp); void dw_pcie_setup_rc(struct pcie_port *pp); int dw_pcie_host_init(struct pcie_port *pp); -int dw_pcie_setup(int nr, struct pci_sys_data *sys); -struct pci_bus *dw_pcie_scan_bus(int nr, struct pci_sys_data *sys); -int dw_pcie_map_irq(const struct pci_dev *dev, u8 slot, u8 pin);
The following variables and functions are used only in pcie-designware.c, so make them static: global_io_offset dw_pcie_rd_own_conf() dw_pcie_wr_own_conf() dw_pcie_setup() dw_pcie_scan_bus() dw_pcie_map_irq() Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> --- drivers/pci/host/pcie-designware.c | 16 ++++++++-------- drivers/pci/host/pcie-designware.h | 7 ------- 2 files changed, 8 insertions(+), 15 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html