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
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
