From mboxrd@z Thu Jan  1 00:00:00 1970
From: linux@arm.linux.org.uk (Russell King - ARM Linux)
Date: Fri, 29 Apr 2011 10:08:09 +0100
Subject: [PATCH v3 02/23] at91: Make Ethernet device common
In-Reply-To: <1304045972-19319-3-git-send-email-ryan@bluewatersys.com>
References: <1304045972-19319-1-git-send-email-ryan@bluewatersys.com>
	<1304045972-19319-3-git-send-email-ryan@bluewatersys.com>
Message-ID: <20110429090809.GY17290@n2100.arm.linux.org.uk>
To: linux-arm-kernel@lists.infradead.org
List-Id: linux-arm-kernel.lists.infradead.org

On Fri, Apr 29, 2011 at 02:59:11PM +1200, Ryan Mallon wrote:
> +#if defined(CONFIG_MACB) || defined(CONFIG_MACB_MODULE)
> +static u64 eth_dmamask = DMA_BIT_MASK(32);
> +static struct at91_eth_data eth_data;
> +
> +static struct resource eth_resources[] = {
> +	[0] = {
> +		.end	= SZ_16K,
> +		.flags	= IORESOURCE_MEM,
> +	},
> +	[1] = {
> +		.flags	= IORESOURCE_IRQ,
> +	},
> +};
> +
> +static struct platform_device at91_eth_device = {
> +	.name		= "macb",
> +	.id		= -1,
> +	.dev		= {
> +				.dma_mask		= &eth_dmamask,
> +				.coherent_dma_mask	= DMA_BIT_MASK(32),
> +				.platform_data		= &eth_data,

These three lines have twice as many tabs as is needed.

> +	},
> +	.resource	= eth_resources,
> +	.num_resources	= ARRAY_SIZE(eth_resources),
> +};
> +
> +void __init at91_add_device_eth(struct at91_eth_data *data)
> +{
> +	struct at91_dev_table_ethernet *info = devices->ethernet;
> +
> +	BUG_ON(!info);
> +	init_resource_mem(&eth_resources[0], info->mmio_base);
> +	init_resource_irq(&eth_resources[1], info->irq);

Hmm.  How about something like this instead:

	struct resource eth_res[2];
	int err;

	BUG_ON(!info);

	init_resource_mem(&eth_res[0], info->mmio_base, SZ_16K);
	init_resource_irq(&eth_res[1], info->irq);

	...

	err = platform_device_add_resources(&at91_eth_device, eth_res, 2);
	if (err) {
		pr_err("eth: unable to add resources: %d\n", err);
		return;
	}

	err = platform_device_add_data(&at91_eth_device, data, sizeof(*data));
	if (err) {
		pr_err("eth: unable to add platform data: %d\n", err);
		/* res leaked */
		return;
	}

	err = platform_device_register(&at91_eth_device);
	if (err) {
		pr_err("eth: unable to register device: %d\n", err);
		/* res and data leaked */
	}

With Uwe's patches, we can plug those leaks.  Note that we can't use
platform_device_register_resndata() because it doesn't set the DMA
masks etc (which is one of the problems I've always had with interfaces
which try to wrap too much stuff up - they always lack something.)

This would get rid of the need to have the resources and platform
data storage declared for each and every device.  I'd suggest wrapping
the last bit adding the platform data and resources up in its own
function common to each device as its going to be the same for
everything.  Remember to pass the size of the platform data though.