Re: [PATCH 02/10] goldfish: add the goldfish virtual bus

[Arnd Bergmann <arnd@arndb.de>]

On Wednesday 09 January 2013, Alan Cox wrote:
> From: Jun Nakajima <jnakajim@gmail.com>
> 
> This imports the current Google code and cleans it up slightly to use pr_ and
> to properly request its resources.
> 
> Goldfish is an emulator used for Android development. It has a virtual bus where
> the emulator passes platform device information to the guest which then creates
> the appropriate devices.
> 
> This part of the emulation is not architecture specific so should not be hiding
> in architecture trees as it does in the Google Android tree. The constants it
> uses do depend on the platform and so live in arch/goldfish.h

Ah, quite nice.

>  drivers/platform/Makefile            |    1 
>  drivers/platform/goldfish/Makefile   |    4 +
>  drivers/platform/goldfish/pdev_bus.c |  265 ++++++++++++++++++++++++++++++++++
>  3 files changed, 270 insertions(+)

Maybe drivers/bus would be more appropriate though. Every platform
handles platforms differently, and x86 seems to be the only one that
likes the model of putting stuff under drivers/platform.

> +static int __devexit goldfish_pdev_bus_remove(struct platform_device *pdev)
> +{
> +	free_irq(pdev_bus_irq, pdev);
> +	release_region(pdev_bus_addr, GOLDFISH_IO_SIZE);
> +	return 0;
> +}
> +
> +static struct platform_driver goldfish_pdev_bus_driver = {
> +	.probe = goldfish_pdev_bus_probe,
> +	.remove = __devexit_p(goldfish_pdev_bus_remove),
> +	.driver = {
> +		.name = "goldfish_pdev_bus"
> +	}
> +};

__devinit/__devexit are going away, so you can skip adding them
for new code.

> +
> +static int __init goldfish_pdev_bus_init(void)
> +{
> +	return platform_driver_register(&goldfish_pdev_bus_driver);
> +}
> +
> +static void __exit goldfish_pdev_bus_exit(void)
> +{
> +	platform_driver_unregister(&goldfish_pdev_bus_driver);
> +}
> +
> +module_init(goldfish_pdev_bus_init);
> +module_exit(goldfish_pdev_bus_exit);

The module_platform_driver() macro takes care of this.

> +static struct resource goldfish_pdev_bus_resources[] = {
> +	{
> +		.start  = GOLDFISH_PDEV_BUS_BASE,
> +		.end    = GOLDFISH_PDEV_BUS_BASE + GOLDFISH_PDEV_BUS_END - 1,
> +		.flags  = IORESOURCE_IO,
> +	},
> +	{
> +		.start	= GOLDFISH_PDEV_BUS_IRQ,
> +		.end	= GOLDFISH_PDEV_BUS_IRQ,
> +		.flags	= IORESOURCE_IRQ,
> +	}
> +};
> +
> +struct platform_device goldfish_pdev_bus_device = {
> +	.name = "goldfish_pdev_bus",
> +	.id = -1,
> +	.num_resources = ARRAY_SIZE(goldfish_pdev_bus_resources),
> +	.resource = goldfish_pdev_bus_resources
> +};
> +
> +static int __init goldfish_init(void)
> +{
> +	return platform_device_register(&goldfish_pdev_bus_device);
> +}
> +device_initcall(goldfish_init);

This is the part that I think should actually be part of the architecture
tree. That way, no architecture specific header files will be necessary,
and it's possible to create the platform device differently, e.g. from
devicetree on architectures that support it, or from ACPI.

Note that statically declaring a platform device is not recommended any
more, a call to platform_device_register_simple() makes this code
simpler and smaller, and allows proper reference counting on the device.

	Arnd