| Message ID | 20140205065254.29445.77939.sendpatchset@w520 (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
On Wed, Feb 5, 2014 at 7:52 AM, Magnus Damm <magnus.damm@gmail.com> wrote: > +static int rcar_pci_probe(struct platform_device *pdev) > { > struct resource *cfg_res, *mem_res; > struct rcar_pci_priv *priv; > void __iomem *reg; > + struct hw_pci hw; > > cfg_res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > reg = devm_ioremap_resource(&pdev->dev, cfg_res); > @@ -308,31 +271,26 @@ static int __init rcar_pci_probe(struct > priv->reg = reg; > priv->dev = &pdev->dev; > > - return rcar_pci_add_controller(priv); > + memset(&hw, 0, sizeof(hw)); > + hw.nr_controllers = 1; > + hw.private_data = (void **)&priv; This raised a red flag: taking the address of a variable on the stack. I know it's correct, as hw.private_data is an array of pointers copied by pci_common_init_dev() below. But perhaps it's a good idea to turn priv into an array with one element, to make this clearer, and avoid the ugly cast? > + hw.map_irq = rcar_pci_map_irq; > + hw.ops = &rcar_pci_ops; > + hw.setup = rcar_pci_setup; > + pci_common_init_dev(&pdev->dev, &hw); > + return 0; Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds -- 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 05/02/14 06:52, Magnus Damm wrote: > From: Magnus Damm <damm@opensource.se> > > Convert the code to allow per-device probe() like other device > drivers. This also delays driver registration due to change from > subsys_initcall() to regular module_platform_driver(). > > Signed-off-by: Magnus Damm <damm@opensource.se> > --- > > drivers/pci/host/pci-rcar-gen2.c | 74 ++++++++------------------------------ > 1 file changed, 16 insertions(+), 58 deletions(-) > > --- 0001/drivers/pci/host/pci-rcar-gen2.c > +++ work/drivers/pci/host/pci-rcar-gen2.c 2014-02-04 16:38:46.000000000 +0900 > @@ -74,9 +74,6 @@ > > #define RCAR_PCI_UNIT_REV_REG (RCAR_AHBPCI_PCICOM_OFFSET + 0x48) > > -/* Number of internal PCI controllers */ > -#define RCAR_PCI_NR_CONTROLLERS 3 > - > struct rcar_pci_priv { > struct device *dev; > void __iomem *reg; > @@ -228,6 +225,8 @@ static int __init rcar_pci_setup(int nr, > pci_add_resource(&sys->resources, &priv->io_res); > pci_add_resource(&sys->resources, &priv->mem_res); > > + /* Setup bus number based on platform device id */ > + sys->busnr = to_platform_device(priv->dev)->id; > return 1; > } > > @@ -236,48 +235,12 @@ static struct pci_ops rcar_pci_ops = { > .write = rcar_pci_write_config, > }; > > -static struct hw_pci rcar_hw_pci __initdata = { > - .map_irq = rcar_pci_map_irq, > - .ops = &rcar_pci_ops, > - .setup = rcar_pci_setup, > -}; > - > -static int rcar_pci_count __initdata; > - > -static int __init rcar_pci_add_controller(struct rcar_pci_priv *priv) > -{ > - void **private_data; > - int count; > - > - if (rcar_hw_pci.nr_controllers < rcar_pci_count) > - goto add_priv; > - > - /* (Re)allocate private data pointer array if needed */ > - count = rcar_pci_count + RCAR_PCI_NR_CONTROLLERS; > - private_data = kzalloc(count * sizeof(void *), GFP_KERNEL); > - if (!private_data) > - return -ENOMEM; > - > - rcar_pci_count = count; > - if (rcar_hw_pci.private_data) { > - memcpy(private_data, rcar_hw_pci.private_data, > - rcar_hw_pci.nr_controllers * sizeof(void *)); > - kfree(rcar_hw_pci.private_data); > - } > - > - rcar_hw_pci.private_data = private_data; > - > -add_priv: > - /* Add private data pointer to the array */ > - rcar_hw_pci.private_data[rcar_hw_pci.nr_controllers++] = priv; > - return 0; > -} > - > -static int __init rcar_pci_probe(struct platform_device *pdev) > +static int rcar_pci_probe(struct platform_device *pdev) > { > struct resource *cfg_res, *mem_res; > struct rcar_pci_priv *priv; > void __iomem *reg; > + struct hw_pci hw; > > cfg_res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > reg = devm_ioremap_resource(&pdev->dev, cfg_res); > @@ -308,31 +271,26 @@ static int __init rcar_pci_probe(struct > priv->reg = reg; > priv->dev = &pdev->dev; > > - return rcar_pci_add_controller(priv); > + memset(&hw, 0, sizeof(hw)); > + hw.nr_controllers = 1; > + hw.private_data = (void **)&priv; > + hw.map_irq = rcar_pci_map_irq; > + hw.ops = &rcar_pci_ops; > + hw.setup = rcar_pci_setup; > + pci_common_init_dev(&pdev->dev, &hw); > + return 0; > } Do we really want to explicitly set the bus number here? > > static struct platform_driver rcar_pci_driver = { > .driver = { > .name = "pci-rcar-gen2", > + .owner = THIS_MODULE, > + .suppress_bind_attrs = true, > }, > + .probe = rcar_pci_probe, > };
On Wed, Feb 5, 2014 at 5:02 PM, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > On Wed, Feb 5, 2014 at 7:52 AM, Magnus Damm <magnus.damm@gmail.com> wrote: >> +static int rcar_pci_probe(struct platform_device *pdev) >> { >> struct resource *cfg_res, *mem_res; >> struct rcar_pci_priv *priv; >> void __iomem *reg; >> + struct hw_pci hw; >> >> cfg_res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >> reg = devm_ioremap_resource(&pdev->dev, cfg_res); >> @@ -308,31 +271,26 @@ static int __init rcar_pci_probe(struct >> priv->reg = reg; >> priv->dev = &pdev->dev; >> >> - return rcar_pci_add_controller(priv); >> + memset(&hw, 0, sizeof(hw)); >> + hw.nr_controllers = 1; >> + hw.private_data = (void **)&priv; > > This raised a red flag: taking the address of a variable on the stack. > I know it's correct, as hw.private_data is an array of pointers copied > by pci_common_init_dev() below. > But perhaps it's a good idea to turn priv into an array with one element, > to make this clearer, and avoid the ugly cast? I simply followed the same style as pci-tegra.c, but I agree that it can be made prettier. Also, there may be some better way how to register independent instances, not sure. Thanks, / magnus -- 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, Feb 5, 2014 at 5:29 PM, Ben Dooks <ben.dooks@codethink.co.uk> wrote: > On 05/02/14 06:52, Magnus Damm wrote: >> >> From: Magnus Damm <damm@opensource.se> >> >> Convert the code to allow per-device probe() like other device >> drivers. This also delays driver registration due to change from >> subsys_initcall() to regular module_platform_driver(). >> >> Signed-off-by: Magnus Damm <damm@opensource.se> >> --- >> >> drivers/pci/host/pci-rcar-gen2.c | 74 >> ++++++++------------------------------ >> 1 file changed, 16 insertions(+), 58 deletions(-) >> >> --- 0001/drivers/pci/host/pci-rcar-gen2.c >> +++ work/drivers/pci/host/pci-rcar-gen2.c 2014-02-04 >> 16:38:46.000000000 +0900 >> @@ -74,9 +74,6 @@ >> >> #define RCAR_PCI_UNIT_REV_REG (RCAR_AHBPCI_PCICOM_OFFSET + 0x48) >> >> -/* Number of internal PCI controllers */ >> -#define RCAR_PCI_NR_CONTROLLERS 3 >> - >> struct rcar_pci_priv { >> struct device *dev; >> void __iomem *reg; >> @@ -228,6 +225,8 @@ static int __init rcar_pci_setup(int nr, >> pci_add_resource(&sys->resources, &priv->io_res); >> pci_add_resource(&sys->resources, &priv->mem_res); >> >> + /* Setup bus number based on platform device id */ >> + sys->busnr = to_platform_device(priv->dev)->id; >> return 1; >> } >> >> @@ -236,48 +235,12 @@ static struct pci_ops rcar_pci_ops = { >> .write = rcar_pci_write_config, >> }; >> >> -static struct hw_pci rcar_hw_pci __initdata = { >> - .map_irq = rcar_pci_map_irq, >> - .ops = &rcar_pci_ops, >> - .setup = rcar_pci_setup, >> -}; >> - >> -static int rcar_pci_count __initdata; >> - >> -static int __init rcar_pci_add_controller(struct rcar_pci_priv *priv) >> -{ >> - void **private_data; >> - int count; >> - >> - if (rcar_hw_pci.nr_controllers < rcar_pci_count) >> - goto add_priv; >> - >> - /* (Re)allocate private data pointer array if needed */ >> - count = rcar_pci_count + RCAR_PCI_NR_CONTROLLERS; >> - private_data = kzalloc(count * sizeof(void *), GFP_KERNEL); >> - if (!private_data) >> - return -ENOMEM; >> - >> - rcar_pci_count = count; >> - if (rcar_hw_pci.private_data) { >> - memcpy(private_data, rcar_hw_pci.private_data, >> - rcar_hw_pci.nr_controllers * sizeof(void *)); >> - kfree(rcar_hw_pci.private_data); >> - } >> - >> - rcar_hw_pci.private_data = private_data; >> - >> -add_priv: >> - /* Add private data pointer to the array */ >> - rcar_hw_pci.private_data[rcar_hw_pci.nr_controllers++] = priv; >> - return 0; >> -} >> - >> -static int __init rcar_pci_probe(struct platform_device *pdev) >> +static int rcar_pci_probe(struct platform_device *pdev) >> { >> struct resource *cfg_res, *mem_res; >> struct rcar_pci_priv *priv; >> void __iomem *reg; >> + struct hw_pci hw; >> >> cfg_res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >> reg = devm_ioremap_resource(&pdev->dev, cfg_res); >> @@ -308,31 +271,26 @@ static int __init rcar_pci_probe(struct >> priv->reg = reg; >> priv->dev = &pdev->dev; >> >> - return rcar_pci_add_controller(priv); >> + memset(&hw, 0, sizeof(hw)); >> + hw.nr_controllers = 1; >> + hw.private_data = (void **)&priv; >> + hw.map_irq = rcar_pci_map_irq; >> + hw.ops = &rcar_pci_ops; >> + hw.setup = rcar_pci_setup; >> + pci_common_init_dev(&pdev->dev, &hw); >> + return 0; >> } > > > Do we really want to explicitly set the bus number here? Right now the code in rcar_pci_setup() sets up the bus number. Ugly but better than before IMO. If you have any suggestions how to make this prettier then please let me know. =) / magnus -- 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
--- 0001/drivers/pci/host/pci-rcar-gen2.c +++ work/drivers/pci/host/pci-rcar-gen2.c 2014-02-04 16:38:46.000000000 +0900 @@ -74,9 +74,6 @@ #define RCAR_PCI_UNIT_REV_REG (RCAR_AHBPCI_PCICOM_OFFSET + 0x48) -/* Number of internal PCI controllers */ -#define RCAR_PCI_NR_CONTROLLERS 3 - struct rcar_pci_priv { struct device *dev; void __iomem *reg; @@ -228,6 +225,8 @@ static int __init rcar_pci_setup(int nr, pci_add_resource(&sys->resources, &priv->io_res); pci_add_resource(&sys->resources, &priv->mem_res); + /* Setup bus number based on platform device id */ + sys->busnr = to_platform_device(priv->dev)->id; return 1; } @@ -236,48 +235,12 @@ static struct pci_ops rcar_pci_ops = { .write = rcar_pci_write_config, }; -static struct hw_pci rcar_hw_pci __initdata = { - .map_irq = rcar_pci_map_irq, - .ops = &rcar_pci_ops, - .setup = rcar_pci_setup, -}; - -static int rcar_pci_count __initdata; - -static int __init rcar_pci_add_controller(struct rcar_pci_priv *priv) -{ - void **private_data; - int count; - - if (rcar_hw_pci.nr_controllers < rcar_pci_count) - goto add_priv; - - /* (Re)allocate private data pointer array if needed */ - count = rcar_pci_count + RCAR_PCI_NR_CONTROLLERS; - private_data = kzalloc(count * sizeof(void *), GFP_KERNEL); - if (!private_data) - return -ENOMEM; - - rcar_pci_count = count; - if (rcar_hw_pci.private_data) { - memcpy(private_data, rcar_hw_pci.private_data, - rcar_hw_pci.nr_controllers * sizeof(void *)); - kfree(rcar_hw_pci.private_data); - } - - rcar_hw_pci.private_data = private_data; - -add_priv: - /* Add private data pointer to the array */ - rcar_hw_pci.private_data[rcar_hw_pci.nr_controllers++] = priv; - return 0; -} - -static int __init rcar_pci_probe(struct platform_device *pdev) +static int rcar_pci_probe(struct platform_device *pdev) { struct resource *cfg_res, *mem_res; struct rcar_pci_priv *priv; void __iomem *reg; + struct hw_pci hw; cfg_res = platform_get_resource(pdev, IORESOURCE_MEM, 0); reg = devm_ioremap_resource(&pdev->dev, cfg_res); @@ -308,31 +271,26 @@ static int __init rcar_pci_probe(struct priv->reg = reg; priv->dev = &pdev->dev; - return rcar_pci_add_controller(priv); + memset(&hw, 0, sizeof(hw)); + hw.nr_controllers = 1; + hw.private_data = (void **)&priv; + hw.map_irq = rcar_pci_map_irq; + hw.ops = &rcar_pci_ops; + hw.setup = rcar_pci_setup; + pci_common_init_dev(&pdev->dev, &hw); + return 0; } static struct platform_driver rcar_pci_driver = { .driver = { .name = "pci-rcar-gen2", + .owner = THIS_MODULE, + .suppress_bind_attrs = true, }, + .probe = rcar_pci_probe, }; -static int __init rcar_pci_init(void) -{ - int retval; - - retval = platform_driver_probe(&rcar_pci_driver, rcar_pci_probe); - if (!retval) - pci_common_init(&rcar_hw_pci); - - /* Private data pointer array is not needed any more */ - kfree(rcar_hw_pci.private_data); - rcar_hw_pci.private_data = NULL; - - return retval; -} - -subsys_initcall(rcar_pci_init); +module_platform_driver(rcar_pci_driver); MODULE_LICENSE("GPL v2"); MODULE_DESCRIPTION("Renesas R-Car Gen2 internal PCI");