Re: [PATCH v6] bcma: register bcma as device tree driver
From: Arnd Bergmann <hidden>
Date: 2014-09-30 13:40:10
Also in:
linux-arm-kernel, linux-wireless
On Friday 26 September 2014 00:09:19 Hauke Mehrtens wrote:
This driver is used by the bcm53xx ARM SoC code. Now it is possible to give the address of the chipcommon core in device tree and bcma will search for all the other cores. Signed-off-by: Hauke Mehrtens <hauke-5/S+JYg5SzeELgA04lAiVw@public.gmane.org>
I'm basically happy with this patch, so Acked-by: Arnd Bergmann <redacted> A few details that I think are worth improving:
quoted hunk
diff --git a/Documentation/devicetree/bindings/bus/bcma.txt b/Documentation/devicetree/bindings/bus/bcma.txt new file mode 100644 index 0000000..e9070c1 --- /dev/null +++ b/Documentation/devicetree/bindings/bus/bcma.txt@@ -0,0 +1,20 @@ +Driver for ARM AXI Bus with Broadcom Plugins (bcma) + +Required properties: + +- compatible : brcm,bus-axi + +- reg : iomem address range of chipcommon core + +The cores on the AXI bus are automatically detected by bcma with the +memory ranges they are using and they get registered afterwards. + +Example: + + axi@18000000 { + compatible = "brcm,bus-axi"; + reg = <0x18000000 0x1000>; + ranges = <0x00000000 0x18000000 0x00100000>; + #address-cells = <1>; + #size-cells = <1>; + };
I think it would be good to document how you can specify child nodes here, and give one node as the example.
+
+#ifdef CONFIG_OF
+static int bcma_host_soc_probe(struct platform_device *pdev)
+{We generally try to avoid #ifdef in device drivers, I think it would be trivial to avoid this one by moving the contents into a new file.
+ struct device *dev = &pdev->dev; + struct device_node *np = dev->of_node; + struct bcma_bus *bus; + int err; + + /* Alloc */ + bus = devm_kzalloc(dev, sizeof(*bus), GFP_KERNEL); + if (!bus) + return -ENOMEM; + + /* Map MMIO */ + bus->mmio = of_iomap(np, 0); + if (!bus->mmio) + return -ENOMEM;
this could use devm_ioremap_resource() to avoid the iounmap in the failu
+#ifdef CONFIG_OF
+static struct device_node *bcma_of_find_child_device(struct platform_device *parent,
+ struct bcma_device *core)
+{
+ struct device_node *node;
+ u64 size;
+ const __be32 *reg;
+
+ if (!parent || !parent->dev.of_node)
+ return NULL;
+
+ for_each_child_of_node(parent->dev.of_node, node) {
+ reg = of_get_address(node, 0, &size, NULL);
+ if (!reg)
+ continue;
+ if (of_translate_address(node, reg) == core->addr)
+ return node;
+ }
+ return NULL;
+}I think this will compile into nothing if CONFIG_OF is disabled, so no need for the #ifdef.
+
+static void bcma_of_fill_device(struct platform_device *parent,
+ struct bcma_device *core)
+{
+ struct device_node *node;
+
+ node = bcma_of_find_child_device(parent, core);
+ if (node)
+ core->dev.of_node = node;
+}and consequently, this function can be moved into the caller. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html