[RESEND PATCH 2/3] PCI: ARM: add support for generic PCI host controller

[arnd@arndb.de (Arnd Bergmann)]

On Tuesday 06 May 2014 19:38:12 Will Deacon wrote:
> Ok, I've respun the patch and included it below. It's turned probe() into a
> bit of a beast, but the cleanup is a lot simpler (unless I'm missing
> something).

Let's see if we can make it a bit more readable. None of these are important,
but together they could improve readability.

> +static int gen_pci_probe(struct platform_device *pdev)
> +{
> +       struct gen_pci *pci;
> +       int err, res_valid;
> +       struct pci_host_bridge_window *win;
> +       struct resource *bus_range;
> +       resource_size_t busn;
> +       const char *type;
> +       struct of_pci_range range;
> +       struct of_pci_range_parser parser;
> +       struct hw_pci hw;
> +       const struct of_device_id *of_id;
> +       const int *prop;
> +       u8 bus_max;
> +       struct device *dev = &pdev->dev;
> +       struct device_node *np = dev->of_node;
> +
> +       if (!dev->of_node)
> +               return -ENODEV;

This check can go away: you don't get here without an of node.

> +       pci = devm_kzalloc(dev, sizeof(*pci), GFP_KERNEL);
> +       if (!pci)
> +               return -ENOMEM;
> +
> +       pci->host.dev.parent = dev;
> +       hw = (struct hw_pci) {
> +               .nr_controllers = 1,
> +               .private_data   = (void **)&pci,
> +               .setup          = gen_pci_setup,
> +               .map_irq        = of_irq_parse_and_map_pci,
> +               .ops            = &gen_pci_ops,
> +       };

C constructors are actually a gcc extension, I think it would be more
common to turn this into an initializer by moving it to the declaration:

	struct gen_pci *pci = devm_kzalloc(dev, sizeof(*pci), GFP_KERNEL);
	struct hw_pci hw = {
               .nr_controllers = 1,
               .private_data   = (void **)&pci,
               .setup          = gen_pci_setup,
               .map_irq        = of_irq_parse_and_map_pci,
               .ops            = &gen_pci_ops,
       };


> +       if (of_pci_range_parser_init(&parser, np)) {
> +               dev_err(dev, "missing \"ranges\" property\n");
> +               return -EINVAL;
> +       }

If you move everything related to the range parser into a separate function,
you end up with two more compact parts. 

> +       /* Create and register resources from the ranges property */
> +       res_valid = 0;
> +       for_each_of_pci_range(&parser, &range) {
> +               struct resource *parent, *res;
> +               resource_size_t offset;
> +               u32 restype = range.flags & IORESOURCE_TYPE_BITS;

You could also move the body of the for loop into another function, or
both.

	Arnd