Re: [PATCH v6] bcma: register bcma as device tree driver

[Arnd Bergmann <arnd@arndb.de>]

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@hauke-m.de>

I'm basically happy with this patch, so

Acked-by: Arnd Bergmann <arnd@arndb.de>

A few details that I think are worth improving:

> 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